P1: add project lifecycle commands#145
Conversation
lzehrung
force-pushed
the
p0/agent-installer
branch
from
86a7083 to
5d8fca4
Compare
There was a problem hiding this comment.
Pull request overview
Adds a new “project lifecycle” surface to Codegraph (init/status/sync/uninit) backed by a project-local .codegraph/manifest.json, with CLI wiring, docs, and tests to validate lifecycle semantics and JSON outputs.
Changes:
- Introduces lifecycle manifest creation/sync/status/uninit logic and user-error handling.
- Adds CLI command plumbing/schemas/help text for lifecycle commands (including
--json,--force,--init, and root semantics). - Adds a comprehensive lifecycle test suite and updates docs/skill guidance to mention the new commands.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/mcp-server.test.ts | Removes a stray blank line in the MCP handler tests. |
| tests/lifecycle.test.ts | Adds end-to-end tests covering lifecycle init/status/sync/uninit behavior and CLI JSON/error semantics. |
| src/sqlite/write.ts | Adds helper for collecting sqlite artifact file signatures (used for artifact metadata). |
| src/mcp/server.ts | Removes a stray blank line in MCP handler wiring. |
| src/lifecycle/manifest.ts | Implements lifecycle manifest schema + init/status/sync/uninit behavior, hashing, and safe removal rules. |
| src/cli/options.ts | Registers lifecycle command schemas, flags, and usage strings. |
| src/cli/lifecycle.ts | Adds lifecycle command handler and pretty/JSON output formatting. |
| src/cli/help.ts | Documents lifecycle commands in CLI help and routes lifecycle help text. |
| src/cli.ts | Wires lifecycle commands into main CLI dispatch and adds root/positional validation + user-error handling. |
| README.md | Mentions lifecycle commands and adds quickstart examples. |
| docs/cli.md | Documents lifecycle commands and their semantics/flags. |
| codegraph-skill/codegraph/SKILL.md | Updates the agent skill guidance to include lifecycle command usage and semantics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const manifestPath = codegraphLifecycleManifestPath(root); | ||
| const manifest = await readLifecycleManifest(root); | ||
| const configHash = await hashConfig(root); | ||
| const buildOptionsHash = hashBuildOptions(options.buildOptions); | ||
| const files = await listAgentSessionFiles({ | ||
| root, | ||
| ...(options.buildOptions ? { buildOptions: options.buildOptions } : {}), | ||
| }); | ||
| if (!manifest) { |
There was a problem hiding this comment.
Fixed in 07cf92c: getCodegraphLifecycleStatus now checks for the manifest first and returns immediately when uninitialized, before touching hashConfig or listAgentSessionFiles. Regression test spies on both computeConfigHash and listAgentSessionFiles and asserts neither is called on an uninitialized project.
| async function hashDiscoveredFiles(files: readonly string[], root: string): Promise<string> { | ||
| const hash = createHash("sha256"); | ||
| for (const file of [...files].sort((left, right) => left.localeCompare(right))) { | ||
| const relative = path.relative(root, file).replace(/\\/g, "/"); | ||
| hash.update(relative); | ||
| hash.update("\0"); | ||
| hash.update(await fsp.readFile(file)); | ||
| hash.update("\0"); | ||
| } | ||
| return hash.digest("hex"); | ||
| } |
There was a problem hiding this comment.
Fixed in 07cf92c: hashDiscoveredFiles now hashes (relative path, size, mtimeMs) via bounded-concurrency fs.stat, matching the metadata-based signature pattern already used by collectAgentFileSignatures in src/agent/session.ts, instead of reading full file bytes. Added a test proving a byte-identical rewrite with a forced mtime bump now registers as filesChanged: true, confirming the hash is metadata-driven.
| function stableStringify(value: unknown): string { | ||
| if (Array.isArray(value)) return `[${value.map(stableStringify).join(",")}]`; | ||
| if (typeof value !== "object" || value === null) return JSON.stringify(value); | ||
| const entries = Object.entries(value).sort(([left], [right]) => left.localeCompare(right)); | ||
| return `{${entries.map(([key, nested]) => `${JSON.stringify(key)}:${stableStringify(nested)}`).join(",")}}`; | ||
| } |
There was a problem hiding this comment.
Fixed in 07cf92c: stableStringify now matches JSON.stringify semantics for undefined \u2014 object keys with undefined values are omitted, array elements that are undefined serialize as null. This resolves both the invalid-JSON output and the real collision it caused (stableStringify([]) and stableStringify([undefined]) previously both produced "[]" via Array.join coercion). Exported stableStringify and added direct unit tests for both cases plus a regression test proving [] and [undefined] no longer collide.
| async function writeLifecycleManifest(root: string, manifest: CodegraphLifecycleManifest): Promise<void> { | ||
| const manifestPath = codegraphLifecycleManifestPath(root); | ||
| await fsp.mkdir(path.dirname(manifestPath), { recursive: true }); | ||
| const tempPath = `${manifestPath}.${process.pid}.${Date.now()}.tmp`; | ||
| await fsp.writeFile(tempPath, `${JSON.stringify(manifest, null, 2)}\n`, "utf8"); | ||
| await fsp.rename(tempPath, manifestPath); | ||
| } |
There was a problem hiding this comment.
Fixed in 07cf92c: temp file is now a hidden, unique name (.manifest.json.<pid>.<uuid>.tmp, matching the pattern in src/indexer/build-cache/manifest.ts's manifestTempFilePath), and the write is wrapped in try/catch with best-effort fs.rm cleanup on failure before rethrowing. Regression test mocks fs.rename to fail for the manifest destination and asserts no *.tmp file remains in .codegraph/ afterward.
…ableStringify, atomic manifest writes, dedupe help line
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (4 files)
Previous Review Summaries (5 snapshots, latest commit 5444ce5)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 5444ce5)Status: No Issues Found | Recommendation: Merge Files Reviewed (4 files)
Previous review (commit 935c4ab)Status: No Issues Found | Recommendation: Merge Files Reviewed (3 files)
Previous review (commit d2534b6)Status: No Issues Found | Recommendation: Merge Files Reviewed (1 file)
Previous review (commit 9a034f1)Status: No Issues Found | Recommendation: Merge Files Reviewed (5 files)
Previous review (commit 07cf92c)Status: No Issues Found | Recommendation: Merge Files Reviewed (13 files)
Reviewed by kimi-k2.6 · Input: 277.5K · Output: 17.1K · Cached: 457.9K |
| const now = new Date().toISOString(); | ||
| const sessionBuildOptions = { ...(buildOptions ?? {}), cache: "disk" as const }; | ||
| const forcedSessionBuildOptions = options.force | ||
| ? { ...sessionBuildOptions, cacheStrict: true, cacheVerify: true } | ||
| : sessionBuildOptions; | ||
| const session = createAgentSession({ root, buildOptions: forcedSessionBuildOptions }); |
There was a problem hiding this comment.
Fixed in 9a034f1: removed --cache from the init/status/sync CLI schemas (new LIFECYCLE_BUILD_OPTIONS = SHARED_BUILD_OPTIONS minus --cache) rather than honoring an override \u2014 lifecycle's disk-cache warmup is the documented, deliberate behavior of these commands (per docs/cli.md's "Project lifecycle" section), so the CLI now truthfully rejects a flag it never acted on instead of silently ignoring it. Updated LIFECYCLE_HELP_TEXT to match. Added a validation test confirming init/status/sync/uninit all reject --cache, plus a control assertion that unrelated commands like orient/search still accept it.
…mmands, surface manifest read failures as user errors
| Index options: | ||
| Supports shared --cache, --cache-strict, --cache-verify, --threads, --native, --workers, --include-glob, --ignore-glob, and --no-gitignore options. | ||
| Supports shared --cache-strict, --cache-verify, --threads, --native, --workers, --include-glob, --ignore-glob, and --no-gitignore options. init/sync always warm the disk cache and do not accept --cache. |
There was a problem hiding this comment.
Good catch — that was my own error: I meant to edit LIFECYCLE_HELP_TEXT's "Index options" line but hit EXPLORE_HELP_TEXT's instead (they share the same wording and I mismapped the line number). Fixed in d2534b6: EXPLORE_HELP_TEXT is restored verbatim to its original text (explore genuinely accepts --cache via SHARED_BUILD_OPTIONS, unaffected by the lifecycle-only fix). Checked LIFECYCLE_HELP_TEXT itself — it never claimed --cache support in the first place (its "State:" section already only describes disk-cache warmup, no options list), so no further edit was needed there.
| export function stableStringify(value: unknown): string { | ||
| if (Array.isArray(value)) { | ||
| return `[${value.map((item) => (item === undefined ? "null" : stableStringify(item))).join(",")}]`; | ||
| } | ||
| if (typeof value !== "object" || value === null) return JSON.stringify(value); | ||
| const entries = Object.entries(value) | ||
| .filter(([, nested]) => nested !== undefined) | ||
| .sort(([left], [right]) => left.localeCompare(right)); | ||
| return `{${entries.map(([key, nested]) => `${JSON.stringify(key)}:${stableStringify(nested)}`).join(",")}}`; | ||
| } |
There was a problem hiding this comment.
Fixed in 935c4ab: stableStringify is now total. Two changes: (1) leaf serialization checks whether JSON.stringify(value) itself returned undefined (functions, symbols, and the top-level undefined case) and coalesces to the string "null" instead of leaking the JS undefined value; (2) array serialization now reads elements by explicit index in a for-loop instead of Array.prototype.map/.join, so sparse-array holes are treated the same as JSON.stringify treats them (coerced to null) rather than being silently skipped — stableStringify(new Array(1)) now correctly differs from stableStringify([]) and matches stableStringify([undefined]). Added tests for: top-level undefined, sparse-array vs empty-array vs explicit-undefined-element, function/symbol leaves (top-level and nested inside an object value).
|
|
||
| expect(report.manifest?.used).toBe(true); | ||
| expect(report.manifest?.reused).toBe(false); | ||
| expect(report.manifest?.reason).toBe("graphOptionsMismatch"); |
There was a problem hiding this comment.
Fixed in 935c4ab: relaxed the assertion to expect(report.manifest?.reason).toBeTruthy() instead of pinning the exact literal. Traced the label to src/indexer/build-index.ts (buildProjectIndexIncremental, not part of this PR's diff) — it only distinguishes "buildOptionsMismatch" (discovery-option changes) from a generic "graphOptionsMismatch" fallback covering both actual graph-option changes and config-hash changes, so "graphOptionsMismatch" is a pre-existing, arguably-mislabeled catch-all rather than something introduced by this PR. Left that reason-reporting logic alone since it's out of scope here (broader blast radius, unrelated file) and just stopped the test from locking in the imprecise label, per your suggestion.
…he-invalidation reason assertion
| async function readCodegraphDirEntries(dir: string): Promise<string[]> { | ||
| try { | ||
| return await fsp.readdir(dir); | ||
| } catch (error) { | ||
| if (error instanceof Error && "code" in error && error.code === "ENOENT") return []; | ||
| throw error; | ||
| } | ||
| } |
There was a problem hiding this comment.
Fixed in 5444ce5: readCodegraphDirEntries now throws CodegraphLifecycleUserError instead of a raw Error for non-ENOENT fs.readdir failures, matching the same pattern already used for the manifest read path. Regression test mocks fsp.readdir to reject EACCES for the .codegraph directory (delegating other paths to the real implementation) and asserts uninitCodegraphLifecycle rejects with instanceof CodegraphLifecycleUserError.
| async function removeDirIfEmpty(dir: string): Promise<void> { | ||
| try { | ||
| await fsp.rmdir(dir); | ||
| } catch (error) { | ||
| if (error instanceof Error && "code" in error && error.code === "ENOTEMPTY") return; | ||
| if (error instanceof Error && "code" in error && error.code === "ENOENT") return; | ||
| throw error; | ||
| } | ||
| } |
There was a problem hiding this comment.
Fixed in 5444ce5: removeDirIfEmpty now throws CodegraphLifecycleUserError instead of a raw Error for non-ENOTEMPTY/non-ENOENT fs.rmdir failures. Regression test mocks fsp.rmdir to reject EACCES for the .codegraph directory (delegating other paths to the real implementation), driving the non-force uninit path down to removeDirIfEmpty after the manifest file is removed, and asserts uninitCodegraphLifecycle rejects with instanceof CodegraphLifecycleUserError.
| await fsp.chmod(gitignorePath, 0o000); | ||
|
|
||
| const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => undefined); | ||
| try { | ||
| const result = await initCodegraphLifecycle(root); | ||
|
|
||
| // computeConfigHash is also consulted by the indexer's own disk-cache build/manifest layers | ||
| // (each logging their own generic "Warning: ..." message), so assert on the lifecycle-specific | ||
| // wording rather than an exact call count. | ||
| expect(warnSpy).toHaveBeenCalled(); | ||
| const lifecycleWarnCall = warnSpy.mock.calls.find( | ||
| (call) => typeof call[0] === "string" && call[0].includes("Codegraph lifecycle config drift check"), | ||
| ); | ||
| expect(lifecycleWarnCall).toBeDefined(); | ||
| expect(lifecycleWarnCall?.[0]).toEqual(expect.stringContaining(".gitignore")); | ||
|
|
||
| // The command completes successfully (no throw) and still produces a hash-backed manifest, | ||
| // computed from whichever config files it *could* read (here, package.json). | ||
| expect(result.manifest.configHash).toMatch(/^[0-9a-f]{40}$/); | ||
| expect(await readManifest(root)).toEqual(result.manifest); | ||
| } finally { | ||
| warnSpy.mockRestore(); | ||
| await fsp.chmod(gitignorePath, 0o644); | ||
| } |
There was a problem hiding this comment.
Fixed in 5444ce5: rewrote the test to mock fsp.readFile (rejecting EACCES only for the .gitignore path, delegating everything else to the real implementation) instead of fsp.chmod, mirroring the existing readLifecycleManifest EACCES test's pattern in the same file. Deterministic across platforms now, no chmod dependency.
…deflake config-unreadable test
| function diffLifecycleFileCounts( | ||
| previous: readonly string[] | undefined, | ||
| current: readonly string[] | undefined, | ||
| totalDelta: number, | ||
| ): { added: number; removed: number; totalDelta: number } { | ||
| if (previous === undefined || current === undefined) { | ||
| // Legacy manifest predates per-file tracking; approximate from the net file-count delta. | ||
| return { added: Math.max(0, totalDelta), removed: Math.max(0, -totalDelta), totalDelta }; | ||
| } | ||
| const previousSet = new Set(previous); | ||
| const currentSet = new Set(current); | ||
| const added = current.filter((file) => !previousSet.has(file)).length; | ||
| const removed = previous.filter((file) => !currentSet.has(file)).length; | ||
| return { added, removed, totalDelta }; | ||
| } |
There was a problem hiding this comment.
Fixed in ddbe388: diffLifecycleFileCounts now derives totalDelta from current.length - previous.length directly (the file lists themselves) whenever both are present, instead of trusting a caller-supplied fileCount-based delta. This guarantees totalDelta === added - removed always holds when per-file tracking is available, regardless of whether a manifest's fileCount and files.length have diverged. Regression test hand-desyncs a manifest's fileCount to 99 while its real files array stays 2 entries, then syncs with a 1-add/1-remove churn; asserts totalDelta === current.length - previous.length (0) and totalDelta === added - removed, proving the old fileCount-trusting math (which would have produced 2 - 99 = -97) is gone.
| function formatStatus(status: CodegraphLifecycleStatus): string { | ||
| if (!status.initialized) { | ||
| return `Codegraph is not initialized at ${status.root}. Next: ${status.suggestedNextCommand}`; | ||
| } | ||
| const fileCount = status.fileCount ? `${status.fileCount.then} then, ${status.fileCount.current} current` : "unknown"; | ||
| const configChanged = status.configChanged ? "yes" : "no"; | ||
| const buildOptionsChanged = status.buildOptionsChanged ? "yes" : "no"; | ||
| const analysis = status.analysis?.label ?? "unknown"; | ||
| return [ | ||
| `Codegraph initialized at ${status.root}.`, | ||
| `Last sync: ${status.lastSyncAt ?? "unknown"}`, | ||
| `Files: ${fileCount}`, | ||
| `Config changed: ${configChanged}`, | ||
| `Build options changed: ${buildOptionsChanged}`, | ||
| `Analysis: ${analysis}`, | ||
| `Next: ${status.suggestedNextCommand}`, | ||
| ].join("\n"); | ||
| } |
There was a problem hiding this comment.
Fixed in ddbe388: formatStatus now includes a 'Files changed: yes/no' line. While in the area, also fixed the closely related issue in formatSyncResult: it previously showed only the net delta, so e.g. 3 files added and 3 removed in the same sync produced totalDelta: 0 and the change label was suppressed entirely (deltaLabel was only shown when non-zero), silently hiding real churn. It now prints explicit +/- instead. Regression tests cover both via captureCli against the actual pretty-printed stdout (no --json).
…, transparent pretty output, remaining error wrapping, narrow status schema
|
Also ran a proactive self-review pass (per request) looking for issues matching this thread's established pattern before the next automated review round. Fixed 4 more, alongside the two confirmed findings above, all in ddbe388:
Considered but deliberately did NOT add: a |
Summary
Verification