ORB-106: Wire sleep orchestration to strict merge sweep - #625
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.
Code Review: PR #625 — Wire sleep orchestration to strict merge sweep
Scope: PR #625, thomasluizon/orbit-ui-mobile, single file: .claude/skills/orchestrate/SKILL.md
Recommendation: APPROVE
Summary
This PR replaces the hand-rolled --sleep merge decision in the /orchestrate skill's section 4a with a call to the existing bash tools/merge-sweep.sh <owner/repo> <pr-number>... script, keeping only the conditions the script structurally cannot decide (sleep authorization, wave-blocker/Phase-1 gates, D7 evidence, attempts:2, and full reviewer-comment reconciliation). No prior reviews or unresolved threads exist on this PR (first pass). I cross-checked every behavioral claim the new prose makes against the actual tools/merge-sweep.sh / tools/merge-sweep-cov.sh (both unchanged by this diff) and found the descriptions accurate. One internal-consistency defect survived the renumbering from 6 conditions to 5.
Findings
Critical
None.
High
None.
Medium
[MEDIUM] Stale self-reference after condition renumbering
· dimension: 1. Correctness
· location: orbit-ui-mobile/.claude/skills/orchestrate/SKILL.md, section "4a `--sleep`", condition 5
· issue: The diff renumbers the old six-condition list down to five (old condition 6, "every
comment addressed," becomes the new condition 5), but leaves the worked example inside that
same condition unchanged: "Measured on PR #621 ... conditions 1 to 5 alone would have merged
both defects." Under the old numbering this correctly excluded condition 6 (itself). Under the
new numbering, condition 5 IS the comment-reconciliation gate, so the sentence now says
"conditions 1 through 5" (including itself) would have merged the defects without the
comment check — a self-contradiction, since the comment check is condition 5.
· risk: Low operational risk (the surrounding prose still unambiguously requires comment
reconciliation), but it is a genuine broken cross-reference in a document an unattended agent
reads verbatim to decide whether to merge a PR while Thomas is asleep — exactly the kind of
precision this PR's own body holds itself to elsewhere (the five-condition mapping table, the
UNMET reconciliation notes).
· fix: Change "conditions 1 to 5" to "conditions 1 to 4" in that sentence so it again excludes
the condition it sits inside.
· reference: CLAUDE.md rule 1 (root-cause correctness) / internal document consistency
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 change, single-repo diff |
| security-reviewer | N/A — no orbit-api code changed |
| design-reviewer | N/A — no apps/* / landing UI file changed |
Validation
| Check | Result |
|---|---|
| Lint | PASS (CI: Lint — SUCCESS) |
| Type check | PASS (CI: Type Check — SUCCESS) |
| Tests | N/A per task scope (Build/Unit Tests run as a separate required check, not gated by this skill) |
| Harness Execution | PASS (CI: Harness Execution job — SUCCESS; PR body also cites node tools/test-tools.mjs and node .claude/hooks/test-hooks.mjs PASS output, satisfying rubric dimension 15's execution-evidence requirement) |
| Build (api) | N/A — orbit-api not touched, not checked out in this CI context |
Deferred — N/A dimensions & files not verdicted
- Dimension 2 (Dead/stale code), 3 (SOLID/clean-arch), 4 (Comment policy), 6 (Type safety), 7 (
console.log): N/A — the diff is prose in a Markdown skill file, not source code these checks apply to. - Dimension 8 (DESIGN.md/AI-slop), 9 (Parity), 10 (i18n): N/A — no
apps/*UI, no cross-platform files, no i18n strings touched. - Dimension 11 (Contract drift/backward-compat): N/A — no
packages/shared/src/types/*,endpoints.ts, or orbit-api DTO touched; not verifiable further since orbit-api is not checked out in this CI context. - Dimension 12 (Security), 13 (Backend hard rules): N/A — no
orbit-apicode in this diff. - Dimension 14 (FEATURES.md parity): N/A — this changes agent-skill orchestration behavior, not a user-facing product feature surface.
tools/merge-sweep.sh/tools/merge-sweep-cov.sh: not part of this diff (last touched in commit9b662a06, predates this PR) — read in full to verify the new SKILL.md prose's behavioral claims accurately describe them; no defect found there.- Nothing else deferred: the single changed file's full diff was walked hunk by hunk.
What's good
- The PR body's five-condition mapping table and explicit UNMET call-outs (exit-code semantics,
CLEAN-vs-CLEAN|UNSTABLE) are honest about what the tool does and does not cover, rather than overclaiming equivalence. - The new prose's description of
merge-sweep.sh(workflow-lookup fail-closed, review-check-settle-on-current-head, orphan-head verification, no--admin) was verified against the actual script and is accurate. - The full six-state comment-reconciliation paragraph (addressed / disagreed / informational, GraphQL resolve-and-verify) is preserved verbatim — the highest-value part of the old logic wasn't lost in the consolidation.
- Harness Execution evidence is present both in CI (green) and in the PR body (actual command + PASS output), satisfying dimension 15 without asking the reviewer to trust an unexecuted claim.
Recommendation
Approve as-is; the one Medium finding (stale "conditions 1 to 5" self-reference) is a one-line text fix and does not block merge per the rubric's deterministic Signal gate (NEEDS WORK requires a surviving Critical/High). Worth fixing in this PR or a fast immediate follow-up so the skill's own worked example stays internally consistent for the next --sleep run that reads it.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b26e7d1421
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Code Review: PR #625
Scope: PR #625, thomasluizon/orbit-ui-mobile, single file .claude/skills/orchestrate/SKILL.md
Recommendation: APPROVE
Summary
Replaces the hand-rolled --sleep merge decision in orchestrate's section 4a with a delegated
call to the existing bash tools/merge-sweep.sh <owner/repo> <pr-number>, keeping only the
conditions the script structurally cannot decide (sleep authorization + wave/Phase-1 gates, D7
evidence, attempts:2, full reviewer-comment reconciliation). Every behavioral claim the new
prose makes was cross-checked against the actual tools/merge-sweep.sh / merge-sweep-cov.sh
source (both unchanged, last touched in #623) and is accurate. No Critical/High findings.
Findings
Critical
None.
High
None.
Medium
None. I specifically re-checked the "conditions 1 to 5" self-reference inside condition 6's
PR-#621 anecdote, which a prior automated review on this PR flagged as a stale cross-reference
from a claimed 6-to-5 renumbering. That renumbering did not happen: the list is six items in
both the old and new text (verified line-by-line at the PR head) — only the content of items
1-3 changed; item 6 (comment reconciliation) is untouched by this diff and still sits at
position 6, so "1 to 5" still correctly excludes it. That earlier Medium finding does not
reproduce.
Low / Info
- INFO — Condition 3 ("mergeStateStatus is CLEAN") is stricter than the script it
delegates to, which accepts CLEAN or UNSTABLE. This is the exact "UNMET as literal
equivalence" gap the PR body's five-condition mapping table already discloses, not something
this diff introduces or hides. No action needed; changing the reviewed tool is explicitly out
of scope for ORB-106.
Subagents
All N/A: no apps/web/**/apps/mobile/**, no i18n strings, no packages/shared/src/types/*,
no orbit-api, no apps/* UI file in this diff.
Validation
Lint PASS, Type Check PASS, Harness Execution PASS (CI green; PR body also cites executed
node tools/test-tools.mjs / node .claude/hooks/test-hooks.mjs output). Build PASS. orbit-api
build N/A (not touched).
Deferred
Dimensions 2-4, 6-14 are N/A — this diff is prose in a Markdown skill file with no source code,
UI, i18n, contract, or backend surface. tools/merge-sweep.sh / merge-sweep-cov.sh are
unchanged by this diff but were read in full to verify the new prose's claims; no defect found,
and test-tools.mjs already covers both scripts' bad-usage paths, so no new harness-coverage
gap. Inline PR comments beyond the two review bodies were not independently enumerated (gh api
scope not available in this session); not required by this rubric — that reconciliation is
/orchestrate --sleep's own job at actual merge time.
What's good
- The PR body's five-condition mapping table and explicit "UNMET" call-outs are honest about
what the delegated script does and does not cover, rather than overclaiming equivalence. - Every behavioral claim about
tools/merge-sweep.sh(workflow-lookup fail-closed,
review-check-settle-on-current-head, no--admin, orphan-head verification, per-PR output
lines, exit codes) was verified against the actual script source and is accurate. - The full six-state comment-reconciliation paragraph is preserved verbatim; nothing was lost
in the consolidation. - A prior automated review's Medium finding (stale "conditions 1 to 5" reference) was checked
against the actual PR-head text and does not reproduce — see Findings above.
Recommendation
Approve as-is. No Critical/High findings survive.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c04337e47
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Code Review: PR #625 — ORB-106: Wire sleep orchestration to strict merge sweep
Scope: single file .claude/skills/orchestrate/SKILL.md (+52/-17)
Recommendation: APPROVE
Summary
This PR replaces the hand-rolled --sleep merge decision in /orchestrate section 4a with a delegated call to bash tools/merge-sweep.sh <owner/repo> <pr-number>, keeping only what the script structurally cannot decide (sleep authorization, wave-blocker/Phase-1 gates, D7 evidence, attempts:2, full reviewer-comment reconciliation). Two full "claude" reviews and one Codex pass already ran on this PR; the current head (9c04337e) is the exact commit the second "claude" review (APPROVE, no Critical/High) assessed — there is no diff since that last review. This pass independently re-verified the one disputed point from the first review and cross-checked the new prose's behavioral claims against the actual tools/merge-sweep.sh and tools/merge-sweep-cov.sh source; both hold up.
Findings
Critical: None.
High: None.
Medium: None survives. The first "claude" review flagged the "conditions 1 to 5" self-reference inside the old item-6 comment-reconciliation anecdote as stale after a claimed 6→5 renumbering. Tracing this against the base file and the PR's unified diff hunks shows: the diff only rewords items 1–3 and replaces the closing paragraph; the entire item-6 block falls in the diff's unshown context gap between the two hunks and is untouched. The list is still 6 items total after the PR, so "conditions 1 to 5" still correctly excludes item 6, the condition it sits inside. The second "claude" review's rebuttal was correct; this does not reproduce.
Low / Info: One holdover, already disclosed and non-blocking — new condition 3 ("mergeStateStatus is CLEAN") is stricter than merge-sweep.sh, which accepts CLEAN or UNSTABLE (tools/merge-sweep.sh:119). The PR body's own five-condition mapping table already discloses this as an explicit UNMET-as-literal-equivalence gap; changing the reviewed tool is out of scope for this ticket.
Independent script verification (both files unchanged by this diff, last touched in #623):
tools/merge-sweep.sh: update-branch first, pollsmergeStateStatus/review-check settlement (up to ~17 min), workflow-lookup fails closed (line 70-74), squash-merges with no--admin(line 121), verifies no orphaned head post-merge, exit 0/1/2/3 exactly as the new SKILL.md prose describes.tools/merge-sweep-cov.sh:--adminused only for a coverage-only SonarCloud failure (lines 152-160), matching the prose's claim that--sleepnever invokes this variant.
Subagents
| Agent | Verdict |
|---|---|
| parity-checker | N/A — no apps/web/**/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 changed, single-repo diff; orbit-api not checked out (not verifiable in CI) |
| security-reviewer | N/A — no orbit-api code changed |
| design-reviewer | N/A — no apps/*/landing UI file changed |
Validation (skipped executing /validate per task scope; Build/Unit Tests/SonarCloud are separate required checks)
| Check | Result |
|---|---|
| Lint | PASS (CI) |
| Type Check | PASS (CI) |
| Unit Tests | PASS (CI) |
| SonarCloud Analysis | PASS (CI, quality gate passed, 0 new issues) |
| Harness Execution | PR body cites executed node tools/test-tools.mjs / node .claude/hooks/test-hooks.mjs PASS output |
| Build (api) | N/A — orbit-api not touched, not checked out |
What's good
- The PR body's five-condition mapping table and explicit UNMET call-outs (exit-code semantics,
CLEAN-vs-CLEAN|UNSTABLE) are honest about what the delegated script does and doesn't cover rather than overclaiming equivalence. - The six-state comment-reconciliation paragraph (addressed/disagreed/informational, GraphQL resolve-and-verify) is preserved verbatim — the highest-value part of the old logic survived the consolidation.
- Every behavioral claim about
merge-sweep.sh/merge-sweep-cov.shchecks out against the actual scripts.
Recommendation
Approve as-is; no Critical/High findings survive independent verification, and the current head matches what the last full review already assessed.
| Severity | Count |
|---|---|
| Critical (incl. breaks old mobile clients) | 0 |
| High | 0 |
| Medium | 0 |
| Low / Info | 1 |
|
@codex review |
There was a problem hiding this comment.
Code Review: PR #625 — ORB-106: Wire sleep orchestration to strict merge sweep
Scope: PR #625, thomasluizon/orbit-ui-mobile, single file: .claude/skills/orchestrate/SKILL.md (+66/-17)
Recommendation: APPROVE
Summary
The diff replaces the hand-rolled --sleep merge-decision prose in /orchestrate section 4a with a delegated call to the pre-existing bash tools/merge-sweep.sh <owner/repo> <pr-number> script, keeping only what the script cannot decide (sleep authorization, wave-blocker/Phase-1 gates, D7 evidence, attempts:2, full reviewer-comment reconciliation). This review reads the current head (bbdae7ad, "Verify gated head after sleep sweep"), which adds a safeguard verifying the merged head matches the expected gated head. Every behavioral claim in the new prose was cross-checked against the actual tools/merge-sweep.sh source (unchanged by this diff — exit codes 0/1/2/3, the fail-closed workflow lookup, the CLEAN/UNSTABLE acceptance, the per-PR polling loop) and found accurate.
I specifically chased a race-condition hypothesis: does delegating the merge to a script with an internal poll reopen the exact reviewer-comment-staleness window that condition 6's own cited incident (PR #621) describes? On inspection it does not: each candidate PR's full six-condition check (including fresh comment/thread enumeration) is re-run immediately before that PR's own script invocation, and the newest commit (bbdae7a) adds a closing safeguard — comparing the merged headRefOid against the headRefOid that actually passed conditions 1–6, and refusing to stay silent (stop-all-further-merges + loud report) if the script merges a head the skill's own checks never saw. That closes the gap rather than leaving it open.
Findings
Critical
None.
High
None.
Medium
None.
Low / Info
None surfaced (rubric signal gate — Low/Info not posted as PR findings).
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 changed |
| security-reviewer | N/A — no orbit-api code changed (not checked out in this job) |
| design-reviewer | N/A — no apps/* / landing UI file changed |
Validation
Skipped per this workflow's CI adaptation — Build / Unit Tests / SonarCloud run as separate required checks. Harness Execution: PR body cites executed node tools/test-tools.mjs and node .claude/hooks/test-hooks.mjs, both PASS; this diff touches no file under tools/**, so merge-sweep.sh's existing test-tools.mjs coverage is unaffected.
What's good
- The newest commit (
bbdae7ad) closes a real gap: recording the pre-invocationheadRefOidas the "expected head" and comparing it to the post-merge head, with a loud stop-all-further-merges response on mismatch. - The PR body's "Five-condition mapping" table is honest about what the delegated script does and does not cover (script exit-code semantics vs. the old ticket's claim,
CLEAN-only vs. the tool's actualCLEAN/UNSTABLEacceptance), rather than overclaiming equivalence. - The six-state comment-reconciliation paragraph (addressed / disagreed / informational, GraphQL enumerate-and-resolve, the PR #621 incident evidence) survives the consolidation verbatim.
- "One PR at a time" is explicitly justified rather than asserted, tying it to why re-batching would reopen the stale-head window.
Recommendation
Approve. No Critical/High findings survive review of the current head.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 782a97a1f8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Code Review: PR #625
Scope: PR #625 in thomasluizon/orbit-ui-mobile (ORB-106: Wire sleep orchestration to strict merge sweep)
Recommendation: APPROVE
Summary
The diff is a single-file change to .claude/skills/orchestrate/SKILL.md: it replaces the
five hand-checked --sleep merge conditions with a six-condition skill-owned preflight plus
a handoff to the existing tools/merge-sweep.sh script for the mechanical merge decision.
Every claim in the new prose was cross-checked against the actual (unchanged)
tools/merge-sweep.sh source and its --help text; the described behavior (review-staleness
guard, fail-closed workflow lookup, no --admin, post-merge orphan-head scan, exit codes)
matches the real script exactly. One documentation gap survived verification (Medium, below);
no Critical/High findings.
Findings
Critical
None.
High
None.
Medium
[MEDIUM] CLEAN-vs-CLEAN-or-UNSTABLE gap disclosed in the PR body never reaches the persisted skill file
· dimension: Correctness (#1) / Harness changes (#15, doc-accuracy angle)
· location: .claude/skills/orchestrate/SKILL.md:327 (condition 3)
· issue: Condition 3 states the skill-owned invariant as "`mergeStateStatus` is `CLEAN`", and
the "Immediately before handoff" paragraph (~line 383) requires the same strict CLEAN read
before invoking the script. But `tools/merge-sweep.sh` itself merges on
`[ "$ms" = "CLEAN" ] || [ "$ms" = "UNSTABLE" ]`, and condition 2 already admits the tool "does
not keep a non-required pending check from being merged past" - i.e. UNSTABLE can mean a
non-required check is still running when the script decides to merge. The PR body's own
"Five-condition mapping" table names both gaps explicitly (rows 2 and 3, "marked UNMET...
because changing the tool is out of scope"), but that table lives only in the ephemeral PR
description. Nothing in the persisted `SKILL.md` says the invoked tool's real gate is
looser than condition 3's stated CLEAN-only text.
· risk: A future `--sleep` run (or a human auditing one after the fact) reads only the skill
file, not this PR's description. If GitHub's `mergeStateStatus` shifts from CLEAN to
UNSTABLE during the script's ~17-minute poll window (e.g. a non-required check is still in
flight), the script will squash-merge anyway - a state the skill's own condition 3 text says
is disallowed. Unlike condition 6, which documents its own tool gap in-file with the PR #621
incident, this gap is invisible to anyone who only reads `SKILL.md`.
· fix: Add a short note under condition 2 or 3 (same style as condition 6's incident note)
stating plainly that `tools/merge-sweep.sh` merges on `CLEAN` OR `UNSTABLE` and does not
itself wait out a still-running non-required check, so this is a known, accepted gap (or add
a skill-owned preflight that enumerates non-required pending checks the way condition 6
enumerates review threads).
· reference: root CLAUDE.md rule 1 (root cause / no undocumented workaround) and the
documentation-accuracy spirit of rubric dimension 15 ("a claim... must trace to a command
that ran"); the PR body itself half-discloses this but the artifact that governs future runs
does not.
Low / Info
None posted (signal gate: style/wording nits on a bounded, well-reasoned prose diff are not
actionable findings).
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/* / orbit-landing-page UI file changed |
Validation
| Check | Result |
|---|---|
| Lint | PASS (PR's live CI: PR Tests / Lint) |
| Type check | PASS (PR's live CI: PR Tests / Type Check) |
| Tests | PASS (PR's live CI: PR Tests / Unit Tests) |
Harness Execution (tools/test-tools.mjs + .claude/hooks/test-hooks.mjs) |
PASS (PR's live CI: Guards / Harness Execution, gated in because the diff touches .claude/skills/**) |
| Build (api) | N/A - orbit-api not touched |
Sourced from the PR's own live GitHub Actions check-runs rather than a local re-run: the local
working tree in this session is a detached checkout carrying an unrelated pre-existing
modification to the same file, so re-running lint/type-check/tests locally against it would
validate the wrong state. The PR's server-run CI is the authoritative, higher-fidelity source
for this diff's actual head commit.
Deferred - N/A dimensions & files not verdicted
- Dimension 8 (DESIGN.md / AI-slop): N/A, no
apps/*UI files in the diff. - Dimension 9 (Parity): N/A, no
apps/web/**orapps/mobile/**files in the diff. - Dimension 10 (i18n): N/A, no user-facing strings or i18n JSON in the diff.
- Dimension 11 (Contract drift + backward-compat): N/A, no
packages/shared/src/types/*or
orbit-api DTO in the diff. - Dimension 13 (Backend hard rules): N/A, no
orbit-apicode in the diff. - Dimension 14 (FEATURES.md parity): N/A, this changes an internal agent-orchestration skill
with no user-facing feature surface. - ORB-106's Linear ticket body was not independently re-read against the PR's own
reconciliation claims (no Linear access in this session); the PR body's stated UNMET items
are taken at face value and cross-checked only against the actualtools/merge-sweep.sh
source, not against the ticket text itself. - Every changed hunk in the single changed file (
.claude/skills/orchestrate/SKILL.md) was
read and given a verdict; nothing in the diff was skipped.
What's good
- Every behavioral claim about
tools/merge-sweep.shin the new prose (review-staleness
guard, fail-closed workflow lookup, no--admin, post-merge orphan-head scan, exit code
semantics) was checked line-by-line against the actual script and matches exactly - this is
unusually well-verified documentation. - The PR body's own "Five-condition mapping" and "Reconciliation discrepancies" sections
proactively name the two real gaps between the old five-condition text and the unchanged
tool's actual behavior, with an explicit, defensible reason each was left out of scope
(changing the reviewed tool). That transparency is exactly why the one surviving finding
here is Medium rather than Critical: the gap is real but already reasoned about, just not
yet carried into the file that future runs actually read. - Condition 6 (reviewer-comment reconciliation, unchanged by this diff) already sets the right
precedent for how to document a known tool gap in-file with a concrete incident (PR #621);
the fix for the Medium finding above is to extend that same treatment to conditions 2 and 3. - CI is green end to end on this diff's actual head commit (Lint, Type Check, Unit Tests,
Harness Execution, SonarCloud Quality Gate), which is stronger evidence than the PR body's
self-reported command transcript alone.
Recommendation
Approve as-is; the one Medium finding is a documentation-completeness gap, not a merge
blocker. Worth a quick follow-up (a two-to-three line addition near conditions 2/3) before
this skill is next relied on for an unattended --sleep run, so a future reader of
SKILL.md alone - not this PR's description - knows the tool's real merge gate is
CLEAN-or-UNSTABLE and doesn't itself wait out a still-running non-required check.
There was a problem hiding this comment.
Code Review: PR #625 — ORB-106: Wire sleep orchestration to strict merge sweep
Scope: PR #625, thomasluizon/orbit-ui-mobile, single file: .claude/skills/orchestrate/SKILL.md
Recommendation: APPROVE
Summary
The diff replaces the hand-rolled --sleep merge-decision prose in /orchestrate section 4a
with a delegated call to the pre-existing bash tools/merge-sweep.sh <owner/repo> <pr-number>
script, keeping only what the script structurally cannot decide (sleep authorization,
wave-blocker/Phase-1 gates, D7 evidence, attempts:2, full reviewer-comment reconciliation).
This pass reviews the current head (12dabfe6, "Document residual sleep merge race"), the
commit added since my last review on this PR (at 782a97a1). That commit documents the
Dependabot-auto-merge residual race — I verified .github/workflows/dependabot-auto-merge.yml
exists and does exactly what the new prose claims (approves + enables auto-merge for safe
Dependabot lanes independent of any /orchestrate --sleep run), so the new claim is accurate.
It does not touch the one Medium finding from my last review, which still does not reproduce
as fixed at this head.
Findings
Critical
None.
High
None.
Medium
[MEDIUM] CLEAN-vs-CLEAN-or-UNSTABLE gap disclosed in the PR body never reaches the persisted skill file
· dimension: 1. Correctness / 15. Harness changes (doc-accuracy angle)
· location: orbit-ui-mobile/.claude/skills/orchestrate/SKILL.md, section "4a `--sleep`", condition 3
· issue: Condition 3 states the skill-owned invariant as "`mergeStateStatus` is `CLEAN`", and the
"Immediately before handoff" paragraph requires the same strict CLEAN read before invoking the
script. But `tools/merge-sweep.sh:119` merges on
`[ "$ms" = "CLEAN" ] || [ "$ms" = "UNSTABLE" ]`, and condition 2 already admits the tool "does
not keep a non-required pending check from being merged past" — i.e. UNSTABLE can mean a
non-required check is still running when the script decides to merge anyway. The PR body's own
"Five-condition mapping" table names this gap explicitly (row 3, "marked UNMET below because
changing the tool is out of scope"), but that table lives only in the ephemeral PR description.
Nothing in the persisted `SKILL.md` says the invoked tool's real gate is looser than condition
3's stated CLEAN-only text. This is unchanged by the newest commit (`12dabfe6`), which adds a
different, unrelated disclosure (the Dependabot-auto-merge race) in the same section without
touching condition 2 or 3.
· risk: A future `--sleep` run, or a human auditing one after the fact, reads only the skill file,
not this PR's description. If `mergeStateStatus` shifts from CLEAN to UNSTABLE during the
script's own ~17-minute poll window (e.g. a non-required check is still in flight), the script
will squash-merge anyway — a state the skill's own condition 3 text says is disallowed. Unlike
condition 6, which documents its own tool gap in-file with the PR #621 incident, this gap is
invisible to anyone who only reads `SKILL.md`.
· fix: Add a short note under condition 2 or 3 (same style as condition 6's incident note, or the
new Dependabot-race paragraph this same commit just added) stating plainly that
`tools/merge-sweep.sh` merges on `CLEAN` OR `UNSTABLE` and does not itself wait out a
still-running non-required check, so this is a known, accepted gap — not an undocumented one.
· reference: root CLAUDE.md rule 1 (root cause / no undocumented workaround) and the
documentation-accuracy spirit of rubric dimension 15 ("a claim... must trace to a command that
ran"); the PR body itself half-discloses this but the artifact that governs future unattended
runs does not.
Low / Info
None posted (signal gate: no concretely-actionable Low/Info on this bounded prose diff).
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; sibling repo not checked out in this job (not verifiable in CI regardless) |
| security-reviewer | N/A — no orbit-api code changed |
| design-reviewer | N/A — no apps/* / orbit-landing-page UI file changed |
Validation
Per this workflow's CI adaptation, Phase 6 (/validate) is skipped — Build / Unit Tests /
SonarCloud run as separate required checks on this PR and all report SUCCESS on the current head
(12dabfe6): Lint, Type Check, Unit Tests, Build, SonarCloud Analysis, Contract Drift,
Cross-Platform Parity, Harness Execution, Dash Ban, Copy Register, Suppressions Ratchet, Expo SDK
Pin, Skill and Agent Frontmatter, arch-map, CodeQL, Dependency Review, Dependabot auto-merge
(skipped, not a Dependabot PR), GitGuardian, React Doctor, Mutation Testing — all SUCCESS/SKIPPED
as appropriate; no FAILURE among them. Harness Execution evidence is also present in the PR body
itself (node tools/test-tools.mjs and node .claude/hooks/test-hooks.mjs, both PASS), satisfying
rubric dimension 15 without asking the reviewer to trust an unexecuted claim.
Deferred — N/A dimensions & files not verdicted
- Dimensions 2–4, 6–7 (dead code, SOLID, comment policy, type safety,
console.log): N/A — the
diff is prose in a Markdown skill file, not source code these checks apply to. - Dimension 8 (DESIGN.md / AI-slop), 9 (Parity), 10 (i18n): N/A — no
apps/*UI, no
cross-platform files, no i18n strings touched. - Dimension 11 (Contract drift / backward-compat): N/A — no
packages/shared/src/types/*,
endpoints.ts, or orbit-api DTO touched; not verifiable further since orbit-api is not checked
out in this CI context. - Dimension 12 (Security), 13 (Backend hard rules): N/A — no
orbit-apicode in this diff. - Dimension 14 (FEATURES.md parity): N/A — this changes agent-skill orchestration behavior, not a
user-facing product feature surface. tools/merge-sweep.sh/tools/merge-sweep-cov.sh: not part of this diff (unchanged, last
touched before this PR) — read in full to verify the new SKILL.md prose's behavioral claims;
confirmed accurate, no new defect there..github/workflows/dependabot-auto-merge.yml: read in full to verify the newest commit's
residual-race claim; confirmed accurate.- Nothing else deferred: the single changed file's full diff, including the commit added since my
last review on this PR, was walked hunk by hunk.
What's good
- The PR body's "Five-condition mapping" table and explicit UNMET call-outs (exit-code semantics,
CLEAN-vs-CLEAN|UNSTABLE, the Dependabot race) are honest about what the delegated script does
and does not cover, rather than overclaiming equivalence. - The newest commit (
12dabfe6) closes a real documentation gap: naming
dependabot-auto-merge.ymlas an independent merge actor the "one PR at a time" serialization
cannot control, and stating plainly that closing it needs a tool change that is out of scope for
this ticket rather than silently leaving the race unstated. - The full six-state comment-reconciliation paragraph (addressed / disagreed / informational,
GraphQL enumerate-and-resolve, the PR #621 incident evidence) survives the consolidation
verbatim — the highest-value part of the old logic was not lost. - Every behavioral claim about
tools/merge-sweep.sh(workflow-lookup fail-closed,
review-check-settle-on-current-head, no--admin, orphan-head verification, exit-code
semantics) checks out against the actual script source.
Recommendation
Approve as-is. The one Medium finding is a documentation-completeness gap disclosed in the PR
body but not yet carried into the persisted skill file; it does not block merge per the rubric's
deterministic Signal gate (NEEDS WORK requires a surviving Critical/High). Worth a quick
follow-up — a two-to-three line addition near conditions 2/3, in the same style as the
Dependabot-race paragraph this PR just added — before this skill is next relied on for an
unattended --sleep run.
| Severity | Count |
|---|---|
| Critical (incl. breaks old mobile clients) | 0 |
| High | 0 |
| Medium | 1 |
| Low / Info | 0 |
|
@codex review |
1 similar comment
|
@codex review |
There was a problem hiding this comment.
Code Review: PR #625 — ORB-106: Wire sleep orchestration to strict merge sweep
Scope: PR #625, thomasluizon/orbit-ui-mobile. Focused on the diff since the last bot review (commit 12dabfe684 → 29214faa83, "Enforce expected heads in merge sweeps"): .claude/skills/orchestrate/SKILL.md (small update), tools/merge-sweep.sh, tools/merge-sweep-cov.sh, tools/test-tools.mjs. Six prior bot reviews already approved earlier SKILL.md-only revisions of this PR.
Recommendation: APPROVE
Summary
This increment closes the residual merge race the prior revision had only documented. Both merge-sweep scripts now accept repeated --expected-head <pr>=<sha> mappings, re-read headRefOid after update-branch and on every poll, print a distinct HEAD-MOVED skip naming both SHAs, and pass GitHub's atomic --match-head-commit at the merge call so a last-moment base advance (e.g. dependabot-auto-merge.yml) is refused server-side instead of merely detected after the fact. SKILL.md's prose matches the scripts' actual behavior exactly (verified line by line against both scripts). tools/test-tools.mjs adds a hermetic gh stub and four new cases per script (matched / changed-head / atomic-race / bare-invocation); the PR body's "Round 4 decision-path evidence" shows all eight passing against the real scripts.
Findings
Critical
None.
High
None.
Medium
[MEDIUM] New --expected-head input-validation branches ship with zero test coverage
· dimension: 15. Harness changes need EXECUTED evidence
· location: tools/merge-sweep.sh:56-92, tools/merge-sweep-cov.sh:55-106
· issue: Both scripts add ~7 new exit-2 decision paths for `--expected-head`: missing value,
malformed `<pr>=<sha>` (no `=`, empty pr, empty sha), non-numeric PR, non-hex SHA, wrong
SHA length (not 40/64 chars), duplicate mapping for the same PR, and a mapping naming a PR
not in the sweep. `tools/test-tools.mjs`'s `mergeSweepCases()` covers only the merge-time
behavior (matched/changed/race/bare); the pre-existing `INVALID_INPUT` table only exercises
a generic unknown-flag case that predates this diff and never reaches the new parsing code.
· risk: A future edit to this argument parser (e.g. relaxing the SHA-length check, or a typo in
the duplicate-detection loop) regresses silently — the rubric's own rule: "a new decision path
... needs its own case, not an extension of an existing assertion."
· fix: Add cases to `mergeSweepCases` (or a sibling) asserting `status === 2` for:
`--expected-head 615` (no `=`), `--expected-head =abc...` (empty pr), `--expected-head 615=`
(empty sha), `--expected-head 615=zz...` (non-hex), `--expected-head 615=abc` (wrong length),
`--expected-head 615=<sha> --expected-head 615=<sha2>` (duplicate), and
`--expected-head 999=<sha> <repo> 615` (mapping names an unrequested PR).
· reference: rubric.md dimension 15, "a new decision path added to a tool that already has
coverage ... needs its own case"
Low / Info
None posted (signal gate: Low/Info are not posted on a PR review).
Subagents
| Agent | Verdict |
|---|---|
| parity-checker | N/A — no apps/web/** or apps/mobile/** file changed |
| i18n-syncer | N/A — no user-facing strings touched |
| contract-aligner | N/A — no packages/shared/src/types/*/endpoints.ts change, only one repo touched |
| security-reviewer | N/A — no orbit-api code in this diff (sibling repo not checked out; not verifiable in CI either way) |
| design-reviewer | N/A — no apps/* UI files changed |
Validation
Phase 6 (/validate) skipped per this run's instructions — Build / Unit Tests / SonarCloud already run as required checks on this PR.
| Check | Result |
|---|---|
| Lint | N/A (skipped) |
| Type check | N/A (skipped) |
| Tests | Not independently re-run this session. PR body cites a real run producing 8/8 PASS for the new merge-sweep.sh/merge-sweep-cov.sh cases; CI's Harness Execution job (guards.yml) is authoritative once it lands. |
| Build (api) | N/A — orbit-api not touched, sibling repo not checked out |
Deferred — N/A dimensions & files not verdicted
- Parity, i18n, Contract drift, DESIGN.md/AI-slop, Backend hard rules, FEATURES.md parity: all N/A — diff touches only
.claude/skills/**andtools/**, neverapps/*,packages/shared/src/types/*,orbit-api, or a user-facing feature surface. - Security (API side): N/A — no
orbit-apicode in the diff; not verifiable in this CI session regardless since the sibling repo isn't checked out. - Every changed file (
SKILL.md,merge-sweep.sh,merge-sweep-cov.sh,test-tools.mjs) got a verdict above; nothing left un-reviewed.
What's good
- Root-cause fix (rule 1): uses GitHub's native
--match-head-commitso the server refuses a last-moment change, rather than a client-side workaround. SKILL.mdupdated in the same edit and stays fully accurate against the actual shell logic — no doc/code drift.- New
--expected-headvalidation is strict and fails closed (hex-only, exact 40/64-char length, duplicate-mapping and unrequested-PR checks all error with usage+exit 2). - The hermetic
ghstub models the "moved exactly at merge time" race via a marker file written inside the stubbedpr mergecall — precisely the scenario--match-head-commitexists to catch — and asserts the literal flag+value reached the real invocation.
Recommendation
Approve as-is. The one Medium finding (missing input-validation test coverage for the new --expected-head parsing branches) is a good follow-up but does not block merge.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 89ff7881a1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…orchestrate-sleep-call-the-merge
|
@codex review |
1 similar comment
|
@codex review |
There was a problem hiding this comment.
Code Review: PR #625
Scope: PR #625 in thomasluizon/orbit-ui-mobile — ORB-106: wire /orchestrate --sleep to the strict, head-pinned merge sweep (tools/merge-sweep.sh, tools/merge-sweep-cov.sh, .claude/skills/orchestrate/SKILL.md, tools/test-tools.mjs).
Recommendation: REQUEST CHANGES
This review runs after several prior rounds already resolved a HIGH finding on this PR (head-pinning aborting a routine BEHIND-branch update) via the new adopt_routine_update re-baselining logic — verified present at tools/merge-sweep.sh:346-372 and confirmed not to recur. This pass focuses on what's new/unresolved since that round.
Critical
The PR's own required "Context Budget" CI check is failing.
Confirmed live: gh pr view 625 --json statusCheckRollup shows Context Budget = FAILURE (Guards workflow, both duplicate runs). This diff doesn't touch CLAUDE.md or .claude/rules/core.md, and tools/check-context-budget.mjs --check compares this PR's measured bytes against the baseline committed on main — likely the branch trailing main's post-ORB-118 (context-trim) baseline rather than a defect in this diff's own content. The PR body's gate-output sections never show this check being run or passing. Fix: rebase onto current main and confirm node tools/check-context-budget.mjs --check passes before merge.
High
The pending-checks gate blocks on non-required checks, contradicting the PR's own documented scope.
tools/merge-sweep.sh:383-393,446-450 and tools/merge-sweep-cov.sh:218-227,502-506 compute pending/checks_pending from every row in statusCheckRollup that hasn't reached a terminal state — with no way to filter to required checks, since gh pr view --json statusCheckRollup exposes no isRequired field. This directly contradicts the PR's own "Condition mapping" table, which states: "the tool rejects failed conclusions and waits on merge-blocking state, but it does not enumerate non-required pending checks. The all-concluded requirement remains a skill-owned preflight." The shipped code does exactly what that row says is out of scope.
Practically: GitHub's mergeStateStatus=UNSTABLE (which the script explicitly accepts alongside CLEAN) means required checks passed while a non-required one is still failing/pending — that's the scenario UNSTABLE exists to describe. But checks_pending fires on ANY pending row regardless of required-ness, so a still-running non-required job (CodeQL, Dependency Audit, Vercel Preview Comments — all present on this very PR) blocks the merge anyway, negating the UNSTABLE acceptance in the common case. tools/test-tools.mjs:1608-1612 only exercises this path for the required review check pending, not for a non-required check under ms=UNSTABLE, so the gap is untested as well as undocumented. Given this is the tool --sleep (unattended, ~17min budget per PR) depends on, a PR sitting behind one slow non-required job can spuriously SKIP at timeout instead of merging — undermining ORB-106's purpose.
Fix: either filter pending to required checks (e.g. gh pr checks <n> --required or cross-reference branch protection), or correct the condition-mapping/help text to state that non-required pending checks ARE now enumerated and block merge.
Medium
review_safety_gate's "gh api lookup, fail closed" block is repeated near-identically four times within the function in each script (tools/merge-sweep.sh:272-345, mirrored in merge-sweep-cov.sh), pushing it toward the ~50-line soft cap (rule 7). A small fetch_or_fail() helper would collapse the repetition. (Note: the cross-script duplication between the two sweep files themselves is not a finding — tools/CONVENTIONS.md explicitly permits that until a third caller exists.)
Low / Info
Edited PR-review bodies sit outside the --reviewed-through reconciliation net — a documented GitHub API limitation (no edit timestamp on review bodies) called out honestly in SKILL.md, not an oversight.
Subagents
parity-checker / i18n-syncer / contract-aligner / security-reviewer / design-reviewer: N/A — diff touches only tools/** and .claude/skills/orchestrate/SKILL.md; no apps/*, orbit-api, or packages/shared surface.
Validation
Sourced from live CI (Phase 6 skipped per workflow instructions): Lint PASS, Type Check PASS, Unit Tests PASS, Harness Execution PASS, Harness Lockstep PASS, SonarCloud PASS, Context Budget FAIL (see Critical).
What's good
The adopt_routine_update re-baselining fix from the prior review round is correctly implemented and closes that HIGH cleanly. The --reviewed-through reconciliation gate remains thorough (paginated, edit-aware, exclusive cutoff, fails closed). Harness coverage is extensive and the round-by-round PASS evidence in the PR body is consistent with live CI.
Reviewed via the /pr-review skill against .claude/skills/pr-review/rubric.md.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ca73cdcd14
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f2e02ca607
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Code Review: PR #625 — ORB-106: Wire sleep orchestration to strict merge sweep
Scope: PR #625, thomasluizon/orbit-ui-mobile, head f2e02ca6070bbe9c8145a014eca710cefe1ad9f4 (checked out here as merge commit 2040a6a9). Files: tools/merge-sweep.sh, tools/merge-sweep-cov.sh, .claude/skills/orchestrate/SKILL.md, tools/test-tools.mjs.
Recommendation: APPROVE
Summary
This is at least the 10th review pass on this PR, which has already been through nine documented self-review rounds. It wires /orchestrate --sleep to a head-pinned, review-activity-aware strict merge sweep (--expected-head, --reviewed-through), closing several real unattended-merge races: stale-head merges, unreconciled review/comment activity, edited COMMENTED review bodies, and a residual post-merge activity window. Both blocking findings from the immediately-prior CHANGES_REQUESTED review (13:10 UTC, commit ca73cdcd) are confirmed resolved on this head:
- Critical (
Context BudgetCI check failing): confirmed live — bothContext Budgetcheck runs now readSUCCESS. - High (pending-checks gate silently contradicted the PR's own "non-required checks are skill-owned" claim): resolved not by filtering to required-only checks, but by making the stricter behavior (block on any pending check, required or not) explicit and consistent everywhere — the script
--helptext,SKILL.mdcondition 2, and the PR body's condition-mapping table now all agree. The contradiction was the actual finding; it's gone. - Medium (
review_safety_gate's repeated "gh api lookup, fail closed" block, 4x duplicated): resolved via the newreport_review_lookup_failure()/lookup_review_activity()helpers in commitd485cb73.
This pass found two new Medium-severity gaps in the newly-added post-merge safety net. Neither blocks.
Findings
Critical
None.
High
None.
Medium
SKILL.md's exit-4 operator guidance never names two of the three post-merge failure markers the script can print.
· dimension: 1 (Correctness) / 15 (Harness — operator contract)
· location: .claude/skills/orchestrate/SKILL.md (the "Read the script's per-PR output..." paragraph); tools/merge-sweep.sh:244,273,310 and the mirrored lines in tools/merge-sweep-cov.sh
· issue: review_safety_gate <pr> post can print three distinct markers under the identical exit-4 contract — POST-MERGE-ACTIVITY, POST-MERGE-UNRESOLVED-THREADS, and POST-MERGE-REVIEW-LOOKUP-FAILED. SKILL.md only ever names and quotes POST-MERGE-ACTIVITY ("report the exact POST-MERGE-ACTIVITY line" / "A PR named by POST-MERGE-ACTIVITY is already merged...").
· risk: exit code 4 itself is generically handled ("stop further unattended merges"), so the core safety property holds, but an agent following this doc literally has no instruction for what a POST-MERGE-UNRESOLVED-THREADS or POST-MERGE-REVIEW-LOOKUP-FAILED line means or how to log it — those two cases are likely to land in the closing report as an under-detailed generic "exit 4" instead of with the count/source the design elsewhere insists on.
· fix: generalize the paragraph to cover all three POST-MERGE-* markers instead of naming one three times.
· reference: CLAUDE.md rule 8
The post-merge branches for unresolved threads and lookup failure have no test coverage.
· dimension: 15 (Harness changes need EXECUTED evidence)
· location: tools/test-tools.mjs mergeSweepCases(); tools/merge-sweep.sh:305-312 and the tools/merge-sweep-cov.sh mirror
· issue: mergeSweepCases() covers POST-MERGE-ACTIVITY but never drives review_safety_gate's post-phase through the unresolved-threads branch or a lookup-failure branch — the existing "unresolved review threads..." / "...lookup-failed by name" cases only exercise the pre-merge phase.
· risk: per dimension 15, a new decision path (the post/pre phase split and its two other message variants) ships unexecuted; a defect in post-merge unresolved-count parsing or the phase branch of report_review_lookup_failure would go uncaught.
· fix: add two cases mirroring postMergeActivity's shape, with unresolvedThreads/threadsLookupFailure/reviewsLookupFailure gated to fire only after the merge call (the stub's existing postMergeMarker mechanism), asserting POST-MERGE-UNRESOLVED-THREADS #615 count= / POST-MERGE-REVIEW-LOOKUP-FAILED #615 source= with exit code 4.
· reference: .claude/skills/pr-review/rubric.md dimension 15
Low / Info
Not posted per the Signal gate. For the record, non-blocking: squash_merge() in tools/merge-sweep-cov.sh assigns actual=$(head_oid "$pr") without local (unlike its sibling pr/expected/label/branch) — a global leak on a call graph that's top-level-only today, so no live bug, just an inconsistency.
Subagents
| Agent | Verdict |
|---|---|
| parity-checker | N/A — no apps/web/**/apps/mobile/** file changed |
| i18n-syncer | N/A — no user-facing strings/i18n JSON changed |
| contract-aligner | N/A — no packages/shared/src/types/*/endpoints.ts, no backend repo touched |
| security-reviewer | N/A — no orbit-api code changed |
| design-reviewer | N/A — no apps/*/orbit-landing-page UI file changed |
Validation
Sourced from live CI (CI-wrapper context; Phase 7 skipped per workflow instructions; direct in-session execution of node tools/test-tools.mjs was blocked by this sandbox's approval policy for that command class).
| Check | Result |
|---|---|
| Lint | PASS (live CI) |
| Type Check | PASS (live CI) |
| Unit Tests | PASS (live CI) |
| Harness Execution | PASS (live CI — both duplicate Guards runs SUCCESS at query time) |
| Harness Lockstep | PASS (live CI) |
| Context Budget | PASS (live CI — previously-flagged Critical now resolved) |
| SonarCloud | PASS (live CI) |
| Build (api) | N/A — no orbit-api change |
Deferred — N/A dimensions & files not verdicted
- Dimensions 8, 9, 10, 11, 12, 13, 14: N/A — diff touches only
tools/**and.claude/skills/orchestrate/SKILL.md, noapps/*,packages/shared, ororbit-apisurface.orbit-apiwas not checked out in this session. - All four changed files were read in full at the PR head and given a verdict; nothing skipped for size.
- Harness Execution evidence is sourced from the live CI check run rather than a command this session ran itself (sandbox blocked direct execution).
What's good
Nine rounds of self-directed review against real, cited findings (a reproduced production incident on orbit-api#403, a measured reviewDecision-staleness gap on #621) is unusual rigor for a tooling PR. The --match-head-commit atomic assertion, adopt_routine_update's parent-based re-baselining after a routine update-branch, and the three-source (reviews/inline/issue) paginated, edit-aware, exclusive-cutoff reconciliation gate are all correctly implemented and now consistently documented across the script --help text, SKILL.md, and the PR body.
Recommendation
Approve as-is. The two Medium findings are real and worth a fast follow-up but don't weaken the shipped safety property — exit 4 already stops further unattended merging in both under-documented/under-tested cases; it's the operator-facing detail that's thin.
|
@codex review |
There was a problem hiding this comment.
Code Review: PR #625 — ORB-106: Wire sleep orchestration to strict merge sweep
Scope: PR #625, thomasluizon/orbit-ui-mobile, head 24543dedf17dc849ef1e2885140e389adcb5dd84. Reviewed the full diff plus, specifically, the delta since my prior APPROVE (f2e02ca6 → 24543ded, commit "Close review cutoff read race", .claude/skills/orchestrate/SKILL.md only).
Recommendation: APPROVE
Summary
This pass covers the last commit added since my previous review: it reorders the reconciliation-cutoff logic in SKILL.md so capturing reviewed-through happens BEFORE the final validation reads (byte-for-byte activity-snapshot comparison + unresolved-thread recheck), rather than after. That closes a real residual race — activity landing during the final re-read could previously sort before the boundary and be missed. The new ordering (reconciliation mutations → boundary → validation reads → sweep) is internally consistent and correctly reasoned. No new Critical/High issues.
Three Medium findings survive from a full-file review, all verified directly against the git-committed head (not the working tree, which carries an unrelated pre-existing uncommitted revert of this same file — confirmed via git status/git diff at session start, not something introduced by this review).
Findings
Critical
None.
High
None.
Medium
[MEDIUM] SKILL.md's illustrative reconciliation query omits `updatedAt`, which the script's own gate checks
· location: .claude/skills/orchestrate/SKILL.md:400-409 (the `reviews(first:100,...){nodes{author{login} body submittedAt lastEditedAt}}` snippet and "Treat both `submittedAt` and every non-null `lastEditedAt` as review activity")
· issue: tools/merge-sweep.sh:317-319 and the merge-sweep-cov.sh mirror query and check THREE fields per review — submittedAt, updatedAt, AND lastEditedAt (verified directly in both scripts). SKILL.md's human-facing example and prose name only two.
· risk: An operator reconciling strictly per the doc can believe reconciliation is complete, then have the script reject the same head because `updatedAt` moved for a reason the doc never mentioned — a confusing false-negative SKIP.
· fix: Add `updatedAt` to the illustrative query and the "treat as activity" sentence.
[MEDIUM] Reviews-connection pagination (--paginate) has no dedicated multi-page test
· location: tools/test-tools.mjs:203 (the `reviews(first:100` stub branch) vs. :250-251 (`ORBIT_MERGE_SWEEP_INLINE_PAGE_TWO`, wired only for inline comments)
· issue: review_safety_gate fetches reviews via `gh api graphql --paginate --slurp`. The stub always returns a single flat page for reviews regardless of `--paginate`; only the inline-comment stub has a page-two injection mechanism.
· risk: A regression in the slurp-flattening jq (losing the outer `.[] |` iteration) would ship silently.
· fix: Extend the reviews stub the same way, with an env-var-gated second page, plus a test case exercising it.
[MEDIUM] Dead PATH-based `sleep` stub in the merge-sweep test harness
· location: tools/test-tools.mjs:277-278 (`MERGE_SWEEP_SLEEP` staged + chmod'd) vs. :279/304 (`MERGE_SWEEP_BASH_ENV`'s `sleep() { :; }` wired via `BASH_ENV`)
· issue: `.sh` fixtures run as `bash <target>`; bash resolves shell functions before PATH executables, so the `BASH_ENV` function shadows the PATH stub — the `merge-sweep-bin/sleep` file can never run.
· risk: None functionally — unused code shipped and maintained for no reason.
· fix: Delete `MERGE_SWEEP_SLEEP` and its `chmodSync` call.
Low / Info
None (Signal gate).
Subagents
| Agent | Verdict |
|---|---|
| parity-checker | N/A — no apps/web/**/apps/mobile/** changed |
| i18n-syncer | N/A — no user-facing strings/i18n JSON changed |
| contract-aligner | N/A — no shared types/DTOs, single-repo scope |
| security-reviewer | N/A — no orbit-api changed |
| design-reviewer | N/A — no UI files changed |
Validation
CI-wrapper context; Phase 6 (/validate) and Phase 7 (posting) skipped per task instructions. At review time PR #625's head (24543ded) shows Build, Unit Tests, SonarCloud, Guards/Harness Execution, Harness Lockstep, and Cross-Platform Parity all green; the PR body documents repeated executed npm run lint/type-check/test + node tools/test-tools.mjs + node .claude/hooks/test-hooks.mjs runs with pass counts.
Deferred
- Dimensions requiring
apps/*,packages/shared, ororbit-api: N/A, no such surface in this diff;orbit-apinot checked out in this job. - Noted for the record: the checked-out working directory carries a pre-existing, uncommitted local revert of
.claude/skills/orchestrate/SKILL.mdto stale content (present before this review began — visible in the session's initialgit status). All findings above were verified against the actual git-committed blobs (git show <sha>:<path>), not the working-tree file, to avoid reviewing stale content. Worth a human check on why the worktree is dirty, though it is not a PR defect.
What's good
- The reordered cutoff logic (reconciliation → boundary → validation reads → sweep) correctly closes the race the commit title names, with a clear stated invariant (byte-for-byte snapshot match + no activity at/after cutoff) rather than a vague "re-check".
- Nine-plus rounds of self-directed review with real, cited, reproduced findings is unusual rigor for a tooling PR.
Recommendation
Approve. The three Medium findings are real and worth a fast follow-up but do not weaken the shipped safety property.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 24543dedf1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Reconciled the approved current-head review findings in |
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
Code Review: PR 625
Scope: PR #625 in thomasluizon/orbit-ui-mobile — "ORB-106: Wire sleep orchestration to strict merge sweep", head 4f99b663daf0394ce8ab82572a0309e6e1fdea71.
Recommendation: APPROVE
(Note: a review posted moments ago on this PR with the body "test" was a tool-call mistake, not a verdict — please disregard it; this review supersedes it.)
Summary
The diff is tooling-only: .claude/skills/orchestrate/SKILL.md, tools/merge-sweep.sh, tools/merge-sweep-cov.sh, and tools/test-tools.mjs. It teaches both merge-sweep scripts an --expected-head/--reviewed-through pinning contract, a review-activity safety gate (reviews, inline comments, conversation comments, all via paginated GraphQL/REST with an exclusive cutoff), atomic --match-head-commit merges, and a post-merge recheck that reports residual-race activity via exit 4. I traced the bash logic line-by-line and independently confirmed against a live gh 2.96.0 install (man pages for gh-pr-merge, gh-pr-update-branch, gh-api) that every flag used (--match-head-commit, --paginate, --slurp, -F/-f, --jq) is real and used correctly — this matters because the test harness stubs gh entirely and could not itself have caught a wrong flag name. No Critical or High finding survived; two Medium findings below.
Findings
Critical
None.
High
None.
Medium
[MEDIUM] Review-safety machinery duplicated verbatim across both sweep scripts
· dimension: 3 (SOLID / clean architecture — DRY, CLAUDE.md rule 10)
· location: tools/merge-sweep.sh:201-369, tools/merge-sweep-cov.sh:233-426
· issue: This diff adds ~200 lines of new, intricate, safety-critical logic —
review_safety_gate, lookup_review_activity, check_review_items,
newest_review_item_after, report_review_lookup_failure, adopt_routine_update,
head_oid, expected_head_for/reviewed_through_for, and the --expected-head /
--reviewed-through CLI-parsing block — byte-for-byte identical in both
merge-sweep.sh and merge-sweep-cov.sh. This is not 2-3 similar lines (rule 6's
"extract on the third use" bar); it is a second verbatim copy of the PR's entire
new safety contract.
· risk: The PR's own history (visible in its body) shows ~9 rounds where a fix had
to be re-applied to both files in lockstep. Any future fix to this logic now has
to be applied twice by hand; a fix landed in one file and missed in the other
silently reopens the exact merge-safety hole this PR exists to close, in
whichever script wasn't patched.
· fix: Extract the shared block into tools/lib/merge-sweep-common.sh (the two
shared flags' arg parsing, review_safety_gate and its helpers,
adopt_routine_update, head_oid) and source it from both scripts, keeping only
the genuinely divergent logic (Sonar-aware admin override vs. strict
single-shot) local to each file.
· reference: CLAUDE.md rule 10; rubric.md dimension 3.
[MEDIUM] New reviewThreads-pagination fail-closed path has zero test coverage
· dimension: 15 (Harness changes need EXECUTED evidence)
· location: tools/merge-sweep.sh:295-298, tools/merge-sweep-cov.sh:317-320
(mirrored gap in tools/test-tools.mjs's mergeSweepCases)
· issue: review_safety_gate's reviewThreads query caps at first:100 and, when
pageInfo.hasNextPage is true (a PR with more than 100 review threads), the jq
filter returns the literal string "PAGINATED" instead of a count. The shell
then hits case "$unresolved" in '' | *[!0-9]*), correctly failing closed as
REVIEW-LOOKUP-FAILED source=reviewThreads. This is a genuinely new decision
path, and a real scenario for this exact workflow — PR 625 itself went through
nine-plus review rounds by its own body. I confirmed tools/test-tools.mjs never
sets unresolvedThreads to a non-numeric value (only "0"/"2"), and the string
"PAGINATED" appears nowhere in the file, so the hermetic gh stub can never
produce it. The path is fail-closed today (never an unsafe merge), but it is
unexercised.
· fix: Add a case to mergeSweepCases that sets unresolvedThreads: "PAGINATED" and
asserts SKIP #615 REVIEW-LOOKUP-FAILED source=reviewThreads, for both scripts.
· reference: rubric.md dimension 15; TESTING.md Harness Execution job.
Low / Info
[INFO] --match-head-commit and the gh api graphql pagination usage verified against a live gh 2.96.0 install
· dimension: 1 (Correctness)
· issue: None — praise. man gh-pr-merge confirms --match-head-commit <SHA> is a
real, documented flag, and man gh-api confirms the --paginate/--slurp/-F/-f/--jq
combinations used for the GraphQL reviews connection and the REST
inline/issue-comments endpoints match the CLI's documented pagination contract
exactly. This is exactly the class of bug a fully-hermetic test suite cannot
catch, so I checked it by hand rather than trusting the diff read.
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 shared types/DTOs changed; orbit-api untouched |
| security-reviewer | N/A — no orbit-api code changed |
| design-reviewer | N/A — no apps/* or landing-page UI file changed |
Validation
Local session; could not re-run npm run lint/type-check/test or the harness
independently (sandbox blocked git worktree add/a separate local checkout of the
PR branch). Relied on the PR body's own extensive, repeated gate output instead —
most recently node tools/test-tools.mjs giving ORBIT TOOLS GATE OK and
node .claude/hooks/test-hooks.mjs giving ORBIT HOOK PARITY OK, both exit 0 —
while independently reading and tracing the current ~600-line mergeSweepCases
suite in tools/test-tools.mjs against both scripts' logic by hand (see the
Medium findings above for what that turned up).
Deferred
- Dimensions 8/9/10/11/13/14 (DESIGN.md, Parity, i18n, Contract drift, Backend
hard rules, FEATURES.md): N/A, this diff touches no apps/*, packages/shared, or
orbit-api surface. - Dimension 6 (Type safety): N/A in the TS/C# sense — changed files are POSIX sh
and a plain untyped .mjs tool script, the established tools/ convention. - All four changed files got a verdict; nothing changed-but-unreviewed.
- Local lint/type-check/test/harness re-execution: not run this session (sandbox
constraint noted above under Validation).
What's good
- The --match-head-commit atomic merge, the pre/post review_safety_gate pairing,
and adopt_routine_update's parent-check (distinguishing a routine
update-branch merge from an adversarial or unrelated head change) together
close the exact race the script's own header comment documents
(orbit-api#403: a stale APPROVED shipping a CHANGES_REQUESTED finding). - The --reviewed-through cutoff is exclusive (>=) and covers three independently
fail-closed activity sources (reviews via GraphQL
submittedAt/updatedAt/lastEditedAt, inline comments, conversation comments),
each with its own lookup-failure case in tools/test-tools.mjs — genuinely
thorough boundary testing (exactly-at, strictly-after, strictly-before, per
source, per script). - merge-sweep.sh structurally cannot accept --admin (unrecognized flags hit the
--*) "unknown argument" branch), enforcing in code, not just in prose, that
the unattended --sleep path can never bypass a red check. - The exit-4 post-merge recheck honestly reports what it cannot prevent (the
residual response-to-merge race) rather than pretending the pre-merge check
was sufficient.
Recommendation
Safe to merge as-is — nothing found rises to Critical or High. The two Medium
findings (duplicated safety logic across the two scripts, and the untested
100-review-threads fail-closed path) are legitimate but don't block: worth a
fast follow-up so the next change to this machinery has one copy to edit and one
more real decision path gets its own test case.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b0ce9ac5a4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| printf '%s\n' "$parents" | grep -Fxq "$old_expected" || return 1 | ||
| printf '%s\n' "$parents" | grep -Fxq "$base_tip" || return 1 | ||
| UPDATED_EXPECTED="$UPDATED_ACTUAL" |
There was a problem hiding this comment.
When another actor pushes while gh pr update-branch is in flight, the resulting commit can name both the recorded head and current base tip as parents while containing an arbitrary tree, such as additional edits or custom conflict resolutions. These checks then promote that commit to UPDATED_EXPECTED, so the unattended sweep can merge it without rerunning the skill-only head gates, including visible-effect evidence validation. Treat every changed head as HEAD-MOVED and require the caller to repeat the full preflight rather than authenticating an update from parent SHAs alone; the same logic in merge-sweep-cov.sh needs the same correction.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Code Review: PR #625 (ORB-106: Wire sleep orchestration to strict merge sweep)
Scope: PR #625 in thomasluizon/orbit-ui-mobile, head b0ce9ac5a4e15a7fa9411de2eeb96671ab40855b.
Recommendation: APPROVE
(Note: a review posted moments ago on this PR with body "test" was a tool-call mistake made during this same review pass, not a verdict — please disregard it; this review supersedes it.)
Summary
The diff is confined to orchestration tooling: .claude/skills/orchestrate/SKILL.md, tools/merge-sweep.sh, tools/merge-sweep-cov.sh, and their harness coverage in tools/test-tools.mjs. It threads a per-PR expected-head SHA and a --reviewed-through review-activity cutoff through both merge-sweep scripts, adds --match-head-commit for an atomic server-side head assertion, requires every status check (not just required ones) to reach a terminal state, and adds a post-merge residual-race check that reports (but cannot prevent) activity landing between the last safety read and the merge call. No apps/*, packages/shared, or orbit-api surface is touched, so platform-parity, i18n, contract-drift, DESIGN.md, and backend-hard-rule dimensions are all out of this diff's surface (see Deferred). The new logic is heavily covered by new cases in tools/test-tools.mjs, and CI's Harness Execution job passed on this exact head. No Critical or High finding survived review; three Medium findings are concrete and actionable but don't block merge.
Findings
Critical
None.
High
None.
Medium
[MEDIUM] Missing test case for the initial head-lookup failure ("both unavailable") path
· dimension: 15 (Harness changes need EXECUTED evidence)
· location: tools/merge-sweep.sh:394-402 (mirrored at tools/merge-sweep-cov.sh:428-436)
· issue: When no --expected-head mapping is supplied AND the first head_oid "$n" call itself fails (network/auth hiccup, PR not found, etc.), both expected and the fallback head_oid read are empty, and the script prints SKIP #$n HEAD-MOVED expected=<unavailable> actual=<unavailable>. This is a new, distinct decision path introduced by this PR's expected-head mechanism. grep -n "expected=<unavailable>" tools/test-tools.mjs returns nothing — no case in mergeSweepCases exercises it.
· risk: A regression here (e.g. a future refactor that drops the second if [ -z "$expected" ] guard) would ship unexecuted, exactly the hole dimension 15 exists to catch.
· fix: Add a mergeSweepCases case where the gh stub's pr view --json headRefOid branch returns empty for one PR with no --expected-head mapping supplied, and assert the exact SKIP #<n> HEAD-MOVED expected=<unavailable> actual=<unavailable> output with zero merge calls.
· reference: rubric.md dimension 15 ("a new decision path... needs its own case, not an extension of an existing assertion")
[MEDIUM] Missing test case for the reviewThreads pagination-overflow fail-closed branch
· dimension: 15 (Harness changes need EXECUTED evidence)
· location: tools/merge-sweep.sh:295-298 (mirrored at tools/merge-sweep-cov.sh:317-320)
· issue: The reviewThreads GraphQL query is capped at first:100 with no --paginate, and the --jq filter deliberately emits the literal string "PAGINATED" instead of a count when pageInfo.hasNextPage is true; review_safety_gate then treats that as an unparseable count and fails closed. This is a real, new fail-closed branch (any PR with >100 review threads — and this very PR's own history shows nine-plus review rounds). Neither tools/test-tools.mjs nor its gh stub ever produces "PAGINATED" for unresolvedThreads.
· risk: If this branch silently regressed (sentinel string changed, or the numeric-only case guard loosened), a PR with heavy review activity could fail-closed forever (denial of service on legitimate merges) or, worse, parse "PAGINATED" as falsy/zero and merge past unresolved threads. Neither failure mode would be caught before shipping.
· fix: Add a case to mergeSweepCases where the reviewThreads GraphQL stub reports hasNextPage:true, and assert SKIP #<n> REVIEW-LOOKUP-FAILED source=reviewThreads with zero merges, for both scripts.
· reference: rubric.md dimension 15
[MEDIUM] ~150-200 lines of review-safety-gate logic duplicated verbatim between the two scripts
· dimension: 3 (SOLID / clean architecture — DRY at the right level)
· location: tools/merge-sweep.sh:211-369 is byte-for-byte identical (aside from the merge-sweep.sh: vs merge-sweep-cov.sh: error-message prefix) to tools/merge-sweep-cov.sh:233-416: reviewed_through_for, newest_review_item_after, report_review_lookup_failure, lookup_review_activity, check_review_items, review_safety_gate, head_oid, adopt_routine_update, and expected_head_for.
· issue: This PR is what introduces this specific block into both files — it is new functionality, not an existing pattern being extended. The security-critical review-reconciliation gate, the one mechanism standing between an unattended sweep and merging past an unresolved or edited review, now exists in two copies that must be kept identical by hand.
· risk: A future fix (a new activity source, a cutoff-comparison bug, a new fail-closed case) landing in one script and not its twin silently reopens the exact race this PR was written to close, in whichever script was missed. The PR's own round-by-round history shows this exact lockstep-maintenance cost already happening repeatedly during development.
· fix: Extract the shared functions into a single sourced library, e.g. tools/lib/merge-sweep-safety.sh, and have both scripts source it. gate()/mstate() and the Sonar-specific admin-override logic can stay script-local since they genuinely differ.
· reference: CLAUDE.md rule 10
Low / Info
[INFO] The pending computation in gate()/mstate() also lists the review check itself as pending while it runs, which overwrites block_reason with the generic "checks... never all concluded" text instead of the more specific stale-review message. Cosmetic only — the merge is correctly blocked either way (review_stale still gates it) — not actionable per the 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 or orbit-api DTO changed |
| security-reviewer | N/A — no orbit-api code changed |
| design-reviewer | N/A — no apps/*/orbit-landing-page/src/** UI file changed |
Manual security pass (dimension 12, since no subagent fires for pure tooling): all new gh api graphql calls pass repo owner/name/PR number as parameterized -F GraphQL variables, never string-interpolated into the query text, so there is no injection surface from attacker-controlled review content flowing back only into echo output. --admin remains confined to merge-sweep-cov.sh's coverage-only path and is never reachable from --sleep, matching root CLAUDE.md's "Never --admin" convention for the unattended path. No secrets or credentials appear in the diff.
Validation
Local execution (node/bash/gh pr checks) was unavailable in this review session. Falling back to GitHub Actions' own run against this exact head (b0ce9ac5a4e15a7fa9411de2eeb96671ab40855b), fetched via gh pr view --json statusCheckRollup:
| Check | Result |
|---|---|
| Lint | PASS (CI) |
| Type Check | PASS (CI) |
| Unit Tests | PASS (CI) |
Harness Execution (test-tools.mjs + test-hooks.mjs) |
PASS (CI) |
| Harness Lockstep | PASS (CI) |
| SonarCloud Code Analysis | PASS (CI) |
| Build (api) | N/A — orbit-api not touched |
This satisfies dimension 15's execution-evidence bar via an independent CI run rather than a self-reported claim, but is not a rerun performed by this review pass itself — stated honestly rather than presented as something this session executed.
Deferred — N/A dimensions & files not verdicted
- Dimensions 8 (DESIGN.md/AI-slop), 9 (parity), 10 (i18n), 13 (backend hard rules), 14 (FEATURES.md parity): N/A, diff touches no
apps/*,orbit-api, orpackages/sharedsurface these dimensions gate on. - Dimension 11 (contract drift + backward-compat): N/A, no
packages/shared/src/types/*or orbit-api DTO changed. - Local validation re-run (Phase 7): deferred to CI's own result (see Validation table) because this session could not execute
node/bashlocally. - All four changed files (
SKILL.md,merge-sweep.sh,merge-sweep-cov.sh,test-tools.mjs) received a verdict above; nothing in the changed-file inventory was left unexamined.
What's good
- The expected-head +
--match-head-commitmechanism closes a real race (an independent actor advancing the base or a PR's head between the sweep's last read and its merge call) with a server-side atomic assertion, not just a client-side re-check. - The
--reviewed-throughgate treats every author (including bots and the orchestrator itself) as reconcilable activity, with no author-based exemption that could hide a late finding. - Test coverage for this diff's core new behavior (matching/changed head, atomic merge-race refusal, routine-update adoption vs. adversarial rejection, exclusive-cutoff boundary cases across all three activity sources, post-merge residual-race reporting) is thorough and specific, not just happy-path.
- The scripts fail closed by default on every new lookup (thread count, reviews, inline comments, issue comments), consistent with the script's established fail-closed convention.
Recommendation
Approve as-is. The two dimension-15 test-coverage gaps and the cross-script duplication are real but Medium: none block a merge tool that is otherwise well-tested and already green in CI. Worth a quick follow-up to add the two missing cases and consider the shared-lib extraction before a third script ever needs this same safety gate.
* chore(tools): reject acceptance criteria with no provable finish line check-ticket.mjs now fails an acceptance criterion that quantifies over an open set (every/all/any/each with nothing in the same criterion bounding it) or trails off into an unnamed remainder (etc., and so on, ellipsis). Measured cause: ORB-122 (PR #633) took 24 review rounds and 12 hours because "block every phrasing an agent could emit" is not a set anyone can enumerate, so every round the reviewer legitimately found one more member and was right. ORB-106 (PR #625) shows the same shape at 32 reviews. A criterion that names its enumeration (a count, a named list, a file, or a backticked command) has a finish line; one that does not cannot be proven done by anyone. The gate runs at every point a ticket already passes through: /ticket and /feature on the draft, /orchestrate again per ticket at wave-plan time, and the four /audit-* skills plus /prod-readiness on what they propose. node tools/test-tools.mjs -> ORBIT TOOLS GATE OK node .claude/hooks/test-hooks.mjs -> ORBIT HOOK PARITY OK Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G3TtwQhtWZdAndmpfmf8rC * fix(tools): tie the bound to the clause its quantifier is in Review finding on PR #638: BOUNDED_BY searched the whole criterion, so an unrelated token rescued an unbounded claim. Measured bypass, which the gate accepted before this commit and rejects after it: "every phrasing a worker could emit is blocked and the command exits 1" The stray 1 satisfied the bound while the unprovable finish line survived untouched. The bound now has to appear in the same clause as the quantifier. Code spans and file paths are masked before the split so a backticked command containing dots is not cut in half by its own extension. Six measured cases through the real tool, all correct: the bypass above and "...blocked, see tools/check-ticket.mjs" reject; "every phrasing rejected by `node tools/check-ticket.mjs` is blocked", "The three docs each carry a WHY note" and "all 5 call sites route through the shared reader" pass; "any single check failing produces a FAIL row" rejects. node tools/test-tools.mjs -> ORBIT TOOLS GATE OK node .claude/hooks/test-hooks.mjs -> ORBIT HOOK PARITY OK Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G3TtwQhtWZdAndmpfmf8rC * fix(tools): accept a bound stated before its quantifier, and hyphenated compounds Two precision defects found by running the gate against the three ticket bodies still queued in this run, before their workers start. ORB-107: "A clean environment produces an all-PASS table and exit 0" was rejected because "all-PASS" is a hyphenated compound adjective, not a quantifier. The quantifier now requires no following hyphen. ORB-120: "`design-specialist.md` and `product-manager.md`, which currently declare no `model:`, each gain one" was rejected because the two named files sit in clauses BEFORE the one holding "each". A bound now counts when it appears in the quantifier's own clause or any clause preceding it, never in a clause after it, which is exactly the direction the PR #638 review bypass exploited ("...is blocked and the command exits 1"). Verdicts after this commit, via --issue against the live bodies: ORB-89 clean, ORB-107 "ticket ok", ORB-120 clean. node tools/test-tools.mjs -> ORBIT TOOLS GATE OK node .claude/hooks/test-hooks.mjs -> ORBIT HOOK PARITY OK Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G3TtwQhtWZdAndmpfmf8rC --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
There was a problem hiding this comment.
Review Complete
Scope: PR #625 in thomasluizon/orbit-ui-mobile — ORB-106: Wire sleep orchestration to strict merge sweep
Recommendation: APPROVE
| Severity | Count |
|---|---|
| Critical (incl. |
0 |
| High | 0 |
| Medium | 2 |
| Low / Info | 0 |
Summary
Pure tooling change — tools/merge-sweep.sh / tools/merge-sweep-cov.sh gain --expected-head + --reviewed-through pinning, an atomic --match-head-commit merge assertion, a fail-closed review/inline/issue-comment reconciliation gate with exclusive-cutoff semantics, post-merge race detection (exit 4), and a stricter "every check must conclude" rule; .claude/skills/orchestrate/SKILL.md documents the new --sleep protocol. No apps/*, orbit-api, or packages/shared files touched, so parity/i18n/DESIGN.md/contract-drift/backend-hard-rules/FEATURES.md are all N/A and none of the five gated subagents fire.
The merge/skip control flow in both scripts (head-pin match/mismatch, the "routine update adopted only if parents include both the old expected head and the base tip" adversarial-update rejection, the review-safety gate called immediately pre- and post-merge, exit-code precedence) was hand-traced and no path was found that lets a stale-headed or unreviewed PR merge. Test coverage in tools/test-tools.mjs (mergeSweepCases) maps to every new decision path found by reading the scripts.
Findings (Medium, both concrete follow-ups, neither blocking)
tools/README.mdcatalog rows (lines 22-23) are stale. The merge-sweep.sh/merge-sweep-cov.sh entries still show the pre-PR usage (bash merge-sweep.sh <repo> <pr...>) with no mention of--expected-head,--reviewed-through,--match-head-commit, or exit code 4 — even though both scripts' own--helptext and SKILL.md were updated correctly. The PR's "no tools/README.md row was missing" note addresses row presence, not row accuracy.- The review-safety-gate helpers (~150 lines:
review_safety_gate,check_review_items,newest_review_item_after,adopt_routine_update, etc.) are byte-identical between the two scripts, unlike this repo's other intentional "twin" files (e.g.second-opinion.mjs), which are protected bycheck-lockstep.mjs. Nothing in CI would catch the two copies drifting apart on a future fix.
Validation
Not independently re-run in this session (npm/node execution declined by the sandbox's permission system on every attempt; per the CI-adaptation instructions, Phase 6//validate is skipped here since Build/Unit Tests/SonarCloud run as separate required checks). Relying on the PR body's own Round 9 evidence (node tools/test-tools.mjs → ORBIT TOOLS GATE OK, node .claude/hooks/test-hooks.mjs → ORBIT HOOK PARITY OK on the final commit), cross-checked by reading the added test cases directly rather than trusting the claim at face value.
Deferred / not verifiable in CI
Dimensions 8, 9, 10, 11, 13, 14 — all N/A, diff surface never touches apps/*, i18n, packages/shared/types, or orbit-api. Cross-repo dimensions (contract-aligner, orbit-api side of backward-compat guard) are not applicable — no orbit-api changes in this diff, and the sibling repo is not checked out in this job regardless. All four changed files got a full verdict; nothing deferred at the file level.
1 participant
Summary
Implements ORB-106 by making
/orchestrate --sleeppass its gatedheadRefOidinto the strict sweep:Both sweep scripts now accept repeated per-PR expected-head mappings. A bare invocation remains supported and captures each PR head at entry as its expectation. The scripts re-read the head after branch updates and on every poll, emit a distinct
HEAD-MOVEDskip with both SHAs, and use GitHub's atomic--match-head-commitassertion at the merge call. A last-moment atomic refusal is followed by another head read so the same greppable reason is reported.The strict script remains the only sleep path.
merge-sweep-cov.shremains an attended choice outside--sleepbecause its coverage-only path uses--admin.Condition mapping
reviewDecisionisAPPROVEDAPPROVED; the current-headreviewcheck must settle first when the workflow exists or workflow lookup is unreadable.BEHIND, then requireCLEANCLEAN. The script acceptsCLEANorUNSTABLE, but now only for the exact pinned head.CLEANhandoff remain skill-owned; head identity is tool-enforced.attempts:2labelRound 4 decision-path evidence
The real scripts ran against a hermetic
ghstub. No GitHub PR was updated or merged.The coverage matching and atomic-refusal cases exercise the coverage-only
--adminpath. The matching cases also assert the exact--match-head-commit <expected-sha>argument.Updated help contracts
Gate output
Reconciliation notes
MERGED,SKIP,MERGE-REFUSED, orFAIL-ADMINline. Exit 1 remains reserved for an orphaned merged head.CLEAN, while the pinned-head script acceptsCLEANorUNSTABLEafter rejecting failed conclusions.Decisions taken unattended
--expected-head <pr>=<sha>mappings so multi-PR callers can pin every candidate while the existing bare positional form remains valid.--sleeppath. The coverage script receives the same head protection but remains an attended tool because it can use--admin.tools/README.mdrow was missing.Round 5 merge integration
Merged current
origin/mainat58431c61into the branch as merge commit89ff7881. The live conflict was intools/test-tools.mjs; the resolution keeps both ORB-121'sISSUES_WAVE_STUBand explicit--issuescases and ORB-106's expected-head merge-sweep cases. The auto-merged orchestrate skill retains both scope systems.GitHub now reports
mergeStateStatus: BLOCKED, notDIRTY, on head89ff7881a127aa5ef99d52e223b7d8ede34f12b2.Context budget evidence
ORB-106 does not change either measured file, so no reseed or
context:reseedlabel applies.Round 5 gate output
Round 6 review safety gate
Merged
origin/mainat58f40476through merge commitfa657cd4, preserving ORB-119's sibling-import removal and context baseline. The ORB-106 fix commit isbe78cbb6.Both sweep scripts now require a per-PR
--reviewed-throughreconciliation boundary before merge. Immediately before every strict, normal coverage, or admin coverage merge path, they fail closed on unreadable review data, refuse unresolved threads, and refuse reviews or issue comments newer than the caller's boundary with the author and timestamp named.The orchestrate skill records the reconciliation boundary beside the gated head and passes both values into the strict sweep.
Context budget evidence
ORB-106 does not change either measured file, so no reseed or
context:reseedlabel applies.Round 6 gate output
Round 7 inline activity gate
Fix commit
30aca557closes the remaining reviewed-through endpoint gap. Both sweep scripts now paginate review submissions, inline review comments, and conversation comments. Inline and conversation comments contribute bothcreated_atandupdated_at, so an edit after the cutoff is newer activity. Every lookup fails closed; newer inline activity reports the endpoint, author, and timestamp.Round 7 gate output
Round 8 exclusive cutoff
Fix commit
4fc4e393closes the same-second boundary. Both sweep scripts now treat--reviewed-throughas an exclusive cutoff: activity at or after the timestamp is new. The shared comparison uses>=, so the rule applies to review submissions, inline comment creation and edits, and conversation comment creation and edits. The help contracts state the boundary explicitly.The harness proves equal, strictly-after, and strictly-before timestamps independently for all three endpoints and for both sweep scripts. All Round 7 cases remain green.
Round 8 gate output
Round 9 verified update and residual race handling
Fix commit and head: `ca73cdcd142d0dee46916e5db19541719814d32e`
Both sweep scripts now distinguish their own routine update-branch merge from an unrelated head move. Before update they verify the current head still equals the expected head. After update, a changed head is adopted only when its parents include both the prior expected head and the current base tip. The adopted head is then fully re-gated: all checks must be terminal and successful, the current-head review check must be settled, reviewDecision must be APPROVED, and the review safety gate must pass before the atomic match-head merge.
The orchestrate skill now enumerates all review submissions, inline comments, and conversation comments without author exclusions. It posts reconciliation replies and resolves threads first, mechanically verifies that reconciliation, then captures reviewed-through as the final reconciliation act at an instant strictly later than every reconciled create, edit, or submission timestamp. Because GitHub timestamps have second precision and equality counts as new, the skill waits for the next second when required and immediately invokes the sweep without another review mutation.
The response-to-merge interval cannot be made atomic with GitHub review activity. The pre-merge safety query is therefore the last operation before each merge call, and the same query runs immediately after a successful merge. Activity found in that residual window is reported exactly as:
```text
POST-MERGE-ACTIVITY # at
```
Post-merge activity or an unverifiable post-merge review state sets exit code 4, stops further unattended merges, and is carried into the Merged ledger.
Updated help contract
```text
A routine update-branch merge is adopted only when its parents
include both the prior expected SHA and the current base tip; any other change prints HEAD-MOVED.
The safety query is the last operation before merge and runs again after success. Post-merge
activity or an unverifiable post-merge review state is reported with a URL when available and
exits 4; this detects but cannot prevent the residual response-to-merge race.
Exit codes: 0 every merged head verified clean; 1 at least one orphaned head branch; 2 bad usage;
3 a head branch could not be verified; 4 post-merge review activity or an unverifiable review
state (unknown is not a clean pass).
```
The normal and coverage-aware help output carry the same contract.
Round 9 tools harness
```text
PASS merge-sweep.sh: help documents the exclusive cutoff and residual post-merge window
PASS merge-sweep.sh: a routine update adopts and rechecks the new head before merging
PASS merge-sweep.sh: an adversarial update without the expected parents is rejected
PASS merge-sweep.sh: a failing check on the adopted head skips without merging
PASS merge-sweep.sh: an unsettled current-head review check skips without merging
PASS merge-sweep.sh: a reconciled reply before the refreshed cutoff merges
PASS merge-sweep.sh: activity in the residual merge window is reported after the merge
PASS merge-sweep.sh: genuine third-party activity at the refreshed cutoff skips
PASS merge-sweep-cov.sh: help documents the exclusive cutoff and residual post-merge window
PASS merge-sweep-cov.sh: a routine update adopts and rechecks the new head before merging
PASS merge-sweep-cov.sh: an adversarial update without the expected parents is rejected
PASS merge-sweep-cov.sh: a failing check on the adopted head skips without merging
PASS merge-sweep-cov.sh: an unsettled current-head review check skips without merging
PASS merge-sweep-cov.sh: a reconciled reply before the refreshed cutoff merges
PASS merge-sweep-cov.sh: activity in the residual merge window is reported after the merge
PASS merge-sweep-cov.sh: genuine third-party activity at the refreshed cutoff skips
ORBIT TOOLS GATE OK
Exit 0
```
Round 9 complete gates
```text
$ node tools/test-tools.mjs
ORBIT TOOLS GATE OK
Exit 0
$ node .claude/hooks/test-hooks.mjs
ORBIT HOOK PARITY OK
Exit 0
$ npm run lint
Tasks: 3 successful, 3 total
Exit 0
$ npm run type-check
Tasks: 3 successful, 3 total
Exit 0
$ npm run test
Tasks: 4 successful, 4 total
Mobile: 247 files passed, 1611 tests passed
Exit 0
Post-push:
$ node tools/test-tools.mjs
ORBIT TOOLS GATE OK
Exit 0
$ node .claude/hooks/test-hooks.mjs
ORBIT HOOK PARITY OK
Exit 0
```
The commit hook dash gate and push target guard also passed. No visible-effect screenshot applies because this round changes orchestration tooling and its skill, not an application surface.
Self-driven review: edited review bodies
Fix commit: `d485cb73bf75099744ad5673528d2d94bd044638`
Integrated head: `f2e02ca6070bbe9c8145a014eca710cefe1ad9f4`
Both sweep scripts now enumerate review submissions through the paginated GraphQL reviews connection and evaluate `submittedAt`, `updatedAt`, and `lastEditedAt` with the existing exclusive cutoff. An edit to an already-submitted `COMMENTED` review is therefore new review activity even when its original submission predates `reviewed-through`. Lookup, pagination, or malformed-data failures remain fail-closed as `source=reviews`, and activity reports retain the author, timestamp, and URL.
The repeated lookup and phase-aware failure handling in each script is consolidated into one helper. The orchestrate skill uses the same GraphQL edit timestamps during reconciliation and sets the final cutoff strictly after both submitted and edited review activity, with no author exclusions.
The all-concluded condition is intentionally strict. Every pending status check blocks, including a non-required check, so the implementation stays aligned with the ticket requirement. The earlier condition-mapping row that described this as skill-only has been corrected.
Current `main` at `f1513383` was merged without rebasing. ORB-113's `/ticket` workflow, scope-completeness section, check-ticket cases, and context baseline all remain intact. The final PR diff remains limited to the four ORB-106 files.
Verification
```text
$ node tools/test-tools.mjs
PASS merge-sweep.sh: an already-submitted COMMENTED review edited after the cutoff skips without merging
PASS merge-sweep.sh: a COMMENTED review edited strictly before the cutoff still merges
PASS merge-sweep-cov.sh: an already-submitted COMMENTED review edited after the cutoff skips without merging
PASS merge-sweep-cov.sh: a COMMENTED review edited strictly before the cutoff still merges
PASS check-ticket.mjs: issue mode rejects zero type labels and names every valid value
PASS check-ticket.mjs: a named dependency with its blockedBy relation is accepted
ORBIT TOOLS GATE OK
Exit 0
$ node .claude/hooks/test-hooks.mjs
ORBIT HOOK PARITY OK
Exit 0
$ npm run lint
Tasks: 3 successful, 3 total
Exit 0
$ npm run type-check
Tasks: 3 successful, 3 total
Exit 0
$ npm run test
Tasks: 4 successful, 4 total
Mobile: 247 files passed, 1611 tests passed
Exit 0
$ node tools/check-context-budget.mjs --check --json
enforcedBytes: 15381
baselineBytes: 15381
deltaBytes: 0
status: ok
```
No baseline was edited or reseeded. No visible-effect screenshot applies.
Self-driven review: cutoff before final reads
Fix commit and head: `24543dedf17dc849ef1e2885140e389adcb5dd84`
The reconciliation protocol now captures `reviewed-through` after all required replies and resolutions but before the final validation reads. It retains a complete reconciled activity snapshot, waits for a strictly later GitHub timestamp second, captures the boundary, then re-reads all three activity sources and the unresolved-thread count.
The final snapshot must be byte-for-byte unchanged, no activity may be at or after the boundary, and unresolved threads must remain zero. The snapshot comparison catches activity that arrives while waiting for the next timestamp second but sorts before the boundary; the sweep cutoff catches activity after the final reads. Any difference restarts reconciliation and requires a new boundary. No author exclusion is used.
Required order:
```text
reconciliation mutations
reviewed-through boundary
final snapshot and unresolved-thread validation
strict sweep
```
Verification:
```text
$ node tools/check-dashes.mjs --files .claude/skills/orchestrate/SKILL.md
Exit 0
$ node tools/check-frontmatter.mjs
frontmatter ok
Exit 0
$ node .claude/hooks/test-hooks.mjs
ORBIT HOOK PARITY OK
Exit 0
$ node tools/test-tools.mjs
ORBIT TOOLS GATE OK
Exit 0
```
The isolated tools harness completed in 811.1 seconds after the first parallel run exceeded its 600-second process bound. The timed-out process tree was terminated and the isolated rerun passed; this was execution contention, not a failed assertion.