Add live run lists, detail pages and transcript tails - #149
Conversation
Broadcast committed run and spend changes so operators can follow progress without refreshing. Record the first observed agent start and expose run definition and token spend while preserving unknown values. Add encoded detail routes and bounded, filtered transcript tails with session-aware reconnects and a REST cursor for earlier history. Cover the read models, sockets and UI with regression tests and mutation checks. Signed-off-by: Artjoms Stukans <artjoms.stukans@gmail.com>
artyomsv
left a comment
There was a problem hiding this comment.
Review — the epic is delivered; one regression to settle before merge
All six sub-tasks are implemented and their named mutation checks are real. Verified locally on e228dea:
| Check | Result |
|---|---|
npx vitest run |
613 passed / 75 files — matches the claim |
styles.contract.test.ts + App.routes.test.tsx |
green |
:spire-orchestrator:test --tests '…factory.*' |
BUILD SUCCESSFUL |
| New suites | EveryRunWriteIsBroadcastTest 1 · RunsBroadcasterTest 3 · RunAgentStartedTest 5 · RunDetailReadTest 9 · RunsSocketTest 11 · RunChargesTest 13 — 0 failures |
What is notably well done
EveryRunWriteIsBroadcastTestis a real guard, not a ceremony. It parses the javac AST, resolves SQL held in field constants, builds the local call graph, and falls back to "every caller of this private writer must push". A writer with no callers fails.assertFalse(writers.isEmpty(), …)stops it passing on an unreadable source — that is the failure mode this project has been bitten by.- The cost-push gap was found and closed.
RunResultSagaprojects before it charges, so the outcome push carries no cost.RunChargesnow pushes again after the ledger commits, which is the only thing that keeps a FIX run's cost live — a FIX run has no later PR write to piggyback on. The stale comment inReviewProjectionthat predicted this was updated rather than left to rot. RunSpend.cost()is honest. No charge lines → unknown. Any unpriced line → unknown. Never zero. ADR-023 survives to the screen.- The splat decoding is tested where it matters.
RunDetail.test.tsxdrives/runs/${encodeURIComponent(id)}with an id carrying both/and:, assertsgetRunwas called with the decoded id, and adds a case with a literal%41that a second decode would corrupt. That is the discriminating case, not the obvious one. queuedTimeis correct.Date.parseties on microsecond-precision PostgreSQL timestamps; the fraction slice recovers sub-millisecond nanoseconds, and the tiebreak (run_iddescending) matches the server'sORDER BY r.started_at DESC, r.run_id DESCexactly.
Findings
| # | Where | Severity |
|---|---|---|
| 1 | useLiveRuns.ts:33 / Runs.tsx:147 |
fix before merge — a filtered screen says "No matching runs" when matching runs exist |
| 2 | RunResource.java:317,567 + RunTranscriptSocket.onOpen |
worth fixing — three existence checks now pay for a spend aggregate |
| 3 | RunsBroadcaster.java:38 |
worth fixing — a permanently unsendable row is silent in production |
| 4 | FactoryRunProjection.java:970 |
nit |
Detail is inline. Nothing here loses data and nothing blocks the feature; #1 is the only one that makes a screen state something untrue.
Epic status
#133 #134 #135 #137 #138 are satisfied. #136 is satisfied apart from finding 1, which is a consequence of the client-side filter that issue chose.
Disclose and enforce the newest-200 filter window, avoid spend reads for existence checks, and warn on permanent serialization failures. Cover the regressions and both WebSocket protocols without insecure URL test literals. Signed-off-by: Artjoms Stukans <artjoms.stukans@gmail.com>
artyomsv
left a comment
There was a problem hiding this comment.
Re-review of 2b6d159 — GO
All four findings are fixed, each with a test, and the two that mattered were mutation-verified here rather than taken on trust. Every review thread is resolved.
What was fixed
| # | Fix | Test |
|---|---|---|
| 1 | MAX_LIVE_RUNS is one exported constant driving the REST limit, the trim and both strings. The window is disclosed beside the filters, the empty state reads No matching runs in the newest 200, and sortRuns ends .slice(0, MAX_LIVE_RUNS) — which also closes the unbounded-growth half. |
300 rows, the only cancelled at index 249, asserts the new wording, asserts the old wording is gone with { exact: true }, pushes the out-of-window run and asserts it does not appear, pushes an in-window change and asserts it does, asserts one getRuns call. Plus a 202-row trim test across REST, snapshot, older update and new arrival. |
| 2 | exists(runId) is SELECT 1 …; all three existence checks moved to it — resolveDispatch, transcript, RunTranscriptSocket.onOpen. find() is left to the one caller wanting the view. |
existenceDistinguishesAnEmptyRunFromAnUnknownId |
| 3 | Catch split: JsonProcessingException warns, RuntimeException stays DEBUG. |
anUnserialisableRowWarnsWithoutFailingTheProjection attaches a handler and asserts the level, the run id in the message and the throwable type — it verifies the warning, not merely the absence of a throw. |
| 4 | Spend read hoisted to a local, with the PgJDBC guarantee written down. | unchanged behaviour, so nothing new needed |
Verified here, not read
| Check | Result |
|---|---|
npx vitest run |
617 passed / 75 files (613 → 617) |
:spire-orchestrator:test factory + ws |
BUILD SUCCESSFUL — 349 tests, 0 failures, 0 errors |
Mutation: drop .slice(0, MAX_LIVE_RUNS) |
2 failed / 34 passed — exactly the two intended |
Mutation: restore the combined JsonProcessingException | RuntimeException catch |
4 tests, 1 failed — exactly anUnserialisableRowWarnsWithoutFailingTheProjection |
| Working tree after both mutations | clean, re-run green |
CI is green on every required check, including service tests + packaging (3m28s). That closes the one uncertainty the PR description left open — packaging was interrupted locally and is now proven on a runner, so no claim in this PR rests on an unfinished local build.
Two Semgrep findings on insecure ws:// literals in the new tests were also fixed in passing: both socket tests now parameterise over http:/https: and assert the derived ws:/wss: protocol, which is a better test than the literal it replaced.
Not a blocker, but say it out loud
- The run detail page is read-only. Cancel, steer and resolve-dispatch stay
curl. That was #137's explicit scope decision and it is tracked intechdebt/spire-ui/4-3-three-factory-surfaces-still-have-no-screen.md, which is now the only factory screen gap left. - Nightly
testE2ewas not run, as the description says. That tier drives a containerised GitLab and reaches none of this code, so it is the right thing to skip — noted only so the gap is on the record rather than assumed away. agent_started_atis NULL for every existing run and always will be. The phase strip saysnot recorded, which is the honest answer; nobody should later "fix" it with a backfill.
Merge
mergeStateStatus is BLOCKED on REVIEW_REQUIRED — branch protection wants an approving review, and this review cannot supply it because the reviewing account is the author. A human approval is the only thing standing between this and merge. Everything reviewable is done.
Runs previously required a refresh to show progress and had no detail screen. This adds a live list and linked run pages showing recorded phases, runtime, spend, task summary, delivery and a live transcript.
/api/ws/runs.beforecursor to the REST transcript so the earlier-history link retrieves the preceding page.Validation:
e228dea(the original local packaging attempts were inconclusive)../gradlew testFast testServicesone228dea: passed; aggregate reports contain 2,938 tests across 331 suites, zero failures/errors and one existing skip. Unchanged tasks reused their results.npm test: 617 tests across 75 files passed;npx tsc --noEmit: clean.Implementation decisions and verification details: live-runs record.
Review follow-up validation:
Closes #147
Closes #133
Closes #134
Closes #135
Closes #136
Closes #137
Closes #138