diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 75e40152..24eb3887 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -318,6 +318,129 @@ jobs: if: always() && env.RUSTC_WRAPPER == 'sccache' run: sccache --show-stats || true + # PROB-102: the `check` job above proves the semantic-search feature + # COMPILES. It has never proven the embedding engine is CORRECT — that + # needs `tests/embedding_reference.rs`, which is entirely behind the + # feature, so `test`'s plain `cargo nextest run --workspace` never even + # compiles it, let alone runs it. `0 passed` for a file with three real + # assertions read as success on every PR since v0.35.0 (the ONNX -> tract + # engine swap this oracle exists to catch). + # + # This job pays the cost the `check` job comment said "is not worth + # downloading per run": the model is ~2.1 GB, cached across runs by + # `actions/cache` keyed on the model repo name, so only the FIRST run + # after a cache eviction pays the download. `FORGEPLAN_REQUIRE_MODEL_IN_ + # TESTS=1` makes a cold or broken cache a loud red failure instead of the + # oracle's normal quiet local-dev skip (an early `return` on a missing + # model reads as PASS to nextest — the same defect one level down inside + # the fix for it, so this job would silently prove nothing if the cache + # ever failed to warm and the guard were not here). + test-embedding-oracle: + name: Embedding correctness oracle + runs-on: ubuntu-latest + needs: check + timeout-minutes: 20 + steps: + - name: Checkout + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + + - name: Install Rust stable + uses: dtolnay/rust-toolchain@29eef336d9b2848a0b548edc03f92a220660cdb8 # stable branch @ 2026-03-27 (no tag; verify with git ls-remote) + + - name: Install protoc + uses: arduino/setup-protoc@c65c819552d16ad3c9b72d9dfd5ba5237b9c906b # v3.0.0 + with: + repo-token: ${{ secrets.GITHUB_TOKEN }} + + - name: Install sccache + uses: mozilla-actions/sccache-action@9e7fa8a12102821edf02ca5dbea1acd0f89a2696 # v0.0.10 + + - name: Probe sccache backend + # Same probe pattern as the check/test jobs — see comment there. + run: | + export SCCACHE_GHA_ENABLED=true + if timeout 30 sccache --start-server >/dev/null 2>&1; then + echo 'fn main() {}' > /tmp/probe.rs + if timeout 30 sccache rustc \ + --edition 2021 \ + --crate-name probe \ + --crate-type bin \ + /tmp/probe.rs \ + -o /tmp/probe 2>/dev/null; then + echo "RUSTC_WRAPPER=sccache" >> "$GITHUB_ENV" + echo "SCCACHE_GHA_ENABLED=true" >> "$GITHUB_ENV" + echo "✓ sccache probe passed — wrapper enabled" + else + sccache --stop-server >/dev/null 2>&1 || true + echo "✗ sccache probe failed (rustc invocation) — falling back to direct rustc" + fi + else + echo "✗ sccache probe failed (server start) — falling back to direct rustc" + fi + + - name: Rust cache + uses: Swatinem/rust-cache@c19371144df3bb44fab255c43d04cbc2ab54d1c4 # v2.9.1 + with: + key: embedding-oracle + + - name: Cache embedding model + # `resolve_cache_dir()` (crates/forgeplan-core/src/embed/mod.rs) puts + # the model under the platform cache dir when `FORGEPLAN_MODEL_CACHE` + # is unset — `~/.cache/forgeplan/models` on this runner. Keyed on the + # model repo name, not a version tag: the model itself doesn't churn + # per-PR, and a stale-but-present cache is exactly what this job's + # `FORGEPLAN_REQUIRE_MODEL_IN_TESTS` guard is for — if the cached + # weights were ever wrong, the oracle's vector comparison would be + # the thing that catches it, not the cache key. + uses: actions/cache@0057852bfaa89a56745cba8c7296529d2fc39830 # v6.1.0 + with: + path: ~/.cache/forgeplan/models + key: embedding-model-bge-m3 + + - name: Install cargo-nextest + uses: taiki-e/install-action@3d7d7cd5ac7f994c1892ae0c06165095b9139094 # v2.85.1 + with: + tool: cargo-nextest + + - name: cargo nextest run (embedding oracle) + # Two independent fail-closed paths, verified separately rather than + # assumed: + # 1. `--test embedding_reference` with the feature dropped compiles + # zero tests. Verified locally: nextest exits 4 with + # "error: no tests to run" on its own — no extra guard needed + # for that case. + # 2. The feature present but the model missing/broken: without + # `FORGEPLAN_REQUIRE_MODEL_IN_TESTS`, `embedder_or_skip` returns + # `None` and each test `return`s early, which nextest reports as + # PASS — this is the actual PROB-102 shape, one level inside the + # fix for it. `=1` turns that `None` into a panic instead. + # Verified locally by forcing `Embedder::new()` to `Err`: the + # job fails loudly with `=1` set, passes silently without it. + # + # `--test-threads=1`: found the hard way, on this job's own first + # cold-cache run. `dimension_is_unchanged` and `embeddings_match_ + # the_captured_reference` both call `Embedder::new()`, and nextest + # defaults to running them concurrently. Against a genuinely empty + # cache both start downloading the same ~2.1 GB model into the same + # directory at once — one finished in 62s, the other failed 7s in + # trying to fetch `onnx/model.onnx_data`. Once `actions/cache` has + # populated the directory this race cannot occur (`find_snapshot` + # returns immediately, no write happens) — but the FIRST run after + # any cache eviction hits it every time, and that run reporting red + # for a race rather than a real defect is exactly the kind of noise + # that trains people to re-run and ignore, not investigate. Serial + # execution removes the download race outright; two tests don't + # need parallelism. + env: + FORGEPLAN_REQUIRE_MODEL_IN_TESTS: "1" + run: | + cargo nextest run -p forgeplan-core --features semantic-search \ + --test embedding_reference --no-fail-fast --test-threads=1 + + - name: sccache stats + if: always() && env.RUSTC_WRAPPER == 'sccache' + run: sccache --show-stats || true + smoke-e2e: name: End-to-end smoke test runs-on: ubuntu-latest diff --git a/crates/forgeplan-cli/src/commands/deprecate.rs b/crates/forgeplan-cli/src/commands/deprecate.rs index 3a5dc2d0..5594e97a 100644 --- a/crates/forgeplan-cli/src/commands/deprecate.rs +++ b/crates/forgeplan-cli/src/commands/deprecate.rs @@ -32,10 +32,21 @@ pub async fn run(id: &str, reason: &str) -> anyhow::Result<()> { .await .map_err(|e| anyhow::anyhow!("{}\nFix: forgeplan validate {}", e, id))?; - // Re-render projection with updated status + // Re-render projection with the updated status AND the appended body. + // + // #478: this used `render_projection`, which is files-first — it discards + // the body it is handed whenever the file already has one (RFC-004, so a + // user's edits survive `link`/`tag`/`activate`). The status reached the + // file because status lives in frontmatter; the `## Deprecation` section + // did not. `lance/` is gitignored, and the next lifecycle command syncs + // the section-less file body back over the DB, so the reason ended up + // nowhere at all. + // + // Forcing is safe *here* because `sync_file_to_store` ran above: at this + // point the DB body is the file body plus the section just appended. if let Some(record) = store.get_record(id).await? { let links = store.get_relations(id).await.unwrap_or_default(); - projection::render_projection( + projection::render_projection_with_body( &ws, &record.id, &record.kind, diff --git a/crates/forgeplan-cli/src/commands/renew.rs b/crates/forgeplan-cli/src/commands/renew.rs index b010d225..1819453b 100644 --- a/crates/forgeplan-cli/src/commands/renew.rs +++ b/crates/forgeplan-cli/src/commands/renew.rs @@ -21,10 +21,12 @@ pub async fn run(id: &str, reason: &str, until: &str) -> anyhow::Result<()> { let result = lifecycle::renew(&store, id, reason, until).await?; - // Re-render projection with updated status + // Re-render projection with the updated status AND the appended body. + // #478 — `render_projection` is files-first and would drop the + // `## Renewal` section. Safe to force: `sync_file_to_store` ran above. if let Some(record) = store.get_record(id).await? { let links = store.get_relations(id).await.unwrap_or_default(); - projection::render_projection( + projection::render_projection_with_body( &ws, &record.id, &record.kind, diff --git a/crates/forgeplan-cli/src/commands/reopen.rs b/crates/forgeplan-cli/src/commands/reopen.rs index 969a6009..f1799b56 100644 --- a/crates/forgeplan-cli/src/commands/reopen.rs +++ b/crates/forgeplan-cli/src/commands/reopen.rs @@ -34,13 +34,20 @@ pub async fn run(id: &str, reason: &str) -> anyhow::Result<()> { .await .map_err(|e| anyhow::anyhow!("{}\nFix: forgeplan validate {}", e, id))?; - // Render projections for both old (deprecated) and new (draft) + // Render projections for both old (deprecated) and new (draft). + // + // #478 — the OLD artifact needs the forcing variant: `render_projection` + // is files-first and dropped its `## Reopened` section, so the record of + // why it was retired never reached the file. Safe to force here because + // `sync_file_to_store` ran above. The NEW artifact below keeps the plain + // call: its file does not exist yet, so the renderer already takes the + // passed body. if let Some(old_record) = store.get_record(&result.old_id).await? { let links = store .get_relations(&result.old_id) .await .unwrap_or_default(); - projection::render_projection( + projection::render_projection_with_body( &ws, &old_record.id, &old_record.kind, diff --git a/crates/forgeplan-cli/tests/cli_integration_test.rs b/crates/forgeplan-cli/tests/cli_integration_test.rs index 7d4be43b..0e1227a0 100644 --- a/crates/forgeplan-cli/tests/cli_integration_test.rs +++ b/crates/forgeplan-cli/tests/cli_integration_test.rs @@ -2918,6 +2918,188 @@ fn e2e_full_lifecycle_deprecate() { .stdout(predicate::str::contains("deprecated")); } +// ----------------------------------------------------------------------- +// #478: the lifecycle reason must reach the FILE, not just LanceDB. +// +// The tests above, and every unit test in `lifecycle/mod.rs`, assert through +// `forgeplan get` — which reads the store. The store was never the broken +// half. `deprecate` appended its `## Deprecation` section to LanceDB and the +// files-first renderer discarded it, so the status reached the markdown and +// the reason did not. `lance/` is gitignored, so on a fresh clone the reason +// did not exist anywhere; worse, the next mutation synced the section-less +// file body back over the store, erasing it there too. +// +// These read the `.md` off disk. Asserting through the store is precisely +// what let the defect ship. +// ----------------------------------------------------------------------- + +/// Read an artifact's markdown projection from a workspace. +fn read_projection(workspace: &std::path::Path, dir: &str, prefix: &str) -> String { + let d = workspace.join(".forgeplan").join(dir); + let entry = std::fs::read_dir(&d) + .unwrap_or_else(|e| panic!("cannot read {}: {e}", d.display())) + .filter_map(Result::ok) + .find(|e| e.file_name().to_string_lossy().starts_with(prefix)) + .unwrap_or_else(|| panic!("no file starting with {prefix} in {}", d.display())); + std::fs::read_to_string(entry.path()).unwrap() +} + +#[test] +fn deprecate_writes_the_reason_into_the_markdown_file() { + let tmp = TempDir::new().unwrap(); + forgeplan() + .args(["init", "-y"]) + .current_dir(tmp.path()) + .assert() + .success(); + forgeplan() + .args(["new", "note", "Lifecycle Test"]) + .current_dir(tmp.path()) + .assert() + .success(); + forgeplan() + .args(["activate", "NOTE-001"]) + .current_dir(tmp.path()) + .assert() + .success(); + forgeplan() + .args(["deprecate", "NOTE-001", "--reason", "replaced by REPRO-478"]) + .current_dir(tmp.path()) + .assert() + .success(); + + let md = read_projection(tmp.path(), "notes", "NOTE-001"); + assert!( + md.contains("## Deprecation"), + "the markdown file must carry the Deprecation section, got:\n{md}" + ); + assert!( + md.contains("Reason: replaced by REPRO-478"), + "the markdown file must carry the reason itself, got:\n{md}" + ); + assert!( + md.contains("status: deprecated"), + "status must still project, got:\n{md}" + ); +} + +/// The permanent-loss half. When the file lacks a section the store has, the +/// two disagree — and `read_file_body_if_newer` compares content, so the next +/// mutation writes the file body over the store and the reason is gone from +/// both. Agreement after the command is what makes the loss impossible. +#[test] +fn after_deprecate_the_file_and_the_store_agree() { + let tmp = TempDir::new().unwrap(); + forgeplan() + .args(["init", "-y"]) + .current_dir(tmp.path()) + .assert() + .success(); + forgeplan() + .args(["new", "note", "Divergence Test"]) + .current_dir(tmp.path()) + .assert() + .success(); + forgeplan() + .args(["activate", "NOTE-001"]) + .current_dir(tmp.path()) + .assert() + .success(); + forgeplan() + .args(["deprecate", "NOTE-001", "--reason", "AGREEMENT-MARKER"]) + .current_dir(tmp.path()) + .assert() + .success(); + + let md = read_projection(tmp.path(), "notes", "NOTE-001"); + assert!( + md.contains("AGREEMENT-MARKER"), + "file lost the reason:\n{md}" + ); + + // `get` reads the store. Both surfaces must show it. + forgeplan() + .args(["get", "NOTE-001"]) + .current_dir(tmp.path()) + .assert() + .success() + .stdout(predicate::str::contains("AGREEMENT-MARKER")); +} + +#[test] +fn reopen_writes_its_reason_into_the_retired_artifacts_file() { + let tmp = TempDir::new().unwrap(); + forgeplan() + .args(["init", "-y"]) + .current_dir(tmp.path()) + .assert() + .success(); + forgeplan() + .args(["new", "note", "Reopen Test"]) + .current_dir(tmp.path()) + .assert() + .success(); + forgeplan() + .args(["activate", "NOTE-001"]) + .current_dir(tmp.path()) + .assert() + .success(); + forgeplan() + .args(["reopen", "NOTE-001", "--reason", "REOPEN-MARKER-478"]) + .current_dir(tmp.path()) + .assert() + .success(); + + // The OLD artifact is the one that loses its section — the new one's file + // does not exist yet at render time, so it always kept its body. + let md = read_projection(tmp.path(), "notes", "NOTE-001"); + assert!( + md.contains("## Reopened"), + "the retired artifact must record why it was retired, got:\n{md}" + ); + assert!( + md.contains("REOPEN-MARKER-478"), + "the retired artifact must carry the reason, got:\n{md}" + ); +} + +#[test] +fn renew_writes_its_reason_into_the_markdown_file() { + let tmp = TempDir::new().unwrap(); + forgeplan() + .args(["init", "-y"]) + .current_dir(tmp.path()) + .assert() + .success(); + forgeplan() + .args(["new", "note", "Renew Test"]) + .current_dir(tmp.path()) + .assert() + .success(); + forgeplan() + .args([ + "renew", + "NOTE-001", + "--reason", + "RENEW-MARKER-478", + "--until", + "2099-01-01", + ]) + .current_dir(tmp.path()) + .assert() + .success(); + + let md = read_projection(tmp.path(), "notes", "NOTE-001"); + assert!( + md.contains("## Renewal"), + "the markdown file must carry the Renewal section, got:\n{md}" + ); + assert!( + md.contains("RENEW-MARKER-478"), + "the markdown file must carry the reason, got:\n{md}" + ); +} + // ----------------------------------------------------------------------- // ADR-005: draft → deprecated directly is NOT allowed // ----------------------------------------------------------------------- diff --git a/crates/forgeplan-cli/tests/lifecycle_body_projection_invariant.rs b/crates/forgeplan-cli/tests/lifecycle_body_projection_invariant.rs new file mode 100644 index 00000000..477a4389 --- /dev/null +++ b/crates/forgeplan-cli/tests/lifecycle_body_projection_invariant.rs @@ -0,0 +1,94 @@ +//! #478 regression guard. +//! +//! `lifecycle::{deprecate, renew, reopen}` append a section to the body +//! (`## Deprecation` / `## Renewal` / `## Reopened`) and push it through +//! `LanceStore::update_body` only. The CLI/MCP caller must then project with +//! the *_with_body variant — `render_projection_with_body` / +//! `render_after_mutation_with_body` — or the section is silently dropped: +//! the plain `render_projection` is files-first (RFC-004) and discards +//! whatever body it is handed whenever the file already has a non-empty one. +//! Status still reaches the file (it lives in frontmatter), so this reads as +//! success everywhere except the one place that matters. +//! +//! Worse, the loss compounds: `read_file_body_if_newer` compares content, not +//! mtime, so the *next* mutation on that artifact syncs the section-less file +//! body back over LanceDB, erasing the reason there too. +//! +//! This is a source-grep invariant, not a behavioural test — the behavioural +//! coverage lives in `forgeplan-cli/tests/cli_integration_test.rs` +//! (`deprecate_writes_the_reason_into_the_markdown_file` and siblings), which +//! read the `.md` off disk. This test exists so a *future* call site cannot +//! reintroduce the plain renderer without a compile-time-adjacent failure — +//! the existing unit tests in `lifecycle/mod.rs` assert through the store and +//! would not have caught this the first time. +//! +//! One caller is exempt: `reopen`'s NEW artifact. Its file does not exist yet +//! at render time, so the plain (non-forcing) renderer already takes the +//! passed body — forcing there would be a no-op, not a bug. + +use std::fs; +use std::path::Path; + +/// (file relative to `forgeplan-cli/src/commands/`, section this call site is +/// responsible for landing in the file, must appear exactly this many times) +const REQUIRED_FORCING_CALLS: &[(&str, &str, usize)] = &[ + ("deprecate.rs", "## Deprecation", 1), + ("renew.rs", "## Renewal", 1), + // reopen.rs handles two artifacts: the retired one (needs forcing) and + // the freshly-created one (does not — see module doc). + ("reopen.rs", "## Reopened", 1), +]; + +#[test] +fn cli_lifecycle_commands_use_the_forcing_projection() { + let dir = Path::new("src/commands"); + for (file, section, expected) in REQUIRED_FORCING_CALLS { + let path = dir.join(file); + let text = fs::read_to_string(&path) + .unwrap_or_else(|e| panic!("cannot read {}: {e}", path.display())); + let forcing_calls = text.matches("render_projection_with_body(").count(); + assert!( + forcing_calls >= *expected, + "{}: expected at least {expected} call(s) to \ + `render_projection_with_body`, found {forcing_calls}. \ + The plain `render_projection` is files-first and will silently \ + drop the {section} section it is asked to write — see #478.", + path.display() + ); + } +} + +#[test] +fn reopen_leaves_the_new_artifacts_plain_render_alone() { + // Documents the one legitimate plain call, so a future edit that removes + // it (thinking both should force) gets a signal rather than silence. + let path = Path::new("src/commands/reopen.rs"); + let text = + fs::read_to_string(path).unwrap_or_else(|e| panic!("cannot read {}: {e}", path.display())); + let plain_calls = text.matches("projection::render_projection(").count(); + assert_eq!( + plain_calls, 1, + "reopen.rs should call the plain (non-forcing) `render_projection` \ + exactly once, for the newly-created artifact whose file does not \ + exist yet. A count of 0 means someone deleted the new-artifact \ + render; a count > 1 means the retired-artifact call site regressed \ + back to the plain renderer (#478)." + ); +} + +/// The MCP path has no dedicated integration test (unlike the CLI, which is +/// covered end-to-end in `cli_integration_test.rs`), so this one carries both +/// jobs: source-grep AND the reason it matters. +#[test] +fn mcp_deprecate_handler_uses_the_forcing_projection() { + let path = Path::new("../forgeplan-mcp/src/server.rs"); + let text = + fs::read_to_string(path).unwrap_or_else(|e| panic!("cannot read {}: {e}", path.display())); + assert!( + text.contains("render_after_mutation_with_body"), + "forgeplan-mcp/src/server.rs must call \ + `render_after_mutation_with_body` after `lifecycle::deprecate` — the \ + plain `render_after_mutation` is files-first and drops the \ + `## Deprecation` section the same way the CLI command did (#478)." + ); +} diff --git a/crates/forgeplan-core/src/projection/mod.rs b/crates/forgeplan-core/src/projection/mod.rs index 0b59f7b7..6a55ff0e 100644 --- a/crates/forgeplan-core/src/projection/mod.rs +++ b/crates/forgeplan-core/src/projection/mod.rs @@ -167,10 +167,45 @@ pub async fn render_projection( /// Render a full ArtifactRecord (includes tags) to its markdown file. /// Used by mutations like `tag` / `untag` that need to persist tags to /// frontmatter so they survive a reindex (ADR-003 files-first). +/// +/// Files-first: an existing non-empty file body wins over `record.body`. +/// For a mutation that *appends to the body* (the lifecycle sections), use +/// [`render_projection_record_with_body`] instead — see #478. pub async fn render_projection_record( workspace: &Path, record: &crate::db::store::ArtifactRecord, links: &[(String, String)], +) -> anyhow::Result { + render_projection_record_inner(workspace, record, links, false).await +} + +/// Like [`render_projection_record`], but writes `record.body` verbatim +/// instead of preserving whatever the file already holds. +/// +/// #478: `deprecate` / `renew` / `reopen` append a `## Deprecation` / +/// `## Renewal` / `## Reopened` section to the body. Rendered files-first, +/// that section is discarded — the status reaches the file (it is +/// frontmatter) and the reason does not. Because `lance/` is gitignored and +/// the next mutation syncs the section-less file body back over the DB, the +/// reason ends up in neither place. +/// +/// Only safe when the caller has just synced file → store, so that +/// `record.body` is the file body plus the appended section. Do not reach +/// for this in `link` / `tag` / `activate`: they pass a possibly-stale DB +/// body, and files-first is what protects a user's edits there. +pub async fn render_projection_record_with_body( + workspace: &Path, + record: &crate::db::store::ArtifactRecord, + links: &[(String, String)], +) -> anyhow::Result { + render_projection_record_inner(workspace, record, links, true).await +} + +async fn render_projection_record_inner( + workspace: &Path, + record: &crate::db::store::ArtifactRecord, + links: &[(String, String)], + force_body: bool, ) -> anyhow::Result { let artifact_kind = record .kind @@ -184,7 +219,10 @@ pub async fn render_projection_record( let filepath = dir.join(&filename); // Files-first: preserve existing body + agent-owned fm keys (PRD-057 FR-009). - let (effective_body, preserved_fm) = if filepath.exists() { + // `force_body` (#478) opts out for mutations that append to the body. + let (effective_body, preserved_fm) = if force_body { + (record.body.clone(), read_preserved_fm(&filepath).await) + } else if filepath.exists() { match tokio::fs::read_to_string(&filepath).await { Ok(file_content) => match frontmatter::parse_frontmatter(&file_content) { Ok((fm, file_body)) => { @@ -396,6 +434,24 @@ pub async fn render_after_mutation( Ok(()) } +/// Like [`render_after_mutation`], but writes the store's body verbatim. +/// +/// For handlers whose mutation *appended to the body* — the lifecycle +/// sections. The plain variant is files-first and silently drops them +/// (#478). Requires that the handler synced file → store first, which +/// `sync_before_mutation` does. +pub async fn render_after_mutation_with_body( + workspace: &Path, + store: &crate::db::store::LanceStore, + id: &str, +) -> anyhow::Result<()> { + if let Some(record) = store.get_record(id).await? { + let links = store.get_relations(id).await.unwrap_or_default(); + render_projection_record_with_body(workspace, &record, &links).await?; + } + Ok(()) +} + /// Sync file body to LanceDB store if file was edited by user. /// Call this before render_projection to ensure LanceDB has the latest body. /// Returns true if sync happened (file was newer). diff --git a/crates/forgeplan-core/tests/embedding_reference.rs b/crates/forgeplan-core/tests/embedding_reference.rs index 58196b07..510c52a6 100644 --- a/crates/forgeplan-core/tests/embedding_reference.rs +++ b/crates/forgeplan-core/tests/embedding_reference.rs @@ -151,10 +151,27 @@ fn read_json_string(s: &str) -> String { /// Returns `None` when the model is not on this machine. Deliberately loud: /// "skipped" alone would read as "checked and fine" in a scroll-back, and this /// is the one test whose silence is indistinguishable from success. +/// +/// PROB-102: an early `return` here reports as a PASS to nextest — zero +/// assertions ran, and the exit code says everything is fine. That is exactly +/// the failure class this oracle exists to catch, one level down. The CI job +/// that runs this file with a warm model cache sets +/// `FORGEPLAN_REQUIRE_MODEL_IN_TESTS=1` so a cold or broken cache is a loud, +/// red failure instead of a silent green one. Local runs without the env var +/// keep the quiet skip — a developer without the 2.1 GB model on their +/// machine should not be blocked from running the rest of the suite. fn embedder_or_skip(test_name: &str) -> Option { match forgeplan_core::embed::Embedder::new() { Ok(e) => Some(e), Err(err) => { + if require_model_in_tests_from(std::env::var("FORGEPLAN_REQUIRE_MODEL_IN_TESTS").ok()) { + panic!( + "{test_name}: FORGEPLAN_REQUIRE_MODEL_IN_TESTS=1 and the model \ + is unavailable — {err}. This is the CI oracle job; a cold or \ + broken model cache must fail loudly, not silently pass with \ + zero assertions run (PROB-102)." + ); + } eprintln!( "\n!! {test_name} DID NOT RUN — NOTHING WAS VERIFIED.\n\ !! The embedding model is not available on this machine:\n\ @@ -167,6 +184,56 @@ fn embedder_or_skip(test_name: &str) -> Option } } +/// The decision behind the panic branch above, with the env read pulled out +/// so it can be tested without touching process environment — the same +/// reasoning `embed::resolve_cache_dir_from` documents: mutating env vars in +/// tests is `unsafe` in Rust 2024 (the write races reads of any other +/// variable from other threads), and this crate already has enough +/// env-sensitive tests without adding one more. +/// +/// Anything other than exactly `"1"` is "not required" — an unset var, an +/// empty string, `"true"`, `"0"` all fall through to the quiet local skip. +/// A CI job that means to require the model sets it to `"1"`; anything else +/// reads as "not configured for this", not as "configured wrong". +fn require_model_in_tests_from(value: Option) -> bool { + value.as_deref() == Some("1") +} + +#[cfg(test)] +mod require_model_gate_tests { + use super::require_model_in_tests_from; + + /// The branch this session almost shipped without exercising: a cold + /// model cache under `FORGEPLAN_REQUIRE_MODEL_IN_TESTS` must fail loudly, + /// not return the quiet `None` that reads as a pass to nextest. This is + /// the pure decision the panic branch is gated on — the branch itself + /// needs a real `Embedder::new()` failure to exercise directly, which + /// needs either no network or a populated-then-emptied cache; both are + /// impractical to assert in a unit test. What IS practical, and what + /// actually carries the risk of a silent regression, is this: does the + /// gate read the env var correctly. + #[test] + fn require_1_means_required() { + assert!(require_model_in_tests_from(Some("1".to_string()))); + } + + #[test] + fn unset_means_not_required() { + assert!(!require_model_in_tests_from(None)); + } + + #[test] + fn anything_other_than_exactly_1_means_not_required() { + for v in ["0", "true", "yes", "TRUE", "1 ", " 1", ""] { + assert!( + !require_model_in_tests_from(Some(v.to_string())), + "{v:?} must not be treated as \"required\" — only the exact \ + string \"1\" opts in" + ); + } + } +} + /// The oracle itself: every fixture case must reproduce component-for-component. #[test] fn embeddings_match_the_captured_reference() { diff --git a/crates/forgeplan-mcp/src/server.rs b/crates/forgeplan-mcp/src/server.rs index 02d956a4..136023dc 100644 --- a/crates/forgeplan-mcp/src/server.rs +++ b/crates/forgeplan-mcp/src/server.rs @@ -4545,8 +4545,15 @@ impl ForgeplanServer { // markdown projection's `status:` frontmatter reflects the // transition. Without this, a CLI re-deprecate would see a // file `status: active` and a store `status: deprecated`. + // + // #478: must be the *_with_body variant. `deprecate` appends a + // `## Deprecation` section carrying the reason; the plain + // renderer is files-first and drops it, so the status reached + // the file and the reason did not. Safe here because + // `sync_before_mutation` ran above. if let Err(e) = - forgeplan_core::projection::render_after_mutation(&ws, &store, &p.id).await + forgeplan_core::projection::render_after_mutation_with_body(&ws, &store, &p.id) + .await { tracing::warn!( "post-mutation render for {} failed: {e} — \