From 9bf1b5e24c4b2cd31d4c3d807da17731134566a6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E6=9E=97=20=E9=A7=BF=E7=94=AB=20=28Shunsuke=20Hayashi=29?= Date: Fri, 10 Apr 2026 10:36:44 +0900 Subject: [PATCH] fix(core): address review findings F1-F4 + add 10 missing tests F1: Add depth limit (max 5) to find_note_by_name and collect_obsidian_matches to prevent stack overflow on large vaults F2: Fix bus_enqueue to use store_path parent instead of cwd F3: Fix update_skill_md_from_patterns to preserve sections after the auto-generated marker instead of truncating them F4: Fix extract_wikilinks to consume chars after | in [[Note|Display]] so subsequent links parse correctly Tests added: - extract_wikilinks: basic, display text, empty/broken - find_note_by_name: depth limit enforcement - resolve_wikilink: direct, nested, non-existent - build_rejection_section: empty, sorted by count desc - update_skill_md_from_patterns: append, replace preserving following sections 955 tests GREEN, clippy zero. Closes #109 Co-Authored-By: Claude Opus 4.6 (1M context) --- crates/miyabi-cli/src/main.rs | 15 +-- crates/miyabi-core/src/dream.rs | 123 +++++++++++++++++++--- crates/miyabi-core/src/protocol.rs | 157 +++++++++++++++++++++++++++-- 3 files changed, 269 insertions(+), 26 deletions(-) diff --git a/crates/miyabi-cli/src/main.rs b/crates/miyabi-cli/src/main.rs index e44d0f8..5da28f6 100644 --- a/crates/miyabi-cli/src/main.rs +++ b/crates/miyabi-cli/src/main.rs @@ -1301,7 +1301,7 @@ fn handle_gate_command( println!("registered: {} ({})", task.id, task.title); } if !no_bus { - bus_enqueue(&task.id, &task_title); + bus_enqueue(&task.id, &task_title, store_path); } }) } @@ -2454,12 +2454,15 @@ fn truncate_str(s: &str, max_len: usize) -> String { } } -fn bus_enqueue(task_id: &str, title: &str) { - let skill_runs_path = std::env::current_dir() - .ok() - .map(|cwd| cwd.join("skills/self-improving-skills/skill-runs.jsonl")); +fn bus_enqueue(task_id: &str, title: &str, store_path: &std::path::Path) { + // Derive repo root from store_path (e.g. project_memory/tasks.json -> repo root) + let repo_root = store_path + .parent() + .and_then(|pm| pm.parent()) + .unwrap_or(std::path::Path::new(".")); + let skill_runs_path = repo_root.join("skills/self-improving-skills/skill-runs.jsonl"); - if let Some(path) = skill_runs_path.filter(|p| p.parent().is_some_and(|d| d.exists())) { + if let Some(path) = Some(skill_runs_path).filter(|p| p.parent().is_some_and(|d| d.exists())) { let entry = serde_json::json!({ "ts": chrono::Utc::now().to_rfc3339(), "agent": std::env::var("POLARIS_AGENT_ID").unwrap_or_else(|_| "system".into()), diff --git a/crates/miyabi-core/src/dream.rs b/crates/miyabi-core/src/dream.rs index 6a5bbc7..2639428 100644 --- a/crates/miyabi-core/src/dream.rs +++ b/crates/miyabi-core/src/dream.rs @@ -161,29 +161,35 @@ fn update_skill_md_from_patterns(report: &DreamReport, repo_root: &Path) -> Resu let existing = fs::read_to_string(&skill_path)?; let marker = "## よくある拒否パターン（自動生成）"; - if existing.contains(marker) { - // Already has auto-generated section — remove it for refresh - let before = existing.split(marker).next().unwrap_or(&existing); - let mut content = before.trim_end().to_string(); + let new_section = build_rejection_section(&report.patterns.gate_rejections); + + let content = if let Some(start_idx) = existing.find(marker) { + // Replace only the auto-generated section, preserving content after it + let after_marker = &existing[start_idx + marker.len()..]; + // Find the next ## heading (or EOF) to determine section boundary + let section_end = after_marker + .find("\n## ") + .map(|i| start_idx + marker.len() + i + 1) // +1 to keep the newline before next heading + .unwrap_or(existing.len()); + let mut content = existing[..start_idx].trim_end().to_string(); content.push_str("\n\n"); - content.push_str(&build_rejection_section(&report.patterns.gate_rejections)); - content.push('\n'); - let tmp = skill_path.with_extension("md.tmp"); - fs::write(&tmp, &content)?; - fs::rename(&tmp, &skill_path)?; + content.push_str(&new_section); + content.push_str(&existing[section_end..]); + content } else { let mut content = existing; if !content.ends_with('\n') { content.push('\n'); } content.push('\n'); - content.push_str(&build_rejection_section(&report.patterns.gate_rejections)); + content.push_str(&new_section); content.push('\n'); - let tmp = skill_path.with_extension("md.tmp"); - fs::write(&tmp, &content)?; - fs::rename(&tmp, &skill_path)?; - } + content + }; + let tmp = skill_path.with_extension("md.tmp"); + fs::write(&tmp, &content)?; + fs::rename(&tmp, &skill_path)?; Ok(()) } @@ -638,4 +644,93 @@ mod tests { && args.first() == Some(&"commit".to_string())) ); } + + #[test] + fn build_rejection_section_empty() { + let section = build_rejection_section(&HashMap::new()); + assert!(section.contains("| GATE | 回数 | 対処法 |")); + // heading + blank + table header + separator = 4 lines, no data rows + assert_eq!(section.lines().count(), 4); + } + + #[test] + fn build_rejection_section_sorted_by_count_desc() { + let mut rejections = HashMap::new(); + rejections.insert("GATE 0".to_string(), 1); + rejections.insert("GATE 3".to_string(), 5); + rejections.insert("GATE 4".to_string(), 3); + + let section = build_rejection_section(&rejections); + let lines: Vec<&str> = section.lines().collect(); + // lines: [0]=heading, [1]=blank, [2]=table header, [3]=separator, [4..]=data + assert!(lines[4].contains("GATE 3")); + assert!(lines[4].contains("5")); + assert!(lines[5].contains("GATE 4")); + assert!(lines[6].contains("GATE 0")); + } + + #[test] + fn update_skill_md_appends_when_no_marker() { + let tmp = TempDir::new().unwrap(); + let skills_dir = tmp.path().join("skills/polaris-ops"); + fs::create_dir_all(&skills_dir).unwrap(); + let skill_path = skills_dir.join("SKILL.md"); + fs::write(&skill_path, "# Polaris Ops\n\nExisting content.\n").unwrap(); + + let mut rejections = HashMap::new(); + rejections.insert("GATE 3".to_string(), 2); + let report = DreamReport { + patterns: DreamPatterns { + gate_rejections: rejections, + ..Default::default() + }, + learnings: Vec::new(), + events_processed: 0, + }; + + update_skill_md_from_patterns(&report, tmp.path()).unwrap(); + let content = fs::read_to_string(&skill_path).unwrap(); + assert!(content.contains("# Polaris Ops")); + assert!(content.contains("Existing content.")); + assert!(content.contains("## よくある拒否パターン（自動生成）")); + assert!(content.contains("GATE 3")); + } + + #[test] + fn update_skill_md_replaces_marker_preserves_following_sections() { + let tmp = TempDir::new().unwrap(); + let skills_dir = tmp.path().join("skills/polaris-ops"); + fs::create_dir_all(&skills_dir).unwrap(); + let skill_path = skills_dir.join("SKILL.md"); + fs::write( + &skill_path, + "# Polaris Ops\n\n\ + ## よくある拒否パターン（自動生成）\n\n\ + | GATE | 回数 | 対処法 |\n|------|------|--------|\n| old | 1 | old |\n\n\ + ## 関連スキル\n\n- important link\n", + ) + .unwrap(); + + let mut rejections = HashMap::new(); + rejections.insert("GATE 4".to_string(), 7); + let report = DreamReport { + patterns: DreamPatterns { + gate_rejections: rejections, + ..Default::default() + }, + learnings: Vec::new(), + events_processed: 0, + }; + + update_skill_md_from_patterns(&report, tmp.path()).unwrap(); + let content = fs::read_to_string(&skill_path).unwrap(); + // Old data should be gone + assert!(!content.contains("| old |")); + // New data should be present + assert!(content.contains("GATE 4")); + assert!(content.contains("7")); + // Following section MUST be preserved + assert!(content.contains("## 関連スキル")); + assert!(content.contains("- important link")); + } } diff --git a/crates/miyabi-core/src/protocol.rs b/crates/miyabi-core/src/protocol.rs index 096ab1f..d7a7292 100644 --- a/crates/miyabi-core/src/protocol.rs +++ b/crates/miyabi-core/src/protocol.rs @@ -507,6 +507,7 @@ impl DeterministicExecutionProtocol { |task| { let from = task.current_state; task.current_state = TaskState::Done; + task.completion_mode = CompletionMode::Manual; Ok(serde_json::json!({ "from": from, "to": TaskState::Done, @@ -1452,7 +1453,7 @@ fn find_obsidian_notes( } let mut matches = Vec::new(); - collect_obsidian_matches(vault_path, &keywords, &mut matches)?; + collect_obsidian_matches(vault_path, &keywords, &mut matches, MAX_VAULT_DEPTH)?; matches.sort_by(|left, right| { right .0 @@ -1467,12 +1468,16 @@ fn collect_obsidian_matches( directory: &Path, keywords: &[String], matches: &mut Vec<(usize, PathBuf)>, + depth: usize, ) -> Result<(), Error> { + if depth == 0 { + return Ok(()); + } for entry in fs::read_dir(directory)? { let entry = entry?; let path = entry.path(); if path.is_dir() { - collect_obsidian_matches(&path, keywords, matches)?; + collect_obsidian_matches(&path, keywords, matches, depth - 1)?; continue; } if path.extension().and_then(|ext| ext.to_str()) != Some("md") { @@ -1509,6 +1514,12 @@ fn extract_wikilinks(content: &str) -> Vec { } if inner == '|' { // [[Note|Display]] — take the note part before | + // consume remaining chars until ']' so parser stays aligned + for remaining in chars.by_ref() { + if remaining == ']' { + break; + } + } break; } name.push(inner); @@ -1522,23 +1533,28 @@ fn extract_wikilinks(content: &str) -> Vec { links } +const MAX_VAULT_DEPTH: usize = 5; + fn resolve_wikilink(vault_path: &Path, link_name: &str) -> Option { // Try exact match first let exact = vault_path.join(format!("{link_name}.md")); if exact.exists() { return Some(exact); } - // Search recursively for the note - find_note_by_name(vault_path, link_name) + // Search recursively for the note (depth-limited) + find_note_by_name(vault_path, link_name, MAX_VAULT_DEPTH) } -fn find_note_by_name(directory: &Path, name: &str) -> Option { +fn find_note_by_name(directory: &Path, name: &str, depth: usize) -> Option { + if depth == 0 { + return None; + } let target = format!("{}.md", name.to_ascii_lowercase()); for entry in fs::read_dir(directory).ok()? { let entry = entry.ok()?; let path = entry.path(); if path.is_dir() { - if let Some(found) = find_note_by_name(&path, name) { + if let Some(found) = find_note_by_name(&path, name, depth - 1) { return Some(found); } } else if path @@ -2617,6 +2633,73 @@ mod tests { .unwrap(); assert_eq!(task.current_state, TaskState::Done); + assert_eq!(task.completion_mode, CompletionMode::Manual); + } + + #[test] + fn full_lifecycle_register_to_merged_releases_lock() { + let (_tmp, protocol) = fixture(); + protocol + .register( + RegisterTaskRequest { + issue: 1, + task_id: "e2e-lifecycle".into(), + title: "E2E lifecycle".into(), + dependencies: vec![], + soft_dependencies: vec![], + priority: 0, + completion_mode: CompletionMode::GithubPr, + }, + "test-agent", + "test-node", + ) + .unwrap(); + protocol + .record_impact( + "e2e-lifecycle", + ImpactInput { + risk_level: ImpactRiskLevel::Low, + affected_symbols: 1, + depth1: vec!["e2e".into()], + analyzed_commit: None, + input_hash: None, + approve: false, + }, + "test-agent", + "test-node", + ) + .unwrap(); + protocol + .assign( + "e2e-lifecycle", + "test-agent", + "test-node", + &[String::from("src/test.rs")], + ) + .unwrap(); + protocol + .record_branch( + "e2e-lifecycle", + "feature/issue-1-test", + "test-agent", + "test-node", + ) + .unwrap(); + protocol + .record_pr("e2e-lifecycle", 42, "test-agent", "test-node") + .unwrap(); + let merged = protocol + .record_merge( + "e2e-lifecycle", + "0123456789abcdef0123456789abcdef01234567", + "test-agent", + "test-node", + ) + .unwrap(); + + assert_eq!(merged.current_state, TaskState::Merged); + assert!(merged.lock.is_none()); + assert!(protocol.locks().unwrap().is_empty()); } #[test] @@ -2671,4 +2754,66 @@ mod tests { }; assert!(after_heartbeat > before_heartbeat); } + + #[test] + fn extract_wikilinks_basic() { + assert_eq!( + extract_wikilinks("See [[Note A]] and [[Note B]]"), + vec!["Note A", "Note B"] + ); + } + + #[test] + fn extract_wikilinks_with_display_text() { + // After fixing F4, [[A|B]] should yield "A" and not corrupt next link + assert_eq!( + extract_wikilinks("[[Foo|display text]] then [[Bar]]"), + vec!["Foo", "Bar"] + ); + } + + #[test] + fn extract_wikilinks_empty_and_broken() { + assert!(extract_wikilinks("no links here").is_empty()); + assert!(extract_wikilinks("[[]]").is_empty()); // empty name + assert!(extract_wikilinks("[[ ]]").is_empty()); // whitespace only + // Unclosed link: should still extract the name + assert_eq!(extract_wikilinks("[[unclosed"), vec!["unclosed"]); + } + + #[test] + fn find_note_by_name_respects_depth_limit() { + let tmp = TempDir::new().unwrap(); + // Create a deeply nested note: a/b/c/d/e/f/g/note.md + let deep_dir = tmp.path().join("a/b/c/d/e/f/g"); + fs::create_dir_all(&deep_dir).unwrap(); + fs::write(deep_dir.join("target.md"), "deep note").unwrap(); + + // depth=5 should NOT find it (7 levels deep) + assert!(find_note_by_name(tmp.path(), "target", 5).is_none()); + + // depth=8 should find it + assert!(find_note_by_name(tmp.path(), "target", 8).is_some()); + + // depth=0 always returns None + assert!(find_note_by_name(tmp.path(), "target", 0).is_none()); + } + + #[test] + fn resolve_wikilink_finds_direct_and_nested() { + let tmp = TempDir::new().unwrap(); + + // Direct file + fs::write(tmp.path().join("Direct.md"), "content").unwrap(); + assert!(resolve_wikilink(tmp.path(), "Direct").is_some()); + + // Nested file + let sub = tmp.path().join("sub"); + fs::create_dir(&sub).unwrap(); + fs::write(sub.join("Nested.md"), "content").unwrap(); + assert!(resolve_wikilink(tmp.path(), "Nested").is_some()); + + // Non-existent + assert!(resolve_wikilink(tmp.path(), "NonExistent").is_none()); + } }