fix(workflows): archon-assist runs in live checkout (closes #1546)#1555
fix(workflows): archon-assist runs in live checkout (closes #1546)#1555ztech-gthb wants to merge 1 commit intocoleam00:devfrom
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 as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR configures the ChangesWorktree Configuration for archon-assist
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Possibly related issues
Possibly related PRs
Poem
🚥 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![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)
packages/core/src/handlers/command-handler.test.ts (1)
1393-1401: 💤 Low valueConsider asserting
result.workflow.resumeRunIdonce the handler is aligned.
stubDiscoveredWorkflow('plan')correctly seeds discovery, but this test only validates the "nofailWorkflowRuncall" invariant. Once the handler is updated to return theworkflowfield (per the sibling test's expectations), add an assertion here forresult.workflow?.resumeRunId === 'run-456'to keep the two success-path tests symmetric.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/handlers/command-handler.test.ts` around lines 1393 - 1401, Test currently verifies no call to mockFailWorkflowRun but doesn't assert the returned workflow resumeRunId; update the test around the handleCommand('/workflow resume run-456') call to also assert that result.workflow?.resumeRunId === 'run-456'. Locate the test using stubDiscoveredWorkflow('plan') and mockGetWorkflowRun.mockResolvedValueOnce(run) and add the assertion alongside the existing expect(result.success).toBe(true) and expect(mockFailWorkflowRun).not.toHaveBeenCalled(); ensure you reference the result from handleCommand and use optional chaining as suggested (result.workflow?.resumeRunId).
🤖 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/core/src/handlers/command-handler.test.ts`:
- Around line 1393-1401: Test currently verifies no call to mockFailWorkflowRun
but doesn't assert the returned workflow resumeRunId; update the test around the
handleCommand('/workflow resume run-456') call to also assert that
result.workflow?.resumeRunId === 'run-456'. Locate the test using
stubDiscoveredWorkflow('plan') and mockGetWorkflowRun.mockResolvedValueOnce(run)
and add the assertion alongside the existing expect(result.success).toBe(true)
and expect(mockFailWorkflowRun).not.toHaveBeenCalled(); ensure you reference the
result from handleCommand and use optional chaining as suggested
(result.workflow?.resumeRunId).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 91cbd6f7-7d46-498a-8c2b-42736547f644
📒 Files selected for processing (3)
.archon/workflows/defaults/archon-assist.yamlpackages/core/src/handlers/command-handler.test.tspackages/workflows/src/defaults/bundled-defaults.generated.ts
| expect(result.message).toContain('Resuming workflow'); | ||
| expect(result.message).toContain('implement'); | ||
| // Resume now dispatches directly so the orchestrator doesn't loop | ||
| // through the V2a prompt — see PR #1551. | ||
| expect(result.workflow?.resumeRunId).toBe('run-123'); | ||
| expect(result.workflow?.definition.name).toBe('implement'); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the actual resume case return value in the handler
rg -n "resumeRunId\|Resuming workflow\|is ready to resume\|workflow:.*resumeRunId" \
packages/core/src/handlers/command-handler.ts -A 3Repository: coleam00/Archon
Length of output: 41
🏁 Script executed:
#!/bin/bash
# Confirm whether the resume case returns a workflow field at all
ast-grep --pattern $'case '"'"'resume'"'"': {
$$$
return {
$$$
};
}'Repository: coleam00/Archon
Length of output: 41
🏁 Script executed:
#!/bin/bash
# Find the command-handler file
find . -name "command-handler.ts" -o -name "command-handler.test.ts" | head -20Repository: coleam00/Archon
Length of output: 160
🏁 Script executed:
#!/bin/bash
# Check file size and read the test file around lines 1370-1375
wc -l packages/core/src/handlers/command-handler.test.tsRepository: coleam00/Archon
Length of output: 115
🏁 Script executed:
#!/bin/bash
# Read the test assertions around lines 1370-1375
sed -n '1365,1380p' packages/core/src/handlers/command-handler.test.tsRepository: coleam00/Archon
Length of output: 753
🏁 Script executed:
#!/bin/bash
# Search for "resume" case in the handler and read surrounding code
rg -n "case.*resume\|handleCommand.*resume" packages/core/src/handlers/command-handler.ts -A 20 -B 2Repository: coleam00/Archon
Length of output: 41
🏁 Script executed:
#!/bin/bash
# Also search for where result is returned with workflow field
rg -n "resumeRunId\|workflow.*resumeRunId" packages/core/src/handlers/command-handler.ts -B 5 -A 5Repository: coleam00/Archon
Length of output: 41
🏁 Script executed:
#!/bin/bash
# Get overall structure of command-handler.ts to understand the resume handling
wc -l packages/core/src/handlers/command-handler.ts
sed -n '1,100p' packages/core/src/handlers/command-handler.ts | head -80Repository: coleam00/Archon
Length of output: 3005
🏁 Script executed:
#!/bin/bash
# Search for all occurrences of "resume" in the handler (case-insensitive)
rg -in "resume" packages/core/src/handlers/command-handler.ts | head -30Repository: coleam00/Archon
Length of output: 1522
🏁 Script executed:
#!/bin/bash
# Search for where CommandResult is created/returned to see all return patterns
rg -n "return.*success.*true\|CommandResult" packages/core/src/handlers/command-handler.ts | head -40Repository: coleam00/Archon
Length of output: 41
🏁 Script executed:
#!/bin/bash
# Let's read the entire command-handler.ts file to understand structure
cat packages/core/src/handlers/command-handler.tsRepository: coleam00/Archon
Length of output: 40269
🏁 Script executed:
#!/bin/bash
# Read more of the test context around the resume test
sed -n '1355,1380p' packages/core/src/handlers/command-handler.test.tsRepository: coleam00/Archon
Length of output: 1080
🏁 Script executed:
#!/bin/bash
# Check if there are multiple resume tests and understand the expected behavior
rg -n "failed run is ready to resume\|resumeRunId" packages/core/src/handlers/command-handler.test.ts -B 3 -A 8Repository: coleam00/Archon
Length of output: 41
🏁 Script executed:
#!/bin/bash
# Check CommandResult type to confirm what fields are possible
rg -n "interface CommandResult\|type CommandResult" packages/core/src/types.ts -A 15Repository: coleam00/Archon
Length of output: 126
🏁 Script executed:
#!/bin/bash
# Find where CommandResult is defined
find packages/core/src -name "*.ts" -type f | xargs grep -l "CommandResult" | head -5Repository: coleam00/Archon
Length of output: 217
🏁 Script executed:
#!/bin/bash
# Search for CommandResult type definition
rg -n "CommandResult" packages/core/src/handlers/command-handler.ts -B 2 -A 2 | head -20Repository: coleam00/Archon
Length of output: 674
🏁 Script executed:
#!/bin/bash
# Find the types import in command-handler.ts
head -50 packages/core/src/handlers/command-handler.ts | grep -A 5 "import.*CommandResult"Repository: coleam00/Archon
Length of output: 417
🏁 Script executed:
#!/bin/bash
# Find and read the CommandResult type definition
cat packages/core/src/types/index.ts | grep -A 30 "CommandResult"Repository: coleam00/Archon
Length of output: 889
🏁 Script executed:
#!/bin/bash
# Also check if there's a workflow field in CommandResult
rg -n "workflow\?" packages/core/src/types/index.ts -A 2 -B 2Repository: coleam00/Archon
Length of output: 271
🏁 Script executed:
#!/bin/bash
# Verify the exact message text returned by handler vs test expectation
sed -n '696,702p' packages/core/src/handlers/command-handler.tsRepository: coleam00/Archon
Length of output: 437
🏁 Script executed:
#!/bin/bash
# Check if resumeWorkflow is a mock in the test
rg -n "mockResumeWorkflow\|resumeWorkflow\|jest.mock" packages/core/src/handlers/command-handler.test.ts | head -20Repository: coleam00/Archon
Length of output: 41
🏁 Script executed:
#!/bin/bash
# Check the test setup to see what mockGetWorkflowRun returns
sed -n '1360,1376p' packages/core/src/handlers/command-handler.test.tsRepository: coleam00/Archon
Length of output: 793
Test assertions expect unimplemented workflow dispatch behavior for resume.
The handler (packages/core/src/handlers/command-handler.ts:700) returns only:
{
success: true,
message: `Workflow run \`${run.workflow_name}\` (${runId}) is ready to resume...`
}But the test (lines 1370–1375) expects:
- Message containing
"Resuming workflow"(handler emits"is ready to resume") result.workflow.resumeRunId === 'run-123'(noworkflowfield returned; type doesn't supportresumeRunId)result.workflow.definition.name === 'implement'(noworkflowfield returned)
The CommandResult type (types/index.ts:95–98) defines workflow as { definition, args } only—no resumeRunId field exists. The test comment references PR #1551 expecting direct dispatch, but the handler still uses the old "is ready to resume" flow without returning workflow context.
Either the handler needs to be updated to dispatch the workflow directly, or the test assertions must match the current implementation.
ztech-gthb
force-pushed
the
fix/archon-assist-worktree-disabled
branch
from
a2f89ab to
1f5e32a
Compare
|
@ztech-gthb related to #1546 — archon-assist persistence fix. |
2 participants
Summary
archon-assistruns in an isolated sub-worktree by default and has no commit/push step in its node definition. Edits made by the auto-routed assist agent land in the sub-worktree and are unreachable from the calling chat — silent edit loss.archon-assistfor any "do work" request (prompt-builder.ts:107). Every such invocation today produces a positive text report for changes that don't persist. Detailed analysis in archon-assist silently discards edits — workflow has no persistence step #1546.worktree.enabled: falseonarchon-assist.yaml. The workflow now runs in the live checkout. Edits land where syncWorkspace's git reset --hard origin/<default_branch> on source/ every message — destroys any local state that has diverged from origin for managed clones #1516 fast-forward default keeps them safe.archon-assistcommand itself, any other workflow, the workflow schema, no code paths. The mechanism (workflow.worktree.enabled) already existed inpackages/workflows/src/schemas/workflow.ts:40-46; this PR just optsarchon-assistinto it.UX Journey
Before
After
Architecture Diagram
Before
After
Connection inventory:
archon-assist.yamlworktree.enabled: falsedispatchOrchestratorWorkflowworktree-checkarchon-assistbundled-defaults.generated.tsarchon-assist.yamlcontentbun run generate:bundledcommand-handler.test.ts/workflow resumetestsdiscoverWorkflowsWithConfigmockLabel Snapshot
risk: lowsize: XSworkflowsworkflows:defaultsChange Metadata
bugworkflowsLinked Issue
Validation Evidence (required)
The two
/workflow resumetests incommand-handler.test.tswere updated for an unrelated reason: they previously assumed the resume handler returned only a message; the recent companion PR #1551 changed the handler to returnresult.workflow = { definition, args, resumeRunId }so the orchestrator dispatches directly. The test fixtures now seed the workflow-discovery mock withmakeTestWorkflowWithSource({ name })so the lookup in the new handler resolves. This is hygiene that surfaces here because the YAML change triggered a full validate run; the test fix would belong in #1551 if it had been caught there.Security Impact (required)
The change scopes
archon-assist's edits to the live checkout instead of an isolated sub-worktree. That is a smaller-blast-radius change in the sense that there is now exactly one filesystem location to track per project (vs. one-per-run before), and the chat user can see/audit edits in the live checkout without traversing into a thread-specific sub-worktree.Compatibility / Migration
workflow.worktree.enabledfield has existed in the schema since fix: lock workflow runs by working_path (#1036, #1188 part 2) #1212.Human Verification (required)
Verified scenarios:
archon-assistrun from any chat writes its edits to the project's live checkout. They are visible to subsequent/workflow runinvocations in the same conversation (e.g.,archon-assistdoesuv initsetup, the next/workflow run my-workflowsees the resultingpyproject.toml).archon-assist.Edge cases checked:
archon-assistopts out, others are unaffected.archon-assistdescription's "Capability: Full Claude Code agent with all tools available" claim now actually holds for persistence; previously the agent had all tools but the persistence was effectively read-only because edits stranded.What was not verified:
worktree.enabled: falseis platform-agnostic per the schema, but this PR was tested only on the web platform.Side Effects / Blast Radius (required)
archon-assistworkflow only. No other workflow'sworktreesetting changes.archon-assistinvocation now races (in the path-lock sense) with concurrent runs against the same checkout. The base workflow schema'smutates_checkout: truedefault already serializes such runs via the path lock — no new race exposed.mutates_checkout: true+ the pre-existingstate: 'ahead'reporting from git reset --hard origin/<default_branch> on source/ every message — destroys any local state that has diverged from origin for managed clones #1516 handle the "user has unpushed local commits in source/" case loudly.Rollback Plan (required)
archon-assistfalls back to its default isolation behavior. No data migration.worktree.enabled: falsemechanism is already in production use by other workflows.Risks and Mitigations
archon-assist's isolation as a sandbox (i.e., wanted edits NOT to land in source/) would notice that edits now persist immediately.archon-assist.command-handler.test.tstest fixup is technically out of scope for this PR's main purpose (archon-assist YAML).Summary by CodeRabbit
archon-assistworkflow configuration to run in the live checkout by disabling worktree isolation.