From 40a264a4472ba1351d2448ce861c967e20945618 Mon Sep 17 00:00:00 2001 From: CleverThis Date: Sat, 11 Apr 2026 02:30:50 +0000 Subject: [PATCH] docs: dual-account PR review arch, ATM interval centralization, reviewer rename (Cycle 13 rebased) --- CHANGELOG.md | 35 +++++++++++++++++ docs/development/automation-tracking.md | 50 ++++++++++++++++++++++++- docs/development/review_playbook.md | 41 ++++++++++++++++++++ 3 files changed, 124 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 89d67e2c3..d6f192b53 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,41 @@ The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Added +- **Dual-Account PR Review Architecture**: Introduced a separate Forgejo reviewer + account (`FORGEJO_REVIEWER_PAT` / `FORGEJO_REVIEWER_USERNAME` / + `FORGEJO_REVIEWER_PASSWORD`) to solve the shared-bot-account problem where a + single account cannot approve its own PRs. The `pr-reviewer` agent now uses + reviewer credentials for formal `APPROVED` reviews via curl (the primary + approval path). The `pr-review-pool-supervisor` and `product-builder` pass + reviewer credentials when dispatching reviewers. The `pr-merge-pool-supervisor` + recognises the reviewer account as the primary approver. + +- **ATM-Centralized Cycle Interval Calculation**: The + `automation-tracking-manager` now handles rolling-average cycle interval + calculation internally via the `--sleep-interval-default` parameter. All 17 + supervisor agents have had their duplicated 7-line interval calculation blocks + removed; they now pass a single default and let the ATM compute the actual + interval. A new `CYCLE_ANNOUNCEMENT_REVIEW` operation (#11) was added to the + ATM operation set. + +- **Curl-Based Review Authentication**: Because the Forgejo MCP server + authenticates with a single server-level token that cannot be overridden + per-call, the `pr-reviewer` agent now uses `curl` with `FORGEJO_REVIEWER_PAT` + for all write operations (reviews, comments). MCP tools are used only for + read operations. The `pr-review-pool-supervisor` documents the + READ-via-MCP / WRITE-via-curl split. + +### Changed + +- **PR Reviewer Renamed**: `pr-self-reviewer` renamed to `pr-reviewer` + throughout the agent system. All references to "PR Self-Reviewer" and + "self-reviewer" updated to "PR Reviewer" in agent definitions, spec, and + coordination sections. + +- **Staleness Threshold Increased**: `system-watchdog-pool-supervisor` now + uses a 2× expected interval as the staleness threshold (previously 1.2×), + reducing false-positive stall detections for agents with variable cycle times. + - **Git Worktree Sandbox Apply** (#4454): The `plan apply` command now merges LLM-generated changes via `git merge` from an isolated worktree branch instead of flat `shutil.copy2`. Displays spec-aligned Apply Summary diff --git a/docs/development/automation-tracking.md b/docs/development/automation-tracking.md index 2cd7e25f2..38f5e1aba 100644 --- a/docs/development/automation-tracking.md +++ b/docs/development/automation-tracking.md @@ -237,15 +237,23 @@ When a stalled agent is detected: # Pseudocode for health monitoring for issue in automation_tracking_issues: if issue.title.matches("[AUTO-*] * (Cycle *)"): - expected_interval = get_expected_interval(issue.agent_prefix, issue.type) + # Interval is parsed from the issue body's + # "Estimated Cycle Interval" field (ATM-injected) + expected_interval = parse_interval_from_body(issue.body) time_since_creation = now() - issue.created_at - staleness_threshold = expected_interval * 1.2 # 20% tolerance + staleness_threshold = expected_interval * 2.0 # 2× tolerance if time_since_creation > staleness_threshold: mark_agent_as_stalled(issue.agent_prefix) trigger_recovery_actions(issue.agent_prefix, issue) ``` +> **Note**: The staleness threshold was increased from 1.2× to 2× to reduce +> false-positive stall detections for agents with variable cycle times. The +> expected interval is now parsed from the `**Estimated Cycle Interval**` field +> in the issue body (injected by the ATM) rather than looked up from a hardcoded +> table. + ## Standardized Tracking Issue Templates ### Common Header Format @@ -524,9 +532,47 @@ A second migration introduced the centralized `automation-tracking-manager` suba to prevent cycle reuse issues where agents were commenting on old tracking issues instead of creating new ones. +## Dual-Account PR Review Architecture + +The agent system uses two separate Forgejo accounts to handle the constraint that +a single account cannot approve its own pull requests. + +### Accounts + +| Account | Purpose | Credentials | +|---------|---------|-------------| +| **Bot account** (`HAL9000`) | Creates PRs, reads issues, general automation | `FORGEJO_PAT` | +| **Reviewer account** | Submits formal `APPROVED` reviews on bot PRs | `FORGEJO_REVIEWER_PAT`, `FORGEJO_REVIEWER_USERNAME`, `FORGEJO_REVIEWER_PASSWORD` | + +### MCP Token Limitation + +The Forgejo MCP server authenticates with a **single server-level token** that +cannot be overridden per-call. This means: + +- **Read operations** (list PRs, get issues, etc.) → use MCP tools (bot account) +- **Write operations** (submit reviews, post approval comments) → use `curl` with + `FORGEJO_REVIEWER_PAT` (reviewer account) + +```bash +# Example: submit a formal APPROVED review as the reviewer account +curl -s -X POST \ + -H "Authorization: token ${FORGEJO_REVIEWER_PAT}" \ + -H "Content-Type: application/json" \ + "https://git.cleverthis.com/api/v1/repos/${OWNER}/${REPO}/pulls/${PR_NUMBER}/reviews" \ + -d '{"event":"APPROVED","body":"LGTM — automated review"}' +``` + +### Credential Propagation + +The `product-builder` passes all three reviewer credentials when launching the +`pr-review-pool-supervisor`, which in turn passes them to each dispatched +`pr-reviewer` worker. The `pr-merge-pool-supervisor` recognises the reviewer +account as the primary approver when checking merge readiness. + ## Related Documentation - [System Watchdog](system-watchdog.md) - Specific documentation for watchdog agent - [Documentation Writer](docs-writer.md) - Documentation writer agent reference +- [Review Playbook](review_playbook.md) - PR review process and dual-account review guide - [Quality Automation](quality-automation.md) - Quality gate automation details - [Ops Runbook](ops-runbook.md) - General operational procedures diff --git a/docs/development/review_playbook.md b/docs/development/review_playbook.md index 93f6181fc..c0b6777b3 100644 --- a/docs/development/review_playbook.md +++ b/docs/development/review_playbook.md @@ -294,3 +294,44 @@ A PR may only be merged if **all** of the following are true: 5. `nox -s security_scan` reports zero high/critical findings 6. At least one reviewer has approved 7. All P0 and P1 findings are resolved + +--- + +## Automated PR Review (Dual-Account Architecture) + +The `pr-reviewer` agent performs automated code reviews using a **separate +reviewer Forgejo account** to satisfy the constraint that a single account +cannot approve its own pull requests. + +### How It Works + +1. **Bot account** (`HAL9000`) creates the PR using `FORGEJO_PAT` / MCP tools. +2. **Reviewer account** submits a formal `APPROVED` review using + `FORGEJO_REVIEWER_PAT` via `curl` (MCP cannot override the server-level token). + +### MCP Token Limitation + +The Forgejo MCP server uses a single server-level token. The `pr-reviewer` +agent therefore splits operations: + +| Operation | Method | Credential | +|-----------|--------|-----------| +| Read PR details, list files | MCP tools | `FORGEJO_PAT` (bot) | +| Submit `APPROVED` review | `curl` POST | `FORGEJO_REVIEWER_PAT` (reviewer) | +| Post approval comment | `curl` POST | `FORGEJO_REVIEWER_PAT` (reviewer) | + +### Approval Forms + +The `pr-merge-pool-supervisor` recognises the following as valid approvals: + +- Formal `APPROVED` review from the reviewer account (primary path) +- Comment containing `LGTM`, `✅`, `Approved`, or `"ready to merge"` from the + reviewer account (fallback) + +### Required Credentials + +| Variable | Description | +|----------|-------------| +| `FORGEJO_REVIEWER_PAT` | Personal access token for the reviewer account | +| `FORGEJO_REVIEWER_USERNAME` | Username of the reviewer account | +| `FORGEJO_REVIEWER_PASSWORD` | Password for the reviewer account (used by `pr-review-pool-supervisor` for stuck-PR recovery) | -- 2.52.0