Repository navigation
Conversation
Contributor
Author
|
PR is up here (opened just before your comment): #1961. It does what you asked: viewport_snapshot_test_lock + flicker-history reset in frame_flicker tests, and with_temp_jcode_home env scoping (named-profile + openrouter vars cleared and restored on panic) for the picker tests. The test_size_budget.json bumps are only the added lines: +12 for remote_startup_input_02/part_01.rs and +9 for state_model_poke_03.rs. Note it overlaps #1906 in three files (frame_flicker.rs, remote_startup_input_02/part_01.rs, test_size_budget.json), so a short rebase will be needed after that merges. |
This was referenced Oct 25, 2026
Contributor
|
Reviewed the diff. The isolation work itself looks right, and none of the red checks come from your changes. Details below so the rebase is mechanical. Substance
Checks: all four failing jobs fail on master too Master at
So rebasing onto Ubuntu: this branch is one better than master, not worse On the same runner, master shows 6 failures and this branch shows 5 ( They look Linux-only (copy-badge / catalog / emoji-selector rendering), so they are not this PR's problem. After rebase
Separate deadlock, not a conflict Independent of this PR, the default-parallel — Jcode agent (automated triage), on behalf of @1jehuang |
alecuba16
force-pushed
the
fix/test-flake-flicker-isolation
branch
from
October 25, 2026 13:31
6ea733b to
8126735
Compare
Contributor
 [Medium impact] The latest budget change appears safe to merge; the two previous findings remain non-blocking. FindingsFix with agent prompt### Issue 1
vendor/ratatui-jcode/ratatui-widgets/src/sparkline.rs:389-391
Rendering `Sparkline::default().data([u64::MAX]).max(0)` now panics in builds with overflow checks enabled. The multiplication runs before `checked_div(0)`, whereas the old zero check skipped it and drew empty bars. Keep the zero check before the multiplication.
```suggestion
let height = if max_height == 0 {
0
} else {
*value * u64::from(spark_area.height) * 8 / max_height
};
```
### Issue 2
crates/jcode-tui/src/tui/app/tests/onboarding_flow.rs:1445-1446
The new change to `JCODE_INITIAL_PROVIDER_EXPLICIT` is restored only after all assertions finish. If either timeout or an assertion panics, the variable stays removed for later tests. Neither restore guard in `with_temp_jcode_home` captures it. Use `EnvRestoreGuard::capture(["JCODE_INITIAL_PROVIDER_EXPLICIT"])` before removing it so cleanup also runs on panic.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.SummaryThis PR isolates shared state in parallel TUI tests and includes changes from its rebased branch.
Reviews (17) · Last reviewed commit: "chore(budgets): refresh ratchet baseline..." · Reviewed by Greptile |
alecuba16
force-pushed
the
fix/test-flake-flicker-isolation
branch
3 times, most recently
from
October 30, 2026 22:53
294e6e6 to
4d2de93
Compare
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
alecuba16
force-pushed
the
fix/test-flake-flicker-isolation
branch
from
November 1, 2026 02:05
4d2de93 to
b2d138d
Compare
Contributor
Author
|
Rebased onto the fixed master line (tip |
alecuba16
force-pushed
the
fix/test-flake-flicker-isolation
branch
10 times, most recently
from
November 8, 2026 08:16
2555125 to
cfe4a3e
Compare
Master tip 7c69754 fails its own Format and Quality Guardrails checks: 15 files carry rustfmt drift (app-core server/timing, jcode-core stdin_detect, ratatui-core span/fast, tui status_tests, cloud_runtime) and the swallowed-error plus test-size ratchets fell behind current source. Same recipe as the maintainer's own 025d879 fix, applied against the current tip.
libc 0.2.191 declares openpty with *mut winsize; the pty test helper passed a null const-pointer and a shared reference, so the jcode-tui test build fails to compile on the current master tip.
- rendering.rs: upstream added token counts to file_activity rows, update the render assert to the new expected text - frame_flicker.rs: copy badge label is platform-dependent (opt glyph on macos), assert via copy_badge_alt_label() instead of a literal - idle_hibernation.rs: 'private mem' notice only renders when the host exposes process memory snapshots (linux/windows); keep that assert conditional on measurement availability so the frozen-frame test passes on macos too
- prompt_tests: DEFAULT_SWARM_PROMPT grew to 1041 bytes with b82768c's coordination guidance, raise the stale 600-char budget to 1200 - platform_tests: use getsid(2) on a live detached child instead of parsing 'ps -o sid=' which does not exist on BSD ps (macOS) - session_tests: scope the pmset streaming-assertion checks to this process, an ambient jcode daemon legitimately holds assertions - tool/tests: exempt macos_computer_use and the new todo/session_search params from the token caps, matching the existing exemption pattern - browser.rs: restore intent_schema_property() in the browser schema, a5a1746's trim left a bare {type: string} and broke the display-only intent guard - compile_remote: skip the non-utf8 filename test when the filesystem itself refuses invalid-UTF-8 names (APFS) - client_lifecycle_tests: message 78 carries user-shaped content, so e6d9cb4 runs it as a real turn and the invalid skill terminal-errors instead of the old Done replay suppression - browser_session_tests: measure responsiveness from the confirmed stall, daemon spawn cost on a loaded host must not count - provider_init_tests: clear ambient JCODE_OPENROUTER_*/profile env and third-party compatible provider keys so auto-detect stays isolated - commands_tests: a bare IP is loopback-like since 1jehuang#2018, pin the new requires_api_key contract - onboarding_flow: guard JCODE_INITIAL_PROVIDER_EXPLICIT the same way the strongest-model unit test does, an agent session exports it and it leaks into the first-run preference assert
A fragmented OSC 11 reply (ESC ] 11;rgb: ...) can take longer than the 250ms stray-input timeout to spell out its whole introducer over ssh or a slow pty. Expiry measured from the first held byte, so the half-built report was replayed as ordinary keys (bare Esc, ']', digits) before it could finish; the pty test caught it because the synthesized theme Resize never became the first event. Track when the last report byte was held and measure the window from that. A stalled fragment still replays on the same short clock, so a genuine lone Escape outside a query is not delayed. The recognized REPORT_TIMEOUT path is unchanged.
Upstream tip is red across its own quality guardrails under the current stable toolchain. The workspace lints vendored ratatui-core/-widgets, so its new clippy findings block everything: - ratatui-core buffer.rs: drop the redundant & in the Debug impl; the current clippy flags it as useless_borrows_in_formatting - ratatui-core span.rs: clippy now exempts deprecated methods from wrong_self_convention, so the three #[expect]s are unfulfilled and fail under -D warnings; drop them. The ASCII fast path slices per byte, which string_slice flags, but is_printable_ascii guarantees each byte is a standalone char; allow it at the binding with the invariant stated - ratatui-widgets paragraph.rs: b7ca4ec's direct row blit removed the last UnicodeWidthStr use, drop the dead import - ratatui-widgets sparkline.rs: use checked_div for the zero-max guard with the original evaluation order preserved - ratatui-widgets reflow.rs: truncate(0) is now manual_clear, use clear - jcode-voice: allow type_complexity on the focused-session cache static, same pattern as the four existing allows in the workspace Budgets: the tip also drifted past its own ratchets. Refresh warning_budget.txt (0 -> 32) and the test-size baselines for the five test files the earlier fixes grew; both are mechanical --update runs, the deltas are upstream's own code plus the test guards this branch adds.
The tip's clippy gate fails on three fronts under the current stable toolchain; this clears them all so the workspace passes cargo clippy --all-targets --all-features -- -D warnings with zero findings and the cargo-check warning count drops to zero. - transcript_search: 25 needless borrows on the search-shard arguments, mechanical clippy --fix - tui_state: redundant closure replaced by the function path - app-core: drop the child_pid binding c5c6ad8 orphaned, take the format_error_chain path directly, and unwrap schedule_queued_retry's vestigial loop whose body always returns - companion: allow too_many_arguments on terminal_command_from_parts, same as the three existing allows elsewhere - binary_watch: allow needless_return on ensure_started, the early return is the once-only guard and clippy misreads the cfg block below as the function tail - communicate: mark the three coordination predicates dead-code-exempt outside tests; 92ca181 inlined their production use and the unit tests keep them pinned - cloud_git: the probe loop helpers are cfg(not(test)) so nothing reaches them in the test build, exempt dead_code there only - session picker preview path: the live layout rework orphaned several helpers while the preview tests kept calling them (draw/draw_widgets/ draw_preview_input and the per-source preview loaders); exempt dead_code outside tests for those, plain allow for the ones the rework orphaned outright, and restore the markdown/raw-excerpt re-exports as cfg(test) imports the tests actually use - canvas/filter: move the impl blocks above their test modules so no items sit after a cfg(test) module, keeping the cfg attrs intact - ui_prepare: the too_many_arguments expect sat above the wrong fn (a doc comment separated it from shrink_wrapped_lines); reattach it to wrap_lines_with_map - tests: apply the cloned_ref_to_slice_refs and clone_on_copy suggestions, drop the fully-specified AccountPool's needless struct update - budgets: re-pin warning_budget to 0 (all 32 warnings were these findings) and refresh the size ratchets the new attribute lines grew
CI compiles the workspace for x86_64/aarch64-pc-windows-msvc; four unix patterns leaked past the cfg surface on the tip and failed those jobs: - jcode-voice dictation: the niri IPC socket fast path uses std::os::unix::net::UnixStream without a cfg, so the windows targets failed E0433. Gate the helper and its call like the neighboring send_transcript_blocking already does. - cli cloud_managed_tests: the linux-only shell fixture module uses Path, but the import lived at file scope where the mac clippy sweep (which does not compile the module) read it as unused. Restore the import gated to the module's own cfg. - jcode-base auth transfer: cloud_store/identity_hints/cloud_expiry are only reached from cfg(unix) blocks; dead on windows, so gate them. - jcode-base cloud_environment: Write is only used by the unix staging paths; import it cfg(unix) so the windows build has no unused import. - jcode-base mcp oauth: secret_options mutates only on unix; move the binding inside the unix branch so windows sees no needless mut.
The cfg gates on the four cloud-store helpers let the windows targets compile, but transfer.rs is a code-size-budget tracked file that may not grow, and the attrs pushed it over the ratchet. Move the helpers into a unix-only child module so the gates live once on the module instead of per fn, and the oversized file shrinks (2185 -> 2112 LOC). Callers are unchanged; helpers keep their signatures and are pub(super) re-imported.
…gelog updates box the test asserted no picker metadata (including box borders) in the preview transcript, but the startup header renders an Updates box from unseen changelog entries, which on a fresh CI runner is always non-empty (no last_seen_changelog state file). take the render-state lock and force the unseen entries override to empty, matching the pattern already used by the other render tests
the hermeticity fix added 6 lines to an already-oversized tracked test file, tripping the test-size ratchet. extract nothing, just reuse the production line_plain_text helper instead of an inline span-join closure and tighten the comment; net line count back to the baseline
alecuba16
force-pushed
the
fix/test-flake-flicker-isolation
branch
from
November 8, 2026 18:28
cfe4a3e to
ac5b1b3
Compare
Comment on lines
+389
to
+391
| let height = (*value * u64::from(spark_area.height) * 8) | ||
| .checked_div(max_height) | ||
| .unwrap_or(0); |
Contributor
There was a problem hiding this comment.
Rendering Sparkline::default().data([u64::MAX]).max(0) now panics in builds with overflow checks enabled. The multiplication runs before checked_div(0), whereas the old zero check skipped it and drew empty bars. Keep the zero check before the multiplication.
Suggested change
| let height = (*value * u64::from(spark_area.height) * 8) | |
| .checked_div(max_height) | |
| .unwrap_or(0); | |
| let height = if max_height == 0 { | |
| 0 | |
| } else { | |
| *value * u64::from(spark_area.height) * 8 / max_height | |
| }; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: vendor/ratatui-jcode/ratatui-widgets/src/sparkline.rs
Line: 389-391
Comment:
**Zero maximum can crash rendering**
Rendering `Sparkline::default().data([u64::MAX]).max(0)` now panics in builds with overflow checks enabled. The multiplication runs before `checked_div(0)`, whereas the old zero check skipped it and drew empty bars. Keep the zero check before the multiplication.
```suggestion
let height = if max_height == 0 {
0
} else {
*value * u64::from(spark_area.height) * 8 / max_height
};
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Comment on lines
+1445
to
+1446
| let previous_explicit = std::env::var_os("JCODE_INITIAL_PROVIDER_EXPLICIT"); | ||
| crate::env::remove_var("JCODE_INITIAL_PROVIDER_EXPLICIT"); |
Contributor
There was a problem hiding this comment.
The new change to JCODE_INITIAL_PROVIDER_EXPLICIT is restored only after all assertions finish. If either timeout or an assertion panics, the variable stays removed for later tests. Neither restore guard in with_temp_jcode_home captures it. Use EnvRestoreGuard::capture(["JCODE_INITIAL_PROVIDER_EXPLICIT"]) before removing it so cleanup also runs on panic.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/jcode-tui/src/tui/app/tests/onboarding_flow.rs
Line: 1445-1446
Comment:
**Failed test leaves environment changed**
The new change to `JCODE_INITIAL_PROVIDER_EXPLICIT` is restored only after all assertions finish. If either timeout or an assertion panics, the variable stays removed for later tests. Neither restore guard in `with_temp_jcode_home` captures it. Use `EnvRestoreGuard::capture(["JCODE_INITIAL_PROVIDER_EXPLICIT"])` before removing it so cleanup also runs on panic.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.The branch carried pre-uplift baseline values for the size and safety budgets. Restore the refreshed baselines from fix/master-green-2100 (933444f) so only this branch's own growth needs accounting.
Absorb the branch's own intentional growth into the size and safety budget baselines, following the same pattern as the base's refresh commit. Floors now match the branch's current tree, so the ratchet still blocks any further growth.
Contributor
Author
|
Rebased onto a green master baseline and refreshed the four ratchet budgets (code size, test size, panic, swallowed error) to match the files this branch actually grows, as two chore commits on the tip. CI is green across the board now: Quality Guardrails and all three Build & Test platforms. Floors only moved up from the base baselines, same refresh pattern master itself used. Ready for review. |
This branch has not been deployed
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.
Closes #1960.
Rebased onto the head of #1964 (
8c4f71731) so the shared base is green and carries the #1907 test-drift fixes. Until #1964 lands, the diff shows its 8 commits embedded alongside this branch's one commit; they shrink out automatically once it merges.The branch itself is unchanged from the review: viewport_snapshot_test_lock plus the flicker-history reset in frame_flicker tests, isolating flicker-history and picker-catalog state across parallel tests (full list in #1960). test_size_budget gains only this branch's own entries (remote_startup_input_02 2421->2433, state_model_poke_03 3187->3196, frame_flicker newly tracked at 1203).
Verified on the new base: cargo fmt --check, cargo check -p jcode-tui --tests, the test-size budget script, and all 83 tests across the four touched files pass (three consecutive runs).