Reject acceptance criteria with no provable finish line - #638
Conversation
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
|
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 #638 — Reject acceptance criteria with no provable finish line
Scope: PR #638 in thomasluizon/orbit-ui-mobile — tools/check-ticket.mjs, tools/test-tools.mjs
Recommendation: APPROVE
| Severity | Count |
|---|---|
| Critical (incl. |
0 |
| High | 0 |
| Medium | 0 |
| Low / Info | 2 |
Summary
Adds two regex-driven checks to check-ticket.mjs's acceptance-criteria validation: reject a criterion that quantifies over an open set (every/all/any/each) unless the same line names what bounds it (a count, a backticked command/file, an extension, or listed/named/above/below/in the table), and reject a criterion that trails into an unnamed remainder (etc., and so on, an ellipsis). Both new decision paths get dedicated test cases in tools/test-tools.mjs, and CI's Harness Execution job (which runs node tools/test-tools.mjs + node .claude/hooks/test-hooks.mjs) is green on this PR. The change is small, self-contained, and matches its stated intent with no regressions to the existing gate's behavior.
Findings
Critical
None.
High
None.
Medium
None.
Low / Info
[INFO] Multi-line criteria items are only checked on their first line
· dimension: 1. Correctness
· location: tools/check-ticket.mjs:66-69
· issue: `criteriaItems` is extracted via /^[ \t]*(?:[-*]|\d+\.)[ \t]+.*$/gm, which captures only the bullet's own line. If a criterion wraps onto a continuation line without a new marker, an "every X, etc." tail or an unbounded quantifier on that continuation line is invisible to both new checks.
· risk: Low in practice — every criterion in the repo's own fixtures (VALID_TICKET_BODY, the 6.2 template) is single-line, and this matches the prior counting behavior (the old regex also only matched the marker line). Worth a one-line note if multi-line criteria ever become idiomatic.
· fix: no action needed now; if multi-line bullets appear, extend the item regex to consume continuation lines (e.g. up to the next marker or blank line).
· reference: CLAUDE.md rule 1 (root cause) — informational only, not a defect in this diff.
[INFO] Quantifier words used in a domain-bounded (not enumerable-set) sense will be rejected without a marker
· dimension: 1. Correctness
· location: tools/check-ticket.mjs:48-51
· issue: BOUNDED_BY requires a digit, a spelled-out number, a backticked span, a file extension, or an explicit "listed/named/above/below/in the table" cue. A criterion like "the button disables when all fields are filled" is bounded by domain context (a fixed, known form) but carries none of those markers, so it would be rejected and need rewording (e.g. add a digit or backtick).
· risk: Adds authoring friction on some criteria that are not actually open-set — a false positive, not a false negative. No concrete example found of this breaking an existing committed ticket template or fixture (searched .claude/skills/ticket, _shared/audit-to-tickets.md — no matching wording), so this is a design-tradeoff observation, not a reproduced defect.
· fix: none required; the PR body already frames this as a deliberately partial gate with measured hit/miss data. If false positives prove common in practice, widen BOUNDED_BY (e.g. a domain-noun list) rather than loosening the quantifier match.
· reference: PR #638 body, "This is a partial gate, deliberately shipped as one."
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/* or endpoints.ts changed; single-repo diff |
| security-reviewer | N/A — no orbit-api code changed (sibling repo also not checked out) |
| design-reviewer | N/A — no apps/* or orbit-landing-page UI files changed |
Validation
| Check | Result |
|---|---|
| Lint | PASS (CI: PR Tests / Lint, SUCCESS) |
| Type check | PASS (CI: PR Tests / Type Check, SUCCESS) |
| Build | PASS (CI: PR Tests / Build, SUCCESS) |
Tests / Harness Execution (node tools/test-tools.mjs + node .claude/hooks/test-hooks.mjs) |
PASS (CI: Guards / Harness Execution, SUCCESS) |
| SonarCloud | PASS (SUCCESS) |
| Dash Ban / Copy Register / Suppressions Ratchet / Cross-Platform Parity / Skill and Agent Frontmatter / Expo SDK Pin / Harness Lockstep | all PASS (Guards, SUCCESS) |
Per task instructions, Phase 6 (/validate) was skipped — Build/Unit Tests/SonarCloud are covered by required CI checks, cited above as PASS.
Deferred — N/A dimensions & files not verdicted
- Dimension 8 (DESIGN.md / AI-slop): N/A — diff touches no
apps/*UI files. - Dimension 9 (Parity): N/A — diff touches no
apps/web/**orapps/mobile/**. - Dimension 10 (i18n): N/A — no user-facing strings or i18n JSON touched.
- Dimension 11 (Contract drift + backward-compat): N/A — no
packages/shared/src/types/*, no orbit-api DTOs in this diff. The orbit-api side of the backward-compat guard is not verifiable in this CI job (sibling repo not checked out), but is moot regardless since this diff carries no contract changes. - Dimension 13 (Backend hard rules): N/A —
orbit-apiuntouched (and not checked out in this session). - Dimension 14 (FEATURES.md parity): N/A — internal tooling change, no user-facing feature/screen/tool/gating surface changed.
- Prior review threads: none exist on this PR (reviews: [], no inline comments) — nothing to reconcile against; this is a full first-pass review.
Both changed files (tools/check-ticket.mjs, tools/test-tools.mjs) received a full verdict; nothing in scope was left unverdicted.
What's good
- The new checks are narrowly scoped, each backed by its own dedicated positive and negative test case (new decision paths get their own cases, not just extended existing ones).
- The JSDoc comments on the new regex constants cite the concrete motivating incident (PR #633 / ORB-122, 24 review rounds) with real measured numbers rather than vague rationale.
- The PR body is unusually honest about the gate's limits: it states plainly what it catches (2/4 non-converging tickets) and what it misses, rather than overclaiming completeness.
- No regression risk to the existing
>= 2 checkable itemscheck — the extraction regex was widened to capture full lines but the counting semantics are preserved and covered by pre-existing tests.
Recommendation
Merge as-is. No Critical or High findings survive review. The two Info-level observations (multi-line criteria blind spot, occasional false-positive friction on domain-bounded quantifiers) are worth keeping in mind if ticket-writing patterns evolve, but neither is a defect in this diff and neither should block merge.
![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: 953badf6f3
ℹ️ 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".
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
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. 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.
Review Complete
Scope: PR #638 in thomasluizon/orbit-ui-mobile — tools/check-ticket.mjs, tools/test-tools.mjs
Recommendation: APPROVE
Prior review context: This same reviewer (claude) already approved commit 953badf6 (0 Critical/High, 2 Info). Since then, one new commit landed — da82721a "fix(tools): tie the bound to the clause its quantifier is in" — which is what this pass reviewed. (An intervening merge-main commit, 7d2896f1, pulled in unrelated ORB-129 test-tools.mjs changes from main; those are not part of this PR's diff and were excluded from review.)
What the new commit does: BOUNDED_BY previously searched the whole criterion string, so an unrelated token elsewhere in the sentence could rescue an unbounded quantifier claim — e.g. "every phrasing a worker could emit is blocked and the command exits 1" passed only because of the stray 1. The fix adds quantifierClauses(), which masks backtick spans and file-extension paths first (so a backticked command's own dots don't get split), then splits the criterion into clauses on . ; : , / and / or, and requires every clause that contains a quantifier word to independently satisfy BOUNDED_BY. One new regression test (tools/test-tools.mjs) directly covers the fixed bypass and asserts status 1 / quantifies over an open set.
Hand-traced against all six scenarios the commit message claims are correct (the bypass, a comma-separated "see file" reference, a backticked-command bound, two spelled-out-number bounds, and an "any" with no bound) — all matched the claimed behavior. One theoretical edge case exists (a quantifier word that exists only inside a masked backtick/file span vacuously passes .every() on an empty clause list) but it is not a regression — the pre-fix code also passed that shape via the same backtick match — and no realistic ticket-writing pattern reaches it, so it does not rise to a reportable finding.
| Severity | Count |
|---|---|
| Critical | 0 |
| High | 0 |
| Medium | 0 |
| Low / Info | 0 |
Subagents: parity-checker, i18n-syncer, contract-aligner, security-reviewer, design-reviewer — all N/A (diff touches only tools/*.mjs, no apps/, packages/shared/src/types, or orbit-api).
Validation: All completed CI checks are SUCCESS (Lint, Type Check, Build, Dash Ban, Copy Register, Suppressions Ratchet, Cross-Platform Parity, Expo SDK Pin, Skill/Agent Frontmatter, Harness Lockstep, Contract Drift, CodeQL, GitGuardian, Dependency Review/Audit, Architecture map drift). Harness Execution, Unit Tests, and SonarCloud Analysis run in parallel as separate required checks per this CI job's instructions and were not re-validated locally in this pass.
Deferred: Dimensions 8–14 (DESIGN.md, Parity, i18n, Contract drift/backward-compat, Security, Backend hard rules, FEATURES.md parity) — N/A, no matching surface in this diff. The orbit-api side of the backward-compat guard is not verifiable in this CI job (sibling repo not checked out), but is moot since this diff carries no contract changes.
…ed 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
|
There was a problem hiding this comment.
Code Review: PR #638
Recommendation: APPROVE
Summary
tools/check-ticket.mjs gains a heuristic that rejects an acceptance criterion quantifying over an open set (every/all/any/each with no in-clause bound) or trailing into an unnamed remainder (etc., ..., "or similar"). The diff is tooling-only (tools/check-ticket.mjs, tools/test-tools.mjs) and touches no apps/*, packages/shared, or orbit-api surface. It ships with four new hermetic test-tools.mjs cases covering every new decision path. The PR's own commit history shows two fixup commits (da82721a, 832c35c8) that already closed a real review bypass ("...is blocked and the command exits 1") and a hyphenated-compound false positive (all-PASS) found while this PR was under review. Hand-tracing the regex logic against both the new and existing fixtures found no surviving Critical/High defect.
Findings
Critical / High / Medium
None.
Low / Info
- Intentional, disclosed tradeoff, not an oversight:
isBounded(tools/check-ticket.mjs:60-68) only accepts a bound in the quantifier's own clause or a clause before it, never after. A criterion like"every scenario is covered: A, B, C, three total"still gets flagged, because the enumeration trails the quantifier's clause. This is the deliberate fix (832c35c8) for the exact bypass direction this PR's own review found, and the rejection message already tells the author the fix (state the bound in-clause, or backtick it). No action needed.
Subagents
N/A across the board — no apps/*, packages/shared types, or orbit-api file changed, so parity-checker / i18n-syncer / contract-aligner / security-reviewer / design-reviewer all sit outside their gate.
Validation
Lint / Type Check / Unit Tests all PASS per the PR's own CI rollup (PR Tests workflow, run 30370677394). Guards / Harness Execution PASS (run 30370677100, job 90313392105). This session's sandbox blocked a local re-run of node tools/test-tools.mjs / node .claude/hooks/test-hooks.mjs (arbitrary script execution needs interactive approval unavailable here), so validation is sourced from the PR's green statusCheckRollup plus the author's own gate output quoted in the PR body.
Deferred
DESIGN.md/AI-slop (#8), Parity (#9), i18n (#10), Contract drift (#11), Backend hard rules (#13), FEATURES.md parity (#14): all N/A, no matching surface in the diff. Type safety (#6): N/A in the strict TS sense (tools/** is plain untyped .mjs outside the ESLint-covered workspaces); no any/as any/unjustified null! present regardless. Both changed files were read in full and given a verdict.
What's good
- Self-aware about its own limits: names the two tickets its heuristic misses (ORB-129, ORB-113) and why, instead of overclaiming completeness.
- Every new decision path gets its own
test-tools.mjscase pertools/CONVENTIONS.md/ rubric dimension 15. - Already shows one full review-and-fix cycle against itself in its commit history, and the fixes are correctly reasoned.
Recommendation
Merge as-is. No action items.
1 participant
Problem
PR #633 (ORB-122) has taken 24 review rounds over 12 hours and has not converged. PR #625 (ORB-106) is at 32 reviews. Neither is a bad worker or a noisy reviewer: I read the review that un-approved #633 and it is a real HIGH, traced against the actual blob with a working bypass string and a two-line fix.
The cause is the ticket. ORB-122 asks the worker to block every phrasing an agent could emit that surfaces a raw repo command. That is not a set anyone can enumerate, so every round the reviewer legitimately finds one more member of it and is correct. There is no state of the code that satisfies the criterion, so review cannot terminate.
Its last two commit headlines say it plainly:
cover node invocation variants,close remaining surfacing gaps.What this adds
check-ticket.mjsnow rejects an acceptance criterion that:every/all/any/eachwith nothing in the same criterion bounding it), oretc.,and so on,or similar, an ellipsis).A criterion is bounded when it names its enumeration: a count (digits or
one..ten), a file path, a backticked command, or an explicitlisted/named/in the tablereference. So this stays legal and is better written:every phrasing rejected by+ "node tools/check-ticket.mjs" +is blockedand this is rejected:
every phrasing a worker could emit is blockedWhere it fires
No new checkpoint. The gate already runs everywhere a ticket passes:
/ticketand/featureon the draft and again on the created issue,/orchestrateper ticket at wave-plan time (so it catches tickets written before this rule existed), and the four/audit-*skills plus/prod-readinesson what they propose.What it actually buys, measured
Run against all 20 harness ticket bodies in the current run, correlated with each PR's review count:
It catches two of the four non-converging tickets and misses two. ORB-129 and ORB-113 stalled for other reasons (a screen-scraping predicate over a TUI, and a merge conflict), not unbounded criteria, so no wording rule would have caught them. The two low-review tickets it stops are not false alarms in kind: both say "every X" without naming where X is enumerated, and the fix is one edit that makes the criterion checkable.
This is a partial gate, deliberately shipped as one. It does not claim to detect every unbounded ticket, which would be the same mistake the gate exists to prevent.
Gates
Three new cases in
tools/test-tools.mjs: the open-set rejection, the same criterion passing once it names its deciding command, and the unnamed-remainder rejection.🤖 Generated with Claude Code
https://claude.ai/code/session_01G3TtwQhtWZdAndmpfmf8rC