Skip to content

Commit da9bcd8

Browse files
joshwilhelmiclaude
andcommitted
[gobby-cli-#466] fix: stop gwiki research on genuine write conflict
ResearchStopReason::WriteConflict was a dead variant: the materializer set a write_conflict bool on legitimate title-slug collisions, OR- accumulated it into the output, and the loop continued — contradicting docs/contracts/gwiki-research.md ("stop on the first matching rule", "exits without overwriting"). research_note_file_state now compares the on-disk body of a completed, draft-id-matching note against the validated draft and returns a new CompletedConflict state on mismatch (a concurrent writer changed it). write_accepted_note surfaces that as write_conflict without overwriting; the loop maps it to StepOutcome::Stop(WriteConflict) (status Failed) and records neither the note nor a changed path. Numeric-suffix title collisions and create-marker races are no longer reported as conflicts. Tests: write layer (draft-id collision with changed body -> write_conflict, no overwrite, no suffix note) and loop layer (run stops with stop_reason write_conflict, nothing recorded). Companion to gobby epic #15586 / M4 #15590. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 40b86ac commit da9bcd8

2 files changed

Lines changed: 224 additions & 35 deletions

File tree

crates/gwiki/src/research.rs

Lines changed: 125 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -763,14 +763,15 @@ fn write_accepted_note(
763763
})?;
764764

765765
let draft_id = accepted_note_draft_id(note);
766-
if let Some(path) = find_completed_accepted_note(&research_dir, &draft_id)? {
766+
let expected_body = note.body.trim();
767+
if let Some(found) = find_completed_accepted_note(&research_dir, &draft_id, expected_body)? {
767768
return Ok(AcceptedNoteWrite {
768769
note: AcceptedResearchNote {
769770
title: note.title.clone(),
770-
path,
771+
path: found.path,
771772
},
772773
created: false,
773-
write_conflict: false,
774+
write_conflict: found.write_conflict,
774775
});
775776
}
776777

@@ -824,7 +825,13 @@ enum AcceptedNoteSlot {
824825
enum ResearchNoteFileState {
825826
Missing,
826827
CompletedMatching,
827-
MaterializingMatching { stale: bool },
828+
/// A completed note carries our draft id but its on-disk body differs from
829+
/// the validated draft — a concurrent writer changed it. The run must stop
830+
/// without overwriting rather than dedup to tampered content.
831+
CompletedConflict,
832+
MaterializingMatching {
833+
stale: bool,
834+
},
828835
Occupied,
829836
}
830837

@@ -865,19 +872,25 @@ fn create_materializing_research_note(
865872
) -> Result<AcceptedNoteSlot, WikiError> {
866873
let title = &note.title;
867874
let slug = slugify(title);
868-
let mut write_conflict = false;
875+
let expected_body = note.body.trim();
869876
for attempt in 1..=MAX_RESEARCH_NOTE_SUFFIX_ATTEMPTS {
870877
let file_name = if attempt == 1 {
871878
format!("{slug}.md")
872879
} else {
873880
format!("{slug}-{attempt}.md")
874881
};
875882
let path = research_dir.join(file_name);
876-
match research_note_file_state(&path, draft_id)? {
883+
match research_note_file_state(&path, draft_id, expected_body)? {
877884
ResearchNoteFileState::CompletedMatching => {
878885
return Ok(AcceptedNoteSlot::Existing {
879886
path,
880-
write_conflict,
887+
write_conflict: false,
888+
});
889+
}
890+
ResearchNoteFileState::CompletedConflict => {
891+
return Ok(AcceptedNoteSlot::Existing {
892+
path,
893+
write_conflict: true,
881894
});
882895
}
883896
ResearchNoteFileState::MaterializingMatching { stale } if stale => {
@@ -888,16 +901,19 @@ fn create_materializing_research_note(
888901
})?;
889902
}
890903
ResearchNoteFileState::MaterializingMatching { .. } => {
891-
if let Some(path) = wait_for_materializing_research_note(&path, draft_id, title)? {
904+
if let Some((path, write_conflict)) =
905+
wait_for_materializing_research_note(&path, draft_id, expected_body, title)?
906+
{
892907
return Ok(AcceptedNoteSlot::Existing {
893908
path,
894909
write_conflict,
895910
});
896911
}
897912
continue;
898913
}
914+
// A different note occupying this title slug is a legitimate
915+
// collision, not a write conflict — bump the numeric suffix.
899916
ResearchNoteFileState::Occupied => {
900-
write_conflict = true;
901917
continue;
902918
}
903919
ResearchNoteFileState::Missing => {}
@@ -924,11 +940,12 @@ fn create_materializing_research_note(
924940
}
925941
return Ok(AcceptedNoteSlot::Materializing {
926942
path,
927-
write_conflict,
943+
write_conflict: false,
928944
});
929945
}
946+
// Another writer created this marker between our check and create —
947+
// a slot race, not a content conflict. Try the next suffix.
930948
Err(error) if error.kind() == std::io::ErrorKind::AlreadyExists => {
931-
write_conflict = true;
932949
continue;
933950
}
934951
Err(error) => {
@@ -949,13 +966,19 @@ fn create_materializing_research_note(
949966
fn wait_for_materializing_research_note(
950967
path: &Path,
951968
draft_id: &str,
969+
expected_body: &str,
952970
title: &str,
953-
) -> Result<Option<PathBuf>, WikiError> {
971+
) -> Result<Option<(PathBuf, bool)>, WikiError> {
954972
let started = Instant::now();
955973
let mut delay = RESEARCH_NOTE_MATERIALIZE_INITIAL_DELAY;
956974
loop {
957-
match research_note_file_state(path, draft_id)? {
958-
ResearchNoteFileState::CompletedMatching => return Ok(Some(path.to_path_buf())),
975+
match research_note_file_state(path, draft_id, expected_body)? {
976+
ResearchNoteFileState::CompletedMatching => {
977+
return Ok(Some((path.to_path_buf(), false)));
978+
}
979+
ResearchNoteFileState::CompletedConflict => {
980+
return Ok(Some((path.to_path_buf(), true)));
981+
}
959982
ResearchNoteFileState::Missing | ResearchNoteFileState::Occupied => return Ok(None),
960983
ResearchNoteFileState::MaterializingMatching { stale } if stale => {
961984
fs::remove_file(path).map_err(|error| WikiError::Io {
@@ -998,10 +1021,16 @@ fn accepted_note_draft_id(note: &AcceptedNoteDraft) -> String {
9981021
uuid::Uuid::new_v5(&RESEARCH_NOTE_NAMESPACE, key.as_bytes()).to_string()
9991022
}
10001023

1024+
struct CompletedAcceptedNote {
1025+
path: PathBuf,
1026+
write_conflict: bool,
1027+
}
1028+
10011029
fn find_completed_accepted_note(
10021030
research_dir: &Path,
10031031
draft_id: &str,
1004-
) -> Result<Option<PathBuf>, WikiError> {
1032+
expected_body: &str,
1033+
) -> Result<Option<CompletedAcceptedNote>, WikiError> {
10051034
let entries = match fs::read_dir(research_dir) {
10061035
Ok(entries) => entries,
10071036
Err(error) if error.kind() == ErrorKind::NotFound => return Ok(None),
@@ -1024,11 +1053,20 @@ fn find_completed_accepted_note(
10241053
if path.extension().and_then(|value| value.to_str()) != Some("md") {
10251054
continue;
10261055
}
1027-
if matches!(
1028-
research_note_file_state(&path, draft_id)?,
1029-
ResearchNoteFileState::CompletedMatching
1030-
) {
1031-
return Ok(Some(path));
1056+
match research_note_file_state(&path, draft_id, expected_body)? {
1057+
ResearchNoteFileState::CompletedMatching => {
1058+
return Ok(Some(CompletedAcceptedNote {
1059+
path,
1060+
write_conflict: false,
1061+
}));
1062+
}
1063+
ResearchNoteFileState::CompletedConflict => {
1064+
return Ok(Some(CompletedAcceptedNote {
1065+
path,
1066+
write_conflict: true,
1067+
}));
1068+
}
1069+
_ => continue,
10321070
}
10331071
}
10341072
Ok(None)
@@ -1037,6 +1075,7 @@ fn find_completed_accepted_note(
10371075
fn research_note_file_state(
10381076
path: &Path,
10391077
draft_id: &str,
1078+
expected_body: &str,
10401079
) -> Result<ResearchNoteFileState, WikiError> {
10411080
let contents = match fs::read_to_string(path) {
10421081
Ok(contents) => contents,
@@ -1058,7 +1097,10 @@ fn research_note_file_state(
10581097
return Ok(ResearchNoteFileState::Occupied);
10591098
}
10601099
if yaml_field_eq(frontmatter, "research_status", "completed") {
1061-
return Ok(ResearchNoteFileState::CompletedMatching);
1100+
if research_note_body_matches(&contents, expected_body) {
1101+
return Ok(ResearchNoteFileState::CompletedMatching);
1102+
}
1103+
return Ok(ResearchNoteFileState::CompletedConflict);
10621104
}
10631105
if yaml_field_eq(frontmatter, "research_status", "materializing") {
10641106
return Ok(ResearchNoteFileState::MaterializingMatching {
@@ -1076,6 +1118,26 @@ fn frontmatter_block(markdown: &str) -> Option<&str> {
10761118
Some(&rest[..end])
10771119
}
10781120

1121+
/// Extract the markdown body (the content after the frontmatter fence) of an
1122+
/// accepted research note, trimmed. Returns `None` for malformed notes that
1123+
/// lack a closing fence.
1124+
fn research_note_body(markdown: &str) -> Option<&str> {
1125+
let rest = markdown
1126+
.strip_prefix("---\n")
1127+
.or_else(|| markdown.strip_prefix("---\r\n"))?;
1128+
let end = rest.find("\n---").or_else(|| rest.find("\r\n---"))?;
1129+
let after_fence = rest[end..].trim_start_matches(['\r', '\n']);
1130+
let body = after_fence.strip_prefix("---")?;
1131+
Some(body.trim())
1132+
}
1133+
1134+
/// Whether an on-disk completed note's body matches the validated draft body.
1135+
/// A malformed note (no extractable body) is treated as a mismatch so the run
1136+
/// stops rather than dedups to tampered content.
1137+
fn research_note_body_matches(contents: &str, expected_body: &str) -> bool {
1138+
research_note_body(contents).is_some_and(|body| body == expected_body.trim())
1139+
}
1140+
10791141
fn yaml_field_eq(frontmatter: &str, key: &str, value: &str) -> bool {
10801142
let plain = format!("{key}: {value}");
10811143
let quoted = format!("{key}: \"{value}\"");
@@ -1420,8 +1482,49 @@ mod tests {
14201482
second.note.path.file_name().and_then(|name| name.to_str()),
14211483
Some("same-title-2.md")
14221484
);
1485+
// A different note sharing the title slug is a legitimate numeric-suffix
1486+
// collision, not a write conflict.
14231487
assert!(!first.write_conflict);
1424-
assert!(second.write_conflict);
1488+
assert!(!second.write_conflict);
1489+
}
1490+
1491+
#[test]
1492+
fn accepted_note_draft_collision_with_changed_body_is_write_conflict() {
1493+
let temp = tempfile::tempdir().expect("tempdir");
1494+
let root = temp.path();
1495+
let research_dir = root.join("raw/research");
1496+
std::fs::create_dir_all(&research_dir).expect("research dir");
1497+
1498+
let draft = AcceptedNoteDraft {
1499+
title: "Concurrent note".to_string(),
1500+
body: "the validated draft body".to_string(),
1501+
sources: Vec::new(),
1502+
};
1503+
let draft_id = accepted_note_draft_id(&draft);
1504+
// Simulate a concurrent writer: a completed note carrying our draft id
1505+
// but a body that changed since draft validation.
1506+
let tampered = AcceptedNoteDraft {
1507+
title: draft.title.clone(),
1508+
body: "a different body written by another process".to_string(),
1509+
sources: draft.sources.clone(),
1510+
};
1511+
let path = research_dir.join("concurrent-note.md");
1512+
let on_disk =
1513+
render_accepted_note_body("research-other", &tampered, &draft_id, "completed", true)
1514+
.expect("tampered note body");
1515+
std::fs::write(&path, &on_disk).expect("write tampered note");
1516+
1517+
let result = write_accepted_note(root, "research-1", &draft).expect("write result");
1518+
1519+
assert!(result.write_conflict);
1520+
assert!(!result.created);
1521+
assert_eq!(result.note.path, path);
1522+
// The existing note is not overwritten and no suffix-bumped note is made.
1523+
assert_eq!(
1524+
std::fs::read_to_string(&path).expect("note still present"),
1525+
on_disk
1526+
);
1527+
assert!(!research_dir.join("concurrent-note-2.md").exists());
14251528
}
14261529

14271530
#[test]

0 commit comments

Comments
 (0)