Skip to content

fix(approval): fail closed when deliberation gate runs unscoped - #57

Merged
elkaix merged 3 commits into
mainfrom
fix/deliberation-gate-fail-closed-unscoped
Jun 2, 2026
Merged

fix(approval): fail closed when deliberation gate runs unscoped#57
elkaix merged 3 commits into
mainfrom
fix/deliberation-gate-fail-closed-unscoped

Conversation

@elkaix

@elkaix elkaix commented Jun 2, 2026

Copy link
Copy Markdown
Member

Summary

Follow-up to #56 (squash-merged). That PR scoped the auto-deliberation one-shot to (execution context, generation) but left the scope-less fallback in Approval.deliberation_gate storing generation = 0, where an identical destructive retry could never release the one-shot — and, more importantly, the prior design would have auto-approved an unscoped destructive retry, i.e. fail-open on a security gate.

This change makes the scope-less branch fail closed and explicit instead of silently degrading.

What changed

  • When no deliberation_scope is bound, deliberation_gate now always bounces a destructive call (never auto-approves one it cannot prove was deliberated), logs a warning (reaching this path signals a wiring bug), and no longer pollutes deliberated_fingerprints with a dead unscoped::…: 0 entry.
  • Added a regression test pinning the fail-closed contract (test_unscoped_destructive_calls_always_bounce_fail_closed).

Why this is the robust choice

The scope-less branch is unreachable in production: every real-tool path enters via pythinker_core.step() wrapped in deliberation_scope, and tool futures inherit it through asyncio.create_task's context copy; the only other step() callers (btw, compaction, blind-advisor) use empty/deny-all toolsets, so request() cannot fire there. Given that, the safe default for a destructive-action gate with no turn-boundary signal is fail-closed, and the right fix is to make the contract explicit rather than reopen a fail-open hole. A hard assert/raise on this runtime approval path would be more fragile, not more robust.

Verification

  • tests/core/test_approval_auto.py — passing (incl. new regression test).
  • ruff check + ruff format --check — clean.
  • Full pythinker-code suite was green on the source commit (4242 passed).

Single commit, cherry-picked clean onto current main.

Summary by CodeRabbit

  • Bug Fixes

    • Strengthened approval gating so destructive actions without the active turn context now fail-closed and continue deliberation rather than proceeding.
  • Tests

    • Added test ensuring unscoped destructive calls consistently bounce and do not record deliberation fingerprints.
  • Documentation

    • Clarified release notes explaining per-turn/context scoping for destructive one-shots and the fail-closed behavior when context is missing.

When no deliberation scope is bound, deliberation_gate stored
generation 0 and never released the one-shot, and the prior design
would have auto-approved an unscoped destructive retry — fail-open on
a security gate. The scope-less branch is unreachable in production
(every tool runs via pythinker_core.step inside deliberation_scope,
inherited through asyncio.create_task's context copy; other step
callers use empty/deny-all toolsets), so the robust choice is to make
the fallback explicit rather than silently degrade.

Reaching it now fails CLOSED: always bounce a destructive call that
cannot be proven deliberated, log a warning (it signals a wiring bug),
and stop polluting deliberated_fingerprints. Add a regression test
pinning the fail-closed contract.

Refs: .pythinker/reports/deep-code-scan-deliberation-turn-boundary.md
@coderabbitai

coderabbitai Bot commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 4203f246-685e-483b-9153-bd810ed3dc41

📥 Commits

Reviewing files that changed from the base of the PR and between 85cf48b and 056881c.

📒 Files selected for processing (2)
  • CHANGELOG.md
  • docs/en/release-notes/changelog.md

📝 Walkthrough

Walkthrough

Approval.deliberation_gate now fails closed when no deliberation scope is bound: it logs a warning and returns the destructive reason to force deliberation. A new test verifies unscoped destructive calls always bounce and leave no cached fingerprint state; changelogs/docs were updated.

Changes

Fail-Closed Behavior for Unscoped Destructive Calls

Layer / File(s) Summary
Fail-closed scope check in deliberation_gate
src/pythinker_code/soul/approval.py
deliberation_gate now branches on scope is None: logs a warning and returns the destructive reason directly (fail-closed) to force deliberation bounce, instead of generating a fingerprint via fallback "unscoped" context. Scoped calls continue one-shot keying via scope.context_id and generation.
Test coverage for unscoped fail-closed behavior
tests/core/test_approval_auto.py
New test test_unscoped_destructive_calls_always_bounce_fail_closed asserts that destructive commands bounce repeatedly when no deliberation scope is active and that approval._state.deliberated_fingerprints remains empty for the unscoped path.
Changelog and release notes
CHANGELOG.md, docs/en/release-notes/changelog.md
Update Unreleased notes to state that destructive one-shots are scoped to execution context and generation and that evaluating destructive actions without a turn context fails closed (continues deliberation).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested labels

bug

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 60.00% which is insufficient. The required threshold is 70.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title follows conventional commits format with type 'fix', scope 'approval', and a clear description of the change.
Description check ✅ Passed The description covers all required template sections: related issue (#56), detailed explanation of changes, and verification results.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/deliberation-gate-fail-closed-unscoped

Comment @coderabbitai help to get the list of available commands and usage tips.

elkaix added 2 commits June 2, 2026 11:52
Augments the existing auto-deliberation entry to record that the
gate fails closed when a destructive action is evaluated without a
turn context. Regenerated the docs copy via docs/scripts/sync-changelog.mjs.
Satisfies the changelog-entry-required check on this PR.
@elkaix
elkaix merged commit 9addb0e into main Jun 2, 2026
23 checks passed
@elkaix
elkaix deleted the fix/deliberation-gate-fail-closed-unscoped branch June 2, 2026 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant