ORB-89: Route worker models by engine-neutral tier - #639
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR #639 Review — ORB-89: route worker models by tier
Recommendation: APPROVE
No prior reviews or review threads existed on this PR, so this is a full first pass, not a delta review.
| Severity | Count |
|---|---|
| Critical | 0 |
| High | 0 |
| Medium | 2 |
| Low / Info | 0 |
Findings
[MEDIUM] New "args must not carry --model" validation has no test case
tools/lib/orchestrator-config.mjs:42-48 — resolveWorkerInvocation throws when engine.args contains --model/-m/--model=..., guarding against exactly the base-args-vs-models-map migration mistake this PR performs. tools/test-tools.mjs asserts the resolved command in every --model case, but none stage a config that trips this specific guard. Suggested fix: add a stageLaunchWorker case with args: ["--model", "opus"] alongside a models map, asserting { status: 2, stderr: /non-model strings/ }.
[MEDIUM] Missing-cheap-tier branch of the required-tier loop is untested
tools/lib/orchestrator-config.mjs:54-58 — for (const requiredTier of ["cheap", "deep"]); tools/test-tools.mjs:679 only covers the missing-deep case (no-deep-model), never missing-cheap (the loop's first iteration). Suggested fix: add a no-cheap-model case mirroring the existing no-deep-model one.
Both trace to rubric dimension 15 ("a new decision path added to a tool that already has coverage needs its own case — Medium"). Neither blocks merge.
What's good
tools/lib/orchestrator-config.mjscleanly removes 5x duplicated JSON-parse/try-catch boilerplate (rule-10 DRY, not premature abstraction).- The resolver's dry-run output matches the code path (
invocationForordering) on inspection. - Broad new test coverage otherwise (10+ cases across both engines and the full failure matrix).
- The PR body's "Decisions taken unattended" section transparently explains the one piece of bundled scope creep (
RELATION_FETCH_CONCURRENCY8→10 to de-flake an existing timing test).
Deferred / not applicable
- Dimensions 6, 8-14 (type safety, DESIGN.md, parity, i18n, contract drift, security, backend hard rules, FEATURES.md): N/A — diff touches only
.claude/**,tools/**,CLAUDE.md(no TS, noapps/*UI, noorbit-api, no shared types, no user-facing feature). - Phase-4 subagents (parity-checker, i18n-syncer, contract-aligner, security-reviewer, design-reviewer): not triggered — none of their trigger surfaces are touched.
- Harness execution (
node tools/test-tools.mjs,node .claude/hooks/test-hooks.mjs): required per CLAUDE.md fortools/**/.claude/**changes; verified via CI's Harness Execution job (Build/Unit Tests/SonarCloud run as separate required checks in this workflow — not re-run here). - Cross-repo (orbit-api): not applicable — no contract/shared-type changes in this diff; orbit-api is not checked out in this job.
All changed files received a verdict; nothing was left unexamined.
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
@codex review Context budget baseline reseeded after Byte proof from committed blobs: Merged Post-merge verification: |
There was a problem hiding this comment.
Code Review: PR #639 — ORB-89: Route worker models by tier
Recommendation: APPROVE
Summary
This is a delta review. A prior claude review already ran and APPROVED this PR at commit a772986d (2026-07-28T15:29:57Z), with 0 Critical/High findings and 2 Medium findings (missing test cases for two specific guard branches in the new resolveWorkerInvocation resolver). Since that review, two commits landed: 8113b619 ("chore: reseed context budget for ORB-89") and merge commit 7750e9c9 (merging origin/main, which brought in unrelated ORB-106 content already reviewed on main and nets to zero against the three-dot diff this review is scoped to). The only substantive new content since the last review is the context-budget reseed. Independently verified it and found no defects.
Findings
Critical
None.
High
None.
Medium
Carried forward from the last review, still unaddressed in this diff (not new; confirmed still present at the cited locations):
- New "args must not carry
--model" validation has no test case —tools/lib/orchestrator-config.mjs:45-47.tools/test-tools.mjshas nostageLaunchWorkercase that trips this specific guard. (rubric dimension 15) - Missing-
cheap-tier branch of the required-tier loop is untested —tools/lib/orchestrator-config.mjs'sfor (const requiredTier of ["cheap", "deep"])loop; only the missing-deepcase (no-deep-model,tools/test-tools.mjs:859) exists, never missing-cheap. (rubric dimension 15)
Neither blocks merge (Medium, not Critical/High).
Low / Info
[INFO] Context-budget reseed verified byte-accurate
· location: tools/context-budget.json (commit 8113b619)
· issue: none — verifying, not flagging. The original feature commit (a772986d) changed CLAUDE.md's D5 sentence but left tools/context-budget.json stale at {bytes: 15381, CLAUDE.md: 11998}. This follow-up commit reseeds it to {bytes: 15416, CLAUDE.md: 12033}.
· verification: independently checked out the PR head and confirmed CLAUDE.md is exactly 12033 bytes and .claude/rules/core.md is exactly 3383 bytes (12033 + 3383 = 15416), matching the new baseline exactly.
· reference: CLAUDE.md "Conventions & tooling" (context-budget reseed policy)
Subagents
| Agent | Verdict |
|---|---|
| parity-checker | N/A — no apps/web/** or apps/mobile/** files touched |
| i18n-syncer | N/A — no user-facing strings or i18n JSON touched |
| contract-aligner | N/A — no packages/shared/src/types/*/endpoints.ts touched; not verifiable in CI (sibling orbit-api not checked out) |
| security-reviewer | N/A — no orbit-api code touched |
| design-reviewer | N/A — no UI files touched |
Validation
Per this run's CI adaptation, the /validate phase (lint/type-check/test) is skipped since Build, Unit Tests, and SonarCloud already run as separate required checks in this PR.
| Check | Result |
|---|---|
| Lint | PASS (CI, separate required check) |
| Type check | PASS (CI, separate required check) |
| Tests | PASS (CI, separate required check) |
| Context Budget | SKIPPED — expected: PR carries the context:reseed label, the sanctioned escape hatch; independently re-verified the byte counts (see Info finding above) |
| Harness Execution | The PR author's own posted evidence in the PR description shows node tools/test-tools.mjs → ORBIT TOOLS GATE OK and node .claude/hooks/test-hooks.mjs → ORBIT HOOK PARITY OK, both run against the current head. |
Deferred — N/A dimensions & files not verdicted
- Dimensions covering type safety escape hatches beyond what's reviewed, DESIGN.md/AI-slop, parity, i18n, contract drift, security, backend hard rules, FEATURES.md: N/A — diff touches only
.claude/**,tools/**,CLAUDE.md; no TS type surface beyond the reviewed resolver, noapps/*UI, noorbit-api, no shared types, no user-facing feature surface. - Contract-aligner / cross-repo backward-compat (orbit-api side): not verifiable in CI — sibling
orbit-apirepo is not checked out in this job, and the diff carries no shared-type or endpoint changes regardless. - Merge-commit content diff (
7750e9c9's raw stat:SKILL.md,check-ticket.mjs,merge-sweep*.sh,test-tools.mjs): not separately re-reviewed — this content originates from ORB-106 (edccbe81), already merged and reviewed onmainindependently of this PR; it nets to zero against the three-dot diff this PR is scored on.
What's good
- The reseed commit is a precise, minimal, byte-verified fix for a gap the original commit left (CLAUDE.md changed, baseline not reseeded) — exactly the kind of thing
check-context-budget.mjs --checkexists to catch, and the author caught it themselves before the caller had to. - Everything substantive about this PR (the resolver, its tests, the doc updates) was already reviewed and approved in the prior pass with only two Medium, non-blocking gaps.
Recommendation
APPROVE. No Critical/High findings survive, either from the prior review or from the diff since. The two outstanding Medium test-coverage gaps (orchestrator-config.mjs:45-47 and the missing no-cheap-model case) are fine to merge with a tracked follow-up rather than blocking here, per the Signal gate (Medium never forces a request for changes).
|
@codex review Addressed both review-body coverage findings in
Verified on this commit: |
|
Codex Review: Didn't find any major issues. Keep it up! 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 #639
Scope: PR #639 in thomasluizon/orbit-ui-mobile — "ORB-89: Route worker models by engine-neutral tier"
Recommendation: APPROVE
Summary
This PR replaces the single worker:sonnet-only model swap in tools/launch-worker.mjs with an
engine-neutral tier resolver (tools/lib/orchestrator-config.mjs) that both claude and codex
engines declare models.{default,cheap,deep} against in .claude/orchestrator.json. It is
tooling/config-only (no apps/*, packages/shared, or orbit-api files touched), carries the
context:reseed label needed for the CLAUDE.md/tools/context-budget.json byte-budget growth,
and ships extensive new tools/test-tools.mjs coverage for every validation branch (legacy label,
unknown tier, conflicting tiers, missing default/cheap/deep, identical cheap/deep, tier resolving
to the unchanged default, model flag leaking into base args). The logic is sound and the
supporting docs (CLAUDE.md, .claude/skills/orchestrate/SKILL.md, tools/README.md) were
updated in step with the code.
Findings
Critical
None
High
None
Medium
None
Low / Info
None posted (signal gate). Two harmless dead-ish validation branches were noted in
tools/lib/orchestrator-config.mjs during review — the cheapArgs/deepArgs null branches are
unreachable once the earlier models.cheap/models.deep presence check has already passed, and
the engine/engine.args shape checks duplicate guards launch-worker.mjs already runs before
calling the resolver. Neither is a defect (both are defensive redundancy in a shared library that
could gain a second caller), so neither is worth a blocking finding or a follow-up ticket.
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/*.json changed |
| contract-aligner | N/A — neither repo's DTOs/Zod types nor endpoints.ts changed, and only one repo (ui-mobile) is in scope |
| security-reviewer | N/A — orbit-api not touched |
| design-reviewer | N/A — no apps/*/orbit-landing-page UI files changed |
Validation
| Check | Result |
|---|---|
| Lint | N/A — could not execute npm run lint in this review session (see Deferred) |
| Type check | N/A — could not execute npm run type-check in this review session (see Deferred) |
| Tests | N/A — could not execute node tools/test-tools.mjs / node .claude/hooks/test-hooks.mjs in this review session (see Deferred) |
| Build (api) | N/A — orbit-api not touched |
The PR body's own reported run of npm run lint, npm run type-check, npm run test,
node tools/test-tools.mjs, node .claude/hooks/test-hooks.mjs, node tools/check-frontmatter.mjs,
node tools/check-dashes.mjs, and node tools/arch-map.mjs all show green, plus a --dry-run
evidence block for Claude default/cheap/deep and Codex default/cheap/deep, legacy-label
remediation, unknown/conflicting labels, and missing/identical mappings. That is self-reported
evidence, not independently re-executed by this review (see Deferred), but a hand-trace of
tools/lib/orchestrator-config.mjs against tools/test-tools.mjs's new assertions confirms the
described behavior matches the code path-for-path: the legacy-label message, the unknown/
conflicting-tier messages, the missing-default/cheap/deep messages, the identical-tiers message,
and the unchanged-tier message all match the regexes the new tests assert on.
Independently confirmed by static inspection (not by execution): tools/context-budget.json's new
byte figures are internally consistent — git show <PR head>:CLAUDE.md | wc -c = 12033 and
.claude/rules/core.md = 3383, summing to the claimed 15416 — and the PR carries the
context:reseed label the budget-growth policy requires.
Deferred — N/A dimensions & files not verdicted
- DESIGN.md / AI-slop (#8): N/A, no
apps/*UI files changed. - Parity (#9): N/A, no
apps/web/**orapps/mobile/**files changed. - i18n (#10): N/A, no user-facing strings or i18n JSON changed.
- Contract drift + backward-compat (#11): N/A, no
packages/shared/src/types/*,endpoints.ts,
or orbit-api DTOs changed. - Backend hard rules (#13): N/A,
orbit-apinot touched. - FEATURES.md parity (#14): N/A, this is internal harness/tooling, not a user-facing feature
surface change. - Validation (Phase 7): this review session's
Bashtool required interactive approval to run
node/npm(harness scripts, lint, type-check, tests) that was not obtainable in this
non-interactive run, so Lint/Type check/Tests were not independently re-executed here — the
Validation table above relies on the PR body's self-reported output plus a hand-trace of the new
code against the new test assertions, not a fresh run. - All 12 changed files (
.claude/orchestrator.json,.claude/skills/orchestrate/SKILL.md,
CLAUDE.md,tools/README.md,tools/context-budget.json,tools/launch-worker.mjs,
tools/lib/orchestrator-config.mjs,tools/nudge-worker.mjs,tools/test-tools.mjs,
tools/wave-plan.mjs,tools/worker-status.mjs,tools/worker-watch.mjs) were read and given a
verdict above; nothing changed was skipped.
What's good
tools/lib/orchestrator-config.mjsis a clean, single-responsibility module: one shared reader,
one shared resolver, both engines going through the exact same validation instead of a
Claude-only special case.- The refusal messages are specific and actionable (they name the engine, the missing tier, and
the declared alternatives), matching this repo's own "loud failure over silent workaround"
standard. - Test coverage is unusually thorough for a config-validation change: every refusal branch
(legacy label, unknown tier, conflicting tiers, missing default/cheap/deep, identical cheap/deep,
tier-equals-default, model flag stuck in base args) has its own case intools/test-tools.mjs. - Docs (
CLAUDE.md,SKILL.md,tools/README.md) were updated in the same PR and, on inspection,
accurately describe the new resolver's behavior — no doc/code drift found. - The unrelated
wave-plan.mjsRELATION_FETCH_CONCURRENCYbump (8→10) is called out explicitly
in the PR's "Decisions taken unattended" section with the measured timing that motivated it,
consistent with this repo's "fix it in the same PR" rule rather than being silently smuggled in.
Recommendation
Approve as-is. The two dead-ish validation branches noted under Low/Info are optional cleanup, not
blockers — nothing here needs a follow-up ticket.
|
SUPERSEDED: head 2887199 was replaced by the ADR correction. The review loop continues on the new head. |
|
@codex review The previous ready marker is superseded. The accepted ADR correction is now on current head Codex routing now resolves:
Live no-label dry-run: Post-merge verification: The context baseline was reseeded after renormalizing the combined tree and verified against the committed blobs: |
|
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 #639 — ORB-89: Route worker models by engine-neutral tier
Scope: PR #639 in thomasluizon/orbit-ui-mobile (feature/orb-89-route-codex-workers-to-a-per → main, head d7a6dcd1)
Recommendation: NEEDS WORK
Summary
The diff replaces the single hardcoded --model flag per engine with a models map (default/cheap/deep) resolved through a new shared module, tools/lib/orchestrator-config.mjs, consumed by five tools. The validation logic itself is solid — it rejects the legacy worker:sonnet label, conflicting/unknown tier labels, missing tier mappings, and tier resolutions that don't actually change the invocation, all with reasonable, well-tested error messages. The problem is that the same PR bumps wave-plan.mjs's relation-fetch concurrency pool from 8 to 10 (to fix a real Windows timing gate) but never updates the harness test that asserts the pool never exceeds 8 concurrent fetches — and GitHub's own check runs on this exact head confirm the Harness Execution gate is currently failing.
Findings
Critical
[CRITICAL] RELATION_FETCH_CONCURRENCY bump breaks its own harness assertion — confirmed red on the current head
· dimension: 15 (Harness changes need EXECUTED evidence) / 1 (Correctness)
· location: tools/wave-plan.mjs:44 vs tools/test-tools.mjs:2505-2519
· issue: This PR changes const RELATION_FETCH_CONCURRENCY = 8 to = 10 (per the PR body: "The relation pool was increased from 8 to 10 so the implementation meets the existing threshold"). mapBounded (tools/wave-plan.mjs:79-90) spawns Math.min(RELATION_FETCH_CONCURRENCY, items.length) workers via Promise.all(Array.from({ length: 10 }, worker)) — all 10 start synchronously in the same tick, each immediately calling execFileAsync to spawn a real child process. The untouched test "wave-plan.mjs: fetches 100 relations in a bounded pool while preserving the table order" (tools/test-tools.mjs:2510-2519) still asserts concurrency.peak <= 8. With the pool now at 10, the measured peak concurrency will be ~10, not ≤8, so this assertion fails.
· This is not speculative: gh pr view 639 --json statusCheckRollup shows the Harness Execution check (Guards workflow) with conclusion: FAILURE on head d7a6dcd1 — the PR's actual current head, reconfirmed independently in this review (gh pr view 639 --json headRefOid,statusCheckRollup → head d7a6dcd17b0368b3daaef0f707576c40fded4381, one Harness Execution run FAILURE). A second, same-named "Harness Execution" run shows SUCCESS but is stale/too-fast to have executed the real 100-process concurrency test — it is not evidence this gate is clean. This directly contradicts the PR body's "ORBIT TOOLS GATE OK" transcript, which must predate the RELATION_FETCH_CONCURRENCY bump or the branch-advance-to-main step.
· risk: The PR merges with a red required gate, or a human overrides it, undermining the exact guarantee dimension 15 exists to protect ("harness cannot be certified by reading it").
· fix: Update the assertion to match the new pool size, e.g. concurrency.peak <= 10 (and consider asserting >= 9 or comparing against an exported constant from wave-plan.mjs instead of a hardcoded literal, so a future retune can't silently desync the test again).
· reference: TESTING.md (Harness Execution job); .claude/skills/pr-review/rubric.md dimension 15.
High
None beyond the Critical above.
Medium
[MEDIUM] New shared config-read failure path has zero test coverage across all 5 call sites
· dimension: 15 (Harness changes need EXECUTED evidence)
· location: tools/lib/orchestrator-config.mjs:25-33 (readOrchestratorConfig)
· issue: This new module centralizes .claude/orchestrator.json reading for launch-worker.mjs, nudge-worker.mjs, wave-plan.mjs, worker-status.mjs, and worker-watch.mjs. Its catch branch (malformed/unreadable JSON) is a new decision path with no test anywhere in tools/test-tools.mjs. The one existing "malformed orchestrator.json" test (tools/test-tools.mjs:2579) exercises check-lockstep.mjs, which has its own independent reader with different (silent-fallback) behavior — it does not touch this module at all.
· risk: wave-plan.mjs's new try/catch around readOrchestratorConfig() (replacing an unhandled-exception crash with a clean exit 2) ships unexercised, same as the other four call sites' shared error message.
· fix: Add one staged-config test (any one of the five tools, e.g. wave-plan.mjs with a truncated .claude/orchestrator.json) asserting exit 2 and the "could not be read as JSON" message — since the reader is now shared, one case covers all five callers.
· reference: .claude/skills/pr-review/rubric.md dimension 15 ("a new decision path... needs its own case").
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 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 | PASS (per CI: Lint / PR Tests, SUCCESS) |
| Type check | PASS (per CI: Type Check / PR Tests, SUCCESS) |
| Tests | PASS (per CI: Unit Tests / PR Tests, SUCCESS) |
| Harness Execution | FAIL (Guards workflow — confirmed via gh pr view 639 --json headRefOid,statusCheckRollup on head d7a6dcd1; see Critical finding) |
| Build (api) | N/A — orbit-api not touched |
This review skips the skill's own Phase 6 (/validate); this PR already runs Build / Unit Tests / SonarCloud as separate required checks. The Harness Execution failure above is read from the PR's own GitHub check-run results, independently reconfirmed in this session.
Deferred — N/A dimensions & files not verdicted
- Dimensions 8, 9, 10, 11, 12, 13, 14 (DESIGN.md, Parity, i18n, Contract drift, Security, Backend hard rules, FEATURES.md): N/A — diff is confined to
tools/**and.claude/**, none of these surfaces are touched. - Cross-repo dimensions (contract-aligner, the orbit-api side of the backward-compat guard): not verifiable in this CI job —
orbit-apiis not checked out here. - Cross-model second opinion (
/second-opinion): not run in this CI job; the Critical finding is instead corroborated directly against the PR's live GitHub check-run status. - All 12 changed files (
.claude/orchestrator.json,.claude/skills/orchestrate/SKILL.md,CLAUDE.md,tools/README.md,tools/context-budget.json,tools/launch-worker.mjs,tools/lib/orchestrator-config.mjs,tools/nudge-worker.mjs,tools/test-tools.mjs,tools/wave-plan.mjs,tools/worker-status.mjs,tools/worker-watch.mjs) received a verdict.
What's good
- The
worker:sonnet→tier:cheapmigration is handled loudly and unambiguously, with a dedicated remediation message rather than a silent translation. resolveWorkerInvocation's validation is genuinely defense-in-depth: it independently catches missingdefault/cheap/deepmappings, a--model/-mflag baked into baseargs, identicalcheap/deepresolutions, and a tier that resolves identically todefault— each with its own test case intest-tools.mjs.- The
readOrchestratorConfig/resolveWorkerInvocationextraction intotools/lib/orchestrator-config.mjsis a clean DRY win: five tools previously each hand-rolledJSON.parse(readFileSync(...))with slightly different error wrapping; now there's one source of truth, documented intools/README.mdconsistently with the existinglib/tui-repaint.mjsprecedent. - Documentation (
CLAUDE.md,.claude/skills/orchestrate/SKILL.md,tools/README.md,tools/context-budget.json) was updated in the same PR and is internally accurate — the byte counts incontext-budget.jsonmatch the actual file sizes atHEAD.
Recommendation
Fix the stale concurrency.peak <= 8 assertion in tools/test-tools.mjs (or the RELATION_FETCH_CONCURRENCY value it should track) so the tools test harness genuinely passes, then re-run the Harness Execution gate and confirm it goes green before merge. Optionally add one test case covering readOrchestratorConfig's malformed-JSON path (Medium, non-blocking).
|
@codex review Current head: The previous Harness Execution failure exposed a merge-resolution mismatch: Verified locally on this exact head:
Codex routing remains default Sol at high, cheap Luna at low, and deep Sol at max. |
|
There was a problem hiding this comment.
Review Complete
Scope: PR #639 in orbit-ui-mobile — ORB-89: Route worker models by engine-neutral tier
Recommendation: APPROVE
| Severity | Count |
|---|---|
| Critical | 0 |
| High | 0 |
| Medium | 0 |
| Low / Info | 2 |
Summary
Internal tooling/config change only (.claude/, tools/, CLAUDE.md, tools/README.md) — no apps/, packages/shared, or orbit-api files touched, so all five review subagents (parity-checker, i18n-syncer, contract-aligner, security-reviewer, design-reviewer) are gated out (N/A). It replaces the old worker:sonnet-only --model opus→sonnet swap hack with an engine-neutral models map (default/cheap/deep) in .claude/orchestrator.json, resolved by a new shared module tools/lib/orchestrator-config.mjs (a legitimate third-use DRY extraction over five tools that previously duplicated the same config-read pattern). The resolver fails loud on every broken-config shape (missing tier, --model/-m leaking into base args, conflicting/unknown tier:* labels, identical cheap/deep, or a tier resolving to the same invocation as default), each with a matching new test in tools/test-tools.mjs. Doc updates (CLAUDE.md, tools/README.md, .claude/skills/orchestrate/SKILL.md, tools/context-budget.json) are internally consistent; byte counts for CLAUDE.md + core.md were hand-verified against context-budget.json.
Findings
Low — tools/lib/orchestrator-config.mjs:240-241: dead/unreachable ternary branches.
const cheapArgs = entries.has("cheap") ? invocationFor(engine.args, entries.get("cheap")) : null
const deepArgs = entries.has("deep") ? invocationFor(engine.args, entries.get("deep")) : nullThe required-tier loop a few lines above already throws if cheap or deep is missing from engine.models, so entries.has(...) is always true here — the : null branches can never run. Root CLAUDE.md rule 2 ("delete unused code immediately... no defensive branches for a problem already handled upstream"). Fix: call invocationFor directly for both tiers, no ternary. Purely cosmetic — no behavior bug.
Info — PR body / shipped-code mismatch. The "Decisions taken unattended" section claims "The relation pool was increased from 8 to 10...", but the PR's own last commit (00b044cb, fix(orchestrator): preserve relation pool bound) reverts RELATION_FETCH_CONCURRENCY back to 8, unchanged from main — net diff carries no change to that constant. Worth a one-line correction to the PR description so it doesn't misrepresent shipped behavior to a future reader.
Subagents / Validation
All five subagents: N/A (diff never touches their gated surfaces). This CI job skips Phase 6 (/validate) per workflow instructions — Build / Unit Tests / SonarCloud run as separate required checks. Relying on the PR's own self-reported green results (150 problems, 0 errors; type-check pass; 1611 passed (1611); ORBIT TOOLS GATE OK; ORBIT HOOK PARITY OK) plus direct code reading, which found the new resolver logic and its tests internally sound.
What's good
- Clean DRY extraction (
orchestrator-config.mjs) eliminating 5x duplicated config-read boilerplate. - Fail-loud, well-tested config validation with specific, actionable error messages (declared tiers listed on an unknown-tier error, etc.).
- Legacy label rejected explicitly with remediation text rather than silently reinterpreted.
- No stale
wantsSonnetor old-swap-logic residue left behind.
|
Codex Review: Didn't find any major issues. Keep it up! 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". |
|
READY-TO-MERGE 00b044c |
1 participant
Summary
.claude/orchestrator.jsonreader and tier resolver for the five worker toolstier:cheapandtier:deepthrough engine-specific model maps while keeping no-label launches on each engine defaultworker:sonnet, unknown or conflicting labels, missing mappings, identical tiers, and tier selections that leave the invocation unchangedmainWhy
The previous launcher rewrote the literal
--model opuspair only forworker:sonnet. That silently did nothing for Codex and let an engine-specific model name leak into the ticket label contract. ORB-89 makes the label name intent and makes every invalid or ineffective mapping a launch error.Validation
The tools and hook harnesses were rerun after merging current
mainat72811a59; both remained green.Dry-run evidence
The full tools output also passed Claude default, cheap, and deep; Codex default, cheap, and deep; legacy label remediation; unknown and conflicting labels; missing default and named mappings; identical cheap/deep mappings; and unchanged non-default invocations.
Decisions taken unattended
tier:cheapmaps to Luna low, andtier:deepmaps to Sol max. This preserves the live worker tier and spends the effort level the ADR reserved for per-ticket routing.--effort maxso the deep tier remains Opus but is observably distinct.mainalready selects Codex as the top-level worker. This PR leaves that key unchanged and changes only its model routing.mainbefore final gates. ORB-129's hermetic nudge coverage and ORB-122's raw repo-tool guard were preserved during reconciliation.Links ORB-89.