feat(isolation,workflows): worktree location + per-workflow isolation policy#1310
Merged
feat(isolation,workflows): worktree location + per-workflow isolation policy#1310
Conversation
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughAdds workflow-level Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI
participant Orch as Orchestrator
participant Prov as IsolationProvider
participant Git as GitHelpers
participant Config as RepoConfig
CLI->>Orch: request run(workflow YAML, CLI flags)
Orch->>Config: inspect workflow.worktree
alt workflow.worktree?.enabled === false
Orch->>Orch: log workflow.worktree_disabled_by_policy
Orch->>Git: set cwd = codebase.default_cwd
Orch-->>CLI: run in live checkout
else
Orch->>Prov: validateAndResolveIsolation(request)
Prov->>Config: load repo config (once)
Prov->>Git: getWorktreeBase(repoPath, codebaseName, override)
Git-->>Prov: return { base, layout }
Prov->>Prov: create/find worktree under resolved base
Prov-->>Orch: return resolved cwd
Orch-->>CLI: run at resolved cwd
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes 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 docstrings
🧪 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 |
Wirasm
force-pushed
the
feat/worktree-path-and-enabled-policy
branch
from
3246c23 to
7d054c8
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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/docs-web/src/content/docs/reference/configuration.md (1)
83-87:⚠️ Potential issue | 🟡 MinorRemove or clarify the stale
paths.worktreesexample.Line 86 still advertises
~/.archon/worktrees, but Line 182 says new worktrees default to the workspace-scoped layout. This conflicts with the PR’s removal of the legacy global worktree fallback and can mislead users about where new worktrees are created.📝 Proposed docs adjustment
# Custom paths (usually not needed) paths: workspaces: ~/.archon/workspaces - worktrees: ~/.archon/worktreesAlso applies to: 182-182
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/docs-web/src/content/docs/reference/configuration.md` around lines 83 - 87, The docs show a stale example for the paths.worktrees key (alongside paths.workspaces); remove or clarify that paths.worktrees is legacy and no longer used so it doesn't imply a global worktree location; instead update the example to either omit paths.worktrees entirely or add a short note stating that new worktrees default to the workspace-scoped layout (no global ~/.archon/worktrees fallback) so readers know where worktrees are actually created.packages/isolation/src/providers/worktree.ts (1)
726-730:⚠️ Potential issue | 🟡 MinorPreserve the single config-load boundary when config is absent.
Line 729 passes the already-loaded
nullconfig, but Line 825 treatsnullthe same as “not provided” and callsloadConfig()again. That contradicts the new create-boundary invariant and can make absent config behave differently between the first and second read.Proposed fix
private async copyConfiguredFiles( canonicalRepoPath: string, worktreePath: string, - worktreeConfig?: { baseBranch?: string; copyFiles?: string[] } | null + worktreeConfig?: WorktreeCreateConfig | null ): Promise<{ configLoadFailed: boolean }> { @@ - if (worktreeConfig) { - userCopyFiles = worktreeConfig.copyFiles ?? []; - } else { + if (worktreeConfig !== undefined) { + userCopyFiles = worktreeConfig?.copyFiles ?? []; + } else { // Config not provided - try loading it try { const loadedConfig = await this.loadConfig(canonicalRepoPath);Also applies to: 814-842
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/isolation/src/providers/worktree.ts` around lines 726 - 730, The code currently treats a returned null worktreeConfig the same as “not provided” and calls loadConfig() again later, breaking the create-boundary invariant; update the logic in the caller (the block using copyConfiguredFiles and later code that calls loadConfig()) to distinguish explicit null from undefined: accept the returned worktreeConfig value as the authoritative result (including null meaning “explicitly absent”) and only call loadConfig() when the config variable is strictly undefined (i.e., not provided), ensuring functions copyConfiguredFiles, configLoadFailed and subsequent code do not re-load when worktreeConfig === null.
🧹 Nitpick comments (2)
packages/core/src/orchestrator/orchestrator-agent.ts (1)
237-242: Use the standard structured log event suffix.
workflow.worktree_disabled_by_policydoes not follow the repo’s{domain}.{action}_{state}convention. A name likeworkflow.worktree_policy_validatedkeeps the policy audit event while matching the accepted state suffix.🪵 Suggested event rename
getLog().info( { workflowName: workflow.name, conversationId, codebaseId: codebase.id }, - 'workflow.worktree_disabled_by_policy' + 'workflow.worktree_policy_validated' );As per coding guidelines, use Pino structured logging with
createLogger()from@archon/paths; follow event naming format{domain}.{action}_{state}(states:_started,_completed,_failed,_validated,_rejected); always pair_startedwith_completedor_failed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/orchestrator/orchestrator-agent.ts` around lines 237 - 242, The structured log event string "workflow.worktree_disabled_by_policy" does not follow the repo convention; update the getLog().info call in the workflow.worktree?.enabled check to emit a standard `{domain}.{action}_{state}` event (e.g. "workflow.worktree_policy_validated") while preserving the same metadata ({ workflowName: workflow.name, conversationId, codebaseId: codebase.id }) and behavior (setting cwd = codebase.default_cwd); ensure the new `_validated` suffix is used for this policy-audit event and keep pairing conventions elsewhere (use `_started`/`_completed` or `_failed` pairs where applicable).packages/workflows/src/loader.ts (1)
346-346: Avoid duplicating the workflow worktree shape.Line 346 redefines the
worktreepolicy shape locally. Use the existing schema-derived workflow contract so futureworktreefields do not drift between the loader and schema.♻️ Proposed refactor
- let worktreePolicy: { enabled?: boolean } | undefined; + let worktreePolicy: WorkflowDefinition['worktree'] | undefined;As per coding guidelines, "Always derive types from Zod schemas using
z.infer<typeof schema>instead of writing parallel hand-crafted interfaces".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/workflows/src/loader.ts` at line 346, The local type declaration for worktreePolicy (the variable worktreePolicy) duplicates the workflow worktree shape; instead derive its type from the existing Zod workflow schema by replacing the ad-hoc type with z.infer<typeof <WorkflowSchema>>['worktree'] (or the exact exported schema name used in your codebase), import that schema where loader.ts defines worktreePolicy, and use that inferred type so the loader stays in sync with the schema.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.archon/workflows/e2e-worktree-disabled.yaml:
- Around line 22-31: The current checks only catch workspace-scoped worktrees
and treat any .git path as a real repo root; update the logic to also detect
repo-local worktrees by (1) extending the worktree detection to test if
"$cwd/.git" is a file whose contents start with "gitdir:" (indicating a gitfile
pointing to a worktree) or if the resolved git dir contains a "worktrees"
directory, and (2) fail when either case is true; replace or augment the
existing [ ! -e "$cwd/.git" ] check with a test that reads "$cwd/.git" and
rejects when it's a gitfile (or when git --git-dir="$cwd" rev-parse --git-dir
resolves outside the cwd), referencing the shell pattern matching and the
"$cwd/.git" existence check in the diff to locate where to change the code.
In @.archon/workflows/repo-triage.yaml:
- Around line 11-15: The comment claiming this workflow is “Read-only” is
inaccurate; update the comment near the worktree.enabled setting
(worktree.enabled: false) to clarify that while source files are not mutated,
the workflow does persist Archon state in the live checkout, writes artifacts,
and may interact with GitHub (comments/issue updates/closures); change the
wording to something like “does not mutate source files; state and artifacts are
intentionally persisted in the live checkout and the workflow may perform
GitHub-side actions” so readers aren’t misled.
In `@packages/cli/src/commands/workflow.test.ts`:
- Around line 881-917: The test incorrectly compares the .create call counts on
the last provider instance (providerBefore/providerAfter) which can false-pass
because getIsolationProvider() returns fresh providers; instead snapshot the
number of times getIsolationProvider was called: capture
getIsolationProviderMock.mock.calls.length before invoking workflowRunCommand
and assert it is unchanged after, replacing the providerBefore/providerAfter and
createCallsBefore/createCallsAfter checks; reference
getIsolationProvider/getIsolationProviderMock and workflowRunCommand to locate
where to change the assertion.
In `@packages/core/src/config/config-types.ts`:
- Around line 180-201: The config docs for worktree.path still reference
obsolete global fallbacks (e.g., global paths.worktrees and default
~/.archon/worktrees) — update the JSDoc above the worktree.path (the path?:
string field) to remove those fallback items and reflect the new precedence:
only repo-local worktree.path and workspace-scoped layouts remain; keep the
“must be a safe relative path” requirement and the example. Ensure the
precedence list and descriptive sentences mention only worktree.path
(repo-local) and the workspace-scoped behavior, and delete any mention of global
paths.worktrees or ~/.archon/worktrees.
In `@packages/git/src/worktree.ts`:
- Around line 65-70: The current workspace detection uses startsWith on repoPath
against workspacesPath (variables workspacesPath and repoPath in
getArchonWorkspacesPath usage) which misclassifies sibling directories; change
to compute the canonicalized relative path using
path.relative(path.resolve(workspacesPath), path.resolve(repoPath)) and only
treat it as a child when that relative value does not start with '..' and is not
equal to '' (i.e., ensure boundary containment), then split that relative path
into parts to derive owner/repo (update the logic inside the block that builds
relative and parts).
In `@packages/isolation/src/providers/worktree.test.ts`:
- Around line 2530-2540: The test currently checks only forward-slash `..`
escapes; update the test invoking provider.getWorktreePath (using
provider.generateBranchName and baseRequest) to also assert that Windows-style
backslash traversal patterns (e.g. '..\\worktrees', '..\\', and mixed
'nested\\..\\..\\escape') throw the same /must stay within the repo/ error so
backslash-based escaping is rejected on Windows while preserving Linux CI
behavior.
In `@packages/isolation/src/providers/worktree.ts`:
- Around line 94-100: The containment check using resolve(...) and
startsWith(repoRootResolved + '/') is platform- and symlink-unsafe; replace the
startsWith logic with path.relative(repoRootResolved, resolved) and ensure the
relative path does not begin with '..' (or is not equal to '..' segments) to
correctly validate containment on Windows and POSIX in the code around the
resolved/repoRootResolved variables in providers/worktree.ts; additionally,
after creating the worktree base (where the code creates the directory for the
worktree), call fs.realpath() (or fs.promises.realpath) on that created path and
verify its realpath is contained within the repo realpath (using path.relative
again) to detect and reject symlink escapes before using the directory.
In `@packages/isolation/src/types.ts`:
- Around line 251-263: Update the JSDoc for the `path?: string` field to clarify
precedence under the two-layout model: state that this repo-local relative path
overrides the workspace-scoped default (not a legacy global `~/.archon/...`
layout), and mention validation is still enforced in
`WorktreeProvider.getWorktreePath()`; keep the example and rules about safe
relative paths but replace “global defaults” wording with “workspace-scoped
default” (or equivalent) to avoid implying a removed global layout participates
in resolution.
In `@packages/workflows/src/schemas/workflow.ts`:
- Around line 39-48: The workflowWorktreePolicySchema currently allows unknown
keys (Zod's default strips extras); update the z.object definition for
workflowWorktreePolicySchema to reject unknown keys by appending .strict() to
the object schema so typos like "enable" or extra properties cause a validation
error; locate the workflowWorktreePolicySchema symbol in this file and change
its definition to use z.object({...}).strict() and run/adjust any callers/tests
that expect permissive parsing.
---
Outside diff comments:
In `@packages/docs-web/src/content/docs/reference/configuration.md`:
- Around line 83-87: The docs show a stale example for the paths.worktrees key
(alongside paths.workspaces); remove or clarify that paths.worktrees is legacy
and no longer used so it doesn't imply a global worktree location; instead
update the example to either omit paths.worktrees entirely or add a short note
stating that new worktrees default to the workspace-scoped layout (no global
~/.archon/worktrees fallback) so readers know where worktrees are actually
created.
In `@packages/isolation/src/providers/worktree.ts`:
- Around line 726-730: The code currently treats a returned null worktreeConfig
the same as “not provided” and calls loadConfig() again later, breaking the
create-boundary invariant; update the logic in the caller (the block using
copyConfiguredFiles and later code that calls loadConfig()) to distinguish
explicit null from undefined: accept the returned worktreeConfig value as the
authoritative result (including null meaning “explicitly absent”) and only call
loadConfig() when the config variable is strictly undefined (i.e., not
provided), ensuring functions copyConfiguredFiles, configLoadFailed and
subsequent code do not re-load when worktreeConfig === null.
---
Nitpick comments:
In `@packages/core/src/orchestrator/orchestrator-agent.ts`:
- Around line 237-242: The structured log event string
"workflow.worktree_disabled_by_policy" does not follow the repo convention;
update the getLog().info call in the workflow.worktree?.enabled check to emit a
standard `{domain}.{action}_{state}` event (e.g.
"workflow.worktree_policy_validated") while preserving the same metadata ({
workflowName: workflow.name, conversationId, codebaseId: codebase.id }) and
behavior (setting cwd = codebase.default_cwd); ensure the new `_validated`
suffix is used for this policy-audit event and keep pairing conventions
elsewhere (use `_started`/`_completed` or `_failed` pairs where applicable).
In `@packages/workflows/src/loader.ts`:
- Line 346: The local type declaration for worktreePolicy (the variable
worktreePolicy) duplicates the workflow worktree shape; instead derive its type
from the existing Zod workflow schema by replacing the ad-hoc type with
z.infer<typeof <WorkflowSchema>>['worktree'] (or the exact exported schema name
used in your codebase), import that schema where loader.ts defines
worktreePolicy, and use that inferred type so the loader stays in sync with the
schema.
🪄 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: 44e6b46c-3ebc-4cdf-8202-c9a63d119150
📒 Files selected for processing (19)
.archon/workflows/e2e-worktree-disabled.yaml.archon/workflows/repo-triage.yamlCHANGELOG.mdpackages/cli/src/commands/workflow.test.tspackages/cli/src/commands/workflow.tspackages/core/src/config/config-types.tspackages/core/src/orchestrator/orchestrator-agent.tspackages/docs-web/src/content/docs/guides/authoring-workflows.mdpackages/docs-web/src/content/docs/reference/configuration.mdpackages/git/src/git.test.tspackages/git/src/index.tspackages/git/src/worktree.tspackages/isolation/src/factory.tspackages/isolation/src/providers/worktree.test.tspackages/isolation/src/providers/worktree.tspackages/isolation/src/types.tspackages/workflows/src/loader.test.tspackages/workflows/src/loader.tspackages/workflows/src/schemas/workflow.ts
Comment on lines
+22
to
+31
| case "$cwd" in | ||
| */.archon/workspaces/*/worktrees/*) | ||
| echo "FAIL: workflow ran inside a worktree ($cwd) despite worktree.enabled: false" | ||
| exit 1 | ||
| ;; | ||
| esac | ||
| if [ ! -e "$cwd/.git" ]; then | ||
| echo "FAIL: cwd $cwd is not a git checkout root (.git missing)" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
Detect repo-local worktrees too.
The current assertion only catches workspace-scoped paths. If policy enforcement regresses with worktree.path: .worktrees, this workflow can run inside a repo-local worktree and still pass because .git exists as a gitfile.
🧪 Proposed assertion hardening
case "$cwd" in
*/.archon/workspaces/*/worktrees/*)
echo "FAIL: workflow ran inside a worktree ($cwd) despite worktree.enabled: false"
exit 1
;;
esac
if [ ! -e "$cwd/.git" ]; then
echo "FAIL: cwd $cwd is not a git checkout root (.git missing)"
exit 1
fi
+ if [ -f "$cwd/.git" ] && grep -q '/worktrees/' "$cwd/.git"; then
+ echo "FAIL: workflow ran inside a git worktree ($cwd) despite worktree.enabled: false"
+ exit 1
+ fi
echo "PASS: ran in live checkout (no worktree created by policy)"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| case "$cwd" in | |
| */.archon/workspaces/*/worktrees/*) | |
| echo "FAIL: workflow ran inside a worktree ($cwd) despite worktree.enabled: false" | |
| exit 1 | |
| ;; | |
| esac | |
| if [ ! -e "$cwd/.git" ]; then | |
| echo "FAIL: cwd $cwd is not a git checkout root (.git missing)" | |
| exit 1 | |
| fi | |
| case "$cwd" in | |
| */.archon/workspaces/*/worktrees/*) | |
| echo "FAIL: workflow ran inside a worktree ($cwd) despite worktree.enabled: false" | |
| exit 1 | |
| ;; | |
| esac | |
| if [ ! -e "$cwd/.git" ]; then | |
| echo "FAIL: cwd $cwd is not a git checkout root (.git missing)" | |
| exit 1 | |
| fi | |
| if [ -f "$cwd/.git" ] && grep -q '/worktrees/' "$cwd/.git"; then | |
| echo "FAIL: workflow ran inside a git worktree ($cwd) despite worktree.enabled: false" | |
| exit 1 | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.archon/workflows/e2e-worktree-disabled.yaml around lines 22 - 31, The
current checks only catch workspace-scoped worktrees and treat any .git path as
a real repo root; update the logic to also detect repo-local worktrees by (1)
extending the worktree detection to test if "$cwd/.git" is a file whose contents
start with "gitdir:" (indicating a gitfile pointing to a worktree) or if the
resolved git dir contains a "worktrees" directory, and (2) fail when either case
is true; replace or augment the existing [ ! -e "$cwd/.git" ] check with a test
that reads "$cwd/.git" and rejects when it's a gitfile (or when git
--git-dir="$cwd" rev-parse --git-dir resolves outside the cwd), referencing the
shell pattern matching and the "$cwd/.git" existence check in the diff to locate
where to change the code.
Comment on lines
+11
to
+15
| # Read-only triage runs directly in the live checkout. Creating a worktree | ||
| # every run would be wasted work (nothing is mutated) and would scatter stale | ||
| # branches under ~/.archon/workspaces/<owner>/<repo>/worktrees/. | ||
| worktree: | ||
| enabled: false |
There was a problem hiding this comment.
Clarify that this workflow still mutates state and GitHub.
worktree.enabled: false is fine here, but the comment says this is “Read-only” and that “nothing is mutated.” The workflow writes .archon/state/*, writes artifacts, and can comment/close issues. Consider narrowing the claim to “does not mutate source files” and call out that state is intentionally persisted in the live checkout.
📝 Suggested wording
-# Read-only triage runs directly in the live checkout. Creating a worktree
-# every run would be wasted work (nothing is mutated) and would scatter stale
-# branches under ~/.archon/workspaces/<owner>/<repo>/worktrees/.
+# Repo maintenance runs directly in the live checkout: it does not modify source
+# files, but it intentionally persists `.archon/state/*` and may comment/close
+# issues via GitHub. Creating a worktree every run would hide that state and
+# scatter stale branches under ~/.archon/workspaces/<owner>/<repo>/worktrees/.
worktree:
enabled: false🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.archon/workflows/repo-triage.yaml around lines 11 - 15, The comment
claiming this workflow is “Read-only” is inaccurate; update the comment near the
worktree.enabled setting (worktree.enabled: false) to clarify that while source
files are not mutated, the workflow does persist Archon state in the live
checkout, writes artifacts, and may interact with GitHub (comments/issue
updates/closures); change the wording to something like “does not mutate source
files; state and artifacts are intentionally persisted in the live checkout and
the workflow may perform GitHub-side actions” so readers aren’t misled.
Comment on lines
+881
to
+917
| const getIsolationProviderMock = isolation.getIsolationProvider as ReturnType<typeof mock>; | ||
| const providerBefore = getIsolationProviderMock.mock.results.at(-1)?.value as | ||
| | { create: ReturnType<typeof mock> } | ||
| | undefined; | ||
| const createCallsBefore = providerBefore?.create.mock.calls.length ?? 0; | ||
|
|
||
| (discoverWorkflowsWithConfig as ReturnType<typeof mock>).mockResolvedValueOnce({ | ||
| workflows: [ | ||
| makeTestWorkflowWithSource({ | ||
| name: 'triage', | ||
| description: 'Read-only triage', | ||
| worktree: { enabled: false }, | ||
| }), | ||
| ], | ||
| errors: [], | ||
| }); | ||
| (conversationDb.getOrCreateConversation as ReturnType<typeof mock>).mockResolvedValueOnce({ | ||
| id: 'conv-123', | ||
| }); | ||
| (codebaseDb.findCodebaseByDefaultCwd as ReturnType<typeof mock>).mockResolvedValueOnce({ | ||
| id: 'cb-123', | ||
| default_cwd: '/test/path', | ||
| }); | ||
| (conversationDb.updateConversation as ReturnType<typeof mock>).mockResolvedValueOnce(undefined); | ||
| (executeWorkflow as ReturnType<typeof mock>).mockResolvedValueOnce({ | ||
| success: true, | ||
| workflowRunId: 'run-123', | ||
| }); | ||
|
|
||
| // No flags — policy alone should disable isolation | ||
| await workflowRunCommand('/test/path', 'triage', 'go', {}); | ||
|
|
||
| const providerAfter = getIsolationProviderMock.mock.results.at(-1)?.value as | ||
| | { create: ReturnType<typeof mock> } | ||
| | undefined; | ||
| const createCallsAfter = providerAfter?.create.mock.calls.length ?? 0; | ||
| expect(createCallsAfter).toBe(createCallsBefore); |
There was a problem hiding this comment.
Assert the provider boundary is not entered.
This compares create calls on whichever provider object was last returned by a process-global mock. Because getIsolationProvider() returns a fresh provider each call, this can false-pass when previous tests leave matching call counts. Snapshot getIsolationProvider calls instead.
🧪 Proposed test hardening
const getIsolationProviderMock = isolation.getIsolationProvider as ReturnType<typeof mock>;
- const providerBefore = getIsolationProviderMock.mock.results.at(-1)?.value as
- | { create: ReturnType<typeof mock> }
- | undefined;
- const createCallsBefore = providerBefore?.create.mock.calls.length ?? 0;
+ const providerCallsBefore = getIsolationProviderMock.mock.calls.length;
(discoverWorkflowsWithConfig as ReturnType<typeof mock>).mockResolvedValueOnce({
workflows: [
@@
// No flags — policy alone should disable isolation
await workflowRunCommand('/test/path', 'triage', 'go', {});
- const providerAfter = getIsolationProviderMock.mock.results.at(-1)?.value as
- | { create: ReturnType<typeof mock> }
- | undefined;
- const createCallsAfter = providerAfter?.create.mock.calls.length ?? 0;
- expect(createCallsAfter).toBe(createCallsBefore);
+ expect(getIsolationProviderMock.mock.calls.length).toBe(providerCallsBefore);As per coding guidelines, "Prefer reproducible commands and locked dependency behavior in CI-sensitive paths; keep tests deterministic with no flaky timing or network dependence without guardrails".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const getIsolationProviderMock = isolation.getIsolationProvider as ReturnType<typeof mock>; | |
| const providerBefore = getIsolationProviderMock.mock.results.at(-1)?.value as | |
| | { create: ReturnType<typeof mock> } | |
| | undefined; | |
| const createCallsBefore = providerBefore?.create.mock.calls.length ?? 0; | |
| (discoverWorkflowsWithConfig as ReturnType<typeof mock>).mockResolvedValueOnce({ | |
| workflows: [ | |
| makeTestWorkflowWithSource({ | |
| name: 'triage', | |
| description: 'Read-only triage', | |
| worktree: { enabled: false }, | |
| }), | |
| ], | |
| errors: [], | |
| }); | |
| (conversationDb.getOrCreateConversation as ReturnType<typeof mock>).mockResolvedValueOnce({ | |
| id: 'conv-123', | |
| }); | |
| (codebaseDb.findCodebaseByDefaultCwd as ReturnType<typeof mock>).mockResolvedValueOnce({ | |
| id: 'cb-123', | |
| default_cwd: '/test/path', | |
| }); | |
| (conversationDb.updateConversation as ReturnType<typeof mock>).mockResolvedValueOnce(undefined); | |
| (executeWorkflow as ReturnType<typeof mock>).mockResolvedValueOnce({ | |
| success: true, | |
| workflowRunId: 'run-123', | |
| }); | |
| // No flags — policy alone should disable isolation | |
| await workflowRunCommand('/test/path', 'triage', 'go', {}); | |
| const providerAfter = getIsolationProviderMock.mock.results.at(-1)?.value as | |
| | { create: ReturnType<typeof mock> } | |
| | undefined; | |
| const createCallsAfter = providerAfter?.create.mock.calls.length ?? 0; | |
| expect(createCallsAfter).toBe(createCallsBefore); | |
| const getIsolationProviderMock = isolation.getIsolationProvider as ReturnType<typeof mock>; | |
| const providerCallsBefore = getIsolationProviderMock.mock.calls.length; | |
| (discoverWorkflowsWithConfig as ReturnType<typeof mock>).mockResolvedValueOnce({ | |
| workflows: [ | |
| makeTestWorkflowWithSource({ | |
| name: 'triage', | |
| description: 'Read-only triage', | |
| worktree: { enabled: false }, | |
| }), | |
| ], | |
| errors: [], | |
| }); | |
| (conversationDb.getOrCreateConversation as ReturnType<typeof mock>).mockResolvedValueOnce({ | |
| id: 'conv-123', | |
| }); | |
| (codebaseDb.findCodebaseByDefaultCwd as ReturnType<typeof mock>).mockResolvedValueOnce({ | |
| id: 'cb-123', | |
| default_cwd: '/test/path', | |
| }); | |
| (conversationDb.updateConversation as ReturnType<typeof mock>).mockResolvedValueOnce(undefined); | |
| (executeWorkflow as ReturnType<typeof mock>).mockResolvedValueOnce({ | |
| success: true, | |
| workflowRunId: 'run-123', | |
| }); | |
| // No flags — policy alone should disable isolation | |
| await workflowRunCommand('/test/path', 'triage', 'go', {}); | |
| expect(getIsolationProviderMock.mock.calls.length).toBe(providerCallsBefore); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/commands/workflow.test.ts` around lines 881 - 917, The test
incorrectly compares the .create call counts on the last provider instance
(providerBefore/providerAfter) which can false-pass because
getIsolationProvider() returns fresh providers; instead snapshot the number of
times getIsolationProvider was called: capture
getIsolationProviderMock.mock.calls.length before invoking workflowRunCommand
and assert it is unchanged after, replacing the providerBefore/providerAfter and
createCallsBefore/createCallsAfter checks; reference
getIsolationProvider/getIsolationProviderMock and workflowRunCommand to locate
where to change the assertion.
Comment on lines
+180
to
+201
| /** | ||
| * Per-project worktree directory (relative to repo root). When set, | ||
| * worktrees are created at `<repoRoot>/<path>/<branch>` instead of under | ||
| * `~/.archon/worktrees/` or the workspaces layout. | ||
| * | ||
| * Opt-in — co-locates worktrees with the repo so they appear in the IDE | ||
| * file tree. The user is responsible for adding the directory to their | ||
| * `.gitignore` (no automatic file mutation). | ||
| * | ||
| * Path resolution precedence (highest to lowest): | ||
| * 1. this `worktree.path` (repo-local) | ||
| * 2. global `paths.worktrees` (absolute override in `~/.archon/config.yaml`) | ||
| * 3. auto-detected project-scoped (`~/.archon/workspaces/owner/repo/...`) | ||
| * 4. default global (`~/.archon/worktrees/`) | ||
| * | ||
| * Must be a safe relative path: no leading `/`, no `..` segments. Absolute | ||
| * or escaping values fail loudly at worktree creation (Fail Fast — no silent | ||
| * fallback). | ||
| * | ||
| * @example '.worktrees' | ||
| */ | ||
| path?: string; |
There was a problem hiding this comment.
Remove the obsolete global worktree precedence from this public config doc.
This comment still documents global paths.worktrees and the default ~/.archon/worktrees/ layout, but the PR summary says those fallback layouts were removed. Public type docs should describe only worktree.path → repo-local, otherwise workspace-scoped.
📝 Suggested wording
- * worktrees are created at `<repoRoot>/<path>/<branch>` instead of under
- * `~/.archon/worktrees/` or the workspaces layout.
+ * worktrees are created at `<repoRoot>/<path>/<branch>` instead of under
+ * the workspace-scoped layout.
@@
- * Path resolution precedence (highest to lowest):
- * 1. this `worktree.path` (repo-local)
- * 2. global `paths.worktrees` (absolute override in `~/.archon/config.yaml`)
- * 3. auto-detected project-scoped (`~/.archon/workspaces/owner/repo/...`)
- * 4. default global (`~/.archon/worktrees/`)
+ * Path resolution precedence:
+ * 1. this `worktree.path` (repo-local)
+ * 2. workspace-scoped (`~/.archon/workspaces/<owner>/<repo>/worktrees/`)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /** | |
| * Per-project worktree directory (relative to repo root). When set, | |
| * worktrees are created at `<repoRoot>/<path>/<branch>` instead of under | |
| * `~/.archon/worktrees/` or the workspaces layout. | |
| * | |
| * Opt-in — co-locates worktrees with the repo so they appear in the IDE | |
| * file tree. The user is responsible for adding the directory to their | |
| * `.gitignore` (no automatic file mutation). | |
| * | |
| * Path resolution precedence (highest to lowest): | |
| * 1. this `worktree.path` (repo-local) | |
| * 2. global `paths.worktrees` (absolute override in `~/.archon/config.yaml`) | |
| * 3. auto-detected project-scoped (`~/.archon/workspaces/owner/repo/...`) | |
| * 4. default global (`~/.archon/worktrees/`) | |
| * | |
| * Must be a safe relative path: no leading `/`, no `..` segments. Absolute | |
| * or escaping values fail loudly at worktree creation (Fail Fast — no silent | |
| * fallback). | |
| * | |
| * @example '.worktrees' | |
| */ | |
| path?: string; | |
| /** | |
| * Per-project worktree directory (relative to repo root). When set, | |
| * worktrees are created at `<repoRoot>/<path>/<branch>` instead of under | |
| * the workspace-scoped layout. | |
| * | |
| * Opt-in — co-locates worktrees with the repo so they appear in the IDE | |
| * file tree. The user is responsible for adding the directory to their | |
| * `.gitignore` (no automatic file mutation). | |
| * | |
| * Path resolution precedence: | |
| * 1. this `worktree.path` (repo-local) | |
| * 2. workspace-scoped (`~/.archon/workspaces/<owner>/<repo>/worktrees/`) | |
| * | |
| * Must be a safe relative path: no leading `/`, no `..` segments. Absolute | |
| * or escaping values fail loudly at worktree creation (Fail Fast — no silent | |
| * fallback). | |
| * | |
| * `@example` '.worktrees' | |
| */ | |
| path?: string; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/config/config-types.ts` around lines 180 - 201, The config
docs for worktree.path still reference obsolete global fallbacks (e.g., global
paths.worktrees and default ~/.archon/worktrees) — update the JSDoc above the
worktree.path (the path?: string field) to remove those fallback items and
reflect the new precedence: only repo-local worktree.path and workspace-scoped
layouts remain; keep the “must be a safe relative path” requirement and the
example. Ensure the precedence list and descriptive sentences mention only
worktree.path (repo-local) and the workspace-scoped behavior, and delete any
mention of global paths.worktrees or ~/.archon/worktrees.
Comment on lines
65
to
+70
| const workspacesPath = getArchonWorkspacesPath(); | ||
| if (repoPath.startsWith(workspacesPath)) { | ||
| const relative = repoPath.substring(workspacesPath.length + 1); | ||
| const parts = relative.split(/[/\\]/).filter(p => p.length > 0); | ||
| if (parts.length >= 2) { | ||
| return getProjectWorktreesPath(parts[0], parts[1]); | ||
| return { owner: parts[0], repo: parts[1] }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
node - <<'NODE'
const workspacesPath = '/home/me/.archon/workspaces';
const repoPath = '/home/me/.archon/workspaces-backup/acme/widget/source';
const relative = repoPath.substring(workspacesPath.length + 1);
const parts = relative.split(/[/\\]/).filter(Boolean);
console.log({ startsWith: repoPath.startsWith(workspacesPath), derived: parts.slice(0, 2) });
// Expected with current logic: startsWith is true even though repoPath is a sibling, not a child.
NODERepository: coleam00/Archon
Length of output: 110
🏁 Script executed:
cat -n packages/git/src/worktree.ts | sed -n '50,80p'Repository: coleam00/Archon
Length of output: 1491
🏁 Script executed:
cat -n packages/git/src/worktree.ts | head -100Repository: coleam00/Archon
Length of output: 4873
Use path-boundary containment for workspace detection.
Line 66 uses repoPath.startsWith(workspacesPath) without boundary checking, causing sibling paths such as ~/.archon/workspaces-backup/acme/widget/source to be incorrectly classified as workspace children and derive wrong owner/repo values (e.g., { owner: 'backup', repo: 'acme' } instead of the actual path segments). Use relative(resolve(workspacesPath), resolve(repoPath)) with a boundary check instead.
Proposed fix
-import { join, resolve } from 'path';
+import { isAbsolute, join, relative, resolve, sep } from 'path';
@@
- const workspacesPath = getArchonWorkspacesPath();
- if (repoPath.startsWith(workspacesPath)) {
- const relative = repoPath.substring(workspacesPath.length + 1);
- const parts = relative.split(/[/\\]/).filter(p => p.length > 0);
+ const workspacesPath = resolve(getArchonWorkspacesPath());
+ const resolvedRepoPath = resolve(repoPath);
+ const relativeToWorkspaces = relative(workspacesPath, resolvedRepoPath);
+ if (
+ relativeToWorkspaces &&
+ relativeToWorkspaces !== '..' &&
+ !relativeToWorkspaces.startsWith(`..${sep}`) &&
+ !isAbsolute(relativeToWorkspaces)
+ ) {
+ const parts = relativeToWorkspaces.split(/[/\\]/).filter(p => p.length > 0);
if (parts.length >= 2) {
return { owner: parts[0], repo: parts[1] };
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/git/src/worktree.ts` around lines 65 - 70, The current workspace
detection uses startsWith on repoPath against workspacesPath (variables
workspacesPath and repoPath in getArchonWorkspacesPath usage) which
misclassifies sibling directories; change to compute the canonicalized relative
path using path.relative(path.resolve(workspacesPath), path.resolve(repoPath))
and only treat it as a child when that relative value does not start with '..'
and is not equal to '' (i.e., ensure boundary containment), then split that
relative path into parts to derive owner/repo (update the logic inside the block
that builds relative and parts).
Comment on lines
+2530
to
+2540
| test('rejects a worktree.path that escapes the repo root via `..`', () => { | ||
| const branch = provider.generateBranchName(baseRequest); | ||
| expect(() => provider.getWorktreePath(baseRequest, branch, { path: '../worktrees' })).toThrow( | ||
| /must stay within the repo/ | ||
| ); | ||
| expect(() => provider.getWorktreePath(baseRequest, branch, { path: '..' })).toThrow( | ||
| /must stay within the repo/ | ||
| ); | ||
| expect(() => | ||
| provider.getWorktreePath(baseRequest, branch, { path: 'nested/../../escape' }) | ||
| ).toThrow(/must stay within the repo/); |
There was a problem hiding this comment.
Cover backslash traversal in worktree.path validation.
These tests only cover /-separated .. escapes. Since this suite already validates Windows-style path handling, add ..\\... cases so repo-local worktrees cannot escape the repo root on Windows while Linux CI still passes.
🧪 Proposed additional cases
expect(() =>
provider.getWorktreePath(baseRequest, branch, { path: 'nested/../../escape' })
).toThrow(/must stay within the repo/);
+ expect(() =>
+ provider.getWorktreePath(baseRequest, branch, { path: '..\\worktrees' })
+ ).toThrow(/must stay within the repo/);
+ expect(() =>
+ provider.getWorktreePath(baseRequest, branch, { path: 'nested\\..\\..\\escape' })
+ ).toThrow(/must stay within the repo/);
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| test('rejects a worktree.path that escapes the repo root via `..`', () => { | |
| const branch = provider.generateBranchName(baseRequest); | |
| expect(() => provider.getWorktreePath(baseRequest, branch, { path: '../worktrees' })).toThrow( | |
| /must stay within the repo/ | |
| ); | |
| expect(() => provider.getWorktreePath(baseRequest, branch, { path: '..' })).toThrow( | |
| /must stay within the repo/ | |
| ); | |
| expect(() => | |
| provider.getWorktreePath(baseRequest, branch, { path: 'nested/../../escape' }) | |
| ).toThrow(/must stay within the repo/); | |
| test('rejects a worktree.path that escapes the repo root via `..`', () => { | |
| const branch = provider.generateBranchName(baseRequest); | |
| expect(() => provider.getWorktreePath(baseRequest, branch, { path: '../worktrees' })).toThrow( | |
| /must stay within the repo/ | |
| ); | |
| expect(() => provider.getWorktreePath(baseRequest, branch, { path: '..' })).toThrow( | |
| /must stay within the repo/ | |
| ); | |
| expect(() => | |
| provider.getWorktreePath(baseRequest, branch, { path: 'nested/../../escape' }) | |
| ).toThrow(/must stay within the repo/); | |
| expect(() => | |
| provider.getWorktreePath(baseRequest, branch, { path: '..\\worktrees' }) | |
| ).toThrow(/must stay within the repo/); | |
| expect(() => | |
| provider.getWorktreePath(baseRequest, branch, { path: 'nested\\..\\..\\escape' }) | |
| ).toThrow(/must stay within the repo/); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/isolation/src/providers/worktree.test.ts` around lines 2530 - 2540,
The test currently checks only forward-slash `..` escapes; update the test
invoking provider.getWorktreePath (using provider.generateBranchName and
baseRequest) to also assert that Windows-style backslash traversal patterns
(e.g. '..\\worktrees', '..\\', and mixed 'nested\\..\\..\\escape') throw the
same /must stay within the repo/ error so backslash-based escaping is rejected
on Windows while preserving Linux CI behavior.
Comment on lines
+251
to
+263
| /** | ||
| * Per-project relative path (from repo root) where worktrees should be created. | ||
| * When set, worktrees live at `<repoRoot>/<path>/<branch>` with `repo-local` layout. | ||
| * Highest priority in path resolution — overrides project-scoped and global defaults. | ||
| * | ||
| * Must be a safe relative path: no leading `/`, no `..` segments, non-empty after trim. | ||
| * Validation is enforced in `WorktreeProvider.getWorktreePath()` (fails fast with a | ||
| * clear error rather than silently falling back). | ||
| * | ||
| * Sourced from `.archon/config.yaml > worktree.path` in the repo. | ||
| * @example '.worktrees' | ||
| */ | ||
| path?: string; |
There was a problem hiding this comment.
Update the precedence wording to match the two-layout model.
The new path field is correct, but “global defaults” reads like the removed legacy ~/.archon/worktrees/... layout still participates in resolution. This should describe only repo-local overriding the workspace-scoped default.
📝 Suggested wording
- * Highest priority in path resolution — overrides project-scoped and global defaults.
+ * Highest priority in path resolution — overrides the workspace-scoped default.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /** | |
| * Per-project relative path (from repo root) where worktrees should be created. | |
| * When set, worktrees live at `<repoRoot>/<path>/<branch>` with `repo-local` layout. | |
| * Highest priority in path resolution — overrides project-scoped and global defaults. | |
| * | |
| * Must be a safe relative path: no leading `/`, no `..` segments, non-empty after trim. | |
| * Validation is enforced in `WorktreeProvider.getWorktreePath()` (fails fast with a | |
| * clear error rather than silently falling back). | |
| * | |
| * Sourced from `.archon/config.yaml > worktree.path` in the repo. | |
| * @example '.worktrees' | |
| */ | |
| path?: string; | |
| /** | |
| * Per-project relative path (from repo root) where worktrees should be created. | |
| * When set, worktrees live at `<repoRoot>/<path>/<branch>` with `repo-local` layout. | |
| * Highest priority in path resolution — overrides the workspace-scoped default. | |
| * | |
| * Must be a safe relative path: no leading `/`, no `..` segments, non-empty after trim. | |
| * Validation is enforced in `WorktreeProvider.getWorktreePath()` (fails fast with a | |
| * clear error rather than silently falling back). | |
| * | |
| * Sourced from `.archon/config.yaml > worktree.path` in the repo. | |
| * `@example` '.worktrees' | |
| */ | |
| path?: string; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/isolation/src/types.ts` around lines 251 - 263, Update the JSDoc for
the `path?: string` field to clarify precedence under the two-layout model:
state that this repo-local relative path overrides the workspace-scoped default
(not a legacy global `~/.archon/...` layout), and mention validation is still
enforced in `WorktreeProvider.getWorktreePath()`; keep the example and rules
about safe relative paths but replace “global defaults” wording with
“workspace-scoped default” (or equivalent) to avoid implying a removed global
layout participates in resolution.
Comment on lines
+39
to
+48
| export const workflowWorktreePolicySchema = z.object({ | ||
| /** | ||
| * Pin worktree isolation on or off for this workflow. | ||
| * - `true` — always run inside a worktree; CLI `--no-worktree` hard-errors | ||
| * - `false` — always run in the live checkout; CLI `--branch` / `--from` | ||
| * hard-error, orchestrator skips isolation resolution | ||
| * - omitted — caller decides (current default = worktree for most types) | ||
| */ | ||
| enabled: z.boolean().optional(), | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Zod object .strict unknown keys behavior
💡 Result:
In Zod, object schemas have three behaviors for unknown (extra) keys by default, .passthrough, .strict, and the default .strip: - Default z.object({}): Strips unknown keys from the output, ignoring them silently. Example: z.object({name: z.string}).parse({name: 'test', extra: true}) returns {name: 'test'}. - .strict or z.strictObject({}): Throws a ZodError with "unrecognized_keys" if any unknown keys are present in the input. Example: z.strictObject({name: z.string}).parse({name: 'test', extra: true}) throws an error listing the extra keys. - .passthrough or z.looseObject({}): Allows unknown keys to pass through to the output unchanged. This behavior is consistent across Zod v3 and v4 (current as of 2026). In v4, use z.strictObject(shape) directly instead of chaining .strict on z.object, though both work. Strict mode is shallow (top-level keys only); nested objects use their own rules. Example code: import { z } from 'zod'; const strictSchema = z.strictObject({ name: z.string, }); strictSchema.parse({ name: 'Alice' }); // OK strictSchema.parse({ name: 'Alice', age: 30 }); // Throws: Unrecognized key(s) in object: 'age'
Citations:
- 1: https://zod.dev/?id=strict
- 2: https://v4.zod.dev/?id=strict
- 3: https://zod.dev/api?id=zstrictobject
- 4: https://v4.zod.dev/api?id=zstrictobject
- 5: https://v3.zod.dev/?id=strict
- 6: https://stackoverflow.com/questions/70415330/do-not-allow-extra-properties-with-zod-parse
- 7:
strictObjectchanges behaviour in v4 colinhacks/zod#4823 - 8: https://mtsknn.fi/blog/zod-object-strict/
- 9: https://www.codelessgenie.com/blog/do-not-allow-extra-properties-with-zod-parse/
🏁 Script executed:
cat -n packages/workflows/src/schemas/workflow.ts | head -80Repository: coleam00/Archon
Length of output: 3969
🏁 Script executed:
rg "workflowWorktreePolicySchema" -A 5 -B 5 packages/workflows/src/Repository: coleam00/Archon
Length of output: 3017
🏁 Script executed:
rg "\.strict\(\)" packages/workflows/src/schemas/Repository: coleam00/Archon
Length of output: 230
Add .strict() to reject unknown worktree policy keys.
The schema at lines 39–48 uses the default Zod object behavior, which silently strips unknown keys. This means a typo like worktree: { enable: true } parses as an empty policy {} and falls back to caller defaults, defeating the workflow's explicit isolation intent.
Per the Fail Fast + Explicit Errors guideline, unknown keys should be rejected:
Proposed fix
export const workflowWorktreePolicySchema = z.object({
/**
* Pin worktree isolation on or off for this workflow.
@@
*/
enabled: z.boolean().optional(),
-});
+}).strict();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/workflows/src/schemas/workflow.ts` around lines 39 - 48, The
workflowWorktreePolicySchema currently allows unknown keys (Zod's default strips
extras); update the z.object definition for workflowWorktreePolicySchema to
reject unknown keys by appending .strict() to the object schema so typos like
"enable" or extra properties cause a validation error; locate the
workflowWorktreePolicySchema symbol in this file and change its definition to
use z.object({...}).strict() and run/adjust any callers/tests that expect
permissive parsing.
Collaborator
Author
PR Review Summary (multi-agent)Ran 7 specialist agents: code-reviewer, docs-impact, pr-test-analyzer, silent-failure-hunter, type-design-analyzer, comment-analyzer, code-simplifier. Aggregated below. Critical Issues (1)
Important Issues (5)
Suggestions (14)
Strengths
VerdictNEEDS FIXES — one cross-platform correctness bug blocks merge on Windows; four important items (stale JSDoc, untested orchestrator short-circuit, undocumented resume bypass, silent typo swallow) should be addressed before merge. Everything else is follow-up-grade. Recommended Actions
|
Adds an opt-in `worktree.path` to .archon/config.yaml so a repo can co-locate worktrees with its own checkout (`<repoRoot>/<path>/<branch>`) instead of the default `~/.archon/workspaces/<owner>/<repo>/worktrees/<branch>`. Requested in joelsb's #1117. Primitive changes (clean up the graveyard rather than add parallel code paths): - Collapse worktree layouts from three to two. The old "legacy global" layout (`~/.archon/worktrees/<owner>/<repo>/<branch>`) is gone — every repo resolves to the workspace-scoped layout (`~/.archon/workspaces/<owner>/<repo>/worktrees/<branch>`), whether it was archon-cloned or locally registered. `extractOwnerRepo()` on the repo path is the stable identity fallback. Ends the divergence where workspace-cloned and local repos had visibly different worktree trees. - `getWorktreeBase()` in @archon/git now returns `{ base, layout }` and accepts an optional `{ repoLocal }` override. The layout value replaces the old `isProjectScopedWorktreeBase()` classification at the call sites (`isProjectScopedWorktreeBase` stays exported as deprecated back-compat). - `WorktreeCreateConfig.path` carries the validated override from repo config. `resolveRepoLocalOverride()` fails loudly on absolute paths, `..` escapes, and resolve-escape edge cases (Fail Fast — no silent default fallback when the config is syntactically wrong). - `WorktreeProvider.create()` now loads repo config exactly once and threads it through `getWorktreePath()` + `createWorktree()`. Replaces the prior swallow-then-retry pattern flagged on #1117. `generateEnvId()` is gone — envId is assigned directly from the resolved path (the invariant was already documented on `destroy(envId)`). Tests (packages/git + packages/isolation): - Update the pre-existing `getWorktreeBase` / `isProjectScopedWorktreeBase` suite for the new two-layout return shape and precedence. - Add 8 tests for `worktree.path`: default fallthrough, empty/whitespace ignored, override wins for workspace-scoped repos, rejects absolute, rejects `../` escapes (three variants), accepts nested relative paths. Docs: add `worktree.path` to the repo config reference with explicit precedence and the `.gitignore` responsibility note. Co-authored-by: Joel Bastos <joelsb2001@gmail.com>
Introduces a declarative top-level `worktree:` block on a workflow so
authors can pin isolation behavior regardless of invocation surface. Solves
the case where read-only workflows (e.g. `repo-triage`) should always run in
the live checkout, without every CLI/web/scheduled-trigger caller having to
remember to set the right flag.
Schema (packages/workflows/src/schemas/workflow.ts + loader.ts):
- New optional `worktree.enabled: boolean` on `workflowBaseSchema`. Loader
parses with the same warn-and-ignore discipline used for `interactive`
and `modelReasoningEffort` — invalid shapes log and drop rather than
killing workflow discovery.
Policy reconciliation (packages/cli/src/commands/workflow.ts):
- Three hard-error cases when YAML policy contradicts invocation flags:
• `enabled: false` + `--branch` (worktree required by flag, forbidden by policy)
• `enabled: false` + `--from` (start-point only meaningful with worktree)
• `enabled: true` + `--no-worktree` (policy requires worktree, flag forbids it)
- `enabled: false` + `--no-worktree` is redundant, accepted silently.
- `--resume` ignores the pinned policy (it reuses the existing run's worktree
even when policy would disable — avoids disturbing a paused run).
Orchestrator wiring (packages/core/src/orchestrator/orchestrator-agent.ts):
- `dispatchOrchestratorWorkflow` short-circuits `validateAndResolveIsolation`
when `workflow.worktree?.enabled === false` and runs directly in
`codebase.default_cwd`. Web chat/slack/telegram callers have no flag
equivalent to `--no-worktree`, so the YAML field is their only control.
- Logged as `workflow.worktree_disabled_by_policy` for operator visibility.
First consumer (.archon/workflows/repo-triage.yaml):
- `worktree: { enabled: false }` — triage reads issues/PRs and writes gh
labels; no code mutations, no reason to spin up a worktree per run.
Tests:
- Loader: parses `worktree.enabled: true|false`, omits block when absent.
- CLI: four new integration tests for the reconciliation matrix (skip when
policy false, three hard-error cases, redundant `--no-worktree` accepted,
`--no-worktree` + `enabled: true` rejected).
Docs: authoring-workflows.md gets the new top-level field in the schema
example with a comment explaining the precedence and the `enabled: true|false`
semantics.
resolveRepoLocalOverride was hardcoding '/' as the separator in the startsWith check, so on Windows (where `resolve()` returns backslash paths like `D:\Users\dev\Projects\myapp`) every otherwise-valid relative `worktree.path` was rejected with "resolves outside the repo root". Fixed by importing `path.sep` and using it in the sentinel. Fixes the 3 Windows CI failures in `worktree.path repo-local override`.
Wirasm
force-pushed
the
feat/worktree-path-and-enabled-policy
branch
from
0bb458b to
cd5c465
Compare
4 tasks
7 tasks
1 task
2 tasks
Wirasm
added a commit
that referenced
this pull request
Apr 22, 2026
) The two "Smoke-test Claude binary-path resolver" steps in .github/workflows/release.yml run `archon workflow run archon-assist "hello"` against a fresh `git init` temp repo with no origin. As of #1310's worktree-policy changes, default isolation auto-syncs the worktree with origin before creating it, which fails with "neither origin/HEAD nor origin/main exist" — hit before Claude's resolver is even reached, so the test assertions ("Claude Code not found", "CLAUDE_BIN_PATH") never match and the linux-x64 build aborts the whole release matrix. The tests exercise the Claude resolver path, not worktree setup, so --no-worktree is the correct fix: it short-circuits validateAndResolveIsolation and skips the origin sync entirely. Matches the documented usage in CLAUDE.md (`archon workflow run quick-fix --no-worktree "Fix typo"`). Surfaced while cutting v0.3.8 — the release CI failed deterministically on both builds. Binaries themselves are fine (the v0.3.7 Pi-lazy-load fix works; local pre-flight passed on --help). v0.3.8's GitHub Release has been deleted so `releases/latest` falls back to v0.3.6; next release will be v0.3.9 with this fix.
Merged
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
repo-triageneeded every caller (CLI/web/chat/scheduled trigger) to remember to opt out of worktree creation. No declarative way for a workflow to pin "I always run live."worktree.path). The legacy~/.archon/worktrees/<owner>/<repo>/fallback is removed; every repo resolves to~/.archon/workspaces/<owner>/<repo>/worktrees/by default regardless of how it was registered.worktree.pathto.archon/config.yaml— validated repo-relative path, opt-in co-location (e.g..worktreeslives at<repoRoot>/.worktrees/<branch>).worktree.enabled: true | falseto workflow YAML — pins isolation regardless of invocation surface. Hard-errors on conflict with CLI flags. First consumer:.archon/workflows/repo-triage.yaml(enabled: false).--no-worktree,--branch,--fromstill work); no new env vars; no migration of already-created worktrees (existing paths remain reachable through storedworking_path); no changes to isolation adapters outsideWorktreeProvider; no web UI changes beyond what generated types produce.UX Journey
Before
After
Architecture Diagram
Before
After
Connection inventory:
getWorktreeBase()return type{ base, layout }stringWorktreeBaseOverride.repoLocalgetWorktreeBase()WorktreeCreateConfig.pathRepoConfigLoader→WorktreeProvider.archon/config.yaml worktree.pathWorkflowBase.worktree.enabledworkflow.tsschema + loaderworkflow.worktree.enabledworkflow.worktree.enableddispatchOrchestratorWorkflowextractOwnerRepo(repoPath)resolveOwnerRepo()fallback~/.archon/worktrees/fallbackgenerateEnvId()envId = worktreePathLabel Snapshot
risk: medium(changes worktree base resolution semantics for locally-registered repos — existing worktrees remain accessible via stored paths, but new worktrees for such repos go to workspace-scoped locations)size: Misolation,workflows,cli,core,docsisolation:worktree-provider,git:worktree-base,workflows:schema,workflows:loader,cli:workflow-cmd,core:orchestrator-agent,core:config-typesChange Metadata
featuremultiLinked Issue
Co-authored-byon commit 1)Validation Evidence (required)
bun run validate # ✅ check:bundled + type-check + lint + format + test (all 10 packages)Targeted suites:
@archon/git— 142 tests pass (getWorktreeBasereturn-shape migration + 2 new repo-local override tests +isProjectScopedWorktreeBasedeprecation notes)@archon/isolation— 244+ tests pass; 8 new tests forworktree.path: custom path, empty/whitespace ignored, null/undefined fallback, override-wins-over-workspace, rejects absolute path, rejects..escapes (3 variants), accepts nested relative@archon/workflows— 149+ tests pass; 3 new loader tests forworktree.enabled: true | false | omitted@archon/cli— 84 tests pass (was 79); 5 new reconciliation tests for policy vs flag combinationsE2E smoke (run manually from repo root):
Security Impact (required)
NoNoNo~/.archon/). Users opting in viaworktree.pathare implicitly trusting Archon to write into their repo tree; we validate the config value rejects../absolute paths so a hostile or typo'd value can't escape the repo root. No automatic.gitignoremutation — users manage that themselves.Compatibility / Migration
worktree.pathin their config continue to get worktrees under~/.archon/workspaces/owner/repo/worktrees/. This was already the case for workspace-cloned repos.~/.archon/workspaces/owner/repo/worktrees/instead of~/.archon/worktrees/owner/repo/. Existing worktrees under the old path are still reachable via their storedworking_path(no DB migration needed); Archon just won't create new ones there.Yes— two new optional fields:worktree.path(repo config) andworktree.enabled(workflow YAML).NoHuman Verification (required)
bun run validatepasses locally (type-check, lint, format, tests, bundled defaults).e2e-worktree-disabledruns and producesPASS: ran in live checkout (no worktree created by policy). Verified viapwdassertion inside a bash node.e2e-deterministic --no-worktreestill passes (existing smoke, no regression).archon workflow run e2e-worktree-disabled "smoke" --branch feat-xexits non-zero with the expected policy-mismatch message.worktree.pathvalues (absolute,.., nested../) — all rejected with actionable messages.worktree.enabled+--resume— policy is ignored on resume (documented in code comment).worktree.enabledvalues.bun --filter @archon/web generate:types— requires the dev server running. Not strictly necessary: webHealthResponse/WorkflowDefinitiontypes do not yet reference the new optional fields, and backend continues to validate on submission. Will regenerate separately when touching other web-visible API surface.worktree.enabled: false. The CLI path is exercised by the e2e smoke + tests; the orchestrator path is covered by the new guard + existing mocks, but not by a live end-to-end run.Side Effects / Blast Radius (required)
WorktreeProvider(layout + config load + envId assignment),@archon/git/worktree(primitive signature + tests),core/config(RepoConfig.worktree.pathfield), workflow loader + schema (worktree.enabled), CLI workflow command (reconciliation), orchestrator dispatch (short-circuit),.archon/workflows/repo-triage.yaml(first consumer).~/.archon/worktrees/. Flagged above under Migration..archon/config.yaml worktree.pathnow surface at worktree creation instead of defaulting silently. This is intentional (Fail Fast / CLAUDE.md), but it does mean a typo breaks runs rather than being ignored.'worktree.*_failed'structured log events fire on any error fromresolveRepoLocalOverride(). New'workflow.worktree_disabled_by_policy'log event makes the orchestrator short-circuit observable.Rollback Plan (required)
git revertthe two commits. Both are cohesive and self-contained..archon/config.yaml/ workflow YAML, so not setting those fields returns you to default behavior..archon/config.yaml worktree.path must be …errors at run time, repo-triage creating a worktree instead of running live.Risks and Mitigations
~/.archon/worktrees/owner/repo/<branch>will land new worktrees in~/.archon/workspaces/owner/repo/worktrees/<branch>. Existing worktrees remain accessible (storedworking_pathis absolute), butarchon isolation listmay present a mixed tree for a transition period.archon isolation cleanupthe old ones.worktree.path: .worktreeswithout.gitignoreing the directory will see "untracked files" when runninggit status.configuration.md. Explicit non-goal of this PR — Archon doesn't mutate user-owned files.worktree.enabled: trueis a no-op ceiling on surfaces that don't have a--no-worktreeequivalent (web UI, most chat adapters). Explicit in docs.Credit to @joelsb for the repo-local worktree idea in #1117. Commit 1 carries a
Co-authored-by:trailer.Summary by CodeRabbit
New Features
Documentation
Tests