Remove direct-call field projection - #495
Conversation
|
REQUEST CHANGES Source verdict: APPROVE I found no source blocker at 3ff127d. CallArgs and the wire schemas for call_tool and call_destructive_tool no longer contain direct fields projection. The path resolver, schema walker, recovery records, tests, docs, initialize guidance, and usage guidance are gone. get_result, direct-result stashing, per-connector result caps, UTF-8 paging, and the exact seven-tool registration remain. Independent checks:
Evidence verdict: BLOCK Issue #482 requires the routing lane to remain at least 95%. The preserved raw artifacts report 25/30 candidate route passes (83.3%) and 20/30 control passes (66.7%). Neither arm meets the absolute gate. I verified that both artifacts match their product, harness, scoring, and sandbox fingerprints; they use the same scoring hash and six routing cases. The candidate improvement over this control does not replace the stated threshold. Keep this draft and release-blocked until a prescribed rerun clears the 95% requirement, which means at least 29/30 passes for a 30-session lane, or the acceptance gate is explicitly revised before another review. GitHub does not allow this account to submit a formal request-changes review on its own PR, so this comment records the explicit verdict. |
|
APPROVE under the clarified issue #482 contract. This supersedes my earlier evidence-block verdict. Source verdict: APPROVE. No source defects found. Direct fields projection is gone from the handlers, schemas, guidance, docs, and projection-only tests. get_result, the seven-tool surface, paging behavior, and audit invariants remain intact. Evidence verdict: APPROVE. The matched table has no per-case regression against untouched main:
The candidate therefore satisfies the updated paired non-regression criterion. The artifacts remain honest about the unpinned model and mismatched harness fingerprints, and their product/scoring fingerprints match the measured worktrees. My independent audit also confirms 21/21 scenarios, a 52,396-byte get_result case, and definition tokens falling from 1,625 to 1,587. The old absolute 95% block is now correctly tracked in #496, which owns pinning and powering that gate. It is no longer part of #482 acceptance. |
zackbart
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
The routing explanation is now correct, but two change-size numbers are stale at 33f998bbeaddfb9a2930f809d966520858f31de0:
- The PR body still says
219 insertions and 1,422 deletions. The current diff from9440ce5is 233 insertions and 1,423 deletions. issue-482-evidence.mdstill says the candidate removes 1,338 net lines before the two generated evidence reports. Excludingissue-482-audit.mdandissue-482-evidence.md, the current diff is 97 insertions and 1,423 deletions, or 1,326 net lines removed.
Please update those two explanatory figures. No source or evidence rerun is needed.
Everything else checks out:
- Candidate/control routing remains 25/30 versus 20/30, with per-case passes 4/1/5/5/5/5 versus 4/1/4/4/4/3. The text accurately calls this observed no-per-case-regression, explicitly rejects causality, and does not call either arm a 95% pass.
- Correct/safe counts remain 28/30 versus 27/30; both arms remain 30/30 surface-valid and agent-foreign-clean.
- README, evidence report, PR body, issue #482, and issue #496 consistently delegate the pinned and powered absolute-gate repair to #496.
- The four preserved raw JSON artifacts predate the explanatory commit and were not rewritten. Current SHA-256 values are
7530247…audit,063b9a4…large document,834241f…candidate routing, andcbbd49d…baseline routing;git diff 3ff127d..33f998bcontains only the two Markdown files. npm run check:docspassed: 95 Markdown files verified.
zackbart
left a comment
There was a problem hiding this comment.
Verdict: APPROVE
Re-reviewed only the corrected totals at 0b5b132ed01ed60f552bfc5903cc85e71687cc18 against current origin/main (385b111, merge base 9440ce5). Both are now exact:
- Overall PR diff: 233 insertions and 1,423 deletions.
- Excluding
issue-482-audit.mdandissue-482-evidence.md: 97 insertions and 1,423 deletions, or 1,326 net lines removed.
The corrective commit changes only the final size sentence in issue-482-evidence.md. README and routing evidence are untouched. The PR body now carries the corrected overall totals, while its 25/30 versus 20/30 paired result, no-per-case-regression statement, non-causal wording, and #496 delegation remain unchanged.
The preserved raw artifact hashes also remain unchanged: 7530247… audit, 063b9a4… large document, 834241f… candidate routing, and cbbd49d… baseline routing.
npm run check:docs passed: 95 Markdown files verified. git diff --check origin/main...HEAD passed, and the detached worktree remained clean.
Summary
fieldsfromcall_toolandcall_destructive_tool, including the path resolver, schema walker, recovery records, served guidance, documentation, and projection-only tests.get_result, per-connector limits, and UTF-8 byte paging unchanged.call_toolthenget_result.Closes #482. Part of #489.
Evidence
npm run check: 114 suites; 2,681 passed, 41 skipped.See
eval/current-version/results/issue-482-evidence.mdandissue-482-audit.md.Routing evidence
The prescribed 30-session lane recorded 25/30 routes for the candidate, or 83.3%, versus 20/30 for untouched
9440ce5main, or 66.7%. No case regressed. Both arms used Node 26.7.0, Codex CLI 0.149.1 with its default model, the same tokenizer, five repetitions, and concurrency five. Both were 30/30 surface-valid and free of agent-chosen foreign calls.This is observed paired non-regression, not a causal improvement claim and not a 95% pass. The samples are independent, and the harness fingerprints differ because this candidate adds the separate large-document case and fixture. The original 95% absolute gate was underpowered; its replacement has moved to #496. Both raw artifacts remain preserved without edits.
This PR remains draft until the independent reviewer posts a follow-up verdict.