fix: autorelease unattended hardening - #56
Merged
Conversation
Recovery is a pure function of the release list and the event store, with no bound, so suppressing the model call let a repair that stays blocked starve reconciliation, lifecycle and selection on every later run. The trigger label still reports the missing record, but modelCall keeps the value the rest of the chain produced. A record filename already occupied by an unrelated document is declined outright: the filer refuses to overwrite a file, so the decision would otherwise request the same failing repair forever.
The assets and the SHA256SUMS of a hand-made release prove each other and nothing else, so the route now verifies the release attestation exactly as the publish job does and requires the tagged commit to be reachable from main before any record is filed. Recovery also moves out of the dispatch chain into its own step that is allowed to fail, so a blocked repair no longer withholds the reconciliation, lifecycle and selection paths of the same run; the failure is re-raised after them.
Recovery used to switch this checkout onto its own branch, which forced the dispatch step to switch it back and so discarded the unstaged evidence file the no-change step had just written: the commit died with nothing to commit, the evidence never advanced, and the resulting failure hid the real recovery error. The record is now committed from a separate worktree, the compensating checkout is gone, and the no-change branch is cut from a freshly fetched origin/main so it commits whether recovery ran, succeeded or failed. The EOL completion files on the same branch name in the same run, so a repair that fails after its push now closes the pull request it opened and deletes the remote branch before exiting, leaving that namespace clean. The exit trap returns the original status so cleanup cannot mask the failure. The re-raise step gains always() because a plain condition carries an implicit success(), which let a later failing step swallow the recovery diagnostic.
Without --repo, gh also deletes the local branch after a squash merge, and git refuses while the recovery worktree still holds it. gh wraps that error and exits non-zero, so the step aborted after the merge had already landed: the merged output was never written, the success notification never sent, a working run raised a critical failure, and a same-run EOL completion proceeded against a base that had moved. The close call in the cleanup trap names the repository for the same reason.
The watcher dispatch was a 168-line if/elif that exited 0 on any action combination it did not recognise. route_watch_action() in control.py now enumerates every legal shape, including the ones that legitimately do nothing, and raises otherwise; recovery keeps its own recoveryRoute field because it is an overlay on the admitted plan rather than a competing branch. The operator pause gate was inlined in three shapes across four workflows while control.mutation_allowed() was unreachable; it is now the operator-gate subcommand, with --require-enabled for the sites that must fail the job. The action-key-to-filename mapping had eight copies; it is now action-filename. Also right-sizes the coordinate job timeout from 5 to 40 minutes, since a run can complete two full PR cycles and dispatch-pr-checks waits 900s in each, and tightens the recovery re-raise condition from always() to !cancelled().
route_watch_action raised on watch_decision's own output: every record_missing run carries action record_completed_event, which no row matched, so composing the two functions hit the raise. Only the workflow's use of the plan action kept that latent. The recovery action now has its own row, and the recovery key is read from either the separate argument the workflow passes or the decision's own actionKey, so both conventions answer the same. The a18 guard asserted the operator-gate subcommand appeared, not that it was armed. Without --require-enabled the subcommand prints the state and exits 0, so stripping the flag from every publish site left a18 green while a paused operator would still have released. a18 now asserts the invocation: the publish and implement sites must carry the flag, the watcher sites must not and must test the reported state, keeping the two shapes mutually exclusive.
autorelease-event, policy-invariants, and support-policy were never wired
to a validator or a workflow. The only schemas that carry weight are the
ones a workflow names via --output-schema, which is exactly how
scripts/validate-structured-output-schemas discovers them, so these three
could not be reached even in principle.
Zero-reference proof (the only hits are each file's own "$id" line and the
plan document describing this deletion):
$ grep -rn "autorelease-event.schema.json" --exclude-dir=.git --exclude-dir=.build .
docs/superpowers/plans/2026-08-03-autorelease-unattended-hardening.md:674:...
schemas/autorelease-event.schema.json:3: "$id": ".../autorelease-event.schema.json",
$ grep -rn "policy-invariants.schema.json" --exclude-dir=.git --exclude-dir=.build .
docs/superpowers/plans/2026-08-03-autorelease-unattended-hardening.md:674:...
schemas/policy-invariants.schema.json:3: "$id": ".../policy-invariants.schema.json",
$ grep -rn "support-policy.schema.json" --exclude-dir=.git --exclude-dir=.build .
docs/superpowers/plans/2026-08-03-autorelease-unattended-hardening.md:674:...
schemas/support-policy.schema.json:3: "$id": ".../support-policy.schema.json",
The plan schema's completionAssessment fragment and the standalone assessment schema describe the same document, but the standalone copy let an agent return zero criteria or empty criterion ids. Deterministic admission (validate_completion_assessment) already rejects both, so the schema was strictly looser than the code it feeds. The remaining differences are phase-specific on purpose and stay: the plan fragment fixes the four investigation criterion ids and constrains evidence to plan-internal references, while implementation and repair assessments answer whatever completionCriteria their task contract declares.
The evidence manifest digest formula existed twice: capture_evidence wrote
it and validate_recaptured_evidence recomputed it from its own copy of the
projected fields. A one-sided edit would have made every prior manifest
unverifiable, so the projection now lives in manifest_digest().
The 40-hex commit SHA pattern was re-spelled inline at three call sites
next to the COMMIT_SHA_RE that already held it.
retry_decision and audit_reconstruction stay: the brief allowed deleting
them only if nothing outside tests calls them, and autorelease/verify.py
does, at checks A06 and A19.
$ grep -rn "retry_decision\|audit_reconstruction" --exclude-dir=.git --exclude-dir=.build .
autorelease/control.py:...
autorelease/verify.py:...
tests/test_autorelease.py:...
Both now carry a docblock naming that caller, so the next sweep does not
have to rediscover it.
Every workflow now declares `defaults: run: shell: bash`. GitHub's implicit
shell is `bash -e {0}`, which does not set pipefail, so a failing producer
in a pipeline was silently reported as success. Each pipeline was walked
before the switch: the shasum-into-awk substitutions and the gh-content
into base64 decodes only get stricter, and the one place that deliberately
inspects a producer's status (implement.yml's authoritative checks) already
reads PIPESTATUS under `set +e`.
Job-level `permissions:` blocks exist only to narrow the workflow-level
grant. Four re-declared it exactly (e2e agent-canary, implement validate,
repair, and validate-repair), so they read as intent while changing
nothing. Publish's preflight had the opposite problem: it validates
dispatch inputs and never checks out, but silently inherited the release
job's write-everything grant. It is now `contents: read`.
The reviewed autorelease-e2e.yml digest in .github/autorelease-pins.json
moves with the file, which is the pin's whole purpose.
The two exemptions that let a bot merge a protected control path without a human review checked only the protected subset of the diff. A PR that changed autorelease-state/last-evidence.json plus any unprotected file still matched, because the unprotected file never entered `protected`. Both writers commit exactly one path onto a branch cut fresh from origin/main (watch.yml's evidence and recovery records, publish.yml's event record), so requiring the whole diff to be that one file costs the trusted path nothing and denies a passenger commit the ride. The inline action-key filename mapping and both branch regexes are unchanged: they are the parts an attacker would want moved.
The assertion that every `gh pr merge|close` in the recovery step passes --repo scanned single lines. A call wrapped after `gh pr` matched nothing, so the loop silently asserted about zero calls, and a call whose --repo sat on a continuation line failed for no reason. Folding continuations first fixes both directions, and pinning the call count keeps a call that disappears entirely from passing by absence. Probed against four shapes: verb wrapped without --repo (previously invisible, now caught), arguments wrapped without --repo, arguments wrapped with --repo on the second line (previously a false failure, now passes), and the single-line form in the file today.
Claim under review: the EXIT trap misses SIGTERM, so a cancelled job can
orphan the pushed branch and its PR.
Probed against bash 5.3 with a fixture holding the same trap shape:
trap cleanup EXIT SIGTERM -> cleanup x1, exit 143
trap cleanup EXIT SIGINT -> cleanup x1
trap cleanup EXIT INT TERM SIGTERM -> cleanup x2, exit 0
trap cleanup EXIT INT TERM SIGINT -> cleanup x1
Bash already runs the EXIT trap when the shell dies from a signal, so the
withdraw path is armed. Adding INT and TERM would withdraw twice and, worse,
report a cancelled step as successful, because the signal trap enters with
$? of 0 and hands that to the EXIT trap.
The residual gap is SIGKILL after the runner's grace period, which no trap
can close. It is accepted: the orphaned branch and PR both carry this run
id, so a later run never collides with them, and nothing merges an event
record PR on its own.
The check ran ripgrep when it was installed and grep otherwise, and the two branches did not look at the same files: ripgrep honoured ignore rules and skipped nothing hidden but .git, while grep read build output and every untracked local file. Which tool a runner happened to have installed decided what the gate covered. git ls-files is one scope on every machine. Untracked files are no longer scanned, which is the intended trade: CI checks out a clean tree, and a new file joins the gate when it joins the repository. xargs returns 123 when any grep batch matches nothing, so the finding is read from the output instead of the exit status. This file is in mise-php/autorelease/shared-files.json and lands byte for byte identical in both repositories.
scripts/test.sh packaged a fixture into $PROJECT_ROOT/.artifacts and then deleted the three paths it knew about. It runs inside checkouts that autorelease inspects for an exact tree, so anything the cleanup missed reads as an unsealed edit. The output directory is now scratch space, removed by a trap rather than by name. package.sh keeps .artifacts as its default because the build and release workflows read it from the working tree on purpose.
The snapshot reimplemented canonical JSON and the sha256 prefix that autorelease/control.py already defines. Two spellings of one digest formula is one edit away from a snapshot nobody can verify against the control plane. Verified the replacement produces identical bytes for the same input.
serve_forever() ran with no handler, so the release job's kill left the process to die on the default disposition: exit 143, no server_close, and the listening socket released only when the process was reaped. SIGTERM and SIGINT now stop the loop and close the server. shutdown() waits for serve_forever() to return and the handler runs on that very thread, so it is called from another one. Probed three times: the process exits 0 and the port is immediately reusable.
lib.sh disabled SC2034 for the whole file, which would have hidden the next unused variable too. Only PROJECT_ROOT is unused within the file, since the scripts that source it read it, so the disable sits on that line. CI checked scripts/*.sh and silently skipped the two extensionless bash scripts. They are named explicitly because every other extensionless script under scripts/ is Python, which shellcheck cannot parse. Both were already clean at -S warning.
php-bin files an event record under a name derived from the action key and mise-php reads that record back by the same derivation. Nothing compared the two mappings, so a one-sided edit would have left one repository waiting forever on a file the other never wrote, and no check would have said so. A09 already covers cross-repository coordination, so the comparison lives there. It runs mise-php's own entry point over every action key form both alphabets admit, rather than diffing source text, so a differently written mapping that behaves identically still passes. The single asymmetry is asserted too: mise-php's alphabet excludes no_change on purpose, because a quiet run files no record for it to read. Probed by changing mise-php's separator to "_": A09 fails with "mise-php names new_patch:8.5.9 new_patch_8.5.9.json, php-bin names it new_patch-8.5.9.json". mise-php was restored.
A cancelled job is signalled twice: SIGINT, then SIGTERM once the grace window
closes. Only EXIT was trapped, so the second signal kept its default disposition
and killed the withdraw partway through, leaving exactly the open bot PR and
live remote branch the trap exists to prevent. Ignoring INT and TERM inside
cleanup gives the withdraw the rest of the grace window.
Probed on bash 5.3.9 and 3.2.57, signalling the whole process group as the
runner does. Before, with the escalation arriving one second into cleanup:
rc=-15 log=['working', 'cleanup-start']
The withdraw never reached its second step. After:
rc=-2 log=['working', 'cleanup-start', 'cleanup-mid', 'cleanup-end']
Cleanup runs once, finishes, and bash re-raises the signal afterwards so the
cancelled exit code survives. A plain failure exit is unchanged at rc 7 with the
full cleanup log in both variants.
`git ls-files -z | xargs -0 grep ... || true` needed the tolerance for xargs
exit 123, which grep produces whenever a batch matches nothing, but the same
tolerance covered a failing listing. Run outside a repository the check printed
"fatal: not a git repository" and then "Public-language check passed." with exit
0, having scanned nothing.
The listing is now produced and checked before the grep runs, in a file because
command substitution drops the NUL separators.
Probes, in order: outside a repository, a clean repository, a tracked violation,
a repository with no tracked files.
$ ./scripts/check-public-language.sh # not a repository
fatal: not a git repository (or any of the parent directories): .git
Public-language check could not list the tracked files of /tmp/probe/nogit.
rc=1
$ ./scripts/check-public-language.sh # clean
Public-language check passed.
rc=0
$ ./scripts/check-public-language.sh # tracked violation
doc.txt:1:a HERD of cows
Public-language check failed.
rc=1
$ ./scripts/check-public-language.sh # empty repository
Public-language check found no tracked files in /tmp/probe/empty.
rc=1
The same file before this change answered rc 0 with "passed" to the first probe.
This path is in autorelease/shared-files.json, so the file is byte-identical to
mise-php's copy: sha256
280d32fb10e58baa0b6b364f6e5cc1d122528a8cb7a0f4aa4e504eeb0ba76ea8.
control.py had grown to ~1,470 lines across six unrelated concerns. The code moves verbatim into four private modules and control.py becomes the facade the workflows, scripts, verifier, and tests already import, so every existing public name is still importable as autorelease.control.<name>. The file is both a script and a package module, so direct execution (./autorelease/control.py <command>) borrows the same repository-root sys.path shim the scripts use; relative imports would have broken it and bare imports would have broken the package consumers. Two lines are not verbatim: capture_evidence now takes its source set instead of defaulting to it, and main() supplies EVIDENCE_SOURCES. The registry of authoritative sources is a reviewed decision, so it stays in the control surface that verify.py check A11 reads, while the fetch client moves out.
validate_plan ran ~175 lines of unrelated rejections in one body. The rejections move unchanged into _validate_plan_shape, _validate_plan_preconditions, and _validate_plan_actions, and validate_plan now names the order they run in, which matters because each gate reads what the previous one proved.
A04 and A11 asserted the absence of BeautifulSoup, support_table_to_events, and classify_php_release by grepping autorelease/control.py alone. Since the control module was split into a package, that file is a 256-line facade, so a parser added to _evidence.py satisfied both checks. Both now read every module in the package through one helper, excluding verify.py itself because it names the markers to assert them absent. A11 keeps reading control.py for the positive half: the authoritative source registry still has to live on the facade. Also tighten the recovery --repo matcher: gh pr required exactly one space, and the continuation fold collapsed across blank lines.
The supported-branch range was hardcoded as "8.2 through 8.5" and the status section listed exact release tags, neither of which anything regenerates. Both now point at the generated policy and the releases page. docs/release-process.md described a human tagging a release and a workflow that rebuilds from the tag. No such trigger exists: autorelease-publish.yml is workflow_dispatch only, so a hand-pushed tag published nothing. It now describes the automated transaction, and keeps the human steps to the recipe change that feeds it. AUTORELEASE.md pointed at docs/autorelease-verification.md, which is not a checked-in file; the verifier writes it into its --output directory. It also now records two invariants the gates already enforce: harness code is protected while product code stays agent-admissible, and validation runs repo scripts at the sealed model commit precisely because protected paths cannot be in the patch. A new "Unattended lifecycle" section states that any major or minor needs zero human input, and that EOL delists a branch without making any published release less installable. Also correct the admin-state snapshot filenames in both repos to the -after convention actually committed, and drop the claim that PHP 8 anchored validators need review; none remain.
The consumer workflow in mise-php was corrected to record passed=true as its last step, but php-bin's validate job kept the original shape: it set the output inside the check step, before the bundle step and both uploads. The merge job gates on that output and then downloads the validated artifact, so a bundle or upload failing after the output was set advertised a patch that is missing or incomplete. Merge dies loudly when it reads the absent validation.json, but the two repositories disagreed about an invariant one of them now documents. The check step reports a separate status output that gates the bundle and both uploads, and passed=true is recorded last. A failed run leaves passed unset, which merge and repair already read as not passed. validate-repair needs no equivalent: merge gates on that job's own result and its upload is already the last step.
The A04 and A11 marker scan globbed autorelease/*.py, which covers the package as it is today but would miss autorelease/parsers/foo.py. Sub-packaging is exactly the move that made this scan necessary, so the hole it leaves is the one already found once. rglob closes it; the module-name assertion below still holds because it tests names, not paths. Verified by adding a nested autorelease/parsers/lifecycle.py naming the markers: A04 and A11 both fail, and both pass again once it is removed.
release-process.md and README.md still told the reader to "use" a rebuild revision such as 8.4.5-1. Nobody can: the revision is a field of the admitted recipe_rebuild:<version>:<n> action key, proposed by the plan and validated at admission. Leaving the imperative reinstated a human step three lines after the same document says a hand-pushed tag publishes nothing. Also correct the version named in the "still installs" promise. Both AUTORELEASE.md copies used 8.2.29, which was never published; the branch's published tag is 8.2.32. Split the verification-artifact sentence so "it" has one antecedent, and keep the protected-versus-admissible list to this repository's own paths, pointing at mise-php's AUTORELEASE.md for the consumer side rather than duplicating a list that nothing keeps in sync.
…y merge A recovery merge moves main inside the coordinate job, and the no-change evidence commit asserts an untouched base exactly as the EOL completion does. The trusted-automation exemption is anchored to the run head SHA, so after a recovery the evidence PR no longer matches it, the admission assert fails the job, and the next scheduled run sees the same manifest delta and repeats the failure. Defer the evidence record to that next run instead, which re-derives everything from the moved main.
The unrecorded-release lookup was computed before the branches that decide whether it may be used at all, against the guard-first shape of the rest of the decision. Behaviour is unchanged: an unhealthy capture already yielded an empty release list.
.github/dependabot.yml is on mise-php's shared-file manifest and is protected and code-owned there, but php-bin's patterns fell through it entirely. Any change to it on php-bin's main breaks byte parity and hard-fails mise-php's consumer on the first step of every scheduled run, and only a human can re-sync the protected copy. A09 now asserts every manifest path is protected in both repositories, using mise-php's own admission module for its half, so the asymmetry is a gate failure rather than a review miss.
The plan narrates gate weaknesses, verification shortcuts, and the trust boundary in detail, which is permanent disclosure and permanent noise in a public release-artifact repository. It stays with the rest of the SDD material outside the tree.
The date, attempt, and evidence suffixes of an action key are model-chosen, and the EOL completion step hard-asserts the record file its earlier run opened. Re-deriving a different suffix leaves the original event incomplete forever, so the investigation phase now reuses an incomplete record's key.
…ed event The recovery record is assembled by four jq programs in the watcher and was only judged by the protected-controls evaluator at merge time, so a field drifting out of one of them wedged a live run instead of failing a test. The programs are asserted to still be the workflow's own text, then run for real through the same three transitions and fed to validate_completed_event_record.
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_13a97800-9632-4167-9530-91a5ebc818b8) |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis change adds deterministic autorelease admission, evidence, state, recovery, and workflow controls. It centralizes validation and routing, expands recovery handling, strengthens protected workflow checks, broadens PHP version support, and updates related scripts, tests, documentation, and repository policy. ChangesAutorelease hardening
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Watcher
participant Control
participant Evidence
participant GitHub
participant Release
Watcher->>Control: compute watch action and action key
Control->>Evidence: validate captures and retained records
Control->>GitHub: dispatch implementation, publish, recovery, or EOL action
GitHub->>Release: validate, publish, or reconstruct release event
Release-->>GitHub: transaction state and event record
GitHub-->>Watcher: notification or deferred recovery result
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (4)
autorelease/_validation.py (1)
127-134: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse case-sensitive matching so protection is platform-independent.
fnmatch.fnmatchappliesos.path.normcase. Matching is therefore case-insensitive on macOS and case-sensitive on Linux..github/workflows/protected-controls.ymlline 123 evaluates the sameprotected-paths.jsonmanifest withfnmatch.fnmatchcase, so the CI gate and this local gate can disagree about the same path.path_is_alloweddrifts in the permissive direction on macOS.Switch both helpers to
fnmatch.fnmatchcase.♻️ Proposed fix
def path_is_protected(path: str) -> bool: normalized = pathlib.PurePosixPath(path).as_posix() - return any(fnmatch.fnmatch(normalized, pattern) for pattern in PROTECTED_PATTERNS) + return any(fnmatch.fnmatchcase(normalized, pattern) for pattern in PROTECTED_PATTERNS) def path_is_allowed(path: str, patterns: Iterable[str]) -> bool: normalized = pathlib.PurePosixPath(path).as_posix() - return any(fnmatch.fnmatch(normalized, pattern) for pattern in patterns) + return any(fnmatch.fnmatchcase(normalized, pattern) for pattern in patterns)
autorelease/_admission.pyline 410 usesfnmatch.fnmatchfor the same purpose and needs the same change.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@autorelease/_validation.py` around lines 127 - 134, Update path_is_protected and path_is_allowed in _validation.py to use fnmatch.fnmatchcase instead of fnmatch.fnmatch for platform-independent, case-sensitive matching. Also update the matching logic in _admission.py around its visible fnmatch usage to use fnmatchcase consistently..github/workflows/ci.yml (1)
30-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSelect the extensionless bash scripts by shebang.
The explicit list is correct today. It does not cover a bash script added later, and the miss is silent. Select by shebang so the gate stays complete.
♻️ Proposed fix
- name: Check shell scripts - # The extensionless bash scripts are named one by one: every other - # extensionless script under scripts/ is Python, which shellcheck cannot read. - run: shellcheck scripts/*.sh scripts/assert-admission-checks scripts/dispatch-pr-checks + # Extensionless scripts under scripts/ are a mix of bash and Python, so the + # bash ones are selected by shebang; shellcheck cannot read the Python ones. + run: | + mapfile -t bash_scripts < <(grep -rlE '^#!.*\bbash\b' scripts | sort) + shellcheck "${bash_scripts[@]}"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 30 - 33, Update the “Check shell scripts” step in the CI workflow to discover extensionless scripts under scripts/ by their bash shebang instead of maintaining an explicit filename list. Keep the existing .sh coverage and ensure newly added shebang-based bash scripts are passed to shellcheck automatically.autorelease/_evidence.py (1)
218-228: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winPin the evidence source scheme to https.
The URL scheme is never asserted before the fetch.
RestrictedRedirectrequires https only for redirect targets, so an initialhttp:orfile:source would still be fetched and digested as authoritative evidence. The sources are reviewed constants today, which makes this a posture gap rather than an exploit, but the assertion belongs at the boundary that trusts the bytes.🔒️ Proposed fix
for source in sources: + parsed = urllib.parse.urlparse(source.url) + require(parsed.scheme == "https" and bool(parsed.hostname), f"unsafe evidence source: {source.capture_id}") headers = { "Accept": "application/vnd.github+json, application/json, text/html", "User-Agent": "bigpixelrocket-autorelease/1", } - if token and urllib.parse.urlparse(source.url).hostname == "api.github.com": + if token and parsed.hostname == "api.github.com": headers["Authorization"] = f"Bearer {token}"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@autorelease/_evidence.py` around lines 218 - 228, Validate each source URL’s scheme before constructing the request in the evidence-fetch loop, accepting only https and rejecting http, file, or any other scheme. Use the existing source URL handling around urllib.parse.urlparse and ensure invalid schemes cannot be fetched or treated as authoritative evidence.Source: Linters/SAST tools
.github/workflows/autorelease-watch.yml (1)
587-595: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valuePass the record action key through
envinstead of interpolating it into the script.Line 594 expands
${{ needs.investigate.outputs.record_action_key }}directly into the shell body. The recovery step at lines 196-198 already uses the saferenvform for the same value. The key is constrained byACTION_KEY_REtoday, so this is hygiene rather than an active injection, and the change also removes the zizmortemplate-injectionfinding.♻️ Proposed change
- name: Report an unrecovered event record if: ${{ !cancelled() && steps.recover.outcome == 'failure' }} + env: + RECORD_ACTION_KEY: ${{ needs.investigate.outputs.record_action_key }} run: | - echo "Recovering the event record for ${{ needs.investigate.outputs.record_action_key }} failed." >&2 + echo "Recovering the event record for $RECORD_ACTION_KEY failed." >&2 exit 1🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/autorelease-watch.yml around lines 587 - 595, Update the “Report an unrecovered event record” step to pass needs.investigate.outputs.record_action_key through the step’s env configuration, then reference the environment variable in the shell command instead of interpolating the GitHub expression directly. Follow the existing env-based pattern used by the recovery step.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/autorelease-publish.yml:
- Around line 298-317: Update the release transaction workflow around
transaction-state recording to persist an intermediate publishing state before
the GitHub release becomes public, and reconcile the GitHub release state
afterward before determining whether the operation completed. Ensure a live
release with transaction.json still at draft_verified is classified as
recoverable with a warning rather than a critical failure, while preserving the
existing published/public_verified/complete success states.
In `@autorelease/_admission.py`:
- Around line 559-575: Update the patch serialization around tracked and
untracked diff collection to preserve raw bytes: avoid text-mode
decoding/newline translation in the relevant git/subprocess calls, combine byte
outputs, and write them with a binary file operation instead of write_text. Keep
the existing patch size validation and digest flow unchanged.
- Around line 608-612: Update verify_merge to validate every check named in
REQUIRED_PLAN_CHECKS has a "success" status, while retaining rejection of any
additional reported check whose value is not "success".
In `@autorelease/verify.py`:
- Around line 498-531: Update the sandbox collection in the workflow
verification block to retain every CODEX_ACTION invocation per (workflow,
job_name), rather than overwriting earlier entries in observed_sandboxes.
Compare the collected invocation counts and sandbox values against
reviewed_sandboxes so additional invocations in any reviewed job cause
validation to fail.
In `@scripts/check-public-language.sh`:
- Around line 29-39: Update the scan logic surrounding the matches assignment in
scripts/check-public-language.sh to capture stdout, stderr, and the command
status without using “|| true”. Treat status 123 as no matches only when stderr
is empty; otherwise report the scan failure and exit nonzero. Preserve
matched-term reporting while allowing cd, xargs, grep, directory, and I/O errors
to fail the check.
In `@scripts/test.sh`:
- Around line 32-34: Update the ARTIFACT_DIR initialization in scripts/test.sh
so the unset-RUNNER_TEMP branch assigns the directory created directly by mktemp
-d, without creating a separate parent path first. Preserve the export and EXIT
trap so the complete temporary directory is removed.
In `@tests/test_autorelease.py`:
- Around line 646-651: Update test_codeowners_covers_every_protected_script to
resolve both protected-paths.json and CODEOWNERS from the repository root
derived via pathlib.Path(__file__).resolve().parents[1], rather than the current
working directory; also change the CODEOWNERS assertion to accept any whitespace
after each pattern instead of requiring a literal single space.
---
Nitpick comments:
In @.github/workflows/autorelease-watch.yml:
- Around line 587-595: Update the “Report an unrecovered event record” step to
pass needs.investigate.outputs.record_action_key through the step’s env
configuration, then reference the environment variable in the shell command
instead of interpolating the GitHub expression directly. Follow the existing
env-based pattern used by the recovery step.
In @.github/workflows/ci.yml:
- Around line 30-33: Update the “Check shell scripts” step in the CI workflow to
discover extensionless scripts under scripts/ by their bash shebang instead of
maintaining an explicit filename list. Keep the existing .sh coverage and ensure
newly added shebang-based bash scripts are passed to shellcheck automatically.
In `@autorelease/_evidence.py`:
- Around line 218-228: Validate each source URL’s scheme before constructing the
request in the evidence-fetch loop, accepting only https and rejecting http,
file, or any other scheme. Use the existing source URL handling around
urllib.parse.urlparse and ensure invalid schemes cannot be fetched or treated as
authoritative evidence.
In `@autorelease/_validation.py`:
- Around line 127-134: Update path_is_protected and path_is_allowed in
_validation.py to use fnmatch.fnmatchcase instead of fnmatch.fnmatch for
platform-independent, case-sensitive matching. Also update the matching logic in
_admission.py around its visible fnmatch usage to use fnmatchcase consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 059ef154-7fcc-41df-8939-8510c17022c7
📒 Files selected for processing (36)
.github/CODEOWNERS.github/autorelease-pins.json.github/codex/autorelease/investigation.md.github/workflows/autorelease-e2e.yml.github/workflows/autorelease-implement.yml.github/workflows/autorelease-publish.yml.github/workflows/autorelease-watch.yml.github/workflows/build.yml.github/workflows/ci.yml.github/workflows/protected-controls.ymlAUTORELEASE.mdREADME.mdautorelease/_admission.pyautorelease/_evidence.pyautorelease/_state.pyautorelease/_validation.pyautorelease/control.pyautorelease/protected-paths.jsonautorelease/verify.pydocs/release-process.mddocs/repository-settings.mdschemas/agent-completion-assessment.schema.jsonschemas/autorelease-event.schema.jsonschemas/policy-invariants.schema.jsonschemas/support-policy.schema.jsonscripts/assert-admission-checksscripts/build.shscripts/check-public-language.shscripts/lib.shscripts/package.shscripts/serve-autorelease-artifactscripts/snapshot-github-admin-statescripts/test.shscripts/validate-codex-action-inputsscripts/watch-autorelease-evidencetests/test_autorelease.py
💤 Files with no reviewable changes (3)
- schemas/autorelease-event.schema.json
- schemas/policy-invariants.schema.json
- schemas/support-policy.schema.json
Comment on lines
+559
to
+575
| output_dir.mkdir(parents=True, exist_ok=True) | ||
| patch_path = output_dir / "sealed.patch" | ||
| tracked_patch = git(repo, "diff", "--binary", "--full-index", base, "--").stdout | ||
| untracked_patch_parts = [] | ||
| for path in git(repo, "ls-files", "--others", "--exclude-standard").stdout.splitlines(): | ||
| proc = subprocess.run( | ||
| ["git", "diff", "--binary", "--no-index", "--", "/dev/null", path], | ||
| cwd=repo, | ||
| check=False, | ||
| text=True, | ||
| stdout=subprocess.PIPE, | ||
| stderr=subprocess.PIPE, | ||
| ) | ||
| require(proc.returncode in {0, 1}, f"failed to serialize untracked path: {path}") | ||
| untracked_patch_parts.append(proc.stdout) | ||
| patch_path.write_text(tracked_patch + "".join(untracked_patch_parts)) | ||
| require(patch_path.stat().st_size <= 4 * 1024 * 1024, "sealed patch exceeds size limit") |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
The sealed patch passes through locale decoding and newline translation.
git() runs with text=True (Line 490), and the untracked diff at Line 564 does the same. Text mode decodes with the locale encoding and translates CRLF to LF. patch_path.write_text at Line 574 then re-encodes with the locale encoding.
Two consequences follow. The patchDigest is not reproducible across runners with different locales. A tracked file that contains CRLF is sealed with LF, so applying the patch produces bytes that do not match the digests recorded at Line 582, and verify_merge rejects a legitimate patch.
Capture the diff as bytes and write bytes.
🐛 Proposed fix: keep patch bytes unmodified
def git(repo: pathlib.Path, *arguments: str, check: bool = True) -> subprocess.CompletedProcess[str]:
return subprocess.run(
["git", *arguments],
cwd=repo,
check=check,
text=True,
+ encoding="utf-8",
+ newline="",
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
)An alternative is a dedicated bytes helper used only for patch serialization:
output_dir.mkdir(parents=True, exist_ok=True)
patch_path = output_dir / "sealed.patch"
- tracked_patch = git(repo, "diff", "--binary", "--full-index", base, "--").stdout
- untracked_patch_parts = []
+ tracked_patch = subprocess.run(
+ ["git", "diff", "--binary", "--full-index", base, "--"],
+ cwd=repo, check=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE,
+ ).stdout
+ untracked_patch_parts: list[bytes] = []
for path in git(repo, "ls-files", "--others", "--exclude-standard").stdout.splitlines():
proc = subprocess.run(
["git", "diff", "--binary", "--no-index", "--", "/dev/null", path],
cwd=repo,
check=False,
- text=True,
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
)
require(proc.returncode in {0, 1}, f"failed to serialize untracked path: {path}")
untracked_patch_parts.append(proc.stdout)
- patch_path.write_text(tracked_patch + "".join(untracked_patch_parts))
+ patch_path.write_bytes(tracked_patch + b"".join(untracked_patch_parts))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| output_dir.mkdir(parents=True, exist_ok=True) | |
| patch_path = output_dir / "sealed.patch" | |
| tracked_patch = git(repo, "diff", "--binary", "--full-index", base, "--").stdout | |
| untracked_patch_parts = [] | |
| for path in git(repo, "ls-files", "--others", "--exclude-standard").stdout.splitlines(): | |
| proc = subprocess.run( | |
| ["git", "diff", "--binary", "--no-index", "--", "/dev/null", path], | |
| cwd=repo, | |
| check=False, | |
| text=True, | |
| stdout=subprocess.PIPE, | |
| stderr=subprocess.PIPE, | |
| ) | |
| require(proc.returncode in {0, 1}, f"failed to serialize untracked path: {path}") | |
| untracked_patch_parts.append(proc.stdout) | |
| patch_path.write_text(tracked_patch + "".join(untracked_patch_parts)) | |
| require(patch_path.stat().st_size <= 4 * 1024 * 1024, "sealed patch exceeds size limit") | |
| output_dir.mkdir(parents=True, exist_ok=True) | |
| patch_path = output_dir / "sealed.patch" | |
| tracked_patch = subprocess.run( | |
| ["git", "diff", "--binary", "--full-index", base, "--"], | |
| cwd=repo, check=True, stdout=subprocess.PIPE, stderr=subprocess.PIPE, | |
| ).stdout | |
| untracked_patch_parts: list[bytes] = [] | |
| for path in git(repo, "ls-files", "--others", "--exclude-standard").stdout.splitlines(): | |
| proc = subprocess.run( | |
| ["git", "diff", "--binary", "--no-index", "--", "/dev/null", path], | |
| cwd=repo, | |
| check=False, | |
| stdout=subprocess.PIPE, | |
| stderr=subprocess.PIPE, | |
| ) | |
| require(proc.returncode in {0, 1}, f"failed to serialize untracked path: {path}") | |
| untracked_patch_parts.append(proc.stdout) | |
| patch_path.write_bytes(tracked_patch + b"".join(untracked_patch_parts)) | |
| require(patch_path.stat().st_size <= 4 * 1024 * 1024, "sealed patch exceeds size limit") |
🧰 Tools
🪛 ast-grep (0.45.0)
[error] 563-570: Command coming from incoming request
Context: subprocess.run(
["git", "diff", "--binary", "--no-index", "--", "/dev/null", path],
cwd=repo,
check=False,
text=True,
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
🪛 Ruff (0.16.0)
[error] 564-564: subprocess call: check for execution of untrusted input
(S603)
[error] 565-565: Starting a process with a partial executable path
(S607)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@autorelease/_admission.py` around lines 559 - 575, Update the patch
serialization around tracked and untracked diff collection to preserve raw
bytes: avoid text-mode decoding/newline translation in the relevant
git/subprocess calls, combine byte outputs, and write them with a binary file
operation instead of write_text. Keep the existing patch size validation and
digest flow unchanged.
Contributor
Author
There was a problem hiding this comment.
Agreed that bytes-mode capture is the right hardening, and we are queueing it as a follow-up rather than folding it into this PR since the admission module is protected trust-boundary code that just went through final review. Two things bound the risk in the meantime: every runner in these workflows is ubuntu-latest with the same UTF-8 locale, so digests are reproducible in practice, and any mismatch fails closed at verify_merge with a digest rejection, so this can never admit wrong bytes, only reject a patch that contains CRLF content.
There was a problem hiding this comment.
@loadinglucian, understood. The current workflow environment and fail-closed verify_merge behavior limit the impact to rejection of a valid CRLF-containing patch. They do not permit admission of altered bytes.
The finding remains valid for the follow-up. I will leave it unresolved for this PR. Would you like me to create a GitHub issue for the bytes-mode patch serialization change?
🐇
You are interacting with an AI system.
Comment on lines
+608
to
+612
| require(bool(COMMIT_SHA_RE.fullmatch(expected_head or "")), "expected head is not an exact commit SHA") | ||
| actual_head = git(repo, "rev-parse", "HEAD").stdout.strip() | ||
| require(actual_head == expected_head, "PR head does not equal validated SHA") | ||
| require(checks and all(value == "success" for value in checks.values()), "required checks did not succeed") | ||
| require(preconditions == current, "merge preconditions changed") |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find verify_merge callers and the checks mapping they build.
set -euo pipefail
rg -nP '\bverify_merge\s*\(' -C 12
rg -n 'requiredChecks|Script checks' -C 3Repository: Bigpixelrocket/php-bin
Length of output: 160
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- candidate files ---'
git ls-files | rg '(^|/)_admission\.py$|admission'
printf '%s\n' '--- verify_merge references ---'
rg -n -C 8 '\bverify_merge\b|REQUIRED_PLAN_CHECKS|requiredChecks|Script checks' . || trueRepository: Bigpixelrocket/php-bin
Length of output: 33826
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- admission implementation ---'
sed -n '400,430p;590,660p' autorelease/_admission.py
printf '%s\n' '--- direct caller ---'
sed -n '1,80p' scripts/verify-merge-admission
printf '%s\n' '--- check assertion script ---'
sed -n '1,100p' scripts/assert-admission-checks
printf '%s\n' '--- workflow and check-file references ---'
rg -n -C 10 'verify-merge-admission|assert-admission-checks|checks\.json|check-runs|status' .github scripts autorelease testsRepository: Bigpixelrocket/php-bin
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- all checks-file producers and consumers ---'
rg -n -C 6 --glob '*.yml' --glob '*.yaml' --glob '*.sh' --glob '*.py' \
'(^|[[:space:]])(--checks|checks_file|checks\.json|check-runs|statusCheck|status-check|Script checks)' \
.github scripts autorelease tests || true
printf '%s\n' '--- workflow invocations of merge admission ---'
rg -n -C 15 --glob '*.yml' --glob '*.yaml' \
'verify-merge-admission|assert-admission-checks|merge admission' \
.github || trueRepository: Bigpixelrocket/php-bin
Length of output: 36263
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- dispatch-pr-checks implementation ---'
sed -n '1,240p' scripts/dispatch-pr-checks
printf '%s\n' '--- verify_merge tests and nearby helpers ---'
sed -n '860,915p' tests/test_autorelease.py
sed -n '560,720p' autorelease/verify.py
printf '%s\n' '--- all non-test verify_merge call sites ---'
rg -n -C 4 '\bverify_merge\s*\(' --glob '*.py' --glob '*.sh' --glob '*.yml' --glob '*.yaml' . || trueRepository: Bigpixelrocket/php-bin
Length of output: 21881
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
REQUIRED_PLAN_CHECKS = ["Script checks"]
def current_gate(checks):
return bool(checks) and all(value == "success" for value in checks.values())
def required_gate(checks):
return (
all(checks.get(name) == "success" for name in REQUIRED_PLAN_CHECKS)
and all(value == "success" for value in checks.values())
)
cases = {
"required check succeeds": {"Script checks": "success"},
"required check missing": {"Other check": "success"},
"required check fails": {"Script checks": "failure"},
"empty mapping": {},
}
for label, checks in cases.items():
print(label, "current=", current_gate(checks), "membership=", required_gate(checks))
assert current_gate(cases["required check succeeds"])
assert current_gate(cases["required check missing"])
assert not required_gate(cases["required check missing"])
assert not required_gate(cases["required check fails"])
assert not required_gate(cases["empty mapping"])
PYRepository: Bigpixelrocket/php-bin
Length of output: 371
Enforce REQUIRED_PLAN_CHECKS in verify_merge. The function accepts {"Other check": "success"} because it checks only that all supplied values succeed. Require every name in REQUIRED_PLAN_CHECKS to map to "success" and retain the check for additional reported failures.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@autorelease/_admission.py` around lines 608 - 612, Update verify_merge to
validate every check named in REQUIRED_PLAN_CHECKS has a "success" status, while
retaining rejection of any additional reported check whose value is not
"success".
Contributor
Author
There was a problem hiding this comment.
The mapping is not free-form at that point: admission pins the plan's requiredChecks to exactly REQUIRED_PLAN_CHECKS when the plan is validated, and the workflow builds the checks dict through scripts/assert-admission-checks, which fails the job unless each required check name is present and successful before verify_merge ever runs. So a caller passing only unrelated names cannot reach a merge in the real flow. That said, the in-function assertion you suggest is cheap defense in depth and we will add it as a follow-up.
There was a problem hiding this comment.
@loadinglucian, understood. The production flow already enforces REQUIRED_PLAN_CHECKS before verify_merge receives the checks mapping.
The proposed assertion is therefore defense in depth, not a bypass in the current merge path. Thanks for confirming that you will add it in a follow-up.
🐇
You are interacting with an AI system.
Comment on lines
+498
to
+531
| # Every reviewed agent invocation, keyed by the workflow and job that may | ||
| # start it, with the sandbox that bounds its network and write authority. | ||
| reviewed_sandboxes = { | ||
| ("autorelease-watch.yml", "investigate"): "read-only", | ||
| ("autorelease-implement.yml", "implement"): "workspace-write", | ||
| ("autorelease-implement.yml", "repair"): "workspace-write", | ||
| } | ||
| observed_sandboxes = {} | ||
| for name in ("autorelease-watch.yml", "autorelease-implement.yml"): | ||
| steps = workflow_steps(load_workflow(PHP_ROOT / ".github/workflows" / name)) | ||
| for job_name, index, step in steps: | ||
| if not str(step.get("uses") or "").startswith(CODEX_ACTION): | ||
| continue | ||
| inputs = step.get("with") or {} | ||
| observed_sandboxes[(name, job_name)] = inputs.get("sandbox") | ||
| assert_true( | ||
| not any( | ||
| item.startswith("--profile") | ||
| for item in json.loads(inputs.get("codex-args") or "[]") | ||
| ), | ||
| f"{name}:{job_name} selects a named profile instead of the canonical config", | ||
| ) | ||
| assert_true( | ||
| any( | ||
| other_job == job_name | ||
| and other_index < index | ||
| and CANONICAL_CODEX_CONFIG.search(other.get("run") or "") | ||
| for other_job, other_index, other in steps | ||
| ), | ||
| f"{name}:{job_name} starts the agent without loading its canonical config", | ||
| ) | ||
| assert_true( | ||
| "sandbox: workspace-write" in implementation | ||
| and 'cp ".codex/$phase.config.toml" "$RUNNER_TEMP/codex-home/config.toml"' in implementation | ||
| and 'cp .codex/repair.config.toml "$RUNNER_TEMP/codex-home/config.toml"' in implementation | ||
| and '"--profile"' not in implementation, | ||
| "phase-bound implementation/repair canonical config loading is missing", | ||
| observed_sandboxes == reviewed_sandboxes, | ||
| "investigation and implementation agents are not bound to their reviewed sandboxes", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
A07 can miss a second agent invocation inside a reviewed job.
observed_sandboxes is keyed by (workflow, job_name). If a job gains a second openai/codex-action step, the later step overwrites the earlier entry, so the dict can still equal reviewed_sandboxes while an unreviewed invocation runs. A06 bounds the count for the repair job only, so no check bounds the other jobs.
Collect every observed sandbox per key and compare the counts.
🛡️ Proposed fix
- observed_sandboxes = {}
+ observed_sandboxes: dict[tuple[str, str], list[str | None]] = {}
for name in ("autorelease-watch.yml", "autorelease-implement.yml"):
steps = workflow_steps(load_workflow(PHP_ROOT / ".github/workflows" / name))
for job_name, index, step in steps:
if not str(step.get("uses") or "").startswith(CODEX_ACTION):
continue
inputs = step.get("with") or {}
- observed_sandboxes[(name, job_name)] = inputs.get("sandbox")
+ observed_sandboxes.setdefault((name, job_name), []).append(inputs.get("sandbox"))
@@
assert_true(
- observed_sandboxes == reviewed_sandboxes,
+ observed_sandboxes == {key: [value] for key, value in reviewed_sandboxes.items()},
"investigation and implementation agents are not bound to their reviewed sandboxes",
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Every reviewed agent invocation, keyed by the workflow and job that may | |
| # start it, with the sandbox that bounds its network and write authority. | |
| reviewed_sandboxes = { | |
| ("autorelease-watch.yml", "investigate"): "read-only", | |
| ("autorelease-implement.yml", "implement"): "workspace-write", | |
| ("autorelease-implement.yml", "repair"): "workspace-write", | |
| } | |
| observed_sandboxes = {} | |
| for name in ("autorelease-watch.yml", "autorelease-implement.yml"): | |
| steps = workflow_steps(load_workflow(PHP_ROOT / ".github/workflows" / name)) | |
| for job_name, index, step in steps: | |
| if not str(step.get("uses") or "").startswith(CODEX_ACTION): | |
| continue | |
| inputs = step.get("with") or {} | |
| observed_sandboxes[(name, job_name)] = inputs.get("sandbox") | |
| assert_true( | |
| not any( | |
| item.startswith("--profile") | |
| for item in json.loads(inputs.get("codex-args") or "[]") | |
| ), | |
| f"{name}:{job_name} selects a named profile instead of the canonical config", | |
| ) | |
| assert_true( | |
| any( | |
| other_job == job_name | |
| and other_index < index | |
| and CANONICAL_CODEX_CONFIG.search(other.get("run") or "") | |
| for other_job, other_index, other in steps | |
| ), | |
| f"{name}:{job_name} starts the agent without loading its canonical config", | |
| ) | |
| assert_true( | |
| "sandbox: workspace-write" in implementation | |
| and 'cp ".codex/$phase.config.toml" "$RUNNER_TEMP/codex-home/config.toml"' in implementation | |
| and 'cp .codex/repair.config.toml "$RUNNER_TEMP/codex-home/config.toml"' in implementation | |
| and '"--profile"' not in implementation, | |
| "phase-bound implementation/repair canonical config loading is missing", | |
| observed_sandboxes == reviewed_sandboxes, | |
| "investigation and implementation agents are not bound to their reviewed sandboxes", | |
| # Every reviewed agent invocation, keyed by the workflow and job that may | |
| # start it, with the sandbox that bounds its network and write authority. | |
| reviewed_sandboxes = { | |
| ("autorelease-watch.yml", "investigate"): "read-only", | |
| ("autorelease-implement.yml", "implement"): "workspace-write", | |
| ("autorelease-implement.yml", "repair"): "workspace-write", | |
| } | |
| observed_sandboxes: dict[tuple[str, str], list[str | None]] = {} | |
| for name in ("autorelease-watch.yml", "autorelease-implement.yml"): | |
| steps = workflow_steps(load_workflow(PHP_ROOT / ".github/workflows" / name)) | |
| for job_name, index, step in steps: | |
| if not str(step.get("uses") or "").startswith(CODEX_ACTION): | |
| continue | |
| inputs = step.get("with") or {} | |
| observed_sandboxes.setdefault((name, job_name), []).append(inputs.get("sandbox")) | |
| assert_true( | |
| not any( | |
| item.startswith("--profile") | |
| for item in json.loads(inputs.get("codex-args") or "[]") | |
| ), | |
| f"{name}:{job_name} selects a named profile instead of the canonical config", | |
| ) | |
| assert_true( | |
| any( | |
| other_job == job_name | |
| and other_index < index | |
| and CANONICAL_CODEX_CONFIG.search(other.get("run") or "") | |
| for other_job, other_index, other in steps | |
| ), | |
| f"{name}:{job_name} starts the agent without loading its canonical config", | |
| ) | |
| assert_true( | |
| observed_sandboxes == {key: [value] for key, value in reviewed_sandboxes.items()}, | |
| "investigation and implementation agents are not bound to their reviewed sandboxes", | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@autorelease/verify.py` around lines 498 - 531, Update the sandbox collection
in the workflow verification block to retain every CODEX_ACTION invocation per
(workflow, job_name), rather than overwriting earlier entries in
observed_sandboxes. Compare the collected invocation counts and sandbox values
against reviewed_sandboxes so additional invocations in any reviewed job cause
validation to fail.
Contributor
Author
There was a problem hiding this comment.
True, the dict keying can only hold one sandbox per job. The mitigating control is that adding a second codex-action step means editing a workflow file, and workflows are protected paths that require an owner-approved change, so this hole cannot be opened by an admitted agent patch. Collecting per-key lists and comparing counts is a nice strengthening and we will pick it up as a follow-up to the verifier.
There was a problem hiding this comment.
@loadinglucian, agreed. Protected workflow paths prevent an admitted agent patch from adding the extra invocation. This makes the gap non-blocking for this PR.
The per-invocation collection remains useful defense in depth for owner-approved workflow changes. Would you like me to create a follow-up GitHub issue for the A07 verifier strengthening?
✏️ Learnings added
Learnt from: loadinglucian
Repo: Bigpixelrocket/php-bin PR: 56
File: autorelease/verify.py:498-531
Timestamp: 2026-08-03T17:33:49.612Z
Learning: In `autorelease/verify.py`, A07 validates Codex Action workflow invocations. GitHub workflow files are protected paths, so admitted agent patches cannot add or modify `openai/codex-action` steps. An A07 check that records one sandbox per `(workflow, job)` is therefore a defense-in-depth gap for owner-approved workflow changes, not an admitted-agent bypass.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
You are interacting with an AI system.
Comment on lines
+29
to
+39
| # xargs reports 123 when any grep batch matches nothing, so the finding is read | ||
| # from the output rather than from the exit status. | ||
| matches="$(cd "$PROJECT_ROOT" && xargs -0 grep -HIFni -e "$REJECTED_TERM" < "$tracked" || true)" | ||
|
|
||
| if [[ -n "$matches" ]]; then | ||
| printf '%s\n' "$matches" >&2 | ||
| echo "Public-language check failed." >&2 | ||
| exit 1 | ||
| fi | ||
|
|
||
| echo "Public-language check passed." |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
shellcheck scripts/check-public-language.shRepository: Bigpixelrocket/php-bin
Length of output: 160
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '--- file ---'
cat -n scripts/check-public-language.sh
printf '%s\n' '--- tracked references ---'
rg -n 'check-public-language|REJECTED_TERM|PROJECT_ROOT|xargs -0 grep|scan_errors|matches=' .github scripts README.md 2>/dev/null || true
printf '%s\n' '--- shellcheck availability ---'
command -v shellcheck || true
shellcheck --version 2>/dev/null || trueRepository: Bigpixelrocket/php-bin
Length of output: 4603
🏁 Script executed:
#!/bin/bash
set -u
printf '%s\n' '--- file metadata and syntax ---'
wc -l scripts/check-public-language.sh
bash -n scripts/check-public-language.sh
printf '%s\n' '--- shellcheck result ---'
shellcheck -f gcc scripts/check-public-language.sh 2>&1; printf 'shellcheck_status=%s\n' "$?"
printf '%s\n' '--- relevant implementation ---'
sed -n '1,100p' scripts/check-public-language.shRepository: Bigpixelrocket/php-bin
Length of output: 1745
🏁 Script executed:
#!/bin/bash
set -u
tmp="$(mktemp -d)"
trap 'rm -rf "$tmp"' EXIT
printf '%s\0' "$tmp/missing.txt" > "$tmp/tracked"
printf '%s\n' '--- current command: failed cd ---'
matches="$(cd "$tmp/does-not-exist" && xargs -0 grep -HIFni -e needle < "$tmp/tracked" || true)"
printf 'status=%s matches=%q\n' "$?" "$matches"
printf '%s\n' '--- current command: grep error ---'
matches="$(cd "$tmp" && xargs -0 grep -HIFni -e needle < "$tmp/tracked" || true)"
printf 'status=%s matches=%q\n' "$?" "$matches"
printf '%s\n' '--- current command: normal no-match ---'
printf 'content\0' > "$tmp/content.txt"
printf '%s\0' "$tmp/content.txt" > "$tmp/tracked"
matches="$(cd "$tmp" && xargs -0 grep -HIFni -e needle < "$tmp/tracked" || true)"
printf 'status=%s matches=%q\n' "$?" "$matches"
printf '%s\n' '--- raw xargs statuses ---'
printf '%s\0' "$tmp/content.txt" | xargs -0 grep -HIFni -e needle >/dev/null
printf 'no_match_xargs_status=%s\n' "$?"
printf '%s\0' "$tmp/missing.txt" | xargs -0 grep -HIFni -e needle >/dev/null 2>"$tmp/error"
printf 'grep_error_xargs_status=%s stderr_nonempty=%s\n' "$?" "$([[ -s "$tmp/error" ]] && echo yes || echo no)"Repository: Bigpixelrocket/php-bin
Length of output: 578
🏁 Script executed:
#!/bin/bash
set -u
shellcheck -s bash - <<'SCRIPT'
#!/usr/bin/env bash
set -euo pipefail
PROJECT_ROOT=/tmp
REJECTED_TERM=needle
tracked=/tmp/tracked
scan_errors=/tmp/scan_errors
scan_status=0
matches="$(
cd "$PROJECT_ROOT" || exit 1
xargs -0 grep -HIFni -e "$REJECTED_TERM" < "$tracked" 2>"$scan_errors"
)" || scan_status=$?
if [[ -s "$scan_errors" ]] || (( scan_status != 0 && scan_status != 123 )); then
cat "$scan_errors" >&2
echo "Public-language check could not scan tracked files." >&2
exit 1
fi
SCRIPT
printf 'shellcheck_status=%s\n' "$?"
SCRIPTRepository: Bigpixelrocket/php-bin
Length of output: 448
Preserve scan failures instead of treating them as no matches.
The || true suppresses failures from cd, xargs, and grep. Handle the scan status explicitly. Treat status 123 as no matches only when stderr is empty. Fail on directory, I/O, or xargs errors.
🧰 Tools
🪛 GitHub Actions: CI / 0_Script checks.txt
[error] 31-31: ShellCheck SC2015: The 'A && B || C' construct is not equivalent to if-then-else; C may run when A succeeds. The shellcheck command failed with exit code 1.
🪛 GitHub Actions: CI / Script checks
[error] 31-31: ShellCheck SC2015: The 'cd ... && xargs ... || true' expression is not equivalent to an if-then-else; the fallback may run even when the preceding command succeeds. ShellCheck command failed with exit code 1.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/check-public-language.sh` around lines 29 - 39, Update the scan logic
surrounding the matches assignment in scripts/check-public-language.sh to
capture stdout, stderr, and the command status without using “|| true”. Treat
status 123 as no matches only when stderr is empty; otherwise report the scan
failure and exit nonzero. Preserve matched-term reporting while allowing cd,
xargs, grep, directory, and I/O errors to fail the check.
Source: Pipeline failures
Contributor
Author
There was a problem hiding this comment.
Partially fixed already: the latest commit groups the tolerance so a cd or listing failure is now fatal instead of reading as no matches, which also resolves the pipeline failure this comment came from. The remaining tolerance is deliberate for the xargs no-match status. The narrower nuance about grep reporting an I/O error was reviewed and accepted as bounded, since the file list is validated non-empty beforehand and the search term is a fixed literal, but your explicit status handling suggestion is a fair refinement if we revisit the script.
There was a problem hiding this comment.
@loadinglucian, acknowledged. The current grouping makes cd failures fatal, and the separate tracked-file listing keeps listing failures fatal. This resolves the original pipeline failure.
The remaining xargs/grep error tolerance is an accepted bounded tradeoff for this PR. I will not treat it as a blocking issue.
🐇
✏️ Learnings added
Learnt from: loadinglucian
Repo: Bigpixelrocket/php-bin PR: 56
File: scripts/check-public-language.sh:29-39
Timestamp: 2026-08-03T17:33:56.983Z
Learning: In `scripts/check-public-language.sh`, the `matches` command deliberately tolerates the `xargs` no-match status with `|| true`. The script separately fails on `git ls-files -z` failures, an empty tracked-file list, and `cd` failures. The maintainers reviewed and accepted the remaining possibility that `grep` or `xargs` I/O errors can be masked as a bounded tradeoff.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
You are interacting with an AI system.
Comment on lines
+32
to
+34
| ARTIFACT_DIR="${RUNNER_TEMP:-$(mktemp -d)}/php-bin-test-artifacts" | ||
| export ARTIFACT_DIR | ||
| trap 'rm -rf "$ARTIFACT_DIR"' EXIT |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- scripts/test.sh ---'
cat -n scripts/test.sh | sed -n '20,45p'
printf '%s\n' '--- scripts/package.sh references ---'
rg -n -C 3 'ARTIFACT_DIR|mktemp|trap' scripts/package.sh scripts/test.shRepository: Bigpixelrocket/php-bin
Length of output: 3546
🏁 Script executed:
#!/bin/bash
set -eu
root="$(mktemp -d)"
cleanup() { rm -rf "$root"; }
trap cleanup EXIT
run_case() {
label="$1"
expression="$2"
case_dir="$root/$label"
mkdir "$case_dir"
bash -c '
set -eu
RUNNER_TEMP=
TMPDIR="$1"
ARTIFACT_DIR='"$expression"'
trap '"'"'rm -rf "$ARTIFACT_DIR"'"'"' EXIT
mkdir -p "$ARTIFACT_DIR"
printf "%s\n" "$ARTIFACT_DIR"
' bash "$case_dir" >"$case_dir/path"
printf '%s\n' "[$label]"
cat "$case_dir/path"
find "$case_dir" -mindepth 1 -maxdepth 2 -print
}
run_case original '"${RUNNER_TEMP:-$(mktemp -d)}/php-bin-test-artifacts"'
run_case replacement '"$(mktemp -d "${RUNNER_TEMP:-${TMPDIR:-/tmp}}/php-bin-test-artifacts.XXXXXX)"'Repository: Bigpixelrocket/php-bin
Length of output: 313
🏁 Script executed:
#!/bin/bash
set -eu
root="$(mktemp -d)"
trap 'rm -rf "$root"' EXIT
for variant in original replacement; do
base="$root/$variant"
mkdir "$base"
if [ "$variant" = original ]; then
env -u RUNNER_TEMP TMPDIR="$base" bash -c '
set -eu
ARTIFACT_DIR="${RUNNER_TEMP:-$(mktemp -d)}/php-bin-test-artifacts"
trap '"'"'rm -rf "$ARTIFACT_DIR"'"'"' EXIT
mkdir -p "$ARTIFACT_DIR"
'
else
env -u RUNNER_TEMP TMPDIR="$base" bash -c '
set -eu
ARTIFACT_DIR="$(mktemp -d "${RUNNER_TEMP:-${TMPDIR:-/tmp}}/php-bin-test-artifacts.XXXXXX")"
trap '"'"'rm -rf "$ARTIFACT_DIR"'"'"' EXIT
'
fi
printf '[%s]\n' "$variant"
find "$base" -mindepth 1 -maxdepth 2 -print
doneRepository: Bigpixelrocket/php-bin
Length of output: 229
Remove the temporary parent directory
When RUNNER_TEMP is unset, create ARTIFACT_DIR directly with mktemp -d. The EXIT trap then removes the complete temporary directory.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/test.sh` around lines 32 - 34, Update the ARTIFACT_DIR initialization
in scripts/test.sh so the unset-RUNNER_TEMP branch assigns the directory created
directly by mktemp -d, without creating a separate parent path first. Preserve
the export and EXIT trap so the complete temporary directory is removed.
Contributor
Author
There was a problem hiding this comment.
Accepted as a conscious tradeoff. CI always sets RUNNER_TEMP, so the mktemp fallback only runs on a developer machine, the EXIT trap removes the artifact directory itself, and the empty parent sits in TMPDIR until the OS cleans it. Creating the artifact dir directly with mktemp would also work, we just kept the two cases symmetric.
There was a problem hiding this comment.
@loadinglucian Understood. The fallback behavior is limited to developer machines, and the empty temporary parent directory is an accepted cleanup tradeoff. No change is required.
🐇
✏️ Learnings added
Learnt from: loadinglucian
Repo: Bigpixelrocket/php-bin PR: 56
File: scripts/test.sh:32-34
Timestamp: 2026-08-03T17:33:56.425Z
Learning: In `scripts/test.sh`, CI always sets `RUNNER_TEMP`. When `RUNNER_TEMP` is unset on a developer machine, `ARTIFACT_DIR="${RUNNER_TEMP:-$(mktemp -d)}/php-bin-test-artifacts"` intentionally leaves an empty temporary parent directory after the `EXIT` trap removes `ARTIFACT_DIR`. This is an accepted tradeoff to keep the `RUNNER_TEMP` and fallback paths symmetric.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
You are interacting with an AI system.
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_a0e1fa97-5d2a-4db7-9cd9-a5e3ff3cd307) |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_2557c4f0-184a-44da-a593-c9c278b3c569) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hardens the autorelease system so new PHP builds (any major or minor), patch releases, and branch EOL delisting all run end to end with zero maintainer input, while every published release stays installable.
Build path (Phase 2)
\d+\.\d+) driven by the support-policy snapshot.Gate hardening (Phase 3)
.github/dependabot.ymlprotected and code-owned to match the mise-php parity gate.Structural verification (Phase 4)
Reliability and structure (Phase 5)
--repoon merge/close, and evidence records defer one run when a recovery just moved main (prevents a repeating fail-closed wedge).route-watch-action) that fails loud on unrouted combinations; operator gate and action-filename mapping are single-sourced._validation,_admission,_state,_evidence), verbatim moves verified.defaults: run: shell: bashwith least-privilege permissions.Docs (Phase 6)
All gates green: 34 unit tests, acceptance verifier 21/21, shellcheck, markdownlint.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation