Repository navigation
fix(webfetch): unify source validation and bound browser capture - #61
Conversation
lanyue-llk
changed the title
da7fc57 to
1e64eb2
Compare
1e64eb2 to
adac91a
Compare
lanyue-llk
left a comment
lanyue-llk
left a comment
There was a problem hiding this comment.
Review conclusion
Fail. The PR aims to enforce explicit live-reference policies across task kinds, reuse declared content requirements for source qualification, and bound browser capture. The new live-contract path still permits certificate reuse after acceptance rules change, and the subprocess boundary introduces two capture defects.
Findings and recommendations
- [P1]
harness/web_fetch/reference_lifecycle.py:150: Bind the newly supported acceptance contracts to reference scope; an ordinary rescore currently accepts changed rules under the original source certificate. - [P2]
harness/web_fetch/browser_observations.py:631: Resolve caller-relative browser paths before changing the worker directory. - [P2]
harness/web_fetch/browser_observations.py:649: Clean up the worker's detached Chrome processes when the driver cannot exit.
The inline comments give reproductions and minimal fixes. Source/reference tests passed (138 tests, 324 subtests), adjacent lifecycle tests passed (104 tests, 42 subtests), and real-Chrome capture tests passed (48 tests, one skipped); lint passed. The additional scenarios below expose gaps in those controls.
The production changes extend shared owners and do not hard-code a report instance. The case-ID branches in _entry_facts construct historical test inputs; they are not production report routing. Extra class hierarchies would not address these defects. Both the PR title and description follow the repository's conventions.
|
✅ OpenCodeReview: Review complete: 0 finding(s) across 1 selected item(s). |
![lexmount-ocr[bot]](https://avatars.githubusercontent.com/u/277637559?s=60&v=4){kind=small}
lanyue-llk
left a comment
lanyue-llk
left a comment
There was a problem hiding this comment.
Review conclusion
Fail. The original acceptance-change bypass and caller-relative browser path defect are fixed, and the ordinary hung-driver cleanup controls pass. The latest version still rejects valid historical evidence and cannot complete its newly admitted mixed text/resource live-reference flow. Cleanup also remains incomplete when neither process-discovery utility is available.
Findings and recommendations
- [P1]
harness/web_fetch/reference_lifecycle.py:224: Version the expanded scope binding and validate unchanged legacy contracts against their original hash-bound dataset. Adding fields to the existing scope format invalidates previously valid certificates and observations. - [P1]
harness/web_fetch/reference_lifecycle.py:150: Carry declared text requirements through resource observation and freezing before enabling these tasks forlive_exact. - [P2]
harness/web_fetch/browser_observations.py:660-674: Establish a usable ownership-discovery mechanism before launching captures; the remaining cleanup case is documented in the original cleanup thread.
Source/reference suites passed: 152 tests and 327 subtests. Real-Chrome capture suites passed: 78 tests, one skipped. Lint and the isolated installed-wheel path check passed. Additional controls reproduce the two lifecycle failures below and the remaining detached-browser leak. Neither theoretical PID races nor test-fixture case IDs are included as findings.
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.
✅ Re-review complete · 05bcba2
No findings in this incremental review.
05bcba2 to
bcd5b18
Compare
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 · c5ee0a8
No findings in this incremental review.
PR61 fixes how Web Fetch obtains and qualifies source evidence. A new task could skip its requested live check; a resource task could retain an image but lose its article text; a stalled browser could block the remaining cases. PR57, now merged into main, handles the later reuse and merge of measured results shown above.
Use the same declared-content checks for reference freezing, live preflight and dynamic-source validation. Mixed text/resource tasks must retain and qualify both parts.
Give new source scopes an explicit version. Reuse old certificates and observations only with their original hash-bound inputs, retaining those inputs through rescore and evidence copying. Changed or removed rules require new authority.
Bound each capture worker, preserve failures in checkpoints and continue with the next case. Resolve browser paths from the caller and clean up owned browser groups, including detached and late-starting children. Check that process discovery works before starting a batch.
Exclude hidden fallback text from visible captures while preserving explicitly requested DOM content.
Use absolute imports across the repository and move ordinary imports to module headers. Preserve runtime dependency lookup, keep optional dependencies and initialization lazy, and enforce the absolute-import rule in lint. Standalone workers also run from source checkouts without an installed harness.
Validation: the full repository suite and installed-package checks cover all tracks. Real Chrome controls exercise mixed text/resource capture, missing content, stalled drivers and owned-process cleanup. Actual legacy fixtures cover unchanged inputs, changed or removed rules, conflicting embedded rules and unknown scope versions. A separate CowAgent process runs with site packages and inherited Python paths disabled. The latest focused regression passed 308 tests and 33 subtests; one optional historical-input fixture was skipped. The 18 synthetic route matrices retain the same behavior; their baseline now binds the updated source bytes.
Full CI runs on
c5ee0a88, including installed-package and real-browser checks.