Skip to content

feat(map): tidy terminal UX — suppress raw output, heartbeat, clean finish - #42

Merged
justinhelmer merged 1 commit into
mainfrom
onb/map-progress-ux
Aug 19, 2026
Merged

justinhelmer merged 1 commit into
mainfrom
onb/map-progress-ux

Conversation

@justinhelmer

Copy link
Copy Markdown
Contributor

What

Fixes the terminal UX of polylane map (the command the installer's "run it for you now?" calls).

Before: polylane map inherited the agent's stdout, so a run dumped the agent's raw markdown (literal **bold**, tool-call noise) into the terminal with no framing, and showed nothing at all until the agent's final buffered response landed — which reads as hung during a multi-minute map.

After:

  • The agent's output is captured, not streamed (stdio: ['ignore','pipe','pipe']).
  • A short intro line, then a heartbeat with elapsed time while it runs (Mapping… 2m 10s elapsed), TTY-aware.
  • A clean completion line pointing at the workspace: ✓ Mapped this repository. Your topology, issues, and first-run thread are in your workspace: <console-url>.
  • On failure, the tail of the captured output is surfaced so a failure is never silent, plus the manual fallback.

Honest scope

Mapping submits its topology in a single shot at the end (nodes/edges land only at the submit phase), so there is no per-node streaming to show in the terminal. This PR makes the terminal tidy and honest; a live phase indicator in the console (empty→mapping transition + phase label) is a separate follow-up under investigation.

Verification

typecheck + lint clean; full suite 255 pass, 0 fail; map --dry-run still prints the resolved command. The pure helpers (resolveRunnable, onPath, recipe table) are unchanged and still covered.

Part of unified onboarding (coreplanelabs/nominal#1165). Needs a CLI release after merge.

🤖 Generated with Claude Code

claude[bot]
claude Bot previously requested changes Aug 18, 2026

@claude claude Bot 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.

Requesting changes on one Major finding; everything else about this PR checks out and the UX direction is a clear improvement.

What this PR does

Replaces polylane map's stdio: 'inherit' child handling with captured output (64 KiB rolling tail), a TTY-aware elapsed-time heartbeat, a clean ✓ Mapped this repository completion line with a console link, and a surfaced output tail on non-zero exit. runChild → runMapping rename (no other references to runChild remain in the repo).

Verified at head b14da29

  • PR-body claims hold: raw agent markdown is fully suppressed on success, the heartbeat and intro lines appear as described, --dry-run still prints the resolved command, non-zero agent exit surfaces the captured tail plus the manual fallback, and the pure helpers are untouched.
  • Local checks: npm run typecheck and npm run lint clean; full suite passes with 0 failures (250/250 here — the PR body says 255; the count appears to drift with the live-spec codegen, and either way nothing fails). CI green 3/3 (Node 20/22/24).
  • All 8 headlessRun recipes pass the prompt via argv, so the new stdin: 'ignore' is safe for every agent.

Blocking finding (Major)

A signal-killed agent is reported as success: close delivers code === null on signal death, code ?? 0 maps it to 0, and the CLI prints the success banner and exits 0. Reproduced by execution at this head with a fake agent that kill -KILLs itself mid-run — output ends with ✓ Mapped this repository… and exit code 0. That defeats the PR's stated guarantee that a failure is never silent, and a crash/OOM-kill during a multi-minute map is the realistic case this UX exists for. The fix is a few lines in the close handler — see the inline comment on line 109.

Non-blocking notes

  • Minor: the non-TTY heartbeat writes a log line every 5 s (~360 lines for a 30-minute map in CI/installer logs) — consider widening or skipping when !tty.
  • Minor: the "your workspace" link is the bare console root; scan.ts already resolves {consoleBaseUrl}/{slug}/… via workspacesGet.
  • Nit: the tail capture can split multi-byte UTF-8 across chunks and counts UTF-16 code units rather than bytes.
  • Observation: no tests were added for runMapping/heartbeat/failure-tail — consistent with the repo's pattern of testing only pure helpers, so just noting it.

Happy to re-review once the signal path is handled — with that fixed this looks ready to go.


Generated by Claude Code

Comment thread src/commands/map.ts Resolved
Comment thread src/commands/map.ts Resolved
Comment thread src/commands/map.ts Resolved
Comment thread src/commands/map.ts Resolved
…lean finish

`polylane map` inherited the agent's stdout, so a run dumped the agent's raw
markdown (literal **bold**, tool-call noise) into the terminal with no framing,
and showed nothing at all until the agent's final buffered response landed —
which reads as hung during a multi-minute map.

Now the agent's output is captured, not streamed: we print a short intro, a
heartbeat with elapsed time while it runs, and a clean completion line pointing
at the workspace. The captured output is surfaced only if the run fails, so a
failure is never silent. The rendered result (topology, issues, first-run
thread) lives in the workspace, not the terminal.

Note: mapping submits its topology in a single shot at the end, so there is no
per-node streaming to show here; a live phase indicator in the console is a
separate follow-up.

Part of unified onboarding (coreplanelabs/nominal#1165).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@justinhelmer
justinhelmer dismissed claude[bot]’s stale review August 18, 2026 23:20

Addressed all findings (Major signal-death, both Minors, the Nit) in the amended commit; replied on each thread and resolved them.

@claude claude Bot 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.

LGTM: all four round-1 findings are fixed at 784df0c — verified by reading the code and re-running the round-1 repros at this head.

Re-verification of round-1 findings

  • Major — signal-killed agent reported as success: fixed (execution-confirmed). runMapping now resolves the raw (code, signal) pair (src/commands/map.ts:135–138) and the caller's single if (code !== 0) branch treats null as a failure. Re-ran the round-1 repro (fake agent that kill -KILL $$s itself mid-run): the CLI surfaced the captured tail, reported "was terminated by SIGKILL before finishing the map", and exited 1 — no success banner. The normal paths still behave: clean exit prints ✓ Mapped… and exits 0 (raw agent markdown suppressed); exit 7 surfaces the tail and exits 1. The new regression tests (test/map.test.ts:107–125) cover all three cases and pass.
  • Minor — non-TTY heartbeat every 5 s: fixed (execution-confirmed). Tick is now tty ? 5000 : 30000 (map.ts:117). A 35 s piped run emitted exactly one Mapping… 30s elapsed line (round 1 saw 2 lines in 11 s). TTY behavior unchanged.
  • Minor — success message linked the bare console root: fixed. workspaceTopologyUrl (map.ts:22–33) resolves ${consoleBaseUrl}/${slug}/topology via workspacesGet, mirroring the established scansFallbackUrl pattern (scan.ts:490–501), guarded by the non-throwing tryResolveCredential and a catch-all fallback. Executed the no-credential path: it degrades to the console root without prompting or hanging, and the success run still exits 0. Copy softened to match.
  • Nit — tail capture could split multi-byte UTF-8: fixed in substance. Capture is now Buffer-based — bytes concatenated, trimmed by byte length, decoded once at the end (map.ts:102–107, 137).

Checks

Local at 784df0c: npm run typecheck and npm run lint clean; suite 253 pass / 0 fail (includes the 3 new runMapping tests; exact counts wobble with the known output.test.ts flake). CI green 3/3 (checks / Test & typecheck on Node 20/22/24).

Non-blocking notes

  • Residual cosmetic edge on the UTF-8 fix: the 64 KiB byte-trim (subarray) can still cut mid-character at the start of the buffer, so the decoded capture could begin with one U+FFFD. Practically invisible — only the last 20 lines are surfaced on failure — so not worth a change; noting it because the thread-4 reply says the tail cut is handled.
  • The PR body is mildly stale after the amend: it quotes the old completion copy ("…are in your workspace:") and "255 pass" — the head prints "View your topology, issues, and first-run thread:" and the suite is 253 locally. Cosmetic only.

Nice turnaround on the fixes — the regression tests on the signal path are a welcome addition.


Generated by Claude Code

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Auto-approved: Claude reviewed this PR and posted an LGTM verdict (see its review). A repo admin enabled this via the auto-approve workflow.


Generated by Claude Code

@justinhelmer
justinhelmer merged commit 619f1b6 into main Aug 19, 2026
8 checks passed
@justinhelmer
justinhelmer deleted the onb/map-progress-ux branch August 19, 2026 00:11
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.

1 participant