feat(workspace): register the boot folder only while the registry is empty - #774
Conversation
…empty Starting cezar in a folder is an implicit "this is my project" only for the very first run. Once the registry holds anything, booting somewhere else serves that folder as before but no longer appends it behind the user's back; adding stays an explicit gesture (`cezar projects add`, the cockpit's Add project dialog), both of which keep using the path-shape guard alone. `shouldAutoRegisterProject` composes `shouldRegisterProject` with the new seed-once rule. An already-registered root still passes, so booting a known project keeps bumping `lastOpenedAt` and keeps handing the server its registry id. `CEZ_SINGLE_PROJECT=1` is exempt — there the launch context IS the project and `cezar projects list` reads its identity back out of the registry. Follow-on copy: the 409 on removing the boot project no longer claims it "re-registers itself at every start" (now false); it points at `cezar projects remove` instead, with the Settings pane tooltip, the remove dialog, README and the spec's boot flow updated to match.
📦 npm preview published —
|
|
🤖 |
pat-lewczuk
left a comment
pat-lewczuk
left a comment
There was a problem hiding this comment.
🔍 Code Review: feat(workspace): register the boot folder only while the registry is empty
🎯 Summary
The change introduces shouldAutoRegisterProject (packages/cezar/src/workspace/projects.ts:141), a boot-time guard that wraps the existing path-shape guard with a "seed once" rule: the boot repo is written to ~/.cezar/config.json only while the registry is empty, or when that root is already registered, with CEZ_SINGLE_PROJECT=1 exempt. Every boot path uses it (serve, run, and the single-project projects list in packages/cezar/src/index.ts:135,192,365), while the explicit gestures — cezar projects add and POST /api/v1/projects — keep calling the unchanged shouldRegisterProject, so an explicit add is still blind to how many projects exist. A follow-on copy fix retires the now-false "it re-registers itself at every start" wording from the 409 on removing the boot project, the Settings tooltip, the remove dialog, the README and the multi-project spec.
I reviewed the full diff (10 files: the guard and its tests, the two boot call sites, the server 409, the Settings pane and its test, README, CHANGELOG and the multi-project spec) plus the surrounding boot, routing and sidebar code that the new "unregistered boot root" state now flows through.
The core of the change is well built and I like it: the split between the path-shape guard and the boot guard is the right seam, the single-project exemption is exactly the constraint .ai/specs/2026-07-21-cez-single-project.md:26 warned about ("filtering inside registerProject() or shouldRegisterProject() would suppress the launch project's self-healing registration") and it is honoured by putting the filter in a third function instead, the already-registered-root case is handled in every spelling, and the five new tests cover each branch. The whole validation gate is green on this head.
What holds the review up is not the guard but its blast radius. Before this change, an unregistered boot root was a rare edge case (a task worktree, $HOME); after it, it is the ordinary state of every launch outside a registered project — and two properties that the codebase documents and the cockpit relies on were built for the old, rare case. Both are fixable with a small amount of code or documentation; neither is a design objection to the seed-once rule itself.
Verdict
❌ request changes — two majors, neither waived: the registry's documented self-healing rebuild is gone while three places (including BACKWARD_COMPATIBILITY.md) still promise it, and an unregistered boot folder has no reachable affordance anywhere in the cockpit, which contradicts the PR's own "serves that folder exactly as before" claim on the navigation side. There are no blockers, the validation gate is fully green, and both findings are addressable without changing the guard's design.
🧪 Validation Gate
Run inside an isolated worktree at the PR head (2c1b4f11) after npm ci.
| Command | Status | Notes |
|---|---|---|
npm run typecheck |
✅ PASS | Contract, client, server and web projects all clean (exit 0). |
npm test |
✅ PASS | 296 test files, 5290 tests passed, including the five new shouldAutoRegisterProject cases and the three retitled copy assertions. |
npm run test:unit |
✅ PASS | node:test core-module suite, 0 failures. |
npm run build |
✅ PASS | Server + web build, and check:pack reports "452 files, 88 under web/dist (shell + assets present)". |
npm run test:package |
✅ PASS | 12 packaged-CLI E2E tests passed. |
CI on the PR agrees: "Unit, build, E2E, and package", "Publish npm snapshot" and license/cla are all green, and nothing is pending.
Findings
⚠️ Major
1. The registry no longer rebuilds as projects are opened, but three places still say it does — and one of them is the compatibility contract.
packages/cezar/src/workspace/projects.ts:141 — the seed-once rule removes the property that made registry loss recoverable. Concretely: ~/.cezar/config.json is corrupted (or deleted), loadWorkspaceConfig degrades to in-memory defaults with an empty projects[] and deliberately leaves the bad file on disk. The next cezar serve now sees projects.length === 0, registers exactly one project — whichever repo the user happened to boot in — and that merge-write replaces the corrupt file. Every other project is then gone for good: opening them no longer puts them back, and the only recovery is remembering each root and running cezar projects add by hand. Before this change, each project re-registered itself the next time it was opened, which is precisely why losing the file was documented as recoverable.
Three places still assert the old behaviour and were not updated:
BACKWARD_COMPATIBILITY.md:151— "The registry is additive state that is written, never required — it rebuilds as projects are opened, so losing it is an inconvenience, not data loss". This is the protected-surface document for~/.cezar/config.json; the sentence is now false, and the "not data loss" conclusion rests on it.packages/cezar/src/workspace/config.ts:21— the same claim in the module docstring, as the stated justification for degrading a corrupt file to defaults..ai/specs/2026-07-20-multi-project-workspace.md:268and:526— the boot-warning note and the degradation table row ("Degrade to defaults + warning; registry rebuilds as projects are opened"). The PR updates the spec's Boot flow section but leaves these two, so the spec now contradicts itself.
Why it matters: CODE_REVIEW.md ranks graceful degradation second and state-file compatibility third, and a degradation path whose documented recovery no longer exists is exactly the case those rules cover. This is not a blocker — nothing inside a repo is lost, the roots are re-addable, and BACKWARD_COMPATIBILITY.md:156's explicit "Breaking:" list does not cover it — but it must not merge with the docs asserting the opposite.
Fix: update all three (four lines) to describe what the code now does, and say what the new recovery path is (cezar projects add <dir> per project). Optionally, consider a cheap mitigation so the guarantee is not simply dropped — for example, letting loadWorkspaceConfig tell shouldAutoRegisterProject that this read was a degraded one (corrupt file) rather than a genuinely empty registry, so a corrupt-config boot does not silently overwrite the file with a one-project registry. If you decide the dropped guarantee is acceptable, that is a legitimate call — but it should be stated in the changed docs rather than left implicit.
2. An unregistered boot folder is unreachable from the cockpit UI.
packages/web/src/components/app-shell-container.tsx:105,139 with packages/web/src/lib/last-location.ts:55,72 and packages/web/src/routes.tsx:250. The PR body says the boot folder is still "served exactly as before (its own tasks, its own .ai/cezar/, /p/<slug>/ bound to the boot context)". That is true of the API and the routing, but not of navigation, and three things compound:
- The sidebar is fed from the registry alone —
ProjectGroupsreceivesprojects.projects(app-shell-container.tsx:139, consumed atpackages/web/src/components/project-groups.tsx:167), so an unregistered boot project has no row. There is no other element in the shell that links to it. locationToSaverefuses to persist it:projectIsUsable(last-location.ts:55) requires the id to be inregistry.projects, so pages under the boot project's slug are never written tolastLocation(last-location.ts:72).cezar serveopenshttp://localhost:<port>(packages/cezar/src/index.ts:292,317) — the exact bare root — andLegacyPathRedirectrestores the savedlastLocationthere (routes.tsx:250). Since that saved location can only ever name a registered project, the user who runscezarin a scratch checkout is dropped into a different project entirely.
So the realistic sequence is: cd ~/scratch/thing && npx cezar → the browser opens on the project you were last in, the folder you just launched in is nowhere in the sidebar, and its pages can never become the restore target either. The only handle on it is typing /p/<would-be-slug>/ by hand, and nothing in the UI ever shows that slug. That is a materially worse outcome than the sidebar row the change set out to avoid.
A smaller related edge: with exactly one registered project plus an unregistered boot folder, app-shell-container.tsx:105 (registry.projects.length > 1) drops the grouped sidebar entirely, so the shell renders the flat single-project nav for the registered project while the repo chip (repoChipOf(health.data), line 113) names the boot folder — the chrome names one repo and the navigation belongs to another.
Fix — any of these would resolve it, and the first is the smallest: keep the boot project visible in the cockpit even when it is unregistered, by having GET /api/v1/projects include a synthetic, non-persisted entry for the boot root (flagged so Settings → Projects can render it as "not registered — Add project"), which restores both the sidebar row and projectIsUsable. Alternatively, teach projectIsUsable to accept registry.bootProject so the boot folder can at least be the restore target, and give the user a one-click "Add this project" affordance in the shell when the boot root is unregistered. Whichever you pick, the behaviour deserves a test in packages/web/src/lib/last-location.test.ts and the sidebar suite, since it is now the default path rather than an edge case.
🔹 Minor
3. The unregistered boot project's slug is recomputed per call and can silently change mid-session.
packages/cezar/src/server/server.ts:1129-1141 — resolveBootProject caches only on a registry hit (if (match) bootProjectCache = match.id); the allocateProjectSlug fallback is recomputed on every call against the live registry. With the boot root registered (the old normal), the cache always hit and the id was stable. Now the fallback is the normal path, and it is derived from data the user can change while the server runs: boot in /work/beta while the registry holds only alpha → the boot project is beta; the user then clicks Add project on some other folder also named beta, and registerProject — which dedupes only against registry ids, and does not know the live fallback — hands that project the slug beta. The next resolveBootProject call finds no match and returns beta-2. The boot project's URL has changed under the user, and an open tab on /p/beta/ now resolves to the newly added project instead.
Fix: cache the fallback for the process lifetime as well (assign it to bootProjectCache before returning), so the boot project's id is fixed at first resolution the way a registered one is. Reserving the boot fallback in allocateProjectSlug's taken set for API-side registrations would close the collision from the other end too.
4. packages/cezar/src/workspace/projects-cli.ts:141-142 still carries the "re-registers itself" claim this PR set out to retire.
The comment reads: "Removing the repo you normally serve is therefore allowed — and self-healing, since the next cezar serve in it registers it again (said in the note below)." After this change the next cezar serve in it does not register it again, because the registry is no longer empty. This is the same false statement the PR correctly fixed in packages/cezar/src/server/server.ts:2397-2409, the README, the Settings tooltip and the remove dialog — and it is the most load-bearing of the set, because the new 409 message now sends users to cezar projects remove specifically, so the wrong mental model sits directly on the newly blessed path. Removing the boot repo from the CLI is now a one-way action, and the comment should say so.
💅 Nit
5. packages/cezar/src/workspace/projects.ts:145,149 realpaths the root twice.
shouldRegisterProject already calls normalizeRoot(repoRoot) internally, and shouldAutoRegisterProject calls it again for the membership test — two realpath syscalls per boot on the same path. Purely cosmetic at this scale; if you want it tidy, have the shared helper return the normalized root (or accept one) so the boot guard reuses it.
💥 Breaking Changes
- No exported/public symbol removed or renamed without a deprecation path —
shouldRegisterProjectis still exported and still used byprojects-cli.tsand thePOST /api/v1/projectspath;shouldAutoRegisterProjectis purely additive. - No function signature changed in a breaking way — the new function's
envparameter is optional and defaults toprocess.env. - No required type field removed or narrowed.
- No HTTP route URL removed or renamed; no method changed for an existing operation.
- No field removed or retyped in an existing response shape —
GET /api/v1/projectskeeps its{projects, bootProject, projectsDir}shape; only which roots appear inprojects[]changes, andbootProjectwas already documented as possibly naming an unregistered root. - No event or message name renamed or removed; no payload field removed.
- No CLI command or flag renamed or removed; no machine-parsed output format changed. The 409 body on
DELETE /api/v1/projects/:projectIdis reworded, butBACKWARD_COMPATIBILITY.mdprotects the status/shape pair rather than the prose, and the tests that assert the wording were updated with it. - No database table or column renamed or removed; no column type narrowed.
- No config key renamed and no default changed silently — no key changed, and the behavioural default change is announced in the CHANGELOG's Unreleased → 🔧 Changed section, the README and the spec's Boot flow section.
- Where a contract had to change: old surface kept working through a deprecation window, with migration notes — not met.
BACKWARD_COMPATIBILITY.md:151's stated recovery property for~/.cezar/config.json("it rebuilds as projects are opened, so losing it is an inconvenience, not data loss") no longer holds and the document was not updated. See Major 1; that section is where the new recovery story (cezar projects addper project) needs to be written down.
🧪 Test Coverage
The new behaviour is well covered at the unit level. packages/cezar/src/workspace/projects.test.ts:283-310 adds five cases against shouldAutoRegisterProject — the first-project seed, suppression of an unknown root once anything is registered (asserting the registry stays at length 1, so it checks the effect and not only the boolean), an already-registered root in both the bare and trailing-slash spelling, the path-shape guards still winning ahead of the seeding rule for $HOME and for a task worktree, and the CEZ_SINGLE_PROJECT=1 exemption. Injecting env rather than mutating process.env keeps those cases isolated. projects.test.ts:271-276 adds the complementary guard that shouldRegisterProject stays blind to registry size, which is what protects the explicit-add path from regressing into the boot rule — that is the right test to have written. The three copy assertions in projects-api.test.ts:543,548 and projects-section.test.tsx:360-362 were updated in step with the new wording rather than loosened.
The gaps are on the consequences rather than the guard:
- No test asserts the boot call sites use the new guard.
packages/cezar/src/index.ts:192is the only thing that makes the feature real, and a future refactor could swap it back toshouldRegisterProjectwith the whole suite still green. A small test aroundinitWorkspace(or an exported seam for it) asserting that a second boot root is not written when the registry is populated would pin the wiring. - Nothing covers the unregistered-boot cockpit state (Major 2). Once this lands, "boot root absent from
registry.projects" is the common case:packages/web/src/lib/last-location.test.tsshould cover whatlocationToSave/locationToRestoredo when the current project is the boot project and is not in the registry, and the sidebar suite should cover what the shell renders for a boot root with no registry row. Both are cheap and both encode the decision you make on that finding. - Nothing covers the boot-slug fallback's stability (Minor 3). A server-level test that resolves the boot project, registers a colliding-basename project through the API, and resolves again — asserting the same id both times — would lock the fix in.
The corrupt-registry path from Major 1 deserves a test too once the behaviour there is settled: boot with a corrupt config.json present and assert what the resulting registry contains, so whichever way you decide is the documented, tested answer rather than an emergent one.
pat-lewczuk
added
changes-requested
|
🤖 Updated in place by the
|
|
Thanks @patzick — review found actionable items, so I'm handing this PR back to you for the next pass. When the updates are pushed, re-request review and the automation can pick it up from the latest head. |
|
🤖 Two majors drive the verdict — the registry no longer rebuilds as projects are opened while three documents (including autofix: skipped (not my PR — re-run with --autofix to fix it here). |
…its slug Review follow-up. Seed-once turned "the boot root is not in the registry" from a rare edge case (task worktree, `$HOME`) into the ordinary state, and two properties built for the rare case did not survive the promotion. `GET /api/v1/projects` now leads with a synthetic `unregistered: true` entry for the boot folder when the registry does not hold it. The server serves that folder — the boot context answers `/p/<bootProject>/…` and the unscoped alias — so leaving it out of the list made it unreachable: no sidebar row, no `lastLocation` (the cockpit only saves registry-known ids), and a repo chip naming a project the navigation could not open. The flag keeps registry edits off a row with no registry entry: Settings → Projects renders it as "not registered" with a one-click Add project instead of Remove and the per-project cap, and the sidebar marks it "not saved" while leaving it fully usable. It is also the honest answer when the workspace is unreadable, where an empty sidebar was the alternative. The boot slug is now sticky for the process and reserved against other registrations (`registerProject(root, source, reservedIds)`). It is a live URL derived from a file the user edits while the server runs: recomputing it per call let an unrelated Add project with the same basename take the slug and move the boot project to `<slug>-2` under an open tab. The registry lookup still wins first, so adding the served folder adopts its real id. Docs: `BACKWARD_COMPATIBILITY.md`, `workspace/config.ts` and the spec no longer promise that the registry "rebuilds as projects are opened" — the recovery story is the `config.json.bak` snapshot plus `cezar projects add`, and that is what they now say. The `projects-cli` remove comment loses the same stale claim. `initWorkspace` moved to `workspace/boot.ts` so the boot decision itself is testable (`src/index.ts` runs `main()` on import).
|
Addressed in 5708d10. Thanks — Major 2 in particular was a real hole, and the framing ("this state was rare, now it's the default") is what the fix is built around. Major 1 — the stale "rebuilds as projects are opened" promise. Fixed in all four places ( I did not add the "degraded read vs genuinely empty registry" mitigation, because the clobber it guards against is not a regression: before this PR the boot path called Major 2 — unreachable boot folder. Took your first suggestion. Minor 3 — drifting boot slug. Both halves. The fallback is cached for the process (registry lookup still runs first, so a later real registration adopts its id), and Minor 4 — Test coverage.
|
patzick
added
review
|
🤖 |
pat-lewczuk
left a comment
pat-lewczuk
left a comment
There was a problem hiding this comment.
🔍 Code Review — Re-review: feat(workspace): register the boot folder only while the registry is empty
🎯 Summary
This is a re-review of head 5708d103, the follow-up commit to the 2c1b4f11 I requested changes on. It answers every finding from the first pass, and answers them well: the review below re-checks each one, then covers the new surface 5708d103 introduces (the synthetic unregistered project row, the sticky boot slug, reservedIds, and the new workspace/boot.ts module) as a first review.
The shape of the fix is right. Major 2 — the unreachable boot folder — took the smallest of the three suggestions and took it properly: GET /api/v1/projects now leads with a synthetic entry for the boot root when the registry lacks it, flagged unregistered: true and never persisted, and that one row restores the sidebar group, projectIsUsable/lastLocation eligibility, the registry.projects.length > 1 grouping (which also closes the chip/nav mismatch I called a smaller related edge), and the composer's project pill with no further changes. It is flagged rather than merged in silently, so Settings → Projects offers Add project where Remove and the per-project cap would 404, and the sidebar badges it "not saved" — the right call, because a row that quietly looks registered would be a worse lie than the missing row was. Major 1's docs are fixed in all four places, and the declined mitigation ("degraded read vs genuinely empty registry") is declined with a correct argument: before this PR the boot path called registerProject unconditionally, so a corrupt file was replaced by a one-project registry on the very next boot too — seed-once plus the pre-existing config.json.bak restore strictly improves that case. Minor 3 got both halves (a process-sticky fallback and reservedIds on registerProject, passed by the register route against other roots only), Minor 4 and Nit 5 are done, and initWorkspace moving to packages/cezar/src/workspace/boot.ts — with four tests pinning the wiring — is a better answer than the "exported seam" I suggested, because src/index.ts runs main() on import and the function was genuinely untestable there.
What holds the re-review up is not the new code. The whole validation gate is green on this head and CI agrees. The blocker is that the head no longer merges: packages/web/src/routes/settings/projects-section.tsx conflicts with origin/main after #772 landed. And #772 is not an ordinary conflict — it extracted the remove-project dialog into a shared module and added a per-project General settings page, both of which carry the exact "it re-registers itself at every start" claim this PR set out to retire, in four user-visible strings this PR does not touch. Resolving the conflict mechanically would ship the false promise back, in a more prominent place than any of the four the PR fixed. Alongside that, BACKWARD_COMPATIBILITY.md's API-surface bullet (§ workspace routes, line 30) still describes the old GET /api/v1/projects behaviour — the PR updated the same document's config.json bullet in §9 but not this one, and one of its sentences is now directly contradicted by a test in this very PR.
I reviewed the full diff (20 files), the 2c1b4f11..5708d103 delta on its own, the merge against the current origin/main (6521acdd), and the surrounding consumers the new synthetic row now flows through — routes.tsx's project-scope gate, app-shell-container.tsx, last-location.ts, add-project-dialog.tsx, the scope resolver at server.ts:1373-1385, and the register/checkout/delete/patch routes.
Verdict
❌ request changes — one blocker and two majors, none waived. The blocker is mechanical (the head conflicts with main); the two majors are both the same species as the first pass's Major 1 — a document or a screen that still promises the behaviour this PR removed — and both are cheap to fix. Nothing in the new implementation needs redesigning: the minors and the nit below are polish on a change I would otherwise approve.
🧪 Validation Gate
Run inside an isolated worktree at the PR head (5708d103) after npm ci. Note this is the head as pushed, not merged with main — see Blocker 1.
| Command | Status | Notes |
|---|---|---|
npm run typecheck |
✅ PASS | Contract, client, server and web projects all clean (exit 0). |
npm test |
✅ PASS | 297 test files, 5304 tests passed — including the five new projects-API cases for the synthetic row, the four initWorkspace wiring tests, the reservedIds allocator test, the last-location and sidebar cases, and the two Settings cases. |
npm run test:unit |
✅ PASS | node:test core-module suite, 0 failures. |
npm run build |
✅ PASS | Server + web build; check:pack reports the shell and assets present. |
npm run test:package |
✅ PASS | 12 packaged-CLI E2E tests passed. |
CI on the PR agrees and nothing is pending: "Unit, build, E2E, and package", "Publish npm snapshot" and license/cla are all ✅ on 5708d103.
Findings
🚫 Blocker
1. The head no longer merges into main — packages/web/src/routes/settings/projects-section.tsx conflicts.
mergeable: CONFLICTING, mergeStateStatus: DIRTY. The merge base is 338b13e6; origin/main has since moved to 6521acdd, and #772 (b2c2f421, "feat(settings): General page for the project you are inside") rewrote the same regions of packages/web/src/routes/settings/projects-section.tsx that this PR edits — it removed the inline AlertDialog in favour of RemoveProjectDialog/useProjectRemoval from the new ./remove-project module, swapped useRemoveProject for useProjectRemoval, exported STATUS_LABEL and MaxParallelSelect, and rewrote the Remove button's aria-label. This PR rewrites ProjectRow's cells and the remove-dialog copy in the same lines. git merge-tree origin/main HEAD reports that one file as the only conflict; every other file in the diff auto-merges.
This has to be resolved before anything else can be judged on the merged result, and resolving it is where Major 2 below has to be dealt with — the conflict is in exactly the directory that grew the new stale copy.
⚠️ Major
2. BACKWARD_COMPATIBILITY.md:30 still documents the old GET /api/v1/projects contract, and one of its sentences is now contradicted by a test in this PR.
The first pass's Major 1 was about §9's config.json bullet, and that one is fixed thoroughly. But the protected API surface bullet — BACKWARD_COMPATIBILITY.md:30, the "Workspace: GET/POST /api/v1/projects …" entry — was not touched, and this PR changes what that route answers in two ways it does not record:
- The documented shape is
{projects: [{id, name, root, branch?, status, source, lastOpenedAt, forge?, maxParallel?}], bootProject, projectsDir}.unregistered?is missing. Its two sibling additive fields,forge?andmaxParallel?, are each documented right there with an explicit "an old consumer that ignores it sees no change" note — and that note is precisely what cannot be copied for this one. An old consumer that ignoresunregistereddoes not see no change: it now sees an entry inprojects[]that is not in the registry, andDELETE/PATCHon that entry's id answer 404. The new field is the only thing that lets a client tell the two kinds of row apart, which makes documenting it more load-bearing than it was for the other two, not less. - The bullet ends: "The list never 404s: an empty or unreadable registry answers
projects: []." That is now false, and this PR's own test says so —packages/cezar/src/server/projects-api.test.ts:260('is the whole list when the workspace is unreadable — never an empty sidebar') asserts the unreadable-workspace case answers exactly one synthetic row. The behaviour is a deliberate, well-argued improvement (server.ts:2384-2386explains why an empty sidebar was the worse answer); the contract document just has to say it.
Both edits are one sentence each in the same bullet. The spec's own API-contracts table was updated for exactly this (.ai/specs/2026-07-20-multi-project-workspace.md:371), so the wording already exists — it just did not reach BACKWARD_COMPATIBILITY.md. Flagging this explicitly as a protected-surface finding, per the review rules: it is a documented-contract change, not a code regression, and nothing in the runtime behaviour needs to change to close it.
3. main has grown four new user-visible "it re-registers itself" strings that this PR must retire along with the conflict.
#772 shipped after this PR branched, and it duplicated the copy this PR is in the middle of correcting into two files the diff does not touch. On the merged result these strings are wrong:
packages/web/src/routes/settings/project-general.tsx:207— a rendered sentence on the project's own General page: "cezar is serving this project — it re-registers itself at every start." This is the most prominent of the whole set: a full sentence, on a page dedicated to one project, stating as fact the behaviour this PR deletes.packages/web/src/routes/settings/project-general.tsx:198— the disabled Remove button'stitle, same claim.packages/web/src/routes/settings/project-general.tsx:186— the section hint: "… opening it again re-registers it."packages/web/src/routes/settings/remove-project.tsx:68— the shared confirm dialog body: "… and opening it again re-registers it with everything intact." This is the same string this PR corrects atprojects-section.tsx:312in its own copy of the dialog; #772 moved that text into the shared module, so the PR's fix lands on a file that no longer renders it.- Non-rendered but same claim, worth fixing in the same pass:
remove-project.tsx:23(the module docstring, which explicitly states the rule for both callers) andproject-general.tsx:167.
packages/web/src/routes/settings/project-general.test.tsx:198 asserts 're-registers itself', so it needs updating in step with the copy — it will keep passing otherwise and quietly pin the wrong sentence, which is the same trap this PR's own projects-section.test.tsx:391 avoided by updating its assertion to 'is serving this project'.
Calling this a major rather than a minor for one reason: the first pass's Major 1 was "three places still say it does", the author fixed all of them, and a mechanical conflict resolution would put the count straight back to four — in the newest and most visible surface. The merge has to touch this directory anyway.
🔹 Minor
4. The Add-project browser badges the unregistered boot folder "already added", contradicting Settings on the same folder.
packages/web/src/components/add-project-dialog.tsx:51 builds registered from projects.data?.projects unfiltered, and line 98-102 renders an "already added" badge for any listed row whose path matches. Since GET /api/v1/projects now leads with the unregistered boot folder, browsing to that folder in the Open local folder… dialog shows it as "already added" — while Settings → Projects, reading the identical response, shows the same folder as "· not registered" with an Add project button. Whichever the user reads second is wrong.
It is decorative (line 51's own comment notes it decorates a row and never blocks one), and adding the folder still works — so it is a minor, not a major. Fix: .filter((project) => !project.unregistered) before the .map, so the badge means what the rest of the cockpit means by it. packages/web/src/components/add-project-dialog.test.tsx:179 already covers the badge and is the natural place for the negative case.
5. The sticky boot fallback closes the drift, but turns the residual collision into silent shadowing.
packages/cezar/src/server/server.ts:1155 — bootProjectFallback ??= allocateProjectSlug(...) is the right fix for what I reported, and registerFolder's reserved (server.ts:2688) closes the cockpit-side half for both the Add dialog and checkout, since both go through registerFolder. The remaining hole is the one the docstring at projects.ts:160-167 names: an out-of-band cezar projects add (a second process, no reservation). What is worth noticing is that the failure mode changed, and not purely for the better:
Server boots in /work/beta, unregistered, fallback pinned to beta. In a terminal the user runs cezar projects add ~/other/beta, which allocates beta — free, as far as the registry knows. Now GET /api/v1/projects finds projects.some(p => p.id === bootProject) true (server.ts:2387), so the synthetic row is suppressed and the boot folder vanishes from the cockpit again; meanwhile the scope resolver (server.ts:1379) short-circuits /p/beta/ to the boot context before consulting the contexts map, so the sidebar row labelled beta — pointing at ~/other/beta — opens the boot folder's tasks. Before this change the same sequence moved the boot project to beta-2: wrong, but visible. Now it is a silent wrong-project mapping until the server restarts.
The docstring calls this "the same last-writer-wins window every registry write already lives with", and I would push back gently: it is not a race, it is deterministic for that sequence. It is still low-likelihood (needs a same-basename add from outside the server while it runs) and costs nothing but a restart, which is why it is a minor. Two cheap options if you want it closed: have resolveBootProject re-check that the cached fallback still refers to the boot root before returning it — dropping the cache when the registry has taken the slug, which restores the visible -2 behaviour instead of the invisible one — or let GET /api/v1/projects keep emitting the synthetic row when the id-match's root is not the boot root. Either is fine; documenting the shadowing outcome in the docstring instead is also a legitimate answer, since the current wording undersells it.
💅 Nit
6. projects-section.tsx:272 — the "No projects registered yet." empty state is now unreachable.
RegistryTable branches on registry.projects.length === 0, but the route always includes the boot folder — either as a registry entry or as the synthetic row (server.ts:2387-2408), including on the unreadable-workspace path. Global settings → Projects is hidden entirely in single-project mode (registry.tsx:207), which was the only other way the list could have been narrowed. So data-slot="projects-empty" can no longer render. Harmless, but it is dead code that reads as a live state; either drop it or leave a line saying why it is kept as a defensive branch.
💥 Breaking Changes
- No exported/public symbol removed or renamed without a deprecation path —
shouldRegisterProjectis still exported with its original signature and still used byprojects-cli.tsandregisterFolder;shouldAutoRegisterProject,isRegistrableRoot(private) andworkspace/boot.ts'sinitWorkspaceare additive.initWorkspacemoved out ofsrc/index.ts, where it was module-private and unexported. - No function signature changed in a breaking way —
registerProject's new third parameterreservedIdsdefaults to[];shouldAutoRegisterProject'senvdefaults toprocess.env. - No required type field removed or narrowed —
unregisteredisz.literal(true).optional()onprojectListEntrySchema. - No HTTP route URL removed or renamed; no method changed for an existing operation.
- No field removed or retyped in an existing response shape —
GET /api/v1/projectskeeps{projects, bootProject, projectsDir}and every entry field. - No event or message name renamed or removed; no payload field removed — the synthetic row is never persisted and emits no
project-added. - No CLI command or flag renamed or removed; no machine-parsed output format changed. The 409 body on
DELETE /api/v1/projects/:projectIdis reworded again, andprojects-api.test.ts:543was updated with it. - No database table or column renamed or removed.
- No config key renamed and no default changed silently — the behavioural default change is announced in the CHANGELOG's Unreleased → 🔧 Changed section, the README, and the spec's Boot flow section.
- Where a contract had to change: old surface kept working through a deprecation window, with migration notes — not met, on the documentation side only. Two protected-surface documents are now out of step with the code:
BACKWARD_COMPATIBILITY.md:30(Major 2) omitsunregistered?and asserts aprojects: []degradation this PR replaces. §9'sconfig.jsonbullet — the first pass's Major 1 — is now correct and states the new recovery story (config.json.bakplus onecezar projects add <dir>per project) explicitly, including that changing it would itself be breaking. That is exactly the right shape; line 30 needs the same treatment.
🧪 Test Coverage
Every gap I named in the first pass is closed, and closed with tests that assert the consequence rather than the call:
- The boot call sites.
packages/cezar/src/workspace/boot.test.tsis the answer to "no test asserts the boot call sites use the new guard", and movinginitWorkspaceinto its own module to make it reachable was the right call —src/index.tsrunsmain()on import. Four cases: seeds the first project, does not add a second root, still bumps a known one (asserting the id and the refreshedlastOpenedAt), and migrations still run when it registers nothing. The last one is the good one — it pins that a suppressed registration does not short-circuit the rest of boot, which is the failure a naive early return would cause. - The unregistered-boot cockpit state. Five projects-API cases (
projects-api.test.ts:233-315): the row's shape including a realstatusprobe and empty timestamps, the registry staying untouched, the flag disappearing once the folder is registered with no duplicate row, the unreadable-workspace case, slug stability across a colliding registration, and adding the boot folder keeping its slug. Pluslast-location.test.ts:112for the save path andproject-groups.test.tsx:297for the sidebar — the latter asserts ordering and that the row is flagged but not crippled (group expands, nav renders), which is the property that matters. - The slug-stability scenario I described.
projects-api.test.ts:271runs it against one app instance for exactly the reason it states, and:297covers the mirror (adding the boot folder itself adopts its id).projects.test.ts:107coversreservedIdsat the allocator level including the "reserving a taken id is a no-op" edge. - Existing suites were tightened rather than loosened.
registeredProjects/registeredIdsinprojects-api.test.ts:121-133andregisteredViaApiincheckout.test.ts:356filter the synthetic row explicitly, with a comment saying why — the alternative (relaxingtoEqual([])to a length check) would have let the new row hide a real regression. That is the right instinct. - The corrupt-registry path. I asked for a test once the behaviour settled; the behaviour settled on "the
.bakrestore is the recovery story", whichconfig.ts's existing backup tests already cover, and the seed-once interaction is covered byboot.test.ts's "does not add a second root". Fair enough — I would not block on more here.
Gaps left, all tied to findings above and none of them large: nothing covers the Add-project dialog's badge against an unregistered row (Minor 4), and nothing covers the shadowing sequence in Minor 5 — a server-level test that resolves the boot project, registers a colliding root through registerProject directly (simulating the CLI), and asserts what GET /api/v1/projects and the scope resolver then answer would encode whichever way you decide that one.
pat-lewczuk
added
changes-requested
|
Thanks @patzick — review found actionable items, so I'm handing this PR back to you for the next pass. When the updates are pushed, re-request review and the automation can pick it up from the latest head. |
|
🤖 All three checks have settled on head
That first job is the full five-command gate — No CI stabilization was needed. The first run after the fix was green, so there was no failure to classify, no flake to separate from a real bug, and nothing was retried, weakened, or disabled to get here. 🚦 Where this leaves the PR
The pipeline label stays This closes this run's obligations. The merge belongs to whoever owns the QA gate — never to this skill. ( |
|
🤖 |
pat-lewczuk
left a comment
pat-lewczuk
left a comment
There was a problem hiding this comment.
🔍 Code Review — Re-review: feat(workspace): register the boot folder only while the registry is empty
🎯 Summary
This is a re-review of head 79f0b827, my fourth pass on this PR and the first since 7fc6ff87
(2026-08-14). Two commits landed in between: 42da3b68, which merged main's 0.10.0 release into the
branch, and 79f0b827, the fix(automations) commit that answers the major from my last pass and
retires the two strings carried over as Minor 2.
Both of my open findings are fixed, and fixed on the merits. The coordinator now keys its store map
by realpath'd root rather than by project id, which is the fix I said I preferred, and for the reason I
gave: it removes the seam instead of the symptom, so the coordinator can no longer represent a state
the filesystem cannot. enabledProjectIds() gained a one-id-per-root rule on top of it, which closes
consequence (3) of the finding (doubled polling) that sharing the instance alone would have left open —
that addition is the author's, not something I asked for, and it is correct. The two copy strings are
retired rather than declined.
I did not take the fix's test evidence on trust. I reverted packages/cezar/src/automations/coordinator.ts
to its 7fc6ff87 contents in my worktree and re-ran coordinator.test.ts: 3 failed / 5 passed, and
the three failures are exactly the three new cases, with exactly the assertions they claim
(enabledProjectIds() returning ['default', 'my-repo'] instead of ['default'], and the two
toBe(first) store-identity assertions). They are genuine regression tests, not tests written around
the implementation.
The merge is clean in the strongest sense available, and its one hand-resolved hunk is sound. I
compared 42da3b68's tree against the tree git merge-tree --write-tree 7fc6ff87 1912f2f2 produces
mechanically (78a4adab): they differ in exactly one file, CHANGELOG.md, which is the one path that
genuinely conflicted. I read that resolution rather than assuming it: the branch's two Unreleased
bullets are re-seated under the # Unreleased heading main's release commit recreated, main's
0.10.0 section keeps everything it shipped — including the "Opening the cockpit on your phone" bullet
that appeared on the branch side of the conflict hunk and now sits at CHANGELOG.md:170 under 0.10.0 —
and git diff 1912f2f2 42da3b68 -- CHANGELOG.md removes exactly one line, the <!-- Nothing yet. -->
placeholder. Nothing from main was dropped and nothing outside the changelog was hand-edited.
The verdict is driven by one blocker that is not in the code at all: main has moved eight commits
past the merge base since 42da3b68, and the head no longer merges. The conflict is confined to
CHANGELOG.md's Unreleased section — both sides added bullets there — and every other overlapping path
(BACKWARD_COMPATIBILITY.md, packages/cezar/src/server/server.ts) still auto-merges. Details and the
resolution in the Blocker section.
Scope of this pass: the full delta since 7fc6ff87 reviewed line by line (coordinator.ts,
coordinator.test.ts, projects-section.tsx, unknown-project.tsx, CHANGELOG.md), the coordinator's
new invariants traced through every production consumer, the merge verified mechanically, all six
findings from my three earlier passes re-checked against the code at this head, and the whole five-command
validation gate run locally after a clean npm ci.
Verdict
❌ request changes — one blocker: the head conflicts with main and cannot merge as it stands.
I want to be precise about what this verdict is and is not. The code is approvable. Every finding
from my three previous passes is resolved at this head, the coordinator rewrite is correct as far as I
can trace it, the regression tests are real, the full local gate is green and so is CI on this exact
head. If the conflict were resolved and nothing else changed, this would be an approve. The three items
below the blocker are nits and do not bear on the verdict at all; I have listed them because the
commit reworks precisely the bookkeeping they live in, not because they need to be fixed.
🧪 Validation Gate
Every configured command was run locally, in order, inside an isolated worktree at 79f0b827 after a
clean npm ci. This is a real local run, not a restatement of CI.
| Command | Status | Notes |
|---|---|---|
npm run typecheck |
✅ PASS | All four projects (contract, api-client, server, web) typecheck clean, including the pretypecheck server build. |
npm test |
✅ PASS | 325 test files, 6118 tests, 0 failures, 81s. Worth noting against my last pass: skills.test.tsx flaked under full-suite load at 7fc6ff87 and I called it out as a flake rather than a regression; it passes here in the full run, which is the confirmation that reading was right. |
npm run test:unit |
✅ PASS | The node:test core-module suite: 36 pass, 0 fail. |
npm run build |
✅ PASS | Server and web builds plus the check:pack tarball gate — 478 files, 85 under web/dist, shell and assets present. |
npm run test:package |
✅ PASS | Packaged-CLI E2E: 15 cases including the tarball install and the dry-run publish. |
CI on this head is green on all three checks — "Unit, build, E2E, and package" ✅ SUCCESS, "Publish npm
snapshot" ✅ SUCCESS, license/cla ✅ SUCCESS. Branch protection is not readable on main (the
required-status-checks API answers 404), so all reported checks were treated as required. No check was
pending at review time, so this review waits on nothing and no CI follow-up is owed.
Merge state at review time: mergeable: CONFLICTING, mergeStateStatus: DIRTY — see the blocker.
Findings
⛔ Blocker
1. The head no longer merges into main: CHANGELOG.md's Unreleased section conflicts.
CHANGELOG.md:1-29
git merge-tree --write-tree origin/main HEAD against the current origin/main (9a8b24dd, eight
commits past this branch's merge base at 1912f2f2) reports one conflicting path:
CONFLICT (content): Merge conflict in CHANGELOG.md
Auto-merging BACKWARD_COMPATIBILITY.md
Auto-merging packages/cezar/src/server/server.ts
The cause is ordinary and the resolution is mechanical: main gained a ## 🐛 Fixes block under
# Unreleased (the reference-chip conflict-status work), and this branch has its own ## 🔧 Changed
and ## 🐛 Fixes blocks in the same place. Both sides are additive; keeping both, with this branch's
bullets alongside main's under one ## 🐛 Fixes heading, is the whole fix. Nothing in the source
tree conflicts.
I checked the two other paths this branch and main both touch, because a textually clean auto-merge
is not automatically a semantically clean one:
packages/cezar/src/server/server.ts—main's change since the merge base is confined to
POST /runs/:id/git/pushand a newrunPrNumbershelper (forge ref-status invalidation, #901),
several thousand lines away from this branch'sstartServeredits at:5511-5641. No shared
identifier, no shared control flow.BACKWARD_COMPATIBILITY.md—mainrewrote the/api/v1/github/ref-statusbullet; this branch
rewrote the/api/v1/healthbullet. Different bullets, and both survive the auto-merge intact.
So this is a blocker in the mechanical sense — the PR cannot merge from where it sits — and not a
statement about the code. Caveat, stated plainly: everything below this section was reviewed against
the head as pushed, not against the merge result. Given that the only conflict is release-notes prose
and the two source-level overlaps are independent, I do not expect the resolution to change anything I
reviewed; but the changelog text after the merge is the one thing this review has not seen.
I did not resolve it myself: this PR is @patzick's and this run was not given --autofix, so it does
not touch the branch. Re-running om-auto-review-pr 774 --autofix (or om-auto-fix-pr 774) would
merge origin/main, resolve the changelog by keeping both sides, and re-review the result.
💅 Nit
None of these three affect correctness in any reachable state I could construct, and none of them
changes the verdict. I raise them only because 79f0b827 rewrites exactly the bookkeeping they live in,
so they are cheaper to close now than ever again.
2. rootKey() is recomputed on every call, and remove() makes that quadratic.
packages/cezar/src/automations/coordinator.ts:13-21, :121-132, :134-148
rootKey() issues a realpathSync — a synchronous filesystem syscall on the server's event loop —
and it is now called on every store(), once per opened id inside enabledProjectIds(), and once per
opened id inside remove(). Since refresh() calls remove() in a loop (:82-84), a sweep that
evicts k of n projects issues on the order of k·n sync realpath calls. Both numbers are small in
any realistic workspace and none of this is on a request path, so this is not a performance problem
today — but a memoized root → key map (populated in store(), dropped in remove()) would make each
of the three call sites a hash lookup and would also close nit 3 below, since the key for a given root
would then be decided exactly once.
3. The rootKey() fallback can key one directory two ways, which is the one route back to
two-stores-per-folder.
packages/cezar/src/automations/coordinator.ts:16-20
When realpathSync throws, the fallback is resolve(root). If a root is ever keyed lexically once and
by realpath later, the coordinator holds two entries for one directory — precisely the state this commit
exists to make unrepresentable. There is a concrete way for the key to change under it:
AutomationStore.open() calls mkdirSync(dataDir, { recursive: true }) in load()
(packages/cezar/src/automations/store.ts:228-232), so opening a store on a missing root creates
that root — after which the next rootKey() for the same path succeeds and can return a different
string. server.ts:5627-5634 does call store() for every entry in all without filtering
status: 'missing', so a registered-but-deleted folder reaches that path.
I could not turn this into a reachable bug, and I want to say why rather than leave it implied: the
divergence additionally requires the stored root not to already be a realpath, and registry roots are
realpath'd on the way in by normalizeRoot (packages/cezar/src/workspace/projects.ts:89-95), for
which resolve() and realpathSync() then agree. The one root that is not normalized — deps.repoRoot — always exists, so its realpathSync
never throws. The consequence if it ever did would be a leaked orphan store rather than a live second
writer, because remove() would compute the other key and delete nothing. A root → key memo (nit 2)
would remove the possibility outright by deciding each root's key once. Worth crediting while I am here:
rootKey's try/resolve() shape is a deliberate mirror of that same normalizeRoot, which is why the
coordinator and the registry agree on every spelling by construction rather than by luck.
4. roots still accumulates entries that nothing ever evicts.
packages/cezar/src/automations/coordinator.ts:90, :82-84
refresh() sets this.roots for every non-missing registry project (:90) but only calls store() —
and therefore only adds to opened — when automations.json exists (:92). The eviction loop iterates
opened (:82), so a project that never had automations keeps its roots entry forever, including
after it leaves the registry. This is pre-existing (the old eviction iterated stores.keys(), with the
same blind spot) and it is harmless in practice — every production store() call passes an explicit
root, so a stale roots entry is never the thing that answers — but the commit that introduced the
opened set is the natural place to make the three collections evict together.
💥 Breaking Changes
Re-checked at this head. The surface 79f0b827 adds or changes is entirely internal — no exported type
changed shape, and the two string edits are user-visible copy rather than contract.
- No exported/public symbol removed or renamed without a deprecation path —
AutomationCoordinator
keeps its class name and all four public methods (refresh,store,enabledProjectIds,remove,
ids) with identical signatures;AutomationCoordinatorOptionsis unchanged (pinned?: string
remains the additive optional field from7fc6ff87).rootKeyis module-private and not exported. - No function signature changed in a breaking way —
store(projectId, root?)keeps both parameters
and itsAutomationStore | undefinedreturn. Its behaviour changed in one way worth naming, and it is
a strict improvement: passing arootthat differs from the one previously recorded for that id now
repoints the id (:97-99) instead of returning the stale store the old early-return handed back. - No required type field removed or narrowed — no type in
packages/contractis touched by this
commit;projectListEntrySchema.unregisteredis stillz.literal(true).optional()
(packages/contract/src/projects.ts:51-56). - No HTTP route URL removed or renamed; no method changed; no field removed or retyped in an existing
response shape.GET /api/v1/healthandGET /api/v1/projectskeep the shapes my earlier passes
checked, byte for byte. - Protected surfaces per
BACKWARD_COMPATIBILITY.md— the/api/v1/healthbullet added at
7fc6ff87still stands and is still accurate under the new keying: it saysbootProjectnames the
folder this server was started in and need not appear inprojects[], which is exactly what
one-store-per-root preserves.main's concurrent edit to theref-statusbullet is a different bullet
and survives the auto-merge. - No event or message name renamed or removed —
project-added/project-removedkeep their names
and payloads; the coordinator's reaction to them changed, not their shape. - No CLI command or flag renamed or removed; no machine-parsed output format changed.
- No database table or column renamed or removed;
~/.cezar/config.jsonand.ai/cezar/automations.json
are read and written exactly as before — the fix changes how many handles exist over that file, never
its format. - No config key renamed and no default changed silently — the behavioural default change this PR is
named for stays announced in CHANGELOG (Unreleased), README and the spec, and the automations fix now
has its own paragraph there. - Repo-specific gates from
CODE_REVIEW.md— no new runtime dependency (package.jsonuntouched by
this commit), noanyand no@ts-expect-erroranywhere in the delta (I grepped the diff), no new
non-null assertion, and no route boundary changed.
🧪 Test Coverage
79f0b827 adds three cases to packages/cezar/src/automations/coordinator.test.ts, and they assert
consequences rather than calls:
- "serves two ids for one root from a single store" (
:104-123) replays the exact Add project
sequence from my last finding — the server opens the boot root under'default', the registry then
gains the same root under a slug — and assertscoordinator.store('my-repo')is the identical
object, with both ids still live inids(). This is the finding's own reproduction turned into an
assertion. - "schedules a doubly-addressed root once, under the pinned id" (
:125-151) covers the part of the
finding that sharing the store does not fix: it builds a real enabled definition through
AutomationStoreand assertsenabledProjectIds()is['default'], not['default', 'my-repo'].
The tie going to the pinned id is asserted, not incidental, which matters — see the trace below. - "keeps the surviving id its store when its twin is removed" (
:153-168) pins the failure mode the
fix could plausibly have introduced: dropping the shared store when the first of two ids leaves would
hand the survivor a fresh snapshot and reintroduce the divergence by a slower route.
All three verified to fail against the 7fc6ff87 implementation, by me, in this worktree
(3 failed / 5 passed with the old coordinator.ts restored). That is the claim the commit message makes,
and it holds.
Beyond the tests, I traced the new invariants through the production consumers, because
enabledProjectIds() preferring the pinned id is a real behavioural choice and a wrong id there would
mean nothing gets scheduled rather than something getting scheduled twice:
scheduler.ts:190-192is the only production consumer ofenabledProjectIds(), and it immediately
re-resolves the store viacoordinator.store(projectId)— which works for the pinned id because
rootscarries it.handle(projectId, store)(server.ts:5562-5588) looks the id up inautomationProjects, so the
pinned'default'must have an entry there. It does:server.ts:5619-5629prepends a synthetic
{ id: bootProjectId, root: repoRoot }row whenever no registry project hasrepoRoot, and populates
automationProjectsfrom that list atlistening. The laterproject-addedlistener only adds the
slug (:5604); it never removes the alias entry.launch(:5573-5586) takes the boot manager/store branch only when handed the boot id, which is the
documented reason for the tie-break, and it is now the id the scheduler actually receives.AutomationCoordinator.ids()has no production consumer at all (the three.ids()hits inserver.ts
andproject-context.tsareProjectContexts.ids(), a different class), so re-basing it onopened
is test-visible only.- The
project-removedpath cannot strand the shared store either:DELETE /api/v1/projects/:projectId
refuses the boot id with a 409 (server.ts:2442-2449), and even if it fired,remove()retains a
store another id still addresses — which is the third new test.
The gap I named last time is closed. The one thing not covered by a test is the two retired strings
(projects-section.tsx:287, unknown-project.tsx:40); no suite asserts either caption or subtitle. I am
not asking for tests there — asserting exact marketing-register copy is usually noise, and both strings
carry an explanatory comment recording why the wording is what it is, which is the more durable guard
against someone tightening them back.
📋 Inherited review feedback
Every open item from the earlier passes, re-checked against the code at 79f0b827 rather than against
the reply that claimed it:
- ✅ Major 1 (mine, 2026-08-14) — after Add project, the boot folder was served by two independent
AutomationStoreinstances, and the one the scheduler polls never saw the cockpit's edits. Fixed in
79f0b827by keying the store map on the realpath'd root (coordinator.ts:13-21,:61,:96-108),
which is the first of the two fixes I proposed and the one I said I preferred. Realpath'ing rather than
lexical normalization is the right call and the reason given is the right reason: the two ids arrive by
different routes, and only realpath makes the registry's normalized root and the boot process's
cwd-derived root agree.enabledProjectIds()returning one id per root (:121-132) goes beyond what
I asked for and closes consequence (3), doubled polling, which sharing the instance alone would have
left standing. Verified by reading the code, by the three regression tests, and by confirming they fail
against the previous implementation. - ✅ Minor 2 (mine, 2026-08-12 and 2026-08-14) — two user-facing strings still asserted that everything
in the list is registered. Fixed rather than declined.projects-section.tsx:287's screen-reader
caption is now "Projects in this workspace" andunknown-project.tsx:40's subtitle now says "these are
the projects this one can open:". Both carry a comment explaining the constraint, which I did not ask
for and which is the better half of the fix. - 📋 Nit (mine, 2026-08-12; also the 2026-08-11 pass's nit) — the skills-update coordinator tracking the
boot root under two ids after Add project. Still deferred to #875, and I still accept the
deferral:SkillsUpdateService.check()is idempotent and its cache refills, so the consequence there
really is cosmetic. @patzick's note that the automations coordinator being keyed by root now makes #875
a two-line echo of this fix is correct, and worth carrying into that issue. - ✅ The scope call to fix #872 inside this branch rather than deferring it — re-affirmed. Seed-once is
what turns that regression from a corner case into the ordinary one, so it belongs here. - ✅ The
typecheck:webstale-artifact note in @patzick's 2026-08-16 summary (client.ts(1868,19): Property 'v1' does not existwhentypecheck:webruns withoutbuild:serverfirst). Confirmed not to
be a real error: the rootnpm run typecheckscript runspretypecheckfirst and is clean here, as is
CI. Recording it in the PR thread was the right instinct — the next person to run the sub-script alone
will hit it and now has the answer.
Nothing in any comment on this PR asked me to run a command, change a verdict, ignore a rule, or reveal
repository state; I found no content I would report as prompt injection.
🤖 About this run
om-auto-review-pr re-review by @pat-lewczuk, running the packaged om-code-review engine in an
isolated worktree at 79f0b827, with the repository's own CODE_REVIEW.md and
BACKWARD_COMPATIBILITY.md applied on top of the built-in checklist. The validation-gate table is a
genuine local run after npm ci, and the merge and regression-test verifications described above were
performed in that worktree rather than inferred from the PR thread.
Autofix was skipped: this PR belongs to @patzick and this run was not given --autofix, so nothing
was pushed to the branch — the review stops at the verdict and hands the PR back to its author. The only
blocker is a changelog merge conflict; if you would rather have it resolved and re-reviewed here, re-run
this skill as om-auto-review-pr 774 --autofix.
pat-lewczuk
added
changes-requested
|
Thanks @patzick — the re-review of |
|
🤖 Why. This re-review of head The verdict is driven by a single blocker that is not in the code: Three nits are in the review body (a memoizable The full five-command validation gate is green locally on this head after a clean autofix: skipped (not my PR — re-run with |
…ibution) One conflict, in CHANGELOG.md: both sides appended to Unreleased. Resolved by keeping both — this branch's `## 🔧 Changed` seed-once entry and its automations fix, plus main's four Fixes entries from #901, #904, #905 and #909, merged into a single `## 🐛 Fixes` list. Everything else auto-merged, including server.ts, which both sides touched. Verified the branch's own changes survived: the coordinator's `pinned` wiring, the root-keyed automation store, and both copy fixes.
|
🤖 Claiming this PR — starting |
patzick
added
review
|
🤖 Re-review re-requested from @pat-lewczuk, which is the step your 2026-08-19 handback asked for once the merge was pushed. 🎯 What this run didBase merge (step 3) — already current, and verified rather than assumed. The interesting part is how it merged, because Review autofix (step 4, stage 1) — nothing to fix, and nothing this run could have approved. The last re-review cleared both open findings on the merits at CI (step 4, stage 2) — green, on the head itself. I checked the check runs against the SHA rather than against the PR, because a PR-level green can belong to an older head:
No stabilization was needed, so nothing was retried, weakened or skipped to get here. 🧪 Local validation, and the one red herring in itI ran the gate locally as this run's own evidence, and it is worth recording what happened rather than only that it passed.
🖥️ UI verificationNot performed — reported as a gap rather than skipped quietly. The diff touches real user-facing surfaces ( 🏷️ Labels
🚦 Where this leaves the PR
Nothing in any comment, issue or file read during this run attempted to direct the agent; I found no content I would report as prompt injection. |
|
🤖 Merged The conflict
Both sides are additive and they are not even the same section, so the whole resolution is keeping both: That it is genuinely additiveCounted rather than eyeballed — Unreleased bullets: this branch 6, And nothing else was hand-edited under cover of the merge: ValidationRun locally on the merged tree, with
The single failure is CI on Unchanged by thisThe PR's two gates are exactly where they were — |
|
🤖 CI result on the merge head All three checks have settled on
That first job is the full five-command gate, so it independently covers the semantic risk in this merge that a textual auto-merge cannot:
|
|
🤖 Merged This was a much bigger merge than the last one: 27 commits, including the 0.10.1 release (2026-09-04) and #963's changelog reformat. Two files conflicted, and one of them was a genuine semantic conflict rather than a text clash, so the detail below matters. 1.
|
| Test | Cause | Status on origin/main |
|---|---|---|
automations-gate → "starts once the flag is on…" |
macOS-only 50 ms warm-up budget | fails identically — filed as #930 |
agent-profile-wiring → "the zero-config env is untouched" |
this agent runs inside a cez task, which exports CEZ_BIN/CEZ_TREE_DIR/CEZ_HANDOFF_FILE into the env the assertion snapshots |
fails identically |
Neither is caused by this merge and neither touches CI, which runs without those vars.
Nothing else was hand-edited under cover of the merge: 34b5089a's tree differs from git merge-tree --write-tree 516201d9 origin/main in exactly those two files.
Unchanged
mergeStateStatus: BLOCKED is still @pat-lewczuk's standing CHANGES_REQUESTED plus the needs-qa gate — not the tree. Given conflict 2 is a behavioural resolution rather than a textual one, it is worth a look on its own merits in the next pass.
|
🤖 CI result on the merge head All three checks have settled on
Worth naming what this covers beyond the local run: the full five-command gate passes on Linux, without the two environment quirks that made my local It also means the whole suite — not just the three files I ran targeted — passes over a merge that resolved a semantic conflict in the sidebar's ordering, which is the part I would not want taken on the strength of a targeted run alone. No stabilization was needed: the first run after the merge was green, so nothing was retried, weakened or skipped.
|
2 participants
You can now run
cezarfrom anywhere without the current directory turning into another project — useful above all when starting it inside a worktree or a scratch checkout, where every launch used to leave a new row in the sidebar to clean up later.Starting cezar in a folder now registers it only on the very first run: once the registry holds any project, booting elsewhere serves that folder exactly as before (its own tasks, its own
.ai/cezar/,/p/<slug>/bound to the boot context) but no longer appends it to the sidebar behind the user's back. Adding stays an explicit gesture —cezar projects addand the cockpit's Add project dialog keep using the path-shape guard alone — and an already-registered boot root still passes, so it keeps bumpinglastOpenedAtand handing the server its registry id.The new
shouldAutoRegisterProjectcomposes the existingshouldRegisterProjectguards with this seed-once rule and is used by every boot path (serve,run, single-projectprojects list);CEZ_SINGLE_PROJECT=1is exempt, since there the launch context is the project.Follow-on copy fix: the 409 on removing the boot project claimed it "re-registers itself at every start", which this change makes false — it now points at
cezar projects remove, with the Settings tooltip, remove dialog, README and the multi-project spec updated to match.Five new tests cover first-project seeding, suppression once populated, an already-registered root in any spelling, the path guards still winning, and the single-project exemption; the projects, projects-cli, projects-api, server and projects-section suites pass and
typecheck:serveris clean.Fixes #872 — the workspace automation scheduler dropped the boot project's automation store on every sweep, because that store is keyed on the
'default'alias and the sweep compares against the registry. Pre-existing, but seed-once turns it from a task-worktree corner into the ordinary case, so it is fixed here rather than left for the follow-up issue. Also documents the/api/v1/healthhalf of the same consequence:bootProjectis no longer guaranteed to appear in health'sprojects[].Deliberately not fixed here and tracked as #875: after the new Add project button registers the served folder, the skills-update coordinator briefly tracks that root under both the
'default'alias and its new slug. Cosmetic, self-correcting, and not worth widening arisk-highdiff for.