fix(omp): route branch bash through extension runner - #170
Merged
Merged
Conversation
OMP 18.3.0 moved the bash tool's env parameter behind service mode, but the legacy createBashToolDefinition shim still forwards the spawnHook's env into the native bash execute, which throws "ready and env require a service name." before spawning. Every supervision-branch bash call has failed since the upgrade, so the branch could not inspect, drain, or acknowledge wakes. Hand the shim a BashOperations runner in lib/fm-async-exec.ts instead: the tool now executes through runCommandAsync with the spawnHook's injected actor environment (FM_SUPERVISION_ACTOR=branch, FM_LEASE_HOLDER_PID, and the scriptEnv home overrides), streaming onData, signal abort, and the native 300-second default timeout. The seam predates 18.3, so the same wiring is correct on every supported OMP version, and an older shim that ignores the option keeps the env-forwarding path that works there. The readonly prelude still makes an in-shell actor-env override fail loudly. tests/fm-omp-branch-bash.test.sh drives the real installed omp against a probe extension wired through the same two seams: before this change it failed with the reported throw; now it proves the branch actor drains and acknowledges a granted wake row end to end while a main drain only sees the held notice.
…ild process trees
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
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
Fix the OMP supervision branch so its bash tool works on OMP 18.3.x.
Root cause (full diagnosis: data/fm-branch-executor-fails-inspection/report.md): .omp/extensions/fm-branch-supervision-omp.ts builds the branch bash tool with createBashToolDefinition(fmRoot, { spawnHook }), and the spawnHook returns env. OMP 18.3.0 moved bash env behind service mode; its legacy shim still forwards env: spawn?.env into the native bash tool, which throws "ready and env require a service name." before spawning. Every branch bash call has failed since the 18.3.0 install, so the branch cannot inspect, drain, or acknowledge wakes.
Required: apply the report's recommended firstmate-side fix - pass operations.exec to createBashToolDefinition so the extension executes through its own runner with the actor env (FM_SUPERVISION_ACTOR=branch, FM_LEASE_HOLDER_PID, FM_HOME and the other injected variables) - or the alternative only if the recommended one proves unworkable (record why). Keep the confused-agent-grade actor injection intact: the branch's commands must still carry FM_SUPERVISION_ACTOR=branch and the lease holder identity, so bin/fm-wake-drain.sh scopes branch acks correctly. Must work on OMP 18.3.x and not break older OMP versions the repo supports (the adapter's documented version floor is OMP 17.1.8; the operations seam exists there with the same shape).
Out of scope: upstream OMP changes (an upstream issue draft was recorded in evidence instead), other extensions, the main primary adapter. Never run Firstmate supervision scripts against ~/Desktop/firstmate; reproduce in a scratch home only.
Acceptance criteria:
Firstmate-Validation-Generation: e9de87fe77406a108a2337162a4edea7
What Changed
BashOperationsrunner, preserving injected actor and lease-holder environment on OMP 18.3.x and earlier supported versions.Risk Assessment
✅ Low: The operations seam wiring preserves actor environment injection, timeout behavior, and process-tree cancellation without introducing a source-verifiable defect in the reviewed changes.
Testing
Against the real installed OMP 18.3.0, the new branch bash regression exercised command execution, injected actor identity, readonly override protection, and branch-scoped wake acknowledgement successfully; existing supervision tests also passed. The prior payload did not establish live results for the extension typecheck or documentation scenario.
bash tests/fm-omp-branch-bash.test.shartifactEvidence: OMP 18.3 branch bash regression
Evidence: OMP extension typecheck failure
Evidence: OMP branch supervision tests
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 2 issues found → auto-fixed ✅
.omp/extensions/lib/fm-async-exec.ts:149-execBashToolimposes a 300-second timeout whenever the tool caller omitstimeout(.omp/extensions/lib/fm-async-exec.ts:147-162). The prior native bash path leaves an omitted timeout unset, so this silently changes valid long-running branch commands into failures after five minutes. Preserve the native semantics by passing no timeout whenoptions.timeoutis undefined (and only configuringtimeoutMswhen explicitly provided)..omp/extensions/lib/fm-async-exec.ts:95- The new runner aborts only the top-levelbashprocess withchild.kill()on cancellation, timeout, and output overflow (.omp/extensions/lib/fm-async-exec.ts:68-72,93-97,103-106). Unlike OMP's native executor, this does not terminate the command's process tree; a branch command that is running a foreground child can leave that child alive after the tool reports failure, allowing injected branch-actor commands to continue mutating wake/lease state after supervision has moved on. Kill the owned process group/tree before settling these paths.🔧 Fix applied.
✅ Re-checked - no issues remain.
tests/fm-omp-branch-types.test.sh:89- The required extension typecheck fails against installed OMP 18.3.0 because unchanged fm-task-inbox-doorbell.ts omits the now-required failureJournal option. This is outside the changed files but leaves AC4 unproven.bash tests/fm-omp-branch-bash.test.shartifactomp --versionbash tests/fm-omp-branch-bash.test.shbash tests/fm-omp-branch-supervision.test.shbash tests/fm-omp-branch-types.test.sh✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.