ORB-155 gate ledger child filing threshold - #650
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
![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: f6567a23a8
ℹ️ 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: Didn't find any major issues. What shall we delve into next? 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". |
…the-ledger-a-filing-threshold
|
Automated activity reconciliation for current head
|
|
No code change required for automated activity |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 398e822ff3
ℹ️ 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 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review: PR #650 — ORB-155 gate ledger child filing threshold
Scope: PR #650 in thomasluizon/orbit-ui-mobile (feature/orb-155-give-the-ledger-a-filing-threshold → main)
Recommendation: NEEDS WORK
Summary
The PR replaces the "one child per ledger entry" repair rule with a 3-occurrence-or-blocking threshold, adds a fixed Ledger occurrence: <count>; blocked: no|<what it blocked> line to the ticket template contract, and teaches tools/check-ticket.mjs to detect ledger children (via issue.parent, a relations entry, or a new live Linear API fallback) and validate that line. The diff is scoped to three files: .claude/skills/orchestrate/SKILL.md, tools/check-ticket.mjs, tools/test-tools.mjs — no apps/*, orbit-api, or packages/shared surface is touched. Two concrete defects survived an adversarial skeptic pass: the new occurrence-threshold check silently stops validating the blocked claim once the count reaches 3, and the new live-Linear-API fallback path ships with zero automated test coverage despite being the PR's stated central mechanism.
Findings
Critical
None.
High
[High] validateLedgerOccurrence never validates the blocked claim once the occurrence count reaches the threshold
- dimension: 1. Correctness
- location:
tools/check-ticket.mjs:191-212 - issue:
if (occurrenceCount >= LEDGER_OCCURRENCE_THRESHOLD) return(line 203) exits before theblockingClaimbare-claim check (lines 204-208) ever runs. That check only fires whenoccurrenceCount < 3. - risk: A ledger child stating
Ledger occurrence: 5; blocked: yes(orblocked:with a stray character) passescheck-ticket.mjseven thoughblockedis neither the literalnonor a real description — violating the fixed line's ownno|<what it blocked>contract, which the PR description itself states applies unconditionally ("checker failures for missing, malformed, below-threshold, and bare blocking claims"). None of the 8 new tests exercise a bare claim at or above the threshold. Confirmed via an independent skeptic pass: no other code path validatesblockedcontent once the threshold check returns. - fix: Validate
blockingClaim(reject bare/empty values) unconditionally, before or independent of the threshold early-return; useoccurrenceCountonly to decide whetherblocked: noalone is sufficient. - reference: PR body's own stated contract ("bare blocking claims" as unconditional);
.claude/skills/orchestrate/SKILL.md's "a blocking value must name … that could not proceed."
[High] The new Linear-parent network fallback and relations-based parent detection ship with zero harness coverage
- dimension: 15. Harness changes need EXECUTED evidence
- location:
tools/check-ticket.mjs:145-189(isLedgerChild'srelationsbranch,readLinearParent's HTTPS/GraphQL call and its error/missing-key throws);tools/test-tools.mjs:4742-4787(the 8 new cases) - issue: Every new test sets
issue.parentdirectly and mocksrelations: [].VALID_ISSUE(test-tools.mjs:3265) has noidfield, soreadLinearParent's first guard (!issue.id || Object.hasOwn(issue, "parent")) returns early before the HTTPS call in every test. No test omitsparentwhile supplying anid, and none supplies a parent viarelationsinstead ofissue.parent. - risk: The actual network call (success path), the "fail closed" error-exit path, the missing/empty API-key-file throws, and the
relations-array detection branch are all unexercised bytools/test-tools.mjs. The PR's acceptance evidence proves this mechanism only via a one-off manual run against a live ticket (ORB-154), not via the automated Harness Execution gate. Confirmed via an independent skeptic pass. - fix: Add cases that (a) supply an
idwith noparentand stub/inject the HTTPS call to prove the success path, (b) prove the error path exits 2 with the expected stderr, (c) proveisLedgerChilddetects a parent via arelationsentry. - reference: rubric dimension 15 ("a new decision path … needs its own case"; "'Verified' without an execution is itself a finding").
Medium
[Medium] New raw Node https Linear client, no timeout, diverges from the repo's sanctioned curl idiom, fires on every ticket check
- dimension: 5. No-workaround / root-cause
- location:
tools/check-ticket.mjs:154-189 - issue:
readLinearParenthand-rolls anhttps.requestPOST toapi.linear.app/graphqlwith no timeout. This repo already has a sanctioned, gate-recognized way to make this exact call —curl -H "Authorization: $(cat ~/.linear-api-key)"againstapi.linear.app/graphql, documented in.claude/hooks/_lib/rules-linear.mjs:67-68and referenced from.claude/skills/orchestrate/SKILL.md(D36) — and the PR introduces a second, novel mechanism instead, with no WHY-with-URL comment justifying the divergence (CLAUDE.md rule 1 / rubric dimension 5). - risk: Per the PR's own evidence, the installed Orca build omits
parenton every issue payload, so this fallback fires on every--issuecheck, not only ledger children, making a live Linear round-trip +~/.linear-api-keyread a hard dependency of the whole ticket-validation gate. With no timeout, a stalled connection blockscheck-ticket.mjs(and anything shelling out to it) indefinitely with no diagnostic. - fix: Add an explicit request/socket timeout with a clear error, and either reuse the existing
curlidiom for consistency or add a one-line WHY-with-URL comment. - reference: CLAUDE.md rule 1; rubric dimension 5;
.claude/hooks/_lib/rules-linear.mjs:67-68.
Low / Info
None posted (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 packages/shared/src/types/*/endpoints.ts, no orbit-api change |
| security-reviewer | N/A — no orbit-api code changed |
| design-reviewer | N/A — no UI files changed |
Validation
| Check | Result |
|---|---|
| Lint | NOT RE-RUN this session (sandbox blocked); PR body reports npm run lint exit 0 at head 398e822f |
| Type check | NOT RE-RUN this session; PR body reports npm run type-check exit 0 |
| Tests | NOT RE-RUN this session; PR body reports npm test, node tools/test-tools.mjs, node .claude/hooks/test-hooks.mjs all exit 0 |
| Build (api) | N/A — orbit-api not touched |
Deferred — N/A dimensions & files not verdicted
- DESIGN.md / AI-slop (#8), Parity (#9), i18n (#10): N/A — no
apps/*UI/i18n files changed. - Contract drift + backward-compat (#11) / Phase 5 guard, Security (#12 API)/Backend hard rules (#13): N/A — no
packages/shared/src/types/*ororbit-apitouched. - FEATURES.md parity (#14): N/A — internal orchestrator policy/gate change, not an in-app feature surface.
- Validation re-execution: this review session's shell tool could not obtain approval to re-run
npm run lint/type-check/tools/test-tools.mjs/test-hooks.mjs, so the validation table reflects the PR author's self-reported run, not an independent re-execution. - All three changed files (
SKILL.md,check-ticket.mjs,test-tools.mjs) received a verdict.
What's good
- The threshold logic's core shape is a clean, well-isolated addition; the pre-existing
blockedBy/dependency-prose checks are untouched. - The "fails closed" intent for an unreadable parent relation (
process.exit(2)) is the right default for a gate meant to catch defects before a worker burns a worktree. - Good test hygiene elsewhere: below-threshold, at-threshold, missing-line, unparseable-line, and non-ledger-parent cases are distinct, well-named, and assert on both stdout and stderr shape.
isLedgerChildcorrectly restricts ledger validation to children of a markedHarness defect ledgerparent (the "Review fix" commit), so an ordinary child with an unrelated parent is unaffected.
Recommendation
Fix the two High findings before merge: (1) make the bare-blocking-claim check apply regardless of occurrence count, with a test at count ≥ 3, and (2) add real coverage (success path, error path, and the relations-based detection branch) for the new Linear fallback in tools/test-tools.mjs. Address the Medium finding (timeout + idiom consistency) in the same PR or an immediate fast-follow, since the fallback fires on every ticket check today.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 398e822ff3
ℹ️ 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 Please review current head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c108bff6f
ℹ️ 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 Please review current head |
There was a problem hiding this comment.
Code Review: PR #650
Scope: PR #650 in thomasluizon/orbit-ui-mobile — ORB-155 gate ledger child filing threshold
Recommendation: NEEDS WORK
Summary
The PR replaces the one-child-per-ledger-entry rule with a filing threshold (3 occurrences or a
run-blocking defect), adds a fixed Harness defect ledger parent marker and Ledger occurrence: <count>; blocked: no|<what it blocked> child line, and extends check-ticket.mjs to enforce
both. The threshold/format logic itself is well-tested and correct. The new "prefer Orca parent
data, fall back to a direct Linear lookup" logic, however, misclassifies the common "this ticket
has no parent at all" case as "Orca's parent data is incomplete," so it fires a live, untested,
timeout-less network call on ordinary tickets rather than only on genuine ledger-parent stubs —
and this checker gates every worker launch in /orchestrate, not only ledger children.
Findings
Critical
None.
High
[HIGH] Parent-classification bug turns the universal ticket gate into a live, untested, timeout-less network dependency
· dimension: Correctness (#1)
· location: tools/check-ticket.mjs:145-154 (parentFromOrca), tools/check-ticket.mjs:160-195 (readLinearParent), tools/check-ticket.mjs:249-261 (the resolution block)
· issue: parentFromOrca returns undefined when Orca's issue payload has no parent key and no parent/childOf relation — i.e. "this ticket definitively has no parent," the same fact an explicit parent: null represents. But parentIsClassifiable (line 250) only treats null as classifiable; undefined falls through to readLinearParent, which fires a live HTTPS POST to https://api.linear.app/graphql whenever issue.id is set and $env:USERPROFILE\.linear-api-key exists — the exact personal-key file this same skill already establishes as Thomas's real dev-machine setup (.claude/skills/orchestrate/SKILL.md:61-62).
· risk: node tools/check-ticket.mjs --issue ORB-N is the universal ticket gate run before every worker launch (.claude/skills/orchestrate/SKILL.md:133), not just for ledger children. readLinearParent sets no timeout on its https.request (no .setTimeout(), no timeout option, no abort), so a network partial failure hangs the whole check indefinitely instead of failing fast; an outright API/network error exits 2 (documented as the tool's "usage error" code) for an entirely unrelated, otherwise-valid ticket. This isn't hypothetical: the PR's own new tests "an Orca parent relation validates without a separate Linear key" and "a standalone Orca issue validates without a separate Linear key" (tools/test-tools.mjs, new ledger block) both have to point USERPROFILE/HOME at a directory with no .linear-api-key to avoid hitting this exact branch — direct evidence it fires for ordinary, non-ledger tickets whenever the key is present. Verified independently by an adversarial skeptic pass, which confirmed the mechanism and the missing timeout.
· fix: Treat "no parent key and no parent-type relation at all" the same as an explicit parent: null — both mean "definitively no parent," so skip the Linear fallback for that case. Reserve the Linear fallback for when a parent signal exists (a non-null issue.parent, or a parent/childOf relation) but its object lacks title/description. Separately, add a timeout to the https.request call ({ timeout: <ms> } plus a req.on("timeout", () => req.destroy(new Error(...))) handler) so a network partial failure fails fast instead of hanging the process.
· reference: CLAUDE.md rule 1 (no defensive/fallback branch beyond what its trigger actually requires); rubric dimension 1; rubric dimension 15 (harness reliability)
Medium
[MEDIUM] Bare/vague blocked: claims escape validation once the occurrence count meets the threshold
· dimension: Correctness (#1) / Harness changes (#15)
· location: tools/check-ticket.mjs:197-219 (validateLedgerOccurrence)
· issue: The bare-claim check (rejecting blocked: yes / true / blocked with nothing named) only runs when occurrenceCount < LEDGER_OCCURRENCE_THRESHOLD (line 209 returns early once the threshold is met). The contract this enforces, per .claude/skills/orchestrate/SKILL.md ("Use no only for a non-blocking defect; a blocking value must name the run step, ticket, or operation that could not proceed"), is not threshold-conditional — it describes the fixed line's shape unconditionally.
· risk: Ledger occurrence: 5; blocked: true passes check-ticket.mjs silently even though it names nothing concrete, so a legitimately-filed high-occurrence child can still carry an unusable blocked value.
· fix: Run the bare-claim regex check regardless of occurrenceCount; only skip the "below-threshold-without-a-name" alias rejection once the threshold is met, not the bare-claim rejection.
· reference: CLAUDE.md rule 1; rubric dimension 15 ("a new decision path...needs its own case")
[MEDIUM] readLinearParent's actual network branches (success and API-error) have zero executed test coverage
· dimension: Harness changes need EXECUTED evidence (#15)
· location: tools/check-ticket.mjs:160-195; tools/test-tools.mjs (new ledger test block, ~lines 4907-4995)
· issue: Every new ledger test either supplies an already-classifiable parent (so readLinearParent is never entered) or points USERPROFILE/HOME at a key-less directory (so the function returns at the existsSync check before touching the network, tools/check-ticket.mjs:163). Neither the successful-fetch/parse branch nor the catch → process.exit(2) branch is exercised anywhere in tools/test-tools.mjs.
· risk: The one new external-network code path this PR adds ships with no executed proof it parses a real Linear GraphQL response or handles a real API/network error correctly — exactly the class of gap rubric dimension 15 exists to catch ("a new decision path... needs its own case, not an extension of an existing assertion").
· fix: Add a fixture that stubs the request (e.g. a test-only injection point, or a tiny local HTTP(S) stub the test points readLinearParent at) to cover: (a) a successful GraphQL response resolving a parent stub to its full title, and (b) a non-2xx / GraphQL-error response driving the documented process.exit(2) path.
· reference: rubric dimension 15; TESTING.md
Low / Info
None posted (signal gate: Low/Info are not posted as PR-review 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, and only one repo touched |
| security-reviewer | N/A — no orbit-api code changed |
| design-reviewer | N/A — no apps/* or landing-page UI file changed |
Validation
CI on PR #650 (gh pr view 650 --json statusCheckRollup) already ran the full gate set for this
head commit; local re-execution was unavailable in this review session (sandboxed, no shell-exec
permission), so these are the CI-reported results rather than a local re-run:
| Check | Result |
|---|---|
| Lint | PASS (CI: Guards/PR Tests) |
| Type check | PASS (CI: PR Tests → Type Check) |
| Tests (Unit) | PASS (CI: PR Tests → Unit Tests) |
Harness Execution (node tools/test-tools.mjs + node .claude/hooks/test-hooks.mjs) |
PASS (CI: Guards → Harness Execution) |
| Harness Lockstep | PASS (CI: Guards) |
| Cross-Platform Parity | PASS (CI: Guards — trivially, no apps/* touched) |
| Dash Ban / Copy Register / Context Budget / Suppressions Ratchet / Expo SDK Pin | PASS (CI: Guards) |
| Build (api) | N/A — orbit-api not touched |
Deferred — N/A dimensions & files not verdicted
- Dimension 8 (DESIGN.md/AI-slop), 9 (Parity), 10 (i18n), 11 (Contract drift), 13 (Backend hard
rules): N/A — the diff touches only.claude/skills/orchestrate/SKILL.md,tools/check-ticket.mjs,
andtools/test-tools.mjs; noapps/*,packages/shared/src/types/*, ororbit-apisurface is
in scope. - Dimension 14 (FEATURES.md parity): N/A — this changes internal dev-tooling behavior (the harness
defect-ledger workflow), not a user-facing app feature, screen, tool count, or gating rule. - Dimension 12 (Security): reviewed the new
readLinearParentnetwork call (credential handling,
error-message content) — no secret leakage or injection issue found; the personal-key convention
it follows is pre-established elsewhere in this same skill. - Local
/validateexecution: not run in this session (sandbox blocked shell execution ofnode/
chmod/mkdir); substituted with the PR's own CI check-run results, which were all green for
this head commit at review time. - All three changed files (
.claude/skills/orchestrate/SKILL.md,tools/check-ticket.mjs,
tools/test-tools.mjs) received a verdict above; nothing in the diff was left unexamined.
What's good
- The threshold/format logic itself (
LEDGER_OCCURRENCE_THRESHOLD, the fixed-line regex, the
alias rejection forfalse/none/n/a/no.) is precise and thoroughly tested — 15 new focused
cases intools/test-tools.mjs, each naming one exact decision path. isLedgerParentcorrectly scopes the new validation to children of a marked ledger parent only,
with an explicit regression ("a recorded non-ledger child ticket is unaffected") proving ordinary
children are untouched.- The SKILL.md prose changes accurately describe the new threshold/marker contract and match the
shipped code (the fixed line format, the threshold of three, the marker string). - PR body carries genuine executed evidence (the
tools/test-tools.mjsPASS transcript, lint/
type-check/test exit codes), which is exactly the standard rubric dimension 15 asks for.
Recommendation
Fix the parent-classification bug before merge (High): treat "no parent signal from Orca at all"
the same as parent: null, and add a timeout to the readLinearParent HTTPS request. This is the
blocking item — as written, it turns the gate run before every single worker launch into a
network-dependent, occasionally-hanging check for the common case of an ordinary, non-ledger
ticket. The two Medium findings (the threshold-conditional bare-claim gap, and the untested live
network branches) are good to fix in the same PR but would not block merge on their own.
|
Codex Review: Didn't find any major issues. Keep them coming! 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 Please review current head |
There was a problem hiding this comment.
Review Complete
Scope: PR #650 in thomasluizon/orbit-ui-mobile (feature/orb-155-give-the-ledger-a-filing-threshold → main), head commit 4cfdd06f
Recommendation: REQUEST CHANGES
Note: this review re-runs /pr-review against the current head. Two prior automated reviews on this PR already flagged the parent-classification/network-timeout issue below as High — it remains unresolved at the current head, so it is re-flagged (not a new nit). All three findings below were independently verified against the actual committed code (tools/check-ticket.mjs at current head) and the real PR diff (gh pr diff 650), not just the skill's report.
| Severity | Count |
|---|---|
| Critical | 0 |
| High | 3 |
| Medium | 1 |
| Low / Info | 0 |
Findings
High
[HIGH] Standalone-ticket checks trigger an unwanted live Linear API call, contradicting the PR's own stated design
· dimension: Correctness (#1) / Harness reliability (#15)
· location: tools/check-ticket.mjs:147-156 (parentFromOrca), :247-257 (resolution block)
· issue: parentFromOrca returns undefined when Orca's issue payload has no parent key and no parent/childOf relation — i.e. "this ticket definitively has no parent," the same fact an explicit parent: null represents. But parentIsClassifiable (line 247) only treats null (or an object with a string title/description) as classifiable; undefined falls through to readLinearParent, firing a live HTTPS POST to Linear's GraphQL API whenever issue.id is set and ~/.linear-api-key exists — for every ordinary ticket, not just ledger children.
· risk: check-ticket.mjs --issue gates every worker launch in /orchestrate, not only ledger children. This turns a previously fully-local, deterministic checker into one with a hidden live external dependency for the common case. Flagged as High in two prior reviews on this PR already; still present at the current head.
· fix: Treat "no parent key and no parent-type relation at all" the same as an explicit parent: null (skip the Linear fallback). Reserve the fallback for when a parent signal exists but the object lacks title/description.
[HIGH] No timeout on the new outbound Linear HTTPS call — can hang check-ticket.mjs indefinitely
· dimension: Harness changes need EXECUTED evidence (#15) / Correctness (#1)
· location: tools/check-ticket.mjs:172-191 (readLinearParent)
· issue: The request() call sets no timeout option, no .setTimeout(), no abort signal. Node applies no default timeout, so a connect-then-hang or slow-drip response from Linear never resolves or rejects the promise.
· risk: Combined with the finding above, this is reachable from ordinary ticket checks. A stalled connection blocks check-ticket.mjs (and anything shelling out to it, including /orchestrate and /next) indefinitely with no diagnostic.
· fix: linearRequest.setTimeout(ms, () => linearRequest.destroy(new Error("Linear API timeout"))), treated as an ordinary fetch failure in the existing catch/exit-2 path.
[HIGH] Affirmative-blocking regex accepts a vague claim, undermining the threshold gate ORB-155 exists to add
· dimension: Correctness (#1) / harness decision-path soundness (#15)
· location: tools/check-ticket.mjs:47-48 (AFFIRMATIVE_BLOCKING_CLAIM); documented grammar at .claude/skills/orchestrate/SKILL.md:629-631
· issue: The first regex alternative — \b(?:could not|couldn't|cannot|can't|failed to|(?:was|were) unable to)\s+\S+ — matches could not <any single word> with no requirement that a named operation/subject precede it. Verified: "could not proceed" and "failed to run" both match. This is looser than the SKILL.md grammar this PR itself documents, which requires <operation> could not <action> (an operation named before the modal), blocked the <operation>, or the <operation> was blocked — all three of which require a named subject/operand.
· risk: Ledger occurrence: 2; blocked: could not proceed passes validateLedgerOccurrence, despite being exactly as content-free as the bare blocked: yes the test suite explicitly rejects ("a bare blocking claim does not bypass the threshold"). No test in tools/test-tools.mjs exercises a vague-but-modal-shaped claim like this, so the gap ships silently. Defeats the anti-vagueness purpose of the whole ticket.
· fix: Require the modal phrase to be preceded by a named subject, mirroring the other two patterns' (?:the|a|an)\s+\S+ requirement, and add a rejection test for "could not proceed" / "failed to run".
Medium
[MEDIUM] readLinearParent's network request/response code path has zero test execution
· dimension: Harness changes need EXECUTED evidence (#15)
· location: tools/check-ticket.mjs:172-197; tools/test-tools.mjs ledger test block
· issue: Every test reaching readLinearParent points USERPROFILE at a directory with no .linear-api-key file, so existsSync(keyPath) returns false and the function short-circuits before building the request, parsing a response, or exercising the HTTP-error/GraphQL-error branches.
· risk: The success path, the non-2xx branch, and the payload.errors branch are all unexecuted by the suite that reports "ORBIT TOOLS GATE OK" — a request-shape or response-parsing bug here would ship uncaught.
· fix: Add a test with a stub key file present and the network layer stubbed/injectable, covering success, HTTP-error, and GraphQL-error branches.
Low / Info
None.
Subagents
All five gated subagents were N/A — this diff touches only .claude/skills/orchestrate/SKILL.md, tools/check-ticket.mjs, tools/test-tools.mjs: no apps/web/**, apps/mobile/**, orbit-landing-page/src/**, packages/shared/src/types/*, endpoints.ts, or orbit-api files changed, and no user-facing i18n strings.
| Agent | Verdict |
|---|---|
| parity-checker | N/A — no apps/* files touched |
| i18n-syncer | N/A — no user-facing strings/i18n JSON touched |
| contract-aligner | N/A — no shared types / endpoints / orbit-api touched |
| security-reviewer | N/A — orbit-api not touched |
| design-reviewer | N/A — no apps/* or landing-page UI touched |
Validation
Sourced from live CI (gh pr view 650 --json statusCheckRollup).
| Check | Result |
|---|---|
| Lint | PASS (CI) |
| Type check | PASS (CI) |
| Tests | PASS (CI) |
| Harness Execution | PASS (CI, both matrix legs) |
| Cross-Platform Parity | PASS (CI gate; no apps/* touched) |
| Contract Drift | PASS (CI gate) |
| Build (api) | N/A — orbit-api not touched |
Deferred — N/A dimensions & files not verdicted
- Dimensions 8-11, 13, 14 (DESIGN.md, parity, i18n, contract drift, backend hard rules, FEATURES.md parity): N/A — no
apps/*,packages/shared/src/types,endpoints.ts, ororbit-apifiles touched. - Dimension 12 (Security): reviewed inline — no injection/authz/data-exposure surface beyond the reliability issues above (internal tooling reading a pre-existing sanctioned key-file convention over HTTPS).
- Cross-model second opinion: not fired — scoped to Critical-only findings; none of the three survivors is Critical.
- All three changed files received a full verdict; nothing left unexamined.
What's good
- The core design — parent stays the durable record, a child is filed only for recurring (3+) or blocking defects — is a sound, well-motivated fix for the stated backlog-growth problem.
- Test coverage for everything reachable without a live personal key is thorough and well-named: threshold boundary at exactly 3, below-threshold rejection with count+threshold in the message, six nonblocking aliases, both active and passive affirmative phrasings, malformed/missing occurrence line, and a non-ledger parent correctly left unaffected.
- Preserving
Object.hasOwn(issue, "parent")to honor an explicitparent: nullfrom Orca (skipping an unnecessary lookup when Orca's answer is already complete) is the right instinct — the bug is that the same courtesy wasn't extended to "no parent relation found in a fetched relations array," which is the far more common shape of "complete negative answer." - SKILL.md prose and code agree on the marker string, the threshold number, and the field format everywhere except the one documented-vs-implemented gap in finding 3.
Recommendation
Fix the three High findings before merge. Findings 1 and 2 both concern the new live-Linear-lookup path becoming a hidden, unbounded dependency for ordinary ticket checks (not just ledger children) — a regression in the tool's reliability for the whole ticket-validation pipeline, already flagged twice in prior review rounds and still unresolved. Finding 3 is a real bypass of the exact validation this ticket exists to add. Add the missing coverage from the Medium finding alongside the fix for findings 1/2 (the fix will need its own test anyway).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8de522694c
ℹ️ 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".
|
Automated activity reconciliation for current head
|
|
@codex review Please review current head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7c40ae8e1f
ℹ️ 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 #650 — ORB-155 gate ledger child filing threshold
Recommendation: REQUEST CHANGES
Scope of this pass
This is a re-review focused on the diff since my last review (submitted 2026-07-29T04:06:58Z,
against head 4cfdd06f): the new tip commit 7c40ae8e ("ORB-155 bound parent fallback") and the
no-op merge commit dfd9d69a. I re-verified each of that review's three High findings against the
current committed code rather than trusting the PR author's summary comment.
What's fixed (not re-flagged)
- Parent-classification bug (
parentFromOrcareturningundefinedfor "no parent at all"
instead ofnull): fixed attools/check-ticket.mjs:156— now returnsnull, so
parentIsClassifiablecorrectly short-circuits and standalone/non-ledger tickets no longer
trigger the Linear network fallback. Confirmed viagit diff 8de52269 7c40ae8e. - No timeout on the Linear HTTPS call: fixed —
LINEAR_PARENT_TIMEOUT_MS = 5_000is now passed
as the requesttimeoutoption, with atimeoutevent handler that destroys the request and
surfaces a clear error (tools/check-ticket.mjs:182,192-194). - Untested network branches: fixed —
tools/test-tools.mjs'sorca-shim.cjsnow monkeypatches
node:https.requestwhenORBIT_LINEAR_PARENT_STUBis set (a clean hermetic injection point,
not a test-only branch in production code), and new cases exercise the success path, the
requireTimeoutassertion (proves the 5s timeout option is actually set), and the GraphQL-error
→process.exit(2)path. - Bare/vague
blocked:claim escaping validation once the occurrence count reaches the
threshold: fixed —validateLedgerOccurrencenow runsAFFIRMATIVE_BLOCKING_CLAIMunconditionally
wheneverblockingClaim !== "no", regardless ofoccurrenceCount(tools/check-ticket.mjs:216-225).
Findings
High
[HIGH] AFFIRMATIVE_BLOCKING_CLAIM still accepts a subject-less vague claim, contradicting the grammar this same PR documents — carried over, unfixed
- dimension: 1. Correctness
- location:
tools/check-ticket.mjs:48-49 - issue: This is the same regex flagged in my prior review at this head's ancestor;
git diff 8de52269 7c40ae8e -- tools/check-ticket.mjsshows it is byte-identical across both fix commits —
it was not touched. The first alternative,
\b(?:could not|couldn't|cannot|can't|failed to|(?:was|were) unable to)\s+\S+, requires only
any single word after the modal phrase, with no requirement that an operation/subject precede
it."could not proceed"and"failed to run"both match. But.claude/skills/orchestrate/SKILL.md
(edited by this same PR) documents the grammar as<operation> could not <action>,blocked the <operation>, orthe <operation> was blocked— all three require a named operation. The
implementation is a strict superset of the documented contract. - risk:
Ledger occurrence: 2; blocked: could not proceed(orblocked: failed to run) passes
check-ticket.mjsdespite naming nothing concrete — exactly as content-free as the bare
blocked: yesthe test suite explicitly rejects ("a bare blocking claim does not bypass the threshold",tools/test-tools.mjs:6139-6143). This defeats the anti-vagueness purpose ORB-155
exists to add. No test intools/test-tools.mjsexercises a subject-less modal claim, so the gap
ships silently; the two "affirmative claim passes" tests (tools/test-tools.mjs:6149-6157) both
use claims with a named subject ("blocked the merge sweep","the merge sweep was blocked by unavailable authentication"), so they don't cover this branch. - fix: Require the first alternative's modal phrase to be preceded by a named subject too, mirroring
the other two patterns'(?:the|a|an)\s+\S+requirement (e.g.
\b(?:the|a|an)\s+\S+(?:\s+\S+){0,4}\s+(?:could not|couldn't|cannot|can't|failed to|(?:was|were) unable to)\s+\S+), and add a rejection test for"could not proceed"/"failed to run". - reference:
.claude/skills/orchestrate/SKILL.md's own documented grammar (edited in this PR);
rubric dimension 15 ("a new decision path…needs its own case").
Medium / 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 packages/shared/src/types/*/endpoints.ts, no orbit-api change |
| security-reviewer | N/A — no orbit-api code changed |
| design-reviewer | N/A — no UI files changed |
Validation
CI (gh pr view 650 --json statusCheckRollup) — Build, Unit Tests, SonarCloud are this PR's
required separate checks per the CI-adaptation instructions (Phase 6 /validate skipped here).
PR-body/comment evidence reports the full tools/test-tools.mjs + .claude/hooks/test-hooks.mjs
harness green at head 7c40ae8e.
Deferred — N/A dimensions
Dimensions 8-11, 13, 14 (DESIGN.md/AI-slop, parity, i18n, contract drift, backend hard rules,
FEATURES.md parity): N/A — diff touches only .claude/skills/orchestrate/SKILL.md,
tools/check-ticket.mjs, tools/test-tools.mjs. No orbit-api cross-repo dimension applicable
(sibling repo not checked out in this job, and none of the changed surfaces need it).
What's good
- All three previously-flagged High findings and the accompanying Medium (untested network
branches) are now genuinely fixed with real, hermetic test coverage — not just asserted fixed. - The hermetic
https.requeststub viaorca-shim.cjs+ORBIT_LINEAR_PARENT_STUBis a clean
injection point: it patches the shim process the tool runs under, not production code, so
check-ticket.mjsitself carries no test-only branches. parentFromOrcareturningnull(notundefined) for "definitively no parent" is the right fix
and is exactly what the prior reviews asked for.
Recommendation
Fix the one remaining High finding — tighten AFFIRMATIVE_BLOCKING_CLAIM's first alternative to
require a named subject, matching the grammar .claude/skills/orchestrate/SKILL.md already
documents — before merge, with a regression test for a subject-less claim like "could not proceed". Everything else in this round of fixes is solid.
|
@codex review Please review current head |
|
Automated activity reconciliation for current head
|
|
No code change required for automated activity |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9d2ddf9f59
ℹ️ 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 #650
Scope: PR #650 in thomasluizon/orbit-ui-mobile (.claude/skills/orchestrate/SKILL.md, tools/check-ticket.mjs, tools/test-tools.mjs)
Recommendation: APPROVE
Summary
This PR replaces the orchestrator's one-child-per-ledger-entry rule with a filing threshold
(3 occurrences or a run-blocking defect), teaches check-ticket.mjs to enforce the new fixed
Ledger occurrence: <count>; blocked: no|<what it blocked> line on children of a
marker-tagged parent, and adds an affirmative-grammar check plus a bounded Linear-API fallback
for partially-described parent relations. The diff is tooling/skill-only (no apps/*,
packages/shared, or orbit-api surface touched), so parity, i18n, DESIGN.md, contract-drift
and security-reviewer gates are all N/A by scope. The core validation logic (threshold math, the
affirmative-blocking regex, marker-gated scoping to ledger parents only) is sound and is backed by
extensive, well-targeted test-tools.mjs cases, including the grammar's negated-inability edge
cases called out in the PR's own review-fix commits. One concrete test-coverage gap survives review.
Findings
Critical
None.
High
None. (One hypothesis — that readLinearParent's if (!issue.id) return null could silently
skip ledger validation when Orca returns a partially-described parent — was raised and then
refuted under adversarial review: the same issue object already supplies title/description/
labels unconditionally earlier in the --issue branch (check-ticket.mjs:249-251), which only
holds if Orca returns a fully-hydrated primary issue record, and Linear's Issue type carries id
and identifier together as base scalars. The wave-plan.mjs identifier ?? id pattern cited as
support turned out to apply to different, synthetically-built objects, not a fetched issue detail
record, so it doesn't transfer. Dropped per the verification protocol's default-to-refuted rule.)
Medium
[MEDIUM] New Linear-fallback failure branches ship with zero harness coverage
· dimension: 15 (Harness changes need EXECUTED evidence)
· location: orbit-ui-mobile/tools/check-ticket.mjs:166,168
· issue: `readLinearParent` adds two new decision paths — `if (!existsSync(keyPath)) throw new Error(...)`
(missing `.linear-api-key`) and `if (!apiKey) throw new Error(...)` (empty key file) — introduced
in this PR. No case in `tools/test-tools.mjs` drives either branch: every test that reaches the
Linear fallback (`"a partial Orca parent relation uses the bounded Linear fallback"`,
`"a Linear parent GraphQL error exits with a tool error"`) uses `linearKeyHome`, which always
writes a non-empty `fixture-key` file first. The three cases using `noLinearKeyHome` (no key file)
all use a fully-classifiable parent (`ledgerIssue(...)` / `ledgerParentRelation` with a `title`),
so `readLinearParent` — and therefore the missing-key-file check — is never invoked in any of them.
· risk: A future refactor of the key-loading logic can silently break the "missing key" and "empty
key" error paths (e.g. producing the wrong exit code, or an uncaught exception instead of the
intended `check-ticket: could not read the Linear parent relation: …` message) with no test to
catch the regression — exactly the class of gap dimension 15 exists to catch ("a new branch on an
external tool's payload needs its own case, not an extension of an existing assertion").
· fix: Add a `checkIssue(...)` case combining `partialParentRelation` with `noLinearKeyHome` (asserts
`status: 2` and a `missing .*\.linear-api-key` stderr match), and a second case using a fresh temp
home with an empty `.linear-api-key` file (asserts `status: 2` and an `is empty` stderr match).
· reference: `.claude/skills/pr-review/rubric.md` dimension 15 ("A new tool or new branch on an
external tool's payload needs its own case… Medium, and concrete: name the path and the missing case").
Low / Info
check-ticket.mjs'sUSAGEstring documents exit code2as "usage error" only; this PR
reuses2for a Linear GraphQL/network failure inside--issuemode too (check-ticket.mjs:265).
No current caller in this repo branches on exit code specifically (they checkexit 0/exit 1
only), so this is Info rather than an actionable Medium — worth a one-lineUSAGEupdate if a
caller ever needs to distinguish "fix your ticket" from "Linear was unreachable," but not blocking.- The SKILL.md prose only spells out
blocked the <operation>/the <operation> was blockedas
the affirmative grammar, whileAFFIRMATIVE_BLOCKING_CLAIMalso acceptshalted/stopped/
preventedand thewerepassive form. The implementation is a strict superset of the documented
grammar, so no worker following the doc can be rejected — non-blocking, Info only.
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/* / orbit-api DTO changed, and orbit-api is not touched |
| security-reviewer | N/A — no orbit-api code changed |
| design-reviewer | N/A — no apps/* or orbit-landing-page UI file changed |
Validation
| Check | Result |
|---|---|
| Lint | N/A — not executable: this review session's sandbox blocks direct node/npm script execution (approval-gated, no interactive approver available) |
| Type check | N/A — same restriction |
Tests (node tools/test-tools.mjs) |
N/A — same restriction; relying on the PR body's self-reported run, which lists 18 PASS lines matching the exact test names added in this diff's tools/test-tools.mjs hunk 1:1, plus a clean ORBIT TOOLS GATE OK, and a separately reported npm run lint/type-check/test all exit 0 on the integrated branch |
| Build (api) | N/A — orbit-api not touched |
Deferred — N/A dimensions & files not verdicted
- Dimensions 8 (DESIGN.md/AI-slop), 9 (Parity), 10 (i18n), 13 (Backend hard rules): N/A, diff never
touchesapps/*ororbit-api. - Dimension 14 (FEATURES.md parity): N/A, internal orchestration/tooling change with no user-facing
feature surface. - Dimension 11 (Contract drift): N/A, no
packages/shared/src/types/*,endpoints.ts, or orbit-api
DTO in the diff — Phase 5 backward-compat guard has no candidate hunks to classify. - Whether Orca's real
linear issue <id> --relations --jsonCLI output matches the shapes assumed
byparentFromOrca/isLedgerParent/readLinearParent(e.g., whether aparentrelation is
actually modeled underrelationswithrelationship: "parent", versus a top-levelissue.parent
field only) could not be verified: the realorcabinary is a local Windows executable not present
in this review environment, and no other file in the repo documents its exact JSON schema. The
code's own primary path (Object.hasOwn(issue, "parent")) and its hermetic test stubs are
internally consistent, so this is a documented gap, not a suspected defect. - All three changed files (
.claude/skills/orchestrate/SKILL.md,tools/check-ticket.mjs,
tools/test-tools.mjs) received a verdict; nothing in the changed-file inventory was skipped.
What's good
- The threshold/marker design closes a real gap cleanly:
LEDGER_PARENT_MARKERscopes the new
validation strictly to children of a fixed-marker parent, so ordinary child tickets are provably
unaffected ("a recorded non-ledger child ticket is unaffected"test). - The affirmative-blocking regex was hardened over several review-fix commits specifically against
negated/inability phrasing ("could not block", "did not block", "could not have blocked") — the
test list shows those exact adversarial cases were anticipated and locked down. - The Linear-fallback mechanism reuses the repo's existing, documented
.linear-api-keyconvention
(already used by/featureand/orchestratefor direct GraphQL calls) rather than inventing a
new credential path, and bounds it to a 5s timeout with a real assertion (stub.requireTimeout)
that the timeout option is actually wired into thehttps.requestcall, not just documented. - The
orca-shim.cjsNODE_OPTIONS/syncBuiltinESMExports()technique correctly intercepts the
ESM named importimport { request } from "node:https"insidecheck-ticket.mjs's own process
(not just the stubbedorcasubprocess), keeping the new network path fully hermetic per
tools/CONVENTIONS.md's "no network" gate contract.
Recommendation
Merge as-is. Optionally follow up with the two missing test-tools.mjs cases for the
missing/empty .linear-api-key branches (Medium finding above) — not blocking, since the branches
are defensive and the primary/common path is otherwise thoroughly covered.
|
@codex review Please review current head |
|
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.
PR Review: #650
Branch: feature/orb-155-give-the-ledger-a-filing-threshold → main
Head: 5d592cb9f38d29e7291eaf9a006344617b0dd4b1
Recommendation: ✅ APPROVE
| Severity | Count |
|---|---|
| Critical | 0 |
| High | 0 |
| Medium | 0 |
| Low / Info | 1 |
Scope
This review covers the diff since the last recorded review on this PR. The prior claude review already APPROVED at head 9d2ddf9f, leaving one non-blocking Medium open (untested .linear-api-key missing/empty error branches in readLinearParent, tools/check-ticket.mjs:166,168). The only commit since that approval is 5d592cb9 ("ORB-155 reject negated blocking claims"), touching tools/check-ticket.mjs, tools/test-tools.mjs, and .claude/skills/orchestrate/SKILL.md.
Findings
That commit:
- Tightens
AFFIRMATIVE_BLOCKING_CLAIM(tools/check-ticket.mjs:47-48) to accept only the two grammar shapes SKILL.md documents —blocked the <op>/the <op> was blocked— anchored so nothing may trail the word "blocked" (previously it also acceptedhalted/stopped/preventedand, per two earlier review rounds, a subject-less modal like"could not proceed"). - Adds
NEGATED_BLOCKING_CLAIMas a belt-and-suspenders pre-check rejecting any claim containing a negation token (not,never,n't,without,from being/getting blocked, etc.) anywhere in the value. - Adds two new regression tests for the closed bypasses and tightens two existing passing fixtures to the new anchored grammar.
Hand-traced both regexes against the diff's new/changed strings and several adversarial inputs; no surviving bypass found. This closes the exact High finding left open by the immediately-prior review round.
Low / Info (non-blocking)
NEGATED_BLOCKING_CLAIM's bare-word list can false-positive on a legitimate operation name that happens to contain a listed word (e.g.blocked: blocked the deploy without staginggets rejected because of\bwithout\b, even though it's a correctly-shaped affirmative claim). This only over-rejects — it can never let a vague/negated claim through — so it's cosmetic friction, not a correctness or security issue.
CI
Green on every completed check on the current head (Lint, Type Check, Unit Tests, Build, SonarCloud, Harness Lockstep, Contract Drift, Cross-Platform Parity, Dash Ban, Copy Register, Expo SDK Pin, Suppressions Ratchet). Harness Execution was still IN_PROGRESS at review time on both matrix legs.
Cross-repo note
The sibling orbit-api repo is not checked out in this job; no dimension in this diff required it (this PR touches only tools/ and .claude/skills/).
🤖 Generated with automated PR review (/pr-review)
|
@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.
Review Complete
Scope: PR #650 in thomasluizon/orbit-ui-mobile (feature/orb-155-give-the-ledger-a-filing-threshold → main)
Recommendation: APPROVE
| Severity | Count |
|---|---|
| Critical (incl. |
0 |
| High | 0 |
| Medium | 3 |
| Low / Info | 0 posted (1 Info note only) |
Summary
The diff is confined to tooling/skill docs — .claude/skills/orchestrate/SKILL.md, tools/check-ticket.mjs, tools/test-tools.mjs — replacing the one-child-per-ledger-entry rule with a 3-occurrence-or-blocking threshold, enforced via a fixed Ledger occurrence: <count>; blocked: no|<what it blocked> line and an affirmative-blocking grammar. No apps/*, orbit-api, or packages/shared files changed, so all five review subagents (parity, i18n, contract-aligner, security-reviewer, design-reviewer) and FEATURES.md parity are N/A. Static trace of the regex grammar, threshold logic, and parent-classification precedence against the diff's own 19 new test cases shows everything behaves as documented and the negation-bypass hardening is genuinely thorough. The findings below are about an unverified external-payload assumption and coverage gaps, not proven correctness bugs.
Findings
Critical / High
None.
Medium
[MEDIUM] Bounded Linear fallback keys off issue.id, a field this codebase treats as unreliable on Orca issue objects
· location: tools/check-ticket.mjs:166
· issue: readLinearParent only fires the Linear GraphQL fallback when issue.id is truthy; otherwise it silently returns null, treating the child as parentless and skipping ledger validation with no error. Every pre-existing "issue" fixture in tools/test-tools.mjs (VALID_ISSUE at line 3775, and linearIssueStub at 731-738, reused across many other tools' tests) carries identifier but never id. tools/wave-plan.mjs:246,276,287 defensively reads issue.identifier ?? issue.id in three places — evidence these two field names aren't reliably both present on the same "issue" shape elsewhere in this codebase. The new PR's own tests for this path only pass because they hand-inject id: "linear-partial-parent" / id: "linear-parent-error", a shape that doesn't match the established stub convention.
· risk: If the real orca linear issue <ORB-N> --relations --json payload doesn't carry a raw id, the fallback never fires in production, and a child with a genuinely shallow parent relation — the exact scenario this PR was built to handle — silently skips ledger-occurrence validation while check-ticket.mjs still reports "ticket ok".
· fix: Verify against a real orca linear issue --json output whether id is populated; if not, key the GraphQL variable and the guard off issue.id ?? issue.identifier, mirroring wave-plan.mjs.
· reference: CLAUDE.md rule 1; rubric dimension 15.
[MEDIUM] Three new readLinearParent decision paths have no dedicated test case
· location: tools/check-ticket.mjs:168-170 (missing/empty key file) and :194-196 (real timeout event)
· issue: Only "key present + GraphQL error" and "key present + success" are tested. The !existsSync(keyPath) branch, the empty-key-file branch, and the linearRequest.on("timeout", ...) handler actually firing (as opposed to just asserting the timeout: 5000 option was set) are all uncovered.
· risk: A regression in any of these three branches would ship silently.
· fix: Add three checkIssue cases exercising each branch directly.
· reference: rubric dimension 15 ("a new decision path... needs its own case").
[MEDIUM] tools/README.md's check-ticket.mjs catalog row is now stale
· location: tools/README.md:46
· issue: The row doesn't mention the new ledger-threshold gate or the direct Linear GraphQL fallback — the only tool in the catalog now making a raw network call outside the orca/gh stub surface.
· fix: Add one clause describing the ledger gate and the bounded Linear lookup.
· reference: root CLAUDE.md, Conventions & tooling.
Low / Info
None posted. (Info only: SKILL.md's documented grammar is a strict subset of what the regex accepts — a/an articles are also allowed — harmless, no action needed.)
Subagents
| Agent | Verdict |
|---|---|
| parity-checker | N/A |
| i18n-syncer | N/A |
| contract-aligner | N/A |
| security-reviewer | N/A |
| design-reviewer | N/A |
Validation
| Check | Result |
|---|---|
| Lint / Type check / Tests | N/A — not independently executable in this session (sandbox denied); PR body cites all-PASS npm run lint/type-check/test and a full node tools/test-tools.mjs log naming every new ledger case |
| Build (api) | N/A — orbit-api untouched |
| CI: Build / Unit Tests / SonarCloud | Handled as separate required checks (this review skips /validate per the CI adaptation) |
Deferred
Dimensions 8-14 (DESIGN.md, parity, i18n, contract drift, backend hard rules, FEATURES.md) are N/A — no apps/*, orbit-api, or packages/shared surface touched. Dimension 15's execution proof could not be independently re-run this session; cross-checked statically instead. All three changed files got a full-diff read and a verdict. Note: the PR branch's current head also carries a merge of main, which pulled in unrelated main-side work (tools/merge-sweep*.sh, tools/ai-quota.mjs, etc.) into the git history; the actual PR diff (base...head) remains the same 3 files reviewed here — confirmed against gh pr diff --name-only and against the prior approved review at commit 5d592cb9.
What's good
The affirmative/negated blocking-claim regex pair is well hardened (ten distinct bypass attempts tested and rejected). Parent-classification precedence (direct field → relations → bounded Linear fallback, with "no parent" authoritative) avoids unnecessary network calls on the common path. SKILL.md and the checker agree on the core contract.
1 participant
What changed
Ledger occurrence: <count>; blocked: no|<what it blocked>and required blocking values to state an affirmative blocked outcome at every occurrence count.Harness defect ledgermarker. Complete Orca parent data is authoritative; only an actual but incomplete parent signal uses a bounded read-only Linear lookup.Why
ORB-155 stops unattended orchestration from turning every transient ledger observation into backlog growth. Repeated defects and defects that actually stop a run remain independently repairable, while the parent stays the durable record for measured evidence.
Acceptance evidence
Current head:
5d592cb9.The integrated branch also passed:
Review fixes
e5f4cedfrestricts ledger validation to children with a marked ledger parent and adds an ordinary-parent regression.6c108bffrejects nonblocking sentinel aliases below the threshold and normalizes punctuation.2b684972preserves Orca-only issue validation by consulting returned parent data first.8de52269replaces the nonblocking denylist with an affirmative blocking grammar and documents that grammar in the skill.7c40ae8emakes no-parent Orca results authoritative, bounds the partial-parent fallback to 5 seconds, covers GraphQL success and error paths, and validates blocking values above the threshold.9d2ddf9fanchors the grammar to affirmative active or passive blocked outcomes and adds regressions for negated inability claims.5d592cb9rejects whole-claim negation and inversion after the blocking verb, restricts accepted claims to the two documentedblockedshapes, and covers both reported suffix bypasses.Decisions taken unattended
mainwith ordinary merge commits and preserved every mainline harness case.