Skip to content

Port Registrar: onboarding scaffolder to emit port-registrar-compliant templates#2368

Open
TalZaccai wants to merge 11 commits into
mainfrom
talzacc/onboarding-scaffolder-migration
Open

Port Registrar: onboarding scaffolder to emit port-registrar-compliant templates#2368
TalZaccai wants to merge 11 commits into
mainfrom
talzacc/onboarding-scaffolder-migration

Conversation

@TalZaccai
Copy link
Copy Markdown
Contributor

@TalZaccai TalZaccai commented May 19, 2026

Summary

Updates the onboarding scaffolder so newly-generated websocket-bridge and view-ui agents follow the modern port-allocation pattern introduced by the port-registrar work: OS-assigned port + context.registerPort(role, port), discoverable by external clients via discoverPort(name, role).

Templates emitted by pnpm onboarding scaffold were stuck on hardcoded ports (8081 / 9092) and a module-level singleton server, which collides with the dynamic-port contract every other agent has migrated to.

What changed

ts/packages/onboarding/src/scaffolder/scaffolderHandler.ts:

  • buildWebSocketBridgeTemplate (scaffoldPlugin): emits a class with a static start(port = 0) factory, close(), connected getter, and sendCommand helper. Wraps AgentWebSocketServer from websocket-utils so callers never bind a literal port.
  • buildWebSocketBridgeHandler (scaffoldAgent): full AppAgent with initializeAgentContext / updateAgentContext lifecycle, refcounted shared server, context.registerPort("bridge", port), and a backstop close().
  • buildViewUiHandler: full AppAgent with view server, context.registerPort("view", port), setLocalHostPort for shell integration, and an ActivityContext-driven openLocalView in executeAction.
  • Both templates honor <NAME>_BRIDGE_PORT / <NAME>_VIEW_PORT env-var overrides for debugging.
  • PLUGIN_TEMPLATES.nextSteps updated to show the new await <PascalName>Bridge.start() usage.

ts/docs/architecture/agent-patterns.md:

  • Sections 5 and 8 document the new port contract (registerPort / discoverPort, role name "default", env-var overrides).

Validation

  • pnpm --filter onboarding build
  • prettier --check
  • repo-policy-check
  • Manual scaffold smoke test: scaffolded a fresh websocket-bridge agent and a view-ui agent in a temp dir; both build clean, register their ports with the dispatcher on init, and are discoverable via discoverPort. Generated code matches the patterns established in already-migrated agents (e.g. code, localView).

Related

Updates the scaffolder so newly-generated websocket-bridge and view-ui agents follow the modern port-allocation pattern: OS-assigned port + context.registerPort(role, port), discoverable by external clients via discoverPort(name, role).

- buildWebSocketBridgeTemplate (scaffoldPlugin): static start(port=0) factory, close(), connected getter, sendCommand helper.
- buildWebSocketBridgeHandler (scaffoldAgent): full AppAgent lifecycle with refcounted shared server, registerPort, and backstop close.
- buildViewUiHandler: full AppAgent with view server, registerPort, setLocalHostPort for shell integration, and ActivityContext-driven openLocalView in executeAction.
- Both templates honor <NAME>_BRIDGE_PORT / <NAME>_VIEW_PORT env-var overrides for debugging.

- Updated PLUGIN_TEMPLATES nextSteps for websocket-bridge to reflect the new await <PascalName>Bridge.start() usage.
- agent-patterns.md sections 5 and 8 document the new port contract.

The office-addin template is left unchanged in this PR.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Updates the onboarding scaffolder templates (websocket-bridge + view-ui) to follow the port-registrar contract: bind on OS-assigned ports by default and publish them via context.registerPort(...), with docs updated to describe the new contract.

Changes:

  • WebSocket bridge template now uses an async start(port=0) factory, tracks connections, and exposes .port for registrar publication.
  • WebSocket bridge agent template now manages a refcounted shared server, registers the bound port per session, and supports env-var port overrides.
  • View UI agent template now binds an HTTP server on port=0, registers/discloses ports, and surfaces the view via ActivityContext.openLocalView.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.

File Description
ts/packages/agents/onboarding/src/scaffolder/scaffolderHandler.ts Updates scaffolded templates and next-steps text to use dynamic port binding + port registration patterns.
ts/docs/architecture/agent-patterns.md Documents the new register/discover port contract and env-var overrides for bridge + view patterns.
Comments suppressed due to low confidence (1)

ts/packages/agents/onboarding/src/scaffolder/scaffolderHandler.ts:1

  • isFirstForSession is computed before the await ensureSharedBridge(). If two updateAgentContext(true, ...) calls interleave for the same session (e.g. two schemas enabled concurrently), the second call can observe enabledSchemas.size > 0 and skip registration/refcounting, and then the first call can fail and roll back—leaving the session enabled with no portRegistration and no refcount increment. To make this robust, determine “first for session” at the time of registration (after the await) or gate registration on ctx.portRegistration (e.g., register iff ctx.portRegistration is undefined and ctx.enabledSchemas.size > 0), ideally under a per-session async mutex to prevent interleaving.
// Copyright (c) Microsoft Corporation.

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

Comment thread ts/packages/agents/onboarding/src/scaffolder/scaffolderHandler.ts Outdated
Comment thread ts/packages/agents/onboarding/src/scaffolder/scaffolderHandler.ts Outdated
Comment thread ts/packages/agents/onboarding/src/scaffolder/scaffolderHandler.ts Outdated
Comment thread ts/packages/agents/onboarding/src/scaffolder/scaffolderHandler.ts Outdated
Comment thread ts/packages/agents/onboarding/src/scaffolder/scaffolderHandler.ts Outdated
Comment thread ts/docs/architecture/agent-patterns.md
- websocket-bridge nextSteps: use <PascalName> placeholder instead of literal ${PascalName} interpolation
- Both bridge templates: pending map now stores {resolve, reject}; close() rejects all pending so callers don't hang
- Both bridge templates: post-listen errors are now console.error'd (was a TODO)
- websocket-bridge template: add closeAgentContext backstop that releases the per-session port registration and decrements the shared refcount

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@TalZaccai TalZaccai temporarily deployed to development-fork May 19, 2026 22:40 — with GitHub Actions Inactive
@TalZaccai TalZaccai temporarily deployed to development-fork May 19, 2026 22:40 — with GitHub Actions Inactive
@TalZaccai TalZaccai requested a review from robgruen May 21, 2026 19:23
@TalZaccai TalZaccai marked this pull request as ready for review May 21, 2026 19:23
@TalZaccai TalZaccai temporarily deployed to development-fork May 21, 2026 19:45 — with GitHub Actions Inactive
@TalZaccai TalZaccai temporarily deployed to development-fork May 22, 2026 01:48 — with GitHub Actions Inactive
Comment thread ts/packages/agents/onboarding/src/scaffolder/scaffolderHandler.ts
Per PR #2368 review feedback, 14 build* functions that emitted multi-
hundred-line TypeScript blobs as template literals have been split into
plain .template files loaded at runtime by a shared templateLoader.

Reviewers can now read each emitted file as syntax-highlighted code in
its own file instead of scrolling through escaped backticks inside
scaffolderHandler.ts. The source file drops from ~2100 lines to ~970.

Pattern matches the existing cliHandler.template precedent in the same
directory: {{TOKEN}} placeholders (so they don't collide with the
${...} template literals that survive into the emitted code), loaded
from src/ at runtime so no postbuild copy step is needed. The loader
throws on unsubstituted placeholders to catch typos at scaffold time.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The previous sequential split/join loop would re-process a substituted
value that happened to contain `{{KEY}}` text. Not exploitable today --
all callers pass values derived from toPascalCase(name) etc. -- but a
future caller passing a var derived from user input could see surprising
behavior.

Switch to a single-pass regex replace that treats each substituted value
as opaque. Bonus: an evil `{{KEY}}` value now correctly surfaces via
the leftover-placeholder check rather than silently re-expanding.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@TalZaccai TalZaccai temporarily deployed to development-fork May 28, 2026 20:37 — with GitHub Actions Inactive
@TalZaccai TalZaccai requested a review from robgruen May 28, 2026 21:57
Comment thread ts/packages/agents/onboarding/src/scaffolder/templates/officeAddinTs.template Outdated
…PORT placeholder

Addresses the three remaining open comments on PR #2368 from @robgruen:

1. commandHandlerTemplate / externalApiHandler / other handler templates:
   "Should we make this file a .ts so it compiles and we can verify it
   has no compilation issues?"

   Renamed 11 of the 14 scaffolder templates from `.template` to `.ts`
   so `tsc` validates each one as standalone TypeScript at build time.

   Changes:
   - Renamed placeholder convention from `{{TOKEN}}` to sentinel
     identifiers `__token__` / `__Token__` / `__TOKEN__` so the
     templates remain valid TypeScript when read by the compiler
     (`class __AgentName__Bridge {}`, `process.env["__PORT_ENV__"]`,
     `import { __AgentName__Actions } from "./__agentName__Schema.js"`).
   - The substitution regex requires *paired* double-underscores, so
     Node's `__filename` / `__dirname` are left untouched.
   - Added `templates/__agentName__Schema.ts` — a stub schema module
     so the import paths in the templates resolve.
   - Added `templates/tsconfig.json` — composite sub-project that
     extends `tsconfig.base.json` and disables `noUnusedLocals` /
     `noUnusedParameters` (templates intentionally pre-import helpers
     for the user's TODOs). Wired in via project reference from
     `src/tsconfig.json`.
   - `templateLoader` regex updated to match the new `__TOKEN__`
     convention; reserved-name guard added so the stub schema /
     placeholders d.ts can never be `loadTemplate`'d at scaffold time.
   - Added `ws`, `@types/ws`, `@types/node` to devDependencies so the
     websocket-bridge templates type-check standalone.
   - `viewUiHandler.ts` switched from `agentContext.x = undefined` to
     `delete agentContext.x` to satisfy `exactOptionalPropertyTypes`,
     matching the convention already in `websocketBridgeHandler`.

   The three browser/markup templates (officeAddinHtml,
   officeManifestXml, officeAddinTs) stay as `.template` — they're
   not Node TypeScript and would need a separate DOM/Office.js lib
   configuration. They use the same `__TOKEN__` placeholder syntax
   for consistency.

2. officeAddinTs.template:8 (`const BRIDGE_PORT = 5678;`):
   "Should this be a replaceable template value?"

   Yes — the addin's bridge port must match whatever port the bridge
   agent is bound to. Made it a `__BRIDGE_PORT__` placeholder with a
   `5678` default (the same pinned-sideload default the bridge
   handler uses when `<NAME>_BRIDGE_PORT` is unset). The scaffolder
   substitutes the value at scaffold time via `buildOfficeAddinTs`.

Verification:
- `pnpm --filter onboarding-agent build` clean.
- Migration smoke test (verifyTemplateMigration.mjs) renders each
  template with a canned (NAME, PASCAL_NAME, PORT_ENV) tuple and
  diffs against the pre-migration output. 13 of 14 templates are
  byte-identical; the one diff is `viewUiHandler.ts` switching to
  `delete` (intentional, as above).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
TalZaccai and others added 2 commits June 2, 2026 20:22
…affolder-migration

# Conflicts:
#	ts/pnpm-lock.yaml
…lected XSS

Addresses CodeQL alert #309 on the view-UI scaffolded template. The placeholder handler echoed 
eq.url into HTML unescaped, which would generate XSS-vulnerable code for any user who scaffolded a view-UI agent from this template.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 21 out of 22 changed files in this pull request and generated 8 comments.

Files not reviewed (1)
  • ts/pnpm-lock.yaml: Language not supported

Comment thread ts/packages/agents/onboarding/src/scaffolder/templateLoader.ts Outdated
Comment thread ts/packages/agents/onboarding/src/scaffolder/templateLoader.ts Outdated
Comment thread ts/pnpm-lock.yaml
The bridge template's updateAgentContext mutated enabledSchemas before awaiting ensureSharedBridge, so two concurrent enables for the same session could interleave: the second saw size>0 and skipped registration, then the first failed and rolled back, leaving the session 'enabled' with no portRegistration. A later disable would then decrement sharedRefCount this session never incremented, tearing down the bridge for any other session that still depended on it.

Fix: wrap updateAgentContext + closeAgentContext bodies in a per-session async lock (withSessionLock) and use ctx.portRegistration !== undefined as the sole source of truth for 'this session has incremented sharedRefCount'.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- websocketBridgeHandler/Template: reject pending requests when the last client disconnects (so callers don't hang) and surface ws.send errors via the send callback (so a socket-close race between readyState check and send no longer leaks pending entries).

- viewUiHandler: log post-listen HTTP server errors via console.error instead of swallowing them as a TODO.

- templateLoader: drop stale references to a __placeholders__.d.ts stub that no longer exists (the schema stub __agentName__Schema.ts is the only reserved template name).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@TalZaccai TalZaccai temporarily deployed to development-fork June 3, 2026 06:46 — with GitHub Actions Inactive
@TalZaccai TalZaccai requested a deployment to development-fork June 3, 2026 06:46 — with GitHub Actions In progress
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.

4 participants