Repository navigation
Conversation
Implement OpenSpec task 3.3 (issue #7): the mTLS status view now exposes a documented contract that keeps approval and execution states distinct. - GET /v1/broker/requests/{request_id} returns execution_state alongside the unchanged approval lifecycle status; the vocabulary is closed (not_executed, executing, succeeded, failed, uncertain) and the broker reports only not_executed, so approval, lease fetch and certificate issuance never imply execution. - Unknown stored states fail closed; cross-identity reads stay refused without field disclosure. - Contract and limits documented in docs/architecture.md; task 3.3 ticked with the issue link. Verification: uv sync --frozen, ruff check/format --check, pytest (269 passed), examples/pve_authorization.py, uv build, check_estate_identifiers.py, openspec validate --strict, git diff --check: all PASS.
There was a problem hiding this comment.
Independent snapshot review of f8a766accbf7...e3e0feee95cd in mikomiel/privilege-broker by pi on codex-lb (gpt-6.1-sol).
Verdict: APPROVE
Reviewer output
Findings
No concrete reachable failure found in the reviewed change.
privilege_broker/pve/service.py:1069–1095preserves existing approval wire values and adds a separateexecution_state, alwaysnot_executed. Unknown stored approval states are rejected rather than exposed.privilege_broker/pve/service.py:1269–1273checks both original identity and source before constructing the status view, preserving the nondisclosing 403 response.- The change is focused, introduces no dependency or database migration, and leaves lease/certificate authorization paths unchanged. Documentation accurately limits the execution claim. No secret values were apparent in the diff.
- The supplied PR title follows Conventional Commits.
Optional test improvements
-
Exercise actual certificate issuance and lease retrieval.
tests/test_pve_service.py:828–851uses the default client without a certificate signer, so its certificate call reaches thenot_readygate rather than issuance. The test at lines 917–925 modifies only a copied row. Extend the existing real-issuance test to read status after successful lease retrieval and certificate issuance, assertingcertificate_fetched is Trueandexecution_state == "not_executed". -
Exercise fail-closed behavior through HTTP.
tests/test_pve_service.py:905–915tests the helper directly. Add an invalid stored status and verify the original client receives a generic 409 without request fields, while another identity still receives exactly the nondisclosing 403. This would cover route wiring and authorization ordering as well as helper validation.
Review scope and limitations
Read the complete supplied diff, the full service implementation, relevant fixtures and certificate tests, and the applicable repository standards and OpenSpec design.
Tests, lint, builds, scanners and OpenSpec validation were not run; no command-execution tool was available. Commit messages/history and exact-head CI evidence were not available in the supplied material, so I could not independently verify Conventional Commit compliance across the range or the PR’s validation claims. No relevant source file requested for inspection remained unread because of an access failure.
VERDICT: APPROVE
Evidence: not written. This review is evidence, not merge authorization.
Change and reason
Implement OpenSpec task 3.3: the agent-facing status view now exposes a documented contract that keeps approval and execution states distinct. One decision: add a closed
execution_statevocabulary to the mTLS status read and document that the broker only ever reportsnot_executeduntil a trusted executor boundary exists.GET /v1/broker/requests/{request_id}(mTLS, original identity and source only) returnsexecution_statealongside the unchanged approval lifecyclestatus(pending/approved/denied/expired/cancelled).execution_statevocabulary is closed:not_executed,executing,succeeded,failed,uncertain. The broker reports onlynot_executed; approval, lease fetch and certificate issuance are authorization events, never execution evidence._agent_status_viewrejects them); cross-identity reads stay 403 with no field disclosure.docs/architecture.md; OpenSpec task 3.3 ticked with the issue link.Linked issues
Closes #7
Verification
Source head:
ccd5b20b9b82c98e966f244322bf11b295921c2d.uv sync --frozenuv run ruff check .anduv run ruff format --check .uv run python -m pytest -q: 269 tests, including 7 newTestAgentStatusContracttestsuv run python examples/pve_authorization.pyuv builduv run python scripts/check_estate_identifiers.pyopenspec validate extract-pve-foundation --strict --no-interactivegit diff --checktest_status_view_fails_closed_on_unknown_staterejects a storedsucceeded/completedstatus;test_approved_is_authorized_but_not_executedproves approval plus the certificate gate never report execution.Limits and risk
No executor exists in this source yet, so
execution_stateis alwaysnot_executed; outcome ingestion lands with tasks 4.x/5.1 through a trusted boundary. No agent-supplied execution evidence is accepted.Approval lifecycle wire values are unchanged; existing PVE request, lease and certificate contracts are preserved.
This PR touches broker code and tests, so it requires exact-head CI, independent Gandalf review and a human merge; no merge, protection change or release is performed here.
Tests and documentation match the change.
Public-content scans cover the intended commits, blobs and messages.
No production credentials or real deployment identifiers are included.
Source, merge, release and deployment states remain distinct.