From 144d6ce37ffbe2e2f2e1fd9c5b9edbfeaf6075c0 Mon Sep 17 00:00:00 2001 From: Asterios Raptis Date: Mon, 12 Oct 2026 18:11:52 +0200 Subject: [PATCH] chore(tooling): an accepted warning raised to an error is not "resolved" (template#101) Byte-identical copy of the template-owned files from adaptive-learner-content-template#104: the engine runner prints the engine's rename table and errors with their rule ID, quality_state.py follows an accepted record to a raised rule, and the warnings summary also runs after a failed gate. Co-Authored-By: Claude Opus 5.5 (1M context) --- .github/workflows/engine-validate.yml | 13 +++- scripts/quality_state.py | 95 ++++++++++++++++++------ scripts/validate_with_engine.mjs | 18 ++++- tests/test_quality_state.py | 101 +++++++++++++++++++++++++- 4 files changed, 196 insertions(+), 31 deletions(-) diff --git a/.github/workflows/engine-validate.yml b/.github/workflows/engine-validate.yml index 85d906f..2fd5d96 100644 --- a/.github/workflows/engine-validate.yml +++ b/.github/workflows/engine-validate.yml @@ -35,6 +35,7 @@ jobs: # itself); the version range matches the engine's own dependency. run: npm install --no-save "learn-content-engine@$(cat schema/engine-version.txt)" "yaml@^2.9.0" - name: Self-test (the gate must bite before it may pass) + id: selftest run: node scripts/validate_with_engine.mjs --self-test - name: Validate all lessons and manifests with the engine run: node scripts/validate_with_engine.mjs . @@ -45,12 +46,16 @@ jobs: # otherwise reach nobody unless someone runs `make lint-warnings` by # hand, so a pin bump changes a version number and nothing on screen. # The findings go into the job summary, readable without opening the - # log. + # log. It also runs when the gate above failed: a pin that raises an + # accepted warning to an error turns the gate red, and the summary is + # where that is explained (template#101). + if: ${{ !cancelled() && steps.selftest.outcome == 'success' }} continue-on-error: true run: | - # Keep the runner's exit code: it is 0 when it only finds warnings, so - # anything else is a crash, and a crash must show red instead of a - # green step with an empty table that reads like "no warnings". + # Keep the runner's exit code: 0 without errors, 1 with errors (the + # gate above has failed on them already), anything else a crash. A + # crash must show red instead of a green step with an empty table + # that reads like "no warnings". set +e node scripts/validate_with_engine.mjs --warnings . > /tmp/lint-warnings.txt 2>&1 status=$? diff --git a/scripts/quality_state.py b/scripts/quality_state.py index da99456..90911fd 100755 --- a/scripts/quality_state.py +++ b/scripts/quality_state.py @@ -15,7 +15,11 @@ of the elements it covers. A decision that cannot be read off the state is a backlog three months later: the warning summary shows the decision next to the number, and compares the recorded elements with the current ones, so - "still the same 105" is a set comparison, not a count. + "still the same 105" is a set comparison, not a count. An error cannot be + accepted: it blocks the gate. When the engine raises an accepted rule to an + error, its ID changes tier (``W-X`` becomes ``E-X``); the runner prints the + engine's rename table, so the record is still recognized under its old name + and the summary reports the raise instead of calling the findings resolved. Commands (all write Markdown for the job summary to stdout): @@ -40,8 +44,10 @@ TABLE_ROWS = 50 FINDING_LINE = re.compile(r"^(?P[^ ].*?):(?P\d+): ") -WARN_FILE = re.compile(r"^WARN (?P\S+)") -WARN_ENTRY = re.compile(r"^\s+\[(?PW-[A-Z0-9-]+)\] (?P\S+)") +RULE_ID = r"[EW]-[A-Z0-9-]+" +ISSUE_FILE = re.compile(r"^(?:WARN|✗) (?P\S+)") +ISSUE_ENTRY = re.compile(rf"^\s+\[(?P{RULE_ID})\] (?P\S+)") +RENAMED_LINE = re.compile(rf"^RENAMED (?P{RULE_ID}) -> (?P{RULE_ID})$") STEP_POINTER = re.compile(r"^/steps/(?P\d+)/exercise") @@ -64,6 +70,8 @@ def load_state(path: Path = STATE_PATH) -> dict: missing = [key for key in ("rule", "since", "reason", "ids") if key not in entry] if missing: raise ValueError(f"{path}: accepted warning {entry.get('rule', '?')} needs {', '.join(missing)}") + if is_error(entry["rule"]): + raise ValueError(f"{path}: {_refusal(entry['rule'])}") return state @@ -121,17 +129,39 @@ def unchanged(self) -> bool: return not self.new and not self.resolved -def parse_warnings(output: str) -> list[Finding]: - """Every author warning in the engine runner's --warnings output.""" +def is_error(rule: str) -> bool: + """Whether a rule ID names an error: its tier is its prefix, ``E-`` blocks.""" + return rule.startswith("E-") + + +def _refusal(rule: str) -> str: + return f"{rule} is an error: it blocks the gate and cannot be accepted" + + +def parse_findings(output: str) -> list[Finding]: + """Every finding in the engine runner's --warnings output, warnings + (``WARN`` blocks) and errors (``✗`` blocks) alike. An error line without a + rule ID, as an older runner prints it, is skipped.""" findings, current = [], None for line in output.splitlines(): - if match := WARN_FILE.match(line): + if match := ISSUE_FILE.match(line): current = match.group("path") - elif current and (match := WARN_ENTRY.match(line)): + elif current and (match := ISSUE_ENTRY.match(line)): findings.append(Finding(current, match.group("rule"), match.group("pointer"))) return findings +def parse_renames(output: str) -> dict[str, str]: + """The engine's rename table as the runner prints it: every retired rule + ID mapped to the ID its rule carries now. Empty when the runner printed + none; a rename is never guessed from a shared name.""" + return { + match.group("retired"): match.group("current") + for line in output.splitlines() + if (match := RENAMED_LINE.match(line)) + } + + def element_key(finding: Finding, root: Path) -> str: """The identity of the element a warning is about: the exercise's stable_id (or its id before minting) when the pointer lands on one, else @@ -165,21 +195,36 @@ def _status_text(status: AcceptedStatus) -> str: return "; ".join(parts) +def _raised_text(current_rule: str, count: int) -> str: + if count == 0: + return f"raised to `{current_rule}`: no findings left, remove the record" + noun = "finding" if count == 1 else "findings" + return f"raised to `{current_rule}`: an error blocks the gate, the acceptance no longer applies ({count} {noun})" + + def warnings_report(output: str, state: dict, root: Path = Path(".")) -> str: """Markdown for the job summary: warnings per rule, each accepted rule with - its decision next to the number and a set comparison against the record.""" - findings = parse_warnings(output) + its decision next to the number and a set comparison against the record. + An accepted rule the engine has since raised to an error keeps its row + under the recorded name and says so; other errors stay out of the table.""" + findings = parse_findings(output) + renames = parse_renames(output) per_rule = Counter(finding.rule for finding in findings) accepted = {entry["rule"]: entry for entry in state.get("accepted_warnings", [])} + warned = {rule for rule in per_rule if not is_error(rule)} lines = ["### Author lints (warnings, non-blocking)", "", "| rule | count | accepted | status |", "|---|---|---|---|"] - for rule in sorted(set(per_rule) | set(accepted), key=lambda r: (-per_rule[r], r)): + for rule in sorted(warned | set(accepted), key=lambda r: (-per_rule[renames.get(r, r)], r)): entry = accepted.get(rule) if entry is None: lines.append(f"| `{rule}` | {per_rule[rule]} | - | |") continue - current = {element_key(f, root) for f in findings if f.rule == rule} - status = compare_accepted(entry, current) - lines.append(f"| `{rule}` | {per_rule[rule]} | {len(entry['ids'])} since {entry['since']} | {_status_text(status)} |") + current_rule = renames.get(rule, rule) + if is_error(current_rule): + status_text = _raised_text(current_rule, per_rule[current_rule]) + else: + current = {element_key(f, root) for f in findings if f.rule == rule} + status_text = _status_text(compare_accepted(entry, current)) + lines.append(f"| `{rule}` | {per_rule[current_rule]} | {len(entry['ids'])} since {entry['since']} | {status_text} |") if accepted: lines += ["", "**Accepted warnings**", ""] for rule, entry in sorted(accepted.items()): @@ -189,13 +234,18 @@ def warnings_report(output: str, state: dict, root: Path = Path(".")) -> str: def accept_warnings(state: dict, rule: str, output: str, root: Path, reason: str, since: str, decision: str | None = None) -> dict: - """The state with the current findings of ``rule`` recorded as accepted.""" - ids = sorted({element_key(f, root) for f in parse_warnings(output) if f.rule == rule}) - entry = {"rule": rule, "count": len(ids), "since": since, "reason": reason} + """The state with the current findings of ``rule`` recorded as accepted, + under the ID the rule carries now. Raises ValueError for an error, also for + the old name of a rule the engine has since raised to one.""" + current_rule = parse_renames(output).get(rule, rule) + if is_error(current_rule): + raise ValueError(_refusal(current_rule)) + ids = sorted({element_key(f, root) for f in parse_findings(output) if f.rule == current_rule}) + entry = {"rule": current_rule, "count": len(ids), "since": since, "reason": reason} if decision: entry["decision"] = decision entry["ids"] = ids - kept = [e for e in state.get("accepted_warnings", []) if e["rule"] != rule] + kept = [e for e in state.get("accepted_warnings", []) if e["rule"] != current_rule] return {**state, "accepted_warnings": kept + [entry]} @@ -225,12 +275,15 @@ def main(argv: list[str]) -> int: if args.command == "warnings": sys.stdout.write(warnings_report(args.output.read_text(encoding="utf-8"), state)) return 0 - updated = accept_warnings(state, args.rule, args.output.read_text(encoding="utf-8"), Path("."), - reason=args.reason, since=args.since, decision=args.decision) + try: + updated = accept_warnings(state, args.rule, args.output.read_text(encoding="utf-8"), Path("."), + reason=args.reason, since=args.since, decision=args.decision) + except ValueError as refusal: + parser.error(str(refusal)) STATE_PATH.parent.mkdir(parents=True, exist_ok=True) STATE_PATH.write_text(json.dumps(updated, ensure_ascii=False, indent=2) + "\n", encoding="utf-8") - entry = next(e for e in updated["accepted_warnings"] if e["rule"] == args.rule) - print(f"recorded {entry['count']} accepted {args.rule} finding(s) in {STATE_PATH}") + entry = updated["accepted_warnings"][-1] + print(f"recorded {entry['count']} accepted {entry['rule']} finding(s) in {STATE_PATH}") return 0 diff --git a/scripts/validate_with_engine.mjs b/scripts/validate_with_engine.mjs index 6b56d71..22caabc 100644 --- a/scripts/validate_with_engine.mjs +++ b/scripts/validate_with_engine.mjs @@ -29,9 +29,20 @@ * `--warnings` also lists the author lints (W-*) that never block. It runs * through the SAME extension registry as the error gate, so ext: lessons are * validated instead of refused - `make lint-warnings` used to shell out to the - * bare CLI (no registry) and died on ext content (content-test#71). + * bare CLI (no registry) and died on ext content (content-test#71). It also + * prints the engine's rename table (`RENAMED W-X -> E-X`): raising a rule + * renames it, and scripts/quality_state.py follows an accepted warning to its + * new ID instead of reporting it resolved (template#101). Errors carry their + * rule ID in both modes, in the same `[ID] path message` form as warnings. */ -import { validateLesson, validateLessonQuality, validateManifest, validateManifestPair } from "learn-content-engine"; +import { + currentRuleId, + RENAMED_RULE_IDS, + validateLesson, + validateLessonQuality, + validateManifest, + validateManifestPair, +} from "learn-content-engine"; import { readFileSync, readdirSync, statSync } from "node:fs"; import { join, relative } from "node:path"; import { parse as parseYaml } from "yaml"; @@ -372,6 +383,7 @@ function validateAll(repoRoot, { showWarnings = false } = {}) { (showWarnings ? `, ${totalWarnings} warning(s)` : ""), ); if (showWarnings) { + for (const retired of Object.keys(RENAMED_RULE_IDS)) console.log(`RENAMED ${retired} -> ${currentRuleId(retired)}`); for (const w of warned) { console.log(`\nWARN ${w.file}`); for (const issue of w.warnings) console.log(` [${issue.id}] ${issue.path} ${issue.message}`); @@ -379,7 +391,7 @@ function validateAll(repoRoot, { showWarnings = false } = {}) { } for (const p of problems) { console.error(`\n✗ ${p.file}`); - for (const e of p.errors) console.error(` ${e.path}: ${e.message}`); + for (const e of p.errors) console.error(` [${e.id}] ${e.path} ${e.message}`); } return problems.length === 0 ? 0 : 1; } diff --git a/tests/test_quality_state.py b/tests/test_quality_state.py index 00ec500..be6c8ee 100644 --- a/tests/test_quality_state.py +++ b/tests/test_quality_state.py @@ -114,7 +114,7 @@ def _lesson(tmp_path): def test_warnings_are_parsed_per_file(tmp_path): - findings = quality_state.parse_warnings(WARNINGS_OUTPUT) + findings = quality_state.parse_findings(WARNINGS_OUTPUT) assert [(f.path, f.rule, f.pointer) for f in findings] == [ ("sets/de/x/lessons/01-a.json", "W-CLOZE-NO-CARRIER", "/steps/1/exercise/sentence"), ("sets/de/x/lessons/01-a.json", "W-CARD-UNUSED", "/cards"), @@ -123,13 +123,13 @@ def test_warnings_are_parsed_per_file(tmp_path): def test_a_finding_on_an_exercise_is_identified_by_its_stable_id(tmp_path): root = _lesson(tmp_path) - finding = quality_state.parse_warnings(WARNINGS_OUTPUT)[0] + finding = quality_state.parse_findings(WARNINGS_OUTPUT)[0] assert quality_state.element_key(finding, root) == "ex-abc" def test_a_finding_without_an_exercise_falls_back_to_file_and_pointer(tmp_path): root = _lesson(tmp_path) - finding = quality_state.parse_warnings(WARNINGS_OUTPUT)[1] + finding = quality_state.parse_findings(WARNINGS_OUTPUT)[1] assert quality_state.element_key(finding, root) == "sets/de/x/lessons/01-a.json#/cards" @@ -176,3 +176,98 @@ def test_accepting_records_the_ids_count_date_and_reason(tmp_path): "reason": "kept on purpose", "ids": ["ex-abc"], } + + +# --- a rule raised to an error (template#101) --------------------------------- +# Raising a rule renames it: the tier is the prefix of its ID, so 0.38.0 turned +# W-MANIFEST-ENTRY-MISMATCH into E-MANIFEST-ENTRY-MISMATCH (engine#256). The +# runner prints the engine's rename table (RENAMED_RULE_IDS, followed with +# currentRuleId) and the errors with their IDs, so a record kept under the old +# name is still recognized. This output is the example in the template README. + +RAISED_OUTPUT = """engine-validate: 1 lesson(s), 2 manifest(s) checked - 1 file(s) with errors, 0 warning(s) +RENAMED W-MANIFEST-ENTRY-MISMATCH -> E-MANIFEST-ENTRY-MISMATCH + +✗ sets/de/x/manifest.yaml + [E-MANIFEST-ENTRY-MISMATCH] /sets/0/description set 'x': 'description' is "Root text" in the root manifest but "Set text" in the set manifest; the two must be identical +""" + +RAISED_RECORD = { + "rule": "W-MANIFEST-ENTRY-MISMATCH", + "since": "2026-09-23", + "reason": "the root text is shorter on purpose", + "ids": ["sets/de/x/manifest.yaml#/sets/0/description"], +} + + +def test_errors_are_parsed_with_their_rule_ids(): + [finding] = quality_state.parse_findings(RAISED_OUTPUT) + assert (finding.path, finding.rule, finding.pointer) == ( + "sets/de/x/manifest.yaml", + "E-MANIFEST-ENTRY-MISMATCH", + "/sets/0/description", + ) + + +def test_the_renames_are_read_from_the_runner_output(): + assert quality_state.parse_renames(RAISED_OUTPUT) == {"W-MANIFEST-ENTRY-MISMATCH": "E-MANIFEST-ENTRY-MISMATCH"} + + +def test_without_a_rename_table_there_are_no_renames(): + # An older runner prints no table: nothing is renamed, nothing is guessed + # from a shared suffix. + assert quality_state.parse_renames(WARNINGS_OUTPUT) == {} + + +def test_an_accepted_warning_raised_to_an_error_is_not_reported_as_resolved(tmp_path): + # The reproduction: the record names the old ID, the findings carry the new + # one. Compared under the old ID, every recorded element looked resolved. + report = quality_state.warnings_report(RAISED_OUTPUT, {"accepted_warnings": [RAISED_RECORD]}, tmp_path) + assert "resolved" not in report + assert ( + "| `W-MANIFEST-ENTRY-MISMATCH` | 1 | 1 since 2026-09-23 | raised to `E-MANIFEST-ENTRY-MISMATCH`:" + " an error blocks the gate, the acceptance no longer applies (1 finding) |" + ) in report + + +def test_a_raised_rule_without_findings_asks_for_the_record_to_go(tmp_path): + output = RAISED_OUTPUT.split("\n✗")[0] + report = quality_state.warnings_report(output, {"accepted_warnings": [RAISED_RECORD]}, tmp_path) + assert "raised to `E-MANIFEST-ENTRY-MISMATCH`: no findings left, remove the record |" in report + + +def test_errors_of_rules_nobody_accepted_stay_out_of_the_warnings_table(tmp_path): + output = RAISED_OUTPUT.replace("E-MANIFEST-ENTRY-MISMATCH]", "E-SCHEMA]") + report = quality_state.warnings_report(output, {}, tmp_path) + assert "E-SCHEMA" not in report + + +def test_an_error_cannot_be_accepted(tmp_path): + with pytest.raises(ValueError, match="cannot be accepted"): + quality_state.accept_warnings( + {}, "E-MANIFEST-ENTRY-MISMATCH", RAISED_OUTPUT, tmp_path, reason="r", since="2026-10-12" + ) + + +def test_the_old_name_of_a_raised_rule_cannot_be_accepted_either(tmp_path): + with pytest.raises(ValueError, match="E-MANIFEST-ENTRY-MISMATCH"): + quality_state.accept_warnings( + {}, "W-MANIFEST-ENTRY-MISMATCH", RAISED_OUTPUT, tmp_path, reason="r", since="2026-10-12" + ) + + +def test_the_old_name_of_a_lowered_rule_is_recorded_under_its_new_one(tmp_path): + # Lowering renames too (E-X becomes W-X); the record carries the ID the + # findings carry, or it would compare against nothing from the next run on. + root = _lesson(tmp_path) + output = "RENAMED E-CLOZE-NO-CARRIER -> W-CLOZE-NO-CARRIER\n" + WARNINGS_OUTPUT + state = quality_state.accept_warnings({}, "E-CLOZE-NO-CARRIER", output, root, reason="r", since="2026-10-12") + [entry] = state["accepted_warnings"] + assert (entry["rule"], entry["ids"]) == ("W-CLOZE-NO-CARRIER", ["ex-abc"]) + + +def test_a_recorded_error_fails_loudly(tmp_path): + path = tmp_path / "state.json" + path.write_text(json.dumps({"accepted_warnings": [{**RAISED_RECORD, "rule": "E-MANIFEST-ENTRY-MISMATCH"}]})) + with pytest.raises(ValueError, match="cannot be accepted"): + quality_state.load_state(path)