fix(platformctl): bind apply approval to Forgejo PR #162

Merged

pdurlej merged 2 commits from codex/issues/142-apply-approval-binding into main 2026-05-12 01:28:22 +02:00

Conversation 4 Commits 2 Files changed 3 +397 -20

codex commented 2026-05-10 23:26:31 +02:00

Collaborator

Copy link

Canary status: missing — fire canary 3+3 manually before merge

Canary Context Pack

Product story

Phase 3 apply must never treat an arbitrary SHA as production approval. This slice makes approval verification real and bound to Forgejo PR identity before any later runtime path can use it.

What changed

• Added Forgejo PR approval lookup for platformctl apply.
• Added --approved-pr so the operator/action can bind apply to one merged PR.
• Removed the old 40-hex-only approval stub behavior.
• Added unit and CLI tests for approval success/failure.

Why it changed

PR #160 review found that the apply path was too broad and approval binding was not reviewable inside the larger runtime mutation PR.

Files touched

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

Relevant context

Split from superseded PR #160. Stacked on #159 / codex/issues/142-phase3-plan.

Runtime evidence

No runtime mutation in this PR. Local tests only.

Known constraints

Requires PLATFORMCTL_FORGEJO_TOKEN or actor-scoped token env at apply time.

Explicit out-of-scope

Plan provenance, compose execution, status artifacts, and Forgejo Actions wiring are later split PRs.

Requested decision

Approve this as the approval-binding foundation for the stacked apply split.

Merge blockers

Any path that accepts an unmerged PR, wrong actor identity, or arbitrary SHA as approval.

Spec sources read

• AGENTS.md — repo workflow, canary, security-sensitive lane.
• control-plane/platformctl/apply.py — apply approval gate.
• control-plane/platformctl/cli.py — apply CLI contract.
• control-plane/platformctl/tests/test_apply_phase3.py — new contract tests.

Refs #142
Supersedes part of #160

Canary status: missing — fire canary 3+3 manually before merge ## Canary Context Pack ### Product story Phase 3 apply must never treat an arbitrary SHA as production approval. This slice makes approval verification real and bound to Forgejo PR identity before any later runtime path can use it. ### What changed - Added Forgejo PR approval lookup for `platformctl apply`. - Added `--approved-pr` so the operator/action can bind apply to one merged PR. - Removed the old 40-hex-only approval stub behavior. - Added unit and CLI tests for approval success/failure. ### Why it changed PR #160 review found that the apply path was too broad and approval binding was not reviewable inside the larger runtime mutation PR. ### Files touched - `control-plane/platformctl/apply.py` - `control-plane/platformctl/cli.py` - `control-plane/platformctl/tests/test_apply_phase3.py` ### Relevant context Split from superseded PR #160. Stacked on #159 / `codex/issues/142-phase3-plan`. ### Runtime evidence No runtime mutation in this PR. Local tests only. ### Known constraints Requires `PLATFORMCTL_FORGEJO_TOKEN` or actor-scoped token env at apply time. ### Explicit out-of-scope Plan provenance, compose execution, status artifacts, and Forgejo Actions wiring are later split PRs. ### Requested decision Approve this as the approval-binding foundation for the stacked apply split. ### Merge blockers Any path that accepts an unmerged PR, wrong actor identity, or arbitrary SHA as approval. ## Spec sources read - `AGENTS.md` — repo workflow, canary, security-sensitive lane. - `control-plane/platformctl/apply.py` — apply approval gate. - `control-plane/platformctl/cli.py` — apply CLI contract. - `control-plane/platformctl/tests/test_apply_phase3.py` — new contract tests. Refs #142 Supersedes part of #160

codex added 1 commit 2026-05-10 23:26:31 +02:00

fix(platformctl): bind apply approval to Forgejo PR c39d909968

codex added the

class/security-sensitive

phase/03

risk/process

risk/runtime

labels 2026-05-10 23:26:31 +02:00

codex referenced this pull request 2026-05-10 23:29:31 +02:00

feat(platformctl): implement approved apply path #160

codex referenced this pull request 2026-05-10 23:29:31 +02:00

[coordination-lane] Cutover Flight Phase 2: v2 catalog to gate #142

pdurlej changed target branch from codex/issues/142-phase3-plan to main 2026-05-10 23:34:49 +02:00

codex commented 2026-05-11 00:53:47 +02:00

Author

Collaborator

Copy link

Ralph review (5-iter chmurowy) — ITERATE_BLOCKER 3/9

Niezależny 5-iter ralph chain (kimi-k2.6 planner → glm-5.1 critic → deepseek-v4-pro red-team → kimi-k2.6 synth → deepseek-v4-pro arbiter) na tym PR. Verdict + drafted patches poniżej.

Per-dim scoring

• Correctness: 4/9
• Security: 2/9 ⚠️ (arbitrary-SHA bypass + error body leak)
• Observability: 5/9
• Test coverage: 7/9
• Scope discipline: 8/9

Evidence: ~/Iskra-i-Piotr/05 System/Swarmheart Backups/ralph-phase3-apply/162/ (full chain).

---

BLOCKER 1 — Remove paginated search fallback

WHAT: verify_approved_sha (lines 183-207) ma fallback który gdy approved_pr is None searches ostatnich 250 closed PRs. Akceptuje arbitrary historical merge-commit SHA jako approval.

WHY: Red-team confirmed-exploitable:

# Insider z platform access:
SHA=$(git log --merges --format=%H origin/main | tail -1)   # public!
platformctl apply malicious-plan.json --approved $SHA       # ✓ approved

Public historical merge SHA = golden ticket dla arbitrary production change. Bezpośrednio łamie deklarowany kontrakt PR-a: "approval must be bound to a specific Forgejo PR identity".

HOW (drafted patch w apply.py linie 151-209):

def verify_approved_sha(
    approved_sha: str,
    *,
    approved_pr: int | None = None,
    forgejo_host: str = DEFAULT_FORGEJO_HOST,
    forgejo_repo: str = DEFAULT_FORGEJO_REPO,
    token: str | None = None,
    search_pages: int = 5,  # kept for compat; unused after fallback removal
) -> ApprovalCheck:
    """Verify SHA matches a merged Forgejo PR. Requires explicit --approved-pr."""
    if not _valid_sha(approved_sha):
        return ApprovalCheck(False, "approved SHA must be a 40-character hex commit")

    if approved_pr is None:
        # SECURITY: search fallback removed (ralph #162 BLOCKER 1).
        # Arbitrary historical merge SHAs must NOT satisfy approval.
        return ApprovalCheck(
            False,
            "--approved-pr is required (search fallback removed; bind apply to explicit PR)",
        )

    api_token = _forgejo_token(token)
    if not api_token:
        return ApprovalCheck(False, "PLATFORMCTL_FORGEJO_TOKEN is required for apply approval verification")

    try:
        repo_path = _repo_api_path(forgejo_repo)
        return _check_specific_pr(
            approved_sha, approved_pr,
            repo_path=repo_path, host=forgejo_host, token=api_token,
        )
    except (RuntimeError, ValueError) as exc:
        return ApprovalCheck(False, _safe_error_summary(exc))

Delete entire for page in range(...) block + final fallback return.

VERIFY (add to tests/test_apply_phase3.py):

def test_verify_approved_sha_requires_approved_pr():
    """approved_pr=None must reject — search fallback removed (ralph BLOCKER 1)."""
    result = verify_approved_sha("a" * 40, approved_pr=None)
    assert not result.approved
    assert "--approved-pr is required" in result.detail


def test_verify_approved_sha_with_pr_still_works(mocker):
    """Explicit --approved-pr path unchanged."""
    mocker.patch("...module._check_specific_pr", return_value=ApprovalCheck(True, "ok", pr_number=42))
    mocker.patch.dict(os.environ, {"PLATFORMCTL_FORGEJO_TOKEN": "fake"})
    result = verify_approved_sha("a" * 40, approved_pr=42)
    assert result.approved is True

---

BLOCKER 2 — Decouple _pull_files from approval decision

WHAT: _check_specific_pr (linia 147) calls _pull_files jako część successful approval return. Jeśli /files endpoint fails (500, timeout, malformed JSON), całe approval is rejected.

WHY: Forgejo API flakiness convertuje valid SHA↔PR approval w hard rejection → pressure na operator do użycia --break-glass dla otherwise legitimate changes. Files lookup jest enrichment, nie gating decision.

HOW (drafted patch w apply.py linie 115-148):

def _check_specific_pr(
    approved_sha: str,
    approved_pr: int,
    *,
    repo_path: str,
    host: str,
    token: str,
) -> ApprovalCheck:
    pr = _forgejo_get_json(
        f"/api/v1/repos/{repo_path}/pulls/{approved_pr}",
        host=host, token=token,
    )
    if not isinstance(pr, dict):
        return ApprovalCheck(False, f"PR #{approved_pr} lookup returned non-object payload")
    if not _is_pr_merged(pr):
        return ApprovalCheck(False, f"PR #{approved_pr} is not merged", pr_number=approved_pr)

    sha_values = _pr_sha_values(pr)
    if approved_sha not in sha_values:
        return ApprovalCheck(
            False,
            f"approved SHA does not match PR #{approved_pr} head/merge SHA",
            pr_number=approved_pr,
            head_sha=(pr.get("head") or {}).get("sha") if isinstance(pr.get("head"), dict) else None,
            merge_sha=pr.get("merge_commit_sha") or pr.get("merged_commit_sha"),
        )

    # Approval is decided. Files lookup is enrichment; never gate on /files flakiness.
    # (ralph #162 BLOCKER 2)
    changed_files: tuple[str, ...] = ()
    try:
        changed_files = _pull_files(approved_pr, repo_path=repo_path, host=host, token=token)
    except (RuntimeError, ValueError):
        pass  # /files unavailable — emit approval anyway with empty file list

    return ApprovalCheck(
        True,
        f"approved SHA matches merged PR #{approved_pr}",
        pr_number=approved_pr,
        head_sha=(pr.get("head") or {}).get("sha") if isinstance(pr.get("head"), dict) else None,
        merge_sha=pr.get("merge_commit_sha") or pr.get("merged_commit_sha"),
        changed_files=changed_files,
    )

VERIFY:

def test_check_specific_pr_succeeds_when_files_endpoint_fails(mocker):
    """Approval must succeed even if /files returns 500 (ralph BLOCKER 2)."""
    mocker.patch("...module._forgejo_get_json", return_value={
        "merged": True,
        "head": {"sha": "a" * 40},
    })
    mocker.patch("...module._pull_files", side_effect=RuntimeError("500 Internal"))
    result = _check_specific_pr("a" * 40, 42, repo_path="x/y", host="h", token="t")
    assert result.approved is True
    assert result.changed_files == ()

---

BLOCKER 3 — Sanitize Forgejo error bodies (defense-in-depth)

WHAT: _forgejo_get_json (linia 79) embeds body[:200] z Forgejo error response w RuntimeError. Body flows do ApprovalCheck.detail → result dict → audit logs.

WHY: Forgejo error responses mogą zawierać: internal paths, user IDs, w trybie debug nawet tokens, stack traces. Audit logs będą emit'owane do CI logs (potentially public). Defense-in-depth.

HOW (drafted patch w apply.py linie 64-81):

import logging
_LOG = logging.getLogger(__name__)


def _safe_error_summary(exc: BaseException) -> str:
    """Strip body details from Forgejo exception flowing into ApprovalCheck.detail."""
    msg = str(exc)
    # Only let known-safe prefix patterns through; never raw response bodies.
    safe_prefixes = ("Forgejo API returned HTTP ", "Forgejo API request failed", "PR #", "approved SHA")
    if any(msg.startswith(p) for p in safe_prefixes):
        return msg
    return f"{type(exc).__name__}"


def _forgejo_get_json(path: str, *, host: str, token: str) -> Any:
    url = host.rstrip("/") + path
    request = urllib.request.Request(
        url,
        headers={
            "Accept": "application/json",
            "Authorization": f"token {token}",
            "User-Agent": "platformctl-apply",
        },
    )
    try:
        with urllib.request.urlopen(request, timeout=20) as response:
            return json.loads(response.read().decode("utf-8"))
    except urllib.error.HTTPError as exc:
        # SECURITY (ralph #162 BLOCKER 3): do NOT embed response body —
        # may leak internal paths, user IDs, debug tokens. Log to debug-only.
        try:
            raw_body = exc.read().decode("utf-8", errors="replace")[:1024]
            _LOG.debug("Forgejo %s → HTTP %s body=%r", path, exc.code, raw_body)
        except Exception:  # noqa: BLE001
            pass
        raise RuntimeError(f"Forgejo API returned HTTP {exc.code} for {path} (response omitted)") from exc
    except urllib.error.URLError as exc:
        raise RuntimeError(f"Forgejo API request failed for {path}: {type(exc.reason).__name__}") from exc

Plus apply _safe_error_summary w verify_approved_sha final except clause (zamiast str(exc) na linii 209).

VERIFY:

def test_forgejo_get_json_omits_response_body(monkeypatch):
    """RuntimeError must NOT include response body content (ralph BLOCKER 3)."""
    class FakeHTTPError(urllib.error.HTTPError):
        def __init__(self):
            super().__init__("u", 500, "Internal", {}, None)
        def read(self):
            return b'{"secret_token": "supersensitive", "internal_path": "/var/forgejo/secrets/x"}'

    def fake_urlopen(*a, **kw):
        raise FakeHTTPError()
    monkeypatch.setattr(urllib.request, "urlopen", fake_urlopen)

    with pytest.raises(RuntimeError) as ei:
        _forgejo_get_json("/api/test", host="h", token="t")
    assert "supersensitive" not in str(ei.value)
    assert "internal_path" not in str(ei.value)
    assert "response omitted" in str(ei.value)

---

Follow-up issues (file as separate)

BREAK-GLASS-HARDEN-01 — Require secondary approval for --break-glass

apply_plan (line ~267) accepts --break-glass --reason "<any-string>" jako total bypass całej Forgejo gate. Audit trail = log line + free-text reason. Insider z CLI access bypasses everything.

Proposed: secondary signed approval. Options: (a) GPG-signed break-glass token, (b) second operator Forgejo comment z magic string na tracking issue, (c) PagerDuty incident ack ID. Track failed break-glass attempts w audit.

Class: security-sensitive, phase/03, risk/process

PR-BIND-01 — Verify approved PR base.ref matches deployment branch

_check_specific_pr validates merged + SHA, ale nigdy nie sprawdza pr.base.ref. PR merged do release/staging lub feature branch satisfy main-line apply approval.

Proposed: pass expected_base_ref param do verify_approved_sha (default main), reject jeśli pr.base.ref != expected_base_ref.

Class: security-sensitive, phase/03

ACTOR-ID-01 — Bind apply actor identity to PR merger/reviewers

Merge blockers state że "wrong actor identity must be rejected", ale no actor check istnieje. Operator running apply jest unvalidated against PR's merger / reviewers / CODEOWNERS.

Proposed: capture merged_by.login z PR object, accept optional --actor-identity <login> (or env), reject mismatch chyba że --allow-foreign-actor. Może mieć overlap z #163 plan provenance.

Class: security-sensitive, phase/03

ERROR-CODE-01 — Machine-readable approval failure exit codes

ApprovalCheck(False, str) jest human-friendly ale CI/auditors nie mogą distinguish "Forgejo unreachable" od "SHA mismatch" od "missing token". Wszystko EXIT_APPROVAL_MISMATCH.

Proposed: add ApprovalCheck.reason: enum (NETWORK, SHA_MISMATCH, NOT_MERGED, MISSING_TOKEN, PR_NOT_FOUND). Map do distinct exit codes (EXIT_APPROVAL_NETWORK=12, etc.).

Class: observability, phase/03

---

Action items

1. Apply 3 BLOCKER patches → apply.py + tests
2. Force-push do codex/issues/142-apply-approval-binding
3. File 4 follow-up issues (titles above)
4. Comment "ready for re-review" tutaj
5. Operator może odpalić re-ralph dla potwierdzenia że score ≥7/9

— ralph batch 2026-05-10, claude-opus-4.7 (Pan Herbata) dispatching via codex identity

## Ralph review (5-iter chmurowy) — ITERATE_BLOCKER 3/9 Niezależny 5-iter ralph chain (kimi-k2.6 planner → glm-5.1 critic → deepseek-v4-pro red-team → kimi-k2.6 synth → deepseek-v4-pro arbiter) na tym PR. Verdict + drafted patches poniżej. ### Per-dim scoring - Correctness: 4/9 - **Security: 2/9** ⚠️ (arbitrary-SHA bypass + error body leak) - Observability: 5/9 - Test coverage: 7/9 - Scope discipline: 8/9 Evidence: `~/Iskra-i-Piotr/05 System/Swarmheart Backups/ralph-phase3-apply/162/` (full chain). --- ## BLOCKER 1 — Remove paginated search fallback **WHAT:** `verify_approved_sha` (lines 183-207) ma fallback który gdy `approved_pr is None` searches ostatnich 250 closed PRs. Akceptuje arbitrary historical merge-commit SHA jako approval. **WHY:** Red-team confirmed-exploitable: ```bash # Insider z platform access: SHA=$(git log --merges --format=%H origin/main | tail -1) # public! platformctl apply malicious-plan.json --approved $SHA # ✓ approved ``` Public historical merge SHA = golden ticket dla arbitrary production change. Bezpośrednio łamie deklarowany kontrakt PR-a: *"approval must be bound to a specific Forgejo PR identity"*. **HOW (drafted patch w `apply.py` linie 151-209):** ```python def verify_approved_sha( approved_sha: str, *, approved_pr: int | None = None, forgejo_host: str = DEFAULT_FORGEJO_HOST, forgejo_repo: str = DEFAULT_FORGEJO_REPO, token: str | None = None, search_pages: int = 5, # kept for compat; unused after fallback removal ) -> ApprovalCheck: """Verify SHA matches a merged Forgejo PR. Requires explicit --approved-pr.""" if not _valid_sha(approved_sha): return ApprovalCheck(False, "approved SHA must be a 40-character hex commit") if approved_pr is None: # SECURITY: search fallback removed (ralph #162 BLOCKER 1). # Arbitrary historical merge SHAs must NOT satisfy approval. return ApprovalCheck( False, "--approved-pr is required (search fallback removed; bind apply to explicit PR)", ) api_token = _forgejo_token(token) if not api_token: return ApprovalCheck(False, "PLATFORMCTL_FORGEJO_TOKEN is required for apply approval verification") try: repo_path = _repo_api_path(forgejo_repo) return _check_specific_pr( approved_sha, approved_pr, repo_path=repo_path, host=forgejo_host, token=api_token, ) except (RuntimeError, ValueError) as exc: return ApprovalCheck(False, _safe_error_summary(exc)) ``` Delete entire `for page in range(...)` block + final fallback return. **VERIFY (add to `tests/test_apply_phase3.py`):** ```python def test_verify_approved_sha_requires_approved_pr(): """approved_pr=None must reject — search fallback removed (ralph BLOCKER 1).""" result = verify_approved_sha("a" * 40, approved_pr=None) assert not result.approved assert "--approved-pr is required" in result.detail def test_verify_approved_sha_with_pr_still_works(mocker): """Explicit --approved-pr path unchanged.""" mocker.patch("...module._check_specific_pr", return_value=ApprovalCheck(True, "ok", pr_number=42)) mocker.patch.dict(os.environ, {"PLATFORMCTL_FORGEJO_TOKEN": "fake"}) result = verify_approved_sha("a" * 40, approved_pr=42) assert result.approved is True ``` --- ## BLOCKER 2 — Decouple `_pull_files` from approval decision **WHAT:** `_check_specific_pr` (linia 147) calls `_pull_files` jako część successful approval return. Jeśli `/files` endpoint fails (500, timeout, malformed JSON), całe approval is rejected. **WHY:** Forgejo API flakiness convertuje valid SHA↔PR approval w hard rejection → pressure na operator do użycia `--break-glass` dla otherwise legitimate changes. Files lookup jest enrichment, nie gating decision. **HOW (drafted patch w `apply.py` linie 115-148):** ```python def _check_specific_pr( approved_sha: str, approved_pr: int, *, repo_path: str, host: str, token: str, ) -> ApprovalCheck: pr = _forgejo_get_json( f"/api/v1/repos/{repo_path}/pulls/{approved_pr}", host=host, token=token, ) if not isinstance(pr, dict): return ApprovalCheck(False, f"PR #{approved_pr} lookup returned non-object payload") if not _is_pr_merged(pr): return ApprovalCheck(False, f"PR #{approved_pr} is not merged", pr_number=approved_pr) sha_values = _pr_sha_values(pr) if approved_sha not in sha_values: return ApprovalCheck( False, f"approved SHA does not match PR #{approved_pr} head/merge SHA", pr_number=approved_pr, head_sha=(pr.get("head") or {}).get("sha") if isinstance(pr.get("head"), dict) else None, merge_sha=pr.get("merge_commit_sha") or pr.get("merged_commit_sha"), ) # Approval is decided. Files lookup is enrichment; never gate on /files flakiness. # (ralph #162 BLOCKER 2) changed_files: tuple[str, ...] = () try: changed_files = _pull_files(approved_pr, repo_path=repo_path, host=host, token=token) except (RuntimeError, ValueError): pass # /files unavailable — emit approval anyway with empty file list return ApprovalCheck( True, f"approved SHA matches merged PR #{approved_pr}", pr_number=approved_pr, head_sha=(pr.get("head") or {}).get("sha") if isinstance(pr.get("head"), dict) else None, merge_sha=pr.get("merge_commit_sha") or pr.get("merged_commit_sha"), changed_files=changed_files, ) ``` **VERIFY:** ```python def test_check_specific_pr_succeeds_when_files_endpoint_fails(mocker): """Approval must succeed even if /files returns 500 (ralph BLOCKER 2).""" mocker.patch("...module._forgejo_get_json", return_value={ "merged": True, "head": {"sha": "a" * 40}, }) mocker.patch("...module._pull_files", side_effect=RuntimeError("500 Internal")) result = _check_specific_pr("a" * 40, 42, repo_path="x/y", host="h", token="t") assert result.approved is True assert result.changed_files == () ``` --- ## BLOCKER 3 — Sanitize Forgejo error bodies (defense-in-depth) **WHAT:** `_forgejo_get_json` (linia 79) embeds `body[:200]` z Forgejo error response w `RuntimeError`. Body flows do `ApprovalCheck.detail` → result dict → audit logs. **WHY:** Forgejo error responses mogą zawierać: internal paths, user IDs, w trybie debug nawet tokens, stack traces. Audit logs będą emit'owane do CI logs (potentially public). Defense-in-depth. **HOW (drafted patch w `apply.py` linie 64-81):** ```python import logging _LOG = logging.getLogger(__name__) def _safe_error_summary(exc: BaseException) -> str: """Strip body details from Forgejo exception flowing into ApprovalCheck.detail.""" msg = str(exc) # Only let known-safe prefix patterns through; never raw response bodies. safe_prefixes = ("Forgejo API returned HTTP ", "Forgejo API request failed", "PR #", "approved SHA") if any(msg.startswith(p) for p in safe_prefixes): return msg return f"{type(exc).__name__}" def _forgejo_get_json(path: str, *, host: str, token: str) -> Any: url = host.rstrip("/") + path request = urllib.request.Request( url, headers={ "Accept": "application/json", "Authorization": f"token {token}", "User-Agent": "platformctl-apply", }, ) try: with urllib.request.urlopen(request, timeout=20) as response: return json.loads(response.read().decode("utf-8")) except urllib.error.HTTPError as exc: # SECURITY (ralph #162 BLOCKER 3): do NOT embed response body — # may leak internal paths, user IDs, debug tokens. Log to debug-only. try: raw_body = exc.read().decode("utf-8", errors="replace")[:1024] _LOG.debug("Forgejo %s → HTTP %s body=%r", path, exc.code, raw_body) except Exception: # noqa: BLE001 pass raise RuntimeError(f"Forgejo API returned HTTP {exc.code} for {path} (response omitted)") from exc except urllib.error.URLError as exc: raise RuntimeError(f"Forgejo API request failed for {path}: {type(exc.reason).__name__}") from exc ``` Plus apply `_safe_error_summary` w `verify_approved_sha` final except clause (zamiast `str(exc)` na linii 209). **VERIFY:** ```python def test_forgejo_get_json_omits_response_body(monkeypatch): """RuntimeError must NOT include response body content (ralph BLOCKER 3).""" class FakeHTTPError(urllib.error.HTTPError): def __init__(self): super().__init__("u", 500, "Internal", {}, None) def read(self): return b'{"secret_token": "supersensitive", "internal_path": "/var/forgejo/secrets/x"}' def fake_urlopen(*a, **kw): raise FakeHTTPError() monkeypatch.setattr(urllib.request, "urlopen", fake_urlopen) with pytest.raises(RuntimeError) as ei: _forgejo_get_json("/api/test", host="h", token="t") assert "supersensitive" not in str(ei.value) assert "internal_path" not in str(ei.value) assert "response omitted" in str(ei.value) ``` --- ## Follow-up issues (file as separate) ### `BREAK-GLASS-HARDEN-01` — Require secondary approval for --break-glass > `apply_plan` (line ~267) accepts `--break-glass --reason "<any-string>"` jako total bypass całej Forgejo gate. Audit trail = log line + free-text reason. Insider z CLI access bypasses everything. > > Proposed: secondary signed approval. Options: (a) GPG-signed break-glass token, (b) second operator Forgejo comment z magic string na tracking issue, (c) PagerDuty incident ack ID. Track failed break-glass attempts w audit. > > Class: `security-sensitive`, `phase/03`, `risk/process` ### `PR-BIND-01` — Verify approved PR base.ref matches deployment branch > `_check_specific_pr` validates merged + SHA, ale nigdy nie sprawdza `pr.base.ref`. PR merged do `release/staging` lub feature branch satisfy `main`-line apply approval. > > Proposed: pass `expected_base_ref` param do `verify_approved_sha` (default `main`), reject jeśli `pr.base.ref != expected_base_ref`. > > Class: `security-sensitive`, `phase/03` ### `ACTOR-ID-01` — Bind apply actor identity to PR merger/reviewers > Merge blockers state że "wrong actor identity must be rejected", ale no actor check istnieje. Operator running `apply` jest unvalidated against PR's merger / reviewers / CODEOWNERS. > > Proposed: capture `merged_by.login` z PR object, accept optional `--actor-identity <login>` (or env), reject mismatch chyba że `--allow-foreign-actor`. Może mieć overlap z #163 plan provenance. > > Class: `security-sensitive`, `phase/03` ### `ERROR-CODE-01` — Machine-readable approval failure exit codes > `ApprovalCheck(False, str)` jest human-friendly ale CI/auditors nie mogą distinguish "Forgejo unreachable" od "SHA mismatch" od "missing token". Wszystko `EXIT_APPROVAL_MISMATCH`. > > Proposed: add `ApprovalCheck.reason: enum` (`NETWORK`, `SHA_MISMATCH`, `NOT_MERGED`, `MISSING_TOKEN`, `PR_NOT_FOUND`). Map do distinct exit codes (`EXIT_APPROVAL_NETWORK=12`, etc.). > > Class: `observability`, `phase/03` --- ## Action items 1. Apply 3 BLOCKER patches → `apply.py` + tests 2. Force-push do `codex/issues/142-apply-approval-binding` 3. File 4 follow-up issues (titles above) 4. Comment "ready for re-review" tutaj 5. Operator może odpalić re-ralph dla potwierdzenia że score ≥7/9 — ralph batch 2026-05-10, claude-opus-4.7 (Pan Herbata) dispatching via codex identity

codex commented 2026-05-11 00:54:00 +02:00

Author

Collaborator

Copy link

Ralph review (5-iter chmurowy) — ITERATE_BLOCKER 3/9

Niezależny 5-iter ralph chain (kimi-k2.6 planner → glm-5.1 critic → deepseek-v4-pro red-team → kimi-k2.6 synth → deepseek-v4-pro arbiter) na tym PR. Verdict + drafted patches poniżej.

Per-dim scoring

• Correctness: 4/9
• Security: 2/9 ⚠️ (arbitrary-SHA bypass + error body leak)
• Observability: 5/9
• Test coverage: 7/9
• Scope discipline: 8/9

Evidence: ~/Iskra-i-Piotr/05 System/Swarmheart Backups/ralph-phase3-apply/162/ (full chain).

---

BLOCKER 1 — Remove paginated search fallback

WHAT: verify_approved_sha (lines 183-207) ma fallback który gdy approved_pr is None searches ostatnich 250 closed PRs. Akceptuje arbitrary historical merge-commit SHA jako approval.

WHY: Red-team confirmed-exploitable:

# Insider z platform access:
SHA=$(git log --merges --format=%H origin/main | tail -1)   # public!
platformctl apply malicious-plan.json --approved $SHA       # ✓ approved

Public historical merge SHA = golden ticket dla arbitrary production change. Bezpośrednio łamie deklarowany kontrakt PR-a: "approval must be bound to a specific Forgejo PR identity".

HOW (drafted patch w apply.py linie 151-209):

def verify_approved_sha(
    approved_sha: str,
    *,
    approved_pr: int | None = None,
    forgejo_host: str = DEFAULT_FORGEJO_HOST,
    forgejo_repo: str = DEFAULT_FORGEJO_REPO,
    token: str | None = None,
    search_pages: int = 5,  # kept for compat; unused after fallback removal
) -> ApprovalCheck:
    """Verify SHA matches a merged Forgejo PR. Requires explicit --approved-pr."""
    if not _valid_sha(approved_sha):
        return ApprovalCheck(False, "approved SHA must be a 40-character hex commit")

    if approved_pr is None:
        # SECURITY: search fallback removed (ralph #162 BLOCKER 1).
        # Arbitrary historical merge SHAs must NOT satisfy approval.
        return ApprovalCheck(
            False,
            "--approved-pr is required (search fallback removed; bind apply to explicit PR)",
        )

    api_token = _forgejo_token(token)
    if not api_token:
        return ApprovalCheck(False, "PLATFORMCTL_FORGEJO_TOKEN is required for apply approval verification")

    try:
        repo_path = _repo_api_path(forgejo_repo)
        return _check_specific_pr(
            approved_sha, approved_pr,
            repo_path=repo_path, host=forgejo_host, token=api_token,
        )
    except (RuntimeError, ValueError) as exc:
        return ApprovalCheck(False, _safe_error_summary(exc))

Delete entire for page in range(...) block + final fallback return.

VERIFY (add to tests/test_apply_phase3.py):

def test_verify_approved_sha_requires_approved_pr():
    """approved_pr=None must reject — search fallback removed (ralph BLOCKER 1)."""
    result = verify_approved_sha("a" * 40, approved_pr=None)
    assert not result.approved
    assert "--approved-pr is required" in result.detail


def test_verify_approved_sha_with_pr_still_works(mocker):
    """Explicit --approved-pr path unchanged."""
    mocker.patch("...module._check_specific_pr", return_value=ApprovalCheck(True, "ok", pr_number=42))
    mocker.patch.dict(os.environ, {"PLATFORMCTL_FORGEJO_TOKEN": "fake"})
    result = verify_approved_sha("a" * 40, approved_pr=42)
    assert result.approved is True

---

BLOCKER 2 — Decouple _pull_files from approval decision

WHAT: _check_specific_pr (linia 147) calls _pull_files jako część successful approval return. Jeśli /files endpoint fails (500, timeout, malformed JSON), całe approval is rejected.

WHY: Forgejo API flakiness convertuje valid SHA↔PR approval w hard rejection → pressure na operator do użycia --break-glass dla otherwise legitimate changes. Files lookup jest enrichment, nie gating decision.

HOW (drafted patch w apply.py linie 115-148):

def _check_specific_pr(
    approved_sha: str,
    approved_pr: int,
    *,
    repo_path: str,
    host: str,
    token: str,
) -> ApprovalCheck:
    pr = _forgejo_get_json(
        f"/api/v1/repos/{repo_path}/pulls/{approved_pr}",
        host=host, token=token,
    )
    if not isinstance(pr, dict):
        return ApprovalCheck(False, f"PR #{approved_pr} lookup returned non-object payload")
    if not _is_pr_merged(pr):
        return ApprovalCheck(False, f"PR #{approved_pr} is not merged", pr_number=approved_pr)

    sha_values = _pr_sha_values(pr)
    if approved_sha not in sha_values:
        return ApprovalCheck(
            False,
            f"approved SHA does not match PR #{approved_pr} head/merge SHA",
            pr_number=approved_pr,
            head_sha=(pr.get("head") or {}).get("sha") if isinstance(pr.get("head"), dict) else None,
            merge_sha=pr.get("merge_commit_sha") or pr.get("merged_commit_sha"),
        )

    # Approval is decided. Files lookup is enrichment; never gate on /files flakiness.
    # (ralph #162 BLOCKER 2)
    changed_files: tuple[str, ...] = ()
    try:
        changed_files = _pull_files(approved_pr, repo_path=repo_path, host=host, token=token)
    except (RuntimeError, ValueError):
        pass  # /files unavailable — emit approval anyway with empty file list

    return ApprovalCheck(
        True,
        f"approved SHA matches merged PR #{approved_pr}",
        pr_number=approved_pr,
        head_sha=(pr.get("head") or {}).get("sha") if isinstance(pr.get("head"), dict) else None,
        merge_sha=pr.get("merge_commit_sha") or pr.get("merged_commit_sha"),
        changed_files=changed_files,
    )

VERIFY:

def test_check_specific_pr_succeeds_when_files_endpoint_fails(mocker):
    """Approval must succeed even if /files returns 500 (ralph BLOCKER 2)."""
    mocker.patch("...module._forgejo_get_json", return_value={
        "merged": True,
        "head": {"sha": "a" * 40},
    })
    mocker.patch("...module._pull_files", side_effect=RuntimeError("500 Internal"))
    result = _check_specific_pr("a" * 40, 42, repo_path="x/y", host="h", token="t")
    assert result.approved is True
    assert result.changed_files == ()

---

BLOCKER 3 — Sanitize Forgejo error bodies (defense-in-depth)

WHAT: _forgejo_get_json (linia 79) embeds body[:200] z Forgejo error response w RuntimeError. Body flows do ApprovalCheck.detail → result dict → audit logs.

WHY: Forgejo error responses mogą zawierać: internal paths, user IDs, w trybie debug nawet tokens, stack traces. Audit logs będą emit'owane do CI logs (potentially public). Defense-in-depth.

HOW (drafted patch w apply.py linie 64-81):

import logging
_LOG = logging.getLogger(__name__)


def _safe_error_summary(exc: BaseException) -> str:
    """Strip body details from Forgejo exception flowing into ApprovalCheck.detail."""
    msg = str(exc)
    # Only let known-safe prefix patterns through; never raw response bodies.
    safe_prefixes = ("Forgejo API returned HTTP ", "Forgejo API request failed", "PR #", "approved SHA")
    if any(msg.startswith(p) for p in safe_prefixes):
        return msg
    return f"{type(exc).__name__}"


def _forgejo_get_json(path: str, *, host: str, token: str) -> Any:
    url = host.rstrip("/") + path
    request = urllib.request.Request(
        url,
        headers={
            "Accept": "application/json",
            "Authorization": f"token {token}",
            "User-Agent": "platformctl-apply",
        },
    )
    try:
        with urllib.request.urlopen(request, timeout=20) as response:
            return json.loads(response.read().decode("utf-8"))
    except urllib.error.HTTPError as exc:
        # SECURITY (ralph #162 BLOCKER 3): do NOT embed response body —
        # may leak internal paths, user IDs, debug tokens. Log to debug-only.
        try:
            raw_body = exc.read().decode("utf-8", errors="replace")[:1024]
            _LOG.debug("Forgejo %s → HTTP %s body=%r", path, exc.code, raw_body)
        except Exception:  # noqa: BLE001
            pass
        raise RuntimeError(f"Forgejo API returned HTTP {exc.code} for {path} (response omitted)") from exc
    except urllib.error.URLError as exc:
        raise RuntimeError(f"Forgejo API request failed for {path}: {type(exc.reason).__name__}") from exc

Plus apply _safe_error_summary w verify_approved_sha final except clause (zamiast str(exc) na linii 209).

VERIFY:

def test_forgejo_get_json_omits_response_body(monkeypatch):
    """RuntimeError must NOT include response body content (ralph BLOCKER 3)."""
    class FakeHTTPError(urllib.error.HTTPError):
        def __init__(self):
            super().__init__("u", 500, "Internal", {}, None)
        def read(self):
            return b'{"secret_token": "supersensitive", "internal_path": "/var/forgejo/secrets/x"}'

    def fake_urlopen(*a, **kw):
        raise FakeHTTPError()
    monkeypatch.setattr(urllib.request, "urlopen", fake_urlopen)

    with pytest.raises(RuntimeError) as ei:
        _forgejo_get_json("/api/test", host="h", token="t")
    assert "supersensitive" not in str(ei.value)
    assert "internal_path" not in str(ei.value)
    assert "response omitted" in str(ei.value)

---

Follow-up issues (file as separate)

BREAK-GLASS-HARDEN-01 — Require secondary approval for --break-glass

apply_plan (line ~267) accepts --break-glass --reason "<any-string>" jako total bypass całej Forgejo gate. Audit trail = log line + free-text reason. Insider z CLI access bypasses everything.

Proposed: secondary signed approval. Options: (a) GPG-signed break-glass token, (b) second operator Forgejo comment z magic string na tracking issue, (c) PagerDuty incident ack ID. Track failed break-glass attempts w audit.

Class: security-sensitive, phase/03, risk/process

PR-BIND-01 — Verify approved PR base.ref matches deployment branch

_check_specific_pr validates merged + SHA, ale nigdy nie sprawdza pr.base.ref. PR merged do release/staging lub feature branch satisfy main-line apply approval.

Proposed: pass expected_base_ref param do verify_approved_sha (default main), reject jeśli pr.base.ref != expected_base_ref.

Class: security-sensitive, phase/03

ACTOR-ID-01 — Bind apply actor identity to PR merger/reviewers

Merge blockers state że "wrong actor identity must be rejected", ale no actor check istnieje. Operator running apply jest unvalidated against PR's merger / reviewers / CODEOWNERS.

Proposed: capture merged_by.login z PR object, accept optional --actor-identity <login> (or env), reject mismatch chyba że --allow-foreign-actor. Może mieć overlap z #163 plan provenance.

Class: security-sensitive, phase/03

ERROR-CODE-01 — Machine-readable approval failure exit codes

ApprovalCheck(False, str) jest human-friendly ale CI/auditors nie mogą distinguish "Forgejo unreachable" od "SHA mismatch" od "missing token". Wszystko EXIT_APPROVAL_MISMATCH.

Proposed: add ApprovalCheck.reason: enum (NETWORK, SHA_MISMATCH, NOT_MERGED, MISSING_TOKEN, PR_NOT_FOUND). Map do distinct exit codes (EXIT_APPROVAL_NETWORK=12, etc.).

Class: observability, phase/03

---

Action items

1. Apply 3 BLOCKER patches → apply.py + tests
2. Force-push do codex/issues/142-apply-approval-binding
3. File 4 follow-up issues (titles above)
4. Comment "ready for re-review" tutaj
5. Operator może odpalić re-ralph dla potwierdzenia że score ≥7/9

— ralph batch 2026-05-10, claude-opus-4.7 (Pan Herbata) dispatching via codex identity

## Ralph review (5-iter chmurowy) — ITERATE_BLOCKER 3/9 Niezależny 5-iter ralph chain (kimi-k2.6 planner → glm-5.1 critic → deepseek-v4-pro red-team → kimi-k2.6 synth → deepseek-v4-pro arbiter) na tym PR. Verdict + drafted patches poniżej. ### Per-dim scoring - Correctness: 4/9 - **Security: 2/9** ⚠️ (arbitrary-SHA bypass + error body leak) - Observability: 5/9 - Test coverage: 7/9 - Scope discipline: 8/9 Evidence: `~/Iskra-i-Piotr/05 System/Swarmheart Backups/ralph-phase3-apply/162/` (full chain). --- ## BLOCKER 1 — Remove paginated search fallback **WHAT:** `verify_approved_sha` (lines 183-207) ma fallback który gdy `approved_pr is None` searches ostatnich 250 closed PRs. Akceptuje arbitrary historical merge-commit SHA jako approval. **WHY:** Red-team confirmed-exploitable: ```bash # Insider z platform access: SHA=$(git log --merges --format=%H origin/main | tail -1) # public! platformctl apply malicious-plan.json --approved $SHA # ✓ approved ``` Public historical merge SHA = golden ticket dla arbitrary production change. Bezpośrednio łamie deklarowany kontrakt PR-a: *"approval must be bound to a specific Forgejo PR identity"*. **HOW (drafted patch w `apply.py` linie 151-209):** ```python def verify_approved_sha( approved_sha: str, *, approved_pr: int | None = None, forgejo_host: str = DEFAULT_FORGEJO_HOST, forgejo_repo: str = DEFAULT_FORGEJO_REPO, token: str | None = None, search_pages: int = 5, # kept for compat; unused after fallback removal ) -> ApprovalCheck: """Verify SHA matches a merged Forgejo PR. Requires explicit --approved-pr.""" if not _valid_sha(approved_sha): return ApprovalCheck(False, "approved SHA must be a 40-character hex commit") if approved_pr is None: # SECURITY: search fallback removed (ralph #162 BLOCKER 1). # Arbitrary historical merge SHAs must NOT satisfy approval. return ApprovalCheck( False, "--approved-pr is required (search fallback removed; bind apply to explicit PR)", ) api_token = _forgejo_token(token) if not api_token: return ApprovalCheck(False, "PLATFORMCTL_FORGEJO_TOKEN is required for apply approval verification") try: repo_path = _repo_api_path(forgejo_repo) return _check_specific_pr( approved_sha, approved_pr, repo_path=repo_path, host=forgejo_host, token=api_token, ) except (RuntimeError, ValueError) as exc: return ApprovalCheck(False, _safe_error_summary(exc)) ``` Delete entire `for page in range(...)` block + final fallback return. **VERIFY (add to `tests/test_apply_phase3.py`):** ```python def test_verify_approved_sha_requires_approved_pr(): """approved_pr=None must reject — search fallback removed (ralph BLOCKER 1).""" result = verify_approved_sha("a" * 40, approved_pr=None) assert not result.approved assert "--approved-pr is required" in result.detail def test_verify_approved_sha_with_pr_still_works(mocker): """Explicit --approved-pr path unchanged.""" mocker.patch("...module._check_specific_pr", return_value=ApprovalCheck(True, "ok", pr_number=42)) mocker.patch.dict(os.environ, {"PLATFORMCTL_FORGEJO_TOKEN": "fake"}) result = verify_approved_sha("a" * 40, approved_pr=42) assert result.approved is True ``` --- ## BLOCKER 2 — Decouple `_pull_files` from approval decision **WHAT:** `_check_specific_pr` (linia 147) calls `_pull_files` jako część successful approval return. Jeśli `/files` endpoint fails (500, timeout, malformed JSON), całe approval is rejected. **WHY:** Forgejo API flakiness convertuje valid SHA↔PR approval w hard rejection → pressure na operator do użycia `--break-glass` dla otherwise legitimate changes. Files lookup jest enrichment, nie gating decision. **HOW (drafted patch w `apply.py` linie 115-148):** ```python def _check_specific_pr( approved_sha: str, approved_pr: int, *, repo_path: str, host: str, token: str, ) -> ApprovalCheck: pr = _forgejo_get_json( f"/api/v1/repos/{repo_path}/pulls/{approved_pr}", host=host, token=token, ) if not isinstance(pr, dict): return ApprovalCheck(False, f"PR #{approved_pr} lookup returned non-object payload") if not _is_pr_merged(pr): return ApprovalCheck(False, f"PR #{approved_pr} is not merged", pr_number=approved_pr) sha_values = _pr_sha_values(pr) if approved_sha not in sha_values: return ApprovalCheck( False, f"approved SHA does not match PR #{approved_pr} head/merge SHA", pr_number=approved_pr, head_sha=(pr.get("head") or {}).get("sha") if isinstance(pr.get("head"), dict) else None, merge_sha=pr.get("merge_commit_sha") or pr.get("merged_commit_sha"), ) # Approval is decided. Files lookup is enrichment; never gate on /files flakiness. # (ralph #162 BLOCKER 2) changed_files: tuple[str, ...] = () try: changed_files = _pull_files(approved_pr, repo_path=repo_path, host=host, token=token) except (RuntimeError, ValueError): pass # /files unavailable — emit approval anyway with empty file list return ApprovalCheck( True, f"approved SHA matches merged PR #{approved_pr}", pr_number=approved_pr, head_sha=(pr.get("head") or {}).get("sha") if isinstance(pr.get("head"), dict) else None, merge_sha=pr.get("merge_commit_sha") or pr.get("merged_commit_sha"), changed_files=changed_files, ) ``` **VERIFY:** ```python def test_check_specific_pr_succeeds_when_files_endpoint_fails(mocker): """Approval must succeed even if /files returns 500 (ralph BLOCKER 2).""" mocker.patch("...module._forgejo_get_json", return_value={ "merged": True, "head": {"sha": "a" * 40}, }) mocker.patch("...module._pull_files", side_effect=RuntimeError("500 Internal")) result = _check_specific_pr("a" * 40, 42, repo_path="x/y", host="h", token="t") assert result.approved is True assert result.changed_files == () ``` --- ## BLOCKER 3 — Sanitize Forgejo error bodies (defense-in-depth) **WHAT:** `_forgejo_get_json` (linia 79) embeds `body[:200]` z Forgejo error response w `RuntimeError`. Body flows do `ApprovalCheck.detail` → result dict → audit logs. **WHY:** Forgejo error responses mogą zawierać: internal paths, user IDs, w trybie debug nawet tokens, stack traces. Audit logs będą emit'owane do CI logs (potentially public). Defense-in-depth. **HOW (drafted patch w `apply.py` linie 64-81):** ```python import logging _LOG = logging.getLogger(__name__) def _safe_error_summary(exc: BaseException) -> str: """Strip body details from Forgejo exception flowing into ApprovalCheck.detail.""" msg = str(exc) # Only let known-safe prefix patterns through; never raw response bodies. safe_prefixes = ("Forgejo API returned HTTP ", "Forgejo API request failed", "PR #", "approved SHA") if any(msg.startswith(p) for p in safe_prefixes): return msg return f"{type(exc).__name__}" def _forgejo_get_json(path: str, *, host: str, token: str) -> Any: url = host.rstrip("/") + path request = urllib.request.Request( url, headers={ "Accept": "application/json", "Authorization": f"token {token}", "User-Agent": "platformctl-apply", }, ) try: with urllib.request.urlopen(request, timeout=20) as response: return json.loads(response.read().decode("utf-8")) except urllib.error.HTTPError as exc: # SECURITY (ralph #162 BLOCKER 3): do NOT embed response body — # may leak internal paths, user IDs, debug tokens. Log to debug-only. try: raw_body = exc.read().decode("utf-8", errors="replace")[:1024] _LOG.debug("Forgejo %s → HTTP %s body=%r", path, exc.code, raw_body) except Exception: # noqa: BLE001 pass raise RuntimeError(f"Forgejo API returned HTTP {exc.code} for {path} (response omitted)") from exc except urllib.error.URLError as exc: raise RuntimeError(f"Forgejo API request failed for {path}: {type(exc.reason).__name__}") from exc ``` Plus apply `_safe_error_summary` w `verify_approved_sha` final except clause (zamiast `str(exc)` na linii 209). **VERIFY:** ```python def test_forgejo_get_json_omits_response_body(monkeypatch): """RuntimeError must NOT include response body content (ralph BLOCKER 3).""" class FakeHTTPError(urllib.error.HTTPError): def __init__(self): super().__init__("u", 500, "Internal", {}, None) def read(self): return b'{"secret_token": "supersensitive", "internal_path": "/var/forgejo/secrets/x"}' def fake_urlopen(*a, **kw): raise FakeHTTPError() monkeypatch.setattr(urllib.request, "urlopen", fake_urlopen) with pytest.raises(RuntimeError) as ei: _forgejo_get_json("/api/test", host="h", token="t") assert "supersensitive" not in str(ei.value) assert "internal_path" not in str(ei.value) assert "response omitted" in str(ei.value) ``` --- ## Follow-up issues (file as separate) ### `BREAK-GLASS-HARDEN-01` — Require secondary approval for --break-glass > `apply_plan` (line ~267) accepts `--break-glass --reason "<any-string>"` jako total bypass całej Forgejo gate. Audit trail = log line + free-text reason. Insider z CLI access bypasses everything. > > Proposed: secondary signed approval. Options: (a) GPG-signed break-glass token, (b) second operator Forgejo comment z magic string na tracking issue, (c) PagerDuty incident ack ID. Track failed break-glass attempts w audit. > > Class: `security-sensitive`, `phase/03`, `risk/process` ### `PR-BIND-01` — Verify approved PR base.ref matches deployment branch > `_check_specific_pr` validates merged + SHA, ale nigdy nie sprawdza `pr.base.ref`. PR merged do `release/staging` lub feature branch satisfy `main`-line apply approval. > > Proposed: pass `expected_base_ref` param do `verify_approved_sha` (default `main`), reject jeśli `pr.base.ref != expected_base_ref`. > > Class: `security-sensitive`, `phase/03` ### `ACTOR-ID-01` — Bind apply actor identity to PR merger/reviewers > Merge blockers state że "wrong actor identity must be rejected", ale no actor check istnieje. Operator running `apply` jest unvalidated against PR's merger / reviewers / CODEOWNERS. > > Proposed: capture `merged_by.login` z PR object, accept optional `--actor-identity <login>` (or env), reject mismatch chyba że `--allow-foreign-actor`. Może mieć overlap z #163 plan provenance. > > Class: `security-sensitive`, `phase/03` ### `ERROR-CODE-01` — Machine-readable approval failure exit codes > `ApprovalCheck(False, str)` jest human-friendly ale CI/auditors nie mogą distinguish "Forgejo unreachable" od "SHA mismatch" od "missing token". Wszystko `EXIT_APPROVAL_MISMATCH`. > > Proposed: add `ApprovalCheck.reason: enum` (`NETWORK`, `SHA_MISMATCH`, `NOT_MERGED`, `MISSING_TOKEN`, `PR_NOT_FOUND`). Map do distinct exit codes (`EXIT_APPROVAL_NETWORK=12`, etc.). > > Class: `observability`, `phase/03` --- ## Action items 1. Apply 3 BLOCKER patches → `apply.py` + tests 2. Force-push do `codex/issues/142-apply-approval-binding` 3. File 4 follow-up issues (titles above) 4. Comment "ready for re-review" tutaj 5. Operator może odpalić re-ralph dla potwierdzenia że score ≥7/9 — ralph batch 2026-05-10, claude-opus-4.7 (Pan Herbata) dispatching via codex identity

codex commented 2026-05-11 00:54:09 +02:00

Author

Collaborator

Copy link

Ralph review (5-iter chmurowy) — ITERATE_BLOCKER 3/9Niezależny 5-iter ralph chain (kimi-k2.6 planner → glm-5.1 critic → deepseek-v4-pro red-team → kimi-k2.6 synth → deepseek-v4-pro arbiter) na tym PR. Verdict + drafted patches poniżej.### Per-dim scoring- Correctness: 4/9- Security: 2/9 ⚠️ (arbitrary-SHA bypass + error body leak)- Observability: 5/9- Test coverage: 7/9- Scope discipline: 8/9Evidence: ~/Iskra-i-Piotr/05 System/Swarmheart Backups/ralph-phase3-apply/162/ (full chain).---## BLOCKER 1 — Remove paginated search fallbackWHAT: verify_approved_sha (lines 183-207) ma fallback który gdy approved_pr is None searches ostatnich 250 closed PRs. Akceptuje arbitrary historical merge-commit SHA jako approval.WHY: Red-team confirmed-exploitable:bash# Insider z platform access:SHA=$(git log --merges --format=%H origin/main | tail -1) # public!platformctl apply malicious-plan.json --approved $SHA # ✓ approvedPublic historical merge SHA = golden ticket dla arbitrary production change. Bezpośrednio łamie deklarowany kontrakt PR-a: "approval must be bound to a specific Forgejo PR identity".HOW (drafted patch w apply.py linie 151-209):pythondef verify_approved_sha( approved_sha: str, *, approved_pr: int | None = None, forgejo_host: str = DEFAULT_FORGEJO_HOST, forgejo_repo: str = DEFAULT_FORGEJO_REPO, token: str | None = None, search_pages: int = 5, # kept for compat; unused after fallback removal) -> ApprovalCheck: """Verify SHA matches a merged Forgejo PR. Requires explicit --approved-pr.""" if not _valid_sha(approved_sha): return ApprovalCheck(False, "approved SHA must be a 40-character hex commit") if approved_pr is None: # SECURITY: search fallback removed (ralph #162 BLOCKER 1). # Arbitrary historical merge SHAs must NOT satisfy approval. return ApprovalCheck( False, "--approved-pr is required (search fallback removed; bind apply to explicit PR)", ) api_token = _forgejo_token(token) if not api_token: return ApprovalCheck(False, "PLATFORMCTL_FORGEJO_TOKEN is required for apply approval verification") try: repo_path = _repo_api_path(forgejo_repo) return _check_specific_pr( approved_sha, approved_pr, repo_path=repo_path, host=forgejo_host, token=api_token, ) except (RuntimeError, ValueError) as exc: return ApprovalCheck(False, _safe_error_summary(exc))Delete entire for page in range(...) block + final fallback return.VERIFY (add to tests/test_apply_phase3.py):pythondef test_verify_approved_sha_requires_approved_pr(): """approved_pr=None must reject — search fallback removed (ralph BLOCKER 1).""" result = verify_approved_sha("a" * 40, approved_pr=None) assert not result.approved assert "--approved-pr is required" in result.detaildef test_verify_approved_sha_with_pr_still_works(mocker): """Explicit --approved-pr path unchanged.""" mocker.patch("...module._check_specific_pr", return_value=ApprovalCheck(True, "ok", pr_number=42)) mocker.patch.dict(os.environ, {"PLATFORMCTL_FORGEJO_TOKEN": "fake"}) result = verify_approved_sha("a" * 40, approved_pr=42) assert result.approved is True---## BLOCKER 2 — Decouple _pull_files from approval decisionWHAT: _check_specific_pr (linia 147) calls _pull_files jako część successful approval return. Jeśli /files endpoint fails (500, timeout, malformed JSON), całe approval is rejected.WHY: Forgejo API flakiness convertuje valid SHA↔PR approval w hard rejection → pressure na operator do użycia --break-glass dla otherwise legitimate changes. Files lookup jest enrichment, nie gating decision.HOW (drafted patch w apply.py linie 115-148):pythondef _check_specific_pr( approved_sha: str, approved_pr: int, *, repo_path: str, host: str, token: str,) -> ApprovalCheck: pr = _forgejo_get_json( f"/api/v1/repos/{repo_path}/pulls/{approved_pr}", host=host, token=token, ) if not isinstance(pr, dict): return ApprovalCheck(False, f"PR #{approved_pr} lookup returned non-object payload") if not _is_pr_merged(pr): return ApprovalCheck(False, f"PR #{approved_pr} is not merged", pr_number=approved_pr) sha_values = _pr_sha_values(pr) if approved_sha not in sha_values: return ApprovalCheck( False, f"approved SHA does not match PR #{approved_pr} head/merge SHA", pr_number=approved_pr, head_sha=(pr.get("head") or {}).get("sha") if isinstance(pr.get("head"), dict) else None, merge_sha=pr.get("merge_commit_sha") or pr.get("merged_commit_sha"), ) # Approval is decided. Files lookup is enrichment; never gate on /files flakiness. # (ralph #162 BLOCKER 2) changed_files: tuple[str, ...] = () try: changed_files = _pull_files(approved_pr, repo_path=repo_path, host=host, token=token) except (RuntimeError, ValueError): pass # /files unavailable — emit approval anyway with empty file list return ApprovalCheck( True, f"approved SHA matches merged PR #{approved_pr}", pr_number=approved_pr, head_sha=(pr.get("head") or {}).get("sha") if isinstance(pr.get("head"), dict) else None, merge_sha=pr.get("merge_commit_sha") or pr.get("merged_commit_sha"), changed_files=changed_files, )VERIFY:pythondef test_check_specific_pr_succeeds_when_files_endpoint_fails(mocker): """Approval must succeed even if /files returns 500 (ralph BLOCKER 2).""" mocker.patch("...module._forgejo_get_json", return_value={ "merged": True, "head": {"sha": "a" * 40}, }) mocker.patch("...module._pull_files", side_effect=RuntimeError("500 Internal")) result = _check_specific_pr("a" * 40, 42, repo_path="x/y", host="h", token="t") assert result.approved is True assert result.changed_files == ()---## BLOCKER 3 — Sanitize Forgejo error bodies (defense-in-depth)WHAT: _forgejo_get_json (linia 79) embeds body[:200] z Forgejo error response w RuntimeError. Body flows do ApprovalCheck.detail → result dict → audit logs.WHY: Forgejo error responses mogą zawierać: internal paths, user IDs, w trybie debug nawet tokens, stack traces. Audit logs będą emit'owane do CI logs (potentially public). Defense-in-depth.HOW (drafted patch w apply.py linie 64-81):pythonimport logging_LOG = logging.getLogger(__name__)def _safe_error_summary(exc: BaseException) -> str: """Strip body details from Forgejo exception flowing into ApprovalCheck.detail.""" msg = str(exc) # Only let known-safe prefix patterns through; never raw response bodies. safe_prefixes = ("Forgejo API returned HTTP ", "Forgejo API request failed", "PR #", "approved SHA") if any(msg.startswith(p) for p in safe_prefixes): return msg return f"{type(exc).__name__}"def _forgejo_get_json(path: str, *, host: str, token: str) -> Any: url = host.rstrip("/") + path request = urllib.request.Request( url, headers={ "Accept": "application/json", "Authorization": f"token {token}", "User-Agent": "platformctl-apply", }, ) try: with urllib.request.urlopen(request, timeout=20) as response: return json.loads(response.read().decode("utf-8")) except urllib.error.HTTPError as exc: # SECURITY (ralph #162 BLOCKER 3): do NOT embed response body — # may leak internal paths, user IDs, debug tokens. Log to debug-only. try: raw_body = exc.read().decode("utf-8", errors="replace")[:1024] _LOG.debug("Forgejo %s → HTTP %s body=%r", path, exc.code, raw_body) except Exception: # noqa: BLE001 pass raise RuntimeError(f"Forgejo API returned HTTP {exc.code} for {path} (response omitted)") from exc except urllib.error.URLError as exc: raise RuntimeError(f"Forgejo API request failed for {path}: {type(exc.reason).__name__}") from excPlus apply _safe_error_summary w verify_approved_sha final except clause (zamiast str(exc) na linii 209).VERIFY:pythondef test_forgejo_get_json_omits_response_body(monkeypatch): """RuntimeError must NOT include response body content (ralph BLOCKER 3).""" class FakeHTTPError(urllib.error.HTTPError): def __init__(self): super().__init__("u", 500, "Internal", {}, None) def read(self): return b'{"secret_token": "supersensitive", "internal_path": "/var/forgejo/secrets/x"}' def fake_urlopen(*a, **kw): raise FakeHTTPError() monkeypatch.setattr(urllib.request, "urlopen", fake_urlopen) with pytest.raises(RuntimeError) as ei: _forgejo_get_json("/api/test", host="h", token="t") assert "supersensitive" not in str(ei.value) assert "internal_path" not in str(ei.value) assert "response omitted" in str(ei.value)---## Follow-up issues (file as separate)### BREAK-GLASS-HARDEN-01 — Require secondary approval for --break-glass> apply_plan (line ~267) accepts --break-glass --reason "<any-string>" jako total bypass całej Forgejo gate. Audit trail = log line + free-text reason. Insider z CLI access bypasses everything.>> Proposed: secondary signed approval. Options: (a) GPG-signed break-glass token, (b) second operator Forgejo comment z magic string na tracking issue, (c) PagerDuty incident ack ID. Track failed break-glass attempts w audit.>> Class: security-sensitive, phase/03, risk/process### PR-BIND-01 — Verify approved PR base.ref matches deployment branch> _check_specific_pr validates merged + SHA, ale nigdy nie sprawdza pr.base.ref. PR merged do release/staging lub feature branch satisfy main-line apply approval.>> Proposed: pass expected_base_ref param do verify_approved_sha (default main), reject jeśli pr.base.ref != expected_base_ref.>> Class: security-sensitive, phase/03### ACTOR-ID-01 — Bind apply actor identity to PR merger/reviewers> Merge blockers state że "wrong actor identity must be rejected", ale no actor check istnieje. Operator running apply jest unvalidated against PR's merger / reviewers / CODEOWNERS.>> Proposed: capture merged_by.login z PR object, accept optional --actor-identity <login> (or env), reject mismatch chyba że --allow-foreign-actor. Może mieć overlap z #163 plan provenance.>> Class: security-sensitive, phase/03### ERROR-CODE-01 — Machine-readable approval failure exit codes> ApprovalCheck(False, str) jest human-friendly ale CI/auditors nie mogą distinguish "Forgejo unreachable" od "SHA mismatch" od "missing token". Wszystko EXIT_APPROVAL_MISMATCH.>> Proposed: add ApprovalCheck.reason: enum (NETWORK, SHA_MISMATCH, NOT_MERGED, MISSING_TOKEN, PR_NOT_FOUND). Map do distinct exit codes (EXIT_APPROVAL_NETWORK=12, etc.).>> Class: observability, phase/03---## Action items1. Apply 3 BLOCKER patches → apply.py + tests2. Force-push do codex/issues/142-apply-approval-binding3. File 4 follow-up issues (titles above)4. Comment "ready for re-review" tutaj5. Operator może odpalić re-ralph dla potwierdzenia że score ≥7/9— ralph batch 2026-05-10, claude-opus-4.7 (Pan Herbata) dispatching via codex identity

## Ralph review (5-iter chmurowy) — ITERATE_BLOCKER 3/9Niezależny 5-iter ralph chain (kimi-k2.6 planner → glm-5.1 critic → deepseek-v4-pro red-team → kimi-k2.6 synth → deepseek-v4-pro arbiter) na tym PR. Verdict + drafted patches poniżej.### Per-dim scoring- Correctness: 4/9- **Security: 2/9** ⚠️ (arbitrary-SHA bypass + error body leak)- Observability: 5/9- Test coverage: 7/9- Scope discipline: 8/9Evidence: `~/Iskra-i-Piotr/05 System/Swarmheart Backups/ralph-phase3-apply/162/` (full chain).---## BLOCKER 1 — Remove paginated search fallback**WHAT:** `verify_approved_sha` (lines 183-207) ma fallback który gdy `approved_pr is None` searches ostatnich 250 closed PRs. Akceptuje arbitrary historical merge-commit SHA jako approval.**WHY:** Red-team confirmed-exploitable:```bash# Insider z platform access:SHA=$(git log --merges --format=%H origin/main | tail -1) # public!platformctl apply malicious-plan.json --approved $SHA # ✓ approved```Public historical merge SHA = golden ticket dla arbitrary production change. Bezpośrednio łamie deklarowany kontrakt PR-a: *"approval must be bound to a specific Forgejo PR identity"*.**HOW (drafted patch w `apply.py` linie 151-209):**```pythondef verify_approved_sha( approved_sha: str, *, approved_pr: int | None = None, forgejo_host: str = DEFAULT_FORGEJO_HOST, forgejo_repo: str = DEFAULT_FORGEJO_REPO, token: str | None = None, search_pages: int = 5, # kept for compat; unused after fallback removal) -> ApprovalCheck: """Verify SHA matches a merged Forgejo PR. Requires explicit --approved-pr.""" if not _valid_sha(approved_sha): return ApprovalCheck(False, "approved SHA must be a 40-character hex commit") if approved_pr is None: # SECURITY: search fallback removed (ralph #162 BLOCKER 1). # Arbitrary historical merge SHAs must NOT satisfy approval. return ApprovalCheck( False, "--approved-pr is required (search fallback removed; bind apply to explicit PR)", ) api_token = _forgejo_token(token) if not api_token: return ApprovalCheck(False, "PLATFORMCTL_FORGEJO_TOKEN is required for apply approval verification") try: repo_path = _repo_api_path(forgejo_repo) return _check_specific_pr( approved_sha, approved_pr, repo_path=repo_path, host=forgejo_host, token=api_token, ) except (RuntimeError, ValueError) as exc: return ApprovalCheck(False, _safe_error_summary(exc))```Delete entire `for page in range(...)` block + final fallback return.**VERIFY (add to `tests/test_apply_phase3.py`):**```pythondef test_verify_approved_sha_requires_approved_pr(): """approved_pr=None must reject — search fallback removed (ralph BLOCKER 1).""" result = verify_approved_sha("a" * 40, approved_pr=None) assert not result.approved assert "--approved-pr is required" in result.detaildef test_verify_approved_sha_with_pr_still_works(mocker): """Explicit --approved-pr path unchanged.""" mocker.patch("...module._check_specific_pr", return_value=ApprovalCheck(True, "ok", pr_number=42)) mocker.patch.dict(os.environ, {"PLATFORMCTL_FORGEJO_TOKEN": "fake"}) result = verify_approved_sha("a" * 40, approved_pr=42) assert result.approved is True```---## BLOCKER 2 — Decouple `_pull_files` from approval decision**WHAT:** `_check_specific_pr` (linia 147) calls `_pull_files` jako część successful approval return. Jeśli `/files` endpoint fails (500, timeout, malformed JSON), całe approval is rejected.**WHY:** Forgejo API flakiness convertuje valid SHA↔PR approval w hard rejection → pressure na operator do użycia `--break-glass` dla otherwise legitimate changes. Files lookup jest enrichment, nie gating decision.**HOW (drafted patch w `apply.py` linie 115-148):**```pythondef _check_specific_pr( approved_sha: str, approved_pr: int, *, repo_path: str, host: str, token: str,) -> ApprovalCheck: pr = _forgejo_get_json( f"/api/v1/repos/{repo_path}/pulls/{approved_pr}", host=host, token=token, ) if not isinstance(pr, dict): return ApprovalCheck(False, f"PR #{approved_pr} lookup returned non-object payload") if not _is_pr_merged(pr): return ApprovalCheck(False, f"PR #{approved_pr} is not merged", pr_number=approved_pr) sha_values = _pr_sha_values(pr) if approved_sha not in sha_values: return ApprovalCheck( False, f"approved SHA does not match PR #{approved_pr} head/merge SHA", pr_number=approved_pr, head_sha=(pr.get("head") or {}).get("sha") if isinstance(pr.get("head"), dict) else None, merge_sha=pr.get("merge_commit_sha") or pr.get("merged_commit_sha"), ) # Approval is decided. Files lookup is enrichment; never gate on /files flakiness. # (ralph #162 BLOCKER 2) changed_files: tuple[str, ...] = () try: changed_files = _pull_files(approved_pr, repo_path=repo_path, host=host, token=token) except (RuntimeError, ValueError): pass # /files unavailable — emit approval anyway with empty file list return ApprovalCheck( True, f"approved SHA matches merged PR #{approved_pr}", pr_number=approved_pr, head_sha=(pr.get("head") or {}).get("sha") if isinstance(pr.get("head"), dict) else None, merge_sha=pr.get("merge_commit_sha") or pr.get("merged_commit_sha"), changed_files=changed_files, )```**VERIFY:**```pythondef test_check_specific_pr_succeeds_when_files_endpoint_fails(mocker): """Approval must succeed even if /files returns 500 (ralph BLOCKER 2).""" mocker.patch("...module._forgejo_get_json", return_value={ "merged": True, "head": {"sha": "a" * 40}, }) mocker.patch("...module._pull_files", side_effect=RuntimeError("500 Internal")) result = _check_specific_pr("a" * 40, 42, repo_path="x/y", host="h", token="t") assert result.approved is True assert result.changed_files == ()```---## BLOCKER 3 — Sanitize Forgejo error bodies (defense-in-depth)**WHAT:** `_forgejo_get_json` (linia 79) embeds `body[:200]` z Forgejo error response w `RuntimeError`. Body flows do `ApprovalCheck.detail` → result dict → audit logs.**WHY:** Forgejo error responses mogą zawierać: internal paths, user IDs, w trybie debug nawet tokens, stack traces. Audit logs będą emit'owane do CI logs (potentially public). Defense-in-depth.**HOW (drafted patch w `apply.py` linie 64-81):**```pythonimport logging_LOG = logging.getLogger(__name__)def _safe_error_summary(exc: BaseException) -> str: """Strip body details from Forgejo exception flowing into ApprovalCheck.detail.""" msg = str(exc) # Only let known-safe prefix patterns through; never raw response bodies. safe_prefixes = ("Forgejo API returned HTTP ", "Forgejo API request failed", "PR #", "approved SHA") if any(msg.startswith(p) for p in safe_prefixes): return msg return f"{type(exc).__name__}"def _forgejo_get_json(path: str, *, host: str, token: str) -> Any: url = host.rstrip("/") + path request = urllib.request.Request( url, headers={ "Accept": "application/json", "Authorization": f"token {token}", "User-Agent": "platformctl-apply", }, ) try: with urllib.request.urlopen(request, timeout=20) as response: return json.loads(response.read().decode("utf-8")) except urllib.error.HTTPError as exc: # SECURITY (ralph #162 BLOCKER 3): do NOT embed response body — # may leak internal paths, user IDs, debug tokens. Log to debug-only. try: raw_body = exc.read().decode("utf-8", errors="replace")[:1024] _LOG.debug("Forgejo %s → HTTP %s body=%r", path, exc.code, raw_body) except Exception: # noqa: BLE001 pass raise RuntimeError(f"Forgejo API returned HTTP {exc.code} for {path} (response omitted)") from exc except urllib.error.URLError as exc: raise RuntimeError(f"Forgejo API request failed for {path}: {type(exc.reason).__name__}") from exc```Plus apply `_safe_error_summary` w `verify_approved_sha` final except clause (zamiast `str(exc)` na linii 209).**VERIFY:**```pythondef test_forgejo_get_json_omits_response_body(monkeypatch): """RuntimeError must NOT include response body content (ralph BLOCKER 3).""" class FakeHTTPError(urllib.error.HTTPError): def __init__(self): super().__init__("u", 500, "Internal", {}, None) def read(self): return b'{"secret_token": "supersensitive", "internal_path": "/var/forgejo/secrets/x"}' def fake_urlopen(*a, **kw): raise FakeHTTPError() monkeypatch.setattr(urllib.request, "urlopen", fake_urlopen) with pytest.raises(RuntimeError) as ei: _forgejo_get_json("/api/test", host="h", token="t") assert "supersensitive" not in str(ei.value) assert "internal_path" not in str(ei.value) assert "response omitted" in str(ei.value)```---## Follow-up issues (file as separate)### `BREAK-GLASS-HARDEN-01` — Require secondary approval for --break-glass> `apply_plan` (line ~267) accepts `--break-glass --reason "<any-string>"` jako total bypass całej Forgejo gate. Audit trail = log line + free-text reason. Insider z CLI access bypasses everything.>> Proposed: secondary signed approval. Options: (a) GPG-signed break-glass token, (b) second operator Forgejo comment z magic string na tracking issue, (c) PagerDuty incident ack ID. Track failed break-glass attempts w audit.>> Class: `security-sensitive`, `phase/03`, `risk/process`### `PR-BIND-01` — Verify approved PR base.ref matches deployment branch> `_check_specific_pr` validates merged + SHA, ale nigdy nie sprawdza `pr.base.ref`. PR merged do `release/staging` lub feature branch satisfy `main`-line apply approval.>> Proposed: pass `expected_base_ref` param do `verify_approved_sha` (default `main`), reject jeśli `pr.base.ref != expected_base_ref`.>> Class: `security-sensitive`, `phase/03`### `ACTOR-ID-01` — Bind apply actor identity to PR merger/reviewers> Merge blockers state że "wrong actor identity must be rejected", ale no actor check istnieje. Operator running `apply` jest unvalidated against PR's merger / reviewers / CODEOWNERS.>> Proposed: capture `merged_by.login` z PR object, accept optional `--actor-identity <login>` (or env), reject mismatch chyba że `--allow-foreign-actor`. Może mieć overlap z #163 plan provenance.>> Class: `security-sensitive`, `phase/03`### `ERROR-CODE-01` — Machine-readable approval failure exit codes> `ApprovalCheck(False, str)` jest human-friendly ale CI/auditors nie mogą distinguish "Forgejo unreachable" od "SHA mismatch" od "missing token". Wszystko `EXIT_APPROVAL_MISMATCH`.>> Proposed: add `ApprovalCheck.reason: enum` (`NETWORK`, `SHA_MISMATCH`, `NOT_MERGED`, `MISSING_TOKEN`, `PR_NOT_FOUND`). Map do distinct exit codes (`EXIT_APPROVAL_NETWORK=12`, etc.).>> Class: `observability`, `phase/03`---## Action items1. Apply 3 BLOCKER patches → `apply.py` + tests2. Force-push do `codex/issues/142-apply-approval-binding`3. File 4 follow-up issues (titles above)4. Comment "ready for re-review" tutaj5. Operator może odpalić re-ralph dla potwierdzenia że score ≥7/9— ralph batch 2026-05-10, claude-opus-4.7 (Pan Herbata) dispatching via codex identity

codex referenced this pull request 2026-05-11 01:01:06 +02:00

feat(platformctl): add health rollup #161

codex force-pushed codex/issues/142-apply-approval-binding from c39d909968 to 1feb290d12 2026-05-12 00:44:54 +02:00 Compare

codex referenced this pull request 2026-05-12 00:45:20 +02:00

BREAK-GLASS-HARDEN-01: require secondary approval for platformctl break-glass #188

codex referenced this pull request 2026-05-12 00:45:21 +02:00

PR-BIND-01: verify approved apply PR base branch #189

codex referenced this pull request 2026-05-12 00:45:21 +02:00

ACTOR-ID-01: bind apply actor identity to approval context #190

codex referenced this pull request 2026-05-12 00:45:21 +02:00

ERROR-CODE-01: add machine-readable approval failure reasons #191

codex commented 2026-05-12 00:45:34 +02:00

Author

Collaborator

Copy link

Codex iteration — ralph BLOCKERs addressed

Updated branch codex/issues/142-apply-approval-binding at 1feb290.

Applied:

• BLOCKER 1: removed paginated closed-PR search fallback; --approved-pr is now required.
• BLOCKER 2: /pulls/<pr>/files lookup is enrichment only and no longer gates approval.
• BLOCKER 3: Forgejo HTTP error response bodies are omitted from approval details/loggable result paths.

Verification:

• PYTHONPATH=control-plane pytest control-plane/platformctl/tests/test_apply_phase3.py -q → 9 passed.
• ./tests/run-verify.sh currently fails on pre-existing origin/main prompt debt: prompts/codex-rs2000-close-2026-05-12.md exceeds token budget and links missing prompts/codex-rs2000-image-prune-2026-05-12.md. I did not fold that unrelated P2 prompt fix into this security-sensitive #162 scope.

Filed follow-up issues from the ralph note:

• #188 BREAK-GLASS-HARDEN-01
• #189 PR-BIND-01
• #190 ACTOR-ID-01
• #191 ERROR-CODE-01

Ready for re-review / operator review.

## Codex iteration — ralph BLOCKERs addressed Updated branch `codex/issues/142-apply-approval-binding` at `1feb290`. Applied: - BLOCKER 1: removed paginated closed-PR search fallback; `--approved-pr` is now required. - BLOCKER 2: `/pulls/<pr>/files` lookup is enrichment only and no longer gates approval. - BLOCKER 3: Forgejo HTTP error response bodies are omitted from approval details/loggable result paths. Verification: - `PYTHONPATH=control-plane pytest control-plane/platformctl/tests/test_apply_phase3.py -q` → 9 passed. - `./tests/run-verify.sh` currently fails on pre-existing `origin/main` prompt debt: `prompts/codex-rs2000-close-2026-05-12.md` exceeds token budget and links missing `prompts/codex-rs2000-image-prune-2026-05-12.md`. I did not fold that unrelated P2 prompt fix into this security-sensitive #162 scope. Filed follow-up issues from the ralph note: - #188 BREAK-GLASS-HARDEN-01 - #189 PR-BIND-01 - #190 ACTOR-ID-01 - #191 ERROR-CODE-01 Ready for re-review / operator review.

codex referenced this pull request 2026-05-12 00:47:44 +02:00

fix(platformctl): bind apply plans to source sha #163

codex referenced this pull request 2026-05-12 00:49:39 +02:00

fix(platformctl): allow approved no-op apply #164

codex referenced this pull request 2026-05-12 00:53:37 +02:00

feat(platformctl): execute approved compose apply #165

codex referenced this pull request 2026-05-12 01:09:37 +02:00

[coordination-lane] Cutover Flight Phase 2: v2 catalog to gate #142

pdurlej merged commit 8639c217b6 into main 2026-05-12 01:28:22 +02:00

pdurlej referenced this pull request from a commit 2026-05-12 01:28:23 +02:00

Merge pull request 'fix(platformctl): bind apply approval to Forgejo PR' (#162) from codex/issues/142-apply-approval-binding into main

codex referenced this pull request 2026-05-12 01:37:44 +02:00

fix(platformctl): land Phase 3 apply stack on main #215

codex referenced this pull request 2026-05-12 01:38:14 +02:00

[coordination-lane] Cutover Flight Phase 2: v2 catalog to gate #142

claude referenced this pull request 2026-05-12 01:56:39 +02:00

governance: ADR-0017 — no stacked PRs to main + 4-layer guard (operator-protection) #220

codex referenced this pull request from a commit 2026-05-12 01:56:39 +02:00

governance: ADR-0017 — no stacked PRs to main + 4-layer guard (pre/post-merge + cousin + operator)

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels   

W6d-automerge-calibration

  

agent/claude-code

Vistula actor: Claude Code.

  

agent/codex

Vistula actor: Codex.

  

agent/hermes

Vistula actor: Hermes Upstream.

  

agent/iskra

Vistula actor: Iskra.

  

agent/ollama

Vistula actor: Ollama or compatible model.

  

agent/patchwarden

Vistula actor: Patchwarden.

  

automerge-candidate

Candidate for narrow W6d autonomous merge readiness pilot

  

class/security-sensitive

Security-sensitive class of service: smallest coherent PRs and full canary

  

cutover-gate

blocks production declare / final RS2000 replacement

  

dependency/blocked

Vistula dependency state: blocked.

  

dependency/blocks-others

Vistula dependency state: blocks other work.

  

dependency/cross-repo

Vistula dependency scope: cross-repository.

  

dependency/needs-confirmation

Vistula dependency state: needs confirmation.

  

domain:agents

Agent identity, delegation, subagents, routing, or workflows.

  

domain:ci

CI, checks, Actions, runners, or release automation.

  

domain:docs

Documentation, packets, practices, or source-of-truth text.

  

domain:forgejo

Forgejo issues, PRs, labels, repo settings, or identity.

  

domain:infra

Infrastructure, storage, backup, DNS, network, or host substrate.

  

domain:memory

Memory, recall, write gates, salience, or Honcho-adjacent behavior.

  

domain:runtime

Runtime services, deploys, daemons, timers, or live behavior.

  

domain:signal

Signal UX, delivery, context, or outbound behavior.

  

domain:ux

Product UX, operator experience, or conversation ergonomics.

  

flow/architecture

Vistula lifecycle: architecture phase in progress.

  

flow/blocked

Vistula lifecycle: blocked on dependency or decision.

  

flow/deployed

Vistula lifecycle: deployed to runtime where applicable.

  

flow/done

Vistula lifecycle: work merged or otherwise done.

  

flow/implementation

Vistula lifecycle: implementation phase in progress.

  

flow/intake

Vistula lifecycle: incoming signal not refined yet.

  

flow/maintained

Vistula lifecycle: maintained/steady state.

  

flow/observed

Vistula lifecycle: observed after delivery.

  

flow/ready

Vistula lifecycle: ready for architecture or implementation.

  

flow/refining

Vistula lifecycle: Hermes/spec refinement in progress.

  

flow/retired

Vistula lifecycle: retired or intentionally closed.

  

flow/review

Vistula lifecycle: review phase in progress.

  

iterating

Orchestrator/Codex actively amending in response to review — operator can ignore until resolved

  

judge/codex-candidate

Candidate for Codex implementation.

  

judge/hermes-candidate

Candidate for Hermes discovery/research.

  

judge/low-confidence

Judgment confidence is low.

  

judge/needs-refinement

Needs clearer evidence or scope.

  

judge/operator-needed

Needs Piotr decision or input.

  

judge/p0

Urgent, operator attention or unblock now.

  

judge/p1

High value, schedule soon.

  

judge/p2

Useful, normal queue.

  

judge/p3

Low priority, keep but do not chase.

  

judge/park

Parked from current attention.

  

judge/patchwarden-candidate

Needs Patchwarden merge-safety attention.

  

judge/stale-priority

Existing priority judgment appears stale.

  

kind/adr

Vistula work kind: architecture decision record.

  

kind/bug

Vistula work kind: bug fix.

  

kind/chore

Vistula work kind: chore.

  

kind/feature

Vistula work kind: feature.

  

kind/infra

Vistula work kind: infrastructure.

  

kind/ops

Vistula work kind: operations.

  

kind/refactor

Vistula work kind: refactor.

  

kind/research

Vistula work kind: research/discovery.

  

large-impact

Per charter §3 review-by-impact: large impact (≥300 LOC OR new module OR identity/secret change OR architectural pivot) — Oracle pre-review recommended

  

merge/auto

Vistula merge mode: automated merge.

  

merge/manual

Vistula merge mode: manual merge.

  

merge/manual-dependency-conflict

Manual merge reason: dependency conflict.

  

merge/manual-failing-tests

Manual merge reason: failing tests.

  

merge/manual-merge-conflict

Manual merge reason: merge conflict.

  

merge/manual-missing-review

Manual merge reason: missing review.

  

merge/manual-operator-preference

Manual merge reason: operator preference.

  

merge/manual-red-zone

Manual merge reason: red-zone risk.

  

merge/manual-security-sensitive

Manual merge reason: security-sensitive work.

  

merge/manual-unclear-scope

Manual merge reason: unclear scope.

  

merge/manual-unknown

Manual merge reason: unknown.

  

meta

meta-decomposition issue; first agent decomposes into atomic children

  

mode:operator-only

Apply step requires explicit operator approval.

  

mode:patchwarden-iskra-approved

Work eligible for Patchwarden/Iskra approval inside repo policy.

  

mode:safe-auto

Low-risk work eligible for lightweight autonomous handling.

  

needs-operator-decision

PR is blocked on operator yes/no decision — fresh-Claude/Codex won't make this call alone

  

needs-triage

decomposing agent has open questions / unknowns

  

not-ready

reviewed but blocked on a precondition

  

observed/erroring

Vistula observation: erroring.

  

observed/needs-followup

Vistula observation: needs follow-up.

  

observed/pending

Vistula observation: pending.

  

observed/retire-candidate

Vistula observation: retire candidate.

  

observed/unused

Vistula observation: unused.

  

observed/used

Vistula observation: used.

  

operator-emotional

operator-emotional importance (Iskra memory etc.)

  

owner-attention

owner must decide; blocks default path

  

phase/02

Phase 02 cataloging

  

phase/03

Phase 03 control plane (platformctl)

  

priority:p0

Immediate safety, uptime, data, or operator-blocking issue.

  

priority:p1

Important active work; should be pulled soon.

  

priority:p2

Useful normal-priority work.

  

priority:p3

Opportunistic cleanup, research, or backlog work.

  

proposed

auto-generated by meta-decomposition; awaiting human review

  

ready-for-agent

reviewed and pickable; agents may claim per cookbook

  

ready-for-operator

PR is ready for operator review/merge — orchestrator finished its part

  

recovery

disaster recovery / restore semantics relevant

  

review:claude-reviewed

Reviewed by Claude-family agent.

  

review:codex-reviewed

Reviewed by Codex-family agent.

  

review:dziadek-reviewed

Reviewed by Dziadek pinch-point reviewer.

  

review:needs-human

Needs human review before merge or apply.

  

risk/exposure

incorrectly public/private/tailnet/auth/routed

  

risk/process

workflow/review/orchestration may produce bad outcomes

  

risk/product

work may have lost owner/user value

  

risk/runtime

platform may not run, recover, validate, behave reproducibly

  

safety:external-write

May write to external systems, users, rooms, repos, or services.

  

safety:no-prod-mutation

Must not mutate production or live external state.

  

safety:prod-impact

May affect production/runtime behavior.

  

safety:secret-touch

Touches secrets, credentials, auth, or secret-adjacent config.

  

size/large

Vistula size: large.

  

size/medium

Vistula size: medium.

  

size/small

Vistula size: small.

  

size/tiny

Vistula size: tiny.

  

size/unknown

Vistula size: not classified yet.

  

source/adr

Vistula source: ADR.

  

source/agent-generated

Vistula source: agent generated.

  

source/manual

Vistula source: manual entry.

  

source/operator-chat

Vistula source: operator chat.

  

source/voice-note

Vistula source: voice note.

  

status:blocked

Blocked by missing dependency, evidence, access, or operator decision.

  

status:codex-ready

Ready for a Codex implementation pass.

  

status:merged:pending-evidence

Merged but waiting for live or artifact evidence before closure.

  

status:needs-evidence

Needs proof, logs, tests, or operator-visible evidence.

  

status:operator-needed

Requires explicit operator input before safe progress.

  

status:parked

Intentionally deferred; not active campaign work.

  

tier/full

Full review tier per ADR-0007

  

tier/lite

Lite review tier per ADR-0007

  

tier/stacked

Stacked PR (base != main) escape hatch per ADR-0017. Requires consolidator-PR plan in PR body.

  

tier:0-platform-substrate

Data, secrets, backup, storage, or critical platform substrate.

  

tier:1-iskra-value-layer

Iskra/OpenClaw behavior, memory, runtime, or value-layer work.

  

tier:2-tools-products-modules

Tools, products, modules, experiments, and lower-blast-radius work.

  

type:bug

Incorrect behavior or regression.

  

type:chore

Maintenance, cleanup, metadata, or operational task.

  

type:docs

Documentation-only work.

  

type:feat

New capability or product behavior.

  

type:policy

Governance, protocol, boundary, or decision-contract change.

  

type:research

Investigation, spike, or evidence-gathering task.

No labels

W6d-automerge-calibration

agent/claude-code

agent/codex

agent/hermes

agent/iskra

agent/ollama

agent/patchwarden

automerge-candidate

class/security-sensitive

cutover-gate

dependency/blocked

dependency/blocks-others

dependency/cross-repo

dependency/needs-confirmation

domain:agents

domain:ci

domain:docs

domain:forgejo

domain:infra

domain:memory

domain:runtime

domain:signal

domain:ux

flow/architecture

flow/blocked

flow/deployed

flow/done

flow/implementation

flow/intake

flow/maintained

flow/observed

flow/ready

flow/refining

flow/retired

flow/review

iterating

judge/codex-candidate

judge/hermes-candidate

judge/low-confidence

judge/needs-refinement

judge/operator-needed

judge/p0

judge/p1

judge/p2

judge/p3

judge/park

judge/patchwarden-candidate

judge/stale-priority

kind/adr

kind/bug

kind/chore

kind/feature

kind/infra

kind/ops

kind/refactor

kind/research

large-impact

merge/auto

merge/manual

merge/manual-dependency-conflict

merge/manual-failing-tests

merge/manual-merge-conflict

merge/manual-missing-review

merge/manual-operator-preference

merge/manual-red-zone

merge/manual-security-sensitive

merge/manual-unclear-scope

merge/manual-unknown

meta

mode:operator-only

mode:patchwarden-iskra-approved

mode:safe-auto

needs-operator-decision

needs-triage

not-ready

observed/erroring

observed/needs-followup

observed/pending

observed/retire-candidate

observed/unused

observed/used

operator-emotional

owner-attention

phase/02

phase/03

priority:p0

priority:p1

priority:p2

priority:p3

proposed

ready-for-agent

ready-for-operator

recovery

review:claude-reviewed

review:codex-reviewed

review:dziadek-reviewed

review:needs-human

risk/exposure

risk/process

risk/product

risk/runtime

safety:external-write

safety:no-prod-mutation

safety:prod-impact

safety:secret-touch

size/large

size/medium

size/small

size/tiny

size/unknown

source/adr

source/agent-generated

source/manual

source/operator-chat

source/voice-note

status:blocked

status:codex-ready

status:merged:pending-evidence

status:needs-evidence

status:operator-needed

status:parked

tier/full

tier/lite

tier/stacked

tier:0-platform-substrate

tier:1-iskra-value-layer

tier:2-tools-products-modules

type:bug

type:chore

type:docs

type:feat

type:policy

type:research

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

pdurlej/platform!162

No description provided.