feat(tools): teardown completed worktrees - #619
Merged
thomasluizon merged 3 commits intoJul 27, 2026
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR #619 Review — feat(tools): teardown completed worktrees (closes ORB-124)
Scope reviewed: full diff — tools/teardown-worktree.mjs (new), tools/test-tools.mjs, tools/README.md, .claude/skills/orchestrate/SKILL.md. No prior reviews or resolved threads existed on this PR.
Recommendation: REQUEST CHANGES
| Severity | Count |
|---|---|
| Critical | 0 |
| High | 1 |
| Medium | 1 |
High — tools/teardown-worktree.mjs:99-101 — tree-present check compares the whole repo tree, not the branch's own changes
git(path, ["fetch", "--quiet", "origin", base], { allowFailure: true })
const baseRef = git(path, ["rev-parse", "--verify", "--quiet", `origin/${base}`], { allowFailure: true }) ? `origin/${base}` : base
const treePresent = git(path, ["diff", "--quiet", baseRef, branch], { allowFailure: true }) !== nullgit diff --quiet baseRef branch only exits 0 (no differences) when the entire trees are byte-identical. /orchestrate runs tickets in parallel waves forked from the same base commit. Once sibling ticket A merges to main first, origin/main's tree contains A's files, which ticket B's branch (forked before A merged) never had. A whole-tree diff between B's own branch and the now-updated main shows a difference from A's content alone — even though B's own work merged cleanly. treePresent comes back false and teardown refuses B, for every ticket in a wave except whichever merged first. There is no override flag in the documented CLI flow. This is fail-closed (no data loss), so High rather than Critical, but it defeats the tool's primary real-world use case: tearing down worktrees after a normal multi-ticket /orchestrate wave.
Verified directly against the code (not just the skill's report): the only fixture exercising the merged case is selector in tools/test-tools.mjs (stageTeardownWorktree(..., { changed: true, squashMerged: true })), which writes the same captured.txt content into both primary and child — i.e. it only proves exact-tree-match detection, and cannot catch a sibling ticket's unrelated content landing on main first.
Fix: scope the presence check to the branch's own changed paths (diff against its actual fork-point) rather than the entire tree, and add a fixture where an unrelated file lands on main from a sibling ticket before asserting the branch under test is still recognized as merged.
Medium — tools/teardown-worktree.mjs:47-52 — dead validation branch + silent fallback on malformed --base
const requestedIssue = argOf("--issue")
...
if ((requestedIssue && !argOf("--issue")) || (requestedWorktree && !argOf("--worktree")) || (requestedBase === null && process.argv.includes("--base"))) fail(2, ...)requestedIssue && !argOf("--issue")is alwaysx && !x(false) sincerequestedIssuewas just assigned fromargOf("--issue")— same for the--worktreeclause. Both are unreachable dead code.- The
--baseclause checksrequestedBase === null, but a trailing--basewith no following value producesargOfreturningundefined(out-of-bounds array access), notnull. That malformed input never triggers the usage error and instead silently falls through tobase = requestedBase ?? worktree.baseRef ?? "main", defaulting tomain/the worktree's base rather than erroring on the typo.
Low real-world exposure, but a brand-new tool shipping unreachable code plus a silent fail-open path for malformed CLI input.
What's good
The four-check evidence gate (clean tree, tree-present, Linear Done, terminals idle) fails closed and verifies orca's removal response against the filesystem/git worktree list rather than trusting it, per tools/CONVENTIONS.md's "gate tools" discipline. test-tools.mjs coverage is thorough for the scenarios it does model: dirty tree, unmerged content, non-Done issue, a repainting terminal, a lying "removed" response from orca, and squash-merge without ancestry.
Not applicable to this diff: parity, i18n, contract drift, DESIGN.md, backend hard rules, FEATURES.md — this PR touches only tools/** and .claude/skills/**, no apps/* or orbit-api surface. Cross-repo dimensions (contract-aligner, orbit-api side of the backward-compat guard) are not verifiable in this CI job since orbit-api is not checked out here.
Owner
Author
|
Review round 1 complete in c4e7016. High: tree presence now derives branch-owned paths from the merge base and compares only those paths against the target. The new sibling-target-advance fixture was verified against the old whole-tree comparison first: it failed with UNMET tree-present-in-target, then passes with the fix. Medium: value-taking flags now distinguish absent from valueless input and reject valueless --issue, --worktree, and --base with exit 2. The dead validation branch was removed. No inline review threads are available through GitHub to reply to or resolve; the submitted review contains these as two review sections. This is recorded in the PR decisions section. |
There was a problem hiding this comment.
Code Review: PR #619 - feat(tools): teardown completed worktrees
Recommendation: NEEDS WORK (1 High finding; everything else is clean)
Summary
Reviewed the full diff (.claude/skills/orchestrate/SKILL.md, tools/README.md,
tools/teardown-worktree.mjs, tools/test-tools.mjs) against the PR review rubric. This
is a tools-only change (no apps/*, packages/shared/src/types, or orbit-api touched),
so Parity, i18n, Contract-drift, DESIGN.md, backend-hard-rules, and FEATURES.md dimensions
are N/A. Harness Execution (dimension 15) is verified GREEN via this PR's own CI run
(Guards / Harness Execution: SUCCESS, matching the PR body's node tools/test-tools.mjs
and node .claude/hooks/test-hooks.mjs output), and the added test coverage for the new
tool is extensive (usage errors, all-good/runtime-unavailable removal, dirty tree,
unmerged tree, non-Done issue, repainting terminal, lying removal response, path selector
with squash merge, sibling-branch-advance).
Findings
High
[High] normalize() crashes on any terminal missing worktreePath, before the worktree-scoped filter even runs
- dimension: Type safety (untyped external payload across a trust boundary)
- location: orbit-ui-mobile/tools/teardown-worktree.mjs:79, 95
- issue:
normalize = (path) => resolve(selectorPath(path))...calls Node'spath.resolve()
on whateverorca terminal listreturns, for EVERY terminal in the whole fleet
(terminals.filter((terminal) => normalize(terminal.worktreePath) === normalize(path))
runs over the unfiltered list before narrowing to this worktree).resolve(undefined)
throwsTypeError [ERR_INVALID_ARG_TYPE]— an unhandled crash, not one of this tool's
ownfail(code, message)exits. - risk: This is untyped external CLI output crossing a trust boundary with no narrowing.
The sibling tool that already iterates the same terminal list,worker-watch.mjs,
guards for exactly this:const normalize = (path) => (path ?? "").replaceAll(...)
(tools/worker-watch.mjs:118) — it never callsresolve()and treats a missing path as
an empty string.teardown-worktree.mjs'snormalizediverges from that established,
defensive convention and will hard-crash the automated/orchestrateteardown step for
a whole ticket if ANY terminal in the live fleet (e.g. a main-repo/orchestrator terminal
not tied to a child worktree) is returned without aworktreePath— not just terminals
belonging to the worktree being torn down. No fixture in the addedtest-tools.mjs
coverage exercises a terminal list with more than one entry or with a missing
worktreePath, so this path is unexercised by the otherwise-thorough harness. - fix: Make
normalizenull-safe before resolving, matchingworker-watch.mjs's pattern,
e.g.const normalize = (path) => (path ? resolve(selectorPath(path)) : "").replaceAll(...),
or filter out entries with noworktreePathbefore normalizing on line 95. - reference: CLAUDE.md rule 3 / rubric dimension 6;
tools/worker-watch.mjs:118as the
established in-repo precedent.
Medium
[Medium] Several new refusal branches have no dedicated test case
- dimension: Harness changes need EXECUTED evidence (missing test)
- location: orbit-ui-mobile/tools/teardown-worktree.mjs:53, 54, 85, 86, 87, 141, 144
- issue: the exactly-one-selector refusal (neither or both of
--issue/--worktree), the
--issueformat guard, the not-found case, refusing a worktree with
isMainWorktree: true, refusing a worktree with nolinkedLinearIssue, and the
branch-still-exists-after--Dfailure all have real, reachable code paths but no
matching row intools/test-tools.mjs'steardownWorktreeCases. The tool's other
decision paths (dirty, tree-not-present, not-Done, repainting terminal, lying removal
response, squash-merge equivalence) are all well covered — these six are the gap. - risk: an unexercised refusal branch is exactly the class of defect this rubric dimension
exists to catch — it merges unexecuted and the next change to this tool inherits the
same hole. - fix: add a
check(...)row per branch, following the samestageTeardownWorktree/
teardownPlanpattern already used for the other cases. - reference: rubric dimension 15; severity ladder "missing test".
Low
[Low] Unused import realpathSync
- dimension: Dead / stale code
- location: orbit-ui-mobile/tools/teardown-worktree.mjs:9
- issue:
realpathSyncis imported fromnode:fsbut never referenced anywhere else in
the file.tools/has no rooteslint.config.*, so this is not caught by any lint gate. - fix: drop
realpathSyncfrom the import. - reference: CLAUDE.md rule 2.
Subagents
| Agent | Verdict |
|---|---|
| parity-checker | N/A — no apps/web/** or apps/mobile/** files changed |
| i18n-syncer | N/A — no user-facing strings changed |
| contract-aligner | N/A — no orbit-api or packages/shared/src/types changes |
| security-reviewer | N/A — no orbit-api code changed |
| design-reviewer | N/A — no UI files changed |
Validation
Sourced from this PR's own passing CI checks (local execution of node tools/test-tools.mjs
/ node .claude/hooks/test-hooks.mjs was not possible from this review session's sandbox).
| Check | Result |
|---|---|
| Lint | PASS |
| Type check | PASS |
| Unit tests | PASS |
| Harness Execution (test-tools.mjs + test-hooks.mjs) | PASS |
| Build | PASS |
Deferred
- Parity, i18n, Contract-drift + backward-compat, DESIGN.md/AI-slop, Backend hard rules,
FEATURES.md parity: N/A, diff touches none of their surfaces. - Validation above is sourced from this PR's own CI run rather than a fresh local
execution, becausenodeinvocations were not available in this review session's
sandbox.
What's good
- The four-check evidence gate (clean tree, tree-present-in-target via content diff
rather than ancestry, linear-done, terminals-idle) correctly handles squash-merges
without requiring commit ancestry, with tests proving both the squash-merge-present and
sibling-branch-advanced-on-unrelated-paths cases. - Orca's
worktree rmresponse is correctly treated as non-authoritative; removal is
independently verified from the filesystem andgit worktree list, matching the tool's
own doc comment and the PR's stated design decision. SKILL.mdandtools/README.mdupdates accurately describe the shipped tool's
contract and refusal behavior.- Harness Execution CI check is green for this exact diff.
Recommendation
Fix the normalize() null-safety gap before merge (High — it can crash the automated
/orchestrate teardown step on a live fleet with unrelated open terminals). The Medium
test-coverage gap and the Low unused import are worth cleaning up in the same pass but do
not block on their own.
|
Owner
Author
|
Review round 2 complete in f8d69f5. High: terminal-path normalization now ignores absent or non-string paths before resolving. The new multi-entry-terminal fixture was run against the pre-fix normalizer and failed with ERR_INVALID_ARG_TYPE; it passes after the guard. Medium: added harness rows for no selector, both selectors, malformed issue identifiers, absent worktrees, primary worktrees, missing linked issues, branch deletion failure, and a branch that remains after a reported deletion. The branch cases use isolated real Git reference-transaction hooks. Low: removed the unused realpathSync import. No inline review threads are available to reply to or resolve; the findings are top-level review sections. |
There was a problem hiding this comment.
Review Complete
Scope: PR #619 in thomasluizon/orbit-ui-mobile
Recommendation: APPROVE
| Severity | Count |
|---|---|
| Critical (incl. |
0 |
| High | 0 |
| Medium | 2 |
| Low / Info | 1 |
Summary
Adds tools/teardown-worktree.mjs (removes a completed Orca worktree/terminals/branch only after clean-tree, tree-present-in-target, Linear-Done, and terminals-idle all pass, verifying removal from the filesystem and git worktree list rather than trusting Orca's reply), wires it into /orchestrate's Advance step, and adds 21 hermetic test cases. Diff is tools/ + .claude/skills/ only — no apps/*, packages/shared, or orbit-api touched, so all five review subagents (parity, i18n, contract, security, design) are N/A. Hand-traced the safety-critical tree-presence logic (squash-merge, regular-merge, sibling-target-advance, partial-mismatch scenarios) and it fails closed in every case.
Findings (Medium)
--baseoverride flag has no dedicated test case —tools/teardown-worktree.mjs:93. Every fixture intest-tools.mjs'steardownWorktreeRecordsetsbaseRef: "main"and no case ever passes--baseon the CLI, sorequestedBasetaking precedence overworktree.baseRefis untested. Fix: add one case passing--base <ref>differing from the fixture default.worktree.branchgit-rev-parse fallback has no dedicated test case —tools/teardown-worktree.mjs:92. Every fixture suppliesbranch, so thegit rev-parse --abbrev-ref HEADfallback for an Orca payload missing.branchnever executes in the suite. Fix: add one case withbranchomitted from the stubbed record.
Both are per rubric dimension 15 ("a new decision path… needs its own case"), non-blocking.
Validation (read from PR's actual CI, not re-run locally)
Lint, Type Check, Unit Tests, Build, Harness Execution (both matrix legs), Cross-Platform Parity, Contract Drift, Expo SDK Pin, Suppressions Ratchet, Skill and Agent Frontmatter, Dash Ban, Copy Register, Design Token Guard, Architecture map drift — all SUCCESS. This matches the PR body's claimed gate output (ORBIT TOOLS GATE OK, ORBIT HOOK PARITY OK, lint 3/3, type-check 3/3).
What's good
- Verification-not-trust design (filesystem +
git worktree listover Orca's response) directly traces to a measured Orca runtime-disconnect failure, and is exercised end-to-end by a test that deletes a real fixture directory and checks the tool still reports success correctly. - Squash-merge and regular-merge both handled by the same tree-presence check; a partial mismatch on any one of the branch's own changed paths correctly refuses teardown rather than averaging it away.
- All git/orca subprocess calls use
execFileSync/spawnSyncwith argument arrays — no shell interpolation, no command-injection surface for branch/path values sourced from Orca/Linear data. - Coverage (21 cases +
INVALID_INPUTrow) lands in the same PR pertools/CONVENTIONS.md, and theSKILL.mdprose update for/orchestrateaccurately reflects the tool's real refusal/exit-code contract.
Deferred: DESIGN.md(#8), Parity(#9), i18n(#10), Contract drift(#11), Backend hard rules(#13), FEATURES.md(#14) — all N/A, no surface touched. All 4 changed files got a verdict. Local harness execution deferred to CI's own Harness Execution SUCCESS (session sandbox blocked running it directly).
thomasluizon
deleted the
feature/orb-124-close-a-worktree-automatically-when-its
branch
July 27, 2026 22:27
thomasluizon
added a commit
that referenced
this pull request
Jul 27, 2026
…fort high (#620) * chore(orchestrator): flip the codex worker model to gpt-5.6-sol Thomas's call on 2026-07-27. Reverses the "Sol never as the routine executor" line in the 2026-07-26 ADR, which chose Terra medium on throughput grounds (~75-450 msgs per 5h window on Sol vs ~1.3x that on Terra) and named "PR-green-on-first-try vs retries per ticket" as the metric that would settle it. The ORB-124 run supplied that metric: Terra medium took two consecutive CHANGES_REQUESTED rounds on a 3-point harness ticket (PR #619), shipping an always-false `x && !x` validation branch, a whole-tree `git diff` that refuses every worktree in a wave except the first to merge, an unused import, and six reachable refusal branches with no test row. Each round costs a full worker launch plus a review pass, so the cheaper tier was not cheaper here. Model id verified live against codex's own models cache: gpt-5.6-sol, "Latest frontier agentic coding model". Reasoning effort stays medium. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014zWQV8N53p7aUvvZL7rAe1 * chore(orchestrator): raise the codex worker reasoning effort to high Sol's own default reasoning level is low, not medium, and its model card advises starting low and turning it up for harder jobs. Ticket execution is the harder job: the two review rounds Terra medium lost on ORB-124 were care failures (dead code, an unused import, an unguarded resolve(), six untested refusal branches), which is what reasoning depth buys. Rejected the tiers above it deliberately. `ultra` is "maximum reasoning with automatic task delegation", and worker contract clause 10 already governs when a worker fans work out to subagents, so an engine-level auto-delegation would compete with it. `max` is priced for problems harder than a 3-point ticket. Both stay available for per-ticket routing once ORB-89 lands. Supported levels read live from codex's models cache: low, medium, high, xhigh, max, ultra. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014zWQV8N53p7aUvvZL7rAe1 --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
teardown-worktree.mjs, which checks clean state, branch-owned content presence, live Done state, and terminal repaint activity before teardown.git worktree listresults./orchestrateand hermetic decision-path coverage for success, refusals, squash merges, and Orca runtime disconnects.Closes ORB-124.
Decisions taken unattended
tools/lib/tui-repaint.mjsexport directly, matching the ticket reconciliation note;launch-worker.mjsremains unchanged.npm run lintas the configured lint entry point because ESLint 9 has no rooteslint.config.*for a directnpx eslint tools .claude --ext .mjs,.jsinvocation.Gate output