diff --git a/skills/issue/references/format.md b/skills/issue/references/format.md index d41d063..2a6fd93 100644 --- a/skills/issue/references/format.md +++ b/skills/issue/references/format.md @@ -145,6 +145,11 @@ you — `issue_check.py` warns when the section names an id that `depends:` does not list. Omit the section when there are no dependencies; never write an empty one. +A `type/feature` container writes the same relation under `## Issues` instead +(see the template below). Same direction, same rule: every id named there also +belongs in that issue's `depends:`. The warning names whichever of the two +sections the reference actually came from. + Draw the graph with `issue_tree.py`. The reverse direction is a grep: ```bash @@ -258,9 +263,31 @@ grep -ln 'depends:.*migrate-schema' tmp/issues/*.md ## Template: `type/feature` A container: one unit of business value delivered by several child issues. -Child issues carry their own `type/*` (`task`, `bug`, `test`, …) and link back -via their `depends:`. Keep implementation detail in the children; the feature -body stays at business level. +Child issues carry their own `type/*` (`task`, `bug`, `test`, …) and know +nothing about the container. + +**The container depends on its children, never the reverse.** Every child id +goes in the container's own `depends:` and, as prose, in its `## Issues` +section; a child's `depends:` is for that child's real dependencies and must +not point back at the container. Keep implementation detail in the children; +the feature body stays at business level. + +That direction is not a convention picked at random. "The container is closed +when its children are closed" *is* a dependency relation. "This child belongs +to that feature" is a membership relation, and membership has no place in a +dependency graph. Pointed the other way the two rules contradict each other: +the moment the container listed a child that already depended on it, +`issue_check.py` would report `ERROR cycle`. With the edge going down, the +graph reads as nesting — `issue_tree.py` draws the container as the root with +its children beneath it — and the check is green. + +So the container's metadata block carries the children: + +```markdown +depends: [wire-sqlc-appclick, add-pool-cfg] +``` + +and its body repeats them for a human: ```markdown ## Summary @@ -274,7 +301,7 @@ body stays at business level. ## Issues - [ ] wire-sqlc-appclick — краткое описание части -- [ ] … +- [ ] add-pool-cfg — краткое описание части ## Acceptance criteria - [ ] все дочерние issues закрыты diff --git a/skills/issue/scripts/issue.py b/skills/issue/scripts/issue.py index 293dba9..c16119b 100644 --- a/skills/issue/scripts/issue.py +++ b/skills/issue/scripts/issue.py @@ -80,13 +80,20 @@ EXCLUSIVE_NS = ("type/", "severity/") REQUIRED_SECTIONS = ["## Summary", "## Spec"] AC_SECTION = "## Acceptance criteria" DEPENDS_SECTION = "## Depends on" +ISSUES_SECTION = "## Issues" +# Both sections name what an issue depends on, so both are edge sources and +# both point the same way. In a `type/feature` that reads container -> child: +# "the container is closed when its children are closed" IS a dependency. +# "a child belongs to a feature" is membership, and membership has no place in +# a dependency graph — which is why a child never names its container back. +DEP_SECTIONS = (DEPENDS_SECTION, ISSUES_SECTION) # Per-type sections from the templates — absence is a warning, not a stop. EXPECTED_SECTIONS = { "bug": ["## Steps to reproduce", "## Expected", "## Actual", "## Environment"], "task": ["## Motivation"], "refactor": ["## Motivation", "## Invariants"], "test": ["## Motivation", "## Test cases"], - "feature": ["## Motivation", "## Issues"], + "feature": ["## Motivation", ISSUES_SECTION], "draft": ["## Notes"], } @@ -267,24 +274,37 @@ def section_body(body, header): return "\n".join(out).strip() -def body_dep_refs(body): - """Tokens referenced from `## Depends on` / `## Issues` only — never from - prose, or a graph walk would drag in half the backlog. Returns whatever was - written there (slugs, and `#N` on issues that came from a tracker).""" - out, active = [], False +def body_dep_ref_sections(body): + """[(section, ref)] for every reference under one of DEP_SECTIONS — never + from prose, or a graph walk would drag in half the backlog. Refs are + whatever was written there (slugs, and `#N` on issues that came from a + tracker), deduplicated on first sight. + + The section is carried out with the ref so a caller can name the one the + reader actually has in front of them: a container's children come from + `## Issues`, and pointing at `## Depends on` would name a section that is + not in the file.""" + out, seen, section = [], set(), "" for line in (body or "").splitlines(): if line.startswith("## "): - active = line.strip() in (DEPENDS_SECTION, "## Issues") + head = line.strip() + section = head if head in DEP_SECTIONS else "" continue - if not active: + if not section: continue for tok in re.findall(r'#(\d+)|\b([a-z0-9]+(?:-[a-z0-9]+)+)\b', line): ref = ("#" + tok[0]) if tok[0] else tok[1] - if ref not in out: - out.append(ref) + if ref not in seen: + seen.add(ref) + out.append((section, ref)) return out +def body_dep_refs(body): + """Just the refs, in order of first appearance.""" + return [ref for _, ref in body_dep_ref_sections(body)] + + # -------------------------------------------------------------------------- # validation # -------------------------------------------------------------------------- @@ -342,12 +362,13 @@ def validate(issue, known_ids=None): warn.append("depends on %r, which is not in the store" % d) # `depends:` is the machine-readable graph; the body section is prose for - # humans. They drift silently unless something says so. + # humans. They drift silently unless something says so. Name the section + # the reference actually came from — for a container that is `## Issues`. listed = set(issue.depends) - for ref in body_dep_refs(issue.body): + for section, ref in body_dep_ref_sections(issue.body): if not ref.startswith("#") and ref not in listed: warn.append("%s mentions %r but `depends:` does not list it" - % (DEPENDS_SECTION, ref)) + % (section, ref)) return err, warn diff --git a/tests/test_container_edges.py b/tests/test_container_edges.py new file mode 100644 index 0000000..0ebcbcf --- /dev/null +++ b/tests/test_container_edges.py @@ -0,0 +1,234 @@ +#!/usr/bin/env python3 +""" +The container <-> child edge points ONE way: container -> child. + +A `type/feature` is closed when its children are closed, and that is a +dependency relation, so the container lists its children in `depends:`. A child +belongs to a feature, which is a membership relation, and membership has no +place in a dependency graph — so a child never names its container back. These +tests pin that direction down in all three places it shows up: the validator, +the desync warning, and the drawn tree. + + python3 -m unittest discover -s tests -v + +Stdlib only, like the scripts under test. `skills/*/scripts/` are directories, +not packages, so they go on sys.path by hand. +""" +import contextlib +import io +import os +import sys +import tempfile +import unittest + +REPO = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) +SCRIPTS = os.path.join(REPO, "skills", "issue", "scripts") +if SCRIPTS not in sys.path: + sys.path.insert(0, SCRIPTS) + +import issue # noqa: E402 +import issue_check # noqa: E402 +import issue_new # noqa: E402 +import issue_tree # noqa: E402 + + +def run(module, argv): + """Call a script's main() with argv, returning (exit_code, stdout). + + stderr is swallowed: issue_new.py notes on it when a `--depends` id is not + in the store yet, which is fine and not what these tests are about.""" + buf = io.StringIO() + old = sys.argv + sys.argv = [module.__name__ + ".py"] + argv + try: + with contextlib.redirect_stdout(buf), contextlib.redirect_stderr(io.StringIO()): + code = module.main() + except SystemExit as e: # argparse / sys.exit("msg") + code = e.code if isinstance(e.code, int) else 1 + finally: + sys.argv = old + return (code or 0), buf.getvalue() + + +def drawn(tree_output): + """The rows inside the tree's code fence, header and blanks dropped.""" + return [l for l in tree_output.splitlines() if l.rstrip().endswith(".md")] + + +class StoreCase(unittest.TestCase): + """A scratch store per test. Never touches tmp/issues/.""" + + def setUp(self): + self._tmp = tempfile.TemporaryDirectory() + self.root = self._tmp.name + self.addCleanup(self._tmp.cleanup) + + def new(self, type, id, title, depends=()): + argv = ["--type", type, "--id", id, "--title", title, "--out", self.root] + for d in depends: + argv += ["--depends", d] + code, _ = run(issue_new, argv) + self.assertEqual(code, 0, "issue_new.py failed for %s" % id) + + def edit(self, id, old, new): + p = issue.path_of(self.root, id) + with open(p) as f: + text = f.read() + self.assertIn(old, text, "%s.md does not contain %r" % (id, old)) + with open(p, "w") as f: + f.write(text.replace(old, new)) + + def fill_issues_section(self, id, *children): + """Replace the type/feature template's `## Issues` placeholder.""" + self.edit(id, + "- [ ] slug-дочернего-issue — краткое описание части\n- [ ] …\n", + "".join("- [ ] %s — часть\n" % c for c in children)) + + def container_and_child(self): + """The canonical shape from references/format.md: the container names + the child in `depends:` AND in `## Issues`; the child names nobody.""" + self.new("task", "child-y", "Child y") + self.new("feature", "feat-x", "Container x", depends=["child-y"]) + self.fill_issues_section("feat-x", "child-y") + + +class CanonicalContainerIsClean(StoreCase): + """A container from the template plus a child per format.md: green.""" + + def test_check_is_silent_and_exits_zero(self): + self.container_and_child() + code, out = run(issue_check, ["--out", self.root]) + self.assertEqual(code, 0, out) + self.assertNotIn("ERROR", out) + self.assertNotIn("warn", out) + self.assertIn("ok feat-x", out) + self.assertIn("ok child-y", out) + + def test_validate_reports_nothing_for_either_issue(self): + self.container_and_child() + issues = issue.load_all(self.root) + for id in ("feat-x", "child-y"): + err, warn = issue.validate(issues[id], known_ids=set(issues)) + self.assertEqual((err, warn), ([], []), id) + + def test_the_child_does_not_depend_on_its_container(self): + self.container_and_child() + issues = issue.load_all(self.root) + self.assertEqual(issues["feat-x"].depends, ["child-y"]) + self.assertEqual(issues["child-y"].depends, []) + + +class OnlyOneDirectionIsLegal(StoreCase): + """format.md and the validator agree on container -> child, and the + reverse edge is an error rather than a matter of taste.""" + + def test_the_reverse_edge_is_a_cycle(self): + self.container_and_child() + self.edit("child-y", "depends: []", "depends: [feat-x]") + code, out = run(issue_check, ["--out", self.root]) + self.assertEqual(code, 1, out) + self.assertIn("ERROR cycle:", out) + self.assertIn("feat-x", out) + self.assertIn("child-y", out) + + def test_a_child_pointing_at_its_container_alone_is_not_the_graph(self): + """The shape format.md used to document: the child depends on the + container and the container's depends: is empty. It no longer matches + what the container's own `## Issues` says, so the check complains.""" + self.new("feature", "feat-x", "Container x") + self.new("task", "child-y", "Child y", depends=["feat-x"]) + self.fill_issues_section("feat-x", "child-y") + _, out = run(issue_check, ["--out", self.root]) + self.assertIn("warn feat-x:", out) + + def test_issues_section_is_an_edge_source_pointing_down(self): + body = "## Issues\n- [ ] child-y — часть\n" + self.assertEqual(issue.body_dep_refs(body), ["child-y"]) + + +class WarningNamesItsOwnSection(StoreCase): + """The desync warning quotes the section the reference came from, not + `## Depends on` unconditionally — a container has no such section.""" + + def test_container_warning_says_issues(self): + self.new("feature", "feat-x", "Container x") + self.new("task", "child-y", "Child y") + self.fill_issues_section("feat-x", "child-y") # but not depends: + issues = issue.load_all(self.root) + err, warn = issue.validate(issues["feat-x"], known_ids=set(issues)) + self.assertEqual(err, []) + self.assertEqual( + warn, ["## Issues mentions 'child-y' but `depends:` does not list it"]) + self.assertNotIn("## Depends on", "\n".join(warn)) + body = issue.load(self.root, "feat-x").body + self.assertNotIn("## Depends on", body, + "the warning must not name a section that is not in the file") + + def test_plain_issue_warning_still_says_depends_on(self): + self.new("task", "child-y", "Child y", depends=["migrate-schema"]) + self.edit("child-y", "depends: [migrate-schema]", "depends: []") + issues = issue.load_all(self.root) + _, warn = issue.validate(issues["child-y"]) + self.assertEqual( + warn, + ["## Depends on mentions 'migrate-schema' but `depends:` does not list it"]) + + def test_each_reference_is_named_with_its_own_section(self): + body = ("## Depends on\n- migrate-schema\n\n" + "## Issues\n- [ ] child-y — часть\n") + self.assertEqual( + issue.body_dep_ref_sections(body), + [("## Depends on", "migrate-schema"), ("## Issues", "child-y")]) + + def test_body_dep_refs_still_returns_bare_strings(self): + """skills/sync/scripts/map.py filters this list for `#N` refs.""" + body = "## Depends on\n- migrate-schema\n- #42\n" + refs = issue.body_dep_refs(body) + self.assertEqual(refs, ["migrate-schema", "#42"]) + self.assertTrue(all(isinstance(r, str) for r in refs)) + + def test_tracker_numbers_never_warn(self): + """`#42` is a tracker handle, not a slug; `depends:` holds ids only.""" + self.new("task", "child-y", "Child y") + self.edit("child-y", "## Motivation", "## Depends on\n- #42\n\n## Motivation") + issues = issue.load_all(self.root) + _, warn = issue.validate(issues["child-y"]) + self.assertEqual(warn, []) + + +class TreePutsTheContainerOnTop(StoreCase): + + def test_container_is_the_root_and_children_hang_below(self): + self.container_and_child() + code, out = run(issue_tree, ["--out", self.root]) + self.assertEqual(code, 0, out) + rows = drawn(out) + self.assertEqual(len(rows), 2, out) + self.assertTrue(rows[0].startswith("feat-x "), out) + self.assertTrue(rows[1].startswith("└── child-y "), out) + self.assertEqual(out.count("child-y ["), 1, "child drawn more than once") + + def test_the_container_is_the_only_root(self): + self.container_and_child() + _, out = run(issue_tree, ["--out", self.root]) + self.assertIn("# Dependency tree — feat-x", out) + + def test_two_children_hang_off_one_container(self): + self.new("task", "child-y", "Child y") + self.new("task", "child-z", "Child z") + self.new("feature", "feat-x", "Container x", + depends=["child-y", "child-z"]) + self.fill_issues_section("feat-x", "child-y", "child-z") + code, out = run(issue_check, ["--out", self.root]) + self.assertEqual(code, 0, out) + _, tree = run(issue_tree, ["--out", self.root]) + self.assertEqual(tree.count("feat-x ["), 1, + "the container must not repeat once per child") + self.assertEqual([r.split(" ")[0] for r in drawn(tree)], + ["feat-x", "├──", "└──"], tree) + self.assertIn("├── child-y ", tree) + self.assertIn("└── child-z ", tree) + + +if __name__ == "__main__": + unittest.main()