From b7a6aa8d0b4e1e969b3ba890a3372725351be50d Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 11 Sep 2026 09:24:31 +0000 Subject: [PATCH 1/3] =?UTF-8?q?=F0=9F=A4=96=20docs:=20bound=20the=20PR=20r?= =?UTF-8?q?eview=20loop=20by=20scope,=20not=20round=20count?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add early-stop cases for non-converging review loops and a scope classification for review fixes to the PR Workflow section of AGENTS.md. --- _Generated with `xum` • Model: `anthropic:claude-fable-5-1` • Thinking: `high` • Cost: `$940.64`_ --- docs/AGENTS.md | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/docs/AGENTS.md b/docs/AGENTS.md index d613c66071..90ad1f7f64 100644 --- a/docs/AGENTS.md +++ b/docs/AGENTS.md @@ -79,7 +79,7 @@ Core workflow: > PR readiness is mandatory. You MUST keep iterating until the PR is fully ready. > A PR is fully ready only when: (1) Codex confirms approval (thumbs-up reaction on the PR description or an approval comment like "Didn't find any major issues"), (2) all Codex review threads are resolved, and (3) all required CI checks pass. -> You MUST NOT report success or stop the loop before these conditions are met. +> You MUST NOT report success or stop the loop before these conditions are met, except in the early-stop cases below. When a PR exists, you MUST remain in this loop until the PR is fully ready: @@ -90,7 +90,22 @@ When a PR exists, you MUST remain in this loop until the PR is fully ready: 5. If Codex leaves comments, address them, resolve threads with `./scripts/resolve_pr_comment.sh `, push, and repeat. 6. If checks/mergeability fail, fix issues locally, push, and repeat. -The only early-stop exception is when the reviewer is clearly misunderstanding the intended change and further churn would be counterproductive. In that case, leave a clarifying PR comment and pause for human direction. +Stop the loop early in three cases. In each case, leave a PR comment that states the reason, then pause for human direction: + +1. The reviewer misunderstands the intended change, and more rounds will only add churn. +2. The loop does not converge. Each round must shrink the set of findings. If the same area produces new findings round after round, the design is the problem. Do not add more mechanism. Simplify the design or reduce the scope. +3. The next fix grows the scope (see below). Report the state with a split proposal: what you fixed, what you deferred, and how to split the PR. + +### Review fixes and scope + +The PR must stay the change it started as. Before you fix a finding, compare the fix with the original change and classify it: + +1. If the fix is small and stays inside the behavior and files of the original change, fix it in this PR. +2. If the fix adds more code than the original change, or adds a new module, on-disk artifact, persisted field, or subsystem, do not fix it here. This is scope growth. Reply on the thread with the intended fix and resolve the thread. Deliver the fix as a stacked PR (`gh stack`) or a tracked follow-up. +3. If the finding needs corrupted persisted state, a narrow crash window, or a second racing backend (for example `XUM_ALLOW_MULTIPLE_INSTANCES`), and the PR did not introduce that surface, defer it the same way. +4. If the finding is wrong, or the behavior is intended, reply with the reason and resolve the thread. A reasoned rejection is a valid resolution. + +Deferred does not mean fixed. Every deferred fix must have a follow-up: a stacked PR, an issue, or a note in the PR description. ## Testing: HistoryService From a60cdd0010d757ed6f1b4dbae44e5aeef70d9254 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 11 Sep 2026 09:28:23 +0000 Subject: [PATCH 2/3] =?UTF-8?q?=F0=9F=A4=96=20docs:=20regenerate=20built-i?= =?UTF-8?q?n=20skill=20content=20for=20AGENTS.md=20change?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- _Generated with `xum` • Model: `anthropic:claude-fable-5-1` • Thinking: `high` • Cost: `$940.64`_ --- .../builtInSkillContent.generated.ts | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/src/node/services/agentSkills/builtInSkillContent.generated.ts b/src/node/services/agentSkills/builtInSkillContent.generated.ts index 585ec6ad33..bba03f2c5c 100644 --- a/src/node/services/agentSkills/builtInSkillContent.generated.ts +++ b/src/node/services/agentSkills/builtInSkillContent.generated.ts @@ -1770,7 +1770,7 @@ export const BUILTIN_SKILL_FILES: Record> = { "", "> PR readiness is mandatory. You MUST keep iterating until the PR is fully ready.", '> A PR is fully ready only when: (1) Codex confirms approval (thumbs-up reaction on the PR description or an approval comment like "Didn\'t find any major issues"), (2) all Codex review threads are resolved, and (3) all required CI checks pass.', - "> You MUST NOT report success or stop the loop before these conditions are met.", + "> You MUST NOT report success or stop the loop before these conditions are met, except in the early-stop cases below.", "", "When a PR exists, you MUST remain in this loop until the PR is fully ready:", "", @@ -1781,7 +1781,22 @@ export const BUILTIN_SKILL_FILES: Record> = { "5. If Codex leaves comments, address them, resolve threads with `./scripts/resolve_pr_comment.sh `, push, and repeat.", "6. If checks/mergeability fail, fix issues locally, push, and repeat.", "", - "The only early-stop exception is when the reviewer is clearly misunderstanding the intended change and further churn would be counterproductive. In that case, leave a clarifying PR comment and pause for human direction.", + "Stop the loop early in three cases. In each case, leave a PR comment that states the reason, then pause for human direction:", + "", + "1. The reviewer misunderstands the intended change, and more rounds will only add churn.", + "2. The loop does not converge. Each round must shrink the set of findings. If the same area produces new findings round after round, the design is the problem. Do not add more mechanism. Simplify the design or reduce the scope.", + "3. The next fix grows the scope (see below). Report the state with a split proposal: what you fixed, what you deferred, and how to split the PR.", + "", + "### Review fixes and scope", + "", + "The PR must stay the change it started as. Before you fix a finding, compare the fix with the original change and classify it:", + "", + "1. If the fix is small and stays inside the behavior and files of the original change, fix it in this PR.", + "2. If the fix adds more code than the original change, or adds a new module, on-disk artifact, persisted field, or subsystem, do not fix it here. This is scope growth. Reply on the thread with the intended fix and resolve the thread. Deliver the fix as a stacked PR (`gh stack`) or a tracked follow-up.", + "3. If the finding needs corrupted persisted state, a narrow crash window, or a second racing backend (for example `XUM_ALLOW_MULTIPLE_INSTANCES`), and the PR did not introduce that surface, defer it the same way.", + "4. If the finding is wrong, or the behavior is intended, reply with the reason and resolve the thread. A reasoned rejection is a valid resolution.", + "", + "Deferred does not mean fixed. Every deferred fix must have a follow-up: a stacked PR, an issue, or a note in the PR description.", "", "## Testing: HistoryService", "", From f910f6ae0acb4d97bfb067ed7e969891df26e6ee Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 11 Sep 2026 09:35:27 +0000 Subject: [PATCH 3/3] =?UTF-8?q?=F0=9F=A4=96=20docs:=20address=20Codex=20ro?= =?UTF-8?q?und=20one=20on=20review-loop=20scope=20guidance?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Split the success prohibition from the stop exception, classify fixes by whether this PR introduced the defect instead of by diff size alone, and base deferral on the defect rather than its trigger condition. --- _Generated with `xum` • Model: `anthropic:claude-fable-5-1` • Thinking: `high` • Cost: `$940.64`_ --- docs/AGENTS.md | 11 ++++++----- .../agentSkills/builtInSkillContent.generated.ts | 11 ++++++----- 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/docs/AGENTS.md b/docs/AGENTS.md index 90ad1f7f64..5687467115 100644 --- a/docs/AGENTS.md +++ b/docs/AGENTS.md @@ -79,7 +79,7 @@ Core workflow: > PR readiness is mandatory. You MUST keep iterating until the PR is fully ready. > A PR is fully ready only when: (1) Codex confirms approval (thumbs-up reaction on the PR description or an approval comment like "Didn't find any major issues"), (2) all Codex review threads are resolved, and (3) all required CI checks pass. -> You MUST NOT report success or stop the loop before these conditions are met, except in the early-stop cases below. +> You MUST NOT report success before these conditions are met. You MUST NOT stop the loop before these conditions are met, except in the early-stop cases below. An early stop is never success. Report it as incomplete. When a PR exists, you MUST remain in this loop until the PR is fully ready: @@ -100,10 +100,11 @@ Stop the loop early in three cases. In each case, leave a PR comment that states The PR must stay the change it started as. Before you fix a finding, compare the fix with the original change and classify it: -1. If the fix is small and stays inside the behavior and files of the original change, fix it in this PR. -2. If the fix adds more code than the original change, or adds a new module, on-disk artifact, persisted field, or subsystem, do not fix it here. This is scope growth. Reply on the thread with the intended fix and resolve the thread. Deliver the fix as a stacked PR (`gh stack`) or a tracked follow-up. -3. If the finding needs corrupted persisted state, a narrow crash window, or a second racing backend (for example `XUM_ALLOW_MULTIPLE_INSTANCES`), and the PR did not introduce that surface, defer it the same way. -4. If the finding is wrong, or the behavior is intended, reply with the reason and resolve the thread. A reasoned rejection is a valid resolution. +1. If the fix repairs a defect that this PR introduced, fix it in this PR. Size does not matter here. If that fix needs a new module, on-disk artifact, persisted field, or subsystem, remove the surface from this PR instead of growing it. +2. If the fix is small and stays inside the behavior and files of the original change, fix it in this PR. +3. If the fix adds a new module, on-disk artifact, persisted field, or subsystem, do not fix it here. The same applies if the fix is larger than the original change and does not repair a defect that this PR introduced. This is scope growth. Reply on the thread with the intended fix and resolve the thread. Deliver the fix as a stacked PR (`gh stack`) or a tracked follow-up. +4. If the finding needs corrupted persisted state, a narrow crash window, or a second racing backend (for example `XUM_ALLOW_MULTIPLE_INSTANCES`), classify the defect, not its trigger. If this PR added the failing code or removed a defense, fix it here. If the defect existed before this PR, defer it the same way. +5. If the finding is wrong, or the behavior is intended, reply with the reason and resolve the thread. A reasoned rejection is a valid resolution. Deferred does not mean fixed. Every deferred fix must have a follow-up: a stacked PR, an issue, or a note in the PR description. diff --git a/src/node/services/agentSkills/builtInSkillContent.generated.ts b/src/node/services/agentSkills/builtInSkillContent.generated.ts index bba03f2c5c..ba18e328df 100644 --- a/src/node/services/agentSkills/builtInSkillContent.generated.ts +++ b/src/node/services/agentSkills/builtInSkillContent.generated.ts @@ -1770,7 +1770,7 @@ export const BUILTIN_SKILL_FILES: Record> = { "", "> PR readiness is mandatory. You MUST keep iterating until the PR is fully ready.", '> A PR is fully ready only when: (1) Codex confirms approval (thumbs-up reaction on the PR description or an approval comment like "Didn\'t find any major issues"), (2) all Codex review threads are resolved, and (3) all required CI checks pass.', - "> You MUST NOT report success or stop the loop before these conditions are met, except in the early-stop cases below.", + "> You MUST NOT report success before these conditions are met. You MUST NOT stop the loop before these conditions are met, except in the early-stop cases below. An early stop is never success. Report it as incomplete.", "", "When a PR exists, you MUST remain in this loop until the PR is fully ready:", "", @@ -1791,10 +1791,11 @@ export const BUILTIN_SKILL_FILES: Record> = { "", "The PR must stay the change it started as. Before you fix a finding, compare the fix with the original change and classify it:", "", - "1. If the fix is small and stays inside the behavior and files of the original change, fix it in this PR.", - "2. If the fix adds more code than the original change, or adds a new module, on-disk artifact, persisted field, or subsystem, do not fix it here. This is scope growth. Reply on the thread with the intended fix and resolve the thread. Deliver the fix as a stacked PR (`gh stack`) or a tracked follow-up.", - "3. If the finding needs corrupted persisted state, a narrow crash window, or a second racing backend (for example `XUM_ALLOW_MULTIPLE_INSTANCES`), and the PR did not introduce that surface, defer it the same way.", - "4. If the finding is wrong, or the behavior is intended, reply with the reason and resolve the thread. A reasoned rejection is a valid resolution.", + "1. If the fix repairs a defect that this PR introduced, fix it in this PR. Size does not matter here. If that fix needs a new module, on-disk artifact, persisted field, or subsystem, remove the surface from this PR instead of growing it.", + "2. If the fix is small and stays inside the behavior and files of the original change, fix it in this PR.", + "3. If the fix adds a new module, on-disk artifact, persisted field, or subsystem, do not fix it here. The same applies if the fix is larger than the original change and does not repair a defect that this PR introduced. This is scope growth. Reply on the thread with the intended fix and resolve the thread. Deliver the fix as a stacked PR (`gh stack`) or a tracked follow-up.", + "4. If the finding needs corrupted persisted state, a narrow crash window, or a second racing backend (for example `XUM_ALLOW_MULTIPLE_INSTANCES`), classify the defect, not its trigger. If this PR added the failing code or removed a defense, fix it here. If the defect existed before this PR, defer it the same way.", + "5. If the finding is wrong, or the behavior is intended, reply with the reason and resolve the thread. A reasoned rejection is a valid resolution.", "", "Deferred does not mean fixed. Every deferred fix must have a follow-up: a stacked PR, an issue, or a note in the PR description.", "",