Desktop: Phase 4 (batch) broker owns queue + autonomy - #77
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughA new shared ChangesQueue and Autonomy Endpoints (Full Stack)
Sequence Diagram(s)sequenceDiagram
participant Client
participant Broker as desktop/broker.js
participant EngineAction as scripts/engine_action.py
participant SopWrites as scripts/sop_writes.py
rect rgba(100, 149, 237, 0.5)
Note over Client,SopWrites: /api/autonomy flow
Client->>Broker: POST /api/autonomy (token, id, level)
Broker->>Broker: token-gate via ACTION_PATHS
Broker->>EngineAction: spawn autonomy --sop_dir --id --level
EngineAction->>SopWrites: set_autonomy(sop_dir, sop_id, level)
SopWrites->>SopWrites: drift check + atomic os.replace
SopWrites-->>EngineAction: {id, autonomy} or raises
EngineAction-->>Broker: JSON + exit code
Broker-->>Client: HTTP 200 / 4xx
end
rect rgba(144, 238, 144, 0.5)
Note over Client,EngineAction: /api/queue flow
Client->>Broker: POST /api/queue (token, id, inputs, scope)
Broker->>Broker: token-gate via ACTION_PATHS
Broker->>EngineAction: spawn queue --sop_dir --id --scope
EngineAction->>EngineAction: lib.queue_run(...)
EngineAction-->>Broker: JSON {run_id, project} + exit code
Broker-->>Client: HTTP 200 / 4xx
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 43f83dc759
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return { argv, input: inputs } | ||
| } | ||
| case '/api/queue': { | ||
| const argv = ['queue', sopDir, sanitizeId(body.id), '--scope=' + String(body.scope || 'here'), '--launch-cwd=' + process.cwd()] |
There was a problem hiding this comment.
When the broker owns POST /api/queue, this now records the Electron/broker process cwd as --launch-cwd, but the queue semantics depend on the dashboard server's launch cwd: queue_run writes that value into the queue file's project: field, and serve_dashboard.launch(... kind="queue") later opens the Claude session in that folder. If the desktop app is started from Finder/an app bundle, or simply from a different shell than the FastAPI dashboard, “Queue here” will be persisted with an unrelated directory and the queued run will launch in the wrong project.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch, fixed. The broker no longer passes its own process.cwd() (an Electron app dir, maybe /) as --launch-cwd -- that would persist an unrelated directory into the queue file's project: and launch the run there. Now it passes --launch-cwd only when the desktop app sets $SMBOS_LAUNCH_CWD (the real launch folder); otherwise it omits it, so the engine uses None and a folder-less SOP simply gets no project (a declared-folder SOP still gets its own folder). Verified: a folder-less SOP queued via the broker now persists project="" instead of the broker cwd.
|
Adversarial self-review: clean on the trust-critical refactor. Verified line-by-line that sop_writes.set_autonomy + _write_autonomy is a verbatim move of the original inline gate + _write_autonomy (same order: level-validate before find_sop; same sid regex; same status gate; same is_drifted-before-write + always-re-stamp + atomic mkstemp/os.replace). Exception mapping matches the original on both callers (BadLevel->400, UnknownSop->404, DraftNotAllowed/SopDrifted->409, OSError/ValueError->500). The drift/re-stamp security property holds (and the dashboard tests that assert it are unchanged + green). Two P3s:
|
07b615d to
caa40af
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/dashboard_app.py (1)
606-611: ⚡ Quick winChain the mapped
HTTPExceptionraises.Ruff B904 flags these raises inside
exceptblocks;from exckeeps the original autonomy refusal available for diagnostics and clears the warning.Proposed fix
except sop_writes.BadLevel as exc: - raise HTTPException(status_code=400, detail=str(exc)) + raise HTTPException(status_code=400, detail=str(exc)) from exc except sop_writes.UnknownSop as exc: - raise HTTPException(status_code=404, detail=str(exc)) + raise HTTPException(status_code=404, detail=str(exc)) from exc except (sop_writes.DraftNotAllowed, sop_writes.SopDrifted) as exc: - raise HTTPException(status_code=409, detail=str(exc)) + raise HTTPException(status_code=409, detail=str(exc)) from exc🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/dashboard_app.py` around lines 606 - 611, Chain the exception context in all three HTTPException raises within the except blocks to comply with Ruff B904. In the except blocks for sop_writes.BadLevel, sop_writes.UnknownSop, and the combined except block for sop_writes.DraftNotAllowed and sop_writes.SopDrifted, add "from exc" to each raise statement so that the original exception is chained and diagnostics are preserved while clearing the Ruff warning.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/engine_action.py`:
- Around line 43-48: The exception handlers for sop_writes.BadLevel,
sop_writes.UnknownSop, and the combined handler for sop_writes.DraftNotAllowed
and sop_writes.SopDrifted all use semicolons to combine the print statement and
return statement on single lines, which violates the Ruff E702 rule. Split each
of these three exception blocks so that the print statement and return statement
are on separate lines instead of being joined by a semicolon, maintaining the
same exit code values for each exception type.
---
Nitpick comments:
In `@scripts/dashboard_app.py`:
- Around line 606-611: Chain the exception context in all three HTTPException
raises within the except blocks to comply with Ruff B904. In the except blocks
for sop_writes.BadLevel, sop_writes.UnknownSop, and the combined except block
for sop_writes.DraftNotAllowed and sop_writes.SopDrifted, add "from exc" to each
raise statement so that the original exception is chained and diagnostics are
preserved while clearing the Ruff warning.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ca3960ee-5ad8-416c-9591-2c7afd6ef33b
📒 Files selected for processing (6)
desktop/broker.jsdesktop/broker.test.jsscripts/dashboard_app.pyscripts/engine_action.pyscripts/sop_writes.pytests/test_engine_action.py
🚧 Files skipped from review as they are similar to previous changes (3)
- desktop/broker.test.js
- scripts/sop_writes.py
- desktop/broker.js
Two more action endpoints off FastAPI: - POST /api/queue enqueue a run for later (lib.queue_run) - POST /api/autonomy set the per-procedure autonomy dial (gate + fingerprint re-stamp) The autonomy write is the trust-critical one (the security work from the dial PR), so its gate + re-stamp/drift logic is MOVED to a shared stdlib module scripts/sop_writes.py (set_autonomy raising typed exceptions: BadLevel/UnknownSop/DraftNotAllowed/SopDrifted). dashboard_app's /api/autonomy endpoint now calls set_autonomy too (mapping the typed exceptions to the same HTTP codes/messages), so the app + the engine gate + write through ONE implementation -- never re-implemented in Node, no divergence. The dashboard suite is unchanged (78), confirming parity through the refactor. - engine_action: queue (lib.queue_run) + autonomy (sop_writes.set_autonomy) subcommands, mapping to the existing exit-code -> HTTP convention (8->400, 4->404, 9->409, 0->200). - broker: /api/queue + /api/autonomy added to the action dispatcher; a shared sanitizeId for the positional id; queue passes --launch-cwd=process.cwd() for FastAPI parity. Verified end-to-end against the real engine (autonomy on_its_own -> 200 persisted; the bad-level/unknown/draft refusals; queue -> 200; no token -> 401) and by tests: engine subcommands + the broker dispatch/exit-code map. 289 stdlib, 78 dashboard, 35 node. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
caa40af to
2db70ff
Compare
1 participant
Next Phase 4 batch: two more action endpoints off FastAPI.
POST /api/queuelib.queue_run)POST /api/autonomyThe autonomy refactor (trust-critical, shared)
Autonomy is the security-sensitive one (the dial PR's fingerprint/drift protection). Its gate + re-stamp/drift logic is moved to a shared stdlib module
scripts/sop_writes.py(set_autonomyraising typed exceptions:BadLevel/UnknownSop/DraftNotAllowed/SopDrifted).dashboard_app's/api/autonomynow callsset_autonomytoo, mapping the typed exceptions to the same HTTP codes/messages — so the app and the engine gate + write through one implementation, never re-implemented in Node. The dashboard suite is unchanged (78), confirming parity through the refactor.Engine + broker
engine_action:queue+autonomysubcommands, mapping to the existing exit-code→HTTP convention (8→400, 4→404, 9→409, 0→200).sanitizeIdfor the positional id;queuepasses--launch-cwd=process.cwd()for FastAPI parity.Verification
End-to-end against the real engine (autonomy
on_its_own→ 200 persisted; bad-level/unknown/draft refusals; queue → 200; no token → 401) and tests: engine subcommands + the broker dispatch/exit-code map. 289 stdlib, 78 dashboard, 35 node.Remaining Phase 4
The osascript launch family (
launch/launch-sop/open-session/apply-item) andsettings. Then FastAPI is removable.🤖 Generated with Claude Code
Summary by CodeRabbit
/api/queueto queue SOP runs with optional stdin inputs and launch working directory settings./api/autonomyto set procedure autonomy levels, including validation and consistent drift handling shared across components./api/autonomyendpoint to delegate autonomy persistence and return standardized error codes.idhandling for/api/runby slug-sanitizing it when constructing argv.queue/autonomy, including token-gating, invalid inputs, unknown/draft SOPs, drift behavior, and persistence checks.