feat!: realign types with Tako's current API and gate drift in CI

[25eliu]

Why

@takoviz/ai-sdk was last updated 2026-06-28 and the Tako API moved on since. Six fields in src/types.ts no longer describe the live API — one of them makes any request that uses it fail outright.

Every claim below is verified two ways: against Tako's published OpenAPI document, and against tako-sdk, the official generated client built from that same document. tako-sdk is a devDependency, so it acts as a test-only oracle and this package keeps zero runtime dependencies.

What was wrong

	Problem	Symptom
1	sources.data.deferDataRetrieval no longer exists	The API's data-source settings forbid unknown properties, so setting this option failed the entire request
2	contents_total_cost removed	Read undefined at runtime while TypeScript promised number
3	ResultContent.format → content_format, enum changed	.format always undefined. 'text' is gone, so branching on it silently took the wrong path
4	Source taxonomy is now "data" | "web"	source_index === 'tako' compiles and never matches. Two exported types described shapes the API does not send
5	cards / web_results / contents typed as guaranteed	Not in the API's required list, so a valid response may omit them and result.cards.length would throw
6	Five card fields were dropped	exportable, data_freshness, relevance_score, nodes, metric_definitions arrived over the wire and were invisible to consumers

Note the severity spread: nothing breaks for code that calls takoSearch() and reads cards[i].title. That is why the existing suite stayed green — all 18 tests passed while asserting the wrong wire shape.

Preventing recurrence

The types were hand-maintained with nothing checking them against the API. That is now closed by tests/contract/:

• request.contract.test.ts — every request body the SDK builds is validated against the spec's SearchRequest / ContentsRequest via ajv. Those schemas set additionalProperties: false, mirroring the server's extra="forbid".
• response.contract.test.ts — spec facts (property names, enums, required lists) plus the official client's decoders as an independent second oracle.
• types.conformance.ts — assigns tako-sdk's generated types into this SDK's public types and back, and compares key sets in both directions. Must compile clean. Assignability alone missed drift by a whole key (deleting data_freshness compiled clean with every contract test green), so the key-symmetry check is what actually gates the card fields.

pnpm test already runs on every PR. Also adds pnpm test:contract and pnpm spec:refresh.

Behaviour improvements, not just types

• exportable is now actionable. exportable: false means a /contents call is a guaranteed 403. examples/contents.ts filters on it and reports what it skipped; the previous try-every-URL loop existed only because the flag was invisible. Both tool descriptions teach the model the same rule.
• Tool descriptions rewritten — an intentional behaviour change beyond the wire realignment, called out here so it is attributable rather than buried in a types PR. A routing eval is a sensible follow-up. Changes cover: when to use search vs answer, query shape (one entity + one metric), traffic keyed by domain, and where to get figures for non-exportable cards. takoContents's description now varies with mode, so the model knows whether it will receive data or a download link.
• takoContents validates its url input, so a malformed value fails locally instead of costing a priced round trip.
• Corrected a documented limit: inline contents were described as "capped at 1000 rows". The real behaviour is a 20-row default against a 2,000-row ceiling.

Live verification

Verified against the real API. All six fixes hold, and every live response validates against the vendored spec through ajv (SearchResponse ×3 variants, AnswerResponse, ContentsResponse ×2 modes, BaseAPIError).

Claim	Live result
1 defer_data_retrieval is rejected	HTTP 400 — "sources.data.defer_data_retrieval: Extra inputs are not permitted". The removal was load-bearing
3 new payload shapes are real	includeContents returned content_format: "json_compact" with a full TakoDataset (typed columns, positional rows) — a response the old format: 'csv' | 'text' could not represent
4 taxonomy is data / web	Only "data" observed across all cards
6 exportable predicts export outcomes	false → 403 "Data export is not available for this card (protected source)"; true → 200
Row-cap correction	export_pricing returned free_rows: 20, max_rows_ceiling: 2000

Two documentation claims did not survive live testing and are corrected in f6697cb:

1. usage is not populated. contents_total_cost is confirmed gone, but the spec's replacement was absent from every response tested — plain search, deep search, includeContents search, answer, and both contents modes. Pointing users at usage?.total_cost_usd was wrong; it always reads undefined. Per-item content.cost and content.export_pricing are populated and are where pricing lives today. The field stays typed as optional so it works if Tako begins emitting it.
2. Empty collections are sent, not omitted. The API returns cards: [] on a web-only search, so the always-present typing was a latent hazard rather than an active crash. The contract still permits omission and tako-sdk decodes an absent collection to undefined, so the optional wire type and the normalizer both stay.

Neither correction changes a type in this PR; both change what the docs tell people to do.

Also found while reviewing this PR

• content_format === null is fragile. The field is optional as well as nullable, and the official client maps both absent and explicit-null to undefined. Live testing shows the API currently sends explicit null, so this was latent rather than broken — but the loose == null form is correct against the contract, and both shapes are now tested.
• methodology_name / methodology_description are required-but-nullable, not optional. The conformance check caught this; all mirrored schemas were then audited programmatically for the same pattern, and this was the only instance.

Scope

This PR is limited to wire correctness. Further improvements to the SDK's surface are tracked separately.

Three items raised in review are deferred rather than folded in: a scheduled job to refresh the two pinned references (both are snapshots, so the suite gates regressions here rather than detecting upstream changes), a test/typecheck gate on the release-please publish job, and a routing eval for the rewritten tool descriptions.

callTako still performs no response-shape validation, so a 200 with an unexpected body yields request_id: undefined. Pre-existing and unchanged here.

Test plan

• pnpm test — 52 passed (previously 18, all of which passed against the wrong wire shape)
• pnpm typecheck clean
• pnpm build clean; dist/ contains zero references to tako-sdk
• dependencies: {} — zero runtime dependencies preserved
• Public export surface verified 1:1 (38 types exported, 38 defined)
• Enum completeness: TakoSourceIndex and TakoContentFormat traced through every consumer in src/, tests/, examples/
• Live verification against the real API (results above)

---

Release-please will cut 3.0.0 from the BREAKING CHANGE: footer. MIGRATING.md has the field-by-field guide with before/after code.

[robertabbott]

Does this pass when tsc fails to launch, rather than reporting diagnostics? tsc writes diagnostics to stdout, but a launch failure writes to stderr and leaves stdout empty — so err.stdout ?? "" returns "" and expect("").toBe("") passes.

I hit exactly that running the suite locally (installed pnpm rejecting the local node version):

✓ tests/contract/types.conformance.test.ts (1 test) 250ms
stderr: "ERROR: This version of pnpm requires at least Node.js v22.13"

Reported green in 250ms with tsc never invoked — against the 60s budget the comment below sets for "a cold tsc run". Since this is the one test carrying the whole drift claim, would it be worth keying the return on the child's exit status and surfacing stderr — something like if (typeof err.code === "number" && err.stdout) return err.stdout; throw err — so "could not check" is loud instead of indistinguishable from "no drift"?

[25eliu]

Fixed — and you were right that it was silent. Reproduced your case with a bad PATH: err.code is a string (ENOENT) or stdout is empty, both fell through to "" and passed.

Now requires a numeric exit code and non-empty stdout to treat output as diagnostics; anything else throws with exit code and stderr. Verified both branches: a non-existent binary rejects with /did not run/, and a real clean run still returns "". Given this test carries the whole drift claim, "could not check" needed to be loud.

[robertabbott]

Assigning official into ours proves our type is not over-strict. Does it also catch the two directions where ours and the API disagree by a whole key?

I regressed each of the six findings against tsc -p tsconfig.conformance.json on this branch:

Class	Regression applied	Gate
P0-2	contents_total_cost: number back on TakoSearchResponse	caught (TS2741)
P0-4	widen TakoSourceIndex with "tako"	caught (TS2322, via the …Back assignment)
P0-5	make cards required on the wire type	caught (TS2322)
P0-3	stale optional format?: "csv" | "text" on TakoResultContent	exit 0
P0-6	delete data_freshness + relevance_score from TakoCard	exit 0, all 26 contract tests green

Excess-property checks don't fire on a declare const, and an absent optional key is assignable — so a field the API sends that src/types.ts omits, and a field the API stopped sending that lingers here, both slide through. Those are the two silent classes, and P0-6 accounts for five of the card fields this PR adds. The TakoCard — P0-6 test in response.contract.test.ts doesn't close it either: it asserts the vendored yaml carries those props, not that src/types.ts declares them.

Would a key-set symmetry check be worth adding here? I tried it and it names the offender:

type KeyDiff<Ours, Official> =
  | Exclude<keyof Official, keyof Ours>
  | Exclude<keyof Ours, keyof Official>;
type SameKeys<Ours, Official> = [KeyDiff<Ours, Official>] extends [never]
  ? true
  : { KEY_DRIFT: KeyDiff<Ours, Official> };

export const cardKeys: true = true as SameKeys<TakoCard, OfficialTakoCard>;

error TS2322: Type '{ KEY_DRIFT: "data_freshness"; }' is not assignable to type 'true'.

I checked all eight mirrored types (TakoCard, the three responses, ContentItem, ResultContent, WebResult, TakoDataset) — every key set is already exactly symmetric, so this passes as-is with no other change.

[25eliu]

This was the most valuable comment on the PR — you were right, and I reproduced both classes before fixing.

Deleting data_freshness + relevance_score from TakoCard: tsc exit 0, all 26 contract tests green. Stale optional format?: "csv" | "text": also exit 0. So five of the six card fields this PR adds were ungated, which undercut the central claim.

Adopted your SameKeys check across all eight mirrored types. Both regressions now fail and name the key ({ KEY_DRIFT: "data_freshness" }, { KEY_DRIFT: "format" }). Confirmed your finding that all eight were already symmetric — it compiles clean with no other change.

[robertabbott]

Should this be == null? As written, the guide teaches the bug the PR description reports catching in review (finding 1) — src/types.ts says "Test it loosely (content_format == null), never with === null", README.md's table row says "null or absent", and examples/contents.ts:43 uses == null.

The prose at line 43 ("web page text is now signalled by content_format === null") has the same problem. MIGRATING.md is the one file a 2.x user reads while writing their 3.0 code, so this is the highest-leverage place for the loose form.

[25eliu]

Fixed both lines. Teaching === null in the migration guide while the PR body claimed credit for catching that exact bug was the worst version of this — and you are right that MIGRATING.md is the highest-leverage place, since it is the file someone reads while writing their 3.0 code.

Line 43 prose now says web text is signalled by the absence of a format and to test with == null; the snippet at 51 uses == null.

[robertabbott]

What refreshes the two oracles? openapi.yaml is vendored and only moves when someone runs this script by hand, and tako-sdk is pinned to exactly 1.1.10 in pnpm-lock.yaml with CI running --frozen-lockfile. I don't see a dependabot/renovate config or a scheduled workflow.

That makes the suite a regression test against the 2026-08-03 snapshot rather than a drift detector — it can only fail after a human already suspected drift and refreshed. The claim in the PR body, MIGRATING.md:7 and the README ("API drift fails CI rather than shipping") reads as continuous. Worth either a scheduled job that runs spec:refresh + bumps tako-sdk and opens a PR, or softening the wording to what it actually guarantees?

Separately: release-please.yml's publish job runs install/build/publish with no test or typecheck, so the gate is absent at the moment a version actually ships. Is that intentional given ci.yml covers the PR?

[25eliu]

You are right on both counts, and I did the honest half now while leaving two decisions open.

Wording — fixed. "API drift fails CI rather than shipping" overclaimed. README, MIGRATING.md and CONTRIBUTING.md now say a type that stops matching either reference fails CI, and that both are pinned snapshots refreshed deliberately. CONTRIBUTING gained a short section on how to refresh them. So the suite is accurately described as a regression gate on this repo, not a continuous upstream detector.

Scheduled refresh — not done, deferred to the repo owner. A weekly job running spec:refresh + pnpm update tako-sdk --latest and opening a PR on any diff is the right shape and would make the original claim true, but adding a scheduled workflow is beyond a wire-correctness PR and wants a decision on cadence and who triages the noise.

release-please publish gate — also deferred, and I think you found a real hole. release-please.yml's publish job runs install/build/publish with no test or typecheck, so nothing gates the moment a version actually ships. ci.yml covers the PR, but the release commit lands on main and publishes without re-verification. Adding the two steps is cheap insurance; I have deliberately not touched the publish pipeline unilaterally.

Both flagged to the repo owner rather than folded in here.

[robertabbott]

Since the body is reproduced here rather than built by the code under test, what fails if src/tools/contents.ts:52 changes shape — renames mode, or adds a field? The search cases call buildSearchRequestBody, so they're anchored to the implementation; these two aren't. Would extracting a buildContentsRequestBody helper (or importing the tool and reading fetch's body, as tests/contents.test.ts already does) make these load-bearing?

There's a second gap stacked on this one: ContentsRequest is the only request schema in the spec that does not declare additionalProperties: false — SearchRequest, Sources, DataSourceSettings, WebSourceSettings, OutputSettings and GeoLocation all do. So check("ContentsRequest", …) accepts unknown properties, and the header comment here (and spec.ts:29) generalizes "Request schemas declare additionalProperties: false" past where it holds. If the server does forbid extras on this endpoint too, that's a spec bug worth adding to the follow-ups list; if it doesn't, the P0-1 class of failure is ungated on the contents surface in both directions.

[25eliu]

Both parts fixed, and I verified the second one — ContentsRequest is indeed the only request schema without additionalProperties: false:

SearchRequest false / Sources false / DataSourceSettings false
WebSourceSettings false / OutputSettings false / GeoLocation false
ContentsRequest undefined

Changes: extracted buildContentsRequestBody(url, mode) into src/request.ts, now used by both the tool and this test, so a body-shape change breaks the test. Since ajv cannot catch extras on that schema, added a direct assertion that the body emits only schema-defined properties, plus a test pinning the asymmetry so it fails if Tako tightens the schema. Corrected the header comment here and spec.ts to stop generalizing past where it holds.

Flagging the spec asymmetry to Tako as a possible upstream gap — leaving that with the repo owner.

[robertabbott]

Is "Renamed" the whole story? The alias's values changed as well — 2.x had "tako" | "web" | "connected_data" | "tako_deep_v2", and this now resolves to "data" | "web". Someone who kept importing TakoCardSourceIndex to avoid the rename gets a silently different union (which at least fails to compile on === "tako", so the outcome is good), but the doc comment tells them nothing moved. Worth "Renamed to {@link TakoSourceIndex}; the value set also collapsed to data | web"?

Related: MIGRATING.md:73 says "source_index is also a plain string now" — it's this two-member union, and required.

[25eliu]

Fixed. The alias now reads: renamed to TakoSourceIndex, and the value set collapsed from "tako" | "web" | "connected_data" | "tako_deep_v2" to "data" | "web", with a note that comparisons against the removed values no longer compile.

Also corrected MIGRATING.md — "a plain string now" is now "the required two-member union data | web".

[robertabbott]

Should this row carry the correction the prose below it makes? 868dac2 updated the surrounding text to say usage is never populated, but the table still presents result.usage?.total_cost_usd as the migration target — and the table is the part people skim. Something like **No replacement.** Read per-item content.cost/content.export_pricing (see below) would keep the two consistent.

[25eliu]

Fixed — the row now reads No replacement. Read per-item content.cost / content.export_pricing. You are right that the table is the skimmed part; leaving it pointing at usage after the prose was corrected was the worst of both.

[jed326]

Finding: This sentence claims the API "omits empty collections", but the PR's own live verification found the opposite — the API returned cards: [] on a web-only search. Omission is contract-permitted, not observed behavior.

Evidence: PR body, correction 2 after live verification: "Empty collections are sent, not omitted." MIGRATING.md already carries the corrected wording ("the contract permits omission"); this README sentence is the one place the correction didn't reach.

Proposed fix:

Suggested change

	The API guarantees only `request_id` and omits empty collections, so the tools normalize: `cards`, `web_results` and `contents` are **always arrays**. No `?.` needed.
	The API guarantees only `request_id` — the contract permits omitting the collections — so the tools normalize: `cards`, `web_results` and `contents` are **always arrays**. No `?.` needed.

[25eliu]

Fixed with your suggested wording. MIGRATING.md had the corrected phrasing and this was the one place it did not reach.

[jed326]

Finding: This prints downloaded.usage?.total_cost_usd ?? 0, but the PR's live verification found usage is not populated on any endpoint — so this line prints 0 on every run.

Evidence: PR body, correction 1: "usage is not populated... it always reads undefined. Pricing actually lives in the per-item content.cost and content.export_pricing." The example already reads item.cost six lines up, so it demonstrates both the right pattern and the dead one.

Proposed fix: Drop this line, or replace the aggregate with the per-item field the API does populate:

Suggested change

	console.log("Request cost (USD):", downloaded.usage?.total_cost_usd ?? 0);
	console.log("Item cost (USD):", item.cost ?? 0);

[25eliu]

Fixed — now prints item.cost, which the API does populate. You are right that the example demonstrated the right pattern six lines up and the dead one here.

[jed326]

Finding: The description is fully rewritten with new routing policy — "one entity + one metric per query", "traffic data is keyed by domain", and "use the answer tool instead" for single figures — beyond what the wire-contract realignment required (teaching exportable and correcting the row cap). Same applies to the answer and contents tool descriptions.

Evidence: The base description contained none of this routing guidance; the new text changes which tool a model picks and how it phrases queries, and no routing eval accompanies the change.

Proposed fix: No code change requested — confirm the rewrite is deliberate, note it in the PR body as a behavior change, and consider a follow-up routing eval so a regression here is attributable.

[25eliu]

Confirmed deliberate — the descriptions were rewritten intentionally by the repo owner during review of this PR, not incidentally by the type work. Your read is correct that it goes beyond what the realignment required.

No code change, but I have noted it in the PR body as an intentional behaviour change so it is attributable rather than buried in a types PR. Agreed a routing eval is the right follow-up: without one, a regression in tool selection has nothing to bisect against. Leaving that with the repo owner.

For the record, the parts that were required by this PR are the exportable guidance and the row-cap correction; the routing policy ("one entity + one metric", traffic keyed by domain, prefer answer for a single figure) is the additive part.

[jed326]

Finding: This fixture was normalized to staging.tako.com, but the equivalent fixture in tests/search.test.ts (the "honors baseUrl override" test, ~line 88) still uses staging.trytako.com — the PR normalizes the host in one file and leaves the other.

Evidence: tests/search.test.ts:88-90 at this commit: baseUrl: "https://staging.trytako.com/" and the matching assertion. Both are arbitrary override strings, so behavior is unaffected; the inconsistency is cosmetic but survives from the very host cleanup (#5) that standardized on tako.com.

Proposed fix: Update the search.test.ts fixture to staging.tako.com for consistency.

[jed326]

Also, I'm not sure if we should commit the staging endpoint anywhere publicly at all

[25eliu]

Fixed — search.test.ts now uses staging.tako.com to match.

[jed326]

Finding: Comments in this public repo describe Tako backend internals: "generated from the Tako monorepo" (here), "released from the Tako monorepo" (response.contract.test.ts:17), and "the server's Pydantic extra=\"forbid\"" (line 29-30 below, and request.contract.test.ts:10-12).

Evidence: None of these facts are needed to understand the tests, and none are stated in the published spec or docs — the observable contract (additionalProperties: false → the API rejects unknown properties with a 400) carries the full meaning.

Proposed fix: Rewrite in observable terms, e.g. here: "vendored verbatim from https://docs.tako.com/api-reference/openapi.yaml — the authoritative description of the live API"; and for the Pydantic references: "additionalProperties: false means the API rejects unknown properties (400), not silently ignores them."

[25eliu]

Fixed, and this one matters beyond tidiness — this is a public repo, so those comments were leaking internal architecture for no explanatory benefit. Rewritten in observable terms as you suggested:

• spec.ts: vendored verbatim from the published URL, "the authoritative description of the live API"
• the extra="forbid" references: now "the API rejects unknown properties with a 400 rather than ignoring them"
• response.contract.test.ts: dropped the monorepo reference

Swept the rest of the diff for the same class and found no others.

[jed326]

Finding: Backend-internals reference in a public repo comment ("the server's Pydantic extra=\"forbid\"") — see the comment on tests/contract/spec.ts:11 for the full list and suggested rewording.

[25eliu]

Fixed — see the reply on spec.ts:11. This comment now describes the observable behaviour (400 on unknown properties) rather than the server implementation.

[jed326]

Finding: Backend-internals reference in a public repo comment ("released from the Tako monorepo") — see the comment on tests/contract/spec.ts:11 for the full list and suggested rewording.

[25eliu]

Fixed — monorepo reference dropped; it now just says tako-sdk is generated from the same document.

[jed326]

Finding: This suite never exercises the normalizer's guarantee for the answer tool — the fixture always supplies cards: [] and web_results: [], so a bare { answer, request_id } response (which the contract permits) is untested here.

Evidence: tests/search.test.ts ("normalizes absent collections") and tests/contents.test.ts both cover the absent-collections case for their tools; answer.test.ts has a single test and it pre-fills the collections.

Proposed fix: Add one test that stubs { answer: "x", request_id: "r" } and asserts res.cards and res.web_results are [] — three lines, closes the one normalizer path with no direct coverage.

[25eliu]

Added. Stubs { answer: "x", request_id: "r" } and asserts both collections normalize to []. You were right that answer was the one normalizer path with no direct coverage.

[jed326]

Finding: CONTRIBUTING.md was not updated for the new scripts — it still lists only pnpm test / typecheck / build, and doesn't mention that pnpm test now includes a cold tsc run via the conformance test (the slowest test in the suite).

Evidence: CONTRIBUTING.md:6-9 at this commit lists the three original commands; test:contract and spec:refresh appear nowhere in it.

Proposed fix: Add the two scripts to CONTRIBUTING.md with one line each, plus a sentence noting the conformance test shells out to tsc so a slow first run is expected.

[25eliu]

Added both scripts to CONTRIBUTING.md, plus the note that pnpm test shells out to a cold tsc so a slow first run is expected.

Also added a short section on refreshing the two pinned references (pnpm spec:refresh + a tako-sdk bump), which ties into your package.json:24 comment about the oracles being snapshots.

[jed326]

Finding: "Tako's API moved on between June and August 2026 and this SDK's types did not follow" is vendor self-criticism in a public, consumer-facing migration doc.

Evidence: This is the first sentence a 2.x user reads. The factual content (types realigned; old code relying on removed fields was already broken at runtime) is fully carried by the rest of the paragraph.

Proposed fix:

Suggested change

	Tako's API moved on between June and August 2026 and this SDK's types did not follow. 3.0 realigns them. Every change below is a case where 2.x described something the API no longer does — so if code depended on it, it was already broken at runtime, whatever TypeScript said.
	3.0 realigns this SDK's types with the current Tako API. Every change below is a case where 2.x described something the API no longer does — so if code depended on it, it was already broken at runtime, whatever TypeScript said.

[25eliu]

Fixed with your suggested wording. Agreed — a consumer-facing migration doc is the wrong place for it, and the paragraph carries the factual content without it.

[25eliu]

Addressed all 17 comments from this round. Two were test-integrity bugs I could reproduce — the conformance test passed when tsc never launched, and one-directional assignability missed key-level drift (deleting data_freshness compiled clean with 26 tests green). Both now gated. The rest were accuracy fixes, including MIGRATING.md teaching the === null bug this PR claimed to catch.

Three items deferred to the repo owner rather than folded in: a scheduled job to refresh the two pinned references, adding a test/typecheck gate to the release-please publish job, and a routing eval for the rewritten tool descriptions. Reasoning is on each thread.

54 tests, CI green. @claude-address

[jed326]

Round 2 — verified the fixes against e21eac7..e85ba81. The tsc-launch gate is genuinely correct: I traced every branch, and the ENOENT-as-string case (the easy one to get wrong) lands on throw, with stderr surfaced. buildContentsRequestBody is a real extraction with no surviving parallel body, and the == null, usage table row, alias JSDoc, bare-response test, CONTRIBUTING and MIGRATING items are all fully closed.

Four things left, on the threads below. One is a real gap in the key-symmetry fix; two are sweeps that reached the docs but not src/types.ts, where the text ships to consumers.

One item has no line to attach to — the PR description itself. Under "Preventing recurrence" it still reads: "Those schemas set additionalProperties: false, mirroring the server's extra=\"forbid\"." The reply on the earlier thread said the rest of the diff was swept for that class — true of the tree, but the description is equally public on this repo. Worth the same rewording used in the test comments ("the API rejects an unknown property with a 400").

While checking that: the new "lone schema" claim is not accurate either. Of the 89 schemas in the vendored spec, 6 declare additionalProperties: false (SearchRequest, Sources, DataSourceSettings, WebSourceSettings, OutputSettings, GeoLocation), and 4 of the 5 request-named schemas lack it — ContentsRequest, AnswerAgentRunRequest, CreateCardRequest, RetrievalAgentRunRequest. The claim holds only if scoped to the two request bodies this SDK sends, which neither the test name nor the header comment says. Not blocking; flagging since the fix replaced one over-generalization with a narrower one.

Also minor: the test plan still says "52 passed" against the 54 in your summary comment.

[jed326]

Finding: The key-symmetry check closes both regression classes at the 8 top-level types, but every nested mirrored type is still ungated — and the same two classes slide through one level down.

Evidence: Concretely, delete last_updated from TakoDataFreshness (src/types.ts:249-254) and this file still compiles clean — exit 0, 54 tests green. OfficialDataFreshness stays assignable to the narrowed type, and the parent TakoCard check passes because keyof TakoCard is unchanged. That is the P0-6 regression displaced exactly one level, sitting under data_freshness — the field this fix was written to protect.

Two more that also exit 0: adding a stale format?: "csv" | "text" to TakoCardSource or TakoUsage (P0-3 class — an absent optional key is assignable), and deleting max_rows_ceiling from TakoExportPricing.

response.contract.test.ts:170 does not close this: expect(propertiesOf("DataFreshness")).toEqual([...]) asserts the vendored spec carries both keys, not that src/types.ts declares them — the same objection raised against the original TakoCard — P0-6 test.

Proposed fix: Extend SameKeys to the nested pairs. I dumped both sides against the vendored spec and the key sets are already symmetric for every one I checked — TakoDataFreshness/DataFreshness (data_as_of, last_updated), TakoCardSource/TakoCardSource (5 keys), TakoCardNode/TakoCardNode (4), TakoMetricDefinition/MetricDefinition (2), TakoUsage/Usage (3), TakoUsageCompute/UsageCompute (1), TakoUsageData/UsageData (2), TakoExportPricing/ExportPricing (4), TakoColumnDescriptor/ColumnDescriptor (5), TakoKnowledgeCardMethodology/KnowledgeCardMethodology (2) — plus TakoDatasetColumn and TakoDatasetSource. So the extension should compile clean with no other change; it needs the corresponding tako-sdk imports added at the top (I could not confirm the exact exported names locally, but the generator follows the schema names).

[25eliu]

Reproduced and fixed. Deleting last_updated from TakoDataFreshness compiled clean with all 27 contract tests green, exactly as you described — and you are right that it is the P0-6 regression sitting directly under the field the previous fix was written to protect, which makes it the worst possible place for a hole.

Extended SameKeys to all twelve nested pairs. Your read was correct that every one is already symmetric, so it compiles clean with no other change. Confirmed the official export names all exist as interfaces in tako-sdk before wiring them up.

Verified four regressions now fail and name the key:

Regression	Result
delete last_updated from TakoDataFreshness	KEY_DRIFT: "last_updated"
stale format? on TakoCardSource	KEY_DRIFT: "format"
delete max_rows_ceiling from TakoExportPricing	KEY_DRIFT: "max_rows_ceiling"
stale format? on TakoUsage	KEY_DRIFT: "format"

Agreed on your point about response.contract.test.ts:170 — asserting the vendored spec carries both keys says nothing about what src/types.ts declares. The key check is what actually closes it.

[jed326]

Finding: This JSDoc still states the behavior the PR's own live verification disproved — and it is the copy with the widest reach.

Evidence: "the collections are absent rather than empty when there is nothing to report". README.md:108 was corrected this round to "the contract permits omitting the collections", and MIGRATING.md:83 says the API sends cards: []. This one sits on an exported interface, so it ships in dist/index.d.ts and surfaces in consumer IntelliSense. Same claim at src/request.ts:88 ("omits collections rather than sending them empty").

Proposed fix: Mirror the corrected wording in both places — the contract permits omission, the API currently sends empty arrays, and the tools normalize either way.

[25eliu]

Fixed. You were right that this was the highest-reach copy — verified the corrected wording now appears in the built dist/index.d.ts, so it reaches consumer IntelliSense rather than just the repo.

Both places now say the contract permits omission while the API currently sends empty arrays, matching README and MIGRATING.

[jed326]

Finding: Second copy of the disproven empty-collections claim — "omits collections rather than sending them empty". See the comment on src/types.ts:286 for the evidence and fix; that one is higher priority since it ships in the published .d.ts.

[25eliu]

Fixed alongside the types.ts copy — same wording.

[jed326]

Finding: staging.tako.com is a live, internet-reachable pre-production environment, committed as a fixture in a public repo.

Evidence: It resolves (Cloudflare) and responds — root returns 200, and POST /api/v3/search returns 401, not 404, so the repo confirms the environment exists and is reachable. Nothing is exploitable from this alone and the subdomain is guessable, but it is gratuitous: this test only needs some base URL to prove the override and trailing-slash handling. Three occurrences across two files (here at :88 and :90, plus tests/types.test.ts:114).

My error, and the correction: my earlier comment asked for host consistency with tests/types.test.ts, which is what prompted normalizing onto staging.tako.com — taking it from one occurrence to three. Consistency was the wrong ask. The right fix is to drop the staging host entirely.

Proposed fix: Use a placeholder. These same test files already use https://e.com for web-result fixtures, so there is in-repo precedent:

Suggested change

	const t = takoSearch({ apiKey: "key", baseUrl: "https://staging.tako.com/" });
	const t = takoSearch({ apiKey: "key", baseUrl: "https://e.com/" });

and update the assertion on :90 to match. Same swap at tests/types.test.ts:114. (tests/client.test.ts:11,19,30,38 uses trytako.com — pre-existing and outside this diff, but the same cleanup if you are touching it.)

[25eliu]

Fixed, and thank you for correcting your own earlier ask rather than letting it stand — normalizing onto staging.tako.com is exactly what I did in response to it, so this went from one occurrence to three on my change.

Swapped all three to the https://e.com placeholder, using the in-repo precedent you pointed at. Also took trytako.com in tests/client.test.ts while in the area, even though it is outside the diff. No real Tako host remains in any fixture:

grep -rn "staging\.\|trytako" tests/ src/ examples/   →   none

The tests still prove what they were there to prove: the baseUrl override and trailing-slash stripping.

[jed326]

Finding: Third occurrence of the live staging host — see the comment on tests/search.test.ts:88 for evidence and the suggested placeholder.

Proposed fix:

Suggested change

	baseUrl: "https://staging.tako.com",
	baseUrl: "https://e.com",

[25eliu]

Fixed — same https://e.com placeholder.

[25eliu]

Addressed 5 comments from this round.

The substantive one: key symmetry only covered the eight top-level types, so both drift classes were still live one level down — deleting last_updated from TakoDataFreshness compiled clean with 27 tests green, directly under the field the previous fix was meant to protect. Extended to all twelve nested pairs; four regressions now fail naming the key.

Also removed the live staging.tako.com hostname from fixtures (my earlier consistency fix had taken it from one occurrence to three), and corrected the last two copies of the empty-collections claim — including the JSDoc that ships in dist/index.d.ts.

54 tests, CI green. @claude-address

[robertabbott]

Re-reviewed the delta since e85ba81. Every open comment from the previous rounds is resolved, and I verified them rather than taking the replies on trust.

The round-1 report was that the conformance test passed green in 250ms with tsc never invoked. My local node is still below pnpm's v22.13 floor, so I re-ran the exact scenario that broke it:

FAIL tests/contract/types.conformance.test.ts
Error: The conformance compile did not run, so drift is unverified. exit=1
stderr: ERROR: This version of pnpm requires at least Node.js v22.13

Loud, with the exit code and stderr surfaced. Also confirmed on this branch: the last_updated regression now fails naming the key, all 20 wire object types in src/types.ts are gated, no real Tako host remains in any fixture, every copy of the empty-collections claim carries the corrected wording, and the publish job now mirrors ci.yml step for step. Full suite 53 passed, typecheck clean, conformance compile exit 0.

One non-blocking gap left on the unions — see the inline comment. It is gate completeness, not a live mismatch.

[robertabbott]

Request-side enums are the one union pair left ungated.

The nested extension closes the object types — I reproduced deleting last_updated and it now fails with KEY_DRIFT: "last_updated", and I walked every export interface in src/types.ts against the gated list: all 20 wire object types are covered.

Working through the unions the same way, three of the response-side enums turn out to be gated by something other than SameKeys — the reverse result assignment at :194 catches them, since a widened union makes TakoSearchResult no longer assignable to the official response. The two request-side ones have nothing on either side:

Regression applied to src/types.ts	tsc -p tsconfig.conformance.json
TakoGraphNodeType += "dimension"	caught (TS2322 at :194)
TakoDatasetColumnType += "text"	caught (TS2322 at :194)
TakoKnowledgeCardRelevance += "Critical"	caught (TS2322 at :194)
TakoSearchEffort += "balanced"	exit 0
TakoContentsMode += "csv"	exit 0

Both match the spec today — SearchEffortLevel is fast \| instant \| deep and ContentsDeliveryMode is url \| inline, so there is no live mismatch. But these are the only two unions that ride on a request body, which makes a stale member the P0-1 failure shape rather than the P0-6 one: it compiles, TypeScript blesses it, and the API rejects the whole request. request.contract.test.ts does not reach it either — :28 validates effort: "deep" only, and :98 iterates a hand-written ["url", "inline"] as const rather than the type, so neither enumerates what the union actually declares.

Would extending the both-directions pattern at :119-122 to them be worth it? tako-sdk exports both names, and I confirmed this compiles clean on the branch as-is and turns both rows above into failures:

export const effort: TakoSearchEffort = null as unknown as OfficialSearchEffortLevel;
export const effortBack: OfficialSearchEffortLevel = null as unknown as TakoSearchEffort;
export const mode: TakoContentsMode = null as unknown as OfficialContentsDeliveryMode;
export const modeBack: OfficialContentsDeliveryMode = null as unknown as TakoContentsMode;

(Comment anchored here because the enum block at :119-122 is outside this diff.)