fix: widen the ts-ci PR filter to match its own push filter; consume contractViolations - #40
Conversation
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.
Fixes two defects: (1) ensures the TS required CI check runs for PRs that affect ts/dist indirectly via vendored ontology/scripts changes, and (2) enforces the documented rule that re-vendor proposals with contract violations are not emitted, while still reporting those violations in the sealed analysis.
Changes:
- Widen PR change detection in
ts-ci.ymlto includeontology/**andscripts/**, and add a guard asserting PR detection coverson.push.paths. - Update
analyzeVendorFreshnessto withhold contract-violating proposals and surface them as a new top-levelcontractViolationsfield in the sealed record. - Add tests verifying withholding + reporting behavior and that
contractViolationsparticipates in the seal hash.
Reviewed changes
Copilot reviewed 3 out of 7 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
ts/src/vendor-graph.ts |
Adds top-level contractViolations to the sealed analysis and filters out invalid proposals. |
ts/src/vendor-graph.test.ts |
Adds regression tests ensuring violating proposals are withheld and still reported/sealed. |
.github/workflows/ts-ci.yml |
Aligns PR path detection with push paths and adds a self-guard to prevent filter drift. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /** | ||
| * Proposals WITHHELD because they did not validate against the vendored contract, as | ||
| * `${pinKey}: ${violation}`. `RevendorProposal.contractViolations` says a violating proposal is | ||
| * not emitted; this is where the ones that were not emitted are named, so the withholding is | ||
| * visible in the seal instead of being a silent omission. | ||
| */ | ||
| contractViolations: string[] | ||
| /** | ||
| * Re-vendor proposals. Emitted, never executed; empty unless `requestedAt` is supplied. | ||
| * Contains only proposals that validated — see `contractViolations` for the rest. | ||
| */ |
There was a problem hiding this comment.
Fair — the doc comment should say so. contractViolations is derived from the same candidates list as proposals (both are produced by the revendor pass), so it is likewise empty unless requestedAt is supplied. The behavior is already that; only the doc omits the constraint that proposals states explicitly. Worth a one-line doc tweak to name it; not a behavior change.
| python3 - <<'PY' | ||
| import os, re, sys, pathlib | ||
| wf = pathlib.Path('.github/workflows/ts-ci.yml').read_text().split('\n') | ||
| start = next(i for i, l in enumerate(wf) if l.startswith(' push:')) | ||
| end = next(i for i, l in enumerate(wf[start + 1:], start + 1) if re.match(r'^ \S', l)) | ||
| declared = re.findall(r'^\s*-\s*"([^"]+)"', '\n'.join(wf[start:end]), re.M) | ||
| if not declared: | ||
| sys.exit('::error::could not read on.push.paths from ts-ci.yml — this guard has gone blind') | ||
| rx = re.compile(os.environ['TS_PATHS']) | ||
| # One representative path per declared family; a glob stands for a file beneath it. | ||
| bad = [p for p in declared if not rx.match(p.replace('**', 'x').replace('*', 'x'))] | ||
| if bad: | ||
| sys.exit(f'::error::on.push.paths declares {bad} relevant, but the PR change-detector ' | ||
| f'regex does not match them. A PR touching only those paths would report ' | ||
| f'build-and-verify-dist green without building anything.') | ||
| print(f'detector covers all {len(declared)} declared push paths: {declared}') |
There was a problem hiding this comment.
Keeping the guard — both concerns resolve to a false RED under reformatting, never a false pass, so it fails safe. (1) If on.push.paths were rewritten as unquoted scalars, declared would be empty and the very next line if not declared: sys.exit('::error::...has gone blind') fails the step loudly — it does not silently pass. (2) end = next(...) raising StopIteration if push: were the last on: key is likewise an uncaught error → red job, not a skipped build. The guard's job is to catch drift between on.push.paths and the PR change-detector, and it has teeth: verified that adding a docs/** push entry the TS_PATHS regex doesn't cover makes it exit 1 with "would report build-and-verify-dist green without building anything." Falling back to len(wf) and accepting unquoted items is reasonable hardening, but there is no false-green to close here.
…h filter
ts-ci.yml declared ontology/** and scripts/** relevant in on.push.paths, but the
PR change-detector matched only ^(package(-lock)?\.json|ts/|\.github/workflows/ts-ci\.yml).
The derivation those push paths exist for is real:
ontology/kko/kko-2.10.n3 -> scripts/gen-kko.mjs -> ts/src/kko-data.ts
ontology/effect-request/*.json -> scripts/gen-effect-request.mjs -> ts/src/*
ontology/semantic-action/*.json -> scripts/gen-semantic-action.mjs -> ts/src/semantic-action-data.ts
-> ts/dist
so a re-vendor touching only ontology/ took the "required check trivially
satisfied" branch and build-and-verify-dist -- a REQUIRED check on main, per
branches/main/protection: ["build-and-verify-dist","CodeQL"] -- went green
having built nothing.
Worth stating precisely: the kko-provenance job does cover the FIRST of those
three chains, on every PR and with no path filter. But it is not a required
check, and nothing covers effect-request or semantic-action at all.
The regex moves to a TS_PATHS env var used by the grep, widened with ontology/
and scripts/. A new step then derives on.push.paths from this file and asserts
the detector matches every declared family, so widening one filter and
forgetting the other fails loudly instead of silently skipping a build.
Red-then-green, same probe against both trees:
origin/main ontology/kko/kko-2.10.n3 -> skipped (green, built nothing)
ontology/effect-request/EffectRequest.json -> skipped
scripts/gen-kko.mjs -> skipped
assertion: FAILS -- push declares ['ontology/**','scripts/**'], detector does not match
fixed all of the above -> BUILDS
README.md, rust/src/lib.rs -> still skipped
assertion: PASSES -- detector covers all 5 declared push paths
…nread
`RevendorProposal.contractViolations` is populated by proposeRevendor and sealed
into the analysis, and its own doc comment has always said "Non-empty means the
proposal is NOT emitted". Nothing implemented that sentence. Across the repo the
field occurs 17 times: 8 in the four compiled ts/dist copies, 4 in vendor-graph.ts
(the interface field, the definition, the populate, the return), and 5 in tests.
No production reader -- no branch, no `.length`, no throw, no exit code.
analyzeVendorFreshness sealed every candidate regardless. Meanwhile the sibling
dispositionViolations IS a named top-level field of the sealed analysis, and the
validator provably works (vendor-graph.test.ts's TEETH test is a real counter-test).
The declared rule is now the actual rule. A candidate whose contractViolations is
non-empty is WITHHELD from `proposals`, and its violations are named in a new
top-level sealed field `contractViolations`, formatted `${pinKey}: ${violation}`
exactly as dispositionViolations is.
Both halves are needed. Withholding alone is a silent drop. Naming alone leaves a
consumer free to act on a proposal the contract says was never emitted.
Reported, not thrown, for three reasons: analyzeVendorFreshness is documented PURE
and deterministic; one malformed pin must not destroy the verdicts for every other
pin in the run, which is why dispositionViolations is a list and not an exception;
and the module's whole doctrine is that the engine proposes while a membrane gate
outside it decides -- throwing would be the engine taking an action.
proposeRevendor's own signature and behaviour are unchanged, so direct callers see
no difference. For a conforming graph the observable output is also unchanged; only
the new empty `contractViolations: []` is added, which does change the seal hash.
No test pins a literal analysis hash.
Red-then-green, ts/src/vendor-graph.test.ts:
restored 23 pass / 0 fail
withholding reverted 22 / 1 "only the conforming proposal is emitted"
reporting reverted (field dropped) 21 / 2 "the withheld proposal is reported, not dropped"
both reverted (origin/main behaviour) 21 / 2
Full suite 412 pass / 0 fail; typecheck clean; ts/dist rebuilt.
fd48869 to
ce2964b
Compare
Two defects from the estate sweep. Separate commits.
1.
ts-ci.yml— the required check's PR filter was narrower than its own push filteron.push.pathsdeclaresontology/**andscripts/**relevant. The PR change-detector matched only^(package(-lock)?\.json|ts/|\.github/workflows/ts-ci\.yml). The derivation those push paths exist for is real:So a re-vendor touching only
ontology/took the "required check trivially satisfied" branch andbuild-and-verify-distwent green having built nothing. It is a required check —branches/main/protectionreturns["build-and-verify-dist","CodeQL"].One correction to the finding, stated because it narrows it: the
kko-provenancejob does cover the first of those three chains — it runs on every PR with no path filter andcheck-kko-provenance.mjsverifies thatts/src/kko-data.tsreproduces from the vendored.n3. But it is not a required check, and nothing coverseffect-requestorsemantic-action. PR #37 touchesontology/effect-request/PROVENANCE.md, so that is live traffic on the uncovered path.The regex moves into a
TS_PATHSenv var, widened withontology/andscripts/. A new step then deriveson.push.pathsfrom the file itself and asserts the detector matches every declared family, so widening one filter and forgetting the other fails loudly rather than silently skipping a build.Red-then-green
Same probe, both trees, reading the regex straight out of whichever
ts-ci.ymlis on disk:origin/maints/src/vendor-graph.tspackage-lock.jsonontology/kko/kko-2.10.n3ontology/effect-request/EffectRequest.jsonontology/semantic-action/SemanticAction.jsonscripts/gen-kko.mjsREADME.mdrust/src/lib.rsAgreement assertion:
2.
vendor-graph.ts— a verdict computed, sealed, and never readRevendorProposal.contractViolationsis populated atproposeRevendorand sealed into the analysis, and its own doc comment has always said "Non-empty means the proposal is NOT emitted." Nothing implemented that sentence.17 occurrences across the repo: 8 in the four compiled
ts/distcopies, 4 invendor-graph.ts(interface field, definition, populate, return), 5 in tests. No production reader — no branch, no.length, no throw, no exit code.analyzeVendorFreshnesssealed every candidate regardless. The siblingdispositionViolationsis a named top-level field, and the validator provably works (theTEETHtest is a genuine counter-test).What was done, and why
The declared rule becomes the actual rule. A candidate with a non-empty
contractViolationsis withheld fromproposals, and its violations are named in a new top-level sealed fieldcontractViolations, formatted${pinKey}: ${violation}exactly asdispositionViolationsis.Both halves, not one. Withholding alone is a silent drop. Naming alone leaves a consumer free to act on a proposal the contract says was never emitted.
Reported, not thrown, for three reasons:
analyzeVendorFreshnessis documented PURE and deterministic; one malformed pin must not destroy the verdicts for every other pin in the run, which is exactly whydispositionViolationsis a list and not an exception; and the module's doctrine is that the engine proposes while a membrane gate outside it decides — throwing would be the engine taking an action.proposeRevendor's signature and behaviour are unchanged, so direct callers see no difference. For a conforming graph the only observable change is the newcontractViolations: [], which does alter the seal hash. No test pins a literal analysis hash.The new test drives the violation from the graph rather than mutating a finished request: the pin declares
policy_labels: ['ops','ops'],proposeRevendorjoins and re-splits it, and the vendored contract'suniqueItemsrejects it — the shape a bad register entry would really take.Red-then-green
ts/src/vendor-graph.test.ts:only the conforming proposal is emittedthe withheld proposal is reported, not droppedorigin/mainbehaviour)only the conforming proposal is emittedFull suite 412 pass / 0 fail,
typecheckclean,check:kkoclean,ts/distrebuilt and committed.Overlap to be aware of before merging
PR #37 also edits
ts/src/vendor-graph.tsandts/src/vendor-graph.test.ts, and itsontology/effect-request/PROVENANCE.mdprose states the current behaviour explicitly:That is the same defect, independently observed and worked around from the other side. Once this lands, that paragraph's premise no longer holds and the sentence should be revisited — I have deliberately not touched that file, since it belongs to #37.
Both #34 and #37 also modify
ts/dist/*, so whichever merges second will need anpm run buildand a re-commit ofts/dist. Unavoidable for a committed build artifact.Rebased on
origin/main@97c14e0immediately before pushing.