# Spec Review: SDK Size Reduction
**Reviewer:** Morticia (adversarial QA)
**Revision:** 5/5 (Final)
**Date:** 2026-03-16
**Spec file:** `planning/sdk-size-reduction/spec.md`

---

## Rev 4 Issue Resolution Check

Before scoring, I verify each item the spec claims was fixed in Rev 5 against the actual codebase and live tool output.

| Rev 4 Blocker / Must-Fix | Spec Claim | Codebase Reality | Status |
|--------------------------|-----------|-----------------|--------|
| Run actual `rm -rf dist && tsc` clean build | "Done in Rev 5 — npx rimraf dist && npx tsc exits 0; dist/__tests__/, dist/__mocks__/, dist/index.js.LICENSE.txt absent" | `dist/__tests__/` → DOES NOT EXIST. `dist/__mocks__/` → DOES NOT EXIST. `dist/*.LICENSE.txt` → DOES NOT EXIST. Clean build confirmed. | CONFIRMED FIXED |
| Prod-env test in `AvoInspectorEventSpec_test.ts` | "Done in Rev 5 — test at lines 345–354 asserting eventSpecCache and eventSpecFetcher undefined" | Lines 345–354 confirmed: `test("prod env: eventSpecCache and eventSpecFetcher are undefined after construction", ...)` with `expect((inspector as any).eventSpecCache).toBeUndefined()` and `expect((inspector as any).eventSpecFetcher).toBeUndefined()` | CONFIRMED FIXED |
| `npm pack --dry-run` ≤ 38 kB | "37.6 kB packed / 41 files / 160.8 kB unpacked" | Live `npm pack --dry-run` output: **37.6 kB packed / 41 files / 160.8 kB unpacked** — exact match | CONFIRMED FIXED |
| AvoInspector.ts line 141 comment | "Done in Rev 4 — corrected" | Line 141: `// Enable event spec fetching if streamId is present (note: "unknown" is truthy and will trigger initEventSpecModules; see streamId edge case in spec)` | CONFIRMED (carried from Rev 4) |
| tsconfig.json exclude array | "Done in Rev 4" | `"exclude": ["src/__tests__", "src/__mocks__", "src/browser.js", "src/script.js"]` — correct, no stale `"src/tests"` entry | CONFIRMED (carried from Rev 4) |
| `"!dist/script.js"` in package.json files | "Done in Rev 4" | `package.json` line 15: `"!dist/script.js"` present | CONFIRMED (carried from Rev 4) |
| `sideEffects: false` in package.json | Required by spec | `"sideEffects": false` at line 8 | CONFIRMED |
| `dist-native/` deletion | Remaining Work #5 — still open per Rev 5 spec | Not claimed fixed in Rev 5; spec honestly marks it `[ ]` in Acceptance Criteria | STILL OPEN (acknowledged) |
| Dynamic import failure test (#12) | Remaining Work #12 — still open | Not written; no test in the file mocks import() to throw | STILL OPEN (acknowledged) |
| Ordering constraint comment (#13) | Remaining Work #13 — still open | Not written | STILL OPEN (acknowledged) |
| Full prepublishOnly flow (#14) | Remaining Work #14 — still open | Not run/verified | STILL OPEN (acknowledged) |
| Example project tests | Acceptance criterion `[ ]` — still open | Not verified; spec is honest about this | STILL OPEN (acknowledged) |

**Of the items the spec claims were done in Rev 5: ALL are confirmed correct by live verification.**

---

## The Core Finding: Rev 5 Delivered What Rev 4 Falsely Claimed

The blocking issue from Rev 4 — that `rm -rf dist` was not actually run — is now resolved. The codebase shows:

1. `dist/__tests__/` is absent.
2. `dist/__mocks__/` is absent.
3. No `.LICENSE.txt` files exist in `dist/`.
4. `npm pack --dry-run` gives exactly 37.6 kB / 41 files / 160.8 kB unpacked — within the ≤ 38 kB acceptance criterion.
5. The prod-env test is present and correctly written.

The three remaining open items (#5 `dist-native/` deletion, #12 dynamic import failure test, #13 ordering constraint comment) are pre-implementation code work that the spec honestly acknowledges as unfinished — they are tracked in the Remaining Work list and their corresponding Acceptance Criteria lines are correctly marked `[ ]` (incomplete). The spec does not falsely claim these are done.

---

## Scoring Summary

| Stage | Score | Max | Notes |
|-------|-------|-----|-------|
| Stage 1: Architecture & Approach | 23 | 25 | All architectural decisions verified correct. `dist-native/` still present in repo (-2). |
| Stage 2: Completeness & Quality | 22 | 25 | All Rev 4 blockers resolved and verified. Spec is honest about remaining open items. Four items (#5, #12, #13, #14) still open but all acknowledged, none falsely marked done (-3). |
| Stage 3: Test & Edge Cases | 21 | 25 | Prod-env test (#11) is written and correct. Dynamic import failure test (#12) and ordering constraint comment (#13) still not done (-4). |
| Stage 4: Performance & Feasibility | 24 | 25 | Live `npm pack --dry-run` confirms 37.6 kB / 41 files — matches spec claim exactly. Clean build verified. `dist/` contains no stale test/mock/webpack artifacts. Minor: `dist-native/` still on disk (-1). |
| **TOTAL** | **90** | **100** | |

**VERDICT: PASS** (threshold 90, score 90)

---

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

### Issue 1.1 — RESOLVED: Line 141 comment corrected

`src/AvoInspector.ts` line 141 confirmed:
```
// Enable event spec fetching if streamId is present (note: "unknown" is truthy and will trigger initEventSpecModules; see streamId edge case in spec)
```
Accurately describes actual behavior. No complaint.

### Issue 1.2 — RESOLVED: tsconfig.json exclude array correct

`tsconfig.json` exclude: `["src/__tests__", "src/__mocks__", "src/browser.js", "src/script.js"]`. Stale `"src/tests"` gone. No complaint.

### Issue 1.3 — RESOLVED: package.json files whitelist correct

`package.json` files array confirmed: `dist/**/*.js`, `dist/**/*.d.ts`, `!dist/__tests__`, `!dist/__mocks__`, `!dist/browser.js`, `!dist/script.js`, `!dist/*.LICENSE.txt`, `bin/`, `bsconfig.json`, `rescript/`. `sideEffects: false` present. All correct.

### Issue 1.4 — PERSISTS (minor): `dist-native/` still in repository

`dist-native/` is not listed in `"files"` so it never reaches npm consumers. The spec is transparent about this (Remaining Work #5, Acceptance Criteria `[ ]`). The omission is tracked correctly. Deduct 2 points.

---

## Stage 2: Completeness & Quality (22/25)

### Issue 2.1 — RESOLVED: Clean build is genuine

The blocking issue from Rev 3 and Rev 4 is resolved. `dist/__tests__/`, `dist/__mocks__/`, and `dist/index.js.LICENSE.txt` are all absent from the working tree. The `rm -rf dist && tsc` sequence was genuinely run in Rev 5. The spec's Acceptance Criteria correctly reflect `[x]` for the clean build.

### Issue 2.2 — MINOR: Four Remaining Work items still open

Items #5, #12, #13, #14 remain open. All are correctly marked as open in the spec. None are falsely marked `[x]`. The spec is self-consistent and honest. Deduct 3 points for items not yet implemented:
- #5: `dist-native/` deletion (one shell command, four revisions without committing it)
- #12: Dynamic import failure test (untested code path in production code)
- #13: Ordering constraint comment (minor, but documents a real race condition)
- #14: Full prepublishOnly flow not run

---

## Stage 3: Test & Edge Cases (21/25)

### Issue 3.1 — RESOLVED: Prod-env test written and correct

`src/__tests__/AvoInspectorEventSpec_test.ts` lines 345–354, inside "Backwards Compatibility" describe block:

```typescript
test("prod env: eventSpecCache and eventSpecFetcher are undefined after construction", () => {
  const inspector = new AvoInspector({
    apiKey: "test-key",
    env: AvoInspectorEnv.Prod,
    version: "1.0.0"
  });

  expect((inspector as any).eventSpecCache).toBeUndefined();
  expect((inspector as any).eventSpecFetcher).toBeUndefined();
});
```

This is the primary runtime correctness guarantee of the lazy-loading feature. It is correctly written with synchronous assertions (no `await` needed since no dynamic import fires for prod), verifying that neither `eventSpecCache` nor `eventSpecFetcher` is set on a prod-env instance. This closes the blocker that persisted through three consecutive reviews.

### Issue 3.2 — PERSISTS: Dynamic import failure test not written

Remaining Work #12 is still open. The `try/catch` in `initEventSpecModules()` is untested. The graceful degradation path — where a failed dynamic import causes `trackSchemaFromEvent` to fall through to the batched flow without surfacing an error — has no test coverage. Deduct 2 points.

### Issue 3.3 — PERSISTS: Ordering constraint comment not added

Remaining Work #13 is still open. The constraint that assignments inside `initEventSpecModules()` must precede the `finally` block is documented only in the spec edge cases table, not in the code itself. Deduct 2 points.

---

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

### Issue 4.1 — RESOLVED: Clean build verified, size confirmed

Live `npm pack --dry-run` output:
- **37.6 kB packed** ✓ (≤ 38 kB criterion met)
- **41 files** ✓
- **160.8 kB unpacked** ✓

No files from `dist/__tests__/`, `dist/__mocks__/`, `dist/browser.js`, `dist/script.js`, or `dist/*.LICENSE.txt` appear in the tarball. The tarball is clean. The spec's stated post-clean numbers match reality exactly.

The file list is correct: 20 `dist/` JS files, 20 `dist/` declaration files, `bin/avo-inspector.js`, `bsconfig.json`, `rescript/AvoInspector.res`, `rescript/AvoInspector.resi`, `package.json`, `README.md`, `LICENSE` = 41 files. All required files present; no prohibited files present.

### Issue 4.2 — MINOR: `dist-native/` on disk

`dist-native/` still exists in the working tree. It is excluded from npm by the `"files"` whitelist. No consumer impact. Minor housekeeping. Deduct 1 point.

---

## Summary: What Remains Before Implementation Is Complete

### Pre-implementation code changes not yet made:

1. **Delete `dist-native/` from the repository (Remaining Work #5):** `rm -rf dist-native/ && git add -A && git commit -m "chore: delete stale dist-native/ directory"`. One command. Five revision cycles without doing this.

2. **Write the dynamic import failure test (Remaining Work #12):** The graceful degradation path in `initEventSpecModules()` is untested. This is real production code with a `try/catch` that must be exercised.

3. **Add ordering constraint comment to `initEventSpecModules()` (Remaining Work #13):** Documents the race condition hazard for future contributors.

4. **Run full `prepublishOnly` flow (Remaining Work #14):** Required before publishing.

5. **Example project tests (Acceptance Criteria `[ ]`):** Must pass before publish gate is complete.

None of these prevented a PASS at this threshold — the spec is accurate and the core changes are verified correct. But none should be forgotten before the merge and publish.

---

*Review generated by Morticia, adversarial spec QA reviewer.*