ORB-141 Fix merge sweep review lookup - #646
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
/pr-review — PR #646 (ORB-141)
Scope: the merge-sweep review-activity lookup scripts plus their shared test harness. Changed files:
tools/merge-sweep.sh— Repo-tool appeal: file path cited only as diff scope for this review report, not surfaced as an executable commandtools/merge-sweep-cov.sh— Repo-tool appeal: file path cited only as diff scope for this review report, not surfaced as an executable commandtools/test-tools.mjs
Recommendation: APPROVE
| Severity | Count |
|---|---|
| Critical (incl. |
0 |
| High | 0 |
| Medium | 0 |
| Low / Info | 0 |
Summary
ORB-141 removes the unsupported --slurp flag from the paginated GraphQL review-activity lookup in both merge-sweep twins, since GitHub CLI 2.88.1 rejects --slurp combined with --jq/--template. The --jq selector is updated to read .data.repository.pullRequest.reviews.nodes[] | ... directly off each un-slurped page, which is the correct shape for how gh api graphql --paginate applies --jq per page (vs. once over a slurped array) — consistent with the sibling REST-endpoint lookups' existing .[] | ... pattern in the same file. A new static harness guard (mergeSweepCliFlagCases in tools/test-tools.mjs) scans both real script files and fails if any gh api invocation combines --slurp with --jq/--template; the existing pagination assertion was updated to require --paginate and forbid --slurp. Diff is scoped entirely to tools/** — no apps/*, packages/shared, or orbit-api touched.
Findings
None across all severities.
Subagents
All five gated subagents (parity-checker, i18n-syncer, contract-aligner, security-reviewer, design-reviewer) are N/A — the diff touches no apps/*, packages/shared/src/types/*, orbit-api, or UI files.
Deferred
- Dimensions 8–14 (DESIGN.md, Parity, i18n, Contract drift, Security, Backend hard rules, FEATURES.md): N/A, surface never touched by this tools-only diff.
- Phase 6 (
/validate) skipped per this workflow's CI adaptation — Build / Unit Tests / SonarCloud run as separate required checks on this PR. - Harness Execution evidence for dimension 15 comes from the PR body's own before/after command output (lint/type-check/test/check-lockstep.mjs/test-tools.mjs, including the new guard failing pre-fix and passing post-fix), corroborated by static reading of both changed shell scripts (confirmed no stray
--slurpremains and no false-positive risk in the new regex guard).
What's good
Root-cause fix (no fallback/workaround), a genuine regression guard added for the exact defect class, unusually rigorous PR-body evidence (live before/after API calls, calibrated fail→pass harness output), and an honest callout of the one acceptance criterion (a real end-to-end merge) left unmet under the worker contract rather than glossed over.
|
@codex review |
…erge-sweep-can-never-merge
|
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". |
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 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 #646
Scope: PR #646 in thomasluizon/orbit-ui-mobile
Recommendation: APPROVE
Summary
PR #646 (ORB-141) removes the unsupported gh api graphql --slurp flag (incompatible with --jq/--template on GitHub CLI 2.88.1) from the paginated review-activity lookup in both merge-sweep twins (merge-sweep.sh:316, merge-sweep-cov.sh:338), adjusts the jq selector to match the now-unwrapped paginated response shape, and adds a static harness guard (mergeSweepCliFlagCases in test-tools.mjs:2220) that fails if either twin ever recombines --slurp with --jq/--template again. The fix is narrowly scoped, backed by live gh api evidence against a real PR (#642) showing before (exit 1) / after (exit 0, correct TSV rows), and the PR body carries a full pasted test-harness run (571 assertions, ORBIT TOOLS GATE OK) plus green lint / type-check / test and lockstep-check output.
Note: since the prior review on this PR, the branch picked up a merge commit from origin/main (bringing in the already-merged, unrelated ORB-120 calibration-gate commit). Diffing the PR's actual merge base against the current head confirms the PR's own three changed files are unchanged since that review — the merge added no new PR-authored content.
Findings
Critical
None.
High
None.
Medium
None.
Low / Info
[INFO] Acceptance criterion for a real end-to-end merge is honestly disclosed as UNMET
- dimension: 1 (Correctness)
- location: orbit-ui-mobile/PR #646 body, "UNMET" section
- issue: The ticket asked for proof via a real qualifying PR actually merging end-to-end; the author could not do this because the standing worker contract forbids merging a PR or touching another ticket.
- risk: None — disclosed plainly rather than papered over, and proven two other ways: live
gh apievidence against a real, already-merged PR (#642) showing the exact before/after invocation and output, and the hermetic test harness asserting the merge call is reached and made for both twins. - fix: No action needed; noted only so the human merging is aware one acceptance criterion was substituted with equivalent evidence.
- reference: CLAUDE.md D7 (evidence gate) / rubric dimension 1
Subagents
| Agent | Verdict |
|---|---|
| parity-checker | N/A — diff touches only tooling scripts |
| i18n-syncer | N/A — no user-facing strings / locale JSON changed |
| contract-aligner | N/A — no shared types / orbit-api changed |
| security-reviewer | N/A — orbit-api not touched |
| design-reviewer | N/A — no UI files touched |
Validation
Running inside the CI wrapper, so this cites the PR's own CI checks and the executed evidence pasted in the PR body rather than a local re-run.
| Check | Result |
|---|---|
| Lint | PASS (CI job Lint, SUCCESS) |
| Type check | PASS (CI job Type Check, SUCCESS) |
| Tests | PASS (CI job Unit Tests, SUCCESS; PR body also carries the test-harness run: 571/571 PASS, ORBIT TOOLS GATE OK) |
| Harness Lockstep | PASS (CI job Harness Lockstep, SUCCESS; PR body also carries the lockstep-check output → HARNESS LOCKSTEP OK: 6 pairs checked) |
| Harness Execution (dimension 15 gate) | The PR body's pasted harness output already shows the new "merge sweep CLI flag guard" cases passing for both twins |
| Build (api) | N/A — orbit-api not touched |
Deferred — N/A dimensions & files not verdicted
- Dimensions 8-14 (DESIGN.md, Parity, i18n, Contract drift, Security, Backend hard rules, FEATURES.md): N/A — diff touches only tooling scripts, no
apps/*,packages/shared, ororbit-apisurface.
All three changed files (merge-sweep-cov.sh, merge-sweep.sh, test-tools.mjs) received a verdict above.
What's good
- Root-cause fix, not a workaround: the broken flag combination is removed outright.
- Proven against the real GitHub API (PR #642), not just asserted.
- A regression guard is added in the same PR so this exact class of bug cannot recur unnoticed.
- Both merge-sweep twins changed in lockstep, consistent with the harness lockstep contract.
- Unusually rigorous PR body evidence: live before/after CLI output, the new static case's before/after PASS→PASS flip, and a full harness run pasted verbatim.
Recommendation
Approve. Small, well-evidenced, root-cause bug fix with its own regression coverage added in the same change. No Critical or High findings survive review. This branch's only change since the prior review is a no-op merge from main.
|
READY-TO-MERGE 9c6946c |
1 participant
ORB-141
Summary
--slurpflag from the paginated review GraphQL lookup in both merge sweep twins.--slurpcombined with--jqor--template.--paginateand forbid--slurp.Root cause and impact
GitHub CLI 2.88.1 rejects
--slurpwhenever--jqor--templateis present. The review lookup therefore always exited nonzero, and the correct fail-closed behavior skipped every pull request. The fix removes only the impossible invocation. Review activity definitions, cutoff semantics, malformed response handling, and fail-closed behavior are unchanged.Live GitHub CLI evidence
Executed against the real GitHub API and PR #642.
Before:
After:
Acceptance evidence
The new static case was calibrated against the original invocation before the fix:
The same case passes after the fix:
The existing decision paths prove the required behavior remains reachable in both twins:
UNMET: The ticket requests a real end-to-end merge of a qualifying pull request. The standing worker contract forbids merging any pull request and forbids touching another ticket. No real pull request was merged. The hermetic harness reaches the merge decision and asserts the merge call for both scripts.
Gates
npm run lintnpm run type-checknpm run testnode tools/check-lockstep.mjsnode tools/test-tools.mjsDecisions taken unattended
origin/mainbefore editing because the launched worktree base did not contain the strict sweep implementation named by ORB-141. This exposed the exact defect described by the ticket without introducing unrelated changes.