Skip to content

fix(runtime): complete secret finalization coverage - #2341

Closed
ohdearquant wants to merge 1 commit into
mainfrom
codex/secret-finalization-coverage
Closed

fix(runtime): complete secret finalization coverage#2341
ohdearquant wants to merge 1 commit into
mainfrom
codex/secret-finalization-coverage

Conversation

@ohdearquant

Copy link
Copy Markdown
Owner

Summary

  • route every property-bearing finalization target in the Route all property-bearing write paths through the shared finalization and reservation contract #2057 runtime census through one shared secret-gate boundary, including entity/note create and update, atomic and proposal materialization, curation merge, portability restore, direct code ingest, and knowledge writes
  • make knowledge atom and domain admission consume the same one-shot, namespace-bound manifest contract and commit the durable posture stamp plus redacted target-linked audit atomically with the write
  • normalize only a byte-identical persisted secret-gate stamp echo on read-modify-write updates; reject forged or transplanted stamps and refinalize whenever the scoped candidate changes

Contract and census

Verification

  • cargo fmt --all -- --check
  • cargo check --workspace
  • cargo clippy --workspace --all-targets -- -D warnings
  • cargo test --workspace
  • focused finalizer, entity/note merge, knowledge atom/domain, direct code-ingest, exact-echo, anti-transplant, rollback, non-disclosure, and cross-namespace ownership regressions

Closes #2057

Closes #2058

Closes #2065

@ohdearquant

Copy link
Copy Markdown
Owner Author

Reviewed at head 1c335c69d763414279f80ac1d151286407ffc9a6.

The structural walk is real, and it is a genuine improvement over scanning a rendered string: keys,
nested leaves, array elements, tags and names all reach the detector now, and known-prefix secrets are
caught in every one of those positions. The routing work is thorough and the merge/absorbed-stamp
handling is careful.

The problem is one level down, and it means the headline claim does not hold for the case that
matters most.

The walk reaches every string, then throws away the only context the detector needs

collect_json_strings flattens an object by pushing the key as one &str and then recursing into
the value, which pushes the value as a separate &str
(crates/khive-runtime/src/secret_gate_finalizer/boundary.rs:486-503). scan_one then calls
secret_gate::scan(value) on each entry independently (:455-484).

So {"api_key": "Xk9mZ2vQpLrT8nJwYuA/HfBsDcGiONvMabcdefgh"} becomes two unrelated scans:
scan("api_key") and scan("Xk9m…"). The entropy arm of the detector only fires in trigger context
— at this head, crates/khive-runtime/src/secret_gate.rs:1191 is still if near_trigger { return Some((token, "high-entropy-token")) }, and this PR adds 156 lines to that file without changing it.
The first scan has the trigger and no token. The second has the token and no trigger. Neither
fires, and the credential is written to the store.

This is the shape a properties map takes in practice: the key is the label and the value is the
secret. A detector that needs them adjacent, fed by a walker that guarantees they never are, is
structurally blind to the common case — and the blindness is invisible from either component alone,
which is why the routing census reads as complete.

Worth flagging as a program-level point rather than a PR-local one: #2334 is routing a different
surface into the same detector and hits the same wall from the other side, masking a key evaluated
standalone where nothing can ever be adjacent to it. Two changes now depend on this detector reading
machine-generated JSON, which is the input its entropy heuristic was least designed for. Fixing it in
the shared detector — take an optional key/path context, or scan a "key":"value" pair as one unit —
closes both, and closes them for the next caller too. Fixing it twice at two call sites does not.

Tests to add with it: a trigger key with a high-entropy value, a nested one, and a pre-stringified
one.

The census promises what its mechanism cannot deliver

declared_origins_have_mechanized_source_reachability is documented as: "Adding or renaming a writer
without routing it through the boundary makes this census fail in CI instead of silently creating
another bypass" (crates/khive-runtime/src/secret_gate_finalizer/declaration.rs:165-169).

The implementation is a hard-coded list of (source_name, source_text, marker) tuples over
include_str! sources, asserting source.contains(marker) (:174-213). That tests containment in
files someone already listed
. It cannot fail for a new writer added to one of those files, because
the marker it asserts is still present elsewhere in the same string. It cannot fail for a writer in a
file not on the list. It enumerates nothing.

That gap is not hypothetical here. An independent inventory of property-bearing persistence routes
comes to 19; the assertions represent 16. The three outside are the knowledge.sections delegation
at sections.rs:951 and the two direct UPDATE knowledge_atoms SET properties=json_set(...) writers
at sections.rs:1055 and :1205. The dispute counter those last two write is a fixed integer and
cannot carry an injected key, so this is not an active hole — but it is property-bearing persistence
outside a census that claims to cover it, and the next writer added there inherits the silence.

A guard whose population is a hand-maintained list is a checklist, which is fine as long as the
comment says so. Deriving the population from the source — every properties-bearing persistence
expression, matched against a finalizer call or a declared exclusion — is what the docstring
currently describes. Either build that or soften the claim; the dangerous state is the one where a
reader trusts the stronger sentence.

A duplicate slug clears a persisted stamp

In upsert_atoms, a slug already seen in the same batch is recorded as (id.clone(), false, None)
(crates/khive-pack-knowledge/src/knowledge/crud.rs:119-122). That None is the persisted-properties
slot, so the omission-preserving logic at :188-204 gets nothing to preserve, finalization takes the
fresh path at :228-257, and the update binds properties to NULL at :317-332.

The reachable case is a two-entry request naming the same existing, stamped slug twice with the
second entry's properties omitted. The stamp is cleared — with no exact echo supplied and no change
to the scoped value, which are the two conditions the design says are required to move a stamp. Carry
the persisted properties for duplicates, or reject duplicate slugs before finalization.

Knowledge writes finalize outside the transaction they protect

upsert_atoms reads targets at :111-170, drops the reader at :171, finalizes at :216-257, and
takes a writer at :344-351. The update predicates on id and namespace only (:317) — no
expected updated_at, content hash, or candidate hash. A concurrent writer can change the row between
the scan and the write, and the stale candidate overwrites it. upsert_domains has the same shape at
:416-493 and :560-619.

The last-writer-wins predicate is itself unchanged from the base, so this is not new SQL. What is new
is that a finalizer result computed outside the transaction is now part of a security contract, which
raises the bar the old predicate has to clear. The surrounding code already sets that bar: entity and
note updates use revision-guarded replacement, and merge recomputes and compares inside the
transaction (curation.rs:3047-3055, :3724-3741). Knowledge is the one family that does not.

Smaller, and one that is only latent

  • "One-shot" is currently a test property. The consuming path is the fixture map removal at
    boundary.rs:621-629; production returns a clone of an immutable, always-empty manifest at :631,
    and ManifestManager::current/refresh swap reusable snapshots rather than spending entries. Also
    worth noting that code ingest finalizes all candidates at code_ingest.rs:217-276 before the
    existing-ID rejection at :307-316, so ordering is claim-then-check. None of this is reachable
    while the production manifest is empty — but the one-shot contract should either be implemented
    inside the writer transaction or described as an intended future property rather than a current one.
  • Stamp binding is adjacent rather than intrinsic. scoped_digest covers scope plus exact value,
    not the row UUID or namespace; the binding lives in the success-event query at boundary.rs:398-453.
    The consequence is narrow and acceptable — a stamp copied from another row is accepted as an
    identical echo on a target already legitimately stamped, and is rejected against an unstamped or
    unlinked target, so it cannot authorize changed bytes. Worth stating explicitly in the ADR, since
    "reject forged or transplanted stamps" reads stronger than what the digest alone enforces.
  • Domain mirrors stay independently mutable through the section verbsresolve_atom_id
    (util.rs:334-366) does not exclude type:domain, so challenge and adjudicate reach a mirror
    and update its properties directly. Pre-existing: the same resolver and the same direct updates are
    at the base ref. Not a blocker for this change, but this PR is what makes the mirror
    security-relevant, so it belongs on the follow-up list rather than being left where it is.

Verified sound

Dry-run consumes nothing on both paths — code ingest returns after the legacy preflight
(code_ingest.rs:205-210) and curation sets the plan to None (curation.rs:1286-1292). Rejected
merge ownership is checked before finalization (curation.rs:1294-1317). The record and success event
land in one execute_batch with a rollback test that asserts neither survives (crud.rs:305-351,
:1468-1524). Merge removes the absorbed row's stamp before folding and revalidates the folded
candidate (curation.rs:2813-2863) — the anti-transplant claim holds where two rows' stamps actually
meet. The production exemption manifest is empty and its canonical bytes are pinned; non-empty parsing
requires a matching corpus identity and refresh empties on any error.

No builds, tests, benchmarks or database queries were run for this pass. Every source claim above was
checked at the head named, with base comparisons read at origin/main.

@ohdearquant

Copy link
Copy Markdown
Owner Author

Requesting changes. The write-path consolidation is the right move and the stamp-echo logic is
sound; the issue is with the mechanism the amendment leans on to keep it sound over time.

The stamp echo holds — for a narrower reason than the ADR states

normalize_secret_gate_property_echo accepts only when
persisted_runtime_stamp && supplied == SECRET_GATE_EXEMPTION_STAMP. Since the stamp is a
constant rather than a per-record value, "byte-for-byte against the persisted target value" and
"a stamp copied to a different target fails closed" collapse into one property: a caller can
never introduce the stamp onto a record that does not already carry it.
That is the property
that matters, and the code delivers it — an echo on an unstamped target, a differing value, a
non-object properties value, and a Replace that omits a persisted stamp all fail closed, and
each has a test.

Worth restating in the ADR, because "transplanted to a different target" implies a per-target
binding that does not exist. Copying a stamp between two already-stamped records is a no-op, not
an attack the code detects — and a reader who believes stamps are target-bound may later build
on a guarantee that was never there.

Major: the census cannot fail in the direction the amendment relies on

Amendment 2 says an origin carrying neither a marker nor an excluded-surface obligation is "a
test failure", and declared_origins_have_mechanized_source_reachability documents itself as
making a writer added without routing through the boundary "fail in CI instead of silently
creating another bypass."

As implemented, every census test iterates a hard-coded list and asserts presence:

  • declared_origins_have_mechanized_source_reachability walks 15 literal (file, marker) pairs
    and asserts source.contains("secret-gate-finalizer-entry: <marker>").
  • follow_on_inventory_origins_are_mechanized_in_the_declaration walks 10 literal
    (entry, origin) pairs against the declaration.
  • declares_complete_entry_point_set and matches_contract_id_list_exactly pin the declaration
    to 8 known ids.

All of these are keyed on what is already declared. A ninth property-bearing write path that
declares nothing and marks nothing adds no pair to any list, so no assertion changes and the
suite stays green. The failure these tests are described as catching is the one case they are
structurally blind to. declares_complete_entry_point_set fires only when someone adds a
declaration, which is the conscientious path, not the risky one.

Two narrower consequences of the same design:

  1. The marker is matched anywhere in the file, so it certifies a string's presence, not a code
    relationship. Placement is correct today — operations.rs:1499 sits directly above the
    finalize_secret_gate_candidate call — but nothing holds it there. Move the marker away from
    its writer, or leave it behind when the writer moves, and the assertion stays green.
  2. Because of (1), "renaming a writer" is caught only when the rename happens to delete the
    comment.

None of this makes the current state wrong. It makes the current state unverified going forward,
which matters more here than usual: the value of routing twenty-odd call sites through one
boundary is that the twenty-first is forced through it too.

Suggested shape

Invert the direction: derive the population from the code and require each member to be declared,
instead of deriving it from a list and requiring each member to be present. A test that finds
property-bearing writes structurally — and asserts each is covered — fails correctly when
something new appears.

The stronger version is to make it impossible to persist properties without going through the
boundary type, so an undeclared writer fails to compile rather than failing a text search. That
is larger than this PR; the minimum here is a census whose population comes from the source
rather than from a literal.

Acceptance

Add a case that introduces a property-bearing write with no marker and no declaration, and
require the suite to go red. The current tests pass that case, which is the point.

@ohdearquant

Copy link
Copy Markdown
Owner Author

Follow-up on the ADR side. Four edits are required to Amendment 2 before this lands — two
structural, two acceptance arms. Implementation notes where I checked them against the code.

1. Amendment 2 is nested inside Amendment 1

It sits as ### 6. Amendment 2: ... within Amendment 1's section, with no Status or Date line of
its own. Amendment 1 establishes a last-in-time precedence rule for resolving conflicts between
amendments, and that rule cannot rank a nested child — a subsection is not an amendment the
ordering can resolve against. Lift it to a top-level ## Amendment 2 (2026-08-30) with its own
Status line, and move Alternatives retained as rejected under whichever amendment owns it; it
currently trails Amendment 2's text while belonging to the enclosing section.

2. Mirror-row invisibility needs a normative acceptance arm

The atomicity text covers the write. It does not cover the projection, which is where the risk
sits. Add an arm asserting that a domain's mirror row appears in no atom-surface read — atom
get, list, search, recall, export — and that atom population counts are unchanged by a domain
exemption. A same-UUID row in knowledge_atoms carrying a domain's posture must not mint a
phantom atom on any atom-shaped surface. Include that UUID-resolution paths disambiguate by
aggregate kind, since the mirror deliberately shares its UUID with the domain.

3. The census needs its failure construction stated as an acceptance criterion

Same point as my earlier comment, restated as a requirement rather than a suggestion: "silently
bypassing both is a test failure" has to name the mechanism that produces the failure. A census
derived by AST or compile-time enumeration fails on an unmarked origin. A hand-maintained list
cannot, and reproduces exactly the inventory drift the shared boundary was introduced to
eliminate. State the derivation in the ADR, not only in the test.

4. Pre-reservation reserved key needs an acceptance arm

A legacy caller-written value under the reserved key, carrying no audit linkage, must be
refused as an operator-repair case — not silently stripped, and not accepted.

Implementation note: the behaviour is already there. normalize_secret_gate_property_echo
returns persisted property key ... is not a runtime-issued stamp; remove it through operator repair when the persisted value is present but is not the runtime stamp. What is missing is the
assertion — I found no test naming that arm.

Worth folding in while you are there: that error text is duplicated verbatim at
crates/khive-runtime/src/secret_gate.rs:192 and
crates/khive-runtime/src/secret_gate_finalizer/boundary.rs:218. Two copies of a message that
encodes a contract will drift, and an acceptance arm asserting on one of them would not notice
the other changing. A shared constant makes one arm cover both sites.

Once these are in, the contract side is settled and re-reading is limited to those hunks.

@ohdearquant

Copy link
Copy Markdown
Owner Author

One more required edit, arising from reading the implementation against the amendment text.

5. Narrow the stamp-echo claims to the property actually delivered

Amendment 2 says target-bound audit linkage "proves which exact scoped digest the runtime issued
it for", and describes a stamp copied to a different target as failing closed. Read against the
code, that describes a stronger control than exists.

SECRET_GATE_EXEMPTION_STAMP is a constant. normalize_secret_gate_property_echo accepts when
the target's persisted value is that constant and the supplied value is that constant. There is
no per-record binding in the compared value, so there is nothing in the comparison that could
distinguish "the stamp this record was issued" from "the same constant read off another record".

The property that is delivered, and that is tested:

  • a caller can never introduce the stamp onto a record that does not already carry it — an
    unstamped target rejects on key presence, before any value comparison;
  • a differing value, a non-object properties value, and a Replace that omits a persisted stamp
    all fail closed;
  • a content change drops the stamp and the candidate is rescanned.

That is sufficient. Copying a stamp from one stamped record to another stamped record is a
harmless no-op — the target already carries the identical constant — not an attack the code
detects. So no new mechanism is needed; what is needed is for the text to say this.

Please either rewrite the stamp-echo section to state the delivered property and name
stamped-to-stamped copying explicitly as a no-op rather than a blocked transplant, or implement
the per-record linkage the current wording describes. Narrowing the text is the cheaper and more
honest of the two, and it matters because the base ADR's own rule forbids describing a mechanism
as more than it is — a later reader who believes stamps are target-bound may build on a guarantee
that was never there.

This rides with the four edits in the previous comment.

@ohdearquant

Copy link
Copy Markdown
Owner Author

Update to the first requested edit, from something that is not visible inside this change.

#2314 also amends ADR-115, and it adds a top-level section named:

## Amendment 2 (2026-08-29): Permanent mask-only redaction surfaces

This change adds, nested inside Amendment 1:

### 6. Amendment 2: write-inventory, knowledge, and stamp-echo completion

Different content, same name. git merge-tree on the two heads returns clean, because the two
insertions are far apart in the file — so nothing blocks both from landing, and the document
would then carry a top-level Amendment 2 and, inside Amendment 1, a subsection also calling
itself Amendment 2. A clean auto-merge means the edits do not overlap, not that they agree.

Resolution, and it does not depend on merge order: the top-level form in #2314 keeps the name,
since it matches Amendment 1's shape on the base — a ## heading with a date. This section
becomes ## Amendment 3 (<date>), lifted to top level, with a Status line.

That supersedes the earlier note here, which asked only to lift it out of Amendment 1 and give
it Status and Date. The lift and the Status line still stand; the number is now fixed to 3
rather than 2. Nothing else in the earlier list changes.

For completeness on why the nesting mattered independently of the collision: ## Amendment 1 on
the base runs subsections ### 1. through ### 5., so a ### 6. reads as a sixth clause of
Amendment 1 while its title announces a new amendment. The two readings disagree about what the
section is, and only one of them survives a later reader looking for the amendment list.

@ohdearquant

Copy link
Copy Markdown
Owner Author

Blocking: the JSON boundary scans object keys and values in isolation, so a keyed secret is not detected

This one is a correctness defect in the masking path itself, and it outranks the document edits
requested above.

collect_json_strings flattens an object into a list of independent strings, pushing the key and
then recursing into the value:

// crates/khive-runtime/src/secret_gate_finalizer/boundary.rs:486-500
Value::Object(values) => {
    for (key, value) in values {
        if top_level && key == RESERVED_SECRET_GATE_KEY { continue; }
        output.push(key);
        collect_json_strings(value, false, output);

Each element is then scanned on its own — there is no join, so no element ever carries its
neighbour's text:

// boundary.rs:455-461
fn scan_one(..., value: &str, ...) -> RuntimeResult<()> {
    let Some(detected) = secret_gate::scan(value) else { return Ok(()); };

And the high-entropy rule only fires when a trigger word is present in the string being scanned:

// crates/khive-runtime/src/secret_gate.rs:1183-1192
let entropy = shannon_entropy(token.as_bytes());
if entropy < ENTROPY_THRESHOLD { continue; }
// High-entropy token in trigger context — flag it.
if near_trigger { return Some((token, "high-entropy-token")); }

Put together: {"api_key": "Xk9mZ2vQpLrT8nJwYuAeHfBsDcGiONvMabcdef"} is scanned as api_key and as
the token, separately. The key scan has a trigger and no secret; the value scan has a secret and no
trigger. near_trigger is false on the only scan that sees the token, nothing is flagged, and the
value reaches a durable properties column unmasked.

This is INTRODUCEDboundary.rs does not exist on origin/main (git cat-file -e fails
there and succeeds at this head). The separation is inherited from the older scan_json_value
shape, but this change is what makes it the consolidated write-path boundary, which is exactly why
it should be fixed here rather than carried forward.

The direction is the bad one: this is the headline masking surface, and under-masking is durable
exposure that no later step re-checks. A structural-coverage argument — every writer reaches the
boundary — is compatible with the boundary being blind to the most common shape a secret takes in
structured data.

Suggested fix: scan a representation that preserves adjacency, so the key travels with its value
(a path-aware or joined key: value form, including nested and already-stringified objects), or give
the detector an API that accepts structured context. Add a regression with a benign key and a
high-entropy value — that case currently passes while unmasked.


Separately, on process, and this one is mine: there are now five comments from me on this pull
request and every one of them is against the same head. That is one review with four requirements
appended to it, not five rounds, and it is not a reasonable thing to ask an author to converge
against — each visit added a condition without the change having moved. I should have consolidated.

Taking the whole picture — a blocking defect in the masking boundary plus the accumulated
documentation conditions — the honest recommendation is to rework this rather than patch it again.
The write-path consolidation is still the right idea and worth landing; splitting the boundary
correctness fix from the documentation changes would make both reviewable in one pass.

On provenance: an automated pass over this diff ran on a model from the same family as the one that
authored it, so its output was treated as input evidence only. Every fact above — the two code
shapes, the near_trigger condition, and the file's absence on the base — was re-derived from the
refs directly before posting.

@ohdearquant
ohdearquant marked this pull request as ready for review September 1, 2026 16:38

@ohdearquant ohdearquant left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 1c335c6: REQUEST-CHANGES, 3 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.

@oceanwaves630

Copy link
Copy Markdown
Collaborator

Superseded by work already on main. The shared secret-gate boundary this branch proposed exists there as crates/khive-runtime/src/secret_gate_finalizer/, with the manifest contract, the declaration-source markers and a call-site census test enforcing that no write path reaches the masker directly. The knowledge half is covered too: the declared path list includes knowledge writes, and the same-UUID domain mirror atom is present with its collision guard.

Closing with that pointer. Nothing here is a gap in main.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants