feat(annotate): preserve notes on structured approval - #1092
Merged
backnotprop merged 12 commits intoJul 27, 2026
Conversation
This was referenced Jul 20, 2026
rNoz
marked this pull request as ready for review
July 20, 2026 13:27
Adopt the grep convention for the strict annotate gate's exit codes: 0 = approved, 1 = negative human outcome (annotated/dismissed under --require-approval), 2 = the gate itself was misconfigured or could not start/deliver a decision. Previously all usage/startup/validation failures shared exit 1 with "reviewer did not approve", so callers could not tell a denied review from a broken gate. - parseStrictAnnotateOptions failures (bad flag combos, strict flags outside annotate --gate --json) now exit 2 - --result-file preflight failures (missing parent, pre-existing or dangling-symlink destination) now exit 2 - post-decision publication failures (destination raced into existence, hard links unavailable, stdout write failure) now exit 2: they deliver no decision record at all, so the code's own fail-closed handling presents them as environment errors, never as a reviewer outcome -- and never approval, since only 0 means approved - decision outcomes keep 0/1 exactly as before; signal deaths keep 128+n - document the contract in AGENTS.md and the annotate-gates guide Claude-Session: https://claude.ai/code/session_01YXkgsNucxDwAL4GdR4XYRk
rNoz
force-pushed
the
rnoz/feat-annotate-approve-with-notes
branch
from
July 24, 2026 07:26
abaa847 to
c57ebea
Compare
The six startup-failure sites in the annotate path (missing path, unreachable URL, empty folder, ambiguous name, missing/unsupported file, oversized file) run after flag parsing and exited 1. Under --require-approval / --result-file, 1 is the "reviewer requested changes" signal, so a typo'd path made automation misclassify a configuration error as a legitimate rejection. Route those sites through exitAnnotateStartupFailure(), which picks its code from the already-parsed strict options via the new pure helper annotateStartupFailureExitCode(). Non-strict invocations still exit 1 with byte-identical stderr; strict invocations exit STRICT_GATE_ERROR_EXIT_CODE (2). Claude-Session: https://claude.ai/code/session_01H5KQWqXqjrPxyxUNso1QHS
writeResultFile ran before the decision JSON reached stdout. On a filesystem without hard links (exFAT, FAT32, most SMB/NFS, some container bind mounts) publication fails deterministically, the catch exited 2 with nothing written anywhere — and the reviewer's autosaved draft had already been deleted by the feedback flow, so their completed decision was lost. Emit the stdout record first, then publish the result file. Exit semantics are unchanged: a publication failure still exits 2, but the decision has reached stdout by then. Only a stdout write failure now leaves no record at all. Correct the docs and comments that claimed exit 2 delivers no decision record: it means the result *file* was not published. Also document the two publication caveats: the 0600 mode is a no-op on Windows, and the atomic link/rename is not followed by a parent-directory fsync, so publication is atomic but not crash-durable. Claude-Session: https://claude.ai/code/session_01H5KQWqXqjrPxyxUNso1QHS
# Conflicts: # packages/server/annotate.test.ts
…e on export
buildCompleteAnnotateFeedback re-parsed each linked document with
parseMarkdownToBlocks(entry.markdown) — no options, so frontmatter
stripping defaulted on. The render side parses with
{ frontmatter: shouldStripFrontmatter(path) }.
For plain-text linked docs (.yaml/.json/.toml/…) a leading `---` is real
content, not frontmatter: a multi-document YAML opens with it. Stripping
it on the export side shifted every block id, so ordinary Send Feedback
and deny emitted wrong `(line N)` labels — or dropped them entirely when
the annotation's block no longer existed.
Pass the same shouldStripFrontmatter(filepath) option at the export call
site so both sides agree.
Claude-Session: https://claude.ai/code/session_01H5KQWqXqjrPxyxUNso1QHS
/api/feedback forwards selectedMessageId and feedbackScope; /api/approve dropped them. Pi resolves the anchor message from those fields, so notes delivered on the approve path anchored to the last message instead of the one the reviewer picked in a multi-message annotate-last session — while Send Feedback in the same session anchored correctly. Forward both fields on the approve path in the Bun and Pi servers, and have the client build the approval body with the same scope resolution Send Feedback uses (extracted as getFeedbackMessageScope so the two can no longer drift). Claude-Session: https://claude.ai/code/session_01H5KQWqXqjrPxyxUNso1QHS
The skill and slash-command files still described `"decision": "approved"` as "acknowledge and stop", with no mention of the feedback field the gate can now attach — so an agent reading them would silently drop the reviewer's approval notes. Update the Claude core/claude skills, the Copilot commands, the Gemini annotate command, and the annotate command reference so the approved branch names the optional feedback field and says what to do with it: carry it into subsequent work, do not treat it as a change request. Claude-Session: https://claude.ai/code/session_01H5KQWqXqjrPxyxUNso1QHS
The default annotate.approvedWithNotes template is
`{{contextBlock}}{{feedback}}`, not `{{context}}` on its own line, and
{{contextBlock}} was missing from the variable table entirely.
Show the actual default, add {{contextBlock}} to the variable table, and
explain why the default prefers it: it collapses to nothing for message
annotations instead of leaving a stray blank line.
Claude-Session: https://claude.ai/code/session_01H5KQWqXqjrPxyxUNso1QHS
Owner
|
All three of your PRs are merged: #1089, #1091, and this one. Thank you for the stack and for dogfooding it daily, that showed in the quality. During final review we pushed a few small fix commits onto your branches: Linux watcher hardening on #1089, strict exit-code and publication-order corrections on #1091, and frontmatter parity plus approve note anchoring here. Everything ships in the next release. |
backnotprop
added a commit
that referenced
this pull request
Jul 27, 2026
The #1092 approval-notes work added annotate-outcome.ts as a top-level import of index.ts but the package.json files array was never updated, so the published tarball failed to load under Pi's jiti loader for every npm-installed user. Found by the pre-release QA sweep; verified the packed tarball now contains the module and no other top-level import is missing from the files array. Claude-Session: https://claude.ai/code/session_01H5KQWqXqjrPxyxUNso1QHS
Souptik96
added a commit
to Souptik96/plannotator
that referenced
this pull request
Jul 28, 2026
…ng it The Approve with Notes flow (backnotprop#1092) emits decision:"approved" with a feedback field, but both adapters collapsed every approved decision to a bare "Approved." -- the reviewer's notes were silently lost. Amp had two drop sites: formatAnnotationFeedback returned null for anything not "annotated", and handleAnnotateResult early-returned on approved before formatting was ever reached. Droid's emitAnnotateDecision printed only "Approved." Both adapters now emit the shared approved-with-notes prompt (mirroring DEFAULT_ANNOTATE_APPROVED_WITH_NOTES_PROMPT in packages/shared/prompts.ts); note-less approvals keep the old behavior. The amp config override key approvedWithNotes matches the shared prompt config shape. Fixes backnotprop#1137
backnotprop
pushed a commit
that referenced
this pull request
Jul 30, 2026
…ng it (#1146) The Approve with Notes flow (#1092) emits decision:"approved" with a feedback field, but both adapters collapsed every approved decision to a bare "Approved." -- the reviewer's notes were silently lost. Amp had two drop sites: formatAnnotationFeedback returned null for anything not "annotated", and handleAnnotateResult early-returned on approved before formatting was ever reached. Droid's emitAnnotateDecision printed only "Approved." Both adapters now emit the shared approved-with-notes prompt (mirroring DEFAULT_ANNOTATE_APPROVED_WITH_NOTES_PROMPT in packages/shared/prompts.ts); note-less approvals keep the old behavior. The amp config override key approvedWithNotes matches the shared prompt config shape. Fixes #1137
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.
Built on top of #1091
Summary
Annotate gates now preserve complete reviewer feedback when their outcome transport can deliver notes with approval (saving another unnecessary turn with the Agent).
Bun and Pi advertise an explicit
approvalNotesSupportedcapability. Direct gated JSON, active OpenCode sessions, and Pi follow-up delivery opt in. When capable feedback exists, the UI presents Approve with Notes and confirms that the artifact will be approved, the notes are non-blocking guidance rather than a revision request, and the gate will close.Plaintext and hook-native output remain unchanged. Unsupported approval paths retain the existing warning and Send Feedback action. ;)
Fixes #930
Why
Annotate approval previously posted no body and resolved an empty approved decision. A reviewer who left small non-blocking notes had to choose between silently losing them and using Send Feedback, which means revise and reopen.
Structured transports can carry an approved decision and feedback together. Making that capability explicit lets the editor preserve notes without changing transports whose approval protocol has no feedback channel.
Changes
approvalNotesSupportedto Bun and Pi annotate server options and/api/planpayloads with a default offalse.--gate --jsonwithout--hook, OpenCode embedded sessions with a session ID, gated OpenCode CLI bridge launches with a session ID, and Pi annotation sessions./api/approve, preserve feedback and annotations, keep bodyless approval compatible, and reject malformed or wrong-type bodies with400without resolving.packages/editor/annotateSubmission.tsto compose complete feedback and build capability-aware approval requests.annotate.approvedWithNotesprompt while preserving the existing file or message context.apps/hook/server/annotate-output.ts, preserve plaintext and hook output byte-for-byte, and include nonempty feedback only in direct JSON approval.--hookremains unchanged.Validation
Focused rebased proof
bun run typecheck: passed.bun run build:hook: passed; Vite transformed 5,582 modules.bun run build:opencode: passed; bundled 690 embedded modules and 90 Node modules.bun run build:pi: passed; the build rebuilt the review app with 3,511 modules and the hook app with 5,582 modules.git diff --check: passed.In the integration branch combining the main PRs in-flight:
Live proof (ad hoc tests for this branch)
However, I have been using it for days as part of the integration branch, in real sessions (multi-cli harness, multi-project).
Some fake note, exited0, and its authoritative result was consumed.Global note!. Retainedvalidation/live/approval-ui-v2.stdout.logandvalidation/live/approval-ui-v2.result.jsoncontain the same JSON line. No final-UI process exit code was captured; automated tests pin the confirmation copy and prove approved exit0plus exact stdout/result bytes.These runs validate the direct structured transport and rebuilt UI. They do not validate native OpenCode or Pi delivery.
Example of usage E2E with the integration branch:
Screen.Recording.2026-07-20.at.15.44.17.mov
Manual testing gaps
Compatibility
The server capability defaults to
false, and bodyless/api/approveremains valid. Existing callers that do not opt in retain their current approval behavior.Direct plaintext approval still emits
The user approved.. Hook approval and close still emit empty stdout, while hook feedback still emits the existing blocking JSON.--hookis unchanged because its native approval path has no feedback channel.Send Feedback continues to mean revise and reopen. Approval notes are used only when the transport can represent "approved, with non-blocking guidance." The default approval-note prompt tells the receiving agent not to revise or reopen solely because notes were supplied unless the user explicitly requests it.
Prior art and overlap
GitHub overlap was refreshed on 2026-07-20. Open issue #930, “Approve with annotations,” directly requests this annotate approval-notes behavior. This PR fixes #930; no duplicate issue should be opened.
Issue #570 established annotate gate decisions, but approved results still lack notes. Closed issue #30 requested "Approve with comments"; the maintainer explicitly requested confirmation and shipped OpenCode plan support in 0.4.8.
#30 is plan-mode prior art, while #570 established gate decisions. This contribution implements #930 and provides annotate-gate transport parity for direct structured output, OpenCode, and Pi. It does not add Claude hook support.