Port Registrar: onboarding scaffolder to emit port-registrar-compliant templates#2368
Open
TalZaccai wants to merge 7 commits into
Open
Port Registrar: onboarding scaffolder to emit port-registrar-compliant templates#2368TalZaccai wants to merge 7 commits into
TalZaccai wants to merge 7 commits into
Conversation
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>
Contributor
There was a problem hiding this comment.
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.portfor 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 viaActivityContext.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
isFirstForSessionis computed before theawait ensureSharedBridge(). If twoupdateAgentContext(true, ...)calls interleave for the same session (e.g. two schemas enabled concurrently), the second call can observeenabledSchemas.size > 0and skip registration/refcounting, and then the first call can fail and roll back—leaving the session enabled with noportRegistrationand no refcount increment. To make this robust, determine “first for session” at the time of registration (after the await) or gate registration onctx.portRegistration(e.g., register iffctx.portRegistrationis undefined andctx.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.
- 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>
robgruen
requested changes
May 27, 2026
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>
robgruen
approved these changes
Jun 2, 2026
…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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Updates the onboarding scaffolder so newly-generated
websocket-bridgeandview-uiagents follow the modern port-allocation pattern introduced by the port-registrar work: OS-assigned port +context.registerPort(role, port), discoverable by external clients viadiscoverPort(name, role).Templates emitted by
pnpm onboarding scaffoldwere 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 staticstart(port = 0)factory,close(),connectedgetter, andsendCommandhelper. WrapsAgentWebSocketServerfromwebsocket-utilsso callers never bind a literal port.buildWebSocketBridgeHandler(scaffoldAgent): fullAppAgentwithinitializeAgentContext/updateAgentContextlifecycle, refcounted shared server,context.registerPort("bridge", port), and a backstopclose().buildViewUiHandler: fullAppAgentwith view server,context.registerPort("view", port),setLocalHostPortfor shell integration, and anActivityContext-drivenopenLocalViewinexecuteAction.<NAME>_BRIDGE_PORT/<NAME>_VIEW_PORTenv-var overrides for debugging.PLUGIN_TEMPLATES.nextStepsupdated to show the newawait <PascalName>Bridge.start()usage.ts/docs/architecture/agent-patterns.md:"default", env-var overrides).Validation
pnpm --filter onboarding build✓prettier --check✓repo-policy-check✓websocket-bridgeagent and aview-uiagent in a temp dir; both build clean, register their ports with the dispatcher on init, and are discoverable viadiscoverPort. Generated code matches the patterns established in already-migrated agents (e.g.code,localView).Related