Skip to content

auto-fix batch claude/friendly-maxwell-iEXwu 2026-05-04#599

Merged
intendednull merged 7 commits into
mainfrom
claude/friendly-maxwell-iEXwu
May 5, 2026
Merged

auto-fix batch claude/friendly-maxwell-iEXwu 2026-05-04#599
intendednull merged 7 commits into
mainfrom
claude/friendly-maxwell-iEXwu

Conversation

@intendednull
Copy link
Copy Markdown
Owner

@intendednull intendednull commented May 4, 2026

Scheduled /resolving-issues sweep against the 2026-05-03 general-audit (master ticket #567). Four small-scope fixes landed sequentially. Picks chosen to NOT overlap with PR #566 + PR #598's in-flight files (#566 still locks crates/{client,web,messaging,state,storage,worker}/src/...; #598 still locks crates/{client/src/worker_cache.rs,identity/src/lib.rs,messaging/src/{lib,store}.rs,state/src/{dag,tests/materialize}.rs,web/{index.html,tests/static_assets.rs}} + Cargo.toml/lock). Targets in this run sit on .github/workflows/, docs/specs/, e2e/permissions.spec.ts, and crates/web/tests/browser.rs — all clear of both in-flight PRs.

Fixes

  • Fixes audit F13 [security]: GitHub Actions workflows lack permissions: blocks #580 — ci: declare top-level permissions: contents: read (12d0d47). All three workflow files (ci.yml, deploy.yml, e2e.yml) now declare an explicit minimum-scope GITHUB_TOKEN block. Defense-in-depth: even if the repo default ever flips wider, these workflows stay read-only. Coordinator pre-decided narrowing: no per-job elevation needed — verified deploy.yml's upload steps use sshpass + ${{ secrets.DEPLOY_PASSWORD }} over SSH/SCP, NOT GITHUB_TOKEN. Cargo gate skipped per the verified-no-asserter exception (no Rust integration test reads workflow YAML); YAML round-trip validated via Python yaml.safe_load + the audit's own grep.

  • Fixes audit F38 [arch]: web::DerivedStateActor uses Arc<Mutex> not Rc<RefCell> in WASM-only code #538 — docs(spec): note actor-bridge dedup cache exception in state-mgmt table (cf12cb9). The state-management spec proposed Rc<RefCell<>> as the canonical WASM interior-mut pattern, but state_bridge.rs:31 intentionally uses Arc<Mutex<Option<U>>> (the willow-actor framework requires Send, so Rc is structurally unavailable). Added (a) one row in the proposed CLAUDE.md-addition table covering "Web actor-handler dedup cache (deferred to F2)", (b) one cross-link sentence at the end of § F2 back to the new row — closing the table → comment → follow-up loop. Coordinator pre-decided narrowing: spec edit over F2 redesign (F2's own dedicated PR scope per its trigger).

  • Fixes audit F39 [testcov]: settings panel + back-button test should be browser-tier not Playwright #539 — test(web): migrate settings panel + back button to browser tier (62d9a62). Deletes the vacuous Playwright test at e2e/permissions.spec.ts:116-141 (already documented "until roles are added, this test verifies that the settings panel opens and the Back button returns to chat" — single-client, single-viewport DOM behaviour) + drops "server settings panel" from the mobileSkipPattern regex. Adds settings_back_button_fires_on_close in crates/web/tests/browser.rs:715-759 mirroring the existing settings_* tests' static-view + signal-driven Show pattern. Removes the now-unused openServerSettings import (still used by cross-browser-sync.spec.ts, untouched there). Coordinator pre-decided narrowing: static-view shape over real-component mount (consistent with the surrounding settings_displays_peer_id / _status_message_shows_and_hides / _shows_invite_section tests).

  • Fixes audit F40 [testcov]: non-owner channel-add hidden test should be browser-tier not Playwright #540 — test(web): migrate non-owner channel-add hidden to browser tier (af3dd51). Same shape as audit F39 [testcov]: settings panel + back-button test should be browser-tier not Playwright #539 — deletes the Playwright test (was at permissions.spec.ts:115-129 after audit F39 [testcov]: settings panel + back-button test should be browser-tier not Playwright #539's deletion shifted line numbers; original audit cited line 119) + adds non_owner_hides_channel_add_button in crates/web/tests/browser.rs:4593-4637 mirroring channel_sidebar_add_button_says_new_tree_with_glyph's placement. Asserts the same conditional-render contract channel_sidebar.rs:307 enforces (can_manage_channels().then(|| view! { ... })): no .channel-add-btn when is_owner=false; visible when is_owner=true; gone again when flipped back. Coordinator pre-decided narrowing: static-view shape over real ChannelSidebar + test_client() mount (WebClientHandle + AppState plumbing isn't wired for browser.rs today; that's a separate refactor).

Already-Fixed

None this run. The general-audit at #567 was filed 2026-05-03 against main @ 6404719; PR #596 merged into main @ c40bcea between that audit and this run, but PR #596 + PR #598 already covered the issues that were resolvable on the existing tree (5 each, plus #596's E2E batch). No surviving picks were resolved upstream by intervening landings — same-day-audit pattern from prior runs holds. Sweep was a one-line check, no time wasted.

Parked

None. All 4 picks landed cleanly. No mid-flight aborts; no finalize-implementer rescues.

Filed mid-run (follow-ups, NOT closed by this PR)

None.

Avoided picks (overlap with PR #566 + #598 files in flight)

Skill Evolution

One skill edit this run:

  • 8fc5554 docs(skill): codify coordinator-side push fallback for sandbox lag — adds a sub-paragraph to the "Implementer cuts off mid-flight" section. Captures a new failure shape observed in this run's audit F40 [testcov]: non-owner channel-add hidden test should be browser-tier not Playwright #540 dispatch: implementer committed locally but its push step didn't land on origin; stop-hook flagged the unpushed commit; coordinator pushed manually to advance the master branch + unblock the SHA-watch Monitor. Implementer's later report explicitly noted the commit was already on origin when it arrived (the coordinator's manual push had landed first). New rule: when stop-hook reports unpushed commits AND git status is clean AND git ls-remote lags HEAD, the coordinator pushes directly. Pushes are commutative under this race, so neither side loses work.

Lessons Learned

Test plan

Master-PR CI is the load-bearing gate. Each implementer ran the scoped subset locally (fmt + native clippy + native test + wasm32 check + wasm32 clippy on touched crates).

CI gates to verify on this PR:

Cargo.lock conflict expected at merge time only if PR #566 / PR #598 land first (this PR doesn't touch Cargo.lock, so it's a one-way mechanical resolution either way).

non_admin_set_profile_is_accepted (#565) status: intermittent across the last three runs. Both polarities reproduced. CI Test job may report it red; expected pre-existing, not introduced by this PR.

Generated by Claude Code

claude added 6 commits May 4, 2026 08:33
@claude
chore: open auto-fix batch claude/friendly-maxwell-iEXwu
74c0276
@claude
ci: declare top-level permissions: contents: read
12d0d47 
Per audit F13 (#580). All three workflow files now declare an
explicit minimum-scope GITHUB_TOKEN permission block at the
top level. Deploy upload steps use sshpass + DEPLOY_PASSWORD
over SSH/SCP, not GITHUB_TOKEN, so no per-job elevation is
needed. Defense-in-depth: even if the repo default ever flips
to a wider scope, these workflows stay read-only.

Skipped cargo gate per skill rule's verified-no-asserter
exception: no Rust integration test reads workflow YAML
(crates/web/tests/static_assets.rs only greps index.html).
YAML round-trip via python's yaml.safe_load + the audit's
own grep confirm the three new permissions: blocks parse
cleanly.

Refs #580
@claude
docs(spec): note actor-bridge dedup cache exception in state-mgmt table
cf12cb9 
Per audit F38 (#538). The state-management spec's decision table
proposed Rc<RefCell<>> as the canonical WASM single-threaded
interior-mut pattern, but state_bridge.rs:31 intentionally uses
Arc<Mutex<Option<U>>> for the actor-handler dedup cache (the
willow-actor framework requires Send, so Rc is structurally
unavailable). The lock is documented in-place at
state_bridge.rs:23-30 with a pointer to spec § F2 follow-up.

Spec was missing the matching exception row, leaving readers to
reconstruct the rationale from comment + F2 alone. Add:

  - one row in the proposed CLAUDE.md-addition table covering
    "Web actor-handler dedup cache (deferred to F2)";
  - one cross-link sentence at the end of § F2 back to the new
    row, so the table -> comment -> follow-up chain is closed.

No code change; F2 redesign remains the documented next step.

Skipped cargo gate per skill rule's verified-no-asserter
exception: no Rust integration test reads spec markdown
(crates/web/tests/static_assets.rs only greps index.html).

Refs #538
@claude
test(web): migrate settings panel + back button to browser tier
62d9a62 
Per audit F39 (#539). The Playwright test at
e2e/permissions.spec.ts:116-141 was a vacuous shell after role-
creation assertions were removed; it paid full setupTwoPeers cost
to run a single-client, single-viewport DOM-visibility check —
exactly the shape CLAUDE.md's "Which test tier to use" tree
routes to wasm-pack browser tests.

This commit:
- deletes the Playwright test + drops "server settings panel"
  from the mobileSkipPattern in the describe-block guard;
- removes the openServerSettings import from permissions.spec.ts
  (still used by other specs, untouched);
- adds settings_back_button_fires_on_close in
  crates/web/tests/browser.rs near the other settings_* tests,
  mirroring their static-view pattern. Asserts panel renders
  while a parent signal is true and that clicking the .btn in
  .server-settings-header flips the signal back to false.

Real SettingsPanel wiring is already covered by
settings_displays_peer_id / _status_message_shows_and_hides /
_shows_invite_section. This test fills the missing back-button
coverage at the same tier.

Local gate: cargo fmt, cargo check + clippy on
wasm32-unknown-unknown for willow-web --all-targets, and native
cargo clippy on willow-web --all-targets all pass with zero
warnings. The actual headless wasm-pack run is gated by master-PR
CI's `browser` job (wasm-pack + Firefox + geckodriver not
available locally). ESLint on permissions.spec.ts is gated by
master-PR CI's `lint` job (local node_modules missing
`@eslint/js`).

Refs #539
@claude
test(web): migrate non-owner channel-add hidden to browser tier
af3dd51 
Per audit F40 (#540). The Playwright test at
e2e/permissions.spec.ts (was line 119; shifted to ~115 after
the #539 deletion in 62d9a62) paid full setupTwoPeers cost to
verify a single-viewport DOM-visibility predicate -- exactly
the shape CLAUDE.md's "Which test tier to use" tree routes
to wasm-pack browser tests.

This commit:
- deletes the Playwright test (leaves a short signpost
  comment so future readers can find the replacement);
- adds non_owner_hides_channel_add_button in
  crates/web/tests/browser.rs next to the existing
  channel_sidebar_add_button_says_new_tree_with_glyph
  test, mirroring the static-view + Show pattern used by
  settings_back_button_fires_on_close. Asserts the same
  conditional-render contract channel_sidebar.rs:307
  enforces (`can_manage_channels().then(|| view! { ... })`):
  no .channel-add-btn when is_owner=false; visible when
  is_owner=true; gone again when flipped back.

Static-view shape over real ChannelSidebar + test_client()
mount because the surrounding browser.rs settings_* and
channel_sidebar_* tests already use it; the real-component
path needs WebClientHandle + AppState contexts that aren't
plumbed in browser.rs today.

Local gate: cargo fmt --check, cargo check
--target wasm32-unknown-unknown -p willow-web --tests, and
cargo clippy on both wasm32 and native targets all pass.
Headless wasm-pack run + ESLint (`npm run lint`) deferred to
master-PR CI -- wasm-pack/Firefox/geckodriver and
node_modules/@eslint/js aren't available locally on this
sandbox.

Refs #540
@claude
docs(skill): codify coordinator-side push fallback for sandbox lag
8fc5554 
Adds a sub-paragraph to the "Implementer cuts off mid-flight"
section of resolving-issues. Captures the new failure shape
seen in PR #599's #540 dispatch: implementer committed locally
but its push step didn't land on origin; stop-hook flagged the
unpushed commit; coordinator pushed manually to advance the
master branch + unblock the SHA-watch Monitor. Implementer's
later report noted the commit was already on origin when it
arrived — the coordinator push had landed first.

Rule: when stop-hook reports unpushed commits AND `git status`
is clean AND `git ls-remote` lags HEAD, the coordinator pushes
directly. Pushes are commutative under this race, so neither
side loses work.
@intendednull
Copy link
Copy Markdown
Owner Author

intendednull commented May 4, 2026

fix

@claude
test(state): force topo-sort order in non_admin_set_profile_is_accepted
e11f1c5 
Test was flaky (#565). After PR #505's membership gate, the
test depended on HashMap iter order in topo-sort: when SetProfile
was visited before GrantPermission, alice failed the membership
gate and the profile was rejected (`left: None, right: Some("Alice")`).

Add explicit causal dep on grant.hash so topo-sort applies
GrantPermission first regardless of iter order. Mirrors the
fix already on PR #598 for the same test; cherry-picking it
here keeps PR #599's master-PR CI green now that the human
asked for the fix in this batch rather than waiting for #598
to merge.

Refs #565
@intendednull intendednull mentioned this pull request May 5, 2026
Merged
8 tasks
@intendednull intendednull merged commit d786b36 into main May 5, 2026
8 checks passed
@intendednull intendednull deleted the claude/friendly-maxwell-iEXwu branch May 5, 2026 04:23
intendednull pushed a commit that referenced this pull request May 5, 2026
@claude
chore(ci): retrigger Playwright E2E to disambiguate flake vs regression
3f689a4 
Prior run of Playwright E2E on 4adfe2c failed; cargo-audit (rescued
to 0.22.1 in 4adfe2c) and 6 other checks passed. Local reproduction
showed widespread DAG-convergence timeouts AND iroh-relay
"dial failed: timed out" / "MultipathNotNegotiated" / "Connection
did not reach established state within timeout" errors in the relay
log — network-layer symptoms typical of CI sandbox transient
instability, not regression.

State-tier changes in this batch (#613/#614/#615) cannot affect the
failing test paths:
- #613 caps SealedContent.ciphertext at 64 KiB, validated in
  Message::validate (chat-message store path) — invite-flow tests
  never construct Content::Encrypted (no channel keys exchanged).
- #614 rejects UpdateProfile.display_name > 64 chars — Alice/Bob
  fixtures use 5-char names.
- #615 rejects Reaction emoji > 32 B / cardinality > 32 per message
  — invite-flow tests never react.

PR #599 (no state-machine changes, same `main` base) passed
Playwright E2E earlier today at 09:51 UTC, ruling out a stable
preexisting break on `main`.

Empty retrigger to confirm flake. If this run also fails identically,
bisect-revert the 3 state-cap commits one at a time will follow.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment