Skip to content

#1304 Gate the review divergence callout on criteria, not timestamps #1305

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
williamthorsen merged 2 commits into from
Aug 13, 2026
Merged

#1304 Gate the review divergence callout on criteria, not timestamps #1305

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4e01025
agents|fix: Gate the divergence callout on criteria, not timestamps
williamthorsen Aug 13, 2026
e66fcc2
agents|fix: Match the callout's retention clause to its render condition
williamthorsen Aug 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/agents/content/skills/review-branch/SKILL.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ This skill is the canonical home of the shared review process. `review-pr` invok
- If `--spec-source=remote|local` is set, use that candidate. If the named candidate is unavailable (e.g. `--spec-source=local` with no snapshot, or `--spec-source=remote` with a failed/offline fetch or null `ticket_id`), stop and report the missing source rather than silently using the other side — an explicit instruction must not be redirected to the wrong contract. State the remedy (drop the flag to re-enable recency) in the message.
- Otherwise use the candidate with the newer `last_updated`. Both values are ISO 8601 at the same precision, so they compare chronologically as plain strings — no per-format parsing. On an exact tie, prefer the remote candidate (canonical for the owned-ticket majority).
- If only one candidate exists, use it. This single-candidate fallback also covers a failed/offline remote fetch and a null `ticket_id`.
- Append the chosen candidate as a `ticket` source carrying its `provenance` and `last_updated`. When both candidates existed, hold the rejected candidate's `last_updated` in-process for the divergence note — it rides working memory, not the `spec_sources` record, because resolution here and rendering in the output are the same `review-branch` invocation (which is also why the record needs only one `last_updated`).
- Append the chosen candidate as a `ticket` source carrying its `provenance` and `last_updated`. When the chosen source is the local snapshot and a remote candidate existed, hold that rejected candidate's `content` in-process for the divergence check. Hold raw content rather than extracted criteria: the comparison runs at render time under the extraction rule the compliance table states, so both candidates are read by one rule. It rides working memory, not the `spec_sources` record, because resolution here and rendering in the output are the same `review-branch` invocation (which is also why the record needs only one `last_updated`). When the chosen source is the remote candidate, retain nothing; the callout cannot fire on that branch.
- **No source available**: Leave the list empty. The "Specification compliance" section is omitted from the output.

`review-pr` may pass additional sources (notably the PR description as `pr_description`). The list is the canonical input for the "Specification compliance" section regardless of who populated it.
Expand Down Expand Up @@ -183,7 +183,7 @@ Score: X/10

**Source:** {Render the source's `provenance` and `last_updated` (echoed verbatim — `last_updated` is already ISO 8601) so the reader knows what contract this section measured against — e.g. `remote issue (last updated 2026-06-07T01:56:34Z)`, ``local snapshot `20260606-090337Z_..._ticket.md` (last updated 2026-06-06T09:03:37Z)``, or `local source (last updated …)` for an explicit file/plain-text `--ticket` that has no snapshot filename. Omit the "last updated" clause when `last_updated` is null.}

{When this ticket source was auto-resolved and a competing candidate existed whose `last_updated` differs, add a divergence callout: ⚠️ name both timestamps, state which side this review measured against, and point to `--spec-source=remote|local` to re-resolve against the other. Omit when there was no competing candidate (single source, explicit `--ticket`, or a `pr_description` source).}
{When the chosen source is the local snapshot and a remote candidate existed, compare their acceptance criteria by the same extraction rule the table below uses. Add a callout only when they differ materially: a criterion on one side with no counterpart on the other (absence on either side counts), or worded so the verdict could differ. Rewording that cannot change a verdict is not material. The callout names the difference with a ⚠️, states that this review measured against the snapshot rather than the published ticket, and points to `--spec-source=remote` to re-resolve against the remote. Say nothing when the criteria agree, whatever the timestamps. Omit entirely when the chosen source is the remote candidate, when there was no competing candidate (single source or explicit `--ticket`), or for a `pr_description` source.}

#### Acceptance criteria

Expand Down