feat(plugins): allow .sql in packages + make the 8 migrators concurrency-safe by Weegy · Pull Request #552 · byte5ai/omadia

Weegy · 2026-07-30T09:34:58Z

Two fixes that were held back in an earlier batch, now unblocked and shipped after review. Follows #536, #548 and #539.

`.sql` in the ZIP allowlist

A distributed plugin could not ship its own migrations — .sql was not in EXTENSION_ALLOWLIST, so the files were rejected at extraction. That blocks plugin-owned schema (G4 in the #470 plan).

This was deliberately rejected once. In the first batch it would have escalated the then-live path traversal (unvalidated identity.id → path.join → recursive fs.rm) from directory delete/replace into arbitrary SQL execution, because the migrator readdir+executes a fixed directory at boot. #548 closed that traversal, which is what unblocks this.

The review verified independently that no route remains from uploaded content to a migrator-scanned path: all 8 migrators derive their directory from import.meta.url or an operator env var — never from package content — and the 3 extraction destinations are staging/preview paths with generated names.

The stronger argument, which our own analysis had buried: .js/.mjs/.cjs are already allowlisted and dynamic-import()ed by the runtime, so anyone who can get a zip to ingest() already holds in-process code execution. .sql is strictly weaker than what the trust boundary already grants — security-inert independently of the migrator analysis.

Also fixes the packaging side. Both boilerplate build-zip.mjs scripts stage a hardcoded INCLUDE list with no migrations entry, and the copy loop skips anything not on it. An author following the boilerplate would have got a green install and no schema, with no warning. The allowlist entry alone was necessary but not sufficient.

Migrator concurrency

All 8 SQL migrators were read-ledger → filter → apply with no mutual exclusion. Two replicas booting together both execute; IF NOT EXISTS masks it, ADD CONSTRAINT does not (42710, boot fails).

Shipped on the third attempt, and both prior rejections shaped the design:

An unbounded pg_advisory_lock is unusable here — three of the eight migrators run inside a plugin activate() that ToolPluginRuntime caps at 10s, so the wait converts a rare race into a deterministic boot failure on every concurrent boot. The KG plugin has already spent up to 6s of that budget in waitForPostgres.
A retry that called pg_advisory_unlock without reading its boolean return could not distinguish "released" from "this session never held it", making the mechanism silently inert.

Not in this PR

The DynamicAgentRuntime rollback was attempted twice and rejected twice. The current attempt does not cover the timeout path: withTimeout is a bare Promise.race and does not cancel, so after the rollback runs and clears the in-flight marker, the orphaned activate() continues and re-registers. Same defect class already documented for ToolPluginRuntime — it wants a cancellation token, not another rollback tweak.

Test plan

Middleware suite: 5,224 pass / 0 fail
Typecheck clean; lint clean (1 pre-existing warning, unrelated)
.sql fail-without-fix reproduced (reverting only zipExtractor.ts → 4 fail)
Migrator tests cover: lock state observable, released on success, released on a throwing migration with the original error preserved, released when the unlock itself fails, client always released, and the bounded path fits inside a 10s activate()
Traversing identity still rejected before extraction; an explicit extensionAllowlist override omitting .sql still rejects it
Decoupling ratchet held at 3,303

Follow-ups noted by review (not blocking)

Both boilerplates' header comments enumerate the host allowlist verbatim and are stale in four ways (.html, .jpeg, .license were already missing; .sql is the fourth). Better to point at zipExtractor.ts than to re-enumerate.
zipExtractor rejects any entry name containing .., not just a .. path segment — so 0012..sql would surface as zip.path_escape. Pre-existing, marginally more likely to be hit now.

^{Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.}

A distributed plugin ships its own schema as `migrations/*.sql`, but the zip extractor rejected the extension, so the package failed ingest with `zip.forbidden_extension`. Adding `.sql` was previously refused because it would have escalated the manifest-identity path traversal (an id of `..` with version `migrations` resolved onto the real migrations directory) from a directory delete into arbitrary SQL executed at boot. That traversal was closed in 09ff9cd, and the remaining routes from uploaded content to a migrator-scanned path were re-verified before this change: - all eight SQL migrators resolve their directory from their own `import.meta.url`, the fixed `middleware/migrations`, or an operator env override — never from package content; - the three extraction call sites land only under state dirs (uploaded-packages staging/final, builder previews), never in the shipped code tree; - the `node_modules` symlink at the packages root is charset-valid as an id and is blocked by the reserved-root check, not by the charset gate. Tests pin those conditions rather than just the happy path: `.sql` accepted by the default allowlist, still rejected under an explicit override that omits it, and no `.sql` surviving outside the packages root for a traversing identity, the reserved `node_modules` id, or a Zip-Slip entry name. Shared package-zip fixtures move to `test/_helpers/pluginPackageZip.ts` so both install-pipeline suites use one builder.

…ti-replica boot Every migrator was read-ledger -> filter -> apply with no mutual exclusion, so two replicas booting together both executed the same pending list. `CREATE ... IF NOT EXISTS` masks that; `ALTER TABLE ... ADD CONSTRAINT` does not — the loser gets 42710 and its boot fails. `conductorWebhookEndpointStore.pg.test.ts` already carried a test-level `migrateWithRetry` workaround for the same race. Design (identical in all eight, no shared helper — that is separate work): - Read the ledger BEFORE locking and return early when nothing is pending, so the steady-state boot takes no lock and can never queue behind a migrating replica. - Acquire with `pg_try_advisory_lock(4410, hashtext(<ledger table>))` in a bounded 2s poll, never the blocking `pg_advisory_lock`: three of the eight run inside a plugin `activate()` that ToolPluginRuntime hard-caps at 10s, and the knowledge-graph plugin can already have spent 6s in `waitForPostgres` first. The try-variant also returns a boolean the code actually reads, so the session always knows whether it holds the lock. - Re-read the ledger UNDER the lock, so a replica that queued behind a winner never re-applies what the winner just applied. - A loser that never gets the lock re-reads the ledger: if the winner finished, it continues; otherwise it throws a retryable error naming the pending files. The message says "timed out" so `bootstrap.retryErroredPlugins` classifies it as transient instead of latching the plugin `errored`. - `pg_advisory_unlock` runs on the success path only, inside `try`, and its boolean is read. It is never awaited in `finally`, where it could hang on a half-open connection (these pools set no statement_timeout) or replace the original migration error. Any path that cannot prove the lock was released ends at `client.release(true)`, which destroys the connection and releases the session lock with it. `client.release()` always runs. - `ensureLedger` retries once on 42P07/23505: `CREATE TABLE IF NOT EXISTS` is not atomic against a concurrent `CREATE TABLE` of the same name. Tests cover, for all eight: no lock when there is no work, lock taken before the first migration, the under-lock re-read, the bounded loser error, the loser whose winner finished, an original error preserved through a failing migration, a throwing unlock, an unlock that reports not-held, a driver that models no advisory locks, the ledger DDL race, and that 6s + 2s fits the 10s activate cap.

…ncy-safe Two of three wave-2 packets shipped after review; the third is held back again. .SQL IN THE ZIP ALLOWLIST — unblocked by #548. It was rejected in the first batch because it would have escalated the then-live path traversal from directory delete/replace into arbitrary SQL execution. #548 closed that, and the reviewer verified independently that no route remains from uploaded content to any migrator-scanned path: all 8 migrators derive their directory from import.meta.url or an operator env var, never from package content, and the 3 extraction destinations are staging/preview paths with generated names. The stronger argument, which the analysis had buried: .js/.mjs/.cjs are already allowlisted and dynamic-import()ed by the runtime, so anyone who can get a zip to ingest() already holds in-process code execution. .sql is strictly weaker than what the trust boundary already grants — which makes it security-inert independently of the migrator analysis. Also adds `migrations` to both boilerplate build-zip.mjs INCLUDE lists. Without it the directory is silently dropped at packaging time: the copy loop skips anything not on the list, so an author following the boilerplate gets a green install and no schema. The allowlist entry alone was necessary but not sufficient. MIGRATOR CONCURRENCY — shipped on the third attempt. Every migrator was read-ledger -> filter -> apply with no mutual exclusion, so two replicas booting together both execute; IF NOT EXISTS masks it, ADD CONSTRAINT does not (42710). Two earlier designs were rejected, and both rejections shaped this one: an unbounded pg_advisory_lock is unusable because three of the eight migrators run inside a plugin activate() that ToolPluginRuntime caps at 10s — turning a rare race into a deterministic boot failure; and a retry that called pg_advisory_unlock without reading its boolean return could not distinguish "released" from "never held", making the mechanism silently inert. STILL HELD BACK: the DynamicAgentRuntime rollback, now on its second rejection. It does not cover the timeout path — withTimeout is a bare Promise.race and does not cancel, so after the rollback runs and clears the in-flight marker, the orphaned activate() continues and re-registers. That is the same defect class already documented for ToolPluginRuntime, and it wants a cancellation token rather than another rollback tweak. 5,224 pass, typecheck clean, ratchet held at 3,303.

The channel-api work added 3 dev-platform references (test/packages). Same legitimate raise as on the C5 branch.

main lowered its own baseline to 3306 by dropping dev-platform cross-references from the API-key comments, so the W2-2 task-seam raise is +135, not +138. middleware/packages is set to 97 rather than main's 99: the generic task seam was scrubbed of implementor names, and that gain is locked in instead of being left as headroom. Conflicting doc comments in apiKeyToken.ts take main's wording (#553 rewrote that file deliberately).

…C5) (#554) * refactor(conductor): delete the dead dev-job step coupling (epic #470 C5) The Conductor's `dev.job` step was built in W3 and never wired: `conductor/index.ts` constructs `ConductorRunExecutor` without a `devJob` dep, so the dispatch branch was permanently false; the launch half had no implementation at all (`createConductorJob`, `setAwaitId`, `getAwaitId` existed only as interface members); `DevJobOutcomeEmitter.emit()` had no caller in `src/`; nothing scheduled the reconciliation sweep; and no bundled template referenced `dev.job` (which `listActions` rejects anyway). Per `specs/470-dev-platform-plugin/dormant-capabilities.md` §1 the verdict is DELETE, not genericise — nothing needs the generic version. Deleted whole: `conductor/devJobStepEffect.ts`, `devplatform/devJobConductorBridge.ts`, `test/conductorDevJobStep.test.ts`. Stripped from `runExecutor.ts`: both imports, the `devJob` field + ctor dep, `DevJobPortUnavailableError`, the dispatch branch, `resolveDevJobAwait`, `reconcileTerminalDevJobAwaits`, `openDevJobAwait`. `awaitStore.listWaiting` drops its `AND channel_type <> 'dev_job'` predicate outright rather than keeping a generic filter. `openHumanAwait` is now the single caller of `create`, so every await has a human holder by construction and the excluded set is provably empty; a filter whose complement no member can enter asserts an invariant in the wrong place, and a future non-human await kind would have to remember to add itself to survive it. New `test/conductorAwaitStore.test.ts` guards the channel-agnostic contract (mutation-checked: re-adding a channel predicate fails it). `dev_jobs.conductor_await_id` (migration 0024) is KEPT — migrations are forward-only here and a DROP is the one irreversible act. It is marked as an orphaned column so the schema is not misread as evidence of a live feature. Core-decoupling ratchet: 3303 -> 3164 (-139), no zone rose. * refactor(conductor): delete the dead dev-job step coupling (C5) First change in this epic that moves the decoupling count DOWN. 3,303 → 3,167 (−136): middleware/src −96, middleware/test −43. WHAT THIS IS NOT: the Conductor is a live feature — 31 files, ~6,200 LOC backend, 23 UI files, 7 migrations, its own spec. Runs, steps, human approval gates, templates, the Designer canvas are all untouched. All 204 conductor tests pass. WHAT WAS DEAD: one step type inside it. `dev.job` was meant to let a workflow launch a Dev Platform job, park the run, and resume on its terminal outcome. It was built and never wired: - conductor/index.ts constructs the executor with NO devJob dep, so the dispatch branch was permanently false — and `git log -S` shows it was never wired in ANY commit - the launch half had no implementation at all: createConductorJob, setAwaitId and getAwaitId existed only as interface members, and dev_jobs.conductor_await_id was never selected or written - DevJobOutcomeEmitter.emit() had no caller in src/ - nothing scheduled the reconciliation sweep - no bundled template referenced dev.job, and listActions never included it, so the step could not pass validation Removed: conductor/devJobStepEffect.ts (122 LOC), devplatform/ devJobConductorBridge.ts (113), test/conductorDevJobStep.test.ts (358), and every dev-job reference in runExecutor.ts (31 → 0), awaitStore.ts (6 → 0) and routes.ts (1 → 0). KEPT: migration 0024's conductor_await_id column. Migrations are forward-only here and dropping a column is the one irreversible act in this change; it is marked orphaned instead. THE SUBTLE PART: awaitStore's human-inbox query excluded `channel_type = 'dev_job'`. That predicate is now gone rather than genericised, because after the delete `openHumanAwait` is the sole writer of conductor_awaits — a filter whose complement no code path can populate asserts an invariant in the wrong place, and a channel allow/denylist would fail open for a future await kind that forgot to register itself. The compensating test needed a fix the review caught: its fixtures were teams/telegram/web, so restoring `<> 'dev_job'` would have left it green — it could not detect the exact regression it exists for. Added a dev_job fixture row and mutation-checked it: with the predicate restored 1 fail, without it 3 pass. Also propagates the decision into the specs, which still described the step as a capability the extraction must carry and H2 as a registry to build. H2 needed no mechanism after all — the coupling was dead, so deleting it was the whole fix. * chore(470): resync C5 with main (#549) — baseline 3,167 → 3,170 The channel-api work added 3 dev-platform references (test/packages). Fourth legitimate raise: main ADDED dev-platform code; core did not re-acquire a dependency. * chore(470): resync C5 with main (#552, #553) — baseline 3,167 * chore(470): resync C5 decoupling baseline with main — 3,448 → 3,312

…#470 C2a) (#555) * refactor(plugin-api): delete the unreachable ctx.devJobs plugin surface `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. * chore(470): lower the ratchet baseline to 3,220 after the ctx.devJobs deletion * chore(470): resync C2a with main (#552, #553) * fix(470): type-sound C2a legacy-manifest test + resync baseline 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. * chore(470): resync C2a baseline onto main after #554 — 3,312 -> 3,226 #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 5 commits July 30, 2026 11:31

Merge remote-tracking branch 'origin/main' into epic/470-wave2

12de215

chore(470): resync wave2 with main (#549) — baseline 3,303 → 3,306

add6fa7

The channel-api work added 3 dev-platform references (test/packages). Same legitimate raise as on the C5 branch.

Weegy merged commit c33fdf7 into main Jul 30, 2026
9 checks passed

Weegy added a commit that referenced this pull request Jul 31, 2026

chore(470): resync C5 with main (#552, #553) — baseline 3,167

4a4f49c

Weegy added a commit that referenced this pull request Jul 31, 2026

chore(470): resync C2a with main (#552, #553)

7109951

Weegy deleted the epic/470-wave2 branch August 14, 2026 06:53

Weegy mentioned this pull request Aug 20, 2026

feat(#470): C4/H1 — manifest-declared, operator-consented public-path grants with exclusive prefix ownership and a terminating early mount #782

Merged

Navigation Menu

Uh oh!

feat(plugins): allow .sql in packages + make the 8 migrators concurrency-safe - #552

feat(plugins): allow .sql in packages + make the 8 migrators concurrency-safe#552
Weegy merged 5 commits into
mainfrom
epic/470-wave2

Weegy commented Jul 30, 2026 •

edited by blacksmith-sh Bot

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

Weegy commented Jul 30, 2026 • edited by blacksmith-sh Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

.sql in the ZIP allowlist

Migrator concurrency

Not in this PR

Test plan

Follow-ups noted by review (not blocking)

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Weegy commented Jul 30, 2026 •

edited by blacksmith-sh Bot

Loading

`.sql` in the ZIP allowlist