A pull request may arrive without the artifact (#719, T-1503) - #764
Merged
Conversation
… costs T-1503, now that T-1502 has been proven on a real merge: #761 carried source only, `canonical-merge` rebuilt it, and `e4e5154` landed with `artifact:verify` and `git diff -- dist/` both clean and nobody having built anything by hand. "Drop the diff line" was wrong, and the run that showed it is on #761. CI builds before it verifies, so after `build:canonical` the tree holds a bundle made from the new source while the committed manifest still describes the old one. All three checksum comparisons fail together and the diff is never reached -- which is exactly how #720 failed. So the verifier gets a second mode instead. `--contract-only` keeps what the manifest *declares* and drops what it *records*. The platform, the image, the build command, the runtime asset list and the source input list do not move when source moves, so a pull request has no honest reason to touch them and this mode still refuses one that does. It refuses to combine with `RELEASE_COMMIT` as well: a release is the one place the recorded checksums have to be the reason it is allowed. **What is given up.** On the source-only path nothing checks that the committed manifest tells the truth about the committed bundle. That is not theoretical -- it is the property #763 was built to falsify, and it did: a hand-edited `dist/commitlore.mjs` with `src/` untouched passed `artifact:verify` completely and was caught only by the rebuild-and-diff after it. That property now lives on the push and tag paths, and in `canonical-merge.yml`, which regenerates the manifest rather than trusting one. The relaxation is conditional rather than blanket. A pull request that does carry `dist/` or the manifest is still checked in full, so the old path stays available as a fallback instead of being replaced. The decision is read from the commit range before the build runs, because afterwards the tree describes what this job just built. Limit: the mode is chosen from the pull request's changed paths, so a pull request that carries the artifact unchanged from base is checked in full and one that carries none is not checked against a committed bundle at all Blast: module Undo: easy Certainty: firm Record-Id: r-t1503mode Provenance: authored Verified: reverting the second mode fails all three new tests and restoring it passes seven; the tampered-contract test was first passing in both directions and was strengthened until it could tell them apart; exit codes read directly rather than through a pipe -- 1 for a tampered contract, 2 for RELEASE_COMMIT, 0 for source-only CommitLore-Version: 2.0.0
CommitLore — record lintTrailers: clean — 1 commit in Trailer violations fail this check. Active constraints are informational — they are what the repository already decided, not a verdict on this PR. |
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.
T-1502 is proven on a real merge — #761 carried source only,
canonical-mergerebuilt it, ande4e5154landed withartifact:verifyandgit diff -- dist/both clean and nobody having built anything by hand. That was T-1503's stated dependency, so this is the next step."Drop the diff line" was wrong, and #761 is the run that shows it
CI builds before it verifies. After
build:canonicalthe tree holds a bundle made from the new source while the committed manifest still describes the old one, so on a source-only pull request all three checksum comparisons fail together andgit diffis never reached:That is exactly how #720 failed. Removing the diff line would not have moved it.
The second mode
--contract-onlykeeps what the manifest declares and drops what it records. Platform, image, build command, runtime asset list, source input list — none of those move when source moves, so a pull request has no honest reason to touch them, and this mode still refuses one that does.Measured by exit code, read directly rather than through a pipe:
--contract-onlyRELEASE_COMMITsetThe success line is worded differently on purpose —
canonical artifact contract intact (checksums not compared)— so a reader scanning a log forcanonical artifact verifiedcannot find it on a run that never compared them.What this gives up
On the source-only path, nothing checks that the committed manifest tells the truth about the committed bundle.
That is not theoretical. It is the property #763 was built to falsify, and it did: a hand-edited
dist/commitlore.mjswithsrc/untouched passedartifact:verifycompletely and was caught only by the rebuild-and-diff after it (the run).That property now lives on the push and tag paths, and in
canonical-merge.yml, which regenerates the manifest rather than trusting one.The relaxation is conditional, not blanket
A pull request that does carry
dist/or the manifest is checked exactly as before. The old path stays available as a fallback rather than being replaced. The decision is read from the commit range before the build runs, because afterwards the tree describes what the job just built rather than what the author pushed.Reproducibility — two canonical builds compared byte for byte — runs in both modes. It asks whether the builder is deterministic, which has nothing to do with what the author pushed.
Verified
Reverting the second mode fails all three new tests; restoring it passes seven.
The tampered-contract test initially passed in both directions — with the mode and without it — because editing only the manifest leaves strict and relaxed reporting the same line. It was strengthened by moving source as well, so the two modes have something to disagree about, and it now fails on the negative control like the other two.
EXPECTED_CI_WORKFLOW_SHA256re-locked to24f749bb…;test/release-publish-prerequisites.test.tspasses 29.Not in this pull request
T-1503's acceptance — a pull request touching
src/with nodist/change is green — needs a pull request that touchessrc/. This one does not, so it will be observed on the next one after this lands.