Skip to content

R0: module-migration rails (src/ serving, live-edit cache, scriptType loading, governance) - #812

Merged
byrongamatos merged 5 commits into
mainfrom
feat/r0-plugin-module-rails
Jul 8, 2026
Merged

byrongamatos merged 5 commits into
mainfrom
feat/r0-plugin-module-rails

Conversation

@byrongamatos

@byrongamatos byrongamatos commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

R0 — module-migration rails

The first phase of the monolith-killing refactor: the one-time host enablement that lets a plugin move off a single global-scope screen.js IIFE onto a native ES-module graph — no build step, no framework, source-served. No plugin can migrate until core can serve and load a src/ module graph; today a migrated plugin would 404. This unblocks the R1 pilots (stems, then studio) and gates the later code-move phases.

Three commits, each its own concern:

A1 — host code (feat(plugins))

  • New sandboxed GET /api/plugins/{id}/src/{path} route serving a plugin's src/ source subtree, containment-checked by the same safe_join guard as assets/ (traversal/absolute/NUL → 404).
  • Live-edit cache contractCache-Control: no-cache + a weak mtime/size ETag + If-None-Match304 on src/, screen.js, and assets/ (previously screen.js sent no cache headers and assets/ emitted an ETag but never revalidated). An edited module reloads on refresh; unchanged ones 304.
  • scriptType / minHost passthrough from plugin.json to /api/plugins; the loader injects a "scriptType":"module" plugin as <script type="module">. A module script's load event awaits its whole static-import graph, so the loader's completion-by-onload + _loadingPluginId contract is preserved. Classic plugins are untouched; minHost is passthrough-only (enforcement deferred to a later phase).

A2 — governance & rails (docs(governance))

  • Constitution → v1.2.0: Principle II names native ES modules as a first-class, build-free extension mechanism (the scriptType:"module" load path), keeping the no-bundler/source-served rule; a new Operating Constraints "Module load contract" clause. Mirrored in CLAUDE.md.
  • docs/plugin-modules.md (migration playbook) and docs/size-exemptions.md (the signed 1,500-line size-norm register).
  • Maintainer/CI-only ESLint gate: max-lines warns at 1,500 as a non-blocking ratchet (exempt-file ceilings mirror the register), and import-x/no-unresolved + no-cycle hard-error on ES-module graphs. New lint CI job; never on the serve/Docker path.

A0 — perf baseline (chore(perf))

  • scripts/perf-baseline.mjs (maintainer-only) + docs/perf-baseline.md — a rerunnable harness (server p50/p95/p99, cold boot, JS-heap soak) so every phase can be checked for "screen-entry and frame-time no worse."

Deliberately scoped out (validated/enforced in later phases)

  • The boot() re-mount hook — research showed core never re-injects screen.js; plugins re-init via the screen:changed event, so a module plugin behaves identically. Confirmed by the R1 pilot's leave/re-enter test instead of built speculatively.
  • minHost enforcement (refuse-with-message) — deferred; passthrough only for now.

Verification

  • Full pytest (2334 passed) + node --test JS suite (1019 passed); eslint . 0 errors.
  • Real-server curl (serve / 304 / traversal / live-edit) and a headless-browser check (a module plugin injected type=module, its src/ graph loaded and executed).
  • Playwright smoke compared byte-identical against clean main — no regression to classic-plugin loading.
  • Local Codex preflight clean (caught one real latent lint-parse bug, fixed).

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features
    • Added ES-module plugin support with sandboxed serving of plugin src/ and screen.js via plugin.json (scriptType, minHost).
    • Improved live-edit performance for plugin screen.js, src/, and assets using ETag-based conditional responses.
  • Bug Fixes
    • Ensured module plugins set the injected script element to type="module" for correct load timing.
    • Hardened plugin source route access (path containment + readiness gating).
  • Tests
    • Added automated coverage for module loading, media types, caching/304 behavior, and traversal protection.
  • Documentation
    • Updated plugin module migration guides, constitution, and added performance baseline guidance.
  • Chores
    • Added CI lint job and introduced CI-focused Flat ESLint with size-exemption support.

byrongamatos and others added 3 commits July 8, 2026 09:05
…ype loading (R0)

Host rails for the module-migration refactor: a plugin can move off a single
global-scope screen.js IIFE onto native ES modules with no build step.

- New GET /api/plugins/{id}/src/{path:path} route serving a plugin's src/
  source subtree, containment-checked with the same safe_join guard as assets/.
- Live-edit cache contract (Cache-Control: no-cache + weak mtime/size ETag +
  If-None-Match -> 304) on src/, screen.js, and assets/ so edited modules reload
  on refresh while unchanged ones 304 (screen.js sent no cache headers before;
  assets/ emitted an ETag but never revalidated).
- scriptType/minHost passthrough from plugin.json to /api/plugins; the loader
  injects a "scriptType":"module" plugin as <script type="module">, which fires
  onload only after its static-import graph evaluates -> preserves the
  completion-by-onload + _loadingPluginId contract. minHost is passthrough-only.

Classic plugins are unaffected. Verified end-to-end: full pytest (2334) + JS
suite (1019) green; real-server curl (serve/304/traversal/live-edit) and a
headless-browser check (module plugin injected type=module, src/ graph loaded
and executed) both pass.

Tests: tests/test_plugin_src_route.py, tests/js/plugin_loader_script_type.test.js

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…r, eslint gate (R0)

The governance rails for the module-migration refactor, gating the later
code-move phases.

- Constitution -> v1.2.0: Principle II names native ES modules as a first-class,
  build-free extension mechanism (scriptType:"module" load path, plugins and
  eventually core static/js/), keeping the no-bundler/source-served rule; new
  Operating Constraints "Module load contract" clause captures the two findings
  that make it correct (type=module preserves completion-by-onload; per-visit
  re-init comes from screen:changed, not screen.js re-execution). Mirrored in
  CLAUDE.md.
- docs/plugin-modules.md: the migration playbook (layering, import-time purity,
  import.meta.url assets, worklet-separate-graph, the ETag live-edit loop).
- docs/size-exemptions.md: the signed 1,500-line size-norm register (Byron signs
  core/bundled rows, Christian the authored virtuoso row).
- Maintainer/CI-only ESLint gate: eslint.config.js (max-lines warn ratchet with
  register-mirrored ceilings; import-x/no-unresolved + no-cycle hard-error on
  ES-module graphs, dormant until module code lands) + a `lint` CI job + `npm run
  lint`. Never on the serve/Docker path (Principle I). eslint . -> 0 errors.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
scripts/perf-baseline.mjs (maintainer/CI-only, Playwright-driven) captures
server p50/p95/p99 latency, cold boot-to-interactive, JS-heap after an idle
soak, and the injected plugin-script shape (count of type=module) so every
refactor phase (R0 -> R3c) can be checked for "screen-entry and frame-time no
worse." docs/perf-baseline.md holds the methodology + the R0 baseline; the
playback frame-time and chart-loaded screen-entry rows need a seeded library
and are re-taken per environment.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings July 8, 2026 07:26
@coderabbitai

coderabbitai Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 649d7c65-afee-4e43-b4f3-bb753885b3e2

📥 Commits

Reviewing files that changed from the base of the PR and between ffe8e6e and 802f61e.

📒 Files selected for processing (3)
  • .github/workflows/ci.yml
  • docs/perf-baseline.md
  • docs/plugin-modules.md
✅ Files skipped from review due to trivial changes (2)
  • docs/plugin-modules.md
  • docs/perf-baseline.md
🚧 Files skipped from review as they are similar to previous changes (1)
  • .github/workflows/ci.yml

📝 Walkthrough

Walkthrough

This PR adds ES-module plugin support with sandboxed src/ serving and live-edit caching, module-aware loader behavior, governance/docs updates, a CI lint gate with size exemptions, and a Playwright performance baseline harness.

Changes

Plugin module loading and live-edit caching

Layer / File(s) Summary
Backend live-edit caching and module metadata
plugins/__init__.py
Adds weak ETag and conditional response helpers, passes through module metadata, and updates plugin file-serving endpoints to use conditional caching for screen.js, assets, and src files.
Frontend loader script type handling
static/app.js, tests/js/plugin_loader_script_type.test.js
Sets injected <script> type to module for module plugins before append, validated by guarded-conditional tests.
Backend route and caching tests
tests/test_plugin_src_route.py
Tests src serving, traversal rejection, conditional 304 behavior, cache invalidation after edits, and readiness gating.
Governance and documentation for module migration
.specify/memory/constitution.md, CLAUDE.md, docs/plugin-modules.md, CHANGELOG.md
Formalizes the module load contract and plugin.json keys, bumps the constitution to v1.2.0, adds the migration playbook, and updates the changelog.

CI lint gate and size exemptions

Layer / File(s) Summary
ESLint config, size exemptions, and CI wiring
eslint.config.js, docs/size-exemptions.md, package.json, .github/workflows/ci.yml
Adds Flat ESLint config with max-lines and import-cycle rules, a signed size-exemptions registry, lint script/dependencies, and a CI lint job.

Performance baseline harness

Layer / File(s) Summary
Perf baseline script and docs
scripts/perf-baseline.mjs, docs/perf-baseline.md
Adds a Playwright/fetch-based CLI harness measuring server latency and client heap/boot metrics, with recorded baseline documentation.

Estimated code review effort: 3 (Moderate) | ~30 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Browser
  participant PluginAPI
  participant FileSystem
  Browser->>PluginAPI: GET /api/plugins/{id}/src/{path} with If-None-Match
  PluginAPI->>FileSystem: read file mtime+size
  PluginAPI->>PluginAPI: compute weak ETag
  alt ETag matches
    PluginAPI-->>Browser: 304 no body
  else changed
    PluginAPI-->>Browser: 200 with content, new ETag, no-cache
  end
Loading
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title is specific and clearly related to the PR’s main module-migration rails work, covering src serving, cache behavior, scriptType loading, and governance.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/r0-plugin-module-rails

Comment @coderabbitai help to get the list of available commands.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR introduces the “R0 module-migration rails” needed to migrate plugins from a single global-scope screen.js IIFE to a source-served native ES-module graph (no bundler/build step). It adds a sandboxed src/ serving route, establishes a consistent live-edit cache revalidation contract (ETag + 304) across plugin file routes, and updates the loader to inject module plugins via <script type="module">, alongside governance/docs and a CI-only ESLint guardrail + perf baseline harness.

Changes:

  • Add /api/plugins/{id}/src/{path} source-serving route plus shared no-cache + weak ETag + If-None-Match304 behavior for src/, screen.js, and assets/.
  • Update frontend plugin loader to set script.type = 'module' when plugin.script_type === 'module' and add tests for both the route + loader contract.
  • Add CI-only ESLint flat config (size norm + module hygiene), a perf-baseline script/docs, and governance/documentation updates.

Reviewed changes

Copilot reviewed 14 out of 15 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
tests/test_plugin_src_route.py New FastAPI-level tests covering src/ serving, containment, and ETag/304 live-edit behavior.
tests/js/plugin_loader_script_type.test.js New Node tests asserting the loader’s guarded <script type="module"> injection structure.
static/app.js Loader change: set script.type = 'module' for module plugins while preserving onload completion semantics.
scripts/perf-baseline.mjs New Playwright-driven perf harness to capture server latency + client boot/memory metrics.
plugins/__init__.py Adds src/ route, shared file-response helper with ETag/304, and scriptType/minHost passthrough to /api/plugins.
package.json Adds eslint + eslint-plugin-import-x dev deps and npm run lint.
package-lock.json Lockfile updates for new lint dependencies.
eslint.config.js Flat ESLint config enforcing size warning ratchet and ESM graph hygiene (CI-only).
docs/size-exemptions.md New signed register of line-count exemptions for the size norm.
docs/plugin-modules.md New ES-module migration playbook and non-negotiable rules for plugin module graphs.
docs/perf-baseline.md New methodology doc + initial baseline results section.
CLAUDE.md Adds guidance for ES-module plugins and the module load contract.
CHANGELOG.md Documents the new module-migration rails, governance/lint gate, and perf baseline harness.
.specify/memory/constitution.md Constitution bump to v1.2.0 incorporating native ES modules and the module load contract.
.github/workflows/ci.yml Adds a lint job running npm run lint on Node 20.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread plugins/__init__.py
Comment thread scripts/perf-baseline.mjs
Comment thread tests/test_plugin_src_route.py Outdated
….path inserts

- scripts/perf-baseline.mjs: createRequire(import.meta.url) instead of a
  directory URL (still resolves @playwright/test via upward traversal).
- tests/test_plugin_src_route.py: drop the sys.path inserts — pyproject's
  pytest pythonpath=[".","lib"] already makes `plugins` importable, and the
  global inserts persisted for the whole run.

(Kept _if_none_match as a strict compare — we serve one weak ETag the browser
echoes verbatim; weak/strong/wildcard matching is dead for these GETs in a
single-user self-hosted app.)

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@byrongamatos

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@byrongamatos

byrongamatos commented Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@byrongamatos

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (7)
eslint.config.js (1)

22-25: 📐 Maintainability & Code Quality | 🔵 Trivial

Magic-number "unlimited" ceiling.

100000 is a de facto "no ceiling" for the two screen.js exemptions, but a named constant (e.g. const NO_CEILING = Number.MAX_SAFE_INTEGER or Infinity) would make the intent explicit rather than relying on readers inferring "huge number = unlimited."

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@eslint.config.js` around lines 22 - 25, The two `screen.js` ESLint exemptions
in `eslint.config.js` use a magic number to represent an effectively unlimited
max, so replace the hardcoded `100000` values with a named constant such as
`NO_CEILING` defined near the config and reused in both entries. Update the
`files` rules for `plugins/capability_inspector/screen.js` and
`plugins/folder_library/screen.js` to reference that constant so the intent is
explicit and easy to maintain.
package.json (1)

15-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Track the ESLint v10 bump soon. eslint-plugin-import-x supports ESLint v10, so the upgrade path isn’t blocked here.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@package.json` around lines 15 - 16, The dependency set in package.json is
still pinned to ESLint v9, so plan and apply the ESLint v10 upgrade soon. Update
the eslint entry in the package manifest and verify any related lint tooling,
including eslint-plugin-import-x integration, still works with the new major
version; use the package.json dependency list as the main place to locate the
change.
scripts/perf-baseline.mjs (3)

53-70: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Wrap browser lifecycle in try/finally to avoid orphaned Chromium processes.

If page.goto (60s timeout) or any page.evaluate call throws, browser.close() at Line 68 never runs, leaking a Chromium process on every failed run.

♻️ Proposed fix
 async function clientMetrics() {
     const browser = await chromium.launch();
-    const page = await browser.newPage();
-    const t0 = Date.now();
-    await page.goto(BASE, { waitUntil: 'networkidle', timeout: 60000 });
-    const bootMs = Date.now() - t0;
+    try {
+        const page = await browser.newPage();
+        const t0 = Date.now();
+        await page.goto(BASE, { waitUntil: 'networkidle', timeout: 60000 });
+        const bootMs = Date.now() - t0;

-    // performance.memory is Chromium-only; JS heap after settle.
-    const mem0 = await page.evaluate(() => (performance.memory ? performance.memory.usedJSHeapSize : null));
-    await page.waitForTimeout(SOAK_S * 1000);
-    const mem1 = await page.evaluate(() => (performance.memory ? performance.memory.usedJSHeapSize : null));
+        // performance.memory is Chromium-only; JS heap after settle.
+        const mem0 = await page.evaluate(() => (performance.memory ? performance.memory.usedJSHeapSize : null));
+        await page.waitForTimeout(SOAK_S * 1000);
+        const mem1 = await page.evaluate(() => (performance.memory ? performance.memory.usedJSHeapSize : null));

-    const scripts = await page.evaluate(() =>
-        document.querySelectorAll('script[data-plugin-id]').length);
+        const scripts = await page.evaluate(() =>
+            document.querySelectorAll('script[data-plugin-id]').length);

-    await browser.close();
-    return { bootMs, memStartMB: mem0 && mem0 / 1048576, memSoakMB: mem1 && mem1 / 1048576, scripts };
+        return { bootMs, memStartMB: mem0 && mem0 / 1048576, memSoakMB: mem1 && mem1 / 1048576, scripts };
+    } finally {
+        await browser.close();
+    }
 }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@scripts/perf-baseline.mjs` around lines 53 - 70, The clientMetrics() flow in
the perf-baseline script should guarantee browser cleanup even when page.goto or
either page.evaluate call throws. Wrap the Chromium launch/newPage work in a
try/finally block and move browser.close() into the finally path so the browser
is always closed regardless of failures; keep the existing clientMetrics()
return shape and timing/memory collection logic unchanged.

33-50: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

No timeout on server-latency fetches.

If BASE is unreachable or hangs, fetch(BASE + path) at Line 41 can block the loop indefinitely (Node's default fetch has no timeout), stalling the whole harness with no output. Consider an AbortController with a bounded timeout so a hung endpoint degrades to a recorded failure rather than a hang.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@scripts/perf-baseline.mjs` around lines 33 - 50, The serverLatency() helper
currently calls fetch(BASE + path) without any timeout, so a hung or unreachable
endpoint can stall the perf harness indefinitely. Update serverLatency() to use
an AbortController (or equivalent bounded timeout) around the fetch call, and
treat timeout aborts as a recorded failure by setting status appropriately and
continuing the loop, so the existing per-path timing rows still get produced.

15-17: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Declare the Node version floor for this script. package.json has no engines field, while this .mjs entrypoint uses top-level await and CI is pinned to Node 20. Add an explicit engine requirement so local and CI expectations stay aligned.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@scripts/perf-baseline.mjs` around lines 15 - 17, This script relies on a
minimum Node runtime, so add an explicit engine floor to keep local and CI
expectations aligned. Update the package metadata to declare the required Node
version for the perf-baseline entrypoint, and make sure it matches the top-level
await usage in the script and the Node 20 CI pin. Reference the
perf-baseline.mjs entrypoint and the package.json engines configuration when
making the change.
docs/perf-baseline.md (1)

9-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a language to the fenced code block (MD040).

Static analysis flags this block as missing a language tag.

📝 Proposed fix
-```
+```sh
 # 1. start core against a library with real charts (see caveat below)
 CONFIG_DIR=… DLC_DIR=/path/to/songs PYTHONPATH=lib \
   python3 -m uvicorn server:app --host 127.0.0.1 --port 8000
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/perf-baseline.md` around lines 9 - 16, The fenced code block in the perf
baseline docs is missing a language tag, triggering MD040. Update the markdown
fence around the example commands in the perf baseline section to include an
appropriate shell language identifier, using the existing fenced block in
docs/perf-baseline.md as the target.

Source: Linters/SAST tools

plugins/__init__.py (1)

2444-2497: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider extracting the shared "serve file from plugin subtree" logic.

plugin_asset and plugin_src now duplicate the same safe_joinis_file()_plugin_file_response(_plugin_media_type(...)) sequence almost verbatim, differing only in subdirectory name and log label. Since both were touched together in this PR, extracting a small shared helper would remove the duplication and prevent the two routes from silently drifting apart in future edits.

♻️ Proposed helper extraction
+def _serve_plugin_subtree_file(
+    request: Request, plugin_dir: Path, subdir: str, rel_path: str, plugin_id: str, kind: str
+) -> Response | None:
+    """Shared safe_join + conditional-cache serving for a plugin's assets/ or
+    src/ subtree. Returns None (caller should 404) if the path is rejected or
+    missing."""
+    target = safe_join(plugin_dir / subdir, rel_path)
+    if target is None:
+        log.warning("Plugin %r: %s path rejected: %r", plugin_id, kind, rel_path)
+        return None
+    if target.is_file():
+        return _plugin_file_response(request, target, _plugin_media_type(target))
+    return None
+
+
 `@app.get`("/api/plugins/{plugin_id}/assets/{asset_path:path}")
 def plugin_asset(request: Request, plugin_id: str, asset_path: str):
     ...
     for p in snapshot:
         if p["id"] == plugin_id:
             if p.get("status", "ready") != "ready":
                 break
-            target = safe_join(p["_dir"] / "assets", asset_path)
-            if target is None:
-                log.warning("Plugin %r: asset path rejected: %r", plugin_id, asset_path)
-                break
-            if target.is_file():
-                return _plugin_file_response(request, target, _plugin_media_type(target))
-            break
+            resp = _serve_plugin_subtree_file(request, p["_dir"], "assets", asset_path, plugin_id, "asset")
+            if resp is not None:
+                return resp
+            break
     return Response("", status_code=404)

Same pattern applies to plugin_src.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@plugins/__init__.py` around lines 2444 - 2497, Extract the duplicated plugin
file-serving flow in plugin_asset and plugin_src into a shared helper so both
routes use the same safe_join, is_file, and
_plugin_file_response/_plugin_media_type logic. Keep the helper parameterized by
the base subtree and log label, then have both plugin_asset and plugin_src call
it so their behavior stays identical and future changes only happen in one
place.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In @.github/workflows/ci.yml:
- Line 136: The new lint job’s checkout currently uses the default
actions/checkout@v4 behavior, which persists the GitHub token in local git
config; update the checkout step to disable credential persistence by setting
persist-credentials to false. Make this change on the lint job’s checkout in the
workflow so subsequent steps like npm ci cannot access the token, and keep the
rest of the job unchanged.

In `@docs/perf-baseline.md`:
- Line 15: The perf baseline document has a mismatch between the recorded run
header and the example command: the `Running it` invocation uses `--n 60`, while
the R0 baseline summary says `n=50`. Verify the actual harness value used for
the recorded run, then update the baseline header in the `R0 baseline` section
(and the example command if needed) so the documented `n` matches the real run.

In `@docs/plugin-modules.md`:
- Around line 47-50: The asset-path example in the plugin module docs uses a
module-relative URL that points to the wrong location from files like
src/main.js. Update the example in the assets/import.meta.url guidance to show a
path relative to the importing module, such as ../assets/x.js from src/main.js,
or use the absolute /api/plugins/<id>/assets/x.js route. Keep the references to
import.meta.url and assets/ clear so readers can locate the correct pattern.

---

Nitpick comments:
In `@docs/perf-baseline.md`:
- Around line 9-16: The fenced code block in the perf baseline docs is missing a
language tag, triggering MD040. Update the markdown fence around the example
commands in the perf baseline section to include an appropriate shell language
identifier, using the existing fenced block in docs/perf-baseline.md as the
target.

In `@eslint.config.js`:
- Around line 22-25: The two `screen.js` ESLint exemptions in `eslint.config.js`
use a magic number to represent an effectively unlimited max, so replace the
hardcoded `100000` values with a named constant such as `NO_CEILING` defined
near the config and reused in both entries. Update the `files` rules for
`plugins/capability_inspector/screen.js` and `plugins/folder_library/screen.js`
to reference that constant so the intent is explicit and easy to maintain.

In `@package.json`:
- Around line 15-16: The dependency set in package.json is still pinned to
ESLint v9, so plan and apply the ESLint v10 upgrade soon. Update the eslint
entry in the package manifest and verify any related lint tooling, including
eslint-plugin-import-x integration, still works with the new major version; use
the package.json dependency list as the main place to locate the change.

In `@plugins/__init__.py`:
- Around line 2444-2497: Extract the duplicated plugin file-serving flow in
plugin_asset and plugin_src into a shared helper so both routes use the same
safe_join, is_file, and _plugin_file_response/_plugin_media_type logic. Keep the
helper parameterized by the base subtree and log label, then have both
plugin_asset and plugin_src call it so their behavior stays identical and future
changes only happen in one place.

In `@scripts/perf-baseline.mjs`:
- Around line 53-70: The clientMetrics() flow in the perf-baseline script should
guarantee browser cleanup even when page.goto or either page.evaluate call
throws. Wrap the Chromium launch/newPage work in a try/finally block and move
browser.close() into the finally path so the browser is always closed regardless
of failures; keep the existing clientMetrics() return shape and timing/memory
collection logic unchanged.
- Around line 33-50: The serverLatency() helper currently calls fetch(BASE +
path) without any timeout, so a hung or unreachable endpoint can stall the perf
harness indefinitely. Update serverLatency() to use an AbortController (or
equivalent bounded timeout) around the fetch call, and treat timeout aborts as a
recorded failure by setting status appropriately and continuing the loop, so the
existing per-path timing rows still get produced.
- Around line 15-17: This script relies on a minimum Node runtime, so add an
explicit engine floor to keep local and CI expectations aligned. Update the
package metadata to declare the required Node version for the perf-baseline
entrypoint, and make sure it matches the top-level await usage in the script and
the Node 20 CI pin. Reference the perf-baseline.mjs entrypoint and the
package.json engines configuration when making the change.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: b691d7c9-7c05-462a-9cde-e3dfe9e28c7f

📥 Commits

Reviewing files that changed from the base of the PR and between a18a818 and ffe8e6e.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (14)
  • .github/workflows/ci.yml
  • .specify/memory/constitution.md
  • CHANGELOG.md
  • CLAUDE.md
  • docs/perf-baseline.md
  • docs/plugin-modules.md
  • docs/size-exemptions.md
  • eslint.config.js
  • package.json
  • plugins/__init__.py
  • scripts/perf-baseline.mjs
  • static/app.js
  • tests/js/plugin_loader_script_type.test.js
  • tests/test_plugin_src_route.py

Comment thread .github/workflows/ci.yml
Comment thread docs/perf-baseline.md
Comment thread docs/plugin-modules.md Outdated
- .github/workflows/ci.yml: persist-credentials: false on the lint job's
  checkout (it runs npm ci with third-party postinstall scripts and never
  pushes) — zizmor artipacked.
- docs/plugin-modules.md: correct the import.meta.url asset example — assets/ is
  at the plugin root, so a src/ module needs `../assets/x.js` (or the absolute
  /api/plugins/<id>/assets/ route), not `assets/x.js` (which resolves under src/).
- docs/perf-baseline.md: note the recorded R0 numbers were a quick --n 50
  --soak 8 pass; the recommended run stays --n 60 --soak 30.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants