fix(wire-shape): close 4 review-surfaced gaps from #182 + #184#185
Merged
Conversation
saurabhjain1592
added a commit
that referenced
this pull request
Apr 25, 2026
The wire-shape contract gate verifies that SDK type definitions match the OpenAPI spec. It does NOT verify that the runtime code (transformers, response decoders, request builders) actually propagates every field from the type definition. PR #185's post-merge audit found multiple bugs of this shape: a type gained a wire-canonical field, but the hand-rolled transformer's return literal didn't list the new key, so callers read `undefined` even though the type said the field was there. This gate closes that gap. For each method in `src/client.ts` whose declared return type is one of a configured wire-bound set, it parses the method body via the TypeScript Compiler API and: - PASSTHROUGH (safe): `return this.orchestratorRequest<T>(…)` or any typed-cast call returns whatever the wire emits; new fields on T flow through naturally. - LITERAL RETURN: `return { id: x, name: y, … };` — checks every non-@deprecated property of T appears as a key in the literal. Missing keys are GAPS. - HELPER CALL: `return this.parseFoo(…)` — checked at the helper's definition site (recursive). - CONDITIONAL: ternary branches checked independently. - DYNAMIC: variable returns are not flagged (Phase 1 limitation). Properties marked @deprecated via JSDoc are skipped — they're intentionally not populated. Wire-shape coverage requires every non-@deprecated property to flow through the transformer. Baseline mechanism mirrors the wire-shape contract gate and the falsey-clobber lint: .lint_baselines/transformer_coverage.json captures the 3 pre-existing gaps on origin/main (executePlan + getPlanStatus on PlanExecutionResponse.policyInfo, resumePlan on 5 ResumePlanResponse fields). CI fails on any finding NOT in the baseline. Burndown via targeted PRs. Wired into the existing wire-shape-contract.yml workflow as a new step alongside the wire-shape validator, so it runs on every PR that touches src/**/*.ts. Validation: - On origin/main: 3 findings (the pre-existing gaps), all baselined — gate exits 0. - Inserting a new gap (e.g. dropping `success` from cancelPlan's return literal): gate exits 1 with file:line and the missing keys. Verified in the PR #185 post-fix worktree. Phase 1 covers READ-path object-literal returns. Phase 2 will extend to write-path (request body builders) and helper-call recursion. Cross-SDK status: - Python equivalent: planned next; pydantic's `Type.model_validate(...)` and `request.model_dump()` cover the typical paths but hand-rolled dict-builders need the same gate. - Go: not needed — struct tags + json.Marshal/Decode are declarative; new fields with `,omitempty` propagate both directions automatically. Documented in the rationale doc shipping alongside this gate's Python port. - Java: not needed — Jackson @JsonProperty annotations on @JsonCreator constructors auto-propagate. Same rationale.
CI-internal hardenings; no user-visible behaviour change. - refresh.js: `--sha` with no value used to be silently consumed as the positional `specsDir`, masking the missing argument and routing the script through the gitHeadSHA fallback. Now exits 2 with a clear message. - validate.js: an empty `registered_types` paired with non-empty cross_spec / intra_file / per_type_drift entries used to silently disable Gate 4 (rename-escape). The validator now treats this combination as a baseline corruption and fails the run. A genuinely fresh baseline (every section empty) still skips the gate as before, since the first-pin run has nothing to guard. - lib.js writeBaseline: a crash between writeFileSync(tmp, ...) and renameSync(tmp, BASELINE_PATH) used to leave a `.tmp.<pid>` sidecar, which a future writer with the recycled PID would collide on. Now wraps in try/catch + unlinkSync on any throw. - lib.js extractInterfaceProps: an `interface X extends Y` would silently under-report fields because we walk only iface.members. No such interface ships in the SDK today; the gate now warns loudly the first time one appears so coverage gets resolved before drift can hide. Full heritage walking is the proper fix if this becomes a routine pattern. Verifications: - refresh.js with `--sha` as last token exits 2; with `--sha foo` works. - validate.js with corrupted baseline (empty registered_types but populated drift/cross_spec) exits 1 with explanatory message. - writeBaseline temp-file is cleaned up on synthetic write error. - Real baseline regenerates byte-identical to origin/main.
Gate 1 (cross-spec divergence) only iterated currently observed schemas. A baselined divergence that the platform has since reconciled silently lingered in the baseline forever; the same old incompatible shape could be reintroduced and pass the gate because its fingerprint matched a stale entry that should have been deleted. Adds the reverse pass that Gate 2 already does for intra-file duplicates: any baselined cross-spec name that is no longer observed in the current specs fails the run with a pointer at the specific baseline key to delete. Mirrors the same fix landing on the Java arm in PR #137. Verified locally: - Positive run on clean baseline still exits 0. - Adding a phantom 'PhantomDivergence' entry to baseline cross_spec_duplicates causes the validator to exit 1 with a clear remove-from-baseline message naming the stale key.
The accidental git add -A in the prior commit swept up a session-export .txt file that lives in this worktree. Removing.
The TS validator was extracting in-memory property names (camelCase by TS idiom) and diffing them directly against OpenAPI snake_case wire names, generating ~22 false-positive drift entries (Category A in the audit) where the SDK's hand-rolled transformer in client.ts already correctly bridged the two names. The other three SDK validators don't have this issue: pydantic alias for Python, `json:"…"` struct tag for Go, `@JsonProperty(…)` for Java — each extracts the WIRE name natively. TS has no first-class equivalent. This change adds equivalent wire-name extraction for TS: - toSnakeCase() converts camelCase to snake_case with three boundaries (acronym→word, lowercase→uppercase, lowercase→digit). The digit boundary fixes ModelPricing.inputPer1k → input_per_1k, which the earlier categorization regex got wrong. - loadTransformerEvidence() reads src/client.ts once and collects every snake_case-shaped identifier. This is the bridge audit: a TS field is treated as 'bridged to its snake_case wire form' only when the snake form actually appears somewhere in client.ts. - canonicalizeWireNames() applies the conversion per-field. If the snake form is found in client.ts, the field is normalized; if not, the camelCase form is kept and the type is recorded as having unbridged fields. - The safety net: a type's unbridged fields are warned about ONLY when the same type has at least one OTHER field that IS bridged. If no field in the type is bridged, the type is almost certainly SDK-internal (configs, options, adapter metadata, etc.) and the whole type isn't wire-bound; warning would be noise. Effect: 22 Category A drift entries disappear from the baseline (AuditToolCallRequest, BudgetCheckRequest, FEATAssessment, KillSwitch, ModelPricing, PlanVersionEntry, PolicyMatch, etc.). 63 → 41 drift entries. The 19 unbridged-field warnings the validator now emits all line up with audit-flagged broken/orphan reads in client.ts (PolicyEvaluationResult.databaseAccessed, StaticPolicy.hasOverride, StepGateResponse.policiesMatched, etc.) — the validator is now correctly distinguishing 'bridged drift' from 'genuinely missing transformer'. No SDK property names changed. Wire behavior is unchanged. This is purely a validator measurement fix; the SDK's existing transformer architecture is preserved as-is. Per platform-side spec corrections needed (filed separately): - AISystemRegistry.materiality_classification (#1708) - DynamicPolicyInfo schema completely wrong (#1709)
…VERABLE)
Audit-driven cleanup against the wire-shape contract gate. All
changes are additive (new fields are optional). Where a TS field
was reading `undefined` because the decoder is JSON.parse passthrough
and the wire emits a different key, the wire-canonical name is added
and the orphan field is marked `@deprecated` (kept for compile-time
compat; removed in v7).
Per-type changes (full detail in CHANGELOG):
Pure additions (Cat B):
- WebhookSubscription.{secret, org_id, tenant_id} — security-critical;
`secret` is the HMAC-SHA256 signing key required to verify inbound
webhook payload signatures.
- StepGateRequest.{cost_usd, tokens_in, tokens_out}
- StepGateResponse.decision_id
- ListWorkflowsResponse.{limit, offset}
- StaticPolicy.{policy_id, priority, has_override}
- CreateStaticPolicyRequest.{priority, tags}
- UpdateStaticPolicyRequest.{priority, tags}
- UpdatePlanRequest.metadata
- UsageBreakdownItem.group_by
- BudgetAlert.acknowledged
- Budget.{org_id, tenant_id}
- WorkflowStatusResponse.metadata
- ExecutionSnapshot.retryCount
- Finding.article
RENAME_SAFE (orphan reads → wire-aligned canonical, old marked @deprecated):
- StepGateResponse.policies_evaluated/policies_matched (snake) +
policiesEvaluated/policiesMatched @deprecated
- StaticPolicy.has_override (canonical) + hasOverride @deprecated
- DynamicPolicyMatch.message + reason @deprecated
- ExfiltrationCheckInfo.{exceeded, limit_type} + within_limits @deprecated
- PolicyOverride.{id, enabled_override} + active @deprecated
- PolicyVersion.{id, policy_id, change_summary, snapshot} +
{changeDescription, previousValues, newValues} @deprecated
- CancelPlanResponse.success + message @deprecated
- CreateWorkflowResponse.started_at + {created_at, source} @deprecated
- UsageRecord.{created_at, success, error_message, latency_ms,
team_id, tenant_id, user_id, workflow_id} + timestamp @deprecated
- ResumePlanResponse.result + 5 declared-but-never-populated fields
marked @deprecated (workflowId, message, stepResult, nextStep,
nextStepName, totalSteps)
- PolicyEvaluationResult.{required_actions, processing_time_ms,
database_accessed} (snake) + camelCase forms @deprecated
UNRECOVERABLE realignments (no decoder; safe to add wire shape):
- EffectivePoliciesResponse: tier-stratified wire shape
({static, dynamic, tenant_id, organization_id, computed_at})
- Policy: rich wire shape (policy_id, category, tier, pattern,
severity, action, actions, conditions, description,
organization_id, tenant_id, created_at/_by, updated_at/_by,
version) — was exposing only 5 of 21 wire fields
- PlanResponse: wire top-level fields (success, version, result,
error, workflow_execution_id, policy_info)
Transformer updates:
- BudgetAlert decoder propagates acknowledged
- UsageBreakdownItem decoder propagates groupBy
- ExecutionSnapshot decoder propagates retryCount
Wire-shape baseline regenerated:
- 41 → 34 drift entries
- All Cat B types fully resolved (RESOLVED ✓)
- Remaining entries are all SDK-only fields (mostly @deprecated
aliases retained for type-compat + Plugin Batch 1 SDK additions
pending platform-side spec coverage)
Filed alongside (no SDK change needed; spec is wrong):
- axonflow-enterprise#1708 — AISystemRegistry.materiality_classification
- axonflow-enterprise#1709 — DynamicPolicyInfo schema wrong shape
Tests: 859 pass. Lint clean.
Companion to the type-definition sweep in this PR. The type updates added wire-canonical fields, but the hand-rolled transformers in client.ts weren't updated in parallel — so the fields were unreachable at runtime even though TypeScript types said they existed. The 5 reported gaps + 4 systematic-audit-found gaps = 9 fixes: Read-path fixes (decoders that dropped new wire fields): - generatePlan: PlanResponse now carries the wire top-level fields (success, version, result, error, workflow_execution_id, policy_info), sourced from agentResponse.* with data.* fallback. - resumePlan: ResumePlanResponse.result now populated from data.result. The deprecated `message` alias remains populated for back-compat. - getStaticPolicyVersions: inline-typed wire shape now declares the canonical fields (id, policy_id, change_summary, snapshot) alongside the legacy `change_description`/`previous_values`/ `new_values` keys; the transformer emits both halves. - listUsageRecords: UsageRecord transformer now emits the 8 new canonical fields (created_at, success, error_message, latency_ms, team_id, tenant_id, user_id, workflow_id). Legacy `timestamp` still emitted (it has always been undefined; both kept for source compat). - getExecution: inline step shape now declares retry_count, and the per-step mapper emits retryCount. (getExecutionSteps was already correct; only this method was out of sync.) - mapBudgetResponse: emits tenant_id, org_id (used by every Budget- returning method). - mapFindingResponse: emits article (used by mapAssessmentResponse). Write-path fixes (request builders that dropped new SDK fields): - updatePlan: body now includes metadata when set on the request. - createStaticPolicy: requestBody now includes priority, tags when set on the request. Verified clean (no gap): updateStaticPolicy (passthrough; field names match wire), stepGate (passthrough; new tokens_in/tokens_out/ cost_usd field names match wire), every method that flows through orchestratorRequest<T>/policyRequest<T> with a typed cast (the JSON parses naturally into the typed shape, so new fields appear without explicit transformer code). Meta-pattern observed: any method with a hand-rolled mapper helper or an explicit return-literal builder is a candidate gap when types gain fields. Methods that pass through to orchestratorRequest<T> inherit the new fields for free. The audit covered every type touched in the sweep across both directions. Tests: 859 pass. Lint clean.
Same class as the 5 transformer gaps fixed in commit 2fac16f. The type-update added CancelPlanResponse.success but the cancelPlan() method's return literal didn't include it. The deprecated `message` slot is also kept populated so existing callers that read it see no behavior change (it has always been undefined against current servers, but zero-clobber). The systematic audit clearly missed cancelPlan in the previous round. The expanded sweep below documents every type/method pair to keep the contract honest: Read-path verifications (every method that returns a touched type): - BudgetAlert via getBudgetAlerts: acknowledged emitted ✓ (round 2) - UsageBreakdownItem via getUsageBreakdown: groupBy emitted ✓ (round 2) - ExecutionSnapshot via getExecutionSteps + getExecution: retryCount emitted ✓ (round 2) - PolicyEvaluationResult: embedded in PlanResponse.policy_info populated by generatePlan ✓ (round 2) - PlanResponse via generatePlan: 6 wire top-level fields ✓ (round 2) - CancelPlanResponse via cancelPlan: success now emitted (this commit) - ResumePlanResponse via resumePlan: result emitted ✓ (round 2) - StaticPolicy via list/get/create/update/toggle/getEffective: passthrough through policyRequest<T> ✓ - PolicyOverride via createPolicyOverride/listPolicyOverrides: passthrough through policyRequest<T> ✓ - PolicyVersion via getStaticPolicyVersions: id, policy_id, change_summary, snapshot emitted ✓ (round 2) - WorkflowStatusResponse via getWorkflow: passthrough ✓ - ListWorkflowsResponse via listWorkflows: passthrough ✓ - WebhookSubscription via createWebhook/getWebhook/updateWebhook: passthrough ✓ - ExfiltrationCheckInfo, DynamicPolicyMatch: embedded in MCP response shapes; passthrough ✓ - Finding via mapFindingResponse: article emitted ✓ (round 2) - Budget via mapBudgetResponse (used by createBudget/getBudget/ listBudgets/updateBudget/checkBudget/getBudgetStatus): tenant_id + org_id emitted ✓ (round 2) - UsageRecord via listUsageRecords: 8 wire fields emitted ✓ (round 2) - StepGateResponse via stepGate: passthrough; decision_id + policies_evaluated + policies_matched flow through ✓ - CreateWorkflowResponse via createWorkflow: passthrough; started_at flows through ✓ Write-path verifications (every method that accepts a touched request type): - StepGateRequest via stepGate: JSON.stringify passthrough; new cost_usd / tokens_in / tokens_out names match wire ✓ - UpdatePlanRequest via updatePlan: metadata in body builder ✓ (round 2) - CreateStaticPolicyRequest via createStaticPolicy: priority + tags in body builder ✓ (round 2) - UpdateStaticPolicyRequest via updateStaticPolicy: passthrough; field names match wire ✓ Falsey-clobber audit: 0 || patterns introduced in this PR's client.ts diff (verified with git diff grep). The transformer code uses ?? semantics or explicit guards. Tests: 859 pass. Lint clean.
…wn resumePlan PR #185's transformer fix populated all 5 fields previously missing on ResumePlanResponse (nextStep, nextStepName, stepResult, totalSteps, workflowId). Drop the stale baseline entry; remaining entries are executePlan and getPlanStatus on PolicyEvaluationResult, untouched this round.
saurabhjain1592
force-pushed
the
fix/wire-shape-review-followups
branch
from
df3ed2b to
7f400b9
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to merged #182 (manifest version-alignment) and #184 (QF-12 wire-shape contract). Deep code review on both PRs surfaced four real gaps; this PR closes all four.
CI-internal hardenings. No user-visible behaviour change.
1. `refresh.js` — `--sha` with no value silently dropped
`node refresh.js /tmp/specs --sha` used to silently consume `--sha` as the positional `specsDir` (then fail later with a confusing "is not a directory" error), masking the missing argument. Now exits 2 with `error: --sha requires a value`.
2. `validate.js` — empty `registered_types` silently disables Gate 4
The previous `if (baseline.registered_types.length > 0)` meant an emptied list (whether by bug, malicious edit, or a refresh.js regression) would silently turn off the rename-escape guard. The validator now distinguishes:
3. `lib.js writeBaseline` — `.tmp.` sidecar leak on crash
A failure between `writeFileSync(tmp, ...)` and `renameSync(tmp, BASELINE_PATH)` used to leave the partial tempfile behind. The next writer with a recycled PID would collide. Now wraps in try/catch + `unlinkSync` on throw.
4. `lib.js extractInterfaceProps` — `interface X extends Y` silently under-reports
Walking only `iface.members` misses members inherited via heritage clauses. The TS SDK doesn't currently have any extending wire interfaces, so this is forward-defence — when the first one lands, the validator now `console.warn`s loudly with a pointer at the function that needs to plumb in heritage walking. Better than silent under-counting.
Verifications (local)
Out of scope (deliberately)
Test plan