fix(meta-285): make the candidate-consumption harness admissible evidence - #12
Merged
qmarcelle merged 6 commits intoAug 13, 2026
Conversation
…ence The harness that gates the 0.4.5 candidate train could not be trusted in either direction: some checks could never pass, others could never fail, and a crash mid-run still emitted a clean verdict. Structural changes: - Every assertion moves into scripts/migration/consumption-checks.mjs as a pure predicate the harness calls and the tests exercise. One implementation, reachable by tests. Inline assertions are how a check enters service unwatched. - run() now requires an explicit cwd. The old repoRoot default is how the installer --help probe came to run a real install into the source checkout; the probe now runs in a disposable sandbox that is then asserted empty, and the source tree is fingerprinted before/after as a recorded check. - The check plan is declared up front, so unreached checks are recorded as not_run rather than omitted. The verdict was previously computed over only the checks that happened to execute, so an abort after seven passes emitted "7/7 passed - CONSUMABLE". CONSUMABLE now requires that every planned check ran. - The MCP client closes in a finally. A throw between connect and close leaked the spawned server and hung CI instead of failing it. Vacuous-pass defects found by auditing the check set in the direction the original review did not: - hook.denies read `status !== 0`. spawnSync returns status null when the process never launched, and null !== 0 is true, so a missing hook scored a PASS and the receipt claimed the artifact enforced denial. - mcp.tool-call-responds read `!result.isError`. isError is optional on CallToolResult, so any malformed result passed. verify-receipt.mjs exports compareReceipts, which its test now imports instead of reimplementing; the copy's messages had already drifted from the source. readReceipt reports missing/malformed input with the role and path, and exits 2 (did not run) rather than 1 (diverged). An empty check set is refused rather than compared clean against another empty one. Every check in CHECK_PLAN is demonstrated red against a controlled broken input and green against a valid one, and a coverage test fails if a check is added to the plan without both. Refs META-285, META-140, META-165, META-238
Running the harness against a candidate built with hooks/ removed from package.json files[] exposed a vacuous pass in the previous commit's own replacement predicate. `node <missing-script>` launches successfully — the node binary exists — and exits 1 after failing to resolve the module. checkHookDenies asked only "did it launch, and was the exit non-zero", so a candidate shipping no hook at all scored a passing deny check. Same shape as the original `status !== 0`, one level further in. The deny is now proven by exit code AND the absence of a Node module-resolution or unhandled-error signature in the output. A genuine deny whose message happens to contain a path-like fragment still passes. Worth recording how this was found: the unit cases did not catch it. Only running the whole harness against a deliberately broken candidate did, which is the argument for that acceptance criterion rather than a formality to satisfy after the fact. Refs META-285
Reviewer's GuideRefactors the standard-candidate consumption harness so all checks are centralized in a reusable module, fully test-covered (watched red/green), and its receipts and parity verifier become trustworthy, non-destructive evidence of candidate consumability and CI parity. Sequence diagram for the updated consumption harness run and receipt generationsequenceDiagram
actor Dev
participant Harness as consume_standard_candidate_mjs
participant Checks as consumption_checks_mjs
participant Recorder as createRecorder
participant Receipt as consumption_receipt_json
Dev->>Harness: node consume-standard-candidate.mjs [--out <dir>]
Harness->>Checks: import CHECK_PLAN, check* predicates, createRecorder
Harness->>Recorder: createRecorder()
Harness->>Harness: treeFingerprint() (before)
note over Harness: Steps 1-6: pack, install, MCP, hook, installer
Harness->>Checks: checkTarballExists(...)
Harness->>Recorder: record("pack.tarball", outcome)
Harness->>Checks: checkToolCallResponded(...)
Harness->>Recorder: record("mcp.tool-call-responds", outcome)
Harness->>Checks: checkHookDenies(...)
Harness->>Recorder: record("hook.denies", outcome)
Harness->>Checks: checkInstallerHelp(...)
Harness->>Recorder: record("installer.help-usage", outcome)
Harness->>Harness: treeFingerprint() (after)
Harness->>Checks: checkTreeUnchanged(before, after)
Harness->>Recorder: record("repo.tree-unchanged", outcome)
Harness->>Recorder: buildReceipt({ aborted? })
Recorder-->>Harness: receipt {verdict, summary, checks[]}
Harness->>Receipt: write consumption-receipt.json
Harness->>Dev: log verdict (CONSUMABLE / NOT_CONSUMABLE / INCOMPLETE)
Harness->>Dev: exitCode (0 if CONSUMABLE, else 1/2)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
|
| Filename | Overview |
|---|---|
| scripts/migration/consume-standard-candidate.mjs | Refactors the packed-candidate workflow around explicit working directories, reusable predicates, reliable client cleanup, and complete receipt generation. |
| scripts/migration/consumption-checks.mjs | Introduces the declared check plan, defensive predicates, and receipt recorder used by the consumption harness. |
| scripts/migration/verify-receipt.mjs | Adds reusable receipt parsing and shape checks, but duplicate check IDs still collapse during Map-based comparison. |
| tests/migration/consumption-checks.test.ts | Adds bidirectional predicate and check-plan coverage for the consumption harness. |
| tests/migration/verify-receipt.test.ts | Expands direct comparator and malformed-input coverage, while leaving duplicate-ID behavior uncovered. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Build and pack candidate] --> B[Install packed artifact in temporary directory]
B --> C[Exercise installed MCP server]
C --> D[Exercise packed hook]
D --> E[Run installer help in empty sandbox]
E --> F[Check source tree unchanged]
F --> G[Build receipt from declared check plan]
G -->|Failures present| H[NOT_CONSUMABLE]
G -->|Checks not run| I[INCOMPLETE]
G -->|All checks pass| J[CONSUMABLE]
Reviews (2): Last reviewed commit: "Merge branch 'main' into feature/meta-28..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…didate-harness-three-checks
qmarcelle
added a commit
that referenced
this pull request
Aug 12, 2026
Records the 2026-08-12 branch-protection change: Greptile Review added to required contexts, pinned to app id 867647, with strict, both build-and-smoke contexts, and required_conversation_resolution preserved. Sourcery deliberately not promoted. Also records the first post-policy proof on PR #12 at 76d495d and the accepted cost to stale PR #6. Refs META-322
…n PATH trust Three findings from review, all in the same class the PR already treats as its subject: a check that cannot fail, or that trusts something it should have proven. Duplicate check ids (Greptile P1). `compareReceipts` projects `checks` into a Map keyed by id, which keeps only the last entry for a repeated id. A receipt carrying the same check twice with divergent statuses lost the earlier one and compared clean against the survivor — an internally inconsistent receipt reported as PARITY. `assertReceiptShape` now rejects duplicates on both sides. `createRecorder` already refused to emit such a receipt; this refuses to read one, so neither end of the gate trusts it. Watched red: three cases fail against the previous comparator and only those three. Bare `git` on PATH (Sonar S4036, the Security rating). `treeFingerprint` is the evidence that the harness did not mutate the source checkout, and it resolved `git` through PATH — letting a writable PATH entry decide what "git status" reports. The one tool that must not be forgeable is the one asked to prove nothing changed. Resolved from fixed locations; absence yields a non-string fingerprint, which `checkTreeUnchanged` already fails on rather than skipping. Bare `node` for the spawned server, hook, and installer. `process.execPath` instead, so the packed artifact runs under the same Node the harness runs under. As written, the Node 20/22 CI matrix was only ever applied to the harness process; whatever `node` resolved to on PATH ran the server under test, so one matrix dimension was not testing what it claimed to. Also: comparator sorts with an explicit comparator (S2871), and the crash detector's `^\s*` no longer spans newlines under /m — `\s` matches `\n`, so the indent could be consumed across line boundaries, which both backtracks super-linearly and let a non-indented "at" line match (S8786). Harness re-run end to end: 21/21 CONSUMABLE, tree unmodified.
There was a problem hiding this comment.
qmarcelle has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
…didate-harness-three-checks
There was a problem hiding this comment.
qmarcelle has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
qmarcelle
added a commit
that referenced
this pull request
Aug 13, 2026
The §2 table claimed three required contexts. The API returns five —
`standard-candidate-consumption` and `SonarCloud Code Analysis` were promoted
without the change reaching this file.
This is the case §2 already legislates for ("the API is right and this file is
stale — treat that as a documentation defect, and correct it in the PR that
noticed"), exercised for the first time. The drift is its own argument: the
stale table said `SonarCloud Code Analysis` was non-blocking, so a reader
trusting it would have concluded PR #12 was mergeable while it was blocked on
exactly that context.
Corrected:
- §2 required-context row and the sentence restating it.
- §2 "Observed gap" — `standard-candidate-consumption` is now required, leaving
`parity-receipt-reproduction` as the only CI job whose failure does not block.
That is this PR's subject, so the section now names the dependency rather than
describing a gap that has since narrowed.
- §2 and §4 PR #12 observations re-dated to the protection in force when they
were taken. They are kept, not rewritten — the mechanism they demonstrate is
unaffected, only the non-blocking claim expired.
- Provenance line re-measured: 2026-08-13 against `main` at `70cfd57`.
No protection setting was changed by this commit. It only makes the file agree
with what the API already returns.
qmarcelle
added a commit
that referenced
this pull request
Aug 13, 2026
) Prerequisite for promoting `parity-receipt-reproduction` to a required check under META-337. Both substantive steps of the job were gated on `has_receipt == 'true'`, so a branch with no committed receipt skipped them, concluded `success`, and logged "No committed parity receipt found — skipping reproduction check." Harmless while the job is advisory; merge-authorizing the moment it is required, because the cheapest way to satisfy a failing parity gate would be to delete the receipt. Absence is now a hard failure, and the failure output names the invariant, the authority, the evidence that discharges it, and what must not be weakened. Also in this PR: - The step is pinned to `shell: bash`, since its semantics depend on `set -euo pipefail` and runner defaults are an implementation detail (Sourcery review). - `docs/review/merge-policy.md` §2 corrected against the live API. It claimed three required contexts; the API returns five —`standard-candidate-consumption` and `SonarCloud Code Analysis` had been promoted without the change reaching the file. §2 already legislates for exactly this ("the API is right and this file is stale — correct it in the PR that noticed"); this is its first exercise. The drift mattered: the stale table called SonarCloud non-blocking, so a reader trusting it would have judged PR #12 mergeable while it was blocked on that context.
…didate-harness-three-checks
There was a problem hiding this comment.
qmarcelle has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
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.
Closes META-285.
The harness that gates the 0.4.5 candidate train could not be trusted in either direction. Some checks could never pass, others could never fail, and a crash mid-run still emitted a clean verdict.
Repository state vs the issue description
Two of the three "always-fail" checks the issue names had already been repaired in-tree by
296e7c0andce0a175. They were never demonstrated red-then-green, so they were unverified fixes, and they get the same treatment here as the rest.a31242bcallTool()destructured fortoolslistTools()path === "hooks/"startsWith--helprepoRootclient.close()happy-path onlyfinallyENOENT/SyntaxErrorThe audit the review did not run
The issue asked whether any check can never fail. Three found:
hook.deniesreadstatus !== 0.spawnSyncreturnsstatus: nullwhen the process never launched, andnull !== 0is true — a candidate shipping no hook scored a passing deny.mcp.tool-call-respondsread!result.isError.isErroris optional onCallToolResult, so any malformed result passed.failures === 0 ? "CONSUMABLE"over only what executed. A throw after seven passing checks emitted7/7 passed — CONSUMABLE.A fourth was found by running the harness, not by unit tests:
node <missing-script>launches fine and exits 1, so the replacementhook.deniesstill read a module-load crash as a deny. Fixed in1dc3784. This is the argument for the end-to-end watched-red criterion rather than a formality.Structure
Every assertion moved into
scripts/migration/consumption-checks.mjsas a pure predicate the harness calls and the tests exercise — one implementation, reachable by tests.run()now requires an explicitcwd; there is norepoRootdefault left to forget. Installer--helpruns in a disposable sandbox that is then asserted empty, and the source tree is fingerprinted before/after as a recorded check.The check plan is declared up front, so unreached checks appear as
not_runandCONSUMABLErequires that every planned check actually ran.Evidence
Three real harness runs, receipts inspected:
CONSUMABLEhooks/removed fromfiles[]NOT_CONSUMABLEINCOMPLETEThe third is the case the old harness reported as
8/8 passed — CONSUMABLE.Mutation campaign: each defect reintroduced one at a time, suite must go red. 12/12 RED. A test that stays green under its own defect is not evidence.
npm run checkALL GREEN. 222/222 tests (baseline 147, +75). Working tree clean before and after every harness run.Changesets
No changeset.
workspacejson/integrationshas no.changeset/directory, and the fixed group in §6 of the Aug 12 contract covers@workspacejson/specand@workspacejson/rulesin the Standard repo. This PR touches onlyscripts/migration/**andtests/migration/**— verification scripts and tests. No published@workspacejson/codex-mcpbytes, exports, schema, or metadata change. Verified againstorigin/main...HEAD: all five changed paths (scripts/migration/{consume-standard-candidate,consumption-checks,verify-receipt}.mjs,tests/migration/{consumption-checks,verify-receipt}.test.ts) fall outsidepackage.jsonfiles[], which is itself unchanged (dist,hooks,scripts/install.mjs,.codex-plugin,.mcp.json,vsix,README.md,LICENSE).src/is untouched, sodist/is unchanged.scripts/install.mjs— the one shipped script — is deliberately not modified.Merge order
Per §4/§7 of the Aug 12 contract, META-322 calibrates the Integrations review contract and merges first. This PR is not merge-eligible until that calibrated current-head contract exists and this head passes it. No
.greptile/**or review-policy files are touched here.Summary by Sourcery
Harden the standard-candidate consumption harness and receipt verifier so their outputs are trustworthy evidence of package consumability and CI parity.
New Features:
Bug Fixes:
Enhancements:
Tests: