Skip to content

Land #4 (prek hooks + flaky-test fixes + drop vendored Codex), rebased onto #1-3 - #6

Merged
rominf merged 13 commits into
mainfrom
land-pr4-rebased-onto-main
Jun 15, 2026
Merged

Land #4 (prek hooks + flaky-test fixes + drop vendored Codex), rebased onto #1-3#6
rominf merged 13 commits into
mainfrom
land-pr4-rebased-onto-main

Conversation

@michaelroy-amd

Copy link
Copy Markdown
Member

Lands PR #4 (prek-hooks: prek hooks + headless/flaky test fixes + drop vendored Codex) rebased onto current main (which already contains #1 lemonade-auto-backend, #2 native-certs, #3 uv-migration).

GitHub could not auto-rebase-merge the original #4 because it conflicted with #3, and main's branch protection (required signatures + reviews) blocks direct pushes — so #4's 12 commits were replayed locally onto main and the conflict resolved here. This PR squash-lands that resolved result; original #4 will be closed as landed.

Conflict resolution (verified, 3-agent refute panel clean)

Validation

  • cargo build --workspace --all-targets → exit 0
  • cargo test --workspace -- --test-threads=10 failures (1068 passed). Fix CI: prek hooks, headless/flaky test fixes, and drop vendored Codex #4 fixes the 2 pre-existing therock python_launcher failures. The 2 remaining tui.rs failures under default parallel test are headless-contention flakes (pass in isolation) and are slated for removal when tui.rs is retired.
  • cargo clippy --workspace → exit 0

Authorship of the 12 underlying commits is preserved as Roman Inflianskas (rominf).

@michaelroy-amd

Copy link
Copy Markdown
Member Author

This will get things aligned to use rocm-dash-tui instead of tui.rs but it's not going to be feature-parity at this commit. Once properly rebased, I'll implement the remaining slash commands and other TUI work that was done.

@michaelroy-amd

Copy link
Copy Markdown
Member Author

We need this PR reviewed so that it makes PR #4 merge cleanly against main.

Once reviewed and merged, we'll approve #4 and then we can get to re-implementing all the old tui.rs functionalinty.

@rominf rominf left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for landing this, @michaelroy-amd — and especially for preserving authorship of the 12 underlying commits as me (Roman Inflianskas). Replaying them onto main rather than squashing them under a single name keeps the history honest; much appreciated.

I reviewed the CI-fix commit on top (b4dfb63, fix(ci): resolve clippy/fmt/windows-build-and-test failures). It looks clean and correctly scoped:

  • clippyformat!("failed to launch uv").to_string(), the nested if let { if … } blocks collapsed into let-chains, and the Tab/BackTab/Up/Down arms collapsed into guard arms. All behavior-preserving; the let-chain form matches what's already used in uv.rs::find_binary_in.
  • dead code — dropping the now-orphaned command_succeeds and write_fake_python_without_venv_support test helpers is the right call once their callers were removed.
  • fmt — pure formatting churn, nothing semantic.
  • windows test + smoke_local.py — the "Windows V1""Windows" and --format pip--format wheel updates are correct realignments to #3's rename; these were genuine stale assertions.
  • correctness — nice catch on the rocm-core legacy-ROCm hint still suggesting the invalid --format pip. That's a real user-facing string, so fixing it (and its test) is more than cosmetic.

The conflict-resolution writeup (keeping the codex-bridge snapshot API while dropping the vendored-Codex TUI subgraph, and the therock_sdk_install_test.py pipwheel + doubled-quote syntax fix) all matches my intent for #4. LGTM.

@rominf rominf left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving — see my review comment above. Authorship preservation and the b4dfb63 CI fix both LGTM. Thanks again, @michaelroy-amd.

@rominf rominf left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switching to request-changes — withdrawing my earlier approval.

The content is fine (see my detailed comment review above; authorship preservation and the b4dfb63 CI fix both LGTM). The blocker is commit signing: all 13 commits are unsigned.

$ git log -13 --format='%h %G? %an'
b4dfb63 N Michael Roy
f16db5c N Roman Inflianskas
... (all 13 show G? = N)

main's branch protection requires signed commits, so this won't satisfy the merge gate as-is. Could you re-sign and force-push the stack (e.g. git rebase --exec 'git commit --amend --no-edit -S' <base>, or -S on each)? Happy to re-approve once the commits verify — nothing else is blocking from my side.

rominf and others added 13 commits June 15, 2026 09:23
Wire up prek (a fast drop-in pre-commit replacement) so contributors run
the same checks locally that CI enforces. Hooks cover Rust (cargo fmt on
commit; clippy and tests on pre-push to keep commits fast), Python via
ruff lint+format, shell via shellcheck, PowerShell syntax, and file
hygiene through prek's native builtin hooks. Vendored third_party/ is
excluded. Document setup and usage in the README.
Mechanical pass to make the new prek hooks pass on a clean checkout:
ruff format and import sorting across the Python helper scripts and the
PyTorch engine worker, the three shellcheck fixes in install.sh and the
WSL build-deps script, and a trailing-whitespace trim. No behavior change.
The cargo hooks surfaced two pre-existing issues on the base branch:
apps/rocm/src/main.rs had an unformatted call, and rocm-core imported
OsStr unconditionally though it is only used under cfg(windows), tripping
clippy's unused-import lint on non-Windows builds. Reformat main.rs and
gate the OsStr import to Windows so cargo fmt and clippy pass cleanly.
Recent changes on the base branch added Windows GPU/doctor inventory
helpers whose call sites are #[cfg(any(windows, target_vendor =
"cosmo"))] but whose definitions were not gated, so they are dead code
on Linux and fail clippy's -D warnings. Gate the definitions with the
same cfg as their callers so the workspace is clippy-clean on all
targets.
Three test failures only reproduced in CI's no-TTY, Python-3.12 runner:

- Chat follow-the-bottom resolved against the real terminal size before the
  first paint. When `size()` fails (no TTY) the max-scroll helper returned the
  follow sentinel and the input scroll handler bailed out, so the view stayed
  at the top instead of following the latest message. Add an
  `effective_terminal_size` fallback (COLUMNS/LINES env, then the existing
  default) and use it in both paths. This also fixes auto-follow when the CLI
  runs without a controlling terminal.

- The TUI test artifact directory lived under the workspace
  (`<crate>/../../.rocm-work/...`); on CI checkouts the resulting install paths
  are long enough to wrap inside the panels, breaking snapshot assertions and
  pushing content off-screen. Put it under the system temp dir instead.

- `python_launcher_prefers_path_python_before_saved_managed_python` appended the
  real PATH, so on a host with a genuine cp312 python the resolver chose it over
  the fake; the panic then poisoned the shared lock and cascaded into the
  sibling test. Keep PATH hermetic.

Whole suite passes; clippy and fmt clean.
`assistant_tui_support_prompts_reach_validated_local_model` and
`served_model_chat_accepts_typed_messages_and_uses_selected_model` failed
only on the Windows runner with `assert!(app.running_job.is_none())`.

Root cause: under `cargo test`'s full parallelism the runner is
CPU-saturated, so the in-test HTTP server thread and the chat worker
thread get starved. A chat turn issues up to two sequential requests
(each with a 30s client timeout), and that did not finish within
`poll_app_until_idle`'s old 500x10ms (5s) budget, so the assert fired
while the job was still running. This reproduces locally under the same
parallel load, not just on CI. The local chat path uses a raw TCP client
(not ureq), so a connection-pooling theory did not apply.

Raise the poll budget to 60s so it exceeds the worst-case request time.
It still returns as soon as the job finishes, so the happy path is
unchanged and a genuine hang still surfaces as a real failure. The fake
server also sends `Connection: close`, correct hygiene for a server that
closes after each reply (the raw-TCP client reads to EOF regardless).
The Linux CI job timed out building the ~70-crate vendored Codex
workspace (third_party/openai-codex) during packaging/acceptance.
Codex was not a build dependency and shipped no essential
functionality: it was reachable only via the hidden
--experimental-codex-tui flag spawning a separate rocm-codex binary.

Remove the vendored tree, its build script, and all Codex
build/packaging/install/acceptance coupling. Keep the CodexBridge*
machine-snapshot types and the rocm-codex uninstall matcher (to clean
up prior installs). The interactive agentic TUI will instead be
provided via an ACP client driving Goose.

Refs: EAI-6912, EAI-6913
The smoke sent only `q`, but `q` opens a "Quit ROCm CLI?" confirmation
overlay that waits for input, so the pty session hung until the 20s
timeout (exit 124). Send `y` to confirm the quit so the TUI exits
cleanly. This surfaced once the vendored-Codex build was removed and CI
started reaching the acceptance step.
No MI300X self-hosted runner is online, so the GPU smoke job sat queued
indefinitely and never reported. Retarget it to the available
windows-gfx1151-gpu-rocm self-hosted runners and adapt the job for
Windows (pwsh shell, .exe paths). The smoke only runs
detection/capabilities/doctor/resolve-model, so gfx1151 is a suitable
target.
Mirror the Linux build job's actions/cache step so Windows CI reuses the
cargo registry, git, and target dirs across runs. The cache key is
prefixed with runner.os, so Windows gets its own bucket.
The GPU smoke now runs on a Windows gfx1151 runner, so depend on
windows-build-and-test rather than the Linux build-and-test job.
The gfx1151 self-hosted runner isn't picking up the job, so the GPU smoke
hangs queued. Gate the job off with `if: false` until the runner works;
the original condition is preserved in a comment for easy re-enable.
…ek-hooks landing

The #1-4 base had CI failures from PR #3's incomplete pip->wheel rename and
pre-existing lints, exposed by #4's workspace clippy/fmt/windows jobs:

- clippy (-D warnings): removed useless format!, collapsed if-lets/match guards
  in therock.rs + tui.rs; removed two now-orphaned dead test helpers
  (command_succeeds, write_fake_python_without_venv_support).
- fmt: cargo fmt --all (comfyui/therock/tui/uv/lemonade/pytorch/rocm-core).
- windows-build-and-test: fixed stale assertions in the cfg(windows) test
  install_sdk_rejects_tarball_on_windows_before_resolution and the windows
  branch of scripts/smoke_local.py ("Windows V1"->"Windows", --format pip->wheel)
  to match #3's renamed error message.
- correctness: rocm-core legacy-ROCm hint suggested the now-invalid
  '--format pip'; updated to '--format wheel' (+ its test).
@michaelroy-amd
michaelroy-amd force-pushed the land-pr4-rebased-onto-main branch from b4dfb63 to 6f71d01 Compare June 15, 2026 16:26

@rominf rominf left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commits are signed now.

@rominf
rominf added this pull request to the merge queue Jun 15, 2026
Merged via the queue into main with commit 527d5d0 Jun 15, 2026
5 checks passed
michaelroy-amd added a commit that referenced this pull request Jun 29, 2026
… scroll, doc fixes

- #3: guard the Esc menu on a pure menu_fits() helper so it only renders
  when the logo AND all three items (Options/Help/Quit) fit; the old
  inner.height < 6 guard painted the logo with no reachable items at
  heights 6..=10. New unit test exercises the broken range + boundary.
- #4: remove the dead 't theme' launcher footer hint (no t handler).
- #5: global Help jump-to-tab now reads '1 .. 5' (5 tabs), matching draw_help.
- #6: scroll-offset casts use u16::try_from(..).unwrap_or(u16::MAX) instead
  of 'as u16' (console v/h, dock logs, bench detail, chat) — no wrap >65535.
- #7: split misattributed doc comments so apply_action and clamp_selectors
  each carry their own doc (and drop a stale gpu_scroll line).

Signed-off-by: Michael Roy <michael.roy@amd.com>
@rominf
rominf deleted the land-pr4-rebased-onto-main branch July 22, 2026 10:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants