fix(task/controller): filename-collision idempotency via gitClient.ReadFile + ErrTaskAlreadyExists sentinel

[bborbe]

Summary

• Filename-based collision check via gitClient.ReadFile (not os.Stat / os.ReadFile) — fixes silent overwrites of vault task files on every recurring-task-creator tick / /trigger replay
• New task.ErrTaskAlreadyExists sentinel in lib/command/task → CQRS Failure on result topic for cross-repo classification by recurring-task-creator (separate spec)
• Drops the UUID-fallback-on-collision + frontmatter-re-parse branches — collision = filename owns the slot, period

Spec

specs/in-progress/045-bug-task-controller-filename-collision-idempotency.md (kind: bug)

Repro (prod, 2026-06-20)

POST /admin/recurring-task-creator/trigger?date=2026-06-20 → 12 git-rest: update commits on Personal vault, each stripping claude_session_id + phase from existing W25-sat task files. Commit 7279ef31d is one such diff.

Root cause: git_client_adapter.go:64 documents that Path() returns a logical path that does NOT exist on disk, but task_create_task_executor.go used os.Stat / os.ReadFile against it. Existence check always failed → executor always wrote.

Test plan

• cd lib && make precommit — passes
• cd task/controller && make precommit — passes (new Ginkgo contexts cover AC2/AC3/AC4/AC5)
• Post-merge: make buca to dev, then POST .../trigger?date=$(date -u +%F) against dev → zero git-rest: update commits in Personal vault
• Same against prod after promotion

[ben-s-pull-request-reviewer]

Based on my thorough review of the changed files, here is my consolidated report:

---

PR Review: fix(task/controller): filename-collision idempotency via gitClient.ReadFile + ErrTaskAlreadyExists sentinel

Diff scope

• CHANGELOG.md
• lib/command/task/errors.go + errors_test.go
• task/controller/pkg/command/task_create_task_executor.go + _test.go
• 2 prompt metadata files (non-code)

---

Must Fix (Critical)

None.

Should Fix (Important)

1. task_create_task_executor.go:134 — double title validation on every write

resolveCreateTaskRelPath calls cmd.Validate(ctx) as a second validation pass (defense-in-depth), but the command was already validated at the top of HandleCommand before routing. This re-runs validation logic including frontmatter checks on every write, not just title. If CreateCommand.Validate ever grows more expensive fields, this adds latency to every write. The resolver-level re-validation was in the original code — consider whether it's actually needed now that the command is already checked at entry, or if a narrower title-only check would suffice.

2. task_create_task_executor.go:143 — strings.ContainsAny rejects valid Linux/macOS filenames

strings.ContainsAny(cmd.Title, "/\\") blocks forward slashes even on Unix where / is legal in filenames. The original code had this same guard, so this is pre-existing — but it means titles like "Q1/Report" are silentlyUUID-fallbacked even on Linux. If the vault scanner (which runs on the same host as the controller) enforces a tighter filename policy, this is moot. If not, the separator check could be platform-aware or document that titles must be portable across OSes.

Nice to Have (Optional)

3. resolveCreateTaskRelPath docstring omits the path-separator defense

The docstring says "If cmd.Title passes validation... the title-derived path is returned" but doesn't mention the strings.ContainsAny guard that can also trigger UUID fallback. A reader of the docstring would not know about this second fallback path. Not a bug, just incomplete docs.

4. gitClient.Path() in executor closure — no nil check

gitClient is injected as a concrete *gitRestGitClientAdapter (via factory), which always returns a.basePath (never nil). Safe in practice, but if the interface ever gained a different implementation this could panic. Low risk given the factory wiring.

---

Verdict Summary

Approve — this is a well-targeted bug fix. The core change (switching collision detection from local os.Stat to git-rest ReadFile) is correct and necessary for the git-rest adapter. The ErrTaskAlreadyExists sentinel is properly declared with stderrors.New, documented, and tested for errors.Is survivability after bborbe/errors.Wrapf wrapping. AC2–AC5 are all covered with appropriate mocks. The behavioral shift (collision now returns an error/Failure rather than silent success) is an improvement — the old silent-success-on-collision meant replayed commands gave no feedback to operators.

The two Should Fix items are pre-existing patterns, not regressions introduced by this PR.

{
  "verdict": "approve",
  "summary": "Well-targeted bugfix: replaces broken local-filesystem collision checks (os.Stat always returns nil on the git-rest adapter) with git-rest ReadFile round-trips, and surfaces filename collisions via a proper ErrTaskAlreadyExists sentinel wrapped with bborbe/errors so callers can use errors.Is. AC2–AC5 fully tested; ErrTaskAlreadyExists correctly declared with stdlib errors and survivable after wrapping. Two pre-existing patterns flagged as Should Fix (duplicate cmd.Validate call in resolver, platform-unsafe path-separator guard), but neither is a regression.",
  "comments": [
    {
      "file": "task/controller/pkg/command/task_create_task_executor.go",
      "line": 134,
      "severity": "major",
      "message": "resolveCreateTaskRelPath re-runs cmd.Validate(ctx) on every write (defense-in-depth). The command was already validated at HandleCommand entry — this double-pass adds overhead and frontmatter re-checks on the hot write path. Consider narrowing to title-only re-validation, or removing if upstream validation is sufficient."
    },
    {
      "file": "task/controller/pkg/command/task_create_task_executor.go",
      "line": 143,
      "severity": "major",
      "message": "strings.ContainsAny(cmd.Title, \"/\\\\\") silently UUID-fallbacks titles with forward slashes even on Unix/macOS where '/' is a legal filename character. Pre-existing (inherited from original code), but means titles like 'Q1/Report' work on Windows vault scanners but silently break on Linux controllers."
    },
    {
      "file": "task/controller/pkg/command/task_create_task_executor.go",
      "line": 120,
      "severity": "nit",
      "message": "resolveCreateTaskRelPath docstring does not document the strings.ContainsAny path-separator guard as a second fallback trigger. A reader of the docstring would assume title-validation is the only path to UUID fallback."
    }
  ],
  "concerns_addressed": [
    "correctness: gitClient.ReadFile handles the same error cases as the old approach — isNotFoundReadError correctly classifies 'returned 404' as not-found, all other errors propagate (AC5). Confirmed.",
    "correctness: ErrTaskAlreadyExists callers check errors.Is() — sentinel is stdlib stderrors.New, wrapped with bborbe/errors.Wrapf which preserves errors.Is matchability. Confirmed by errors_test.go.",
    "correctness: UUID-fallback-on-collision removed; filename owns the slot regardless of task_identifier. AC4 test verifies this. Confirmed.",
    "tests: AC2/AC3/AC4/AC5 all covered with ReadFile mock driving. BeforeEach default (ReadFile returns 404) ensures non-collision tests are unaffected. Confirmed."
  ]
}