Skip to content

fix(ci): dry_run never skipped the real publish; make the gate work - #1477

Merged
drewstone merged 1 commit into
mainfrom
fix/publish-dry-run-guard
Jul 25, 2026
Merged

fix(ci): dry_run never skipped the real publish; make the gate work#1477
drewstone merged 1 commit into
mainfrom
fix/publish-dry-run-guard

Conversation

@drewstone

Copy link
Copy Markdown
Contributor

Summary

  • dry_run in publish-crates.yml never skipped the real publish. The input is declared type: boolean, so inputs.dry_run is a boolean, but both guards compared it to the string 'true'. GitHub coerces mismatched types to numbers (true → 1, 'true' → NaN), so inputs.dry_run != 'true' is always true and == 'true' is always false — every "dry run" executed the real publish step and skipped the report.
  • Affects the release/maintainer flow: the only safety gate before an irreversible ~55-crate publish was inert.

This is not theoretical. A run dispatched with dry_run=true published blueprint-manager-bridge 0.2.0-alpha.11 to crates.io before failing. crates.io versions are immutable — a bad batch cannot be undone.

Change Class

  • Class A (docs/tooling only)
  • Class B (single-crate behavior)
  • Class C (cross-crate/runtime behavior)
  • Class D (protocol/security/metadata semantics)
  • Selected class: Class A
  • Why this class: CI workflow only. No crate source, no public API, no runtime behavior.

Behavior Contract

  • Current behavior: dry_run=true runs batch-publish.sh against crates.io and skips the report — the inverse of the intent.
  • Intended behavior: dry_run=true skips publishing and runs a report that fails on a real problem; dry_run=false publishes.
  • Invariants that must hold: a dry run must never mutate crates.io; a dry run that passes must mean a real run would not hit a version collision.
  • Failure mode choice (fail-closed or fail-open): fail-closed — the dry run exits non-zero and lists the offending crates when any target version already exists.

Risk and Scope

  • Security impact: removes an unintended path to an irreversible public publish from a run the operator believed was a no-op.
  • Compatibility impact (APIs, metadata format, configs): none — workflow only; inputs unchanged.
  • Migration notes (if any): none.
  • Rollback plan: revert the workflow file.

Verification

# guards now compare the boolean, not a string
grep -n 'dry_run' .github/workflows/publish-crates.yml
#   59:        if: ${{ !inputs.dry_run }}
#   66:        if: ${{ inputs.dry_run }}

# collision check exercised against the real registry (pre-release-merge checkout,
# so these versions are legitimately already published -> the check fires):
#   blueprint-manager-bridge 0.2.0-alpha.9   ALREADY PUBLISHED
#   blueprint-manager        0.4.0-alpha.11  ALREADY PUBLISHED
#   cargo-tangle             0.5.0-alpha.11  ALREADY PUBLISHED
#   blueprint-qos            0.2.0-alpha.11  ALREADY PUBLISHED

# structural validation: guards present, heredoc balanced, no tabs

Harness Evidence

  • Reproducer added/updated: the dry-run step itself is now the reproducer — it fails on the exact condition (already-published version) that strands a batch mid-run.
  • Negative-path coverage added: registry-lookup failures are caught per crate and reported without aborting the whole report.
  • Key assertions that prove the fix: the publish step is guarded by ${{ !inputs.dry_run }}, so a boolean true skips it; the report step is guarded by ${{ inputs.dry_run }}.

Deliberately not using a per-crate cargo publish --dry-run: dry-run still resolves dependencies from the registry, so every crate depending on a sibling being published in the same batch would fail spuriously. The version-collision check is the meaningful, cheap gate — it is the failure that actually strands batch-publish.sh partway through its topological, rate-limited run.

Checklist

  • I followed docs/engineering/HARNESS_ENGINEERING_PLAYBOOK.md.
  • I followed docs/engineering/HARNESS_ENGINEERING_SPEC.md.
  • I used docs/engineering/HARNESS_REVIEW_CHECKLIST.md while implementing/reviewing.
  • I added or updated tests that reproduce the original issue. — the dry-run report is the check; validated against the live registry.
  • I added or updated negative-path tests for invalid/missing/conflicting input. — per-crate registry-lookup failures are handled and reported.
  • I documented behavior or interface changes in docs/examples when needed. — rationale is inline in the workflow.
  • I evaluated compatibility/migration impact explicitly. — workflow-only, inputs unchanged.
  • I validated local formatting, clippy, and relevant tests. — n/a for a workflow file; structural YAML validation run instead.

`dry_run` is declared `type: boolean`, so `inputs.dry_run` is a BOOLEAN, but
both guards compared it to the STRING 'true'. GitHub coerces mismatched types to
numbers (true -> 1, 'true' -> NaN), so `inputs.dry_run != 'true'` is always true
and `== 'true'` is always false: every "dry run" ran the REAL publish step and
skipped the report.

This is not theoretical — a run dispatched with dry_run=true published
blueprint-manager-bridge 0.2.0-alpha.11 to crates.io before failing. crates.io
versions are immutable, so a bad batch cannot be undone.

- Guards now compare the boolean: ${{ !inputs.dry_run }} / ${{ inputs.dry_run }}.
- The dry-run report now actually checks something: it resolves each crate's
  workspace version and flags any already published on crates.io, failing the
  job with the list. That is the collision that strands batch-publish.sh
  half-way through its topological, rate-limited run. A per-crate
  'cargo publish --dry-run' is deliberately NOT used — dry-run still resolves
  dependencies from the registry, so every crate depending on a sibling in the
  same batch would fail spuriously.

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Auto-approved drewstone PR — 088db74a

This PR was opened by the trusted drewstone account.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.

tangletools · auto-approval · reason: drewstone_author · 2026-07-25T01:35:11Z

@github-actions

Copy link
Copy Markdown

PR Quality Gate Summary

  • Status: pass
  • Selected class: Class A
  • Required class: Class A
  • Reason: Docs/process-only changes.
  • Changed files: 1

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Value Audit — sound-with-nits

Verdict sound-with-nits
Concerns 2 (2 weak-concern)
Heuristic 0.0s
Duplication 0.0s
Interrogation 108.6s (2 bridge agents)
Total 108.6s

💰 Value — sound-with-nits

Fixes a real, inert safety gate (boolean-vs-string comparison made dry_run always publish for real) and adds a fail-closed version-collision pre-check; ships correctly with one minor reuse opportunity.

  • What it does: Two changes in .github/workflows/publish-crates.yml. (1) Flips the two job-step guards from broken string comparisons (inputs.dry_run != 'true' / == 'true') to idiomatic boolean expressions (!inputs.dry_run / inputs.dry_run). The input is declared type: boolean (line 10), so inputs.dry_run is a JSON boolean; GitHub's expression evaluator does not coerce boolean-to-string, so the old `!
  • Goals it achieves: Restore the only safety gate before an irreversible ~55-crate publish: make dry_run actually skip publishing and instead run a meaningful pre-check that catches version collisions before they strand a batch half-published. The collision check is the specific failure that batch-publish.sh cannot cleanly recover from mid-flight (a late-list collision aborts the run with earlier crates already live o
  • Assessment: Good change, correct on the merits. The bug is real and serious — I confirmed the old guards via git show on the parent commit and the type mismatch via the input declaration at line 10; a boolean-vs-string comparison in Actions expressions does not coerce, so the publish step was unconditionally active and the dry-run step was dead code. The fail-closed collision check is a coherent, well-scoped
  • Better / existing approach: Searched .github/scripts/, .github/actions/, and all workflows for existing version/collision/publish checks. Found .github/scripts/batch-publish.sh:168 version_is_live() — already answers 'is version V of crate N on crates.io?' against the sparse index (index.crates.io), which is the SAME index cargo resolves dependencies against and is used at lines 209/269 to skip already-published crates. Th
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 2
  • Bridge warning: opencode/kimi-for-coding/k2p7: bridge stream ended without value-audit content

🎯 Usefulness — sound-with-nits

Correctly fixes an inert safety gate (boolean dry_run compared to string 'true' always mismatched, so every dry run actually published) and upgrades the dry-run report into a real pre-flight collision check against crates.io.

  • Integration: Reachable and correctly wired. publish-crates.yml is workflow_dispatch-only — the manual maintainer publish/validation path, distinct from the automated release-plz.yml (push-to-main) path which has no dry-run gate at all (release-plz.yml:80-85). Both consume the same .github/scripts/batch-publish.sh. The fixed guards (publish-crates.yml:59 ${{ !inputs.dry_run }}, :66 `${{ inputs.dry
  • Fit with existing patterns: Fits the established grain precisely. The collision check reuses cargo metadata --format-version 1 --no-deps — the identical invocation in the release-JSON step (:41) and batch-publish.sh (:61, :196). Its comment explaining why per-crate cargo publish --dry-run is unusable (sibling-dep registry resolution) mirrors the same propagation-aware reasoning already in batch-publish.sh (`:152-16
  • Real-world viability: Holds up on the real path with one caveat. crates.io web-API lookup is authoritative for 'is this version already published' — a version visible there is definitely live, so no false negatives from the source. Fail-closed on real collisions (sys.exit(1) at :111) matches the stated fail-closed contract. However, transient network/HTTP errors are fail-open (:94 catches all exceptions, treats t
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 1

💰 Value Audit

🟡 Collision check duplicates batch-publish.sh's version_is_live against a different registry endpoint [duplication] ``

batch-publish.sh:168-172 already implements 'is version V of crate N published?' via version_is_live() / sparse_index_path() against index.crates.io (the sparse index cargo resolves against), used at lines 209 and 269. The new dry-run step re-implements the same question in Python against the crates.io web API (crates.io/api/v1/crates/{n}, lines 89-93). Two implementations of one fact that can drift; the web API also lags the sparse index that the real publish path keys off. Prefer adding a --ch

🎯 Usefulness Audit

🟡 Transient crates.io API errors fail-open, masking the exact collision the dry run exists to catch [robustness] ``

At publish-crates.yml:94 the broad except Exception swallows any HTTP/timeout error, prints a note, and treats the crate as non-colliding. crates.io rate-limits and 503s do happen, so under realistic use a blip during the dry run silently disables the pre-flight for that crate. Consider distinguishing HTTP errors (fail-closed or surface prominently in the summary) from the genuine 'crate not yet on registry' case (404 → ok). Low impact because collisions are deterministic and the operator can


What this audit checks

It judges the change on its merits — not whether it was tasked out in an issue. Unticketed, fast-moving work is fine; the question is whether the change is good and whether a better or existing approach should be used instead.

Pass What it asks
Heuristic Vague title? Whitespace-only or cruft-bearing diff? (content signals only)
Duplication Do added function/class names already exist elsewhere in the repo?
Value Audit What does it do? What goal does it achieve? Is it good? Better architecture or already-exists?
Usefulness Audit Does it integrate and fit? Will it hold up in real use and actually get used?

Findings are concerns, not blocks — the human reviewer decides what to do with them.

value-audit · 20260725T014151Z

@tangletools

Copy link
Copy Markdown
Contributor

✅ No Blockers — 088db74a

Review health 100/100 · Reviewer score 79/100 · Confidence 65/100 · 3 findings (1 medium, 2 low)

glm: Correctness 79 · Security 79 · Testing 79 · Architecture 79

Reviewer score is advisory once the run is complete and the verdict has no blockers.

Full multi-shot audit completed 1/1 planned shots over 1 changed files. Global verifier still owns final merge decision.

🟠 MEDIUM Broad except handler masks transient API errors as 'no collision' — .github/workflows/publish-crates.yml

Line 94 except Exception as e: catches everything — HTTP 404 (crate genuinely unpublished, correctly → empty set), but also 429/500/503/DNS/timeout (transient, state UNKNOWN). For transient errors the script sets published = set() and continue, which can let a real collision pass undetected and print 'No version collisions. A real run would proceed.' — exactly the false-green the check was built to prevent. Fix: catch urllib.error.HTTPError explicitly; if e.code == 404, treat as new crate; for any other status or URLError/timeout, print a warning and either retry (2-3x with backoff) or append to a separate unchecked list and exit nonzero if any

🟡 LOW No rate-limit pacing between crates.io API requests — .github/workflows/publish-crates.yml

The loop fires requests back-to-back with only a 20s per-request timeout. crates.io's unauthenticated GET limit is ~1 req/sec; with 30+ workspace crates a burst can draw 429s that then get swallowed by the broad except above. Add time.sleep(1) between iterations (or after every request) to stay under the limit. Low severity because this only affects dry-run fidelity, but it directly amplifies the medium finding above.

🟡 LOW Unknown local version silently reports 'ok' — .github/workflows/publish-crates.yml

v = version.get(n, "?") — if a crate in release-output.json is missing from cargo metadata, v becomes "?", which is never in published, so the line-102 branch prints {n} ? ok. This is defensively dead code today (release-output.json is generated from the same metadata moments earlier), but if the two sources ever diverge it would emit a false 'ok' instead of an error. Suggest: if n not in version, append to collisions (or a separate unknown list) and exit nonzero.


tangletools · 2026-07-25T01:41:54Z · trace

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Approved — 3 non-blocking findings — 088db74a

Full multi-shot audit completed 1/1 planned shots over 1 changed files. Global verifier still owns final merge decision.

Full immutable report for this review: trace

Summary comment for this run: full summary


tangletools · 2026-07-25T01:41:54Z · immutable trace

@drewstone
drewstone merged commit 7ac1b3c into main Jul 25, 2026
27 of 31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants