Repository navigation
Conversation
Make GitHub Issues the repository work queue and retain OpenSpec as the technical acceptance and source-evidence owner. Link remaining tasks and distinguish full issue resolution from partial PR references. Refs #6
There was a problem hiding this comment.
Independent snapshot review of f6d85ca00c35...b9d8eddf041d in mikomiel/privilege-broker by pi on codex-lb (gpt-6.1-sol).
Verdict: APPROVE
Reviewer output
Findings
No blocking defects or concrete reachable failures found in the supplied diff.
- Canonical tracking rule:
CONTRIBUTING.md:9–19requires an issue before repository work, including work already covered by OpenSpec. It distinguishes full resolution (Closes) from partial or related work (Refs) and forbids closing issues with unmet criteria.AGENTS.mddelegates to this rule rather than defining an alternative. - OpenSpec ownership: The README and task-file introduction retain technical design, acceptance criteria and source evidence in OpenSpec while assigning the work queue to GitHub Issues. Existing acceptance text and completion states are unchanged.
- Task mappings: All seven unfinished numbered tasks are linked: 3.3 → #7, 4.1–4.3 → #8–#10, and 5.1–5.3 → #11–#13. Follow-up links assign closeout to #14, the release-action update to #15 and version/changelog preparation to #16. None of these links marks product work complete or authorizes publication.
- Security and merge boundaries: The private
SECURITY.mdreporting route remains intact and is reinforced in the guide and template. Existing exact-head review, human-merge, sensitive-path and separate publication/deployment requirements are preserved. The merge-policy code is unchanged and classifies this PR’s agent instructions, template and tests as human-merge paths. - Scope and conventions: The supplied
docs:title follows Conventional Commits. The change is focused on workflow documentation, links and source-contract tests; it introduces no broker behavior or new merge gate.
Optional suggestions
-
Strengthen the documentation contract test —
tests/test_scaffold.py:45–57.
It checks both reference tokens but not the instructions assigningClosesto full resolution andRefsto partial work. Reversing those meanings while preserving the tokens would still pass. Assert the classification instructions and the issue-before-work requirement, with corresponding rejecting controls. -
Align the governance summary —
docs/repository-standards.md:11–12.
The existing statement that OpenSpec “owns its implementation tasks” is less explicit than the new ownership split. Consider linking toCONTRIBUTING.mdand describing OpenSpec as the technical criteria/evidence owner to avoid ambiguity.
Review limits
Read the complete supplied diff, all six changed files, and relevant security, governance, OpenSpec, CI and merge-policy context. No required source material was unreadable.
This was static, read-only review. I did not run tests, builds, scanners, OpenSpec validation or Git commands. GitHub issue contents, issue creation, existing PR-body preservation, closing-reference readback and hosted checks were not available for independent verification; the task mappings above describe the authored source links, not verified external issue acceptance. Those external claims are not broker runtime acceptance.
VERDICT: APPROVE
Evidence: not written. This review is evidence, not merge authorization.
There was a problem hiding this comment.
Independent snapshot review of f8a766accbf7...675aa24a21ea in mikomiel/privilege-broker by pi on codex-lb (gpt-6.1-sol).
Verdict: APPROVE
Reviewer output
Findings
No blocking defects or concrete reachable failures found in the supplied range.
The change is focused on issue-first tracking and its documentation contracts. It preserves OpenSpec’s technical acceptance ownership, distinguishes closing references from related-work references, and retains the private vulnerability-reporting route. No broker behavior, credential handling, workflow permissions, or merge-policy implementation changes appear in the diff. The changed instructions, template, and tests remain human-merge-only under the existing path policy.
The PR title follows Conventional Commits (docs:). I did not inspect commit messages.
Optional suggestion
tests/test_scaffold.py:46–56— strengthen the mandatory-issue assertion. The test checks the phrase “including work covered by an active OpenSpec change,” but not the requirement to open or reuse an issue before starting work. Removing that requirement while retaining the checked phrase would leave the test passing. Consider asserting the complete mandatory instruction, with a rejecting control that removes it. This is non-blocking: the current documentation states the requirement correctly.
Review coverage and limits
- Read the complete supplied diff, all six changed files, and relevant security, governance, CI, merge-policy, and OpenSpec configuration files.
- No secret values or real deployment identifiers were apparent in the changed content; this was not a substitute for a secret scan.
- NOT RUN: tests, lint, build, OpenSpec validation, link-check execution, and public-content scanners. No command-execution tool was available.
- Not independently verified: GitHub issue contents, duplicate checks, PR #2/#3 body preservation and closing links, hosted checks, or live protection settings. Those external materials and commit history were not available through the supplied review artifacts.
- The PR’s reported verification names source head
b9d8eddf…, whereas this review covers675aa24a…; those reported results do not establish exact-head validation for the reviewed head.
VERDICT: APPROVE
Evidence: not written. This review is evidence, not merge authorization.
Change and reason
Make GitHub Issues the repository work queue and connect resolving PRs to their issues. Keep OpenSpec as the technical design, acceptance-criteria and merged-source-evidence owner.
closingIssuesReferencesreadback confirms the connections.Closes #<issue-number>for full resolution andRefs #<issue-number>for partial or related work.Linked issues
Closes #6
Refs #7, #8, #9, #10, #11, #12, #13, #14, #15, #16 for backlog links only. This PR does not implement the remaining product features or resolve the dependency/release-preparation PRs.
Verification
Source head:
b9d8eddf041d45fb028e06aecbdbc8d01529e083.uv sync --frozen.uv run ruff check .anduv run ruff format --check ..uv run python -m pytest -q: 264 tests, Python 3.14.8. Existing Starlette/SlowAPI deprecation warnings remain visible.uv build.openspec validate extract-pve-foundation --strict --no-interactive.git diff --check.Limits and risk
This is a source workflow/documentation and contract-test change. It changes no broker behavior, target policy, credentials, App permissions, protection settings or workflow execution permissions.
The documentation/template tests protect the authored contract; they do not add a new GitHub merge gate. Existing trusted CI and independent-review requirements remain in force.
Human merge required: this PR changes agent instructions, the PR template and tests. The authoring agent will not merge manually, enable a standing auto-merge order or bypass protection.
No PVE deployment, consumer cutover, tag, release or package/image publication is performed or authorized by this PR.