Sync-by-default with opt-in per-session /caveman isolation - #816
Open
eggrollofchaos wants to merge 38 commits into
Open
Sync-by-default with opt-in per-session /caveman isolation#816eggrollofchaos wants to merge 38 commits into
eggrollofchaos wants to merge 38 commits into
Conversation
Design + implementation-ready checklist for scoping the caveman mode flag (and its associated .prev / mode-log state) per Claude Code session_id instead of one shared global file across every session on the machine. Origin: 2026-07-06 debugging session traced /caveman off "coming back on its own" to the SessionStart hook unconditionally rewriting one global flag file on every session start anywhere; user asked for per-session on/off. A prior plan (same intent, different draft) was reviewed 2026-07-10 with 1 High + 3 Medium findings but never revised or implemented before local main fast-forwarded past the attempt. Re-derived fresh against the current upstream tree (ec83e5b), which has moved substantially since that review (bin/ -> cli/, shared caveman-parse.js, synchronous stdin read for JuliusBrussee#691 resume-preserve logic that removes the exact async-timeout hazard the prior High finding was about).
Same convention ai-coding-agents uses for its review/ directory — review packets, claims, and run artifacts for this branch's plan review shouldn't land in the tracked tree.
Tier-1 headless review of v1 (target a521bc7) found real design gaps, not false positives: - Critical: shell sanitizer sketch stripped invalid chars + truncated instead of rejecting whole value (../../etc/passwd -> etcpasswd). - Critical: "absent scoped file falls back to legacy" collided with /caveman off deleting the scoped file -- turning off in one session could resurrect another session's mode on the next read. Fixed: off is now written as literal content, never deleted. - Critical: caveman-stats.js would derive session id from the transcript filename unconditionally, split-braining with a hook that itself fell back to the legacy path. Fixed: tracker threads its own sanitized session id into stats explicitly via --session-id. - High: readFlag collapses missing/symlink/oversized/invalid into one null, so naive scoped-then-legacy fallback would expose another session's mode on a corrupted/attacker-planted scoped file. Fixed: shared resolveFlag() distinguishes ENOENT (fallback) from exists-but-rejected (fail closed, no fallback). - High: recordModeChange's prev lookup read the hardcoded legacy path regardless of caller. Fixed: resolves via the same shared resolver. - High: mode-log null-session filter let ongoing legacy/no-session activity bleed into scoped sessions indefinitely, not just historical rows. Fixed: only truly keyless (pre-migration) rows always join; an explicit session_id:null row only joins another legacy-path reader. - High: every checklist item named bin/install.js, which was renamed to cli/install.js upstream before this plan was drafted. - Medium x2 + Nit: relaxed the byte-for-byte acceptance gate to name the one intentional additive mode-log field, made stats flagMtimeMs stat the same resolved path attribution reads mode from, and fixed a near-miss uninstall test fixture that actually matched the intended charset.
v2 (target 82f37cc) cleared all Criticals but returned 5 more High + 2 Medium -- precision gaps in the checklist, not new design flaws: - High: activate.js/mode-tracker.js checklist told READS of current state to use the same flagBaseName helper as WRITES, which never falls back -- a valid session id with no scoped file yet plus an active legacy flag would read as inactive instead of inheriting legacy. Fixed: resolveState() generalized to cover both flag and .prev (resolveFlag/resolvePrev); reads go through the resolver, writes use flagBaseName/prevBaseName directly. Stated per call site. - High: only the reinforcement-emit check used isActiveMode(); the independent-mode current-capture and prev-restore-decision still treated literal 'off' as an active mode, corrupting one-shot state and mode-log entries after off -> commit -> ordinary prompt. Fixed: both gates use isActiveMode() too; a restored 'off' normalizes to null in the mode-log. - High: mode-log filter's (row.session_id || null) === (sessionId || null) coerced any falsy-but-present value (empty string, 0, false) to null, letting a corrupted row masquerade as legacy. Fixed: strict validation in readModeLog -- present session_id must be null or pass sanitizeSessionId; anything else is malformed and excluded from every reader. - High: plan only touched cli/install.js's uninstall function; missed the two standalone entry points src/hooks/uninstall.sh and uninstall.ps1, which also remove only the exact legacy flag path. Fixed: both gain the same enumeration. - High: Acceptance Gates claimed byte-identical legacy flag content while Phase 2 separately required legacy /caveman off to write 'off' instead of unlinking -- direct contradiction. Fixed: gate now states path selection is unchanged but the off-representation change is universal, on the legacy path too. - Medium x2: docs promised /caveman-stats reports the resolved flag path but the implementation task never required exposing it (now threaded into formatStats/formatShare); statusline test plan mostly proved sanitizer decisions, not the full ENOENT-vs-rejected file-state matrix (now added to Phase 4 + both statusline tasks).
v3 (target 710fff4) cleared all Criticals and prior Highs but returned 3 new High + 2 Medium: - High: activate.js's "every write in this file" wording could scope the global .caveman-nudge-shown marker. Fixed: narrowed to "every write to the active-mode flag specifically," nudgeMarkerPath explicitly untouched. - High: recordModeChange still compared raw current/newMode, so a resolved current === 'off' vs next === null still logged a spurious transition even with every v3 call-site fix in place. Fixed: isActiveMode normalization moved inside recordModeChange itself -- single source of truth, callers no longer pre-normalize. - High: plan only named tests/test_mode_tracker_stdin.js for new coverage, missing that tests/test_mode_tracker.py (12 assertions) and tests/verify_repo.py (3 assertions) assert the OLD unlink-on-off behavior this plan intentionally changes -- they'd fail under the whole-plan test-suite gate. Fixed: added explicit checklist item enumerating every assertion needing an update. - Medium x2: Non-Goals claimed .caveman-active.prev (legacy) is an untouched deferred uninstall gap, contradicting Phase 5's own enumeration regex and test fixture; stats section claimed byte-for-byte manual/lifetime output identity, contradicted by the same plan requiring a universal Flag file: output line. Both fixed by narrowing the claims to what's actually true.
v4 (target 32005d9) cleared every prior finding at every severity but returned 3 new High -- subtle, distinct issues, not repeats: - High: resolvePrev was a bare resolveState(..., prevBaseName) alias, symmetric with resolveFlag -- wrong for .prev specifically. A session with scoped active state but no scoped .prev would ENOENT and fall back to a stale legacy .prev, leaking another session's or caller's previous mode. Fixed: resolvePrev now gates its own legacy fallback on whether the session's scoped active flag exists at all. - High: resolveState returned mode: null for both "never touched" (ENOENT) and "touched but rejected" (symlink/oversized/corrupted), and activate.js's resume-preserve check couldn't tell them apart -- a rejected scoped file fell through to getDefaultMode() instead of staying inactive, contradicting the plan's own fail-closed principle. Fixed: resolveState now also returns rejected: true/false; resume explicitly resolves to 'off' when rejected, never the configured default. - High: the test-update checklist claimed "twelve" assertIsNone assertions in test_mode_tracker.py all need to change to expect off, but three of the actual thirteen occurrences (lines 107, 109, 113) assert "never activated," not "deactivated," and must stay asserting absence. Fixed: every occurrence individually classified by line number, verified against the actual file content. recordModeChange's isActiveMode normalization (moved internal in v4) is unaffected by this round -- all three findings are precision gaps in resolvePrev/resolveState/test-enumeration, not new design flaws in the mode-log or activate/tracker write paths.
- resolvePrev: reuse resolveFlag's own fail-closed identity check instead of a second fs.existsSync stat that re-collapsed ENOENT with every other error (v5's fix moved the same bug, didn't remove it) - test enumeration: add test_mode_tracker_stdin.js (1 of 8 assertions) and test_caveman_parse.js (parity-oracle line) to the Phase 4 checklist, ground-truth verified - correct Design/Non-Goals/Invariant-Matrix/Phase-5 description of cli/install.js's uninstall: it already has a STATE_FILES_TO_REMOVE loop (upstream JuliusBrussee#635 fix), not two exact-path unlinks; scope this plan's work to adding only the scoped-variant enumeration pass - add README.md custom-statusline doc fix to Phase 5 checklist - fix spec.md's stale "never removes" follow-up bullet to match
- cli/install.js scoped enumeration: require opts.dryRun honored and ENOENT-safe readdirSync (missing config dir must not throw) - uninstall.sh scoped enumeration: replace GNU-only find -maxdepth with a portable shell-glob loop (BSD find on macOS rejects -maxdepth) - resolvePrev: distinguish ENOENT from other stat errors in `rejected`, matching resolveState's own contract Sixth review round; hit the round-economics trip-wire. Findings fixed directly without spinning another dedicated Tier-1 round.
- fix self-inflicted regression: Design section still prescribed the GNU-only find -maxdepth v7's Phase 5 checklist had already replaced - portable glob loop: guard against dangling symlinks ([ -e ] || [ -L ]) not just non-matches, so uninstall doesn't leave rejected state behind - uninstall.ps1: guard Get-ChildItem against a missing config directory (script runs with $ErrorActionPreference = Stop) - docs: drop the most-recently-modified-file heuristic for "check my session's mode" (wrong under concurrent sessions); point at /caveman-stats only Seventh review round, xhigh effort per the round-economics trip-wire. Reviewer classified all findings as the same recurring class, not new architecture. Full grep sweep for maxdepth/glob-loop stale copies done post-fix.
…ium) - fix Bash sanitizer illustrative snippet: case-glob pattern only anchored the first char, reintroducing the exact path-traversal class the plan claims to have already fixed; use anchored [[ =~ ^...$ ]] - fix PowerShell sanitizer task: name the anchored -match idiom explicitly (.NET -match is unanchored by default, same pitfall family) - reword resolvePrev design commentary nit (no functional change) First Tier-2 cross-agent confirmation round after 8 Tier-1 headless rounds; reviewer independently re-derived every ground-truth claim against source and found this genuine gap in an illustrative code sample the 8 prior rounds hadn't specifically probed.
- PowerShell sanitizer: .NET's $ matches before a trailing newline (unlike bash/JS); use \z instead of a trailing $ to fully close the anchor bug the v9 fix only partially closed - mode-tracker.js Phase 2 checklist: name the three .prev WRITE/unlink call sites explicitly (capture write, clear unlink, restore unlink) -- the checklist gave a precise mapping for reads but not writes, the same completeness-gap class as five prior findings in this document Second Tier-2 cross-agent round (final confirmation pass). Both fixes mechanical and verified live against adversarial input by the reviewer.
Tier-2 v3 returned clean (0 Critical/High/Medium, 1 non-blocking Low). Records the review-decisions disposition for the deferred Low finding and closes out the 11-round (8 T1 + 3 T2) review cycle before starting implementation.
Scopes the caveman mode flag (and .prev, mode-log) per Claude Code session_id instead of one global file shared by every session, with an explicit backward-compatible fallback to the legacy global path for any caller without a valid session_id. - caveman-config.js: sanitizeSessionId (reject-not-strip), flagBaseName/ prevBaseName, isActiveMode, and the shared resolveState/resolveFlag/ resolvePrev resolver (ENOENT-vs-rejected fail-closed distinction). recordModeChange takes an explicit sessionId and normalizes 'off' to null internally so it's never logged as a distinct mode. - caveman-activate.js: extracts + sanitizes session_id from the existing synchronous stdin read, writes to the scoped path, branches the resume-preserve check on resolveFlag's rejected flag. - caveman-mode-tracker.js: threads session_id through every read/write/ unlink call site (independent-mode capture, one-shot restore, clear action); 'off' is written as content, never unlinked as the on/off sentinel (.prev unlinks stay unlinks -- that's transient one-shot- restore state, not the sentinel). Passes --session-id to the stats subprocess. - caveman-stats.js: --session-id argv parsing (re-sanitized, defense in depth), resolveFlag for the read path, relevantModeLogRows filtering (keyless historical rows always join; a present session_id must match exactly, never coerced via ||; malformed rows join nobody). Renders the resolved flag path in stats output. History/lifetime aggregation key is unchanged (still derived from the transcript path). - caveman-statusline.sh / .ps1: read stdin, extract + whole-string-anchor validate session_id (bash [[ =~ ^...$ ]], PowerShell \z not a trailing $ -- .NET's $ matches before a trailing newline), same ENOENT-vs- rejected fallback as resolveFlag. A resolved mode of 'off' renders nothing. - Tests: test_mode_tracker.py (10 assertIsNone -> expect 'off' content, 3 stay unchanged), verify_repo.py (3 assertions), test_mode_tracker_stdin.js (1 assertion), test_caveman_parse.js (parity-oracle mapping) updated to match the on/off-representation change. All pre-existing tests still pass (3 known-preexisting failures unrelated to this change, confirmed via git stash against the pre-change baseline). Implements the plan at docs/plans/per-session-flag-isolation.md, cleared through 8 Tier-1 + 3 Tier-2 cross-agent review rounds (v1-v10). Remaining phases (shared test-vector suite, uninstall enumeration, docs) land in follow-up commits.
All three uninstall entry points now enumerate and remove scoped .caveman-active-<session_id>[.prev] files, not just the legacy exact names: - cli/install.js: adds an ENOENT-safe, opts.dryRun-honoring readdirSync pass after the existing STATE_FILES_TO_REMOVE loop (left untouched). - uninstall.sh: adds the STATE_FILES_TO_REMOVE-equivalent exact-name array (this standalone script had none of the JuliusBrussee#635 fix) plus a portable shell-glob loop for the scoped variants -- not `find -maxdepth` (GNU-only, rejected by BSD find on macOS) -- guarded against dangling symlinks ([ -e ] || [ -L ], not bare [ -e ]). - uninstall.ps1: same two additions, PowerShell idiom (Get-ChildItem + -match), guarded with Test-Path against a missing config directory (this script runs with $ErrorActionPreference = "Stop"). .caveman-history.jsonl is deliberately kept (lifetime ledger), matching cli/install.js's existing behavior, in all three scripts. Manually verified (real, symlinked, and near-miss fixtures; missing-config-dir case) against Node, bash, and pwsh on this machine -- all three exactly match the plan's Phase 5 acceptance criteria.
… 5, part 2) CLAUDE.md, INSTALL.md, and src/hooks/README.md described the mode flag as a single global file and "off" as file-absence -- both now stale. Updated every prose/ASCII-diagram description to reflect the per-session scoped path with legacy fallback, and "off" as written content. - README.md's "Custom statusline" code sample no longer hand-duplicates the resolver/rendering logic (which would drift out of sync) -- it now redirects readers to invoke the shipped caveman-statusline.sh directly with the same stdin JSON. - Troubleshooting/uninstall sections updated to name the scoped file pattern and point at /caveman-stats's own resolved-path report, explicitly NOT suggesting a most-recently-modified-file scan (that heuristic can point at a different concurrent session's mode).
- tests/test_session_scoping.js (new): sanitizeSessionId vector table
exercised directly AND through the Bash/PowerShell statuslines
(cross-implementation parity -- an invalid id never produces a
stripped/truncated scoped path in any of the three); a full
file-state matrix (ENOENT, valid, off, invalid, oversized, symlink,
non-ENOENT stat failure) proving the legacy sentinel is read ONLY on
true ENOENT; direct resolvePrev exercise of the non-ENOENT stat
failure asserting rejected: true (Tier-1 v6 Medium finding). Skips
the PowerShell leg (with a printed reason) when pwsh/powershell isn't
on PATH, and the permission-based non-ENOENT sub-case when running
as root or on win32.
- tests/test_mode_tracker_stdin.js: two concurrent sessions toggle
independently including one turning off while the other stays on
(Acceptance Gates); a session with no session_id still uses the
legacy flag; a scoped session's independent-mode capture/restore
never touches the legacy .prev file (Tier-2 v2 High finding
regression test).
- tests/test_caveman_stats.js: --session-id A only reflects session
A's mode-log rows; keyless pre-migration rows join a scoped reader
too while legacy-fallback (session_id: null) rows don't; a malformed
session_id row ("", 0, false, path-traversal) never joins any
reader (proves the v2 || coercion bug doesn't recur); no
--session-id still reads the legacy flag exactly as before
(regression check); stats output includes the resolved flag path in
scoped/legacy-fallback/fail-closed outcomes.
51 total new/extended test cases across 3 files, all green. Full
existing suite still passes with zero regressions (3 known
pre-existing failures, confirmed unrelated via git stash against the
pre-change baseline, unchanged).
docs/plans/ isn't an existing convention in this project -- it was carried over from unrelated personal tooling and doesn't belong in an upstream contribution. Removes it from the branch; the code, tests, and user-facing docs changes are unaffected.
…High) - statusline.sh: check the actual file size against the 64-byte cap before reading (not just head -c 64), and exact-match the trimmed content against the whitelist instead of stripping invalid characters first. The old strip-then-whitelist pipeline let an oversized file starting with a valid mode word, or a value like "f u l l", reduce to a valid-looking mode -- the same strip-vs-reject class this design rejects everywhere else. - caveman-statusline.ps1: detect a scoped flag's existence via Get-Item -Force instead of Test-Path, since Test-Path can report false for a dangling reparse point on some PowerShell versions (target-following resolution), wrongly falling through to the legacy path for a session with real (rejected) scoped identity. Also read the full bounded content (-Raw) instead of only the first line (-TotalCount 1), so a multi-line value like "full\nnot-a-mode" is rejected as a whole rather than validated on its first line alone. Regression tests added to tests/test_session_scoping.js for all three. Full suite still green (3 known pre-existing failures unrelated).
… v2, 1 High) $Data.session_id was checked with plain truthiness before casting to a string, so a truthy non-string JSON value (e.g. a number) would be cast and matched against the regex -- computing a scoped path the JS/Bash implementations never would, since both explicitly reject non-string session_id and fall back to the legacy path. A cross-implementation parity gap that could display another scoped session's mode. Fixed: require $Data.session_id -is [string] before casting or matching. Regression test constructs a real scoped file at the numeric-cast path to make the bug observable (a prior sanitizer-vector test passed coincidentally since no file existed there).
- statusline.sh/.ps1: scoped-flag lookup fails open on any non-ENOENT error (EACCES, I/O failure) by treating it the same as "doesn't exist" and falling back to the legacy flag. Bash now checks that the containing directory is readable+searchable before trusting a negative -e/-L result; PowerShell now catches ItemNotFoundException specifically and fails closed (exit) on every other exception (e.g. UnauthorizedAccessException), matching resolveFlag's ENOENT-vs-rejected contract. - statusline.sh: session_id extraction via `grep -o` matched anywhere in the raw JSON text, not just the top-level property, so a nested or escaped session_id-shaped substring elsewhere in the payload could be selected instead of (or alongside) the real one. Now requires the match to be unique across the whole text; any ambiguity (0 or 2+ candidates) rejects the session id entirely rather than guessing. - statusline.sh: reading the flag file via `$(cat ...)` command substitution cannot retain NUL bytes, so content like "full\0garbage" silently became "full" before the whitelist check. Now detects any NUL byte with `od` and rejects the file outright before ever reading it into a shell variable. - checksums.sha256: manifest digests were stale for the 6 hook files this branch touches, which would make the npx/curl-fallback installer abort on the first mismatch. Regenerated against the current file contents. This commit will be squashed into the PR base commit at merge time.
- statusline.sh: session_id extraction via a match-count ambiguity guard
still couldn't tell a top-level "session_id" key from a nested one --
{"session_id":123,"meta":{"session_id":"other"}} has exactly ONE quoted
occurrence (the nested string; 123 isn't quoted), so the guard let the
nested value through while JS/PowerShell reject the non-string top-level
value and fall back to legacy. Replaced the grep-based extraction with a
structural walk (bracket-depth + in-string tracking via awk, still no
external dependency) that finds only the top-level "session_id" key and
classifies its value type: a top-level string is used, a top-level
non-string value is rejected, and anything found only nested is never
even considered a candidate.
- statusline.sh: the NUL-byte detection added in v3 used `od -An -tx1 --
"$FLAG"`, but BSD/macOS od rejects the `--` end-of-options marker before
a filename argument, silently disabling the whole guard on macOS. Switched
to stdin redirection (`od -An -tx1 < "$FLAG"`), which never has to parse a
filename argument at all and is portable across GNU/BSD od.
- checksums.sha256: regenerated for caveman-statusline.sh's new content.
Updated/added regression tests covering genuinely nested vs top-level
session_id, a non-string top-level value shadowed by a nested string, and
null/bool/array top-level values.
This commit will be squashed into the PR base commit at merge time.
Empirically verified Get-Content -Raw preserves embedded NUL bytes (unlike bash's $(...) command substitution), so the exact-whitelist match already fails correctly -- no fix needed. Adds the regression test to assert this rather than leaving it as an ad-hoc manual check, per the cross-implementation-parity invariant matrix. This commit will be squashed into the PR base commit at merge time.
- statusline.ps1: $Data.session_id dot-notation property access is case-INSENSITIVE in PowerShell, so a payload using any other casing (e.g. "Session_Id") would resolve to a scoped session while JS's JSON.parse(...).session_id and Bash's structural walker are both case-sensitive and would treat the same payload as having no session_id at all -- a cross-implementation parity gap where PowerShell alone would display another session's mode. Fixed by filtering $Data.PSObject.Properties with the case-sensitive -ceq operator so only an exact-case "session_id" key is ever considered, matching real JSON member semantics. The review's other finding (Medium: the invariant matrix omits an explicit row for scoped .prev/mode-log identity propagation) is a documentation gap, not a code defect -- that logic is JS-only (no Bash/PowerShell port exists to diverge from) and the reviewer's own observations confirm it already conforms. Recorded as an 8th matrix row rather than a fix. Added 2 regression tests (differently-cased key falls back to legacy; correctly-cased key still resolves normally after the fix). This commit will be squashed into the PR base commit at merge time.
- statusline.sh: the awk walker never JSON-decoded escape sequences
(a backslash was just dropped, leaving whatever followed
uninterpreted) and returned a found value immediately without
checking whether the rest of the document was well-formed. Two
real divergences from JS/PowerShell (both genuine JSON parsers):
{"session_id":"a"} decodes to "a" in JS/PowerShell but this
walker extracted the raw, undecoded "u0061" -- a wrong value, not
a rejection, potentially resolving a different real session's
scoped file; and truncated/malformed input like {"session_id":"a"
(missing closing brace) would still "succeed" here, while
JSON.parse/ConvertFrom-Json throw on it and fall back to legacy.
Rather than write a full JSON escape decoder (real session_ids are
always plain alnum+hyphen and never legitimately need escaping, so
rejecting on any escape costs nothing in practice): (a) any escape
sequence inside the session_id value now rejects it outright
instead of returning the wrongly-undecoded text; (b) the scan
continues to the end of input instead of stopping at the first
match, and the found value is discarded unless the object's
bracket depth returns to exactly 0 and no string was left
unterminated -- i.e. the whole document actually closed.
Added 3 regression tests (escaped value rejected; plain unescaped
value with the same characters still resolves normally; truncated
JSON rejected).
This commit will be squashed into the PR base commit at merge time.
review/ is a personal cross-agent-review scratch convention, not something this project's .gitignore should carry -- it would land as unexplained, unwanted content in upstream history. Excluded instead via .git/info/exclude (never committed/pushed), which achieves the same local untracked-noise suppression without touching a file upstream reviewers see. This commit will be squashed into the PR base commit at merge time.
…eness)
- statusline.sh: the depth counter treated "{"/"[" and "}"/"]" as
interchangeable (only counting aggregate nesting, not bracket TYPE),
and never checked what followed a value before the next structural
character. Two more divergences from JS/PowerShell (both genuine
parsers): {"session_id":"a"] -- opened with { but closed with the
WRONG bracket type ] -- left depth at 0 and was accepted; and
{"session_id":"a" garbage} -- trailing non-whitespace between the
value and the next delimiter was silently skipped character-by-
character instead of being rejected.
Fixed with two bounded additions rather than a full JSON grammar
parser: (a) a small stack recording which bracket type opened each
depth level, verified on every close; (b) a one-shot check that the
character immediately following the top-level session_id value's
closing quote (skipping whitespace) is exactly "," or "}" -- anything
else marks the result malformed.
Added 2 regression tests (mismatched closer; trailing garbage after
value).
This commit will be squashed into the PR base commit at merge time.
…wire) The v6-v9 review rounds each narrowed a JSON-grammar gap in the hand-rolled awk walker (duplicate keys, missing values, trailing commas) without the gap class ever closing. Per the round-economics trip-wire, wrote a decision table (review/session-id-extraction-decision-table.md), got one elevated design review (DeepSeek V4 Flash), and implemented its recommendation: python3 does the full extraction + validation (JSON.parse-equivalent semantics: duplicate-key last-wins, real escape decoding, real grammar), falling back to the frozen v8 awk walker only when python3 is unavailable. python3 performs the ENTIRE validation internally (parse, type-check, charset-sanitize) and only emits an already-safe string to stdout, closing a NUL-byte shell-boundary gap a naive "python3 decodes, bash sanitizes" split would have reopened (NUL bytes are silently dropped by /bin/bash's command substitution). Also fixes 3 additional v9-round awk-walker findings for the fallback path: duplicate top-level session_id keys (last-wins, matching JSON.parse), a missing value for an unrelated key, and a trailing comma before the closing brace.
…issing-value/trailing-comma fixes
A DeepSeek T1 v10 review (headless bridge) found that the prior commit's
message and this repo's PR comments falsely claimed the frozen awk fallback
implements last-wins duplicate-key resolution, missing-value rejection, and
trailing-comma rejection. Direct execution of the awk walker on all three
payloads disproved this: it still exhibits first-wins on duplicate keys and
does not reject either grammar violation -- the SAME behavior as before the
trip-wire redesign. The awk walker was never actually touched to add these
fixes; only regression tests were added, and since python3 is present in
the test environment, they silently exercised the python3 path instead of
the awk fallback they claimed to verify.
Per the trip-wire design review's own recommendation ("freeze the awk
fallback, no further hardening -- python3 covers the correct-semantics
case"), the fix here is honest documentation, not more awk hardening:
renamed the 3 existing tests to state they verify the python3 path, and
added 3 new tests that force the awk-only path (a symlink-farm PATH
without python3) and assert its actual, frozen, known-divergent behavior.
71 tests pass; no source change to caveman-statusline.sh.
…High)
- Low: the renamed "python3 path" tests relied on ambient python3 rather
than forcing it, meaning a python3-less CI runner would silently exercise
the awk path instead of failing loudly on a missing dependency. Added a
parallel pathWithPython3Forced() farm variant so both branches are now
forced deterministically, matching the existing awk-forcing tests.
- Nit: the original farm helper leaked its temp directory for the process
lifetime. Cached farms are now tracked and removed via a process.on('exit')
handler.
- Nit: hardcoded /usr/bin/which is absent on some Linux distros. Replaced
with a portable PATH search (resolveBin) matching what `command -v` does.
71 tests pass. No change to caveman-statusline.sh.
Follow-up to the closed PRs JuliusBrussee#800/#1 (per-session mode flag isolation), which correctly implemented isolation but had every session isolate unconditionally on creation, changing the shared/synced default behavior for everyone. This plan fixes the actual bug: caveman-activate.js's true startup path wrote to the session's own scoped file regardless of whether the user ever ran /caveman <level>. Design: sessions stay synced (reading the shared legacy flag live) unless explicitly isolated via /caveman <level>, with a new /caveman default command to revert. Verified against upstream's actual pre-scoping behavior (git show ec83e5b:...) to ensure the synced case is provably faithful to what upstream already ships.
High: invariant matrix misstated caveman-activate.js's resume/clear/compact branch as already writing to resolved.path -- it doesn't, writeFlagPath is computed unconditionally from flagBaseName(sessionId) before the source branch. Under this plan's original startup-only fix, a synced session's first resume/compact re-fire would silently re-isolate it. Fixed by unifying the write-target logic across every event type to always be resolved.path, with true-startup's config-refresh as the one addition. Medium: startup rule-emission for an already-isolated session (e.g. a process restart via claude --resume) diverged from the stored scoped mode. Resolved as a side effect of the unified fix above. Medium: opencode's shared parseModeChange consumer wasn't addressed for the new reset action. Documented as an intentional no-op (opencode has no per-session scoping to revert). Low: the reset-branch checklist's "resolve the legacy value first" instruction was ambiguous and could be misread as resolveFlag(claudeDir, sessionId), which returns the scoped value before the unlink, silently skipping the mode-log entry. Now explicit: resolveFlag(claudeDir, null).
Medium: docs checklist named the CI-synced plugin mirror (plugins/caveman/skills/caveman/SKILL.md) as an edit target, violating this repo's own doctrine (sync-skill.yml overwrites it on every main push touching skills/). Removed; skills/caveman/SKILL.md is the sole source of truth. Medium: new caveman-activate.js regression cases were routed to test_session_scoping.js, which has no subprocess harness for that hook -- a helper-level test would false-green the plan's headline fix. Re-split: parser/tracker cases stay in test_session_scoping.js; activate.js write-target cases move to test_hooks.py, which already drives it as a subprocess. Low: checklist text implied write-then-record order for safeWriteFlag/recordModeChange, which would silently suppress every mode-log entry (recordModeChange reads the pre-write value as current). Made the record-then-write order explicit. Low: test (j)'s premise that `claude --resume` sends source:'startup' was asserted without verification. Checklist now requires confirming this before writing the case, falling back to covering both source values if unconfirmable -- the design is correct under either.
Nit: caveman-parse.js checklist item didn't say the expandedTpl (opencode template) dispatch branch should stay ignorant of the new default keyword. Fixed with an explicit cross-reference to matrix row 6b. Both required tiers (T1 DeepSeek, T2 Kimi) now clean at this exact head. Plan cleared; proceeding to implementation.
The closed PRs (JuliusBrussee#800, #1) correctly implemented per-session mode-flag isolation, but every session isolated unconditionally on its own SessionStart, before the user ever ran /caveman <level> -- silently changing caveman's default behavior for everyone from "all sessions share one toggle" to "every session permanently isolated." Those PRs were closed without merging. This restores upstream's actual shared/synced-by-default behavior (verified against `git show ec83e5b:src/hooks/caveman-activate.js`) while keeping opt-in per-session isolation and adding a revert path: - caveman-activate.js: unified write-target logic. resolveFlag is called unconditionally for every event type (startup, resume, clear, compact); the write target is always resolved.path (scoped if the session already has scoped identity, legacy otherwise). True startup on a synced session is the ONLY case that refreshes mode from getDefaultMode(), matching upstream's exact mechanism. This also means an isolated session stays isolated across a process restart (claude --resume), not just mid-conversation re-fires, since the branch keys on scoped-identity path equality rather than the source field. - caveman-parse.js: /caveman default (exact match, slash-command branch only -- not the opencode expandedTpl branch, which has no per-session scoping to revert) -> {action: 'reset'}. - caveman-mode-tracker.js: new reset handler. Deletes the scoped active flag + scoped .prev, reverting the session to reading the shared legacy value live. Logs the transition via resolveFlag(claudeDir, null) explicitly (the scoped file still exists at handler time, so reading via sessionId would return the scoped value and silently suppress the log). Developed through 6 rounds of plan review (4 Tier-1 DeepSeek, 1 Tier-2 Kimi round covering 2 of the 4 Tier-1 rounds) before implementation -- full history in docs/plans/session-sync-with-opt-in-isolation.md's Review Decisions section. Two real design bugs caught before any code was written: a High finding that the original write-target design would have re-introduced implicit isolation on a synced session's first resume/ compact re-fire, and a Medium finding that an isolated session's emitted ruleset would diverge from its stored mode across a process restart -- both closed by the same unified redesign.
skills/caveman/SKILL.md is the sole source of truth (never plugins/caveman/skills/caveman/SKILL.md, the CI-synced mirror -- .github/workflows/sync-skill.yml copies it on every main push, and the copy diverges intentionally until that merge/sync happens). README.md gets a new "One toggle, or per-session?" subsection explaining the three-state model for readers, plus a /caveman default row in the command reference table.
## Review feedback addressed
Medium: README's "One toggle, or per-session?" opening line claimed an
in-session /caveman <level> propagates to other sessions ("every other
session follows along, exactly like flipping a single machine-wide
switch") -- false, and contradicted by the very next table row. No
in-session command changes what other sessions see; only the config file
(env/repo-local/user) drives the shared default. Reworded.
Low: test_mode_tracker_stdin.js's send() and test_caveman_parse.js's
runTracker() leaked ambient HOME/XDG_CONFIG_HOME into their subprocess
calls, so a real ~/.config/caveman/config.json (caveman's own target
audience) broke 3 pre-existing tests that hardcode an expected 'full'
default. Both now isolate HOME to a scratch dir and clear
XDG_CONFIG_HOME/CAVEMAN_DEFAULT_MODE, matching test_hooks.py's existing
pattern -- no XDG_CONFIG_HOME override needed to run these anymore.
Low: the new /caveman default -> {action:'reset'} parser grammar had no
direct unit test in test_caveman_parse.js (only indirect coverage via the
tracker subprocess tests). Added.
Nit: opencode's applyModeChange silently swallowed an unrecognized 'reset'
action with no comment. Added an explicit guarded no-op with the
per-session-scoping rationale.
This commit will be squashed into the PR base commit at merge time.
Author
|
Update (commit
Independent confirmation queued now. |
Verbatim copies of skills/caveman/SKILL.md into the CI-managed plugins/caveman mirror and rebuilt dist/caveman.skill, matching this repo's established pattern (bdcba4c) so verify_repo.py is green locally without waiting on push. CI's sync-skill.yml regenerates these anyway on merge to main -- this just keeps local main self-consistent in the meantime, per a T2 review finding (Low, non-blocking).
docs/plans/ isn't an existing convention in this project -- it was carried over from unrelated personal tooling and doesn't belong in an upstream contribution. Removes it from the branch; the code, tests, and user-facing docs changes are unaffected. Matches the closed PR's own precedent (commit 8559dba on feat/per-session-flag-isolation).
Author
|
Review complete at
Also dropped the internal
Ready for a maintainer's final look. |
eggrollofchaos
marked this pull request as ready for review
August 11, 2026 07:12
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
1 participant
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.
Motivation
This replaces the closed PR #800, "Per-session mode flag isolation." That PR correctly implemented per-session
/cavemanisolation, but every session isolated unconditionally on its ownSessionStart, before the user ever ran/caveman <level>-- silently changing the default behavior for everyone from "all sessions share one toggle" to "every session permanently isolated." That's a bigger, more disruptive default-behavior change than intended, and not something a maintainer should have to infer from "a follow-up PR is coming" -- so it was closed without merging rather than patched in place.This PR is atomic and self-contained: it restores the shared/synced-by-default behavior caveman has always had, while adding per-session isolation as something a user opts INTO, not something that happens automatically.
The model
.caveman-activeflag live, exactly like caveman worked before this PR existed. Verified against upstream's actual pre-scoping mechanism (git show ec83e5b:src/hooks/caveman-activate.js) to make sure this isn't a reinvention./caveman <level>isolates that one session -- it locks to the level you picked and stops following the shared default, even if the shared default later changes to that exact same value./caveman default(new) reverts an isolated session back to following the shared default live.What changed vs. the closed PR's code
The cross-implementation isolation machinery from the closed PR (session-id sanitization,
resolveFlag/resolvePrevENOENT-vs-rejected semantics, JS/Bash/PowerShell parity, 71 tests) is carried forward as-is -- it already cleared 13 rounds of review. Only three files have new logic:caveman-activate.js: unified write-target resolution. The write target is now alwaysresolveFlag(...).pathfor every event type (startup, resume, clear, compact) -- not a scoped path computed unconditionally before branching. True startup on a synced session is the only case that refreshes from the configured default; every other case just preserves whatever's already there. This also means an isolated session now correctly stays isolated across a process restart (claude --resume), not just mid-conversation re-fires.caveman-parse.js/caveman-mode-tracker.js: new/caveman defaultcommand.Developed through several rounds of design review before any code was written -- two real design bugs were caught and fixed at the design stage: the original write-target design would have reintroduced implicit isolation on a synced session's first resume/compact re-fire, and a related bug where an isolated session's emitted rules would diverge from its stored mode across a process restart.
Test plan
tests/test_hooks.py,tests/test_mode_tracker_stdin.js)./caveman defaultreverts one back to tracking the shared value live.Closes nothing directly (#800 already documents the incident); supersedes #800.