Repository navigation
fix(#3583): the upstream gate's three refusals are told apart in the log - #4084
Conversation
This gate held MeshWeaver.Reinsurance 23 times in 24 hours. Every refusal exits
1 and still does — that is not what changed, and nothing here is advisory, gets
a retry, or gets a fallback. What changed is that a reader can now tell WHICH of
three things happened, because they want three different actions:
CANNOT RESOLVE … no identity at all. A refusal: there is no directory to ask
about, so NOTHING was checked and no upstream may be called
absent.
CANNOT DETERMINE … an identity, but a probe ERRORED. Also a refusal: with
probes failing the absent count has an unknown denominator,
so it is printed as a FLOOR under this headline, never as
the verdict.
release availability: N of M source(s) are not available …
every probe answered, and the answer was NO. The only one
that is a statement about an upstream.
Before this, the middle case was folded into the last one's MISSING array, so "I
could not ask" was reported as "the upstream has not published" — a smaller
number in place of a refusal, which is exactly the reading that sends someone to
chase an upstream when the real fault is a credential or the share.
Second half: the identity and its ORIGIN are printed on EVERY path, before the
first probe, success included. The identity is resolved two steps up from the
portal image, and WHICH portal image a run gets depends on the trigger — a repo
pin on push, the wake payload on repository_dispatch, a moving release tag on
schedule. Those want different actions (move the pin / wait for the wave / stop
expecting a moving tag to converge), so a log that does not name the identity
and where it came from cannot answer "why is this waiting".
Not done here, deliberately: resolving the identity from the newest SEALED set
(4b). That is a resolution change and is still open.
TEST, and it earned its place immediately: test-release-availability-refusals.py
executes the REAL script against an `az` PATH stub — no credential, no network —
and asserts on the DISTINGUISHING TEXT plus the ABSENCE of the other two
headlines, since asserting exit codes alone would pass even if all three printed
one sentence. Case 4 is a control that must PASS.
Falsified three ways before acceptance:
• fold the errored probe back into the absent count (the pre-change
behaviour) -> 5 assertions fail, on cases 3 and 3b
• drop the "identity resolved" line
-> 4 assertions fail, on cases 5 and 6
• give the resolve-failure the absent headline
-> 2 assertions fail, on case 1
Restored: 0 failures.
It also caught a real bug in this change before it could ship: escaped backticks
inside a ${VAR:-default} expansion broke the substitution, which would have
errored on EVERY gate run including the success path. Verified by execution and
`bash -n`, not by "the YAML parses".
check-workflow-timeouts: 84 jobs checked, 0 violations, cap 45; the job the new
step joins is capped at 10.
Touches only the core shared lane — no satellite ci.yml.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…condition Two additions to the churn memo, both measured rather than asserted. WHAT LANDED. #4084 makes the gate's three refusals distinguishable and logs the resolved identity with its origin — log quality only, every refusal still exits 1. 4b (resolve from the newest SEALED set) is NOT done and stays open. A future reader should not have to reconstruct which half shipped. WHY 4b SHRANK. Failures classified by dispatch type, with denominators: in the same 24h window, meshweaver-framework-released ran 45 times with 23 failures, meshweaver-upstream-published 35 times with 10. Of the 23 GATE failures, 20 were framework-released. Phase 1 drops that trigger type, so it removes 20 of 23 and ~45 runs/day by a route unrelated to 4b. Of the paths that remain, upstream-published and push resolve from a wake payload and a repo pin — both already settled, so 4b does not apply — leaving only `schedule`, which resolves a moving tag. Measured failures there: 0 of 2. n=2, stated as such. 🚨 PHASE 1 REMOVES THE REPORT, NOT THE CONDITION. Those 20 failures were the system telling us no upstream was baked for the new identity. Dropping the wake stops the telling; it bakes nothing. A portal rolling onto such an identity still compiles every NodeType locally at runtime — the cost stage 4 exists to remove — and the daily cron catches it within 24h instead of immediately. That may well be the right trade; 45 runs/day is real money. But it is the same shape as making a gate advisory — a loud CI red becomes a quiet runtime compile — so it should be chosen knowingly rather than arrived at. Written down here because it is exactly the kind of fact that gets rediscovered expensively. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
Moderate issues remain in summary reporting, provenance logging, caller error handling, and regression-test coverage.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This pull request improves shared CI release-availability diagnostics by distinguishing unresolved identities, probe errors, and genuinely absent publications.
Changes:
- Adds distinct refusal classifications and identity provenance logging.
- Integrates origin details into the publish-bake workflow.
- Adds execution-based regression coverage.
File summaries
| File | Summary |
|---|---|
.github/workflows/node-repo-publish-bake.yml |
Integrates the availability gate and provenance logging. |
.github/workflows/dotnet-test.yml |
Runs the refusal regression test. |
.github/scripts/test-release-availability-refusals.py |
Tests refusal messaging with stubbed Azure responses. |
.github/scripts/check-release-availability.sh |
Separates availability outcomes and reports identity context. |
Review details
Suppressed comments (3)
.github/scripts/check-release-availability.sh:160
- In the mixed-error case, the
ABSENTentries are emitted only as::error::lines and are not written toGITHUB_STEP_SUMMARY, while this script's contract says the summary names what is missing. A reader using the GitHub summary therefore sees the undetermined source(s) but not which source(s) answered NO, losing the floor detail this change introduces. Add a summary entry for each absent source and mark it as floor-only.
for m in "${ABSENT[@]}"; do echo "::error:: • $m"; done
.github/scripts/check-release-availability.sh:127
- The new identity/origin line is after the
BAKE_PUBLISH_TARGETSand target-shape checks, so an explicit--identityinvocation that fails because the store target is missing or malformed exits without the promised identity or origin. That leaves the framework being diagnosed unnamed on a determination failure. Emit the identity/origin immediately after parsing for explicit identities (and after marker resolution for version mode), before validating the storage target.
echo "identity resolved: $IDENTITY — from $ORIGIN_TEXT"
.github/scripts/test-release-availability-refusals.py:76
- Case 1 claims that no availability probe is made after identity resolution fails, but the
azstub neither records probes nor rejects an unexpectedstorage file existscall; its default path returnstrue. A regression that probescrmand then still printsCANNOT RESOLVEwould therefore pass this case. Make the stub fail or record any exists call whileAZ_MARKER_FAIL=1, and assert that no probe occurred.
if [ "${AZ_EXISTS_FAIL:-0}" = "1" ]; then exit 3; fi
printf '%s\n' "${AZ_EXISTS:-true}"; exit 0 ;;
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| env: | ||
| BAKE_PUBLISH_TARGETS: ${{ inputs.bake-publish-targets }} | ||
| UPSTREAMS: ${{ inputs.upstream-sources }} | ||
| PLATFORM_IMAGE: ${{ inputs.platform-image }} | ||
| PLATFORM_DIGEST: ${{ inputs.platform-image-digest }} | ||
| run: | | ||
| set -uo pipefail | ||
| # 🚨 The identity is resolved two steps up, from the PORTAL image — and WHICH portal image | ||
| # this run got depends entirely on the trigger (a repo PIN on push/pull_request, the wake's | ||
| # payload on repository_dispatch, a MOVING release tag on schedule). When this gate holds, | ||
| # "why is it waiting" is a question about that choice, and the three paths want three | ||
| # different actions. Stating it here makes the log self-contained (MeshWeaver#3583). | ||
| ORIGIN="the portal image '$PLATFORM_IMAGE'${PLATFORM_DIGEST:+ @ $PLATFORM_DIGEST}, chosen by this run's trigger (event=$GITHUB_EVENT_NAME)" |
There was a problem hiding this comment.
Verified and fixed in 9115c8185. You are right on both halves.
The origin named the declaration, not the effective value. Confirmed by reading the lane: steps.identity reads the /app of steps.hosts.outputs.portal_app, which comes from PORTAL_REF: ${{ steps.platform.outputs.ref }} (line 796) — while I logged inputs.platform-image. On exactly the dispatch paths this line exists to explain, those can be different images. That is the "a config value is not a running one" trap, committed inside a change whose whole purpose is logging honesty. Now PORTAL_REF: ${{ steps.platform.outputs.ref }}, described as "the ref this run RESOLVED, whose /app the identity was read from".
github.event.action — and I should have caught this one myself, because I measured it: on MeshWeaver.Reinsurance over 24 h, meshweaver-framework-released ran 45 times with 23 failures and meshweaver-upstream-published 35 times with 10, and 20 of the 23 gate refusals were the first kind. They are not the same wait — one carries a new identity nothing may have baked for, the other a set just sealed — so collapsing them onto one event_name loses precisely the distinction that explains the number. The origin now reads event=… action=… when an action is present.
| if mw-platform-gate/.github/scripts/check-release-availability.sh \ | ||
| --identity "${{ steps.identity.outputs.identity }}" $UPSTREAMS; then | ||
| --identity "${{ steps.identity.outputs.identity }}" \ | ||
| --identity-origin "$ORIGIN" $UPSTREAMS; then |
There was a problem hiding this comment.
Verified and fixed in 9115c8185. This one defeated half the change and I am glad you caught it.
The script made the distinction and then the very next block in the caller threw it away: after any non-zero return it wrote "at least one upstream … has no sealed publication" plus the Upstreams not ready error — a statement that is false on both refusal paths, where nothing was ever checked. The reader would have been sent to chase an upstream over a credential failure, which is the original defect wearing the workflow's clothes.
The step now branches on the classification the script printed:
| script said | summary + error |
|---|---|
CANNOT RESOLVE |
"no upstream was checked and none is being reported as unpublished… nothing in $UPSTREAMS is implicated" |
CANNOT DETERMINE |
"an ACCESS problem, not an upstream one… waiting for an upstream will not clear it", and any absent count is named a floor |
| the availability verdict | the existing Upstreams not ready wait, unchanged |
All three still exit 1.
And the workflow half is now tested, which it was not before. The step got id: upstream-gate, and the harness EXTRACTS and EXECUTES it rather than re-implementing it, substituting only expressions in KNOWN_EXPRESSIONS and refusing loudly on any other. Falsified by making neither refusal branch match — the pre-fix behaviour — which fails 6 assertions, including the direct one that a refusal "never claims an upstream has no sealed publication".
…psing the refusals
Two review findings (Copilot), both verified and both real. The second one
defeated half the change.
1. THE ORIGIN NAMED THE DECLARATION, NOT THE EFFECTIVE VALUE. I logged
inputs.platform-image, but steps.identity reads the /app of
steps.platform.outputs.ref — which the lane resolves from the wake payload or
from <platform-image>:<released-version>. On exactly the dispatch paths this
line exists to explain, it could name a DIFFERENT image from the one the
identity came from. That is the "a config value is not a running one" trap,
in the change written to fix a logging defect. Now logs the resolved ref.
Also: github.event_name alone collapses the two repository_dispatch actions,
and they are NOT the same wait — meshweaver-framework-released carries a NEW
identity nothing may have baked for, meshweaver-upstream-published carries a
set just sealed. Measured on Reinsurance: 20 of 23 refusals were the first
kind, 3 the second. github.event.action is now included, so the distinction
that explains the number survives into the log.
2. THE CALLER RE-COLLAPSED WHAT THE SCRIPT DISTINGUISHED. After a non-zero
return the step unconditionally wrote "at least one upstream ... has no
sealed publication" and the "Upstreams not ready" error — FALSE on both
refusal paths, where nothing was ever checked. One line after the script made
the distinction, the workflow threw it away. Now branches on the printed
classification: a resolve failure says nothing was checked and no upstream is
implicated; a probe failure says it is an ACCESS problem and that waiting for
an upstream will not clear it.
TEST, extended to the half that was untested. The step gets `id: upstream-gate`
and test-release-availability-refusals.py now EXTRACTS and EXECUTES it — never a
copy — substituting only expressions listed in KNOWN_EXPRESSIONS and REFUSING
loudly on any other, so a step that grows an input fails the harness instead of
running on a guess.
Falsified:
• make neither refusal branch match (the pre-fix behaviour)
-> 6 assertions fail, including "never claims an upstream has no sealed
publication" on both refusal paths — the false claim itself
• remove a known expression
-> the harness REFUSES to execute and says which one
• restored -> 0 failures
PyYAML 6.0.2 is already pinned for this job and five sibling scripts import it.
check-workflow-timeouts: 84 jobs, 0 violations. Workflow parses. Still no
satellite ci.yml touched.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The residual piece from the #3583 triage. Log quality only — no resolution change, nothing advisory, no retry, no fallback. Every refusal still exits 1 and the gate still fails loudly when an upstream genuinely has not published.
The defect
check-release-availability.shis the step that heldMeshWeaver.Reinsurance23 times in 24 hours. It folded an errored probe into the sameMISSINGarray as a genuinely absent seal, so the headline read:…whether the share had answered "no" or had not answered at all. "I could not ask" was reported as "the upstream has not published" — a smaller number in place of a refusal, which sends the reader to chase an upstream when the fault is a credential, the share, or the network.
Three outcomes, three headlines
CANNOT RESOLVE …CANNOT DETERMINE …release availability: N of M source(s) are not available …All three exit 1. Distinguishing them changes what the reader does, never whether the gate fails.
Second half — which identity, and why
The identity and its origin now print on every path, before the first probe, success included. The identity is resolved two steps up from the portal image, and which portal image a run gets depends entirely on the trigger — a repo pin on
push, the wake payload onrepository_dispatch, a moving release tag onschedule. Those want different actions (move the pin / wait for the wave / stop expecting a moving tag to converge), so the caller now states it:Verification — by execution, not "the YAML parses"
test-release-availability-refusals.pyruns the real script against anazPATH stub (no credential, no network) and asserts on the distinguishing text and the absence of the other two headlines — asserting exit codes alone would pass even if all three printed one sentence, which is the defect. Case 4 is a control that must PASS, so the harness cannot confuse "refuses correctly" with "always refuses".🚨 Falsified three ways before acceptance:
identity resolvedlineThat the first mutation is exactly the old behaviour is the point: this test would have caught the original defect.
It also caught a real bug in this change before it shipped — escaped backticks inside a
${VAR:-default}expansion broke the substitution and would have errored on every gate run, including the success path. Found by running it, not by reading it.check-workflow-timeouts: 84 jobs checked, 0 violations, cap 45; the job the new step joins is capped at 10.Scope
check-release-availability.sh,node-repo-publish-bake.yml,dotnet-test.yml, plus the new test. No satelliteci.ymlis touched (another session is editing those).schedulepath alone.Refs #3583. Companion docs: #4082 (
FrameworkIdentityChurn), #4079 (BundleDeliveryStages).🤖 Generated with Claude Code