feat: add guarded hybrid PHP maintenance - #7
Conversation
📝 WalkthroughWalkthroughThis change introduces a guarded PHP maintenance system with evidence-bound plans, deterministic controls, offline implementation and repair workflows, immutable release transactions, GitHub coordination, policy schemas, verification tooling, and updated repository administration documentation. ChangesGuarded maintenance automation
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Watcher
participant Investigation
participant Implementation
participant Release
participant Notification
Watcher->>Investigation: Capture evidence and produce admitted plan
Investigation->>Implementation: Dispatch admitted edit phase
Implementation->>Implementation: Seal and validate patch
Implementation->>Release: Provide validated maintenance result
Release->>Release: Build, verify, and publish immutable assets
Release->>Notification: Record completion and notify maintenance owner
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 2
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (19)
.github/workflows/maintenance-release.yml-95-96 (1)
95-96: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
policyInvariantsDigestis read from the wrong file.Line 95 correctly compares the readiness record against the event record, but Line 96 reads
.policyInvariantsDigestfromsupport-policy.json. Everywhere else that digest is the SHA-256 ofmaintenance/policy-invariants.jsonand is carried on the event (seemaintenance-watch.ymlLine 249 andmaintenance-implementation.ymlLine 398). Ifsupport-policy.jsonhas no such key, jq yieldsnulland everynew_branchrelease is blocked.🐛 Proposed fix
- test "$(jq -r .policyInvariantsDigest release-run/mise-readiness.json)" = "$(jq -r .policyInvariantsDigest support-policy.json)" + test "$(jq -r .policyInvariantsDigest release-run/mise-readiness.json)" = "$(jq -r .policyInvariantsDigest "$event")"#!/bin/bash # Confirm support-policy.json has no policyInvariantsDigest field. fd -t f 'support-policy.json' -x sh -c 'echo "== {}"; jq -r "keys" {}' rg -n 'policyInvariantsDigest' --glob '!**/.git/**'🤖 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/maintenance-release.yml around lines 95 - 96, Update the policyInvariantsDigest assertion in the maintenance release workflow to compare the readiness record’s digest with the event record’s policyInvariantsDigest, matching the policyDigest assertion above. Stop reading policyInvariantsDigest from support-policy.json so new_branch releases validate against the event-carried digest..github/workflows/maintenance-e2e.yml-139-149 (1)
139-149: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDispatch inputs are expanded into
run:scripts before they are validated. Both workflows validate a version string with a[[ =~ ]]guard on a line that itself already contains the raw${{ inputs.* }}expansion, so the guard can never protect the script it lives in. Bind every dispatch input to anenv:variable and validate the shell variable instead.
.github/workflows/maintenance-e2e.yml#L139-L149: setenv: LIVE_VERSION: ${{ inputs.live_version }}and replace all four${{ inputs.live_version }}expansions with"$LIVE_VERSION"..github/workflows/maintenance-release.yml#L57-L64: setenv:entries forversion,exact_commit, andaction_key, then validate and compare against"$VERSION","$EXACT_COMMIT", and"$ACTION_KEY".🤖 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/maintenance-e2e.yml around lines 139 - 149, Bind the dispatch inputs to environment variables before the shell scripts execute. In .github/workflows/maintenance-e2e.yml lines 139-149, add LIVE_VERSION from inputs.live_version and replace all four direct expansions with quoted "$LIVE_VERSION" references. In .github/workflows/maintenance-release.yml lines 57-64, add env entries for version, exact_commit, and action_key, then update validation and comparisons to use quoted "$VERSION", "$EXACT_COMMIT", and "$ACTION_KEY".Source: Linters/SAST tools
.github/workflows/maintenance-implementation.yml-46-72 (1)
46-72: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
phase=repairloadsrepair.config.tomlbut still passes--profile implementation.Line 72 copies the phase-specific Codex config, and Lines 50/62 correctly parameterize the prompt, yet Line 84 hardcodes the profile. With
--strict-config, a repair-phase dispatch either errors on an unknown profile or silently runs under implementation authority.🐛 Proposed fix
- codex-args: '["--strict-config","--ephemeral","--profile","implementation","--output-schema","schemas/agent-completion-assessment.schema.json"]' + codex-args: '["--strict-config","--ephemeral","--profile","${{ inputs.phase }}","--output-schema","schemas/agent-completion-assessment.schema.json"]'Also applies to: 84-84
🤖 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/maintenance-implementation.yml around lines 46 - 72, Update the Codex dispatch invocation in the maintenance workflow to stop hardcoding the implementation profile. Derive the profile from inputs.phase so phase=repair uses the repair profile matching the copied phase-specific configuration, while retaining implementation for the implementation phase and preserving strict-config behavior..github/workflows/maintenance-implementation.yml-318-323 (1)
318-323: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winFetch the bundle ref instead of requesting the bare SHA.
The bundle is created as
HEAD ^<base>, so a destination checkout that only downloads the artifact can lack the prerequisite base commit. Fetchmaintenance-run/validated.bundle HEAD(or another local ref created by the bundle), then checkout/verifyHEAD == $validated.🤖 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/maintenance-implementation.yml around lines 318 - 323, Update the “Restore exact validated commit” step to fetch the bundle’s local ref by requesting `maintenance-run/validated.bundle HEAD` instead of fetching the bare validated SHA. Keep checking out the validated SHA and verifying `git rev-parse HEAD` equals `$validated`.scripts/configure-github-maintenance-109-114 (1)
109-114: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winCode-owner review is configured but never actually enforced. The branch protection payload requires code-owner reviews while requiring zero approvals, and the CODEOWNERS file drops its catch-all entry — together these mean maintenance-critical paths can merge with no human approval at all.
scripts/configure-github-maintenance#L109-L114: setrequired_approving_review_countto at least1sorequire_code_owner_reviewstakes effect..github/CODEOWNERS#L1-L13: restore a*@loadinglucian`` catch-all and broaden/schemas/agent-*to `/schemas/` so admission-semantics schemas and `support-policy.json` are owned.🤖 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/configure-github-maintenance` around lines 109 - 114, Update the required_pull_request_reviews payload in scripts/configure-github-maintenance to set required_approving_review_count to at least 1. In .github/CODEOWNERS, restore the * `@loadinglucian` catch-all entry and broaden the /schemas/agent-* ownership pattern to /schemas/ so all admission-semantics schemas and support-policy.json are covered.maintenance/control.py-295-325 (1)
295-325: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winSupport-policy shape validation is duplicated verbatim in
seal_patch.The key-set check,
schemaVersion,policyInvariantsDigestbinding, branch canonicalization, andacceptedAtparsing here are repeated at Lines 548-588. Two copies of a security invariant will drift. Extract a shared_validate_support_policy_document(policy, invariants_path)helper and have both call sites layer their extra constraints on top.🤖 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 `@maintenance/control.py` around lines 295 - 325, Extract the duplicated support-policy validation from the current flow into a shared _validate_support_policy_document(policy, invariants_path) helper, including key-set, schemaVersion, policyInvariantsDigest, maintainedBranches, sourceEvidenceDigests, and acceptedAt checks. Update both the current validator and seal_patch to call this helper, while retaining only their call-site-specific constraints afterward.maintenance/control.py-244-254 (1)
244-254: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winUntrusted record paths are joined to a base directory without containment checks. Both loaders take a
path/bodyPathstring out of agent- or event-supplied JSON and join it directly, so../segments and absolute paths escape the intended directory; the subsequent digest comparison then attests whatever file was reached. One shared containment helper fixes both.
maintenance/control.py#L244-L254: validatecapture["bodyPath"]is relative and..-free, resolve it, and requireis_relative_to(manifest_path.parent.resolve())beforeis_file().maintenance/control.py#L796-L807: apply the same helper toitem["path"]againstroot, and convert theitem["path"]/item["digest"]lookups intoControlError-raisingrequirechecks.🤖 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 `@maintenance/control.py` around lines 244 - 254, Untrusted record paths can escape their intended roots before digest validation. Add one shared containment helper, then update load_capture to validate capture["bodyPath"] as relative and free of “..” segments, resolve it, and require containment within manifest_path.parent.resolve() before checking is_file(); apply the same helper to the loader at maintenance/control.py lines 796-807 for item["path"] against root, and make missing item["path"] and item["digest"] values raise ControlError via require checks. Both affected sites are in maintenance/control.py; the shared helper should serve load_capture and the second loader.scripts/snapshot-github-admin-state-26-34 (1)
26-34: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
gh_apireturns three different shapes, and callers assume one. It yieldsNone, a single parsed document, or a flattenedlistdepending on page count, which makes every call site guess. Normalizing the return contract removes both downstream defects.
scripts/snapshot-github-admin-state#L26-L34: usegh api --paginate --slurpsoghproduces a single JSON array, and drop the line-splitting and manual flattening.scripts/snapshot-github-admin-state#L76-L83: extract a helper that returns alistof items for a given key regardless of page count, so.get(...)is never called on alistand multi-page variable/secret listings stop raising an uncaughtAttributeError.🤖 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/snapshot-github-admin-state` around lines 26 - 34, Normalize gh_api to always return a list by adding --slurp to the paginated gh api invocation and replacing line-splitting/manual flattening in the gh_api parsing block with direct JSON parsing. In the variable/secret listing logic, extract a helper that accepts the parsed response and key and returns a list of items for both single-object and multi-page responses, then use it at the affected call sites so .get is never invoked on a list. Apply these changes at scripts/snapshot-github-admin-state lines 26-34 and 76-83.maintenance/control.py-524-524 (1)
524-524: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winGit ref arguments reach
gitbefore being validated as 40-hex SHAs.verify_mergevalidatesbase_shaat Line 643 but nothing validatesbaseinseal_patchorexpected_headinverify_mergebefore the first subprocess call, and several invocations lack an end-of-options--. A ref beginning with-is parsed as a git option.
maintenance/control.py#L524-L524: addrequire(bool(re.fullmatch(r"[0-9a-f]{40}", base or "")), ...)before thegit rev-parsecall, and add--to thegit diff --no-indexinvocation at Line 595.maintenance/control.py#L638-L648: add the same hex validation forexpected_headahead of thegit rev-parseat Line 638.🤖 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 `@maintenance/control.py` at line 524, Validate base in seal_patch and expected_head in verify_merge with re.fullmatch(r"[0-9a-f]{40}", value or "") before their first git subprocess calls, using require with the existing descriptive error style. In maintenance/control.py lines 524-524, add this validation before git rev-parse; in lines 638-648, validate expected_head before its git rev-parse. Also add the end-of-options -- to the git diff --no-index invocation at line 595.maintenance/control.py-481-485 (1)
481-485: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winOmitting
budgetsskips every reviewed bound.
if budgets:means a plan with nobudgetskey (or{}) is admitted with no model-call, retry, or timeout limit. Also,int()on a non-numeric value raises an uncaughtValueErrorrather than aControlError. Require the block and validate its types.🔒️ Proposed fix
budgets = plan.get("budgets", {}) - if budgets: - require(0 < int(budgets.get("maxModelCalls", 0)) <= 5, "model-call budget is outside reviewed bound") - require(0 < int(budgets.get("maxRetries", 0)) <= 3, "retry budget is outside reviewed bound") - require(0 < int(budgets.get("timeoutMinutes", 0)) <= 60, "time budget is outside reviewed bound") + require(isinstance(budgets, dict) and budgets, "plan must declare reviewed budgets") + for field, upper in (("maxModelCalls", 5), ("maxRetries", 3), ("timeoutMinutes", 60)): + value = budgets.get(field) + require(isinstance(value, int) and not isinstance(value, bool), f"{field} must be an integer") + require(0 < value <= upper, f"{field} is outside reviewed bound")🤖 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 `@maintenance/control.py` around lines 481 - 485, Update the budget validation around the budgets lookup to require a non-empty budgets block, validate that maxModelCalls, maxRetries, and timeoutMinutes are numeric integer values, and convert invalid or missing values into the existing ControlError path instead of allowing ValueError. Preserve the reviewed bounds of 1–5, 1–3, and 1–60 respectively.scripts/configure-github-maintenance-71-87 (1)
71-87: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winTighten Actions permissions:
allowed_actions: "all"and bot PR approval undercut the guard model.
allowed_actions: "all"contradicts the SHA-pinning discipline this system relies on (any third-party action becomes runnable).can_approve_pull_request_reviews: trueletsGITHUB_TOKENsatisfy review requirements, which — withrequired_approving_review_count: 0andprevent_self_review: falsebelow — means an automated run can approve its own change.🔒️ Proposed fix
gh( "api", f"repos/{args.repo}/actions/permissions", "--method", "PUT", - input_value={"enabled": True, "allowed_actions": "all"}, + input_value={"enabled": True, "allowed_actions": "selected"}, ) + gh( + "api", + f"repos/{args.repo}/actions/permissions/selected-actions", + "--method", + "PUT", + input_value={ + "github_owned_allowed": True, + "verified_allowed": False, + "patterns_allowed": [], + }, + ) gh( "api", f"repos/{args.repo}/actions/permissions/workflow", "--method", "PUT", input_value={ "default_workflow_permissions": "write", - "can_approve_pull_request_reviews": True, + "can_approve_pull_request_reviews": False, }, )🤖 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/configure-github-maintenance` around lines 71 - 87, Update the Actions permissions payloads in the configure-maintenance flow: replace the unrestricted allowed_actions value with the repository’s SHA-pinned allowlist policy, and set can_approve_pull_request_reviews to false so GITHUB_TOKEN cannot approve pull requests. Leave the remaining workflow permission settings unchanged.maintenance/control.py-41-56 (1)
41-56: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
PROTECTED_PATTERNSomits the rest of themaintenance/package, notablymaintenance/verify.py.Only
control.py,protected-paths.json, andpolicy-invariants.jsonare listed, so an admitted plan can seal edits tomaintenance/verify.py— the acceptance verifier that gates the whole system — and tomaintenance/__init__.py..github/CODEOWNERSalready treats all of/maintenance/as owner-controlled; the runtime gate should match.schemas/has the same asymmetry (schemas/agent-*only).🔒️ Proposed fix
"schemas/agent-*", - "maintenance/control.py", - "maintenance/protected-paths.json", + "schemas/*", + "maintenance/*", "scripts/admit-maintenance-plan", "scripts/seal-maintenance-patch", "scripts/verify-merge-admission", "scripts/release-maintenance", - "maintenance/policy-invariants.json", "maintenance-events/*",🤖 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 `@maintenance/control.py` around lines 41 - 56, Update PROTECTED_PATTERNS to protect the entire maintenance/ package and schemas/ directory, including maintenance/verify.py and maintenance/__init__.py, while preserving the existing specific entries and matching the owner-controlled scope used by .github/CODEOWNERS.maintenance/control.py-443-448 (1)
443-448: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
preconditions./researchSources[references bypass evidence binding.Any criterion can satisfy the evidence requirement with an arbitrary string such as
preconditions.whatever— neither prefix is resolved against the plan. Given the PR's evidence-bound premise, these references should resolve to actual keys inplan["preconditions"]andplan["researchSources"].🔒️ Proposed fix
+ precondition_refs = {f"preconditions.{key}" for key in declared_heads} + source_refs = {f"researchSources[{index}]" for index, _ in enumerate(plan.get("researchSources", []))} for result in plan["completionAssessment"]["criteria"]: for reference in result["evidence"]: require( - reference in evidence_refs or reference.startswith(("preconditions.", "researchSources[")), + reference in evidence_refs or reference in precondition_refs or reference in source_refs, f"criterion evidence reference does not resolve: {reference}", )🤖 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 `@maintenance/control.py` around lines 443 - 448, Update the evidence validation loop in the completion-assessment criteria check so preconditions.* and researchSources[...] references are resolved against the corresponding plan entries rather than accepted by prefix alone. Require each reference to map to an actual key in plan["preconditions"] or plan["researchSources"], while preserving evidence_refs validation for ordinary references.maintenance/control.py-849-894 (1)
849-894: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRetry loop has no backoff and retries non-transient failures; GitHub calls are unauthenticated.
Three immediate retries with no sleep add little over a single attempt against a flaky endpoint, and
ControlErrorfrom thecapture too largecheck at Line 854 is deterministic — retrying it twice more is wasted work. Separately, four of the sevenEVIDENCE_SOURCEShitapi.github.comwith noAuthorizationheader, so the run is subject to the 60-requests/hour unauthenticated IP limit shared across hosted runners; a 403 there recordsstatus: 403, whichwatch_decision(Line 760) turns intosource_unhealthyand stalls maintenance for a reason unrelated to the sources.♻️ Proposed fix
+import time+def capture_evidence( + output_dir: pathlib.Path, + sources: Iterable[EvidenceSource] = EVIDENCE_SOURCES, + token: str | None = None, +) -> dict[str, Any]: output_dir.mkdir(parents=True, exist_ok=True) opener = urllib.request.build_opener(RestrictedRedirect) captures = [] for source in sources: + headers = { + "Accept": "application/vnd.github+json, application/json, text/html", + "User-Agent": "bigpixelrocket-maintenance/1", + } + if token and urllib.parse.urlparse(source.url).hostname == "api.github.com": + headers["Authorization"] = f"Bearer {token}" request = urllib.request.Request( source.url, - headers={ - "Accept": "application/vnd.github+json, application/json, text/html", - "User-Agent": "bigpixelrocket-maintenance/1", - }, + headers=headers, ) last_error: Exception | None = None - for _attempt in range(3): + for attempt in range(3): + if attempt: + time.sleep(2**attempt) try:- except (OSError, urllib.error.URLError, ControlError) as error: + except ControlError as error: + last_error = error + break + except (OSError, urllib.error.URLError) as error: last_error = errorPass the workflow's
GITHUB_TOKEN(read-only scope) through thecapture-evidencesubcommand.🤖 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 `@maintenance/control.py` around lines 849 - 894, Update the capture-evidence retry loop to retry only transient network failures, add bounded backoff between attempts, and preserve immediate failure for deterministic ControlError cases such as oversized captures. Thread the workflow’s read-only GITHUB_TOKEN through capture-evidence and apply it as an Authorization header for requests to api.github.com, while retaining existing capture metadata and failure recording.scripts/admit-maintenance-plan-20-46 (1)
20-46: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMake admission freshness and replay inputs mandatory.
--php-bin-head,--mise-php-head,--policy-digest, and--completed-actionsare optional. When omitted,validate_planskips the corresponding repository/policy checks and treats completed actions as empty, so this entrypoint can admit a plan against stale state or reuse an already-completed action key. Require these inputs (or introduce an explicit, separately reviewed bootstrap mode) and fail closed when the state file is absent or malformed.Proposed admission-boundary fix
- parser.add_argument("--php-bin-head") - parser.add_argument("--mise-php-head") - parser.add_argument("--policy-digest") - parser.add_argument("--completed-actions", type=pathlib.Path) + parser.add_argument("--php-bin-head", required=True) + parser.add_argument("--mise-php-head", required=True) + parser.add_argument("--policy-digest", required=True) + parser.add_argument("--completed-actions", type=pathlib.Path, required=True)🤖 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/admit-maintenance-plan` around lines 20 - 46, Make --php-bin-head, --mise-php-head, --policy-digest, and --completed-actions required parser arguments in the admission entrypoint, and remove the optional defaults around heads and completed actions before calling validate_plan. Require the completed-actions path to exist and load it strictly, propagating failure for missing or malformed state instead of treating it as an empty set; preserve validate_plan’s admission checks using the supplied freshness and replay inputs.scripts/watch-maintenance-evidence-13-28 (1)
13-28: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winFail closed when supplied evidence paths are missing.
When
--healthis provided but the file is missing, the script silently assumes{"healthy": true}. When the required--eventspath is missing or is not a directory, it silently uses no events. A deleted or unmounted control input can therefore suppresshealth_failedandevent_incompletetriggers. Use defaults only when inputs are intentionally omitted; otherwise reject missing paths.🤖 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/watch-maintenance-evidence` around lines 13 - 28, Update the argument-loading logic around args.health and args.events to fail closed when supplied paths are missing or invalid: only use the healthy default when --health is omitted, and reject a missing or non-directory --events path instead of treating it as empty. Preserve the existing optional behavior for omitted health and the event JSON loading flow for valid directories.maintenance/protected-paths.json-3-17 (1)
3-17: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winProtect the remaining maintenance control inputs.
validate_plan()checksmaintenance/policy-invariants.jsonwhile seal/merge checks change digests againstsupport-policy.json, but runtime patches can still admit changes to operator state, pins, and validation schemas unless they are added toPROTECTED_PATTERNS; add the relevant authority/contract inputs and coverage for them.🤖 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 `@maintenance/protected-paths.json` around lines 3 - 17, The PROTECTED_PATTERNS list is missing maintenance authority and contract inputs that runtime patches must not modify. Update the patterns in maintenance/protected-paths.json to include operator state, pin, validation-schema, and support-policy paths checked by validate_plan() and the seal/merge digest validation, preserving the existing protected entries and matching the relevant paths used by those checks.scripts/watch-maintenance-evidence-26-36 (1)
26-36: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMake previous-state reconstruction deterministic.
Path.glob()does not guarantee event ordering, whilematching[-1]selects the last seenevidenceManifestDigestbefore decidingquietvsevidence_changed. Sort the events by an explicit event sequence/sorting key or raise on missing previous state when reconstruction is required.🤖 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/watch-maintenance-evidence` around lines 26 - 36, Make previous-state reconstruction in the events-loading logic deterministic: sort the JSON events using their explicit event sequence or another stable event-order key before selecting the final evidenceManifestDigest, or reject missing previous state when reconstruction is required. Update the matching[-1] selection within the !previous.get("manifestDigest") branch while preserving the existing quiet versus evidence_changed behavior.maintenance/verify.py-645-645 (1)
645-645: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
instructionDigestskeys collide across the two repositories, dropping half the digests.Every instruction path is
<repo>/.github/codex/maintenance/<name>.md, sopath.relative_to(path.parents[3])yields the identical key.github/codex/maintenance/<name>.mdfor both php-bin and mise-php. The comprehension therefore overwrites the php-bin digests with the mise-php ones: the report records 4 entries instead of 8, and a drifted php-bin instruction file is invisible in the verification evidence.🐛 Proposed fix: qualify keys by repository
- instruction_paths = [ - PHP_ROOT / ".github/codex/maintenance/shared.md", - PHP_ROOT / ".github/codex/maintenance/investigation.md", - PHP_ROOT / ".github/codex/maintenance/implementation.md", - PHP_ROOT / ".github/codex/maintenance/repair.md", - self.mise_root / ".github/codex/maintenance/shared.md", - self.mise_root / ".github/codex/maintenance/investigation.md", - self.mise_root / ".github/codex/maintenance/implementation.md", - self.mise_root / ".github/codex/maintenance/repair.md", - ] + instruction_roots = {"php-bin": PHP_ROOT, "mise-php": self.mise_root} + instruction_names = ("shared.md", "investigation.md", "implementation.md", "repair.md") + instruction_digests = { + f"{repo}/.github/codex/maintenance/{name}": sha256_file( + root / ".github/codex/maintenance" / name + ) + for repo, root in instruction_roots.items() + for name in instruction_names + }- "instructionDigests": {str(path.relative_to(path.parents[3] if ".github" in path.parts else path.parent)): sha256_file(path) for path in instruction_paths}, + "instructionDigests": instruction_digests,🤖 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 `@maintenance/verify.py` at line 645, Update the instructionDigests comprehension to include the repository identity in each key, using the path segment for the repository rather than stripping it via path.parents[3]. Ensure corresponding instruction names from php-bin and mise-php produce distinct keys while preserving sha256_file(path) values.
🟡 Minor comments (2)
maintenance/control.py-543-547 (1)
543-547: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winNon-UTF-8 patch content raises an uncaught
UnicodeDecodeError.The NUL check at Line 544 does not cover all non-text payloads; e.g. Latin-1 or UTF-16LE-without-BOM content reaches
body.decode("utf-8")and raises, escaping theControlErrorcontract thatmain()handles.🐛 Proposed fix
require(b"\0" not in body, f"patch contains binary file: {path}") - decoded = body.decode("utf-8") + try: + decoded = body.decode("utf-8") + except UnicodeDecodeError as error: + raise ControlError(f"patch file is not valid UTF-8: {path}") from error🤖 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 `@maintenance/control.py` around lines 543 - 547, Update the patch-content validation around the decode step in the candidate-processing flow to catch invalid UTF-8 and convert it into the same ControlError-based failure path used by require, with the existing patch path included in the message. Preserve the NUL and SECRET_PATTERNS checks for successfully decoded UTF-8 content.scripts/seal-maintenance-patch-22-34 (1)
22-34: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAdd
subprocess.CalledProcessErrorto the control CLI error boundary.Both wrappers call
seal_patch()/verify_merge(), which invokegitcommands that can raisesubprocess.CalledProcessError; the currentControlError/OSErrorhandlers can leave those failures as raw tracebacks. Use a shared fail-closed dispatch pattern or catch and normalize the subprocess error alongside JSON/input 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 `@scripts/seal-maintenance-patch` around lines 22 - 34, Extend the error boundary around seal_patch in scripts/seal-maintenance-patch lines 22-34 and verify_merge in scripts/verify-merge-admission lines 23-36 to catch subprocess.CalledProcessError alongside ControlError and OSError, normalizing it through the existing fail-closed CLI handling so git failures produce the standard rejection message and exit status instead of raw tracebacks.
🧹 Nitpick comments (13)
scripts/notify-maintenance (2)
157-159: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBoth scripts capture
ghstderr and then throw it away. Thegh()helper was copied into both files withstderr=subprocess.PIPE, but neither handler includeserror.stderrin its message, so any GitHub API failure reduces to a bare exit code in the workflow log.
scripts/notify-maintenance#L157-L159: appenderror.stderrto thenotification rejectedmessage.scripts/release-maintenance#L174-L176: appenderror.stderrto therelease transition rejectedmessage.🤖 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/notify-maintenance` around lines 157 - 159, Include the captured gh stderr in both error messages: update scripts/notify-maintenance lines 157-159 to append error.stderr to “notification rejected,” and scripts/release-maintenance lines 174-176 to append error.stderr to “release transition rejected,” while preserving the existing exception handling and exit behavior.
26-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate issue-lookup block in both functions.
Lines 28-46 and 69-87 are byte-identical search-and-match logic. Extract a
find_issue(repo, action_key) -> dict | Nonehelper and call it from both, so the marker format and the "multiple issues" invariant live in one place.Also applies to: 66-87
🤖 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/notify-maintenance` around lines 26 - 46, The issue-lookup and duplicate-match validation is duplicated across discover_github_prior and the other issue-discovery function. Extract the shared marker construction, GitHub issue query, body matching, and multiple-match ControlError into a find_issue(repo, action_key) helper, then have both functions call it while preserving their existing return behavior..github/workflows/maintenance-release.yml (1)
186-193: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winHealth-check loop failure is silent.
If the artifact server never becomes healthy within ~5s the loop simply exits and the run continues to
mise install, which fails with an unrelated network error. Add an explicit failure after the loop so the cause is obvious.♻️ Proposed change
- for _ in {1..50}; do curl --silent --fail "http://127.0.0.1:$port/health" && break; sleep 0.1; done + ready=false + for _ in {1..50}; do + if curl --silent --fail "http://127.0.0.1:$port/health"; then ready=true; break; fi + sleep 0.1 + done + "$ready" || { echo "staged artifact server did not become healthy" >&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/maintenance-release.yml around lines 186 - 193, Update the health-check loop in the maintenance artifact server setup to track whether a curl request succeeded, and explicitly fail with a clear server-health error after all retries are exhausted. Preserve the existing retry timing and continue to mise install only when the health endpoint responds successfully..github/workflows/maintenance-e2e.yml (1)
43-48: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueSelf-referential pin check.
The expected digest is read from
.github/maintenance-pins.jsonat the same checked-out commit as the workflow being hashed, so any change that also updates the pin passes. This only catches a forgotten pin update, not unreviewed bytes — worth a comment stating that intent, or pinning the checkout to a reviewed ref.🤖 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/maintenance-e2e.yml around lines 43 - 48, Add a comment to the “Reject unreviewed workflow bytes” step clarifying that it only detects workflow changes whose corresponding pin was not updated, or change the checkout used for .github/maintenance-pins.json to a reviewed immutable ref so the expected digest cannot be modified alongside the workflow. Keep the existing digest comparison behavior intact.scripts/prepare-agent-task (1)
97-108: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnhandled I/O and JSON errors, unlike the sibling scripts.
notify-maintenanceandrelease-maintenancewrap their bodies intry/except (ControlError, OSError, ...)and exit 1 with a one-line message; here a missing or malformed--preconditionsfile surfaces as a raw traceback in the workflow log. Wrap for consistent fail-closed diagnostics.🤖 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/prepare-agent-task` around lines 97 - 108, Wrap the main contract-building and output flow around the preconditions read/JSON parsing in the script’s existing entrypoint, catching ControlError, OSError, and JSON decoding errors consistently with notify-maintenance and release-maintenance. On failure, emit a concise one-line diagnostic and exit with status 1 instead of exposing a traceback; preserve successful contract generation unchanged.maintenance/control.py (1)
160-178: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMalformed contract shapes escape as
TypeError/AttributeErrorinstead ofControlError.
set(contract["allowedAuthority"])raisesTypeErroron unhashable elements, andcriterion.get(...)raisesAttributeErrorwhencompletionCriteriaholds non-objects. These inputs are agent-produced, andmain()(Line 977) only catchesControlError/OSError/CalledProcessError, so the gate exits with a traceback rather than a structured rejection. The outcome is still a deny, but the fail-closed diagnostics are lost.♻️ Proposed fix
require( isinstance(contract["allowedAuthority"], list), "allowedAuthority must be an array", ) + require( + all(isinstance(item, str) for item in contract["allowedAuthority"]), + "allowedAuthority must contain only strings", + ) require( not (set(contract["allowedAuthority"]) & PROHIBITED_AGENT_AUTHORITY), "agent contract grants prohibited irreversible authority", ) criteria = contract["completionCriteria"] require(isinstance(criteria, list) and criteria, "completion criteria are empty") + require(all(isinstance(item, dict) for item in criteria), "completion criteria must be objects") ids = [criterion.get("id") for criterion in criteria]🤖 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 `@maintenance/control.py` around lines 160 - 178, Update contract validation around allowedAuthority and completionCriteria to reject malformed shapes through require/ControlError rather than allowing TypeError or AttributeError to escape. Validate that allowedAuthority elements are hashable or otherwise safely comparable before the prohibited-authority set intersection, and validate each completionCriteria entry is a mapping/object before calling .get in the ids and requirement checks; preserve the existing structured rejection messages and fail-closed behavior.scripts/snapshot-github-admin-state (1)
26-34: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse
--slurpinstead of line-based page parsing.
gh api --paginatecan stream pages without an explicit one-JSON-per-line contract, andsplitlines()will break on any pretty-printed JSON. Passing--slurpwraps paginated results in one outer JSON array, removing the manual split/load/flattening.🤖 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/snapshot-github-admin-state` around lines 26 - 34, Update the GitHub API pagination flow to pass the --slurp option and parse result.stdout as one JSON document instead of splitting lines and manually flattening pages. Remove the line-based json.loads comprehension and associated empty/single-page/flattening logic, while preserving the existing return behavior for the slurped response.scripts/build.sh (1)
84-84: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winRedundant alternation, and the check passes for a universal binary.
Mach-O 64-bit executable arm64is already matched by thearm64alternative, so the second branch is dead. More importantly,fileon a fat binary lists every slice, so anx86_64 + arm64universal build satisfies this grep even though the invariant is arm64-only (schemas/policy-invariants.schema.jsonpinsarchitecture: arm64).lipo -archsgives an exact answer.♻️ Suggested tightening
-file "$PHP_BIN" | grep -Eq 'arm64|Mach-O 64-bit executable arm64' +[ "$(lipo -archs "$PHP_BIN")" = "arm64" ]🤖 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/build.sh` at line 84, Update the architecture validation in the build script to use lipo -archs on PHP_BIN and require the exact arm64-only result, rather than grepping file output. Remove the redundant alternation and ensure universal binaries containing x86_64 are rejected.maintenance/verify.py (2)
393-410: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winA08 accepts any
ControlError, not the protected-path rejection.
assert_rejectis called withoutcontains, so an unrelated fail-closed rejection (e.g. the completion assessment or base-commit check firing first) still marks the forbidden-diff test as passed. For the security-critical cases, pin the expected reason as done elsewhere in this file.♻️ Suggested tightening
- assert_reject(lambda repo=repo, base=base, plan=plan, result=result, contract=contract, index=index: seal_patch(repo, base, plan, result, contract, directory / f"sealed-{index}")) + expected = "unadmitted path" if path == "unadmitted.txt" else "protected path" + assert_reject( + lambda repo=repo, base=base, plan=plan, result=result, contract=contract, index=index: seal_patch( + repo, base, plan, result, contract, directory / f"sealed-{index}" + ), + expected, + )🤖 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 `@maintenance/verify.py` around lines 393 - 410, Update the protected-path rejection checks in the loop over protected_classes to pass the expected protected-path rejection reason via assert_reject’s contains parameter. Ensure each seal_patch failure must specifically identify the forbidden-path rejection, rather than accepting any ControlError.
494-497: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePermission gating relies on exact YAML text.
"permissions:\n contents: read" in watchpasses only for one specific serialization; adding a second permission key or reindenting silently breaks the check even though the policy is unchanged (and, worse, apermissions:block that starts withcontents: readbut also grantscontents: writeelsewhere would still match). Parsing the workflow and asserting on the resolved permission map would make A13 robust.🤖 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 `@maintenance/verify.py` around lines 494 - 497, The permission check in the verification flow should parse the workflow YAML instead of matching exact text. Update the logic around watch and release in verify.py to inspect the resolved permissions map, require the runtime investigation workflow’s contents permission to be read-only, and reject any configuration that grants contents write regardless of key order or indentation.schemas/agent-task-contract.schema.json (1)
15-28: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider guarding against duplicate criterion
ids.
validate_completion_assessmentinmaintenance/control.pyderivesexpected_idsas a set fromcompletionCriteria, so duplicate ids in a contract collapse silently and weaken the "all criteria reported" check. Adding"uniqueItems": truehere only rejects fully identical objects, so the stronger guard belongs invalidate_task_contract(reject duplicate ids explicitly).🤖 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 `@schemas/agent-task-contract.schema.json` around lines 15 - 28, Update validate_task_contract to explicitly detect duplicate completionCriteria id values and reject the contract when any id appears more than once. Keep the existing completionCriteria schema validation intact, and do not rely on uniqueItems because uniqueness must be based specifically on the criterion id field.schemas/maintenance-plan.schema.json (1)
6-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSchema omits fields the validator enforces (
agentOperations,budgets,releaseIntentshape).
validate_planinmaintenance/control.pychecksagentOperationsagainstPROHIBITED_AGENT_AUTHORITYand enforces reviewed bounds onbudgets.maxModelCalls/maxRetries/timeoutMinutes, and requiresreleaseIntent.versionto match the stable pattern — none of which appear here. Since plan authors (Codex prompts) work from this schema, declaring them keeps the authored contract and the runtime gate aligned.♻️ Suggested property declarations
"requiredChecks": {"type": "array", "items": {"type": "string"}}, - "releaseIntent": {"type": ["object", "null"]}, + "releaseIntent": { + "type": ["object", "null"], + "properties": { + "version": {"type": "string", "pattern": "^\\d+\\.\\d+\\.\\d+$"}, + "sourceIdentifier": {"type": "string", "minLength": 1} + }, + "required": ["version"] + }, + "agentOperations": {"type": "array", "items": {"type": "string"}, "uniqueItems": true}, + "budgets": { + "type": "object", + "properties": { + "maxModelCalls": {"type": "integer", "minimum": 1, "maximum": 5}, + "maxRetries": {"type": "integer", "minimum": 1, "maximum": 3}, + "timeoutMinutes": {"type": "integer", "minimum": 1, "maximum": 60} + } + }, "notification": {"type": "object"},🤖 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 `@schemas/maintenance-plan.schema.json` around lines 6 - 23, Update the maintenance-plan schema’s properties to declare the validator-enforced agentOperations, budgets, and releaseIntent fields. Model agentOperations to support the PROHIBITED_AGENT_AUTHORITY validation, budgets with reviewed maxModelCalls, maxRetries, and timeoutMinutes bounds, and releaseIntent with a version matching the stable required pattern while preserving its nullable form.tests/test_maintenance.py (1)
20-91: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider covering
seal_patch/verify_mergehere, not only in the cross-repo verifier.The protected-path and exact-SHA merge gates are the highest-risk fail-closed paths, but they are only exercised by
maintenance/verify.pyA08/A12, which need amise-phpcheckout and cannot run fromscripts/test.sh. A couple oftempfile-backed git fixtures here would keep those invariants under the routine gate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ac84575c-cfc7-4744-8c37-7738c2b79b03
📒 Files selected for processing (51)
.codex/implementation.config.toml.codex/investigation.config.toml.codex/repair.config.toml.github/CODEOWNERS.github/codex/maintenance/implementation.md.github/codex/maintenance/investigation.md.github/codex/maintenance/repair.md.github/codex/maintenance/shared.md.github/maintenance-operator.json.github/maintenance-pins.json.github/workflows/build.yml.github/workflows/ci.yml.github/workflows/maintenance-e2e.yml.github/workflows/maintenance-implementation.yml.github/workflows/maintenance-release.yml.github/workflows/maintenance-watch.yml.github/workflows/release.yml.gitignoreREADME.mddocs/repository-settings.mdmaintenance-events/.gitkeepmaintenance-state/.gitkeepmaintenance/__init__.pymaintenance/control.pymaintenance/policy-invariants.jsonmaintenance/protected-paths.jsonmaintenance/verify.pyschemas/agent-completion-assessment.schema.jsonschemas/agent-task-contract.schema.jsonschemas/maintenance-event.schema.jsonschemas/maintenance-plan.schema.jsonschemas/policy-invariants.schema.jsonschemas/support-policy.schema.jsonscripts/admit-maintenance-planscripts/build.shscripts/capture-maintenance-evidencescripts/configure-github-maintenancescripts/maintenance-eventscripts/notify-maintenancescripts/prepare-agent-taskscripts/release-maintenancescripts/seal-maintenance-patchscripts/serve-maintenance-artifactscripts/snapshot-github-admin-statescripts/test.shscripts/validate-maintenance-archivescripts/verify-maintenance-systemscripts/verify-merge-admissionscripts/watch-maintenance-evidencesupport-policy.jsontests/test_maintenance.py
💤 Files with no reviewable changes (1)
- .github/workflows/release.yml
1 participant
Summary
Implements the php-bin side of
PLAN-HYBRID-AI.md: evidence-bound investigation, offline implementation and repair, deterministic admission and sealing, exact-SHA merge gates, immutable release transactions, durable events, notifications, and production-parity verification.The review follow-up also hardens input binding, archive and evidence path handling, mandatory budgets and replay state, selected Action permissions, and routine exact-merge tests.
Verification
scripts/test.shA13 is intentionally completed during live GitHub administration after merge. A21 requires inbox observation, and A22 requires the next real upstream stable PHP patch.
Security and licensing