Repository navigation
docs: the SDK's CLI compatibility job is opt-in; smoke goldens stop pinning the tool version - #491
Conversation
bomly-dev/bomly-sdk#107 runs it only with the cli-compat label. Co-Authored-By: Claude Opus 5.5 <[email protected]>
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughAGENTS.md now documents that local SDK commands use the checked-out SDK for builds, tests, and plugin fixture compilation. It also states that the CI CLI compatibility job runs when an SDK pull request has the ChangesSDK and CLI compatibility instructions
Priority: ⬇️ Low Estimated code review effort: 1 (Trivial) | ~3 minutes Change: Other Merge Risk: ⚪ Minimal · up to The documented local commands and label-gated compatibility job match the inspected behavior; no actionable merge risk remains. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Bomly Diff SummaryCompared Overview
Dependency Changes✅ No dependency changes. Vulnerabilities✅ No vulnerability changes. License Changes✅ No license changes. Project Posture✅ No project posture changes ( Policy Findings✅ No policy differences were identified. |
run.tool.version is stamped from the latest release tag, so the v0.27.0 release made every scan golden that pinned 0.26.0 fail. The normalizer now treats it like the run's id and timestamps, and the 48 goldens carry the placeholder instead. Co-Authored-By: Claude Opus 5.5 <[email protected]>
The catalog stored a SHA-256 for every fixture and expected-result file a claim names. Those files are regenerated routinely, so every golden update left the catalog stale, and the `catalog-valid` gate then refused the next release until someone rewrote the checksums. The previous commit on this branch did that rewrite. This one removes the reason for it. A file in this repository is already content-addressed by git: a release tag and a path identify exact bytes, and the report records the commit. The stored checksum was a second, hand-kept copy of what git knows. It could not catch anything git does not, and the only thing it could do was fall behind. It could also not be kept in step by checking earlier. `make test` already verified the checksums on every pull request. #491 changed goldens and #398 added the catalog; each passed CI on its own, they touched different files so they merged cleanly, and `main` failed the gate. A refresh step in `Update Smoke Goldens` only helps when that workflow is what changes a golden. So the catalog names files by path only, `catalog-validate --refresh` and its step in the goldens workflow are gone, and that workflow no longer needs Go. What is checked is what can actually go wrong: a claim naming a file that was renamed or deleted fails `make assurance-catalog` and `TestRepositoryCatalogIsValid` in `make test`, and both refuse a run that checked no files at all (ADR-0044). Still pinned, because this repository's history cannot address them: a git input by revision and a container by digest. The catalog and report keep their schema versions. No report has been published, the site does not read the removed field, and the catalog has no reader outside this repository. ADR-0047 carries a dated amendment. Co-Authored-By: Claude Fable 5.1 <[email protected]>
…sum (#492) * fix(assurance): refresh the catalog checksums #491 invalidated #491 rewrote smoke goldens so they stop pinning the tool version, and landed on main between the last sync of #398 and its merge. The two touched different files, so they merged cleanly, but the catalog's claims pin the checksum of the golden that backs each of them, and fifteen of those now named a file that had changed underneath them. The first `Release prerequisites` run on main caught it: `catalog-valid` failed on `finding-baseline-workflow.golden.json`. That is the gate working as designed, and it would have refused the next tag. Refreshed with `catalog-validate --refresh`; no claim changed, only the recorded checksums. Co-Authored-By: Claude Fable 5.1 <[email protected]> * fix(assurance): a claim names its files by path, with no stored checksum The catalog stored a SHA-256 for every fixture and expected-result file a claim names. Those files are regenerated routinely, so every golden update left the catalog stale, and the `catalog-valid` gate then refused the next release until someone rewrote the checksums. The previous commit on this branch did that rewrite. This one removes the reason for it. A file in this repository is already content-addressed by git: a release tag and a path identify exact bytes, and the report records the commit. The stored checksum was a second, hand-kept copy of what git knows. It could not catch anything git does not, and the only thing it could do was fall behind. It could also not be kept in step by checking earlier. `make test` already verified the checksums on every pull request. #491 changed goldens and #398 added the catalog; each passed CI on its own, they touched different files so they merged cleanly, and `main` failed the gate. A refresh step in `Update Smoke Goldens` only helps when that workflow is what changes a golden. So the catalog names files by path only, `catalog-validate --refresh` and its step in the goldens workflow are gone, and that workflow no longer needs Go. What is checked is what can actually go wrong: a claim naming a file that was renamed or deleted fails `make assurance-catalog` and `TestRepositoryCatalogIsValid` in `make test`, and both refuse a run that checked no files at all (ADR-0044). Still pinned, because this repository's history cannot address them: a git input by revision and a container by digest. The catalog and report keep their schema versions. No report has been published, the site does not read the removed field, and the catalog has no reader outside this repository. ADR-0047 carries a dated amendment. Co-Authored-By: Claude Fable 5.1 <[email protected]> * fix(assurance): report goldens are checked out byte for byte The first prerequisites run failed TestReportGoldens on Windows only. The goldens are compared byte for byte, and a Windows checkout rewrites text files to CRLF; converting a golden to CRLF locally reproduces the same "golden differs" failure. The assurance testdata is marked -text so it is checked out exactly as committed on every platform. Scoped to this directory on purpose: the rest of the repository's testdata has passed on Windows as it is, and changing how it is checked out is not this change's business. Co-Authored-By: Claude Fable 5.1 <[email protected]> * fix: three failures the unit suite had only on Windows Nothing ran the unit suite on Windows except the portable stability workflow, which was manual and last ran in July. Making it a release gate ran it again, and three tests had gone red there in the meantime. - SwiftPM published its evidence patterns backslash-separated on Windows. The candidate list was built with filepath.Join and is used both to read files and as the detector's evidence patterns, so the support matrix and `bomly plugin list` said different things depending on the machine. The list is slash-separated now; every read already joins an entry onto a directory with filepath.Join, which converts the separators for the host. - The SBOM detector test looked for a file path inside an error that quotes it. Quoting doubles a Windows path's backslashes, so the bare path was not a substring there. It looks for the quoted path. - The file-cache contract test asserted Unix permission bits. Windows has none: Go reports 0777 for every directory whatever mode it was created with. The mode assertions are Unix-only; the containment half of the contract is still checked everywhere. Co-Authored-By: Claude Fable 5.1 <[email protected]> * perf(assurance): the release stage fuzzes four targets at a time Thirty fuzz targets ran one after another at 45 seconds each, plus a build and a baseline pass per target. That is over half an hour, and it made fuzzing the slowest job in the release prerequisites stage by a wide margin. `scripts/run-fuzz.sh` takes FUZZ_JOBS and runs that many targets at once. Each target is its own `go test` process, so they parallelize cleanly: workers are shared out evenly instead of every fuzzer claiming every CPU, each target's output is printed whole rather than interleaved, and every target is still recorded. The default stays one at a time, so `make fuzz` and the nightly run are unchanged. The prerequisites stage asks for four at a time and thirty seconds each. Run locally with a three-second budget, all thirty targets finish in 77 seconds. A failing target still fails the run and is recorded in both modes. Co-Authored-By: Claude Fable 5.1 <[email protected]> * docs(assurance): say when a schema version starts to bind, and guard it Review pointed out that removing the per-file checksum from report claims without raising the report's schema version contradicts the rule this framework wrote down for itself: only an optional addition may keep a version. The rule was stated without its starting point. A version names a shape someone already holds, and for the report that begins with the first report committed to docs/assurance/reports -- from then on bomly.dev has mirrored a document of that shape. No report has been published, the site never read the removed field, and its sync does not validate it, so there was no reader to break. Raising the version would have made the first report ever published a "v2" beside a "v1" that never existed. The developer guide now says that in so many words, and the ADR amendment records it. More usefully, TestPublishedReportsParse makes it mechanical: every committed report is read with the current parser, which rejects unknown fields, so once a report exists a change that strands it fails in `make test`. Checked by mutation -- a report in the current shape passes, one carrying the removed field fails, and an index naming a missing report fails. Also marks the sentence in ADR-0047 that the amendment supersedes, so a reader does not have to reconcile the two. Co-Authored-By: Claude Fable 5.1 <[email protected]> * test(assurance): published reports must round-trip byte for byte The guard added in the previous commit had two holes, both found in review. It parsed only the reports the index lists, so a report file the index did not mention was never read. And it relied on the strict parser alone, which rejects a removed or renamed field but not a newly required one: such a field decodes silently as its zero value, so an incompatible addition under the same schema version passed. Both close on one property. Every file in docs/assurance/reports must parse under the current schema and encode back to exactly the bytes it was read from. A removal or rename fails the parser. A newly required field appears in the re-encoding and the bytes differ. An added optional field is omitted when empty, so it passes -- which is precisely the one change allowed to keep a version. The check asks the encoder instead of listing fields, so it cannot fall behind the struct. The directory and the index must also agree in both directions. The same property is asserted on the fixture reports, so it is exercised before the first report is published. Checked by mutation: a current-shape report passes; a field added without omitempty fails; the same field with omitempty passes; an unlisted report fails, and is still read when its shape is broken; a listed report that is missing fails. Co-Authored-By: Claude Fable 5.1 <[email protected]> * test(assurance): remove the published-report shape guard This pull request fixes a stale-checksum failure. In its first review round a reviewer noted that dropping a report field without raising the schema version contradicted the written rule, so a guard was added to hold that rule mechanically. Every round since has been about the guard: - round two: it skipped report files the index did not list, and it let a newly required field through, because such a field decodes as a zero value; - round three: it did not cover the index, and its byte comparison would fail on a Windows checkout, which rewrites line endings. The last one is the reason to stop. That failure would land in the portable suite, which gates a release, as soon as the first real report was committed. A guard that byte-compares published data had become a release risk in its own right, and it was being refined in place of the change it was attached to. So it is removed, and the reasoning is recorded where the next person will look: beside TestSchemaVersionsArePinned and in the developer guide. The rule itself is unchanged and is still stated there, including when a version starts to bind. It is held by the pinned-version test, by the report goldens, where any change of shape shows up as a diff in review, and by the reviewer. A mechanical guard, if wanted, is its own change, and should compare the report's JSON schema against the one recorded at the first published release rather than committed bytes. Co-Authored-By: Claude Fable 5.1 <[email protected]> --------- Co-authored-by: Claude Fable 5.1 <[email protected]>
Follows bomly-dev/bomly-sdk#107.
CLI compatibilityjob run only on SDK pull requests labelledcli-compat. The full CLI suite took 435s, against 26s for the SDK's own tests. This PR corrects the sentence inAGENTS.mdthat said the job runs on every SDK pull request.run.tool.versionat0.26.0, and the build now stamps0.27.0.run.tool.versionlike the run's id and timestamps, so a release can no longer break the goldens. The 48 affected goldens carry the placeholder.🤖 Generated with Claude Code