Guard logic duplicated 17 times across 9 tool handlers — extract to shared module

[igouss]

Summary

The closed-status check status === "complete" || status === "done" is duplicated 19 times across 10 files. Validation helpers isNonEmptyString() and validateStringArray() are duplicated across 5 and 3 files respectively. No shared utility module exists.

Investigation Results

Closed-Status Guard: 19 occurrences across 10 files

Two forms:

• Positive: status === "complete" || status === "done" — 10 occurrences
• Negative: status !== "complete" && status !== "done" — 9 occurrences (filters + "not yet complete" guards)

File	Line(s)	Guards	Inside Transaction?
complete-task.ts	160, 166, 172	milestone closed, slice closed, task already done	YES
complete-slice.ts	228, 234, 246	milestone closed, slice already done, incomplete tasks filter	YES
complete-milestone.ts	137, 149, 159	milestone already done, incomplete slices filter, incomplete tasks filter	YES
reopen-task.ts	66, 76, 86	milestone closed, slice closed, task not complete	YES
reopen-slice.ts	65, 75	milestone closed, slice not complete	YES
plan-task.ts	84, 89	slice closed, task already done	NO
plan-slice.ts	153, 161	milestone closed, slice already done	NO
plan-milestone.ts	194, 205	milestone already done, dependency not complete	NO
reassess-roadmap.ts	112, 121, 129	milestone closed, slice not complete, completed slice filter	NO
replan-slice.ts	98, 107, 115	slice closed, blocker not complete, completed task filter	NO

Subtle Inconsistencies (Drift)

1. Null-check wrapping differs:

// complete-task.ts:160 — with null guard
if (milestone && (milestone.status === "complete" || milestone.status === "done"))

// reopen-task.ts:66 — without null guard  
if (milestone.status === "complete" || milestone.status === "done")

2. Error message wording inconsistent:

• "cannot reopen task in a closed milestone" vs "cannot reopen task inside a closed slice" — "in" vs "inside"
• Complete handlers: "is already complete — use gsd_task_reopen first if you need to redo it"
• Plan handlers: "cannot re-plan task: it is already complete — use gsd_task_reopen first" — different phrasing, same guard

3. Transaction placement split:

• complete-* and reopen-* handlers: guards INSIDE transaction (TOCTOU-safe)
• plan-*, reassess-*, replan-* handlers: guards OUTSIDE transaction (TOCTOU-exposed, see TOCTOU: plan-task, plan-slice, reassess-roadmap, replan-slice guards outside transaction #11)

isNonEmptyString() — identical in 5 files

All implementations are byte-for-byte identical:

function isNonEmptyString(value: unknown): value is string {
  return typeof value === "string" && value.trim().length > 0;
}

File	Lines
plan-task.ts	35-37
plan-slice.ts	54-56
plan-milestone.ts	58-60
reassess-roadmap.ts	50-52
replan-slice.ts	51-53

validateStringArray() — identical in 3 files

function validateStringArray(value: unknown, field: string): string[] {
  if (!Array.isArray(value)) {
    throw new Error(`${field} must be an array`);
  }
  if (value.some((item) => !isNonEmptyString(item))) {
    throw new Error(`${field} must contain only non-empty strings`);
  }
  return value;
}

File	Lines
plan-task.ts	39-47
plan-slice.ts	58-66
plan-milestone.ts	62-70

No shared utility module exists

Searched the entire src/resources/extensions/gsd/ tree. No validators.ts, guards.ts, utils.ts, or any exported isNonEmptyString/validateStringArray. Zero shared modules for this purpose.

Status Values in Use

Value	Meaning	Used in guards?
"complete"	Finished	Yes — primary closed check
"done"	Legacy finished	Yes — secondary closed check
"pending"	New/default	No — set in INSERTs only
"active"	Milestone running	No
"in_progress"	Slice running	No — set in reopen-slice only

Proposed Fix

New file: src/resources/extensions/gsd/guard-checks.ts

export function isClosedStatus(status: string): boolean {
  return status === "complete" || status === "done";
}

export function guardMilestoneOpen(
  milestoneId: string, status: string, action: string
): string | null {
  return isClosedStatus(status)
    ? `cannot ${action} in a closed milestone: ${milestoneId} (status: ${status})`
    : null;
}

export function guardSliceOpen(
  sliceId: string, status: string, action: string
): string | null {
  return isClosedStatus(status)
    ? `cannot ${action} in a closed slice: ${sliceId} (status: ${status})`
    : null;
}

export function guardTaskNotComplete(
  taskId: string, status: string, action: string
): string | null {
  return isClosedStatus(status)
    ? `cannot ${action}: task ${taskId} is already complete`
    : null;
}

export function guardTaskComplete(
  taskId: string, status: string, context: string
): string | null {
  return !isClosedStatus(status)
    ? `${context}: task ${taskId} is not complete (status: ${status})`
    : null;
}

New file: src/resources/extensions/gsd/validation.ts

export function isNonEmptyString(value: unknown): value is string {
  return typeof value === "string" && value.trim().length > 0;
}

export function validateStringArray(value: unknown, field: string): string[] {
  if (!Array.isArray(value)) {
    throw new Error(`${field} must be an array`);
  }
  if (value.some((item) => !isNonEmptyString(item))) {
    throw new Error(`${field} must contain only non-empty strings`);
  }
  return value;
}

Then replace all 19 inline status checks and all 8 duplicated validation functions with imports from these modules. Error messages get standardized as a side effect.

Related Issues

• TOCTOU: plan-task, plan-slice, reassess-roadmap, replan-slice guards outside transaction #11 — TOCTOU: the plan-*/reassess-*/replan-* guards that are outside transactions should be moved inside as part of the same cleanup