ADR-302: Add tdd-refactorer agent and wire post-done refactor turn into implement-tdd - #53
Conversation
…to implement-tdd
Completes the tdd-tester/tdd-implementer/tdd-refactorer trio: adds a new
tdd-refactorer sub-agent scoped to behavior-preserving changes only (test or
production files, never a behavior change), plus its own tdd-refactor-turn
turn-mechanics skill (mirroring the ADR-301 agent+skill split established for
tdd-red-turn/tdd-green-turn, since the exit criteria's "same tool set as the
other two tdd-agents" now includes Skill).
implement-tdd/SKILL.md gains a new step 7: after a component reaches done and
is committed, Developer spawns tdd-refactorer for one turn with the spec's
exact turn message, and commits again only if it replies refactored (never
folded into the done-commit). Replaces the prior "Scope note" that explicitly
deferred this wiring to ADR-302.
Verified via two synthetic dry runs (logged under
~/.dev-team/jodavis/agent-plugins/logs/ADR-302-tdd-refactorer-dryrun{1,2}.log):
1. A TtlValidator component left with two near-identical
ValueError-raising tests (differing only by input literal) is
consolidated into a single parameterized test, full suite still green
(3 passed) with both cases retained.
2. Re-running the pass against that now-clean component reports
no-refactor-needed with zero file changes (confirmed via md5sum).
No live Agent/SendMessage sub-agent spawning is available in this
environment (same constraint noted in ADR-301/PR #52) — the dry runs manually
simulate the tdd-refactorer role/skill against real pytest runs.
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.
Review summary — ADR-302
Reviewed the diff (plugins/dev-team/agents/tdd-refactorer.md, plugins/dev-team/skills/tdd-refactor-turn/SKILL.md, plugins/dev-team/skills/implement-tdd/SKILL.md) against the task brief's exit criteria, _spec_TddForImplementation.md, CONTRIBUTING.md, and the ADR-301-established tdd-tester/tdd-implementer/tdd-red-turn/tdd-green-turn templates.
Exit criteria — all four verified:
agents/tdd-refactorer.mdexists, scoped to behavior-preserving changes only (test or production files, only afterdone), same tool set (Read,Glob,Grep,Edit,Write,Bash,Skill) astdd-tester/tdd-implementer. Confirmed.implement-tdd/SKILL.mdstep 7 spawnstdd-refactorerafter step 6's commit, sends the spec's exact turn message verbatim, and reruns the full component suite (delegated totdd-refactor-turn's own turn mechanics, matching how steps 2/3 already delegate test/build execution to the sub-agent rather than the orchestrator itself — a consistent pattern, not a shortcut). Confirmed.- Dry run 1 (
ADR-302-tdd-refactorer-dryrun1.log):TtlValidatorwith two near-identicalValueErrortests differing only by input literal (0 / -5) consolidated into one@pytest.mark.parametrizetest; full suite (3 passed) stayed green with both cases retained. Matches the exit criterion exactly. - Dry run 2 (
ADR-302-tdd-refactorer-dryrun2.log): rerunning against the now-clean component reportedno-refactor-needed, confirmed viamd5sumas zero file changes. Matches the exit criterion exactly.
Other checks:
- Diff is minimal and clean (3 files, 179 insertions / 8 deletions) — no stray edits, no leftover "Scope note" text anywhere in the repo.
tdd-refactorer.md/tdd-refactor-turn/SKILL.mdstructurally mirrortdd-tester.md+tdd-red-turnandtdd-implementer.md+tdd-green-turn(Role / constraint section / Ground rules / Practice rules / Taking a turn / Skills), keeping the trio consistent per ADR-301's established split.- Turn-mechanics-skill split (Known ambiguity #2) is a reasonable, well-justified choice given the "same tool set... includes
Skill" reading of the exit criteria, and is called out explicitly as a design decision in the work summary. plugin.jsonleft unbumped, consistent with ADR-301's own merged precedent (f03d11e/4c3a4f6/29349c8) — not a regression introduced here.- No async/CancellationToken/security/N+1-style concerns apply — this change is agent/skill prose only, no application code.
- CI (
build-and-test) passes.
No Priority 1–4 issues found. No inline comments posted.
Status: approved.
jodavis
left a comment
jodavis
left a comment
There was a problem hiding this comment.
Important change: The refactorer doesn't just run at the end. It should run after each red/green turn. In other words, the cycle is red-green-refactor. This gives the refactor agent an opportunity each turn to guide the process toward well-designed code. It doesn't have to make a change on every turn, but it get an opportunity to look for duplicated code, readability issues, etc. after each code change.
…just once at the end Addresses PR #53 review feedback (jodavis, CHANGES_REQUESTED): the refactorer shouldn't only run once after a component reaches done — it should run after each red/green turn, giving it a genuine opportunity every cycle to clean up duplication, brittle setup, or naive implementations as the component is built, not just a single pass at the very end. - implement-tdd/SKILL.md: tdd-refactorer is now spawned up front alongside tdd-tester/tdd-implementer (step 1, "Spawn the trio"). A new step 6 ("Refactor turn") runs after every real green (step 3's `green` reply, or step 4's `resolve_directly` resolution) — never after `structural-green`, since no real behavior exists yet to clean up. Removed the old standalone post-done pass (former step 7); the final green before `tdd-tester` reports `done` already gets its own refactor turn via the interleaved step, so a separate end-of-loop pass is redundant. Commit granularity simplifies as a result: since refactor turns are now staged mid-loop like every other turn (never committed individually), there's a single real commit per component once `tdd-tester` reports `done` — the previous "second, separate commit when tdd-refactorer makes changes" concept no longer applies now that refactoring happens continuously rather than as a distinct post-done phase. - tdd-refactorer.md / tdd-refactor-turn/SKILL.md: updated role description, "Use this skill when" trigger, and turn-discipline wording to reflect the per-green cadence instead of "runs exactly once per component, after done". Verified via a new dry run (logged at ~/.dev-team/jodavis/agent-plugins/logs/ADR-302-tdd-refactorer-dryrun3-interleaved.log): a synthetic TtlValidator2 component's loop runs three green turns; the first two refactor turns report no-refactor-needed (nothing to consolidate yet), and the third — triggered immediately after the green that creates a second near-identical ValueError-raising test — consolidates the pair into a parameterized test mid-loop, before tdd-tester even reports done. Full suite stays green (3 passed) throughout. Note: this is a real design deviation from _spec_TddForImplementation.md's merged "Refactor, after done" text (one turn, once, after done) and its "second, separate commit" decision. Scoped this fix to the reviewed implementation files per fix-pr's scope; the spec text itself was left unchanged — flagging as a follow-up in the PR reply rather than editing the spec here.
|
Thanks — agreed, this is the right call. Addressed in
Structural-green turns are excluded from triggering a refactor turn (no real behavior exists yet to clean up at that point — just a compiling stub). One knock-on effect worth flagging: this simplifies commit granularity. Since refactor turns are now staged mid-loop like every other turn (never committed individually, per the existing "never commit mid-loop" rule), there's a single real commit per component once Verified with a new dry run ( Note: |
jodavis-claude
left a comment
jodavis-claude
left a comment
There was a problem hiding this comment.
Sign-off review — ADR-302
Re-reviewed after commit 2b38859 (addressing jodavis's CHANGES_REQUESTED review from
2026-07-06T02:34:59Z).
Prior thread — resolved. jodavis's request ("the refactorer... should run after each
red/green turn... red-green-refactor... it doesn't have to make a change on every turn, but it
gets an opportunity each turn") is fully addressed in 2b38859:
implement-tdd/SKILL.mdstep 1 now spawnstdd-refactorerup front alongside the pair; a new
step 6 ("Refactor turn") runs after every real green (step 3'sgreen, or step 4's
resolve_directly) — never afterstructural-green, correctly, since no real behavior exists
yet to clean up there. The former standalone post-donepass is gone; commit granularity
simplified to a single real commit per component, consistent with everything now being staged
mid-loop.tdd-refactorer.md/tdd-refactor-turn/SKILL.mdrole/trigger/turn-discipline text updated to
match the per-green cadence.- All step-number cross-references were checked and are consistent after the renumbering (old
step 6 "Commit" → step 7; old step 7 "Refactor pass" → step 6; "Staging between turns" and the
"Skills" footer both updated to match). - New dry run (
ADR-302-tdd-refactorer-dryrun3-interleaved.log) demonstrates the requested
cadence concretely: 3 real-green turns on a syntheticTtlValidator2component, the first two
refactor turns reportno-refactor-needed, and the third — right after the green that creates
a second near-identicalValueErrortest — consolidates the pair into a parameterized test
mid-loop (beforetdd-testerreportsdone), full suite staying green (3 passed) throughout.
This is a stronger demonstration than the original two dry runs alone. - The original two exit-criterion dry runs (consolidation / no-cleanup-opportunity scenarios)
remain valid and unaffected by this change.
Marking this thread resolved.
New scan of modified files — no new Priority 1–4 issues. All three files
(tdd-refactorer.md, tdd-refactor-turn/SKILL.md, implement-tdd/SKILL.md) are internally
consistent, keep the established agent/turn-skill split, and CI (build-and-test) passes.
One non-blocking observation (Priority 4 — documentation): the commit message and PR reply
both correctly flag that this is a real deviation from _spec_TddForImplementation.md's merged
"Refactor, after done" text and its "second, separate commit" decision, which still describe
the old once-at-the-end cadence. Since jodavis (the spec's own author) requested this exact
change, I'm treating it as an implicitly-authorized supersession of that spec text rather than a
blocking gap — but recommend updating that section of the spec in a follow-up (or when this
epic's spec is finalized into a _doc_*.md) so it doesn't mislead whoever reads it next for
ADR-303/304/305.
Sign-off: approved.
… agent+turn-skill split Follow-up to commit 2b38859, per explicit user decision to fold the sign-off reviewer's Priority 4 recommendation into this same PR rather than deferring it: _spec_TddForImplementation.md still described the old once-at-the-end tdd-refactorer cadence and the "second, separate commit" rule that no longer applies, and described tdd-tester/tdd-implementer as monolithic agent files (predating ADR-301's merged agent+turn-skill split) and tdd-refactorer.md as single self-contained file (predating this PR's own tdd-refactor-turn split). - "TDD ping-pong protocol" section: tdd-refactorer's role description, "The loop"'s numbered steps, the resolve_directly escalation text, the "Refactor, after done" subsection (renamed "Refactor, interleaved after every green"), and the "Commits" section all now describe a tdd-refactorer turn after every real green (never after structural-green), staged mid-loop like every other turn, with a single real commit per component rather than a second separate one. - "Key Classes / Files": tdd-tester.md/tdd-implementer.md/tdd-refactorer.md bullets rewritten to describe the actual thin-agent + turn-mechanics-skill split (tdd-red-turn / tdd-green-turn / tdd-refactor-turn), matching what's merged on disk instead of describing the protocol as embedded directly in the agent files. implement-tdd/SKILL.md bullet updated to describe spawning the trio together and a single commit. - Data Flow diagram updated to match (trio spawned together, refactor turn after every real green, single commit-changes). - ADR-302's own Tasks-section entry (title, description, and exit criterion #2) reworded to match the interleaved behavior instead of "one turn after a component reaches done". No code changes in this commit — spec/documentation only.
|
Per the sign-off review's Priority 4 note and explicit user direction, updated
No code changes in this commit — spec/documentation only. |
jodavis-claude
left a comment
jodavis-claude
left a comment
There was a problem hiding this comment.
Sign-off review (2nd pass) — ADR-302
Re-reviewed after commit f07acf4 ("Update spec to describe the interleaved refactor cadence
and agent+turn-skill split").
Follow-up addressed. My previous sign-off comment recommended updating
_spec_TddForImplementation.md's "Refactor, after done" / "Commits" text and the ADR-302 exit
criteria wording, since they still described the old once-at-the-end cadence after 2b38859
interleaved tdd-refactorer turns after every real green. f07acf4 is a spec/doc-only change
(no code files touched, confirmed via git show --stat) that:
- Renames "Refactor, after
done" → "Refactor, interleaved after every green" and rewrites it,
"The loop"'s numbered steps, theresolve_directlyescalation text, and "Commits" to describe
the actual interleaved-turn/single-commit behavior now on disk. - Rewrites "Key Classes / Files" for
tdd-tester.md/tdd-implementer.md/tdd-refactorer.mdto
describe the real thin-agent + turn-mechanics-skill split (tdd-red-turn/tdd-green-turn/
tdd-refactor-turn), matching what's merged rather than the pre-ADR-301 monolithic-file
description. - Updates the Data Flow diagram and ADR-302's own Tasks-section title/description/exit-criterion
#2 wording to match.
Spot-checked the updated spec text against the actual implement-tdd/SKILL.md,
tdd-refactorer.md, and tdd-refactor-turn/SKILL.md on disk — consistent (interleaved
after-every-real-green cadence, excluding structural-green, single commit per component,
agent+turn-skill split). This closes out the one non-blocking observation from my prior sign-off
pass; there is no longer a stale spec/implementation gap for a future reader (including
ADR-303/304/305) to trip over.
No new Priority 1–4 issues — this commit only touches prose in _spec_TddForImplementation.md.
CI (build-and-test) still passes.
Sign-off: approved.
…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.
…plement-direct (#54) * ADR-303: Rewrite implement-task as a per-component dispatcher, add implement-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. * ADR-303: Rework implement-task dispatcher and implement-direct per PR #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 --------- Co-authored-by: Joe Davis <ElwoodMoves@hotmail.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
3 participants
Work item
ADR-302 — Add the
tdd-refactorersub-agentand wire a post-
donerefactor turn (including parameterized-test consolidation) into theimplement-tddorchestration skill, completing the tdd-tester/tdd-implementer/tdd-refactorertrio for one Testable component's implementation loop. Depends on
ADR-301 (merged, #52).
Changes
plugins/dev-team/agents/tdd-refactorer.md(new) — the third tdd-agent. Constraint-firstdefinition modeled on
tdd-tester.md/tdd-implementer.md. Tools:Read,Glob,Grep,Edit,Write,Bash,Skill— same set as the other two tdd-agents. Scoped tobehavior-preserving changes only, to test or production files, only after a component
reaches
done; any behavior gap it notices is reported as a new red fortdd-tester, neverfixed in place; never spawns further sub-agents.
plugins/dev-team/skills/tdd-refactor-turn/SKILL.md(new) — turn-mechanics skilltdd-refactorerinvokes each turn (exactly one, no retry/escalation tiers), modeled ontdd-red-turn/tdd-green-turn: reviews the component for duplication/brittlesetup/naive implementations, applies one behavior-preserving change (or none), reruns the
full component suite, and replies
refactored: <summary>orno-refactor-needed.plugins/dev-team/skills/implement-tdd/SKILL.md(modified) — added step 7 ("Refactor pass")after step 6's commit: spawns
tdd-refactorerviaAgent, sends the spec's exact turnmessage, and calls
commit-changesagain (a second, separate commit) only if the reply isrefactored: <summary>. Replaced the prior "Scope note" that deferred this wiring toADR-302. Updated the skill's description and "Skills" footer to reference
tdd-refactor-turn.Design decisions
tdd-refactor-turnas a separate skill rather thankeeping
tdd-refactorer.mdfully self-contained (the spec's own text predates this split).Matches the ADR-301-established agent+skill pattern for
tdd-tester/tdd-implementer, andis a direct reading of the exit criteria's "same tool set as the other two tdd-agents"
(which includes
Skill).plugins/dev-team/.claude-plugin/plugin.jsonversion bump — left unchanged,consistent with ADR-301's merged commits, which also didn't bump it despite the spec's
general "version bump per existing convention" note.
Agent/SendMessagesub-agent spawning is available in this environment yet(
developer.mddoesn't have that grant — ADR-304's scope). Verified via two manually-drivendry runs against a real Python component under pytest (same methodology as ADR-301's merged
dry runs):
TtlValidatorcomponent reacheddonewith two near-identicalValueError-raising tests differing only by input literal; the refactor pass consolidatedthem into a single parameterized test, full suite still green (3 passed) with both cases
retained.
no-refactor-neededwithzero file changes (confirmed via
md5sum).