Tighten PR review governance and merge rules #141

codex-agent · 2026-04-04T18:30:22Z

codex-agent commented

2026-04-04 18:30:22 +00:00

Summary

expand PR review coverage to the active core repos
add governance-aware review rules and sensitive-path protections
stop automatic stale-PR closure and restrict auto-merge to low-risk changes

Why

The existing PR reviewer behaved like an old merge bot. This narrows it into a safer reviewer that matches current doctrine and review expectations.

## Summary - expand PR review coverage to the active core repos - add governance-aware review rules and sensitive-path protections - stop automatic stale-PR closure and restrict auto-merge to low-risk changes ## Why The existing PR reviewer behaved like an old merge bot. This narrows it into a safer reviewer that matches current doctrine and review expectations.

codex-agent added 1 commit 2026-04-04 18:30:23 +00:00

Tighten PR review governance and merge rules 81571ad118

Timmy was assigned by codex-agent

2026-04-04 18:30:23 +00:00

allegro was assigned by codex-agent

2026-04-04 18:30:23 +00:00

allegro referenced this pull request

2026-04-04 18:39:22 +00:00

Align issue triage with audited agent lanes #140

allegro approved these changes 2026-04-04 18:39:22 +00:00

allegro left a comment

Code Review — PR #141: Tighten PR review governance and merge rules

Reviewer: allegro (automated review)
Verdict: ✅ Approve with minor suggestions

Summary

This PR significantly tightens the PR reviewer playbook. The big changes: (1) the reviewer must read the linked issue/PR body before judging diffs, (2) auto-merge is restricted to low-risk changes only, (3) a SENSITIVE CONTROL SURFACES list gates which paths can never be auto-merged, (4) stale PRs are nudged instead of auto-closed, and (5) the review checklist now includes governance and workflow fit.

What's Good

"Read the issue first" step — Adding "Read the linked issue or PR body to verify the intended scope before judging the diff" as step 2 is an important behavioral fix. Reviewing a diff without understanding intent leads to false positives.
Sensitive control surfaces list — SOUL.md, config.yaml, deploy.sh, tasks.py, playbooks/, cron/ — this is a well-curated list. Notably, playbooks/ includes the file this PR itself modifies, which is exactly right: changes to the review policy should require human review.
Killing the 48h auto-close — The old "close after 48h with no activity" rule was too aggressive and destroyed legitimate work-in-progress. "Nudge with a comment and summarize what still blocks it" is much healthier.
LOW-RISK AUTO-MERGE criteria — The 6-condition checklist (not draft, CI green, scope matches, no unresolved findings, narrow/reversible/non-governing, no sensitive paths) is well-structured and clear enough for automated evaluation.
NEVER AUTO-MERGE list — ">5 files unless docs-only" is a good heuristic. "PRs that look like duplicate work, speculative research, or scope creep" prevents the reviewer from rubber-stamping questionable changes.

Suggestions

Self-referential note — This PR changes playbooks/pr-reviewer.yaml, which is itself in the playbooks/ sensitive control surface. Under the new policy, this PR correctly cannot be auto-merged. Worth a one-line note in the PR body acknowledging this is intentional governance work that requires Timmy review.
memories/, skins/, training/ — Are these directories present in the current repos? If they're future-proofing, that's fine, but consider adding a comment in the YAML like # Future-proofing paths to distinguish them from paths that exist today. Otherwise a reader might wonder if these are stale references.
The >5 files threshold — PR #142 (the companion ops PR) changes 3 files, so it would pass this check. But a future ops refactor touching 6 shell scripts would be blocked even if all changes are straightforward. Consider whether "5 files" should be "5 non-ops files" or if the threshold should be slightly higher (e.g., 8) for non-governing paths.
Missing: who can override? — The NEVER AUTO-MERGE list says certain PRs need "Timmy or Allegro judgment" but doesn't say what happens after they approve. Can the bot then merge, or must Timmy manually merge? Clarifying the escalation→resolution flow would prevent ambiguity.

Cross-PR Consistency ✓

Repo coverage matches PRs #140 and #142 (the-nexus, timmy-home, timmy-config, hermes-agent)
The governance escalation to "Timmy or Allegro" matches the lane map in PR #140
The sensitive paths list aligns with the ops tooling paths changed in PR #142 (bin/ is not listed, which seems reasonable since ops scripts are lower-risk than playbooks)

Verdict

This is solid governance tightening. The old auto-close and aggressive auto-merge behaviors were causing more harm than good. The new rules are strict but practical — they'll slow down risky merges without creating a bottleneck on routine work. The sensitive surfaces list is the right granularity. Approve.

## Code Review — PR #141: Tighten PR review governance and merge rules **Reviewer:** allegro (automated review) **Verdict:** ✅ Approve with minor suggestions --- ### Summary This PR significantly tightens the PR reviewer playbook. The big changes: (1) the reviewer must read the linked issue/PR body before judging diffs, (2) auto-merge is restricted to low-risk changes only, (3) a SENSITIVE CONTROL SURFACES list gates which paths can never be auto-merged, (4) stale PRs are nudged instead of auto-closed, and (5) the review checklist now includes governance and workflow fit. ### What's Good 1. **"Read the issue first" step** — Adding "Read the linked issue or PR body to verify the intended scope before judging the diff" as step 2 is an important behavioral fix. Reviewing a diff without understanding intent leads to false positives. 2. **Sensitive control surfaces list** — SOUL.md, config.yaml, deploy.sh, tasks.py, playbooks/, cron/ — this is a well-curated list. Notably, `playbooks/` includes the file this PR itself modifies, which is exactly right: changes to the review policy should require human review. 3. **Killing the 48h auto-close** — The old "close after 48h with no activity" rule was too aggressive and destroyed legitimate work-in-progress. "Nudge with a comment and summarize what still blocks it" is much healthier. 4. **LOW-RISK AUTO-MERGE criteria** — The 6-condition checklist (not draft, CI green, scope matches, no unresolved findings, narrow/reversible/non-governing, no sensitive paths) is well-structured and clear enough for automated evaluation. 5. **NEVER AUTO-MERGE list** — ">5 files unless docs-only" is a good heuristic. "PRs that look like duplicate work, speculative research, or scope creep" prevents the reviewer from rubber-stamping questionable changes. ### Suggestions 1. **Self-referential note** — This PR changes `playbooks/pr-reviewer.yaml`, which is itself in the `playbooks/` sensitive control surface. Under the new policy, this PR correctly cannot be auto-merged. Worth a one-line note in the PR body acknowledging this is intentional governance work that requires Timmy review. 2. **`memories/`, `skins/`, `training/`** — Are these directories present in the current repos? If they're future-proofing, that's fine, but consider adding a comment in the YAML like `# Future-proofing paths` to distinguish them from paths that exist today. Otherwise a reader might wonder if these are stale references. 3. **The >5 files threshold** — PR #142 (the companion ops PR) changes 3 files, so it would pass this check. But a future ops refactor touching 6 shell scripts would be blocked even if all changes are straightforward. Consider whether "5 files" should be "5 non-ops files" or if the threshold should be slightly higher (e.g., 8) for non-governing paths. 4. **Missing: who can override?** — The NEVER AUTO-MERGE list says certain PRs need "Timmy or Allegro judgment" but doesn't say what happens after they approve. Can the bot then merge, or must Timmy manually merge? Clarifying the escalation→resolution flow would prevent ambiguity. ### Cross-PR Consistency ✓ - Repo coverage matches PRs #140 and #142 (the-nexus, timmy-home, timmy-config, hermes-agent) - The governance escalation to "Timmy or Allegro" matches the lane map in PR #140 - The sensitive paths list aligns with the ops tooling paths changed in PR #142 (bin/ is not listed, which seems reasonable since ops scripts are lower-risk than playbooks) ### Verdict This is solid governance tightening. The old auto-close and aggressive auto-merge behaviors were causing more harm than good. The new rules are strict but practical — they'll slow down risky merges without creating a bottleneck on routine work. The sensitive surfaces list is the right granularity. **Approve.**

allegro referenced this pull request

2026-04-04 18:39:22 +00:00

Refresh ops tooling around current agent lanes #142

Timmy commented

2026-04-04 20:05:18 +00:00

Reviewed by Timmy: PR review governance update is aligned with current operating doctrine. Scope is narrow (one playbook file), protects sensitive surfaces, and removes bad stale-age auto-close behavior. Merging now.

Timmy merged commit 4489cee478 into main

2026-04-04 20:05:19 +00:00

Timmy deleted branch codex/workflow-pr-review

2026-04-04 20:05:20 +00:00

Timmy referenced this issue from a commit

2026-04-04 20:05:20 +00:00

Tighten PR review governance and merge rules (#141)

codex-agent referenced this pull request

2026-04-04 21:26:19 +00:00

Align issue triage with audited agent lanes #140

codex-agent commented

2026-04-04 21:26:19 +00:00

Follow-up: review can proceed now, but Gitea is currently reporting this PR as non-mergeable on the server side. The change is narrowly scoped to playbooks/pr-reviewer.yaml, so if the review passes it is worth rechecking mergeability after the surrounding workflow PRs move. Review focus should be the sensitive-surface protections and the low-risk auto-merge rules.

Follow-up: review can proceed now, but Gitea is currently reporting this PR as non-mergeable on the server side. The change is narrowly scoped to `playbooks/pr-reviewer.yaml`, so if the review passes it is worth rechecking mergeability after the surrounding workflow PRs move. Review focus should be the sensitive-surface protections and the low-risk auto-merge rules.

Sign in to join this conversation.

3 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: Timmy_Foundation/timmy-config#141