chore: address coderabbit cleanup notes
This commit is contained in:
@@ -150,6 +150,9 @@ implementation state.
|
||||
catalog snapshots are not mutated. Config-shadowed add is rejected in v1.
|
||||
Remove requires `--confirm` in CLI; local/static servers report
|
||||
unavailable.
|
||||
- Cleanup candidate: consolidate store/source registry id validation patterns
|
||||
(`SOURCE_REGISTRY_ID_PATTERN`, `STORE_ID_PATTERN`) only after another package
|
||||
needs the same rule. Today they intentionally stay close to their stores.
|
||||
- Longer term: make the MCP frontend an adapter over these neutral workflow,
|
||||
source-admin, and config-admin surfaces so the old `wf_mcp` server entry
|
||||
point can shrink or retire.
|
||||
|
||||
@@ -2,6 +2,16 @@
|
||||
|
||||
> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking.
|
||||
|
||||
### Target Selection Precedence
|
||||
|
||||
1. `--url` CLI override selects an RPC HTTP target.
|
||||
2. `--local` CLI override selects the in-process local target.
|
||||
3. Config file `client.target` selects the configured target.
|
||||
4. Missing target config defaults to local.
|
||||
|
||||
CLI overrides intentionally win over config so one-off diagnostics can point at
|
||||
a different server without editing the config file.
|
||||
|
||||
**Goal:** Add neutral workflow config models and let selected `wf` CLI commands target either local execution or the JSON-RPC HTTP server.
|
||||
|
||||
**Architecture:** Introduce `wf_config` as the protocol-neutral config package. Keep existing `wf_mcp.config.json` loading for compatibility, but add a new `wf.json`-style shape with `client.target`, `server.store`, `server.transports`, and bootstrap `server.sources`. Put the JSON-RPC client adapter in `wf_transport_rpc_http.client`; CLI context chooses local `WorkflowApi` or remote RPC adapter based on config plus CLI overrides.
|
||||
|
||||
@@ -0,0 +1,220 @@
|
||||
# CodeRabbit Cleanup Follow-Ups Implementation Plan
|
||||
|
||||
> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking.
|
||||
|
||||
**Goal:** Resolve non-blocking cleanup findings from the June 4 CodeRabbit review without mixing broad refactors into the focused correctness patch.
|
||||
|
||||
**Architecture:** Keep behavior unchanged unless a task explicitly says otherwise. Prefer documentation, small helpers, and narrowly scoped tests over cross-package refactors. Do not move modules or rename public APIs in this follow-up.
|
||||
|
||||
**Tech Stack:** Python 3.14, Pydantic v2, Typer, FastAPI JSON-RPC, pytest, ruff, basedpyright.
|
||||
|
||||
---
|
||||
|
||||
## Scope
|
||||
|
||||
This plan intentionally excludes the already-fixed review items:
|
||||
|
||||
- CLI config exception tuple syntax in `src/wf_cli/context.py`
|
||||
- CLI source-registry file JSON object validation and exception chaining
|
||||
- RPC method module structure-test coverage
|
||||
- stale long-lived API and CLI/API alignment doc statuses
|
||||
- typed `tmp_path` annotations in `tests/wf_config/test_config_models.py`
|
||||
|
||||
---
|
||||
|
||||
### Task 1: Document Source-Registry Mutation Asymmetry
|
||||
|
||||
**Files:**
|
||||
- Modify: `docs/superpowers/plans/2026-06-04-source-registry-mutations.md`
|
||||
|
||||
- [ ] **Step 1: Add rationale under shadow handling**
|
||||
|
||||
Find the section mentioning:
|
||||
|
||||
```markdown
|
||||
A future `allow_shadow` flag can relax add; do not add it in this slice.
|
||||
```
|
||||
|
||||
Append:
|
||||
|
||||
```markdown
|
||||
Rationale: `add` rejects config-shadowed ids to prevent silent no-ops: adding a
|
||||
registry entry that cannot activate while config owns the same id. Existing
|
||||
shadowed registry entries can still be updated, enabled, disabled, or removed so
|
||||
operators can prepare store state for config removal or future `seed` ownership
|
||||
policy.
|
||||
```
|
||||
|
||||
- [ ] **Step 2: Verify doc diff**
|
||||
|
||||
Run:
|
||||
|
||||
```bash
|
||||
git diff -- docs/superpowers/plans/2026-06-04-source-registry-mutations.md
|
||||
```
|
||||
|
||||
Expected: only the rationale paragraph changed.
|
||||
|
||||
---
|
||||
|
||||
### Task 2: Clarify RPC Target Selection Precedence
|
||||
|
||||
**Files:**
|
||||
- Modify: `docs/superpowers/plans/2026-06-03-workflow-config-and-rpc-cli-target.md`
|
||||
|
||||
- [ ] **Step 1: Add precedence table near the plan introduction**
|
||||
|
||||
Add:
|
||||
|
||||
```markdown
|
||||
### Target Selection Precedence
|
||||
|
||||
1. `--url` CLI override selects an RPC HTTP target.
|
||||
2. `--local` CLI override selects the in-process local target.
|
||||
3. Config file `client.target` selects the configured target.
|
||||
4. Missing target config defaults to local.
|
||||
|
||||
CLI overrides intentionally win over config so one-off diagnostics can point at
|
||||
a different server without editing the config file.
|
||||
```
|
||||
|
||||
- [ ] **Step 2: Verify no code references are changed**
|
||||
|
||||
Run:
|
||||
|
||||
```bash
|
||||
git diff --stat
|
||||
```
|
||||
|
||||
Expected: only the plan document is changed by this task.
|
||||
|
||||
---
|
||||
|
||||
### Task 3: Extract Source-Registry RPC Availability Helper
|
||||
|
||||
**Files:**
|
||||
- Modify: `src/wf_transport_rpc_http/methods_source_registry.py`
|
||||
- Test: `tests/wf_transport_rpc_http/test_source_registry_rpc.py`
|
||||
|
||||
- [ ] **Step 1: Add a helper**
|
||||
|
||||
In `src/wf_transport_rpc_http/methods_source_registry.py`, add:
|
||||
|
||||
```python
|
||||
def _require_source_registry_admin(
|
||||
server: WorkflowServer,
|
||||
*,
|
||||
operation: str,
|
||||
) -> WorkflowSourceRegistrySurface:
|
||||
admin = server.source_registry_admin
|
||||
if admin is None:
|
||||
raise WorkflowRpcError(
|
||||
data={
|
||||
"code": "source_registry_unavailable",
|
||||
"message": (
|
||||
f"source registry admin {operation} are not available "
|
||||
"for this server"
|
||||
),
|
||||
}
|
||||
)
|
||||
return admin
|
||||
```
|
||||
|
||||
Import `WorkflowSourceRegistrySurface` from `wf_api` if needed.
|
||||
|
||||
- [ ] **Step 2: Replace repeated `None` checks**
|
||||
|
||||
Use:
|
||||
|
||||
```python
|
||||
admin = _require_source_registry_admin(server, operation="reads")
|
||||
```
|
||||
|
||||
for list/inspect, and:
|
||||
|
||||
```python
|
||||
admin = _require_source_registry_admin(server, operation="mutations")
|
||||
```
|
||||
|
||||
for add/update/enable/disable/remove.
|
||||
|
||||
- [ ] **Step 3: Run source-registry RPC tests**
|
||||
|
||||
Run:
|
||||
|
||||
```bash
|
||||
uv run pytest tests/wf_transport_rpc_http/test_source_registry_rpc.py -q
|
||||
```
|
||||
|
||||
Expected: all tests pass.
|
||||
|
||||
---
|
||||
|
||||
### Task 4: Clarify Chronological Event Ordering
|
||||
|
||||
**Files:**
|
||||
- Modify: `src/wf_api/admin.py`
|
||||
- Test: existing admin API tests if present
|
||||
|
||||
- [ ] **Step 1: Add a comment above `list_events`**
|
||||
|
||||
Add a short comment/docstring note near `WorkflowAdminApi.list_events`:
|
||||
|
||||
```python
|
||||
# Preserve provider order for events; event providers are expected to return
|
||||
# chronological order and callers may rely on that ordering for diagnostics.
|
||||
```
|
||||
|
||||
Do not sort event payloads in this task.
|
||||
|
||||
- [ ] **Step 2: Run admin API tests**
|
||||
|
||||
Run:
|
||||
|
||||
```bash
|
||||
uv run pytest tests/wf_api -q
|
||||
```
|
||||
|
||||
Expected: all `wf_api` tests pass.
|
||||
|
||||
---
|
||||
|
||||
### Task 5: Record Shared ID Pattern Follow-Up
|
||||
|
||||
**Files:**
|
||||
- Modify: `docs/current_roadmap.md`
|
||||
|
||||
- [ ] **Step 1: Add a small platform cleanup bullet**
|
||||
|
||||
Under the platform cleanup/architecture section, add:
|
||||
|
||||
```markdown
|
||||
- Cleanup candidate: consolidate store/source registry id validation patterns
|
||||
(`SOURCE_REGISTRY_ID_PATTERN`, `STORE_ID_PATTERN`) only after another package
|
||||
needs the same rule. Today they intentionally stay close to their stores.
|
||||
```
|
||||
|
||||
- [ ] **Step 2: Verify docs only**
|
||||
|
||||
Run:
|
||||
|
||||
```bash
|
||||
git diff -- docs/current_roadmap.md
|
||||
```
|
||||
|
||||
Expected: only the cleanup bullet changed.
|
||||
|
||||
---
|
||||
|
||||
## Final Verification
|
||||
|
||||
Run:
|
||||
|
||||
```bash
|
||||
uv run pytest tests/wf_transport_rpc_http/test_source_registry_rpc.py tests/wf_api -q
|
||||
uv run ruff check src/wf_transport_rpc_http/methods_source_registry.py src/wf_api/admin.py docs/superpowers/plans/2026-06-04-source-registry-mutations.md docs/superpowers/plans/2026-06-03-workflow-config-and-rpc-cli-target.md docs/current_roadmap.md
|
||||
uv run basedpyright --level error src/wf_transport_rpc_http/methods_source_registry.py src/wf_api/admin.py
|
||||
git diff --check
|
||||
```
|
||||
|
||||
Expected: pytest exits 0, ruff exits 0, basedpyright exits 0, and `git diff --check` reports no whitespace errors.
|
||||
@@ -42,6 +42,12 @@ For mutation v1:
|
||||
- `update`, `enable`, `disable`, and `remove` may operate on existing registry entries even if they are currently shadowed by config.
|
||||
- A future `allow_shadow` flag can relax `add`; do not add it in this slice.
|
||||
|
||||
Rationale: `add` rejects config-shadowed ids to prevent silent no-ops: adding a
|
||||
registry entry that cannot activate while config owns the same id. Existing
|
||||
shadowed registry entries can still be updated, enabled, disabled, or removed so
|
||||
operators can prepare store state for config removal or future `seed` ownership
|
||||
policy.
|
||||
|
||||
### Full-Registry Validation
|
||||
|
||||
Every mutation must:
|
||||
|
||||
@@ -85,8 +85,11 @@ surface, or plain local CLI utilities.
|
||||
and `wf source list` / `wf source inspect`.
|
||||
- Read-only admin/config operations are now available through JSON-RPC HTTP
|
||||
and `wf admin connections`, `wf admin statuses`, and `wf admin events`.
|
||||
- Next source work is persistence for server-owned dynamic source changes.
|
||||
- Keep mutation out until the store-backed source registry is designed.
|
||||
- Source registry mutations (`add` / `update` / `enable` / `disable` /
|
||||
`remove`) are now implemented for server-owned dynamic source changes.
|
||||
- Remaining source-registry work is migration policy: config can bootstrap
|
||||
or lock sources, while the store-backed registry owns mutable desired
|
||||
state for dynamic sources.
|
||||
|
||||
2. **Mutable source/admin commands**
|
||||
- Config can bootstrap sources, but server-owned dynamic source changes
|
||||
|
||||
@@ -2,7 +2,12 @@
|
||||
|
||||
Date: 2026-06-03
|
||||
|
||||
Status: design spec; implementation not started
|
||||
Status: Slices 1-3 implemented. `wf_server` provides
|
||||
`build_local_static_workflow_server`, `wf_transport_rpc_http` provides JSON-RPC
|
||||
methods and client support, `wf_cli` has target-aware context, and `wf_config`
|
||||
owns neutral config models. WebSocket transport, source providers, auth,
|
||||
streaming/progress, database backend, and live MCP source management remain
|
||||
future work.
|
||||
|
||||
Related:
|
||||
|
||||
@@ -92,12 +97,14 @@ It should prove:
|
||||
|
||||
Implementation status:
|
||||
|
||||
- `wf_server.build_local_static_workflow_server()` constructs a durable
|
||||
- Slice 1 complete: `wf_server.build_local_static_workflow_server()` constructs a durable
|
||||
`WorkflowApi` with required file-backed stores, local `wf.std`/`wf.recipes`
|
||||
sources, and a local runtime runner.
|
||||
- This first slice has no transport adapter. Clients still call the in-process
|
||||
`WorkflowApi` in tests; HTTP/JSON-RPC/WebSocket/MCP transport adapters are
|
||||
later slices.
|
||||
- Slice 2 complete: `wf_transport_rpc_http` provides JSON-RPC 2.0 over HTTP via
|
||||
`create_rpc_app(server)` and the `wf-rpc-server` CLI.
|
||||
- Slice 3 complete: `wf_cli` supports target-aware context with `--local`,
|
||||
`--url`, and `--timeout` overrides, and works with remote RPC targets for
|
||||
capability and run commands.
|
||||
|
||||
First slice should not include:
|
||||
|
||||
@@ -279,10 +286,12 @@ Implementation status:
|
||||
|
||||
- `wf_transport_rpc_http.create_rpc_app(server)` exposes a fixed JSON-RPC
|
||||
method set over an existing `wf_server.WorkflowServer`.
|
||||
- `wf-rpc-server --store-root <path>` starts the local/static server over
|
||||
`/rpc`.
|
||||
- This slice still does not include remote `wf` CLI targeting, auth,
|
||||
streaming/progress, or live upstream MCP source management.
|
||||
- `wf-rpc-server --store-root <path>` and `wf-rpc-server --config <path>` start
|
||||
the local/static server over `/rpc`.
|
||||
- Remote `wf` CLI targeting is implemented through `wf_config` and target-aware
|
||||
context in `wf_cli`.
|
||||
- Auth, streaming/progress, and live upstream MCP source management remain
|
||||
future work.
|
||||
|
||||
Preferred implementation dependency:
|
||||
|
||||
|
||||
Reference in New Issue
Block a user