Fix annotations list hiding every pre-project-scoping review - #2427
Open
nicolai-rhesis wants to merge 3 commits into
Open
Fix annotations list hiding every pre-project-scoping review#2427nicolai-rhesis wants to merge 3 commits into
nicolai-rhesis wants to merge 3 commits into
Conversation
The annotations query scoped test results with strict project equality, while the ORM auto-filter and the project_isolation RLS policy both admit project_id = :pid OR project_id IS NULL. A run whose test configuration carries no project stamps its results with a NULL project_id, so their reviews showed on the test run page but were invisible to /annotations/ — the architect reported no annotations on runs that visibly had them. trace.project_id is NOT NULL, so the trace branch needs no allowance.
_compact_list_result_for_history renders one line per item from name, id and description. Annotations have none of those — no name, no title, and a null id — so a page of reviews arrived at the LLM as a count plus a column of '- ?'. Told that N reviews existed but shown none of them, the architect invented reviewers, comments and turn numbers. Items without a name now render as their own trimmed JSON: empty values dropped, long strings clipped, non-ASCII left readable. Named entities keep the existing compact form. Also marks truncated tool results, which otherwise read as complete records.
The NULL project_id population is not runs that happen to lack a project — it is every row predating migration a1b2c3d4e5f0, which added project_id as a nullable column with no backfill.
There was a problem hiding this comment.
Looks good. The SQL scoping change matches the ORM/RLS behavior for org-level (NULL project_id) test results, and the Architect history renderer now preserves unnamed records and marks truncation to avoid data-starvation hallucinations. Tests cover both regressions. Ship it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
The Annotations page shows
0–0 of 0while the Test Runs grid shows review counts on nearly every run, and the Architect reports "no annotations" on runs that visibly have them — then invents reviewer names, comment text and turn numbers when it does have annotation data. Two independent bugs, one per symptom.Nothing is listed.
list_annotationsscoped test results with strictproject_id = :project_id. Migrationa1b2c3d4e5f0addedproject_idas a nullable column with no backfill, so every test result predating project scoping carries a NULLproject_idpermanently — and nothing has assigned one since. Everywhere else in the app those rows are visible: the ORM auto-filter (models/scope_events.py) and theproject_isolationRLS policy both applyproject_id = :pid OR project_id IS NULL, which is why the test run page counts their reviews viareviewed_tests. Only this query disagreed, so the annotations list came back empty for the entire historical dataset. This is not an edge case — for an org whose reviewed runs all predate the migration, the feature has never returned anything.trace.project_idisNOT NULL, so the trace branch needs no equivalent allowance.The content is invented. Every list-shaped tool result passes through
_compact_list_result_for_historybefore reaching the prompt. That renderer builds each line fromname/title,idanddescription. An annotation has none of them — the human's words live incomments, the key isreview_id, and the inheritedidis always null. A real production payload:{"id": null, "nano_id": null, "review_id": "e87b6bc0-…", "comments": "Test review", "status": {"name": "Pass"}, "user": {"name": "Nicolai Bohn"}, "behavior_name": "Off-Domain Request Redirect"}rendered to the LLM as
List response: 1 item(s)followed by a single line reading- ?. Told that reviews exist but shown none of their content, the model fills the gap from imagination. This is data starvation, not creativity: restore the payload and there is nothing left to guess at.Both were confirmed against production. Worker logs for the reported session show
list_annotationscalled five times, all succeeding, none erroring — ruling out a 400 (missing project scope), a 422 (URL passed instead of a UUID) and any permission problem. The model then fell back tolist_test_results→get_test_result, which returnstest_reviewsinline; that is where the real review text it quoted came from. A freshly created review on a project-stamped test result comes back from the endpoint correctly, isolating NULLproject_idas the discriminator.What Changed
services/annotations.py: the test-result branch now admitstr.project_id IS NULL, matching the ORM auto-filter and the RLS policy. Org isolation is untouched, and the trace branch is deliberately left strict.architect/agent.py: items without aname/titlerender as their own trimmed JSON — empty values dropped, long strings clipped, non-ASCII left readable — instead of collapsing to- ?. Named entities keep the existing compact one-line form, solist_metricsand friends are unaffected. This also fixeslist_test_results, which has nonameeither and was collapsing to- ? (id: …).architect/agent.py: tool results cut at the 4000-char preview now say so. An unmarked cut reads as a complete record, and a largeget_test_resulteasily exceeds it.Additional Context
test_result.test_reviews(test-run-summary-utils.ts), and the runs grid usescounts.reviewed_testsfromresult_processor.py. Both go through the ORM, which is why the UI and the API disagreed with no error anywhere.tests/backend/routes/test_annotations.pycreated test results with an explicitproject_id, which is exactly why the NULL case slipped through.test_run → test_configuration → endpoint.project_id). TheOR IS NULLpredicate is correct on its own merits — it is what the ORM and RLS already do — but a backfill would additionally stop those rows appearing in every project's view. That is a separate change with its own migration and risk.BaseAgent._format_historyin the SDK has the same unmarked 4000-char truncation. Left alone to keep this focused on the architect path; worth a follow-up for the agents using the base implementation.Testing
test_pre_project_scoping_rows_are_visibleseeds a run with no ambient project scope, so auto-stamp leavesproject_idNULL exactly as pre-migration rows are, then asserts the review comes back under an active project. Reverting the one-line SQL change makes it fail withassert set() == {'e8c33efb-…'}— the endpoint returning nothing, reproducing the production symptom exactly.TestCompactUnnamedListResultsasserts that annotation comments, reviewer, verdict and behavior all survive the renderer and that no line collapses to- ?, with an end-to-end case through_format_history().TestToolResultTruncationMarkercovers the truncation notice.To verify by hand: open the Annotations page on an org whose reviewed runs predate project scoping. It should list reviews the Test Runs grid already counts. Then ask the Architect about one of those runs and confirm it both finds the reviews and quotes them accurately.