Add TypeScript code review skill and updated findings plans

AGENTS.md

@@ -160,6 +160,18 @@ Significant changes should have a plan file in `plans/` before implementation
 begins. Plans document the context, approach, and verification steps. Review
 existing plans in that directory for format examples.
 
+## AI Skills
+
+Reusable workflow instructions for AI agents live in `skills/`. Each skill is a
+self-contained folder with a `SKILL.md` and optional supporting assets
+(templates, scripts, references).
+
+| Skill | Purpose |
+|-------|---------|
+| `typescript-code-review` | Perform comprehensive TypeScript code reviews covering type safety, security, performance, and code quality with actionable feedback |
+
+Skills are loaded on-demand when the agent detects a matching task.
+
 ## Agent-First Documentation
 
 AI coding agents are first-class citizens of TKO. The docs site serves both

plans/typescript-code-review-findings-2.md

@@ -0,0 +1,100 @@
+# Plan: TypeScript Code Review — Findings (Round 2, Deduplicated)
+
+**Risk class:** `MEDIUM`
+
+**Status:** Active Backlog
+
+## Status Update (2026-04-21)
+
+- Updated to current toolchain (Bun + Biome).
+- Removed completed/obsolete items and cross-round duplicates.
+- This file now keeps only Round-2-specific backlog items not promoted as canonical findings in rounds 3–4.
+
+## Stack Baseline (Current)
+
+- Type-check: `bun run tsc`
+- Lint/format: `bun run check`
+- Tests: `bun run test`
+- Unused analysis: `bun run knip`
+
+## Summary
+
+Round 2 currently tracks medium/low-risk cleanup and consistency work that is still relevant,
+but not duplicated in newer findings plans.
+
+## Remaining Round-2 Findings
+
+### Important Improvements 🟡
+
+1. `subscribable.when()` may keep subscription alive forever when condition never becomes true.
+File: `packages/observable/src/subscribable.ts`
+
+2. Duplicate `bindingKey` in apply-bindings error message.
+File: `packages/bind/src/applyBindings.ts`
+
+3. `value.isInput()` checks `this.$element` instead of the parameter.
+File: `packages/binding.core/src/value.ts`
+
+4. Loose equality debt (`==`/`!=`) still needs intentional triage and selective replacement.
+Scope: multiple packages
+
+5. Deprecated `clonePlainObjectDeep` remains exported.
+File: `packages/utils/src/object.ts`
+
+6. `AttributeMustacheProvider.getBindingAccessors` returns `false` for non-element nodes (type-contract mismatch).
+File: `packages/provider.mustache/src/AttributeMustacheProvider.ts`
+Cross-ref: Issue [#235](https://github.com/knockout/tko/issues/235) — re-check against current code before committing to a fix route (per PR #297 review).
+
+7. `Parser` imported as value and downcast via `as any` in component provider path.
+File: `packages/provider.component/src/ComponentProvider.ts`
+PR: [#362](https://github.com/knockout/tko/pull/362) (open)
+
+8. JsxObserver subscription callback ignores callback value.
+File: `packages/utils.jsx/src/JsxObserver.ts`
+
+9. `NativeProvider` redundant null fallback.
+File: `packages/provider.native/src/NativeProvider.ts`
+
+### Suggestions 🔵
+
+10. Unused `SubscriptionCallback` import.
+File: `packages/bind/src/bindingEvent.ts`
+
+11. Legacy `event.returnValue` fallback.
+File: `packages/binding.core/src/submit.ts`
+
+12. `readElseChain()` returns `false` where object shape is expected.
+File: `packages/binding.if/src/else.ts`
+
+13. Redundant `nodeType` check after `instanceof Element`.
+File: `packages/provider.component/src/ComponentProvider.ts`
+
+14. AMD `require` call has no explicit error callback path.
+File: `packages/utils.component/src/loaders.ts`
+PR: [#361](https://github.com/knockout/tko/pull/361) (open)
+
+## De-duplication Rules
+
+- Findings tracked in rounds 3 or 4 must not be re-added here.
+- Fixed findings stay in git history, not in the active list.
+- Obsolete paths (for example removed files) are excluded.
+
+## PR #297 Review — Verification Status
+
+Brian's review flagged five original Round-2 critical findings for independent spot-checking.
+Disposition of each (findings may have been moved to later rounds or resolved via separate PRs):
+
+| Finding | Status | Reference |
+|---------|--------|-----------|
+| Proxy `deleteProperty` trap dropping property key | **Fixed** | PR [#336](https://github.com/knockout/tko/pull/336) (merged) |
+| `??` behaves identically to `||` (earlyOut) | **Fixed** | PR [#345](https://github.com/knockout/tko/pull/345) (merged) ; canonical in Round 3 |
+| Parser operator-precedence inversion | **Fixed** | Issue [#342](https://github.com/knockout/tko/issues/342) (closed); PR [#343](https://github.com/knockout/tko/pull/343) (merged) |
+| `TextInputLegacyFirefox` dead code | **Fixed** | PR [#346](https://github.com/knockout/tko/pull/346) (merged) |
+| `style` binding referencing global `jQuery` | Uses `options.jQuery` (gated) | PR [#339](https://github.com/knockout/tko/pull/339) (merged) |
+
+## Verification
+
+- `bun run tsc`
+- `bun run check`
+- `bun run test`
+- `bun run knip`

plans/typescript-code-review-findings-3.md

@@ -0,0 +1,59 @@
+# Plan: TypeScript Code Review — Findings (Round 3, Deduplicated)
+
+**Risk class:** `MEDIUM`
+
+**Status:** Active Backlog
+
+## Status Update (2026-04-21)
+
+- Updated to current software stack terminology.
+- Kept only canonical Round-3 findings.
+- Removed overlap duplicates with rounds 1, 2, and 4.
+
+## Stack Baseline (Current)
+
+- Type-check: `bun run tsc`
+- Lint/format: `bun run check`
+- Tests: `bun run test`
+
+## Summary
+
+Round 3 focuses on parser-semantics parity and runtime portability concerns.
+It tracks one critical and two important findings that remain canonical for this round.
+
+## Detailed Findings
+
+### Critical Issues 🔴
+
+1. `LifeCycle.__addEventListener` removes listeners with mismatched signature.
+File: `packages/lifecycle/src/LifeCycle.ts`
+
+- Register uses `addEventListener(..., options)` but dispose used `removeEventListener(... )` without options.
+- Capture listeners can remain attached after disposal.
+- PR: [#363](https://github.com/knockout/tko/pull/363) (open)
+
+### Important Improvements 🟡
+
+2. Parser accepts invalid JavaScript mixing of `??` with `||` / `&&` without required parentheses.
+File: `packages/utils.parser/src/operators.ts`
+
+- Native JS requires explicit grouping for these mixes.
+- Current parser accepts and evaluates such expressions.
+- **Independently verified**: `operators['??'].earlyOut = a => a` is identical to `||`'s earlyOut, making `??` short-circuit on any falsy value instead of only `null`/`undefined`. Confirmed bug.
+- PR [#345](https://github.com/knockout/tko/pull/345) (merged) — fixes the earlyOut.
+- PR [#343](https://github.com/knockout/tko/pull/343) (merged) — fixes operator precedence.
+- Issue [#342](https://github.com/knockout/tko/issues/342) (parser operator-precedence, closed).
+
+3. Runtime paths use ambient `document/window` instead of configured options.
+Files:
+- `packages/utils.component/src/loaders.ts`
+- `packages/binding.foreach/src/foreach.ts`
+
+- Should consistently use `options.document` / `options.global` for portability.
+
+## Verification
+
+- `bun run tsc`
+- `bun run check`
+- `bun run test`
+- Targeted parser and lifecycle regression tests for the above paths

plans/typescript-code-review-findings-4.md

@@ -0,0 +1,215 @@
+# Plan: TypeScript Code Review — Findings (Round 4)
+
+**Risk class:** `MEDIUM`
+
+**Status:** Active Backlog
+
+## Status Update (2026-04-21)
+
+- Updated to current toolchain language (Bun + Biome).
+- Deduplicated against rounds 1–3.
+- This file keeps only Round-4-canonical findings.
+
+## Stack Baseline (Current)
+
+- Type-check: `bun run tsc`
+- Lint/format: `bun run check`
+- Tests: `bun run test`
+- Unused analysis: `bun run knip`
+
+## Summary
+
+Round 4 tracks **2 critical bugs**, **8 important improvements**, and **9 suggestions**
+that are not duplicated as canonical findings in rounds 1–3.
+
+## Prior Rounds
+
+For historical context and previously cataloged findings, see:
+
+1. `plans/typescript-code-review-findings.md`
+2. `plans/typescript-code-review-findings-2.md`
+3. `plans/typescript-code-review-findings-3.md`
+
+---
+
+## Detailed Findings
+
+### Critical Issues 🔴
+
+#### 1. `notifyNextChange` closure variable never reset — sticky spurious notifications
+**File**: `packages/observable/src/observable.ts:212`
+- **Issue**: In `subscribable.fn.limit`, the chained assignment
+  `self._notifyNextChange = didUpdate = ignoreBeforeChange = false` writes to
+  the instance property `self._notifyNextChange` — which is never read anywhere
+  in the codebase — instead of resetting the closure variable `notifyNextChange`.
+  Once `_notifyNextChangeIfValueIsDifferent()` sets `notifyNextChange = true`
+  (line 238), it is **never** reset to `false`. Every subsequent call to
+  `finish()` will see `shouldNotify === true` regardless of value equality,
+  causing spurious notifications for any rate-limited or deferred observable.
+- **Verified**: `notifyNextChange` is a closure-scoped `let` (line 194).
+  `self._notifyNextChange` is never read. Confirmed bug.
+- **Current**:
+  ```ts
+  self._notifyNextChange = didUpdate = ignoreBeforeChange = false
+  ```
+- **Recommended**:
+  ```ts
+  notifyNextChange = didUpdate = ignoreBeforeChange = false
+  ```
+
+#### 2. Missing `getOwnPropertyDescriptor` Proxy trap breaks `Object.keys()`, `JSON.stringify()`
+**File**: `packages/computed/src/proxy.ts:47–84`
+- **Issue**: The Proxy defines an `ownKeys` trap returning keys from the source
+  `object`, but no `getOwnPropertyDescriptor` trap. The proxy target is an empty
+  `function () {}`. When `Object.keys(proxy)` is called, the engine calls
+  `ownKeys` (returns real keys), then calls `getOwnPropertyDescriptor` for each —
+  which falls through to the empty function target, returning `undefined`. All
+  keys are filtered out.
+- **Impact**: `Object.keys(proxy)` returns `[]`, `JSON.stringify(proxy)` returns
+  `undefined`, and `{...proxy}` produces `{}`.
+- **Verified**: Confirmed — no `getOwnPropertyDescriptor` trap exists in the
+  handler. The related `deleteProperty` trap bug in the same file was independently
+  confirmed and fixed in PR [#336](https://github.com/knockout/tko/pull/336),
+  validating the review's accuracy for this proxy handler.
+- **Recommended**:
+  ```ts
+  getOwnPropertyDescriptor(_target, prop) {
+    return Object.getOwnPropertyDescriptor(object, prop)
+      ?? Reflect.getOwnPropertyDescriptor(_target, prop)
+  }
+  ```
+
+---
+
+### Important Improvements 🟡
+
+#### 3. Base `Provider.preprocessNode` returns `[node]`, short-circuiting MultiProvider
+**File**: `packages/provider/src/Provider.ts:53–55`
+- **Issue**: The base `Provider.preprocessNode` returns `[node]` (truthy) instead
+  of `null`. In `MultiProvider.preprocessNode`, the first provider returning
+  truthy wins — subsequent providers are never consulted. Any provider inheriting
+  the default claims every node, preventing later providers from preprocessing.
+- **Current**: `return [node]`
+- **Recommended**: `return null`
+
+#### 4. `Text.textNodeReplacement` ignores `textNode` parameter, uses global `document`
+**File**: `packages/provider.mustache/src/mustacheParser.ts:67–70`
+- **Issue**: `Text.textNodeReplacement()` declares zero parameters, but callers
+  pass `textNode`. The argument is silently ignored. Uses global
+  `document.createTextNode(...)` while sibling `Expression.textNodeReplacement`
+  correctly uses `textNode.ownerDocument`. Causes cross-document DOM adoption
+  errors.
+- **Recommended**: Accept `textNode` parameter, use `textNode.ownerDocument`.
+
+#### 5. `VirtualProvider.preprocessNode` removes node unconditionally when `parent` is null
+**File**: `packages/provider.virtual/src/VirtualProvider.ts:21–31`
+- **Issue**: Replacement insertions use `parent?.insertBefore(...)` (no-op when
+  null), but `node.remove()` always executes. Content silently lost.
+- **Recommended**: Early-return `null` when `!parent`.
+
+#### 6. `slotBinding.ts` `getSlot()` returns `Node[]` typed as `Node`
+**File**: `packages/binding.component/src/slotBinding.ts:76–86`
+- **Issue**: `getSlot` declares return type `Node`, but the default-slot fallback
+  returns a filtered `Node[]`.
+- **Recommended**: Correct return type to `Node | Node[]` or normalize.
+
+#### 7. `foreach.ts` `removeNodes` — null `parentNode` dereference
+**File**: `packages/binding.foreach/src/foreach.ts:~413`
+- **Issue**: `removeFn` reads `nodes[0].parentNode` into `parent`, then calls
+  `parent.removeChild()`. If nodes are already detached, throws `TypeError`.
+- **Recommended**: Use `parent?.removeChild(nodes[i])`.
+
+#### 8. `foreach.ts` `makeTemplateNode` bypasses HTML sanitization
+**File**: `packages/binding.foreach/src/foreach.ts:~60`
+- **Issue**: For `<script>` template elements, assigns `sourceNode.text` directly
+  to `parentNode.innerHTML`, bypassing `options.sanitizeHtmlTemplate`,
+  `options.templateSizeLimit`, and `options.allowScriptTagsInTemplates`.
+- **Recommended**: Route through `parseHtmlFragment`.
+- PR: [#352](https://github.com/knockout/tko/pull/352) (open)
+
+#### 9. `LifeCycle.mixInto` copies `constructor`, overwriting target class identity
+**File**: `packages/lifecycle/src/LifeCycle.ts:18–22`
+- **Issue**: `Object.getOwnPropertyNames(mixin)` includes `'constructor'`. Breaks
+  `instance.constructor === TargetClass` and `instanceof` transitivity.
+- **Recommended**: `if (prop === 'constructor') continue`
+
+#### 10. Shell command injection surface in build script
+**File**: `tools/build.ts:14–18`
+- **Issue**: `esbuild()` passes interpolated string to `sh -c`. Shell
+  metacharacters in `package.json` fields are interpreted.
+- **Recommended**: Replace `sh -c` with array-based `Bun.spawn`.
+
+---
+
+### Suggestions 🔵
+
+#### 11. `hasSubscriptionsForEvent` returns `number | undefined`, annotated `boolean`
+**File**: `packages/observable/src/subscribable.ts:155–157`
+- **Recommended**: `return (this._subscriptions[event]?.length ?? 0) > 0`
+
+#### 12. `LifeCycle.addEventListener` crashes when no anchor node is set
+**File**: `packages/lifecycle/src/LifeCycle.ts:60–66`
+- **Recommended**: Throw a clear error message early.
+
+#### 13. `BindingResult.completionPromise` may be `undefined` despite non-optional type
+**File**: `packages/bind/src/BindingResult.ts:5`
+- **Recommended**: Mark as optional or assign `Promise.resolve(this)` for sync.
+
+#### 14. Inconsistent `getBindingAccessors` return types across providers
+**Files**: `provider/src/Provider.ts`, `provider.bindingstring/src/BindingStringProvider.ts`,
+`provider.native/src/NativeProvider.ts`
+- **Recommended**: Standardize to `BindingAccessors | null`.
+
+#### 15. `editScriptItem` typed as `number[]` but is a keyed object
+**File**: `packages/bind/src/arrayToDomNodeChildren.ts:143–148`
+- **Recommended**: Define proper `EditScriptItem` interface.
+
+#### 16. `memoization.ts` variable shadowing of module-level `memos`
+**File**: `packages/utils/src/memoization.ts:6,56`
+- **Recommended**: Rename local to `foundMemos`.
+
+#### 17. ESM import-fix regex over-matches non-JS extensions
+**File**: `tools/build.ts:58`
+- **Recommended**: Use `(?<!\.\w+)` instead of `(?<!\.js)`.
+
+#### 18. Self-referential package import in `extenders.ts`
+**File**: `packages/observable/src/extenders.ts:8`
+- **Recommended**: `import type { ObservableArray } from './observableArray'`
+
+#### 19. Duplicate import from `@tko/utils.jsx` in reference build
+**File**: `builds/reference/src/index.ts:19–20`
+- **Recommended**: Merge into single import statement.
+
+---
+
+## PR #297 Review — Additional Notes
+
+- Brian recommended treating findings plans as separate follow-ups rather than bundling with the skill PR.
+- Each critical finding should be independently verified before locking it in as a plan item.
+- Findings #1 and #2 are confirmed as real bugs (see verification notes above).
+- Open PRs addressing findings from across all rounds: #352 (makeTemplateNode sanitization), #361 (AMD require error callback), #362 (Parser instantiation in ComponentProvider), #363 (LifeCycle removeEventListener options).
+- Merged PRs: #343 (operator precedence), #345 (`??` earlyOut), #346 (TextInputLegacyFirefox), #347 (duplicate options import).
+
+---
+
+## Steps
+
+### Phase 1: Critical Bug Fixes (HIGH priority)
+
+1. Fix `notifyNextChange` closure reset and add focused tests.
+2. Add `getOwnPropertyDescriptor` proxy trap and add focused tests for key enumeration/serialization.
+
+### Phase 2: Important Fixes (MEDIUM priority)
+
+3. Fix findings #3–#10 with package-scoped tests and one full-suite pass.
+
+### Phase 3: Suggestions (LOW priority)
+
+4. Triage findings #11–#19 and land low-risk cleanup separately.
+
+## Verification
+
+- `bun run tsc`
+- `bun run check`
+- `bun run test`
+- `bun run knip`