The proposal's prose is wanted, not required - #173
Conversation
summary and an operation's reason are written for the person at the review, and the importer treated them as protocol: a reply that forgot the sentence lost every operation with it, and the user was sent back to the chat — where they got no sentence either, just another round trip. Assistants forget these fields in the field, and the cost of the miss was nowhere near the value of the words. Both are optional now, and their absence is reported instead of punished. The review warns that the reply gave no summary and how many rows say why; the dialog marks the rows themselves, since a blank cell reads like there was nothing to say. A row the parser refused keeps its blank reason — its message IS the explanation — so the mark means what it says. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9f510e83cc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| StyleGrid(); | ||
| labelSummary.Text = review.Proposal.Summary; | ||
| labelSummary.Text = review.Proposal.Summary ?? NoSummaryText; |
There was a problem hiding this comment.
The new missing-summary placeholder and associated warnings are user-visible behavior, but this commit updates only the agent-facing guide and protocol. REFERENCE.md still says every proposed change is shown with the assistant's reason and does not explain that missing summaries/reasons are accepted or how they are marked, so the product's only relevant user manual is now incomplete and misleading.
AGENTS.md reference: AGENTS.md:L143-L147
Useful? React with 👍 / 👎.
| : $"{unexplained} of {proposal.Operations.Count} operations say why they are " + | ||
| "proposed; the rest are marked in the reason column."); |
There was a problem hiding this comment.
When only some operations lack reasons, unexplained is the number without prose, yet the warning claims that this number of operations “say why.” For example, one missing reason among three operations produces “1 of 3 operations say why” even though two do; either subtract this count from the total or say that these operations do not say why.
Useful? React with 👍 / 👎.
| if (!WithinLength(operation.Reason)) | ||
| { | ||
| return "The reason is missing or too long."; | ||
| return "The reason is too long."; |
There was a problem hiding this comment.
For a reply containing "reason": "" or whitespace, WithinLength succeeds and the value remains non-null. The validator therefore emits no missing-reason warning, while the dialog's null-coalescing placeholder is bypassed and displays an empty reason cell—the exact ambiguous presentation this change is intended to eliminate. Normalize whitespace-only reasons to null, as is already done for the summary.
Useful? React with 👍 / 👎.
Hostile pass over my own change, three findings.
The sentence read "{unexplained} of {N} operations say why", printing the
count of the ones that DO NOT. The test could not see it: two operations
with one reason missing make both readings print "1". The fixture now has
three operations and one silence, so the numbers differ and only the true
sentence passes.
A blank "reason": "" slipped through the mark it was meant to earn — the
summary was normalized to null and the reason was not, so a reply that
wrote an empty string looked explained. Both go through one Prose helper
now, and a parser-refused row still carries its own empty reason, which is
what keeps it blank.
The reason cell showed "(no reason given)" while its tooltip showed
nothing.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
REFERENCE described the review as showing every change with the reason the assistant gave, which stopped being true one commit ago. The user manual is the one document a person reads before ticking a box, so it now says the words are optional, why refusing a reply over them would cost them more than it saves, and what stands in their place — down to the row the importer itself refused, whose refusal IS its reason. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Prose collapsed any whitespace-only reason to "missing" before CheckStrings looked at its length, so a reason of 2001 spaces was read as no reason at all — through the one check the protocol states plainly. The summary never had the hole: its length is judged on the wire value, before the collapse. Only a blank the limits already allow is collapsed now. The test sends a reason of MaxStringLength + 1 spaces and expects that operation refused; it passes on the old Prose. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The excess group delay is a menu item and a probe kind, and the guide named both without saying when either wins, which leaves the assistant guessing at a choice the user pays for. The menu is a click and a paste; the probe costs a proposal, a review and an import. So: name the menu path when the curve is all you want, ask for the probe when you are asking for junction readings anyway and it rides along in one document, read off one session. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
Field report from the owner: assistants periodically forget
summaryand anoperation's
reason.Both fields exist for the person reading the review, and the importer was
treating them as protocol:
summary— the whole reply was refused (The proposal has no summary.), so the user went back to the chat for one sentence and everyoperation went with it;
reason— the field wasrequiredfor the deserializer, so the operationwas rejected with
Not the shape the protocol describes: … missing required properties.Refusing gives the user no sentence either. It gives them a round trip.
What changed
Both are optional, and their absence is shown rather than punished — an
empty cell would otherwise read as "there was nothing to say":
The reply gave no summary of what it is proposing.and1 of 2 operations say why they are proposed; the rest are marked in the reason column.(orNo operation says why: judge the 3 rows below by the values alone.);(no reason given),(the reply gave no summary).An operation the parser refused carries an empty reason rather than a missing
one and stays blank — its own message is the explanation — so the mark means
what it says.
AGENT_GUIDE.mdandPROTOCOL.mdnow say "wanted" instead of "required":write them because the user is about to act on them, not because the importer
insists. Length limits still apply to a string that IS present.
Tests
Parse_KeepsAReplyThatLeftTheSummaryOut— the field absent, and present butblank.
Parse_RejectsOneBadOperationAndKeepsTheOthersnow shows the contrast: theoperation with no
reasonis kept withReason == null, the malformedshapes beside it are still refused.
Review_SaysWhenTheReplyLeftItsProseOut,Review_SaysWhenNoOperationSaysWhy.App suite: 2275 passed, 11 skipped.
🤖 Generated with Claude Code