Give Content comment AI explicit reply, suggest, and apply intents - #4546
Give Content comment AI explicit reply, suggest, and apply intents#45463mdistal wants to merge 9 commits into
Conversation
Visual recap — generation failedThe visual recap could not be generated for this pull request. This is informational only and does not block the PR. Diagnostic: No plan URL: create-visual-recap failed 403 Forbidden: {"error":"Creating an org-visible visual recap requires an active organization. Connect Plan from an organization or publish with private visibility."} Agent output: Created and validated recap-source.json for PR #4546. It covers 65 files, three UI states, the request model, and six key implementation tabs. --- ⠀ 🟢 Recap source is ready for CI’s deterministic publisher. stderr: Reading additional input from stdin... 2026-09-08T22:56:16.238971Z ERROR codex_core::tools::router: error=apply_patch verification failed: invalid patch: multiple operations target /home/runner/work/agent-native/agent-native/recap-source.json |
![builder-io-integration[bot]](https://avatars.githubusercontent.com/in/353721?s=60&v=4){kind=small}
There was a problem hiding this comment.
Builder reviewed your changes and found 4 potential issues 🟡
Review Details
Code Review Summary
PR #4546 adds a durable, explicitly scoped Content comment-AI workflow for replying in-thread, creating anchored suggestions, and applying edits while resolving feedback. The overall architecture is sound: intents are persisted, retries and continuation carry request scope, suggestions are reviewable before acceptance, and the implementation reuses the canonical document edit/suggestion primitives. The added tests cover substantial replay, access, transaction, and UI behavior.
Risk assessment: Standard risk, elevated by comment mutation, document editing, access checks, and long-running/retried AI execution.
Key Findings
- 🟡 MEDIUM — Several authorization and source-thread checks occur before the final write transaction, allowing revocation or new feedback to race an AI result.
- 🟡 MEDIUM — Rejected start requests can leave resolved/stale-thread failures without visible UI feedback.
- 🟢 The explicit intent model and durable request records provide a good foundation for safe retries and clear reviewer control.
🧪 Browser testing: Will run after this review (PR touches UI code) — the initial full run was blocked because Chrome automation tools were unavailable, although the dev server was healthy; all 15 planned cases were consequently unverified.
| const access = await assertAccess("document", documentId, "commenter"); | ||
| const ownerEmail = access.resource.ownerEmail as string; | ||
| const db = getDb(); | ||
| const access = await assertAccess("document", documentId, "commenter"); |
There was a problem hiding this comment.
🟡 Recheck comment permission inside the write transaction
assertAccess is evaluated before opening the transaction, while the transaction only locks/validates the document and reply parent. If the caller's share is downgraded or revoked while the transaction waits on the document lock, the insert can still commit for a caller who no longer has comment permission. Re-resolve commenter access against the active transaction immediately before inserting.
Additional Info
Reported independently by 2 of 4 review agents.
| const request = await requireCommentAiRequest("apply-resolve"); | ||
| if (request.status === "resolved") | ||
| return serializeCommentAiRequest(request); | ||
| let result = serializeCommentAiRequest(request).result ?? {}; | ||
| try { | ||
| await assertCommentAiSourceUnchanged(request); | ||
| const payload = payloadSchema.parse( | ||
| await retainCommentAiPayload(request, args), | ||
| ); | ||
| const edit = (await editDocument.run( |
There was a problem hiding this comment.
🟡 Recheck editor access at the document mutation boundary
The request-level editor check and the check inside editDocument both occur before the document mutation transaction. A collaborator can lose editor access while the AI run is in progress, then still commit the canonical edit and resolve the thread using the earlier authorization result. Add a transaction-scoped editor authorization check at the document write boundary.
Additional Info
Reported by 1 of 4 review agents; confirmed against the long-running apply-and-resolve flow.
| }); | ||
| } | ||
| const payload = payloadSchema.parse(retained); | ||
| const suggestion = await createSuggestion.run( |
There was a problem hiding this comment.
🟡 Recheck the source thread before committing a suggestion
assertCommentAiSourceUnchanged() runs before the proposal is retained, but the later createSuggestion.run() transaction validates only the Page/suggestion base and does not recheck the bound commentThreadDigest. A human reply or edit can land between these steps, allowing a suggestion and receipt to be committed from stale feedback. Revalidate and lock the bound thread at commit time, transitioning the request to needs-review when it changed.
Additional Info
Reported by 1 of 4 review agents; the reply and apply flows already perform a later consistency check.
| }; | ||
| if (started.dispatch) sendToAgentChat(message); | ||
| await query.refetch(); | ||
| } finally { |
There was a problem hiding this comment.
🟡 Surface failed AI request starts to the user
start() awaits callAction() but only has a finally, and the menu handler invokes it with void onStart(...). Server rejections such as a resolved comment or stale thread therefore reject without a user-facing error state or toast; the starting indicator simply clears. Catch the action error and show the existing localized error feedback so reviewers know why no request started.
Additional Info
Identified during browser-test planning and confirmed by source inspection; browser execution was unavailable.
2 participants
Problem
Content reviewers need to choose what AI may do with their feedback. The previous Ask AI entry point could blur answering, proposing edits, and changing the page. Results could also lose the original conversation or display the wrong author, making it difficult to tell what changed and who did it.
Approach
Ask AI now offers Suggest changes, Reply in thread, and Apply changes and resolve. Opening the menu performs no work. Each request binds the selected intent to the page, body field, original conversation, requester, and submitted revision. The server limits the run to the corresponding operation, so a reply request cannot become an edit merely because the model chooses a different tool.
Suggestions use native anchored proposals with Accept/Reject controls and links back to the original feedback. The page remains unchanged until acceptance, and acceptance alone does not resolve the source comment. Apply verifies saved content and unchanged feedback before resolving. Ambiguous, stale, or unfinished work stays open for review.
What changed
Dependency and merge boundary
Open for review; do not merge this PR into its temporary base. This is a stacked PR against
codex/content-review-integrationat2863bbaa720ffc7d3bfd4375b5735e00024da76c, so the displayed diff isolates this feature and its integration repairs.The base combines main
c05091274d04e9704f7380c6cf9af64cf71a714c, #4274 at2e7c9c50a2c32c395272c28f569d3b2345abf22e, and #4376 at0762e2a4ac5ac69bf9b50c98e70dad06f2413578. Both dependencies remain open. #4376 has since advanced to295c9a296282675f30db45a7fb59f6eceea8d0b2, including comment operation/draft ownership and draft settlement after sidebar remounts; those changes are not incorporated here. A prospective merge check against this head reports conflicts inDocumentEditor.tsxandcontent-action-refresh.ts. #4376 remains open and GitHub reports APPROVED (Steve approved the earlier pinned head), with checks queued. #4274 remains open, requires review, conflicts with main, and its last lint/format check failed ontemplates/content/shared/feature-flags.ts. Reconcile against the actual merged dependencies and rerun affected acceptance before mainline integration. No deployment is claimed.Verification
09ba9e48d7488f62df1e68c602a5a79046852186: core build, Content TypeScript, 5 Markdown tests, 11 comment-flow tests, and all 71 repository guards passed.Known limitations: stopping before the first tool can retain working status until the existing 60-second recovery grace expires. The model occasionally reused an older answer despite current page context; a fresh explicit clarification produced the correct answer. Target/permission enforcement passed, but model answer accuracy and animation smoothness are not guaranteed. Initial QA reporting recorded latency globally rather than as a contemporaneous verdict for each step. The main CI workflow is filtered to PRs targeting
main, so it does not run against this temporary stacked base. The local checks above are the technical verification currently available; GitGuardian passed, while the review agent and visual recap checks remain in progress/queued. Mainline CI and affected real-interface retests are required after dependency reconciliation and retargeting.Review focus
Check that continued and retried runs cannot broaden the selected intent; that partial writes never become false completion; and that source-thread identity, revision checks, and proposal replay remain correct when combined with the final dependency changes.