Repository navigation
Conversation
4 of 5 tasks
Member
Please do not feel bound to follow the pattern of the current - name: Build main module
run: python .ci/cue.py build
- name: analyze NEW
run: abicheck ... > NEW
- name: clean
run: python .ci/cue.py build distclean
- name: roll-back
run: |
# find previous tag,
NEW="$(git describe --tags)"
OLD="$(git describe --tags --abbrev=0 "$NEW")"
[ "$OLD" = "$NEW" ] && OLD="$(git describe --tags --abbrev=0 "$NEW"~)"
git reset --hard "$OLD"
git log -n1
- name: Build main module
run: python .ci/cue.py build
- name: analyze OLD
run: abicheck ... > OLD
- name: abicheck
run: abicheck OLD NEW |
Member
|
A The only header associated with |
Member
|
I think it is also reasonable to assume the location of the special epics-base dependency is eg. |
Member
|
@napetrov Thank you for following up with this PR. Adding a GHA job with your |
Replace the 232-line .ci-local/abicheck-diff.sh wrapper and its standalone workflow with an integration that consumes the build PVXS already performs. Build reuse The capture now runs inside the existing "Native Linux (WError)" matrix leg, marked with `abicheck: 1`, after that leg's own tests. It reads the installed include/pvxs headers and the installed shared objects; it no longer rebuilds both revisions under Bear, copies headers out of src/ and ioc/, parses Makefiles, rewrites configure/*.local, or moves HOME. Each component is captured exactly once per revision and that capture is reused for every comparison, the job summary and the PR comment. Scope Evidence depth drops from L3 to L2 (--depth headers). Build-drift coverage is intentionally removed, not silently retained; this is stated in documentation/abicheck.md rather than implied. L4/L5 stay off. Ownership libpvxs owns every installed public header except iochooks.h; libpvxsIoc owns iochooks.h alone, with the core and EPICS headers as include-only context. The two shared objects are resolved explicitly and validated, so symlinks are not analysed as duplicate components and static archives are never eligible. Baselines accepted-main resolves the default-branch run for the pull request's exact base commit and rejects a mismatch; release-contract resolves a published release asset. Both are labelled separately and never merged. A missing or incompatible baseline is an explicit incomplete outcome. Publication The analysis stays unprivileged (contents: read, no secrets), and a separate default-branch workflow_run publisher with only actions: read and pull-requests: write verifies the producer run, resolves the pull request through the API, distinguishes the PR head SHA from the built merge commit, and renders the canonical report without running any analysis. Removed The 120-minute timeout and its ~100-minute rationale, the mandatory Bear capture, the projected compile databases, the hand-rolled non-atomic summary.json, the hard-coded 0/2/4 exit-code acceptance, and the practice of concatenating full review reports into $GITHUB_STEP_SUMMARY. The abicheck extraction config keeps its -std=c++17 override, now with the measured reason: C++11 and C++14 both fail to parse libstdc++ 13 under the supported CastXML 0.7.0. Two abicheck product bugs found while validating this are documented as known issues; they are owed by abicheck and are not papered over here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BFumn6v9CSsM3euZ8ekcqy
The first real run of the comparison job failed at "Locate candidate snapshots" with "manifest entry for libpvxs escapes the baseline-set". A baseline-set manifest records two different paths per library, and this script read the wrong one. "artifact" echoes the input binary the producing job dumped -- an absolute path in that job's workspace, which is meaningless once the set has travelled as an artifact. "snapshot" is the snapshot file's own name inside the set. Joining an absolute "artifact" against the download directory yielded a path outside it, which the containment guard then correctly rejected. Read "snapshot" instead, keep the containment guard (it is what caught this), and say "snapshot path ... escapes" so the message names the field. Content identity is deliberately not re-verified here: the manifest's per-artifact sha256 is abicheck's normalised content hash, not a whole-file digest, and reimplementing that recipe would be a second, silently divergent verifier. resolve-baseline, which check-target already invokes, is what validates baseline-set identity. Reproduced and fixed against a real baseline-set built by actions/baseline's own build_manifest.py from this branch's captured snapshots, then moved to a different directory to model the cross-job handoff. Negative controls: path traversal, absolute path, a symlink leaving the set, a missing snapshot file, a component absent from the manifest, an absent manifest, and a malformed manifest are each refused with a distinct message; the valid set resolves both components. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BFumn6v9CSsM3euZ8ekcqy
…ract Addresses the review of f1cc10a. Every finding below was reproduced locally before it was changed. Report matching was silently losing every finding abicheck aggregate matches a report to its expected target by the report's own target_id, falling back to the file stem after the "abi-report-" prefix. abicheck-collect.sh sanitised '@', '#' and '~' out of the check id when naming the file, so the stem could never match and every report aggregated as an unavailable target. Check ids are now written verbatim, with path separators refused. A local run over a real compare report went from 0/2 to 1/2 targets analyzed. Accepted-main baselines were not required to be trustworthy The lookup selected any completed push run at the base SHA, from any branch, regardless of conclusion -- and the capture step deliberately runs after a test failure, so a failed run's snapshot could become the baseline. It now requires this repository's own ci-scripts-build.yml, a push to the pull request's base branch, and conclusion success. "Not found", "lookup failed" and "artifact expired" are distinct outcomes. The publisher conflated the PR head with the tested commit workflow_run.head_sha is the pull request head, not the merge commit the producer built, and it was being emitted as build-sha. The hand-rolled resolution is replaced by abicheck's actions/verify-source-run, which returns pr-head-sha and tested-sha as separate verified coordinates and applies the artifact size/entry/ratio caps. Publication is bound to the triggering run attempt, and concurrency is keyed per pull request and profile rather than per producer run. Producer and publisher disagreed on failure A missing candidate wrote abicheck-incomplete.json and skipped aggregation, while the publisher read aggregate.json. There is now one shape: the expected checks are always declared and aggregated, so an incomplete analysis is an aggregate whose targets are unavailable. Only the checks an event actually has are declared -- accepted-main exists on a pull request, not on a push or tag build. Aggregate validation was a key-presence check {"aggregate_schema_version": "nonsense"} passed it. Replaced with a structural check of status, coverage (against abicheck's own CoverageStatus values), gate and per-target states, and the aggregate exit code is recorded rather than discarded. Release publication could not work as documented The automatic trigger required a "v" prefix; PVXS tags are bare versions such as 1.5.2, so it excluded every real release. Tag identity is now proved through refs/tags/<name> with annotated-tag resolution, and must point at the built revision. The documented bootstrap was circular -- the workflow at a historical tag has no capture step -- so it now builds the requested revision once in the trusted workflow and captures it with the same shared action the matrix leg uses. --clobber is gone: an existing asset is left in place unless replacement is explicitly requested. Profile and component coverage are checked before upload, since stage-baseline does not check them. Duplicate manifest components were last-one-wins; now refused. Shared .github/actions/abicheck-capture removes the duplication between the matrix capture and the bootstrap build. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BFumn6v9CSsM3euZ8ekcqy
The capture-failure steps referenced steps.abi-inputs, which stopped existing when the resolver moved into the shared capture action -- the reference rendered as an empty string in the marker it wrote. Rather than repair it, remove it. The artifact it produced was never consumed by anything, and it was a second way of saying "the analysis did not complete" alongside the aggregate-of-unavailable-targets shape this branch just made canonical. A one-line warning replaces it; the comparison job still reports the incomplete outcome. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BFumn6v9CSsM3euZ8ekcqy
Two review findings, both confirmed against the code. gh release upload refuses an asset name that already exists unless --clobber is passed. The overwrite branch logged its warning and then fell through to an upload without it, so "overwrite: true" failed the job instead of replacing the asset -- the bootstrap dispatch input could not do what it offered. --clobber is now passed on that branch only; the default path still leaves a published baseline untouched, which was the point of removing the blanket --clobber in the first place. abicheck-inputs.sh pointed readers at .github/workflows/abicheck-analyse.yml for the ownership rules. That file never existed on this branch -- it was a name from an earlier draft -- so the reference now points at documentation/abicheck.md, which is where those rules actually live. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BFumn6v9CSsM3euZ8ekcqy
Rendering a real aggregate through the publisher's own renderer, with no
credentials, surfaced two integration bugs that CI would have shipped.
The expected-target manifest was written into the report directory that
`abicheck aggregate` scans, so the manifest was itself picked up as a
report. A two-component run declared "3 targets", one of them
`expected-targets`, reported as outside the expected set. It is now
written outside that directory.
Aggregation ran with an absolute report-directory argument, so each
target's recorded `report_path` was absolute. The publisher reads member
reports only from the aggregate document's own directory and refuses an
absolute path -- so every per-target detail was dropped and replaced by a
limitation row, which reads exactly like a finding-free result.
Aggregation now runs from inside the report directory, making each
`report_path` a bare filename beside `aggregate.json`.
Verified end to end in the CI shape: 2 targets instead of 3, no
absolute-path refusals, and the analysed component's addition now counted
("2 safe" where it was "1") instead of being silently lost.
The publisher is pinned to abicheck 9bc92e635fbfae3252189d3bd09059cc28599490,
the head of abicheck PR #1311, replacing the earlier branch-tip pin. The
caller's inputs and outputs were re-validated against that revision's
declared schemas. The pin is immutable but the PR is not merged and its
own CI had not finished, which documentation/abicheck.md now states.
That revision also carries the fix for the persistent-hygiene bug reported
from this branch: the rendered comment separates "339 pre-existing
cross-source hygiene findings present on both sides -- not introduced by
this change" from what the change actually introduced.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BFumn6v9CSsM3euZ8ekcqy
0beca2b set the abicheck job's EXPECTED_TARGETS from ${{ runner.temp }} in the job's `env:` block. The `runner` context is not available there -- only github, needs, strategy, matrix, vars, inputs and secrets are -- so the whole workflow file became invalid. GitHub failed the run instantly with no jobs at all, listing it by path rather than by its `name:`, which is what that failure looks like. The manifest path moves to ${{ github.workspace }}, which is valid in a job env block and is still outside REPORT_DIR, so `abicheck aggregate` still does not pick the manifest up as an extra target. YAML well-formedness was never the issue and parsing the file locally never would have caught this; the check that does is whether each expression's context is permitted at the level it appears. Verified by scanning every job's env/if/concurrency for `runner.` before pushing, and by re-running the collect/aggregate/validate chain in the corrected layout: 2 targets, relative report_path, artifact carrying only the reports and the aggregate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BFumn6v9CSsM3euZ8ekcqy
Two findings from re-checking the publisher dependency. actions/report grew source-run-id and source-run-attempt, and this caller was not passing them. Left unset they default to the PUBLISHER's own run id, which orders by when publication was triggered rather than when the analysis ran -- so a late re-run of an older commit would look newer than the current result and overwrite it. That is the precise inversion the ordering guard exists to prevent, and it is the lifecycle property this integration claims to have. Both are now passed from the workflow_run event. The pin moves from 9bc92e6 to 2324460, six commits later on the same unmerged PR. Those commits fix defects in the parts this integration depends on: the PR-head/merge-commit association check, what a refused artifact leaves behind, member-report work bounds, shell-value handling, and an eleven-defect review round. Remaining on the older revision would have meant pinning to known-defective run-selection code -- the security boundary of this whole design. Still an unmerged revision, which documentation/abicheck.md continues to say. The caller's inputs and outputs re-validate against the new revision's schemas, and the render path still produces the same comment from a real aggregate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BFumn6v9CSsM3euZ8ekcqy
abicheck PR #1311 merged as bc2ee0c, so the temporary pin on an unmerged revision is gone. Every Action and the analysis package now name one commit instead of two, which also removes the split where the publisher and the analysis ran different abicheck revisions. The move was checked rather than assumed, in two parts. The five Action definitions this caller uses -- report, verify-source-run, baseline, check-target, stage-baseline -- are byte-identical between the previously pinned revisions and bc2ee0c, so no interface changed. The analysis package is not identical: bc2ee0c carries four later fixes, among them sided --header/--include resolution and the demotion of binary churn on exports no public header declares. Both components were therefore re-captured at --depth headers on bc2ee0c and re-aggregated -- schema 1.11, status pass, coverage complete, 2/2 analyzed -- and the PR comment was rendered from that real aggregate. The Known issues section is re-measured rather than carried forward. The merged revision does bound the damage: an unchanged pair now folds to zero gating findings and the comment labels the 845 hygiene findings as pre-existing on both sides. The attribution itself is still wrong -- EPICS Base and libstdc++ symbols reached only through -I roots are still listed as libpvxs's own, and pvxs::version_* as libpvxsIoc's -- so the section says what improved and what did not, and the integration stays advisory. Also recorded: one dump failed the CastXML version probe against the same 0.7.0 binary that a probe and the next dump both accepted, once, not reproducible. Noted as a flake to recognise, not worked around. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BFumn6v9CSsM3euZ8ekcqy
napetrov
marked this pull request as draft
September 16, 2026 14:53
Author
|
will play a bit with setup , would return back to ready once ready |
abicheck #1315 merged as 0b50f80, adding actions/aggregate,
actions/verify-baseline-source, and library-spec resolution in
actions/baseline. Those are the owners this integration was hand-rolling,
so the hand-rolled versions go.
Deleted outright, with the upstream owner named:
abicheck-inputs.sh (126) actions/baseline library-spec
abicheck-snapshots.sh (69) actions/resolve-baseline kind: members
abicheck-collect.sh (63) actions/aggregate
abicheck-validate-aggregate.sh (65) actions/aggregate
That is every ELF inspection, manifest parse, report-identity rule and
aggregate-document check PVXS was maintaining. Hand-written shell/python
across the integration drops from 415 lines to 212. Zero abicheck .sh
files remain.
What replaces them is declaration. .ci-local/abicheck-components.json
states the ownership split -- libpvxs owns every installed public header
except iochooks.h, libpvxsIoc owns only iochooks.h, the core and EPICS
trees are include context that never widens what a component owns -- and
abicheck resolves it.
Two assertions are kept deliberately rather than lost to a glob, because a
pattern matching nothing is a hard error upstream:
* versionNum.h is named explicitly as well as matched by *.h. Verified:
with the glob alone a missing generated header resolves silently to 14
headers, which would report every versionNum declaration as removed.
* header_exclude on iochooks.h fails if that header ever moves, so
libpvxs cannot quietly re-acquire its sibling's declarations.
Capture parity was checked before deleting anything, not assumed: the new
resolver and the old script name the same two artifacts, the same 15/1
header split and the same four include roots on a real PVXS install, with
machine EM_X86_64. Both guards above were fired against a disposable copy
of that tree.
The eligibility shell goes to verify-baseline-source: producer-run for the
PR base, tag for the release channel. It keeps what the shell established
-- refs/tags resolution rather than the commits endpoint, annotated-tag
peeling, no v prefix, the capture must belong to the tagged commit, and
not_found distinct from lookup_failed -- and adds printed rejection
reasons. PVXS still fetches the bytes; only the decision moved.
actions/aggregate replaces collect + aggregate + validate, including the
|| true. compatibility-exit carries aggregate's own 0/1/2/4 without
failing the step, so the gate stays advisory, while a refused declaration
or a document describing no real outcome fails loudly. Exercised on real
reports: a half-declared run yields status=fail coverage=partial with
channels split accepted-main 2/0 and release-contract 0/2, the old
{"aggregate_schema_version":"nonsense"} bypass is refused, and a corrupted
report is tracked as unusable rather than as one that never ran -- a
distinction the deleted validator could not make.
Known issues re-measured on 0b50f80 rather than carried forward: unchanged
at 505/340 detected, 0 gating, NO_CHANGE.
Not delivered upstream, so kept and disclosed: abicheck-publish-baseline
has no upstream replacement yet -- publish-baseline.yml captures rather
than publishing a pre-captured set. Extending it is the owed follow-up; a
second publisher here would be the competing implementation this change
exists to remove.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BFumn6v9CSsM3euZ8ekcqy
PVXS EPICS run 35130816212 at 22ca9d5, success. This is the execution abicheck's HANDOFF.md said was missing: its Actions' composite step wiring had never run on a GitHub runner, only as unit-tested decisions. All four ran. library-spec resolved the same 15/1 header split and the same two artifacts the deleted script produced, from the same SONAME alias chain -- lib/linux-x86_64 holds libpvxs.so -> libpvxs.so.1.5, and the glob de-duplicated to the one real object instead of analysing the alias twice. resolve-baseline kind: members returned outcome=resolved for both, with libpvxsIoc keeping its real casing. The eligibility check justified itself on this run rather than in theory. A push run for the PR's exact base commit, on the right branch, from the right workflow, did exist -- and had failed. verify-baseline-source refused it by name and printed why. Without that rule a failed run's snapshot would have become the baseline this PR is measured against, which is exactly the case selecting on base SHA alone cannot catch. The aggregate reported status=fail, coverage=empty, 0/4 analyzed, compatibility-exit=1, channels split per baseline channel -- and the step still succeeded. That is the advisory contract: the code is carried, not swallowed, and not converted into a job failure. Timings recorded separately per step so the bounded timeout has evidence behind it: capture 360s dominates, everything else is under 30s. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BFumn6v9CSsM3euZ8ekcqy
All four verified against the code before changing anything; all four are in files this PR owns. 1. The component declaration was resolved after `cd "$TOP"`. In the matrix leg TOP is the workspace so it worked, but the historical bootstrap points TOP at a checkout of an OLD tag -- a revision that predates .ci-local/abicheck-components.json entirely. The bootstrap would have read a missing file, or a stale declaration if one ever existed there. The declaration belongs to the checkout, so it now resolves against the workspace, with an explicit existence check. This was introduced by the migration itself. 2. The bootstrap gave one job `contents: write` AND ran the historical revision's own code in it. cue.py, its submodules and its makefiles run in the shared workspace, and the local publishing action is loaded from that same workspace afterwards and handed github.token -- so historical code could replace the action that receives the token. `persist-credentials: false` does not cover this: it stops the historical checkout getting credentials, not a later step loading a tampered local action. Split into a `contents: read` build+capture job that hands the baseline-set over as an artifact, and a `contents: write` publishing job that checks out only the default branch and runs none of that code. Asserted mechanically: the write-scoped job's definition contains neither "historical" nor "cue.py". This is also what the task asked for -- read-only build/capture separated from write-capable publication -- and the single-job version did not meet it. 3. post-on was `changes`, so a run whose comparison came back clean published nothing and left an earlier run's warning standing. The contract is one sticky comment per PR/profile that reflects the current state, so a clean resolution has to clear a stale warning. Now `always`. Checked rather than assumed: an INCOMPLETE analysis already counts as a change (total_changes=5 on the real aggregate), so `changes` was publishing that correctly -- it is specifically the clean case it skipped. 4. `gh release list --limit 1` defaults both exclusions to false, so a draft or pre-release could be selected as the release contract. Now --exclude-drafts --exclude-pre-releases. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BFumn6v9CSsM3euZ8ekcqy
3 of 4 tasks
abicheck #1319 fixes a defect that silently disabled a guarantee this integration claims. At the previous pin, actions/report declared `source-run-id` and `source-run-attempt`, documented them, and its run.sh read INPUT_SOURCE_RUN_ID / INPUT_SOURCE_RUN_ATTEMPT -- but action.yml never forwarded the inputs into the step's env block. The values this caller passes were discarded, and run.sh fell through to its documented default: GITHUB_RUN_ID, the PUBLISHER's own run. That orders the sticky comment by when publication was triggered rather than when the analysis ran, so a late re-run of an older commit carries the larger id and overwrites the newer result. It is the exact inversion 440fa8e wired those inputs to prevent, and it was inert from then until now. Nothing in PVXS was wrong; the values simply never arrived. Verified at both revisions rather than inferred: at 0b50f80 action.yml contains no INPUT_SOURCE_RUN_ID at all while run.sh reads it; at 80cf72a both lines are present. Drift across the eight pinned Actions between the two revisions is exactly this: actions/report/action.yml, +2 lines. Nothing else changed, so this is a two-line behavioural fix and not a re-validation of the whole surface. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BFumn6v9CSsM3euZ8ekcqy
abicheck #1319 added publish-baseline.yml's baseline-set-artifact-prefix -- the pre-captured-set input this integration's local publish action has been waiting for, and stronger than that action: it validates the set against the profile, tag and generation it is being published as. It fits one of the two publication paths. The bootstrap (workflow_dispatch) produces its set in the same run, so the workflow can download it. Tag publication is a workflow_run job whose set lives in the PRODUCING run, and publish-baseline.yml fetches the pre-captured set with actions/download-artifact using `pattern:` alone -- no run-id -- so it can only see artifacts of the run it executes in. Moving the capture into the publishing run would fix the fetch and break the security boundary: the separation between the unprivileged run that captures and the trusted run that publishes is the point. So that is not an option. Adopting it for the bootstrap alone would leave two publishers for one job, which is the duplication this whole change exists to remove. The local action therefore stays, with the gap stated rather than worked around: publish-baseline.yml needs a run-id passthrough on the pre-captured path, or the ability to accept an already-downloaded directory. Either retires the action outright. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BFumn6v9CSsM3euZ8ekcqy
The write-scoped bootstrap-publish job carried the comment "Default branch only" and did not enforce it. workflow_dispatch runs from whichever ref is selected, and actions/checkout with no `ref:` takes that ref -- so dispatching from any branch would load ./.github/actions/abicheck-publish-baseline from that branch into a job holding contents: write and github.token. That is the same class of exposure the previous commit split the jobs to close, reached by a different route: not historical code overwriting the action, but the dispatch ref selecting which copy of it runs. Splitting the jobs removed the first route and left this one. Worse than a missing guard, the comment asserted the property: a reviewer reading "Default branch only" has no reason to check whether the YAML says so. It did not. Now enforced twice. The job refuses to run unless github.ref is the default branch, and its checkout names that branch explicitly rather than inheriting it. bootstrap-build stays unrestricted and read-only on purpose: dispatching it from a branch to exercise the capture is safe, and the publish job simply does not run. The documentation said "checks out only the default branch" too, which was the same unbacked claim; it now describes the enforcement. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BFumn6v9CSsM3euZ8ekcqy
The advisory job concludes success on every run of this branch while the aggregate reports `status=fail coverage=empty, 0/4 target(s) analyzed`. That combination is correct -- the gate is advisory and missing coverage is reported as missing rather than as a clean result -- but nothing in the documentation said it was the current state, so a reviewer seeing a green check had no way to know that no ABI comparison has actually run here. Both channels are unavailable for reasons this pull request cannot fix: the base branch's own ci-scripts-build.yml run concluded failure, which verify-baseline-source refuses by name, and no baseline release asset has been published yet. What the real runners do demonstrate is the candidate half -- build reuse, declarative component resolution, capture, and resolve-baseline returning outcome=resolved with two members -- plus the eligibility and aggregation machinery behaving correctly with nothing to compare against. The comparison half has only been exercised against locally constructed snapshot pairs. Separating those two claims is the point of this section. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BFumn6v9CSsM3euZ8ekcqy
The publisher checked the manifest's profile and that both components were present, but never that the set describes the commit the tag actually names. A set captured from a different revision could therefore be published as a release's baseline, and every later release-contract comparison would report ABI changes that are really just the gap between two revisions. Nothing downstream catches this. The accepted-main comparisons pass expected-project-ref to check-target, because a pull request knows exactly which commit its baseline must describe. The release-contract comparisons deliberately pass none -- the release is selected at consumption time, so the consumer has no expected ref to assert. That leaves the publisher as the only place either path can catch it. It matters most on the bootstrap path, which runs a historical revision's own build system (cue.py, its submodules, its makefiles) in the shared workspace before the runner loads the capture action from this checkout. Splitting that job in two took the write token out of that code's reach; it did not make the artifact it hands over trustworthy. Verifying the claim where both paths converge is the difference between trusting the producer and checking it. The tag is resolved the same way the rest of this integration resolves tags: refs/tags/<name> explicitly rather than the commits endpoint, which would resolve a branch of the same name, and an annotated tag is peeled rather than having its tag-object sha accepted as the commit. Verified against a stubbed API: a matching revision passes, a wrong revision is refused by name, an unpeeled annotated-tag sha is refused, and a manifest with no project_ref is refused. Reported by CodeRabbit on PR #2. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BFumn6v9CSsM3euZ8ekcqy
Three defects the completion review identified, each verified against the
real code before changing it.
1. The component declaration was rendered with `sed`, which is not JSON
escaping. Measured against the real declaration with EPICS_BASE values
a path may legitimately contain: `&` silently produced a WRONG value (in
a sed replacement it means "the whole match"), `"` produced INVALID
JSON, a backslash was dropped, and the `|` delimiter failed the command
outright. EPICS_BASE comes from configure/RELEASE.local, so its content
is not ours to constrain. The binding is now done by a JSON reader and
writer that substitutes exactly two names and refuses any other ${...},
so it stays a two-variable binding rather than a local template engine.
All five previously-broken values now bind correctly.
The extraction config is now resolved against the trusted checkout
explicitly too. It was correct only by accident: a `uses:` step runs
with cwd = workspace, so a relative path happened to land in the right
tree. Accident is not a property.
2. The publisher displayed a commit the analysis never looked at.
verify-source-run's `tested-sha` input was never passed, and empty means
"the run's own head SHA" -- for a pull_request producer that is the PR
HEAD, not the ephemeral merge commit that was actually built and
captured. The comment above that step asserted the opposite, which is
the same failure as the "Default branch only" comment fixed earlier: the
claim was in prose, not in the code.
Now the documented two-pass shape. The producer records the commit it
analysed into the REPORTS artifact -- previously this context went only
into the candidate artifact, which the publisher never downloads, so it
could not have been used even in principle. The publisher reads that
claim, validates it as a full hex SHA before it becomes a step output (a
bare echo of file contents lets a newline forge any other output of a
privileged job), and passes it back through verify-source-run, which
accepts it only as the PR head or a merge of it. It remains a claim
until the API agrees. Verified: a valid SHA is accepted, and forgery,
abbreviated, non-hex, empty and command-substitution inputs are refused.
3. A green advisory job said nothing about having compared nothing. The
job summary now leads with the canonical counts -- "ABI/API comparison
not performed: 0/4 checks completed. Baselines unavailable; no
compatibility verdict was produced." -- and then renders the aggregate
through the same upstream renderer the publisher uses, in dry-run mode:
no API, no token, no posting, so the analysis job stays contents: read.
Every value comes from actions/aggregate's outputs; no report file is
re-read and no reason string is parsed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BFumn6v9CSsM3euZ8ekcqy
…hing Two things this branch had been asserting without having measured them. **The C++17 extraction override.** The recorded reason for it was wrong. It blamed CastXML 0.7.0 / Clang 20 and quoted one libstdc++ error for both C++11 and C++14 -- but that was a local experiment on a toolchain CI does not use. Re-measured on the toolchain CI actually selects (CastXML 0.6.20260105-g9864b1e, bundled Clang 21.1.8, GCC 13.3.0), by running `abicheck dump` over the real declared header set once per mode: C++11 and C++14 both still fail, for two DIFFERENT reasons, and C++17 is the only mode that extracts. So the override stays -- but the limitation recorded beside it is now the one that was demonstrated, on the toolchain that matters. Worth noting for anyone who repeats this: a bare `castxml` invocation parses these headers in C++11 quite happily. It is the extraction pipeline that does not. Testing the parser instead of the pipeline is how the previous measurement reached a confident wrong answer. **Four real comparisons.** Until now nothing had compared anything: every CI run reports 0/4, for reasons outside this pull request, and this branch had no evidence the comparison path works at all. All three revisions were built and captured locally with one declaration and one toolchain -- base b5024df, release tag 1.5.2, and head -- and all four checks run. The two channels disagree, which is the result worth having. Against its own base this pull request is COMPATIBLE on both components with zero public additions, removals or modifications, which is what a CI-and-docs-only diff should produce. Against release 1.5.2, libpvxs is BREAKING: two weak typeinfo symbols for a lambda inside SharedPV::Impl::connectSub are in 1.5.2 and gone afterwards. Confirmed independently of abicheck with `nm -D` -- present in 1.5.2, absent from both base and head -- so it is drift between the release and master, not something this pull request did. That distinction is the entire reason the channels are separate, and it is now demonstrated on real data rather than asserted. Controls: an independent rebuild of the same revision is COMPATIBLE at 100% binary compatibility; a disposable public break is caught as func_removed / breaking / artifact_proven; a disposable compatible addition is caught as func_added / compatible / non-gating; snapshots still compare in 1s after their source and install trees are deleted. The controls lived in a throwaway worktree and HEAD is untouched by them. The rebuild control also puts a number on the noise: comparing a revision against an independent rebuild of ITSELF still reports 339 risk changes. That is the floor of the known upstream attribution defect, and it is why the 339-517 risk counts elsewhere are reported but never gated. **A wrong answer, recorded rather than quietly fixed.** The first attempt reported libpvxsIoc BREAKING against its own base -- impossible for this diff. The shared checkout's installed include/ tree (a gitignored build output) still held an earlier session's disposable controls while its source and binary were clean, so the candidate snapshot came from a stale mutated install. The tool was right about the inputs it was given; the inputs were wrong. Everything above was re-measured from fresh worktrees. Also re-checked the upstream publisher gap at abicheck main (db3b12c): actions/ and publish-baseline.yml are byte-identical to this pin, so the missing run-id passthrough is unchanged and there is nothing newer to adopt. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BFumn6v9CSsM3euZ8ekcqy
The abicheck job failed at "Set up job" on the previous commit -- before any
step ran -- because the dry-run renderer step I added references
abicheck/abicheck/actions/report, and that action fails GitHub Actions
template validation at the pinned revision:
actions/report/action.yml (Line: 119, Col: 18): Unrecognized named-value:
'github'. Located at position 1 within expression:
github.event.workflow_run.id
The cause is prose, not logic. Two input DESCRIPTIONS quote
`${{ github.event.workflow_run.id }}` and
`${{ github.event.workflow_run.run_attempt }}` as usage examples
(action.yml:122 and :133). Actions evaluates expressions inside input
descriptions and the `github` context does not exist in action metadata.
The same syntax is legal in inputs.*.default -- actions/check-target uses
`default: ${{ github.repository }}` without trouble -- but not in a
description. It is unconditional: validation happens when the file is
parsed, so no caller, input set or trigger avoids it.
actions/report is the only abicheck Action with an expression in a
description, and no abicheck workflow references actions/report at all,
which is why it had no in-repo consumer to catch this.
The step is removed rather than worked around. The mandated summary
survives: the headline is written from actions/aggregate's own outputs and
depends on nothing that fails to load.
The finding that matters more than my step: abicheck-report.yml cannot work
as pinned either, since its publish step loads the same action. That
workflow has never run -- it is workflow_run-only and not yet on a default
branch -- so nothing had exercised it. The publication path is therefore
blocked on upstream twice: a default-branch deployment AND this fix. Not
papered over, and not vendored: patching a published Action into this
repository is the local reimplementation this integration exists to remove.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BFumn6v9CSsM3euZ8ekcqy
Delete the machinery abicheck now owns, and pin every Action to one
merged, immutable revision (e38c3f9, abicheck#1325 on main).
Deleted
.github/actions/abicheck-publish-baseline/action.yml (149 lines).
Both publication paths now call abicheck's reusable
publish-baseline.yml, which owns packaging, the
manifest/schema/profile/generation/digest gate, tag resolution and
the immutability chain -- identity-verified idempotent republish and
fail-closed on a conflicting asset, rather than filename-only
idempotence. The automatic path takes the bytes from the producer
run through the API with that run's repository, workflow, event, id,
attempt and conclusion verified; the bootstrap hands its set over as
an artifact, not a path assumed to survive a job boundary. The
publication tag and the captured revision stay separate values
(expected-project-ref: commit).
The consumer tested-sha.txt writer and parser, the second
verify-source-run call and the second artifact download. One
acquisition now carries the whole canonical context:
actions/aggregate records analysis_context, and verify-source-run
reads it back with provenance-from/report-from. Missing context is
refused (require-provenance), never reported as the PR head.
The inline recursive JSON binding routine in the capture Action,
replaced by actions/baseline's library-spec-bindings.
The hand-written gh/jq tag peeling in the bootstrap, replaced by
verify-baseline-source mode: tag.
The Python that rebuilt every target@profile#channel@depth string and
reconstructed the expected set. Each check is declared from
check-target's own check-id output; which checks an event asks for
stays PVXS policy, expressed as the step's `if:`.
The handwritten rule that analyzed == 0 means "baselines unavailable",
and the duplicate status-echo block. One canonical bounded renderer
(aggregate.txt) plus the document's own coverage/status/channels
outputs; the cause comes from the structured outcome, because zero
analyses can equally mean a failed capture or a corrupt report.
Restored post-on: changes -- the pinned revision clears a resolved
result in place and posts for a material analysis limitation, so
`always` is not needed to clear a stale warning.
Preserved: one build and one capture per component reused across both
channels; all four PR comparisons (two components x two channels) with
accepted-main limited to pull-request events; the C++17 extraction
deviation; read-only build and trusted write-capable publication in
separate jobs with the default-branch restriction; bounded artifact
processing; advisory gate.
Documentation reduced to a maintenance guide; investigation history
moves to the pull request evidence.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q2Herd15iLCwK3ainaw52a
Deriving the aggregate declaration from check-target's own check-id outputs lost the expected set in the one case that matters most: when the candidate download or snapshot resolution fails, every comparison step is skipped and emits no id, so the filter reduced the declaration to an empty list and the run reported a shorter set of checks than it actually had -- an ABI analysis failure disappearing from the advisory result. Declare from the event's own policy instead, unconditionally, with the report path left empty when a comparison produced nothing: that is a declared-but-unavailable check, which is what the aggregate contract expects. The ids are still cross-checked upstream, where a report recording its own target_id is authoritative and a disagreeing declaration is refused rather than silently relabelled. Verified by running the step against three inputs: a pull_request event with every comparison skipped now declares 4 checks with empty reports (previously 0), the same event with reports declares 4 with paths, and a push event declares only the 2 release-contract checks. Also corrects the publication section of documentation/abicheck.md: the producer run and its pull request come from the API, but the analysed commit is read from aggregate.json and then verified against the API -- the text had the trust boundary backwards, and stated it accurately only in the following paragraph. Both found by CodeRabbit on #4. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q2Herd15iLCwK3ainaw52a
The first real run of the consolidated job failed on both accepted-main checks: resolve-baseline failed (ambiguous): .../abicheck-baseline-main exists but does not contain a manifest.json -- this looks like an empty/partial cache restore or stripped artifact directory, not simply an unpublished baseline (a baseline-path that does not exist at all is the not_found/bootstrap case). The `mkdir -p` step defeated its own purpose. It was added so a staging failure would reach resolve-baseline as that channel's own `not_found` outcome, but resolve-baseline deliberately separates the two states: a path that does not exist is `not_found`, the bootstrap case these checks opt into with baseline-required: false, while a path that exists without a manifest.json is `ambiguous` -- a hard failure, because that is what a partial cache restore or a stripped artifact looks like. Pre-creating the directory therefore converted every unpublished baseline into a corrupt-evidence error. Both download commands create their own --dir, so nothing else needed it. The same run already demonstrates the intended behaviour on the release channel, whose baseline-path is a file inside the directory and so did not exist: outcome not_found, verdict NO_BASELINE, step success, the check still declared and reported as unavailable. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Q2Herd15iLCwK3ainaw52a
Author
Review sweep 2026-10-03Status notes on the current head (
|
Audited the integration against abicheck main 902ee99 and against that
revision's own set-up-abi-compatibility-ci skill.
Repin: all 16 Action/workflow references move from e38c3f9 to 902ee99.
Verified behaviour-neutral for this consumer -- the single intervening
commit removes the ABICC compat front end (which this integration never
used) and strips ADR cross-references from prose; the only interface
additions are optional recording inputs we do not pass
(baseline's extraction-context, check-target's build-system /
build-generator), and publish-baseline's new project-config /
toolchain-bindings-path apply to its capture mode, while this project
publishes through its existing-set mode.
Leanness: the skill's rule is never to restate a default, so
`severity-preset: default` and `head-sha: ${{ github.sha }}` are dropped
from all four check-target calls -- both are exactly those inputs'
upstream defaults. Eight lines, no behaviour change. `base-ref` stays on
the accepted-main pair, where the default is empty and the value is real.
Documented two non-obvious invariants:
- the pin is a main revision rather than the v0.6.0 release tag because
aggregate, report, verify-baseline-source and verify-source-run do not
exist in v0.6.0, and that tag is not an ancestor of main;
- the repeated literal SHA in each `uses:` is required (GitHub does not
expand expressions there), so ABICHECK_REF cannot replace it;
- runner-image toolchain drift needs no guard here: a mismatched pair
resolves as profile_mismatch with verdict null and exit 16, which is
explicitly not a pass.
Re-confirmed unchanged and not worked around: the actions/report blocker
is still present on main at 902ee99 (expressions in two input
descriptions), and check-project.yml still consumes candidate binaries
plus build-output.json, so adopting it would re-extract per cell instead
of reusing the single capture.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q2Herd15iLCwK3ainaw52a
This branch has not been deployed
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
3 participants
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.
Summary
Validation
ABICC remains authoritative during shadow adoption.