fix(kko-guard): the byte check could not fail — a missing **Bytes:** disabled it and still printed "verified" - #41
Open
mdheller wants to merge 1 commit into
Open
Conversation
…disabled it and still printed "verified"
`scripts/check-kko-provenance.mjs` read the declared byte count as
Number((doc.match(/\*\*Bytes:\*\*\s*([\d,]+)/) || [])[1] || '0')
and then compared it as `if (declaredBytes && bytes.length !== declaredBytes)`.
A missing or unparseable **Bytes:** therefore evaluated to 0, `if (0 && …)` never
fired, and the byte comparison silently evaporated.
Worse than absent: the run still exited 0 and printed
✓ KKO provenance verified — d907919fb40f20ed… (327,797 bytes)
quoting the file's OWN length — not the record's — as though it had been checked
against the record. The gate manufactured the evidence that it had run. Deleting
the **Bytes:** line from ontology/kko/PROVENANCE.md left `npm run check:kko`
green, which is exactly the drift this script was added to catch.
**Bytes:** is now a required field: unparseable or absent input dies, mirroring
how the missing **SHA-256:** case is already handled ten lines above, and the
comparison is unconditional.
Second fix — the commit-pin check located the pin with a bare `/\b[0-9a-f]{40}\b/`
scan of the whole document. That does not answer "is the source pinned?", it
answers "does any 40-hex string appear anywhere in this file?": a sha1 of some
other artifact, another repo's commit sha, or a hash inside an unrelated URL all
satisfy it while the source line still reads `@ master`. It is now anchored to the
label that introduces the pin, ``commit `<40-hex>` ``, with a (?![0-9a-f]) tail so
a 64-char sha256 labelled `commit` cannot supply the first 40 of its characters.
The pre-existing digest strip is kept as a second line of defence.
Note for the record: the originally-reported mechanism for this second defect —
that a 40-hex window inside any other 64-hex digest would satisfy the scan — does
NOT reproduce. The `\b` on both ends already blocks every contiguous framing of a
64-hex run. The reproducible weakness is the broader one described above.
The comment at the pin check already said "A checker that validates itself
validates nothing; this repo's estate has been bitten by exactly that shape
before." It is extended to name the instance sitting a few lines above it.
Regression coverage: ts/src/kko-provenance-guard.test.ts drives the real script as
a subprocess against rewritten records in a throwaway sandbox (the repo's own
PROVENANCE.md is never written to). Reverting this fix turns exactly three of its
six cases red; the three controls — including "the committed PROVENANCE.md still
passes" — stay green either way.
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Hardens the KKO provenance gate so missing/malformed **Bytes:** and unpinned commit references fail closed (instead of silently skipping checks), and adds regression coverage that executes the real guard script end-to-end.
Changes:
- Make
**Bytes:**a required, parseable field and always compare declared bytes vs actual bytes. - Anchor the commit pin detection to the
commit <sha>label rather than any arbitrary 40-hex token. - Add subprocess-based regression tests that mutate
PROVENANCE.mdin an isolated sandbox.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| ts/src/kko-provenance-guard.test.ts | Adds end-to-end regression tests that run the real guard against mutated provenance records. |
| scripts/check-kko-provenance.mjs | Hardens byte-count validation and improves commit pin detection to fail closed and avoid accidental matches. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+44
to
+45
| fs.symlinkSync(n3Path, path.join(root, 'ontology', 'kko', 'kko-2.10.n3')) | ||
| fs.symlinkSync(path.join(repo, 'ts'), path.join(root, 'ts')) |
Comment on lines
+75
to
+96
| // The pin is located by its LABEL — ``commit `<40-hex>` `` — not by scanning the document for a | ||
| // bare 40-hex run. An unanchored scan does not answer "is the source pinned?", it answers "does | ||
| // any 40-hex string appear anywhere in this file?", and those come apart: a sha1 of some other | ||
| // artifact, another repo's commit sha, a hash sitting inside an unrelated URL, all satisfy it | ||
| // while the source line still reads `@ master`. The check would pass by accident and report a pin | ||
| // that is not there. | ||
| // Two details are load-bearing: | ||
| // · the (?![0-9a-f]) tail — without it the pattern accepts the first 40 characters of a 64-char | ||
| // sha256 that happens to be labelled `commit`; | ||
| // · the digest strip below — so the artifact's own sha256 can never be the thing that supplies | ||
| // its own pin, even if this pattern is loosened later. | ||
| // (A checker that validates itself validates nothing; this repo's estate has been bitten by | ||
| // exactly that shape before.) | ||
| // exactly that shape before — including by the byte check a few lines above, which until | ||
| // 2026-07-30 switched ITSELF off whenever PROVENANCE.md stopped declaring a byte count, and went | ||
| // on printing a success line that named the bytes it had not verified. Absent input must fail, | ||
| // never skip.) | ||
| const withoutDigest = doc.split(declared).join('') | ||
| const commitPin = (withoutDigest.match(/\b([0-9a-f]{40})\b/) || [])[1] | ||
| const commitPin = (withoutDigest.match(/\b[Cc]ommit\s+`?([0-9a-f]{40})(?![0-9a-f])/) || [])[1] | ||
| if (!commitPin) | ||
| die('ontology/kko/PROVENANCE.md pins no commit sha. "@ master" is a MOVING reference — the same ' + | ||
| 'retrieval later can return different bytes and still claim this provenance. Pin the commit.') | ||
| 'retrieval later can return different bytes and still claim this provenance. ' + | ||
| 'Pin the commit, and record it in the form: commit `<40-hex-sha>`') |
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
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.
The byte check could not fail
scripts/check-kko-provenance.mjsparsed the declared byte count with a|| '0'fallback and then guarded the comparison on the value being truthy:A missing or unparseable
**Bytes:**yields0, andif (0 && …)never fires. The byte check silently evaporates.That is not the bad part. The run still exits 0 and prints a success line naming a byte count, because the number in that line is
bytes.length— the file's actual size — not the value from the record:The gate reports having verified a number it never compared to anything. A check that disappears when its input goes missing is worse than no check: it manufactures the evidence that it ran — and this is the script whose entire purpose is that
ontology/kko/PROVENANCE.md"is verified by NOTHING" was unacceptable.Fix:
**Bytes:**is a required field. Absent or unparseable inputdie()s — mirroring how the missing**SHA-256:**case is already handled ten lines above — and the comparison is unconditional.Proof (all against the real repo,
npm run check:kko)✓ … (327,797 bytes)exit 0✓ … (327,797 bytes)exit 0**Bytes:**line deleted✓ KKO provenance verified …exit 0✗ … does not declare a parseable **Bytes:** <count>exit 1**Bytes:** abc✓ KKO provenance verified …exit 0✗ … does not declare a parseable **Bytes:** <count>exit 1**Bytes:** 999,999✗ … is 327797 bytes, … declares 999999exit 1✗ … is 327797 bytes, … declares 999999exit 1The unmodified record still passes. A checker hardened into rejecting the genuine artifact would be worse than the bug.
Commit pin: anchored on its label
The pin was located by scanning the whole document for a bare 40-hex run. That does not answer "is the source pinned?" — it answers "does any 40-hex string appear anywhere in this file?", and those come apart. Now anchored to the markup that introduces it,
commit `<40-hex>`, with a(?![0-9a-f])tail so a 64-char sha256 labelledcommitcannot supply the first 40 of its characters. The pre-existing digest strip is kept as a second line of defence.Proof — record de-pinned to
@ master, with an unrelated 40-hex sha elsewhere in the document:And the converse — a second sha256 recorded alongside the genuine pin still passes (
exit=0both before and after). No false rejection.Correction to the reported finding
The mechanism as originally reported — "any other 64-hex digest in the document still contains 40-hex windows that satisfy the pin check" — does not reproduce. The
\bon both ends of/\b([0-9a-f]{40})\b/already blocks every contiguous framing of a 64-hex run; there is no word boundary inside one. Measured directly:Constructing that case confirms it: with the source de-pinned and a second 64-hex digest added, the old code already failed correctly. That vector was never open. The reproducible weakness is the broader one above — any unrelated 40-hex token satisfying the check — and that is what this change closes. Flagging it rather than dressing a passing case up as a catch.
Same shape elsewhere in the file
Swept for conditions that degrade to "skipped" instead of "failed".
if (declaredBytes && …)was the only instance — one&&, one|| '<default>', both on the line fixed here. The other four checks (missing**SHA-256:**, missing commit pin, theSocioProphet/kbpedia/CC-BY-4.0needles, and thets/src/kko-data.tsreproduction) all fail closed already.Regression coverage
ts/src/kko-provenance-guard.test.ts— 6 cases, picked up bynpm test, whichts-ciruns as of #36.They drive the real script as a subprocess against rewritten records rather than re-declaring its regexes in the test; a test that re-implements the thing it tests agrees with itself by construction. Each case runs in a throwaway sandbox (copy of the script, copy of the record under test, symlinks back to the real
.n3andts/) — the repo's ownPROVENANCE.mdis never written to. The string surgery asserts it actually bit, so a reworded record fails the fixture loudly instead of quietly testing an unmodified document.Teeth, verified both ways. Reverting
scripts/check-kko-provenance.mjstoorigin/mainand re-running:Exactly the three defect cases go red; the three controls hold either way.
Local verification
npm test— 416 pass, 0 fail (410 + 6 new)npm run typecheck— cleannpm run check:kko— green on the unmodified recordnpm run build—ts/distin sync, no drift, nothing to commitCI status is not asserted here: org Actions capacity is exhausted and jobs are queueing. Pending is not green — please read the checks before merging.
Scope
scripts/check-kko-provenance.mjsand the new test file only.ontology/kko/PROVENANCE.mdis untouched and already satisfies the tightened rules (it records@ commit `3f888b39…`and**Bytes:** 327,797). No overlap withlog-safe.ts,nlq.ts,vendor-graph.tsorts-ci.yml(#34, #37).🤖 Generated with Claude Code