diff --git a/CHANGELOG.md b/CHANGELOG.md index a56658b..39d4b8f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,13 @@ ## Unreleased +- End ordinary worker delivery at REVIEW_READY; require an implementing-worker review + request and a different task, with ordinary self-review only by explicit user approval. +- Add MERGE_READY alongside REVIEW_READY after eligible review and current head/base/checks; + invalidate it when the underlying evidence changes without granting merge authority. +- Permit named commit-error/conflict resolvers to close assigned PRs, edit assigned issues + and self-review only under explicit scoped grants; retain original authors and history. + ## 3.0.0 - 2026-09-07 - Replace predecessor-acknowledged ownership transfer with explicit user-approved succession, diff --git a/README.md b/README.md index df01a52..6bc0d85 100644 --- a/README.md +++ b/README.md @@ -124,6 +124,10 @@ for the major version, not an incompatible storage migration. The coordination reference defines collision-checked IDs, assignment lineage, OPS ownership/commit receipts, and `DEV`/`OPS` plus `OCCUPIED`/`REVIEW_READY` label synchronization. +Ordinary implementation ends at `REVIEW_READY`. Code review requires the implementing +worker's request and a different task; ordinary self-review needs explicit user approval. +A valid review and current head/base/checks add `MERGE_READY` alongside `REVIEW_READY`. +Neither label grants merge rights; new edits or invalidated evidence remove `MERGE_READY`. Personal accounts still author development commits and create PRs when the repository separates those actions from OPS records. Labels do not grant ownership or merge authority. diff --git a/SKILL.md b/SKILL.md index 94544f5..7716396 100644 --- a/SKILL.md +++ b/SKILL.md @@ -194,7 +194,8 @@ Preserve checkpoints, reconcile current ownership and stop only actual conflicti This does not expand merge, deployment, credential or published-history authority. Use the reference's Draft/Ready stages, authoritative work-state record, issue mirrors, -OPS commit/PR receipts and managed `DEV`/`OPS`, `OCCUPIED`/`REVIEW_READY` labels. Provision +OPS commit/PR receipts and managed `DEV`/`OPS`, `OCCUPIED`/`REVIEW_READY` labels, with +`MERGE_READY` added only after eligible review and current verification. Provision missing labels only within setup authorization. Separate personal development commits and PR creation from configured OPS ownership/status/review writes; authenticate each operation independently. Keep personal mappings, host paths and credentials outside this @@ -211,7 +212,9 @@ For an explicitly assigned resolver of a named PR set, read [conflict resolution](references/conflict-resolution.md) before editing or publishing. The mode preserves the actual worker ID and original authorship, records a versioned merge sequence, and permits `Conflict resolution: LGTM` only for verified integration. -It does not expand ordinary worker permissions. Named merge authorization is separate +A named commit-error or conflict resolver may close assigned PRs, edit assigned issues +and self-review only under an explicit user grant for those actions and scope. It gains +no general maintainer authority. Named merge authorization is separate from review approval; execute/resume an authorized sequence in the foreground using fresh head/base/tree evidence and actual merge receipts. The reference includes the complete copyable prompt; configure its model and reasoning effort in the launcher. @@ -363,35 +366,36 @@ Default squash is a proposed merge method, not authorization to merge. Keep the preview, approval, issue-creation, PR-publication, and branch-deletion boundaries. Do not introduce a PR into an explicitly direct-commit or local-only task merely to apply squash. -## Quick PR Review - -When the user requests PR reviews or the repository opts into them, read -[PR review guidance](references/pr-review.md). Review-only requests do not authorize -implementation, issue management, merging, or unrelated cleanup. - -- Default to a brief review of the exact diff, direct contracts/callers, and relevant - verification. Expand only for a concrete risk or finding. Reuse valid test evidence. -- When assigned to review PRs made by others, identify work this agent actually created; - account authorship alone cannot distinguish workers sharing one GitHub account. A successor - who modifies inherited code uses `Self-review: LGTM`; changing workers is not independent review. -- If review publication is authorized and the scope has no blockers, the first line must - be exactly `Self-review: LGTM` for work this agent authored or `Review: LGTM` for - another worker's work. Only an explicitly assigned conflict resolver may instead use - `Conflict resolution: LGTM` under the conflict-resolution reference. These are the - only approval headings; never use bare `OK`, bare `LGTM`, or another variant. - For blockers, use `Changes requested` with actionable - findings from the reference template. Record the reviewed head and scope; review the - new delta when the head changes. -- A shared author account cannot formally approve or request changes on its own PR. - Publish the result as a comment review in that case and describe its actual state. -- For an ordinary worker's own completed PRs, reuse the implementation and tester's passing - evidence, briefly review the final diff, and post `Self-review: LGTM` after required - record, label and Draft/Ready synchronization succeeds and no blocker remains. - Do not add another audit or repeat valid tests. Recheck only a relevant change or failure. - Do not present self-review as independent or human approval. Unfinished work and pending - required checks must not receive any approval heading. -- Keep PR-only worker boundaries intact. No approval heading authorizes merge, local-main - integration, issue edits, or additional implementation. Do not repeat an unchanged review. +## Requested PR Review + +For worker delivery, implementation and verification finish at `REVIEW_READY`. Do not +perform or publish an ordinary code review by default, at delivery checkpoints, or just +because a repository enables tracking. Read [PR review guidance](references/pr-review.md) +only for an eligible request or an explicit user-approved exception. + +- Ordinary review requires the implementing worker's durable request and takes place + in a different conversation/task, strictly within that implementation scope. Record the + requesting worker, implementation task, reviewer task, PR, head/base and outcome. A + changed worker name in the same conversation does not satisfy task separation. Do not + spawn a new task without the runtime's required user authority; record a pending request. +- Self-review is prohibited for ordinary workers unless the user explicitly approves it + for the named scope. An implementing worker cannot approve its own exception. This + exception does not claim independent authorship or human review. A successor taking + implementation ownership follows the same rule, including for inherited code. +- Reuse implementation/test evidence and inspect only the requested diff and direct + contracts/callers. No proactive unrelated review or wider audit is implied. Verification + and final diff/scope checks remain part of implementation; they are not a PR approval. +- Authorized successful reviews start exactly with `Review: LGTM` for a separate reviewer, + `Self-review: LGTM` for explicitly user-approved self-review, or `Conflict resolution: LGTM` + for an explicitly granted resolver review under [conflict resolution](references/conflict-resolution.md). + Preserve historical headings; use a concrete `Changes requested` outcome for blockers. +- A valid review plus current passing checks and matching PR head/base permits adding + `MERGE_READY` alongside `REVIEW_READY`; it does not replace the work status. Reconcile + the record and labels before reporting readiness. Remove `MERGE_READY` when edits, + head/base changes, a required check failure or an invalid review invalidate its evidence. +- Publish through the configured OPS identity where required and use COMMENT when formal + self-approval is unavailable. No heading or either ready label grants merge, issue-edit, + deployment or local-main authority. Do not repeat an unchanged review. ## Tutorial and Documentation Checkpoint diff --git a/references/conflict-resolution.md b/references/conflict-resolution.md index 76439b3..dbe0cfb 100644 --- a/references/conflict-resolution.md +++ b/references/conflict-resolution.md @@ -18,6 +18,11 @@ this mode. User and repository authority still govern every action. alone does not grant merge, issue-management, branch-deletion or checkout-sync authority. Use the explicit grant for this set; ask only for missing authority or a material scope change, not repeated routine confirmation. +- An explicit user-designated commit-error or conflict resolver may also close assigned + PRs, edit assigned issue scope/records and self-review when the grant names those actions + and targets. Record that grant and preserve original authors, links and historical + reviews. This exception does not authorize unrelated maintenance or merge by label. + Commit-error repair alone does not activate this reference's merge-sequence machinery. - Prefer updating existing PRs with ordinary commits and pushes. Use a separate integration PR only when the existing split cannot produce safe, verifiable intermediate states and its publication is authorized. Preserve source PR/issue @@ -57,7 +62,9 @@ missing write access or partially applied updates before advancing. The existing `Work status` model remains `OCCUPIED` / `REVIEW_READY`. Record the separate `Merge status` here; do not substitute merge states for worker states or treat a PR's -Ready flag as sequence approval. +Ready flag as sequence approval. `MERGE_READY` is a separate, additive label alongside +`REVIEW_READY`, issued only after an eligible review and current head/base/check evidence; +it does not replace Work status or authorize the sequence. | Merge status | Meaning | | --- | --- | @@ -144,8 +151,9 @@ copied from commit messages, PR bodies or sequence comments. ## Review, foreground merge and resume -Only the explicitly assigned resolver may publish this exact first line for its -verified integration scope: +Only the explicitly assigned resolver with an explicit scoped review/self-review grant +may publish this exact first line for its verified integration scope. Without that grant, +stop at `REVIEW_READY` and use the ordinary requested-review path: ```markdown Conflict resolution: LGTM @@ -206,6 +214,7 @@ Target PRs: Base branch: Resolver ID: Merge authorization: granted for this named PR set +Review authorization: user-approved resolver self-review for this named PR set Merge method: squash Act as the designated conflict resolver. Preserve original authorship, diff --git a/references/pr-review.md b/references/pr-review.md index 050246a..0cd2d4b 100644 --- a/references/pr-review.md +++ b/references/pr-review.md @@ -1,8 +1,27 @@ # Focused Pull Request Reviews -Use this mode for an explicitly requested review or a repository-authorized review -checkpoint. Where the repository requests peer review at delivery, preserve the -assigned implementation queue and review only the relevant delta. Do not create a monitor. +Ordinary worker reviews are off by default. The implementing worker must request a +review of its exact implementation scope, and a different conversation/task performs it. +A delivery checkpoint, ready label or repository tracking opt-in is not a request. +Do not review unrelated work, start a monitor or invent new implementation scope. + +## Request and eligibility + +- Preserve a durable request with implementing worker, Assignment, PR, implementation + task identifier, requested scope, exact head/base and existing verification evidence. + The reviewer records a different task identifier and its actual worker ID. A new model + or ID in the same task is not a separate review. If task identity cannot be established, + leave review pending instead of asserting separation. Missing chat history does not + block ordinary user-approved succession; only review eligibility needs this evidence. +- The implementing worker may request review but may not waive separation or authorize + its own self-review. Only explicit user approval for the named self-review permits that + exception. Record the approval and mark the result as self-review, never independent + or human approval. An implementation successor has the same restriction. +- A named commit-error or conflict resolver follows its explicit action/scope grant in + [conflict resolution](conflict-resolution.md); a role name alone does not grant self-review. +- Use the configured guarded review request/publication operation where available. Check + current owner, task separation or explicit exception, scope and head/base before writing. + A request is not permission to create a new task if the runtime requires user authorization. ## Review and publication @@ -29,33 +48,37 @@ Judge the implementation against the agreed outcome and code evidence, not again unpublished personal design. A further improvement is blocking only when necessary for the authorized outcome; otherwise keep it explicitly optional. -For an ordinary worker's completed PR, reuse the implementation and tester's passing evidence. -After required record, label and Draft/Ready synchronization succeeds, briefly inspect -the final diff and post `Self-review: LGTM` if it has no blocker; do not add a second audit or rerun valid tests. Recheck only a relevant new -change or failure and name the reviewed head. An unfinished scope, known blocker or -pending required check must not receive any approval heading. +Implementation ends with verified `REVIEW_READY` delivery and no approval heading. +For an eligible review, reuse the implementation and tester's passing evidence; inspect +the requested delta and rerun checks only for a relevant change or failure. An unfinished +scope, known blocker or pending required check must not receive any approval heading. Successful reviews use one exact first line, according to the actual assignment: | Approval heading | Applicable work | | --- | --- | -| `Self-review: LGTM` | Work the reviewer authored. | -| `Review: LGTM` | Another worker's work reviewed without taking ownership. | -| `Conflict resolution: LGTM` | Verified integration by the explicitly assigned `conflict-resolver` for the named PR set. | +| `Self-review: LGTM` | Own or inherited implementation only under explicit user-approved self-review for this scope. | +| `Review: LGTM` | Implementing-worker-requested review in a different task without implementation ownership. | +| `Conflict resolution: LGTM` | Verified integration by the explicitly assigned `conflict-resolver` with review authority for the named PR set. | These are the only approval headings; never use bare `OK`, bare `LGTM`, or another variant. All require the same completed scope and verified evidence for the recorded head/base; none claims independent human approval or guarantees that every behavior was tested. Determine authorship from actual work, not the shared GitHub account. -A successor who edits inherited code uses `Self-review: LGTM`, preserves earlier -worker attribution and links the reused verification. A new worker/model/account is -not independent review by itself. A separate review-only assignment may use `Review: LGTM` only -without taking implementation ownership or implementing the reviewed work. A successor -accepting implementation ownership reports completion as self-review, including inherited work. Publish through the configured OPS review -identity when required; the record still identifies the actual reviewing worker. -A review never grants permission to merge or deploy. A conflict resolver preserves -original authorship and follows the additional tree/sequence requirements in -[conflict resolution](conflict-resolution.md); ordinary reviews do not activate it. +A successor accepting implementation ownership finishes at `REVIEW_READY`, including +inherited work. It may self-review only with the user's explicit exception. Changing +worker/model/account does not make the work independent. Publish through the configured +OPS identity where required and retain actual worker, task, request or exception evidence. + +Add the independent `MERGE_READY` label alongside `REVIEW_READY` only after a valid review, +current passing required checks and matching recorded/current head and base. Preserve the +review receipt and approval basis; an OPS author alone is not eligibility proof. If any +of those facts changes, remove `MERGE_READY` until the affected evidence is renewed. +New implementation returns to Draft/`OCCUPIED`; record and mirrors must agree. Keep earlier +reviews as history rather than rewriting their labels or claiming they cover new code. +Neither ready label nor any review heading grants merge, issue-edit or deployment rights. +An explicit resolver preserves original authors and follows the extra tree/sequence gates +in [conflict resolution](conflict-resolution.md); ordinary reviews do not activate them. ## Project-oriented reason codes @@ -101,7 +124,8 @@ Reviewed head: ``. ``` Omit the issue field only when no issue is assigned; never invent a number. -For a self-review completion, use the same compact evidence format with this exact first line: +Only for explicit user-approved self-review, use the same compact evidence format and +include the approval reference with this exact first line: ```markdown Self-review: LGTM diff --git a/references/worker-coordination.md b/references/worker-coordination.md index 57d6a04..3f62aaa 100644 --- a/references/worker-coordination.md +++ b/references/worker-coordination.md @@ -14,7 +14,8 @@ titles are optional navigation aids, never the only location of checkpoints or a - The coordinator handles authorized intake, scope and allocation. Ordinary workers implement an approved assignment in persistent isolated worktrees, verify it and - deliver its PR and review. They do not inherit coordinator merge or checkout rights. + deliver its PR at `REVIEW_READY`. Review is a separate, requested step; they do not + inherit coordinator merge or checkout rights. - Preserve user-supplied IDs, including legacy names such as `worker-1`. If none is supplied, allocate `--<8 lowercase hex digits>`, using UTC and a random suffix; for example `codex-20260907T190000Z-a1b2c3d4`. Verify against @@ -120,7 +121,8 @@ A returning old worker must re-read current ownership before any mutation and st former assignment after transfer unless explicitly reassigned. Succession authorizes preservation, implementation and the already approved delivery scope; it does not grant merge, deployment, credential, paid-work or force-push authority. A successor modifying -inherited code uses `Self-review: LGTM`, not independent `Review: LGTM`. +inherited code cannot self-review without explicit user approval and must not present +that exception as independent `Review: LGTM`. ## OPS records, labels and transitions @@ -146,13 +148,16 @@ per-worker labels or a Project board. Labels summarize records and never lock ow | --- | --- | | Work kind | Exactly one of `DEV` or `OPS`, determined by task purpose, not API author. Product implementation is `DEV`; operational tooling and workflow policy are `OPS`. | | Work status | Exactly one of `OCCUPIED` or `REVIEW_READY` on active managed issues/PRs. | +| Review readiness | Add `MERGE_READY` alongside `REVIEW_READY` only after eligible review and current passing head/base/check evidence; this is an independent label, not a new Work status. | | Blocked | Keep `OCCUPIED` and put the concrete blocker and next action in verification. | For multiple assignments on one issue, derive the issue state from all current records: any unfinished, running or blocked assignment makes it `OCCUPIED`; only all completed, verified assignments make it `REVIEW_READY`. Apply `OPS` only to wholly operational scope; use `DEV` when the combined issue includes product implementation. Preserve individual -assignment kinds in records. Missing or inconsistent records block a readiness claim. +assignment kinds in records. Add issue `MERGE_READY` only when every current assignment +has eligible review and matching current evidence; remove it if any loses eligibility. +Missing or inconsistent records block a readiness claim. 1. **Assign/open:** check claims and record `OCCUPIED` before implementation. Create a Draft PR at the first meaningful pushed change and mirror its actual link to issues. @@ -160,15 +165,25 @@ assignment kinds in records. Missing or inconsistent records block a readiness c 2. **Work/revise:** keep Draft/`OCCUPIED` during implementation, waiting, failures or required checks. Before revising a ready PR, return its state and mirrors to occupied. Verification is `not run`, `running`, `passed`, `failed` or `blocked: `. -3. **Ready:** only after scope, checks and writer work finish, synchronize authoritative - state, issue mirrors, labels and Draft/Ready status. Read back all required targets; - interrupted synchronization remains incomplete and must be reconciled before claiming - readiness. A changed head invalidates old review evidence. Post `Self-review: LGTM` - for own completed work or `Review: LGTM` for peer review without implementation. - Only the explicit resolver may use `Conflict resolution: LGTM` under its extra gates. -4. **Finish:** only after an authorized maintainer merge, reconcile actual merge/issue - receipts under that authority. A worker never infers merged status from readiness. - Partial delivery keeps remaining scope open; no label grants permission to close it. +3. **Review ready:** after implementation, checks and writer work finish, set + `REVIEW_READY`, synchronize authoritative state, mirrors, labels and PR Ready status. + Verify all updates before claiming readiness. The ordinary implementing worker is done; + do not automatically review its own or other workers' code or post an approval heading. +4. **Requested review:** the implementing worker records its exact scope and requests review + in a different conversation/task under [review eligibility](pr-review.md#request-and-eligibility). + Record both task IDs, actual workers and head/base evidence. A worker cannot authorize + its own self-review; only an explicit user-approved exception permits `Self-review: LGTM`. + Eligible separate reviews use `Review: LGTM`; an explicitly granted resolver may use + `Conflict resolution: LGTM` under its additional gates. +5. **Merge ready:** add `MERGE_READY` alongside `REVIEW_READY` only with a valid eligible + review and current passing checks for the matching head/base. Re-read all records and + labels; partial synchronization is not readiness. Remove `MERGE_READY` on new edits, + changed head/base, failed required checks or invalid review. Return to Draft/`OCCUPIED` + before further implementation. Neither readiness label grants merge authority. +6. **Finish:** only after an authorized maintainer or resolver merge, reconcile actual + merge/issue receipts under that authority. Partial delivery leaves scope open. Named + commit-error/conflict resolvers may close assigned PRs or edit assigned issues only if + those actions are explicitly granted; no global maintainer rights are implied. Re-read ownership and head immediately before mutations. Update only owned records; ambiguous matches, changed owners and partial writes require reconciliation. A peer diff --git a/tests/test_package.py b/tests/test_package.py index a9971bc..299bd75 100644 --- a/tests/test_package.py +++ b/tests/test_package.py @@ -99,10 +99,12 @@ def test_copyable_prompt_requires_named_authority_and_keeps_launcher_settings_ou prompts = [body for kind, body in blocks if body.startswith('Use $commit-it in conflict-resolution mode.')] self.assertEqual(len(prompts), 1) prompt = prompts[0] - fields = dict(re.findall(r'^(Repository|Target PRs|Base branch|Resolver ID|Merge authorization|Merge method): (.+)$', prompt, re.MULTILINE)) + fields = dict(re.findall(r'^(Repository|Target PRs|Base branch|Resolver ID|Merge authorization|Review authorization|Merge method): (.+)$', prompt, re.MULTILINE)) self.assertEqual(fields, {'Repository': '', 'Target PRs': '', 'Base branch': '', 'Resolver ID': '', - 'Merge authorization': 'granted for this named PR set', 'Merge method': 'squash'}) + 'Merge authorization': 'granted for this named PR set', + 'Review authorization': 'user-approved resolver self-review for this named PR set', + 'Merge method': 'squash'}) self.assertNotIn('gpt-6-astra', prompt) self.assertIn('model `gpt-6-astra`, reasoning effort\n`high`', text.replace(prompt, '')) self.assertIn('On resume, reconcile live state and never repeat an already completed merge.', prompt)