Review Summary

This PR correctly implements section-marker-based merging for shared instruction files (CLAUDE.md, AGENTS.md), fixing the user content overwrite issue described in #628.

No Blocking Issues

The implementation is solid and follows project guidelines.

Code Quality ✓

• TypeScript strict mode: No any types, proper type annotations throughout
• Named exports: Follows project conventions
• Error handling: Proper use of instanceof Deno.errors.NotFound checks
• Atomic writes: Uses atomicWriteTextFile for safe file updates

DDD Compliance ✓

RepoService is appropriately designed as a Domain Service:

• Stateless (version passed to constructor, paths passed to methods)
• Coordinates file operations without owning state
• Clear separation between shared-file tools (claude/opencode/codex) and tool-specific files (cursor/kiro)

Test Coverage ✓

Excellent coverage with tests for all four cases:

1. No file exists → creates with managed section
2. File has markers → replaces content between markers
3. Legacy swamp content (no markers) → migrates to marked section
4. No swamp content → prepends managed section

Plus explicit tests verifying cursor/kiro files are still fully overwritten (no markers).

Suggestions (Non-blocking)

Minor redundancy in migrateLegacyInstructions at line 514:

const prefix = before.length > 0 ? before : "";

This could simply be const prefix = before; since an empty string already equals "". Not worth changing for this PR.

🤖 Generated with Claude Code

Adversarial Review

Critical / High

1. HIGH: Missing END marker causes cryptic crash (repo_service.ts:417-423)

   The ensureInstructionsSection method checks only for the BEGIN marker before calling replaceManagedSection:

   if (existingContent.includes(INSTRUCTIONS_SECTION_BEGIN)) {
     const updatedContent = this.replaceManagedSection(...);

   But replaceManagedSection throws if either marker is missing:

   if (beginIndex === -1 || endIndex === -1) {
     throw new Error("Internal error: managed section markers not found");
   }

   Breaking example: User accidentally deletes the END marker from their CLAUDE.md:

   <!-- BEGIN swamp managed section - DO NOT EDIT -->
   # Project
   ...template content...
   (END marker accidentally deleted)
   
   ## My Custom Rules

   Running swamp repo upgrade throws "Internal error: managed section markers not found" — a confusing message that doesn't help the user fix their file.

   Suggested fix: Check for both markers in ensureInstructionsSection. If only BEGIN exists, either:

   • Fall through to case 4 (prepend) and warn the user, OR
   • Add the missing END marker at a reasonable location

2. HIGH: Legacy migration duplicates content on partial template match (repo_service.ts:461-492)

   The migrateLegacyInstructions method searches for exact strings:

   const startPattern = /# Project\n\nThis repository is managed with \[swamp\]/;
   const endMarker = "Use `swamp --help` to see available commands.\n";

   If the start pattern matches but the end marker doesn't (user modified it slightly), the fallback prepends the new section while keeping all old content:

   if (endIndex === -1) {
     return newSection + "\n" + separator + content;
   }

   Breaking example: User has legacy CLAUDE.md where they changed one word:

   # Project
   
   This repository is managed with [swamp]...
   ...
   Use `swamp --help` to see commands.  ← changed "available commands" to "commands"

   After upgrade, they get:

   <!-- BEGIN swamp managed section -->
   ...new template...
   <!-- END swamp managed section -->
   
   # Project
   
   This repository is managed with [swamp]...
   ...
   Use `swamp --help` to see commands.

   The user now has duplicate swamp instructions — the old template plus the new managed section.

   Suggested fix: When start matches but end doesn't, scan for likely end boundaries (e.g., look for ## headers that aren't part of the known template) rather than blindly prepending. Or at minimum, warn the user that migration was partial.

Medium

3. MEDIUM: Reversed markers corrupt file (repo_service.ts:716-738)

   replaceManagedSection doesn't validate that beginIndex < endIndex. If a user (or merge conflict) produces:

   <!-- END swamp managed section -->
   content
   <!-- BEGIN swamp managed section - DO NOT EDIT -->

   The method produces malformed output with duplicated/corrupted content because before captures up to BEGIN (including END) and after captures from END position (including BEGIN and beyond).

   Suggested fix: Add if (endIndex < beginIndex) throw new Error(...) or treat as invalid markers.

4. MEDIUM: Multiple marker pairs cause silent data loss (repo_service.ts:722-723)

   indexOf finds only the first occurrence. If a file has duplicate BEGIN/END pairs (e.g., from a bad merge):

   <!-- BEGIN swamp managed section -->
   ...section 1...
   <!-- END swamp managed section -->
   
   <!-- BEGIN swamp managed section -->
   ...section 2...
   <!-- END swamp managed section -->

   Only the first section is replaced; the second is treated as user content. On subsequent upgrades, only the first is updated. This could lead to confusion and stale instructions.

   Suggested fix: Detect multiple occurrences and either error with a clear message or warn the user.

Low

5. LOW: TOCTOU race condition (repo_service.ts:401-410)

   There's a time gap between reading the file and writing it. If another process creates the file in between, the write silently overwrites it. Unlikely in practice since this is user-initiated CLI.

6. LOW: Empty file gets extra trailing newline (repo_service.ts:446-450)

   When existing content is empty string, the separator logic adds \n producing <section>\n\n instead of <section>\n. Cosmetic only.

Verdict

FAIL — Issues #1 and #2 are high severity bugs that can cause confusing errors or content duplication during normal upgrade operations. The marker validation issues (#3, #4) are lower priority but should be addressed for robustness.

Review: Approved ✅

This is a well-implemented fix for the CLAUDE.md/AGENTS.md overwrite issue (#628).

Strengths

Domain-Driven Design Compliance:

• RepoService correctly follows the Domain Service pattern with stateless, well-factored methods
• Good separation of concerns: usesSharedInstructionsFile() for classification, ensureInstructionsSection() for orchestration, migrateLegacyInstructions() for conversion
• Exhaustive type handling with assertNever() in the tool switch statement
• Clear ubiquitous language: "managed section", "legacy content", "shared instructions file"

Code Quality:

• No any types
• Named exports
• Uses atomicWriteTextFile for safe file operations
• Proper error handling with UserError for user-facing issues

Test Coverage:

• Comprehensive tests covering all 4 cases (no file, with markers, legacy content, no swamp content)
• Edge case tests: missing END marker recovery, duplicate markers detection, partial legacy match
• Good coverage of tool-specific vs shared file behavior (cursor/kiro still fully overwritten)

Security:

• No injection vulnerabilities in marker detection
• Validates marker order and duplicate markers
• Safe atomic writes

Minor Suggestions (non-blocking)

The replaceManagedSection() refactoring to accept begin/end markers as parameters is clean and avoids code duplication. The 4-case handling in ensureInstructionsSection() is clear and well-documented with comments.

The manual testing items in the test plan should be completed before merging, but the automated tests provide good confidence.

🤖 Reviewed with Claude Code

Adversarial Review

Critical / High

1. Duplicate BEGIN markers when END marker is missing (repo_service.ts:489-526)

   • What's wrong: When a file has the BEGIN marker but is missing the END marker (e.g., user accidentally deleted it), AND the file body contains the legacy signature "This repository is managed with [swamp]", case 3 (legacy migration) matches instead of case 4 (prepend).
   • Breaking example:
     1. Run swamp repo init - creates CLAUDE.md with both markers
     2. Accidentally delete the <!-- END swamp managed section --> line
     3. Run swamp repo upgrade
     4. File now has duplicate BEGIN markers
     5. Next swamp repo upgrade fails with "Found multiple swamp managed sections"
   • Code path: ensureInstructionsSection → case 2 fails (END missing) → case 3 matches (body contains signature) → migrateLegacyInstructions treats orphaned BEGIN as "prefix" and prepends newSection (which includes its own BEGIN marker)
   • Why test passes: The test at lines 1862-1892 uses assertStringIncludes which succeeds even with duplicates. It doesn't verify exactly ONE BEGIN marker exists.
   • Suggested fix: In ensureInstructionsSection, if BEGIN marker exists but END doesn't, check for this corrupted state explicitly and either throw a clear UserError or strip the orphaned BEGIN before prepending.

2. Legacy migration silently deletes user content (repo_service.ts:508-514)

   • What's wrong: When migrating legacy content, if the end marker text "Use \swamp --help` to see available commands.\n"is not found (because user edited it), the fallback at lines 508-514 returnsprefix + newSection + "\n"`, discarding EVERYTHING after the legacy start pattern.
   • Breaking example:

     # Project
     
     This repository is managed with [swamp]...
     
     ## Commands
     
     Use `swamp --help` to see commands.
     
     ## My Team Standards
     
     Always write tests.
     Everything here will be SILENTLY DELETED

     If user changed "available commands." to just "commands.", the entire "## My Team Standards" section is lost.
   • Why test passes: The test at lines 1894-1937 sets up this exact scenario with "## My Custom Section\n\nUser content here.\n" but DOES NOT assert this content is preserved. It only checks for markers and no duplicate headings.
   • Suggested fix: When end marker isn't found, preserve everything after the matched start pattern that doesn't look like part of the swamp template. Alternatively, abort migration with a warning and fall back to prepend-only mode to avoid data loss.

Medium

3. Duplicate END markers not detected (repo_service.ts:743-750)
   • Only duplicate BEGIN markers are detected and rejected. If a bad merge creates BEGIN...content...END...END, the second END marker is left orphaned in the file.
   • Not a functional break but leaves garbage in user's file that could confuse them.
   • Suggested fix: Add a similar check: const secondEnd = content.indexOf(endMarker, endIndex + 1);

Low

4. Test assertions are too weak (repo_service_test.ts)
   • Multiple tests use assertStringIncludes for marker presence without verifying uniqueness. Tests pass with corrupted outputs.
   • Consider adding explicit count assertions like the one at line 1931-1936.

Verdict

FAIL - Two HIGH severity issues found:

1. Missing END marker recovery creates duplicate markers, breaking subsequent upgrades
2. Legacy migration can silently delete user content when template text was modified

Both issues have incomplete test coverage that allows them to pass CI while producing incorrect results.