Skip to content

Enforce clean release workflow cache boundaries - #2412

Merged
Chris0Jeky merged 13 commits into
mainfrom
issue-2333/release-cache-contract
Sep 4, 2026
Merged

Enforce clean release workflow cache boundaries#2412
Chris0Jeky merged 13 commits into
mainfrom
issue-2333/release-cache-contract

Conversation

@Chris0Jeky

@Chris0Jeky Chris0Jeky commented Sep 3, 2026

Copy link
Copy Markdown
Owner

Summary

  • Keeps release container builds cache-free and audits every release workflow root through transitive local reusable workflows.
  • Rejects cache actions, unsafe dependency caches, cross-run artifact downloads, missing callees, cycles, and unsupported effective action-key shapes.
  • Repairs the parked parser bypasses across explicit, escaped, anchored, aliased, and tagged YAML keys, flow mappings and flow-sequence mapping pairs.
  • Fixes block-scalar scope and quote-aware comment handling so sibling actions remain structural.
  • Leaves Smart CI in observation mode and does not change release triggers, permissions, tag handling, or artifact publication.

Advances #2333.

Verification

  • Failing-first regressions reproduced the reviewed comment, sequence block-scalar, multiline explicit-key, and alias-key bypasses, plus later flow-entry and flow-sequence boundary variants.
  • PyYAML decoded every hostile fixture to an effective uses: actions/cache@v5 or cross-run actions/download-artifact@v8 mapping before the scanner rejected it.
  • Release workflow contract pair: 101/101 passed with BASH_BIN=C:\Program Files\Git\bin\bash.exe.
  • Smart CI suite: 86/86 passed.
  • Actionlint portable bootstrap boundary: 5/5 passed.
  • GitHub operations governance tests: 27/27 passed.
  • Changed-workflow PyYAML parse, GitHub operations governance, docs governance, golden principles, and diff checks passed.
  • Exact-head hosted R4 is pending.
  • No live tag, GHCR push, release publication, or production mutation was performed.

Review

Fresh repair head: b2fdefc, based on current main 98f3fbd.

First-round review found four direct HIGH false negatives: sequence-item block-scalar scope, aliases used as mapping keys, apostrophes inside plain scalars, and multiline explicit keys. One bounded fix round resolves all four. Pre-push adversarial checks also closed the same boundary class for later flow entries, flow-sequence mapping pairs, anchored keys, and standard YAML tag decorators. A fresh final-head review is pending.

Documentation

No canonical docs changed. This repair corrects the executable release safety contract without changing release sequencing or operator instructions.

Remaining #2333 scope

Alternate .yaml root naming, external reusable workflows, and cache/download commands embedded in shell blocks remain non-blocking follow-up. SC-2 cleanup and any CI-13 spend decision remain human-gated.

CLAUDE_SYNC_PACKET
canonical truth changes: none
issue/project changes: #2333 remains open at Priority II and Project Blocked for separate residual gates; PR #2412 is in final exact-head CI and review
architecture/decision impact: none
manual validation still needed: none; hosted exact-head CI is the remaining automated gate
human actions: SC-2 artifact deletion and any CI-13 spend decision remain owner-gated

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 3, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-03T19:36:40.786493Z 0e95427 New commits
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@Chris0Jeky

Copy link
Copy Markdown
Owner Author

Independent review disposition:

  • Round 1 HIGH: with-before-uses and wider sequence-marker spacing could hide cross-run artifact inputs. Fixed by 5bea139; both regressions now fail closed.
  • Round 2 HIGH: valid YAML anchor syntax such as - &unsafe uses: actions/download-artifact@v8 is not recognized, so a sibling run-id can still select a prior-run artifact without a contract violation.
  • Non-blocking follow-up: alternate .yaml root names, external reusable workflows, and cache/download commands inside shell blocks remain tracked by CI-09: Bounded CI caches and artifact retention with measured utility (storage under the Pro allowance) #2333. None exists in the current release closure.

This PR is parked at the two-round review ceiling and must not merge at head 5bea139. Exact restart: add a failing-first anchored-action regression, make anchored action syntax parse or fail closed, rerun the 83-test release pair plus Smart CI, Actionlint, YAML/governance checks and hosted R4, then enter one fresh bounded review cycle from current main.

@Chris0Jeky

Copy link
Copy Markdown
Owner Author

Review closeout for the fresh repair cycle:

  • Round 1 HIGH reproduced: anchored, escaped, and explicit mapping keys could decode to uses without entering the scanner. The repair at 6d5b798d8a2e27437b26bcbe29c9117e98da62c8 now rejects the block-mapping variants, and the new regression passed.
  • Round 2 HIGH reproduced: this valid YAML flow mapping still bypasses every current guard and scan:
- { ? "\u0075ses" : actions/download-artifact@v8, with: {name: release-input, run-id: 123}}

PyYAML decodes the mapping to uses: actions/download-artifact@v8 and run-id: 123; the release-cache contract reports it as unrecognized. This remains a direct cross-run artifact-promotion bypass.

PR #2412 is parked at the two-round ceiling and must not merge at 6d5b798d8a2e27437b26bcbe29c9117e98da62c8. The PR body currently lost the double quotes around \u0075ses during Windows CLI JSON transport; the fenced fixture above is the exact reproduced input.

Exact restart contract: from current main, add the flow-mapping fixture as failing-first proof; fail closed on unsupported explicit or escaped mapping-key syntax at block and flow mapping boundaries; rerun the release workflow pair, Smart CI suite, Actionlint boundary, YAML and governance checks, and exact-head hosted R4; then enter a new bounded review cycle.

Exact-head local proof before parking: release workflow pair 85/85, Smart CI 86/86, Actionlint boundary 5/5, changed-workflow YAML parse, GitHub operations governance, docs governance, and diff check all passed. Hosted checks are still running and cannot waive this review blocker.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6d5b798d8a

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread scripts/ci/release-cache-contract.test.mjs Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0556440d73

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread scripts/ci/release-cache-contract.test.mjs
Comment thread scripts/ci/release-cache-contract.test.mjs Outdated
Comment thread scripts/ci/release-cache-contract.test.mjs Outdated
@Chris0Jeky

Copy link
Copy Markdown
Owner Author

Independent review disposition, fresh repair cycle round 1:

  • HIGH: sequence-item block-scalar scope could hide a sibling action. Fixed at b2fdefc with an exact regression.
  • HIGH: an alias resolving to uses could hide an action key. Fixed at block, flow-map, later-entry, and flow-sequence boundaries with valid-YAML regressions.
  • HIGH: an apostrophe in a plain scalar could defeat comment stripping. Fixed by recognizing quoted scalars only at scalar boundaries, with the exact regression.
  • HIGH: a multiline explicit key could hide an action. Fixed by rejecting standalone explicit-key indicators, with the exact regression.
  • Pre-push adversarial checks found and fixed the same boundary class for anchored keys, flow-sequence pairs, and standard YAML tag decorators.
  • No other CRITICAL or HIGH finding remained in the pre-push checks.

Final head b2fdefc is now in the second and final review round. Local release contract proof is 101/101.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b2fdefc74f

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread scripts/ci/release-cache-contract.test.mjs Outdated
Comment thread scripts/ci/release-cache-contract.test.mjs Outdated
@Chris0Jeky

Copy link
Copy Markdown
Owner Author

Final review disposition: HIGH, merge-blocking.

scripts/ci/release-cache-contract.test.mjs rejects tagged effective uses keys but readSiblingWithInputs recognizes only bare or quoted with. This valid YAML therefore bypasses the cross-run artifact guard:

- uses: actions/download-artifact@v8
  !!str with:
    name: release-input
    run-id: 123

The parser leaves the action input map empty, so run-id is not rejected even though the workflow can consume an artifact from another run. This is a direct artifact-provenance bypass.

This finding arrived in the second and final bounded review round. Per the repository review ceiling, I am parking this head rather than starting a third fix cascade.

Exact restart contract:

  1. Start from b2fdefc74f17feda3d5422ebd4bd2a9674d1555b reconciled with then-current origin/main.
  2. Add a failing regression for the tagged with fixture above and adjacent valid YAML tag forms at structural mapping-key boundaries.
  3. Fail closed on tag-decorated effective with keys, preferably by rejecting tag-decorated structural keys consistently rather than growing one isolated spelling exception.
  4. Re-run the release workflow pair, Smart CI tests, portable Actionlint contract, GitHub ops/governance/docs/golden checks, YAML parsing, git diff --check, exact-head hosted CI, and one fresh bounded review cycle.
  5. Do not merge this parked head.

[Codex lane release v2]
lane: beta-platform-integrity
issue: #2333
PR: #2412
exact head: b2fdefc
result: parked
shipped outcome: none; the exact head remains unmerged, with release cache disabling and parser hardening preserved on the remote branch
remaining residual: HIGH tagged-effective-with bypass; later-scope .yaml roots, external reusable workflows, and shell-embedded cache/download commands; SC-2 cleanup and CI-13 spend decisions remain human-gated
released paths/leases: .github/workflows/ci-required.yml, .github/workflows/release-container.yml, scripts/ci/release-cache-contract.test.mjs
next dependency now unblocked: none

CLAUDE_SYNC_PACKET
canonical truth changes: none
issue/project changes: PR #2412 remains open, ready, Blocked, Priority II; issue #2333 remains open and Blocked
architecture/decision impact: none
manual validation still needed: none before restart; the restart must obtain exact-head hosted CI and fresh review
human actions: SC-2 artifact cleanup and CI-13 spend decision remain open

… release cache contract

The tag guard only rejected a tag attached to `uses`, so `!!str with:` slipped past the sibling input reader and a `run-id` on actions/download-artifact passed the contract; a tagged input value such as `cache-dependency-path: !!str ''` also read as non-empty. Widen the guard to any mapping key, reject tagged with input values, and accept the block-scalar indentation indicator in either order so an empty `|2` body is not mistaken for a path. Three regressions fail on the previous detector.
@Chris0Jeky

Copy link
Copy Markdown
Owner Author

Review disposition after the parked head b2fdefc74 (coordinator pass, 2026-09-03).

An independent fresh-context verification of the two open Codex P1 threads confirmed both mechanisms as real but rated them non-blocking under the causal severity bar: each needs a repository writer to hand-write obfuscated YAML in a workflow, and that actor can edit the in-tree contract test just as easily; neither is reachable by an ordinary edit. Rather than relitigate the earlier HIGH disposition, the head above applies the two cheap tightenings both reviews agreed on, plus one closing the tagged-value half of the first thread, each with a regression that fails on the previous detector:

  • a YAML tag on any mapping key is rejected, not only on uses;
  • a with input value carrying a tag is rejected;
  • block-scalar headers with an indentation indicator (|2, |2-, |-2, >1+) are parsed, so an empty body cannot satisfy the non-empty cache-dependency-path check.

This is the single permitted reopen for the defect the last round raised; no further review rounds follow. Residual, tracked and non-blocking: with: # comment on a step matches against the raw line and reports a false "with must use a block mapping" violation (fail-closed, confusing rather than unsafe). Merge after the aging window once ci-required is green on this exact head.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0e95427b86

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread scripts/ci/release-cache-contract.test.mjs
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

@Chris0Jeky

Copy link
Copy Markdown
Owner Author

[Claude lane claim v2 update]
lane: beta-platform-integrity
issue: #2333
PR: #2412
base: 350f595
exact head: c77ec82

requalification (all run from the merged head, worktree root):

  • BASH_BIN=C:\Program Files\Git\bin\bash.exe node --test scripts/ci/release-cache-contract.test.mjs — 28/28 passed
  • BASH_BIN=... node --test scripts/ci/release-cache-contract.test.mjs scripts/ci/release-desktop-dispatch.test.mjs (the release workflow contract pair) — 104/104 passed (was 101/101; main added 3 desktop-dispatch tests)
  • node --test scripts/ci/smart-ci/*.test.mjs — 89/89 passed (was 86/86; main's Decouple required E2E from Windows frontend unit #2427 added required-e2e-dependencies.test.mjs)
  • node --test scripts/ci/actionlint-bootstrap.test.mjs — 5/7 passed; the 2 failures are ACTIONLINT_BIN must point to the installed tool (binary not installed locally), i.e. the same 5/5 portable bootstrap boundary as before. That file is unchanged by this PR.
  • node scripts/check-github-ops-governance.mjs — passed
  • node scripts/check-docs-governance.mjs — passed
  • PyYAML parse of .github/workflows/ci-required.yml and .github/workflows/release-container.yml — both parsed ok
  • git diff --check — clean

effective diff: three files (.github/workflows/ci-required.yml, .github/workflows/release-container.yml, scripts/ci/release-cache-contract.test.mjs); ci-required.yml merge resolution: none needed — git merge origin/main was conflict-free, and the merged file keeps both main's removal of frontend-unit from e2e-smoke.needs and this PR's added Validate release cache trust contract step.

reopen fix (b2fdefc..0e95427, 63 insertions in the contract test): (1) the tagged-key regex now rejects a YAML tag on any mapping key rather than only uses-shaped keys; (2) a with: input value beginning with ! is rejected outright; (3) block-scalar headers with an indentation indicator (|2, |2-, |-2, >1+) are now recognised in both the line annotator and the with-value reader, so they cannot read as a non-empty scalar. The exact hostile fixture from the parking comment (!!str with: under uses: actions/download-artifact@v8 carrying run-id) is covered by the regression tagged with keys cannot hide cross-run artifact inputs; the other two are tagged with input values cannot satisfy the cache-dependency-path contract and block scalars with indentation indicators are parsed before the cache-dependency-path check.

status: ready for exact-head CI, one scoped fresh-context review of the reopen diff, aging window, and coordinator merge

@Chris0Jeky

Copy link
Copy Markdown
Owner Author

Fresh-context independent review of the reopen diff (b2fdefc -> 0e95427, contract test only), read against the requalified head c77ec82 with current main merged.

Verdict: no CRITICAL or HIGH finding. Merge-blocking: none.

What was checked:

  • The parked HIGH (tag-decorated with key hiding a cross-run run-id) is closed for every spelling constructed: !!str "with":, !!str 'with':, verbatim !<tag:yaml.org,2002:str> with:, explicit ? with keys, anchored &a with:, flow-mapping with: {run-id: 1}, aliased with: *a, and a tagged run-id under a bare with: are all rejected fail-closed by the scanner's key-tag, anchor, explicit-key, or block-mapping guards.
  • No false rejection on the real workflow tree: no indentation-indicator block-scalar headers exist, and every ! in .github/workflows is inside a run/tags block scalar or an expression that cannot satisfy the structural key regex.
  • No scope creep: the diff is exactly the block-scalar header regex, the key-alternation generalization, the tag-value guard, the block-scalar value regex, and three regressions.

Tracked (LOW, pre-existing, not introduced by this PR; recorded on #2333 rather than fixed here):

  • A step supplying inputs via a YAML merge key (<<: *defaults) has no with: sibling, so run-id would be invisible to the guard. GitHub Actions does not evaluate anchors or merge keys in workflow files, so the construct is not executable; a fail-closed <<: rejection next to the anchor/alias/tag guards would still be tidier.
  • cache-dependency-path: written as a block sequence reads as empty and false-rejects. No occurrence on this tree.

Requalification evidence at the head is in the previous comment. Remaining gate: exact-head hosted ci-required green and the three-minute aging floor, then a merge commit.

@Chris0Jeky
Chris0Jeky merged commit 57eb1c8 into main Sep 4, 2026
35 checks passed
@github-project-automation github-project-automation Bot moved this from Blocked to Done in Taskdeck Execution Sep 4, 2026
@Chris0Jeky
Chris0Jeky deleted the issue-2333/release-cache-contract branch September 6, 2026 02:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

1 participant