From aeb21434bde2f579bea8121e3ba3554c2d940d1b Mon Sep 17 00:00:00 2001 From: codex Date: Tue, 3 Nov 2026 22:41:22 +0100 Subject: [PATCH] RPF-WP-0069-T07: approve sets approved only once every required role approves. credential-change.py approve set status approved after the first reviewer; for role-gated request types validation then refused the second approve before it was recorded. append_decision now records each approval and keeps the prior status while the lane validator still reports a missing role approval. Co-Authored-By: Claude Opus 5.5 Assistant: claude-code Assistant-Model: opus Assistant-Process: 509845@bnt-lap001 Assistant-Session: ed7a7d95-d8eb-455c-b594-963fb1830f76 --- WORK-RECORDS.md | 2 +- docs/credential-change-approval.md | 8 +++ scripts/credential-change.py | 37 ++++++++++++- tests/test_credential_change.py | 55 +++++++++++++++++++ .../RPF-WP-0069-bounded-executor-lanes.md | 11 +++- 5 files changed, 108 insertions(+), 5 deletions(-) diff --git a/WORK-RECORDS.md b/WORK-RECORDS.md index c9a6b83..0e11373 100644 --- a/WORK-RECORDS.md +++ b/WORK-RECORDS.md @@ -240,7 +240,7 @@ | task | RPF-WP-0069-T04 | done | — | workplans/RPF-WP-0069-bounded-executor-lanes.md | | task | RPF-WP-0069-T05 | wait | — | workplans/RPF-WP-0069-bounded-executor-lanes.md | | task | RPF-WP-0069-T06 | cancel | — | workplans/RPF-WP-0069-bounded-executor-lanes.md | -| task | RPF-WP-0069-T07 | todo | — | workplans/RPF-WP-0069-bounded-executor-lanes.md | +| task | RPF-WP-0069-T07 | done | — | workplans/RPF-WP-0069-bounded-executor-lanes.md | | task | RPF-WP-0069-T08 | done | — | workplans/RPF-WP-0069-bounded-executor-lanes.md | | task | RPF-WP-0069-T09 | wait | — | workplans/RPF-WP-0069-bounded-executor-lanes.md | | task | RPF-WP-0070-T01 | done | — | workplans/RPF-WP-0070-mas-evidence-recovery.md | diff --git a/docs/credential-change-approval.md b/docs/credential-change-approval.md index 8dc31e8..382d271 100644 --- a/docs/credential-change-approval.md +++ b/docs/credential-change-approval.md @@ -191,6 +191,14 @@ renders the reviewed non-secret `bao policy write` and `bao write auth/.../role` commands for a platform operator; the actual secret value is still provisioned through approved OpenBao/operator custody only. +For request types that require an explicit approval from every role in +`review.required_approvers` (for example `workload-kv-write-next-version`, +`attended-service-credential-delivery`, `attended-host-key-delivery`), each +`approve` records that reviewer's approval. The CCR moves to `approved` only when +the last required role approves; until then it keeps its prior status and the +tool prints the roles still awaited. Run `approve` once per role with +`--reviewer `. + The same operations can be exposed through chat by having the agent create the proposal, show the rendered summary, then call the CLI only after the human gives an explicit approval phrase. diff --git a/scripts/credential-change.py b/scripts/credential-change.py index 92cec1d..8bb2842 100755 --- a/scripts/credential-change.py +++ b/scripts/credential-change.py @@ -2,6 +2,7 @@ from __future__ import annotations import argparse +import copy import difflib import json import os @@ -480,9 +481,15 @@ def validate_kv_metadata(value: Any, errors: list[str]) -> None: def validate_ccr(path: Path) -> tuple[dict[str, Any], list[str], list[str]]: errors: list[str] = [] - warnings: list[str] = [] scan_for_secrets(path, errors) ccr = load_yaml(path) + data_errors, warnings = validate_ccr_data(ccr) + return ccr, errors + data_errors, warnings + + +def validate_ccr_data(ccr: dict[str, Any]) -> tuple[list[str], list[str]]: + errors: list[str] = [] + warnings: list[str] = [] for field in ( "id", "kind", @@ -526,7 +533,7 @@ def validate_ccr(path: Path) -> tuple[dict[str, Any], list[str], list[str]]: errors.extend(lane_module(ccr).validate(ccr, REPO_DIR)) else: errors.append("unsupported request_type") - return ccr, errors, warnings + return errors, warnings def render_summary(ccr: dict[str, Any], warnings: list[str]) -> str: @@ -1857,6 +1864,21 @@ def render_status(payload: dict[str, Any]) -> str: return "\n".join(lines) +ROLE_APPROVAL_MISSING = "explicit role approval missing: " + + +def pending_role_approvals(ccr: dict[str, Any]) -> list[str]: + """Roles the request type still requires before the CCR may be `approved`. + + Only lane validators that enforce role approvals report these; for other + request types the list is always empty. + """ + candidate = copy.deepcopy(ccr) + candidate["status"] = "approved" + errors, _warnings = validate_ccr_data(candidate) + return [e[len(ROLE_APPROVAL_MISSING):] for e in errors if e.startswith(ROLE_APPROVAL_MISSING)] + + def append_decision(path: Path, status: str, reviewer: str, comment: str) -> dict[str, Any]: reject_secret_text(comment, "review comment") ccr, _warnings = validate_or_exit(path) @@ -1872,6 +1894,10 @@ def append_decision(path: Path, status: str, reviewer: str, comment: str) -> dic "comment": comment, } ) + # An approval is recorded per reviewer, but the CCR only becomes `approved` + # once every role its request type requires has approved (RPF-WP-0069-T07). + if status == "approved" and pending_role_approvals(ccr): + status = ccr["status"] ccr["status"] = status ccr["updated"] = datetime.now(timezone.utc).date().isoformat() dump_yaml(path, ccr) @@ -2261,7 +2287,12 @@ def command_record_evidence(args: argparse.Namespace) -> int: def command_decision(args: argparse.Namespace, status: str) -> int: path = resolve_ccr(args.ref) ccr = append_decision(path, status, args.reviewer, args.comment) - print(f"[OK] {path.name} -> {status}") + print(f"[OK] {path.name} -> {ccr['status']}") + if status == "approved" and ccr["status"] != "approved": + print( + f"[INFO] {args.reviewer} approval recorded; still awaiting: " + + ", ".join(pending_role_approvals(ccr)) + ) if args.record_state_hub: openbao = ccr["openbao"] auth = openbao["auth"] diff --git a/tests/test_credential_change.py b/tests/test_credential_change.py index b6c78a8..a4afc39 100644 --- a/tests/test_credential_change.py +++ b/tests/test_credential_change.py @@ -644,6 +644,61 @@ class CredentialChangeTests(unittest.TestCase): type("Args", (), {"ref": str(self.unapproved_ccr())})() ) + def test_role_gated_approval_stays_open_until_every_required_role_approves(self) -> None: + # RPF-WP-0069-T07: the first approval used to set `approved`, after which + # validation refused the second reviewer's approve before it was recorded. + source = REPO_DIR / ( + "credential-change-requests/" + "CCR-2026-0058-net-kingdom-bind-executor-svc-authelia-write.yaml" + ) + tmp = Path(tempfile.mkdtemp()) + self.addCleanup(shutil.rmtree, tmp, True) + copied = tmp / source.name + shutil.copy2(source, copied) + data = credential_change.load_yaml(copied) + self.assertEqual( + data["review"]["required_approvers"], ["platform-owner", "net-kingdom-owner"] + ) + data["status"] = "proposed" + data["review"]["comments"] = [] + credential_change.dump_yaml(copied, data) + + ccr = credential_change.append_decision(copied, "approved", "platform-owner", "ok") + self.assertEqual(ccr["status"], "proposed") + self.assertEqual(credential_change.pending_role_approvals(ccr), ["net-kingdom-owner"]) + _ccr, errors, _warnings = credential_change.validate_ccr(copied) + self.assertEqual(errors, []) + with self.assertRaises(SystemExit): + credential_change.require_apply_ready(ccr, "apply-plan") + + ccr = credential_change.append_decision(copied, "approved", "net-kingdom-owner", "ok") + self.assertEqual(ccr["status"], "approved") + self.assertEqual(credential_change.pending_role_approvals(ccr), []) + self.assertEqual( + [c["reviewer"] for c in ccr["review"]["comments"]], + ["platform-owner", "net-kingdom-owner"], + ) + _ccr, errors, _warnings = credential_change.validate_ccr(copied) + self.assertEqual(errors, []) + + def test_approval_by_unlisted_reviewer_does_not_complete_role_gate(self) -> None: + source = REPO_DIR / ( + "credential-change-requests/" + "CCR-2026-0058-net-kingdom-bind-executor-svc-authelia-write.yaml" + ) + tmp = Path(tempfile.mkdtemp()) + self.addCleanup(shutil.rmtree, tmp, True) + copied = tmp / source.name + shutil.copy2(source, copied) + data = credential_change.load_yaml(copied) + data["status"] = "proposed" + data["review"]["comments"] = [] + credential_change.dump_yaml(copied, data) + credential_change.append_decision(copied, "approved", "unit-test", "ok") + ccr = credential_change.append_decision(copied, "approved", "platform-owner", "ok") + self.assertEqual(ccr["status"], "proposed") + self.assertEqual(credential_change.pending_role_approvals(ccr), ["net-kingdom-owner"]) + def test_approve_records_comment_but_unconfirmed_claim_still_blocks_apply(self) -> None: copied = self.unapproved_ccr() ccr_dir = copied.parent diff --git a/workplans/RPF-WP-0069-bounded-executor-lanes.md b/workplans/RPF-WP-0069-bounded-executor-lanes.md index 5bf085f..535f0dc 100644 --- a/workplans/RPF-WP-0069-bounded-executor-lanes.md +++ b/workplans/RPF-WP-0069-bounded-executor-lanes.md @@ -147,7 +147,7 @@ rotates its own password (hub decision 3d0c8f86), so the executor needs no LLDAP ```task id: RPF-WP-0069-T07 -status: todo +status: done priority: medium state_hub_task_id: "b458e369-73b4-515e-ae01-e8afc0dd35eb" ``` @@ -158,6 +158,15 @@ net-kingdom-owner approval was recorded by hand in the tool's format). Make vali that is still collecting role approvals, or have `approve` set `approved` only once all required roles are present, with a test. +Done 2026-11-03 (second option): `append_decision` records every approval, but sets `approved` only when the +request type's validator reports no `explicit role approval missing` error. Until then the CCR keeps its prior +status and `approve` prints the roles still awaited. The State Hub decision sync uses the same path. Lanes without +role gates are unchanged. `validate_ccr_data` validates in memory. Tests: +`test_role_gated_approval_stays_open_until_every_required_role_approves` and +`test_approval_by_unlisted_reviewer_does_not_complete_role_gate` (CCR-2026-0058 copy). The guide in +`docs/credential-change-approval.md` is updated. Three unrelated `test_credential_change.py` failures predate this +change (CCR drift). + ## Broker admits oidc-service-client@0.2 for mint-secret