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 (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThe CLI now captures repository auto-registration failures caused by stale workspace source symlinks and rethrows them as "Cannot {create worktree,resume}: repository registration failed." with a targeted cleanup hint (pointing to the stale workspace path or the workspaces directory) instead of the previous generic "not in a git repository" message. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant CLI as Archon CLI
participant Reg as Repository Registration
participant FS as Filesystem
participant Git as Git detection
User->>CLI: run `archon workflow run` (or `--resume`)
CLI->>Reg: registerRepository(...) (auto-registration)
Reg->>FS: check/create workspace `.../source` symlink
FS-->>Reg: throws stale-symlink error (createProjectSourceSymlink shape)
Reg-->>CLI: throws registration error
CLI->>CLI: extractStaleWorkspaceEntry(error.message)
CLI-->>User: "Cannot {create worktree,resume}: repository registration failed." + cleanup hint (stale path or workspaces dir)
CLI->>Git: proceed to git detection only if registration succeeded
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
![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
🤖 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/cli/src/commands/workflow.ts`:
- Around line 116-124: buildRegistrationFailureError currently calls
getArchonHome() unguarded which can throw and mask the original registration
error; change it to compute the fallback path inside a try/catch and only
include the home-based hint if getArchonHome() succeeds, otherwise fall back to
a safe generic hint that does not call getArchonHome() or throw; ensure the
returned Error always contains the original error.message and never replaces it
with a secondary home-resolution error (refer to buildRegistrationFailureError,
getArchonHome, extractStaleWorkspaceEntry).
🪄 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: 8c095d7b-ffd9-4c98-942c-10b328ca351e
📒 Files selected for processing (3)
CHANGELOG.mdpackages/cli/src/commands/workflow.test.tspackages/cli/src/commands/workflow.ts
Collaborator
Author
Multi-Agent Review Summary — PR #1332Five specialist agents reviewed the diff. Scope: 3 files (~96 LoC), one focused CLI error-classification change plus test and CHANGELOG. Verdict: NEEDS FIXES (non-blocking)No critical issues. One meaningful behavioral gap (untested resume-path wiring) and one stale docs page that undermines the fix's user-facing value. Otherwise the change is clean. Critical Issues (0)None. Important Issues (3)
Suggestions (4)
Strengths
Out-of-Scope Observations (not blocking)
Recommended Actions (in order)
Multi-agent review: code-reviewer, pr-test-analyzer, silent-failure-hunter, comment-analyzer, docs-impact (parallel). |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/cli/src/commands/workflow.test.ts (1)
868-973: New regression coverage looks solid.Worktree-path, resume-path, and unrecognized-shape fallback are all exercised, the negative assertions correctly track the
"not in a git repository"/"Not in a git repository"casing in the two throw sites, and mocks stay within the existing process-globalmock.module()pattern (nomock.restore()misuse introduced).One optional follow-up (non-blocking): the parser itself (
extractStaleWorkspaceEntry) is only tested indirectly throughworkflowRunCommand. If you want cheap coverage for Windows separators, missing delimiter, and no-separator inputs, export it from./workflowand table-test it directly — otherwise those branches will only break loudly if the main integration paths happen to exercise them.🤖 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 868 - 973, The tests only exercise extractStaleWorkspaceEntry indirectly via workflowRunCommand; export extractStaleWorkspaceEntry from ./workflow and add targeted unit tests that table-drive Windows path separators, inputs missing the expected delimiter, and inputs with no separator to cover those branches directly (create tests referencing extractStaleWorkspaceEntry so failures are pinpointed rather than relying on the larger integration flow).
🤖 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/docs-web/src/content/docs/getting-started/overview.md`:
- Around line 489-493: Add a language identifier to the fenced code block
containing the "Cannot create worktree: repository registration failed." example
by changing the opening fence from ``` to ```text so the block is fenced as
```text; locate the block that begins with the exact text "Cannot create
worktree: repository registration failed." in overview.md and update only the
opening fence to include "text" (no other content changes).
---
Nitpick comments:
In `@packages/cli/src/commands/workflow.test.ts`:
- Around line 868-973: The tests only exercise extractStaleWorkspaceEntry
indirectly via workflowRunCommand; export extractStaleWorkspaceEntry from
./workflow and add targeted unit tests that table-drive Windows path separators,
inputs missing the expected delimiter, and inputs with no separator to cover
those branches directly (create tests referencing extractStaleWorkspaceEntry so
failures are pinpointed rather than relying on the larger integration flow).
🪄 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: a9c23b96-b232-4c6c-8769-6dcc192d0fce
📒 Files selected for processing (3)
packages/cli/src/commands/workflow.test.tspackages/cli/src/commands/workflow.tspackages/docs-web/src/content/docs/getting-started/overview.md
Comment on lines
489
to
493
| ``` | ||
| Source symlink at ~/.archon/workspaces/.../source already points to <old-path>, expected <new-path> | ||
| Cannot create worktree: repository registration failed. | ||
| Error: Source symlink at ~/.archon/workspaces/<owner>/<repo>/source already points to <old-path>, expected <new-path> | ||
| Hint: Remove the stale workspace entry at ~/.archon/workspaces/<owner>/<repo> and retry, or use --no-worktree to skip isolation. | ||
| ``` |
There was a problem hiding this comment.
Add a language to the fenced code block.
Static analysis (markdownlint-cli2 MD040) flags the example block as missing a language identifier. Use text (or similar) so the CI lint stays clean and the code fence gets consistent rendering with the rest of the doc.
📝 Proposed fix
-```
+```text
Cannot create worktree: repository registration failed.
Error: Source symlink at ~/.archon/workspaces/<owner>/<repo>/source already points to <old-path>, expected <new-path>
Hint: Remove the stale workspace entry at ~/.archon/workspaces/<owner>/<repo> and retry, or use --no-worktree to skip isolation.</details>
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.22.0)</summary>
[warning] 489-489: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against the current code and only fix it if needed.
In @packages/docs-web/src/content/docs/getting-started/overview.md around lines
489 - 493, Add a language identifier to the fenced code block containing the
"Cannot create worktree: repository registration failed." example by changing
the opening fence from totext so the block is fenced as ```text; locate
the block that begins with the exact text "Cannot create worktree: repository
registration failed." in overview.md and update only the opening fence to
include "text" (no other content changes).
</details>
<!-- fingerprinting:phantom:medusa:nectarine:40625e77-8497-433d-a84f-91a7b1f27590 -->
<!-- This is an auto-generated comment by CodeRabbit -->
…"not a git repo"
When workflowRunCommand auto-registers an unregistered repo, a stale
~/.archon/workspaces/<owner>/<repo>/source symlink (pointing to an old
checkout) causes createProjectSourceSymlink() in @archon/paths to throw:
Source symlink at <linkPath> already points to <existing>, expected <target>
The CLI caught that in a try/catch, logged it at warn level, continued
with `codebase = null`, and then the isolation / resume branches hit
their "codebase missing" fallback and threw the generic:
Cannot create worktree: not in a git repository.
That message is false — the repo is valid; the Archon workspace entry
is stale. It sends users down the wrong diagnostic path (checking git
config, permissions, etc.) instead of pointing at the workspace dir.
Fix: preserve the registration error on a new `codebaseRegistrationError`
local, and at both fallback sites (resume + worktree-creation) check it
before the generic "not a git repo" branch. When set, throw a truthful:
Cannot {create worktree,resume}: repository registration failed.
Error: <original message>
Hint: Remove the stale workspace entry at <dir> and retry, or
use --no-worktree to skip isolation.
The hint's exact path comes from a small parser that extracts the
workspace directory from the known "Source symlink at …" format; when
the message shape doesn't match (future error text changes), the parser
returns null and we fall back to a generic "check registration under
<archon-home>/workspaces" hint — safe degradation.
Regression test in workflow.test.ts asserts the new error message and
negatively asserts the old "not in a git repository" string is gone.
Supersedes #1157 — that PR was draft + CONFLICTING against current dev,
and also mentioned Windows test-compat changes that weren't in the diff
(pruned scope). This is a fresh re-do focused strictly on #1146.
Closes #1146.
Co-authored-by: Bortlesboat <Bortlesboat@users.noreply.github.com>
…ing docs Addresses multi-agent review feedback on this PR: - Add regression test for the --resume fallback site (the worktree-create site was already covered; the resume site had identical wiring but zero test coverage). - Add test for the unrecognized-error-shape branch of buildRegistrationFailureError so the generic workspace hint is pinned (prevents accidental inversion of the stale-entry vs generic-hint ternary). - Update the troubleshooting page to key on the new "Cannot create worktree: repository registration failed." message. Users hitting the new error won't find the page under the old heading, and the "In the future..." note is obsolete now that the error itself contains the cleanup path. - Trim both new docblocks: keep the load-bearing cross-package error string contract in extractStaleWorkspaceEntry, drop narration of what the code already shows. Drop the "Before this helper existed..." paragraph from buildRegistrationFailureError — that's CHANGELOG material. Drop PR-reference suffix from the test section divider.
Two follow-up fixes to the multi-agent review commit (f32f002): CodeRabbit finding — unguarded getArchonHome() in the fallback hint. If getArchonHome() ever throws (misconfigured env vars, permission issues on the resolution path), the registration-failure Error would never get constructed: we'd throw a secondary home-resolution error that masks the root cause. Wrap the fallback branch in try/catch — prefer losing the exact path in the hint over replacing the actionable registration error. A safe generic hint ("Check your Archon workspace registration and retry") takes over when getArchonHome() throws. The original error.message is always embedded verbatim in the re-thrown Error. S2 — export extractStaleWorkspaceEntry for direct table tests. The parser is where the cross-package string contract with @archon/paths actually lives; direct tests against it are cheaper than end-to-end CLI tests and pin the edge cases: - POSIX path with forward slashes (typical unix user) - Windows path with backslashes (verifies Math.max(lastIndexOf / , lastIndexOf \)) - Unrelated error message (no prefix) → null - Prefix matches but delimiter missing → null - Source path without any separator → null (guards against returning empty string, which would produce a nonsense "Remove the stale workspace entry at " hint) - Empty string → null Six new cases in the test file. The claim of Windows support in the PR description is now actually verified.
Wirasm
force-pushed
the
fix/cli-stale-workspace-registration-error
branch
from
f32f002 to
28d007c
Compare
Collaborator
Author
|
Pushed `28d007cf` (review-response f32f002 from yesterday + new commit c18ce3ce today) and rebased on current `dev` — CHANGELOG conflict resolved. Important (3/3)
CodeRabbit (1/1)
Suggestions
`bun run validate` green locally; 93 CLI tests pass (6 new `extractStaleWorkspaceEntry` direct tests + 2 resume/null-fallback integration tests from the prior review commit). |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/cli/src/commands/workflow.test.ts (1)
2525-2567: Parser edge-case coverage looks solid.Good coverage of POSIX, Windows backslash separators, unrelated error prefix, missing delimiter (truncated), no-separator path, and empty-string inputs — matches the four early-return branches in
extractStaleWorkspaceEntry. Exporting the parser and table-testing it here is exactly what S1/S2 asked for.Minor nit (optional): the repeated
const { extractStaleWorkspaceEntry } = await import('./workflow');inside everyit()could be hoisted to abeforeAllor a top-levelletsince the module mocks are process-global — not a correctness issue.🤖 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 2525 - 2567, Tests repeatedly import extractStaleWorkspaceEntry inside each it() which is redundant; hoist the import once to avoid repetition by moving the dynamic import into a beforeAll or by declaring a top-level let and assigning it in a single beforeAll, referencing the function name extractStaleWorkspaceEntry and the test suite describe('extractStaleWorkspaceEntry', ...) so all specs reuse the same imported symbol without repeated await import calls.
🤖 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/cli/src/commands/workflow.test.ts`:
- Around line 2525-2567: Tests repeatedly import extractStaleWorkspaceEntry
inside each it() which is redundant; hoist the import once to avoid repetition
by moving the dynamic import into a beforeAll or by declaring a top-level let
and assigning it in a single beforeAll, referencing the function name
extractStaleWorkspaceEntry and the test suite
describe('extractStaleWorkspaceEntry', ...) so all specs reuse the same imported
symbol without repeated await import calls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ea6fc43f-78d1-4e29-a4f0-a04d414f2ea0
📒 Files selected for processing (4)
CHANGELOG.mdpackages/cli/src/commands/workflow.test.tspackages/cli/src/commands/workflow.tspackages/docs-web/src/content/docs/getting-started/overview.md
🚧 Files skipped from review as they are similar to previous changes (2)
- CHANGELOG.md
- packages/cli/src/commands/workflow.ts
Windows test runner (CI) hit: Expected to contain: "Check your Archon workspace registration under /home/test/.archon/workspaces" Received: "... under \home\test\.archon\workspaces and retry, ..." path.join normalizes to `\` on Windows and `/` on POSIX. The test hardcoded forward slashes in the expected substring. Split into two separator-agnostic asserts: the prefix up to "under", then `/workspaces\b/` regex for the final path segment. Behavior doesn't change — the hint still gets the full path.join'd workspaces dir on either platform.
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.
Fresh re-do of #1157 (closed — draft + CONFLICTING, plus out-of-scope Windows test-compat changes mentioned in the body but not actually present). Full credit to @Bortlesboat via `Co-authored-by:`.
Problem (#1146)
`archon workflow run` (or `--resume`) runs from inside a perfectly valid git repo. Auto-registration kicks in, trips over a stale `~/.archon/workspaces///source` symlink that points at some old checkout, and `createProjectSourceSymlink()` in `@archon/paths` throws:
```
Source symlink at already points to , expected
```
The CLI caught that error, logged it at warn level, continued with `codebase = null`, and then the isolation / resume fallbacks threw:
```
Cannot create worktree: not in a git repository.
```
That message is wrong. The repo is fine — the Archon workspace entry is stale. It sends users hunting for missing `.git` dirs, checking git permissions, reinstalling git — all wasted effort.
Fix
```
Cannot {create worktree,resume}: repository registration failed.
Error:
Hint: Remove the stale workspace entry at and retry,
or use --no-worktree to skip isolation.
```
Test
`packages/cli/src/commands/workflow.test.ts` — new regression test `surfaces auto-registration failures instead of claiming the repo is invalid`:
Validation
Out of scope
The original #1157 body mentioned test-compat tweaks in `packages/paths/src/env-integration.test.ts` and `packages/core/src/providers/claude.test.ts` for Windows `Path` / `USERPROFILE` env keys, but those changes weren't actually in the diff. Left out of this re-do — if the Windows issue is still real, a follow-up PR is the right home for it.
Closes #1146.
Co-authored-by: Bortlesboat Bortlesboat@users.noreply.github.com
Summary by CodeRabbit