docs(implementation): document TDD project-policy override#1054
docs(implementation): document TDD project-policy override#1054kyle-sexton wants to merge 2 commits into
Conversation
|
Claude finished @kyle-sexton's task in 1m 32s —— View job Code Review
This is a well-scoped, well-executed documentation PR. The core objective — making the existing project-policy override explicitly discoverable across all execution surfaces without changing the TDD-by-default identity — is accomplished cleanly. Here are my findings, most significant first. Minor issue:
|
|
Claude finished @kyle-sexton's task in 30s —— View job Security Review — PR #1054
FindingsNo security issues found. All six changed files are pure documentation and metadata:
None of the changes introduce code execution paths, new permissions, external URLs, credential handling, injection surfaces, supply-chain dependencies, or GitHub Actions modifications. The diff presents no attack surface beyond the existing plugin runtime that loads these files. |
|
Claude finished @kyle-sexton's task in 1m 17s —— View job Code Review — PR #1054 (re-review after
|
| Surface | Verdict |
|---|---|
| README Configuration section | ✅ "TDD remains the fallback when the consuming project's CLAUDE.md or rules do not declare another cadence." |
| SKILL.md step 2 item 3 | ✅ "TDD is the fallback when the consuming project does not declare another testing cadence … A consumer can opt out in its CLAUDE.md / rules" |
feature.md step 5 |
✅ "first honor the consuming project's testing cadence … that project policy overrides the test-first instructions in this step" |
bugfix.md step 1 + pitfalls |
✅ Both now qualified; "test-first, always" is gone |
Unchanged items confirmed intentional
- SKILL.md frontmatter: still
"TDD by default"in the one-line description. Brevity is appropriate for a summary field; the full qualifier lives in step 2. The PR description explicitly states this preserves the plugin's "TDD-by-default identity" — the trade-off is deliberate. plugin.jsondescription: still"TDD-by-default cadence"for the same reason.refactor.md: left untouched. It carries no test-first instructions, so there is nothing to qualify.
Semver and changelog
- 0.7.8 → 0.7.9 patch bump is appropriate for a documentation-only, no-behavior-change PR.
CHANGELOG.md## [0.7.9]entry is well-structured, scoped correctly, and cross-references issue implementation: TDD-by-default fires when consumer CLAUDE.md is silent — no userConfig toggle #406.
Verdict
Both commits are clean. No outstanding issues. This PR is ready to merge.
|
Claude finished @kyle-sexton's task in 1m 51s —— View job Security Review — PR #1054
FindingsNo security issues found. All six changed files are pure documentation and metadata:
Pre-computed shell context ( Prompt-injection surface: the skill None of the changes introduce code execution paths, new permissions, external URLs, credential handling, injection surfaces, supply-chain dependencies, or GitHub Actions modifications. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9925c11724
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ## Sequence | ||
|
|
||
| 1. **Reproduce first (test-first, always)** — write a failing test that demonstrates the bug before touching any production code. If you can't reproduce it in a test, you can't prove you fixed it. Test name should describe the bug: `Should_ReturnError_When_InputIsNull`, not `TestFix42`. Invoke `/tdd:principles` via Skill tool (when the `tdd` plugin is installed) for test design guidance (what kind of test, where it goes, what to assert). Bug fixes are the strongest case for test-first — the failing test IS the bug report | ||
| 1. **Reproduce first (test-first by default when project policy is silent)** — first honor the consuming project's testing cadence from its `CLAUDE.md` / rules; that project policy overrides the test-first instructions in this step. When the project declares no cadence, write a failing test that demonstrates the bug before touching any production code. If you can't reproduce it in a test, you can't prove you fixed it. Test name should describe the bug: `Should_ReturnError_When_InputIsNull`, not `TestFix42`. Invoke `/tdd:principles` via Skill tool (when the `tdd` plugin is installed) for test design guidance (what kind of test, where it goes, what to assert). Under the fallback cadence, bug fixes are the strongest case for test-first — the failing test IS the bug report |
There was a problem hiding this comment.
Align bugfix opt-out with later steps
When a repo declares a tests-after cadence, this new branch skips creating the step-1 failing test, but the rest of the bugfix sequence still depends on that artifact (the failing test from step 1 should now pass and the failing-test checkpoint). That leaves the documented opt-out internally inconsistent for bug fixes and can push agents back into test-first despite the project policy; make the later verification/checkpoints conditional or spell out the tests-after bugfix sequence.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
| # | Finding | Classification | Evidence | Reacted |
|---|---|---|---|---|
| 1 | Bugfix opt-out is internally inconsistent: step 1 now defers test-first to project policy, but step 4 ("the failing test from step 1 should now pass") and the first Checkpoint still assume that step-1 failing test unconditionally | VALID | At head 9925c11, step 1 defers to the consuming project's CLAUDE.md/rules cadence, but context/bugfix.md step 4 and the "Failing test committed" checkpoint reference the step-1 failing test with no otherwise-style hedge — the same gap step 1 and the Common pitfalls entry already close. Under a project-declared tests-after cadence the step-1 test is never written, so those two spots are internally inconsistent. |
👍 |
Scope (safe-tier babysit): defer-only, not a prescribed tests-after sequence. Applying the same hedge already used in step 1 and the Common pitfalls entry; deliberately not spelling out a tests-after bugfix flow, since that would resolve a design tension this PR leaves open (step 1 itself calls bug fixes "the strongest case for test-first — the failing test IS the bug report"). Suggested edits:
- Step 4 →
**Verify the fix** — under the test-first fallback, the failing test from step 1 should now pass; otherwise verify per the project's declaredCLAUDE.md/ rules testing cadence. Run the full test suite for the affected project — your fix may have side effects - First checkpoint →
Failing test committed first (proves the bug exists — optional but valuable for git history; applies under the test-first fallback, not a project-declared tests-after cadence)
I could not land the commit this cycle: a PostToolUse formatter hook on this machine deletes tool-written files inside the babysit worktree (git-written files persist), so the edit above is left for the author or a later clean run to apply. Not resolving this thread (safe tier).
|
🔒 babysit-prs lane claiming this PR for on-branch fix work this cycle (amendment-round: 16, safe tier). Will fix clear branch-owned findings and push; will not resolve threads or merge (safe tier). |
Closes #406
Summary
Documents the implementation plugin's existing project-level testing-cadence override without changing its advertised TDD-by-default identity. TDD remains the fallback when a consuming project is silent; an explicit testing policy in the project's
CLAUDE.mdor rules now clearly takes precedence in every execution mode.This gives teams a discoverable, repository-scoped opt-out while keeping the plugin's zero-configuration default unchanged.
Fix
/implementation:implementcadence that project policy overrides both the fallback and mode-context guidance.test-first, always.userConfigkey deferred, matching the accepted operator decision.Research
The implementation follows Anthropic's current official scope model:
CLAUDE.mdas team-shared instruction space for build/test instructions, coding standards, and common workflows.userConfigas user/managed-scoped and states that project and local plugin-option entries are ignored.Together, those sources support keeping testing cadence in project instructions rather than introducing a user-scoped plugin option.
Verification
test-first, always.claude plugin validate plugins/implementation— passed before and after rebasing.git diff --check origin/main...HEAD— passed after rebasing.## [0.7.9]entry.plugins/implementation/**.origin/mainatb123108ed745c51478d77737a2a01480623fd2ae.Local disclosure: the repository's canonical Bash changed-skill/changelog wrappers and markdownlint were not completed locally. The default
bashcommand resolves to an unconfigured WSL environment, direct Git Bash wrapper attempts exceeded the strict local timebox, and no standalone markdownlint binary is installed. The focused direct equivalents above passed; canonical wrappers and markdownlint are left to required CI.Related