fix(policy): keep approved chunk when a mechanistic denial resubmits its endpoint

[laitingsheng]

Summary

A mechanistic denial flush for an endpoint already covered by an auto-approved mechanistic chunk flipped that chunk from approved to rejected with no human action, corrupting the governance ledger: the self-reject path never un-merges the rule, so the endpoint stayed enforced while the ledger reported it revoked (and the chunk became permanently stuck, since dedup resubmits land on a rejected row and auto-approval only fires from pending). The root cause is id aliasing — put_draft_chunk's dedup upsert returns the existing approved row's id, which the self-reject scan then matched against itself.

The same ledger/enforcement mismatch was also reachable through a concurrent path: the self-reject read a chunk, confirmed it was pending, then issued an unconditional status update, so an approval that committed between the read and the write flipped the approved chunk to rejected. That window is now closed by making the pending -> rejected transition an atomic compare-and-set in the policy store.

Related Issue

Fixes #2165
Refs NVIDIA/NemoClaw#6329

Changes

• self_reject_mechanistic_if_already_covered now only acts on a still-pending effective chunk: it re-fetches the chunk the dedup upsert resolved to and returns early unless its status is pending, so a dedup hit that aliases onto an already-decided (approved) row can never flip it.
• The covering-chunk scan excludes the incoming id (c.id != new_chunk_id), so an aliased row cannot match itself as its own "covering approved chunk".
• New conditionally_reject_draft_chunk store method performs the pending -> rejected transition as an atomic compare-and-set: the status = 'pending' predicate rides the final write and the call reports whether a row changed. Zero changed rows is a benign no-op, meaning another operation already decided the chunk. Implemented for both SQLite and PostgreSQL. The self-reject path now commits through it, so an approval racing the reject can no longer clobber an approved row; the pending pre-read and the self-exclusion stay as defense-in-depth.
• Added regression test resubmitted_mechanistic_endpoint_keeps_approved_chunk: auto-approve a mechanistic endpoint, resubmit the same endpoint mechanistically, and assert the chunk stays approved, hit_count increments, no rejection_reason is set, and the rule remains merged in the active policy.
• Added persistence-level regression tests for the compare-and-set: conditionally_reject_transitions_pending_chunk (a pending chunk is rejected), conditionally_reject_leaves_approved_chunk_untouched (an approved row cannot be conditionally rejected), and conditionally_reject_loses_race_to_approval (an approval that commits before the reject wins).
• Documented the mechanistic dedup / self-reject invariant in architecture/security-policy.md (Policy Advisor pipeline).

Testing

• mise run pre-commit passes
• Unit tests added/updated
• E2E tests added/updated (if applicable)

Checklist

• Follows Conventional Commits
• Commits are signed off (DCO)
• Architecture docs updated (if applicable)

[TaylorMutch]

The deterministic self-aliasing fix and handler-level regression test are sound, but the new pending-status guard does not yet guarantee the invariant documented by this PR.

In self_reject_mechanistic_if_already_covered, the code reads the chunk and confirms that it is pending, then later calls the existing unconditional update_draft_chunk_status(..., "rejected", ...). An approval can race between those operations, allowing an approved chunk to be changed to rejected while its rule remains merged. That recreates the same ledger/enforcement mismatch through a concurrent path.

Please make pending -> rejected an atomic compare-and-set in the policy store and implement it for both SQLite and PostgreSQL. The final database write should include status = 'pending' in its predicate and return whether a row changed; zero changed rows should be treated as a benign no-op because another operation already decided the chunk. Since the persisted payload mirrors the status and decision metadata, construct and write that payload transactionally or otherwise retain the status predicate on the final update.

Please also add a persistence-level regression test proving that an approved row cannot be conditionally rejected. Keep the c.id != new_chunk_id exclusion as defense-in-depth.

[johntmyers]

gator-agent

PR Review Status

Validation: This is a project-valid, focused fix for the confirmed policy-governance defect tracked in #2165.
Head SHA: a31e0417fc4be27a37bb92e8e0b87ef9219a947d

Review findings:

• Blocking: the new pending guard in self_reject_mechanistic_if_already_covered is a separate read from the later unconditional status update. An approval can merge the rule and set the chunk to approved between those operations; the self-reject path can then overwrite it to rejected without unmerging, recreating the ledger/enforcement mismatch this PR fixes. Both production backends have this race: crates/openshell-server/src/persistence/sqlite.rs:742 and crates/openshell-server/src/persistence/postgres.rs:720 update by ID without an expected-status predicate.
• Please make the pending → rejected transition atomic in the store API and both backends (for example, an update with AND status = 'pending' that returns whether it won). Use that conditional transition in the automated rejection paths, treat a lost race as benign rather than logging rejection success, and handle the pending branch of human rejection without overwriting a concurrent approval.
• Add persistence coverage for SQLite and PostgreSQL showing that an expected-pending transition cannot overwrite approved, plus a controlled approval/self-reject interleaving regression. The current sequential handler test covers the deterministic ID-aliasing fix but not this race.

Thanks @TaylorMutch. I checked the atomic-transition concern you raised on #2165 against the self-reject, supersede, direct approval/rejection, and bulk approval call paths, as well as both production persistence backends; the concern is confirmed and remains blocking on this head.

What looks good: excluding new_chunk_id fixes the deterministic self-aliasing path, the focused regression checks status/hit count/rejection reason/active policy, and the architecture note documents the intended invariant.

Docs: no Fern update is needed because this restores existing internal policy behavior; the relevant architecture documentation is updated.

Next state: gator:in-review

[github-actions]

Label test:e2e applied for a48dcc4. Open the existing run and click Re-run all jobs to execute with the label set. The run will execute the standard E2E suite after building the required gateway and supervisor images once. The matching required CI gate status on this PR will flip green automatically once the run finishes.

[johntmyers]

gator-agent

PR Review Status

Validation: This remains a project-valid, focused fix for the confirmed policy-governance defect tracked in #2165.
Head SHA: a48dcc4febf6fdacf400c61fffb9448c8932cad9

Review findings:

• No blocking findings remain. The new conditional reject writes status, decision metadata, and timestamp together with a final status = 'pending' predicate in both SQLite and PostgreSQL; a concurrent approval that wins first now produces a benign zero-row result.
• The self-ID exclusion remains in place, and the regression coverage checks approved-row preservation, rejection metadata, and the approval-before-reject ordering.
• Non-blocking follow-up: the pre-existing supersede_other_pending_chunks_for_endpoint path still uses a scan followed by an unconditional reject and could reuse the new compare-and-set separately. This is adjacent existing behavior, not introduced by this PR.

Thanks @TaylorMutch. I checked the atomic-transition concern you raised against the current self-reject flow and both persistence backends; this head resolves it for the defect in scope.

Docs: no Fern update is needed because this restores intended internal policy behavior without changing commands, configuration, API contracts, or user workflows. The architecture documentation is updated appropriately.

E2E: test:e2e is applied because the change affects policy enforcement and gateway state transitions. The current mirror already matches this head, and the existing Branch E2E workflow has been re-run with the label active.

Next state: gator:watch-pipeline

Resolved

[johntmyers]

gator-agent

Monitoring Complete

Monitoring is complete because this PR has merged.

Final status: head a48dcc4febf6fdacf400c61fffb9448c8932cad9 had no blocking review findings, maintainer approval was present, and the required Branch Checks, Helm Lint, and E2E gates passed.

I removed the active gator:* label because there is nothing left for gator to monitor on this PR.