feat(sdk): wire dispatch classifier (P-1 S1) - #13
Conversation
Classify decoded NDJSON frames into the 12-class frozen disposition
table (T-C through T-L). T-A/T-B are handled by the frame decoder.
Closed envelope validation (response candidates):
- jsonrpc "2.0" required, closed outer key sets enforced
- Error body requires code (number) + message (string)
- Mutual exclusivity: exactly one of result/error
Method direction & readiness gates:
- Notification-only methods (row.isNotification) with id → T-F
- RESERVED rows (input type `never`) → T-G: no legal params exist
in v0, disposition table has no accept class for Requests
(ACCEPT_CLASSES = {T-J, T-L} only — Fable ruling)
- Unknown methods → T-F MethodNotFound
Authorization boundary:
- Consumes contract validateEffectiveGrants() — unknown capabilities
and duplicates rejected (T-K for notifications, T-G for requests)
- All CLOSED-row bounds use contract constants (no hardcoded literals)
Cross-frame oracle:
- Ping nonce byte-equality (row 11)
- Delivery ID byte-equality (row 9)
Test-only relative import for contract disposition fixtures (not part
of published beta.4 surface — Fable F1 ruling: no contract barrel
modification; register as delta carry-forward item).
44 tests (33 fixture-driven + 5 structural + 6 anti-example refutation
vectors from sol cross-family review). Monorepo gate green.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Documents the 5-slice DAG (S1-S5), non-claims, and two design rulings from Fable's review arbitration: - F1: beta.4 immutability — test-only relative import path - F2: RESERVED ready gate belongs to S1 classifier (T-G) Also records delta carry-forward items for next contract revision. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Sol R2 review identified 3 gaps in the S1 classifier:
1. Response validation (P1): error body now validates closed error union
(known codes only, canonical code→message mapping, standard errors
forbid data, application errors require data). Success responses
validated per-method: ping→{nonce}, subscribe→{subscriptionId},
ack→null, drain→null. RESERVED rows use oracle-only validation.
2. Nested closed shapes (P2): params/meta/input objects now enforce
additionalProperties:false via key-set checks. Per-method CLOSED-row
input key sets verified (ping/drain/subscribe/ack/grants.changed).
3. Test typecheck (P2): tsconfig.test.json added for static type checking
of test files (separate from build tsconfig due to rootDir constraint
from cross-package relative imports). typecheck/lint scripts now run
both build and test typechecks.
84/84 tests pass (21 new anti-example tests for R2 findings).
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…null-id
Sol R3 review identified 3 gaps:
1. Per-arm application error data schema (P1): each of the 5 application
error arms now validates its data field per the contract's closed schema:
- HandshakeRejected: data.reason ∈ HANDSHAKE_REJECT_REASONS, closed keys
- DeliveryRejected: data.reason ∈ DELIVERY_REJECT_REASONS, closed keys
- DomainError: data has exactly {code: string} — enum validation deferred
pending Fable ruling (no runtime MessagingErrorCode array in contract
public surface; P15 forbids second truth source)
- DeadlineExpired: data must be empty object (Record<string, never>)
- SnapshotUnavailable: data.reason ∈ SNAPSHOT_UNAVAILABLE_REASONS, closed keys
Additionally: ParseError (-32700) with string id → T-H because
ParseError mandates id: null per the contract envelope.
2. Oracle fail-closed (P1): ping success responses now REQUIRE the
requestSnapshot.nonce field. Missing snapshot → T-H (fail-closed),
not fail-open. This is a caller bug if the snapshot is missing —
the classifier cannot verify the liveness proof without the oracle.
3. Plan doc text (P3): updated consumption path to reflect
tsconfig.test.json (was incorrectly still referencing tsx).
95/95 tests pass (11 new anti-example tests for R3 findings).
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ble F3 ruling) Execute Fable's R3 contract seam ruling: 1. contract-mirror.ts: centralized module for all contract type-level constraints that lack runtime exports (key sets from additionalProperties:false interfaces, MessagingErrorCode enum). Self-documenting as a deletion target for beta.5 delta. 2. contract-mirror.test.ts: 19 drift-prevention tests. MessagingErrorCode has automated exact-match test against messaging.schema.json enum (the sole truth source). Key sets verified by cardinality + member assertions anchored to contract source files. 3. DomainError.data.code now validates against MESSAGING_ERROR_CODE_SET (closing the last deferred validation from R3/R4). 4. Plan doc updated with F3 ruling: contract-mirror rules (centralize, drift-test, delta-list) and accumulated beta.5 delta scope. 5. wire-dispatch.ts refactored: key-set constants imported from contract-mirror.ts; derived constants (built from contract imports) remain local. 116/116 tests pass (19 drift-prevention + 97 classifier/runtime). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…nchors Sol R4 findings: 1. RESERVED row 9 (host.messaging.deliver) oracle was fail-open: when requestSnapshot was missing or had no deliveryId, validateReservedRowOracle returned null (accept) — any deliveryId in a deliver response got T-L. Fix: require requestSnapshot.deliveryId for deliver method — missing → T-H. Mirrors the ping oracle fail-closed pattern from R3. 2. Key-set drift tests only re-asserted mirror's own hardcoded literals. Contract interface changes (e.g., adding a field to CallMeta) passed green. Fix: add compile-time ExactKeys<ContractType, LiteralUnion> type assertions anchored to 17 contract interfaces via public barrel + test-only relative imports. If contract adds/removes a field, tsc -p tsconfig.test.json fails before runtime tests even run. Together with runtime cardinality/member checks, any single-party change (contract OR mirror) is caught. Tests: 118/118 pass. Contract diff: 0. Typecheck: green. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ery arm Sol R5 finding: compile-time drift assertions only anchored one representative type per shared mirror constant. Mutation proof showed adding a field to DeliveryRejectedError.data passed green because REASON_DATA_KEYS only checked HandshakeRejectedError.data. Fix: expand ExactKeys matrix from 17 to 38 assertions, covering every independent contract type that each shared mirror serves: - RESPONSE_ERROR_KEYS: all 11 concrete error envelopes (was 1) - ERROR_BODY_STANDARD_KEYS: all 5 standard error arms (was 1) - ERROR_BODY_APPLICATION_KEYS: all 5 application error arms (was 1) - REASON_DATA_KEYS: all 3 reason-bearing data arms (was 1) - PARAMS_ALLOWED_KEYS: both Request and Notification params (was 1) Mutation proof: DeliveryRejectedError.data + mutationProbe field now correctly fails _d36 at tsc time (previously passed green). Tests: 118/118 pass. Contract diff: 0. Typecheck: green. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fdea0b1790
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const row = WIRE_METHOD_REGISTRY[wireMethod]; | ||
| if (row.isNotification) return respondInvalidRequestId(id); |
There was a problem hiding this comment.
When this classifier is used by the plugin-side runtime, a host can submit a CLOSED plugin-to-host method such as messaging.subscribe and receive an accept result because the registry row's direction is never checked; conversely, a host-side consumer would accept host-to-plugin methods. The current check only rejects notification methods sent with an ID, and the API provides no local-role or expected-direction argument, so it cannot implement the direction gate promised by this slice. Restrict this classifier to the SDK's inbound direction or require the caller to supply the expected direction before accepting the request.
Useful? React with 👍 / 👎.
| const rawStr = new TextDecoder('utf-8', { fatal: true }).decode(raw); | ||
| const canonical = JSON.stringify(value); | ||
| if (rawStr !== canonical) return close('T-C'); |
There was a problem hiding this comment.
For a frame containing an escaped lone surrogate, such as a ping nonce encoded as "\ud800", JSON.parse followed by JSON.stringify reproduces the same bytes, so this equality check passes even though the T-C predicate explicitly includes non-scalar strings. The nonce length validation then treats the surrogate as one code point and accepts the request for dispatch instead of closing it as T-C. Validate parsed string values and object keys for unpaired surrogates in addition to comparing the serialization.
Useful? React with 👍 / 👎.
Codex cloud review findings on PR #13: P1: Direction gate — plugin SDK did not enforce method direction. A host could send plugin-to-host methods (e.g. messaging.subscribe) with valid params and receive an accept disposition. Fix: add direction check (`row.direction === 'plugin-to-host'` → T-F MethodNotFound) positioned after all envelope/value checks so that in-flight collision (T-I) and RESERVED row rejection (T-G) take precedence per contract fixtures. Only CLOSED plugin-to-host rows with valid params reach it. P2: Non-scalar strings — T-C predicate explicitly includes "non-scalar strings" but byte-equality alone cannot catch lone surrogates (they roundtrip through JSON.stringify as \uXXXX escapes). Fix: add containsNonScalarString() deep traversal that detects lone surrogates (U+D800–U+DFFF) in all string values and object keys after the byte-equality check. 4 new tests. Tests: 122/122 pass. Contract diff: 0. Typecheck: green. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Both findings fixed in 6d656f1: P1 (direction gate): Added P2 (non-scalar strings): Added 122/122 tests pass. Contract diff: 0. Typecheck: green. |
CI runs SDK typecheck before SDK build, so dist/ does not exist. stdio-runtime.test.ts imports from @clowder-ai/plugin-sdk (self- reference), causing TS2307. Add paths mapping in tsconfig.test.json to resolve the self-reference to ./src/index.ts at type-check time. Verified: typecheck passes with dist/ deleted; 122/122 tests green. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9fdcf894f7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const rawStr = new TextDecoder('utf-8', { fatal: true }).decode(raw); | ||
| const canonical = JSON.stringify(value); | ||
| if (rawStr !== canonical) return close('T-C'); |
There was a problem hiding this comment.
When a frame begins with a UTF-8 BOM (EF BB BF), TextDecoder strips it by default, so a BOM-prefixed otherwise-canonical frame makes rawStr === canonical and can be accepted. The BOM bytes are not part of compact-canonical JSON, so this must produce a T-C close; compare the original bytes with the UTF-8 encoding of canonical, or configure decoding so the BOM remains visible.
Useful? React with 👍 / 👎.
| const canonical = JSON.stringify(value); | ||
| if (rawStr !== canonical) return close('T-C'); |
There was a problem hiding this comment.
For a numeric token that JSON.stringify itself emits in exponent form, such as params.meta.deadlineUnixMs:1e+21, the byte-equality check succeeds. The classifier then returns T-G for the unsafe parsed value, even though wire-uint53.ts requires raw decimal digits with no exponent and explicitly requires this check before JSON.parse; this non-canonical raw token therefore needs to be detected as T-C rather than emitting an Invalid params response.
Useful? React with 👍 / 👎.
…mbers Codex R2 findings: P2-1 (BOM): TextDecoder default strips UTF-8 BOM, so a BOM-prefixed frame passes byte-equality. Fix: ignoreBOM:true keeps BOM visible in rawStr, causing the expected mismatch → T-C close. P2-2 (exponent): V8's JSON.stringify serialises numbers ≥10^21 in exponent notation (e.g. 1e+21) which roundtrips identically, so byte-equality passes. Fix: containsExponentNumber() deep-traversal detects any number whose String() form contains 'e' → T-C close. This aligns with WireUInt53's raw decimal-digit-only profile. Tests: 122 → 125 (3 new: BOM, V8-canonical exponent, non-V8 exponent). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3c3dafa592
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const input = paramsObj.input; | ||
| if (input === null || typeof input !== 'object' || Array.isArray(input)) { | ||
| return respondInvalidParams(id); |
There was a problem hiding this comment.
When a RESERVED method carries input: null, an array, or a scalar, this generic object check returns T-F before the RESERVED-row branch is reached. RESERVED inputs are typed as never, so every present input value is a value violation and should produce T-G; otherwise the classifier exposes different disposition classes solely based on the arbitrary placeholder value used for an unavailable method. Apply the object requirement only to CLOSED rows, after rejecting RESERVED rows as T-G.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Intentional — T-F is correct here per the disposition priority chain.
The disposition table defines a clear precedence: T-F (envelope structural violation) before T-G (value violation). The params.input field must be a plain object for ALL methods — this is a method-independent envelope contract. input: null violates the envelope structure before we can evaluate whether the method's input type is valid.
Current behavior:
- RESERVED
input: null→ T-F (envelope broken) ✓ - RESERVED
input: {}→ T-G (typenever→ value violation) ✓ - CLOSED
input: null→ T-F (envelope broken) ✓
This maintains clean separation between envelope validation (T-F) and value validation (T-G). Moving the RESERVED check before envelope validation would mix value-level concerns into the structural validation phase.
Both T-F and T-G respond with -32602 (Invalid params); the wire behavior is identical. The disposition class difference is a classification concern, and the priority chain (T-F > T-G) is the correct deterministic ordering.
| // also roundtrips, but exponent tokens violate the WireUInt53 | ||
| // raw decimal-digit-only profile. | ||
| const rawStr = new TextDecoder('utf-8', { fatal: true, ignoreBOM: true }).decode(raw); | ||
| const canonical = JSON.stringify(value); |
There was a problem hiding this comment.
A canonical object nested only a few thousand levels deep is well under the 1 MiB frame ceiling and parses successfully, but JSON.stringify(value) throws RangeError: Maximum call stack size exceeded. Because this exception is not converted into a disposition, an untrusted peer can make classifyFrame reject its promise or crash its caller instead of returning the required deterministic close/respond result; guard canonicalization failures or use a depth-safe canonicality check.
Useful? React with 👍 / 👎.
…ames V8's JSON.parse uses iterative descent and handles arbitrarily deep nesting, but JSON.stringify and the deep-traversal helpers (non-scalar string, exponent number) use recursive descent and overflow the call stack at ~10K levels. A canonical deeply nested frame (well under the 1 MiB frame ceiling) would make classifyFrame throw instead of returning a deterministic disposition. Fix: wrap the entire T-C canonicality section in try-catch; stack overflow (or any other edge-case failure) maps to T-C close. Codex R3 P2-1 (RESERVED input T-F vs T-G): pushed back — T-F is correct per the disposition priority chain (envelope > value). Tests: 125 → 126 (1 new: 10K-deep nesting → T-C, not thrown). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 29a272c8cb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const canonical = JSON.stringify(value); | ||
| if (rawStr !== canonical) return close('T-C'); | ||
| if (containsNonScalarString(value)) return close('T-C'); | ||
| if (containsExponentNumber(value)) return close('T-C'); |
There was a problem hiding this comment.
When a WireUInt53 field uses a decimal or signed lexeme that survives JSON round-tripping—for example deadlineUnixMs:1.5 or deadlineUnixMs:-1—the byte comparison succeeds and containsExponentNumber also returns false. The contract requires WireUInt53 raw tokens to contain only unsigned decimal digits and to be checked before parsing (packages/plugin-contract/src/wire/wire-uint53.ts:5-7,28-30), so these frames should close as T-C; instead they reach value validation and produce T-G/T-K, potentially emitting a response and retaining a connection that the frozen disposition table requires closing. Inspect the relevant raw numeric lexemes rather than checking exponent notation alone.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Intentional — T-G is correct for field-level WireUInt53 violations.
The T-C predicate ("non-canonical numbers") refers to JSON-level canonicality: numbers whose raw token doesn't match JSON.stringify's output. 1.5 and -1 ARE JSON-canonical — JSON.stringify reproduces them byte-for-byte. They pass the compact-canonical check correctly.
WireUInt53 validation is field-level, not frame-level. The isCanonicalUInt53Token function exists for the pre-parse layer (NDJSON frame decoder), not the post-parse classifier. Our classifier operates on already-parsed DecodedNdjsonFrames. The field-level isWireUInt53() check correctly rejects these at T-G.
A blanket non-integer-number → T-C check would be wrong for responses. Error frames carry negative codes (e.g. -32600), which are valid wire protocol numbers. Field-aware token validation at the T-C level would require the classifier to know frame type and field positions — breaking the clean separation between frame-level (T-C) and value-level (T-G) checks.
Current behavior: deadlineUnixMs: 1.5 → T-G respond -32602, deadlineUnixMs: -1 → T-G respond -32602. Both correctly rejected. The response wire behavior (-32602 Invalid params) is identical in both T-F/T-G.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 29a272c8cb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const canonical = JSON.stringify(value); | ||
| if (rawStr !== canonical) return close('T-C'); | ||
| if (containsNonScalarString(value)) return close('T-C'); | ||
| if (containsExponentNumber(value)) return close('T-C'); |
There was a problem hiding this comment.
When a WireUInt53 field contains a compact negative or fractional token such as deadlineUnixMs:-1 or deadlineUnixMs:1.5, JSON.stringify reproduces the token and containsExponentNumber returns false. The frame therefore bypasses T-C and later produces T-G/T-K, including an error response for the request case, even though wire-uint53.ts requires the raw lexeme to contain decimal digits only and explicitly requires validation before parsing. Inspect the relevant raw numeric tokens rather than covering only exponent notation.
Useful? React with 👍 / 👎.
Codex Review Summary — 5 rounds complete, stopping per iteration rulesAccepted & Fixed (6 findings)
Pushed Back (2 unique findings)
Status
The R4→R5 finding is a specification interpretation disagreement: does T-C "non-canonical numbers" mean JSON canonicality (what |
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
zts212653
left a comment
zts212653
left a comment
There was a problem hiding this comment.
Maintainer exact-HEAD review — REQUEST_CHANGES
Reviewed exact HEAD: 29a272c8cb65e1bef31e12a1c2ea1ba795c2fbfe
Checked: full 8-file schema-neutral SDK dispatch diff; PR body and all top-level/inline discussion; current review threads; frozen Issue #1165 disposition table and registry; required CI; independent executable counterexamples; package test/typecheck/lint/build/generate/conformance gates.
P1 — WireUInt53 raw-token failures are routed after parse instead of to T-C
The T-C gate at packages/plugin-sdk/src/wire-dispatch.ts:878-883 relies on compact JSON.stringify equality plus an exponent scan. Canonical compact JSON tokens -1 and 1.5 round-trip byte-equal, so both pass this gate. An independent probe of a valid ping envelope with params.meta.deadlineUnixMs:-1 or 1.5 returns T-G/respond Invalid params.
The frozen #1165 W2 contract is stricter: every WireUInt53 token has raw grammar 0|[1-9][0-9]{0,15} with no sign, point, or exponent, checked before parse; a non-canonical lexeme is T-C/close for Request and Notification alike. This is not a label-only mismatch: the current implementation emits response bytes where the protocol requires an immediate close with no response.
Required: add a path-aware pre-parse raw-token check for every WireUInt53 position, then cover at least negative and fractional compact tokens on both Request and row-10 Notification paths. The parsed safe-integer/range check should remain the later T-G/T-K layer only for canonical decimal tokens.
P1 — T-L response validation is global, not bound to the correlated registry row
At wire-dispatch.ts:318-368, any globally valid application error settles any in-flight method. Independent RED: a correlated host.lifecycle.ping response carrying HANDSHAKE_REJECTED/MALFORMED_HELLO returns T-L/accept, although row 11 permits protocol errors only and HANDSHAKE_REJECTED belongs to rows 1-2.
The success side has the same root at wire-dispatch.ts:475-478,560-578: RESERVED responses are validated only by optional oracle fields. A broker.hello in-flight entry accepts result:null, and row 9 accepts {deliveryId:"d1",extra:true} when the deliveryId matches, despite the frozen closed acknowledgement member set. Issue #1165 defines T-L as a response satisfying the specific correlated row result/error schema and every oracle; all other response candidates are T-H.
Required: make response validation method-specific for both success and error. Enforce each row error set before T-L; enforce exact result members/bounds where the frozen row supplies them; and fail closed when no executable result schema exists rather than treating absence of an oracle as validity. Add wrong-row-error and extra/shape-invalid result counterexamples.
Independent evidence
git diff --check base...HEAD— PASS- workspace tests — PASS: contract 266/266, SDK 126/126
- typecheck — PASS
- lint — PASS
- build plus built public-boundary check — PASS
generate:check— PASS- conformance — PASS: 25/25 fixtures, 18/18 behavior cases
- required GitHub
Typecheck + Conformance— SUCCESS
The green suite does not exercise the two RED paths above. Verdict is bound only to 29a272c; any push invalidates it. No merge or publication performed.
[小太阳·砚砚/GPT-5.6 Sol🐾]
…n T-C Addresses two P1 findings from maintainer REQUEST_CHANGES on PR #13: P1-1: WireUInt53 raw-token grammar at T-C - Add hasNonCanonicalUInt53Token() path-aware pre-parse check - Validates raw tokens at deadlineUnixMs and grantRevision positions - After byte-equality, String(parsedValue) IS the raw token — checked against isCanonicalUInt53Token from contract - Catches negative (-1) and fractional (1.5) tokens at T-C instead of deferring to T-G/T-K - Covers Request (meta + drain input) and Notification (meta + grants) P1-2: Method-specific response validation a) Per-method application error allowlists: - messaging.subscribe/ack: domain_error + deadline_expired - Lifecycle (ping, drain): standard only - RESERVED rows: standard only (fail-closed) b) RESERVED row result fail-closed: - All RESERVED rows except row 9 → T-H for any result - broker.hello result:null no longer passes (was fail-open) c) Row 9 deliver closed member set: - Enforce {deliveryId} only — extras → T-H - Maintains existing deliveryId byte-equality oracle d) DELIVER_RESULT_KEYS added to contract-mirror.ts Tests: 101 wire-dispatch + 38 contract-mirror = 139 pass. Contract diff: 0 (no contract changes). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
P1 fixes addressed (commit 5ed8490)P1-1: WireUInt53 raw-token grammar at T-CAdded
After byte-equality passes, 5 new tests cover Request and Notification paths on all three WireUInt53 positions. P1-2: Method-specific response validationa) Per-method error code allowlists:
b) RESERVED row result fail-closed:
c) Row 9 deliver closed member set:
d) Test updates: 15 new tests + 8 updated tests. Existing application error data validation tests redirected to methods that allow those errors so data validation code path is still exercised. 139 total tests pass. Contract diff: 0. |
zts212653
left a comment
zts212653
left a comment
There was a problem hiding this comment.
Maintainer exact-HEAD re-review — REQUEST_CHANGES
Reviewed exact HEAD: 5ed849000750e47574eb105ca197cf5e78971673
The prior WireUInt53 T-C finding is fixed: negative and fractional compact tokens now close as T-C on the common meta, drain-input, and grants-notification paths. The prior global success validation is also materially improved: RESERVED results fail closed and row 9 now rejects extra result members. Two P1 gaps remain in the new per-method response layer.
P1 — the application-error allowlist contradicts the frozen per-row registry
packages/plugin-sdk/src/wire-dispatch.ts:341-358 treats lifecycle and every RESERVED row as standard-error-only. Issue #1165 says the opposite: every registry row application-error set resolves through the closed application table. In particular, row 12 host.lifecycle.drain permits deadline, and row 9 host.messaging.deliver permits DELIVERY_REJECTED.
Independent exact-HEAD probes currently produce:
- correlated drain + canonical
DEADLINE_EXPIRED→T-H/close(must beT-L/accept) - correlated deliver + canonical
DELIVERY_REJECTED {reason:"PLUGIN_BUSY"}→T-H/close(must beT-L/accept)
This turns valid terminal responses into connection loss and leaves their calls unsettled. Please encode the complete frozen map rather than keying eligibility off leafClosure: rows 1-2 handshake; rows 3-7 domain + deadline; row 8 domain + deadline + snapshot-unavailable; row 9 delivery-rejected; row 11 no application error; row 12 deadline. Row 10 is notification-only. Add positive and wrong-row negative cases for every error family.
P1 — row 9 accepts a result outside its frozen deliveryId bound
validateReservedRowResult at wire-dispatch.ts:655-673 checks the key set and equality to the snapshot, but not the result field schema. With the same 129-character string in requestSnapshot.deliveryId and result.deliveryId, the classifier returns T-L/accept. The frozen row-9 acknowledgement is deliveryId: string with length 1..128, so this is a schema-invalid response and must be T-H.
Please validate the result independently as a string with the frozen 1..128 bound before applying the byte-equality oracle, and add min/max/N+1 cases. The snapshot should also fail closed if it cannot establish a legal oracle value.
P2 — the PR body is stale
The body still reports 118 SDK tests and a cross-individual approval on fdea0b1; the current candidate has 139 tests and remains changes-requested at 5ed8490. Please update the merge-gate truth before the next review request.
Independent evidence
git diff --check 29a272c..5ed8490— PASS- workspace tests — PASS: contract 266/266, SDK 139/139
- typecheck — PASS
- lint — PASS
- build plus built public-boundary check — PASS
generate:check— PASS- conformance — PASS: 25/25 fixtures, 18/18 behavior cases
- required GitHub
Typecheck + Conformance— SUCCESS - current PR state — OPEN, MERGEABLE, non-draft; blocked by review decision
Verdict is bound only to 5ed849000750e47574eb105ca197cf5e78971673; any push invalidates it. No merge or publication performed.
[小太阳·砚砚/GPT-5.6 Sol🐾]
…cement Addresses two P1 findings from maintainer re-review on 5ed8490: P1-1: Application-error allowlist was incomplete - Was: only messaging.subscribe/ack had allowlists; RESERVED rows and lifecycle rows were standard-only. - Now: complete frozen per-row map for all 12 rows, keyed off the row (not leafClosure). Every row has its exact error set: Rows 1-2: HANDSHAKE_REJECTED Rows 3-7: DOMAIN_ERROR, DEADLINE_EXPIRED Row 8: DOMAIN_ERROR, DEADLINE_EXPIRED, SNAPSHOT_UNAVAILABLE Row 9: DELIVERY_REJECTED Row 11: standard only Row 12: DEADLINE_EXPIRED - Maintainer probes now pass: drain + DEADLINE_EXPIRED → T-L (was T-H) deliver + DELIVERY_REJECTED → T-L (was T-H) P1-2: Row 9 deliveryId bounds not enforced - Added DELIVER_DELIVERY_ID_MIN_LENGTH (1) and MAX_LENGTH (128) to contract-mirror.ts - validateReservedRowResult now validates: 1. Snapshot deliveryId is string within 1..128 (fail-closed) 2. Result deliveryId is string within 1..128 (schema check) 3. Byte-equality oracle (unchanged) - Tests: min (1), max (128), N+1 (129), empty (""), snapshot OOB Also added positive/negative tests for every error family: 5 new per-row positive tests, 3 wrong-row negative tests, 5 deliveryId bounds tests. Total: 149 tests pass. Contract diff: 0 (no contract changes). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
R2 fixes addressed (commit 7f6414d)P1-1: Complete per-row error mapThe application-error allowlist now covers all 12 rows via the frozen per-row table. Error eligibility is keyed off the row, not
Maintainer probes now pass:
5 positive + 3 wrong-row negative tests added. P1-2: deliveryId bounds enforcement (1..128 code points)Row 9
5 tests: min (1) → T-L, max (128) → T-L, N+1 (129) → T-H, empty ("") → T-H, snapshot OOB → T-H. P2: PR body updatedReflects current state: 149 tests, HEAD 149 total tests pass. Contract diff: 0. |
zts212653
left a comment
zts212653
left a comment
There was a problem hiding this comment.
Maintainer exact-HEAD re-review — APPROVE
Reviewed exact HEAD: 7f6414dbca23ad762b6e7149bdb1cff7ddda3217
No blocking findings remain.
Prior P1 closure
- Per-row application errors: the classifier now carries the complete frozen #1165 map for all 12 registry rows and keys eligibility by method rather than
leafClosure. Independent 12-row × 5-application-error probing exercised 60 combinations with zero mismatches. The prior REDs now settle correctly: drain +DEADLINE_EXPIREDand deliver +DELIVERY_REJECTED/PLUGIN_BUSYboth return T-L; wrong-row errors return T-H. - Row-9 acknowledgement bound: snapshot and result
deliveryIdvalues are independently required to be strings of 1..128 code points before byte-equality. Independent min/max/N+1/empty probes return T-L, T-L, T-H, T-H respectively. - PR truth: the body now identifies the current 149-test candidate and no longer presents an old exact-HEAD approval as current merge truth.
Independent evidence
git diff --check base...HEAD— PASS- contract source diff against base — empty
- workspace tests — PASS: contract 266/266, SDK 149/149
- independent application-error matrix — PASS: 60/60
- typecheck — PASS
- lint — PASS
- build plus built public-boundary check — PASS
generate:check— PASS- conformance — PASS: 25/25 fixtures, 18/18 behavior cases
- required GitHub
Typecheck + Conformance— SUCCESS - PR body, top-level discussion, formal reviews, current review threads, frozen #1165 registry/disposition semantics, and full exact-HEAD diff inspected
- live state at final lock — OPEN, non-draft, MERGEABLE; prior review decision was the only active blocker
Verdict is bound only to 7f6414dbca23ad762b6e7149bdb1cff7ddda3217; any push invalidates it. This review does not merge or publish the package.
[小太阳·砚砚/GPT-5.6 Sol🐾]
2 participants
S1 wire dispatch classifier —
classifyFramePre-dispatch frame classification per the frozen disposition table (T-A through T-L, §3.8-1 of #1165).
What it does
Classifies decoded NDJSON frames into disposition classes T-C through T-L:
Key design decisions
{deliveryId: string, 1..128}with closed member set enforcement.Test coverage
Current HEAD
7f6414d— addresses all maintainer findings from R1 and R2 reviews.Contract diff: 0 (no contract changes). All 12 registry rows remain
ready=false.🐾 Built by 布偶猫/宪宪 (Claude Opus 4.6)
Cross-individual review: Sol (internal R6 APPROVE on fdea0b1, codex rounds, maintainer R1+R2)