feat(oh-my-pi): Cotal connector — headless peer + interactive extension - #5
Merged
Conversation
…ctive extension Native-embed peer (runOmpPeer/connector, on the shared InboxTurn loop) for a manager-spawned worker, plus a `pi --extension` that joins a human/Compass-launched session to the mesh (cotal_* tools, presence, sendMessage delivery). Mirrors the pi and opencode connectors; renders the shared cotalToolSpecs so the surface can't drift. Co-Authored-By: seal <noreply@sealedsecurity.com>
bin/cotal.ts imports @cotal-ai/delivery (added with the Plane-3 delivery daemon) but it was never in the root package.json deps, so a clean checkout can't run `cotal` (or `pnpm cotal`) — ERR_MODULE_NOT_FOUND on @cotal-ai/delivery. Co-Authored-By: seal <noreply@sealedsecurity.com>
A task/print/RPC subagent inherits the parent's COTAL_* env, so gating on hasIdentity() alone made every subagent a stray same-named mesh peer (roster pollution + ambiguous DMs, and a subagent could receive traffic meant for the main session). Defer the mesh-join to session_start and start only when ctx.hasUI is true; non-interactive sessions stay off the mesh. Smoke covers both branches. Co-Authored-By: seal <noreply@sealedsecurity.com>
…the host TUI A mid-session mesh drop made MeshAgent log every endpoint reconnect-failure (TIMEOUT) to stderr on a fixed 3s retry, which in the in-process oh-my-pi extension flooded and corrupted the live terminal. Log the drop/recover edges once (suppressing the retry churn), inject a logger so oh-my-pi routes through pi.logger (a file, not the shared terminal), and back the endpoint's reconnect retries off exponentially (3s->30s). Co-Authored-By: seal <noreply@sealedsecurity.com>
…uilds standalone loop.ts imports InboxTurn/InboxSource from @cotal-ai/connector-core, but the connector-core half (inbox-turn.ts + the ackInbox source method + the index export) was dropped when this branch was rebased onto upstream/main, leaving a dangling import that fails tsc. Roll those pieces in from the upstream feat/connector-pi work so the branch builds independently of that open PR while staying rebasable on upstream/main. Co-Authored-By: seal <noreply@sealedsecurity.com>
pi-coding-agent 16.3.7 changed the tool-registry typing so the legacy TypeBox
`defineTool`/`Type` shim no longer infers params (execute's `params` fell to
`unknown`; the result narrowed to `ToolDefinition<ArkSchema, {}>` and broke
`customTools` variance), and `registerTool` began recursing into
`Static<TParams>` on inline zod literals (TS2589 excessively-deep).
Move the peer's cotal_roster/cotal_status off the retired shim to zod schemas
(the SDK's canonical param format — `Static` infers `z.infer` first), and pin
`registerTool<ReturnType<typeof z.object>>` in the extension so the registry
generic no longer deep-infers. Drop the `details: {}` literals that narrowed
TDetails. Bump the dep to ^16.3.12 (latest); the whole build + all connector
smokes are green.
Co-Authored-By: seal <noreply@sealedsecurity.com>
📝 WalkthroughWalkthroughAdds inbox-turn ack handling and MeshAgent reconnect/logging changes in connector-core, introduces the oh-my-pi connector package with embedded and interactive entrypoints, and wires an example app plus workspace scripts and build settings. ChangesConnector-core inbox turns, logging, and backoff
connector-oh-my-pi package
Estimated code review effort: 4 (Complex) | ~75 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR adds the oh-my-pi Cotal connector and its shared delivery support. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (8): Last reviewed commit: "fix(connector): clear the fold-settle ti..." | Re-trigger Greptile |
![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.
All reported issues were addressed across 25 files
Heads up: you’re close to your included review allowance. Set a flex budget so reviews don’t pause.
Re-trigger cubic
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
extensions/connector-core/src/agent.ts (1)
91-96: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPreserve log severity in the default logger.
defaultLoggeracceptsMeshLogLevelbut discards it, so out-of-process connectors losewarn/errorvisibility even though the new logger contract carries severity.Proposed fix
-function defaultLogger(msg: string, _level?: MeshLogLevel): void { - process.stderr.write(`[cotal-connector] ${msg}\n`); +function defaultLogger(msg: string, level: MeshLogLevel = "info"): void { + process.stderr.write(`[cotal-connector:${level}] ${msg}\n`); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@extensions/connector-core/src/agent.ts` around lines 91 - 96, The defaultLogger in agent.ts is ignoring the MeshLogLevel argument, so warn/error messages are not distinguishable in out-of-process connectors. Update defaultLogger to use the provided severity when writing to stderr, likely by including the level in the emitted prefix or otherwise preserving it in the log format, while keeping the existing cotal-connector context.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@examples/04-oh-my-pi/src/manager.ts`:
- Line 11: The example currently imports the built package entrypoint from
`@cotal-ai/oh-my-pi`, which assumes extensions/connector-oh-my-pi/dist/index.js
already exists. Update examples/04-oh-my-pi/src/manager.ts (and any related
example setup) to either import the source entrypoint directly or add a
prebuild/prepare step that builds the connector before tsx runs, so the example
works on a clean checkout. Use the manager.ts self-registration import as the
location to adjust.
In `@extensions/connector-core/smoke/reconnect-log.smoke.ts`:
- Around line 32-33: The smoke test logger callback is storing a possibly
omitted log level into `lines`, but `MeshLogLevel` is required. Update the
`MeshAgent` callback used in `reconnect-log.smoke.ts` to default the `level`
argument to `"info"` before pushing into `lines`, so the `msg`/`level` pair
stays type-safe under strict settings.
In `@extensions/connector-oh-my-pi/oh-my-pi-extension.smoke.ts`:
- Around line 80-84: Test 3 only verifies approval on cotal_inbox and never
exercises the peek-forced execute path mentioned in the comment. In
oh-my-pi-extension.smoke.ts, update the cotal_inbox check to call inbox.execute
with an empty object and assert it does not throw, so the test matches the
stated behavior and validates the end-to-end read-only flow using the
cotal_inbox tool reference.
In `@extensions/connector-oh-my-pi/src/loop.ts`:
- Around line 183-191: The shutdown path in the returned object from loop.ts is
fire-and-forget on session disposal, so the async cleanup may not finish before
process exit. Update the async shutdown() method to await session.dispose()
after any abort handling, and keep the existing
turn.inFlight/turn.abandon/session.abort flow intact so the real session cleanup
completes before peer.ts exits.
In `@extensions/connector-oh-my-pi/src/peer.ts`:
- Around line 84-93: The shutdown flow in shutdown() can run concurrently if
SIGINT or SIGTERM arrives more than once before process.exit(0), which risks
double cleanup. Add a simple in-flight boolean guard inside the peer.ts shutdown
path so repeated signals become no-ops after the first invocation. Keep the
guard near the shutdown() function and reuse the existing loop.shutdown() and
mesh.stop() sequence, ensuring the exit still happens only once.
---
Nitpick comments:
In `@extensions/connector-core/src/agent.ts`:
- Around line 91-96: The defaultLogger in agent.ts is ignoring the MeshLogLevel
argument, so warn/error messages are not distinguishable in out-of-process
connectors. Update defaultLogger to use the provided severity when writing to
stderr, likely by including the level in the emitted prefix or otherwise
preserving it in the log format, while keeping the existing cotal-connector
context.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 84b54a26-9563-43c1-b9f8-70d6a12021d8
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (24)
examples/04-oh-my-pi/package.jsonexamples/04-oh-my-pi/src/manager.tsexamples/04-oh-my-pi/tsconfig.jsonextensions/connector-core/inbox-turn.smoke.tsextensions/connector-core/smoke/reconnect-log.smoke.tsextensions/connector-core/src/agent.tsextensions/connector-core/src/inbox-turn.tsextensions/connector-core/src/index.tsextensions/connector-oh-my-pi/README.mdextensions/connector-oh-my-pi/interactive-loop.smoke.tsextensions/connector-oh-my-pi/oh-my-pi-extension.smoke.tsextensions/connector-oh-my-pi/oh-my-pi-peer.smoke.tsextensions/connector-oh-my-pi/package.jsonextensions/connector-oh-my-pi/src/connector.tsextensions/connector-oh-my-pi/src/extension.tsextensions/connector-oh-my-pi/src/index.tsextensions/connector-oh-my-pi/src/interactive-loop.tsextensions/connector-oh-my-pi/src/loop.tsextensions/connector-oh-my-pi/src/main.tsextensions/connector-oh-my-pi/src/peer.tsextensions/connector-oh-my-pi/tsconfig.jsonpackage.jsonpackages/core/src/endpoint.tspnpm-workspace.yaml
Fix two P1 `loop.ts` delivery bugs (a declined `prompt()` no longer wedges the peer; a rejected `steer()` un-surfaces its id via new `InboxTurn.unsurface` so it redelivers instead of being falsely acked), make the connect-retry backoff and peer shutdown cancellable/idempotent, `await session.dispose()`, and wire the three hermetic oh-my-pi smokes into `pnpm -r test`. Adds regression tests 7-8 for the P1s. Co-Authored-By: seal <noreply@sealedsecurity.com>
|
@coderabbitai review Head is now |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
cd /tmp
gh pr view 5 --repo sealedsecurity/Cotal --json headRefOid,headRefName,commits --jq '{head: .headRefOid, branch: .headRefName}'
echo "---"
gh api repos/sealedsecurity/Cotal/pulls/5/commits --jq '.[-5:] | .[] | {sha: .sha[0:7], msg: .commit.message}'Length of output: 3831
✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 1 minute. |
There was a problem hiding this comment.
All reported issues were addressed across 9 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
The review of the prior commit surfaced real races in it: a folded `steer()` that rejects after `agent_end` acked the turn still lost the message; a `prompt()` settling during shutdown drove a disposed session; and awaiting `dispose()` let its failure skip the caller's `mesh.stop()`. Track pending steers and un-surface any unconfirmed at `agent_end`, guard the prompt callback with a `stopped` flag, and `dispose().catch(log)`. Peer smoke gates the async callbacks on a macrotask drain so the assertions aren't hop-count fragile. Co-Authored-By: seal <noreply@sealedsecurity.com>
`smoke:inbox` and `smoke:reconnect-log` (both added in this PR) ran only by hand. Add a package `test` aggregate so `pnpm -r test` in the unit lane executes them, matching the oh-my-pi connector. Co-Authored-By: seal <noreply@sealedsecurity.com>
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Heads up: you’re close to your included review allowance. Set a flex budget so reviews don’t pause.
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
The fix-review of c09b21e found two races the finding-level tests missed. Add regression tests that fail on c09b21e and pass on the race fix: - test 9: a rejecting same-scope fold with agent_end emitted before the rejection settles must not be acked (c09b21e acks it → message lost; the fix un-surfaces pending steers at agent_end before commit). - test 10: a rejecting session.dispose() must not reject loop.shutdown() (c09b21e's bare await rejects → peer.ts skips mesh.stop → ghost peer; the fix awaits dispose().catch(log)). Both proven red on c09b21e (tests 9 line 368, 10 line 400), green on head. Co-Authored-By: seal <noreply@sealedsecurity.com>
…rdown The fix-review of the race fix found three deeper races in agent_end's "un-surface every pending fold before commit". All three are real (the mesh dedups only ACKED ids, so an un-acked redelivery re-surfaces to the model): - Accepted-steer redeliver (greptile): a fold whose steer() RESOLVED but whose .then hadn't flushed when agent_end fired was un-surfaced → not acked → redelivered though the model already received it. Now agent_end defers the commit until each fold's steer settles (Promise.race against a one-macrotask boundary so a never-settling steer can't hang the turn): an accepted fold stays acked, only a rejected or stranded one redelivers. - Cross-turn mutation (cubic P1): the reject callback called turn.unsurface() with no turn identity, so a steer settling after its turn committed could strip an id a LATER turn had re-surfaced. A monotonic generation captured at fold time makes a late settle a no-op on any turn but its own. - Post-shutdown dispatch (cubic P2): pump()/foldSameScope()/the session handler now early-return once stopped, so a mesh event landing mid-teardown can't drive a disposed session. The common one-message turn still commits synchronously (no fold → no defer). Tests (red on the pre-fix 6f82f76, green here): accepted-steer-acked, late-settle-no-ops-later-turn, shutdown-blocks-dispatch, strand-safety-no-hang. Co-Authored-By: seal <noreply@sealedsecurity.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@extensions/connector-oh-my-pi/src/loop.ts`:
- Around line 254-258: `loop.shutdown()` currently lets a rejected
`session.abort()` escape before `session.dispose()`, which can short-circuit
cleanup and block `mesh.stop()`. Update the shutdown path in `loop.ts` so the
`session.abort()` call is handled like `session.dispose()` by catching and
logging its failure, then always continuing to the dispose step and preserving
the existing cleanup flow in `turn.abandon()`, `session.abort()`, and
`session.dispose()`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 45e73905-17da-4724-92a6-5cc0f83d79c8
📒 Files selected for processing (10)
extensions/connector-core/package.jsonextensions/connector-core/smoke/reconnect-log.smoke.tsextensions/connector-core/src/agent.tsextensions/connector-core/src/inbox-turn.tsextensions/connector-oh-my-pi/interactive-loop.smoke.tsextensions/connector-oh-my-pi/oh-my-pi-extension.smoke.tsextensions/connector-oh-my-pi/oh-my-pi-peer.smoke.tsextensions/connector-oh-my-pi/package.jsonextensions/connector-oh-my-pi/src/loop.tsextensions/connector-oh-my-pi/src/peer.ts
✅ Files skipped from review due to trivial changes (1)
- extensions/connector-core/package.json
🚧 Files skipped from review as they are similar to previous changes (6)
- extensions/connector-oh-my-pi/package.json
- extensions/connector-oh-my-pi/oh-my-pi-extension.smoke.ts
- extensions/connector-core/smoke/reconnect-log.smoke.ts
- extensions/connector-oh-my-pi/interactive-loop.smoke.ts
- extensions/connector-core/src/agent.ts
- extensions/connector-oh-my-pi/src/peer.ts
…l runs The sibling of the dispose-teardown fix: shutdown() awaited session.abort() bare, so a rejected abort() rejected shutdown() → skipped dispose() AND the caller's mesh.stop() → ghost peer on the mesh. Handle it like dispose(): await session.abort().catch(log). Regression test (red on the pre-fix commit, green here): a rejecting abort still resolves shutdown() and dispose() still runs (disposed === 1). Co-Authored-By: seal <noreply@sealedsecurity.com>
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Heads up: you’re close to your included review allowance. Set a flex budget so reviews don’t pause.
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
Two review findings on the deferred-commit + shutdown hardening: - Slow-accept redeliver: the deferred commit raced pending fold steers against a setTimeout(0) macrotask boundary. A steer accepting after that 0ms tick was treated as undelivered and redelivered though the model already had it. Replace the 0ms boundary with an injectable steerSettleTimeoutMs (default 5s): a healthy steer settles in <=1 microtask so allSettled wins by orders of magnitude and the timeout never fires on the happy path; it only bounds a genuinely stuck steer so the turn can't wedge. Correct by a real margin, not by microtask-vs-macrotask ordering. - Shutdown teardown coupling: abort() and dispose() now sit in independent try/catch blocks, so a failed OR synchronously-thrown abort() still runs dispose() and still resolves shutdown() — peer.ts's mesh.stop() always runs, no ghost peer. (A single wrapping try/catch would let an abort failure skip dispose.) Tests (red on the pre-fix boundary/shutdown, green here): slow-accept steer is acked under the human-scale timeout; a sync-throwing abort still resolves shutdown and runs dispose. Timeout is injectable so tests don't wait the real delay. Co-Authored-By: seal <noreply@sealedsecurity.com>
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Heads up: you’re close to your included review allowance. Set a flex budget so reviews don’t pause.
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
The human-scale steerSettleTimeoutMs timer added to bound the deferred commit was never cleared on the common path (allSettled wins before it fires). An uncleared setTimeout stays ref'd on the Node event loop, so every folded turn delayed process/CLI exit by up to the timeout — negligible at the old 0ms boundary, but up to 5s per turn at the new human-scale default. Clear it in a finally so the happy path leaves no live handle; the timeout still bounds a genuinely-stuck steer. Test 18 (red on an uncleared-timer loop, green here): after a fold commits via allSettled, the active Timeout-handle count is unchanged (no leak). Test 16 now exercises the production default (5s) rather than an injected 50ms — safe only because the timer is cleared, and it makes the suite exit promptly instead of hanging on a leaked handle. Co-Authored-By: seal <noreply@sealedsecurity.com>
There was a problem hiding this comment.
2 issues found across 2 files (changes from recent commits).
Heads up: you’re close to your included review allowance. Set a flex budget so reviews don’t pause.
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="extensions/connector-oh-my-pi/src/loop.ts">
<violation number="1" location="extensions/connector-oh-my-pi/src/loop.ts:227">
P2: If `agent_end` triggers a deferred commit with pending steers and `shutdown()` is called before the timeout fires, the ref'ed `setTimeout` stays on the Node event loop even after teardown completes. The `finally` block that clears the timer only runs when `Promise.race` resolves, so a mid-shutdown wait is not interrupted. Because `commitAfterSteers` is a floating promise, `shutdown()` does not await it, and the process can be delayed from exiting by up to `steerSettleTimeoutMs` (5s default) per folded turn. Adding `timer.unref()` prevents the timer from keeping the event loop alive during teardown.</violation>
</file>
<file name="extensions/connector-oh-my-pi/oh-my-pi-peer.smoke.ts">
<violation number="1" location="extensions/connector-oh-my-pi/oh-my-pi-peer.smoke.ts:698">
P3: The timer-leak assertion in test 18 compares the process-wide active `Timeout` resource count before and after the scenario to verify the 5_000ms settle timer was cleared. Because `process.getActiveResourcesInfo()` counts ALL active timeouts in the process, the assertion is coupled to global state rather than the specific timer under test. Any unrelated timer created or destroyed by the test runner, ambient process code, dependencies, or a future refactor can produce a spurious failure or, conversely, mask a real leak. For a more deterministic check, consider instrumenting `setTimeout`/`clearTimeout` within the test scope (for example, a local shim around `runPeerLoop`) so the assertion can verify that the exact settle-timer handle returned by `commitAfterSteers` is cleared, rather than relying on the global process count.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
| let timer: ReturnType<typeof setTimeout> | undefined; | ||
| try { | ||
| const timeout = new Promise<void>((resolve) => { | ||
| timer = setTimeout(resolve, steerSettleTimeoutMs); |
There was a problem hiding this comment.
P2: If agent_end triggers a deferred commit with pending steers and shutdown() is called before the timeout fires, the ref'ed setTimeout stays on the Node event loop even after teardown completes. The finally block that clears the timer only runs when Promise.race resolves, so a mid-shutdown wait is not interrupted. Because commitAfterSteers is a floating promise, shutdown() does not await it, and the process can be delayed from exiting by up to steerSettleTimeoutMs (5s default) per folded turn. Adding timer.unref() prevents the timer from keeping the event loop alive during teardown.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At extensions/connector-oh-my-pi/src/loop.ts, line 227:
<comment>If `agent_end` triggers a deferred commit with pending steers and `shutdown()` is called before the timeout fires, the ref'ed `setTimeout` stays on the Node event loop even after teardown completes. The `finally` block that clears the timer only runs when `Promise.race` resolves, so a mid-shutdown wait is not interrupted. Because `commitAfterSteers` is a floating promise, `shutdown()` does not await it, and the process can be delayed from exiting by up to `steerSettleTimeoutMs` (5s default) per folded turn. Adding `timer.unref()` prevents the timer from keeping the event loop alive during teardown.</comment>
<file context>
@@ -218,8 +218,18 @@ export function runPeerLoop({
+ let timer: ReturnType<typeof setTimeout> | undefined;
+ try {
+ const timeout = new Promise<void>((resolve) => {
+ timer = setTimeout(resolve, steerSettleTimeoutMs);
+ });
+ await Promise.race([Promise.allSettled([...pendingSteers.values()]), timeout]);
</file context>
Author
There was a problem hiding this comment.
Context (not resolving): the proper fix for this is specified in the design record at mattwilkinsonn/zireael#280 (docs/designs/agents/connector-deferred-commit-lifecycle.md) — shutdown() tracks and cancels/awaits the floating commitAfterSteers so the settle timer can't keep the event loop alive during teardown. That's the full shutdown-join; timer.unref() here is a narrower band-aid over the same symptom. Implementation follows once that design record freezes on merge, so leaving this open as a tracking reference until then.
| // commit's `finally` (which clears the settle timer) has run before we sample the handle table. | ||
| await new Promise((r) => setTimeout(r, 20)); | ||
| await new Promise((r) => setImmediate(r)); | ||
| const after = process.getActiveResourcesInfo().filter((r) => r === "Timeout").length; |
There was a problem hiding this comment.
P3: The timer-leak assertion in test 18 compares the process-wide active Timeout resource count before and after the scenario to verify the 5_000ms settle timer was cleared. Because process.getActiveResourcesInfo() counts ALL active timeouts in the process, the assertion is coupled to global state rather than the specific timer under test. Any unrelated timer created or destroyed by the test runner, ambient process code, dependencies, or a future refactor can produce a spurious failure or, conversely, mask a real leak. For a more deterministic check, consider instrumenting setTimeout/clearTimeout within the test scope (for example, a local shim around runPeerLoop) so the assertion can verify that the exact settle-timer handle returned by commitAfterSteers is cleared, rather than relying on the global process count.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At extensions/connector-oh-my-pi/oh-my-pi-peer.smoke.ts, line 698:
<comment>The timer-leak assertion in test 18 compares the process-wide active `Timeout` resource count before and after the scenario to verify the 5_000ms settle timer was cleared. Because `process.getActiveResourcesInfo()` counts ALL active timeouts in the process, the assertion is coupled to global state rather than the specific timer under test. Any unrelated timer created or destroyed by the test runner, ambient process code, dependencies, or a future refactor can produce a spurious failure or, conversely, mask a real leak. For a more deterministic check, consider instrumenting `setTimeout`/`clearTimeout` within the test scope (for example, a local shim around `runPeerLoop`) so the assertion can verify that the exact settle-timer handle returned by `commitAfterSteers` is cleared, rather than relying on the global process count.</comment>
<file context>
@@ -668,4 +670,38 @@ console.log("16) slow-accept steer is still acked under a human-scale timeout (#
+ // commit's `finally` (which clears the settle timer) has run before we sample the handle table.
+ await new Promise((r) => setTimeout(r, 20));
+ await new Promise((r) => setImmediate(r));
+ const after = process.getActiveResourcesInfo().filter((r) => r === "Timeout").length;
+ assert(after === before,
+ `the 5_000ms settle timer was CLEARED after allSettled won — no leaked Timeout handle ` +
</file context>
Author
There was a problem hiding this comment.
Context (not resolving): this test-oracle coupling is addressed in the design record at mattwilkinsonn/zireael#280 (docs/designs/agents/connector-deferred-commit-lifecycle.md) — it specifies a setTimeout/clearTimeout shim scoped to the test (a local ledger around runPeerLoop tracking armed/cleared/fired handles) in place of the process-wide getActiveResourcesInfo() count, so the assertion checks the exact settle-timer handle rather than global state. Implementation follows on that freeze; leaving this open as a tracking reference until then.
This was referenced Jul 10, 2026
seal-agent
added a commit
that referenced
this pull request
Jul 11, 2026
Adversarial read-only critic pass (SEA-1188) before freeze. Five findings folded into the body; two code-false core claims surfaced as load-bearing Open Questions for Matt's call before freeze. Forks (now OQ#4/#5, blocking freeze): - The inbound cards do NOT inherit OMP's frame. renderFramedMessage mounts a returned MessageRenderer Component unframed; the outlined card is built only on the undefined-return fallback. Record's "mirror CustomMessageComponent frame" was wrong. OQ#4: return undefined + pre-format content (OMP frames it) vs hand-build a frame (couples to unexported internal theme keys). - ircToolRenderer is not a drop-in template. Its inline/mergeCallAndResult live on OMP's internal ToolRenderer; the extension ToolDefinition exposes only renderCall/renderResult. OQ#5: accept two rows vs hand-build a merged card. Folded improvements: - Discriminate on message.customType (authoritative by dispatch), drop the redundant kind field from CotalInjectionDetails and the call site. - Thread sendMessage<CotalInjectionDetails> so details is type-checked, not erased to unknown at the pi boundary. - Re-anchor every OMP cite to the installed 16.3.12 tree (record cited stale 16.3.4/16.3.15 coords; dep resolves 16.3.12). - Sharpen the Problem: formatInjection already newline-joins bullets; the defect is default-Markdown reflow with no renderer, not a literal paragraph. Co-Authored-By: seal <noreply@sealedsecurity.com>
seal-agent
added a commit
that referenced
this pull request
Jul 26, 2026
Adversarial read-only critic pass (SEA-1188) before freeze. Five findings folded into the body; two code-false core claims surfaced as load-bearing Open Questions for Matt's call before freeze. Forks (now OQ#4/#5, blocking freeze): - The inbound cards do NOT inherit OMP's frame. renderFramedMessage mounts a returned MessageRenderer Component unframed; the outlined card is built only on the undefined-return fallback. Record's "mirror CustomMessageComponent frame" was wrong. OQ#4: return undefined + pre-format content (OMP frames it) vs hand-build a frame (couples to unexported internal theme keys). - ircToolRenderer is not a drop-in template. Its inline/mergeCallAndResult live on OMP's internal ToolRenderer; the extension ToolDefinition exposes only renderCall/renderResult. OQ#5: accept two rows vs hand-build a merged card. Folded improvements: - Discriminate on message.customType (authoritative by dispatch), drop the redundant kind field from CotalInjectionDetails and the call site. - Thread sendMessage<CotalInjectionDetails> so details is type-checked, not erased to unknown at the pi boundary. - Re-anchor every OMP cite to the installed 16.3.12 tree (record cited stale 16.3.4/16.3.15 coords; dep resolves 16.3.12). - Sharpen the Problem: formatInjection already newline-joins bullets; the defect is default-Markdown reflow with no renderer, not a literal paragraph. Co-Authored-By: seal <noreply@sealedsecurity.com>
seal-agent
added a commit
that referenced
this pull request
Aug 7, 2026
Fix two P1 `loop.ts` delivery bugs (a declined `prompt()` no longer wedges the peer; a rejected `steer()` un-surfaces its id via new `InboxTurn.unsurface` so it redelivers instead of being falsely acked), make the connect-retry backoff and peer shutdown cancellable/idempotent, `await session.dispose()`, and wire the three hermetic oh-my-pi smokes into `pnpm -r test`. Adds regression tests 7-8 for the P1s. Co-Authored-By: seal <noreply@sealedsecurity.com>
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
3 participants
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.
What
The oh-my-pi Cotal connector — makes an OMP session a first-class Cotal mesh peer, at parity with the Claude Code (MCP) and OpenCode connectors. Two halves:
connector-oh-my-pi/src/peer.ts) — holds theMeshAgent(NATS endpoint, inbox, presence) for the session lifetime; delivers inbound mesh traffic into the session viapi.sendMessage(..., { deliverAs })(idle → woken, live → steered, never interrupting a turn).connector-oh-my-pi/src/extension.ts) — thecotal_*tools (roster, send, dm, anycast, status, …), presence, and message delivery, joined only from a real interactive session.Plus the shared
connector-core(agent.ts,inbox-turn.tsack-on-surface) it builds on.Base
Targets
sealed-fork(a bare mirror ofupstream/main), so the diff is exactly the connector work.sealed-forkis our integration/deploy branch — once this and the zellij PR (#2) both land,sealed-forkcarries both features and nix builds it. The connector never lands onsealed-forkbefore review; this PR is the review.Commits
feat(oh-my-pi): Cotal connector for oh-my-pi — headless peer + interactive extensionfix(cli): declare @cotal-ai/delivery so bin/cotal.ts resolvesfix(oh-my-pi): only join the mesh from an interactive sessionfix(connector): stop mesh reconnect churn from flooding + corrupting the host TUIfix(connector-core): roll in InboxTurn ack-on-surface so the branch builds standalonefeat(oh-my-pi): track pi-coding-agent 16.3.12, migrate tools to zodfix(connector): address #5 review findings — delivery, shutdown, CISDK migration
pi-coding-agent 16.3.7 retired the TypeBox
defineTool/Type.Objectshim; the tools now use zod schemas (the canonicalStatic/TSchemapath).extension.tspinsregisterTool<ReturnType<typeof z.object>>to stop the tool-registry generic recursing intoStatic(TS2589) on an emptyz.object({}). Dep bumped to^16.3.12.Review-fix commit (
c09b21e)Addresses the CodeRabbit / cubic / Greptile findings on this PR:
loop.ts— a declinedprompt()(resolvesfalse) no longer wedges the peer: it completes the turn (ack + idle + pump) like a pre-flight failure. A rejectedsteer()now un-surfaces its id (newInboxTurn.unsurface) so the terminalcommit()can't falsely ack an undelivered fold — it redelivers.agent.ts— the connect-retry backoff is cancellable;stop()interrupts it instead of blocking shutdown up to 30s on an unreachable mesh.peer.ts— a double-shutdown guard (a second SIGINT during teardown won't re-abort/-dispose/-stop);await session.dispose()inloop.tsshutdown.pnpm -r test(unit lane), via a packagetestaggregate. Trivials: logger level preserved; smoke doc/exec fixes.Two cubic findings on
examples/04were declined (they mirror the endorsed example-01 composition-root pattern exactly). One P2 (presence dropped while disconnected) is a cross-connector public-API design fork, surfaced to Matt rather than auto-fixed.Verification
pnpm buildgreen; connector bundle (extension.bundle.js) + all workspace packages build.inbox-turn,reconnect-log,oh-my-pi-peer(8/8, incl. the 2 new P1 regressions),oh-my-pi-extension,interactive-loop.upstream/main; standalone-buildable.Sequencing
Merges into
sealed-fork. Matt holds the nix-switch until both this and #2 land, so live mesh sessions (on the current dist) are unaffected until then. A session-title fix (pi.setSessionName(COTAL_NAME)atextension.tssession_start) is stacked on this branch as #6.Refs #5
Co-Authored-By: seal noreply@sealedsecurity.com
Review-swarm findings (advisory — mandate #618, parked for Matt)
OMP-native review-swarm ran all 7 lenses over this diff (parallel to the SaaS bots; advisory, never gates the merge). Floor: 4 high gradings / 3 distinct issues, ~9 medium, ~13 low. Nothing pushed overnight — a commit would dismiss the current approvals and re-fire CI while unmergeable; all fixes bundle into ONE revision pass after the rulings below.
Judgment calls (yours)
[HIGH — 3-lens: correctness + concurrency + security] MAX_INBOX front-eviction silently drops UNHANDLED directed messages (
agent.ts:330-334; reachable viainteractive-loop.ts:110-115+loop.tsbuffering).ingest()pushes every buffered message (dm/anycast/@mention/ambient) into one FIFO, then on overflow splices the front +ack()s indiscriminate of kind. During a long turn delivery is held, so newer messages queue behind the surfaced prefix. Trigger:openattention, busy channel, mid-turn — a DM arrives (buffered, unsurfaced), then 200+ ambient messages flood in → the DM is at the front → spliced + acked. It was never surfaced, sohandledIdsnever recorded it, so the ack tells JetStream to stop redelivering a message the model never saw → permanently lost (not redelivered). The PR's own ack-on-surface design ("InboxTurn tolerates front-eviction because evicted ids were already handled") rests on an assumption that is FALSE for a directed message evicted before its turn ran. Security lens grades it a targeted-suppression vector (flood to drop an approval DM); concurrency adds broken abandon-redelivery. Scope nuance: the offending line is pre-existing (commitba94085, in main) but this PR promotes it to load-bearing and wires the contract that assumes it safe → fix-here vs follow-up is your call. Fix: never evict an unhandled directed message (split the cap: evict ambient first, keep dm/anycast/@mention until surfaced+acked; or nak/leave-on-stream instead of ack when force-evicting an id not inhandledIds).[HIGH — api-surface]
buildLaunchnever forwards the resolved access policy (connector.ts:41-53) — zheng-verified against the code. The other three connectors splice...aclEnv(opts)(COTAL_SUBSCRIBE / ALLOW_SUBSCRIBE / ALLOW_PUBLISH / CAPABILITIES); oh-my-pi emits none. The manager mints creds from exactly that set then passes it tobuildLaunch(manager.ts:849-872); the runtime passes onlyspec.env(pty.ts:44-48confirms "never...process.env"). Result: (a) subscribe is lost →configFromEnvfalls back to["general"]which scoped creds deny → agent joins nothing; (b) a spawn-capable agent never gets COTAL_CAPABILITIES →cotalToolSpecshidescotal_spawn/cotal_persona. Fix: splice...aclEnv(opts).[HIGH — api-surface]
buildLaunchomits the entire OS env allow-list (connector.ts:41-53) — zheng-verified. Builds env from scratch (COTAL_* + a private 9-key provider list), omittinglaunchEnv()'s PATH/HOME/USER/SHELL/TERM/LANG/TMPDIR/XDG_* (+ Windows mandatory SystemRoot/windir). A manager-spawned child runs with no PATH/HOME/TERM → Windows child aborts at startup; POSIX breaks oh-my-pi's env-based auth + model-registry discovery (~/.omp) and any shell-out. Also forks its own provider-key list vs coreMODEL_PROVIDER_KEYS. Fix: build fromlaunchEnv({providerKeys: MODEL_PROVIDER_KEYS}), overlay COTAL_*.H2+H3 blast radius (both): they bite the manager/CLI-spawned path. The fleet currently hand-launches OMP in zellij panes (SEA-1227 wiring not landed), so this is latent today — but this PR's whole purpose is that wired path. Current hand-launch usage is unaffected; recommend fixing before the SEA-1227 wiring goes live. Gate-vs-follow-up is your call.
[design] Should both
PeerLoop/PeerMeshflavors exist, and should the ack-on-surface invariant flow through one abstraction? The PR extractsInboxTurnto core (canonical, id-based ack) and wires the headlessloop.tsthrough it — butinteractive-loop.tshand-rolls the same invariant and diverges on mechanism (position-based ack). This directly caused the medium correctness bug below. Design owns the should-both-exist judgment; the rename +ackInboxfix are mechanical once you decide.Mechanical / decision-neutral (bundled into the post-ruling pass, not pushed now)
loop.ts:203-210):finishTurnacks the origin BEFORE the fire-and-forgetdeliver(). A mesh drop acrossagent_end→ reply silently dropped, trigger consumed, redelivery hitshandledIds→ never re-answered. Fix: await deliver success before commit; on failureabandon()notcommit().interactive-loopacks by front POSITION, not id (interactive-loop.ts:97-104) → duplicate reply on a busy channel once a predecessor is front-evicted. This IS the drift the design finding (feat(cli): cotal provision-acl + spawn provision the full durable-delivery footprint #4) predicted. Fix:mesh.ackInbox(surfaced)(the id-based primitiveloop.tsalready uses).buildLaunchsilently dropsopts.model—spawn --agent oh-my-pi --model <m>is ignored with no fail-loud (unlikevariant). Fix: render COTAL_MODEL + apply, or throw.buildLaunchomits fail-loud backstops the contract mandates —resume(CLI foreground spawns fresh silently) +mcpServers(silently dropped). Fix: guard the top ofbuildLaunchmirroring hermes.connectLoopbackoff/cap/stop-interrupt untested;reconnect()(thecotal_reconnecttool) untested;buildLaunchenv assembly untested — decision-neutral additions (now vs follow-up is a scope call).runHeadlessPeerLoop/HeadlessPeerMesh) so the two flavors are distinguishable and the barrel stays collision-free.retryMs=3000literal, unusedlogexport, transcript drop, etc. — all bundle.Disposition: hold all edits; fold the mechanical set + your rulings on (1)–(4) into one revision pass → approvals dismiss + CI re-fires exactly once. Full detail: session
local/review/pr5-aggregation.md+findings-ledger.md.