fix(implementation): demote Clean-Architecture layer names to an example#457
Conversation
The implement skill baked Core/Domain/Application/Infrastructure — .NET/DDD/ Clean-Architecture vocabulary — as a universal execution order. A consumer without layered architecture (Rails, Go, data pipeline, SPA) got an opinion that does not map, breaking the repo-agnostic design boundary. Restate the universal principle as dependency direction (implement depended-upon components before their dependents, respecting the project's own dependency direction) and demote the Clean-Architecture layer names to a clearly-marked ".NET, for example" illustration. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 1m 4s —— View job PR Review
Summary: This is a clean, correctly-scoped fix. The change precisely addresses the problem stated in #404 — baking Clean-Architecture vocabulary into guidance presented as universal — and restates the principle in a vocabulary-agnostic way. No unrelated changes. What changedSKILL.md L90 — "Dependency direction" subsection:
feature.md L9 — Step 3:
EvaluationCorrectness: The universal principle ("implement the depended-upon components before the ones that depend on them") is stated accurately in both files. The .NET example is clearly illustrative, not prescriptive. No functional change to the guidance for .NET consumers — same advice, now correctly framed. Repo-agnostic design rule (PLUGIN-PHILOSOPHY): Fix directly addresses the "runtime behavior must not assume an undocumented consumer layout" boundary. A Rails, Go, or SPA consumer no longer receives an opinion that doesn't map. Consistency between the two changed files: Both files now use the same structure (agnostic principle → "for example" .NET illustration). Minor asymmetry: SKILL.md adds "a project with a different structure applies the same principle to its own layout" as a closing sentence; feature.md omits it. The omission is fine — feature.md's opening clause ("following the project's own dependency direction") already establishes the scope, so the example reads as an illustration rather than a rule. No issues found. The diff is minimal, targeted, and does not touch routing, contracts, or anything unrelated to the stated fix. Verdict: approved. Ready to merge. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a998aafd04
ℹ️ 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".
Version bump and CHANGELOG entry so existing installs receive the rewritten prompts from the Clean-Architecture layer-vocabulary agnosticism fix. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 1m 3s —— View job PR Review (updated — v0.7.1 bump commit reviewed)
The PR now has two commits. The first was already reviewed and approved. This review covers the second commit ( What changed in the new commit
EvaluationCorrectness: Version bump and CHANGELOG are internally consistent with each other and with the skill changes from the first commit. Nothing mismatches. Delivery contract: The manifest version is the update cache key. With Scope discipline: The bump commit touches only the two files it needs to touch — No issues found. Verdict: approved. Both commits are clean and correct. Ready to merge. |
# Conflicts: # plugins/implementation/CHANGELOG.md
|
Claude finished @kyle-sexton's task in 1m 21s —— View job PR Review (post-merge update — v0.7.2)
This review covers the merge commit ( What changed in the merge commit
Final state of the skill files (unchanged by merge)SKILL.md L90 — agnostic principle first, feature.md L9 — heading updated to EvaluationMerge correctness: The conflict was resolved cleanly. The CHANGELOG now has both Delivery contract: With Repo-agnostic design rule (PLUGIN-PHILOSOPHY): Satisfied. The guidance no longer prescribes a layout consumers may not have. No issues found. The merge was executed correctly, the version collision is resolved, and the full PR is clean across all three commits. Verdict: approved. All three commits are correct and internally consistent. Ready to merge. |
Problem
The
implementationplugin'simplementskill baked Core/Domain/Application/Infrastructure — .NET / DDD / Clean-Architecture vocabulary — into text presented as a universal execution order:plugins/implementation/skills/implement/SKILL.md(Dependency direction) — "Implement inner layers first (Core/Domain), then outer layers (Application/Infrastructure)"plugins/implementation/skills/implement/context/feature.md(step 3) — "Inner layers first — implement Core/Domain types before Application/Infrastructure"A consumer without a layered architecture (Rails, Go, a data pipeline, an SPA) gets an opinion that does not map, breaking the repo-agnostic design boundary (PLUGIN-PHILOSOPHY "Design boundary": runtime behavior must not assume an undocumented consumer layout).
Fix
Keep the agnostic principle and restate it as dependency direction — implement the depended-upon components before the ones that depend on them, respecting the project's own dependency direction — then demote the Clean-Architecture layer names to a clearly-marked ".NET, for example" illustration. No routing/contract change; the guidance still lives inline.
Bumps the
implementationplugin to 0.7.1 with a matching CHANGELOG entry so existing installs receive the rewritten prompts.Verification
markdownlint-cli2@0.23.0(repo-pinned) on all changed markdown files: 0 errors.plugin.jsonvalidated as parseable JSON; version 0.7.0 -> 0.7.1.Related
Closes #404