fix(roadmap): the plan check fails a citation of a line that moved

openspec/changes/3422-the-plan-check-fails-a-citation/tasks.md

@@ -4,18 +4,18 @@
 
 Reads: `src/devops_cli/roadmap/stage_change.py`, `src/devops_cli/roadmap/stage_paths.py`, `tests/roadmap/test_roadmap_stage_content.py`. Changes: `src/devops_cli/roadmap/stage_change.py`, `tests/roadmap/test_roadmap_stage_content.py`.
 
-- [ ] 1.1 In `tests/roadmap/test_roadmap_stage_content.py`, add a failing test that passes a dict as `read_text` to `check_proposal`: a citation of line 4 that holds other text is refused with the field and the path, a citation of the line that holds the text is accepted, a line past the end is refused, a `path:def f` citation of a file that defines `def f(` at line 7 is accepted, a `path:def f` citation of a file that defines only `def g(` is refused, a stale citation of `what_changes` names that field, a plan with no `read_text` gets no citation problem, and a citation whose spacing differs is accepted
+- [x] 1.1 In `tests/roadmap/test_roadmap_stage_content.py`, add a failing test that passes a dict as `read_text` to `check_proposal`: a citation of line 4 that holds other text is refused with the field and the path, a citation of the line that holds the text is accepted, a line past the end is refused, a `path:def f` citation of a file that defines `def f(` at line 7 is accepted, a `path:def f` citation of a file that defines only `def g(` is refused, a stale citation of `what_changes` names that field, a plan with no `read_text` gets no citation problem, and a citation whose spacing differs is accepted
   - Verification: `uv run devops ci test tests/roadmap/test_roadmap_stage_content.py` fails on the base
-- [ ] 1.2 Add a helper in `src/devops_cli/roadmap/stage_change.py` that finds each citation of the form `path:line:text`, `path:def name` and `path:class name` in `feasibility`, `why` and each line of `what_changes`. Give `check_proposal` an optional `read_text: Callable[[str], str | None] | None = None`, default it to `None`, and call the helper with the texts of the three fields and that reader. Refuse a citation of fewer than 3 characters, and compare the cited text with the cited line as a substring after collapsing white space in both and stripping backticks and one trailing period. Refuse a citation whose path or line the reader does not return. Name the field and the cited path in each problem
+- [x] 1.2 Add a helper in `src/devops_cli/roadmap/stage_change.py` that finds each citation of the form `path:line:text`, `path:def name` and `path:class name` in `feasibility`, `why` and each line of `what_changes`. Give `check_proposal` an optional `read_text: Callable[[str], str | None] | None = None`, default it to `None`, and call the helper with the texts of the three fields and that reader. Refuse a citation of fewer than 3 characters, and compare the cited text with the cited line as a substring after collapsing white space in both and stripping backticks and one trailing period. Refuse a citation whose path or line the reader does not return. Name the field and the cited path in each problem
   - Verification: `uv run devops ci test tests/roadmap/test_roadmap_stage_content.py` passes
 
 ## 2. Pass a reader of the staged commit to check_proposal
 
 Reads: `src/devops_cli/roadmap/stage.py`, `src/devops_cli/roadmap/stage_change.py`, `src/devops_cli/roadmap/refine.py`, `src/devops_cli/roadmap/stage_eval_gates.py`. Changes: `src/devops_cli/roadmap/stage.py`.
 
-- [ ] 2.1 In `src/devops_cli/roadmap/stage.py`, where `_Planner` calls `check_proposal` in `_settle`, pass a `read_text` callable built from `self._source` and `self._sha` that returns the text of a cited file at the staged commit with the `git show <sha>:<path>` read (the form `refine.py` uses in `_read_git_file`), or `None` when the file is absent at that commit, so the check reads the staged commit and not the working tree
+- [x] 2.1 In `src/devops_cli/roadmap/stage.py`, where `_Planner` calls `check_proposal` in `_settle`, pass a `read_text` callable built from `self._source` and `self._sha` that returns the text of a cited file at the staged commit with the `git show <sha>:<path>` read (the form `refine.py` uses in `_read_git_file`), or `None` when the file is absent at that commit, so the check reads the staged commit and not the working tree
   - Verification: `uv run devops ci test tests/roadmap/test_roadmap_stage_plan.py` passes
-- [ ] 2.2 Confirm the replay of the golden set in `src/devops_cli/roadmap/stage_eval_gates.py` and the plan-check command in `src/devops_cli/roadmap/plan_check.py` call `check_proposal` with no reader, so the default `read_text` judges no citation and the recorded plans keep passing
+- [x] 2.2 Confirm the replay of the golden set in `src/devops_cli/roadmap/stage_eval_gates.py` and the plan-check command in `src/devops_cli/roadmap/plan_check.py` call `check_proposal` with no reader, so the default `read_text` judges no citation and the recorded plans keep passing
   - Verification: `uv run devops roadmap eval stage --replay` passes
 
 ## 3. Ask the planner for a symbol citation

src/devops_cli/roadmap/stage.py

@@ -136,6 +136,7 @@
     NextStep,
     body_dependencies,
     collect_context,
+    git_result,
     inspect_checkout,
     recorded_blockers,
 )
@@ -1019,6 +1020,9 @@ def _settle(
             overlaps=self._overlaps,
             search_evidence=evidence,
             plan_creates=self._creators(),
+            # The citation rule reads the staged commit, the one the probe and the model read
+            # (#3422), and never the working tree.
+            read_text=partial(_read_staged_file, self._source, self._sha),
         )
         if problems:
             return _Refused(" ".join(problems), proposal)
@@ -1450,6 +1454,14 @@ def _git(cwd: Path, *args: str) -> str:
     return git(cwd, *args, operation=_GIT_OPERATION)
 
 
+def _read_staged_file(source: Path, sha: str, path: str) -> str | None:
+    """The text of `path` at the staged commit `sha`, or None when that commit holds no such file
+    (#3422). The citation check reads the commit that the job plans on, and not the working tree,
+    whose lines can differ from that commit."""
+    shown, content = git_result(source, "show", f"{sha}:{path}")
+    return content if shown == 0 else None
+
+
 def _move_clone(checkout: Path, sha: str) -> None:
     """Move the clone to the staged commit with a fast-forward, so that its HEAD, its index and
     its files are that commit. The toolbox refuses any other HEAD (#3301). A clone that has

src/devops_cli/roadmap/stage_change.py

@@ -3,9 +3,10 @@
 
 The reply is one `StageProposal`. `check_proposal` finds what the rules of `openspec/config.yaml`
 ask and a schema cannot say: a Purpose on a new capability, a MODIFIED requirement only on a
-capability that exists, a spec delta only when the change is observable, and a path that is a file
-of the staged commit or that a group creates (`stage_paths`). `render_change`
-turns the reply into the files of `openspec/changes/<issue>-<slug>/`. `measure_groups` counts the
+capability that exists, a spec delta only when the change is observable, a path that is a file
+of the staged commit or that a group creates (`stage_paths`), and a citation of a line or a symbol
+that the staged file does not hold (#3422). `render_change` turns the reply into the files of
+`openspec/changes/<issue>-<slug>/`. `measure_groups` counts the
 inputs of one implementation step for each group with tiktoken. It counts the files that the tasks
 of the group name, each packed as the step packs it (`step_files`), so a group costs what its step
 reads.
@@ -16,7 +17,7 @@
 import re
 import string
 import unicodedata
-from collections.abc import Collection, Mapping, Sequence
+from collections.abc import Callable, Collection, Mapping, Sequence
 from dataclasses import dataclass
 from datetime import date
 from enum import StrEnum
@@ -118,6 +119,30 @@
 # in `changes`, so the stage check names it when no group lists it (#3405).
 _MESSAGES_FILE: Final[str] = "src/devops_cli/lang/en/messages.py"
 _MESSAGE_NAME: Final[re.Pattern[str]] = re.compile(r"\bMESSAGES\.roadmap\b")
+# A plan cites a file of the staged commit in its evidence fields: `path:line:text`, or
+# `path:def name` and `path:class name`, which stay true when the code moves (#3422). The path
+# starts at a word boundary, so the host of a URL is no citation, and the cited text of a line
+# ends at the backtick that closes its code span.
+_CITED_PATH: Final[str] = r"[A-Za-z0-9_][A-Za-z0-9_./-]*\.[A-Za-z0-9_]+"
+_LINE_CITATION: Final[re.Pattern[str]] = re.compile(
+    rf"(?<![\w./-])(?P<path>{_CITED_PATH}):(?P<line>\d+):(?P<text>[^`]*)"
+)
+_SYMBOL_CITATION: Final[re.Pattern[str]] = re.compile(
+    rf"(?<![\w./-])(?P<path>{_CITED_PATH}):(?P<kind>def|class)\s+(?P<symbol>[A-Za-z_]\w*)"
+)
+# The fewest characters of the cited text that the check judges: a shorter text matches almost any
+# line, so the check refuses it and asks for a longer one.
+_CITATION_MIN_CHARS: Final[int] = 3
+_CITATION_STALE: Final[str] = (
+    "The {field} of the plan cites `{path}`, and the staged file holds no such text there. Cite "
+    "the text of the line in a code span at the staged commit, or cite a symbol as "
+    "`path:def name` or `path:class name`, which stays true when the line moves."
+)
+_CITATION_SHORT: Final[str] = (
+    "The {field} of the plan cites `{path}` with fewer than {least} characters of the line. Quote "
+    "at least {least} characters of the line, or cite a symbol as `path:def name` or "
+    "`path:class name`."
+)
 _SENTENCE_END: Final[str] = f"{string.whitespace}."
 REGENERATE_DOCUMENTS: Final[str] = "uv run devops docs generate --sync-readme"
 
@@ -574,6 +599,96 @@ def _what_changes_problems(proposal: StageProposal, tree: StagedTree) -> list[st
     ]
 
 
+def _evidence_texts(proposal: StageProposal) -> list[tuple[str, str]]:
+    """The texts of the plan that hold evidence, each with the name of its field: `why`, the lines
+    of `feasibility` and each line of `what_changes` (#3422)."""
+    named = [("why", proposal.why)]
+    named.extend(("feasibility", line) for line in proposal.feasibility)
+    named.extend(("what_changes", line) for line in proposal.what_changes)
+    return named
+
+
+def _cited_text(text: str) -> str:
+    """The cited text as the check compares it: without its backticks and its one trailing full
+    stop, and with each run of white space as one space, as a line of the file reads."""
+    return _squash(text.replace("`", "").strip().removesuffix("."))
+
+
+def _line_at(content: str, number: int) -> str | None:
+    """The line `number` of `content`, counting from 1, or None when the file is shorter."""
+    lines = content.splitlines()
+    return lines[number - 1] if 1 <= number <= len(lines) else None
+
+
+def _defines(content: str, kind: str, symbol: str) -> bool:
+    """Whether `content` defines `symbol` with `kind` at any line, as `def name` or
+    `class name` does."""
+    pattern = re.compile(rf"\b{re.escape(kind)}\s+{re.escape(symbol)}\b")
+    return any(pattern.search(_squash(line)) for line in content.splitlines())
+
+
+def _line_citation_problem(
+    field: str, path: str, line: str, text: str, content: str | None
+) -> str | None:
+    """The problem of one `path:line:text` citation, or None when the staged line holds the text."""
+    if content is None:
+        return _CITATION_STALE.format(field=field, path=path)
+    cited = _cited_text(text)
+    if len(cited) < _CITATION_MIN_CHARS:
+        return _CITATION_SHORT.format(field=field, path=path, least=_CITATION_MIN_CHARS)
+    cited_line = _line_at(content, int(line))
+    if cited_line is not None and cited in _squash(cited_line):
+        return None
+    return _CITATION_STALE.format(field=field, path=path)
+
+
+def _symbol_citation_problem(
+    field: str, path: str, kind: str, symbol: str, content: str | None
+) -> str | None:
+    """The problem of one `path:def name` or `path:class name` citation, or None when the staged
+    file defines the symbol at some line."""
+    if content is not None and _defines(content, kind, symbol):
+        return None
+    return _CITATION_STALE.format(field=field, path=path)
+
+
+def _citation_problems(field: str, text: str, read_text: Callable[[str], str | None]) -> list[str]:
+    """One problem for each citation of `text` that the staged file does not hold."""
+    problems: list[str] = []
+    for match in _LINE_CITATION.finditer(text):
+        path = match.group("path")
+        problem = _line_citation_problem(
+            field, path, match.group("line"), match.group("text"), read_text(path)
+        )
+        if problem is not None:
+            problems.append(problem)
+    for match in _SYMBOL_CITATION.finditer(text):
+        path = match.group("path")
+        problem = _symbol_citation_problem(
+            field, path, match.group("kind"), match.group("symbol"), read_text(path)
+        )
+        if problem is not None:
+            problems.append(problem)
+    return problems
+
+
+def _plan_citation_problems(
+    proposal: StageProposal, read_text: Callable[[str], str | None] | None
+) -> list[str]:
+    """One problem for each citation of the evidence fields that the staged file does not hold, a
+    citation of the same field and path once. A check with no reader judges no citation: the
+    replay of the golden set holds no file text."""
+    if read_text is None:
+        return []
+    return list(
+        dict.fromkeys(
+            problem
+            for field, text in _evidence_texts(proposal)
+            for problem in _citation_problems(field, text, read_text)
+        )
+    )
+
+
 def content_problems(proposal: StageProposal, probes: Sequence[str] = ()) -> list[str]:
     """What is wrong with the text of `proposal` that no schema says (#3200): a text that is too
     short or a placeholder word (`is_placeholder` of the pull request form), a `feasibility` that
@@ -607,6 +722,7 @@ def check_proposal(
     overlaps: Sequence[Overlap] = (),
     search_evidence: Mapping[str, Sequence[str]] | None = None,
     plan_creates: Mapping[str, int] | None = None,
+    read_text: Callable[[str], str | None] | None = None,
 ) -> list[str]:
     """What the rules of `openspec/config.yaml` and the files of the commit find wrong in
     `proposal`, one sentence each. `known` holds the names of the capabilities under
@@ -619,9 +735,13 @@ def check_proposal(
     change a path the item cites: the plan names each in `dependencies` (#3252).
     `search_evidence` maps each negative-search literal to the paths that the staged tree holds:
     the job records it, the replay reads it from its case, and None records no search. `plan_creates`
-    maps each path that the plan of an open item creates to that item's number (#3418)."""
+    maps each path that the plan of an open item creates to that item's number (#3418). `read_text`
+    returns the text of a cited file at the staged commit, or None when the commit holds no such
+    file: the stage job passes the reader of its checkout and commit, and a check with none judges
+    no citation (#3422)."""
     problems = _delta_problems(proposal, known, requirements or {})
     problems.extend(content_problems(proposal, probes))
+    problems.extend(_plan_citation_problems(proposal, read_text))
     problems.extend(form_problems(proposal.groups))
     problems.extend(path_problems(proposal.groups, tree))
     problems.extend(_what_changes_problems(proposal, tree))

tests/roadmap/test_roadmap_stage_content.py

@@ -235,3 +235,107 @@ def test_a_what_changes_folder_that_a_group_writes_under_is_accepted() -> None:
     problems = _check(plan)
 
     assert not any("src/devops_cli/sample/" in p for p in problems)
+
+
+# ── A citation of the staged file (#3422) ─────────────────────────────────────
+
+CITED = "src/sample.py"
+OTHER_CITED = "src/other.py"
+CITED_FILES = {
+    CITED: "\n".join(
+        [
+            "import os",
+            "",
+            "",
+            "def f(): pass",
+            "",
+            "",
+            "def f():",
+            "    return 1",
+            "",
+            "class G:",
+            "    pass",
+        ]
+    ),
+    OTHER_CITED: "\n".join(["import os", "", "", "def g(): pass", "", "", "def g():"]),
+}
+
+
+def _cited(text: str, **overrides: Any) -> list[str]:
+    plan = make_stage_proposal(feasibility=[text], **overrides)
+    return check_proposal(plan, ["comment-thread"], TREE, read_text=CITED_FILES.get)
+
+
+def test_the_sample_plan_cites_no_line_that_the_staged_file_lacks() -> None:
+    assert (
+        check_proposal(make_stage_proposal(), ["comment-thread"], TREE, read_text=CITED_FILES.get)
+        == []
+    )
+
+
+def test_a_citation_of_a_line_that_holds_other_text_is_refused() -> None:
+    problems = _cited(f"The probe shows `{CITED}:4:def g():` at the commit.")
+
+    assert any("feasibility" in p and CITED in p for p in problems)
+
+
+def test_a_citation_of_the_line_that_holds_the_text_is_accepted() -> None:
+    problems = _cited(f"The probe shows `{CITED}:4:def f():` at the commit.")
+
+    assert not any(CITED in p for p in problems)
+
+
+def test_a_citation_of_a_line_past_the_end_is_refused() -> None:
+    problems = _cited(f"The probe shows `{CITED}:99:def f():` at the commit.")
+
+    assert any("feasibility" in p and CITED in p for p in problems)
+
+
+def test_a_citation_of_an_absent_path_is_refused() -> None:
+    problems = _cited("The probe shows `src/absent.py:1:def f():` at the commit.")
+
+    assert any("feasibility" in p and "src/absent.py" in p for p in problems)
+
+
+def test_a_citation_of_a_text_of_fewer_than_three_characters_is_refused() -> None:
+    problems = _cited(f"The probe shows `{CITED}:4:de` at the commit.")
+
+    assert any("feasibility" in p and CITED in p for p in problems)
+
+
+def test_a_citation_of_a_symbol_is_accepted_when_the_file_defines_it() -> None:
+    problems = _cited(f"The probe shows `{CITED}:def f` at line 7.")
+
+    assert not any(CITED in p for p in problems)
+
+
+def test_a_citation_of_a_class_is_accepted_when_the_file_defines_it() -> None:
+    problems = _cited(f"The probe shows `{CITED}:class G` at line 10.")
+
+    assert not any(CITED in p for p in problems)
+
+
+def test_a_citation_of_an_absent_symbol_is_refused() -> None:
+    problems = _cited(f"The probe shows `{OTHER_CITED}:def f` at the commit.")
+
+    assert any("feasibility" in p and OTHER_CITED in p for p in problems)
+
+
+def test_a_stale_citation_of_what_changes_names_that_field() -> None:
+    plan = make_stage_proposal(what_changes=[f"The probe reads `{CITED}:4:def g():` now."])
+
+    problems = check_proposal(plan, ["comment-thread"], TREE, read_text=CITED_FILES.get)
+
+    assert any("what_changes" in p and CITED in p for p in problems)
+
+
+def test_a_plan_with_no_reader_gets_no_citation_problem() -> None:
+    plan = make_stage_proposal(feasibility=[f"The probe shows `{CITED}:4:def g():` at the commit."])
+
+    assert not any(CITED in p for p in _check(plan))
+
+
+def test_a_citation_holds_when_the_white_space_differs() -> None:
+    problems = _cited(f"The probe shows `{CITED}:4:def   f():` at the commit.")
+
+    assert not any(CITED in p for p in problems)