Repository navigation
fix(runtime): sanitize underscore-separated reference labels - #284
Conversation
Held as draft — a reviewer challenges the policy, not the changeTwo reviews. One recommended merge with two P2s (one taken, one correctly rejected — see below). The second raised a P1 that I verified in part and that changes what this PR should be. What I verifiedThe reviewer's factual claims hold:
So in the configuration these runs actually use, every citation any lens emits is "unauthorized", and the reviewer is right that R44's twelve Why I am not making the change the reviewer asks forThe reviewer's remedy is to treat "no catalogue supplied" as unverifiable — warn, keep the identifiers, do not rewrite the lens. I implemented exactly that. It fails seven existing tests, because the current behaviour is deliberate and pinned:
Those tests construct nodes with no catalogue and assert that doc-shaped labels are stripped with a Under that policy this PR is aligned with intent and merely makes it consistent — hyphenated doc labels were already being stripped in precisely the same way; Flipping a deliberately-tested, security-relevant gate is not something I should do unilaterally on one reviewer's read, so I have left the policy alone, kept this PR to the consistency fix, and am recording the disagreement instead of resolving it by fiat. The decision that needs an ownerGiven a lens that cites identifiers present in its supplied pinned-reference documents but absent from any structured catalogue, should the gate:
Option 3 looks right to me and is out of scope here. Until it is decided, this stays a draft. Also from review
39/39 tests in |
`isUnsupportedExternalReferenceLabel` only allowed a hyphen before the digits, so an underscore-separated catalog identifier was not recognized as an unsupported external label. The sanitizer therefore left it in place and `verifyPropertyProvenanceArtifacts` reported it as an unauthorized expectation, failing the node. R44's `property-specification-0kn0t` died on `LEND_ACC_01` this way, while `LEND-01` would have been stripped silently in the same position — so whether a lens node survived turned on the model's choice of punctuation rather than on anything meaningful. Because `property-specification-fanin` depends on all eight lenses, one such node blocks fan-in, invariant setup, Recon, findings and the report. Allow `_` as well as `-` as the separator. This only affects identifiers that are already absent from every supplied catalog, so no authorized expectation changes behaviour. Regression asserts an underscore-separated unauthorized ID is sanitized alongside a hyphenated one, that no unauthorized diagnostic is raised, and that the node does not fail. Fixes #283 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review found the first version was a point fix for a class bug: only `_` was widened, so hyphen-segmented labels still failed the node. Measured through the gate, `LEND-ACC-01` and `CRYTIC-ERC4626-05` both died as unauthorized while `LEND_ACC_01` was stripped — and those multi-segment forms are common in the catalogues these lens prompts cite, so the next label would have reproduced #283 under a new number. Allow segments separated by `-` or `_` in any combination with a numeric suffix. Verified against 22 cases: matches `LEND_ACC_01`, `LEND-ACC-01`, `CRYTIC-ERC4626-05`, `ERC4626-999`, `LEND-01`, `A-1`; rejects `lend_acc_01`, `_LEAD-1`, `A-`, `A-1x`, `LEND01`, `FOO.01`, `4626-01`, `LEND--1`, and the namespaced `benchmark:unexpected` / `scfuzzbench:aave-v4:iSpoke_supply` forms that must keep failing. Review also suggested stripping per identifier so one unrecognised sibling would not poison the rest of the property. Not taken: `"property lens gate strips unsupported external labels without weakening benchmark authority"` already pins the opposite, asserting that a lens mixing a label with a forged namespaced citation is left byte-unchanged and unsanitized. That is deliberate — the artifact as the model wrote it is the evidence the authority check exists to surface, and sanitizing around the forgery would destroy it. The all-or-nothing rule now says so in a comment, and a new test pins the preservation so the next reader does not mistake it for the same oversight. Refs #283 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
8cc40ae to
b393758
Compare
Merging with
|
Integrates origin/main (fa8be64): PR #287's invariant-suite handoff deletion channel and durable provenance, plus #282, #284, #290, #294, #296, #298, #300, #303, #305 and #308. CHANGELOG keeps both Unreleased sections; the modal runner test import list keeps both new symbols. Fixes the review finding on #165 (issue #212). Compiled task specs can only name singular `test/...` output roots, because they are built before the worktree exists. Targets are not consistent: the Aave v4 target at 6959e321 has no `test/` at all, only plural `tests/`. The generated workflow created the singular tree regardless, so such a repository grew empty `test/recon`, `test/chimera` and `test/invariants` directories, which setup.md tells the model to read as the repository's convention, and retry cleanup then scrubbed that invented tree while leaving the real `tests/` output behind. Workspace output roots are now re-anchored at execution time onto the test root the repository actually uses, restoring the semantics of main's `invariantTestRoots` helper that this branch dropped in merge 2bbecba, at the layer this branch's retry preparation uses. Three review findings on the first cut of that fix are addressed here: - Exactly ONE root is selected, never the union of those present. Anchoring under every existing root would have recreated the same defect from the other side: once an ancestor stage introduces the other root, each invariant stage would create an empty duplicate tree beside the real one. Preference follows INVARIANT_TEST_ROOT_NAMES, so a repository that has `test/` behaves exactly as it did before this helper existed and only a repository without it is redirected. - The resolution is pinned per attempt at preparation time. Rediscovering per call was unsafe: prepareTaskWorkspaceOutputRoots finishes by replaying ancestor workspace patches, and an ancestor stage can introduce the other test root as a side effect, so a second independent discovery could select a root whose subdirectories were never created. taskWorkspaceOutputRoots would then lstat a missing path and throw a bare ENOENT instead of the typed artifact-contract failure, killing a retries:0 node on exactly the restart and resume paths this machinery exists to survive. - Only ENOENT and ENOTDIR mean "this root is absent". Swallowing EACCES or ELOOP would silently fall back to the singular default against a root that does exist. Test changes, also from review: - The non-directory case was vacuous: with only a regular file named `test` present, "rejected it" and "fell back to the default" both read `test`. It now runs against a repository that also has a real `tests/`, so the outputs differ; verified by mutation (dropping the isDirectory() guard fails the test). - taskWorkspaceOutputRoots' use of the helper is now pinned in the generated-source assertions. Reverting only that call site previously left every test green while every task on a `tests/`-only target died on a bare ENOENT. - Adds coverage for the pinning behaviour, for per-attempt isolation of the pin, and for surfacing an unreadable candidate root. Also repairs a merge-integration gap the new #217 harness could not have anticipated: it lifts resetTaskArtifactsForRetry, which on this branch calls prepareAnchoredDirectory (branch-only, absent from main), so the harness now supplies that collaborator and drops the dead invariantTestRoots stub. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Summary
The failure
isUnsupportedExternalReferenceLabel(packages/runtime/src/artifact-gates.ts) was:[A-Z0-9]*cannot span an underscore, soLEND_ACC_01did not match. The sanitizer's guard then bailed out for that property, the ID survived intoverifyPropertyProvenanceArtifacts, and it was reported asPROPERTY_REFERENCE_EXPECTATION_UNAUTHORIZEDat severityerror— failing the node.LEND-01in the same position would have been stripped silently. So whether a lens node survived turned on the model's choice of punctuation.R44 (
aave-v4-v0012-main-issue272-r44-invariant-only, imageim-01KZBJ546AZANSE7RNBY35A9DG), nodeproperty-specification-0kn0tat2026-08-06T14:49Z:Run state:
19 succeeded / 2 failed / 11 pending, runfailed,property-specification-faninnever ran. Because fan-in depends on all eight lenses, one node failing this way blocks fan-in, invariant setup, Recon, findings andreport.md.The fix
Allow
_as well as-as the separator:This is reached only for identifiers already absent from every supplied catalog, so no authorized expectation changes behaviour.
Validation
LEND_ACC_01PROPERTY_REFERENCE_EXPECTATION_UNAUTHORIZEDis raised, and that the node does not failgit diff --checkcleanWorth deciding separately
This makes the existing behaviour consistent rather than settling what it should be. An unauthorized reference expectation is currently sanitized in some cases and fatal in others, and that split was an accident of the regex. Whichever is intended — drop a citation that does not resolve, or reject an artifact that fabricates one — should apply uniformly. I have kept the existing sanitize-and-continue behaviour and only made it consistent, rather than changing the policy unreviewed.
Related
Greptile Summary
The PR broadens unsupported external-reference label recognition so underscore-separated and multi-segment catalog labels are sanitized consistently before provenance verification.
Confidence Score: 5/5
The PR appears safe to merge.
No blocking failure remains.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart LR A[Property lens artifact] --> B[Read supplied catalog IDs] B --> C{Unauthorized ID shape} C -->|External catalog label| D[Remove unauthorized reference] D --> E[Refresh verification digest] E --> F[Continue node successfully] C -->|Forged namespaced citation| G[Preserve artifact] G --> H[Provenance verifier reports unauthorized citation] H --> I[Fail node]Reviews (3): Last reviewed commit: "fix(runtime): match multi-segment catalo..." | Re-trigger Greptile
Context used: