You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix(approval): let a person deny a request an earlier launch left waiting - #8679
Follow-up to #8657. That PR carried requests an earlier version left waiting into TinySecurity as pending orphans and listed them in approval_list_pending, but nothing could clear one: approval_decide answered "no pending approval ever registered", so an orphan stayed on the list until its window closed.
This PR lets a person deny (dismiss) an orphan. Approving one is still refused: nothing in this launch can run the call it was for, and TinySecurity's Decide2 accepts only a denial for a row another host instance owns.
Changes
Gate (security/approval/gate_orphans.rs, hooked into ApprovalGate::decide_as):
Runs only for a request that is not parked here, decided by the default tenant with no agent named. That is the only tenant orphans are listed for.
A denial dismisses the orphan, publishes ApprovalDecided and returns the row.
Any approve decision is refused with "can only be denied", and the orphan stays listed.
Everything else falls through to the existing miss handling.
Engine trait: new dismiss_orphan. Its default answers NotPending.
Native engine (native/engine_orphans.rs):
Re-reads the row from the import's history scope. It must be pending, carry legacy_history_json, and be inside its window.
Issues a deny-only ticket and calls Decide2 on that scope's audit stream.
Keeps the deny label, and reports a lost race as NotPending.
Human verification (native/callbacks.rs, native/state.rs):
With no request of this process behind a row, verify_human accepts only an orphan currently under review.
The row must match the listed one byte for byte, and the choice must be Deny.
Anything else fails closed.
Stub engine: models orphans (leave_orphan, orphans, dismiss_orphan). Its decide refuses an orphan, as the native engine does.
Docs: approval README.md and the native/mod.rs layout table.
Real binary and released module, run in the CI image.
Approving the carried-over request is refused and it stays listed. Denying it succeeds, and it is not listed again in this launch or the next.
RED before the fix: no pending approval ever registered for request_id '…'. GREEN after.
security/approval/gate_orphan_tests.rs (4 tests): deny dismisses; the approve decisions are refused; another profile or a named agent cannot dismiss; an expired orphan and an unknown id are not dismissed.
Two of them fail with the decide_as hook disabled.
cargo test -p openhuman --lib -- security::approval passes: 223 tests, plus the new callbacks test.
Notes
History shows a different id. A dismissed orphan appears in recent decisions as "Recorded by an earlier version", under its earlier-version request id. The pending list showed the module's id, and the history view maps rows that carry legacy_history_json back to the id from the earlier version.
Summary by CodeRabbit
New Features
Pending approval requests carried over from an earlier launch are now listed for the default workspace.
You can deny a carried-over request after the host verifies the exact listed request. These requests cannot be approved and remain listed until denied or expired.
Bug Fixes
Decisions for requests owned by another running instance are restricted, preventing unintended changes to those requests.
Add an end-to-end test asserting that a pending approval left by an earlier version cannot be approved in a later launch but can be denied, and that the denial persists across restarts.
Auto-committed-on: cuttlefish
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Human verification now accepts a pending orphan from a previous launch when the choice is a denial, matching the row as listed rather than requiring it to be pending in this process. Orphan rows under review are held in a bounded map so the module can verify the denial byte for byte.
Auto-committed-on: cuttlefish
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Introduce a module that detects and cleans up approval engine instances left orphaned after their owning task ends. This prevents stale engines from accumulating and holding resources.
Auto-committed-on: cuttlefish
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Adds a dismiss_orphan method to the ApprovalEngine trait that denies a pending orphan request as a person's decision, returning NotPending when the request is not one. The native engine implements it behind the legacy-import feature by delegating to its existing orphan dismissal, which was renamed to avoid clashing with the new trait method.
Auto-committed-on: cuttlefish
Co-authored-by: Medulla <medulla@tinyhumans.ai>
…ovals
Decision validation now returns the parked approval so the gate can tell a missing request from an orphan, and the default tenant may resolve orphaned pending approvals that are otherwise listed but undismissable.
Auto-committed-on: cuttlefish
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Wrapped long lines and collapsed a multi-line signature in the native approval callbacks and orphan engine so the files match rustfmt output. No behaviour changes.
Auto-committed-on: cuttlefish
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Pending requests left over from an earlier launch are now recorded as orphans so they can be listed separately and only ever denied, since no host instance owns them and a decision cannot reach them. Added an orphans listing and a dismiss path that marks such requests denied with a label.
Auto-committed-on: cuttlefish
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Wire the new gate_orphan_tests.rs file into the approval gate test suite so its orphan-handling cases are compiled and run alongside the existing gate tests.
Auto-committed-on: cuttlefish
Co-authored-by: Medulla <medulla@tinyhumans.ai>
The orphan test no longer asserts on the result of naming an agent when denying an orphan, since the agent path's answer is irrelevant to the invariant that the orphan stays pending. Module declarations were also reordered alphabetically and a call was reformatted.
Auto-committed-on: cuttlefish
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Add a test asserting that an orphan row under review verifies only a denial of the exact listed row, that a second concurrent review is refused, and that verification fails before the review begins and after it ends.
Auto-committed-on: cuttlefish
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Added a row to the native approval module's file table describing engine_orphans.rs, which denies pending orphans left behind by an earlier launch, so the module overview reflects the file's role.
Auto-committed-on: cuttlefish
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Explain how the legacy import carries undecided requests into TinySecurity as pending orphans owned by no host instance, and note that such rows can only be denied, never approved, until they are denied or their window closes.
Auto-committed-on: cuttlefish
Co-authored-by: Medulla <medulla@tinyhumans.ai>
Verdict: Incomplete · Priority: none Coverage: Reviewed 6 of 16 files; not reviewed (large PR): crates/openhuman…, crates/openhuman…, and 8 additional unreviewed files.
Pull request purpose (title): fix(approval): let a…
Diagnostics and supporting details
Full summary
Pull request purpose (title): fix(approval): let a person deny a request an earlier launch left waiting. Tiny Sweeper completed its review; deterministic results follow.
critique: crates/openhuman-core/src/security/approval/README.md — not selected by the large-review budget
critique: crates/openhuman-core/src/security/approval/engine.rs — not selected by the large-review budget
security: crates/openhuman-core/src/security/approval/engine.rs — not selected by the large-review budget
critique: crates/openhuman-core/src/security/approval/gate.rs — not selected by the large-review budget
security: crates/openhuman-core/src/security/approval/gate.rs — not selected by the large-review budget
critique: crates/openhuman-core/src/security/approval/gate_state.rs — not selected by the large-review budget
security: crates/openhuman-core/src/security/approval/gate_state.rs — not selected by the large-review budget
critique: crates/openhuman-core/src/security/approval/gate_tests.rs — not selected by the large-review budget
security: crates/openhuman-core/src/security/approval/gate_tests.rs — not selected by the large-review budget
critique: crates/openhuman-core/src/security/approval/native/callbacks.rs — not selected by the large-review budget
security: crates/openhuman-core/src/security/approval/native/callbacks.rs — not selected by the large-review budget
critique: crates/openhuman-core/src/security/approval/native/engine.rs — not selected by the large-review budget
security: crates/openhuman-core/src/security/approval/native/engine.rs — not selected by the large-review budget
critique: crates/openhuman-core/src/security/approval/native/engine_impl.rs — not selected by the large-review budget
security: crates/openhuman-core/src/security/approval/native/engine_impl.rs — not selected by the large-review budget
critique: crates/openhuman-core/src/security/approval/native/mod.rs — not selected by the large-review budget
security: crates/openhuman-core/src/security/approval/native/mod.rs — not selected by the large-review budget
critique: crates/openhuman-core/src/security/approval/native/state.rs — not selected by the large-review budget
security: crates/openhuman-core/src/security/approval/native/state.rs — not selected by the large-review budget
Review snapshot
Change surface
Files
Review signal
Count
Production
11
Active findings
0
Tests
4
Noted findings
0
Documentation
1
Resolved findings
0
Configuration
0
Pending checks/questions
29
Completeness: Incomplete Test assessment: Test coverage is assessed from changed tests and lane evidence; execution is not claimed without trusted check data.
Features
Added — `dismiss_orphan` on the approval engine trait: Engines gain an opt-in path to dismiss an orphan; the default answers `NotPending`, so an engine that does not model orphans keeps rejecting such decisions. (crates/openhuman-core/src/security/approval/engine.rspub(crate) trait ApprovalEngine: sealed::Sealed + std::fmt::Debug + Send + Sync)
Added — Native engine orphan dismissal through `Decide2`: The native path re-validates the row (pending, legacy-imported, unexpired), issues a deny-only ticket, calls `Decide2` on the import's scope, records the label, and treats a competing decision or expiry as `NotPending`; it only compiles under the `legacy-import` feature. (crates/openhuman-core/src/security/approval/native/engine_orphans.rs, crates/openhuman-core/src/security/approval/native/engine_impl.rsimpl ApprovalEngine for NativeEngine {, crates/openhuman-core/src/security/approval/native/engine.rsimpl NativeEngine {)
Added — Orphan review registry on the host: Tracks at most one in-flight review per request and bounded overall, so a row can only be dismissed while a matching denial review is held; the entry is removed when the decision completes. (crates/openhuman-core/src/security/approval/native/state.rspub(super) struct Host {)
Added — Stub engine orphan model: Lets the gate's orphan behavior be exercised without the TinySecurity module: orphans are listed, a plain decision on one is refused, and only `dismiss_orphan` denies it. (crates/openhuman-core/src/security/approval/engine_stub.rspub(crate) struct StubEngine {, crates/openhuman-core/src/security/approval/engine_stub.rsimpl StubEngine {, crates/openhuman-core/src/security/approval/engine_stub.rsimpl ApprovalEngine for StubEngine {)
Added — Documentation of the orphan dismissal path: The approval README explains the pending-orphan lifecycle and the deny-only rule, and the native module table names the new file. (crates/openhuman-core/src/security/approval/README.mdpending request once after an audited decision; it cannot create a reusable, crates/openhuman-core/src/security/approval/native/mod.rs)
Tests
unit — Gate-level orphan tests cover dismissal by a denial (state and `deny` label, one-shot removal from the list), refusal of every approve decision with the orphan left pending, rejection of non-default profiles and named agents, and non-dismissal of expired or unknown ids.: The tests lane answered with no reportable defect, describing these as asserting observable state transitions and error paths that appear to fail on regression. The PR description says two of them fail with the `decide_as` hook disabled; that was not verified by this review. (crates/openhuman-core/src/security/approval/gate_orphan_tests.rs, crates/openhuman-core/src/security/approval/gate_tests.rsmod gate_origin_class_tests;)
unit — Human verification of an orphan under review accepts only a denial of the listed row, refuses other choices, refuses an altered row, refuses verification before review begins and after it ends, and refuses a second concurrent review.: The tests lane answered with no reportable defect. The PR description notes this test in `callbacks_tests.rs`; execution was not verified by this review. (crates/openhuman-core/src/security/approval/native/callbacks_tests.rsasync fn only_the_choices_the_host_offers_are_verified() {)
Findings
No active actionable findings.
Before merge
Complete the critique review for crates/openhuman-core/src/security/approval/gate_state.rs, crates/openhuman-core/src/security/approval/native/state.rs, crates/openhuman-core/src/security/approval/native/callbacks.rs, crates/openhuman-core/src/security/approval/README.md, crates/openhuman-core/src/security/approval/engine.rs, crates/openhuman-core/src/security/approval/native/engine_impl.rs, crates/openhuman-core/src/security/approval/native/engine.rs, crates/openhuman-core/src/security/approval/gate_tests.rs, crates/openhuman-core/src/security/approval/gate.rs, crates/openhuman-core/src/security/approval/native/mod.rs.
Complete the security review for crates/openhuman-core/src/security/approval/gate_state.rs, crates/openhuman-core/src/security/approval/native/state.rs, crates/openhuman-core/src/security/approval/native/callbacks.rs, crates/openhuman-core/src/security/approval/engine.rs, crates/openhuman-core/src/security/approval/native/engine_impl.rs, crates/openhuman-core/src/security/approval/native/engine.rs, crates/openhuman-core/src/security/approval/gate_tests.rs, crates/openhuman-core/src/security/approval/gate.rs, crates/openhuman-core/src/security/approval/native/mod.rs.
Lane summary: Reviewed 6 of 15 files — 9 not reviewed. Reviewed 6 files; 0 findings. 9 files were omitted: not selected by the large-review budget. 1 file was not security-reviewed: crates/openhuman-core/src/security/approval/README.md (prose or tabular data).
tests
Conclusion: Success
Scope reviewed: all assigned evidence
Positive: The tests lane answered with no reportable defect, judging the new tests to assert observable transitions and error paths that appear to catch regressions; the lane noted the code index was behind the pull request.
Lane summary: The change adds pending-orphan handling: orphans are listed, only a denial dismisses them, and the native path verifies that denial against the listed row byte for byte. The new tests (gate_orphan_tests.rs, callbacks_tests.rs, and the storage_default_import_e2e.rs addition) assert on observable state transitions and error paths, and appear to fail if the behaviour regresses; I found no reportable defect. _The code index is behind this pull request (indexed at `33054317cfbe`), so retrieved context may be out of date._
commits
Conclusion: Success
Scope reviewed: all assigned evidence
Positive: The commits lane answered that no committed files contain sensitive content; it also noted the code index was behind the pull request.
Lane summary: No sensitive content in committed files. _The code index is behind this pull request (indexed at `33054317cfbe`), so retrieved context may be out of date._
description
Conclusion: Success
Scope reviewed: all assigned evidence
Positive: The description lane answered that the description matches the diff, including the note that history shows an earlier-version request id for dismissed orphans, and that no repository rule is broken by the code shown.
Lane summary: The change adds a deny-only dismissal path for pending orphans, wiring it into `ApprovalGate::decide_as`, the engine trait (with a default), the native and stub engines, human verification, docs and tests; the description matches the diff, including the note about the history id. I found no defect in the covered paths — the description is accurate and no repository rule is broken by the code shown. _The code index is behind this pull request (indexed at `33054317cfbe`), so retrieved context may be out of date._
No actionable comments were generated in the recent review.
Walkthrough
Pending approvals imported from earlier launches are listed as orphans. The approval gate routes eligible default-tenant decisions to orphan handling. The native engine verifies the listed row and permits denial, while rejecting approval.
Changes: Legacy approval orphan denial
Layer / File(s)
Summary
Orphan engine contract and representation crates/openhuman-core/src/security/approval/engine.rs, crates/openhuman-core/src/security/approval/engine_stub.rs, crates/openhuman-core/src/security/approval/README.md
The engine adds a default dismiss_orphan method. StubEngine tracks, lists, and denies eligible orphans. The README describes imported orphan requests and their denial rules.
Gate routing and denial behavior crates/openhuman-core/src/security/approval/gate*.rs
The gate routes default-tenant, process-scoped decisions without a parked route to orphan handling. It accepts denial, publishes the decision, and has tests for scope, expiry, and other decision types.
Native orphan verification and denial crates/openhuman-core/src/security/approval/native/*, tests/storage_default_import_e2e.rs
The native engine verifies the exact listed row, tracks active reviews, and submits a ticketed denial. Tests cover verification and the default-storage import flow.
sequenceDiagram
participant ApprovalGate
participant NativeEngine
participant Host
participant Decide2
ApprovalGate->>NativeEngine: dismiss_orphan(request_id, label)
NativeEngine->>Host: begin_orphan_review(request_id, row)
NativeEngine->>Decide2: submit ticketed Deny
Decide2-->>NativeEngine: denial result
NativeEngine->>Host: end orphan review
NativeEngine-->>ApprovalGate: return dismissal result
Loading
Suggested reviewers:m3ga-mind, tinysweeper
Merge Risk: Minimal · up to d9319
The change lets a person deny pending approval requests left over from an earlier launch, while still refusing to approve them. A lookup failure on this path would show a misleading error message, but no wrong approval or data loss follows from it. No merge-blocking risk remains.
Pre-merge checks: 4
Check name
Status
Explanation
Description Check
Check skipped - CodeRabbit’s high-level summary is enabled.
Title check
The title names the main behavior change: a person can deny a pending approval request that an earlier launch left waiting. The changeset adds orphan tracking, a dismiss_orphan path in the engine and …
Linked Issues check
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
Check skipped because no linked issues were found for this pull request.
Full details: Title check
Explanation
The title names the main behavior change: a person can deny a pending approval request that an earlier launch left waiting. The changeset adds orphan tracking, a dismiss_orphan path in the engine and gate, and callback verification for denials. The title does not mention the deny-only restriction, but it is specific and matches the primary change.
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
A rabbit checks the pending row, And finds the old request below. No approval passes through the gate, A checked denial clears its state. The bunny hops; the list is neat.
Recent review info
Run configuration
Source mode: Auto
Repository: Unavailable
Branch: Unavailable
File: No configuration file from this repository applied
Commit: Unavailable
Layers: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: 3c475ded-721b-49f3-a0f9-5a392ef5937a
Commits
Reviewing files that changed from the base of the PR and between b4359e0 and d931950.
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.
Following #8657, this PR lets users dismiss pending approval requests imported from an earlier launch. It preserves the deny-only restriction because the original call cannot run in the current process.
Changes:
Adds default-tenant orphan dismissal through the approval gate and native engine.
Verifies denial tickets against the exact orphan row.
Adds gate, callback, and restart-persistence tests, plus supporting documentation.
File
Description
tests/storage_default_import_e2e.rs
Tests approval refusal and durable dismissal across restarts.
The reason will be displayed to describe this comment to others. Learn more.
Preserve the live posture when locating an orphan's scope
When a default-tenant tool check with auto_approve_all enabled runs concurrently with an orphan denial, this scope lookup can overwrite the check's configured posture. history_scope() calls scopes::ensure() with require_approval=true and auto_approve_all=false, and ScopeEntry::register() replaces the existing standard scope when that differs from its current posture. NativeEngine::check() registers its intended posture before acquiring scope.stream, so the denial can replace it between registration and EvaluateApproval2, causing an unrelated call to prompt or be refused unexpectedly. Reuse the existing scope without changing its posture, registering the conservative posture only when the scope is new.
Useful? React with 👍 / 👎.
This branch has not been deployed
No deployments
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
priority: p3Whenever. Cosmetic, a nicety, or a cleanup with no user visible effect.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
![tinysweeper[bot]](https://avatars.githubusercontent.com/in/4526585?s=60&v=4){kind=small}
tinysweeper
Bot
added
the
priority: p3
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
Summary
Follow-up to #8657. That PR carried requests an earlier version left waiting into TinySecurity as pending orphans and listed them in
approval_list_pending, but nothing could clear one:approval_decideanswered "no pending approval ever registered", so an orphan stayed on the list until its window closed.This PR lets a person deny (dismiss) an orphan. Approving one is still refused: nothing in this launch can run the call it was for, and TinySecurity's
Decide2accepts only a denial for a row another host instance owns.Changes
security/approval/gate_orphans.rs, hooked intoApprovalGate::decide_as):ApprovalDecidedand returns the row.dismiss_orphan. Its default answersNotPending.native/engine_orphans.rs):legacy_history_json, and be inside its window.Decide2on that scope's audit stream.denylabel, and reports a lost race asNotPending.native/callbacks.rs,native/state.rs):verify_humanaccepts only an orphan currently under review.Deny.leave_orphan,orphans,dismiss_orphan). Itsdeciderefuses an orphan, as the native engine does.README.mdand thenative/mod.rslayout table.Tests (RED → GREEN)
tests/storage_default_import_e2e.rs::a_request_an_earlier_version_left_waiting_can_be_denied_but_never_approvedno pending approval ever registered for request_id '…'. GREEN after.security/approval/gate_orphan_tests.rs(4 tests): deny dismisses; the approve decisions are refused; another profile or a named agent cannot dismiss; an expired orphan and an unknown id are not dismissed.decide_ashook disabled.native/callbacks_tests.rs::an_orphan_under_review_verifies_only_a_denial_of_the_listed_rowcargo test -p openhuman --lib -- security::approvalpasses: 223 tests, plus the new callbacks test.Notes
legacy_history_jsonback to the id from the earlier version.Summary by CodeRabbit