Repository navigation
fix: guard omp launches against missing or invalid default roles - #68
Merged
Merged
Conversation
…isted An omp launch with no --model reads the global modelRoles.default. Any interactive omp session can clear it, and omp then silently runs the first credentialed model. Here that is a free-tier model that answers every call with HTTP 429. fm-spawn now reads the role through omp config get and the catalog through omp models, and refuses with the remedy.
…configuration directory
…t-role validation
… timeout regression
…: Bash 5.2 removed inline single quotes from the expected __MODELFLAG__ replacement, causing a false mismatch against the correctly quoted launch. The test now uses literal variable-based replacement, matching production’s portable pattern. Updated coverage documentation; production behavior is unchanged. Local CI-runner verification passed the formerly failing raw-model case and all related guard cases, including the timeout-bound long glob. Executable launch smoke and bash -n passed. The full local suite subsequently stopped at the unrelated restriction against secondmate fixture homes inside the repository. Linux CI was not rerun
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Intent
I would like all bugs to be fixed so tomorrow can be focused entirely to Vernant and not problems preventing Vernant from getting built. A recommended architecture should come with proof.
Context: omp (Oh My Pi) keeps its model roles in one shared global file (~/.omp/agent/config.yml, modelRoleStorage: global). modelRoles.default was lost from that file. omp did not fail. It silently picked the first model with credentials, google/gemini-3.1-pro-preview, which is free tier and answers HTTP 429. Pipeline runs and omp worker launches failed because of it. Isolated repro with omp 18.8.1 shows a missing default and an unresolvable default both resolve to that model; --config overlay beats the global file; --model beats both.
Decisions already made (deliberate, not mistakes):
Tests are behavioral only (fake omp executable driving fm-spawn; five cases in tests/fm-omp-harness.test.sh). Do not add source-text assertions. Do not change ~/.no-mistakes/config.yaml or other captain-private config.
What Changed
Risk Assessment
✅ Low: The changes are bounded to the authorized launch guard and its regression coverage, with no additional material defects or unrequired components substantiated in this pass.
Testing
Live CLI admission and real omp RPC checks passed, including colon-preserving model selection and long-glob pass-through without catalog probing; baseline ancestry and early timing limitations were addressed with focused selectors and serialized verification. The fake-harness regression demonstrated fixed-code success and old-regex timeout failure, but did not establish a live result for the bounded external-timeout scenario. CLI/RPC evidence was retained and disposable fixtures removed.
Evidence: Live spawn decisions, remedies, and persisted task state
Source: Live spawn decisions, remedies, and persisted task state
Evidence: Selected fields from genuine omp RPC responses
Source: Selected fields from genuine omp RPC responses
Evidence: Original three-second regression: fixed passes, old regex times out
Source: Original three-second regression: fixed passes, old regex times out
Evidence: Baseline and completed targeted-selector results
Source: Baseline and completed targeted-selector results
Pipeline
Updates from git push no-mistakes
... (11 earlier update rounds omitted to keep the PR body within GitHub's 65536-char limit; full history is in the run log.)
🔧 **Review** - 2 issues found → auto-fixed (8) ✅
🔧 Fix applied.
8 issues (5 errors, 3 warnings) still open:
bin/fm-spawn.sh:2285- Intent criterion 3 requires that "an unknown provider passes with a notice." With no explicit model and modelRoles.default='claude-bridge/claude-opus-4-8', omp_catalog_verdict returns unknown-provider, but the added hunk checks onlyif [ "$verdict" = unlisted ]and then returns success silently. The explicit-model sibling at bin/fm-spawn.sh:2229 prints the required notice; the default-role consumer at bin/fm-spawn.sh:2284 must preserve that contract too. Add the unknown-provider notice without changing pass-through behavior.bin/fm-spawn.sh:2829- The new guard also runs for supported raw launch commands, but MODEL does not include their native arguments. With the optional session-launch restriction disabled, a spawn using the documented launch-command interface withomp --model openai-codex/gpt-6-astraand a missing shared default resolves HARNESS=omp at lines 2590–2603 while MODEL remains empty. The check at bin/fm-spawn.sh:2269 therefore reads the shared role and refuses this explicitly pinned launch, although it cannot suffer the fallback being guarded. Make default-role validation depend on the effective model override for the supported raw omp path, rather than only the separate fm-spawn MODEL variable. Both the call at bin/fm-spawn.sh:2829 and its dependency predicate at bin/fm-spawn.sh:2269 must agree with the actual launch.bin/fm-spawn.sh:2315- The new probe is not a read-only inspection of the global file. Criterion 4 requires that the guard "reads the global config only," and criterion 5 states "Firstmate code never writes that file." In omp 18.8.1, config get initializes writable Settings and returns the effective merged setting (https://github.com/can1357/oh-my-pi/blob/v18.8.1/packages/coding-agent/src/cli/config-cli.ts#L163-L164 and #L258-L275). Two concrete failures follow: (1) a listed default in the supervisor cwd's project config masks a missing global default, so a worker launched into another project without that override passes the guard and falls back; (2) malformed global YAML is renamed to .broken-* during initialization (https://github.com/can1357/oh-my-pi/blob/v18.8.1/packages/coding-agent/src/config/settings.ts#L2281-L2325), after which the suppressed probe failure permits launch against the now-missing config instead of preserving the startup parse error. Replace this probe with non-mutating, global-only role inspection at the shared validator, retaining unreadable-evidence pass-through. Affected sibling sites: bin/fm-spawn.sh:2317 consumes the merged default; bin/fm-spawn.sh:2880 applies it to canonical and raw launches; docs/configuration.md:977-981 promises global-only, non-writing behavior; tests/fm-omp-harness.test.sh:118-124 models neither layering nor initialization side effects.bin/fm-spawn.sh:2324- The R3 fix round reads the invoking process's agent directory, but leaves the actual launch's configuration identity unaligned. With a listed default in directory A and a missing default in directory B, the supported raw commandPI_CODING_AGENT_DIR=/B omp --auto-approveresolves HARNESS=omp, passes this guard using A, then launches against B and still reaches the credentialed-model fallback. Canonical launches have the same mismatch when the invoking process sets PI_CODING_AGENT_DIR but the persistent tmux pane inherits another value: the omp template does not forward it, and the launch-env allowlist can remove it. Resolve the actual launch's effective global-config directory at the shared validation boundary and keep role inspection, catalog inspection, and execution aligned. Related sites: bin/fm-spawn.sh:2278-2310 (raw assignments are skipped, not applied to config inspection), bin/fm-spawn.sh:2253 (catalog uses invoking-process environment), bin/fm-spawn.sh:2887 (both launch paths use this validator), docs/configuration.md:977 (claims inspection of the launch's shared Default role), tests/fm-omp-harness.test.sh:316 and :328 (custom-directory cases record launch text but do not establish which directory the launched process receives).bin/fm-spawn.sh:2306- Round 3's R4 fix introduces a guard bypass for ordinary raw-command redirections. With /B/config.yml containingmodelRoles: {},PI_CODING_AGENT_DIR=/B omp --auto-approve 2>/B/errors.logresolves HARNESS=omp, but the redirection makestokens.every(token => token.type === "word")false. Lines 2316 and 2334 then discard the known directory and permit the launch, leaving the credentialed-model fallback reachable. The redirection does not make this literal directory uncertain. Preserve directory evidence for an otherwise supported simple command with redirections; the existing token collector already skips their targets. Related sites: bin/fm-spawn.sh:2298 skips redirections; :2316 clears the directory; :2334 bypasses validation; docs/configuration.md:981 promises that literal absolute assignments are honored.bin/fm-spawn.sh:2316- Round 3's R4 fix still treats PI_CODING_AGENT_DIR as the launch's effective directory when omp profile selection overrides it. For example, let /A/config.yml have a listed default and ~/.omp/profiles/work/agent/config.yml have no default. The supported raw commandPI_CODING_AGENT_DIR=/A omp --profile work --auto-approvepasses this guard using /A, then omp activates work and launches against the missing profile default. omp 18.8.1 explicitly ignores agent-directory overrides for named profiles and replaces PI_CODING_AGENT_DIR during activation (https://github.com/can1357/oh-my-pi/blob/v18.8.1/packages/utils/src/dirs.ts); CLI bootstrap applies both profile flags and environment selection (https://github.com/can1357/oh-my-pi/blob/v18.8.1/packages/coding-agent/src/cli.ts). Remaining sibling inputs are OMP_PROFILE, PI_PROFILE, --profile, and --profile=; canonical launches also remain vulnerable when the destination pane inherits a different profile. At the shared directory-evidence boundary, mark profile-dependent identity unreadable whenever it cannot be established, as the R4 instructions authorize, rather than inspecting an unrelated directory. Related sites: bin/fm-spawn.sh:2307-2313 processes assignments without accounting for profile overrides; :2318-2325 scans arguments without profile handling; :2356 probes the catalog under the invoking process's profile environment; :2910-2913 forwards a directory without establishing the pane's profile; docs/configuration.md:981 claims alignment.bin/fm-spawn.sh:2321- Round 4's R6 fix leaves shell-expanded profile flags behind. With OMP_PROFILE and PI_PROFILE unset, /A/config.yml containingmodelRoles: {}, and the destination pane's PROFILE_FLAG set to--profile=work, the supported raw commandPI_CODING_AGENT_DIR=/A omp "$PROFILE_FLAG" --auto-approveshould pass through to the work profile. Instead, the lexer marks the argument nonliteral but the argument scan ignores that evidence, retainscertain=true, reads /A, and refuses the launch. This contradicts the selected rule: "when the effective directory cannot be established with certainty, pass through." At the shared raw-command evidence boundary, treat shell-expanded option positions as unreadable rather than inspecting an unrelated directory; do not evaluate the expansion. Related sites: bin/fm-spawn.sh:2307 initializes certainty from token types only; :2319-2327 consumes argument values without their literal/expansion flags; :2328 retains the directory; :2338 and :2359 respectively read the role and probe its catalog; docs/configuration.md:981 promises shell-expansion pass-through; tests/fm-omp-harness.test.sh:423 covers only literal profile flags. Upstream confirms that the expanded--profile=workselects the profile: https://github.com/can1357/oh-my-pi/blob/v18.8.1/packages/coding-agent/src/cli/profile-bootstrap.ts.bin/fm-spawn.sh:2294- Round 5's selected R7 fix (2c836b8) leaves tilde expansion outside the new pass-through rule. The decision requires that "any shell-expanded token in a raw launch command makes it unreadable evidence that passes through unchanged," but the added predicate checks only !token.literal or token.unquotedExpansion. The reused Lexer flags dollar/substitution and wildcard/brace forms, not unquoted tilde prefixes (bin/fm-arm-command-policy.mjs:384-385). With profiles unset, /A/config.yml containing modelRoles: {}, and raw commands enabled,PI_CODING_AGENT_DIR=/A omp --auto-approve 2>~/omp-errors.logretains certain=true and is refused, although its redirection target undergoes shell expansion. With an unlisted default, it also performs the catalog probe that Option C forbids. Recognize unquoted tilde expansion at this guard's shared evidence check without evaluating it or changing canonical launches. Related changed sites: bin/fm-spawn.sh:2299-2301 skips redirection targets after classification; :2327 retains directory evidence; :2349 refuses missing roles; :2358 probes unlisted roles; docs/configuration.md:981 promises every expanded token passes through; tests/fm-omp-harness.test.sh:478 enumerates expansion cases but omits tilde-expanded arguments and redirection targets.🔧 Fix applied.
9 issues (6 errors, 3 warnings) still open:
bin/fm-spawn.sh:2285- Intent criterion 3 requires that "an unknown provider passes with a notice." With no explicit model and modelRoles.default='claude-bridge/claude-opus-4-8', omp_catalog_verdict returns unknown-provider, but the added hunk checks onlyif [ "$verdict" = unlisted ]and then returns success silently. The explicit-model sibling at bin/fm-spawn.sh:2229 prints the required notice; the default-role consumer at bin/fm-spawn.sh:2284 must preserve that contract too. Add the unknown-provider notice without changing pass-through behavior.bin/fm-spawn.sh:2829- The new guard also runs for supported raw launch commands, but MODEL does not include their native arguments. With the optional session-launch restriction disabled, a spawn using the documented launch-command interface withomp --model openai-codex/gpt-6-astraand a missing shared default resolves HARNESS=omp at lines 2590–2603 while MODEL remains empty. The check at bin/fm-spawn.sh:2269 therefore reads the shared role and refuses this explicitly pinned launch, although it cannot suffer the fallback being guarded. Make default-role validation depend on the effective model override for the supported raw omp path, rather than only the separate fm-spawn MODEL variable. Both the call at bin/fm-spawn.sh:2829 and its dependency predicate at bin/fm-spawn.sh:2269 must agree with the actual launch.bin/fm-spawn.sh:2315- The new probe is not a read-only inspection of the global file. Criterion 4 requires that the guard "reads the global config only," and criterion 5 states "Firstmate code never writes that file." In omp 18.8.1, config get initializes writable Settings and returns the effective merged setting (https://github.com/can1357/oh-my-pi/blob/v18.8.1/packages/coding-agent/src/cli/config-cli.ts#L163-L164 and #L258-L275). Two concrete failures follow: (1) a listed default in the supervisor cwd's project config masks a missing global default, so a worker launched into another project without that override passes the guard and falls back; (2) malformed global YAML is renamed to .broken-* during initialization (https://github.com/can1357/oh-my-pi/blob/v18.8.1/packages/coding-agent/src/config/settings.ts#L2281-L2325), after which the suppressed probe failure permits launch against the now-missing config instead of preserving the startup parse error. Replace this probe with non-mutating, global-only role inspection at the shared validator, retaining unreadable-evidence pass-through. Affected sibling sites: bin/fm-spawn.sh:2317 consumes the merged default; bin/fm-spawn.sh:2880 applies it to canonical and raw launches; docs/configuration.md:977-981 promises global-only, non-writing behavior; tests/fm-omp-harness.test.sh:118-124 models neither layering nor initialization side effects.bin/fm-spawn.sh:2324- The R3 fix round reads the invoking process's agent directory, but leaves the actual launch's configuration identity unaligned. With a listed default in directory A and a missing default in directory B, the supported raw commandPI_CODING_AGENT_DIR=/B omp --auto-approveresolves HARNESS=omp, passes this guard using A, then launches against B and still reaches the credentialed-model fallback. Canonical launches have the same mismatch when the invoking process sets PI_CODING_AGENT_DIR but the persistent tmux pane inherits another value: the omp template does not forward it, and the launch-env allowlist can remove it. Resolve the actual launch's effective global-config directory at the shared validation boundary and keep role inspection, catalog inspection, and execution aligned. Related sites: bin/fm-spawn.sh:2278-2310 (raw assignments are skipped, not applied to config inspection), bin/fm-spawn.sh:2253 (catalog uses invoking-process environment), bin/fm-spawn.sh:2887 (both launch paths use this validator), docs/configuration.md:977 (claims inspection of the launch's shared Default role), tests/fm-omp-harness.test.sh:316 and :328 (custom-directory cases record launch text but do not establish which directory the launched process receives).bin/fm-spawn.sh:2306- Round 3's R4 fix introduces a guard bypass for ordinary raw-command redirections. With /B/config.yml containingmodelRoles: {},PI_CODING_AGENT_DIR=/B omp --auto-approve 2>/B/errors.logresolves HARNESS=omp, but the redirection makestokens.every(token => token.type === "word")false. Lines 2316 and 2334 then discard the known directory and permit the launch, leaving the credentialed-model fallback reachable. The redirection does not make this literal directory uncertain. Preserve directory evidence for an otherwise supported simple command with redirections; the existing token collector already skips their targets. Related sites: bin/fm-spawn.sh:2298 skips redirections; :2316 clears the directory; :2334 bypasses validation; docs/configuration.md:981 promises that literal absolute assignments are honored.bin/fm-spawn.sh:2316- Round 3's R4 fix still treats PI_CODING_AGENT_DIR as the launch's effective directory when omp profile selection overrides it. For example, let /A/config.yml have a listed default and ~/.omp/profiles/work/agent/config.yml have no default. The supported raw commandPI_CODING_AGENT_DIR=/A omp --profile work --auto-approvepasses this guard using /A, then omp activates work and launches against the missing profile default. omp 18.8.1 explicitly ignores agent-directory overrides for named profiles and replaces PI_CODING_AGENT_DIR during activation (https://github.com/can1357/oh-my-pi/blob/v18.8.1/packages/utils/src/dirs.ts); CLI bootstrap applies both profile flags and environment selection (https://github.com/can1357/oh-my-pi/blob/v18.8.1/packages/coding-agent/src/cli.ts). Remaining sibling inputs are OMP_PROFILE, PI_PROFILE, --profile, and --profile=; canonical launches also remain vulnerable when the destination pane inherits a different profile. At the shared directory-evidence boundary, mark profile-dependent identity unreadable whenever it cannot be established, as the R4 instructions authorize, rather than inspecting an unrelated directory. Related sites: bin/fm-spawn.sh:2307-2313 processes assignments without accounting for profile overrides; :2318-2325 scans arguments without profile handling; :2356 probes the catalog under the invoking process's profile environment; :2910-2913 forwards a directory without establishing the pane's profile; docs/configuration.md:981 claims alignment.bin/fm-spawn.sh:2321- Round 4's R6 fix leaves shell-expanded profile flags behind. With OMP_PROFILE and PI_PROFILE unset, /A/config.yml containingmodelRoles: {}, and the destination pane's PROFILE_FLAG set to--profile=work, the supported raw commandPI_CODING_AGENT_DIR=/A omp "$PROFILE_FLAG" --auto-approveshould pass through to the work profile. Instead, the lexer marks the argument nonliteral but the argument scan ignores that evidence, retainscertain=true, reads /A, and refuses the launch. This contradicts the selected rule: "when the effective directory cannot be established with certainty, pass through." At the shared raw-command evidence boundary, treat shell-expanded option positions as unreadable rather than inspecting an unrelated directory; do not evaluate the expansion. Related sites: bin/fm-spawn.sh:2307 initializes certainty from token types only; :2319-2327 consumes argument values without their literal/expansion flags; :2328 retains the directory; :2338 and :2359 respectively read the role and probe its catalog; docs/configuration.md:981 promises shell-expansion pass-through; tests/fm-omp-harness.test.sh:423 covers only literal profile flags. Upstream confirms that the expanded--profile=workselects the profile: https://github.com/can1357/oh-my-pi/blob/v18.8.1/packages/coding-agent/src/cli/profile-bootstrap.ts.bin/fm-spawn.sh:2294- Round 5's selected R7 fix (2c836b8) leaves tilde expansion outside the new pass-through rule. The decision requires that "any shell-expanded token in a raw launch command makes it unreadable evidence that passes through unchanged," but the added predicate checks only !token.literal or token.unquotedExpansion. The reused Lexer flags dollar/substitution and wildcard/brace forms, not unquoted tilde prefixes (bin/fm-arm-command-policy.mjs:384-385). With profiles unset, /A/config.yml containing modelRoles: {}, and raw commands enabled,PI_CODING_AGENT_DIR=/A omp --auto-approve 2>~/omp-errors.logretains certain=true and is refused, although its redirection target undergoes shell expansion. With an unlisted default, it also performs the catalog probe that Option C forbids. Recognize unquoted tilde expansion at this guard's shared evidence check without evaluating it or changing canonical launches. Related changed sites: bin/fm-spawn.sh:2299-2301 skips redirection targets after classification; :2327 retains directory evidence; :2349 refuses missing roles; :2358 probes unlisted roles; docs/configuration.md:981 promises every expanded token passes through; tests/fm-omp-harness.test.sh:478 enumerates expansion cases but omits tilde-expanded arguments and redirection targets.bin/fm-spawn.sh:2295- The latest R8 fix (1f2cae6) introduces catastrophic backtracking: the unquoted-character branch has an inner '+' inside an outer '+'. For the supported raw commandPI_CODING_AGENT_DIR=/A omp --auto-approve /work/vernant/generated/reports/input*.txt, the disallowed '*' forces the matcher to explore exponentially many partitions of the preceding literal characters. [INFERENCE from static regex analysis] This can stall fm-spawn indefinitely instead of passing unreadable evidence through unchanged. Remove the inner '+' from the unquoted-character branch so each outer repetition consumes one literal character or one complete quoted segment, preserving the required allow-list. Related sites: bin/fm-spawn.sh:2297 applies this predicate to command words, assignments, arguments, and redirection targets; :2282 synchronously waits for the Node process; tests/fm-omp-harness.test.sh:508 quotes the long glob prefix, avoiding this failure; docs/configuration.md:981 promises expansion pass-through.🔧 Fix applied.
1 error still open:
tests/fm-omp-harness.test.sh:514- Round 7's R9 fix introduces a false-positive regression test on hosts with timeout/gtimeout, including Ubuntu CI. The new Node wrapper calls fm_run_timed, whose external-timeout arm backgrounds the command without preserving stdin (bin/fm-timeout-lib.sh:157-164). Non-interactive Bash therefore supplies /dev/null instead of the validator's JavaScript heredoc from bin/fm-spawn.sh:2282. Node executes an empty program successfully; dependency is empty, agent_dir becomes empty, and validation passes through at bin/fm-spawn.sh:2338. Consequently, restoring the catastrophic regex would still pass this case without reaching the regex or timing out. Preserve the incoming script through an inherited file descriptor and restore it inside the bounded command. Related changed sites: tests/fm-omp-harness.test.sh:515 records only actual timeouts; :535 checks that marker's absence; :536-541 also accept this skipped-validation path.🔧 Fix applied.
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
bash tests/fm-omp-harness.test.sh— initial attempt stopped at the ancestry-sensitiveompddetection check, before the changed scenarios.env TMPDIR="$PWD/.nm-omp-validation/tmp" FM_TEST_SKIP_ORPHAN_REAP=1 bash tests/.nm-omp-targeted.sh— model-validation, default-role refusal, and read-only/global-only selectors completed successfully; the outer deadline interrupted the remaining selectors.env TMPDIR="$PWD/.nm-omp-validation/tmp" FM_TEST_SKIP_ORPHAN_REAP=1 bash tests/.nm-omp-literal.sh— unquoted, single-quoted, and double-quoted literal command boundaries passed.bash tests/.nm-omp-long-glob.shandbash tests/.nm-omp-mutation.sh— after serializing validation, the unchanged three-second regression passed corrected code and failed the temporarily restored nested-quantifier regex by timeout.python3 .nm-omp-validation/live.py— drove isolated real fm-spawn admission scenarios using installed omp, Treehouse, and a private tmux socket.env SCENARIO=colon python3 .nm-omp-validation/handshake.pyandenv SCENARIO=long-glob python3 .nm-omp-validation/handshake.py— observed real omp RPC state against a disposable localhost model catalog.env SCENARIO=unreadable python3 .nm-omp-validation/handshake.py— verified pass-through when a disposable extension made the real vendor catalog output unreadable.Stopped owned tmux servers and the localhost endpoint, removed marked labs, temporary executables, selector runners, caches, and fixture repositories; confirmed no matching worktree fixtures remained.✅ **Document** - passed
✅ No issues found.
🔧 **Lint** - 1 issue found → auto-fixed ✅
🔧 Fix applied.
✅ Re-checked - no issues remain.
✅ **Push** - passed
✅ No issues found.