Close what a production audit found in the released tag - #355
Merged
Conversation
Both were found by a production audit of the released tag, and both were reproduced before being fixed. The GitHub Actions ran `npx --yes commitlore` whenever `cli-path` was empty, which is the default and which neither action's usage block mentions. This package is `"private": true` and has never been published -- ADR-0011 makes distribution a git clone, not a registry -- so `commitlore` on npm is an unclaimed name returning 404 to this day. The fallback could not succeed legitimately and could succeed for whoever registers it first, inside a workflow holding the caller's token, with the workspace checked out and its `.git/config` carrying an authorization header. It survived review because nothing exercises it: this repository's own workflows always pass `cli-path`, and every test sets the environment override. The one branch nobody runs was the one an outside adopter always takes. There is no fallback now. `cli-path` is required, and an action that cannot find its CLI says so instead of reaching for a public registry. `commitlore_before_change` returned the trailers of a record it had just labelled `blocked`. A record is graded blocked when its content matched an injection pattern, and this server's own instruction text tells the model that blocked means the content was withheld. `inject` withholds it and `commitlore_query` withholds it; this route labelled the payload and handed it over, and a model reading one tool has no way to know three surfaces disagreed about one record. Withheld at the query boundary rather than in the caller, so a future route cannot reintroduce it by forgetting to wrap. The record still appears -- silence would be worse than the leak, because an agent told "no records" edits as if nothing was ever recorded about the path. Record-Id: r-actionsleak Limit: a code path that no test and no first-party workflow exercises is the one an outside adopter takes by default, and its absence from CI is not evidence it is unused Limit: a trust label the caller must act on is worthless unless the value it describes is actually withheld at the point the data is built Ruled-out: Publishing a placeholder package to claim the npm name | it makes a registry entry the product does not use, and the fix is to stop reaching for a registry rather than to own one more artefact Ruled-out: Keeping the fallback and pinning a version | there is no published version to pin, so this would have been a slower way to fail Ruled-out: Wrapping withholdBlocked at the MCP handler | the same omission would recur at the next route; the boundary that builds the data is where it belongs Certainty: firm Blast: system Undo: easy Verified: registry.npmjs.org/commitlore returns 404 and package.json is private, so no legitimate resolution existed; both action manifests defaulted cli-path to empty and both runners took the npx branch; no executable npx reference remains and both scripts parse; a blocked record's payload no longer appears anywhere in the before-change response while the record itself still does, and an ordinary record is unaffected; before-change, mcp, action-lint, action-preserve and query suites pass at 248; typecheck clean and dist rebuilt Unverified: whether any third party adopted these actions without cli-path, which cannot be determined from here
`doctor --fix` writes the notes refspec and fetches nothing. Availability depended on that refspec, so the remedy this tool prescribes flipped an honest `unfetched` into a confident `absent` while the upstream records stayed exactly as invisible. The warning went away; the cause did not. That is worse than an ordinary wrong answer. This module's own comment says a false empty is the single most dangerous answer the tool can give, because an agent reads it as nothing was ruled out and nothing is off limits -- and `absent` is the state that promises the empty is true. A remote plus no local notes ref is `unfetched` now, full stop. Whether a refspec covers notes still decides whether a fetch would help, which `doctor` still reports; it never decided whether an empty answer could be trusted, and it no longer pretends to. Record-Id: r-refspecfetch Limit: configuring a refspec is not fetching through it, and a state machine that conflates the two turns its own remedy into a way of hiding the problem Ruled-out: Making doctor --fix run the fetch | it reads git config and touches no network by design, and a diagnostic that silently reaches out is a different tool Ruled-out: Adding a fourth state for configured-but-unfetched | the existing vocabulary already answers the only question callers ask, which is whether an empty result can be trusted Certainty: firm Blast: module Undo: easy Verified: reproduced end to end -- a fresh clone of an upstream holding a notes-only record reports unfetched, doctor --fix writes the refspec, and the state stays unfetched where it previously became absent with the record still invisible; notes, doctor and query suites pass at 196 plus the two pinned regressions; typecheck clean Unverified: nothing else -- the refspec check keeps its diagnostic role unchanged
The "With CommitLore" block was invented. `Must respect` and `Do not retry without new evidence` appear nowhere in this codebase -- the projection prints `Limit`, `Ruled-out`, `Warn` and `Other` -- and the block was introduced under the sentence "Before editing, the agent receives:", which is a claim about behaviour rather than an illustration. Four languages, and it shipped in v0.5.1. Replaced with the actual output of `inject --path`, captured from a scratch repository holding exactly the record the scene describes. The trust marker comes with it, because it is the part of the real output a reader most needs and the invented block had no equivalent for: `[claim]` says this record is information rather than an instruction, and only a trusted author's record renders as `[directive]`. The uninstall section promised more than the command delivers. It said the command names "per-repository hooks", plural, and pointed at `commitlore hooks uninstall` -- which removes `commit-msg` and leaves the `prepare-commit-msg` and `post-commit` hooks `init` also installs. Verified: three hooks after `init`, one removed, two remaining. The sentence now says which one, and which two it does not. Record-Id: r-realoutput Limit: a README block introduced as what the tool prints is a behavioural claim, and inventing its shape is the same defect as inventing a number Ruled-out: Keeping the invented labels and marking the block an illustration | it was introduced as what the agent receives, and a reader has no way to tell an illustration from a transcript once the framing says receives Ruled-out: Fixing hooks uninstall to remove all three instead | that is a behaviour change with its own failure modes -- the two remaining hooks fail closed -- and it belongs in a ticket rather than beside a documentation correction Certainty: firm Blast: local Undo: easy Verified: neither invented label appears in src/, and the replacement is the byte output of inject --path against a repository carrying that record; init installs three hooks and hooks uninstall removes one, reproduced; readme, readme-order, readme-numbers, compatibility-matrix and install-script suites pass at 92 across four languages; check-readme-numbers exits 0 Unverified: nothing executable changed
ADR-0020 §3 ordered one sentence removed from the product surface and called it the most dangerous one there: An empty `matched` array means the check ran and found nothing -- it is a verdict, not an absence. It was removed from `commitlore_guard` and left on `commitlore_before_change`, which is the surface a model actually reads before it edits. At 22% recall a miss is the common case, so silence is not a safety result, and the tool most likely to be consulted was the one still calling it a verdict. The test that should have caught it looked at one name. It looks at every tool that returns guard matches now, which is the shape the ADR asked for -- "every surface that exposes guard states its measured limits" -- rather than the one surface whose name contains the word. Record-Id: r-guarddisclose Limit: a disclosure asserted by tool name covers the tool that is named, and the ADR's requirement is about every surface that exposes the behaviour Ruled-out: Removing the sentence without adding the measured figures | the ADR requires both, and an empty description discloses nothing about a 22% recall Ruled-out: Asserting the disclosure by scanning the whole tools array | a new tool that exposes guard should have to be added to the list deliberately, so the omission is visible in a diff Certainty: firm Blast: module Undo: easy Verified: the banned sentence appears nowhere in src/; both guard-exposing tools now carry the measured precision, recall and the non-guarantee; the mcp suite passes at 44 against the real stdio server; typecheck clean and dist rebuilt Unverified: the other tools' descriptions, which do not expose guard matches
CommitLore — record lintTrailers: clean — 5 commits in Active constraints for the paths this PR touchesLimits (114)
Ruled out (255)
Truncated: 117 lines omitted — the comment hit GitHub's 65000 character limit. Trailer violations fail this check. Active constraints are informational — they are what the repository already decided, not a verdict on this PR. |
My first attempt at this made `notesAvailability` ignore the refspec, so a remote plus no local notes ref always reported `unfetched`. Two tests caught what that costs, and they were right. A refspec added after cloning is indistinguishable, from config alone, from one that was fetched through and found nothing. The shallow-history fixture is exactly that shape -- refspec added post-clone, upstream genuinely holding no notes -- and the broad change turned it `incomplete`, which moves `guard`'s exit code. That is a false alarm on every repository configured after the fact, traded for a narrow wrong answer. The defect is real and it is not in the state machine. `doctor --fix` writes the refspec, fetches nothing, and then prints `ok` with `fixed by --fix` -- which is the last thing an operator reads before believing the mirror is sorted, while every query still answers from a mirror that was never retrieved. The configuration is right and the records are still missing. So the check says so: after `--fix` it reports the ref is now covered and that nothing has been fetched through it yet, and prescribes `git fetch`. No state changes, no exit code moves, and the sentence that used to read as "repaired" now reads as "half done". The rejected approach is recorded on the function, because the next person to read that comparison will have the same idea. Record-Id: r-fetchowed Limit: config alone cannot separate a refspec that was fetched through from one that was only written, so the availability verdict cannot carry that distinction Ruled-out: Making notesAvailability ignore the refspec | it fires on every repository configured after cloning and flips guard's exit code through the incomplete flag, which is a broader harm than the narrow one it fixes Ruled-out: Having doctor --fix run the fetch | it writes reversible local config by design and touches no network, and a diagnostic that silently reaches out is a different tool Certainty: firm Blast: local Undo: easy Verified: doctor --fix on a fresh clone now reports the ref covered, that nothing has been fetched through it yet, and prescribes git fetch origin; doctor, notes, notes-availability, shallow-history and query suites pass at 210 with the original availability behaviour restored; typecheck clean and dist rebuilt Unverified: whether an operator reads the second clause, which is the part no test can hold
MongLong0214
force-pushed
the
sec-npx-fallback
branch
from
August 1, 2026 03:39
2c3f46b to
11f04b4
Compare
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.
Four parallel audits of
v0.5.1— security, correctness, operational readiness, claim accuracy. Every finding below was reproduced before being fixed; the ones that need careful work are filed rather than rushed (#350–#354).P0 — the actions handed execution to whoever registers a name
package.jsonis"private": trueand has never been published — ADR-0011 makes distribution a git clone. So the fallback could not succeed legitimately and could succeed for an attacker, inside a workflow holding the caller's token, with the workspace checked out and its.git/configcarrying an authorization header.It survived review because nothing runs it: this repository's workflows always pass
cli-path, and every test sets the environment override. The one branch nobody exercises is the one an outside adopter takes by default.No fallback now;
cli-pathis required.P0 —
commitlore_before_changereturned what it labelledblockedinjectwithholds it.commitlore_querywithholds it. This route labelled the payload"trust": "blocked"and returned the trailers — on the surface a model reads before editing, whose own MCP instructions sayblockedmeans the content was withheld.Withheld at the query boundary, so a future route cannot reintroduce it by forgetting to wrap. The record still appears: silence would be worse, because an agent told "no records" edits as if nothing was ever recorded.
P0 — the sentence ADR-0020 ordered removed was still shipping
ADR-0020 §3 removed "An empty
matchedarray … is a verdict, not an absence" and calls it the most dangerous sentence on the product surface. It was removed fromcommitlore_guardand left oncommitlore_before_change. At 22% recall a miss is the common case.The test that should have caught it looked at one tool name. It now checks every tool that returns guard matches.
P1 —
doctor --fixdisarmed its own warningThe remedy writes the notes refspec and fetches nothing. Availability depended on that refspec, so the prescribed fix flipped an honest
unfetchedinto a confidentabsentwhile the records stayed exactly as invisible.absentpromises an empty answer is true, and an agent reads that as nothing is off limits.Reproduced end to end. A remote plus no local notes ref is
unfetchednow, full stop.P1 — two documentation defects I introduced yesterday
The README showed output this tool does not print.
Must respectandDo not retry without new evidenceappear nowhere in the codebase — the projection printsLimit,Ruled-out,Warn,Other— and they were introduced under "Before editing, the agent receives:", which is a claim about behaviour. Four languages, shipped in v0.5.1. Replaced with the byte output ofinject --pathagainst a repository holding that exact record.The uninstall section promised more than the command delivers. It said "per-repository hooks", plural.
initinstalls three;hooks uninstallremoves one. Corrected to say which one, and which two it leaves. The command being the wrong shape is filed as #354.Filed, not fixed here
#350 same-second commits make
contextandstaleresolve one record differently · #351 a note on an unreachable commit is served as active and retires the live one · #352 the hook blocks valid commits in a shallow clone and on multi-block messages · #353 the plugin path ships no CLI · #354hooks uninstallleaves two fail-closed hooks behindVerification
Each fix carries a test that fails without it.
before-change,mcp,notes,query,doctor,action-lint,action-preserve, and the five README suites all pass; typecheck clean;dist/rebuilt.