Repository navigation
fix(approval): recheck authority after publication, retryable retirement #23
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -207,6 +207,11 @@ impl ScopeTable { | |
| .insert(entry.scope.scope_id.clone(), Arc::new(entry)); | ||
| fresh | ||
| } | ||
| /// Put back an entry removed by a retirement the ports could not complete. | ||
| fn reinstate(&mut self, entry: Arc<ScopeEntry>) { | ||
| self.subjects.insert(Self::key(&entry.scope)); | ||
| self.by_id.insert(entry.scope.scope_id.clone(), entry); | ||
| } | ||
| fn remove(&mut self, id: &str) -> Option<Arc<ScopeEntry>> { | ||
| let removed = self.by_id.remove(id)?; | ||
| self.subjects.remove(&Self::key(&removed.scope)); | ||
|
|
@@ -504,11 +509,7 @@ impl Engine { | |
| if reg.generation != self.generation.load(Ordering::SeqCst) { | ||
| return Err(Error::Denied); | ||
| } | ||
| // A grant or posture replaced while the ports were awaited must not publish | ||
| // an automatic approval from the obsolete snapshot. | ||
| if automatic && !Arc::ptr_eq(&entry, &self.entry(&request.scope_id)?) { | ||
| return Err(Error::Denied); | ||
| } | ||
| self.automatic_stands(&entry, &request.scope_id, automatic)?; | ||
| let now = self.now()?; | ||
| if reusable | ||
| .as_ref() | ||
|
|
@@ -550,8 +551,23 @@ impl Engine { | |
| audit: request.audit, | ||
| }; | ||
| let row = self.memo_begin(row, deadline.is_some())?; | ||
| self.finish_native_begin(®, row, now, deadline.is_some()) | ||
| .await | ||
| let row = self | ||
| .finish_native_begin(®, row, now, deadline.is_some()) | ||
| .await?; | ||
| // The publication awaited too: never hand back an approval whose automatic | ||
| // authority was replaced meanwhile (consumption would refuse it anyway). | ||
| if row.state == ApprovalLifecycle2::Approved { | ||
| self.automatic_stands(&entry, &request.scope_id, automatic)?; | ||
|
Comment on lines
+559
to
+560
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a grant or automatic posture is replaced while AGENTS.md reference: AGENTS.md:L162-L166 Useful? React with 👍 / 👎. |
||
| } | ||
| Ok(row) | ||
| } | ||
| /// An automatic approval stands only while the scope entry it was derived from is | ||
| /// still the live one: a grant or posture replaced during an await revokes it. | ||
| fn automatic_stands(&self, entry: &Arc<ScopeEntry>, id: &str, automatic: bool) -> Result<()> { | ||
| if automatic && !Arc::ptr_eq(entry, &self.entry(id)?) { | ||
| return Err(Error::Denied); | ||
| } | ||
| Ok(()) | ||
| } | ||
| /// Who approved a row that needed no human: reviewed reuse, the judge, a host | ||
| /// grant or the scope's own automatic posture. | ||
|
|
@@ -1212,7 +1228,15 @@ impl Engine { | |
| .map_err(|_| Error::Unavailable)? | ||
| .remove(&request.scope_id) | ||
| .ok_or(Error::Denied)?; | ||
| self.ports.release_scope(&removed.scope).await?; | ||
| if let Err(error) = self.ports.release_scope(&removed.scope).await { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift Preserve the scope if retirement is cancelled. If the caller drops 🤖 Prompt for AI Agents |
||
| // Keep ownership so the retirement can be retried; the ports fail closed | ||
| // for whatever they already released. | ||
| self.scopes | ||
| .write() | ||
| .map_err(|_| Error::Unavailable)? | ||
| .reinstate(removed); | ||
| return Err(error); | ||
| } | ||
| let scopes = self.scopes.read().map_err(|_| Error::Unavailable)?; | ||
| Self::receipt( | ||
| &scopes, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -316,7 +316,22 @@ async fn retire_denies_later_calls_releases_the_subject_and_reports_once() -> Re | |
| .err(), | ||
| Some(Error::Unavailable) | ||
| ); | ||
| // A failed release still leaves the scope unusable. | ||
| // A failed release keeps ownership: the subject cannot register again, and the | ||
| // retirement can be retried until the ports complete it. | ||
| let mut again = register(7); | ||
| again.scope.scope_id = id(8); | ||
| assert_eq!( | ||
| owner.register_scope(again).await.err(), | ||
| Some(Error::Conflict) | ||
| ); | ||
| fixture.release_fails.store(0, Ordering::SeqCst); | ||
| assert_eq!( | ||
| owner | ||
| .retire_scope(RetireApprovalScopeRequest2 { scope_id: id(7) }) | ||
| .await? | ||
| .outcome, | ||
| ApprovalScopeOutcome2::Retired | ||
| ); | ||
| assert!( | ||
| owner | ||
| .begin(begin_for(7, "local_diagnostic", None, Origin::Operator)) | ||
|
|
@@ -930,3 +945,43 @@ async fn initialized_scopes_cannot_be_retired() -> Result<()> { | |
| ); | ||
| Ok(()) | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn grant_revoked_while_the_approval_is_published_is_not_returned() -> Result<()> { | ||
| let (fixture, owner) = owner()?; | ||
| let owner = Arc::new(owner); | ||
| let mut req = register(7); | ||
| req.grants = grants(&["shell"], &[]); | ||
| owner.register_scope(req).await?; | ||
| let before = fixture.cas_calls.load(Ordering::SeqCst); | ||
| fixture | ||
| .barrier | ||
| .lock() | ||
| .map_err(|_| Error::Unavailable)? | ||
| .replace(Arc::new(tokio::sync::Barrier::new(2))); | ||
| let racing = tokio::spawn({ | ||
| let owner = owner.clone(); | ||
| async move { | ||
| owner | ||
| .begin(begin_for(7, "shell", None, Origin::Operator)) | ||
| .await | ||
| } | ||
| }); | ||
| // The new row's write is parked: the grant is revoked after the early checks. | ||
| while fixture.cas_calls.load(Ordering::SeqCst) == before { | ||
| tokio::task::yield_now().await; | ||
| } | ||
|
Comment on lines
+971
to
+973
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win Stop waiting if If the spawned 🤖 Prompt for AI Agents |
||
| owner.replace_scope(register(7))?; | ||
| let barrier = fixture | ||
| .barrier | ||
| .lock() | ||
| .map_err(|_| Error::Unavailable)? | ||
| .clone() | ||
| .ok_or(Error::Unavailable)?; | ||
| barrier.wait().await; | ||
| assert_eq!( | ||
| racing.await.map_err(|_| Error::Unavailable)?.err(), | ||
| Some(Error::Denied) | ||
| ); | ||
| Ok(()) | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Validate the approval that
beginreturns.If
replace_scoperevokes a tool grant without changingpolicy_ref, a repeatedbegincan return the existingApprovedrow at Line 490 before this check runs.consume_innerthen denies that row because the grant no longer holds. Validate the returned row’s live authority on both the existing-row and newly published paths. Checking only theautomaticvalue calculated for the new attempt does not cover an older row.🤖 Prompt for AI Agents