ADR-307: Dependency declaration and graph parsing - #67
Conversation
Step 1 authors a **Depends on:** line under each task's title using local task-number references (or - none -), inferred best-effort the same way titles/descriptions already are. Step 5 rewrites those references into real task-work-item keys alongside the existing title rewrite, then validates the whole Tasks section by invoking the new task_dependencies.py script and surfacing any dangling-reference or cycle error to the user.
…ependencies covers no-Depends-on-line, explicit — none —, single/multiple dependencies, dangling reference, and two/three-task cycles; all 8 tests passing)
… consistency with other SKILL.md script invocations
build-and-test: Python test resultsStatus: ✅ Passed Test log |
jodavis-claude
left a comment
jodavis-claude
left a comment
There was a problem hiding this comment.
Solid implementation overall: parse_task_dependencies matches the spec's interface, handles the no-dependency/— none —/single/multi/dangling/cycle cases correctly (verified by running the full 140-test suite plus a manual CLI smoke test), reuses graphlib.TopologicalSorter/CycleError for cycle detection as recommended, and the four "Known ambiguities" from the task brief were each resolved reasonably. One correctness/robustness issue below in SKILL.md's new step-5 script invocation that I'd like addressed before sign-off; everything else is optional/non-blocking.
…rev-parse --show-toplevel instead of an implicit repo-root CWD
jodavis-claude
left a comment
jodavis-claude
left a comment
There was a problem hiding this comment.
Sign-off review — ADR-307
Prior thread status: 1 unresolved thread from the first-pass review (Priority 1 — the task_dependencies.py invocation in spec-task-breakdown/SKILL.md step 5 used a bare repo-root-relative path). Verified the fix in 87aec0b: step 5 now resolves REPO_ROOT=$(git rev-parse --show-toplevel) before building the script's absolute path. I independently reproduced both the original failure mode and the fix — invoking the command from a non-repo-root CWD inside the repo now correctly resolves and runs (previously failed with a raw python3: can't open file error), and this resolves correctly inside a git worktree, which matches this feature's own worktree-based concurrent-pipeline use case. Re-ran the full workflow-orchestrate/scripts test suite (140/140 passing) — the fix only touched the SKILL.md prose, no production code changed, so no regression risk. Thread resolved.
The accompanying non-blocking note (TASK_HEADING_RE scanning the whole spec_text rather than being scoped to ## Tasks) was acknowledged by the developer as harmless today; agreed, no action needed.
New issues in files modified since last review: none. Only plugins/dev-team/skills/spec-task-breakdown/SKILL.md changed (6 insertions, 2 deletions) — a minimal, targeted fix anchoring the script invocation to an absolute path via git rev-parse --show-toplevel, consistent with how every other cross-skill script invocation in this repo anchors to an absolute path.
CI: build-and-test and gate both green.
Decision: approved. All prior review threads are resolved and no new Priority 1–4 issues were found in the modified files. Ready for human sign-off.
…d of repo root Reviewer (jodavis) noted the repo-root-based path assumes this skill is always run inside the agent-plugins repo, which won't hold once it's used as an installed plugin elsewhere. Step 5 now resolves task_dependencies.py relative to spec-task-breakdown's own <skill-dir> instead of shelling out to git rev-parse --show-toplevel. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TzenM31PAQmpj6ERNZjpTY
Reviewer (jodavis) asked that, after validating the dependency tree, step 5 also record each Depends on relationship in the tracker so it's visible there too, not just in the spec. For Jira, this uses the Blocks link type with the dependency as the blocker; documented as a new createIssueLink operation on work-with-Jira-tasks. Skipped for a provider whose adapter skill doesn't document a linking operation, or when no tracker is configured, matching the rest of this skill's no-tracker fallback pattern. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TzenM31PAQmpj6ERNZjpTY
3 participants
Work item: ADR-307
Extends
spec-task-breakdownwith a**Depends on:**field convention (authored via local task numbers, rewritten to real task-work-item keys once assigned) and addsparse_task_dependencies, the function that turns a spec's## Taskssection into a validated dependency graph — the foundational piece other tasks in the concurrent-development feature build on.Changes
plugins/dev-team/skills/spec-task-breakdown/SKILL.md(modified) — Step 1 now instructs authoring a**Depends on:** <ref>[, <ref>...]line directly under each task's title (local task-number references,**Depends on:** — none —default, best-effort inferred the same way titles/descriptions already are). Step 5 now rewrites those local references into real task-work-item keys alongside the existing title-hyperlink rewrite, then invokestask_dependencies.pyviaBashas a single validation pass and surfaces any dangling-reference or cycle error to the user before the spec is considered done.plugins/dev-team/skills/workflow-orchestrate/scripts/task_dependencies.py(new) — implementsparse_task_dependencies(spec_text: str) -> dict[str, list[str]], mapping each real task-key to its dependency task-keys; raisesTaskDependencyError(aValueErrorsubclass), naming the offending task and reference, for a dangling reference or a dependency cycle (cycle detection via stdlibgraphlib.TopologicalSorter/CycleError). Includes a thin CLImain()so the prosespec-task-breakdownskill can invoke it viaBash: prints the graph as JSON to stdout on success, orError: ...to stderr with a non-zero exit otherwise.plugins/dev-team/skills/workflow-orchestrate/scripts/test_task_dependencies.py(new) — 8 unit tests forparse_task_dependenciescovering every exit-criteria scenario plus one boundary case.Design decisions
workflow-orchestrate/scripts/task_dependencies.py, co-located with the futureconcurrent_schedule.pyconsumer, per the task brief's recommended, pattern-consistent choice._spec_ConcurrentDevelopment.md's own## Taskssection to add real**Depends on:**lines — left as a pre-existing, out-of-scope inconsistency. Test fixtures use their own small inline spec-text strings rather than the real spec file.TaskDependencyError(ValueError), raised for both the dangling-reference and cycle cases.Task #3) —parse_task_dependenciesis only ever called after step 5's rewrite, when every reference is already a real key.missing-test-harnessskill that no.feature/Gherkin/BDD test harness exists anywhere in this repo, so none was invented for this Wrapper-shaped skill-instruction change.main()wrapper intask_dependencies.py, following the existing repo pattern where thin CLI wrappers are untested while the core function they call is fully covered.Depends on:task field Wrapper component (theSKILL.mdprose change) — expected per the spec's component taxonomy, not a coverage gap.Testing completed
plugins/dev-team/skills/workflow-orchestrate/scripts/test_task_dependencies.py: 8 unit tests, all passing — no dependencies, single dependency, multiple dependencies,— none —amid other tasks, dangling reference, and two/three-task cycles.plugins/dev-team/skills/workflow-orchestrate/scripts/suite (140 tests) also passes, confirming no regressions.