fix(server): surface CLI resume command in web approve/reject responses#1523
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 (2)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughApprove and reject handlers now return explicit CLI instructions ( ChangesApprove/Reject CLI hint & tests
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/server/src/routes/api.ts (1)
1151-1167: ⚡ Quick winAdd exhaustive
defaultguard to theswitchto prevent silentundefinedon future union extension.All four current members of
reasonare handled, but without adefault: neverbranch the function will returnundefinedat runtime if the union is extended — TypeScript only catches this reliably whennoImplicitReturns: trueis active in tsconfig.✅ Proposed fix
case 'dispatch_failed': return `Auto-resume dispatch failed. Run ${cliCommand} from a terminal to ${verb}.`; + default: { + // Exhaustive check — forces a compile error if the union grows without a matching case. + const _exhaustive: never = reason; + return `Run ${cliCommand} from a terminal to ${verb}.`; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server/src/routes/api.ts` around lines 1151 - 1167, The switch over the variable `reason` currently handles four cases but lacks an exhaustive `default`, which can lead to returning undefined if the `reason` union is extended; modify the switch (the block that references `reason`, `cliCommand`, and `verb`) to include a `default` branch that enforces exhaustiveness—e.g., assign `reason` to a `never`-typed variable or call a shared `assertUnreachable(reason)` helper and then throw a clear Error (or return a safe fallback message) so the compiler/runtime will catch any future unknown `reason` values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/server/src/routes/api.ts`:
- Around line 1151-1167: The switch over the variable `reason` currently handles
four cases but lacks an exhaustive `default`, which can lead to returning
undefined if the `reason` union is extended; modify the switch (the block that
references `reason`, `cliCommand`, and `verb`) to include a `default` branch
that enforces exhaustiveness—e.g., assign `reason` to a `never`-typed variable
or call a shared `assertUnreachable(reason)` helper and then throw a clear Error
(or return a safe fallback message) so the compiler/runtime will catch any
future unknown `reason` values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f57de0a2-6e16-4a15-b160-e59c374f7ec8
📒 Files selected for processing (2)
packages/server/src/routes/api.tspackages/server/src/routes/api.workflow-runs.test.ts
Review SummaryVerdict: ready-to-merge This PR replaces the boolean return from Minor / nice-to-have (2 items — both style-only, non-blocking)
Compliments
Reviewed via maintainer-review-pr workflow (Pi/Minimax). Aspects run: code-review, error-handling, test-coverage, comment-quality. |
3374fc0 to
21cd3ad
Compare
|
Rebased onto current The rebase was clean (no merge conflicts) — |
21cd3ad to
ae3a75b
Compare
When a workflow run is approved/rejected via the Web UI but `tryAutoResumeAfterGate` cannot auto-resume — because there is no `parent_conversation_id`, the parent conversation is gone, or the parent sits on a non-web platform (Slack/Telegram/GitHub/CLI) — the success message said only "Send a message to continue" / "On-reject prompt will run on resume". A web-UI user whose run originated from a terminal has no obvious next step from that text and the run sits in `failed` status. Both approve and reject (on_reject branch) now include the exact `archon workflow resume <runId>` command in the non-auto-resumed response, so the web-UI surface always carries an actionable next step. The auto-resume happy path and the no-on_reject cancellation path are unchanged. The Resume endpoint's CLI hints (covered by coleam00#1329) are not touched. Closes coleam00#1522. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
ae3a75b to
b32dce3
Compare
Summary
tryAutoResumeAfterGateskips dispatch — because there is noparent_conversation_id, the parent conversation is gone, or the parent sits on a non-web platform (Slack/Telegram/GitHub/CLI) — the success message said only"Send a message to continue"/"On-reject prompt will run on resume". A web-UI user whose run originated from a terminal has no obvious next step from that text; the run sits infailedstatus withmetadata.rejection_reasonpopulated and the user has to read the DB schema to discoverarchon workflow resume <id>exists.archon workflow resume <runId>command in the non-auto-resumed response. The auto-resume happy path is unchanged.parent_conversation_id-based cross-adapter guard, log events, DB writes, and the no-on_reject cancellation path are byte-identical. The Resume endpoint's CLI hints (/api/workflows/runs/:runId/resume) are covered by fix(server,web,workflows): web approval gates auto-resume + reject-with-reason dialog #1329 and are not touched here.Scope History
This PR was rebased after #1329 (web approval auto-resume + cross-adapter guard) and #1646 (explicit resume pipeline) merged into
dev. Both eliminated the original PR's larger surface (discriminated-union refactor oftryAutoResumeAfterGateand Resume-endpoint hint additions). Scope reduced on rebase to only the gate-time messaging delta that remains uncovered.Linked Issue
Validation Evidence
bun x prettier --check packages/server/src/routes/{api.ts,api.workflow-runs.test.ts} # All matched files use Prettier code style bun x eslint --max-warnings 0 --no-warn-ignored \ packages/server/src/routes/api.ts packages/server/src/routes/api.workflow-runs.test.ts # clean bun --filter @archon/server test packages/server/src/routes/api.workflow-runs.test.ts # 14/14 pass (one new regression test for reject + on_reject + non-web parent)The four pre-existing approve auto-resume-skipped tests had their substring assertion updated from
"Send a message to continue"to"archon workflow resume run-paused-1". One new test covers the symmetric reject case (on_reject + non-web parent).Security Impact
Compatibility / Migration
messagetext differs in the non-auto-resumed branches.Rollback Plan
🤖 Generated with Claude Code
Summary by CodeRabbit