fix: accept in-progress checkbox in goal roll-up

[bborbe]

task complete marks the task's checkbox on its parent goal. The matcher only accepted - [ ]:

if strings.Contains(line, "- [ ]") && strings.Contains(strings.ToLower(line), strings.ToLower(taskName))

A goal entry written as - [/] — the in-progress marker this tool itself emits from task-status grouped-checkbox output — never matched. Completion then failed the roll-up with:

failed to update goal [[<Goal>]]: checkbox not found for task <Task> in goal [[<Goal>]]

The task still completed, so nothing looked broken until someone read the goal and found it claiming less progress than reality.

The sharp edge: a task being completed has usually been worked, and a worked task is marked [/] on its goal. So the roll-up failed on exactly the tasks most likely to be completed. Hit twice on 2026-08-21 completing two migration tasks against one goal; both goal lines had to be hand-edited.

Change

• goalCheckboxMarker returns whichever unchecked marker a line carries (- [ ] or - [/]), empty when neither
• markGoalCheckbox replaces the marker it matched, so both roll up to - [x]
• Test covers a goal whose entry is - [/]

Verification

make precommit — PASS (17.8s), tests included.

Known adjacent gap, deliberately not fixed here: a goal using a non-checkbox list (plain numbered entries) still fails, and a task never registered on its goal still fails. Both are documented separately.

[ben-s-pull-request-reviewer]

The diff is small and I've read the relevant files. Here is the full adjudication:

Diff summary: 3 files — CHANGELOG.md, pkg/ops/complete.go, pkg/ops/complete_test.go

Scope: Bugfix — markGoalCheckbox only matched - [ ] checkboxes, skipping - [/] (in-progress) entries on goal files. Fixed by introducing a goalCheckboxMarker helper that accepts both markers.

---

Must Fix (Critical)

• CHANGELOG.md:11 — Version alignment breach. CHANGELOG.md has an ## Unreleased section, which signals a pending release. Per CLAUDE.md, all four version strings must equal: CHANGELOG.md (top section header), .claude-plugin/plugin.json ("version"), and both places in .claude-plugin/marketplace.json (metadata.version and plugins[0].version). Currently all three plugin files are 0.114.3 while CHANGELOG is in Unreleased state. Either remove ## Unreleased (not a real release yet) or bump all four files together. This blocks make precommit.

Should Fix (Important)

• pkg/ops/complete.go:325 — goalCheckboxMarker is an exported-but-package-local helper with no direct test. The mechanical funnel confirmed it is called only from markGoalCheckbox, which is exercised by the new Context("task with an in-progress goal checkbox" test — but that test covers only the caller's happy path. Edge cases (empty string, line with neither marker) are not verified. Either add unit tests for the helper directly or expand the integration test to cover the marker-not-found branch. The rule go-testing/no-bare-error-call flagged numerous mock setter calls in the test file, but all of those setter methods (FindTaskByNameReturns, WriteTaskReturns, etc.) are void-returning counterfeiter-generated methods — the findings are false positives from the rule's signature check; no action needed.

Nice to Have (Optional)

None.

---

Selector-mode traceability

• Candidates: rules matched by Step 4b-i glob filter — limited to this PR's changed files
• Applicable: go-testing/no-bare-error-call, go-testing/suite-test-file-required, goalCheckboxMarker test coverage
• Skipped: go-composition/no-package-function-calls-in-business-logic → ops structs are the composition layer itself, calling domain setters is intentional (not a hidden dependency); go-context/cancel-check-in-loop → SHOULD-tier, loops are bounded by line count (≤~20 iterations); go-logging/no-tight-loop-without-sampler → SHOULD-tier, bounded loop with guarded log call

---

{
  "verdict": "request-changes",
  "summary": "Bugfix is correct and well-tested for the primary case, but the CHANGELOG has entered '## Unreleased' state without bumping the plugin version strings — a `make precommit` failure. The new `goalCheckboxMarker` helper also lacks direct unit tests.",
  "comments": [
    {
      "file": "CHANGELOG.md",
      "line": 11,
      "severity": "critical",
      "message": "Version alignment breach: CHANGELOG.md is in '## Unreleased' state but plugin manifests are at v0.114.3. Per CLAUDE.md, all four version strings must match. Either remove ## Unreleased or bump plugin.json + marketplace.json to a new version."
    },
    {
      "file": "pkg/ops/complete.go",
      "line": 325,
      "severity": "major",
      "message": "goalCheckboxMarker helper has no direct unit tests. It is exercised indirectly by the new 'task with an in-progress goal checkbox' integration test, but edge cases (empty input, no marker present) are not verified."
    }
  ],
  "concerns_addressed": [
    "correctness: goalCheckboxMarker handles both - [ ] and - [/] correctly — verified by new integration test",
    "version: CHANGELOG enters Unreleased but plugin files stay at 0.114.3 — not resolved, must fix before merge",
    "testing: goalCheckboxMarker edge cases not directly tested — should add tests"
  ]
}

[bborbe]

Addressed the MAJOR; disputing the CRITICAL — I believe it is inverted for this repo.

CRITICAL — version alignment

The finding asks me to either drop ## Unreleased or hand-bump the manifests. This repo's CLAUDE.md:111 says explicitly not to:

This repo is autoRelease: true (.maintainer.yaml). The github-releaser owns version bumps + tags. For a normal change, add a ## Unreleased bullet and let the releaser convert it → ## vX.Y.Z, bump all four version strings, and tag post-merge — do NOT hand-bump the manifests or git tag (that races the releaser).

The alignment rule it cites (CLAUDE.md:98) defines target #1 as "the top ## vX.Y.Z entry (the most-recent versioned section)". ## Unreleased is not a versioned section, so the four strings are aligned right now at 0.114.3.

The finding also states this is a make precommit failure. It is not — verified on this exact tree, with ## Unreleased present and manifests at 0.114.3:

make precommit  →  PASS (19.5s)
go mod verify, goimports-reviser, golines, gofmt, go test (all packages),
golangci-lint (0 issues), go vet, osv-scanner, trivy fs (0 vulns),
CHANGELOG structure, license headers
No check-versions issues detected.

Complying would hand-bump manifests pre-merge and race the releaser — the exact failure that CLAUDE.md section exists to prevent. Leaving as-is; happy to be shown wrong if check-versions has a mode I have not hit.

MAJOR — goalCheckboxMarker edge cases

Fair, and covered in 13850e3. Two cases, both named in the finding:

• goal entry written as a plain numbered list → no marker present → no goal rewrite
• goal entry already - [x] → no unchecked marker → no goal rewrite

Tested through the public operation rather than as direct unit tests on the unexported helper: pkg/ops has zero internal (package ops) test files, so adding one would break the package's convention. go test ./pkg/ops/... → PASS.

superseded by new automated review

[ben-s-pull-request-reviewer]

Now I have enough context. The counterfeiter directive is present (line 21) and the suite file exists — both are false positives from the mechanical funnel. Let me compile the findings.

Mechanical findings confirmed:

• go-testing/no-bare-error-call on complete_test.go lines 42, 58, 59 — the mock Returns calls discard error returns. These are pre-existing in the unchanged part of the test, not introduced by this PR.

Code review of the diff:

The change is a targeted bugfix. Three new things:

1. goalCheckboxMarkers slice and goalCheckboxMarker() helper
2. Updated markGoalCheckbox to use the helper (accepting - [/] in addition to - [ ])
3. Three new test cases covering the fixed behavior

Assessing the plan concerns:

• Sequential marker iteration: goalCheckboxMarkers = ["- [ ]", "- [/]"] — the closed set means there's no ambiguity; each checkbox line can have only one state
• Single replacement with strings.Replace(..., 1) — correct
• marker == "" guard before the name check — intentional early-exit optimization, order is correct
• break after first match — correct, only one checkbox per line

Correctness verdict: The bugfix is sound. No issues in the changed logic.

{
  "verdict": "approve",
  "summary": "Bugfix correctly extends goal checkbox matching from only `- [ ]` to both `- [ ]` and `- [/]` markers. The new helper and three test cases are well-structured. Pre-existing bare-error-call violations in the unchanged test setup are noted but outside the scope of this PR.",
  "comments": [
    {
      "file": "pkg/ops/complete_test.go",
      "line": 42,
      "severity": "minor",
      "message": "currentDateTime.SetNow(...) discards its return value. If SetNow returns (error), wrap in Gomega: Expect(currentDateTime.SetNow(...)).To(Succeed()). This is pre-existing in the unchanged BeforeEach setup, not introduced by this PR."
    },
    {
      "file": "pkg/ops/complete_test.go",
      "line": 58,
      "severity": "minor",
      "message": "mockTaskStorage.FindTaskByNameReturns(task, nil) discards its return value. If the mock's Returns method returns (error), wrap in Gomega. Pre-existing, not introduced by this PR."
    },
    {
      "file": "pkg/ops/complete_test.go",
      "line": 59,
      "severity": "minor",
      "message": "mockTaskStorage.WriteTaskReturns(nil) discards its return value. Pre-existing, not introduced by this PR."
    }
  ],
  "concerns_addressed": [
    "correctness: goalCheckboxMarker iterates sequentially — addressed: the marker set is closed (only 2 entries), each checkbox line has exactly one state, so ordering is unambiguous",
    "correctness: strings.Replace with 1 as n — addressed: correct single replacement scoped to one occurrence per line",
    "correctness: marker==\"\" guard before task name check — addressed: intentional early-exit ordering, confirmed correct",
    "go-testing/counterfeiter-directive-on-interface — not an issue: directive is present at line 21 of complete.go",
    "go-testing/suite-test-file-required — not an issue: pkg/ops/ops_suite_test.go exists"
  ]
}