From c163d717147b92b16d89da2fbccef775647b5a07 Mon Sep 17 00:00:00 2001 From: DanConwayDev Date: Tue, 3 Feb 2026 16:13:59 +0000 Subject: fix: accept no-op pushes where old_oid == new_oid Fixes race condition where user's push becomes no-op after state event is applied between fetch and push. Now accepts these as successful no-ops, matching Git's 'Everything up-to-date' behavior. - Add early detection in get_state_authorization_for_specific_owner_repo - Return success for all-noop pushes without requiring purgatory event - Document behavior in inline-authorization.md --- src/git/authorization.rs | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) (limited to 'src/git/authorization.rs') diff --git a/src/git/authorization.rs b/src/git/authorization.rs index e174b51..db2b992 100644 --- a/src/git/authorization.rs +++ b/src/git/authorization.rs @@ -575,6 +575,28 @@ pub async fn get_state_authorization_for_specific_owner_repo( owner_pubkey ); + // Accept pushes where all refs are already at the desired state (old_oid == new_oid) + // This handles race conditions where state events are applied between fetch and push + if !pushed_refs.is_empty() { + let all_refs_unchanged = pushed_refs + .iter() + .all(|(old_oid, new_oid, _)| old_oid == new_oid); + + if all_refs_unchanged { + debug!( + "All pushed refs unchanged (old_oid == new_oid) for {} owned by {}, accepting without purgatory check", + identifier, owner_pubkey + ); + return Ok(AuthorizationResult { + authorized: true, + reason: "Push accepted: all refs already at desired state (no-op)".to_string(), + state: None, + maintainers: authorized.into_iter().collect(), + purgatory_events: vec![], + }); + } + } + // Check purgatory for matching state events // Convert pushed refs to RefUpdate (filter out refs/nostr/* refs) let pushed_updates: Vec = pushed_refs -- cgit v1.2.3 From d392f0bc14bcd687e918d4653ae016226496b4c4 Mon Sep 17 00:00:00 2001 From: DanConwayDev Date: Tue, 3 Feb 2026 21:21:33 +0000 Subject: feat: add diagnostic logging for partial state event matches Improves observability when pushes are rejected due to state events that only partially match the pushed refs. Previously, logs only showed 'No state event found' even when state events existed but didn't match. Changes: - Add diagnose_state_mismatch() to explain why state events don't match - Log specific reasons: missing refs, wrong SHAs, or extra refs - Update rejection message to 'No matching state event found' (more accurate) - Add 4 unit tests for diagnostic function Example diagnostic output: WARN State event abc123 from authorized author doesn't match push: refs/heads/main missing (state declares 9cc3d93b) This addresses the issue where a push with only refs/heads/test was rejected because the state event also declared refs/heads/main, but logs didn't explain why the match failed. --- src/git/authorization.rs | 80 ++++++++++++++++++- src/git/handlers.rs | 2 +- src/purgatory/helpers.rs | 204 +++++++++++++++++++++++++++++++++++++++++++++++ src/purgatory/mod.rs | 2 +- 4 files changed, 284 insertions(+), 4 deletions(-) (limited to 'src/git/authorization.rs') diff --git a/src/git/authorization.rs b/src/git/authorization.rs index db2b992..27107db 100644 --- a/src/git/authorization.rs +++ b/src/git/authorization.rs @@ -666,12 +666,88 @@ pub async fn get_state_authorization_for_specific_owner_repo( debug!("Purgatory events found but none from authorized authors"); } } else { - debug!("No matching state events found in purgatory"); + // Check if there are ANY state events in purgatory for this identifier + let all_purgatory_states = purgatory.find_state(identifier); + + if !all_purgatory_states.is_empty() { + // There are state events but none match the push - diagnose why + debug!( + "Found {} state event(s) in purgatory for {} but none match the push", + all_purgatory_states.len(), + identifier + ); + + // Count authorized state events and collect diagnostic info + let mut authorized_count = 0; + let mut diagnostic_reasons = Vec::new(); + + // Diagnose why each authorized state event doesn't match + for entry in all_purgatory_states.iter() { + let author_hex = entry.event.pubkey.to_hex(); + if authorized.contains(&author_hex) { + authorized_count += 1; + if let Some(reason) = crate::purgatory::diagnose_state_mismatch( + &entry.event, + &pushed_updates, + &local_refs, + ) { + debug!( + "State event {} from authorized author {} doesn't match push: {}", + entry.event.id, + entry + .event + .pubkey + .to_bech32() + .unwrap_or_else(|_| author_hex.clone()), + reason + ); + diagnostic_reasons.push(reason); + } + } + } + + // Create concise WARN message summarizing the rejection + let summary = if authorized_count > 0 { + let reason_summary = if !diagnostic_reasons.is_empty() { + // Take the first diagnostic reason as representative + format!(" ({})", diagnostic_reasons[0]) + } else { + String::new() + }; + format!( + "{} state event{} in purgatory from authorized publisher{} but doesn't match push{}", + authorized_count, + if authorized_count == 1 { "" } else { "s" }, + if authorized_count == 1 { "" } else { "s" }, + reason_summary + ) + } else { + format!( + "{} state event{} in purgatory but none from authorized publishers", + all_purgatory_states.len(), + if all_purgatory_states.len() == 1 { + "" + } else { + "s" + } + ) + }; + + warn!("Push rejected for {}: {}", identifier, summary); + return Ok(AuthorizationResult::denied(summary)); + } else { + debug!("No state events found in purgatory for {}", identifier); + warn!( + "Push rejected for {}: No state events in purgatory", + identifier + ); + return Ok(AuthorizationResult::denied("No state events in purgatory")); + } } // No matching state found in purgatory Ok(AuthorizationResult::denied( - "No state event found in purgatory from authorized publishers", + "No matching state event found in purgatory from authorized publishers", )) } diff --git a/src/git/handlers.rs b/src/git/handlers.rs index e3a6ad4..7244abb 100644 --- a/src/git/handlers.rs +++ b/src/git/handlers.rs @@ -254,7 +254,7 @@ pub async fn handle_receive_pack( } // GRASP Authorization Check - info!( + debug!( "Authorizing push for {} owned by {} via database query", identifier, owner_pubkey ); diff --git a/src/purgatory/helpers.rs b/src/purgatory/helpers.rs index 193ef99..a9f6e66 100644 --- a/src/purgatory/helpers.rs +++ b/src/purgatory/helpers.rs @@ -225,6 +225,117 @@ pub fn get_unpushed_refs(event: &Event, pushed_refs: &[RefPair]) -> Vec .collect() } +/// Diagnose why a state event doesn't match the push. +/// +/// Returns a human-readable explanation of the mismatch between the state event +/// and what would result from applying the push to local refs. +/// +/// # Arguments +/// * `event` - The state event to check +/// * `pushed_updates` - Ref updates in the current push operation +/// * `local_refs` - Refs already existing locally (ref_name -> SHA) +/// +/// # Returns +/// String explaining why the state doesn't match, or None if it matches +pub fn diagnose_state_mismatch( + event: &Event, + pushed_updates: &[RefUpdate], + local_refs: &HashMap, +) -> Option { + let state_refs = extract_refs_from_state(event); + + // Filter local_refs to only branches and tags + let mut would_be_state: HashMap = local_refs + .iter() + .filter(|(ref_name, _)| { + ref_name.starts_with("refs/heads/") || ref_name.starts_with("refs/tags/") + }) + .map(|(k, v)| (k.clone(), v.clone())) + .collect(); + + // Apply all pushed updates to create the would-be state + for update in pushed_updates { + // Only process branches and tags + if !update.ref_name.starts_with("refs/heads/") && !update.ref_name.starts_with("refs/tags/") + { + continue; + } + + if update.is_deletion() { + would_be_state.remove(&update.ref_name); + } else { + would_be_state.insert(update.ref_name.clone(), update.new_oid.clone()); + } + } + + // Convert event's state refs to a HashMap for comparison + let declared_state: HashMap = state_refs + .into_iter() + .map(|r| (r.ref_name, r.object_sha)) + .collect(); + + // Check if they match + if would_be_state == declared_state { + return None; // No mismatch + } + + // Build diagnostic message + let mut reasons = Vec::new(); + + // Check for refs in declared state but not in would-be state + for (ref_name, declared_sha) in &declared_state { + if let Some(would_be_sha) = would_be_state.get(ref_name) { + if would_be_sha != declared_sha { + let would_be_short = if would_be_sha.len() >= 8 { + &would_be_sha[..8] + } else { + would_be_sha.as_str() + }; + let declared_short = if declared_sha.len() >= 8 { + &declared_sha[..8] + } else { + declared_sha.as_str() + }; + reasons.push(format!( + "{} would be at {} but state declares {}", + ref_name, would_be_short, declared_short + )); + } + } else { + let declared_short = if declared_sha.len() >= 8 { + &declared_sha[..8] + } else { + declared_sha.as_str() + }; + reasons.push(format!( + "{} missing (state declares {})", + ref_name, declared_short + )); + } + } + + // Check for refs in would-be state but not in declared state + for (ref_name, would_be_sha) in &would_be_state { + if !declared_state.contains_key(ref_name) { + let would_be_short = if would_be_sha.len() >= 8 { + &would_be_sha[..8] + } else { + would_be_sha.as_str() + }; + reasons.push(format!( + "{} would exist at {} but state doesn't declare it", + ref_name, would_be_short + )); + } + } + + if reasons.is_empty() { + Some("Unknown mismatch".to_string()) + } else { + Some(reasons.join("; ")) + } +} + #[cfg(test)] mod tests { use super::*; @@ -695,4 +806,97 @@ mod tests { // Should return true - real OID exists, symbolic ref skipped assert!(can_apply_state(&event, repo_path)); } + + #[test] + fn test_diagnose_state_mismatch_missing_ref() { + // State declares both main and test branches + let event = create_test_state_event( + "test-repo", + vec![("refs/heads/main", "abc123"), ("refs/heads/test", "def456")], + ); + + // Push only creates test branch + let pushed_updates = vec![RefUpdate { + old_oid: "0000000000000000000000000000000000000000".to_string(), + new_oid: "def456".to_string(), + ref_name: "refs/heads/test".to_string(), + }]; + + // No local refs + let local_refs = HashMap::new(); + + let diagnosis = diagnose_state_mismatch(&event, &pushed_updates, &local_refs); + assert!(diagnosis.is_some()); + let msg = diagnosis.unwrap(); + assert!(msg.contains("refs/heads/main")); + assert!(msg.contains("missing")); + } + + #[test] + fn test_diagnose_state_mismatch_wrong_sha() { + // State declares main at abc123 + let event = create_test_state_event("test-repo", vec![("refs/heads/main", "abc123")]); + + // Push updates main to different SHA + let pushed_updates = vec![RefUpdate { + old_oid: "0000000000000000000000000000000000000000".to_string(), + new_oid: "wrong123".to_string(), + ref_name: "refs/heads/main".to_string(), + }]; + + let local_refs = HashMap::new(); + + let diagnosis = diagnose_state_mismatch(&event, &pushed_updates, &local_refs); + assert!(diagnosis.is_some()); + let msg = diagnosis.unwrap(); + assert!(msg.contains("refs/heads/main")); + assert!(msg.contains("would be at")); + assert!(msg.contains("state declares")); + } + + #[test] + fn test_diagnose_state_mismatch_extra_ref() { + // State declares only main + let event = create_test_state_event("test-repo", vec![("refs/heads/main", "abc123")]); + + // Push creates both main and test + let pushed_updates = vec![ + RefUpdate { + old_oid: "0000000000000000000000000000000000000000".to_string(), + new_oid: "abc123".to_string(), + ref_name: "refs/heads/main".to_string(), + }, + RefUpdate { + old_oid: "0000000000000000000000000000000000000000".to_string(), + new_oid: "def456".to_string(), + ref_name: "refs/heads/test".to_string(), + }, + ]; + + let local_refs = HashMap::new(); + + let diagnosis = diagnose_state_mismatch(&event, &pushed_updates, &local_refs); + assert!(diagnosis.is_some()); + let msg = diagnosis.unwrap(); + assert!(msg.contains("refs/heads/test")); + assert!(msg.contains("doesn't declare")); + } + + #[test] + fn test_diagnose_state_mismatch_no_mismatch() { + // State declares main + let event = create_test_state_event("test-repo", vec![("refs/heads/main", "abc123")]); + + // Push creates main at correct SHA + let pushed_updates = vec![RefUpdate { + old_oid: "0000000000000000000000000000000000000000".to_string(), + new_oid: "abc123".to_string(), + ref_name: "refs/heads/main".to_string(), + }]; + + let local_refs = HashMap::new(); + + let diagnosis = diagnose_state_mismatch(&event, &pushed_updates, &local_refs); + assert!(diagnosis.is_none()); // No mismatch + } } diff --git a/src/purgatory/mod.rs b/src/purgatory/mod.rs index 8094450..2c278f6 100644 --- a/src/purgatory/mod.rs +++ b/src/purgatory/mod.rs @@ -16,7 +16,7 @@ pub mod persistence; pub mod sync; mod types; -pub use helpers::{can_apply_state, can_satisfy_state, extract_refs_from_state, get_unpushed_refs}; +pub use helpers::{can_apply_state, can_satisfy_state, diagnose_state_mismatch, extract_refs_from_state, get_unpushed_refs}; pub use types::{EventSource, PrPurgatoryEntry, RefPair, RefUpdate, StatePurgatoryEntry}; use dashmap::DashMap; -- cgit v1.2.3