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) <noreply@anthropic.com>
This commit is contained in:
林 駿甫 (Shunsuke Hayashi) 2026-04-10 10:36:44 +09:00
parent 8b49926d6c
commit 9bf1b5e24c
3 changed files with 269 additions and 26 deletions

View file

@ -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()),

View file

@ -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"));
}
}

View file

@ -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<String> {
}
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<String> {
links
}
const MAX_VAULT_DEPTH: usize = 5;
fn resolve_wikilink(vault_path: &Path, link_name: &str) -> Option<PathBuf> {
// 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<PathBuf> {
fn find_note_by_name(directory: &Path, name: &str, depth: usize) -> Option<PathBuf> {
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());
}
}