[mikomiel]

Change and reason

Implement OpenSpec task 4.1: the agent-side execution components move into the product as explicit-configuration libraries. One decision: bridge, client and worker take required constructor arguments only — no environment reads, no built-in addresses, paths or identities — so a missing value fails closed.

• privilege_broker.pve.client: mTLS broker HTTP client (BrokerClientConfig: https broker URL, CA, client cert/key, timeout). Transient failures (network, timeout, HTTP 429/502/503/504) raise BrokerUnavailable; final HTTP refusals raise BrokerHTTPError.
• privilege_broker.pve.bridge: agent-facing request creation with an ephemeral Ed25519 key (removed on any failure), strict create-response validation (canonical UUID, pending status, integer expiry) and pending-input publication. read_status returns result / pending / unknown.
• privilege_broker.pve.worker: poll-and-dispatch worker preserving the broker wire contract — lease fetched once, one-time certificate, {"action", "ct_id", "lease", "signature"} on stdin over fixed non-interactive SSH options. Local expiry plus grace bounds retries; transient pre-dispatch broker failures retry on the next poll; a consumed certificate (HTTP 409) is terminal with no re-mint; wrong-node answers (exit 3 + not_on_this_node) fail over to the next configured node with the same certificate; a transport timeout is unknown and never retried; key/certificate material never appears in results or logs.
• Synthetic tests cover failure classification, node failover, bounded retries, local expiry, terminal states, the dispatcher stdin contract and secret non-leakage. Docs updated (architecture.md, configuration.md); task 4.1 ticked with the issue link.

Linked issues

Closes #8

Verification

Source head: 1c9cd7a0302425a6a64d77a9938d6b6bf244e144.

• PASS — uv sync --frozen
• PASS — uv run ruff check . and uv run ruff format --check .
• PASS — uv run python -m pytest -q: 337 tests, including 75 new bridge/client/worker tests
• PASS — uv run python examples/pve_authorization.py
• PASS — uv build
• PASS — uv run python scripts/check_estate_identifiers.py
• PASS — openspec validate extract-pve-foundation --strict --no-interactive
• PASS — git diff --check
• PASS — Gitleaks 8.30.1 (SHA-256-pinned archive) over git history, tracked blobs and commit messages: no leaks; estate-identifier scan over patch and messages: clean
• Negative controls — timeout is reported unknown with exactly one SSH attempt (no replay); HTTP 409 on the certificate fetch is a terminal error with exactly one fetch (no re-mint); the result JSON and worker logs contain neither key nor certificate material.

Limits and risk

• The target dispatcher and durable claim store remain OpenSpec task 4.2 (issue Extract the typed PVE dispatcher and durable replay store #9); this PR adds no executor on the target and no new dependency.

• Broker wire formats, SSH argv semantics and result statuses are unchanged; the 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.

[ecomwizards-gandalf]

Independent snapshot review of f8a766accbf7...1c9cd7a03024 in mikomiel/privilege-broker by pi on codex-lb (gpt-6.1-sol).

Verdict: REQUEST_CHANGES

Reviewer output

Findings

1. [P1] The default HTTP opener permits leaving the configured HTTPS/mTLS boundary.
   privilege_broker/pve/client.py:99–102
build_opener() installs urllib’s default redirect and plain-HTTP handlers alongside the supplied HTTPS handler. Consequently, a status or lease GET receiving 302 Location: http://… is followed, and its response is accepted as a broker response without TLS. Cross-origin HTTPS redirects can also reuse the client TLS context outside the configured broker origin. Checking only the initial URL does not enforce the promised transport boundary.
   Fix: Reject redirects, or explicitly restrict every redirect to the configured HTTPS origin. Add regression coverage exercising redirect handling rather than only a fake opener.

2. [P2] Filesystem publication failures leave orphaned private keys.
   privilege_broker/pve/bridge.py:119–134
   Key cleanup covers only the broker call and response validation. If the broker successfully creates a request but creating input/ or writing its JSON fails—for example, because input is an existing regular file, permissions deny publication, or storage is full—request_mutation() raises while retaining the private key. There is then no valid pending input for the worker to expire and clean up, and the caller never receives the request ID. Key-writing/chmod failures before the try likewise bypass cleanup. This contradicts the claim that the ephemeral key is removed on any failure.
   Fix: Cover key creation through successful publication with failure cleanup, remove partial input files, and publish inputs atomically. Test failures at directory creation and input writing.

3. [P2] Final HTTP refusals during status polling are treated as transient.
   privilege_broker/pve/worker.py:288–294
BrokerHTTPError inherits from RuntimeError, so this handler catches definitive status refusals such as 401, 403 and 404 and silently preserves the pending request. For example, after the service returns 403 because the requester’s source address changed, every subsequent poll repeats the refusal while read_status() continues returning pending until local expiry. The client distinguishes final refusals, but the worker discards that classification here; this also contradicts the documented “any final refusal is terminal” behaviour.
   Fix: Handle BrokerUnavailable as retryable and finish final HTTP refusals as error, with key cleanup. Add status-endpoint refusal tests alongside the existing transient-status test.

4. [P2] HTTP error responses bypass the response-size bound.
   privilege_broker/pve/client.py:124–130
   Successful responses are capped at 1 MiB, but HTTP errors use unrestricted exc.read() and decode the entire body before truncating the exception message. An oversized proxy error response can therefore exhaust worker memory before a 429/502/503/504 is classified as retryable. _DETAIL_LIMIT bounds only the final message, not resource consumption.
   Fix: Bound the error-body read before decoding, close the response, and test that an oversized error body is never read without a limit.

Optional suggestions

• Update SECURITY.md: its implemented-boundary description still says there is no execution worker.
• The change is otherwise focused on the stated extraction decision, and the PR title follows Conventional Commits. The failure-path tests above would make the verification claims substantially stronger; the existing non-leakage test covers only benign successful output.

Review scope and unavailable material

I read the complete supplied diff and relevant service, authorization, policy, security and repository-standard files. I did not run tests, builds, scanners or other commands; reported PR checks were not independently validated. The supplied snapshot contains no Git metadata, so I could not inspect the range’s actual commit messages or independently verify its commit ancestry.

VERDICT: REQUEST_CHANGES

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