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 index 01a44499..9f52b5f0 100644 --- 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 @@ -141,3 +141,4 @@ GitHub: #325, #392, #393, #330. + 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..77e68a55 --- /dev/null +++ b/.forgeplan/problems/PROB-105-the-spec-validator-passes-empty-templates-and-blocks-real-behavioural-specs.md @@ -0,0 +1,113 @@ +--- +depth: tactical +id: PROB-105 +kind: problem +links: +- target: PRD-086 + relation: informs +status: active +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). + + + + diff --git a/CHANGELOG.md b/CHANGELOG.md index b6c4d724..7145c44d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -89,6 +89,55 @@ corresponding sprint evidence under `.forgeplan/evidence/`. 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. + ### Internal - ADR-025 (orchestration sits above ForgePlan; per-surface dispositions) and diff --git a/crates/forgeplan-core/src/lifecycle/mod.rs b/crates/forgeplan-core/src/lifecycle/mod.rs index 329c6032..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." ) }); diff --git a/crates/forgeplan-core/src/validation/checks.rs b/crates/forgeplan-core/src/validation/checks.rs index ebd5d93f..c147e3f8 100644 --- a/crates/forgeplan-core/src/validation/checks.rs +++ b/crates/forgeplan-core/src/validation/checks.rs @@ -79,6 +79,80 @@ pub fn extract_related_artifacts_table_ids(body: &str) -> Vec { 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 { @@ -868,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)> { @@ -1318,6 +1441,128 @@ 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 diff --git a/crates/forgeplan-core/src/validation/rules.rs b/crates/forgeplan-core/src/validation/rules.rs index fe65625b..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). @@ -438,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", @@ -702,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() { @@ -955,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()) @@ -967,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(), + ) } } @@ -1666,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) @@ -1675,6 +1815,7 @@ mod tests { + prd_base + fr_format + measurability + + nfr + density_detection + traceability + classification @@ -1689,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 @@ -1699,6 +1841,7 @@ mod tests { + standard_extra + fr_format + measurability + + nfr + density_detection + traceability + classification @@ -1719,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 @@ -1730,6 +1874,7 @@ mod tests { + deep_extra + fr_format + measurability + + nfr + density_detection + traceability + classification @@ -1756,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] @@ -2077,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/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/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}`