pdurlej/platform

Fork 0

fix(platformctl): emit approval failure reasons #659

Merged

pdurlej merged 1 commit from codex/191-approval-failure-reasons into main

2026-06-01 14:03:18 +02:00

codex commented

2026-06-01 08:56:11 +02:00

Collaborator

Canary status: missing - platformctl apply approval hardening; run canary manually before merge if branch policy requires it.

Canary Context Pack

Product story

CI/audit consumers should be able to distinguish why an apply approval failed without scraping human text or exposing Forgejo response bodies.

What changed

Added stable enum-like approval failure reasons to ApprovalCheck, mapped common failure paths, and surfaced approval_reason in failed platformctl apply JSON results.

Why it changed

Issue #191 / ERROR-CODE-01 asked for machine-readable approval failure reasons for missing token, network/API errors, SHA mismatch, unmerged PR, PR-not-found, and related approval failures.

Files touched

control-plane/platformctl/apply.py
control-plane/platformctl/tests/test_apply_phase3.py

Relevant context

ApprovalCheck now carries an optional reason field.
Human-facing detail remains redacted/safe and unchanged in spirit.
Forgejo response bodies remain omitted from apply results/log summaries.

Runtime evidence

No runtime commands were run. Validation:

UV_CACHE_DIR=/private/tmp/codex-uv-cache uv run pytest platformctl/tests/test_apply_phase3.py -k 'verify_approved_sha or check_specific_pr or apply_plan_rejects_failed_approval'
UV_CACHE_DIR=/private/tmp/codex-uv-cache uv run pytest platformctl/tests/test_apply_phase3.py platformctl/tests/test_apply_env_file.py platformctl/tests/test_pr_sanity.py
PYTHONPATH=control-plane UV_CACHE_DIR=/private/tmp/codex-uv-cache uv run --project control-plane python -m platformctl.cli validate all --json

Results: targeted 14 passed; broader 133 passed; validate all exitCode 0.

Known constraints

This PR does not change approval pass/fail decisions. It only attaches stable reason strings to existing failure decisions.

Explicit out-of-scope

No apply transport change.
No production runtime mutation.
No new Forgejo write path.
No PR/base/actor binding changes from neighboring M06 issues.

Requested decision

Approve if the reason names are stable enough for CI/audit consumers.

Merge blockers

A reason conflates two states that downstream automation must distinguish.
The new field could leak secret-bearing error text. It should not: reason is a fixed string and detail remains scrubbed/summarized.

Spec sources read

Issue #191 - scope and acceptance sketch.
control-plane/platformctl/apply.py - approval verification and apply failure JSON path.
control-plane/platformctl/tests/test_apply_phase3.py - approval/apply regression tests.
state/memory/task-run-schema.md - adjacent observability contract, checked to avoid inventing a parallel metric surface.

Closes #191

Canary status: missing - platformctl apply approval hardening; run canary manually before merge if branch policy requires it. ## Canary Context Pack ### Product story CI/audit consumers should be able to distinguish why an apply approval failed without scraping human text or exposing Forgejo response bodies. ### What changed Added stable enum-like approval failure reasons to `ApprovalCheck`, mapped common failure paths, and surfaced `approval_reason` in failed `platformctl apply` JSON results. ### Why it changed Issue #191 / ERROR-CODE-01 asked for machine-readable approval failure reasons for missing token, network/API errors, SHA mismatch, unmerged PR, PR-not-found, and related approval failures. ### Files touched - `control-plane/platformctl/apply.py` - `control-plane/platformctl/tests/test_apply_phase3.py` ### Relevant context - `ApprovalCheck` now carries an optional `reason` field. - Human-facing `detail` remains redacted/safe and unchanged in spirit. - Forgejo response bodies remain omitted from apply results/log summaries. ### Runtime evidence No runtime commands were run. Validation: ```bash UV_CACHE_DIR=/private/tmp/codex-uv-cache uv run pytest platformctl/tests/test_apply_phase3.py -k 'verify_approved_sha or check_specific_pr or apply_plan_rejects_failed_approval' UV_CACHE_DIR=/private/tmp/codex-uv-cache uv run pytest platformctl/tests/test_apply_phase3.py platformctl/tests/test_apply_env_file.py platformctl/tests/test_pr_sanity.py PYTHONPATH=control-plane UV_CACHE_DIR=/private/tmp/codex-uv-cache uv run --project control-plane python -m platformctl.cli validate all --json ``` Results: targeted 14 passed; broader 133 passed; `validate all` exitCode 0. ### Known constraints This PR does not change approval pass/fail decisions. It only attaches stable reason strings to existing failure decisions. ### Explicit out-of-scope - No apply transport change. - No production runtime mutation. - No new Forgejo write path. - No PR/base/actor binding changes from neighboring M06 issues. ### Requested decision Approve if the reason names are stable enough for CI/audit consumers. ### Merge blockers - A reason conflates two states that downstream automation must distinguish. - The new field could leak secret-bearing error text. It should not: reason is a fixed string and detail remains scrubbed/summarized. ## Spec sources read - Issue #191 - scope and acceptance sketch. - `control-plane/platformctl/apply.py` - approval verification and apply failure JSON path. - `control-plane/platformctl/tests/test_apply_phase3.py` - approval/apply regression tests. - `state/memory/task-run-schema.md` - adjacent observability contract, checked to avoid inventing a parallel metric surface. Closes #191

codex added this to the 06 - Agent execution and CI governance milestone

2026-06-01 08:56:11 +02:00

codex added the

class/security-sensitive

label

2026-06-01 08:56:11 +02:00

codex added 1 commit

2026-06-01 08:56:11 +02:00

fix(platformctl): emit approval failure reasons

canary-required / collect-diff (pull_request) Successful in 6s

Details

platformctl plan / auto-apply scope (pull_request) Successful in 24s

Details

pyfallow / Pyfallow gate (control-plane) (pull_request) Successful in 23s

Details

python-ci / Python 3.11 (pull_request) Successful in 45s

Details

python-ci / Python 3.12 (pull_request) Successful in 49s

Details

python-ci / Python 3.13 (pull_request) Successful in 49s

Details

base-is-main / guard (pull_request) Successful in 2s

Details

canary-required / canary (pull_request) Successful in 17s

Details

patchwarden-client-dry-run / dry-run (pull_request) Successful in 26s

Details

patchwarden-client-dry-run / collect-diff (pull_request) Successful in 7s

Details

patchwarden-pr-sanity / sanity (pull_request) Successful in 4m58s

Details

patchwarden-pr-sanity / collect-diff (pull_request) Successful in 8s

Details

01209433d3

codex commented

2026-06-01 09:53:57 +02:00

Author

Collaborator

Patchwarden PR sanity

Status: advisory_findings
PR: 659
Commit: 01209433d374e744fdda8cf7098f7bd5360783e6
Security-sensitive label: present
Authority: advisory model review plus deterministic blockers only
3+3 canary: still alive; this does not replace it

Deterministic findings

info sensitive-path-touched Sensitive path touched — control-plane/platformctl/apply.py
- Evidence: control-plane/platformctl/apply.py
- Next: Route through the existing 3+3/risk-tier process; model review remains advisory.

Model reviewers

`global-glm` / `glm-5.1:cloud`

Status: ok
Verdict: NOT_OK
medium Misleading fallback reason when approval.reason is None
- Evidence: control-plane/platformctl/apply.py line 746: "approval_reason": approval.reason or APPROVAL_REASON_FORGEJO_API_ERROR — if a future failure path omits setting reason, or reason is None, this defaults to 'forgejo_api_error' which misclassif
- Next: Add APPROVAL_REASON_UNKNOWN constant and use it as the fallback instead of APPROVAL_REASON_FORGEJO_API_ERROR, or add an assertion/guard that reason is never None when approved is False
low Unrecognized RuntimeError misclassified as Forgejo API error
- Evidence: control-plane/platformctl/apply.py lines 191-192: _approval_reason_for_error returns APPROVAL_REASON_FORGEJO_API_ERROR as default fallback for any RuntimeError not matching known patterns, conflating 'unknown error' with 'known Forgejo AP
- Next: Return APPROVAL_REASON_UNKNOWN (or similar) for unrecognized errors instead of defaulting to a specific error category

`global-deepseek` / `deepseek-v4-pro:cloud`

Status: ok
Verdict: OK
medium ValueError misclassification as config_error may mislead automation
- Evidence: control-plane/platformctl/apply.py: _approval_reason_for_errormaps anyValueErrortoAPPROVAL_REASON_CONFIG_ERROR. Non-config ValueErrorinstances (e.g., from_repo_api_pathparsing) will be labeled as config errors, which could
- Next: Narrow the ValueError check to specific config-related exception types or introduce a more generic reason for unexpected ValueError cases.
low Fallback reason in apply_plan may hide missing reason assignments
- Evidence: control-plane/platformctl/apply.py: apply_plansetsapproval_reasontoapproval.reason or APPROVAL_REASON_FORGEJO_API_ERROR. If a future code path returns an ApprovalCheckwithreason=None, the output will incorrectly indicate a F
- Next: Ensure all ApprovalCheck failure paths set a reason, and consider logging a warning when reason is None during apply result construction.

`redteam` / `kimi-k2.6:cloud`

Status: error
Verdict: -
Note: ReadTimeout: The read operation timed out
Findings: none

Policy notes

GLM 5.1 + DeepSeek V4 Pro are the operator-required model mix for this bot.
Optional red-team model is enabled only when PLATFORMCTL_PR_SANITY_REDTEAM_MODEL is configured.
Auto-merge is not enabled here.

# Patchwarden PR sanity - Status: `advisory_findings` - PR: `659` - Commit: `01209433d374e744fdda8cf7098f7bd5360783e6` - Security-sensitive label: `present` - Authority: advisory model review plus deterministic blockers only - 3+3 canary: still alive; this does not replace it ## Deterministic findings - **`info` `sensitive-path-touched`** Sensitive path touched — `control-plane/platformctl/apply.py` - Evidence: `control-plane/platformctl/apply.py` - Next: Route through the existing 3+3/risk-tier process; model review remains advisory. ## Model reviewers ### `global-glm` / `glm-5.1:cloud` - Status: `ok` - Verdict: `NOT_OK` - **`medium`** Misleading fallback reason when approval.reason is None - Evidence: `control-plane/platformctl/apply.py line 746: `"approval_reason": approval.reason or APPROVAL_REASON_FORGEJO_API_ERROR` — if a future failure path omits setting reason, or reason is None, this defaults to 'forgejo_api_error' which misclassif` - Next: Add APPROVAL_REASON_UNKNOWN constant and use it as the fallback instead of APPROVAL_REASON_FORGEJO_API_ERROR, or add an assertion/guard that reason is never None when approved is False - **`low`** Unrecognized RuntimeError misclassified as Forgejo API error - Evidence: `control-plane/platformctl/apply.py lines 191-192: `_approval_reason_for_error` returns APPROVAL_REASON_FORGEJO_API_ERROR as default fallback for any RuntimeError not matching known patterns, conflating 'unknown error' with 'known Forgejo AP` - Next: Return APPROVAL_REASON_UNKNOWN (or similar) for unrecognized errors instead of defaulting to a specific error category ### `global-deepseek` / `deepseek-v4-pro:cloud` - Status: `ok` - Verdict: `OK` - **`medium`** ValueError misclassification as config_error may mislead automation - Evidence: `control-plane/platformctl/apply.py: `_approval_reason_for_error` maps any `ValueError` to `APPROVAL_REASON_CONFIG_ERROR`. Non-config `ValueError` instances (e.g., from `_repo_api_path` parsing) will be labeled as config errors, which could ` - Next: Narrow the `ValueError` check to specific config-related exception types or introduce a more generic reason for unexpected `ValueError` cases. - **`low`** Fallback reason in apply_plan may hide missing reason assignments - Evidence: `control-plane/platformctl/apply.py: `apply_plan` sets `approval_reason` to `approval.reason or APPROVAL_REASON_FORGEJO_API_ERROR`. If a future code path returns an `ApprovalCheck` with `reason=None`, the output will incorrectly indicate a F` - Next: Ensure all `ApprovalCheck` failure paths set a reason, and consider logging a warning when `reason` is `None` during apply result construction. ### `redteam` / `kimi-k2.6:cloud` - Status: `error` - Verdict: `-` - Note: ReadTimeout: The read operation timed out - Findings: none ## Policy notes - GLM 5.1 + DeepSeek V4 Pro are the operator-required model mix for this bot. - Optional red-team model is enabled only when `PLATFORMCTL_PR_SANITY_REDTEAM_MODEL` is configured. - Auto-merge is not enabled here.