refactor(plugins): delete the never-provided ctx.devJobs surface (epic #470 C2a) - #555
Merged
Conversation
`ctx.devJobs` was a published plugin accessor that nothing ever provided.
It resolved its host service lazily per call, so every invocation threw
"dev-platform host service unavailable". No manifest in this repo, in the
private byte5 plugin set, or in any sibling repo ever declared
`permissions.devJobs`, so no consumer has ever existed.
Per specs/470-dev-platform-plugin/dormant-capabilities.md section 2 this is
a pure deletion of an unreachable surface. No permission gate is added to
ctx.services: `provide("devJobs", ...)` exists nowhere, so both `ctx.devJobs`
and `ctx.services.get("devJobs")` already yield nothing. Removing the
accessor opens no hole. No package manifest, serviceRegistry or builder
codegen is touched, and no installed plugin changes behaviour.
Deleted:
- plugin-api: the `devJobs?` field on PluginContext and the six types
DevJobKind, DevJobStatus, DevJobDescriptor, DevJobCreateRequest,
DevJobEventRecord, DevJobsAccessor
- host pluginContext: the permissions gate, the context spread,
DevJobsHostService and createPluginDevJobsAccessor
- manifestLoader: the `permissions.devJobs` parse and its two summary fields
- admin-v1: the dev_jobs / dev_jobs_repos_hint DTO fields
- devRepoPluginGrantStore.ts and pluginDevJobsAccessor.test.ts (whole files)
The descriptor/event view types survive core-locally in
src/devplatform/devJobTypes.ts, which travels with the dev-platform tree when
it moves. DevJobKind/DevJobStatus are re-exported from src/devplatform/types.ts
rather than redefined.
devJobsHostService survives for the chat surface but sheds everything that
existed only for the plugin path: listGrantedRepoIds and the grants dep,
the plugin-shaped createJob (and with it repoStore, resolveJobPlacement and
mintRunnerToken), and cancelJob's requestedByPluginId creator check with its
finalize dep. chatDevJobService only ever called getJob, listJobs and
listJobEvents; it passed inert stubs for the rest.
Migrations 0024 (dev_repo_plugin_grants) and 0025 (source='plugin') are kept
- the migration set is forward-only - and are annotated as knowingly orphaned
so a future reader does not read the schema as evidence of a live feature.
Back-compat: a stale manifest still declaring `permissions.devJobs` installs
and activates unchanged. Unknown permission keys are ignored today; that was
implicit, and test/manifestDevJobsLegacyKey.test.ts now asserts it explicitly
so a future strict-validation change cannot break stale manifests silently.
…ervice # Conflicts: # specs/470-dev-platform-plugin/decoupling-baseline.json
…ervice # Conflicts: # specs/470-dev-platform-plugin/decoupling-baseline.json
The test/ typecheck ratchet (#573, landed after this branch was last touched) flags 4 errors in test/manifestDevJobsLegacyKey.test.ts: - `createPluginContext` gained two required options (notificationRouter, uiRouteCatalog). They are only dereferenced inside lazy accessors, so the test passed at runtime while being type-unsound. Stubbed both. - Three `as Record<string, unknown>` casts were unsound (TS2352). Two are unnecessary — `mcp` is a declared field on PluginPermissionsSummary. The key-absence checks now use `Object.hasOwn`, which asserts the key is absent rather than merely undefined. Ratchet baseline unchanged at 406: no new debt. Decoupling baseline resynced against current main, 3,448 -> 3,362.
…ervice # Conflicts: # specs/470-dev-platform-plugin/README.md # specs/470-dev-platform-plugin/acceptance.md # specs/470-dev-platform-plugin/decoupling-baseline.json
#554 (C5) landed, so the shared counter moved under this branch. Both PRs reduce disjoint sets of references: C2a still removes exactly 86, the same delta it removed against the pre-#554 main. Also fixes doc drift the merge exposed: README and acceptance.md both still quoted 3,167 on main while the committed baseline was already 3,312 — the number is in three places and #554 updated only the JSON. All three now read 3,226.
Weegy
added a commit
that referenced
this pull request
Aug 12, 2026
…,288 C5 (#554) and C2a (#555) both landed, taking main's baseline to 3,226. C3's own delta is unchanged: +62 (middleware/src +29, middleware/test +33), so the hand-raised baseline is 3,288. That +62 has now measured identically against three successive mains — before #554, after #554, after #555. It is C3's own concentration effect, not drift picked up from whatever else landed, and the README now says so instead of quoting a single merge-time measurement. Also fixes the baseline number in acceptance.md, which still read 3,365. The number lives in three places — the JSON, the README, and the acceptance ratchet row — and nothing checks that they agree.
Weegy
added a commit
that referenced
this pull request
Aug 12, 2026
… C3) (#557) * refactor(dev-platform): one-way layering + namespaced config (C3) * chore(470): hand-raise the ratchet baseline to 3,365 for C3 FIRST TIME the baseline rises for OUR OWN change rather than for something main added. That deserves the explanation to be in the diff, which is why the script refuses `--update` here and forces a hand-edit. Why it rose (+59): the refactor collapses 41 flat DEV_*/FLY_* config keys into one `config.devPlatform` object. The zod schema is unchanged — proven byte-identical, 562 lines, empty diff — so every key name still appears there, and the new mapping layer that builds the namespace names each one a SECOND time. Net: config.ts +48, the new devplatform/config.ts +36, against index.ts −33 and wireDevPlatform.ts −8. The eight moved routers are a wash: same zone, different path. Why it is worth taking: index.ts drops from ~20 threaded config values to one argument, and the layering arrow now points one way (zero src/devplatform → src/routes edges, zero src/routes → devplatform). Both make the P4 file-move mechanical instead of a rewrite. And every one of those +59 references is deleted at extraction — the schema keys, the mapping, and the type file all go with the tree. The honest tension: the ratchet counts identifiers, and this refactor concentrates identifiers rather than removing them. Letting that block a correct refactor would be the measurement driving the work. Documented in README so the next raise is judged against the same two criteria and nothing wider. * chore(470): re-justify the C3 baseline raise against current main — 3,510 Merging main moved the numbers: the hand-raised 3,365 was measured against a main that has since grown. Re-measured, C3's own delta is +62 over main's 3,448 (middleware/src +29, middleware/test +33), not the +59 recorded when the branch was last touched. The src half is unchanged in kind: 41 flat DEV_*/FLY_* keys collapse into one `config.devPlatform` object, and the mapping layer names each key a second time. The test half was never written down. It is the same effect — a shared `devPlatformConfig.harness.ts` plus the moved routers' new `src/devplatform/routes/…` import paths. Checked rather than asserted: all 212 added matching lines in the test zone are inside `middleware/test/devplatform/`, none outside. No core test acquired a dev-platform dependency, which is the only thing a rise is allowed to mean here. README updated to 3,510 with the test-zone reasoning, so the next raise is judged against the same two criteria. * chore(470): re-justify the C3 baseline raise onto main after #555 — 3,288 C5 (#554) and C2a (#555) both landed, taking main's baseline to 3,226. C3's own delta is unchanged: +62 (middleware/src +29, middleware/test +33), so the hand-raised baseline is 3,288. That +62 has now measured identically against three successive mains — before #554, after #554, after #555. It is C3's own concentration effect, not drift picked up from whatever else landed, and the README now says so instead of quoting a single merge-time measurement. Also fixes the baseline number in acceptance.md, which still read 3,365. The number lives in three places — the JSON, the README, and the acceptance ratchet row — and nothing checks that they agree.
Weegy
added a commit
that referenced
this pull request
Aug 20, 2026
#783) * feat(#470): grant-gate ctx.services.get and cut plugin-api 1.0.0 (C2b) Closes the second half of C2 (epic #470 Phase A): the G8 contract break and bug B1. G8 — the contract break, taken once, deliberately. C2a (#555) already deleted `ctx.devJobs` and moved the `DevJob*` view types out of `@omadia/plugin-api` into `middleware/src/devplatform/devJobTypes.ts`, where they travel with the extraction. This records that break and cuts the package at 1.0.0: there is no installed base, nothing is published to npm, and every consumer is a repository we control, so the break is cheap now and expensive later (`implementation.md` §1 row 4). Adds a CHANGELOG naming the removed types, so a consumer grepping its own source for `DevJobDescriptor` lands on the migration note. `harness-channel-api` pinned `^0.1.0` and would have failed to resolve against the bump; repinned to `*` like its siblings. B1 — `ctx.services.get` was a bare pass-through. Any installed plugin could resolve any registered service, `graphPool` included, with no manifest declaration and nothing in the install dialog. `serviceRegistry.ts`'s own header conceded it: "enforcement lives at the consumer seam". The seam now exists. `get` resolves only capability names the plugin declares in `requires:` (or `provides:`, to read back its own registration) and throws the new typed `ServiceNotDeclaredError` otherwise, naming the capability and the manifest field that would grant it. `has` stays ungated — existence is not a capability. Removing `ctx.devJobs` without this would have converted a permission-gated, kernel-attributed accessor into an ungated, self-attributed one (§2.2). So `provide` also accepts `perCallerService(factory)`: the kernel invokes the factory with the id it activated the consumer under, never with an argument the consumer supplies. The factory is a symbol-branded object, so a service that is itself a function cannot be mistaken for one; value providers are untouched. A call-site audit across this repo's built-in plugin packages and all ten standalone plugin repos found 27 (plugin, capability) pairs consumed without being declared — a fail-closed gate in one step would have broken every shipped plugin. Those exact pairs sit behind a dated, frozen, per-plugin allowlist: they warn once and resolve, everything else fails closed. The allowlist is closed in both directions — a different plugin asking for the same name still throws, and an allowlisted plugin asking for a new name still throws. Counter-proof: reverting the gate to `return serviceRegistry.get<T>(name)` fails 9 of the 25 new tests; restoring it passes all 25. Ratchet 3296 → 3300, hand-raised for `middleware/packages` only. All five lines are the new CHANGELOG documenting a removal; three of them are literal strings that cannot be reworded (a spec path, a test filename, the future package name). The first measurement was +31 — the avoidable 26 were reworded away rather than excused, leaving `middleware/test` at its 1,030 baseline. Justification recorded in the baseline JSON, README and acceptance.md. Also corrects plan.md §4.2, which still claimed the `DevJob*` types stay in core — `implementation.md` §2.5 had already flagged it as contradicting §4.1, and shipped code now settles it. * fix(#470 C2b): close three gaps in the ctx.services.get grant gate Cross-family review of PR #783 found the gate correct in shape but incomplete in three ways that a passing test suite did not surface. 1. The dated legacy allowlist was missing 21 (plugin, capability) pairs that are resolved today, 9 of them on @omadia/orchestrator itself. `harness-orchestrator/src/plugin.ts:452` reads 'llmProviderCatalog' unconditionally near the top of activate(), so the gate as merged would have thrown ServiceNotDeclaredError and killed the chat orchestrator at boot. The first audit missed these because the names sit behind exported constants (PROCESS_MEMORY_SERVICE_NAME, PLUGIN_CAPABILITIES_SERVICE, CHANNEL_RESOLVER_SERVICE, ...) rather than string literals, and because some channel plugins resolve capabilities through shared @omadia/channel-sdk helpers instead of a literal call site in their own source. 2. Nothing derived the allowlist from the repository, so the miss above was invisible to CI. test/pluginServiceGrantCoverage.test.ts now walks every middleware/packages/*/manifest.yaml, resolves each `services.get` argument through the TypeScript checker (literals and const identifiers alike, comments excluded), and fails when a name is neither declared nor allowlisted. It found three further gaps on @omadia/ui-orchestrator that hand analysis had also missed. A second case fails on stale built-in rows so the ramp cannot rot. 3. Only ctx.services.get passed the caller. Every other plugin-facing surface — ctx.memory, ctx.entities, ctx.mcp, ctx.subAgents, ctx.llm, ctx.events — still called serviceRegistry.get(name) and therefore silently received KERNEL_SERVICE_CALLER, so a perCallerService provider would have handed the plugin the kernel-scoped instance. That is the exact self-attribution failure the feature exists to prevent. The frozen ServiceCaller built in createPluginContext is now threaded through all six. Also: perCallerService documented "one implementation per consuming plugin" while invoking the factory on every .get(). Resolution is now memoized by factory object then by caller.pluginId, which makes the documented contract true and self-invalidates on provider replacement because a replaced provider is a different object. The gate test fixture stored `manifest: {}`, but memoryDeclared reads permissions off the catalog entry's unparsed manifest — so ctx.memory was undefined regardless of what the fixture declared and the new attribution test asserted on nothing. The fixture now carries the raw manifest document. Ratchet unchanged at 3300; no banned strings added. * chore(#470 C2b): regenerate the plugin-api golden snapshot after C1 landed PR #780 (C1) merged to main while this branch was in review, so the snapshot the gate compares against now exists. Merged origin/main and regenerated it with `npm run api:update -w packages/plugin-api`. The diff is exactly this PR's intended surface change and nothing else: ServiceCaller, PerCallerFactory, perCallerService, isPerCallerService, resolvePerCallerService and ServiceNotDeclaredError added, and ServicesAccessor.provide/replace widened to accept a PerCallerFactory. No removals — the DevJob* types were already gone from `src/` before either branch, leaving only a tombstone comment that the snapshot strips. So this PR's own surface change is additive; the 0.1.0 -> 1.0.0 bump is the deliberate departure from 0.x, not a break in this diff. npm run api:check -w packages/plugin-api ✓ up to date npm test -w packages/plugin-api 1/1
Weegy
added a commit
that referenced
this pull request
Aug 20, 2026
…locked plugin migrations (epic #470 C7/G4) (#787) * feat(#470): grant-gate ctx.services.get and cut plugin-api 1.0.0 (C2b) Closes the second half of C2 (epic #470 Phase A): the G8 contract break and bug B1. G8 — the contract break, taken once, deliberately. C2a (#555) already deleted `ctx.devJobs` and moved the `DevJob*` view types out of `@omadia/plugin-api` into `middleware/src/devplatform/devJobTypes.ts`, where they travel with the extraction. This records that break and cuts the package at 1.0.0: there is no installed base, nothing is published to npm, and every consumer is a repository we control, so the break is cheap now and expensive later (`implementation.md` §1 row 4). Adds a CHANGELOG naming the removed types, so a consumer grepping its own source for `DevJobDescriptor` lands on the migration note. `harness-channel-api` pinned `^0.1.0` and would have failed to resolve against the bump; repinned to `*` like its siblings. B1 — `ctx.services.get` was a bare pass-through. Any installed plugin could resolve any registered service, `graphPool` included, with no manifest declaration and nothing in the install dialog. `serviceRegistry.ts`'s own header conceded it: "enforcement lives at the consumer seam". The seam now exists. `get` resolves only capability names the plugin declares in `requires:` (or `provides:`, to read back its own registration) and throws the new typed `ServiceNotDeclaredError` otherwise, naming the capability and the manifest field that would grant it. `has` stays ungated — existence is not a capability. Removing `ctx.devJobs` without this would have converted a permission-gated, kernel-attributed accessor into an ungated, self-attributed one (§2.2). So `provide` also accepts `perCallerService(factory)`: the kernel invokes the factory with the id it activated the consumer under, never with an argument the consumer supplies. The factory is a symbol-branded object, so a service that is itself a function cannot be mistaken for one; value providers are untouched. A call-site audit across this repo's built-in plugin packages and all ten standalone plugin repos found 27 (plugin, capability) pairs consumed without being declared — a fail-closed gate in one step would have broken every shipped plugin. Those exact pairs sit behind a dated, frozen, per-plugin allowlist: they warn once and resolve, everything else fails closed. The allowlist is closed in both directions — a different plugin asking for the same name still throws, and an allowlisted plugin asking for a new name still throws. Counter-proof: reverting the gate to `return serviceRegistry.get<T>(name)` fails 9 of the 25 new tests; restoring it passes all 25. Ratchet 3296 → 3300, hand-raised for `middleware/packages` only. All five lines are the new CHANGELOG documenting a removal; three of them are literal strings that cannot be reworded (a spec path, a test filename, the future package name). The first measurement was +31 — the avoidable 26 were reworded away rather than excused, leaving `middleware/test` at its 1,030 baseline. Justification recorded in the baseline JSON, README and acceptance.md. Also corrects plan.md §4.2, which still claimed the `DevJob*` types stay in core — `implementation.md` §2.5 had already flagged it as contradicting §4.1, and shipped code now settles it. * feat(#470): permissions.sql + shared plugin migration runner (C7 / G4) Plugins can own tables. Three things had to become true for that to be safe rather than merely possible, and this commit is all three. 1. Reaching a Postgres pool is now a DECLARED, GRANTED permission. C2b made `ctx.services.get(name)` resolve only capabilities the manifest declares in `requires:` (bug B1). That is the right rule for an ordinary capability, but not for `graphPool`: the pool is not a service another plugin provides, it is the operator's own database — the same one core writes user data through. A `requires:` line is the plugin author's own say-so, and that is the wrong bar for handing it over. So pool-shaped capabilities need both halves: `permissions.sql` in the manifest (visible in the install dialog) AND an operator grant row. The two denial reasons stay distinct types because they have different fixers — `undeclared` is the author's, `ungranted` is the operator's, and collapsing them would send every author chasing a manifest bug that is not theirs. The existing `graphPool` capability name is reused; no second name is invented for the same pool. `POOL_SHAPED_CAPABILITIES` is a closed set rather than a heuristic, so a new pool capability is ungated until someone adds it — a reviewable omission instead of an invisible one. 2. A ledger is unambiguously ITS plugin's ledger. A plugin able to name any table could forge another plugin's migration history and thereby suppress that plugin's schema changes at its next boot. The name is charset-validated (`^[a-z][a-z0-9_]{2,62}$`) BEFORE it is quoted into DDL — an allowlist that rejects `"` cannot be defeated by a cleverer `"`, whereas an escaping approach has to be right about every case. Exclusive ownership is enforced by `UNIQUE (ledger)` in `plugin_sql_grants`, not by the prefix rule. The prefix rule cannot separate `acme_tool` from `acme_tool_extra`: a name carrying both prefixes would pass for either plugin. The constraint has no such edge, so the prefix check is documented as the cheap offline half, not as the boundary. 3. Applying migrations is serialised, once, for everyone. `implementation.md` B3 recorded core migrators racing on multi-replica boot: two replicas read an empty ledger and both execute. Handing plugin authors a documented pattern to copy would have recreated that bug once per plugin, in code the operator cannot patch. So there is one `runPluginMigrations` and a plugin cannot opt out of any of it. `pg_advisory_xact_lock` rather than the session-scoped variant: it is released by COMMIT or ROLLBACK, including the server's rollback on a dead connection, so it needs none of the release-or-destroy care a leaked session lock demands. `SET LOCAL lock_timeout` rather than a try-lock poll loop: "wait, but not forever" in one statement. A plugin-only advisory namespace (4420) rather than core's 4410, so a plugin-supplied ledger name that happens to hash onto a core ledger is not an availability lever. One transaction for the whole batch, not one per file: a half-applied plugin schema is a state no file in the directory describes. Empty directory throws. A plugin declaring `permissions.sql.migrations` is asserting it ships schema; a directory with nothing in it means the build dropped the files. "0 applied, all good" would let it activate and fail later against tables that were never created, several layers from the cause. Checksum drift on an applied file throws unless explicitly opted into. An edited migration means the database and the package disagree about what ran. `.js`/`.mjs` migrations receive the same transaction-bound client as `.sql` ones, so the codegen path in implementation.md D6 is not a weaker path. Unknown `permissions.*` keys now warn instead of vanishing silently — same implementation and same `KNOWN_PERMISSION_KEYS` shape C4 (#782) introduced, so the two branches merge as a one-line union of the key set. Migration 0045 (0044 is taken by sandbox_registry on main). * feat(#470): wire the SQL gate and auto-migrate at activate (C7 / G4) The grant is read ONCE, at activate, before the context is built. `ctx.services.get` is synchronous and cannot await a database read. Doing the lookup inside the accessor would force either an async accessor (a breaking change to every plugin) or a cached best-effort answer that is permissive while the cache is cold — and a permission that is permissive while cold is not a permission. So the async read happens where awaiting is free and the gate is a pure function of its result. The grant must also still MATCH the manifest. An operator granted a specific ledger; a package that later ships a manifest naming a different one has not been granted that one. Carrying the stale row forward would let a plugin update silently move its schema somewhere the operator never approved, so a mismatch is treated as ungranted and logged. `ctx.sql` exists only for a plugin that declared, was granted, and has a package root to resolve paths against. The root is not optional bookkeeping: the migrations directory comes from a plugin-supplied manifest, so `../../../etc` is a string a package can ship. Every other core migrator resolves its directory from its own `import.meta.url` and inherits containment for free; this one takes a path from a manifest and therefore has to re-establish it. The `+ path.sep` on the containment comparison is load-bearing — without it a sibling directory named `<root>-evil` passes a bare `startsWith`. Migrations run BEFORE the plugin's `activate()`, and only when the manifest declares `permissions.sql.migrations`. The ordering is not a convenience: it is what makes "the tables exist" an invariant `activate()` can rely on rather than a race each plugin re-loses in its own way. A failure fails the activation, because the alternative is a plugin running against a schema it was not built for — which fails later, further away, and with the database in a state nobody chose. The grant store resolves `graphPool` per call rather than capturing it. `ToolPluginRuntime` is constructed ~600 lines before the pool exists, because the pool is published by a plugin this very runtime activates. A store bound at construction would therefore always be the null one. A plugin that activates before the provider (or before migration 0045 has been applied) reads no grant and is treated as ungranted — the fail-closed direction, self-correcting on the next boot once ordering has settled. * test(#470): C7 gate + migration suites, with the lock counter-proof (G4) 42 tests across two suites, split by what they can honestly prove. `pluginSqlPermission.test.ts` (22, no database) covers the decision table and the name validation — both pure, so the outcome is asserted directly rather than inferred from which exception escaped. `pluginMigrations.pg.test.ts` (20, real Postgres) covers everything whose property belongs to the SERVER rather than to this code: that `pg_advisory_xact_lock` actually serialises, that `CREATE TABLE IF NOT EXISTS` under it cannot double-create, that `UNIQUE (ledger)` actually refuses a second owner. A hand-rolled fake would have all three by construction and would prove only that the fake has them. THE COUNTER-PROOF The concurrency cases are only worth something if they fail without the lock. Removing the `pg_advisory_xact_lock` call and re-running: without lock: RUN 1..5 → pass=0 fail=2 (10/10 failures) with lock: RUN 1..5 → pass=2 fail=0 (10/10 passes) Failure mode is `23505` on `pg_type_typname_nsp_index` — both transactions pass the `IF NOT EXISTS` existence check and both reach the catalog insert. That is the exact race B3 documented, reproduced deterministically, which is the only way to know the lock is load-bearing rather than decorative. Cases chosen so the assertion is the mechanism, not the report: - Ordering is proven by the second migration's column existing, not by the report's own array; the files are created in an order that disagrees with lexical order, so a runner using readdir() order would pass on some filesystems and fail on others. - `allowChecksumDrift` asserts the file is SKIPPED, not re-applied — re-running the CREATE would have thrown 42P07, so the assertion also proves the opt-in does not silently re-execute. - Batch rollback checks the ledger table is gone too, including the row for the migration that individually succeeded. - The injection case re-queries `plugin_sql_grants` afterwards: the table still being there is the proof the name never reached SQL. - A case asserts two DIFFERENT plugins do not serialise, or every boot would funnel every plugin's schema through one lock. - The store's read path is exercised against an unreachable database to show it degrades to "ungranted" rather than to "granted". The `PG_URL` narrowing is an `assert.ok`, not a non-null assertion — the suite-level skip is not a type narrowing, and a bad value should not reach `pg`. Suite uses `PLUGIN_SQL_PG_TEST_URL` first and skips cleanly with a reason when no test database is configured (issue #572: no hardcoded default port). * fix(#470): close six defects found by cross-family review of C7 / G4 Adversarial review of the permissions.sql gate and the plugin migration runner, from a different model family than the one that wrote them. Six findings; each fix carries a test that fails without it (verified by mutation, not assumed). 1. Ledger names now live in a reserved `plg_<sanitized-id>_` namespace. The old rule only required a ledger to START WITH the plugin's folded id, and some plugin ids fold onto real core table names (`@tasks` -> `tasks`, `@agents` -> `agents`, `@plugin/sql` -> `plugin_sql_grants`). For those, `CREATE TABLE IF NOT EXISTS` adopted core's table as the plugin's ledger. Nothing downstream re-checked the name; the only thing that stopped a write was the adopted table happening to lack the ledger's columns, which is a coincidence of column shape rather than a rule. Core migration ledgers were safe only because they all start with `_`, which the charset regex rejects — also not an enforced property. No core table can be inside `plg_`, so the syntactic check now carries the guarantee its own doc comment claimed. Free to tighten today: nothing calls `grant()` yet, so no deployment holds a row under the looser rule. 2. Migrations are time-bounded, and the runner is bounded too. `runMigrations()` was awaited BEFORE `withTimeout(activateFn, 10_000)`, so the activate cap never covered it, and `lock_timeout` bounds only lock ACQUISITION. A migration that took its locks and then ran forever held the advisory lock and blocked boot for every replica. Adds a server-side `statement_timeout` and wraps the call in its own `MIGRATION_TIMEOUT_MS`. The test that claimed "the lock wait budget stays inside the 10s activate() cap" asserted a relationship between two numbers that were never related — it compared one constant to a literal and passed regardless. Replaced with the ordering that does matter (lock wait << statement budget) plus a case that reads both budgets back out of the live transaction. 3. `graphPool` is handed to plugins BORROWED, not owned. `services.get` returned the raw `pg.Pool` — the same one core writes user data through — so `ctx.services.get<Pool>('graphPool')?.end()` in any plugin tore down the whole middleware's connection pool. This is the #665 class of bug and `adminEmbeddingProvider.ts` records it having already happened once. `query`/`connect`/`release` pass through (arbitrary SQL is the granted, intended behaviour); `end` and the listener removers throw, and the `connect().pool` escape is closed. Applied at the plugin-facing seam, so core keeps the real object. 4. Read-once grant semantics are documented and pinned. Revoking a grant does not disarm a plugin that is already running — the gate closes over a boolean taken at activate. That is defensible (see the sync `services.get` argument) but was nowhere stated. Documented on `revoke()` and on `sqlGranted`, and pinned by a test so drift in either direction has to be deliberate. 5. `pluginSqlPermission.test.ts` is text again. It contained a literal NUL byte, so git classified the file as binary: the diff read `Bin 0 -> 11636 bytes` and the entire 11.6 KB of the test proving the injection defence was invisible in the PR, unreviewable and un-blameable. Now escaped. 6. Timeout conditions surface as typed errors. `55P03` / `57014` reached the caller as bare pg errors, so an activation log blamed the package for what is usually another replica holding the lock. Both are re-typed as SqlMigrationError; every other pg error still propagates raw, because a syntax error in a plugin's own SQL is most useful unwrapped. Verified: build, typecheck, typecheck:test (406 baseline, no regressions), lint all pass. Full suite 7298 tests / 7286 pass / 0 fail. pg suite 22/22, re-run 3x. check-core-decoupling held at 3300 (the +3 my new tests cost was reworded away, not raised).
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.
Deletes a published plugin surface that no code has ever provided. Ratchet 3,303 → 3,220 (−83).
What was dead
ctx.devJobsresolves its host service lazily, per call — andprovide('devJobs', …)exists nowhere inmiddleware/src. So every invocation throwsdev-platform host service unavailable. Verified: zeropermissions.devJobsdeclarations in this repo, inomadia-byte5-plugins(six production private plugins), or in any sibling repo. No consumer has ever existed.Five layers were independently missing: no provider, no grant writer (
DevRepoPluginGrantStorewas never constructed, sodev_repo_plugin_grantshad no writer at all), no grant API, no consent surface, no consumer.Deleted
PluginContext.devJobs+ the sixDevJob*types from@omadia/plugin-api· thepermissions.devJobsgate,DevJobsHostServiceandcreatePluginDevJobsAccessorfromplatform/pluginContext.ts· the manifest parse and its two summary fields · thedev_jobs/dev_jobs_repos_hintadmin DTO fields ·devRepoPluginGrantStore.ts(whole file) ·pluginDevJobsAccessor.test.ts(whole file).Types moved to a local
src/devplatform/devJobTypes.ts— not a new published package — so they travel with the tree at P4.src/devplatform/types.tsalready ownedDevJobKind/DevJobStatusasas constunions backing the runtime validators, so the new file re-exports rather than redefines.DevJobCreateRequesthad no surviving consumer and was dropped outright.devJobsHostService.tssurvives and shrinks — 163 → 113 lines, deps six → one.chatDevJobServicecalls exactlygetJob,listJobs,listJobEvents; everything else was plugin-only and the chat surface was already faking it (it passedasync () => []for grants and afinalizethat unconditionally threw — which is itself the proof those paths were never live).Migrations 0024/0025 kept and annotated as knowingly orphaned. Forward-only repo.
Scope
Pure deletion. No package manifest, no
serviceRegistry.ts, no builder codegen, no permission gate added toctx.services.That is deliberate. A previous attempt bundled this with a capability gate on
services.getand was rejected: three private byte5 integration plugins callctx.services.provide()while declaringprovides: [], so they would have failed to activate outright — andchannel-teamsresolvesgraphTenantIdwith a?? 'default'fallback, so a silently-undefinedgetwould have written graph data under the wrong tenant id. Data corruption, not an outage.The gate is still needed, and it remains a real hole — but it is not a prerequisite for this deletion. Both
ctx.devJobsandctx.services.get('devJobs')already yield nothing today, so removing the accessor opens no path that was not already open. The gate becomes a prerequisite before the extracted plugin registersdevJobs(P4), and it needs a compat path for out-of-repo plugins.Backward compatibility
A manifest still declaring
permissions.devJobsinstalls and activates cleanly withctx.devJobs === undefined. Unknown manifest keys being ignored was implicit — nothing stated it — somanifestDevJobsLegacyKey.test.tsnow asserts it explicitly, in both thetrueand{ repos_hint: [...] }forms, so a future strict-validation change cannot silently start rejecting stale manifests.Test plan
5,290 pass / 0 fail;npx tsc --noEmitclean; workspace typecheck (31 packages) exit 0; build exit 0devJobOrchestratorTool.test.ts(19 tests incl. the authorization suite),chatDevJobToolWiring.test.ts,chatDevJobService.pg.test.tsserviceRegistry.ts, no codegen touched — every installed plugin resolves exactly what it resolved beforeweb-ui/app/_lib/storeTypes.tsneeded no change: itsPluginPermissionsSummarynever mirroreddev_jobs(normcp), which independently corroborates the "no consent surface" findingTwo intermittent full-suite failures were investigated rather than assumed: different tests each run, both green in isolation, and a stash-rebuild-baseline run was clean — the documented load-sensitivity, not this change.
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.