Skip to content

fix(cli): surface the agent result in output and bad view - #137

Open
AtelyPham wants to merge 2 commits into
mainfrom
tin/print-agent-result
Open

fix(cli): surface the agent result in output and bad view#137
AtelyPham wants to merge 2 commits into
mainfrom
tin/print-agent-result

Conversation

@AtelyPham

Copy link
Copy Markdown
Contributor

Problem

When a run's completion message (agentResult.result) is empty, the agent's actual answer was written only to goalVerification.evidence in report.json. The CLI printed a bare "Goal achieved" and the bad view viewer showed the same — so the user had to jq the report to find the result.

Fix

  • test-runner.ts buildVerdict — when the completion message is empty, fall back to goalVerification.evidence (joined). This surfaces the answer in the CLI output and the report's verdict field, instead of a bare "Goal achieved".
  • cli-ui.ts — indent every line so a multi-line result stays aligned under the ✓/✗ line (single-task); collapse newlines before truncating multi-task rows so they stay single-line.
  • cli-view.ts normalizeReport — derive an answer field (completion message → else evidence) so the viewer shows the real result even for older reports whose verdict was baked as "Goal achieved".
  • viewer.html — render it in a "Result" panel (white-space: pre-wrap, HTML-escaped).

Testing

  • pnpm lint, pnpm check:boundaries, pnpm test all green — 1921 tests (+3 new normalizeReport regression tests covering: evidence fallback when the message is empty, message preferred when present, undefined when both empty).
  • Verified bad view against a real run: the answer now appears in the Result panel (previously hidden).

Empty completion message meant the answer lived only in
goalVerification.evidence in report.json; CLI printed a bare "Goal
achieved" and the viewer showed the same. buildVerdict now falls back to
that evidence; cli-view derives an `answer`; viewer renders a "Result"
panel (multi-line preserved, multi-task rows collapsed). +normalizeReport
regression test.
@AtelyPham AtelyPham self-assigned this Jul 27, 2026

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Value Audit — sound-with-nits

Verdict sound-with-nits
Concerns 3 (1 low, 2 weak-concern)
Heuristic 0.0s
Duplication 0.0s
Interrogation 99.5s (2 bridge agents)
Total 99.5s

💰 Value — sound-with-nits

Surfaces the agent's actual answer (completion message, falling back to goal-verification evidence) in the CLI output, report verdict, and bad view viewer — a real UX fix; only nit is a small duplicated derivation.

  • What it does: When a run succeeds but the agent's complete action left agentResult.result empty (its findings lived only in goalVerification.evidence), the CLI printed a bare 'Goal achieved' and the viewer showed the same — forcing users to jq report.json. This PR makes three layers surface the real answer: (1) buildVerdict in test-runner.ts:1065-1074 now derives `result?.trim() || evidence.join('\n')
  • Goals it achieves: The agent's user-visible answer reaches the user without grepping report.json — both in the terminal (single- and multi-task modes) and in the bad view HTML viewer. Particularly valuable for extract-heavy goals ('list the top 3', 'find the price') where the agent's answer is the entire point of the run but the completion message itself is often terse or empty while the substantive content sits i
  • Assessment: Good change on its merits. The problem is real and verified: runner.ts:2126/2146 just pass action.result through as agentResult.result, and that field is whatever the model typed as its completion message — frequently empty for extract goals where the answer was emitted as evidence via runner.ts:1825-2015 / execute-plan.ts:272. The fix lands in the right seams: buildVerdict already owned ver
  • Better / existing approach: Searched src/ for existing answer-extraction helpers (deriveAnswer, extractAnswer, getResult) — none exist. The one improvement worth flagging: the 'extract answer from agentResult' logic is now duplicated between test-runner.ts:1071-1074 (filter(Boolean) + join) and cli-view.ts:231-234 (strict string-only filter + join). The two filters behave the same in practice because `GoalVerificatio
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 2
  • Bridge warning: opencode/kimi-for-coding/k2p7: bridge stream ended without value-audit content

🎯 Usefulness — sound-with-nits

A focused, fully-wired fix that surfaces the agent's actual answer (which was already collected in goalVerification.evidence) into the CLI output, the report's verdict field, and the bad view viewer — instead of the bare 'Goal achieved' that previously forced users to jq the report.

  • Integration: Every change site is on a live path. buildVerdict (src/test-runner.ts:1060) is called at src/test-runner.ts:457 for every TestResult and its return value is stored as the verdict field (test-runner.ts:469) — the canonical path for all test runs. normalizeReport (src/cli-view.ts:183) is called at src/cli-view.ts:409 by bad view. CliRenderer.testComplete (src/cli-ui.ts:182) is wired via th
  • Fit with existing patterns: Fits the codebase's grain. The fallback chain result || evidence.join('\n') || 'Goal achieved' (test-runner.ts:1072-1074) is a direct generalization of the prior result || 'Goal achieved'. normalizeReport already exists to reshape report JSON for the viewer (cli-view.ts:183-254), so deriving an answer field there is in-pattern, not a new surface. The verdict field is preserved alongside
  • Real-world viability: Robust on the edge cases I checked. Both code paths guard undefined (?? []), non-array evidence (Array.isArray ? ... : []), non-string entries (.filter((e): e is string => ...)), and whitespace-only results (.trim()). Multi-line verdict is handled in the CLI: per-line indent for single-task (cli-ui.ts:204), newline-collapse + 80-char truncate for multi-task rows (cli-ui.ts:208-209). I veri
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 1

🔎 Heuristic Signals

🟡 Cruft: console debug added src/cli-ui.ts

  •  console.log(resultText.split('\n').map(line => `     ${line}`).join('\n'))
    

🎯 Usefulness Audit

🟡 'result → evidence → default' fallback duplicated across test-runner and cli-view [ergonomics] ``

The same three-step fallback (agentResult.result → goalVerification.evidence.join('\n') → 'Goal achieved'/undefined) now exists at src/test-runner.ts:1071-1074 (writes verdict going forward) and src/cli-view.ts:231-234 (derives answer for any report, including old ones). The duplication is intentional — the second is a backfill for historical reports whose verdict was already baked as 'Goal achieved' — but a tiny shared helper like deriveAgentAnswer(agentResult) would keep the two paths fr

💰 Value Audit

🟡 Answer derivation duplicated in buildVerdict and normalizeReport [duplication] ``

test-runner.ts:1071-1074 and cli-view.ts:231-234 both compute result?.trim() || evidence.join('\n') || fallback from the same agentResult. The filter predicates differ (filter(Boolean) vs (e): e is string => typeof e === 'string' && e.trim().length > 0) — equivalent today only because evidence is typed string[] (types/result.ts:37). Extract a single deriveAnswer(agentResult) helper (next to GoalVerification in types/result.ts, or in cli-view.ts and imported by test-runner.ts) so


What this audit checks

It judges the change on its merits — not whether it was tasked out in an issue. Unticketed, fast-moving work is fine; the question is whether the change is good and whether a better or existing approach should be used instead.

Pass What it asks
Heuristic Vague title? Whitespace-only or cruft-bearing diff? (content signals only)
Duplication Do added function/class names already exist elsewhere in the repo?
Value Audit What does it do? What goal does it achieve? Is it good? Better architecture or already-exists?
Usefulness Audit Does it integrate and fit? Will it hold up in real use and actually get used?

Findings are concerns, not blocks — the human reviewer decides what to do with them.

value-audit · 20260727T151848Z

@tangletools

Copy link
Copy Markdown
Contributor

✅ No Blockers — 98b57d7c

Review health 100/100 · Reviewer score 71/100 · Confidence 85/100 · 8 findings (8 low)

glm: Correctness 71 · Security 71 · Testing 71 · Architecture 71

Reviewer score is advisory once the run is complete and the verdict has no blockers.

Full multi-shot audit completed 5/5 planned shots over 6 changed files. Global verifier still owns final merge decision.

🟡 LOW Failed multi-line verdict loses per-line red coloring — src/cli-ui.ts

For passed=false, resultText = chalk.red(verdict) wraps the entire string as \x1B[31m${verdict}\x1B[39m. Then resultText.split('\n') puts \x1B[31m only on the first mapped line and \x1B[39m only on the last. Most terminal emulators carry SGR state across newlines so all lines still render red, but CI log viewers / pagers that re-parse ANSI per physical line will show middle lines in default color. Cosmetic only; consider verdict.split('\n').map(l => passed ? l : chalk.red(l)).join('\n') if uniform per-line red is desired. No correctness impact on the pass case.

🟡 LOW New multi-line branches are untested — src/cli-ui.ts

tests/cli-ui.test.ts only exercises single-line verdicts (e.g. 'User logged in successfully', 'Cart was empty'). Neither the new single-task split/indent path (line 204) nor the multi-task newline-collapse path (line 208) is covered. The PR's whole behavioral delta is multi-line handling — a 2-case vitest adding renderer.testComplete('x', true, 'line1\nline2', 4, 8000) for both total===1 and total>1 would lock it in. CLAUDE.md sets a senior-staff quality bar; missing coverage on the changed branch is the main gap on this shot.

🟡 LOW Trailing newline yields a stray indented-blank line — src/cli-ui.ts

resultText.split('\n') on a verdict ending in '\n' produces a trailing empty string, which maps to ' ' (5 spaces) — a blank indented row. outcome.verdict from the LLM path can plausibly end in a newline. Harmless but visible. .replace(/\n+$/, '') before split, or filter empty tail, would tighten it. Not a regression (old code printed ' ' + verdict verbatim, which also exhibited trailing-newline artifacts).

🟡 LOW Single-agent shape does not surface answersrc/cli-view.ts

The suite branch (lines 235-246) now adds answer; the single-agent branch (lines 273-281) returns only {id, name, recording, turns} — no answer, no verdict, no success. The viewer's Result panel (viewer.html:949 test.answer || test.verdict) is therefore never rendered for single-agent runs. Pre-existing for verdict, but this PR extends the gap by adding answer to only one of the two shapes. Either backfill answer from (data as Record<string,unknown>).result / `(data as Record<string,unknown>).goalVerification?.evid

🟡 LOW Unsafe as string cast on report-loaded data can crash viewer — src/cli-view.ts

Line 234: ((agentResult.result as string) || '').trim(). agentResult is typed Record<string, unknown> (line 199), so result is unknown. The as string assertion is compile-time only. If a malformed or hand-edited report.json has result as an object/array/number (truthy), .trim() throws TypeError and normalizeReport propagates the exception to runView, breaking the entire viewer for that run. Evidence filter on [line 233](https://github.com/tangle-network/browser-agent-driver/blob/98b57d7cecc0db0a15b86476dbdcee141c12a936/src/cli

🟡 LOW No direct unit test for buildVerdict evidence fallback — src/test-runner.ts

buildVerdict is private and has no direct test. tests/cli-view.test.ts:469-490 covers the parallel derivation in normalizeReport, but a refactor of one without the other could silently diverge. Add a test exercising the three branches (result wins; empty result + evidence surfaces joined evidence; both empty -> 'Goal achieved').

🟡 LOW Verdict length is unbounded when surfacing concatenated evidence — src/test-runner.ts

evidence.join('\n') can produce arbitrarily long verdicts (evidence entries include script/extract results per runner.ts:1695,1727). junit slices to 200, fan-out collapses+truncates, but test-report.ts:81,216 and the terminal renderer (run.ts:689) emit the full string. Consider capping (e.g. slice(0, 2000)) or moving the truncation into a shared helper alongside cli-view.ts to keep both layers consistent.

🟡 LOW Coverage gap: whitespace-only result and evidence-element filtering untested — tests/cli-view.test.ts

The impl at src/cli-view.ts:234 does ((agentResult.result as string) || '').trim() and filters evidence with typeof e === 'string' && e.trim().length > 0. The new tests only exercise result: '' (not whitespace-only like ' '), and only string evidence (no non-string or blank entries that would exercise the filter). Add a case with result: ' ' and evidence containing a non-string + a blank string to prove the trim and filter paths. Non-blocking — current tests are correct for what they cover.


tangletools · 2026-07-27T15:24:46Z · trace

tangletools
tangletools previously approved these changes Jul 27, 2026

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ Approved — 8 non-blocking findings — 98b57d7c

Full multi-shot audit completed 5/5 planned shots over 6 changed files. Global verifier still owns final merge decision.

Full immutable report for this review: trace

Summary comment for this run: full summary


tangletools · 2026-07-27T15:24:46Z · immutable trace

Address review nits on #137:
- Extract shared deriveAnswer() used by buildVerdict + normalizeReport —
  removes the duplicated result->evidence->default derivation and gives a
  directly unit-tested seam.
- Guard non-string `result` via typeof so a malformed report.json can't
  crash the viewer.
- cli-ui: color multi-line results per line; strip trailing newline so it
  doesn't emit a stray indented blank row.
- Backfill `answer` on the single-agent report shape so its viewer Result
  panel renders too.
- Tests: deriveAnswer branches (+ whitespace/non-string evidence) and the
  multi-line cli-ui render paths.
@AtelyPham

Copy link
Copy Markdown
Contributor Author

Addressed the review findings in e2db722:

  • Unsafe as string cast (crash risk)deriveAnswer now guards with typeof result === 'string', so a report with a non-string result can't throw and break the viewer.
  • Duplicated result → evidence → default derivation (+ no direct buildVerdict test) — extracted a shared deriveAnswer() (src/answer.ts) used by both buildVerdict and normalizeReport, with direct unit tests (tests/answer.test.ts).
  • Per-line red / trailing-newline blank rowcli-ui now colors each line and strips a trailing newline, so a failed multi-line result stays uniformly red and there's no stray indented blank row.
  • Untested multi-line branches — added cli-ui render tests for the single-task indent, trailing-newline, and multi-task newline-collapse paths.
  • Whitespace/non-string coveragederiveAnswer tests include a whitespace-only result and non-string/blank evidence filtering.
  • Single-agent report shape didn't surface answer — backfilled answer on that branch so its viewer Result panel renders too.

Skipped — cap verdict length: intentional. Surfacing the full answer is the point of this change; the length-constrained sinks already bound their own output (junit slices to 200, fan-out truncates to 2000), while the terminal and report.json/bad view deliberately show the complete result.

Gate green on e2db722: lint, check:boundaries (271 files), 1931 tests.

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