ADR-301: Add tdd-tester/tdd-implementer agents and the implement-tdd red/green loop

[jodavis-claude]

Work item

ADR-301: Add the tdd-tester and tdd-implementer sub-agent definitions and the implement-tdd skill that drives one Testable component through the structural-then-behavioral red/green ping-pong loop — the behavior-selection rubric plus all three escalation tiers — up through the component reaching done and being committed. (The post-done tdd-refactorer pass is out of scope; that's ADR-302.)

Changes

• plugins/dev-team/agents/tdd-tester.md (new) — Tester half of the TDD pair. Tools: Read, Glob, Grep, Edit, Write, Bash only (no Task/Agent/SendMessage). Scoped to test-files-only. Covers structural-vs-behavioral red decision logic, the three-point behavior-selection rubric (skip Wrapper-tier, cheapest structural fit, happy-path-first), and the one-line reply contract (structural-red/red/done). References test-driven-development's Practice rules (AAA structure, Red must fail for the right reason, Arrange/Act frozen after first green, Naming convention) by name only.
• plugins/dev-team/agents/tdd-implementer.md (new) — Implementer half, same tool grant, scoped to production-files-only. Covers the smallest-possible-change rule, structural-turn stub resolution, the Tier 1 pair-internal retry, and the Tier 2/3 escalate reply (clarify/resolve_directly/split_scope).
• plugins/dev-team/skills/implement-tdd/SKILL.md (new) — orchestration skill invoked by Developer (not the sub-agents). Drives: per-component log file setup, spawning the pair, the "needs a structural turn?" judgment, structural and behavioral turn routing using the spec's exact verbatim turn-message templates, the Tier 1 mechanical relay, Tier 2 escalation handling (including a resolve_directly path that calls out to implement-direct), Tier 3 best-effort fallback, protocol-violation handling via git diff --name-only after every turn, and the post-done commit-changes call. Ends cleanly at "done → commit" — no post-done refactor step.

Design decisions

• ADR-300 (PR ADR-300: Standardize TDD practice rules and add logging to coverage checklist #51) is still unmerged. The Practice-rule headings (AAA structure, Red must fail for the right reason, Arrange/Act frozen after first green, Naming convention) are referenced by name in these files on the assumption PR ADR-300: Standardize TDD practice rules and add logging to coverage checklist #51 lands unchanged before this PR merges. If PR ADR-300: Standardize TDD practice rules and add logging to coverage checklist #51's final heading names differ, a follow-up fix will be needed.
• No live Agent/SendMessage sub-agent spawning available in this session. Both required dry runs were driven by manually simulating the orchestrator/tester/implementer roles against a synthetic TTLCache component, running real pytest builds/tests at each turn so red/green transitions are genuine. The protocol logic and build/test execution were real; the sub-agent invocation mechanism was simulated. Re-validating with a live multi-agent run once developer.md gains SendMessage (ADR-304) would be worthwhile.
• resolve_directly stood in for implement-direct (ADR-303, not yet built). In dry run 2, Developer implemented the disputed piece (FIFO eviction) directly itself, then ran the disputed test plus the full suite to confirm the regression check passes.
• Tier 1 kept pair-internal. tdd-implementer.md owns the Tier 1 judgment; implement-tdd/SKILL.md only describes Developer's role as a mechanical, no-judgment relay of the revise-request note and tdd-tester's reply, since neither sub-agent has Agent/SendMessage to message each other directly.

Dry run results

Dry run 1 (TTL cache, empty test file to full coverage) — sequence of one-line replies produced (full transcript: ~/.dev-team/jodavis/agent-plugins/logs/ADR-301-tdd-TTLCache-dryrun.log):

structural-red: test_set_then_get_returns_value — ModuleNotFoundError: No module named 'ttl_cache'
structural-green: test_set_then_get_returns_value
red: test_set_then_get_returns_value — assert None == 1 (stub get() always returns None)
green: test_set_then_get_returns_value
structural-red: test_get_after_ttl_elapsed_returns_none — TypeError: unexpected keyword argument 'clock'
structural-green: test_get_after_ttl_elapsed_returns_none
red: test_get_after_ttl_elapsed_returns_none — assert 1 is None (expiry not enforced yet)
green: test_get_after_ttl_elapsed_returns_none
red: test_get_after_elapsed_time_respects_ttl_boundary[60-None] — cheapest-structural-fit: added as a new case on the (now-)parameterized elapsed-time test
green: test_get_after_elapsed_time_respects_ttl_boundary[60-None]
red: test_constructor_rejects_non_positive_ttl_seconds — Failed: DID NOT RAISE ValueError
green: test_constructor_rejects_non_positive_ttl_seconds
green: test_get_missing_key_returns_none  (already-implemented member, new input)
green: test_set_overwrites_existing_key  (already-implemented member, new input)
done: TTLCache — 7 tests covering constructor validation, set/get happy path, the missing-key branch, TTL expiry (past and exact boundary), and overwrite; __len__ correctly excluded as Wrapper-tier

Ends with every branch of TTLCache covered. Confirms the structural/behavioral split, cheapest-structural-fit rubric, and Wrapper-tier skip all fire as designed.

Dry run 2 (forcing a Tier 2 resolve_directly escalation) — full transcript: ~/.dev-team/jodavis/agent-plugins/logs/ADR-301-tdd-TTLCache-resolve-directly-dryrun.log:

structural-red: test_set_evicts_oldest_entry_when_max_size_exceeded — TypeError: unexpected keyword argument 'max_size'
escalate: max_size eviction policy is undefined (FIFO vs LRU vs reject-insert) and requires restructuring internal storage — recommended_action: resolve_directly

Developer resolved directly: implemented FIFO eviction, then ran the disputed test plus the full 7-test suite. Result: all 7 tests pass, including the disputed test — retained unmodified in the test file (not discarded or rewritten) and counted toward coverage, exactly as the exit criterion requires.

Tests

No unit/E2E tests apply — the deliverable is markdown-only agent/skill definition files with no executable production code (per missing-test-harness, this repo has no test harness for prose/instruction files). The two dry runs above are this repo's documented stand-in for automated verification of this kind of deliverable.

Known ambiguities for reviewer attention

1. ADR-300 (PR ADR-300: Standardize TDD practice rules and add logging to coverage checklist #51) is still open/unmerged. If its final Practice-rule heading names differ from what's referenced here, a follow-up update to these three files will be needed.
2. Both dry runs were driven by manually simulating agent turns rather than live Agent/SendMessage spawning, since this session's persona lacks SendMessage and the new agent types aren't yet registered as spawnable.
3. Dry run 2's resolve_directly path stood in for implement-direct (ADR-303, not yet built) by having Developer implement the disputed piece directly inline.

[github-actions]

build-and-test: Python test results

Status: ✅ Passed

Test log

[jodavis-claude]

Solid, careful piece of work overall — the turn-message templates match the spec verbatim, the tool grants and file-scope constraints are exactly as required, both required dry runs are documented with real (not fabricated) build/test output, and the practice-rule references are by-name only as required. I found two protocol-completeness gaps in implement-tdd/SKILL.md that should be addressed before merge (see inline comments) — both are cases where a sub-agent reply that the design allows for isn't actually routed anywhere in the skill's step-by-step instructions, which would leave a future live run of this loop stuck with no next action.

[jodavis-claude]

Sign-off review

Both prior review threads are resolved by commit d6087fd:

1. Step 4 missing an escalate branch for structural turns — fixed. implement-tdd/SKILL.md step 4 now routes tdd-implementer's escalate reply to step 7, with a note that the escalation originated from the structural turn (no Assert exists yet). Step 7's clarify handling correctly distinguishes retrying step 6 vs. re-sending step 4's structural stub-resolution message. tdd-implementer.md's "Structural turns" section now also notes escalation is possible there.
2. Step 5 missing a structural-red branch — fixed. implement-tdd/SKILL.md step 5 now routes an unexpected structural-red reply back to step 4's structural-red handling, correctly framing step 3's skip-structural decision as a heuristic guess rather than a guarantee.

Re-read both modified files in full (plugins/dev-team/skills/implement-tdd/SKILL.md, plugins/dev-team/agents/tdd-implementer.md) against the loop's own internal consistency (all reply branches now route somewhere, no dangling states) and against the spec's turn-message contract — no regressions or new Priority 1-5 issues found. CI (build-and-test) is green.

Approving. Thank you for the quick, well-scoped fix.

[jodavis-claude]

Sign-off review (round 2)

Re-checked this PR after the pipeline looped back from validation. No new commits or review comments have landed since the prior sign-off review (submitted 2026-07-05T02:49:47Z, which approved commit d6087fd) — status is unchanged from that pass:

1. Both prior review threads remain resolved (confirmed via the PR's review-threads API): the step 4 missing-escalate-branch gap and the step 5 missing-structural-red-branch gap, both fixed by commit d6087fd.
2. No files have changed since that commit — re-scanning plugins/dev-team/skills/implement-tdd/SKILL.md and plugins/dev-team/agents/tdd-implementer.md found no new Priority 1-4 issues; all reply branches route somewhere, no dangling states.
3. CI (build-and-test) is green.

Flagging one open item that is outside this review's scope to resolve via further code changes to this PR: the subsequent researcher-validate pass reported exit criterion 1 as partial because tdd-tester.md/tdd-implementer.md reference test-driven-development's Practice-rule headings ("AAA structure", "Red must fail for the right reason", "Arrange and Act are frozen after first green", "Naming convention") by name, but those headings don't yet exist in this branch's test-driven-development/SKILL.md — they're added by ADR-300 (GitHub PR #51), which is still open/unmerged. This was flagged and accepted as a known, deliberate risk as far back as the original task brief (Known ambiguity 1), and confirmed again in fix round 2 that the referenced heading names are an exact match against PR #51's current diff, so no rewrite will be needed here once #51 merges. This is a merge-order/sequencing question (should ADR-301 wait for PR #51 to merge first), not a defect in this PR's diff, and merging another PR is outside what a code review on PR #52 can or should resolve. Re-approving PR #52's own code changes on their merits; the merge-order question needs a decision from the user/orchestrator, independent of this review.

Approving.

[jodavis-claude]

Sign-off review — ADR-301 (PR #52)

Prior review threads: Both Priority-1 protocol-completeness threads from round 1 are confirmed resolved and correctly fixed in the current file content (commit 4c3a4f6, rebased equivalent of d6087fd):

• implement-tdd/SKILL.md step 4 now has an explicit escalate: ... — go to step 7 branch for the structural turn, and step 7's clarify handling correctly distinguishes retrying step 4 vs. step 6 depending on escalation origin.
• Step 5 now has the structural-red: ... — route back to step 4 branch for the behavioral turn.
• tdd-implementer.md's "Structural turns" section now states escalation is a possible outcome there too.

Blocking dependency resolved since last validation: ADR-300 (PR #51) has merged (0a142e4, mergedAt: 2026-07-05T02:57:44Z) and is now an ancestor of both this PR's head and its base branch (feature/ADR-288-tdd-milestone-1). Confirmed test-driven-development/SKILL.md now contains the four Practice-rule headings ("AAA structure", "Red must fail for the right reason", "Arrange and Act are frozen after first green", "Naming convention") verbatim, and tdd-tester.md/tdd-implementer.md's by-name references match exactly — no rewording needed. This resolves the "partial" finding on exit criterion 1 from the prior validation; all four exit criteria are now satisfied.

New issues in modified files: none found. Tool grants remain exactly [Read, Glob, Grep, Edit, Write, Bash] with no Agent/SendMessage/Task in either agent file; file-scope discipline, the escalation tiers, and the behavior-selection rubric are all correctly wired.

Minor, non-blocking note: the PR description's "Design decisions" and "Known ambiguities" sections still say ADR-300/PR #51 "is still unmerged" — this is now stale given the merge above. Worth a quick edit to the PR body before merge, but not a code issue and not blocking sign-off.

Signing off — approved.

[jodavis]

At a high level, the two new agents should each be broken into an agent and a skill. The agent should focus on constraints ("You only modify test code, never product code", and vice versa) and general guidelines ("You always make the simplest, dumbest change possible to make tests pass"). The skill should describe the mechanics of a turn. That skill will give the Developer agent a simple prompt to send as well ("invoke tdd-add-red-test or tdd-make-test-green as examples) so maybe that will help the 'Developer' not need to predict whether a structural or behavioral prompt is needed.

[jodavis-claude]

Addressed the CHANGES_REQUESTED review and all outstanding inline threads in commit 29349c8.

Agent/skill split (the core ask): tdd-tester.md and tdd-implementer.md are now trimmed to constraints and general guidelines only (file scope, "smallest possible change"/dumbest-thing examples, Practice rules by name). The mechanics of a turn — the structural-vs-behavioral decision, the behavior-selection rubric, escalation handling, and reply format — moved into two new skills each agent invokes on every turn:

• plugins/dev-team/skills/tdd-red-turn/SKILL.md — invoked by tdd-tester
• plugins/dev-team/skills/tdd-green-turn/SKILL.md — invoked by tdd-implementer

This directly fixes the turn-ownership inconsistency flagged in the inline threads: Developer no longer predicts or names whether the next turn is structural or behavioral (the old step 3 in implement-tdd/SKILL.md is gone). Developer now sends the exact same generic message every turn — "take your next turn for <Component>" to tdd-tester, "tdd-tester reported: <reply>" to tdd-implementer — and each sub-agent's own skill decides what that means and reports back. On a clarify escalation, Developer resends that same generic message with the answer folded in, rather than tracking whether the escalation originated from a structural or behavioral turn.

Other threads, all replied to inline:

• Log file — dropped entirely rather than mitigated; no shared per-component log, each sub-agent's own transcript holds its build/test output.
• Commit granularity — switched to staging between turns (git add after each verified turn, never a commit) so file-scope checks stay exact without ever committing a red/broken state; a single real commit happens once the component reaches done.
• Added the async/dependency-failure coverage examples to tdd-red-turn's rubric and the C# "dumbest thing possible" examples to tdd-implementer.
• Fixed the forward-referencing escalation wording (the sentence no longer exists in the trimmed agent doc — the detail lives in tdd-green-turn now).

Ready for another look whenever you have a chance.