v1.5.0 — interactive archive picker, restore subsets, framework protection#5
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis release (v1.5.0) introduces interactive archival via TUI picker, adds Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/ccaudit/src/__tests__/fixtures/tmux-e2e.md (1)
119-120:⚠️ Potential issue | 🟡 MinorRemove stray literal at EOF (Line 120).
Line 120 contains an orphaned
120token that will render as unintended content in the markdown fixture.🧹 Proposed fix
Those remain human QA rows. -120🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ccaudit/src/__tests__/fixtures/tmux-e2e.md` around lines 119 - 120, Remove the stray orphaned literal "120" at the end of the tmux-e2e.md fixture (the token at EOF); delete that trailing numeric token so the file ends with the intended markdown content/newline and re-run the tests to confirm no unintended content is rendered.apps/ccaudit/src/cli/commands/restore.ts (1)
649-656:⚠️ Potential issue | 🟡 MinorUpdate the restore summaries to account for command restores.
counts.unarchivednow includescommanditems, but these labels still say "agents/skills". A restore that only replays command archives will be reported under the wrong category.Also applies to: 765-776
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ccaudit/src/cli/commands/restore.ts` around lines 649 - 656, Update the restore summary text to correctly reflect that counts.unarchived may include commands: change the hardcoded "agents/skills" label in the lines that push summaries using c.unarchived (and the similar block at the other occurrence) to a more accurate label such as "agents/skills/commands" or a generic "items" so command restores are reported correctly; locate the pushes that reference c.unarchived (and the corresponding block around lines 765-776) and replace the label text only, leaving the existing counts and note interpolation intact.
🟡 Minor comments (14)
apps/ccaudit/src/__tests__/restore-flag-mutual-exclusion.test.ts-48-64 (1)
48-64:⚠️ Potential issue | 🟡 MinorAdd the missing
--interactive --all-matchingcombination.The file header asserts that "combining any two of the three mode-selecting flags is a hard error", but only 2 of the 3 pairs are exercised:
--interactive+--nameand--name+--all-matching. The third pair (--interactive+--all-matching) is uncovered, which would let a regression in that specific branch ofvalidateRestoreFlagExclusionslip through this guard.🧪 Suggested third case
+ it('--interactive --all-matching foo → exit 1, "flags are mutually exclusive" on stderr', async () => { + const r = await runCcauditCli( + tmpHome, + ['restore', '--interactive', '--all-matching', 'foo'], + { env: { PATH: `${path.join(tmpHome, 'bin')}:${process.env.PATH ?? ''}` } }, + ); + expect(r.exitCode).toBe(1); + expect(r.stderr).toContain('flags are mutually exclusive'); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ccaudit/src/__tests__/restore-flag-mutual-exclusion.test.ts` around lines 48 - 64, Add a third test case that asserts the CLI errors when combining --interactive and --all-matching: mirror the existing tests that call runCcauditCli(tmpHome, ['restore', ...], { env: { PATH: ... } }) but pass ['restore', '--interactive', '--all-matching', 'foo']; assert exitCode is 1 and stderr contains 'flags are mutually exclusive'. Place the new it(...) alongside the other two tests in restore-flag-mutual-exclusion.test.ts so validateRestoreFlagExclusion's --interactive + --all-matching branch is covered.apps/ccaudit/src/__tests__/restore-json-envelope.test.ts-1-14 (1)
1-14:⚠️ Potential issue | 🟡 MinorDocstring describes a non-existent envelope nesting that contradicts what the test actually reads.
The header comment (lines 5–10) claims fields are either top-level (
command,exit_code) or nested underdata(data.status,data.selection_filter,data.skipped). The test file itself reveals the truth: the envelope has ametawrapper for command/exit-code, andstatus,selection_filter, andskippedsit at the root level—nodatawrapper. Since the suite is green, the docstring is the stale half; align it to the actual envelope shape so future readers don't get misdirected.Also note: the docstring uses snake_case
exit_codewhile the actual field is camelCaseexitCodein the envelope.📝 Correct docstring rewrite
- * Asserts the v1.5 additive fields land in the envelope: - * - top-level `command === 'restore'`, `exit_code === 0` - * - `data.status === 'success'` - * - `data.selection_filter.mode === 'subset'` - * - `data.selection_filter.ids` has exactly 2 entries (pencil-dev + pencil-review) - * - `Array.isArray(data.skipped)` is true (empty on the happy path, but present) + * Asserts the v1.5 additive fields land in the envelope: + * - `meta.command === 'restore'`, `meta.exitCode === 0` + * - `status === 'success'` + * - `selection_filter.mode === 'subset'` + * - `selection_filter.ids` has exactly 2 entries (pencil-dev + pencil-review) + * - `Array.isArray(skipped)` is true (empty on the happy path, but present)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ccaudit/src/__tests__/restore-json-envelope.test.ts` around lines 1 - 14, Update the docstring at the top of restore-json-envelope.test.ts to match the actual envelope shape the test reads: state that command and exitCode are inside a meta wrapper (use camelCase exitCode), and that status, selectionFilter (with mode and ids), and skipped are at the root (not under a data wrapper). Mention selectionFilter.mode === 'subset' and that selectionFilter.ids contains exactly two entries, and that skipped is an array (possibly empty). Ensure the wording replaces references to a non-existent data wrapper and to snake_case exit_code.apps/ccaudit/src/__tests__/scanner-multi-config-mcp.test.ts-91-94 (1)
91-94:⚠️ Potential issue | 🟡 MinorTautological "sortedness" assertion —
[...arr]preserves order, sotoEqualalways passes.
const sorted = [...it.configRefs!]is a shallow copy, not a sorted copy, soexpect(it.configRefs).toEqual(sorted)is structurally equivalent toexpect(arr).toEqual(arr)and cannot fail. The comment "Sorted output is stable across invocations" describes what should be tested, but the assertion doesn't test it.🐛 Proposed fix — actually validate stability across invocations
- const items = await scanMcpServers(fixture.userConfigPath, fixture.projectPaths); + const items = await scanMcpServers(fixture.userConfigPath, fixture.projectPaths); + // Stability: a second scan produces the same configRefs in the same order. + const items2 = await scanMcpServers(fixture.userConfigPath, fixture.projectPaths); const quintetItems = items.filter((i) => i.name === 'quintet'); + const quintetItems2 = items2.filter((i) => i.name === 'quintet'); expect(quintetItems.length).toBeGreaterThanOrEqual(1); - for (const it of quintetItems) { + for (let idx = 0; idx < quintetItems.length; idx++) { + const it = quintetItems[idx]!; // At minimum: project-local + user scopes both present. expect(it.configRefs!.length).toBeGreaterThanOrEqual(2); - // Sorted output is stable across invocations. - const sorted = [...it.configRefs!]; - expect(it.configRefs).toEqual(sorted); + // Stable across invocations (same input → same output order). + expect(it.configRefs).toEqual(quintetItems2[idx]?.configRefs); }If the intent is to validate bucket order (project-local → user → system) instead of just stability, assert that explicitly via path predicates as the first test in this file already does on lines 53-56.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ccaudit/src/__tests__/scanner-multi-config-mcp.test.ts` around lines 91 - 94, The assertion using const sorted = [...it.configRefs!] is tautological because it's only a shallow copy and will always equal the original; replace it with a real stability/sorting check: either create a deterministically sorted version (e.g., sort([...it.configRefs!]) using the same comparator used by the production code and assert expect(it.configRefs).toEqual(sortedVersion), or call the code that produces configRefs twice and assert the two results are equal (to verify stability across invocations); alternatively, assert the explicit bucket order (project-local → user → system) via path predicates like the earlier test does, referencing the test variable it.configRefs to locate where to change the expectation.apps/ccaudit/scripts/bundle-size-check.mjs-62-72 (1)
62-72:⚠️ Potential issue | 🟡 MinorValidate
CCAUDIT_PHASE_BUDGET_BYTESlike the baselines.Both baseline parses are guarded with
Number.isFinite(...) || x <= 0, butphaseBudgetis not. IfCCAUDIT_PHASE_BUDGET_BYTESis set to a non-numeric value (or empty string —??only catchesundefined),parseIntreturnsNaN, andphaseDelta > NaNis alwaysfalse. The phase-local size gate then silently passes regardless of how large the bundle grew.🛡️ Proposed fix
- const phaseBudget = Number.parseInt(process.env.CCAUDIT_PHASE_BUDGET_BYTES ?? '10240', 10); + const phaseBudgetRaw = process.env.CCAUDIT_PHASE_BUDGET_BYTES; + const phaseBudget = Number.parseInt( + phaseBudgetRaw && phaseBudgetRaw.length > 0 ? phaseBudgetRaw : '10240', + 10, + ); + if (!Number.isFinite(phaseBudget) || phaseBudget <= 0) { + console.error( + `[bundle-size] FAIL: invalid CCAUDIT_PHASE_BUDGET_BYTES: ${JSON.stringify(phaseBudgetRaw)}`, + ); + process.exit(1); + } const phaseDelta = actual - phaseBaseline;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ccaudit/scripts/bundle-size-check.mjs` around lines 62 - 72, The phase budget parsed from CCAUDIT_PHASE_BUDGET_BYTES is not validated, so non-numeric or empty values produce NaN and silently disable the check; update the parsing of phaseBudget (where Number.parseInt is used and the CCAUDIT_PHASE_BUDGET_BYTES env var is read) to mirror the baseline validation: after parsing, check if Number.isFinite(phaseBudget) and phaseBudget > 0, and if not, assign the same default (10240) before using phaseBudget in the comparison and logging.apps/ccaudit/src/__tests__/tabbed-picker-live-counter.test.ts-474-480 (1)
474-480:⚠️ Potential issue | 🟡 MinorTautological assertion — the OR clause makes this check vacuously true.
afterResize.includes('Terminal too small')was already asserted to betrueon lines 464-467, so the disjunction here always passes regardless ofafterSecondSpace's contents. The check fails to validate the intended invariant: that the banner remains rendered after the suppressed Space input. If a future regression caused the banner to disappear after a key press in sub-minimum state, this test would not catch it.🐛 Proposed fix to actually verify banner persistence after suppressed input
const afterSecondSpace = stripAnsi(stdoutBuf.slice(beforeLen)); - // Banner still present (interactivity suppressed). + // Banner still present (interactivity suppressed). Either the post-Space + // slice re-rendered the banner OR the diff renderer produced no new + // frame at all (zero new content) — both prove suppression. + const fullAfter = stripAnsi(stdoutBuf); expect( - afterSecondSpace.includes('Terminal too small') || - afterResize.includes('Terminal too small'), - `expected banner after suppressed Space; stdoutSlice:\n${afterSecondSpace}`, + afterSecondSpace.includes('Terminal too small') || afterSecondSpace.trim() === '', + `expected banner re-rendered or zero new bytes after suppressed Space; got:\n${afterSecondSpace}`, + ).toBe(true); + // Defensive: counter must NOT have incremented (Space was suppressed). + expect( + fullAfter.includes('1 of 3 selected across all tabs') && + !fullAfter.includes('2 of 3 selected across all tabs'), + `Space should have been suppressed in sub-minimum state`, ).toBe(true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ccaudit/src/__tests__/tabbed-picker-live-counter.test.ts` around lines 474 - 480, The assertion is tautological because afterResize was already asserted true; update the test in tabbed-picker-live-counter.test (look for variables stdoutBuf, beforeLen, afterResize, afterSecondSpace) to explicitly assert that afterSecondSpace includes the banner text—i.e., replace the disjunction with a direct check that afterSecondSpace.includes('Terminal too small') (or assert both afterResize and afterSecondSpace separately) so the test verifies the banner persists after the suppressed Space input.apps/ccaudit/src/__tests__/restore-interactive-roundtrip.test.ts-134-145 (1)
134-145:⚠️ Potential issue | 🟡 MinorInverted polling condition — the "wait for picker readiness" loop is dead code in both this test and the referenced
safety-invariants-tui-abort.test.ts.The condition on line 138 reads
while (spawned.child.exitCode !== null && ...), meaning while the child has exited. Since the child just spawned and is waiting on stdin,exitCode === null, so the loop condition is false on the first check and the loop body never executes. The exponential-backoff polling is dead code; only the 400ms grace period on lines 142–144 actually waits.The comment states the intent clearly: "poll child.exitCode === null with exponential backoff." The reference test
safety-invariants-tui-abort.test.ts(lines 139 and 284) has the identical inverted conditionexitCode !== nulland an explicit comment at line 127 confirming intent: "We pollchild.exitCode === null". Both tests work only because the grace period (200–300ms in the reference, 400ms here) happens to suffice for the picker to mount. Under CI slowdown or slower picker initialization, both tests will race.🐛 Proposed fix
const maxWaitMs = 5_000; const startMs = Date.now(); let delayMs = 100; - while (spawned.child.exitCode !== null && Date.now() - startMs < maxWaitMs) { + while (spawned.child.exitCode === null && Date.now() - startMs < maxWaitMs) { await new Promise((r) => setTimeout(r, delayMs)); delayMs = Math.min(delayMs * 2, 500); + if (Date.now() - startMs > 600) break; } if (spawned.child.exitCode === null) { await new Promise((r) => setTimeout(r, 400)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ccaudit/src/__tests__/restore-interactive-roundtrip.test.ts` around lines 134 - 145, The loop condition is inverted so the exponential-backoff polling never runs; change the while in the test block that references spawned.child.exitCode (and variables maxWaitMs, startMs, delayMs) to loop while spawned.child.exitCode === null && Date.now() - startMs < maxWaitMs so it polls until the child exits or timeout, keeping the same backoff logic and retaining the final short grace sleep (await new Promise(...)) for safety.README.md-468-468 (1)
468-468:⚠️ Potential issue | 🟡 MinorWording: "MCP re-enable ops" is inaccurate.
Purge classifies archive ops only; the ops it skips are MCP disable ops (the
name → ccaudit-disabled:namekey-rename written bybust), not "re-enable" ops (which would only ever be produced by a future restore-side mutation). The same phrasing appears verbatim indocs/JSON-SCHEMA.mdline 186 — both should be corrected together for consistency.📝 Proposed wording
- Scope is **archive ops only**. Flag ops (memory frontmatter) and MCP re-enable ops are untouched by this command. Each executed mutation is recorded as an append-only `archive_purge` op in a fresh `purge-<ts>-<rand>.jsonl` manifest — the original archive op stays intact for audit. Real purge requires an **explicit `--yes`**; there is no prompt fallback. Failures on individual items are reported in `purge.failures[]` but do not abort the batch. + Scope is **archive ops only**. Flag ops (memory frontmatter) and disable ops (MCP key-rename) are untouched by this command. Each executed mutation is recorded as an append-only `archive_purge` op in a fresh `purge-<ts>-<rand>.jsonl` manifest — the original archive op stays intact for audit. Real purge requires an **explicit `--yes`**; there is no prompt fallback. Failures on individual items are reported in `purge.failures[]` but do not abort the batch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 468, Update the incorrect phrasing "MCP re-enable ops" to "MCP disable ops" and add the clarifying parenthetical used in the comment; locate the exact sentence in README.md and the identical sentence in docs/JSON-SCHEMA.md (search for the string "MCP re-enable ops"), and change it to something like "MCP disable ops (the `name → ccaudit-disabled:name` key-rename written by `bust`)" so the docs correctly state that purge skips MCP disable ops rather than re-enable ops.docs/JSON-SCHEMA.md-186-186 (1)
186-186:⚠️ Potential issue | 🟡 MinorWording: "disable (MCP re-enable)" is the wrong gloss.
The op-type that purge skips is
disable(the bust-time MCP key-renamename → ccaudit-disabled:name). "Re-enable" is whatrestoreperforms later, and is also untouched by purge — but the parenthetical here confusingly labels the disable op as a "re-enable" op. The README mirrors this same wording at line 468. Suggest:📝 Proposed wording
-**Scope.** Archive ops only. Flag (memory frontmatter) and disable (MCP re-enable) ops are **never** touched by purge. `classifyArchiveOps` filters by `op_type === 'archive'`. +**Scope.** Archive ops only. Flag ops (memory frontmatter) and disable ops (MCP key-rename) are **never** touched by purge. `classifyArchiveOps` filters by `op_type === 'archive'`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/JSON-SCHEMA.md` at line 186, The parenthetical "disable (MCP re-enable)" is incorrect — the op purge skips is disable (the MCP bust-time key-rename `name → ccaudit-disabled:name`) and "re-enable" is performed later by restore; update the wording in the sentence that mentions classifyArchiveOps and the mirrored sentence in the README to replace the parenthetical with a correct gloss such as "(MCP key-rename `name → ccaudit-disabled:name`)" or simply "(disable)" and ensure `disable` and `restore` are named distinctly so readers cannot confuse the two.CHANGELOG.md-8-8 (1)
8-8:⚠️ Potential issue | 🟡 MinorMinor: inconsistent date separator between version headers.
Line 8 uses
[1.5.0] - 2026-04-26(hyphen) while Line 87 uses[1.5.0-beta.0] — 2026-04-19(em-dash). Keep a Changelog uses a hyphen. The em-dash on the beta header is the only one in the file (the rest of the existing entries — 1.4.0, 1.3.1, 1.3.0, 1.2.1 — all use a hyphen).📝 Suggested fix
-## [1.5.0-beta.0] — 2026-04-19 +## [1.5.0-beta.0] - 2026-04-19Also applies to: 87-87
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CHANGELOG.md` at line 8, Replace the em-dash in the beta header with a hyphen so changelog version headers use a consistent separator: update the header for "[1.5.0-beta.0] — 2026-04-19" to use a hyphen ("-") like "[1.5.0] - 2026-04-26", ensuring all version headers (e.g., the "[1.5.0-beta.0]" header) match the hyphen style used elsewhere.apps/ccaudit/src/__tests__/tmux-e2e-manual-qa.test.ts-35-41 (1)
35-41:⚠️ Potential issue | 🟡 MinorTop-level
beforeAlldist guard fires even when the suite is opt-in-skipped.
beforeAllis registered at module scope, not inside thedescribe.skipIf(... || !optIn)block at Line 64, so running this file withoutCCAUDIT_TMUX_E2E=1and without a builtdist/will still throw from the guard. Move the dist-existence check inside the describe (or gate it byoptIn) so the file is a no-op for normalpnpm testruns without a fresh build.🛡️ Suggested fix
-beforeAll(() => { - if (!existsSync(distPath)) { - throw new Error( - `dist binary not found at ${distPath}. Run \`pnpm -F ccaudit build\` before running this test.`, - ); - } -}); @@ describe.skipIf(process.platform === 'win32' || !optIn)( 'tmux E2E manual-QA coverage (opt-in)', () => { + beforeAll(() => { + if (!existsSync(distPath)) { + throw new Error( + `dist binary not found at ${distPath}. Run \`pnpm -F ccaudit build\` before running this test.`, + ); + } + });Also applies to: 64-66
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ccaudit/src/__tests__/tmux-e2e-manual-qa.test.ts` around lines 35 - 41, The top-level beforeAll guard that checks existsSync(distPath) must not run unconditionally; move that dist existence check into the describe.skipIf(...) block (or wrap it with the same optIn condition) so it only executes when the suite is opted-in; specifically relocate the beforeAll that references distPath and existsSync into the describe.skipIf(... || !optIn) scope (or add an early if (!optIn) return around it) so describe.skipIf, optIn, beforeAll, distPath and existsSync are aligned and the file is a no-op when not opt-in.apps/ccaudit/src/cli/commands/purge-archive.ts-105-109 (1)
105-109:⚠️ Potential issue | 🟡 MinorMutual-exclusion failure path bypasses
--jsonenvelope and history recording.When both
--dry-runand--yesare set, the command writes a plain stderr message and exits without honoringoutMode.jsonor invokingsafeRecordHistory. Compare with the classification (Lines 160-191) and execute (Lines 202-239) failure paths, which both emit a JSON envelope onoutMode.jsonand record history before exiting. JSON consumers will see no parsable output, and the audit trail at~/.claude/ccaudit/history.jsonlwill miss this invocation.🛡️ Suggested fix
// D6 safety gate: --yes and --dry-run are mutually exclusive. if (dryRunFlag && yesFlag) { - process.stderr.write('flags are mutually exclusive: --dry-run, --yes\n'); + const message = 'flags are mutually exclusive: --dry-run, --yes'; + if (outMode.json) { + process.stdout.write( + JSON.stringify( + buildJsonEnvelope('purge-archive', 'n/a', 1, { + purge: { + summary: { + purgedCount: 0, + reclaimedCount: 0, + skippedOccupiedCount: 0, + staleFilteredCount: 0, + }, + failures: [{ path: '', reason: message }], + dryRun: true, + }, + }), + ) + '\n', + ); + } else { + process.stderr.write(`${message}\n`); + } + await safeRecordHistory({ + homeDir, + command: 'purge-archive', + argv, + exitCode: 1, + durationMs: Date.now() - historyStartMs, + cwd: process.cwd(), + result: null, + errors: [message], + ccauditVersion: CCAUDIT_VERSION, + }); process.exit(1); }Note: this requires moving the
homeDir/historyStartMs/safeRecordHistorysetup (Lines 115-131) above the mutual-exclusion check.Based on learnings: "History writes must never crash the main command;
recordHistorymust be wrapped in try/catch at the outermost layer; on failure emit one stderr warning and continue".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ccaudit/src/cli/commands/purge-archive.ts` around lines 105 - 109, Move the homeDir/historyStartMs/safeRecordHistory setup so it runs before the mutual-exclusion check of dryRunFlag and yesFlag, and change the mutual-exclusion failure path to emit the same JSON envelope when outMode.json is set and to invoke safeRecordHistory before exiting; ensure the call to recordHistory (safeRecordHistory) is wrapped in try/catch so it cannot crash the command—on failure write a single stderr warning and continue—use the existing symbols dryRunFlag, yesFlag, outMode.json, safeRecordHistory, homeDir, and historyStartMs to locate and update the logic.apps/ccaudit/src/__tests__/tmux-e2e-manual-qa.test.ts-67-93 (1)
67-93:⚠️ Potential issue | 🟡 MinorSilent
returnwhen tmux is missing causes tests to falsely report green.Each test starts with
if (!(await hasTmux())) return;. Without tmux installed, every assertion is skipped and the test reports as passed — which is indistinguishable from real coverage. Promote this to an explicit skip so the absence is visible in the report:beforeAll(async () => { if (!(await hasTmux())) { // Vitest 4.1: skip the whole describe at runtime ctx.skip(); } });…or pivot to a top-level
describe.skipIf(...)augmented by a synchronous tmux probe. As written, a contributor on a machine without tmux gets a green run that hides regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ccaudit/src/__tests__/tmux-e2e-manual-qa.test.ts` around lines 67 - 93, Tests silently return when tmux is missing because each case uses "if (!(await hasTmux())) return;", so change the suite to explicitly skip when tmux is absent: add a beforeAll that awaits hasTmux() and calls ctx.skip() if false (or replace the top-level block with a describe.skipIf / synchronous tmux probe) so missing tmux produces a visible skipped test instead of silently passing; locate uses of hasTmux() in the tmux-e2e-manual-qa.test.ts file (the individual it blocks and the session helpers like startTmuxE2E) and remove the per-test early returns after implementing the centralized beforeAll/describe.skipIf guard.apps/ccaudit/src/__tests__/safety-invariants-framework.test.ts-221-230 (1)
221-230:⚠️ Potential issue | 🟡 MinorTest C leaks the
beforeEach-allocated tmpdir.
beforeEach(Line 63) creates anccaudit-inv-s6-*tmpdir with the single-framework fixture, then Test C overwrites thetmpHomebinding with a newccaudit-inv-s6-multi-*tmpdir at Lines 222-230. The original directory is now unreferenced and theafterEachrm(tmpHome, ...)(Line 75) cleans up only the multi-framework one. Every run of Test C leaks the baseline tmpdir underos.tmpdir().🛡️ Suggested fix
it('Test C — INV-S6 multi-framework: ...', async () => { - tmpHome = await (async () => { - const p = await mkdtemp(path.join(tmpdir(), 'ccaudit-inv-s6-multi-')); - await buildFakePs(p); - await createMultiFrameworkFixture(p, [ - { prefix: 'gsd', usedMembers: ['planner'], ghostMembers: ['researcher', 'verifier'] }, - { prefix: 'sc', usedMembers: ['analyze'], ghostMembers: ['audit', 'improve'] }, - ]); - return p; - })(); + // Discard the single-framework baseline staged by beforeEach and reuse the + // same tmpHome to avoid leaking the unreferenced fixture directory. + await rm(tmpHome, { recursive: true, force: true }); + tmpHome = await mkdtemp(path.join(tmpdir(), 'ccaudit-inv-s6-multi-')); + await buildFakePs(tmpHome); + await createMultiFrameworkFixture(tmpHome, [ + { prefix: 'gsd', usedMembers: ['planner'], ghostMembers: ['researcher', 'verifier'] }, + { prefix: 'sc', usedMembers: ['analyze'], ghostMembers: ['audit', 'improve'] }, + ]);Alternative: extend
createMultiFrameworkFixtureto be additive against an existing tmpHome (or wipe.claude/first) and skip the secondmkdtemp.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ccaudit/src/__tests__/safety-invariants-framework.test.ts` around lines 221 - 230, The test leaks the tmpdir because beforeEach created a ccaudit-inv-s6-* tmpHome and Test C reassigns tmpHome by calling mkdtemp again; instead reuse the beforeEach tmpHome by removing the mkdtemp/new tmpHome allocation in Test C and call createMultiFrameworkFixture(tmpHome, [...]) (or, if you prefer the alternate approach, make createMultiFrameworkFixture accept an existing tmpHome and mutate it) so only the single tmpHome created in beforeEach is referenced and cleaned up by afterEach (refer to the tmpHome variable, beforeEach/afterEach hooks, mkdtemp call in Test C, and createMultiFrameworkFixture).apps/ccaudit/src/__tests__/fixtures/manual-qa-followups.ts-223-224 (1)
223-224:⚠️ Potential issue | 🟡 MinorUse real ISO timestamps in the fixture headers.
These
checkpoint_timestampvalues are filename-safe dashed strings, not ISO-8601 UTC timestamps. Any test path that parses header times will now diverge from production manifests.As per coding guidelines, "Use ISO 8601 UTC strings (
new Date().toISOString()) for time values".Also applies to: 285-286
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ccaudit/src/__tests__/fixtures/manual-qa-followups.ts` around lines 223 - 224, The fixture header uses filename-safe dashed timestamps for checkpoint_timestamp (e.g., checkpoint_timestamp: '2026-04-22T09-00-00-000Z') instead of a real ISO-8601 UTC string; update each checkpoint_timestamp entry in this fixture (manual-qa-followups.ts) to a proper ISO 8601 UTC format produced by new Date().toISOString() (for example '2026-04-22T09:00:00.000Z'), and make the same change for the other occurrence of checkpoint_timestamp in this file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 555e64e8-8245-4110-ae30-6a7bcb37be2f
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (117)
CHANGELOG.mdCLAUDE.mdREADME.mdapps/ccaudit/package.jsonapps/ccaudit/scripts/bundle-baseline-03.2.txtapps/ccaudit/scripts/bundle-baseline-phase-05.txtapps/ccaudit/scripts/bundle-baseline-phase-06.txtapps/ccaudit/scripts/bundle-baseline-phase-09.txtapps/ccaudit/scripts/bundle-baseline.txtapps/ccaudit/scripts/bundle-size-check.mjsapps/ccaudit/scripts/bundle-smoke-test.mjsapps/ccaudit/src/__tests__/__fixtures__/regime-post-2-1-117/.claude.jsonapps/ccaudit/src/__tests__/__fixtures__/regime-post-2-1-117/.claude/settings.jsonapps/ccaudit/src/__tests__/__fixtures__/regime-pre-2-1-116/.claude.jsonapps/ccaudit/src/__tests__/__fixtures__/regime-pre-2-1-116/.claude/settings.jsonapps/ccaudit/src/__tests__/__fixtures__/restore-interactive/bust-2026-04-17T10-00-00-000Z-aaaa.jsonlapps/ccaudit/src/__tests__/__fixtures__/restore-interactive/bust-2026-04-18T10-00-00-000Z-bbbb.jsonlapps/ccaudit/src/__tests__/_test-helpers.tsapps/ccaudit/src/__tests__/command-archival.test.tsapps/ccaudit/src/__tests__/empty-inventory.test.tsapps/ccaudit/src/__tests__/fixtures/ghost-500-items.tsapps/ccaudit/src/__tests__/fixtures/manual-qa-followups.mdapps/ccaudit/src/__tests__/fixtures/manual-qa-followups.tsapps/ccaudit/src/__tests__/fixtures/tmux-e2e.mdapps/ccaudit/src/__tests__/fixtures/tmux-e2e.tsapps/ccaudit/src/__tests__/ghost-select-ids.test.tsapps/ccaudit/src/__tests__/help-output.test.tsapps/ccaudit/src/__tests__/interactive-smoke.test.tsapps/ccaudit/src/__tests__/no-interactive-env.test.tsapps/ccaudit/src/__tests__/pagination-500.test.tsapps/ccaudit/src/__tests__/preflight-retry.test.tsapps/ccaudit/src/__tests__/purge-archive.test.tsapps/ccaudit/src/__tests__/regime-detection-cc-2-1-117-compat.test.tsapps/ccaudit/src/__tests__/restore-all-matching.test.tsapps/ccaudit/src/__tests__/restore-command.test.tsapps/ccaudit/src/__tests__/restore-flag-mutual-exclusion.test.tsapps/ccaudit/src/__tests__/restore-interactive-memory-roundtrip.test.tsapps/ccaudit/src/__tests__/restore-interactive-roundtrip.test.tsapps/ccaudit/src/__tests__/restore-interactive-source-exists.test.tsapps/ccaudit/src/__tests__/restore-json-envelope.test.tsapps/ccaudit/src/__tests__/restore-name-ambiguity.test.tsapps/ccaudit/src/__tests__/restore-stale-filter.test.tsapps/ccaudit/src/__tests__/safety-invariants-framework.test.tsapps/ccaudit/src/__tests__/safety-invariants-mcp.test.tsapps/ccaudit/src/__tests__/safety-invariants-restore-roundtrip.test.tsapps/ccaudit/src/__tests__/safety-invariants-tui-abort.test.tsapps/ccaudit/src/__tests__/scanner-multi-config-mcp.test.tsapps/ccaudit/src/__tests__/tabbed-picker-50-item-fixture.test.tsapps/ccaudit/src/__tests__/tabbed-picker-bust-parity.test.tsapps/ccaudit/src/__tests__/tabbed-picker-filter.test.tsapps/ccaudit/src/__tests__/tabbed-picker-force-partial-banner.test.tsapps/ccaudit/src/__tests__/tabbed-picker-framework-group.test.tsapps/ccaudit/src/__tests__/tabbed-picker-help-overlay.test.tsapps/ccaudit/src/__tests__/tabbed-picker-live-counter.test.tsapps/ccaudit/src/__tests__/tabbed-picker-mcp-multi-config.test.tsapps/ccaudit/src/__tests__/tabbed-picker-overflow.test.tsapps/ccaudit/src/__tests__/tabbed-picker-protection.test.tsapps/ccaudit/src/__tests__/tabbed-picker-sort-cycle.test.tsapps/ccaudit/src/__tests__/tabbed-picker-tab-nav-keys.test.tsapps/ccaudit/src/__tests__/tabbed-picker-terminal-too-short.test.tsapps/ccaudit/src/__tests__/tmux-e2e-manual-qa.test.tsapps/ccaudit/src/cli/_shared-args.tsapps/ccaudit/src/cli/commands/ghost.tsapps/ccaudit/src/cli/commands/inventory.tsapps/ccaudit/src/cli/commands/mcp.tsapps/ccaudit/src/cli/commands/purge-archive.tsapps/ccaudit/src/cli/commands/restore.tsapps/ccaudit/src/cli/commands/trend.tsapps/ccaudit/src/cli/index.tsdocs/JSON-SCHEMA.mdpackage.jsonpackages/internal/src/cli/env.tspackages/internal/src/cli/index.tspackages/internal/src/index.tspackages/internal/src/remediation/__fixtures__/ghost-hash-golden.jsonpackages/internal/src/remediation/_archive-move.tspackages/internal/src/remediation/atomic-write.tspackages/internal/src/remediation/bust.tspackages/internal/src/remediation/change-plan.tspackages/internal/src/remediation/checkpoint.tspackages/internal/src/remediation/index.tspackages/internal/src/remediation/manifest.tspackages/internal/src/remediation/purge.tspackages/internal/src/remediation/reclaim.tspackages/internal/src/remediation/restore.tspackages/internal/src/remediation/savings.tspackages/internal/src/scanner/_config-refs.tspackages/internal/src/scanner/_present-path.tspackages/internal/src/scanner/annotate.tspackages/internal/src/scanner/index.tspackages/internal/src/scanner/scan-hooks.tspackages/internal/src/scanner/scan-mcp.tspackages/internal/src/scanner/types.tspackages/internal/src/token/format.tspackages/internal/src/token/index.tspackages/internal/src/types.tspackages/terminal/package.jsonpackages/terminal/src/index.tspackages/terminal/src/tables/change-plan.tspackages/terminal/src/tui/_filter-sort.tspackages/terminal/src/tui/_force-partial-banner.tspackages/terminal/src/tui/_glyph-capability.tspackages/terminal/src/tui/_glyphs.tspackages/terminal/src/tui/_help-overlay.tspackages/terminal/src/tui/_mcp-warning-render.tspackages/terminal/src/tui/_preflight-copy.tspackages/terminal/src/tui/_protection-render.tspackages/terminal/src/tui/_tab-bar.tspackages/terminal/src/tui/_tui-mode.tspackages/terminal/src/tui/_viewport.tspackages/terminal/src/tui/auto-open-prompt.tspackages/terminal/src/tui/confirmation.tspackages/terminal/src/tui/index.tspackages/terminal/src/tui/select-ghosts.tspackages/terminal/src/tui/select-restore.tspackages/terminal/src/tui/tabbed-picker.tspnpm-workspace.yaml
c534da3 to
d83833e
Compare
There was a problem hiding this comment.
Actionable comments posted: 11
♻️ Duplicate comments (1)
packages/internal/src/scanner/scan-mcp.ts (1)
353-415:⚠️ Potential issue | 🔴 CriticalWindows CI is still failing on these in-source tests.
The pipeline shows the assertion at Line 394 still red on
windows-latest, 22(refs[0]receivesC:/Users/.../scan-mcp-refs-*/claude.jsoninstead of.mcp.json). The integration counterpart inapps/ccaudit/src/__tests__/scanner-multi-config-mcp.test.tswas guarded withdescribe.skipIf(process.platform === 'win32'), but this in-sourcedescribeblock was not — so Windows CI keeps failing on the samepresentPathrendering gap.Pick one path forward:
- Preferred: fix
_present-path.tsto normalize Windows separators (resolve +path.relativeagainstprojPath/home, then convert backslashes to/) so.mcp.jsonand~/...rendering work cross-platform.- Tactical: gate this
describewithdescribe.skipIf(process.platform === 'win32')(...)to mirror the integration test until presentPath is fixed.🩹 Tactical skip-on-Windows (mirrors the integration test)
- describe('scanMcpServers — configRefs (Phase 6, D6-02)', () => { + describe.skipIf(process.platform === 'win32')('scanMcpServers — configRefs (Phase 6, D6-02)', () => { let tmpDir: string;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/internal/src/scanner/scan-mcp.ts` around lines 353 - 415, The tests fail on Windows because _present-path.ts doesn't normalize backslashes, so presentPath produces Windows-style absolute paths instead of project-relative strings like '.mcp.json'; update the presentPath implementation (the function presentPath in _present-path.ts) to resolve then path.relative against the projPath/home bucket and convert any backslashes to forward slashes (e.g., replace all '\\' with '/') before returning, ensuring project-local refs render as '.mcp.json' and ~/... rendering is consistent across platforms.
🧹 Nitpick comments (31)
packages/internal/src/scanner/scan-memory.ts (1)
88-90: Optional: add a Windows-path test for the new normalization.The change correctly normalizes Windows separators in the emitted
nameso manifest IDs/display strings are stable across platforms. The existing T40 tests, however, only exercise POSIX paths, so the newreplace(/\\/g, '/')is a no-op in CI. Consider adding a small unit test forscanImportChain(or a focused helper) that passes a Windows-styleprojRoot/imp.pathpair and asserts the resultingnamecontains forward slashes only — this guards against regressions if the normalization is later moved or removed.Note also that on POSIX,
\is technically a valid filename character; the unconditional backslash→slash replace would mangle such names. In practice this is a non-issue for Claude memory imports, so flagging only as a minor caveat.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/internal/src/scanner/scan-memory.ts` around lines 88 - 90, Add a unit test for scanImportChain (or the helper that computes relPath/name) that supplies a Windows-style projRoot and imp.path (e.g., containing backslashes) and asserts the produced name uses only forward slashes; specifically exercise the branch that computes relPath using path.relative and the replace(/\\/g, '/') normalization so the test fails if backslashes remain or the normalization is removed, and place the test next to other T40-style scanner tests referencing scanImportChain, projRoot, imp.path, relPath, and name.apps/ccaudit/scripts/bundle-size-check.mjs (1)
14-14: Drop the hardcoded15360Bfrom log/error strings.
BUDGET_BYTESis already a constant; mirror what the phase block does and interpolate it so the two stay in sync if the budget ever changes.♻️ Suggested change
-console.log(`[bundle-size] actual=${actual}B baseline=${baseline}B delta=${delta}B budget=15360B`); +console.log( + `[bundle-size] actual=${actual}B baseline=${baseline}B delta=${delta}B budget=${BUDGET_BYTES}B`, +); if (delta > BUDGET_BYTES) { - console.error(`[bundle-size] FAIL: delta exceeds 15 KB budget (${delta} > ${BUDGET_BYTES})`); + console.error(`[bundle-size] FAIL: delta exceeds budget (${delta} > ${BUDGET_BYTES})`); process.exit(1); }Also applies to: 42-42, 44-46
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ccaudit/scripts/bundle-size-check.mjs` at line 14, Replace the hardcoded "15360B" in log/error messages with the BUDGET_BYTES constant so they stay in sync; locate occurrences that currently embed "15360B" (the messages printed when comparing size to budget and the phase block logs) and interpolate or format them to use BUDGET_BYTES (and an appropriate unit string like "B") instead of the literal number, keeping the wording the same but deriving the value from the BUDGET_BYTES constant.apps/ccaudit/src/__tests__/fixtures/tmux-e2e.ts (2)
228-233: Consider adding a timeout to thetmuxwrapper.
execFilehere has notimeout, so a wedgedtmuxinvocation (e.g., a hung server, an unresponsive pane) will block until vitest's outer test timeout, producing a less actionable failure. SincewaitForTextalready does its own polling/timeout, individualtmuxcalls being long-running is unexpected and worth bounding defensively.♻️ Proposed addition
async function tmux(args: readonly string[]): Promise<TmuxRunResult> { const { stdout, stderr } = await execFile('tmux', [...args], { maxBuffer: 10 * 1024 * 1024, + timeout: 10_000, }); return { stdout, stderr }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ccaudit/src/__tests__/fixtures/tmux-e2e.ts` around lines 228 - 233, The tmux wrapper (function tmux) calls execFile without a timeout which can hang tests; update tmux to accept an optional timeout parameter (or use a sensible default like 5000–10000ms) and pass it into execFile options (e.g., { timeout, maxBuffer: ... }), so individual tmux invocations are bounded; ensure the function signature (tmux and TmuxRunResult usage sites) is updated accordingly and propagate/handle the timeout error so tests fail fast with a clear message.
142-146: Redundant doublestripAnsion the default path.When
opts.stripAnsiisundefined/true,capture()is called withansi: false, which already strips ANSI internally and returns clean text. The branch on Line 146 then runsstripAnsi(raw)again on already-stripped output. Functionally a no-op, but it does extra regex passes per poll iteration and obscures intent.♻️ Proposed simplification
- const raw = await this.capture({ - startLine: opts.startLine ?? -200, - ansi: opts.stripAnsi === false, - }); - lastCapture = opts.stripAnsi === false ? raw : stripAnsi(raw); + lastCapture = await this.capture({ + startLine: opts.startLine ?? -200, + ansi: opts.stripAnsi === false, + });
capture()already honors theansiflag, so the caller doesn't need to re-strip.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ccaudit/src/__tests__/fixtures/tmux-e2e.ts` around lines 142 - 146, The code double-strips ANSI: capture() already respects the ansi flag derived from opts.stripAnsi, so remove the redundant stripAnsi(raw) call and simply assign lastCapture = raw; update the block around the this.capture(...) call to stop re-running stripAnsi and rely on capture's ansi handling (refer to this.capture(...) and lastCapture / opts.stripAnsi).packages/internal/src/scanner/scan-hooks.ts (2)
534-555: Extend privacy assertion to thetool_nameandnamefallback paths.The
mcp_toolprivacy invariant is only asserted on thetoolfield path (line 497). The fallback branches (tool_name,name) flow through the sameshortHash, so there's no real risk today, but adding anoCommandLeakcheck here would lock the invariant against future refactors that might (e.g.) inadvertently copynameonto the emitted item'snamefield — note that the inventory item itself has anameproperty, so a regression whereleafObj['name']ends up assigned instead of hashed wouldn't be caught by the existing assertions.Proposed addition
const items = await extractHookItemsFromFile(settingsPath, 'global', null); expect(items).toHaveLength(2); + expect(noCommandLeak(items, 'mcp__a__x')).toBe(true); + expect(noCommandLeak(items, 'mcp__b__y')).toBe(true); for (const item of items) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/internal/src/scanner/scan-hooks.ts` around lines 534 - 555, The mcp_tool privacy invariant currently only asserts noCommandLeak on the 'tool' path; extend that assertion to the fallback branches that use 'tool_name' and 'name' so the same shortHash privacy guarantee is enforced there too. Locate the mcp_tool handling in scan-hooks.ts (the branch that computes shortHash and currently calls noCommandLeak for the 'tool' field) and add the same noCommandLeak check when the code selects tool_name or name as the identifier (the logic that produces item.name / emitted item's name); ensure you call noCommandLeak with the same parameters used for the 'tool' path so regressions that copy raw name into item.name are caught.
160-167: Address potential name collision when matcher contains:toolas a substring.Claude Code hook matchers can include colon characters when treated as regex patterns. A
commandleaf with matcher like"Bash:tool"would generatePreToolUse:Bash:tool:<hash>, identical to anmcp_toolleaf with matcher"Bash". While unlikely in practice, this could cause name collisions and deduplication conflicts.Defensively, either:
- Sanitize
:frommatcherPart, or- Reposition the discriminator to prevent mid-string collision (e.g.,
${event}:tool@${matcherPart}:${hash})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/internal/src/scanner/scan-hooks.ts` around lines 160 - 167, The constructed `name` can collide when matcher contains ":tool" (e.g., matcherPart "Bash:tool") because the command branch produces `${event}:${matcherPart}:${hash}` which can equal the mcp_tool branch; update the `name` generation in scan-hooks (symbols: matcherPart, leafType, name, event, hash) to avoid mid-string collisions by either stripping or encoding colons from matcherPart (e.g., replace ":" with "_" or percent-encode) OR move the discriminator out of the matcher segment (e.g., `${event}:tool@${matcherPart}:${hash}` for tool leaves) and apply the same change consistently to any consumers that parse or dedupe names so existing comparisons keep working.packages/terminal/src/tui/_glyphs.ts (1)
9-13: Minor doc/code mismatch onNO_COLORsemantics.Line 11 says
NO_COLORtriggers ASCII for "any value", but the implementation (correctly, per no-color.org) requires a non-empty string and the dedicated test at lines 109-114 enforces that. Worth tightening the comment so future readers don't "fix" the empty-string check.📝 Proposed doc tweak
- * 2. `NO_COLOR` env var set (any value; convention is truthy-presence) + * 2. `NO_COLOR` env var set to a non-empty value (per no-color.org)Also applies to: 72-77
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/terminal/src/tui/_glyphs.ts` around lines 9 - 13, The comment describing the ASCII fallback in packages/terminal/src/tui/_glyphs.ts is misleading about NO_COLOR semantics (it says "any value" but the code and tests require a non-empty string); update the comment text near the ASCII fallback triggers to state that NO_COLOR must be a non-empty string to trigger ASCII mode, and make the same wording change for the second occurrence that documents the same behavior; reference the NO_COLOR env check and the opts.noColor documentation so the comment aligns with the implementation and existing tests.packages/internal/src/remediation/change-plan.ts (1)
313-330: Minor: hard-coded canonical id strings in tests will silently rot ifcanonicalItemIdever changes shape.The literal templates
agent|global||/tmp/a1andmcp-server|global||m1|/tmp/m1reproduce the pipe-delimited internals ofcanonicalItemId. If that format gets a new field or separator, these tests break far from the change site and the failure message won't mention the canonical-id contract.Cheap defense: build the id via
canonicalItemId(...)directly so the tests pin behavior, not a string format.Sketch
- const idA1 = `agent|global||/tmp/a1`; - const idM1 = `mcp-server|global||m1|/tmp/m1`; + const idA1 = canonicalItemId({ category: 'agent', scope: 'global', projectPath: null, name: 'a1', path: '/tmp/a1' }); + const idM1 = canonicalItemId({ category: 'mcp-server', scope: 'global', projectPath: null, name: 'm1', path: '/tmp/m1' });(Same idea for the other tests in the block.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/internal/src/remediation/change-plan.ts` around lines 313 - 330, Replace the hard-coded pipe-delimited id strings (e.g., idA1 and idM1) with calls to the canonicalItemId helper so tests rely on the canonicalId builder rather than string format; locate the test block that uses makeResult and change the id construction for idA1, idM1 (and the other similar ids in this file) to call canonicalItemId with the same fields used when creating the makeResult item (category, scope, projectPath, name, path) so the test will follow any future shape changes to canonicalItemId.packages/terminal/src/tables/change-plan.ts (1)
75-89: Optional: thecommandsrow arrow doesn't column-align withagents/skills.
agents/skillsare 6 chars;commandsis 8. With the current two-space gap before→, the arrow column shifts right two positions on the commands row only. Cosmetic, but if you want the ARCHIVE block to look like an aligned table (which today's two rows already do), pad the noun to a fixed width.Proposed fix
if (plan.counts.agents > 0) { lines.push( - ` ${String(plan.counts.agents).padStart(3)} agents → ~/.claude/ccaudit/archived/agents/`, + ` ${String(plan.counts.agents).padStart(3)} ${'agents'.padEnd(8)} → ~/.claude/ccaudit/archived/agents/`, ); } if (plan.counts.skills > 0) { lines.push( - ` ${String(plan.counts.skills).padStart(3)} skills → ~/.claude/ccaudit/archived/skills/`, + ` ${String(plan.counts.skills).padStart(3)} ${'skills'.padEnd(8)} → ~/.claude/ccaudit/archived/skills/`, ); } if (plan.counts.commands > 0) { lines.push( - ` ${String(plan.counts.commands).padStart(3)} commands → ~/.claude/ccaudit/archived/commands/`, + ` ${String(plan.counts.commands).padStart(3)} ${'commands'.padEnd(8)} → ~/.claude/ccaudit/archived/commands/`, ); }Note: the existing test at line 382 uses
/1 commands\s+→/so it'd still match; line 320' 2 agents'would need either updating or relaxing if you take this.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/terminal/src/tables/change-plan.ts` around lines 75 - 89, The three lines that push agent/skill/command summary strings (the lines.push calls that read plan.counts.agents, plan.counts.skills and plan.counts.commands) produce misaligned arrows because the noun lengths differ; fix by formatting the noun field to a fixed width before the two-space gap (e.g., pad the noun to the same width for "agents"/"skills"/"commands" using String(...).padEnd or template construction) so the "→" column aligns across all rows; update the lines.push expressions for the agents/skills/commands rows (or introduce a small helper to format "N <noun>" with a constant noun column width) and run/adjust any tests that assert spacing if needed.apps/ccaudit/src/__tests__/restore-flag-mutual-exclusion.test.ts (1)
48-64: Optional: third mutual-exclusion combination is not covered.You assert
--interactive --nameand--name --all-matching, but not--interactive --all-matching. Cheap to add for completeness — ifvalidateRestoreFlagExclusionever evolves into per-pair logic instead of a single 3-way guard, the missing case will silently regress.Proposed addition
+ it('--interactive --all-matching foo → exit 1, "flags are mutually exclusive" on stderr', async () => { + const r = await runCcauditCli( + tmpHome, + ['restore', '--interactive', '--all-matching', 'foo'], + { env: { PATH: `${path.join(tmpHome, 'bin')}:${process.env.PATH ?? ''}` } }, + ); + expect(r.exitCode).toBe(1); + expect(r.stderr).toContain('flags are mutually exclusive'); + });As per coding guidelines: "Integration tests live under
apps/ccaudit/src/__tests__/" — the file location is correct.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ccaudit/src/__tests__/restore-flag-mutual-exclusion.test.ts` around lines 48 - 64, Add the missing mutual-exclusion test for the `restore` command covering the `--interactive` + `--all-matching` pair: add a new test in restore-flag-mutual-exclusion.test.ts that calls `runCcauditCli(tmpHome, ['restore','--interactive','--all-matching','foo'], ...)`, asserts exitCode is 1 and stderr contains "flags are mutually exclusive"; this completes coverage for the three pairwise combinations so future changes to `validateRestoreFlagExclusion` (or similar validation logic) won’t silently regress.apps/ccaudit/src/__tests__/restore-corrupt-manifest.test.ts (1)
86-90: Use definite-assignment-aware pattern instead ofparsed!.The
parsed!non-null after writing inside anexpect(...).not.toThrow()callback works but loses type narrowing for thetoHavePropertyline if anyone refactors the assertion away. Minor — assigning before the expect makes intent clearer.- let parsed: Record<string, unknown>; - expect(() => { - parsed = JSON.parse(stdout) as Record<string, unknown>; - }, `stdout must be valid JSON, got:\n${stdout}`).not.toThrow(); - // The envelope must have a meta block (standard ccaudit JSON envelope shape) - expect(parsed!).toHaveProperty('meta'); + let parsed: Record<string, unknown> = {}; + try { + parsed = JSON.parse(stdout) as Record<string, unknown>; + } catch (e) { + throw new Error(`stdout must be valid JSON, got:\n${stdout}\n${(e as Error).message}`); + } + expect(parsed).toHaveProperty('meta');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ccaudit/src/__tests__/restore-corrupt-manifest.test.ts` around lines 86 - 90, Assign the parsed value before asserting to avoid using the non-null assertion; create a parsing function (e.g., const parseFn = () => JSON.parse(stdout) as Record<string, unknown>) and use expect(parseFn).not.toThrow(), then set parsed = parseFn() and call expect(parsed).toHaveProperty('meta'). This keeps JSON.parse, parsed, stdout, and the expect(...).not.toThrow()/toHaveProperty assertions but removes the need for parsed! and preserves type narrowing if the test is refactored.packages/terminal/src/tui/_force-partial-banner.ts (1)
36-41: Banner suffix joins without space when needed — current code is correct, just locking it.
${BASE_TEXT}${suffix}works becauseZERO_PROTECTED_SUFFIXalready starts with a leading space. If anyone edits the constant later and drops that space, output runs together. Worth a tiny test to lock this:it('active + Unicode + protectedCount===0 → adds "(no protected items in this scan)" suffix (D6-14)', () => { const out = renderForcePartialBanner({ active: true, protectedCount: 0, ascii: false }); expect(out.startsWith('⚠ ')).toBe(true); expect(out.endsWith('(no protected items in this scan)')).toBe(true); + expect(out).toMatch(/\.\s+\(no protected/); // ensure space before '(' });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/terminal/src/tui/_force-partial-banner.ts` around lines 36 - 41, renderForcePartialBanner currently concatenates BASE_TEXT and ZERO_PROTECTED_SUFFIX relying on the suffix to include a leading space; change renderForcePartialBanner to always insert a single separating space when suffix is non-empty (e.g., compute suffixNormalized = suffix.trim() and return `${BASE_TEXT} ${suffixNormalized}` when suffixNormalized is truthy, otherwise just BASE_TEXT) so future edits to ZERO_PROTECTED_SUFFIX can't remove the space, and add a small unit test for renderForcePartialBanner that asserts the banner contains a space between BASE_TEXT and the suffix when protectedCount is zero (use the renderForcePartialBanner function and ZERO_PROTECTED_SUFFIX/BASE_TEXT constants to construct the test).apps/ccaudit/src/__tests__/_test-helpers.ts (2)
10-32: Doc references undefinedmaxWaitMs.The comment talks about "until maxWaitMs elapses" / "loop runs until maxWaitMs", but the function only takes
graceMsand uses an internalearlyExitMs = 1_000ceiling. Either rename the doc to mentionearlyExitMs, or exposemaxWaitMsas a parameter for consistency withwaitForMarker.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ccaudit/src/__tests__/_test-helpers.ts` around lines 10 - 32, The comment for waitForPicker mentions a non-existent maxWaitMs; update the code to make docs and signature consistent by adding an optional maxWaitMs parameter (or renaming references to earlyExitMs). Specifically, modify the waitForPicker function signature to accept maxWaitMs?: number (default e.g. 1000) instead of hardcoding earlyExitMs, replace the internal earlyExitMs usage with maxWaitMs, and update the JSDoc text to reference maxWaitMs (or, if you prefer the opposite approach, change the doc text to reference earlyExitMs and mention graceMs for clarity); mirror the same naming/behavior used by waitForMarker to keep consistency.
253-258: Deadvoid killed— drop or actually swallow the rejection path.
killedis set in the timeout branch but the promise has alreadyreject(...)-ed there, so by the timeclosefires the consumer no longer sees this resolve. Thevoid killedis a no-op marker. Either delete it, or guard the resolve so a post-rejectionclosedoesn't double-settle (Node's Promise will silently swallow that, but it's still confusing).Suggested cleanup
child.on('close', (code: number | null) => { clearTimeout(timer); - // Resolve in both killed and non-killed cases so callers always get output. - void killed; - resolve({ stdout, stderr, exitCode: code, durationMs: Date.now() - start }); + if (killed) return; // already rejected via timeout + resolve({ stdout, stderr, exitCode: code, durationMs: Date.now() - start }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ccaudit/src/__tests__/_test-helpers.ts` around lines 253 - 258, The no-op "void killed" in the child.on('close', ...) handler should be removed and the resolve should be guarded so we don't attempt to resolve after the promise was already rejected on timeout; update the handler around child.on('close', (code) => { ... resolve({ stdout, stderr, exitCode: code, durationMs: ... }) }) to check the existing killed flag (or use a new settled boolean) and only call resolve when not killed/not already settled (i.e., if (!killed) resolve(...)); keep clearTimeout(timer) as-is and ensure the timeout branch still calls reject(...) and sets killed/settled consistently.apps/ccaudit/src/__tests__/tabbed-picker-protection.test.ts (2)
76-80: Hoist the dynamicnode:fs/promisesimport.Dynamic
await import(...)for a Node built-in insidebeforeEachadds churn for no benefit; static import keeps tree-shake/lint behavior consistent with the other test files in the suite.+import { writeFile, utimes } from 'node:fs/promises'; ... - const fs = await import('node:fs/promises'); const sixtyDaysAgo = new Date(Date.now() - 60 * 86_400_000); const plainPath = path.join(tmpHome, '.claude', 'agents', 'plain-ghost.md'); - await fs.writeFile(plainPath, '# plain ghost\n', 'utf8'); - await fs.utimes(plainPath, sixtyDaysAgo, sixtyDaysAgo); + await writeFile(plainPath, '# plain ghost\n', 'utf8'); + await utimes(plainPath, sixtyDaysAgo, sixtyDaysAgo);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ccaudit/src/__tests__/tabbed-picker-protection.test.ts` around lines 76 - 80, Replace the dynamic import(await import('node:fs/promises')) inside the test setup with a top-level static import of the built-in module so tests behave consistently; locate the dynamic import usage in the test that assigns to fs (used with sixtyDaysAgo and plainPath) and change it to reference a statically imported fs from 'node:fs/promises' so the beforeEach no longer performs a runtime import.
87-106: Unicode assertion is vulnerable to flakiness due to piped stdio bypassing color detection.
shouldUseAscii()inpackages/terminal/src/tui/_glyph-capability.ts(line 48) gates onstdout.hasColors?.() === false, which returnstruefor piped streams regardless ofFORCE_COLOR=1. SincerunCcauditGhost()spawns withstdio: ['pipe', 'pipe', 'pipe'], the child'sprocess.stdout.hasColors()will return false, triggering ASCII fallback mode and replacing[🔒]with[L]. The test explicitly disablesNO_COLOR(seebaseEnv()comment) to avoid this, butFORCE_COLOR=1andCCAUDIT_FORCE_TTY=1alone cannot override the piped stdio detection. Consider mocking or controlling the stdout capability object in the test harness to guaranteehasColors()returns true, or accept the flake risk if the test runner's environment reliably inherits color support.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ccaudit/src/__tests__/tabbed-picker-protection.test.ts` around lines 87 - 106, The test is flaky because glyph selection (packages/terminal/src/tui/_glyph-capability.ts -> shouldUseAscii) uses stdout.hasColors() which returns false for piped stdio from runCcauditGhost(), causing Unicode "[🔒]" to become ASCII "[L]". Fix the test by forcing the child process to report color support or mocking the capability: either spawn with a pseudo-tty so stdout.hasColors() returns true (adjust runCcauditGhost() spawn options) or, inside the test, inject/mutate the child's stdout.hasColors to a function that returns true (or stub shouldUseAscii behavior) before the app renders; reference runCcauditGhost(), baseEnv(), and shouldUseAscii() when making the change so the test reliably sees Unicode glyphs.apps/ccaudit/src/__tests__/restore-interactive-roundtrip.test.ts (2)
63-110: InlinespawnRestoreInteractiveis fine for now, but consider hoisting.Three v1.5 restore-interactive tests already pattern-match this same boilerplate (spawn
restore --interactivewithCCAUDIT_FORCE_TTY=1, COLUMNS/LINES, NO_COLOR). When you add a 4th, promote this to_test-helpers.tsalongsiderunCcauditGhost(e.g.runCcauditCommand(tmpHome, 'restore', flags, opts)) so the env, PATH, and timeout policy stay identical across files. Not blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ccaudit/src/__tests__/restore-interactive-roundtrip.test.ts` around lines 63 - 110, The tests duplicate spawnRestoreInteractive boilerplate (env vars like CCAUDIT_FORCE_TTY, COLUMNS, LINES, NO_COLOR, PATH setup and timeout policy); hoist this helper into a shared test helper module (e.g., _test-helpers.ts) and replace local uses with a common function such as runCcauditCommand(tmpHome, 'restore', flags, opts) or reuse runCcauditGhost style helper; ensure the hoisted helper preserves the same behavior/signature as the current spawnRestoreInteractive (returns { child, done } and enforces the 15_000ms timeout, env keys, cwd and stdio) so all tests share identical environment and timeout semantics.
144-152: Confirm-prompt flip via ArrowLeft is brittle to clack default changes.The script depends on
select-restore.ts:120havinginitialValue: false. If that ever flips totrue, ArrowLeft will move No→Yes→ but the prompt is already on Yes and this becomes No, silently. Consider asserting on a stdout marker between steps (e.g., the prompt label) or pressingyif the prompt accepts shortcut keys, so the test fails loudly rather than restoring nothing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ccaudit/src/__tests__/restore-interactive-roundtrip.test.ts` around lines 144 - 152, The test in restore-interactive-roundtrip.test.ts relies on cursor movement (sendKeys(spawned.child, ['\x1b[D']) ) assuming select-restore.ts sets initialValue: false, which makes the ArrowLeft brittle; update the test to either assert a visible stdout marker for the confirmation prompt (check the prompt label/string after awaiting the grace timeout) before toggling, or send an explicit confirmation shortcut key (e.g., 'y' or 'n') instead of ArrowLeft so the intent is clear and the test fails loudly if the prompt state changes; reference the test's use of sendKeys(spawned.child, ...) and the select-restore.ts initialValue behavior when making the change.apps/ccaudit/src/__tests__/tabbed-picker-help-overlay.test.ts (1)
92-103: Optional: replace fixedsetTimeout(500)withwaitForMarkerfor symmetry with the second test.Test 2 (lines 162–167) waits on
(Press ? or Esc to close)to confirm the overlay is rendered before asserting; test 1 here uses a fixed 500 ms wait. If a slow CI runner takes longer than 500 ms to render the overlay, theexpect(afterOpen).toContain('Selection')failure mode will be misleading (a diff against partial transcript) rather than a clear timeout. UsingwaitForMarkerhere keeps both tests symmetric and gives loud failures when the overlay never opens.♻️ Suggested fix
- await sendKeys(spawned.child, ['?']); - await new Promise((r) => setTimeout(r, 500)); - const afterOpen = stripAnsi(stdoutBuf); + await sendKeys(spawned.child, ['?']); + await waitForMarker( + () => stripAnsi(stdoutBuf), + () => spawned.child.exitCode !== null, + '(Press ? or Esc to close)', + ); + const afterOpen = stripAnsi(stdoutBuf);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ccaudit/src/__tests__/tabbed-picker-help-overlay.test.ts` around lines 92 - 103, Replace the fixed await new Promise((r) => setTimeout(r, 500)) pause with the same waitForMarker usage used in the second test: after sendKeys(spawned.child, ['?']) call await waitForMarker(stdoutBuf, '(Press ? or Esc to close)') so the test waits deterministically for the overlay to render before calling stripAnsi(stdoutBuf) and asserting; update the test to import/ensure waitForMarker is available and remove the hardcoded timeout to make it symmetric with the second test.apps/ccaudit/src/__tests__/safety-invariants-restore-roundtrip.test.ts (1)
165-170: Nit: assertion message reports unfiltered count.
manifests.lengthin the assertion message is the rawreaddirlength (which can include non-.jsonlartifacts), but the assertion itself runs against the filtered list. If the filter ever drops something, the failure message will be confusing.♻️ Suggested fix
- const manifests = await readdir(manifestsDir); - expect( - manifests.filter((m) => m.endsWith('.jsonl')), - `expected 2 manifests, got ${manifests.length}: ${manifests.join(', ')}`, - ).toHaveLength(2); + const allEntries = await readdir(manifestsDir); + const jsonlManifests = allEntries.filter((m) => m.endsWith('.jsonl')); + expect( + jsonlManifests, + `expected 2 jsonl manifests, got ${jsonlManifests.length}: ${jsonlManifests.join(', ')} (all entries: ${allEntries.join(', ')})`, + ).toHaveLength(2);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ccaudit/src/__tests__/safety-invariants-restore-roundtrip.test.ts` around lines 165 - 170, The assertion message uses the unfiltered manifests.length which can include non-`.jsonl` files; instead compute and use the filtered list (e.g., const jsonlManifests = manifests.filter(m => m.endsWith('.jsonl'))) and reference jsonlManifests.length and jsonlManifests.join(', ') in the failure message and in the expect call (replace the inline filter with the precomputed jsonlManifests) so the message matches the actual value being asserted; update uses of manifests.filter(...) in this test block accordingly (variables: manifestsDir, manifests, the filter expression).apps/ccaudit/src/__tests__/fixtures/manual-qa-followups.ts (1)
391-404: Optional: parallelize file writes instagePaginationFixture.For
count=550, the sequentialawait writeFile+await utimesissues ~1100 serialized syscalls per fixture build. Setting up a 500-item picker test takes longer than necessary, which compounds across runs.Promise.allover anArray.from({ length: count })keeps ordering immaterial here (filename suffix encodes order) and shaves a meaningful chunk off CI time on filesystems that support concurrent metadata ops.♻️ Suggested fix
await mkdir(path.join(tmpHome, '.claude', 'agents'), { recursive: true }); - for (let i = 1; i <= count; i++) { - const name = `agent-${String(i).padStart(3, '0')}`; - const filePath = path.join(tmpHome, '.claude', 'agents', `${name}.md`); - await writeFile(filePath, `# ${name} ghost\n`, 'utf8'); - await utimes(filePath, OLD_DATE, OLD_DATE); - } + await Promise.all( + Array.from({ length: count }, async (_v, idx) => { + const name = `agent-${String(idx + 1).padStart(3, '0')}`; + const filePath = path.join(tmpHome, '.claude', 'agents', `${name}.md`); + await writeFile(filePath, `# ${name} ghost\n`, 'utf8'); + await utimes(filePath, OLD_DATE, OLD_DATE); + }), + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ccaudit/src/__tests__/fixtures/manual-qa-followups.ts` around lines 391 - 404, stagePaginationFixture currently performs writeFile and utimes sequentially for each agent, causing many serialized syscalls; change it to build an array of tasks (e.g., using Array.from or a loop) that for each index computes name and filePath and returns a Promise that writes the file and sets utimes, then await Promise.all on that array so writeFile and utimes run concurrently; keep the existing filename generation (agent-${String(i).padStart(3,'0')}) and ensure writeRecentSession and the mkdir/writeFile for .claude.json remain awaited after the parallel file ops so test setup order remains correct.apps/ccaudit/src/__tests__/tabbed-picker-tab-nav-keys.test.ts (1)
129-161: Inline marker polling silently times out — use sharedwaitForMarkerinstead.Both polling blocks (lines 129–141 for the picker render, lines 149–161 for the confirmation prompt) fall through when the marker never appears. If the picker never renders or the confirmation prompt never shows, the test continues sending keys against a dead/blocked subprocess and downstream assertions surface a misleading "expected exactly 1 manifest, got 0" error rather than a clear "timed out waiting for
Proceed with archive?" message.The same helper module already exports
waitForMarker(used bytabbed-picker-help-overlay.test.ts) which throws on timeout — using it here keeps the failure mode loud and consistent across the suite.♻️ Suggested fix
import { makeTmpHome, cleanupTmpHome, buildFakePs, runCcauditGhost, listManifestsDir, sendKeys, + waitForMarker, } from './_test-helpers.ts'; @@ - // Wait for the first frame: `AGENTS (0/2)` is emitted by the picker's - // per-tab header and only appears once the picker has rendered. - { - const maxWaitMs = 5_000; - const startMs = Date.now(); - let delayMs = 100; - while ( - spawned.child.exitCode === null && - !stdoutBuf.includes('AGENTS (0/2)') && - Date.now() - startMs < maxWaitMs - ) { - await new Promise((r) => setTimeout(r, delayMs)); - delayMs = Math.min(delayMs * 2, 500); - } - } + await waitForMarker( + () => stdoutBuf, + () => spawned.child.exitCode !== null, + 'AGENTS (0/2)', + ); @@ - { - const maxWaitMs = 5_000; - const startMs = Date.now(); - let delayMs = 100; - while ( - spawned.child.exitCode === null && - !stdoutBuf.includes('Proceed with archive?') && - Date.now() - startMs < maxWaitMs - ) { - await new Promise((r) => setTimeout(r, delayMs)); - delayMs = Math.min(delayMs * 2, 500); - } - } + await waitForMarker( + () => stdoutBuf, + () => spawned.child.exitCode !== null, + 'Proceed with archive?', + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ccaudit/src/__tests__/tabbed-picker-tab-nav-keys.test.ts` around lines 129 - 161, Replace the two manual polling loops that poll stdoutBuf for 'AGENTS (0/2)' and 'Proceed with archive?' with the shared test helper waitForMarker to ensure timeouts throw; specifically, remove the while blocks that reference spawned.child, stdoutBuf, startMs, delayMs and instead call waitForMarker(spawned, 'AGENTS (0/2)', {timeout: 5000}) before sending keys and waitForMarker(spawned, 'Proceed with archive?', {timeout: 5000}) after sendKeys (or adjust arguments to the existing waitForMarker signature), keeping references to spawned and stdoutBuf only as needed by waitForMarker so failures surface a clear timeout error.README.md (1)
454-459: Optional: clarifypurge-archive --jsondefaults to dry-run.The example block lists
purge-archive --jsonwith the comment "structured envelope, combinable with--yes", but a reader scanning the block sees--jsonwithout--yesand may not realize it inherits the safe-default dry-run behavior. A one-word clarification keeps the contract obvious for someone who pastes the line into CI.📝 Suggested wording
-ccaudit purge-archive --json # structured envelope, combinable with --yes +ccaudit purge-archive --json # dry-run + structured envelope (add --yes to execute)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 454 - 459, Update the README example to clarify that the "ccaudit purge-archive --json" invocation defaults to dry-run by explicitly noting it (e.g., "structured envelope, dry-run by default; combinable with --yes"); target the line showing "ccaudit purge-archive --json" and add the brief parenthetical or phrase that it inherits the safe default (--dry-run) so readers know they must add "--yes" to execute changes.packages/terminal/src/tui/_help-overlay.ts (1)
113-117: Truncation can split surrogate pairs.
truncate()usesString.prototype.sliceon UTF-16 code units. The Unicode glyph legend includes🔒(U+1F512), which is a surrogate pair (length 2 in JS). For narrowcolsvalues where the cut falls between the high and low surrogate of🔒(or any future astral character added to bindings), the returned line will contain a lone surrogate and render as a replacement character. The in-source tests only assertline.length <= cols, not validity, so they don't catch this.This is low-risk in practice (the lock glyph sits at column ~3, well inside typical viewports and the compact-mode test runs at
cols=15where the lock line happens not to be cut mid-pair), but a defensive splitter avoids future regressions when bindings change.♻️ Suggested fix
function truncate(line: string, cols: number): string { if (cols <= 0) return ''; - if (line.length <= cols) return line; - return line.slice(0, cols); + if (line.length <= cols) return line; + let cut = line.slice(0, cols); + // Avoid leaving a lone high surrogate at the cut boundary. + const lastCode = cut.charCodeAt(cut.length - 1); + if (lastCode >= 0xd800 && lastCode <= 0xdbff) { + cut = cut.slice(0, -1); + } + return cut; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/terminal/src/tui/_help-overlay.ts` around lines 113 - 117, The truncate function can split UTF-16 surrogate pairs because it slices by code units; update truncate to operate on Unicode code points instead of raw string indices: when cols > 0, convert the string into an array of characters using a codepoint-aware method (e.g., Array.from or the spread operator) and return the first cols code points joined back into a string, preserving the existing early-return behavior for cols <= 0 and the check for short lines; keep the function name truncate and ensure the return type remains string.apps/ccaudit/src/__tests__/pagination-500.test.ts (1)
63-88: Manually-mirrored handler logic can drift from the implementation.This test mutates
picker.filterMode,tab.cursor, andtab.savedCursorPreFilterdirectly, then re-implements the Esc handler's restore-and-clamp logic inline (lines 80–85). If the real?/ Esc handler ever changes its restore semantics (clamp policy, savedCursorPreFilter semantics, or the call intoapplyScroll), this test will keep passing while the actual picker regresses.The test header comment is honest about the trade-off, but consider invoking the picker's filter-on/off methods directly so the assertion exercises shipped code rather than a hand-rolled mirror. The next test (
applyScroll round-trips) already covers the pure reducer; this one would be more valuable hitting the integration boundary.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ccaudit/src/__tests__/pagination-500.test.ts` around lines 63 - 88, The test currently mutates picker.filterMode, tab.cursor, and tab.savedCursorPreFilter and re-implements the Esc restore/clamp logic; replace those direct mutations with calls into the picker's real filter-on/off handlers so the test exercises shipped code (i.e., invoke the picker method that activates filter mode and the method/handler that exits filter mode / simulates Esc so savedCursorPreFilter, applyScroll and the clamp semantics run in production code rather than being mirrored in the test). Locate references to filterMode, savedCursorPreFilter, tab.cursor and applyScroll in the picker implementation and call the corresponding enter-filter and exit-filter/Esc handler functions instead of manipulating fields directly.packages/terminal/src/tui/_filter-sort.ts (1)
118-125: Optional: future-datedmtimeMsranks as least-stale.When
item.mtimeMs > now(e.g. clock skew, or files touched on a system with a fast clock),bAge - aAgebecomes negative and the item floats to the bottom ofstaleness-descrather than being clamped to "most stale" or "freshest". Probably never visible in practice, but easy to defend if you want strictly monotonic staleness:♻️ Optional clamp
case 'staleness-desc': copy.sort((a, b) => { - const aAge = now - (a.item.mtimeMs ?? 0); - const bAge = now - (b.item.mtimeMs ?? 0); + const aAge = Math.max(0, now - (a.item.mtimeMs ?? 0)); + const bAge = Math.max(0, now - (b.item.mtimeMs ?? 0)); return bAge - aAge; }); return copy;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/terminal/src/tui/_filter-sort.ts` around lines 118 - 125, The staleness calculation in the 'staleness-desc' branch can produce negative ages for future-dated mtimeMs (variables aAge, bAge inside the case in file _filter-sort.ts), causing those items to sort to the bottom; clamp ages to a non-negative value before subtracting (e.g., compute age = Math.max(0, now - (item.mtimeMs ?? 0))) so future timestamps are treated as zero age and sort correctly, then use those clamped aAge/bAge values in the copy.sort comparator.apps/ccaudit/src/__tests__/tabbed-picker-filter.test.ts (1)
161-169: Optional: replace fixed-durationsetTimeoutsleeps withwaitForMarkerwhere possible.The interactive flow already uses
waitForMarkerfor known render landmarks, but several state transitions still rely on fixed sleeps (200/300/400/500 ms) before slicingstdoutBuf. Under loaded CI runners these can flake or, worse, pass for the wrong reason if the render lands a hair late. Where there's a deterministic post-keystroke marker (e.g.0 of 6 selected across all tabsafter Esc-clear, orFiltered: 2 of 3 visible | 1 selectedafter Space), preferwaitForMarker(...); keep the bare sleep only when asserting the absence of an event (e.g. "Enter must NOT submit").Also applies to: 217-228, 262-279, 329-338
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ccaudit/src/__tests__/tabbed-picker-filter.test.ts` around lines 161 - 169, The test uses fixed-duration sleeps (e.g., new Promise(r => setTimeout(r, 400))) after sendKeys to wait for renders, which flakes; replace those sleeps with calls to the existing waitForMarker helper to wait for deterministic output landmarks (for example wait for the "0 of 6 selected across all tabs" marker before slicing stdoutBuf) in the test case around sendKeys/spawned.child and stripAnsi usage (and similarly update the other occurrences noted at lines ~217-228, ~262-279, ~329-338) so the test slices stdoutBuf only after waitForMarker resolves; keep a short sleep only where you are explicitly asserting the absence of a marker.apps/ccaudit/src/__tests__/restore-list-skips-purge.test.ts (1)
71-71: Use a real ISO 8601 timestamp forcheckpoint_timestampin the fixture.
'2026-04-20T10-00-00-000Z'swaps the time-component colons for dashes — that's the filename-safe slug, not a valid ISO 8601 string, and it diverges from what real bust manifests write (they usenew Date().toISOString()). If a future schema tightening starts parsing this field, the fixture will silently fall through; better to mirror production output now.🔧 Suggested fix
- checkpoint_timestamp: '2026-04-20T10-00-00-000Z', + checkpoint_timestamp: bustMtime.toISOString(),As per coding guidelines: "Use ISO 8601 UTC strings (
new Date().toISOString()) for time values".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ccaudit/src/__tests__/restore-list-skips-purge.test.ts` at line 71, The fixture uses a filename-safe timestamp string for checkpoint_timestamp ('2026-04-20T10-00-00-000Z') which is not valid ISO 8601; update the fixture so checkpoint_timestamp is a real ISO 8601 UTC string (e.g., '2026-04-20T10:00:00.000Z') or generate it via new Date(...).toISOString() so the test mirrors production manifest output.apps/ccaudit/src/__tests__/restore-json-envelope.test.ts (1)
1-14: Docblock paths drift from the actual assertions.The header reads
top-level command === 'restore'anddata.status === 'success', but the test assertsparsed.meta.command,parsed.meta.exitCode, andparsed.status(nodatawrapper). Future readers will trust the comment; please align it with the envelope shape now in use.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ccaudit/src/__tests__/restore-json-envelope.test.ts` around lines 1 - 14, Update the docblock to match the actual envelope fields asserted in the test: mention that the command and exit code live under parsed.meta (parsed.meta.command and parsed.meta.exitCode), that parsed.status (not data.status) should equal 'success', that parsed.selection_filter (parsed.selection_filter.mode === 'subset' and parsed.selection_filter.ids has exactly 2 entries) is used, and that parsed.skipped is present and is an array. Ensure the header text references parsed.meta.command, parsed.meta.exitCode, parsed.status, parsed.selection_filter, and parsed.skipped so it matches the assertions in the test.packages/internal/src/remediation/manifest.ts (2)
238-247: Update JSDoc and stale test names to reflect purge-*.jsonl inclusion.The function-level JSDoc still says "Discover all bust-*.jsonl manifest files" and "Filters to only files matching the
bust-*.jsonlpattern (T-09-01)", but the implementation on lines 259‑261 now also returnspurge-*.jsonl. The existing test descriptions on line 1047 ('returns only bust-*.jsonl entries…') and line 1093 ('filters entries not matching bust-*.jsonl pattern') are similarly stale — they still pass because their inputs only contain bust entries, but they no longer describe the actual contract (which the new test on lines 1239‑1260 does cover correctly).📝 Suggested doc tweak
/** - * Discover all bust-*.jsonl manifest files in the manifests directory, - * sorted newest-first by mtime. + * Discover all bust-*.jsonl and purge-*.jsonl manifest files in the + * manifests directory, sorted newest-first by mtime. * * Returns [] when the directory doesn't exist (no bust history). - * Filters to only files matching the `bust-*.jsonl` pattern (T-09-01). + * Filters to files matching `bust-*.jsonl` or `purge-*.jsonl` (T-09-01, + * Phase 9 SC6 — purge follow-ups must be visible to restore dedup). * * `@param` deps Injectable readdir + stat (inject fakes in tests; production * passes `readdir: (d) => fs.readdir(d), stat: (p) => fs.stat(p)`). */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/internal/src/remediation/manifest.ts` around lines 238 - 247, Update the function-level JSDoc that currently says "Discover all bust-*.jsonl manifest files" and "Filters to only files matching the `bust-*.jsonl` pattern" to reflect that the implementation returns both bust-*.jsonl and purge-*.jsonl manifests (mention both patterns and that results are sorted newest-first by mtime); also update the stale test description strings ('returns only bust-*.jsonl entries…' and 'filters entries not matching bust-*.jsonl pattern') to mention purge-*.jsonl so the documentation and test names match the behavior of the manifest discovery function.
200-205: Use crypto for the disambiguator suffix to remove a rare-but-real truncation case.
Math.random().toString(36).slice(2, 6)is not guaranteed to yield 4 characters. For "round" values whose base-36 representation is short, the slice can return fewer chars (e.g.(0.5).toString(36) === '0.i'→'i', and(0).toString(36) === '0'→''). Probability is ~46655/2^52 per call — vanishingly small in practice, but it would produce filenames likebust-…-.jsonlthat fail the test regex[a-z0-9]{4}on line 671 and weaken the same-millisecond collision-avoidance contract documented on lines 187‑188.Since
node:cryptois already imported (createHash,randomUUID), prefer a deterministic fixed-width source. The same applies toresolvePurgeManifestPath(line 449).♻️ Proposed change
-import { createHash, randomUUID } from 'node:crypto'; +import { createHash, randomUUID, randomBytes } from 'node:crypto'; @@ export function resolveManifestPath(now: Date = new Date()): string { // Millisecond-precision timestamp: keep ms digits, replace `:` and `.` with `-` const stamp = now.toISOString().replace(/:/g, '-').replace(/\./g, '-'); - const rand = Math.random().toString(36).slice(2, 6); + // 2 random bytes → 4 hex chars (16^4 = 65,536), guaranteed fixed width. + const rand = randomBytes(2).toString('hex'); return path.join(homedir(), '.claude', 'ccaudit', 'manifests', `bust-${stamp}-${rand}.jsonl`); } @@ export function resolvePurgeManifestPath(now: Date = new Date()): string { const stamp = now.toISOString().replace(/:/g, '-').replace(/\./g, '-'); - const rand = Math.random().toString(36).slice(2, 6); + const rand = randomBytes(2).toString('hex'); return path.join(homedir(), '.claude', 'ccaudit', 'manifests', `purge-${stamp}-${rand}.jsonl`); }If you keep this change, also relax the test regexes at lines 671 and 1191 to
[a-f0-9]{4}(or[a-z0-9]{4}works as a strict superset).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/internal/src/remediation/manifest.ts` around lines 200 - 205, The disambiguator uses Math.random().toString(36).slice(2,6) which can produce fewer than 4 chars; replace it with a fixed-width crypto-derived hex string (e.g., use node:crypto to derive 2 bytes and hex-encode to 4 chars) in resolveManifestPath and the analogous resolvePurgeManifestPath so the suffix is always 4 hex chars; update the implementation to call the crypto routine (or createHash(randomUUID()) truncated) to generate a deterministic 4-character [a-f0-9]{4} disambiguator and keep the existing stamp and path construction unchanged.
61cdc6c to
95fe0c1
Compare
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (21)
eslint.config.ts (1)
10-15: Consider including scripts in a tsconfig instead of bumping the default-project cap.The option name itself (
..._THIS_WILL_SLOW_DOWN_LINTING) is a deliberate signal from typescript-eslint that this path is a performance escape hatch. Comment notes "currently 5+" but you're raising the cap to 20, which both slows linting and just defers the problem. A more durable fix is to add the.mjsscript directories to a dedicatedtsconfig.scripts.json(or include them in an existing tsconfig) referenced viaprojectService/project, and drop bothallowDefaultProjectand themaximumDefaultProjectFileMatchCount_*knob.Non-blocking — the current change is fine as a stopgap.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@eslint.config.ts` around lines 10 - 15, The current change raises projectService.maximumDefaultProjectFileMatchCount_THIS_WILL_SLOW_DOWN_LINTING to include more .mjs files; instead add those script paths to a proper tsconfig and point ESLint at it: create or update a tsconfig (e.g. tsconfig.scripts.json) that includes apps/ccaudit/scripts/*.mjs and scripts/*.mjs, then modify the ESLint config to reference that project via projectService.project (or project) and remove the allowDefaultProject and maximumDefaultProjectFileMatchCount_THIS_WILL_SLOW_DOWN_LINTING entries; update any comments to reflect the new tsconfig usage.apps/ccaudit/src/__tests__/tabbed-picker-tab-nav-keys.test.ts (2)
213-240: Optional: parameterize the two near-identical cases withit.each.The Tab and ArrowRight tests are byte-for-byte identical apart from the key sequence and the title. Collapsing them into an
it.eachtable would remove duplication and make it trivially extensible (e.g., for the symmetric Shift-Tab / ArrowLeft pair mentioned in the file header).♻️ Proposed refactor
- it("Tab key ('\\t') cycles tabs forward and produces a manifest with exactly 2 planned ops", async () => { - const { exitCode, manifest, stderr } = await runPickerWithTabKey(tmpHome, '\t'); - - expect(exitCode, `stderr:\n${stderr.slice(-500)}`).toBe(0); - expect(manifest.header).not.toBeNull(); - expect(manifest.header?.planned_ops.archive).toBe(2); - // Defensive: the actual op records should also number 2. - expect(manifest.ops.length).toBe(2); - // Narrow to archive ops (only ArchiveOp + SkippedOp have `category`); - // both planned ops are archives in this fixture. - const archiveOps = manifest.ops.filter((o) => o.op_type === 'archive'); - expect(archiveOps.length).toBe(2); - const cats = archiveOps.map((o) => o.category).sort(); - expect(cats).toEqual(['agent', 'skill']); - }, 25_000); - - it("ArrowRight key ('\\x1b[C') cycles tabs forward and produces a manifest with exactly 2 planned ops", async () => { - const { exitCode, manifest, stderr } = await runPickerWithTabKey(tmpHome, '\x1b[C'); - - expect(exitCode, `stderr:\n${stderr.slice(-500)}`).toBe(0); - expect(manifest.header).not.toBeNull(); - expect(manifest.header?.planned_ops.archive).toBe(2); - expect(manifest.ops.length).toBe(2); - const archiveOps = manifest.ops.filter((o) => o.op_type === 'archive'); - expect(archiveOps.length).toBe(2); - const cats = archiveOps.map((o) => o.category).sort(); - expect(cats).toEqual(['agent', 'skill']); - }, 25_000); + it.each([ + { label: "Tab ('\\t')", key: '\t' }, + { label: "ArrowRight ('\\x1b[C')", key: '\x1b[C' }, + ])( + '$label cycles tabs forward and produces a manifest with exactly 2 planned ops', + async ({ key }) => { + const { exitCode, manifest, stderr } = await runPickerWithTabKey(tmpHome, key); + + expect(exitCode, `stderr:\n${stderr.slice(-500)}`).toBe(0); + expect(manifest.header).not.toBeNull(); + expect(manifest.header?.planned_ops.archive).toBe(2); + expect(manifest.ops.length).toBe(2); + const archiveOps = manifest.ops.filter((o) => o.op_type === 'archive'); + expect(archiveOps.length).toBe(2); + const cats = archiveOps.map((o) => o.category).sort(); + expect(cats).toEqual(['agent', 'skill']); + }, + 25_000, + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ccaudit/src/__tests__/tabbed-picker-tab-nav-keys.test.ts` around lines 213 - 240, Two tests ("Tab key ('\\t') cycles tabs forward..." and "ArrowRight key ('\\x1b[C') cycles tabs forward...") are duplicated; replace them with a parameterized it.each that iterates over cases with a descriptive title and the key sequence and runs the same assertions. Locate the tests that call runPickerWithTabKey(tmpHome, <key>) and consolidate into a single it.each table (e.g., rows for ['Tab key (\\t)', '\t'] and ['ArrowRight key (\\x1b[C)', '\x1b[C']) and keep the existing assertions against exitCode, manifest.header?.planned_ops.archive, manifest.ops.length, and archiveOps categories unchanged. Ensure the test timeout (25_000) is preserved and the test body still destructures { exitCode, manifest, stderr } from runPickerWithTabKey.
129-161: Optional: extract the duplicated stdout-poll into a small helper.The two
whileloops on Lines 129–141 and 149–161 are structurally identical (exponential-backoff poll for a stdout substring while subprocess is alive, capped at 5 s). Extracting awaitForStdout(spawned.child, () => stdoutBuf, marker, maxWaitMs)helper — or better, promoting it to_test-helpers.ts— would simplify this file and benefit other TUI integration tests that follow the same pattern.Not blocking; current code is correct.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ccaudit/src/__tests__/tabbed-picker-tab-nav-keys.test.ts` around lines 129 - 161, Extract the duplicated exponential-backoff stdout polling into a reusable helper (e.g., waitForStdout) and replace both while loops with calls to it: implement waitForStdout(spawned.child, () => stdoutBuf, marker, maxWaitMs) that polls until the marker substring appears or the child exits or timeout elapses; put the helper in a shared test helper module (e.g., _test-helpers.ts) and update the two call sites in tabbed-picker-tab-nav-keys.test.ts to call waitForStdout(spawned.child, () => stdoutBuf, 'AGENTS (0/2)', 5000) and waitForStdout(spawned.child, () => stdoutBuf, 'Proceed with archive?', 5000) respectively.apps/ccaudit/scripts/bundle-smoke-test.mjs (1)
4-9: Optional: use the ESM-idiomatic URL resolution.The
resolve(__filename, '../../dist/index.js')trick works (the trailing filename is consumed by the first..), but it’s easy to misread. The standard ESM form is one line and avoidsfileURLToPathround-tripping just to feedpathToFileURLlater.♻️ Proposed refactor
-import { fileURLToPath, pathToFileURL } from 'node:url'; -import { resolve } from 'node:path'; +import { fileURLToPath } from 'node:url'; import { existsSync } from 'node:fs'; -const __filename = fileURLToPath(import.meta.url); -const distPath = resolve(__filename, '../../dist/index.js'); +const distUrl = new URL('../dist/index.js', import.meta.url); +const distPath = fileURLToPath(distUrl); @@ - await import(pathToFileURL(distPath).href); + await import(distUrl.href);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ccaudit/scripts/bundle-smoke-test.mjs` around lines 4 - 9, Replace the round-trip path resolution using fileURLToPath/__filename and resolve with the ESM URL form: create the dist file URL via new URL('../../dist/index.js', import.meta.url) (remove the __filename variable and the fileURLToPath import) and use that URL (or convert to a filesystem path only where needed, e.g. with fileURLToPath(new URL(...)) at the point of calling existsSync). Update references to distPath to use the new URL variable (or its file path) and remove the unused imports (fileURLToPath and resolve) so the script uses canonical ESM URL resolution.apps/ccaudit/src/__tests__/fixtures/tmux-e2e.ts (2)
176-178: Generated runner script is never cleaned up.
runnerPathis written intoopts.tmpDirand chmod'd, but nothing inTmuxE2ESessionremoves it onkill()or on session exit. In practice callers usually nuketmpDirwholesale, so this is non-blocking — but if a test suite re-usestmpDiracross cases or skips teardown on early failures, stale*.tmux-runner.shfiles accumulate (and could shadow each other ifopts.namecollides). Consider unlinking the script inkill()or returning the path so callers can manage it explicitly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ccaudit/src/__tests__/fixtures/tmux-e2e.ts` around lines 176 - 178, The generated runner script (runnerPath) written by buildRunnerScript is not removed and should be cleaned up: store the runnerPath on the TmuxE2ESession instance when you write it, then unlink it in TmuxE2ESession.kill() (and also on normal session exit) so the file is removed even on early failures; alternatively modify the API that creates the script to return the script path to callers so they can manage deletion. Ensure you reference the runnerPath property in the cleanup logic and handle ENOENT errors safely when unlinking.
142-146: Redundant ANSI strip inwaitForText.
capture()already strips ANSI when called withansi: false(line 116). With the defaultstripAnsi !== false,rawis already stripped, andlastCapture = stripAnsi(raw)strips it a second time. Idempotent and harmless, but it’s extra regex work on every poll iteration and the double-conditional logic obscures what the line is doing. You can just trustcapture():♻️ Proposed simplification
const raw = await this.capture({ startLine: opts.startLine ?? -200, ansi: opts.stripAnsi === false, }); - lastCapture = opts.stripAnsi === false ? raw : stripAnsi(raw); + lastCapture = raw;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ccaudit/src/__tests__/fixtures/tmux-e2e.ts` around lines 142 - 146, The code in waitForText redundantly calls stripAnsi on raw even though capture(...) already respects opts.stripAnsi via its ansi flag; replace the conditional assignment "lastCapture = opts.stripAnsi === false ? raw : stripAnsi(raw)" with a single unconditional "lastCapture = raw" (leave the capture(...) call and its ansi: opts.stripAnsi === false argument intact) so lastCapture trusts capture() to include/strip ANSI and removes the extra regex work; update references to lastCapture accordingly.packages/terminal/src/tables/change-plan.ts (1)
73-91: Optional: column alignment regression for the newcommandsrow.
agents/skillsare 6-char labels followed by→, so all arrows align.commandsis 8 chars with the same trailing two spaces, which shifts its→two columns right relative to the agents/skills rows. Pure cosmetic — feel free to ignore — but if you want consistent arrows, pad the labels to a common width (e.g. 8) so→lines up across all three rows.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/terminal/src/tables/change-plan.ts` around lines 73 - 91, The three archive rows in the change-plan display (the lines.push template strings that use plan.counts.agents / plan.counts.skills / plan.counts.commands) produce misaligned arrows because "commands" is longer; normalize the label column width by padding the label text to a common width (e.g., use label.padEnd(8) or format the string so "agents", "skills", and "commands" occupy the same character width) in each of the three template strings so the "→ ~/.claude/ccaudit/archived/..." arrows align vertically.apps/ccaudit/src/__tests__/restore-list-skips-purge.test.ts (1)
110-130: Purge-manifest header here is sparser than inrestore-after-purge.test.ts— pick one.The sister test at
apps/ccaudit/src/__tests__/restore-after-purge.test.ts(lines 130-141) writes a full purge header (checkpoint_ghost_hash,checkpoint_timestamp,since_window,os,node_version,planned_ops,selection_filter). Here the header contains only 4 fields. If the manifest reader is currently lenient about purge headers both pass, but if validation tightens later one of the two tests will silently start covering nothing. Worth aligning these fixtures (probably toward the fuller shape, or factoring amakePurgeManifest()helper into_test-helpers.ts) so test intent doesn't drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ccaudit/src/__tests__/restore-list-skips-purge.test.ts` around lines 110 - 130, The purge manifest header in this test is missing fields present in restore-after-purge.test.ts; update the purgeHeader object (the variable named purgeHeader used to build purgeBody and written to purgePath) to include the fuller set of keys used in the sister test (e.g., checkpoint_ghost_hash, checkpoint_timestamp, since_window, os, node_version, planned_ops, selection_filter and any other fields there) so both fixtures match, or alternatively add a shared makePurgeManifest()/makePurgeHeader() helper in _test-helpers.ts and use it here to construct the header before stringifying and writing purgeBody.CHANGELOG.md (1)
8-8: Minor: inconsistent dash style on version headers.Line 87 uses an em-dash (
## [1.5.0-beta.0] — 2026-04-19) while line 8 and all preceding releases use a plain hyphen (## [1.5.0] - 2026-04-26,## [1.4.0] - 2026-04-13, etc.). Some changelog parsers/anchors are punctuation-sensitive — worth normalizing on the ASCII hyphen for consistency.Optional follow-up: consider adding an empty
## [Unreleased]section above line 8 once 1.5.0 ships, per Keep a Changelog convention.Also applies to: 87-87
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CHANGELOG.md` at line 8, Normalize the dash style in the changelog headers: replace the em-dash in the header containing "## [1.5.0-beta.0] — 2026-04-19" with an ASCII hyphen to match the other headers like "## [1.5.0] - 2026-04-26" and ensure all release headers use " - " consistently; optionally add an empty "## [Unreleased]" section above the "## [1.5.0] - 2026-04-26" header to follow Keep a Changelog convention.apps/ccaudit/src/__tests__/pagination-500.test.ts (1)
20-21: Consider package-name imports over deep relative paths.
../../../../packages/terminal/src/tui/tabbed-picker.ts,./fixtures/ghost-500-items.ts, and the dynamicimport('../../../../packages/terminal/src/tui/_viewport.ts')reach into internal source paths. If the@ccaudit/terminalbarrel exposesTabbedGhostPickerandapplyScroll, prefer those imports — they survive directory restructures and avoid coupling integration tests to package internals. Also, the dynamicawait import(...)at line 96 can be hoisted to a static top-level import unless test-isolation requires the lazy form.Also applies to: 96-96
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ccaudit/src/__tests__/pagination-500.test.ts` around lines 20 - 21, Tests import internal source via deep relative paths (TabbedGhostPicker, buildGhosts500 and a dynamic import of _viewport), which couples tests to package internals; change imports to use the package barrel exports (e.g. import { TabbedGhostPicker, applyScroll } from '@ccaudit/terminal') and move fixture import to a package-level or test fixtures path (replace buildGhosts500 relative import accordingly). If test isolation is not required, replace the dynamic await import('../../../../packages/terminal/src/tui/_viewport.ts') with a static top-level import of applyScroll (or the exported symbol) from the package; otherwise keep it but import the exported symbol from the package barrel instead of the internal path.apps/ccaudit/src/cli/commands/purge-archive.ts (1)
134-250: SurfacemanifestErrorsin failure envelopes as well, not only on success.
manifestErrorsis collected by the per-readManifesttry/catch (line 145–156) but is only included in the success envelope at line 264. IfdiscoverManifestsorclassifyArchiveOpsthen fails (lines 167–201) — orexecutePurgereturnsResult.isFailure(lines 207–250) — the operator's JSON only sees the top-level error message; any preceding unreadable-manifest entries are silently lost. Same applies for the per-class history record on those paths.♻️ Suggested fix
if (outMode.json) { process.stdout.write( JSON.stringify( buildJsonEnvelope('purge-archive', 'n/a', 1, { purge: { summary: { ... }, - failures: [{ path: '', reason: message }], + failures: [ + { path: '', reason: message }, + ...manifestErrors, + ], dryRun, }, }), ) + '\n', ); }(Apply analogously to the
Result.isFailureenvelope at lines 215–224.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ccaudit/src/cli/commands/purge-archive.ts` around lines 134 - 250, The code collects unreadable-manifest entries in manifestErrors inside the readManifest loop but only includes them in the success envelope; fix by appending manifestErrors (mapped to {path,reason} or their reason strings) into the JSON failure envelopes and the safeRecordHistory errors/result for both the catch branch that handles discoverManifests/classifyArchiveOps failures and the Result.isFailure branch after executePurge; update the objects built for buildJsonEnvelope('purge-archive', ...) and the errors/result arrays passed to safeRecordHistory so they include manifestErrors (e.g., include manifestErrors as additional failures in the purge.failures array and merge their reasons into the history.errors array) while preserving the existing top-level message.apps/ccaudit/src/__tests__/tabbed-picker-filter.test.ts (1)
93-102: Consider addingTZ: 'UTC'tobaseEnvfor parity with sibling integration tests.
restore-interactive-roundtrip.test.ts(and the broader CI norm in this PR) setsTZ='UTC'on the spawned subprocess. The fixture here writesnew Date(...).toISOString()so the test isn't actively timezone-sensitive today, but pinning TZ keeps any future timestamp/marker assertion reproducible across local dev environments.♻️ Optional fix
function baseEnv(extra: Record<string, string> = {}): Record<string, string> { return { CCAUDIT_FORCE_TTY: '1', CCAUDIT_TEST_STDOUT_ROWS: '30', LINES: '30', COLUMNS: '100', NO_COLOR: '1', + TZ: 'UTC', ...extra, }; }As per coding guidelines: Use ISO 8601 UTC strings for time values.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ccaudit/src/__tests__/tabbed-picker-filter.test.ts` around lines 93 - 102, The test environment helper baseEnv is missing a TZ override which other integration tests set to ensure reproducible timestamps; update the baseEnv function to include TZ: 'UTC' in its returned object (i.e., add the TZ key alongside CCAUDIT_FORCE_TTY, LINES, COLUMNS, NO_COLOR, etc.) so spawned subprocesses use UTC and timestamp-related fixtures remain deterministic across environments.apps/ccaudit/src/__tests__/ghost-select-ids.test.ts (1)
39-63: DuplicatedFAKE_PS_SCRIPT/buildFakePs— other tests import these from_test-helpers.ts.
apps/ccaudit/src/__tests__/preflight-retry.test.ts(line 33) andapps/ccaudit/src/__tests__/safety-invariants-framework.test.ts(line 34) already importbuildFakePsfrom./_test-helpers.ts. Re-defining it inline here drifts in two ways: (1) bug fixes / behavior changes to the helper won't reach this suite, and (2) theagentItemIdhelper in this file (lines 165–174) is a re-implementation of the version exported from_test-helpers.ts.♻️ Suggested cleanup
-import { readManifest, canonicalItemId } from '@ccaudit/internal'; -import type { InventoryItem } from '@ccaudit/internal'; +import { readManifest } from '@ccaudit/internal'; +import { buildFakePs, agentItemId } from './_test-helpers.ts'; -const FAKE_PS_SCRIPT = `#!/bin/sh -... -`; - -async function buildFakePs(tmpHome: string): Promise<string> { ... } -function agentItemId(tmpHome: string, fileName: string): string { ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ccaudit/src/__tests__/ghost-select-ids.test.ts` around lines 39 - 63, This test file duplicates FAKE_PS_SCRIPT, buildFakePs and agentItemId that already live in _test-helpers.ts; remove the local FAKE_PS_SCRIPT and buildFakePs definitions and the re-implemented agentItemId, and instead import buildFakePs and agentItemId from ./_test-helpers.ts and use those imports throughout the test (referencing the symbols buildFakePs and agentItemId to locate where to replace the local code).apps/ccaudit/src/__tests__/tabbed-picker-live-counter.test.ts (1)
51-58: ANSI stripper covers common CSI but not OSC/DCS sequences — fine for picker output, fragile if rendering grows.The current regex set handles CSI cursor/SGR/erase codes, which is sufficient for the
@clack/promptsand ink-style output the picker emits today. If future picker work introduces OSC (\x1b]...\x1b\\for hyperlinks/title sets) or DCS sequences, this stripper would leak them into the assertion buffer and produce confusing failures. Not a blocker — flagging for awareness if the picker adds richer terminal features.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ccaudit/src/__tests__/tabbed-picker-live-counter.test.ts` around lines 51 - 58, The stripAnsi function currently only removes CSI sequences and cursor show/hide codes and may leave OSC (`\x1b]... \x1b\\`) and DCS sequences; update the stripAnsi function to also remove OSC and DCS sequences by adding regexes that strip OSC patterns (`\x1b\][^\x1b]*(\x1b\\|\x07)`) and DCS patterns (`\x1bP[^\x1b]*\x1b\\`) in addition to the existing CSI rules so assertions won’t leak richer terminal escapes if the picker output grows.apps/ccaudit/src/cli/commands/restore.ts (1)
286-330: Duplicated manifest-walk pre-dispatch — extract a helper.The blocks for
--name(288–310),--all-matching(318–330), and--interactive(337–348) each rebuildpairsvia the same incantation:const entries = await findManifestsForRestore(deps); const pairs = await Promise.all( entries.map(async (entry) => ({ entry, ops: (await readManifest(entry.path)).ops })), );Three near-identical IO passes that each re-read every manifest. Lifting this into a small helper (e.g.,
loadManifestPairs(deps)) would deduplicate and also let you cache the result if a future invocation combines flags or uses the same data twice.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ccaudit/src/cli/commands/restore.ts` around lines 286 - 330, The restore command duplicates manifest-reading logic in the nameFlag, allMatchingFlag and interactive branches by repeatedly calling findManifestsForRestore and readManifest to build pairs; extract this into a helper (e.g., loadManifestPairs(deps)) that performs the single IO pass: call findManifestsForRestore(deps), map entries to { entry, ops: (await readManifest(entry.path)).ops } with Promise.all, and return the pairs (or deduped result if you prefer); then replace the repeated blocks in the nameFlag and allMatchingFlag branches to reuse loadManifestPairs and feed its result into dedupManifestOps/matchByName to produce nameResolvedId and allMatchingResolvedIds.packages/internal/src/remediation/bust.ts (1)
305-313: Minor: emittingconsole.warnfrom the domain layer.The warning at lines 308–312 fires from inside
runBust, which is otherwise a pure orchestrator returning a discriminatedBustResult. Per the project's "catch at the outermost layer, propagate via Result inside the domain" convention, this informational message would arguably be cleaner as a non-error field onBustResult.success(or simply emitted by the CLI command after inspectingselectedItems). Not a blocker — the message is benign and idempotent — but it's the only stderr write inside the orchestrator.Based on learnings: "Catch errors at the outermost layer (CLI command, history writer) and degrade gracefully; propagate errors inside the domain via
Resulttype."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/internal/src/remediation/bust.ts` around lines 305 - 313, The orchestrator runBust currently emits a console.warn when selectedItems is an empty set; remove that stderr write and instead propagate the informational message via the BustResult success value (e.g., add a warnings or infoMessage field on BustResult.success) so callers can decide how to display it; locate the selectedItems check in runBust (and the filteredPlan/filterChangePlan logic) and when selectedItems.size === 0 populate the new warning field on the returned success result rather than calling console.warn, or leave it unset and let the CLI inspect selectedItems and emit the message.packages/internal/src/remediation/manifest.ts (2)
447-512: Optional: dedupe purge-manifest path generation.
openPurgeManifestWriterreconstructs the filename inline at lines 501–503 rather than reusingresolvePurgeManifestPath. The two implementations will drift if the format ever changes (e.g., suffix length, prefix). CallingresolvePurgeManifestPath(new Date(ts))and thenpath.dirname/basenamefor the override path keeps a single source of truth.♻️ Suggested refactor
export async function openPurgeManifestWriter( header: { ccaudit_version: string; purge_timestamp: string }, manifestsDir?: string, ): Promise<{ writer: ManifestWriter; path: string }> { const ts = header.purge_timestamp; - const baseDir = manifestsDir ?? path.join(homedir(), '.claude', 'ccaudit', 'manifests'); - const stamp = new Date(ts).toISOString().replace(/:/g, '-').replace(/\./g, '-'); - const rand = Math.random().toString(36).slice(2, 6); - const manifestPath = path.join(baseDir, `purge-${stamp}-${rand}.jsonl`); + const defaultPath = resolvePurgeManifestPath(new Date(ts)); + const manifestPath = manifestsDir + ? path.join(manifestsDir, path.basename(defaultPath)) + : defaultPath; const writer = new ManifestWriter(manifestPath);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/internal/src/remediation/manifest.ts` around lines 447 - 512, openPurgeManifestWriter duplicates the purge filename logic from resolvePurgeManifestPath, risking drift; replace the inline stamp/rand generation by calling resolvePurgeManifestPath(new Date(ts)) to get a canonical filename, then if manifestsDir is provided use path.join(manifestsDir, path.basename(resolvedPath)) otherwise use resolvedPath as manifestPath, and proceed to construct the ManifestWriter and call writer.open(buildPurgeManifestHeader(...)) as before (update references in openPurgeManifestWriter to use resolvePurgeManifestPath, ManifestWriter, buildPurgeManifestHeader).
560-604: Optional: collapsewritePurgeManifest's dual-mode branches.The two branches (lines 570–588 with explicit
manifestPath, lines 590–604 viaopenPurgeManifestWriter) duplicate the open/write-loop/close/error-rewind logic. Once the path-resolution helper above is shared, these collapse to a single try block. Since the doc-comment already calls this a backward-compat wrapper, this is purely a nice-to-have.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/internal/src/remediation/manifest.ts` around lines 560 - 604, writePurgeManifest duplicates the writer creation/open/write/close/error-rewind logic in two branches; refactor to resolve writer and manifestPath before the try/catch: if input.manifestPath create const writer = new ManifestWriter(input.manifestPath) and await writer.open(buildPurgeManifestHeader(...)) and set manifestPath = input.manifestPath, otherwise call const { writer, path: manifestPath } = await openPurgeManifestWriter(...); then have a single try block that loops await writer.writeOp(op) for each op, calls await closePurgeManifestWriter(writer, { durationMs: writer.elapsedMs }), and a single catch that does await writer.close(null) and rethrows; reference symbols: writePurgeManifest, ManifestWriter, openPurgeManifestWriter, closePurgeManifestWriter, buildPurgeManifestHeader.packages/internal/src/remediation/purge.ts (1)
268-300: Minor: comment drift onstale_archive_missingbranch.The comment at lines 270–273 says "we still write the follow-up op to suppress the stale entry from future restore listings," and indeed the unlink is correctly skipped for
stale_archive_missing(line 274 only unlinks onsource_occupied). This is the intended behavior. However, the surroundingtrystill records a generic failure on any throw at line 293–299, which would attribute filesystem failures to a stale op that didn't perform any I/O — only thewriteOpcould realistically throw here. Readability nit only; the logic is correct.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/internal/src/remediation/purge.ts` around lines 268 - 300, The outer try currently wraps both the conditional unlink (deps.unlinkFile) and the writer.writeOp, causing filesystem errors to be reported against ops that didn't perform I/O (e.g., reason === 'stale_archive_missing'); split the logic so unlink errors are handled separately from manifest write errors: perform deps.unlinkFile inside its own try/catch (or a narrowly-scoped try) and push a failure entry specific to unlink failures if it throws, then run buildArchivePurgeOp and the existing writer.writeOp try/catch exactly as-is to record manifest write failures; reference the loop over plan.drop, the conditional check for reason === 'source_occupied', deps.unlinkFile, buildArchivePurgeOp, writer.writeOp, and the failures/appendedOps/summary updates when moving the catch scopes.packages/internal/src/remediation/restore.ts (2)
1250-1267: Inconsistent corrupt-manifest handling inall-matchingpath.Corrupt middle manifests are silently skipped here (
if (m.header === null) continue;), but bothexecuteInteractiveOps(Line 954) and thefullpath (Line 1201) emit a warning viadeps.onWarning?.for the same condition. Worth aligning so users always see when a manifest is being skipped.♻️ Proposed alignment
for (const e of allEntries) { const m = await deps.readManifest(e.path); - if (m.header === null) continue; + if (m.header === null) { + deps.onWarning?.(`⚠️ Skipping corrupt manifest ${path.basename(e.path)} (no header record)`); + continue; + } zipped.push({ entry: e, ops: m.ops }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/internal/src/remediation/restore.ts` around lines 1250 - 1267, In the all-matching branch, when iterating allEntries and calling deps.readManifest (inside the block that currently does "if (m.header === null) continue;"), emit the same warning as other paths instead of silently skipping: call deps.onWarning?.(...) with a message/metadata indicating a corrupt or unreadable manifest (include the entry/path or canonical id) before continuing. This aligns behavior with executeInteractiveOps and the full path and ensures users see skipped corrupt manifests; update the loop that builds zipped (the readManifest check) to perform the warning prior to continue.
349-366: Optional: parallelizepathExistschecks.
filterRestoreableItemsawaits eachisStaleArchiveOpsequentially. For users with many archived items (e.g. a tab in the TUI containing dozens of archive ops across multiple manifests), this serializes the per-oppathExists × 2calls and can be noticeable on slow filesystems. The list-mode loop (Lines 497-503) and full-restore loop (Lines 1207-1217) have the same shape.A drop-in
Promise.allover the items keeps the kept-order deterministic and the stale count exact:♻️ Proposed parallelization (filterRestoreableItems)
- const kept: Array<{ entry: ManifestEntry; op: RestoreableOp; canonical_id: string }> = []; - let filteredStaleCount = 0; - for (const item of items) { - if (item.op.op_type === 'archive' && (await isStaleArchiveOp(item.op, pathExists))) { - filteredStaleCount += 1; - continue; - } - kept.push(item); - } - return { kept, filteredStaleCount }; + const staleness = await Promise.all( + items.map(async (item) => + item.op.op_type === 'archive' ? isStaleArchiveOp(item.op, pathExists) : false, + ), + ); + const kept: Array<{ entry: ManifestEntry; op: RestoreableOp; canonical_id: string }> = []; + let filteredStaleCount = 0; + items.forEach((item, i) => { + if (staleness[i]) filteredStaleCount += 1; + else kept.push(item); + }); + return { kept, filteredStaleCount };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/internal/src/remediation/restore.ts` around lines 349 - 366, filterRestoreableItems currently awaits isStaleArchiveOp inside a for..of loop, causing sequential pathExists calls; instead, map items to promises (call isStaleArchiveOp only for items with op.op_type === 'archive', else resolve false), await Promise.all to get stale flags, then iterate original items and corresponding flags to build kept and filteredStaleCount (this preserves order and exact counts). Apply the same Promise.all pattern to the other two loops with the same shape (the list-mode loop and the full-restore loop) so their per-item pathExists checks run in parallel while keeping deterministic ordering.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/ccaudit/scripts/bundle-smoke-test.mjs`:
- Around line 17-25: The smoke test currently imports dist/index.js which
triggers top-level execution of run() from src/index.ts and can conflate CLI
help/missing-subcommand exits with real import-resolution failures; fix by
either (A) making src/index.ts only call await run() when executed as the main
module (guard the top-level invocation with import.meta.main or equivalent so
importing the module does not execute cli()), referencing the run() and cli(...)
calls in src/index.ts, or (B) change apps/ccaudit/scripts/bundle-smoke-test.mjs
to invoke the built CLI with a benign argument (e.g., "--help") when
importing/executing distPath and assert the process exit code to prove proper
initialization; pick one approach and implement the corresponding guard or
test-argument so the smoke test can reliably detect import-resolution failures.
In `@apps/ccaudit/src/__tests__/empty-inventory.test.ts`:
- Around line 88-96: The test is currently passing vacuously because the process
is not running in TTY mode; update the invocation of runCcauditGhost in the
'plain `ghost` on empty inventory' test to force TTY by injecting the
CCAUDIT_FORCE_TTY=1 env (or equivalent option supported by runCcauditGhost) so
the auto-open prompt code path is exercised; specifically modify the call to
runCcauditGhost(tmpHome, [], { timeout: 15_000 }) to include the environment
override (CCAUDIT_FORCE_TTY=1) so the assertions against stdout for the
interactive picker and prompt are meaningful.
In `@apps/ccaudit/src/__tests__/fixtures/tmux-e2e.ts`:
- Around line 212-222: The generated runner script silently continues if the cd
fails because the shebang block uses "set -uo pipefail" without "-e"; update the
script generation in the tmux-e2e fixture so the shell aborts on failing cd —
either add "-e" to the "set -uo pipefail" line or explicitly guard the cd (check
the directory using the same shellQuote(opts.cwd) value and exit with a clear
error and non-zero status if cd fails) so the test cannot continue in the wrong
working directory and the exit-status marker reflects the real failure.
In `@apps/ccaudit/src/__tests__/preflight-retry.test.ts`:
- Around line 167-186: Comment and test env disagree: the narrative describes
N=2 but the test sets CCAUDIT_TEST_PREFLIGHT_DIRTY='3'; update one to match the
other. Either change the explanatory text in the preflight accounting comment
(the paragraph describing "N=2" and the call-count math) to refer to N=3 and
adjust the described call counts accordingly, or set
CCAUDIT_TEST_PREFLIGHT_DIRTY to '2' in the runCcauditGhost invocation; locate
the test harness where runCcauditGhost is called and the
CCAUDIT_TEST_PREFLIGHT_DIRTY env entry to make the change so the comment and env
value are consistent.
In `@apps/ccaudit/src/__tests__/safety-invariants-framework.test.ts`:
- Around line 221-230: The test reassigns the shared tmpHome set up in
beforeEach, leaking the original temp directory and conflicting fixtures; fix by
either not reusing the shared beforeEach for this test or explicitly cleaning
the original tmpHome before reassigning it: locate the test that uses tmpHome
(the IIFE that calls buildFakePs and createMultiFrameworkFixture) and either
move it into its own describe with its own setup/teardown (so it doesn't rely on
beforeEach/afterEach), or call the same cleanup used in afterEach (rm(tmpHome, {
recursive: true }) or the project helper used for teardown) right before
creating the new temp dir so the original tmpHome is removed and no fixtures are
overlaid.
In `@apps/ccaudit/src/__tests__/scanner-multi-config-mcp.test.ts`:
- Around line 90-94: The current stability check is tautological — replace the
trivial copy-equality with a meaningful deterministic-order assertion: either
create an independently sorted reference by calling sorted =
[...it.configRefs!].sort(compareConfigRef) and assert
expect(it.configRefs).toEqual(sorted), or call scanMcpServers twice and assert
the two returned configRefs arrays are deep-equal (e.g.,
expect(first.configRefs).toEqual(second.configRefs)); reference
compareConfigRef, scanMcpServers and configRefs when making the change.
In `@apps/ccaudit/src/__tests__/tabbed-picker-force-partial-overflow.test.ts`:
- Around line 116-127: The test is counting unique agent-NN across the
accumulated output (agentMatches / plain) which can span multiple frames because
stdoutBuf is cumulative and stripAnsi removes overwrite cues; to fix, detect
when the banner first appears (e.g. by matching the banner text or using the
existing banner detection logic in this test) then reset/clear stdoutBuf (or
re-snapshot plain) immediately after that banner render and re-run the agent-NN
match on the fresh buffer so agentMatches/uniqueAgents reflect a single frame;
update the test around the agentMatches computation to re-collect plain after
clearing stdoutBuf (or re-snapshot) before asserting uniqueAgents ≤ 13.
In `@apps/ccaudit/src/__tests__/tabbed-picker-tab-nav-keys.test.ts`:
- Around line 175-185: The failure message when jsonlManifests.length !== 1
lacks useful diagnostics; update the thrown Error in the block that inspects
listManifestsDir/tmpHome (around variables jsonlManifests, manifestBase,
manifestPath, manifest) to include the subprocess exit code (result.exitCode)
and whether the confirmation prompt was reached by adding a boolean like
result.stdout.includes('Proceed with archive?') to the message; ensure you keep
the existing stdout/stderr slices and clearly label the new fields so CI logs
show exit code and the confirmation-marker presence.
In `@apps/ccaudit/src/cli/commands/restore.ts`:
- Around line 244-271: Several pre-dispatch hard-fail sites (mutual-exclusion
check, CCAUDIT_NO_INTERACTIVE refusal, TTY guard, the --name/--all-matching
no-match/ambiguity blocks, and interactive empty-archive) currently call
process.stderr.write(...) + process.exit(...) and must instead emit the JSON
failure envelope when outMode.json is active; replace those early exits with the
same JSON-emitting path used in the catch block (call
buildJsonEnvelope('restore','n/a', exitCode, { error: <message> }) and write it
to stdout or throw a typed Error that the existing catch will convert), ensure
you consult outMode.json before emitting plain-text, and call safeRecordHistory
where appropriate before exiting so history is not silently skipped.
In `@packages/internal/src/remediation/_archive-move.ts`:
- Line 62: The docstring for the ArchiveMove operation incorrectly claims
mkdirRecursive uses mode 0o700 while ArchiveMoveDeps.mkdirRecursive is typed as
(dir: string) => Promise<void> and the caller in
apps/ccaudit/src/cli/commands/restore.ts actually calls mkdir(dir, { recursive:
true, mode }) defaulting to 0o755; fix this by either (A) updating the docstring
near the ArchiveMoveDeps/ArchiveMove comment to state that the parent directory
is created with the caller-provided/default mode (0o755) and remove the "0o700
where honored" text, or (B) if 0o700 is required, add a mode parameter to
ArchiveMoveDeps.mkdirRecursive (e.g., mkdirRecursive(dir: string, mode?:
number)) and propagate that parameter through the restore.ts wiring to
mkdir(..., { mode: 0o700 }); choose one approach and update the signature,
calls, and docstring consistently (references: ArchiveMoveDeps.mkdirRecursive
and the restore.ts mkdir call).
In `@packages/internal/src/remediation/purge.ts`:
- Around line 316-326: The footer duration is hardcoded to 0 when closing the
manifest; instead use the writer's elapsedMs so the manifest footer records the
real elapsed time. Update the close call in purge.ts to pass the writer's
elapsedMs (the getter provided by ManifestWriter.open) into
closePurgeManifestWriter rather than 0, mirroring how writePurgeManifest in
manifest.ts reads writer.elapsedMs for bust manifests.
In `@README.md`:
- Around line 163-178: The README picker example incorrectly shows a "[ hooks
(0/1) ]" tab which contradicts SC6 and the PR scope; update the ASCII picker
sample to remove the hooks tab so it only lists agents, skills, mcp, commands,
and memory (remove the "[ hooks (0/1) ]" fragment and adjust spacing/counts
accordingly), and add a one-line note that hook archival is deferred in v1.5
(referencing the advisory string in ghost.ts such as "Hook archival deferred —
selectable archive coming in a future phase"); ensure this aligns with the test
expectation in apps/ccaudit/src/__tests__/preflight-retry.test.ts that the
rendered tab bar does not contain "HOOKS".
---
Nitpick comments:
In `@apps/ccaudit/scripts/bundle-smoke-test.mjs`:
- Around line 4-9: Replace the round-trip path resolution using
fileURLToPath/__filename and resolve with the ESM URL form: create the dist file
URL via new URL('../../dist/index.js', import.meta.url) (remove the __filename
variable and the fileURLToPath import) and use that URL (or convert to a
filesystem path only where needed, e.g. with fileURLToPath(new URL(...)) at the
point of calling existsSync). Update references to distPath to use the new URL
variable (or its file path) and remove the unused imports (fileURLToPath and
resolve) so the script uses canonical ESM URL resolution.
In `@apps/ccaudit/src/__tests__/fixtures/tmux-e2e.ts`:
- Around line 176-178: The generated runner script (runnerPath) written by
buildRunnerScript is not removed and should be cleaned up: store the runnerPath
on the TmuxE2ESession instance when you write it, then unlink it in
TmuxE2ESession.kill() (and also on normal session exit) so the file is removed
even on early failures; alternatively modify the API that creates the script to
return the script path to callers so they can manage deletion. Ensure you
reference the runnerPath property in the cleanup logic and handle ENOENT errors
safely when unlinking.
- Around line 142-146: The code in waitForText redundantly calls stripAnsi on
raw even though capture(...) already respects opts.stripAnsi via its ansi flag;
replace the conditional assignment "lastCapture = opts.stripAnsi === false ? raw
: stripAnsi(raw)" with a single unconditional "lastCapture = raw" (leave the
capture(...) call and its ansi: opts.stripAnsi === false argument intact) so
lastCapture trusts capture() to include/strip ANSI and removes the extra regex
work; update references to lastCapture accordingly.
In `@apps/ccaudit/src/__tests__/ghost-select-ids.test.ts`:
- Around line 39-63: This test file duplicates FAKE_PS_SCRIPT, buildFakePs and
agentItemId that already live in _test-helpers.ts; remove the local
FAKE_PS_SCRIPT and buildFakePs definitions and the re-implemented agentItemId,
and instead import buildFakePs and agentItemId from ./_test-helpers.ts and use
those imports throughout the test (referencing the symbols buildFakePs and
agentItemId to locate where to replace the local code).
In `@apps/ccaudit/src/__tests__/pagination-500.test.ts`:
- Around line 20-21: Tests import internal source via deep relative paths
(TabbedGhostPicker, buildGhosts500 and a dynamic import of _viewport), which
couples tests to package internals; change imports to use the package barrel
exports (e.g. import { TabbedGhostPicker, applyScroll } from
'@ccaudit/terminal') and move fixture import to a package-level or test fixtures
path (replace buildGhosts500 relative import accordingly). If test isolation is
not required, replace the dynamic await
import('../../../../packages/terminal/src/tui/_viewport.ts') with a static
top-level import of applyScroll (or the exported symbol) from the package;
otherwise keep it but import the exported symbol from the package barrel instead
of the internal path.
In `@apps/ccaudit/src/__tests__/restore-list-skips-purge.test.ts`:
- Around line 110-130: The purge manifest header in this test is missing fields
present in restore-after-purge.test.ts; update the purgeHeader object (the
variable named purgeHeader used to build purgeBody and written to purgePath) to
include the fuller set of keys used in the sister test (e.g.,
checkpoint_ghost_hash, checkpoint_timestamp, since_window, os, node_version,
planned_ops, selection_filter and any other fields there) so both fixtures
match, or alternatively add a shared makePurgeManifest()/makePurgeHeader()
helper in _test-helpers.ts and use it here to construct the header before
stringifying and writing purgeBody.
In `@apps/ccaudit/src/__tests__/tabbed-picker-filter.test.ts`:
- Around line 93-102: The test environment helper baseEnv is missing a TZ
override which other integration tests set to ensure reproducible timestamps;
update the baseEnv function to include TZ: 'UTC' in its returned object (i.e.,
add the TZ key alongside CCAUDIT_FORCE_TTY, LINES, COLUMNS, NO_COLOR, etc.) so
spawned subprocesses use UTC and timestamp-related fixtures remain deterministic
across environments.
In `@apps/ccaudit/src/__tests__/tabbed-picker-live-counter.test.ts`:
- Around line 51-58: The stripAnsi function currently only removes CSI sequences
and cursor show/hide codes and may leave OSC (`\x1b]... \x1b\\`) and DCS
sequences; update the stripAnsi function to also remove OSC and DCS sequences by
adding regexes that strip OSC patterns (`\x1b\][^\x1b]*(\x1b\\|\x07)`) and DCS
patterns (`\x1bP[^\x1b]*\x1b\\`) in addition to the existing CSI rules so
assertions won’t leak richer terminal escapes if the picker output grows.
In `@apps/ccaudit/src/__tests__/tabbed-picker-tab-nav-keys.test.ts`:
- Around line 213-240: Two tests ("Tab key ('\\t') cycles tabs forward..." and
"ArrowRight key ('\\x1b[C') cycles tabs forward...") are duplicated; replace
them with a parameterized it.each that iterates over cases with a descriptive
title and the key sequence and runs the same assertions. Locate the tests that
call runPickerWithTabKey(tmpHome, <key>) and consolidate into a single it.each
table (e.g., rows for ['Tab key (\\t)', '\t'] and ['ArrowRight key (\\x1b[C)',
'\x1b[C']) and keep the existing assertions against exitCode,
manifest.header?.planned_ops.archive, manifest.ops.length, and archiveOps
categories unchanged. Ensure the test timeout (25_000) is preserved and the test
body still destructures { exitCode, manifest, stderr } from runPickerWithTabKey.
- Around line 129-161: Extract the duplicated exponential-backoff stdout polling
into a reusable helper (e.g., waitForStdout) and replace both while loops with
calls to it: implement waitForStdout(spawned.child, () => stdoutBuf, marker,
maxWaitMs) that polls until the marker substring appears or the child exits or
timeout elapses; put the helper in a shared test helper module (e.g.,
_test-helpers.ts) and update the two call sites in
tabbed-picker-tab-nav-keys.test.ts to call waitForStdout(spawned.child, () =>
stdoutBuf, 'AGENTS (0/2)', 5000) and waitForStdout(spawned.child, () =>
stdoutBuf, 'Proceed with archive?', 5000) respectively.
In `@apps/ccaudit/src/cli/commands/purge-archive.ts`:
- Around line 134-250: The code collects unreadable-manifest entries in
manifestErrors inside the readManifest loop but only includes them in the
success envelope; fix by appending manifestErrors (mapped to {path,reason} or
their reason strings) into the JSON failure envelopes and the safeRecordHistory
errors/result for both the catch branch that handles
discoverManifests/classifyArchiveOps failures and the Result.isFailure branch
after executePurge; update the objects built for
buildJsonEnvelope('purge-archive', ...) and the errors/result arrays passed to
safeRecordHistory so they include manifestErrors (e.g., include manifestErrors
as additional failures in the purge.failures array and merge their reasons into
the history.errors array) while preserving the existing top-level message.
In `@apps/ccaudit/src/cli/commands/restore.ts`:
- Around line 286-330: The restore command duplicates manifest-reading logic in
the nameFlag, allMatchingFlag and interactive branches by repeatedly calling
findManifestsForRestore and readManifest to build pairs; extract this into a
helper (e.g., loadManifestPairs(deps)) that performs the single IO pass: call
findManifestsForRestore(deps), map entries to { entry, ops: (await
readManifest(entry.path)).ops } with Promise.all, and return the pairs (or
deduped result if you prefer); then replace the repeated blocks in the nameFlag
and allMatchingFlag branches to reuse loadManifestPairs and feed its result into
dedupManifestOps/matchByName to produce nameResolvedId and
allMatchingResolvedIds.
In `@CHANGELOG.md`:
- Line 8: Normalize the dash style in the changelog headers: replace the em-dash
in the header containing "## [1.5.0-beta.0] — 2026-04-19" with an ASCII hyphen
to match the other headers like "## [1.5.0] - 2026-04-26" and ensure all release
headers use " - " consistently; optionally add an empty "## [Unreleased]"
section above the "## [1.5.0] - 2026-04-26" header to follow Keep a Changelog
convention.
In `@eslint.config.ts`:
- Around line 10-15: The current change raises
projectService.maximumDefaultProjectFileMatchCount_THIS_WILL_SLOW_DOWN_LINTING
to include more .mjs files; instead add those script paths to a proper tsconfig
and point ESLint at it: create or update a tsconfig (e.g. tsconfig.scripts.json)
that includes apps/ccaudit/scripts/*.mjs and scripts/*.mjs, then modify the
ESLint config to reference that project via projectService.project (or project)
and remove the allowDefaultProject and
maximumDefaultProjectFileMatchCount_THIS_WILL_SLOW_DOWN_LINTING entries; update
any comments to reflect the new tsconfig usage.
In `@packages/internal/src/remediation/bust.ts`:
- Around line 305-313: The orchestrator runBust currently emits a console.warn
when selectedItems is an empty set; remove that stderr write and instead
propagate the informational message via the BustResult success value (e.g., add
a warnings or infoMessage field on BustResult.success) so callers can decide how
to display it; locate the selectedItems check in runBust (and the
filteredPlan/filterChangePlan logic) and when selectedItems.size === 0 populate
the new warning field on the returned success result rather than calling
console.warn, or leave it unset and let the CLI inspect selectedItems and emit
the message.
In `@packages/internal/src/remediation/manifest.ts`:
- Around line 447-512: openPurgeManifestWriter duplicates the purge filename
logic from resolvePurgeManifestPath, risking drift; replace the inline
stamp/rand generation by calling resolvePurgeManifestPath(new Date(ts)) to get a
canonical filename, then if manifestsDir is provided use path.join(manifestsDir,
path.basename(resolvedPath)) otherwise use resolvedPath as manifestPath, and
proceed to construct the ManifestWriter and call
writer.open(buildPurgeManifestHeader(...)) as before (update references in
openPurgeManifestWriter to use resolvePurgeManifestPath, ManifestWriter,
buildPurgeManifestHeader).
- Around line 560-604: writePurgeManifest duplicates the writer
creation/open/write/close/error-rewind logic in two branches; refactor to
resolve writer and manifestPath before the try/catch: if input.manifestPath
create const writer = new ManifestWriter(input.manifestPath) and await
writer.open(buildPurgeManifestHeader(...)) and set manifestPath =
input.manifestPath, otherwise call const { writer, path: manifestPath } = await
openPurgeManifestWriter(...); then have a single try block that loops await
writer.writeOp(op) for each op, calls await closePurgeManifestWriter(writer, {
durationMs: writer.elapsedMs }), and a single catch that does await
writer.close(null) and rethrows; reference symbols: writePurgeManifest,
ManifestWriter, openPurgeManifestWriter, closePurgeManifestWriter,
buildPurgeManifestHeader.
In `@packages/internal/src/remediation/purge.ts`:
- Around line 268-300: The outer try currently wraps both the conditional unlink
(deps.unlinkFile) and the writer.writeOp, causing filesystem errors to be
reported against ops that didn't perform I/O (e.g., reason ===
'stale_archive_missing'); split the logic so unlink errors are handled
separately from manifest write errors: perform deps.unlinkFile inside its own
try/catch (or a narrowly-scoped try) and push a failure entry specific to unlink
failures if it throws, then run buildArchivePurgeOp and the existing
writer.writeOp try/catch exactly as-is to record manifest write failures;
reference the loop over plan.drop, the conditional check for reason ===
'source_occupied', deps.unlinkFile, buildArchivePurgeOp, writer.writeOp, and the
failures/appendedOps/summary updates when moving the catch scopes.
In `@packages/internal/src/remediation/restore.ts`:
- Around line 1250-1267: In the all-matching branch, when iterating allEntries
and calling deps.readManifest (inside the block that currently does "if
(m.header === null) continue;"), emit the same warning as other paths instead of
silently skipping: call deps.onWarning?.(...) with a message/metadata indicating
a corrupt or unreadable manifest (include the entry/path or canonical id) before
continuing. This aligns behavior with executeInteractiveOps and the full path
and ensures users see skipped corrupt manifests; update the loop that builds
zipped (the readManifest check) to perform the warning prior to continue.
- Around line 349-366: filterRestoreableItems currently awaits isStaleArchiveOp
inside a for..of loop, causing sequential pathExists calls; instead, map items
to promises (call isStaleArchiveOp only for items with op.op_type === 'archive',
else resolve false), await Promise.all to get stale flags, then iterate original
items and corresponding flags to build kept and filteredStaleCount (this
preserves order and exact counts). Apply the same Promise.all pattern to the
other two loops with the same shape (the list-mode loop and the full-restore
loop) so their per-item pathExists checks run in parallel while keeping
deterministic ordering.
In `@packages/terminal/src/tables/change-plan.ts`:
- Around line 73-91: The three archive rows in the change-plan display (the
lines.push template strings that use plan.counts.agents / plan.counts.skills /
plan.counts.commands) produce misaligned arrows because "commands" is longer;
normalize the label column width by padding the label text to a common width
(e.g., use label.padEnd(8) or format the string so "agents", "skills", and
"commands" occupy the same character width) in each of the three template
strings so the "→ ~/.claude/ccaudit/archived/..." arrows align vertically.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
…ction Ships an interactive TUI for selective ghost archival, scoped restore flows, framework-as-unit protection, and a purge command for draining the archive directory. All flows preserve the v1.4.0 safety invariants: nothing is deleted, every mutation is manifest-logged, every bust is gated by a SHA-256 inventory checkpoint. ### Added - `ccaudit ghost --interactive` (default when stdout is a TTY): tabbed picker across agents, skills, MCP servers, commands, and memory with live token counter, fuzzy filter (`/`), sort cycle (`s`), framework group toggle, help overlay (`?`), and SIGWINCH-safe pagination. Subset busts write filtered `manifest.header.planned_ops` and a new `bust.summary.freedTokens` (subset-accurate) alongside the additive `totalPlannedTokens` (full-plan figure preserved). - `ccaudit restore --interactive` / `--name <pattern>` / `--all-matching`: scoped restore selection across all manifests; surfaces `restore.selection_filter` and `restore.skipped[]` in the JSON envelope. - `ccaudit purge-archive`: classifier-driven drain of `~/.claude/ccaudit/archived/` (reclaim-if-free → drop-if-occupied → drop-if-stale → skip). Default `--dry-run`; real purge requires `--yes`. Each mutation appends an `archive_purge` op to a fresh purge manifest; originals are never rewritten. - Framework-as-unit protection: members of partially-used frameworks are unselectable in the picker and skipped by the non-interactive path unless `--force-partial` is set on both `--dry-run` and `--dangerously-bust-ghosts`. - `CCAUDIT_NO_INTERACTIVE=1` env gate: silently suppresses auto-open; hard-refuses explicit `--interactive` with exit 2. - Empty-inventory short-circuit on `ghost` and `ghost --interactive`. - Color-blind-friendly glyph set with ASCII fallback under `NO_COLOR` / `TERM=dumb`: ◉ selected, ◯ unselected, 🔒 protected, ⚠ multi-config MCP, ⌛ stale memory. - Tabbed-picker pagination bounded by terminal rows; 500+ item tabs scroll cleanly; scroll position preserved across filter / sort / framework-toggle. - Stale-archive filter on `restore` listing; surfaces `restore.filtered_stale_count` in JSON. ### Changed - README headline reframed around the Opus 4.7 1M-token context window: ~108k token ghost inventory now expressed as ~11% of 1M, with the absolute number kept inline. New "Native alternatives" section compares `/skills t-sort`, `/usage`, and `claude plugin disable` against ccaudit's cross-component scope, regime-aware token math, and archive-with-rollback differentiator. - Manifest header records subset-vs-full provenance; `restore` walks every manifest in `~/.claude/ccaudit/manifests/` deduplicated by `archive_path` (newer wins). - `restore --list` now skips purge follow-up manifests so they don't appear as restoreable busts. - Memory canonical_ids include op_type and op_id so distinct flag/refresh ops on the same file remain individually restoreable (INV-S3 strengthening). ### Fixed - Unselected MCP server keys are byte-preserved in `~/.claude.json` across subset busts (no whitespace churn, no key reordering). - MCP rename in global-disable now scopes its key search to the top-level `mcpServers` block, preventing accidental mutation of a project-scoped entry that happens to share the same name. - SIGINT during any TUI flow produces zero manifest writes. - `--help` output for `restore` and `ghost` subcommands (negatable flags, drop `-v` short, pattern metavars). - Memory ops surfaced in restore picker via `collectRestoreableItems`. - Windows path rendering: `presentPath` and `scan-memory` import paths now normalize backslashes so project-relative and home-relative paths render correctly on Windows. - Full-bust health summary now excludes archived commands from the remaining ghost count (was previously only excluding agents/skills/MCP). - `reclaim` performs a fresh source-existence check at move-time via the injected `pathExists` callback, closing a TOCTOU window between orphan detection and rename. - `ghost --interactive` under `CCAUDIT_NO_INTERACTIVE=1` now sets `process.exitCode` and returns instead of `process.exit()`, so the outer try/finally writes a history entry for the refused invocation. - `restore` CLI wraps `findManifestsForRestore`, `readManifest`, and `openRestorePicker` in the outer try/catch so corrupt manifests and TUI init failures degrade gracefully through the structured stderr / JSON / history path. - `purge` `executePurge()` now wraps `writeManifest` in try/catch and returns `Result.fail(...)` on throw, keeping the domain-layer Result contract. - TUI integration test helpers `waitForPicker` and `waitForMarker` centralized in `_test-helpers.ts`; the inverted `child.exitCode` poll condition is fixed and `waitForMarker` now throws on timeout rather than silently continuing past missing output. ### Invariants INV-S1..INV-S6 added to CLAUDE.md as fixture-backed integration tests. See [CHANGELOG.md](./CHANGELOG.md) and [docs/JSON-SCHEMA.md](./docs/JSON-SCHEMA.md) for the full surface.
95fe0c1 to
1876e84
Compare
Two stale `v1.5.0-beta.0` strings remained at lines 34 and 685 after the package.json bump to 1.5.0. Aligns headline + Version section with the shipped package version and adds the missing `purge-archive` mention.
Three upcoming directions: an interactive TUI process killer for live Claude Code sessions, a stable v1.6 JSON CLI game-API contract (initial consumer: the ccaudit Ghost Town game), and a forthcoming visual frontend.
There was a problem hiding this comment.
Actionable comments posted: 13
♻️ Duplicate comments (2)
packages/internal/src/remediation/restore.ts (1)
473-503:⚠️ Potential issue | 🟠 Major
restore --liststill leaks items that were already drained bypurge-archive.This loop skips the
purge-*.jsonlfiles themselves, but it never suppresses olderarchiveops whoseop_idis referenced by those purge manifests. If the source path later disappears, those purged items show back up in--listeven though the full-restore and interactive paths intentionally hide them. ReusecollectPurgedOpIds()here before buildingkeptso list mode stays consistent with the other restore flows.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/internal/src/remediation/restore.ts` around lines 473 - 503, In executeListMode, call collectPurgedOpIds() (using the same deps) before iterating manifest.ops and use the returned Set of purged op_ids to exclude any ManifestOp whose op_id is present (in addition to the existing stale archive check); update the kept construction (the loop that builds kept and increments filteredStaleCount) to skip ops when op.op_id is in the purged set so list mode mirrors full-restore/interactive behavior (refer to collectPurgedOpIds, executeListMode, manifest.ops, isStaleArchiveOp, filteredStaleCount, and ManifestOp to locate the change).apps/ccaudit/src/cli/commands/restore.ts (1)
377-380:⚠️ Potential issue | 🟠 MajorDon’t bypass the shared JSON/history path on picker cancellation.
This direct
process.exit(0)is the remaining early-exit hole in the new pre-dispatch flow.restore --interactive --jsonwill emit plain stderr with empty stdout on cancel, and cancellation never reachessafeRecordHistory(). Route cancel through the same typed preflight/result path instead of exiting inside thetry. Based on learnings,recordHistorymust be wrapped in try/catch at the outermost layer, and the--jsonenvelope is a public contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ccaudit/src/cli/commands/restore.ts` around lines 377 - 380, The code currently calls process.exit(0) when openRestorePicker returns outcome.kind === 'cancelled', which bypasses the shared JSON/history path; remove the direct exit and instead convert the cancelled outcome into the same typed preflight/result flow that other outcomes use so cancellation is handled by the existing post-dispatch logic (i.e., pass a cancellation result object into the same branch that calls safeRecordHistory()/recordHistory); ensure recordHistory is invoked inside the outermost try/catch (wrap the caller that handles outcomes so recordHistory errors are caught) and preserve the --json envelope on stdout even for cancellations so the public JSON contract remains intact.
🧹 Nitpick comments (23)
apps/ccaudit/scripts/bundle-smoke-test.mjs (1)
29-33: Optional: surface signal/stdout on failure for easier diagnosis.When
spawnSynchits the 10s timeout,result.statusisnullandresult.signalis set (e.g.SIGTERM); the failure message will readexited with code nullandresult.stdout(which may contain partial CLI output explaining the hang) is dropped. Consider including both for faster triage:♻️ Proposed refinement
if (result.status !== 0) { - console.error(`[bundle-smoke] FAIL: dist/index.js --help exited with code ${result.status}`); + const reason = result.signal + ? `terminated by signal ${result.signal}` + : `exited with code ${result.status}`; + console.error(`[bundle-smoke] FAIL: dist/index.js --help ${reason}`); + if (result.stdout) console.error(result.stdout); if (result.stderr) console.error(result.stderr); process.exit(1); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ccaudit/scripts/bundle-smoke-test.mjs` around lines 29 - 33, The failure branch currently only logs result.status and stderr; update it to also surface result.signal when status is null and always print result.stdout (alongside stderr) to aid diagnosis of timeouts/hangs from the spawnSync call that produces the result object. Locate the block referencing result.status (the spawnSync result variable) and change the error message to include either "exited with code <status>" or "killed by <signal>" depending on whether result.status is null, and ensure console.error prints result.stdout and result.stderr (if present) before calling process.exit(1).apps/ccaudit/src/__tests__/fixtures/ghost-500-items.ts (2)
47-47: Path pluralization is unsafe for non-default categories.
/fake/${category}s/${name}.mdonly yields a sensible plural foragent → agents,skill → skills,command → commands. Categories likememoryormcpwould render asmemorys/mcps, which is fine for purely synthetic fixtures but will mislead anyone grepping fixture output by realistic path shape. Since the only consumer today (pagination-500.test.ts) uses the'agent'default this is not breaking, but consider mapping category → directory explicitly if non-agent fixtures get added.♻️ Suggested mapping
- path: `/fake/${category}s/${name}.md`, + path: `/fake/${categoryDir(category)}/${name}.md`,function categoryDir(c: TokenCostResult['item']['category']): string { switch (c) { case 'agent': return 'agents'; case 'skill': return 'skills'; case 'command': return 'commands'; case 'mcp': return 'mcp'; case 'memory': return 'memory'; default: return `${c}s`; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ccaudit/src/__tests__/fixtures/ghost-500-items.ts` at line 47, The path pluralization is unsafe; create a helper function categoryDir (or similar) that maps TokenCostResult item categories to their correct directory names (e.g., 'agent'→'agents', 'skill'→'skills', 'command'→'commands', 'mcp'→'mcp', 'memory'→'memory', default → `${c}s`) and replace the inline `/fake/${category}s/${name}.md` with `/fake/${categoryDir(category)}/${name}.md` in the ghost-500-items fixture so paths are correct for non-default categories.
7-9: Doc nit: comment says001..500but factory supports up to 999.The block comment on lines 7–9 states names go
agent-001..agent-500, butcountis validated against the range1..999(line 32). Consider tightening the comment to reflect the 1–999 range so future readers don't think 500 is a hard ceiling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ccaudit/src/__tests__/fixtures/ghost-500-items.ts` around lines 7 - 9, Update the block comment that currently says "agent-001..agent-500" to reflect the actual supported range (1..999) enforced by the count validation; mention tokens decrementing from 1000 and the deterministic mtime behavior as before. Locate the comment near the factory that uses the count variable (the same scope that validates count against 1..999) and change the example to "agent-001..agent-999" or otherwise state the 1–999 range so readers don’t assume 500 is the limit.apps/ccaudit/src/__tests__/pagination-500.test.ts (2)
90-103: Test name overstates what's verified; consider adding a real clamp case.The header (line 17) advertises that
applyScroll"clamps cursor on heavy filter narrowing", but this test passesrowsLen: 500tofilterOffso no clamping branch is exercised — it only confirms a no-op round-trip. A small additional assertion withrowsLensmaller than the saved cursor (e.g.applyScroll({cursor: 300, savedCursorPreFilter: 300}, {type: 'filterOff', rowsLen: 50})→ cursor≤ 49) would actually cover the clamp path.Also, the dynamic
await import(...)on line 96 has no functional reason here (no circular-import or lazy-load constraint mentioned); a top-levelimport { applyScroll } from '...'would be simpler and let the test stay synchronous.♻️ Suggested additions
- it('applyScroll reducer round-trips cursor across filter on/off', async () => { - const { applyScroll } = await import('../../../../packages/terminal/src/tui/_viewport.ts'); + it('applyScroll reducer round-trips cursor across filter on/off and clamps on narrowing', () => { const s0 = { cursor: 300, savedCursorPreFilter: null as number | null }; const s1 = applyScroll(s0, { type: 'filterOn' }); const s2 = applyScroll(s1, { type: 'filterQueryChange' }); const s3 = applyScroll(s2, { type: 'filterOff', rowsLen: 500 }); expect(s3.cursor).toBe(300); expect(s3.savedCursorPreFilter).toBeNull(); + + // Clamp branch: rowsLen shrank below saved cursor. + const sClamp = applyScroll(s2, { type: 'filterOff', rowsLen: 50 }); + expect(sClamp.cursor).toBeLessThanOrEqual(49); + expect(sClamp.savedCursorPreFilter).toBeNull(); });Plus add
import { applyScroll } from '../../../../packages/terminal/src/tui/_viewport.ts';at the top.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ccaudit/src/__tests__/pagination-500.test.ts` around lines 90 - 103, The test name claims clamping is verified but currently only exercises a no-op round-trip because applyScroll is dynamically imported and filterOff is called with rowsLen: 500; change the test to import { applyScroll } top-level rather than using await import(...) and add a second assertion that exercises the clamp branch by calling applyScroll({ cursor: 300, savedCursorPreFilter: 300 }, { type: 'filterOff', rowsLen: 50 }) (or any rowsLen < savedCursorPreFilter) and asserting the returned cursor is <= rowsLen - 1 and savedCursorPreFilter is null; this covers the heavy-filter-narrowing clamp path and keeps the original round-trip assertion.
63-88: This test re-implements the handler it claims to verify.Lines 70–84 manually set
filterMode,savedCursorPreFilter, then manually clamp and restore — exactly the operations the production/andEschandlers perform. As written, the test asserts that the test code itself is consistent, not thatTabbedGhostPickerpreserves cursor across filter on/off. The comment on lines 74–76 acknowledges this ("We DON'T actually run the filter here").If the goal is to lock the reducer contract, the next test (lines 90–103) already does that via
applyScroll. Consider either (a) driving the picker through its real filter entry/exit methods (e.g. the keystroke or method that the/handler eventually calls) so the invariant is enforced on the production code, or (b) deleting this case as redundant with the reducer test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ccaudit/src/__tests__/pagination-500.test.ts` around lines 63 - 88, This test manually replicates the production '/' and 'Esc' handlers (setting picker.filterMode, tab.savedCursorPreFilter, clamping and restoring) so it only verifies the test code itself; either remove this redundant test or change it to exercise the actual production handlers: instead of directly mutating picker.filterMode and tab.cursor, drive the picker via the real entry/exit methods (e.g., call the keystroke handler or public methods the UI uses such as picker.handleKey('/') and picker.handleKey('Escape') or picker.enterFilterMode()/picker.exitFilterMode() if those exist) and then assert that tab.savedCursorPreFilter and tab.cursor are preserved/restored; keep references to makePicker, picker, tab.savedCursorPreFilter, and applyScroll when updating or removing the test to avoid duplication with the reducer test.apps/ccaudit/src/__tests__/tabbed-picker-50-item-fixture.test.ts (2)
155-175: Hard-coded settle delays may be flaky on slow CI runners.The 200–300ms sleeps between keystrokes are pragmatic for a PTY harness, but on a loaded CI box the picker may not have repainted before the next key arrives, which would manifest as intermittent failures of the
'selected across all tabs'marker check or as keys consumed in the wrong tab/mode. If you start seeing intermittent failures, consider replacing eachsetTimeoutwith a smallwaitForMarkerthat pins on the expected post-keystroke state (e.g., help overlay visible/hidden, filter input echoed, tab title changed) instead of wall-clock time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ccaudit/src/__tests__/tabbed-picker-50-item-fixture.test.ts` around lines 155 - 175, Replace the hard-coded setTimeout-based delays after sendKeys(spawned.child, ...) calls with targeted waits that assert the UI reached the expected post-keystroke state (e.g., visible/hidden help overlay, filter input echoed, tab title changed) using a helper like waitForMarker; update the sequence around sendKeys(...) and the following new Promise(...) calls so each keystroke is followed by waitForMarker(markerStringOrPredicate) instead of setTimeout, referencing the existing sendKeys and spawned.child usage and adding/using waitForMarker in this test to pin on the expected render before continuing.
183-186: Optional: prefertoContainfor the exit-code assertion.
expect([...].includes(x)).toBe(true)collapses to a boolean and loses the diff in Vitest's output.expect([0, 130, null]).toContain(result.exitCode)produces a clearer failure message that names the actual value, and you can drop the manual stderr/stdout tail in the message string (Vitest will print the assertion itself).♻️ Proposed refactor
- // Exit contract. - expect( - [0, 130, null].includes(result.exitCode), - `unexpected exit code ${result.exitCode}\nstderr tail:\n${stderrBuf.slice(-500)}\nstdout tail:\n${stdoutBuf.slice(-500)}`, - ).toBe(true); + // Exit contract. Tails are appended to the assertion message so that + // CI failures still surface the picker's last output without losing the + // diff that `toContain` provides. + try { + expect([0, 130, null]).toContain(result.exitCode); + } catch (err) { + (err as Error).message += + `\nstderr tail:\n${stderrBuf.slice(-500)}\nstdout tail:\n${stdoutBuf.slice(-500)}`; + throw err; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ccaudit/src/__tests__/tabbed-picker-50-item-fixture.test.ts` around lines 183 - 186, Replace the boolean-inclusion assertion that uses expect([0, 130, null].includes(result.exitCode)).toBe(true) with Vitest's clearer form expect([0, 130, null]).toContain(result.exitCode) in the test (referencing result.exitCode), and remove the custom failure message (the manual stderrBuf/ stdoutBuf tail string) since Vitest will show the actual value and diff automatically.apps/ccaudit/src/__tests__/tabbed-picker-help-overlay.test.ts (3)
105-120: Space-swallow assertion is correct but indirect — consider an explicit check.The "Space was swallowed while overlay was open" guarantee is verified transitively: after closing via
?, the footer slice must still contain0 of 3 selected across all tabs. That works (a processed Space would flip the counter to1 of 3), but the failure message will point at "footer not restored" rather than "Space leaked into the picker", which is harder to triage.Consider also asserting that the overlay text (
(Press ? or Esc to close)or one of the visible headings) is still present in the slice between sending Space and sending the closing?, to make the swallow behavior explicit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ccaudit/src/__tests__/tabbed-picker-help-overlay.test.ts` around lines 105 - 120, Add an explicit assertion that the overlay remained open after the Space key was sent instead of only relying on the footer after the `?` toggle; specifically, after the sendKeys(spawned.child, [' ']) and the following delay, capture the intermediate stdoutBuf slice (using stripAnsi on the bytes appended since before the Space) and assert it contains a known overlay string such as "(Press ? or Esc to close)" or one of the overlay headings, so the test checks the Space-swallow behavior directly before you later sendKeys(spawned.child, ['?']) and verify footer restoration.
138-138: Minor: includeresult.exitCodein the assertion message.Both tests end with
expect([0, 130, null].includes(result.exitCode)).toBe(true);(also at L185). On failure, vitest only reportsexpected false to be true, which forces a re-run with logging to discover the actual exit code. A small wrapper makes the diagnostic self-contained.♻️ Proposed tweak
- expect([0, 130, null].includes(result.exitCode)).toBe(true); + expect( + [0, 130, null].includes(result.exitCode), + `expected exitCode ∈ {0,130,null} after Ctrl-C, got ${String(result.exitCode)}`, + ).toBe(true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ccaudit/src/__tests__/tabbed-picker-help-overlay.test.ts` at line 138, Replace the brittle assertion pattern expect([0, 130, null].includes(result.exitCode)).toBe(true) with an assertion that reports the actual value, e.g. expect([0, 130, null]).toContain(result.exitCode) (update both occurrences around the test endings), so failures will show the received exitCode in the test output.
39-46: Consider usingstrip-ansi(available as transitive dependency) or extending the regex to cover additional escape sequences.The local
stripAnsiimplementation handles cursor visibility (?25l/h), a fixed set of CSI final bytes (mGKHFABCD), and JST sequences, but does not cover bracketed-paste mode (?2004h/l), other DEC private modes, OSC sequences (\x1b]…\x07/…\x1b\\), orr/n/csequences. If the renderer emits any of these, residual bytes can land mid-token in assertion checks liketoContain('Selection'), causing flaky failures.The
strip-ansi@6.0.1package is available as a transitive dependency (via@clack/prompts→string-width), making it a practical choice. Low priority since the current regex evidently covers what the picker emits today.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ccaudit/src/__tests__/tabbed-picker-help-overlay.test.ts` around lines 39 - 46, Replace the fragile local stripAnsi function in the test with the well-tested strip-ansi implementation: remove the stripAnsi function and import stripAnsi from the strip-ansi package (used transitively) in the test file so all OSC, DEC private modes (e.g. ?2004h/l), bracketed paste, and other escape sequences are stripped consistently before assertions; if you prefer to keep a local solution instead, expand the regex in the stripAnsi function to also match OSC sequences (\x1b] ... \x07 / ... \x1b\\), additional DEC private modes like ?2004[h|l], and remaining CSI/SGR final bytes to avoid stray bytes in test tokens.packages/internal/src/remediation/atomic-write.ts (1)
65-129: Optional: factor the shared tmp+rename body out ofatomicWriteJson/atomicWriteText.The two functions are byte-identical except for the body production (
JSON.stringify(value, null, 2)vs the caller-suppliedtext). Extracting a privateatomicWriteRaw(targetPath, body, opts)and having both publics call it would keep the retry/unlink/tmp-suffix invariants in one place — important when SC-9's Windows EPERM retry semantics evolve.♻️ Sketch
+async function atomicWriteRaw( + targetPath: string, + body: string, + options: AtomicWriteOptions, +): Promise<void> { + const opts = { ...DEFAULTS, ...options }; + const dir = path.dirname(targetPath); + const rand = Math.random().toString(36).slice(2, 10).padEnd(8, '0'); + const tmpPath = `${targetPath}.tmp-${process.pid}-${rand}`; + await mkdir(dir, { recursive: true, mode: opts.dirMode }); + try { + await writeFile(tmpPath, body, { mode: opts.mode, encoding: 'utf8' }); + await renameWithRetry(tmpPath, targetPath, opts); + } catch (err) { + try { await unlink(tmpPath); } catch { /* swallow */ } + throw err; + } +} + export async function atomicWriteJson<T>(...): Promise<void> { - // inline body + await atomicWriteRaw(targetPath, JSON.stringify(value, null, 2), options); } export async function atomicWriteText(...): Promise<void> { - // inline body + await atomicWriteRaw(targetPath, text, options); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/internal/src/remediation/atomic-write.ts` around lines 65 - 129, The two functions atomicWriteJson and atomicWriteText duplicate the tmp-file creation, mkdir, write/rename with retry and cleanup logic; extract that shared behavior into a private helper atomicWriteRaw(targetPath: string, body: string, options?: AtomicWriteOptions): Promise<void> that performs rand/tmpPath creation, mkdir(dir,..), writeFile(tmpPath,...), renameWithRetry(tmpPath,targetPath,opts) and the unlink-on-error cleanup, then have atomicWriteJson call atomicWriteRaw(targetPath, JSON.stringify(value, null, 2), options) and atomicWriteText call atomicWriteRaw(targetPath, text, options) so the EPERM retry/unlink invariants live in one place.packages/internal/src/scanner/_present-path.ts (1)
30-46: Confirm passthrough mutates non-home Windows paths to forward slashes.L45 returns
abs(the forward-slashed copy), notabsPath. So a Windows-style input likeC:\\some\\system\\path.jsonthat matches neitherhomeDirnorprojectRootis returned asC:/some/system/path.json— not the original. The module doc at L17 ("Forward slashes only") suggests this is intentional, but the JSDoc at L28 says "absPath unchanged" which is now slightly inaccurate. Either:
- update the JSDoc to read "the forward-slash-normalized form of
absPath", or- pass through
absPathverbatim when no rule matches.Most callers will already be feeding pre-normalized paths in, so this is a doc/behavior alignment nit rather than a bug.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/internal/src/scanner/_present-path.ts` around lines 30 - 46, The JSDoc for presentPath is inaccurate: the function normalizes backslashes to forward slashes and returns the normalized string even in the passthrough case; update the JSDoc for presentPath to state it returns the forward-slash-normalized form of absPath (e.g., "Returns the forward-slash-normalized form of absPath, with projectRoot or homeDir compressed when applicable") so the documentation matches the existing behavior (alternatively, if you prefer original raw behavior, change the final return to return absPath verbatim instead of abs).apps/ccaudit/src/__tests__/restore-preflight-json.test.ts (1)
66-138: Repeated PATH-override env construction can be hoisted.Each of the five JSON-envelope tests (and the plain-stderr test, and the empty-archive test) recomputes
PATH: \${path.join(tmpHome, 'bin')}:${process.env.PATH ?? ''}`. Pulling this into a small helper (e.g.,withFakePsPath(tmpHome, extra?)) would shrink each test by a line, reduce copy-paste drift across the suite, and make it easier to keep parity with howrunCcauditGhostwiresPATHin_test-helpers.ts`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ccaudit/src/__tests__/restore-preflight-json.test.ts` around lines 66 - 138, The tests in restore-preflight-json.test.ts repeatedly build the same PATH override for runCcauditCli calls; introduce a small helper (e.g., withFakePsPath(tmpHome, extraEnv?)) that returns an env object with PATH: `${path.join(tmpHome, 'bin')}:${process.env.PATH ?? ''}` merged with any extra vars, add it to _test-helpers.ts (next to runCcauditGhost wiring) and replace the inline env objects in the tests (the runCcauditCli invocations in this file) with calls to withFakePsPath(tmpHome) or withFakePsPath(tmpHome, { CCAUDIT_FORCE_TTY: '1', ... }) to reduce duplication.apps/ccaudit/src/__tests__/fixtures/manual-qa-followups.ts (1)
391-404:stagePaginationFixturewrites 550 files serially — non-trivial test setup latency.Each iteration awaits
writeFilethenutimes, serialized. 550 × 2 = 1100 sequential FS round-trips. Batching withPromise.all(or chunked) cuts this dramatically and keeps pagination tests within reasonable budgets on slower CI runners.- for (let i = 1; i <= count; i++) { - const name = `agent-${String(i).padStart(3, '0')}`; - const filePath = path.join(tmpHome, '.claude', 'agents', `${name}.md`); - await writeFile(filePath, `# ${name} ghost\n`, 'utf8'); - await utimes(filePath, OLD_DATE, OLD_DATE); - } + await Promise.all( + Array.from({ length: count }, async (_, idx) => { + const name = `agent-${String(idx + 1).padStart(3, '0')}`; + const filePath = path.join(tmpHome, '.claude', 'agents', `${name}.md`); + await writeFile(filePath, `# ${name} ghost\n`, 'utf8'); + await utimes(filePath, OLD_DATE, OLD_DATE); + }), + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ccaudit/src/__tests__/fixtures/manual-qa-followups.ts` around lines 391 - 404, stagePaginationFixture currently performs writeFile and utimes sequentially per file which causes 1100 serial FS round-trips for count=550; change it to run file writes/utimes in parallel by creating per-file async tasks (each task does writeFile then utimes) and collecting them into batches, then await Promise.all for each batch to limit concurrency (e.g., CHUNK_SIZE 50) so the functions (stagePaginationFixture, the per-file task) still ensure utimes runs after writeFile but many files are processed concurrently to drastically reduce test setup latency.apps/ccaudit/src/__tests__/tabbed-picker-force-partial-banner.test.ts (2)
113-120: Fixed 400ms wait before reading the post-aframe is racy.
sendKeys(['a'])followed bysetTimeout(400)then assertion is timing-dependent. On a slow CI (or under coverage instrumentation) the picker may not have repainted the2 of 2 selected across all tabsline within 400ms, producing a flaky failure that won't reproduce locally. Prefer reusingwaitForMarkeragainst the post-press buffer slice — it already polls until the predicate matches or the child exits.- const before = out.length; - await sendKeys(spawned.child, ['a']); - await new Promise((r) => setTimeout(r, 400)); - const afterA = stripAnsi(out.slice(before)); - expect( - /2 of 2 selected across all tabs/.test(afterA), - `expected '2 of 2 selected' under --force-partial; got:\n${afterA.slice(-1500)}`, - ).toBe(true); + const before = out.length; + await sendKeys(spawned.child, ['a']); + await waitForMarker( + () => out.slice(before), + () => spawned.child.exitCode !== null, + '2 of 2 selected across all tabs', + );Same pattern recurs in Scenario C (line 196-200) and in
tabbed-picker-protection.test.ts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ccaudit/src/__tests__/tabbed-picker-force-partial-banner.test.ts` around lines 113 - 120, The test currently uses sendKeys(spawned.child, ['a']) then a fixed await new Promise(setTimeout 400) and immediate slice+regex, which is racy; replace the fixed timeout by awaiting the existing waitForMarker helper to poll the post-press buffer until the expected text appears. Concretely, after sendKeys(spawned.child, ['a']) remove the setTimeout and the immediate const afterA = stripAnsi(out.slice(before)); expect(...) and instead call await waitForMarker(() => /2 of 2 selected across all tabs/.test(stripAnsi(out.slice(before)))) (or the project’s waitForMarker API variant that accepts the buffer slice/predicate), then perform the assertion using the matched buffer; apply the same change to the Scenario C occurrence and the similar pattern in tabbed-picker-protection.test.ts, referencing sendKeys, waitForMarker, stripAnsi, spawned.child and out to locate each spot.
38-44:stripAnsiis duplicated across test files; consolidate to a shared test helper.The test files in
apps/ccaudit/src/__tests__/each define their ownstripAnsifunction—at least 8 files (tabbed-picker-force-partial-banner.test.ts, tabbed-picker-overflow.test.ts, tabbed-picker-protection.test.ts, tabbed-picker-framework-group.test.ts, tabbed-picker-sort-cycle.test.ts, tabbed-picker-live-counter.test.ts, tabbed-picker-mcp-multi-config.test.ts, and tabbed-picker-sort-cycle.test.ts). While a shared helper exists inpackages/terminal/src/utils/table-utils.ts, it only strips SGR codes and is not imported into tests.A shared test helper (e.g., in
apps/ccaudit/src/__tests__/_test-helpers.ts) would reduce duplication. The current regex variants are adequate for the sequences actually emitted by the TTY under test (cursor show/hide, SGR colors, cursor movement, and erase), but a consolidated approach ensures consistency and simplifies maintenance.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ccaudit/src/__tests__/tabbed-picker-force-partial-banner.test.ts` around lines 38 - 44, Multiple tests duplicate the stripAnsi function; extract it into a shared test helper (e.g., create an exported stripAnsi in a new _test-helpers.ts) and update each test (those that currently define stripAnsi such as tabbed-picker-force-partial-banner.test.ts, tabbed-picker-overflow.test.ts, tabbed-picker-protection.test.ts, etc.) to import and use the shared stripAnsi instead of their local copy; keep the current regex behavior (cursor show/hide, SGR, movement, erase) or align with the existing strip logic in table-utils.ts if preferred, and remove the duplicated stripAnsi definitions from all test files.packages/internal/src/remediation/checkpoint.ts (1)
595-647: Minor: fixture is re-read three times.Each golden-fixture test reloads
ghost-hash-golden.jsonviareadFile. Hoisting the load (e.g., into abeforeAllthat populates a closure variable) avoids three identical disk reads and centralizes the JSON parse/cast. Not blocking — three reads on a tiny fixture is fine — but it would tighten the suite.Also worth considering: Test 2 silently passes if the fixture happens to contain no
memory-category items (the.mapwould no-op andmutatedwould equalfixture.inputmodulo identity). Aexpect(mutatedAtLeastOne).toBe(true)guard inside the map would harden the test against future fixture changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/internal/src/remediation/checkpoint.ts` around lines 595 - 647, Hoist the repeated read/parse of ./__fixtures__/ghost-hash-golden.json into a beforeAll that sets a closure variable (e.g. fixture: { input: TokenCostResult[]; expectedHash: string }) and reuse it in the three it() blocks instead of calling readFile each time; keep using computeGhostHash and the deterministic StatFn (StatFn, MCP_MTIME) as now but create them once per test or in beforeEach as needed. Additionally, in the "mutating any mtime" test, assert that you actually mutated at least one item (e.g. check fixture.input.some(r => r.item.category === 'memory') or set a mutatedAtLeastOne flag during the map and add expect(mutatedAtLeastOne).toBe(true)) before computing mutatedHash to avoid a silent no-op if the fixture contains no memory items. Ensure references to computeGhostHash, ghost-hash-golden.json, TokenCostResult, and StatFn remain unchanged so the diff is localized to test setup and the mutation check.packages/internal/src/remediation/purge.ts (1)
190-207: Nit: thevoid item;workaround in the dry-run reclaim loop is unnecessary.The body doesn't read anything from
item, so the cleanest expression of the intent is a direct assignment:Suggested simplification
if (opts.dryRun) { // Dry-run: compute counters from the plan; NO deps called at all. - for (const item of plan.reclaim) { - summary.reclaimedCount += 1; - void item; // explicit noop — satisfy no-unused-vars without mutating item - } + summary.reclaimedCount = plan.reclaim.length; for (const item of plan.drop) { summary.purgedCount += 1; if (item.reason === 'source_occupied') summary.skippedOccupiedCount += 1; else summary.staleFilteredCount += 1; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/internal/src/remediation/purge.ts` around lines 190 - 207, Remove the unnecessary noop `void item;` in the dry-run reclaim loop inside the opts.dryRun branch: the loop over plan.reclaim should simply increment summary.reclaimedCount for each item without referencing item; update the for (const item of plan.reclaim) { summary.reclaimedCount += 1; } body accordingly (no other changes to the surrounding Result.succeed return or the drop loop).apps/ccaudit/src/__tests__/restore-interactive-roundtrip.test.ts (1)
148-152: Optional: the 300 ms fixed delay between picker submit and confirm-prompt toggle is a flake risk on slow CI runners.The assertion plan relies on the clack confirm prompt being mounted before the
ArrowLeftkeystroke arrives. On a hot loaded CI box (or under sandbox/cgroup throttling) 300 ms can be tight, and a missed keystroke would leave the prompt at itsinitialValue: falseand silently flip the round-trip to a dry-fail. Consider either bumping to ~600 ms or extending the helper module with awaitForConfirmPrompt(child)that polls stdout for a clack-confirm marker the waywaitForPickerdoes. Not a blocker for v1.5.0 — flagging because the test gates INV-S3.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ccaudit/src/__tests__/restore-interactive-roundtrip.test.ts` around lines 148 - 152, The fixed 300ms grace delay before sending ArrowLeft to the clack confirm prompt is flaky on slow CI; update the test in restore-interactive-roundtrip.test.ts to wait deterministically for the confirm prompt instead of (or in addition to) a short timeout: either increase the sleep to ~600ms or (preferred) add a waitForConfirmPrompt(child) helper that polls the spawned.child stdout for the same clack-confirm marker pattern used by waitForPicker, then call that before invoking sendKeys(spawned.child, ['\x1b[D'], 100) and sendKeys(spawned.child, ['\r'], 100) so the keystrokes arrive only after the confirm prompt is mounted.packages/internal/src/remediation/change-plan.ts (1)
102-157: Optional: extract arecomputeCountshelper to DRY up count recomputation.The counts recomputation block is duplicated verbatim between
buildChangePlan(lines 102-108) andfilterChangePlan(lines 146-152). When a future category is added, both sites must change in lockstep — a small extract avoids the drift risk.♻️ Proposed extract
+function computeCounts( + archive: ChangePlanItem[], + disable: ChangePlanItem[], + flag: ChangePlanItem[], +): ChangePlan['counts'] { + return { + agents: archive.filter((i) => i.category === 'agent').length, + skills: archive.filter((i) => i.category === 'skill').length, + mcp: disable.length, + memory: flag.length, + commands: archive.filter((i) => i.category === 'command').length, + }; +}Then call it from both
buildChangePlanandfilterChangePlan.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/internal/src/remediation/change-plan.ts` around lines 102 - 157, The counts object is duplicated in buildChangePlan and filterChangePlan; extract a helper (e.g., recomputeCounts) that accepts the archive/disable/flag arrays or a ChangePlan-like partial and returns the counts shape used (agents, skills, mcp, memory, commands), then replace the inline count blocks in buildChangePlan and filterChangePlan to call recomputeCounts and assign its result to counts before recomputing savings with calculateDryRunSavings; reference the functions buildChangePlan, filterChangePlan, calculateDryRunSavings, and the counts property to locate where to replace logic.packages/internal/src/remediation/index.ts (1)
146-148: Underscore-prefixed module exposed through the public barrel.
_archive-move.tsuses the_prefix that conventionally signals "private/internal", yet it's now re-exported from the package barrel (moveArchiveToSource+ types). Either the underscore is misleading (the helper is actually a shared public primitive used by both reclaim and purge) or the re-export should be reconsidered. Renaming toarchive-move.tswould make the intent unambiguous.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/internal/src/remediation/index.ts` around lines 146 - 148, The file and exports currently re-export a module named _archive-move.ts (exporting moveArchiveToSource and types ArchiveMoveDeps, MoveArchiveInput, MoveArchiveFailure) which uses a leading underscore implying private but is publicly surfaced; rename the module to archive-move.ts and update the export lines to import/export from './archive-move.ts' (and rename the file on disk) so the public API (moveArchiveToSource and the three types) no longer references a misleading underscore and clearly indicates a public/shared primitive.packages/internal/src/remediation/bust.ts (2)
686-750: Tolerate JSON whitespace between key and colon infindTopLevelMcpServersBlock.
text.startsWith('"mcpServers":', i)and the per-mutationneedle = "${name}":both require zero whitespace between the closing quote and:. Valid JSON permits"mcpServers" : { ... }, so any pretty-printer that emits a space here triggersnulland silently downgrades toatomicWriteJson(loses INV-S1 byte preservation). SincedisableMcpTransactionalrecovers via the JSON fallback this is not a correctness bug, just a degraded path users won't notice.♻️ Sketch: skip whitespace before the colon
- if (depth === 1 && text.startsWith('"mcpServers":', i)) { - let j = i + '"mcpServers":'.length; + if (depth === 1 && text.startsWith('"mcpServers"', i)) { + let j = i + '"mcpServers"'.length; + while (j < text.length && /\s/.test(text[j]!)) j++; + if (text[j] !== ':') { + inString = true; + continue; + } + j++; while (j < text.length && /\s/.test(text[j]!)) j++;The same relaxation can apply to
needlelookup at line 779 (e.g., search via a regex bounded to[block.start, block.end)).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/internal/src/remediation/bust.ts` around lines 686 - 750, The findTopLevelMcpServersBlock function currently uses text.startsWith('"mcpServers":', i) which fails when JSON has whitespace between the closing quote and colon; change the logic to first match the quoted key '"mcpServers"' at index i, then advance past any intervening whitespace and assert the next non-space char is ':' before continuing to locate the object (update the same pattern inside the function where it computes j). Also update the per-mutation needle construction/lookup (the code that builds/checks `${name}":`) to allow optional whitespace between the closing quote and ':' — either by scanning/skipping whitespace after the quoted name or by using a regex bounded to the block range — so keys like `"mcpServers" : {` are accepted.
949-1061: Mixed-scope MCP files always fall back to full JSON rewrite.When a single config file produces both a project-scope item (sets
hasSurgicalFallback = true) and a global/flat item, the global items' surgical mutations are discarded and the entire file is rewritten viaatomicWriteJson, dropping byte-preservation for keys that could otherwise be patched surgically. This matches the JSDoc (project-scope is unsupported), but it means INV-S1 is silently coarser than necessary in mixed busts. Not a bug — flagging for awareness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/internal/src/remediation/bust.ts` around lines 949 - 1061, Mixed-scope files currently drop surgical mutations when hasSurgicalFallback is true; change the write logic so surgicalMutations are attempted via patchMcpConfigText regardless of hasSurgicalFallback and, if patchMcpConfigText returns non-null, apply its result to the final write path: (a) if there are only surgical mutations (no project-scope changes), write the patched text with deps.atomicWriteText as before; (b) if project-scope changes exist, parse or otherwise incorporate the patched result into the config object (so the surgical key moves are reflected) before calling deps.atomicWriteJson, ensuring surgicalMutations (variables surgicalMutations, hasSurgicalFallback) are preserved while still performing an atomic JSON rewrite for project-scope edits; use patchMcpConfigText, deps.atomicWriteText, deps.atomicWriteJson and the config object to implement this.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e172ff65-62e8-4a42-aa4e-3085bcbf9a70
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (124)
CHANGELOG.mdCLAUDE.mdREADME.mdapps/ccaudit/package.jsonapps/ccaudit/scripts/bundle-baseline-03.2.txtapps/ccaudit/scripts/bundle-baseline-phase-05.txtapps/ccaudit/scripts/bundle-baseline-phase-06.txtapps/ccaudit/scripts/bundle-baseline-phase-09.txtapps/ccaudit/scripts/bundle-baseline.txtapps/ccaudit/scripts/bundle-size-check.mjsapps/ccaudit/scripts/bundle-smoke-test.mjsapps/ccaudit/src/__tests__/__fixtures__/regime-post-2-1-117/.claude.jsonapps/ccaudit/src/__tests__/__fixtures__/regime-post-2-1-117/.claude/settings.jsonapps/ccaudit/src/__tests__/__fixtures__/regime-pre-2-1-116/.claude.jsonapps/ccaudit/src/__tests__/__fixtures__/regime-pre-2-1-116/.claude/settings.jsonapps/ccaudit/src/__tests__/__fixtures__/restore-interactive/bust-2026-04-17T10-00-00-000Z-aaaa.jsonlapps/ccaudit/src/__tests__/__fixtures__/restore-interactive/bust-2026-04-18T10-00-00-000Z-bbbb.jsonlapps/ccaudit/src/__tests__/_test-helpers.tsapps/ccaudit/src/__tests__/command-archival.test.tsapps/ccaudit/src/__tests__/empty-inventory.test.tsapps/ccaudit/src/__tests__/fixtures/ghost-500-items.tsapps/ccaudit/src/__tests__/fixtures/manual-qa-followups.mdapps/ccaudit/src/__tests__/fixtures/manual-qa-followups.tsapps/ccaudit/src/__tests__/fixtures/tmux-e2e.mdapps/ccaudit/src/__tests__/fixtures/tmux-e2e.tsapps/ccaudit/src/__tests__/ghost-select-ids.test.tsapps/ccaudit/src/__tests__/help-output.test.tsapps/ccaudit/src/__tests__/interactive-smoke.test.tsapps/ccaudit/src/__tests__/no-interactive-env.test.tsapps/ccaudit/src/__tests__/pagination-500.test.tsapps/ccaudit/src/__tests__/preflight-retry.test.tsapps/ccaudit/src/__tests__/purge-archive.test.tsapps/ccaudit/src/__tests__/regime-detection-cc-2-1-117-compat.test.tsapps/ccaudit/src/__tests__/restore-after-purge.test.tsapps/ccaudit/src/__tests__/restore-all-matching.test.tsapps/ccaudit/src/__tests__/restore-command.test.tsapps/ccaudit/src/__tests__/restore-corrupt-manifest.test.tsapps/ccaudit/src/__tests__/restore-flag-mutual-exclusion.test.tsapps/ccaudit/src/__tests__/restore-interactive-memory-roundtrip.test.tsapps/ccaudit/src/__tests__/restore-interactive-roundtrip.test.tsapps/ccaudit/src/__tests__/restore-interactive-source-exists.test.tsapps/ccaudit/src/__tests__/restore-json-envelope.test.tsapps/ccaudit/src/__tests__/restore-list-skips-purge.test.tsapps/ccaudit/src/__tests__/restore-name-ambiguity.test.tsapps/ccaudit/src/__tests__/restore-preflight-json.test.tsapps/ccaudit/src/__tests__/restore-stale-filter.test.tsapps/ccaudit/src/__tests__/safety-invariants-framework.test.tsapps/ccaudit/src/__tests__/safety-invariants-mcp.test.tsapps/ccaudit/src/__tests__/safety-invariants-restore-roundtrip.test.tsapps/ccaudit/src/__tests__/safety-invariants-tui-abort.test.tsapps/ccaudit/src/__tests__/scanner-multi-config-mcp.test.tsapps/ccaudit/src/__tests__/tabbed-picker-50-item-fixture.test.tsapps/ccaudit/src/__tests__/tabbed-picker-bust-parity.test.tsapps/ccaudit/src/__tests__/tabbed-picker-filter.test.tsapps/ccaudit/src/__tests__/tabbed-picker-force-partial-banner.test.tsapps/ccaudit/src/__tests__/tabbed-picker-force-partial-overflow.test.tsapps/ccaudit/src/__tests__/tabbed-picker-framework-group.test.tsapps/ccaudit/src/__tests__/tabbed-picker-help-overlay.test.tsapps/ccaudit/src/__tests__/tabbed-picker-live-counter.test.tsapps/ccaudit/src/__tests__/tabbed-picker-mcp-multi-config.test.tsapps/ccaudit/src/__tests__/tabbed-picker-overflow.test.tsapps/ccaudit/src/__tests__/tabbed-picker-protection.test.tsapps/ccaudit/src/__tests__/tabbed-picker-sort-cycle.test.tsapps/ccaudit/src/__tests__/tabbed-picker-tab-nav-keys.test.tsapps/ccaudit/src/__tests__/tabbed-picker-terminal-too-short.test.tsapps/ccaudit/src/__tests__/tmux-e2e-manual-qa.test.tsapps/ccaudit/src/cli/_shared-args.tsapps/ccaudit/src/cli/commands/ghost.tsapps/ccaudit/src/cli/commands/inventory.tsapps/ccaudit/src/cli/commands/mcp.tsapps/ccaudit/src/cli/commands/purge-archive.tsapps/ccaudit/src/cli/commands/restore.tsapps/ccaudit/src/cli/commands/trend.tsapps/ccaudit/src/cli/index.tsdocs/JSON-SCHEMA.mdeslint.config.tspackage.jsonpackages/internal/src/cli/env.tspackages/internal/src/cli/index.tspackages/internal/src/index.tspackages/internal/src/remediation/__fixtures__/ghost-hash-golden.jsonpackages/internal/src/remediation/_archive-move.tspackages/internal/src/remediation/atomic-write.tspackages/internal/src/remediation/bust.tspackages/internal/src/remediation/change-plan.tspackages/internal/src/remediation/checkpoint.tspackages/internal/src/remediation/index.tspackages/internal/src/remediation/manifest.tspackages/internal/src/remediation/purge.tspackages/internal/src/remediation/reclaim.tspackages/internal/src/remediation/restore.tspackages/internal/src/remediation/savings.tspackages/internal/src/scanner/_config-refs.tspackages/internal/src/scanner/_present-path.tspackages/internal/src/scanner/annotate.tspackages/internal/src/scanner/index.tspackages/internal/src/scanner/scan-hooks.tspackages/internal/src/scanner/scan-mcp.tspackages/internal/src/scanner/scan-memory.tspackages/internal/src/scanner/types.tspackages/internal/src/token/format.tspackages/internal/src/token/index.tspackages/internal/src/types.tspackages/terminal/package.jsonpackages/terminal/src/index.tspackages/terminal/src/tables/change-plan.tspackages/terminal/src/tui/_filter-sort.tspackages/terminal/src/tui/_force-partial-banner.tspackages/terminal/src/tui/_glyph-capability.tspackages/terminal/src/tui/_glyphs.tspackages/terminal/src/tui/_help-overlay.tspackages/terminal/src/tui/_mcp-warning-render.tspackages/terminal/src/tui/_preflight-copy.tspackages/terminal/src/tui/_protection-render.tspackages/terminal/src/tui/_tab-bar.tspackages/terminal/src/tui/_tui-mode.tspackages/terminal/src/tui/_viewport.tspackages/terminal/src/tui/auto-open-prompt.tspackages/terminal/src/tui/confirmation.tspackages/terminal/src/tui/index.tspackages/terminal/src/tui/select-ghosts.tspackages/terminal/src/tui/select-restore.tspackages/terminal/src/tui/tabbed-picker.tspnpm-workspace.yaml
✅ Files skipped from review due to trivial changes (30)
- apps/ccaudit/scripts/bundle-baseline-phase-05.txt
- apps/ccaudit/scripts/bundle-baseline-03.2.txt
- apps/ccaudit/scripts/bundle-baseline-phase-06.txt
- apps/ccaudit/src/tests/fixtures/regime-pre-2-1-116/.claude.json
- apps/ccaudit/scripts/bundle-baseline.txt
- apps/ccaudit/scripts/bundle-baseline-phase-09.txt
- apps/ccaudit/src/tests/fixtures/regime-pre-2-1-116/.claude/settings.json
- packages/internal/src/cli/index.ts
- packages/terminal/src/index.ts
- packages/terminal/package.json
- packages/internal/src/scanner/scan-memory.ts
- apps/ccaudit/package.json
- packages/internal/src/remediation/fixtures/ghost-hash-golden.json
- apps/ccaudit/src/tests/fixtures/regime-post-2-1-117/.claude/settings.json
- apps/ccaudit/src/tests/fixtures/manual-qa-followups.md
- apps/ccaudit/src/tests/fixtures/restore-interactive/bust-2026-04-18T10-00-00-000Z-bbbb.jsonl
- apps/ccaudit/src/tests/fixtures/regime-post-2-1-117/.claude.json
- apps/ccaudit/src/cli/commands/trend.ts
- apps/ccaudit/src/tests/fixtures/restore-interactive/bust-2026-04-17T10-00-00-000Z-aaaa.jsonl
- apps/ccaudit/scripts/bundle-size-check.mjs
- packages/internal/src/scanner/index.ts
- apps/ccaudit/src/cli/commands/mcp.ts
- apps/ccaudit/src/tests/fixtures/tmux-e2e.md
- apps/ccaudit/src/tests/tabbed-picker-sort-cycle.test.ts
- package.json
- apps/ccaudit/src/tests/tabbed-picker-overflow.test.ts
- apps/ccaudit/src/tests/tabbed-picker-filter.test.ts
- packages/internal/src/scanner/annotate.ts
- apps/ccaudit/src/tests/safety-invariants-mcp.test.ts
- apps/ccaudit/src/cli/commands/ghost.ts
🚧 Files skipped from review as they are similar to previous changes (27)
- packages/internal/src/remediation/savings.ts
- packages/internal/src/token/index.ts
- eslint.config.ts
- apps/ccaudit/src/tests/restore-flag-mutual-exclusion.test.ts
- apps/ccaudit/src/tests/empty-inventory.test.ts
- apps/ccaudit/src/tests/restore-after-purge.test.ts
- apps/ccaudit/src/tests/restore-corrupt-manifest.test.ts
- apps/ccaudit/src/tests/tabbed-picker-force-partial-overflow.test.ts
- packages/internal/src/scanner/types.ts
- packages/internal/src/cli/env.ts
- apps/ccaudit/src/tests/regime-detection-cc-2-1-117-compat.test.ts
- apps/ccaudit/src/tests/restore-json-envelope.test.ts
- apps/ccaudit/src/tests/tabbed-picker-bust-parity.test.ts
- apps/ccaudit/src/tests/tabbed-picker-tab-nav-keys.test.ts
- packages/internal/src/scanner/_config-refs.ts
- apps/ccaudit/src/tests/interactive-smoke.test.ts
- packages/internal/src/token/format.ts
- apps/ccaudit/src/tests/restore-name-ambiguity.test.ts
- apps/ccaudit/src/tests/restore-stale-filter.test.ts
- apps/ccaudit/src/tests/tabbed-picker-framework-group.test.ts
- apps/ccaudit/src/tests/scanner-multi-config-mcp.test.ts
- apps/ccaudit/src/tests/restore-interactive-memory-roundtrip.test.ts
- apps/ccaudit/src/tests/preflight-retry.test.ts
- apps/ccaudit/src/tests/tmux-e2e-manual-qa.test.ts
- apps/ccaudit/src/tests/tabbed-picker-mcp-multi-config.test.ts
- apps/ccaudit/src/tests/_test-helpers.ts
- apps/ccaudit/src/tests/tabbed-picker-live-counter.test.ts
| it('empty archive under --interactive --json → JSON envelope on stdout, exit 0', async () => { | ||
| // Use a fresh home with no manifests: findManifestsForRestore returns [] | ||
| // → collectRestoreableItems returns [] → RestorePreflightError(0, ...). | ||
| const emptyHome = await makeTmpHome(); | ||
| try { | ||
| await mkdir(path.join(emptyHome, '.claude'), { recursive: true }); | ||
| await writeFile(path.join(emptyHome, '.claude.json'), '{}', 'utf8'); | ||
| await buildFakePs(emptyHome); | ||
|
|
||
| const r = await runCcauditCli(emptyHome, ['restore', '--interactive', '--json'], { | ||
| env: { | ||
| CCAUDIT_FORCE_TTY: '1', | ||
| PATH: `${path.join(emptyHome, 'bin')}:${process.env.PATH ?? ''}`, | ||
| }, | ||
| }); | ||
| expect(r.exitCode).toBe(0); | ||
| const env = parseEnvelope(r.stdout); | ||
| expect(env.meta.command).toBe('restore'); | ||
| expect(env.meta.exitCode).toBe(0); | ||
| expect(env.error).toContain('archive is empty'); | ||
| expect(r.stderr).toBe(''); | ||
| } finally { | ||
| await cleanupTmpHome(emptyHome); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look up the documented envelope shape for empty-archive / no-op cases.
fd -t f 'JSON-SCHEMA' -e md
rg -nP -C5 'archive is empty|empty archive|no-op|filtered_stale_count'
rg -nP --type=ts -C3 'RestorePreflightError\b'Repository: fabio-dee/ccaudit
Length of output: 50375
🏁 Script executed:
head -200 docs/JSON-SCHEMA.mdRepository: fabio-dee/ccaudit
Length of output: 11490
🏁 Script executed:
grep -nA 10 -B 2 "error\|preflight\|status" docs/JSON-SCHEMA.md | head -100Repository: fabio-dee/ccaudit
Length of output: 6487
🏁 Script executed:
rg -nA 15 "restore.*status|preflight|empty archive" docs/JSON-SCHEMA.md | head -150Repository: fabio-dee/ccaudit
Length of output: 1063
🏁 Script executed:
tail -200 docs/JSON-SCHEMA.md | head -150Repository: fabio-dee/ccaudit
Length of output: 6738
🏁 Script executed:
rg -n "restore.*envelope|RestorePreflight" docs/JSON-SCHEMA.mdRepository: fabio-dee/ccaudit
Length of output: 131
🏁 Script executed:
rg -B 5 -A 20 "^\| \`.*restore" docs/JSON-SCHEMA.mdRepository: fabio-dee/ccaudit
Length of output: 5140
🏁 Script executed:
sed -n '1,300p' docs/JSON-SCHEMA.md | grep -A 100 "restore"Repository: fabio-dee/ccaudit
Length of output: 14394
🏁 Script executed:
rg -B 5 -A 15 "RestorePreflightError|error.*field|error.*envelope" docs/JSON-SCHEMA.mdRepository: fabio-dee/ccaudit
Length of output: 43
🏁 Script executed:
rg -B 10 -A 15 "parseEnvelope|buildJsonEnvelope" apps/ccaudit/src/__tests__/restore-preflight-json.test.ts | head -80Repository: fabio-dee/ccaudit
Length of output: 3035
🏁 Script executed:
wc -l docs/JSON-SCHEMA.md && rg -n "error\|status" docs/JSON-SCHEMA.mdRepository: fabio-dee/ccaudit
Length of output: 85
🏁 Script executed:
cat docs/JSON-SCHEMA.md | grep -A 5 -B 5 "error\|preflight"Repository: fabio-dee/ccaudit
Length of output: 3389
🏁 Script executed:
rg -B 5 -A 25 "class RestorePreflightError\|interface.*Preflight" packages/internal/src/remediation/restore.ts | head -100Repository: fabio-dee/ccaudit
Length of output: 43
🏁 Script executed:
rg -B 5 -A 15 "RestorePreflightError" packages/internal/src/remediation/restore.ts | head -150Repository: fabio-dee/ccaudit
Length of output: 43
🏁 Script executed:
find . -name "*.ts" -type f -exec grep -l "RestorePreflightError" {} \;Repository: fabio-dee/ccaudit
Length of output: 164
🏁 Script executed:
rg -B 5 -A 15 "RestorePreflightError" apps/ccaudit/src/cli/commands/restore.ts | head -200Repository: fabio-dee/ccaudit
Length of output: 7871
🏁 Script executed:
rg "empty.*archive|Nothing to restore|preflight.*envelope" docs/JSON-SCHEMA.mdRepository: fabio-dee/ccaudit
Length of output: 43
Document the preflight error envelope shape in docs/JSON-SCHEMA.md.
The test correctly expects exitCode: 0 with error: "archive is empty" — this is intentional design for informational preflight outcomes (e.g., empty archive detected before execution begins). However, per the public contract rule, the preflight error envelope shape is not currently documented in the schema. Add a section covering the shape for cases like:
- Empty archive when
--interactivefinds no restoreable items - Other preflight validation failures that exit non-zero but still emit JSON
The envelope includes meta.exitCode and error at the top level (spread via buildJsonEnvelope), with stderr remaining empty.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ccaudit/src/__tests__/restore-preflight-json.test.ts` around lines 172 -
196, Add documentation for the preflight error JSON envelope used by
buildJsonEnvelope (cases like RestorePreflightError) to the project's JSON
schema document: describe the top-level fields emitted (meta.command,
meta.exitCode, error string) and note that stderr is empty for informational
preflight outcomes (e.g., "archive is empty" with meta.exitCode: 0) and that
other preflight failures may emit a non-zero meta.exitCode while still returning
JSON; include an example envelope and update schema section for preflight
validation responses so consumers know the exact shape.
4c37c26 to
328e9bb
Compare
Restore correctness & safety invariants - Stop anchoring restore on the purge manifest (use bust manifests) - Skip unreadable manifests instead of crashing restore - Filter purged items out of `restore --list` - Record history on cancel paths and JSON envelope on flag conflict Parser hardening - patchMcpConfigText: tolerate whitespace variants (INV-S1) - scan-hooks: stop accepting non-canonical fields Public API field renames (snake_case → camelCase) - selection_filter → selectionFilter - filtered_stale_count → filteredStaleCount - canonical_id → canonicalId - Coordinated across CLI, tests, fixtures, and docs/JSON-SCHEMA.md Test reliability - Replace remaining setTimeout sleeps with waitForMarker in tabbed-picker-help-overlay (CR-003) — protects INV-S2 coverage - Make reclaim TOCTOU in-source test Windows-safe via path.join(tmpdir(), ...) + path.resolve() (CR-021) - Sync stale docstring in tabbed-picker-help-overlay (CR-039) Refactor - Rename _archive-move.ts → archive-move.ts (drop underscore prefix)
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (1)
apps/ccaudit/src/cli/commands/purge-archive.ts (1)
201-216:⚠️ Potential issue | 🟡 MinorVerify
purge.manifestPath/purge.manifestErrorsare intentionally public.These envelopes add two top-level keys beyond
purge.summary.*andpurge.failures[]. If that surface is intentional, it should be documented alongside the rest of the JSON contract; otherwise it would be cleaner to fold manifest-read problems intopurge.failures[]and drop the extra keys. Based on learnings: "The--jsonenvelope is a public contract; shape changes must go through docs/JSON-SCHEMA.md and require a CHANGELOG note flagged as breaking".#!/bin/bash # Verify whether the extra purge envelope fields are documented anywhere public. rg -n -C2 'manifestPath|manifestErrors|purge\.summary|purge\.failures' \ README.md CHANGELOG.md docs/JSON-SCHEMA.mdAlso applies to: 257-260, 293-303
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ccaudit/src/cli/commands/purge-archive.ts` around lines 201 - 216, The JSON envelope being emitted when outMode.json is true currently exposes purge.manifestPath and purge.manifestErrors as top-level fields beyond purge.summary and purge.failures; decide whether those fields are part of the public contract: if they are intentional, update the JSON schema (docs/JSON-SCHEMA.md) and add a CHANGELOG entry noting the public shape change, otherwise remove the extra keys from the envelope constructed by buildJsonEnvelope in purge-archive.ts and fold any manifest read problems into purge.failures[] (append failure entries derived from manifestErrors with a clear reason and drop manifestPath), and apply the same change to the other envelope constructions that reference manifestErrors/manifestPath so the JSON output shape remains consistent.
🧹 Nitpick comments (14)
apps/ccaudit/scripts/bundle-smoke-test.mjs (1)
1-41: LGTM — smoke test correctly gates the bundle.The script addresses the prior concern by spawning the built bundle with
--helpand asserting a clean exit, so import-resolution failures are now distinguishable from CLI startup paths. Spawn-error, signal-termination (incl. timeout SIGTERM), and non-zero exit are each handled with a clear[bundle-smoke]diagnostic, and stdout/stderr are echoed only when non-empty. Path resolution at line 10 correctly producesapps/ccaudit/dist/index.jsbecausepath.resolvetreats__filenameas a left-to-right segment.Optional nit (Line 10): for readability, consider
resolve(dirname(__filename), '../dist/index.js')so the relative depth no longer relies on the implicit "trailing filename consumes one.." behavior ofpath.resolve. Not a defect — purely stylistic.✏️ Optional readability tweak
import { fileURLToPath } from 'node:url'; -import { resolve } from 'node:path'; +import { dirname, resolve } from 'node:path'; import { existsSync } from 'node:fs'; import { spawnSync } from 'node:child_process'; const __filename = fileURLToPath(import.meta.url); -const distPath = resolve(__filename, '../../dist/index.js'); +const __dirname = dirname(__filename); +const distPath = resolve(__dirname, '../dist/index.js');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ccaudit/scripts/bundle-smoke-test.mjs` around lines 1 - 41, The dist path computation using resolve(__filename, '../../dist/index.js') is correct but slightly non-obvious; change it to compute dirname(__filename) first and then resolve that with '../dist/index.js' so the intent is clearer — update the distPath assignment (where __filename is derived via fileURLToPath(import.meta.url) and currently used in the distPath constant) to call dirname(__filename) from 'node:path' and then resolve(dirname(__filename), '../dist/index.js') so the relative depth is explicit and more readable.apps/ccaudit/src/__tests__/tabbed-picker-help-overlay.test.ts (1)
113-121: DeadbeforeLenand a no-opwaitForMarkerafter Space.Two small issues in the Space-swallowed step:
- L113 captures
beforeLen = stdoutBuf.lengthbut it is never read before being reassigned on L127.- The
waitForMarkeron L115–119 waits for(Press ? or Esc to close)against the fullstdoutBuf, but that marker was already present from the open-overlay step on L96–100, so this call returns immediately without actually synchronizing on any post-Space render. The real "Space was swallowed" contract is what's asserted later via the0 of 3 selected across all tabsfooter after the?close (L134–138), which is fine — but the inline assertion on L120 is just re-asserting the same already-buffered marker.Consider either dropping L113 + tightening the post-Space wait to slice from the new offset (so we genuinely wait for the next render after Space), or trimming the redundant assertion and letting the post-toggle counter check carry the contract.
♻️ Suggested simplification
// While overlay open, press Space — must be swallowed per D5-13. - let beforeLen = stdoutBuf.length; + let beforeLen = stdoutBuf.length; await sendKeys(spawned.child, [' ']); - await waitForMarker( - () => stripAnsi(stdoutBuf), - () => spawned.child.exitCode !== null, - '(Press ? or Esc to close)', - ); - expect(stripAnsi(stdoutBuf)).toContain('(Press ? or Esc to close)'); - expect(spawned.child.exitCode).toBeNull(); + // Give Space a chance to be processed; the post-close counter check + // below is what actually proves the keystroke was swallowed. + expect(spawned.child.exitCode).toBeNull();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ccaudit/src/__tests__/tabbed-picker-help-overlay.test.ts` around lines 113 - 121, The test captures beforeLen = stdoutBuf.length but never uses it, and waitForMarker(...) is checking the full stdoutBuf for the already-present "(Press ? or Esc to close)" marker, so the post-Space synchronization is a no-op; fix by either removing the dead beforeLen and the redundant wait/assert, or make the wait actually observe new output by using the slice from beforeLen (e.g., pass a function that returns stripAnsi(stdoutBuf).slice(beforeLen)) when calling waitForMarker after sendKeys; update references to beforeLen, stdoutBuf, sendKeys, waitForMarker, stripAnsi and spawned.child.exitCode accordingly so the test truly waits for the post-Space render or removes the redundant checks.apps/ccaudit/src/__tests__/pagination-500.test.ts (1)
43-48: Nit: tighten the upper bound to the actual viewport height.The comment correctly derives
viewportHeight = max(8, 24 - 10) = 14, soagentMatches.sizeshould never exceed 14 distinct names. The≤ 20ceiling will silently accept a regression that lets the picker render up to 20 unique rows (e.g. an off-by-N in viewport slicing) — exactly the kind of overflow this test exists to catch. Lowering the cap to 14 makes the test match the documented invariant.- expect(agentMatches.size).toBeGreaterThanOrEqual(10); - expect(agentMatches.size).toBeLessThanOrEqual(20); + expect(agentMatches.size).toBeGreaterThanOrEqual(10); + expect(agentMatches.size).toBeLessThanOrEqual(14);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ccaudit/src/__tests__/pagination-500.test.ts` around lines 43 - 48, The test's upper bound is too loose: replace the ceiling check that expects agentMatches.size ≤ 20 with a stricter check matching the derived viewportHeight (14). Update the assertion that references agentMatches (created via new Set(frame.match(/agent-\d{3}/g) ?? [])) so it uses toBeLessThanOrEqual(14) instead of 20, and adjust or keep the comment noting viewportHeight = max(8, 24-10) = 14 to reflect the invariant.apps/ccaudit/src/__tests__/fixtures/ghost-500-items.ts (1)
59-74: Optional: add an exhaustiveness guard tocategoryDir.The switch implicitly relies on
TokenCostResult['item']['category']being exactly these six members. If the union later gains a new variant (e.g.'plugin'), this function will silently fall through and returnundefined, which would then end up insideitem.pathas/fake/undefined/...and degrade fixture realism without a type error in most strictness configs.♻️ Suggested guard
function categoryDir(category: TokenCostResult['item']['category']): string { switch (category) { case 'agent': return 'agents'; case 'skill': return 'skills'; case 'command': return 'commands'; case 'mcp-server': return 'mcp'; case 'memory': return 'memory'; case 'hook': return 'hooks'; + default: { + const _exhaustive: never = category; + throw new Error(`buildGhosts500: unsupported category ${String(_exhaustive)}`); + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ccaudit/src/__tests__/fixtures/ghost-500-items.ts` around lines 59 - 74, The switch in categoryDir(category: TokenCostResult['item']['category']) lacks an exhaustiveness guard and can return undefined if a new category is added; update categoryDir to include a default / unreachable branch that either throws an informative Error or uses a never-type assertion to force a compile-time error (e.g., assertUnreachable) and ensure the function always returns a string, referencing the categoryDir function and the TokenCostResult['item']['category'] union so future new variants fail loudly.apps/ccaudit/src/__tests__/tabbed-picker-live-counter.test.ts (2)
173-175: Acceptingnullas a valid exit code may hide hangs.
expect([0, 130, null].includes(result.exitCode)).toBe(true)allowsnull, which in Node corresponds to a process terminated by signal without an exit code. Afterspawned.doneresolves you've awaited termination, so 0 (clean exit) and 130 (SIGINT) are the only legitimate outcomes for these Ctrl-C cleanups. Acceptingnullhere (and at lines 215, 277, 325, 401, 472) means a hung-then-killed-by-timeout subprocess would still pass. Consider tightening to[0, 130]once the harness reliably reports the SIGINT exit code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ccaudit/src/__tests__/tabbed-picker-live-counter.test.ts` around lines 173 - 175, The test currently accepts null as a valid exit code which can mask hangs; update the assertions that check process termination (the expect([...].includes(result.exitCode)) calls that follow spawned.child.stdin!.end() and await spawned.done — e.g., the one assigning const result = await spawned.done and using result.exitCode — to only accept [0, 130] instead of [0, 130, null]; apply the same tightening to the other identical assertions in this test file (the other expect([...].includes(result.exitCode)) occurrences) so a hung/timeout-killed subprocess no longer passes the test.
263-271: Regex tolerates loose forms but only validates "a number is present".
/(?:~|≈)?\s*(\d+)(?:\.\d+)?k?\s+tokens saved/makes the glyph, decimal, andkall optional, then only asserts the captured integer is finite. That's effectively "any digit followed by tokens saved". Given Test 3's purpose is to prove cross-tab sums populate the global footer, consider also assertingparsed > 0(the fixture has 3 non-trivial bodies, so the sum must be positive) — otherwise a regression that zeroes the counter while still emitting "0 tokens saved" would slip past.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ccaudit/src/__tests__/tabbed-picker-live-counter.test.ts` around lines 263 - 271, The test's regex in tabbed-picker-live-counter.test.ts currently only checks that a numeric match exists and that the parsed integer is finite; update the assertion after parsing (the `parsed` variable derived from `match[1]`) to also assert parsed > 0 so the test fails if the counter is zero or negative, ensuring the cross-tab sum is actually positive; keep the existing regex and Number.isFinite check and simply add expect(parsed).toBeGreaterThan(0) (or equivalent) after the parse step.apps/ccaudit/src/__tests__/tabbed-picker-bust-parity.test.ts (1)
148-154: Optional: silent-skip path can mask real regressions.When
pickerTokensKisnullor0the test logs and returns success rather than failing. If the fixture estimator ever silently returns 0 for non-trivial agent bodies (e.g. due to a tokenizer regression), this test will go green forever instead of catching the drift it's meant to guard. Consider assertingpickerTokensK !== null && pickerTokensK > 0(or only allowing skip when explicitly opted in via env), sincebuildManyGhostsFixtureis a controlled fixture that should reliably yield >0 tokens.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ccaudit/src/__tests__/tabbed-picker-bust-parity.test.ts` around lines 148 - 154, The test currently silently skips when pickerTokensK is null or 0 (the if block that logs and returns), which can mask tokenizer regressions; replace that silent-skip with an explicit assertion that pickerTokensK !== null && pickerTokensK > 0 (or only allow a skip when an explicit env var like ALLOW_FIXTURE_SKIP is set), and if the assertion fails fail the test (clean up by sending '\x03' to spawned.child and awaiting spawned.done as before) instead of returning success; locate the conditional around pickerTokensK (produced by buildManyGhostsFixture) and the cleanup using sendKeys(spawned.child, ['\x03']) / spawned.child.stdin!.end() to implement this change.apps/ccaudit/src/__tests__/tmux-e2e-manual-qa.test.ts (2)
230-248: Possible flake:cleared.toContain('agent-081')couples to picker scroll-position-after-Esc.After typing
/agent-09+ Enter the filter narrows to 10 items, then Esc clears the filter. Whetheragent-081is still under the cursor (and therefore in the captured viewport) when the unfiltered list returns depends on the picker's "preserve cursor across filter clear" behavior. If the picker resets to row 0 on Esc — a reasonable UX choice — this assertion will start failing on a future picker tweak that's logically correct.If the intent is "Esc cleared the filter" (not "cursor preserved its row"), the
Filtered: 10 of 550 visibleabsence check on line 246 already proves that. Consider dropping line 247 or replacing it with a check on tab header counts (e.g.AGENTS (0/550)).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ccaudit/src/__tests__/tmux-e2e-manual-qa.test.ts` around lines 230 - 248, The assertion that cleared.toContain('agent-081') is flaky because it depends on picker cursor/scroll behavior after Esc; update the test in tmux-e2e-manual-qa.test.ts to stop asserting the presence of a specific row after clearing the filter and instead assert that the filter was cleared via an invariant that doesn't depend on scroll position (for example, remove the cleared.toContain('agent-081') check and replace it with a check against the tab header text like asserting session.capture or session.waitForText contains 'AGENTS (0/550)', or simply rely on the existing expect(current).not.toContain('Filtered: 10 of 550 visible') to prove the filter was cleared). Ensure you update references to cleared/current/session accordingly so the test still verifies the filter-clear behavior without coupling to cursor position.
35-43: Minor: tmux availability probe runs at import-time even when opt-in is off.The IIFE at lines 35-43 short-circuits on
!optIn || win32, soexecFileSync('tmux', ['-V'])only runs when opt-in is set. That's fine. However, ifCCAUDIT_TMUX_E2E=1is exported across a CI matrix where tmux is missing on some runners, the probe will swallow the error and silently skip the suite. Consider logging a single stderr line in that case so misconfigured CI is easier to debug.- } catch { - return false; + } catch { + process.stderr.write('tmux-e2e: opt-in set but `tmux -V` failed; suite will skip.\n'); + return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ccaudit/src/__tests__/tmux-e2e-manual-qa.test.ts` around lines 35 - 43, The tmux availability IIFE (tmuxAvailable) currently swallows errors when execFileSync('tmux', ['-V']) fails, which hides misconfigured CI; update the catch block so that when optIn is true and execFileSync throws you print a single stderr line (e.g., via console.error) that includes a brief diagnostic message and the caught error (or error.message) before returning false; keep the short-circuit for process.platform === 'win32' and only emit the log when optIn is enabled and execFileSync fails so the test matrix surfaces missing tmux, referencing tmuxAvailable, optIn, and execFileSync.packages/internal/src/remediation/change-plan.ts (1)
312-340: Brittle test: hand-rolled canonical IDs duplicate the format fromcanonicalItemId.Tests 2 and 3 reconstruct the canonical ID strings by hand (e.g.
`agent|global||/tmp/agentA`,`mcp-server|global||m1|/tmp/m1`). If the format produced bycanonicalItemIdever changes (separator, ordering, additional segments), these assertions break far away from the production change and the failure won't make the cause obvious.Consider invoking
canonicalItemId(...)directly against the same inputs that produced the plan so the tests assert behavior, not a snapshot of the format string.♻️ Suggested approach
- it('Test 2: Set with 2 of 3 ids returns only those items', () => { - const plan = makePlan3(); - // Build canonical ids for agentA and agentB (category|scope|projectPath|path) - const idA = `agent|global||/tmp/agentA`; - const idB = `agent|global||/tmp/agentB`; - const result = filterChangePlan(plan, new Set([idA, idB])); + it('Test 2: Set with 2 of 3 ids returns only those items', () => { + const plan = makePlan3(); + const idA = canonicalItemId(plan.archive.find((i) => i.name === 'agentA')!); + const idB = canonicalItemId(plan.archive.find((i) => i.name === 'agentB')!); + const result = filterChangePlan(plan, new Set([idA, idB])); expect(result.archive).toHaveLength(2);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/internal/src/remediation/change-plan.ts` around lines 312 - 340, Replace the brittle hand-constructed canonical ID strings in the tests with calls to the canonicalItemId helper so the tests assert behavior instead of a hardcoded format: in the Test 2 block that uses makePlan3() and then constructs idA/idB, call canonicalItemId(...) with the same category/scope/projectPath/name/path used to build those results; likewise in Test 3, build idA1 and idM1 by calling canonicalItemId with the arguments matching makeResult(...) and the mcp-server item's fields. Update the imports if necessary and keep the rest of the assertions (filterChangePlan, counts, savings) unchanged.packages/internal/src/remediation/archive-move.ts (1)
90-102: Minor:io_errormessage conflates mkdir and rename failures.If
mkdirRecursive(...)throws, the catch wraps the error asrename ${archivePath} → ${sourcePath}: ..., which misattributes the failure (it was a mkdir, not a rename). For most callers the distinction doesn't matter, but reclaim'sfailed[].errorand purge'sfailures[].reasonsurface this string verbatim, so debugging an EACCES onmkdir .claude/agentsbecomes harder than necessary.♻️ Suggested fix
try { - await deps.mkdirRecursive(path.dirname(sourcePath)); - await deps.renameFile(archivePath, sourcePath); - return Result.succeed({ moved: true }); + try { + await deps.mkdirRecursive(path.dirname(sourcePath)); + } catch (err) { + const message = err instanceof Error ? err.message : String(err); + return Result.fail({ + reason: 'io_error', + message: `mkdir ${path.dirname(sourcePath)}: ${message}`, + }); + } + await deps.renameFile(archivePath, sourcePath); + return Result.succeed({ moved: true }); } catch (err) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/internal/src/remediation/archive-move.ts` around lines 90 - 102, The error message always labels failures as a "rename" even when mkdirRecursive fails; update the logic in the function that calls deps.mkdirRecursive and deps.renameFile so the failure message reflects which operation failed (mkdir vs rename). Implement this by tracking the current operation (e.g. set a local var op = 'mkdir' before await deps.mkdirRecursive(...) and op = 'rename' before await deps.renameFile(...)) or by splitting into two try/catch blocks, and use that operation name when building the Result.fail payload (the reason/message returned from Result.fail should include the correct operation and path), leaving the Result.succeed({ moved: true }) behavior unchanged.apps/ccaudit/src/__tests__/fixtures/tmux-e2e.ts (1)
145-165: Minor:stripAnsiruns twice on the default path.When
opts.stripAnsi !== false(the default),capture()already callsstripAnsi(stdout)internally (line 118), thenwaitForTextcallsstripAnsi(raw)again on line 155. It's harmless (the regex finds nothing on the second pass) but redundant. Either trustcapture()to do it, or skip it incapture()when called fromwaitForText.♻️ Suggested simplification
- while (Date.now() - start <= timeoutMs) { - const raw = await this.capture({ - startLine: opts.startLine ?? -200, - ansi: opts.stripAnsi === false, - }); - lastCapture = opts.stripAnsi === false ? raw : stripAnsi(raw); + while (Date.now() - start <= timeoutMs) { + lastCapture = await this.capture({ + startLine: opts.startLine ?? -200, + ansi: opts.stripAnsi === false, + }); const matched =🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ccaudit/src/__tests__/fixtures/tmux-e2e.ts` around lines 145 - 165, The waitForText function is redundantly calling stripAnsi on the capture output even though capture already strips ANSI by default; update waitForText to trust capture's ansi handling: when calling this.capture pass ansi: opts.stripAnsi === false (so capture strips unless explicitly disabled), then assign lastCapture = raw (no extra stripAnsi call) and only call stripAnsi if capture was invoked with ansi: true; adjust logic around opts.stripAnsi and the capture({ ansi }) call in waitForText accordingly so stripAnsi is not applied twice.apps/ccaudit/src/__tests__/safety-invariants-restore-roundtrip.test.ts (1)
105-113: Helper dropsrunCcauditGhostoverrides other thanenv.
runGhostonly forwardsenvtorunCcauditGhost; any other future option (timeout, cwd, stdin, signal) the helper supports will be silently ignored by callers of this local wrapper. Right now there is one call site so this is fine, but as the PR adds more--interactive-flavored flows on top of this fixture, prefer passing throughoptsopaquely.♻️ Suggested refactor
- async function runGhost( - args: string[], - env?: Record<string, string>, - ): Promise<{ stdout: string; stderr: string; exitCode: number | null }> { - const spawned = runCcauditGhost(tmpHome, args, env ? { env } : {}); + async function runGhost( + args: string[], + opts: Parameters<typeof runCcauditGhost>[2] = {}, + ): Promise<{ stdout: string; stderr: string; exitCode: number | null }> { + const spawned = runCcauditGhost(tmpHome, args, opts); spawned.child.stdin?.end(); const r = await spawned.done; return { stdout: r.stdout, stderr: r.stderr, exitCode: r.exitCode }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ccaudit/src/__tests__/safety-invariants-restore-roundtrip.test.ts` around lines 105 - 113, The helper runGhost only forwards the env option to runCcauditGhost, dropping any other options; change runGhost to accept an additional opts (or options) parameter (e.g., opts?: Record<string, any> or the actual runCcauditGhost options type), pass those options through to runCcauditGhost (merging env if provided) instead of only sending { env }, and preserve the existing behavior of ending stdin and returning stdout/stderr/exitCode; update the signature and the call site(s) that use runGhost accordingly so future options like timeout, cwd, stdin, or signal are not ignored.packages/internal/src/remediation/purge.ts (1)
265-297: Optional: dedupe the reclaim/drop per-op scaffold.The reclaim loop (232–263) and drop loop (265–297) share the same shape — outer try/catch, inner
writer.writeOptry/catch withmanifest_write_failedformatting,appendedOps.push, summary increment. A smallrunOpWithJournal(op, mutate, buildPurgeOp, onSuccess)helper would let the two loops collapse to their distinguishing parts (the mutation + the summary delta) and keep themanifest_write_failed:reason string in one place. Not blocking; current duplication is small and well-tested.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/internal/src/remediation/purge.ts` around lines 265 - 297, The two loops ("reclaim" and "drop") duplicate the same outer try/catch, inner writer.writeOp error handling, appendedOps push and summary updates; extract a helper (e.g., runOpWithJournal) that accepts the op, a mutate function (the conditional unlink or noop), a function to build the journal op (e.g., buildArchivePurgeOp or the reclaim equivalent), and an onSuccess callback to update appendedOps and summary; inside the helper perform the try { await mutate(); const journal = buildOp(...); try { await writer.writeOp(journal); } catch (writeErr) { push to failures with manifest_write_failed formatted exactly as now; return; } appendedOps.push(journal); call onSuccess(); } catch (err) { push to failures with existing error formatting; } then replace both loops to call runOpWithJournal(...) with the appropriate mutate and onSuccess lambdas so the manifest_write_failed formatting and push-to-failures logic remain centralized.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/ccaudit/src/__tests__/restore-interactive-source-exists.test.ts`:
- Around line 80-83: Update the JSON-SCHEMA.md example and description to
reflect that canonicalId for archive operations includes the full archive path
(format: "agent:<archive_path>") not just the basename; find references/examples
showing "canonicalId": "agent:code-reviewer" and change them to the full path
form (e.g., "agent:/path/to/.claude/ccaudit/archived/agents/code-reviewer.md")
and add a short clarifying sentence near the "canonical item identifiers"
section explaining that archive ops emit the full archive path rather than only
the name so the public --json API contract is accurate (tests reference
parsed.skipped[*].canonicalId).
In `@apps/ccaudit/src/cli/commands/restore.ts`:
- Around line 305-308: The current use of Promise.all when mapping entries to
readManifest(entry.path) causes one unreadable/corrupt manifest to reject the
whole preflight; change the logic in the restore command near the
findManifestsForRestore usage so you individually attempt to read each manifest
(e.g., with a for-loop or Promise.allSettled) and collect only the successfully
read results into the pairs array, skipping and logging a warning for unreadable
manifests; preserve the existing deduplication semantics (dedupe by
archive_path, newer wins) when building the final list. Apply the same fix to
the other similar readManifest usages in this file where entries are bulk-read.
In `@docs/JSON-SCHEMA.md`:
- Around line 106-115: The two adjacent blockquotes titled "Manifest casing
exception." and "Pre-dispatch validation envelope." are separated by a blank
line which triggers MD028; either merge them into a single blockquote or insert
non-blockquote content between them. To fix, remove the blank line so both
paragraphs become one continuous blockquote (keeping the exact text of both
headers and bodies) or add a short paragraph or horizontal rule between the two
quoted sections to explicitly separate them; apply this change to the
blockquotes that begin with "Manifest casing exception." and "Pre-dispatch
validation envelope." in the diff.
- Around line 86-104: The docs incorrectly show short canonical IDs; update the
JSON-SCHEMA.md entries for selectionFilter (selectionFilter.ids) and
skipped[].canonicalId to document the real full-ID formats and replace the
example: state that canonicalId/selectionFilter.ids are strings that can be one
of the op-specific shapes — archive ops use "agent:<archive_path>" or generally
"<category>:<archive_path>" with a full filesystem path (e.g.
agent:/home/user/.claude/agents/_archived/code-reviewer.md), disable ops use
"mcp:<config_path>:<key>", and memory ops use
"memory:<op_type>:<file_path>:<op_id>"; update the prose lines describing
selectionFilter and skipped and replace the JSON example to show these full
formats so callers see selectionFilter.ids and skipped[].canonicalId as the real
full op identifiers.
In `@packages/internal/src/remediation/atomic-write.ts`:
- Around line 69-71: The change to pretty-print JSON in atomicWriteJson is
intentional: keep JSON.stringify(value, null, 2) in atomicWriteJson (the call
that delegates to atomicWriteRaw) and do not revert to compact JSON; if team
policy mandates recording on-disk format changes, add a short CHANGELOG entry
referencing the internal manifest files (e.g., ~/.claude/ccaudit/manifests) and
the commit c466f2b, otherwise leave the implementation as-is since tests cover
atomicity/retries not formatting.
In `@packages/internal/src/remediation/purge.ts`:
- Around line 232-311: The current all-failed check treats manifest_write_failed
(journal persist failures after disk mutations) the same as disk-level failures;
update the final-all-failed gate in the purge function so it ignores
manifest_write_failed entries when deciding to return Result.fail: compute
failureCount = failures.filter(f =>
!f.reason?.startsWith('manifest_write_failed')).length and use that against
totalRequested (keep the existing closePurgeManifestWriter(writer, null)
behavior and existing failures array), so that if only manifest_write_failed
entries exist (disk mutations succeeded) the function returns Result.success
while still reporting the manifest failures in failures[].
In `@packages/internal/src/remediation/restore.ts`:
- Around line 985-991: The unreadable manifest bug comes from calling
deps.readManifest() in the executeInteractiveOps / "{ kind: 'all-matching' }"
paths without the per-entry try/catch used by full/list mode; wrap each
deps.readManifest(e.path) call in a try/catch, on error call deps.onWarning with
a descriptive message including path basename and skip that entry (do not
throw), and only push to zipped when a manifest object is successfully read and
m.header !== null—mirror the error-handling pattern used in the full/list
restore flow so restore continues despite single corrupt manifests (apply same
fix to both the executeInteractiveOps and the all-matching branch).
In `@README.md`:
- Around line 323-329: The README incorrectly documents the positional restore
operand as accepting a canonical id; update the wording to state it expects an
archived item name (the archive basename / MCP server name) to match how
apps/ccaudit/src/cli/commands/restore.ts routes the positional arg through
findManifestForName() and how packages/internal/src/remediation/restore.ts
resolves items. Change the `<name>` row (and the duplicate description at lines
referenced 409-413) to explicitly say "archive name (basename / MCP server
name)" or similar, and ensure examples and error text align with the actual
behavior implemented by findManifestForName() and the remediation restore logic.
- Around line 234-243: Add a new row for the purge-archive command to the
top-level command table so the README’s command matrix includes this v1.5
feature: locate the markdown table and insert a row with `purge-archive` as the
Command, a concise description like "Purge archived files not referenced by any
manifest", and the relevant Notable options (e.g. `--dry-run`, any other flags
documented in the existing "purge-archive" section); ensure formatting matches
the other rows and the pipe-separated columns align with the header.
---
Duplicate comments:
In `@apps/ccaudit/src/cli/commands/purge-archive.ts`:
- Around line 201-216: The JSON envelope being emitted when outMode.json is true
currently exposes purge.manifestPath and purge.manifestErrors as top-level
fields beyond purge.summary and purge.failures; decide whether those fields are
part of the public contract: if they are intentional, update the JSON schema
(docs/JSON-SCHEMA.md) and add a CHANGELOG entry noting the public shape change,
otherwise remove the extra keys from the envelope constructed by
buildJsonEnvelope in purge-archive.ts and fold any manifest read problems into
purge.failures[] (append failure entries derived from manifestErrors with a
clear reason and drop manifestPath), and apply the same change to the other
envelope constructions that reference manifestErrors/manifestPath so the JSON
output shape remains consistent.
---
Nitpick comments:
In `@apps/ccaudit/scripts/bundle-smoke-test.mjs`:
- Around line 1-41: The dist path computation using resolve(__filename,
'../../dist/index.js') is correct but slightly non-obvious; change it to compute
dirname(__filename) first and then resolve that with '../dist/index.js' so the
intent is clearer — update the distPath assignment (where __filename is derived
via fileURLToPath(import.meta.url) and currently used in the distPath constant)
to call dirname(__filename) from 'node:path' and then
resolve(dirname(__filename), '../dist/index.js') so the relative depth is
explicit and more readable.
In `@apps/ccaudit/src/__tests__/fixtures/ghost-500-items.ts`:
- Around line 59-74: The switch in categoryDir(category:
TokenCostResult['item']['category']) lacks an exhaustiveness guard and can
return undefined if a new category is added; update categoryDir to include a
default / unreachable branch that either throws an informative Error or uses a
never-type assertion to force a compile-time error (e.g., assertUnreachable) and
ensure the function always returns a string, referencing the categoryDir
function and the TokenCostResult['item']['category'] union so future new
variants fail loudly.
In `@apps/ccaudit/src/__tests__/fixtures/tmux-e2e.ts`:
- Around line 145-165: The waitForText function is redundantly calling stripAnsi
on the capture output even though capture already strips ANSI by default; update
waitForText to trust capture's ansi handling: when calling this.capture pass
ansi: opts.stripAnsi === false (so capture strips unless explicitly disabled),
then assign lastCapture = raw (no extra stripAnsi call) and only call stripAnsi
if capture was invoked with ansi: true; adjust logic around opts.stripAnsi and
the capture({ ansi }) call in waitForText accordingly so stripAnsi is not
applied twice.
In `@apps/ccaudit/src/__tests__/pagination-500.test.ts`:
- Around line 43-48: The test's upper bound is too loose: replace the ceiling
check that expects agentMatches.size ≤ 20 with a stricter check matching the
derived viewportHeight (14). Update the assertion that references agentMatches
(created via new Set(frame.match(/agent-\d{3}/g) ?? [])) so it uses
toBeLessThanOrEqual(14) instead of 20, and adjust or keep the comment noting
viewportHeight = max(8, 24-10) = 14 to reflect the invariant.
In `@apps/ccaudit/src/__tests__/safety-invariants-restore-roundtrip.test.ts`:
- Around line 105-113: The helper runGhost only forwards the env option to
runCcauditGhost, dropping any other options; change runGhost to accept an
additional opts (or options) parameter (e.g., opts?: Record<string, any> or the
actual runCcauditGhost options type), pass those options through to
runCcauditGhost (merging env if provided) instead of only sending { env }, and
preserve the existing behavior of ending stdin and returning
stdout/stderr/exitCode; update the signature and the call site(s) that use
runGhost accordingly so future options like timeout, cwd, stdin, or signal are
not ignored.
In `@apps/ccaudit/src/__tests__/tabbed-picker-bust-parity.test.ts`:
- Around line 148-154: The test currently silently skips when pickerTokensK is
null or 0 (the if block that logs and returns), which can mask tokenizer
regressions; replace that silent-skip with an explicit assertion that
pickerTokensK !== null && pickerTokensK > 0 (or only allow a skip when an
explicit env var like ALLOW_FIXTURE_SKIP is set), and if the assertion fails
fail the test (clean up by sending '\x03' to spawned.child and awaiting
spawned.done as before) instead of returning success; locate the conditional
around pickerTokensK (produced by buildManyGhostsFixture) and the cleanup using
sendKeys(spawned.child, ['\x03']) / spawned.child.stdin!.end() to implement this
change.
In `@apps/ccaudit/src/__tests__/tabbed-picker-help-overlay.test.ts`:
- Around line 113-121: The test captures beforeLen = stdoutBuf.length but never
uses it, and waitForMarker(...) is checking the full stdoutBuf for the
already-present "(Press ? or Esc to close)" marker, so the post-Space
synchronization is a no-op; fix by either removing the dead beforeLen and the
redundant wait/assert, or make the wait actually observe new output by using the
slice from beforeLen (e.g., pass a function that returns
stripAnsi(stdoutBuf).slice(beforeLen)) when calling waitForMarker after
sendKeys; update references to beforeLen, stdoutBuf, sendKeys, waitForMarker,
stripAnsi and spawned.child.exitCode accordingly so the test truly waits for the
post-Space render or removes the redundant checks.
In `@apps/ccaudit/src/__tests__/tabbed-picker-live-counter.test.ts`:
- Around line 173-175: The test currently accepts null as a valid exit code
which can mask hangs; update the assertions that check process termination (the
expect([...].includes(result.exitCode)) calls that follow
spawned.child.stdin!.end() and await spawned.done — e.g., the one assigning
const result = await spawned.done and using result.exitCode — to only accept [0,
130] instead of [0, 130, null]; apply the same tightening to the other identical
assertions in this test file (the other expect([...].includes(result.exitCode))
occurrences) so a hung/timeout-killed subprocess no longer passes the test.
- Around line 263-271: The test's regex in tabbed-picker-live-counter.test.ts
currently only checks that a numeric match exists and that the parsed integer is
finite; update the assertion after parsing (the `parsed` variable derived from
`match[1]`) to also assert parsed > 0 so the test fails if the counter is zero
or negative, ensuring the cross-tab sum is actually positive; keep the existing
regex and Number.isFinite check and simply add expect(parsed).toBeGreaterThan(0)
(or equivalent) after the parse step.
In `@apps/ccaudit/src/__tests__/tmux-e2e-manual-qa.test.ts`:
- Around line 230-248: The assertion that cleared.toContain('agent-081') is
flaky because it depends on picker cursor/scroll behavior after Esc; update the
test in tmux-e2e-manual-qa.test.ts to stop asserting the presence of a specific
row after clearing the filter and instead assert that the filter was cleared via
an invariant that doesn't depend on scroll position (for example, remove the
cleared.toContain('agent-081') check and replace it with a check against the tab
header text like asserting session.capture or session.waitForText contains
'AGENTS (0/550)', or simply rely on the existing
expect(current).not.toContain('Filtered: 10 of 550 visible') to prove the filter
was cleared). Ensure you update references to cleared/current/session
accordingly so the test still verifies the filter-clear behavior without
coupling to cursor position.
- Around line 35-43: The tmux availability IIFE (tmuxAvailable) currently
swallows errors when execFileSync('tmux', ['-V']) fails, which hides
misconfigured CI; update the catch block so that when optIn is true and
execFileSync throws you print a single stderr line (e.g., via console.error)
that includes a brief diagnostic message and the caught error (or error.message)
before returning false; keep the short-circuit for process.platform === 'win32'
and only emit the log when optIn is enabled and execFileSync fails so the test
matrix surfaces missing tmux, referencing tmuxAvailable, optIn, and
execFileSync.
In `@packages/internal/src/remediation/archive-move.ts`:
- Around line 90-102: The error message always labels failures as a "rename"
even when mkdirRecursive fails; update the logic in the function that calls
deps.mkdirRecursive and deps.renameFile so the failure message reflects which
operation failed (mkdir vs rename). Implement this by tracking the current
operation (e.g. set a local var op = 'mkdir' before await
deps.mkdirRecursive(...) and op = 'rename' before await deps.renameFile(...)) or
by splitting into two try/catch blocks, and use that operation name when
building the Result.fail payload (the reason/message returned from Result.fail
should include the correct operation and path), leaving the Result.succeed({
moved: true }) behavior unchanged.
In `@packages/internal/src/remediation/change-plan.ts`:
- Around line 312-340: Replace the brittle hand-constructed canonical ID strings
in the tests with calls to the canonicalItemId helper so the tests assert
behavior instead of a hardcoded format: in the Test 2 block that uses
makePlan3() and then constructs idA/idB, call canonicalItemId(...) with the same
category/scope/projectPath/name/path used to build those results; likewise in
Test 3, build idA1 and idM1 by calling canonicalItemId with the arguments
matching makeResult(...) and the mcp-server item's fields. Update the imports if
necessary and keep the rest of the assertions (filterChangePlan, counts,
savings) unchanged.
In `@packages/internal/src/remediation/purge.ts`:
- Around line 265-297: The two loops ("reclaim" and "drop") duplicate the same
outer try/catch, inner writer.writeOp error handling, appendedOps push and
summary updates; extract a helper (e.g., runOpWithJournal) that accepts the op,
a mutate function (the conditional unlink or noop), a function to build the
journal op (e.g., buildArchivePurgeOp or the reclaim equivalent), and an
onSuccess callback to update appendedOps and summary; inside the helper perform
the try { await mutate(); const journal = buildOp(...); try { await
writer.writeOp(journal); } catch (writeErr) { push to failures with
manifest_write_failed formatted exactly as now; return; }
appendedOps.push(journal); call onSuccess(); } catch (err) { push to failures
with existing error formatting; } then replace both loops to call
runOpWithJournal(...) with the appropriate mutate and onSuccess lambdas so the
manifest_write_failed formatting and push-to-failures logic remain centralized.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 00132340-b539-4ac3-960a-bc4f1a87a981
📒 Files selected for processing (39)
CHANGELOG.mdREADME.mdapps/ccaudit/scripts/bundle-size-check.mjsapps/ccaudit/scripts/bundle-smoke-test.mjsapps/ccaudit/src/__tests__/__fixtures__/restore-interactive/bust-2026-04-17T10-00-00-000Z-aaaa.jsonlapps/ccaudit/src/__tests__/__fixtures__/restore-interactive/bust-2026-04-18T10-00-00-000Z-bbbb.jsonlapps/ccaudit/src/__tests__/_test-helpers.tsapps/ccaudit/src/__tests__/fixtures/ghost-500-items.tsapps/ccaudit/src/__tests__/fixtures/manual-qa-followups.tsapps/ccaudit/src/__tests__/fixtures/tmux-e2e.tsapps/ccaudit/src/__tests__/pagination-500.test.tsapps/ccaudit/src/__tests__/purge-archive.test.tsapps/ccaudit/src/__tests__/restore-after-purge.test.tsapps/ccaudit/src/__tests__/restore-command.test.tsapps/ccaudit/src/__tests__/restore-flag-mutual-exclusion.test.tsapps/ccaudit/src/__tests__/restore-interactive-memory-roundtrip.test.tsapps/ccaudit/src/__tests__/restore-interactive-source-exists.test.tsapps/ccaudit/src/__tests__/restore-json-envelope.test.tsapps/ccaudit/src/__tests__/restore-list-skips-purge.test.tsapps/ccaudit/src/__tests__/restore-stale-filter.test.tsapps/ccaudit/src/__tests__/safety-invariants-restore-roundtrip.test.tsapps/ccaudit/src/__tests__/tabbed-picker-bust-parity.test.tsapps/ccaudit/src/__tests__/tabbed-picker-help-overlay.test.tsapps/ccaudit/src/__tests__/tabbed-picker-live-counter.test.tsapps/ccaudit/src/__tests__/tabbed-picker-protection.test.tsapps/ccaudit/src/__tests__/tmux-e2e-manual-qa.test.tsapps/ccaudit/src/cli/commands/purge-archive.tsapps/ccaudit/src/cli/commands/restore.tsdocs/JSON-SCHEMA.mdpackages/internal/src/remediation/archive-move.tspackages/internal/src/remediation/atomic-write.tspackages/internal/src/remediation/bust.tspackages/internal/src/remediation/change-plan.tspackages/internal/src/remediation/index.tspackages/internal/src/remediation/purge.tspackages/internal/src/remediation/reclaim.tspackages/internal/src/remediation/restore.tspackages/internal/src/scanner/_present-path.tspackages/internal/src/scanner/scan-hooks.ts
✅ Files skipped from review due to trivial changes (1)
- apps/ccaudit/src/tests/fixtures/restore-interactive/bust-2026-04-17T10-00-00-000Z-aaaa.jsonl
🚧 Files skipped from review as they are similar to previous changes (15)
- apps/ccaudit/src/tests/restore-flag-mutual-exclusion.test.ts
- apps/ccaudit/src/tests/restore-command.test.ts
- apps/ccaudit/src/tests/restore-list-skips-purge.test.ts
- apps/ccaudit/src/tests/restore-json-envelope.test.ts
- apps/ccaudit/src/tests/restore-after-purge.test.ts
- apps/ccaudit/src/tests/fixtures/restore-interactive/bust-2026-04-18T10-00-00-000Z-bbbb.jsonl
- apps/ccaudit/src/tests/restore-stale-filter.test.ts
- apps/ccaudit/src/tests/tabbed-picker-protection.test.ts
- apps/ccaudit/scripts/bundle-size-check.mjs
- CHANGELOG.md
- apps/ccaudit/src/tests/fixtures/manual-qa-followups.ts
- apps/ccaudit/src/tests/purge-archive.test.ts
- packages/internal/src/remediation/index.ts
- packages/internal/src/remediation/bust.ts
- apps/ccaudit/src/tests/_test-helpers.ts
| > **Manifest casing exception.** Manifest JSONL headers intentionally keep | ||
| > snake_case fields such as `manifest.header.selection_filter` for backward | ||
| > compatibility with v1.5 dry-run/bust manifests. Public `--json` envelopes use | ||
| > camelCase and do not expose the manifest header casing directly. | ||
|
|
||
| > **Pre-dispatch validation envelope.** Restore preflight failures such as | ||
| > mutually-exclusive flags, `CCAUDIT_NO_INTERACTIVE`, TTY refusal, no-match, or | ||
| > ambiguity emit the standard envelope when `--json` is active: | ||
| > `{ "meta": { "command": "restore", "exitCode": <code>, ... }, "error": "..." }`. | ||
| > Without `--json`, the same message is written to stderr. |
There was a problem hiding this comment.
MD028: blank line between two adjacent blockquotes (carried over from earlier review).
Static analysis flags line 110 with MD028 — the previous identical issue around line 143/147 was fixed, but the same pattern landed again here between the "Manifest casing exception" blockquote and the "Pre-dispatch validation envelope" blockquote. If they're meant to be one blockquote, drop the blank line; if they're meant to be two, separate them with non-blockquote content (e.g. a horizontal rule or a paragraph) so markdownlint doesn't see them as a split-quote.
♻️ Proposed fix (merge into one blockquote)
> **Manifest casing exception.** Manifest JSONL headers intentionally keep
> snake_case fields such as `manifest.header.selection_filter` for backward
> compatibility with v1.5 dry-run/bust manifests. Public `--json` envelopes use
> camelCase and do not expose the manifest header casing directly.
-
-> **Pre-dispatch validation envelope.** Restore preflight failures such as
+>
+> **Pre-dispatch validation envelope.** Restore preflight failures such as
> mutually-exclusive flags, `CCAUDIT_NO_INTERACTIVE`, TTY refusal, no-match, or📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| > **Manifest casing exception.** Manifest JSONL headers intentionally keep | |
| > snake_case fields such as `manifest.header.selection_filter` for backward | |
| > compatibility with v1.5 dry-run/bust manifests. Public `--json` envelopes use | |
| > camelCase and do not expose the manifest header casing directly. | |
| > **Pre-dispatch validation envelope.** Restore preflight failures such as | |
| > mutually-exclusive flags, `CCAUDIT_NO_INTERACTIVE`, TTY refusal, no-match, or | |
| > ambiguity emit the standard envelope when `--json` is active: | |
| > `{ "meta": { "command": "restore", "exitCode": <code>, ... }, "error": "..." }`. | |
| > Without `--json`, the same message is written to stderr. | |
| > **Manifest casing exception.** Manifest JSONL headers intentionally keep | |
| > snake_case fields such as `manifest.header.selection_filter` for backward | |
| > compatibility with v1.5 dry-run/bust manifests. Public `--json` envelopes use | |
| > camelCase and do not expose the manifest header casing directly. | |
| > | |
| > **Pre-dispatch validation envelope.** Restore preflight failures such as | |
| > mutually-exclusive flags, `CCAUDIT_NO_INTERACTIVE`, TTY refusal, no-match, or | |
| > ambiguity emit the standard envelope when `--json` is active: | |
| > `{ "meta": { "command": "restore", "exitCode": <code>, ... }, "error": "..." }`. | |
| > Without `--json`, the same message is written to stderr. |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 110-110: Blank line inside blockquote
(MD028, no-blanks-blockquote)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/JSON-SCHEMA.md` around lines 106 - 115, The two adjacent blockquotes
titled "Manifest casing exception." and "Pre-dispatch validation envelope." are
separated by a blank line which triggers MD028; either merge them into a single
blockquote or insert non-blockquote content between them. To fix, remove the
blank line so both paragraphs become one continuous blockquote (keeping the
exact text of both headers and bodies) or add a short paragraph or horizontal
rule between the two quoted sections to explicitly separate them; apply this
change to the blockquotes that begin with "Manifest casing exception." and
"Pre-dispatch validation envelope." in the diff.
| for (const { op } of plan.reclaim) { | ||
| try { | ||
| const moved = await moveArchiveToSource( | ||
| { archivePath: op.archive_path, sourcePath: op.source_path }, | ||
| deps, | ||
| ); | ||
| if (Result.isFailure(moved)) { | ||
| failures.push({ path: op.archive_path, op_id: op.op_id, reason: moved.error.message }); | ||
| continue; | ||
| } | ||
| const purgeOp = buildArchivePurgeOp({ original_op_id: op.op_id, reason: 'reclaimed' }); | ||
| // NEW-C1: fsync the op entry immediately after the mutation succeeds. | ||
| try { | ||
| await writer.writeOp(purgeOp); | ||
| } catch (writeErr) { | ||
| failures.push({ | ||
| path: op.archive_path, | ||
| op_id: op.op_id, | ||
| reason: `manifest_write_failed: ${writeErr instanceof Error ? writeErr.message : String(writeErr)}`, | ||
| }); | ||
| continue; | ||
| } | ||
| appendedOps.push(purgeOp); | ||
| summary.reclaimedCount += 1; | ||
| } catch (err) { | ||
| failures.push({ | ||
| path: op.archive_path, | ||
| op_id: op.op_id, | ||
| reason: err instanceof Error ? err.message : String(err), | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| for (const { op, reason } of plan.drop) { | ||
| try { | ||
| // Only unlink when archive is physically present. For | ||
| // `stale_archive_missing` the file is already gone by definition — | ||
| // we still write the follow-up op to suppress the stale entry | ||
| // from future restore listings. | ||
| if (reason === 'source_occupied') { | ||
| await deps.unlinkFile(op.archive_path); | ||
| } | ||
| const purgeOp = buildArchivePurgeOp({ original_op_id: op.op_id, reason }); | ||
| // NEW-C1: fsync the op entry immediately after the mutation succeeds. | ||
| try { | ||
| await writer.writeOp(purgeOp); | ||
| } catch (writeErr) { | ||
| failures.push({ | ||
| path: op.archive_path, | ||
| op_id: op.op_id, | ||
| reason: `manifest_write_failed: ${writeErr instanceof Error ? writeErr.message : String(writeErr)}`, | ||
| }); | ||
| continue; | ||
| } | ||
| appendedOps.push(purgeOp); | ||
| summary.purgedCount += 1; | ||
| if (reason === 'source_occupied') summary.skippedOccupiedCount += 1; | ||
| else summary.staleFilteredCount += 1; | ||
| } catch (err) { | ||
| failures.push({ | ||
| path: op.archive_path, | ||
| op_id: op.op_id, | ||
| reason: err instanceof Error ? err.message : String(err), | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| // If everything we attempted failed, surface it as Result.err so the | ||
| // CLI can exit non-zero without having to count failures itself. | ||
| if (totalRequested > 0 && failures.length === totalRequested) { | ||
| // Best-effort close without footer (crash-signature: header present, footer absent). | ||
| try { | ||
| await closePurgeManifestWriter(writer, null); | ||
| } catch { | ||
| // ignore close errors on the all-failed path | ||
| } | ||
| return Result.fail( | ||
| new Error(`all ${totalRequested} purge ops failed; see failures[] for per-item reasons`), | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd packages/internal/src/remediation && wc -l purge.tsRepository: fabio-dee/ccaudit
Length of output: 74
🏁 Script executed:
cd packages/internal/src/remediation && sed -n '230,320p' purge.tsRepository: fabio-dee/ccaudit
Length of output: 3257
🏁 Script executed:
cd packages/internal/src/remediation && sed -n '1,50p' purge.tsRepository: fabio-dee/ccaudit
Length of output: 2039
🏁 Script executed:
# Check test cases related to manifest write failures
cd packages/internal/src/remediation && sed -n '400,700p' purge.ts | grep -A 20 "manifest_write_failed\|writeOp"Repository: fabio-dee/ccaudit
Length of output: 5250
🏁 Script executed:
# Look for any docstrings or comments about the all-failed behavior
cd packages/internal/src/remediation && grep -n "manifest-close\|all-failed\|all.*failed" purge.tsRepository: fabio-dee/ccaudit
Length of output: 310
🏁 Script executed:
# Find the test for "all items failed" scenario
cd packages/internal/src/remediation && sed -n '680,750p' purge.tsRepository: fabio-dee/ccaudit
Length of output: 3234
🏁 Script executed:
# Check if there are any tests specifically for manifest_write_failed scenarios
cd packages/internal/src/remediation && grep -n "manifest_write_failed" purge.tsRepository: fabio-dee/ccaudit
Length of output: 380
🏁 Script executed:
# Look for the complete all-failed test and any related tests
cd packages/internal/src/remediation && sed -n '675,750p' purge.tsRepository: fabio-dee/ccaudit
Length of output: 3397
🏁 Script executed:
# Find the test around line 834 that checks manifest_write_failed
cd packages/internal/src/remediation && sed -n '800,860p' purge.tsRepository: fabio-dee/ccaudit
Length of output: 2370
🏁 Script executed:
# Search for NEW-C1 tests that specifically test writeOp failures
cd packages/internal/src/remediation && grep -B 10 -A 15 "manifest_write_failed" purge.ts | head -100Repository: fabio-dee/ccaudit
Length of output: 2879
🏁 Script executed:
# Look for the complete test context around line 834
cd packages/internal/src/remediation && sed -n '820,870p' purge.tsRepository: fabio-dee/ccaudit
Length of output: 2133
🏁 Script executed:
# Search for tests that specifically have all writeOp failures (not mutation failures)
cd packages/internal/src/remediation && grep -n "it\|describe" purge.ts | grep -i "write\|manifest" | head -20Repository: fabio-dee/ccaudit
Length of output: 1490
🏁 Script executed:
# Look for any test that sets up writeOp to fail for all items
cd packages/internal/src/remediation && sed -n '750,850p' purge.ts | grep -B 5 -A 20 "writeOp.*throw\|writeOpFn"Repository: fabio-dee/ccaudit
Length of output: 2696
🏁 Script executed:
# Check if there's a specific test name mentioning all items or all failures with writeOp
cd packages/internal/src/remediation && rg -A 10 "all.*failed.*writeOp|every.*writeOp.*fail" purge.tsRepository: fabio-dee/ccaudit
Length of output: 43
🏁 Script executed:
# Get full context of the test that checks "2 out of 3 succeeded"
cd packages/internal/src/remediation && sed -n '795,855p' purge.tsRepository: fabio-dee/ccaudit
Length of output: 2343
🏁 Script executed:
# Check if there's any test or documentation about the "all writeOp failures" scenario
cd packages/internal/src/remediation && sed -n '300,330p' purge.tsRepository: fabio-dee/ccaudit
Length of output: 1159
🏁 Script executed:
# Look for any comments or documentation about the all-failed semantics
cd packages/internal/src/remediation && sed -n '160,230p' purge.tsRepository: fabio-dee/ccaudit
Length of output: 2758
🏁 Script executed:
# Check if there's a test that explicitly tests all writeOp failures (not mutation failures)
cd packages/internal/src/remediation && rg -c "writeOp" purge.tsRepository: fabio-dee/ccaudit
Length of output: 64
Clarify behavior when all writeOp calls fail after successful mutations.
The docstring states "Returns Result.err only when every requested item failed," but doesn't clarify whether "failed" includes manifest_write_failed (journal persist failure after disk mutation succeeded). The existing test at line 800 covers partial writeOp failures (2 succeed, 1 fails) and correctly returns Result.success. However, there is no explicit test for the case where all mutations succeed but all writeOp calls fail (e.g., ENOSPC on manifest fd while unlinks/moves succeed across different mounts). In that scenario, failures.length === totalRequested would trigger the all-failed branch, returning Result.fail with message "all N purge ops failed" despite disk mutations having succeeded and appendedOps / summary counters remaining at 0.
If this "journal failure suppresses disk success" semantics is intentional (audit-trail philosophy: "if not journaled, didn't happen"), document it explicitly in the docstring or add a test covering it. Otherwise, consider excluding manifest_write_failed entries from the all-failed gate, or distinguish the error message between "no disk effect" and "journal failed after mutation."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/internal/src/remediation/purge.ts` around lines 232 - 311, The
current all-failed check treats manifest_write_failed (journal persist failures
after disk mutations) the same as disk-level failures; update the
final-all-failed gate in the purge function so it ignores manifest_write_failed
entries when deciding to return Result.fail: compute failureCount =
failures.filter(f => !f.reason?.startsWith('manifest_write_failed')).length and
use that against totalRequested (keep the existing
closePurgeManifestWriter(writer, null) behavior and existing failures array), so
that if only manifest_write_failed entries exist (disk mutations succeeded) the
function returns Result.success while still reporting the manifest failures in
failures[].
1 participant
Summary
ccaudit ghost --interactive): tabbed picker across agents, skills, MCP, commands, and memory with live token counter, fuzzy filter, sort cycle, framework group toggle, help overlay, and SIGWINCH-safe pagination.restore --interactive/--name <pattern>/--all-matching, walking every manifest deduplicated byarchive_path.--force-partialis set on both--dry-runand--dangerously-bust-ghosts.purge-archivecommand: classifier-driven drain of~/.claude/ccaudit/archived/(reclaim → drop-occupied → drop-stale → skip). Default dry-run; real purge requires--yes.bust.summary.freedTokens(subset-accurate),manifest.header.planned_ops(filtered count),restore.selection_filter,restore.skipped[],restore.filtered_stale_count,purge.summary.*,purge.failures[].All v1.4.0 invariants preserved: nothing is deleted, every mutation is manifest-logged, every bust is gated by a SHA-256 inventory checkpoint.
Test plan
pnpm verifygreen (1733 passed, 8 skipped, 2 todo across 145 files)safety-invariants-restore-roundtrip.test.ts)--force-partial(INV-S6)purge-archiveclassifier behaviors (purge-archive.test.ts)tmux-e2e-manual-qa.test.ts)Summary by CodeRabbit
v1.5.0 Release
New Features
--interactiveflag and terminal UI picker for subset selection--interactive,--name, and--all-matchingoptionspurge-archivecommand to permanently remove archived itemsCCAUDIT_NO_INTERACTIVEenvironment variable to disable interactive flowsImprovements
Bug Fixes