feat(platform): plugin-contributed navigation (phase 1 of Dev Platform extraction) - #536
Merged
Merged
Conversation
Foundation for extracting the Dev Platform into an installable plugin
(specs/470-dev-platform-plugin/plan.md). Ships the one capability the
extraction cannot be done without, and fixes a live bug found while
mapping it. No dev-platform code moves here.
Nav contribution
----------------
A plugin can now contribute entries to the operator navigation:
ctx.uiRoutes.registerNav({ navId, href, cluster?, order?, label })
Extends the existing UiRouteCatalog rather than adding a third catalog
beside it and admin_ui_path. Nav entries are a separate registration
from uiRoute descriptors because their path semantics differ — a
descriptor is relative to the plugin's /p/<pluginId> mount, a nav entry
is an absolute in-app path. One registration carrying both would make
one of the two fields a lie.
Labels are resolved server-side for the requested locale and fetched in
the root layout, so the shell stays on next-intl's single i18n clock. A
client fetch of pre-resolved strings would lag next-intl on a locale
switch and render a half-translated nav, plus shift the header after
hydration.
Every field is validated as untrusted input — it renders inside the
shell's trusted header. href is confined to single-slash in-app paths
(blocking //host, /\host and schemes); labels are length-capped and
screened for control and bidirectional-formatting characters, which
could otherwise visually spoof adjacent core entries.
Merge rules: an entry joins the cluster it names; an unknown or absent
cluster promotes it to top level rather than swallowing it on version
skew; plugin entries never reorder static ones; an entry colliding with
a static href is dropped so a plugin cannot shadow a core destination.
Dev Platform is the first consumer: its menu entry now comes from the
DEV_PLATFORM_ENABLED block instead of Nav.tsx's literal, and the /admin
grid card is gated on it. Turn the feature off and both are gone with no
frontend rebuild.
Stale-route fix
---------------
ToolPluginRuntime.deactivate() stopped jobs and disposed UI routes but
never called pluginRouteRegistry.disposeBySource(), despite holding the
dependency and threading it into every plugin context.
DynamicAgentRuntime already did. Express cannot unmount, so a
deactivated tool plugin's routers stayed live — and because Express
matches first-mount-wins, kept serving after uninstall and shadowed
later mounts at the same prefix after a hot-upgrade.
This is a prerequisite rather than a drive-by: "with the plugin not
installed, no dev-platform code paths" is unverifiable while routers
outlive their plugin. The regression test fails 3/4 without the fix.
Tests: 29 catalog, 11 route, 4 disposal (node:test), 12 merge (vitest).
Full suites green — middleware 4890 pass/0 fail, web-ui 331 pass.
Two independent GPT-5.6 review passes over the previous commit. Findings
that were real, with the fixes:
Security / correctness
----------------------
- href was validated in raw rather than canonical form, so the "core
destinations win" rule was bypassable by aliasing: `/x/%2e%2e/admin`,
`/admin/`, `/admin?a=1` and `/admin#x` all navigate to Admin but none
string-match `/admin`. Rather than normalising (and having to track the
URL parser), only already-canonical paths are accepted — unreserved
charset segments, no dot-segments, no trailing slash, no query, no
fragment, no percent-encoding. For that subset, string equality and
browser resolution agree.
- Zero-width codepoints joined the label screen; they allow a label that
renders identically to a core entry while comparing unequal.
- Added the bounds that were missing: href/navId/cluster length, locales
per label, and nav entries per plugin. The catalogue is serialised into
every page's root-layout RSC payload, so an unbounded href or a
thousand entries degrade the whole UI.
- mergeNav deduplicated only against static hrefs, so two plugins
claiming the same path rendered duplicate React keys and both lit up as
active. Now first-wins across all entries.
- Equal-order entries sorted with localeCompare, which can order the same
pair differently under Node's ICU than under the visitor's browser — a
hydration mismatch. Replaced with codepoint comparison, with pluginId
and navId as tiebreakers so the order is total.
- Longest-prefix matching had no segment boundary: /admin was active on
/administrator, and a plugin leaf /reports claimed /reports-old.
- The web-ui parse did not actually enforce the contract it claimed to.
It now re-applies the middleware's rules — the middleware is a separate
deployable, and version skew or a compromised control plane must not be
able to put an off-origin link into the shell's chrome. Notably
`JSON.parse('{"order":1e400}')` yields Infinity, which is a number and
would poison every comparison in the merge sort.
- The admin card was keyed on href alone, so any plugin could resurrect a
core card by claiming the path. Keyed on the contributing plugin now.
Lifecycle
---------
- Route/nav disposal moved BEFORE the plugin-controlled close() is
awaited. close() gets 5s; disposing after it left routers answering and
the menu entry visible for that whole window after the operator
triggered deactivation, and for the full budget when close() hangs.
- activate() now rolls back its own route/nav/job registrations when a
plugin registers and then throws or times out. Such a plugin never
reaches active.set, so deactivate() returned false and never cleaned
up — the orphan served for the life of the process.
Availability
------------
- The nav fetch is awaited by the root layout, i.e. on every page's
critical path, and had no timeout. Added 2s; a hung middleware now
costs one menu, not the UI.
Tests
-----
Added canonical-href, bounds, zero-width, prototype-pollution,
cross-plugin duplicate, segment-boundary, dispose-ordering and
defensive-parse coverage. Middleware tests converted to run socket-free
via test/_helpers/httpInvoke.ts, which drives the real Express pipeline
through app.handle without binding a port.
Docs
----
docs/CHANGELOG.md and docs/middleware-agent-handoff.md updated per
AGENTS.md; specs/470-dev-platform-plugin/plan.md corrected (the "both
halves ship in the same image" justification was false — middleware and
web-ui are separate images, so cross-image skew needs a compatibility
signal in P3) and now records known limitations rather than carrying
them silently.
…-plugin-extraction # Conflicts: # docs/CHANGELOG.md # web-ui/app/admin/page.tsx
Two constraints added to the extraction: the plugin lives in its OWN
REPOSITORY, and every hardcoded dev-platform part leaves core entirely.
They sound small. They invalidate two decisions in the previous draft:
- The plugin can no longer be a built-in package in middleware/packages.
An own repo makes it a distributed plugin, a materially weaker delivery
vehicle.
- The React pages can no longer stay compiled into web-ui. The previous
draft justified that by "both halves ship in the same image" — under
the new constraints those 3,933 LOC are simply hardcoded core
references and must go. That resurfaces the "a distributed plugin
cannot ship React" problem the draft had sidestepped.
New: specs/470-dev-platform-plugin/core-decoupling-checklist.md — the
work-list. 276 items across 18 zones, ~49,100 LOC across ~200 files
(166 delete / 66 move / 44 genericise), with file:line references.
Three items are not deletions at all. Core has no extension point for
them, so a generic mechanism has to be built first:
H1 auth/publicPaths.ts is a frozen literal. Deleting the two
dev-platform entries without a replacement 401s every runner
phone-home and kills jobs in flight. Needs manifest-declared,
operator-consented grants AND exclusive prefix ownership — a grant
alone only makes requireAuth call next() for a URL, it does not say
which router may answer it.
H2 The conductor hardcodes the dev_job step kind and channel type
across runExecutor.ts, awaitStore.ts and all of devJobStepEffect.ts
(incl. `AND channel_type <> 'dev_job'` in the human-inbox query).
Needs a generic long-running-step registry.
H3 chat/page.tsx hardcodes `tool.name === 'dev_job_start'` and renders
a core-compiled React card for it. An iframe per tool call is not
acceptable, so this needs a declarative card schema or an accepted
degradation to ToolRow.
Also newly identified: the ZIP extension allowlist has no `.css`
(verified) — a distributed plugin cannot ship a compiled SPA's
stylesheet, only inline styles in one .html. So G7 (plugin UI) is a hard
blocker, and the recommendation is to fix the mechanism (allowlist +
static-asset serving + published Lume tokens) rather than rewrite 3,163
LOC of Lume React as hand-rolled HTML. That is worth more than a one-off
fix: it is the platform's weakest extension point today.
And G8: the DevJob* types are published `@omadia/plugin-api` surface, so
removing them is a deliberate SemVer-major break, not an internal
refactor. `api/admin-v1.ts` leaks `dev_jobs` onto the public admin DTO
too.
Code change (the one item that could be done now):
middleware/src/devplatform/githubApp/appJwt.ts
→ middleware/src/services/githubAppJwt.ts
That file was the ONLY core→devplatform reverse dependency: core's
builder (plugins/builder/githubAppAuth.ts) imported it out of the
dev-platform tree, which made that tree a dependency of core and blocked
extracting it. The primitive is generic GitHub App JWT minting with
nothing dev-platform-specific about it. All three call sites updated.
Middleware 4971 pass / 0 fail, typecheck clean.
Weegy
enabled auto-merge (squash)
July 29, 2026 05:14
This was referenced Jul 29, 2026
Weegy
added a commit
that referenced
this pull request
Jul 29, 2026
One design pass per hard problem (H1 public paths, H2 conductor step
kinds, H3 chat card + plugin UI, G4 plugin SQL + migration handoff, G8
plugin-api contract, P4 repo split + supply chain). specs/470-dev-
platform-plugin/implementation.md is the synthesis: what they changed,
what they found, and the PR sequence.
Five decisions in plan.md were wrong or under-specified:
1. publicPaths must NOT become a dynamic set. requireAuth runs before
routing and structurally cannot know who will answer, so putting the
grant there rebuilds the hole it is meant to close. Use a mount slot
BEFORE requireAuth that terminates — fail-closed by construction,
and publicPaths.ts stays a frozen literal.
2. The chat card is neither a generic schema nor a degradation. A
generic node tree makes core a rendering engine for untrusted markup;
degradation makes the human gate — the platform's principal safety
mechanism — annoying, and annoying safety mechanisms get bypassed.
A closed 7-node contract with liveness mediated by core. A
plugin-supplied SSE URL would be an SSRF aimed at the operator's own
session.
3. Do NOT add .css to the ZIP allowlist. The inability to ship CSS IS
the enforcement for the Tailwind vocabulary.
4. The plugin-api break has no installed base — see below.
5. The vault re-key, not the migrations, is the most irreversible step.
Migrations are idempotent and additive; a deleted GitHub App private
key is gone.
Six live bugs found, none caused by the extraction. Two verified here:
B1 ctx.services.get is completely ungated (platform/pluginContext.ts
:230-233 is a bare pass-through). Any installed plugin can call
ctx.services.get('graphPool') and receive the superuser pg.Pool —
full read/write on users, conductor_runs, everything. No manifest
declaration, nothing in the install dialog. Biggest hole found in
this epic and it is live today.
B4 .sql is not in the ZIP extension allowlist, so a distributed
plugin cannot ship migrations at all. Blocks G4 exactly as the
missing .css blocked G7.
Reported and not yet independently verified: ServiceRegistry is never
disposed on deactivate (same class as the router bug fixed in #536, one
layer down); all five core migrators race on multi-replica boot with no
advisory lock; the conductor dev-job step is dead code in production so
the reconciliation sweep has never run; dev_repo_plugin_grants is never
cleaned on uninstall.
And a trap in my own plan: removing ctx.devJobs without replacing its
gate would have converted a permission-gated, kernel-attributed accessor
into an ungated, self-attributed one — because B1 is the only remaining
path. Two designs flagged it independently from different directions.
Two risks moved in opposite directions. H2 got much cheaper: the
conductor step never ran, so there are almost certainly no live dev_job
awaits and backwards compatibility is nearly free. G8's SemVer risk
evaporated: @omadia/plugin-api is private:true and its publish job is
gated `if: false` — never published, no installed base, so take the
break now and cut 1.0.0 clean. H3 and P4 got visibly more expensive.
Also newly found: next/font and data-theme do not cross an iframe
boundary (silent font and dark-mode regressions), and keyless cosign
binds the certificate identity to repo+workflow+ref — publishing the
same image from the new repo makes every daemon with a pinned identity
refuse to launch jobs.
The migration handoff needs per-file schema WITNESSES, not trust in the
donor ledger. Donor rows present with tables absent — a restore, a
skewed rollback, an incident — makes a naive seed activate green while
every request 500s.
Sequence: Phase A (C1-C8) ships five reusable platform capabilities and
moves zero dev-platform code, ending at the abandonment checkpoint.
Phase B (P0-P5, C10-C13) is copy → prove → delete, with the proof gate
at P5 and the two publicPaths exemptions deleted last, alone, in a
revertible commit.
Six decisions block work; D1 (publish plugin-api to public npm) blocks
everything after it.
Weegy
added a commit
that referenced
this pull request
Jul 30, 2026
ServiceRegistry had no owner tracking and no disposeBySource, and toolPluginRuntime.deactivate() disposed routes and uiRoutes but not services. A provider whose close() forgets its handle left the service registered against a torn-down module, and reinstall then threw "duplicate provider". Same bug class PR #536 fixed for Express routers, one layer down. - serviceRegistry.ts: owner tracking on provide()/replace(), and disposeBySource() unwinding LIFO — an older `replace` restore would otherwise reinstate a provider a newer one has since shadowed. `owner` is optional, so core's ~25 boot-time provide() calls stay untracked and can never be bulk-disposed. - pluginContext.ts: ctx.services.provide/replace pass agentId, so attribution comes from the kernel-known id and never from a caller-supplied argument. No plugin-api contract change. - toolPluginRuntime.ts: disposeBySource before the awaited close(), same 5s-budget reasoning as the route disposal, plus the activate-failure rollback. - dynamicAgentRuntime.ts: same gap confirmed and mirrored. 13 tests. Verified fail-without-fix in three staged reverts: reverting both runtimes gives 4 real assertion failures (not TypeErrors); reverting the context fix alone fails the attribution test; full pre-fix state fails 11. Also fixes toolPluginRuntimeRouteDisposal.test.ts's fixture, which omitted the now-required serviceRegistry dep — fixed the fixture rather than making the production call defensive, since the real wiring always supplies it. 5,058 pass, typecheck and lint clean. TWO OTHER FIXES FROM THIS BATCH WERE DELIBERATELY NOT SHIPPED — see the follow-up notes. Adding '.sql' to the zip allowlist would weaponise a pre-existing path traversal into arbitrary SQL execution, and wrapping the migrators in an unbounded advisory lock would convert a rare race into a deterministic boot failure. Both verified against the code. Known gaps in this fix, both worth follow-ups: - withTimeout is a bare Promise.race and does not cancel, so a timed-out activate can still register services after the rollback ran. - DynamicAgentRuntime.activate() has no rollback block at all, and its route disposal still sits after the awaited close() — the pre-#536 ordering.
Weegy
added a commit
that referenced
this pull request
Jul 30, 2026
… implementation (#539) * docs(470): plugins use Tailwind, so they ship no CSS (G7 reduced) Marcel's point: web-ui is Tailwind, so require Tailwind in plugins and the missing `.css` in the ZIP allowlist stops mattering. Validated, and it is better than the workaround it replaces. The catch is the whole design: Tailwind v4 emits only classes it has SEEN. It detects them by scanning source at build time, and a plugin installed at runtime from another repository is never scanned. So this only works if core pre-generates a documented, finite vocabulary. v4 supports exactly that — `@source inline(...)` (the replacement for v3's `safelist`, brace-expandable) plus `@import "tailwindcss" source(none)` to disable scanning. Measured with the repo's own tailwindcss 4.3.3 + @tailwindcss/postcss, not estimated (probe kept as specs/470-dev-platform-plugin/ plugin-tailwind-subset.probe.css): 43,199 B raw → 7,704 B gzip → 5.7 KB brotli for layout/flex/grid/spacing/typography/borders/shadows, sm:/md:/lg: and hover:/focus:/disabled: variants, with colours restricted to the Lume tokens — .bg-accent, .text-fg-muted, .border-border, .text-danger all verified present in the output. Worth more than unblocking this extraction: - Plugins inherit the design system by construction. They get OUR colour names wired to the runtime CSS variables, so they follow the active palette and light/dark automatically and cannot hardcode a hex. - It retires a known drift hazard. middleware/src/admin-ui/ harness-admin-css.ts is 345 hand-maintained lines whose own header says "mirror web-ui/app/_lib/theme.css; keep the two roughly in sync when the design system changes". Generating both from the same tokens removes the sync obligation instead of restating it. - It is enforceable: reject `[` in class attributes at ingest. HARD CONSTRAINT now in the contract: no arbitrary values (`w-[137px]`, `bg-[#abc]`). That space is unbounded, cannot be pre-generated, and such a class renders unstyled with no diagnostic — the worst failure mode. Documentation alone is not enough; it needs the ingest check. Implementation note: the `@theme inline` bridge currently lives inside globals.css:16-48 and must be extracted to its own file that both it and the plugin stylesheet import — otherwise the two drift, which is the exact failure this is meant to end. G7 is downgraded from "hard blocker" to the JS-bundle question alone: `.js` and `.map` are already allowlisted, so a compiled SPA can ship today; what is still missing is a static-asset serving path from the plugin's router. Much smaller than a styling story. * feat(470): automated decoupling ratchet + functional acceptance matrix Answers a question the existing docs could not: how do we KNOW every function got extracted and that the result is installable? They could not, and this is the gap: - core-decoupling-checklist.md enumerates FILES. You can move all ~200 and still silently lose a feature — a file inventory cannot tell you a capability survived. - plan.md stated success criteria in prose. Prose is not a probe. - The only "completeness check" was a single `rg` in the P6 exit criterion, i.e. a one-shot grep nobody runs. Two additions. 1. scripts/check-core-decoupling.mjs — a ratchet, wired into CI as the `core decoupling ratchet (#470)` job. Counts Dev Platform references across 12 zones of core and FAILS if the count rises. Baseline is 3,171 (middleware/src 1621, test 925, web-ui/app 192, sidecars 180, packages 86, migrations 70, compose 39, scripts 30, ci 16, messages 4). `--update` only ever lowers it; raising it needs a hand-edit, so a new coupling shows up in review instead of slipping in. This is what makes the checklist's staleness survivable: even if the sweep missed a reference, the count still sees it, and the count cannot reach zero while it survives. It also stops core re-acquiring a dependency mid-extraction, which is the realistic failure mode for a multi-week epic touching ~200 files. And it turns "finished" into a machine-checked fact (count 0) rather than an assertion. Verified in both directions: passes at baseline, exits 1 with the offending zone named when a reference is added. 2. specs/470-dev-platform-plugin/acceptance.md — the functional contract, which is the actual answer to "all the functions". 34 HTTP endpoints (9 job admin, 9 repo/credential, 2 gates, 5 GitHub App, 7 runner phone-home, job-policy, webhook), 3 chat tools, ctx.devJobs, 4 background loops, 4 UI screens, the chat card, the dev-transcript CLI and the conductor `dev.job` step kind — each with an owner and a probe. Plus install/uninstall/upgrade acceptance, which does not exist yet and belongs in P4. Rows whose MECHANISM must exist in core first are marked: the seven runner endpoints and the App callback need H1 (public paths), the webhook needs G3 (raw body), the chat card needs H3, the conductor step needs H2, ctx.devJobs needs the G8 contract decision. Honest about what is still not covered, in acceptance.md §4: the capability matrix is a review checklist rather than a smoke suite, install/uninstall cannot be tested before P3/P4, and once the plugin leaves this repo nothing here verifies it still satisfies §2 — that becomes the plugin repo's CI against a published core contract. Also flagged: the boot-time safety refusals (SUBSCRIPTION_MODE without ACK, UNSAFE_LOCAL without LOCAL_UID) must become activation refusals, or misconfiguration silently activates instead of failing closed. Stacked on the Tailwind commit because both edit plan.md; the ratchet is independently reviewable and independently revertible. * docs(470): index the epic — one entry point for plan, checklist, acceptance Marcel wants the whole planning to live in ONE PR, because the implementation happens there too. This is the entry point that makes that real: what each of the four documents answers, what already merged via #536, what is in flight, and the two decisions that block code. Also writes down the working agreement for a long-lived epic PR: one commit per phase so ~49k LOC stays reviewable and revertible; wire paths frozen (deployed runners phone home to literal URLs); do not delete the publicPaths exemptions before H1 is proven; and the abandonment checkpoint after P3/P3b. * docs(470): implementation plan from six parallel design passes One design pass per hard problem (H1 public paths, H2 conductor step kinds, H3 chat card + plugin UI, G4 plugin SQL + migration handoff, G8 plugin-api contract, P4 repo split + supply chain). specs/470-dev- platform-plugin/implementation.md is the synthesis: what they changed, what they found, and the PR sequence. Five decisions in plan.md were wrong or under-specified: 1. publicPaths must NOT become a dynamic set. requireAuth runs before routing and structurally cannot know who will answer, so putting the grant there rebuilds the hole it is meant to close. Use a mount slot BEFORE requireAuth that terminates — fail-closed by construction, and publicPaths.ts stays a frozen literal. 2. The chat card is neither a generic schema nor a degradation. A generic node tree makes core a rendering engine for untrusted markup; degradation makes the human gate — the platform's principal safety mechanism — annoying, and annoying safety mechanisms get bypassed. A closed 7-node contract with liveness mediated by core. A plugin-supplied SSE URL would be an SSRF aimed at the operator's own session. 3. Do NOT add .css to the ZIP allowlist. The inability to ship CSS IS the enforcement for the Tailwind vocabulary. 4. The plugin-api break has no installed base — see below. 5. The vault re-key, not the migrations, is the most irreversible step. Migrations are idempotent and additive; a deleted GitHub App private key is gone. Six live bugs found, none caused by the extraction. Two verified here: B1 ctx.services.get is completely ungated (platform/pluginContext.ts :230-233 is a bare pass-through). Any installed plugin can call ctx.services.get('graphPool') and receive the superuser pg.Pool — full read/write on users, conductor_runs, everything. No manifest declaration, nothing in the install dialog. Biggest hole found in this epic and it is live today. B4 .sql is not in the ZIP extension allowlist, so a distributed plugin cannot ship migrations at all. Blocks G4 exactly as the missing .css blocked G7. Reported and not yet independently verified: ServiceRegistry is never disposed on deactivate (same class as the router bug fixed in #536, one layer down); all five core migrators race on multi-replica boot with no advisory lock; the conductor dev-job step is dead code in production so the reconciliation sweep has never run; dev_repo_plugin_grants is never cleaned on uninstall. And a trap in my own plan: removing ctx.devJobs without replacing its gate would have converted a permission-gated, kernel-attributed accessor into an ungated, self-attributed one — because B1 is the only remaining path. Two designs flagged it independently from different directions. Two risks moved in opposite directions. H2 got much cheaper: the conductor step never ran, so there are almost certainly no live dev_job awaits and backwards compatibility is nearly free. G8's SemVer risk evaporated: @omadia/plugin-api is private:true and its publish job is gated `if: false` — never published, no installed base, so take the break now and cut 1.0.0 clean. H3 and P4 got visibly more expensive. Also newly found: next/font and data-theme do not cross an iframe boundary (silent font and dark-mode regressions), and keyless cosign binds the certificate identity to repo+workflow+ref — publishing the same image from the new repo makes every daemon with a pinned identity refuse to launch jobs. The migration handoff needs per-file schema WITNESSES, not trust in the donor ledger. Donor rows present with tables absent — a restore, a skewed rollback, an incident — makes a naive seed activate green while every request 500s. Sequence: Phase A (C1-C8) ships five reusable platform capabilities and moves zero dev-platform code, ending at the abandonment checkpoint. Phase B (P0-P5, C10-C13) is copy → prove → delete, with the proof gate at P5 and the two publicPaths exemptions deleted last, alone, in a revertible commit. Six decisions block work; D1 (publish plugin-api to public npm) blocks everything after it. * docs(470): correct D1 — publishing plugin-api was never required Marcel pushed back on D1 ("warum ist das der Blocker? Wir brauchen das doch nicht public?!") and he was right. The evidence was two directories away and I did not look. - omadia-byte5-plugins already solves this in production for six private plugins: package.json declares "@omadia/plugin-api": "file:../odoo-bot/middleware/packages/plugin-api" with the sub-packages carrying "*" as a peer resolved by the workspace root. No registry, no publish, nothing public. - The boilerplate contract mandates the OPPOSITE of what I recommended. Point 1: "KEIN Cross-Import ... Die Interface-Definition wird bewusst in ./types.ts dupliziert ... Absicht nicht Bug." omadia-plugin-starter ships vendored types/omadia-plugin-api.d.ts for exactly this. - There is no runtime dependency at all: every @omadia/plugin-api import in the dev-platform tree is `import type` and vanishes from the emitted JS. Even a value import would resolve against the host's own node_modules, which the uploaded-package store symlinks in. Root cause of the error: the design pass recommended public npm on a PRODUCT argument — GitHub Packages needs auth even to read, which would hurt a third-party plugin ecosystem. Sound for a public ecosystem, irrelevant for a private byte5 plugin. I passed it through as a technical blocker without checking how this org actually builds private plugins. D1 drops from "blocks everything after it" to a P3-typecheck-only choice between three options, none of them public: file: sibling (proven, but the plugin repo's CI then needs a core checkout — friction already recorded in project memory), vendored .d.ts (CI-isolated, drifts silently), or a git dependency on a tag (CI-isolated, explicit version). Consequence for the sequence: the first real step is no longer C1 but the B-fix PR — the three live bugs that are wrong today independently of this epic. * fix(470): corrections from the codex deep-check Full verification pass against the code (GPT-5.6, reasoning=high) over all five planning documents. It found eleven errors. The three that would have cost the most: 1. THREE CAPABILITIES IN THE ACCEPTANCE MATRIX ARE DEAD IN PRODUCTION. Verified here: - the conductor dev.job step — conductor/index.ts builds the executor with no devJob dep, so the dispatch branch never fires - ctx.devJobs — `provide('devJobs', …)` exists NOWHERE in src/, so the accessor throws on every call - tracker polling — TrackerPoller is never constructed or started The matrix was written from source, and source presence is not production reality. It would have certified preservation of capabilities the operator never had. Marked in acceptance.md; each needs a delete-or-wire decision in P2b. 2. THE RATCHET HAD A ZONE GAP AND OVERLAPPING ZONES. middleware/.env.example (19 references) was covered by no zone, so the count could have read 0 while it still documented DEV_* keys — the exact false-negative that would make "0 means done" a lie. And a root-config zone rescanned the whole web-ui tree, double-counting web-ui/app. Fixed: 14 depth-bounded, disjoint zones; baseline 3,171 → 3,181; and the check is now PER ZONE, because an aggregate-only comparison passes while one zone falls and another rises — which is precisely what a half-finished move looks like. Verified the guard now catches a regression in the previously invisible zone. acceptance.md now states plainly what the ratchet does NOT prove: it counts identifiers, not behaviour. Necessary condition, not sufficient. The earlier "machine-checked definition of completion" claim was too strong. 3. A MISSING CAPABILITY, WHICH IS THE DANGEROUS DIRECTION. The endpoint count was 34; it is 35 business endpoints / 36 handlers. The miscount hid the omission: the LLM proxy has TWO handlers and only one was listed. GET /api/v1/dev-runner/llm/ is a liveness probe the CLI depends on and it was absent from the matrix entirely. Also corrected: - The wireDevPlatform ↔ routes "cycle" is NOT an import cycle. wireDevPlatform is imported only by index.ts and no route imports back. One-way layering inversion. C3 is boundary cleanup and must not be justified as fixing hoist-dependent behaviour. - `pgPool@1` invented a capability name; the established contract is `graphPool@1`, already provided by harness-knowledge-graph-neon. Second D1-class error — a recommendation contradicting house practice. Now: gate the EXISTING graphPool@1 behind permissions.sql. - C1 still said "publish to public npm" — residue of the corrected D1. plugin-api stays private:true; only the .d.ts golden snapshot lands. - B3: at least eight migrators race, not five. - B1: the hole is real, the "superuser" characterisation is unproven. - B6: the MCP grant bug is live; the dev-repo grant half is unwired. - Arbitrary Tailwind values CAN be pre-generated when named exactly (@source inline("w-[137px]") emits it). What cannot is the unbounded universe. The vocabulary argument holds; the absolute phrasing did not. And ingest sees compiled Vite JS, not JSX class attributes, so "reject [ in class attributes" is under-specified. - src/devplatform is 53 files / 14,457 LOC, not 54 / 14,498. - plan.md §4.1 and §4.2 contradicted each other on DevJob type ownership. §4.2 wins: the types move to the plugin repo. Flagged, not yet resolved: a Vite multi-file SPA is not supported by today's plugin contract (boilerplate mandates single-file HTML and a tsc-only build), so P2 is viable only after C8 ships static serving. And unknown manifest keys are silently IGNORED, not rejected — a plugin declaring permissions.public_paths against an unpatched core would activate with no grant and no error. * feat(470): decide the dormant capabilities — and there are five, not three Three design passes (one per capability) plus a codex verification round. The verdicts differ, which is the finding — "activate all three" would have been wrong. 1. Conductor dev.job step → ACTIVATE as its own PR (C5b), or delete 2. ctx.devJobs → DELETE 3. Tracker polling → DEFER, move dormant 4. TrackerRegistry → DEFER, moves with #3 (newly found) 5. Comment-back → DEFER, moves with #3 (newly found) #4 and #5 surfaced while designing #3. acceptance.md listed comment-back as live with the probe "result posted to the issue" — it is not wired, so a polled job's result never reaches the issue and the loop is half-open even if the poller ran. THE DECISIVE FINDING is on ctx.devJobs, and it inverts the intuition. Every access gate lives in the ACCESSOR, and every identity is a parameter the CALLER passes — listGrantedRepoIds(pluginId), cancelJob(jobId, requestedByPluginId), createdBy:{kind:'plugin',id}. Verified: the host service itself verifies nothing, and DevRepoPluginGrantStore is never constructed, so the grant table has no writer at all. Combined with the ungated ctx.services.get (B1), the moment ANYONE registers 'devJobs' — core today or the extracted plugin tomorrow — any installed plugin can fetch it with no manifest declaration and no operator consent, pass an arbitrary pluginId, and bypass the permission gate, the repo-grant scope, the creator check and the audit attribution, while framing another plugin. So the dead state is SAFER than the wired state. "It throws on every call" is currently load-bearing. That also sharpens implementation.md §2.2 by a notch: I had written that REMOVING ctx.devJobs without replacing its gate opens the hole. True — but ADDING the provider opens it too. `provide` is the dangerous operation. C2 bundling the gate fix with the removal is a correctness requirement, not a convenience. And a test-shape lesson worth more than this epic: pluginDevJobsAccessor.test.ts has a case titled "throws a clear error when the host service is unregistered" — it asserts the PRODUCTION BEHAVIOUR as the error path and stays green, against a two-line fake registry. No test in the repo boots a real ServiceRegistry and asks whether anything provided the service. A boot-level accessor/provider invariant should ship independent of #470. ONE THING SHIPS NOW: cross-source trigger dedupe. hasActiveTriggerJob filters on `source` and dev_jobs_webhook_one_active is scoped WHERE source='webhook', so a repo with both triggers would get two runners, two LLM budgets and two PRs for one issue. Latent only because no tracker job has ever been created. It is the only artifact here that is not thrown away by the extraction. CORRECTIONS FROM THE VERIFICATION ROUND — the first draft had errors, and two of them understated risk: - cold start costs $500, not $150 (default budget is $5, page limit 100). Understated by 3x. - "a dry run would spend real money" was overstated: the supported route is previewRun, which explicitly stubs action steps. - "nobody could ever author the step" is wrong — validation is bypassed on the raw POST / path. - "a poller with nothing to poll" is overstated — TrackerRegistry has a built-in GitHub fallback needing no registration. - hooking DevJobStore.finishTerminal contradicts our own contract: finalizeDevJob is the documented choke point, and the decoupling checklist names it. Fix the split finalizer wiring instead. - phase-engine terminals do NOT bypass boundFinalize; only the worker-driven ones do. - the widened index is not "mandatory today" — the existing webhook-only index already makes the live path replica-safe — and it is NOT a safe drop-in: it needs a duplicate preflight, and it should land after the migrator advisory-lock fix, not before. PROCESS FAILURE, named because it matters: the first draft proposed resolutions and did not propagate them, leaving acceptance.md and implementation.md still saying "three" and still requiring preservation of things this document deletes. A decision doc that contradicts its siblings leaves the spec set worse than before. Propagated here, along with the stale README ratchet numbers (12 zones/3,171 → 14 zones/3,181). * docs(470): record Marcel's answers — devJobs delete confirmed, tracker is a roadmap foundation Two open questions answered, and the second changes more than the first. NO customer-side or unreleased plugin declares permissions.devJobs. The DELETE verdict for ctx.devJobs is confirmed — the single fact that could have inverted it does not exist. G8 collapses almost entirely with it, and the plugin-api major bump becomes hygiene rather than a break with downstream cost. YES, a Jira/Linear tracker is on the roadmap and is considered important. The tracker verdict keeps its direction — defer, move dormant — but loses its meaning: 'and forget about it' was wrong. TrackerRegistry is not dead weight being tolerated, it is the extension point for a roadmap feature. Three consequences: - The blockers stop being hypothetical. Cold start ($500 ceiling), requireGate:false with no sender allowlist, firing on any ticket update rather than on label application, and the cross-source dedupe gap become must-fix before a Jira tracker runs. - A new architectural question, in no document until now: after extraction the registry lives in the dev-platform PLUGIN repo, so a Jira tracker would be a plugin registering into another plugin's registry. That seam constrains the extraction — it may argue for keeping a generic job-trigger-source extension point in CORE rather than moving the registry out. - It inherits B1. A tracker registry is a WRITE surface: registering a tracker influences which issues become code-execution jobs. With ctx.services.get ungated, any plugin could register one. The per-caller-factory fix in C2 becomes a prerequisite, not optional hardening. Design pass on the cross-plugin seam is in flight. * feat(470): invert the tracker seam — provider, not registry Marcel confirmed a Jira/Linear tracker IS on the roadmap and matters. That turned "move the registry dormant and forget it" into an architecture question: after extraction the registry lives in the dev-platform PLUGIN repo, so a Jira tracker would be a plugin registering into another plugin's registry. The answer is to invert the direction: Jira plugin = PROVIDER provides: ["devTracker.jira@1"] dev-platform = CONSUMER services.get('devTracker.' + repo.trackerKind) per repo, per sweep — no tracker `requires` TrackerRegistry is then DELETED, not moved. Its plugin-map half becomes the services.get lookup; its GitHub-fallback half folds into dev-platform's own resolver. The naive direction fails four ways: it hands a MUTABLE registry through an ungated accessor; registerTracker(kind, factory) has no caller attribution (identity is the key the caller picks); services.replace() is an exposed MITM primitive; and the ABI is DevRepo-shaped — a ~40-field internal type that moves to the plugin repo at P4, paired with a return type from a core route file deleted at C10. Both sides of that signature cease to exist where a third party can reach them. Inverted, the object crossing the seam is a read-only stateless service — the same risk class as graphPool@1, which this org already ships. And it is what makes C2's per-caller factory pay off: the credential owner decides who may use its credentials. Applied to a shared registry the factory would gate who may REGISTER, which is the wrong question. THREE VERIFIED FINDINGS, all with consequences beyond the tracker: 1. The hot-install path bypasses capability resolution entirely. index.ts routes `case 'extension'` straight to toolPluginRuntime.activate(agentId) — no resolveEligiblePlugins, no topo-sort. So `requires`-based ordering applies only on the BOOT path; for the normal case (operator installs from the hub at runtime) it does nothing. Any design leaning on activation ordering is already broken there — which weakens ordering arguments elsewhere in these docs, including the ctx.devJobs inversion discussion. 2. findDependents checks depends_on only, never capability `requires`. An operator can uninstall a provider with live consumers, no 409. 3. source_ref is `owner/name#N`. FINDING 3 INVERTS THE SHIP ORDER I RECOMMENDED. A Jira PROJ-123 coerced to 123 collides with GitHub issue #123, so widening the unique index BEFORE namespacing source_ref ships a false-POSITIVE dedupe: a Jira ticket silently suppressing an unrelated GitHub issue. Namespace first (jira:PROJ-123), widen second. And the dedupe fix is less load-bearing than I claimed. listPollableRepos selects `... AND (tracker_kind IS NOT NULL OR credential_kind = 'github_app')` — that OR is what drags webhook-covered GitHub repos into the poll set, the sole source of the double-job risk. Delete the built-in GitHub fallback and no repo is ever both polled and webhooked for the same ticket. The widened index drops to defence-in-depth (still worth having: migration 0025's source='plugin' is a third potential writer). Verdict changes: - Tracker polling: DEFER → DEFER-AND-HARDEN. Behind a flag, contract frozen, expiry kept. P3's exit condition becomes "cannot be switched on without all six hardening fixes". - TrackerRegistry: DEFER → DELETE. Nothing to move once inverted, and that is what lets the ratchet reach 0 without an allowlist entry. - Comment-back: no longer "moves with #3" — REWRITTEN at P3 against the tracker contract; only the marker/idempotency logic survives. The contract must be frozen BEFORE the poller is hardened: Ticket needs ticketId (opaque string), displayKey, labels[] and labelAppliedAt, plus updatedSince as a provider parameter. Without labelAppliedAt the "fires on any update" bug is unfixable at the consumer. Home: src/devplatform/trackerContract.ts in Phase A, travelling at P4 — the treatment already agreed for devJobTypes.ts. * docs(470): scope correction — conductor dev-job is delete, not genericise Marcel: 'Der Conductor ist eine neue Funktion. Was hat das mit der Dev Platform zu tun?' Correct, and it exposed a scope error. The Conductor is a real, live feature (31 files, 6202 LOC backend, 23 UI files, 7 migrations, its own spec). It is in this epic only because its code holds 73 dev-platform references that must leave for the ratchet to reach 0 — not because anything about Conductor itself is being changed. I turned that into 'build a generic step-kind registry (H2/C5) and activate the step (C5b)' — a new platform capability plus a new feature, neither of which anyone asked for, propagated through the plan as a hard blocker. Removing a dev-platform reference from core has exactly two paths: genericise, or delete. Genericising is only justified when something real needs the generic version. Nothing did. So G9 drops from hard blocker to a deletion, C5 shrinks to 'delete dead code', and C5b disappears along with the await_kind migration, the registry deactivation semantics and the cross-kind guard — all unbuilt. Deleting is not lost work: the existing code is dev-job-SHAPED, so a generic registry would replace it anyway. Only the design has value, and that survives in this document. * chore(470): resync with main (PR #529) and re-baseline the ratchet main brought PR #529 — a substantial dev-platform change: 59 files, +3,751 LOC. New web-ui surfaces (PhaseArtifactPanel, PrettyArtifact, ToolCallCard, lineDiff/prettyArtifact/toolCallLog libs + tests), LLM proxy test coverage, and 27 new i18n lines per locale. The ratchet caught it exactly as designed: 3,181 → 3,293 across six zones (src +14, test +32, packages +10, sidecars +15, web-ui/app +35, compose +6). This is the documented hand-edit case — main legitimately ADDED dev-platform code, so the count rises for a legitimate reason rather than core re-acquiring a dependency. Baseline raised deliberately and recorded here. Re-measured, since the checklist is a snapshot: src/devplatform 53 files, 14,457 → 14,520 LOC web-ui admin surface 20 files / 3,163 → 29 files / 4,344 LOC adminDevPlatform i18n 269 → 288 keys (3,205 total) Also propagated the verdicts into acceptance.md's warning box, which still carried the pre-correction versions: conductor step now DELETE (not "activate as C5b"), TrackerRegistry DELETE (the seam inverts), comment-back REWRITE at P3, polling DEFER-AND-HARDEN. Verification after merge: middleware build + typecheck clean, 5,044 pass; web-ui typecheck clean, 388 pass, i18n parity OK at 3,205 keys. One middleware failure did not reproduce on re-run — consistent with the pre-existing load-sensitive flakiness already documented (a file of 48 trivial assertions reproduces it; baseline without added files is clean). * fix(platform): dispose plugin-provided services on deactivate ServiceRegistry had no owner tracking and no disposeBySource, and toolPluginRuntime.deactivate() disposed routes and uiRoutes but not services. A provider whose close() forgets its handle left the service registered against a torn-down module, and reinstall then threw "duplicate provider". Same bug class PR #536 fixed for Express routers, one layer down. - serviceRegistry.ts: owner tracking on provide()/replace(), and disposeBySource() unwinding LIFO — an older `replace` restore would otherwise reinstate a provider a newer one has since shadowed. `owner` is optional, so core's ~25 boot-time provide() calls stay untracked and can never be bulk-disposed. - pluginContext.ts: ctx.services.provide/replace pass agentId, so attribution comes from the kernel-known id and never from a caller-supplied argument. No plugin-api contract change. - toolPluginRuntime.ts: disposeBySource before the awaited close(), same 5s-budget reasoning as the route disposal, plus the activate-failure rollback. - dynamicAgentRuntime.ts: same gap confirmed and mirrored. 13 tests. Verified fail-without-fix in three staged reverts: reverting both runtimes gives 4 real assertion failures (not TypeErrors); reverting the context fix alone fails the attribution test; full pre-fix state fails 11. Also fixes toolPluginRuntimeRouteDisposal.test.ts's fixture, which omitted the now-required serviceRegistry dep — fixed the fixture rather than making the production call defensive, since the real wiring always supplies it. 5,058 pass, typecheck and lint clean. TWO OTHER FIXES FROM THIS BATCH WERE DELIBERATELY NOT SHIPPED — see the follow-up notes. Adding '.sql' to the zip allowlist would weaponise a pre-existing path traversal into arbitrary SQL execution, and wrapping the migrators in an unbounded advisory lock would convert a rare race into a deterministic boot failure. Both verified against the code. Known gaps in this fix, both worth follow-ups: - withTimeout is a bare Promise.race and does not cancel, so a timed-out activate can still register services after the rollback ran. - DynamicAgentRuntime.activate() has no rollback block at all, and its route disposal still sits after the awaited close() — the pre-#536 ordering. * chore(470): resync with main (#537) and re-baseline; refresh status main brought the #440/#537 embedding work, which added 10 more dev-platform references (mostly tests): 3,293 → 3,303 across src, test and packages. Third hand-edit of the baseline, same legitimate reason as the previous two — main ADDED dev-platform code; core did not re-acquire a dependency. That distinction is now stated in the README so the next raise is not read as a regression. Also fixes a sentence an earlier perl replacement broke in the README ("... It / But it counts ...") and refreshes the status section, which still claimed only Tailwind + ratchet were in flight. It now records what actually landed, what went out separately, and — more useful — the three fixes that were implemented and deliberately NOT shipped because review found real harm in them.
6 tasks
Weegy
added a commit
that referenced
this pull request
Aug 7, 2026
…curity review (#550) * ci: apply middleware/migrations in the schema job MIGRATION_DOMAINS listed five domains and omitted middleware/migrations, the core runtime domain holding 0001-0030. Every migration there had shipped without ever being applied — or re-applied for the idempotency check — against a real Postgres in CI: the entire MCP schema (0003, 0008, 0009, 0010/0013, 0012/0014, 0015/0016, 0017-0020) and every dev-platform migration (0022-0030). Suspected during #330, now confirmed and closed. No latent schema defect was exposed. All 30 files apply and re-apply cleanly against pgvector/pgvector:pg16, in both possible domain orderings and additionally with rows present. The domain is self-contained: no cross-domain foreign keys, no object names shared with the other five domains, and no extension dependency (gen_random_uuid is core since pg13). The comment now records the three domains that remain uncovered, each of which needs its own audit before being enabled. * test(mcp): first pg coverage for the MCP registry and OAuth schema No pg test touched MCP before this — only memoryStoreConformance, pluginVerdictStore and skillLifecycleStore existed. Covers the registry seed and catalog-kind backfill (0010 + 0013, including that 0013's UPDATE actually lifts the official registry off the 'generic' column default), the kind/auth_kind/source/registered_via CHECK sets, marketplace provenance with ON DELETE SET NULL detaching an imported server from a deleted catalog, the 0014 partial unique index on top-level MCP grants (and that it leaves native grants alone), and the 0015/0016 OAuth surface — authorize-time endpoint pinning plus token/flow cascade on server delete. Each assertion was mutation-checked against a deliberately broken schema. A second suite covers what the CI gate structurally cannot: the CI idempotency check re-applies against an EMPTY database, so it can never catch a migration that only breaks once rows exist. It re-applies all 30 files with MCP rows in place, in its own throwaway database — re-running 0001/0003 drops and recreates the NOTIFY triggers, which must not happen underneath a concurrently running suite. Both suites skip when no test Postgres is reachable and scope every row to a w04-mcp- tenant prefix. Pools are capped: the runner executes files concurrently and ~16 other pg suites each hold a default-sized (max 10) pool, so an uncapped extra pool here exhausts max_connections and cancels an unrelated suite mid-run (observed on ConductorWebhookSubscriptionStore). * feat(mcp): preserve structuredContent via an out-of-band sidecar and capture outputSchema Issue #547 (W1-3) — plumbing only, no canvas synthesis. Discovery now keeps a tool's declared outputSchema: McpToolDescriptor and McpDiscoveredTool gained an optional outputSchema, and listTools copies it from tools/list (object-valued only; anything else is dropped). It rides along in the existing mcp_servers.discovered_tools jsonb column, so it survives a restart without re-discovery and needs no migration. subAgentToolHydration rehydrates it on the way back out and seeds the manager's cache, since mcpNativeHandler only closes over a tool name. structuredContent is no longer discarded. A new extractStructured() reads it and McpManager hands it to an optional McpManagerOptions.structuredSink as { kind: 'structured_output', serverId, toolName, turnId, structured, outputSchema? }. Error results and absent/null payloads emit nothing. This is deliberately out-of-band rather than a widened return type. callTool() still returns Promise<string> and NativeToolHandler is untouched, which keeps the published plugin contract stable and keeps every MCP result on the 'typeof result === string' path that gates Privacy Shield masking in the orchestrator — a non-string result would bypass the shield. The payload union is a discriminated 'kind' so #544 (MRTR) can add 'input_required' without another refactor. renderToolResult is byte-for-byte unchanged and is now pinned by a golden suite (text-only, mixed blocks, structuredContent-only, empty content, array-valued structuredContent, isError, whitespace fallback, nullish). A mutation check installs a hostile sink that rewrites and deep-mutates its payload and returns a different object, then asserts the LLM-bound string is unchanged; verified to fail against a deliberately in-band mutant. Operator surface: a read-only 'returns structured output' badge in the MCP Control Center, with en + de strings. * fix(mcp-oauth): validate RFC 9207 iss, make delegation explicit, single-flight refresh Three live security/correctness defects in the MCP OAuth path. D1 — no RFC 9207 `iss` validation. The OAuth callback trusted the `state` parameter alone. `state` proves a response belongs to a flow we started; it does NOT prove which authorization server issued the code, so a malicious or compromised MCP server could steer the callback and have a code minted by one AS redeemed at another. `iss` is now validated against the issuer bound to the flow BEFORE the code is exchanged, so a rejected callback persists nothing — no token row, no vault write. A mismatched `iss`, or an absent one from an AS that advertised `authorization_response_iss_parameter_supported`, is rejected. That advertisement is captured at authorize time in the new `mcp_oauth_flows.iss_required` column rather than re-discovered at the callback, for the same reason migration 0016 pinned the token endpoint: a server that can flip the flag in between would simply opt itself out of the check. D2 — silent 'operator' fallback (confused deputy). Both the operator router and the runtime McpManager resolved the OAuth user key as `… ?? 'operator'`, so a Teams or Telegram turn whose user had no mapped identity reached the customer's MCP server holding the OPERATOR's token. Resolution now goes through the new `services/mcpDelegation.ts` and the new `mcp_servers.delegation` column: `per_user` yields no token when no identity resolves and the turn fails closed through the existing `onAuthFailure` path with an explanation; `service` is the explicit opt-in to one shared identity. The fallback literal is gone from every call site. D3 — refresh race. `getValidAccessToken` permitted N concurrent refreshes per (server, user). Against an AS with rotating refresh tokens the losers get `invalid_grant` and the last writer can persist an already-retired token, silently disconnecting the user. Concurrent callers now share one in-flight promise keyed by (serverId, userKey), cleared in a `finally` so a failed refresh never poisons later attempts. Also: - `mcp_oauth_tokens.issuer` records which AS minted a token; a rotated issuer drops the stored token instead of replaying it against a different server. - `mcp_call_log.acting_identity` records WHOSE authority each call used (`caller_agent` is the orchestrator slug, not the identity). Resolved via a new optional `McpAuthProvider.resolveIdentity`, threaded through `callTool` before the dispatch guard so denied calls are attributed too. An unattributable call is recorded as `unresolved`, never left blank. - OAuth failure logging goes through `services/secretRedaction.ts`: tokens, `code`, and `code_verifier` can no longer reach a log line, including values a provider echoed back that we never minted. The callback's error page is redacted too. - New `PUT /mcp-servers/:id/delegation` plus a delegation control in McpAuthSection, with `adminMcp.auth.delegation*` keys in en.json and de.json. Tests: 40 in test/mcpOAuth.test.ts covering iss present/absent/mismatched/blank and trailing-slash equivalence, fail-closed resolution, issuer rotation, and redaction. The D3 test is mutation-checked — it asserts exactly ONE token-endpoint HTTP request under 8 concurrent callers (verified to report 8 and fail when the in-flight map is removed), not a count of mock invocations. BEHAVIOUR CHANGE (operator-visible): a fail-closed `per_user` default for every row would break installed deployments whose channel users reach MCP servers today BECAUSE of the 'operator' fallback. Migration 0031 is therefore deliberately asymmetric — every EXISTING `mcp_servers` row that already holds a stored operator token is set to `delegation = 'service'`, preserving today's behaviour, and only NEWLY created servers get the safe `per_user` default. Operators must review grandfathered servers and switch the ones that should be per-user. * feat(orchestrator): race every tool dispatch against a per-tool deadline One hung sub-agent used to pin the whole Promise.allSettled batch for the rest of the turn: domainQueryTool awaits agent.ask() with no abort and no timeout, and there was no per-tool deadline anywhere in the orchestrator. dispatchTool now races an AbortSignal-backed deadline (default 120s, OMADIA_TOOL_DISPATCH_TIMEOUT_MS, 0 disables) and returns a structured Error: result on timeout. The abandoned dispatch is marked aborted, so a late result is discarded before the first write into turn state (raw-result capture, canvas sentinel, KG ingestion, privacy interning) and late sub-agent events are dropped by an abort-guarded observer. * fix(mcp): state the callTool request policy and stop retrying Unauthorized callTool passed no RequestOptions and silently inherited the SDK's 60s default, so the real ceiling was undocumented and un-tunable. It now passes an explicit { timeout, resetTimeoutOnProgress, maxTotalTimeout } (env-tunable), where resetTimeoutOnProgress keeps long streaming calls alive and maxTotalTimeout is the absolute ceiling. looksTransient() also matched a bare -32001, contradicting its own contract: the code is implementation-defined and servers legitimately use it for Unauthorized (omadia's LoopbackMcpServer does), so a genuine auth failure got one doomed retry before surfacing. Auth now wins; a real SDK request timeout still retries via its "Request timed out" message. * test(orchestrator): prove the dispatch deadline discards late results The mutation check is captureRawToolResult: a real turn-state write the routine runner reads back. Verified by temporarily removing the deadlineSignal guard — the suite then fails on the capture assertion, not on a missing error string. Also covers batch siblings resolving normally, the 0-disables path, and a bad env value falling back to the default. * test(mcp): isolate the re-apply check in a schema, not a database The re-apply-under-data suite originally built its private copy of the domain in a throwaway database. That isolates correctly, but CREATE/DROP DATABASE is a cluster-wide operation: run inside the full suite with a test Postgres reachable, it stalled the concurrently executing dev-platform pg suites long enough that 29 of their tests were cancelled with "test did not finish before its parent". Reproduced deterministically against appStore.pg.test.ts and devJobStore.pg.test.ts, and confirmed absent from the same run with this file removed. It now runs against a dedicated schema on one pinned connection with `public` off the search_path. The migrations name every object unqualified, so they build a private copy there and never touch — or take ACCESS EXCLUSIVE on — the shared tables. Cancellations: 29 -> 0. The test asserts the isolation itself (table count in the schema), because a leaked search_path would turn the migrations into no-ops against the shared tables and make every later assertion pass vacuously. Both suites now share the file's single capped pool, closed once in a file-level after hook. * perf(orchestrator): deterministic tool ordering + stateless loopback MCP server W0-3 — sort the dynamic tool segments by name so the Anthropic prompt-cache tool block is byte-stable across machines and deploys. `buildToolsList()` stamps `cache_control: {type:'ephemeral'}` on the last tool spec, which makes the whole tool block a single cacheable chunk. The cache keys on a byte-exact prefix, but two of the segments feeding that block were iterated straight out of Maps — plugin load order for the native tool registry, `created_at` row order for domain tools — with no sort anywhere. Stable within one process, divergent across Fly machines and across deploys: a silent, signal-free cache miss for the tool block and everything after it. - new `toolOrdering.ts`: `compareToolNames` (locale-pinned `localeCompare(b,'en')` so the result does not depend on the host's LANG/LC_COLLATE), `sortByToolName`, `sortBySpecName`, `normalizeDiscoveredToolOrder` - `buildToolsList()`: native + domain segments sorted; the deliberate fixed-literal prefix (memory, knowledge-graph, ...) keeps its existing order - `ToolDispatchService.listDispatchableToolSpecs()`: sorted, so the loopback server and the CLI bridge inherit it - `LoopbackMcpServer` tools/list: sorted independently, because `deps.tools` is caller-supplied - `resolveSubAgentTools()`: sorted (grants arrive in `created_at` order) - `setMcpDiscoveredTools()`: normalizes by name before persisting, so a server that returns `tools/list` in a different order each call stops churning the JSONB column and any grant-epoch diff derived from it Ordering is advertisement-only. Collision resolution is unchanged — native tools still win a duplicate name, decided by Map insertion and never by array position — and that is now pinned by a test whose colliding name deliberately sorts last. W1-2 — make the loopback MCP server stateless. `sessionIdGenerator: undefined` is the SDK's stateless mode: no session id is issued and no session validation is performed, so the CLI bridge needs neither the `initialize` handshake nor `Mcp-Session-Id`. The previous comment claiming session ids "remain required by the protocol" was wrong. SDK 1.29.0 enforces the other half of that contract — a stateless transport throws "Stateless transport cannot be reused across requests" on its second use. The MCP server + transport pair is therefore built per request (matching the SDK's own stateless example) and torn down in a `finally`. Both are in-memory handler tables with no I/O, and this server sees a handful of requests per CLI turn. `enableJsonResponse` stays on, which also guarantees the response is fully written before teardown. Non-POST is now declined with 405, which the MCP spec explicitly allows for the optional GET SSE stream. Without it the per-request transport leaks: a GET stream never ends, so `handleRequest` never resolves and the request scope never tears down. Under the old stateful transport a session-less GET was rejected with 400, so nothing regresses. Tests were written first for W1-2 and observed to fail (HTTP 400) before the production change. The wire test is parameterized over replaying vs never sending the session header, plus a case with no `initialize` at all. The 401 `-32001` body and the 413 oversized-POST case still hold. Deliberately NOT implemented: the `ttlMs` tool-list cache also proposed in #545. Its premise is false — `subAgentToolHydration` reads `mcp_servers.discovered_tools` and never calls `listTools`, so steady state is already ~4 wire calls per server per day — and it would re-advertise removed or repurposed tools inside exactly the window the #454 scan-verdict gate exists to close. MANUAL VERIFICATION REQUIRED BEFORE MERGE: the loopback server's only consumer is the Claude CLI bridge, spawned with `--strict-mcp-config --mcp-config <path> --allowedTools mcp__omadia__*`. If the installed CLI refuses to proceed when the server issues no `mcp-session-id`, the bridge yields a server with ZERO tools and the turn silently degrades to a toolless answer rather than erroring — no automated test catches that. A pass against the real `claude` CLI with the stubbed `createLoopbackServer` bypassed is needed. Not performed here: spawning a nested `claude` session is blocked in this environment. Installed CLI version on this machine is 2.1.220. * test(mcp): first real McpManager round-trip against a live MCP server Nothing in the repo proved the client could complete an initialize → tools/list → tools/call sequence: mcpCallAudit dials a refused port, mcpRescan stubs listTools, and the cliBridge tests stub the server. These drive a live in-process LoopbackMcpServer, sometimes through a recording proxy that injects one transport-level failure, so retry and pool behaviour are observed rather than inferred: - listTools + callTool succeed over the wire; a second call reuses the pool - a successful call is audited as ok (previously uncovered) - a genuine Unauthorized surfaces immediately: exactly one POST, exactly one pool invalidation (fails if -32001 is classified transient again) - the shipped once-retry still fires exactly once for -32000 and then succeeds, dropping the pooled connection once - a stale token invalidates the pool and the next call reconnects * feat(mcp): mark the legacy HTTP+SSE transport deprecated (MCP 2026-07-28) MCP 2026-07-28 reclassifies the legacy HTTP+SSE transport as Deprecated with a minimum 12-month removal window. Discourage 'sse' for NEW registrations while keeping every existing SSE server fully working — no protocol work, SSEClientTransport stays wired and the DB CHECK is untouched. - DEPRECATED_MCP_TRANSPORTS + isDeprecatedMcpTransport in mcpClient.ts as the single source of truth, re-exported from @omadia/orchestrator. - mcpNode() gains an additive transportDeprecated flag derived from it (no migration); exported for unit tests. - Marketplace import path (the second way an sse row can be minted): prefer an http remote when a catalog entry offers both, still allow an sse-only entry but flag it via McpCatalogEntry.transportDeprecated. - web-ui McpServerNode gains optional transportDeprecated; McpTransport keeps 'sse' (published plugin contract stays as-is). Refs #541 * feat(web-ui): gate the deprecated sse transport behind an operator toggle Issue #541 acceptance 4 + 6. http (Streamable HTTP) stays the default and the only remote option shown; 'sse' appears in the picker only after ticking 'Show deprecated transports', labelled '(deprecated)'. Existing sse rows get a Deprecated badge in the transport column with a hint pointing at Streamable HTTP as the migration target. Nothing is hard-blocked: the MCP removal window is at least 12 months, so an operator can still deliberately register a legacy SSE server. i18n: adminMcp.servers.{transportDeprecated,transportDeprecatedHint, showDeprecatedTransports,deprecatedOption} in both en.json and de.json. Refs #541 * test(mcp): cover the sse deprecation flag, catalog preference, and regression - mcpRegistryClient: prefers http when a catalog entry offers both remotes, still imports an sse-only entry (flagged), and the preference cannot bypass the untrusted-remote guard; the pre-existing sse fixture now also asserts transportDeprecated. - mcpNode: transportDeprecated true for sse, false for http/stdio. - Regression: an sse config still yields a real SSEClientTransport (and http a StreamableHTTPClientTransport) — the unit discourages, it does not remove. Refs #541 * test(web-ui): cover the deprecated-transport toggle and badge; changelog Issue #541 acceptance 8 + 9. First test file for the MCP Control Center page: the picker offers only http/stdio by default, exposes 'sse (deprecated)' after ticking 'Show deprecated transports' and lets it be selected (never hard-blocked), resets to http when the toggle goes off, badges existing sse rows and falls back to the local list when an older middleware omits the flag. Refs #541 * refactor(tasks): extract a generic long-running task seam Lift the shape devJobOrchestratorTool.ts hand-rolled (start returns a handle at once, work runs detached, a card streams into the turn) into a reusable seam so any tool can be marked longRunning. - taskTypes.ts: TaskDescriptor + TaskStore/TaskReadStore, with a status vocabulary (working | input_required | completed | failed) chosen to project mechanically onto MCP Tasks later. - inMemoryTaskStore.ts: reference implementor of the claim/lease and terminal-transition semantics. - longRunningTool.ts: defineLongRunningTool() -> the non-blocking <tool>_start / _status / _list triple + pending card buffer. - taskReaper.ts: orphan sweep (abandoned live tasks, accumulated terminal tasks). - subAgentTaskTool.ts: deferred sub-agent dispatch as second consumer. No MCP Tasks protocol handlers: internal LocalSubAgent dispatches never cross an MCP boundary, and SEP-2663's tasks/update is unshipped. * feat(mcp): add CIMD as a third client-acquisition mode alongside manual Client ID Metadata Documents (issue #546) become the third link of an explicit acquisition chain: stored -> cimd -> dcr (deprecated, warns) -> manual. Corrects the issue's premise: the OAuth 2.1 + PKCE stack already shipped in epic #459 W9, so this is a delta on it. CIMD replaces Dynamic Client Registration at MCP-native brokers only -- Entra ID and Okta do not support it and keep using the existing manual path, which has no sunset. DCR is kept working and merely warns. - migration 0032: 'cimd' in the registered_via CHECK set + client_metadata_url - GET /.well-known/omadia-mcp-client, allowlisted via the shared constant - 501 (not 500) when FLOW_PUBLIC_BASE_URL is unset -- CIMD needs INBOUND https reachability, so a firewalled install degrades to manual instead of breaking - SSRF guard reuses assertPublicHttpsUrl on the metadata probe - describeAuth gains acquisitionMode / cimdSupported / cimdBlockedReason * refactor(dev-platform): make dev_job the task seam's first implementor Additive adapter (new file only): projects DevJobStore onto the generic TaskStore seam - ten-value DevJobStatus down to the four-value MCP-Tasks-shaped vocabulary, dev_job_events onto the seam's event tail, claimNextQueued onto claimNextPending, and finalizeDevJob onto finish so the brand-gated terminal choke point is preserved. Zero edits to devJobStore.ts / devJobOrchestratorTool.ts and no migration: dev_job_start still returns {"status":"job_started",...} so the web-ui card parser and existing tests are untouched. Keeping this in its own file also keeps the in-flight dev-platform plugin extraction (PR #536/#538) to a file move rather than a conflict. Documents the one intentional divergence: the seam's finish() is lease-fenced, dev_job's finishTerminal deliberately is not (cancel routes and the reaper finalize jobs they never claimed), so the adapter accepts a matching lease OR no lease, and rejects a mismatched one. * feat(mcp): park input_required tool calls (MRTR, #544) Extend the lenient CallToolResult schema with resultType + inputRequests, both readable off the shipped SDK 1.29.0. An input_required result parks the call in a new PendingMcpInput store keyed on the {userId, sessionId, correlationId} triple and returns a stable sentinel to the model instead of a result: no retry attempt consumed, no failure audit row. The MCP call audit gains a three-valued outcome so a parked call is neither reported as a failure nor as a delivered result. Rides W1-3's McpSidecarKind union, which was shaped for exactly this second member. * test(mcp): cover the CIMD acquisition chain, metadata endpoint, SSRF guard, migration 0032 - strategy chain: stored beats cimd beats dcr beats manual, each asserted via an OBSERVABLE consequence (which client_id reaches the provider, what was persisted) rather than a mock call count - CIMD skipped when the AS does not advertise support, when no metadata URL is configured, and when the document is not inbound-reachable - metadata endpoint shape, 501 without FLOW_PUBLIC_BASE_URL, and a direct assertion that served redirect_uris === McpOAuthService.redirectUri - SSRF rejection of loopback / RFC1918 / link-local / non-https metadata URLs, including that no request leaves before the guard runs - publicPaths asserted against the shared CIMD_METADATA_PATH constant - migration 0032 pg test isolated in a dedicated tenant SCHEMA (never a scratch database: CREATE/DROP DATABASE is cluster-wide and cancels concurrent suites) * test(tasks): pin the seam's claim/lease, non-blocking, card and privacy invariants 31 tests across the reference store and the registration helper. Each invariant was verified by deliberately breaking it, rebuilding, and confirming a real assertion failure: - M1 lease fence removed -> claim+lease suite fails - M2 reaper keyed on heartbeat only -> never-claimed-orphan test fails - M3 task input copied onto a card -> privacy invariant test fails - M4 takePendingCards stops draining -> card test fails - M5 terminal immutability guard removed -> zombie-worker test fails M5 initially SURVIVED: finish() cleared the lease, so the lease check alone carried terminal immutability and the guard was unreachable. Fixed properly rather than by weakening the claim - reapOrphans now PRESERVES claimedBy, which is the correct semantics (the reaper is not the owner, and a zombie worker waking up after being reaped legitimately still holds a matching lease). The guard is now the only thing rejecting it, and the new zombie-worker test fails without it. * fix(test): pin the pg tenant schema as a connection option, not a SET statement A pool-level 'SET search_path' binds only the one pooled client that served it, so the next query lands on a different client and silently resolves against public. Two assertions failed on the first real run because of it. The search_path is now a connection option on a dedicated pool, with a separate admin pool for CREATE/DROP SCHEMA. * feat(agents): wire deferred sub-agent dispatch through the task seam The second real consumer, proving the seam is general. A slow sub-agent delegated from a chat turn currently blocks that turn for as long as its LocalSubAgent loop runs; opting it in registers ask_<slug>_start / _status / _list alongside the unchanged blocking ask_<slug>. - Opt-in via LONG_RUNNING_SUBAGENT_TOOLS (no DB column: that needs a migration and 0031/0032 are taken by parallel units). Empty default leaves every sub-agent on today's inline path. - The Askable handed to the seam is the DomainTool's own handle, so the deferred and inline routes share one dispatch path and cannot drift. - Orphan reaper started at boot when the feature is on. Tests drive a REAL LocalSubAgent (only the LLM wire is stubbed) with the provider latched, so 'did the turn block?' is decidable rather than a timing guess. Mutation M6 (await the runner in _start) fails 12 tests. * feat(web-ui): surface the CIMD acquisition mode and keep the manual path first-class - badge per acquisition mode (metadata document / auto-registered / registered app), with tooltips explaining each - diagnostic when the AS supports CIMD but this deployment cannot publish a document it can fetch, naming the reason and pointing at the manual client - copy states plainly that the manual client is the standard Entra ID / Okta path by design, not a workaround pending CIMD - adminMcp.auth.* keys added to BOTH en.json and de.json (no i18nexus here) * docs(mcp): record the CIMD decision, deployment requirement, and changelog entry ADR-0006 states plainly that CIMD needs INBOUND https reachability, that a byte5-hosted metadata relay is not offered by default (it would make every customer's client_id identify byte5 to that customer's IdP), that manual client registration remains the supported enterprise path for Entra ID and Okta, and that single tenancy is the reality -- no migration defines a tenant column. * test(dev-platform): seam-conformance suite for dev_job on real Postgres 13 pg tests driving dev_job's real claim/lease, event tail, and brand-gated terminal write through the generic TaskStore interface, mirroring the assertions devJobStore.pg.test.ts already makes on the same operations. Verified green against a dedicated pgvector container. Tenant isolation: MARK carries a fresh UUID per run, so a concurrent dev-platform pg suite in the same cluster cannot see or delete these rows - no CREATE/DROP DATABASE (cluster-wide) and no reliance on the database-wide migration advisory lock being free. Mutation M7 (drop the adapter's mismatched-lease rejection) fails the fence test. * feat(web-ui): generic long-running task card + i18n, and changelog - taskChatCardState.ts: tool-agnostic parser for the seam's {"status":"task_started",...} handle, plus a label helper. Reuses the existing delivery mechanism (the tool_use/tool_result pair already reaches the web-ui) so no new stream event is needed. - TaskChatCard.tsx: minimal card stating what started and that the answer arrives separately. Carries NO result - that would route it around the Privacy Shield, since cards never pass through dispatchTool. - chat/page.tsx: dev_job_start is matched FIRST so it keeps its richer, gate-capable card; any other <tool>_start falls to the generic one. - chat.task.* strings in BOTH en.json and de.json; i18n:check OK (3271 keys, no new warnings). Mutation M8 (drop the task_started guard so job_started also parses) fails the 'dev_job_start keeps its own card' test. * test(mcp): mutation-verified coverage for the input_required park (#544) 40 tests over the store, the inputRequests parser, the reply envelope and a real fake MCP server that answers input_required. A temporary mutation harness (scripts/w2-1-mutation-check.mjs, deleted before push) breaks each invariant, rebuilds and re-runs: 9 of 10 mutations turn a real assertion red. The 10th survivor is a finding, not a gap: SDK 1.29.0's CallToolResultSchema derives from a passthrough ResultSchema, so resultType/inputRequests already survive parse without our extension. Corrected the schema doc comment, which claimed the opposite, and added a characterization test so a future SDK that tightens passthrough fails loudly instead of killing MRTR silently. * fix(test): stop the dev_job orphan sweep from polluting sibling pg suites DevJobStore.findStalled is DATABASE-GLOBAL - no tenant predicate - so a real sweep with a forward-dated cutoff finalized OTHER suites' in-flight jobs as 'stalled' and broke devPlatformPipeline.wire.pg.test.ts. That is correct production behaviour (the real reaper IS global) and an untenable pg test, so the sweep moves to where it can be isolated: - test/tasks/devJobTaskStoreReap.test.ts drives it against a controlled fake findStalled, and additionally pins the day-rounding (a retain window under a day must clamp to 1, not round down to 0 and throw) and that a refused finalize is not counted as a reap. - The pg suite keeps a comment explaining why the sweep is absent. Verified: all 11 dev-platform pg suites now pass together (94/94) when run sequentially. Running them in PARALLEL still fails broadly - that is the pre-existing database-wide migration advisory-lock race, reproducible on main and unrelated to this change. * feat(orchestrator): short-circuit the turn on a pending MCP input request (#544) Rides the ask_user_choice drain: drainPendingMcpInput() is a sibling of drainPendingChoice() on the same code path, and pendingUserChoice is the deterministic winner when both are pending in one batch (documented, and the losing MCP record stays replayable rather than vanishing). The card answer arrives as a machine envelope which runTurn/chatStream normalise away before any downstream reader sees it, then replay as an orchestrator-driven forced tool call — the arguments are already known exactly, so leaving the re-call to the model could only make it wrong. ChatTurnResult and the done event gain pendingMcpInput as a SIBLING of pendingUserChoice (free-text fields, not 2-4 buttons), plus an OutgoingMcpInputForm; toSemanticAnswer also folds a plain-text prompt into text so a channel with no form support cannot silently swallow the request. Every surface names the asking server. * feat(mcp): wire the pending-input store and replayer into the kernel (#544) The McpManager (kernel index.ts) writes parked records, the Orchestrator (orchestrator plugin deps) reads them, and neither can reach the other's construction site — so the single process-local instance lives in pendingMcpInput.ts behind two accessors, the same module-singleton shape mcpGrantPolicy.ts already uses. The replayer is registered from index.ts because that is the only place holding both the manager and the server registry: a replay re-resolves the server's live config rather than replaying a snapshot, and a server deleted between the two turns surfaces as "no longer reachable" instead of silently dropping the user's input. buildOrchestrator enables the path only when store AND replayer are present. * fix(mcp): bind the parked-record owner at claim time, not at park time Found while writing the orchestrator tests: turnContext is EMPTY inside tool handlers on the streaming path. turnContext.enter() uses AsyncLocalStorage.enterWith inside an async generator, which does not propagate into the generator's own continuations — verified with a probe on both the buffered and streaming paths. That is pre-existing (it also affects mcpCallerKind/mcpUserKey attribution on every chatStream turn) and W2-1 must not lean on it. So the manager now parks WITHOUT an owner and returns a sentinel carrying the correlation id; the orchestrator — which holds the turn input reliably on both paths — reads that id out of the batch's tool results and claims the record, binding {userId, sessionId} at that moment. The linkage is the tool result itself, exactly the mechanism extractToolEmittedChoice already uses. Security is strengthened, not weakened: an unclaimed record is replayable by nobody, claim is single-shot so a leaked sentinel cannot re-bind ownership, and a wrong-owner take() misses WITHOUT consuming the rightful owner's card. * test(orchestrator): cover the buffered path and close both mutation survivors The mutation harness caught a real coverage gap: chatInContextInner carries a hand-mirrored copy of the short-circuit, the winner rule and the envelope normalisation, and breaking ONLY that copy left the whole suite green — the streaming tests do not exercise it. Non-streaming callers (Teams, every chat()/runTurn() consumer) go through exactly that code. Added buffered-path coverage (short-circuit, winner, replay round trip, cross-session refusal) and taught the fake provider to serve complete() from the same script as stream(), so one scenario covers both paths. 16/16 mutations now detected, no survivors: 14 turn a real assertion red, 2 are compile-time guards. * feat(web-ui): MCP mid-call input form, i18n, changelog (#544) New McpInputCard next to ChoiceCard — a sibling, not a variant: N free-text fields demanded by a third-party server rather than 2-4 buttons the model chose. The asking server is named in the heading and again in an explicit warning, its own prose is rendered quoted and attributed so untrusted text cannot read as omadia's copy, and a secret field says plainly that the value still reaches the server as entered. The one duplicated constant (MCP_INPUT_REPLY_PREFIX, unavoidable because web-ui does not depend on the middleware packages) is pinned by a contract test that reads the web-ui source and feeds the card's exact envelope to the real parser — drift would otherwise be silent and total. Mutation run found the stale-affordance strip in chat/page.tsx completely uncovered, including its pre-existing pendingUserChoice half; extracted it as stripStaleInteractives() and tested it. 8/8 web-ui mutations now detected. i18n in en.json AND de.json, i18n:check clean. Changelog documents the two-turn design, the stdio replay limitation, the audit outcome widening and what is out of scope. * feat(orchestrator): idempotency primitive for write-capable tool dispatch Adds the process-local dedupe store plus an AsyncLocalStorage channel that carries the active idempotency key down to the MCP transport layer without touching the published NativeToolHandler contract. Also adds isWriteCapableTool() to the existing (previously unwired) WriteCapability contract in plugin-api. * feat(orchestrator): close the privacy/trace seam in ToolDispatchService and add idempotency Task 1 — ToolDispatchService now applies the privacy data-plane boundary (intern-exemption, operator bypass + receipt, internToolResultV4 masking) and raw-result capture in the same order as Orchestrator.dispatchToolDeadlined, so a caller reaching tools via the loopback/public path no longer bypasses the PII masking the chat path enforces. Adds an optional caller-context carrier (principal/scopes/tenantId/userId/requestId) propagated ambiently. SEAM comment rewritten to state what is closed and what stays orchestrator-only. Task 2 — write-capability is declared via the existing WriteCapability contract, now wired onto its intended non-model-facing carriers (NativeToolRegistration, DomainTool). A write-capable dispatch with an idempotency key dedupes on the key and clamps McpManager.callTool to a single attempt; reads keep the flaky-proxy retry unchanged. * test(orchestrator): mutation-checked privacy masking on the dispatch path 11 tests asserting masked CONTENT, never call counts. Verified empirically by breaking three invariants and confirming failures: masking removed (5 fail), intern-exemption dropped (1 fail), raw capture fed the masked value (1 fail). * fix(orchestrator): propagate turnContext into the streaming tool loop `chatStream` established the turn scope with `turnContext.enter` (`AsyncLocalStorage.enterWith`). That binding survives only until the generator's first real suspension: from the first yielded event onward every continuation is resumed in the async context of whoever called `.next()`, so the store is gone. Anything read BEFORE the first yield (the privacy handle for prompt masking) accidentally worked; everything read at or after tool dispatch did not. Consequences, all silent and all on every streaming turn (web-ui + every channel): MCP audit rows degraded to `callerKind: 'unattributed'`, `turnId: null`, `callerAgent: null`; `mcpUserKey` was unreachable so `resolveIdentity` recorded `unresolved` and a `per_user` server got no token; the skill-binding persona gate refused every skill-bound MCP tool; chat-launched dev jobs failed closed on a missing `userId`; plugin memory writes landed under the `default` Agent namespace instead of the acting one. - Add `turnContext.runGenerator`, which wraps every advance of an inner generator in `storage.run` — the `run()` equivalent that composes with `yield`. The context value is passed BY REFERENCE per step so documented live-store writes (`activePersonaSkillId`, `mcpInputReplayNote`) keep working, and an abandoned stream's teardown runs inside the scope. - Split `chatStream` into a context-establishing wrapper plus `chatStreamInContext`, and document why `enter` must not be used from a generator. - Carry `mcpUserKey` through the three nested scopes that deliberately inherit the turn but dropped it: both orchestrator entry points, `runWithChatParticipants`, the plugin MCP accessor and the skill-tool hydration wrapper. Tests assert the OBSERVABLE audit row, not just the context object. All six were red before the fix; three deliberate mutations (dropped carry-over, per-step context copy, skipped inner teardown) each turn a real assertion red. * test(orchestrator): mutation-checked exactly-once semantics for write tool dispatch 20 tests. The end-to-end suite drives a real LoopbackMcpServer behind a proxy that forwards the first tools/call upstream (the server really executes) and only then loses the response — the shape the transport retry cannot distinguish from a pre-execution failure. Side effects are counted on the server itself. A CONTROL test proves the hazard is real (2 writes without a key) alongside the protected case (1 write with one). Verified by breaking three more invariants: retry clamp removed (1 fail), dispatch dedupe bypassed (4 fail), every tool treated as write-capable so reads lose the mitigation (4 fail). Records a dual-module-graph gotcha: mixing dist and src imports yields two AsyncLocalStorage instances and the scope silently never arrives. * fix(orchestrator): make the tool-dispatch and MCP timeout bounds coherent `OMADIA_TOOL_DISPATCH_TIMEOUT_MS` defaulted to 120 s, which sits INSIDE the MCP call ceiling (60 s idle budget per request, 180 s absolute via `OMADIA_MCP_CALL_MAX_TOTAL_TIMEOUT_MS`). The outer schranke was therefore tighter than the inner one: an MCP-backed sub-agent legitimately streaming progress notifications for its full 180 s allowance was aborted by the per-tool dispatch deadline first, and the model saw a generic dispatch-deadline error instead of the MCP layer's own diagnosis — with no `mcp_call_log` failure row naming the slow server. - Raise the dispatch default to 240 s so the outer bound is genuinely looser. - Document the three-level ordering next to BOTH defaults, each pointing at the other and at the guard test. - Export `resolveToolDispatchTimeoutMs` and a new `resolveMcpCallTimeouts` so the invariant is asserted against the real resolvers (env overrides included) rather than copies of the literals; `callTool` now uses the same helper. - Add `test/orchestrator/timeoutHierarchy.test.ts` asserting `dispatchDeadline > mcpAbsoluteCeiling > mcpRequestBudget`, plus the shipped numbers so lowering both together is not silently green. Mutation-checked in both directions: restoring the 120 s dispatch default and independently raising the MCP ceiling to 300 s each turn the ordering assertion red with the exact diagnosis. * test(embeddings): stop the gate-fence fixture from reaching into public `embeddingGateWriteFence.pg.test.ts` failed on every with-pg run after the first. Not the repo's known flake pattern (which produces disjoint failure sets) — a deterministic fixture bug, and the FK that surfaced it was luck. `freshSchema()` ran `DROP TABLE IF EXISTS graph_nodes, processes, process_history, graph_embedding_model` UNQUALIFIED. A DROP resolves an unqualified name through the search_path, and this suite is the only one in the cluster whose pool has `public` on it (it needs `public.vector` for the bare `::vector` casts the writers emit). On the first test its own schema is still empty, so every name fell through to `public`: - run 1, pristine database: `public.graph_nodes` does not exist yet → no-op → suite green. - run 2+: the real KG suites have since created `public.graph_nodes` and `public.graph_edges`, which survive in the container. The DROP now hits `public.graph_nodes` and errors 2BP01 on `graph_edges`' foreign keys, so the CREATE never runs, so the next test's DROP falls through again — self-perpetuating across all six tests. The FK is the ONLY reason this was loud. `public.processes` and `public.graph_embedding_model` have no dependents, so the same fall-through was silently dropping sibling suites' tables. - `freshSchema` now recreates the SCHEMA instead of enumerating tables, so it can never name an object outside its own namespace. - All DDL, DML and assertion reads are schema-qualified; an unqualified READ would not error, it would quietly assert against another suite's rows. - Added `assertFixtureIsIsolated()` after every fixture build, turning a future search_path regression into an immediate named failure. `embeddingGateReevaluation`, `embeddingModelGateMigration` and `embeddingModelGateMigrationGuards` use `search_path=<own schema>` with no `public`, so they cannot fall through and are left unchanged. Mutation check: restoring only the unqualified DROP against the same dirty container reproduces 6/6 failures with the identical 2BP01 error; the fix takes it back to 6/6 green. * feat(mcp): add mcp:list / mcp:invoke / mcp:write:<tool> scopes and the public-MCP key binding migration Per-tool write scopes are unreachable via WILDCARD_SCOPE — the exception lives inside hasScope itself so a parallel matcher cannot be forgotten. Migration 0033 adds public_mcp_key_bindings (allowlist per KEY, one agent per key) and widens mcp_call_log.caller_kind with 'api_key'. * fix(orchestrator): remove a literal NUL byte and disambiguate the idempotency cache key The cache-key template carried a raw NUL (git classified the source file as binary). Replaced with a length-prefixed ASCII composition, which also closes a real collision: a naive `${toolName}:${key}` maps both ("a:b","t") and ("b","t:a") to the same entry, letting one caller key replay another tool's stored write result. Regression test added and mutation-verified. * feat(plugin-api): expose writeCapabilities on the plugin-facing tool accessor ToolRegistrationOptions gains writeCapabilities and the kernel shim forwards it on both register() and registerHandler(). Without this hop only kernel-internal registrations could declare themselves write-capable, so every real plugin (Odoo, M365) would have stayed unprotected while the unit tests passed. Test walks the actual ctx.tools.register shim; mutation-verified by dropping the forward. * feat(mcp): PublicMcpServer, per-key binding store and the /api/v1/mcp router New class rather than a flag on LoopbackMcpServer: the loopback's trust boundary (any local process that can read the 0600 bearer) does not transfer. Per-request Server+transport with sessionIdGenerator: undefined torn down in a finally, 405 on non-POST, 8MB cap, per-tool timeout, concurrency ceiling. tools/list returns exactly the callable set so a name is never leaked. * feat(devplatform): make the stalled-job sweep narrowable, and restore its pg test `DevJobStore.findStalled` had no scope predicate at all: `WHERE status IN (provisioning, running, applying) AND COALESCE(last_heartbeat_at, started_at, claimed_at) < $1`. Database-global is CORRECT production behaviour for the single-tenant deployment — the reaper must reach every abandoned job whoever launched it — but it made the sweep untestable in a shared cluster: a forward-dated cutoff in one suite finalized a sibling suite's in-flight jobs as `stalled` and broke `devPlatformPipeline.wire.pg.test.ts`, so the pg-level test was removed and downgraded to a fake-`findStalled` unit test. - `findStalled(cutoff, scope?)` takes an optional `DevJobSweepScope`. `dev_jobs` has no tenant column, so `repo_id` is the tenancy axis: `AND repo_id = ANY($2::uuid[])`. Omitting the scope emits byte-identical SQL to before, so production behaviour is unchanged. - An EXPLICITLY EMPTY `repoIds` returns no rows. It must never widen back to global — that would turn a caller who computed an empty entitlement set into a caller who sweeps everything. - `DevJobTaskStoreDeps.sweepScope` threads it into `reapOrphans` at CONSTRUCTION, not per call, so the generic `TaskReapOptions` contract stays free of dev-platform concepts. - Restored the pg-level sweep test, now with the assertion whose absence forced the downgrade: a sibling repo's in-flight job is left `provisioning`. Setup activates jobs by stamping the columns rather than `claimNextQueued`, which pops the oldest queued row DATABASE-WIDE and cannot be aimed at a repo with concurrent suites running. Kept the fake-based unit test for its driven clock. Mutation-checked: ignoring the predicate, letting an empty scope widen to global, and dropping the forwarding in `reapOrphans` each turn a real assertion red. `devPlatformPipeline.wire.pg.test.ts` passes alongside the restored sweep. * feat(mcp): mount /api/v1/mcp behind publicPaths + requireApiKey, dark by default PUBLIC_MCP_ENABLED=false mounts no router at all. Mounted after the /api requireAuth line so the publicPaths entry is load-bearing: losing it makes the route go dark (401), not open. Widens McpCallerKind with 'api_key' and points McpCallLogRow at the shared union instead of a retyped copy. * test(mcp): 102 tests for the public MCP endpoint against the real middleware chain Harness reproduces index.ts's chain (express.json 10mb, the OB-106 /api requireAuth line, mountPublicMcp, the shared publicPaths array) rather than a bare express() app — the epic #470 bug publicPaths.ts documents. Also rejects the bare 'mcp:write' scope, which validated and granted nothing. * test(mcp): mutation harness for the public MCP endpoint 27 mutations that each break one invariant with a real source edit and require a real assertion failure. A mutation that leaves the suite green means the invariant is untested — the check fails in that direction. * docs(mcp): correct the pending-input rationale that cited the turnContext defect `pendingMcpInput.ts` justified its two-phase park/claim design by asserting, as a standing repo property, that `turnContext.current()` is `undefined` inside a tool handler on every `chatStream` turn. That was true when written and is no longer: the streaming entry point now establishes the scope via `turnContext.runGenerator`. Records the fix and keeps the two-phase design deliberately, with the reason restated so it no longer rests on a defect: claim-time binding does not depend on ambient context being correct at park time, so it stays robust for dispatch surfaces that legitimately have no turn scope (the standalone `ToolDispatchService`, a public endpoint). Reading the owner from ambient context would be a regression in robustness, not a simplification. Comment-only; no behaviour change. * feat(mcp): consume the landed privacy seam and fail CLOSED for public callers Merged feat/w3-b-dispatch-privacy-seam-and-idempotency. The endpoint runs outside any turn, so it supplies the privacy dependency EXPLICITLY (the ambient turnContext fallback is undefined there) and closes all three fail-open paths the seam left at chat-path parity: 1. masking throws -> the gate returns a placeholder instead of rethrowing, so the dispatcher's fail-open branch never returns raw rows; the endpoint then discards the result and refuses. 2. operator per-plugin privacy bypass -> pinned off; it does not extend to an anonymous HTTP caller. 3. intern-exempt tools (memory, read_attachment, ...) -> never servable, since the dispatcher checks that exemption before consulting the handle. Plus: no privacy provider installed refuses the call (PUBLIC_MCP_ALLOW_WITHOUT_ PRIVACY_MASKING is the documented escape hatch); write capability is the UNION of isWriteCapableTool's declaration and the operator's write_tools, so a mistake in either direction fails toward 'treat it as a write'; ToolDispatchCallerContext carries the API-key principal; _meta.idempotencyKey is forwarded. 117 tests pass, including PII assertions against the real ToolDispatchService. * docs(mcp): external-consumer README, CHANGELOG entry, and acting-identity in the call-log UI README states plainly what a public consumer can and cannot rely on: masked results, the wildcard-excludes-writes rule, and that _meta.idempotencyKey is process-local retry safety rather than distributed exactly-once. Surfaces mcp_call_log.acting_identity in the admin UI for every row — it had been recorded since the column landed but never displayed, which left 'whose credentials touched that server?' unanswerable in the UI. i18n in en + de. * fix(mcp): restore the per-tool timeout, leaked as disabled by the mutation harness 84d73ab8 committed `return 2_147_483_647` for PublicMcpServer.toolTimeoutMs: a `git add -A` raced the mutation harness and captured a mid-mutation file, so the per-tool timeout shipped DISABLED inside a docs commit. Found by auditing every mutation target against HEAD rather than trusting the harness's own revert. Adds two guards so it cannot recur: the harness refuses to start against a dirty tree, and it fails loudly (exit 3) if any target file is still modified when it finishes. Also counts a CANCELLED test as CAUGHT — removing the concurrency ceiling deadlocks its test instead of failing an assertion, which the previous grep read as 'invariant untested'. * test(mcp): correct three mutations that were passing for the wrong reason - statelessness: needs THREE edits to actually share the transport (slot + memoized assignment + no teardown). The one-line version only deleted the teardown, which leaks without causing reuse, so the suite stayed green and the headline invariant read as untested. Verified: 39/51 tests fail, including 'answers two sequential tools/call requests'. - null tool list: '!Array.isArray(null)' already denies, so skipping the null branch changed nothing. Now replaces its RESULT with an empty grant. - per-tool timeout: re-anchored on the getter after the seam refactor moved it. * style(mcp): drop the now-unused ApiKeyScope import Left over from PublicMcpCallerContext, which the landed ToolDispatchCallerContext replaced. * fix(tasks): stop stranding claims, losing outcomes and reaping human-parked tasks Three defects in the long-running task seam, all found by adversarial review of PR #550. They live in the same 20-line runner and the same store, so they ship together. 1. CROSSED CLAIMS STRANDED TWO TASKS FOR 15 MINUTES. `claimNextPending(lease, kind)` returns the OLDEST unclaimed task of that kind, not the one the runner was spawned for. With two same-kind tasks in one turn, B's runner claimed A, saw the id mismatch and returned WITHOUT releasing the claim; A's runner did the same to B. Both sat `working` under live-but-dead leases with no executor until the reaper failed them. Fixed on two levels: `claimNextPending` gains an optional, ADVISORY `taskId` hint (`InMemoryTaskStore` honours it exactly; `devJobTaskStore` documents why it cannot and ignores it), and the runner now treats the RETURNED descriptor as authoritative and follows its claim through. A claim it cannot hand back is a claim it must finish — walking away is what strands a task. 2. A SUCCESSFUL RESULT WAS DISCARDED WHEN THE LEASE HAD BEEN REAPED. `finish(…, 'completed')` threw `TaskLeaseLostError`; the generic catch then called `finish(…, 'failed')` on the now-terminal row, which threw again and escaped to `onRunnerError`. A task that genuinely succeeded was narrated to the caller as the reaper's generic "task abandoned" and its result vanished. The completion path gets an explicit lease-loss branch reporting through a new `onOutcomeLost` hook. The row is NOT overwritten: terminal immutability is what stops a zombie worker from clobbering a new owner's outcome, so the outcome is surfaced rather than forced. The default handler withholds the payload — a task result may carry PII and the log is outside the shield. 3. THE REAPER FORCE-FAILED TASKS LEGITIMATELY WAITING ON A HUMAN. `requireInput` releases the lease and freezes `lastHeartbeatAt`, nothing heartbeats a parked row, and the sweep treated every non-terminal status as live — so answering an `input_required` card after 16 minutes landed on a task already marked `failed`. `input_required` is now excluded from the worker-liveness sweep and expires only under an explicit, opt-in `parkedStaleAfterMs`, aged from `updatedAt` rather than the frozen heartbeat. (dev_job never had this: `findStalled` sweeps only provisioning|running|applying, never the `waiting` gate state.) Also here, same subsystem: - `startTaskReaper` no longer re-enters a sweep that is still running. Harmless against the in-memory store; a Postgres implementor would stack concurrent transactions on the same rows. - Corrected two comments that claimed more than the code does: "every write is lease-fenced" (`reapOrphans` is deliberately unfenced, and that is WHY the terminal guard is separate from the lease check) and the "hard ceiling" on retained tasks (only terminal rows are evictable, so it is a soft one). Tests drive the real interleaving and the real reaper — no mock-call counting. * fix(mcp,orchestrator): count the retry in the timeout invariant and enforce it in production The W3-A ordering invariant (outer dispatch deadline > absolute MCP ceiling > per-request idle budget) was defeated two independent ways. (a) THE INVARIANT WAS STATED PER-ATTEMPT, ENFORCED AGAINST A TWO-ATTEMPT REALITY. `McpManager.callTool` retries once on a transient transport failure, and each attempt got a FRESH `maxTotalTimeout` — so one dispatch was worth up to 2 x 180s = 360s of wall clock against a 240s outer deadline. That is exactly the inversion W3-A was written to remove, re-created by a knob nobody counted. Fixed on the MCP side rather than by inflating the outer bound: the attempts now SHARE one absolute budget, so `maxTotalTimeout` means what its name says. Attempt 2 runs with the remainder, the per-request idle budget is clamped to it too, and no retry is started once the budget is spent (a call that consumed its whole ceiling is not "transiently flaky"). `resolveMcpCallTimeouts` reports a `worstCaseTotalMs` so the invariant is asserted against the real worst case. (b) THE INVARIANT WAS NEVER ENFORCED OUTSIDE A TEST. `assertOuterIsLooser()` existed only as a local helper in test/orchestrator/timeoutHierarchy.test.ts; nothing shipped ever called it, and `resolveToolDispatchTimeoutMs` accepts any non-negative value — so OMADIA_TOOL_DISPATCH_TIMEOUT_MS=90000 re-created the inversion with fully green CI, surfacing much later as MCP calls dying on a generic dispatch-deadline error. The check now lives in production as `assertTimeoutHierarchy()` and runs at boot, so an incoherent deployment refuses to start. `resolveToolDispatchTimeoutMs` is deliberately left PURE rather than clamping to a coherent value: silently substituting a number the operator did not choose hides the misconfiguration, and a short deadline is legitimate for a deployment that also lowers the MCP ceiling. Startup is where an incoherent pair is refused, and startup is when the operator is looking. The test suite only ever exercised RAISING the inner ceiling. It now also exercises LOWERING the outer deadline — the likelier operator action, and the one that produced the original 120s inversion — plus the retry-doubling case and an MCP ceiling below its own per-request budget. Also in orchestrator.ts: corrected the late-result firewall's comment, which overstated its reach. The guard sits AFTER `dispatchToolInner` returns, so MCP mutations, mcp_call_log rows, KG and memory writes, and a sub-agent's dataset registrations on the turn's privacyHandle have already landed. A 241s knowledge_graph write IS in the graph while the model is told the call was aborted and its result discarded. What is guaranteed is narrower: the late result never enters TURN state and the model never sees it. * fix(orchestrator): a failing turn teardown must not replace the exit reason `turnContext.runGenerator` drives the inner generator's own `finally` blocks (steering-bus teardown, privacy finalisation) inside the turn scope when a consumer stops early. That drive — `await inner.return(undefined)` — sat bare inside the wrapper's OWN `finally`, and an abrupt completion in a `finally` REPLACES the pending completion of the whole generator. So a throwing privacy finaliser overwrote the client abort that actually triggered teardown: the caller was handed a secondary teardown failure and the real reason, the one worth debugging, was gone. The teardown failure is now caught and never allowed to become the completion. It is not swallowed either — it goes to `onTurnTeardownError`, an overridable reporter (default: `console.error` naming the turn and saying explicitly that the original exit reason was preserved), so real error reporting can route it and a test can assert on it instead of scraping console output. A reporter that itself throws cannot become the exit reason either. The original property is unchanged and re-pinned by test: teardown still runs INSIDE `storage.run`, or the finaliser would run context-less — the very bug this helper exists to prevent. * fix(orchestrator): bound in-flight idempotency entries instead of pinning keys forever An in-flight entry in `ToolIdempotencyStore` was exempt from BOTH expiry and eviction, with no upper bound at all. The reasoning — "it never expires out from under its own execution" — holds only while the execution finishes. A handler that hangs forever (the exact failure the dispatch deadline exists for, and the deadline resolves the SLOT; it does not make the underlying promise settle) pinned its key permanently: every later call under that key awaited a promise that never resolved, and the entry could not be evicted, so the map grew past `maxEntries` unchecked. Past its TTL an in-flight entry is now treated exactly like a stale completed one — no longer live, so it neither blocks its key nor survives eviction — and `evictOverflow` also runs on the in-flight path, which it previously did not (it ran only after a SUCCESSFUL completion, so a burst of hung calls never triggered it). Deleting the map entry does not cancel the execution; nothing here can. It stops a stuck tool from also being a stuck key. Consequence handled: two executions can now legitimately exist for one key, so every write in `run()` is conditional on OUR entry still being the one in the map. A blind set/delete would have let an older execution's outcome clobber a newer entry. Also: documented the verdict on `pendingMcpInput.claim()`, flagged in review as missing the owner comparison `take()` performs. It cannot perform one — a parked `PendingMcpInput` carries NO owner fields; binding one at claim time IS how it acquires them, which is the whole point of the two-phase design. Trust-on-first-claim is sound here: the correlation id is a random UUID learned only from the tool result of the call that parked it, inside the same turn's batch, and the property that matters (REPLAY) is enforced by `take()` on the full triple. Behaviour unchanged; the reasoning is now in the file. * test(tasks,mcp): pin the claim hint, the sweep re-entrancy guard and the shared MCP retry budget Direct coverage for three invariants the higher-level tests could not fail on: - `claimNextPending`'s `taskId` hint, asserted at the STORE. The runner has its own defence-in-depth layer (it follows through on whatever it claimed), so a broken filter stays invisible end-to-end — verified by mutation: ignoring the hint leaves the crossed-claim test green. - `startTaskReaper` does not re-enter a sweep that is still running. Mutation check observed 36 concurrent sweeps without the guard. - `McpManager.callTool`'s retry shares the absolute budget rather than getting a fresh one, driven through the real env knob against the existing losing-proxy harness. The CONTROL test in the same file proves that proxy produces TWO attempts when the budget allows, so a green result is the budget doing the work rather than the retry having disappeared. * fix(mcp): mask tool ERROR text at the same boundary as tool results Both dispatch branches of `ToolDispatchService` ran `afterDispatch` — raw capture, intern-exemption, operator bypass, `internToolResultV4` masking — on the SUCCESS path only. A handler that threw returned `error.message` verbatim, and the public MCP endpoint serialized it straight to the caller. Handler exceptions are not sanitized strings. ORMs echo the failing row and drivers echo bound parameters, so a key scoped `['mcp:list','mcp:invoke']` calling a failing Odoo tool could receive `Fault: Invalid field 'x' on record {'id':42,'name':'Jane Doe',...}` over HTTP. The endpoint did not compensate: `maskingFailed()` is false when masking never RAN, and the result was returned regardless. Error text now goes through a narrower `maskErrorText`: intern-exemption plus `internToolResultV4`, deliberately WITHOUT raw capture (its consumers, incl. KG ingest on the chat path, are documented to receive tool RESULTS — a driver stack trace is not one) and WITHOUT the operator per-plugin bypass (consent about a tool's declared output shape does not transfer to arbitrary exception text, and a byte-counted receipt would mis-describe the disclosure). `isError: true` is preserved. Every result also carries a new `origin` field so a consumer can distinguish handler-authored content (must have been masked) from this service's own refusal strings (nothing to mask). Absent ⇒ treat as `'tool'`, i.e. fail closed. The chat path is untouched and deliberately still surfaces raw exception text to the operator; `chatPathToolErrorText.test.ts` fences that divergence, with a control asserting the same configuration still masks successful results. Mutation-checked empirically: dropping the mask call (5 failures), calling the masker but discarding its result (6 failures), reusing the whole `afterDispatch` chain (2 failures), and dropping the intern exemption (1 failure) each turned these tests red. * fix(mcp): enforce `masked()` — a result that skipped the boundary is refused `publicMcpPrivacy.ts` has always exposed `masked()`, documented as "Lets the endpoint assert that the boundary was actually crossed rather than skipped". Nothing ever called it: `rg '\.masked\(\…
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.
Phase 1 of extracting the Dev Platform into an installable plugin. Ships the one capability the extraction cannot be done without, fixes a live bug found while mapping it, and lands the full plan. No dev-platform code moves here — deliberately, so the visible win is not coupled to the riskiest steps.
Plan:
specs/470-dev-platform-plugin/plan.mdWhy this first
The investigation found the plugin API simply cannot express what Dev Platform needs: no nav contribution, no plugin-owned tables, no React-in-a-ZIP, no sidecar declaration. So "extract it as a plugin" is gated on building platform capabilities first. Navigation is the one with no dependency on the migration/auth/service-inversion minefield, and it is the user-visible promise — a plugin that brings its own menu.
What's in it
Nav contribution API.
ctx.uiRoutes.registerNav({ navId, href, cluster?, order?, label }), backed byUiRouteCatalogand served by a session-gatedGET /api/v1/ui/navigation?locale=<l>.Extends the existing
UiRouteCatalograther than adding a third catalog beside it andadmin_ui_path. Nav entries are a separate registration from uiRoute descriptors because the path semantics differ — a descriptor is relative to the plugin's/p/<pluginId>mount, a nav entry is an absolute in-app path. One registration carrying both would make one of the two fields a lie.Labels are resolved server-side for the requested locale and fetched in the root layout, so the shell stays on next-intl's single i18n clock. A client fetch of pre-resolved strings would lag next-intl on a locale switch and render a half-translated nav, plus shift the header after hydration.
Dev Platform is the first consumer. Its menu entry and
/admingrid card now come from that registration instead ofNav.tsx's literal. TurnDEV_PLATFORM_ENABLEDoff and both are gone with no frontend rebuild. When the plugin package lands, the call becomesctx.uiRoutes.registerNav(...)inside itsactivate()and nothing about the shell changes.Bug fix: deactivated plugins kept serving their routes.
ToolPluginRuntime.deactivate()stopped jobs and disposed UI routes but never calledpluginRouteRegistry.disposeBySource(), though it held the dependency and threaded it into every plugin context —DynamicAgentRuntimealready did. Express cannot unmount, so an uninstalled plugin's routers stayed live and shadowed later mounts at the same prefix.This is a prerequisite, not a drive-by: "with the plugin not installed, no dev-platform code paths" is unverifiable while routers outlive their plugin. The regression test fails 3/4 without the one-line fix.
Security posture
Everything a plugin supplies renders inside the shell's trusted header, so it is all treated as untrusted input:
/x/%2e%2e/admin,/admin/,/admin?a=1,/admin#xall navigate to Admin while comparing unequal to/admin. Only unreserved-charset segments, no dot-segments, no trailing slash, no query, no fragment, no percent-encoding. For that subset, string equality and browser resolution agree.Review
Two adversarial passes on the plan before implementation, then two more on the diff (GPT-5.4 and GPT-5.6 via codex). They inverted one of my premises: I had claimed plugin routers get no auth, but
app.use('/api', requireAuth, ...)runs beforepluginRouteRegistry.mountAll(app), so/api-prefixed plugin routes are already gated. The real gap is the inverse — no way to opt out, sincepublicPathsis a frozen literal. That matters a lot for the next phase: naively "deleting the two dev-platform exemptions" would 401 every runner phone-home and kill jobs in flight. It's now a blocking constraint in the plan.The 5.6 pass additionally found the href-aliasing bypass, the missing bounds, dispose-ordering, an activate() rollback gap, hydration-unstable sorting, prefix matching without a segment boundary, and a defensive parser that did not actually enforce its contract. All fixed in the second commit; the plan records what was not fixed.
Test plan
i18n:checkOK (3,183 keys, en/de parity)node:test); 13 merge, 12 parse (vitest)DEV_PLATFORM_ENABLEDagainst a running stack and confirm the menu entry and admin card appear/disappearNote on suite flakiness
Adding test files made
registryInstallMergefail intermittently in the full run while passing in isolation. Investigated rather than assumed — a file containing nothing but 48assert.equal(1, 1)tests at the same sort position reproduces it (2 of 3 runs), while the baseline is 3/3 clean. The race is latent inregistryInstallMerge(which itself binds ~6 sockets) and is exposed by test-runner worker scheduling; any sufficiently large added file triggers it. Pre-existing, not caused by this PR, and matching the known "passes isolated, fails in the full suite" issue in this repo. My tests were nevertheless converted to run socket-free viatest/_helpers/httpInvoke.ts, which drives the real Express pipeline throughapp.handlewithout binding a port. Follow-up owed onregistryInstallMergeseparately.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.