Make the session-trace tests hermetic, and cover the Bun path in CI#157
Open
justinstimatze wants to merge 9 commits into
Open
Make the session-trace tests hermetic, and cover the Bun path in CI#157justinstimatze wants to merge 9 commits into
justinstimatze wants to merge 9 commits into
Conversation
The tuning in fiorastudio#129 left the older detector spec tests behind — they still assume the stricter pre-tuning thresholds, so npm test fails on a fresh clone. This updates the fixtures and expectations to the current values. Tests only, no behavior changes.
…ion (fiorastudio#150) Some detectors are specializations of others. `echo_chamber`'s predicate is `load_bearing_vibes`' predicate plus three conjuncts (user speaker, assistant-only supports, no pushback), so every echo case is also a load-bearing case — one situation at two resolutions, not two competing findings. Selection takes candidates[0] within a category, so the general finding won purely on the order detectors run in. The per-anchor cooldown made it self-reinforcing rather than self-correcting: it keys on anchor_claim_id without regard to type, so emitting the general finding also blocked the specific one on that anchor. `echo_chamber` could not surface at all. `SUBSUMES` names the containment; `runAllDetectors` drops a general finding when a more specific one fired on the same anchor. Thresholds are untouched — Also fixes two defects fiorastudio#129 exposed in the same area: - `unchallenged_chain` fired on the unchallenged *suffix* of a challenged chain. The walk started from every node, so once the minimum length dropped 4→3 a suffix below the challenged pair became a candidate in its own right. It now starts only from maximal chains, matching its documented "anchors on the head" intent. - The "below threshold" / "below minimum length" fixtures hardcoded edge counts that fiorastudio#129's retuning turned into *at*-threshold cases, so they went green while testing nothing. They derive from REASONING_CONFIG now. Three fixtures were mislabeled: they seeded a user vibes claim backed by assistants with no pushback — a textbook echo chamber — and read as load-bearing only because of the ordering bug. They now use an assistant-authored anchor so they test what they claim to. Builds on fiorastudio#151, which fixes the same five failures by pinning the current behaviour and explicitly defers the real fix here. The two fixture sets conflict and should not both be merged as-is. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WFi4A9eG3CWPjDDMvdmUfy
…ntical bones The CC-compat rescue path reproduces Claude Code's `Bun.hash()` by shelling out to `bun -e`, passing the userId as a trailing argument and reading it back as `process.argv[1]`. Bun does not forward trailing arguments to `-e` scripts: inside the eval, `process.argv` is `['bun', '<cwd>/[eval]']` and nothing more. `process.argv[1]` was therefore a constant path string, identical on every call, and the userId never reached the hash. Measured on master with Bun installed: 300 distinct userIds produced exactly one distinct stat vector. Rarity, species, eye and stats were the same for every rescued companion. Silent because the roll stayed perfectly deterministic — which is what the determinism test checks — and because the FNV-1a fallback path, used on machines without Bun, was always correct. The input now travels through the environment instead of argv, preserving the no-string-interpolation property that argv was chosen for. After: 200/200 distinct, still deterministic per userId. Already-rescued companions keep their stored bones, so no migration is needed, but every existing rescue was a clone of the same roll. The existing test compared two userIds, which reads as an unlucky fixture precisely when the bug is total. Replaced with a 40-id spread assertion. Not covered by any open PR — fiorastudio#152 and fiorastudio#153 fix the other fresh-clone failures. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WFi4A9eG3CWPjDDMvdmUfy
…test suite resolveSessionTrace walks ~/.claude/projects, ~/.claude/sessions and ~/.codex/sessions to map a session id back to its cwd. It needs the first JSONL record (Claude) or the first dozen (Codex), but read every candidate file in full to get them, so cost scaled with total transcript volume instead of with the header actually parsed. On a machine with real history — 646 files, 8.5 GB — that meant reading 8.5 GB to extract a few hundred kilobytes of headers. reasoning/session-trace.test.ts never finished, and because vitest runs files serially it took the rest of the suite down with it; `npm test` appeared to hang with no failing test to point at. Fresh clones and CI runners have no transcript history, which is why this went unnoticed. Reads are now bounded to a 32 KiB head per file, enough for a first JSONL record by a wide margin. Whole-file .json parsing keeps its own 256 KiB ceiling since it is parsed as a single object. A truncated read discards its trailing partial line rather than trying to parse half a record. Resolution is unchanged — the real workspace still resolves to the same cwd and source, in 4ms. The test goes from a >90s hang to 2s. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WFi4A9eG3CWPjDDMvdmUfy
The repo has no CI — .github/workflows contained only the stats updater — so nothing has been running the test suite on PRs or merges. fiorastudio#129 shipped a red suite in May and it stayed red until a contributor cloned the repo in July and ran npm test by hand (fiorastudio#150, fiorastudio#151, fiorastudio#152, fiorastudio#153). Node 20 (the engines floor) and 24, npm ci / build / test. `npm ci` compiles better-sqlite3 from source when no prebuild matches the runner ABI, which is the same path users hit on install, so a broken native build fails here too. MERGE ORDER: this gates every PR the moment it lands, and the suite is not green yet. fiorastudio#152 fixes the two doctor check-count failures and fiorastudio#153 fixes the two Penguin frame-width/pose failures; those are the only four remaining, and both PRs are open. Merge them first, or this red-gates unrelated work. Note that CI could not have caught the bug fixed in the preceding commit: a fresh runner has no ~/.claude transcript history, so resolveSessionTrace's unbounded scan costs nothing there. Green CI is not evidence the suite is sound on a real machine. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WFi4A9eG3CWPjDDMvdmUfy
…veloper's resolveSessionTrace takes an optional `home`, defaulting to homedir(), and the tests point it at a temporary fixture tree they build and tear down. Before, these tests searched the real ~/.claude. That made the outcome a property of the machine rather than the code: on a CI runner with no transcript history they passed in milliseconds and asserted almost nothing; on a working machine they read every transcript in full — 8.5 GB across 646 files here. Because the scan is synchronous, it blocked the vitest worker so the 5s test timeout could not fire, and the suite presented as a hang with no failing test to point at. Bounding the reads made that survivable; this makes it absent. The fixture also lets the tests assert behaviour they previously could not, since nothing about the developer's home was known: a Claude transcript resolves to its cwd and label, a non-matching transcript is ignored, Codex metadata is used as fallback with its session id, a malformed session id yields empty fields, and a match on the first record survives an unparseable tail. Deliberately NOT asserted: that reads are bounded. A file small enough for a unit test is cheap to read whole, so no timing assertion could distinguish the bounded implementation from the unbounded one. That is a performance property and pretending to test it would be worse than leaving it untested. Test time for this file: >90s (blocking) to 12ms. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WFi4A9eG3CWPjDDMvdmUfy
ccHashString shells out to `bun` to reproduce Claude Code's Bun.hash(), and falls back to FNV-1a when Bun is not installed. A bare GitHub runner has no Bun, so it only ever exercised the fallback — the Bun branch would have been covered by CI in name only. That is not hypothetical. Measured against master, on the identical-bones bug fixed earlier in this branch: master, Bun on PATH → 1 failed (bug visible) master, Bun absent → 50 passed (completely green) So the CI added in the preceding commit would have reported success on code where every rescued companion rolled the same rarity, species, eye and stats. One matrix leg with Bun installed executes the branch for real. Verified that the fixed code passes both ways, so the leg tests the path rather than just pinning an environment. Excluded from the Node 20 leg: the Bun branch does not vary with Node version, so a second copy would buy nothing. The general lesson is worth stating, because it generalises past this one path: a conditional gated on a tool the runner lacks is not covered by CI, it is hidden from it, and the green check actively misleads. Any future branch taken only when some external binary is present needs a leg that provides it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WFi4A9eG3CWPjDDMvdmUfy
The workflow only reads the tree, but declared no `permissions` block, so it inherited the repository default. On a public repo with 21 forks that means a PR-triggered run could hold a wider token than it needs, and the scope would widen silently if the repository default ever changed. `contents: read` is declared explicitly instead. This also matches the convention already set by update-stats.yml, which declares `contents: write` for the one job that genuinely needs it. Adding workflow_dispatch for parity with that workflow's manual trigger. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WFi4A9eG3CWPjDDMvdmUfy
Each rollWithCCCompat spawns a `bun` subprocess. That is ~9ms warm but swings to seconds under load, and these tests were already timing-fragile for it — on master, before any change here, `returns valid bones` took 36s against a 15s budget in one run. A 40-id sample added 40 more spawns to that file and made neighbouring tests likelier to blow their timeouts. Twelve ids keep all the discrimination that matters. The bug collapsed every id to a single stat vector, so the assertion fails at 1-of-12 with enormous margin; nothing is gained by sampling forty. The underlying fragility is untouched and worth a separate look: a unit test should probably not shell out per call at all. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WFi4A9eG3CWPjDDMvdmUfy
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
2 participants
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.
Stacked on #156, which is stacked on #151 — so this shows their commits too until they merge. The five commits that are mine are the top five.
Follow-up to the two bugs in #156. Both were invisible to the checks the project had, in opposite directions, and this closes those gaps rather than just fixing the symptoms.
The session-trace tests now use a fixture home
resolveSessionTracetakes an optionalhome(defaulting tohomedir()), and the tests build a temp fixture tree instead of reading the developer's real~/.claude. Both production callers are single-arg, so nothing else changes.Reading the real home made the outcome a property of the machine: on a runner with no transcript history the tests passed in milliseconds and asserted almost nothing, and on a working machine they read gigabytes. Bounding the reads in #156 made that survivable; this makes it absent. The fixture also lets the tests assert things they previously couldn't, since nothing about the developer's home was knowable — resolution to cwd and label, a non-matching transcript being ignored, Codex fallback with its session id, malformed ids. 2 tests → 6, and >90s → 12ms.
A Bun leg in the CI matrix
ccHashStringshells out tobunand falls back to FNV-1a when it's absent. A bare runner only ever takes the fallback, so the Bun branch would have been covered in name only. Measured against master, on the identical-bones bug:So the CI in #156 would have reported success on that code. One leg with Bun installed executes the branch for real. Excluded from the Node 20 leg since it doesn't vary by Node version.
The general point is worth keeping in mind past this one path: a branch gated on a tool the runner lacks isn't covered by CI, it's hidden from it, and the green check actively misleads.
Two smaller things
permissions: contents: readon the workflow — it only reads the tree, andupdate-stats.ymlalready sets its scope explicitly, so this matches. Plusworkflow_dispatchfor parity.And I trimmed my own spread test from 40 sampled userIds to 12. Each roll spawns a
bunsubprocess, and that file is already timing-fragile for it — on masterreturns valid bonestook 36s against a 15s budget in one run. Twelve keeps all the discrimination that matters, since the bug collapsed every id to one vector.Worth a separate look sometime: a unit test probably shouldn't shell out per call at all.
🤖 Generated with Claude Code
https://claude.ai/code/session_01WFi4A9eG3CWPjDDMvdmUfy