fix: report the true function count, not the capped inventory slice#287
Conversation
Every decompiler probe truncated the returned function-name list — Ghidra to 400, radare2 to 200 — and the tools reported that slice length as the total. `re_list_functions` paged over the truncated list and told the agent "400 total" as if complete, and a decompile fallback said an address was "not found among the 400 defined functions". On a firmware with thousands of functions, agents that enumerate by listing never saw past the first 400/200, and the decompile messages misrepresented the binary's size. The `[:400]`/`[:200]` was only a payload bound (names are cheap), never a real analysis limit — targeted decompile-by-address/name always searched the whole program. So: - Probes now return the full name inventory (up to a defensive 20000 bound) plus `functions_total`, the true whole-program count: pyghidra_lib (headless + the resident-bridge list op), the r2 decompile_probe, and both ghidra_bridge ops. - `re_list_functions` reports functions_total as the count and pages over the real inventory; if the returned set is ever bounded below the true count, it says how many are withheld instead of implying completeness. - `focus_only_payload` keeps only a bounded sample of the name list (with functions_total) so the fuller inventory doesn't bloat every per-function decompilation Observation — the full list stays in its own function_list one. - The decompile fallback reports the true total and gives an ADDRESS miss its own message: it explains the address isn't inside any defined function and points at re_disassemble_range (raw disasm) + re_decompile_at(reanalyze=True), instead of dumping a function-name list that can't help an address lookup. A NAME miss shows a bounded sample marked "+N more". Tests: true-total reporting on a capped inventory (list_functions + the decompile fallback), and the distinct address-miss message. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
branover
left a comment
There was a problem hiding this comment.
Independent merge-gate review — PR #287
Verdict: APPROVE — ready to merge. No blocking correctness or security issues. Two optional LOW/consistency nits below; neither needs to hold the merge.
Reviewed main...HEAD (8 files, +127 / −33) with both /code-review (high) and /security-review. CI is green on all 7 checks, including the live sandbox and Ghidra decompile gates that exercise the real probe paths.
What I verified (all correct)
functions_totalis plumbed consistently across all four probe paths. pyghidradecompile_core(len(all_names)) and the resident-bridgelistop (len(names)); the r2decompile_probe(len(functions)); and bothghidra_bridgeops (_RemoteOpslen(names),_ManagedOpspassthrough). For_ManagedOps.decompilethe value comes from the resident server'sresp— I traced it end to end: thedecompileop runsdecompile_core, which now setsfunctions_total, soresp.get("functions_total")is populated. Theping/listops also set it. Good.- Every user-facing total is defensively guarded.
_format_decomp,_decomp, and_list_functionseach readfunctions_totalbehindisinstance(x, int)+ a< len(...)floor, so all five cases resolve correctly: nofunctions_total(old fakes / stale server) falls back tolen; present-and-equal uses it; capped (functions_total > len) reports the true total; and a bogus/too-small value can't underflow belowlen. - Pagination math is sound. The
pageable(matches in the returned inventory) vsgrand_total(true count) split is clean;offsetclamps topageableexactly as before (pageable == old total);more = next_offset < pageable;withheld = grand_total − len(names). No off-by-one. The "…N more" note counts pageable remainder; the withheld note is separate and only fires when the inventory is actually capped. - The 400-sample cap in
focus_only_payloadbreaks no reader. I confirmed the only consumers of adecompilationObservation's payload readfocus, neverfunctions:enrichment._extract_functionstakes thefocusbranch (a decompilation payload always has a truthy focus), andobservations.search_decompiledreads onlypayload["focus"](obsobservations.py:327). The whole-program name list thatbinutils.search_symbols_projectreads comes from the separatefunction_listObservation, which is recorded uncapped (up to 20000) — an improvement over the old 400/200. - The address-miss vs name-miss discriminator is correct.
label.startswith("address ")matches the caller contract:decompile_atbuildsf"address {addr}",decompile_functionbuildsf"function {fn!r}"(a function literally named "address …" still renders asfunction '…', so no collision). The address branch correctly omits the useless name-list dump and points atre_disassemble_range+re_decompile_at(reanalyze=True). function_list_pagetotalchange is safe. That kind is write-only (no engine consumer, no extractor), so switchingtotaltogrand_totalwhen there's no pattern affects nothing downstream; the payload also now carriesgrand_totaland the page length.- Payload size is acceptable. A
function_listObservation now holds up to 20000 names (~600 KB worst case), stored once and CAS-deduped by content hash;record_observationimposes no size cap that would reject or truncate it. The per-functiondecompilationObservations stay small via the 400 sample. Reasonable trade ("names are cheap"). - Security invariants intact. This is purely returning more function names (already-exposed static-analysis metadata carried in
TaskContext, never raw target bytes) from probes that already ran in the sandbox. No new execution, no egress, no secret handling, no bind/loopback change, no policy-seam touch./security-reviewfound nothing exploitable (HIGH or MEDIUM). - Docs/migration correctly omitted. No model change (no migration needed) and no UI/tool-surface change (no
ux-contract.md/docs/mcp.mdupdate needed) — consistent with the CLAUDE.md gate. - Tests cover the two behaviors that matter: true-total-vs-capped reporting (
test_list_functions.py,test_decompiler_fallback.py) and the distinct address-miss message (test_address_access.py,test_decompiler_fallback.py).
Findings (both LOW / non-blocking)
1. LOW — focus_only_payload doesn't coerce a present-but-None functions_total (src/hexgraph/sandbox/decompiler.py:42).
"functions_total": (out or {}).get("functions_total", len(fns)) only falls back to len(fns) when the key is absent. _ManagedOps.decompile returns functions_total: resp.get("functions_total"), which is None if a stale resident bridge (pre-rebuild image) doesn't emit it — so the stored decompilation payload gets functions_total: null instead of len(fns). The three user-facing sites all use an isinstance(int) guard; this one doesn't, so it's inconsistent. Impact is cosmetic today (nothing reads functions_total off a decompilation Observation; no message misreports), but normalizing it here — e.g. t = (out or {}).get("functions_total"); ... len(fns) if not isinstance(t, int) else t — would match the other sites and avoid a latent surprise if a future reader trusts that field. Optional.
2. LOW — the reanalyze inline output still reports the capped slice length, not functions_total (src/hexgraph/agent/agent_tools.py:942-943).
fns = out.get("functions", []) / return _clip(f"re-analyzed ({len(fns)} functions):\n" ...) reports len(fns), which is capped at MAX_FUNCTION_NAMES (20000). This is the same "slice length reported as the total" bug the PR fixes everywhere else, left in this one inline path — so on a monolithic binary with >20000 functions the reanalyze tool would say "re-analyzed (20000 functions)". The Observation summary this path records (line 689) already uses the correct ftotal, so only the inline tool text is affected, and only above the 20000 defensive bound (rare, but reachable on a large statically-linked blob). Consider reporting out.get("functions_total") (guarded) here too for consistency. Optional.
Test gap (minor, non-blocking)
The new assertions exercise the consumer side (_list_functions, _format_decomp). No unit test asserts the functions_total field on the r2 decompile_probe output or on the two ghidra_bridge ops directly (the existing bridge tests assert functions equality but not functions_total). The plumbing is a trivial len(), and the live sandbox + Ghidra CI gates exercise the real probes, so this is low-risk — worth a one-line assertion if convenient.
Nothing here blocks the merge. Nice, well-scoped fix with clear intent-documenting comments.
Follow-ups from the merge review of #287: - focus_only_payload derived functions_total with `.get("functions_total", len(fns))`, which defaults only on a MISSING key — but _ManagedOps.decompile returns `functions_total: resp.get(...)` = None from a stale resident bridge, so the stored payload got functions_total: null. Guard with isinstance (< len floor) to match the three user-facing sites. - The reanalyze tool's inline text still reported len(fns) (the capped slice) as the count — the same slice-as-total bug fixed elsewhere. Report functions_total and mark the 300-name body as a sample. Tests: a direct focus_only_payload unit test (present-but-None, real total + sample cap, missing key). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
branover
left a comment
There was a problem hiding this comment.
Re-review of b3849a7 — both nits resolved
Verdict: APPROVE — merge-ready. Reviewed the delta a21e6da..b3849a7 (agent_tools.py, decompiler.py, +1 test; +39/−3). Both LOW nits from the first pass are correctly fixed, and neither fix introduces a regression or a new issue.
Finding #1 — focus_only_payload None-guard (src/hexgraph/sandbox/decompiler.py). Now derives functions_total via ftotal = (out or {}).get("functions_total"); if not isinstance(ftotal, int) or ftotal < len(fns): ftotal = len(fns) — the exact guard used at the three user-facing sites (_format_decomp, _decomp, _list_functions). A present-but-None value from a stale _ManagedOps.decompile now falls back to len(fns) instead of being stored as null, and ftotal is correctly computed from the full returned list (len(fns)), not the 400-name sample. The new unit test test_focus_only_payload_guards_none_functions_total covers all three branches (present-but-None → len, real total preserved + sample cap enforced, missing key → len). Resolved.
Finding #2 — reanalyze inline text (src/hexgraph/agent/agent_tools.py). The reanalyze handler now reports the guarded functions_total instead of len(fns), and appends "; showing first 300" when ftotal > 300 so the 300-name body reads as a sample. The error guard (if out.get("error"): return out["error"]) and fns binding are untouched. Boundary is correct: at exactly 300 nothing is withheld and no note fires; above 300 (including a >20000 capped monolith) it reports the true total with the sample note. Resolved.
No new correctness or security concerns — still purely returning static-analysis function names (no execution/egress/secret/bind change; /security-review clean). Minor, non-blocking: the reanalyze text change has no direct unit test of its own, but it's a trivial mirror of the same guard pattern already covered by the list_functions / fallback tests.
Clearing my two review threads. Good to merge once CI is green on b3849a7.
What
Every decompiler probe truncated the returned function-name list — Ghidra to 400, radare2 to 200 — and the tools reported that slice length as the total.
re_list_functionspaged over the truncated list and told the agent "400 total" as if complete, and a decompile fallback said an address was "not found among the 400 defined functions". On a firmware with thousands of functions, an agent that enumerates by listing never saw past the first 400/200, and the decompile messages misrepresented the binary's size.Why it's safe to lift
The
[:400]/[:200]was only a payload bound (names are cheap), never a real analysis limit — targeted decompile-by-address/name always searched the whole program viagetFunctionContaining()/ the full record set. Only enumeration and the display list were capped.Fix
20000bound) plusfunctions_total, the true whole-program count:pyghidra_lib(headless + the resident-bridgelistop), the r2decompile_probe, and bothghidra_bridgeops.re_list_functionsreportsfunctions_totalas the count and pages over the real inventory; if the returned set is ever bounded below the true count, it says how many are withheld instead of implying completeness.focus_only_payloadkeeps only a bounded sample of the name list (withfunctions_total) so the fuller inventory doesn't bloat every per-functiondecompilationObservation — the full list stays in its ownfunction_listObservation.re_disassemble_range(raw disasm) +re_decompile_at(reanalyze=True), instead of dumping a function-name list that can't help an address lookup. A name miss shows a bounded sample marked "+N more".Verification
just test(fast tier): 1699 passed, 6 skipped, 14 deselected.tests/test_list_functions.py,tests/test_decompiler_fallback.py) and the distinct address-miss message (tests/test_decompiler_fallback.py,tests/test_address_access.py).No schema/migration/UI changes; no tool rename/addition.