Repository navigation
fix(agent-sessions): record submit_diagnosis's arguments on its tool span - #962
Merged
Merged
Conversation
…span `submit_diagnosis` was the one tool registered straight through `toolkit.toLayer`, so it never picked up `withToolCallContent` and the engine's `execute_tool` span — content-free by design — carried no arguments, no result and no session identity. In prd over the last 48h every other tool annotated 100% of its calls; submit_diagnosis annotated 0 of 67, and its arguments ARE the diagnosis report, so Agent Sessions rendered the whole verdict as an empty tool call. Registration is now a whole-map operation: `toolHandlersWithContent` wraps every handler in a toolkit and returns both the map and its layer, so there is no unwrapped map left to register, and `maple/no-raw-tool-layer` keeps `toLayer` out of every other module. The diagnosis toolkit also takes the run's session attributes, like the Maple toolkit already did.
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (7)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
JeremyFunk
added a commit
that referenced
this pull request
Sep 21, 2026
Two conflicts in the `submit_diagnosis` completion, both resolved by keeping main's new behaviour and this branch's origin. `buildDiagnosisCompletion` now registers its handler through `toolHandlersWithContent` with the run's session attributes (#962), so the report and the result land on the tool span — and it still takes the turn's origin, refuses a connector by it, and reports `autonomous` from it rather than from a user id. `tools.test.ts` keeps both tests: the span assertion, now driven by an autonomous origin instead of the internal-service tenant, and the connector refusal. #960's triage gate starts investigations through `startInvestigationTurn`, which already states `origin: { kind: "autonomous" }`, so it needs nothing.
Makisuo
added a commit
that referenced
this pull request
Sep 22, 2026
… cost by 80% Every review now writes one summary comment on the pull request and edits it in place on later pushes, found again by a hidden `<!-- maple-pr-review -->` line and only ever edited when the App wrote it. It carries a score out of 100, a grade, the verdict, a counts table, the summary, findings linked to their lines at the reviewed commit, what to change, and what was reviewed. The check run shares the renderer and leads its title with the score; the inline review now only carries the notes and points at the comment. The score is computed, not asked of the model: 100 minus 25 per critical finding, 10 per warning and 2 per note, stated in the comment footer and stored in pr_reviews.score (with comment_url beside it; the unreleased migration is regenerated to include both). The verdict is `gaps` exactly when a warning or critical finding is kept, which fixes a check titled "0 observability gaps to close". Iterating with the local runner cut a review of #962 from 388k input tokens and 25 calls to 48k and 9, and #977 from 374k and 23 to 52k and 5, with the same verdicts. pr_changed_files states a call budget sized to the reviewable files, the prompt reads the repository's conventions before the diffs and only verifies what a finding depends on, it forbids speculative findings and ending a message on a promise, and tooling files are not reviewed. A local gap fixture with four planted gaps came back with all four, on the right lines, in 5 calls. The runner gains --range base..head for reviewing a local branch, --post for writing the summary comment with your own gh login, and runs sandbox_exec in a worktree at the head commit as production does.
Makisuo
added a commit
that referenced
this pull request
Sep 23, 2026
* feat(pr-review): review pull requests for observability gaps A repository can opt in to having Maple review every pull request for whether the code it adds will be visible in traces, logs and metrics. The pull_request webhook now carries the head and base commits, and the sink fans out to two isolated readers: the existing fix-verification link and the new review trigger. PrReviewService writes a pr_reviews row per head SHA, skips drafts, bots and duplicates, enforces a daily ceiling per org, and starts one turn of the pr-review agent on a chat session named after the review, the way an investigation's pass is started. The agent runs under an allowlist of diff, source, sandbox and read-only telemetry tools, reads the diff through two new internal tools backed by the provider's pull request files endpoint (with new-side line numbers), and files its report through submit_review, a lenient completion tool normalized in the handler. The report is stored first, then posted as a neutral or successful check run plus a comment-only review with inline comments; a refused post is recorded on the row. Settings get a per-repository switch and PUT /api/integrations/github/repositories/:id/pr-review. The GitHub App needs Checks and Pull requests write permissions, documented in docs/github-app-setup.md. Migration 20260921100708_pr_reviews adds the table and vcs_repositories.pr_review_enabled; production applies it by hand. * fix(pr-review): retry failed rows, drop stale submissions, reconcile the verdict Review feedback on the first cut. A redelivery for a head whose row is `failed` reclaims the row (a conditional update from `failed` to `queued`) and starts it again, instead of reporting a duplicate that could never retry. `submitReview` and `failReview` now move a row only from `queued` or `running`, so a review superseded while its completion call was in flight stays `skipped` and never posts stale findings. A retained warn or critical finding forces the `gaps` verdict whatever the model called it, so the check run's conclusion and its inline comments cannot disagree. The check summary escapes backslashes before pipes and is cut under GitHub's 65,535-character limit. Also: the switch gets an accessible name on narrow screens, the anticipated error identifiers include the review not-found error, the dispatcher test lists the two new internal tools, and both docs match what shipped. * fix(pr-review): purge pr_reviews with the organization The org-scoped table registry pins every table carrying an org column; pr_reviews belongs with the VCS tables that are deleted by org_id. * fix(pr-review): clamp the check summary by UTF-8 bytes GitHub's 65,535 limit on output.summary is bytes, not characters, so a report in a multi-byte script could pass the character count and be refused, losing the review post with it. The clamp now budgets encoded bytes, notice included, and cuts on a code point boundary. * fix(pr-review): keep the review switch's label visible at every width The visible label was hidden below the sm breakpoint, leaving sighted users an unlabeled switch; the aria-label only covered assistive tech. * feat(pr-review): run the reviewer locally against any pull request `bun run --cwd apps/ai review:local <owner/repo> <number>` drives the real pr-review agent (prompt, budget, allowlist, engine loop, model, submit_review and buildPublication) against a real pull request fetched with the caller's gh login. The diff tools print through the same renderers production uses, now pure functions over the PR's files. The source tools read a local clone at the head commit through git, sandbox_exec is limited to read-only git, and the telemetry tools say no warehouse is attached. Nothing is posted; each run writes review.md (the check run and comments as GitHub would render them, with findings off the diff flagged), transcript.md and report.json. --prompt-file swaps the system prompt for one run through a new promptOverride on the run input, and --model picks the OpenRouter model. The first real run found a production bug: sandbox_grep ran git grep in basic-regex mode, so a model's `a|b` matched a literal bar and `\(` failed outright. The reviewer concluded the web app had no telemetry. It now uses extended regex, pinned by a test against a real repository. * fix(pr-review): match the local clone's remote without building a regex from arguments The owner and repository came from the command line and were interpolated into a RegExp. The remote URL is now parsed with a fixed pattern and its parts compared as strings. * feat(pr-review): always post a scored summary comment, and cut review cost by 80% Every review now writes one summary comment on the pull request and edits it in place on later pushes, found again by a hidden `<!-- maple-pr-review -->` line and only ever edited when the App wrote it. It carries a score out of 100, a grade, the verdict, a counts table, the summary, findings linked to their lines at the reviewed commit, what to change, and what was reviewed. The check run shares the renderer and leads its title with the score; the inline review now only carries the notes and points at the comment. The score is computed, not asked of the model: 100 minus 25 per critical finding, 10 per warning and 2 per note, stated in the comment footer and stored in pr_reviews.score (with comment_url beside it; the unreleased migration is regenerated to include both). The verdict is `gaps` exactly when a warning or critical finding is kept, which fixes a check titled "0 observability gaps to close". Iterating with the local runner cut a review of #962 from 388k input tokens and 25 calls to 48k and 9, and #977 from 374k and 23 to 52k and 5, with the same verdicts. pr_changed_files states a call budget sized to the reviewable files, the prompt reads the repository's conventions before the diffs and only verifies what a finding depends on, it forbids speculative findings and ending a message on a promise, and tooling files are not reviewed. A local gap fixture with four planted gaps came back with all four, on the right lines, in 5 calls. The runner gains --range base..head for reviewing a local branch, --post for writing the summary comment with your own gh login, and runs sandbox_exec in a worktree at the head commit as production does. * fix(pr-review): read diffs in batches so a large pull request fits its budget A 33-file review read one pr_file_diff per call; every call re-sends the conversation, so it reached 724k input tokens, ran out of budget and was killed for asking for another diff. pr_file_diff now takes several paths per call, up to 60k characters, and names what did not fit. The same review now takes 11 calls and 402k tokens and completes. The local runner also runs the close-out after a failed pass, as the turn runner does in production, unless the provider itself failed. * feat(pr-review): stage the rollout per organization behind the prreview flag Merging the feature now turns it on for no one. An organization is flagged in with `prreview: true` in its Clerk public metadata, the same place as the other rollout flags. The flag contract moves from apps/web into @maple/domain so the server decodes the same metadata the web app does, and a new OrganizationFeatureFlagsService reads it from Clerk (every rollout on for self-hosted builds, off when Clerk cannot be reached). The switch in Integrations → GitHub appears only for a flagged organization, the settings endpoint refuses to turn reviews on for an unflagged one, and the review trigger skips its pull requests as not_rolled_out, so withdrawing the flag stops reviews on repositories already switched on. * fix(pr-review): post the review even when the installation cannot write check runs The production App registers checks: read, so every installation today answers 403 to a check run, and the check run was created first: the whole publication failed and nothing reached the pull request. A 403 on the check run is now recorded on the span and the summary comment and inline notes are posted anyway, since they need only pull_requests: write. * docs(github-app): the Checks permission is optional for reviews * refactor(pr-review): cleanup pass from review - Trigger: the daily ceiling counts every row started today; state transitions are guarded by the statuses they leave, so a superseded row cannot be flipped back to running or failed. - submitReview resolves the repository and installation before marking the row completed, so a missing one lands as publish_error in the same update. - Publishing: a rate-limited 403 no longer reads as a missing checks permission; a 422 on the inline review drops the notes instead of re-posting an empty review under the summary comment. - Findings need a maple-audit check id; an unknown id is dropped rather than defaulted to SPAN-02. A gap is never graded excellent. - submit_review is offered to the unattended pass only; a follow-up in the review session answers in prose. - Review turns bill as source "review" with their own idempotency key. - The close-out transcript is shared (chat/close-out.ts) and the local runner uses it; the runner's prompt override is an agent override on ChatRunInput, its telemetry stubs derive from PR_REVIEW_TOOLS, and its sandbox_exec refuses git flags that reach outside the object store. - OrganizationFeatureFlagsService caches an org's flags for 60s. - Dead PullRequestEventSinkLive export and unused tool param removed; prReviewSessionId lives in @maple/domain/chat-session. - docs/pr-review-agent-plan.md rewritten as a present-tense design doc. * feat(vcs): give the pull request review publish its own span The check run, summary comment and inline review posts ran under the caller's span, so the skip and rejection annotations landed there. The review's own summary flagged it as MAP-02.
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
1 participant
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.
What
submit_diagnosisrenders in Agent Sessions as a tool call with no arguments and no result. It is the only tool that does.Measured in prd over 2026-09-20 → 21, every
execute_toolspan:gen_ai.tool.call.arguments.call.resultWhy
effect-agent's
execute_toolspan is content-free by design; Maple adds the content inwithToolCallContent.buildMapleToolkitapplied it to every catalogue handler, butbuildDiagnosisCompletionregistered its handler straight throughtoolkit.toLayerand so never got it — andsubmit_diagnosis's arguments are the diagnosis report, so the one call whose payload matters most was the one that got dropped. It was also missing the session attributes every other tool span carries.How
toolHandlersWithContent(toolkit, handlers, sessionAttributes?)wraps every handler in a toolkit and returns both the wrapped map and its layer, so there is no unwrapped map left for a caller to register. Descriptions come off the toolkit, so the span records what the model was actually given.withToolCallContentno longer constrains the failure type, since it now sees any handler's error (a bareAiErrorincluded).maple/no-raw-tool-layer(local oxlint plugin,error) bans.toLayereverywhere but the one line ingenai-spans.ts— that is the "no other tool can run into this" half.buildDiagnosisCompletionnow takes the run's session attributes, wired fromrun.ts.Testing
apps/ai—src/chat,src/platform,llm-tools: 187 passed, including a new regression test that runs the diagnosis handler under anexecute_tool submit_diagnosisspan and asserts the report, the result and the session id land on it.tsc --noEmitclean forapps/ai;oxlintclean acrossapps packages lib scripts.Existing spans are unaffected — only calls after deploy carry the content.
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by CodeRabbit