Skip to content

fix(agent): enforce hard guardrail manual review - #1090

Merged
JSONbored merged 2 commits into
mainfrom
codex/fix-auto-approval-and-closure-of-guarded-prs
Jun 23, 2026
Merged

fix(agent): enforce hard guardrail manual review#1090
JSONbored merged 2 commits into
mainfrom
codex/fix-auto-approval-and-closure-of-guarded-prs

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Motivation

  • Hard guardrails must force human review for sensitive paths and must not be bypassable by automated planners; a recent change removed those checks, allowing guarded PRs to be auto-approved or auto-closed.

Description

  • Require !guardrailHit before emitting an approve action and before emitting a contributor close disposition in src/settings/agent-actions.ts, so guarded PRs are withheld from automated approve/close/merge flows.
  • Update in-file comments and review-reason text to reflect that guardrails suppress auto-approve/auto-merge/auto-close while preserving the reversible labeling behavior (needs-human-review).
  • Modify test/unit/agent-actions.test.ts to assert guarded PRs are not auto-approved nor auto-closed.

Testing

  • Ran the focused unit tests with npm test -- --run test/unit/agent-actions.test.ts --reporter=dot, which passed (37 tests).
  • Ran the full test suite with npm test -- --reporter=dot, which exhibited unrelated timeouts/failures in long-running MCP/queue/remediation-route tests (several tests timed out and 8 tests failed), while the targeted agent-actions tests remained green.
  • Verified the planner now omits approve/close plans for guardrail-hit inputs via the updated unit tests.

Codex Task

@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@ghost

ghost commented Jun 23, 2026

Copy link
Copy Markdown

Tip

🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩🟩

✅ Gittensory review — safe to merge

2 files · 2 AI reviewers · no blockers · readiness 93/100 · CI green

✅ Approved — safe to merge

Review summary
The changes tighten guardrail handling by ensuring that any PR hitting a hard guardrail never triggers auto‑approve, auto‑merge, or auto‑close actions. A new guardrail check is added to the approve block and the close condition now requires the PR not to be guardrail‑hit. The label logic already distinguishes guarded PRs, and the comment updates reflect the new behavior. The removal of the now‑unused `wouldMergeButGuarded` variable is safe because its only previous use was in the approve reason, which is now unconditional. All related unit tests have been updated to assert the absence of approve/close on guarded PRs, and the existing test suite continues to cover the other decision paths. No type errors or missing imports are introduced, and the logic remains consistent with the documented policy.

Blockers

  • In `src/settings/agent-actions.ts`, the `willClose` variable is updated to include `!guardrailHit` in its condition. However, the comment above it does not reflect this change, stating 'A CONTRIBUTOR PR is CLOSED one-shot when it isn't review-good OR it conflicts, unless a hard guardrail requires manual review.' This comment should be updated to accurately reflect the new logic.
  • In `test/unit/agent-actions.test.ts`, the test case 'DOES auto-close a failing PR on a guarded path (the guardrail withholds GOOD PRs for review — it never rescues a bad one)' is updated to 'does NOT auto-close a failing PR on a guarded path'. The comment above this test case should be updated to reflect the new behavior, stating 'A failing PR on a guarded path is not auto-closed, as it requires manual review.'

Nits

  • The comment on the guardrail handling still mentions "would‑approve/would‑merge/would‑close" even though the code now explicitly checks guardrailHit in each branch; consider updating the wording for clarity.
  • The label reason for guardrail hits includes "owner safety review" but does not mention CI status; while not a bug, aligning the phrasing with other label reasons could improve consistency.
  • Update the guardrail comment to explicitly reference the new `!guardrailHit` checks in the approve and close branches for clearer documentation.
  • Consider extracting the guardrail‑hit check into a helper function (e.g., `isGuarded(input)`) to avoid repetition and improve readability.
  • In `src/settings/agent-actions.ts`, the comment 'A CONTRIBUTOR PR is CLOSED one-shot when it isn't review-good OR it conflicts, unless a hard guardrail requires manual review.' is somewhat verbose and could be simplified for clarity.
  • In `src/settings/agent-actions.ts`, the comment 'A good PR on a guarded path → held for the owner's manual safety review (NOT auto-approved, auto-merged, or auto-closed).' could be split into two separate comments for better readability.
  • In `test/unit/agent-actions.test.ts`, the test case 'APPROVES a passing PR on a guarded path (pre-cleared for the owner) but never auto-merges it' is updated to 'does NOT approve or auto-merge a passing PR on a guarded path'. The test case name could be updated to better reflect the new behavior, such as 'does not approve or auto-merge a passing PR on a guarded path'.
  • In `test/unit/agent-actions.test.ts`, the test case 'DOES auto-close a failing PR on a guarded path (the guardrail withholds GOOD PRs for review — it never rescues a bad one)' is updated to 'does NOT auto-close a failing PR on a guarded path'. The test case name could be updated to better reflect the new behavior, such as 'does not auto-close a failing PR on a guarded path'.
  • In `src/settings/agent-actions.ts`, consider adding a comment above the `guardrailHit` variable to explain its purpose and how it is used in the function.
  • In `src/settings/agent-actions.ts`, the `willClose` variable could be renamed to `shouldClose` for better clarity, as it determines whether a PR should be closed based on the given conditions.
  • In `test/unit/agent-actions.test.ts`, consider adding a comment above the test cases for guarded PRs to explain the purpose of these tests and the expected behavior.
  • In `test/unit/agent-actions.test.ts`, the test case 'does NOT approve or auto-merge a passing PR on a guarded path' could be split into two separate test cases for better clarity, one for approving and one for auto-merging.
Signal Result Evidence
Code review ✅ No blockers 2 reviewers, synthesized
Linked issue ⚠️ Missing No linked issue or no-issue rationale found.
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Review load ✅ 20/20 Readiness component derived from cached public PR metadata and labels; size label size:M.
Validation evidence ✅ 25/25 PR body includes validation/test evidence.
Open PR queue ✅ 10/10 0 open PR(s), 0 likely reviewable.
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 84 PR(s), 254 issue(s).
Gate result ✅ Passing No configured blocker found.
Nits — 1 non-blocking
  • No linked issue detected — If this PR is intended to solve an issue, link it explicitly in the PR body.
Review context
  • Author: JSONbored
  • Role context: owner (maintainer lane)
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: Python, TypeScript, JavaScript, Ruby, Go, Kotlin, MDX, Shell
  • Official Gittensor activity: 84 PR(s), 254 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Treat this as maintainer-lane context rather than normal contributor-lane activity.
  • Explain no-issue PR.
  • Link the issue being solved, or explicitly explain why this is a no-issue PR.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Review load = cached public PR metadata such as size labels, changed paths, and preflight status.
  • Open PR queue = repo-wide review pressure; it is not a PR quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.
Review details

Generated from public PR metadata and the diff. Advisory only; deterministic signals remain authoritative.

The changes tighten guardrail handling by ensuring that any PR hitting a hard guardrail never triggers auto‑approve, auto‑merge, or auto‑close actions. A new guardrail check is added to the approve block and the close condition now requires the PR not to be guardrail‑hit. The label logic already distinguishes guarded PRs, and the comment updates reflect the new behavior. The removal of the now‑unused `wouldMergeButGuarded` variable is safe because its only previous use was in the approve reason, which is now unconditional. All related unit tests have been updated to assert the absence of approve/close on guarded PRs, and the existing test suite continues to cover the other decision paths. No type errors or missing imports are introduced, and the logic remains consistent with the documented policy.

Blockers

  • In `src/settings/agent-actions.ts`, the `willClose` variable is updated to include `!guardrailHit` in its condition. However, the comment above it does not reflect this change, stating 'A CONTRIBUTOR PR is CLOSED one-shot when it isn't review-good OR it conflicts, unless a hard guardrail requires manual review.' This comment should be updated to accurately reflect the new logic.
  • In `test/unit/agent-actions.test.ts`, the test case 'DOES auto-close a failing PR on a guarded path (the guardrail withholds GOOD PRs for review — it never rescues a bad one)' is updated to 'does NOT auto-close a failing PR on a guarded path'. The comment above this test case should be updated to reflect the new behavior, stating 'A failing PR on a guarded path is not auto-closed, as it requires manual review.'

Nits

  • The comment on the guardrail handling still mentions "would‑approve/would‑merge/would‑close" even though the code now explicitly checks guardrailHit in each branch; consider updating the wording for clarity.
  • The label reason for guardrail hits includes "owner safety review" but does not mention CI status; while not a bug, aligning the phrasing with other label reasons could improve consistency.
  • Update the guardrail comment to explicitly reference the new `!guardrailHit` checks in the approve and close branches for clearer documentation.
  • Consider extracting the guardrail‑hit check into a helper function (e.g., `isGuarded(input)`) to avoid repetition and improve readability.
  • In `src/settings/agent-actions.ts`, the comment 'A CONTRIBUTOR PR is CLOSED one-shot when it isn't review-good OR it conflicts, unless a hard guardrail requires manual review.' is somewhat verbose and could be simplified for clarity.
  • In `src/settings/agent-actions.ts`, the comment 'A good PR on a guarded path → held for the owner's manual safety review (NOT auto-approved, auto-merged, or auto-closed).' could be split into two separate comments for better readability.
  • In `test/unit/agent-actions.test.ts`, the test case 'APPROVES a passing PR on a guarded path (pre-cleared for the owner) but never auto-merges it' is updated to 'does NOT approve or auto-merge a passing PR on a guarded path'. The test case name could be updated to better reflect the new behavior, such as 'does not approve or auto-merge a passing PR on a guarded path'.
  • In `test/unit/agent-actions.test.ts`, the test case 'DOES auto-close a failing PR on a guarded path (the guardrail withholds GOOD PRs for review — it never rescues a bad one)' is updated to 'does NOT auto-close a failing PR on a guarded path'. The test case name could be updated to better reflect the new behavior, such as 'does not auto-close a failing PR on a guarded path'.
  • In `src/settings/agent-actions.ts`, consider adding a comment above the `guardrailHit` variable to explain its purpose and how it is used in the function.
  • In `src/settings/agent-actions.ts`, the `willClose` variable could be renamed to `shouldClose` for better clarity, as it determines whether a PR should be closed based on the given conditions.
  • In `test/unit/agent-actions.test.ts`, conside

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers.

  • Re-run Gittensory review

@ghost ghost added gittensory:reviewed gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. labels Jun 23, 2026
ghost
ghost previously approved these changes Jun 23, 2026

@ghost ghost left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gittensory approves — the gate is satisfied and CI is green.

@JSONbored JSONbored self-assigned this Jun 23, 2026
Resolve agent-actions.ts: keep main's #no-request-changes policy AND apply
#1090's hard-guardrail enforcement (approve + close now gated on !guardrailHit).
@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Jun 23, 2026

Copy link
Copy Markdown

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Preview URL Updated (UTC)
✅ Deployment successful!
View logs
gittensory-ui b43179c Commit Preview URL

Branch Preview URL
Jun 23 2026, 06:33 PM

@codecov

codecov Bot commented Jun 23, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.77%. Comparing base (f759e88) to head (b43179c).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1090      +/-   ##
==========================================
- Coverage   94.77%   94.77%   -0.01%     
==========================================
  Files         153      153              
  Lines       18552    18550       -2     
  Branches     6707     6705       -2     
==========================================
- Hits        17582    17580       -2     
  Misses        418      418              
  Partials      552      552              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@JSONbored
JSONbored merged commit b34fddd into main Jun 23, 2026
17 checks passed
@JSONbored
JSONbored deleted the codex/fix-auto-approval-and-closure-of-guarded-prs branch June 23, 2026 18:35
JSONbored added a commit that referenced this pull request Jun 23, 2026
…(guard blocks merge, not rejection) (#1106)

#1090 gated willClose on !guardrailHit, so a rejected CONTRIBUTOR PR touching a guarded path was HELD instead
of closed (e.g. gittensory #1098: rejected, touches src/review|services|signals → stayed open with a
'closing' message). That contradicts the spec: 'guarded + would-merge → hold; otherwise → closure.'

The hard-guardrail exists to stop auto-MERGING/APPROVING crucial-path changes without owner review (canMerge
+ approve still gate on !guardrailHit). It must NOT keep a rejected PR open — closing rejects bad changes and
merges nothing, so it is always safe. willClose no longer checks guardrailHit; owner/automation PRs are still
never closed (isContributor gates it); GOOD-but-guarded PRs still fall through to the owner (held).

Test flipped: a failing contributor PR on a guarded path now CLOSES.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant