chore(lessons): promote both pending lessons, one as a gate and one as a rule - #597
Conversation
…s a rule Clears `.claude/pending-lessons.md` before Phase 7. Lesson 1 (checkable) becomes a gate in the merge sweeps. `gh pr update-branch` rewrites the head SHA and re-triggers the `review` check, but `reviewDecision` is PR-level and keeps the pre-update APPROVED while that re-review runs, so both sweeps could squash-merge on a snapshot that predated the head they were merging. That is how a HIGH backend-contract finding reached main on orbit-api#403 and the fix landed on the orphaned head branch instead. Both scripts now: - discover whether the repo runs `.github/workflows/claude-review.yml`, and if it does, refuse every merge path until the `review` check on the CURRENT head SHA is terminal, then re-read reviewDecision; - record each merged PR's head branch and re-check it at end of sweep, printing ORPHANED-HEAD and exiting 1 when a post-merge push re-created it; - gain `--help`, documented exit codes (0/1/2), and a timeout line that names the actual blocker instead of a generic "waiting for CLEAN". `merge-sweep.sh` carried the same race in a stronger form (it never waited for checks at all), so it is fixed in the same pass rather than left as known-broken. Lesson 2 (judgment) becomes standing guidance in the orchestrate skill's delegation-discipline section, which is the one place in the repo that carries a delegation contract template and a wait-on-CI loop. Both halves land: subagents poll in the foreground inside their own turn, and the parent treats a "standing by / monitor armed" completion as a nudge trigger, not as progress.
|
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.
Code Review: PR #597
Scope: PR #597 in thomasluizon/orbit-ui-mobile (chore/promote-pending-lessons → main)
Recommendation: NEEDS WORK
Summary
The PR graduates two pending lessons: tools/merge-sweep.sh and tools/merge-sweep-cov.sh gain a
review-staleness guard (blocking merge until the review check on the current head SHA settles,
per the orbit-api #403 incident) and an end-of-sweep orphaned-head-branch scan; --help/exit-code
support is added per tools/CONVENTIONS.md; the second lesson lands as prose in
.claude/skills/orchestrate/SKILL.md. .claude/pending-lessons.md and tools/README.md are
updated in step, and the dash baseline shrinks correctly (verified: zero em dashes remain in either
script). The mechanism is sound in the case the PR's own test transcript exercises, but the
review_required gate that turns the whole safety mechanism on is itself computed by an
unpaginated, error-swallowed gh api call that silently reverts to the pre-patch (unsafe) behavior
on failure — a correctness gap in the exact code path this PR exists to harden.
Findings
Critical
None.
High
[HIGH] review-staleness guard silently disables itself on an unpaginated/failed workflow lookup
· dimension: 1. Correctness (this is the PR's own stated guarantee failing silently)
· location: orbit-ui-mobile/tools/merge-sweep.sh:55 (identical logic at orbit-ui-mobile/tools/merge-sweep-cov.sh:62)
· issue: `review_required` — the single switch that turns the new review-staleness guard on — is
set from `gh api "repos/$repo/actions/workflows" --jq '.workflows[].path' 2>/dev/null | grep -qx
"$REVIEW_WORKFLOW_PATH"`. `gh api` does not auto-paginate (opt-in via `--paginate`), and GitHub's
"List repository workflows" endpoint defaults to `per_page=30`; a repo with more than 30 workflow
files can return `claude-review.yml` on page 2+, which this call never fetches. Separately,
`2>/dev/null` swallows ANY failure of the call (auth, rate limit, transient network) with no
fallback. In every one of these cases `review_required` stays empty (not an error, just silently
unset) — verified end-to-end: it is read exactly once, at merge-sweep.sh:96 / merge-sweep-cov.sh:129,
and when empty, `review_stale` is never set, so the loop merges as soon as `ms` is CLEAN/UNSTABLE
and `rev=APPROVED` with zero wait for the current SHA's `review` check. That is exactly the
pre-patch behavior that caused orbit-api #403 (a HIGH backend-contract finding merged and deployed
before the re-review landed) — the one incident this PR exists to prevent.
· risk: Currently latent for orbit-ui-mobile (20 workflow files today, confirmed via `ls
.github/workflows`, under the 30-item page default), but silent and undetectable when it does
trip — no error line, no warning, just a merge that shouldn't have happened. It also reintroduces
the original bug on ANY transient `gh api` failure today, regardless of workflow count, since
fail-open (guard skipped) is the wrong default direction for a safety gate — it should fail closed
(assume the guard is required) when the check itself can't be confirmed.
· fix: Add `--paginate` to the `gh api "repos/$repo/actions/workflows"` call (or `-F per_page=100`,
since workflow counts realistically stay under 100) so the lookup can't silently truncate. Also
invert the failure direction: default `review_required=1` and only clear it when the `gh api` call
positively succeeds AND positively confirms the workflow file is absent, rather than defaulting to
"not required" and only setting it on a confirmed hit. That makes an API hiccup fail toward extra
waiting (safe, at worst a slower sweep) instead of toward skipping the guard (unsafe).
· reference: CLAUDE.md rule 8 (error handling at boundaries; a trust-boundary call like this needs a
defined failure behavior, not a silent swallow); rubric dimension 1 (Correctness — the diff's own
stated guarantee).
Medium
[MEDIUM] orphaned-head-branch check can false-positive on a delayed or failed branch delete
· dimension: 1. Correctness / 3. SOLID (missing edge case in new logic)
· location: orbit-ui-mobile/tools/merge-sweep.sh:119-128; orbit-ui-mobile/tools/merge-sweep-cov.sh:160-169
· issue: The end-of-sweep scan treats "branch exists" as proof the branch was "re-created after the
merge, so its commits are NOT on main." It never compares the branch's current tip SHA to the SHA
that was actually merged — it only checks existence via `gh api repos/$repo/branches/$branch`. A
branch can still exist after a successful `--squash --delete-branch` merge for reasons that have
nothing to do with a post-merge push: `--delete-branch`'s own delete step failing separately from
the merge (branch protection on the head branch, an insufficient token scope, a transient error)
and the deletion itself lagging behind the read. That race is sharpest for the single-PR
invocation shown in the PR's own test transcript (`merge-sweep-cov.sh <repo> 403`), where the
orphan check runs immediately after that PR's own merge with no other PRs' processing time to
absorb the delay.
· risk: A benign "branch didn't get deleted" is reported as "ORPHANED-HEAD ... commits are NOT on
main" (a factually wrong claim — the squash-merge to main already happened) and the script exits 1,
which any caller (the nightly sweep, a human running it ad hoc) will read as "an incident
happened," triggering unnecessary investigation or fix-forward action.
· fix: Record the merged PR's `headRefOid` (already available from `gate()`'s SHA field in
merge-sweep-cov.sh; add it to `mstate()` in merge-sweep.sh) alongside the branch name, and at
scan time compare the still-existing branch's current tip SHA to that recorded SHA. Only flag
ORPHANED-HEAD when the tip SHA differs from (or is ahead of) the merged SHA — proving a new commit
landed on the branch post-merge — not merely when the branch still exists.
· reference: rubric dimension 1 (Correctness — boundary condition: delete-branch failure vs.
genuine recreation); tools/CONVENTIONS.md "Gate tools" (a verdict should be computed from hard
evidence, not a proxy that a benign condition can also satisfy).
Low / Info
None posted (signal gate: style-only observations excluded).
Subagents
| Agent | Verdict |
|---|---|
| parity-checker | N/A — no apps/web/** or apps/mobile/** file changed |
| i18n-syncer | N/A — no user-facing strings or packages/shared/src/i18n/* changed |
| contract-aligner | N/A — no orbit-api or packages/shared/src/types/*/endpoints.ts change |
| security-reviewer | N/A — no orbit-api code changed |
| design-reviewer | N/A — no apps/*/orbit-landing-page UI file changed |
Validation
| Check | Result |
|---|---|
| Lint | N/A — this review session's sandbox blocks executing repo scripts/shell commands beyond git/gh; not run |
| Type check | N/A — same sandbox restriction |
| Tests | N/A — same sandbox restriction |
| Build (api) | N/A — diff does not touch orbit-api |
Deferred — N/A dimensions & files not verdicted
- DESIGN.md / AI-slop (#8): N/A — no
apps/*UI files in the diff. - Parity (#9): N/A — no
apps/web/**orapps/mobile/**files in the diff. - i18n (#10): N/A — no i18n JSON or user-facing string changes.
- Contract drift + backward-compat (#11): N/A — no
packages/shared/src/types/*,endpoints.ts,
ororbit-apiDTO changes. - Security (#12, backend categories): N/A — no
orbit-apicode changed. Frontend-security
categories (XSS, auth-state leakage) also N/A — no frontend code in this diff. - Backend hard rules (#13): N/A — no
orbit-apichanged. - FEATURES.md parity (#14): N/A — no user-facing feature surface change (internal tooling only).
- Validation (Phase 7): not executed — this review session's tool sandbox declined to run shell
scripts/node/npmcommands (repeated "requires approval" on every attempt beyondgit/gh
read commands); the PR's own verification block (node tools/check-frontmatter.mjs,
node .claude/hooks/test-hooks.mjs,node tools/check-dashes.mjs --check-baseline,bash -non
both scripts) was not independently re-run. Manually verified instead: grepped both scripts for
the em-dash character and confirmed zero remain, matching thedash-baseline.jsondeletions;
manually traced both scripts' bash control flow line-by-line for syntax and logic soundness in
lieu ofbash -n. - All 6 changed files received a verdict; nothing was skipped for size or scope.
What's good
- The core review-staleness mechanism (poll the CURRENT head SHA's
reviewcheck to a terminal
state before trustingreviewDecision, sincereviewDecisionis PR-level and survives an
update-branch) correctly targets the actual root cause of orbit-api #403, and the fix is applied
to both sibling scripts (merge-sweep.shandmerge-sweep-cov.sh) rather than leaving one as a
known-broken twin, matching the PR body's own reasoning. --help, documented exit codes (0/1/2), and theusage()-on-bad-args path bring both
scripts up totools/CONVENTIONS.md's contract, which they previously lacked.- The orphaned-head-branch scan (existence check aside — see Medium finding) is a reasonable
secondary signal for detecting a past occurrence of the original bug class. .claude/pending-lessons.mdcorrectly clears the queue and records where each lesson landed with
dates, andtools/README.mdcatalog rows were updated in the same PR to describe the new
behavior — no stale documentation left behind.dash-baseline.jsonshrinks by exactly the count of em dashes actually removed (verified by grep
against both files: zero remain).
Recommendation
Fix the High finding before merge: make the review_required detection resilient to pagination and
API failure, and fail closed (assume the guard is needed) rather than fail open when the workflow
lookup can't be confirmed — this is the exact silent-failure shape the PR itself exists to close.
The Medium finding (SHA-based orphan detection instead of bare existence) is worth fixing in the
same pass since it touches the same block of new code, but would not block merge on its own.
…ot existence Addresses both review findings on #597. HIGH: `review_required`, the single switch enabling the review-staleness guard, was computed from an unpaginated, error-swallowed `gh api ... /actions/workflows` call and defaulted to OFF. `gh api` does not auto-paginate (the endpoint defaults to per_page=30), and `2>/dev/null` hid auth, rate-limit and network failures, so either condition silently restored the exact pre-patch merge behaviour the guard exists to prevent. Now `--paginate`, and the default inverts: start required, and clear it only when the lookup SUCCEEDS and positively shows no claude-review.yml. A failed lookup warns on stderr and keeps waiting. This is the same rule as the locked "Fail-closed the completion gate: a verifier error is not a clean pass" decision, applied one level up, to the guard's own precondition check. MEDIUM: the orphaned-head scan treated "branch still exists" as proof of a post-merge push, but `--delete-branch` can fail or lag independently of the merge, which would report a benign leftover branch as an incident and exit 1. Both scripts now record the head SHA that was actually merged and flag ORPHANED-HEAD only when the surviving branch's tip has MOVED past it. `mstate()` in merge-sweep.sh gained the headRefOid field it needed for this.
There was a problem hiding this comment.
Code Review: PR #597
Scope: PR #597 in orbit-ui-mobile — chore(lessons): promote both pending lessons, one as a gate and one as a rule
Recommendation: APPROVE
Summary
This PR hardens tools/merge-sweep.sh and tools/merge-sweep-cov.sh against the review-staleness race that let a HIGH backend-contract finding reach main on orbit-api#403 (reviewDecision is PR-level and survives an update-branch push, so a sweep could merge on a stale pre-update APPROVED while the re-triggered review check was still running on the new head SHA). Both scripts now block every merge path — including the coverage-only admin override — until the review check on the current head SHA settles, fail closed on an unreadable workflow list (paginated gh api ... --paginate, default-required unless the lookup positively proves the workflow absent), and scan merged PRs' surviving head branches for a tip that moved past the merged SHA (not mere existence, which would false-positive on a slow --delete-branch). It also gains --help, documented exit codes, and graduates two pending-lessons.md entries. Diff is scoped entirely to .claude/ and tools/; no apps/*, packages/shared, or orbit-api files are touched, so the platform-parity/i18n/contract/design/backend-security dimensions are all N/A by gate.
I read both scripts end to end and traced the guard's control flow against the race it targets: the review_required fail-closed default, the per-iteration re-fetch of reviewDecision alongside the review check's settle state (so a flip to CHANGES_REQUESTED is picked up in the same gh pr view payload that observes the check settling), the coverage-admin path sitting after the staleness gate in merge-sweep-cov.sh, and the SHA-based (not existence-based) orphan check. All of it holds up, including the two edge cases the PR's own proof output claims (case 4b: failed workflow lookup keeps the guard on; case 5b: a branch that merely survived --delete-branch with an unmoved tip is not reported). The two findings this PR's own body lists as already fixed (unpaginated/error-swallowed workflow lookup defaulting open; existence- vs SHA-based orphan check) are present and correctly implemented in the diff as pushed — nothing further to flag there.
Findings
Critical
None.
High
None.
Medium
[MEDIUM] merge-sweep.sh has no fast-path SKIP when reviewDecision is not APPROVED, unlike its sibling
· dimension: SOLID / clean architecture (rule 10, cross-script consistency) — pattern inconsistency
· location: tools/merge-sweep.sh:82-122
· issue: merge-sweep-cov.sh's loop (tools/merge-sweep-cov.sh:112-116) checks `[ "$rev" != "APPROVED" ]`
first and SKIPs immediately with an explicit `review=$rev` message. merge-sweep.sh has no equivalent
early exit: once the review check settles to CHANGES_REQUESTED, `review_stale` clears but the merge
condition at line 107 still requires `rev = APPROVED`, so a non-approved PR just falls through to
`sleep 20` and loops for up to the full ~50 iterations before printing a generic
"SKIP #$n (timeout: never reached a mergeable state (ms=... rev=CHANGES_REQUESTED))".
· risk: not a correctness bug (it still correctly refuses to merge), but every rejected/changes-requested
PR in a batch costs the sweep up to ~17 minutes of pointless polling instead of exiting immediately,
and the eventual message reads as a generic timeout rather than the clear, fast `SKIP #$n review=$rev`
its sibling script gives. This is exactly the kind of drift between the two scripts that
`tools/README.md`'s "like merge-sweep.sh, but…" framing implies shouldn't exist.
· fix: add the same early check used in merge-sweep-cov.sh right after the `failed`/`DIRTY` checks:
`if [ "$rev" != "APPROVED" ]; then echo "SKIP #$n review=$rev"; done_pr=1; break; fi`
· reference: CLAUDE.md rule 10 (DRY/consistency at the right level); tools/CONVENTIONS.md ("one clear
purpose per script" — the two sweep scripts are meant to behave identically apart from the
Sonar-coverage carve-out)
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, and only one repo changed |
| security-reviewer | N/A — no orbit-api code changed |
| design-reviewer | N/A — no apps/web, apps/mobile, or orbit-landing-page/src UI file changed |
Validation
| Check | Result |
|---|---|
| Lint | N/A — CI wrapper skips /validate; Build/Unit Tests/SonarCloud run as separate required checks |
| Type check | N/A — same |
| Tests | N/A — same |
| Build (api) | N/A — orbit-api not touched |
Manually verified: bash -n on both scripts (syntax), field-order alignment between each mstate/gate
JS emitter and its corresponding bash read, and that the --paginate/fail-closed workflow lookup and
SHA-based (not existence-based) orphan check the PR body claims are actually present in the diff as
pushed.
Deferred — N/A dimensions & files not verdicted
- Parity / i18n / contract-drift / backend-security / backend-hard-rules / FEATURES.md parity: all N/A,
gated out because the diff touches only.claude/andtools/(noapps/*,packages/shared,
orbit-api, or user-facing feature surface). - Comment-policy dimension (#4): scoped by the rubric to
no-comments.cjs(TS/JS) andORBIT0001(C#);
bash isn't gated by either, so the new inline race-condition comments in the two scripts were read for
quality but not held to the JSDoc/WHY-URL lint bar. .claude/pending-lessons.md: the newly Graduated "2026-07-24 background subagents idle on phantom
background waiters" entry never appears as a## Pendingqueue row anywhere in this file's history —
only the "2026-07-14 sweep-merge" entry was actually staged there before this PR. This is a bookkeeping
observation on a prose changelog, not a code defect, so it is not raised as a Low finding per the
Signal gate; noted here for the author's awareness.- All six changed files (
.claude/pending-lessons.md,.claude/skills/orchestrate/SKILL.md,
tools/README.md,tools/dash-baseline.json,tools/merge-sweep-cov.sh,tools/merge-sweep.sh) were
read and verdicted; nothing left uncovered.
What's good
- The core race fix is correct and precisely targeted: the guard re-fetches
reviewDecisionand the
reviewcheck's settle state together in onegh pr viewcall every poll, so a flip to
CHANGES_REQUESTEDis caught in the same read that observes the check going terminal — no window for
a staleAPPROVEDto slip through, including on the coverage-admin override path.
(tools/merge-sweep-cov.sh:133-139,tools/merge-sweep.sh:98-106) - Fail-closed default on the workflow lookup (
review_required=1unless a successful lookup positively
shows noclaude-review.yml) correctly means an auth/rate-limit hiccup makes the sweep slower, never
less safe. (tools/merge-sweep.sh:55-62,tools/merge-sweep-cov.sh:62-69) - The orphan-head scan is SHA-based, not existence-based, so a branch that merely outlives
--delete-branch's eventual-consistency lag is correctly not reported — only a tip that actually moved
past the merged commit is. (tools/merge-sweep.sh:125-139,tools/merge-sweep-cov.sh:166-180) - New
--help/usage text, documented exit codes, and thetools/README.mdcatalog rows are accurate to
the new behavior and matchtools/CONVENTIONS.md's contract. - The PR body's own reproduction table (stub-
ghcases 1-7) is a genuinely good piece of evidence for a
concurrency fix that's otherwise hard to test.
Recommendation
Approve as-is. The one Medium finding (merge-sweep.sh's missing fast-path SKIP on a non-approved PR) is a
minor efficiency/consistency gap, not a safety issue — the script still correctly refuses to merge, it
just takes longer to say so. Fine to land now and pick up in a follow-up if the author wants the two
scripts to match exactly.
There was a problem hiding this comment.
Code Review: PR #597
Recommendation: APPROVE
Summary
Promotes both queued entries in .claude/pending-lessons.md: a checkable lesson that becomes a fail-closed merge guard in tools/merge-sweep.sh / tools/merge-sweep-cov.sh (block every merge path until the review check on the current head SHA settles, then re-read reviewDecision, plus an end-of-sweep scan for orphaned head branches), and a judgment lesson landed as prose in .claude/skills/orchestrate/SKILL.md. Pure tooling + docs diff, no apps/*, orbit-api, or packages/shared surface touched. The guard logic was traced step by step against every branch (APPROVED/CHANGES_REQUESTED, BEHIND, DIRTY, Sonar FAILURE, workflow-lookup failure, orphaned head) and is sound; one Medium defense-in-depth gap survived in the new orphan-scan error handling.
Findings
Critical
None.
High
None.
Medium
Orphan-scan swallows non-404 gh api failures as "no orphan", the same fail-open pattern this PR eliminates elsewhere
- location:
tools/merge-sweep.sh:134,tools/merge-sweep-cov.sh:175 - issue:
tip=$(gh api "repos/$repo/branches/$branch" --jq .commit.sha 2>/dev/null) || continuetreats every non-zerogh apiexit identically. A confirmed 404 (branch genuinely deleted by--delete-branch, the expected/benign case) and a transient failure (rate limit, network blip, auth hiccup) both silentlycontinuewith no diagnostic, so the run reports "clean" (exit 0, noORPHANED-HEADline) whether or not that PR's branch was actually verified. - risk: this is the identical fail-open shape the PR's own "Why the precondition check itself fails closed" section calls out and fixes for the workflow-lookup a few lines above in the same diff — the fix wasn't carried to this second lookup. If the orphan check silently no-ops during a transient error, a real orphaned head (the exact HIGH-severity failure mode from orbit-api#403 that this PR exists to catch) goes unreported and is never re-checked later, since
merged_headsis per-invocation, in-memory state. - fix: distinguish "confirmed deleted" from "could not verify" (e.g. check for a literal
404via--include), and only silently skip on a confirmed 404. On any other failure, print aWARN: could not verify branch <branch> for #<pr>; orphan status unknownto stderr, mirroring the existingWARN: could not list ... workflowspattern used one lookup above. - reference: CLAUDE.md rule 1 (root-cause consistency within the same diff); rubric dimension 3 (defense-in-depth gap, Medium).
Low / Info
None posted (signal gate).
Subagents
All gated N/A — the diff touches no apps/web/**, apps/mobile/**, orbit-api, packages/shared/src/types/*, or i18n JSON. Frontend-security categories reviewed inline instead: no injection surface (repo/pr only ever passed as gh argv, never eval'd); the two check/workflow-name interpolations into the inline node -e scripts are hardcoded constants, not user input.
Validation
Lint / type check / tests / build: N/A — diff is .sh + .md + .json only, no ESLint/Roslyn/tsc-covered surface changed, and this repo has no CI gate for shell syntax. Dash-baseline removal independently verified (git show <ref>:<file> | grep -c '—' = 0 for every changed prose file). Shell syntax verified by manual trace of every case/esac, if/fi, for/done, and function brace, rather than an automated bash -n run (blocked in this review session's sandbox).
Deferred
Dimensions 8/9/10/11/13/14 (DESIGN.md, parity, i18n, contract drift, backend hard rules, FEATURES.md) — N/A by gate, no matching surface in the diff. Phase 5 backward-compat guard — N/A, no shared-type/DTO hunks to classify. All 6 changed files received a verdict.
What's good
The review-staleness guard is genuinely correct end to end (traced through approved-settle, changes-requested-settle, workflow-absent-repo, and workflow-lookup-failure branches). The workflow-lookup fail-closed fix (--paginate, inverted default, WARN on failure) is exactly right. Both scripts gained --help and documented exit codes per tools/CONVENTIONS.md. The pr^branch^sha orphan-tracking encoding is small and correctly parsed.
Recommendation
Approve as-is. The one Medium finding is a real but narrow defense-in-depth gap in a post-merge, best-effort detection pass — worth a fast follow-up, not a blocker.
Addresses the Medium finding on the approving review of #597: the orphan scan's `gh api repos/<repo>/branches/<b> || continue` treated a confirmed 404 (the expected case, the branch really was deleted) and a transient failure (rate limit, network, auth) identically, so a run could report exit 0 with no ORPHANED-HEAD line without having verified anything. That is the same fail-open shape this PR removes from the workflow lookup twenty lines above. Both scripts now resolve the ref through GraphQL, which exits 0 with an EMPTY oid for a deleted branch. A non-zero exit is therefore unambiguously "could not verify": it warns on stderr and exits 3, a distinct code from 1 (a real orphaned head). Silence now means verified, per the locked "a verifier error is not a clean pass" decision.
There was a problem hiding this comment.
Code Review: PR #597
Recommendation: APPROVE
Summary
Tooling/process PR: promotes both .claude/pending-lessons.md entries (one as a gate, one as a rule), hardens tools/merge-sweep.sh and tools/merge-sweep-cov.sh against the orbit-api #403 race (update-branch re-triggers the review check while the PR-level reviewDecision still reports the stale pre-update value), adds an end-of-sweep orphaned-head-branch scan proven by SHA rather than existence, and lands "waiting is foreground work" guidance in .claude/skills/orchestrate/SKILL.md. No apps/*, packages/shared, or orbit-api files touched. I traced both scripts' control flow line-by-line against the PR's embedded 7-case stub-harness proof and independently re-derived the same behavior; the review-staleness guard correctly gates before the coverage-only admin-override path too, not just the plain-merge path — the one place a bypass could have slipped back in.
Findings
Critical: None. High: None. Medium: None. Low/Info: none posted per the rubric's signal gate.
Subagents (all N/A — diff touches only .claude/ and tools/)
parity-checker N/A · i18n-syncer N/A · contract-aligner N/A · security-reviewer N/A · design-reviewer N/A
Validation
No lintable/testable app surface changed. bash -n, check-frontmatter.mjs, and check-dashes.mjs --check-baseline were not re-executed in this sandboxed session (execution required an approval this environment could not grant); the PR body's own verification transcript reports all green, and I independently confirmed via gh pr diff that no added line introduces an em/en dash, matching the dash-baseline.json shrink.
Deferred
Dimensions 8/9/10/11/13/14 (DESIGN.md, parity, i18n, contract drift, backend hard rules, FEATURES.md) are N/A — the diff never touches their gated surface. Lint/type-check/test execution not independently re-run this session; PR's own transcript is the evidence of record. All 6 changed files received a verdict; nothing skipped for size.
What's good
- Review-staleness guard is ordered before the Sonar coverage-only
--adminoverride inmerge-sweep-cov.sh, so the one branch-protection-bypassing path isn't a back door around the new guard. - Fail-closed by construction: a failed/unpaginated workflow lookup now defaults to "review required" instead of silently disabling the guard (closes the hole the first draft shipped with, per the PR's own case 4b).
- Orphan scan proves drift by comparing the recorded merged SHA against the branch's current tip via GraphQL (empty oid = confirmed-deleted, non-zero exit = confirmed-unknown), not branch existence alone.
--help, documented exit codes (0/1/2/3), cwd-independent and non-interactive pertools/CONVENTIONS.md.- Watch-item, not a finding:
merge-sweep.sh/merge-sweep-cov.shnow duplicate a fair amount of new logic (workflow lookup,branch_tip(), orphan-scan loop) verbatim. Per CLAUDE.md rule 6 ("extract on the third real use"), two scripts don't clear the bar yet for a shared helper — worth revisiting if a third sweep script appears.
Recommendation
Merge as-is.
There was a problem hiding this comment.
Code Review: PR #597
Scope: PR #597 in thomasluizon/orbit-ui-mobile
Recommendation: APPROVE
Summary
This PR promotes both queued .claude/pending-lessons.md entries. The checkable lesson becomes a
fail-closed review-staleness + orphaned-head guard added to tools/merge-sweep.sh and
tools/merge-sweep-cov.sh; the judgment lesson becomes standing guidance in
.claude/skills/orchestrate/SKILL.md. The diff is entirely tooling/docs (no apps/*,
packages/shared, or orbit-api files), so the design, parity, i18n, contract-drift, security,
backend-hard-rules, and FEATURES.md dimensions are all out of surface. The two shell scripts
(safety-critical, since they auto-merge PRs to main) got the closest read; the added guard logic
is sound and does fix the documented #403 race, with one concrete edge-case gap below.
Findings
Critical
None.
High
None.
Medium
[MEDIUM] Branch-name lookup for the orphan-scan can fail silently and skip verification without counting it as unverified
· dimension: 1. Correctness / 5. No-workaround (error handling, CLAUDE.md rule 8)
· location: orbit-ui-mobile/tools/merge-sweep.sh:109 and orbit-ui-mobile/tools/merge-sweep-cov.sh:97 (squash_merge)
· issue: Right before merging, both scripts fetch the branch name with a second, independent gh pr view … --json headRefName --jq .headRefName 2>/dev/null call. If that call fails (network blip, secondary rate limit) it silently yields an empty string; the merge still proceeds and merged_heads gets an entry with an empty branch. In the orphan-scan loop, [ -n "$branch" ] || continue then skips that entry entirely — it is not added to unverified and never printed as a WARN, so it neither shows up in the report nor trips exit code 3.
· risk: This is the exact failure mode both scripts' own header comments and exit-code contract explicitly promise never happens ("unknown is not a clean pass", exit 3 documented for "a head branch could not be verified"). A transient API hiccup at merge time silently drops that one PR out of the orphan check, and the sweep still reports SWEEP-DONE / exit 0 as if every merged head had been verified clean.
· fix: Don't make a second network call at all — add headRefName to the existing gate/mstate gh pr view --json ... call that already runs every poll iteration (it already succeeded, since that's the data driving the merge decision), and use that value directly in squash_merge/the merge branch. That removes the extra failure surface entirely. If a separate call is kept, at minimum treat an empty branch the same as a failed branch_tip lookup: warn on stderr and increment unverified instead of continue.
· reference: CLAUDE.md rule 8 (error handling — never swallow errors silently); tools/CONVENTIONS.md "Gate tools" (a gate's verdict must be computed from artifacts on disk, never silently assumed clean).
Low / Info
None.
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 or orbit-api DTOs changed |
| security-reviewer | N/A — no orbit-api code changed |
| design-reviewer | N/A — no apps/* or orbit-landing-page UI files changed |
Validation
| Check | Result |
|---|---|
| Lint | N/A — this review session's sandbox blocks node/npm execution; diff has no TS/JS/C# files to lint |
| Type check | N/A — same reason; no typed source changed |
| Tests | N/A — same reason; the PR's own proof transcript (stub-gh harness, scratchpad-only) is in the PR body |
| Build (api) | N/A — orbit-api not touched |
Deferred — N/A dimensions & files not verdicted
- 8. DESIGN.md / AI-slop — N/A, no
apps/*UI files touched. - 9. Parity — N/A, no
apps/web/**/apps/mobile/**files touched. - 10. i18n — N/A, no user-facing strings or locale JSON touched.
- 11. Contract drift + backward-compat — N/A, no
packages/shared/src/types/*or orbit-api DTOs touched. - 12. Security — N/A, no
orbit-api, noapps/*trust-boundary code touched; the shell scripts run only againstgh/GitHub's API with no new secret handling. - 13. Backend hard rules — N/A, orbit-api not touched.
- 14. FEATURES.md parity — N/A, purely internal tooling/process change, no user-facing feature surface.
- Phase 7 Validate — not executable in this review sandbox (Bash calls to
node/npmrequire an approval this session does not have); the PR body's own verification transcript (check-frontmatter.mjs,test-hooks.mjs,check-dashes.mjs --check-baseline,bash -non both scripts) is the available evidence and was not independently re-run here. - Every changed file (
tools/merge-sweep.sh,tools/merge-sweep-cov.sh,tools/README.md,
tools/dash-baseline.json,.claude/pending-lessons.md,.claude/skills/orchestrate/SKILL.md)
received a verdict above; nothing else deferred.
What's good
- The core fix is correct and well-targeted: gating every merge path on the
reviewcheck for the
current head SHA reaching a terminal state before re-readingreviewDecisiongenuinely closes
the #403 race (a stale PR-levelreviewDecisionsurviving a re-triggered check). - The workflow-lookup precondition is properly fail-closed now:
--paginatefixes the page-30 cap
bug, and a failed lookup defaultsreview_required=1(warns on stderr) rather than silently
disabling the guard — this is the right shape for a gate whose own precondition check can fail. - The orphaned-head scan's "moved past" comparison (vs. "branch merely survived deletion") correctly
avoids the false-positive the PR's own changelog says a prior draft had, and separates "confirmed
orphaned" (exit 1) from "could not verify" (exit 3) instead of collapsing unknown into clean. --help, explicit usage, and documented exit codes bring both scripts in line with
tools/CONVENTIONS.md.- The judgment-lesson landing in
orchestrate/SKILL.md's existing "Delegation discipline" section
(rather than the always-loaded.claude/rules/core.md) matches the stated rationale: it's read
only when delegating, not paid for on every turn.
Recommendation
Approve as-is. The one Medium finding is a narrow, well-scoped follow-up (fold headRefName into
the already-polled JSON instead of a second lookup) — worth fixing soon since it's cheap and removes
a real gap in the exact invariant this PR exists to establish, but it does not block merge.
There was a problem hiding this comment.
Code Review: PR #597
Scope: PR #597 in thomasluizon/orbit-ui-mobile (chore/promote-pending-lessons → main), head bdb63a7a
Recommendation: APPROVE
Summary
Pure tooling/docs diff (.claude/, tools/ only — no apps/*, packages/shared, or orbit-api). It graduates both queued .claude/pending-lessons.md entries: a fail-closed review-staleness + orphaned-head guard added to tools/merge-sweep.sh / tools/merge-sweep-cov.sh (targeting the orbit-api #403 race where reviewDecision survives an update-branch push while the re-triggered review check on the new head SHA is still running), plus prose guidance landed in .claude/skills/orchestrate/SKILL.md. This PR has already been through 7 review passes; five prior findings were fixed in later commits and are confirmed fixed in the current head (paginated + fail-closed workflow lookup, SHA-based rather than existence-based orphan detection, GraphQL-based orphan lookup that distinguishes confirmed-deleted from unknown). Two Medium findings from earlier passes were never addressed and are confirmed still present — re-raised below per the instruction to flag unresolved (not already-addressed) concerns.
Notably: at the time of this review, the PR's own reviewDecision field reads APPROVED while the review check for the current head SHA (this very review) is still IN_PROGRESS — a live instance of exactly the race this PR patches.
Findings
Critical: None.
High: None.
Medium:
tools/merge-sweep.sh:83-122— no fast-path SKIP whenreviewDecision != APPROVED, unlikemerge-sweep-cov.sh:113-117. A rejected PR just sleeps/re-polls for up to ~17 minutes before a generic timeout message instead of an immediateSKIP #$n review=$rev. Fix: add the same early check merge-sweep-cov.sh has, right after thefailed/DIRTYchecks.tools/merge-sweep.sh:109(andtools/merge-sweep-cov.sh:97insquash_merge) — the second, independentgh pr view --json headRefNamecall made right before merging can fail transiently, silently leavingbranchempty (swallowed by2>/dev/null, no exit-status check). The merge still proceeds, and the orphan-scan's[ -n "$branch" ] || continuedrops that entry without incrementingunverifiedor printing a WARN — the run can report exit 0 as if every merged head were verified clean, the exact fail-open shape this PR closes for the adjacentbranch_tip()lookup a few lines below. Fix: source the branch name from the existinggate/mstatepayload (already fetched every poll) instead of a second call; if a second call is kept, treat an empty result the same as a failedbranch_tiplookup (WARN + incrementunverified).
Both were raised in earlier review passes on this same PR and remain unaddressed in the current head; both are Medium (consistent with how this review history classified the closely analogous, already-fixed branch_tip() error-swallowing issue), so per the rubric's signal gate neither forces NEEDS WORK.
Subagents
All N/A — diff touches only .claude/ and tools/ (no apps/web, apps/mobile, orbit-api, packages/shared/src/types, or i18n JSON).
Validation
Phase 6 (/validate) skipped per instructions — CI runs Build/Unit Tests/SonarCloud separately as required checks. As of this review: Lint, Type Check, Build, Dash Ban, Copy Register, Suppressions Ratchet, Cross-Platform Parity, Contract Drift, CodeQL all SUCCESS; Unit Tests and SonarCloud Analysis were still IN_PROGRESS on the current head at review time (a prior SonarCloud run on an earlier SHA had passed cleanly, since superseded by the last merge-from-main commits). No orbit-api dimension to mark "not verifiable in CI" — the diff never touches that repo.
Deferred
Dimensions covering DESIGN.md, parity, i18n, contract drift, backend security, backend hard rules, FEATURES.md — N/A, no matching surface in the diff. All 6 changed files (.claude/pending-lessons.md, .claude/skills/orchestrate/SKILL.md, tools/README.md, tools/dash-baseline.json, tools/merge-sweep-cov.sh, tools/merge-sweep.sh) received a verdict.
What's good
- The core review-staleness fix is correct and applied to both sibling scripts: re-reads
reviewDecisionalongside thereviewcheck's settle state on the current head SHA every poll, gated before both the plain-merge and the Sonar coverage-only--adminoverride path. - Fail-closed workflow-lookup (
--paginate,review_required=1default, WARN on failure) correctly closes the earlier High finding (unpaginated + error-swallowed lookup). - Orphan-scan is SHA-based (not existence-based) via a GraphQL ref lookup that distinguishes a confirmed-deleted branch (empty oid) from an unverifiable one (non-zero exit → WARN + exit 3), correctly closing two earlier Medium findings.
--help, documented exit codes, andtools/README.mdcatalog rows matchtools/CONVENTIONS.md's contract; zero em/en dashes remain in either script, matching thedash-baseline.jsonshrink.
Recommendation
Approve as-is. The two Medium findings are real but narrow (wasted polling time; a rare transient-failure gap in a post-merge, best-effort audit), not safety-blocking — worth a fast follow-up rather than another review cycle on this already seven-times-reviewed PR.
|
1 participant
Clears
.claude/pending-lessons.mdbefore Phase 7. Both queued entries were real failures that had never been acted on; each is routed to the tier its type calls for.Lesson 1 (checkable) becomes a gate, not prose
The race is real, and the current code does not handle it.
gh pr update-branchrewrites the head SHA and re-triggers thereviewcheck, butreviewDecisionis a PR-level field that survives the push, so it keeps reporting the pre-update APPROVED for as long as the re-review runs. Sincereviewis not a required check,mergeStateStatuscan reach CLEAN with Build/Tests/Sonar green while the Claude review is still executing. The sweeps re-readreviewDecisionevery poll, but every read in that window returns the stale APPROVED, so they merge. That is how a HIGH backend-contract finding reachedmainand deployed on orbit-api#403, with the fix landing on the orphaned head branch.Both sweeps now:
.github/workflows/claude-review.yml, and unless it positively does not, block every merge path (including the coverage-only admin override) until thereviewcheck on the current head SHA is terminal, then re-readreviewDecision.statusCheckRollupis scoped to the head commit, so a run from the pre-update SHA cannot satisfy the wait. A repo that positively has no such workflow (orbit-landing-page) never waits for a check that will not run;main: printed asORPHANED-HEAD, exit 1. A branch that merely survived--delete-branchwith an unchanged tip is benign and is not reported;--help, documented exit codes (0clean /1orphaned head /2bad usage) pertools/CONVENTIONS.md, and a timeout line that names the actual blocker instead of a generic "waiting for CLEAN".Why the precondition check itself fails closed
The first draft of this guard computed
review_requiredfrom an unpaginated, error-swallowedgh api repos/<repo>/actions/workflowscall and defaulted to OFF.gh apidoes not auto-paginate and that endpoint defaults toper_page=30, soclaude-review.ymlcould sit on page 2 in a repo with more than 30 workflows; separately,2>/dev/nullhid every auth, rate-limit and network failure. Either condition silently disabled the whole guard and restored the exact merge behaviour this PR exists to prevent, with no error line.It now uses
--paginateand inverts the default: required unless a successful lookup positively shows the workflow is absent, with a stderr warning when the lookup fails. That is the locked "Fail-closed the completion gate: a verifier error is not a clean pass" decision (2026-07-18, re-confirmed 2026-07-23 after a gate-tamper hook was found failing open) applied one level up. A guard whose own precondition check fails open is the same defect, just moved.merge-sweep.shcarried the same race in a stronger form (it merged the moment the state was decidable, without waiting for checks at all), so it is fixed in the same pass rather than left as a known-broken twin of the script being hardened.Proof the guard fires
Run against a stub
ghon PATH (stub + harness live in the scratchpad, never against real PRs). Case 1 feeds both the pre-change and post-change scripts the exact#403shape:mergeStateStatus=CLEAN,reviewDecision=APPROVED,reviewcheckIN_PROGRESSon the head SHA.Review findings addressed
--paginateplus an inverted default (required unless positively absent), with a stderr warning on lookup failure. Proven by case 4b.gh apifailures as "no orphan"Lesson 2 (judgment) becomes standing guidance
Landed in
.claude/skills/orchestrate/SKILL.md, under the existing Delegation discipline section, as "Waiting is foreground work, on both sides". That section is the one place in the repo carrying both a delegation contract template and a wait-on-CI loop (## 3. Babysit), so it is the only candidate home with a real consumer;.claude/rules/core.mdwas rejected because it is paid for on every turn of every session while this applies only when delegating.Both halves land, because the lesson records that the subagent-side warning alone had already failed twice:
Housekeeping
## Graduatedin.claude/pending-lessons.mdwith the date and where each landed; the queue is now empty.tools/README.mdcatalog rows updated for the changed behaviour and the new exit codes.tools/dash-baseline.jsonshrinks by 4 (the em dashes in the two scripts are gone).Verification