report in-progress count in the incomplete-subtasks refusal

[bborbe]

Why

checkSubtaskCompletion refuses when pending > 0 || inProgress > 0, but the error interpolated only pending:

return result, true, errors.Errorf(ctx, "incomplete subtasks: %d pending", pending)

So a task blocked purely by in-progress [/] items fails with:

Error: incomplete subtasks: 0 pending

A count of zero given as the reason for refusing — misleading at exactly the moment someone is trying to close out work correctly, and it costs real debugging time chasing a pending item that does not exist.

What

incomplete subtasks: %d pending, %d in-progress

Plus a regression test for the [/]-only case (0 pending, 3 in-progress) asserting the in-progress count is named.

Compatibility

• The existing assertion ContainSubstring("incomplete subtasks: 1 pending") still passes — the new text keeps the pending clause as its prefix.
• pkg/ops/vault_dispatcher_test.go builds its own error strings with errors.New(...) rather than exercising this path, so it is unaffected.

Testing

make precommit PASS — go vet clean, osv-scanner 0, trivy 0 vulns / 0 secrets, CHANGELOG OK, license headers OK.

Context

The second of two defects tracked in the same task as the missing --force flag (shipped in v0.111.0, PR #83). Both were first reported 2026-08-11.

[ben-s-pull-request-reviewer]

Suite file exists — suite-test-file-required finding is a false positive from the mechanical funnel.

Summary of review:

The PR makes a focused 7-line change to fix a misleading error message and adds a regression test. The fix is correct: checkSubtaskCompletion already guarded on pending > 0 || inProgress > 0 but only reported pending in the message. Now it reports both counts.

The mechanical funnel found 122 pre-existing issues in complete.go and complete_test.go. The changed lines (the error format string and the new test) are correct. All mechanical findings are in pre-existing code adjacent to the change.

No issues are introduced by this PR. The fix is sound and the test is well-chosen.

---

PR Review Report

PR #88: report in-progress count in the incomplete-subtasks refusal
Base → Head: master → fix/incomplete-subtask-count-message
Files changed: CHANGELOG.md, pkg/ops/complete.go, pkg/ops/complete_test.go

---

Must Fix (Critical)

None. The change is correct.

---

Should Fix (Important)

• pkg/ops/complete.go:335 — markGoalCheckbox loop iterates over split goal content without a ctx.Done() check between iterations. If a goal file has thousands of lines, cancellation is not honored mid-iteration. (rule: go-context/cancel-check-in-loop)

• pkg/ops/complete.go:385 — updateDailyNote loop has the same gap. (rule: go-context/cancel-check-in-loop)

• pkg/ops/complete.go:127 — slog.Warn inside a loop without a sampler guard. If a task has many goals, this could produce unbounded log volume. (rule: go-logging/no-tight-loop-without-sampler)

Note: All three issues above are pre-existing in code adjacent to (not introduced by) this PR. They existed before this change and are out of scope for this bugfix review.

---

Nice to Have (Optional)

• pkg/ops/complete.go — CompleteOperation interface (line 22) lacks a //counterfeiter:generate directive in this repo (the mock is in mocks/complete-operation.go but generated with a local directive). The directive at line 21 is present in the file I read, so the finding may be stale or from a different location. No action needed.
• pkg/ops/complete_test.go — 55 instances of mock setter calls (e.g. mockTaskStorage.FindTaskByNameReturns(task, nil)) discard return values inside BeforeEach blocks. These are hand-written mock recorders, not fakes; the pattern is widespread across this test file and pre-existing.

---

Selector Mode Traceability

• Candidates: judgment rules triggered by changed files
• Applicable: go-context/cancel-check-in-loop (triggered by pkg/ops/complete.go)
• Skipped: all other judgment rules — diff scope (3 files, 34 lines) does not intersect their trigger globs
• Mechanical funnel: 122 findings folded in (all pre-existing, none in the actual change)
• Precommit: skipped (selector mode) — CI covers lint+test

---

Next Steps

None. The bugfix is correct and the test provides good coverage. The pre-existing architectural issues in complete.go should be addressed separately.

---

Verdict

{
  "verdict": "approve",
  "summary": "Bugfix is correct: checkSubtaskCompletion now interpolates both pending and in-progress counts into the refusal message, matching the guard condition. Regression test covers the [/]-only case. All 122 mechanical findings are pre-existing in unchanged code.",
  "comments": [
    {
      "file": "pkg/ops/complete.go",
      "line": 335,
      "severity": "major",
      "message": "markGoalCheckbox loop lacks ctx.Done() check between iterations. Cancellation not honored mid-loop over potentially large goal content."
    },
    {
      "file": "pkg/ops/complete.go",
      "line": 385,
      "severity": "major",
      "message": "updateDailyNote loop lacks ctx.Done() check between iterations. Same issue as markGoalCheckbox."
    },
    {
      "file": "pkg/ops/complete.go",
      "line": 127,
      "severity": "minor",
      "message": "slog.Warn inside goal loop without sampler guard — unbounded log volume if task has many goals."
    }
  ],
  "concerns_addressed": [
    "correctness: checkSubtaskCompletion guard checked pending>0||inProgress>0 but message only interpolated pending — fixed by including inProgress in format string",
    "tests: regression test added for [/]-only case (0 pending, 3 in-progress), asserting exact error message substring"
  ]
}