feat(compass-agent): migrate off the vendored oh-my-pi subtree onto @oh-my-pi/* 18.0.11 (RIG-2407) - #924
Open
rigel-mintaka wants to merge 3 commits into
Open
feat(compass-agent): migrate off the vendored oh-my-pi subtree onto @oh-my-pi/* 18.0.11 (RIG-2407)#924rigel-mintaka wants to merge 3 commits into
rigel-mintaka wants to merge 3 commits into
Conversation
…oh-my-pi/* 18.0.11 (RIG-2407)
Repoints `compass-agent` from the vendored `forks/oh-my-pi/` subtree onto the published upstream SDK, `@oh-my-pi/{pi-agent-core,pi-ai,pi-coding-agent,omptype}@^18.0.11`. This is PR1 of 3: runtime only. The generator repoint (PR2) and the actual deletion of `forks/oh-my-pi/` (PR3) stack on top, so the subtree is still present and still the generator's schema source after this lands.
### Why upstream, not the Rigel fork
`forks/README.md:151-155` records the subtree's Sealed changes as **NONE** — byte-identical to upstream, with one sealed-added `forks/oh-my-pi/moon.yml`. The `forks/oh-my-pi/patches/` dir is upstream's own `patchedDependencies`, not ours. With no compass-specific patches to preserve there is nothing to carry, so this points at upstream `latest` and drops the vendoring rather than tracking a rebrand.
`bunfig.toml` is untouched: 18.0.11 published 2026-08-29 and is already past the 5-day `minimumReleaseAge` soak, so no exemption entry was needed.
### The five API deltas, each fixed on the real contract
1. **`arktype` → `@oh-my-pi/omptype/ark`.** The direct `arktype` dependency is dropped; `omptype` is an ArkType-compatible lazy-JIT implementation whose `/ark` facade is purpose-built for this swap (it re-exports `type`, `Type`, and `ArkErrors = OmpErrors`). 8 files moved over. One cosmetic delta: `.expression` renders `number % 1 | undefined` instead of `<= 100`. The bound is still **enforced** — probed 101 and 1000 rejected, 100/1/omitted accepted — so `comms.test.ts` now asserts enforcement through the existing `rejects` helper instead of matching a rendered string.
2. **`irc` → `hub`.** The SDK renamed the COOP peer-channel tool. Compass's gate keeps its own name (`isIrcEnabled`, `tools/hub/messaging.ts:109-110`) with identical `taskDepth > 0` semantics, so this is a rename in tests and citations only.
3. **Progressive tool disclosure — `loadMode: "essential"`.** 18.x defaults an omitted `loadMode` at every adapter boundary (including SDK custom tools) to `"discoverable"`: registered, but not in the model's top-level callable schema. Left alone, the container agent would start with none of `comms_*` / `agents_*` / `forge_*` / `board_*` directly callable. Set once at the single registration seam (`cli.ts` `nativeTools`) rather than across 19 tool literals in 4 factories; the test helper mirrors it.
4. **Compaction supersession moved seam.** `elideSupersededCompactionEntries` is gone; elision is no longer a load-time step. It now happens during context assembly (`session/session-context.ts:377`, on the display-transcript path), and stored entries keep their real summaries. The round-trip test now asserts the chain survives load intact, which is what it was actually there to prove.
5. **Project-prompt footer restructured.** The cwd line moved out of `project-prompt.md` into a separate `date-cwd-reminder.md` block this render path does not include. The assertion now checks `<workstation>`, which is what the footer renders.
### Verification
- `bunx tsc --noEmit` — 0 errors
- `bun test` — **677 pass / 0 fail** across 28 files (up from 380 pre-migration: the real SDK runtime now loads, so previously-skipped suites execute)
- `moon run compass-agent:ci` — 5/5 tasks pass
- `moon run toolchain-parity:ci` — green, all 71 pinned tools match the dev shell
The `nodeModules` fixed-output derivation's `outputHash` is repinned (`sha256-JbgM44Aw…`). This is REQUIRED, not incidental: the FOD pins the installed dependency tree, so changing the closure (adding `omptype`, dropping `arktype`, bumping versions) while leaving the hash pinned makes the container build install the OLD 16.5.2 tree — which has no `omptype`, so the bundle failed with `Could not resolve: "@oh-my-pi/omptype/ark"`. The local `bun test` cannot catch this (it reads the real `node_modules`); only the container build does. Verified by building the image: `nix run … container build agent` now produces `image-compass-agent.json`.
`agent-image/entrypoint.nix` natives path re-grounded against the installed package: `--external omp-legacy-pi-modules` still matches the SDK's optional dynamic import (`legacy-pi-compat.ts:751`), the loader's `execDir` candidate is confirmed at `loader-state.js:779`, and both `.node` CPU variants exist at the copy path. Upstream natives naming is self-consistent (`pi-natives` resolving `pi-natives-linux-x64`), so the addon-resolution bug the renamed fork build carries does not apply here.
Spec-impact: none. Refs RIG-2407
Co-authored-by: Matt Wilkinson <matt@rigel.build>
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
|
Compass engineering docs preview: https://compass-repo-rig-2407-sdk-mi.compass-eng-docs.pages.dev Deployed from |
…adMode on all custom tools (RIG-2407) Addresses the review of #924. Two `high` findings, both real defects the migration's own verification missed, plus the mediums and lows. ### high — a `.narrow` rule could reach the model through no channel at all Under omptype a `.describe()` applied after a `.narrow()` SHADOWS the narrow's `ctx.mustBe(...)` reason in the rejection message; arktype appended it. Since a `.narrow` predicate also has no JSON Schema form, a rule spelled only in the narrow became unreachable: the model was told the field's PURPOSE and never the violated RULE, so it could not self-correct and would retry the same invalid value. Verified in isolation (identical schema, same call): ``` withDescribe => must be A label (was "toolongvalue") noDescribe => must be at most 5 chars (was "toolongvalue") ``` Fixed by folding every narrow's rule into its description. The two shared `nonBlank` helpers (`forge.ts`, `board.ts`) now append `(must not be blank)` themselves, so no call site can forget it; the hand-written descriptions in `comms.ts` (`topic` ×3, `channel` ×2, both `peer_handle`s) state their non-blank and ≤120-character bounds explicitly. Audited all 22 `mustBe` sites across the four files; every rule now appears in its description. The blind spot was the assertion, not just the schema: the migration rewrote `comms.test.ts` to check `rejects(...)` booleans, and the boolean verdicts genuinely did not change. Added a regression test that pins the MESSAGE — the surface the harness feeds back to the model verbatim (`pi-ai/src/utils/validation.ts:1722`). Confirmed red-green: reverting one description to its pre-fix text fails the test, restoring it passes. ### high — the loadMode stamp missed every mounted-MCP tool The stamp was applied to `nativeTools` only, but MCP tools merge into the SAME `customTools` array and go through the same adapter. `MCPManager.getTools()` never sets `loadMode` (0 matches under `src/mcp/`), so every mounted-MCP tool silently defaulted to `"discoverable"` — registered but absent from the model's top-level callable schema. The `xd://` transport does not recover them in this session shape: it is gated on a top-level `write` tool this headless session never requests (`tools/index.ts:772`). Net effect was a tool neither top-level callable nor `xd://`-reachable, which is exactly the silent-no-surface failure the RIG-1741/CD-3 mount contract exists to prevent. Moved the stamp to the merged array at the real registration seam, so natives and MCP tools get one uniform documented mode. The mount test now asserts PRESENTATION (`loadMode: "essential"` per tool) rather than bare array membership — membership passed while the tool was unreachable. ### medium / low - Dropped the hardcoded `18.0.3` from the omptype pin note in `comms.ts` (every artifact in the tree says 18.0.11); the lockstep rule is the durable content and `package.json` is the authority for the number. - Recorded the second omptype introspection delta in the `comms.ts` header: `.get(k).description` on an optional UNION node returns the rendered union rather than the authored text, so description contracts must be asserted through `arkToWireSchema`, never `.get().description`. - Fixed the `cli.test.ts` in-body comment that still claimed `elideSupersededCompactionEntries` collapses a summary on load — it contradicted the rewritten assertion 30 lines below it. - Finished the `irc` → `hub` rename in `subagent-tool-split.test.ts`'s docblock (the file's own documentation of what it defends), corrected `lockfile 16.5.2` → `18.0.11`, and repointed the stale citations: `executor.ts:2394/:2489` → `src/task/executor.ts:3113/:3234`, `cli.ts:896` → `cli.ts:926`. - Sorted `@oh-my-pi/omptype` into alphabetical order in `package.json`. ### Verification - `bunx tsc --noEmit` — 0 errors - `bun test` — **678 pass / 0 fail** across 28 files (677 + the new message-level regression test) - `biome check packages/compass-agent` — 0 errors Spec-impact: none. Refs RIG-2407 Co-authored-by: Matt Wilkinson <matt@rigel.build>
…de (RIG-2407) Addresses round 2 of the review of #924. The previous fix commit introduced a high-severity regression; this corrects it and adds the coverage that should have caught it. ### high — the loadMode stamp destroyed every mounted-MCP tool's `execute` The stamp was written as `.map((tool) => ({ ...tool, loadMode: "essential" }))`. `mcp.tools` are `MCPTool` CLASS instances (`pi-coding-agent src/mcp/tool-bridge.ts:492`, built by `fromTools` at `:514`) whose `execute` / `renderCall` / `renderResult` live on the **prototype**. An object spread copies only own enumerable properties, so all three were silently sheared off, and the SDK adapter's `tool.execute(...)` (`sdk.ts:989`) would throw `is not a function` at the model's first call. That traded a demoted-but-intact tool for a top-level-callable one that crashes — strictly worse than the bug the previous commit fixed. The natives are plain object literals, so they were unaffected, which is why nothing reddened. Reproduced directly: ``` instance execute: function | renderCall: function SPREAD execute: undefined | renderCall: undefined FIXED execute: function | renderCall: function | loadMode: essential ``` Fixed by cloning onto the original prototype (`Object.assign(Object.create(Object.getPrototypeOf(tool)), tool, { loadMode })`), which applies the mode without reconstructing the object or mutating the SDK's own instances. ### medium — the fixture could not see it The mount test's fake was `[{ name: "db.query" }]`, a prototype-less literal. Spreading a literal is lossless, so the assertion passed identically whether the implementation spread or not: the test validated the stamp while being blind to the damage the stamp did. Replaced with a class-based fake and added assertions that `execute` and `renderCall` survive. Confirmed red-green — reverting to the spread fails it, the corrected stamp passes. ### medium — the shared `nonBlank` helpers stuttered the rule The previous commit appended `(must not be blank)` unconditionally, but 17 of 18 call sites already ended their text with `must not be blank`, so the model-facing wire schema read `"... must not be blank (must not be blank)"`. The append is now gated on whether the caller already states the rule; the one site that genuinely needed it (`body: nonBlank(STAMP_DESC)`) still gets it. Probed across both helper modules: 0 duplicates, 16 fields each stating the rule exactly once. ### low — helper-path message coverage, and prose Round 1's regression test covered only the hand-written `comms.ts` descriptions, which is exactly how the stutter shipped past it green. Added a sibling test over the shared-helper path in `forge.test.ts` that pins both the rejection message and the absence of stutter in the model-facing schema (`arkToWireSchema`), also confirmed red-green. Re-wrapped a paragraph in `subagent-tool-split.test.ts` left broken by the previous commit's edit. ### Verification - `bunx tsc --noEmit` — 0 errors - `bun test` — **679 pass / 0 fail** across 28 files - `biome check packages/compass-agent` — 0 errors - All three new tests confirmed red-green against their specific defect Spec-impact: none. Refs RIG-2407 Co-authored-by: Matt Wilkinson <matt@rigel.build>
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.
Repoints
compass-agentfrom the vendoredforks/oh-my-pi/subtree onto the published upstream SDK,@oh-my-pi/{pi-agent-core,pi-ai,pi-coding-agent,omptype}@^18.0.11. This is PR1 of 3: runtime only. The generator repoint (PR2) and the actual deletion offorks/oh-my-pi/(PR3) stack on top, so the subtree is still present and still the generator's schema source after this lands.Why upstream, not the Rigel fork
forks/README.md:151-155records the subtree's Sealed changes as NONE — byte-identical to upstream, with one sealed-addedforks/oh-my-pi/moon.yml. Theforks/oh-my-pi/patches/dir is upstream's ownpatchedDependencies, not ours. With no compass-specific patches to preserve there is nothing to carry, so this points at upstreamlatestand drops the vendoring rather than tracking a rebrand.bunfig.tomlis untouched: 18.0.11 published 2026-08-29 and is already past the 5-dayminimumReleaseAgesoak, so no exemption entry was needed.The five API deltas, each fixed on the real contract
arktype→@oh-my-pi/omptype/ark. The directarktypedependency is dropped;omptypeis an ArkType-compatible lazy-JIT implementation whose/arkfacade is purpose-built for this swap (it re-exportstype,Type, andArkErrors = OmpErrors). 8 files moved over. One cosmetic delta:.expressionrendersnumber % 1 | undefinedinstead of<= 100. The bound is still enforced — probed 101 and 1000 rejected, 100/1/omitted accepted — socomms.test.tsnow asserts enforcement through the existingrejectshelper instead of matching a rendered string.irc→hub. The SDK renamed the COOP peer-channel tool. Compass's gate keeps its own name (isIrcEnabled,tools/hub/messaging.ts:109-110) with identicaltaskDepth > 0semantics, so this is a rename in tests and citations only.loadMode: "essential". 18.x defaults an omittedloadModeat every adapter boundary (including SDK custom tools) to"discoverable": registered, but not in the model's top-level callable schema. Left alone, the container agent would start with none ofcomms_*/agents_*/forge_*/board_*directly callable. Set once at the single registration seam (cli.tsnativeTools) rather than across 19 tool literals in 4 factories; the test helper mirrors it.elideSupersededCompactionEntriesis gone; elision is no longer a load-time step. It now happens during context assembly (session/session-context.ts:377, on the display-transcript path), and stored entries keep their real summaries. The round-trip test now asserts the chain survives load intact, which is what it was actually there to prove.project-prompt.mdinto a separatedate-cwd-reminder.mdblock this render path does not include. The assertion now checks<workstation>, which is what the footer renders.Verification
bunx tsc --noEmit— 0 errorsbun test— 677 pass / 0 fail across 28 files (up from 380 pre-migration: the real SDK runtime now loads, so previously-skipped suites execute)moon run compass-agent:ci— 5/5 tasks passmoon run toolchain-parity:ci— green, all 71 pinned tools match the dev shellThe
nodeModulesfixed-output derivation'soutputHashis repinned (sha256-JbgM44Aw…). This is REQUIRED, not incidental: the FOD pins the installed dependency tree, so changing the closure (addingomptype, droppingarktype, bumping versions) while leaving the hash pinned makes the container build install the OLD 16.5.2 tree — which has noomptype, so the bundle failed withCould not resolve: "@oh-my-pi/omptype/ark". The localbun testcannot catch this (it reads the realnode_modules); only the container build does. Verified by building the image:nix run … container build agentnow producesimage-compass-agent.json.agent-image/entrypoint.nixnatives path re-grounded against the installed package:--external omp-legacy-pi-modulesstill matches the SDK's optional dynamic import (legacy-pi-compat.ts:751), the loader'sexecDircandidate is confirmed atloader-state.js:779, and both.nodeCPU variants exist at the copy path. Upstream natives naming is self-consistent (pi-nativesresolvingpi-natives-linux-x64), so the addon-resolution bug the renamed fork build carries does not apply here.Spec-impact: none. Refs RIG-2407
Co-authored-by: Matt Wilkinson matt@rigel.build