diff --git a/CHANGELOG.md b/CHANGELOG.md index 5fc4bdaa0..911200efb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -332,6 +332,11 @@ breaking changes may land in a minor release. `BMAD_LOOP_*` environment, not a shared app-server daemon's; require Codex >= 0.156. A Codex run paused across the upgrade prints the config-changed advisory once on resume (#901). +- Keep a deferred finding's severity when the dev session qualifies it: `high (unverified)` + files `severity: high` and keeps the declared text in `reason:`. An unrecognized + severity (`P1`) files the entry without the line and journals + `spec-deferrals-severity-unrecognized` instead of dropping it silently; a collection + or NUL severity marks the item malformed (#913). ### Security diff --git a/src/bmad_loop/deferredwork.py b/src/bmad_loop/deferredwork.py index 6a538e5f9..09276633e 100644 --- a/src/bmad_loop/deferredwork.py +++ b/src/bmad_loop/deferredwork.py @@ -2990,6 +2990,20 @@ def _normalize_severity(value: str) -> str | None: return SEVERITY_ALIASES.get(value.lower()) +# The word must end there: a digit, letter or `_word` suffix ("critical1", +# "high_priority") is a different token, while a closing `_` emphasis or a +# hyphenated qualifier ("high-if-true") still reads as the level. +_LEADING_SEVERITY_RE = re.compile(r"[ \t\r\n*_`]*([A-Za-z]+)(?![^\W_]|_\w)") + + +def leading_severity(text: str) -> str | None: + """Normalize a dev session's declared severity by its leading word, so a + qualifier ("high (unverified)") keeps its level; harvest-only — the ledger + reader's SEVERITY_FIELD_RE is deliberately untouched.""" + m = _LEADING_SEVERITY_RE.match(text) + return _normalize_severity(m.group(1)) if m else None + + @dataclass(frozen=True) class LegacyEntry: key: str # stable content-derived identity, unique within the file diff --git a/src/bmad_loop/devcontract.py b/src/bmad_loop/devcontract.py index 5754a88e8..dbbe3c087 100644 --- a/src/bmad_loop/devcontract.py +++ b/src/bmad_loop/devcontract.py @@ -310,6 +310,7 @@ def auto_run_result_fingerprint(text: str) -> tuple[int, str]: _SUMMARY_LIMIT = 200 _EVIDENCE_LIMIT = 1000 _LOCATION_LIMIT = 200 +_SEVERITY_LIMIT = 80 @dataclass(frozen=True) @@ -321,6 +322,9 @@ class DeferredFinding: location: str severity: str fingerprint: str + # The dev session's flattened severity text, kept only when it said more + # than a bare alias (a qualifier, or an unrecognized word); "" otherwise. + declared_severity: str = "" def harvest_fingerprint(*parts: str) -> str: @@ -381,6 +385,13 @@ def parse_deferred_findings(fm: dict[str, Any]) -> tuple[list[DeferredFinding], scalars (numeric and boolean scalars are string-normalized), never collections or embedded NULs. This observation path never raises for the YAML shapes it accepts. + + ``severity`` is read by its leading word (``deferredwork.leading_severity``), + so a qualified value ("high (unverified)") keeps its level. Whenever the + text says more than a bare alias -- a qualifier, or a leading word that is + no alias at all ("P1") -- it is kept as ``declared_severity`` so the ledger + entry can record it; an unrecognized one leaves ``severity`` empty but the + finding stays well-formed. """ raw = fm.get(DEFERRED_FIELD) if raw is None: @@ -401,7 +412,11 @@ def parse_deferred_findings(fm: dict[str, Any]) -> tuple[list[DeferredFinding], ) continue bad_optional = next( - (field for field in ("evidence", "location") if not _is_yaml_scalar(item.get(field))), + ( + field + for field in ("evidence", "location", "severity") + if not _is_yaml_scalar(item.get(field)) + ), None, ) if bad_optional is not None: @@ -414,6 +429,7 @@ def parse_deferred_findings(fm: dict[str, Any]) -> tuple[list[DeferredFinding], "summary": summary_raw, "evidence": item.get("evidence"), "location": item.get("location"), + "severity": item.get("severity"), } bad_nul = next( (field for field, value in raw_text_values.items() if _contains_nul(value)), None @@ -427,15 +443,17 @@ def parse_deferred_findings(fm: dict[str, Any]) -> tuple[list[DeferredFinding], continue evidence = _flatten(item.get("evidence"), _EVIDENCE_LIMIT) location = _flatten(item.get("location"), _LOCATION_LIMIT) + declared = _flatten(item.get("severity"), _SEVERITY_LIMIT) + severity = deferredwork.leading_severity(declared) or "" + bare_alias = bool(severity) and declared.lower() in deferredwork.SEVERITY_ALIASES findings.append( DeferredFinding( summary=summary, evidence=evidence, location=location, - severity=deferredwork.SEVERITY_ALIASES.get( - str(item.get("severity", "")).strip().lower(), "" - ), + severity=severity, fingerprint=harvest_fingerprint(summary, location), + declared_severity="" if bare_alias else declared, ) ) return findings, malformed diff --git a/src/bmad_loop/diagnostics.py b/src/bmad_loop/diagnostics.py index 232135e63..3f2185acb 100644 --- a/src/bmad_loop/diagnostics.py +++ b/src/bmad_loop/diagnostics.py @@ -502,6 +502,10 @@ # the session touched, customer source included) and its retro doc filenames. "retro-auto-dirty": frozenset({"paths"}), "retro-auto-finished": frozenset({"docs"}), + # The harvested findings whose declared severity no alias reads (#913): each + # item carries the dev session's summary and severity text, so only the count + # ships. + "spec-deferrals-severity-unrecognized": frozenset({"items"}), "stale-restore-excluded": frozenset({"files"}), } diff --git a/src/bmad_loop/engine.py b/src/bmad_loop/engine.py index 7d2a2ba52..4a103aece 100644 --- a/src/bmad_loop/engine.py +++ b/src/bmad_loop/engine.py @@ -772,11 +772,15 @@ def _harvest_row( finding whose `seen-again:` match died inside the ledger lock files the byte-identical row it would have filed had the match never existed. Two spellings of this tuple would diverge on exactly the rare path nothing - exercises.""" + exercises. It therefore stays a pure function of the finding, including + the declared-severity note appended to the reason (#913).""" + reason = finding.evidence or finding.summary + if finding.declared_severity: + reason += f" (declared severity: {finding.declared_severity})" return ( f"{HARVEST_ORIGIN} {finding.fingerprint}", finding.summary, - finding.evidence or finding.summary, + reason, finding.location or None, finding.severity or None, ) @@ -6097,6 +6101,19 @@ def _harvest_spec_deferrals( pending: list[tuple[str, str, str, str | None, str | None]] = [ _harvest_row(finding) for finding in harvestable ] + # An unrecognized severity is no reason to drop the finding: its entry is + # still filed (without `severity:`), and this event keeps the missing + # level visible instead of a silent drop (#913). + unrecognized = [f for f in harvestable if f.declared_severity and not f.severity] + if unrecognized: + self.journal.append( + "spec-deferrals-severity-unrecognized", + story_key=task.story_key, + spec=spec_name, + items=[ + {"summary": f.summary, "declared": f.declared_severity} for f in unrecognized + ], + ) if malformed: self.journal.append( "spec-deferrals-malformed", @@ -6285,6 +6302,7 @@ def _harvest_spec_deferrals( seen_again=seen_again_ids, deduped=deduped, malformed=len(malformed), + severity_unrecognized=len(unrecognized), ) def _manifest_closes_deferred(self, task: StoryTask) -> tuple[str, ...]: diff --git a/tests/test_deferredwork.py b/tests/test_deferredwork.py index f9f5d43a2..99f85698e 100644 --- a/tests/test_deferredwork.py +++ b/tests/test_deferredwork.py @@ -1251,6 +1251,30 @@ def test_field_severity_forms(): assert field_severity("no field here") is None +@pytest.mark.parametrize( + ("text", "expected"), + [ + ("high (unverified)", "high"), + ("Medium (unverified)", "medium"), + ("**major**", "high"), + ("high-if-true", "high"), + ("med.", "medium"), + ("blocker: confirmed", "critical"), + ("highest", None), + ("critical1", None), + ("high_priority", None), + ("lowé", None), + ("_high_", "high"), + ("P1", None), + ("unknown", None), + ("", None), + (" ", None), + ], +) +def test_leading_severity_reads_the_leading_word(text, expected): + assert deferredwork.leading_severity(text) == expected + + def test_canonical_severity_uses_the_whole_file_fence_index(): text = """\ # Deferred Work diff --git a/tests/test_devcontract.py b/tests/test_devcontract.py index b904c7ed1..e03ff4f26 100644 --- a/tests/test_devcontract.py +++ b/tests/test_devcontract.py @@ -2598,7 +2598,7 @@ def test_parse_deferred_findings_defaults_optional_fields_and_coerces_scalars(): ] -@pytest.mark.parametrize("field", ["summary", "evidence", "location"]) +@pytest.mark.parametrize("field", ["summary", "evidence", "location", "severity"]) @pytest.mark.parametrize("collection", [["nested", "values"], {"nested": "value"}]) def test_parse_deferred_findings_rejects_collection_valued_text_fields(field, collection): """A malformed optional field costs its whole item too: silently dropping @@ -2614,7 +2614,7 @@ def test_parse_deferred_findings_rejects_collection_valued_text_fields(field, co assert malformed == [f"item 1: `{field}` is not a scalar (got {type(collection).__name__})"] -@pytest.mark.parametrize("field", ["summary", "evidence", "location"]) +@pytest.mark.parametrize("field", ["summary", "evidence", "location", "severity"]) @pytest.mark.parametrize("value", ["before\0after", "x" * 1001 + "\0"]) def test_parse_deferred_findings_rejects_embedded_nul_in_text_fields(field, value): """Reject before clamping too, so a late NUL cannot evade the stated schema.""" @@ -2627,7 +2627,11 @@ def test_parse_deferred_findings_rejects_embedded_nul_in_text_fields(field, valu assert malformed == [f"item 1: `{field}` contains a NUL character"] -def test_parse_deferred_findings_normalizes_known_severity_and_drops_unknown(tmp_path): +def test_parse_deferred_findings_normalizes_known_severity_and_keeps_unknown_as_declared( + tmp_path, +): + """An unrecognized severity leaves `severity` empty but is never discarded: + the declared text rides along so the harvest can file and journal it (#913).""" fm = _deferred_spec( tmp_path / "spec.md", [ @@ -2638,7 +2642,29 @@ def test_parse_deferred_findings_normalizes_known_severity_and_drops_unknown(tmp ) findings, malformed = devcontract.parse_deferred_findings(fm) assert malformed == [] - assert [finding.severity for finding in findings] == ["critical", "high", ""] + assert [(f.severity, f.declared_severity) for f in findings] == [ + ("critical", ""), + ("high", ""), + ("", "spicy"), + ] + + +def test_parse_deferred_findings_keeps_qualified_severity_level_and_text(tmp_path): + """The leading word carries the level; the qualifier is kept as declared + text, while a bare alias in any case or padding says nothing more (#913).""" + fm = _deferred_spec( + tmp_path / "spec.md", + [ + {"summary": "a", "severity": "high (unverified)"}, + {"summary": "b", "severity": " HIGH "}, + ], + ) + findings, malformed = devcontract.parse_deferred_findings(fm) + assert malformed == [] + assert [(f.severity, f.declared_severity) for f in findings] == [ + ("high", "high (unverified)"), + ("high", ""), + ] def test_parse_deferred_findings_isolates_each_malformed_item(tmp_path): diff --git a/tests/test_diagnostics.py b/tests/test_diagnostics.py index b47900036..c2d36a5f9 100644 --- a/tests/test_diagnostics.py +++ b/tests/test_diagnostics.py @@ -1242,6 +1242,24 @@ def test_auto_retro_path_lists_reduce_to_counts(kind, field, values): assert all(value not in json.dumps(scrubbed) for value in values) +def test_unrecognized_severity_items_reduce_to_a_count(): + """#913: the harvest's unrecognized-severity items carry the dev session's own + summary and severity text, so a dump ships their count, never the text. + + Ablation: drop the kind's row from `_JOURNAL_KIND_COUNTLIST_FIELDS` and the + one-word values ride `scrub_json` into the dump.""" + items = [{"summary": "AcmeVault", "declared": "AcmePriority"}] + scrubbed = diagnostics._scrub_entry( + {"kind": "spec-deferrals-severity-unrecognized", "items": items}, + sanitize.Pseudonymizer(salt=b"fixed"), + {}, + None, + ) + + assert scrubbed == {"kind": "spec-deferrals-severity-unrecognized", "items_count": 1} + assert "Acme" not in json.dumps(scrubbed) + + @pytest.mark.parametrize("verdict", ["accepted", "accepted-with-open-items", "rejected", "unknown"]) def test_auto_retro_verdict_ships_as_its_closed_token(verdict): """DW-487: `retro-auto-finished`'s `verdict` is a closed token the engine diff --git a/tests/test_engine.py b/tests/test_engine.py index bd09a8a30..13fcfc0ea 100644 --- a/tests/test_engine.py +++ b/tests/test_engine.py @@ -22597,6 +22597,74 @@ def test_spec_deferrals_malformed_siblings_file_valid_and_one_aggregated_entry(p assert len(malformed) == 1 and len(malformed[0]["items"]) == 2 +def test_spec_deferrals_qualified_severity_files_level_and_keeps_qualifier(project): + """A qualified severity files its leading-word level and keeps the declared + text in `reason:`, so `--min-severity` still selects the entry (#913).""" + from bmad_loop import sweep + + write_sprint(project, {"epic-1": "backlog", "1-1-a": "ready-for-dev"}) + engine, _ = make_engine( + project, + [ + dev_effect( + project, + "1-1-a", + followup_review=False, + deferred=[{**HARVEST_A, "severity": "high (unverified)"}], + ) + ], + policy=_harvest_policy(), + ) + + assert engine.run().done == 1 + + entries = _harvest_entries(project) + assert len(entries) == 1 + entry = entries[0] + assert re.search(r"^severity: high$", entry.body, re.M) + assert deferredwork.field_severity(entry.body) == "high" + reason = next(line for line in entry.body.splitlines() if line.startswith("reason:")) + assert "(declared severity: high (unverified))" in reason + kinds = [e["kind"] for e in engine.journal.entries()] + assert "spec-deferrals-severity-unrecognized" not in kinds + selection = sweep.select_entries([e for e in entries if e.open], min_severity="high") + assert [e.id for e in selection.selected] == [entry.id] + + +def test_spec_deferrals_unrecognized_severity_files_entry_and_journals(project): + """An unrecognized severity still files the finding — without `severity:` — + and journals the declared text rather than dropping it silently (#913).""" + write_sprint(project, {"epic-1": "backlog", "1-1-a": "ready-for-dev"}) + engine, _ = make_engine( + project, + [ + dev_effect( + project, + "1-1-a", + followup_review=False, + deferred=[{**HARVEST_A, "severity": "P1"}], + ) + ], + policy=_harvest_policy(), + ) + + assert engine.run().done == 1 + + entries = _harvest_entries(project) + assert len(entries) == 1 + entry = entries[0] + assert entry.title == HARVEST_A["summary"] + assert not re.search(r"^severity:", entry.body, re.M) + reason = next(line for line in entry.body.splitlines() if line.startswith("reason:")) + assert "(declared severity: P1)" in reason + journal = engine.journal.entries() + unrecognized = [e for e in journal if e["kind"] == "spec-deferrals-severity-unrecognized"] + assert len(unrecognized) == 1 + assert unrecognized[0]["items"] == [{"summary": HARVEST_A["summary"], "declared": "P1"}] + harvested = [e for e in journal if e["kind"] == "spec-deferrals-harvested"] + assert len(harvested) == 1 and harvested[0]["severity_unrecognized"] == 1 + + def test_spec_deferrals_unreadable_spec_returns_retry_without_harvest(project, monkeypatch): engine, _ = make_engine(project, [], policy=_harvest_policy()) sp = spec_path(project, "1-1-a") diff --git a/tests/test_portability_guard.py b/tests/test_portability_guard.py index eda4bb633..f0ccc4cf1 100644 --- a/tests/test_portability_guard.py +++ b/tests/test_portability_guard.py @@ -520,6 +520,11 @@ # which would also wave through a branch-carrying ``target`` on a kind nobody has # looked at — exactly the hole the flattening left. JOURNAL_KIND_BENIGN_FIELDS = { + # `items` is the parser's own malformed-item notes on one harvest kind and the + # stale DW ids on the other; on `spec-deferrals-severity-unrecognized` it carries + # the dev session's summary text and is reduced to a count (#913). + "spec-deferrals-malformed": frozenset({"items"}), + "spec-deferral-sighting-stale": frozenset({"items"}), "board-advance-carried": frozenset({"target"}), "board-advance-carry-failed": frozenset({"target"}), "board-advance-carry-foreign-dirt": frozenset({"target"}), @@ -797,7 +802,6 @@ # `verify-command-result` (DW-353): a bare `True`, present only on a pass a # hard stop request cut short. No identifier. "interrupted", - "items", "kept", "key", # `sweep-decision-option-mismatch`'s label clause, as a BARE BOOLEAN. The @@ -979,6 +983,9 @@ "sentinel_kind", "session_status", "session_vanished", + # `spec-deferrals-harvested` (#913): how many filed findings carried a + # severity the alias table does not know — a count, never the text. + "severity_unrecognized", "signum", # `session-idle` (#680): the wall timestamp the idle stretch began — the # `ts`-shaped float the TUI ages the `· idle ` text from. @@ -1591,6 +1598,7 @@ "spec-deferral-sighting-stale", "spec-deferrals-harvested", "spec-deferrals-malformed", + "spec-deferrals-severity-unrecognized", "spec-deferrals-skipped-out-of-tree", "spec-marker-repair-failed", "spec-marker-repair-skipped",