fix(dash): wire client-side log file and instrument chat lifecycle#103
Open
michaelroy-amd wants to merge 7 commits into
Open
fix(dash): wire client-side log file and instrument chat lifecycle#103michaelroy-amd wants to merge 7 commits into
michaelroy-amd wants to merge 7 commits into
Conversation
tracing-subscriber is a declared dependency of rocm-dash-tui and rocm-dash-daemon but tracing_subscriber:: is never called anywhere, so every info!/warn!/debug! across those crates is silently dropped and there is no client-side log file at all. Add apps/rocm/src/logging.rs: a file-only, non-blocking tracing subscriber (tracing_appender::non_blocking) writing daily-rotated files under AppPaths::client_log_dir() (~/.rocm/logs), respecting RUST_LOG via EnvFilter with an info-for-our-crates/warn-otherwise default. It never writes to stdout/stderr, since the TUI owns the raw-mode terminal and a stray write there would corrupt the display. Old rotated files beyond a retention cap are pruned on startup so a long-lived install's logs directory stays bounded. Instrument the chat request path (crates/rocm-dash-tui/src/agent.rs): finish_agent_request, the shared complete() tail for all three live backends (Rig/OpenAI, ChatGPT OAuth, Anthropic), now logs request-start, completion (elapsed ms, reply length, skills fired), error, and timeout, tagged with the backend name — so a hung chat request is traceable in the log file. Relates to EAI-7357. Signed-off-by: Michael Roy <michael.roy@amd.com>
init_creates_log_dir_and_a_log_file asserted that the guard returned by logging::init() was always Some, but tracing::subscriber::set_global_default can only succeed once per process. When the full test suite ran unfiltered/multi-threaded, another test could win that race first, making the assertion flaky. The rolling file appender opens its current-period file eagerly at construction time, before the subscriber install is attempted, so the directory/file existence checks hold regardless of which test wins the race. Only assert on filesystem state, and skip the info! log line when the guard is None. Signed-off-by: Michael Roy <michael.roy@amd.com>
…ering the tree Adversarial-review follow-up for EAI-7357: - Retention off-by-one: prune_old_logs() ran before the daily appender opened today's file, so the effective on-disk bound was MAX_RETAINED_LOGS + 1 (8), not 7. Reorder init() to construct the appender first (which opens today's file eagerly), then prune, so today's active file is counted within the retention window. Update the MAX_RETAINED_LOGS doc to state it is the total including today's file, and add a regression test asserting the ordering keeps the total at MAX_RETAINED_LOGS. - Tests wrote fixtures under the repo tree (.rocm-work/tests/logging), cluttering the working copy. Switch temp_paths to std::env::temp_dir(). Relates to EAI-7357 Signed-off-by: Michael Roy <michael.roy@amd.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
CI follow-up for EAI-7357: - Regenerate MANIFEST.md and THIRD_PARTY_NOTICES.txt for the new tracing-appender dependency (and its transitive crossbeam-channel / symlink additions). Verified with `cargo xtask manifest --check` and `cargo xtask tpn --check`. - Harden the client log directory resolution against the CodeQL rust/path-injection alerts: the base path is derived from $HOME / $ROCM_CLI_DATA_DIR / config, so route it through a single validated_log_dir() barrier that creates the dir, canonicalizes both it and the AppPaths root, rejects `..` traversal, and requires containment under the root — degrading to no logging on any mismatch. All fs ops (appender, pruner) now run on the validated, canonicalized PathBuf. - Canonicalize the test fixtures' temp base for the same reason. Relates to EAI-7357 Signed-off-by: Michael Roy <michael.roy@amd.com>
…ducible The CI "Third-party notices current" check regenerates the notices and compares them to the committed file. ureq is dual-licensed "MIT OR Apache-2.0", and cargo-about's choice between the two offered licenses is not stable across environments — it flips ureq between the Apache-2.0 and MIT sections run-to-run, so the committed file (generated in one environment) can fail the check when CI regenerates in another. Pin ureq to MIT (one of its own offered licenses) via a per-crate `accepted` entry in about.toml. With the pin, four consecutive full regenerations are byte-identical and match the committed THIRD_PARTY_NOTICES.txt, so the check is deterministic. The generated notices content is unchanged by this commit (ureq was already under MIT in the committed file); the pin only guarantees CI reproduces it. Relates to EAI-7357 Signed-off-by: Michael Roy <michael.roy@amd.com>
…otices The previous per-crate `accepted = ["MIT"]` entry did not fix the flaky "Third-party notices current" check: cargo-about treats per-crate `accepted` as ADDITIVE to the global list, not restrictive, so it never forced ureq's license. ureq ships both LICENSE-MIT and LICENSE-APACHE and cargo-about's pick between them varies with filesystem read order, so a notice generated locally (ureq under MIT) failed CI's regenerate-and- compare (CI, like main, resolves ureq under Apache-2.0). Replace it with a proper `[ureq.clarify]` entry that pins ureq to Apache-2.0 with LICENSE-APACHE as the source of truth (checksum-verified). Three consecutive regenerations are now byte-identical, ureq resolves to Apache-2.0 exactly as main does, and THIRD_PARTY_NOTICES.txt now differs from main only by the three crates the new tracing-appender dependency adds (tracing-appender, crossbeam-channel, symlink). Verified with `cargo xtask tpn --check`. Relates to EAI-7357 Signed-off-by: Michael Roy <michael.roy@amd.com>
The logging test helper canonicalized the OS temp dir but fell back to the raw `std::env::temp_dir()` on error (`.unwrap_or(base)`). That fallback re-introduces the env-derived (tainted) path into the test's filesystem operations, so CodeQL's rust/path-injection flow was not fully cut for the test-code sinks. Replace the fallback with `expect` (the OS temp dir always exists and canonicalizes), matching production's `canonicalize(..)?` barrier which has no tainted fallback. Relates to EAI-7357 Signed-off-by: Michael Roy <michael.roy@amd.com>
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.
Summary
tracing-subscriberwas a declared dependency ofrocm-dash-tuiandrocm-dash-daemon, buttracing_subscriber::was never called anywhere in the codebase — everyinfo!/warn!/debug!call was silently dropped with no subscriber installed.apps/rocm/src/logging.rsmodule that installs a process-wide, file-only, non-blockingtracingsubscriber writing daily-rotated files under~/.rocm/logs(via a newAppPaths::client_log_dir()), with pruning to the 7 most recent files.AgentClient::complete()backends (RigAgentClient,ChatGptAgentClient,AnthropicAgentClient) incrates/rocm-dash-tui/src/agent.rs, tagged with abackendfield so a hung or failing chat is traceable to a specific provider.Root cause
The TUI owns the terminal in raw mode, so no subscriber could safely write to stdout/stderr — but no subscriber was ever wired up at all, meaning EAI-7357's client-side logging requirement was entirely unimplemented despite the dependency being present in
Cargo.toml.Implementation notes
tracing_appender::non_blocking) so it never risks corrupting the TUI's raw-mode display.logging::init()returnsOption<WorkerGuard>; the guard is held for the process lifetime inmain()since dropping it flushes and stops the writer.AppPaths::discover()or log-dir creation degrades to no logging rather than a startup failure.warn,rocm=info,rocm_dash_tui=info,rocm_dash_daemon=info, overridable viaRUST_LOG.init()call, bounding disk usage for long-lived installs.Relates to EAI-7357
Test Plan
cargo buildcleancargo clippy --all-targets --all-features -- -D warningscleancargo test -p rocm-dash-tui agent:: -- --test-threads=1— all 28 existing agent tests pass unchangedcargo test -p rocm(full suite, 3x repeated to check for flakiness) — 340/340 passed each runapps/rocm/src/logging.rs: log-dir/file creation, idempotent re-init when a global subscriber already exists, and log pruning retains only the most recent filesFollow-up / out of scope (reviewer note)
This PR wires client-side logging for the dash/TUI client (the
rocmbinary), which is the scope of EAI-7357. Instrumenting the standalonerocmddaemon binary is intentionally not included here and is left as a follow-up.