Compare complete Boolean source functions and literal Agent membership - #606
Conversation
pengfei-threemoonslab
left a comment
There was a problem hiding this comment.
Independent coding-agent review, round 1, at 2c539f0 (base 8265576). This is a coding-agent COMMENT review, not human approval, release qualification, or merge authority.
I verified GitHub's current head and all 10 changed-file blob SHAs against the reviewed snapshot. One P2 finding remains: repeated helper calls multiply evaluation cost beyond the intended bounded source model.
Validation: a counting probe against the actual static reader returned observed with 256 return rows and exactly 53,760 helper leaf evaluations for eight Boolean tool parameters, a ten-literal helper, and twenty repeated helper calls, although the helper has only two possible inputs. A separate AST-only construction fits the existing byte and AST limits but would require 809,984,000 helper leaf evaluations; I did not run that expensive case. Fixture application/SDK code was never imported or executed, and no worktree files or control artifacts were changed.
The remaining reviewed source semantics, separate return/binding comparisons, redaction, and unknown-case handling are coherent within this deliberately narrow profile. Complete capability dependencies, deployed wiring, finding-predicate attribution, historical qualification, and #557/#515 closure remain unproved, as the PR states. I did not rerun the full suite or verifier during this review.
| arguments = [_expression(arg, names, None, None, depth + 1) for arg in node.args] | ||
| return lambda values: guard[1]( | ||
| {name: arg(values) for name, arg in zip(guard[0], arguments, strict=True)} | ||
| ) |
There was a problem hiding this comment.
[P2] Bound helper evaluation independently of repeated call sites
Each supported call re-evaluates the entire helper expression for every tool input. The separate 4,096-node limits therefore multiply: a helper returning an and of 4,000 True literals is 36,030 bytes / 4,009 AST nodes; a tool with eight Boolean parameters, its required early guard, and an and of 790 allow(p0) calls is 11,327 bytes / 4,010 nodes. Both are admissible, but this path performs 256 × 791 × 4,000 = 809,984,000 helper leaf evaluations during one source model. A small actual-reader counting probe confirmed the same multiplication (53,760 evaluations for 10 literals and 20 repeated calls).
Compile and structurally validate the helper once, then bind its finite truth table (at most 256 parameter tuples) or memoize by the complete ordered Boolean argument tuple, so call sites perform bounded lookups. Preserve validation of every operand before evaluating any input, including unsupported expressions behind short circuits. Add a count/budget regression for a repeated-call fixture rather than a timing-sensitive test.
|
Addressed review 5149940982 in The helper's entire expression is validated first, then each possible Boolean helper input is evaluated once and calls use positional table lookups. The full admissible 4,000-leaf / 790-call fixture reproduced 202,496 helper invocations before the fix; it now requires exactly two, and its unstubbed static read produces the identical model. This eliminates multiplicative helper recomputation while preserving validation of unsupported/unreachable operands. Final full local suite: 9,347 passed, 5 skipped; 113 focused checks passed; Ruff, generated artifacts and committed current-base verification passed. The independent reviewer rechecked the fix and confirmed GitHub's final head and all 10 published blobs. Only the two reviewed fix files changed; no remaining actionable findings. The recheck belongs to this address loop, not a new formal review or human approval. This completes one review/address loop in #606 and three total for #557 across #599/#606 in this implementation pass. Final-head CI, aggregate coverage and a fresh current-control check remain required before merge. #607 records the newly isolated operation/predicate relationship for later; #557/#515 remain open and no finding-exclusion or release qualification claim is made. |
An unchanged first guard can hide a changed continuation or different Agent tool membership. This extends #599's guard evidence with a bounded
sdk_boolean_function/v1model that inspects the entire selected function, a pure imported predicate and one literal source Agent binding. Reports compare return values, true-return input sets, tool membership and the bound true-return relation separately.Every statement and branch is inspected before evaluating the finite Boolean model. Unknown calls, configuration/free variables, mutation, alternate bindings, unsupported signatures and missing or redacted evidence remain unresolved. The helper's entire AST is validated before its finite truth table is built; each possible helper input is evaluated once, independently of repeated calls. Input bytes use the existing verifier dependency identity; real committed base/head and stale-control tests exercise the path. Markdown and the generated report schema document the distinction.
A true return is not approval, effect or authority. All finding exclusions remain false and capability dependency coverage remains incomplete. This is the next bounded source-proof slice of #557, not its closure: operation-to-finding predicate attribution is now isolated in deferred #607; modeled external operations, the TypeScript
cal-1case, #563's reviewed historical scope and #515's scope/default migration remain open. No gate, declaration, policy, receipt authority or release qualification is changed.Validation: 9,347 passed, 5 skipped in the final full local suite; 113 focused reader/verification checks passed. Regressions cover narrowed-guard/widened-function, unchanged-guard/changed-membership, unreachable unsupported code, ambiguous identities, redaction and ignored-source currency. The full-size bounded helper fixture first reproduced 202,496 helper invocations, then required exactly two after the fix and compared the actual unstubbed result. Ruff, generated schemas, sample-golden checks and current-base verification passed.
Independent GitHub review 5149940982 found the helper-cost multiplication; the fix has been independently rechecked. This PR has one review/address loop, bringing #557 to three across #599/#606 in this implementation pass. Final published-head confirmation and author address are recorded in the conversation.
Refs #557, #515, #563, #607, #572.