[lilyshen0722]

Draft, stacked on the record-v1 branch while #2672 is open. It retargets to main after record-v1 lands.

What

• Supervisor (src/supervisor.js): superviseRun names a hung turn stuck and always settles the run. A stall window (no progress for stallMs) catches a stalled stream and a silent cut, and a SIGKILL backstop catches a child that ignores the adapter's own kill. That kill targets the child only, not its process group. So a native binary behind an npm launcher can outlive it: that's listed under the open gaps below.
• Redact path through the supervisor (7e04c869b): the supervisor's trace wrapper now forwards actor and redact to the store's run. Before this, a supervised run wrote agent text with the system actor and no redaction.
• Proof (record-redact-supervised.test.js): a credential is planted in agent text, in a tool argument and in a permission question asked through the valet. The test asserts the secret appears in neither the store file nor the OTLP spans and events, and that the actor survives the wrapper.

Tests

I ran the packages/seats suite myself in a capped container (node:22-bookworm, --network none, 1 GB, 2 CPUs) at head a1e3d9ec4:

run	user	result
1	root	151 / 153
2	root	151 / 153 (same 2)
3	node (uid 1000), empty ~/.claude and ~/.codex created in the container	153 / 153
supervisor base 9fd4e5fef	root	28 / 28

Both root failures come from the environment, not the code:

• record-store "a failed append breaks the run" relies on chmod 0400 refusing a write, and root ignores that mode.
• valet-env asserts a positive control: a real ~/.claude must exist for "not reachable" to mean anything, and a bare container has none.

Run 3 meets both preconditions and is clean. There were no timing flakes in these runs, though host load was high (1-minute average 77–89).

CI runs non-root: the seats step (tool-tests in tests.yml) runs on ubuntu-latest as the runner user, not in a container. That step isn't on this base yet, so the self-run above is the evidence until this retargets.

Open gaps, not fixed here

• Process-group kill: the backstop child.kill('SIGKILL') reaches the direct child only. We measured that killing an npm launcher leaves the native CLI running, so the kill must go to the process group. The ACP adapter work is adding that, with a grandchild test.
• Run header: superviseRun calls store.startRun({ runId, adapter, label }) and passes no actor, taskId or podId. So a supervised run's run.started carries the default actor, and the run isn't bound to its task or pod. The redact and actor forwarding above applies to emits inside the run. The run header is the remaining piece, and it lands before any writer that records agent text goes live.

🤖 Generated with Claude Code

https://claude.ai/code/session_013ENddpGzhE7S8qjDSYAMuu

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

Run configuration

• Source mode: Auto
• Repository: Team-Commonly/commonly
• Branch: feat/jellymoon-record
• File: .coderabbit.yaml
• Commit: fb686d4
• Layers: Repository YAML
• Review profile: QUIET
• Plan: Advanced
• Run ID: a1ab76c3-6d7d-45a2-ad35-a38120a9c8bf

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{Comment @coderabbitai help to get the list of available commands.}

[lilyshen0722]

Security gate at head a1e3d9ec49fc9c0d4f703d8ec6ded5c429eaadeb.

Answer 1: does any supervised path to the store or OTLP skip the scrub?
Every write goes through createTraceStore's write() (trace/store.js), so trace.emit, run.emit, run.settled and the index row all pass through scrub(). The index failure is copied from the already-scrubbed event. OTLP reads from store.readRun() and drops agent.message_chunk and agent.thought_chunk entirely, so it cannot carry chunk text. The one path that does skip the scrub is the streamed-text one below.

Defect 1 (HOLD): a secret split across streamed chunks reaches the record.
scrub() runs per write() call, and matching is exact per string. A granted value that arrives across two agent.message_chunk deltas matches neither. I reproduced it against this head with the store and redact([secret]). The chunks "the key is sk-test-CANA" and "RY-7f3a91c2d4e5b6a8 end" were stored unchanged, and joining the stored texts gives the full secret. OTLP does not carry it, but the on-disk record does. This breaks the invariant in the store's own doc comment, "known credential values never reach a line, even inside agent text". The test does not cover it: every delta in echoingAdapter holds the whole value, so [redacted] appears in whole-value chunks only. The absence checks use the full canary, so they cannot see a split value either.
Fix: scrub the run's streamed text over a rolling window (hold back up to maxSecretLength - 1 characters per stream until the next chunk or settle), or scrub the concatenation before writing. Add a test that splits the canary across at least two deltas and asserts that neither the record bytes nor the stored chunk texts reconstruct it.

Defect 2 (HOLD, same fix): values under eight characters are stored unscrubbed.
redactKnownCredentials skips values shorter than 8 characters. store.redact() accepts any string, and prepareRunEnv's usable() checks only for non-empty. So a granted credential of six characters is written verbatim. I reproduced it: the record contains value abc123 here. Either refuse short values in prepareRunEnv with credential_denied, or scrub every length in the store and keep the floor only for the refusal-text reader.

Positive controls. The [redacted] assertions prove the scrub ran for the whole-value case, and the decided, settled and export checks are real. They do not prove the streamed case, for the reason in Defect 1.

Non-blocking. spawnCredentials includes ctx.runtimeToken, but the store's scrub set only holds values passed to redact(), which is the valet's granted values. failureData passes RunFailure detail through unscrubbed. No adapter in this PR throws a RunFailure carrying a runtime token, so I did not reproduce a leak. Register runtimeToken with the store, or document the boundary. Labels, adapter names, task ids and pod ids are caller metadata, not agent text, and are not scrubbed. That is acceptable as written.

Checked and fine. The actor is validated and passed through the wrapper unchanged, and the test asserts it. The redact hook is offered only when the run has one. The supervisor's own events cannot be forged by an adapter (SUPERVISOR_EVENTS). The permission question and argument digest go through the same store write.

Written by Rhea, a Commonly agent — https://commonly.me/v2/pods/6a8f6dc7a1dccf2e02f31015

GATE: HOLD

[lilyshen0722]

#2701 security re-gate at ac55640 (draft, base feat/jellymoon-record)

Last gated head: a1e3d9e (HOLD, review 5642835986). The delta is a merge of the base-branch commits fb686d4, 92d6d09, ccfd71c, 79e580d and e1bd2cc, plus the fix commit ac55640. I reviewed all of it.

The three items from the last HOLD

1. Split secrets, per-stream rolling window: fixed for values split across deltas. The 11 tests in record-redact-stream.test.js pass at this head (11/11 in a worktree). The two-chunk canary case stores ["the key is ","[redacted] end"].
2. Short values: fixed. prepareRunEnv refuses a value under MIN_CREDENTIAL_LENGTH (8) with credential_denied, reason too_short, before spawn. The store scrubs every registered value of any length, longest first.
3. ctx.runtimeToken is registered through run.redact in superviseRun, and RunFailure detail is scrubbed with the spawn's credentials. Fixed.

HOLD: the held tail is written verbatim when a stream ends (kill path included)

flushStreams() writes each held tail as its own delta: writeLine(kind, { text: held }, actor) in trace/store.js. The tail is held back precisely because it is a proper prefix of a registered value, so it is a partial credential, and it is written with no scrub. The stream can end without the value completing: a kill, a stall, a cancel. The flush then writes the prefix to the record.

Reproduced against this head with createTraceStore:

• Stream text "key: " + SECRET.slice(0, -1) (all but the last character of a 32-character canary), then a kill recovery, then run.settled.
• Stored chunks: ["key: ", "sk-test-CANARY-7f3a91c2d4e5b6a"]. The record holds 31 of 32 characters.

The tests pin this behaviour. held text is written before settle, never lost and never whole asserts that CANARY.slice(0, 12) is written, and the kill test asserts CANARY.slice(0, 10). The name says "never whole", but a value missing its last characters is effectively the value on the record, and the invariant in the store's own doc comment is "known credential values never reach a line".

The flush ordering on the kill path is correct: liveness.recovery does not flush, but run.settled does, so the tail lands before the settle. The problem is what is written, not when.

Fix (I tried it on a copy; the probe then stores "[redacted]" and no prefix): in flushStreams, write { text: '[redacted]' } in place of { text: held }, since a held tail is only ever a prefix of a value and a stream that has ended can no longer complete it. Only those two tests change. They should assert the redaction marker and the absence of the prefix. Add the kill-mid-value case with allBytes(root) checked for the 10-character prefix. A short ordinary tail (e.g. "s") redacted at a stream end is an acceptable cost.

Checked and fine

• Permissions (valet/permissions.js): readAnswer now needs approved as a boolean, by as a user id, and role as an owner role. A bare true, a missing by and pod-member all fail, which is the ADR-033 D5 rule. The pod-member refusal re-asks inside the same timeout, bounded at 10. decider names a person only on an approved or rejected verdict.
• format.js: permission.requested.askedOf.role and permission.decided.decidedByRole use PARKED_CALL_ROLES, which excludes pod-member. policy is tied to decidedBy in both directions. A permission.decided with no attribution and no rule decision now throws.

Non-blocking

• decision.ruled validates decidedByRole against the full role list but has no policy/role consistency check like permission.decided does. It is not an attribution hole, but add the same check.
• The held tail lives only in memory. A crash between writes loses it. That is not a leak, but it should be stated in the record doc.
• The flush lands after the liveness.recovery kill line, so the streamed text sits after the event that caused it. Ordering only.

Not run: the full packages/seats suite and the mutant list in SWE's note. I ran only the stream file and the probe.

Written by Rhea, a Commonly agent — https://commonly.me/v2/pods/6a8f6dc7a1dccf2e02f31015

GATE: HOLD

[lilyshen0722]

#2701 security re-gate at 672ad4a (draft, base feat/jellymoon-record)

Last gated head: ac55640 (HOLD, review 5642835986 for the earlier head, and review ac55640 for the kill-path flush). The delta is one commit, 672ad4a, on ac55640. Measured against the PR's own files: 3 files, +35/-13 (store.js, record-v1.md, record-redact-stream.test.js).

The HOLD: held tail written verbatim at stream end

Fixed. flushStreams (trace/store.js:154-160) now writes { text: '[redacted]' } for each held tail and never the held text. The held map entry is only set when cut < joined.length (store.js:146), so every entry in the map is a proper prefix of a registered value. No other writer uses held for output: the only other reads are the scrub and the cut at store.js:144-146.

Tests:

• a stream that ends mid-value writes the marker before settle, never the prefix: expects the marker, and allBytes(root) excludes the 12-character prefix.
• a value cut one character short never reaches the record (new): the exact probe from the HOLD. Stored text is key: [redacted]; neither the 31-character prefix nor the 8-character prefix is on disk.
• a supervised run killed mid-value writes the marker for its held tail...: expects [redacted] before run.settled and on the retry, with the 10-character prefix absent from disk.

Ran at 672ad4a in a clean worktree (no dependencies needed):

• node --test test/record-redact-stream.test.js test/record-redact-supervised.test.js: 14/14 pass.
• npm test in packages/seats: 170/170 pass.

record-v1.md rule 1

Matches the code. It says the marker is written, that a short ordinary tail is redacted too, and that a held tail lives only in memory and is lost on a crash before the next write. That crash note was the non-blocking item from the last review and is now stated.

Checked and unchanged

• The flush ordering: the marker lands before run.settled (liveness.recovery still does not flush; run.settled does).
• Permissions, format.js attribution checks, and the upstream and env changes are not in this delta. They are unchanged since the last gate.

Not in this PR

• The decision.ruled policy/role consistency check is TASK-544, against feat/jellymoon-record. The earlier review marked it non-blocking, so it does not gate this head.

Not checked here

• CI at this head. The press should check it.
• I did not re-run the full packages/seats suite's mutant list from SWE's note.

Written by Rhea, a Commonly agent — https://commonly.me/v2/pods/6a8f6dc7a1dccf2e02f31015

GATE: PASS

[lilyshen0722]

Sable code-leg gate for #2701 at 672ad4a. Rhea's security leg is PASS at this head (5644346867). Scope: the stream-end flush fix from my ac55640 HOLD, its tests and docs, and the required checks.

Code leg: the fix holds.

• flushStreams now writes [redacted] in place of a held tail. It never writes the held prefix. streams gets an entry only while a tail is held (windowed deletes the entry otherwise). So the flush writes a marker only for a real held prefix, never for an ordinary stream.
• The held tail is a suffix that is a prefix of a known value, by construction of heldLength. The marker therefore never discards text that was complete.
• Flush happens before FLUSH_BEFORE kinds, as before. The marker goes through writeLine, so it is hash-chained like any other line. It contains no secret.
• A crash before the next write loses the held tail and writes nothing. The record-v1 text says so.

Verified at this head

• packages/seats tests on Node 22: record-redact-stream, record-redact-supervised, supervisor, supervisor-process. 37 of 37 pass.
• Mutant: restoring the verbatim-tail write ({ text: held }) in store.js makes 3 tests fail across the two redaction suites. Restored and confirmed with diff -q.

Required checks: two are red.

1. Detect secrets (gitleaks) fails. The finding is packages/seats/test/supervisor.test.js:299, a literal '^[[1;3;m…' ANSI escape that gitleaks reads as a generic API key. It sits in commit 9fd4e5fe by Lily. This is a false positive, but the check gates the press. The fix is either an inline gitleaks allow on that line or a path/regex allowlist entry. The test should not change.
2. PR targets main fails. The base is feat/jellymoon-record, so this PR is stacked. Retarget to main once the parent lands, or Lily rules the stacking is intended and the check is waived for this PR.

Both are press blockers. Neither is a code finding. The gate verdict covers the code at this head, and a red required check is a HOLD. Re-gate is not needed for the code once the two checks are green, unless the head moves.

Written by Sable, a Commonly agent — https://commonly.me/v2/pods/6a692a1be833c668acdb84cf

GATE: HOLD

[lilyshen0722]

#2701 security re-stamp at 239e4e3 (base feat/jellymoon-record)

Last gated head: 672ad4a (PASS, the redaction re-gate, review-672ad4a44.md). The delta is one commit, 239e4e3, touching one file: .gitleaks.toml. No code, test or doc changes. Measured against the PR's own files.

The change

One regex added to the gitleaks allowlist: the literal sk-or-v1-abcdefabcdefabcdef, the fake OpenRouter key used by the scrub test in packages/seats/test/supervisor.test.js. The test file is unchanged, as Lily's ruling asked.

Effect on the scanner

gitleaks matches allowlist regexes against the matched secret, so the entry suppresses only a finding whose value contains that exact literal. A real OpenRouter key is random after its prefix, so the entry does not reach one. This is the narrowest fix that clears the false positive without an inline allow on a line that changes.

Holds

• The redaction behaviour from the 672ad4a gate is unchanged: flushStreams still writes the marker, not the held prefix. The seats code at this head is byte-identical to 672ad4a for every file in the PR.
• The rest of the PR's files are also unchanged since 672ad4a (the restricted diff is the .gitleaks.toml hunk only).

Not checked here

• CI at this head. The detect-secrets check is what this commit targets; the press should confirm it is green.
• The base-branch question (PR targets feat/jellymoon-record, not main) is Lily's ruling and is not a security item.

Written by Rhea, a Commonly agent — https://commonly.me/v2/pods/6a8f6dc7a1dccf2e02f31015

GATE: PASS

[lilyshen0722]

Sable re-stamp of the code leg at 239e4e3. This follows Lily's ruling of 86419 that a re-stamp at the new head is enough when the diff touches only the allowlist.

What changed since 672ad4a (my HOLD, review 5644369077): one commit, 239e4e3, which touches .gitleaks.toml only (+5 lines). It adds one exact-literal regex, sk-or-v1-abcdefabcdefabcdef, with a comment naming the test.

Checked:

• No code or test change since 672ad4a. packages/seats/test/supervisor.test.js is byte-identical between the two heads.
• The allowlist literal matches the fake key at packages/seats/test/supervisor.test.js:299 exactly. It is not a pattern, so a real key still fails the scan.
• The earlier code-leg findings are unchanged and still PASS at this head. The record redaction and the mutant results stand from 672ad4a.

Not checked here: CI. The Detect secrets run is the one this allowlist is meant to clear. I did not confirm its result at filing, so the press should read the check state itself. The PR-targets-main check is waived for this stacked PR by Lily's ruling of 86419.

Written by Sable, a Commonly agent — https://commonly.me/v2/pods/6a692a1be833c668acdb84cf

GATE: PASS