Repository navigation
test(tui): reproduce upstream 4031a93 and 75f07d6 on macOS - #59
Merged
Merged
Conversation
Upstream 75f07d6 keeps the skills log inside its folder for any session id: the hook and the Skills widget built the log path from the payload's session_id, and an id containing `/` or `..` named a file outside ~/.cache/ccstatusline/skills (the hook appended to it, the widget read it). Session ids are UUIDs, so only [A-Za-z0-9_-] get a log now; any other id writes nothing and shows no skills. Upstream CI only runs Linux; this proves the fix holds on macOS. Red: against the pre-fix tree (f51332a) all seven traversal ids (`x/../../outside`, `../outside`, `a/b`, `..`, empty, `with space`, `a\b`) get a log path, and a hook with a traversal id creates files outside the skills folder. The probe runs in a subprocess (node, HOME in a temp directory) so the hook's writes land in the temp home, never in the live one. Green: bun test 9/9 and vitest/Node 9/9. Mutation: skills.ts and hook-handler.ts are exercised through the subprocess harness, whose coverage the mutation runner cannot see (both files report uncovered rather than surviving); the red/green pair above is the regression proof. Upstream commit: sirmalloc@75f07d6 Co-Authored-By: Claude Code <noreply@anthropic.com>
Upstream 4031a93 lists new Custom Command shell commands in full before applying an import, escapes control characters in preview values, and requires explicit confirmation for Replace All and Merge with Cancel preselected; imports adding no new commands keep applying immediately. Upstream CI only runs Linux; this proves the fix holds on macOS. Red: against the pre-fix tree (610dcc9) both confirm cases fail — an import adding a shell command applies immediately (onApply fires on the first Enter, no confirmation screen), for Replace All and for Merge. The no-new-commands control passes on both trees. Green: bun test 3/3 and vitest/Node 12/12 (two batch files). Mutation (StrykerJS over ImportPreviewDialog.tsx): the fix's pure functions (escapeControlCharacters, getImportedCommands/ getCustomCommands) have zero surviving mutants. The ~25 survivors in the confirm-screen wiring (useState defaults, item labels and descriptions, the confirmation prompt text, selection-change display handling) are display-only: none of them inverts the apply/cancel choice, which the first test pins with an explicit cancel-then-apply sequence ending in a single onApply('replace'). Upstream commit: sirmalloc@4031a93 Co-Authored-By: Claude Code <noreply@anthropic.com>
On the Linux CI runner the node:child_process mock from git.test.ts is present for every later file, so the probe's execFileSync calls returned undefined and the tests parsed that as JSON. Spawn through Bun.spawnSync under bun and the real spawnSync under vitest/Node, same as the usage-probe harness. Co-Authored-By: Claude Code <noreply@anthropic.com>
axisrow
commented
Oct 12, 2026
axisrow
left a comment
Owner
Author
There was a problem hiding this comment.
Review: PR #59 — approved
Test-only PR reproducing upstream fixes 4031a93 (import: shell commands shown and confirmed) and 75f07d6 (skills log path traversal) on macOS. Two new test files, no source changes. Approving — no changes requested.
Verified during review:
- Red claims hold. At f51332a,
src/utils/skills.tshas noSAFE_SESSION_ID(traversal session ids got a log path); at 610dcc9,ImportPreviewDialog.tsxhas nopendingModeconfirm flow. Both test files would fail on those trees and pass on this base (origin/main 6979c23 already contains both fixes). - APIs match the source.
getSkillsFilePathreturns null exactly for ids failing[A-Za-z0-9_-]+(covers all 7 parametrized ids incl. empty, space, backslash);handleHookInputreturns beforemkdirSyncwhen the path is null, so theescapedFile/ccstatuslineDirabsence assertions are meaningful; the dialog test pins the full flow end to end — escaped command shown in full (\u001b[2Jliteral), first Enter lands on the Cancel-preselected confirm screen withonApplyuncalled, Enter on Cancel returns to preview, then ArrowDown+Enter applies exactlyonApply('replace'); and a no-commands import still applies immediately. - Hermetic. The skills probe runs in a subprocess with
HOME/USERPROFILEpointed at a temp dir (node honorsHOME), so no writes reach the real home; bun build + node per test is the documented harness. - Convention match. MockTtyStream/waitFor/down-arrow (
\u001B[B) patterns mirror the sibling ink tests; all imports are used; assertions are not tautological.
Non-blocking notes:
getOutput()in the new dialog test does not strip ANSI, unlike every sibling test in the directory. It works because every asserted string is a contiguous single-Text run (selected labels are one chalk-wrapped run, the command line and the yellow warning are single uncolored/colored texts), but it is a latent fragility if ink's rendering ever splits runs; stripping would make it uniform with siblings.- The MockTtyStream harness is copy-pasted rather than shared — consistent with existing per-file convention, though #56 showed the dedup path exists if the ink harness ever grows a shared helper too.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Batch 6: reproduce upstream TUI/skills fixes on macOS — 4031a93, 75f07d6
TDD verification per the /fix pipeline: red on the pre-fix tree → green on main under both runners (bun test + vitest/Node) → mutation check (StrykerJS) → lint. Last batch of the wave; follows the merged #55/#56/#57 and open #58 (batches 4–5).
Upstream CI only runs Linux; 4031a93 is security-relevant (imported shell commands were applied without showing them), 75f07d6 is a path-traversal fix (session ids naming files outside the skills folder).
x/../../outside,../outside,a/b,.., empty,with space,a\b) get a log path; a hook with a traversal id writes outside the skills folder @f51332a610dcc9Harness notes
HOMEin a temp directory): the hook's writes land in the temp home, never in the live one.Survivors triage (fix lines)
escapeControlCharacters/getImportedCommands/getCustomCommands(pure functions added by the fix): zero surviving mutants.ImportPreviewDialog.tsxconfirm-screen wiring (~25 survivors): all display-only — item labels, descriptions, the confirmation prompt text and the selection-change display handling. None inverts the apply/cancel choice, which the main test pins end to end: first Enter lands on Cancel (no apply), returning to the preview, then ArrowDown + Enter applies exactly once withonApply('replace').skills.ts/hook-handler.tsreport uncovered rather than surviving: the subprocess harness's coverage is invisible to the mutation runner. The red/green pair above is the regression proof (same triage class as usage-fetch in batch 5 and powerline in batch 4).Notes
docs/review-remaining-prs-2026-10-12.mdexists in the working tree of the main checkout — left alone deliberately.Commits
🤖 Generated with Claude Code