[olafura]

65 scenarios in features/timeline/tool-calls.feature were still @backlog or @backlog-<surface>: how tool calls read in a thread's timeline, as the web client and the old mobile app showed them, plus upstream pingdotgg/t3code#2829 and #13386. This makes all of them pass on the surfaces they name and takes their tags off; the file has no backlog left.

What changed

• MC: a finished command whose output says it failed is marked failed (CommandOutput.failed?/1, reading at most the first 32 KB). A running tool's progress keeps the agent's message whole (#13386; the MC already did, so this adds the steps).
• Desktop and phone (shared TimelineModel, TimelineSummary, Timeline.qml):
  • A finished turn's calls are summarised by the kind of work, and commands are named by their program.
  • HAL-C2's own tools get display names.
  • Each call has a details panel, and the changed-files card shows counts.
  • A call made through a website, an integration or an app shows that one's icon, ported from the old mobile app. App icons are signed like attachments, and only the rows that show one redraw.
• Phone:
  • The timeline reads as its activity log: one live row while a turn runs, and calls that never reported an end are dropped once it settles.
  • Long-pressing a row copies it.
  • A turn's diff opens in the thread screen.
• TUI: summaries, command labels and HAL-C2 tool names to match. A lone call reads as itself, as the web showed it.

Verified

Surface	Run	Result
Desktop	tst_Features, every timeline glob	304 passed
Phone	tst_Features, tool-calls.feature	141 passed
TUI	tool-calls.feature	83 passed
TUI	full CI set	1402 passed, 1 failed (an existing failure in the plugin-disabling scenario)
MC	timeline/*.feature	passes
MC	turn_writer tests, format, compile with warnings as errors	pass

• Property tests: tst_TimelineModelProp gained the phone's brief mode and icon signing. Each property was checked to fail when its fix is reverted.
• QML tests: desktop 787 passed, 1 failed (the existing test_sensibleTitleAndSize). Phone 105 passed.

Known failures left: the phone's full feature run fails 8 scenarios that were failing before this branch: #561, #230, #722 and the new #938. The phone's QML test entry uses the wrong test runner on some machines: #939.

Merged with main: main's phone fold mode (TimelineModel::Folds::Phone) now carries this branch's brief rules: one live row for a running turn, and unfinished calls, workspace preparation and already-carded delegations left out once a turn settles. The separate brief flag is gone. The phone's diff review and copy notice come from main, one implementation each.

Review fixes:

• A command's output marks it failed only on a line that reads as a diagnostic, and the check reads its first and last 16 KB.
• Signing a workspace image redraws only the rows that show it.

Done by Claude Opus 5.5 in Claude Code, running in HAL-C2.

🤖 Generated with Claude Code

---

^{Open in [code]smith · Need help on this PR? Tag @codesmith-bot with what you need.}

[olafura]

3 findings: 1 medium, 2 low.

This takes the last 65 tool-call scenarios off backlog across the MC, the shared Qt timeline, the phone's brief mode and the TUI. It matches the description, and the brief mode, the icon signing and the HAL-C2 tool inventory read correctly. The biggest risk is the MC's new outputIndicatesFailure heuristic. The MC writes it into the event log for good. It reads only the first 32 KB, so it misses an exit code printed at the end of a long output. It also marks a successful cat or grep as failed when the text it prints contains a failure phrase.

Not run

• Desktop and phone tst_Features, and the tst_TimelineModelProp property test: they need a full Qt build; the PR reports them passing, and I reviewed the code by reading it

Blocked on the reviewing machine (a setup problem there, not in this change)

• mise exec -- mix test test/hal_c2/turn_writer_test.exs: (Mix) Can't continue due to errors on dependencies: bandit, slack_elixir, tz and others aren't available ("run mix deps.get") in this review checkout

Ran 2 checks

• bun test src/toolSummary.test.ts (apps/tui): 11 pass, 0 fail
• elixir -r lib/hal_c2/orchestration/command_output.ex: failed? on 63 KB of output ending in 'Exit code 1', and on a source line containing 'ENOENT': false for the long output (trailer missed), true for the ENOENT line (false positive)

Findings (3)

• 🟡 Medium: apps/server-ex/lib/hal_c2/orchestration/command_output.ex:20 — The comment says a long output shows how it ended near its start, but trailers such as `<e…
• 🔵 Low: apps/server-ex/lib/hal_c2/orchestration/command_output.ex:9 — The @failures patterns match any output that merely contains the words. A completed `cat…
• 🔵 Low: apps/desktop-qt/src/native/TimelineModel.cpp:1505 — Each time a workspace-file: image is signed, every work row redraws, not just the rows t…

[olafura]

No findings.

Since the last review the only new commit is the merge with main. It folds this branch's phone "brief" mode into main's TimelineModel::Folds::Phone, and the merge holds together. In project() the brief rules are now gated on phone(): settled turns drop calls that never ended, carded delegations and workspace preparation are hidden, and a running turn ends in one live row. Main's failed-turn rule (one call per row) and its keep-the-first-reply rule are kept alongside. ThreadStore::setFolds and MobileApp's layout interceptor replace setBriefTimelines. tst_TimelineModelProp folds the separate brief() property into the main property with a Layout command, so both modes and the switch between them are now exercised. Hold-to-copy still goes through Shell.setClipboardText, so copy() now returning a string does not affect it. Nothing references brief any more.

None of my three earlier findings is fixed; the code is the same:

• CommandOutput.failed?/1 reads only the first 32 KB, so an exit-code trailer at the end of a long output is missed.
• Its patterns match any output that merely contains words like ENOENT, and the result is written to the event log.
• setAttachmentUrl still redraws every work row for each workspace-file: signing.

The first two are the biggest risk, because the wrong outputIndicatesFailure is persisted.

Not run

• mise prop:desktop tst_TimelineModelProp and the desktop/phone tst_Features runs after the merge: A full Qt build is needed; the merge was checked by reading the model next to its RapidCheck spec, which match.
• mix features timeline/*.feature: The MC files in this PR are unchanged since the last review; turn_writer.ex changed only through main's commits.

Ran 2 checks

• git diff 2c05b094a HEAD restricted to the PR's files: The only change is the merge with main; command_output.ex is unchanged, and so is the setAttachmentUrl branch for workspace-file images.
• grep for brief/setBrief/briefChanged in apps/desktop-qt and apps/mobile-qt: Nothing left over from the rename.

[olafura]

1 finding: 1 medium.

The new commit narrows CommandOutput.failed?/1: a missing-file or missing-command phrase now counts only at the start of a line or after a program: prefix, and a long output is read at both ends, so an exit code at the end is still seen. My previous review listed no open findings, so there is nothing to carry over. Reading both ends is correct. The new prefix only allows a bare word, though, so path-prefixed shell errors such as /bin/bash: line 1: foo: command not found and /usr/bin/env: 'python': No such file or directory no longer mark the command failed.

Not run

• mise exec -- mix test test/hal_c2/turn_writer_test.exs: The review checkout has no fetched Hex deps (mix run stopped with "run mix deps.get"), and fetching them was outside the review's scope.

Ran 1 check

• elixir $TMPDIR/t2.exs (the new @missing regex on sample lines): /bin/bash: line 1: … command not found, ./run.sh: … and /usr/bin/env-style lines do not match; bash: line 1: …, Makefile:12: no such file and note: … ENOENT do

[olafura]

1 finding: 1 medium.

The new commit lets a diagnostic that names a program by its path (/bin/bash: line 1: …, ./run.sh: …, /usr/bin/env: …) fail a finished command, which fixes my previous finding. The new prefix now accepts / and ., though, so a grep or rg match printed without line numbers and followed by a space now reads as a failure. That is the false positive the old prefix kept out.

Blocked on the reviewing machine (a setup problem there, not in this change)

• mise exec -- mix test test/hal_c2/turn_writer_test.exs: deps missing in this checkout: "the dependency is not available, run mix deps.get" (e.g. tz)

Ran 2 checks

• git diff 888dc64d7 HEAD: one commit: the @missing regex prefix changed, plus 3 new failure cases and 1 new success case in turn_writer_test.exs
• elixir -e (new @missing regex against grep-style lines): lib/foo.ex: {:error, :enoent} -> :ok and src/a.ts: // no such file here match (would fail the command); lib/foo.ex:12: enoent does not

[olafura]

Approved

1 finding: 1 low.

The new commit narrows CommandOutput's diagnostic prefix. It now takes either a bare program name followed by : or a path followed by : line N: or a quoted name, so search output like path:content from grep without -n, or from piped rg, no longer marks a command failed. That addresses my previous finding. The main risk left is at the edges of the prefix rule: grep matches in extensionless top-level files can still count as failures, and older bash's /bin/bash: foo: command not found now goes unnoticed. Neither should block the merge.

Blocked on the reviewing machine (a setup problem there, not in this change)

• mise exec -- mix test test/hal_c2/turn_writer_test.exs: the dependency is not available, run "mix deps.get" (tz missing from this review checkout); I did not fetch deps

Ran 2 checks

• git diff 3b40f466a HEAD: one commit: the regex change in command_output.ex plus two new negative cases in turn_writer_test.exs
• elixir script matching the new @missing regex against sample lines: lib/foo.ex: {:error, :enoent} → no match (the fix works); /bin/bash: line 1: ... and /usr/bin/env: 'python': ... → match; Makefile: # no such file here → match (false positive); /bin/bash: foo: command not found → no match (missed)

[olafura]

Approved

No findings.

The only new commit since my last review (4549da8) changes no code. It extends the comment above @missing in command_output.ex to name the two edges I raised earlier: a hit in an extensionless file at the search root, like Makefile: # no such file, still marks the command failed, and pre-5.1 /bin/bash: foo: command not found does not. My earlier finding is settled by accepting both edges and documenting them. The comment matches the regex, so both trade-offs are now explicit and nothing new is open.

Not run

• mix test for turn_writer and command_output: The new commit only changes a comment, so the tests have nothing new to cover.

Ran 1 check

• git diff 72f7f0bd9a7a9cd688e42331437868fa1586cbcc HEAD: One file changed: a 4-line comment edit in apps/server-ex/lib/hal_c2/orchestration/command_output.ex. The regex and the code are unchanged.

[olafura]

Approved

No findings.

Since the last review, one commit landed: tapping a web result now emits the brick's two-argument linkActivated(link, false), the same way every other link in the timeline does. Its step now spies on linkActivated(QString, bool). Before this, the one-argument call didn't match the signal the brick declares, so the scenario could never see the link open. The fix is correct and consistent with the brick's other link paths. The previous review raised no findings, so nothing carries over, and the new commit introduces no new risk.

Not run

• tst_Features for the timeline details web-result scenario: needs a desktop-qt native build. The change is a two-line signature match that reading the code settles.

Ran 2 checks

• git diff 4549da83e..HEAD: one commit: Timeline.qml:1832 and TimelineDetailsSteps.cpp:267
• grep linkActivated( in desktop-qt and mobile-qt QML and tests: every call to root.linkActivated in Timeline.qml and Markdown.qml now passes two arguments, matching signal linkActivated(string link, bool external) at Timeline.qml:74. The only QSignalSpy on it uses the (QString, bool) signature.

[olafura]

Approved

No findings.

Since the last review there is one test-only commit. In the property model, the phone's live "Thinking" row now shows for a latest turn that is starting as well as running. That matches TimelineModel.cpp:1327, where kRunning includes starting, and it matches main's prop generator, which starts runs as starting. The previous review had no findings, so there is nothing to follow up. The remaining gap is small: kRunning also counts preparing and waiting, which the model doesn't handle, but the generator never produces those statuses today, so the property can't diverge from the code yet.

Not run

• mise prop:desktop TimelineModelProp: This is a two-line change to the model predicate, and reading it against the implementation's status set settled it. Building the Qt prop suite wasn't needed for that.

Ran 2 checks

• git diff 48eeb0e64 HEAD: One file changed: apps/desktop-qt/tests/prop/tst_TimelineModelProp.cpp, 4 lines added and 3 removed
• grep starting/kRunning in TimelineModel.cpp and tst_TimelineModelProp.cpp: The implementation's kRunning set covers preparing/starting/running/waiting. The prop generator only produces starting/running, and the model now accepts both.

[olafura]

Approved

No findings.

The new commit (ee67940) brings workspace-image signing into line with app-icon signing. A signed workspace-file: address now redraws only the work rows that have an entry with that imageId, and only when the address or its expiry changed. Before, it redrew every work row, and the QML then asked again for images whose signing had failed. The key it matches on is the one entryFor writes (TimelineModel.cpp:1745), so the change is correct. My previous review had no findings, so nothing carries over. The one remaining gap is small: the new SignImage branch of the property test only checks that an image no call viewed redraws nothing. Nothing in the property model checks the other direction, that a row showing the image does redraw.

Not run

• mise prop:desktop tst_TimelineModelProp: the change is small and a read settles it; the author reports running the property suite

Ran 2 checks

• git diff 3807633b4 HEAD: one commit, TimelineModel.cpp setAttachmentUrl and the tst_TimelineModelProp SignIcon command
• grep imageId / workspace-file: in TimelineModel.cpp: entries write imageId = "workspace-file:" + path, the same key setAttachmentUrl now matches on

[olafura]

Approved

No findings.

This PR takes the tool-call scenarios off the backlog on the MC, desktop, phone and TUI. The new commit (f72a018) removes the step definitions the phone's StreamingSteps and ToolCallSteps both defined: "the user is looking at a thread in", "it reads" and "the turn completes". StreamingSteps now owns the first. For the other two, StreamingSteps handles its own turn and leaves the rest to ToolCallSteps: "it reads" goes to StreamingSteps once a duration was shown, and "the turn completes" once a run was started. My previous review had no open findings. The remaining risk is small: tool-calls scenarios now open their thread through StreamingSteps' setup, which also pins the clock and locale and sets a stream handle. The PR's phone run table may predate this commit, so rerun tool-calls.feature and streaming.feature on the phone before merging.

Not run

• Phone tst_Features for timeline/streaming.feature and timeline/tool-calls.feature at f72a018: Building and running the Qt feature tests was out of scope for this incremental test-only review. The author's run table may predate this commit.

Ran 2 checks

• git diff ee67940817b8c6dc1857275757c9d1f3373a96cd HEAD: one test-only commit touching apps/mobile-qt/tests/features/StreamingSteps.cpp and ToolCallSteps.cpp
• grep 'the turn completes' / 'it reads' across features/: In tool-calls.feature, the one scenario that uses "the turn completes" (changed files) never calls startRun, so Turn.run stays empty and that step falls through to ToolCallSteps. The "it reads" scenarios in tool-calls never set durationShown, so they also fall through.

[olafura]

Approved

No findings.

Since f72a018 the only new commit is the merge of origin/main (135d4b0). The one conflict, in apps/mobile-qt/src/MobileApp.cpp, is resolved correctly. It keeps main's TerminalController::setSingleScreen (and drops the old setDrawn call) and the branch's ThreadStore::setFolds(Folds::Phone/Desktop), both at startup and in the layout.desktop interceptor, and the comment now covers both. My previous review raised no findings, so nothing is carried over. The biggest remaining risk is untested: main's Run-from-code-block steps in StreamingSteps.cpp now sit next to the branch's ToolCallSteps and Phone fold mode, and I couldn't build or run the phone feature suite here. I checked by reading that their step patterns don't overlap and their helpers live in separate anonymous namespaces. The PR description's note that the brief/fold reconciliation is still to come in the merge is now stale; it is done.

Not run

• mobile tst_Features on timeline/streaming.feature and timeline/tool-calls.feature after the merge: this checkout has no Qt build directory, and building the phone app was out of scope for re-reviewing a merge commit

Ran 3 checks

• git show --cc 135d4b067: one conflicted hunk in MobileApp.cpp: it keeps main's setSingleScreen and the branch's setFolds, in both places they are called
• comm of files changed on both sides since the merge base: 5 files; I read the 4 that merged without a conflict (ThreadScreen.qml, FilesSteps.cpp, StreamingSteps.cpp, tst_Features.cpp). Each carries only main's changes, and none clashes with the branch
• grep step patterns and namespaces in StreamingSteps.cpp / ToolCallSteps.cpp / TerminalSteps.cpp: main's new code-block Run steps have no ambiguous overlap with the tool-call steps; the two files' collect helpers are each in their own anonymous namespace