ADR-303: Rewrite implement-task as a per-component dispatcher, add implement-direct - #54
Conversation
build-and-test: Python test resultsStatus: ✅ Passed Test log |
jodavis-claude
left a comment
There was a problem hiding this comment.
Reviewed against the ADR-303 exit criteria and the ADR-288 spec (_spec_TddForImplementation.md). All three exit criteria are met:
implement-task/SKILL.mdcorrectly branches on Components in scope (absent/empty vs. one-or-more) and dispatches each component toimplement-direct(Wrapper/Orchestrator) orimplement-tdd(Testable) in dependency order, with no change to the skill's single-spawn_agentinterface.implement-direct/SKILL.mdimplements a Wrapper (no test) or Orchestrator (one narrow integration test against real, non-mocked dependencies) directly, pluscommit-changes, and documents theskip-commitmechanism used byimplement-tdd's Tier 2resolve_directlyescalation. Cross-checked againstimplement-tdd/SKILL.md's existing step 4 text ("Stage the change, then go to step 6") — the contract lines up correctly with no edit needed there, as the PR claims.- Both dry runs (zero-component and three-component
SystemClock/TTLCache/CachingService) are backed by real commits in their own scratch git repos and real pytest transcripts, not stubbed output. The three-component run shows a genuine structural/behavioral red-green loop across 6 turns plus an interleaved refactor consolidation, and the environment constraint (no liveAgent/SendMessagespawning of thetdd-*agent types) is disclosed plainly rather than glossed over, consistent with ADR-301/302's precedent. - No scope creep: only the two files named in the task brief changed;
plugin.jsonand the pre-existingidentify-work-itemskill-name mismatch were correctly left untouched per the brief's own instructions.
One minor, non-blocking note left inline below. No Priority 1-4 issues found — approving.
jodavis
left a comment
There was a problem hiding this comment.
Most of my comments are about reconciling this new TDD procedure with the existing test-driven-development skill, which should be superseded, but has some useful concepts as well (e.g. writing E2E tests as an overarching TDD of the whole task). This will require some redesign that we should discuss, and we'll need to consider other skills where test-driven-development has been referenced.
…plement-direct
implement-task/SKILL.md now branches on the task brief's Components in scope section:
absent or explicitly empty falls back to the pre-existing single-agent
test-driven-development flow (one commit for the whole task); one or more components
dispatches each, in dependency order, to implement-direct (Wrapper/Orchestrator) or
implement-tdd (Testable), each of which commits its own component. The pipeline's
single-spawn_agent interface with Developer is unchanged. implement-direct/SKILL.md
(new) implements one Wrapper (no test) or Orchestrator (one narrow integration test
against real, non-mocked direct dependencies) directly, then commits — or stages
instead of committing when invoked with the optional skip-commit argument from
implement-tdd's Tier 2 resolve_directly escalation, which still owns that component's
single commit.
Verified implement-tdd's existing "follow the implement-direct skill... Stage the
change" resolve_directly text already resolves correctly against implement-direct's
new stage-vs-commit contract with no change needed there.
Dry runs (scratch fixtures under scratchpad/adr303_dryrun_{zero,three}, transcripts
under ~/.dev-team/jodavis/agent-plugins/logs/ADR-303-dryrun-*.log):
- Zero-component task brief: implemented directly via test-driven-development, single
commit, no dispatcher iteration.
- Three-component task brief (SystemClock Wrapper, TTLCache Testable, CachingService
Orchestrator depending on both): produced exactly one commit per component in
dependency order, each routed to the correct skill. The Testable component went
through a real structural/behavioral red-green loop with an interleaved refactor
turn (consolidating two tests into a parametrized one) using real pytest runs.
Known limitation, consistent with ADR-301 PR #52 and ADR-302 PR #53: this session's
Agent tool does not expose tdd-tester/tdd-implementer/tdd-refactorer as spawnable
agent types, so Developer manually played each role's turn rather than using literal
Agent/SendMessage sub-agent spawning. The protocol choreography and every build/test
run were genuine, real pytest executions — only sub-agent process isolation was
simulated.
…54 review implement-task/SKILL.md's steps 5-11 now: write E2E scenarios first (behavior-driven-development), dispatch each declared Components-in-scope row to implement-direct (Wrapper/Orchestrator) or implement-tdd (Testable), triage any leftover exit-criteria work -- including the whole task when there's no Components in scope section at all -- into component-shaped-but-uncaptured (classified on the spot via component-taxonomy, routed through implement-direct/implement-tdd) or non-component-shaped (implemented directly, staged for later), re-confirm E2E scenarios, self-review the cumulative diff, fix anything it surfaces, then make one final commit covering leftover work plus review fixups (skipped when nothing remains). There is no longer any code path that reaches test-driven-development's old write-all-tests-then-implement procedure. implement-direct/SKILL.md now states explicitly to fix any build/test failures before committing or staging (step 3), removes the stale "no Components in scope -> fall back to test-driven-development" bullet (that case is now implement-task's triage bucket instead), and cites tdd-practices in place of test-driven-development for the Orchestrator integration test's AAA/naming conventions. Dry runs (scratch fixtures under scratchpad/adr303_dryrun_{adhoc,leftover}, transcripts under ~/.dev-team/jodavis/agent-plugins/logs/ADR-303-dryrun-*.log): - Zero-Components-in-scope task brief with real logic (a percent_change utility): Developer classified the whole task ad hoc as Testable via component-taxonomy and drove it through implement-tdd's real red/green loop (manually playing tdd-tester/tdd-implementer/tdd-refactorer's turns -- these agent types are not in this session's available agent list, the same disclosed constraint as PR #54). Result: exactly 1 commit, made by implement-tdd itself, not a whole-task commit from a test-driven-development-style flow. - One declared Testable component (RetryPolicy) plus a leftover demo script and doc note: RetryPolicy went through its own real red/green loop and commit via implement-tdd; the script and doc were implemented directly and staged; a deliberately-introduced self-review finding (a doc file missing CONTRIBUTING.md's required Summary: line) was caught and fixed before the final commit. Result: 2 commits -- RetryPolicy's own commit, then one final commit covering the leftover work and the self-review fix, confirmed by a clean `git status` afterward. - Exit criterion 5's one-Wrapper/one-Orchestrator/one-Testable dry run reuses PR #54's already-executed evidence (ADR-303-dryrun-three-*.log) rather than re-running it: the declared-component dispatch mechanics (this rework's step 6) and both E2E wrapper calls are unchanged in substance, only renamed to cite behavior-driven-development -- re-deriving an identical result was out of scope. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RUxmZpJaDFeN8edPacgkZQ
0e1a706 to
eb67c1c
Compare
jodavis-claude
left a comment
There was a problem hiding this comment.
Reviewed the rework of implement-task/SKILL.md and implement-direct/SKILL.md against ADR-303's exit criteria, the _spec_TddForImplementation.md design decisions, and the six human review comments from the earlier CHANGES_REQUESTED pass.
All eight exit criteria are met:
- The dispatcher (step 6) iterates Components in scope in dependency order, routing Wrapper/Orchestrator to implement-direct and Testable to implement-tdd, with no change to the pipeline's single-call interface (confirmed against workflow-orchestrate/SKILL.md, which still treats implement-task as one opaque step).
- Step 7's two-bucket triage (component-shaped-but-uncaptured via component-taxonomy, vs. non-component-shaped implemented directly) correctly collapses the absent/explicit-empty Components-in-scope cases into the same bucket, matching write-task-brief's documented conventions.
- Steps 8-10 correctly reorder to E2E re-run -> self-review -> fix -> one final commit, with an explicit skip condition when nothing remains, and per-component commits are untouched.
- implement-direct's step 3 now explicitly states to fix build/test failures before proceeding to step 4 -- resolves review comments #1 and #6 from the prior pass.
- Every test-driven-development reference is gone from both files (grep confirms zero hits) -- resolves review comments #2 and #7. The old "no components -> fall back to test-driven-development" bullet is removed outright from implement-direct, not just reworded.
- The four dry runs (three fresh: ADR-303-dryrun-adhoc.log, ADR-303-dryrun-leftover.log, plus the reused ADR-303-dryrun-three-*.log pair) are real: genuine pytest failures/passes at every red/green/refactor turn, not narrated. The adhoc and leftover runs specifically demonstrate exit criteria 6-8 with real forced self-review findings landing in the final commit.
One open question worth human confirmation before merge (non-blocking, already flagged as an unresolved ambiguity in the task brief): see inline comment on implement-direct/SKILL.md.
Two minor, non-blocking notes for awareness:
- The PR description/body still describes the pre-review (original PR #54) design rather than this rework -- worth updating so the PR overview matches the current diff.
- Exit criterion 5's three-component dry run reuses PR #54's original evidence (ADR-303-dryrun-three-*.log) rather than a fresh run against the rewritten skill text. This is reasonable since the dispatch mechanics for that flow are unchanged in substance (only citation renames), and it's disclosed transparently in the Implementation Summary, but flagging so it's visible during human sign-off.
No Priority 1-3 (correctness, security, performance) issues found. Approving.
Work item
ADR-303: Rewrite
implement-taskas a per-component dispatcher, and addimplement-directforWrapper/Orchestrator components (and Tier 2
resolve_directlyreuse), so the Developer'simplementingstep routes each component in a task brief's Components in scope list to theright implementation strategy while keeping the pipeline's single-
spawn_agentinterfaceunchanged.
Changes
plugins/dev-team/skills/implement-task/SKILL.md— rewrote steps 5–6 as a branch: absent orexplicitly empty Components in scope falls back to the pre-existing single-agent
test-driven-developmentflow (one commit for the whole task); one or more componentsdispatches each, in dependency order, to
implement-direct(Wrapper/Orchestrator) orimplement-tdd(Testable) — each of which commits its own component, so this skill's owncommit step is skipped in that branch. Step 7's self-review now explicitly covers the
cumulative multi-component diff and each component's tier-appropriate test expectation. Step
8's report gained a "Known ambiguities" section for Tier 3 escalation notes. No change to the
skill's single-
spawn_agentinterface with the pipeline.plugins/dev-team/skills/implement-direct/SKILL.md(new) — implements oneWrapper(nodedicated test) or
Orchestrator(one narrow integration test against real, non-mocked directdependencies, primary scenario only) component directly, then
commit-changes. Documents anoptional
skip-committrailing argument soimplement-tdd's Tier 2resolve_directlyescalation stages instead of committing (that component's single commit still happens later,
in
implement-tdd's own step 7).implement-tdd/SKILL.md's existingresolve_directlytext ("follow theimplement-directskill... Stage the change, then go to step 6") already resolves correctlyagainst
implement-direct's new stage-vs-commit contract — no edit needed there, per the taskbrief's own expectation.
Design decisions
skip-commit(
<component-row> <task-brief-path> <spec-path> [skip-commit]), consistent with thebracket-optional-argument convention already used elsewhere in this plugin.
TTLCachescenario with aSystemClockWrapper and a
CachingServiceOrchestrator that genuinely wiresSystemClock.nowinto aninternally-owned
TTLCache, so the Orchestrator's integration test exercises real wiringrather than pass-through construction.
Agent/SendMessagespawning oftdd-tester/tdd-implementer/tdd-refactoreris still unavailable in this Claude Codesession (same constraint recorded in ADR-301 PR ADR-301: Add tdd-tester/tdd-implementer agents and the implement-tdd red/green loop #52 and ADR-302 PR ADR-302: Add tdd-refactorer agent and wire post-done refactor turn into implement-tdd #53 — these agent types
are not in the session's "Available agent types" list). Developer manually played each role's
turn, running real
pytestcommands at every turn, so the protocol choreography and everybuild/test result are genuine — only literal sub-agent process isolation was simulated. Stated
plainly rather than presented as a live spawn.
Testing completed
No unit tests apply to this diff — the deliverable is markdown-only skill-definition files with
no executable production code, per
missing-test-harness(this repo has no test harness forprose/instruction files, consistent with ADR-301/302's precedent). Verification instead took the
form of two dry runs (scratch fixtures under
scratchpad/adr303_dryrun_{zero,three}, transcriptsunder
~/.dev-team/jodavis/agent-plugins/logs/ADR-303-dryrun-*.log):test-driven-development(unit testwritten first, confirmed red for the right reason, then green), single commit, no dispatcher
iteration.
SystemClockWrapper →TTLCacheTestable →CachingServiceOrchestrator depending on both): produced exactly one commit per component in dependency
order, each routed to the correct skill. The Testable component went through a real
structural/behavioral red-green loop (6 turns) with one interleaved refactor turn that
consolidated two near-identical tests into a parametrized test, confirmed via real
pytestruns at every turn. Full 7-test suite (6
TTLCache+ 1CachingService) passes.