From 85cf48b1f945f8699f3a9dfc3217f07c05c41b20 Mon Sep 17 00:00:00 2001 From: mohamed-elkholy95 Date: Tue, 2 Jun 2026 10:52:06 -0400 Subject: [PATCH 1/2] fix(approval): fail closed when deliberation gate runs unscoped MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- src/pythinker_code/soul/approval.py | 24 +++++++++++++++++++----- tests/core/test_approval_auto.py | 13 +++++++++++++ 2 files changed, 32 insertions(+), 5 deletions(-) diff --git a/src/pythinker_code/soul/approval.py b/src/pythinker_code/soul/approval.py index 9627b5bc..0e3254ac 100644 --- a/src/pythinker_code/soul/approval.py +++ b/src/pythinker_code/soul/approval.py @@ -275,9 +275,23 @@ def deliberation_gate(self, tool_call: ToolCall) -> str | None: if reason is None: return None scope = _current_deliberation_scope.get() - context_id = scope.context_id if scope is not None else "unscoped" - generation = scope.generation if scope is not None else 0 - fingerprint = self._deliberation_fingerprint(context_id, tool_call.function.name, arguments) + if scope is None: + # Defensive fallback. The production path (PythinkerSoul._step) always binds a + # scope around step + tool-result collection, and the only caller of this gate + # (Approval.request, via a tool future created inside that scope) inherits it. + # Reaching here means a destructive call was gated with no turn-boundary signal, + # so we cannot distinguish a same-response duplicate from a deliberated re-issue. + # Fail CLOSED: keep bouncing rather than auto-approving a destructive action we + # cannot prove was deliberated. Surface it loudly — it indicates a wiring bug. + logger.warning( + "deliberation_gate reached without a deliberation scope for {tool_name}; " + "bouncing fail-closed (no turn boundary to authorize a retry)", + tool_name=tool_call.function.name, + ) + return reason + fingerprint = self._deliberation_fingerprint( + scope.context_id, tool_call.function.name, arguments + ) # One-shot keyed by (execution context, generation): the first sighting and any # same-generation duplicate are bounced; only a re-issue in a strictly LATER # generation of the same context is let through once. The context_id prefix prevents @@ -285,11 +299,11 @@ def deliberation_gate(self, tool_call: ToolCall) -> str | None: # shared via Approval.share()). prior_generation = self._state.deliberated_fingerprints.get(fingerprint) if prior_generation is not None: - if prior_generation < generation: + if prior_generation < scope.generation: del self._state.deliberated_fingerprints[fingerprint] return None return reason - self._state.deliberated_fingerprints[fingerprint] = generation + self._state.deliberated_fingerprints[fingerprint] = scope.generation return reason async def request( diff --git a/tests/core/test_approval_auto.py b/tests/core/test_approval_auto.py index 75b673c4..ad8a72a5 100644 --- a/tests/core/test_approval_auto.py +++ b/tests/core/test_approval_auto.py @@ -189,6 +189,19 @@ def test_older_generation_duplicate_destructive_call_still_bounces() -> None: assert approval.deliberation_gate(_shell_call("rm -rf build")) is not None +def test_unscoped_destructive_calls_always_bounce_fail_closed() -> None: + # No deliberation scope means no turn-boundary signal to authorize a retry. The gate + # fails CLOSED: every sighting (including identical re-issues) bounces, never auto- + # approving a destructive action it cannot prove was deliberated. Production always + # binds a scope, so reaching this path is a wiring bug, not an expected flow. + approval = Approval(state=ApprovalState(auto=True, auto_deliberate=True)) + assert approval.deliberation_gate(_shell_call("rm -rf build")) is not None + assert approval.deliberation_gate(_shell_call("rm -rf build")) is not None + assert approval.deliberation_gate(_shell_call("rm -rf build")) is not None + # Fail-closed bounce must not accumulate state for the unscoped path. + assert approval._state.deliberated_fingerprints == {} + + def test_deliberation_gate_conditions() -> None: """The gate fires only when the feature is on, we would otherwise auto-approve (auto OR yolo), and the command is destructive.""" From 056881c559a80dea5ad3f54322eb0bbb070ff3f4 Mon Sep 17 00:00:00 2001 From: mohamed-elkholy95 Date: Tue, 2 Jun 2026 11:57:38 -0400 Subject: [PATCH 2/2] docs(changelog): note fail-closed deliberation gate under Unreleased 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. --- CHANGELOG.md | 4 +++- docs/en/release-notes/changelog.md | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2cdcdf80..9b61ca85 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,7 +23,9 @@ GitHub Releases page; `0.8.0` is the new starting line. - **Auto-mode destructive actions deliberate per turn and context.** Auto-deliberation now scopes destructive-command one-shots to the active execution context and LLM generation, so duplicate destructive calls in one response keep bouncing while later deliberate retries - and isolated subagent calls are handled independently. + and isolated subagent calls are handled independently. If a destructive action is ever + evaluated without that turn context, the gate now fails closed — it keeps deliberating + rather than auto-approving the action. - **Release packaging keeps SDK/core pins in lockstep.** The SDK's `pythinker-core` dependency is now updated by release automation and checked by CI/release validation, preventing no-sources binary builds from resolving against a stale core pin. diff --git a/docs/en/release-notes/changelog.md b/docs/en/release-notes/changelog.md index 3d5c2e44..4d9c62a0 100644 --- a/docs/en/release-notes/changelog.md +++ b/docs/en/release-notes/changelog.md @@ -25,7 +25,9 @@ GitHub Releases page; `0.8.0` is the new starting line. - **Auto-mode destructive actions deliberate per turn and context.** Auto-deliberation now scopes destructive-command one-shots to the active execution context and LLM generation, so duplicate destructive calls in one response keep bouncing while later deliberate retries - and isolated subagent calls are handled independently. + and isolated subagent calls are handled independently. If a destructive action is ever + evaluated without that turn context, the gate now fails closed — it keeps deliberating + rather than auto-approving the action. - **Release packaging keeps SDK/core pins in lockstep.** The SDK's `pythinker-core` dependency is now updated by release automation and checked by CI/release validation, preventing no-sources binary builds from resolving against a stale core pin.