-
Notifications
You must be signed in to change notification settings - Fork 0
Codebase Review
A strict review of DeepDelve's architecture, state management, grounding systems, and specific bug patterns, from an early point in the project's history. Most of the flaws it found have since been fixed; each entry below is marked with its real current status, checked directly against the live code rather than left to stand as if still open.
Main strengths: per attempt quota top up (fixes retry starvation), auto fetch fusion (search and fetch combined so the LLM can't shortcut to snippets), artifact quarantine (renames flawed drafts aside so the model can't re condition on its own mistakes), and deterministic URL grounding (tracking what was actually fetched, not trusting the model's narration).
Note
Fixed, though not the way originally proposed below. The original flaw and fix sketch are kept for the historical reasoning; see Completed's "Immediate narration salvage" entry for what actually shipped.
_salvage_narrated_report is meant to catch a report the model narrates as chat text instead of
writing via write_workspace_file. It was flawed because the text accumulator reset every loop
iteration in CLI mode, and state["current_msg"] cleared to None on any tool call in TUI mode, so
salvage only ever checked the immediate last turn's text. In a real traced incident, the model
narrated a full, accurate report on attempt 3 but never called the write tool, then on the final
attempt produced nothing at all; the good narration from attempt 3 was thrown away in favor of
attempt 4's empty string.
The fix sketched at the time was to scan backward through the session log for the last substantial Planner text block instead of trusting only the immediate last turn. The fix that actually shipped took a different, simpler shape: salvage now runs immediately after every Write dispatch, using that dispatch's own returned text directly, rather than scanning history for an older one. Same underlying problem closed, different mechanism.
Note
Half fixed. DocumentAnalyzer/DataAnalyzer now have a real, structural tool-level distinction;
WebSearcher/AcademicSearcher still share an identical tool list.
WebSearcher/AcademicSearcher and DocumentAnalyzer/DataAnalyzer shared identical tool lists in
app.py, nothing but the prompt enforced the split, so a misrouted task or model drift had no
system level guardrail to catch it. DataAnalyzer has since gained extract_structured_data, a
tool DocumentAnalyzer deliberately does not have, giving that pair a real structural distinction
(table/code/JSON/CSV extraction versus prose reading), confirmed directly in app.py's own
comments. WebSearcher and AcademicSearcher still share the exact same tool list
(web_search, fetch_url_to_workspace, think_tool, search_verified_findings); that half of the
original critique still stands.
Note
Still accurate as stated. The real-world consequence for multi-user web mode was avoided by design elsewhere (see below), not by fixing this directly.
Session state (_session_events, _current_call_by_source, etc.) is still module level globals in
tui.py, fine for single user CLI but a real risk in web mode, where multiple concurrent users
would share and corrupt the same lists. This didn't get fixed at the source; instead, the optional
HTTP API added later (see Completed's "Output/export gaps" entry) sidesteps it
architecturally with a single in-process job queue that's always single-flight, so two runs can
never actually execute concurrently against these globals in practice, even though the underlying
globals themselves are unchanged.
Note
Fixed. Now actively wired with 3 real call sites.
Was defined but never called anywhere, so the structured findings cache stayed empty and the "reuse
extracted facts across queries" goal was only half implemented. orchestrator.py's own
_run_single_task now calls run_state.add_finding() directly at 3 call sites, and a code comment
in run_state.py notes plainly that this was "the reason RunState.add_finding() existed but was
dead code until this wiring."
Note
Fixed. The shipped fix matches the one originally proposed here almost verbatim.
A multi word proper noun starting with a stopword ("The Python Programming Language") used to get
discarded entirely instead of just stripping the leading stopword, causing false negatives in
grounding checks. grounding.py's _normalize_proper_noun_phrase now strips leading stopwords in a
loop and only discards the phrase if nothing real is left, its own docstring citing the identical
"The Python Programming Language" example as the motivating case.
Note
Fixed. The shipped fix matches the one originally proposed here almost verbatim.
A shared, lazily initialized DDGS() singleton used to get called concurrently from
asyncio.gather, and the library isn't documented as thread safe for that. web.py now
instantiates a fresh, short-lived DDGS() client inside each search call instead of reusing a
global instance, its own comment citing exactly this concurrency concern as the reason.
Query: "Who eliminated Colombia from the 2026 FIFA World Cup?" (Switzerland, round of 16, 0-0 then 4-3 on penalties, outside any local model's training data). The agent burned its first two attempts on memory refusal and planning without delegating, finally researched correctly on attempt 3 but forgot to write the report, then on attempt 4 wrote a fully correct, well synthesized report, but cited a FIFA match URL it never actually fetched, only a Sporting News page it had really fetched was real. The grounding check correctly caught the fabrication and refused to verify the report, exactly as designed. Net result: fact accuracy was 100%, but the agent fabricated a more authoritative sounding citation for content it already had a real source for, and had already burned its retry budget on the earlier delegation stalling.
History
Model Research
Reviews & Audits
Reference