ORB-107: preflight autonomous worker launches - #642
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review: PR #642 — ORB-107: preflight autonomous worker launches
Scope: PR #642 in thomasluizon/orbit-ui-mobile (.claude/skills/orchestrate/SKILL.md, tools/README.md, tools/launch-worker.mjs, tools/preflight.mjs [new], tools/test-tools.mjs)
Recommendation: APPROVE
Summary
This PR adds tools/preflight.mjs, a fast fail-loud environment gate that checks the
worker invocation's unattended shell policy, GitHub CLI auth, Orca reachability, the
target repo's branch/cleanliness, and required CLIs before /orchestrate creates any
worktree. It also hardens tools/launch-worker.mjs so a claude/codex invocation that
lacks its engine's known-good "run-permitting" flag (--permission-mode bypassPermissions / --dangerously-bypass-approvals-and-sandbox) is refused outright,
closing the exact acceptEdits-stalls-on-shell-approval failure class the ticket names.
The whole diff is confined to tools/** plus the two docs that describe it — no
apps/*, no orbit-api, no packages/shared/types — so the UI/parity/i18n/contract/
security-API rubric dimensions are genuinely out of surface, not skipped. The PR body
carries a real execution transcript (lint/type-check/test, the hook-parity harness, and
the tool harness gate's new preflight/launch-worker cases, plus a real run of the new
preflight tool and Orca/GitHub auth checks against the live environment), which is the
kind of evidence dimension 15 asks for rather than an impression. Three Medium findings
below are all defense-in-depth / coverage gaps, none of them let an unsafe unattended
launch through in practice (the newly-added launch-worker.mjs check remains the
authoritative last-line gate either way).
Findings
Critical
None.
High
None.
Medium
[MEDIUM] Worker run-permission policy is asserted twice, from two data shapes that already diverge on an equals-joined flag
· dimension: SOLID / clean architecture (#3) — DRY at the right level
· location: tools/preflight.mjs:215-224 (workerPolicies regexes) vs tools/launch-worker.mjs:87-101 (ENGINE_PROFILES.runPermissionTokens)
· issue: The "known-good unattended shell policy" per engine is encoded independently in two files: launch-worker.mjs matches a consecutive-token array ("--permission-mode" then "bypassPermissions" as two separate argv elements), while preflight.mjs re-encodes the same fact as a hand-written regex whose (?:=|\s+) alternation also accepts an equals-joined form. A workers.claude.args entry of ["--permission-mode=bypassPermissions"] (syntactically valid today — nothing in orchestrator-config.mjs forbids it) makes preflight.mjs print PASS Worker shell policy, while launch-worker.mjs's token-array match on that same invocation still finds no "--permission-mode" element and refuses the launch.
· risk: Preflight's whole value is catching an unsafe config before a worktree is created; this divergence lets it green-light a config the authoritative launcher then rejects anyway (so no unattended-and-stuck worker ships, but the "fail fast, no wasted worktree" promise this ticket is about is defeated for that shape). The bigger risk is drift: the next person to touch either file's policy data only has to remember to update one of the two.
· fix: Put the per-engine run-permission fact in one place — e.g. export it from tools/lib/orchestrator-config.mjs (which already centralizes engine-shaped config reading) — and have both launch-worker.mjs and preflight.mjs import it, so one token list backs both the authoritative check and the fast probe.
· reference: CLAUDE.md rule 10 (DRY at the right level)
[MEDIUM] Hardcoded 2s timeout on the two network/IPC-touching preflight checks, no override
· dimension: Correctness (#1)
· location: tools/preflight.mjs:160 (COMMAND_TIMEOUT_MS = 2000), applied to the GitHub-auth and Orca-reachability checks at tools/preflight.mjs:256-258
· issue: The GitHub-auth check (a GitHub API round trip) and the Orca-reachability check (a live runtime query) share one fixed, non-configurable 2000ms budget — well under this codebase's own established wait budget for external-tool calls (WAIT_TIMEOUT_MS = 60000 in both launch-worker.mjs and nudge-worker.mjs).
· risk: A slow network path or a briefly-busy Orca runtime times out at 2s and preflight reports FAIL GitHub authentication / FAIL Orca reachability even though the environment is fine, blocking a legitimate orchestrator run on a false negative (the remedy text does distinguish the timeout case, but the run still stops).
· fix: Either raise the default to something closer to the codebase's other external-tool timeouts, or add a --timeout-ms override (documented in --help) so a slow link doesn't need a code change to pass.
· reference: pattern consistency with the existing WAIT_TIMEOUT_MS precedent in tools/launch-worker.mjs / tools/nudge-worker.mjs
[MEDIUM] The "Target repository" FAIL path has no test case
· dimension: Harness changes need EXECUTED evidence (#15)
· location: tools/preflight.mjs:320-332 (the repoPresent/target-repo check); no matching case in tools/test-tools.mjs's preflightCases (~1005-1120)
· issue: Every preflightCases scenario reuses a repo directory that the test staging helper always creates on disk; none points --repo at a configured path that doesn't exist, so the repoPresent branch's FAIL output and its "restore the configured … repository" remedy have never actually run.
· risk: Per tools/CONVENTIONS.md's Harness Execution bar, a decision path with no case merges unexecuted — a future edit to that check's logic or wording ships unverified.
· fix: Add a preflightCases entry pointing --repo at a repos-map path that doesn't exist, asserting FAIL Target repository plus the restore remedy.
· reference: tools/CONVENTIONS.md ("exercises each tool's real decision paths"); rubric dimension 15
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/* or endpoints.ts changed, only one repo touched |
| security-reviewer | N/A — no orbit-api code changed |
| design-reviewer | N/A — no apps/web/**, apps/mobile/**, or orbit-landing-page/src/** UI file changed |
Frontend-security categories (injection, auth-state leakage) reviewed inline instead:
no shell-string interpolation (subprocess calls take argv arrays throughout, never a
shell-interpreted string), --require <cli> values are regex-validated
(^[a-zA-Z0-9][a-zA-Z0-9._+-]*$) before ever reaching the executable-existence check,
and no secret or token is read into argv. Clean.
Validation
Per this workflow's CI adaptation, /validate is skipped here — this PR runs Build /
Unit Tests / SonarCloud as separate required checks. The PR body itself carries an
already-executed transcript covering the dimension-15 bar: the lint, type-check, and
test tasks (all green), the hook-parity harness (ORBIT HOOK PARITY OK), and the tool
harness gate (ORBIT TOOLS GATE OK, including every new preflight/launch-worker case
named in this review). A real run of the new preflight tool against the author's live
environment is also included, showing the intended FAIL-and-name-the-repair behavior for
an off-base-branch checkout rather than a silent pass.
| Check | Result |
|---|---|
| Lint | PASS (PR body transcript) |
| Type check | PASS (PR body transcript) |
| Tests | PASS (PR body transcript) |
| Build (api) | N/A — orbit-api not touched |
Deferred — N/A dimensions & files not verdicted
- DESIGN.md / AI-slop (#8), Parity (#9), i18n (#10), Contract drift + backward-compat
(#11), Backend hard rules (#13), FEATURES.md parity (#14): all N/A — the diff touches
noapps/*UI file, nopackages/shared/src/types/*, noorbit-apifile, and no
user-facing feature surface. - Security (#12), API side: N/A —
orbit-apinot checked out in this CI job and not
touched by the diff. - Every changed file (
.claude/skills/orchestrate/SKILL.md,tools/README.md,
tools/launch-worker.mjs,tools/preflight.mjs,tools/test-tools.mjs) received a
verdict above. Nothing else deferred.
What's good
tools/preflight.mjsfollows thetools/CONVENTIONS.mdcontract precisely:--help
covers every flag and exit code, non-interactive, cwd-safe via-C, binary overrides
for hermetic tests, and it reports rather than repairs (the gate-tools rule).- The new
launch-worker.mjsrun-permission check reuses the existing per-engine
ENGINE_PROFILESshape and scans the whole resolved invocation (command + args), the
same lesson the file's own header already draws from the headless-token guard — a
flag hidden incommandisn't waved through just because the guard only looked at
args. - The PR fixed every pre-existing test fixture that the new run-permission check would
otherwise have broken (the codex-profile case gained the required bypass flag) rather
than leaving a hidden test gap. - Real command execution against the author's own machine (not just the hermetic suite)
is included as evidence, and it demonstrates the "stop and name the exact repair"
contract working against a genuinely dirty precondition (branch offmain), which is
the behavior this ticket exists to guarantee.
Recommendation
Approve as-is. The three Medium findings are fix-soon, not fix-now: none of them lets an
unsafe unattended launch through today (the diff still refuses a claude acceptEdits
invocation and every existing safety case stays green), and the divergence and timeout
risks only surface under configuration edits or environment conditions that don't exist
in the current orchestrator config. Worth a fast follow-up ticket before the policy data
is touched again.
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary
tools/preflight.mjsgate with text and JSON output for worker policy, required CLIs, GitHub auth, Orca reachability, base branch, and clean tree/orchestratePhase 0 before any worktree and cover the clean and forced-failure paths hermeticallyCloses ORB-107.
Gate output
Preflight table evidence
The configured root UI checkout is intentionally not on
main, so the real preflight stops and names the exact repair instead of changing it:The clean all-PASS environment is proven by the hermetic case in
node tools/test-tools.mjsabove.Rejected launch evidence
Decisions taken unattended
origin/mainbefore implementation validation because ORB-89 had landed the required shared orchestrator-config reader and engine-neutral model-tier schema after the worktree was created.--permission-mode bypassPermissions; Codex requires--dangerously-bypass-approvals-and-sandbox.--require <cli>for ticket-specific executable checks. Third-party service authentication remains ticket-owned as required.