fix: reconcile the feature-container convention with the depends validator
`format.md` told a child issue to link back to its container through its own `depends:`, while the validator wanted the container to list its children. Satisfying both made a cycle, caught as an ERROR, so every `type/feature` with a filled `## Issues` ended in either a warning or a hard failure — no third option. Variant B is chosen: the container depends on its children, and a child never names its container. "The container is closed when its children are closed" IS a dependency relation, so it belongs in the graph; "a child belongs to a feature" is membership, and membership does not. The code already walked the edge that way — `## Issues` is an edge source pointing container -> child — so this rewrites the documentation to match instead of inverting the graph, and the tree draws containers as roots for free. - format.md: the `type/feature` template states the direction, shows the container's `depends:`, and says why the reverse cycles; the Dependencies section names `## Issues` as the second edge source. - issue.py: `body_dep_ref_sections()` carries the section each reference came from, so the desync warning names `## Issues` on a container rather than a `## Depends on` that is not in the file. `body_dep_refs()` stays as a thin wrapper — `skills/sync/scripts/map.py` calls it and is untouched. - tests/test_container_edges.py: the repo's first tests. Stdlib unittest, `python3 -m unittest discover -s tests`. issue_check.py's cycle detector and issue_tree.py need no change: with the edge pointing down there is no cycle to break and the container is already the root. Refs claude-skills/tea#14 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
naudachu
@@ -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 закрыты
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
@@ -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()
|
||||
Reference in New Issue
Block a user