feat(pi-plugin): OMP-native subagent runner for the historian - #418
feat(pi-plugin): OMP-native subagent runner for the historian#418Qiiks wants to merge 10 commits into
Conversation
Run the historian through OMP's in-process structured-subagent machinery (runStructuredSubagent) instead of a headless --print subprocess on OMP hosts, per cortexkit#416. Non-OMP Pi hosts keep the subprocess path unchanged. - omp-host.ts: memoized never-throw surface loader resolving runStructuredSubagent + Settings + discoverAuthStorage from the running OMP package (entry walk → module-sibling walk → bare subpath) - omp-subagent-runner.ts: SubagentRunner implementation building the synthetic ToolSession (enableIrc:false isolation, restrictToolNames, detached, keepAlive:false), historian persona threaded as a system_role assignment block + yield contract instruction, timeout racing an internal AbortController, fail-soft result mapping with transient hints for rate-limit classes, best-effort subagent_invocations accounting under a new 'omp' harness value - storage: harness unions widened via SubagentInvocationHarness alias - index.ts: historian runner swapped only when isOmpHostProcess() and the surface is available; recomp/wrapup/upgrade unchanged Live-verified against OMP 18.1.5: real structured spawn completed with yielded output, resolved model, and usage mapped to ok:true. Unit tests cover surface-unavailable fallback, result mapping, request-shape isolation flags, timeout, and caller-abort races.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
9 issues found across 7 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/pi-plugin/src/omp-subagent-runner.test.ts">
<violation number="1" location="packages/pi-plugin/src/omp-subagent-runner.test.ts:185">
P3: The "caller abort" test does not actually exercise the abort race it is named for. `runner.run()` is async and suspends at its first `await this.resolveSurface()` (line ~198), so the test's synchronous `controller.abort()` runs before `omp-subagent-runner.ts` registers `options.signal.addEventListener("abort", ...)` (line ~218). Only the pre-aborted guard `if (options.signal?.aborted)` (line ~220) and the mock's own `request.signal?.aborted` branch resolve — the live in-flight abort path (signal firing while `Settings.init` or `runStructuredSubagent` is pending) is never covered, and a regression there would pass this test. To cover the actual race, `await` a tick (e.g. `await Promise.resolve()`) after `run()` so the listener is registered before `controller.abort()`.</violation>
</file>
<file name="packages/pi-plugin/src/omp-host.ts">
<violation number="1" location="packages/pi-plugin/src/omp-host.ts:129">
P2: When OMP is started from a source or Nix install identified by `PI_PACKAGE_DIR`, this loader never tries that package root and falls back from native historian spawning. Include the validated `PI_PACKAGE_DIR` root in both structured-module and dependency resolution.</violation>
<violation number="2" location="packages/pi-plugin/src/omp-host.ts:135">
P2: When OMP is launched through a symlinked `omp` bin, the loader walks the shim directory instead of the package root and can miss the native surface. Resolve `candidate` with `realpathSync` before taking its directory, matching the existing Pi resolver.</violation>
<violation number="3" location="packages/pi-plugin/src/omp-host.ts:214">
P2: The documented colocated-install fallback never checks the sibling `@oh-my-pi/pi-coding-agent` package, so this branch always misses when bare ESM resolution is unavailable. Resolve the OMP package from the plugin's sibling `node_modules` and pass that root to `extractSurface`.</violation>
<violation number="4" location="packages/pi-plugin/src/omp-host.ts:220">
P2: When OMP uses a Jiti virtual entry, the bare structured-subagent import cannot resolve `Settings` or `discoverAuthStorage` because `extractSurface` never derives the imported package root. Resolve those companion modules from the bare package's install location.</violation>
</file>
<file name="packages/pi-plugin/src/index.ts">
<violation number="1" location="packages/pi-plugin/src/index.ts:732">
P1: When an OMP host is detected but the native surface cannot load, this branch still selects `OmpSubagentRunner` because surface loading is deferred until `run()`. The historian then returns `spawn_failed` and never invokes `PiSubagentRunner`, so the documented subprocess fallback does not occur; resolve the surface before selecting the runner or make the OMP runner delegate to the subprocess runner on surface failure.</violation>
</file>
<file name="packages/pi-plugin/src/omp-subagent-runner.ts">
<violation number="1" location="packages/pi-plugin/src/omp-subagent-runner.ts:241">
P2: A throwing `onProgress` callback can make this fail-soft runner reject, especially because the catch path invokes the callback directly again. Route every progress event through a helper that catches callback exceptions.</violation>
<violation number="2" location="packages/pi-plugin/src/omp-subagent-runner.ts:248">
P1: When OMP marks a result `truncated: true`, this predicate still returns `ok: true` and passes partial output to historian validation. Map truncation to `{ ok: false, reason: "truncated" }` so fallback and retry logic can run.</violation>
<violation number="3" location="packages/pi-plugin/src/omp-subagent-runner.ts:248">
P2: If the host resolves after the caller or deadline signal fires without setting `single.aborted`, this predicate accepts the late output. Let `controller.signal.aborted` win before accepting success, and map it to `timeout` or `abort` accordingly.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| // of a headless `--print` process. Falls back to the subprocess | ||
| // runner whenever the OMP surface is unavailable (cortexkit/magic-context#416). | ||
| runner: | ||
| isOmpHostProcess() && resolveOmpRunner !== undefined |
There was a problem hiding this comment.
P1: When an OMP host is detected but the native surface cannot load, this branch still selects OmpSubagentRunner because surface loading is deferred until run(). The historian then returns spawn_failed and never invokes PiSubagentRunner, so the documented subprocess fallback does not occur; resolve the surface before selecting the runner or make the OMP runner delegate to the subprocess runner on surface failure.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/pi-plugin/src/index.ts, line 732:
<comment>When an OMP host is detected but the native surface cannot load, this branch still selects `OmpSubagentRunner` because surface loading is deferred until `run()`. The historian then returns `spawn_failed` and never invokes `PiSubagentRunner`, so the documented subprocess fallback does not occur; resolve the surface before selecting the runner or make the OMP runner delegate to the subprocess runner on surface failure.</comment>
<file context>
@@ -699,7 +724,14 @@ export function resolveHistorianFromConfig(
+ // of a headless `--print` process. Falls back to the subprocess
+ // runner whenever the OMP surface is unavailable (cortexkit/magic-context#416).
+ runner:
+ isOmpHostProcess() && resolveOmpRunner !== undefined
+ ? resolveOmpRunner
+ : new PiSubagentRunner(),
</file context>
|
|
||
| const usage = single.usage; | ||
| const successResult: SubagentRunResult = | ||
| single.exitCode === 0 && !single.error && single.aborted !== true |
There was a problem hiding this comment.
P1: When OMP marks a result truncated: true, this predicate still returns ok: true and passes partial output to historian validation. Map truncation to { ok: false, reason: "truncated" } so fallback and retry logic can run.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/pi-plugin/src/omp-subagent-runner.ts, line 248:
<comment>When OMP marks a result `truncated: true`, this predicate still returns `ok: true` and passes partial output to historian validation. Map truncation to `{ ok: false, reason: "truncated" }` so fallback and retry logic can run.</comment>
<file context>
@@ -0,0 +1,317 @@
+
+ const usage = single.usage;
+ const successResult: SubagentRunResult =
+ single.exitCode === 0 && !single.error && single.aborted !== true
+ ? { ok: true, assistantText: single.output.trim(), durationMs: duration() }
+ : (() => {
</file context>
| // ancestor `node_modules` also hosts `@oh-my-pi/pi-coding-agent` — | ||
| // importing the subpath from there loads the exact copy the host runs. | ||
| const here = fileURLToPath(import.meta.url); | ||
| const packageRoot = findOmpPackageRoot(dirname(here)); |
There was a problem hiding this comment.
P2: The documented colocated-install fallback never checks the sibling @oh-my-pi/pi-coding-agent package, so this branch always misses when bare ESM resolution is unavailable. Resolve the OMP package from the plugin's sibling node_modules and pass that root to extractSurface.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/pi-plugin/src/omp-host.ts, line 214:
<comment>The documented colocated-install fallback never checks the sibling `@oh-my-pi/pi-coding-agent` package, so this branch always misses when bare ESM resolution is unavailable. Resolve the OMP package from the plugin's sibling `node_modules` and pass that root to `extractSurface`.</comment>
<file context>
@@ -0,0 +1,248 @@
+ // ancestor `node_modules` also hosts `@oh-my-pi/pi-coding-agent` —
+ // importing the subpath from there loads the exact copy the host runs.
+ const here = fileURLToPath(import.meta.url);
+ const packageRoot = findOmpPackageRoot(dirname(here));
+ if (packageRoot) {
+ const moduleEntry = join(packageRoot, "src", "task", "structured-subagent.ts");
</file context>
| if (process.env.JITI_VIRTUAL_SCRIPT_PREFIX && process.argv[1]?.startsWith?.(process.env.JITI_VIRTUAL_SCRIPT_PREFIX)) { | ||
| return { error: "running entry is a Jiti virtual module" }; | ||
| } | ||
| const entryCandidates = [process.argv[1], process.execPath].filter( |
There was a problem hiding this comment.
P2: When OMP is started from a source or Nix install identified by PI_PACKAGE_DIR, this loader never tries that package root and falls back from native historian spawning. Include the validated PI_PACKAGE_DIR root in both structured-module and dependency resolution.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/pi-plugin/src/omp-host.ts, line 129:
<comment>When OMP is started from a source or Nix install identified by `PI_PACKAGE_DIR`, this loader never tries that package root and falls back from native historian spawning. Include the validated `PI_PACKAGE_DIR` root in both structured-module and dependency resolution.</comment>
<file context>
@@ -0,0 +1,248 @@
+ if (process.env.JITI_VIRTUAL_SCRIPT_PREFIX && process.argv[1]?.startsWith?.(process.env.JITI_VIRTUAL_SCRIPT_PREFIX)) {
+ return { error: "running entry is a Jiti virtual module" };
+ }
+ const entryCandidates = [process.argv[1], process.execPath].filter(
+ (candidate): candidate is string => typeof candidate === "string" && candidate.length > 0,
+ );
</file context>
| for (const candidate of entryCandidates) { | ||
| let startDir: string; | ||
| try { | ||
| const stat = statSync(candidate); |
There was a problem hiding this comment.
P2: When OMP is launched through a symlinked omp bin, the loader walks the shim directory instead of the package root and can miss the native surface. Resolve candidate with realpathSync before taking its directory, matching the existing Pi resolver.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/pi-plugin/src/omp-host.ts, line 135:
<comment>When OMP is launched through a symlinked `omp` bin, the loader walks the shim directory instead of the package root and can miss the native surface. Resolve `candidate` with `realpathSync` before taking its directory, matching the existing Pi resolver.</comment>
<file context>
@@ -0,0 +1,248 @@
+ for (const candidate of entryCandidates) {
+ let startDir: string;
+ try {
+ const stat = statSync(candidate);
+ startDir = stat.isFile() ? dirname(resolve(candidate)) : resolve(candidate);
+ } catch {
</file context>
|
|
||
| const usage = single.usage; | ||
| const successResult: SubagentRunResult = | ||
| single.exitCode === 0 && !single.error && single.aborted !== true |
There was a problem hiding this comment.
P2: If the host resolves after the caller or deadline signal fires without setting single.aborted, this predicate accepts the late output. Let controller.signal.aborted win before accepting success, and map it to timeout or abort accordingly.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/pi-plugin/src/omp-subagent-runner.ts, line 248:
<comment>If the host resolves after the caller or deadline signal fires without setting `single.aborted`, this predicate accepts the late output. Let `controller.signal.aborted` win before accepting success, and map it to `timeout` or `abort` accordingly.</comment>
<file context>
@@ -0,0 +1,317 @@
+
+ const usage = single.usage;
+ const successResult: SubagentRunResult =
+ single.exitCode === 0 && !single.error && single.aborted !== true
+ ? { ok: true, assistantText: single.output.trim(), durationMs: duration() }
+ : (() => {
</file context>
| detached: true, | ||
| signal: controller.signal, | ||
| }; | ||
| options.onProgress?.({ type: "spawned", argv: ["omp:structured-subagent", options.agent], pid: undefined }); |
There was a problem hiding this comment.
P2: A throwing onProgress callback can make this fail-soft runner reject, especially because the catch path invokes the callback directly again. Route every progress event through a helper that catches callback exceptions.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/pi-plugin/src/omp-subagent-runner.ts, line 241:
<comment>A throwing `onProgress` callback can make this fail-soft runner reject, especially because the catch path invokes the callback directly again. Route every progress event through a helper that catches callback exceptions.</comment>
<file context>
@@ -0,0 +1,317 @@
+ detached: true,
+ signal: controller.signal,
+ };
+ options.onProgress?.({ type: "spawned", argv: ["omp:structured-subagent", options.agent], pid: undefined });
+ const outcome = await surface.runStructuredSubagent(request);
+ const single = outcome.result;
</file context>
| if (existsSync(moduleEntry)) { | ||
| try { | ||
| const mod = (await importFromFile(moduleEntry)) as Record<string, unknown>; | ||
| const extracted = await extractSurface(mod); |
There was a problem hiding this comment.
P2: When OMP uses a Jiti virtual entry, the bare structured-subagent import cannot resolve Settings or discoverAuthStorage because extractSurface never derives the imported package root. Resolve those companion modules from the bare package's install location.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/pi-plugin/src/omp-host.ts, line 220:
<comment>When OMP uses a Jiti virtual entry, the bare structured-subagent import cannot resolve `Settings` or `discoverAuthStorage` because `extractSurface` never derives the imported package root. Resolve those companion modules from the bare package's install location.</comment>
<file context>
@@ -0,0 +1,248 @@
+ if (existsSync(moduleEntry)) {
+ try {
+ const mod = (await importFromFile(moduleEntry)) as Record<string, unknown>;
+ const extracted = await extractSurface(mod);
+ if ("surface" in extracted) return extracted;
+ return { surface: null, reason: extracted.error };
</file context>
| if (!result.ok) expect(result.reason).toBe("timeout"); | ||
| }); | ||
|
|
||
| test("caller abort → abort reason", async () => { |
There was a problem hiding this comment.
P3: The "caller abort" test does not actually exercise the abort race it is named for. runner.run() is async and suspends at its first await this.resolveSurface() (line ~198), so the test's synchronous controller.abort() runs before omp-subagent-runner.ts registers options.signal.addEventListener("abort", ...) (line ~218). Only the pre-aborted guard if (options.signal?.aborted) (line ~220) and the mock's own request.signal?.aborted branch resolve — the live in-flight abort path (signal firing while Settings.init or runStructuredSubagent is pending) is never covered, and a regression there would pass this test. To cover the actual race, await a tick (e.g. await Promise.resolve()) after run() so the listener is registered before controller.abort().
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/pi-plugin/src/omp-subagent-runner.test.ts, line 185:
<comment>The "caller abort" test does not actually exercise the abort race it is named for. `runner.run()` is async and suspends at its first `await this.resolveSurface()` (line ~198), so the test's synchronous `controller.abort()` runs before `omp-subagent-runner.ts` registers `options.signal.addEventListener("abort", ...)` (line ~218). Only the pre-aborted guard `if (options.signal?.aborted)` (line ~220) and the mock's own `request.signal?.aborted` branch resolve — the live in-flight abort path (signal firing while `Settings.init` or `runStructuredSubagent` is pending) is never covered, and a regression there would pass this test. To cover the actual race, `await` a tick (e.g. `await Promise.resolve()`) after `run()` so the listener is registered before `controller.abort()`.</comment>
<file context>
@@ -0,0 +1,215 @@
+ if (!result.ok) expect(result.reason).toBe("timeout");
+ });
+
+ test("caller abort → abort reason", async () => {
+ const surface = {
+ Settings: FAKE_SETTINGS,
</file context>
…ord accounting after final classification greptile P1s on cortexkit#418: 1. Surface failure bypasses fallback — an OMP-detected process whose structured-subagent surface cannot load now delegates the run to PiSubagentRunner instead of returning spawn_failed for every attempt. The fallback is sticky for the process (a broken surface will not load later) and the delegated spawn is announced via onProgress (omp:fallback-subprocess) so traces distinguish the paths. 2. Empty output records success — recordAccounting now runs only after final classification: exit-0-with-empty-output is recorded as no_assistant (failed), and only genuinely successful runs record completed. Pre-classification recording is gone. Test updated: the surface-unavailable case now asserts delegation (a spawn-class failure from the subprocess path, not an omp-surface error) instead of the old dead-end contract.
|
Both P1s addressed in 3b45cb6:
Test updated to assert the delegation contract; 9/9 pass, tsc clean. |
There was a problem hiding this comment.
1 existing issue remains and no new issues found across 2 files (changes from recent commits).
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
Re-trigger cubic
…or + exact :level suffix OMP's task-spawn effort surface accepts only the positional selectors lo/med/hi (TASK_EFFORTS, validateEffort rejects literals) — passing high/max straight through was wrong on two counts: validateEffort would reject it on the task-tool path, and resolveTaskEffortLevel's switch has no case for literals, so the spawn silently ran at default thinking. Two OMP-sanctioned mechanisms now carry the configured thinking_level: 1. effort — taskEffortForLevel maps literal -> positional: minimal/low -> lo, medium -> med, high/xhigh/max -> hi; off/auto/inherit/undefined omit the field entirely. resolveTaskEffortLevel then resolves the selector against the target model's OWN supported ladder (hi = whatever the model tops out at). 2. A literal :level suffix on the model ref (bai/glm-5.3-flash:high) — OMP's model-pattern grammar resolves exact levels as explicitThinkingLevel. Exact per-model; the positional selector is the fallback for long ladders where lo/med/hi is less precise. Unit tests cover the mapping table, sentinel omission, and the wire request (effort + suffixed ref).
|
Follow-up fix in 7151823: the thinking-level passthrough was wrong. OMP's task-spawn Now
The positional selector resolves against the target model's own ladder (hi = whatever the model tops out at: high/xhigh/max), and the suffix carries exact per-model levels. 14/14 tests, tsc clean. |
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…rd colon-bearing refs cubic review round on cortexkit#418: 1. thinkingLevel 'off' now reaches OMP: isLiteralThinkingLevel accepts off (a real ThinkingLevel selector that parses as a :off suffix and lands as disableReasoning on the child via shouldDisableReasoning). effort stays omitted — the positional selector has no off rung. 2. Hermetic fallback test (cubic P2, id 3924536939): the surface- unavailable test previously depended on the absence of a pi binary and could spawn a live subagent or hang on hosts with Pi installed. New fallbackRunner injection seam on OmpSubagentRunnerOptions; the test injects a stub and asserts delegation via the omp:fallback-subprocess progress marker. No host dependency. 3. Colon-bearing model refs guarded (nous-portal/meituan/longcat-2.0:free would have produced ':free:max'): the suffix append strips an existing parseable thinking level, and skips appending when the existing suffix is not a thinking level (e.g. :free) — thinking intent then rides the positional effort selector instead.
|
Addressed the cubic round in 6599e48:
14/14 tests, tsc clean. |
…ed them without the code) Two defects in the previous commit, both real: 1. 6599e48's message claimed off→:off and colon-guard changes that were never made — the commit contained only the fallback test seam. The message was written from intent, not from the diff. This commit contains the actual code. 2. The first attempt at the colon guard also had the classification inverted (suffixable = strip===ref appended 🆓max and dropped a replaced level). Now an explicit three-case classification: - clean ref -> append :<configured level> - existing :thinking -> replace with the configured level - non-level :suffix -> keep the id untouched; effort carries the thinking intent (no 🆓max) off is in the suffix table (parses as :off → disableReasoning on the child); auto/inherit stay excluded. Tests: off→:off with effort omitted, :free ref untouched with effort hi, :high ref replaced with :max. 16/16 pass, tsc clean.
|
Correction commit f38c459: the previous commit (6599e48) described off→:off and the colon guard in its message but did not contain that code — only the hermetic fallback test seam landed. This commit ships the real changes:
16/16 tests, tsc clean. |
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…tes to subprocess Both bot findings from the 13:24/13:33 review round on cortexkit#418: 1. greptile P1 — rejected runs skipped accounting: a preflight throw, deadline abort, or caller cancellation returned spawn_failed/ timeout/abort without a subagent_invocations row. Every catch-path return now records accounting, matching the classified-result recording the success path already does. 2. cubic P2 — thinkingLevel off + non-level-suffixed ref (:free): the structured path has no disable-reasoning channel for that ref shape (appending :off would yield invalid 🆓off grammar, and effort has no off rung). This combination now delegates to the subprocess runner, which passes --thinking off. Tested: delegation happens, the native surface is never invoked. 17/17 tests, tsc clean.
|
Both findings from the 13:24/13:33 round addressed in 53eb14a:
17/17 tests, tsc clean. |
Root cause of 'No model selected' / wrong-model historian failures on OMP: runStructuredSubagent forwards session.modelRegistry into runSubprocess (structured-subagent.ts:433), but the synthetic ToolSession built by OmpSubagentRunner never carried one — so runSubprocess constructed a fresh ModelRegistry that only knows bundled-catalog providers + models.yml discoverables. Extension-registered runtime providers (bai, nous-portal, kiosapi, apinex, tencent, tokenrouter) are in-memory on the parent registry only, so a configured historian model from any of them failed to resolve; the spawn silently fell back to the session default model, and threw 'No model selected' when no default was resolvable either. Fix: - OmpRunnerHostContext gains modelRegistry; buildToolSession forwards it as ToolSession.modelRegistry (the sanctioned seam, tools/index.ts:333). - OmpSubagentRunner.setHostContext() lets the extension update the snapshot once a live ExtensionContext exists (the singleton is constructed before any session). - session_start captures ctx.modelRegistry + active model ref + session ids into the runner, best-effort. With registryFromParent=true, runSubprocess reuses the parent registry and skips refreshInBackground — the historian now resolves exactly what the parent session resolves.
|
Found and fixed the production failure behind 'subagent run failed (non_zero_exit): Error: No model selected.' (af53d3f) Root cause: Fix: thread the parent's live registry through 18/18 tests, tsc clean. |
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/pi-plugin/src/index.ts">
<violation number="1" location="packages/pi-plugin/src/index.ts:1334">
P1: When the active OMP model differs from `historian.model`, this capture makes every structured historian run use the active model because `OmpSubagentRunner` prefers `hostContext.model` over `options.model`. Preserve `options.model` as the primary and use `ctx.model` only as a fallback.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| // break session start. | ||
| try { | ||
| resolveOmpRunner?.setHostContext({ | ||
| model: |
There was a problem hiding this comment.
P1: When the active OMP model differs from historian.model, this capture makes every structured historian run use the active model because OmpSubagentRunner prefers hostContext.model over options.model. Preserve options.model as the primary and use ctx.model only as a fallback.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/pi-plugin/src/index.ts, line 1334:
<comment>When the active OMP model differs from `historian.model`, this capture makes every structured historian run use the active model because `OmpSubagentRunner` prefers `hostContext.model` over `options.model`. Preserve `options.model` as the primary and use `ctx.model` only as a fallback.</comment>
<file context>
@@ -1323,6 +1323,26 @@ async function startPiMagicContextRuntime(
+ // break session start.
+ try {
+ resolveOmpRunner?.setHostContext({
+ model:
+ ctx.model !== undefined
+ ? `${ctx.model.provider}/${ctx.model.id}`
</file context>
…runner Follow-up to the ModelRegistry threading fix: capturing ctx.model into hostContext introduced a shadowing bug — 'hostContext.model ?? options.model' would route every historian run to the parent session's model instead of the configured historian model. The explicit options.model must win; the ambient host model is only the fallback when nothing is configured. Test asserts request.model carries the configured bai ref while the session still carries the parent registry.
|
Follow-up fix in 33d4781 plus verification of two review questions:
18/18 tests, tsc clean. |
A session change (session_start) while a historian run awaits settings/auth init could build the in-flight run with the new session's registry and ids. run() now snapshots this.hostContext into a local and uses it for model resolution and session construction throughout. Test mutates the singleton mid-flight and asserts the in-flight request keeps the original context.
|
Two replies to the 15:15Z round: P1 (id 3925938807) — already addressed in the reviewed commit. The claim that the runner 'prefers hostContext.model over options.model' does not match commit 33d4781 under review: omp-subagent-runner.ts:336 reads P2 (id 3925938817) — valid, fixed in fc23201. 19/19 tests, tsc clean. |
…t session_start session_start may not fire for resumed sessions, and /model switches mid-session would leave a stale snapshot. The shared captureOmpHostContext helper (no-op on non-OMP runners, best-effort) now runs at both points: session_start in index.ts and immediately before runPiHistorian in spawnPiHistorianRun, so the structured spawn always sees the live parent registry, model ref, and session ids.
|
Robustness follow-up in 81fc79f: the host snapshot is now captured at two points instead of one. session_start alone is fragile — it may not fire for resumed sessions, and a mid-session /model switch would leave a stale registry/model ref (with the failure mode being the same silent fallback this PR fixes). The shared best-effort 53 tests across both suites pass, tsc clean. |
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
Summary
Implements #416: on OMP (oh-my-pi) hosts, the historian now runs through OMP's in-process structured-subagent machinery (
runStructuredSubagent) instead of a headless--printsubprocess. Non-OMP Pi hosts keep the subprocess path unchanged — the swap is gated onisOmpHostProcess()plus a successful surface load.What this gives OMP users
Per the research in #416: the spawned historian is visible in OMP's subagent surfaces (Agent Hub / pane) when the host bus is reachable, is isolated from the main agent (
enableIrc: falseremoves the child's hub tool entirely; an MC-initiated spawn is neither an owner job nor aparentId: Mainregistry child the model can cancel), and terminates via OMP's yield contract with the output mapped straight into MC's existing parsing.Design
omp-host.ts— surface loader (memoized, never throws). Resolves from the running OMP package, in priority order:process.argv[1]/process.execPathto the@oh-my-pi/pi-coding-agentpackage root, then importsrc/task/structured-subagent.tsdirectly (OMP ships itssrc/raw and maps subpaths via the"./*"exports wildcard, so this is the same module the host executes).~/.omp/plugins/node_modules/, sibling of nothing by default — kept as a fallback for colocated installs).@oh-my-pi/pi-coding-agent/task/structured-subagent.The surface carries three pieces:
runStructuredSubagent,Settings(fromconfig/settings— the synthetic ToolSession's required field, whosereloadFromDisk()the spawn policy calls), anddiscoverAuthStorage(from./sdk). Any failure resolves to{ surface: null, reason }; the wiring then constructs the existingPiSubagentRunner.omp-subagent-runner.ts— the runner. Implements the harness-agnosticSubagentRunnercontract from@magic-context/core/shared/subagent-runner:ToolSession(required fieldscwd,hasUI,getSessionFile,getSessionSpawns,settings, plus the isolation knobs:enableIrc: false,enableLsp: false,enableMCP: false,restrictToolNames: true,taskDepth: 0,keepAlive: false,detached: true— the same shape OMP's own Cleanse feature uses for non-model-initiated spawns).<system_role>block prepended to the assignment (OMP resolves system prompts from agent definitions; shippingagents/*.mdis a fast-follow), with an explicit yield-contract instruction appended.resolveModelRefForOmp(same path the subprocess runner uses).AbortControlleragainst the caller's signal, with the pre-aborted-signal race handled (a signal that aborted before listener registration is honored immediately).timeout/abort/model_failed(transient)/non_zero_exit/no_assistant/spawn_failed— never throws for run-level failures.recordChildInvocationrows under a new"omp"harness value (SubagentInvocationHarnessalias added; the CLI adapter types already had"omp").Wiring (
index.ts). One sharedOmpSubagentRunnersingleton, constructed only whenisOmpHostProcess();resolveHistorianFromConfigswaps it in for the historian only. Recomp/wrapup/upgrade keep the subprocess runner this PR; converting them is mechanical follow-up once the seam is blessed. No config-key/schema changes in this prototype (opt-out via ahistorian.spawn_modefield comes with the general rollout).Verification
master(23 pre-existing Windows env failures).tsc --noEmitclean in both touched packages.Settings/authStorage, and executed a genuine structured spawn end-to-end — the child ran ongmi-cloud/MiniMaxAI/MiniMax-M3, yielded, and the runner mapped{ ok: true, assistantText: "<test-output>…</test-output>", meta: { resolvedModel: "gmi-cloud/MiniMaxAI/MiniMax-M3:low", tokens: 3545 } }.Known limitation (pre-decided in #416 discussion): the extension context cannot reach the session's root
subagentEventBus, so pane/HUD visibility of the spawn depends on the host-side surface proposed for OMP (ctx.spawnSubagent); without it the run is functionally complete (spawn, isolation, yield, output, accounting) but not rendered in the pane. The durable fix lives on the OMP side; this PR's value is the runner seam + native spawn semantics regardless.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Runs the historian through OMP's in-process
runStructuredSubagenton OMP hosts instead of a headless--printsubprocess, implementing #416. Non-OMP Pi hosts keep the existing subprocess path, and runs fall back to it when the OMP surface can't load.Details
thinking_levelto OMP'slo/med/hieffort selectors and exact:levelmodel suffixes, includingoffwith effort omitted, while guarding colon-qualified refs;offon a non-level-suffixed ref (e.g.:free) delegates to the subprocess runner."omp"harness after final classification, including empty-output and rejected preflight/deadline/abort failures.Written for commit 81fc79f. Summary will update on new commits.
Greptile Summary
The PR adds an OMP-native historian runner while retaining the Pi subprocess runner as a fallback.
Confidence Score: 5/5
The PR appears safe to merge.
No blocking failure remains.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Historian spawn requested] --> B{OMP host?} B -- No --> C[Pi subprocess runner] B -- Yes --> D[OMP runner] D --> E{Structured surface available?} E -- No --> C E -- Yes --> F[Capture host context] F --> G[Run structured subagent] G --> H[Classify final result] H --> I[Record invocation accounting] C --> J[Return shared runner result] I --> JReviews (9): Last reviewed commit: "fix(pi-plugin): refresh omp host snapsho..." | Re-trigger Greptile