From 30c8a3136f697139c1a755b053483a745fe06636 Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Mon, 28 Sep 2026 17:14:58 +0200 Subject: [PATCH] fix(gate-19): read the scenarios of an open change Gate 19 read openspec/specs/ only. A change's scenarios live in openspec/changes//specs//spec.md until it is archived, so the PR that writes a change and the PRs that implement it came back EMPTY SCOPE and the scenarios reached the gate after the code had merged. A touched delta spec is now in scope, and so is every delta spec of an open change the diff touches any file in (an implementation PR ticks tasks.md). A full sweep reads open changes too. A scenario a MODIFIED block restates from the main spec is not new, REMOVED and RENAMED blocks owe no test, and openspec/changes/archive/ is skipped. The shape test now calls the real selector instead of a copy of it. Fixes ConductionNL/hydra#711 --- hydra-gates/README.md | 8 + hydra-gates/scripts/lib/check_e2e_coverage.py | 169 ++++++++++++-- .../scripts/lib/test_check_e2e_coverage.py | 209 ++++++++++++++++-- hydra-gates/scripts/run-hydra-gates.sh | 15 +- 4 files changed, 367 insertions(+), 34 deletions(-) diff --git a/hydra-gates/README.md b/hydra-gates/README.md index 77764f03..725484df 100644 --- a/hydra-gates/README.md +++ b/hydra-gates/README.md @@ -439,6 +439,14 @@ those an `@e2e exclude` does not count — they are reported on their own lines `new scenario without a test` and counted separately on the summary line. A scenario written today is written with its test; an exclusion is for a scenario that predates the suite. Existing scenarios keep their exclusion semantics. +Since hydra#711 (2026-09-28) the gate also reads the scenarios of **open +changes**: a touched `openspec/changes//specs//spec.md` is in scope, +and so is every delta spec of an open change the diff touches any file in, so +an implementation PR that ticks the change's `tasks.md` is checked against the +change's scenarios. A delta scenario's ref is `::`, the ref it keeps +after archive. A scenario a `MODIFIED` block restates from the main spec is not +new, `REMOVED` and `RENAMED` blocks owe no test, and `openspec/changes/archive/` +is skipped. Before this, both kinds of PR came back `EMPTY SCOPE`. A delta gate also has to decide what *counts* as a change, and a plain `git diff` answers "a line moved". gate-16 therefore compares each changed file against its diff --git a/hydra-gates/scripts/lib/check_e2e_coverage.py b/hydra-gates/scripts/lib/check_e2e_coverage.py index e539199b..c7782184 100644 --- a/hydra-gates/scripts/lib/check_e2e_coverage.py +++ b/hydra-gates/scripts/lib/check_e2e_coverage.py @@ -26,6 +26,16 @@ openspec/specs//spec.md -> (parent directory) openspec/specs/.md -> (file stem) +The delta spec of an OPEN CHANGE is read too (hydra#711):: + + openspec/changes//specs//spec.md -> + +In diff mode a touched delta is in scope, and so is every delta of an open +change the diff touches any file in. ``REMOVED`` and ``RENAMED`` blocks owe no +test, and ``openspec/changes/archive/`` is history and is skipped. Until +2026-09-28 the gate read ``openspec/specs/`` only, so a change's scenarios were +first checked after the code meant to satisfy them had merged. + The flat shape was invisible to this gate until 2026-09-08. Measured across the 21 core apps that day, planninq kept 153 of its 224 scenarios in 13 flat files and the gate reported 70. Those scenarios were not counted, not @@ -390,6 +400,92 @@ def spec_files(spec_root: Path) -> list[Path]: return sorted(set(found)) +# --------------------------------------------------------------------------- +# THE SCENARIOS OF AN OPEN CHANGE (hydra#711) +# --------------------------------------------------------------------------- +# +# A change's scenarios live in `openspec/changes//specs//spec.md` +# until the change is archived, and only then land in `openspec/specs/`. This +# gate read `openspec/specs/` alone, so the PR that writes a change and the PRs +# that implement it reported EMPTY SCOPE, and a scenario reached the gate only +# after the code meant to satisfy it had merged. Counted on 28 Sep 2026: 3,897 +# scenarios in open changes across six repos, none of them read here, and no +# `@e2e exclude` reason written inside a change read either. +# +# A delta scenario's ref is `::`, the ref it keeps once archived, so +# an anchor written against the change keeps working after the archive. +# `openspec/changes/archive/` is history, not an open change, and is skipped. +# A `## REMOVED Requirements` (or RENAMED) block describes what goes away, so +# its scenarios are not asked for a test. +_CHANGES_DIR = "openspec/changes/" +_ARCHIVE_DIR = "archive" +_DELTA_DROP_RE = re.compile(r"^##\s+(?:REMOVED|RENAMED)\s+Requirements\b", re.IGNORECASE) +_H2_RE = re.compile(r"^##\s") + + +def _open_change_of(rel: str) -> str | None: + """The change name a repo-relative path sits in, or None. + + ``openspec/changes/add-x/tasks.md`` -> ``add-x``. An archived change and a + path outside ``openspec/changes//`` return None. + """ + if not rel.startswith(_CHANGES_DIR): + return None + parts = rel[len(_CHANGES_DIR):].split("/") + if len(parts) < 2 or parts[0] in ("", _ARCHIVE_DIR): + return None + return parts[0] + + +def is_change_spec_path(rel: str) -> bool: + """True for ``openspec/changes//specs//spec.md`` of an open change.""" + change = _open_change_of(rel) + if change is None: + return False + rest = rel[len(_CHANGES_DIR) + len(change) + 1:].split("/") + return len(rest) == 3 and rest[0] == "specs" and rest[2] == "spec.md" and rest[1] != "" + + +def _is_change_spec_file(spec_path: Path) -> bool: + """Path form of :func:`is_change_spec_path`, read from the path's own parts.""" + parts = spec_path.parts + return ( + len(parts) >= 6 + and parts[-1] == "spec.md" + and parts[-3] == "specs" + and parts[-5] == "changes" + and parts[-6] == "openspec" + and parts[-4] != _ARCHIVE_DIR + ) + + +def change_spec_files(app_dir: Path) -> list[Path]: + """Every delta spec of every open change under ``app_dir``.""" + changes = app_dir / "openspec" / "changes" + if not changes.is_dir(): + return [] + return sorted( + p for p in changes.glob("*/specs/*/spec.md") + if p.is_file() and p.parts[-4] != _ARCHIVE_DIR + ) + + +def strip_delta_removals(text: str) -> str: + """Drop ``## REMOVED Requirements`` and ``## RENAMED Requirements`` blocks. + + A removed requirement is not built, so its scenarios owe no test. The + block runs to the next ``## `` heading. + """ + out: list[str] = [] + dropping = False + for line in text.splitlines(): + if _H2_RE.match(line): + dropping = bool(_DELTA_DROP_RE.match(line)) + if not dropping: + out.append(line) + return "\n".join(out) + ("\n" if text.endswith("\n") else "") + + def spec_name_collisions(spec_root: Path) -> dict[str, list[str]]: """Find spec names claimed by more than one file. @@ -456,6 +552,8 @@ def parse_spec_scenarios(spec_path: Path) -> list[dict]: text = spec_path.read_text(encoding="utf-8") except OSError: return [] + if _is_change_spec_file(spec_path): + text = strip_delta_removals(text) return parse_spec_text(text, spec_name) @@ -2100,28 +2198,54 @@ def _git(args: list[str], cwd: Path) -> str: return "" -def changed_spec_files(base_ref: str, app_dir: Path) -> set[str]: - """Return relative paths of spec.md files touched in the PR diff.""" - diff = _git(["diff", "-U0", "--diff-filter=ACMR", "--name-only", - f"{base_ref}...HEAD"], app_dir) - if not diff.strip(): - diff = _git(["diff", "-U0", "--diff-filter=ACMR", "--name-only", - base_ref], app_dir) - paths: set[str] = set() - for line in diff.splitlines(): +def select_spec_paths(paths) -> set[str]: + """The spec sources among repo-relative paths: both main shapes and open change deltas. + + Pure, so the selection a diff gets is the selection a test asserts. + """ + keep: set[str] = set() + for line in paths: line = line.strip() - if not line.startswith("openspec/specs/") or not line.endswith(".md"): + if not line.endswith(".md"): + continue + if is_change_spec_path(line): + keep.add(line) + continue + if not line.startswith("openspec/specs/"): continue rest = line[len("openspec/specs/"):] depth = rest.count("/") # `/spec.md` — the classic shape, exactly one level down. if depth == 1 and rest.endswith("/spec.md"): - paths.add(line) + keep.add(line) continue # `.md` — the flat shape, which this gate used to skip entirely. # README.md is documentation about the directory, not a spec. if depth == 0 and rest.lower() not in _NOT_A_SPEC: - paths.add(line) + keep.add(line) + return keep + + +def changed_spec_files(base_ref: str, app_dir: Path) -> set[str]: + """Return relative paths of spec files the PR diff brings into scope. + + A spec file the diff touches, main or change delta. And every delta spec of + an open change the diff touches ANY file in: an implementation PR ticks the + change's ``tasks.md`` and does not edit its specs, and it is the PR those + scenarios are about (hydra#711). + """ + diff = _git(["diff", "-U0", "--diff-filter=ACMR", "--name-only", + f"{base_ref}...HEAD"], app_dir) + if not diff.strip(): + diff = _git(["diff", "-U0", "--diff-filter=ACMR", "--name-only", + base_ref], app_dir) + lines = [ln.strip() for ln in diff.splitlines() if ln.strip()] + paths = select_spec_paths(lines) + changes = {c for c in (_open_change_of(ln) for ln in lines) if c} + for change in changes: + for spec_md in (app_dir / "openspec" / "changes" / change / "specs").glob("*/spec.md"): + if spec_md.is_file(): + paths.add(str(spec_md.relative_to(app_dir))) return paths @@ -2168,7 +2292,17 @@ def added_scenario_refs(base_ref: str, app_dir: Path, # stdout, so every scenario in it is added. `_git` already swallows # the non-zero exit git uses for "no such path at that commit". base_text = _git(["show", f"{base}:{rel}"], app_dir) + if base_text and is_change_spec_path(rel): + base_text = strip_delta_removals(base_text) base_refs = {s["ref"] for s in parse_spec_text(base_text, spec_name)} if base_text else set() + if is_change_spec_path(rel): + # A MODIFIED block restates scenarios the main spec already has. + # Those predate the change, so they are not new here either. + for main_rel in (f"openspec/specs/{spec_name}/spec.md", + f"openspec/specs/{spec_name}.md"): + main_text = _git(["show", f"{base}:{main_rel}"], app_dir) + if main_text: + base_refs |= {s["ref"] for s in parse_spec_text(main_text, spec_name)} added |= head_refs - base_refs return added @@ -2342,13 +2476,15 @@ def run_gate(app_dir: Path) -> int: spec_root = app_dir / "openspec" / "specs" all_specs = {str(p.relative_to(app_dir)) for p in spec_files(spec_root)} + # The deltas of open changes are declared scenarios too (hydra#711). + all_specs |= {str(p.relative_to(app_dir)) for p in change_spec_files(app_dir)} if not all_specs: print( f"[gate-{GATE_NUM}] e2e-coverage: NOT APPLICABLE — no " - f"openspec/specs/*/spec.md or openspec/specs/*.md in this " - f"repository, so there is no declared scenario for an e2e test to " - f"trace back to." + f"openspec/specs/*/spec.md, openspec/specs/*.md or " + f"openspec/changes/*/specs/*/spec.md in this repository, so there " + f"is no declared scenario for an e2e test to trace back to." ) return EXIT_NOT_APPLICABLE @@ -2362,7 +2498,8 @@ def run_gate(app_dir: Path) -> int: if not touched: print( f"[gate-{GATE_NUM}] e2e-coverage: EMPTY SCOPE — diff-scoped " - f"against '{base_ref}' and NO spec file was touched. " + f"against '{base_ref}' and NO spec file was touched, " + f"main or open change. " f"{len(all_specs)} spec file(s) exist here and NONE were " f"inspected: @e2e traceability (ADR-020) is UNVERIFIED by this " f"run. This is not a pass. Audit the whole tree by running " diff --git a/hydra-gates/scripts/lib/test_check_e2e_coverage.py b/hydra-gates/scripts/lib/test_check_e2e_coverage.py index 84627680..48195b8d 100644 --- a/hydra-gates/scripts/lib/test_check_e2e_coverage.py +++ b/hydra-gates/scripts/lib/test_check_e2e_coverage.py @@ -2398,22 +2398,16 @@ def test_short_form_has_no_flatspec_group(self): class ChangedSpecFilesShapeTest(unittest.TestCase): - """Diff scoping selects both spec shapes and nothing else.""" + """Diff scoping selects both spec shapes, open change deltas, and nothing else. + + This test used to re-implement the filter it was testing, so it could not + fail on the real one, and it asserted that a change delta is "not a spec". + That expectation was hydra#711 written down: gate 19 never read a scenario + in an open change. + """ def _select(self, paths: list[str]) -> set[str]: - # changed_spec_files' filter, applied to a synthetic diff listing. - keep = set() - for line in paths: - line = line.strip() - if not line.startswith("openspec/specs/") or not line.endswith(".md"): - continue - rest = line[len("openspec/specs/"):] - depth = rest.count("/") - if depth == 1 and rest.endswith("/spec.md"): - keep.add(line) - elif depth == 0 and rest.lower() not in cec._NOT_A_SPEC: - keep.add(line) - return keep + return cec.select_spec_paths(paths) def test_selects_both_shapes_and_rejects_the_rest(self): selected = self._select([ @@ -2422,15 +2416,198 @@ def test_selects_both_shapes_and_rejects_the_rest(self): "openspec/specs/README.md", # documentation "openspec/specs/kanban/design.md", # not a spec source "openspec/specs/a/b/spec.md", # too deep - "openspec/changes/foo/specs/x/spec.md", # a change, not a spec + "openspec/changes/foo/specs/x/spec.md", # an open change's delta + "openspec/changes/foo/tasks.md", # not a spec source + "openspec/changes/archive/2026-01-01-foo/specs/x/spec.md", # archived "src/components/Thing.vue", # not markdown ]) self.assertEqual( selected, - {"openspec/specs/projects.md", "openspec/specs/kanban/spec.md"}, + {"openspec/specs/projects.md", "openspec/specs/kanban/spec.md", + "openspec/changes/foo/specs/x/spec.md"}, ) +# --------------------------------------------------------------------------- +# THE SCENARIOS OF AN OPEN CHANGE (hydra#711) +# +# A change's scenarios live in openspec/changes//specs//spec.md +# until the change is archived. The gate read openspec/specs/ only, so the PR +# that writes a change and the PRs that implement it reported EMPTY SCOPE, and +# the scenarios reached the gate only after the code meant to satisfy them had +# merged. 3,897 scenarios in six repos sat there on 28 Sep 2026. +# --------------------------------------------------------------------------- + +DELTA_ADDED = """\ +## ADDED Requirements + +### Requirement: Show the thing + +#### Scenario: The thing is shown + +- WHEN a user opens the page +- THEN the thing is shown +""" + +DELTA_ADDED_EXCLUDED = """\ +## ADDED Requirements + +### Requirement: Plumb the thing + +#### Scenario: The thing is plumbed + +@e2e exclude pure plumbing, verified by PHPUnit + +- WHEN the job runs +- THEN the thing is plumbed +""" + +DELTA_MODIFIED_EXISTING = """\ +## MODIFIED Requirements + +### Requirement: Foo behaviour + +Foo shall do bar, and faster. + +#### Scenario: Foo does bar + +- WHEN foo is called +- THEN bar happens +""" + +DELTA_REMOVED = """\ +## REMOVED Requirements + +### Requirement: Old thing + +**Reason**: nobody used it. + +#### Scenario: The old thing works + +- WHEN the old thing is used +- THEN it works +""" + + +class OpenChangeScenarioTest(unittest.TestCase): + """hydra#711: gate 19 reads the scenarios of an open change.""" + + # The git fixture of GateModeTest, borrowed rather than inherited so its + # own tests do not run twice. + setUp = GateModeTest.setUp + tearDown = GateModeTest.tearDown + _run = GateModeTest._run + _commit = GateModeTest._commit + _gate = GateModeTest._gate + + CHANGE = "openspec/changes/add-thing/specs/thing/spec.md" + + def _base_readme(self) -> str: + _write(self.root, "README.md", "# app\n") + return self._commit("base") + + def test_a_change_pr_that_adds_a_delta_scenario_is_checked(self): + base = self._base_readme() + _write(self.root, self.CHANGE, DELTA_ADDED) + self._commit("write the change") + rc, out = self._gate(base) + self.assertNotEqual(rc, cec.EXIT_EMPTY_SCOPE, out) + self.assertEqual(rc, cec.EXIT_FAIL, out) + self.assertIn("thing::the-thing-is-shown — new scenario without a test", out) + + def test_a_change_pr_with_the_test_passes(self): + base = self._base_readme() + _write(self.root, self.CHANGE, DELTA_ADDED) + _write(self.root, "tests/e2e/thing.spec.ts", + "// @e2e thing::the-thing-is-shown\n" + "test('x', async ({ page }) => { await expect(page).toHaveTitle(/x/) })\n") + self._commit("write the change and its test") + rc, out = self._gate(base) + self.assertEqual(rc, cec.EXIT_PASS, out) + + def test_an_implementation_pr_that_ticks_tasks_reads_the_change_scenarios(self): + _write(self.root, "README.md", "# app\n") + _write(self.root, self.CHANGE, DELTA_ADDED) + _write(self.root, "openspec/changes/add-thing/tasks.md", "- [ ] 1.1 build it\n") + base = self._commit("the change is on the base") + _write(self.root, "lib/Thing.php", " { await expect(page).toHaveTitle(/x/) })\n") + base = self._commit("base") + _write(self.root, "openspec/changes/faster/specs/my-spec/spec.md", DELTA_MODIFIED_EXISTING) + self._commit("change") + rc, out = self._gate(base) + self.assertEqual(rc, cec.EXIT_PASS, out) + + def test_a_removed_requirement_needs_no_test(self): + base = self._base_readme() + _write(self.root, "openspec/changes/drop-old/specs/old/spec.md", DELTA_REMOVED) + self._commit("remove") + rc, out = self._gate(base) + self.assertNotIn("the-old-thing-works", out) + self.assertEqual(rc, cec.EXIT_PASS, out) + + def test_an_archived_change_stays_out_of_scope(self): + base = self._base_readme() + _write(self.root, "openspec/changes/archive/2026-01-01-add-thing/specs/thing/spec.md", + DELTA_ADDED) + _write(self.root, "openspec/specs/other/spec.md", BASIC_SPEC) + self._commit("archive") + _write(self.root, "openspec/changes/archive/2026-01-01-add-thing/specs/thing/spec.md", + DELTA_ADDED + "\nprose\n") + base2 = self._commit("touch the archive") + del base2 + rc, out = self._gate(base) + self.assertNotIn("thing::", out) + + def test_a_full_sweep_reads_open_changes_too(self): + _write(self.root, "README.md", "# app\n") + _write(self.root, self.CHANGE, DELTA_ADDED) + self._commit("change only") + buf = io.StringIO() + with redirect_stdout(buf): + rc = cec.run_gate(self.root) + self.assertEqual(rc, cec.EXIT_FAIL, buf.getvalue()) + self.assertIn("thing::the-thing-is-shown — missing @e2e", buf.getvalue()) + + class ClassifyEvidenceTest(unittest.TestCase): """A test that proves a page mounted is not a test that proves a scenario.""" diff --git a/hydra-gates/scripts/run-hydra-gates.sh b/hydra-gates/scripts/run-hydra-gates.sh index 35b0e209..68e7e248 100755 --- a/hydra-gates/scripts/run-hydra-gates.sh +++ b/hydra-gates/scripts/run-hydra-gates.sh @@ -4546,10 +4546,21 @@ fi # Newman) by placing `@e2e exclude ` after the spec's ## Purpose # heading, which suppresses all its scenarios without per-scenario markers. # +# THE SCENARIOS OF AN OPEN CHANGE COUNT (hydra#711, 2026-09-28). A change's +# scenarios live in openspec/changes//specs//spec.md until it is +# archived, and this gate used to read openspec/specs/ only: the PR that +# writes a change and the PRs that implement it came back EMPTY SCOPE, and a +# scenario reached the gate after the code meant to satisfy it had merged. +# Now a touched delta spec is in scope, and so is every delta spec of an open +# change the diff touches any file in (an implementation PR ticks tasks.md). +# A delta scenario's ref is ::, the ref it keeps after archive. A +# scenario a MODIFIED block restates from the main spec is not new; REMOVED +# and RENAMED blocks owe no test; openspec/changes/archive/ is skipped. +# # See scripts/lib/check_e2e_coverage.py for the parse + annotation logic. # See .claude/skills/hydra-gate-e2e-coverage/SKILL.md for the fix action. # --------------------------------------------------------------------------- -if [ -d openspec/specs ] || [ -d tests/e2e ]; then +if [ -d openspec/specs ] || [ -d openspec/changes ] || [ -d tests/e2e ]; then _e2e_log=${HYDRA_GATE_LOG_DIR}/hydra-gate-e2e-coverage.log : > "${_e2e_log}" _e2e_ran=1 @@ -4625,7 +4636,7 @@ if [ -d openspec/specs ] || [ -d tests/e2e ]; then # missing and no change the author could make would put a spec # file into a diff that does not touch one. See _skip's header. _e2e_ran=0 - _skip 19 "e2e-coverage" na "the diff against '${BASE_REF}' touched NO spec file, so no scenario was inspected. Delta-scoped out (the ADR-020 diff-scoping rule, kept for this gate at every file scope since 2026-09-12), as gate-16 is for the same change — not a gap: the specs in this repo are unchanged from the base, so this change introduces no scenario whose @e2e traceability could be missing. This gate runs on the next change that touches a spec; the whole-repo sweep is a run with no base. See ${_e2e_log}." + _skip 19 "e2e-coverage" na "the diff against '${BASE_REF}' touched NO spec file and no open change, so no scenario was inspected. Delta-scoped out (the ADR-020 diff-scoping rule, kept for this gate at every file scope since 2026-09-12), as gate-16 is for the same change — not a gap: the specs in this repo are unchanged from the base, so this change introduces no scenario whose @e2e traceability could be missing. This gate runs on the next change that touches a spec; the whole-repo sweep is a run with no base. See ${_e2e_log}." elif [ "${_e2e_fail}" -eq 4 ]; then _e2e_ran=0 _skip 19 "e2e-coverage" na "no openspec/specs/*/spec.md in this repository — there is no declared scenario for an e2e test to trace back to."