ORB-126: Print full terminal handles and retry timed-out terminal creation - #623
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
There was a problem hiding this comment.
Code Review: PR #623
Scope: PR #623 in thomasluizon/orbit-ui-mobile (feature/orb-126-print-full-terminal-handles-in-watch -> main)
Recommendation: APPROVE
Summary
ORB-126 does three things to tools/: prints the full Orca terminal handle (not a 13-char
truncation) in worker-watch.mjs, adds a bounded retry (3 attempts, 1s then 2s backoff) around
orca terminal create timeouts in launch-worker.mjs, and tightens check-ticket.mjs's
dependency-prose heuristic so it only fires when the prose actually names a Linear identifier.
All four changed files are covered by new, hermetic tools/test-tools.mjs cases that exercise the
real decision paths (recovery, bounded exhaustion + rollback, the rendered handle, and the four
prose cases), and this PR's CI shows Lint / Type Check / Build / Harness Execution all green. One
concrete test gap survives review (Medium, dimension 15).
Findings
Critical
None.
High
None.
Medium
[MEDIUM] createTerminal's non-timeout immediate-fail path has no test case
· dimension: 15 (Harness changes need EXECUTED evidence)
· location: tools/launch-worker.mjs:279-288 (paired with tools/test-tools.mjs:513-536)
· issue: createTerminal has two failure branches: a timeout (retried up to MAX_TERMINAL_CREATE_ATTEMPTS) and any other orca terminal create failure, which is meant to fail(3, ...) immediately with no retry (if (!timedOut) fail(3, ...)). The new terminalCreateRetryCases() only stages a timeout-then-success sequence and a timeout x3 sequence; there is no case that returns a non-timeout error (e.g. { ok: false, error: { code: "worktree-not-found", message: "..." } }) on the first attempt and asserts it fails immediately (1 terminal create call, no backoff pause, no retry).
· risk: this is exactly the shape dimension 15 exists to catch: a branch with real behavior (immediate-fail vs. retry) that ships unexecuted. A future edit that accidentally widens timedOut's regex or drops the !timedOut guard (so every failure gets retried, including permanent ones like a missing worktree) would pass every existing case and only surface as a real launch silently retrying for ~3s before failing anyway.
· fix: add one terminalCreateRetryCases-style case with a single non-timeout ok:false payload and assert terminalCreates === 1, exit 3, and the failure message in stderr (no "retrying" line, no "failed after N attempts" wording).
· reference: .claude/skills/pr-review/rubric.md dimension 15 ("A new decision path added to a tool that already has coverage... needs its own case, not an extension of an existing assertion").
Low / Info
None posted (signal gate).
Subagents
| Agent | Verdict |
|---|---|
| parity-checker | N/A - no apps/web/** or apps/mobile/** file changed |
| i18n-syncer | N/A - no user-facing strings or i18n JSON changed |
| contract-aligner | N/A - no packages/shared/src/types/* / endpoints.ts / orbit-api DTO changed |
| security-reviewer | N/A - no orbit-api code changed |
| design-reviewer | N/A - no apps/web/**, apps/mobile/**, or landing UI file changed |
Validation
Phase 6 (/validate) skipped per scope instructions; Build/Unit Tests/SonarCloud run as separate
required CI checks on this PR.
| Check | Result |
|---|---|
| Lint (CI: PR Tests / Lint) | PASS |
| Type check (CI: PR Tests / Type Check) | PASS |
| Build (CI: PR Tests / Build) | PASS |
Harness Execution (node tools/test-tools.mjs + node .claude/hooks/test-hooks.mjs) |
PASS |
| Unit Tests (CI: PR Tests / Unit Tests) | running at review time; PR body reports a local npm run test pass (4/4, 5m4.678s) as independent evidence |
| Guards.yml (Dash Ban, Copy Register, Suppressions Ratchet, Expo SDK Pin, Cross-Platform Parity, Skill and Agent Frontmatter) | PASS |
Deferred - N/A dimensions & files not verdicted
- Dimension 8 (DESIGN.md/AI-slop) - N/A, diff touches only
tools/**, noapps/*UI files. - Dimension 9 (Parity) - N/A, no
apps/web/**orapps/mobile/**changed. - Dimension 10 (i18n) - N/A, no user-facing strings or locale JSON changed.
- Dimension 11 (Contract drift + backward-compat) - N/A, no
packages/shared/src/types/*or orbit-api DTO changed; not verifiable in CI regardless since the siblingorbit-apirepo is not checked out in this session. - Dimension 12 (Security, API side) / Dimension 13 (Backend hard rules) - N/A, no
orbit-apicode in the diff; not verifiable in CI (sibling repo not checked out). - Dimension 14 (FEATURES.md parity) - N/A, internal agent-tooling change with no user-facing feature-surface effect.
- All four changed files (
tools/check-ticket.mjs,tools/launch-worker.mjs,tools/test-tools.mjs,tools/worker-watch.mjs) received a verdict; nothing in the diff was left unexamined.
What's good
- The retry logic matches its own stated bound precisely: 3 attempts, backoff
1000 * attemptms
(1s then 2s), and the exhaustion path is proven end to end including rollback of the one worktree
and both branches it created (tools/test-tools.mjs:529-536), not just the retry itself. check-ticket.mjs's narrowed regex was validated against exactly the three false-positive
sentences it exists to stop flagging, plus one true positive.worker-watch.mjs's test distinguishes the full handle from the old truncated-then-space form
with a negative assertion, the right level of care for a change that looks purely cosmetic and is
easy to under-test.- The ORCA stub (
orca-shim.cjs) extension (pruneRepo, object-shapedsequenceentries) is a
minimal, backward-compatible generalization; existing string-sequence callers are untouched.
Recommendation
Approve as-is. The one Medium finding (missing non-timeout-failure test case in
terminalCreateRetryCases) is a concrete, easily-added follow-up but does not block merge per the
rubric's signal gate (Medium is "fix soon; OK to merge with a tracked follow-up").



Summary
/watch, one terminal per line.orca terminal createcalls with three total attempts and bounded 1-second then 2-second backoff.blockedByrelation.Closes ORB-126.
Decisions taken unattended
Terminal creation timed outmessage. Other failures remain immediate.issue ORB-Nforms.Verification
One final tools-harness run first hit the existing
wave-plan.mjsbounded-pool timing assertion at 5.61 seconds while all ORB-126 cases passed. It was rerun after concurrent load cleared and finished withORBIT TOOLS GATE OK; no threshold or baseline was changed.