Skip to content

fix(buzz-acp): make resolved relay URL authoritative for ACP child sp… - #5695

Open
azhodl wants to merge 1 commit into
block:mainfrom
azhodl:acp-child-relay-url
Open

fix(buzz-acp): make resolved relay URL authoritative for ACP child sp…#5695
azhodl wants to merge 1 commit into
block:mainfrom
azhodl:acp-child-relay-url

Conversation

@azhodl

@azhodl azhodl commented Aug 12, 2026

Copy link
Copy Markdown

…awns

A child spawned by AcpClient::spawn inherited the parent environment and only received persona env vars under operator-wins semantics, so with a missing or stale parent BUZZ_RELAY_URL it silently fell back to the ws://localhost:3000 default and died with "404 no community for this host" after consuming the mention. The mention is lost: the harness has already taken the work, and the child never reaches the relay to reply.

This is reachable whenever the operator passes --relay-url without also exporting BUZZ_RELAY_URL, which the flag's existence invites.

  • config.rs: Config::from_args pushes the resolved relay URL (CLI over env over default, per clap) into persona_env_vars, so every pool spawn/respawn path carries it. Preflight: an unparseable relay URL now fails terminally (missing-config) before the harness subscribes or takes work; the error prints only the URL, never keys.
  • acp.rs: AcpClient::spawn force-sets BUZZ_RELAY_URL from extra_env, overriding both missing and stale parent env. All other keys keep operator-wins semantics.
  • Tests: child-env probe for unset-parent and stale-parent scenarios; full-chain CLI-to-PoolStartup extra_env test; missing-config preflight test.

No unsafe. New unwrap()/expect() calls are confined to #[cfg(test)] modules.

Summary

Related issue

Testing

…awns

A child spawned by AcpClient::spawn inherited the parent environment and
only received persona env vars under operator-wins semantics, so with a
missing or stale parent BUZZ_RELAY_URL it silently fell back to the
ws://localhost:3000 default and died with "404 no community for this
host" after consuming the mention. The mention is lost: the harness has
already taken the work, and the child never reaches the relay to reply.

This is reachable whenever the operator passes --relay-url without also
exporting BUZZ_RELAY_URL, which the flag's existence invites.

- config.rs: Config::from_args pushes the resolved relay URL (CLI over
  env over default, per clap) into persona_env_vars, so every pool
  spawn/respawn path carries it. Preflight: an unparseable relay URL now
  fails terminally (missing-config) before the harness subscribes or
  takes work; the error prints only the URL, never keys.
- acp.rs: AcpClient::spawn force-sets BUZZ_RELAY_URL from extra_env,
  overriding both missing and stale parent env. All other keys keep
  operator-wins semantics.
- Tests: child-env probe for unset-parent and stale-parent scenarios;
  full-chain CLI-to-PoolStartup extra_env test; missing-config preflight
  test.

No unsafe. New unwrap()/expect() calls are confined to #[cfg(test)]
modules.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: blinc-claude[bot] <297159213+blinc-claude[bot]@users.noreply.github.com>
@azhodl
azhodl requested a review from a team as a code owner August 12, 2026 19:33
serina-mcfall added a commit to launchpad-26/buzz that referenced this pull request Aug 12, 2026
…t disagrees

STEP 4 of #116, built the way Serina chose rather than the way
the plan prescribed. The plan said to reuse launchpad-pr-check.yml's body regex.
That is the mechanism sibling PR #126 is replacing as bug #125, and it is wrong
in a way this record must not repeat.

Probed against four real pull requests before deciding:

- fork #92 — the body carries a visible `Closes #n` and GitHub reports NOTHING,
  because the PR's base was not the default branch. Merging it closes no issue,
  and a regex says the board updates.
- fork #86 — closes TWO issues. The plan's `re.search` reports one. A control now
  reproduces the single search and requires the record to hold more than it.
- upstream block#5695 — the only keyword sits in an unfilled `<!-- Fixes block#1234 -->`
  placeholder. GitHub ignores it; so does the text half, which strips comments
  first.
- fork #124 — `Refs #116`, correctly not a closing reference.

So closingIssuesReferences decides `present` and the issue list, the body still
supplies which keyword was written, and a disagreement between them is recorded
instead of resolved silently — a disagreement is the shape of #125 and is signal
a reviewer wants.

An unreadable GitHub answer yields `present: null` — unknown, never false. "We
could not ask" and "it closes nothing" differ on whether the board updates, and
closing_refs is therefore skip-only: the run still exits 0 carrying the skip.

closing_issue grew from three fields to six, added to the module's enumerated
field list in the same commit as the plan requires.

Refs #116

Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
serina-mcfall added a commit to launchpad-26/buzz that referenced this pull request Aug 12, 2026
…t disagrees

STEP 4 of #116, built the way Serina chose rather than the way
the plan prescribed. The plan said to reuse launchpad-pr-check.yml's body regex.
That is the mechanism sibling PR #126 is replacing as bug #125, and it is wrong
in a way this record must not repeat.

Probed against four real pull requests before deciding:

- fork #92 — the body carries a visible `Closes #n` and GitHub reports NOTHING,
  because the PR's base was not the default branch. Merging it closes no issue,
  and a regex says the board updates.
- fork #86 — closes TWO issues. The plan's `re.search` reports one. A control now
  reproduces the single search and requires the record to hold more than it.
- upstream block#5695 — the only keyword sits in an unfilled `<!-- Fixes block#1234 -->`
  placeholder. GitHub ignores it; so does the text half, which strips comments
  first.
- fork #124 — `Refs #116`, correctly not a closing reference.

So closingIssuesReferences decides `present` and the issue list, the body still
supplies which keyword was written, and a disagreement between them is recorded
instead of resolved silently — a disagreement is the shape of #125 and is signal
a reviewer wants.

An unreadable GitHub answer yields `present: null` — unknown, never false. "We
could not ask" and "it closes nothing" differ on whether the board updates, and
closing_refs is therefore skip-only: the run still exits 0 carrying the skip.

closing_issue grew from three fields to six, added to the module's enumerated
field list in the same commit as the plan requires.

Refs #116

Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>

@Chessing234 Chessing234 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the new spawn test mutates the real BUZZ_RELAY_URL process-wide, and config.rs — a file this PR also edits — writes down the opposite convention at :2766: "We pass the value via the CLI flag ... rather than std::env::set_var to avoid test-parallelism races on shared env state. The env-var wiring is covered by the clap #[arg(env)] attribute itself."

that matters more for this key than for the BUZZ_AUTH_TAG precedent you cite: BUZZ_RELAY_URL is declared at config.rs:240 with #[arg(env = "BUZZ_RELAY_URL", default_value = "ws://localhost:3000")], so any test in the binary that builds CliArgs through clap's env fallback while scenario 2 holds "http://localhost:3000" picks it up. cargo test runs these in parallel threads, so across the crate's ~776 tests that's a live flake vector rather than a theoretical one.

scenario 1 needs no mutation at all. scenario 2 needs it only because spawn reads std::env::var_os directly — taking that lookup as a parameter (defaulting to std::env::var_os) would make the stale-parent case testable without touching global state.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants