Repository navigation
fix(samples): the shipped control pointer hashes the bytes committed beside it - #446
Conversation
…beside it `samples/conductor_agent/expected/current-control.json` bound a `report.json` digest and `size_bytes` that no committed file could ever match, so `read_current_control()` refused Shipgate's own shipped sample with `artifact_mismatch` -- the product rejecting its own evidence set. The cause is an ordering bug in the golden recipe, not in the producer. A regeneration runs `run_scan(...)`, which writes `report.json` carrying the absolute `manifest_dir` and hashes exactly those bytes; only afterwards does the recipe substitute the generating checkout's path with `<REPO>`. The committed digest therefore describes a pre-substitution file that is never committed, and `size_bytes` moves with the contributor's path length -- which is why the value churned across releases without ever being right. `report_markdown` was correct throughout, because nothing rewrites `report.md` after it is hashed. A half-correct artifact is worse than a wholly broken one: the accurate half is what made the broken half easy to trust. The pointer is rebound through the product's own model and `content_id(current_control_identity_payload(...))` rather than hand-edited, because `current_control_id` covers the artifact refs -- a patched digest without a recomputed identity fails `model_validate` outright. Serializing as `_publish` does (`indent=2`, `sort_keys=True`, trailing newline) keeps the diff to the three lines that actually changed. The guard is what keeps the class closed, since nothing read this file before: `test_sample_current_control_pointers_bind_the_committed_artifacts` walks every `samples/*/expected/current-control.json` and confirms each `artifacts.*` entry against the file it names. It hashes with plain `hashlib`, deliberately not `bind_current_control_artifacts` -- an oracle that reuses the producer passes vacuously when the producer is wrong -- and asserts the glob is non-empty, so moving the sample cannot make the invariant silently disappear, which is how it rotted in the first place. Every branch was shown to fail on a perturbation: the pre-fix pointer, drifted artifact bytes, a bound file that is not committed, a hand-edited digest (caught by the model validator), the `report_markdown` half, and an empty glob. The ordering constraint is recorded in `samples/README.md`, since the recipe otherwise lives only in team notes and no script enforces it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
pengfei-threemoonslab
left a comment
There was a problem hiding this comment.
Review outcome: blocking findings (submitted as COMMENT because the connected GitHub identity is the PR author and cannot self-request changes).
The pointer correction itself checks out on an LF checkout: I independently verified the report SHA/size and recomputed identity, read_current_control() succeeds, all 52 targeted report tests pass, all applicable GitHub Actions jobs are green, and the source verifier reports complete / passed / mergeable with no findings.
The remaining issues are in making that raw-byte invariant portable and keeping the new regression guard non-vacuous and aligned with the production reader. Details and reproductions are inline.
| f"{path} binds {key!r} to {ref.path}, which is not committed " | ||
| f"beside it." | ||
| ) | ||
| data = bound.read_bytes() |
There was a problem hiding this comment.
[P2] Pin hash-bound fixture artifacts to LF
This hashes working-tree bytes, but Git is free to rewrite these tracked text files on checkout because git check-attr text eol reports both report.json and report.md as unspecified. I reproduced this at 3b44c462 with core.autocrlf=true: report.json became 52,236 bytes / sha256:9ddd4fc0…c763 instead of the pointer's 50,809 / sha256:cd87ed0b…d024; report.md changed too. The new test fails, and read_current_control(samples/conductor_agent/expected) raises artifact_mismatch. Please pin the hash-bound sample artifacts to LF in .gitattributes. Normalizing only inside this test would leave the actual reader broken.
There was a problem hiding this comment.
Confirmed and fixed in acab0ca. Reproduced your scenario exactly: cloning this branch with core.autocrlf=true gave a 52,236-byte report.json hashing 9ddd4fc0…c763, your numbers to the byte, and report.md moved too. The repo had no .gitattributes at all.
Added one pinning samples/**/expected/** -text, scoped to the sample goldens rather than repo-wide. I took your point that normalizing inside the test would be the wrong fix — the reader hashes real bytes, so that hides the breakage instead of removing it.
Verified by re-cloning the committed branch with core.autocrlf=true: report.json now checks out at 50,809 / cd87ed0b…d024, the guard passes, and read_current_control() returns cleanly in that CRLF checkout. The committed blobs contain no CR bytes, so -text rewrites nothing.
Worth flagging for anyone on Windows with an existing clone: -text takes effect on re-checkout, so a working tree that already has CRLF copies needs git checkout -- samples/ (or git add --renormalize .) once.
The digest-mismatch message now also names line-ending translation as a cause, since that is the form this will take when someone hits it.
| pointer = CurrentControlPointer.model_validate( | ||
| json.loads(path.read_text(encoding="utf-8")) | ||
| ) | ||
| for key, ref in sorted(pointer.artifacts.items()): |
There was a problem hiding this comment.
[P2] Assert the fixture still binds the required artifacts
This loop is vacuous when pointer.artifacts is empty. That state is schema-valid for a scan pointer: I removed the refs, recomputed current_control_id, and CurrentControlPointer.model_validate accepted it, after which this test performed zero checks. That permits the exact alternative described in the PR as unacceptable—dropping the report binding—while the new guard remains green. Please assert the expected keys (currently report and report_markdown, or a deliberate per-fixture contract) before iterating.
There was a problem hiding this comment.
Confirmed and fixed in acab0ca. Reproduced it: stripped the refs, recomputed current_control_id, and CurrentControlPointer.model_validate accepted the result — the carve-out is explicit, a terminal pointer must bind at least one artifact is only raised when operation != "scan". The loop then ran zero times and the guard stayed green.
This one stung, because it is the same vacuity class I claimed to have closed with the non-empty-glob assertion. I closed it one level up and left it open one level down, which is exactly how the original drift went unnoticed.
Took the per-fixture contract option: EXPECTED_CURRENT_CONTROL_ARTIFACTS maps each sample to the keys it binds (conductor_agent → report, report_markdown). Both directions now fail — dropping a binding, and adding a fixture the map does not declare, so a future sample cannot arrive with nothing asserted about it.
| json.loads(path.read_text(encoding="utf-8")) | ||
| ) | ||
| for key, ref in sorted(pointer.artifacts.items()): | ||
| bound = path.parent / ref.path |
There was a problem hiding this comment.
[P2] Exercise the production reader's safe-file rules
is_file() and read_bytes() follow symlinks, while read_current_control() deliberately rejects symlinked artifacts and other unsafe paths. I moved the tracked report.md bytes beside the fixture and replaced expected/report.md with a relative symlink: this test passed against the same hash, but read_current_control(expected) failed with artifact_unreadable. Add a reader-level assertion (or equivalent independent no-symlink/size checks) so the sample cannot be green here while the product still refuses it.
There was a problem hiding this comment.
Confirmed and fixed in acab0ca. Reproduced it: replaced report.md with a relative symlink to its own bytes, the guard passed on the same digest, and read_current_control() failed with artifact_unreadable. A test that goes green while the product refuses the sample is precisely the failure this PR claims to close.
Did both things you offered rather than choosing:
- An explicit
not bound.is_symlink()assertion, so the failure names the file and the rule instead of surfacing as a reader error. read_current_control(path.parent)over each fixture, wrapped so the message distinguishes "digests match but the reader still refuses" from a digest mismatch.
The reader call is not redundant with the local checks, and I checked rather than assumed: with the symlink assertion added first, I needed a case only the reader catches. A symlinked current-control.json is one — every digest matches, neither local check looks at the pointer file itself, and the reader refuses it as unsafe_pointer. That is in the sweep now, so the reader assertion is demonstrably load-bearing rather than decorative.
Full sweep re-run, each perturbation failing on its own assertion: all three of your findings, plus drifted bytes, a missing bound file, a hand-edited digest (caught by the model validator), a symlinked pointer, and an empty glob.
…d removal Addresses all three review comments on #446. Each was reproduced at 3b44c46 before being fixed, and each is a way the new guard could stay green while `read_current_control()` refuses the shipped sample -- the exact failure the branch exists to close, left open one level down. [P2] Nothing pinned the line endings of files whose digests are recorded. `git check-attr text eol` reported both `report.json` and `report.md` as unspecified, and the repository had no `.gitattributes` at all. Cloning this branch with `core.autocrlf=true` -- the Git for Windows default, and this repository ships a `windows-launcher` job -- yields a 52,236-byte `report.json` hashing `9ddd4fc0...c763` against a pointer that records 50,809 and `cd87ed0b...d024`; `report.md` moves too. The guard fails and the reader raises `artifact_mismatch`, so the previous commit would have handed Windows contributors a red suite. Normalizing inside the test was rejected: the reader hashes real bytes, so that would hide the breakage rather than fix it. `samples/**/expected/** -text` makes Git hand these files over exactly as committed. The committed blobs contain no CR bytes, so nothing is rewritten. [P2] The per-artifact loop was vacuous on an empty `artifacts` map, and that map is schema-valid for a `scan` pointer -- `CurrentControlPointer` requires a binding only for `verify` and `preview`. Stripping the refs and recomputing `current_control_id` produced a pointer that validated and left the guard with nothing to assert, permitting exactly the alternative the PR argued against. `EXPECTED_CURRENT_CONTROL_ARTIFACTS` now names each fixture's keys, so both dropping a binding and adding an undeclared fixture are failures. [P2] `is_file()` and `read_bytes()` follow symlinks; the reader refuses them. Replacing `report.md` with a relative symlink to its own bytes passed the guard while `read_current_control()` failed with `artifact_unreadable`. The test now rejects a symlinked artifact directly, for a message naming the file, and then runs the reader over the fixture -- the end-to-end property the digests are only a proxy for. The reader call is not redundant with the local checks: a symlinked `current-control.json` satisfies every digest, is caught by neither, and is refused by the reader as `unsafe_pointer`. Verified reachable that way. Perturbation sweep re-run, each failing on its own assertion: all three findings above, plus drifted bytes, a missing bound file, a hand-edited digest (caught by the model validator), a symlinked pointer, and an empty glob. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Pushed 3935060. All three P2 comments are addressed in acab0ca, each reproduced at 3b44c46 first and each answered in its own thread. The common thread is worth naming: all three were ways the new guard could stay green while Also merged Verification on the merged head: full suite green ( Still not done, and still worth a follow-up rather than this PR: nothing regenerates these goldens mechanically. A |
Summary
samples/conductor_agent/expected/current-control.jsonbound areport.jsondigest andsize_bytesthat no committed file could ever match, soread_current_control()refused Shipgate's own shipped sample withartifact_mismatch— the product rejecting its own evidence set. Reproduced onmainbefore fixing.run_scan(...), which writesreport.jsoncarrying the absolutemanifest_dirand hashes exactly those bytes; only afterwards does the recipe substitute the generating checkout's path with<REPO>. The committed digest therefore describes a pre-substitution file that is never committed, andsize_bytesmoves with the contributor's path length — which is why the value churned across releases (50873→50876) without ever being right.report_markdownwas correct throughout, because nothing rewritesreport.mdafter it is hashed. A half-correct artifact is worse than a wholly broken one: the accurate half is what made the broken half easy to trust.Type
src/change)What changed
samples/conductor_agent/expected/current-control.json— reboundartifacts.reportto the committed bytes (50876/6174baab…→50809/cd87ed0b…). Rebuilt through the product's own model andcontent_id(current_control_identity_payload(...))rather than hand-edited, becausecurrent_control_idcovers the artifact refs: a patched digest without a recomputed identity failsmodel_validateoutright. Serializing as_publishdoes (indent=2,sort_keys=True, trailing newline) keeps the diff to the three lines that actually changed.tests/test_reports.py—test_sample_current_control_pointers_bind_the_committed_artifactswalks everysamples/*/expected/current-control.jsonand confirms eachartifacts.*entry against the file it names.samples/README.md— records the ordering constraint, since the recipe otherwise lives only in team notes and no script enforces it.Notes for the reviewer
Two deliberate choices in the test, both aimed at the failure mode that let this rot:
hashlib, notbind_current_control_artifacts. An oracle that reuses the producer passes vacuously when the producer is wrong.Every branch was shown to fail on a perturbation rather than assumed reachable: the pre-fix pointer, drifted artifact bytes, a bound file that is not committed, a hand-edited digest (caught by the model validator, not the digest assert), the
report_markdownhalf, and an empty glob.Two options were considered and not taken. Dropping the
artifacts.reportentry would make the sample unrepresentative of whatscanactually publishes, since the pointer is the only place a scan's binding is demonstrated. Ascripts/regenerate_samples.py --check, following thegenerate_schemas.pyprecedent, would close this more firmly than a documented recipe — worth a follow-up if the notes drift again, but it is new contributor tooling beyond the scope of this fix.supersedesis deliberately untouched: it is excluded from the identity hash and records real generation lineage.This branch was rebased onto 4edd70a (#438), which regenerated
report.mdand republished this pointer; the pointer was re-derived against those goldens rather than carried over.report.jsonitself is byte-identical across that commit.Verification
CI is authoritative for
python -m ruff check .,python -m compileall -q src tests, andpython -m pytest.Additional local checks run (on the rebased head):
pytest -n auto -m "not perf" --ignore=tests/test_adapter_static_only.py— greentests/test_latency_budget.py -m perfandtests/test_adapter_static_only.pyseparately — greenpython -m ruff check .andpython -m compileall -q src tests— greenread_current_control(Path("samples/conductor_agent/expected"))now returns cleanly; it raisedartifact_mismatchbefore the fixRelease-readiness notes
docs/checks.md(n/a — no check IDs change)STABILITY.md(n/a — no schema moves; a sample artifact is corrected to match its existing schema)🤖 Generated with Claude Code