Repository navigation
feat(web-search): add Yuanbao and Bocha AI output comparisons - #275
lanyue-llk wants to merge 70 commits into
Conversation
![lexmount-ocr[bot]](https://avatars.githubusercontent.com/u/277637559?s=60&v=4){kind=small}
There was a problem hiding this comment.
This review found 2 issue(s), including 2 blocking issue(s).
src/harness/web_search/adapters.py(token_budget_reached): skipped round 2 of group "src/harness/web_search/adapters.py,src/harness/web_search/comparison.py,src/harness/web_search/core.py,src/harness/web_search/messages.json,src/harness/web_search/profiles.py,src/harness/web_search/publication.py,src/harness/web_search/report.py,src/harness/web_search/runner.py": used 560851 tokens exceeds budget 500000
| profile["protocol"].pop("comparison", None) | ||
| validate_profile(profile) | ||
| return profile, rows, {"certified": all(r["collection_receipt"].get("certified") for r in receipts), | ||
| "synthetic": all(r["collection_receipt"].get("synthetic") for r in receipts), |
There was a problem hiding this comment.
P2 — Reject mixed synthetic/measured collections or preserve their synthetic classification. load_run() accepts both kinds, so composing one synthetic provider with one measured provider makes this all(...) false. _report_edition then labels the combined numbers “Measured API evaluation” (with only a diagnostic suffix), removing the synthetic warning despite fixture results contributing to the comparison. Certification still prevents publication, but the generated local report misrepresents its evidence. Reject mixed provenance before composition, or explicitly disclose it without treating the combined rows as wholly measured.
| ) | ||
| except (ValueError, AttributeError, TypeError) as exc: | ||
| error = error or str(exc) | ||
| native_account_error = adapter_for(method["adapter"]).account_error(decoded, method) |
There was a problem hiding this comment.
P2 — Do not apply the edition check to failed transports or HTTP error responses. With expected_response_version configured, a timeout with an empty body (or an HTML 502 response) leaves decoded = {}; account_error() then reports ResponseVersionMismatch, sets stopped, and breaks before the configured retry. A temporary network/provider failure consequently interrupts the entire collection as an account problem. Gate edition validation on a successfully received HTTP 200 response, while retaining native account-error detection and the stop behavior for genuine missing/mismatched editions.
There was a problem hiding this comment.
✅ Re-review complete · 4361d1f
No findings in this incremental review.
There was a problem hiding this comment.
✅ Re-review complete · a3dcd33
No findings in this incremental review.
There was a problem hiding this comment.
ℹ️ Review skipped · 6181eb1
No reviewable content was selected for this run.
6181eb1 to
f9c3179
Compare
lanyue-llk
changed the title
There was a problem hiding this comment.
This review found 3 issue(s), including 3 blocking issue(s).
| const rows=[...table.tBodies[0].rows]; | ||
| rows.sort((a,b)=>{ | ||
| const x=value(a.cells[index]),y=value(b.cells[index]); | ||
| if(x.value===null||y.value===null)return x.value===y.value?original.get(a)-original.get(b):x.value===null?1:-1; |
There was a problem hiding this comment.
P2: The null-handling expression introduces a nested ternary, which the supplied JavaScript Code Quality Checks explicitly prohibit. Use separate branches for both-null, left-null, and right-null cases, preserving original-order ties and nulls-last behavior in both directions.
| prior_call = saved | ||
| request = dict(request, messages=reference_repair_messages(p, request['messages'])) |
There was a problem hiding this comment.
P2: Use the same invalid-answer predicate when authorizing and validating a format repair. A saved answer with an exact quoted string (or a field: "url" reference) passes parse_judgments but fails _validate_judge_field_references, so this branch spends a repair call. Final verify_provenance then invokes repaired_messages, which checks only parse_judgments on the prior response and rejects it with “A valid model judgment cannot be retried as a format repair”. Subsequent resumes reject the saved repaired call too. Align the repair-provenance check with both validators used here, and add a regression for a parseable answer that violates the required title/snippet-reference format.
| expected = {(cid, method['id']) for cid in ids for method in profile['methods']} | ||
| actual = [(r['case_id'], r['method']) for r in relevance_scores if r['case_id'] in ids] | ||
| if len(actual) != len(expected) or set(actual) != expected: | ||
| raise ValueError('Relevance comparison requires every method and query') |
There was a problem hiding this comment.
P1: This rejects the supported provider-scoped annotation scenario. With relevance_methods containing only the frozen providers, score_relevance() deliberately returns scores only for those methods. For an independent benchmark (or metric_kind='relevance'), this comparison instead requires every configured provider, so adding an unjudged Yuanbao configuration aborts report generation rather than rendering its relevance as unavailable. Preserve exact coverage checks within the declared annotation scope and retain unavailable values for other methods; enforce complete assessment separately when explicitly required. Add a build regression combining scoped frozen judgments with an unjudged provider.
There was a problem hiding this comment.
This review found 4 issue(s), including 4 blocking issue(s).
| expected = (method or {}).get("expected_response_version") | ||
| if expected is not None and not error: |
There was a problem hiding this comment.
P2 — Edition verification silently disappears when a Yuanbao method omits expected_response_version (or sets it to null). validate_profile() accepts that configuration, and normalize_response() then accepts both a missing Response.Version and a different edition. For example, a method with output_options.edition='lite' but no expectation accepts a Standard response and can attribute its results and price to Lite. This violates the requirement to verify the returned edition on every call. Require a nonempty edition expectation for Yuanbao methods during preflight, while preserving the intentional no-version-check path for failed transports; add a regression for an omitted expectation.
| const original=new Map([...table.tBodies[0].rows].map((row,index)=>[row,index])); | ||
| function value(cell){ | ||
| if(cell.hasAttribute('data-sort'))return {value:Number(cell.dataset.sort),numeric:true}; | ||
| if(cell.hasAttribute('data-value'))return {value:cell.dataset.value===''?null:Number(cell.dataset.value),numeric:true}; |
There was a problem hiding this comment.
P2: Text sorting is still broken on the standard overview and interpretation comparison. Their method-name cells are produced by numeric_cell(..., None, ...) (report.py lines 959 and 2928), so they carry data-value="" despite displaying provider names. This branch treats every provider as missing, and the new first-column sort button only restores original order in either direction. Render those text cells as plain <td> elements, as this PR already does in population_summary, while retaining empty numeric values as missing.
| "score": query_score, | ||
| "primary": acceptance(case, row, primary_factual_metric if "reference_answer" in case else "primary", cutoff=cutoff, quality_excluded=quality_excluded, relevance_grades=[x["grade"] for x in items] if case["id"] in labels else None), | ||
| "quality": any(x["grade"] is not None and x["grade"] >= 2 for x in items) if case["id"] in labels and not quality_excluded else None, | ||
| "primary": acceptance(case, row, primary_factual_metric if "reference_answer" in case else "primary", cutoff=cutoff, quality_excluded=quality_excluded, relevance_grades=[x["grade"] for x in items] if has_labels else None), |
There was a problem hiding this comment.
P2: Keep the primary verdict unavailable for methods outside the relevance scope. When a labeled scenario has anchors (or a site/vertical constraint), passing relevance_grades=None makes task_acceptance() fall back to source/site acceptance instead of returning None. Meanwhile, comparison.js chooses the primary verdict label from the case-wide has_relevance, so this unannotated method is displayed as “Relevant result found” or “No relevant result in top 5” and included in relevance outcome relations despite quality being unavailable. For a case with relevance labels but an out-of-scope method, set primary to None while retaining the separate source/site diagnostics. Extend the scoped-method regression to assert the primary verdict as well as score and quality.
| prior_call = saved | ||
| request = dict(request, messages=reference_repair_messages(p, request['messages'])) |
There was a problem hiding this comment.
P2 — Validate the original physical receipt before authorizing a format repair. This new branch decides that the saved answer is invalid using only saved["response"]; it never checks that this object matches the preserved response bytes. After repair, validate_model_evidence() validates only the replacement call's attempts, while repaired_messages() merely parses prior_call["response"]. Consequently, changing a previously valid saved response to an invalid judgment (without changing its raw receipt) allows --repair-format to repeat that valid call and produce evidence that passes validation. This defeats the promised audited, invalid-output-only repair boundary. Verify the prior request/attempt hashes and response-byte equality before spending, and apply the same verification to retained format_repair.prior_call during evidence validation, allowing only judgment-format validation to fail.
There was a problem hiding this comment.
This review found 1 issue(s), including 1 blocking issue(s).
| edition = method.get('output_options', {}).get('edition') | ||
| if (not isinstance(expected, str) or not expected.strip() | ||
| or edition is not None and edition != expected): |
There was a problem hiding this comment.
P2: Validate against the effective edition when output_options.edition is omitted. tencent_wsa_request defaults that option to standard, but this condition accepts expected_response_version='lite' (or any other nonempty string) when the option is absent. Such a profile passes the preflight in run() and sends Mode=0, Cnt=10; only after the network call can runtime version checking stop it. This defeats the new pre-call edition consistency check and can spend quota on an incorrectly configured request. Default edition to standard here as well (or require an explicit edition), and cover the omitted-option mismatch in the profile-validation test.
There was a problem hiding this comment.
✅ Re-review complete · e60e367
No findings in this incremental review.
There was a problem hiding this comment.
This review found 1 issue(s), including 1 blocking issue(s).
| for target, text in updates.items(): | ||
| target.write_text(text, encoding='utf-8') | ||
| write_json(path, manifest) |
There was a problem hiding this comment.
P2 — Make paragraph revisions recoverable across partial write failures. If one page is written successfully but a later page write or the manifest replacement fails (for example, a read-only second edition or a full disk), the report contains revised HTML with the old manifest. Retrying the same command then raises Existing authored paragraphs differ from their frozen content on the already-updated page, so the workflow cannot recover without manual restoration or rebuilding. Stage the update with retained originals and rollback/recovery state, and add a failure-injection test that verifies the report remains consistent or the same revision can resume after a mid-commit failure.
There was a problem hiding this comment.
This review found 1 issue(s), including 1 blocking issue(s).
| 'content_sha256': bytes_sha256(raw), 'provider_calls': 0, | ||
| 'changed_sections': changed, 'producer': software_identity()} | ||
| manifest['authored'], manifest['content_sha256'] = revised, bytes_sha256(raw) | ||
| manifest['content_revisions'] = [*manifest.get('content_revisions', []), receipt] |
There was a problem hiding this comment.
P2: Revision provenance is recorded but never validated. The repository search finds no consumer of content_revisions, before_manifest_sha256, or before_content_sha256 outside this writer and assertions about receipt presence. Consequently, changing a receipt's predecessor hashes, changed-section inventory, or provider-call count still passes check; a newly frozen publication plan merely binds those unchecked claims. This leaves the requested revision-provenance verification incomplete, although stale visual receipts and publication plans are correctly rejected by their manifest bindings. Validate the revision receipt schema and chain/current-content bindings in check, with negative tests for altered receipt fields; retain enough predecessor evidence to verify any historical claims the receipt makes.
There was a problem hiding this comment.
This review found 1 issue(s), including 1 blocking issue(s).
src/harness/web_search/report.py(token_budget_reached): stopped group "src/harness/web_search/report.py" mid-review: used 598136 tokens exceeds budget 500000
| source = item.get('source', '') | ||
| bound_quote = item.get('source_kind') == 'owner-quote' and any( | ||
| asset.get('path') == source and asset.get('sha256') for asset in content.get('assets', [])) | ||
| if (item.get("currency") != "USD" or item.get("unit") not in ("1000-successful-searches", "1000-requests") |
There was a problem hiding this comment.
P2 — Accepting mixed billing units here loses their meaning in the rendered comparison. catalog_price_panel, benchmark_operations, and population_summary display both units as “USD / 1,000 calls” and compare the raw amounts for winner markers; none renders item['unit'], and purchase terms are optional. For example, the mixed-unit fixture in test_report.py supplies no terms, so readers cannot distinguish a rate charged for every request from one charged only for successful searches. Render the declared billing basis per method and either normalize to an explicitly defined common workload or avoid cross-basis price winner markers. This is a correctness issue in the newly supported mixed-unit comparison, not just a wording preference.
There was a problem hiding this comment.
This review found 5 issue(s), including 5 blocking issue(s).
There was a problem hiding this comment.
This review found 5 issue(s), including 5 blocking issue(s).
lanyue-llk
changed the title
There was a problem hiding this comment.
This review found 6 issue(s), including 6 blocking issue(s).
There was a problem hiding this comment.
This review found 2 issue(s), including 2 blocking issue(s).
There was a problem hiding this comment.
This review found 2 issue(s), including 2 blocking issue(s).
There was a problem hiding this comment.
✅ Re-review complete · daced06
No findings in this incremental review.
There was a problem hiding this comment.
This review found 1 issue(s), including 1 blocking issue(s).
There was a problem hiding this comment.
This review found 2 issue(s), including 2 blocking issue(s).
There was a problem hiding this comment.
This review found 1 issue(s), including 1 blocking issue(s).
There was a problem hiding this comment.
This review found 1 issue(s), including 1 blocking issue(s).
There was a problem hiding this comment.
This review found 1 issue(s), including 1 blocking issue(s).
Add Tencent Yuanbao Standard/Lite and Bocha AI Search through the existing adapter registry. Compare result lists, additional source excerpts and generated answers separately, with shared source matching and contamination checks.
Preserve native result slots and bind citations to complete local sentences. Parse rendered Markdown labels, image alternative text and HTML line breaks without treating hidden attributes or hidden subtrees as claims. Verify physical judging receipts and common inference settings, including token limits and disabled thinking. Collection retries default to zero; invalid judgment formats have bounded, explicit correction.
The maintained report renderer provides primary-metric sorting, scrollable tables, task-level method selection and consistent evidence links. Reference finalization preserves original search calls, rejects retained HTTP failures and supports supplementary cohorts. Report data and publication bindings remain outside Git.
Validation: 1,056 local Web Search tests and 77 subtests passed, together with repository lint checks. Both CI Python shards, rendered report checks and installation/packaging checks passed. Regression coverage includes native slots, hidden HTML attributes, rendered image labels, line breaks, sentence boundaries, incompatible inference settings, missing answer scopes and reference recovery.
No package or benchmark edition bump; no release tags.