feat(reports): agent-facing report lifecycle — prompt, read-back, filters, scale, export (#1534) - #1838
Conversation
) The report MCP tool (#918) has been fully functional and invisible: nothing in PLATFORM_INSTRUCTIONS mentioned it, so reports only got published by agents whose own CLAUDE.md happened to say so. Add a "Publishing Reports" block — the call, when to reach for it, the payload shape per display_hint, and the aggregate-before-publishing expectation given the 256 KB cap. The shape table is the load-bearing part. An agent cannot guess `tiles` vs `metrics` or `markdown` vs `body`, and guessing wrong fails SILENTLY: the write succeeds and the dashboard quietly falls back to the raw JSON viewer. So the documented shapes are lifted from the renderers rather than invented, and pinned by tests against both other surfaces — the MCP tool's display_hint enum and the ReportRenderer payload keys. Verified those guards bite (tiles->metrics, timeline->chronology => 2 failed). One explicit line separates reports from the operator queue, because the neighbouring block documents an `alert` request type that looks like the same thing; without it agents publish a report when they need an approval and nothing ever answers them. Runtime-awareness came free via _adapt_instructions_for_runtime (#1187), but two things did need doing: the Codex orientation note enumerates tool names, so `report` joins it, and the call example is indented independently of the call name — stripping the mcp__trinity__ prefix would otherwise misalign every continuation line. Budget: the block ships on every turn of every agent, so the first draft was trimmed 1.8 KB -> 1.3 KB (~330 tokens, +18% on a 7.1 KB prompt) and a test caps it at 2 KB, making future growth a decision rather than an accident. Additive: templates that already instruct reporting are unaffected. First child of epic #1534 — chosen first because the other four (export, scale, read-back, search) all process agent_reports rows, and would otherwise be designed against a mostly-empty table. Related to #1535
|
Full unit suite on this branch: All 4 are pre-existing and environmental, not from this change — the first is One honesty note on sequencing: that 15-minute run started just before a whitespace-only tweak to the code example in the prompt block (indenting the call arguments independently of the call name, so stripping the Codex prefix doesn't misalign them). The five suites that actually read |
dolho
changed the title
Second child of epic #1534, on the same branch as #1535 because the two are one behaviour: the prompt block tells an agent to publish, and this tells it what it already published. Landing them apart would have shipped a prompt that says "continue the series" against a write-only surface. `list_reports` (metadata; filters agent_name/report_type/hours/search, paged) and `get_report` (payload by id) proxy the EXISTING access-controlled endpoints — no new endpoint, no new table, no new tenant-boundary logic in the MCP layer. The one gate the backend structurally cannot apply: an agent-scoped key resolves to its OWNER, so the backend scopes a read to everything the owner can see — wider than the calling agent's permits. The tools narrow a broad listing to {self} ∪ permitted (the #1104 rule list_operator_queue established) and re-check the owning agent on get_report. A denied get_report returns the backend's own "Report not found" shape rather than a distinguishable 403. That is deliberate: GET /api/reports/{id} answers 404 precisely so an id cannot be probed for existence, and returning "exists but forbidden" for agent keys would undo that choice one layer up. Write is untouched and stays self-gated — reading another agent's reports never widens what you can write. The #1535 prompt block now points at read-back in one sentence ("read back what you already filed... that is how you continue a series"), and the Codex orientation lists the two new tool names so the stripped prompt stays complete. Block is 1600 chars, still under the 2000 cap the #1535 test pins. Tests: src/mcp-server/src/tools/reports.test.ts — 9 cases driving the real execute() against a fake client: the pure filter, broad-listing narrowing for an agent key, no narrowing for a user key (with getPermittedAgents throwing to prove it is not consulted), scoped denial, self-listing, route selection, and the not-found-shape check on a denied get_report. Verified live against a running instance: list returns summaries with no payload field, get returns the payload. Docs: architecture.md (MCP catalog row 1→3 tools + read-back bullet), requirements FR-8, feature-flows/agent-reports.md, feature-flows.md changelog. Related to #1538
Added: child 2 of the epic — agent read-back (#1538)Kept on this branch rather than a second PR, per the "same PR" call. They're one behaviour: the prompt block tells an agent to publish, and this tells it what it already published. Shipping them apart would have meant a prompt that says "continue the series" pointing at a write-only surface. What
The gate the backend can't applyAn agent-scoped key resolves to its owner, so the backend scopes a read to everything the owner can see — wider than the calling agent's permits. The tools narrow a broad listing to One deliberate detail worth reviewing: a denied Write is untouched and stays self-gated — reading another agent's reports never widens what you can write. PromptThe #1535 block gains one sentence pointing at read-back, and the Codex orientation lists both new tool names so the prefix-stripped variant stays complete. Block is now 1600 chars, still under the 2000 cap the #1535 test pins. VerificationNine cases drive the real Driven live against a running instance: One edge worth knowing (pre-existing, not from this PR)That live report belongs to an agent I'd since deleted. Reports are KEEP-policy, so the row outlives the agent: the fleet list happily returns it, while Docsarchitecture.md (MCP catalog row 1→3 tools + a read-back bullet on the Agent Reports block), requirements FR-8, Epic #1534 status: children 1 (#1535) and 2 (#1538) are here. Remaining: search/filter (#1539), export (#1536), scale (#1537). The |
#1538) Two findings from /review on this PR, both mine: 1. `hours` accepted any non-negative integer, but the backend whitelists `_VALID_HOURS = {0,1,6,24,168,720}` and silently coerces anything else to 168 (routers/reports.py:136). So `hours: 48` answered with SEVEN DAYS of reports and nothing told the agent its window was ignored — the worst shape for a tool whose whole job is "read back what you filed, then decide". The schema now mirrors the whitelist and rejects the rest. 2. `get_report` re-checked the owning agent only `if (owner)`. A response without `agent_name` skipped the check and returned the payload — an auth gate failing OPEN on malformed input. Now an agent-scoped key with no resolvable owner gets the same "Report not found" as a denial; other scopes are unaffected (the backend already gated them). Also documents on the `hours` param that it is ignored on the per-agent route — `search` already said so and `hours` did not, which made the omission look deliberate for one and accidental for the other. #1539 gives that route both. Tests: 4 new cases — schema rejects 48 / accepts 24 and 0, the per-agent call carries only {report_type, limit, offset}, get_report refuses an ownerless response for an agent key and still serves a user key. 13 passed. Related to #1538
Third child of epic #1534. The fleet list has had report_type/hours/search since #918; the per-agent route had report_type only. Two consequences: the Agent Detail Reports tab was a flat unfilterable list, and any caller scoping to one agent — including the #1538 list_reports tool shipped hours ago — had both filters silently dropped. Both routes now build their WHERE from the SAME _fleet_conditions, with one parameterized difference: `search` matches agent_name on the fleet list (that is how you find "everything scout published") but NOT on a single-agent list, where every row already carries that name — searching "recon" inside agent `recon-bot` would have returned its entire history, indistinguishable from search being ignored. `hours` is whitelist-validated on both routes (_VALID_HOURS), falling back to the 7-day default instead of erroring, so an old client keeps working. The MCP tool now passes both down the scoped path and the "ignored when agent_name is set" caveats are gone from its description. Frontend: the fleet view's filter bar, minus the agent picker, on the per-agent tab; type options derive from the loaded page (no per-agent stats endpoint exists and inventing one for a dropdown would be a new surface); search debounced 300ms; the empty state now distinguishes "no reports yet" from "no reports match these filters". PAYLOAD IS NOT SEARCHED, deliberately. A LIKE over a 256 KB TEXT column with no index degrades exactly as reporting succeeds; an FTS answer belongs with #1537's storage rework rather than behind a filter box that looks free. Found by verifying live rather than by test: database.py delegated to the ops layer POSITIONALLY, so two new parameters rebound limit→hours and every request 500'd with "unexpected keyword argument 'hours'". A wholesale-mocked db cannot see that (learnings 2026-07-04). The facade now forwards by keyword and test_facade_forwards_the_new_filters pins it — verified failing against the positional version. Tests: 5 backend cases (search matches title/type, does NOT match the agent's own name while the fleet list still does, window excludes older rows, filters compose, facade forwarding) + the MCP test inverted to the new contract. 16 passed across the report suites; tsc clean; 13 MCP tests pass. Verified live: seeded 3 reports on a real agent, exercised every filter through the API, and drove the UI — the search box narrows the list to 1 row. Related to #1539
) Fourth child of epic #1534. Measured before designing: on a live fleet the existing reports averaged 201 bytes and the largest was 683 — four orders of magnitude under the 256 KiB cap. So the cap was never a limit agents were hitting; it was the wall the FIRST real tabular report would hit. That measurement decides the shape. Ceiling 256 KiB -> 5 MiB, and GET /api/reports/{id}/rows windows a `table` payload (offset/limit, columns once, the true total). Storage stays a single TEXT blob and there is NO migration: with no payload anywhere near the old cap, an off-row rows table would be a schema commitment made against a hypothetical. Frontend fetches tabular reports through the row reader — branching on the display_hint already present in the summary, so no extra request decides — with a "Showing N of M · Load more" footer. Other hints are bounded documents and still fetch whole. Create gains a Content-Length pre-check so an oversized body is refused on the header rather than after re-serializing the parsed payload; the exact byte check still enforces, and an unparseable header falls through to it rather than bypassing the cap. Non-tabular payloads answer 400 on the rows route (no row axis to slice, and inventing one is worse than saying so). No-access answers 404, matching GET /reports/{id} so the sibling route can't probe an id for existence. Verified end to end, not just unit-tested: a 12,000-row / 1.16 MB report (4.5x the old cap) creates successfully, the row reader answers total=12000 with 100 rows, and expanding the card in the UI transfers 8,699 bytes instead of 1.16 MB — 137x less over the wire. HONEST RESIDUALS, both documented rather than papered over: * the row slice happens in Python after the whole blob is read, so it bounds the RESPONSE, not the read. Moving it into SQL needs the off-row model. * Starlette buffers the body before the handler runs, so the Content-Length guard bounds storage and response size, not peak memory. A true streaming guard needs the body off the typed-model path (the webhooks.py pattern). Both should be triggered by a measured payload distribution approaching the new ceiling, not by this issue's premise. `request` is optional on create so direct in-process calls keep working — FastAPI injects it regardless — which is what kept the #918 endpoint tests green after the signature grew. Tests: 8 new cases (ceiling raised + still enforced, header rejection, unparseable header falls through, windowing with true total, offset window, past-the-end is an empty page not an error, non-tabular 400, missing/inaccessible 404). 25 passed across the report suites. Related to #1537
Fifth and last child of epic #1534. GET /api/reports/{id}/export?format=xlsx|pdf renders a stored report as a real spreadsheet — typed cells, both row encodings (positional list and column-keyed object) landing in the declared column order — or a formatted PDF where a table stays a table and markdown stays prose. Builders are pure (payload, display_hint, title) -> bytes in services/report_export.py; the router owns access, format validation, headers. Tests read the produced file BACK (openpyxl loads the workbook, the PDF magic is checked) because a mocked workbook asserts that a method was called, not that a value landed in the right cell — which is the only question an export raises. Shape mismatch degrades, never 500s: kpi -> label/value/unit, timeline -> event columns, anything unrecognized -> pretty-printed JSON in one cell. None of those is an error path; a stakeholder holding a plain file beats one holding a trace. Three decisions worth reviewing: * Dependencies are openpyxl + reportlab, both PURE-PYTHON wheels, so the image build is unchanged beyond two pins. WeasyPrint was rejected for needing cairo/pango — a PDF button should not become a container-build concern. * They are imported LAZILY. start.sh does not rebuild on an in-place upgrade (#1814), so a module-level import would take the whole reports router down on such an instance. Lazily importing turns that into one endpoint answering 503 with a rebuild hint. Confirmed live on an un-rebuilt container: 503 with "Rebuild the backend image", not a 500. * Access reuses the detail route's 404-not-403 — an export URL must not become the existence oracle that route deliberately refuses to be. Content-Disposition is built from a sanitized title (quotes/newlines/separators stripped, not escaped) and carries nosniff like the FILES-001 download. The PDF caps at 2000 rows WITH A VISIBLE NOTE pointing at the spreadsheet. Silent truncation of an export is a data-integrity trap, and a 12,000-row PDF is not a document anyone reads. The cap decision is a separate pure function because reportlab compresses content streams — asserting on rendered bytes would have proven nothing about whether the user was told. Agent-authored text is escaped before reportlab parses its mini-HTML dialect: an unclosed <tag in a report body would otherwise reflow or crash the document. Verified live against real data, not only unit tests: the 12,000-row / 1.16 MB report from #1537 exports to a 362 KB .xlsx that reads back with all 12,000 rows and typed values, and to a 153 KB PDF; a markdown report exports to a 1.7 KB PDF. Tests: 17 new cases. 48 passed across every report suite. Related to #1536
AndriiPasternak31
left a comment
AndriiPasternak31
left a comment
There was a problem hiding this comment.
/validate-pr — Request changes
Two blockers, both concrete and cheap to fix, plus a scope question that decides how this gets reviewed at all.
First the credit, because it's earned: the docs on this branch are the best I've read in a while — FR-7 through FR-11 in requirements/lifecycle-observability.md, architecture.md updated per-surface, the feature flow refreshed, and the rationale is measured (201-byte average payload as the argument for 5 MiB rather than a vibe). The cross-surface drift guards in test_1535_report_prompt_guidance.py are exactly the right instinct: pinning the prompt's shape table to the MCP z.enum and to the renderer's dispatch keys is how you stop a silent-fallback bug. Security scan is clean, the Dockerfile pins are mirrored into tests/requirements-test.txt, lazy imports → 503-with-rebuild-hint instead of a router-import crash is the right call for #1814, and the MCP read gate correctly mirrors the #1104 {self} ∪ permitted rule.
1. ❌ CI is red on a real regression, not a flake
test_1310_auth_wiring::test_no_inline_auth_gates_in_routers fails on HEAD under all three seeds and passes on BASE under all three. Deterministic. Reproduced against the head file with the guard's own AST predicates:
INV-8 inline gate -> export_report() reports.py:235
INV-8 inline gate -> get_report_rows() reports.py:304
INV-8 inline gate -> get_report() reports.py:330 # allowlisted
The two new routes use the if not report or not db.can_user_access_agent(...): raise HTTPException(404) shape, which is the exact pattern Invariant #8's guard exists to catch. Only ("reports.py", "get_report") is allowlisted.
To be clear — the design is right: uniform-404 so an export URL can't become the id oracle the detail route refuses to be. The guard is complaining about the wiring, not the intent. Cleanest fix is one private helper rather than three allowlist entries:
def _report_or_404(report_id: str, current_user: User) -> dict:
"""Fetch + gate. 404 (not 403) so a report id stays unprobeable."""
report = db.get_report(report_id)
if not report or not db.can_user_access_agent(current_user.username, report["agent_name"]):
raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail="Report not found")
return reportAll three routes call it, the allowlist becomes ("reports.py", "_report_or_404"), and the duplicated gate collapses from three copies to one — which is the actual risk the guard is proxying for.
2. ❌ The prompt block ships a number that is wrong on merge
platform_prompt_service.py:63
payload={...}, # max 256 KB serialized
models.py:358
REPORT_PAYLOAD_MAX_BYTES = 5 * 1024 * 1024 # 5 MiBSame PR. So on merge every agent, on every turn, is told the ceiling is 20× below what the platform actually accepts.
This isn't cosmetic — it partly cancels #1537. The stated purpose of the raise is "the wall the first real table would hit"; an agent that believes the wall is at 256 KB will pre-aggregate a 1 MB lead list down before publishing, and the raise buys nothing. It's also precisely the silent-divergence class this PR's own test file was written to defend against: six drift guards, and none of them pins the size.
Fix: correct the number, and add the seventh guard — assert the byte figure in the block matches REPORT_PAYLOAD_MAX_BYTES. Interpolating the constant into the block is even better, since then it can't drift again.
Two stale echoes of the same number to sweep while you're there: architecture.md (unindexed LIKE over a 256 KB blob) and requirements FR-9 (a LIKE over a 256 KB TEXT blob).
3. ⚠️ The description is for a different PR than the branch
The body says:
Not in scope — Reports remain write-only from the agent's side (#1538), unsearchable (#1539), un-exportable (#1536) and capped at 256 KB (#1537) — the other four epic children.
The branch contains all four. Seven commits, 25 files, +2278/−35: a new services/report_export.py (238 lines), two new REST endpoints, two new MCP tools, the cap raise, five frontend files, three additional test modules. report_export.py does not exist on dev — none of this is inherited from the merge commit.
A reviewer reading that description prepares to review ~1.3 KB of prompt text and is instead handed a new service module and two new endpoints. That's how the auth-wiring gate in #1 slips past a human: it's in the half of the PR the description says isn't there.
The docs already discuss all five as separate FRs and each commit is clean and independently revertible, so splitting into the five PRs the epic already has issues for is close to free — git cherry-pick per commit onto five branches. If you'd rather land it as one, that's a legitimate call given they're one coherent epic slice, but then the title and body need to say so, and #1535's "prompt-only, 1.3 KB, +18%" framing has to go — the trade being made is no longer the one the body argues for.
My preference is the split: #1535 is a prompt-text change whose whole review is "are these five shapes right", while #1537 is a 20× storage-ceiling decision. Those want different reviewers and different scrutiny.
4. ⚠️ Title is still wip: and there's no closing keyword
Related to #1535 is a bare reference. .github/workflows/issue-status-on-merge.yml only promotes on Fixes / Closes / Resolves, so on merge all five issues strand in status-in-progress and get swept up by the next /groom. Needs Closes #1535 (plus the others, or one per PR if split). And drop the wip: — the PR is marked ready for review.
5. Minor
routers/reports.py:62 — request: Request = None should be Optional[Request]. FastAPI injects it regardless so the behaviour is fine, but the annotation currently says a non-optional type has a None default.
Validation summary
| Category | Status | Notes |
|---|---|---|
| Base branch | ✅ | targets dev |
| Commit messages | ✅ | conventional, one per issue, clean |
| PR size | 25 files / +2278 — five issues in one PR (#3) | |
| Issue link | bare #1535, no closing keyword (#4) |
|
| Requirements | ✅ | FR-7…FR-11, thorough |
| Architecture | ✅ | per-surface, incl. the drift-pinning rationale |
| Feature flows | ✅ | agent-reports.md + index |
| Security scan | ✅ | no secrets / emails / IPs / creds |
| Build packaging | ✅ | new module under services/ (copied wholesale); pins added to Dockerfile + test reqs |
| Config packaging | ➖ | no new os.getenv |
| Test adequacy | ✅ | 4 new modules + MCP unit tests; non-happy-path covered (413, 400, 503, 404) |
| CI | ❌ | 1 deterministic new failure across 3 seeds (#1) |
| Cross-surface consistency | ❌ | prompt says 256 KB, code says 5 MiB (#2) |
Blocking: #1 and #2. Needs a decision before re-review: #3.
Happy to re-review as soon as the guard is green — and if you split, ping me and I'll take the five in sequence.
…eiling (#1838 review) Addresses the two blockers from @AndriiPasternak31's /validate-pr. 1) CI red on a real regression, not a flake. `test_1310_auth_wiring::test_no_inline_auth_gates_in_routers` flagged two new inline agent-gates (`export_report`, `get_report_rows`). The design was right — uniform 404 so an export URL can't become the id oracle the detail route refuses to be — but the wiring was three byte-identical copies of the gate, which is exactly the duplication the guard proxies for. Extracted `_report_or_404(report_id, current_user)`; detail / rows / export all call it. The allowlist drops from a would-be three entries to one, and the gate exists once, so it cannot drift into a 403 in one route only. The allowlist meta-test moves with it (its synthetic function has to carry the allowlisted name or it stops testing anything). Green across seeds 1/2/3. 2) The prompt shipped a ceiling 20x below the enforced one. The block said `max 256 KB` while `REPORT_PAYLOAD_MAX_BYTES` was already 5 MiB in the same PR. That partly cancels #1537: an agent told the wall is at 256 KB pre-aggregates exactly the payloads the raise exists to accept. The block now INTERPOLATES the constant (`__REPORT_PAYLOAD_MAX__` substituted at import) rather than restating it, so it cannot drift again, plus the seventh drift guard asserting the figure matches and that `256 KB` is gone. A third agent-facing surface carried the same stale number and was not in the review: the MCP `report` tool's own `payload` description. Fixed — a tool description is read by the model just as the prompt is. Docs swept where they state CURRENT behaviour (architecture.md x3, agent-reports.md x3, lifecycle-observability.md x2). The before/after table, the 201-byte measurement narrative, and `models.py`'s "raised from 256 KiB" are history and stay as written. Not changed: `request: Request = None` (review item 5). `Optional[Request]` is wrong here — FastAPI special-cases the bare annotation as an ASGI injection; wrapping it makes FastAPI build a Pydantic field for it and the module fails to import outright ("Invalid args for response field!"). Verified by making the change and watching 9 tests fail. Comment added so it isn't re-suggested. Related to #1535, #1536, #1537, #1538, #1539
dolho
changed the title
|
@AndriiPasternak31 — both blockers fixed in 1 — auth wiring. Took your 2 — the ceiling. Interpolated rather than corrected, per your "even better" — the block substitutes Sweeping your two stale doc echoes turned up a third live surface with the same number that wasn't in the review: the MCP 3 — scope. Keeping it as one PR; the body now says five issues plainly and the "prompt-only, 1.3 KB, +18%" framing is withdrawn. Your point stands that the old description is how the auth gate slipped a human read, and that was the real damage. If you'd still rather take them in sequence, say so and I'll split — the commits are one-per-issue and cherry-pick cleanly. 5 — not applied, and I think the suggestion is wrong. I made the change before rejecting it — 9 tests fail on import. Reverted with a comment recording why, so it doesn't get re-suggested. Happy to be shown otherwise if you've seen it work. Ready for re-review. |
Reported from the UI: clicking **Export .xlsx** returned 401.
The buttons were `<a :href="/api/reports/{id}/export?format=xlsx" download>`,
carrying a comment that claimed "the session cookie/JWT interceptor is not
involved in a binary body". That reasoning is wrong for this platform. Trinity
holds its JWT in localStorage and attaches it via the `api.js` request
interceptor; a raw browser navigation sends no Authorization header at all, so
the endpoint correctly refused it. Export was unusable from the UI — the entire
point of #1536 — even though the endpoint itself was fine.
Why it survived verification: I tested the ENDPOINT with
`curl -H "Authorization: Bearer ..."`, which passes, instead of the BUTTON,
which is the feature. An endpoint that works under curl and 401s under a click
is exactly the gap that shape of testing cannot see.
Fix: `downloadReportExport(reportId, format)` in the reports store fetches
through the shared api client (so the interceptor runs), then hands the browser
a blob URL. Mirrors the existing `agents.js:getFilePreviewBlob` pattern and
keeps Invariant #7 (one API client, no raw fetch). The server's
Content-Disposition filename is still what names the file. Both panels
(per-agent + fleet) switch from anchors to buttons with an in-flight state, and
a failure now surfaces the backend detail — notably the #1814 503 rebuild hint —
instead of a silent no-op.
Guard: `test_1536_export_download_auth.py` — a static check, because the defect
lives in markup rather than in a callable. Asserts the anchors are gone, both
panels call the store helper, and the helper requests a blob through `api`.
Verified it fails on the pre-fix markup (5 failed) and passes after.
Verified in a real browser, clicking the real buttons: both formats download
200 with the server-supplied filenames (362 KB xlsx / 153 KB pdf), no alert.
Related to #1536
…sults (#1535) Found in live use, not in a test. Asked "I need a list of weather for 500 places in europe" from the Chat tab, the agent hand-wrote a CSV + JSON to /home/developer/public/, called share_file, hit FEATURE_DISABLED because sharing was off for that agent, and delivered nothing downloadable. The report block sat directly underneath, unused — on the single request it describes best (500 rows, tabular, exportable). Two causes, both in the prompt, both mine: 1. The "Sharing Files with Users" block sits IMMEDIATELY above and its trigger list literally read "(CSV, PDF, report, image, exported data, etc.)". It claimed the word `report` and `exported data`, so it won every structured request. The dead-end message the user saw is that block's own fallback text verbatim. 2. The report block's trigger was framed entirely around recurring work — "a scheduled run", "numbers someone compares against next period". A one-off interactive "give me 500 rows" matched none of it. Fixes: the file-sharing trigger now lists only genuine file artifacts (image, PDF, document, generated asset) and explicitly hands structured results to the report block, noting reports export to Excel/PDF anyway and work even when file sharing is off. The report trigger now leads on SHAPE and VOLUME — "any result that is rows-and-columns … a table you just produced (10 rows or 10,000)" — and says outright that publishing a report beats hand-writing a CSV + share_file. The lesson generalises: a prompt block cannot be validated in isolation. Six existing guards pinned this block against the MCP enum, the renderer keys and the byte ceiling, and every one passed while the block was being out-competed by its neighbour. What the ADJACENT block claims decides which one fires. Guards: `test_file_sharing_block_does_not_claim_structured_results` (scoped to the parenthesised claim list, so the handoff pointer doesn't self-trip) and `test_report_trigger_covers_a_one_off_table_not_only_scheduled_work`. Verified by re-running the exact failing request on the same agent: type : weather.europe_500_cities title: Current Weather — 500 European Cities (2026-07-29) hint : table rows : 505 cols : City, Country, Temp °C, Feels Like °C, Humidity %, Wind km/h, Wind Dir, Precip mm, Condition Honest caveat: the agent still called share_file once before publishing, so the redirect is a strong preference rather than an absolute. The deliverable is now the report, which is the outcome that matters. Block is 1888 chars, under the 2000 budget. Related to #1535
|
Resolve by running |
AndriiPasternak31
left a comment
AndriiPasternak31
left a comment
There was a problem hiding this comment.
Approving. Round-1 findings are all resolved, and #5 was correctly rejected — the bare Request annotation is a FastAPI ASGI-injection contract, and the comment recording that is the right outcome so it isn't re-suggested.
Spot-checking the invariants I expected this to break, it doesn't: _fleet_conditions is shared by both list routes (Inv #1/#2), the window uses iso_cutoff() rather than datetime('now') (Inv #16), /reports/stats still precedes /reports/{report_id} (Inv #4), the Codex prompt strip is a global .replace("mcp__trinity__", "") so the new block is covered for free, and the export download goes through the shared api client with a static guard pinning it (Inv #7). The _report_or_404 consolidation is the right answer to the auth-wiring guard — one gate, one allowlist entry, uniform 404 preserved. Non-happy-path coverage is well above average: 404-not-403, 503-not-500, unparseable Content-Length, offset-past-end, non-tabular 400, reportlab markup escaping, MCP fail-closed.
Follow-up (not blocking this PR)
export_report runs the builders on the event loop — routers/reports.py:268 and :271 call build_xlsx/build_pdf synchronously inside an async def handler, so they execute on the loop rather than in FastAPI's threadpool.
I measured it against the real builders at the new ceiling rather than guessing:
| payload | builder | wall time |
|---|---|---|
| 5.10 MiB / 72,000 rows | build_xlsx |
1.57 s |
same, capped at PDF_MAX_ROWS |
build_pdf |
0.24 s |
The PDF cap does its job; the xlsx path is the uncapped one. On container CPU that is plausibly 3-5 s, and /export has no rate limit (rate_limiter.enforce is only in create_report), so concurrent exports serialize into N x that stall on the worker — blocking heartbeat processing, WS dispatch and every other request on it.
Not blocking, because the expensive input does not exist yet: the fleet's reports average 201 bytes, which is precisely why #1537 raised the cap. This is a latent characteristic of a path whose worst-case payload nobody has published. Fix is one line when someone picks it up:
content = await asyncio.to_thread(report_export.build_xlsx, ...)Minor
docs/memory/feature-flows.md— the index row credits only #1535/#1538 ("children 1-2"); #1536/#1537/#1539 ship in the same PR and aren't in it.- #1536 carries only
status-in-progress— no priority or type label, so it's invisible to/roadmapordering. db/reports.py—like = f"%{search}%"passes%/_through as wildcards. Parameterized so not injection, and pre-existing from #918, but worth escaping while the code is open.
On the five-issues-in-one-PR question: keeping it as one PR is the right call given the children only make sense together, and the body now says so plainly. The commits are cleanly separable if that judgement turns out wrong.
3 participants
Epic #1534, five children in one PR. Landing them together is deliberate — see Scope below.
Closes #1535
Closes #1536
Closes #1537
Closes #1538
Closes #1539
What
reporttool in the platform prompt: when a result belongs in a report rather than chat, the five payload shapes, and read-back-before-refilinglist_reports/get_report— reports stop being write-only from the agent's sidehours+searchon the per-agent list (the fleet list had them since #918)REPORT_PAYLOAD_MAX_BYTES256 KiB → 5 MiB +GET /api/reports/{id}/rowspaginated row readerGET /api/reports/{id}/export?format=xlsx|pdfvia a newservices/report_export.pyScope
This is five issues, not one. The earlier description claimed prompt-text only, which was wrong and is corrected here — a reviewer prepared for ~1.3 KB of prompt text was instead handed a new service module and two endpoints, which is how the auth-wiring gate below slipped past a human read.
They ship together because they are one coherent epic slice: #1535 tells agents to publish reports, and #1537/#1538/#1539/#1536 are what makes that instruction honest (a payload big enough to matter, readable back, findable, exportable). Each commit is clean and independently revertible if that judgement turns out wrong.
The
+18% / prompt-onlyframing from the previous body is withdrawn — that is not the trade being made.Review round 1 — @AndriiPasternak31
1. CI red on a real regression ✅ fixed in
a02a9f59test_1310_auth_wiring::test_no_inline_auth_gates_in_routersflaggedexport_reportandget_report_rows. The uniform-404 design was right; the wiring was three byte-identical copies of the gate — the duplication the guard proxies for. Took the suggested shape: one_report_or_404(report_id, current_user), all three routes call it, allowlist is one entry instead of three. The allowlist meta-test moved with it (its synthetic function must carry the allowlisted name or it stops asserting anything). Green on seeds 1/2/3.2. Prompt ships a 20×-wrong ceiling ✅ fixed in
a02a9f59Correct, and correct about the consequence — it partly cancels #1537. Took the stronger option: the block now interpolates
REPORT_PAYLOAD_MAX_BYTESinstead of restating it, so it cannot drift again, plus the seventh drift guard (asserts the figure matches, asserts256 KBis absent).Sweeping for the two stale doc echoes turned up a third live agent-facing surface carrying the same number, which wasn't in the review: the MCP
reporttool's ownpayloaddescription (tools/reports.ts:106). A tool description is read by the model exactly as the prompt is, so it was the same bug in the same class. Fixed. Docs updated where they state current behaviour (architecture.md ×3, agent-reports.md ×3, lifecycle-observability.md ×2); the before/after table, the 201-byte measurement narrative andmodels.py's "raised from 256 KiB" are history and stay.3. Description was for a different PR ✅ fixed — this body
Kept as one PR. Title and body now say five issues plainly and the prompt-only framing is gone.
4.
wip:title, no closing keyword ✅ fixed — title updated, fiveClosesabove5.
request: Request = None→Optional[Request]❌ not applied — the suggestion is wrongFastAPI special-cases the bare
Requestannotation as an ASGI injection. Wrapping it inOptionalloses that, so FastAPI tries to build a Pydantic field for it and the module fails to import:Verified by making the change and running the suite — 9 tests failed on module import. Reverted, with a comment recording why so it isn't re-suggested. The annotation is a framework contract here, not a nullability claim.
Verification
test_1310_auth_wiring— 12 passed (was 1 failed), seeds 1/2/3test_1535/test_1536/test_1537/test_1539/test_918_report_broadcast+ auth wiring: 34 passed, 1 skippednpx tsc --noEmiton the MCP server: clean🤖 Generated with Claude Code