Conversation
Adds support for the new `knowledge` request parameter on POST /v1/search and the knowledge result models that come back with it. Request: `knowledge="core"` on `search()` / `search_async()` and the deprecated `you.search.unified*` shims. Normalized to lowercase like the other enum-typed params; an invalid value raises ValidationError locally before any network call, mirroring the server's 422. All params on these methods are keyword-only, so the addition is non-breaking. Response: `Results.knowledge` is an optional list of `KnowledgeResult`, each carrying `type`, `title`, and `attribution`, plus `description` and an optional `as_of` date for `type: answer` results. `type` is a plain `str` so an unrecognized future kind parses instead of raising. The API omits the key entirely when nothing relevant is found, so the field is None rather than an empty list. Also fixes a blind spot in scripts/check_drift.py: the response-schema check compared top-level fields only, so a new nested field such as `results.knowledge` was invisible. It now recurses through nested object schemas and arrays, and falls back to the previous flat comparison when a response resolves to a oneOf union rather than a single object schema. Recursion surfaced two pre-existing gaps unrelated to this change (AnswerSearchResult.description/.thumbnail_url and FinanceResearchSource.snippets); those are listed in an explicit KNOWN_RESPONSE_GAPS table that reports itself stale once the SDK catches up. Verified: 425 offline tests pass, 6 live knowledge tests pass against prod, mypy clean over 85 files, pylint 10.00/10 on the CI gate, and the drift check is clean in both --verbose and --strict modes.
… (DX-835) Two of three review findings verified valid and fixed here. check_drift.py: the KNOWN_RESPONSE_GAPS staleness check compared against `missing_in_sdk`, so a field the spec *dropped* was reported as stale with the message "the SDK model now defines it" -- false, and it would fail a strict drift run for the wrong reason. Staleness now compares against the model's own fields (`known & model_fields`). Negative-tested four ways: spec-drops-field emits nothing, SDK-defines-field still reports stale, genuine new drift is still caught, and existing suppressions still hide the known gaps. The previous logic reproduced the false positive on the first case. test_live.py: test_knowledge_result_shape hard-asserted `type == "answer"` and required `description` on every result, contradicting the forward-compat design behind modeling `type` as a plain str. The spec states description is "Required on `type: answer` results" only, and that an unrecognized kind should be ignored rather than failed on. The test now asserts the fields the spec requires of every kind and scopes description to `type == "answer"`. Rejected: rewording the Knowledge docstring to drop "alongside web and news search". That sentence is verbatim from the published OpenAPI spec, which the model docstrings mirror by convention. The live API contradicting it is the server-side bug tracked separately; rewriting the SDK contract to match a launch-blocking bug would desync the SDK from the spec and docs.
The Knowledge results snippet used a bare `you`, so pasting it alone raised NameError, and it made a network call without timeout_ms. Its direct structural precedent -- `#### Page content extraction`, the same kind of new-search-param subsection at the same heading level -- is fully wrapped with imports, client construction, and timeout_ms=60_000. The bare-style snippets in this README are only the short top-level API-overview blurbs that follow the Quickstart. Now matches the extraction subsection. Verified by extracting the rendered snippet and running it verbatim against prod: prints knowledge card titles, descriptions, and attribution credits.
`Results.web` and `Results.news` are `Optional[List[...]]` in
`searchresponse.py`, but their `docs/models/results.md` type cells read
plain `List[...]`. The wrong cells pre-date this branch, but adding the
`knowledge` row re-padded the table and re-emitted both lines, so they
are changed lines in this diff.
AGENTS.md ("Type cells in field tables come from the annotation") names
`Optional[X]` vs `X` as a trap and requires eyeballing every row of a
touched model page, not just the rows whose prose changed.
Verified with an annotation-vs-docs comparison across all four touched
model pages (25 rows): all match, including the new `knowledge` row.
…ert (DX-835) Two findings from the second droid-review round, both verified valid. Cross-table identity: the `knowledge` description in docs/sdks/you/README.md, docs/sdks/search/README.md and docs/models/searchrequestbody.md carried the `"core"`-only / ValidationError / 25-result / `count` constraints, but both `SearchRequestBody` field docstrings stopped after the first sentence. AGENTS.md requires those surfaces to read identically. Expanded the docstrings rather than shrinking the tables, matching how `include_domains` and `extraction` already carry SDK-layer guidance past the spec sentence. The `Knowledge` enum class docstring stays spec-verbatim: it is not one of the four cross-table surfaces, `components.schemas.Knowledge.description` is exactly that sentence (re-fetched), and `SearchRequestBody.properties.knowledge` carries no description of its own. `FreshnessValue` and `LiveCrawlFormats` likewise have enum docstrings that differ from their field docstrings. Live assert: `assert kr.description` -> `assert kr.description is not None`. `description` is `Optional[str]`, so `is not None` is the real presence check; truthiness would also enforce non-empty, which AGENTS.md's "`is not None`, not truthiness" rule warns against for server-returned strings. `type`, `title` and `attribution[].name` keep truthiness deliberately: those are required `str` fields, where `is not None` is vacuous (pydantic already guarantees it) and non-empty is the meaningful claim.
…ntory (DX-835) Docs-only. Two independent staleness fixes found while auditing every doc surface in the repo. Type cells: 11 rows across 8 model pages declared a bare type where the annotation is Optional -- contentsrequest (urls, formats), sourcecontrol (include_domains, exclude_domains, boost_domains), source (snippets), searchrequest (livecrawl_formats), researchdetail and financeresearchdetail (ctx), researchrequest (output_schema), researchtaskstreameventdata (data). AGENTS.md names Optional[X] <-> X as a type-cell trap. All pre-date this branch. Every cell fits its existing padded column, so only those 11 lines change and table alignment is preserved. The Required cells were already right: is_required() is False for all 11. tests/README.md: listed 11 of the 20 test files and carried wrong counts (Contents 12 vs 13, Answer 23 vs 25). Now lists all 20, adds groups for extraction, knowledge, page_age, stream events and the cross-cutting suites, and labels counts as pytest --collect-only numbers. The groups sum to 425, exactly the CI-gate collection. Verified with a heading-aware audit comparing every field table in docs/models/ against typing.get_type_hints -- 151 rows, all four AGENTS.md traps (Optional<->X, datetime<->date, bool<->int, str<->int): 0 findings left. The 15 enum "## Values" pages were also diffed against their code members: clean, the one apparent hit being contentsformats' intentional "metadata _(deprecated)_" annotation.
…o the retries example (DX-835) Two docs footguns, one from review and one found sweeping every snippet. docs/models/knowledgeresult.md: the `as_of` note told readers to call `datetime.strptime(card.as_of, "%Y-%m-%d")` without an import, so following it raised NameError. Replaced with a fenced block that carries `from datetime import datetime`, matching the pattern docs/models/webresult.md already uses for its own `page_age` datetime note. The block also guards the optional section (`res.results.knowledge or []`) and uses `is not None` rather than truthiness, consistent with the model's Optional[str] annotation. README.md: the Retries example builds its own client but passed no `timeout_ms`, so copying it inherits httpx's 5s default and raises ReadTimeout -- the exact failure the adjacent Timeouts section warns about. AGENTS.md requires an explicit `timeout_ms` on any snippet that triggers a round-trip. The other README snippets that call `you.search` / `you.contents` without a `timeout_ms` are deliberately untouched: they reuse the client constructed in Quick Start, which already passes `timeout_ms=60_000`, so the rule is satisfied once upstream rather than per snippet.
Contributor
|
Droid finished @tyler5673's task —— View job LGTM — no high-confidence, actionable issues found in the PR diff. |
Two spots in tests/test_knowledge.py referenced a `knowledge` value that the published spec does not define -- one in a docstring that also described it as not public yet, which discloses an unreleased parameter value in a public repo, and one as the input to the invalid-value case. Both now state the contract without naming anything unpublished. The docstring explains that `core` is the only value the published spec defines and why the enum is pinned to exactly that, and the invalid-value case uses an obviously non-value string instead. Coverage is unchanged: the enum is still pinned to ["core"], and an unrecognized value still raises ValidationError locally before any request is sent. 21 knowledge tests and the 425-test offline gate pass, mypy clean, pylint 10.00/10 on the CI errors-only gate, drift --strict no_drift.
Contributor
|
Droid finished @tyler5673's task —— View job LGTM — I did not find any high-confidence, actionable issues in the PR diff. |
…ess (DX-835) The two async knowledge tests each re-declared the mock-transport handler and the AsyncClient/You scaffolding already living in `_capture`, differing only in the response body. Extracted `_acapture` as the async twin. Worth doing for the lifecycle rather than the line count (net +1): the suite treats a leaked transport as a failure via ResourceWarning-as-error, and the copy-pasted form put an `await client.aclose()` in every caller's hands. One helper now owns it, so a future async test cannot forget it. Coverage unchanged: 21 knowledge tests and the 425-test offline gate pass with no ResourceWarning.
Contributor
|
Droid finished @tyler5673's task —— View job The PR looks consistent with existing SDK conventions and is well-tested. One correctness issue: the new recursive response drift checker uses a global |
![factory-droid[bot]](https://avatars.githubusercontent.com/in/358017?s=60&v=4){kind=small}
…he (DX-835) _compare_response_fields() used `visited` as an "already compared this model" cache, returning early whenever the same pydantic class reappeared. One model can legitimately sit at several response paths backed by different spec schemas, so every branch after the first was skipped and real drift went unreported. Reproduced before fixing: a root model with two fields of the same type whose spec schemas differ by one field reported drift on the first path only and silently missed the second. `visited` is now discarded on exit via try/finally, so it breaks cycles without suppressing sibling branches. Checked against self-referential and mutual A->B->A schemas: both still terminate, and drift inside the cycle is still reported. Fixing the cache unmasked one genuine asymmetry, now suppressed explicitly rather than left as a standing warning: web-search results.news.contents: SDK has `highlights`, spec does not results.web[].contents resolves to WebContentsPost, which defines `highlights`, while results.news[].contents resolves to the narrower Contents schema (html, markdown). The SDK shares one Contents model for both. Confirmed against prod that highlights never arrives on the news path: with extraction_mode=highlights news items carry no contents at all, and with the deprecated livecrawl=all they carry html only. Narrowing the model would be a breaking change for no behavioral gain, so KNOWN_SHARED_MODEL_EXTRAS records the gap. It mirrors KNOWN_RESPONSE_GAPS and is self-invalidating the same way, reporting the entry as stale if the spec ever defines the field at that path. drift --verbose and --strict both clean. 425 offline tests, all 46 live tests against prod, mypy clean over 85 files, pylint 10.00/10 on the CI gate.
Contributor
|
Droid finished @tyler5673's task —— View job The knowledge request/response surface looks consistent with existing SDK patterns and is well-tested. Remaining issues are minor but user-facing: a few docs snippets should guard the optional |
…tale descriptions (DX-835) Five review findings: four fixed, one rejected. `SearchResponse.results` is `Optional[Results]` and not required, so `for card in res.results.knowledge or []:` still raises AttributeError when a response omits `results` -- the `or []` only guards the inner key. Guarded all five snippets (README, USAGE, and three in docs/models/knowledgeresult.md) with `if res.results:`, matching examples/api-example-calls.py, which already did it correctly. `SearchResponseTypedDict` / `SearchResponse` described the payload as "unified search results from web and news sources"; `Results` now carries a third section. Updated both docstrings and the mirrored docs/models/searchresponse.md. That text is SDK-authored -- `SearchPostResponse.description` is null in the published spec -- so there is no spec wording to stay verbatim against. The endpoint summaries in docs/sdks/* and sdk.py are left alone deliberately: they quote the spec's operation.summary verbatim, the spec has not been updated, and knowledge is opt-in, so describing the endpoint's default output as web and news is still accurate. The knowledge perf case measures the request side only under the default mock target, because tests/mockserver returns one fixed payload regardless of the request body and includes no knowledge section. Documented that in the docstring rather than changing the Go mock: the mock ignores the request entirely, so emitting knowledge conditionally would mean parsing the body, and the pre-existing extraction cases have the same property. Rejected: the claim that tests/README.md undercounts test_performance.py (33 vs 34). pytest collects 33; the 34th `def test_` line is `test_target`, a module-level @pytest.fixture, not a test. The doc's counts are labelled as `pytest --collect-only` numbers and match.
Contributor
|
Droid finished @tyler5673's task —— View job LGTM. I did not find any high-confidence, actionable issues in the PR diff. |
…esearch (DX-835) Two response models were narrower than the spec they parse against, so documented fields were discarded at parse time. AnswerSearchResult lacked `description` and `thumbnail_url`. The answer spec defines both on results.web[] items and prod returns them: a live call came back with description on 20/20 web results and thumbnail_url on 16/20, all silently dropped. WebResult on the search endpoint already declared both, so the answer model was simply the narrower of two siblings describing the same shape. FinanceResearchSource lacked `snippets`. The finance-research spec defines it on output.sources[] and the sibling Source model on the Research API already declared it, with an identical description. Prod was not returning it -- two live calls at `deep` and `exhaustive`, 10 sources between them, all carrying only title and url -- so nothing was being lost yet. Added anyway: it was held back by a KNOWN_RESPONSE_GAPS suppression whose stale check can only see the SDK catching up, never the API starting to honour its own spec, so that gap would have stayed silent from the API side indefinitely. Both suppressions are removed and KNOWN_RESPONSE_GAPS is now empty. drift --verbose reports no drift with an empty table, which is the actual proof the gaps closed rather than got hidden; the stale mechanism was negative-tested separately so it is still live for the next one. The Go mock for finance_research now emits `snippets`, and its comment no longer claims the model defines only url and title. tests/test_research.py asserted that old behaviour and now asserts snippets parse. tests/test_answer.py feeds both new fields through _ANSWER_BODY and pins them present on the first web result and None on the second, covering optionality as well. Folds into the unreleased 3.5.0 rather than bumping again; CHANGELOG records both under Fixed.
Contributor
|
Droid finished @tyler5673's task —— View job Overall the PR is well-scoped and consistent with existing SDK patterns. One small improvement: guard the optional |
…at lacked it (DX-835) `SearchResponse.results` is `Optional[Results]`, so `assert res.results.knowledge` raises AttributeError on a response that omits `results`, hiding the assertion that was actually meant to fail. Three of the six tests in TestLiveSearchKnowledge already asserted `res.results is not None` first; the other three did not. All six now do. Verified by parsing the class and confirming, per test, that the guard precedes the first dereference rather than just that both strings appear. 6 live knowledge tests pass against prod; 425 offline tests unchanged.
Contributor
|
Droid finished @tyler5673's task —— View job The PR changes look consistent with existing SDK patterns and are well-tested. Only remaining note is to strengthen the existing Contents live tests so they assert requested fields are present (using |
…X-835)
Three of the four TestLiveContents cases could pass without the requested field
ever arriving:
test_html_format guarded with `if res[0].html:`, so a response carrying no
html asserted nothing -- while its own comment said
"HTML should be present when HTML format is requested"
test_markdown_format asserted only that a non-empty list came back
test_multiple_formats same; never checked html or markdown at all
test_metadata_format in the same class already did this correctly
(`assert res[0].metadata is not None`), so the other three now match it.
Presence is asserted with `is not None` rather than truthiness, per AGENTS.md's
rule for server-returned strings.
Pre-existing, and untouched by this branch until now. Verified against prod
after strengthening rather than assuming: all 5 Contents live tests pass, so the
fields really do arrive and the assertions were simply missing. The full
non-research live suite (31 tests) and the 425-test offline gate pass.
Contributor
|
Droid finished @tyler5673's task —— View job The PR looks solid and well-tested. One remaining user-facing docs wording issue: |
…-835) The `knowledge` parameter description opened with "Requests knowledge results alongside web and news search", which reads as a promise that those sections accompany knowledge results. `SearchResponse.results` and each of its sections are optional, and this PR's own description records that prod currently drops `results.web` entirely when knowledge is requested. The docs and tests here deliberately make no claim about `results.web` alongside knowledge, so the parameter wording was the one surface still implying it. Reworded the opening to "Requests knowledge results from licensed data providers." -- accurate, matches how the CHANGELOG and USAGE already describe knowledge, and makes no claim about other sections. It is one character shorter than the sentence it replaces, so all three parameter tables keep their existing row padding; a longer rewrite would have forced re-padding every row of three tables for a wording change. Applied to the five SDK-authored surfaces the cross-table identity rule covers (both SearchRequestBody field docstrings, docs/sdks/search/README.md, docs/sdks/you/README.md, docs/models/searchrequestbody.md) and verified identical afterwards. SearchRequestBody.properties.knowledge carries no description in the published spec, so this text is entirely SDK-authored. Deliberately NOT applied to the two spec-verbatim surfaces: the Knowledge enum class docstring and docs/models/knowledge.md mirror components.schemas.Knowledge.description, which is exactly "Requests knowledge results alongside web and news search." AGENTS.md pins model docstrings to the spec, so those follow the spec rather than editorialising ahead of it; if the upstream wording changes they change with it.
Contributor
|
Droid finished @tyler5673's task —— View job Validated 3 candidate comments: 2 are high-confidence and actionable (one drift-checker edge case, one docs wording fix), and 1 was rejected as a subjective rewording that would also require cross-surface docstring changes. |
… page (DX-835)
Two review findings.
check_drift.py: `stale = known & model_fields` contradicted the comment sitting
directly above it. A field the spec *dropped* is still in model_fields, so its
suppression was reported stale even though the spec no longer defines it -- while
the comment said that case "must not be reported as stale". Intersecting with
`props` makes the code match the stated intent. Proven real rather than
theoretical: with spec props {kept}, suppressed {added} and SDK fields
{kept, added}, the old expression yields stale={added} and the new one yields
empty. All three staleness cases now verified -- gap still open (not stale), SDK
caught up (stale), spec dropped the field (not stale).
The mirror table KNOWN_SHARED_MODEL_EXTRAS already intersected with props, so it
was correct; the two directions differ and only this one was wrong.
docs/models/knowledge.md: the H1 said knowledge is requested "alongside web and
news search" -- the same composition promise removed from the parameter surfaces
in 34bde1e. I declined to change this page then, on the grounds that it mirrors
the spec-verbatim Knowledge enum docstring. That was over-broad. AGENTS.md's
spec-verbatim rule binds model docstrings; docs pages carry their own prose.
docs/models/extraction.md, the page AGENTS.md names as the format template, does
not match the Extraction class docstring, and contentsmetadata.md uses a richer
deprecation callout. Across all 65 model pages, 22 mirror their class docstring
exactly once RST/markdown backtick dialect is normalised and 7 genuinely carry
their own wording. So the page is reworded and the enum docstring stays verbatim.
Worth noting: scripts/check_drift.py has no committed test coverage at all, which
is why two bugs in this one function surfaced only through review.
Contributor
|
Droid finished @tyler5673's task —— View job No high-confidence, actionable issues found in the PR diff. The knowledge request/response surface and the drift-checker recursion changes look consistent with existing SDK patterns, and the concurrent security review found no issues. |
scripts/check_drift.py had no committed test coverage, which is why two bugs in one function -- the `visited` global cache and the staleness intersection -- surfaced through review rather than through a failing test, and why each reproduction written while fixing them was thrown away afterwards. tests/test_check_drift.py loads the script by path (it is not a package module) and pins 22 cases: response recursion down through arrays and $refs, drift on a sibling branch that reuses a model, cycle safety for self-referential and mutual A->B->A schemas, oneOf bail-out, all three staleness cases for both suppression tables, the _resolve_schema and _nested_model helpers, and the shipped state of both tables. Both regressions were verified to actually fail when their fix is reverted and pass when it is restored, so these are regression tests rather than coverage for its own sake. The script is byte-identical afterwards. Offline gate 425 -> 447. tests/README.md inventory and group counts updated to match: 21 files listed, groups summing to exactly the CI-gate collection.
Contributor
|
Droid finished @tyler5673's task —— View job The PR looks consistent with existing SDK patterns and is very thoroughly tested. One remaining user-facing issue is a docstring for |
…ten two weak live tests (DX-835)
check_drift.py compares the published specs against the SDK models. That cannot
see a field the API returns but no spec declares -- which is exactly how
AnswerSearchResult came to drop `description` and `thumbnail_url` while every
static check agreed with itself and reported no drift.
scripts/audit_wire.py closes that gap: it walks the raw JSON from a live call
next to the parsed model and reports any key the model discarded. Across all
seven endpoints that is 228 wire keys, none unexplained. It needs YDC_API_KEY so
it is a pre-release check, not a CI gate; --strict exits 1 on an unexplained
drop, negative-tested by emptying the known list.
Two keys are recorded in KNOWN_WIRE_EXTRAS as observed-but-undeclared rather than
modeled, so the tool stays quiet about decisions instead of hiding accidents:
results.web[].original_thumbnail_url declared by no published spec
finance-research root `warnings` declared by the sibling research spec
and sent by prod as [], but absent from
finance-research.json
`warnings` is the interesting one. Adding it to FinanceResearchResponse is
tempting -- ResearchResponse has it and prod sends it -- but finance-research.json
declares only `output`, so the field would put the SDK permanently ahead of that
endpoint's contract and leave a standing drift warning. Modeled it, watched
check_drift --strict fail on exactly that, and reverted. The rule that falls out
is the one already applied to snippets: add the field when the endpoint's spec
declares it, record it when only prod does. A test pins the current state so
closing the gap later is a deliberate act.
Also here:
- docs/models/researchresponse.md never documented `warnings`, though the field
has always existed and parsed. Found by a missing-row audit; the type-cell
audit could not see it because it only validates rows that already exist.
- tests/test_live.py TestLiveSearch.test_search_with_filters guarded its only
substantive assertion behind `if res.results.web:`, so an empty response
passed and the filters went unverified despite the name. Now asserts the
section is populated; verified against prod over five consecutive runs.
- two pre-existing mypy errors in check_drift.py, from indexing a heterogeneous
list of dicts, so `mypy src/youdotcom/ scripts/` is clean at 87 files.
Offline gate 425 -> 450. tests/README.md counts updated to match.
Contributor
|
Droid finished @tyler5673's task —— View job Overall this PR is thorough and consistent with existing SDK patterns, with extensive test and docs updates. The only additional actionable issue I found is a small resource-lifecycle fix in the new audit script. |
…ransport (DX-835) Two review findings. sdk.py's `_search_impl` `:param knowledge:` still said knowledge is returned "alongside web and news search" -- the same composition promise removed from the five parameter surfaces in 34bde1e. It survived because that sweep grepped line by line and this copy wraps across two lines. A whitespace-normalised search over src/, docs/, README, USAGE and CHANGELOG now finds the phrase in exactly one place: the Knowledge enum docstring, which stays verbatim against components.schemas.Knowledge.description. scripts/audit_wire.py's _Spy wrapped an httpx.HTTPTransport but never closed it. BaseTransport.close() is a no-op and Client.close() only calls the outer transport, so the inner pool leaked once per audited call -- seven times on a full run. Added close() delegating inward, and verified both directions: with the override the inner transport's close() is called exactly once, and a control spy without it is called zero times.
Contributor
|
Droid finished @tyler5673's task —— View job Changes look solid and well-tested overall; I only found one small docstring inconsistency that could mislead IDE/help() users about what |
… pattern (DX-835) docs/models/results.md was a bare field table with no prose, unlike its sibling searchresponse.md. That matters here: all three sections are Optional and independent, `results` itself is Optional, and prod currently omits `web` entirely whenever knowledge is requested -- returning a completely empty results object for roughly half the queries tested. A user reading the field table alone had nothing telling them to guard each level. Added the contract statement and a runnable example. The example was executed verbatim against prod on a query that returns no `web` section: it parses, meets the copy-paste-runnable and timeout_ms rules, and runs clean. The wording describes optionality, which the schema guarantees and which stays true however the server-side issue resolves. It deliberately does not say "`web` is omitted when knowledge is requested" -- that would document a bug as contract, be wrong once fixed, and nothing would catch the staleness, because check_drift.py compares field names, enums, endpoints and servers but never descriptions.
Contributor
|
Droid finished @tyler5673's task —— View job The PR looks solid and consistent with existing SDK patterns, with extensive tests and docs. The only actionable issue I found is a small CLI help mismatch in the new wire-audit script. |
The help said "list kept keys too", but the flag never printed kept keys -- both branches print dropped keys and differ only in path normalization. Reworded to describe the real behavior and why the default is the more useful one: the normalized form (results.web[].x) is what matches KNOWN_WIRE_EXTRAS, while --verbose preserves list indices (results.web[0].x) for locating a specific item. Verified against a run with the known-extras list emptied, so the dropped-key paths are actually printed in both modes: default normalizes, --verbose does not.
Contributor
|
Droid finished @tyler5673's task —— View job The PR looks consistent with existing SDK conventions and is well-tested. The one remaining high-confidence, actionable issue is a TypedDict typing mismatch that breaks the advertised plain-string parameter contract for type-checking users. |
…corded (DX-835) `livecrawl=web` combined with `livecrawl_formats=[markdown]` no longer returns `contents` on any web result. Verified against prod: livecrawl=web + formats=[markdown] 0/3 web results have contents <- fails livecrawl=all + formats=[markdown] 2/3 livecrawl=web (no formats) 2/3 extraction full_page markdown 3/3 extraction highlights 3/3 So it is one deprecated combination, and the replacement works fully. The test fails identically on origin/main in a clean worktree, so it predates this branch and is not a regression from the knowledge work. Marked xfail rather than relaxed or deleted: relaxing the assertion would bake in behavior nobody has said is intended, and deleting it would lose the record. The reason string carries the measured matrix so the next reader does not have to re-derive it. Non-strict, so it xpasses harmlessly if the server starts returning contents again -- at which point the marker should come off. Worth noting for whoever owns the search backend: MIGRATION.md still tells users "`livecrawl` and `livecrawl_formats` still work on `POST /v1/search`... removal is targeted for 4.0.0", and we are on 3.5.0. Deprecated is not the same as broken, so either the promise or the behavior needs to move. Live suite now 45 passed / 1 xfailed instead of 1 failed.
Contributor
|
Droid finished @tyler5673's task —— View job The PR is thorough and consistent with existing SDK patterns, with strong test and docs coverage. The main issues are in the new wire-audit script (it can crash on omitted root sections and can report false drops for |
…models (DX-835) Two review findings, both reproduced before fixing. _audit indexed spy.last[root], so a response omitting that section raised KeyError and was reported as a call failure (exit 2), aborting the remaining endpoints. Reproduced with a body containing only `metadata`. Now uses .get() and reports the absent section explicitly rather than printing a vacuous "ok" -- "nothing to walk" and "walked it, kept everything" are different claims and the output should not conflate them. _walk only considered declared model_fields, so a model configured extra="allow" would report its retained keys as dropped. Not hypothetical: Result and TaskDetailInput are both configured that way. Extras present in model_extra now count as kept, and a real drop on an ordinary model is still reported. Also widened KNOWN_WIRE_EXTRAS to accept a None label meaning "any call". `results.web[].original_thumbnail_url` is undeclared by every published spec, and keying it to one call label made the same undeclared field look unexplained as soon as another call returned it. Verified: the absent-root simulation reports instead of raising; an extra="allow" model yields 0 dropped / 2 kept; an undeclared field on WebResult is still reported. Full audit over 7 endpoints and 241 wire keys exits 0.
Contributor
|
Droid finished @tyler5673's task —— View job No candidate inline comments to validate or post. Overall: no high-confidence, actionable issues found in the PR diff. |
Contributor
Author
|
Superseded by #64 — same branch, same 23 commits, identical diff. Closing only to retrigger a clean agent review pass; all 19 threads here were resolved, and the last droid round reported no actionable issues. |
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.
Superseded by #64 — closed to retrigger a clean review pass on an uncluttered diff. Same branch, same commits, identical diff; all 19 threads here were resolved before closing, and the final review round reported no actionable issues.
See #64 for the current description, verification, and review record.