[Phase A · Child] ext-api/i-tf — test framework reorg + harness + content fill (I-TF.2/3/6)

[christian-byrne]

Stacks on: ext-api/i-foundation
Sibling: ext-api/i-ext

What's in

• Layout reorg: nested __tests__/{v1,v2,migration}/BC.XX/ → flat __tests__/bc-XX.{v1,v2,migration}.test.ts (124 deletions, 121 fills)
• Harness (src/extension-api-v2/harness/):
  • synthetic mini-ComfyApp + World stub
  • loadEvidenceSnippet() pulling R8 clone-and-grep excerpts from snapshot of touch-point database
  • runV1 / runV2 runners (stubs until Phase B)
• vitest config: vitest.extension-api.config.mts adjusted for flat layout
• BC coverage: 41 categories × 3 stub types (v1/v2/migration)

Note

The deletion of the old src/services/extensionV2Service.ts is in this PR rather than i-foundation (where the canonical extension-api-service.ts is created). Final merged state is correct; flagging for reviewer awareness.

Refs

I-TF.1 (done) · R8 clone-and-grep evidence (887 rows, 52/56 patterns) · D9 strangler-fig phases

Draft for review collaboration via inline comments.

┆Issue is synchronized with this Notion page by Unito

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 63a550c1-78ff-4a0d-805f-5cd6f199a2aa

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch ext-api/i-tf

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[github-actions]

🎨 Storybook: ✅ Built — View Storybook

Details

⏰ Completed at: 05/11/2026, 03:51:55 AM UTC

Links

• 📊 View Workflow Run

[github-actions]

🎭 Playwright: 🕵🏻 0 passed, 0 failed

📊 Browser Reports

• chromium: ❌ Deployment failed
• chromium-2x: ❌ Deployment failed
• chromium-0.5x: ❌ Deployment failed
• mobile-chrome: ❌ Deployment failed

[github-actions]

📦 Bundle Size

⏳ Size data collection in progress…

⚡ Performance

⚠️ Performance tests failed. Check the CI workflow logs.

[christian-byrne]

Adversarial Architectural Review — I-TF.2/3/6

Sev	Location	Issue	Fix
major	harness/loadEvidenceSnippet.ts:41	Module-level mutable singleton evidenceIndex — tests running in the same Vitest worker share this lazily-built Map with no reset path. Harmless now (DB is read-only JSON), but the singleton also has a shadowing collision: the parameter evidenceIndex on line 65 of loadEvidenceSnippet() silently shadows the module-level evidenceIndex that getIndex() mutates.	Rename the parameter (e.g. rowIndex) or extract getIndex() closer to the data. Mutable module globals in test helpers are a reliability trap once parallel test workers or DB mutation land.
major	harness/loadEvidenceSnippet.ts:71	Error message references touch-point-database.yaml but the actual file is touch-point-database.json. Tests that hit an unknown patternId will produce a misleading debug path.	Fix the error string.
major	bc-01.v2.test.ts:28–92, bc-01.migration.test.ts:42–91, and ~7 other files	createTestRuntime() / createV2Runtime() are individually re-implemented inline in each BC file. Each copy re-implements the same sorting logic ([...extensions].sort((a,b) => a.name.localeCompare(b.name))), NodeHandle stub, and mountNode dispatch — with subtle shape differences (some use NodeEntityId, some raw number, some string). When Phase B replaces these stubs with the real mountExtensionsForNode, every copy diverges independently.	Extract a shared src/extension-api-v2/harness/testRuntime.ts (or extend the existing harness/index.ts exports) with a canonical createV2TestRuntime() that all BC files import. Copies can always add per-test overrides, but the sorting/mounting contract should be a single source.
major	bc-05.v2.test.ts:14–39, bc-05.migration.test.ts:14–39, bc-11.v2.test.ts:14–39, bc-11.migration.test.ts:14–39	World mock boilerplate duplicated across 4+ files — identical vi.mock('@/world/worldInstance'), vi.mock('@/world/widgets/widgetComponents'), vi.mock('@/world/entityIds'), vi.mock('@/world/componentKey') blocks, each with the same mockGetComponent = vi.fn() setup and identical stubNodeType() helper. When Phase B changes the real World API, all four need simultaneous updates, and divergence is guaranteed.	Move the 5 vi.mock calls + mockGetComponent + stubNodeType into a shared harness/worldMocks.ts file and import from there. Vitest's vi.mock factory runs per-module anyway so the hoisting constraint doesn't prevent extraction.
minor	harness/runV1.ts and harness/runV2.ts	Both runners are stubs that do nothing with the snippet string (no eval, no registerExtension call). Tests that import them use runV1(snippet, { app }) with the genuine expectation it will exercise the snippet — but it doesn't. The v1 tests currently testing "snippet is capturable by runV1 without throwing" are asserting vacuously true (a do-nothing function never throws). This is fine for Phase A but it creates a false green — the tests pass today not because the snippet is safe but because the runner ignores it entirely.	Add a clearly visible comment/assertion in the test itself: // runV1 is a no-op stub in Phase A — this test only proves the import chain, not snippet execution. Keeps the intent honest and avoids future contributors thinking these tests are real coverage.
minor	bc-23.migration.test.ts:24, bc-24.v2.test.ts:26, bc-25.v2.test.ts:26, bc-26.v2.test.ts:25	void [loadEvidenceSnippet, runV1, runV2] is being used to suppress the unused-import lint warning. This is a symptom: runV2 is imported but the test doesn't use it yet (the eval sandbox doesn't exist).	Either drop the import and re-add it when the sandbox lands, or use a /* eslint-disable-next-line */ with an explanatory comment. void [...] is not an idiom the codebase uses elsewhere and it obscures intent.
minor	harness/comfyApp.ts — MiniComfyApp.world field	The harness MiniComfyApp exposes world: HarnessWorld directly on the public surface. This leaks internal test-harness plumbing (HarnessNodeRecord, addNode, clear) to any test that receives a MiniComfyApp. When Phase B replaces HarnessWorld with the real @/ecs/world, every test casting .world will break.	Mark world as _world or suffix it /* @internal */. Better: gate advanced world access behind a type cast in the test that actually needs it, rather than exposing it on the main interface.

---

Detailed findings

F1 — evidenceIndex parameter shadow (major)

loadEvidenceSnippet.ts uses evidenceIndex as both the module-level cache (let evidenceIndex: Map<…> | null = null) and as the numeric evidenceIndex = 0 parameter to loadEvidenceSnippet(patternId, evidenceIndex). Inside the function body, line 81 (const row = withExcerpts[evidenceIndex]) resolves to the parameter (a number), which is correct — but getIndex() mutates the module-level Map. The shadowing makes the two roles visually indistinguishable and will confuse the next author touching this file. The error message typo (.yaml vs .json) compounds the confusion.

F2 — Inline runtime factories diverge before Phase B (major)

The inline createTestRuntime() / createV2Runtime() copies all implement D10b (lexicographic sort) independently. In bc-01.v2.test.ts line 76, the sort is a.name.localeCompare(b.name). In bc-01.migration.test.ts line 79 it's the same. So far so good. But the copies have already started to drift in how they construct NodeHandle stubs: bc-01.v2.test.ts builds a full stub with as unknown as NodeHandle; bc-03.v2.test.ts builds a plain object and casts it with a different field set. This is the exact divergence pattern that makes Phase B replacements expensive — each file has locally-reasonable simplifications that collectively mean no single file is a canonical reference for "what does the Phase A → Phase B migration look like?"

The fix (canonical createV2TestRuntime() in the harness) also directly solves the as unknown as NodeHandle anti-pattern: four files use this forced cast because there's no shared test-friendly NodeHandle builder. A harness factory with sensible defaults eliminates the need.

F3 — World mock boilerplate accumulation (major)

The 4-file duplication of the vi.mock('@/world/…') block is a Phase B time-bomb. When @/world/worldInstance API changes (which it will — it's a sibling-PR stub that will be replaced by the real ECS World), all four files need simultaneous updates. The stubNodeType helper is also 1-for-1 identical across all four copies. Centralising this into harness/worldMocks.ts aligns with the principle that the harness owns the stable seam between tests and the not-yet-stable World internals.

F4 — Vacuous "no-throw" tests against no-op runners (minor)

The pattern expect(() => runV1(snippet, { app })).not.toThrow() appears across most bc-*.v1.test.ts files. Since runV1 ignores the snippet string entirely, these tests cannot fail no matter what the snippet contains — even a syntax-invalid string would pass because the string is never evaluated. This is fine as scaffolding but it's not labeled as scaffolding, which creates a false confidence signal in CI.

F5 — Phase C todo gaps (minor, extensibility concern)

bc-07.v2.test.ts:231–235 has todos for veto/intercept semantics tagged [Phase B], but the D9 decision is that prototype-patching interception is explicitly Phase C, not Phase B. The veto semantic (returning false from connectInput to prevent a link) is also noted as "if adopted in Phase B API" — indicating it's not a committed API surface. Mislabeling veto semantics as Phase B risks test infrastructure being claimed as "ready for Phase B" when the underlying API decision isn't made. Consider tagging these [Phase B/C — API not committed] to reflect D9 §Phase B: "veto/intercept: prototype-patching interception is not a Phase B concern."

F6 — MiniComfyApp.world exposure (minor, layering)

D9 Phase B replaces HarnessWorld with the real World. Any test that does app.world.addNode(...) or app.world.clear() is coupling to the harness-internal World shape. If world is on the public MiniComfyApp interface, the migration cost is invisible until Phase B rewrites arrive. Gate it or make the internal dependency explicit.

---

What's well-designed

• The flat bc-NN.{v1,v2,migration}.test.ts layout is a genuine improvement: grepping by BC number now works, and the files are easier to diff across versions.
• The HarnessWorld interface is cleanly separated from MiniComfyApp — the dependency direction is right.
• Phase A / Phase B / Phase C todo tagging in most files is accurate and useful (F5 is the exception, not the rule).
• The compat-floor CI gate (check-compat-floor.py) is the right architectural enforcement mechanism: it prevents shipping v2 without coverage on high-blast-radius categories.
• Evidence-fixture loading via loadEvidenceSnippet is the correct long-term shape — the runner stubs are honest about their scope.

[christian-byrne]

Adversarial review — Minimalist (agent: codex)

#	Severity	Location	Issue
1	High	src/extension-api-v2/harness/runV1.ts, runV2.ts	Both runners ignore the snippet arg entirely (only typeof snippet === 'string' check) and just hand back the harness. Tests built on top — expect(() => runV1(snippet, { app })).not.toThrow() — are tautological: a function that does nothing cannot throw.
2	High	src/extension-api-v2/__tests__/bc-*.test.ts (121 files, ~22k LOC)	~405 it.todo/it.skip interleaved with ~802 real it(s, many of which test the mock itself (e.g. bc-01.v1.test.ts L54-63 calls extension.nodeCreated(fakeNode) and then asserts extension.nodeCreated was called once). Repo AGENTS.md is explicit: "Do not write tests that just test the mocks" + "Be parsimonious in testing".
3	High	PR scope	Description claims "test framework reorg + harness + content fill" yet ships 3,537 lines of new product code outside the harness: src/extension-api/* (1,714), src/services/extension-api-service.ts (643), src/world/*, src/extensions/core/*.v2.ts, packages/extension-api/* (incl. 470-line build-docs.ts). Per project guideline ("If 300+ lines of non-test code, suggest splits") — split, please.
4	Medium	src/services/extension-api-service.ts:96-111	_setDispatchImplForTesting + _dispatchImpl mutable module-level state exposes a test seam on the public service. dispatch() itself is a no-op that console.warns in DEV. Until ECS commands land in #11939, delete dispatch and the test-only setter — call sites can DI a dispatcher when needed.
5	Medium	src/services/extension-api-service.ts (_clearExtensionsForTesting, getScopeRegistry)	Test-only escape hatches on the service surface. Either move to a __tests__/internal.ts import or namespace under __test__.
6	Medium	src/extension-api-v2/harness/loadEvidenceSnippet.ts:41-51	Lazy memoisation (evidenceIndex || build()) of an already-resident JSON import. Build the map once at module load: const evidenceIndex = new Map(typedDb.patterns.map(p => [p.pattern_id, p.evidence ?? []])). Removes the conditional and the null typing.
7	Low	src/extension-api-v2/harness/{world.ts,comfyApp.ts} (172 LOC)	Carefully shaped HarnessWorld/MiniGraph/MiniExtensionRegistration with default geometry ([200,100], title: input.type, etc.) — but no consumer of these defaults exists because runV1/runV2 never execute snippets. Until snippets actually run, this is engineering for a use case Phase B will rewrite. Trim to whatever the ~30 currently-running assertions actually touch.
8	Low	packages/extension-api/scripts/build-docs.ts (470 LOC) + typedoc.json	A bespoke 470-line doc builder and a typedoc config in the same package. Pick one. If typedoc is sufficient (it usually is for .d.ts surface generation), drop the script.

Net

Roughly 22k lines to land scaffolding whose only currently-exercising assertion is "the stub didn't throw". The harness, the dispatch test-seam, and the doc builder are all reasonable Phase B work — but landing them now costs review surface and locks shape choices before there's evidence. Cut to: (a) the test layout reorg, (b) the smallest harness that supports the assertions you keep, (c) leave product-code modules to their own siblings.

[christian-byrne]

Adversarial Review — PR #12104 (ext-api/i-tf)

Role: Skeptic
Reviewer: codex
Focus: Bugs, race conditions, hidden invariants, edge cases, error paths

---

Summary Table

Severity	File:Line	Issue
High	extension-api-service.ts:184-187	Widget name parsing breaks on names containing colons
High	extension-api-service.ts:273-278	Type mismatch: Point/Size fallbacks are objects, not tuples
High	extension-api-service.ts:327-335	dispatch() stub returns undefined, callers cast it to WidgetEntityId
Medium	extension-api-service.ts:29-30	pauseTracking/resetTracking are no-ops — accidental deps possible
Medium	extension-api-service.ts:599-609	Potential ordering issue if same nodeId in both added and removed sets
Medium	loadEvidenceSnippet.ts:78-79	Silent empty-string return vs throw inconsistency
Low	extension-api-service.ts:634-640	invokeV2AppExtensions swallows errors, caller never knows
Low	world.ts:90-91	nextId resets on clear() — stale entityId aliasing risk
Low	extension-api-service.ts:451	onNodeMounted relies on watch(() => null) behavior

---

Detailed Findings

1. Widget name parsing breaks on names containing colons

File: src/services/extension-api-service.ts:184-187
Severity: High

get name() {
  const raw = widgetId as unknown as string
  const lastColon = raw.lastIndexOf(':')
  return lastColon !== -1 ? raw.slice(lastColon + 1) : raw
}

If a widget is named "model:v2", the entityId would be "widget:graphId:nodeId:model:v2". The current parsing returns "v2" instead of "model:v2".

Suggested fix: Store widget name in a dedicated component, or use a delimiter that cannot appear in widget names.

---

2. Type mismatch: Point/Size fallbacks

File: src/services/extension-api-service.ts:273-278
Severity: High

getPosition(): Point {
  return world.getComponent(nodeId, PositionKey)?.pos ?? { x: 0, y: 0 }
}
getSize(): Size {
  return world.getComponent(nodeId, DimensionsKey)?.size ?? { width: 0, height: 0 }
}

Point is declared as [x: number, y: number] (tuple), but fallbacks are { x: 0, y: 0 } (object). Same for Size. This is a runtime type violation that tests don't catch because the world is mocked.

Suggested fix: Use [0, 0] as fallbacks, or change the type definitions to use { x, y } objects.

---

3. dispatch() stub returns undefined, callers expect values

File: src/services/extension-api-service.ts:327-335
Severity: High

addWidget(type, name, defaultValue, options) {
  const widgetId = dispatch({ type: 'CreateWidget', ... }) as WidgetEntityId
  return createWidgetHandle(widgetId)
}

When _dispatchImpl is null, dispatch() returns undefined. Casting undefined as WidgetEntityId creates a handle where handle.entityId is undefined, which will fail on any subsequent operation. Tests don't catch this because they mock dispatch or don't exercise addWidget.

Suggested fix: Either throw when dispatch has no impl and CreateWidget is called, or return a deterministic placeholder ID.

---

4. pauseTracking/resetTracking are no-ops

File: src/services/extension-api-service.ts:29-30
Severity: Medium

const pauseTracking = (): void => {}
const resetTracking = (): void => {}

The comment claims "Vue internals not publicly exported" but they ARE exported from @vue/reactivity. Without actual tracking pauses, any reactive reads during setup (e.g., world.getComponent()) can accidentally create dependencies, causing hooks to re-run unexpectedly.

Suggested fix: Import from @vue/reactivity or document that tracking control is intentionally disabled for Phase A.

---

5. Watch callback ordering assumption

File: src/services/extension-api-service.ts:599-609
Severity: Medium

for (const nodeId of currentIds) {
  if (!prev.has(nodeId)) mountExtensionsForNode(nodeId)
}
for (const nodeId of previousIds ?? []) {
  if (!curr.has(nodeId)) unmountExtensionsForNode(nodeId)
}

Mount always runs before unmount in the same flush. If the same nodeId somehow appears in both sets (e.g., rapid add-remove-add between flushes), the mount would run, then unmount would immediately destroy the scope. The second add wouldn't remount because the loop already processed it.

Suggested fix: Process removals before additions to ensure clean teardown.

---

6. Silent empty-string vs throw inconsistency

File: src/extension-api-v2/harness/loadEvidenceSnippet.ts:78-79
Severity: Medium

if (withExcerpts.length === 0) {
  return ''
}

Unknown patternId throws. Known patternId with no excerpts returns ''. A test using loadEvidenceSnippet('BC.99', 0) passes silently with empty input if BC.99 exists but lacks excerpts. This masks fixture data issues.

Suggested fix: Throw when pattern exists but has no excerpts, or return null/undefined to force explicit handling.

---

7. Error swallowing in invokeV2AppExtensions

File: src/services/extension-api-service.ts:634-640
Severity: Low

try {
  await fn()
} catch (err) {
  console.error(`[extension-api] Error in v2 extension "${ext.name}" ${hook}():`, err)
}

Errors are logged but not propagated. If a critical extension's init() throws, the app continues as if nothing happened. Callers cannot detect partial initialization failure.

Suggested fix: Consider collecting errors and throwing an aggregate at the end, or exposing a return value indicating failures.

---

8. nextId resets on clear() — stale reference aliasing

File: src/extension-api-v2/harness/world.ts:90-91
Severity: Low

clear() {
  nodes.clear()
  nextId = 1
}

If a test stores const id = world.addNode(...), then calls world.clear(), then world.addNode(...), the new entity gets the same ID as the old one. Any stale references now point to unrelated data.

Suggested fix: Never reset nextId, or track cleared ID ranges to prevent reuse.

---

9. onNodeMounted relies on watch(() => null) trick

File: src/services/extension-api-service.ts:451
Severity: Low

watch(() => null, fn, { flush: 'post', once: true })

This works because Vue flushes once-watches after the current sync code. But it's undocumented behavior that could change. If Vue optimizes away constant-source watches in a future version, onNodeMounted callbacks silently stop firing.

Suggested fix: Use queuePostFlushCb from Vue internals (it's exported), or schedule via nextTick.

---

Notes

• The stub architecture is sound for Phase A, but the type mismatches (Point/Size) will cause runtime failures when real ECS lands unless the component data shapes are aligned.
• Tests mock extensively, which is appropriate for Phase A, but the test/impl contract divergence on types should be tracked as tech debt.
• The 100+ BC test files are mostly it.todo or use local stubs — good for proving API shape, insufficient for behavioral confidence.

[christian-byrne]

architecture-reviewer findings (Phase A scope)

Reviewed against architecture-reviewer.md criteria 1–8. Diff is overwhelmingly test-stub fill against ext-api/i-foundation. No blockers — flagging structural concerns that will compound as Phase B lands.

Severity	File:Line	Issue	Suggested fix
recommended	src/extension-api-v2/__tests__/bc-*.v2.test.ts & bc-*.migration.test.ts (≥90 files)	Change amplification + harness bypass. src/extension-api-v2/harness/ was built as the shared substrate, yet ~96/126 test files don't import from it (from '../harness'). Each rolls its own near-identical stubs (createTestRuntime, createV2Runtime, createV1App, createHandle). When the v2 contract or NodeHandle shape shifts in Phase B, every duplicated stub must be touched. Criterion 8 (change amplification) + 6 (consistency).	Extract the per-file createV2Runtime / createHandle / createV1App shims into src/extension-api-v2/__tests__/__helpers__/ (or fold into harness/), then re-export. Keep one canonical NodeHandle stub factory.
recommended	src/extension-api-v2/harness/runV1.ts:34-51, harness/runV2.ts:45-65	Leaky / misleading API contract. Both accept snippet: string, then ignore it. runV1 returns registered: app.extensions.length — derived from registrations done out of band by the caller, not from the snippet. Callers will encode assumptions against a fake contract that the Phase B replacement won't satisfy without rework. Criterion 4 (API design).	Either (a) name them createV1Probe / createV2Probe and drop the snippet arg until evaluation actually exists, or (b) keep the name but throw NotImplemented on a non-empty snippet so tests can't accidentally rely on a no-op runner.
recommended	src/extension-api-v2/harness/comfyApp.ts:37-46	Leaky abstraction in the facade. MiniComfyApp exposes world: HarnessWorld as a public field. Real app has no such property; tests reaching through .world couple to the harness's internal substrate, not the production-shaped surface they claim to mirror. Criterion 4 + 5 (coupling).	Drop .world from MiniComfyApp. If tests need the World, return it as a sibling tuple from createMiniComfyApp() (e.g. { app, world }) so the leak is explicit.
recommended	src/extension-api-v2/harness/loadEvidenceSnippet.ts:9, 71	Dangling tooling reference + format mismatch. Comment line 9 cites scripts/sync-touch-point-db.mjs; that script does not exist in the tree. Error string at line 71 instructs readers to check touch-point-database.yaml while the imported fixture is .json. Criterion 6 (consistency).	Either land the sync script (even as a stub) or remove the reference. Update the error message to point at the actual .json fixture path.
nit	src/extension-api-v2/harness/index.ts:5-13	Public surface intentionally omits types (HarnessWorld, MiniComfyApp, RunV1Result). The migration/v2 tests already declare values of these shapes; deep-imports from harness/comfyApp / harness/world will proliferate. Criterion 4.	Re-export the types now; they're zero runtime cost and unblock harness adoption from the test files flagged above.
nit	src/extension-api-v2/harness/runV2.ts:21-24	Imports NodeExtensionOptions / WidgetExtensionOptions from @/types/extensionV2 while the rest of the suite (bc-*.v2.test.ts) imports NodeExtensionOptions from @/extension-api/lifecycle. Two type sources for the same concept — pick one. Criterion 6.	Standardize on the @/extension-api/* paths used by tests, or add an ADR note clarifying the split.

Out of scope for this review

• Snapshot freshness of __fixtures__/touch-point-database.json (2.5 MB) — not an architectural concern.
• The extensionV2Service.ts deletion is acknowledged in the PR body and the canonical service exists at src/services/extension-api-service.ts. No dependency-direction violation in this PR's diff.

Posted by amp / codex agent per .agents/checks/architecture-reviewer.md.

[christian-byrne]

ecosystem-compat review

Severity	File:Line	Issue	Suggested fix
nit	src/services/extensionV2Service.ts (deleted, 413 lines)	Removes internal exports defineNodeExtension, defineWidgetExtension, startExtensionSystem. None are reachable through the published @comfyorg/extension-api barrel (src/extension-api/index.ts re-exports from @/services/extension-api-service, not extensionV2Service), and the file has zero inbound refs in the parent branch (git grep -l 'extensionV2Service' 96addd0e → empty). PR description already flags the deletion for reviewer awareness.	None required — verifying the public barrel was untouched is sufficient.
medium (process)	repo-wide	mcp__comfy_codesearch__search_code is currently unavailable (gitserver DNS lookup failure), so external custom-node usage of @/services/extensionV2Service could not be programmatically verified. The path is internal (@/services/...), so custom nodes shouldn't reach it via the published package, but this could not be confirmed.	Per the check's "Unknown usage" guidance, add an explicit note in the PR body that external usage of the deleted internal service was not verifiable at review time (codesearch MCP down).

Scope

Non-test changes are limited to:

• src/extension-api-v2/harness/* (new internal test scaffolding)
• src/services/extensionV2Service.ts (deletion, called out in PR body)
• vitest.extension-api.config.mts (test config)

The public extension API surface (src/extension-api/index.ts, src/services/extension-api-service.*, src/types/extension*) is unchanged in this PR. No exported function signatures, return types, event names/payloads, CSS selectors, or extension hooks on the public barrel are altered. No ecosystem-blocking findings.

[christian-byrne]

sonarjs-lint review — PR #12104

Ran eslint-plugin-sonarjs (v4.0.3, recommended preset) against the new harness files. Test files (__tests__/bc-*.test.ts) are excluded by the strict config's ignores glob, leaving the 6 harness files in scope.

Severity	File:Line	Issue (rule)	Suggested fix
nit	src/extension-api-v2/harness/runV1.ts:7	sonarjs/todo-tag — TODO comment ("it.todo")	False positive — "todo" appears as part of the Vitest API name in JSDoc. Either reword to "test stubs (it.todo API)" so the linter doesn't trip, or accept and silence per-file.
nit	src/extension-api-v2/harness/runV1.ts:42,44	sonarjs/todo-tag — TODO(I-WS / Phase B): actually evaluate snippet…	Intentional per PR description ("stubs until Phase B"). Replace bare TODO with a tracked link (// FIXME(I-WS): see issue/PR …) or leave as-is and document the carve-out.
nit	src/extension-api-v2/harness/runV2.ts:5	sonarjs/todo-tag — TODO ("it.todo")	Same false positive as runV1.ts:7 — "todo" is part of the Vitest API name.
nit	src/extension-api-v2/harness/runV2.ts:55	sonarjs/todo-tag — TODO(I-WS / Phase B): actually evaluate snippet…	Same as runV1.ts:42 — intentional Phase B placeholder.
nit	src/extension-api-v2/harness/world.ts:21	sonarjs/redundant-type-aliases — export type EntityId = number	Sonar suggests inlining number. Recommend keeping the alias — it carries semantic meaning and is referenced by HarnessNodeRecord and HarnessWorld. Silence with a per-line // eslint-disable-next-line sonarjs/redundant-type-aliases if Sonar is wired into CI.

Notes on the check itself (not blockers for this PR)

• The colocated .agents/checks/eslint.strict.config.js cannot be loaded as-written against eslint-plugin-sonarjs@4.0.3: it references rules that no longer exist (no-one-iteration-loop, no-collection-size-mischeck, no-duplicated-branches, no-identical-functions, no-unused-collection, no-gratuitous-expressions, no-duplicate-string, no-small-switch, prefer-immediate-return, prefer-single-boolean-return, no-inverted-boolean-check, no-nested-template-literals). Per the check spec's fallback ("if the strict config … fails to load, skip"), the strict pass would skip; I instead ran with sonarjs.configs.recommended to surface real signal. Worth a follow-up to refresh the strict config against the current plugin's rule set.
• All findings are advisory; no blockers. The TODO-tag hits are aligned with the PR's stated stub-until-Phase-B scope, and the redundant-type-alias hit is semantically motivated.

Verdict

No blockers. Comment-only.

[christian-byrne]

Reviewed; no product changes required from this fix-cycle. The test framework / harness reorganisation already in this PR stands as-is. Companion harness stubs land in #12102 and #12105 (src/extension-api-v2/harness.ts).

[github-actions]

🌐 Website E2E

Tip

All tests passed.

Status	✅ Passed
Report	View Report

[christian-byrne]

Review rollup — triage of 6 prior reviews

Triaging the Adversarial-Architectural, Minimalist, Skeptic, Architecture-Reviewer, Ecosystem-Compat, and SonarJS reviews against this branch's recent fix cycle. This branch (ext-api/i-tf) carries no new product diff in the last fix pass — it inherits foundation fixes from sibling commits (58d6d2a review-feedback, e010c47 brand/World stub, 192c102 lockfile, c614243 setup-python pin + console eslint-disables, e7f6427 dead-service delete, bcc9b48 HEAD = current).

Status legend

✅ addressed · ❌ needs-fix · 🔒 wontfix (Phase A scope) · 🚫 out-of-scope (foundation/sibling PR)

Architectural-Adversarial (PRR_kwDOMIrOxs791aMK)

ID	Finding	Status	Evidence
F1a	evidenceIndex param shadows module Map (loadEvidenceSnippet.ts:65)	❌ needs-fix	still evidenceIndex = 0 at L65
F1b	Error string says .yaml, fixture is .json (L71)	❌ needs-fix	L71 unchanged
F2	Inline createTestRuntime / createV2Runtime duplicated across BC files	❌ needs-fix	harness exists, ~96/126 BC files still bypass
F3	vi.mock('@/world/…') block duplicated in 4 BC files	❌ needs-fix	no harness/worldMocks.ts
F4	Vacuous not.toThrow() against no-op runV1/V2	🔒 wontfix	Phase A scaffolding; mark intent in tests is a nit
F5	[Phase B] todos misclassified vs D9 (Phase B/C)	❌ needs-fix	minor, single-pass relabel
F6	MiniComfyApp.world exposed publicly	❌ needs-fix	comfyApp.ts:45 world: HarnessWorld still on interface

Minimalist (PRR_kwDOMIrOxs791ij7)

ID	Finding	Status	Evidence
1	runV1/V2 ignore snippet — tautological tests	🔒 wontfix	Phase A stub, TODO(I-WS/Phase B) in source
2	Mocks-testing-mocks anti-pattern (~405 todos / 802 its)	🚫 out-of-scope	tracked as Phase A scope decision
3	PR ships 3.5k LoC product code outside harness	🚫 out-of-scope	inherits from foundation PR; not added in i-tf cycle
4	_setDispatchImplForTesting + dispatch stub on public service	❌ needs-fix	extension-api-service.ts:96-114 unchanged
5	_clearExtensionsForTesting, getScopeRegistry test seams	❌ needs-fix	move to __tests__/internal.ts
6	Eager-build evidence map (drop lazy memo)	🔒 wontfix	functional; cosmetic
7	Harness over-engineered for ~30 live assertions	🔒 wontfix	designed for Phase B; deliberate
8	build-docs.ts (470 LoC) vs typedoc.json redundancy	🚫 out-of-scope	packages/extension-api owns; separate cleanup

Skeptic (PRR_kwDOMIrOxs791ock)

ID	Finding	Status	Evidence
1	Widget name parsing breaks on names with : (L184-187)	❌ needs-fix	lastIndexOf(':') at L191 unchanged
2	Point/Size fallback shape mismatch (object vs tuple)	✅ addressed	58d6d2a "Align getPosition()/getSize() defaults to tuple shape [0, 0]"
3	dispatch() returns undefined, cast to WidgetEntityId	❌ needs-fix	_dispatchImpl still nullable, no throw on CreateWidget
4	pauseTracking/resetTracking no-ops mislabeled	❌ needs-fix	comment claim re @vue/reactivity non-export is wrong
5	Mount-before-unmount ordering risk (L599-609)	❌ needs-fix	swap loop order at L641/645
6	loadEvidenceSnippet empty-string vs throw inconsistency	❌ needs-fix	L78-79 still silent return
7	invokeV2AppExtensions swallows errors	🔒 wontfix	DEV diagnostic intent; eslint-disable applied in c614243
8	nextId resets on clear() — stale-id aliasing	🔒 wontfix	harness-only; test contract
9	onNodeMounted relies on watch(() => null) trick	✅ addressed	58d6d2a "onNodeMounted: immediate: true so callback fires with no reactive deps"

Architecture-reviewer (PRR_kwDOMIrOxs796_aM)

ID	Finding	Status	Evidence
A1	Harness bypass — 96/126 BC tests inline stubs	❌ needs-fix	overlaps F2/F3
A2	runV1/V2 accept snippet arg they ignore	🔒 wontfix	rename or NotImplemented throw deferred — Phase A stub
A3	MiniComfyApp.world leak	❌ needs-fix	duplicate of F6
A4	Dangling sync-touch-point-db.mjs ref + .yaml error	❌ needs-fix	partial dup of F1b; sync script still missing
A5	harness/index.ts omits type re-exports	❌ needs-fix	nit; zero-runtime
A6	NodeExtensionOptions two-source-of-truth import paths	❌ needs-fix	standardize on @/extension-api/lifecycle

Ecosystem-compat (PRR_kwDOMIrOxs796_-X)

ID	Finding	Status	Evidence
E1	extensionV2Service.ts deletion	✅ addressed	e7f6427 deleted file; public barrel re-exports extension-api-service only
E2	codesearch MCP unavailable — external usage unverifiable	🚫 out-of-scope	process note; path is internal @/services/...

SonarJS (PRR_kwDOMIrOxs797A5g)

ID	Finding	Status
S1–S4	sonarjs/todo-tag hits in runV1.ts / runV2.ts	🔒 wontfix — intentional Phase B markers; false positives on "todo" in it.todo JSDoc
S5	redundant-type-aliases on EntityId = number	🔒 wontfix — semantic alias, kept

Foundation fixes already absorbed by this branch

• ✅ Brand unification NodeEntityId / WidgetEntityId (e010c47)
• ✅ World stub generics <TData, TEntity> + widget-component data shapes (e010c47)
• ✅ Pinned actions/setup-python + intentional-console eslint-disables (c614243)
• ✅ pnpm-lock.yaml regenerated for packages/extension-api workspace (192c102)
• ✅ extensionV2Service.ts removed (e7f6427)
• ✅ Foundation review feedback: tuple defaults, DEV warns, onNodeMounted immediate, idempotent startExtensionSystem, async-setup catch (58d6d2a)

Net needs-fix (16 items)

High-signal cluster (single follow-up PR):

1. Test-harness adoption: extract createV2TestRuntime + worldMocks.ts, drop public MiniComfyApp.world, re-export harness types (F2 / F3 / F6 / A1 / A3 / A5).
2. loadEvidenceSnippet: rename param evidenceIndex → rowIndex, fix .yaml→.json error string, throw on empty-excerpt pattern, add or remove sync-touch-point-db.mjs reference (F1a / F1b / Skeptic-6 / A4).
3. Service hygiene: gate _setDispatchImplForTesting / _clearExtensionsForTesting / getScopeRegistry behind a __tests__/internal.ts import; throw (or return placeholder) when dispatch() runs without impl on CreateWidget; fix widget-name colon parsing; swap mount/unmount loop order; correct pauseTracking comment (Min-4 / Min-5 / Skeptic-1 / Skeptic-3 / Skeptic-4 / Skeptic-5).
4. Cosmetics: relabel [Phase B] → [Phase B/C] per D9; standardize NodeExtensionOptions import path (F5 / A6).

No blockers for landing this PR; needs-fix items are deferrable to a follow-up ext-api/i-tf-polish branch. SonarJS, vacuous-no-throw, and over-engineered-harness flags are explicit Phase A carve-outs.

[christian-byrne]

Closing in favor of #12145 (restratified — properly-scoped tf only).

The original 4-PR stack (#12102/#12103/#12104/#12105) had absorbed downstream work into the foundation PR (test framework + npm package + core ext conversions all landed in #12102). The new stack (#12142/#12143/#12145/#12144) has clean per-branch boundaries.

See #12142 description for the new stack overview. Original branch tips backed up at backup/restratify/ext-api-i-tf tag on christian-byrne fork.