diff --git a/CHANGELOG.md b/CHANGELOG.md index 19dd18a..b11eaef 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- Reject PR and PR-update events missing commit metadata as terminal invalid + events, avoiding repeated recovery attempts for immutable malformed data. + - Retire incomplete historic batches when transient requests time out or close before EOSE, and bound SDK subscription retention with an auto-close deadline. diff --git a/docs/explanation/architecture.md b/docs/explanation/architecture.md index c2a71d4..34884d3 100644 --- a/docs/explanation/architecture.md +++ b/docs/explanation/architecture.md @@ -398,6 +398,12 @@ The purgatory system solves two related problems: **Design Document**: See [`purgatory-design.md`](purgatory-design.md) for complete design specifications. +PR and PR-update events must include a commit value in their signed `c` tag. +Missing metadata is rejected as `invalid` before Git checks, so sync treats the +event as terminally accounted rather than repeatedly retrying an apparent +server failure. Events with commit metadata but unavailable Git objects remain +eligible for purgatory; Git and database failures remain retryable. + #### Architecture ```rust diff --git a/src/nostr/builder.rs b/src/nostr/builder.rs index 7a40df6..80568c4 100644 --- a/src/nostr/builder.rs +++ b/src/nostr/builder.rs @@ -551,6 +551,11 @@ impl Nip34WritePolicy { /// * `event` - The PR event to validate /// * `is_synced` - True if this event came from proactive sync (vs user-submitted) async fn handle_pr_event(&self, event: &Event, is_synced: bool) -> WritePolicyResult { + // Missing signed metadata cannot be repaired by fetching Git data or + // retrying another relay. Keep actual Git/database faults retryable. + if PrEventPolicy::commit(event).is_none() { + return reject_invalid(format!("PR event {} has no 'c' tag", event.id)); + } let event_id_str = event.id.to_bech32().unwrap_or_else(|_| event.id.to_hex()); // duplicate check in purgatory @@ -1176,3 +1181,80 @@ pub async fn create_relay( }, }) } + +#[cfg(test)] +mod pr_metadata_tests { + use super::*; + use crate::grasp06::receive::RepoInitLocks; + use std::path::PathBuf; + + #[tokio::test] + async fn missing_pr_commit_is_invalid_but_missing_git_data_enters_purgatory() { + let directory = tempfile::tempdir().unwrap(); + let mut config = Config::for_testing(); + config.grasp06_enable = true; + config.git_data_path = directory.path().join("git").to_string_lossy().into_owned(); + config.relay_data_path = directory + .path() + .join("relay") + .to_string_lossy() + .into_owned(); + let purgatory = Arc::new(crate::purgatory::Purgatory::new(PathBuf::from( + &config.git_data_path, + ))); + let runtime = create_relay(&config, purgatory, RepoInitLocks::default(), None) + .await + .unwrap(); + let keys = Keys::generate(); + let tags = vec![ + Tag::custom("a", [format!("30617:{}:test", keys.public_key().to_hex())]), + Tag::custom( + "clone", + [format!( + "https://{}/prs/{}/test.git", + config.service_address(), + keys.public_key().to_bech32().unwrap() + )], + ), + ]; + for kind in [Kind::GitPullRequest, Kind::GitPullRequestUpdate] { + for empty_tag in [false, true] { + let mut event_tags = tags.clone(); + if empty_tag { + event_tags.push(Tag::custom("c", Vec::::new())); + } + let event = EventBuilder::new(kind, "missing commit") + .tags(event_tags) + .finalize(&keys) + .unwrap(); + for synced in [false, true] { + let result = runtime.write_policy.handle_pr_event(&event, synced).await; + assert!( + matches!( + result, + WritePolicyResult::Reject { + prefix: MachineReadablePrefix::Invalid, + status: false, + .. + } + ), + "missing immutable metadata must be a terminal validation failure" + ); + } + } + let event = EventBuilder::new(kind, "awaiting Git data") + .tags(tags.clone()) + .tag(Tag::custom( + "c", + ["1111111111111111111111111111111111111111"], + )) + .finalize(&keys) + .unwrap(); + let result = runtime.write_policy.handle_pr_event(&event, true).await; + assert!( + matches!(result, WritePolicyResult::Reject { status: true, .. }), + "valid metadata with unavailable Git objects must remain eligible for purgatory" + ); + } + } +} diff --git a/src/nostr/policy/pr_event.rs b/src/nostr/policy/pr_event.rs index 9a78806..449b85d 100644 --- a/src/nostr/policy/pr_event.rs +++ b/src/nostr/policy/pr_event.rs @@ -30,6 +30,16 @@ impl PrEventPolicy { } } + /// Extract required commit metadata separately from fallible Git I/O. + pub(crate) fn commit(event: &Event) -> Option { + event.tags.iter().find_map(|tag| { + let values = tag.as_slice(); + (values.first().map(String::as_str) == Some("c")) + .then(|| values.get(1).cloned()) + .flatten() + }) + } + /// Check if git data exists for a PR event /// /// This unified method checks for git data existence and handles: @@ -46,15 +56,7 @@ impl PrEventPolicy { pub async fn git_data_check(&self, event: &Event) -> Result { let event_id = event.id.to_hex(); - // Extract the `c` tag (commit hash) from the PR event - let commit = event.tags.iter().find_map(|tag| { - let tag_vec = tag.clone().to_vec(); - if tag_vec.len() >= 2 && tag_vec[0] == "c" { - Some(tag_vec[1].clone()) - } else { - None - } - }); + let commit = Self::commit(event); let commit = match commit { Some(c) => c,