[Phase A · Base] ECS + Vue hoisted client state & hook API — research bundle + canonical surface#12101
[Phase A · Base] ECS + Vue hoisted client state & hook API — research bundle + canonical surface#12101christian-byrne wants to merge 5 commits intomainfrom
Conversation
Compatibility-surface map for the upcoming v2 extension API redesign. 52 patterns across 15 surface families derived from: - AGENTS.md §5 (40+ repo callouts) - ADR 0008 entity taxonomy - widget-api-thoughts.md output system axis - Real-world MCP code-search across 87 ecosystem repos See docs/architecture/extension-api-v2/README.md.
Adds 21 evidence rows + 4 new patterns from second MCP code-search burst: S11.G3 graph.beforeChange/afterChange — explicit batching seam S7.G1 window.LiteGraph / window.comfyAPI globals (CRITICAL) S11.G4 graph.setDirtyCanvas — manual redraw flush idiom S10.D3 node.setSize(node.computeSize()) — manual relayout idiom Pattern S11.G4 + S10.D3 together justify v2's reactive-layout value proposition; S11.G3 motivates a first-class world.batch() API; S7.G1 sets the deprecation sequence for window globals. star-cache.yaml grew 87 → 105 starred repos; #1 blast-radius is still S6.A1 graphToPrompt (★17,122) — unchanged. Reproducible via docs/architecture/extension-api-v2/scripts/add-evidence-pass2.py
Adds the v2 extension API public surface:
src/types/extensionV2.ts (169 lines) — branded entity IDs,
geometry primitives, slot/widget options, NodeHandle/WidgetHandle
contracts. Pure types. The stable public surface extensions depend on.
src/services/extensionV2Service.ts (413 lines) — surface-only impl.
Thin pass-throughs to existing LGraphNode/widget/canvas. No new
internal architecture; no patching guards yet (Phase A scope per D9).
Reactive mounting via Vue EffectScope; scope registry keyed by
extension:entityId.
src/extensions/core/{dynamicPrompts,imageCrop,previewAny}.v2.ts
Three smallest core extensions ported as proof-of-concept. Used
as the I-COORD.1 reference for Simon + Austin to push back on
the API shape with concrete code.
Phase A goals (per todo.md "Branch + phasing topology"):
- Stable v2 surface coworkers can branch off
- Internal methods are pass-throughs (no behavior change)
- I-PG.A: no interception/blocking; v1 path coexists unchanged
Stacks under this branch (will be opened as draft PRs):
- ext-v2/i-tf-test-framework
- ext-v2/i-sr-scope-registry
- ext-v2/i-ws-lazy-serialize
Merges three pass-3 staging sources via scripts/merge-staging-pass3.py: R8 (clone-grep top-20 ecosystem packs) — 887 raw rows, 380 deduped R9.security — 3 NEW S16.* dynamic-code patterns R9.cookiecutter / R9.guides — sanctioned-public surface evidence Rollup re-ranked. Notable shifts: rank pattern ★sum occ 1 S6.A1 graphToPrompt monkey-patching⚠️ CRITICAL 23,604 15 2 S2.N15 prototype.serialize replacement 5,996 12 3 S11.G2 graph add/remove/findNodesByType 21,086 12 4 S11.G3 beforeChange/afterChange batching 18,973 13 5 S7.G1 window.LiteGraph globals as ABI 18,853 14 6 S6.A4 app.queuePrompt patching 3,464 11 7 S10.D1 dynamic addInput/Output 5,287 11 8 S10.D2 node.connect/disconnect programmatic 18,643 14 9 S4.W4 widget.options.values mutation 7,985 11 10 S2.N16 node.widgets array access 7,086 13 S11.G3 (beforeChange batching) and S7.G1 (window globals) entered top-5 — both drive the v2 'three orthogonal capabilities' thesis: batched mutations, typed import deprecation path. 3 NEW S16.* patterns from security scan: S16.D1 SVG with onload/onerror (XSS-equivalent dynamic JS) S16.D3 obfuscated string-replace pipeline S16.D7 Python web/ template uses exec/eval to emit JS star-cache: 105 → 119 starred repos (added Acly, ssitu, cubiq, Fannovel16, WASasquatch, Crystools, cg-use-everywhere, mikey_nodes, etc. — actual top-20 ecosystem heavyweights now represented).
…-TF) Adds the isolated vitest config, CI workflow, and all 41×3 compat-floor stub triples so the blast_radius≥2.0 gate passes from day one. - vitest.extension-api.config.mts — targets src/extension-api-v2/__tests__/ - .github/workflows/ci-tests-extension-api.yaml — two jobs: vitest run + python3 scripts/check-compat-floor.py (exits 1 on missing stubs) - package.json — test:extension-api / :watch / :coverage scripts - src/extension-api/ — public declaration files (NodeHandle, WidgetHandle, defineNodeExtension, typed events, lifecycle hooks) - src/extension-api-v2/__tests__/v1|v2|migration — 123 stub files covering BC.01–BC.41 (all 34 compat-floor categories × 3 stub types) - packages/extension-api/ — typedoc wrapper package compat-floor gate: OK — 102 stub files present (34 categories × 3 types) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis change establishes a complete V2 Extension API framework for ComfyUI: a stable, branded TypeScript API contract, canonical YAML compatibility DB with evidence/rollup automation, triple-format (v1/v2/migration) BC test scaffolds, real node extension examples, documentation build/publish pipeline, CI/test coverage, and service/mount logic. ChangesExtension API V2: Contracts, BC, Compatibility, Tooling, Docs
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
🌐 Website E2ECaution Some tests failed.
|
🎭 Playwright: 🕵🏻 0 passed, 0 failed📊 Browser Reports
|
🎨 Storybook: ❌ FailedDetails⏰ Completed at: 05/09/2026, 04:42:18 AM UTC Links
|
📦 Bundle Size
⚡ Performance
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (36)
src/extension-api-v2/__tests__/v1/BC.08.v1.test.ts-2-8 (1)
2-8:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
compat-floor: yesconflicts with TODO-only coverage.Line 2 marks this suite as compat-floor enforced, but Lines 6–8 are non-executable TODOs, so no actual floor is enforced yet. Either implement at least one runnable assertion now or temporarily mark this suite
compat-floor: nountil executable cases land.Quick metadata-safe interim fix
-// blast_radius: 11.81 compat-floor: yes +// blast_radius: 11.81 compat-floor: noAs per coding guidelines "Write tests for all changes, especially bug fixes to catch future regressions."
🤖 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 `@src/extension-api-v2/__tests__/v1/BC.08.v1.test.ts` around lines 2 - 8, The suite is marked compat-floor: yes but contains only it.todo entries, so no executable assertions satisfy the floor; either change the suite metadata from "compat-floor: yes" to "compat-floor: no" or add at least one runnable test (for example implement an it() that uses app.graph.connectNodes(srcId, srcSlot, dstId, dstSlot) and asserts graph.links or onConnectionsChange) inside the describe('BC.08 v1 — Programmatic linking') block to provide executable coverage and satisfy the compat-floor requirement.src/extension-api-v2/__tests__/v1/BC.09.v1.test.ts-2-9 (1)
2-9:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDeclared compat-floor is not currently enforced by this suite.
Line 2 marks BC.09 as compat-floor, but Lines 6–9 are all TODO placeholders. Please add at least one executable parity test now, or set compat-floor to
nountil runnable cases are added.Quick metadata-safe interim fix
-// blast_radius: 38.63 compat-floor: yes +// blast_radius: 38.63 compat-floor: noAs per coding guidelines "Write tests for all changes, especially bug fixes to catch future regressions."
🤖 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 `@src/extension-api-v2/__tests__/v1/BC.09.v1.test.ts` around lines 2 - 9, The test suite declares compat-floor but contains only TODOs; either add at least one runnable parity test inside the describe block "BC.09 v1 — Dynamic slot and output mutation" (replace one of the it.todo entries such as the node.addInput/node.addOutput/node.removeInput/dynamically added outputs case with an actual it(...) test that sets up a node, performs the dynamic slot/output mutation via the public API, asserts the canvas/connectivity or serialized node.outputs, and cleans up), or change the compat-floor marker from yes to no so the suite is not treated as a compat-floor; update the describe block by replacing one it.todo with a concrete assertion-driven test (use existing test helpers in the repo) or flip the compat-floor flag to "no".src/extension-api-v2/__tests__/v2/BC.23.v2.test.ts-2-9 (1)
2-9:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCompat-floor is declared, but no executable tests are present.
With
compat-floor: yeson Line 2, this suite should enforce behavior now; TODO-only cases on Lines 6–9 do not. Please either add at least one concrete assertion or downgrade compat-floor until implementation is ready.Quick metadata-safe interim fix
-// blast_radius: 14.42 compat-floor: yes +// blast_radius: 14.42 compat-floor: noAs per coding guidelines "Write tests for all changes, especially bug fixes to catch future regressions."
🤖 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 `@src/extension-api-v2/__tests__/v2/BC.23.v2.test.ts` around lines 2 - 9, The test suite currently declares "compat-floor: yes" but contains only it.todo entries, so add at least one executable test assertion or remove/downgrade the compat-floor marker; locate the describe('BC.23 v2 — Node property bag mutations') block and replace or supplement one of the it.todo('...') entries with a concrete it(...) that asserts expected behavior (e.g., call the NodeHandle API such as setTitle/getTitle or event hooks like on('configured') and assert results) or if you can't implement a test yet, change/remove the compat-floor comment so the suite doesn't require executable tests.src/extension-api-v2/__tests__/v2/BC.15.v2.test.ts-5-9 (1)
5-9:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCompat-floor scenario has no executable assertions yet
Lines 6-8 are all
it.todo, so this “compat-floor: yes” case currently can’t catch regressions in workflow-load hook behavior. Please convert at least one scenario to a runnable assertion in this PR (even a minimal smoke path), then keep the rest as TODOs.As per coding guidelines "Write tests for all changes, especially bug fixes to catch future regressions".
🤖 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 `@src/extension-api-v2/__tests__/v2/BC.15.v2.test.ts` around lines 5 - 9, The test file declares a describe block 'BC.15 v2 — Workflow loading into the editor' with three it.todo entries; convert at least one of them (e.g., the first: "defineExtension setup can register a loadWorkflow hook that fires when a workflow JSON is loaded") into a runnable assertion by replacing the it.todo with an it test that mounts or initializes the v2 extension environment, registers a minimal defineExtension providing a loadWorkflow hook (reference the hook name loadWorkflow and the test case BC.15 v2), triggers the code path that loads a simple workflow JSON, and asserts the hook was called (e.g., by using a spy/flag or mock) so the test exercises the hook firing; keep the other two it.todo entries unchanged.src/extension-api-v2/__tests__/bc-14.migration.test.ts-10-39 (1)
10-39:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftCompat-floor migration suite is non-executable (
it.todoonly).This currently provides zero behavioral verification for a high-blast-radius contract. Please convert core parity scenarios to runnable assertions (or keep it out of compat-floor accounting until executable).
As per coding guidelines: "Write tests for all changes, especially bug fixes to catch future regressions".
🤖 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 `@src/extension-api-v2/__tests__/bc-14.migration.test.ts` around lines 10 - 39, The suite "BC.14 migration — graphToPrompt interception" currently contains only it.todo placeholders; replace the core parity todos with real tests that assert functional equivalence or remove them from compat-floor: for each todo under the "payload equivalence", "execution ordering", and "coexistence during migration window" describes, implement an it(...) that sets up a minimal reproducible scenario (e.g., create a sample {output, workflow} payload, register a v1 monkey-patch and the v2 beforeGraphToPrompt handler, or simulate cancellation), execute both codepaths (invoke the v1 wrapper and the v2 handler pipeline using the same inputs), and use expect(...) assertions to compare final payloads, injected metadata, virtual-node removal behavior, ordering effects, and cancellation side-effects; alternatively, if coverage cannot be implemented now, replace the it.todo with it.skip or remove the test from compat-floor accounting. Ensure tests reference the suite name and the specific behaviors (payload equivalence, execution ordering, coexistence) so reviewers can locate them.src/extension-api-v2/__tests__/migration/BC.37.migration.test.ts-1-9 (1)
1-9:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove duplicate BC contract test files across all 41 migration contracts.
The test suite has systematic duplication: all 41 BC contracts exist in both naming conventions (
bc-NN.type.test.tsat root andBC.NN.type.test.tsin subdirectories). The Vitest glob patternsrc/extension-api-v2/__tests__/**/*.{test,spec}.{ts,mts}discovers both sets, risking double execution and ambiguous compat-floor accounting.Keep one canonical structure (recommend the subdirectory approach with uppercase naming for clarity), and remove all 123 duplicate files from the root
__tests__/directory. This affects all three variants (migration, v1, v2) for each of the 41 contracts.🤖 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 `@src/extension-api-v2/__tests__/migration/BC.37.migration.test.ts` around lines 1 - 9, The test suite duplicates 41 BC contract tests causing double execution; keep the canonical uppercase subdirectory versions (e.g., the describe titled "BC.37 migration — VueNode bridge timing — deferred mount access") and delete the duplicate root copies (the bc-NN.type.test.ts variants) across all three variants (migration, v1, v2); then update the Vitest glob (the pattern src/extension-api-v2/__tests__/**/*.{test,spec}.{ts,mts}) to only match the retained subdirectory structure so only the BC.NN.*.test files run and remove the 123 root duplicate files.src/extension-api-v2/__tests__/v1/BC.37.v1.test.ts-6-8 (1)
6-8:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
it.todoplaceholders provide no compatibility protection yetThese cases currently don’t execute or assert behavior, so this contract file won’t catch regressions in BC.37 until at least core scenarios are converted to runnable tests.
As per coding guidelines, “Write tests for all changes, especially bug fixes to catch future regressions”.
🤖 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 `@src/extension-api-v2/__tests__/v1/BC.37.v1.test.ts` around lines 6 - 8, Replace the three it.todo placeholders in the BC.37.v1.test.ts file with runnable tests: convert the first todo into an it test that mounts a VueNode-backed widget and asserts widget.element is null or undefined when accessed synchronously inside nodeCreated; convert the second into an it that awaits nextTick (imported from 'vue' or test utils) after nodeCreated and asserts widget.element is non-null; and convert the third into an it that defers DOM access with Promise.resolve() inside nodeCreated and asserts the resolved tick sees a fully initialized VueNode element—locate the tests by the todo descriptions and ensure you use the same widget creation/mount helpers used elsewhere in the suite and include proper assertions instead of todo placeholders.src/extension-api-v2/__tests__/bc-31.v1.test.ts-40-46 (1)
40-46:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd trusted-source and failure-path expectations for external asset loading
These TODOs currently validate only happy-path injection. The migration contract should also require trusted-source validation and explicit error propagation for rejected/failed loads.
Suggested TODO additions
describe('S16.DOM4 — external script/asset loading via DOM', () => { it.todo( 'extension can dynamically create and append a <script> element to load external code' ) it.todo( 'extension can create a <link rel="stylesheet"> element for external CSS' ) + it.todo( + 'external asset URLs are validated against trusted sources before insertion' + ) + it.todo( + 'failed or blocked asset loads propagate actionable errors to the caller' + ) })As per coding guidelines, “Validate trusted sources before processing” and “Implement proper error propagation”.
🤖 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 `@src/extension-api-v2/__tests__/bc-31.v1.test.ts` around lines 40 - 46, The two it.todo tests under the 'S16.DOM4 — external script/asset loading via DOM' suite currently only cover happy-path injection; replace each TODO with concrete tests that (1) assert trusted-source validation is enforced by simulating an extension attempt to create/append a <script> or <link> with a disallowed URL and expecting the operation to be rejected/throw (covering the "trusted-source" requirement) and (2) assert explicit error propagation by simulating a load failure (triggering the element's error event) and verifying that the code path handling dynamic insertion surfaces/propagates that error to the caller. Target the existing test identifiers (the two it.todo entries) and use the DOM element creation/append sequence plus onload/onerror handling to validate both trust-check and failure-path behavior. Ensure tests assert both allowed sources succeed and disallowed sources or network/load errors produce explicit rejections/errors.src/extension-api/identifiers.ts-13-25 (1)
13-25:⚠️ Potential issue | 🟠 Major | ⚡ Quick winConvert all
@/aliases to relative paths in extension-api sources to prevent alias leakage in published declarations.The
@comfyorg/extension-apipackage is built withtsc --emitDeclarationOnly, which will emit.d.tsfiles to npm. Sincepackages/extension-apihas notsconfig.json, it inherits the root configuration with"@/*": ["./src/*"]. Downstream npm consumers won't have this alias configured, breaking their TypeScript resolution.Five
@/imports across four files need fixing:
src/extension-api/identifiers.ts(lines 16, 25):from '@/types/nodeIdentification'src/extension-api/shell.ts(line 21):from '@/types/extensionTypes'src/extension-api/lifecycle.ts(line 306):from '@/services/extension-api-service'src/extension-api/index.ts(line 64):from '@/services/extension-api-service'Convert each to relative paths (e.g.,
from '../types/nodeIdentification').Example for identifiers.ts
export type { NodeLocatorId, NodeExecutionId -} from '@/types/nodeIdentification' +} from '../types/nodeIdentification' export { isNodeLocatorId, isNodeExecutionId, parseNodeLocatorId, createNodeLocatorId, parseNodeExecutionId, createNodeExecutionId -} from '@/types/nodeIdentification' +} from '../types/nodeIdentification'🤖 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 `@src/extension-api/identifiers.ts` around lines 13 - 25, Replace the aliased import path '@/types/nodeIdentification' with a relative path so generated .d.ts files don't reference the project alias: update both exports in this file that re-export NodeLocatorId, NodeExecutionId and the functions isNodeLocatorId, isNodeExecutionId, parseNodeLocatorId, createNodeLocatorId, parseNodeExecutionId, createNodeExecutionId to import from the relative module (e.g., '../types/nodeIdentification') instead of '@/types/nodeIdentification'.src/extension-api-v2/__tests__/bc-31.v2.test.ts-9-55 (1)
9-55:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReplace TODO-only contract cases with at least one executable check
This suite is entirely
it.todo(...), so CI can pass without validating BC.31 behavior. Please add at least one runnable assertion for the highest-risk path (e.g., sanitization or style cleanup) in this file.As per coding guidelines: "Write tests for all changes, especially bug fixes to catch future regressions."
🤖 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 `@src/extension-api-v2/__tests__/bc-31.v2.test.ts` around lines 9 - 55, Replace at least one it.todo with a real test that exercises the highest-risk path: call extensionManager.injectStyles(css) (or the extension.addPanel/addToolbarItem variants if easier) with a unique CSS string, assert a <style> tag with that content is appended to document.head, then call the returned cleanup handle or unregister the extension scope and assert the style tag is removed; reference injectStyles(css), extensionManager.injectStyles, and the cleanup/unregister handle to locate where to add the assertions. Ensure the test runs in Jest (not todo) and cleans up DOM state so it won’t leak into other tests.src/extension-api-v2/__tests__/bc-06.v1.test.ts-14-54 (1)
14-54:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd one executable BC.06 guard test for canvas override behavior
The suite is currently all TODOs, so prototype-override regressions won’t be detected. Please add at least one runnable assertion for a high-risk canvas override path.
As per coding guidelines: "Write tests for all changes, especially bug fixes to catch future regressions."
🤖 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 `@src/extension-api-v2/__tests__/bc-06.v1.test.ts` around lines 14 - 54, Add a runnable test that asserts prototype override behavior instead of leaving todos: implement a test in this file that temporarily replaces LGraphCanvas.prototype.drawNodeShape (or LGraphCanvas.prototype.processContextMenu) with a spy/flagging function, creates an LGraphCanvas instance, triggers the relevant action (render or right-click), and asserts the spy was invoked (and that calling the original is optional depending on which behavior you check); make sure to restore the original prototype method at test teardown so other tests are unaffected and reference the exact symbols LGraphCanvas.prototype.drawNodeShape or LGraphCanvas.prototype.processContextMenu to locate the code to modify.src/extension-api-v2/__tests__/bc-21.migration.test.ts-11-33 (1)
11-33:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBC.21 migration needs at least one executable contract assertion
All checks are TODO placeholders, so none of the migration guarantees are currently enforced. Please add one runnable assertion for widget registration/cleanup parity.
As per coding guidelines: "Write tests for all changes, especially bug fixes to catch future regressions."
🤖 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 `@src/extension-api-v2/__tests__/bc-21.migration.test.ts` around lines 11 - 33, The tests currently contain only it.todo placeholders; add at least one executable test that asserts migration BC.21 behavior—e.g., replace one of the todos under "scope cleanup on dispose" with a real test that registers a v2 widget via defineWidgetExtension(handle,...), verifies the widget appears in getCustomWidgets (or node.widgets after nodeCreated), then calls the extension's dispose/unregister and asserts the widget type is removed and nodes fall back to default rendering; use the existing test helpers to create a temporary extension, call defineWidgetExtension and nodeCreated, inspect node.widgets (or getCustomWidgets), then call dispose and re-check that the custom widget type no longer exists. Ensure the test names match existing descriptions (e.g., "v2 cleanup on dispose does not affect widget types registered by other extensions") and exercise defineWidgetExtension, getCustomWidgets/nodeCreated, and dispose to produce a runnable assertion rather than a TODO.src/extension-api-v2/__tests__/bc-08.v2.test.ts-10-38 (1)
10-38:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd at least one executable BC.08 v2 assertion
All cases are
it.todo(...), which gives no runtime signal for linking compatibility. Add one concreteit(...)assertion forconnect/disconnectInputto prevent false-green CI on this contract.As per coding guidelines: "Write tests for all changes, especially bug fixes to catch future regressions."
🤖 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 `@src/extension-api-v2/__tests__/bc-08.v2.test.ts` around lines 10 - 38, Replace at least one it.todo with an executable test that actually exercises NodeHandle.connect(slotIndex, targetHandle, dstSlot) and NodeHandle.disconnectInput(slotIndex): create two nodes via the test harness, call sourceHandle.connect(outIndex, targetHandle, inIndex) and assert the returned LinkHandle is valid and matches the graph (e.g., stable id present and link exists), then call targetHandle.disconnectInput(inIndex) and assert the LinkHandle becomes invalid and the graph no longer contains the link; also assert on('connectionChange') handlers on both NodeHandles were invoked. Use the existing test utilities to create nodes and obtain NodeHandle/LinkHandle references; focus on exercising NodeHandle.connect, NodeHandle.disconnectInput, LinkHandle validity and the 'connectionChange' event.src/extension-api-v2/__tests__/bc-03.migration.test.ts-10-35 (1)
10-35:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMigration suite needs at least one runnable parity test
With only
it.todo(...), this file does not currently validate BC.03 migration behavior. Please add one executable parity assertion (e.g., workflow-loadonConfigurecall semantics) to anchor the suite.As per coding guidelines: "Write tests for all changes, especially bug fixes to catch future regressions."
src/extension-api-v2/__tests__/bc-19.v1.test.ts-11-30 (1)
11-30:⚠️ Potential issue | 🟠 Major | ⚡ Quick winQueue interception contract should include a runnable test
This v1 contract suite is TODO-only, so it won’t catch regressions in wrapper behavior. Add at least one executable
queuePromptinterception assertion in this file.As per coding guidelines: "Write tests for all changes, especially bug fixes to catch future regressions."
🤖 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 `@src/extension-api-v2/__tests__/bc-19.v1.test.ts` around lines 11 - 30, Replace at least one of the todo tests under the "S6.A4 — queuePrompt interception" suite with an executable test that registers an extension wrapper around app.queuePrompt, captures a reference to the original (orig) function, calls the wrapped app.queuePrompt with sample args (e.g., 0, 1), and asserts the wrapper invoked orig with the same (number, batchCount) args and returned orig's result; specifically implement a test that verifies the wrapper receives (number, batchCount), calls orig(), and the returned value from app.queuePrompt equals the orig() result so this file's queuePrompt interception behavior is actually validated.src/extension-api-v2/__tests__/v2/BC.02.v2.test.ts-6-8 (1)
6-8:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBC.02 TODO wording conflicts with the current
removedhandler signature.Line 7 implies a callback argument (
NodeHandlepayload), butsrc/extension-api/node.tscurrently typeson('removed', handler)asHandler<void>. Align the contract text and API before downstream PRs branch on this.🤖 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 `@src/extension-api-v2/__tests__/v2/BC.02.v2.test.ts` around lines 6 - 8, The TODOs assert that the removed event handler receives a NodeHandle payload but the API currently types on('removed', handler) as Handler<void>, so update the contract to be consistent: either change the API signature in NodeHandle/on('removed', handler) from Handler<void> to Handler<NodeHandle> (and update any references to onNodeRemoved) or reword the test TODOs to reflect no payload (e.g., removed handler receives no arguments); locate the handler typing in the NodeHandle/on('removed', handler) declaration and adjust the type and related docs/tests accordingly.src/extension-api-v2/__tests__/v2/BC.07.v2.test.ts-6-9 (1)
6-9:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBC.07 contract TODOs currently describe APIs that are not in
NodeHandle.The TODOs mention slot-index payloads and veto/intercept callbacks, but the exported
NodeHandlesurface only defines passiveconnected/disconnectedlisteners withSlotInfopayloads and no veto hook. Please align either the API or these contract statements.🤖 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 `@src/extension-api-v2/__tests__/v2/BC.07.v2.test.ts` around lines 6 - 9, The test TODOs describe APIs that don't exist on NodeHandle: they expect slot-index payloads and a connection-intercept/veto hook, while the current NodeHandle only exposes passive connected/disconnected listeners with SlotInfo payloads; either update the TODOs to match the existing API or add the missing surface to the implementation—specifically, either change the four TODO descriptions in BC.07.v2.test.ts to reference NodeHandle.on("connected", handler: (info: SlotInfo) => void) and NodeHandle.on("disconnected", handler: (info: SlotInfo) => void), or extend the NodeHandle interface/implementation to include an on("connected", handler: (slotIndex: number, nodeInfo: NodeInfo) => void), on("disconnected", handler: (slotIndex: number) => void) and a connection intercept hook (e.g., NodeHandle.setConnectionInterceptor or NodeManager.onIncomingConnection that returns boolean to veto) so the tests and API match.src/extension-api-v2/__tests__/bc-10.v2.test.ts-10-33 (1)
10-33:⚠️ Potential issue | 🟠 Major | ⚡ Quick winResolve BC.10 contract drift before locking compat-floor semantics.
Line 12 defines
on('change')as(newValue, oldValue), but Line 21 expectsevent.preventDefault().
Also, Lines 25-34 rely onNodeHandle.on('widgetChanged', ...), which is not present insrc/extension-api/node.ts. Please align these TODO contracts (or the API) to a single canonical shape.🤖 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 `@src/extension-api-v2/__tests__/bc-10.v2.test.ts` around lines 10 - 33, Tests and comment reveal API mismatch: WidgetHandle.on('change') is implemented as (newValue, oldValue) but tests expect an event with preventDefault(), and NodeHandle.on('widgetChanged') is referenced but missing; pick a single canonical shape and update code to match. Change WidgetHandle.on('change') handler signature to receive an event object { newValue, oldValue, preventDefault() } and update the emitter so per-widget listeners get that event; implement NodeHandle.on('widgetChanged') in the node-level API (class NodeHandle) to accept listeners and emit after all per-widget on('change') calls, passing payload { name, value, oldValue, widget } as the event argument; ensure preventDefault on the per-widget event can veto the write so the emitter honors event.preventDefault().src/extension-api-v2/__tests__/v2/BC.01.v2.test.ts-9-9 (1)
9-9:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the actual
NodeHandlemethod names in the BC.01 contract text.Line 9 references
getInputs()/getOutputs(), but the exported API insrc/extension-api/node.tsdefinesinputs()/outputs().✏️ Suggested TODO text update
- it.todo('NodeHandle.getInputs() and getOutputs() reflect the node slot configuration at creation') + it.todo('NodeHandle.inputs() and outputs() reflect the node slot configuration at creation')🤖 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 `@src/extension-api-v2/__tests__/v2/BC.01.v2.test.ts` at line 9, The BC.01 test TODO references non-existent methods getInputs() and getOutputs(); update the test text to use the actual exported NodeHandle API method names inputs() and outputs() so the contract matches src/extension-api/node.ts—change the todo string in BC.01.v2.test.ts to mention NodeHandle.inputs() and NodeHandle.outputs().docs/architecture/extension-api-v2/scripts/rollup-blast-radius.py-5-10 (1)
5-10:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThese weights drifted from the formula documented in
touch-points-plan.md.The plan defines
0.40 / 0.20 / 0.15 / 0.15 / 0.10, but this script bakes in1.0 / 0.7 / 0.5 / 0.5 / 0.4. That materially changes ranking and theblast_radius >= 2.0compat-floor threshold, so the script and docs need to be brought back into sync before this rollup is used as a gate.Also applies to: 26-32
🤖 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/architecture/extension-api-v2/scripts/rollup-blast-radius.py` around lines 5 - 10, The documented blast-radius weights in the header comment and in the implementation (variables w_stars, w_occ, w_sig, w_silent, w_lifecycle) do not match touch-points-plan.md; update the default weight values from 1.0/0.7/0.5/0.5/0.4 to 0.40/0.20/0.15/0.15/0.10 everywhere they are defined or used (including the occurrences referenced around lines 26-32) and ensure the compat-floor threshold (the blast_radius >= 2.0 check) is reviewed/adjusted if the threshold in plan differs so the rollup logic and the header comment stay in sync.docs/architecture/extension-api-v2/README.md-50-69 (1)
50-69:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThis “Top 12” table is out of sync with
touch-points-rollup.yaml.For example,
S6.A1is7.016 / 15 / 23604in the checked-in rollup, not6.67 / 7 / 17101here. Because this README is presented as the canonical prioritization view, stale rankings will steer follow-up work toward the wrong surfaces.🤖 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/architecture/extension-api-v2/README.md` around lines 50 - 69, The Top 12 table in README.md is stale versus the authoritative touch-points-rollup.yaml; regenerate the table by reading touch-points-rollup.yaml, recomputing blast radius using the formula shown (blast radius = log10(1+stars)*1.0 + log10(1+occurrences)*0.7 + (signature_count-1)*0.5 + silent_breakage*0.5 + lifecycle_coupling*0.4), sort by BR and replace the table so entries like S6.A1, S9.SG1, etc. match the rollup (ensure S6.A1 shows 7.016 / 15 / 23604 as in the rollup), and consider adding a small regeneration note or script reference so README.md stays in sync with touch-points-rollup.yaml.docs/architecture/extension-api-v2/scripts/fetch-stars.sh-11-14 (1)
11-14:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreflight
jqbefore the first success path.The script exits cleanly when
ghis missing, but a missingjqfails only after the first successful response and can leave a partially written cache. Check both tools up front.Suggested fix
if ! command -v gh >/dev/null 2>&1; then echo "❌ gh CLI not installed" exit 1 fi + +if ! command -v jq >/dev/null 2>&1; then + echo "❌ jq not installed" + exit 1 +fiAlso applies to: 39-43
🤖 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/architecture/extension-api-v2/scripts/fetch-stars.sh` around lines 11 - 14, Add a preflight check for jq alongside the existing gh check in fetch-stars.sh so both tools are verified before any network calls or cache writes; specifically, ensure the script verifies command -v gh and command -v jq early (the same place that currently checks gh) and fail fast with a clear error and exit if either is missing (also update the duplicate check around the block referenced by the second occurrence to avoid late failures and partial cache writes).docs/architecture/extension-api-v2/scripts/merge-staging-pass3.py-24-31 (1)
24-31:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPoint this merger at the canonical database file.
fetch-stars.shandrollup-blast-radius.pyboth readtouch-points-database.yaml, but this script writesresearch/touch-points/database.yaml. As written, pass-3 merges will not feed the dataset that the rest of the pipeline and CI consume.Suggested fix
-DB = ROOT / "research" / "touch-points" / "database.yaml" +DB = ROOT / "touch-points-database.yaml"🤖 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/architecture/extension-api-v2/scripts/merge-staging-pass3.py` around lines 24 - 31, The DB constant is pointing to "database.yaml" but the canonical dataset used by fetch-stars.sh and rollup-blast-radius.py is "touch-points-database.yaml"; update the DB assignment (the DB variable near ROOT and STAGING) to reference the canonical file name "touch-points-database.yaml" so pass-3 merges feed the same dataset the rest of the pipeline/CI consume (leave R8, R9_SEC, R9_GUIDES, R9_CK unchanged).src/extension-api-v2/__tests__/bc-13.v1.test.ts-14-53 (1)
14-53:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftImplement executable tests for BC.13 v1 serialization contract.
This suite currently contains only pending
it.todo(...)placeholders with no executable assertions. Per coding guidelines, tests must be written for all changes, especially for high-blast-radius features like BC.13 v1's per-node serialization interception. The empty suite passes CI without validating any behavior—prototype.serialize patching, onSerialize callbacks, or the NaN→null silent corruption edge cases remain untested at runtime.Replace at least the critical paths (prototype.serialize chaining, onSerialize mutation, NaN coercion) with executable test implementations.
🤖 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 `@src/extension-api-v2/__tests__/bc-13.v1.test.ts` around lines 14 - 53, Replace the it.todo placeholders with real assertions that exercise the BC.13 v1 serialization contract: implement tests that (1) patch node.constructor.prototype.serialize and call the original via origSerialize.call(this) to assert the returned object is the base serialization and that added custom fields are persisted to the workflow JSON on disk (use the same describe block and test title text to locate the case), (2) assign node.onSerialize = fn and assert fn is invoked with the serialization data object after base serialize, that fn may mutate data.myData in place and that mutation appears in the saved JSON, and (3) create a widget whose serializeValue returns NaN and assert JSON.stringify coerces it to null in widgets_values and that restoring the graph reads null back (use symbols graphToPrompt, node.onSerialize, serializeValue, and prototype.serialize to find relevant helpers/mocks); make each test perform setup, call the real serialization path (e.g., graphToPrompt or save/restore helper), and include explicit expect assertions instead of todos.docs/architecture/extension-api-v2/scripts/rollup-blast-radius.py-63-67 (1)
63-67:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDon't use
severityas the silent-breakage input.The plan defines this term from the worst observed
breakage_classin evidence, but the current fallback mapsCRITICAL/HIGH/...into the silent-breakage slot. That conflates impact with failure mode and skews the ranking even when the actual breakage mode is only loud or crash-only.Suggested direction
- sev_map = {"CRITICAL": 2, "HIGH": 1.5, "MEDIUM": 1, "LOW": 0.5} - silent_w = float(h.get("silent_breakage", sev_map.get(p.get("severity", ""), 0))) + breakage_weights = { + "silent": 1.0, + "undefined-behavior": 0.6, + "loud": 0.3, + "crash": 0.2, + } + silent_w = float( + h.get( + "silent_breakage", + max( + ( + breakage_weights.get(e.get("breakage_class"), 0) + for e in evidence + ), + default=0, + ), + ) + )🤖 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/architecture/extension-api-v2/scripts/rollup-blast-radius.py` around lines 63 - 67, The code incorrectly uses p.get("severity") when computing silent_w, conflating impact with breakage mode; change the fallback for the silent_breakage lookup so sev_map is keyed from p.get("breakage_class") (or use p.get("breakage_class","") as the lookup key) instead of p.get("severity"), leaving the explicit h.get("silent_breakage") override intact; update the line that computes silent_w (referencing variables p, h, sev_map, and silent_w) to map breakage_class→sev_map for the default.docs/architecture/extension-api-v2/scripts/merge-staging-pass3.py-36-45 (1)
36-45:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReplace both
eval(...)calls withast.literal_eval(...).These values come from YAML staging files.
eval()remains an execution primitive here even with restricted builtins, and the blanketexcept Exceptionmasks malformed input.literal_eval()plus(SyntaxError, ValueError)handling safely parses literal Python structures without executing arbitrary expressions.Occurs at lines 40 and 132.
Suggested fix
+from ast import literal_eval import sys from pathlib import Path @@ def normalize_lines(lines): if isinstance(lines, str): - # R8 emitted strings like "[119, 131]" — convert try: - return tuple(eval(lines, {"__builtins__": {}}, {})) - except Exception: + return tuple(literal_eval(lines)) + except (SyntaxError, ValueError): return (lines,) @@ evidence_field = sp.get("evidence") if isinstance(evidence_field, str): try: - evidence_field = eval(evidence_field, {"__builtins__": {}}, {}) - except Exception: + evidence_field = literal_eval(evidence_field) + except (SyntaxError, ValueError): evidence_field = []🤖 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/architecture/extension-api-v2/scripts/merge-staging-pass3.py` around lines 36 - 45, The code uses eval(...) in two places (e.g., inside normalize_lines) to parse YAML-derived strings—replace both eval calls with ast.literal_eval and import ast at the top if not present; change the broad except Exception to only catch (SyntaxError, ValueError) and fall back to returning (lines,) to preserve current behavior while avoiding execution of arbitrary code and not masking unrelated errors.src/services/extensionV2Service.ts-132-133 (1)
132-133:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not hand out live ECS references from these getters.
getOptions(),getPosition(), andgetSize()currently return the exact objects stored in the World. An extension can mutate those in place and bypass dispatch/undo/CRDT propagation entirely. These should return shallow snapshots, the same waygetProperties()already does.Proposed fix
getOptions(): WidgetOptions { - return world.getComponent(widgetId, WidgetValue).options ?? {} + return { + ...(world.getComponent(widgetId, WidgetValue).options ?? {}) + } }, @@ getPosition(): Point { - return world.getComponent(nodeId, Position).pos + return [...world.getComponent(nodeId, Position).pos] as Point }, getSize(): Size { - return world.getComponent(nodeId, Dimensions).size + return [...world.getComponent(nodeId, Dimensions).size] as Size },As per coding guidelines, "Every entity state change must be a serializable, idempotent, deterministic command (Command pattern) - replayable, undoable, and transmittable over CRDT."
Also applies to: 183-188
🤖 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 `@src/services/extensionV2Service.ts` around lines 132 - 133, getOptions(), getPosition(), and getSize() currently return live references from world.getComponent(widgetId, WidgetValue).options/position/size; change each to return a shallow snapshot instead (e.g., create and return a shallow copy like { ...orig } or Array.from for arrays) so callers cannot mutate ECS state directly. Locate the getters named getOptions, getPosition, and getSize and replace their return expressions to return a cloned object/array of the corresponding WidgetValue field (use the same snapshot pattern as getProperties()), ensuring you reference widgetId and world.getComponent(widgetId, WidgetValue) to read the source before copying. Ensure the snapshots are shallow (not deep) to match existing behavior and preserve serializability/command patterns.src/services/extensionV2Service.ts-317-326 (1)
317-326:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
defineWidgetExtension()is currently inert.The registry stores widget extensions, but the mount system only ever iterates
nodeExtensions.widgetCreated()is never invoked, and the returnedrender/destroylifecycle is never wired up, so consumers can successfully register a public widget extension that silently never runs.Also applies to: 346-378, 390-413
🤖 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 `@src/services/extensionV2Service.ts` around lines 317 - 326, The widget registry currently collects entries into widgetExtensions via defineWidgetExtension but the mount flow only iterates nodeExtensions and never calls widgetCreated or wires up the render/destroy lifecycle; update the mount/initialization code that currently iterates nodeExtensions to also iterate widgetExtensions (or create a parallel iteration) so that for each WidgetExtensionOptions you call its widgetCreated hook and register its returned render and destroy callbacks into the same lifecycle management as nodes; locate where nodeExtensions is consumed and add analogous handling for widgetExtensions so widgetCreated, render and destroy are invoked and cleaned up correctly.src/types/extensionV2.ts-133-143 (1)
133-143:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAdd a typed teardown hook before freezing
NodeExtensionOptions.
NodeExtensionOptionscurrently has creation hooks only, but the BC.02 v2/migration scaffolds in this PR already requireonRemoved(handle). Without a first-class teardown callback, extensions have no typed place to release timers, observers, or other node-scoped resources on deletion.🤖 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 `@src/types/extensionV2.ts` around lines 133 - 143, Add a typed teardown hook to NodeExtensionOptions so extensions can release node-scoped resources: update the NodeExtensionOptions interface to include an optional onRemoved(node: NodeHandle): void method (mirroring nodeCreated/loadedGraphNode signatures) before the interface is frozen; ensure the new onRemoved is exported with the interface so migration scaffolds and runtime callers can call extension.onRemoved(handle) when a node is deleted.src/types/extensionV2.ts-118-129 (1)
118-129:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftBC.04 interaction events are missing from the published
NodeHandlesurface.The new compat scaffolds in this PR already treat
mousedown,selected/deselected, andresizeas canonical v2 events, but the stable type surface only exposespositionChanged/sizeChanged/modeChanged. Shipping Phase A like this means a follow-up PR has to add new public overloads later or keep the spec/tests permanently ahead of the contract.🤖 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 `@src/types/extensionV2.ts` around lines 118 - 129, The NodeHandle type currently only exposes on(...) overloads for positionChanged/sizeChanged/modeChanged but misses the compat event overloads used elsewhere; add public on(...) overloads for the interaction events used by the new scaffolds—specifically include on(event: 'mousedown', fn: (e: MouseEvent | {button:number, x:number, y:number}) => void): void, on(event: 'selected'|'deselected', fn: () => void): void, and on(event: 'resize', fn: (size: Size) => void): void (alongside existing on signatures) so the NodeHandle interface (where positionChanged/sizeChanged/modeChanged are declared) exposes the same canonical v2 events as the runtime scaffolds.src/services/extensionV2Service.ts-390-412 (1)
390-412:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd
immediate: trueto watch options to mount extensions for pre-existing nodes.Without
immediate: true, the watcher is lazy and skips the callback on initial setup. This means any nodes that already exist in the World whenstartExtensionSystem()runs—preloaded workflows, bootstrap-time nodes—won't have extensions mounted untilworld.queryAll(NodeType)changes again.Fix
watch( () => world.queryAll(NodeType), (currentIds, previousIds) => { const prev = new Set(previousIds ?? []) const curr = new Set(currentIds) for (const nodeId of currentIds) { if (!prev.has(nodeId)) { mountExtensionsForNode(nodeId) } } for (const nodeId of previousIds ?? []) { if (!curr.has(nodeId)) { unmountExtensionsForNode(nodeId) } } }, - { flush: 'post' } + { flush: 'post', immediate: true } )🤖 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 `@src/services/extensionV2Service.ts` around lines 390 - 412, The watcher in startExtensionSystem currently omits immediate:true so pre-existing nodes are skipped; update the watch call in startExtensionSystem (the watch of () => world.queryAll(NodeType)) to include immediate: true in the options alongside flush: 'post' so the callback runs once at setup and mountExtensionsForNode is invoked for existing nodes; keep the existing logic for unmounting unchanged.package.json-50-52 (1)
50-52:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCI is blocked by lockfile/specifier drift; sync lockfiles before merge.
The new scripts are fine, but this PR is currently non-mergeable because multiple jobs fail on
--frozen-lockfilewith specifier mismatches. Please regenerate and commit the lockfile updates for all touched package manifests in this stack.🤖 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 50 - 52, CI is failing due to lockfile/specifier drift caused by changes to package manifests (e.g., the new scripts like "test:extension-api", "test:extension-api:watch", "test:extension-api:coverage" in package.json); regenerate the lockfiles for every affected package manifest by running your package manager's install/lockfile update (e.g., npm/yarn/pnpm install or pnpm -w install) so the lockfile matches the updated package.json, verify builds pass locally with the same flags used in CI (including --frozen-lockfile), and commit the updated lockfile(s) alongside the package.json changes before pushing.src/extension-api-v2/__tests__/v2/BC.18.v2.test.ts-5-9 (1)
5-9:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftCompat-floor is declared but currently non-enforcing.
Line 6 through Line 8 are all
it.todo(...), so this suite can’t fail when BC.18 behavior regresses. For compat-floor coverage, at least one executable assertion per contract branch should be included in this PR (or gate merge on follow-up completion).As per coding guidelines, "Write tests for all changes, especially bug fixes to catch future regressions."
🤖 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 `@src/extension-api-v2/__tests__/v2/BC.18.v2.test.ts` around lines 5 - 9, The test suite currently uses only it.todo entries so compat-floor isn't enforced; replace at least one it.todo in BC.18 v2 — Backend HTTP calls with an executable test that asserts the expected behavior (for example, change the 'defineExtension setup exposes an api object with a fetchApi(path, init?) method' todo into an actual it(...) that calls the extension setup helper and asserts the returned api object has a fetchApi function and that calling it returns a Promise-like Response); target the test in src/extension-api-v2/__tests__/v2/BC.18.v2.test.ts and reference the defineExtension/setup helper and the api.fetchApi symbol so the test fails on regressions and satisfies the compat-floor requirement.src/extension-api-v2/__tests__/bc-01.v2.test.ts-11-40 (1)
11-40:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftReplace TODO-only contract placeholders with at least one runnable assertion per contract block before merge.
From Line 11 onward, the suite is entirely
it.todo(...), so it can’t detect BC.01 regressions even when CI is green. Keep TODOs for backlog depth, but add minimal executable assertions now for lifecycle ordering and handle semantics.Example pattern to start converting placeholders
- it.todo( - 'nodeCreated is called once per node instance and receives a NodeHandle wrapping the created node' - ) + it( + 'nodeCreated is called once per node instance and receives a NodeHandle wrapping the created node', + async () => { + // arrange test graph + extension registration + // create one node instance + // assert handler called once with a NodeHandle + } + )As per coding guidelines: "Write tests for all changes, especially bug fixes to catch future regressions".
🤖 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 `@src/extension-api-v2/__tests__/bc-01.v2.test.ts` around lines 11 - 40, The test file contains only it.todo placeholders so BC.01 checks never run; replace at least one todo in each contract block with a minimal runnable assertion: in the "nodeCreated(handle) — per-instance setup" describe, replace one it.todo with an it that invokes the node lifecycle or a minimal smoke assertion asserting nodeCreated is called or NodeHandle properties exist (reference nodeCreated, NodeHandle.setState, NodeHandle.id, NodeHandle.vueRef); in the "type-level registration (replacement for S2.N8)" describe, replace one it.todo with an it that registers a simple defineNodeExtension({ types: [...] }) or omits types and asserts the appropriate handler was invoked for registered vs unregistered types (reference defineNodeExtension and type-scoped registration); keep other todos, make assertions minimal and deterministic so CI fails on regressions.packages/extension-api/scripts/build-docs.ts-405-410 (1)
405-410:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse argv-based process invocation instead of shell command strings for running TypeDoc.
The current
execSyncwith a shell command string (npx typedoc --options ${...}) creates an unnecessary command-injection risk and lacks error handling. Pass command arguments as an array instead so paths are treated as data, not shell syntax.🔧 Proposed fix
-import { execSync } from 'node:child_process' +import { spawnSync } from 'node:child_process' function runTypedoc(): void { console.log('▶ Running TypeDoc...') - execSync( - `npx typedoc --options ${path.join(pkgRoot, 'typedoc.json')} --out ${rawDir}`, - { cwd: pkgRoot, stdio: 'inherit' } - ) + const bin = process.platform === 'win32' ? 'npx.cmd' : 'npx' + const result = spawnSync( + bin, + [ + 'typedoc', + '--options', + path.join(pkgRoot, 'typedoc.json'), + '--out', + rawDir + ], + { cwd: pkgRoot, stdio: 'inherit' } + ) + if (result.status !== 0) { + throw new Error( + `TypeDoc failed with exit code ${String(result.status ?? 'unknown')}` + ) + } }🤖 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 `@packages/extension-api/scripts/build-docs.ts` around lines 405 - 410, The runTypedoc function currently calls execSync with a single shell command string which risks command injection; replace that call with a no-shell invocation using child_process.spawnSync or child_process.execFileSync, passing "npx" (or the typedoc binary path) as the command and an args array like ["typedoc","--options", path.join(pkgRoot,'typedoc.json'), "--out", rawDir] (or ["typedoc", "--options", ..., "--out", ...] if invoking the binary directly) and keep stdio: 'inherit'; also wrap the call in try/catch to handle and surface errors (log via console.error and rethrow or exit) so failures in runTypedoc are properly reported..github/workflows/ci-tests-extension-api.yaml-15-37 (1)
15-37:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAdd job-level path conditionals instead of trigger-level filters.
This workflow uses
on.push.pathsandon.pull_request.pathsto filter triggers. If these jobs are (or will be) required status checks on main, this pattern prevents check runs from being created on docs-only PRs, leaving checks stuck as pending. Move path filtering to job-levelif:conditions so GitHub still creates check runs but marks them as skipped when path conditions aren't met.For example:
jobs: test: if: | contains(fromJson('["src/extension-api/**", "src/extension-api-v2/**", "packages/extension-api/**", "vitest.extension-api.config.mts", "research/touch-points/rollup.yaml", "research/touch-points/behavior-categories.yaml", "scripts/check-compat-floor.py", "pnpm-lock.yaml"]'), github.event.pull_request.head.ref) || github.event_name == 'push' runs-on: ubuntu-latest # ... rest of jobAlternatively, use a workflow-level output or matrix strategy to centralize path logic.
🤖 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 @.github/workflows/ci-tests-extension-api.yaml around lines 15 - 37, The workflow currently uses trigger-level path filters (on.push.paths and on.pull_request.paths) which prevent GitHub from creating check runs for PRs; change to job-level path conditionals by removing the paths entries under on.push and on.pull_request and adding an if: condition on each job (e.g., the test job) that evaluates whether the event is a push or the PR touches the desired paths (use contains(fromJson(...), <path>) or equivalent logic combined with github.event_name == 'push'); ensure each job (for example the job named test or any jobs under jobs:) uses that if: so checks are created but skipped when irrelevant.
| if "evidence-sweep-pass-2" not in db["meta"].get("sweeps_done", []): | ||
| db["meta"]["sweeps_done"].append("evidence-sweep-pass-2") |
There was a problem hiding this comment.
Initialize meta.sweeps_done before appending to avoid a runtime crash.
If sweeps_done is missing, Line 256 throws KeyError even though Line 255 uses .get(...).
🐛 Proposed fix
- if "evidence-sweep-pass-2" not in db["meta"].get("sweeps_done", []):
- db["meta"]["sweeps_done"].append("evidence-sweep-pass-2")
+ sweeps_done = db["meta"].setdefault("sweeps_done", [])
+ if "evidence-sweep-pass-2" not in sweeps_done:
+ sweeps_done.append("evidence-sweep-pass-2")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if "evidence-sweep-pass-2" not in db["meta"].get("sweeps_done", []): | |
| db["meta"]["sweeps_done"].append("evidence-sweep-pass-2") | |
| sweeps_done = db["meta"].setdefault("sweeps_done", []) | |
| if "evidence-sweep-pass-2" not in sweeps_done: | |
| sweeps_done.append("evidence-sweep-pass-2") |
🤖 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/architecture/extension-api-v2/scripts/add-evidence-pass2.py` around
lines 255 - 256, The code assumes db["meta"]["sweeps_done"] exists before
appending, causing a KeyError when it's missing; update the block around
db["meta"] and "sweeps_done" to initialize sweeps_done as an empty list if
absent (e.g., use db["meta"].setdefault("sweeps_done", []) or check/get and
assign an empty list) before checking membership and appending
"evidence-sweep-pass-2" so db["meta"]["sweeps_done"].append(...) never raises.
| "devDependencies": { | ||
| "tsx": "catalog:", | ||
| "typedoc": "0.28.19", | ||
| "typedoc-plugin-markdown": "^4.6.3", | ||
| "typescript": "catalog:" | ||
| }, |
There was a problem hiding this comment.
Critical: Update pnpm-lock.yaml to reflect new devDependencies.
All pipeline failures report lockfile mismatch. After adding these devDependencies, you must run pnpm install at the repository root to update pnpm-lock.yaml. The frozen-lockfile CI checks will continue to fail until the lockfile is synchronized.
🔧 Resolution steps
# From repository root:
pnpm install
git add pnpm-lock.yaml
git commit -m "chore: update lockfile for extension-api devDependencies"🤖 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 `@packages/extension-api/package.json` around lines 16 - 21, The repo lockfile
is out of sync after adding devDependencies ("tsx" and "typescript" entries) to
packages/extension-api package.json; run pnpm install from the repository root
to regenerate pnpm-lock.yaml, stage the updated pnpm-lock.yaml, and commit
(e.g., commit message like "chore: update lockfile for extension-api
devDependencies") so CI frozen-lockfile checks pass.
| it.todo('v1 nodeType.prototype.onConnectionsChange fires with the same (type, index, connected, linkInfo) args as v2 connectionChanged event') | ||
| it.todo('v1 nodeType.prototype.onConnectInput returning false vetoes the connection identically to v2 connectInput returning false') | ||
| it.todo('v1 nodeType.prototype.onConnectOutput returning false vetoes the connection identically to v2 connectOutput returning false') | ||
| it.todo('v2 connectionChanged event fires once per connect and once per disconnect, matching v1 onConnectionsChange invocation parity') |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify naming consistency for BC.07 connection events in extension-api test scaffolds.
rg -n --type=ts -C2 'connectionChanged|connectionChange|connectInput|connectOutput' src/extension-api-v2/__tests__Repository: Comfy-Org/ComfyUI_frontend
Length of output: 14878
Update BC.07.migration.test.ts to use canonical event name 'connectionChange' before implementing.
The v2 contract uses 'connectionChange' (not 'connectionChanged'). Lines 6 and 9 currently encode the wrong parity target. The canonical event name is confirmed across bc-07.v2.test.ts and bc-08.v2.test.ts. Correct the event names in the TODO descriptions before implementation to ensure migration parity tests target the correct v2 API surface.
🤖 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 `@src/extension-api-v2/__tests__/migration/BC.07.migration.test.ts` around
lines 6 - 9, Update the TODO descriptions in BC.07.migration.test.ts that
reference the v2 event name: replace the incorrect 'connectionChanged' with the
canonical 'connectionChange' so the parity targets the right v2 API;
specifically edit the two TODO strings (the one comparing v1 onConnectionsChange
parity and the one asserting event firing per connect/disconnect) to use
'connectionChange' to match bc-07.v2.test.ts and bc-08.v2.test.ts.
1 participant
Phase A integration branch for the new extension API (
@comfyorg/extension-api).This PR is the base of a 4-PR draft stack. Each child is a separate review surface for narrow scope.
Stack
ecs-vue-hoisted-client-state-hook-apimainext-api/i-foundationext-api/i-pkg@comfyorg/extension-apinpm package + TypeDoc → MDX docgen + CI workflows (sidecar)ext-api/i-tfext-api/i-extWhat is in this base PR
docs/architecture/extension-api-v2/— touch-points database, plan, rollup, evidence-fetching scripts (R7+R8+R9 outputs)src/extension-api/— initial public declaration files (NodeHandle, WidgetHandle, defineExtension, lifecycle, events, identifiers, shell)src/extension-api-v2/__tests__/— first-pass nested test stubs ([Phase A · Child] ext-api/i-tf — test framework reorg + harness + content fill (I-TF.2/3/6) #12104 reorganises these into flat layout)src/services/extensionV2Service.ts— surface shim POC ([Phase A · Foundation] ext-api/i-foundation — declarations + service rename + scope registry #12102 renames this →extension-api-service.ts)src/extensions/core/{dynamicPrompts,imageCrop,previewAny}.v2.ts— three sample v2 extensionspackages/extension-api/scripts/build-docs.ts— TypeDoc → MDX buildervitest.extension-api.config.mts+scripts/check-compat-floor.py+.github/workflows/ci-tests-extension-api.yaml— test framework infraCoworker fork point
Coworkers converting core extensions should branch off
ext-api/i-foundation(#12102) in parallel to #12105.Refs
Draft for review collaboration via inline comments.
┆Issue is synchronized with this Notion page by Unito