Preserve ephemeral sidecar ports and remove parallel plugin contract - #6873
Preserve ephemeral sidecar ports and remove parallel plugin contract#6873eyeszik wants to merge 3 commits into
Conversation
|
Thanks @eyeszik — the manifest/loader consolidation plus the fixed-port sidecar hardening makes the direction of this PR easy to follow. One quick PR-body ask before pool review: could you add a
|
lefarcen
added
risk/high
|
🧪 This PR has changes that need a manual QA pass before merge — please hold off self-merging for now; we’ll loop QA in once it is merge-ready. Thanks for the contribution! 🙏 |
PerishCode
left a comment
PerishCode
left a comment
There was a problem hiding this comment.
This introduces a second plugin contract while leaving the shipped Open Design plugin path untouched, and the new lifecycle has concrete dependency, deletion-safety, launcher, and validation failures. The inline findings below need to be resolved before this can replace or extend the current architecture.
🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.| "$id": "https://open-design.ai/schemas/plugin-loader.manifest.schema.json", | ||
| "title": "Plugin Loader Manifest", | ||
| "type": "object", | ||
| "required": ["id", "name", "version", "entry_point", "permissions", "dependencies"], |
There was a problem hiding this comment.
Use the repository's existing plugin contract instead of introducing this incompatible manifest shape. Open Design already treats SKILL.md plus open-design.json (validated by docs/schemas/open-design.plugin.v1.json and PluginManifestSchema) as the portable plugin seam; the daemon, web UI, CLI, registry, and current plugin-runtime parser all consume that shape. This new required manifest.json contract is not exported from packages/plugin-runtime/src/index.ts or wired to any shipped consumer, and the accompanying example omits both required portable files, so validate:plugins can pass while the product cannot discover or use the example. Extend the existing schema/parser and wire the existing daemon/API/UI/CLI consumers, or remove this parallel loader and example.
| const loaded: string[] = []; | ||
| const skipped: Array<{ id: string; reason: string }> = []; | ||
| const errors = [...discovered.errors]; | ||
| for (const manifest of discovered.manifests) { |
There was a problem hiding this comment.
Enforce declared dependencies before loading plugins. validate_manifest only checks that each value looks like a range; this loop then loads discovery order unconditionally and never checks whether core or another plugin exists, satisfies the range, or has already registered. That directly contradicts extension_contract.md, which says unmet dependencies are skipped, and means a dependent plugin can start against a missing or incompatible host. Resolve and semver-check the dependency graph, topologically load satisfied plugins, and return an explicit skip/error for missing, incompatible, or cyclic dependencies; add a fixture matrix covering those cases.
|
|
||
| async function uninstall(id: string): Promise<void> { | ||
| await disable(id); | ||
| const target = `${opts.rootDir}/plugins/${id}`; |
There was a problem hiding this comment.
Do not derive a recursive deletion path from the manifest ID. The validator accepts every non-empty string as id, so a discovered manifest can use an ID such as ../../outside; uninstall(id) then passes the resulting path to rm(..., { recursive: true }), allowing plugin-controlled data deletion outside the plugin root. It also deletes the wrong folder whenever a valid ID differs from its discovered directory. Restrict IDs to the canonical slug grammar and delete the recorded, realpath-verified manifest.source_dir only after proving it is a direct descendant of the configured plugin root; add traversal, symlink, and ID/folder-mismatch tests.
| ); | ||
| }, | ||
| port: parsePort(process.env[DAEMON_PORT_ENV]), | ||
| port: parsePort(process.env[DAEMON_PORT_ENV]) || 7456, |
There was a problem hiding this comment.
Preserve an explicit port 0; it is the launcher contract for ephemeral ports. Both tools-dev and apps/packaged deliberately set OD_PORT=0, and the packaged supervisor relies on a fresh ephemeral port after restart, but parsePort("0") || 7456 converts that explicit request to 7456. The matching web change similarly converts OD_WEB_PORT=0 to 7457. This makes concurrent namespaces and packaged restarts contend for global fixed ports. Apply the fixed default only when the variable is absent or blank (not when its parsed value is zero) in both sidecars, and retain coverage for explicit zero plus two concurrent namespaces.
| }); | ||
|
|
||
| it('loads, disables, re-enables, and uninstalls the demo plugin', async () => { | ||
| const source = '/home/workspace/open-design/plugins/spec/examples/plugin-architecture-builder-demo'; |
There was a problem hiding this comment.
Make this test portable and restore the required checks. In the prepared PR worktree, pnpm --filter @open-design/plugin-runtime test fails here with ENOENT because /home/workspace/open-design does not exist. The example's test.mjs repeats the same absolute paths, and pnpm guard rejects that new project-owned JavaScript file. Resolve the fixture from the repository/test module location (or keep a self-contained fixture under this package's tests/), convert or remove the .mjs, and verify both the package test and pnpm guard pass from an arbitrary checkout path.
Remove the unshipped parallel plugin loader contract and its example so SKILL.md plus open-design.json remains the only extension model. Preserve explicit ephemeral port requests while retaining fixed defaults for absent sidecar port variables, with focused regression coverage.
eyeszik
changed the title
lefarcen
added
size/S
|
Thanks for pushing the follow-up, @eyeszik. @PerishCode's review is still the blocking review on this PR, so the next useful step is to get that review refreshed against this new head once CI finishes on |
Visual regression reviewHead: 3 changed · 46 unchanged · 0 new without baseline · 0 failed Changed cases
Unchanged cases
Visual diff is advisory only and does not block merging. |
|
Thanks for the approval, @xxiaoxiong. The current head now has your approve + green CI; the remaining step is still a refreshed pass from @PerishCode on |
4 participants
PR Description
Title
Harden plugin architecture loader and lock fixed-port sidecars
Summary
This change set turns the ad hoc add-on path into a single validated plugin contract, adds a canonical manifest schema and loader, ships one working example plugin, and wires validation so broken manifests or unsafe entries fail early.
It also fixes the daemon/web sidecar startup path so the launcher boots on fixed loopback ports by default instead of falling back to dynamic allocation.
What changed
ARCHITECTURE_AUDIT.md,CORE_FREEZE.md,extension_contract.md, andSCALING_PLAN.mdto document the plugin architecture boundary and rollout path.packages/plugin-runtime/manifest.schema.jsonandpackages/plugin-runtime/src/plugin_loader.tsto define and enforce the canonical plugin contract.packages/plugin-runtime/tests/plugin-loader.test.tsto cover duplicate IDs, unsafe entry points, dependency validation, and the load/disable/enable/uninstall lifecycle.plugins/spec/examples/plugin-architecture-builder-demo/as a working example plugin with manifest, implementation, docs, and test.scripts/validate-plugin-loader.tsandpackage.jsonvalidation wiring so plugin manifests are checked before smoke loading.7456and7457.Surface area
Verification
pnpm --filter @open-design/plugin-runtime typecheckpnpm --filter @open-design/plugin-runtime test -- --runInBand packages/plugin-runtime/tests/plugin-loader.test.tspnpm validate:pluginsNotes
57af80991.tsx.