diff --git a/CHANGELOG.md b/CHANGELOG.md index 7a76a7292..de9bf1524 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 e5e12d101..34bcea51a 100644 --- a/docs/development/automation-tracking.md +++ b/docs/development/automation-tracking.md @@ -90,18 +90,55 @@ The `automation-tracking-manager` subagent: 4. Applies all required labels atomically 5. Returns the new issue number to the calling agent +### Cycle Interval Calculation (ATM-Centralized) + +As of the Subagent Specialization Principle refactor, cycle interval calculation +is **centralized in the ATM**. Agents no longer compute rolling averages +themselves. Instead they pass a `--sleep-interval-default` parameter to +`CREATE_TRACKING_ISSUE` and the ATM injects the computed +`**Estimated Cycle Interval**: Nmin` field into the issue body automatically. + +This eliminated 17 duplicated 7-line calculation blocks across all supervisor +agents. + +### Supported Operations + +| Operation | Description | +|-----------|-------------| +| `READ_TRACKING_STATE` | Recover cycle number, timestamps, and interval from previous session | +| `CREATE_TRACKING_ISSUE` | Close old issue, create new one with correct cycle number and labels | +| `UPDATE_TRACKING_ISSUE` | Add a comment to the current tracking issue | +| `CREATE_ANNOUNCEMENT_ISSUE` | Create a cross-agent announcement with priority | +| `CLOSE_ANNOUNCEMENT_ISSUE` | Close an announcement when no longer relevant | +| `LIST_TRACKING_ISSUES` | List all current tracking and announcement issues | +| `READ_ANNOUNCEMENTS` | Read announcements from other agents (cross-agent awareness) | +| `REVIEW_OWN_ANNOUNCEMENTS` | Review and clean up own announcements | +| `CYCLE_ANNOUNCEMENT_REVIEW` | Periodic review of announcements during normal cycle | + ### Usage -Agents that use the centralized manager delegate all tracking operations to it: +Agents that use the centralized manager delegate all tracking operations to it. +The **critical startup order** is: READ state first, then CREATE new issue. ```bash -# Example: implementation-pool-supervisor delegating to tracking manager -# (Inside agent definition — pseudocode) -TRACKING_RESULT=$(call_subagent automation-tracking-manager \ - --prefix "AUTO-IMP-POOL" \ - --type "Health Report" \ - --body "...") -NEW_ISSUE_NUMBER=$(echo "$TRACKING_RESULT" | jq -r '.issue_number') +# STEP 1 (ON STARTUP): Read state from previous session BEFORE creating new +recovered_state=$(task automation-tracking-manager "READ_TRACKING_STATE" \ + --agent-prefix "AUTO-IMP-POOL" \ + --tracking-type "Health Report" \ + --repo-owner "cleveragents" \ + --repo-name "cleveragents-core") +# Returns: cycle_number, created_at, offline_duration_minutes, +# estimated_cycle_interval, issue_body, comments + +# STEP 2: Create new tracking issue (closes ALL old status issues first) +task automation-tracking-manager "CREATE_TRACKING_ISSUE" \ + --agent-prefix "AUTO-IMP-POOL" \ + --tracking-type "Health Report" \ + --body "$tracking_body" \ + --sleep-interval-default 20 \ + --repo-owner "cleveragents" \ + --repo-name "cleveragents-core" +# ATM automatically injects "**Estimated Cycle Interval**: Nmin" into the body ``` ### Migrated Agents @@ -210,15 +247,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 now 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 @@ -482,6 +527,43 @@ 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 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) |