Repository navigation
feat(webfetch): support main and full content acceptance scopes - #268
lanyue-llk wants to merge 10 commits into
Conversation
9ccc896 to
691cd74
Compare
![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 6 issue(s), including 6 blocking issue(s).
| function diagnosticAnchor(){let select=document.querySelector('select[data-filter="issue"]');if(!select)return;let match=location.hash.match(/^#issue=([a-z]+)$/);if(!match||![...select.options].some(o=>o.value===match[1]))return;document.querySelectorAll('select[data-filter]').forEach(s=>s.value=s.dataset.filter==='result'?'fail':'');select.value=match[1];cascadeFilters();document.getElementById('failure-inventory')?.scrollIntoView()} | ||
| document.addEventListener('DOMContentLoaded',()=>{document.querySelectorAll('[data-call-log-open]').forEach(b=>b.addEventListener('click',()=>{openCallLog(b)}));document.querySelectorAll('[data-call-log-close]').forEach(b=>b.addEventListener('click',()=>closeCallLog(b)));diagnosticAnchor()});window.addEventListener('hashchange',diagnosticAnchor); No newline at end of file | ||
| async function loadDiagnosticEvidence(dialog){for(let pre of dialog.querySelectorAll('pre[data-evidence-file]')){if(pre.dataset.loaded)continue;try{if(typeof textFile==='function'){pre.textContent=await textFile(pre.dataset.evidenceFile)}else{let response=await fetch(pre.dataset.evidenceFile);if(!response.ok)throw Error('HTTP '+response.status);pre.textContent=await response.text();}pre.dataset.loaded='1'}catch(error){pre.textContent='Evidence could not be loaded: '+error.message;}}} | ||
| function initDiagnostic(){document.querySelectorAll('[data-call-log-open]').forEach(b=>b.addEventListener('click',()=>{openCallLog(b);loadDiagnosticEvidence(document.getElementById(b.dataset.callLogOpen))}));document.querySelectorAll('[data-call-log-close]').forEach(b=>b.addEventListener('click',()=>closeCallLog(b)));diagnosticAnchor()} |
There was a problem hiding this comment.
P2: Each evidence click now starts two loads. The shared openCallLog() in assets/report.js already calls loadDiagnosticEvidence(dialog), and this handler immediately calls it again. The new function overrides the shared loader and checks only dataset.loaded, which is set after awaiting the fetch, so both invocations fetch/decode the same large evidence concurrently. Keep a single invocation and retain the shared loader's in-flight guard (and large-text handling), adding package textFile support there rather than overriding it.
| document.addEventListener('DOMContentLoaded',()=>{document.querySelectorAll('[data-call-log-open]').forEach(b=>b.addEventListener('click',()=>{openCallLog(b)}));document.querySelectorAll('[data-call-log-close]').forEach(b=>b.addEventListener('click',()=>closeCallLog(b)));diagnosticAnchor()});window.addEventListener('hashchange',diagnosticAnchor); No newline at end of file | ||
| async function loadDiagnosticEvidence(dialog){for(let pre of dialog.querySelectorAll('pre[data-evidence-file]')){if(pre.dataset.loaded)continue;try{if(typeof textFile==='function'){pre.textContent=await textFile(pre.dataset.evidenceFile)}else{let response=await fetch(pre.dataset.evidenceFile);if(!response.ok)throw Error('HTTP '+response.status);pre.textContent=await response.text();}pre.dataset.loaded='1'}catch(error){pre.textContent='Evidence could not be loaded: '+error.message;}}} | ||
| function initDiagnostic(){document.querySelectorAll('[data-call-log-open]').forEach(b=>b.addEventListener('click',()=>{openCallLog(b);loadDiagnosticEvidence(document.getElementById(b.dataset.callLogOpen))}));document.querySelectorAll('[data-call-log-close]').forEach(b=>b.addEventListener('click',()=>closeCallLog(b)));diagnosticAnchor()} | ||
| document.addEventListener('DOMContentLoaded',initDiagnostic);window.addEventListener('report-ready',initDiagnostic);window.addEventListener('hashchange',diagnosticAnchor); |
There was a problem hiding this comment.
P2: Issue drill-down links do not apply their filters in the newly supported packaged diagnostics. The package loader converts analysis/index.html#issue=text into #page=analysis%2Findex.html&anchor=issue%3Dtext, but diagnosticAnchor() only recognizes a hash matching ^#issue=([a-z]+)$. Calling it on report-ready therefore does nothing, and the loader's applyReportAnchor() also has no issue handler. Clicking an output-defect link in the interpretation page shows the unfiltered inventory instead of the relevant failures. Decode the packaged anchor parameter (while retaining standalone hash support) and apply the issue filter after the virtual page is installed.
| if content_scope is not None: | ||
| from lexbench_webfetch.content_scopes import select_content_scope | ||
| raw_cases = [select_content_scope(item, content_scope)[0] for item in raw_cases] |
There was a problem hiding this comment.
P2: Apply scope selection even when there is no dataset-level or CLI override. A task can declare its own content_scope, and annotated tasks otherwise default to full in select_content_scope. This guard skips both cases, returning the original top-level requirements to execution and persistence, while score_attempt later replaces them with the selected profile. validate_case does not repair this: it discards the selector's returned dictionary. Consequently acquisition can use different requirements from scoring (for example, Cloudflare's video-link extraction reads case.expected_resources), and saved task requirements can misrepresent the scored contract. Run every task through select_content_scope(item, content_scope) before constructing EvalCase, retaining the selector's unchanged behavior for legacy tasks.
| page('analysis/index.html',t('analysis', 'Analysis'),analysis) | ||
| guide=hero(t('extraction_results_for_these_pages', 'Extraction results for these pages'),(f"{str(model['metrics']['overall'][0])} / {str(len(rows))} " + t('first_calls_met_the_full_acceptance_requirements', 'first calls met the full acceptance requirements of the time; the diagnostics below cover the designated sample.'))) | ||
| guide+=overall_section+section(t('results_and_measurement_scope', 'Results and measurement scope'),(f"{table([t('task_group', 'Task group'), t('implementation_version', 'Implementation version'), t('tasks', 'Tasks'), t('call_success', 'Call success'), t('pass', 'Pass'), t('mean_latency_s', 'Mean latency (s)'), t('p50_latency_s', 'P50 latency (s)'), t('p95_latency_s', 'P95 latency (s)')], summary_rows, 'guide-summary', sortable=True)}<p>" + t('latency_covers_all_calls_in_each_group', '{metric_note} Latency covers all calls in each group including failures; P50 is the median. Groups without complete timing are not computed.', metric_note=esc(model['metric_note'])) + '</p>'),'guide-results') | ||
| guide=hero(t('extraction_results_for_these_pages', 'Extraction results for these pages'),model.get('interpretation_lead') or (f"{str(model['metrics']['overall'][0])} / {str(len(rows))} " + t('first_calls_met_the_full_acceptance_requirements', 'first calls met the full acceptance requirements of the time; the diagnostics below cover the designated sample.'))) |
There was a problem hiding this comment.
P2: Escape the new model-provided text before inserting it into HTML. hero() interpolates desc verbatim, and load_diagnostic() does not sanitize interpretation_lead; the new pass_label/fail_label interpolations below have the same problem. For example, a literal lead containing '
| if main.get("forbidden_any", []) != full.get("forbidden_any", []): | ||
| raise ValueError("Content scope cannot change blocked-page rejection rules") |
There was a problem hiding this comment.
P2: Compare these invariants against the original task as well, not just against the other profile. Both profiles can omit forbidden_any and source_benchmark, pass validation, and then have the defaults erase the task's blocked-page rules and navigation contract. For example, adding the test's fact-only profiles to a task with a forbidden marker permits an otherwise matching answer containing that marker to pass. The same omission removes the risk_control.navigation_contract used by score_attempt for page identity. Preserve these task-level protections or reject profiles that jointly remove/change them.
| effective = deepcopy(case) | ||
| effective.update(deepcopy(REQUIREMENT_DEFAULTS)) | ||
| effective.update(deepcopy(profiles[selected]["requirements"])) |
There was a problem hiding this comment.
P2: The replacement leaves ordered_tree_reference and ordered_tree_min_similarity from the original case active, while the requirements allowlist prevents either profile from overriding them. A task with a full-page tree threshold and valid main-content requirements therefore still scores main output against the full-page tree; even a response satisfying every main requirement can fail ordered_tree_pass. Include tree acceptance in the scope replacement/validation contract (and its binding), or explicitly reject this combination rather than silently inheriting an unchangeable full-page requirement.
|
Review of head CI (3 failures, all reproduced locally):
New findings:
Version: this changes scoring (main/full selection), so per Paired HF dataset PR (discussion #6): it pins |
There was a problem hiding this comment.
This review found 8 issue(s), including 8 blocking issue(s).
|
|
||
| function diagnosticAnchor(){let select=document.querySelector('select[data-filter="issue"]');if(!select)return;let match=location.hash.match(/^#issue=([a-z]+)$/);if(!match||![...select.options].some(o=>o.value===match[1]))return;document.querySelectorAll('select[data-filter]').forEach(s=>s.value=s.dataset.filter==='result'?'fail':'');select.value=match[1];cascadeFilters();document.getElementById('failure-inventory')?.scrollIntoView()} | ||
| document.addEventListener('DOMContentLoaded',()=>{document.querySelectorAll('[data-call-log-open]').forEach(b=>b.addEventListener('click',()=>{openCallLog(b)}));document.querySelectorAll('[data-call-log-close]').forEach(b=>b.addEventListener('click',()=>closeCallLog(b)));diagnosticAnchor()});window.addEventListener('hashchange',diagnosticAnchor); No newline at end of file | ||
| async function loadDiagnosticEvidence(dialog){for(let pre of dialog.querySelectorAll('pre[data-evidence-file]')){if(pre.dataset.loaded)continue;try{if(typeof textFile==='function'){pre.textContent=await textFile(pre.dataset.evidenceFile)}else{let response=await fetch(pre.dataset.evidenceFile);if(!response.ok)throw Error('HTTP '+response.status);pre.textContent=await response.text();}pre.dataset.loaded='1'}catch(error){pre.textContent='Evidence could not be loaded: '+error.message;}}} |
There was a problem hiding this comment.
P2 — Reuse the existing evidence loader instead of redefining it. generate_cohort_diagnostic concatenates the shared report.js before this asset, and that file already defines loadDiagnosticEvidence and calls it inside openCallLog. This definition overrides the shared loader, dropping its in-flight guard and large-output textarea handling; the new click handler then calls it a second time. Opening any unloaded lazy-evidence dialog therefore starts two reads, and large evidence is inserted into a <pre> instead of using the existing bounded large-output path. Extend the shared loader for packaged textFile support if necessary and let openCallLog invoke it once.
| if content_scope is not None: | ||
| from lexbench_webfetch.content_scopes import select_content_scope | ||
| raw_cases = [select_content_scope(item, content_scope)[0] for item in raw_cases] |
There was a problem hiding this comment.
P2 — Apply selection for case-local scopes as well. With no CLI or dataset-level override, this branch is skipped even when a case declares content_scope='main' (or profiles defaulting to full). validate_case() calls the selector but discards its returned case, while score_attempt() does apply it. Consequently acquisition and scoring can use different requirements: for example, Exa's diagnostic request reads case.acceptance_contract to enable rich links, and Cloudflare reads case.expected_resources to request video links. Select each case unconditionally, passing the optional dataset override, before constructing EvalCase and saving the effective payload; the selector already preserves unannotated legacy cases.
| headings=[h['text'] for h in reference.get('document_structure',{}).get('headings',[])] | ||
| except (KeyError,ValueError,TypeError): | ||
| headings=[] | ||
| add('pattern',t('document_headings_not_preserved','Document headings not preserved'), |
There was a problem hiding this comment.
P2 — Do not equate every document-structure failure with missing headings. evaluate_document_structure also fails when document_body_chars is below document_minimum_body_chars, even when every heading passes and coverage is 100%. That valid scorer result now produces “Document headings not preserved” with “Heading coverage: 100%”, hiding the actual failed check. Distinguish heading failures from insufficient section-body content and display the saved body count and minimum when that predicate fails; add a control with complete headings but insufficient body text.
| if (not rendered | ||
| or [c.get_text() for c in rendered.select('thead th')][1:] != spec['headers'] | ||
| or [[c.get_text() for c in r.select('td')][1:] for r in rendered.select('tbody tr')] | ||
| != [[diagnostic_cell_text(v) for v in r] for r in spec['rows']]): |
There was a problem hiding this comment.
P2 — Validate structured table-link destinations as well as their labels. Replacing an overview table's case link with another existing attempts/*.html route, changing its issue fragment, or removing the anchor while preserving its text passes this comparison. The later generic link check only verifies that a target file exists and ignores fragments, so it does not catch these mutations. This leaves the newly supported diagnosis indexes able to point at the wrong evidence while the release check succeeds. For each case_id/issue_id cell, require the corresponding anchor and exact expected href, and add negative controls that retain the label but change/remove the destination.
| if model.get('problem_focus'): | ||
| expected_examples = [example for block in model['pages'].get('guide', []) |
There was a problem hiding this comment.
P2 — Validate authored diagnoses whenever they are present, not only in problem-focused reports. validate_diagnostic_blocks accepts examples and figures without problem_focus, and generate_cohort_diagnostic.blocks() renders them in ordinary guides as well. For such a report, deleting an entire diagnosis article (or figure) leaves the legacy specimens intact and passes this release check. Run the model-to-rendered example/figure checks independently of this flag, keeping only the requirement for a nonempty diagnosis inventory conditional on problem_focus. Add a negative control that removes an authored diagnosis from a normal guide.
| if comparison_mode: | ||
| summary_rows=comparative_rows(summary_rows,{3:('delivery','max'),4:('acceptance','max')}) |
There was a problem hiding this comment.
P2 — Compare success rates rather than raw counts, or require matched populations before enabling comparison mode. comparative_rows() uses each cell's data-sort, which here contains delivered/accepted counts, and diagnostic validation allows different cohort totals. Thus 8/10 is highlighted over 7/7 even though its acceptance rate is lower. Normalize the comparison metric by the cohort total while retaining the displayed numerator/denominator, or reject unequal populations; add an unequal-denominator control so winner highlighting cannot reverse the result.
| if main.get("forbidden_any", []) != full.get("forbidden_any", []): | ||
| raise ValueError("Content scope cannot change blocked-page rejection rules") |
There was a problem hiding this comment.
P2 — Preserve the original case's invariant constraints, not just agreement between profiles. Both profiles can omit an existing forbidden_any list or supply the same altered source_benchmark identity and pass these checks. The subsequent defaults/update then removes the blocked-page rejection rules or replaces the source/navigation metadata used by scoring. This violates the selector's identity-preservation contract even though main and full agree with each other. Validate the invariant fields against the original case (or inherit them outside the replaceable acceptance surface), with regression controls where both profiles omit or identically alter an original constraint.
| effective = deepcopy(case) | ||
| effective.update(deepcopy(REQUIREMENT_DEFAULTS)) | ||
| effective.update(deepcopy(profiles[selected]["requirements"])) |
There was a problem hiding this comment.
P2 — Include the ordered-tree acceptance fields in scope replacement. ordered_tree_reference and ordered_tree_min_similarity survive this update from the original case, but are also rejected as profile keys because they are absent from REQUIREMENT_DEFAULTS. Consequently, a task with a full-page tree threshold still evaluates that full-page tree after selecting main; score_attempt() applies the inherited threshold to content_pass (scoring.py:346–368), so an answer satisfying the independently annotated main requirements can fail on excluded content. Allow scope-specific tree contracts and clear inherited tree requirements when replacing the acceptance surface, or explicitly reject this combination rather than silently retaining the full-page check.
There was a problem hiding this comment.
This review found 5 issue(s), including 5 blocking issue(s).
| if saved_scope is None and len(task_scopes)==1: | ||
| saved_scope=next(iter(task_scopes)) |
There was a problem hiding this comment.
P2 — Do not promote a filtered population's task scope to a dataset-wide override. A source dataset without a top-level scope can validly contain an annotated task A (defaulting to full) and a historical unscoped task B. Running with --case-ids A saves only A, so this inference selects full on rescore. load_dataset then applies that override to every source task before the runner filters the population, and B raises Explicit content scope requires annotated main and full profiles. Thus an unchanged dataset cannot rescore this supported filtered run. Restore saved scopes per executed task rather than applying an inferred scope to unrelated source tasks, and add a mixed scoped/unscoped filtered-run regression.
| coverage=metrics.get('document_heading_coverage') | ||
| if coverage is not None and coverage < contract.get('minimum_heading_coverage',1): | ||
| headings=[h['text'] for h in contract.get('headings',[])] | ||
| add('pattern',t('document_headings_not_preserved','Document headings not preserved'), |
There was a problem hiding this comment.
P2 — Heading coverage is not a heading-presence metric: document_structure.py counts a heading as supported only when its required section body is long enough, and separately records document_heading_results[].present. An output containing every heading but undersized sections therefore gets reported here as “Document headings not preserved”; if total body length meets the global minimum, no body finding is added either. Use the per-heading present/pass results to distinguish absent headings from insufficient section text, as required by this change.
| path = contained_path(source, figure['path'], reject_symlinks=True) | ||
| raw = path.read_bytes() |
There was a problem hiding this comment.
P2 — Hash-bound figures cannot be built through the supported diagnostic-input-file path. build_diagnostic_cohorts() calls prepare_diagnostic_evidence(spec_path, staging) and then changes source to staging, but preparation only writes case evidence and report.json; it never copies spec['report']['pages'][...]['figure'] assets. This new read therefore raises FileNotFoundError for an illustrated input manifest even when its PNG exists alongside the input. Materialize and hash-check declared figures from the input root during preparation, and cover the file-input build path (not only preassembled report directories).
| expected_examples = [example for block in page_blocks | ||
| for example in block.get('examples', [])] | ||
| rendered_examples = diagnosis_page.select('[data-diagnosis-case]') | ||
| if (model.get('problem_focus') and name=='guide' and not expected_examples) or len(expected_examples) != len(rendered_examples): |
There was a problem hiding this comment.
P2 — The concrete-case requirement can be bypassed by omitting pages.guide entirely. This condition runs only for existing pages entries, while problem_focus also disables the original specimen checks and replaces the rendered guide with blocks('guide'). A model with an explicit interpretation_lead and absent pages (or only an overview) can therefore pass this gate with no case diagnosis on its guide. Check for a nonempty guide example inventory outside this loop whenever problem_focus is enabled; add rejection coverage for both missing pages and missing guide, not just malformed examples.
| guide+=section(t('conclusions_and_check_scope', 'Conclusions and check scope'),(f"<p>{esc(model['scope_note'])}</p><p>" + t('source_page_observations_and_output_checks_are', 'Source-page observations and output checks are kept separately. When an independent browser hits a verification page or access problem, this neither counts as a pass for the tested tool nor establishes that the tool is wrong. {link}', link=link('review/index.html', t('view_check_scope_and_handling', 'View check scope and handling →'))) + '</p>'),'guide-conclusions') | ||
| if model.get('problem_focus'): | ||
| core=section(t('results_and_measurement_scope','Results and measurement scope'),table([t('task_group','Task group'),t('implementation_version','Implementation version'),t('tasks','Tasks'),t('call_success','Call success'),t('pass','Pass'),t('mean_latency_s','Mean latency (s)'),t('p50_latency_s','P50 latency (s)'),t('p95_latency_s','P95 latency (s)')],summary_rows,'guide-summary')+'<p>'+esc(model['metric_note'])+'</p>','guide-results') | ||
| guide=hero(esc(model.get('guide_title')or model['title']),esc(model.get('interpretation_lead')or model['subtitle']))+core+blocks('guide')+section(t('conclusions_and_check_scope','Conclusions and check scope'),''.join('<p>'+esc(value)+'</p>'for value in model.get('guide_conclusions',[model['scope_note']])),'guide-conclusions') |
There was a problem hiding this comment.
P2 — The new problem-focused fallback disagrees with the release checker. With problem_focus=True and no interpretation_lead, this renders model['subtitle'], but check_diagnostic_report() still requires the lead to start with the numeric passed / total prefix. Neither model validation nor the renderer requires an explicit lead, so an otherwise valid model using this fallback fails build_diagnostic_cohorts() at its final check. Use the same numeric fallback as the ordinary guide (or define and validate one consistent alternative), and cover a problem-focused build without interpretation_lead.
There was a problem hiding this comment.
✅ Re-review complete · a59d90d
No findings in this incremental review.
|
P0/P1 re-review of head Checks:
Earlier findings now fixed:
Below P1, for context:
Before merging: the paired dataset PR (HF #6) still has three P1 issues, listed on that PR. It needs rebuilding on dataset v2.2.0 and pinning to this PR's merged commit. |
|
The paired dataset PR has been updated: HF #6, head
For the two nonblocking code observations: the certified runner calls The annotation cohort explicitly identifies its unreleased dependency on this PR at Server-side follow-ups, with Chinese reproductions and attached diagrams: setaria #95. |
There was a problem hiding this comment.
✅ Re-review complete · 085f06b
No findings in this incremental review.
Tasks can declare independently bound main/full acceptance profiles and select them before acquisition or scoring. Rescoring retains the dataset and per-task scopes saved with the run, including filtered mixed scoped/legacy populations. Both profiles preserve the original blocked-page and navigation constraints; unsupported inherited full-page tree checks fail explicitly. Public definitions retain approved reference bindings and omit undeclared evidence payloads.
The maintained diagnostic renderer adds issue guides with hash-bound figures and concrete cases: requirements, observed output, evidenced cause, repair and acceptance checks. Category and issue indexes link to the exact diagnosis, saved result or validated external repair issue. Primary figures lead their sections without folding; release checks reject folded or displaced figures and changed issue destinations. Details distinguish missing headings from insufficient section text, expose target media and file checks, and compare rates when cohort sizes differ. Illustrated input manifests carry their hash-bound PNG assets into the build. Publication retains and rechecks the planned population. Packaged issue filters and the shared guarded evidence loader work through virtual routes; figures use bounded parallel reads.
Validation of the latest presentation change: 43 report-template tests and 82 subtests, including negative controls for hidden/displaced diagrams and unsafe or incorrect issue links; Ruff and diff checks passed. Earlier scope, scoring, CLI, package and navigation checks are recorded in the PR review replies. All 1,060 main/full requirement selections in the report remain identical to the frozen selector; no provider calls were added. The scoring changes belong to the next minor software release under
docs/releases.md.Related deliverables:
Measured responses, report prose and generated figures remain outside Git.