diff --git a/schemas/campaign-finding.schema.json b/schemas/campaign-finding.schema.json index 44e0aa424..c15e6504b 100644 --- a/schemas/campaign-finding.schema.json +++ b/schemas/campaign-finding.schema.json @@ -25,8 +25,13 @@ "file": { "type": "string", "minLength": 1 }, "field": { "type": "string", "minLength": 1 }, "status": { - "enum": ["applied", "open", "flagged", "resolved"], - "description": "applied: the rule table settled the field and the profile was changed. open: no rule settles it, it waits for review. flagged: a declared file the runs never opened. resolved: a later run found the profile in agreement." + "enum": ["applied", "open", "flagged", "resolved", "rejected"], + "description": "applied: the rule table, or a review of the code, settled the field and the profile was changed. open: no rule settles it, it waits for review. flagged: a declared file the runs never opened. resolved: a later run found the profile in agreement. rejected: a review of the code kept the profile as it is." + }, + "reason": { + "type": "string", + "minLength": 1, + "description": "Written by a review (campaign.py review): what the code does, with its file:line." }, "rule": { "type": "string", "minLength": 1 }, "profile_says": {}, diff --git a/scripts/campaign.py b/scripts/campaign.py index 4537075f7..2861a5192 100644 --- a/scripts/campaign.py +++ b/scripts/campaign.py @@ -27,6 +27,12 @@ as `evidence:`. Any other goes to measurements/findings.jsonl, where the site lists it as open. Each pass adds one line to measurements/campaign-runs.jsonl. +`review` closes an open finding once a maintainer has read the core's code: +`applied` writes the fields it names through the same profile editor, with +the finding's record ids as `evidence:`, and `rejected` keeps the profile. +The reason, with its file:line, goes on the finding's line, and a rejected +finding stays closed while the runs keep showing the same thing. + `references` runs the full set once per core and content with its frames saved as PNG, and lists the screens that stay on for several samples: the candidates a reviewer looks at before writing them into oracle.yml. @@ -45,6 +51,7 @@ Usage: python scripts/campaign.py references --only CORE,... [--system ID] python scripts/campaign.py status [--json] python scripts/campaign.py findings [--all] [--json] + python scripts/campaign.py review KEY --status applied|rejected --reason TEXT [--set FIELD=YAML] python scripts/campaign.py gate python scripts/campaign.py install-timer [--enable] @@ -1424,6 +1431,12 @@ class Campaign: and previous["run_shows"] == verdict.value and previous["profile_says"] == says \ and previous["evidence"] == list(verdict.evidence): return + # A review that read the code and kept the profile stands while the + # runs keep showing the same thing. + if previous and previous["status"] == "rejected" and status == "open" \ + and previous["rule"] == verdict.rule and previous["run_shows"] == verdict.value \ + and previous["profile_says"] == says: + return line = mr.seal_line({ "schema_version": 1, "found_at": now_iso(), "key": key, "emulator": emulator, "system": content.system, "file": entry["name"], "field": verdict.field, @@ -1824,6 +1837,52 @@ def open_findings(root: Path, everything: bool = False) -> list[dict]: if everything or line["status"] in ("open", "flagged")] +REVIEWED = ("applied", "rejected") + + +def review_finding(root: Path, key: str, status: str, reason: str, + updates: dict[str, object] | None = None, index: int | None = None) -> dict: + """Close an open finding after reading the core's code. + + `applied` writes `updates` into the entry the finding names, through the + same editor as the rule table, with the finding's record ids added as + `evidence:` under its field. An applied finding with no update records a + profile changed by hand. `rejected` keeps the profile as it is. Either + way a line with the reason is appended to the findings.""" + if status not in REVIEWED: + raise ValueError(f"status {status!r}: one of {', '.join(REVIEWED)}") + if not reason.strip(): + raise ValueError("a review names its reason") + latest = {line["key"]: line for line in mr.read_campaign(root, mr.FINDINGS)} + finding = latest.get(key) + if finding is None: + raise ValueError(f"no finding {key}") + if finding["status"] not in ("open", "flagged"): + raise ValueError(f"{key} is {finding['status']}, not open") + if updates and status != "applied": + raise ValueError("only an applied review changes the profile") + if updates: + path = root / "emulators" / f"{finding['emulator']}.yml" + entries = yaml.safe_load(path.read_text(encoding="utf-8")).get("files") or [] + matches = [i for i, entry in enumerate(entries) if entry.get("name") == finding["file"]] + if index is not None: + matches = [i for i in matches if i == index] + if len(matches) != 1: + raise ValueError(f"{finding['emulator']}: {len(matches)} entries named {finding['file']}" + + ("" if index is not None else ", pass --index")) + entry = entries[matches[0]] + verdict = rules.Verdict(finding["rule"], finding["field"], finding["run_shows"], + finding["statement"], tuple(finding["evidence"])) + evidence = rules.merge_evidence(entry.get("evidence"), verdict) + campaign_profiles.write_fields(path, matches[0], {**updates, "evidence": evidence}) + body = {key_: value for key_, value in finding.items() if key_ != "id"} + line = mr.seal_line({**body, "found_at": now_iso(), "status": status, "reason": reason}, + "found_at") + with (root / mr.records_path(mr.FINDINGS)).open("a", encoding="utf-8") as handle: + handle.write(mr.line_text(line) + "\n") + return line + + # -------------------------------------------------------------------------- # Command line # -------------------------------------------------------------------------- @@ -1866,6 +1925,14 @@ def main(argv: Sequence[str] | None = None) -> int: findings_p = sub.add_parser("findings", help="profile claims the runs contradict, open for review") findings_p.add_argument("--all", action="store_true", help="applied and resolved too") findings_p.add_argument("--json", action="store_true", help="the findings as JSON") + review_p = sub.add_parser("review", help="close an open finding after reading the code") + review_p.add_argument("key", help="emulator/system/file/field, as `findings` prints it") + review_p.add_argument("--status", required=True, choices=REVIEWED) + review_p.add_argument("--reason", required=True, + help="what the code does, with its file:line at the profile's pin") + review_p.add_argument("--set", action="append", default=[], metavar="FIELD=YAML", + help="with --status applied, a field to write into the entry") + review_p.add_argument("--index", type=int, help="the entry's position when several share its name") sub.add_parser("gate", help="tests, schemas, verify and pack of the changed profiles") timer_p = sub.add_parser("install-timer", help="opt-in: write the autorun units, the campaign job's timer among them") @@ -1879,6 +1946,21 @@ def main(argv: Sequence[str] | None = None) -> int: if args.command == "install-timer": command = [sys.executable, str(ROOT / "scripts" / "autorun.py"), "install-timers"] return subprocess.run(command + (["--enable"] if args.enable else []), check=False).returncode + if args.command == "review": + updates: dict[str, object] = {} + for item in args.set: + name, sep, value = item.partition("=") + if not sep or not name: + parser.error(f"--set {item!r}: FIELD=YAML") + updates[name] = yaml.safe_load(value) + try: + line = review_finding(paths.root, args.key, args.status, args.reason, updates, + args.index) + except (ValueError, campaign_profiles.ProfileEditError) as exc: + print(f"campaign: {exc}", file=sys.stderr) + return 1 + print(f"{line['status']} {line['key']} [{line['id']}]") + return 0 if args.command == "findings": lines = open_findings(paths.root, args.all) if args.json: diff --git a/scripts/campaign_profiles.py b/scripts/campaign_profiles.py index 4efee99fc..d935076b1 100644 --- a/scripts/campaign_profiles.py +++ b/scripts/campaign_profiles.py @@ -24,6 +24,8 @@ ANCHORS = { "path": ("name",), "aliases": ("path", "name"), "validation": ("sha1", "md5", "crc32", "size", "name"), + "on_mismatch": ("validation", "sha1", "md5", "crc32", "size", "name"), + "found_by": ("path", "name"), } diff --git a/scripts/campaign_rules.py b/scripts/campaign_rules.py index 89726a3d1..931285aed 100644 --- a/scripts/campaign_rules.py +++ b/scripts/campaign_rules.py @@ -28,7 +28,9 @@ name (zeros of its size, its first half, another region's dump). core reports a BIOS error, or when it boots without the BIOS screens the right file shows (it fell back to a built-in BIOS). It is used when it turns the run into a hang or a crash that the run without any file does not -show. Anything else settles nothing. Of validation, the table writes only a +show. Anything else settles nothing. A check the entry declares with +`on_mismatch: warn` is one the code only logs: a wrong file that runs +agrees with it. Of validation, the table writes only a missing `size`. A contradicted content check goes to review: which hash a core computes is read in its code. @@ -48,7 +50,7 @@ UNKNOWN = "unknown" FAILS = (BIOS_ERROR, CRASH, HANG) REFUSED = "refused" USED = "used" -CONTENT_CHECKS = ("md5", "sha1", "sha256", "crc32", "adler32") +CONTENT_CHECKS = ("md5", "sha1", "sha256", "crc32", "adler32", "signature") # A field an entry leaves out reads as this value. FIELD_DEFAULTS: dict[str, object] = {"hle_fallback": False, "path": None, "aliases": [], @@ -185,6 +187,17 @@ def declared_checks(entry: dict) -> tuple[set[str], bool]: return {str(v) for v in value}, True +def warned_checks(entry: dict, checks: set[str]) -> set[str]: + """The declared checks whose failure the code only logs (`on_mismatch: + warn`, for all of them or check by check): a wrong file still runs.""" + level = entry.get("on_mismatch") + if level == "warn": + return set(checks) + if isinstance(level, dict): + return {check for check, value in level.items() if value == "warn"} & checks + return set() + + def rule_validation(zeros: Run | None, half: Run | None, none: Run, present: Run, entry: dict) -> Verdict | None: """Same-size and short wrong files against the right one: validation.""" @@ -199,19 +212,20 @@ def rule_validation(zeros: Run | None, half: Run | None, none: Run, present: Run if content is None and size is None: return None declared, editable = declared_checks(entry) + enforced = declared - warned_checks(entry, declared) evidence = ids(*(run for run in (none, present, zeros, half) if run)) said = sorted(declared) shown = [] problems = [] if content is not None: shown.append("checks the content" if content else "does not check the content") - if content != bool(declared & set(CONTENT_CHECKS)): + if content != bool(enforced & set(CONTENT_CHECKS)): problems.append("a same-size file of zeros is " + ("refused" if content else "run") + f", the profile declares validation {said}") missing_size = size is True and "size" not in declared if size is not None: shown.append("checks the size" if size else "does not check the size") - if size is False and "size" in declared: + if size is False and "size" in enforced: problems.append(f"the first half of {name} is run, the profile declares a size check") statement = f"the core {' and '.join(shown)} of {name}" # What the runs show, for a finding beside what the profile says. diff --git a/tests/test_campaign.py b/tests/test_campaign.py index 947ad3ade..b3001027a 100644 --- a/tests/test_campaign.py +++ b/tests/test_campaign.py @@ -222,6 +222,25 @@ class RuleTable(unittest.TestCase): self.assertIsNone(rules.rule_validation(run(rules.UNKNOWN), None, NONE_BOOTS, PRESENT, {"name": "b"})) + def test_a_warned_check_lets_a_wrong_file_run(self) -> None: + """Beetle PSX logs a wrong SHA1 and boots the file (libretro.c:853-860), + PCSX ReARMed refuses an image without the kernel's first instruction + or "PS compatible" (frontend/libretro.c:3646-3651).""" + refused = run(rules.BIOS_ERROR) + used = run(rules.HANG) + warned = {"name": "b", "validation": ["sha1"], "on_mismatch": "warn"} + self.assertEqual(rules.rule_validation(used, used, NONE_BOOTS, PRESENT, warned).rule, + "validation_agrees") + by_check = {"name": "b", "validation": ["size", "md5"], "on_mismatch": {"md5": "warn"}} + self.assertEqual(rules.rule_validation(used, refused, NONE_BOOTS, PRESENT, by_check).rule, + "validation_agrees") + self.assertEqual(rules.rule_validation(used, None, NONE_BOOTS, PRESENT, + {"name": "b", "validation": ["sha1"]}).rule, + "validation_contradicted") + signature = {"name": "b", "validation": ["size", "signature"]} + self.assertEqual(rules.rule_validation(refused, None, NONE_BOOTS, PRESENT, signature).rule, + "validation_agrees") + def test_path_and_alias(self) -> None: self.assertIsNone(rules.rule_path(self.ENTRY, "bios.bin", rules.Opened("bios.bin", self.RID))) moved = rules.rule_path(self.ENTRY, "bios.bin", rules.Opened("sys/bios.bin", self.RID)) @@ -900,6 +919,53 @@ class Findings(Workspace): for line in mr.read_campaign(self.root, name): self.assertEqual(list(Draft202012Validator(schema).iter_errors(line)), [], name) + def contradicted(self) -> rules.Verdict: + return rules.rule_validation(rules.Run(rules.HANG, self.RID, "zeros"), None, + self.none(rules.BOOTS), self.present(), + {**self.entry(), "validation": ["md5"]}) + + def test_a_review_applies_through_the_profile_editor(self) -> None: + self.c.settle("fake", self.content, self.core, 0, self.entry(), [self.contradicted()]) + key = self.lines()[-1]["key"] + with self.assertRaises(ValueError): + campaign.review_finding(self.root, key, "applied", " ") + line = campaign.review_finding(self.root, key, "applied", "the core logs the md5 (x.c:1)", + {"on_mismatch": "warn"}) + self.assertEqual((line["status"], line["reason"]), ("applied", "the core logs the md5 (x.c:1)")) + entry = self.entry() + self.assertEqual(entry["on_mismatch"], "warn") + self.assertEqual(entry["evidence"], {"validation": [self.RID]}) + text = self.profile.read_text() + self.assertLess(text.index("validation: [size]"), text.index("on_mismatch")) + self.assertEqual(campaign.open_findings(self.root), []) + with self.assertRaises(ValueError): + campaign.review_finding(self.root, key, "rejected", "again") + try: + from jsonschema import Draft202012Validator + except ImportError: + self.skipTest("jsonschema is a tooling dependency") + schema = json.loads((REPO_ROOT / "schemas" / "campaign-finding.schema.json").read_text()) + for found in self.lines(): + self.assertEqual(list(Draft202012Validator(schema).iter_errors(found)), []) + + def test_a_rejected_finding_stays_closed_on_the_same_run(self) -> None: + verdict = self.contradicted() + self.c.settle("fake", self.content, self.core, 0, self.entry(), [verdict]) + key = self.lines()[-1]["key"] + with self.assertRaises(ValueError): + campaign.review_finding(self.root, key, "rejected", "kept", {"required": False}) + out = io.StringIO() + with contextlib.redirect_stdout(out), unittest.mock.patch.object(campaign.Paths, "default", + return_value=self.paths): + self.assertEqual(campaign.main(["review", key, "--status", "rejected", + "--reason", "the md5 is checked (x.c:2)"]), 0) + before = self.profile.read_text() + later = rules.Verdict(verdict.rule, verdict.field, verdict.value, verdict.statement, + ("2026-10-28-000000000000",)) + self.c.settle("fake", self.content, self.core, 0, self.entry(), [later]) + self.assertEqual([line["status"] for line in self.lines()], ["open", "rejected"]) + self.assertEqual(self.profile.read_text(), before) + def test_the_batch_commit_plan(self) -> None: verdicts = rules.rule_need(self.none(rules.BOOTS), self.present(), self.entry()) self.c.settle("fake", self.content, self.core, 0, self.entry(), verdicts)