Repository navigation
fix(harness): give a browser scan its limits cut a second look (TH-9512) - #281
khushalsonawat wants to merge 12 commits into
Conversation
The browser scan stopped reading an agent's code at any of its limits (file size, the scan, script and Python totals, the parse total, the token total and work, the decode and search totals, a skipped file, a package.json it couldn't read) and returned what it had, so padding read first or a large honest repository hid a plain browser import, silently. The scan now returns the limits that cut it, and a cut reading gets one second look with its own message before the model's answer stands; a cut never refuses on its own. One WARNING per cut scan names the limits and counts. The import walk's own stops are left to its own fix. Refs: TH-9512 Signed-off-by: khushalsonawat <khushalsonawat@gmail.com>
Signed-off-by: khushalsonawat <khushalsonawat@gmail.com>
… (TH-9512) Signed-off-by: khushalsonawat <khushalsonawat@gmail.com>
| def _scanned_agent( | ||
| source_root: Path, subfolder: str, reached: list[Path] | None | ||
| ) -> tuple[tuple[str, ...], frozenset[str]]: | ||
| """``browser_tools_in_agent``'s scan, and the limits that cut it. The walk's own stops aren't |
There was a problem hiding this comment.
P2. A stopped import walk isn't a cut, so padding still hides a browser import outside the agent's folder, with no look.
Repro (subfolder agents/a):
agents/a/m_0.pytom_5.py, each"import os\n" + "x = 1\n" * (100 * 1024 // 6)agents/a/z_main.py=import common.utilcommon/__init__.pyempty,common/util.py=import playwright
browser_scan(root, "agents/a") gives ([], frozenset()) and decide("chat", [], looked_again=False, cut=frozenset()) gives keep. Without the six files it finds playwright. The only signal is the walk's WARNING.
The Summary says "whenever any of the scan's limits or failures leaves a code file unread or unparsed, the scan says it was cut". "Left alone" defers the walk, and test_the_walks_stop_is_no_cut pins this. Bundling's walk has no limit, so common/util.py ships and runs.
Fix: count the walk's stop as a cut only when an import it didn't follow leads to a file whose bytes name a browser package, which avoids a look on most subfolder agents. Or reword the Summary and link the ticket that tracks this.
There was a problem hiding this comment.
Left as a stated limit, rather than a cut. Counting the walk's stops would put a paid look on most subfolder readings. #283 made the stops visible: a WARNING, and a 512 KB walk total. The PR's Summary now says a stopped walk isn't a cut, and "Left alone" states this gap and links #283; it's also in our older-findings register as a Medium. test_the_walks_stop_is_no_cut pins today's behaviour.
…total Brings in #269, #276 (through #283), #283 and #288. Three hunks against #288 (TH-9448) were resolved by hand, keeping both intents: - a skipped file logs its type once at WARNING, and its path and trace at DEBUG (#288); it is also counted as the scan's `skipped` cut, and that count is in the cut line, so #288's separate "skipped N files" line is not repeated (TH-9512); - #288's skip tests and its NUL-byte test now expect the cut line, which holds names and counts only. Tests on 3.12.3: test_agent_type_refusal 720, test_browser_scan_cut 59, test_contract_tools_subfolder 180, all passed. Signed-off-by: khushalsonawat <khushalsonawat@gmail.com>
… read package.json as Node does (TH-9512) - An oversize .py counts as file_size only when its bytes name a browser package, its coding line names a codec that isn't raw-safe or that Python refuses, or the search can't clear it: at most 4 MiB a file and 8 MiB a scan are searched, and a file past either bound, unreadable, not a file, or grown since its size was taken still cuts. It is never parsed. - package.json is read as Node reads it, UTF-8 with replacement characters and integers kept as text; invalid JSON means no dependencies, since npm refuses it too. Only OSError and RecursionError leave it unreadable, a cut. TH-9512 Signed-off-by: khushalsonawat <khushalsonawat@gmail.com>
…t/TH-9512-past-the-parse-total Signed-off-by: khushalsonawat <khushalsonawat@gmail.com>
…H-9512) - A file exactly at the per-file search bound, its needle in the last bytes, cuts. - An oversize module whose only tool is PlayWrightBrowserToolkit cuts. - Past what's left of the scan's search total, an oversize file isn't opened. TH-9512 Signed-off-by: khushalsonawat <khushalsonawat@gmail.com>
…the-parse-total agent_type_check.py, the skip handler: #291 drops the trace at every level; this branch counts a skip as the skipped cut. Kept both: no trace, the type once at WARNING, the count in the cut line. Signed-off-by: khushalsonawat <khushalsonawat@gmail.com>
…and cut only when that stops too (TH-9512) A file the walk reached but its parse total, parse size or script total kept from being parsed has its import lines searched (no parse) and followed under the other limits, with one 8 MiB search total for the walk. The scan's cut set gets import_walk only when a limit or that search's cap left code unread; a file the search covered is no cut. The WARNING stays. Tests pin Dileep's repro, a two-hop line-search chain, and the search cap. Signed-off-by: khushalsonawat <khushalsonawat@gmail.com>
…t after ';' or ':' (TH-9512) A folder .py over the 4 MiB search cap is no longer opened, and counts as an import_walk cut; every byte read is charged to the walk. The line search also reads an import after a ';' and after a compound statement's ':'. The parse-refusal search runs only when a search total is given. Signed-off-by: khushalsonawat <khushalsonawat@gmail.com>
…e search tries (TH-9512) The line search tries a compound statement's first 8 colons. A line with more before its import wasn't followed and wasn't cut, so the import went unseen with no second look. It is now a limit cut, like the walk's other caps; a long line with no import after its colons stays no cut. Signed-off-by: khushalsonawat <khushalsonawat@gmail.com>
|
|
||
|
|
||
| _IMPORT_LINE_NAME = re.compile(r"[A-Za-z_]\w*(?:\.[A-Za-z_]\w*)*") | ||
| _FROM_LINE = re.compile(r"from[ \t]+(\.*)[ \t]*([A-Za-z_][\w.]*)?[ \t]+import\b(.*)", re.DOTALL) |
There was a problem hiding this comment.
P1. This pattern has three overlapping whitespace runs around an optional group, so a failed match backtracks in cubic time. A file reaches it exactly in this PR's case, once the walk has spent its 512 KB parse total.
Repro: _FROM_LINE.fullmatch("from" + " " * n + "x") takes 0.03 s at n=400, 0.19 s at 800, 1.5 s at 1600 and about 9 s at 3200. The 8 MiB search total limits bytes, not time, so one 100 KB line runs for days, and asyncio.to_thread keeps burning the sandbox's CPU after the call is cancelled.
Fix: split the line on whitespace instead of using a regex, or make the runs possessive, e.g. from[ \t]++(\.*+)[ \t]*+(?:([A-Za-z_][\w.]*+)[ \t]++)?import\b(.*). Add a test that a line of 50,000 spaces returns at once.
| hit = None | ||
| else: | ||
| hit = _file_inside(place, root, deepest, lookups_left, unread) | ||
| hit = _file_inside(place, root, deepest, lookups_left, unread, limited) |
There was a problem hiding this comment.
P2. limited goes in as too_big, so an imported code file over 1,000,000 bytes is never queued for the line search and always cuts the reading, whatever it holds. That is the earlier file_size case moved into the walk.
Repro: agents/a/main.py = import openai\nimport common.data, common/data.py = a 1.86 MB list of strings: ([], {'import_walk'}). The same module at 900 KB gives no cut.
Fix: in a walk that searches, let _file_inside return a regular code file up to _LARGEST_UNREAD_SEARCHED so the over-cap branch line-searches it, with a test per case.
| item is set when a line had an import past the colons tried (``_statements``). | ||
| """ | ||
| names: list[tuple[int, str]] = [] | ||
| lines = iter(content.split("\n")) |
There was a problem hiding this comment.
P2. The line search silently misses legal import forms, each giving no cut and no look:
import os, \thencommon.utilreturns onlyosfrom common import \thenutilreturnscommon, notcommon.utilfrom.common import utilandfrom .import utilreturn nothingx = "#"; import common.utilreturns nothing (the#split)- an unclosed
from x import ((e.g. inside a docstring) swallows every later line
Repro end to end: z_main.py = import os, \ then common.util, common/util.py = import playwright: ([], frozenset()).
Fix: join lines ending in \ first, accept from followed by a dot and dots followed by import, and set capped where the search can't be sure, so the reading is cut rather than silent. One test row per form.
| coding = _coding(raw) | ||
| if coding is None or coding[1].name not in _RAW_SAFE_CODECS: | ||
| return True | ||
| if any(name in raw for name in _PYTHON_NAMES if name != b"omputer"): |
There was a problem hiding this comment.
P3. This is a plain substring test, so a 200 KB colors.py full of 'steelblue' matches steel and gives file_size. Scripts are searched for Python import names too, though a read script only contributes tool markers: a 1.1 MB widget.js containing window._selenium gives file_size, while the same text in a small .js finds nothing.
Fix: whole names (_WHOLE_NAMES) for .py, and only _MARKER_START_BYTES for scripts.
| ) | ||
| # A budget or limit left code the walk reached unread, so what it would have led to is | ||
| # unknown (TH-9512, Dileep's C2). _tools_in has already logged its own cuts. | ||
| return names, (cut | {"import_walk"} if walk_cut else cut) |
There was a problem hiding this comment.
P3. _tools_in logs "cut by ..." before import_walk is added, so a reading whose only cut is the walk logs no cut line, and the walk's own WARNING fires on any stop. The logs can't tell which readings paid for a look.
Fix: log the cut line once in _scanned_agent, after merging in import_walk.
| dict.fromkeys((*_agent_files(folder), *above, *shared, *manifests)), local, folder | ||
| ) | ||
| # A budget or limit left code the walk reached unread, so what it would have led to is | ||
| # unknown (TH-9512, Dileep's C2). _tools_in has already logged its own cuts. |
There was a problem hiding this comment.
P3. "(TH-9512, Dileep's C2)" is review history in public code (standard 11); some test comments name the reviewer too. Drop the parenthetical.
| untried. A file the parse total skipped but the search covered, a name dropped for its length | ||
| or depth, a link, a file the parser refuses (syntax error) and a codec refusal are not. | ||
| """ | ||
| from .agent_type_check import _LARGEST_UNREAD_SEARCHED, _UNREAD_SEARCH_TOTAL |
There was a problem hiding this comment.
P3. About 90 lines of import-line parsing (_statements, _import_lines and their regexes) go into the 2,034-line credentials.py, which imports private constants from agent_type_check inside a function to avoid a cycle. Standard 13 says a new function in a file over 1,000 lines goes in its own module or the PR says why it stays.
Fix: move the line search to python_source.py or a new module and pass the caps in, as search_total already is.
…rms, and cut where it can't be sure (TH-9512) The line search moves to import_lines.py: strings and comments are taken out in one pass, then the import lines are read by splitting on whitespace, with no regular expression that can backtrack. A line continued by a backslash is joined, and from.x, from .import x, a # inside a string and a docstring holding an open paren are read as Python reads them. A string, list or colon run it cannot be sure of sets capped, so the reading is cut. The walk also reaches a code file over the read cap, up to the search cap, and searches it. Signed-off-by: khushalsonawat <khushalsonawat@gmail.com>
| for segment in code.split(";"): | ||
| segment = segment.strip() | ||
| pieces.append(segment) | ||
| if _COMPOUND_START.match(segment): |
There was a problem hiding this comment.
P2. The line search misses an import after the colon of a compound statement whose header runs over more than one line. The line that closes the brackets doesn't start with if, with, for, except or class, so its colon is never tried. Nothing is capped, so there's no cut and no look.
Repro (subfolder agents/a, the same six 100 KB padding modules as before):
# agents/a/z_main.py
if (True and
True): import common.util
# common/util.py
import playwrightbrowser_scan(root, "agents/a") gives ([], frozenset()) and decide gives keep. On one line, or without the padding, it finds playwright.
import_lines alone misses the same import for with open(\n 'f') as g: import x, for x in (\n 1, 2): import x, except (A,\n B): import x and def k(\n a): import x. A random mix of 54 legal statements checked against ast gives 446 silent misses in 2,757 files. With the two multi-line headers taken out, it gives 0.
Fix: join each line that ends inside an open bracket to the lines that follow until it closes (one pass, after strings and comments are out), then split statements on the joined line. Add a test row for each header keyword.
| if piece.startswith("import") and piece[6:7].isspace(): | ||
| for item in piece[6:].split(","): | ||
| if name := _name(item): | ||
| names.append((0, name)) |
There was a problem hiding this comment.
P3. A file made of import names is linear but slow, and holds every name in memory. On 3.12.11, one 4 MiB file of from x import ( followed by 50 lines of a, repeated, takes 1.9 s. from x import ( with no close followed by a, lines takes 2.1 s, and import a,a,a,... takes 1.3 s. A plain 4 MiB module takes 0.38 s. Peak memory for the first is about 217 MB (1.86 M tuples). The walk searches 8 MiB, so a crafted repository adds about 4 s to the scan the save waits on. The description says "a 4 MiB file under 0.4 s".
The caller keeps only import_limit - imports_tried names (5,000 at most) and cuts on the rest, so every name past that is wasted work.
Fix: pass the names left into import_lines. Stop and set capped once that many are found, so time and memory end at the limit the walk would cut at anyway. Add a test that a 4 MiB file of names returns within a bound.
This PR goes into
feat/TH-8804-source-subfolder. #276 and #283, which it was built on, have merged there, so its diff is now only its own commits and the base merges.Summary
The browser scan stops reading an agent's code once one of its limits is reached, and returns what it found so far. Until now the reading then went through on that, with nothing logged. So padding read first, or an honest repository bigger than the limits, hid a plain
import playwright. For example, three 126 KB files that nameplaywrightin a comment spent the 256 KB parse total, and the agent's real import was never parsed.Now, whenever one of the browser scan's own limits or failures leaves a code file unread or unparsed, the scan says it was cut. A cut reading gets one second look, with its own message, before the model's answer stands. It is never a refusal on its own.
The import walk, which follows the agent's imports into modules outside its folder, now counts too (Dileep's C2). When one of its totals runs out, the files it reached but couldn't parse are read with a cheap import-line search, for browser names and for further imports. The reading is cut (
import_walk) only if that search hits its own cap, so it is never silent (see "The import walk" below).Ticket: TH-9512.
How it works
agent_type_check._tools_inreturns the names and the set of limits that cut it. The limits are:file_size: over 128 KB for a.py, over 1 MiB for a script orpackage.json, and only when the file may name a tool: its bytes hold a browser package's name or a tool marker (the word "computer" alone doesn't count), it is a.pywhose coding line names a codec that isn't raw-safe or one Python refuses, it is apackage.json, or it can't be read or searched within 4 MiB a file and 8 MiB a scan. The search never decodes or parses. A big data module that names no tool is no cut (Dileep's C3);scan_total,script_total,python_total;parse_total: only a file that names a browser package counts;parse_failed;token_total: both the total and the work limit;decode_total(fix(harness): decode a Python file before searching it when its codec can spell a name (TH-9490) #276's);search_total;skipped;package_unreadable: a manifest we can't read (a read error, or nesting too deep to parse). It's read as Node reads it, with bad bytes replaced and integers of any length, and invalid JSON means no dependencies, as it does for npm (Dileep's C4).browser_scan(root, subfolder, reached)returns both from one walk and the existing cache.browser_tools_foundandbrowser_tools_in_agentare unchanged.decide,SecondLook.verdict,issueandprompttakecut. A cut acts like a found tool for the look, and nothing else changes:agent type scan cut by file_size 17, parse_total 6.What it costs
A cut reading pays one extra model turn, which the customer pays for in authoring credits. That's about $0.10-0.25 a look, or 10-30% of a median $0.88 reading, from 23 cost records.
src/, the futureagi backend, and thefuture-agiandcoreroots.The import walk (Dileep's C2)
import/from ... importlines (each;segment, and the text after a compound statement's:) are followed with no parse. The search has its own caps: 8 MiB in all, 4 MiB a file, and the walk's file, byte, import and lookup limits. Only when it hits one is the reading cut (import_walk). The agent folder's own files over 1 MB are line-searched too, up to 4 MiB; a larger one is skipped on its size before it is opened, and is a cut. Dileep's repro now findsplaywright, with no cut.examples/frameworks,oss/simulation-acceptance,tests,tests/harness,tests/live,tests/runtime) and future-agi'sfutureagi/model_hub. Each is a tree past several limits at once (the 5,000 import-name limit, the 8 MiB search total, the 1,000,000-byte read cap on 1.3-2.1 MB modules, and formodel_hubthe lookup limit), so a cut and a second look are right for them. Re-measured after the last fix: unchanged.import_lines.py, after Dileep's second review) strips strings and comments in one pass and reads lines by splitting on whitespace, with no pattern that can backtrack: its time is in proportion to the text. A 100 KB line of spaces, tabs, dots, parentheses, backslashes, quotes,#,:,;,importorfromtakes under 40 ms, and a 4 MiB file under 0.4 s (fromand 50,000 spaces took over 90 s before, now about 25 ms). It reads\-continued lists,from.x,from .import x, a#or;inside a string,\rline ends, tabs and form feeds. A code file the walk reaches over the 1,000,000-byte read cap is line-searched up to 4 MiB; over that it is a cut. A.pytoo big to read is a cut only for a whole package name (not forsteelblue), and a read script contributes tool markers only.import.importlib.import_module(name),__import__,exec); a name only a decoded codec spells; for a script past its script total, anything the script pattern there misses.Left alone, on the Coordinator's calls
none. It's held with TH-9517 for Khushal, becauseframework-detection.md§7 forbids changinginstrumentation_evidence.py.Merged with the base
HEADmergesc0e51b7e(#287, #289-#293 and dev). One hunk, the skip handler inagent_type_check.py, was resolved by hand: #291 drops the skip's trace at every level, and this PR counts the skip as theskippedcut; both are kept (no trace, the type once at WARNING, the count in the cut line).566a4459mergedcde91b3f(#282, 1132192) with no conflicts. The walk now reaches a uv workspace member past the first 256 when a requirement names its folder, so code there is scanned and can bring a look; a folder link back into the agent's folder no longer counts as code left unread, which changes only the walk's WARNING. No cut changes.By hand (
b812a9a1)Merging the base brought #288 (TH-9448), which changed how the scan logs a file it skips. Three hunks were resolved by hand, keeping both sides:
agent_type_check.py, the loop header: this PR'sorderedlist (it reads the agent's own files first) is kept; [TH-9448] fix: follow-ups from #224's last review of the browser-agent scan #288's separateskippedcounter isn't needed, because the skip is counted as this PR'sskippedcut.agent_type_check.py, the skip handler: [TH-9448] fix: follow-ups from #224's last review of the browser-agent scan #288's logging is kept as it is: the error's type once at WARNING, and the path and trace at DEBUG. The count goes into this PR's cut line (agent type scan cut by skipped N), so [TH-9448] fix: follow-ups from #224's last review of the browser-agent scan #288's own "skipped N files" line isn't repeated.test_agent_type_refusal.py: [TH-9448] fix: follow-ups from #224's last review of the browser-agent scan #288's skip tests now expect the cut line in place of "skipped N files". Its NUL-byte test, which expected no WARNING, now expects exactly the cut lineagent type scan cut by parse_failed 30: those files name a tool and can't be parsed, so they cut the scan, and the line holds names and counts only. No source text reaches the log at any level.Review
Each round used two fresh Opus reviewers.
Design:
Then the Coordinator's call: the flag alone, with no parser imitation.
Plan: r1 clear.
Change: r1 clear, with every plant run: C1-C11 for each limit, and D1-D11 for the look, the log, the cache, the shield, the walk and the closure.
Fixed after change review r1, as the rule allows for a round with nothing above Medium:
package.jsonthat isn't a file (a dangling link) is no cut;Dileep's review: C2 (the walk stop) is fixed, above ("The import walk"); C3 (
file_sizeonly when the file may name a tool) and C4 (package.jsonread as Node reads it) are fixed.Change r2 (those fixes): not clear: a long integer in
package.jsonhid a dependency, and a coding line Python refuses at compile time can still run once its codec is registered. Both fixed, with a test for the 4 MiB search, a narrowercomputerneedle, and the untested branches pinned. The oversize module the search clears now leaves its imports unfollowed: accepted by the Coordinator and stated above.Change r3, the last, with the base merged: clear. Fixed after it: tests for a file exactly at the search bound, a module naming only
PlayWrightBrowserToolkit, and a file past what's left of the search total not being opened.Verification
Where the tests run: CI's SDK smoke runs
test_agent_type_refusal.pyandtest_contract_tools_subfolder.pyon every push. The newtest_browser_scan_cut.pyand the other files below ran locally on Python 3.12.3, as listed; addingtest_browser_scan_cut.pyto the smoke list is pending.Fix-only check r1 (one reviewer) of the C2 fix
868b243e, the tests inda2fba5fand the skip-handler hunk resolved by hand ine174292e: clear, High 0, Medium 2, Low 1. Both Mediums fixed in18e63950(a large file in the agent's folder is skipped on its size before it is read; an import after;or a compound:is followed), and the Low too (the line search runs only for the browser scan, not for bundling or the credential walk).Fix-only check r2 of
18e63950: clear, High 0, Medium 0, Low 2 (a re-measure, done: unchanged; the 8-colon line, fixed in0e2570af).0e2570af: an import past the 8 colons the line search tries is now a cut (import_walk), never silent; a long line with no import after its colons stays no cut. Fix-only check: clear, no findings. On0e2570af: the five files, 1180 passed;ruff checkclean; re-measured, 7 of 45, unchanged.7084b31e(Dileep's second review: the backtracking pattern replaced, the legal import forms, a cut where the search can't be sure, whole names, the cut logged once, the line search moved toimport_lines.py): fix-only check by a fresh reviewer, clear, no findings (every pattern linear on adversarial input; 62 of 62 legal import forms matchast; every unsure case cuts). On7084b31e, Python 3.12.3:test_browser_scan_cut.py130,test_agent_type_refusal.py739,test_contract_tools_subfolder.py180,tests/test_harness_credentials.py81,test_agent_scan.py84,test_import_lines.py140 passed (the last two are now in CI's SDK smoke);ruff checkclean; re-measured, 7 of 45, unchanged.On
18e63950, Python 3.12.3:test_browser_scan_cut.py,test_agent_type_refusal.py,test_contract_tools_subfolder.py,tests/test_harness_credentials.pyandtest_agent_scan.py, 1178 passed;ruff checkclean. Dileep's repro test and a two-hop test fail one174292e.ruff checkandruff format --checkon the new test file; no new findings in the edited files against73e91700;compileall.One test run on Python 3.12.3, a file at a time, on the merged head:
test_browser_scan_cut(new)test_agent_type_refusaltest_contract_tools_subfoldertest_agent_readingtest_scan_walktest_hosted_authoring_entrypointtest_harness_credentialstest_builder_chattest_hosted_conversationtest_python_source_codecs51 bugs planted one at a time, from the change reviewers' runner, over the new file,
test_agent_type_refusalandtest_contract_tools_subfolder: every limit's cut removed (C1-C11), the look, the log, the cache, the shield, the walk and the closure (D1-D11), and each moved test's own bug (M1-M11). 50 failed a named test. The 51st, which kept a non-filepackage.jsonas unreadable, no longer applies after the fix above; checked by hand, its new test fails under it.After Dileep's review: the worker planted each fix's bug (R1-R9), all caught. Change r3 planted 122 more over the merged tree: R1-R9 again, the merge's walk wires, and rounds 1-2's plants re-run. Three survived (a search that skips its last KiB, no check of what's left of the search total, the toolkit name dropped from the search); each is now caught by a new test. Four change nothing reachable.
After merging fix(harness): the save's import walk says when it leaves code unread, and reads more of it (TH-9543) #283:
test_the_walks_stop_is_no_cutwas re-pointed over its 512 KB walk total (7722f1b5), and plant D8 is caught by it again. The merge reviewer re-ran the six files: 59, 693, 180, 92, 81 and 156 passed.TypeScript: not touched.
Checklist