Repository navigation
fix(agents): verify completion and preserve exhaustive inventories - #4374
Conversation
- extraction.py: render() unpacked a per-path (entries, pages, digest) tuple but never used digest -- unused-variable. - agent.py: _tool_overrides, _extraction_ledger, _completion_evidence were all first assigned outside __init__ (lazily, via hasattr guards or directly in process_query) -- attribute-defined-outside-init. Declared upfront: _tool_overrides as the empty dict every call site already assumed, the other two as Optional (they're only ever built fresh per turn).
…g extraction Three real issues from review, one from a field report (amd-pworfolk, PR #4145): - A query like "list all functions in app.py" has no destination preposition for destination_paths to anchor on, so its no-anchor fallback scanned the whole clause and picked up the source file itself as an extraction destination that never resolves -- every such code query reported status: incomplete. Excludes common code extensions from ExtractionLedger.requested; a real destination is never source code, and code-index/read-file tools already own that file. - validate_sources/validate_outputs caught bare Exception on newly written code; narrowed to ValueError (read_snapshot's own signal) and OSError (the file operations it does not pre-validate). - extract_pages only snapped page boundaries to newlines, but transcribe_media's own output is a single unbroken line -- every GAIA-transcribed source opened pages mid-word, and a page landing on pure filler with a truncated opening made the extractor unable to confirm completion, aborting the whole extraction. Falls back to sentence-ending punctuation when no newline exists in the overlap window. Also fixes two Windows-only test failures the same field report flagged: two assertions compared normalized (Windows-lowercased) keys against raw mixed-case tmp_path strings, and a third symlink-based test needs elevated privilege Windows CI runners don't reliably have -- added a skip marker rather than leaving it permanently red.
A slower local model spent all 5 steps searching and never answered, reporting the script's own tight budget as a failed follow-up rather than a real one (amd-pworfolk field report, PR #4145).
"List all functions in app.py" excluded app.py from the requested- destinations set but left the ledger enabled, so gaps() still demanded a verified source that code-index/read-file tools never produce, collapsing the turn to status: incomplete. Gate enabled on the query naming a real (non-code) destination or no destination token at all. Also folds duplicate offset/end/total computation out of completion.py's delivered(), shares one path-normalization helper between CompletionEvidence and ExtractionLedger instead of each constructing its own evidence object, and renames a shadowed loop variable in the extraction validation script.
gaps() == [] holds for two different reasons -- the ledger being off, and the ledger being on with nothing outstanding -- so the existing assertion passes even if activation regresses. Also pins that a document source still activates when the save target happens to be a code file.
…wo pages A real unbroken workshop transcript aborted on page 3 because pages 2 and 3 quoted the same sentence from slightly different start points. Their spans overlapped without either containing the other, so reconciliation treated identical fields as a conflict and discarded every page extracted so far. Field values are verbatim in their quotes, so partially overlapping quotes now merge when every value both sides state sits once, at the same source offset, in each quote. Repeated items that share context still stay separate, because their names sit at different offsets. Free-text entries keep the containment rule, since their text cannot be grounded.
"What time does the store close?" or "List every U.S. state capital" came back
as "I couldn't verify completion" because the save detector counted any
"store/save/export" word and the extraction ledger demanded a source that no
one asked for. Save requests now need the verb as an instruction, and a turn
that never read a document leaves no extraction gap.
- Correct saves no longer read as missing through a symlinked directory, a
named folder, or a relative path inside a repo subdirectory; paths resolve
against the working directory, as the file tools do.
- A Lemonade connection error keeps its message and failed status; unverified
work is listed beneath it rather than replacing it.
- Explaining deletion ("deleted files go to the bin") is not a cleanup claim.
- Extraction never merges two non-overlapping quotes into one entry, so a
broad quote can no longer absorb a repeated item. Ambiguous replies are
re-asked, and values repeated inside one quote still match their occurrence.
- An over-long extraction answer stores neither turn instead of an unanswered
question; loading document-extract keeps code-symbol queries off.
- Docs: tool signature, merge rule, memory limit, gaia-voice token count,
CHANGELOG entry; inline comments trimmed.
A second review round found ordinary requests still ending as incomplete
("Save me some time", "Store this in memory", "Write a guide to Node.js") and
extraction still able to lose or abort on realistic quotes.
- A save request now counts only when it names a file or disk; a pathless
"save it" is checked through the answer's own save claim. A destination path
must follow its preposition directly, so "to clean logs/app.log" is a topic.
- "Write X to Y" is a save only for "it", "this" or "the summary", not "a
guide to Node.js". Every save verb in a sentence is read, and a trailing ?
or ! no longer hides the path.
- The answer check reads only a claim's main clause, so "a function that
saves to x" and "Once saved, the file is at x" are not claims. A deletion
claim about a path that is absent and untouched is not flagged.
- Two overlapping extractions whose values sit at one source location, one
inside the other, merge into one entry spanning both quotes with the fuller
value of each field, so a value cut short at a page boundary no longer aborts.
- Two items from one reply never merge, and a candidate must reconcile with
every quote already merged, so a broad quote cannot chain two items into one.
Conflicts are re-asked with the conflicting quote named.
- Free-text entries with equal text merge when their quotes share most of
their text; repeats sharing only context stay separate.
…lysis
A third review round found the nested-value merge could let an omission-pass
"march in place" swallow an earlier "march", and several ordinary requests
still ending as incomplete.
- Identity fields (name, id, title) must match exactly; only descriptive
values may be cut short across a page boundary.
- Sources are the files a request reads from: "from meeting.txt into
actions.json" now extracts meeting.txt and lets the inventory be saved to
actions.json. Parenthesized paths and multiple sources are found.
- Extraction stays off for agents without a file permission validator, for
PDFs and other binary documents, and for analysis of code ("find all bugs in
main.py"); listing content such as TODOs still extracts from code.
- The inventory is appended to the model's answer instead of replacing it, so
requested analysis ("which one is hardest") survives.
- Save claims keep their relative clauses ("the summary that you asked for");
only code descriptions ("a function that saves to x") are ignored. A save
from an earlier session is not a claim about this turn.
- Only requested or claimed outputs need readback; fixing a typo does not.
- Removing a file from an index or list is not a deletion claim.
…cord
A fourth review round found wording-only checks still misfiring: "Downloads
are saved to ~/Downloads" ended incomplete, "Write a Node.js server" demanded a
file named Node.js, and "List all the features of Node.js" required extracting
a document that does not exist. The checks now lean on what actually happened.
- An answer's save claims are checked only when the user asked for a save or
a disk-writing tool ran this turn; a reused transcript still counts.
- A document source must exist on disk; data files (CSV, JSON, logs, YAML,
XML) are left to code tools.
- Without a field named for a name, id or title (exercise_name, ExerciseName
included), the first requested field identifies the item, so an omission
pass reporting "march in place" can no longer absorb "march".
- "extract/put/copy ... into x.json" names an output, "not to x" drops x, a
folder is a save target, and a bare name that modifies a noun ("a Three.js
scene") or sits past a dash is not a path.
- Malformed tool paths (NUL bytes, non-strings) end as normal results.
A fifth review round found the previous commit let "I saved it to notes.md"
pass after "Create notes.md", dropped "Save it to x.txt thanks" as a request,
and aborted extraction when two copies of an item both left the first field
"not stated".
- A first-person save claim is always checked; only impersonal statements
("downloads are saved to ~/Downloads") need a save instruction or a write.
- The modifier rule ("a Three.js scene") applies only when no destination
preposition names the path.
- With no identity value stated in both, any equal field identifies the item.
- A file a successful shell or Python run modified this turn counts as
written once it is read back, so "ls > listing.txt" can complete.
…ntories A sixth review round found the previous commit treated any "I" in a sentence as a save claim, so "I think downloads are saved to ~/Downloads" ended incomplete, and accepted files touched by a failed or unrelated shell run as the saved output. A live run also showed a turn that ran out of steps dropping its 40 extracted items from the answer and from memory. - A first-person claim needs I/we as the subject of the save verb. - A shell-written file counts only when its modification time falls inside a successful executor call and no failed one, and it never satisfies a pathless "saved it to a file" claim. - "Create/make/generate notes.md" names an output; "a Node.js app" does not. - A turn that ends without a final answer still shows the extracted inventory.
… speaker
A seventh review round found "create a summary of the errors in app.log"
treating the user's source as the save target, and "I've successfully saved
it to notes.md" slipping past the first-person check.
- Create/make/generate name an output only through their direct object
("create notes.md"), never through "in X", and a tech name before a noun
("a Next.js landing page") is not a file.
- A first-person claim is I/we followed within a few words by a save verb,
with no be-verb or clause break between, so adverbs no longer hide it and
"I think downloads are saved to x" is still information.
- A turn that stops at the step limit shows the finished inventory under the
step-limit message.
…verdict
An eighth review round showed that narrowing which save claims count had
dropped main's safety net: "The summary has been saved to notes.md" with no
save requested passed with no challenge at all, while checking every claim
hard had turned informational answers into incomplete turns.
- A save claim still makes the turn incomplete when a save was requested, a
file-writing tool ran, or I/we did the saving. Any other claim now gets one
correction (write it, or say it was not saved now) and then stands, as the
fabricated-save guard on main did.
- "Create/make/generate ... in notes.md" names an output when notes.md does
not exist yet; an existing file there is the source.
- A file name followed by a participle and an article ("report.md summarizing
the project") is the output, while "a Next.js landing page" is not.
A ninth review round found the previous commit treating "create a summary of the action items in meeting.md" as a save to meeting.md whenever the source sat outside the working directory, ending correct answers as incomplete. Only a create verb's direct object names an output now; a claim about a file after "in" still gets the one-correction check.
Testing against real YouTube workout captions (no newlines, no punctuation, non-breaking spaces) and an audit of main since this branch showed the pipeline failing on the input it exists for. - A cloud reasoning model spent its whole 8K budget thinking about one dense caption page and returned no JSON. Page calls now ask cloud models for low reasoning effort, allow 16K output tokens, and report a spent budget by name; the old finish_reason check never fired because the SDK drops it. - Quotes match captions despite non-breaking and doubled spaces; the stored quote is still the source's own text at its exact offsets. - Pages snap to a word boundary when a transcript has no punctuation. - A model that reshaped the saved JSON by hand made correct runs incomplete. Exports now give each requested field its own key, and hand edits to a file save_extracted_items wrote this turn are refused. - Local page calls drop the anti-repetition penalties, which fight verbatim copying; a save into the agent's scratch folder, deleted on close, no longer satisfies "save it"; save_extracted_items gets path grants like write_file.
Adversarial fuzzing and live runs on real YouTube workout captions found the
extractor failing honest models and occasionally losing items.
- An item is located by the source span of its identity value, not its whole
quote. Neighbouring items' quotes may now overlap, which aborted about a
quarter of real-caption runs; an omission-pass repeat whose quote names the
item twice takes the unclaimed occurrence instead of vanishing into the
first. Free text that repeats inside one quote is never merged.
- Matching ignores case and spacing but only on whole words ("arm" is not in
"farm"); stored quotes stay the source's own text.
- A cloud page that exhausts its budget reasoning is retried once with
reasoning off; malformed or truncated replies are retryable errors.
- The fields list stops before a save clause, an all-"not stated" item is
rejected, CSV keeps colliding field names and escapes formulas, large
exports validate right after saving, the prompt cap fits the query cap, a
field-schema mismatch names the fields to resend, and the hand-edit guard
matches case-insensitive filesystems.
…ripts
Live runs on real YouTube workout captions showed honest models failing whole
documents: instructors repeat names inside one quote, cloud reasoning models
exhaust their budget on dense pages, and models compose exercise names
("Big circles (arms)") rather than copying them.
- Merging no longer raises. Extractions are one item only at the same place
(same name span, or quotes sharing half their text) with matching or nested
names; different names stay separate items, differing descriptions are both
kept, and anything too ambiguous keeps both entries marked
"(may repeat item N)".
- A name copied from the quote locates the item; a composed label leaves the
quote to locate it, and one sharing no words with its quote is kept but
marked "(label not in quote)". Only the item's quote must be verbatim.
- A quote naming its item twice parses; an omission pass takes the unclaimed
occurrence, so a missed repeat is never folded into the first.
- Errors name the offending field and value so a retry can fix it.
…ports A 74K-character real caption file (the size of the original workshop) timed out at 30 minutes because a cloud reasoning model spent 50-100 s per page thinking, and a finished extraction then ran out of steps while the model paged through a 60K-character export to "verify" it. - Page calls ask cloud models for no reasoning; copying quotes needs none. The same file now finishes in about two minutes with comparable recall. - The framework reads every exported inventory back in full and compares it exactly, so that counts as the readback; the model no longer pages through it, and a tampered file is still caught. - An entry that states no field is ignored instead of failing its page.
A final planted-item fuzz found no lost items but a rare wrong merge: when a
quote named its item twice, the extraction was pinned to an occurrence already
found and its values landed on a different item of the same name.
- A quote naming its item twice takes an unclaimed occurrence and is never
merged; a true re-report shows as a flagged possible repeat.
- A name with a trailing period still locates itself, a one-sided located
name must sit inside the other quote, and free-text items at different
name positions stay apart.
- The repeat flag also catches label drift ("march" vs "Seated marching").
- Free text normalises carriage returns, invalid Unicode is a retryable
reply error, and the output-size error no longer calls the file a source.
Skills now load when the model calls load_skill rather than being guessed before the turn, so that call is the main way extraction switches on.
# Conflicts: # hub/agents/gaia/npm/CHANGELOG.md # src/gaia/agents/base/agent.py
# Conflicts: # src/gaia/agents/base/agent.py
|
Heads up before anyone rebases this: #4373 landed on What happens: #4373 added a closing model call at the step cap so the user hears what was found. That call now fills the final answer and marks it sealed, which skips the block this PR added just below it — the one that reports what is still unverified and re-attaches the inventory. So asking for every exercise in a long transcript, hitting the step limit, and getting back a bare "reached maximum steps" note with the 60 items already extracted thrown away. Three of this PR's own tests catch it, which is how I found it:
Someone who owns both changes should decide whether the step-cap answer goes through the completion gate. I'd say yes — an answer that says "I saved it" should be checked the same way no matter which exit produced it — but that is a behavior change to #4373, not a merge conflict, so it is not mine to make. 🔍 Technical detailsWhere it breaks. In Status field. That part merges cleanly. #4373 added Test-harness note. The three tests script an exact number of stub model replies. #4373's closing call is one more, so two of them currently surface as State of this branch. It is merged with |
Request changesThis adds a framework-level completion gate and an exhaustive document-extraction path to the base agent loop. The engineering is careful — the heuristics are guarded, the merge logic never drops an item, and ~1,500 lines of new unit tests back it up. But the gate runs on every final answer of every agent, and the one measurement that would tell us it doesn't degrade ordinary turns hasn't been run. What needs to happen before merge:
Nothing here is a security concern, and the docs, changelog and manifest token count all move together with the code. Real-world evidence
$ gaia skill audit hub/agents/gaia/python/gaia_agent/skills/document-extract
ALLOW ✅ document-extract 0.1.0
Allowed at the 'community' tier: no findings.
$ gaia skill info document-extract
consumes : extract_document_items, save_extracted_items, read_fileThe reworked agent core is exercised through the real CLI, and fails loudly as designed: $ gaia prompt "List every item"
❌ Error: Lemonade server is not running or not accessible.
📥 Lemonade server is not installed on your system.
EXIT=1Docs and the 702-token manifest claim both check out ( The bundle marks these "pending strix-halo lane": the Agent UI screenshot of an extraction turn, 🔍 Technical details🟡 Important1. Agent eval not run for an answer-path change ( CLAUDE.md lists "the agent-loop catchall" and error classification among the surfaces that require The PR's own unchecked box says it: "the completion gate changes final answers, so decide before merge." There is no committed flagship baseline, so you can't claim "no regression" — but a run still surfaces a category full of 2. Every read file becomes a mandatory extraction source when the query names no path ( and (not self.requested or self.key(path) in self.requested)With Your test states the behaviour exactly: state = ExtractionLedger("List all planets", str(tmp_path))
state.activate_skill("Extract **every** exercise from the workshop transcript.")
state.observe("read_file", {"file_path": "workshop.txt"}, {"status": "success"})
assert state.gaps()
3. if instructions not in self.query:
self.query += "\nActive extraction instructions:\n" + instructions
self.results.clear()
🟢 Minor4. Conversation search can now build a multi-megabyte tool payload (
5. A dropped extraction turn is invisible to the user ( Over the limit, both the user and assistant turns are skipped and only 6. Redundant locals in the dev benchmark (
7. Skill selection by bare substring ( selected = active | {name for name in skills if name in query}A short skill name matches unrelated prose, and Strengths
|
One conflict, in the turn's result status. `main` (#4373) made a step-capped turn report "incomplete" even when the closing call answered; this branch made a turn with completion gaps report "incomplete" too. Both conditions kept, with the branch's errors-first ordering: "success" now needs a valid answer, no completion gaps, and no step cap.
When the step limit ran out mid-extraction, the closing summary was handed straight to the user as a finished answer: a claim that a file had been saved went unchecked, and the items already extracted were dropped. The two behaviours were each correct alone — the closing call sealed its own answer, and the completion gate only ran for turns that never sealed one — so together the gate was skipped exactly on the turns that needed it most. The closing answer is now judged like any other: an unverified save or an unfinished extraction comes back as incomplete, and the inventory gathered so far is still attached. An answer with nothing to flag is untouched, so a step-capped run still says what it found. The gate itself moved into `_gate_unsealed_answer`, shared by the step-cap path and the post-loop path that already used it. It now keeps the extracted inventory even when there are no gaps, which the step cap used to get from the max-steps fallback that no longer fires.
Skill audit
✅ All audited skills cleared the tier they claim. Per-finding detail is withheld here on purpose. Read it in the Security > Code scanning tab, or download the |
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
|
Merge and tests are clean, but two items from the review above are still open, so I'm holding off on merging.
🔍 Technical detailsMerge with current Tests: Eval: confirmed via I did not push this merge resolution or attempt the squash merge, since the outstanding items are a maintainer decision (eval run + the two behavior confirmations), not a mechanical one. |
# Conflicts: # hub/agents/gaia/npm/CHANGELOG.md
Resolves overlapping turn-state init in process_query: main's amd#4374 renamed file_write_claim_reprompts to completion_corrections, this branch adds cut_off_continuations alongside it.
After finishing what the user asked, the flagship could start an unrelated job and run until cancelled. In one TUI session a one-line bugfix was done and verified in 40 s, followed by 16 minutes of "extract every source into an inventory". The trigger was the `coding` skill: its "find every call site" tip switched on document-inventory mode, so every coding session was pushed toward it. Now only a skill built on the extraction tool turns that mode on, and two general stop conditions end work the request didn't ask for. Closes amd#4448 - **Root cause:** a skill enables inventorying only by listing `extract_document_items` in `tools_required`. None of the starter skills do, while real "list every item in this transcript" requests still get full, source-quoted extraction (amd#4374's guarantees are kept). - **Work after the answer:** once the turn has answered, a tool call that touches nothing the request touched isn't run, and the model is told to finish and offer the work instead. A second such call ends the turn. - **A tool failing with new arguments each time** is stopped like an identical repeat. The repeat guard used to miss it because the arguments varied. ## Test plan - [x] `pytest tests/unit/agents tests/unit/eval`: 3001 passed, 78 skipped, 1 failed. The failure, `test_symlinked_directory_is_the_same_output`, also fails on untouched `main` on Windows (symlink privileges). - [x] New tests: turn scope (drift after the answer, failing-tool loop, what counts as related); the coding skill's wording no longer enables extraction while an extraction skill still does; code read along the way isn't an inventory source; bench-harness hooks. - [x] Before/after on DeepSeek V4.1 Flash via Fireworks, same machine, `gaia eval tasks run --suite everyday --tasks 02-bugfix,01-refactor,b6-config-env --no-judge`: | | Before (`main`) | After | |---|---|---| | Passed / verified by tests | 3/3 · 3/3 | 3/3 · 3/3 | | Steps | 45 | 29 | | Tokens | 832k | 544k | | Time | 323 s | 127 s | n=1, but the deltas are far larger than run-to-run noise. - [x] `python util/lint.py --black --isort`. - [ ] Judged quality pass (`gaia eval tasks judge`) once the judge CLI is signed in on the eval box. --------- Co-authored-by: Kalin Ovtcharov <kalin.ovtcharov@amd.com>
2 participants
Replicates #4145 (
kovtcharov/gaiafork, push access blocked) onto a branch this session can push to. No content changes beyond conflict resolution against currentmain.GAIA could claim it saved a file it never wrote this turn, or turn a long workshop transcript into a condensed list presented as complete. Now a save counts only when that exact file was written and read back, and a long document is extracted page by page into a full inventory in which every item keeps a verbatim quote from the source. The result is kept in memory, and anything unfinished is reported as incomplete. Ordinary questions ("What time does the store close?") behave exactly as on
main.Fixes #3983 and #4141; related to #4009. Closes #4145.
Test plan
python -m pytest tests/unit/agents/ -q: 1,663 pass, 51 skip. The one failure (test_symlinked_directory_is_the_same_output) is this Windows box lacking the symlink privilege, not the code.python -m pytest tests/unit/ -k "memory" -q: 1,136 pass. The one failure (test_memory_discovery.py::TestCredentialManagerContractShape) reproduces on pristinemain.python -m pytest tests/unit/ -k "inventory or transcript or overlap or youtube or extract" -q: 547 pass; remaining failures are a localmcppackage version mismatch and a live-Lemonade ASR test.black --check/isort --check-onlyclean on every touched file.gaia eval agentrun: deferred by maintainer; the completion gate changes final answers, so decide before merge.🔍 Technical details
What a reviewer should know
main.Adversarial coverage (from #4145)
deepseek-v4p1-flashagainst real YouTube auto-captions of 13K, 21K and 74K characters; the 74K end-to-end agent run saved and verified 111 items in 122 s. Local Gemma-4-E4B completed the 13K file (63 items).Conflicts resolved against current
mainTwo, both in
src/gaia/agents/base/agent.py, plus an additive one in the flagshipCHANGELOG.md:mainadded a_drain_followupscheck that re-enters the loop rather than sealing the answer; fix(agents): verify completion and preserve exhaustive inventories #4145 replaced the same lines withfinalize_answer+ the completion-evidence gate. Kept both, drain first: a late user message still gets folded into the running turn, and the completion gate then runs on the answer that actually seals._refresh_active_skill_filtervs. fix(skills): a loaded skill brings the tools it declares #4369 (a loaded skill brings its tools). fix(skills): a loaded skill brings the tools it declares #4369 hoisted that call to before_refresh_active_tool_filter; fix(agents): verify completion and preserve exhaustive inventories #4145 still carried it in its old position below, so a naive merge called it twice per turn. Dropped the duplicate and keptmain's hoisted call — the extraction block that follows reads_active_skill_filter, so running it earlier is strictly correct.Verification of the resolution
main→ this branch): byte-identical on every file except the one blank line the de-duplication above removes. That rules out the silent near-identical duplication a large auto-merge can introduce.compile()(not justast.parse, which does not reject repeated keyword arguments) over every.pyinsrc/,tests/,hub/andscripts/: clean. Repo-wide sweep for stray conflict markers: none.