Conversation
…en helper Three independent developer-tooling gaps hit while contributing from a Windows host with no push access to block/buzz. scripts/bootstrap-windows.ps1 — hermit publishes no Windows build, so bin/flutter 404s on hermit-mingw64_nt-<build>-amd64.gz and activate-hermit leaves flutter unresolvable. The script reads the pinned version from bin/.flutter-<version>.pkg rather than hardcoding it, resolves the archive from the release manifest, verifies SHA256 before extracting, and is idempotent. Windows publishes no arm64 Flutter SDK at all, so arm64 hosts fall back to the x64 archive under emulation instead of failing outright. scripts/post-screenshots.sh — previously pushed blobs to origin and derived raw URLs from a hardcoded block/buzz, both of which fail for fork contributors. Hosting remote is now auto-detected or set with --remote, raw URLs follow the remote actually pushed to, the PR comment still targets block/buzz, and a rejected non-fast-forward rebuilds the tree on the new tip and retries rather than clobbering a concurrent push. Adds --dry-run. mobile/test/helpers/golden_shot.dart — the repo had no golden infrastructure. Produces PR-quality mobile screenshots with no Mac and no simulator. Encodes the two traps: flutter_test renders tofu without real fonts, and Lucide IconData carries fontPackage, so the family must be registered as packages/lucide_icons_flutter/Lucide or every glyph is silently blank. Font bytes are read synchronously because FontLoader.load() awaiting an errored future hangs indefinitely. Verified on Windows with the pinned toolchain (Flutter 3.41.7 / Dart 3.11.5): flutter analyze clean, dart format 407 files 0 changed, flutter test 1423 passed including 6 new golden-helper tests. Signed-off-by: Michael Feth <michael@jira-flow.com>
Replaces prose-inferred agent dispatch (via `send_message` reverse-parsing
of `@Name` mentions) with a pubkey-addressed workflow action. Two failure
modes of the mention path are structural: (a) two channel members sharing
a display name make the mention ambiguous and wake no one; (b) a rename
silently rewrites the target. `assign_agent` binds dispatch to the
target's hex pubkey and membership-checks at emit time.
Contract per Airy's Slice-1 review:
- Singular `agent_pubkey` — group fan-out is a separate explicit step.
- The relay sink emits exactly two `p` tags: workflow owner (attribution)
and `agent_pubkey` (wake). Text is NEVER reverse-parsed for `@Name`.
- Fail-closed: `AssigneeNotMember` if the agent is not a channel member.
- Schema accepts a static 64-hex pubkey OR a single `{{...}}` template
placeholder (e.g. `{{trigger.author}}`); mixed literal+template strings
are rejected so a stray name cannot smuggle an identity in. The resolved
value is re-validated as 64-char lowercase hex at dispatch time.
- No default `ActionSink` impls; the new `assign_agent` method is a hard
compile-time obligation on any implementor.
Tests: 11 schema tests (parse/round-trip/hex-shape rejects), 3 executor
template-resolution tests, 2 postgres-gated relay-sink integration tests
covering the duplicate-name repro (two "Winnie" members, only the
selected pubkey wakes) and the non-member fail-closed path.
Non-goals for this slice (per Airy): task leases, reviewer-independence,
approval suspend/resume, run-event emission. Run history is addressed
separately by wiring the CLI to the existing DB-backed
`GET /workflows/{id}/runs` endpoint in a follow-up PR.
Signed-off-by: Michael Feth <michael@jira-flow.com>
…re drops
When a subscriber's outbound data channel is full, EVENT fan-out frames
are dropped silently: the socket stays up, the grace counter increments
server-side only, and the client's live view keeps a permanent hole
until the grace limit kills the connection (or a reconnect replay
accidentally heals it). Lane 3 of the sync-lag diagnosis proved this is
the first broken boundary behind "messages arrive late until restart".
Give the client a machine-readable gap signal instead:
- RelayMessage::sync_required() formats the extension frame
["BUZZ_SYNC_REQUIRED","backpressure"]. The constructor is deliberately
monomorphic so the relay cannot emit a frame the wire contract does
not define; protocol.rs module docs define the contract and delivery
rules (unknown relay->client heads are non-fatal per NIP-01 client
convention).
- The EVENT fan-out path — ConnectionManager::send_fanout_frame,
renamed from send_to_text_bytes to make its role explicit — routes
through try_send_fanout_frame:
* every dropped frame (full data channel, closed data channel, or
connection already gone) increments buzz_fanout_dropped_frames_total
with no sampling, so a silently lost EVENT always leaves a
telemetry trail;
* on a Full data channel with a live socket, the gap signal is queued
on the connection's priority control channel — never on the data
channel it signals about, never as a human-readable NOTICE — and
the existing consecutive-full grace counter decides cancellation
exactly as before;
* when the control channel is itself full or closed, in-band
signaling is impossible: cancel at once and let reconnect replay
recover the missed events;
* cancellation is idempotent under concurrent drops: a
compare-exchange on ConnEntry::backpressure_disconnect_counted
picks exactly one of the parallel droppers to increment
buzz_ws_backpressure_disconnects_total (a runtime flood harness
showed the unguarded is_cancelled->cancel sequence inflated the
disconnect counter 46x for one connection).
- Non-fan-out sends (CLOSED notices etc.) share the try_data_send
success path but keep the old drop semantics — no gap signal, no
dropped-frame counting.
Tests: 12 new tests plus a wire-shape pin (exact bytes) in the protocol
table test — full/closed/gone/ctrl-full branches, grace-counter
interaction, the signal never riding the data channel, disconnect
idempotence (including an 8-thread concurrent-drop case), and an
end-to-end case through the real send_fanout_frames path.
Gates on this Windows host: cargo fmt --check clean; cargo clippy
-p buzz-relay --tests --all-features -- -D warnings clean; cargo test
-p buzz-relay --lib = 885 passed / 43 ignored, failing only on 3
pre-existing host failures that reproduce on the base commit f956e6f
untouched by this diff (2 bash-HMAC tests resolve `bash` to WSL on
this host; the redis-gated mesh_demo test returns 504 with local
Redis up).
Co-authored-by: Michael Feth <michael@jira-flow.com>
Signed-off-by: Michael Feth <michael@jira-flow.com>
parse_all_action_types exists to catch exactly this and was passing green while blind to the new variant: it asserted steps.len() == 7 over a fixture that never included assign_agent, so an 8th action type could ship without the test noticing. Extended the fixture to 8 steps, bumped the assertion, and added the matches! arm. ARCHITECTURE.md still read '7 action types' above a 7-row table. Added the row and corrected the count. Both are the same pattern: a new entry in a closed enumeration that lives in several places the compiler does not check. 'git grep -ln <new-variant>' should return more than the files you edited. Verified: cargo test -p buzz-workflow 169 passed / 0 failed; cargo fmt clean. Co-authored-by: Michael Feth <michael@jira-flow.com> Signed-off-by: Michael Feth <michael@jira-flow.com>
BUZZ_AGENT_REQUIRE_REPLY defaulted to off, relying on Desktop's mesh launcher to force it on per-process (insert_default_if_unset). Any buzz-agent run outside that path — manual/dev runs, other harnesses — silently missed the guard against a turn that does real work and never posts it anywhere. Flip the binary's own default to on, matching what Desktop already forces for its primary managed use case. An explicit BUZZ_AGENT_REQUIRE_REPLY=0 (agent, persona, or global env) still opts out — Desktop's insert_default_if_unset never overrides an explicit value either, so this doesn't change mesh-agent opt-out behavior. Left the two require_reply: false in config.rs's for_discovery() and llm.rs's test cfg() helper alone — both already run with the entire _Stop guard budget disabled (stop_max_rejections: 0), so require_reply is inert there regardless of value. Signed-off-by: Michael Feth <michael@jira-flow.com>
The default flip is incomplete without this. reply_guard_off_by_default spawned with no env override and asserted exactly one LLM call, with the docstring 'the invariant that keeps the feature free for everyone who hasn't opted in'. That invariant is precisely what the flip reverses, so the test does not merely need a new number — it needed inverting and renaming. reply_guard_on_by_default now asserts the unpublished turn is reminded. The BUZZ_AGENT_REQUIRE_REPLY=0 opt-out test is untouched and becomes the more important of the pair: it is now the only thing standing between a user who opted out and a guard that ignores them. Also updates the two places that documented the old policy: - relay_mesh.rs said 'Everywhere else it stays opt-in and unset', which the flip makes false. The mesh line is kept rather than deleted as newly redundant: it states the requirement at the layer that knows why small local models need it, instead of inheriting a default another crate could reasonably revisit. - docs/MCP_DRIVEN_HOOKS.md described the guard as '(BUZZ_AGENT_REQUIRE_REPLY=1)', which read as the opt-in switch. Now states the default and the opt-out. Co-authored-by: Michael Feth <michael@jira-flow.com> Signed-off-by: Michael Feth <michael@jira-flow.com>
An @mention of an agent that nothing picks up is today indistinguishable
from one that was picked up and is still being worked on: both are silence.
The relay fans out but does not track delivery, so a mention whose target is
not connected and subscribed at the instant of fan-out reaches nobody, and
nothing is ever published in response.
Define kind 44102, an agent-published receipt for a mention. Tags: h
(channel), e (the mention), p (its author), status of accepted or declined,
and for declined a machine-readable reason slug.
The declined half is the load-bearing one. The paths that dead-end a mention
today are precisely the ones where the harness knowingly decided not to act:
the author is outside respond_to (which defaults to owner-only, so any
co-worker mention lands here), no rule matched, a filter failed closed, or
it is busy and configured to drop. Each exits at a debug! log. A declined
receipt turns those into an explicit, explained outcome the sender can act
on.
Two relay gates are required, not one. required_scope_for_kind is an
exhaustive match whose fallthrough is Err("restricted: unknown event kind"),
so without an arm the relay rejects the publish outright -- this is why the
pre-existing kind 43002 KIND_JOB_ACCEPTED has never been publishable, and
why reusing it was rejected in favour of a fresh kind. requires_h_channel_scope
is the gate that routes the write through check_channel_membership; omitting
it would let a non-member inject receipts into any channel.
The kind is deliberately not p-gated: a receipt is channel-visible like the
eyes reaction it accompanies, so co-members and sibling agents can see that a
mention was received. Because the relay cannot verify that a signing key
belongs to an agent, clients must ignore receipts whose author is not a
pubkey the acknowledged message actually mentioned; NIP-MR.md states this as
a requirement.
44102 also joins the channel live-subscription kind set so clients receive
receipts, and the relay metrics bucket so ack volume is not lumped in with
unregistered traffic.
Spec: docs/nips/NIP-MR.md. Named MR (Mention Receipt) rather than MA, which
sat one transposition away from the existing NIP-AM.md in the same directory.
Signed-off-by: Michael Feth <michael@jira-flow.com>
Stacked on the kind-44102 definition.
The harness had exactly one immediate signal that a mention was received: a
fire-and-forget eyes reaction with a 500ms timeout whose failures are
swallowed at debug!. Its own docs call it cosmetic. That makes it unusable as
something a sender can rely on -- on a slow relay a perfectly healthy pickup
is indistinguishable from an agent that is not running at all.
Worse, three paths produced no signal whatsoever, and they are exactly the
paths where the harness knowingly decided not to act:
- the inbound author gate drops the event because the author is outside
respond_to, which defaults to owner-only, so any co-worker mentioning the
agent is dropped here. This is the most likely cause of a dead-ended
mention in practice and it is invisible in every direction.
- no rule matched, or a filter expression failed closed.
- DedupMode::Drop discarded the event because the channel is already in
flight. That path posts no eyes reaction and fires no steer, making it
the most completely silent outcome in the harness.
Publish a kind 44102 receipt at each decision point, carrying accepted or
declined plus a reason slug. A declined receipt is not a failure notice; it
is the agent saying "I received this and I am not going to answer, here is
why", which is what lets the sender act -- ask the owner to widen respond_to
-- instead of waiting on nothing.
Gated on event_mentions(): a receipt fires only for events that actually
p-tag this agent, never for the agent's own output, and never for kind 44102
itself. That last guard matters: a receipt p-tags the author it answers, so
a receipt is itself a mention of that agent, and two sibling agents on
wildcard-kind subscriptions would otherwise acknowledge each other's
acknowledgements indefinitely -- each round a stored event and a model turn,
no human involved, no terminating condition. A test asserts the p-tag
property that makes the loop possible so the guard cannot be quietly dropped.
The mention predicate is extracted from match_event's own require_mention
check rather than rewritten, so an agent acknowledges exactly the events a
require_mention rule treats as mentions; the existing
test_match_event_require_mention covers the refactor.
Publishing is spawned, never awaited, so a receipt cannot add latency to
dispatch or hold the event loop behind a slow relay. It gets a 5s timeout
rather than the reaction's 500ms and warns rather than debugs on failure: a
dropped receipt recreates the dead-end it exists to prevent.
Event construction is split into build_mention_ack_event so the tag shape --
which the desktop matches on -- is testable without a relay.
Note that accepted marks the start of a turn, not its completion: an agent
that accepts and then crashes or hangs publishes no further receipt. That
limitation is recorded in NIP-MR.md and is left to a follow-up.
Verified: this adds 7 tests, all passing. cargo test -p buzz-acp --lib is
752 passed / 24 failed at origin/main and 767 passed / 16 failed here (776
and 783 total respectively -- the 7 new tests account for the difference).
The failures are a flaky subprocess-and-timing set in acp::tests and
pool::tests on this Windows box; the count varies run to run, they reproduce
against a pristine origin/main checkout, and this commit touches none of them.
Signed-off-by: Michael Feth <michael@jira-flow.com>
Stacked on the harness receipt publisher. The harness now publishes a receipt for every mention it sees, which covers every case where an agent is running. It cannot cover the case where nothing is running: the relay fans out without tracking delivery, so if the agent is not connected and subscribed at the instant of fan-out the mention reaches nobody and no receipt is ever published by anyone. Silence is the only observable, and only the sending client is in a position to notice it. Track every agent mention on send, resolve it against incoming kind 44102 receipts, and after 30s report whatever never answered. The message then carries an inline footer naming what happened -- "Ada is not accepting messages from you", "Ada never picked this up, it may be offline" -- instead of sitting there looking delivered. Details worth calling out: Receipts are consumed in useLiveChannelUpdates, which subscribes across every joined channel, not in the per-channel subscription -- that one covers only the channel currently open and is torn down on switch, so consuming there would discard any receipt arriving after the user navigates away and then claim, 30s later, that the agent never picked the message up. Directly above the reply it did send. Registration reads the sent event's own p tags rather than the composer's explicit mentions. Those are the tags an agent actually matches on, and in a DM they include every participant even when the text contains no @ -- so explicit mentions alone would leave DM prompts untracked, which is the case with the strictest gate and the most likely to dead-end. Only agent pubkeys are tracked, via the known-agent set. Mentioning a colleague must never produce "they did not respond within 30 seconds". Registered in onSuccess rather than onMutate because a receipt references the real event id the agent saw, and the optimistic id is local-only. A receipt resolves an entry only when its author is a pubkey the message actually mentioned. Any member can publish a well-formed 44102 -- the relay cannot check agent-ness -- so without that check a third party could suppress someone else's warning. Both pubkeys are normalised, or the check fails silently and genuine receipts look forged. A reply clears tracking outright, on any referenced e tag rather than only the thread parent, because the harness anchors its reply to the thread root. A partial accept suppresses the warning for silent siblings: if a team mention reaches four agents and one takes it, the mention is answered and warning about the other three would be actively misleading. The 30s window is deliberately generous. A managed agent may still be booting when the mention is sent -- the desktop launches it as part of the send flow -- and a premature "nobody picked this up" on an agent that was merely starting is worse than a late one. Rendered through MessageTimeline's messageFooters slot, which existed fully wired with no caller. That keeps the notice clear of MessageRow's hand-written memo comparator, where a new prop would have been silently ignored (AGENTS.md gotcha 7). Store snapshots are content-cached per channel for the same reason: a getter that rebuilds its object or array would re-render forever through useSyncExternalStore. resetPendingMentionAckStore is wired into resetCommunityState per the community-switching contract, and deliberately does not clear its listener set, which would orphan any subscriber outliving the switch. Known limitation, stated in NIP-MR.md: accepted marks the start of a turn, not its completion. An agent that accepts and then crashes or hangs publishes no further receipt, so this does not warn about it. Nothing regresses versus today, but reviewers should not read accepted as a completion guarantee. Verified: 4971 desktop tests pass (17 in the new store), tsc clean, biome clean, check:px-text clean. Signed-off-by: Michael Feth <michael@jira-flow.com>
`reaction_add` wrapped `submit_event` in a 500ms timeout. `submit_event`
retries transient failures after sleeping 500ms, 1s, then 2s, with +/-20%
jitter -- so the first sleep alone lands in [400ms, 600ms). The caller's
timeout therefore fired *during* that first sleep in most cases, and in the
best case left ~100ms for a request that had already failed to finish in 500ms.
The retries were unreachable. Worse than unreachable: the only thing they
changed was the error the caller saw, replacing whatever the relay said with an
opaque `Elapsed` after the full budget had been spent sleeping.
`RestClient` grows `submit_event_once`, a single-attempt sibling, and
`reaction_add` uses it. The 500ms budget now buys one real attempt instead of
most of one backoff sleep, which is what the function's own comment ("returns
immediately on timeout or any error -- reactions are cosmetic") always claimed
it did. `bridge_post` and `submit_event` are unchanged for every other caller.
`reaction_remove` deliberately keeps the retrying path: its budget is 1s, which
clears even the worst-case first backoff with room to spend, so its retries are
genuinely reachable. Its magic `1_000` is now the named
`REACTION_REMOVE_TIMEOUT` so the contrast is visible at the definition site.
A guard test asserts both directions of that line -- that the add budget cannot
outlast the first backoff and the remove budget can. It is arithmetic over the
constants rather than behaviour, which is deliberate: it fails the moment
someone raises a backoff or lowers a budget, and that is the only way this
mismatch quietly comes back. `REST_RETRY_BASE_DELAYS` is `pub(crate)` so the
assertion reads the real ladder instead of a copy of it.
Signed-off-by: Michael Feth <michael@jira-flow.com>
`cargo test -p buzz-acp --lib` fails 16 tests on Windows at main. The names
suggest flaky timing. Only two of them are timing; the rest are deterministic,
and the dominant cause is that the tests were not running under the shell
anyone thought they were.
Dozens of fixtures stand up a fake ACP agent with
`AcpClient::spawn("bash", ["-c", script])`. That bare name goes through the
Win32 search order, which places %SystemRoot%\System32 ahead of PATH, and on
any machine with the WSL optional feature enabled System32\bash.exe is WSL's
launcher. Measured on this host, `uname -s` from a natively-spawned `bash` is
Linux, and System32 precedes all four Git for Windows entries on PATH.
Two things break once the fixture is a Linux process.
`read` silently returns nothing. Over the native Win32 anonymous pipe that
Stdio::piped() creates, WSL bash's `read` builtin completes successfully with
an empty value even though the bytes arrived -- `cat | wc -c` sees all 26 and
`head -n 1` returns the line; only `read` loses it. Git Bash on the identical
pipe returns the data (LEN=25 vs LEN=0). A fixture that echoes "$REQ" back
into a JSON literal therefore emits `"_receivedRequest":}}`, the read loop
logs a parse failure and skips it, the response is never matched, and the test
dies as AgentExited -- a symptom that looks nothing like its cause. That is
all six session_new_full_* tests and both goose_* tests.
Win32 paths stop resolving. WSL has no drive-letter namespace, so a capture
path like C:\Users\...\Temp\x.ndjson is treated as a relative filename and
created in the process cwd, which is the crate source directory. That is the
four pool.rs lifecycle tests, and it is why a test run left nine files in
crates/buzz-acp/ with `:` and `\` stored as the private-use code points U+F03A
and U+F05C that DrvFs substitutes for characters Windows forbids.
The repo already knows this hazard: ci.yml smoke-tests that the resolved bash
is not from System32, and buzz-dev-mcp ships a production resolver
(shell.rs::resolve_bash) built for exactly this. The test fixtures never got
the same treatment. testshell.rs mirrors that resolver's probe order --
BUZZ_TEST_SHELL, then PATH excluding %SystemRoot% and the WindowsApps alias
dir, then git.exe's sibling, then the standard install locations -- and is a
no-op off Windows. Its is_under_dir comparison is component-wise and
case-insensitive because Path::starts_with is case-sensitive on every
platform, so a PATH entry spelled C:\WINDOWS\System32 would otherwise slip
through.
It panics rather than falling back when nothing is found. A missing
prerequisite must be loud: the alternative is the failure mode this commit
exists to remove, a suite that looks green while asserting nothing.
Two further defects the interpreter fix does not cover:
spawn_steer_capture_script interpolated the capture path UNQUOTED, so bash ate
every backslash as an escape and redirected into a relative file named
C:UsersmeAppData...json. Verified this fails under Git Bash too -- quoting is
not a style preference here, it is the fix. quote_for_shell is now the single
place that knows it; pool.rs had the same idiom open-coded four times.
capture_path used fixed names in a shared %TEMP% directory, so two concurrent
`cargo test -p buzz-acp` runs -- two worktrees, or a rerun overlapping its
predecessor -- read each other's captures. Names are now unique.
Worth flagging for review: steer_writes_nothing_when_no_run_id_and_capability_
absent is not among the 16, because it was PASSING on Windows for the wrong
reason. Its whole assertion is `written.is_none()`, and `written` came from
reading a capture file that could never exist there, so it passed vacuously.
It is the only negative-space guard proving a steer stays off the wire with
neither a run id nor an advertised capability. It starts proving that again
here.
The remaining two failures are genuinely timing. `sleep` is an external binary
under MSYS, so each `echo; sleep 0.05` iteration pays a process spawn: the
real inter-line gap is ~98ms, up to ~244ms under load, against a 100ms idle
budget. The fixture was silent for longer than its own deadline, so the read
loop timed out correctly and the test failed for a reason unrelated to the
behaviour under test. Both budgets are widened here, with the floor held at 2x
the idle timeout -- the invariant that keeps the assertion meaningful, since at
or below 1x a run with no reset at all would satisfy it. That turned out not
to be sufficient on its own; see the follow-up commit, which measures the
residual flake and fixes it structurally.
The three steer hard-deadline tests had the same unsoundness in a different
place: their clock started before the shell had booted, so they were measuring
MSYS startup. They now wait for a READY sentinel and use a fixture timeline
with ~1s of slack on either side of the deadline instead of ~0.4s. Two of the
three were passing before this commit only on the luck of WSL starting faster
than Git Bash.
760 passed / 16 failed -> 783 passed, with the two idle-reset tests still
flaking intermittently until the follow-up commit. The suite runs in 44s
instead of 108s because it is no longer waiting on timeouts. Totals reconcile:
776 at base plus the 7 new testshell tests. Nothing was deleted, skipped, or
cfg-gated away to get there; the four pre-existing #[cfg(unix)] gates are
untouched. A test run now leaves zero files in the source tree.
Verified on Windows 11 / MSVC only -- see the PR for what that leaves open.
Signed-off-by: Michael Feth <michael@jira-flow.com>
The 16 Windows failures fixed in the previous commit had been on main for a while. The reason nobody noticed is worth stating plainly: buzz-acp's ~760 unit tests are executed by no CI job on any platform. `just test-unit` enumerates crates explicitly -- buzz-core, buzz-auth, buzz-voice, buzz-cli, buzz-db, buzz-conformance, buzz-push-gateway, buzz-backend-kubernetes, buzz-agent -- and buzz-acp is not among them. The justfile already says why that list is explicit: "nothing in CI runs `cargo test --workspace` -- workspace membership alone buys clippy/check, not a single executed test." buzz-acp gets exactly that, clippy and check, which type-check test code without running it. The windows-rust job compiles the crate but its only test steps are buzz-dev-mcp and the Tauri crate. So the harness that dispatches every agent turn had zero executed test coverage, and CONTRIBUTING.md's promise that `just ci` is "the same check that runs in CI" was true but hid this gap. Three edits, mirroring the existing conventions: justfile and scripts/run-tests.sh get buzz-acp --lib added to the unit list. These two lists are required to stay in step -- the justfile comments say so at the buzz-backend-kubernetes entry -- or the non-nextest fallback silently covers less. The tests are infra-free: they spawn a local POSIX shell as a fake agent, no relay, no database, no network. ci.yml gets a Test (buzz-acp) STEP inside the existing windows-rust job, not a new job and not a path filter. That is deliberate. "Windows Rust (x86_64-pc-windows-msvc)" is already a required context on a ruleset with zero bypass actors, so a new required check that is path-filtered would report nothing on unrelated PRs and wait forever, and a new unrequired job would not gate anything. A step inside an already-required job inherits the gate and needs no ruleset edit. It runs in parallel, unlike the buzz-dev-mcp step above it, because these tests do not mutate process-global env. Windows is where this coverage earns its keep: the fixtures exercise process spawning, pipe reads and path handling that no Linux job can reach, which is precisely the class of bug the previous commit fixed. One caveat the reviewer should weigh. The Windows step's duration is unmeasured -- 44s locally on 16 cores, but a windows-latest runner is smaller and must codegen the crate, where the existing steps only check/clippy it. The job budget is 45 minutes. If it proves too expensive, narrow the step to acp::tests and pool::tests rather than dropping Windows coverage, and do not paper over it with continue-on-error: a step that cannot fail is not a gate. Signed-off-by: Michael Feth <michael@jira-flow.com>
Widening the budgets in the previous commit was not enough. Measured over nine full-suite runs afterwards, idle_resets_on_stdout_activity and keepalive_resets_idle_past_deadline still failed on four of them -- a ~44% failure rate for the suite as a whole, which is not a suite anyone can trust. The first run after that commit was green, which is exactly how a flake of this rate presents itself and why one green run is not evidence. Widening further does not work, and it is worth writing down why so the next person does not try it. Both tests need the fixture's inter-line gap to stay under the idle budget and the total activity span to stay over the floor, with the floor above the idle budget or the assertion proves nothing. Linux finishes the 20-tick activity window in ~1s and Windows takes ~2s, so the floor is squeezed from both sides: raising the idle budget forces the floor up with it, and the floor cannot exceed the *Linux* activity span. There is no constant that survives an unbounded host stall, because `sleep` is an external binary under MSYS and each of the 20 ticks pays a process spawn that parallel load can delay arbitrarily. So the fix is to stop treating a stalled host as a failure. Both tests now go through assert_activity_resets_idle, which retries up to four times and only when the run was inconclusive. This does not weaken the assertion, because the two outcomes have different shapes. A genuine failure to reset the idle timer is deterministic: elapsed comes back at approximately the idle budget on every attempt, all four attempts fall short of the floor, and the test fails with every measurement in the panic message. A host stall is intermittent, so a correct implementation clears the floor within a try or two. The retry can only absorb the intermittent case, which is precisely the case that carries no information. The two assertions that are always meaningful stayed outside the retry: the turn must end via IdleTimeout rather than the hard deadline or a lost child, and it must not run away. Those are checked on every attempt, so a regression in either fails on the first one. Each attempt now also shuts its client down rather than dropping it, since the helper spawns up to four fixtures where the test previously spawned one. Signed-off-by: Michael Feth <michael@jira-flow.com>
… clock The three steer hard-deadline fixtures opened with `sleep 1` before emitting the steer response, betting that a second of wall-clock would pass after the read loop wrote the steer request but before the 2s hard deadline fired. Rebasing onto main made that bet start losing: main added ~14 buzz-acp tests, the extra parallel load pushed an MSYS `sleep 1` past two seconds, and the suite began failing about half the time -- 2 of 4 full runs, rotating between steer_success_renews_hard_deadline_and_survives_past_original, acp_steer_injected_renews_hard_deadline_and_survives_past_original, and acp_steer_started_new_turn_acks_success_without_renewing_hard_deadline. Run in isolation each passed 3 of 3, which is the signature of load rather than logic. The panic reported `silence: 2.0013468s`, so the fixture had emitted nothing at all before the deadline -- it was not merely late, it had not started. Widening the sleep only moves the threshold. `sleep` is an external binary under MSYS, so every tick pays a process spawn a loaded host can delay arbitrarily, while the deadline ticks independently of the fixture. So the fixtures now open with `read -r _`, which blocks until the read loop actually writes the steer request. That turns the ordering the test depends on from a timing coincidence into a causal one: the response cannot precede the request, and it cannot be pushed past the deadline by unrelated load, because it is emitted the moment the request lands. This option only exists now because these fixtures run under a real MSYS bash -- WSL's `read` returns empty over a Win32 pipe, which is what the first commit on this branch fixed. The trailing `sleep 3` is deliberately left alone. It only has to land *after* the original 2s deadline, so a host stall pushes it further into the region the assertion wants rather than out of it. Verified 797 passed / 0 failed across 5 consecutive full-suite runs, against 2 failures in the 4 runs measured before this change. Signed-off-by: Michael Feth <michael@jira-flow.com>
The bounded retry added in the previous commit used four attempts, sized against a 797-test suite. Merged with the agent-lifecycle branch the count is 811, and one full-suite run in three still exhausted all four -- so the budget was already marginal and would only get more so as the crate grows. Eight attempts. This does not weaken anything: the two always-meaningful assertions still run on every attempt, and a genuine failure to reset the idle timer is deterministic, so it exhausts the budget and fails no matter how large the budget is. The extra attempts are only ever spent on a host that is stalling, which is the case that carries no information. Signed-off-by: Michael Feth <michael@jira-flow.com>
spawn_steer_capture_script still used spawn_script, so MSYS shell startup sat inside the 800ms idle budget that run_one_steer gives the fixture. Starting a real bash costs a large and load-dependent fraction of 800ms, so on a loaded host the read loop idled out before the fixture had answered at all. This is the same defect the three hard-deadline tests had, and the same fix they already got in this branch -- it simply was not applied to this family. It presented differently: whichever capture-based test happened to be running failed, so it read as four unrelated flakes (native_steer_with_active_run_id, goose_transport_wins, acp_steer_failed_outcome, acp_steer_missing_outcome) rather than one shared cause with one fix. Found by merging this branch with the agent-lifecycle branch, which raises the suite to 811 tests: one of that family failed in most full-suite runs, always a different one. Signed-off-by: Michael Feth <michael@jira-flow.com>
…dline run_one_steer gave the fixture an 800ms idle budget. That value is only how the read loop exits once the fixture has answered -- the ack and the captured request bytes are asserted afterwards, and no assertion reads it. But it was also, accidentally, a deadline the fixture had to beat: an MSYS `read` plus a file write can exceed 800ms on a loaded host, and the loop then gave up before the response arrived. Three seconds. Nothing is weakened: a fixture that never answers is still bounded, by the 10s cap that was already there and is unchanged. This is the last of the family that kept surfacing as native_steer_with_active_run_id_routes_response_to_ack failing roughly one full-suite run in three once the suite reached 811 tests. Signed-off-by: Michael Feth <michael@jira-flow.com>
I first filed this as a bug -- that `CreateProcess` cannot load a batch script,
so an agent configured as an npm `.cmd` shim would fail to spawn with "%1 is
not a valid Win32 application". Measured against a real `.cmd`, that is wrong:
`std::process::Command` recognises `.bat`/`.cmd` and routes them through
`cmd.exe` itself, with the argument escaping hardened for CVE-2024-24576.
Command::new("probe.cmd").arg("hello world").output()
-> SPAWN_OK stdout="GOT=[\"hello world\"]\r\n"
So no production change is warranted, and the hand-rolled `cmd.exe` command
line I had written to "fix" it is not here -- it duplicated std's behaviour
with weaker quoting, which is a regression risk rather than a fix.
What remains is a test, because the assumption is real even though the bug was
not: `normalize_agent_command_identity` already strips `.cmd`/`.bat` when
deriving agent identity, so the harness assumes such a command runs, and
nothing in the suite spawned a batch shim to check. If that ever regresses --
in std, or by someone routing the spawn path differently -- the symptom is
agents that simply never start on Windows, which is expensive to trace back
from. Cheaper to assert it.
It belongs on this branch rather than with the lifecycle fixes it was written
alongside: this is the branch that makes the buzz-acp suite runnable on Windows
and puts it in CI, and this is a Windows spawn assumption the suite did not
cover.
Signed-off-by: Michael Feth <michael@jira-flow.com>
The mid-session `OK` handler called `acknowledge_observer_frame` on every OK it
received, then logged the outcome at `debug!`. `accepted` was read only to
decide whether an AUTH rejection should force a reconnect; for every other
event it was formatted into a log line and otherwise ignored.
Retiring an in-flight frame means "the relay has this event now" -- it removes
the event from the queue `requeue_observer_in_flight` resends after a NOTICE or
a reconnect. So a rejected publish was discarded on exactly the same path as a
successful one. The event was gone, and the only trace was a `debug!` line
carrying `accepted=false` in the middle of an otherwise ordinary message.
`OK` frames are now classified before anything is retired. NIP-01 requires a
rejection message to start with one of a fixed set of machine-readable
prefixes, which makes this decidable rather than a guess:
* `rate-limited:` / `error:` -- transient. Leave the frame in flight so the
next requeue resends it, and say so at `warn!`.
* `invalid:` / `blocked:` / `restricted:` / `pow:` -- properties of the event
or of our standing with the relay. Resending the same bytes earns the same
refusal, so retire the frame, but at `warn!`, because an event is being
dropped and that should not be invisible.
* `duplicate:` -- a rejection that means the publish already succeeded.
Treated as stored. This is the ordinary outcome of a resend after
reconnect, so reporting it as a failure would raise a false alarm on the
very path the resend queue exists to serve.
* anything unrecognized -- kept queued. Holding an event costs one bounded
slot; dropping it loses it permanently, and unfamiliar prefixes come from
relays we do not control.
The AUTH branch is untouched and still runs first, so `auth-required:` keeps
triggering a reconnect rather than falling into the classifier.
Signed-off-by: Michael Feth <michael@jira-flow.com>
…lout Three issues surfaced while making this suite runnable and green on Windows for the first time (it was previously blocked entirely by the cwd bug below, so none of this had been exercised): 1. Every `session/new` call across regressions.rs, fake_llm.rs, golden_transcripts.rs, and databricks_oauth.rs hardcoded `cwd: "/tmp"`. `Path::is_absolute()` correctly rejects that on Windows (no drive prefix), so every test using it failed with "cwd must be an absolute path" before reaching the behavior under test. Replaced with a small `test_cwd()` helper backed by `std::env::temp_dir()`, which is absolute on every platform. 2. With that unblocked, flipping the reply-guard default to on broke ~21 pre-existing regression tests that assert exact LLM-call counts or transcripts and never anticipated a nag turn. Each of the four independent test harnesses (this suite has no shared test module — every `tests/*.rs` file is its own binary) now pins `BUZZ_AGENT_REQUIRE_REPLY=0` as part of its base spawn env, so tests that don't care about the guard keep their pre-flip behavior; an explicit override in a test's own env list still wins. `reply_guard_on_by_default` — the one test that must observe the real unset default — goes through a new `spawn_with_true_env_defaults` that skips the pin entirely. 3. `reply_guard_on_by_default` itself was wrong even in isolation: it supplied only 2 canned LLM responses, but this harness registers no publish-capable tool, so every text-only reply reads as "silent" to the guard and it nags twice (MAX_REPLY_NAGS) before giving up — 3 LLM calls, not 2. The 3rd call hit the fake server's empty queue and produced a JSON-RPC error, not `end_turn`. Added a 3rd canned response. Verified individually per binary after the fix: regressions 52/52, fake_llm 20/20, golden_transcripts 16/16, databricks_oauth 17/18 (the one remaining failure reproduces identically on unmodified `main` and is unrelated to this PR). cargo fmt and `cargo clippy -p buzz-agent --tests -- -D warnings` clean. Signed-off-by: Michael Feth <michael@jira-flow.com>
Two surfaces answered "which agents exist" with two hand-rolled identity keys, and they had drifted. @-mention autocomplete keys agents by pubkey (block#5202). The Agents library grouped by `personaId` and then rendered ONE card per group via `pickProfileAgent`, so every instance past the first had no card at all. That is invisible while a persona's instances all share a name. It stops being invisible the moment an agent is renamed but keeps its builtin persona id — the owner's `managed-agents.json` has `builtin:fizz` holding two "Claude" and two "Fizz" instances, and `builtin:honey` holding two "Cody" and two "Honey". The library rendered exactly one card for each of those personas, labelled with the persona name and wired to whichever instance `pickProfileAgent` returned. Two of eleven agents were invisible and unmanageable. One identity definition, shared ------------------------------- `agents/lib/agentIdentity.ts` now owns the doctrine that used to live as a comment inside `agentAutocompleteEligibility.ts`: agentIdentityKey() pubkey — THE identity, used by autocomplete coalescing and by the library agentDisplayGroupKey() persona + folded name — presentation only: which agents may share ONE card. Never a substitute for identity; a display group keeps every member identity and callers must keep them all reachable. Autocomplete now imports `agentIdentityKey` instead of re-deriving it, so the two surfaces cannot answer this question differently again. Why not one card per instance ----------------------------- Exploding to a card per pubkey would have produced 18 agent cards from 11 agents, and it would have reverted a deliberate product decision: same- named instances of one persona already collapse onto the persona's card and stay reachable through that card's profile panel (pinned by the e2e "duplicate instances move from the agents gallery into the agent profile"). The autocomplete argument for never collapsing does not transfer — there, collapsing makes a pubkey unmentionable; here, the card lists every instance behind it. So the collapse stays, bounded by one rule: a card may only stand for instances whose label it truthfully shows. All-same-name persona group → one card, labelled with the persona name, exactly as before. Once the owner has renamed an instance, the persona name can no longer stand for all of them, so each surviving name gets its own card. The owner's data goes from 9 cards hiding 9 agents to 11 cards hiding none. Opening a split card must open that instance, so `pickCanonicalProfileAgent` canonicalises within the requested instance's display group rather than across the whole persona. Same-named instances still collapse onto one profile target; a renamed one opens itself instead of silently redirecting to its persona sibling. Tests ----- `unifiedAgentGroups.ts` had no test file. It has one now, plus `agentIdentity.test.mjs`, covering: a renamed instance gets a card; no identity is dropped by persona grouping; the library and autocomplete agree on the identity set; same-name instances still share one card; persona actions stay on exactly one card per persona. No agent records are merged, renamed, or deleted — deletes propagate cross-device as kind:5 tombstones while the nsec does not, so a merge would strand agents on other machines. Co-authored-by: Michael Feth <michael@jira-flow.com> Signed-off-by: Michael Feth <michael@jira-flow.com>
Follow-up to the previous commit, applying an adversarial review of it. The
first pass made renamed instances visible; these are the consequences it left
undecided.
- A split card now titles itself with the INSTANCE name and carries the persona
name on a second line (`subtitle`, `text-2xs`), so a renamed instance is
disambiguated without the persona becoming unfindable. Unsplit personas pass
`personaLabel: null` and render byte-identically to before.
- Persona-level destructive actions (edit / delete / share) were owned by
`pickProfileAgent`, which is active-first — so the Delete-persona menu moved
between cards when an agent started or stopped. `pickPersonaActionsIndex()`
now picks the card whose folded name matches the persona, else index 0, which
is status-independent.
- The React key was `${persona.id}-${pickProfileAgent(...).pubkey}` and so moved
with runtime status too. It is now `${persona.id}::${foldedName}`, folded
through `foldAgentDisplayName()` in `agentIdentity.ts` rather than lowercased
inline, so the card key and the group key cannot disagree.
- `pickCanonicalProfileAgent` gains the two cases that genuinely lacked
coverage: canonicalising within a display group holding both a stopped and a
running member, and a requested instance absent from the persona list.
Render-layer coverage, which was the review's substantive finding: the previous
commit's unit tests all passed with `UnifiedAgentsSection.tsx` reverted, because
the one-card-per-persona decision lived in JSX where nothing could reach it. The
new `agents.spec.ts` case drives the real gallery with four instances under two
names and asserts two cards, the persona name surviving as a second line, one
actions menu on the correctly-named card, and each card opening its own
instance. Verified as a genuine guard: reverting `unifiedAgentGroups.ts` +
`UnifiedAgentsSection.tsx` to d8281b9 and rebuilding makes it fail.
`profile.spec.ts` was rewritten as a parity test rather than deleted, and
`agent-lifecycle-feedback.spec.ts` updated for the split-card labels.
Verified: unit 4974/4975 (the one failure is a pre-existing timing flake in
`useDocumentVisible.test.mjs`, which this branch does not touch, reproduced on
base by stashing); Playwright smoke 1034 passed / 3 skipped / 0 failed;
`agents.spec.ts` 37/37; `tsc --noEmit`, biome, check:px-text, check-file-sizes
and check-pubkey-truncation all clean.
Co-authored-by: Michael Feth <michael@jira-flow.com>
Signed-off-by: Michael Feth <michael@jira-flow.com>
Two edge cases found by exercising agentIdentity.ts directly with degenerate
input, both of which reintroduce the failure this module exists to prevent.
Unicode: foldAgentDisplayName lowercased without normalizing, so the same name
in NFC and NFD folded to two different keys. macOS input methods and file
systems commonly emit NFD while Windows emits NFC, so one owner naming an agent
on a Mac and on a Windows box produced two cards with visually identical labels
-- a split that cannot be seen, explained, or fixed from the UI. Folding now
normalizes to NFC first.
Separator: agentDisplayGroupKey joined its segments with a literal `|name:`,
but a display name is free text and may contain any separator. Verified before
the fix:
agentDisplayGroupKey({personaId: "a", name: "x|name:y"})
agentDisplayGroupKey({personaId: "a|name:x", name: "y"})
-> both "persona:a|name:x|name:y"
Two different agents sharing one card, one of them no longer openable. Segments
are now length-prefixed, so no name can forge another key. This does not change
the card's React key or data-testid, which derive from persona.id and the folded
name rather than from this key.
Also adds a test asserting that unnamed instances of one persona share a card.
That was already true and stays true -- they remain reachable through the card's
profile panel -- but it was implicit in the fold rather than stated, so a future
change to name handling now has to decide it deliberately.
Red-before-green: with agentIdentity.ts reverted and the tests kept, the two
behavioural tests fail (4 pass / 2 fail); the documentation test passes either
way, which is correct.
Verified: full desktop unit suite exit 0, tsc --noEmit clean, biome clean.
Co-authored-by: Michael Feth <michael@jira-flow.com>
Signed-off-by: Michael Feth <michael@jira-flow.com>
The tip commit length-prefixed the segments of agentDisplayGroupKey because a free-text agent name can forge a delimiter, and its message noted that the card's React key and data-testid were left on the plain `::` join. That reads as an oversight; it is a deliberate asymmetry and now says so. The join is unambiguous because the LEFT segment cannot contain the separator. A persona id is a slugify() output (every non-alphanumeric becomes '-'), a v4 UUID, or a 'builtin:<name>' literal carrying a single colon. Traced all three sources: util.rs:28-43, personas/snapshot/import.rs:559, and the builtin literals. None can emit '::', so a forged separator in the free-text right segment cannot shift the boundary. Length-prefixing this key too would be defensive against an unreachable input and would churn four e2e literals plus a unit pin for it, so the invariant is documented rather than enforced. Co-authored-by: Michael Feth <michael@jira-flow.com> Signed-off-by: Michael Feth <michael@jira-flow.com>
The bug the parent commits fix was a drift, not a typo: @-mention autocomplete and the Agents library each hand-rolled a key for "same agent?", block#5202 moved one of them to the pubkey, and nobody moved the other. Renaming an instance then made it disappear from the library entirely. Nothing failed loudly -- a card simply stopped existing. Convention alone does not stop that recurring, so this encodes the invariant: an agent identity or display-group key is minted in exactly one module, src/features/agents/lib/agentIdentity.ts, and everywhere else imports it. The guard flags a string or template literal that begins an identity namespace -- `pubkey:` or `persona:` -- anywhere outside that module. Those are the wire formats agentIdentityKey and agentDisplayGroupKey produce, so a literal starting with one is either a second implementation or one edit away from becoming one. Verified it catches the real thing, not just a synthetic case: restoring the pre-block#5202 `persona:${candidate.personaId}` into agentAutocompleteEligibility.ts fails the check and names that file and line. Restoring the fix passes. Deliberately narrow, because a guard that cries wolf gets disabled: - It does not try to catch every way of grouping agents, only the shape that caused the outage. - Comment lines are skipped. Unrelated subsystems document their own scope keys (channel storage uses "pubkey:normalizedRelayUrl"), and flagging prose teaches people to silence the guard rather than read it. - Four genuine non-identity uses are allowlisted by `path:literal` with a reason each: the persona catalog dialog's selection token, and the profile panel's render key for a persona that has no agent instance yet. Both are adjacent to the real thing, so each entry says what would make it a violation. Wired into `pnpm check` alongside check-file-sizes, check-px-text and check-pubkey-truncation, matching their structure (single-purpose script, `path:matchedLiteral` allowlist, failure message that names the fix). Verified: pnpm check exits 0 with the new guard in the chain; biome clean; package.json parses. Stacks on the agent-identity fix -- the invariant has no canonical module to point at without it. Co-authored-by: Michael Feth <michael@jira-flow.com> Signed-off-by: Michael Feth <michael@jira-flow.com>
…listing them Review follow-up on PR block#6077, and the reviewer is right. The guard's allowlist was keyed on `path:matchedLiteral` where the literal was only the namespace prefix, so one entry exempted EVERY occurrence of that prefix in the file. The four exempted files were the agent-adjacent ones, so the guard was theatre over precisely the code most likely to drift. Reproduced before fixing: adding a second, unrelated `persona:${a.personaId}|${a.name}` to an allowlisted file passed at exit 0. The fix is not a tighter allowlist but removing the need for one. The two legitimate non-identity uses get namespaces of their own: - `catalog-persona:` for the persona catalog dialog's radio-group selection token, now a named constant. It addresses a row in that dialog's own list, is produced and consumed only in that file, and is never persisted. - `profile:` for the profile panel's render key, which exists precisely when there is NO agent to identify (an uninstantiated persona has no pubkey), so looking like an agent identity was the wrong signal to send. The allowlist is now empty, and documented as worth keeping that way. The match also widened from the bare prefix to the prefix plus the first interpolation or word after it, so if an entry ever does become unavoidable it scopes to one literal rather than the whole file. Both attack cases verified after the change: the second key in a formerly-exempt file is now caught, and reintroducing the historical pre-block#5202 `persona:${candidate.personaId}` into agentAutocompleteEligibility.ts still fails with the file and line named. One thing worth noting for reviewers: `check-file-sizes` caught a 2-line comment I had added to UserProfilePanel.tsx, which sits exactly on the 1000-line ceiling (1000 -> 1002). The comment was redundant with the doc on `profilePanelTargetKey` and is gone; the file is unchanged in length. Verified: pnpm check exits 0 with all four guards; desktop unit suite 4978/4978; tsc --noEmit clean. Co-authored-by: Michael Feth <michael@jira-flow.com> Signed-off-by: Michael Feth <michael@jira-flow.com>
… longer has The header still told the reader that non-identity uses "are allowlisted below with a reason each" while `overrides` is `new Set([])` — the tip commit removed every entry by giving those uses their own namespaces instead. That stale paragraph is the first thing anyone re-reading this file sees, and it describes precisely the design the review rejected. Header now states the actual design: no exceptions, and a namespace of its own is the preferred route if one is ever needed. Co-authored-by: Michael Feth <michael@jira-flow.com> Signed-off-by: Michael Feth <michael@jira-flow.com>
`normalizePubkey` is `trim().toLowerCase()`. Across `features/agents`, 20 call sites reached for a bare `.toLowerCase()` instead. The two agree for every well-formed pubkey and disagree the moment one carries surrounding whitespace, so the divergence is invisible until a value arrives from somewhere untidy — a pasted allowlist entry, a relay tag, a config file — and then one surface stops matching another with nothing thrown and nothing logged. That is the same failure shape the agent-identity work keeps turning up: two places answer "is this the same agent?" differently, and a user reports that an agent "isn't there". One site was already asymmetric rather than merely inconsistent. `RespondToField.handleRemove` compared `p.toLowerCase()` against a pubkey that had been normalized, so removing an allowlist entry could fail to match the entry it was given. That is a live bug, not a style point. ## Guard `check-pubkey-normalization.mjs` matches any `.toLowerCase()` call and then filters on the receiver, rather than anchoring the pubkey inside the pattern. The first version did anchor it, and could not see `agent?.pubkey.toLowerCase()` — the optional link sits between the identifier and the segment being matched. Filtering on the receiver found three optional-chained sites the initial sweep had skipped, two of which the anchored pattern could never have reported. Scoped to `src/features/agents` deliberately. Hand-rolled lowercasing is repo-wide — 87 files under `desktop/src` against 113 using `normalizePubkey` — and failing on all of them would make the guard unshippable. A guard that must be disabled to land anything protects nothing. So it covers the surface whose divergence caused an outage, and covers it completely: no allowlist. Widening it is a follow-up that has to arrive with the call-site fixes, not a flag flip. ## Interaction with block#6077 Both this PR and block#6077 add a `check:*` script and extend the same `check` line in `desktop/package.json`, so whichever lands second needs a one-line rebase there. Nothing else overlaps: block#6077 guards how an identity KEY is minted, this guards how the pubkey inside it is normalized. Note that `main` recently removed `pnpm check:file-sizes` from that line (block#6187 made the ratchet a first-class root gate). Neither PR should put it back. ## Verification `npx tsc --noEmit` clean. `pnpm check` clean. Desktop unit suite: 4993 passed, 0 failed. Guard proven to fail, not just to pass: an injected `x.pubkey.toLowerCase()` under `features/agents` is reported and exits 1; removing it exits 0. Signed-off-by: Michael Feth <michael@jira-flow.com>
… on Windows
`_ensure-sidecar-stubs` creates placeholder files named `<name>-<target>`, but
Tauri resolves an `externalBin` as `<name>-<target><exe-suffix>`. On a Windows
host it therefore validates `<name>-<target>.exe`, finds nothing, and the build
script panics on the first sidecar:
thread 'main' panicked at build.rs:143:6:
failed to build Tauri application: resource path
`binaries\buzz-acp-x86_64-pc-windows-msvc.exe` doesn't exist
Every recipe that depends on the stubs — `desktop-tauri-check`,
`desktop-tauri-clippy`, `desktop-tauri-test`, and so `just check` and
`desktop-ci` — is unrunnable on Windows as a result. There is no workaround
short of creating the files by hand, which is what I have been doing.
`desktop-release-build` had the same defect independently, for a target it
explicitly anticipates (it already branches on `*windows*`).
## Also: the two copies had drifted
Both recipes carried their own copy of the sidecar list, each with a comment
telling the reader to keep it in sync with the other. They were not in sync —
the release recipe listed the binaries one `touch` per line while
`_ensure-sidecar-stubs` had moved to an array, and only one of them would have
picked up a future addition.
So `_ensure-sidecar-stubs` now takes an optional target triple, defaulting to
the host, and `desktop-release-build` depends on it instead of repeating it.
One definition, and the Windows suffix is handled once.
The `buzz-backend-kubernetes` exclusion on Windows is preserved — I checked, and
it is deliberate: `tauri.windows.conf.json` overrides `externalBin` to drop that
sidecar, so a stub for it is genuinely not wanted there. That is now stated in a
comment rather than left as an unexplained conditional.
## Verification
On `x86_64-pc-windows-msvc`, against a wiped `desktop/src-tauri/binaries`:
- Reproduced first. Stubs created the old way (no suffix) plus
`touch desktop/src-tauri/build.rs` to defeat the build-script cache →
`cargo check --manifest-path desktop/src-tauri/Cargo.toml` exits **101** with
the panic above. The cache matters: without touching `build.rs` the check
passes on stale state and the bug looks fixed when it is not.
- With the fix, `just _ensure-sidecar-stubs` then the same forced check exits
**0**.
- `just _ensure-sidecar-stubs` → 5 stubs, all `.exe`.
- `just _ensure-sidecar-stubs x86_64-pc-windows-msvc` → same 5, all `.exe`.
- `just _ensure-sidecar-stubs x86_64-unknown-linux-gnu` → 6 stubs, no suffix,
`buzz-backend-kubernetes` included.
- `just --dry-run desktop-tauri-check` still resolves the dependency with no
argument; `just --dry-run desktop-release-build x86_64-pc-windows-msvc` shows
the target threaded through.
Non-Windows behaviour is unchanged: `EXE_SUFFIX` is empty for every other
target, so the emitted paths are byte-identical to before.
Signed-off-by: Michael Feth <michael@jira-flow.com>
block#2884 landed on main and added three `agent_supports_mode` tests to `pool.rs`'s `mod tests`, at the same point this branch appends its mention-ack tests. The diff3 base for the region is empty -- both sides purely added -- so both are kept. The one subtlety is the brace. The `}` on the line after the conflict closes whichever side's final test function survives, so concatenating the two blocks would have left this branch's last test unterminated and nested main's three tests inside it. Compiles either way; the tests would just have stopped being separate tests. An explicit `}` between the blocks keeps all seven as siblings. Verified in this worktree on x86_64-pc-windows-msvc: - `cargo check -p buzz-acp --all-targets`: clean. - `cargo test -p buzz-acp --lib mention_ack`: 4 passed. - `cargo test -p buzz-acp --lib agent_supports_mode`: 3 passed. - `cargo fmt --check -p buzz-acp`: clean. The full buzz-acp suite is not cited here: on Windows it fails a varying set of 21-22 tests run-to-run from an unchanged tree, so its count is not attributable to any branch. The seven tests either side of this conflict are run directly instead. Signed-off-by: Michael Feth <michael@jira-flow.com>
Signed-off-by: Michael Feth <mfethe1@gmail.com>
Signed-off-by: Michael Feth <mfethe1@gmail.com>
Signed-off-by: Michael Feth <mfethe1@gmail.com>
Signed-off-by: Michael Feth <mfethe1@gmail.com>
Signed-off-by: Michael Feth <mfethe1@gmail.com>
Signed-off-by: Michael Feth <mfethe1@gmail.com>
Signed-off-by: Michael Feth <mfethe1@gmail.com>
Signed-off-by: Michael Feth <mfethe1@gmail.com>
Signed-off-by: Michael Feth <mfethe1@gmail.com>
Signed-off-by: Michael Feth <mfethe1@gmail.com>
Signed-off-by: Michael Feth <mfethe1@gmail.com>
Signed-off-by: Michael Feth <mfethe1@gmail.com>
Signed-off-by: Michael Feth <mfethe1@gmail.com>
Signed-off-by: Michael Feth <mfethe1@gmail.com>
Signed-off-by: Michael Feth <mfethe1@gmail.com>
…41 absorb collision Upstream PR block#6994 (wpfleger96) merged 2026-08-31T23:40:36Z carrying both migrations/0041_nip_fi_identity_foundation.sql and migrations/0042_nip_fi_authorization_foundation.sql. Our fork holds migrations/0041_task_system.sql (placed by HW-016), recreating the duplicate-0041 prefix collision in the prospective merged tree — the third recurrence of this defect class (0033->0035, 0035->0041, 0041->0043). git mv migrations/0041_task_system.sql -> migrations/0043_task_system.sql (100% similarity, 0 content lines). Three references updated: - crates/buzz-db/src/migration.rs:1218 assert_eq!(migrations[34].version, 41) -> 43 - crates/buzz-db/src/task.rs:145 doc comment - crates/buzz-relay/src/api/tasks.rs:593 comment Zero SQL bytes edited (md5-proven). migrations.len() stays 35 (renumber, not addition). Fork-only; task system absent from upstream. Signed-off-by: Michael Feth <michael@jira-flow.com>
…ring duplicate-0041 absorb collision) Upstream PR block#6994 (wpfleger96, NIP-FI identity + authorization foundation) merged 2026-08-31T23:40:36Z carrying 0041_nip_fi_identity_foundation.sql and 0042_nip_fi_authorization_foundation.sql. Fork's 0041_task_system.sql (placed by HW-016) recreates the duplicate-0041 prefix collision in the prospective merged tree — third recurrence (0033->0035, 0035->0041, 0041->0043). Zero SQL bytes edited (sha256-verified content identity). 4 files, +3/-3. Gates: buzz-db 116/0, buzz-core 269/0, buzz-acp 876/0, buzz-relay 948/0 (telemetry flake on first run, green on re-run — not in known_baseline_failures but consistent with documented relay telemetry flake history). Merged-tree dup-prefix scan EMPTY (collision cleared). Absorb probe: conflict set byte-identical (13 conflicts both sides). No desktop/mobile paths (scope gates empty). Fork-only, no upstream PR (task_system absent upstream). Signed-off-by: Michael Feth <mfethe1@gmail.com>
Owner
Author
|
Baseline probe complete: product/main @ e788bd2 fails its own CI (file-size ratchet vs main, Desktop, Desktop E2E Integration). Evidence captured. Closing probe. |
mfethe1
pushed a commit
that referenced
this pull request
Sep 19, 2026
Implements the accounting half of the loop-protection policy decided in wayfinder ticket #7 (mfethe1/agent-mesh#7). Buzz already has throttles — thread depth capped at 100, tiered rate limits, in-flight deadlines, session rotation — but nothing that says an exchange is finished. Two agents that open fresh threads at each other never accumulate depth, so they run at a rate-limited 120 messages a minute indefinitely, each one a paid LLM call. This crate budgets the thing that actually matters: measured cost_usd from the kind 44200 turn metric, $5 per (channel, agent-pair) per rolling hour. The window slides, so it self-heals with no human reset. Human-triggered turns are never charged — cutting off someone who is sitting there watching is the failure the policy exists to avoid. Deliberately does not enforce. record() returns a Verdict and the caller decides. Buzz's owner commands (!shutdown, !cancel, !rotate) cannot mute a single peer, so the enforcement mechanism is still open; keeping the accounting pure lets that decision land without touching this code. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013okf5qYe8U8zja6wPBAL2y
mfethe1
pushed a commit
that referenced
this pull request
Sep 19, 2026
Decision D5 of wayfinder #7 says only agent-triggered turns consume budget. Implementing it surfaced a gap: the kind 44200 turn metric has no field naming the author that caused the turn. Its payload is harness, model, channel_id, session_id, turn_id, turn_seq, timestamp, turn, cumulative, delta_reliable and stop_reason — nothing about the trigger. TriggerLog recovers it without a wire-format change, using a property buzz-acp already guarantees: turns are serialised per channel, and all pending events for a channel drain into one batch. So the messages seen in a channel since its last turn are that turn's trigger. A batch mixing human and agent messages counts as human. D5 exists to protect the case where a person is present and watching; charging that turn risks muting an agent mid-conversation with its owner. Erring the other way costs a runaway one extra turn before it trips. Attribution is best-effort and fails open: an observation gap yields None, which is not charged, so a dropped subscription disables the budget rather than tripping it. The durable fix is a trigger field on NIP-AM, which the spec's forward-compatibility rule already permits. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013okf5qYe8U8zja6wPBAL2y
mfethe1
pushed a commit
that referenced
this pull request
Sep 19, 2026
The crate could account and attribute, but nothing connected it to real data. charge_from_metric decrypts a kind:44200 event with the owner's keys and extracts channel, agent, cost and end-of-turn timestamp. Kept in its own module because it is the only part of the crate that knows about Nostr. Ledger and TriggerLog stay testable on plain values with no keys and no events, which is why they have the coverage they do. Tests build real signed events with buzz-core's own encrypt helper and round-trip them, so this is checked against Buzz's actual wire format rather than a hand-rolled fixture. A stranger's key fails closed, an unrelated kind is rejected before any decrypt attempt, and missing or malformed channel ids and timestamps are errors rather than silent defaults. Notable while writing it: decrypt_agent_turn_metric already rejects negative and non-finite costUsd per NIP-AM, so the ledger's own guard against a refund is defence in depth rather than the only barrier. delta_reliable is surfaced on TurnCharge rather than swallowed — #7 OQ7.1 asks how often it is false, and that cannot be answered if the ingest layer discards it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013okf5qYe8U8zja6wPBAL2y
mfethe1
pushed a commit
that referenced
this pull request
Sep 19, 2026
Two real bugs, one security and one correctness, plus the documented standards this crate was skipping. Security: charge_from_metric took turn_end from the agent's own encrypted payload and never checked it. AgentTurnMetricPayload::validate covers only the numerics, so a metric dated in the future moved the ledger's eviction cutoff forward and wiped the pair's whole window — the budgeted agent could zero its own budget at will. The payload timestamp is now tied to the signed created_at within a 300s tolerance. Correctness: TriggerLog consumed the batch per channel rather than per agent, so with two agents in one channel whichever metric was processed first ate the other's trigger and the second turn went uncharged. Relay reordering alone disabled the budget. Consumption is now tracked per (channel, agent) with independent cursors. The spec axis found a third defect that per-agent cursors do NOT fix: the metric's timestamp is end-of-turn and the payload carries no start, so a message arriving mid-turn is attributed to the turn it did not trigger, and one owner message can free two turns. It errs toward under-charging like every other approximation here, and the durable fix is a turn-start or trigger field on NIP-AM. Documented rather than papered over — it is a second argument for #7 OQ7.5. Both reviewers independently flagged Verdict::Allow{spent_usd: 0.0} on human and unattributed turns as a lie: the pair may hold $4.90, and a caller logging spend would see a sawtooth already collapsed to zero. Replaced with an explicit Unbudgeted variant. Standards: adds deny(unsafe_code) and warn(missing_docs) per CONTRIBUTING.md, documents the public API those lints then surfaced, and registers the crate in the AGENTS.md crate map. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013okf5qYe8U8zja6wPBAL2y
mfethe1
added a commit
that referenced
this pull request
Sep 20, 2026
* feat(budget): sliding-window cost accounting for agent exchanges Implements the accounting half of the loop-protection policy decided in wayfinder ticket #7 (mfethe1/agent-mesh#7). Buzz already has throttles — thread depth capped at 100, tiered rate limits, in-flight deadlines, session rotation — but nothing that says an exchange is finished. Two agents that open fresh threads at each other never accumulate depth, so they run at a rate-limited 120 messages a minute indefinitely, each one a paid LLM call. This crate budgets the thing that actually matters: measured cost_usd from the kind 44200 turn metric, $5 per (channel, agent-pair) per rolling hour. The window slides, so it self-heals with no human reset. Human-triggered turns are never charged — cutting off someone who is sitting there watching is the failure the policy exists to avoid. Deliberately does not enforce. record() returns a Verdict and the caller decides. Buzz's owner commands (!shutdown, !cancel, !rotate) cannot mute a single peer, so the enforcement mechanism is still open; keeping the accounting pure lets that decision land without touching this code. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013okf5qYe8U8zja6wPBAL2y * feat(budget): attribute a turn to whatever triggered it Decision D5 of wayfinder #7 says only agent-triggered turns consume budget. Implementing it surfaced a gap: the kind 44200 turn metric has no field naming the author that caused the turn. Its payload is harness, model, channel_id, session_id, turn_id, turn_seq, timestamp, turn, cumulative, delta_reliable and stop_reason — nothing about the trigger. TriggerLog recovers it without a wire-format change, using a property buzz-acp already guarantees: turns are serialised per channel, and all pending events for a channel drain into one batch. So the messages seen in a channel since its last turn are that turn's trigger. A batch mixing human and agent messages counts as human. D5 exists to protect the case where a person is present and watching; charging that turn risks muting an agent mid-conversation with its owner. Erring the other way costs a runaway one extra turn before it trips. Attribution is best-effort and fails open: an observation gap yields None, which is not charged, so a dropped subscription disables the budget rather than tripping it. The durable fix is a trigger field on NIP-AM, which the spec's forward-compatibility rule already permits. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013okf5qYe8U8zja6wPBAL2y * feat(budget): compose attribution and accounting into a Supervisor Ledger charges a pair; TriggerLog attributes a turn. Held separately, a caller has to remember to attribute before charging and to prune both on the same schedule. Supervisor owns that sequencing so it cannot be got wrong, and gives the crate one entry point instead of two loose halves. Adds the integration coverage the pieces lacked. The end-to-end runaway now has a test: two agents alternating at $0.30 a turn trip the $5 budget on the seventeenth charged turn. So does the failure this policy must never cause — two hundred consecutive owner-driven turns at $1.50 each, none blocked, nothing charged. Also covers a human joining a runaway mid-flight, which makes that turn free because the mixed batch attributes to the human; separate pairs in one channel not pooling budget; and an unobserved turn failing open. Writing the runaway test caught a real arithmetic error in its own expectation, not in the code: $5 at $0.30 a turn first exceeds budget on the seventeenth charge, not the eighteenth. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013okf5qYe8U8zja6wPBAL2y * feat(budget): ingest kind:44200 turn metrics from the wire The crate could account and attribute, but nothing connected it to real data. charge_from_metric decrypts a kind:44200 event with the owner's keys and extracts channel, agent, cost and end-of-turn timestamp. Kept in its own module because it is the only part of the crate that knows about Nostr. Ledger and TriggerLog stay testable on plain values with no keys and no events, which is why they have the coverage they do. Tests build real signed events with buzz-core's own encrypt helper and round-trip them, so this is checked against Buzz's actual wire format rather than a hand-rolled fixture. A stranger's key fails closed, an unrelated kind is rejected before any decrypt attempt, and missing or malformed channel ids and timestamps are errors rather than silent defaults. Notable while writing it: decrypt_agent_turn_metric already rejects negative and non-finite costUsd per NIP-AM, so the ledger's own guard against a refund is defence in depth rather than the only barrier. delta_reliable is surfaced on TurnCharge rather than swallowed — #7 OQ7.1 asks how often it is false, and that cannot be answered if the ingest layer discards it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013okf5qYe8U8zja6wPBAL2y * fix(budget): address both axes of code review Two real bugs, one security and one correctness, plus the documented standards this crate was skipping. Security: charge_from_metric took turn_end from the agent's own encrypted payload and never checked it. AgentTurnMetricPayload::validate covers only the numerics, so a metric dated in the future moved the ledger's eviction cutoff forward and wiped the pair's whole window — the budgeted agent could zero its own budget at will. The payload timestamp is now tied to the signed created_at within a 300s tolerance. Correctness: TriggerLog consumed the batch per channel rather than per agent, so with two agents in one channel whichever metric was processed first ate the other's trigger and the second turn went uncharged. Relay reordering alone disabled the budget. Consumption is now tracked per (channel, agent) with independent cursors. The spec axis found a third defect that per-agent cursors do NOT fix: the metric's timestamp is end-of-turn and the payload carries no start, so a message arriving mid-turn is attributed to the turn it did not trigger, and one owner message can free two turns. It errs toward under-charging like every other approximation here, and the durable fix is a turn-start or trigger field on NIP-AM. Documented rather than papered over — it is a second argument for #7 OQ7.5. Both reviewers independently flagged Verdict::Allow{spent_usd: 0.0} on human and unattributed turns as a lie: the pair may hold $4.90, and a caller logging spend would see a sawtooth already collapsed to zero. Replaced with an explicit Unbudgeted variant. Standards: adds deny(unsafe_code) and warn(missing_docs) per CONTRIBUTING.md, documents the public API those lints then surfaced, and registers the crate in the AGENTS.md crate map. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013okf5qYe8U8zja6wPBAL2y * docs(budget): correct the end-of-turn citation to line 111 The spec-review agent cited agent_turn_metric.rs:107 for the end-of-turn timestamp and I carried that into origin.rs without opening the file. Line 107 is turn_seq documentation; the RFC 3339 end-of-turn line is 111. Every other cross-crate citation in the crate was re-checked by opening the cited line: buzz-acp/src/lib.rs:192 (is_owner_or_sibling), buzz-acp/src/queue.rs:1-7 (per-channel serialisation), agent_turn_metric.rs:86 (forward-compatibility rule) and :1-5 (NIP-44 addressing). All four hold. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013okf5qYe8U8zja6wPBAL2y * feat(budget): emit D4's sawtooth and inspect delta_reliable Two gaps the spec review found, both now closed. D4 justifies a self-healing window by the sawtooth it leaves in the logs — "a diagnosable signature rather than a silent drain" — but the crate had no tracing dependency and emitted nothing, so the signature the decision rests on did not exist. Exhaustion now logs at warn with the pair, spend and budget; ordinary charges log at debug. delta_reliable was populated by the ingest layer and read by nobody, so D2 charged unreliable deltas at face value with no seam for the fallback OQ7.1 asks about. on_turn_charge takes a wire-decoded TurnCharge, warns when the publisher lost its cumulative baseline, and still charges — because OQ7.1 is genuinely undecided and its suggested fallback needs cumulative, which TurnCharge does not yet carry. Explicit and logged beats silent, and it lets the question be answered from real data. Both behaviours are tested rather than left accidental. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013okf5qYe8U8zja6wPBAL2y * test(budget): drive the budget at runtime, and find D4's sawtooth missing The verification gate is right that tests are not runtime evidence. This example launches the supervisor with a real tracing subscriber and shows what an operator would actually see. Two of D4's claims hold under observation. Forty consecutive owner-driven turns at $2.50 each: zero blocked, zero charged. After the window slides, spend resets and the verdict returns to Allow with no reset step. The third does not. D4 justifies the design by predicting "a runaway stops within minutes and restarts only to stop again, producing a sawtooth in the logs — a diagnosable signature rather than a silent drain." Running it produces no sawtooth: spend climbs monotonically from $5.40 to $18.00 across 22 consecutive WARN lines, because nothing acts on the verdict. The sawtooth is a property of enforcement, not of accounting. Until an enforcer exists the signature is a continuous alarm, which is arguably worse for diagnosis than the silent drain it was meant to replace. That is a flaw in the decision's reasoning rather than in this code, and it is recorded on the ticket. spent_usd is logged raw rather than rounded. The float noise is real data and these are structured events meant for machine aggregation; rounding at the emit site would lose precision for no gain. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013okf5qYe8U8zja6wPBAL2y * refactor(budget): drop PairKey::members, which nothing called The spec-review agent flagged it as unrequested; spot-checking that claim found exactly one caller, and it was the test asserting on it. A public accessor whose only consumer is its own test is not API, it is speculative generality. The test it existed for still matters — both argument orders must reach one budget — so it now asserts that directly by hashing both keys into a set, which is closer to how PairKey is actually used. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013okf5qYe8U8zja6wPBAL2y * [lenny] chore: trigger CI after un-drafting PR #1 * [lenny] fix(budget): add pricing_identity to test payload after trunk field addition * [lenny] fix(budget): disambiguate nostr in Cargo.lock for --locked release build --------- Co-authored-by: Michael Feth <michael@jira-flow.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
CI baseline probe for product/main @ e788bd2 (plus one empty commit). Do not merge; will be closed after evidence capture.