test(privacy): cover the toTrajectory() bypass; discover trajectories instead of listing them

[OM152002]

Closes #99. Follow-up to #37 (SC-02). Stacked on #109 — base is track1/b5-secret-scan-storage-state; retarget to main once that lands. Both edit docs/privacy/session-custody.md, which is why this is stacked rather than parallel.

Two holes with one root cause: a guarantee that depends on a specific call path or a hand-maintained list, rather than on the data shape itself.

1. The toTrajectory() bypass

assertNoLiteralSecrets() runs inside write(). toTrajectory() — the method that actually builds the object — has no equivalent, so a caller that serializes its return value directly gets no guard at all. Nothing in the tree does that today; nothing stops it either.

tests/unit/trajectory-guard.test.ts records a login-shaped task against a loopback fixture, takes the object from toTrajectory() without ever calling write(), and asserts it passes the same guard write() would have applied — in both pretty-printed and compact form, since "cookies"\s*: matches differently across the two.

The framing is deliberate: it asserts the property the guard protects, not that the guard ran. So it fails only if the recorder actually starts leaking, which is the thing worth knowing. A test that write() calls the guard would tell you nothing about the bypass.

2. Trajectories are discovered, not listed

validate-contracts.mjs schema-checked whatever was named in a hand-maintained extraTrajectories array. A new recording nobody remembered to add was never checked — including against additionalProperties: false, which is the mechanism that makes an accidental cookies field unrepresentable rather than merely discouraged. The failure was silent and grew with every recording.

Now discovered by walking experiments/**/trajectories/*.json. No new dependency: CI runs Node 20 and fs.glob is 22+, so it's a small recursive walk, consistent with the repo's no-new-deps convention.

Discovering zero is a hard failure, not "nothing to do":

no trajectories found under experiments/**/trajectories/*.json — discovery is
broken, or the recordings moved. Refusing to report clean.

An empty result means the walk broke, which is the same silent hole in a new shape.

Guard-proofing found a gap in my own tests

The bypass assertions are all "does not throw" — so weakening the guard makes them more likely to pass. They structurally cannot detect it.

Verified by deleting the "value" pattern from assertNoLiteralSecrets: all eight tests stayed green. Each of the guard's six patterns is now pinned individually, and re-running the same sabotage fails exactly one case:

× the guard still catches typed value

Plus the negative case that matters for that first pattern: {"parameters":{"password":"secret_ref"}} declares a slot and must not fire, while {"password":"an-actual-password"} fills one and must.

Breaking discovery is proven too — the CLI refuses to report clean, and two discovery tests fail.

Not closed, and the doc says so

toTrajectory() still has no guard of its own. Adding one changes behaviour — it could throw where it previously did not — and #99 asked for the bypass to be covered, not closed. docs/privacy/session-custody.md records that plainly under SC-02 rather than implying the gap is gone.

SC-02's schema-validation note is updated too: it previously described the extraTrajectories array and named its residual gap, which is now the thing this PR removes.

npm run ci            # green — 204 unit, 14 integration, secret-scan clean, lint-docs clean (47 docs)
npm run test:canary   # 13 pass

Stacked-PR caveat: pull_request targets main only, so this gets no CI while based on #109. Everything above was run locally against this exact tree. Retarget once #109 merges and it picks up the real thing.

🤖 Generated with Claude Code

[myselfsiddharth]

Review

Covers two SC-02 holes: adds tests/unit/trajectory-guard.test.ts proving the object built by toTrajectory() carries no session material even though the assertNoLiteralSecrets() guard only runs inside write(), and replaces validate-contracts.mjs's hand-maintained extraTrajectories array with a filesystem walk, with zero-discovery treated as a hard failure.

What I checked

Read the full diff and src/recorder/redact.ts for the guard's actual patterns, then pulled the branch into a worktree: ran tests/unit/trajectory-guard.test.ts (14 pass, real Chromium against a loopback fixture) and node scripts/validate-contracts.mjs (clean — finds and validates exactly the two files the old hand-maintained list named). I also wrote a throwaway repro against the exported discoverTrajectories() to probe the one design question below.

Strengths

• The test framing is the best thing here. Asserting the property the guard protects holds of the object, rather than asserting the guard ran, is exactly right — it fails only if the recorder actually starts leaking, which is the fact worth knowing. A test that write() calls the guard would have told you nothing about the bypass.
• The guard-proofing self-catch is genuinely valuable. Noticing that all the bypass assertions are "does not throw" — so weakening the guard makes them more likely to pass — and then verifying it by deleting the "value" pattern and watching all eight stay green is the kind of check most test suites never get. The per-pattern mustBeCaught cases are the right remedy, and the "password":"secret_ref" (slot) vs "password":"an-actual-password" (filled) pair correctly pins the one pattern with a non-obvious negative case.
• Covering both pretty-printed and compact serialization is a real catch — write() pretty-prints, a bypassing caller likely won't, and "cookies"\s*: does match differently across the two.
• Zero-discovery as a hard failure is the right instinct: an empty result means the walk broke, which is the same silent hole wearing a different hat. Good that the function itself stays honest (returns []) and only the CLI escalates.
• Honest scoping — the doc records that toTrajectory() still has no guard of its own rather than implying the gap is closed. Discovery is dependency-free and consistent with the repo's no-new-deps convention (fs.glob is 22+, CI is Node 20).

Issue found

Discovery is location-based, so it swaps a hand-maintained list for a hand-maintained location convention. discoverTrajectories() collects *.json only where path.basename(dir) === "trajectories" under experiments/. Anything elsewhere is silently skipped — no warning, no failure. I reproduced two cases:

• experiments/gate-v2/recordings/leaky.json — a plausible sibling directory name, containing a blatantly invalid trajectory with a cookies field. Not discovered. I confirmed via git check-ignore that this path is not gitignored, so it's committable and would sit unvalidated indefinitely.
• experiments/gate-v1/out/trajectories/leaky.json — out is in SKIP_DIRS, so even a correctly-named trajectories/ dir is skipped underneath it. Lower severity (that path is gitignored) but shows the skip list and the discovery rule interact in a way neither makes obvious alone.

This is reachable through the supported interface: src/recorder/cli.ts:254 accepts --out to an arbitrary path and only defaults to experiments/gate-v1/trajectories/ when the flag is absent. It's worth flagging specifically because it sits against this PR's own stated root cause — "a guarantee that depends on a specific call path or a hand-maintained list, rather than on the data shape itself." Directory-name discovery is still not the data shape.

Filed as #116 with a suggested shape-based fix (validate any *.json under experiments/ carrying the trajectory schema's identifying fields, keeping the zero-discovery hard failure as-is). Not blocking — only two trajectory files exist today, both canonical, both validating, so this is a strict improvement that regresses nothing.

Note on the stacked base

Base is track1/b5-secret-scan-storage-state (#109), so this gets no CI until retargeted, as the description acknowledges. I reviewed #109 separately and recommended merging it (with follow-up #115), so the intended order works — just needs the retarget to main before merge so it picks up real CI rather than a local-only green.

---

Verdict: Merge (after retargeting to main once #109 lands), then resolve #116.

[myselfsiddharth]

@OM152002 Pls retarget to main and address the issues found.