Fix expired pending approvals lingering after restart#2357
Conversation
📝 WalkthroughWalkthroughThe approval store adds durability-oriented expiration handling for pending SQLite rows. A new public ChangesApproval Store Expiration and Durability
Sequence DiagramsequenceDiagram
participant Client
participant expire_stale
participant expire_stale_with_now
participant list_pending
participant decide
participant SQLite
Client->>list_pending: list_pending()
list_pending->>expire_stale: expire_stale()
expire_stale->>expire_stale_with_now: expire_stale_with_now(Utc::now)
expire_stale_with_now->>SQLite: UPDATE undecided rows where expires_at < now
SQLite-->>expire_stale_with_now: count of expired rows
expire_stale_with_now->>expire_stale: return count
expire_stale-->>list_pending: Ok(count)
list_pending->>SQLite: SELECT undecided rows
SQLite-->>list_pending: pending approvals
list_pending-->>Client: Vec<Pending>
Client->>decide: decide(request_id, decision)
decide->>expire_stale: expire_stale()
expire_stale-->>decide: Ok(count)
decide->>SQLite: UPDATE row to decided state
SQLite-->>decide: rows affected
decide-->>Client: Ok(Option<Audit>)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/openhuman/approval/store.rs (2)
217-219: 💤 Low valueLog a warning on args_redacted parse failure.
Silently falling back to
Nullcould mask data corruption or encoding issues in theargs_redactedcolumn. Consider logging at debug level to aid troubleshooting.♻️ Proposed fix to add debug logging
fn row_to_pending(row: &rusqlite::Row<'_>) -> rusqlite::Result<PendingApproval> { let args_str: String = row.get(3)?; - let args_redacted = serde_json::from_str(&args_str).unwrap_or(serde_json::Value::Null); + let args_redacted = serde_json::from_str(&args_str).unwrap_or_else(|e| { + tracing::debug!( + error = %e, + "[approval::store] args_redacted parse failed, falling back to null" + ); + serde_json::Value::Null + }); let created_str: String = row.get(5)?;As per coding guidelines: "Use
log/tracingatdebugortracelevel on ... error paths".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/approval/store.rs` around lines 217 - 219, In row_to_pending, don't silently unwrap args_redacted parse failures: change the serde_json::from_str(&args_str).unwrap_or(...) call to match on the Result from serde_json::from_str and on Err emit a debug-level log (using log::debug or tracing::debug) that includes the parse error and the original args_str, then set args_redacted = serde_json::Value::Null as the fallback; reference the function name row_to_pending and the variables args_str/args_redacted when locating where to implement this.
201-215: ⚡ Quick winAdd trace logging for state transition.
This function performs a significant state transition (pending → terminal) but lacks any logging. Per coding guidelines, state transitions should be logged at
debugortracelevel to aid debugging.♻️ Proposed fix to add trace logging
fn expire_stale_with_now(conn: &Connection, now: DateTime<Utc>) -> Result<usize> { let now_rfc3339 = now.to_rfc3339(); let deny = ApprovalDecision::Deny.as_str(); let updated = conn .execute( "UPDATE pending_approvals SET decided_at = ?1, decision = ?2 WHERE decided_at IS NULL AND expires_at IS NOT NULL AND strftime('%s', expires_at) <= strftime('%s', ?3)", params![now_rfc3339, deny, now_rfc3339], ) .context("[approval::store] expire stale rows")?; + if updated > 0 { + tracing::debug!( + rows = updated, + "[approval::store] transitioned stale pending rows to terminal state" + ); + } Ok(updated) }As per coding guidelines: "Use
log/tracingatdebugortracelevel on ... state transitions".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/approval/store.rs` around lines 201 - 215, The expire_stale_with_now function performs a pending→terminal transition but has no telemetry; add a trace (or debug) log in expire_stale_with_now after executing the UPDATE to record the transition details: log the function name (expire_stale_with_now), the timestamp `now` (or now_rfc3339), the decision being applied (`ApprovalDecision::Deny` / `deny`) and the number of rows updated (`updated`) so operators can correlate when and how many rows moved to a terminal state; use the project’s logging crate (`tracing` or `log`) consistent with other modules and emit the log before returning Ok(updated).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/openhuman/approval/store.rs`:
- Around line 257-295: The test helper formatting is off — run rustfmt (cargo
fmt) or manually reformat the chained method calls in the helper functions
(sample, sample_with_expiry, fetch_decision_state) so chained calls are aligned
correctly; specifically reflow the multi-line chained calls in
sample_with_expiry and the statement/query chaining inside fetch_decision_state
(prepare(...).context(...)? and query(...).context(...)? /
rows.next().context(...)?), or simply run cargo fmt to auto-fix the formatting.
- Around line 371-468: Two multi-line assert! macros in the tests
list_pending_expires_stale_rows_before_returning and
decide_on_expired_row_returns_none_and_keeps_terminal_audit_state are
misformatted and fail cargo fmt; reformat those assert! calls to satisfy rustfmt
(e.g. put the boolean expression and the message on the same line or use a
single-line assert!(expr, "message") or construct the message with format!
inline), then run cargo fmt to apply consistent formatting and commit the
result.
---
Nitpick comments:
In `@src/openhuman/approval/store.rs`:
- Around line 217-219: In row_to_pending, don't silently unwrap args_redacted
parse failures: change the serde_json::from_str(&args_str).unwrap_or(...) call
to match on the Result from serde_json::from_str and on Err emit a debug-level
log (using log::debug or tracing::debug) that includes the parse error and the
original args_str, then set args_redacted = serde_json::Value::Null as the
fallback; reference the function name row_to_pending and the variables
args_str/args_redacted when locating where to implement this.
- Around line 201-215: The expire_stale_with_now function performs a
pending→terminal transition but has no telemetry; add a trace (or debug) log in
expire_stale_with_now after executing the UPDATE to record the transition
details: log the function name (expire_stale_with_now), the timestamp `now` (or
now_rfc3339), the decision being applied (`ApprovalDecision::Deny` / `deny`) and
the number of rows updated (`updated`) so operators can correlate when and how
many rows moved to a terminal state; use the project’s logging crate (`tracing`
or `log`) consistent with other modules and emit the log before returning
Ok(updated).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c9e52c2a-44ae-483e-9584-4937bac81426
📒 Files selected for processing (1)
src/openhuman/approval/store.rs
| fn sample(request_id: &str, session_id: &str) -> PendingApproval { | ||
| sample_with_expiry(request_id, session_id, Some(Utc::now() + Duration::minutes(10))) | ||
| } | ||
|
|
||
| fn sample_with_expiry( | ||
| request_id: &str, | ||
| session_id: &str, | ||
| expires_at: Option<DateTime<Utc>>, | ||
| ) -> PendingApproval { | ||
| PendingApproval { | ||
| request_id: request_id.to_string(), | ||
| tool_name: "composio".to_string(), | ||
| action_summary: "send slack message (12 chars)".to_string(), | ||
| args_redacted: json!({ "action": "execute", "tool_slug": "SLACK_SEND" }), | ||
| session_id: session_id.to_string(), | ||
| created_at: Utc::now(), | ||
| expires_at: Some(Utc::now() + Duration::minutes(10)), | ||
| expires_at, | ||
| } | ||
| } | ||
|
|
||
| fn fetch_decision_state( | ||
| config: &Config, | ||
| request_id: &str, | ||
| ) -> Option<(Option<String>, Option<String>)> { | ||
| with_connection(config, |conn| { | ||
| let mut stmt = conn | ||
| .prepare("SELECT decided_at, decision FROM pending_approvals WHERE request_id = ?1") | ||
| .context("prepare raw decision lookup")?; | ||
| let mut rows = stmt.query(params![request_id]).context("query raw decision lookup")?; | ||
| if let Some(row) = rows.next().context("decision row next")? { | ||
| let decided_at: Option<String> = row.get(0)?; | ||
| let decision: Option<String> = row.get(1)?; | ||
| Ok(Some((decided_at, decision))) | ||
| } else { | ||
| Ok(None) | ||
| } | ||
| }) | ||
| .unwrap() | ||
| } |
There was a problem hiding this comment.
Fix formatting to pass CI.
The pipeline reports cargo fmt failures in the test helpers. Run cargo fmt to fix the formatting at lines 258 and 285 (chained method calls).
🧰 Tools
🪛 GitHub Actions: Type Check / 0_Rust Quality (fmt + clippy).txt
[error] 282-282: cargo fmt --all -- --check failed due to formatting differences in query statement chaining. Rustfmt expects stmt.query(...) to be broken across multiple lines before .context(...).
🪛 GitHub Actions: Type Check / 1_Type Check TypeScript.txt
[error] 282-282: Prettier --check failed for openhuman-app format:check. Diff indicates formatting mismatch near query() call chain in store.rs.
🪛 GitHub Actions: Type Check / Rust Quality (fmt + clippy)
[error] 282-282: cargo fmt --all -- --check failed due to formatting differences in raw SQL query: stmt.query(...) must be reformatted across multiple lines.
🪛 GitHub Actions: Type Check / Type Check TypeScript
[error] 282-282: Prettier --check failed: formatting differences detected in store.rs (stmt.query(...).context(...) chained formatting). Run 'prettier --write .' to fix.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/openhuman/approval/store.rs` around lines 257 - 295, The test helper
formatting is off — run rustfmt (cargo fmt) or manually reformat the chained
method calls in the helper functions (sample, sample_with_expiry,
fetch_decision_state) so chained calls are aligned correctly; specifically
reflow the multi-line chained calls in sample_with_expiry and the
statement/query chaining inside fetch_decision_state (prepare(...).context(...)?
and query(...).context(...)? / rows.next().context(...)?), or simply run cargo
fmt to auto-fix the formatting.
| #[test] | ||
| fn list_pending_expires_stale_rows_before_returning() { | ||
| let (config, _dir) = test_config(); | ||
| insert_pending( | ||
| &config, | ||
| &sample_with_expiry("expired", "sess-A", Some(Utc::now() - Duration::minutes(5))), | ||
| ) | ||
| .unwrap(); | ||
| insert_pending( | ||
| &config, | ||
| &sample_with_expiry("active", "sess-A", Some(Utc::now() + Duration::minutes(5))), | ||
| ) | ||
| .unwrap(); | ||
|
|
||
| let rows = list_pending(&config).unwrap(); | ||
| let ids: Vec<_> = rows.into_iter().map(|row| row.request_id).collect(); | ||
| assert_eq!(ids, vec!["active"]); | ||
|
|
||
| let state = fetch_decision_state(&config, "expired").expect("expired row should persist"); | ||
| assert!(state.0.is_some(), "expired row should have decided_at recorded"); | ||
| assert_eq!(state.1.as_deref(), Some("deny")); | ||
| } | ||
|
|
||
| #[test] | ||
| fn decide_on_expired_row_returns_none_and_keeps_terminal_audit_state() { | ||
| let (config, _dir) = test_config(); | ||
| insert_pending( | ||
| &config, | ||
| &sample_with_expiry("late", "sess-A", Some(Utc::now() - Duration::minutes(1))), | ||
| ) | ||
| .unwrap(); | ||
|
|
||
| let decided = decide(&config, "late", ApprovalDecision::ApproveOnce).unwrap(); | ||
| assert!(decided.is_none(), "late approvals should no longer be actionable"); | ||
|
|
||
| let state = fetch_decision_state(&config, "late").expect("row should remain for audit"); | ||
| assert!(state.0.is_some()); | ||
| assert_eq!(state.1.as_deref(), Some("deny")); | ||
| } | ||
|
|
||
| #[test] | ||
| fn expire_stale_returns_number_of_rows_transitioned() { | ||
| let (config, _dir) = test_config(); | ||
| insert_pending( | ||
| &config, | ||
| &sample_with_expiry("old-1", "sess-A", Some(Utc::now() - Duration::minutes(2))), | ||
| ) | ||
| .unwrap(); | ||
| insert_pending( | ||
| &config, | ||
| &sample_with_expiry("old-2", "sess-B", Some(Utc::now() - Duration::minutes(1))), | ||
| ) | ||
| .unwrap(); | ||
| insert_pending( | ||
| &config, | ||
| &sample_with_expiry("fresh", "sess-B", Some(Utc::now() + Duration::minutes(30))), | ||
| ) | ||
| .unwrap(); | ||
|
|
||
| let expired = expire_stale(&config).unwrap(); | ||
| assert_eq!(expired, 2); | ||
|
|
||
| let rows = list_pending(&config).unwrap(); | ||
| assert_eq!(rows.len(), 1); | ||
| assert_eq!(rows[0].request_id, "fresh"); | ||
| } | ||
|
|
||
| #[test] | ||
| fn expire_stale_is_idempotent() { | ||
| let (config, _dir) = test_config(); | ||
| insert_pending( | ||
| &config, | ||
| &sample_with_expiry("once", "sess-A", Some(Utc::now() - Duration::minutes(3))), | ||
| ) | ||
| .unwrap(); | ||
|
|
||
| assert_eq!(expire_stale(&config).unwrap(), 1); | ||
| assert_eq!(expire_stale(&config).unwrap(), 0); | ||
|
|
||
| let state = fetch_decision_state(&config, "once").expect("row should remain recorded"); | ||
| assert!(state.0.is_some()); | ||
| assert_eq!(state.1.as_deref(), Some("deny")); | ||
| } | ||
|
|
||
| #[test] | ||
| fn expire_stale_leaves_non_expiring_rows_pending() { | ||
| let (config, _dir) = test_config(); | ||
| insert_pending(&config, &sample_with_expiry("no-ttl", "sess-A", None)).unwrap(); | ||
|
|
||
| assert_eq!(expire_stale(&config).unwrap(), 0); | ||
| let rows = list_pending(&config).unwrap(); | ||
| assert_eq!(rows.len(), 1); | ||
| assert_eq!(rows[0].request_id, "no-ttl"); | ||
|
|
||
| let state = fetch_decision_state(&config, "no-ttl").expect("row should still exist"); | ||
| assert!(state.0.is_none()); | ||
| assert!(state.1.is_none()); | ||
| } |
There was a problem hiding this comment.
Fix formatting in assert! macros to pass CI.
The pipeline reports cargo fmt failures for the multi-line assert! calls at lines 387-391 and 401-404. Run cargo fmt to fix.
🧰 Tools
🪛 GitHub Actions: Type Check / 0_Rust Quality (fmt + clippy).txt
[error] 387-387: cargo fmt --all -- --check failed due to formatting differences in assert! with a multi-line condition/message. Rustfmt expects an assert!( ... ) block layout.
[error] 401-401: cargo fmt --all -- --check failed due to formatting differences in assert! for decided.is_none(). Rustfmt expects the assert to be formatted across multiple lines.
🪛 GitHub Actions: Type Check / 1_Type Check TypeScript.txt
[error] 387-389: Prettier --check failed for openhuman-app format:check. Diff indicates formatting mismatch for multi-line assert!(...) in store.rs.
[error] 401-404: Prettier --check failed for openhuman-app format:check. Diff indicates formatting mismatch for multi-line assert!(...) in store.rs.
🪛 GitHub Actions: Type Check / Rust Quality (fmt + clippy)
[error] 387-387: cargo fmt --all -- --check failed due to formatting differences in assert!(): assert!(..., "...") should be reformatted to multi-line macro invocation.
[error] 401-401: cargo fmt --all -- --check failed due to formatting differences in assert!(): assert! for decided.is_none() should be reformatted to multi-line macro invocation.
🪛 GitHub Actions: Type Check / Type Check TypeScript
[error] 387-387: Prettier --check failed: formatting differences detected in store.rs (multi-line assert! call formatting). Run 'prettier --write .' to fix.
[error] 401-401: Prettier --check failed: formatting differences detected in store.rs (multi-line assert! for decided.is_none formatting). Run 'prettier --write .' to fix.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/openhuman/approval/store.rs` around lines 371 - 468, Two multi-line
assert! macros in the tests list_pending_expires_stale_rows_before_returning and
decide_on_expired_row_returns_none_and_keeps_terminal_audit_state are
misformatted and fail cargo fmt; reformat those assert! calls to satisfy rustfmt
(e.g. put the boolean expression and the message on the same line or use a
single-line assert!(expr, "message") or construct the message with format!
inline), then run cargo fmt to apply consistent formatting and commit the
result.
|
@Alexxigang CI is failing on changes in this PR — please fix before review. |
1 similar comment
|
@Alexxigang CI is failing on changes in this PR — please fix before review. |
2 participants
Summary
Why
expires_at was already being persisted for pending approvals, but the store never enforced it on read or decide paths. After a restart, an orphaned request could remain actionable in �pproval.list_pending indefinitely even though its TTL had already elapsed.
This change keeps the fix scoped to the expiry-lifecycle gap in #2135 and does not overlap with #2335's recent-decision audit read path.
Validation
Summary by CodeRabbit
New Features
Improvements
Documentation