# PRD Review: SDK Size Reduction
**Reviewer:** Thing (adversarial PRD QA)
**Revision:** 2/5
**Date:** 2026-03-17
**PRD:** `planning/sdk-size-reduction/prd.md`
**PRD JSON:** `planning/sdk-size-reduction/prd.json`
**Spec:** `planning/sdk-size-reduction/spec.md`

---

## STEP 0 — Rev 1 Issues Checklist

Before scoring, each Rev 1 issue is assessed for genuine resolution.

| Rev 1 Issue | Status | Notes |
|---|---|---|
| 1.1 BLOCKER: Story 1 `git rm dist-native/` will fail | FIXED | Story 1 deleted entirely; PRD now documents dist-native/ as untracked since 2020 and requires no action |
| 1.2 MINOR: baseBranch `main` vs `master` ambiguity | FIXED | PRD now includes explicit note: "main is intentional — fester/sdk-size-reduction targets main, which then merges to master" |
| 1.3 MINOR: Story 3 `approach: general-purpose` should be `test-driven` | FIXED | Story 2 (formerly Story 3) now has `"approach": "test-driven"` in prd.json |
| 2.1 MODERATE: Story 3 mock approach under-specified | FIXED | Full isolation pattern pre-specified: new file `AvoInspectorDynamicImportFailure_test.ts`, no top-level import, `jest.resetModules()` + `jest.doMock()` + `require()` inside test body |
| 2.2 MINOR: Story 4 `estimatedFiles: 2` misleading | FIXED | Story 3 (formerly Story 4) has `"estimatedFiles": 0` with contingency note |
| 2.3 MINOR: Story 2 AC vague about comment content | FIXED | AC now requires specific terms: `_eventSpecReady`, `finally`, and race condition risk |
| 3.1 MODERATE: Dev/staging success-path spec AC not covered | FIXED | Success-path test added to Story 2 (formerly Story 3) as Test 2 |
| 3.2 MINOR: Story 3 "resolves" ambiguity | FIXED | AC now specifies "defined array with length > 0" |
| 3.3 MINOR: Story 4 quality checks included `yarn test` not in prepublishOnly | FIXED | Story 3 quality checks now list `yarn build`, `yarn prepublishOnly`, and `cd examples/... && yarn test --watchAll=false` |
| 4.1 MODERATE: `maxIterations: 8` too low | FIXED | `maxIterations` raised to 12 |
| 4.2 MINOR: No explicit parallelism marking | NOT FIXED | Empty `dependsOn` is sufficient; agreed this is low priority; no new issue raised |

**All blockers and moderates from Rev 1 are genuinely fixed. Minors resolved. One minor (4.2) was intentionally deferred and was accepted as sufficient by the prior analysis.**

---

## STEP 1 — prd.json Structure Validation

| Check | Result |
|-------|--------|
| JSON parseable | PASS |
| All story IDs unique | PASS — `story-1`, `story-2`, `story-3` |
| All `dependsOn` references exist | PASS — `story-3` depends on `[story-1, story-2]`, both present |
| No circular dependencies | PASS — DFS cycle check clean |
| Story ordering (dependents after dependencies) | PASS — stories 1 and 2 appear before story-3 |
| Every story has at least one required quality check | PASS — story-1: 3, story-2: 3, story-3: 3 |
| All `approach` values | PASS — story-1: `general-purpose`, story-2: `test-driven`, story-3: `general-purpose` |
| All `priority` values | PASS — `medium`, `high`, `high` |
| `maxIterations` present and reasonable | PASS — 12 |
| `baseBranch` + `baseBranchNote` present | PASS |

**Structure valid: true**

---

## STEP 2 — Stage Scores

### Stage 1: Architecture & Approach (23/25)

All architecture claims have been verified against the codebase:

**Verified CORRECT:**
- `.npmignore` deleted: confirmed, file does not exist at repo root
- `tsconfig.json` exclude array: `["src/__tests__", "src/__mocks__", "src/browser.js", "src/script.js"]` — confirmed
- `tsconfig.esm.json` present: confirmed; extends tsconfig.json; `module: ESNext`; `outDir: ./dist/esm`
- `package.json` `"files"` whitelist: confirmed correct, including `"!dist/__tests__"`, `"!dist/__mocks__"`, `"!dist/browser.js"`, `"!dist/script.js"`, `"!dist/*.LICENSE.txt"`, `bin/`, `bsconfig.json`, `rescript/`
- `package.json` `"sideEffects": false`: confirmed present
- `package.json` `"module"` field and `"exports"` conditional: confirmed
- `src/index.ts` `AvoStreamId` export: confirmed at line 3
- `src/AvoInspector.ts` `initEventSpecModules()` uses `Promise.all([import(...)])`: confirmed at lines 149–153
- `src/AvoSchemaParser.ts` lazy `_encryptValueFn` cache: confirmed at line 7 with `await import()` at line 37
- Line 141 comment: confirmed corrected to "(note: 'unknown' is truthy and will trigger initEventSpecModules; see streamId edge case in spec)"
- Prod-env test: confirmed at `AvoInspectorEventSpec_test.ts` lines 345–354
- `dist-native/` gitignored since 2020: consistent with Rev 1 verified facts
- `AvoInspectorDynamicImportFailure_test.ts`: NOT yet created (confirmed; this is the remaining work)
- Ordering constraint comment: NOT yet added (confirmed; this is the remaining work)

**Issue 1.1 — MINOR: `initEventSpecModules()` try block starts at line 148, not line 147**

The PRD says the comment should go "before line 148" and the method starts at line 147. Examining the code:
- Line 147: `private async initEventSpecModules(): Promise<void> {`
- Line 148: `  try {`

The PRD's Story 1 says "Add a multi-line comment directly above the `try` block inside `initEventSpecModules()` (before line 148)." This is accurate — the comment goes between line 147 (method signature) and line 148 (try keyword). The prd.json says "around line 147" which is slightly ambiguous (line 147 is the method signature; line 148 is the try). An agent following this literally could place the comment above line 147 (before the function signature) rather than inside the function before the try block.

**Recommended fix:** Tighten the location description. The prd.json AC says "above the try block (around line 147)" — since line 147 is the method signature and line 148 is `try {`, an agent could insert the comment at line 147 (before the function) instead of inside the function body. The MD description says "directly above the `try` block inside `initEventSpecModules()`" which is correct, but the prd.json AC should say "between the method signature and the try block" or "at line 148 (inside the method body, before try)."

This is a minor ambiguity but not a blocker — the description text is correct and should guide the agent.

---

### Stage 2: Story Quality (21/25)

**Issue 2.1 — MODERATE: Story 2 success-path test has an unresolvable contradiction between prd.json and prd.md**

In `prd.json` story-2 acceptance criterion 5:
> "A success-path test exists that awaits _eventSpecReady and asserts ensureEventSpecReady() returns a non-null object containing cache, fetcher, validate, and streamId fields"

But in `prd.md` Story 2, the success-path test code (lines 138–162) is followed by this note:
> "If `ensureEventSpecReady()` returns `null` due to the `streamId === "unknown"` edge case in tests, the test should assert on the individual loaded fields (`eventSpecCache`, `eventSpecFetcher`, `_validateEvent`) rather than on the return value of `ensureEventSpecReady()`."

And the actual test code in the prd.md asserts the individual fields (`eventSpecCache`, `eventSpecFetcher`, `_validateEvent`) but does NOT assert `ensureEventSpecReady()` returns non-null — it calls `ensureEventSpecReady()` but the result is never asserted.

This means:
- The `prd.json` AC says the test must assert `ensureEventSpecReady()` returns non-null with `{cache, fetcher, validate, streamId}`.
- The `prd.md` test pattern explicitly acknowledges `ensureEventSpecReady()` will return `null` in the test environment (due to `streamId === "unknown"`).
- The `prd.md` describes asserting individual fields as the correct approach, not asserting the return of `ensureEventSpecReady()`.

The two documents are contradictory on this point. An autonomous agent reading both will be confused: should it write a test that passes (`prd.md` pattern — assert individual fields) or one that is likely to fail (`prd.json` AC — assert non-null return from `ensureEventSpecReady()`)?

The spec acceptance criterion that must be satisfied is (spec.md line 152):
> "In a dev/staging environment with a valid `streamId`, `initEventSpecModules()` successfully loads all three eventSpec modules and `ensureEventSpecReady()` returns a non-null object with `cache`, `fetcher`, `validate`, and `streamId`."

In a Jest test environment, `AvoStreamId.streamId` returns `"unknown"` (storage not initialized), so `ensureEventSpecReady()` will return `null` due to the `!this.streamId` guard being false for `"unknown"` — wait, actually reading the code at line 187:
```typescript
if (!this.eventSpecCache || !this.eventSpecFetcher || !this.streamId || !this._validateEvent) return null;
```
`this.streamId` is set to `AvoStreamId.streamId` in the constructor (line 139), and `"unknown"` is truthy, so `!this.streamId` is false. The guard does NOT return null for `"unknown"`. However, `ensureEventSpecReady()` returns a non-null object only if `eventSpecCache`, `eventSpecFetcher`, `streamId`, and `_validateEvent` are all set. After successful module loading (Test 2), they ARE set. So `ensureEventSpecReady()` would return a non-null object with `streamId: "unknown"`.

This means the `prd.json` AC could pass if modules load successfully. But the `prd.md` note is misleading — it says "`ensureEventSpecReady()` returns null due to streamId === 'unknown'" which, reading the actual code, is incorrect. The function returns null when `!this.streamId` is true, but `"unknown"` is truthy so `!this.streamId` is false for `"unknown"`.

Net assessment: the `prd.md` note contains a factual error about `ensureEventSpecReady()` returning null for `streamId === "unknown"`. The spec and the code indicate it would NOT return null for `"unknown"` after successful module loading. The prd.json AC requiring non-null assertion may actually be achievable and is the more correct requirement.

However, the contradiction between `prd.md` (assert individual fields as fallback) and `prd.json` (assert non-null return) creates agent confusion. An agent following `prd.md` test code verbatim will write a weaker test. An agent following `prd.json` AC will write a stronger test. The prd.md code example should be updated to remove the incorrect fallback note or align with the prd.json AC.

**Issue 2.2 — MINOR: Story 1 (ordering comment) `prd.md` says "around line 147" but `prd.json` says "above the try block (around line 147)"**

See Issue 1.1 above for full discussion. The try block is at line 148; the method signature is at line 147. "Around line 147" in the prd.json could be interpreted as placing the comment above the method signature rather than inside the method body before try. The prd.md text is clearer: "directly above the `try` block inside `initEventSpecModules()`."

---

### Stage 3: Test & Acceptance (22/25)

**Issue 3.1 — MODERATE: prd.json story-2 AC#5 requires `ensureEventSpecReady()` to return non-null with `{cache, fetcher, validate, streamId}`, but prd.md note says to fall back to asserting individual fields**

This is documented in full under Issue 2.1. The key test-quality concern:

The `prd.md` test code for Test 2 calls `(inspector as any).ensureEventSpecReady()` but stores the result (line 153: `const result = await (inspector as any).ensureEventSpecReady()`) and then — based on the code in prd.md lines 156–158 — only asserts on the individual fields, not on `result`. The `prd.json` AC says to assert on the return value.

An autonomous agent has two paths:
1. Follow the test code in prd.md verbatim → `result` is computed but never asserted → spec AC partially satisfied
2. Follow the prd.json AC → assert `result !== null` AND assert `result.cache`, `result.fetcher`, `result.validate`, `result.streamId` are defined

**Recommended fix:** Remove the incorrect fallback note in prd.md (the note incorrectly predicts ensureEventSpecReady() returns null for "unknown" streamId — it does not). Align the test code with the prd.json AC: add `expect(result).not.toBeNull()` and field assertions on `result`.

**Issue 3.2 — MINOR: Story 2 failure-path test mocks use `() => { throw new Error(...) }` as the mock factory**

The mock pattern specified is:
```typescript
jest.doMock("../eventSpec/AvoEventSpecCache", () => { throw new Error("import failed"); });
```

In Jest, `jest.doMock()` takes a module factory function — the factory is called to produce the module export. The factory `() => { throw new Error("import failed"); }` throws during factory invocation, which simulates a module load failure. This is a valid pattern for CJS module mocking under Jest. However, there is an edge case: TypeScript compiles `import()` to `Promise.resolve().then(() => require(...))`. If Jest intercepts the `require()` call via `jest.doMock` and the factory throws, the `require()` throws synchronously inside the `.then()` callback, which causes the promise to reject. The `catch` block in `initEventSpecModules()` will catch this.

The pattern is sound for the CJS-compiled case. No issue here — this is just confirmation.

**Issue 3.3 — MINOR: Story 3 acceptance criteria do not specify what "fix any regressions" means in binary terms**

Story 3 AC#6 says: "No regressions in example project caused by ESM dual build exports fields." This is technically binary (the example project tests either pass or they don't), but the criterion is framed as a negative check ("no regressions") rather than a positive assertion ("all example project tests pass"). The criterion can be inferred from AC#4, but the wording of AC#6 alone is not independently testable as a binary criterion — it requires knowing what the baseline passing tests are.

This is a very minor issue; AC#4 ("yarn test --watchAll=false exits 0") fully covers it.

---

### Stage 4: Execution & Performance (24/25)

**Issue 4.1 — MINOR: Story 2 uses `await new Promise(resolve => setTimeout(resolve, 50))` as a timing fallback**

The failure-path test (Test 1) in prd.md lines 121–124:
```typescript
if ((inspector as any)._eventSpecReady) {
  await (inspector as any)._eventSpecReady.catch(() => {});
}
await new Promise(resolve => setTimeout(resolve, 50));
```

The `setTimeout(resolve, 50)` is a timing hack. If `_eventSpecReady` is not set (because the catch cleared it synchronously), the `setTimeout` is required. However, `_eventSpecReady` is set in the constructor before mocks are resolved — since `jest.doMock` + `require()` inside the test body ensures the module registry is fresh when `AvoInspector` is required, the inspector's constructor will set `_eventSpecReady = this.initEventSpecModules()`. The `initEventSpecModules()` call proceeds to `await Promise.all([import(...)])`. Under Jest with the mocked factory, `import()` compiles to `require()` which throws synchronously inside a Promise. The catch fires synchronously in the microtask queue. The `finally` clears `_eventSpecReady`.

The practical question: does `await (inspector as any)._eventSpecReady.catch(() => {})` reliably catch the error before the `setTimeout`? Since `_eventSpecReady` is a Promise that rejects (when the factory throws inside `Promise.resolve().then(() => require(...))`), awaiting it with `.catch` should be sufficient. The `setTimeout(50)` is belt-and-suspenders.

This is acceptable but creates a 50 ms delay in the test. Not a correctness issue, just a performance note. The prd.md description acknowledges this.

The overall execution story is sound. maxIterations 12 is appropriate.

---

## STEP 3 — Spec Cross-Reference

### All remaining spec acceptance criteria mapped to stories:

| Spec criterion | Covered by story | Status |
|---|---|---|
| `.npmignore` deleted | Completed (confirmed) | Done |
| `dist-native/` deleted from repo | N/A — gitignored since 2020, no action needed | Done (no story needed) |
| `tsconfig.json` exclude correct | Completed (confirmed) | Done |
| `package.json` files whitelist correct | Completed (confirmed) | Done |
| Clean build ≤ 38 kB | Completed (37.6 kB verified) | Done |
| `yarn test` / `yarn test:browser` pass | Completed (316/315 tests) | Done |
| `AvoStreamId` exported | Completed (src/index.ts line 3) | Done |
| `bsconfig.json` / `rescript/` in tarball | Completed | Done |
| Line 141 comment corrected | Completed (confirmed) | Done |
| Prod-env test written | Completed (lines 345–354) | Done |
| `[ ]` Example project tests pass | Story 3 | Covered |
| `[ ]` Dynamic import failure test | Story 2 (Test 1) | Covered |
| `[ ]` Ordering constraint comment | Story 1 | Covered |
| `[ ]` Dev/staging valid streamId test | Story 2 (Test 2) | Covered — but prd.md/prd.json contradiction (Issue 2.1/3.1) |

### No stories go beyond spec scope.

---

## STEP 4 — Issue Summary

| # | Issue | Severity | Story | Status vs Rev 1 |
|---|-------|----------|-------|----------------|
| 2.1 | prd.json AC#5 for story-2 contradicts prd.md note: prd.json says assert non-null return from ensureEventSpecReady(); prd.md says assert individual fields as fallback. prd.md note is factually incorrect (ensureEventSpecReady() does NOT return null for "unknown" streamId). | MODERATE | 2 | NEW |
| 1.1 | Story 1 prd.json AC says "above the try block (around line 147)" — line 147 is the method signature, line 148 is try. An agent could insert comment above the method rather than inside it. prd.md description is correct ("directly above the try block inside initEventSpecModules()"). | MINOR | 1 | NEW |
| 3.1 | Story 3 AC#6 "No regressions" is not independently binary; redundant with AC#4. | MINOR | 3 | NEW |
| 4.1 | Story 2 Test 1 uses setTimeout(50) timing fallback — acceptable but creates 50 ms test delay. | MINOR | 2 | NEW |

---

## STEP 5 — Score Calculation

| Stage | Score | Max | Key Deductions |
|-------|-------|-----|----------------|
| Stage 1: Architecture & Approach | 23 | 25 | Minor location ambiguity in story-1 prd.json AC for comment placement (-2) |
| Stage 2: Story Quality | 21 | 25 | prd.md/prd.json contradiction on success-path test assertion (-3); minor line number ambiguity (-1) |
| Stage 3: Test & Acceptance | 22 | 25 | Success-path test assertion contradiction means an agent following prd.md verbatim writes a weaker test that may not satisfy spec AC (-2); story-3 AC#6 not independently binary (-1) |
| Stage 4: Execution & Performance | 24 | 25 | setTimeout timing hack minor (-1) |
| **TOTAL** | **90** | **100** | |

**Verdict: PASS** (score 90 ≥ threshold 85)

---

## Required Fixes Before Resubmission

None required — score is above threshold. The following are recommended improvements for robustness:

### MODERATE (recommended):

**Fix 2.1 — Align prd.md success-path test with prd.json AC:**
Remove the incorrect note in prd.md that says `ensureEventSpecReady()` returns null for `streamId === "unknown"` — examination of the actual code shows it does NOT return null for `"unknown"` (line 187: `!this.streamId` is false for `"unknown"` because truthy). Update the test code in prd.md to add `expect(result).not.toBeNull()` and assert `result.cache`, `result.fetcher`, `result.validate`, `result.streamId` are all defined, aligning with the prd.json AC.

### MINOR (optional):

**Fix 1.1 — Tighten story-1 comment location in prd.json:**
Change "above the try block (around line 147)" to "inside the initEventSpecModules() method body, directly above the try block (between the method signature at line 147 and the try keyword at line 148)."

---

*Review by Thing, adversarial PRD QA reviewer.*