Repository navigation
refactor(core): wire-layer/public-layer full separation — frozen per-era schemas, function-only WireCodec, bidirectional lint rule - #2351
Code review found 1 important issue
Found 5 candidates, confirmed 3. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 1 |
| 🟡 Nit | 2 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🔴 Important | packages/client/src/client/probeClassifier.ts:137-146 |
probeClassifier: codec routing is not behavior-neutral for present-but-invalid cache hints |
| 🟡 Nit | packages/core/test/wire/layeringInvariants.test.ts:50-53 |
layeringInvariants: invariant (b) regex misses runtime 'export … from types/schemas.js' |
| 🟡 Nit | packages/core/src/wire/rev2025-11-25/codec.ts:106-119 |
Pure-refactor claim contradicted by runtime behavior changes; no changeset |
Annotations
Check failure on line 146 in packages/client/src/client/probeClassifier.ts
claude / Claude Code Review
probeClassifier: codec routing is not behavior-neutral for present-but-invalid cache hints
Routing classifyResult() through the 2026 codec's validateResult('server/discover', …) is not behavior-neutral for present-but-invalid cache hints: the old neutral DiscoverResultSchema had no ttlMs/cacheScope members, so a malformed value (e.g. cacheScope: 'session', ttlMs: -1) passed through and the server still classified 'modern', whereas the new dispatch schema rejects it and the client silently downgrades to the legacy era. Either tolerate malformed cache hints for classification purposes,
Check warning on line 53 in packages/core/test/wire/layeringInvariants.test.ts
claude / Claude Code Review
layeringInvariants: invariant (b) regex misses runtime 'export … from types/schemas.js'
Invariant (b)'s `RUNTIME_SCHEMAS_IMPORT` regex only matches statements beginning with `import`, so a runtime `export … from '../../types/schemas.js'` inside a `wire/rev*/` module would recreate exactly the public→wire runtime coupling this suite is meant to forbid yet still pass test (b) — even though invariant (a)'s `WIRE_REV_IMPORT` already covers both `import` and `export … from` forms. Mirror pattern (a) with the `(?!type\b)` guard, e.g. `/^(import|export)\s+(?!type\b)…/`.
Check warning on line 119 in packages/core/src/wire/rev2025-11-25/codec.ts
claude / Claude Code Review
Pure-refactor claim contradicted by runtime behavior changes; no changeset
The PR description says "Zero behavior change … runtime accept/reject identical" and ships no changeset, but the diff carries a few consumer-visible deltas in the published packages: the 2026 wire parse for `server/discover` now defaults `resultType`/`ttlMs`/`cacheScope` instead of requiring them (the new `encodeContract.test.ts` block pins the new acceptance, and the branch's own commit is titled "fix(core,client): receiver-side defaults … per spec"), and the sampling tools-vs-no-tools pick now