fix(stella-cli): drop imports left unused when the diagnostic impl moved out - #2045
Merged
Conversation
…ved out `main` fails `cargo check`/`clippy` on the non-test build at 94d013c: error: unused imports: `DiagnosticInvocation` and `DiagnosticRunner` --> crates/stella-cli/src/agent/tools.rs:11:46 #2034 moved `impl DiagnosticRunner for GitDiagnosticRunner` into `agent/diagnostics.rs`. Nothing outside `cfg(test)` in tools.rs names those two types any more, so the top-level import became dead in a non-test build. Why the local gate missed it: `clippy --all-targets` compiles the test cfg, where both types ARE still used (`mod tests`, `mod diff_baseline_tests`), so the import reads as live and the run is green. Only a build WITHOUT the test targets sees it — which is what `cargo check on the declared MSRV` runs. An `--all-targets` clippy can mask an unused import that only tests use; it is not a superset of the plain build. Imported per test module rather than re-added at the top, so the non-test build carries nothing it does not use. Verified both ways: `clippy -p stella-cli --bin stella -- -D warnings` (the build that was failing) and `--all-targets`, plus the 1468 tests.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Sorry @macanderson, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThis PR fixes a non-test build failure in stella-cli by removing now-unused diagnostic imports from the main module and scoping them only into the test modules that still use them, ensuring both normal and all-target clippy/test builds are clean. Flow diagram for clippy vs non-test build unused import detectionflowchart TD
A[Developer runs cargo clippy --all-targets] --> B[Compile test cfg]
B --> C[stella_cli agent_tools tests use DiagnosticInvocation and DiagnosticRunner]
C --> D[Top level import in tools_rs appears used]
D --> E[No unused import warning]
A2["CI runs cargo check on MSRV (non-test build)"] --> B2[Do not compile test cfg]
B2 --> C2[No non-test code uses DiagnosticInvocation or DiagnosticRunner]
C2 --> D2[Top level import in tools_rs is unused]
D2 --> E2[unused-imports error]
E2 --> F[Move DiagnosticInvocation and DiagnosticRunner imports into test modules]
F --> G[Non-test build has no unused imports]
F --> H[clippy --all-targets still compiles tests successfully]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
9 tasks
macanderson
added a commit
that referenced
this pull request
Aug 7, 2026
…awer (#2055) ## What & why Inspecting a turn in the Observatory happened in a right-hand drawer. That was the wrong container for what it held. A reconstructed prompt and a prompt diff are the widest text this product produces, and the drawer gave them `min(680px, 94vw)` at 12px, reached by clicking an unmarked table row, with the diff two disclosures deep and no URL to reload or share. The path to it was invisible too: the sessions and turn tables carried no cursor, no hover, and a `sel` class no stylesheet defined, while the panel a click revealed opened below the fold — so a click read as a click that did nothing. Inspecting a turn is now a route: `#transcript/<execution>`. - **Full page, one step up the type scale.** Diff and transcript bodies move from 12px to 13px with reading line-height, at page width instead of a third of it. - **The prompt diff is the first thing you see.** The newest call's diff opens on arrival — the newest, because a role's *first* call has no previous call to diff against and renders the entire prompt as one wall of additions. The diff is drawn the way a reviewer reads one: both line-number gutters, and tinted rows rather than colored glyphs alone (hue is never the only signal — BRAND.md). - **The call picker is a control that looks like one.** A real turn in this workspace records 202 receipts, so it scrolls in a fixed box with the picked row lit and pulled into view, above the panel it drives. - **A sticky rail** across the six sections, with counts. - **← / → step through the owning session's turns** without going back to the list. - **The descent is visible again:** `tr.click` on both tables, a gold `turns ↓` / `open ↗` cell that names what a row does, a real selected-row style, and a scroll to the panel a click just revealed. Every payload is unchanged — the same four endpoints, the same lazy per-call reconstruction, the same `after_seq` live append. One field is added to `/api/execution`: `session_id`. Without it a transcript reached by URL alone can name neither its session nor the turns either side of it. Exemplar for the route grammar: none needed — it is `#<tab>` / `#<tab>/<arg>`, the same shape the page already used for tabs, extended by one argument. The diff view follows git's own hunk presentation, which is what the served endpoint already emits. ## The witness - [x] This PR includes a witness test (fails on `main`, passes here) Two, one per half: - `execution_detail_carries_its_session_id` — asserts `/api/execution` serves `session_id`, and that the key survives as `null` (not as a missing key) for a run recorded before schema v8 stamped it, because the page distinguishes "no session" from "field not served". On `main` the execution head selects nine columns and `session_id` is not among them, so the first assertion fails. - `inspecting_a_turn_is_a_page_not_a_drawer` — asserts the transcript panel and its route exist, that both drill paths navigate to it, and that the drawer is *gone* rather than merely bypassed (a second, stale way in is how two renderings of the same data drift apart). On `main`, `id="drawer"` is present and `data-tab="transcript"` is not. The second is a structural witness and does not claim the page *renders* right; that is not decidable from Rust. It was verified by driving the served dashboard in headless Chrome over CDP against this repository's own `.stella/private/store.db` — real sessions, real receipts, real 202-call turns: | checked | result | | --- | --- | | `#transcript/83` cold, no session list in memory | route resolves, rail builds 5 sections, 202 call rows, newest diff open at 51 lines / `+48 −0` | | `#transcript/73` cold (turn 3 of 4) | breadcrumb resolves the session from `session_id` alone; ← and → both enabled; all 6 sections including Reflection | | `#transcript/76` (`session_id` NULL, no receipts) | "session unknown" / "Execution 76", prev-next disabled, the server's own no-receipts note preserved | | `#transcript/999999` | the honest "could not be loaded" panel | | `#transcript/notanumber` | route rejected, address restored to the route actually on screen | | Sessions → turn row → back button → Escape | `#transcript/83` → `#sessions`, tab re-hidden, panel cleared | | Overview execution row | navigates to the same page | | full navigation cycle | zero JS errors, zero unhandled rejections | Screenshots of the transcript page and the Sessions tab were reviewed. ## The gate - [x] `cargo fmt --check` - [x] `cargo clippy --workspace --all-targets -- -D warnings` - [x] `cargo test --workspace` - [x] Docs updated where behavior changed - [x] CLA signed `make gate CARGO_SCOPE="-p stella-observatory"` exits 0. Docs: `website/content/docs/telemetry/dashboard.mdx` loses its drawer paragraph and gains `sessions` and `transcript` cards; the crate README gains the route grammar. Three stale claims the change touched are repaired in the same PR — the doc's tab count (said nine, was eleven before this PR), the README's `seeded_workspace` location (`src/lib.rs` → `src/tests.rs`), and its list of store columns the fixture diverges on, which named `session_id` and no longer can. ## Nothing left behind - [x] Filed: #2053, #2054 - **#2054** — the transcript page renders every journal entry eagerly: measured on a real turn in this workspace, 3,289 nodes and 1.1MB of HTML. Inherited from the drawer, which built the same list from the same payload, but the page makes it the primary surface. Options and a definition of done are in the issue. - **#2053** — `cargo build -p stella-cli --bin stella` warns on two dead imports in `agent/tools.rs`. Noticed while running the gate here; this branch touches no `stella-cli` file, so it is inherited from `main` (`6bc6a072`). It is the #2045 shape: `clippy --all-targets` compiles the tests, where the names *are* used, so no gate configuration can see it. ## Ground-rule check - [x] No I/O added to `stella-core`; no new deps - [x] No new outbound network calls — the page stays fully self-contained (`dashboard_html_has_no_external_references` still passes) ## Anything reviewers should know? - **The drawer is deleted, not deprecated.** Both entry points (Overview executions, Sessions turns) now navigate to the page, and the witness asserts no drawer survives. If you would rather keep an overlay for the Overview table, say so — but two renderings of the same four endpoints is exactly the drift this repo files issues about. - **The transcript tab is hidden until a turn is open** and disappears when you leave. It is a child route of Sessions, not a section you visit; an always-present tab with nothing in it is a dead end. The consequence is that the tab bar's roving-tabindex arrow keys now walk `visibleTabs()`, since focus must never park on a `hidden` button. - **Section order puts context above the transcript.** That is deliberate: what a call was *given* is the question the drawer buried deepest, and the rail makes both one click away regardless. - `markCtxRow` scrolls the picker box by setting `scrollTop` rather than calling `scrollIntoView`, which walks every scrollable ancestor — on arrival that would scroll the *page* to reach the list, which is the below-the-fold jump this page exists to remove. ## Summary by Sourcery Replace the execution detail drawer with a dedicated transcript page route and update backend, UI, tests, and docs to support deep-linked turn inspection. New Features: - Add a transcript tab and full-page transcript view addressed as the route `#transcript/<execution>` with sections for context, transcript, steps, tools, files, and reflection. - Enable keyboard and hash-based navigation for transcripts, including previous/next turn stepping within a session and direct linking/reloadable URLs. Bug Fixes: - Make session and turn tables visibly interactive with proper cursor/hover states, selected-row styling, and explicit affordances for opening turns. Enhancements: - Redesign transcript and diff presentation for better readability, including wider layout, larger type, sticky section rail, and an improved unified diff view. - Refine context receipt UI with a scrollable call list, clear selection state, and inline controls to toggle between context and diff views. - Integrate transcript live-refresh into the main polling/live stream loop, continuing to append journal entries while a run is active. - Adjust tab bar behavior so arrow-key navigation respects only visible tabs and correctly handles the conditional transcript tab. Documentation: - Update telemetry dashboard docs and README to describe the new transcript page, routing grammar, and corrected tab/schema details. Tests: - Add tests ensuring `/api/execution` includes `session_id` and that the transcript page and its routes exist while the old drawer implementation is fully removed. Chores: - Extend the test SQLite schema and seed data to include `executions.session_id` now that the observatory reads it from the store.
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
1 participant
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.
mainis red.cargo check/clippyfails on the non-test build at94d013c5:My breakage, from #2034. Fixing rather than leaving it.
Cause
#2034 moved
impl DiagnosticRunner for GitDiagnosticRunnerout ofagent/tools.rsintoagent/diagnostics.rs(tools.rs had crossed the 1500-line limit). After that move, nothing outsidecfg(test)in tools.rs names those two types, so the top-level import is dead in a non-test build.Why the local gate did not catch it — worth knowing
make lintrunsclippy --all-targets, which compiles the test cfg. Both types are still used there (mod testsat 844,mod diff_baseline_testsat 1365), so the import reads as live and clippy is green. Only a build without the test targets sees the unused import — which is exactly what CI'scargo check on the declared MSRVjob runs.clippy --all-targetsis not a superset of the plain build for unused-import purposes. A symbol used only undercfg(test)keeps a top-level import looking alive. Anyone extracting code out of a module wants to check both.Fix
Import the two types per test module instead of at the top, so the non-test build carries nothing it does not use. No behaviour change, no test change.
Verified both ways
cargo clippy -p stella-cli --bin stella -- -D warnings— the build that was failing. Clean.cargo clippy -p stella-cli --all-targets -- -D warnings— clean.cargo test -p stella-cli --bin stella— 1468 pass.Summary by Sourcery
Resolve unused import errors in stella-cli by scoping diagnostic imports to test modules.
Bug Fixes:
Tests: