diff --git a/.github/agents/dev-loop.agent.md b/.github/agents/dev-loop.agent.md index f78bf05d..2e917b5d 100644 --- a/.github/agents/dev-loop.agent.md +++ b/.github/agents/dev-loop.agent.md @@ -390,7 +390,8 @@ pwsh -NoProfile -File .github/skills/evidence-capture/helpers/Publish-Evidence.p gh run list --branch --limit 5 ``` -If CI fails, fix and push. Non-trivial fixes -> Phase 3. +If CI fails, fix and push. Non-trivial fixes -> Phase 3. If no runs appear because +hosted CI cannot run at all, see the hosted-CI-unavailable exception in Phase 8. #### Step 5: Obtain an independent review @@ -485,14 +486,16 @@ Append dry run results to PR body using `--body-file`. Construct the complete bo from scratch -- never read-modify-write. See `copilot-instructions.md` > PR & Issue Body Formatting. -**Exit criteria:** PR created, CI green, **an independent review (a reviewer that is not +**Exit criteria:** PR created, CI green (or the hosted-CI-unavailable exception +below applies), **an independent review (a reviewer that is not the authoring model) read the latest diff and surfaced no new accepted Critical / Important findings** -- via resolved Copilot threads or via the different-model path -- dry run passes (if applicable), no mojibake. ### Phase 8 -- Merge -Runs after Phase 7 exits cleanly. **Preconditions:** CI green, **an +Runs after Phase 7 exits cleanly. **Preconditions:** CI green (or the +hosted-CI-unavailable exception below applies), **an independent review satisfied the invariant** (a reviewer that is not the authoring model read the latest diff and surfaced no new accepted Critical / Important findings -- a substituted different-model review satisfies this @@ -500,6 +503,17 @@ exactly as a Copilot review does), all review threads resolved **when the review used the Copilot transport** (there are no GitHub review threads to resolve on the different-model path), dry run passes (if applicable). +**Merging is pre-authorized.** When the preconditions hold, merge, close the +issue, and run Phase 9 cleanup **without asking**, then report using the Task +Complete Summary Format -- the report replaces the permission prompt. The +independent review is mandatory; the permission is not: being unsure whether +the review gate held means finish the review, not ask about the merge. See +**Merging a Finished PR Is Pre-Authorized** in `CLAUDE.md`. + +**Stop and ask only when:** the PR or its issue carries the `hold` label; the +reviewer's model is not recorded; the PR belongs to another session or another +author; or the change is outside the approved design. + This repo only allows **rebase merges**. Squash and merge-commit modes are disabled. Use: @@ -507,8 +521,18 @@ disabled. Use: gh pr merge --rebase --delete-branch ``` -Never merge while CI is red. If a post-merge check fails, route back to -Phase 3 (TDD) on a new branch. +**Hosted CI that cannot run is not "CI red".** When hosted CI cannot run at all, +a recorded local run of the same checks satisfies the gate, provided the switch +to local CI is developer-confirmed and recorded and the PR carries the runner's +output with real counts. The confirmation must be verifiable by any session or +reviewer -- a linked, timestamped developer comment cited in the PR, or the +repository's own committed instructions or config; a personal or session-local +memory note does not count. While on local CI, hosted CI is rechecked daily and +the repository switches back as soon as it works -- the shared local-CI runner's +job, not implemented by hand. If hosted CI ran and failed, that is a hard stop unless +the identical failure is shown, with evidence in the PR, to be pre-existing on +`main`. If a post-merge check fails, route back to Phase 3 (TDD) on a new +branch. **Exit criteria:** PR merged, remote feature branch deleted, ``main`` contains the change. @@ -586,6 +610,7 @@ Once Phase 7 passes with zero unresolved threads and a successful dry run: linked issue number, independent-review status (Copilot threads or the different-model reviewer that stood in for them). 5. **Execute Phase 8 (Merge)** -- rebase-merge the PR with - ``gh pr merge --rebase --delete-branch``. Never merge while CI - is red or with unresolved review threads. + ``gh pr merge --rebase --delete-branch``, without asking. Never + merge while hosted CI is red (see Phase 8 for when hosted CI cannot run) or + with unresolved review threads. 6. Execute Phase 9 (Cleanup) commands with actual values (no placeholders). diff --git a/.github/agents/tests/merge-preauthorized.Tests.ps1 b/.github/agents/tests/merge-preauthorized.Tests.ps1 new file mode 100644 index 00000000..c43f5c2d --- /dev/null +++ b/.github/agents/tests/merge-preauthorized.Tests.ps1 @@ -0,0 +1,158 @@ +#Requires -Version 7.0 +#Requires -Modules @{ ModuleName = 'Pester'; ModuleVersion = '5.0.0' } + +# Structural tests for "merging a finished PR is pre-authorized" (issue #499). +# +# Sessions stopped at the end of a finished dev loop to ask permission to merge, +# even though Phase 8 was already autonomous. Three causes: no explicit durable +# authorization (so a harness's confirm-outward-actions default won), a CI gate +# that cannot be met when hosted CI cannot run at all, and the mandatory review +# being conflated with an optional permission. These tests pin the statement of +# each fix in all three instruction files, scoped to the section that carries +# it, so a revert of any one file fails here. + +BeforeAll { + $script:RepoRoot = Resolve-Path (Join-Path $PSScriptRoot '..\..\..\') | Select-Object -ExpandProperty Path + $script:ClaudeMd = Join-Path $script:RepoRoot 'CLAUDE.md' + $script:DevLoop = Join-Path $script:RepoRoot '.github/agents/dev-loop.agent.md' + $script:CopilotIns = Join-Path $script:RepoRoot '.github/copilot-instructions.md' + + # A section is its heading line plus every line up to the next heading of + # the same or a higher level. + function Get-Section { + param([string]$Path, [string]$HeadingPattern) + $lines = Get-Content -LiteralPath $Path + $start = -1; $level = 0 + for ($i = 0; $i -lt $lines.Count; $i++) { + if ($lines[$i] -match "^(#{1,6})\s+$HeadingPattern") { + $start = $i; $level = $Matches[1].Length; break + } + } + if ($start -lt 0) { return $null } + $end = $lines.Count + for ($j = $start + 1; $j -lt $lines.Count; $j++) { + if ($lines[$j] -match '^(#{1,6})\s' -and $Matches[1].Length -le $level) { $end = $j; break } + } + return ($lines[$start..($end - 1)]) -join "`n" + } + + # The three places a session reads the merge rule, each scoped to the + # section that states it. + $script:Sections = @{ + 'CLAUDE.md merge section' = Get-Section -Path $script:ClaudeMd -HeadingPattern 'Merging\s+a\s+Finished\s+PR\s+Is\s+Pre-Authorized' + 'copilot-instructions.md Merge Step' = Get-Section -Path $script:CopilotIns -HeadingPattern 'Merge\s+Step' + 'dev-loop.agent.md Phase 8' = Get-Section -Path $script:DevLoop -HeadingPattern 'Phase\s+8\b' + } +} + +Describe 'each merge rule states the authorization, the report, and the exceptions' -ForEach @( + @{ Name = 'CLAUDE.md merge section' } + @{ Name = 'copilot-instructions.md Merge Step' } + @{ Name = 'dev-loop.agent.md Phase 8' } +) { + BeforeAll { $script:Text = $script:Sections[$Name] } + + It ' exists' { + $script:Text | Should -Not -BeNullOrEmpty + } + + It ' states merging is pre-authorized, without asking' { + $script:Text | Should -Match '(?i)pre-authori[sz]ed' + $script:Text | Should -Match '(?i)without\s+asking' + } + + It ' replaces the permission prompt with a report' { + $script:Text | Should -Match '(?i)then\s+report' + $script:Text | Should -Match '(?i)Task\s+Complete\s+Summary' + } + + It ' keeps the independent review mandatory while dropping the permission' { + $script:Text | Should -Match '(?i)independent\s+review\s+is\s+(still\s+)?mandatory' + $script:Text | Should -Match '(?i)permission\s+is\s+not' + } + + It ' names the hold label as a reason to stop' { + $script:Text | Should -Match '`hold`' + } + + It ' names missing reviewer-model evidence as a reason to stop' { + $script:Text | Should -Match '(?i)reviewer(''s)?\s+model\s+(is\s+)?not\s+recorded|reviewing\s+model\s+(is\s+)?not\s+recorded' + } + + It ' names another session''s or author''s PR as a reason to stop' { + $script:Text | Should -Match '(?i)another\s+session' + } + + It ' names an out-of-design change as a reason to stop' { + $script:Text | Should -Match '(?i)outside\s+the\s+approved\s+design' + } + + It ' lets a developer-confirmed local CI run stand in when hosted CI cannot run' { + $script:Text | Should -Match '(?i)hosted\s+CI\s+(cannot|can''t)\s+run' + $script:Text | Should -Match '(?i)developer-confirmed' + $script:Text | Should -Match '(?i)real\s+counts' + } + + It ' keeps a hosted CI run that failed as a hard stop' { + $script:Text | Should -Match '(?i)hosted\s+CI\s+ran\s+and\s+failed' + $script:Text | Should -Match '(?i)pre-existing\s+on\s+`?main`?' + } + + It ' requires the local-CI confirmation to be verifiable by any session' { + # "Recorded" alone let a personal memory note count as the record -- + # something no other session or reviewer can see. + $script:Text | Should -Match '(?i)verifiable\s+by\s+any\s+session' + $script:Text | Should -Match '(?i)timestamped\s+developer\s+comment' + $script:Text | Should -Match '(?i)committed\s+instructions\s+or\s+config' + $script:Text | Should -Match '(?i)(personal|session-local)\s+memory\s+note\s+does\s+not\s+count' + } + + It ' names the daily hosted-CI recheck and leaves its mechanics to the runner' { + $script:Text | Should -Match '(?i)re-?checked\s+daily' + $script:Text | Should -Match '(?i)switch(es)?\s+back' + $script:Text | Should -Match '(?i)not\s+implemented\s+by\s+hand' + } +} + +Describe 'the hosted-CI exception reaches the other CI checkpoints' { + It 'the dev-loop-phase-gate Phase 7 checklist qualifies its CI item and points at CLAUDE.md' { + $gate = Get-Section -Path (Join-Path $script:RepoRoot '.github/skills/dev-loop-phase-gate/SKILL.md') -HeadingPattern 'After\s+Phase\s+7' + $gate | Should -Not -BeNullOrEmpty + $ciItem = [regex]::Match($gate, '(?ims)^- \[ \] CI workflows are green.*?(?=^- \[ \]|\z)').Value + $ciItem | Should -Not -BeNullOrEmpty + $ciItem | Should -Match '(?i)hosted\s+CI\s+(cannot|can''t)\s+run' + $ciItem | Should -Match 'CLAUDE\.md' + } + + It 'dev-loop Phase 7 step 4 cross-references the Phase 8 exception' { + $step4 = Get-Section -Path $script:DevLoop -HeadingPattern 'Step\s+4:\s+Verify\s+CI' + $step4 | Should -Not -BeNullOrEmpty + $step4 | Should -Match '(?i)hosted\s+CI\s+(cannot|can''t)\s+run' + $step4 | Should -Match '(?i)Phase\s+8' + } + + It ' qualifies every bare "CI green" precondition' -ForEach @( + @{ File = '.github/agents/dev-loop.agent.md' } + @{ File = '.github/copilot-instructions.md' } + ) { + $raw = Get-Content -LiteralPath (Join-Path $script:RepoRoot $File) -Raw + $bare = [regex]::Matches($raw, '(?i)CI\s+green(?!\s+\(or\s+the\s+hosted-CI-unavailable\s+exception)') + $bare.Count | Should -Be 0 -Because "every 'CI green' in $File must carry the hosted-CI-unavailable qualifier" + } +} + +Describe 'no bare "CI is red" rule survives without the hosted-CI qualification' { + # A bare "never merge while CI is red" is the rule a session could not + # satisfy when hosted CI cannot run at all. Every paragraph that still says + # it must also say what hosted CI being unavailable means. + It ' qualifies every CI-is-red paragraph' -ForEach @( + @{ File = 'CLAUDE.md' } + @{ File = '.github/copilot-instructions.md' } + @{ File = '.github/agents/dev-loop.agent.md' } + ) { + $raw = Get-Content -LiteralPath (Join-Path $script:RepoRoot $File) -Raw + $paragraphs = $raw -split '(?:\r?\n){2,}' + $offending = @($paragraphs | Where-Object { $_ -match '(?i)CI\s+is\s+red' -and $_ -notmatch '(?i)hosted\s+CI' }) + $offending | Should -BeNullOrEmpty -Because "each 'CI is red' paragraph in $File must say how an unavailable hosted CI is handled" + } +} diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 77109b8e..ed5122c8 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -386,11 +386,13 @@ Brainstorm+Issue -> Worktree -> Plan -> [TDD -> Refactor -> Functional Test -> E #### CI Failure Restart Loop After pushing to a PR branch, if CI fails: investigate, fix locally, push again. -A PR must **never** be merged while CI is red. +A PR must **never** be merged while hosted CI is red. When hosted CI cannot run +at all, see the Merge Step below. #### Merge Step -Once the expanding loop exits cleanly (CI green, all review threads resolved, +Once the expanding loop exits cleanly (CI green (or the hosted-CI-unavailable +exception below applies), all review threads resolved, **an independent review satisfied the invariant** -- a reviewer that is not the authoring model read the latest diff and surfaced no new accepted Critical / Important findings -- dry run passes if applicable), merge the PR before running @@ -399,14 +401,35 @@ different model than the authoring one satisfies it equally. See the **Independent Code Review -- Reviewer != Author Model** section in `CLAUDE.md` for the canonical rule, the substitution trigger, and the detection call. +**Merging is pre-authorized.** When those criteria hold, merge, close the issue, +and clean up the worktree **without asking**, then report using the **Task +Complete Summary Format** below -- the report replaces the permission prompt and +is not optional. The independent review is mandatory; the permission is not. See +**Merging a Finished PR Is Pre-Authorized** in `CLAUDE.md`. + **This repo only allows rebase merges:** ```powershell gh pr merge --rebase --delete-branch ``` -Never merge while CI is red. Never merge with unresolved review threads. If any -post-merge check fails, route back to Phase 3 (TDD) on a new branch. +Never merge with unresolved review threads. **Hosted CI that cannot run is not +"CI red"**: when hosted CI cannot run at all, a recorded local run of the same +checks satisfies the gate, provided the switch to local CI is +developer-confirmed and recorded and the PR carries the runner's output with +real counts. The confirmation must be verifiable by any session or reviewer -- a +linked, timestamped developer comment cited in the PR, or the repository's own +committed instructions or config; a personal or session-local memory note does +not count. While on local CI, hosted CI is rechecked daily and the repository +switches back as soon as it works -- the shared local-CI runner's job, not +implemented by hand. If hosted CI ran and failed, that is a hard stop unless the +identical failure is shown, with evidence in the PR, to be pre-existing on +`main`. If any post-merge check fails, route back to Phase 3 (TDD) on a new +branch. + +**Stop and ask only when** the PR or its issue carries the `hold` label; the +reviewer's model is not recorded; the PR belongs to another session or another +author; or the change is outside the approved design. Use `@plan` when exploring a new idea before committing to implementation. Use `@systematic-debugging` (or the `systematic-debugging` skill) for bugs. diff --git a/.github/skills/dev-loop-phase-gate/SKILL.md b/.github/skills/dev-loop-phase-gate/SKILL.md index b8715203..c0e0549d 100644 --- a/.github/skills/dev-loop-phase-gate/SKILL.md +++ b/.github/skills/dev-loop-phase-gate/SKILL.md @@ -84,7 +84,9 @@ npx tsc && npx vitest run ### After Phase 7 (PR + Independent Review) - [ ] PR created with `Closes #` -- [ ] CI workflows are green +- [ ] CI workflows are green -- or, when hosted CI cannot run at all, the + developer-confirmed local-CI exception in `CLAUDE.md` (**Merging a + Finished PR Is Pre-Authorized**) applies - [ ] An independent review (a reviewer that is **not** the authoring model) read the latest diff and surfaced no new accepted Critical / Important findings diff --git a/CLAUDE.md b/CLAUDE.md index 5e6d13ad..b3e38427 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -145,6 +145,47 @@ An **empty** result means Copilot review is not enabled -- fall through to the different-model path immediately rather than polling for a review that will never arrive. +## Merging a Finished PR Is Pre-Authorized + +**When the dev loop's exit criteria hold, merge the PR, close its issue, and +clean up the worktree without asking -- then report.** This is the durable +authorization: do not seek per-PR confirmation. A merge is outward-facing, and +agent harnesses default to confirming such actions unless durably authorized; +this section is that authorization. + +**The independent review is mandatory; the permission is not.** Merging without +asking weakens no gate: the different-model review above, behavior-first tests, +and evidence capture must all still hold. Being unsure whether the review gate +held is a reason to finish the review, not to ask about the merge. + +**Then report**, using the Task Complete Summary Format (below): what was +implemented, the PR and issue links, the result display, **Assumptions**, and +anything that needs the user. The report replaces the permission prompt; it is +not optional. + +**Hosted CI that cannot run is not "CI red".** When hosted CI cannot run at all +(billing block, quota, outage), a recorded local run of the same checks +satisfies the CI gate -- provided the repository's switch to local CI is +developer-confirmed and recorded, and the PR carries the local runner's output +with real counts pasted in. Whether hosted CI is unavailable is not a judgement a +session makes on its own: the developer's confirmation must be verifiable by any +session or reviewer -- a linked, timestamped developer comment cited in the PR, +or the repository's own committed instructions or config. A personal or +session-local memory note does not count. While a repository is on local CI, +hosted CI is rechecked daily and the repository switches back as soon as it +works; that belongs to the shared local-CI runner and is not implemented by hand +in a session. If hosted CI ran and failed, that is still a hard stop +-- unless the identical failure is shown, with evidence in the PR, to be +pre-existing on `main`. + +**Stop and ask only when:** + +- the PR or its issue carries the `hold` label, or is otherwise explicitly held; +- the independent-review evidence cannot be found -- the reviewer's model is not + recorded; +- the PR belongs to another session or another author; +- the change is outside the approved design and needs an owner decision. + ## Key References - **Development conventions**: [`.github/copilot-instructions.md`](./.github/copilot-instructions.md) -- code style, testing conventions, branching strategy, commit format, and workflow. @@ -300,7 +341,8 @@ See the **Task Complete Summary Format** subsection of - Commit format: `type(scope): description` (Conventional Commits) - Merge to `main` only via pull request after the dev loop passes. **This repo only allows rebase merges** -- use `gh pr merge --rebase --delete-branch`. - Never merge while CI is red. + Merging is pre-authorized -- do not ask. Never merge while hosted CI is red; for + when hosted CI cannot run, see **Merging a Finished PR Is Pre-Authorized**. - **All commits must come from a worktree** — the pre-commit hook blocks commits from the repo root. See the "Concurrent Session Safety" section in `.github/copilot-instructions.md` for details. - **After a PR closes**, clean up the worktree and local branch. The recommended