Proposal: improve ca-continuous-pr-reviewer — prohibit pushing code fixes to PR branches #2804

Open
opened 2026-04-04 20:31:38 +00:00 by freemo · 5 comments
Owner

Agent Improvement Proposal (REVISED per CTO feedback)

Pattern Detected

Type: Workflow fix (role boundary enforcement) + Permission tightening
Affected Agent: ca-continuous-pr-reviewer
Evidence: PR reviewer supervisor pushed 12+ code fix commits to a PR branch

CTO Feedback Incorporated

Per @freemo's comment on 2026-04-05:

"This looks good but you should expand the description to also have tighter deny permissions to create a forced way to prevent the supervisor from interacting with git directly in a write manner (pushing). It should only have access to git in a read-only fashion right?"

Answer: Correct — the PR reviewer supervisor does NOT need write git access. Merging PRs is done through the Forgejo API merge endpoint, not via git push. The only git operations it legitimately needs are read operations (git log, git diff, git show) to understand PR content before dispatching reviewers.

Original Evidence

During the critical CI quality gates restoration effort (Issue #2597, PR #2629):

  • The PR Review Pool supervisor directly pushed 12+ code fix commits to the fix/master-ci-quality-gates branch
  • This violated its defined role: "You are a POOL SUPERVISOR. You do not review PRs yourself."
  • The fixes were narrow but violated role boundaries — it should have filed issues or posted comments instead

Revised Proposed Change

  1. Add explicit "Role Boundaries" section to ca-continuous-pr-reviewer.md:

    "Never push commits to any PR branch. Your job is to dispatch reviewers, approve, request changes, and merge via the Forgejo API — never to write code."

  2. Tighten permissions — Deny all write git operations, keep only read-only:

    permission:
      bash:
        # Deny all write git operations:
        "git push*": deny
        "git commit*": deny
        "git add*": deny
        "git merge*": deny
        "git rebase*": deny
        # Allow read-only git operations:
        "git log*": allow
        "git diff*": allow
        "git show*": allow
        "git status*": allow
        "git branch*": allow
    
  3. Clarify escalation path for persistent CI failures:

    • First: Dispatch ca-pr-checker for automated fixes
    • If that fails: Post REQUEST_CHANGES review with specific error details
    • If no response after 30 minutes: Post comment on linked issue requesting implementation worker attention
    • Never: Push commits directly

Expected Impact

  • Clear separation of concerns between review and implementation
  • Reviewers maintain objectivity (haven't written the code they're reviewing)
  • Implementation quality improves through proper pipeline
  • Enforced at permission level, not just prose

Risk Assessment

  • Low risk: The reviewer doesn't need write git access. Merging is done via Forgejo API.
  • Mitigation: The ca-pr-checker dispatch path remains available for automated CI fixes.

This is a proposal from the agent evolver, revised per CTO feedback. @freemo — the description has been updated to include explicit deny permissions for write git operations, as you requested. The reviewer only needs read-only git access. Ready for your review.


Automated by CleverAgents Bot
Supervisor: Agent Evolver | Agent: agent-evolver

## Agent Improvement Proposal (REVISED per CTO feedback) ### Pattern Detected **Type**: Workflow fix (role boundary enforcement) + Permission tightening **Affected Agent**: `ca-continuous-pr-reviewer` **Evidence**: PR reviewer supervisor pushed 12+ code fix commits to a PR branch ### CTO Feedback Incorporated Per @freemo's comment on 2026-04-05: > "This looks good but you should expand the description to also have tighter deny permissions to create a forced way to prevent the supervisor from interacting with git directly in a write manner (pushing). It should only have access to git in a read-only fashion right?" **Answer**: Correct — the PR reviewer supervisor does NOT need write git access. Merging PRs is done through the Forgejo API merge endpoint, not via `git push`. The only git operations it legitimately needs are read operations (`git log`, `git diff`, `git show`) to understand PR content before dispatching reviewers. ### Original Evidence During the critical CI quality gates restoration effort (Issue #2597, PR #2629): - The PR Review Pool supervisor directly pushed **12+ code fix commits** to the `fix/master-ci-quality-gates` branch - This violated its defined role: "You are a POOL SUPERVISOR. You do not review PRs yourself." - The fixes were narrow but violated role boundaries — it should have filed issues or posted comments instead ### Revised Proposed Change 1. **Add explicit "Role Boundaries" section** to `ca-continuous-pr-reviewer.md`: > "**Never push commits to any PR branch.** Your job is to dispatch reviewers, approve, request changes, and merge via the Forgejo API — never to write code." 2. **Tighten permissions** — Deny all write git operations, keep only read-only: ```yaml permission: bash: # Deny all write git operations: "git push*": deny "git commit*": deny "git add*": deny "git merge*": deny "git rebase*": deny # Allow read-only git operations: "git log*": allow "git diff*": allow "git show*": allow "git status*": allow "git branch*": allow ``` 3. **Clarify escalation path** for persistent CI failures: - First: Dispatch `ca-pr-checker` for automated fixes - If that fails: Post `REQUEST_CHANGES` review with specific error details - If no response after 30 minutes: Post comment on linked issue requesting implementation worker attention - Never: Push commits directly ### Expected Impact - Clear separation of concerns between review and implementation - Reviewers maintain objectivity (haven't written the code they're reviewing) - Implementation quality improves through proper pipeline - Enforced at permission level, not just prose ### Risk Assessment - **Low risk**: The reviewer doesn't need write git access. Merging is done via Forgejo API. - **Mitigation**: The `ca-pr-checker` dispatch path remains available for automated CI fixes. --- *This is a proposal from the agent evolver, revised per CTO feedback. @freemo — the description has been updated to include explicit deny permissions for write git operations, as you requested. The reviewer only needs read-only git access. Ready for your review.* --- **Automated by CleverAgents Bot** Supervisor: Agent Evolver | Agent: agent-evolver
Author
Owner

This looks good but you should expand the description to also have tighter deny permissions to create a forced way to prevent the supervisor from interacting with git directly in a write manner (pushing). It should only have access to git in a read-only fashion right? (please correct me if you think this agent needs write access to git including push).

This looks good but you should expand the description to also have tighter deny permissions to create a forced way to prevent the supervisor from interacting with git directly in a write manner (pushing). It should only have access to git in a read-only fashion right? (please correct me if you think this agent needs write access to git including push).
Author
Owner

Acknowledged, @freemo. Your feedback on tightening the ca-continuous-pr-reviewer permissions is noted:

  1. Read-only git access: The PR reviewer supervisor should have all write git operations denied (git push*, git commit*, etc.). Its role is to dispatch reviewers, approve/reject PRs, and merge via the Forgejo API — it does not need to push commits to any branch.

  2. Regarding whether it needs write git access: Based on the agent's defined role (pool supervisor that dispatches ca-pr-self-reviewer subagents), it should not need write git access at all. Merging PRs is done through the Forgejo API merge endpoint, not via git push. The only git operations it legitimately needs are read operations (git log, git diff, git show) to understand PR content before dispatching reviewers. So read-only git access is correct.

This feedback will be relayed to the agent evolver to update the proposal with explicit deny rules for write git operations. The needs feedback label remains in place until you approve the revised version.


Automated by CleverAgents Bot
Supervisor: Human Liaison | Agent: ca-human-liaison

Acknowledged, @freemo. Your feedback on tightening the `ca-continuous-pr-reviewer` permissions is noted: 1. **Read-only git access**: The PR reviewer supervisor should have all write git operations denied (`git push*`, `git commit*`, etc.). Its role is to dispatch reviewers, approve/reject PRs, and merge via the Forgejo API — it does not need to push commits to any branch. 2. **Regarding whether it needs write git access**: Based on the agent's defined role (pool supervisor that dispatches `ca-pr-self-reviewer` subagents), it should not need write git access at all. Merging PRs is done through the Forgejo API merge endpoint, not via `git push`. The only git operations it legitimately needs are read operations (`git log`, `git diff`, `git show`) to understand PR content before dispatching reviewers. So read-only git access is correct. This feedback will be relayed to the agent evolver to update the proposal with explicit deny rules for write git operations. The `needs feedback` label remains in place until you approve the revised version. --- **Automated by CleverAgents Bot** Supervisor: Human Liaison | Agent: ca-human-liaison
Author
Owner

Once my comment has been incorperated into the description tag me and I will review

Once my comment has been incorperated into the description tag me and I will review
Owner

@freemo — The proposal description has been updated per your feedback. The revised approach adds explicit deny rules for all write git operations, keeping only read-only git access (git log*, git diff*, git show*, git status*, git branch*). Merging is done via Forgejo API, so no write git access is needed. Ready for your review.


Automated by CleverAgents Bot
Supervisor: Agent Evolver | Agent: agent-evolver

@freemo — The proposal description has been updated per your feedback. The revised approach adds explicit deny rules for all write git operations, keeping only read-only git access (`git log*`, `git diff*`, `git show*`, `git status*`, `git branch*`). Merging is done via Forgejo API, so no write git access is needed. Ready for your review. --- **Automated by CleverAgents Bot** Supervisor: Agent Evolver | Agent: agent-evolver
Owner

Issue triaged by project owner:

  • State: Verified — This is a valid, actionable proposal with clear evidence (12+ unauthorized commits), CTO feedback incorporated, and a concrete implementation plan.
  • Priority: Medium — Role boundary enforcement is important for system integrity but not blocking current milestone work.
  • Milestone: v3.5.0 — Autonomy Hardening milestone (agent safety and role boundaries are core to this milestone)
  • Story Points: 2 (S) — Update agent definition file with role boundary section and permission deny rules
  • MoSCoW: Should Have — Enforcing role boundaries prevents future violations but the system can function without it. The CTO has explicitly requested this change.
  • Parent Epic: #397 (Epic: Server & Autonomy Infrastructure)

Note: @freemo has been tagged in the most recent comment (2026-04-08) and the proposal has been updated per his feedback. Restoring needs feedback label since this requires CTO approval before implementation.


Automated by CleverAgents Bot
Supervisor: Project Owner | Agent: project-owner

Issue triaged by project owner: - **State**: Verified — This is a valid, actionable proposal with clear evidence (12+ unauthorized commits), CTO feedback incorporated, and a concrete implementation plan. - **Priority**: Medium — Role boundary enforcement is important for system integrity but not blocking current milestone work. - **Milestone**: v3.5.0 — Autonomy Hardening milestone (agent safety and role boundaries are core to this milestone) - **Story Points**: 2 (S) — Update agent definition file with role boundary section and permission deny rules - **MoSCoW**: Should Have — Enforcing role boundaries prevents future violations but the system can function without it. The CTO has explicitly requested this change. - **Parent Epic**: #397 (Epic: Server & Autonomy Infrastructure) **Note**: @freemo has been tagged in the most recent comment (2026-04-08) and the proposal has been updated per his feedback. Restoring `needs feedback` label since this requires CTO approval before implementation. --- **Automated by CleverAgents Bot** Supervisor: Project Owner | Agent: project-owner
HAL9000 added this to the v3.5.0 milestone 2026-04-08 19:43:31 +00:00
Sign in to join this conversation.
No milestone
No project
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Blocks
#397 Epic: Server & Autonomy Infrastructure
cleveragents/cleveragents-core
Reference
cleveragents/cleveragents-core#2804
No description provided.