Skip to content

fix(release): guard package roster and stable publishing - #3443

Open
miguel-heygen wants to merge 7 commits into
mainfrom
deepwork-builder/hf-release-publish-ssot-guard
Open

fix(release): guard package roster and stable publishing#3443
miguel-heygen wants to merge 7 commits into
mainfrom
deepwork-builder/hf-release-publish-ssot-guard

Conversation

@miguel-heygen

@miguel-heygen miguel-heygen commented Aug 23, 2026

Copy link
Copy Markdown
Collaborator

What

Release publishing now fails closed when a public workspace is missing from the publish path or when a stable merge bypasses the repository's effective review/check policy.

Why

The v0.8.11 review exposed two silent integrity gaps:

  • set-version and the publish workflow maintained separate 13-package lists, so deleting one workflow entry still left every existing release test green.
  • the stable workflow created its tag before the merge SHA's required checks completed, and PR chore: release v0.8.11 #3440 merged with zero approvals through a ruleset bypass.

How

  • One typed roster now owns workspace path, workspace name, npm name, order, and publish mode. Discovery reconciles every configured public/private workspace and pins the only special mapping to packages/cli / @hyperframes/clihyperframes.
  • A shared publisher preserves already-published skips, restores the CLI manifest byte-for-byte, continues after individual failures, and reports every failed npm package together. The workflow carries no package roster.
  • Stable merged-PR publishing now requires an exact-SHA main rule-suite result of pass, a valid final-head non-author approval, and terminal-green latest attempts for every dynamically discovered required context/integration. Immutable event/API/checkout identity, malformed data, bypass, self-reference, API errors, missing checks, failures, and hard-deadline expiry all fail before tag creation.
  • The guard defaults to a 25-minute deadline, accepts only validated 15–40 minute repository-variable overrides, and runs inside a 60-minute publish job budget. Source comments and the release runbook document indirect rule-suite enforcement and fail-closed handling when GitHub introduces a new rule type.
  • Sanctioned workflow recovery searches exact-SHA rule suites across GitHub's month window, the actual checked-out packages/* tree has a direct CI ratchet, and aggregate publish failures retain sanitized npm output while redacting credentials and user paths.
  • Stable policy reads use a guard-only, repository-scoped RELEASE_GUARD_TOKEN; the built-in token remains on release-writing steps. Same-client checks prove effective rules, rule suites, reviews, and check runs while sanitizing authentication, scope, endpoint, rate-limit, and malformed-response failures.
  • A weekly and manually dispatchable credential-health workflow discovers a recent merged main PR and exercises those read surfaces without any release, tag, package, or publish capability. Required contexts without a positive integration ID fail closed instead of falling back to legacy statuses.
  • Prerelease tag publishing keeps its existing path and skips stable PR policy queries.

Test plan

  • RED captured for missing SSOT/publisher/guard modules and workflow ordering/roster duplication
  • Mutation tests for omitted public workspaces and non-CLI npm-name overrides
  • Deterministic guard matrix: approval states, exact identity, effective rules, pass/bypass rule suites, newest reruns, missing/pending/failing checks, self-reference, API failures, and deadline clamping
  • Dedicated credential isolation and four-endpoint capability matrix, including missing/blank tokens and sanitized 401/403/404/rate-limit/malformed failures
  • Weekly/manual health workflow cannot publish; discovery, endpoint, integration-ID, and 15/25/40-minute contracts are mutation-pinned
  • bun run test:scripts — 228 Node tests + 42 catalog tests
  • scripts/core/Studio TypeScript checks, changed-file oxlint/oxfmt, diff check, and fallow
  • full monorepo build
  • all 13 packed manifests, 122 exports, 117 Node exports, clean consumer install, TypeScript/Vite resolution, and CLI startup
  • independent adversarial re-review at exact head: no P1/P2 findings

This PR does not merge, tag, publish, rerun release workflows, or mutate npm/GitHub Releases.


Compound Engineering
Codex

@terencecho terencecho 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.

Comment (holding stamp — Magi/bot-authored). No new blockers; the design meets the review-and-checks bar the v0.8.11 post-mortem exposed. Flagging one operational risk and a small extraction observation. Only stamping on Terence's explicit go.

SSOT roster

  • Manifest at scripts/release-packages.ts:26-58 (PUBLISHABLE_PACKAGES: readonly PublishablePackage[]), typed {workspacePath, workspaceName, npmName, publishMode}.
  • Consumed by scripts/set-version.ts:23+77 (via validatePublishablePackages, used for both updatePackageVersions and releaseAllowedPaths).
  • Consumed by workflow via scripts/publish-packages.ts:71 (validatePublishablePackagesrunPublishPackages); publish.yml:145 calls it as node --import tsx scripts/publish-packages.ts — no roster in the yml.
  • Old lists deleted: scripts/set-version.ts PACKAGES literal removed (diff lines 23-37); publish.yml 60 lines of publish_pkg calls removed. publish-workflow.test.mjs:117-119 pins this: assert.doesNotMatch(workflow, /@hyperframes\//) + /publish_pkg|packages\/cli/.
  • Validation against packages/*: release-packages.ts:169-181discoverWorkspacePackages walks the root workspaces glob, and reconciliation throws Public workspace X is missing from the publish roster when a non-private packages/* entry is not in the roster (and inversely rejects private entries). Fail loud, before any npm boundary. Locks in the exact silent-drift class that let v0.8.11 lose a package.

Prepublish gate

  • Valid approval (stable-release-guard.mjs:117-149): dedupes by lowercased reviewer login keeping latest by (submitted_at, id), filters state==='APPROVED' && commit_id===headSha && login!==authorLogin, requires Math.max(1, requiredCount).
    • At-head SHA: YES (commit_id === headSha, PR head, not merge SHA — correct because reviews carry the pre-merge head).
    • Author excluded: YES (case-insensitive).
    • Post-force-push dismissal (require_last_push_approval): effectively enforced by commit_id===headSha since dismissed approvals retain their old commit_id. Explicit assertion not needed.
    • Floor of 1: even if branch protection sets required_approving_review_count: 0, guard requires ≥1. This is the specific gap #3440 fell through.
  • Required-check enumeration (stable-release-guard.mjs:165-206): from GET /repos/{repo}/rules/branches/main (effective rulesets API), NOT hardcoded. Extracts context + integration_id per required check. No drift risk.
  • Latest-attempt semantics (stable-release-guard.mjs:217-232, evaluateRequiredChecks:236-278): pulls all attempts via /commits/{sha}/check-runs?filter=all paginated, groups by (name, app.id), picks newest by (started_at, completed_at, id). Avoids the raw-stale-runs trap.
  • Bypass detection (stable-release-guard.mjs:210-221, getRuleSuite:378-395): assertRuleSuitePass requires suite.result === "pass" at exact after_sha === mergeSha on refs/heads/main, AND expects exactly one rule suite for the merge SHA. Admin bypass surfaces as result: "bypass" → throws (tested at .test.mjs:344-355). Combined with the ≥1 approval floor, either check alone would have blocked #3440.
  • Hard deadline: DEFAULT_TIMEOUT_MS = 8 * 60 * 1_000 at stable-release-guard.mjs:5. Fail-closed (throws on remaining ≤ 0). Every downstream API call receives a remainingBudget(...) inheriting the same deadline — no request outlives the outer clock.
  • Ordering (publish.yml:89-95 vs 97, 145, 156): Guard stable release step precedes Create release tag, Publish packages, Create GitHub Release. Pinned by publish-workflow.test.mjs:97-107 (indexOf(stableGuard) < indexOf(createReleaseTag) and < indexOf(publishPackages), plus asserts continue-on-error === undefined).
  • Fail-closed on API error: if (!response.ok) throw at line 341, AbortSignal.timeout on every request, top-level catch sets process.exitCode = 1 at line 434. evaluateRequiredChecks malformed → kind: "api-failure" → the loop's outcome.kind !== "pending" guard rethrows.

Test coverage

  • Mutation-sensitive: release-packages.test.ts:59-71 filters @hyperframes/sdk out of the roster and asserts public workspace ... missing. publish-packages.test.ts:95-116 asserts zero npm boundary calls fire when the roster is short one workspace.
  • Check states: green/neutral/skipped/pending/queued/missing/self-reference/failure/cancelled/timed_out/action_required/stale/startup_failure all covered (stable-release-guard.test.mjs:151-230). Latest-attempt out-of-order-by-id verified (:191-200).
  • Approval states: none / author-only / old-head / DISMISSED-supersedes-APPROVED / CHANGES_REQUESTED-supersedes-APPROVED (stable-release-guard.test.mjs:120-142), plus five malformed-record classes (:144-159).
  • Bypass: rule suite result: "bypass" end-to-end (:344-355); rule suite after_sha/ref/result mismatch (assertRuleSuitePass inline). Immutable-identity mutation matrix at :82-102.
  • Deadline clamping: stable-release-guard.test.mjs:357-393 asserts sleeps == [20, 5] and every request budget <= 25 — no request outlives the deadline.

Reviewed-clause enforcement (Rames' point)

Concur — this PR does close the "reviewed" gap that publish.yml's header comment merely asserted. Two independent gates would each have blocked #3440's zero-review admin-merge: (1) assertRuleSuitePass demands result: "pass" — admin bypass returns "bypass"; (2) collectEffectiveApprovals demands ≥1 non-author approval at exact head. The "reviewed" word is now code-enforced, not comment-enforced.

Findings

Non-blocking

N1 — 8-minute deadline may be too tight on slow-CI days. DEFAULT_TIMEOUT_MS = 8 * 60 * 1_000 (stable-release-guard.mjs:5). Required checks at the merge SHA start when the merge commit lands; on a busy runner day, Test + regression can outrun 8 minutes (the v0.8.11 timeline: merge at 18:49:14, Test finished 18:57:54 — that's 8m40s already). Guard would fail-closed and require a rerun. Safe (fail-closed), but the failure mode is likely enough on real days that I'd bump the budget to ~20-30 min or make it env-configurable. Job timeout-minutes: 10 at publish.yml:20 would also need to grow if you bump the guard budget past ~9 min.

N2 — extracted rule flags without direct assertions. extractEffectiveRules returns requireLastPushApproval, requireExtraApprovalForUnattributedChanges, requireSignedCommits (lines 190-200), but only requiredApprovals + requiredChecks flow into gates; the three flags are only logged (line 300). Not a bug — bypass of any of them would surface as a non-pass rule-suite result and be caught by assertRuleSuitePass. Worth a code comment noting the intentional indirection so a future reader doesn't add a redundant assertion or, worse, drop the indirect coverage.

N3 — NON_CHECK_RULES allowlist requires maintenance. Lines 21-31: unrecognized rule types throw (Unsupported effective repository rule). Fail-closed by design, but GitHub occasionally ships new rule kinds (e.g. code_scanning_check, workflow_run in some previews). If GitHub adds a new type to the main ruleset later, publish breaks until this file is updated. Suggest either (a) a comment listing what's out-of-scope + a runbook step, or (b) softening to warn + continue when the unrecognized rule isn't the required-status-checks / pull_request kinds. I'd keep fail-closed unless breakage recurs, but flag it in a runbook.

Concur (not new)

  • Magi's adversarial-review clean: I ran through immutable-identity, rule-suite bypass, at-head approvals, dedupe of latest check attempts, deadline clamping, ordering — no P1/P2 gaps beyond what the tests already pin. The workflow-level test at publish-workflow.test.mjs:97-107, 117-119 is the load-bearing invariant: it structurally forbids re-introducing the drift-prone hardcoded lists AND forbids reordering the guard past side effects.

CI at head (ecb34c62f82e0d7646db479a3d075b66dd135598)

All required release lanes green: CodeQL, Build, Test, Preflight (lint+format), Lint, Format, Typecheck, Fallow audit, File size check, Skills manifest, and every Perf/Preview/Producer/CLI/Studio/Smoke lane. Only Tests on windows-latest still in progress (not a stable-release-required check).

— Review by tai (pr-review)

@mintlify

mintlify Bot commented Aug 23, 2026

Copy link
Copy Markdown

Preview deployment for your docs. Learn more about Mintlify Previews.

Project Status Preview Updated (UTC)
hyperframes 🟢 Ready View Preview Aug 23, 2026, 8:32 PM

💡 Tip: Enable Workflows to automatically generate PRs for you.

@miguel-heygen

Copy link
Copy Markdown
Collaborator Author

Addressed the operational follow-ups in exact head 4c6e53132e39135c582dcdd36def605145de6bc4:

  • guard deadline now defaults to 25 minutes and accepts only integer STABLE_RELEASE_GUARD_TIMEOUT_MINUTES values from 10–40;
  • publish job timeout is 60 minutes, covering the guard plus build/publish work;
  • source now states that last-push approval, unattributed-change approval, and signed commits are enforced indirectly by the exact main-update rule-suite pass, while contexts/review count remain explicit;
  • release-channel docs include the fail-closed NON_CHECK_RULES maintenance and original merged-PR rerun procedure.

Fresh evidence: focused deadline/workflow contracts 22/22; bun run test:scripts 209 Node + 42 catalog; scripts/core/Studio typechecks; changed-file lint/format/diff/fallow; full monorepo build; all 13 packed manifests, 122 exports, 117 Node exports, consumer install, TypeScript/Vite resolution, and CLI startup. No release workflow was rerun and no tag/npm/GitHub Release mutation occurred.

@jrusso1020 jrusso1020 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Additive review at 4c6e53132e39135c582dcdd36def605145de6bc4. @tai reviewed ecb34c62f, so I have skipped what it covered (SSOT wiring, gate mechanics, the extracted-but-only-logged rule fields, NON_CHECK_RULES maintenance) and spent the time measuring the parts that reading cannot settle. The head moved twice while I worked, so everything below is re-anchored to the current one.

Disclosure: I asked for both of these changes in the release thread, so I re-derived instead of ratifying. That cost me one finding I was ready to file and changed the severity of another (see below).

Strengths, specifically. release-packages.ts:158-172 reconciles in both directions, and the discovered-to-roster leg is the one that actually closes the v0.8.11 gap: a new public workspace now throws instead of being silently skipped at publish time. The validator is called inside runPublishPackages (publish-packages.ts:81), so the roster cannot drift past the npm boundary. The CLI unscoped rename survives and is pinned to exactly one allowed triple (release-packages.ts:141-151), with the manifest now restored in a finally (publish-packages.ts:70) which main never did. The guard runs before tag creation and imports only node: builtins, so it works at a point in the job where node_modules does not exist yet.

Retracted: my main objection is already fixed at this head. At ecb34c62f I had a blocker written up: DEFAULT_TIMEOUT_MS was 8 minutes, and the required contexts on a merge commit have never once gone green that fast. Measured, push to all 8 required contexts terminal-green on main:

merge commit pushed all 8 green elapsed
8612cfd40 12:01:48Z 12:15:47Z 13m59s
dd0626a55 00:04:46Z 00:20:53Z 16m07s
65b2299db 17:18:18Z 17:35:18Z 17m00s
32d58a73e (v0.8.11) 18:49:14Z 19:31:20Z 42m06s

4c6e5313 lands exactly the right fix: 25 minutes by default, bounded 10-40, overridable through vars, and timeout-minutes raised 10 to 60. That last number is the half that is easy to miss, because the post-guard tail measured on the v0.8.11 publish run is about 4 minutes (bun install 12s, build 74s, verify:packed-manifests 42s, publish 100s), so raising only the guard budget would have converted a clean guard error into an opaque job timeout with no ::error:: line at all. Credit to @tai for flagging the deadline first; 25 minutes covers the three clean runs above with headroom.


blocker: rule-suites defaults to a one day lookback, so the recovery path this PR documents expires after 24h

getRuleSuite (stable-release-guard.mjs:421-425) queries rulesets/rule-suites?ref=refs/heads/main with no time_period. Measured against this repo just now:

  • default: 4 suites, oldest 2026-08-22T23:42:41Z
  • time_period=day: identical, 4 suites, same oldest timestamp
  • time_period=hour: 0 suites (v0.8.11's own suite has already aged out)
  • time_period=week and month: reach back to 2026-08-18T01:11:12Z

So the default is day. Once a merge is more than roughly 24h old the query returns nothing for that SHA, matching.length !== 1 throws, and the release can never be completed by re-running the event.

That is load-bearing because re-running is the only sanctioned recovery, and this PR says so itself. docs/contributing/release-channels.mdx:81-82, added in this very commit: "After updating and reviewing the guard, rerun the original merged-PR workflow for recovery. Do not push a stable tag, select a different SHA, or introduce a manual publish path." validate-release-channel.mjs:57 says the same thing, and the npm view skip in publish-packages.ts:82 exists precisely so a re-run can finish a partial publish. Actions keeps a run re-runnable for 30 days; this guard works for one of them.

The new timeout makes this materially more likely rather than less. A 25 minute budget is comfortable for a clean merge and, by the table above, not enough for a flake plus re-run (42 minutes on the release that motivated this PR, and the 40 minute ceiling would not have covered it either). Fail-closed then re-run is the right behaviour, which means re-runs are now a routine part of releasing, and each one carries a silent 24h fuse.

Failure mode is also misleading: Expected one rule suite for main update <sha>, found 0 reads like the merge was never rule-evaluated, so it points an operator at tampering rather than at an elapsed lookback window. Concrete cost of hitting it: set-version has already bumped every manifest on main, so the way out is cutting another patch version.

Fix: &time_period=month on that query. Pagination already handles the extra pages.

important: the roster's real-tree ratchet is load-bearing but accidental, and the tests that advertise it cannot see the real tree

This is the finding I was wrong about, and the corrected version is still worth acting on. I expected no CI check to catch a new public package. There is one, and it is not where anyone would look for it.

Control experiment, packages/newthing/package.json containing {"name": "@hyperframes/newthing", "version": "0.8.11"} added to the real tree, nothing else changed:

  • release-packages.test.ts, whose four tests are named for the roster contract: 4/4 pass
  • set-version.test.ts, named for release options: 2/17 fail, Public workspace @hyperframes/newthing is missing from the publish roster.
  • same directory marked "private": true: 17/17 pass, so the carve-out is correct

Cause: fixture() (release-packages.test.ts:18-32) builds its packages/* tree by iterating PUBLISHABLE_PACKAGES itself, so the denominator is generated from the roster under test and cannot disagree with it. assert.equal(discovered.filter((entry) => !entry.private).length, 13) on line 41 re-asserts the roster's own length, not the repository's. Those four tests prove the function works, which is worth having, but none of them can fail for the reason the repo would fail.

The only real-root reconciliation reachable from a PR is set-version.ts:306, hit transitively through releaseAllowedPaths from set-version.test.ts:141. The early ratchet therefore exists by coincidence of an unrelated test's call graph. Two ordinary changes delete it silently: injecting a root into releaseAllowedPaths, or switching that test to a fixture, which is exactly the pattern the two new test files model. Detection then reverts to release time only, which is the class of silent drift this PR exists to close.

Fix is three lines in release-packages.test.ts asserting validatePublishablePackages(join(import.meta.dirname, "..")) does not throw, so the property is pinned where a reader looks for it. check-workspace-contracts.mjs is the other natural home.

important: a failed publish no longer says why

runPublishPackages catches with an empty binding and logs only ❌ ${npmName}@${version} failed to publish (publish-packages.ts:90-92), and execFileAsync captures stdout and stderr rather than inheriting them, so nothing npm printed reaches the job log. The bash this replaces streamed pnpm and npm output straight through. On this path that is the difference between reading 403 Forbidden, ENEEDAUTH or cannot publish over existing version and having no information at all while some packages are already live on npm.

Same call site, secondary: execFile's default maxBuffer is 1 MB and exceeding it rejects the promise, so a package that actually published can be recorded as failed and fail the job. It self-heals on re-run through the npm view skip, but the red is misleading.

Fix: put the error in the log line, and either raise maxBuffer or use stdio: "inherit" for the publish commands.

nit: the 10 minute floor on the override is below anything that can pass

MIN_TIMEOUT_MINUTES = 10, while the fastest all-8-green in the table above is 13m59s. Any override in 10-19 is a guaranteed timeout that looks like a supported configuration. Consider a floor of 20.


operational consequence worth an explicit decision, not a defect

@tai said the rule-suite check would have blocked #3440. Confirmed at source: the suite for 32d58a73e is id 3788695660, actor_name miguel-heygen, result bypass, so assertRuleSuitePass rejects it.

The part not yet said out loud: 70 of the last 100 pushes to main are bypass, not pass. By actor, miguel-heygen 48 bypass / 6 pass, vanceingalls 22 / 15. This gate is not an emergency brake on the rare admin merge, it sits astride the path releases currently take. And a bypassed merge is permanently unpublishable: the suite result for that SHA never changes, no re-run helps, and the only way forward is a fresh release PR merged without bypass. That may well be the correct policy, it is publish.yml's own "merged, reviewed release PR" finally being enforced, but it is worth choosing knowingly rather than discovering it mid-release.

verified, so nobody needs to redo it

  • Stable publishing is exactly the pull_request path, so the guard's if: covers the whole stable surface: validatePrereleaseTagPush rejects expectedDistTag === "latest" outright on the push path, and validateMergedReleasePr requires both a release/vX.Y.Z head and latest. The v1.0.0-1 shape (hyphen present so it matches the v*-* trigger, no alpha characters so Resolve version computes latest) dies on the dist-tag equality check, prerelease id 1 against resolved tag latest.
  • All 8 required contexts do report on a merge commit, with app.id 15368 matching the ruleset's integration_id, and Semantic PR title arrives skipped, which PASSING_CONCLUSIONS accepts. The check gate is satisfiable in practice, not just in the tests.
  • The guard mirrors the ruleset rather than exceeding it. main currently carries required_approving_review_count: 1 and require_last_push_approval: true, which is what Math.max(1, requiredCount) and commit_id === headSha implement.
  • The denominator is sound. 14 directories under packages/, @hyperframes/sdk-playground the only private one, 13 roster entries, a single packages/* workspace pattern which configuredWorkspaceDirectories supports, and no pnpm-workspace.yaml for the root manifest to diverge from.

CI at this head, for the record

Required Test is currently failing, and it is not this PR's code. Job 97256089989 failed in 4 seconds at its first step, "Require producer source tests", which is a gate that fires when the producer jobs have not succeeded; Producer: integration tests was still in flight and Smoke: global install shows cancelled. This PR touches release scripts only and cannot plausibly affect producer tests, so this reads as an in-flight or cancelled-dependency artifact. Worth a re-run to confirm rather than assuming, since it is a required context and most of the others are still pending.

Verdict: REQUEST CHANGES
Reasoning: The design is right, the roster half is sound, and the timeout objection I arrived with is already fixed at this head. The one blocker left is a single query parameter: as written, the guard cannot verify a release older than 24h, which breaks the re-run recovery this PR's own runbook prescribes, and the new fail-closed timeout makes those re-runs routine.

— Rames Jusso

@miguel-heygen

Copy link
Copy Markdown
Collaborator Author

Final Builder head dcd23fcfca54b315ba27d80d1e07e8c6666ec278 addresses the current review findings:

  • sanctioned rerun recovery requests the month-scoped rule-suite window and still fails closed for absent, ambiguous, bypassed, or non-pass results;
  • a direct real-checkout contract reconciles the actual packages/* tree against the authoritative release roster;
  • aggregate publish failures retain useful sanitized npm stdout/stderr while redacting credentials and user paths;
  • the runbook states that bypass rejection is intentional and operators must merge normally.

Fresh exact-head evidence: focused contracts 33/33; bun run test:scripts 213 Node + 42 catalog; scripts/core/Studio typechecks; lint/format/diff/fallow; full monorepo build; all 13 packed manifests, 122 exports, 117 Node exports, clean consumer install, TypeScript/Vite resolution, and CLI startup. Exact-head CI is terminal at 43 successes, 3 expected skips, 0 failures, and 0 pending.

Builder is not requesting an approval stamp or merging; independent review and fresh final-head reviewer stamps remain with deepwork.

@miguel-heygen

Copy link
Copy Markdown
Collaborator Author

Builder revision 4 is pushed at exact head ddd2cedfa011d90888fe13a45cbda499023c474a.

  • The stable guard alone receives secrets.RELEASE_GUARD_TOKEN; tag/GitHub Release writes retain the built-in token, and prerelease behavior is unchanged.
  • Missing or blank credentials fail before API work. The same client preflights effective rules, month-window rule suites, pull-request reviews, exact-SHA check runs, and commit statuses before semantic policy evaluation.
  • 401, 403, 404, rate-limit, and malformed responses fail closed with endpoint/status/request-ID classification only; credentials, headers, request IDs, and response bodies are not emitted.
  • The runbook specifies a repository-only fine-grained PAT with Administration read-only, Pull requests read-only, Commit statuses read-only, automatic Metadata, named ownership/expiry/rotation, and a mandatory nonpublishing capability proof before merge.

Fresh local evidence: focused guard/workflow 30/30; full scripts 219 Node + 42 catalog; scripts/core/Studio typechecks; full monorepo build; all 13 packed manifests, 122 exports, 117 Node exports, clean consumer install, lint/format/diff, and fallow all pass.

Credential provisioning and the real capability proof remain external pre-merge gates. Builder did not inspect a secret, merge, tag, publish, mutate a release, or run/rerun the Publish workflow.

@jrusso1020 jrusso1020 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Re-review at exact head ddd2cedfa011d90888fe13a45cbda499023c474a. My prior CHANGES_REQUESTED (at 4c6e5313) is resolved — approving. @terencecho reviewed two heads back at ecb34c62f; this is additive to that.

I proposed the roster-SSOT and publish-guard shape on #3440, so I re-derived every finding from the code rather than ratifying my own suggestion.

Blocker resolved — verified at source

getRuleSuite now passes &time_period=month, and so does the new preflight probe. That closes the 24-hour fuse: the endpoint defaults to time_period=day, so a rerun more than a day after the merge would have found zero suites, thrown, and left that release permanently unpublishable — while release-channels.mdx prescribes rerunning the original merged-PR workflow as the only recovery.

It is also pinned against regression, which is what makes it stay fixed: stable-release-guard.test.mjs:547 asserts /time_period=month/ on the outgoing request, and the case above it ("finds an exact passing merge suite older than the default day within the month") reproduces the exact failure.

The two importants, both measured rather than assumed

Roster ratchet is now real. validateRepositoryPublishablePackages() + the new release-packages.test.ts case reconcile the actual packages/* tree, so the contract is enforced in the file named for it rather than by accident through releaseAllowedPathsset-version.ts. Re-ran the same control experiment as before — a real unlisted public packages/newthing/package.json:

before (4c6e5313) now (ddd2cedf)
release-packages.test.ts 4 pass / 0 fail (blind) 4 pass / 1 fail
set-version.test.ts 15 / 2 fail 15 / 2 fail

Failure message is the right one: not ok 1 - directly reconciles the actual repository packages treePublic workspace @hyperframes/newthing is missing from the publish roster. The fixture-generated denominator no longer decides the outcome alone.

Publish diagnostics restored. sanitizePublishError folds message + stdout + stderr, redacts bearer/token/password/secret and home paths, caps at 8 KB, and the aggregate throw now carries per-package detail. maxBuffer raised to 16 MB, so npm's output on a release-critical path no longer risks truncation into an opaque failure.

Merge gate, not a code change

You already call this out ("credential provisioning and the real capability proof remain external pre-merge gates") — adding the measurement so nobody has to guess:

  • RELEASE_GUARD_TOKEN is not a repository secret. The repo has exactly one (CLAWHUB_TOKEN, total_count: 1).
  • It is not an npm-publish environment secret (total_count: 0, real 200 — not a permission error).
  • Organization secrets are 403 to me, so I cannot see them. NPM_TOKEN is likewise absent from both listings, so org level is presumably where release credentials live here — which means this may already be provisioned and I simply can't observe it.

So the check that matters is org-side: confirm the secret exists at whatever scope NPM_TOKEN uses, and record the --preflight run, per release-channels.mdx. Worth doing before merge rather than discovering it on the next release, since the guard correctly fails closed and the first symptom would be a blocked stable publish.

Related: npm-publish carries only branch_policy protection — no required reviewers, no wait timer — so "a protected environment that injects the secret" is weaker here than the runbook implies. And STABLE_RELEASE_GUARD_TIMEOUT_MINUTES is unset at repo and environment level, so the || '25' default is what will actually run. That is the value you want; just noting it is the default path, not configuration.

important — credential lifecycle has no proactive check

The runbook mandates a named owner, an explicit expiry, and rotation. Nothing surfaces an expiry before it bites, and the failure mode is a stable release blocked mid-window with a 401 authentication failure from the guard. You already built the tool for this: --preflight is a read-only, non-publishing capability proof.

Suggest scheduling it (a weekly workflow calling node scripts/stable-release-guard.mjs --preflight against a fixed probe PR/SHA) so expiry, scope drift, or revocation surface on a Monday instead of during a release. Longer term, a GitHub App or an org machine account removes the person-shaped dependency entirely — a user PAT on the release path breaks when that person offboards.

nits

  • .github/workflows/publish.yml:22-26 — this PR added actions: read, checks: read, and pull-requests: read for the guard back when it used secrets.GITHUB_TOKEN. Now that the guard has its own credential, the only remaining GITHUB_TOKEN consumer is "Create GitHub Release" (contents: write), so those three grants are dead. Read-only, so no exposure — but least privilege is this commit's whole point, and publish-workflow.test.mjs already pins credential separation, so it is the natural place to pin the permission set too. (Checked: the one /actions/ hit in the guard is a details_url string match at stable-release-guard.mjs:292, not an API call.)
  • stable-release-guard.mjs:501-506 — the preflight probes /commits/{sha}/status and the runbook mandates "Commit statuses (read-only)", but no code path reads commit statuses. extractEffectiveRules requires Number.isInteger(check.integration_id) and throws Required status check identity is malformed. otherwise, so check-runs-only is a coherent, loudly-fail-closed design. Either drop the probe and that permission, or read statuses so a status-backed required context is handled rather than rejected.
  • stable-release-guard.mjs requestIdClassification — reporting present/absent instead of the x-github-request-id value is deliberate and test-pinned, so treat this as a question rather than a request: a request ID carries no secret, and it is the one value GitHub support asks for when a policy read 403s. Emitting it would cost nothing and makes a release-blocking auth failure diagnosable first time.
  • MIN_TIMEOUT_MINUTES = 10 (carried from my last review, still open) — measured push → all-8-required-green on recent main merges was 13m59s / 16m07s / 17m00s / 42m06s, never under 8 minutes. So a configured 10–19 is a footgun that fails closed for timing reasons rather than policy ones. The 25 default is right; the floor is the part that invites a bad override.

Verified so nobody repeats it

  • Local suites at this head, all green: release-packages 5, publish-packages 4, set-version 17, stable-release-guard 22, publish-workflow 8 — 56 total.
  • publish-workflow.test.mjs needs the yaml dev dependency; a bare node --test in a clone without installed deps fails ERR_MODULE_NOT_FOUND, which reads exactly like a defect and is not one.
  • The job-level if: already gates on merged == true && startsWith(head.ref, 'release/v'), so the new preflight cannot run on a closed-unmerged PR and merge_commit_sha is always present when it does — the ordering concern that reads as a bug isn't one.
  • Required contexts at this head: 7 of 8 green, Tests on windows-latest in progress at ~12 minutes (consistent with the 13–17 minute window above). Test is green here, which confirms the red Test at 4c6e5313 was the in-flight producer-source gate, not this PR.

Thanks for the bypass paragraph in the runbook — a documented "prepare a fresh release PR" recovery is the right answer to a bypass result being permanent, and it matters because most recent main merges take that path today.

Verdict: APPROVE
Reasoning: The blocker is fixed at the source and pinned by a test, both importants are fixed and one is confirmed by re-running the control experiment, and everything left is a nit or an operational follow-up. Merge is still gated on the credential proof the runbook requires — approval here is on code merit, not a merge go.

— Rames Jusso

@miguel-heygen

Copy link
Copy Markdown
Collaborator Author

Builder revision 5 is pushed at exact head d166e1da0d7d66e95cd87fcd071b914acf766803.

  • A dedicated weekly plus workflow_dispatch credential-health workflow discovers a recent merged main PR and verifies effective rules, its exact month-window rule suite, reviews, and check runs.
  • The health workflow has only contents: read, exposes RELEASE_GUARD_TOKEN only to its health step, and contains no push/release trigger or tag/npm/GitHub Release path.
  • Dead actions: read, checks: read, and pull-requests: read grants are removed from the publish workflow; its built-in token retains only existing release-writing/attestation permissions.
  • Commit-status probing and its PAT permission claim are removed. Dynamic required contexts must carry a positive integration ID and remain check-run-only; missing/null/nonpositive identities fail closed.
  • The configured hard-deadline range is now 15–40 minutes with the 25-minute default and 60-minute publish job unchanged.

Fresh local evidence: focused release contracts 44/44; full scripts 224 Node + 42 catalog; scripts/core/Studio typechecks; full monorepo build; all 13 packed manifests, 122 exports, 117 Node exports, clean consumer install, lint/format/diff, and fallow all pass.

No health or Publish workflow was run. Credential provisioning and a live sanitized capability proof remain external merge gates; Builder did not inspect a secret, merge, tag, publish, or mutate a release.

…k-builder/hf-release-publish-ssot-guard

# Conflicts:
#	package.json
@miguel-heygen

Copy link
Copy Markdown
Collaborator Author

Builder revision 6 merged latest main (a562946e11538a87ac2a0766fdad7833b2798696) with a normal two-parent merge commit at exact head 4f0dbba5c3a2507fb6f48ed389c26dcee456c157—no rebase, force-push, or history rewrite.

Read-only git merge-tree --write-tree HEAD FETCH_HEAD predicted exactly one content conflict in package.json:test:scripts. The resolution preserves the branch tests release-packages, publish-packages, and stable-release-guard at the front, plus main's check-large-files in its established position after check-cli-process-ownership. Parent-diff audit found no drift in any other task-touched path.

Fresh post-merge evidence: focused release contracts 44/44; full scripts 228 Node + 42 catalog; scripts/core/Studio typechecks; full monorepo build; all 13 packed manifests, 122 exports, 117 Node exports, clean consumer install, lint/format/diff, and fallow all pass.

Builder did not run the health or Publish workflow, inspect a secret, merge PR #3443, tag, publish, mutate a release, or post to Slack. Fresh Rames/tai review requests remain deferred until independent Reviewer r3.

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.

3 participants