fix(runtime): freeze redaction surfaces mask-only - #2314
Conversation
|
The typed contract is a real improvement over three independent direct calls, and the ADR amendment is specific enough to check against. Two ordering defects block it, and the census turns up a surface the set does not name. Masking runs after truncation on two of the three surfacesThe canonical URL-userinfo detector only fires when the terminating Two of the three named surfaces cut the input before that call. MCP diagnostics. let bounded_input: String = message.chars().take(MAX_BACKEND_ERROR_INPUT_CHARS).collect();
let masked = mask_mcp_diagnostic(&bounded_input);
Session tool previews. let input_str = truncate(&serde_json::to_string(&input).unwrap_or_default(), 500);and the enclosing parser masks afterwards ( What makes this actionable rather than a design argument is that the repository already contains the fix, in the same file as the detector. let masked = mask_secrets(&bounded_input);
let masked = if mask_input_truncated {
redact_crossing_boundary_url_userinfo(&masked)
} else { masked };The two surfaces above take the same shape of input bound without the corresponding fallback. Either mask the complete input before bounding, or route these through a boundary-aware helper equivalent to the one above. Both need a regression whose credential terminator sits beyond the input bound — a test that keeps the whole URL inside the bound passes either way and proves nothing. This also means ADR-115's obligations for the session and MCP surfaces are not delivered as the amendment now states them. The amendment marks both resolved and declares the surfaces permanently mask-only; for a boundary-shaped input neither guarantee holds today. "Closed" is a convention here, not an invariant
Worth saying plainly in the ADR which of the two is being claimed. As written the text reads as an invariant; the code provides a convention. The set is missing a surfaceEnumerating redaction call sites independently rather than starting from the three: Some of these may be deliberate exclusions — the coordinator comments treat the raw backend result as pre-wire and the VCS and repo-source paths at TestsThe two added tests ( Suggested additions: the two boundary regressions above; a test asserting the session |
|
Requesting changes. The mechanism here is good and worth saying so explicitly, because it is The closed set is closed to code that opts in, and two call sites have notThe summary says every named call site is routed through the canonical masker and the surface against the three that were routed: This is not a leak — both call sites do mask, with the canonical detector, which is the property
So a fourth redact-not-block surface, self-described as mirroring Two ways out, either is fine. Name them — a Cross-cutting: this and #2341 both add a section named "Amendment 2"Not visible from inside either change. and #2341 adds: Different content, same name. They do not textually conflict — This PR has the correct form: top level, dated, matching Amendment 1's shape. The renumbering is |
ohdearquant
left a comment
ohdearquant
left a comment
There was a problem hiding this comment.
Automated review. Posted by this repository's automated pull-request review pipeline; this is not a human read and does not gate the merge by itself.
Verdict on head a25ac2d: REQUEST-CHANGES, 2 blocking findings. Finding details are delivered to the review's recipients rather than posted here. Do not merge this head while blocking findings are outstanding; a pipeline comment on a newer head supersedes this one.
…ncation Two of the three redact-not-block surfaces frozen by ADR-115 Amendment 2 did not actually meet the contract they were frozen under. The session mirror's ChatGPT parser copied the conversation title into the event/session slug unmasked, and the Claude Code parser did the same with the JSONL line's own slug field, so both reached sessions.slug with any embedded credential intact even though the ADR text claimed every parsed title projection shared the same masker as text/raw. Both now run through the same mask_for_redaction_surface(SessionMirror) call the message body already used, at parse time, before the event or session record is built. The MCP diagnostic path bounded a backend error message to a fixed character window before masking it, so a detector whose match terminates past that window (for example the closing `@` of a scheme://user:pass@host credential) was never recognized and its prefix leaked into the response. Masking now runs over the full, untruncated message first, and the input/output length caps are applied to the masked result afterward. The executable contract in secret_gate.rs is updated to name every stored target for the session mirror surface, and the contract test now compares against the same constants the contract returns instead of a second hand-typed copy of the same strings.
…ll-site census Three of the four mask-only redaction call sites truncated their input before masking it, so a detector's terminating span (for example the `@` closing a `scheme://user:pass@host` credential) could fall past the cut and leave the credential's prefix unmasked in the output: - bounded_backend_error_key in khive-mcp/src/server.rs truncated the raw backend id before masking, leaking a credential prefix into both the sanitized key and the fingerprint suffix input. It now masks the full id first and applies the input/display caps to the masked text, mirroring the already-correct bounded_backend_error_message. - extract_block in khive-pack-session's mirror parser truncated a tool_use input or tool_result content to 500 characters before the caller masked the extracted text. It now masks each block's full text first and truncates the masked result, closing the gap for every parser that shares extract_block (Claude Code, Codex, and the claude.ai export path). - bounded_backend_id_for_log and bounded_backend_cause_for_log in kkernel's coordinator dispatch called the raw mask_secrets primitive on a truncated prefix directly. They now route through mask_for_redaction_surface(RedactionSurface::McpDiagnostic, ...) with the same mask-then-truncate ordering as the MCP wire boundary they mirror. A new census test (khive-runtime/tests/adr115_redaction_call_site_census.rs) walks every .rs file under crates/ and asserts that mask_secrets is never called directly outside the module that defines it, so a future direct caller fails the test instead of silently joining the population. The one remaining direct caller inside that module, the general-purpose log-text bounder bounded_masked_log_text, is documented as an intentional exception: it already implements a mask-then-truncate contract with an additional crossing-boundary fallback that is a strict superset of what the named surfaces provide. Docs for the affected surfaces (secret_gate.rs, coordinator.md, mirror-parse.md) are updated to state the corrected masking order.
ohdearquant
left a comment
ohdearquant
left a comment
There was a problem hiding this comment.
Automated review. Posted by this repository's automated pull-request review pipeline; this is not a human read and does not gate the merge.
Verdict on current head: APPROVE, zero blocking findings. This is a comment, not an approval — a human reviewer decides whether to approve and merge.
…ndary Every diagnostic-boundary masking call (MCP backend error messages and keys, the kkernel coordinator's mirrored log helpers, and the session mirror's cwd/git_branch/tool_use/tool_result fields) used to mask the full, unbounded input before applying any length cap, so scan cost scaled with the caller's raw input length even though the output was always capped. Introduce secret_gate::mask_bounded, a single shared helper that bounds the masker's own input to a fixed window before it ever runs, then caps the masked result. A window cut mid-token could let a masker that never saw a credential's terminating shape emit the visible prefix unmasked, so any token straddling the window boundary is dropped in its entirety (back to the last whitespace inside the window) instead of being partially echoed; a single token longer than the window is replaced by the bare truncation marker. All four diagnostic sites and the session mirror now call this one helper instead of separately duplicating the window-then-mask recipe. Also masks the session mirror's cwd and git_branch fields, which were previously copied verbatim into the sessions/session_messages tables, and makes the ADR-115 redaction call-site census resolve its workspace root by walking up to the nearest ancestor Cargo.toml that declares [workspace] instead of a hard-coded parent-directory count, skipping with an explicit reason when run outside a khive workspace checkout.
ohdearquant
left a comment
ohdearquant
left a comment
There was a problem hiding this comment.
Automated review. Posted by this repository's automated pull-request review pipeline; this is not a human read and does not gate the merge by itself.
Verdict on head 2929175: REQUEST-CHANGES, 1 blocking finding. Finding details are delivered to the review's recipients rather than posted here. Do not merge this head while blocking findings are outstanding; a pipeline comment on a newer head supersedes this one.
mask_bounded cut a token straddling the window boundary but left earlier fragments of a bridged credential chain visible, because bridge_fragment_chain can reconstruct a credential from several whitespace-separated pieces that are each too short to be recognized alone. The gap between two fragments is deliberately unbounded in byte length, so no finite forward lookahead past the window can guarantee seeing every fragment of a chain that starts before the cut. Instead of scanning past the boundary, mask_bounded now walks backward from it, over data already inside the window, dropping every further bridge-fragment-shaped token chained to the fragment it already removed, using the same fragment-count and glue-token budgets bridge_fragment_chain itself uses. This adds no lookahead and keeps scan cost bounded by window_chars alone. The walk only runs on windows carrying trigger-word context, so ordinary prose cut mid-sentence is untouched. Three smaller corrections ride along: - The session-mirror stored-target contract named session_messages.cwd and session_messages.git_branch, columns that do not exist; those fields are session-keyed and live only on sessions.cwd/sessions.git_branch. - The MCP diagnostic docs said masking runs over the full, untruncated input; the code has bounded it to a fixed window since Amendment 3. Docs now state the window and the lookahead-free mechanism above. - mask_bounded only asserted window_chars >= output_cap_chars in debug builds; it now also clamps output_cap_chars to window_chars in every build.
ohdearquant
left a comment
ohdearquant
left a comment
There was a problem hiding this comment.
Automated review. Posted by this repository's automated pull-request review pipeline; this is not a human read and does not gate the merge by itself.
Verdict on head 3e5113c: REQUEST-CHANGES, 1 blocking finding. Finding details are delivered to the review's recipients rather than posted here. Do not merge this head while blocking findings are outstanding; a pipeline comment on a newer head supersedes this one.
…trigger context mask_bounded's trailing_bridge_fragment_cut only ran its backward fragment-drop walk when the truncated window itself carried a trigger word. The unbounded masker's own bridge reconstruction accepts trigger context from either side of a fragment chain, so a credential shaped like "<fragment> <fragment> <fragment> is the api key for ..." has its only trigger word after the last fragment. When the window is cut inside that trailing fragment, the trigger sits past the boundary and is never read into the window, the trigger check fails, the walk never runs, and the earlier whole fragments already inside the window are left unmasked. The walk cannot see past the window boundary by construction, so it cannot use the presence or absence of a trigger inside the window as a signal either way. Run it unconditionally on every truncated window instead: a fragment-shaped token chain at the tail is dropped whether or not it carries a visible trigger, at a bounded cost of at most MAX_BRIDGE_FRAGMENTS - 1 tokens of a tail already slated for truncation.
ohdearquant
left a comment
ohdearquant
left a comment
There was a problem hiding this comment.
Automated review. Posted by this repository's automated pull-request review pipeline; this is not a human read and does not gate the merge by itself.
Verdict on head 6afb3f7: REQUEST-CHANGES, 1 blocking finding. Finding details are delivered to the review's recipients rather than posted here. Do not merge this head while blocking findings are outstanding; a pipeline comment on a newer head supersedes this one.
Three documentation conflicts, additive on both sides: - crates/khive-mcp/docs/api/coordinator.md: the bounded-masking paragraph and the retry/backoff policy paragraph describe different fields of the same error, so both are kept. - docs/guide/api-reference.md: the same pairing on the user-facing page. - docs/adr/ADR-115: both sides append amendments after Amendment 1. Amendments 2 and 3 keep their numbers and the 2026-09-10 bare-hex amendment keeps its unnumbered heading, so the existing citations of "ADR-115 Amendment 2" in the runtime sources and in secret_gate.md stay valid. deno fmt --check, scripts/lint-adr-refs.sh and scripts/lint-adr-status.py pass on the merge result.
main renamed `contains_trigger(text) -> bool` to `find_trigger(text, credential_label_only) -> Option<&'static str>` while this branch added four bounded-masking tests calling the old name. The merge kept both edits, so the merged tree did not compile. The four call sites now read `find_trigger(.., false).is_none()`. That is the same predicate the old helper computed: its body was exactly the `credential_label_only = false` chain (bounded trigger word, compound trigger, standalone/assigned `token`, assignment credential label). One comment naming the old helper was reworded. cargo check for khive-runtime, khive-mcp, khive-pack-git, khive-pack-session and kkernel with --all-targets passes on the merge result; khive-runtime lib tests report 1602 passed, 0 failed.
2 participants
Summary
RedactionSurfacecontract and route every named call site through the canonical masker without any manifest, stamp, or exemption-event capability.Verification
cargo test --manifest-path crates/Cargo.toml -p khive-runtime --lib named_redaction_surfaces -- --nocapturefailed to compile before the typed contract existed.cargo check --manifest-path crates/Cargo.toml -p khive-pack-session -p khive-pack-git -p khive-mcp --all-targetscargo clippy --manifest-path crates/Cargo.toml -p khive-runtime -p khive-pack-session -p khive-pack-git -p khive-mcp --all-targets -- -D warningscargo fmt --manifest-path crates/Cargo.toml --all -- --checkgit diff --checkContract
Git and session persist only masked projections and never write
khive:secret_gateor an exemption-success event. MCP diagnostics have no durable stored target. Caller identity, verb, namespace, path, and request arguments cannot select another mode.Closes #2059
Closes #2060
Closes #2061
Bounded masking trade
MAX_BRIDGE_FRAGMENTS - 1tokens of an already-truncated tail, andtruncatedis reported. The unbounded surfaces are unaffected.