feat: add optional submodule initialization for worktrees#1189
Merged
Wirasm merged 6 commits intocoleam00:devfrom Apr 15, 2026
Merged
feat: add optional submodule initialization for worktrees#1189Wirasm merged 6 commits intocoleam00:devfrom
Wirasm merged 6 commits intocoleam00:devfrom
Conversation
) Repos with git submodules create worktrees with empty submodule directories because git worktree add does not initialize them by default. Add a configurable `worktree.initSubmodules` option in .archon/config.yaml that runs `git submodule update --init --recursive` after worktree creation when enabled. The command only runs when .gitmodules exists in the worktree (no overhead for repos without submodules) and is non-fatal — if submodule init fails (e.g., network issues), the worktree is still created with a warning logged. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Caution Review failedPull request was closed or merged during review Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds optional submodule initialization to worktree creation: provider probes for Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Provider as WorktreeProvider
participant FS as FileSystem
participant Git as Git CLI
Client->>Provider: create(worktreePath, config{initSubmodules})
Provider->>Provider: create branch / add worktree
Provider->>FS: access(worktreePath/.gitmodules)
alt .gitmodules missing (ENOENT)
FS-->>Provider: ENOENT
Provider->>Client: return active (no submodule init)
else probe error (EACCES/other)
FS-->>Provider: error
Provider->>Provider: log warning / throw on probe error
Provider->>Client: return/reject (probe error behavior)
else .gitmodules exists and initSubmodules !== false
FS-->>Provider: exists
Provider->>Git: git -C worktreePath submodule update --init --recursive (120s)
alt git succeeds
Git-->>Provider: success
Provider->>Client: return active
else git fails
Git-->>Provider: stderr/error
Provider->>Client: throw "Submodule initialization failed"
end
else initSubmodules === false
FS-->>Provider: (maybe exists)
Provider->>Client: return active (skipped by config)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 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 |
![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/isolation/src/providers/worktree.test.ts (1)
820-918: Strong coverage for the new config-driven submodule flow.These cases are well targeted. Consider adding one more case where
.gitmodulesaccess fails with non-ENOENT(e.g.,EACCES) to lock in fully non-fatal behavior.🤖 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 820 - 918, Add a test to verify non-ENOENT errors from checking .gitmodules are treated non-fatal: in the WorktreeProvider.create test suite, create a case similar to the ENOENT test where mockAccess.mockImplementation throws a NodeJS.ErrnoException with err.code = 'EACCES' when the path endsWith('.gitmodules'); call new WorktreeProvider(configLoader).create(baseRequest) and assert it does not throw, that execSpy has no 'submodule' calls, and that the returned env.status (from create) is 'active' to confirm the provider tolerates non-ENOENT access errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/isolation/src/providers/worktree.ts`:
- Around line 933-952: The .gitmodules existence probe in initSubmodules
currently can throw and escape the surrounding try/catch, making submodule init
fatal; modify initSubmodules to wrap the directoryExists(join(worktreePath,
'.gitmodules')) call in its own try/catch and treat any error the same as a
missing .gitmodules (i.e., log a debug/warn if you want and return early), so
only the execFileAsync('git', ['-C', worktreePath, 'submodule', 'update',
'--init', '--recursive'], ...) remains in the existing try/catch; reference the
initSubmodules method and the directoryExists call when applying the change.
---
Nitpick comments:
In `@packages/isolation/src/providers/worktree.test.ts`:
- Around line 820-918: Add a test to verify non-ENOENT errors from checking
.gitmodules are treated non-fatal: in the WorktreeProvider.create test suite,
create a case similar to the ENOENT test where mockAccess.mockImplementation
throws a NodeJS.ErrnoException with err.code = 'EACCES' when the path
endsWith('.gitmodules'); call new
WorktreeProvider(configLoader).create(baseRequest) and assert it does not throw,
that execSpy has no 'submodule' calls, and that the returned env.status (from
create) is 'active' to confirm the provider tolerates non-ENOENT access errors.
🪄 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: 8c1d26e1-2a6b-4f70-9a1b-213ba777475e
📒 Files selected for processing (4)
packages/core/src/config/config-types.tspackages/isolation/src/providers/worktree.test.tspackages/isolation/src/providers/worktree.tspackages/isolation/src/types.ts
Move .gitmodules access check inside try/catch so permission errors (EACCES) don't crash worktree creation. Use fs.access directly instead of the misnamed directoryExists helper. Add test for EACCES path. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
Author
QA Round 1 ReportCritical Issues
The Important Issues
Advisory Notes
VerdictCritical issue fixed. QA round 1 complete. 🤖 Generated with Claude Code |
Rename worktree.submodules_initialized to worktree.submodule_init_completed to pair with worktree.submodule_init_failed per CLAUDE.md convention. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
Author
QA Round 2 ReportFindings
Verified Clean
VerdictQA round 2: CLEAN — ready for review. 🤖 Generated with Claude Code |
This was referenced Apr 13, 2026
Two refinements on top of the original submodule support: 1. Default changed from opt-in to auto-detect. Repos with .gitmodules present now have submodules initialized by default, matching git's own convention (submodule.recurse / clone --recurse-submodules). The .gitmodules check is an access() probe — repos without submodules pay zero cost. Explicit opt-out remains via worktree.initSubmodules: false. 2. Submodule init failure now throws instead of logging a warning. A worktree with uninitialized submodules is a silent broken state — the same class of bug this feature exists to prevent. The thrown error flows through classifyIsolationError, which now has a dedicated pattern suggesting opt-out if submodules are not needed by workflows. EACCES on the .gitmodules read itself stays non-fatal — the git op would fail the same way and double-erroring adds no value. Tests updated: default-on behavior, explicit opt-in, explicit opt-out, throw-on-git-failure (replacing the warn-and-continue test), EACCES non-fatal path, and classifier coverage for the submodule pattern.
Collaborator
|
Hey @halindrome — thanks for this, it closes a real gap and fits naturally with the recent worktree-primitive hardening series (#1198, #1206, #1212). I pushed two small follow-ups directly to your branch ( 1. Default flipped from opt-in to auto-detectRepos with Explicit opt-out stays available via 2. Submodule init failure now throws instead of warningThis one is the Archon convention more than a universal truth: CLAUDE.md calls out "Prefer throwing early with a clear error for unsupported or unsafe states — never silently swallow errors". The prior behavior ( The thrown error flows through
EACCES on the Validation
Context on the broader classYour four-issue root-cause mapping (#1183, #1187, #1188, #1192) has been incredibly useful — it's directly shaped the four PRs we've merged. The submodule init gap is orthogonal to the identity/coordination class those addressed; your issue framing made that clean separation possible. #1192 (codebase identity derivation) remains the underlying primitive rearchitect that would retire most of the guard code; that's the next logical step and will be tracked separately. |
…e-init # Conflicts: # packages/isolation/src/errors.ts # packages/isolation/src/providers/worktree.test.ts
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/isolation/src/providers/worktree.test.ts (1)
848-851: Prefer reusing the submodule-call helper to avoid assertion drift.The first new test re-implements call filtering inline instead of reusing
countSubmoduleExecCalls(). Consolidating this keeps test intent consistent.🧹 Small cleanup
- const calls = execSpy.mock.calls.filter((call: unknown[]) => { - const args = call[1] as string[]; - return args.includes('submodule'); - }); - expect(calls).toHaveLength(1); - expect(calls[0][1]).toEqual( + expect(countSubmoduleExecCalls()).toBe(1); + const submoduleCall = execSpy.mock.calls.find((call: unknown[]) => { + const args = call[1] as string[]; + return args.includes('submodule') && args.includes('update'); + }); + expect(submoduleCall?.[1]).toEqual( expect.arrayContaining([ '-C', expect.any(String), 'submodule',🤖 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 848 - 851, The test duplicates logic for filtering execSpy calls for 'submodule' instead of reusing the existing helper countSubmoduleExecCalls(); update the test to call countSubmoduleExecCalls(execSpy) (or the helper's expected signature) rather than the inline filter so assertions use the centralized logic; locate the spot using execSpy and replace the inline filter block with the helper call to ensure consistent behavior across tests.packages/isolation/src/errors.ts (1)
26-27: Reduce drift risk by deriving known patterns fromerrorPatterns.The same literal patterns are now maintained in two places. Consider deriving
knownPatternsfromerrorPatternsso classification and known-error detection stay in sync.♻️ Suggested refactor
- const knownPatterns = [ - 'permission denied', - 'eacces', - 'timeout', - 'no space left', - 'enospc', - 'not a git repository', - 'branch not found', - 'no base branch configured', - 'submodule initialization failed', - ]; + const knownPatterns = errorPatterns.map(({ pattern }) => pattern);Also applies to: 100-110
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/isolation/src/errors.ts` around lines 26 - 27, The code duplicates pattern literals between errorPatterns and knownPatterns which causes drift; update the implementation so knownPatterns is derived from errorPatterns (e.g., map errorPatterns to their pattern strings) and replace any hard-coded duplicate arrays (including the similar block around the range referenced) with this single source of truth; ensure functions that consume knownPatterns (and any references to errorPatterns) use the derived array so classification and known-error detection remain in sync.
🤖 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/isolation/src/errors.ts`:
- Around line 26-27: The code duplicates pattern literals between errorPatterns
and knownPatterns which causes drift; update the implementation so knownPatterns
is derived from errorPatterns (e.g., map errorPatterns to their pattern strings)
and replace any hard-coded duplicate arrays (including the similar block around
the range referenced) with this single source of truth; ensure functions that
consume knownPatterns (and any references to errorPatterns) use the derived
array so classification and known-error detection remain in sync.
In `@packages/isolation/src/providers/worktree.test.ts`:
- Around line 848-851: The test duplicates logic for filtering execSpy calls for
'submodule' instead of reusing the existing helper countSubmoduleExecCalls();
update the test to call countSubmoduleExecCalls(execSpy) (or the helper's
expected signature) rather than the inline filter so assertions use the
centralized logic; locate the spot using execSpy and replace the inline filter
block with the helper call to ensure consistent behavior across tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f9bdbe38-2abf-460b-ba64-8e601eaf6c7d
📒 Files selected for processing (6)
packages/core/src/config/config-types.tspackages/isolation/src/errors.test.tspackages/isolation/src/errors.tspackages/isolation/src/providers/worktree.test.tspackages/isolation/src/providers/worktree.tspackages/isolation/src/types.ts
✅ Files skipped from review due to trivial changes (3)
- packages/isolation/src/types.ts
- packages/core/src/config/config-types.ts
- packages/isolation/src/providers/worktree.ts
Collaborator
PR Review Summary (multi-agent)Ran code-reviewer, docs-impact, pr-test-analyzer, silent-failure-hunter, and type-design-analyzer against the diff. Critical IssuesNone. Important Issues
Suggestions
Strengths
VerdictNEEDS FIXES — the EACCES silent-skip is the only material issue; the rest are polish. No critical blockers. Recommended Actions
|
…cs, dedupe Review feedback from multi-agent pass on PR coleam00#1189: 1. EACCES on .gitmodules read now throws instead of silently skipping. The prior non-fatal path produced a worktree that looked successful but had empty submodule dirs — the exact silent-broken-state this feature exists to prevent. "The git op would fail the same way" was wrong reasoning: surfacing one classified error beats surfacing zero. Only ENOENT (no submodules) remains non-fatal. 2. Add `worktree.initSubmodules` to docs/reference/configuration.md with behavior section explaining default-on semantic and opt-out. 3. Collapse errorPatterns and knownPatterns into a single ERROR_PATTERNS source with `known: boolean` per entry. Removes the drift risk flagged by the reviewer and makes the intentional divergence ("cannot extract owner/repo" is classifiable but NOT known — it crashes as a user-input bug) explicit. 4. Tests: assert --recursive in opt-in test (was only in default-on), flip EACCES test to assert throw, add timeout-path test.
This was referenced Apr 20, 2026
Merged
joaobmonteiro
pushed a commit
to joaobmonteiro/Archon
that referenced
this pull request
Apr 26, 2026
Worktrees created via `git worktree add` do not initialize submodules — monorepo workflows that need submodule content find empty directories. Auto-detect `.gitmodules` and run `git submodule update --init --recursive` after worktree creation; classify failures through the isolation error pipeline. Behavior: - `.gitmodules` absent → skip silently (zero-cost probe, no effect on non-submodule repos) - `.gitmodules` present → run submodule init by default (opt out via `worktree.initSubmodules: false`) - submodule init or `.gitmodules` read failure → throw with classified error including opt-out guidance - Only `ENOENT` on `.gitmodules` is treated as "no submodules"; other access errors (EACCES, EIO) surface as failures to prevent silent empty-dir worktrees Changes: - `packages/isolation/src/providers/worktree.ts` — `initSubmodules()` method + call site in `createWorktree()` - `packages/isolation/src/errors.ts` — collapsed `errorPatterns` + `knownPatterns` into single `ERROR_PATTERNS` source of truth with `known: boolean` per entry; added submodule pattern with opt-out guidance - `packages/isolation/src/types.ts` + `packages/core/src/config/config-types.ts` — new `initSubmodules?: boolean` config option - `packages/docs-web/src/content/docs/reference/configuration.md` — documented the new option and submodule behavior - Tests: default-on, explicit opt-in, explicit opt-out, skip-when-absent, fail-fast on EACCES, fail-fast on git failure, fail-fast on timeout Credit to @halindrome for the original implementation and root-cause mapping across coleam00#1183, coleam00#1187, coleam00#1188, coleam00#1192. Follow-up: coleam00#1192 (codebase identity rearchitect) would retire the cross-clone guard code in `resolver.ts` and `worktree.ts` that coleam00#1198, coleam00#1206 added. Separate PR. Closes coleam00#1187
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
3 participants
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
Fixes #1187
Adds a
worktree.initSubmodulesconfig option that runsgit submodule update --init --recursiveafter worktree creation. This ensures workflows operating on monorepos with submodules have access to submodule content in their isolated worktrees.Changes
packages/isolation/src/types.ts: AddinitSubmodules?: booleantoWorktreeCreateConfigpackages/core/src/config/config-types.ts: AddinitSubmodules?: booleantoRepoConfig.worktreepackages/isolation/src/providers/worktree.ts: AddinitSubmodules()method called aftergit worktree addwhen configured. Checks for.gitmodulesbefore running (no overhead for repos without submodules). Non-fatal — logs a warning if submodule init fails.packages/isolation/src/providers/worktree.test.ts: 5 new tests covering: init when enabled, skip when disabled, skip when not configured, skip when no.gitmodules, and graceful failure.Configuration
Design decisions
--recursiveto handle nested submodules.Test plan
bun run type-check— all packages passbun run lint— zero warningsbun run test— all tests pass (5 new tests for submodule init)worktree.initSubmodules: true, run a workflow, verify submodule dirs are populatedRelated
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes / Behavior
Tests
Documentation / Errors