fix: unblock develop CI — plugin TS patches, agent-skills build, buf.gen guard

[dutchiono]

Summary

This PR is the single recovery branch for the current develop CI unblock work. It supersedes #1770 and #1772.

Scope kept in this cleaned branch:

• apply in-repo compatibility patches to plugin-anthropic, plugin-google-genai, and plugin-personality after submodule checkout via scripts/patch-workspace-plugins.mjs
• build plugin-agent-skills during repo setup so workspace:* consumers have a bundled dist/index.js in CI
• guard buf.gen.yaml generation in test.yml so stale runners do not fail protobuf generation when schemas are absent
• keep Windows/fork CI parity changes for MILADY_SKIP_LOCAL_UPSTREAMS
• remove the dangling plugins/plugin-groq gitlink
• keep the steward-fi and test/contracts/lib/openzeppelin-contracts submodule repairs because the old develop SHAs are not fetchable from GitHub
• keep the refreshed bun.lock that matches these manifests under frozen install

Intentionally removed from this PR during cleanup:

• unrelated app-core and runtime behavior changes
• Hyperscape route/session work
• plugin gitlink bumps to unpublished commits for plugin-agent-skills, plugin-google-genai, and plugin-personality

Plugin compatibility in this PR is carried by the patch files under scripts/workspace-plugin-patches/, not by advancing plugin submodules to unpublished SHAs.

Consolidation

This PR now carries the recovery work that had been split across:

• #1770 fix: harden Windows repo setup
• #1772 fix: repair unreachable submodule refs
• #1774 fix: unblock develop CI — plugin TS patches, agent-skills build, buf.gen guard

Review #1774 only.

Validation

• bun install --frozen-lockfile --ignore-scripts
• bunx vitest run scripts/patch-workspace-plugins.test.ts scripts/run-repo-setup.test.ts scripts/setup-upstreams.test.ts scripts/init-submodules.test.ts packages/app-core/src/runtime/init-submodules-script.test.ts
• bun run typecheck
• bun run check still hits the pre-existing Biome docs-ignore issue under apps/web/src/docs/content/**
• GitHub Actions rerun on cleaned #1774

[github-actions]

I was unable to post the approval to GitHub (write operations require user approval). Here is the full review verdict:

---

Agent Review — PR #1774

---

1. Classification (derived): workflow/CI + build infrastructure (bugfix) — consistent with author's claim. The diff is purely scripts, workflows, lock file, and git plumbing. No runtime, UI, or connector code touched.

2. Rubric (derived): CI workflow correctness (matrix coverage, Windows safety after submodule consolidation), postinstall script soundness (ordering, idempotency, failure modes), and lock file hygiene.

3. Scope verdict: in scope

4. Universal invariants: all intact — packages/agent/src/runtime/eliza.ts, scripts/run-node.mjs, apps/app/electrobun/src/native/agent.ts, scripts/patch-deps.mjs, ports, dynamic imports, uiShellMode, StartupPhase, VrmViewer, RPC schema, dev observability, and access control files are all untouched.

---

5. Judgment:

• Needed? — OK. Four distinct, documented CI failures: stale buf.gen.yaml guard, ERR_MODULE_NOT_FOUND for plugin-agent-skills, Windows submodule init inconsistency, dangling plugin-groq gitlink.
• Better than existing? — OK. buf.gen.yaml guard is strictly additive. init-submodules.mjs unification removes duplicate matrix-branch logic. Patcher is idempotent.
• Duplication? — OK. No existing equivalent for workspace patching. ensure-bundled-workspaces.mjs entry mirrors the plugin-agent-orchestrator pattern exactly.
• Blast radius covered? — OK. All three Generate protobuf types steps in test.yml updated. All four Windows-specific submodule init steps removed across the four affected workflow files. run-repo-setup.mjs step ordering correct (submodules → patch → patch-deps → build).
• Logic sound? — OK. applyPatch checks --reverse first (already-applied guard), then --check (inapplicable guard), then applies. Exits 1 on failure. init-submodules.mjs already has SKIP_SUBMODULES = new Set(["plugins/plugin-openrouter"]) — the Windows-unsafe submodule — verified before accepting the Windows consolidation.
• Complexity appropriate? — OK. 156-line patcher for idempotent git apply iteration. No speculative abstractions.
• Tested meaningfully? — OK with caveat. Four unit tests cover the mapping and resolvePluginDir prefix matching including edge cases. applyPatch itself is not unit-tested (wraps git directly, failure modes logged + propagated) — acceptable for a postinstall script.
• Matches conventions? — OK. Mirrors init-submodules.mjs/ensure-bundled-workspaces.mjs patterns. Exports pure functions for testability. No any, no secrets, no new runtime deps.
• Plausible breakage mode: init-submodules.mjs omits --depth 1 (old Windows steps used shallow clones). On slow runners this increases checkout time but does not cause failures. If plugin-agent-skills build fails on its own TS errors, ensure-bundled-workspaces fails hard — but the PR's patches address those errors upstream.

---

6. PR-type-specific checks:

• buf.gen.yaml guard added to all three relevant steps in test.yml — OK
• Windows submodule unification: init-submodules.mjs Windows safety verified (plugin-openrouter already in SKIP_SUBMODULES; depth-1 removal is perf trade-off, not correctness) — OK
• patch-workspace-plugins.mjs position in run-repo-setup.mjs is correct — OK
• plugin-agent-skills entry in ensure-bundled-workspaces.mjs follows established pattern — OK
• plugins/plugin-groq dangling gitlink removed; confirmed absent from .gitmodules — OK
• bun.lock: plugin-groq/plugin-openrouter workspace entries removed (consistent with gitlink removal and SKIP_SUBMODULES); @miladyai/plugin-2004scape confirmed already in packages/agent/package.json (stale lock, correct dedup) — OK
• Lock version bumps: bun 1.3.11 transitive dedup, no Milady-owned packages affected — OK
• Patch files in-repo under scripts/workspace-plugin-patches/; no external fetch at postinstall — OK

---

7. Security: clear. No credentials, no external network calls, no auth changes, no access control files touched. execSync with in-repo filenames is acceptable.

---

8. Decision: APPROVE

Five independent changes, each clearly motivated and correctly implemented. The Windows submodule consolidation is safe given the existing SKIP_SUBMODULES guard. The patcher is idempotent and designed to be removed incrementally as upstream PRs land. Boring, correct infrastructure work.

[github-actions]

The review is complete. Here's the summary for the record (pending tool approval to post):

---

1. Classification (derived): workflow/CI + build infrastructure — consistent with author's claim.

2. Rubric (derived): CI reliability, postinstall pipeline ordering, patch system idempotency, and whether submodule pointer changes are intentional and safe.

3. Scope verdict: in scope

4. Universal invariants: all intact

5. Judgment:

• Needed? — OK
• Better than existing? — OK (Windows unification is a real improvement)
• Duplication? — OK
• Blast radius covered? — CONCERN: steward-fi and test/contracts/lib/openzeppelin-contracts submodule bumps are undocumented and unrelated to the stated fix. openzeppelin-contracts is a security-sensitive contract library.
• Logic sound? — OK (reverse-check → forward-check → apply ordering is correct)
• Complexity appropriate? — OK
• Tested meaningfully? — OK
• Matches conventions? — OK
• Plausible breakage mode: Stale patches silently persist if submodule pointer isn't bumped after upstream merges the fix.

6. PR-type checks: All CI/workflow items OK; submodule bumps flagged.

7. Security: The undocumented steward-fi and openzeppelin-contracts submodule bumps are the only concern — supply chain hygiene issue for a security-sensitive library.

8. Decision: REQUEST CHANGES

Single required change: explain or revert the two unrelated submodule pointer bumps. Everything else — buf.gen guard, plugin-agent-skills build, Windows unification, groq cleanup, patch runner — is correct and approvable.

[github-actions]

I need approval to post to GitHub. Here is my complete review:

---

Code Review — PR #1774

---

1. Classification (derived): workflow/CI + build — consistent with author's claim. The PR touches workflow YAML, a new postinstall script, patch files, bun.lock, and a submodule removal. All CI/build infrastructure; no runtime code changed.

2. Rubric (derived): Verify workflow changes are correct across all platforms; verify patch-workspace-plugins.mjs is safe and idempotent; check the lockfile delta for surprises; confirm no unexplained side-effects are smuggled in alongside the stated fixes.

3. Scope verdict: in scope

4. Universal invariants: all intact — this PR doesn't touch eliza.ts, run-node.mjs, agent.ts, patch-deps.mjs, RPC schema, access control files, or any guarded invariant sites.

---

5. Judgment:

• Needed? — OK. Three distinct real CI failures addressed: stale-runner buf.gen.yaml missing, plugin-agent-skills ERR_MODULE_NOT_FOUND at test runtime, and upstream TS incompatibilities blocking type-check jobs. plugin-groq dangling gitlink removal fixes fatal: No url found for submodule on every foreach.
• Better than existing? — OK. buf.gen.yaml guard is minimal and correct. ensure-bundled-workspaces addition mirrors the existing plugin-agent-orchestrator entry exactly. Patch infrastructure is the right approach for temporary upstream compat.
• Duplication? — OK. patch-workspace-plugins.mjs doesn't duplicate patch-deps.mjs (which patches npm packages; this patches git submodule checkouts).
• Blast radius covered? — CONCERN: Two unexplained submodule pointer bumps (see section 6). Otherwise covered: run-repo-setup.mjs wires the new script in, lockfile regenerated, workflows updated consistently.
• Logic sound? — OK. applyPatch checks --check --reverse before applying, falls through gracefully on inapplicable, skips on missing submodule, exits 1 only on actual failures. Ordering in run-repo-setup.mjs (init-submodules → patch-workspace-plugins → patch-deps) is correct.
• Complexity appropriate? — OK. 156-line script, well-scoped responsibility.
• Tested meaningfully? — CONCERN (not blocking): Tests cover only the pure resolvePluginDir mapping. applyPatch is untested — acceptable since it wraps git apply which resists unit testing.
• Matches conventions? — CONCERN (minor): .patch files embed Co-Authored-By: Claude Sonnet 4.6 in their upstream commit headers — technically not in Milady's own commit message, so doesn't violate CLAUDE.md. Also: exec(\git apply "${patchPath}"`)uses shell string interpolation;spawnSync` with an array would be safer.
• Plausible breakage mode: Windows CI now uses init-submodules.mjs without --depth 1. First Windows run will do a full checkout (slower but not broken). SKIP_SUBMODULES already handles the Windows-problematic plugin-openrouter. Low probability of real breakage.

---

6. PR-type-specific checks:

• buf.gen.yaml guard in all 3 Generate protobuf types steps — OK
• plugin-agent-skills build mirrors plugin-agent-orchestrator pattern — OK
• Windows submodule init consolidated across all 4 workflow locations — OK
• MILADY_SKIP_LOCAL_UPSTREAMS: "1" in ci-fork.yml — OK, correct for fork environments
• plugins/plugin-groq gitlink deleted, bun.lock cleaned consistently — OK
• @elizaos/plugin-openrouter resolves from npm instead of workspace — OK, consistent with existing SKIP_SUBMODULES entry for Windows compatibility
• steward-fi pointer bumped (c0883fd00 → 3a6b97b13) — CONCERN: Not mentioned in PR description. Unrelated to CI fixes.
• test/contracts/lib/openzeppelin-contracts pointer bumped (99a8f4297 → 9cfdccd35) — CONCERN: Same. Bumping openzeppelin-contracts without explanation in a CI-unblock PR is not acceptable.
• Patch files are targeted: EventPayload cast, ModelType enum, z.record key schema, JSON.stringify for nested MetadataValue — OK

---

7. Security: clear. git apply string interpolation uses only repo-controlled filenames. Patch files make narrow TS-only changes.

---

8. Decision: REQUEST CHANGES

Required before merge:

1. Explain or revert the steward-fi and test/contracts/lib/openzeppelin-contracts submodule pointer bumps. These are unrelated to CI fixes. If accidentally included (e.g. unintentional git submodule update during branch prep), revert them. If intentional, they belong in their own PR with rationale. A CI-unblock PR must not carry unexplained submodule pointer changes.

Optional (not blocking):

2. Document the plugin-openrouter npm-fallback behavior change in the PR description.
3. Switch execSync(\git apply "${patchPath}"`)tospawnSync` with an arg array.

[github-actions]

The tool requires explicit approval to post to GitHub. Here is my complete review to post:

---

1. Classification (derived): workflow/CI + build — consistent with author's claim. Five distinct CI failure vectors fixed; no runtime logic touched.

2. Rubric (derived): Verify each claimed fix, confirm postinstall ordering is safe, check Windows submodule consolidation for regressions, inspect patch shell-invocation for injection risk, confirm no invariant sites touched.

3. Scope verdict: in scope

4. Universal invariants: all intact — NODE_PATH, patch-deps.mjs, Electrobun guards, namespace, ports, StartupPhase, VrmViewer, RPC schema, access control files all untouched.

5. Judgment:

• Needed? — OK. Four traceable CI failures, no speculation.
• Better than existing? — OK. buf.gen.yaml guard strictly improves the old directory-only check; patch-workspace-plugins is idempotent.
• Duplication? — OK.
• Blast radius covered? — CONCERN: windows-dev-smoke.yml and windows-desktop-preload-smoke.yml (see below).
• Logic sound? — OK. postinstall order is correct; applyPatch covers all three states.
• Complexity appropriate? — OK.
• Tested meaningfully? — OK. resolvePluginDir tests cover the mapping and boundary cases.
• Matches conventions? — OK.
• Plausible breakage mode: A future submodule with Windows-unfriendly paths would fail silently inside init-submodules.mjs (exits 0 on failure), potentially missing submodule content in downstream steps.

6. PR-type-specific checks:

• buf.gen.yaml guard (3 instances): OK — correctly handles stale Blacksmith runners with partial eliza checkout.
• plugin-agent-skills in ensure-bundled-workspaces: OK — packages/agent/package.json:124 confirms workspace:*; bun.lock confirms plugins/plugin-agent-skills/typescript is the correct workspace path.
• patch-workspace-plugins.mjs integration: OK — inserted between init-submodules and patch-deps in run-repo-setup.mjs; runs via bun run postinstall in CI before build steps.
• plugin-groq gitlink removal: OK — .gitmodules has no entry for plugins/plugin-groq, confirming the dangling gitlink claim.
• Windows consolidation — test.yml and task-agent-cross-platform-review.yml: OK — both have MILADY_SKIP_LOCAL_UPSTREAMS: "1" globally, so eliza is skipped; plugin-openrouter is in SKIP_SUBMODULES.
• Windows consolidation — windows-dev-smoke.yml and windows-desktop-preload-smoke.yml: CONCERN — neither sets MILADY_SKIP_LOCAL_UPSTREAMS at job/workflow env level. init-submodules.mjs would attempt to init eliza on fresh Windows runners. Mitigated by: init-submodules exits 0 on failure, Blacksmith runners cache eliza/ readiness markers, actual smoke-test functionality is preserved. Non-breaking but imprecise — the original --depth 1 plugins/plugin-agent-orchestrator approach was tighter.
• shell injection in patch-workspace-plugins.mjs: Low risk. execSync is shell-spawned; patchPath is constructed from committed filenames behind a prefix guard. Theoretical, not practical.
• bun.lock: OK — changes explained by plugin-groq removal, bun 1.3.11 deduplication, and pre-existing @miladyai/plugin-2004scape dep surfaced in the rewrite.
• steward-fi + openzeppelin pointer bumps: Unrelated to stated CI fixes, not in PR description. Not blocking.

7. Security: clear.

8. Decision: APPROVE

The four primary CI fixes are correct and verifiable. The Windows consolidation is imprecise (MILADY_SKIP_LOCAL_UPSTREAMS missing at job level in the two smoke workflows) but non-breaking. Post-merge: add MILADY_SKIP_LOCAL_UPSTREAMS: "1" at job env level in windows-dev-smoke.yml and windows-desktop-preload-smoke.yml.

[github-actions]

The review is complete. Here is my full verdict:

---

PR #1774 Review — fix: unblock develop CI
<!-- agent-review-run:24207674447:1:014ddcc3b4191029b2c67c3bb9f9c4d3d8c262cb -->

---

1. Classification (derived): workflow/CI + build — consistent with author's claim.

2. Rubric (derived): CI workflow correctness (platform matrix integrity, buf.gen guard), postinstall script safety and ordering, lockfile delta, Windows branch removal soundness.

3. Scope verdict: in scope

4. Universal invariants: all intact — NODE_PATH (3 sites), patch-deps.mjs, electrobun try/catch guards, namespace, ports all untouched by this PR.

5. Judgment:

• Needed? — OK. Four concrete CI failures, each fix directly traceable to a failure mode.
• Better than existing? — OK. buf.gen.yaml guard is minimal and correct. patch-workspace-plugins.mjs is strictly additive. Windows workflow unification is cleaner.
• Duplication? — OK. plugin-agent-skills entry in ensure-bundled-workspaces.mjs mirrors the existing orchestrator entry exactly.
• Blast radius covered? — CONCERN: steward-fi and test/contracts/lib/openzeppelin-contracts submodule pointer bumps appear in the diff and are not mentioned in the PR description. Unrelated to any stated CI fix. Looks like accidental git submodule update noise from the contributor's checkout. Needs explanation or revert before merge.
• Logic sound? — OK. applyPatch flow: reverse-check → forward-check → apply, graceful skip at each step, non-zero exit only on hard failures. resolvePluginDir prefix match with trailing - prevents partial collisions (test covers this). MILADY_SKIP_LOCAL_UPSTREAMS: "1" in ci-fork.yml is correct.
• Complexity appropriate? — OK. 156-line script, clear separation of concerns, no over-abstraction.
• Tested meaningfully? — OK. patch-workspace-plugins.test.ts covers mapping, null returns, and partial-prefix rejection. run-repo-setup.test.ts updated to enforce step ordering.
• Matches conventions? — OK. Node ESM, no any, no hardcoded ports, elizaOS branding correct.
• Plausible breakage mode: Windows workflow changes now rely on init-submodules.mjs for all platforms. SKIP_SUBMODULES only contains plugins/plugin-openrouter. If any of the 25+ other tracked submodules has Windows-incompatible filenames (colons, trailing dots), Windows CI fails. The old conservative approach (enumerate only safe submodules) was resistant to this failure class. Not a blocker — plugin-openrouter was the known problematic case — but a latent fragility worth monitoring.

6. PR-type-specific checks:

• buf.gen.yaml guard applied to all three protobuf steps in test.yml: OK
• plugin-agent-skills build entry follows plugin-agent-orchestrator pattern (correct typescript/ subpath): OK
• patch-workspace-plugins.mjs inserted after init-submodules, before patch-deps in step order: OK
• Co-Authored-By in .patch files: OK — these are git-formatted patch content for upstream git am use, not Milady commit metadata
• Dangling plugins/plugin-groq gitlink removal: OK — confirmed: no .gitmodules entry for groq on develop
• MILADY_SKIP_LOCAL_UPSTREAMS: "1" in ci-fork.yml: OK
• @miladyai/plugin-2004scape in bun.lock: OK — already in packages/agent/package.json; lockfile deduplication artifact, not a new dependency
• steward-fi / openzeppelin-contracts submodule bumps: CONCERN — unexplained

7. Security: clear. No credential exposure, no outbound HTTP, execSync in patch-workspace-plugins.mjs uses repo-controlled filenames — no injection surface.

8. Decision: APPROVE

The CI fixes are correct, well-structured, and tested. One pre-merge action: confirm whether the steward-fi and test/contracts/lib/openzeppelin-contracts pointer bumps are intentional (with new commit hashes reviewed) or accidental local noise that should be stripped.

<!-- verdict-status: final -->

[github-actions]

The gh pr review and gh pr comment commands require approval to run. Here is the complete review I prepared — ready to post once approved, or you can paste it manually:

---

PR #1774 Review — REQUEST CHANGES

---

1. Classification (derived): workflow/CI + runtime/feature — reclassified. The title advertises a CI unblock but the diff includes a new 286-line Hyperscape built-in route module (hyperscape.ts), runtime canonicalization logic in app-manager.ts, Hyperscape config in registry-client-app-meta.ts, two unexplained submodule bumps (steward-fi, openzeppelin-contracts), and 12 additional plugins added to ensure-bundled-workspaces.mjs that aren't mentioned.

2. Rubric (derived): CI workflow correctness (artifact gating, Windows init), postinstall sequencer integrity, patch-application safety, Hyperscape route-module logic, runtime behavior changes in app-manager, unexplained submodule pointer bumps.

3. Scope verdict: in scope — but disclosure is incomplete.

4. Universal invariants: all intact — NODE_PATH (all three sites), patch-deps, Electrobun startup guards, namespace, ports, access-control files, StartupPhase all untouched or preserved.

5. Judgment:

   • Needed? — OK for the core CI fixes. CONCERN for the steward-fi / openzeppelin-contracts bumps: zero explanation — looks like accidental branch contamination.
   • Better than existing? — OK. init-submodules.mjs on Windows is strictly simpler than the manual submodule list.
   • Duplication? — OK.
   • Blast radius covered? — CONCERN: ensure-bundled-workspaces.mjs now builds 13 plugins; PR body justifies exactly one. bun.lock gains @miladyai/plugin-2004scape workspace dep not mentioned anywhere.
   • Logic sound? — CONCERN: logic bug in hyperscape.ts buildSession:

     const controls = isRunning ? ["pause"] : ["pause", "resume"];

     When agent is NOT running, showing both "pause" and "resume" is wrong. Not-running → ["resume"] only.
   • Complexity appropriate? — OK. pluginEntry() helper and the patch infrastructure are proportionate.
   • Tested meaningfully? — OK. Patch tests, setup-order snapshot, MILADY_STATE_DIR isolation fix, app-manager test fix all meaningful.
   • Matches conventions? — OK.
   • Plausible breakage mode: ensure-bundled-workspaces.mjs now tries to build 12 additional plugins on cold CI runners. One flaky plugin build failure blocks all CI jobs. 12× more surface than before.

6. PR-type-specific checks:

   Check	Status
   buf.gen.yaml guard in all three Generate protobuf types steps	OK
   Windows submodule consolidation to init-submodules.mjs	OK
   MILADY_SKIP_LOCAL_UPSTREAMS: "1" in ci-fork.yml	OK
   plugin-groq gitlink removal (no .gitmodules, no dep)	OK
   patch-workspace-plugins.mjs shell-injection via "${patchPath}"	CONCERN (low): repo-controlled filenames so severity is low, but array-arg execSync is safer
   patch-workspace-plugins.mjs ordering (init → TS patches → bun-exports → upstreams → build)	OK
   resolveLaunchSession await + null fallthrough fix	OK — genuine bug fix
   canonicalizeCuratedRegistryPlugin npm.package guard	OK — correctly preserves Hyperscape's separate runtime plugin name
   steward-fi submodule bump explained	CONCERN: unexplained
   openzeppelin-contracts bump explained	CONCERN: unexplained
   resolveApiBase URL validation	OK — protocol restricted to http/https
   Outbound HTTP in fetchLiveData to HYPERSCAPE_API_URL	OK — user-configured, consistent with existing pattern

7. Security: clear. No credential exposure. Protocol-validated outbound HTTP. Repo-controlled patch files. No auth/loopback changes.

8. Decision: REQUEST CHANGES

Required before merge:

1. Fix buildSession controls in hyperscape.ts: !isRunning branch → ["resume"], not ["pause", "resume"].
2. Explain or revert steward-fi and openzeppelin-contracts bumps. Silent pointer moves with no audit trail are a merge risk.
3. Account for all 13 plugins in ensure-bundled-workspaces.mjs or scope-trim to the actually-failing ones. PR body covers one.
4. Confirm @miladyai/plugin-2004scape in bun.lock is a bun 1.3.11 deduplication of a pre-existing dep, not a new silent addition.

The CI infrastructure changes (buf guard, Windows consolidation, patch mechanism, groq cleanup, bun.lock dedup) are clean and correct. The resolveLaunchSession null-fallthrough and canonicalizeCuratedRegistryPlugin fixes are genuine and well-structured. Only the four items above need resolving before this can merge.

<!-- agent-review-run:24211342705:1:457d96e8f49f1dd801fe666160684c79f6d50703 -->
<!-- verdict-status: final -->

[github-actions]

Now I have everything I need to write the review. Let me compose it.

⏳ Preliminary — awaiting Greptile deep review. A final weighted verdict will be posted once Greptile responds.

---

Review: PR #1774 — fix: unblock develop CI — plugin TS patches, agent-skills build, buf.gen guard

---

1. Classification (derived): Mixed — workflow/CI + build-infrastructure + feature + bugfix — reclassified. Author claims "bugfix/CI" but the diff adds 286 lines of new Hyperscape session-resolution code (built-in-app-routes/hyperscape.ts), a new registry override (launchUrl + viewer config), an import priority inversion in app-package-modules.ts, and new canonicalization logic. The CI fixes (buf.gen guard, buf-gen YAML, MILADY_STATE_DIR env, plugin-groq gitlink, Windows submodule unification, bun.lock dedup) are real and needed, but they share a commit stream with feature work.

2. Rubric (derived): CI workflow correctness (action safety, runner state), build infrastructure soundness (plugin patch idempotency, ensure-bundled-workspaces ordering), new feature security and logic (Hyperscape HTTP fetch, SSRF, controls logic), cross-cutting behavior changes (import priority, resolveLaunchSession null semantics), and Biome compliance — then trust-calibrated line-by-line on the new 286-line module.

3. Scope verdict: Needs deep review — Hyperscape route module is a new feature bundled with CI fixes; import priority inversion affects all app route resolution; probationary contributor.

4. Universal invariants: All intact

• NODE_PATH: present in all 3 sites (eliza.ts, run-node.mjs, agent.ts) — unchanged by this PR. ✓
• scripts/patch-deps.mjs: preserved and still in run-repo-setup.mjs step list. ✓
• Electrobun startup try/catch guards: untouched. ✓
• Namespace milady/MILADY_*: MILADY_SKIP_LOCAL_UPSTREAMS: "1" added to ci-fork.yml correctly. ✓
• No hardcoded ports: confirmed (31337 appears only in a test client constructor, not new code). ✓
• No top-level @elizaos/plugin-* imports: confirmed. ✓
• uiShellMode, StartupPhase, VrmViewer engineReady, RPC schema, dev observability endpoints, access control files: all untouched. ✓

5. Judgment:

• Needed? — OK for the CI parts (MILADY_STATE_DIR isolation fix, buf.gen.yaml guard, plugin-groq gitlink removal, bun.lock dedup, Windows submodule consolidation). CONCERN for Hyperscape route module: a 286-line new module was bundled with CI fixes. If the session-launch tests in app-manager.test.ts were already on develop and failing, this is completing in-progress work — but the PR should say so explicitly.

• Better than existing? — OK. The canonicalizeCuratedRegistryPlugin fix is demonstrably more correct (preserves intended npm.package when it differs from app name). The resolveLaunchSession null-fallthrough change is a genuine behavior improvement. The patch-workspace-plugins.mjs approach (idempotent git apply) is a reasonable bridge pattern while upstream PRs are open.

• Duplication? — OK. No reinvention of existing primitives detected.

• Blast radius covered? — CONCERN: missing explanation for import priority inversion. In app-package-modules.ts:importAppRouteModule, the PR swaps builtInModule and localModule check order — local now takes priority over built-in. This affects every app with a route module, not just Hyperscape. No comment explains why this is needed now vs. previously. The change is directionally correct (local overrides built-in) but the PR description doesn't mention it and no test validates the new ordering. Also: resolveLaunchSession null-fallthrough means any route module returning null now falls through to buildAppSession rather than propagating null to callers — existing callers that handled null explicitly now get a different object.

• Logic sound? — CONCERN: controls derivation in hyperscape.ts:buildSession.

  const controls = isRunning ? ["pause"] : ["pause", "resume"];

  When the agent is NOT running (status = "connecting"), both "pause" AND "resume" are offered. When running, only "pause". This seems backwards: a "connecting" agent isn't running yet, so offering "pause" has no logical target. The expected pattern would be ["resume"] when connecting/paused, ["pause"] when running. This needs explicit justification or is a bug.

• Complexity appropriate? — OK for the patch infrastructure. patch-workspace-plugins.mjs is well-structured (156 lines, clear idempotency contract). ensure-bundled-workspaces.mjs's new pluginEntry helper is pulled once and used 13 times — appropriate. The 286-line Hyperscape module is longer than necessary for session resolution but is structured cleanly.

• Tested meaningfully? — CONCERN (partial). patch-workspace-plugins.test.ts tests the directory mapping logic but cannot test actual patch application (no fixture submodule). The Hyperscape buildSession controls logic has no dedicated unit test that would catch the suspected reversal. app-manager.test.ts integration coverage for session launch exists but does not exercise the !isRunning control branch explicitly. The registry-client test fix (MILADY_STATE_DIR isolation) is well-targeted.

• Matches conventions? — CONCERN (minor). hyperscape.ts uses two as unknown as AppSessionJsonValue casts in buildSession for recommendedGoals and recentThoughts. These bypass type safety. TS strict mode should not require as unknown as for structured data that matches the schema — this typically means the shapes don't actually conform and the cast is hiding a type error. Should be investigated.

• Plausible breakage mode: Any app that previously relied on importAppRouteModule returning the built-in module when a local module also exists will silently switch to the local module. If a local route module exists and returns a bad session format, callers will no longer see null — they'll get the local module's output or fall through to buildAppSession, making bugs harder to diagnose.

6. PR-type-specific checks:

Check	Status
buf.gen.yaml guard added to all 3 Generate protobuf types steps	OK — all 3 sites updated consistently
MILADY_SKIP_LOCAL_UPSTREAMS=1 added to ci-fork.yml env	OK — correct MILADY prefix
Windows submodule init consolidated to init-submodules.mjs	OK — removes dead branch, but init-submodules.mjs must handle Windows paths; no evidence of regression
Plugin-groq gitlink removed from .gitmodules + bun.lock	OK — dangling gitlink, no .gitmodules entry, no package dep
ensure-bundled-workspaces.mjs: 13 new plugin builds	CONCERN — adds significant postinstall time in CI. Rationale is sound (ERR_MODULE_NOT_FOUND at test runtime) but no timing data provided. If any listed plugin lacks a dist/ on CI, these builds chain-fail silently
patch-workspace-plugins.mjs idempotency	OK — git apply --check --reverse guards are correct
Patch files are ASCII-clean git format patches	OK — standard format with commit headers
plugin-agent-skills in PLUGIN_PATCH_DIRS but no .patch file for it	OK — the entry exists for future patches; the actual agent-skills fix was a submodule bump
resolveLaunchSession null fallthrough	CONCERN — behavior change affects all route modules, not documented as intentional
Import priority (local > built-in)	CONCERN — silently changes resolution order for all apps
as unknown as AppSessionJsonValue casts	CONCERN — type safety bypass without explanation
Biome formatting changes	OK — purely mechanical, preserves logic
MILADY_STATE_DIR save/restore in registry-client.test.ts	OK — correct fix, prevents state dir pollution between tests
bun.lock dedup (parse5, tough-cookie, tldts hoisting)	OK — mechanical bun 1.3.11 dedup, no new transitive deps introduced
Access control files	OK — not touched

7. Security:

Partial concern. hyperscape.ts:resolveApiBase() validates that HYPERSCAPE_API_URL uses http: or https: protocol. However, it does not block SSRF to internal targets (e.g., http://169.254.169.254/, http://localhost:PORT/). Since this is a local-first app and the URL is set by the user/admin (not untrusted external input), the SSRF risk is low but not zero — a compromised config or prompt injection into the runtime setting path could redirect fetches to internal services. No auth is added to the four concurrent fetchJson calls, meaning any Hyperscape API endpoint is expected to be unauthenticated or auth is handled externally. This is acceptable for a local-first context but should be documented.

No credential exposure, no new hardcoded secrets, no postinstall script injection vectors. The patch files are checked-in git format patches with known content — not dynamic.

8. Decision: REQUEST CHANGES

Required before merge:

1. Controls logic in hyperscape.ts:buildSession — Justify or fix ["pause", "resume"] for status: "connecting". If this is intentional (UI shows both to let user resume a paused game-session), add a comment explaining why. If it's a bug, ["resume"] is the correct value.

2. as unknown as AppSessionJsonValue casts — Identify why recommendedGoals and recentThoughts don't conform to AppSessionJsonValue and fix the types rather than casting through unknown. If the contract genuinely requires this escape hatch, add a comment.

3. Import priority change in app-package-modules.ts — Add a brief comment explaining why localModule now takes priority over builtInModule. Nothing indicates this was the existing intent, and it's a silent behavior change for all apps.

4. resolveLaunchSession null-fallthrough — Add a comment at the call site noting the contract change: route modules returning null now fall through to buildAppSession instead of propagating null. Verify that no existing callers relied on the null-propagation behavior.

5. PR scope — Reclass the PR or split the Hyperscape feature into a follow-up. At minimum, the PR description should explicitly list the Hyperscape route module as a new feature, not imply it's part of the CI fix. A probationary contributor submitting a mixed scope PR makes review significantly harder.

@greptileai please perform a deep review — this PR was flagged as needing deep analysis by agent-review.

[greptile-apps]

Greptile Summary

This PR consolidates three CI-unblocking fix branches (#1770, #1772, #1774) into a single recovery branch for develop. The changes address five root causes of CI failures: TypeScript incompatibilities in upstream plugin submodules (via an idempotent git apply patch mechanism), a missing dist/ build for @elizaos/plugin-agent-skills, stale buf.gen.yaml on Blacksmith runners, a dangling plugins/plugin-groq gitlink, and a stale bun.lock entry. It also carries Hyperscape app integration work (built-in route module, session resolution, npm.package preservation in canonicalization) that was blocked by the CI failures.

Key changes:

• scripts/patch-workspace-plugins.mjs — new idempotent git apply mechanism; runs as the second postinstall step (after init-submodules, before patch-deps)
• scripts/ensure-bundled-workspaces.mjs — plugin-agent-skills and 11 other workspace plugins added to the build-if-missing list
• .github/workflows/test.yml — buf.gen.yaml existence guard added to all three "Generate protobuf types" steps; Windows-specific submodule init consolidated to node scripts/init-submodules.mjs
• packages/agent/src/services/app-manager.ts — genuine bug fix: resolveLaunchSession result was not previously awaited, causing a live Promise to be returned instead of the resolved AppSessionState | null
• packages/agent/src/services/built-in-app-routes/hyperscape.ts — new 286-line route module for live Hyperscape session resolution via its API
• packages/agent/src/services/app-package-modules.ts — lookup order changed from built-in-first to local-first for app route modules, allowing user-installed packages to override built-ins

Greptile verdict: APPROVE

Confidence Score: 4/5

Safe to merge after the loose-equality fix on the resolveLaunchSession null guard; all other findings are non-blocking P2s.

The core CI-unblocking mechanisms (buf.gen.yaml guard, patch-workspace-plugins, plugin-agent-skills build, gitlink removal, bun.lock fix) are all correct. The resolveLaunchSession await bug fix is a genuine regression fix. The one concrete code concern is the strict !== null guard at app-manager.ts:1353 which allows undefined to propagate; it's a one-character fix. The exec shell-quoting and lookup-order comment issues are P2 style concerns. No invariants are violated.

packages/agent/src/services/app-manager.ts — the session !== null guard; scripts/patch-workspace-plugins.mjs — exec shell quoting

Important Files Changed

Filename	Overview
scripts/patch-workspace-plugins.mjs	New idempotent git-patch mechanism; uses shell-string execSync with double-quoted paths instead of execFileSync + args array, which is fragile for paths containing spaces or special characters
scripts/ensure-bundled-workspaces.mjs	Adds pluginEntry helper and 12 new workspace plugin build entries; manifest-existence guard prevents failures for uninitialised submodules
packages/agent/src/services/app-manager.ts	Correct bug fix: resolveLaunchSession is now awaited and null-guarded; npm.package preservation for Hyperscape canonicalization is correct
packages/agent/src/services/built-in-app-routes/hyperscape.ts	New Hyperscape session resolver; fetch errors are caught and return null; URL validation guards against non-http(s) schemes; controls array for connecting state shows [pause, resume] matching 2004scape convention
packages/agent/src/services/app-package-modules.ts	Lookup order changed from built-in-first to local-first; Hyperscape route entries added correctly; priority change is intentional but undocumented
.github/workflows/test.yml	buf.gen.yaml existence guard added to all three Generate protobuf types steps; Windows-specific submodule init correctly consolidated to init-submodules.mjs
scripts/run-repo-setup.mjs	patch-workspace-plugins.mjs inserted after init-submodules and before patch-deps; ordering is correct since patches need submodules checked out but don't depend on bun exports being fixed
scripts/workspace-plugin-patches/plugin-anthropic-elizaos-core-api-compat.patch	Minimal cast to EventPayload; correct workaround for EventPayloadMap key narrowing; auto-removes when upstream merges
scripts/workspace-plugin-patches/plugin-personality-elizaos-core-api-compat.patch	Adds z.string() key schema to z.record() calls (Zod v4 compat) and stringifies evolutionData for MetadataValue; the settings spread cast is safe since typeof preserves the mapped type

Sequence Diagram

sequenceDiagram
    participant CI as GitHub Actions
    participant RS as run-repo-setup.mjs
    participant IS as init-submodules.mjs
    participant PW as patch-workspace-plugins.mjs
    participant PD as patch-deps.mjs
    participant SU as setup-upstreams.mjs
    participant EB as ensure-bundled-workspaces.mjs

    CI->>RS: bun run postinstall
    RS->>IS: node init-submodules.mjs
    IS-->>RS: submodules checked out
    RS->>PW: node patch-workspace-plugins.mjs
    PW->>PW: git apply --check --reverse (idempotency)
    PW->>PW: git apply patch (plugin-anthropic, google-genai, personality)
    PW-->>RS: patches applied / skipped / failed exit(1)
    RS->>PD: node patch-deps.mjs
    PD-->>RS: broken bun exports patched
    RS->>SU: node setup-upstreams.mjs
    SU-->>RS: local @elizaos/* linked (skipped if MILADY_SKIP_LOCAL_UPSTREAMS=1)
    RS->>EB: node ensure-bundled-workspaces.mjs
    EB->>EB: build plugin-agent-skills, plugin-anthropic, ... (if dist missing)
    EB-->>RS: workspace builds complete
    RS-->>CI: postinstall done

Loading

Comments Outside Diff (1)

1. packages/agent/src/services/app-manager.ts, line 1342-1358 (link)

   undefined return bypasses the fallback

   The guard is if (session !== null) return session. If a route module's resolveLaunchSession returns undefined (allowed at runtime even if typed as AppSessionState | null), the strict !== null check passes and undefined is returned to callers expecting AppSessionState | null. The fallback buildAppSession path is never reached.

   Use loose equality to catch both null and undefined:

   

_{Reviews (1): Last reviewed commit: "fix: sync develop CI app-core and plugin..." | Re-trigger Greptile}

[greptile-apps]

Shell string quoting instead of args array

execSync receives a shell string with "${patchPath}" interpolated. This works for common paths but breaks silently on paths that contain single quotes, shell metacharacters, or — on some Windows configurations — embedded double quotes. execFileSync with a proper args array avoids the shell entirely and handles all path characters correctly.

Suggested change

	encoding: "utf8",
	stdio: ["ignore", "pipe", "pipe"],
	});
	}
	function applyPatch(patchPath, pluginDir) {
	const patchName = patchPath.split(/[\\/]/).pop();
	function exec(cmd, args, cwd) {
	return execFileSync(cmd, args, {
	cwd,
	encoding: "utf8",
	stdio: ["ignore", "pipe", "pipe"],
	});
	}

Then update each call site:

exec("git", ["apply", "--check", "--reverse", patchPath], pluginDir);
exec("git", ["apply", "--check", patchPath], pluginDir);
exec("git", ["apply", patchPath], pluginDir);

Also add execFileSync to the import at the top of the file alongside execSync:

import { execFileSync } from "node:child_process";

[greptile-apps]

Lookup order reversal is undocumented

The built-in route module check moved from before the local module lookup to after it. This inverts the previous priority: a locally installed @hyperscape/plugin-hyperscape (or any other app with a built-in route) now shadows the built-in implementation. That's probably correct for legitimate use (user override wins), but the reasoning isn't captured in a comment, and this is a behaviour change that's easy to miss in a future refactor.

Consider adding a comment explaining why local takes priority:

Suggested change

	// Local (user-installed) module takes priority over built-ins, allowing
	// per-instance overrides. If no local module is found, fall back to the
	// built-in route implementations bundled with the agent.
	try {
	const localModule = await importLocalAppRouteModule(appIdentifier);
	if (localModule) {
	return localModule;
	}
	} catch (localErr) {
	logger.warn(
	`[app-route-modules] Failed to import local module for ${label}: ${localErr instanceof Error ? localErr.message : String(localErr)}`,
	);
	}
	const builtInModule = await importBuiltInAppRouteModule(
	appIdentifier,
	packageName,
	);
	if (builtInModule) {
	return builtInModule;
	}

[dutchiono]

Cleaned this branch back to the intended CI-unblock surface and force-pushed it. The current synchronize run is on head \

[github-actions]

Now I have everything I need for the review.

---

1. Classification (derived): workflow/CI + build system — consistent with author's claim

2. Rubric (derived): Critical axes are (a) Windows CI correctness after unifying submodule init, (b) security and correctness of the new patch-workspace-plugins.mjs infrastructure, (c) bun.lock consistency with the groq/openrouter gitlink cleanup, and (d) the buf.gen.yaml guard.

3. Scope verdict: in scope

4. Universal invariants: all intact — PR does not touch packages/agent/src/runtime/eliza.ts, scripts/run-node.mjs, apps/app/electrobun/src/native/agent.ts, patch-deps.mjs, or any access control files.

5. Judgment:

   • Needed? — OK — CI was materially broken; each fix addresses a specific, named failure mode.
   • Better than existing? — OK — unifying Windows/non-Windows submodule init into the already-maintained init-submodules.mjs (which has an explicit skip list for Windows-unsafe repos) is cleaner than duplicating ad-hoc --depth 1 lists across five workflows.
   • Duplication? — OK
   • Blast radius covered? — OK — plugin-openrouter moves from workspace to npm pin (consistent with its SKIP_SUBMODULES entry); plugin-groq was already absent from .gitmodules and not in any package.json deps (zombie gitlink); runtime dynamic-import behavior for both is unchanged for any user who has them installed. The five workflows that removed Windows-specific steps all now point to node scripts/init-submodules.mjs, which already covers the skip cases.
   • Logic sound? — OK — The patch applicator flow (--check --reverse → already-applied skip; --check → inapplicable skip; apply → fail hard) is correct and idempotent. buf.gen.yaml guard adds [ -f eliza/packages/schemas/buf.gen.yaml ] before the existing directory check — strictly safer, no regression. Three occurrences in test.yml all patched.
   • Complexity appropriate? — OK — 156-line script, defensive, clear skip semantics. No premature abstraction.
   • Tested meaningfully? — OK — patch-workspace-plugins.test.ts covers PLUGIN_PATCH_DIRS mapping, correct prefix resolution for all patched plugins, null return for unknowns, and the partial-prefix non-match. run-repo-setup.test.ts updated to enforce step ordering. Validation checklist in PR body shows test runs were done.
   • Matches conventions? — OK (with noted pre-existing Biome issue in apps/web/src/docs/content/**, not introduced here). Patch commit messages embed Co-Authored-By: lines — these are inside .patch file commit metadata, not in the repo's git history.
   • Plausible breakage mode identified? — init-submodules.mjs uses git submodule update --init --recursive (full depth) whereas the removed Windows steps used --depth 1. Windows CI may run slower for initial submodule clone. Not a correctness regression, but worth monitoring on Windows build times.

6. PR-type-specific checks (workflow/CI + build):

   • MILADY_SKIP_LOCAL_UPSTREAMS: "1" in ci-fork.yml: OK — correct env var per CLAUDE.md; necessary because fork PRs cannot access local upstream checkouts.
   • buf.gen.yaml guard applied to all three occurrences in test.yml: OK — three hunks, all patched.
   • node scripts/init-submodules.mjs works on Windows: OK — script uses cross-platform Node.js execSync; plugins/plugin-openrouter is explicitly in SKIP_SUBMODULES; groq gitlink removed; other submodules have no known Windows path issues.
   • plugin-agent-skills added to BUNDLED_WORKSPACE_BUILDS: OK — submodule exists in .gitmodules; dist/index.js artifact check prevents double-build.
   • PLUGIN_PATCH_DIRS includes plugin-agent-skills with no corresponding patch file: OK — the runner iterates readdirSync, so no-op when no file exists; the entry is a forward registration.
   • steward-fi and test/contracts/lib/openzeppelin-contracts SHA bumps: OK — PR explanation (old SHAs unreachable) is plausible; these are non-runtime test/contract submodules.
   • bun.lock @miladyai/plugin-2004scape added: OK — already in packages/agent/package.json:146 as workspace:*; lock just catching up.
   • Removed workspace entries for plugin-groq, plugin-openrouter: OK — groq was a zombie gitlink (absent from .gitmodules); openrouter moves to npm pin 2.0.0-alpha.13, consistent with SKIP_SUBMODULES.
   • Workflow pinned versions: unchanged (bun: 1.3.10, buf: 1.67.0). OK
   • Concurrency groups: unchanged. OK

7. Security: clear — patch-workspace-plugins.mjs uses execSync with paths constructed from resolve(__dirname, "workspace-plugin-patches") + readdirSync — no user-controlled input reaches the shell. Patch files are checked into the repo and only modify plugin submodule TypeScript sources. No credential exposure, no outbound HTTP, no auth/loopback changes.

8. Decision: APPROVE