feat: own the codebase-memory-mcp lifecycle and wire it into OMP - #1
Merged
Conversation
codebase-memory-mcp indexes a repository into a persistent code knowledge
graph and exposes it over MCP. Its own installer configures 44 client
surfaces and OMP is not one of them, so an OMP-only machine has no path to
the graph at all. This makes `omp plugin install` the whole setup step.
Executable resolution is system-first: pin, PATH, ~/.local/bin, then a
managed copy. An existing installation is adopted as-is and never replaced,
because CBM resolves one canonical per-account cache root and refuses to run
when a process is configured with a different root while another CBM session
is active -- two executables of different versions there produce mismatched
index generations. A private cache root would avoid the conflict by
re-indexing every repository a second time.
Acquisition reproduces upstream install.sh step for step: tag from the
releases/latest redirect rather than the rate-limited API, checksums.txt
digest match for the exact archive name, HTTPS on every redirect hop, the
closed four-member archive namespace, regular-file-not-symlink extraction,
the Linux -portable build, macOS quarantine removal and ad-hoc signing, and
a --version smoke run. Nothing is written under the package-owned root until
every step passes.
One key in the active agent directory's mcp.json is owned and nothing else:
idempotent upsert, siblings and indentation preserved, fail closed on a
command value this package did not write, removal only when it still
matches. It has to be the native file -- a plugin-root .mcp.json gets no
${VAR} expansion and command gets no pre-connect resolution, so a committed
file cannot name a home-relative executable.
No tool_call handler is registered. OMP treats a throwing tool_call handler
as a refusal of the tool call, so a handler there could deny an operator's
grep because a subprocess timed out; the packaging test asserts its absence.
Background work uses the handler context's managed timers, never the
platform globals, because a raw timer callback that throws is fatal to the
whole session.
Refs: rasen/changes/binary-lifecycle-and-mcp-wiring
`tagFromLocation` checked the redirect's path prefix but not its origin, so a `releases/latest` response pointing at `https://elsewhere/DeusData/codebase-memory-mcp/releases/tag/v9.9.9` was mined for a version string this package then treated as the newest release. The path prefix says nothing about who answered. Found by the test added here, which was written to cover the spec scenario that had no coverage. The blast radius was limited -- asset URLs are built from a hard-coded `releases/` base rather than from the redirect -- so the effect was a wrong version string, not a download from an attacker's host. It is still the "reject an unexpected Location" refusal the spec asks for. Two pieces of validation were extracted so they are reachable from a test at all: `nextHop` is the whole transport-downgrade defence, and `tagFromLocation` parses attacker-adjacent input into something used as a URL segment and an on-disk directory name. Neither could be exercised through a real request without a TLS origin that redirects to plain HTTP. The install confirmation moved from the extension entry into `confirmedInstall`/`installHazard` behind a `Confirmer` seam, for the same reason: the spec requires that a session with no interactive UI report the hazard and download nothing rather than block, and that branch was previously only reachable from a real session. `src/index.ts` now only adapts `ctx.hasUI` and `ctx.ui.confirm` to that seam. Coverage added, all previously uncovered spec scenarios: - redirect transport downgrade, missing and empty location headers, and eight release-location refusals including a path separator in the tag - install confirmation: no hazard, declined, accepted, and no interactive UI - session start with nothing resolving, and with a foreign entry present
`run` is the only `Bun.spawn` in the codebase and sits on the session-start `readVersion` path, so both of its limits have to hold against a process that does not cooperate. Neither did. Output was buffered through `new Response(child.stdout).text()` with no byte bound, so a hostile candidate or a member-heavy `tar` could exhaust the OMP process's memory well inside the timeout. Both pipes are now drained through bounded readers that stop the child once either exceeds the cap. The deadline was the worse of the two. It was awaited only against the pipe drain, so a descendant that inherited a pipe held the read open and the timeout never fired: `(sleep 2) & printf ok` under a 100ms deadline took 2014ms and reported success. Racing the drain alone was still not enough, because a child that closes both pipes early and keeps running settles the drain at once and left `child.exited` unbounded: that shape took 2018ms and also reported success. The deadline now races `Promise.all([drained, child.exited])`, so neither can beat it, and every deadline win reaches the SIGKILL escalation before the readers are released. The child is spawned with `detached: true` and reaped by process group so an ordinary descendant dies with it. The trade-off is deliberate and accepted: our subprocesses leave the terminal's foreground group, so Ctrl-C on OMP no longer reaches an in-flight `tar` or `codesign`, and such a child can outlive OMP by seconds. That beats the alternative, where the descendant is never reaped at all. Both reproducers now return in ~100ms with zero surviving descendants.
The 1 MiB bound on `checksums.txt` was checked in `parseChecksums`, which only runs after `download` has already done `await response.arrayBuffer()` on the whole body. The limit therefore provided none of the resource safety it was written for: a large or chunked response could consume arbitrary memory and take the OMP process down before the refusal ever executed. The checksum fetch now goes through a bounded reader that consumes `response.body` chunk by chunk and throws the moment the running total passes the cap, cancelling the reader. There is deliberately no `Content-Length` precheck: a chunked response carries none, so it would add a second code path that the streaming cap has to cover anyway. `parseChecksums` keeps its own byte-length refusal as a belt-and-braces check behind the streaming one. Also retitles the `-ui-` checksums case, which was documented as proving exact-name selection but could not: its expected digest is byte-identical to the one published for the non-`-ui-` archive, so a parser that resolved either name through the other's line would still pass. The neighbouring case carries that proof with a genuinely different digest.
…ally Three verification steps did not hold the property they were written for. The closed four-member check was not closed. Each `tar -tzf` record went through `raw.trim().replace(/^\.\//, "").replace(/\/+$/, "")` and an empty result was skipped, so the real spellings `./`, `/` and a whitespace-only name all normalised to `""` and were silently dropped rather than refused — an archive carrying the four required files plus one of those passed the gate. Records are now either an exact allowlist member or its `./<name>` spelling, or they are rejected, and the refusal quotes the name so an otherwise invisible record is identifiable. Only the terminal line delimiter is skipped. A skipped record is a hole in a closed set, which is the whole point of the check. Adoption was not transactional. `adopt` wrote and chmodded directly at the final `bin/<version>` path with no staging and no rename, and the scratch cleanup in `finally` could reject after the executable was already in place, turning a committed adoption into a reported failure. Re-adopting the version that is currently resolved therefore rewrote the live executable, contradicting the requirement that any failure leave the previously resolved executable untouched. The candidate is now staged inside `managedBinRoot` (same filesystem, so the `rename` is atomic), a single rename is the only commit point, staging is removed on every pre-commit failure, and cleanup can no longer convert success into failure. `xattr` failures were indistinguishable from the tolerated missing-attribute case: the `RunResult` was discarded, so a non-zero exit, a timeout and a spawn failure all looked like "the attribute was not there", while the `codesign` result immediately below was checked. Only the no-such-attribute outcome is tolerated now. Also gives `buildArchive` a single staging root with an `afterAll` removal. It had no matching cleanup and leaked 25 directories per unit run; 553 had accumulated on the review machine.
This package writes exactly one key into a file it does not own, so every failure mode here costs the operator configuration they did not ask us to touch. Several of them did. `mcp.json` and `state.json` were written with `Bun.write` straight at the destination, which keeps the inode: `open(O_TRUNC)` plus write, not temp plus rename. OMP writes the very same file atomically and says why, and OMP's reader drops the whole file silently on a parse failure — so an interrupted write cost the operator every user-level MCP server with no warning, while this package then refused the file forever as unparseable. The design's risk section promises the two-writer race degrades to "the entry is missing", which a truncated file is not. Both writers now stage a per-writer temp file in the same directory and rename it into place. The staging file is chmodded to the destination's own mode first, defaulting to 0600 only when there is no destination: `rename` replaces rather than truncates, so without that step a file OMP had written 0600 came back 0644, and that file can carry per-server `env` secrets and `auth.clientSecret`. Ownership of the entry was decided by exact string match against what state recorded, so an entry whose `command` is provably under `managedBinRoot` — a path only this package ever writes — was reported to the operator as somebody else's, and session start never corrected it. Reachable with no hand editing, because a truncated `state.json` falls back to empty state. Ownership is now decidable from the path as well, via `path.relative` containment rather than a prefix match, so a lookalike sibling stays foreign. Idempotence was decided on re-rendered bytes, so a compact or CRLF file whose entry was already correct came back "updated" and was rewritten reformatted, failing "re-running with no change does not rewrite the file" and needlessly re-exposing the write. It is decided on the parsed entry now. `uninstall` deleted the managed executable even when it had just refused to remove the entry naming it, leaving OMP spawning a deleted file with the state that could reclaim the key gone too — unrecoverable without a hand edit. It now keeps both halves when the refusal would strand the entry, and only when it would: a refusal for an entry legitimately pointing at a system CBM lets the copy go, so the command is not blocked by an entry it never owned. Smaller, same theme: a `mcp.json` that exists but cannot be read was reported as absent instead of as a structural refusal naming the errno; a non-object `mcpServers` was silently discarded and overwritten rather than failing closed; a future-dated `lastCheckedAt` suppressed the upstream check for the whole clock skew; `install <version>` recorded the requested version as the newest upstream one and suppressed the real check for 24 hours; and uninstall left behind an empty `mcp.json` the operator never had. Tests: `state.ts` had no test file at all, and its sanitisation layer — the only thing between a corrupted `state.json` and a recorded value joined straight into a path — could be deleted with the suite green. Three more mutants that survived are now each caught by one named test: `install()` deleting the package root on failure, `syncEntry` collapsing `rewired` into `unchanged`, and `adopt()` wiping previous version directories. The `~/.local/bin` proof was a bare `rejects.toThrow()` that any rejection satisfied, including the path existing as a regular file. `resolve.ts` is unchanged apart from a comment: resolution deliberately does not execute the candidate, because running an unknown binary at every session start is worse than adopting it, and `/cbm status` reads the version separately. A test pins that contract.
`loadIsolated` isolated the module but not the environment. Loading runs the
factory, which resolves the agent directory from the real `homedir()` and
`process.env` and stands down when `<agentDir>/extensions/codebase-memory.ts`
exists, so the two load-bearing assertions — including the `tool_call`
negative — were decided by state outside the repository:
bun test test/packaging # 5 pass
PI_CODING_AGENT_DIR=<dir with that file> ... # 3 pass, 2 fail
`~/.omp/agent/extensions/` already exists on a developer machine, so the
guard was one filename away from firing, and it fires for certain the moment
upstream ships the `--clients=omp` writer the guard exists to detect. The
packaging gate would then fail for a reason that has nothing to do with the
bundle. The load now points at the scratch directory `loadIsolated` already
creates, restoring the previous value afterwards, so the stand-down branch is
decided by the test.
Also removes that scratch directory, which was never cleaned up, and drops
`expect(handlers).not.toContain("tool_call")`, which passes vacuously on an
empty handler list; the sibling `toEqual(["session_start"])` is what actually
catches a regression.
…kout `publish` was the only checkout in the workflow set omitting `persist-credentials: false`, and the only job elevating to `permissions: contents: write`. `actions/checkout` therefore wrote an `AUTHORIZATION: basic <token>` extraheader into `.git/config` and left a push-capable credential on disk for the rest of the job. Nothing in the job needs it: `gh release create --verify-tag` authenticates through the `GH_TOKEN` env var and resolves the tag over the REST API, not through the git remote, and `persist-credentials: false` removes only the extraheader while still configuring `origin`. So the credential was pure blast radius for any step later added to `publish` — an asset build, a notes generator, a `bun install` running a lifecycle script.
The README claimed, directly beneath `omp plugin install github:...`, that "this is the command CI installs into a scratch project on every push, so it is verified against the exact ref you type". None of that was true. CI ran `bun add "github:<repo>#<ref>"` — never `omp plugin install`, always with a `#<ref>` suffix the documented spec does not carry, and only on pushes to `main`, tag pushes and manual dispatch. The documented development install `omp plugin link .` was exercised by no job at all, and the job's own comment claimed it did "the same thing OMP's plugin installer validates" while its `check.ts` only asserted `typeof module.default === "function"`. The gap was material rather than cosmetic: `omp plugin install` runs `#validateInstalledExtensions`, which loads the extension and so actually invokes the factory on the installed tree. Rather than narrow the claim, this makes it true. `install-check` now reads the OMP CLI version out of `devDependencies` (one source of truth, so a second copy cannot drift), installs it, and runs both documented commands — `omp plugin install` and `omp plugin link .` — each under its own scratch `HOME`, asserting discovery through `omp plugin list`. Two homes are required, not stylistic: linking on top of a git-spec install of the same package name fails with EPERM. The hand-written `check.ts` heredoc is gone, since what replaced it is strictly stronger. The version read is guarded to an exact pin. A range would resolve to whatever is newest at job time, which is the drift reading the manifest was meant to avoid, and the accepting glob needs a preceding reject arm because it ends in an unanchored `*` — without it `18.0.8 || 19.0.0` matches on its prefix. Scope guards are unchanged: still `push` or `workflow_dispatch` only, because a pull request's merge ref is not an installable remote ref, and still absent from the aggregate gate's `needs`, so a skipped job cannot fail the required check. README now states exactly what CI does, including the `#<ref>` form and the trigger set, and points the factory-loads-clean property at the packaging test where it actually lives.
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.
Summary
This pull request is the repository's first change: an installable OMP extension package that owns the
codebase-memory-mcp(CBM) executable's lifecycle and wires exactly one MCP server entry into OMP's native user configuration, soomp plugin installis the whole setup step.CBM indexes a repository into a persistent code knowledge graph and exposes it over MCP. Upstream contribution is out of scope for this project, so this repository is the distribution. The current situation has three concrete gaps:
Deliberately deferred to the next change: the durable context rule, the skill, the tiered graph agents and their harvest pipeline, and the
tool_resultgrep/glob augmentation.Design decisions
Three decisions shape everything else and are worth reviewing directly.
PATH,~/.local/bin, then a managed copy — and an existing installation is adopted as-is, never replaced. CBM resolves one canonical per-account cache root and refuses to run when a process is configured with a different root while another CBM session or command is active, so two executables of different versions there produce mismatched index generations. Giving a managed copy a private cache root would avoid the conflict by re-indexing every repository a second time, which for a large tree is hours of work to hold the same answers twice. The cost is that this package cannot guarantee a version; that cost is made visible by/cbm statusrather than hidden.mcp.json, not a plugin-root.mcp.json. A plugin-root MCP file gets no${VAR}expansion andcommandgets no pre-connect environment resolution, so a committed file cannot name a home-relative or package-owned executable. The native file can carry an absolute path and sits at MCP discovery priority 1. The price is that this package writes into operator configuration, so the write is constrained hard: one key, idempotent, siblings and indentation preserved, fail closed on acommandthis package did not write, removal only when the value still matches, and the write itself staged-then-renamed so an interrupted write cannot cost the operator every user-level MCP server.install, which would configure every client surface it detects — precisely what an OMP-only operator installed this package to avoid. Each step exists because upstream's installer has it: tag from thereleases/latestredirect rather than the rate-limited GitHub API,checksums.txtdigest match for the exact archive name, HTTPS re-checked on every redirect hop, the closed four-member archive namespace, regular-file-not-symlink extraction, the Linux-portablebuild, macOS quarantine removal and ad-hoc signing, and a--versionsmoke run. Nothing is written under the package-owned root until every step has passed, and adoption commits through a singlerename.Two things this extension deliberately does not do
Both are absences, so both are asserted rather than described.
tool_callhandler is registered. OMP treats a throwing or blockingtool_callhandler as a refusal of the tool call, so a context provider registered there could deny an operator'sgrepbecause a subprocess timed out.test/packaging/bundle.test.tsasserts the handler set is exactly["session_start"], which is what keeps the next change's output augmentation ontool_resultinstead.src/scheduler.ts, because a rawsetTimeoutcallback that throws escapes handler dispatch, surfaces as anuncaughtException, and OMP's postmortem handler tears down the whole session.Command surface
/cbmcovers every lifecycle decision, and none of it needs an interactive terminal./cbm status: resolved source, absolute path, local version, last known upstream version, pin state, resolved agent directory, and whether the MCP entry is present and current./cbm install [version]: downloads, verifies, and adopts a managed copy. When a system executable already resolves, it explains the shared-cache-root hazard and requires confirmation; with no interactive UI it fails with that reason rather than waiting./cbm update: updates a managed copy. For an adopted system copy it only reports and points at CBM's ownupdate, because CBM's activation path drains active sessions and performs a transactional target swap that a second writer would corrupt./cbm pin <version>and/cbm unpin: hold a version so update checks report but never adopt./cbm uninstall: removes the managed copy, this package's state, and the owned MCP entry, and leaves an adopted system executable in place.Review history, and what it changed
The branch was verified and then put through a three-round review cycle with role-isolated workers (four reviewer slices, three fixer slices, no reviewer reviewing its own fix and no fixer verifying its own work). Both instruments changed the code, which is the argument for having run both.
Verification found one defect:
tagFromLocationchecked the redirect's path prefix but not its origin, so areleases/latestresponse naminghttps://elsewhere/DeusData/codebase-memory-mcp/releases/tag/v9.9.9was mined for a version string. Fixed in58a82d9.The review cycle then found 25 findings verification had not, three of them introduced by the cycle's own fixes and caught only because the re-review was done by a non-author. The substantive ones:
b0be213). A pipe closes when its last writer does, sosh -c "(sleep 2) & printf ok"returned its direct child immediately while a descendant held the read open — measured 2014 ms against a 100 ms timeout, reported as a success. The deadline now racesPromise.all([drained, child.exited]), reaps the child's process group, and releases the readers. Both reproducers now return in ~100 ms with zero surviving descendants. Output is bounded at 256 KiB per stream as well, sincetimeoutbounds time and not bytes.df41a25). The old normalization rantrim()and stripped trailing slashes, which collapsed the real tar spellings./,/and a whitespace-only name to""and skipped them — a hole in a set whose whole purpose is being closed. Records are now accounted for or refused. Adoption also became transactional: it stages insidebin/(same filesystem, sorenameis atomic) with one commit point, so re-adopting the version currently resolved can no longer truncate the live executable behind a failedchmod.mcp.jsonandstate.jsonwere truncated in place (8e9f69c). OMP readsmcp.jsonwith a bareJSON.parseand drops the whole document on failure, so an interrupted write costs the operator every user-level MCP server silently. Both files are now staged-then-renamed, and the rename reproduces the destination's mode — the first version of that fix widened 0600 to 0644 on a file that can carryenvsecrets andauth.clientSecret. Ownership is now decidable from the path as well as from recorded state, so a lost state file no longer makes the owned entry permanently unreclaimable.checksums.txtcap was enforced after buffering (b478c51), which is the one place the memory was not actually saved. It is now enforced while streaming, and still re-checked in the parser, which is reachable from aReleaseSourcethat never downloaded anything.install-checkverified a hand-written approximation of the documented command (496a60c). It now installs the pinned OMP CLI and runs the documentedomp plugin installandomp plugin link .under scratch homes, which exercises OMP's own#validateInstalledExtensions— strictly stronger than thetypeof default === "function"check it replaced. The README's claim was made true rather than narrowed.bc1969e). It is the only job that elevates tocontents: write, andactions/checkoutwas leaving anAUTHORIZATION: basic <token>extraheader in.git/configfor every later step.gh release createauthenticates throughGH_TOKEN, so nothing needed it.ff113df). It was one filename away from failing, and would have failed outright the moment upstream ships the--clients=ompwriter the guard exists to detect.Tests went 142 → 205 (279 → 417 assertions), and the increase is load-bearing: each new test was proved red under a targeted mutant rather than accepted because it passes. Three mutants that survived the first round —
install()deleting the package root on failure,rewiredcollapsing intounchanged,adopt()wiping previous version directories — now each fail exactly one named test.Accepted-known at clean-time, recorded rather than dropped: resolution adopts a
PATHexecutable without running it (deliberate — spawning an unknown binary at every session start is the worse default, and/cbm statusreads the version separately);detached: truemeans Ctrl-C on OMP no longer reaches an in-flighttar/codesign(accepted, because without it a descendant is never reaped at all); and two test rows assume the suite does not run as root, which the declaredruns-on: ubuntu-24.04satisfies.Key files changed
src/platform.tssrc/release.tschecksums.txtcap enforced while streamingsrc/exec.tssrc/acquire.tsrenamesrc/resolve.tssrc/mcp-config.tsmcpServerskey: idempotent on the parsed entry, foreign-entry refusal, path-or-state ownership, and staged-then-renamed writes that preserve the destination's modesrc/lifecycle.tssrc/paths.tssrc/index.ts/cbmcommand, thesession_starthandler, and the early return when a CBM-written native extension is already present.github/workflows/ci.ymlhygiene,bun,install-checkrunning the documented commands through OMP's CLI, and the aggregatecigate.github/workflows/release.ymlsource.ref, withci.ymlreused throughworkflow_callTest plan
Verified on macOS/arm64 with Bun 1.3.14, at tree
ea5915a0945f0be933596c934cb24aa89ed08881:bun install --frozen-lockfile— no changesbun run typecheck— clean understrict,noUncheckedIndexedAccess, andexactOptionalPropertyTypesbun run test:unit— 205 tests, 417 assertions, 0 failures across 10 filesbun run test:packaging— 5 tests; rebuildsdist/index.js, loads it from a directory holding nothing else, and asserts the handler set is exactly["session_start"]git diff --exit-code -- dist/index.js— the committed bundle is byte-identical to a fresh build, and imports onlycrypto,fs,fs/promises,os,pathloadAllMCPConfigsreportingprovider: "native"and a real stdiotools/listreturning 15 toolsrun(["sh","-c","(sleep 5; : marker) & printf ok"], {timeoutMs:100})returns in ~102 ms withok: falseand zero survivorsci / hygiene,ci / bun,ci / cigreen on this PR;install checkskipped by policyVerified only after merge, by design:
install checkon the push tomain— a pull request's merge ref does not exist on the remote as an installable ref, which is why the job runs on pushes and manual dispatch and is absent from the aggregate gate'sneedsrelease.yml'sversion-gateat the first tag pushFull evidence — verification report, review-cycle report with four slice reports, end-to-end transcripts, the hygiene-check transcript, and the ADR for the one task-list deviation — lives in the planning store at
rasen/changes/binary-lifecycle-and-mcp-wiring/evidence/.🤖 Generated with Claude Code