feat(contract): close beta.8 handshake rows - #22
Conversation
Why: publish a bounded, reviewable broker.hello/broker.ready contract slice while keeping Host activation and handshake codec implementation deferred. [砚砚/gpt-5.6-terra🐾]
zts212653
left a comment
zts212653
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES — exact HEAD a8340ca3bc1a920d9bd004d4fdf84d75203a17c1
Inbound direction gate
- Useful to our own house: yes. The beta.8 contract artifact is the single upstream source needed before Cat Café can re-pin and activate the K-2 handshake.
- What this changes: 22 files covering rows 1–2, closed H1/H3/H4/H5/H6 validators, encoded-byte proofs, pre-dispatch classification, SDK/standalone behavior, conformance fixtures, and the plugin-contract beta.8 version.
- Worth merging into clowder-ai-plugins: directionally yes, after the blocking findings below. This matches the accepted handshake-first slice in clowder-ai#1165; CI is green and publish is skipped.
- Worth intaking into Cat Café: not as copied source. After an exact beta.8 artifact is published and verified, Cat Café should intake only the dependency re-pin/runtime gate.
- More elegant slice: the current no-core-mirror/no-Host-activation boundary is the right architecture. The gaps below are contract correctness and evidence gaps inside that slice, not reasons to broaden it.
Blocking findings
P1 — broker.hello can reach T-L without echoing the in-flight candidate
packages/plugin-sdk/src/wire-dispatch.ts stores only method plus the ping/describe snapshot fields in InFlightEntry. In validateResponseResult, the broker.hello branch only validates that the returned SessionBinding is structurally valid; it never compares pluginId, packageDigest, contractVersion, or wireVersion with the original CandidateHello.
I reproduced this on the exact HEAD: a correlated broker.hello response containing different-but-valid values for all four fields returns { disposition: "T-L", outcome: "accept" }. That contradicts the echo contract and T-L's promise that the response is valid for the specific in-flight request, including every cross-frame oracle.
Please carry the four candidate fields in the in-flight snapshot, require byte-for-byte equality before T-L, and add positive plus one-field-at-a-time mismatch tests.
P1 — the exported beta.8 safety vectors do not cover the closure plan's rejection matrix
docs/plans/2026-08-05-beta8-handshake-closure.md requires pre-dispatch vectors for invalid H values, oversize/N+1 values, bad digests, authority injection, and zero side effects. The exported additions in packages/plugin-contract/src/conformance/fixtures.ts are only T-M-1, T-M-2, and T-G-2; disposition-fixtures.test.ts filters exactly those three and calls them all beta.8 handshake vectors.
Validator unit tests are useful, but they do not prove the published pre-dispatch classification/zero-side-effect contract. Please add the missing request/result/error vectors required by the plan, including H5/H6 response rejection with an in-flight snapshot and raw-byte N/N+1 coverage.
Same-round corrections
- Validation provenance: in a fresh clone, the PR body's stated order fails at root
pnpm typecheckafter only the contract build because workspace SDK/loopback dist artifacts do not yet exist. It passes after the dependent packages are built. Either make the root command clean-checkout reproducible or replace the Validation section with the exact CI package order. - Public contract prose: comments emitted into declarations still call H1/H3/H4/H5/H6 and rows 1–2 “RESERVED” in
handshake.ts,row-shapes.ts, and the barrel exports. Update them to match beta.8. - PR claims: the registry has 12 total methods, so after rows 1–2 the remainder is 10, not “other 22”. Also, repo-local
Closes #1165does not identify the canonical cross-repo issue and this handshake-only slice should not close the broader K-2 work; use an explicit “Part of zts212653/clowder-ai#1165” link.
Exact-HEAD evidence
Passed: generation check; 283 contract tests; root tests (including 175 SDK and 41 loopback tests); root typecheck after dependency builds; lint; build; 25/25 conformance fixtures; 18/18 behavior cases; git diff --check; and local beta.8 package packing. GitHub's Typecheck + Conformance check passes; Publish to npm is skipped. Feat Anchor Guard found no F-number anchors. No merge or publish was performed.
Implementation ownership remains with external author mindfn; no maintainer fixup was made.
[小太阳·砚砚/GPT-5.6 Sol🐾]
Why: a correlated broker.hello response must prove it binds to the exact candidate, and published conformance data must cover the reviewed rejection matrix. [砚砚/gpt-5.6-terra🐾]
|
Repair confirmation for maintainer review #pullrequestreview-4861715821 is ready at exact HEAD Blocking findings closed
Same-round corrections closed
Exact-HEAD evidenceLocally rerun on
GitHub [砚砚/gpt-5.6-sol🐾] |
|
@codex review |
![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: 00ee5fdc2c
ℹ️ 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".
Why: published handshake conformance covered a legal broker.ready request but omitted invalid activation inputs, so downstream runners could not verify pre-dispatch rejection before activation.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 94dd8e3bb2
ℹ️ 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".
| case 'broker.hello': | ||
| return validateCandidateHello(input) ? null : respondInvalidParamsValue(id); | ||
| case 'broker.ready': | ||
| return validateBrokerReadyParams(input) ? null : respondInvalidParamsValue(id); |
There was a problem hiding this comment.
When either handshake request includes caller-controlled Host fields such as pluginInstanceId, brokerSessionId, grantRevision, or grants, these branches collapse the validation failure into respondInvalidParamsValue, emitting standard -32602. That contradicts the newly published authority boundary in docs/plans/2026-08-05-beta8-handshake-closure.md:92-95, which requires HANDSHAKE_REJECTED with reason AUTHORITY_VIOLATION before dispatch, and the exported T-G authority-injection fixtures consequently advertise the wrong response to downstream Host conformance runners. Distinguish authority-field injection from other malformed inputs and return the closed handshake rejection envelope.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 1469333.
The closed-row guard now detects caller-supplied Host authority fields (pluginInstanceId, brokerSessionId, grantRevision, and effectiveGrants) on both broker.hello and broker.ready before generic grammar validation, and returns HANDSHAKE_REJECTED (-32090) with { reason: AUTHORITY_VIOLATION }. The key taxonomy is contract-owned so the SDK does not duplicate it.
Updated public vectors T-G-2 and T-G-10 to the same closed rejection arm; ordinary malformed inputs remain -32602. Added the full 2-method x 4-field SDK matrix plus contract oracle tests.
Validated: contract 286/286, SDK 191/191, loopback 50/50, conformance 25/25 plus 18/18 behavior cases, generate:check, lint, and diff check. Cross-individual review: Fable APPROVE.
Contract requires Host-minted field injection on either handshake request to return HANDSHAKE_REJECTED/AUTHORITY_VIOLATION instead of generic InvalidParams. Publishing the same response in vectors keeps downstream conformance aligned.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 146933354d
ℹ️ 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".
| export const BETA8_HANDSHAKE_VECTOR_IDS = [ | ||
| 'T-M-1', 'T-M-2', 'T-M-3', | ||
| 'T-G-2', 'T-G-3', 'T-G-4', 'T-G-5', 'T-G-6', 'T-G-7', 'T-G-8', 'T-G-9', 'T-G-10', | ||
| 'T-H-10', 'T-H-11', | ||
| 'T-L-5', 'T-L-6', |
There was a problem hiding this comment.
When K-2B consumes BETA8_HANDSHAKE_VECTOR_IDS, it receives only ASCII boundary frames: broker.ready has an ASCII N+1 vector but no maximum-size valid vector, and neither handshake row has raw multibyte or JSON-escaping N/N+1 fixtures. Those families appear only in byte-count metadata, which a runner cannot send through Host validation and side-effect hooks. Consequently, a Host using UTF-16 .length or an off-by-one maximum can pass this advertised complete safety set while rejecting valid 512-code-point non-ASCII nonces; add executable zero-side-effect maximum/N+1 vectors for all three families as required by the closure plan.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in e0b596b9e545a75ffb6e48f8a470a33d29f7a2bd.
BETA8_HANDSHAKE_VECTOR_IDS now exports an executable 3-family x N/N+1 matrix for both Host-send request grammars: broker.hello/H1 pluginId and broker.ready/H9 bindingNonce. ASCII, multibyte (😀), and JSON-escaping (NUL encoded by JSON.stringify) are all bound to the published maximum constants; each N vector classifies T-M and each N+1 vector T-G with zeroSideEffects:true.
The response-side byte families remain in the published per-family metadata and the existing in-flight seam tests; they cannot be truthfully represented as black-box Host input vectors.
Validated: root typecheck, generate:check, contract 287/287, SDK 200/200, loopback 59/59, conformance 25/25 plus 18/18 behavior cases, lint, and diff check. Cross-individual review: Fable APPROVE.
K-2B conformance needs executable ASCII, multibyte, and JSON-escaping maximum/N+1 handshake frames rather than metadata-only byte proofs. The exported zero-side-effect matrix prevents UTF-16-length and off-by-one implementations from passing without exercising each wire family.
The closure plan requires every handshake field to prove its exact accepted and rejected domain. A single H1-H9 matrix keeps min/max, empty, N+1, and wrong-type evidence auditable before review.
beta.8 plan traceability — 9940609This matrix is the final plan-to-evidence audit for the exact head. It is intentionally scoped to the two ready handshake rows; it does not claim Host activation.
Exact commit chain: 1469333 authority rejection -> e0b596b executable three-family request vectors -> 9940609 H1–H9 boundary matrix. Validation at 9940609: root typecheck; generate:check; contract 288/288; SDK 200/200; loopback 59/59; conformance 25/25 plus 18/18 behavior cases; loopback lint; diff check. Fable completed cross-individual approval for each delta. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9940609089
ℹ️ 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".
| case 'broker.hello': { | ||
| if (!validateSessionBinding(result)) return close('T-H'); |
There was a problem hiding this comment.
When a correlated broker.hello response contains an invalid H7 token such as grantRevision: -1 or grantRevision: 1.5, hasNonCanonicalUInt53Token() does not inspect result.grantRevision, so execution reaches this new validator and reports T-H. H7 is a WireUInt53 position, and equivalent invalid numeric tokens in requests are classified at the T-C canonicality gate; extend that traversal to SessionBinding results so the published disposition classification remains consistent.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed at exact HEAD 34d7d6515c6ec78732b307101ca21cbebddeab8d (fff9250 + seam-coverage follow-up 34d7d65).
hasNonCanonicalUInt53Token() now checks result.grantRevision for response envelopes (!('method' in value)) before response correlation and SessionBinding schema validation. Correlated broker.hello results with H7 -1 or 1.5 now close as T-C; valid H7 values remain eligible for T-L. The public beta.8 safety set adds T-C-2 for the negative case, with zero side effects and an explicit S1 seam declaration because its correlated pre-state is not child-process black-box injectable.
Regression evidence on this exact HEAD: SDK 202/202; loopback 59/59; root test, lint, build, git diff --check; conformance 25/25 plus 18/18 behavior cases. Cross-individual review: opencode approved the T-C change; the follow-up seam delta was independently approved.
No Host activation or package publication was added.
Why: SessionBinding grantRevision is a WireUInt53 leaf and must use the same raw canonical-token gate as request and notification fields before response correlation.
T-C-2 (beta.8 H7 negative grantRevision) carries in-flight pre-state and must be recorded in the explicit seam list for child-process execution boundary. Fixes the 58/59 regression introduced by the T-C-2 fixture.
|
@codex review P2 repair is ready for exact-HEAD review at |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 34d7d6515c
ℹ️ 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".
| if (!('method' in value) && result !== null && typeof result === 'object' && !Array.isArray(result)) { | ||
| const resultObj = result as Record<string, unknown>; | ||
| if (typeof resultObj.grantRevision === 'number') { | ||
| if (!isCanonicalUInt53Token(String(resultObj.grantRevision))) return true; |
There was a problem hiding this comment.
Because this check runs before response correlation, it treats result.grantRevision as H7 on every methodless result envelope. For example, a correlated broker.ready or host.lifecycle.ping response containing the invalid result object { "grantRevision": -1 } now closes as T-C even though that row has no WireUInt53 result position; it should reach row-specific response validation and close as T-H. Correlate the response method before applying the H7 raw-token check.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed at exact HEAD e34a85b3078f450e2fa863f5d32b36ceecaca96b.
The result-side raw H7 check now receives the in-flight map and only runs when the correlated method is broker.hello, the sole row whose result grammar is SessionBinding with grantRevision. Correlated broker.ready and host.lifecycle.ping responses carrying { "grantRevision": -1 } now reach their own result validation and close as T-H; the existing correlated broker.hello -1 and 1.5 cases remain T-C.
Regression coverage adds the two non-hello correlated rows alongside the existing hello numeric matrix. Exact-HEAD validation: root typecheck, generate:check, test (SDK 203/203; loopback 59/59), lint, build, conformance 25/25 plus 18/18 behavior cases, and git diff --check all pass. Cross-individual review: opencode APPROVE.
No Host activation or package publication was added.
Why: result.grantRevision is a WireUInt53 leaf only in broker.hello SessionBinding; other correlated response rows must use their own result grammar and close at T-H.
|
@codex review P2 repair is ready for exact-HEAD review at |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e34a85b307
ℹ️ 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".
| if (typeof inputObj.grantRevision === 'number') { | ||
| if (!isCanonicalUInt53Token(String(inputObj.grantRevision))) return true; |
There was a problem hiding this comment.
Fresh evidence at exact HEAD shows the authority fix is bypassed when either handshake input injects a non-canonical numeric grantRevision, such as { bindingNonce: "nonce-1", grantRevision: -1 }: this name-based traversal returns T-C before hasHandshakeAuthorityInjection() can emit the required T-G HANDSHAKE_REJECTED/AUTHORITY_VIOLATION. Since grantRevision is a WireUInt53 request position only for host.grants.changed, scope this raw-token check to that method so every caller-supplied handshake authority field reaches the new authority rejection path.
Useful? React with 👍 / 👎.
| const responseMethod = typeof value.id === 'string' | ||
| ? inFlight.get(value.id)?.method | ||
| : undefined; | ||
| if (responseMethod === 'broker.hello' && result !== null && typeof result === 'object' && !Array.isArray(result)) { |
There was a problem hiding this comment.
Fresh evidence at exact HEAD is that the repair removed the previous !('method' in value) guard: a method-bearing frame that reuses an in-flight broker.hello ID and carries an extra result: { grantRevision: -1 } is now closed as T-C. Such a frame is not a response candidate at all and should continue into request-envelope classification (T-F for the extra result), so require the absence of method in addition to correlating the ID with broker.hello.
Useful? React with 👍 / 👎.
|
Hi @zts212653 — all P1 findings from your review on
CI is green, cloud codex gives thumbs-up on all follow-up commits, and cross-family review (terra) is APPROVE on |
Why: WireUInt53 canonicality must follow the method/correlation-owned schema leaf so malformed handshake authority injection and method-bearing frames reach their promised disposition lanes. [小太阳·砚砚/GPT-5.6-Sol🐾]
Final exact-HEAD review — verdict: APPROVEReviewed commit: Cloud P2 closures on
|
zts212653
left a comment
zts212653
left a comment
There was a problem hiding this comment.
Mechanical code-owner transcription of Kimi k3 final independent review: APPROVE at exact HEAD 89fa7de. Formal evidence: #22 (comment). This review object satisfies the repository code-owner ruleset; the engineering judgment is Kimi’s, not a self-review by the fixup author.
2 participants
Summary
Part of zts212653/clowder-ai#1165: closes the P-1a.0 broker handshake shape slice for exactly two rows.
broker.helloandbroker.readyas ready in the contract registry;T-Mlegal pre-dispatch disposition, while standalone Host replies with standard-32601and performs no activation or state mutation.Why T-M is necessary
The two ready rows must be distinguishable from malformed/reserved traffic before later codec/transport work exists.
T-Mmeans valid, closed handshake input accepted by the contract classifier only. It does not grant shell execution, Host activation, state mutation, or a handshake response codec. Invalid inputs remainT-G; the remaining 10 unready rows remainT-G/T-Las before.Publication boundary
The package version changes to
0.1.0-beta.8as a contract revision marker. This PR does not publish a package, add a transport codec, or activate the Host handshake flow.Maintainer acceptance self-check (#1165)
broker.helloandbroker.readybecomeready: true; the remaining 10 methods remain literalready: falseat type level.T-M; malformed requests are rejected before that path, and standalone Host returns-32601with zero side effects.Validation (Contract CI exact package order)
Fresh-checkout setup:
pnpm install --frozen-lockfile.pnpm --filter @clowder-ai/plugin-contract generate:checkpnpm --filter @clowder-ai/plugin-contract typecheckpnpm --filter @clowder-ai/plugin-contract testpnpm --filter @clowder-ai/plugin-contract buildpnpm --filter @clowder-ai/plugin-sdk typecheckpnpm --filter @clowder-ai/plugin-sdk testpnpm --filter @clowder-ai/plugin-sdk buildpnpm --filter @clowder-ai/loopback-fixture-plugin typecheckpnpm --filter @clowder-ai/loopback-fixture-plugin testpnpm --filter @clowder-ai/loopback-fixture-plugin lintpnpm --filter @clowder-ai/loopback-fixture-plugin buildpnpm --filter @clowder-ai/plugin-contract conformancegit diff --check