Rifqi Hafizuddin commited on
Commit ·
8dec182
1
Parent(s): 11de970
[noticket] fix report bug where some answered analysis isnt included in report summary answers
Browse files- src/agents/report/generator.py +21 -8
- src/agents/report/readiness.py +56 -2
src/agents/report/generator.py
CHANGED
|
@@ -28,7 +28,7 @@ from src.middlewares.logging import get_logger
|
|
| 28 |
from ..language import detect_reply_language
|
| 29 |
from ..slow_path.schemas import AnalysisRecord, TaskSummary
|
| 30 |
from .errors import ReportError
|
| 31 |
-
from .readiness import
|
| 32 |
from .schemas import (
|
| 33 |
AnalysisReport,
|
| 34 |
AttributedNote,
|
|
@@ -611,13 +611,26 @@ class ReportGenerator:
|
|
| 611 |
excluded_ids = set(exclude_record_ids or [])
|
| 612 |
excluded = [r for r in all_records if r.record_id in excluded_ids]
|
| 613 |
kept = [r for r in all_records if r.record_id not in excluded_ids]
|
| 614 |
-
# The report body reflects
|
| 615 |
-
#
|
| 616 |
-
#
|
| 617 |
-
#
|
| 618 |
-
#
|
| 619 |
-
|
| 620 |
-
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| 621 |
if not records:
|
| 622 |
raise ReportError(f"no analyses recorded for {analysis_id!r} yet")
|
| 623 |
|
|
|
|
| 28 |
from ..language import detect_reply_language
|
| 29 |
from ..slow_path.schemas import AnalysisRecord, TaskSummary
|
| 30 |
from .errors import ReportError
|
| 31 |
+
from .readiness import has_reportable_result
|
| 32 |
from .schemas import (
|
| 33 |
AnalysisReport,
|
| 34 |
AttributedNote,
|
|
|
|
| 611 |
excluded_ids = set(exclude_record_ids or [])
|
| 612 |
excluded = [r for r in all_records if r.record_id in excluded_ids]
|
| 613 |
kept = [r for r in all_records if r.record_id not in excluded_ids]
|
| 614 |
+
# The report body reflects every run that produced work worth showing —
|
| 615 |
+
# `has_reportable_result`, NOT the floor's `has_successful_analysis`. The two
|
| 616 |
+
# were the same predicate until planner recipes R2/R2b made the `analyze_*`
|
| 617 |
+
# step optional: a grouped/scalar aggregate answered entirely inside one
|
| 618 |
+
# `retrieve_data` IR is a complete analysis with no analyze_* tool, and the
|
| 619 |
+
# floor's predicate dropped those runs from the body, so their business
|
| 620 |
+
# question rendered "Unanswered". Runs whose analysis step actually FAILED
|
| 621 |
+
# are still excluded here (they can't contradict the real findings) and still
|
| 622 |
+
# surface in the JSON `unresolved` list and the /records curation endpoint
|
| 623 |
+
# (the rendered markdown section was dropped 2026-07-09).
|
| 624 |
+
records = [r for r in kept if has_reportable_result(r)]
|
| 625 |
+
unresolved_records = [r for r in kept if not has_reportable_result(r)]
|
| 626 |
+
if unresolved_records:
|
| 627 |
+
# The dropped-runs path was previously silent, which is why a correct
|
| 628 |
+
# answer showing up as "Unanswered" took a bug report to find.
|
| 629 |
+
logger.info(
|
| 630 |
+
"report: runs excluded from body",
|
| 631 |
+
analysis_id=analysis_id,
|
| 632 |
+
excluded=[r.record_id for r in unresolved_records],
|
| 633 |
+
)
|
| 634 |
if not records:
|
| 635 |
raise ReportError(f"no analyses recorded for {analysis_id!r} yet")
|
| 636 |
|
src/agents/report/readiness.py
CHANGED
|
@@ -71,6 +71,20 @@ def _is_newer(a: datetime, b: datetime) -> bool:
|
|
| 71 |
return a > b
|
| 72 |
|
| 73 |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| 74 |
def has_successful_analysis(record) -> bool:
|
| 75 |
"""True if the record has at least one *result-producing* task that succeeded.
|
| 76 |
|
|
@@ -80,14 +94,54 @@ def has_successful_analysis(record) -> bool:
|
|
| 80 |
(2026-07-14), a completed `render_chart`, whose viz-tail upstream necessarily
|
| 81 |
computed the numbers being charted — is the real "we produced a result"
|
| 82 |
signal. A chart-only session therefore satisfies the report floor.
|
|
|
|
|
|
|
|
|
|
| 83 |
"""
|
| 84 |
return any(
|
| 85 |
-
t.status == "success"
|
| 86 |
-
and any(tool.startswith("analyze") or tool == "render_chart" for tool in t.tools_used)
|
| 87 |
for t in record.tasks_run
|
| 88 |
)
|
| 89 |
|
| 90 |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| 91 |
async def report_floor(
|
| 92 |
analysis_id: str | None,
|
| 93 |
state: AnalysisState,
|
|
|
|
| 71 |
return a > b
|
| 72 |
|
| 73 |
|
| 74 |
+
# Catalog-introspection tools return metadata, not results — a run that only
|
| 75 |
+
# inspected the schema has nothing for the report body to show.
|
| 76 |
+
_CATALOG_ONLY_TOOLS = frozenset({"check_data", "check_knowledge"})
|
| 77 |
+
|
| 78 |
+
|
| 79 |
+
def _is_analysis_tool(tool: str) -> bool:
|
| 80 |
+
"""A tool whose success means "we computed a result" (analyze_* / render_chart).
|
| 81 |
+
|
| 82 |
+
Shared by `has_successful_analysis` (the floor) and `has_reportable_result`
|
| 83 |
+
(the body) so the two can never drift on what counts as an analysis step.
|
| 84 |
+
"""
|
| 85 |
+
return tool.startswith("analyze") or tool == "render_chart"
|
| 86 |
+
|
| 87 |
+
|
| 88 |
def has_successful_analysis(record) -> bool:
|
| 89 |
"""True if the record has at least one *result-producing* task that succeeded.
|
| 90 |
|
|
|
|
| 94 |
(2026-07-14), a completed `render_chart`, whose viz-tail upstream necessarily
|
| 95 |
computed the numbers being charted — is the real "we produced a result"
|
| 96 |
signal. A chart-only session therefore satisfies the report floor.
|
| 97 |
+
|
| 98 |
+
This is the report **FLOOR** only. The report *body* uses the broader
|
| 99 |
+
`has_reportable_result` — see its docstring for why the two diverged.
|
| 100 |
"""
|
| 101 |
return any(
|
| 102 |
+
t.status == "success" and any(_is_analysis_tool(tool) for tool in t.tools_used)
|
|
|
|
| 103 |
for t in record.tasks_run
|
| 104 |
)
|
| 105 |
|
| 106 |
|
| 107 |
+
def has_reportable_result(record) -> bool:
|
| 108 |
+
"""True if this run produced work the report **BODY** should include.
|
| 109 |
+
|
| 110 |
+
Deliberately distinct from `has_successful_analysis`, which stays the report
|
| 111 |
+
FLOOR. The two answer different questions:
|
| 112 |
+
|
| 113 |
+
floor — "is this session worth generating a report for at all?"
|
| 114 |
+
body — "did this particular run produce work the report should show?"
|
| 115 |
+
|
| 116 |
+
They were the same predicate until planner recipes R2/R2b made the `analyze_*`
|
| 117 |
+
step optional (`planner.md`: R2 "ONE grouped retrieve_data IR (± analyze_aggregate)";
|
| 118 |
+
R2b "NO analyze_* step"). A grouped or scalar aggregate answered entirely inside
|
| 119 |
+
one `retrieve_data` IR is a correct, complete analysis that uses no `analyze_*`
|
| 120 |
+
tool — so keying the body on the floor's predicate silently dropped those runs.
|
| 121 |
+
Their business question then rendered "Unanswered" in the report with no trace,
|
| 122 |
+
because the "Attempted, Unresolved" section is commented out (2026-07-09).
|
| 123 |
+
|
| 124 |
+
Rule:
|
| 125 |
+
- The plan HAS an analysis step -> it must have succeeded (unchanged). A run
|
| 126 |
+
whose analysis failed still belongs in `unresolved`, so its failure
|
| 127 |
+
narration can't contradict the successful runs' findings.
|
| 128 |
+
- The plan has NO analysis step -> a successful data-producing task is enough
|
| 129 |
+
(`check_*` alone is not — that inspected the schema, it did not answer
|
| 130 |
+
anything).
|
| 131 |
+
"""
|
| 132 |
+
tasks = record.tasks_run
|
| 133 |
+
plan_has_analysis = any(
|
| 134 |
+
_is_analysis_tool(tool) for task in tasks for tool in task.tools_used
|
| 135 |
+
)
|
| 136 |
+
if plan_has_analysis:
|
| 137 |
+
return has_successful_analysis(record)
|
| 138 |
+
return any(
|
| 139 |
+
task.status == "success"
|
| 140 |
+
and any(tool not in _CATALOG_ONLY_TOOLS for tool in task.tools_used)
|
| 141 |
+
for task in tasks
|
| 142 |
+
)
|
| 143 |
+
|
| 144 |
+
|
| 145 |
async def report_floor(
|
| 146 |
analysis_id: str | None,
|
| 147 |
state: AnalysisState,
|