From 937cf4c33191ba14ad2d739ec6284fd260a92f68 Mon Sep 17 00:00:00 2001 From: Travis James Date: Sat, 3 Oct 2026 14:18:14 -0500 Subject: [PATCH 01/10] docs(openspec): propose preserve-wiki-index for issue #15 Approves fs2 for pk-store (already a workspace dependency; one new edge in Cargo.lock, no new crate). Refs #15 Co-Authored-By: Claude Opus 5.5 --- Cargo.lock | 1 + .../changes/preserve-wiki-index/design.md | 23 +++++++++++++ .../changes/preserve-wiki-index/proposal.md | 33 +++++++++++++++++++ .../specs/wiki-index/spec.md | 30 +++++++++++++++++ openspec/changes/preserve-wiki-index/tasks.md | 16 +++++++++ pk-store/Cargo.toml | 1 + 6 files changed, 104 insertions(+) create mode 100644 openspec/changes/preserve-wiki-index/design.md create mode 100644 openspec/changes/preserve-wiki-index/proposal.md create mode 100644 openspec/changes/preserve-wiki-index/specs/wiki-index/spec.md create mode 100644 openspec/changes/preserve-wiki-index/tasks.md diff --git a/Cargo.lock b/Cargo.lock index 10a13eb..0390154 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1363,6 +1363,7 @@ version = "1.9.0" dependencies = [ "anyhow", "chrono", + "fs2", "pk-core", "pulldown-cmark", "serde", diff --git a/openspec/changes/preserve-wiki-index/design.md b/openspec/changes/preserve-wiki-index/design.md new file mode 100644 index 0000000..e0db50e --- /dev/null +++ b/openspec/changes/preserve-wiki-index/design.md @@ -0,0 +1,23 @@ +## Lock + +An exclusive `fs2` lock on `wiki/.index.lock`, blocking, held across scan, merge, render and write. Blocking (not skipping) is right here: a skipped rebuild would leave a stale index, while the wait is bounded by one scan and one write. The lock file is not a concept page: it lacks the `.md` extension, so `scan_wiki_tree` ignores it. + +## Fresh scan + +Rendering calls `scan_wiki_tree(&self.wiki_dir)` inside the lock. The in-memory store is not updated from it; only the index is derived from disk. Two processes that opened the store before either upserted therefore both end up listed, whichever writes last. + +## Preserving unparseable pages + +The existing `index.md` is read inside the lock. An entry line `* [Title](/id.md)…` is carried into the new index, verbatim, under its existing `## Section` when: +- its target file `wiki/.md` exists on disk, and +- `` is not among the entries this binary parsed. + +Parsed entries are always rendered fresh from their frontmatter, so they never duplicate a carried line. Entries whose target file no longer exists are dropped, which is how deletion works. The merge is a pure function in `bundle.rs`, testable without a store. + +## Atomic writes + +Write to a uniquely named temporary file in `wiki/`, sync it, then rename over the target. A killed process leaves either the old file or the new one, never a truncated one. `log.md` gets the same treatment because `append_log` has the same exposure. + +## What this cannot fix + +A binary older than this change (such as the deployed 1.8.0 worker) still rewrites the index destructively. The remedy for that is operational: same-version installs, tracked in the skill-system handoff note. diff --git a/openspec/changes/preserve-wiki-index/proposal.md b/openspec/changes/preserve-wiki-index/proposal.md new file mode 100644 index 0000000..1120c27 --- /dev/null +++ b/openspec/changes/preserve-wiki-index/proposal.md @@ -0,0 +1,33 @@ +## Why + +`pk ingest` and the learning worker can replace `wiki/index.md` with a version that has no `okf_version` frontmatter and only the newest page type's section. Every other entry is dropped, though its page still exists (GitHub issue [#15](https://github.com/Prometheus-AGS/prometheus-knowledge-rs/issues/15), about 90 `## Reference` entries lost in one project). + +The trigger on the reporter's setup, and on the maintainer's machine, is version skew: `pk` is 1.9.0 while `prometheus-learning-worker` is 1.8.0, installed by the skill system from its 1.8.0 submodule pin. A 1.8.0 binary cannot parse pages written in OKF v0.2's mapping-form `sources` (1.9.0 changed `WikiEntry.sources` from `Vec` to `Vec`). `scan_wiki_tree` skips those pages as parse failures, and the 1.8.0 `render_index` writes no frontmatter, so its rebuild overwrites the index with only the pages it can read. + +The code on `main` still allows the same loss: +- **Parse failures vanish.** `MarkdownStore::regenerate_index` renders only from parsed entries. +- **Stale snapshot.** It renders from the snapshot taken when the process opened the store, so overlapping ingests drop each other's pages. +- **Non-atomic write.** It writes in place, so a run killed mid-write can truncate the file. + +## What Changes + +- `regenerate_index` holds an exclusive advisory lock (`wiki/.index.lock`) for the whole read, render and write, so concurrent writers serialize. +- It renders from a fresh scan of the wiki tree inside the lock, not from the process's open-time snapshot. +- Index entries whose target page still exists on disk but cannot be parsed by this binary are preserved in their existing section. Entries whose page is gone are dropped. +- `index.md` and `log.md` are written atomically (temporary file, then rename). + +## Capabilities + +### New Capabilities +- `wiki-index`: maintaining the OKF v0.2 bundle-root `index.md` without losing entries or frontmatter. + +### Modified Capabilities +(none) + +## Impact + +- **Code**: `pk-store/src/store.rs` (`regenerate_index`, `append_log`) and `pk-store/src/bundle.rs` (a pure merge helper beside `render_index`). +- **Dependencies**: `pk-store` gains `fs2`, already a workspace dependency used by `pk-learning-worker`. No new crate enters `Cargo.lock`. +- **Contracts**: no frozen surface changes. The `index.md` format is unchanged, `regenerate_index` keeps its signature, and `pk-cli` and `pk-learning-worker` call it as before. No semver impact. +- **Consumers**: the fix only protects KBs once every writer is at least this version. The skill system must bump its 1.8.0 pin and install `pk`, `pk-cherry` and the worker from one commit; see `.agent-team/prometheus-knowledge-maintainers/handoffs/issue-15-skill-system.md`. +- **GitHub**: closes issue #15. diff --git a/openspec/changes/preserve-wiki-index/specs/wiki-index/spec.md b/openspec/changes/preserve-wiki-index/specs/wiki-index/spec.md new file mode 100644 index 0000000..e4d2685 --- /dev/null +++ b/openspec/changes/preserve-wiki-index/specs/wiki-index/spec.md @@ -0,0 +1,30 @@ +## Purpose + +Keeps the OKF v0.2 bundle-root `index.md` complete: rebuilding it never drops an entry whose page still exists, never drops the `okf_version` declaration, and never leaves a truncated file. + +## ADDED Requirements + +### Requirement: Rebuilding the index preserves pages this binary cannot parse +When the index is rebuilt, an existing index entry whose target page exists on disk but cannot be parsed SHALL be kept under its existing section. + +#### Scenario: Page written in a format this binary does not understand +- **WHEN** `index.md` lists a page whose frontmatter this binary fails to parse, and the index is rebuilt +- **THEN** that entry is still listed, under the same section heading + +#### Scenario: Page deleted from disk +- **WHEN** `index.md` lists a page whose file no longer exists, and the index is rebuilt +- **THEN** that entry is no longer listed + +### Requirement: Rebuilds reflect every page on disk +The index SHALL be rendered from the pages on disk at the time of the rebuild, not from the set known when the process started, and concurrent rebuilds SHALL be serialized. + +#### Scenario: Two writers opened before either ingested +- **WHEN** two processes open the same wiki, each ingests a different page, and each rebuilds the index +- **THEN** the final index lists both pages + +### Requirement: The index keeps its version declaration and is never truncated +Every rebuilt index SHALL begin with the `okf_version: "0.2"` frontmatter block, and `index.md` and `log.md` SHALL be replaced atomically. + +#### Scenario: Rebuild of an index that declares its version +- **WHEN** the index is rebuilt +- **THEN** it still begins with the `okf_version` block, followed by `# Wiki Index` diff --git a/openspec/changes/preserve-wiki-index/tasks.md b/openspec/changes/preserve-wiki-index/tasks.md new file mode 100644 index 0000000..8042e0d --- /dev/null +++ b/openspec/changes/preserve-wiki-index/tasks.md @@ -0,0 +1,16 @@ +## 1. Store (core-engineer) + +- [ ] 1.1 RED: store tests for a preserved unparseable entry, two stale writers, frontmatter retention, and a dropped deleted page +- [ ] 1.2 GREEN: lock, fresh scan, merge helper and atomic writes in `pk-store` + +## 2. Worker boundary (learning-worker-engineer) + +- [ ] 2.1 Regression test: a worker `run-once` keeps unparseable entries and the `okf_version` block + +## 3. Verification (verifier) + +- [ ] 3.1 Adversarial review and the serialized integration gate + +## 4. Downstream (knowledge-lead) + +- [ ] 4.1 Skill-system handoff note: pin bump and same-version installs diff --git a/pk-store/Cargo.toml b/pk-store/Cargo.toml index 9ce0b73..3aa76ba 100644 --- a/pk-store/Cargo.toml +++ b/pk-store/Cargo.toml @@ -17,6 +17,7 @@ tokio = { workspace = true } pulldown-cmark = { workspace = true } tracing = { workspace = true } sha2 = { workspace = true } +fs2 = { workspace = true } [dev-dependencies] tempfile = "3" From 5e71337e0054a0b0a79cf3788c766ed44dac746d Mon Sep 17 00:00:00 2001 From: Travis James Date: Sat, 3 Oct 2026 14:21:05 -0500 Subject: [PATCH 02/10] test(store): rebuilding index.md keeps unparseable pages, stale writers and frontmatter (RED) Refs #15 Co-Authored-By: Claude Opus 5.5 --- pk-store/tests/store_tests.rs | 133 ++++++++++++++++++++++++++++++++++ 1 file changed, 133 insertions(+) diff --git a/pk-store/tests/store_tests.rs b/pk-store/tests/store_tests.rs index 391c988..3ed04c8 100644 --- a/pk-store/tests/store_tests.rs +++ b/pk-store/tests/store_tests.rs @@ -542,3 +542,136 @@ async fn distinct_content_still_creates_a_new_entry() { assert_ne!(second.id, first.id); assert_eq!(second.revision, 0); } + +// ── GitHub issue #15: rebuilding index.md must not destroy entries ────────── + +/// A page whose frontmatter this binary rejects: `tags` must be a sequence, +/// so a mapping fails YAML deserialization in `markdown_to_entry`. +const UNPARSEABLE_PAGE: &str = + "---\nid: zeta-format\ntitle: Zeta Format\ntags:\n nested: {a: 1}\n---\n\nBody.\n"; + +#[tokio::test] +async fn rebuilding_index_keeps_pages_this_binary_cannot_parse() { + let dir = tempfile::tempdir().expect("tempdir"); + let wiki = dir.path().join("wiki"); + tokio::fs::create_dir_all(&wiki).await.unwrap(); + tokio::fs::write(wiki.join("zeta-format.md"), UNPARSEABLE_PAGE) + .await + .unwrap(); + tokio::fs::write( + wiki.join("index.md"), + "---\nokf_version: \"0.2\"\n---\n\n# Wiki Index\n\n## Reference\n\n* [Zeta Format](/zeta-format.md) - written by a newer binary\n", + ) + .await + .unwrap(); + + let store = MarkdownStore::open(dir.path()).await.unwrap(); + // Precondition: the page really is rejected by this binary. + assert_eq!(store.readiness_report().await.parse_failures, 1); + assert_eq!(store.entry_count().await, 0); + + let mut axum = WikiEntry::new("Axum", "Async web framework."); + axum.entry_type = Some("Reference".into()); + store.upsert(axum).await.unwrap(); + store.regenerate_index().await.unwrap(); + + let index = tokio::fs::read_to_string(wiki.join("index.md")) + .await + .unwrap(); + let carried = "* [Zeta Format](/zeta-format.md) - written by a newer binary"; + let section = index + .find("## Reference") + .unwrap_or_else(|| panic!("Reference section missing: {index}")); + let line = index + .find(carried) + .unwrap_or_else(|| panic!("unparseable page dropped from index: {index}")); + assert!( + line > section, + "carried line not under ## Reference: {index}" + ); + assert!( + index.contains("[Axum](/axum.md)"), + "fresh entry missing: {index}" + ); + assert!( + index.find("[Axum]").unwrap() < line, + "entries not sorted by title: {index}" + ); +} + +#[tokio::test] +async fn two_stale_writers_both_end_up_in_the_index() { + let dir = tempfile::tempdir().expect("tempdir"); + let first = MarkdownStore::open(dir.path()).await.unwrap(); + let second = MarkdownStore::open(dir.path()).await.unwrap(); + + first + .upsert(WikiEntry::new("Axum", "Async web framework.")) + .await + .unwrap(); + first.regenerate_index().await.unwrap(); + second + .upsert(WikiEntry::new("Tower", "Middleware layers.")) + .await + .unwrap(); + second.regenerate_index().await.unwrap(); + + let index = tokio::fs::read_to_string(dir.path().join("wiki").join("index.md")) + .await + .unwrap(); + assert!(index.contains("[Axum](/axum.md)"), "Axum lost: {index}"); + assert!(index.contains("[Tower](/tower.md)"), "Tower lost: {index}"); +} + +#[tokio::test] +async fn rebuilt_index_keeps_okf_version_frontmatter() { + let (store, dir) = temp_store().await; + store + .upsert(WikiEntry::new("Axum", "Async web framework.")) + .await + .unwrap(); + store.regenerate_index().await.unwrap(); + store.regenerate_index().await.unwrap(); + + let index = tokio::fs::read_to_string(dir.path().join("wiki").join("index.md")) + .await + .unwrap(); + assert!( + index.starts_with("---\nokf_version: \"0.2\"\n---"), + "frontmatter missing: {index}" + ); + assert!(index.contains("# Wiki Index"), "title missing: {index}"); +} + +#[tokio::test] +async fn rebuilt_index_drops_lines_for_deleted_pages() { + let dir = tempfile::tempdir().expect("tempdir"); + let wiki = dir.path().join("wiki"); + tokio::fs::create_dir_all(&wiki).await.unwrap(); + tokio::fs::write( + wiki.join("index.md"), + "---\nokf_version: \"0.2\"\n---\n\n# Wiki Index\n\n## Reference\n\n* [Gone](/gone.md) - deleted page\n", + ) + .await + .unwrap(); + + let store = MarkdownStore::open(dir.path()).await.unwrap(); + store + .upsert(WikiEntry::new("Axum", "Async web framework.")) + .await + .unwrap(); + store.regenerate_index().await.unwrap(); + + let index = tokio::fs::read_to_string(wiki.join("index.md")) + .await + .unwrap(); + assert!( + !index.contains("/gone.md"), + "deleted page still listed: {index}" + ); + assert!( + !index.contains("## Reference"), + "empty section kept: {index}" + ); + assert!(index.contains("[Axum](/axum.md)"), "Axum missing: {index}"); +} From 5151070bdcc0c448eab6d5ef175550154c7113c4 Mon Sep 17 00:00:00 2001 From: Travis James Date: Sat, 3 Oct 2026 14:23:56 -0500 Subject: [PATCH 03/10] fix(store): rebuild index.md from disk under a lock and keep pages it cannot parse (GREEN) regenerate_index destroyed index.md entries in three ways: - it rendered from the in-memory snapshot taken at open, so a second writer opened earlier overwrote the first writer's entries; it now renders from a fresh scan of wiki/ while holding an exclusive fs2 lock on wiki/.index.lock (acquired in spawn_blocking); - it dropped pages this binary cannot parse; existing index lines whose page still exists on disk and was not parsed are now carried verbatim under their section (bundle::merge_preserved_index_entries), while lines for deleted pages are dropped; - it wrote index.md (and append_log wrote log.md) in place; both are now replaced atomically via a synced temp file and rename. Closes #15 Co-Authored-By: Claude Opus 5.5 --- pk-store/src/bundle.rs | 206 +++++++++++++++++++++++++++++++++++++---- pk-store/src/store.rs | 121 ++++++++++++++++++++++-- 2 files changed, 299 insertions(+), 28 deletions(-) diff --git a/pk-store/src/bundle.rs b/pk-store/src/bundle.rs index dfa2b5e..b0b92fc 100644 --- a/pk-store/src/bundle.rs +++ b/pk-store/src/bundle.rs @@ -7,7 +7,7 @@ use pk_core::types::{ArticleId, LintReport, LintSeverity, WikiEntry}; use pulldown_cmark::{Event, Options, Parser, Tag}; -use std::collections::HashSet; +use std::collections::{BTreeMap, HashSet}; /// OKF v0.2 §6.1: a bundle-relative link begins with `/` (interpreted from the /// bundle root) and, for a concept link, ends in `.md`. Converts such a link @@ -59,42 +59,49 @@ const LOG_TITLE: &str = "# Update Log"; /// are sorted for deterministic output (stable diffs). Opens with the /// `okf_version` declaration and carries no other frontmatter (§8, §12). pub fn render_index(entries: &[WikiEntry]) -> String { - use std::collections::BTreeMap; - - let mut groups: BTreeMap> = BTreeMap::new(); + let mut groups: BTreeMap> = BTreeMap::new(); for e in entries { let group = e .entry_type .clone() .unwrap_or_else(|| "Uncategorized".to_string()); - groups.entry(group).or_default().push(e); + let mut line = format!("* [{}](/{}.md)", e.title, e.id.as_str()); + if let Some(desc) = e.description.as_deref().filter(|d| !d.trim().is_empty()) { + line.push_str(" - "); + line.push_str(desc.trim()); + } + groups.entry(group).or_default().push(line); } + render_index_groups(groups) +} +/// Shared §8 layout: version block, title, then each `## Section` (BTreeMap +/// order) with its entry lines sorted by title, case-insensitively. Ties fall +/// back to the full line so the output never depends on input order. +fn render_index_groups(groups: BTreeMap>) -> String { let mut out = String::new(); out.push_str(INDEX_VERSION_BLOCK); out.push_str(INDEX_TITLE); out.push_str("\n\n"); - if entries.is_empty() { + if groups.values().all(Vec::is_empty) { out.push_str("_No entries yet._\n"); return out; } - for (group, mut items) in groups { - items.sort_by_key(|item| item.title.to_lowercase()); + for (group, mut lines) in groups { + if lines.is_empty() { + continue; + } + lines.sort_by_cached_key(|line| { + let title = parse_index_entry(line).map_or(String::new(), |(t, _)| t.to_lowercase()); + (title, line.clone()) + }); out.push_str("## "); out.push_str(&group); out.push_str("\n\n"); - for e in items { - out.push_str("* ["); - out.push_str(&e.title); - out.push_str("](/"); - out.push_str(e.id.as_str()); - out.push_str(".md)"); - if let Some(desc) = e.description.as_deref().filter(|d| !d.trim().is_empty()) { - out.push_str(" - "); - out.push_str(desc.trim()); - } + for line in lines { + out.push_str(&line); out.push('\n'); } out.push('\n'); @@ -107,6 +114,81 @@ pub fn render_index(entries: &[WikiEntry]) -> String { out } +/// Parse a §8 entry line `* [Title](/id.md)...` into `(title, id)`. +fn parse_index_entry(line: &str) -> Option<(&str, &str)> { + let rest = line.trim_start().strip_prefix("* [")?; + let (title, after) = rest.split_once("](/")?; + let (id, _) = after.split_once(".md)")?; + if id.is_empty() { + return None; + } + Some((title, id)) +} + +/// Split an `index.md` body into `## Section` → entry lines (CRLF tolerated, +/// lines kept verbatim minus trailing whitespace). Entry lines that precede +/// any section heading land under `Uncategorized`, as `render_index` would +/// file them. Placeholders and other prose are ignored. +fn index_sections(index: &str) -> BTreeMap> { + let mut sections: BTreeMap> = BTreeMap::new(); + let mut current = "Uncategorized".to_string(); + for raw in index.lines() { + let line = raw.trim_end(); + if let Some(heading) = line.strip_prefix("## ") { + current = heading.to_string(); + sections.entry(current.clone()).or_default(); + } else if parse_index_entry(line).is_some() { + sections + .entry(current.clone()) + .or_default() + .push(line.to_string()); + } + } + sections +} + +/// Carry entries of an `existing` index into a freshly `rendered` one (issue +/// #15). An existing entry line is kept verbatim, under its existing section, +/// when its id is a safe, non-reserved concept path, is NOT among +/// `parsed_ids` (those are always rendered fresh), and `exists_on_disk(id)` +/// reports its page file still exists. Every other existing line is dropped — +/// that is how a deleted page leaves the index. Output keeps the +/// `render_index` layout: version block, sorted sections, entries sorted by +/// title, and no `_No entries yet._` placeholder once anything is listed. +pub fn merge_preserved_index_entries( + rendered: &str, + existing: &str, + parsed_ids: &HashSet, + exists_on_disk: impl Fn(&str) -> bool, +) -> String { + let mut carried: Vec<(String, String)> = Vec::new(); + for (section, lines) in index_sections(existing) { + for line in lines { + let Some((_, id)) = parse_index_entry(&line) else { + continue; + }; + let article_id = ArticleId::from(id); + let file_name = id.rsplit('/').next().unwrap_or(id); + let keep = article_id.is_safe_path() + && !crate::markdown::is_reserved_filename(&format!("{file_name}.md")) + && !parsed_ids.contains(&article_id) + && exists_on_disk(id); + if keep && !carried.iter().any(|(_, l)| l == &line) { + carried.push((section.clone(), line)); + } + } + } + if carried.is_empty() { + return rendered.to_string(); + } + + let mut groups = index_sections(rendered); + for (section, line) in carried { + groups.entry(section).or_default().push(line); + } + render_index_groups(groups) +} + /// Insert `line` under the `date` group in an existing OKF v0.2 §9 `log.md` body, /// newest date first. Creates the log title and/or the date group when /// absent; prepends within an existing date group so the most recent entry @@ -362,6 +444,94 @@ mod tests { e } + fn article_ids(list: &[&str]) -> HashSet { + list.iter().map(|id| ArticleId::from(*id)).collect() + } + + const EXISTING: &str = "---\r\nokf_version: \"0.2\"\r\n---\r\n\r\n# Wiki Index\r\n\r\n## Reference\r\n\r\n* [Zeta](/zeta.md) - kept verbatim\r\n* [Gone](/gone.md) - deleted\r\n* [Axum](/axum.md) - stale description\r\n\r\n## Tools\r\n\r\n* [Nested](/a/nested.md)\r\n"; + + #[test] + fn merge_carries_unparsed_existing_lines_under_their_section() { + let rendered = render_index(&[entry("axum", "Axum", Some("Reference"), Some("fresh"))]); + let merged = + merge_preserved_index_entries(&rendered, EXISTING, &article_ids(&["axum"]), |id| { + id != "gone" + }); + assert_eq!( + merged, + "---\nokf_version: \"0.2\"\n---\n\n# Wiki Index\n\n## Reference\n\n\ + * [Axum](/axum.md) - fresh\n* [Zeta](/zeta.md) - kept verbatim\n\n\ + ## Tools\n\n* [Nested](/a/nested.md)\n" + ); + } + + #[test] + fn merge_sorts_carried_lines_among_fresh_ones_by_title() { + let rendered = render_index(&[ + entry("alpha", "alpha", Some("Reference"), None), + entry("zulu", "Zulu", Some("Reference"), None), + ]); + let existing = "## Reference\n* [Mike](/mike.md)\n"; + let merged = merge_preserved_index_entries( + &rendered, + existing, + &article_ids(&["alpha", "zulu"]), + |_| true, + ); + let a = merged.find("[alpha]").unwrap(); + let m = merged.find("[Mike]").unwrap(); + let z = merged.find("[Zulu]").unwrap(); + assert!(a < m && m < z, "unsorted: {merged}"); + } + + #[test] + fn merge_replaces_placeholder_when_only_carried_entries_remain() { + let rendered = render_index(&[]); + assert!(rendered.contains("_No entries yet._")); + let merged = merge_preserved_index_entries(&rendered, EXISTING, &article_ids(&[]), |id| { + id == "zeta" + }); + assert!(!merged.contains("_No entries yet._"), "{merged}"); + assert!(merged.starts_with(INDEX_VERSION_BLOCK)); + assert!(merged.contains("## Reference\n\n* [Zeta](/zeta.md) - kept verbatim\n")); + assert!(!merged.contains("## Tools"), "{merged}"); + } + + #[test] + fn merge_returns_rendered_unchanged_when_nothing_is_carried() { + let rendered = render_index(&[entry("axum", "Axum", None, None)]); + let merged = + merge_preserved_index_entries(&rendered, EXISTING, &article_ids(&["axum"]), |_| false); + assert_eq!(merged, rendered); + assert_eq!( + merge_preserved_index_entries(&rendered, "", &article_ids(&[]), |_| true), + rendered + ); + } + + #[test] + fn merge_drops_unsafe_reserved_and_parsed_targets() { + let existing = "## Reference\n* [Up](/../secret.md)\n* [Index](/index.md)\n* [Log](/sub/log.md)\n* [Axum](/axum.md) - stale\n"; + let rendered = render_index(&[]); + let merged = + merge_preserved_index_entries(&rendered, existing, &article_ids(&["axum"]), |_| true); + assert_eq!(merged, rendered); + } + + #[test] + fn merge_puts_sectionless_lines_under_uncategorized_without_duplicates() { + let existing = "# Wiki Index\n\n* [Old](/old.md)\n* [Old](/old.md)\n"; + let merged = + merge_preserved_index_entries(&render_index(&[]), existing, &article_ids(&[]), |_| { + true + }); + assert!( + merged.ends_with("## Uncategorized\n\n* [Old](/old.md)\n"), + "{merged}" + ); + assert_eq!(merged.matches("[Old]").count(), 1); + } + #[test] fn extracts_bundle_relative_links_only() { let body = "See [Orders](/tables/orders.md) and [Customers](/tables/customers.md). \ diff --git a/pk-store/src/store.rs b/pk-store/src/store.rs index b822b25..849c25c 100644 --- a/pk-store/src/store.rs +++ b/pk-store/src/store.rs @@ -9,9 +9,14 @@ use pk_core::{ use serde::Serialize; use sha2::{Digest, Sha256}; use std::{ - collections::HashMap, + collections::{HashMap, HashSet}, + fs::{File, OpenOptions}, + io::Write, path::{Path, PathBuf}, - sync::Arc, + sync::{ + atomic::{AtomicU64, Ordering}, + Arc, + }, }; use tokio::sync::RwLock; use tracing::{debug, info, warn}; @@ -220,14 +225,49 @@ impl MarkdownStore { Ok(inner.entries.values().cloned().collect()) } - /// Regenerate the wiki-root `index.md` (OKF v0.2 §8) from the current entries. + /// Regenerate the wiki-root `index.md` (OKF v0.2 §8) from the pages on disk. /// Called after every ingest so the catalog stays current. + /// + /// Holds an exclusive lock on `wiki/.index.lock` across scan, merge, render + /// and write, so concurrent rebuilds (other stores, other processes) are + /// serialized. Renders from a fresh disk scan rather than this store's + /// in-memory snapshot, keeps existing index lines for pages this binary + /// cannot parse, and replaces the file atomically (issue #15). pub async fn regenerate_index(&self) -> PkResult<()> { - let entries = self.snapshot().await?; - let content = crate::bundle::render_index(&entries); + let lock_path = self.wiki_dir.join(INDEX_LOCK_FILENAME); + let lock = tokio::task::spawn_blocking(move || acquire_exclusive_lock(&lock_path)) + .await + .map_err(join_error)??; + + let result = self.rebuild_index_locked().await; + // Closing the descriptor releases the lock; unlock explicitly anyway. + let _ = fs2::FileExt::unlock(&lock); + drop(lock); + result + } + + async fn rebuild_index_locked(&self) -> PkResult<()> { + let scan = scan_wiki_tree(&self.wiki_dir).await?; + let entries: Vec = scan.entries.values().cloned().collect(); + let parsed_ids: HashSet = scan.entries.into_keys().collect(); + let rendered = crate::bundle::render_index(&entries); + let path = self.wiki_dir.join("index.md"); - tokio::fs::write(&path, content).await?; - debug!(path = %path.display(), "index.md regenerated"); + let existing = match tokio::fs::read(&path).await { + Ok(bytes) => String::from_utf8_lossy(&bytes).into_owned(), + Err(error) if error.kind() == std::io::ErrorKind::NotFound => String::new(), + Err(error) => return Err(error.into()), + }; + let content = + crate::bundle::merge_preserved_index_entries(&rendered, &existing, &parsed_ids, |id| { + article_path(&self.wiki_dir, &ArticleId::from(id)).is_file() + }); + write_atomic(&path, content).await?; + debug!( + path = %path.display(), + parse_failures = scan.parse_failures, + "index.md regenerated" + ); Ok(()) } @@ -240,7 +280,7 @@ impl MarkdownStore { let date = chrono::Utc::now().format("%Y-%m-%d").to_string(); let line = format!("* **{action}**: [{title}](/{}.md)", id.as_str()); let updated = crate::bundle::append_log_line(&existing, &date, &line); - tokio::fs::write(&path, updated).await?; + write_atomic(&path, updated).await?; debug!(path = %path.display(), action, "log.md appended"); Ok(()) } @@ -255,8 +295,6 @@ impl MarkdownStore { /// reserved `index.md`/`log.md` structure. Orphan detection uses the /// in-memory snapshot. pub async fn okf_conformance_reports(&self) -> PkResult> { - use std::collections::HashSet; - let mut concept_files: Vec<(String, String)> = Vec::new(); let mut index_raw: Option = None; let mut log_raw: Option = None; @@ -388,6 +426,69 @@ impl MarkdownStore { } } +/// Lock file serializing `index.md` rebuilds. No `.md` extension, so the +/// wiki scan never treats it as a concept page. +const INDEX_LOCK_FILENAME: &str = ".index.lock"; + +fn join_error(error: tokio::task::JoinError) -> PkError { + PkError::from(std::io::Error::other(error)) +} + +/// Open (creating if needed) `lock_path` and take a blocking exclusive `fs2` +/// lock on it. Call only from a blocking context. The lock lives as long as +/// the returned `File`. +fn acquire_exclusive_lock(lock_path: &Path) -> std::io::Result { + let lock = OpenOptions::new() + .create(true) + .truncate(false) + .read(true) + .write(true) + .open(lock_path)?; + fs2::FileExt::lock_exclusive(&lock)?; + Ok(lock) +} + +/// Replace `path` atomically: write a uniquely named temp file in the same +/// directory, sync it, then rename it over the target. A crash leaves either +/// the old file or the new one, never a truncated one. +async fn write_atomic(path: &Path, contents: String) -> PkResult<()> { + let path = path.to_path_buf(); + tokio::task::spawn_blocking(move || write_atomic_blocking(&path, contents.as_bytes())) + .await + .map_err(join_error)??; + Ok(()) +} + +fn write_atomic_blocking(path: &Path, contents: &[u8]) -> std::io::Result<()> { + static COUNTER: AtomicU64 = AtomicU64::new(0); + let dir = path + .parent() + .filter(|p| !p.as_os_str().is_empty()) + .unwrap_or_else(|| Path::new(".")); + let name = path + .file_name() + .map(|n| n.to_string_lossy().into_owned()) + .unwrap_or_else(|| "file".to_string()); + let nanos = chrono::Utc::now().timestamp_nanos_opt().unwrap_or_default(); + let tmp = dir.join(format!( + ".{name}.{}.{nanos}.{}.tmp", + std::process::id(), + COUNTER.fetch_add(1, Ordering::Relaxed) + )); + + let written = write_synced(&tmp, contents).and_then(|()| std::fs::rename(&tmp, path)); + if written.is_err() { + let _ = std::fs::remove_file(&tmp); + } + written +} + +fn write_synced(path: &Path, contents: &[u8]) -> std::io::Result<()> { + let mut file = OpenOptions::new().write(true).create_new(true).open(path)?; + file.write_all(contents)?; + file.sync_all() +} + async fn scan_wiki_tree(wiki_dir: &Path) -> PkResult { let mut outcome = ScanOutcome { entries: HashMap::new(), From 9e1e9628484d79cf0d640aa576f11e4e41ae8eaa Mon Sep 17 00:00:00 2001 From: Travis James Date: Sat, 3 Oct 2026 14:24:56 -0500 Subject: [PATCH 04/10] test(store): concurrent append_log calls keep every line (RED) Refs #15 Co-Authored-By: Claude Opus 5.5 --- pk-store/tests/store_tests.rs | 37 +++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/pk-store/tests/store_tests.rs b/pk-store/tests/store_tests.rs index 3ed04c8..9512598 100644 --- a/pk-store/tests/store_tests.rs +++ b/pk-store/tests/store_tests.rs @@ -675,3 +675,40 @@ async fn rebuilt_index_drops_lines_for_deleted_pages() { ); assert!(index.contains("[Axum](/axum.md)"), "Axum missing: {index}"); } + +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn concurrent_append_log_calls_keep_every_line() { + const ROUNDS: usize = 40; + let dir = tempfile::tempdir().expect("tempdir"); + let first = Arc::new(MarkdownStore::open(dir.path()).await.unwrap()); + let second = Arc::new(MarkdownStore::open(dir.path()).await.unwrap()); + + let append_all = |store: Arc, prefix: &'static str| async move { + for i in 0..ROUNDS { + let id = ArticleId::from(format!("{prefix}-{i}")); + store + .append_log("Creation", &format!("{prefix} {i}"), &id) + .await + .unwrap(); + } + }; + let a = tokio::spawn(append_all(first, "alpha")); + let b = tokio::spawn(append_all(second, "beta")); + let (ra, rb) = tokio::join!(a, b); + ra.unwrap(); + rb.unwrap(); + + let log = tokio::fs::read_to_string(dir.path().join("wiki").join("log.md")) + .await + .unwrap(); + let missing: Vec = ["alpha", "beta"] + .iter() + .flat_map(|p| (0..ROUNDS).map(move |i| format!("(/{p}-{i}.md)"))) + .filter(|needle| !log.contains(needle.as_str())) + .collect(); + assert!( + missing.is_empty(), + "log.md lost {} lines: {missing:?}", + missing.len() + ); +} From 40442b65b9fc3602f5d62d795d6f9b3439baf741 Mon Sep 17 00:00:00 2001 From: Travis James Date: Sat, 3 Oct 2026 14:25:23 -0500 Subject: [PATCH 05/10] fix(store): serialize log.md appends under the index lock (GREEN) append_log read, modified and rewrote log.md with no lock, so two appenders could each drop the other's line. It now holds the same exclusive wiki/.index.lock as regenerate_index across the read-modify-write, and no longer treats a read error other than NotFound as an empty log. Refs #15 Co-Authored-By: Claude Opus 5.5 --- pk-store/src/store.rs | 51 +++++++++++++++++++++++++++++++++---------- 1 file changed, 39 insertions(+), 12 deletions(-) diff --git a/pk-store/src/store.rs b/pk-store/src/store.rs index 849c25c..86f40e9 100644 --- a/pk-store/src/store.rs +++ b/pk-store/src/store.rs @@ -234,16 +234,21 @@ impl MarkdownStore { /// in-memory snapshot, keeps existing index lines for pages this binary /// cannot parse, and replaces the file atomically (issue #15). pub async fn regenerate_index(&self) -> PkResult<()> { + let lock = self.lock_index().await?; + let result = self.rebuild_index_locked().await; + release_lock(lock); + result + } + + /// Take the exclusive `wiki/.index.lock` that serializes every rewrite of + /// the reserved `index.md` and `log.md` files, across stores and processes. + /// The blocking wait runs off the async runtime threads. + async fn lock_index(&self) -> PkResult { let lock_path = self.wiki_dir.join(INDEX_LOCK_FILENAME); let lock = tokio::task::spawn_blocking(move || acquire_exclusive_lock(&lock_path)) .await .map_err(join_error)??; - - let result = self.rebuild_index_locked().await; - // Closing the descriptor releases the lock; unlock explicitly anyway. - let _ = fs2::FileExt::unlock(&lock); - drop(lock); - result + Ok(lock) } async fn rebuild_index_locked(&self) -> PkResult<()> { @@ -274,17 +279,32 @@ impl MarkdownStore { /// Append an entry to the wiki-root `log.md` (OKF v0.2 §9) under today's date /// group, newest first. `action` is the leading bold verb (`Creation`, /// `Update`, …). + /// + /// The read-modify-write runs under the same `wiki/.index.lock` as + /// `regenerate_index`, so concurrent appenders never drop each other's + /// lines. pub async fn append_log(&self, action: &str, title: &str, id: &ArticleId) -> PkResult<()> { - let path = self.wiki_dir.join("log.md"); - let existing = tokio::fs::read_to_string(&path).await.unwrap_or_default(); - let date = chrono::Utc::now().format("%Y-%m-%d").to_string(); let line = format!("* **{action}**: [{title}](/{}.md)", id.as_str()); - let updated = crate::bundle::append_log_line(&existing, &date, &line); - write_atomic(&path, updated).await?; - debug!(path = %path.display(), action, "log.md appended"); + let lock = self.lock_index().await?; + let result = self.append_log_locked(&line).await; + release_lock(lock); + result?; + debug!(action, "log.md appended"); Ok(()) } + async fn append_log_locked(&self, line: &str) -> PkResult<()> { + let path = self.wiki_dir.join("log.md"); + let existing = match tokio::fs::read(&path).await { + Ok(bytes) => String::from_utf8_lossy(&bytes).into_owned(), + Err(error) if error.kind() == std::io::ErrorKind::NotFound => String::new(), + Err(error) => return Err(error.into()), + }; + let date = chrono::Utc::now().format("%Y-%m-%d").to_string(); + let updated = crate::bundle::append_log_line(&existing, &date, line); + write_atomic(&path, updated).await + } + pub async fn entry_count(&self) -> usize { self.inner.read().await.entries.len() } @@ -448,6 +468,13 @@ fn acquire_exclusive_lock(lock_path: &Path) -> std::io::Result { Ok(lock) } +/// Release a lock taken by `acquire_exclusive_lock`. Closing the descriptor +/// releases it too; unlocking first makes the hand-off explicit. +fn release_lock(lock: File) { + let _ = fs2::FileExt::unlock(&lock); + drop(lock); +} + /// Replace `path` atomically: write a uniquely named temp file in the same /// directory, sync it, then rename it over the target. A crash leaves either /// the old file or the new one, never a truncated one. From 602c856976a527d36e470e48336eb24aa4f470a6 Mon Sep 17 00:00:00 2001 From: Travis James Date: Sat, 3 Oct 2026 14:27:23 -0500 Subject: [PATCH 06/10] test(worker): a worker run keeps index entries for pages it cannot parse Guards issue #15 at the worker boundary. The test runs the real prometheus-learning-worker binary (run-once) against a project KB whose index.md lists a page this binary cannot parse (a `tags:` mapping), and asserts that after the worker writes its SessionRecord the index still carries the okf_version block, still lists [Legacy page](/legacy-page.md) under ## Reference, and also lists the new session record. This is a regression guard for the store fix in 5151070, not a RED commit. Run against pre-fix pk-store/src (937cf4c) it fails with: Reference section missing: --- okf_version: "0.2" --- # Wiki Index ## SessionRecord * [Karpathy session fedcba987654](/karpathy-session-fedcba9876543210.md) Refs #15 Co-Authored-By: Claude Opus 5.5 --- pk-learning-worker/tests/worker.rs | 81 ++++++++++++++++++++++++++++++ 1 file changed, 81 insertions(+) diff --git a/pk-learning-worker/tests/worker.rs b/pk-learning-worker/tests/worker.rs index fd7cb2b..ab7e917 100644 --- a/pk-learning-worker/tests/worker.rs +++ b/pk-learning-worker/tests/worker.rs @@ -222,6 +222,87 @@ fn processes_a_job_once_and_preserves_ambiguous_memory_delivery() { assert_eq!(learning_log.lines().count(), 1); } +#[test] +fn a_worker_run_keeps_index_entries_for_pages_it_cannot_parse() { + // Issue #15: a worker that rebuilt index.md from only the pages it could + // parse dropped every other entry and the okf_version frontmatter. + let fixture = tempfile::tempdir().unwrap(); + let home = fixture.path().join("home"); + let project = fixture.path().join("project"); + let queue = home.join(".prometheus/learning-queue"); + let wiki = project.join(".prometheus/knowledge/wiki"); + fs::create_dir_all(queue.join("pending")).unwrap(); + fs::create_dir_all(project.join(".git")).unwrap(); + fs::create_dir_all(&wiki).unwrap(); + // A `tags:` mapping is the shape pk-store rejects (see pk-store + // store_tests `rebuilding_index_keeps_pages_this_binary_cannot_parse`). + fs::write( + wiki.join("legacy-page.md"), + "---\nid: legacy-page\ntitle: Legacy page\ntags:\n nested: {a: 1}\n---\n\nBody.\n", + ) + .unwrap(); + fs::write( + wiki.join("index.md"), + "---\nokf_version: \"0.2\"\n---\n\n# Wiki Index\n\n## Reference\n\n* [Legacy page](/legacy-page.md)\n", + ) + .unwrap(); + let transcript = fixture.path().join("transcript.jsonl"); + fs::write( + &transcript, + serde_json::to_string(&json!({ + "type":"assistant", + "message":{"role":"assistant","content":[{"type":"text","text":"Kept the index intact."}]} + })) + .unwrap() + + "\n", + ) + .unwrap(); + let event_id = "fedcba9876543210fedcba9876543210fedcba9876543210fedcba9876543210"; + fs::write( + queue.join("pending").join(format!("{event_id}.json")), + serde_json::to_vec_pretty(&json!({ + "schemaVersion":2, + "eventId":event_id, + "eventType":"stop", + "harness":"fixture", + "sessionId":"fixture-session", + "projectRoot":project, + "transcriptPath":transcript, + "capturedAt":"2026-08-03T00:00:00Z", + "payloadDigest":"fixture", + "attempt":0 + })) + .unwrap(), + ) + .unwrap(); + + succeed(&home, &["--memory-url", "http://127.0.0.1:1", "run-once"]); + + assert!( + wiki.join("karpathy-session-fedcba9876543210.md").exists(), + "worker did not write the session record" + ); + let index = fs::read_to_string(wiki.join("index.md")).unwrap(); + assert!( + index.starts_with("---\nokf_version: \"0.2\"\n---\n"), + "okf_version frontmatter lost: {index}" + ); + let section = index + .find("## Reference") + .unwrap_or_else(|| panic!("Reference section missing: {index}")); + let legacy = index + .find("[Legacy page](/legacy-page.md)") + .unwrap_or_else(|| panic!("unparseable page dropped from index: {index}")); + assert!( + legacy > section, + "legacy line not under ## Reference: {index}" + ); + assert!( + index.contains("[Karpathy session fedcba987654](/karpathy-session-fedcba9876543210.md)"), + "new SessionRecord entry missing: {index}" + ); +} + fn worker(home: &Path, args: &[&str]) -> Output { Command::new(env!("CARGO_BIN_EXE_prometheus-learning-worker")) .env("HOME", home) From f57db3f16e2d23e20aa831bcd2be89a33ff6dd12 Mon Sep 17 00:00:00 2001 From: Travis James Date: Sat, 3 Oct 2026 14:32:34 -0500 Subject: [PATCH 07/10] test(store): carried index lines never duplicate parsed pages or drop multi-line entries (RED) Refs #15 Co-Authored-By: Claude Opus 5.5 --- pk-store/tests/store_tests.rs | 66 +++++++++++++++++++++++++++++++++++ 1 file changed, 66 insertions(+) diff --git a/pk-store/tests/store_tests.rs b/pk-store/tests/store_tests.rs index 9512598..611b4db 100644 --- a/pk-store/tests/store_tests.rs +++ b/pk-store/tests/store_tests.rs @@ -712,3 +712,69 @@ async fn concurrent_append_log_calls_keep_every_line() { missing.len() ); } + +#[tokio::test] +async fn page_whose_filename_differs_from_its_id_is_listed_once() { + let dir = tempfile::tempdir().expect("tempdir"); + let wiki = dir.path().join("wiki"); + tokio::fs::create_dir_all(&wiki).await.unwrap(); + tokio::fs::write( + wiki.join("foo.md"), + "---\nid: bar\ntitle: Bar\n---\n\nBody.\n", + ) + .await + .unwrap(); + tokio::fs::write( + wiki.join("index.md"), + "---\nokf_version: \"0.2\"\n---\n\n# Wiki Index\n\n## Uncategorized\n\n* [Foo](/foo.md) - stale line for the file name\n", + ) + .await + .unwrap(); + + let store = MarkdownStore::open(dir.path()).await.unwrap(); + assert_eq!(store.readiness_report().await.parse_failures, 0); + store.regenerate_index().await.unwrap(); + + let index = tokio::fs::read_to_string(wiki.join("index.md")) + .await + .unwrap(); + assert_eq!(index.matches("(/bar.md)").count(), 1, "{index}"); + assert!( + !index.contains("(/foo.md)"), + "stale line carried for a page that parses: {index}" + ); +} + +#[tokio::test] +async fn multi_line_description_survives_when_a_line_is_carried() { + let dir = tempfile::tempdir().expect("tempdir"); + let wiki = dir.path().join("wiki"); + tokio::fs::create_dir_all(&wiki).await.unwrap(); + tokio::fs::write(wiki.join("zeta-format.md"), UNPARSEABLE_PAGE) + .await + .unwrap(); + tokio::fs::write( + wiki.join("index.md"), + "---\nokf_version: \"0.2\"\n---\n\n# Wiki Index\n\n## Reference\n\n* [Zeta Format](/zeta-format.md) - written by a newer binary\n", + ) + .await + .unwrap(); + + let store = MarkdownStore::open(dir.path()).await.unwrap(); + let mut axum = WikiEntry::new("Axum", "Async web framework."); + axum.description = Some("line one\nline two".into()); + store.upsert(axum).await.unwrap(); + store.regenerate_index().await.unwrap(); + + let index = tokio::fs::read_to_string(wiki.join("index.md")) + .await + .unwrap(); + assert!( + index.contains("(/zeta-format.md)"), + "carried line lost: {index}" + ); + assert!( + index.contains("[Axum](/axum.md) - line one\nline two"), + "multi-line description cut: {index}" + ); +} From 0e2791362214d166c96c3ccf590c880a6b5cd36e Mon Sep 17 00:00:00 2001 From: Travis James Date: Sat, 3 Oct 2026 14:33:49 -0500 Subject: [PATCH 08/10] fix(store): carry only lines for pages that failed to parse, and merge structurally (GREEN) Carry-forward compared a frontmatter id with a file path, so a page whose filename differs from its id (or differs in case on a case-insensitive filesystem) kept its stale line forever and was listed twice. The wiki scan now records the bundle-relative paths of pages it could not read or parse, and an existing line is carried only when its link target is exactly one of them. The merge also re-parsed the rendered index, dropping the second line of a multi-line description and the whole entry for a multi-line title. bundle::render_index_preserving (replacing the unreleased merge_preserved_index_entries) now builds one structured group map from the parsed entries plus the carried lines and renders once; parsed entries render byte-identically to render_index. Carried lines are deduplicated per target. Documents that regenerate_index and append_log take wiki/.index.lock and must not be nested. Refs #15 Co-Authored-By: Claude Opus 5.5 --- pk-store/src/bundle.rs | 222 +++++++++++++++++++---------------------- pk-store/src/store.rs | 39 +++++--- 2 files changed, 125 insertions(+), 136 deletions(-) diff --git a/pk-store/src/bundle.rs b/pk-store/src/bundle.rs index b0b92fc..fc72d91 100644 --- a/pk-store/src/bundle.rs +++ b/pk-store/src/bundle.rs @@ -59,7 +59,48 @@ const LOG_TITLE: &str = "# Update Log"; /// are sorted for deterministic output (stable diffs). Opens with the /// `okf_version` declaration and carries no other frontmatter (§8, §12). pub fn render_index(entries: &[WikiEntry]) -> String { - let mut groups: BTreeMap> = BTreeMap::new(); + render_index_groups(entry_groups(entries)) +} + +/// Render `index.md` from `entries` like [`render_index`], and carry over +/// every entry line of the `existing` index whose link target is exactly one +/// of `failed_paths` — the bundle-relative paths (minus `.md`) of pages on disk +/// that this binary could not read or parse (issue #15). A carried line is +/// kept verbatim under its existing `## Section`; only the first line per +/// target is kept. Lines for any other target are dropped: parsed pages are +/// always rendered fresh (whatever id they declare), and deleted pages leave +/// the index. Parsed entries render byte-identically to [`render_index`]. +pub fn render_index_preserving( + entries: &[WikiEntry], + existing: &str, + failed_paths: &HashSet, +) -> String { + let mut groups = entry_groups(entries); + let mut carried_targets: HashSet<&str> = HashSet::new(); + let mut current = "Uncategorized"; + for raw in existing.lines() { + let line = raw.trim_end(); + if let Some(heading) = line.strip_prefix("## ") { + current = heading; + continue; + } + let Some((title, target)) = parse_index_entry(line) else { + continue; + }; + if failed_paths.contains(target) && carried_targets.insert(target) { + groups + .entry(current.to_string()) + .or_default() + .push((title.to_lowercase(), line.to_string())); + } + } + render_index_groups(groups) +} + +/// Section → `(sort key, entry line)` for parsed entries, grouped by concept +/// `type`. The sort key is the lowercased title, as `render_index` always used. +fn entry_groups(entries: &[WikiEntry]) -> BTreeMap> { + let mut groups: BTreeMap> = BTreeMap::new(); for e in entries { let group = e .entry_type @@ -70,15 +111,18 @@ pub fn render_index(entries: &[WikiEntry]) -> String { line.push_str(" - "); line.push_str(desc.trim()); } - groups.entry(group).or_default().push(line); + groups + .entry(group) + .or_default() + .push((e.title.to_lowercase(), line)); } - render_index_groups(groups) + groups } /// Shared §8 layout: version block, title, then each `## Section` (BTreeMap -/// order) with its entry lines sorted by title, case-insensitively. Ties fall +/// order) with its entry lines sorted by key (lowercased title). Ties fall /// back to the full line so the output never depends on input order. -fn render_index_groups(groups: BTreeMap>) -> String { +fn render_index_groups(groups: BTreeMap>) -> String { let mut out = String::new(); out.push_str(INDEX_VERSION_BLOCK); out.push_str(INDEX_TITLE); @@ -93,14 +137,11 @@ fn render_index_groups(groups: BTreeMap>) -> String { if lines.is_empty() { continue; } - lines.sort_by_cached_key(|line| { - let title = parse_index_entry(line).map_or(String::new(), |(t, _)| t.to_lowercase()); - (title, line.clone()) - }); + lines.sort(); out.push_str("## "); out.push_str(&group); out.push_str("\n\n"); - for line in lines { + for (_, line) in lines { out.push_str(&line); out.push('\n'); } @@ -114,79 +155,17 @@ fn render_index_groups(groups: BTreeMap>) -> String { out } -/// Parse a §8 entry line `* [Title](/id.md)...` into `(title, id)`. +/// Parse a §8 entry line `* [Title](/target.md)...` into `(title, target)`, +/// where `target` is the bundle-relative path without the leading `/` and +/// the `.md` suffix. fn parse_index_entry(line: &str) -> Option<(&str, &str)> { let rest = line.trim_start().strip_prefix("* [")?; let (title, after) = rest.split_once("](/")?; - let (id, _) = after.split_once(".md)")?; - if id.is_empty() { + let (target, _) = after.split_once(".md)")?; + if target.is_empty() { return None; } - Some((title, id)) -} - -/// Split an `index.md` body into `## Section` → entry lines (CRLF tolerated, -/// lines kept verbatim minus trailing whitespace). Entry lines that precede -/// any section heading land under `Uncategorized`, as `render_index` would -/// file them. Placeholders and other prose are ignored. -fn index_sections(index: &str) -> BTreeMap> { - let mut sections: BTreeMap> = BTreeMap::new(); - let mut current = "Uncategorized".to_string(); - for raw in index.lines() { - let line = raw.trim_end(); - if let Some(heading) = line.strip_prefix("## ") { - current = heading.to_string(); - sections.entry(current.clone()).or_default(); - } else if parse_index_entry(line).is_some() { - sections - .entry(current.clone()) - .or_default() - .push(line.to_string()); - } - } - sections -} - -/// Carry entries of an `existing` index into a freshly `rendered` one (issue -/// #15). An existing entry line is kept verbatim, under its existing section, -/// when its id is a safe, non-reserved concept path, is NOT among -/// `parsed_ids` (those are always rendered fresh), and `exists_on_disk(id)` -/// reports its page file still exists. Every other existing line is dropped — -/// that is how a deleted page leaves the index. Output keeps the -/// `render_index` layout: version block, sorted sections, entries sorted by -/// title, and no `_No entries yet._` placeholder once anything is listed. -pub fn merge_preserved_index_entries( - rendered: &str, - existing: &str, - parsed_ids: &HashSet, - exists_on_disk: impl Fn(&str) -> bool, -) -> String { - let mut carried: Vec<(String, String)> = Vec::new(); - for (section, lines) in index_sections(existing) { - for line in lines { - let Some((_, id)) = parse_index_entry(&line) else { - continue; - }; - let article_id = ArticleId::from(id); - let file_name = id.rsplit('/').next().unwrap_or(id); - let keep = article_id.is_safe_path() - && !crate::markdown::is_reserved_filename(&format!("{file_name}.md")) - && !parsed_ids.contains(&article_id) - && exists_on_disk(id); - if keep && !carried.iter().any(|(_, l)| l == &line) { - carried.push((section.clone(), line)); - } - } - } - if carried.is_empty() { - return rendered.to_string(); - } - - let mut groups = index_sections(rendered); - for (section, line) in carried { - groups.entry(section).or_default().push(line); - } - render_index_groups(groups) + Some((title, target)) } /// Insert `line` under the `date` group in an existing OKF v0.2 §9 `log.md` body, @@ -444,19 +423,16 @@ mod tests { e } - fn article_ids(list: &[&str]) -> HashSet { - list.iter().map(|id| ArticleId::from(*id)).collect() + fn paths(list: &[&str]) -> HashSet { + list.iter().map(|p| p.to_string()).collect() } const EXISTING: &str = "---\r\nokf_version: \"0.2\"\r\n---\r\n\r\n# Wiki Index\r\n\r\n## Reference\r\n\r\n* [Zeta](/zeta.md) - kept verbatim\r\n* [Gone](/gone.md) - deleted\r\n* [Axum](/axum.md) - stale description\r\n\r\n## Tools\r\n\r\n* [Nested](/a/nested.md)\r\n"; #[test] - fn merge_carries_unparsed_existing_lines_under_their_section() { - let rendered = render_index(&[entry("axum", "Axum", Some("Reference"), Some("fresh"))]); - let merged = - merge_preserved_index_entries(&rendered, EXISTING, &article_ids(&["axum"]), |id| { - id != "gone" - }); + fn preserving_carries_only_failed_paths_under_their_section() { + let entries = [entry("axum", "Axum", Some("Reference"), Some("fresh"))]; + let merged = render_index_preserving(&entries, EXISTING, &paths(&["zeta", "a/nested"])); assert_eq!( merged, "---\nokf_version: \"0.2\"\n---\n\n# Wiki Index\n\n## Reference\n\n\ @@ -466,18 +442,22 @@ mod tests { } #[test] - fn merge_sorts_carried_lines_among_fresh_ones_by_title() { - let rendered = render_index(&[ + fn preserving_never_carries_a_parsed_page_listed_under_another_id() { + // foo.md parsed under id `bar`: its path is not a failed path. + let entries = [entry("bar", "Bar", None, None)]; + let existing = "## Uncategorized\n* [Foo](/foo.md)\n* [Foo](/Foo.md)\n"; + let merged = render_index_preserving(&entries, existing, &paths(&[])); + assert_eq!(merged, render_index(&entries)); + } + + #[test] + fn preserving_sorts_carried_lines_among_fresh_ones_by_title() { + let entries = [ entry("alpha", "alpha", Some("Reference"), None), entry("zulu", "Zulu", Some("Reference"), None), - ]); + ]; let existing = "## Reference\n* [Mike](/mike.md)\n"; - let merged = merge_preserved_index_entries( - &rendered, - existing, - &article_ids(&["alpha", "zulu"]), - |_| true, - ); + let merged = render_index_preserving(&entries, existing, &paths(&["mike"])); let a = merged.find("[alpha]").unwrap(); let m = merged.find("[Mike]").unwrap(); let z = merged.find("[Zulu]").unwrap(); @@ -485,12 +465,9 @@ mod tests { } #[test] - fn merge_replaces_placeholder_when_only_carried_entries_remain() { - let rendered = render_index(&[]); - assert!(rendered.contains("_No entries yet._")); - let merged = merge_preserved_index_entries(&rendered, EXISTING, &article_ids(&[]), |id| { - id == "zeta" - }); + fn preserving_replaces_placeholder_when_only_carried_entries_remain() { + assert!(render_index(&[]).contains("_No entries yet._")); + let merged = render_index_preserving(&[], EXISTING, &paths(&["zeta"])); assert!(!merged.contains("_No entries yet._"), "{merged}"); assert!(merged.starts_with(INDEX_VERSION_BLOCK)); assert!(merged.contains("## Reference\n\n* [Zeta](/zeta.md) - kept verbatim\n")); @@ -498,38 +475,41 @@ mod tests { } #[test] - fn merge_returns_rendered_unchanged_when_nothing_is_carried() { - let rendered = render_index(&[entry("axum", "Axum", None, None)]); - let merged = - merge_preserved_index_entries(&rendered, EXISTING, &article_ids(&["axum"]), |_| false); - assert_eq!(merged, rendered); + fn preserving_matches_render_index_when_nothing_is_carried() { + let entries = [entry("axum", "Axum", None, Some("one\ntwo"))]; + assert_eq!( + render_index_preserving(&entries, EXISTING, &paths(&[])), + render_index(&entries) + ); assert_eq!( - merge_preserved_index_entries(&rendered, "", &article_ids(&[]), |_| true), - rendered + render_index_preserving(&entries, "", &paths(&["zeta"])), + render_index(&entries) ); } #[test] - fn merge_drops_unsafe_reserved_and_parsed_targets() { - let existing = "## Reference\n* [Up](/../secret.md)\n* [Index](/index.md)\n* [Log](/sub/log.md)\n* [Axum](/axum.md) - stale\n"; - let rendered = render_index(&[]); - let merged = - merge_preserved_index_entries(&rendered, existing, &article_ids(&["axum"]), |_| true); - assert_eq!(merged, rendered); + fn preserving_keeps_multi_line_fresh_entries_when_carrying() { + let entries = [entry("axum", "Ax\num", None, Some("one\ntwo"))]; + let merged = render_index_preserving(&entries, EXISTING, &paths(&["zeta"])); + assert!( + merged.contains("* [Ax\num](/axum.md) - one\ntwo\n"), + "{merged}" + ); + assert!( + merged.contains("* [Zeta](/zeta.md) - kept verbatim\n"), + "{merged}" + ); } #[test] - fn merge_puts_sectionless_lines_under_uncategorized_without_duplicates() { - let existing = "# Wiki Index\n\n* [Old](/old.md)\n* [Old](/old.md)\n"; - let merged = - merge_preserved_index_entries(&render_index(&[]), existing, &article_ids(&[]), |_| { - true - }); + fn preserving_files_sectionless_lines_under_uncategorized_once_per_target() { + let existing = "# Wiki Index\n\n* [Old](/old.md)\n* [Old again](/old.md)\n"; + let merged = render_index_preserving(&[], existing, &paths(&["old"])); assert!( merged.ends_with("## Uncategorized\n\n* [Old](/old.md)\n"), "{merged}" ); - assert_eq!(merged.matches("[Old]").count(), 1); + assert_eq!(merged.matches("(/old.md)").count(), 1); } #[test] diff --git a/pk-store/src/store.rs b/pk-store/src/store.rs index 86f40e9..ef2e2de 100644 --- a/pk-store/src/store.rs +++ b/pk-store/src/store.rs @@ -51,6 +51,9 @@ struct ScanOutcome { content_hash_index: HashMap, on_disk_count: usize, parse_failures: usize, + /// Bundle-relative paths (minus `.md`, `/`-separated — the scan's + /// fallback ids) of pages that could not be read or parsed. + failed_paths: HashSet, } /// Read the ingest content-hash stamped on an entry's `extra` frontmatter @@ -233,6 +236,10 @@ impl MarkdownStore { /// serialized. Renders from a fresh disk scan rather than this store's /// in-memory snapshot, keeps existing index lines for pages this binary /// cannot parse, and replaces the file atomically (issue #15). + /// + /// Not reentrant: must not be called while the caller already holds + /// `wiki/.index.lock` (for example from inside `append_log`), or the + /// blocking lock on a second descriptor deadlocks. pub async fn regenerate_index(&self) -> PkResult<()> { let lock = self.lock_index().await?; let result = self.rebuild_index_locked().await; @@ -242,7 +249,9 @@ impl MarkdownStore { /// Take the exclusive `wiki/.index.lock` that serializes every rewrite of /// the reserved `index.md` and `log.md` files, across stores and processes. - /// The blocking wait runs off the async runtime threads. + /// The blocking wait runs off the async runtime threads. There is no + /// reentrancy guard: taking it again while held (on another descriptor) + /// blocks forever, so never nest `regenerate_index` / `append_log`. async fn lock_index(&self) -> PkResult { let lock_path = self.wiki_dir.join(INDEX_LOCK_FILENAME); let lock = tokio::task::spawn_blocking(move || acquire_exclusive_lock(&lock_path)) @@ -253,9 +262,7 @@ impl MarkdownStore { async fn rebuild_index_locked(&self) -> PkResult<()> { let scan = scan_wiki_tree(&self.wiki_dir).await?; - let entries: Vec = scan.entries.values().cloned().collect(); - let parsed_ids: HashSet = scan.entries.into_keys().collect(); - let rendered = crate::bundle::render_index(&entries); + let entries: Vec = scan.entries.into_values().collect(); let path = self.wiki_dir.join("index.md"); let existing = match tokio::fs::read(&path).await { @@ -264,13 +271,11 @@ impl MarkdownStore { Err(error) => return Err(error.into()), }; let content = - crate::bundle::merge_preserved_index_entries(&rendered, &existing, &parsed_ids, |id| { - article_path(&self.wiki_dir, &ArticleId::from(id)).is_file() - }); + crate::bundle::render_index_preserving(&entries, &existing, &scan.failed_paths); write_atomic(&path, content).await?; debug!( path = %path.display(), - parse_failures = scan.parse_failures, + parse_failures = scan.failed_paths.len(), "index.md regenerated" ); Ok(()) @@ -282,7 +287,8 @@ impl MarkdownStore { /// /// The read-modify-write runs under the same `wiki/.index.lock` as /// `regenerate_index`, so concurrent appenders never drop each other's - /// lines. + /// lines. Not reentrant: must not be called while the caller already holds + /// `wiki/.index.lock` (for example from inside `regenerate_index`). pub async fn append_log(&self, action: &str, title: &str, id: &ArticleId) -> PkResult<()> { let line = format!("* **{action}**: [{title}](/{}.md)", id.as_str()); let lock = self.lock_index().await?; @@ -524,6 +530,7 @@ async fn scan_wiki_tree(wiki_dir: &Path) -> PkResult { content_hash_index: HashMap::new(), on_disk_count: 0, parse_failures: 0, + failed_paths: HashSet::new(), }; // Canonicalizing every article schedules one blocking filesystem task per // file. Large project stores made the learning worker spend minutes in @@ -557,10 +564,17 @@ async fn scan_wiki_tree(wiki_dir: &Path) -> PkResult { outcome.on_disk_count += 1; let relative = path.strip_prefix(wiki_dir).unwrap_or(&path); let canonical_path = canonical_wiki_dir.join(relative); + let fallback_id: String = relative + .components() + .map(|component| component.as_os_str().to_string_lossy()) + .collect::>() + .join("/"); + let fallback_id = fallback_id.trim_end_matches(".md"); let content = match tokio::fs::read_to_string(&path).await { Ok(content) => content, Err(error) => { outcome.parse_failures += 1; + outcome.failed_paths.insert(fallback_id.to_owned()); warn!(path = %path.display(), err = %error, "failed to read entry"); continue; } @@ -570,12 +584,6 @@ async fn scan_wiki_tree(wiki_dir: &Path) -> PkResult { format!("{:x}", Sha256::digest(content.as_bytes())), ); - let fallback_id: String = relative - .components() - .map(|component| component.as_os_str().to_string_lossy()) - .collect::>() - .join("/"); - let fallback_id = fallback_id.trim_end_matches(".md"); match markdown_to_entry(&content, Some(fallback_id)) { Ok(wiki_entry) => { debug!(id = %wiki_entry.id, "loaded entry"); @@ -589,6 +597,7 @@ async fn scan_wiki_tree(wiki_dir: &Path) -> PkResult { } Err(error) => { outcome.parse_failures += 1; + outcome.failed_paths.insert(fallback_id.to_owned()); warn!(path = %path.display(), err = %error, "skipping malformed entry"); } } From 36911303a86d4fa17de8f396e7252934e84fe2af Mon Sep 17 00:00:00 2001 From: Travis James Date: Sat, 3 Oct 2026 14:34:41 -0500 Subject: [PATCH 09/10] docs: record the preserve-wiki-index design corrections and the skill-system handoff - design.md: carried index lines are matched on failed-to-parse paths, and rendering is structural (verifier findings F1, F2); append_log shares the lock; consumers should ignore the lock and temp files. - tasks.md: builder and lead tasks ticked; verification remains open. - README: how index and log writes behave, and why the three binaries must share a version. - .gitignore: this repo tracks its wiki, so ignore wiki/.index.lock and wiki/.*.tmp. - Handoff note for prometheus-skill-system: bump the 1.8.0 pin, install all three binaries from one commit, add a version-skew doctor check. Refs #15 Co-Authored-By: Claude Opus 5.5 --- .../handoffs/issue-15-skill-system.md | 28 +++++++++++++++++++ .gitignore | 4 +++ README.md | 2 ++ .../changes/preserve-wiki-index/design.md | 16 ++++++++--- openspec/changes/preserve-wiki-index/tasks.md | 13 ++++++--- 5 files changed, 55 insertions(+), 8 deletions(-) create mode 100644 .agent-team/prometheus-knowledge-maintainers/handoffs/issue-15-skill-system.md diff --git a/.agent-team/prometheus-knowledge-maintainers/handoffs/issue-15-skill-system.md b/.agent-team/prometheus-knowledge-maintainers/handoffs/issue-15-skill-system.md new file mode 100644 index 0000000..7ea0db6 --- /dev/null +++ b/.agent-team/prometheus-knowledge-maintainers/handoffs/issue-15-skill-system.md @@ -0,0 +1,28 @@ +# Handoff: issue #15 to prometheus-skill-system + +**From:** prometheus-knowledge-maintainers / knowledge-lead +**To:** prometheus-skill-system maintainers (Prometheus-AGS/prometheus-skill-system) +**Change:** OpenSpec `preserve-wiki-index`, closing prometheus-knowledge-rs#15 +**Date:** 2026-10-03 + +## Why you need to act +Issue #15 (wiki `index.md` rewritten destructively) is triggered by **mixed binary versions on one knowledge base**. On affected machines `pk` is 1.9.0, but `prometheus-learning-worker` is 1.8.0. Both are installed by `scripts/install-binaries.sh` from the `tools/prometheus-knowledge` submodule, which is pinned at 1.8.0 (`01a1dbe`). + +A 1.8.0 binary cannot parse pages written in OKF v0.2 mapping-form `sources`. When it rebuilds the index, it writes only the pages it can read, with no `okf_version` frontmatter. Every other entry disappears from `index.md`, although the page files remain. + +prometheus-knowledge now rebuilds the index from disk under a lock and keeps entries for pages it cannot parse. That protects a KB only once **every** writer runs the fixed version. + +## Requested changes (in your repository) +1. **Bump the submodule** `tools/prometheus-knowledge` to the merge commit of the preserve-wiki-index PR (or later). +2. **Install from one commit:** build `pk`, `pk-cherry` and `prometheus-learning-worker` together from that one submodule commit in `install-binaries.sh`, and refuse a partial install. Mixed versions corrupt shared KBs. +3. **Doctor check:** add one to `prometheus doctor` that fails when the three installed binaries report different versions (` --version`). +4. **Release matrix:** update the `config/release-version-matrix.json` exemption that still records prometheus-knowledge at 1.8.0. +5. **Tools reference:** in `docs/guide/13-tools-reference.md`, note that `index.md` and `log.md` are now written atomically under `wiki/.index.lock`. +6. **Ignore rules:** projects that commit their wiki should ignore `.prometheus/knowledge/wiki/.index.lock` and the temporary files left by an interrupted write (`wiki/.*.tmp`). If your project scaffolding writes a `.gitignore`, add those two patterns. + +## Not requested +- No change to the MCP tools, port 8942, `pk context` / `pk ingest`, the queue layout or `status.json`. +- No change to forge-rs or prometheus-cli code. Note that their pk-core, pk-librarian and pk-store git dependencies are still locked at the initial release. + +## Recovering damaged indexes +Once the fixed binaries are installed, the next ingest or worker run rebuilds `index.md` from the pages on disk, restoring dropped entries and the `okf_version` block. No manual merge is needed. The reporter's snapshot-and-merge workaround can then be removed. diff --git a/.gitignore b/.gitignore index 1f97308..43379fd 100644 --- a/.gitignore +++ b/.gitignore @@ -45,3 +45,7 @@ tarpaulin-report.html # agent-team-creator: local coordination state and install backups .agent-team/*/state.json .agent-team/recovery/ + +# pk wiki write lock and interrupted-write temp files +.prometheus/knowledge/wiki/.index.lock +.prometheus/knowledge/wiki/.*.tmp diff --git a/README.md b/README.md index 9bbc791..ba40fc9 100644 --- a/README.md +++ b/README.md @@ -10,6 +10,8 @@ Rust knowledge and learning runtime for human-readable Markdown records, immutab - **`prometheus-learning-worker`** owns extraction, queue transitions, Memory operation submission, receipt reconciliation, and snapshot publication. - **`pk doctor --json`** diagnoses the active plugin generation, stable dispatchers, snapshots, queue state, hook log permissions, and project scope without creating or changing state. +**Wiki index and log writes** rebuild `index.md` from the pages on disk under an exclusive `wiki/.index.lock`, keep entries for pages the running binary cannot parse, and replace `index.md` and `log.md` atomically. Projects that commit their wiki should ignore `wiki/.index.lock` and `wiki/.*.tmp`. Keep `pk`, `pk-cherry` and `prometheus-learning-worker` at the same version: an older writer still rebuilds the index from only the pages it can read. + Queue states are explicit. Learning jobs use `pending → processing → completed | rejected`. Memory delivery uses `pending → submitting → accepted → completed | rejected`. Legacy retry/dead-letter directories are migration evidence and must be reconciled rather than treated as success. ### Stalled memory operations diff --git a/openspec/changes/preserve-wiki-index/design.md b/openspec/changes/preserve-wiki-index/design.md index e0db50e..c4aa084 100644 --- a/openspec/changes/preserve-wiki-index/design.md +++ b/openspec/changes/preserve-wiki-index/design.md @@ -8,16 +8,24 @@ Rendering calls `scan_wiki_tree(&self.wiki_dir)` inside the lock. The in-memory ## Preserving unparseable pages -The existing `index.md` is read inside the lock. An entry line `* [Title](/id.md)…` is carried into the new index, verbatim, under its existing `## Section` when: -- its target file `wiki/.md` exists on disk, and -- `` is not among the entries this binary parsed. +The existing `index.md` is read inside the lock. The scan reports the bundle-relative paths of the pages it could not read or parse. An entry line `* [Title](/.md)…` is carried into the new index, verbatim, under its existing `## Section`, only when `` is exactly one of those failed paths. -Parsed entries are always rendered fresh from their frontmatter, so they never duplicate a carried line. Entries whose target file no longer exists are dropped, which is how deletion works. The merge is a pure function in `bundle.rs`, testable without a store. +Matching on the failed path, not on "not among the parsed ids", matters. A page whose filename differs from its frontmatter id (`foo.md` declaring `id: bar`), or whose link differs only in case on a case-insensitive filesystem, parses and is rendered fresh. Its old line must not be carried as well, or the page would be listed twice forever (verifier finding F1). + +Lines for pages that parse, or whose file is gone, are dropped. At most one line is carried per target. Rendering happens once, from the parsed entries plus the carried lines grouped by section (`bundle::render_index_preserving`). It never re-parses rendered text, which would lose entries whose title or description spans lines (finding F2). Parsed entries render byte-identically to `render_index`. ## Atomic writes Write to a uniquely named temporary file in `wiki/`, sync it, then rename over the target. A killed process leaves either the old file or the new one, never a truncated one. `log.md` gets the same treatment because `append_log` has the same exposure. +## Lock scope + +`append_log` holds the same lock across its read-modify-write, so concurrent appends cannot lose a line. Neither method may be called while the caller already holds the lock. + +## Consumer housekeeping + +Projects that commit their wiki should ignore `wiki/.index.lock` and the temporary files an interrupted write leaves (`wiki/.*.tmp`). + ## What this cannot fix A binary older than this change (such as the deployed 1.8.0 worker) still rewrites the index destructively. The remedy for that is operational: same-version installs, tracked in the skill-system handoff note. diff --git a/openspec/changes/preserve-wiki-index/tasks.md b/openspec/changes/preserve-wiki-index/tasks.md index 8042e0d..347029b 100644 --- a/openspec/changes/preserve-wiki-index/tasks.md +++ b/openspec/changes/preserve-wiki-index/tasks.md @@ -1,11 +1,16 @@ ## 1. Store (core-engineer) -- [ ] 1.1 RED: store tests for a preserved unparseable entry, two stale writers, frontmatter retention, and a dropped deleted page -- [ ] 1.2 GREEN: lock, fresh scan, merge helper and atomic writes in `pk-store` +- [x] 1.1 RED: store tests for a preserved unparseable entry, two stale writers, frontmatter retention, and a dropped deleted page +- [x] 1.2 GREEN: lock, fresh scan, merge helper and atomic writes in `pk-store` ## 2. Worker boundary (learning-worker-engineer) -- [ ] 2.1 Regression test: a worker `run-once` keeps unparseable entries and the `okf_version` block +- [x] 2.1 Regression test: a worker `run-once` keeps unparseable entries and the `okf_version` block + +## 1a. Verifier corrections (core-engineer) + +- [x] 1.3 `append_log` takes the same lock (RED 9e1e962, GREEN 40442b6) +- [x] 1.4 Carry only lines for pages that failed to parse, and render structurally (findings F1, F2; RED f57db3f, GREEN 0e27913) ## 3. Verification (verifier) @@ -13,4 +18,4 @@ ## 4. Downstream (knowledge-lead) -- [ ] 4.1 Skill-system handoff note: pin bump and same-version installs +- [x] 4.1 Skill-system handoff note: pin bump and same-version installs From ff600ee682a641e079963eb9f5e61f727d4e1404 Mon Sep 17 00:00:00 2001 From: Travis James Date: Sat, 3 Oct 2026 14:37:18 -0500 Subject: [PATCH 10/10] docs: record the verifier's findings and close task 3.1 Separate-context review (same model family): PASS after one correction cycle; workspace gate 171 passed, 0 failed. Human PR review still required. Refs #15 Co-Authored-By: Claude Opus 5.5 --- .../findings/issue-15-preserve-wiki-index.md | 86 +++++++++++++++++++ openspec/changes/preserve-wiki-index/tasks.md | 2 +- 2 files changed, 87 insertions(+), 1 deletion(-) create mode 100644 .agent-team/prometheus-knowledge-maintainers/findings/issue-15-preserve-wiki-index.md diff --git a/.agent-team/prometheus-knowledge-maintainers/findings/issue-15-preserve-wiki-index.md b/.agent-team/prometheus-knowledge-maintainers/findings/issue-15-preserve-wiki-index.md new file mode 100644 index 0000000..d4ca766 --- /dev/null +++ b/.agent-team/prometheus-knowledge-maintainers/findings/issue-15-preserve-wiki-index.md @@ -0,0 +1,86 @@ +# Verifier findings: issue #15 preserve-wiki-index + +- Range: `main (46b08eb)..fix/preserve-wiki-index (602c856)`, commits 937cf4c, 5e71337, 5151070, 9e1e962, 40442b6, 602c856 +- Date: 2026-10-03 +- Review type: **separate-context review (same model family); human PR review required before merge** +- Verdict: **PASS WITH FINDINGS** (0 BLOCKER, 0 MAJOR, 5 MINOR, 6 NIT) + +## Summary + +The issue #15 failure modes are fixed. A stale in-memory snapshot no longer drops entries: the index is now rendered from a fresh disk scan under an exclusive lock. Index lines for pages that exist on disk but fail to parse are carried over verbatim. The `okf_version` block is always emitted. `index.md` and `log.md` are replaced atomically, and `append_log` shares the lock. Locking is sound: the blocking `lock_exclusive` runs in `spawn_blocking`, the lock is held across scan, merge, render and write, the two methods never nest, and cancellation drops the `File`, which releases the lock. If every upsert is followed by its own rebuild, the last rebuild sees all pages. The public API change is additive: `bundle::merge_preserved_index_entries` is new and no signatures changed. Output for parseable entries matches the old `render_index` byte for byte. The only difference is that ties on case-insensitive titles are now broken deterministically, where the old order came from HashMap order. Frozen surfaces are untouched. The learning worker's ledger code is unchanged. + +## Findings + +### F1 MINOR: a carried line can survive forever when the page parses under a different id +`pk-store/src/bundle.rs:158-190`, `pk-store/src/store.rs:254-266`. A line is carried when its id is not in `parsed_ids` and `.md` exists. The check compares the *frontmatter* id with the *path*. +- Scenario A: `wiki/foo.md` has frontmatter `id: bar`. The fresh line is `/bar.md`. `/foo.md` is not in `parsed_ids` and `foo.md` exists, so the old `* [Foo](/foo.md)` line is kept on every rebuild and the page is listed twice, for good. +- Scenario B: on a case-insensitive filesystem (macOS, Windows CI), an existing line `/Foo.md` with file `foo.md` and id `foo` passes `is_file()`, so it is kept forever. +- Fix: have `scan_wiki_tree` return the relative paths that failed to read or parse. Carry a line only when its exact path is in that set, instead of "exists and id not parsed". + +### F2 MINOR: a multi-line title or description is silently cut, but only when something is carried +`bundle.rs:181-189`. The merge re-parses `rendered` text with `index_sections`, and that keeps only lines starting `* [`. If an LLM-written description contains `\n`, `render_index` emits it across two lines. On the merge path the second line is dropped. A newline inside a title drops the whole entry. The output then differs depending on whether anything was carried. +- Fix: build the merge from the structured groups (refactor `render_index` into a group builder plus `render_index_groups`) rather than re-parsing its output. Optionally collapse newlines in title and description. + +### F3 MINOR: a new on-disk artifact in a git-tracked wiki needs a downstream handoff note +`store.rs:451, 489-506`. The change creates `wiki/.index.lock` on first use, and a crash between create and rename leaves `.index.md....tmp` / `.log.md.*.tmp` files behind. Neither is a `.md` file, so scan, lint and conformance ignore them (verified). But issue #15 shows consumers commit the wiki, so `git add -A` will commit the lock file and any leftover temp files. +- Fix: have `pk init` write `wiki/.gitignore` (`.index.lock`, `.*.tmp`), or document it. Include this in the task 4.1 handoff note: skill-system docs and the release-version-matrix entry. Optionally sweep stale temp files while holding the lock. + +### F4 MINOR: every rebuild parses the whole wiki while holding the cross-process lock +`store.rs:255`. Before, the index came from the in-memory snapshot. Now each ingest parses every page and SHA-hashes every file while holding the exclusive lock, and concurrent `append_log` calls wait for it. The `scan_wiki_tree` comment already reports that large stores take minutes. Each ingest now pays the full open-time scan cost twice. +- Fix: use a scan that only lists ids and pages that fail to parse (no hashing, no TextIndex). Or measure this against a large wiki before release. + +### F5 MINOR: process issues +- A dependency edit (`fs2` added to `pk-store/Cargo.toml` and `Cargo.lock`) landed in the `docs(openspec)` commit 937cf4c. Rule 5 makes a new crate dependency a request to knowledge-lead, even though `fs2` is already a workspace dependency. Record that approval in the PR. +- `tasks.md` boxes 1.1, 1.2 and 2.1 are still unchecked. Task 4.1 (handoff) is open. + +### NITs +- N1 `bundle.rs:164-178`: carried lines with the same id but different text, or the same line under two sections, are not deduplicated. Only byte-identical lines are, so the page is listed twice. +- N2 `store.rs:262`: a non-UTF-8 `index.md` is decoded lossily, so U+FFFD is written permanently into carried lines. This is still better than the old `log.md` behaviour, where `unwrap_or_default` wiped the file. +- N3 `store.rs:489-506`: the directory is not fsynced after the rename, so on power loss the rename may not be durable. The file is still never truncated. +- N4 Windows: `std::fs::rename` over `index.md` fails with ERROR_ACCESS_DENIED if another process holds it open without FILE_SHARE_DELETE (some editors, AV). The error is surfaced, not silent. I did not exercise this here: the gate ran on darwin only. +- N5 There is no reentrancy guard. A future caller that holds `.index.lock` and calls `regenerate_index` or `append_log` would deadlock itself, because flock on a second fd blocks. Document it on `lock_index`. +- N6 RED 9e1e962 (concurrent `append_log`) depends on a race and may pass on main by chance. 5e71337 is deterministic RED. TDD order holds otherwise: both RED commits contain only tests and precede their GREEN commits. 602c856 is a post-GREEN regression test, as tasks.md 2.1 specifies. It does cross the worker boundary: it runs the real `prometheus-learning-worker run-once` binary on a schemaVersion 2 job, which reaches `store.regenerate_index` at `pk-learning-worker/src/main.rs:547`. + +## Spec coverage +All three spec requirements and all four scenarios have tests (store tests plus bundle unit tests). Nothing in the diff goes beyond the spec. The CRLF, sectionless-line, reserved-name and `..` cases are covered by unit tests. + +## Gate (darwin, `CARGO_TARGET_DIR=/verifier-target`, run serially, no hook blocks) +| Command | Result | +|---|---| +| `cargo fmt --all -- --check` | PASS (exit 0) | +| `cargo clippy --workspace --all-targets --locked -- -D warnings` | PASS (exit 0) | +| `cargo test --workspace --locked` | PASS (exit 0): 168 passed, 0 failed, 0 ignored | + +## Confirmation pass (602c856..3691130, 2026-10-03) + +Commits reviewed: f57db3f (RED, tests only), 0e27913 (GREEN, pk-store only), 3691130 (docs). The RED commit comes before the GREEN commit. + +| Finding | Status | Evidence | +|---|---|---| +| F1 | RESOLVED | `scan_wiki_tree` now records `failed_paths`, the exact path-derived fallback ids of pages that failed to read or parse (`store.rs` scan). `render_index_preserving` carries a line only when its target is in that set (`bundle.rs`). A page whose filename differs from its id, or a link that differs only in case, is no longer carried. Covered by the store test `page_whose_filename_differs_from_its_id_is_listed_once` and the unit test `preserving_never_carries_a_parsed_page_listed_under_another_id`. | +| F2 | RESOLVED | The merge is now structural: `entry_groups` produces sort keys and lines, the carried lines are added, and `render_index_groups` renders once. Rendered text is never re-parsed. Covered by `multi_line_description_survives_when_a_line_is_carried` and `preserving_keeps_multi_line_fresh_entries_when_carrying`. | +| F3 | RESOLVED | The repo `.gitignore` ignores `wiki/.index.lock` and `wiki/.*.tmp`. README and design.md tell consumers to do the same. Handoff note item 6 asks the skill system to add these patterns to its scaffolding. Temp-file sweeping is not implemented, which is acceptable. | +| F4 | DEFERRED (accepted) | Measure first. Not a merge blocker. | +| F5 | RESOLVED | tasks.md is ticked, apart from 3.1, which is correct. proposal.md:30 records the `fs2` dependency, which knowledge-lead authored and the lead's docs commit carries. The PR should still state the rule-5 approval explicitly. | +| N1 | RESOLVED | `carried_targets` keeps at most one line per target, whatever its text. Covered by `preserving_files_sectionless_lines_under_uncategorized_once_per_target`. | +| N5 | RESOLVED | Doc comments on `regenerate_index`, `append_log` and `lock_index` say the lock is not reentrant. No runtime guard, which is acceptable. | + +Regression checks: +- **Byte-identical rendering:** `render_index` is `render_index_groups(entry_groups(..))`. It sorts by (lowercased real title, line) and has the same version block, sections, description trimming and placeholder as before. Ties are still broken deterministically. The sort key now comes from the real title instead of a re-parsed one, which is strictly better. `preserving_matches_render_index_when_nothing_is_carried` asserts equality, including a multi-line description. +- **Unreadable or non-UTF-8 pages:** a page whose bytes are not UTF-8 fails `read_to_string`, its path goes into `failed_paths`, and its line is carried. Correct. +- **Path normalisation:** components are joined with `/` (Windows-safe) and reserved files are skipped before recording. Traversal targets (`/../x.md`) can never match a failed path. + +New NITs, none blocking: +- C1: matching is case-sensitive. If a page fails to parse and its existing index line differs in case from the filename (`/Foo.md` for `foo.md`), the line is now dropped rather than kept. pk always writes the link from the id, which equals the path, so this only affects hand-edited indexes. +- C2: if `foo.md` fails to parse while another file parses with frontmatter `id: foo`, `/foo.md` is listed twice: once fresh and once carried. This needs an id collision. +- C3: a non-UTF-8 *filename* is converted lossily to U+FFFD, so it may not match the index line. Very rare. +- The earlier N2, N3, N4 and N6 nits still stand. + +### Gate (second run, same `CARGO_TARGET_DIR`, serial, darwin, no hook blocks) +| Command | Result | +|---|---| +| `cargo fmt --all -- --check` | PASS (exit 0) | +| `cargo clippy --workspace --all-targets --locked -- -D warnings` | PASS (exit 0, 0 warnings) | +| `cargo test --workspace --locked` | PASS (exit 0): 171 passed, 0 failed, 0 ignored. The previous run had 168: this pass adds 2 store tests and 1 net bundle unit test. | + +**Final verdict: PASS.** F4 is deferred and the remaining nits are non-blocking. This is a separate-context review by the same model family; a human must review the PR before merge. diff --git a/openspec/changes/preserve-wiki-index/tasks.md b/openspec/changes/preserve-wiki-index/tasks.md index 347029b..3969dfb 100644 --- a/openspec/changes/preserve-wiki-index/tasks.md +++ b/openspec/changes/preserve-wiki-index/tasks.md @@ -14,7 +14,7 @@ ## 3. Verification (verifier) -- [ ] 3.1 Adversarial review and the serialized integration gate +- [x] 3.1 Adversarial review and the serialized integration gate (PASS after one correction cycle; findings in .agent-team/prometheus-knowledge-maintainers/findings/issue-15-preserve-wiki-index.md) ## 4. Downstream (knowledge-lead)