diff --git a/.forgeplan/adrs/ADR-001-no-adapter-traits-ai-agent-is-the-orchestrator-not-forgeplan.md b/.forgeplan/adrs/ADR-001-no-adapter-traits-ai-agent-is-the-orchestrator-not-forgeplan.md index 2821f633..0f23e0a3 100644 --- a/.forgeplan/adrs/ADR-001-no-adapter-traits-ai-agent-is-the-orchestrator-not-forgeplan.md +++ b/.forgeplan/adrs/ADR-001-no-adapter-traits-ai-agent-is-the-orchestrator-not-forgeplan.md @@ -64,3 +64,4 @@ Accepted - crates/forgeplan-core/src/artifact/** + diff --git a/.forgeplan/adrs/ADR-002-r-eff-skips-non-active-dependencies-draft-deprecated-superseded.md b/.forgeplan/adrs/ADR-002-r-eff-skips-non-active-dependencies-draft-deprecated-superseded.md index cf331164..912b78c9 100644 --- a/.forgeplan/adrs/ADR-002-r-eff-skips-non-active-dependencies-draft-deprecated-superseded.md +++ b/.forgeplan/adrs/ADR-002-r-eff-skips-non-active-dependencies-draft-deprecated-superseded.md @@ -35,3 +35,4 @@ R_eff recursive вычисляет weakest link по всему дереву з - crates/forgeplan-core/src/scoring/reff.rs + diff --git a/.forgeplan/adrs/ADR-003-markdown-files-as-source-of-truth-lancedb-as-index-layer.md b/.forgeplan/adrs/ADR-003-markdown-files-as-source-of-truth-lancedb-as-index-layer.md index 5e703d72..59b1f3be 100644 --- a/.forgeplan/adrs/ADR-003-markdown-files-as-source-of-truth-lancedb-as-index-layer.md +++ b/.forgeplan/adrs/ADR-003-markdown-files-as-source-of-truth-lancedb-as-index-layer.md @@ -320,3 +320,4 @@ stays scoped to the typed-error migration: + diff --git a/.forgeplan/adrs/ADR-009-forgeplan-as-orchestrator-playbook-skill-agent-mapping-pack-marketplace-model.md b/.forgeplan/adrs/ADR-009-forgeplan-as-orchestrator-playbook-skill-agent-mapping-pack-marketplace-model.md index d3d5f987..049cabec 100644 --- a/.forgeplan/adrs/ADR-009-forgeplan-as-orchestrator-playbook-skill-agent-mapping-pack-marketplace-model.md +++ b/.forgeplan/adrs/ADR-009-forgeplan-as-orchestrator-playbook-skill-agent-mapping-pack-marketplace-model.md @@ -270,3 +270,4 @@ Forgeplan-core получает **3 новые core capabilities**: + diff --git a/.forgeplan/adrs/ADR-020-r-eff-excludes-terminal-status-evidence-from-the-weakest-link-min.md b/.forgeplan/adrs/ADR-020-r-eff-excludes-terminal-status-evidence-from-the-weakest-link-min.md index ee16540a..9e707fd4 100644 --- a/.forgeplan/adrs/ADR-020-r-eff-excludes-terminal-status-evidence-from-the-weakest-link-min.md +++ b/.forgeplan/adrs/ADR-020-r-eff-excludes-terminal-status-evidence-from-the-weakest-link-min.md @@ -79,3 +79,4 @@ title: R_eff excludes terminal-status evidence from the weakest-link min - **Фильтровать и draft (буквальный acceptance #436)** — отвергнуто: ломает score-гейт Standard-flow (см. Decision §2). + diff --git a/.forgeplan/adrs/ADR-025-orchestration-sits-above-forgeplan-the-core-is-the-contract-layer.md b/.forgeplan/adrs/ADR-025-orchestration-sits-above-forgeplan-the-core-is-the-contract-layer.md new file mode 100644 index 00000000..b1502fb1 --- /dev/null +++ b/.forgeplan/adrs/ADR-025-orchestration-sits-above-forgeplan-the-core-is-the-contract-layer.md @@ -0,0 +1,91 @@ +--- +depth: standard +id: ADR-025 +kind: adr +links: +- target: ADR-001 + relation: based_on +- target: ADR-009 + relation: refines +status: active +title: Orchestration sits above ForgePlan; the core is the contract layer +--- + +--- +assigned_number: 25 +predicted_number: 25 +slug: adr-orchestration-sits-above-forgeplan-the-core-is-the-contract-layer +--- + +# ADR-025: Orchestration sits above ForgePlan; the core is the contract layer + +## Context + +Two active ADRs contradict each other, and the contradiction predates vNext: + +- **ADR-001**: "AI agent is the orchestrator, not Forgeplan." Rejects adapter + traits; ForgePlan does not integrate into external systems, they read it. +- **ADR-009 §Decision**: "Forgeplan-core становится оркестратором — знает когда + какой playbook запускать, кому делегировать каждый шаг." + +The vNext audit flagged this as the blocker no document can resolve (B2: +"недостижим, пока человек не выберет сторону"). The owner has chosen: +**orchestration of agents lives above ForgePlan**. Orchestrators (Claude Code, +Kandev, Conductor, human operators) decide who runs and when. ForgePlan is the +system of record they run against: artifacts, contracts, evidence, verdicts, +lifecycle. + +The audit also found sixteen shipped CLI/MCP surfaces that sit on or across +this boundary (PB-01/B4) and demanded a per-surface disposition instead of a +blanket claim. This ADR is that disposition. + +## Decision + +ADR-001 is **reaffirmed**. ADR-009's orchestrator clause (§Decision, first +sentence) is **superseded by this ADR**; the rest of ADR-009 — the 4-primitive ++ Pack marketplace model — stands unchanged. + +The boundary test for any surface: **does it manage the artifact graph, or +does it manage a process?** Graph management stays in core. Process management +belongs to the orchestrator above. + +### Disposition of the sixteen surfaces + +| Surface | Verdict | Reasoning | +|---|---|---| +| `dispatch` | **KEEP** | Read-only planner: computes conflict-free buckets from the graph. Spawning was already documented as the orchestrator's job. A projection, not a process. | +| `order`, `blocked` | **KEEP** | Pure graph projections over dependency edges. | +| `progress` | **KEEP** | Reads FR checkboxes out of artifact bodies. A projection. | +| `graph`, `tree`, `stale`, `blindspots` | **KEEP** | Same class, never contested. | +| `claim` / `release` / `claims` | **KEEP, reframed as locks** | These are write-mutexes on artifacts — integrity infrastructure for one workspace, not work assignment. "Who is assigned" belongs to trackers; "who may write this artifact right now without collision" is the graph's own safety and stays. Docs and hints must stop using assignment language. | +| `session` | **KEEP, explicitly non-canonical** | Per-machine plumbing, already gitignored. | +| `phase` / `phase-advance` | **ABSORB, no new investment** | Three parallel state ladders exist today: lifecycle status, DerivedStatus, phase. Phase state is already per-machine (`.forgeplan/state/` is gitignored — it does not even travel with the repo). Keep advisory as shipped, fix the #330 regression because shipped code must not lie, and fold phase into the lifecycle model in vNext (FPV-03) rather than growing it. | +| `estimate` / `calibrate` | **MOVE TO EXTENSION** | Effort estimation is planning-tool territory. It reads the graph but does not manage it. Marketplace extension; deprecation window in core. | +| `remember` / `recall` (memory kind) | **DEPRECATE** | The boundary doc says "not a general-purpose memory platform" and the shipped reality agrees: 2 memory artifacts exist, `new memory` fails with "No template found", and #411 shows they cannot join the graph. NOTE covers durable engineering micro-facts as first-class artifacts; conversational memory is Hindsight's job. Closing #411 by removal, not repair. | +| playbook runtime (5 dispatchers, `playbook run`/`ingest`, ADR-011's `claude --print`) | **MOVE TO EXTENSION, supersede ADR-011** | Spawning agent processes is the definition of the orchestration this ADR places above the core. The playbook *format* (methodology → steps mapping) remains marketplace data; the *runtime* leaves the core binary. This is the largest consequence and gets its own migration RFC before any code moves. | + +## Consequences + +- The FPV-01 blocker (two active ADRs claiming opposite things) is resolved; + the vNext boundary doc's ownership table now matches an actual decision. +- ADR-009 needs an amendment note pointing here; ADR-011 needs supersession + when the playbook-runtime RFC lands. Neither is edited retroactively — + supersede, do not delete. +- #411 closes as deprecation. The two existing memory artifacts get migrated + to NOTE or exported before removal. +- No code changes in this ADR. Each MOVE/DEPRECATE row requires its own RFC + with a deprecation window; KEEP rows require only documentation alignment + (assignment language out of claim/release hints). + +## Related Artifacts + +| Artifact | Relation | +|---|---| +| ADR-001 | based_on | +| ADR-009 | refines | + + + + + + diff --git a/.forgeplan/adrs/ADR-026-storage-classes-for-machine-written-records.md b/.forgeplan/adrs/ADR-026-storage-classes-for-machine-written-records.md new file mode 100644 index 00000000..3e57f74f --- /dev/null +++ b/.forgeplan/adrs/ADR-026-storage-classes-for-machine-written-records.md @@ -0,0 +1,122 @@ +--- +depth: standard +id: ADR-026 +kind: adr +links: +- target: ADR-003 + relation: refines +- target: ADR-025 + relation: based_on +status: active +title: Storage classes for machine-written records +--- + +--- +assigned_number: 26 +predicted_number: 26 +slug: adr-storage-classes-for-machine-written-records +--- + +# ADR-026: Storage classes for machine-written records + +## Context + +vNext introduces four object classes that no shipped decision houses: +WorkContract, ExecutionReceipt, EvidenceBundle, VerificationVerdict, plus an +authority/audit trail. The audit blocked FPV-03/04/05 on this (B3): ADR-003 +declares markdown files the single source of truth and RED LINE #11 forbids +direct edits — but receipts and verdicts are written by machines, at volume, +and a git-tracked tree of machine-written files makes both rules unenforceable +as stated. ADR-018 already rejected a second authoritative non-markdown store. + +Two constraints frame every option: + +- **Local-first, git for sync** (Non-Goals). Anything that must survive a + clone or be trusted by another machine has to ride git. +- **One owner per state** (FORGE-O). ForgePlan should not become the canonical + store for facts another system already owns — CI results are the CI + provider's; ForgePlan references them. + +## Decision + +One rule, three storage classes. The rule: + +> **Git-tracked if a human must review it or another machine must trust it. +> Local if it is raw per-machine material. Referenced if another system owns +> it. Machine-written tracked files are append-only, schema-validated, +> digest-linked, and mutated only through CLI/MCP — RED LINE #11 extends to +> them verbatim.** + +### Class A — canonical, git-tracked, append-only + +| Object | Home | Form | +|---|---|---| +| WorkContract | `.forgeplan/contracts/` | JSON, one file per contract version, digest in the record | +| EvidenceBundle | `.forgeplan/evidence/` | the EVID artifact evolved: structured machine section + human prose, same id space | +| VerificationVerdict | `.forgeplan/verdicts/` | JSON, digest-links to bundle and contract | + +Reviewable in the PR that carries them, survive cloning, referenced by digest +so retargeting is detectable. Append-only means a new version is a new file +and supersession is a link — no merge conflicts by construction, and "edit" +is not an operation that exists. + +This amends ADR-003 rather than violating it: *versioned files under +`.forgeplan/` are the source of truth; markdown for human-authored artifacts, +schema-validated JSON for machine-issued records; LanceDB stays derived.* +ADR-003's actual load-bearing idea was never "markdown" — it was "canonical +truth is versioned plain files, indexes are disposable." + +### Class B — local raw material, gitignored + +| Object | Home | +|---|---| +| ExecutionReceipt | `.forgeplan/receipts/` | +| audit/authority event stream | the existing journal (`forgeplan-core::journal`) | + +Receipts are what a host reports about a run: commands, exit codes, streams. +High-volume, per-machine, valuable for minutes-to-days. They are the raw +material verification consumes on the machine where the run happened. What +deserves to outlive the machine gets promoted: the EvidenceBundle embeds the +receipt extract it relies on plus the receipt digest, and the bundle is +Class A. This mirrors the CI precedent — the CI provider owns the run, the +graph keeps the reference and the extract. + +The same promotion rule covers audit: routine events stay in the local +journal; trust-relevant transitions (activation, dismissal, force, gate +override) are recorded in the artifact's own tracked state history, which is +per-artifact and append-capped, so the durable trail rides git without a +global conflict-prone log file. + +### Class C — referenced, never stored + +CI results, deployment state, tracker assignments. A digest or URL plus the +observation timestamp, inside a Class A record. Copying another system's +state into the graph creates a second owner and guarantees drift. + +## Consequences + +- FPV-03/04/05 unblock: every object in the protocol has a declared home + before a schema is written. +- `.gitignore` gains `receipts/`; `contracts/` and `verdicts/` are tracked + from birth. `.forgeplan/state/` is already gitignored today, consistent + with phase state being advisory and per-machine (ADR-025). +- RED LINE #11 needs one sentence added: machine-issued records under + `contracts/`, `evidence/`, `verdicts/` are written only by the binary; + hand-editing them is the same violation as hand-editing an artifact. +- The pre-existing `journal` module becomes the audit stream's home instead + of a new subsystem. +- Verification MUST re-derive git facts (delta, SHAs) from the repository at + verdict time rather than trusting receipt contents — the receipt says what + the host claims happened; the repo says what happened. + +## Related Artifacts + +| Artifact | Relation | +|---|---| +| ADR-003 | refines | +| ADR-025 | based_on | + + + + + diff --git a/.forgeplan/evidence/EVID-168-prob-100-7-of-8-drift-findings-were-false-get-now-reports-edges.md b/.forgeplan/evidence/EVID-168-prob-100-7-of-8-drift-findings-were-false-get-now-reports-edges.md new file mode 100644 index 00000000..2cacebe2 --- /dev/null +++ b/.forgeplan/evidence/EVID-168-prob-100-7-of-8-drift-findings-were-false-get-now-reports-edges.md @@ -0,0 +1,98 @@ +--- +depth: tactical +id: EVID-168 +kind: evidence +links: +- target: PROB-100 + relation: informs +status: draft +title: 'PROB-100: 7 of 8 drift findings were false; get now reports edges' +--- + +--- +assigned_number: 168 +predicted_number: 168 +slug: evid-prob-100-7-of-8-drift-findings-were-false-get-now-reports-edges +--- + +# EVID-168: measured before and after, on real artifacts + +Both defects in PROB-100 were reproduced on this workspace's own artifacts, not +on fixtures, and re-measured after the fix. Fixtures would not have caught either +one: every layer worked in isolation. + +## body-links-drift (#446) + +`forgeplan validate SPEC-003` + +| | ids named | false | true | +|---|---|---|---| +| before | 8 | 7 | 1 | +| after | 1 | 0 | 1 | + +The seven false ones: `ADR-009`, `PRD-065`, `SPEC-004` are real edges — +`forgeplan graph` prints all three for `SPEC-003` — and `FR-1`, `FR-2`, `FR-3`, +`FR-5` are requirement numbers that can never be link targets. + +The one true finding, `EPIC-007`, survives the fix. Checked directly: the graph +has no `SPEC-003 → EPIC-007` edge, so the body table names something the artifact +is not linked to. That is what the rule is for. + +`forgeplan validate ADR-009`: 10 ids named before, 8 after. The two that dropped +(`ADR-008`, `PROB-042`) are real edges. + +## forgeplan get (#447) + +`forgeplan get SPEC-003 --json` + +- before: 14 keys, none about links +- after: `links.outbound` = `PRD-065 (refines)`, `ADR-009 (based_on)`, + `SPEC-004 (informs)` — identical to `forgeplan graph` +- after: `links.inbound` = `EVID-089 (informs)`, which + `graph | grep "SPEC-003 -->"` cannot show at all + +Empty case, fresh workspace: `{"outbound": [], "inbound": []}` and `Links: none`. +The field is present either way, so "no links" is distinguishable from "not +reported" — the ambiguity the issue was filed about. + +## Tests + +8 new. 2 on the prefix filter, 3 on the store merge, 3 end-to-end on the real +binary against a real workspace. + +The prefix-filter test was mutation-checked: with the filter removed it fails. +It detects the defect rather than confirming current behaviour — the distinction +that let #348 survive for months behind a test asserting the broken string. + +## Gate results + +| Gate | Result | +|---|---| +| `cargo fmt --all -- --check` | exit 0, 0 diffs | +| `cargo clippy --workspace --all-targets -- -D warnings` | exit 0, 0 warnings | +| `cargo test --workspace --no-fail-fast` | 3295 passed, 3 failed, 92 binaries | + +The 3 failures are #454 / PROB-090, not this change. Each passes in isolation and +fails only under parallel load; none touches the modified code. Two are in +`git/`, untouched here. The third, `c33_forgeplan_decompose_no_llm_smoke`, +asserts that **no** LLM provider is configured — it breaks when a sibling test +sets the variable. That widens #454, whose title says "flaky git tests": the +class is any test asserting on process-global state. CI does not see it because +it runs `cargo nextest`, one process per test; local `cargo test` shares one. + +Disk was at 100% with 6.2 GiB free before this run. That state previously +produced `passed=0 failed=0` at exit 0 — a gate reporting a value that is not a +result. 37 GiB was freed before measuring, so these numbers are from a run that +actually happened. + +## Structured Fields + +verdict: supports +congruence_level: 3 +evidence_type: measurement + +base_sha: ca5a7c2c +result_sha: 9dd7242 +changed_paths: crates/forgeplan-cli/src/commands/get.rs, crates/forgeplan-cli/src/commands/validate.rs, crates/forgeplan-cli/tests/cli_get_links.rs, crates/forgeplan-core/src/db/store.rs, crates/forgeplan-core/src/lifecycle/mod.rs, crates/forgeplan-core/src/validation/checks.rs, crates/forgeplan-core/src/validation/rules.rs, crates/forgeplan-mcp/src/convert.rs, crates/forgeplan-mcp/src/server.rs, crates/forgeplan-mcp/src/types.rs, .gitignore + + diff --git a/.forgeplan/evidence/EVID-169-adr-025-and-adr-026-basis-vnext-audit-measurements-plus-owner-approval.md b/.forgeplan/evidence/EVID-169-adr-025-and-adr-026-basis-vnext-audit-measurements-plus-owner-approval.md new file mode 100644 index 00000000..0ff91904 --- /dev/null +++ b/.forgeplan/evidence/EVID-169-adr-025-and-adr-026-basis-vnext-audit-measurements-plus-owner-approval.md @@ -0,0 +1,66 @@ +--- +depth: tactical +id: EVID-169 +kind: evidence +links: +- target: ADR-025 + relation: informs +- target: ADR-026 + relation: informs +status: active +title: 'ADR-025 and ADR-026 basis: vNext audit measurements plus owner approval' +--- + +--- +assigned_number: 169 +created: 2026-09-05 +predicted_number: 169 +slug: evid-adr-025-and-adr-026-basis-vnext-audit-measurements-plus-owner-approval +updated: 2026-09-05 +--- + +# EVID-169: the basis for ADR-025 and ADR-026 + +## What this pack certifies + +Both ADRs resolve blockers the vNext adversarial audit raised +(`docs/vnext/engineering-contract-layer/_audit/`), and both went through the +owner. This pack records the measured basis and the approval, so activation +does not rest on the author's own say-so. + +## Measurements behind ADR-025 + +- ADR-001 and ADR-009 both `status: active` while asserting opposite owners + for orchestration — read directly from frontmatter, audit finding B2. +- The playbook runtime spawns agents from the core binary + (`agent_dispatcher.rs:69`, `claude --print`) against the declared boundary. +- The memory kind: 2 artifacts exist in this workspace, `forgeplan new + memory` fails with "No template found for kind 'memory'", #411 shows + mem-* ids cannot resolve for linking. Verified by execution on 0.36.0. +- `.forgeplan/state/` is gitignored — phase state never leaves the machine. + +## Measurements behind ADR-026 + +- The audit blocked FPV-03/04/05 on storage (B3): zero mentions of a storage + home for the four new object classes across `docs/vnext/architecture/`. +- ADR-018 already rejected a second authoritative non-markdown store; the + journal module already exists in `forgeplan-core` for the audit stream. +- Precedent: CI results are referenced, never copied — the same rule + generalises to receipts. + +## Owner decision + +2026-09-05. Decision #1 (orchestration above ForgePlan) made by the owner +directly. Proposals #2 (storage classes) and #3 (surface dispositions) +reviewed with plain-language explanations and approved: "да все понятно и +все ок - зафиксировать". This pack is that fixation. + +## Structured Fields + +verdict: supports +congruence_level: 3 +evidence_type: audit + + + + diff --git a/.forgeplan/evidence/EVID-170-prd-086-five-trust-layer-defects-measured-before-and-after.md b/.forgeplan/evidence/EVID-170-prd-086-five-trust-layer-defects-measured-before-and-after.md new file mode 100644 index 00000000..6b030fc7 --- /dev/null +++ b/.forgeplan/evidence/EVID-170-prd-086-five-trust-layer-defects-measured-before-and-after.md @@ -0,0 +1,122 @@ +--- +depth: tactical +id: EVID-170 +kind: evidence +links: +- target: PRD-086 + relation: informs +- target: PROB-101 + relation: informs +status: draft +title: 'PRD-086: five trust-layer defects measured before and after' +--- + +--- +assigned_number: 170 +created: 2026-09-05 +predicted_number: 170 +slug: evid-prd-086-five-trust-layer-defects-measured-before-and-after +updated: 2026-09-05 +--- + +# EVID: the trust layer, measured before and after + +Five defects in PRD-086, each with a reproduction on 0.36.0 and a re-measurement +after the fix. Every fix carries a test that was mutation-checked — the fix was +reverted and the test had to fail. Two of my own tests failed that check and were +rewritten; both stories are in the test file, because a test that looks like +proof and is not is the defect this PRD exists to remove. + +## Fail-closed (FR-008 / FR-009, PROB-101) + +| Body | Score of the informed artifact — before | after | +|---|---|---| +| pure prose, no fields | **1.00 "Adequate"** | **0.10** | +| `verdict: unknown`, `congruence_level: 3` | 1.00 | 0.10 | +| `verdict: supports`, no CL source | 1.00 | 0.10 | +| `verdict: supports`, `congruence_level: 3` | 1.00 | 1.00 (unchanged) | + +Three documents already specified the "after" column: `CLAUDE.md` RED LINE #7, +the `/forge` skill installed by `setup-skill`, and `EVIDENCE-PROTOCOL.md`. The +binary did the opposite, and in the inflating direction. + +Blast radius here, counted rather than estimated: **3 of 167 packs** lack a CL +source — EVID-033, EVID-034, EVID-035 — feeding PROB-014, PROB-016 and RFC-004 +respectively. Those three drop to 0.1 and take their targets with them. That is +the correct reading: the scores were never earned. + +## Leaf evidence (#325, FR-001/FR-002) + +- canonical pack (`supports` / CL3 / measurement) scored **0.0** with the factor + `No evidence found (L0)`; now **1.00** +- `weakens` / CL3 / measurement now **0.50** — computed, not stamped +- a canonical pack attached to an unevidenced PRD scored **0.0** because its + outgoing `informs` was walked as a dependency; now **1.00** + +## The cascade (#392, narrowed) + +- PRD with one CL3 `supports` pack, `based_on` an active unevidenced NOTE: + **0.00 AT RISK** before, non-zero after +- PRD with the same evidence, `based_on` an active unevidenced **PRD**: + **0.00 before and after** — deliberately unchanged, and covered by its own + test so a future "simplification" cannot quietly soften it + +## Phase monotonicity (#330) + +`advance_phase(done → validate)` succeeded silently. It now returns an error +naming both phases, and `read_phase` confirms the state was left at `done`. +Forward jumps, no-ops, and the explicit `advance_phase_unchecked` path all still +work, each with a test. + +## Anomaly detector (#393) + +Three separate misreports, each fixed at its source: the literal `0.0` replaced +by the stored score and renamed `r_eff_cached`; the blanket +`cycle or depth cap` replaced by the reason that actually occurred; and the +ancestor walk given the scorer's skip rules so the two stop disagreeing about +which artifact is the weakest link. + +## What dogfooding caught that the tests did not + +Running `forgeplan score EVID-170` on the real graph printed: + +``` + No evidence linked. R_eff = 0.0 + • Leaf evidence scored on its own fields: Supports CL3 = 1.00 +``` + +The engine computed 1.00 and the command announced 0.0 over it. `score.rs` had +its own display branch keyed on "no linked evidence", written when that +condition could only mean one thing. Neither the unit tests nor the integration +tests read that line, so both stayed green — the same shape as the defects this +PRD closes, committed by the printer instead of the scorer. + +Fixed, and verified across all four cases: canonical pack (1.00), fieldless pack +(0.10 plus a remediation naming the missing fields), an artifact with linked +evidence (breakdown unchanged), an artifact with none (unchanged). + +## Gates + +| Gate | Result | +|---|---| +| `cargo fmt --all -- --check` | exit 0 | +| `cargo clippy --workspace --all-targets` | exit 0, 0 warnings | +| `cargo test --workspace --no-fail-fast` | **3307 passed, 3 failed**, 94 binaries | + +A first attempt at the full run produced `тестовых бинарей: 0` with +`ld: write() failed, errno=28` — the disk was at 152 MB. The harness reported +that the run had not happened instead of printing a passing summary, which is +the behaviour PROB-090's class of failure needs. Re-run after `cargo clean` gave the numbers above. + +The 3 failures are #454 / PROB-090, not this change: all three are in +`git::tests`, none of the commits here touch `crates/forgeplan-core/src/git/`, +and all 51 tests in that module pass when run single-threaded. The flake is +parallel env mutation dropping `PATH`; CI does not see it because `cargo +nextest` gives each test its own process. + +## Structured Fields + +verdict: supports +congruence_level: 3 +evidence_type: measurement + diff --git a/.forgeplan/evidence/EVID-171-prob-105-spec-validator-inversion-fixed-3320-tests-4-mutation-checks.md b/.forgeplan/evidence/EVID-171-prob-105-spec-validator-inversion-fixed-3320-tests-4-mutation-checks.md new file mode 100644 index 00000000..46866819 --- /dev/null +++ b/.forgeplan/evidence/EVID-171-prob-105-spec-validator-inversion-fixed-3320-tests-4-mutation-checks.md @@ -0,0 +1,108 @@ +--- +depth: tactical +id: EVID-171 +kind: evidence +links: +- target: PROB-105 + relation: informs +status: active +title: 'PROB-105: SPEC validator inversion fixed — 3320 tests, 4 mutation checks' +--- + +--- +assigned_number: 171 +predicted_number: 171 +slug: evid-prob-105-spec-validator-inversion-fixed-3320-tests-4-mutation-checks +--- + +# EVID-171: the SPEC validator inversion, measured before and after + +## Summary + +PROB-105 claimed the SPEC validator was inverted: it passed empty templates and +blocked real behavioural specs. Both halves reproduced on 0.36.0 with the shipped +binary, and both are closed by `daa103b` + `c8e7bd3` on `fix/spec-validator-inverted`. + +## What was measured + +| Input | Before | After | +|---|---|---| +| untouched `forgeplan new spec` template | `PASS — 0 error(s), 0 warning(s)`, activates at R_eff 1.00 | stub warning; `activate` refuses | +| behavioural spec, 2 requirements + 2 GIVEN/WHEN/THEN scenarios | `x [MUST] spec-contracts` → FAIL, cannot activate | PASS | +| SPEC-002 … SPEC-006 | pass | unchanged | +| SPEC-001 | fails `spec-summary` + `spec-contracts` | unchanged — a document *about* writing specs, filed as a spec | + +Corpus measurements that set the threshold, rather than taste: + +- placeholders: SPEC template **15**, PRD template **5**, the six real SPECs **0–3**. + `MANY_PLACEHOLDERS = 8` sits in the gap with room on both sides. +- a line-count test was measured and rejected: the untouched template has 25 + non-empty lines under `## API Contracts`, because placeholder JSON is still lines. +- subjective-adjective hits across the 69 PRDs: **2** in FR (already checked), + **15** in NFR (never checked). All 15 sit inside the template's own + `` guidance, so a rule + flagging them would be unclosable. With the non-prose strip: **0 findings across + all 69 PRDs**, while hand-written vague prose still produces findings with line + numbers. +- the blanket #450 rule was measured before being rejected: fires on 6 of 6 SPECs + here, none defective. The shipped rule is conditional. + +## Test run + +Per crate — a full `cargo test --workspace` does not fit in this machine's free +space (`ld: write() failed, errno=28` twice at ~300 MB free). + +| Command | Result | +|---|---| +| `cargo fmt --all -- --check` | exit 0 | +| `cargo clippy --workspace --all-targets -- -D warnings` | exit 0, 0 warnings | +| `cargo test -p forgeplan-core --features test-helpers --no-fail-fast` | 2239 passed, 4 failed (17 binaries) | +| `cargo test -p forgeplan --no-fail-fast` | 807 passed, 0 failed (58 binaries) | +| `cargo test -p forgeplan-mcp --no-fail-fast` | 274 passed, 0 failed (19 binaries) | + +**3320 passed, 4 failed, 94 test binaries.** The binary count matches what +`--workspace` produces, so the per-crate split skipped nothing. The 4 failures are +all `git::tests` (#454): they pass serially (51/51 under `--test-threads=1`) and +this diff does not touch that module. + +## Mutation checks + +Each fix reverted; the matching test had to fail. A test that passes on broken +code proves nothing, so each was checked rather than assumed. + +| Mutation | Test | Result | +|---|---|---| +| remove the HTML-comment strip | `nfr_adjectives_inside_html_comments_are_not_flagged` | FAILED | +| remove the placeholder scaling | untouched template back to `PASS — 0 error(s), 0 warning(s)` | reproduced | +| `MANY_PLACEHOLDERS = 100` | `the_shipped_spec_template_is_a_stub_and_declares_no_sections` | FAILED | +| append a real `## Requirements` heading to the template | same test, second assertion | FAILED | + +Four pre-existing rule-count tests failed and were updated, not silenced. They sum +named groups with comments rather than asserting a bare number, so updating one +means explaining where the new term came from. `rules_for_spec_returns_base_plus_3` +was renamed to `_plus_4` rather than edited in place — a test name carrying a number +goes stale silently. + +## Limits of this pack + +- It certifies the validator's behaviour on this repository's corpus (69 PRDs, 6 + SPECs) and on the shipped template. It says nothing about corpora with other + spec conventions. +- `check_measurability_adjectives` (the FR-side rule) still lacks the non-prose + strip and carries the same latent bug. It has never fired only because the + template's BAD examples happen to live under the NFR heading. Out of scope here, + and recorded so it is not rediscovered. +- The 4 `git::tests` failures are a known pre-existing flake (#454), not evidence + of health in that module. + +## Structured Fields + +verdict: supports +congruence_level: 3 +evidence_type: test +base_sha: 5e9b9a9f +result_sha: c8e7bd3 +changed_paths: crates/forgeplan-core/src/validation/rules.rs, crates/forgeplan-core/src/validation/checks.rs, crates/forgeplan-core/src/lifecycle/mod.rs, templates/spec/_TEMPLATE.md, CHANGELOG.md, docs/handoff/spec-validator-and-nfr-rules.md + + + diff --git a/.forgeplan/prds/PRD-086-the-trust-layer-reports-values-it-never-computed.md b/.forgeplan/prds/PRD-086-the-trust-layer-reports-values-it-never-computed.md new file mode 100644 index 00000000..9f52b5f0 --- /dev/null +++ b/.forgeplan/prds/PRD-086-the-trust-layer-reports-values-it-never-computed.md @@ -0,0 +1,144 @@ +--- +depth: standard +id: PRD-086 +kind: prd +links: +- target: ADR-002 + relation: based_on +- target: ADR-020 + relation: based_on +status: draft +title: The trust layer reports values it never computed +--- + +--- +assigned_number: 86 +predicted_number: 86 +slug: prd-the-trust-layer-reports-values-it-never-computed +--- + +# PRD-086: The trust layer reports values it never computed + +## Problem + +R_eff is the number the whole product is for. Four defects make it, and the +detector that reads it, report things that were never computed. + +None of them fail. Each returns a plausible value, which is why all four +survived: a zero looks like an honest zero, and a warning about a missing link +looks like a warning about a missing link. + +**An EvidencePack is asked for its evidence.** `r_eff_recursive` collects the +packs linked to an artifact and takes the min. Run against an EvidencePack it +finds nothing — a pack has no packs — and returns `self_score = 0.0` with the +factor `No evidence found (L0)`. A canonical pack (`verdict: supports`, +`congruence_level: 3`) scores zero. + +The intrinsic score already exists. `score_evidence` computes it from verdict, +congruence level and expiry, and is applied to that same pack whenever it is +scored *as evidence for something else*. Only the pack itself never gets it. + +Worse, the pack's outgoing `informs` edge to the artifact it supports is +classified as a **dependency**, so trust flows from the decision down into the +evidence. Backwards. + +**The cascade contradicts the routing table.** A Note is the artifact for +trivial, reversible work: no ADI, no evidence, expires in 90 days. The +dependency walk then treats an active Note with no evidence as zero trust and +poisons every chain based on it. Forgeplan says a Note needs no evidence and +then scores everything downstream of it as unevidenced. + +ADR-002 already resolved this shape for `draft` dependencies — skip, with a +logged factor — and rejected "count draft as 0" for penalising planning ahead. +The same reasoning applies to kinds that are ceremony-free by design. + +**The anomaly detector reports three things it did not check.** It prints +`R_eff=0` for artifacts whose stored `r_eff` is nonzero but below threshold; it +labels every give-up `cycle or depth cap` in graphs with zero cycles; and its +ancestor walk gives up where `forgeplan_score` resolves the weakest link fine, +emitting `weakest_link: null, chain_depth: 0` for artifacts the scorer answers. + +**`advance_phase` regresses.** It has no monotonicity guard. MCP +`forgeplan_validate` calls it with `Phase::Validate` on PASS, so validating an +already-shipped artifact walks its phase back from `done`, and +`forgeplan_health` then reports a phase mismatch that the artifact did not have +until it was validated. + +## Goals + +- An EvidencePack scores on its own merits, using the formula that already + exists, without inventing child evidence to satisfy the walk. +- The weakest-link cascade is preserved exactly as it is for kinds that can owe + evidence, and skips the kinds the methodology exempts. +- Every number and label the anomaly detector prints is one it computed. +- A phase never moves backwards on its own. + +## Non-Goals + +- **Weakening the weakest-link formula.** `R_eff = min(...)` stays. The three + fixes proposed in #392 — local-only scoring, one-hop propagation, a floor at + `self_score` — are each an average in disguise and are declined. If a + foundation is unproven, the floor above it is not trustworthy, and a number + saying otherwise is the failure the formula prevents. +- Changing CL penalties, decay, or the verdict scale. +- Raising the detector's depth cap. The walk is unified with the scorer's; if a + cap is still hit afterwards it must be reported as a depth cap, not guessed + at. +- Backfilling or migrating stored scores. `forgeplan score --all` recomputes. + +## Target Users + +Agents reading `r_eff` to decide whether an artifact can be relied on, and +maintainers triaging `forgeplan anomalies`. Both currently receive confident +numbers that were never computed, and neither has a way to tell which. + +## Functional Requirements + +- **FR-001**: An EvidencePack with no linked child evidence derives + `self_score` from `score_evidence` over its own parsed fields, not from the + empty-evidence path. A pack whose fields do not parse keeps `0.0` — absent + structured fields remain CL0, per the existing contract. +- **FR-002**: An EvidencePack's outgoing `informs` / `based_on` edges to the + artifacts it supports are excluded from its own dependency walk. Evidence + does not depend on what it evidences. +- **FR-003**: The dependency walk skips `note` and `memory` dependencies, + recording a factor naming the id and kind, in the same shape ADR-002 uses for + non-active dependencies. +- **FR-004**: The anomaly detector reports the artifact's actual stored + `r_eff`, not `0`. +- **FR-005**: The detector distinguishes a depth cap from a cycle and names + which occurred. +- **FR-006**: The detector's ancestor walk resolves a weakest link wherever + `forgeplan_score` resolves one, by sharing the traversal rather than + reimplementing it. +- **FR-008**: An unparseable or unknown `verdict` fails closed. Today it falls + through to `Supports` (score 1.0) while `congruence_level` in the same + function fails closed to CL0 with a warning — one field punishes garbage and + its neighbour rewards it. Protocol v1 introduces a fourth value (`unknown`), + which the current parser would score 1.0. +- **FR-009**: Absent structured fields fail closed. A pack carrying no + `verdict` / `congruence_level` at all currently scores its target 1.00; + `CLAUDE.md` RED LINE #7, the `/forge` skill shipped by `setup-skill`, and + `EVIDENCE-PROTOCOL.md` all state the opposite (CL0, 0.1). The absence is + recorded as a factor rather than applied silently. Breaking: packs without + fields drop from 1.0 to 0.1, and artifacts whose weakest link was such a + pack drop with them — 3 of 166 here, unknown elsewhere. Requires a + migration note and `forgeplan score --all`. + +- **FR-007**: `advance_phase` refuses a transition to an earlier phase, leaving + state unchanged and reporting the refusal to its caller. Explicit + `forgeplan phase-advance --to ` remains available for deliberate + correction. + +## Related Artifacts + +| Artifact | Relation | +|---|---| +| ADR-002 | based_on | +| ADR-020 | based_on | + +GitHub: #325, #392, #393, #330. + + + + diff --git a/.forgeplan/problems/PROB-059-body-links-drift-detector-strict-parser-warning-on-related-artifacts-table-missing-frontmatter-links.md b/.forgeplan/problems/PROB-059-body-links-drift-detector-strict-parser-warning-on-related-artifacts-table-missing-frontmatter-links.md index c0a1cb4a..b820e71c 100644 --- a/.forgeplan/problems/PROB-059-body-links-drift-detector-strict-parser-warning-on-related-artifacts-table-missing-frontmatter-links.md +++ b/.forgeplan/problems/PROB-059-body-links-drift-detector-strict-parser-warning-on-related-artifacts-table-missing-frontmatter-links.md @@ -75,3 +75,4 @@ Reverse drift: `forgeplan link` populates `links:` → agent rewrites body via W + diff --git a/.forgeplan/problems/PROB-100-body-links-drift-and-forgeplan-get-both-read-a-record-that-carries-no-links.md b/.forgeplan/problems/PROB-100-body-links-drift-and-forgeplan-get-both-read-a-record-that-carries-no-links.md new file mode 100644 index 00000000..64e2eed0 --- /dev/null +++ b/.forgeplan/problems/PROB-100-body-links-drift-and-forgeplan-get-both-read-a-record-that-carries-no-links.md @@ -0,0 +1,107 @@ +--- +depth: tactical +id: PROB-100 +kind: problem +links: +- target: PROB-059 + relation: refines +status: draft +title: body-links-drift and forgeplan_get both read a record that carries no links +--- + +--- +assigned_number: 100 +predicted_number: 100 +slug: prob-body-links-drift-and-forgeplan-get-both-read-a-record-that-carries-no-links +--- + +# PROB-100: two tools, one blind spot + +## Problem + +`ArtifactRecord` has no link fields. Everything that reads a record therefore +sees an artifact with five edges exactly as it sees an orphan, and two shipped +tools were built on top of that blindness without anyone noticing. + +**The validator warned about links that exist.** `frontmatter_map()` rebuilds a +frontmatter from the record's columns. The record has no link columns, so the map +never carries `links`, so `extract_frontmatter_link_targets` returned an empty +set for every artifact. `body-links-drift` compared the body's +`## Related Artifacts` table against that empty set and warned on everything it +found there — including targets that were correctly linked and visible in +`forgeplan graph` the same minute. + +**`forgeplan get` reported an orphan for a linked artifact.** Both edge sets were +fetched, reduced to a boolean to choose a hint, and discarded one line before the +output. The response carried about twenty fields and no links at all. + +## Why it survived + +Neither tool failed. The warning was plausible, the response looked complete, and +both were confidently wrong in a way that reads as correct. + +The warning also could not be closed. Linking the named target did not silence +it, because the comparison never saw links; the only way to make it stop was to +delete the table it was complaining about. A SHOULD-level warning that fires on +essentially every well-linked artifact and cannot be resolved by doing the right +thing teaches its readers to skip validator warnings — which is more expensive +than the drift it was built to catch. + +The regex made it worse. `[A-Z]+-[0-9]+` matched any token of that shape, so +requirement numbers (`FR-1`) and invariant numbers (`I-3`) were named as missing +link targets. Those can never be linked, and the remediation the rule printed — +`forgeplan link FR-1` — could not be executed. That is a PRD-071 +violation of the same shape as #348 and #351: a hint an agent is obliged to run +that cannot work. + +## Measurement + +Workspace: this repository, `forgeplan 0.35.0` built from `ca5a7c2`. + +`forgeplan validate SPEC-003`, before: + +``` +mentions ADR-009, EPIC-007, FR-1, FR-2, FR-3, FR-5, PRD-065, SPEC-004 +``` + +Eight ids, seven false. `ADR-009`, `PRD-065` and `SPEC-004` are real edges — +`forgeplan graph` shows all three. `FR-1` through `FR-5` are requirement numbers. + +After: one id, `EPIC-007`, and it is true — the graph has no `SPEC-003 → EPIC-007` +edge. + +`forgeplan get SPEC-003 --json`, before: 14 keys, none about links. After: three +outbound edges matching `graph` exactly, plus one inbound (`EVID-089 informs`) +that `graph | grep "SPEC-003 -->"` cannot show at all. + +A third disagreement surfaced while measuring, not reported in either issue: +`SPEC-003` has **three** different frontmatters — the file on disk (with links), +the copy embedded in the stored `body` (stale, no links, still carrying a +`created:` field the file dropped), and the reconstruction from record columns +(no links, ever). + +## Relation to PROB-059 + +PROB-059 introduced this rule and is still `active`. The drift it describes is +real and remains worth detecting. This records that its detector never worked: +the rule shipped in a state where its condition could not be satisfied. + +## Fix + +Links are read from the relations table — the same source `forgeplan graph` +renders and `forgeplan link` writes — through +`LanceStore::frontmatter_map_with_links`, used at all three sites that run the +full rule set. The extractor keeps only prefixes that map to a real artifact +kind. `get` emits both edge sets unconditionally, as empty arrays when there are +none, so a caller can tell "no links" from "not reported". + +## Related + +| Artifact | Type | Relation | +|---|---|---| +| PROB-059 | Problem | the rule this one reports as non-functional | + +GitHub: #446, #447. + + + diff --git a/.forgeplan/problems/PROB-101-missing-evidence-fields-grant-maximum-trust-and-three-documents-promise-the-opposite.md b/.forgeplan/problems/PROB-101-missing-evidence-fields-grant-maximum-trust-and-three-documents-promise-the-opposite.md new file mode 100644 index 00000000..7e41d52a --- /dev/null +++ b/.forgeplan/problems/PROB-101-missing-evidence-fields-grant-maximum-trust-and-three-documents-promise-the-opposite.md @@ -0,0 +1,114 @@ +--- +depth: tactical +id: PROB-101 +kind: problem +links: +- target: PRD-086 + relation: informs +status: draft +title: Missing evidence fields grant maximum trust, and three documents promise the opposite +--- + +--- +assigned_number: 101 +predicted_number: 101 +slug: prob-missing-evidence-fields-grant-maximum-trust-and-three-documents-promise +--- + +# PROB-101: the evidence gate fails open, and everything says it fails closed + +(touched to force a re-embed) + +## Signal + +An EvidencePack containing no structured fields at all — pure prose, no +`verdict`, no `congruence_level`, no `evidence_type` — gives the artifact it +informs a perfect score. + +Measured on 0.36.0, fresh workspace: + +``` +Evidence breakdown: + EVID-001 [Supports] CL3 = 1.0 +R_eff: 1.00 -- Adequate +``` + +Three documents promise the opposite, in the same words: + +- `CLAUDE.md` RED LINE #7 — "без этих structured fields parser тихо ставит CL0 + (silent failure → R_eff = 0.1)" +- the `/forge` skill, shipped to every user by `setup-skill` — "Without them, + the R_eff parser silently defaults to CL0 (penalty 0.9), making R_eff = 0.1" +- `docs/methodology/EVIDENCE-PROTOCOL.md`, same claim + +The code does the reverse, deliberately: +`crates/forgeplan-core/src/scoring/evidence.rs` — `(None, None) => 3`, with the +comment *"Default CL=3 (same context) — evidence created locally is +same-context by default."* + +## Why this is the dangerous direction + +Absent metadata resolves to **maximum** trust. Every other unknown in this +codebase fails closed: unparseable `congruence_level` → CL0 with a warning; an +unclosed HTML comment that swallows the fields → CL0 with a warning. Both were +fixed as trust-inflation bugs (PROB-034 and its follow-ups). The plain-absence +case was left as the one path where saying nothing is rewarded. + +It also inverts the incentive. An agent that writes the fields honestly: + +``` +verdict: supports +congruence_level: 2 → score 0.9 +``` + +An agent that writes nothing: + +``` +(no fields) → score 1.0 +``` + +Skipping the discipline scores **better** than following it. The red line that +exists to prevent silent failure is enforced by nothing, and the documentation +that would warn an author is wrong in the direction that hides the problem. + +## Scale + +This repository: **3 of 166** packs lack `congruence_level`. The discipline is +followed here, which is exactly why the missing guard went unnoticed — the +convention holds, so nothing ever tested what happens when it does not. + +That number says nothing about other workspaces. A workspace where an agent +skipped the fields has inflated scores and no signal that it did. + +## Not the same as #325 + +#325 is a pack scoring 0 for itself. This is a pack scoring 1.0 for someone +else on no information. Opposite direction, same root: the scorer has no +concept of "this pack did not tell me anything." + +## The decision this needs + +Changing `(None, None)` from CL3 to CL0 aligns behaviour with all three +documents and makes RED LINE #7 real. It is a **breaking scoring change**: +every pack without the fields drops from 1.0 to 0.1, and every artifact whose +weakest link was such a pack drops with it. Three artifacts here; unknown +elsewhere. + +The alternative — keep CL3 and correct the three documents — is cheaper and +defensible on the code's own rationale (locally-authored evidence really is +same-context). But it means the product's headline number treats "I measured +this and it strongly supports the claim" and "I wrote some prose" as identical, +and no gate anywhere distinguishes them. + +Recommendation: fail closed, with the absence reported as a factor rather than +a silent default, and a `forgeplan score --all` note in the release. The whole +point of R_eff is that a number can be traced to something. A default of +maximum trust on absent input is the one case where it cannot. + +## Related + +| Artifact | Relation | +|---|---| +| PRD-086 | informs | + + diff --git a/.forgeplan/problems/PROB-102-ci-compiles-the-semantic-search-surface-and-never-runs-it.md b/.forgeplan/problems/PROB-102-ci-compiles-the-semantic-search-surface-and-never-runs-it.md new file mode 100644 index 00000000..d05a2386 --- /dev/null +++ b/.forgeplan/problems/PROB-102-ci-compiles-the-semantic-search-surface-and-never-runs-it.md @@ -0,0 +1,94 @@ +--- +depth: tactical +id: PROB-102 +kind: problem +links: +- target: PRD-086 + relation: informs +status: draft +title: CI compiles the semantic-search surface and never runs it +--- + +--- +assigned_number: 102 +predicted_number: 102 +slug: prob-ci-compiles-the-semantic-search-surface-and-never-runs-it +--- + +# PROB-102: the embedding oracle has never run + +## Signal + +`crates/forgeplan-core/tests/embedding_reference.rs` describes itself as the +correctness oracle for the embedding engine — *"catches the one failure mode an +engine swap has that nothing else would."* It holds three tests pinning vectors +against values captured from the pre-tract engine. + +It executes zero of them in CI: + +``` +$ cargo test -p forgeplan-core --test embedding_reference +running 0 tests +test result: ok. 0 passed; 0 failed; 0 ignored +``` + +The file is gated on `feature = "semantic-search"`. CI's test step is: + +```yaml +- name: cargo nextest run + run: cargo nextest run --workspace --all-targets +``` + +No `--features`. The two steps above it *do* pass the feature: + +```yaml +- run: cargo check --workspace --all-targets --features semantic-search +- run: cargo clippy --workspace --all-targets --features semantic-search +``` + +So the semantic-search surface is compiled and linted on every PR, and executed +on none. `0 passed` is reported as success. + +## Why this matters more than the count + +v0.35.0 replaced the entire embedding engine — ONNX Runtime to tract. This +oracle is the safety net that swap was performed over. It was written for +exactly that release, and it did not run during it. The vector-equivalence +claim in the v0.35.0 notes (max deviation 7.0e-07 across six reference cases) +came from a manual local run, not from the gate. + +The same hole explains PROB-103 (`embed` loading the model when there is +nothing to embed, 8.22s on a current workspace): no CI job ever executes +`embed`, so its cost was invisible until a user noticed the message. + +This is the release's own theme applied to the harness: a check that exists, +is documented, is well-intentioned, and never runs — while reporting a green +result. + +## Scope + +Only `embedding_reference.rs` is a fully-gated test file. Inline `#[cfg]` +blocks elsewhere hide additional cases, but a reliable count needs a proper +audit rather than a grep — an earlier attempt here produced inflated numbers by +matching from the first `cfg` line to end of file, and was discarded. + +## Fix direction + +Add a nextest invocation with the feature enabled. It cannot simply replace the +current one: the default-feature run is what proves the shipped-without-feature +build still works, and that path has its own refusal branches worth executing. +Two runs, or one matrix, both configurations. + +Cost is real and should be stated rather than discovered: the feature pulls the +model at runtime, so the job needs the model cache or the tests need to be +marked as requiring it. That is the reason it was left out, and it is a +solvable reason, not a justification for reporting `0 passed` as green. + +## Related + +| Artifact | Relation | +|---|---| +| PRD-086 | informs | + + + diff --git a/.forgeplan/problems/PROB-103-embed-loads-a-multi-gigabyte-model-to-discover-it-has-nothing-to-do.md b/.forgeplan/problems/PROB-103-embed-loads-a-multi-gigabyte-model-to-discover-it-has-nothing-to-do.md new file mode 100644 index 00000000..48e94db2 --- /dev/null +++ b/.forgeplan/problems/PROB-103-embed-loads-a-multi-gigabyte-model-to-discover-it-has-nothing-to-do.md @@ -0,0 +1,96 @@ +--- +depth: tactical +id: PROB-103 +kind: problem +links: +- target: PROB-102 + relation: based_on +- target: PRD-086 + relation: informs +status: draft +title: embed loads a multi-gigabyte model to discover it has nothing to do +--- + +--- +assigned_number: 103 +predicted_number: 103 +slug: prob-embed-loads-a-multi-gigabyte-model-to-discover-it-has-nothing-to-do +--- + +# PROB-103: embed pays the model's price to learn there is no work + +## Signal + +Reported by a user watching the output, not by any test: + +``` +$ forgeplan embed + Loading embedding model... +Done: 2 embedded, 425 already current, 0 failed. + +$ forgeplan embed + Loading embedding model... +``` + +The model loads on every invocation, including runs where nothing needs +encoding. + +Measured on 0.36.0, this workspace, 427 artifacts, all current: + +| | | +|---|---| +| before | **8.22 s** to report `0 embedded, 427 already current` | +| after | **0.25 s** (warm, three consecutive runs: 0.83 / 0.25 / 0.25) | + +The model was read off disk and never used. + +## Root cause + +Half of PROB-093. That fix made `embed` incremental — a record is skipped when +it already has a vector and its content hash still matches — which removed the +13m18s of redundant encoding. But the skip decision was made *inside* the loop, +after `Embedder::new()` had already run. The expensive part stayed +unconditional. + +So the fix removed the work and left the setup for the work. + +## Why nobody noticed + +No CI job executes `embed` — see PROB-102, where the whole semantic-search +surface is compiled and linted but never run. The cost was invisible to every +gate and visible only to a person watching a terminal. + +The output made it worse: `Loading embedding model...` prints before the +outcome, so the run *looks* like it is doing something. Only the final line +says otherwise, and by then the 8 seconds are spent. + +## Fix + +Compute the work list first; load the model only if it is non-empty. On an +empty list, print the same `Done:` summary and return. + +Also corrects the progress line, which claimed the wrong total: + +``` +- Embedding 427 artifact(s) ... ← every record, including the skipped ones ++ Embedding 1 of 427 artifact(s) ... +``` + +`Loading embedding model...` now appears only when a model is actually about to +be loaded, which is what a reader assumed it meant. + +## Verified + +- no-op run: 0.25 s warm, and the loading line is absent +- real work: model loads, one changed artifact re-encoded, `1 embedded, + 426 already current, 0 failed` + +## Related + +| Artifact | Relation | +|---|---| +| PROB-102 | based_on | +| PRD-086 | informs | + + + diff --git a/.forgeplan/problems/PROB-104-a-leaf-pack-with-an-evidence-neighbour-reports-the-neighbour-s-score.md b/.forgeplan/problems/PROB-104-a-leaf-pack-with-an-evidence-neighbour-reports-the-neighbour-s-score.md new file mode 100644 index 00000000..687832ab --- /dev/null +++ b/.forgeplan/problems/PROB-104-a-leaf-pack-with-an-evidence-neighbour-reports-the-neighbour-s-score.md @@ -0,0 +1,106 @@ +--- +depth: tactical +id: PROB-104 +kind: problem +links: +- target: PRD-086 + relation: informs +status: draft +title: A leaf pack with an evidence neighbour reports the neighbour's score +--- + +--- +assigned_number: 104 +context: '{grouping tag}' +created: 2026-09-06 +predicted_number: 104 +slug: prob-a-leaf-pack-with-an-evidence-neighbour-reports-the-neighbour-s-score +--- + +# PROB-104: a leaf pack with an evidence neighbour reports the neighbour's score + +## Signal + +PRD-086 FR-001 gave an EvidencePack an intrinsic score computed from its own +structured fields. The guard is: + +```rust +if is_evidence_kind && evidence_items.is_empty() && terminal_skips == 0 { +``` + +`evidence_items` is built from `linked_evidence_ids` — every outgoing target id +plus every incoming source id, filtered to `kind == "evidence"`, with **no +relation-type filter**. So the moment a pack has any evidence-kind neighbour in +either direction, the early return is skipped, its own `verdict` and +`congruence_level` are never parsed, and `self_score` comes from the +neighbour's fields instead. + +Measured on 0.36.0 with the prebuilt binary in a scratch workspace: + +| Pack under test | isolated | after one link to a supports/CL3 pack | +|---|---|---| +| body is pure prose, no fields | 0.10 | **1.00** | +| `verdict: refutes`, `congruence_level: 3` | 0.00 | **1.00** | + +`forgeplan score` on the second row prints +`Evidence breakdown: EVID-001 [Supports] CL3 = 1.0` for a pack whose own body +says `refutes`. + +## Why this is low and not critical + +An adversarial reviewer raised it as a critical trust-laundering vector. A +second agent tasked with refuting it measured the claims and downgraded it, +correctly: + +- **Fail-closed parsing is intact.** FR-008/FR-009 govern how a pack scores its + *target*, and that call site runs `parse_evidence_from_record` from the + consumer's side. A PRD informed by the prose pack reports 0.10 before and + after the laundering link; informed by the refuting pack, 0.00 both times. +- **Nothing propagates.** Only the pack's own self-report changes. +- **It gates nothing.** `forgeplan activate` on an EvidencePack is not R_eff-gated, + and succeeds at 0.0 and 1.0 alike. +- **Zero impact on this graph.** All six EVID→EVID edges here + (EVID-087→086, 089→088, 090→088, 091→089, 097→096, 099→098) are supports/CL3 + packs pointing at supports/CL3 packs, so the substituted value equals the true + one everywhere it currently fires. + +## Mostly pre-existing, partly ours + +The evidence-collection code is untouched by PRD-086, and "self_score is the min +over linked evidence, never the artifact's own fields" is the original design for +every kind. Pre-diff, a prose pack scored 0.0 isolated and 1.0 once linked — so +"one link makes it report 1.00" predates this work; PRD-086 only introduced the +0.10 baseline that makes the jump visible. + +The genuinely new residue is narrow: for a pack with a **negative** verdict plus +an evidence neighbour, FR-002 removed the dependency edge whose recursion used +to pull the result back down. That accidental correctness *was* the backwards +trust flow FR-002 deliberately removed, so the loss is a side effect of an +intended change, not a regression to undo. + +## Fix direction + +The guard should ask whether the pack has *supporting children*, not whether it +has *any evidence-kind neighbour*. Two candidate shapes: + +1. Filter `linked_evidence_ids` by relation direction when the artifact is an + evidence kind — only incoming `informs` / `based_on` / `supports` count as + children; outgoing edges point at what the pack supports. +2. Fall back to the intrinsic score whenever the pack's own fields parse and the + collected set contains no *incoming* evidence, rather than gating on + emptiness. + +Either touches evidence collection, which is shared by every artifact kind, so +it needs its own change with its own tests — the reason it was not folded into +PRD-086. + +Regression test to write first: a `refutes`/CL3 pack linked to a `supports`/CL3 +pack must not report 1.00. + +## Related + +| Artifact | Relation | +|---|---| +| PRD-086 | informs | + + diff --git a/.forgeplan/problems/PROB-105-the-spec-validator-passes-empty-templates-and-blocks-real-behavioural-specs.md b/.forgeplan/problems/PROB-105-the-spec-validator-passes-empty-templates-and-blocks-real-behavioural-specs.md new file mode 100644 index 00000000..ed3ad13f --- /dev/null +++ b/.forgeplan/problems/PROB-105-the-spec-validator-passes-empty-templates-and-blocks-real-behavioural-specs.md @@ -0,0 +1,116 @@ +--- +depth: tactical +id: PROB-105 +kind: problem +links: +- target: PRD-086 + relation: informs +status: deprecated +title: The SPEC validator passes empty templates and blocks real behavioural specs +--- + + +--- +assigned_number: 105 +context: '{grouping tag}' +created: 2026-09-07 +predicted_number: 105 +slug: prob-the-spec-validator-passes-empty-templates-and-blocks-real-behavioural-specs +--- + +# PROB-105: the SPEC validator is inverted + +## Signal + +Measured on 0.36.0 with the shipped binary, in a fresh workspace. + +**An untouched SPEC template validates clean and activates with perfect trust.** + +``` +$ forgeplan new spec "Widget sync API" # 110-line template, nothing edited +$ forgeplan validate SPEC-001 + Result: PASS -- 0 error(s), 0 warning(s) +Next: forgeplan activate SPEC-001 +``` + +Link any evidence pack and it activates: `Status: active`, `R_eff: 1.00`. The +body is still `{METHOD} /v1/{resource}` and "Что специфицируется. Одно +предложение." + +**A complete behavioural spec fails a MUST and cannot activate.** + +``` +$ forgeplan validate SPEC-003 # 2 × ### Requirement, 2 × #### Scenario, GIVEN/WHEN/THEN + x [MUST] spec-contracts: Missing '## API Contracts' or '## Data Models' section + Result: FAIL -- 1 error(s) +``` + +So the validator waves through a document that says nothing and blocks one that +carries a full test oracle. + +## Why each half happens + +**The block.** `check_spec_contracts` looks for the literal headings +`API Contracts` or `Data Models` (rules.rs, `section_exists`). `## Requirements` +and `## Behavioral Contract` are not in `expand_aliases`, so a behavioural spec +has no route through a MUST rule. The kernel already encodes one methodology's +shape as mandatory — which is the opposite of what issue #450 alleges. + +**The pass.** `check_stub` is the gate that exists to catch unfilled templates. +Its twelve `PHRASE_MARKERS` are all PRD prose — "Что мы строим и почему это +важно", "What we are building and why", `[Actor] can [capability]`. None appears +in the SPEC template. It also counts placeholders, but caps that signal at +1 +while the threshold is 3, so fifteen unfilled placeholders count the same as one. + +The stub gate is structurally incapable of seeing a SPEC. + +## The measurement that gives a shape-agnostic fix + +Single-brace placeholders, counted across the corpus: + +| Document | Placeholders | +|---|---| +| SPEC template, untouched | **15** | +| SPEC-001 … SPEC-006 (real) | 0–3 | +| PRD template (which the gate *does* catch) | 5 | + +Clean separation with a wide margin, and it asks nothing about methodology: not +"is this Gherkin or API", but "is this still a form to fill in". + +A line-count test would NOT work — the untouched template has 25 non-empty lines +under `## API Contracts`, because placeholder JSON is still lines. + +## What this explains + +The marketplace TDD flow grew its own gate (`tdd-planner` HARD RULE 1 refusing to +plan against a spec with no `#### Scenario`) because core actively rejects the +shape it needs. Issue #450 read that as "core has no rule"; the truth is core has +a MUST pointing the other way. + +## Fix + +Three changes, none of which teaches the kernel a methodology: + +1. **`check_stub` learns to see a SPEC** — scale the placeholder signal instead of + capping it at +1. Catches the template, silent on all six real specs. +2. **`spec-contracts` accepts a behavioural route** — `Requirements`, + `Contract`, `Behavioral Contract` alongside the API-shaped headings. The rule + still demands *a* contract; it stops demanding one particular form of it. +3. **Conditional consistency rule (#450, literally)** — if a spec opens + `### Requirement`, each must carry a `#### Scenario`. Vacuous on all six of + ours (none uses those headings); fires on a half-authored behavioural spec, + which is the real defect. Should, not Must. + +## Related + +| Artifact | Relation | +|---|---| +| PRD-086 | informs | + +GitHub: #450 (its premise is inverted — see above), #449 (separate). + +## Deprecation + +Reason: Fixed and merged: 551ddb9 (PR #472). check_stub now scales the placeholder signal, spec-contracts accepts a behavioural contract, and spec-requirement-has-scenario covers the half-authored case. Verified by EVID-171 (before/after measurements, 3320 tests, 4 mutation checks). Kept as history rather than deleted. +Date: 2026-09-08 + diff --git a/.github/workflows/assign-id.yml b/.github/workflows/assign-id.yml index 7d4464bf..8283076a 100644 --- a/.github/workflows/assign-id.yml +++ b/.github/workflows/assign-id.yml @@ -51,7 +51,7 @@ jobs: toolchain: stable components: '' - - uses: Swatinem/rust-cache@c19371144df3bb44fab255c43d04cbc2ab54d1c4 # v2.9.1 + - uses: Swatinem/rust-cache@6323deb102c322ba6fcbdcafc7e3dddab59af2b6 # v2.9.2 with: workspaces: crates/forgeplan-cli cache-targets: release diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 75e40152..ba03aa61 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -96,7 +96,7 @@ jobs: uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - name: Setup Node - uses: actions/setup-node@2028fbc5c25fe9cf00d9f06a71cc4710d4507903 # v6.0.0 + uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7.0.0 with: node-version: 22 cache: npm @@ -120,7 +120,7 @@ jobs: # in website/docs/CLAUDE.md/README/TODO, so a docs-only PR never ran it. steps: - name: Checkout - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - name: Self-test the detector's own regex # #421: the extraction pattern had never been asserted, and a @@ -190,7 +190,7 @@ jobs: repo-token: ${{ secrets.GITHUB_TOKEN }} - name: Install sccache - uses: mozilla-actions/sccache-action@9e7fa8a12102821edf02ca5dbea1acd0f89a2696 # v0.0.10 + uses: mozilla-actions/sccache-action@fc920bf0ec8de6ee65d409111f7ec508035751ba # v0.0.11 - name: Probe sccache backend # Probe whether the GHA cache backend is healthy. If reachable, @@ -223,7 +223,7 @@ jobs: fi - name: Rust cache - uses: Swatinem/rust-cache@c19371144df3bb44fab255c43d04cbc2ab54d1c4 # v2.9.1 + uses: Swatinem/rust-cache@6323deb102c322ba6fcbdcafc7e3dddab59af2b6 # v2.9.2 - name: cargo fmt run: cargo fmt --all -- --check @@ -272,7 +272,7 @@ jobs: repo-token: ${{ secrets.GITHUB_TOKEN }} - name: Install sccache - uses: mozilla-actions/sccache-action@9e7fa8a12102821edf02ca5dbea1acd0f89a2696 # v0.0.10 + uses: mozilla-actions/sccache-action@fc920bf0ec8de6ee65d409111f7ec508035751ba # v0.0.11 - name: Probe sccache backend # Same probe pattern as in the check job — see comment above. @@ -298,10 +298,10 @@ jobs: fi - name: Rust cache - uses: Swatinem/rust-cache@c19371144df3bb44fab255c43d04cbc2ab54d1c4 # v2.9.1 + uses: Swatinem/rust-cache@6323deb102c322ba6fcbdcafc7e3dddab59af2b6 # v2.9.2 - name: Install cargo-nextest - uses: taiki-e/install-action@3d7d7cd5ac7f994c1892ae0c06165095b9139094 # v2.85.1 + uses: taiki-e/install-action@5bf6ce016fd2e72eefc647cbca1e4213f65955b8 # v2.87.5 with: tool: cargo-nextest @@ -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 @@ -336,7 +459,7 @@ jobs: repo-token: ${{ secrets.GITHUB_TOKEN }} - name: Install sccache - uses: mozilla-actions/sccache-action@9e7fa8a12102821edf02ca5dbea1acd0f89a2696 # v0.0.10 + uses: mozilla-actions/sccache-action@fc920bf0ec8de6ee65d409111f7ec508035751ba # v0.0.11 - name: Probe sccache backend run: | @@ -361,7 +484,7 @@ jobs: fi - name: Rust cache - uses: Swatinem/rust-cache@c19371144df3bb44fab255c43d04cbc2ab54d1c4 # v2.9.1 + uses: Swatinem/rust-cache@6323deb102c322ba6fcbdcafc7e3dddab59af2b6 # v2.9.2 - name: Build forgeplan binary run: cargo build --bin forgeplan diff --git a/.github/workflows/forgeplan-health.yml b/.github/workflows/forgeplan-health.yml index 4e5a471a..9a6ed54a 100644 --- a/.github/workflows/forgeplan-health.yml +++ b/.github/workflows/forgeplan-health.yml @@ -21,7 +21,7 @@ jobs: uses: dtolnay/rust-toolchain@29eef336d9b2848a0b548edc03f92a220660cdb8 # stable branch @ 2026-03-27 (no tag; verify with git ls-remote) - name: Rust compilation cache - uses: Swatinem/rust-cache@c19371144df3bb44fab255c43d04cbc2ab54d1c4 # v2.9.1 + uses: Swatinem/rust-cache@6323deb102c322ba6fcbdcafc7e3dddab59af2b6 # v2.9.2 - name: Build forgeplan run: cargo build -p forgeplan diff --git a/.github/workflows/perf.yml b/.github/workflows/perf.yml index 65522271..27a0a597 100644 --- a/.github/workflows/perf.yml +++ b/.github/workflows/perf.yml @@ -51,7 +51,7 @@ jobs: repo-token: ${{ secrets.GITHUB_TOKEN }} - name: Cache cargo registry + target - uses: Swatinem/rust-cache@c19371144df3bb44fab255c43d04cbc2ab54d1c4 # v2.9.1 + uses: Swatinem/rust-cache@6323deb102c322ba6fcbdcafc7e3dddab59af2b6 # v2.9.2 with: shared-key: perf-bench save-if: ${{ github.ref == 'refs/heads/main' || github.ref == 'refs/heads/dev' }} diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 7e36ee83..75af4cbc 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -144,7 +144,7 @@ jobs: with: repo-token: ${{ secrets.GITHUB_TOKEN }} - name: Rust compilation cache - uses: Swatinem/rust-cache@c19371144df3bb44fab255c43d04cbc2ab54d1c4 # v2.9.1 + uses: Swatinem/rust-cache@6323deb102c322ba6fcbdcafc7e3dddab59af2b6 # v2.9.2 with: shared-key: "release-${{ join(matrix.targets, '_') }}" - name: Build artifacts diff --git a/.gitignore b/.gitignore index 32e22bbe..b349d7a1 100644 --- a/.gitignore +++ b/.gitignore @@ -145,7 +145,10 @@ docs/vnext/engineering-contract-layer/.created-issues.json # Local agent/tooling state and scratch material — never belongs in git. # Added after `git add -A` swept `.codex/config.toml` (which held a live # HINDSIGHT_API_KEY) into this public repo. Removing the file does not undo -# exposure — the key was rotated. This stops the same sweep repeating. +# exposure: the object stays fetchable by SHA from the closed PR that carried +# it, so rotation is the only thing that actually closes it. Do not read this +# block as a statement that rotation happened — it stops the sweep repeating, +# nothing more. .codex/ design/ ref/ diff --git a/AGENTS.md b/AGENTS.md index 92be669a..7d3ff612 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -41,6 +41,120 @@ This file is the **entry point**. For full details, read the files it points to. 7. **Markdown files in `.forgeplan/` are the source of truth** (per ADR-003). The LanceDB index in `.forgeplan/lance/` is derived — rebuild via `forgeplan scan-import` if needed. +## Parallel agents and the build directory + +`target/` in this repository runs 20-45 GB. The disk it lives on is routinely +under 20 GB free. Several agents compiling at once is the single most reliable +way to break a session here, and it has happened four times. + +**The failure does not look like a failure.** When the disk fills, cargo dies +with `ld: write() failed, errno=28`, and the harness around it reports +`passed=0 failed=0` at exit 0 — or an empty summary. A check that never ran and +a check that passed are indistinguishable in every summary format we use. On +one occasion this produced 26 phantom clippy warnings that did not exist. Twice +the result was reported as green before anyone noticed. + +Rules for anyone dispatching sub-agents in this repo: + +- **Never let more than one agent compile at a time.** No parallel `cargo + build` / `test` / `check` / `clippy`. Feature unification means agents with + different flags will also thrash each other's artifacts even when the disk + holds. +- **Prefer read-only review.** Reading source with Read/Grep answers most + review questions. When execution is genuinely needed, hand agents the + already-built `./target/debug/forgeplan` and let them work in `/tmp` + workspaces via `forgeplan init -y`. Say so in the prompt — agents reach for + `cargo test` by default. +- **Check `df -h .` before any full run**, and again in the same command that + reports the result. Under ~10 GB free, clean first: + `cargo clean -p forgeplan -p forgeplan-core -p forgeplan-mcp` recovers + 15-37 GB while keeping compiled dependencies. `find target -maxdepth 2 -type + d -name incremental -exec rm -r {} +` is the cheap version — note that the + safety hook blocks `rm -rf` but permits this form. +- **Make the harness say when a run did not happen.** Count the test binaries, + not just the pass/fail line: + + ```bash + n=$(grep -c '^test result:' "$log") + if [ "$n" -eq 0 ]; then echo "RUN DID NOT HAPPEN"; df -h . | tail -1; fi + ``` + + A summary that cannot distinguish "zero failures" from "zero tests" is worse + than no summary. + +Two related traps, same family: + +- **Never edit source while a background build runs.** The binary captures the + file mid-edit, so the tree and the artifact disagree. It presents as a real + defect visible only in E2E while unit tests pass — suspect the binary before + the code. +- **Two agents in one worktree is a race.** Use `git worktree add` per agent + for anything that writes. A concurrent commit from a second session has + already cost a lost commit-message file and half an hour of misdiagnosis in + this repo. +- **The exit code you print through a pipe is the wrong one.** `cmd | tail` and + `echo $?` reports `tail`. `${PIPESTATUS[0]}` is a bashism and expands to the + empty string in this repo's zsh, so the guard prints `exit=` and reads as + fine. Redirect to a file and measure without a pipe: + `cargo clippy ... > /tmp/x.log 2>&1; echo "exit=$?"`. Same family as the + disk-full trap above — the number is real, it is just measuring something + else. + +## Findings have a home, and it is not the chat + +A finding that lives only in a reply is gone at the end of the session. This has +already cost real work here: a measurement across 465 artifacts that cancelled a +planned fix lived nowhere but a chat message until someone thought to file it, +and a handoff document once pointed at a scratch directory that did not survive +the session it described. + +**File it the moment you find it, before finishing the thought that produced +it.** Three kinds of finding, three homes: + +| Finding | Home | Why there | +|---|---|---| +| A rule that changes how an agent works | **this file** | Read every session. Costs context every session, so it earns its place only by changing behaviour. | +| Work someone should do | **GitHub issue**, open | Assignable, closable, outside the context budget. | +| A measured dead end — checked, decided not to do | **GitHub issue, closed on arrival**, label `measured-not-planned` | Findable by `gh issue list --state closed --label measured-not-planned`. Costs nothing to keep. | + +The third row is the one people skip, and it is the one that pays. A measurement +that **cancels** work is worth as much as one that starts it, and it evaporates +faster — nobody files "we checked and it does not matter". Then the next person +smells the same thing and re-derives it. #476 is the worked example: two +plausible defects, both measured, both left alone, numbers on the record. + +Rules for filing: + +- **Numbers, not adjectives.** "Zero verdicts change across 465 artifacts" + survives a year. "Seems fine" does not. +- **Every finding carries a trigger to revisit, phrased as a condition.** + `"later"` is not a trigger. `"if stripping comments would change the verdict + on one or more artifacts"` is. Same rule ADRs already follow: name what must + become true for someone to pick this up. +- **Say what was rejected and why.** A finding that records only the conclusion + invites the next person to relitigate the option you already killed. +- **Do not file the same thing twice.** `gh issue list --search` before + creating; extend the existing issue if one fits. + +Revisit is a step, not a habit: **before opening a `release/v*` PR**, list the +`measured-not-planned` issues and re-check their triggers against the current +corpus. A trigger that has become true reopens the issue; one that has not gets +left alone without discussion. + +## GitHub mechanics that silently do nothing + +- **`Closes #N` in a PR body does not close anything when merging into `dev`.** + GitHub honours closing keywords only on merges into the *default* branch, and + ours is `main`. Feature branches merge into `dev`, so the keyword is inert and + the issue stays open with no error anywhere. Close the issue by hand at + dev-merge, naming the merge commit — leaving it open until the release means + the board says "nobody did this" about finished work. +- **An empty `gh` result is not proof of absence.** `Post + "https://api.github.com/graphql": EOF` prints nothing to stdout and can exit + through a pipe as success. Verify a listing you are about to act on by + querying the object directly, not by trusting that zero rows means zero + things. + ## Repository structure (quick map) ``` @@ -71,6 +185,36 @@ ForgePlan/ - **Code identifiers & commit descriptions:** English - **Communication with the user:** Russian +## How to explain things to the owner + +Set by the owner on 2026-09-05, after comparing two answers about the same +decisions: one written in architecture-speak, one in plain language. The +plain one worked; the other one — «хрен поймёшь, о чём речь». + +When presenting a decision, a trade-off, or a piece of analysis: + +- **Plain, but not banal.** Simple words carrying real content — never + simple words instead of content. The test: the reader should be able to + retell the decision to someone else after one read. +- **Name the collision before the answer.** Most decisions exist because two + things conflict. Show the conflict first («два действующих ADR говорили + противоположное»), then the resolution. An answer without its tension + reads as arbitrary. +- **One live example beats three definitions.** «Приходит задача — кто + решает, что сначала запустится аналитик, потом кодер?» explains an + orchestration boundary faster than any glossary. +- **Minimal anglicisms.** Code identifiers, artifact kinds, and command + names stay as-is (`claim`, `EvidencePack`, `based_on`). Everything else + gets a Russian phrasing: «замок на запись», not «лок»; «происхождение», + not «провенанс»; «поверхность команд» needs an explanation the first time + it appears. +- **Consequences, not just verdicts.** «Deprecated» is a verdict; «хоронить, + не лечить — всё, что она обещала, уже делают NOTE и Hindsight» is a + decision someone can agree or argue with. +- **Metaphors must carry weight.** «Нотная тетрадь и приёмная комиссия» is + good because both halves map to real subsystems. Decoration is worse than + nothing. + ## Authorship (single author) Forgeplan is a single-author project. When generating ANY author-attributed content, use: diff --git a/CHANGELOG.md b/CHANGELOG.md index 1cf24882..189e5744 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,213 @@ corresponding sprint evidence under `.forgeplan/evidence/`. ## [Unreleased] +## [0.37.0] - 2026-09-08 + +Sprint headline: **Trust the number, and the write, and the gate that proves it.** +An EvidencePack with no `verdict` and no `congruence_level` has been scoring a +perfect **1.00** — the opposite of every document describing this system. A +`forgeplan deprecate` was printing a reason and putting it nowhere a fresh clone +could ever see it. A test file written to catch a live engine swap has run zero +times in CI since the swap it exists for. None of the three looked broken — +each reported success, correctly formatted, right up until someone read the +file it was supposed to have written to. + +Scope note for scripted consumers: no new CLI flag, no new config key, MCP tool +count unchanged (73). One breaking behaviour change: R_eff for any artifact +whose weakest evidence pack lacks `verdict`/`congruence_level` drops from 1.0 to +0.1 — **run `forgeplan score --all` after upgrading** (see below). + +### Changed — BREAKING, re-score required + +- **Evidence that declares nothing no longer scores full marks** (PROB-101, + PRD-086 FR-008/FR-009). An EvidencePack with no `verdict` and no + `congruence_level` scored the artifact it informs **1.00**. `CLAUDE.md` + RED LINE #7, the `/forge` skill that `setup-skill` installs, and + `EVIDENCE-PROTOCOL.md` all state the opposite — CL0, score 0.1 — so the + binary and every document describing it disagreed, in the direction that + inflates trust. + + The incentive was backwards too: writing `congruence_level: 2` honestly + scored 0.9, writing nothing scored 1.0. An unrecognised `verdict` fell + through to `Supports` for the same reason, while `congruence_level` in the + same function already failed closed with a warning. + + Both now fail closed to CL0 and log why. **Run `forgeplan score --all` + after upgrading.** Packs missing either field drop from 1.0 to 0.1, and + artifacts whose weakest link was such a pack drop with them. In this + repository that is 3 of 167 packs (EVID-033/034/035, feeding PROB-014, + PROB-016 and RFC-004). Your workspace may differ, and the drop is the + correct reading — those scores were never earned. + +### Fixed + +- **A leaf EvidencePack scored zero** (#325). The scorer asked a pack for its + evidence, found none — a pack has no packs — and returned 0.0 with the + factor `No evidence found (L0)`. A canonical pack (`verdict: supports`, + `congruence_level: 3`) was worth nothing, and the only way to raise it was + to invent child evidence. + + The intrinsic score already existed: `score_evidence_full` is applied to + that same pack whenever it scores for something else. It is now applied to + the pack itself. Packs that do carry child evidence keep the normal + weakest-link path. + +- **Trust flowed backwards along `informs`** (#325, FR-002). A pack's + outgoing edges point at what it *supports*; treating them as dependencies + made a measurement's reliability depend on the decision it justifies. They + are excluded from the pack's own dependency walk. + +- **An exempt Note poisoned everything built on it** (#392, narrowed). The + routing table calls a Note the artifact for trivial reversible work — no + ADI, no evidence. The cascade then read an active unevidenced Note as zero + trust, so forgeplan said a Note needs no evidence and scored everything + downstream of it as unevidenced. `note` and `memory` are now skipped in the + dependency walk with a logged factor, exactly as ADR-002 skips non-active + dependencies. + + The weakest-link formula is unchanged for every kind that *can* owe + evidence. The three fixes proposed in #392 — local-only scoring, one-hop + propagation, a floor at `self_score` — were each an average in disguise and + were declined; see the issue for the reasoning. + +- **`advance_phase` walked phases backwards** (#330). It had no monotonicity + guard, and MCP `forgeplan_validate` calls it with `Phase::Validate` on every + PASS — so validating an already-shipped artifact reset its phase from `done` + and `forgeplan_health` then reported a mismatch the artifact did not have + until someone checked it. Backward transitions are refused with an + explanation; `forgeplan phase-advance --to ` still works, because a + human correcting a mistake is not automation misfiring. + +- **The anomaly detector reported three things it never checked** (#393). It + printed `R_eff=0` from a literal rather than the stored score (now + `r_eff_cached`, named so, because the reporter's confusion came from + comparing it against a fresh `score` run); it labelled every give-up + `cycle or depth cap` in graphs with zero cycles (now names which of depth + cap, revisit, or exhausted actually happened); and its ancestor walk + followed edges the scorer skips, so the two disagreed about the weakest + link. The walk now applies the scorer's skip rules. + +- **`embed` loaded the model to discover it had nothing to do** (PROB-103). + 8.22s on a fully-current 427-artifact workspace, spent reading a model that + was never used — the missing half of the PROB-093 incremental fix, which + removed the encoding work but left the setup for it. 0.25s now, and + `Loading embedding model...` prints only when a model is actually loading. + The progress line also counted every record instead of the ones being + encoded (`Embedding 1 of 427`, not `Embedding 427`). + +- **The SPEC validator passed empty templates and blocked real specs** (#450, + PROB-105). An untouched `forgeplan new spec` template — every field still a + placeholder — validated `PASS — 0 error(s), 0 warning(s)` and activated at + **R_eff 1.00**. A spec with two requirements and two GIVEN/WHEN/THEN + scenarios failed the MUST rule `spec-contracts`, so it could not activate at + all. The kernel had a MUST pointing *against* behavioural specs while waving + through documents that said nothing. + + Two causes. `check_stub`'s twelve phrase markers are all PRD prose, and its + placeholder signal was capped at `+1` against a threshold of 3 — fifteen + unfilled slots weighed the same as one. The count scales now; the threshold + comes from the corpus (SPEC template **15** placeholders, PRD template + **5**, the six real SPECs **0–3**), not from taste. And `spec-contracts` + demanded one particular contract shape; it still demands *a* contract, but + `## Requirements`, `## Contract`, and `## Behavioral Contract` now count + alongside `## API` and `## Data Model`. + + This is why the marketplace TDD flow grew its own scenario gate: core was + rejecting the shape TDD needs. + +- **The stub gate told every artifact kind to fill a PRD's sections.** Its + remediation line read `Fill MUST sections (Problem, Goals, FR)` whether the + artifact was a SPEC, an ADR, or an Epic. It now names the sections of the + kind in hand. + +### Added + +- **`spec-requirement-has-scenario`** (Should, #450). Fires only on a spec + that already writes requirements behaviourally and leaves one without a + scenario. Deliberately conditional: the blanket form was measured against + this repository and fired on 6 of 6 SPECs, none of them defective. + +- **`prd-nfr-exist` and `prd-nfr-measurable`** (Should, #449). The PRD had 24 + validator rules and none about non-functional requirements. `extract_nfr_section` + already existed and was called from exactly one place — the tech-leakage + check — so the validator could find the NFR section and asked nothing about + its contents. + + Both are Should, not Must: 30 of 69 PRDs here have no NFR section at all. + `prd-nfr-measurable` strips non-prose before scanning, which is the whole + difficulty — the subjective-adjective list (`scalable`, `robust`, + `efficient`, `responsive`) reads like a list of NFRs and had only ever been + applied to the FR section. Across the 69 PRDs it matches **2** places in FR + and **15** in NFR, and all 15 sit inside the template's own + `` guidance. A rule + flagging those would be unclosable — the only fix would be deleting the + instructions. Verified: **0 findings across all 69 PRDs**, while + hand-written vague prose still produces findings with line numbers. + +### Fixed + +- **`forgeplan deprecate` / `renew` / `reopen` printed a reason and stored it + nowhere durable.** The command echoed the reason back and `forgeplan get` + showed it, but the markdown file never received the `## Deprecation` / + `## Renewal` / `## Reopened` section — only the status did, because status + lives in frontmatter and the section does not. `.forgeplan/lance/` is + gitignored, so the reason did not exist on a fresh clone. Worse than a + missing write: the next lifecycle command on that artifact synced the + section-less file body back over the index, erasing the reason there too — + the disagreement between file and index was temporary, the loss was not. + Root cause was a collision between two individually-correct behaviours: + `render_projection` is files-first by design (a user's on-disk edits must + survive `link`/`tag`/`activate`), and it discarded whatever body these + three commands handed it. The three CLI call sites and the MCP `deprecate` + handler now use the forcing variant, safe only there because a + file→store sync always runs immediately before. Recovery for anyone + already hit by this: `forgeplan update --body @path` projects + correctly and restores the section by hand. + +- **The embedding correctness oracle ran in CI exactly zero times since it + was written** (PROB-102). `tests/embedding_reference.rs` pins the engine's + output against pre-tract values from the v0.35.0 ONNX → tract swap it + exists to catch; the file is entirely behind `semantic-search`, so + `check`/`clippy` compiled it and `cargo nextest run --workspace` — invoked + without the feature — never even built it into that run. A new CI job + runs it in isolation with the model cached across runs, and converts the + oracle's normal quiet local-dev skip (a missing model reads as PASS, not + skipped) into a loud failure for this job specifically — a cold cache + proving nothing would otherwise reproduce the exact defect being closed, + one layer down. + +### Internal + +- ADR-025 (orchestration sits above ForgePlan; per-surface dispositions) and + ADR-026 (storage classes for machine-written records) resolve two vNext + audit blockers that required a human decision. EVID-169 records the basis. +- PROB-104: a leaf pack with an evidence neighbour reports the neighbour's + score. Recorded, not yet fixed — found while scoping this release, not + introduced by it. + +### Security + +33 open Dependabot alerts at release time (8 high / 15 medium / 10 low), one +Rust, 32 npm — full triage in +[`docs/operations/dependabot-triage-2026-09-08.md`](docs/operations/dependabot-triage-2026-09-08.md). + +- **`lru` LOW (GHSA-rhfx-m35p-ff5j) — accepted-with-justification, carried + forward.** The only consumer is `tantivy 0.24.2`, which pins `lru 0.12.x`; + the fix landed in `0.16.3`, a major bump only `tantivy` can take. Forgeplan + never constructs an `lru` cache or calls the affected method. Same verdict + as v0.33.0 through v0.36.0. +- **All 32 npm alerts — scheduled.** Confined to `website/`, a statically + generated docs site shipping no server and no part of any released + artifact. One of them (#442) carries an `astro` 6→7 major inside a + Dependabot group PR opened before the `Website build` CI gate existed — its + green checkmarks don't include the one check that would exercise a + two-major jump. Filed as #485 rather than merged on stale-green. +- `cargo-deny` (`security` workflow) is **green on `dev`** — checked directly + rather than inferred from an empty Dependabot list, because RustSec is not + mirrored into Dependabot and that gap has cost this project a red `dev` gate + twice before (v0.34.0, v0.35.0) without Dependabot ever showing a symptom. + + ## [0.36.0] - 2026-09-04 Sprint headline: **Things that reported success while verifying nothing.** Every defect here behaved correctly — search returned plausible results, hints were runnable, the release built green — which is exactly what hid them. diff --git a/CLAUDE.md b/CLAUDE.md index cff6cb88..88e98463 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -88,6 +88,24 @@ semantic search via BGE-M3, typed links, lifecycle with validation gates. ## Current status +- **v0.37.0** (2026-09-08) — **trust the number, the write, and the gate that checks it**. + An EvidencePack with no `verdict` and no `congruence_level` scored a flat **1.00** — + the opposite of what every document describing this system says. `forgeplan deprecate` + printed a reason and put it nowhere a fresh clone could ever see: the `## Deprecation` + section only reached LanceDB (`.forgeplan/lance/` is gitignored), and the next lifecycle + command synced the section-less file back over the index — the file/index disagreement + was temporary, the data loss was not. The embedding correctness oracle + (`embedding_reference.rs`) had never run in CI since it was written: the file is + entirely behind `semantic-search`, `check`/`clippy` compile it, `cargo nextest run + --workspace` does not, so its three assertions against the live engine never even + entered that build. **None of this looked broken** — each one reported success in the + right format, right up until someone read the file it claimed to have written to. + **Breaking**: R_eff for any artifact whose weakest evidence pack lacks `verdict`/ + `congruence_level` drops from 1.0 to 0.1 — **run `forgeplan score --all` after + upgrading**. Also: a new CI job runs the embedding oracle with a cached model, plus + `--test-threads=1` for it — on the first cold run, two tests that both need the model + ran concurrently and raced for the same download. Open: PROB-104 (a leaf pack with an + evidence neighbour reports the neighbour's score). - **v0.36.0** (2026-09-04) — **вещи, которые отчитывались об успехе, ничего не проверяя**. Каждый дефект релиза вёл себя корректно — поиск возвращал правдоподобное, подсказки были исполнимы, сборка была зелёной, — и именно это их скрывало. @@ -148,7 +166,7 @@ semantic search via BGE-M3, typed links, lifecycle with validation gates. Migration: run `forgeplan score --all`; expect a small number of artifacts whose only evidence was retired to drop to 0 (1 of 89 here) — that is real debt the old formula masked. -- **82 CLI commands** (+`setup`; прежние «82» считали авто-`help` от clap), **73 MCP tools**, **3290 tests + 9 doc-tests** (CI `nextest`), **0 warnings** on both feature configs +- **82 CLI commands** (+`setup`; прежние «82» считали авто-`help` от clap), **73 MCP tools**, **3331 tests + 9 doc-tests** (CI `nextest`), **0 warnings** on both feature configs - **EPIC-001/002/003 ✅**, **Epic #287 ✅** (brownfield). Phase 5 (Desktop Tauri) — backlog - FPF KB semantic search via BGE-M3 on `tract` (pure-Rust inference — RFC-013; feature-gated, graceful fallback) diff --git a/Cargo.lock b/Cargo.lock index 017b80fe..982b62bf 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -98,7 +98,7 @@ version = "1.1.5" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "40c48f72fd53cd289104fc64099abca73db4166ad86ea0b4341abe65af83dadc" dependencies = [ - "windows-sys 0.60.2", + "windows-sys 0.61.2", ] [[package]] @@ -109,7 +109,7 @@ checksum = "291e6a250ff86cd4a820112fb8898808a366d8f9f58ce16d1f538353ad55747d" dependencies = [ "anstyle", "once_cell_polyfill", - "windows-sys 0.60.2", + "windows-sys 0.61.2", ] [[package]] @@ -432,9 +432,9 @@ dependencies = [ [[package]] name = "async-trait" -version = "0.1.91" +version = "0.1.92" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ae36dc4177970ef04fde5178d3e2429882def40e57a451f919c098f72baa6cec" +checksum = "82f6aeea286b8eb4dd3431a1be1b59d290ace00f5bfd8e2a159bc2a05e2c1667" dependencies = [ "proc-macro2", "quote", @@ -851,9 +851,9 @@ dependencies = [ [[package]] name = "clap" -version = "4.6.3" +version = "4.6.6" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "0fb99565819980999fb7b4a1796046a5c949e6d4ff132cf5fadf5a641e20d776" +checksum = "473c7e07f409a8d772161724aa8db6a765a2532a70f9667eeb7b49d3d02fbdca" dependencies = [ "clap_builder", "clap_derive", @@ -861,9 +861,9 @@ dependencies = [ [[package]] name = "clap_builder" -version = "4.6.2" +version = "4.6.6" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f09628afdcc538b57f3c6341e9c8e9970f18e4a481690a64974d7023bd33548b" +checksum = "7b48fea5a88e9ae728a2dcbedbfc0e730f7d60da42e1cb049a83c9fb8b789889" dependencies = [ "anstream", "anstyle", @@ -873,14 +873,14 @@ dependencies = [ [[package]] name = "clap_derive" -version = "4.6.3" +version = "4.6.4" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "32f2392eae7f16557a3d727ef3a12e57b2b2ca6f98566a5f4fb41ffe305df077" +checksum = "d012d2b9d65aca7f18f4d9878a045bc17899bba951561ba5ec3c2ba1eed9a061" dependencies = [ "heck", "proc-macro2", "quote", - "syn 2.0.117", + "syn 3.0.2", ] [[package]] @@ -891,9 +891,9 @@ checksum = "c8d4a3bb8b1e0c1050499d1815f5ab16d04f0959b233085fb31653fbfc9d98f9" [[package]] name = "cliclack" -version = "0.5.5" +version = "0.5.6" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "9fd2a475e9dff35ddebd459b0523aab732572581f7fe53133edd20b3330a0ce6" +checksum = "134778b6a125729ff495c23ba32f64e0c26d61edd52f1f6715a0d036785cf30b" dependencies = [ "console", "indicatif", @@ -1188,9 +1188,9 @@ dependencies = [ [[package]] name = "daachorse" -version = "1.0.1" +version = "3.0.3" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "6f55d7153ba3b507595872a3874803f07a8a81d1e888abed8e5db7da0597d6e2" +checksum = "5614204febbc33cc07a2806aa6440b904ac012b68eecc37f4493ea4a76455a3d" [[package]] name = "darling" @@ -2012,7 +2012,7 @@ dependencies = [ "libc", "option-ext", "redox_users", - "windows-sys 0.59.0", + "windows-sys 0.61.2", ] [[package]] @@ -2122,7 +2122,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "39cab71617ae0d63f51a36d69f866391735b51691dbda63cf6f96d042b63efeb" dependencies = [ "libc", - "windows-sys 0.52.0", + "windows-sys 0.61.2", ] [[package]] @@ -2253,7 +2253,7 @@ checksum = "77ce24cb58228fbb8aa041425bb1050850ac19177686ea6e0f41a70416f56fdb" [[package]] name = "forgeplan" -version = "0.36.0" +version = "0.37.0" dependencies = [ "anyhow", "assert_cmd", @@ -2279,7 +2279,7 @@ dependencies = [ [[package]] name = "forgeplan-core" -version = "0.36.0" +version = "0.37.0" dependencies = [ "anyhow", "arrow-array", @@ -2320,7 +2320,7 @@ dependencies = [ [[package]] name = "forgeplan-mcp" -version = "0.36.0" +version = "0.37.0" dependencies = [ "anyhow", "chrono", @@ -2402,9 +2402,9 @@ checksum = "e6d5a32815ae3f33302d95fdcb2ce17862f8c65363dcfd29360480ba1001fc9c" [[package]] name = "futures" -version = "0.3.32" +version = "0.3.34" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "8b147ee9d1f6d097cef9ce628cd2ee62288d963e16fb287bd9286455b241382d" +checksum = "9a31d2a3fbaaeb2af2368bbdd904aa8e812d3c04a1ee10d3171f52d556e5d0a3" dependencies = [ "futures-channel", "futures-core", @@ -2417,9 +2417,9 @@ dependencies = [ [[package]] name = "futures-channel" -version = "0.3.32" +version = "0.3.34" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "07bbe89c50d7a535e539b8c17bc0b49bdb77747034daa8087407d655f3f7cc1d" +checksum = "b1f9e3d69d39e4862ffed03ed071a76f9a13ba1d9109d355b0f0aa6b15e393c4" dependencies = [ "futures-core", "futures-sink", @@ -2427,15 +2427,15 @@ dependencies = [ [[package]] name = "futures-core" -version = "0.3.32" +version = "0.3.34" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "7e3450815272ef58cec6d564423f6e755e25379b217b0bc688e295ba24df6b1d" +checksum = "92d699e522242e69e3003b94ecc1f960f3a5e015aa7c5d7486e65ad01dd94f5e" [[package]] name = "futures-executor" -version = "0.3.32" +version = "0.3.34" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "baf29c38818342a3b26b5b923639e7b1f4a61fc5e76102d4b1981c6dc7a7579d" +checksum = "031b47cf1a3c6cc8bc2fc76cd437f521619387907d469316e7c0bc278f1f5432" dependencies = [ "futures-core", "futures-task", @@ -2444,38 +2444,38 @@ dependencies = [ [[package]] name = "futures-io" -version = "0.3.32" +version = "0.3.34" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "cecba35d7ad927e23624b22ad55235f2239cfa44fd10428eecbeba6d6a717718" +checksum = "53c0fa8157de1303bfffdaa1cc2a673bfffb60102f76b0ef4441659124373fed" [[package]] name = "futures-macro" -version = "0.3.32" +version = "0.3.34" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e835b70203e41293343137df5c0664546da5745f82ec9b84d40be8336958447b" +checksum = "9fb9654ba8355388abeb8dcb4fc62f511300867002afc858860463bdd9fe0c44" dependencies = [ "proc-macro2", "quote", - "syn 2.0.117", + "syn 3.0.2", ] [[package]] name = "futures-sink" -version = "0.3.32" +version = "0.3.34" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "c39754e157331b013978ec91992bde1ac089843443c49cbc7f46150b0fad0893" +checksum = "1944426bf7d03f1d14f708785e4b33efd750b36d48a157b836b3efc15ede8e1d" [[package]] name = "futures-task" -version = "0.3.32" +version = "0.3.34" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "037711b3d59c33004d3856fbdc83b99d4ff37a24768fa1be9ce3538a1cde4393" +checksum = "cd417de3d1d015fc3bfd2b1ea46dfc7bab72ef86f1cc7cc9c78e728b34a6d1fd" [[package]] name = "futures-util" -version = "0.3.32" +version = "0.3.34" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "389ca41296e6190b48053de0321d02a77f32f8a5d2461dd38762c0593805c6d6" +checksum = "0d50a92467f8ba5dd6e3ee5d4bd04d73ab2e4e1c44474a0674821dfce14b79bc" dependencies = [ "futures-channel", "futures-core", @@ -2580,9 +2580,9 @@ checksum = "0cc23270f6e1808e30a928bdc84dea0b9b4136a8bc82338574f23baf47bbd280" [[package]] name = "globset" -version = "0.4.19" +version = "0.4.20" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e47d37d2ae4464254884b60ab7071be2b876a9c35b696bd018ddcc76847309cd" +checksum = "07c34a9410465b45bd9787443bc7370f37735bad04b0f0cd57ff1a3186c98988" dependencies = [ "aho-corasick", "bstr", @@ -3129,7 +3129,7 @@ dependencies = [ "portable-atomic", "portable-atomic-util", "serde_core", - "windows-sys 0.52.0", + "windows-sys 0.61.2", ] [[package]] @@ -3853,9 +3853,9 @@ dependencies = [ [[package]] name = "libc" -version = "0.2.187" +version = "0.2.189" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a7743783ea728ef5c31194c6590797eed286449b4a4e87d626d8a51f0a94e732" +checksum = "3eaf3ede3fee6db1a4c2ee091bf8a8b4dccdc6d17f656fb07896ee72867612f2" [[package]] name = "libm" @@ -4292,7 +4292,7 @@ version = "0.50.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7957b9740744892f114936ab4a57b3f487491bbeafaf8083688b16841a4240e5" dependencies = [ - "windows-sys 0.59.0", + "windows-sys 0.61.2", ] [[package]] @@ -5180,9 +5180,9 @@ dependencies = [ [[package]] name = "regex-automata" -version = "0.4.16" +version = "0.4.18" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "8fcfdb36bda0c880c5931cdc7a2bcdc8ba4556847b9d912bca70bc94708711ad" +checksum = "ad8553b9b26413251cbf30e620595c7a41b3887f03da04579c0e6b0d6a06b4b2" dependencies = [ "aho-corasick", "memchr", @@ -5364,7 +5364,7 @@ dependencies = [ "errno", "libc", "linux-raw-sys 0.12.1", - "windows-sys 0.52.0", + "windows-sys 0.61.2", ] [[package]] @@ -5594,9 +5594,9 @@ checksum = "1bc711410fbe7399f390ca1c3b60ad0f53f80e95c5eb935e52268a0e2cd49acc" [[package]] name = "serde" -version = "1.0.228" +version = "1.0.229" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "9a8e94ea7f378bd32cbbd37198a4a91436180c5bb472411e48b5ec2e2124ae9e" +checksum = "4148590afebada386688f18773da617792bf2ef03ffc1e4cbd2b1d45b023e0ba" dependencies = [ "serde_core", "serde_derive", @@ -5604,22 +5604,22 @@ dependencies = [ [[package]] name = "serde_core" -version = "1.0.228" +version = "1.0.229" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "41d385c7d4ca58e59fc732af25c3983b67ac852c1a25000afe1175de458b67ad" +checksum = "67dca2c9c51e58a4791a4b1ed58308b39c64224d349a935ab5039aa360942a48" dependencies = [ "serde_derive", ] [[package]] name = "serde_derive" -version = "1.0.228" +version = "1.0.229" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "d540f220d3187173da220f885ab66608367b6574e925011a9353e4badda91d79" +checksum = "e7a5d71263a5a7d47b41f6b3f06ba276f10cc18b0931f1799f710578e2309348" dependencies = [ "proc-macro2", "quote", - "syn 2.0.117", + "syn 3.0.2", ] [[package]] @@ -5881,7 +5881,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "3a766e1110788c36f4fa1c2b71b387a7815aa65f88ce0229841826633d93723e" dependencies = [ "libc", - "windows-sys 0.60.2", + "windows-sys 0.61.2", ] [[package]] @@ -6243,7 +6243,7 @@ dependencies = [ "getrandom 0.4.2", "once_cell", "rustix 1.1.4", - "windows-sys 0.52.0", + "windows-sys 0.61.2", ] [[package]] @@ -6281,18 +6281,18 @@ dependencies = [ [[package]] name = "thiserror" -version = "2.0.19" +version = "2.0.20" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "09a43598840e33d5b0331f38c5e30d13bb11c11210a4b58f0d9b18a5a5eefcd9" +checksum = "ec86235f5fcc2a73650310756d2ac5b138a5780bbbdfae3eeccec992c435ba4f" dependencies = [ "thiserror-impl", ] [[package]] name = "thiserror-impl" -version = "2.0.19" +version = "2.0.20" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "43cbfe0cf76104d42a574802844187e84a305e531ed54455f11fbde0f10541cd" +checksum = "bc04cd3e1236dd4a98afca4569f2deb3f120e5422a4023be2cb683f8486292af" dependencies = [ "proc-macro2", "quote", @@ -6384,9 +6384,9 @@ checksum = "1f3ccbac311fea05f86f61904b462b55fb3df8837a366dfc601a0161d0532f20" [[package]] name = "tokenizers" -version = "0.23.1" +version = "0.23.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "44e5bea67576e04b6ff8564c5d9e09c2ef0cf476502245f2f120e497769d3112" +checksum = "7afbf6e88718afcc138bad01d6ccc3051dbbc3b2ce9793d8b8a3aeb610969cfc" dependencies = [ "ahash", "compact_str", @@ -7268,7 +7268,7 @@ version = "0.1.11" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "c2a7b1c03c876122aa43f3020e6c3c3ee5c05081c9a00739faf7503aeba10d22" dependencies = [ - "windows-sys 0.52.0", + "windows-sys 0.61.2", ] [[package]] diff --git a/Cargo.toml b/Cargo.toml index cd3467d9..103da272 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -7,7 +7,7 @@ members = [ ] [workspace.package] -version = "0.36.0" +version = "0.37.0" edition = "2024" license = "MIT" repository = "https://github.com/ForgePlan/forgeplan" diff --git a/README.md b/README.md index 4b8a1ca6..5ef0d0e3 100644 --- a/README.md +++ b/README.md @@ -14,7 +14,7 @@ Structured artifacts (PRD, RFC, ADR, Epic, Spec), quality scoring, evidence, and [![License: MIT](https://img.shields.io/badge/license-MIT-000.svg?style=flat-square)](LICENSE) [![Release](https://img.shields.io/github/v/release/ForgePlan/forgeplan?include_prereleases&style=flat-square&color=orange)](https://github.com/ForgePlan/forgeplan/releases) [![CI](https://img.shields.io/github/actions/workflow/status/ForgePlan/forgeplan/ci.yml?branch=main&style=flat-square)](https://github.com/ForgePlan/forgeplan/actions) -[![Artifacts](https://img.shields.io/badge/artifacts-394-blue?style=flat-square)](.forgeplan/) +[![Artifacts](https://img.shields.io/badge/artifacts-437-blue?style=flat-square)](.forgeplan/) **[Website](https://forgeplan.dev)** · **[Documentation](docs/README.md)** · @@ -248,8 +248,8 @@ Three entry points — pick the one that matches what you need right now. - - + + diff --git a/crates/forgeplan-cli/Cargo.toml b/crates/forgeplan-cli/Cargo.toml index 67e007ee..79aee510 100644 --- a/crates/forgeplan-cli/Cargo.toml +++ b/crates/forgeplan-cli/Cargo.toml @@ -18,8 +18,8 @@ name = "forgeplan" path = "src/main.rs" [dependencies] -forgeplan-core = { path = "../forgeplan-core", version = "0.36.0" } -forgeplan-mcp = { path = "../forgeplan-mcp", version = "0.36.0" } +forgeplan-core = { path = "../forgeplan-core", version = "0.37.0" } +forgeplan-mcp = { path = "../forgeplan-mcp", version = "0.37.0" } clap = { version = "4", features = ["derive"] } anyhow.workspace = true chrono.workspace = true diff --git a/crates/forgeplan-cli/src/commands/context.rs b/crates/forgeplan-cli/src/commands/context.rs index 235b15a0..81f6cc9f 100644 --- a/crates/forgeplan-cli/src/commands/context.rs +++ b/crates/forgeplan-cli/src/commands/context.rs @@ -175,7 +175,17 @@ pub async fn run(id: &str, json: bool) -> anyhow::Result<()> { Hint::warning(format!("Fix {} MUST error(s)", must_errors)) .with_action(format!("forgeplan validate {}", ref_form)), ); - } else if !has_evidence { + // PRD-086 FR-001. `has_evidence` counts LINKED child packs, which a leaf + // EvidencePack has none of by definition — it is the evidence. Since leaf + // packs began scoring on their own structured fields, this branch fired + // under an `R_eff: 1.00` printed a few lines above it, and its `Next:` told + // the agent to create an EvidencePack for an EvidencePack. That is a + // PRD-071 contract line an agent is obliged to run, so the cost is phantom + // artifacts in the graph, not just a confusing sentence. + // + // Guarding on the score keeps the advice true without teaching this + // function which kinds self-score. + } else if !has_evidence && report.r_eff <= 0.0 { hint_list.push( Hint::warning("No evidence linked") .with_action(format!( @@ -344,7 +354,8 @@ fn build_suggestions( )); } - if !has_evidence { + // PRD-086 FR-001 — see the hint above; same condition, same reason. + if !has_evidence && r_eff <= 0.0 { suggestions.push(format!( "Add evidence — `forgeplan new evidence \"Evidence for {}\"` + `forgeplan link EVID-XXX {} --relation informs`", id, id 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/embed.rs b/crates/forgeplan-cli/src/commands/embed.rs index 790ce3af..c3bc8c05 100644 --- a/crates/forgeplan-cli/src/commands/embed.rs +++ b/crates/forgeplan-cli/src/commands/embed.rs @@ -15,16 +15,6 @@ pub async fn run() -> anyhow::Result<()> { .map(|e| e.chunk_size) .unwrap_or(2000); - // Tell the user about a multi-gigabyte download BEFORE it starts, not - // after they notice the process sitting there. Silent when the model is - // already cached. - if let Some(notice) = forgeplan_core::embed::first_run_notice() { - ui::info(¬ice); - } - - ui::info("Loading embedding model..."); - let mut embedder = Embedder::new()?; - let records = store.list_records(None).await?; if records.is_empty() { ui::info("No artifacts to embed."); @@ -36,22 +26,20 @@ pub async fn run() -> anyhow::Result<()> { return Ok(()); } - println!( - "Embedding {} artifact(s) (title + body, chunk_size={})...\n", - records.len(), - chunk_size - ); - - let mut ok = 0usize; - let mut err = 0usize; + // PROB-093: only encode what actually moved. A record is current when it + // already carries a vector AND its content hash still matches. Before + // that fix, `embed` recomputed all 400+ records to index one new artifact + // — 13m18s on this workspace, which is why the step people were supposed + // to run manually did not get run. + // + // Deciding this BEFORE the model loads is the other half, and it was + // missing: the incremental skip removed the encoding work but left the + // load unconditional, so a fully-current workspace still paid 8.22s to + // report "0 embedded, 427 already current". The model was read off disk + // and never used. Work first, model only if there is work. + let mut work: Vec<(&forgeplan_core::db::store::ArtifactRecord, String)> = Vec::new(); let mut skipped = 0usize; - for record in &records { - // PROB-093: only encode what actually moved. A record is current when - // it already carries a vector AND its content hash still matches. - // Before this, `embed` recomputed all 400+ records to index one new - // artifact — 13m18s on this workspace, which is why the step people - // were supposed to run manually did not get run. let current_hash = forgeplan_core::db::store::compute_content_hash(&record.title, &record.body); if record.embedding.is_some() && record.body_hash.as_deref() == Some(current_hash.as_str()) @@ -59,13 +47,47 @@ pub async fn run() -> anyhow::Result<()> { skipped += 1; continue; } + work.push((record, current_hash)); + } + + if work.is_empty() { + println!("Done: 0 embedded, {skipped} already current, 0 failed."); + let hint_list = vec![ + Hint::info("Run a semantic search") + .with_action("forgeplan search \"\"".to_string()), + ]; + print!("{}", hints::render_next_action_line(&hint_list)); + return Ok(()); + } + + // Tell the user about a multi-gigabyte download BEFORE it starts, not + // after they notice the process sitting there. Silent when the model is + // already cached — and now silent as well when nothing needs encoding, + // which is the common case. + if let Some(notice) = forgeplan_core::embed::first_run_notice() { + ui::info(¬ice); + } + + ui::info("Loading embedding model..."); + let mut embedder = Embedder::new()?; + + println!( + "Embedding {} of {} artifact(s) (title + body, chunk_size={})...\n", + work.len(), + records.len(), + chunk_size + ); + + let mut ok = 0usize; + let mut err = 0usize; + for (record, current_hash) in &work { let text = record.embedding_text(chunk_size); match embedder.embed(&text) { Ok(vec) => { store.update_embedding(&record.id, &vec).await?; store - .update_body_hash(&record.id, ¤t_hash) + .update_body_hash(&record.id, current_hash) .await // The vector is written; a failed hash stamp only costs a // redundant re-encode next run, so it must not fail the diff --git a/crates/forgeplan-cli/src/commands/get.rs b/crates/forgeplan-cli/src/commands/get.rs index 768b4a2f..467200aa 100644 --- a/crates/forgeplan-cli/src/commands/get.rs +++ b/crates/forgeplan-cli/src/commands/get.rs @@ -29,6 +29,23 @@ pub async fn run(id: &str, json: bool) -> anyhow::Result<()> { .await .unwrap_or_default(); let has_links = !relations.is_empty() || !incoming.is_empty(); + + // #447 — both edge sets were already fetched above and then collapsed + // into `has_links` purely to pick a hint. The data was one line from the + // output and thrown away, so a linked artifact and an orphan rendered + // identically. Emitted unconditionally: a caller must be able to tell + // "this has no links" from "this tool does not report links", and an + // absent field says the same thing as an empty one. + let links_json = serde_json::json!({ + "outbound": relations + .iter() + .map(|(target, relation)| serde_json::json!({ "target": target, "relation": relation })) + .collect::>(), + "inbound": incoming + .iter() + .map(|(source, relation)| serde_json::json!({ "source": source, "relation": relation })) + .collect::>(), + }); let kind: forgeplan_core::artifact::types::ArtifactKind = record .kind .parse() @@ -89,6 +106,7 @@ pub async fn run(id: &str, json: bool) -> anyhow::Result<()> { "created_at": record.created_at, "updated_at": record.updated_at, "body": record.body, + "links": links_json, "_next_action": hints::primary_action(&hints_vec), }); println!("{}", serde_json::to_string_pretty(&json_data)?); @@ -118,6 +136,23 @@ pub async fn run(id: &str, json: bool) -> anyhow::Result<()> { ui::kv("R_eff", &ui::styled_reff(record.r_eff_score)); ui::kv("Created", &record.created_at); ui::kv("Updated", &record.updated_at); + // #447 — the same edges the JSON path reports. Printed even when there + // are none, for the same reason. + if relations.is_empty() && incoming.is_empty() { + ui::kv("Links", "none"); + } else { + if !relations.is_empty() { + let out: Vec = relations + .iter() + .map(|(t, r)| format!("{t} ({r})")) + .collect(); + ui::kv("Links out", &out.join(", ")); + } + if !incoming.is_empty() { + let inc: Vec = incoming.iter().map(|(s, r)| format!("{s} ({r})")).collect(); + ui::kv("Links in", &inc.join(", ")); + } + } println!(); println!("{}", record.body); diff --git a/crates/forgeplan-cli/src/commands/phase_advance.rs b/crates/forgeplan-cli/src/commands/phase_advance.rs index 66a22f1b..54b4d169 100644 --- a/crates/forgeplan-cli/src/commands/phase_advance.rs +++ b/crates/forgeplan-cli/src/commands/phase_advance.rs @@ -75,7 +75,16 @@ pub async fn run(id: &str, to: PhaseArg, reason: Option<&str>, json: bool) -> an let target: Phase = to.into(); let state = - phase::store::advance_phase(&ws, &canonical, target, reason.map(|s| s.to_string())).await?; + // #330 — the explicit operator path may move a phase in either + // direction; the guard belongs on automation, not on a deliberate + // command someone typed. + phase::store::advance_phase_unchecked( + &ws, + &canonical, + target, + reason.map(|s| s.to_string()), + ) + .await?; // PRD-071 contract: produce a single Next: action — the next suggested phase // advance. No suggestion when at terminal phase (Done.). 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/src/commands/score.rs b/crates/forgeplan-cli/src/commands/score.rs index da4d21a4..80baf19c 100644 --- a/crates/forgeplan-cli/src/commands/score.rs +++ b/crates/forgeplan-cli/src/commands/score.rs @@ -308,7 +308,29 @@ pub async fn run(id: Option<&str>, json: bool) -> anyhow::Result<()> { // --- Styled display --- ui::header(&target.id, &target.title); - if evidence_items.is_empty() { + // PRD-086 FR-001. "No evidence linked" is now only half a story for an + // EvidencePack: it has no child evidence and it is not supposed to, because + // it IS the evidence. The engine already scores it on its own fields — this + // branch was announcing `R_eff = 0.0` over a report that said 1.00, which is + // the same defect class the PRD exists to close, committed by the printer + // rather than the scorer. Caught by dogfooding on the real graph; the unit + // and integration tests both passed because neither reads this line. + let is_evidence = target.kind.eq_ignore_ascii_case("evidence"); + if evidence_items.is_empty() && is_evidence { + ui::info(&format!( + "Leaf evidence — scored on its own structured fields. R_eff = {:.2}", + report.r_eff + )); + if report.r_eff <= 0.1 { + println!(); + ui::error_hint( + "Structured fields missing or undeclared", + &format!( + "forgeplan get {target_ref} — add `verdict:` and `congruence_level:` under `## Structured Fields`" + ), + ); + } + } else if evidence_items.is_empty() { ui::info("No evidence linked. R_eff = 0.0"); println!(); ui::error_hint( diff --git a/crates/forgeplan-cli/src/commands/validate.rs b/crates/forgeplan-cli/src/commands/validate.rs index 0c861f01..b51f3c8b 100644 --- a/crates/forgeplan-cli/src/commands/validate.rs +++ b/crates/forgeplan-cli/src/commands/validate.rs @@ -56,7 +56,9 @@ Fix: forgeplan list", let mut json_results = Vec::new(); for record in &to_validate { - let fm = record.frontmatter_map(); + // #446 — the link-less reconstruction made body-links-drift compare + // against an empty set and warn on correctly linked artifacts. + let fm = store.frontmatter_map_with_links(record).await; let kind = record.kind.parse::().unwrap_or_else(|_| { if !json { diff --git a/crates/forgeplan-cli/tests/cli_embed_lazy_model.rs b/crates/forgeplan-cli/tests/cli_embed_lazy_model.rs new file mode 100644 index 00000000..629f39a5 --- /dev/null +++ b/crates/forgeplan-cli/tests/cli_embed_lazy_model.rs @@ -0,0 +1,81 @@ +//! `embed` must not load the model when there is nothing to encode — PROB-103. +//! +//! The defect was reported by a user reading the output, because no gate runs +//! `embed` at all (PROB-102). These tests are the gate that was missing. +//! +//! They are gated on `semantic-search` for the same reason the command is, and +//! CI's `cargo nextest run` currently passes no features — so they compile in +//! CI and execute only locally until PROB-102 is closed. That is stated here +//! rather than left for the next reader to discover from a green `0 passed`. + +#![cfg(feature = "semantic-search")] + +use assert_cmd::Command; +use tempfile::TempDir; + +fn fpl(ws: &TempDir) -> Command { + let mut cmd = Command::cargo_bin("forgeplan").unwrap(); + cmd.current_dir(ws.path()); + cmd +} + +fn init(ws: &TempDir) { + fpl(ws).args(["init", "-y"]).output().expect("init"); +} + +/// An empty workspace has nothing to encode, so the model must stay on disk. +/// +/// Note what this does and does not prove. It passes with the PROB-103 fix +/// reverted, because an empty workspace is caught by an older early return +/// (`No artifacts to embed.`) that predates it — verified by mutation. It is a +/// regression guard for that path, not evidence for this one. The test below +/// is the one that fails without the fix. +#[test] +fn no_artifacts_means_no_model_load() { + let ws = TempDir::new().unwrap(); + init(&ws); + + let out = fpl(&ws).arg("embed").output().expect("embed"); + let stdout = String::from_utf8_lossy(&out.stdout); + assert!( + !stdout.contains("Loading embedding model"), + "an empty workspace must not load the model, got:\n{stdout}" + ); +} + +/// The case the user actually hit: everything already embedded, and `embed` +/// run again. Before the fix this cost 8.22s on a 427-artifact workspace and +/// printed the loading banner every time. +#[test] +fn a_current_workspace_does_not_reload_the_model() { + let ws = TempDir::new().unwrap(); + init(&ws); + fpl(&ws) + .args(["new", "note", "Something to index"]) + .output() + .expect("new"); + + // First run does the work — the banner is expected here. + let first = fpl(&ws).arg("embed").output().expect("embed"); + let first_out = String::from_utf8_lossy(&first.stdout); + assert!( + first_out.contains("Loading embedding model"), + "a workspace with unembedded artifacts must load the model, got:\n{first_out}" + ); + + // Second run has nothing left to do. + let second = fpl(&ws).arg("embed").output().expect("embed"); + let second_out = String::from_utf8_lossy(&second.stdout); + assert!( + !second_out.contains("Loading embedding model"), + "nothing changed, so the model must not be loaded again, got:\n{second_out}" + ); + assert!( + second_out.contains("0 embedded"), + "the summary must still report the outcome, got:\n{second_out}" + ); + assert!( + second_out.contains("already current"), + "the skipped count must still be reported, got:\n{second_out}" + ); +} diff --git a/crates/forgeplan-cli/tests/cli_get_links.rs b/crates/forgeplan-cli/tests/cli_get_links.rs new file mode 100644 index 00000000..dc880768 --- /dev/null +++ b/crates/forgeplan-cli/tests/cli_get_links.rs @@ -0,0 +1,124 @@ +//! `forgeplan get` must report the artifact's edges — #447. +//! +//! The defect: `get` fetched both edge sets, collapsed them into a boolean to +//! pick a hint, and dropped them. An artifact with five links and an orphan +//! rendered identically, and the issue reports that costing a wrong conclusion +//! in a live session. +//! +//! These tests run the real binary against a real workspace, because the +//! defect was invisible to unit tests: every layer worked, the output just did +//! not carry the answer. + +use assert_cmd::Command; +use tempfile::TempDir; + +fn fpl(ws: &TempDir) -> Command { + let mut cmd = Command::cargo_bin("forgeplan").unwrap(); + cmd.current_dir(ws.path()); + cmd +} + +fn init(ws: &TempDir) { + fpl(ws).args(["init", "-y"]).output().expect("init"); +} + +fn new_artifact(ws: &TempDir, kind: &str, title: &str) -> String { + let out = fpl(ws).args(["new", kind, title]).output().expect("new"); + let stdout = String::from_utf8_lossy(&out.stdout); + // The `ID:` line is indented (` ID: ADR-001`), so trim before + // matching. Parsing this line rather than grepping for an id-shaped token + // matters: the hint lines below it also contain the id. + stdout + .lines() + .find_map(|l| l.trim().strip_prefix("ID:")) + .map(|s| s.trim().to_string()) + .unwrap_or_else(|| panic!("no `ID:` line in `new {kind}` output:\n{stdout}")) +} + +fn get_json(ws: &TempDir, id: &str) -> serde_json::Value { + let out = fpl(ws).args(["get", id, "--json"]).output().expect("get"); + let stdout = String::from_utf8_lossy(&out.stdout); + serde_json::from_str(&stdout).unwrap_or_else(|e| panic!("get --json not JSON ({e}):\n{stdout}")) +} + +/// The core of #447: an unlinked artifact must say so explicitly. An absent +/// field and an empty one are the same thing to a caller, which is exactly how +/// the orphan/unreported confusion arose. +#[test] +fn an_artifact_without_links_reports_empty_arrays_not_a_missing_field() { + let ws = TempDir::new().unwrap(); + init(&ws); + let id = new_artifact(&ws, "note", "Orphan"); + + let v = get_json(&ws, &id); + assert!( + v.get("links").is_some(), + "links must always be present, got keys: {:?}", + v.as_object().map(|o| o.keys().collect::>()) + ); + assert_eq!(v["links"]["outbound"].as_array().map(Vec::len), Some(0)); + assert_eq!(v["links"]["inbound"].as_array().map(Vec::len), Some(0)); +} + +/// Both directions are reported, and from the right side. Inbound is the half +/// `forgeplan graph | grep " -->"` cannot answer, and it is the one behind +/// "which evidence supports this". +#[test] +fn get_reports_outbound_and_inbound_edges_separately() { + let ws = TempDir::new().unwrap(); + init(&ws); + let prd = new_artifact(&ws, "prd", "Parent"); + let spec = new_artifact(&ws, "spec", "Child"); + let evid = new_artifact(&ws, "evidence", "Measurement"); + + fpl(&ws) + .args(["link", &spec, &prd, "--relation", "refines"]) + .output() + .expect("link spec->prd"); + fpl(&ws) + .args(["link", &evid, &spec, "--relation", "informs"]) + .output() + .expect("link evid->spec"); + + let v = get_json(&ws, &spec); + let outbound = v["links"]["outbound"].as_array().expect("outbound array"); + let inbound = v["links"]["inbound"].as_array().expect("inbound array"); + + assert_eq!(outbound.len(), 1, "expected one outbound edge: {v}"); + assert_eq!(outbound[0]["target"], serde_json::json!(prd)); + assert_eq!(outbound[0]["relation"], serde_json::json!("refines")); + + assert_eq!(inbound.len(), 1, "expected one inbound edge: {v}"); + assert_eq!(inbound[0]["source"], serde_json::json!(evid)); + assert_eq!(inbound[0]["relation"], serde_json::json!("informs")); +} + +/// The human path must not disagree with the JSON path — the same question +/// answered two ways is how a reader and an agent end up with different +/// pictures of the same artifact. +#[test] +fn human_output_reports_the_same_edges_as_json() { + let ws = TempDir::new().unwrap(); + init(&ws); + let prd = new_artifact(&ws, "prd", "Parent"); + let spec = new_artifact(&ws, "spec", "Child"); + fpl(&ws) + .args(["link", &spec, &prd, "--relation", "refines"]) + .output() + .expect("link"); + + let out = fpl(&ws).args(["get", &spec]).output().expect("get"); + let stdout = String::from_utf8_lossy(&out.stdout); + assert!( + stdout.contains("Links out") && stdout.contains(&prd), + "human output must name the edge, got:\n{stdout}" + ); + + let orphan = new_artifact(&ws, "note", "Orphan"); + let out = fpl(&ws).args(["get", &orphan]).output().expect("get"); + let stdout = String::from_utf8_lossy(&out.stdout); + assert!( + stdout.contains("Links") && stdout.contains("none"), + "an unlinked artifact must say so rather than omit the row, got:\n{stdout}" + ); +} 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/anomalies.rs b/crates/forgeplan-core/src/anomalies.rs index 48005a64..195ced3d 100644 --- a/crates/forgeplan-core/src/anomalies.rs +++ b/crates/forgeplan-core/src/anomalies.rs @@ -727,6 +727,20 @@ pub async fn detect_anomalies( .iter() .map(|r| (r.id.as_str(), r.r_eff_score)) .collect(); + // PRD-086 FR-006 (#393). The walk below used to follow every `based_on` / + // `informs` edge, while `r_eff_recursive` skips non-active dependencies + // (ADR-002) and, since PRD-086 FR-003, ceremony-free kinds. Two walks with + // different rules disagree about which ancestor is the weakest link — the + // reporter saw the scorer resolve `NOTE-003` while the detector gave up on + // the same artifact. Same rules, same answer. + let skip_as_dependency: HashSet<&str> = all_records + .iter() + .filter(|r| { + matches!(r.status.as_str(), "draft" | "deprecated" | "superseded") + || matches!(r.kind.as_str(), "note" | "memory") + }) + .map(|r| r.id.as_str()) + .collect(); const WEAKEST_LINK_MAX_DEPTH: usize = 16; for r in &all_records { // weakest_link_unresolvable diagnoses DECISION artifacts whose evidence @@ -765,17 +779,48 @@ pub async fn detect_anomalies( initial_parents.iter().map(|p| (*p, 1usize)).collect(); let mut weakest_link: Option<&str> = None; let mut chain_depth: usize = 0; + // #393 bug 2: every give-up was reported as "cycle or depth cap", in a + // graph with zero cycles — 133 anomalies sending maintainers to hunt + // for something that was not there. Record which actually happened. + // + // `hit_revisit` deliberately does NOT claim a cycle. The push guard + // checks `visited`, not frontier membership, so one node can be queued + // twice on a perfectly acyclic graph: C has parents [D, E] and E also + // points at D. Calling that "the chain loops" would repeat the very + // mistake this hunk fixes, narrowed instead of removed. The wording + // below states what was observed — the same artifact reached more than + // once — and leaves the cause to the reader, who can run + // `forgeplan blocked` to see whether real cycles exist. + let mut hit_depth_cap = false; + let mut hit_revisit = false; while let Some((node, depth)) = frontier.pop() { if !visited.insert(node) { + hit_revisit = true; continue; } if depth > WEAKEST_LINK_MAX_DEPTH { + hit_depth_cap = true; + continue; + } + if skip_as_dependency.contains(node) { continue; } // If this node's r_eff is 0, it's a candidate weakest link // — but check parents to ensure it's not just transitively // inheriting from a deeper artifact. let node_r_eff = r_eff_by_id.get(node).copied().unwrap_or(0.0); + // PRD-086 FR-006, second half. Filtering only at pop time was not + // enough: the acceptance test below asks whether every parent has + // been visited, and a skipped parent never gets visited — it is + // dropped when popped. So a node that IS the weakest link but sits + // behind a note failed the test, was never named, and the walk + // reported "the cause is local" while `forgeplan score` resolved + // the same chain and named the artifact. Two walks disagreeing + // again, which is the defect FR-006 exists to close. + // + // Applying the skip at collection time makes a skipped parent + // invisible to both the acceptance test and the frontier, which is + // exactly what "the scorer does not walk through this edge" means. let parents: Vec<&str> = outgoing .get(node) .map(|edges| { @@ -783,6 +828,7 @@ pub async fn detect_anomalies( .iter() .filter(|(_, rel)| matches!(*rel, "based_on" | "informs")) .map(|(t, _)| *t) + .filter(|t| !skip_as_dependency.contains(t)) .collect() }) .unwrap_or_default(); @@ -821,18 +867,43 @@ pub async fn detect_anomalies( observed_at: now_str.clone(), description: match (&weakest_link_owned, chain_depth) { (Some(wl), d) => format!( - "{}: active with R_eff=0; weakest link in chain = {wl} (depth {d})", - r.id - ), - (None, _) => format!( - "{}: active with R_eff=0; ancestor walk could not identify source (cycle or depth cap)", - r.id + "{}: active with cached R_eff={:.2}; weakest link in chain = {wl} (depth {d})", + r.id, r.r_eff_score ), + (None, _) => { + let why = match (hit_depth_cap, hit_revisit) { + (true, _) => format!( + "walk stopped at the depth cap ({WEAKEST_LINK_MAX_DEPTH} hops)" + ), + (false, true) => { + "walk reached the same artifact by more than one path; \ + run `forgeplan blocked` to check for real cycles" + .to_string() + } + (false, false) => { + "every ancestor is skipped or scores above zero — the cause is local" + .to_string() + } + }; + format!( + "{}: active with cached R_eff={:.2}; no weakest link named — {why}", + r.id, r.r_eff_score + ) + } }, + // #393 bug 1: this was the literal `0.0`. The filter above only + // admits artifacts whose cached score is zero, so the literal was + // accidentally true — and would have started lying the moment the + // filter changed. It is also named `r_eff_cached` now, because the + // reporter's confusion came from comparing it against a fresh + // `forgeplan_score` run: the detector reads the stored column, and + // a stale column is a different number, not a wrong one. evidence: serde_json::json!({ - "r_eff": 0.0, + "r_eff_cached": r.r_eff_score, "weakest_link": weakest_link_owned, "chain_depth": chain_depth, + "walk_hit_depth_cap": hit_depth_cap, + "walk_hit_revisit": hit_revisit, }), suggested_resolution: Some(SuggestedResolution { tier: Tier::Adi, diff --git a/crates/forgeplan-core/src/db/store.rs b/crates/forgeplan-core/src/db/store.rs index 5796844a..ea434cc8 100644 --- a/crates/forgeplan-core/src/db/store.rs +++ b/crates/forgeplan-core/src/db/store.rs @@ -1369,6 +1369,50 @@ impl LanceStore { .await } + /// The record's frontmatter map **with its real links merged in**. + /// + /// #446. [`ArtifactRecord::frontmatter_map`] rebuilds a frontmatter from + /// the record's columns, and the record has no link columns — so the map + /// it returns never carries a `links` key. Every rule that compares + /// something against `extract_frontmatter_link_targets(fm)` was therefore + /// comparing against an empty set, and `body-links-drift` fired on every + /// artifact whose `## Related Artifacts` table named anything at all, + /// including targets that were correctly linked. The warning could not be + /// silenced by doing the right thing, only by deleting the table. + /// + /// Links come from the relations table — the same source `forgeplan graph` + /// renders and `forgeplan link` writes — so the warning now agrees with + /// what the user can check by eye. + /// + /// A failed relations read degrades to the link-less map rather than + /// failing validation outright; that restores the old behaviour for that + /// one record instead of refusing to validate it. + pub async fn frontmatter_map_with_links( + &self, + record: &ArtifactRecord, + ) -> BTreeMap { + use serde_yaml::{Mapping, Value}; + + let mut map = record.frontmatter_map(); + let Ok(relations) = self.get_relations(&record.id).await else { + return map; + }; + if relations.is_empty() { + return map; + } + let seq: Vec = relations + .into_iter() + .map(|(target, relation)| { + let mut entry = Mapping::new(); + entry.insert(Value::String("target".into()), Value::String(target)); + entry.insert(Value::String("relation".into()), Value::String(relation)); + Value::Mapping(entry) + }) + .collect(); + map.insert("links".to_string(), Value::Sequence(seq)); + map + } + /// Get incoming relations where this artifact is the TARGET. /// Returns Vec<(source_id, relation_type)>. /// @@ -3607,6 +3651,93 @@ mod tests { assert!(!map.contains_key("tags")); } + /// #446 — the defect in one assertion: `frontmatter_map()` cannot carry + /// links, so anything comparing against it sees an artifact with five + /// edges as an artifact with none. + #[tokio::test] + async fn frontmatter_map_alone_never_carries_links() { + let tmp = TempDir::new().unwrap(); + let store = make_store(&tmp).await; + store + .add_relation_for_test("SPEC-003", "PRD-065", "refines") + .await + .unwrap(); + + let record = record_for_links_test("SPEC-003"); + + assert!( + !record.frontmatter_map().contains_key("links"), + "the record has no link columns, so the reconstruction cannot have them" + ); + assert!( + store + .frontmatter_map_with_links(&record) + .await + .contains_key("links"), + "the store knows the edge and must merge it in" + ); + } + + /// #446 — the merged map must be in the exact shape + /// `extract_frontmatter_link_targets` reads, otherwise the rule still sees + /// an empty set and the fix is cosmetic. + #[tokio::test] + async fn merged_links_are_readable_by_the_rule_that_needs_them() { + let tmp = TempDir::new().unwrap(); + let store = make_store(&tmp).await; + for (target, relation) in [ + ("PRD-065", "refines"), + ("ADR-009", "based_on"), + ("SPEC-004", "informs"), + ] { + store + .add_relation_for_test("SPEC-003", target, relation) + .await + .unwrap(); + } + + let map = store + .frontmatter_map_with_links(&record_for_links_test("SPEC-003")) + .await; + let mut targets = crate::validation::checks::extract_frontmatter_link_targets(&map); + targets.sort(); + + assert_eq!(targets, vec!["ADR-009", "PRD-065", "SPEC-004"]); + } + + /// An artifact with no edges keeps the link-less map — the merge must not + /// invent an empty `links` key that reads as "checked and empty" when the + /// relations read never happened. + #[tokio::test] + async fn no_relations_leaves_the_map_untouched() { + let tmp = TempDir::new().unwrap(); + let store = make_store(&tmp).await; + let map = store + .frontmatter_map_with_links(&record_for_links_test("PRD-777")) + .await; + assert!(!map.contains_key("links")); + } + + fn record_for_links_test(id: &str) -> ArtifactRecord { + ArtifactRecord { + id: id.to_string(), + kind: "spec".to_string(), + status: "draft".to_string(), + title: "Links".to_string(), + body: "body".to_string(), + depth: "standard".to_string(), + author: None, + parent_epic: None, + r_eff_score: 0.0, + valid_until: None, + created_at: "2026-01-01T00:00:00Z".to_string(), + updated_at: "2026-01-01T00:00:00Z".to_string(), + tags: Vec::new(), + body_hash: None, + embedding: None, + } + } + #[tokio::test] async fn roundtrip_artifact_with_tags_through_frontmatter_map() { let record = ArtifactRecord { diff --git a/crates/forgeplan-core/src/hints.rs b/crates/forgeplan-core/src/hints.rs index ab2b0ea9..2b63f8b7 100644 --- a/crates/forgeplan-core/src/hints.rs +++ b/crates/forgeplan-core/src/hints.rs @@ -135,7 +135,20 @@ pub fn score_hints( ) -> Vec { let mut hints = Vec::new(); - if !has_evidence { + // PRD-086 FR-001. This warning states a number, and after leaf EvidencePacks + // began scoring on their own structured fields it started stating a false + // one: `forgeplan score` on a canonical pack printed "R_eff = 1.00" and then + // this line underneath, claiming the score "will be 0.0". + // + // Same defect class as the display branch in `score.rs` — an assertion keyed + // on a condition ("nothing linked") that used to have exactly one cause and + // now has two. The display was fixed; this was missed because no test reads + // the hint text. Found by running the binary, not by the suite. + // + // Guarding on the score rather than on the artifact kind keeps the hint + // honest for every future case where a nonzero score arrives without linked + // children, without this function needing to know what kinds exist. + if !has_evidence && r_eff <= 0.0 { hints.push( Hint::warning("No evidence linked — R_eff will be 0.0") .with_action(format!( @@ -405,6 +418,35 @@ mod tests { assert!(!action.contains("")); } + #[test] + fn score_hints_does_not_promise_zero_for_a_self_scoring_leaf() { + // PRD-086 FR-001. A leaf EvidencePack has no linked children by + // definition and now scores on its own structured fields, so + // `has_evidence: false` no longer implies a zero. The warning states a + // specific number, and stating it here would contradict the score + // printed two lines above it in `forgeplan score`. + let hints = score_hints("EVID-001", 1.0, false, 0); + assert!( + !hints + .iter() + .any(|h| h.message.contains("R_eff will be 0.0")), + "a nonzero score must not carry a warning promising 0.0: {hints:?}" + ); + } + + #[test] + fn score_hints_still_warns_when_the_score_really_is_zero() { + // The guard must not silence the case the warning exists for: an + // ordinary artifact with nothing linked. + let hints = score_hints("PRD-001", 0.0, false, 0); + assert!( + hints + .iter() + .any(|h| h.message.contains("R_eff will be 0.0")), + "an unevidenced artifact at zero must still be warned: {hints:?}" + ); + } + #[test] fn score_hints_cl0() { let hints = score_hints("PRD-002", 0.7, true, 2); diff --git a/crates/forgeplan-core/src/lifecycle/mod.rs b/crates/forgeplan-core/src/lifecycle/mod.rs index 02192aed..7e81012e 100644 --- a/crates/forgeplan-core/src/lifecycle/mod.rs +++ b/crates/forgeplan-core/src/lifecycle/mod.rs @@ -191,8 +191,21 @@ pub async fn collect_activation_gates( let stub_check = crate::validation::rules::check_stub(&record.body, &record.frontmatter_map()); let stub_ok = stub_check.is_none(); let stub_msg = stub_check.map(|msg| { + // PROB-105. The remediation used to name Problem / Goals / FR for every + // kind — sections a SPEC does not have and an ADR does not have either. + // Until this release the SPEC case was unreachable (the gate could not + // see a SPEC at all), so the wrong advice was never printed; now that it + // can fire, telling a spec author to fill in a Problem section would send + // them looking for something that is not in their template. + let sections = match record.kind.to_ascii_lowercase().as_str() { + "spec" => "Summary, a contract section (API Contracts / Data Models / Requirements)", + "adr" => "Context, Decision, Consequences", + "rfc" => "Summary, Motivation, Options Considered, Proposed Direction", + "epic" => "Vision, Goals, Children", + _ => "Problem, Goals, Functional Requirements", + }; format!( - "{msg} → Fill MUST sections (Problem, Goals, FR) before activating. \ + "{msg} → Replace the template placeholders and fill {sections} before activating. \ See PRD-043 FR-003 for stub detection rules." ) }); @@ -219,7 +232,8 @@ pub async fn review(store: &LanceStore, artifact_id: &str) -> anyhow::Result = validation_result .findings diff --git a/crates/forgeplan-core/src/phase/mod.rs b/crates/forgeplan-core/src/phase/mod.rs index de3df9b8..b21ec9bc 100644 --- a/crates/forgeplan-core/src/phase/mod.rs +++ b/crates/forgeplan-core/src/phase/mod.rs @@ -108,6 +108,26 @@ pub enum Phase { } impl Phase { + /// Position in the canonical ladder, for monotonicity checks (#330). + /// + /// `Unknown` sits below everything: an artifact whose phase was never + /// tracked may advance to any real phase. Every other pair compares by + /// ladder position, so "is this a step backwards" has one answer rather + /// than one per call site. + pub fn rank(self) -> u8 { + match self { + Phase::Unknown => 0, + Phase::Shape => 1, + Phase::Validate => 2, + Phase::Adi => 3, + Phase::Code => 4, + Phase::Test => 5, + Phase::Audit => 6, + Phase::Evidence => 7, + Phase::Done => 8, + } + } + /// Canonical string name (snake_case, matches serde output). pub fn as_str(self) -> &'static str { match self { diff --git a/crates/forgeplan-core/src/phase/store.rs b/crates/forgeplan-core/src/phase/store.rs index c34c8871..606634a7 100644 --- a/crates/forgeplan-core/src/phase/store.rs +++ b/crates/forgeplan-core/src/phase/store.rs @@ -281,6 +281,31 @@ pub async fn advance_phase( artifact_id: &str, to: Phase, reason: Option, +) -> anyhow::Result { + advance_phase_inner(workspace, artifact_id, to, reason, false).await +} + +/// Move the phase anywhere, including backwards — for deliberate correction. +/// +/// #330. The checked [`advance_phase`] refuses a step down the ladder, because +/// its own name promises it will not take one. An operator running +/// `forgeplan phase-advance --to shape` on an artifact that reached `done` is +/// making a decision, not tripping over automation, and that path stays open. +pub async fn advance_phase_unchecked( + workspace: &Path, + artifact_id: &str, + to: Phase, + reason: Option, +) -> anyhow::Result { + advance_phase_inner(workspace, artifact_id, to, reason, true).await +} + +async fn advance_phase_inner( + workspace: &Path, + artifact_id: &str, + to: Phase, + reason: Option, + allow_regression: bool, ) -> anyhow::Result { validate_artifact_id(artifact_id)?; @@ -295,6 +320,27 @@ pub async fn advance_phase( }; let from = state.current_phase; + + // #330. There was no monotonicity guard at all, and MCP `forgeplan_validate` + // calls this with `Phase::Validate` on every PASS — so validating an + // artifact that had already reached `done` walked it back to `validate`, + // and `forgeplan_health` then reported a phase mismatch the artifact did + // not have until someone checked it. A function named `advance` that + // silently reverses is the same defect class as a gate that reports a + // result it never computed. + // + // Refusing leaves state untouched and tells the caller why; the auto-advance + // path in MCP treats phase tracking as advisory and logs the refusal without + // failing the tool call. + if !allow_regression && to.rank() < from.rank() { + anyhow::bail!( + "Phase for {artifact_id} is already `{}`; refusing to move back to `{}`\nFix: forgeplan phase-advance {artifact_id} --to {}", + from.as_str(), + to.as_str(), + to.as_str() + ); + } + // Skip recording a no-op transition (e.g. double-call of auto-advance). if from == to { if was_missing { @@ -527,6 +573,73 @@ mod tests { assert_eq!(still, "sensitive"); } + /// #330. MCP `forgeplan_validate` auto-advances to `Validate` on every + /// PASS. Run against an artifact that already reached `done`, that walked + /// the phase backwards and made `forgeplan_health` report a mismatch the + /// artifact did not have until someone validated it. + #[tokio::test] + async fn validation_of_a_finished_artifact_does_not_walk_the_phase_back() { + let tmp = TempDir::new().unwrap(); + let ws = ws(&tmp); + initialize_phase(&ws, "PRD-MONO", None).await.unwrap(); + advance_phase_unchecked(&ws, "PRD-MONO", Phase::Done, None) + .await + .unwrap(); + + let err = advance_phase(&ws, "PRD-MONO", Phase::Validate, Some("auto".into())) + .await + .unwrap_err(); + assert!( + err.to_string().contains("refusing to move back"), + "the refusal must say what it refused, got: {err}" + ); + + let after = read_phase(&ws, "PRD-MONO").await.unwrap().unwrap(); + assert_eq!( + after.current_phase, + Phase::Done, + "a refused transition must leave state untouched" + ); + } + + /// Forward motion and re-stating the current phase both stay legal — the + /// guard must not turn into a tripwire on the normal path. + #[tokio::test] + async fn forward_and_no_op_transitions_are_unaffected() { + let tmp = TempDir::new().unwrap(); + let ws = ws(&tmp); + initialize_phase(&ws, "PRD-FWD", None).await.unwrap(); + + advance_phase(&ws, "PRD-FWD", Phase::Code, None) + .await + .expect("forward jump is allowed"); + advance_phase(&ws, "PRD-FWD", Phase::Code, None) + .await + .expect("re-stating the current phase is a no-op, not a regression"); + + let s = read_phase(&ws, "PRD-FWD").await.unwrap().unwrap(); + assert_eq!(s.current_phase, Phase::Code); + } + + /// The deliberate operator path keeps both directions — that is the whole + /// reason the guard lives on `advance_phase` and not inside `write_phase`. + #[tokio::test] + async fn the_explicit_path_may_still_correct_a_phase_downward() { + let tmp = TempDir::new().unwrap(); + let ws = ws(&tmp); + initialize_phase(&ws, "PRD-FIX", None).await.unwrap(); + advance_phase_unchecked(&ws, "PRD-FIX", Phase::Done, None) + .await + .unwrap(); + + advance_phase_unchecked(&ws, "PRD-FIX", Phase::Shape, Some("mis-marked".into())) + .await + .expect("an operator correcting a mistake is not a regression"); + + let s = read_phase(&ws, "PRD-FIX").await.unwrap().unwrap(); + assert_eq!(s.current_phase, Phase::Shape); + } + #[tokio::test] async fn history_is_capped_fifo() { // Audit Round 1 H1: runaway loop must not balloon history. @@ -537,7 +650,12 @@ mod tests { for i in 0..(MAX_HISTORY_ENTRIES + 100) { let p = if i % 2 == 0 { Phase::Code } else { Phase::Test }; - advance_phase(&ws, "PRD-CAP", p, None).await.unwrap(); + // #330: this oscillates on purpose to exercise the FIFO cap, which + // is a different question from whether automation may reverse a + // phase. The unchecked entry point is the honest one here. + advance_phase_unchecked(&ws, "PRD-CAP", p, None) + .await + .unwrap(); } let s = read_phase(&ws, "PRD-CAP").await.unwrap().unwrap(); assert!( @@ -609,7 +727,13 @@ mod tests { let ws = ws.clone(); let phase = if i % 2 == 0 { Phase::Code } else { Phase::Test }; tasks.push(tokio::spawn(async move { - advance_phase(&ws, "PRD-CC", phase, Some(format!("concurrent-{i}"))).await + // #330: the question here is tmp-filename collision under + // concurrent writes, not whether a phase may move backwards. + // The oscillation between Code and Test is just a way to make + // every task write; with the monotonicity guard in place the + // losers of that race are refused, which would turn a + // write-safety test into an ordering test by accident. + advance_phase_unchecked(&ws, "PRD-CC", phase, Some(format!("concurrent-{i}"))).await })); } let results = join_all(tasks).await; @@ -621,6 +745,34 @@ mod tests { assert!(matches!(s.current_phase, Phase::Code | Phase::Test)); } + /// #330, the half the rewrite above would otherwise have dropped: under + /// concurrency the CHECKED entry point must refuse cleanly rather than + /// corrupt state or panic. Sixteen tasks all advancing forward to the same + /// phase — every one is either a real transition or a no-op, none is a + /// regression, so all must succeed and the state must land exactly there. + #[tokio::test] + async fn concurrent_checked_advances_are_safe() { + use futures::future::join_all; + let tmp = TempDir::new().unwrap(); + let ws = ws(&tmp); + initialize_phase(&ws, "PRD-CCC", None).await.unwrap(); + + let mut tasks = Vec::new(); + for i in 0..16 { + let ws = ws.clone(); + tasks.push(tokio::spawn(async move { + advance_phase(&ws, "PRD-CCC", Phase::Code, Some(format!("fwd-{i}"))).await + })); + } + for r in join_all(tasks).await { + r.expect("task panicked") + .expect("a forward transition must never be refused"); + } + + let s = read_phase(&ws, "PRD-CCC").await.unwrap().unwrap(); + assert_eq!(s.current_phase, Phase::Code); + } + #[tokio::test] async fn corrupt_yaml_is_quarantined_not_clobbered() { // Audit Round 2 M-sec #3: corrupt state file should be renamed diff --git a/crates/forgeplan-core/src/projection/mod.rs b/crates/forgeplan-core/src/projection/mod.rs index 224bdaa4..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). @@ -951,33 +1007,56 @@ pub async fn update_metadata_with_projection( Ok(()) } -/// ADR-012 / SPEC-005 identity fields. -/// -/// Under the PRD-073 two-block layout these live in the **body's** own -/// frontmatter, which makes them collateral damage of any body replacement — -/// see [`carry_identity_forward`]. -const IDENTITY_FM_KEYS: &[&str] = &["slug", "predicted_number", "assigned_number"]; - -/// Re-attach the ADR-012 identity fields from `old_body` when `new_body` does -/// not carry them. +/// Re-attach body-frontmatter fields from `old_body` that `new_body` does not +/// carry and that the projection does not regenerate. /// -/// The identity triple lives inside the body's frontmatter block (PRD-073 -/// layout: synthetic projection block on top, canonical body below). A body -/// replacement is therefore an *identity deletion* unless the fields are +/// The body has its own frontmatter block (PRD-073 layout: synthetic +/// projection block on top, canonical body below). A body replacement is +/// therefore a *field deletion* for everything in that block unless it is /// carried across — which is why a mature workspace ends up with no artifact /// carrying a slug at all: `forgeplan new` writes one, and the very next /// `update --body` that CLAUDE.md prescribes silently drops it. /// -/// Callers supply prose. Re-attaching here keeps every call site from having to -/// remember, and is a no-op when the caller *did* supply identity (round-trip of -/// a full document) or when the artifact never had one (legacy, pre-Phase-1.5). +/// ## Why the rule is `KNOWN_FM_KEYS`, not a hand-listed triple +/// +/// This carried exactly `["slug", "predicted_number", "assigned_number"]` — +/// the three fields whose loss someone noticed. The argument that justified +/// carrying those three applies verbatim to every other field in the block, +/// so the list was a fix for three instances of a class, and everything +/// outside it kept being deleted in silence. +/// +/// Measured on a real workspace: a no-op round-trip of RFC-022 (read the body +/// back with `forgeplan get --json`, hand it straight to `update --body` +/// unchanged) dropped eight lines. Five of them — `author`, `depth`, `id`, +/// `status`, `title` — are in [`KNOWN_FM_KEYS`] and are legitimately +/// regenerated into the projection block above, so their in-body copies are +/// redundant. Three were pure loss: `created`, `updated`, `prd`. +/// +/// So the rule is the one the neighbouring [`filter_preserved`] already +/// applies to the projection block's own unknown keys (PRD-057 FR-009): +/// **anything the record does not own survives**. Deriving it from +/// `KNOWN_FM_KEYS` rather than from a second authored list means a field +/// added to the record is automatically excluded, and a field added to the +/// body is automatically preserved — neither needs anyone to remember this +/// function exists. Carrying a `KNOWN_FM_KEYS` field would be worse than +/// dropping it: `update_body_with_projection` parses `status` back out of the +/// new body to sync LanceDB, so a stale in-body copy could resurrect a +/// superseded status. +/// +/// Consequence, stated rather than discovered later: a caller cannot delete a +/// body-frontmatter field by omitting it. That is deliberate — silent data +/// loss is the failure this function exists to prevent, and it is the more +/// expensive of the two. Removing a field means writing the block without it +/// AND having the record not own it; for the record-owned ones there are +/// dedicated mutators (`update --status`, `--title`, `--depth`). fn carry_identity_forward(old_body: &str, new_body: &str) -> String { let Ok((old_fm, _)) = frontmatter::parse_frontmatter(old_body) else { return new_body.to_string(); }; - let carried: Vec<(&str, serde_yaml::Value)> = IDENTITY_FM_KEYS + let carried: Vec<(&str, serde_yaml::Value)> = old_fm .iter() - .filter_map(|k| old_fm.get(*k).map(|v| (*k, v.clone()))) + .filter(|(k, _)| !KNOWN_FM_KEYS.contains(&k.as_str())) + .map(|(k, v)| (k.as_str(), v.clone())) .collect(); if carried.is_empty() { return new_body.to_string(); @@ -1043,9 +1122,11 @@ pub async fn update_body_with_projection( }; let links = store.get_relations(id).await.unwrap_or_default(); - // PROB-060 / ADR-012: preserve the identity triple the replacement would - // otherwise delete. Must happen before `derived_status` is parsed and - // before either write, so file and index agree on one body. + // PROB-060 / ADR-012: preserve the body-frontmatter fields the replacement + // would otherwise delete — every field the record does not own, not just + // the identity triple this originally carried. Must happen before + // `derived_status` is parsed and before either write, so file and index + // agree on one body. let carried = carry_identity_forward(&record.body, body); let body: &str = &carried; @@ -2290,6 +2371,69 @@ mod tests { assert_eq!(carry_identity_forward("no frontmatter here\n", new), new); } + /// The identity triple was three instances of a class, and the rest of the + /// class kept being deleted in silence. + /// + /// Measured on a real workspace before this was fixed: reading RFC-022's + /// body back and handing it to `update --body` UNCHANGED — a no-op — cost + /// eight frontmatter lines. Five (`author`, `depth`, `id`, `status`, + /// `title`) are in `KNOWN_FM_KEYS` and are regenerated into the projection + /// block above, so losing the in-body copy is correct. Three were pure + /// loss, and this pins them. + #[test] + fn carry_identity_forward_keeps_every_field_the_record_does_not_own() { + // The second block of a generated artifact, as `forgeplan new` writes + // it. `created`/`updated`/`prd` exist ONLY here — no projection field + // regenerates them. + let old = "---\nassigned_number: 22\nauthor: null\ncreated: 2026-09-03\n\ + depth: standard\nid: RFC-022\nprd: null\npredicted_number: 22\n\ + slug: rfc-production-program\nstatus: Draft\ntitle: 'Production program'\n\ + updated: 2026-09-03\n---\n\n## Summary\n\nold prose\n"; + + let out = carry_identity_forward(old, "## Summary\n\nnew prose\n"); + + // Population before the claim: the call produced frontmatter at all. + // Without this a `!out.contains(...)` pair below would hold for an + // empty string. + assert!( + out.starts_with("---\n"), + "expected a frontmatter block, got:\n{out}" + ); + + for expected in [ + "created: 2026-09-03", + "updated: 2026-09-03", + "prd: null", + "slug: rfc-production-program", + "predicted_number: 22", + "assigned_number: 22", + ] { + assert!( + out.contains(expected), + "`{expected}` was dropped — it is not a record-owned field:\n{out}" + ); + } + + // The other direction, and it is the half that keeps this from being + // "carry everything": a record-owned field must NOT be carried. Its + // authoritative copy is regenerated into the projection block, and + // `update_body_with_projection` parses `status` back out of the new + // body to sync LanceDB — a stale in-body copy could resurrect a + // superseded status. + for owned in ["status:", "title:", "depth:", "id:", "author:"] { + assert!( + !out.contains(owned), + "`{owned}` is in KNOWN_FM_KEYS and must not be carried:\n{out}" + ); + } + + assert!( + out.contains("new prose"), + "the new prose must be what lands" + ); + assert!(!out.contains("old prose"), "old prose must not resurrect"); + } + // ── #419 BLOCKER: same-slug title change must not delete the file ──── #[tokio::test] diff --git a/crates/forgeplan-core/src/scoring/evidence.rs b/crates/forgeplan-core/src/scoring/evidence.rs index 95e18e56..465ab203 100644 --- a/crates/forgeplan-core/src/scoring/evidence.rs +++ b/crates/forgeplan-core/src/scoring/evidence.rs @@ -55,14 +55,34 @@ impl SourceTier { /// PRD-035 Sprint 13.3 security audit H2 (a malicious contributor cannot /// inflate `R_eff` by tagging weak evidence as `source_tier: t1`). pub fn parse_evidence_from_record(record: &ArtifactRecord) -> EvidenceItem { - let verdict = extract_field(&record.body, "verdict") - .map(|s| match s.to_lowercase().as_str() { - "supports" => Verdict::Supports, - "weakens" => Verdict::Weakens, - "refutes" => Verdict::Refutes, - _ => Verdict::Supports, - }) - .unwrap_or(Verdict::Supports); + // PRD-086 FR-008. Both branches used to resolve to `Supports`, which scores + // 1.0 — an unreadable verdict and an absent one were rewarded with maximum + // trust, while `congruence_level` twenty lines below fails closed to CL0 + // with a warning for exactly the same input classes. One field punished + // garbage and its neighbour paid for it. + // + // Protocol v1 adds a fourth value (`verdict: unknown`) that the old parser + // would have scored 1.0 on arrival. + // + // Fail closed via CL0, which is exactly the outcome the docs already + // promise (`Supports` 1.0 minus the CL0 penalty 0.9 = 0.1). `Refutes` would + // be the wrong landing — it means "this evidence argues against the claim", + // a statement nobody made. Undeclared is not opposed; it is unsupported. + let verdict_raw = extract_field(&record.body, "verdict"); + let verdict_declared = match verdict_raw.as_deref().map(str::to_lowercase).as_deref() { + Some("supports") => Some(Verdict::Supports), + Some("weakens") => Some(Verdict::Weakens), + Some("refutes") => Some(Verdict::Refutes), + Some(other) => { + eprintln!( + "warn: evidence {} has unrecognised verdict='{}' — treated as undeclared (score 0.1) to prevent silent trust inflation", + record.id, other + ); + None + } + None => None, + }; + let verdict = verdict_declared.clone().unwrap_or(Verdict::Supports); let tier_cl = extract_field(&record.body, "source_tier") .and_then(|s| SourceTier::parse(&s)) @@ -114,11 +134,41 @@ pub fn parse_evidence_from_record(record: &ArtifactRecord) -> EvidenceItem { // Precedence: take MIN of (tier_cl, explicit_cl). Explicit operator // downgrade can never be silently overridden by an automatic tier mapping. // Default CL=3 (same context) — evidence created locally is same-context by default. - let cl = match (tier_cl, explicit_cl) { - (Some(t), Some(e)) => t.min(e), - (Some(t), None) => t, - (None, Some(e)) => e, - (None, None) => 3, + // PRD-086 FR-009. `(None, None)` used to mean CL3 — maximum trust for a + // pack that declared nothing at all. Three documents state the opposite in + // the same words: CLAUDE.md RED LINE #7, the `/forge` skill that + // `setup-skill` ships to every user, and EVIDENCE-PROTOCOL.md all say + // absent fields mean CL0 and a score of 0.1. Measured before this change: + // a body of pure prose scored its target 1.00 "Adequate". + // + // The rationale for the old default was that locally-authored evidence is + // same-context by construction. True, and beside the point — congruence is + // not the only thing a missing field withholds. A pack that says nothing + // has made no claim to be congruent WITH. + // + // The incentive was also backwards: writing `congruence_level: 2` honestly + // scored 0.9, writing nothing scored 1.0. Skipping the discipline paid + // better than following it. + // + // Verdict participates in the same gate. A pack with `congruence_level: 3` + // and no verdict still made no claim about direction, and the old code + // defaulted that to `Supports`. + let claim_declared = verdict_declared.is_some() && (tier_cl.is_some() || explicit_cl.is_some()); + let cl = if !claim_declared { + if verdict_raw.is_none() && explicit_cl_raw.is_none() && tier_cl.is_none() { + eprintln!( + "warn: evidence {} declares no structured fields — scored CL0 (0.1). Add `verdict:` and `congruence_level:` under `## Structured Fields`", + record.id + ); + } + 0 + } else { + match (tier_cl, explicit_cl) { + (Some(t), Some(e)) => t.min(e), + (Some(t), None) => t, + (None, Some(e)) => e, + (None, None) => unreachable!("claim_declared guarantees one CL source"), + } }; let valid_until = record.valid_until.as_deref().and_then(|s| { @@ -474,21 +524,21 @@ congruence_level: 3 #[test] fn evidence_body_with_source_tier_maps_to_cl() { - let body = "source_tier: t2\nevidence_type: test\n"; + let body = "verdict: supports\nsource_tier: t2\nevidence_type: test\n"; let item = parse_evidence_from_record(&mk_record(body)); assert_eq!(item.congruence_level, 2); } #[test] fn evidence_body_source_tier_t1_maps_to_cl3() { - let body = "source_tier: tier1\n"; + let body = "verdict: supports\nsource_tier: tier1\n"; let item = parse_evidence_from_record(&mk_record(body)); assert_eq!(item.congruence_level, 3); } #[test] fn evidence_body_source_tier_t3_maps_to_cl1() { - let body = "source_tier: 3\n"; + let body = "verdict: supports\nsource_tier: 3\n"; let item = parse_evidence_from_record(&mk_record(body)); assert_eq!(item.congruence_level, 1); } @@ -507,35 +557,69 @@ congruence_level: 3 #[test] fn explicit_cl_does_not_inflate_above_source_tier() { // source_tier=t3 (CL1) + congruence_level=3 → min = 1 - let body = "source_tier: t3\ncongruence_level: 3\n"; + let body = "verdict: supports\nsource_tier: t3\ncongruence_level: 3\n"; let item = parse_evidence_from_record(&mk_record(body)); assert_eq!(item.congruence_level, 1); } #[test] fn source_tier_used_when_no_explicit_cl() { - let body = "source_tier: t2\n"; + let body = "verdict: supports\nsource_tier: t2\n"; let item = parse_evidence_from_record(&mk_record(body)); assert_eq!(item.congruence_level, 2); } #[test] fn explicit_cl_used_when_no_source_tier() { - let body = "congruence_level: 2\n"; + let body = "verdict: supports\ncongruence_level: 2\n"; let item = parse_evidence_from_record(&mk_record(body)); assert_eq!(item.congruence_level, 2); } #[test] - fn neither_field_defaults_to_cl3() { + fn no_congruence_source_fails_closed_to_cl0() { + // PRD-086 FR-009. This test previously asserted CL3 — it defended the + // defect. A pack stating a verdict and no congruence has not said how + // close its context is to the claim's, and CLAUDE.md RED LINE #7, the + // `/forge` skill and EVIDENCE-PROTOCOL.md all specify CL0 for that. + // Renamed rather than edited in place so the old name cannot be found + // and trusted. let body = "verdict: supports\n"; let item = parse_evidence_from_record(&mk_record(body)); - assert_eq!(item.congruence_level, 3); + assert_eq!(item.congruence_level, 0); + } + + #[test] + fn a_body_with_no_structured_fields_scores_a_tenth_not_full_marks() { + // The measured symptom behind PROB-101: pure prose scored its target + // 1.00 "Adequate". The documented outcome is 0.1 — Supports (1.0) + // minus the CL0 penalty (0.9). + let item = parse_evidence_from_record(&mk_record("Just prose. Ran it. Fine.\n")); + assert_eq!(item.congruence_level, 0); + assert!( + (crate::scoring::reff::raw_evidence_score(&item) - 0.1).abs() < 1e-9, + "expected the documented 0.1, got {}", + crate::scoring::reff::raw_evidence_score(&item) + ); + } + + #[test] + fn an_unrecognised_verdict_does_not_score_as_support() { + // PRD-086 FR-008. `_ => Verdict::Supports` scored garbage at 1.0 while + // `congruence_level` in the same function failed closed for the same + // input class. Protocol v1's `verdict: unknown` would have arrived + // scoring full marks. + let item = + parse_evidence_from_record(&mk_record("verdict: unknown\ncongruence_level: 3\n")); + assert_eq!( + item.congruence_level, 0, + "an undeclared direction is not a CL3 claim" + ); } #[test] fn evidence_body_invalid_source_tier_falls_back_to_cl() { - let body = "source_tier: bogus\ncongruence_level: 2\n"; + let body = "verdict: supports\nsource_tier: bogus\ncongruence_level: 2\n"; let item = parse_evidence_from_record(&mk_record(body)); assert_eq!(item.congruence_level, 2); } diff --git a/crates/forgeplan-core/src/scoring/reff.rs b/crates/forgeplan-core/src/scoring/reff.rs index 3d40000b..9f667105 100644 --- a/crates/forgeplan-core/src/scoring/reff.rs +++ b/crates/forgeplan-core/src/scoring/reff.rs @@ -324,6 +324,52 @@ pub async fn r_eff_recursive( evidence_items.push(parse_evidence_from_record(rec)); } + // PRD-086 FR-001 (#325). An EvidencePack asked for its evidence finds none — + // a pack has no packs — and the empty-evidence branch below returns 0.0 with + // the factor "No evidence found (L0)". A canonical pack (`verdict: supports`, + // `congruence_level: 3`) scored zero, and the only way to lift it was to + // invent child evidence, which is graph pollution to satisfy a walk. + // + // The intrinsic score already exists. `score_evidence_full` is applied to + // this very pack whenever it is scored AS evidence for something else; it + // was simply never applied to the pack itself. Trust in a measurement comes + // from its verdict, congruence and freshness — not from someone having + // measured the measurement. + // + // Packs that carry child evidence keep the normal path: an EVID built on + // other EVIDs is a real chain and the weakest link still rules it. + let is_evidence_kind = store + .get_record(artifact_id) + .await + .ok() + .flatten() + .map(|r| r.kind.eq_ignore_ascii_case("evidence")) + .unwrap_or(false); + + if is_evidence_kind && evidence_items.is_empty() && terminal_skips == 0 { + let own = store.get_record(artifact_id).await.ok().flatten(); + if let Some(rec) = own { + let item = parse_evidence_from_record(&rec); + let intrinsic = score_evidence_full(&item); + factors.push(format!( + "Leaf evidence scored on its own fields: {:?} CL{} = {:.2}", + item.verdict, item.congruence_level, intrinsic + )); + return Ok(AssuranceReport { + artifact_id: artifact_id.to_string(), + r_eff: intrinsic, + self_score: intrinsic, + weakest_link: None, + decay_penalty: if is_expired(item.valid_until) { + 0.9 + } else { + 0.0 + }, + factors, + }); + } + } + let self_score = if evidence_items.is_empty() { if terminal_skips > 0 { // quint-code edge case (decision.go:826): all evidence displaced @@ -358,10 +404,17 @@ pub async fn r_eff_recursive( .copied() .collect(); - // Collect dependency IDs from outgoing relations. + // PRD-086 FR-002 (#325). An EvidencePack's outgoing `informs` / `based_on` + // edges point at the artifacts it SUPPORTS. Treating those as dependencies + // makes trust in the measurement flow down from the decision it justifies — + // backwards. A pack does not become less reliable because the PRD it + // informs is poorly evidenced elsewhere. let deps: Vec<(String, String)> = outgoing .iter() .filter(|(_, rel_type)| dep_relation_types.contains(rel_type.as_str())) + .filter(|(_, rel_type)| { + !(is_evidence_kind && matches!(rel_type.as_str(), "informs" | "based_on")) + }) .cloned() .collect(); @@ -370,14 +423,33 @@ pub async fn r_eff_recursive( for (dep_id, rel_type) in &deps { // Skip non-active dependencies — draft/deprecated/superseded should not drag down R_eff - if let Ok(Some(dep_record)) = store.get_record(dep_id).await - && matches!( + if let Ok(Some(dep_record)) = store.get_record(dep_id).await { + if matches!( dep_record.status.as_str(), "draft" | "deprecated" | "superseded" - ) - { - factors.push(format!("Skipped {dep_id} (status: {})", dep_record.status)); - continue; + ) { + factors.push(format!("Skipped {dep_id} (status: {})", dep_record.status)); + continue; + } + + // PRD-086 FR-003 (#392, narrowed). The routing table calls a Note + // the artifact for trivial, reversible work: no ADI, no evidence, + // expires in 90 days. The cascade then read an active Note with no + // evidence as zero trust and poisoned every chain based on it — so + // forgeplan said a Note needs no evidence and scored everything + // downstream of it as unevidenced. Two of its own rules disagreeing. + // + // Same remedy ADR-002 chose for `draft`: skip, logged, rather than + // soften the min. The weakest-link rule is untouched for every kind + // that CAN owe evidence — a PRD or ADR with none is real debt and + // the cascade surfacing it is the product working. + if matches!(dep_record.kind.as_str(), "note" | "memory") { + factors.push(format!( + "Skipped {dep_id} (kind: {} — exempt from evidence by routing depth)", + dep_record.kind + )); + continue; + } } let dep_report = match Box::pin(r_eff_recursive(dep_id, store, visited)).await { diff --git a/crates/forgeplan-core/src/validation/checks.rs b/crates/forgeplan-core/src/validation/checks.rs index 308a2b1b..c147e3f8 100644 --- a/crates/forgeplan-core/src/validation/checks.rs +++ b/crates/forgeplan-core/src/validation/checks.rs @@ -57,12 +57,102 @@ pub fn extract_related_artifacts_table_ids(body: &str) -> Vec { continue; } for m in ID_RE.find_iter(line) { - found.insert(m.as_str().to_string()); + let token = m.as_str(); + // #446 — the regex matches any `-` token, so it + // swept up requirement ids (`FR-1`), invariant numbers (`I-3`) and + // anything else shaped that way. Those can never be link targets, + // so the warning naming them was unclosable by construction, and + // its `Run: forgeplan link FR-1` remediation could not + // be executed — a PRD-071 violation of the same shape as #348 and + // #351. Keep only tokens whose prefix maps to a real artifact kind. + let Some((prefix, _)) = token.split_once('-') else { + continue; + }; + if crate::artifact::types::ArtifactKind::from_slug_prefix(&prefix.to_lowercase()) + .is_none() + { + continue; + } + found.insert(token.to_string()); } } found.into_iter().collect() } +/// Requirement headings in a SPEC body that carry no `#### Scenario` beneath +/// them — issue #450, narrowed to internal consistency. +/// +/// The issue asked for "every `### Requirement` has a `#### Scenario`" as a +/// blanket rule. Measured against this repository first: none of the six SPECs +/// uses either heading — they are API-contract shaped (`## Contract`, +/// `## Data Models`, `## Errors`), which the template prescribes. A blanket +/// rule would have fired on 6 of 6, none of them defective. That is the shape +/// of warning PRD-086 spent a week removing. +/// +/// So the question is asked conditionally instead: a spec that *opens* a +/// Requirement has committed to the behavioural form, and a Requirement with no +/// scenario is a promise with no oracle — the half-authored state that leaves +/// the downstream TDD flow with nothing to plan against. On a spec that never +/// uses the heading this returns empty and the rule is silent. +/// +/// Returns the requirement titles that lack a scenario, in document order. +pub fn requirements_without_scenarios(body: &str) -> Vec { + /// `### Requirement…` — the heading that opens a behavioural requirement. + fn requirement_title(line: &str) -> Option { + let rest = line.strip_prefix("### ")?; + let rest = rest.trim(); + rest.to_lowercase() + .starts_with("requirement") + .then(|| rest.to_string()) + } + + /// `#### Scenario…` — deeper than the requirement, so it belongs to it. + fn is_scenario(line: &str) -> bool { + line.strip_prefix("#### ") + .map(|rest| rest.trim().to_lowercase().starts_with("scenario")) + .unwrap_or(false) + } + + /// Any heading at `###` or shallower closes the requirement being read. + fn closes_requirement(line: &str) -> bool { + line.starts_with("# ") || line.starts_with("## ") || line.starts_with("### ") + } + + let stripped = strip_non_prose_for_leakage(body); + let mut missing = Vec::new(); + let mut open: Option = None; + let mut saw_scenario = false; + + let close = |open: &mut Option, saw: &mut bool, out: &mut Vec| { + if let Some(title) = open.take() + && !*saw + { + out.push(title); + } + *saw = false; + }; + + for line in stripped.lines() { + let trimmed = line.trim(); + + if let Some(title) = requirement_title(trimmed) { + close(&mut open, &mut saw_scenario, &mut missing); + open = Some(title); + continue; + } + if is_scenario(trimmed) { + saw_scenario = true; + continue; + } + if closes_requirement(trimmed) { + close(&mut open, &mut saw_scenario, &mut missing); + } + } + close(&mut open, &mut saw_scenario, &mut missing); + + missing +} + /// PROB-059 — extract `target` IDs от frontmatter `links:` array. pub fn extract_frontmatter_link_targets(fm: &Frontmatter) -> Vec { let Some(links_val) = fm.get("links") else { @@ -852,6 +942,55 @@ const VAGUE_QUANTIFIERS: &[&str] = &[ "numerous", ]; +/// Subjective adjectives inside the NFR section — issue #449. +/// +/// The blacklist this shares with [`check_measurability_adjectives`] reads like +/// a list of non-functional requirements: `scalable`, `robust`, `efficient`, +/// `responsive`, `fast`, `seamless`. It was only ever applied to the FR section. +/// Measured across the 69 PRDs in this repository: 2 hits in FR (checked), 15 in +/// NFR (unchecked). +/// +/// Those 15 are the reason this strips non-prose first. Every one of them sits +/// inside the PRD template's own HTML comment demonstrating what NOT to write — +/// ``. A rule that flagged +/// them would be unclosable: the only way to silence it is to delete the +/// template's guidance. [`check_measurability_adjectives`] does not strip, and +/// carries the same latent bug; it has simply never fired because the template's +/// BAD examples happen to live under the NFR heading. +/// +/// Returns (word, line number relative to body start). +pub fn check_nfr_measurability(body: &str) -> Vec<(String, usize)> { + static ADJECTIVE_REGEXES: LazyLock> = LazyLock::new(|| { + SUBJECTIVE_ADJECTIVES + .iter() + .filter_map(|word| { + let pattern = format!(r"(?i)\b{}\b", regex::escape(word)); + Regex::new(&pattern).ok().map(|re| (word.to_string(), re)) + }) + .collect() + }); + + let nfr_section = match extract_nfr_section(body) { + Some(s) => s, + None => return Vec::new(), + }; + // Line numbers survive the strip — comments are replaced with newlines. + let prose = strip_non_prose_for_leakage(&nfr_section); + + let nfr_start_offset = body.find(&nfr_section).unwrap_or(0); + let line_offset = body[..nfr_start_offset].lines().count(); + + let mut results = Vec::new(); + for (i, line) in prose.lines().enumerate() { + for (word, re) in ADJECTIVE_REGEXES.iter() { + if re.is_match(line) { + results.push((word.clone(), line_offset + i + 1)); + } + } + } + results +} + /// Check for subjective adjectives in FR/requirements sections. /// Returns vec of (found_word, line_number) — line numbers are relative to body start. pub fn check_measurability_adjectives(body: &str) -> Vec<(String, usize)> { @@ -1302,6 +1441,182 @@ mod tests { assert_eq!(ids, vec!["EVID-042", "PRD-001", "RFC-003"]); } + // ── issue #449 / #450: NFR measurability and Requirement/Scenario ────── + + /// #449. The 15 "hits" that made this rule look valuable in the corpus were + /// all inside the template's own `` guidance. Flagging them + /// would be unclosable — the only fix is deleting the instructions. + #[test] + fn nfr_adjectives_inside_html_comments_are_not_flagged() { + let body = "\ +## Non-Functional Requirements + + + + +- NFR-001: p95 latency < 200ms at 50 rps. +"; + assert!( + check_nfr_measurability(body).is_empty(), + "the template's own bad-example comment must not become a finding" + ); + } + + /// The other half: a genuinely vague NFR outside comments still fires, + /// otherwise the strip would have turned the rule into decoration. + #[test] + fn vague_nfr_prose_is_flagged() { + let body = "\ +## Non-Functional Requirements + +- NFR-001: The export is fast and the service is robust. +"; + let found: Vec = check_nfr_measurability(body) + .into_iter() + .map(|(w, _)| w) + .collect(); + assert!(found.contains(&"fast".to_string()), "got {found:?}"); + assert!(found.contains(&"robust".to_string()), "got {found:?}"); + } + + /// The section is optional; absence is `prd-nfr-exist`'s business, not this + /// rule's. Returning findings here would double-report. + #[test] + fn no_nfr_section_yields_no_measurability_findings() { + assert!( + check_nfr_measurability("## Functional Requirements\n\n- FR-001: fast.\n").is_empty() + ); + } + + /// #450. The blanket rule the issue asked for would fire on all six SPECs in + /// this repository, none of which is defective — they are API-contract + /// shaped. Silence on a spec that never opens a Requirement is the point. + #[test] + fn a_spec_without_requirement_headings_is_silent() { + let body = "\ +## Summary + +A contract. + +## API Contracts + +### Endpoint: `GET /v1/things` + +Returns things. +"; + assert!(requirements_without_scenarios(body).is_empty()); + } + + /// A requirement that opened and never delivered a scenario is the real + /// defect: a promise with no oracle. + #[test] + fn a_requirement_without_a_scenario_is_reported() { + let body = "\ +## Requirements + +### Requirement: Failing step halts the run + +The runner MUST stop. + +### Requirement: Report names the step + +#### Scenario: step two fails + +GIVEN three steps WHEN step two exits 1 THEN the report names step two +"; + let missing = requirements_without_scenarios(body); + assert_eq!(missing.len(), 1, "got {missing:?}"); + assert!(missing[0].contains("Failing step halts"), "got {missing:?}"); + } + + /// A `##` heading closes the requirement above it — a scenario appearing + /// later under a different section does not retroactively satisfy it. + #[test] + fn a_scenario_in_a_later_section_does_not_count() { + let body = "\ +## Requirements + +### Requirement: Unsatisfied + +## Appendix + +#### Scenario: unrelated + +GIVEN something +"; + assert_eq!(requirements_without_scenarios(body).len(), 1); + } + + /// Requirement blocks inside fenced examples are documentation, not + /// requirements — the same strip that saves the NFR rule saves this one. + #[test] + fn requirement_headings_inside_code_fences_are_ignored() { + let body = "\ +## Summary + +Write requirements like this: + +```markdown +### Requirement: Example with no scenario +``` +"; + assert!(requirements_without_scenarios(body).is_empty()); + } + + /// #446 — tokens shaped like an id but whose prefix is not an artifact + /// kind are not link candidates. `FR-1` is a requirement number and `I-3` + /// an invariant number; neither can ever be a link target, so naming them + /// produced a warning that could not be resolved by linking anything. + #[test] + fn extract_related_artifacts_table_ids_skips_non_artifact_tokens() { + let body = " +# SPEC-003: Title + +## Related Artifacts + +| Artifact | Type | Relation | +|---|---|---| +| PRD-065 | PRD | refines (contract for FR-1, FR-2, FR-3, FR-5) | +| ADR-009 | ADR | based_on (invariants I-1, I-3) | +"; + let ids = extract_related_artifacts_table_ids(body); + assert_eq!( + ids, + vec!["ADR-009", "PRD-065"], + "only real artifact kinds survive; FR-* and I-* are not link targets" + ); + } + + /// #446 guard — the filter must not become an allow-list that quietly + /// drops real kinds. Every kind prefix the slug parser accepts has to + /// survive extraction, otherwise the drift rule goes blind to that kind. + #[test] + fn extract_related_artifacts_table_ids_keeps_every_real_kind() { + let body = " +## Related Artifacts + +| Artifact | Relation | +|---|---| +| PRD-001 | refines | +| RFC-002 | informs | +| ADR-003 | based_on | +| EPIC-004 | belongs_to | +| SPEC-005 | refines | +| PROB-006 | informs | +| SOL-007 | informs | +| EVID-008 | informs | +| NOTE-009 | informs | +| REF-010 | informs | +| MEM-011 | informs | +"; + let ids = extract_related_artifacts_table_ids(body); + assert_eq!( + ids.len(), + 11, + "all real kinds must survive the #446 prefix filter, got {ids:?}" + ); + } + /// Free-text mention OUTSIDE the Related Artifacts section is NOT /// collected — strict parser by design (no false-flag on "see also"). #[test] diff --git a/crates/forgeplan-core/src/validation/rules.rs b/crates/forgeplan-core/src/validation/rules.rs index 506e649e..17fc6cef 100644 --- a/crates/forgeplan-core/src/validation/rules.rs +++ b/crates/forgeplan-core/src/validation/rules.rs @@ -165,9 +165,39 @@ pub fn check_stub_detailed(body: &str, _fm: &Frontmatter) -> Option // {placeholder} markers — single-brace curly placeholders like {name}. // Avoid false-positives on `{{var}}` (already covered by no-placeholders) // and on JSON/code by requiring word characters only inside the braces. - if PLACEHOLDER_RE.is_match(body) { + // + // PROB-105. This used to be a single +1 no matter how many placeholders a + // body carried, and that cap is why the gate could not see a SPEC. Its + // twelve phrase markers are all PRD prose ("Что мы строим и почему это + // важно", "[Actor] can [capability]"), none of which appears in the SPEC + // template — so an untouched SPEC scored exactly 1 against a threshold of + // 3, validated with zero findings, and activated at R_eff 1.00 while its + // body still read `{METHOD} /v1/{resource}`. + // + // A document that is mostly unfilled slots is a stub whatever kind it is, + // so the count scales. The second threshold is set from the corpus rather + // than taste: + // + // SPEC template, untouched 15 placeholders + // PRD template, untouched 5 + // SPEC-001 … SPEC-006 (real) 0–3 + // + // MANY_PLACEHOLDERS = 8 sits in the gap with room on both sides. A busy + // real spec would have to more than double its placeholder use before this + // fires, and a template cannot avoid it. + // + // Deliberately kind-agnostic: the question is "is this still a form to fill + // in", not "is this Gherkin or an API contract". A line-count test was + // measured and rejected — the untouched template has 25 non-empty lines + // under `## API Contracts`, because placeholder JSON is still lines. + const MANY_PLACEHOLDERS: usize = 8; + let placeholder_count = PLACEHOLDER_RE.find_iter(body).count(); + if placeholder_count > 0 { count += 1; } + if placeholder_count >= MANY_PLACEHOLDERS { + count += 2; + } // 3+ consecutive section bodies that are just "..." // A section body is the content between two `## ` headings (or end of file). @@ -271,11 +301,23 @@ fn check_body_links_drift(body: &str, fm: &Frontmatter) -> Option { if missing.is_empty() { None } else { + // #446 — the ids are known here, so name them instead of shipping + // `` and `` for the reader to substitute. The + // relation is left as a choice because it genuinely is one: only the + // author knows whether the edge is `informs` or `based_on`. + let self_ref = if self_id.is_empty() { + "".to_string() + } else { + self_id.clone() + }; Some(format!( - "Body's `## Related Artifacts` table mentions {} but frontmatter `links:` array doesn't reference \ - them. Run: forgeplan link --relation \ - OR remove the table row if the mention is incidental.", - missing.join(", ") + "Body's `## Related Artifacts` table mentions {} but the artifact is not linked to \ + them. Run: forgeplan link {} {} --relation — pick the \ + relation, the ids are already correct. Or remove the table row if the mention is \ + incidental.", + missing.join(", "), + self_ref, + missing[0], )) } } @@ -426,6 +468,26 @@ fn prd_rules(depth: &Mode) -> Vec { check_prd_fr_format, )); + // Issue #449. PRD carried 24 validator rules and none for non-functional + // requirements. `extract_nfr_section` already existed — called from exactly + // one place, the tech-leakage check — so the validator could find the + // section and asked nothing about its contents. + // + // Should, not Must: 30 of the 69 PRDs here have no NFR section, and turning + // them all red at once is how a rule gets ignored rather than obeyed. + rules.push(rule( + "prd-nfr-exist", + Severity::Should, + "Non-Functional Requirements section", + check_prd_nfr_exists, + )); + rules.push(rule( + "prd-nfr-measurable", + Severity::Should, + "NFRs state numbers, not adjectives", + check_prd_nfr_measurable, + )); + // BMAD Step 5: Measurability checks rules.push(rule( "prd-measurability-adjectives", @@ -690,6 +752,36 @@ fn check_prd_fr_format(body: &str, _fm: &Frontmatter) -> Option { } } +fn check_prd_nfr_exists(body: &str, _fm: &Frontmatter) -> Option { + if checks::extract_nfr_section(body).is_some() { + return None; + } + Some( + "No '## Non-Functional Requirements' section. Performance, reliability and security \ + budgets that are never written down are never verified — state them, or say \ + explicitly that this change has none (aliases: 'NFR', 'Quality Attributes')" + .into(), + ) +} + +fn check_prd_nfr_measurable(body: &str, _fm: &Frontmatter) -> Option { + let findings = checks::check_nfr_measurability(body); + if findings.is_empty() { + return None; + } + let details: Vec = findings + .iter() + .take(5) + .map(|(word, line)| format!("'{word}' at line {line}")) + .collect(); + Some(format!( + "Subjective adjectives in NFR: {}. An NFR without a number cannot be verified — give \ + each one a threshold and how it is measured (e.g. 'p95 < 200ms under 50 rps'), or \ + mark it TBD so the gap stays visible", + details.join(", ") + )) +} + fn check_prd_measurability_adjectives(body: &str, _fm: &Frontmatter) -> Option { let findings = checks::check_measurability_adjectives(body); if findings.is_empty() { @@ -943,9 +1035,40 @@ fn spec_rules(_depth: &Mode) -> Vec { "Related Artifacts", check_spec_related, ), + // Issue #450, narrowed to internal consistency — see + // `checks::requirements_without_scenarios` for why the blanket form was + // rejected. Should, not Must: a half-authored spec is worth flagging, + // not worth blocking mid-draft. + rule( + "spec-requirement-has-scenario", + Severity::Should, + "Each `### Requirement` carries a `#### Scenario`", + check_spec_requirement_scenarios, + ), ] } +fn check_spec_requirement_scenarios(body: &str, _fm: &Frontmatter) -> Option { + let missing = checks::requirements_without_scenarios(body); + if missing.is_empty() { + return None; + } + let shown: Vec = missing.iter().take(3).map(|t| format!("`{t}`")).collect(); + let more = missing.len().saturating_sub(shown.len()); + let tail = if more > 0 { + format!(" (+{more} more)") + } else { + String::new() + }; + Some(format!( + "{} requirement(s) have no `#### Scenario`: {}{tail}. A requirement without a scenario \ + is a promise with no oracle — add a `#### Scenario` with GIVEN / WHEN / THEN beneath \ + each, or move the requirement out until it can be stated as observable behaviour", + missing.len(), + shown.join(", ") + )) +} + fn check_spec_summary(body: &str, _fm: &Frontmatter) -> Option { if !checks::section_exists(body, "Summary") { Some("Missing '## Summary' section".into()) @@ -955,13 +1078,41 @@ fn check_spec_summary(body: &str, _fm: &Frontmatter) -> Option { } fn check_spec_contracts(body: &str, _fm: &Frontmatter) -> Option { - let has_api = checks::section_exists(body, "API"); - let has_data = checks::section_exists(body, "Data Model"); - let has_contracts = checks::section_exists(body, "Contracts"); - if !has_api && !has_data && !has_contracts { - Some("Missing '## API Contracts' or '## Data Models' section".into()) - } else { + // A SPEC must carry a contract someone can check an implementation against. + // The rule does NOT get to decide what form that takes. + // + // PROB-105. Until this fix the accepted headings were `API`, `Data Model` + // and `Contracts` — all structural. A behavioural spec built from + // `## Requirements` / `### Requirement` / `#### Scenario` with GIVEN/WHEN/THEN + // failed this MUST and could not be activated, while an untouched template + // full of `{METHOD} /v1/{resource}` passed it. The kernel was mandating one + // methodology's shape and rejecting the other, which is the reverse of what + // issue #450 reports. + // + // `section_exists` matches by prefix, so "Contracts" never matched a + // singular `## Contract`, and "Behavioral Contract" matched nothing at all + // because the prefix is "behavioral". + // + // Both oracles are legitimate: an API contract tells an implementer what to + // build, a scenario tells a test what to assert. Either satisfies the rule; + // neither is privileged. + let structural = checks::section_exists(body, "API") + || checks::section_exists(body, "Data Model") + || checks::section_exists(body, "Contract"); + let behavioural = checks::section_exists(body, "Requirements") + || checks::section_exists(body, "Behavioral Contract") + || checks::section_exists(body, "Behavioural Contract"); + + if structural || behavioural { None + } else { + Some( + "Missing a contract section — a SPEC needs something an implementation can be \ + checked against: `## API Contracts` / `## Data Models` / `## Contract` for a \ + structural spec, or `## Requirements` with `#### Scenario` blocks for a \ + behavioural one" + .into(), + ) } } @@ -1654,6 +1805,7 @@ mod tests { let prd_base = 5; // problem, goals, non-goals, fr, related let fr_format = 1; // fr-format check (all depths) let measurability = 2; // adjectives + vague quantifiers (all depths) + let nfr = 2; // #449: nfr-exist + nfr-measurable (all depths) let density_detection = 2; // filler-phrases + density-score (all depths) let traceability = 2; // orphan-frs + orphan-goals (all depths) let classification = 2; // domain-sections + project-type-sections (all depths) @@ -1663,6 +1815,7 @@ mod tests { + prd_base + fr_format + measurability + + nfr + density_detection + traceability + classification @@ -1677,6 +1830,7 @@ mod tests { let standard_extra = 3; // density, audience, leakage let fr_format = 1; let measurability = 2; // adjectives + vague quantifiers + let nfr = 2; // #449: nfr-exist + nfr-measurable (all depths) let density_detection = 2; // filler-phrases + density-score let traceability = 2; // orphan-frs + orphan-goals let classification = 2; // domain-sections + project-type-sections @@ -1687,6 +1841,7 @@ mod tests { + standard_extra + fr_format + measurability + + nfr + density_detection + traceability + classification @@ -1707,6 +1862,7 @@ mod tests { let deep_extra = 7; // timeline, stakeholders, acceptance, risk, rollback, success_metrics, dependencies let fr_format = 1; let measurability = 2; // adjectives + vague quantifiers + let nfr = 2; // #449: nfr-exist + nfr-measurable (all depths) let density_detection = 2; // filler-phrases + density-score let traceability = 2; // orphan-frs + orphan-goals let classification = 2; // domain-sections + project-type-sections @@ -1718,6 +1874,7 @@ mod tests { + deep_extra + fr_format + measurability + + nfr + density_detection + traceability + classification @@ -1744,15 +1901,19 @@ mod tests { } #[test] - fn rules_for_spec_returns_base_plus_3() { + fn rules_for_spec_returns_base_plus_4() { let rules = rules_for(&ArtifactKind::Spec, &Mode::Standard); let base_count = base_rules().len(); - assert_eq!(rules.len(), base_count + 3); + // #450 added spec-requirement-has-scenario. Renamed rather than edited + // in place, so a stale reference to "plus_3" cannot resolve and be + // trusted. + assert_eq!(rules.len(), base_count + 4); let ids: Vec<&str> = rules.iter().map(|(id, _, _, _)| *id).collect(); assert!(ids.contains(&"spec-summary")); assert!(ids.contains(&"spec-contracts")); assert!(ids.contains(&"spec-related")); + assert!(ids.contains(&"spec-requirement-has-scenario")); } #[test] @@ -2065,6 +2226,127 @@ mod tests { } // ─── no-stub-content (PRD-043 FR-003) ────────────────────────────────── + /// PROB-105. An untouched SPEC template used to score exactly 1 against a + /// threshold of 3 and validate with zero findings — the twelve phrase + /// markers are PRD prose, and the placeholder signal was capped at +1 no + /// matter how many slots were unfilled. + /// + /// Verified by mutation: removing the scaling puts this template back to + /// `PASS -- 0 error(s), 0 warning(s)`. + #[test] + fn an_untouched_spec_template_is_detected_as_a_stub() { + let body = "\ +# SPEC-{NNN}: {Specification Title} + +## Summary + +Что специфицируется. Одно предложение. + +## API Contracts + +### Endpoint: `{METHOD} /v1/{resource}` + +**Request**: +```json +{ \"field1\": \"string (required)\" } +``` + +### Endpoint: `{METHOD} /v1/{resource}/{id}` + +## Data Models + +### Entity: {EntityName} + +| Field | Type | +|---|---| +| {field} | {type} | + +### Entity: {OtherEntity} + +## Errors + +| {status} | {code} | +"; + assert!( + check_stub(body, &Frontmatter::new()).is_some(), + "a body that is still mostly unfilled slots must read as a stub" + ); + } + + /// The test above pins a hand-abridged copy of the template, so it cannot + /// notice the real file changing. This one reads the file the binary + /// actually ships (`include_str!`, same path as `template::engine`) and + /// asserts two things about it: + /// + /// 1. it still reads as a stub — an author who runs `forgeplan new spec` + /// and stops must not be able to activate the result; + /// 2. the guidance comment added for PROB-105 does not register as a + /// heading. It names `## Requirements` and `### Requirement` inside + /// backticks; if `section_exists` ever started matching those, an empty + /// template would satisfy `spec-contracts` through its own comment. + #[test] + fn the_shipped_spec_template_is_a_stub_and_declares_no_sections() { + let raw = include_str!("../../../../templates/spec/_TEMPLATE.md"); + let body = raw + .strip_prefix("---") + .and_then(|rest| rest.split_once("\n---")) + .map(|(_, after)| after) + .unwrap_or(raw); + + assert!( + check_stub(body, &Frontmatter::new()).is_some(), + "the shipped SPEC template must read as a stub -- \ + it is entirely unfilled slots" + ); + assert!( + !checks::section_exists(body, "Requirements"), + "the guidance comment must not register as a Requirements heading" + ); + } + + /// The other side of the same threshold: a real spec uses a handful of + /// placeholders in examples and must stay silent. Measured across the six + /// real SPECs in this repository: 0-3 placeholders each. + #[test] + fn a_real_spec_with_a_few_placeholders_is_not_a_stub() { + let body = "\ +# SPEC-003: Playbook YAML schema + +## Summary + +The on-disk contract for a playbook file. + +## Contract + +Every playbook declares `schema_version`, `name` and a non-empty `steps` list. +A step names the agent it dispatches to and the artifact kind it may write. +Unknown keys are rejected rather than ignored, so a typo fails loudly. + +## Data Models + +```rust +pub struct Playbook { + pub schema_version: SchemaVersion, + pub name: String, + pub steps: Vec, +} +``` + +Paths are written as `{workspace}/playbooks/.yaml` where the brace is a +literal placeholder in prose, not an unfilled slot. + +## Errors + +| Code | Meaning | +|---|---| +| E_SCHEMA | schema_version is absent or unsupported | +| E_EMPTY | steps is present but empty | +"; + assert!( + check_stub(body, &Frontmatter::new()).is_none(), + "a filled spec with an incidental placeholder must not read as a stub" + ); + } #[test] fn test_check_stub_detailed_returns_count() { 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-core/tests/scoring_cascade_test.rs b/crates/forgeplan-core/tests/scoring_cascade_test.rs new file mode 100644 index 00000000..6a18e592 --- /dev/null +++ b/crates/forgeplan-core/tests/scoring_cascade_test.rs @@ -0,0 +1,273 @@ +//! What R_eff means when the graph is not a clean chain — PRD-086. +//! +//! Three questions the recursive scorer answers badly enough that people filed +//! issues about all three: +//! +//! - can a leaf EvidencePack be trusted at all (#325) +//! - does trust flow the wrong way along an `informs` edge (#325, FR-002) +//! - does a Note the methodology exempts from evidence poison everything +//! built on it (#392, narrowed) +//! +//! These run against a real store with real relations, because every one of +//! them is about how the walk behaves, not about arithmetic on a Vec. + +use forgeplan_core::db::store::{LanceStore, NewArtifact}; +use forgeplan_core::scoring::reff::r_eff_recursive; +use std::collections::HashSet; +use tempfile::TempDir; + +async fn make_store(tmp: &TempDir) -> LanceStore { + let ws = tmp.path().join(".forgeplan"); + LanceStore::init(&ws).await.unwrap() +} + +fn artifact(id: &str, kind: &str, status: &str, body: &str) -> NewArtifact { + NewArtifact { + id: id.into(), + kind: kind.into(), + status: status.into(), + title: format!("Test {id}"), + body: body.into(), + depth: "standard".into(), + author: None, + parent_epic: None, + valid_until: None, + tags: Vec::new(), + } +} + +const CANONICAL: &str = "verdict: supports\ncongruence_level: 3\nevidence_type: measurement\n"; + +async fn score(store: &LanceStore, id: &str) -> f64 { + let mut seen: HashSet = HashSet::new(); + r_eff_recursive(id, store, &mut seen).await.unwrap().r_eff +} + +/// #325. A pack has no packs, so asking it for its evidence found none and +/// returned 0.0 with "No evidence found (L0)". The intrinsic score already +/// existed — it is applied to this same pack whenever it scores for someone +/// else — and was simply never applied to the pack itself. +#[tokio::test] +async fn a_canonical_leaf_pack_is_worth_something_on_its_own() { + let tmp = TempDir::new().unwrap(); + let store = make_store(&tmp).await; + store + .create_artifact_for_test(&artifact("EVID-001", "evidence", "active", CANONICAL)) + .await + .unwrap(); + + let s = score(&store, "EVID-001").await; + assert!( + (s - 1.0).abs() < 1e-9, + "supports + CL3 is the strongest a pack can state; got {s}" + ); +} + +/// The intrinsic score is a score, not a rubber stamp: a weakening pack at a +/// distant congruence level must land low. +#[tokio::test] +async fn the_leaf_score_still_discriminates() { + let tmp = TempDir::new().unwrap(); + let store = make_store(&tmp).await; + store + .create_artifact_for_test(&artifact( + "EVID-002", + "evidence", + "active", + "verdict: weakens\ncongruence_level: 3\nevidence_type: measurement\n", + )) + .await + .unwrap(); + + // `weakens` is 0.5, CL3 costs nothing, `measurement` costs nothing — so the + // computed value is exactly 0.5, and only the fix can produce it. + // + // The first draft of this test used `weakens` at CL1 with `audit` and + // asserted `s < 0.2`. That passes on the broken code too, where every leaf + // is 0.0 — and worse, the honest score for those inputs IS 0.0 after + // penalties, so tightening it to `s > 0.0` made the test wrong rather than + // stronger. Mutation testing found the first mistake; running the fixed + // tree found the second. + let s = score(&store, "EVID-002").await; + assert!( + (s - 0.5).abs() < 1e-9, + "a weakening pack states half a case, not none and not all; got {s}" + ); +} + +/// FR-002. An outgoing `informs` points at what the pack SUPPORTS. Treating it +/// as a dependency made trust in a measurement flow down from the decision it +/// justifies — so a well-formed pack attached to an unevidenced PRD scored +/// zero, which is the tail wagging the dog. +/// +/// NOTE ON WHAT THIS PROVES. For a LEAF pack the FR-001 early return fires +/// first and the dependency walk is never built, so deleting the FR-002 filter +/// leaves this test green — an adversarial review caught that. It is kept as a +/// behavioural guard on the observable outcome, and +/// `a_pack_with_children_is_not_dragged_down_by_what_it_informs` below is the +/// one that actually reaches the filter. +#[tokio::test] +async fn a_pack_is_not_dragged_down_by_the_artifact_it_supports() { + let tmp = TempDir::new().unwrap(); + let store = make_store(&tmp).await; + store + .create_artifact_for_test(&artifact("PRD-001", "prd", "active", "no evidence here")) + .await + .unwrap(); + store + .create_artifact_for_test(&artifact("EVID-003", "evidence", "active", CANONICAL)) + .await + .unwrap(); + store + .add_relation_for_test("EVID-003", "PRD-001", "informs") + .await + .unwrap(); + + let s = score(&store, "EVID-003").await; + assert!( + (s - 1.0).abs() < 1e-9, + "the pack's own quality does not depend on what it informs; got {s}" + ); +} + +/// FR-002 where it is actually reachable: a pack that HAS child evidence, so +/// the FR-001 early return does not fire and the dependency walk runs, pointing +/// at a decision that is weak for a reason of its OWN. +/// +/// Getting this test to reach the code took three attempts, and the two failures +/// are worth recording because both looked correct: +/// +/// 1. A leaf pack informing an unevidenced PRD — the FR-001 early return fires +/// first and the dependency walk is never built. +/// 2. A pack WITH children informing an unevidenced PRD — the PRD is not +/// unevidenced at all, because the pack under test informs it. Evidence +/// collection reads incoming edges, so linking the pack to the PRD is what +/// evidences the PRD. The min had nothing to drag anything down with. +/// +/// So the weak artifact has to be weak independently: PRD-201 is unevidenced, +/// PRD-200 is `based_on` it and therefore zero despite its own evidence, and +/// EVID-200 informs PRD-200. Without the filter, EVID-200 inherits that zero. +#[tokio::test] +async fn a_pack_with_children_is_not_dragged_down_by_what_it_informs() { + let tmp = TempDir::new().unwrap(); + let store = make_store(&tmp).await; + + // The independent source of weakness, two hops away from the pack. + store + .create_artifact_for_test(&artifact("PRD-201", "prd", "active", "no evidence at all")) + .await + .unwrap(); + // The decision the pack supports — evidenced, but zeroed by its own parent. + store + .create_artifact_for_test(&artifact("PRD-200", "prd", "active", "built on PRD-201")) + .await + .unwrap(); + store + .add_relation_for_test("PRD-200", "PRD-201", "based_on") + .await + .unwrap(); + + // The pack under test, plus a child so the FR-001 early return does not fire. + store + .create_artifact_for_test(&artifact("EVID-200", "evidence", "active", CANONICAL)) + .await + .unwrap(); + store + .create_artifact_for_test(&artifact("EVID-201", "evidence", "active", CANONICAL)) + .await + .unwrap(); + store + .add_relation_for_test("EVID-201", "EVID-200", "informs") + .await + .unwrap(); + store + .add_relation_for_test("EVID-200", "PRD-200", "informs") + .await + .unwrap(); + + // Precondition: the decision really is zero, or this test proves nothing. + let prd = score(&store, "PRD-200").await; + assert_eq!( + prd, 0.0, + "setup is wrong — PRD-200 must be zeroed by PRD-201" + ); + + let s = score(&store, "EVID-200").await; + assert!( + s > 0.0, + "a measurement's reliability must not depend on the decision it justifies; got {s}" + ); +} + +/// #392, narrowed. The routing table calls a Note the artifact for trivial +/// reversible work — no evidence required. The cascade then read an active +/// unevidenced Note as zero trust and poisoned everything based on it, so +/// forgeplan contradicted itself: no evidence needed here, everything +/// downstream unevidenced. +#[tokio::test] +async fn an_exempt_note_does_not_poison_what_is_built_on_it() { + let tmp = TempDir::new().unwrap(); + let store = make_store(&tmp).await; + store + .create_artifact_for_test(&artifact("NOTE-001", "note", "active", "a design choice")) + .await + .unwrap(); + store + .create_artifact_for_test(&artifact("PRD-002", "prd", "active", "well evidenced")) + .await + .unwrap(); + store + .create_artifact_for_test(&artifact("EVID-004", "evidence", "active", CANONICAL)) + .await + .unwrap(); + store + .add_relation_for_test("EVID-004", "PRD-002", "informs") + .await + .unwrap(); + store + .add_relation_for_test("PRD-002", "NOTE-001", "based_on") + .await + .unwrap(); + + let s = score(&store, "PRD-002").await; + assert!( + s > 0.0, + "a Note the methodology exempts from evidence must not zero its dependants; got {s}" + ); +} + +/// The other half of the same decision, and the one that keeps the product +/// honest: a kind that CAN owe evidence and has none is real debt, and the +/// weakest-link cascade surfacing it is the whole point. The #392 proposals +/// that would have softened this were declined. +#[tokio::test] +async fn an_unevidenced_prd_still_drags_its_dependants_down() { + let tmp = TempDir::new().unwrap(); + let store = make_store(&tmp).await; + store + .create_artifact_for_test(&artifact("PRD-100", "prd", "active", "no evidence")) + .await + .unwrap(); + store + .create_artifact_for_test(&artifact("PRD-101", "prd", "active", "well evidenced")) + .await + .unwrap(); + store + .create_artifact_for_test(&artifact("EVID-005", "evidence", "active", CANONICAL)) + .await + .unwrap(); + store + .add_relation_for_test("EVID-005", "PRD-101", "informs") + .await + .unwrap(); + store + .add_relation_for_test("PRD-101", "PRD-100", "based_on") + .await + .unwrap(); + + let s = score(&store, "PRD-101").await; + assert_eq!( + s, 0.0, + "an unevidenced PRD in the chain is debt the score must keep showing" + ); +} diff --git a/crates/forgeplan-mcp/Cargo.toml b/crates/forgeplan-mcp/Cargo.toml index bb6786a7..0d14b22e 100644 --- a/crates/forgeplan-mcp/Cargo.toml +++ b/crates/forgeplan-mcp/Cargo.toml @@ -18,7 +18,7 @@ name = "forgeplan_mcp" path = "src/lib.rs" [dependencies] -forgeplan-core = { path = "../forgeplan-core", version = "0.36.0" } +forgeplan-core = { path = "../forgeplan-core", version = "0.37.0" } rmcp = { version = "1.7", features = ["server", "transport-io"] } schemars = "0.8" serde.workspace = true @@ -33,7 +33,7 @@ tracing-subscriber = { version = "0.3", features = ["env-filter"] } semantic-search = ["forgeplan-core/semantic-search"] [dev-dependencies] -forgeplan-core = { path = "../forgeplan-core", version = "0.36.0", features = ["test-helpers"] } +forgeplan-core = { path = "../forgeplan-core", version = "0.37.0", features = ["test-helpers"] } tempfile = "3" serde_yaml.workspace = true # Phase 2.4: enable the rmcp `client` role for E2E integration tests so we diff --git a/crates/forgeplan-mcp/src/convert.rs b/crates/forgeplan-mcp/src/convert.rs index c5918a98..2b4b7eb8 100644 --- a/crates/forgeplan-mcp/src/convert.rs +++ b/crates/forgeplan-mcp/src/convert.rs @@ -140,6 +140,12 @@ impl From for ArtifactRecordDto { assigned_number: identity.assigned_number, id_canonical: identity.id_canonical, id_display: identity.id_display, + // #447 — this conversion has no store handle, so edges cannot be + // read here. The caller that does have one fills them in; see + // `forgeplan_get` in server.rs. Left empty rather than optional so + // a caller who forgets still emits `{"outbound": [], "inbound": []}` + // instead of silently dropping the field. + links: Default::default(), } } } diff --git a/crates/forgeplan-mcp/src/server.rs b/crates/forgeplan-mcp/src/server.rs index d55a774d..136023dc 100644 --- a/crates/forgeplan-mcp/src/server.rs +++ b/crates/forgeplan-mcp/src/server.rs @@ -3697,7 +3697,30 @@ impl ForgeplanServer { next_action.push_str(&claim_hint); } - hinted_result(&ArtifactRecordDto::from(r), next_action) + // #447 — fill the edges the pure `From` conversion cannot + // reach. Failure to read the relations table degrades to empty + // rather than failing the whole read: an artifact body is + // still worth returning when the edge lookup misbehaves. + let outbound = store.get_relations(&canonical).await.unwrap_or_default(); + let inbound = store + .get_incoming_relations(&canonical) + .await + .unwrap_or_default(); + let mut dto = ArtifactRecordDto::from(r); + dto.links = crate::types::ArtifactLinksDto { + outbound: outbound + .into_iter() + .map(|(target, relation)| crate::types::OutboundLinkDto { + target, + relation, + }) + .collect(), + inbound: inbound + .into_iter() + .map(|(source, relation)| crate::types::InboundLinkDto { source, relation }) + .collect(), + }; + hinted_result(&dto, next_action) } Ok(None) => Ok(artifact_not_found(&p.id)), Err(e) => Ok(safe_err_result("", e)), @@ -4522,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} — \ @@ -8122,8 +8152,10 @@ impl ForgeplanServer { #[tool( description = "Manually advance (or set) the advisory **artifact lifecycle phase** marker \ for an artifact (shape/validate/adi/code/test/audit/evidence/done). \ - Appends a transition to the history. Does NOT validate phase ordering — \ - advisory layer allows out-of-order jumps (e.g. direct `done` override). \ + Appends a transition to the history. Forward and out-of-order jumps are \ + allowed, including a direct `done` override, and so is moving BACKWARDS — \ + this tool is the deliberate-correction path (PRD-086 FR-007). Automatic \ + advancement triggered by other tools refuses to move a phase backwards. \ Full phase enforcement lands in a later PRD under EPIC-005. Use when \ auto-advancement missed a transition or when reclassifying workflow state. \ NOTE: this targets the artifact lifecycle phase machine, NOT the \ @@ -8164,8 +8196,20 @@ impl ForgeplanServer { let safe_id = sanitize_for_hint(&p.id); let safe_reason = p.reason.as_deref().map(sanitize_for_hint); - match forgeplan_core::phase::store::advance_phase(&ws, &p.id, target, p.reason.clone()) - .await + // PRD-086 FR-007. The monotonicity guard belongs on AUTOMATIC advancement + // — `maybe_advance_phase`, which fires on every `forgeplan_validate` + // PASS and used to walk a shipped artifact back from `done`. An explicit + // call to THIS tool is the same act as `forgeplan phase-advance` on the + // command line: someone deciding to correct a marker. The CLI was moved + // to the unchecked entry point and this handler must match it, or the + // two surfaces disagree about what the same operation means. + match forgeplan_core::phase::store::advance_phase_unchecked( + &ws, + &p.id, + target, + p.reason.clone(), + ) + .await { Ok(state) => { let current = state.current_phase.as_str(); diff --git a/crates/forgeplan-mcp/src/types.rs b/crates/forgeplan-mcp/src/types.rs index b4dd7a99..de0c9497 100644 --- a/crates/forgeplan-mcp/src/types.rs +++ b/crates/forgeplan-mcp/src/types.rs @@ -38,6 +38,31 @@ pub struct ArtifactSummaryDto { pub id_display: String, } +// #447 — the graph edges of a single artifact. `forgeplan_get` returned ~20 +// fields and none of them were links, so an artifact with five edges and one +// with none were indistinguishable in the response. Against a rich object, a +// missing field reads as "this artifact has none", not "this tool does not +// report them". +#[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)] +pub struct OutboundLinkDto { + pub target: String, + pub relation: String, +} + +#[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)] +pub struct InboundLinkDto { + pub source: String, + pub relation: String, +} + +/// Inbound matters as much as outbound: "which evidence supports this" is the +/// question behind an `r_eff` of 0, and it is invisible from the outbound side. +#[derive(Debug, Clone, Default, Serialize, Deserialize, JsonSchema)] +pub struct ArtifactLinksDto { + pub outbound: Vec, + pub inbound: Vec, +} + #[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)] pub struct ArtifactRecordDto { pub id: String, @@ -66,6 +91,12 @@ pub struct ArtifactRecordDto { pub id_canonical: String, #[serde(default)] pub id_display: String, + // #447 — deliberately NOT `skip_serializing_if`. An absent field and an + // empty one say the same thing to a caller, which is the whole defect; + // the empty arrays are the signal that the question was asked and the + // answer is "none". + #[serde(default)] + pub links: ArtifactLinksDto, } #[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)] diff --git a/docs/handoff/spec-validator-and-nfr-rules.md b/docs/handoff/spec-validator-and-nfr-rules.md new file mode 100644 index 00000000..92b5d1a1 --- /dev/null +++ b/docs/handoff/spec-validator-and-nfr-rules.md @@ -0,0 +1,166 @@ +# Handoff: SPEC validator inversion + NFR rules (#449, #450, PROB-105) + +**Status:** code complete, gates green, **pushed**; PR open against `dev`. +**Branch:** `fix/spec-validator-inverted` off `dev` — `daa103b` (code) + `3e16e48` (this document). +**Written:** 2026-09-07, against `forgeplan 0.36.0`. + +Everything this document points at is committed. That is deliberate: the previous +handoff in this repo pointed at a session scratch directory that did not survive +the session, which is the same defect class the work was about. + +--- + +## What is done + +One commit, `daa103b`, closing two GitHub issues and one problem found while +checking them. + +### The finding that reframed both issues + +Issue #450 asks for a rule enforcing "every `### Requirement` has a +`#### Scenario`", on the grounds that the invariant is declared in prose and +checked nowhere. Measured before implementing, the premise does not hold here — +and the truth is worse than the report. + +| Input | Before | After | +|---|---|---| +| untouched SPEC template | `PASS — 0 error(s), 0 warning(s)` → activates at **R_eff 1.00** | stub warning; `activate` refuses | +| behavioural spec, 2 requirements + 2 GIVEN/WHEN/THEN scenarios | **`x [MUST] spec-contracts`** → FAIL, cannot activate | PASS | +| SPEC-002 … SPEC-006 | pass | unchanged | +| SPEC-001 | fails `spec-summary` + `spec-contracts` | unchanged — it is a document *about* writing specs, filed as a spec | + +So core did not lack a rule about scenarios. It had a MUST pointing against them, +while waving through documents that said nothing. That is why the marketplace TDD +flow grew its own gate (`tdd-planner` HARD RULE 1): core actively rejected the +shape TDD needs. + +### Three fixes, none of which teaches the kernel a methodology + +1. **`check_stub` can see a SPEC.** Its twelve phrase markers are all PRD prose; + the placeholder signal was capped at `+1` against a threshold of 3, so fifteen + unfilled slots weighed the same as one. The count scales now. + + Threshold from the corpus, not taste — SPEC template **15** placeholders, PRD + template **5**, the six real SPECs **0–3**. A line-count test was measured and + rejected: the untouched template has 25 non-empty lines under + `## API Contracts`, because placeholder JSON is still lines. + +2. **`spec-contracts` accepts a behavioural contract.** It still demands *a* + contract; it stopped demanding one particular form. `## Requirements`, + `## Contract`, `## Behavioral Contract` join the structural headings. + +3. **`spec-requirement-has-scenario`** (Should) — #450, conditional. Silent on a + structural spec, fires only on a half-authored behavioural one. The blanket + form would have fired on 6 of 6 SPECs here, none defective. + +Plus: the stub gate's remediation told every kind to "Fill MUST sections +(Problem, Goals, FR)". It now names the sections of the kind in hand. + +### #449 — NFR rules, and the trap in them + +`prd-nfr-exist` and `prd-nfr-measurable`, both Should. `extract_nfr_section` +already existed, called from exactly one place — the tech-leakage check — so the +validator could find the section and asked nothing about its contents. + +The subjective-adjective blacklist reads like a list of NFRs (`scalable`, +`robust`, `efficient`, `responsive`, `fast`) and was only ever applied to the FR +section. Across the 69 PRDs: **2** hits in FR (checked), **15** in NFR (not). + +**All 15 sit inside the template's own guidance** — +``. A rule flagging them +would be unclosable: the only fix is deleting the instructions. The rule strips +non-prose first. Verified **0 findings across all 69 PRDs**, while hand-written +vague prose still produces findings with line numbers. + +> `check_measurability_adjectives`, the FR-side rule that shipped long ago, does +> **not** strip and carries the same latent bug. It has never fired only because +> the template's BAD examples happen to live under the NFR heading. Left alone — +> noted so the next person does not rediscover it. + +--- + +## Verification already done + +| Gate | Result | +|---|---| +| `cargo fmt --all -- --check` | exit 0 | +| `cargo clippy --workspace --all-targets` | exit 0, **0 warnings** | +| `cargo test -p forgeplan-core --features test-helpers --no-fail-fast` | 2239 passed, 3 failed (17 binaries) | +| `cargo test -p forgeplan --no-fail-fast` | 807 passed, **0 failed** (58 binaries) | +| `cargo test -p forgeplan-mcp --no-fail-fast` | 274 passed, **0 failed** (19 binaries) | + +94 binaries total, which matches what `--workspace` produces — the per-crate +split did not skip anything. The 3 failures are #454 in `git::tests`; they pass +in isolation and this diff does not touch that module. + +**Mutation checks** — each fix reverted, the matching test had to fail: + +- remove the comment strip → `nfr_adjectives_inside_html_comments_are_not_flagged` FAILED +- remove the placeholder scaling → untouched template back to `PASS — 0 error(s), 0 warning(s)` + +Four pre-existing rule-count tests failed and were **updated, not silenced**. +They sum named groups with comments rather than asserting a bare number, so +updating one means explaining where the new term came from. +`rules_for_spec_returns_base_plus_3` was **renamed** to `_plus_4` rather than +edited in place — a test name carrying a number goes stale silently. + +--- + +## What is left + +1. ~~Push and open the PR~~ — done; see the PR linked from the branch. +2. **CHANGELOG entry** — drafted but not written into the file. Content is in + this document; the Fixed/Added split is already worked out above. +3. **SPEC template note** (optional, recommended). The template is API-first and + is the only signal an author has about what a spec looks like. Now that the + validator accepts a behavioural contract, a two-line comment near the top + saying both shapes are legitimate removes the surprise. Templates compile into + the binary via `include_str!`, so do not edit them while a build is running. +4. **Issue comments** for #449 and #450 — #450's especially, because its premise + was inverted and the reporter deserves to know the truth was worse than the + report. + +--- + +## Do not re-litigate + +- **No whitelist of legitimate SPEC shapes in the kernel.** A judge panel + considered one and rejected it: a list of six blessed forms rots toward noise, + and a seventh legitimate shape would need a Rust change and a release before + its author stops getting a false finding. The substance tests ask whether a + section is filled, not whether it is Gherkin. +- **The blanket #450 rule.** Measured: fires on 6 of 6 SPECs here, none + defective. +- **Must instead of Should** for the NFR rules. 30 of 69 PRDs have no NFR + section. + +--- + +## Environment notes that cost real time + +- **A full `cargo test --workspace` does not fit** in the free space on this + machine. Twice it reported `ld: write() failed, errno=28` at ~300 MB free. Run + per crate instead; `forgeplan-core` needs `--features test-helpers` because the + `*_for_test` helpers are gated and normally arrive through `forgeplan-mcp`'s + dev-dependency. +- **Always pass `--no-fail-fast`.** Without it the run stops after the first + failing binary and looks like a truncated build. +- **Count test binaries**, not just pass/fail: a run that never happened prints + zeros that read as success. See `AGENTS.md` § *Parallel agents and the build + directory*. + +## Uncommitted work in the tree that is NOT mine + +A second session is working in this worktree. At the time of writing it left +`Cargo.toml` (a `[profile.dev.package."*"] opt-level = 1` block) and a new +`.cargo/config.toml` (aliases only) uncommitted. Both look sane and on-topic — +they speed up builds — but they are someone else's change and were deliberately +left out of `daa103b`. Do not sweep them into a commit without checking with +their author. + +## Artifacts + +- **PROB-105** — the inversion, with the reproduction and the placeholder + measurements +- **PRD-086** — the trust-layer work this grew out of; PROB-105 informs it +- **#449**, **#450** — the GitHub issues diff --git a/docs/operations/dependabot-triage-2026-09-08.md b/docs/operations/dependabot-triage-2026-09-08.md new file mode 100644 index 00000000..7d0fcf15 --- /dev/null +++ b/docs/operations/dependabot-triage-2026-09-08.md @@ -0,0 +1,113 @@ +# Dependabot triage — 2026-09-08 (v0.37.0 release window) + +Per RED-LINE #10 (CLAUDE.md): each release tags every open Dependabot alert as +**addressed** / **scheduled** / **accepted-with-justification**. Follows the +`docs/operations/RELEASE-PROTOCOL.md` step-4 contract. + +## Snapshot at release time + +```bash +gh api repos/ForgePlan/forgeplan/dependabot/alerts --paginate \ + -q '[.[] | select(.state=="open")] | length' +``` + +**33 open: 8 HIGH / 15 MEDIUM / 10 LOW — 1 Rust, 32 npm.** + +The split is the same shape as every prior release triage: the one Rust alert sits +in the dependency tree of the shipped `forgeplan` binary; all 32 npm alerts are +confined to `website/` — a static Astro documentation site that ships no server and +is not part of any released artifact. + +## Rust — the shipped binary + +| Package | Sev | GHSA | Fix in | Verdict | +|---|---|---|---|---| +| `lru` | LOW | GHSA-rhfx-m35p-ff5j | 0.16.3 | **accepted-with-justification** | + +### `lru` LOW — accepted-with-justification + +`IterMut` violates Stacked Borrows by invalidating an internal pointer. **Cannot be +updated without an upstream change**: the only consumer is `tantivy 0.24.2` (confirmed +again in `Cargo.lock` — `lru 0.12.5`, no direct `forgeplan-core`/`forgeplan-cli`/ +`forgeplan-mcp` dependency on the crate), which pins `lru 0.12.x`, while the fix +landed in `0.16.3` — a major bump only `tantivy` can take. Forgeplan never +constructs an `lru` cache itself and never calls `IterMut`; the advisory describes +undefined behaviour observable under Miri, not a reachable exploit in this +dependency path. **Carried forward** — same verdict as v0.33.0, v0.34.0, v0.35.0. +Re-evaluate when `tantivy` bumps its `lru` bound. + +## RustSec — not a Dependabot alert, checked separately + +GitHub's Dependabot feed does not mirror RustSec, and this gap has bitten this +project twice before this window (v0.34.0: crossbeam-epoch red on `dev` for 11 +days; v0.35.0: h2 red for three consecutive merges). Both times the miss was +"Dependabot showed nothing, so nobody looked at `cargo-deny` directly." + +Checked directly, not inferred from Dependabot's silence: + +```bash +gh run list --branch dev --limit 5 --json name,conclusion,createdAt \ + -q '.[] | select(.name == "security")' +``` + +`security` (the `cargo-deny` workflow) is **green on `dev`** — last run +2026-09-08T17:18:26Z, immediately after the rust-deps (#473) and github-actions +(#459) Dependabot PRs merged, `success`. Stated explicitly here rather than left +to be assumed from an empty Dependabot list, per the lesson the two prior misses +left behind. + +## npm — `website/` only + +| Package | Sev | Count | +|---|---|---| +| `browserslist` | HIGH | 2 | +| `astro` | HIGH | 2 | +| `vite` | HIGH | 1 | +| `sharp` | HIGH | 1 | +| `nanoid` | HIGH | 1 | +| `js-yaml` | HIGH | 1 | +| `dompurify` | MEDIUM | 6 | +| `mermaid` | MEDIUM | 4 | +| `astro` | MEDIUM | 3 | +| `vite` | MEDIUM | 1 | +| `@astrojs/rss` | MEDIUM | 1 | +| `dompurify` | LOW | 4 | +| `postcss-selector-parser` | LOW | 1 | +| `mermaid` | LOW | 1 | +| `esbuild` | LOW | 1 | +| `astro` | LOW | 1 | +| `@babel/core` | LOW | 1 | + +**Verdict: scheduled** — same reasoning as every prior triage, restated because it +still holds and because one open item makes it concrete this time: + +1. **Zero exposure through the released product.** Build-time and render-time + dependencies of a statically generated documentation site; the `forgeplan` + binary, MCP server, and marketplace plugins carry none of them. +2. **A blanket update is a known-bad move here** — established by PR #401 (a + sweeping `npm update` broke the build on a peer-major conflict). This + release's own scoping work found the concrete case: **#442** (`npm-website` + Dependabot group, opened 2026-08-17) carries `astro` 6→7 and `@astrojs/mdx` + 5→7 inside it, and predates the `Website build` CI gate — its green + checkmarks don't include the one check that would actually exercise a + two-major-version jump. Filed as its own issue (#485) with the concrete + next step (rebase so the gate runs, verify with a real `npm run build`) + rather than merged on old green. +3. **Bundling it here would make the release un-reviewable.** v0.37.0 already + carries a breaking scoring-semantics change (PRD-086) plus a lifecycle + data-loss fix (#478) plus a CI-coverage fix (PROB-102). A front-end + dependency sweep on top of that would make bisecting any regression + materially harder. + +**Trigger for the scheduled work:** #485 (already filed) — rebase #442 so the +`Website build` gate actually runs against it, then apply by hand with a +verified `npm run build`, not as a merged auto-group. + +## Verification + +```bash +cargo deny check advisories # → advisories ok +grep -A1 'name = "lru"' Cargo.lock # → 0.12.5 (tantivy-pinned) +gh api repos/ForgePlan/forgeplan/dependabot/alerts --paginate \ + -q '[.[] | select(.state=="open")] | length' # → 33 +``` diff --git a/templates/spec/_TEMPLATE.md b/templates/spec/_TEMPLATE.md index 9d8ee399..95872f78 100644 --- a/templates/spec/_TEMPLATE.md +++ b/templates/spec/_TEMPLATE.md @@ -20,6 +20,22 @@ depth: standard / deep / critical Какие контракты и модели описывает эта спецификация. + + ## API Contracts ### Endpoint: `{METHOD} /v1/{resource}`
394
tracked artifacts		3290
tests passing		437
tracked artifacts		3331
tests passing		 82
CLI commands		 73
MCP tools