fix: forbid draft worker pull requests - #655
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
No code change required: IC_kwDOR5Siws8AAAABMMx_eg reports a skipped Vercel deployment and contains no code finding. Evidence: 51e9eec |
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review: PR #655 (orbit-ui-mobile)
Scope: PR #655 — fix: forbid draft worker pull requests (ORB-153)
Recommendation: APPROVE
Summary
Small, surgical tooling change: workers are now forbidden from leaving a pull request as
a draft. AGENTS.md and the injected WORKER_CONTRACT in tools/launch-worker.mjs both
carry the new clause verbatim; tools/pr-watch.mjs gains a draft transition (checked
before checks-failed, exits code 1) so the merge-decision poller refuses a draft even
if it otherwise reads clean/approved; tools/worker-status.mjs splits its old
pr-open check (which silently folded in !isDraft) into a distinct pr-open +
pr-ready-for-review check, giving a worker a precise, separately-named reason for the
refusal. All three new decision paths get dedicated tools/test-tools.mjs cases rather
than piggybacking on existing assertions. Manual trace of transitionOf and the
checks array confirms the logic is correct (draft is checked ahead of the other
terminal transitions; pr-ready-for-review is independent of pr-open so a draft PR
now fails exactly one check, matching the new test's assertion).
The PR body reported the full node tools/test-tools.mjs harness timing out twice
locally (124s, then 604s) with no output, and marked it explicitly UNMET per the
worker contract's "a blocked sub-step never blocks the PR" clause. That looked like a
real evidence gap at first read (rubric dimension 15 requires an executed harness run,
not a code read) — but this repo's own required Guards / Harness Execution CI check,
which runs exactly node tools/test-tools.mjs + node .claude/hooks/test-hooks.mjs,
has since completed SUCCESS (started 05:38:45Z, completed 05:44:39Z), independently
confirming all three new decision paths pass. That resolves the concern; the PR body's
own evidence log is just stale relative to what CI later proved.
Findings
Critical
None.
High
None.
Medium
[Medium] tools/README.md's pr-watch.mjs catalog entry omits the new draft transition
· dimension: 15 (Harness changes — doc/catalog accuracy) / general correctness of the tool catalog
· location: orbit-ui-mobile/tools/README.md:56
· issue: The catalog row for pr-watch.mjs enumerates every transition the tool can emit
(gone, checks-failed, changes-requested, review-comment, approved,
ready-to-merge, head-changed, review-decision, merge-clean, timeout). This PR
adds a new draft transition (tools/pr-watch.mjs:211) and updates the tool's own
JSDoc header comment to list it, but leaves tools/README.md unchanged.
· risk: A worker or agent that reads tools/README.md (the catalog CLAUDE.md points
agents to instead of exploring the codebase) to learn pr-watch.mjs's contract will
not know draft is a possible exit and may mishandle it in any future logic that
branches on the transition name.
· fix: Add draft to the parenthetical transition list in tools/README.md:56 (it reads
naturally right after gone, matching the order in pr-watch.mjs's own header
comment).
· reference: CLAUDE.md rule 1 (root cause / keep the source of truth accurate) and the
"Maximum implementation" directive (fix stale docs you touch in the same PR).
Low / Info
None posted (signal gate).
Subagents
| Agent | Verdict |
|---|---|
| parity-checker | N/A — no apps/web/** or apps/mobile/** file changed |
| i18n-syncer | N/A — no user-facing strings or packages/shared/src/i18n/* changed |
| contract-aligner | N/A — no packages/shared/src/types/* / endpoints.ts / orbit-api DTO changed |
| security-reviewer | N/A — no orbit-api code changed |
| design-reviewer | N/A — no apps/web, apps/mobile, or landing-page UI file changed |
Validation
All from the PR's own live CI (gh pr view 655 --json statusCheckRollup), not a local
re-run — see Deferred.
| Check | Result |
|---|---|
Lint (PR Tests / Lint) |
PASS |
Type check (PR Tests / Type Check) |
PASS |
Build (PR Tests / Build) |
PASS |
Harness Execution (node tools/test-tools.mjs + node .claude/hooks/test-hooks.mjs) |
PASS (SUCCESS, completed 05:44:39Z) |
| Dash Ban / Copy Register / Cross-Platform Parity / Suppressions Ratchet / Expo SDK Pin / Contract Drift / Harness Lockstep | PASS (all green) |
Unit Tests (PR Tests / Unit Tests) |
IN PROGRESS at review time — not yet resolved |
Deferred — N/A dimensions & files not verdicted
- Dimension 8 (DESIGN.md / AI-slop) — deferred, no
apps/*UI file in the diff. - Dimension 9 (Parity) — deferred, no
apps/weborapps/mobilefile in the diff. - Dimension 10 (i18n) — deferred, no locale/user-facing-string file in the diff.
- Dimension 11 (Contract drift) — deferred, no
packages/shared/src/types/*or
orbit-api DTO in the diff. - Dimension 13 (Backend hard rules) — deferred, no
orbit-apifile in the diff. - Dimension 14 (FEATURES.md parity) — deferred, this is internal worker-tooling
behavior, not a user-facing product feature. - Local validation re-run — not performed in this session; this Bash environment
requires interactive approval fornode tools/test-tools.mjs,gh run view, and
similar commands that this review session could not grant itself, so validation
evidence above is CI's own result, not a fresh local run. PR Tests / Unit Tests— still IN_PROGRESS as of this review; not yet confirmed
green (the rootnpm run testresult the PR body separately reports as
"4 successful, 4 total" is consistent with this eventually passing, but the live CI
check itself had not completed at review time).
All five changed files (AGENTS.md, tools/launch-worker.mjs, tools/pr-watch.mjs,
tools/test-tools.mjs, tools/worker-status.mjs) received a verdict above.
What's good
- The three new decision paths (
drafttransition,pr-ready-for-reviewcheck, the two
contract-clause presence checks) each got their own dedicatedtools/test-tools.mjs
case rather than an extension of an existing assertion, exactly as rubric dimension 15
asks. - Splitting
worker-status.mjs's oldpr-open(which silently folded in!isDraft)
intopr-open+pr-ready-for-reviewis a real improvement: a worker now gets a
precise, separately-named reason instead of a generic "PR not open." - The new clause is worded and placed identically (byte-for-byte) in both
AGENTS.md
and the injectedWORKER_CONTRACTintools/launch-worker.mjs, and both are checked
by dedicated harness assertions — exactly the contract-parity pattern this repo's
worker harness relies on. - The PR body was honest about the local harness timeout rather than silently dropping
the evidence or overclaiming a pass, per the worker contract's own "unmet and stated
is acceptable" rule.
Recommendation
Approve. Optionally fold the one Medium doc fix (tools/README.md:56) into this PR or a
fast-follow before the next worker reads that catalog for pr-watch.mjs's contract.
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. 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". |
1 participant
ORB-153\n\nPrevents workers from opening or treating draft pull requests as review-ready.\n\n## Evidence\n\n- Contract parity: AGENTS.md and ools/launch-worker.mjs both contain: The pull request must be ready for review, never a draft.\n- Contract-removal harness assertions: launch-worker.mjs: the injected contract forbids opening a draft pull request and AGENTS.md: the standing worker contract forbids opening a draft pull request. Each independently fails if that source loses the clause.\n- Status-reader refusal: harness case worker-status.mjs: a draft pull request is explicitly not ready for review expects exit 1, the sole unmet check pr-ready-for-review, and detail naming draft pull request. The unchanged ready fixture continues to require an OK result.\n- Merge-decision refusal: harness case pr-watch.mjs: a draft reading clean and approved is refused expects exit 1 with ransition: draft and he PR is a draft and cannot be merged.\n\n## Gates\n\n
ext\nnpm run lint\nexit 0\nTasks: 3 successful, 3 total\n\nnpm run type-check\nexit 0\nTasks: 3 successful, 3 total\n\nnpm run test\nexit 0\nTasks: 4 successful, 4 total\n\nnode --check tools/launch-worker.mjs\nnode --check tools/worker-status.mjs\nnode --check tools/pr-watch.mjs\nnode --check tools/test-tools.mjs\nnode tools/check-dashes.mjs --files AGENTS.md tools/launch-worker.mjs tools/worker-status.mjs tools/pr-watch.mjs tools/test-tools.mjs\nnode tools/check-copy.mjs --check\ngit diff --check\nall exit 0\n\nnode tools/test-tools.mjs\nUNMET: timed out after 604 seconds without output. Reproduced after an earlier 124-second timeout.\n\n\n## Decisions taken unattended\n\n- Treated ools/pr-watch.mjs as the merge-decision path because it emits theeady-to-merge transition from merge state and review decision.\n- Kept the full tools harness timeout explicit while completing every other required gate and the PR handoff, following the standing contract for a genuinely blocked sub-step.