feat: worktree coordination registry — PARALLEL-01#113
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 881f74acbb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| export async function cmdRegistryList(...args) { | ||
| const jsonMode = args.includes('--json'); | ||
| const leases = listLeases(process.cwd()); |
There was a problem hiding this comment.
registry-list, registry-show, and registry-clear all read from process.cwd() instead of resolving the workspace root, so running them from a nested directory silently targets <subdir>/.planning/.local/registry.json and can report “No leases found” even when the real root registry has active leases. This breaks the coordination safety model in common workflows where users run commands from subfolders; these handlers should use resolveWorkspaceContext(args) like other CLI commands.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed. All three handlers in bin/lib/registry-commands.mjs now resolve the workspace root via resolveWorkspaceContext(args) (extracted into a resolveRegistryRoot helper). New regression test registry commands resolve workspace root from a nested cwd confirms a subdir invocation walks up to the workspace .planning/ marker. Enables --workspace-root <path> and GSDD_WORKSPACE_ROOT overrides for free.
| const p = registryPath(workspaceRoot); | ||
| if (!existsSync(p)) return emptyRegistry(); | ||
| try { | ||
| return JSON.parse(readFileSync(p, 'utf8')); |
There was a problem hiding this comment.
The parser returns raw JSON without checking that leases is an array, but downstream operations call data.leases.find(...)/filter(...) unguarded. If registry.json is valid JSON but missing or malformed (for example { "schema_version": 1 }), commands like registry-show and lease mutations crash with an uncaught TypeError instead of handling corruption gracefully. Normalize invalid shapes to emptyRegistry() (or reject with a controlled error) in readRegistry.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed. Normalization is centralized in readRegistry so all six callers (grantLease/closeLease/crashLease/getLease/listLeases/clearLease) inherit the guarantee in one place: parse errors and any non-array leases shape quarantine the offending file to registry.json.broken-<ts> with a stderr warning, then return an empty registry. Three new tests cover the parse-error path, the wrong-shape path ({schema_version:1} with no leases), and the empty-file edge case.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements a new worktree coordination registry using a JSON-based file system approach with atomic renames, replacing the previously considered SQLite-based implementation. The changes include a new registry module, CLI commands for managing leases, integration into the closeout report, and comprehensive test coverage. The reviewer suggested exporting the registry path logic to improve modularity and reduce redundant path construction, which I agree would be a beneficial refactoring.
| function registryPath(workspaceRoot) { | ||
| return join(workspaceRoot, '.planning', '.local', 'registry.json'); | ||
| } |
There was a problem hiding this comment.
Exporting registryPath allows other modules to use this path logic, avoiding redundant path construction and reducing the need for node:path imports in dependent modules.
| function registryPath(workspaceRoot) { | |
| return join(workspaceRoot, '.planning', '.local', 'registry.json'); | |
| } | |
| export function registryPath(workspaceRoot) { | |
| return join(workspaceRoot, '.planning', '.local', 'registry.json'); | |
| } |
There was a problem hiding this comment.
Applied. registryPath, registryTmpPath, and a narrow registryExists(workspaceRoot) helper are now exported from bin/lib/registry.mjs. The test file and bin/lib/closeout-report.mjs consume them directly instead of duplicating the path-construction string.
| const { listLeases } = await import('./registry.mjs'); | ||
| const leases = listLeases(workspaceRoot); | ||
| // If no leases exist and no file exists, listLeases returns []. | ||
| // We need to know whether the registry file itself exists. | ||
| // Use the fs import to check — only add the key if the file is present. | ||
| const { existsSync } = await import('node:fs'); | ||
| const { join } = await import('node:path'); | ||
| const registryFile = join(workspaceRoot, '.planning', '.local', 'registry.json'); |
There was a problem hiding this comment.
Import registryPath from registry.mjs to avoid re-implementing the path logic and to remove the unnecessary node:path import inside the function.
| const { listLeases } = await import('./registry.mjs'); | |
| const leases = listLeases(workspaceRoot); | |
| // If no leases exist and no file exists, listLeases returns []. | |
| // We need to know whether the registry file itself exists. | |
| // Use the fs import to check — only add the key if the file is present. | |
| const { existsSync } = await import('node:fs'); | |
| const { join } = await import('node:path'); | |
| const registryFile = join(workspaceRoot, '.planning', '.local', 'registry.json'); | |
| const { listLeases, registryPath } = await import('./registry.mjs'); | |
| const leases = listLeases(workspaceRoot); | |
| // If no leases exist and no file exists, listLeases returns []. | |
| // We need to know whether the registry file itself exists. | |
| // Use the fs import to check — only add the key if the file is present. | |
| const { existsSync } = await import('node:fs'); | |
| const registryFile = registryPath(workspaceRoot); | |
| if (!existsSync(registryFile)) return null; |
There was a problem hiding this comment.
Applied. buildRegistrySectionSafe imports registryExists from ./registry.mjs — the inline existsSync + join and the dynamic node:fs/node:path imports inside that function are gone.
Adds a JSON-backed registry at .planning/.local/registry.json that records per-phase lease state (open/closed/crashed) and survives session restart. Writes are atomic via per-PID tmp filename + renameSync; corrupt files are quarantined to registry.json.broken-<ts>; a read-after-write fingerprint warning surfaces lost updates between concurrent writers on stderr. The CLI exposes four hyphenated commands: registry-list, registry-show, registry-clear (with --force gate for open leases), and registry-crash (placeholder for the next milestone). Workspace root is resolved via the shared resolveWorkspaceContext helper so commands work from nested directories. closeout-report gains a registry section that tags only foreign-phase open leases as [BLOCK]; the closing phase's own open lease tags as [INFO]. Zero external dependencies, Node >=20 preserved. Design rationale and source evidence are recorded as D64 in distilled/DESIGN.md with the three required research categories cited in distilled/EVIDENCE-INDEX.md. Phase: 65
Adds 23 registry tests covering unit lifecycle, a cross-platform parent-kills-child durability fixture (with .tmp orphan assertion), corrupt-JSON quarantine, per-PID concurrent-write isolation, subdirectory CWD resolution, closeLease state guard, and crashed_at parity. Adds 2 new closeout-report tests confirming the registry key is absent on fresh install and that foreign-phase open leases are tagged [BLOCK] while the closing phase's own active lease is tagged [INFO]. package.json test:gsdd chain extended; facade line limit guard adjusted for the new commands. Phase: 65
gsdd phase-status N done now refuses the status transition when NN-PLAN-CHECK.md or NN-VERIFICATION.md is missing under the phase folder, or when lessons-learned.md has not been touched within the last 7 days. The gate is internal-governance scoped: it skips when no phase folder exists and skips when .internal-research/ is absent, so consumer projects and roadmap-only entries are unaffected. --force --reason <text> overrides the gate and auto-appends an LL-* entry to lessons-learned recording the bypass. --force without --reason is refused. 8 new tests cover each refusal path, the consumer-project skip cases, and the override path. Phase: 65
D64 now cites verifiable sources from each required research class. Spec-framework: GSD confirmed negative by direct inspection of 11 archived gsd-*.md role files; OpenSpec ships only a discovery YAML; LeanSpec has no coordination state. Orchestrator: OpenHands stores session and event state as per-event JSON files at the persistence-dir tree (commit cae76e54, files filesystem_event_service.py and event_service_base.py) — SQLite appears only in the enterprise tier for billing and OAuth, not for session state. MetaGPT writes team.json via write_json_file in team.py. Conductor OSS uses Redis with JSON wire format in RedisExecutionDAO.java; SqliteSchedulerDAO is scheduler-only and still serialises JSON blobs. Industry: Anthropic Claude Code session transcripts are append-only JSONL; the global .claude.json has a documented race condition under concurrent writes with 8+ GitHub issues converging on tmp+rename as the fix — the strongest direct industry endorsement of the chosen pattern. Cursor 2.0 ships .cursor/worktrees.json plus per-task JSON claim files plus atomic mkdir locking — structurally identical to the chosen design. OpenAI Codex CLI uses SQLite for resumable runtime state and is recorded as the upgrade target for concurrent multi- writer scenarios. GitHub Copilot Coding Agent delegates to git worktrees with no client-side registry. D64 title no longer embeds a phase identifier; the design entry captures the architectural decision rather than its first consumer. Universal claims about harness tooling are removed in favor of the scoped, source-cited statements above. Phase: 65
PatrickSys
force-pushed
the
feat/v2-registry
branch
from
881f74a to
cd84087
Compare
PatrickSys
changed the title
1 participant
Summary
Delivers the worktree coordination registry for the v2.0.0 parallel execution milestone (PARALLEL-01). Phases running in parallel worktrees can now record open/closed/crashed lease state through a lightweight JSON store, enabling the write-set advisory layer and crash-recovery role introduced in subsequent phases.
What ships
bin/lib/registry.mjs— core API:openRegistry,grantLease,closeLease,crashLease,clearLease,listLeases,getLease, plus exported helpersregistryPath,registryTmpPath,registryExists. Write path is atomic via per-PIDregistry.json.<pid>.tmp+renameSync; corrupt files are quarantined toregistry.json.broken-<ts>; a read-after-write fingerprint warning surfaces lost updates between concurrent writers on stderr. WindowsEPERM/EBUSYon rename is handled by bounded retry. Zero external dependencies.bin/lib/registry-commands.mjs— four CLI handlers (registry-list,registry-show,registry-clear,registry-crashplaceholder). Workspace root resolved viaresolveWorkspaceContextso commands work from subdirectories. Error messages strip internal function prefixes.bin/gsdd.mjs— four new commands registered alphabetically; help text updated.bin/lib/closeout-report.mjs— registry section surfaced when a registry file is present. Only leases for other phases tag as[BLOCK]when closing a given phase; the closing phase's own open lease tags as[INFO].bin/lib/phase.mjs—gsdd phase-status N donegains a programmatic artifact gate: refuses the transition unlessNN-PLAN-CHECK.md,NN-VERIFICATION.md, and a recent governance doc-sync exist.--force --reason <text>is the audited escape hatch..tmporphan assertion + corrupt-JSON quarantine + per-PID concurrent-write isolation + subdirectory CWD resolution + state-guard coverage), 12 closeout-report tests (incl. foreign-phase blocking distinction), 8 new phase-status gate tests covering refusal paths and override path.Design basis
D64 in
distilled/DESIGN.md(Track C: JSON + atomic rename). Evidence with verifiable URLs and commit refs indistilled/EVIDENCE-INDEX.mdacross the three required research categories: spec frameworks (GSD confirmed negative; OpenSpec discovery YAML only; LeanSpec has no coordination state), orchestrators (OpenHands per-event JSON files at commitcae76e54; MetaGPTteam.json; Conductor OSS JSON-blob wire format), industry guidance (Anthropic Claude Code race-condition fix consensus on tmp+rename; Cursor 2.0.cursor/worktrees.json; OpenAI Codex CLI SQLite recorded as the upgrade target; GitHub Copilot Coding Agent delegates to git worktrees).Upgrade path: revisit SQLite when automated orchestration with concurrent multi-writer requirements lands.
Test plan
node --test tests/gsdd.registry.test.cjs→ 23/23 pass (durability fixture passes on Windows NTFS; concurrent-write per-PID isolation verified; corrupt-JSON quarantine verified)node --test tests/gsdd.closeout-report.test.cjs→ 12/12 pass (foreign-phase blocking distinction verified)node --test tests/phase.test.cjs→ 164/164 pass (8 new artifact-gate tests included; pre-existing scenarios unaffected)npm run test:gsdd→ 792/795 pass; 3 pre-existing README invariant failures are unchanged baselinenode bin/gsdd.mjs registry-list→ "No leases found." on a fresh workspacenode bin/gsdd.mjs registry-listfrom a nested subdirectory → resolves up to the workspace rootnode bin/gsdd.mjs registry-show 99→ exits 1 with clear messagenode bin/gsdd.mjs registry-clear 99→ exits 1 with usage error (no lease) and no internal-function-name leaknode bin/gsdd.mjs registry-crash 99→ exits 1 with deferral message until the next milestone wires the consumernode bin/gsdd.mjs closeout-report --json→ output contains noregistrykey when registry file absentpackage.jsondependencies: {})