Repository navigation
fix(daemon): count non-empty segments for project-root depth - #857
Conversation
Project selection's working-directory step measured a declared root's depth as root.split(path.sep).length, which counts the trailing empty segment that only the filesystem root produces. "/" and "/a" both measured 2, so a catalog declaring "/" beside "/a" reported a tie from a working folder inside "/a" and stopped with project_required, contradicting the documented "deepest declared root that contains the working directory" rule. Counting non-empty segments makes the code match the rule. Relative ordering among non-root paths is unchanged, since every one shifts down by exactly one; the only selection outcome that changes is a catalog that declares "/" itself as a project root. A genuine tie between two entries sharing the deepest root still stops with project_required. The fix serves rig context work-install and operating posture, which both call inferCatalogProject. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe project catalog now measures working-directory containment depth by counting nonempty path components. Tests cover root selection, tied deepest roots, and nested roots. ChangesProject catalog selection
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to Project selection appears ready to merge after normal checks; no unresolved issue is identified in this change. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
|
Thanks, @mv-schwarz, for counting only non-empty segments for project-root depth, so a catalog that declares |
openrig-review
left a comment
openrig-review
left a comment
There was a problem hiding this comment.
Approved at 75ead3f. A one-line fix: project-root depth now counts non-empty path segments. Before, the filesystem root / measured the same depth as /a, so a catalog declaring both reported a tie from inside /a and asked for --project, although the documented rule (the deepest declared root that contains the working directory) already answers it. Ordering among all other roots is unchanged, a genuine tie still returns project_required, and the change adds no refusal while removing a spurious one. Mocking node:fs in the regression test is the right call, since / cannot be built as a real temp root, and its two other cases guard the tie and nested ordering. Thank you for the clear verification notes, including the pre-existing test failures on your machine.
— dev60-planner@v-openrig-build
|
Merged. Thanks, @mv-schwarz, for counting only non-empty segments for project-root depth. It's on main now, not in a release yet. |
3 participants
What a user gets
Project selection's working-directory step is documented as picking "the deepest declared
project root that contains the working directory". It measured a root's depth as
root.split(path.sep).length, which counts the trailing empty segment that only the filesystemroot produces, so
/and/aboth measured 2./beside/a, resolved from a working folder/a/b, reporteda tie and stopped with
project_required— demanding--projectfor a case the documented rulealready answers.
/awins.Counting non-empty segments shifts every non-root path down by exactly one, so relative ordering
among them is unchanged. The only selection outcome that changes is a catalog that declares
/itself as a project root. A genuine tie between two entries sharing the deepest root still stops
with
project_required.One line of behaviour change, plus a regression test. It serves both callers of
inferCatalogProject:rig context work-installand operating posture.No issue was filed for this; it was reported directly.
How you verified it
Revision:
75ead3fce50a7faee97491d3e32b1bf526429579onfix/catalog-project-root-depth, parent09384907. Every check below ran atfd4b9e1d, which was then amended for its commit messageonly;
git diff fd4b9e1d 75ead3fcis empty and both commits carry the identical treef70491dae309379e44b50f2b8fdd436ac64cc842, so the verification transfers unchanged.The new test was run red against the unfixed line first:
Tests 1 failed | 2 passed (3), thefailure thrown at
project-catalog.ts:118with "the working directory is inside several projectswith the same root". The other two cases were green pre-fix, so they are genuine guards rather
than restatements of the fix. Post-fix:
3 passed (3).npm testexits 1, and I am not going to present that as a pass. Three tests fail, none ofthem on this change's path:
staging-docker-invoker.test.ts×2 — "Test timed out in 20000ms", on the two step-timeout casesthat spawn
sleep 30against a 300 ms timeoutterminal-pipe-utf8.test.ts×1 — "native output timed out"All three reproduce identically at the parent commit
09384907(Tests 3 failed | 6 passed (9)), so they are independent of this change. That was established by effect in four independentruns — the author's, both reviewers', and once more in a clean checkout at the parent — not by
arguing that path arithmetic cannot reach docker timeouts. They were also initially assumed to be
load-sensitive; that hypothesis was tested and disproved, since they still fail when run
focused at low load. They are reproducible pre-existing failures on this host. Whether they also
fail in CI is not something this change establishes.
Per leg, at the reviewed tree:
daemon:Test Files 2 failed | 891 passed | 2 skipped (895),Tests 3 failed | 13053 passed | 6 skipped | 5 todo (13067)cli:Test Files 243 passed (243),Tests 3455 passed | 3 skipped (3458)tui:Test Files 98 passed (98),Tests 853 passed (853)npm run buildnpm run lintnpm test(full suite)packages/daemon/test/project-catalog-root-depth.test.tsproject-registration+project-navigation+bundle-routespackages/cli/test/context-work-install.test.tsnpm run buildpass,npm run lintpass, focused Vitest 5 files / 135 tests passnpm ci,npm run build(four workspaces),npm run lintall pass; focused Vitest 5 files / 135 tests pass; repo checks 255 pass / 1 opt-in skip / 0 failThree further failures showed up in earlier runs, were diagnosed to the test environment rather
than the diff, and do not appear in the final run above once their causes were corrected. They
are recorded here only so the earlier red runs are accounted for:
100755materialise on disk as775under thishost's
umask 0002— git preserves only the executable bit, so0777 & ~0002 = 0775. A testasserting the exact mode fails. None of the 23 is in this diff, and the failure reproduced at the
parent commit. Fixed by restoring the recorded modes.
tmux attachment — the documented "Running inside a seat" guard in
docs/as-built/test-layers.md, behaving correctly and protecting the running rig. Fixed byunsetting
TMUX./healthzcheck at 305.2 ms against a 250 ms budget, while the host sat atload 8.4–10.4 on 5 cores from concurrent test runs. Cleared on a focused rerun.
Not run, and why: no live-model evals and no real-provider runs (
OPENRIG_E2E_REAL_CODEX,OPENRIG_REAL_CLAUDE_INTEGRATIONboth unset) — they spend provider credits and prove nothingabout a path-arithmetic fix. No stub scenario and no
npm run test:ui: catalog project selectionis not a
docs/as-built/arteries.mdrow,packages/uiis untouched, and this change reaches noneof message delivery, launch and resume, the queue, rig identity, skill projection, migrations or
restore. No dependency, bin, build-script or shipped-file change, so no packaging layer. No daemon
restart.
Docs:
docs/reference/project-workspace.md:124anddocs/as-built/architecture/workspace-primitive.md:327both state the rule as the deepest rootcontaining the working directory without committing to a mechanical depth measure, so the fix
makes the code match the prose and needs no doc edit. The tie wording at
project-workspace.md:143-144is what the second test case pins.Anything you were unsure about
The regression test mocks
node:fsrather than building a fixture on disk. That is deliberate andis the part worth looking at hardest: the case under test is
/as a declared root, which cannotbe constructed from a real temp directory, and a test that used a real top-level folder such as
/tmpor/privateas a project root would depend on the machine's layout. It uses therepository's established hoisted
importOriginalpassthrough (the shape inslice-indexer.test.ts), overrides only the three calls this path makes —existsSync,readFileSync,realpathSync— and falls through to the real implementation for any path outsidethe fixed fakes. The paths are POSIX on purpose; the project targets macOS and Linux.
.filter(Boolean)was chosen over a named helper to match the file's existing one-liner style.Selection order and steps 1–6 are unchanged, and the change adds no refusal — it removes a
spurious
project_required.If this is security-related
Not security-related.
CHANGELOG.mdeditSummary by CodeRabbit