auto-fix batch claude/friendly-maxwell-bVlpW (2026-04-28)#484
Merged
Conversation
Mirror deploy.yml pin. Eliminates supply-chain drift from unpinned `cargo install trunk`. Refs #475 Co-authored-by: Claude <noreply@anthropic.com>
…479) Replaces 16 trailing `.ok();` swallows on broker `do_send`, topic `broadcast`, and persistence `do_send` calls in `crates/client/src/listeners.rs` with a private `warn_if_err(Result<T, E>, &'static str)` helper that logs `tracing::warn!(?e, "{context}")` on `Err` and drops the success value. Each call site gets a distinct diagnostic context string. The single remaining `.ok()` (GrantPermission branch) is intentional — it coerces `Result<Event, _>` to `Option<Event>` for `if let Some(event) = ...`, not a swallowed fire-and-forget send. Refs #253
…480) PR #443 closed #350 by routing client-mutation errors in handlers.rs through warn_and_toast / warn_and_toast_with so the user sees a toast instead of the action silently vanishing. The pattern was not propagated to component-internal handlers — 11 sites across roles.rs, settings.rs, sync_queue_view.rs, and channel_sidebar.rs still discarded the anyhow::Result with `let _ = h.foo(...).await`. Migrate all 11 sites: - roles.rs: create role, set permission, assign role, delete role - settings.rs: set server display name, mute server - sync_queue_view.rs: retry queue - channel_sidebar.rs: create channel, create voice channel, delete channel, mute channel Each site now captures `let toasts = use_context::<ToastStack>()` on the outer reactive frame (before `wasm_bindgen_futures::spawn_local`, which strips the reactive owner), moves it into the async block, and dispatches `crate::handlers::warn_and_toast_with("<action>", &e, toasts.as_ref())` on Err. `rg -n 'let _ = h\.[a-z_]+\(.*\)\.await' crates/web/src/components/` goes from 11 hits to 0. Refs #476
Add 4 inline #[tokio::test]s to crates/actor/src/lib.rs covering behaviors the audit (issue #232) called out as missing: - system_shutdown_terminates_ctx_spawned_child: ctx.spawn children are tracked by the system and stopped on shutdown. - system_shutdown_awaits_in_flight_handler: shutdown blocks until every actor's mailbox loop runs to completion (parent waits for child). Uses a oneshot ready-signal — no sleep-for-propagation. - broker_delivers_to_many_subscribers: fanout to N=5 subscribers, with ask() round-trips on each subscriber as a deterministic FIFO barrier. Also asserts no replay to late subscribers. - broker_slow_subscriber_does_not_block_others: a blocked Handler on one subscriber must not delay delivery to another. Uses tokio::sync::Notify for release coordination. Test count: 90 -> 94 (well above the issue's >10 default-run target). Note on the issue's other prescription — splitting tests/performance.rs into actor.rs (correctness) + performance.rs (timing): every test in the file is a genuine performance/throughput case (assert ops_per_sec > 5_000.0, 10k-iteration loops, propagation-latency thresholds, multi-source benchmarks). There are no correctness-only tests lurking in there to extract; splitting would be churn without benefit. Inline tests in src/ already cover the correctness counterparts (e.g. broker_publish_to_subscribers in broker.rs vs perf_broker_fanout in performance.rs). Refs #232 Co-authored-by: Claude <noreply@anthropic.com>
Fold lessons from auto-fix batch claude/friendly-maxwell-bVlpW into the resolving-issues skill: 1. Sandbox signing-service rejects worktree-path commits (hit by 2 of 7 implementers this run); document the canonical-dir fallback in Setup. 2. New "stale-audit-with-residual-gap" path on the implementer agent — when an audit's literal premise is outdated but a residual concern remains, narrow scope and ship the residual; keep the issue closer under Fixes. 3. Already-fixed-upstream path: clarify completed vs not_planned choice based on whether the audit's intent still holds.
Owner
Author
|
Resolve conflicts and let's also make a change to the skill. We don't necessarily need the work trees anymore because we're working sequentially and opening the sub-PRs seems to be causing some issues. Sometimes the peers wouldn't get merged and it's a little confusing on my end. Let's adjust the workflow to work sequentially, just on the same branch or by checking out a branch and then merging back into that master branch without any PR or anything like that. That way our Git history stays clean while we don't run into any issues with the GitHub side of things. Once that's all done then we just open a PR with the master branch like we're doing now. |
Resolves two conflicts: 1. crates/client/src/lib.rs — both branches added test modules. Kept all three: tests_actions (this batch, #420), tests_voice + tests_governance (PR #473, #414/#416). `cargo check -p willow-client --tests` clean post-resolution. 2. .claude/skills/resolving-issues/SKILL.md — both branches edited the skill. Resolved by full rewrite per human review feedback on PR #484: drop worktrees and sub-PRs entirely, work sequentially on the master branch directly (or via local feature branches that merge back into master without going through GitHub PRs). Skill rewrite captures the new workflow shape: - One master branch per session, all fixes land sequentially. - No sub-PRs (they were causing merge confusion + silent CI skip when sub-PR base ≠ main). - No worktrees (sequential = no isolation needed; also dodged the sandbox signing-service incompatibility documented in the prior skill version). - Implementer commits directly to master branch (Pattern A) or via a local feature branch + `git merge --no-ff` back into master (Pattern B). Master PR opens at end of run with everything in it. Preserved from the prior skill: - Coordinator-never-codes rule (with two narrow exceptions: session-open commit + Lessons Learned skill edits). - Complexity gate (auto-brainstorm + plan). - Already-fixed-upstream + stale-audit-with-residual-gap paths. - Caveman GH comms. - Sequential between issues, fresh agent per issue, max 10 per run.
Owner
Author
|
Conflicts resolved + skill rewritten per your feedback. New tip: Conflicts
Skill rewrite — what changedDrops worktrees + sub-PRs entirely. New shape:
The full master-PR body assembled at end of run still has CI should re-trigger on the merge. Generated by Claude Code |
Owner
Author
|
Looks like conflict still. Please fix |
…gates PR #467 (SEC-V-03 voice cap, issue #303) merged into main while this batch was open. It modified the same Voice* listener arms that #253 migrated to warn_if_err. Both changes are independent and compose: - Keep all SEC-V-03 logic from origin/main (drop unknown-channel VoiceJoin/Leave/Signal, cap participants at MAX_VOICE_CHANNELS / MAX_PARTICIPANTS_PER_CHANNEL). - Wrap the final event_broker.do_send(Publish(...)) calls in warn_if_err per #253. cargo check -p willow-client --tests + cargo test -p willow-client both clean (310 passed).
Owner
Author
|
Resolved. New tip: PR #467 (SEC-V-03 voice cap, issue #303) merged into main while this batch was open and touched the same Voice* listener arms that #253 migrated to Verified: Generated by Claude Code |
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
2 participants
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.
5 small-scope fixes swept from open issue queue. All sub-PRs merged via local cargo gate (sub-PR base ≠ main → CI no run). Master PR runs full CI on open — load-bearing gate.
Fixes
trunk0.21.14 indocker/web.Dockerfile(sub-PR build(docker): pin trunk to 0.21.14 in web image #478). Mirrors.github/workflows/deploy.yml..ok()silently swallows ~16 errors inlisteners.rsevent-pump hot loop #253 —warn_if_errhelper + 16 site migration incrates/client/src/listeners.rs(sub-PR fix(client): log dropped fire-and-forget sends in listeners hot loop #479). One.ok()onResult→Optioncoercion intentionally left alone.warn_and_toast_with(sub-PR fix(web): route component handler errors through warn_and_toast_with #480). Sites inroles.rs,settings.rs,sync_queue_view.rs,channel_sidebar.rs. Post-fixrgcount: 0.actorcrate tests all#[ignore]'d — zero default-CI coverage of actor runtime #232 — 4 inline#[tokio::test]s incrates/actor/src/lib.rscovering shutdown ordering + broker fanout (sub-PR test(actor): cover shutdown ordering and broker fanout #482). Audit's "split perf.rs" plank declined: everytests/performance.rscase is timing-sensitive, no correctness-only cases to extract. Test count 90 → 94.crates/client/src/tests/actions.rscoveringshare_file_inline256 KiB cap,create_voice_channel,set_permissiongrant/revoke,assign_role, pin lifecycle (sub-PR test(client): cover actions.rs translation logic #483). 19 pure pass-throughs documented module-level. Test count 287 → 294.Already-Fixed
MessageDb::insertswallows SQLite errors with.ok();— silent message loss #269 [GEN-09] —MessageDb::insertalready deleted upstream in23219af("refactor(client): delete dead message_db field + MessageDb storage", 2026-04-26). Audit's literal target gone. Implementer commented + closednot_planned. Did NOT include underFixes(closing issue withFixeswould have been a stale link).Parked
getrandomlinked in four versions (0.2 / 0.3 / 0.4 x2) — feature-unification risk for WASM #252 [TD-03]getrandomfour-version unification — Category 3 (architectural). The version split is forced by transitive deps (aes-gcm 0.10pins 0.2;iroh 0.98pins 0.4;ahash/rusqlite/leptospull 0.3). A workspace-level pin can't collapse them — Cargo resolves each major-version range independently. Existinggetrandom_02/getrandom_03per-crate shims inwillow-identity/willow-crypto/willow-messagingare already the correct WASM-safe pattern. Filed follow-up [TD-03 follow-up]getrandom3-way split is structural — driven by aes-gcm 0.10, ahash, and iroh 0.98 #481 documenting the holdout chain; the issue should stay parked untilaes-gcm 0.11ships stable (the most plausible first domino).Skill Evolution
Skill commit on this branch:
3df1b33— three updates folded into.claude/skills/resolving-issues/SKILL.md:## Setup. Some sandboxes run a commit-signing service that rejects sources outside/home/user/<repo>with400 missing source—git commitfrom/home/user/willow-wt-*paths consistently fails. Document the canonical-dir fallback (commit on the same branch from the main checkout). Hit by 2 of 5 implementers this run ([F2] 11 web component handlers swallow client mutation errors (no toast/log) #476, [TC-6] crates/client/src/actions.rs has zero in-file tests #420).stale-audit-with-residual-gappath on the implementer agent. When an audit's literal premise is outdated (e.g. [TEST-04]actorcrate tests all#[ignore]'d — zero default-CI coverage of actor runtime #232's "0 default tests" was true at2f26d91but PR Unbounded actor mailboxes enable OOM DoS across all actors #78 has since added 90) but its underlying concern is partially valid (audit specifically called out shutdown ordering + multi-subscriber broker isolation, neither of which existed pre-this-PR), narrow scope to the residual gap and ship that. Coordinator still records underFixes #N.completedif the audit's intent still holds (the upstream fix solved it for us);not_plannedif the audit's premise is moot (e.g. the targeted code was deleted). [GEN-09]MessageDb::insertswallows SQLite errors with.ok();— silent message loss #269 took the latter path this run.Lessons Learned
git fetch + reset --hard origin/<batch>) held clean across 7 dispatches. Each implementer's worktree had latest prior merges as base.getrandomlinked in four versions (0.2 / 0.3 / 0.4 x2) — feature-unification risk for WASM #252 was framed as "obvious fix — will be auto-PR'd" but a 5-minute brainstorm showed it was structural. Implementer correctly declined to push a half-fix; filed [TD-03 follow-up]getrandom3-way split is structural — driven by aes-gcm 0.10, ahash, and iroh 0.98 #481 instead. The "obvious fix" tag in audit issues is not load-bearing — implementers should still verify before coding.actorcrate tests all#[ignore]'d — zero default-CI coverage of actor runtime #232 (test-count residual gap) and [GEN-09]MessageDb::insertswallows SQLite errors with.ok();— silent message loss #269 (target deleted) were both correctly handled without ballooning scope or shipping dead PRs. Skill change above codifies the patterns.willow-actor::send_dead_actor_returns_send_errorsurfaced for one implementer ([TEST-04]actorcrate tests all#[ignore]'d — zero default-CI coverage of actor runtime #232), passed on retry. Documented in auto-fix batch 2026-04-28 (claude/friendly-maxwell-Oggvw) #473's lessons; still extant. Worth filing a tracking issue if it persists across more runs.getrandom3-way split is structural — driven by aes-gcm 0.10, ahash, and iroh 0.98 #481) worked — keeps the queue durable for the next scheduled run, no in-session continuation needed.Test plan
just check(fmt + clippy + test + wasm + browser).just dev+ agent-browser navigation through web UI to confirm component error toasts surface (e.g. trigger a channel-create permission failure, verify the toast appears with the new label).Generated by Claude Code