fix: address review feedback on swamp-getting-started skill#1175
fix: address review feedback on swamp-getting-started skill#1175
Conversation
Replace undefined Track A/B/C terminology with self-documenting model-type labels, align the generated CLAUDE.md onboarding detection to use the same `swamp model search --json` check as SKILL.md, add error handling for pre-check command failure, and add missing skill_assets_test.ts coverage. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
Clean, well-scoped PR that addresses specific adversarial review feedback from #1174. All three items are handled correctly.
Blocking Issues
None.
Suggestions
- Minor terminology in SKILL.md: The
references/tracks.mdfilename is a vestige of the old Track A/B/C naming. Not worth renaming in this PR (it would require updating the embedded link in SKILL.md and the test assertions), but worth noting for a future cleanup pass if the team prefers full consistency.
Details
Terminology replacement (SKILL.md): All Track A/B/C references are replaced with self-documenting names (command/shell, "local typed models", "extension models"). The replacement is thorough — I count 9 individual edits across 6 logical locations. The referenced tracks.md file already uses descriptive terminology, so no inconsistency exists.
Detection alignment (repo_service.ts): The generated CLAUDE.md now instructs the AI to run swamp model search --json instead of checking the models/ directory. This matches the SKILL.md pre-check, eliminating the divergence risk noted in the review (extension models living outside models/).
Error handling (SKILL.md): The new paragraph for pre-check failures is well-placed between the success-with-models path and the no-models path, covering the third case (command failure). Delegation to swamp-repo for uninitialized repos is the right recovery path.
Tests (skill_assets_test.ts): Both new tests follow the established patterns exactly — the inclusion test matches "SkillAssets includes <name> skill" and the copy test matches "SkillAssets.copySkillsTo copies <name> reference files". Uses @std/assert and the existing withTempDir helper.
DDD: The repo_service.ts change is a string template in a domain service — appropriate placement. The terminology cleanup is good ubiquitous language practice (replacing internal jargon with domain terms).
CLAUDE.md compliance: No new .ts files, no any types, no libswamp import boundary violations, tests next to source, focused changes with minimal blast radius. ✅
There was a problem hiding this comment.
Adversarial Review
Critical / High
None.
Medium
repo_service.ts:613-616— Generated CLAUDE.md doesn't handle command failure. The SKILL.md now has explicit error-handling instructions for whenswamp model search --jsonfails (repo not initialized, corrupt config, swamp not installed). However, the generated CLAUDE.md inrepo_service.tsonly says: "If no models are returned (empty result), you MUST immediately invoke theswamp-getting-startedskill." A command failure (non-zero exit, stderr output) is not the same as an empty result. An AI reading the generated CLAUDE.md could interpret a failed command as something other than "no models returned" and skip the onboarding trigger entirely — exactly the silent-skip problem the SKILL.md fix addresses. Consider adding a brief clause like "If the command fails or returns no models" to keep the two layers fully aligned.
Low
SKILL.md:88vsSKILL.md:82-84— Tension between On Failure default and Verify guidance. State 1's On Failure says to "default to acommand/shellmodel. It works everywhere" while the Verify step says to "Only usecommand/shellif the user's goal is genuinely a one-off ad-hoc command." This contradiction pre-dates the PR (it existed with the Track A terminology), but since these lines were touched, it's worth noting. An AI following both instructions literally could oscillate. A one-line qualifier like "default tocommand/shellfor the walkthrough demonstration" would resolve it.
Verdict
PASS — Clean, well-scoped changes. The terminology alignment and error-handling additions are correct. The medium finding is a gap in the generated CLAUDE.md trigger that could be closed with a small wording tweak but does not block merge since the SKILL.md pre-check handles the failure case once the skill is actually invoked.
Summary
Addresses three items from the adversarial review on #1174:
Track A/B/C terminology undefined: Replaced all 6 occurrences of the
undocumented Track A/B/C labels in SKILL.md with self-documenting
model-type names (
command/shell, "local typed models", "extensionmodels"). This eliminates ambiguity — an AI no longer needs to infer the
mapping from context.
Dual detection mechanism inconsistency: The generated CLAUDE.md (in
repo_service.ts) told the AI to "check if themodels/directorycontains any model YAML files", while SKILL.md's pre-check uses
swamp model search --json. These could diverge (e.g., extension modelslive in
extensions/models/and get symlinked). Now both layers use thesame CLI-based check, which is the authoritative source for registered
models.
Pre-check error handling: If
swamp model search --jsonfails (reponot initialized, corrupt config, swamp not installed), the skill now
explicitly tells the user what went wrong and delegates to
swamp-repofor setup rather than silently skipping onboarding.
Also adds missing
skill_assets_test.tscoverage forswamp-getting-started(inclusion test + reference file copy test), matching the pattern used by all
other bundled skills.
Impact
Skill-only changes — no runtime code paths affected beyond the generated
CLAUDE.md string in
repo_service.ts. The detection alignment means new reposwill get consistent onboarding behavior regardless of whether the trigger comes
from the generated CLAUDE.md or the SKILL.md pre-check.
Test plan
deno checkpassesdeno lintpassesdeno fmtpassesdeno run test src/infrastructure/assets/skill_assets_test.ts— 31/31tests pass (2 new)
🤖 Generated with Claude Code