Repository navigation
feat(pve): extract authorization foundation and align GitHub tooling - #1
Conversation
Preserve PVE canonicalization and signing contracts while requiring explicit certificate source restrictions. Add synthetic tests, a clean wheel build, contributor/docs/example structure, trusted metadata-only review governance and grouped dependency tooling. Release-please uses the configured secret reference in release-PR-only mode. This source change does not deploy a service, issue production authority, publish a tag or release, or remove the retained broker.
There was a problem hiding this comment.
Independent snapshot review of d81e4190e833...db3157720f21 in mikomiel/privilege-broker by pi on codex-lb (gpt-6.1-sol).
Verdict: APPROVE
Reviewer output
Findings
No blocking, concrete source defect identified in the supplied range.
Static inspection found:
- Consistent PVE canonicalization identifiers and signed-claim structure. This is not independent proof of parity with the retained implementation.
- Policy loading rejects unknown actions, malformed grants and boolean target IDs. Certificate policy requires an explicit network and rejects unrestricted IPv4/IPv6 sources.
- Clear documentation that this is an authorization library—not an approval service, executor or evidence of successful execution.
- Default-branch checkout and metadata-only evaluation in the privileged review/automerge workflows. The merge policy denies sensitive and unknown paths and examines both names of a rename.
- Explicit
skip-github-release: true, separating release-PR preparation from tag/release publication.
Optional suggestions
-
Commit deterministic compatibility vectors.
tests/test_pve_authorization.py:115checks different random nonces rather than determinism for identical inputs;:644computes its expected digest using the same canonicalization helper under test. Add frozen synthetic vectors for all six actions, covering canonical bytes, digests, signed claims and signatures. These would make the reported extraction-parity result reproducible without the private source snapshot. -
Test the automation orchestration, not only its predicates.
scripts/github_pr_automation.py:199–277contains important collection, revocation, ref-revalidation and merge sequencing. Add fake-API tests for pagination failure, changed refs, failed status revocation and uncertain merge responses. The existing policy/workflow tests do not exercise those runtime paths. -
Make refusal tests exercise refusals.
tests/test_pve_authorization.py:790is named as testing rejection of every excluded action, but only checks the constant’s size and string prefixes. Parameterize actual lease-issuance attempts over every excluded action.
Review coverage and limits
- Read the complete 4,369-line diff and the relevant supporting source, workflows, tests and specifications.
- Made no repository changes.
- NOT RUN: pytest, Ruff, builds, examples, secret scans or mutation controls; no command-execution tool was available.
- The frozen retained implementation was not supplied, so I could not independently reproduce the claimed compatibility comparison.
- Full commit history/messages, hosted exact-head CI, repository protection settings and external dependency/action identities were not independently verified.
This is an independent source-review approval, not a CI verdict, runtime acceptance, merge instruction or release authorization. This bootstrap PR remains human-merge-only; its proposed privileged workflows cannot approve it.
VERDICT: APPROVE
Evidence: not written. This review is evidence, not merge authorization.
One-time governance bootstrap — existing approval reusedThe maintainer authorized this specific bootstrap transition; no second review is requested and the source head is unchanged.
The maintainer must perform the human merge. The implementation agent has not merged this PR or enabled an auto-merge order. No App permissions, credentials, PVE consumers, or deployed PVE services changed. After merge, verify that the trusted default-branch The bootstrap paragraph in Recorded by the Pi implementation session as a status and maintainer-authorization record, not as another review. |
Bootstrap exception closedPR #1 was human-merged as The trusted default-branch publisher was verified after merge. Protected main now requires strict The normal path was then exercised by docs-only PR #4: Gandalf approved its new head, exact-head CI passed, and trusted GitHub Actions merged it as Release-please prepared release PR #2 only. No tag, product release, package/image publication, consumer cutover or PVE deployment was performed. The source remains a pre-release authorization foundation; service and execution extraction are still pending in OpenSpec. Status record from the Pi implementation session, not a review or release authorization. |
Change and reason
First source stage of extract-pve-foundation, plus the shared GitHub repository baseline requested by the maintainer.
privilege_broker/pve/.RELEASE_PLEASE_TOKENby reference only, withskip-github-release: true: release-PR preparation is allowed, tag/release creation is not.Verification
uv run ruff check .and formatting check.uv build; clean installed-wheel import/configuration smoke test outside the checkout on Python 3.11; wheel/sdist exclude local private configuration.openspec validate extract-pve-foundation --strict; relative documentation links;git diff --check.Repository configuration verified
Protected main has strict Actions-bound
checks/public-content, admin enforcement, stale-review dismissal, one native independent approval, and no force pushes or deletion. Private vulnerability reporting and dependency alerts are enabled; secret scanning and push protection are enabled. CodeQL default setup is configured for Python and Actions. Description/topics and the shared label vocabulary are set.The native approval requirement is a deliberate bootstrap transition, not the final App-status arrangement: this PR's writer workflows cannot be trusted before they exist on main. After human merge, verify the trusted publisher and add its Actions-bound
reviewcontext before aligning native review settings. Do not weaken protection to merge this PR.Limits and risk
This is an authorization library, not the complete approval service. WebAuthn UI, notifications, HTTP/MCP clients, worker and dispatcher extraction remain in the active OpenSpec tasks. Importing this library does not authenticate a remote caller or prove that a human approved an operation.
No existing deployment or PVE policy has changed. No production credentials, private source history or deployment identifiers are included. No tag, release, package or image has been published. The retained broker implementation is unchanged; no consumer cutover or removal is attempted.
Documentation result: source_docs (
docs/architecture.md,docs/configuration.md,docs/repository-standards.md,docs/releases.mdand updated contributor/OpenSpec documents).Merge owner: human maintainer, after green required exact-head CI and independent review. Authoring agents do not manually merge, enable standing auto-merge, or bypass protection.