feat(acp): isolate signing behind durable reply broker - #3311
Conversation
Co-authored-by: Ryan Laubscher <ryanlaubscher80@gmail.com> Signed-off-by: Ryan Laubscher <ryanlaubscher80@gmail.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a28f209db1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .map(parse_auth_tag) | ||
| .transpose() | ||
| .map_err(|e| format!("invalid broker auth tag: {e}"))?; | ||
| let client = BuzzClient::new(relay_url, keys, auth_tag, auth_tag_json) |
There was a problem hiding this comment.
When ACP uses its supported/default ws:// or wss:// relay URL, this passes it verbatim into BuzzClient, whereas the CLI normally applies normalize_relay_url. The client appends the HTTP /query and /events paths, which reqwest cannot access through a WebSocket scheme, so the first parent lookup fails and relay_reply cannot publish any reply. Convert the URL to http:// or https:// before constructing the client.
AGENTS.md reference: AGENTS.md:L122-L134
Useful? React with 👍 / 👎.
| let mut reader = BufReader::new(stream); | ||
| let bytes = tokio::time::timeout(CONNECT_TIMEOUT, reader.read_line(&mut response)) | ||
| .await |
There was a problem hiding this comment.
When the caller omits idempotency_key and the broker takes longer than five seconds after receiving the request, this timeout returns an error without the internally generated key. The broker continues processing and may publish the event, but the caller can only retry without a key, generating a new UUID and a duplicate signed reply. Include the generated key in every ambiguous post-write error so the exact request can be retried.
Useful? React with 👍 / 👎.
| unsafe { | ||
| std::env::set_var("BUZZ_PRIVATE_KEY", TEST_PRIVATE_KEY); | ||
| std::env::set_var("BUZZ_AUTH_TAG", TEST_AUTH_TAG); | ||
| } |
There was a problem hiding this comment.
This test introduces an unsafe block solely to set process environment variables, violating the repository's unconditional prohibition on unsafe code. Use a safe or subprocess-isolated test approach instead; the corresponding restoration block later in the test must also be removed.
AGENTS.md reference: AGENTS.md:L113-L116
Useful? React with 👍 / 👎.
1 participant
Scope\n\nImplements the approved P0/P1 delivery foundation without activation, credential rotation, or automatic recovery.\n\n- removes raw signing credentials and durable-state path from worker/MCP environments\n- hides credential environment values in CLI and ACP help\n- adds a harness-owned constrained reply broker with a pre-send signed-event ledger, deterministic event ID reconciliation, and delivery-unknown blocking\n- adds the MCP relay_reply client and pair-scoped durable state path\n\n## Verification\n\nAt a28f209:\n\n- cargo fmt --check\n- env -u BUZZ_ACP_LAZY_POOL cargo test -p buzz-acp --lib (618 passed)\n- cargo test -p buzz-dev-mcp --lib (96 passed)\n- cargo test -p buzz-cli --lib (251 passed)\n- cargo check -p buzz-acp -p buzz-dev-mcp\n\nDesktop cargo check is packaging-blocked because this source clone lacks binaries/buzz-acp-aarch64-apple-darwin; it reaches the build script before that missing generated resource.