Skip to content

cmux-tui-core: snapshot_written takes the written snapshot as one value (clippy base red from #31931) - #32093

Closed
lawrencecchen wants to merge 1 commit into
feat-cmux-nextfrom
base-red-clippy-snapshot
Closed

lawrencecchen wants to merge 1 commit into
feat-cmux-nextfrom
base-red-clippy-snapshot

Conversation

@lawrencecchen

@lawrencecchen lawrencecchen commented Nov 2, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Base red (priority zero). On feat-cmux-next, clippy (changed cmux-tui packages) fails on every PR that builds cmux-tui-core (any acpmux or cmux-tui change, for example #32064 and #32065). The error is clippy::too_many_arguments (8/7) on snapshot_written in cmux-tui-core/src/output_store/snapshot.rs:537.

Culprit: e3c495d (#31931) added the eighth argument, gaps, the capture's stream-gap count. Before it, the function took 7 (4430595).

Fix, with no behavior change and no allow: the five values that describe the written snapshot (digest, size, generation, stream end and gap count) are now one Written value. The directory, the log and the tail mode stay as arguments. The body first destructures Written into the same names, so every line after it is unchanged. The one product caller and the three unit tests in snapshot_tests.rs build a Written.

FYI owner PL-daemon (output store) and @teamleaderleo (#31931).

Testing

cmux-ci run --class isolated --os linux --script scripts/ci/cmux-tui-rust-check.sh --ref SHA --arg MODE [--arg FILTER]:

  • Red at the base 4f5bcd158c2a, mode clippy: error: this function has too many arguments (8/7) at crates/cmux-tui-core/src/output_store/snapshot.rs:537, so cmux-tui-core does not compile under -D warnings. Step 3f2f3550fc478e1a670756d3.
  • Green at the head 776e0370874, mode clippy: clippy pass. Step 1d877be709b7f2a879358d6e.
  • The output store's snapshot tests at the head, filter output_store::snapshot: 16 of 16 passed, including the three tests that call snapshot_written. Step 087cdda2ea6c3d11c772abbf.
  • Panic ratchet (--arg panic) at the head: pass, step 572d6037b86502fbe4cd1c31.

This is a refactor with no behavior change, so the regression evidence is the clippy red at the base and the clippy green at the head. The existing tests show that the behavior is the same.

Observed / Not run

  • Observed: the fleet step logs above; python3 scripts/cmux-next/prepush-gate.py passes at the head; the repo's format hook (scripts/ci/format-post-edit.sh, pinned rustfmt) formatted both files.
  • Not run: no local build or test on this VM (fleet only). No macOS clippy run: the function is not platform specific.

Changelog

none

…ue (clippy base red from #31931)

e3c495d (#31931) gave snapshot_written an eighth argument (the
capture's stream-gap count), and clippy -D warnings refused it
(too_many_arguments, 8/7). "clippy (changed cmux-tui packages)" was red
on every PR that builds cmux-tui-core (acpmux, cmux-tui).

The five values that describe the written snapshot (digest, size,
generation, stream end, gap count) are now one Written value; the
directory, the log and the mode stay arguments. The body is unchanged:
it destructures Written into the same names. No allow.
@lawrencecchen lawrencecchen added risk:low Risk tier (docs/ci/risk-tiers.md) risk:normal Risk tier (docs/ci/risk-tiers.md) and removed risk:low Risk tier (docs/ci/risk-tiers.md) labels Nov 2, 2026
@github-actions

github-actions Bot commented Nov 2, 2026

Copy link
Copy Markdown
Contributor

cmux status (one comment, edited in place; each line is updated by the job that owns it)

Review 776e037: HQ feed card · gallery before/after runs · checks

App of this head (app.zip, kept 1 day)

gh workflow run reload-build.yml -R manaflow-ai/cmux --ref feat-cmux-next -f tag=pr32093 -f ref=base-red-clippy-snapshot -f platform=macos -f backend_mode=local

@lawrencecchen

Copy link
Copy Markdown
Contributor Author

Review (subagent, correctness first): SHIP at 776e037

Read-only review of git diff HEAD~1 HEAD at the head. I did not compile or run anything.

Checked:

  • Behavior is the same. The body first destructures Written into the same five names (digest, size, generation, stream_end, gaps), so every line after that is the same as before. generation is still &Arc<str>, so state.generation != *generation and generation.clone() (gives Arc<str>) have the same types as before. The fields are named, so no value can go to the wrong field. The product call site passes digest, size, &generation, stream_end, gaps as before, and tail is still a separate argument. The three tests keep their values: (false, 9, (80,24), "g", 3, 0), (true, ..., 6, 0) and (true, ..., 10, 0).
  • It compiles. Written<'a> borrows &'a Arc<str> from the generation that the Capture::Snapshot arm owns, and written lives only inside that arm. Written<'_> in the fn parameter is correct. The private struct is visible to the child module snapshot_tests.rs (#[path] mod tests) through use super::{..., Written, ...}. The first test now has an explicit let generation: Arc<str> = Arc::from("g"); binding, so the borrow outlives the call. The other two tests already had an Arc<str> binding. Written does not clash with std::io::Write or anything else in scope.
  • Callers. git grep snapshot_written over cmux-tui/ finds only the one call at snapshot.rs:336 and the three tests.
  • Clippy. The function now takes 4 arguments, so too_many_arguments is fixed. Workspace lints add only uninlined_format_args, semicolon_if_nothing_returned, redundant_clone, unused_qualifications and unsafe_op_in_unsafe_fn. The change triggers none of these. needless_pass_by_value is pedantic, and Cargo.toml notes that the workspace does not use it. All fields are read, so there is no dead-code warning.
  • rustfmt (max_width 100, use_small_heuristics Max). On one line, the let written = Written { ... }; statements would be 106/105/106 columns. Each one breaks after = with a +4 indent, and the continuation lines are 96 or fewer columns. The new signature is 92 columns on one line. The use order matches the file's existing order.

Findings (do not block):

  • The PR body's Testing section still has the literal placeholder EVIDENCE where the fleet step logs should be, but "Observed" says "the fleet step logs above". Paste the fleet job IDs and results (or remove that claim) before ready-to-land.

@lawrencecchen lawrencecchen added the ready-to-land Reviewed at this head; next-autoland merges it into feat-cmux-next when its only reds are base reds label Nov 2, 2026
@lawrencecchen lawrencecchen removed the ready-to-land Reviewed at this head; next-autoland merges it into feat-cmux-next when its only reds are base reds label Nov 2, 2026
@lawrencecchen

Copy link
Copy Markdown
Contributor Author

Superseded by #32092 (same base red, merged first).

This branch was previously deployed

3 inactive deployments
ci-routing-agent — 776e0370 Deployed Nov 2, 2026 by github-actions[bot] via cmux-next macOS placement #62360
artifacts — 776e0370 Deployed Nov 2, 2026 by lawrencecchen via publish tree-addressed cmux-tui #100554
sentry-dsyms — 776e0370 Deployed Nov 2, 2026 by lawrencecchen via upload cmux-tui dSYMs to Sentry #100554
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

risk:normal Risk tier (docs/ci/risk-tiers.md)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant