[mikomiel]

Change and reason

Extract the next bounded extract-pve-foundation source stage: the PVE approval store, real WebAuthn HTTP service and passkey UI, restricted local SSH certificate signer, and server-owned Discord delivery. This implements OpenSpec tasks 3.1 and 3.2; authenticated execution/outcome interfaces, agent bridge, worker, dispatcher and end-to-end acceptance remain pending.

Preserve PVE canonical wire versions and the approval store's schema from the pinned retained source. No schema migration or consumer source-owner cutover is included. Require explicit store, persistent signer, target policy and certificate source restriction. Reject unsupported SSH keys before approval, including valid RSA keys that cannot receive the supported Ed25519 certificate.

Standalone adaptations replace private imports with package imports and standard-library HTTP, move UI templates into wheel assets, remove implicit shared temporary state and import-time autostart, close factory-owned state through ASGI lifespan, and use FastAPI-owned notification tasks. Notification text is English with UTC expiry.

Verification

• PASS — 262 tests each on Python 3.11, 3.12 and 3.14; Ruff lint and format check; frozen uv synchronization.
• PASS — real generated P-256 WebAuthn registration/assertion tests: RP, origin, challenge, user presence, user verification, counter rollback, wrong signing key and replay. Only the external mTLS frontend is substituted.
• PASS — real local OpenSSH certificate tests, one-time/concurrent fetch, source/identity isolation, enrollment bootstrap, expiry, audit and notification safety.
• PASS — negative controls: disabling UV enforcement separately for registration and assertion makes each targeted refusal test fail with an unexpected successful approval/enrollment. Source was not changed by these controls. A strengthened valid-RSA fixture also demonstrated the missing early-refusal gap before the shared validator was applied.
• PASS — the pinned and extracted BrokerStore._setup_schema ASTs are identical. Final expiry corrections deliberately add explicit authority transactions; the claim is schema parity, not byte-identical transition statements. Valid canonical PVE payloads retain the already extracted protocol implementation.
• PASS — documented authorization example (no operation executed), wheel/sdist build, and clean installed-wheel import, packaged UI asset and configured-app lifespan smoke outside the checkout.
• PASS — strict OpenSpec validation, relative Markdown links, whitespace checks, full pre-push Gitleaks and private-identifier scans of history, tracked blobs, commit messages and author metadata.
• Hosted exact-head checks and independent Gandalf review are separate publication gates; local tests are not a substitute.

Review correction

Review 5883527063 identified stale expiry checks before certificate issuance. Two deterministic threaded cases reproduced issuance after request expiry, including a reader-persisted expired row. The corrected gate now parses key material before one issuance lock, samples production time inside that lock, and validates identity, current status, request deadline, lease deadline and one-time consumption together before signing. Regression coverage also checks an independently expiring, genuinely signed shorter lease.

The corrected head also documents the implemented single-CIDR contract rather than claiming list support, and adds reproducible tests for factory-owned SQLite closure, bootstrap consumption after reopen/secret rotation, database/WAL/shared-memory permissions and missing/unrestricted certificate-source configuration. No schema or PVE wire-format change was needed. All 262 tests pass again on Python 3.11, 3.12 and 3.14; the corrected source requires fresh exact-head hosted checks and independent review.

The follow-up review 5883729287 identified the same pre-lock timestamp pattern in approval and lease retrieval. The correction covers expiry-enforcing store transitions consistently: approval, enrollment, status/options, denial/cancellation and lease retrieval sample time after acquiring their state lock. Verification/signing stages also recheck deadlines before committing or returning authority. Nine actual held-RLock cases cover these boundaries; eight failed on the prior head while its already-correct certificate case passed. A clock-placement mutation makes the held-lock certificate regression fail if time moves immediately before lock acquisition. Three further cases reject expiry during verification/signing. The controls leave source unchanged. The stale repository-standards UI claim is corrected.

Review 5883935200 identified the remaining interval after lease signing and during SQLite writer acquisition. Approval and registration now refresh their deadline guards after BEGIN IMMEDIATE and before authority writes. Lease/certificate decisions acquire an explicit SQLite writer transaction before their fresh checks and commit audit/consumption atomically. Tests use a genuinely signed lease that advances the clock, and a real competing SQLite writer for approval, registration, lease and certificate paths. The first three cases failed on the previous implementation before correction; all current cases pass. Refusal leaves no approval lease, counter advance, challenge deletion or approval event. The installed-wheel CI smoke now constructs a synthetically configured app and enters/exits its lifespan outside the checkout. A duplicate lexical scaffold assertion was replaced by the existing structural permission owner; mutation controls verify root/job write permissions are still rejected, and GitHub secret expressions remain prohibited.

Hosted CodeQL flagged the former custom JSON-in-script interpolation on both operator pages. The latest correction removes dynamic data from executable JavaScript: JSON is escaped with standard html.escape(..., quote=True) into quoted HTML data attributes, then parsed from the browser dataset. HTML-parser tests preserve the exact original hostile strings; removing attribute escaping makes both safety cases fail. A real local Chromium probe, with outbound requests blocked, verified both pages parse, display original hostile text and execute no injected script. No CodeQL alert dismissal, exclusion or scanner-policy relaxation was used. This is a browser DOM/XSS check, not hardware-passkey or production acceptance. Fresh exact-head CodeQL and review remain required after this source change.

Limits and risk

Human merge required. This changes broker code, tests, dependencies and a build smoke check. The authoring agent must not merge or enable an auto-merge order. This is a new review request, not another review of unchanged PR #1.

No PVE calls, Discord delivery, deployment, credential/target-policy changes, consumer cutover, tags, package/image publication or product release were performed. Certificate signing invokes only local ssh-keygen with fixed restricted argv. Approval and certificate issuance do not establish execution success.

Deployers must provide an isolated local TLS frontend, verify mTLS, strip/overwrite metadata, keep untrusted clients away from the backend and disable Uvicorn proxy-header rewriting. The app alone cannot establish those host facts. Use one worker. Browser approval URLs are opaque but their recipients can deny a request; general passkey administration/recovery is not implemented. Notification tasks are not a durable delivery queue.

Software-authenticator tests prove protocol/crypto checks, not physical user presence, browser compatibility or runtime acceptance. The pinned dependencies emit upstream deprecation warnings; these are not suppressed or represented as test failures.

• 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.

[ecomwizards-gandalf]

Independent snapshot review of 7ec42d7713f9...9c91ae7e3202 in mikomiel/privilege-broker by pi on codex-lb (gpt-6.1-sol).

Verdict: REQUEST_CHANGES

Reviewer output

Findings

1. [P2] Certificate issuance does not revalidate an expired request inside the issuance lock

File: privilege_broker/pve/service.py:900–938, particularly 931–932

The status, request deadline and lease deadline checks happen before the final issuance critical section. After reacquiring the lock, the code reloads the request but checks only certificate_fetched.

A reachable interleaving is:

1. A certificate fetch passes its preliminary checks shortly before expiry.
2. Another request’s signing operation holds the store lock across that deadline.
3. A concurrent status read marks the first request expired.
4. The waiting fetch reloads that expired row, sees certificate_fetched == 0, and signs anyway.

Even without the status read, the issuance section uses the timestamp captured before waiting, so its second read cannot detect elapsed expiry. Local ssh-keygen runs under this same lock with a 30-second timeout, making this more than an instruction-scale race.

This violates the task 3.1 expiry boundary: an expired, unconsumed request can issue a certificate. A lease approved near the request deadline can still be valid at that point, so this is not necessarily limited to issuing an unusable certificate.

Requested fix: Re-evaluate current request status and both deadlines inside the issuance critical section using current production time. Add a deterministic concurrency regression test that advances across expiry between preliminary validation and issuance, including a case where another reader has already persisted expired.

Optional suggestions

• Correct the certificate-source configuration contract. docs/configuration.md:46 advertises comma-separated CIDRs, but CertificatePolicy passes the entire string to ipaddress.ip_network() at privilege_broker/pve/authorization.py:521. For example, 192.0.2.0/24,198.51.100.0/24 causes factory startup to fail. Document the currently supported single CIDR, or deliberately implement and test list validation.
• Commit regression coverage for lifecycle and persistence claims. The supplied tests exercise genuine WebAuthn verification and concurrent one-time certificate fetching, but I did not find tests for factory-owned store closure through lifespan, bootstrap refusal after reopening/rotating the configured secret, or database/WAL permissions. These would make the documented guarantees reproducible rather than dependent on separate local smoke checks.

Scope and validation limits

The change is appropriately bounded around tasks 3.1 and 3.2. I did not count the explicitly deferred execution interfaces, bridge, worker or dispatcher as missing functionality. Source inspection confirms explicit RP/origin/challenge/UV verification arguments, rejecting default agent authentication, original identity/source checks, server-owned notification destinations, and sanitized notification error summaries.

I read the complete supplied diff, including uv.lock, plus the relevant authorization code, OpenSpec requirements and repository standards.

Not run: tests, lint, builds, scans or runtime reproductions; only read-only file-inspection tools were available. The pinned original extraction source and Git history were not available in the supplied snapshot, so I could not independently verify SQL-statement parity or commit-message conventions. Hosted exact-head checks were also not verified.

VERDICT: REQUEST_CHANGES

Evidence: not written. This review is evidence, not merge authorization.

[ecomwizards-gandalf]

Independent snapshot review of 7ec42d7713f9...672aeaefc56d in mikomiel/privilege-broker by pi on codex-lb (gpt-6.1-sol).

Verdict: REQUEST_CHANGES

Reviewer output

Findings

[P2] Approval still evaluates expiry using time sampled before waiting for the store lock

File: privilege_broker/pve/service.py:735–752

approve() samples production time before acquiring self._lock, then uses that timestamp for request expiry, challenge expiry and lease creation.

A reachable interleaving is:

1. Certificate signing for request A holds the shared store lock while running ssh-keygen.
2. Approval for request B samples time just before B’s deadline, then waits for that lock.
3. The lock becomes available after B’s deadline.
4. Approval reads B using the stale timestamp, accepts the ceremony and persists a signed lease with status approved, instead of refusing the expired request.

This requires neither multiple workers nor database modification; signing and approval endpoints run concurrently within the documented single-worker configuration. The corrected certificate gate subsequently refuses issuance, so this is not a remaining certificate-gate bypass. Nevertheless, the approval boundary still creates signed authorization after request/challenge expiry.

Move production clock sampling inside the approval lock and validate deadlines using that fresh timestamp. Add a deterministic held-lock regression asserting refusal, no lease, no counter advancement and no request_approved event.

The related retrieval path at service.py:863–875 also samples time before its locked read, allowing a lease fetch waiting across the request deadline to succeed. Apply the same fresh-clock discipline there.

Requested certificate-race correction

The previously blocking certificate interleavings appear resolved by source inspection in privilege_broker/pve/service.py:900–947:

• Submitted-key fingerprint parsing precedes the issuance lock.
• Production time is sampled after acquiring that lock.
• Current status, original identity/source, request deadline, lease deadline, key binding and one-time consumption are checked while holding it.
• Signing and consumption remain inside the same critical section.

tests/test_pve_service.py:1679 covers request expiry, reader-persisted expiry and an independently expiring, genuinely signed shorter lease. These tests were read, not executed.

Optional suggestions

• tests/test_pve_service.py:1679: Add an explicit lock-contention variant. The current fingerprint barrier catches the previous implementation, but would not catch moving the clock sample immediately after fingerprinting and just before lock acquisition.
• docs/repository-standards.md: Remove the stale statement that no runnable UI has been extracted.

The cumulative change otherwise stays within the stated approval-service/notification stage. Execution/outcome interfaces, bridge, worker and dispatcher remain explicitly deferred; approval and certificate issuance are not presented as execution success.

Review coverage and limitations

• Read the complete 5,464-line supplied diff, including dependency-lock changes, plus supporting authorization, policy, security, configuration and OpenSpec material.
• No repository files were modified.
• NOT RUN: tests, lint, builds, public-content scans or hosted-check verification; available tools provided read-only file inspection, not command execution.
• Unavailable for independent verification: the retained private source snapshot and SQL-parity comparison, commit history/messages for Conventional Commits compliance, and exact-head hosted CI/review evidence. No requested diff material remained unread.

VERDICT: REQUEST_CHANGES

Evidence: not written. This review is evidence, not merge authorization.

[ecomwizards-gandalf]

Independent snapshot review of 7ec42d7713f9...41e59e0b52f0 in mikomiel/privilege-broker by pi on codex-lb (gpt-6.1-sol).

Verdict: REQUEST_CHANGES

Reviewer output

Finding

P2 — Approval still commits after expiry during lease signing

File: privilege_broker/pve/service.py:769–808

BrokerStore.approve() refreshes the clock after WebAuthn verification, but before lease_factory.create_lease(), which signs the lease. It then serializes and commits the lease without another deadline check.

A reachable sequence remains:

1. WebAuthn verification completes just before the request deadline.
2. Lease creation/signing crosses that deadline—for example, signing is delayed or the thread is descheduled.
3. The method persists status='approved', the signed lease and the updated passkey counter, deletes the challenge, and returns successful approval.

The RLock prevents concurrent state changes, not passage of time. Later fetch gates reject the expired request, so this is not a demonstrated expired-certificate bypass. However, approval incorrectly succeeds and commits authorization state after expiry, contrary to the claimed post-signing correction.

The regression at tests/test_pve_service.py:1899–1947 does not cover this interval: its approval case advances time during assertion verification; its slow-signing case exercises only certificate signing.

Requested correction: Refresh production time and recheck request/challenge deadlines after lease creation/signing, before committing approval. Check after any potentially blocking transaction acquisition as well. Add a deterministic regression that creates a genuinely signed lease, advances the clock past the deadline before returning it to approve(), and asserts refusal with no stored lease, counter update, challenge deletion or request_approved event.

Review assessment

• Read the complete supplied cumulative diff, including uv.lock, and supporting authorization, policy, security, governance and OpenSpec files.
• The reviewed expiry-enforcing paths now sample time after acquiring the state lock. Certificate issuance also checks expiry after signing.
• The cumulative scope fits the planned approval-service/notification extraction; deferred execution components are identified explicitly.
• The supplied PR title follows Conventional Commits. Commit-message compliance across the range could not be checked.

Optional suggestion

Extend .github/workflows/ci.yml’s installed-wheel smoke test to instantiate a synthetically configured app and enter/exit its lifespan. Currently it proves service import and packaged-template availability, but not configured factory startup or cleanup outside the checkout.

Validation and unavailable material

This was a read-only source review. I did not run tests, lint, builds, mutation controls or security scans; command-execution tools were unavailable. No supplied diff content remained unread. Git history, the pinned retained-source SQL comparison material, and hosted exact-head check results were not available, so those claims were not independently verified.

VERDICT: REQUEST_CHANGES

Evidence: not written. This review is evidence, not merge authorization.

[ecomwizards-gandalf]

Independent snapshot review of 7ec42d7713f9...9d283558e3f2 in mikomiel/privilege-broker by pi on codex-lb (gpt-6.1-sol).

Verdict: APPROVE

Reviewer output

Findings

No blocking, concrete reachable failure found in the supplied exact-head source under the documented single-worker, single-store and isolated TLS-frontend deployment contract.

The requested corrections are present:

• privilege_broker/pve/service.py:508–590, 744–865 — registration and approval refresh production time after BEGIN IMMEDIATE acquisition. Approval also rechecks request, challenge and signed-lease deadlines after signing and serialization, before writing approval authority, advancing the counter or deleting the challenge.
• privilege_broker/pve/service.py:896–1006 — lease and certificate decisions acquire the SQLite writer transaction before sampling time. Certificate signing is followed by another deadline check; consumption and issuance audit commit together.
• The added tests exercise genuine lease signing, real competing SQLite writers, held state locks, cryptographic WebAuthn refusals and real local certificate issuance rather than relying only on source-text assertions.
• Notification destinations remain server-owned; browser approval does not release leases or certificates. Execution, bridge, dispatcher and outcome functionality remain explicitly deferred.

The workflow change retains read-only permissions. The changed scope is coherent with tasks 3.1/3.2, and the supplied diff contains no apparent production secret values.

Optional suggestions

• Expiry-audit consistency: In service.py:896–1006, an expired read can call get_broker_request(), write request_expired, then raise and roll that update and event back through _transaction(). Likewise, approval’s final deadline refusal does not itself persist expiry. Authority remains correctly refused, and a later status read records expiry, so this is non-blocking. Consider recording expiry independently after rollback and testing audit state before a subsequent status read repairs it.
• Stale comment: service.py:391–398 still says lease and certificate fetches audit outside their critical sections. Update it to reflect the new transaction/lock ownership.

Validation and limitations

I read the complete supplied diff, including the lockfile changes, and supporting authorization, policy, specification, workflow and repository-standard files. No commands, tests, builds, mutation controls or scans were run; the available tools were read-only file inspection tools.

Commit history/messages, the pinned retained-source snapshot, hosted exact-head checks and external review evidence were not available in the supplied tree. I therefore could not independently verify Conventional Commit history, schema-AST parity or the reported multi-version test results. The PR title follows Conventional Commit form. Approval here is a source-review conclusion, not deployment acceptance or authorization to merge automatically; this change still requires human merge.

VERDICT: APPROVE

Evidence: not written. This review is evidence, not merge authorization.

[ecomwizards-gandalf]

Independent snapshot review of 7ec42d7713f9...533ee2148205 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 supplied range.

The UI-context correction is sound by source inspection:

• privilege_broker/pve/service.py:1044–1050 serializes JSON and applies html.escape(..., quote=True) before interpolation into quoted HTML attributes.
• Both page handlers use that helper (service.py:1371, 1433–1437). Dynamic values no longer enter executable JavaScript.
• privilege_broker/pve/templates/approval.html:29–47 and enrollment.html:8–15 retrieve data through dataset and JSON.parse. Approval details are rendered using textContent, not HTML interpretation.
• tests/test_pve_service.py:2171–2204 checks both hostile-content exclusion from markup and exact recovery of the original values after HTML/JSON parsing.

The cumulative extraction retains the important reviewed boundaries: default-deny agent authentication, identity/source isolation, real WebAuthn verification with required UV, explicit target policy, fresh deadline checks after lock/writer acquisition, transactional certificate consumption, and fixed local certificate-signing arguments. Notification delivery remains server-owned and cannot approve requests.

The change is cohesive with the staged OpenSpec extraction. Documentation distinguishes authorization from execution and leaves bridge/dispatcher/outcome work deferred. CI remains read-only; the removed lexical permission assertion has an existing structural owner in tests/test_workflow_policy.py:110–117. I found no apparent production secret values in the reviewed additions.

Optional suggestion

Consider retaining the outbound-blocked Chromium probe as a repeatable automated regression. The HTMLParser tests cover attribute escaping and value preservation well, but do not themselves execute the templates’ JavaScript.

Review scope and limitations

I read the complete supplied diff and relevant supporting source, specifications, and repository standards. No changed-file content in that diff remained unread.

This was static, read-only review. I did not run tests, builds, linters, mutation controls, public-content scans, CodeQL, or Chromium. The reported validation results remain independently unverified.

Commit history/messages and Git identity metadata were unavailable in the supplied checkout, so Conventional Commit compliance across the range could not be assessed. The pinned retained-source snapshot and hosted exact-head results were also unavailable for independent comparison. Approval here is source-review approval, not deployment acceptance or merge authorization; human merge remains required.

VERDICT: APPROVE

Evidence: not written. This review is evidence, not merge authorization.