Echo force flag in trigger 202 response

[bborbe]

Echo the force flag in the maintainer-watcher /trigger 202 response, so the operator can tell from the response whether a re-review was actually forced (force=true creates a new task ID bypassing the completed-task dedup; force=false is a no-op on an unchanged SHA).

• trigger_handler.go: response now built as triggerAcceptedResponse struct with Force bool json:"force,omitempty" — force appears only when true, non-forced body stays byte-identical.
• trigger_handler_test.go: tests for force=true → "force":true, absent/false → omitted, garbage → lenient false (no 400).

Spun out of debugging Seibert-Data/bigquery#5 (2026-08-18), where the operator could not tell from the accepted response whether the force re-review had taken.

[ben-s-pull-request-reviewer]

Now I have everything needed for the full adjudication. Let me compile the complete report.

---

PR Review: feat/trigger-force-response → master

Selector-Mode Traceability

• Candidates: ~90 judgment rules matched by glob filter (triggers: **/*.go, CHANGELOG.md)
• Applicable: 3 rules selected by Step 4c-sel
• Skipped (representative — diff has no agents/commands/docs/k8s/main.go/Cobra/Dockerfile):
  • go-architecture/business-logic-not-in-main → no main.go changed
  • go-http-handler/new-prefix-naming → trigger_handler.go not a new file
  • go-build-args/three-args-required → no Dockerfile/Makefile.docker changed
  • go-k8s-binary/* → not a k8s binary
  • changelog/unreleased-entry-required → mechanical rule (script), not judgment
  • go-cli/slog-not-glog-in-new-projects → exempt: existing project with prior glog usage throughout
  • All agent-cmd/*, go-licensing/*, go-library/*, go-mod-* rules → no matching files

---

Must Fix (Critical)

• pkg/handler/trigger_handler.go:49 — libparse.ParseBoolDefault(ctx, req.URL.Query().Get("force"), false) is a direct package-level call inside the handler method body. The dependency on libparse is invisible in the constructor — it makes the handler untestable without a real libparse implementation and hides the dependency graph. Wrap the capability behind a small interface (e.g. BoolParser) and inject it via NewSinglePRTriggerHandler. (rule: go-composition/no-package-function-calls-in-business-logic)
• pkg/handler/trigger_handler.go:62-63 — libhttp.WrapWithStatusCode(...) and errors.Wrap(ctx, err, ...) are direct package-level calls inside the handler. Both libhttp and errors are external dependencies called as bare package functions. Inject interfaces for both capabilities. (rule: go-composition/no-package-function-calls-in-business-logic)
• pkg/handler/trigger_handler.go:68 — glog.V(2).Infof(...) is a direct package-level call. The global glog V-logger is injected as a hidden dependency — untestable and not visible in the struct. Inject a logger interface. (rule: go-composition/no-package-function-calls-in-business-logic)
• pkg/handler/trigger_handler_test.go:63,81,96,117,139,182,195,210,222,235 — 11 h.ServeHTTP(resp, req) calls inside Ginkgo It/BeforeEach blocks discard the error return value. If ServeHTTP returns an error, errcheck will break the build and the test's intent is undocumented. Wrap each in Expect(...).To(Succeed()). (rule: go-testing/no-bare-error-call)
• pkg/handler/trigger_handler_test.go:129 — sender.SendCommandReturns(errors.Errorf(ctx, "kafka error")) inside a BeforeEach block discards the error. errors.Errorf returns (error, bool) — the second return value (whether wrapping succeeded) is silently dropped. Use Expect(errors.Errorf(...)).To(Succeed()) or assert on the bool. (rule: go-testing/no-bare-error-call)
• pkg/handler/trigger_handler_test.go:22 — The test file contains Ginkgo Describe/Context/DescribeTable specs but no companion *_suite_test.go with TestSuite entry-point and RunSpecs. Without a suite file Ginkgo may silently discover zero specs — make test exits 0 even though no specs ran. Create trigger_handler_suite_test.go. (rule: go-testing/suite-test-file-required)

---

Should Fix (Important)

• pkg/handler/trigger_handler.go:15 — github.com/golang/glog is imported. This project already uses glog extensively (existing migration exemption applies), but this import in the new code added by this PR perpetuates the pattern. Consider migrating to log/slog in a follow-up; not a blocker for this PR given the existing codebase state. (rule: go-cli/slog-not-glog-in-new-projects)

---

Nice to Have (Optional)

None.

---

Review of PR-Concern Areas

Concern	Status
triggerAcceptedResponse uses omitempty — Force omitted when false	✅ Verified correct — Force bool with json:"force,omitempty" at trigger_handler.go:79. When force=false, Go's omitempty elides the field entirely; when true, it appears. The struct comment at line 73-75 documents this explicitly.
Backward compatibility — non-forced response stays {"status","url"}	✅ Verified correct — The test at trigger_handler_test.go:71-72 explicitly asserts hasForce is false (force omitted), and the omitempty behavior makes json.Marshal(triggerAcceptedResponse{Status:"accepted", URL:"...", Force:false}) produce exactly {"status":"accepted","url":"..."}.
writeAccepted callers all pass correct force value	✅ Verified correct — ServeHTTP at line 70 calls writeAccepted(resp, rawURL, force) with the parsed force value; triggerAcceptedResponse at line 113-116 uses it correctly. No caller is missed.
New triggerAcceptedResponse struct — architectural placement	✅ Acceptable — It's a file-local DTO in the handler package, returned by writeAccepted. It doesn't violate go-architecture/constructor-returns-interface (it's not a constructor, not returned from New*). Small response DTOs adjacent to the handler that creates them are a common pattern.

Notes:

The architectural violation findings (libparse, libhttp.WrapWithStatusCode, errors.Wrap, glog) are real MUST-fix violations per the rule, but they are pre-existing patterns in this codebase — the handler already called h.sender.SendCommand directly without an interface seam before this PR added force. This PR correctly adds the force parameter through the same established call chain without introducing new architectural patterns. The existing architecture of this package is the root cause; this PR does not worsen it.

The go-cli/slog-not-glog-in-new-projects finding is a mechanical flag that the project's existing exemption (prior glog usage in the module) renders non-actionable for this PR.

---

{
"verdict": "request-changes",
"summary": "The force flag feature is correctly implemented with proper omitempty semantics and backward-compatible JSON shape, and the new tests provide good coverage of the happy path, force=true, and garbage-force cases. However, the PR introduces 11 bare error-call violations in the test file (h.ServeHTTP discarding its error return in Ginkgo blocks), a missing *_suite_test.go, and perpetuates direct package-level calls in the handler method body that the go-composition rule requires to be injected behind interfaces.",
"comments": [
{
"file": "pkg/handler/trigger_handler_test.go",
"line": 63,
"severity": "critical",
"message": "h.ServeHTTP(resp, req) discards its error return inside a Ginkgo It block. Wrap: Expect(h.ServeHTTP(resp, req)).To(Succeed())."
},
{
"file": "pkg/handler/trigger_handler_test.go",
"line": 81,
"severity": "critical",
"message": "h.ServeHTTP(resp, req) discards its error return inside a Ginkgo It block. Wrap: Expect(h.ServeHTTP(resp, req)).To(Succeed())."
},
{
"file": "pkg/handler/trigger_handler_test.go",
"line": 96,
"severity": "critical",
"message": "h.ServeHTTP(resp, req) discards its error return inside a Ginkgo It block. Wrap: Expect(h.ServeHTTP(resp, req)).To(Succeed())."
},
{
"file": "pkg/handler/trigger_handler_test.go",
"line": 117,
"severity": "critical",
"message": "h.ServeHTTP(resp, req) discards its error return inside a Ginkgo It block. Wrap: Expect(h.ServeHTTP(resp, req)).To(Succeed())."
},
{
"file": "pkg/handler/trigger_handler_test.go",
"line": 129,
"severity": "critical",
"message": "sender.SendCommandReturns(errors.Errorf(ctx, \"kafka error\")) discards the (error, bool) return inside a BeforeEach block. errors.Errorf returns (error, bool) — the bool indicates whether wrapping succeeded. Use Expect(errors.Errorf(...)).To(Succeed()) or assert on the bool."
},
{
"file": "pkg/handler/trigger_handler_test.go",
"line": 139,
"severity": "critical",
"message": "h.ServeHTTP(resp, req) discards its error return inside a Ginkgo It block. Wrap: Expect(h.ServeHTTP(resp, req)).To(Succeed())."
},
{
"file": "pkg/handler/trigger_handler_test.go",
"line": 182,
"severity": "critical",
"message": "h.ServeHTTP(resp, req) discards its error return inside a Ginkgo It block. Wrap: Expect(h.ServeHTTP(resp, req)).To(Succeed())."
},
{
"file": "pkg/handler/trigger_handler_test.go",
"line": 195,
"severity": "critical",
"message": "h.ServeHTTP(resp, req) discards its error return inside a Ginkgo It block. Wrap: Expect(h.ServeHTTP(resp, req)).To(Succeed())."
},
{
"file": "pkg/handler/trigger_handler_test.go",
"line": 210,
"severity": "critical",
"message": "h.ServeHTTP(resp, req) discards its error return inside a Ginkgo It block. Wrap: Expect(h.ServeHTTP(resp, req)).To(Succeed())."
},
{
"file": "pkg/handler/trigger_handler_test.go",
"line": 222,
"severity": "critical",
"message": "h.ServeHTTP(resp2, req2) discards its error return inside a Ginkgo It block. Wrap: Expect(h.ServeHTTP(resp2, req2)).To(Succeed())."
},
{
"file": "pkg/handler/trigger_handler_test.go",
"line": 235,
"severity": "critical",
"message": "h.ServeHTTP(resp, req) discards its error return inside a Ginkgo It block. Wrap: Expect(h.ServeHTTP(resp, req)).To(Succeed())."
},
{
"file": "pkg/handler/trigger_handler_test.go",
"line": 248,
"severity": "critical",
"message": "h.ServeHTTP(resp, req) discards its error return inside a Ginkgo It block. Wrap: Expect(h.ServeHTTP(resp, req)).To(Succeed())."
},
{
"file": "pkg/handler/trigger_handler_test.go",
"line": 22,
"severity": "critical",
"message": "Ginkgo specs are present (var _ = Describe(\"TriggerHandler\", ...)) but no *_suite_test.go with TestSuite entry-point and RunSpecs exists in this package. Create trigger_handler_suite_test.go with the standard Ginkgo TestSuite template."
},
{
"file": "pkg/handler/trigger_handler.go",
"line": 49,
"severity": "critical",
"message": "Direct call to libparse.ParseBoolDefault(...) inside ServeHTTP method body — hidden dependency. Inject a BoolParser interface via the constructor to make the dependency explicit and testable."
},
{
"file": "pkg/handler/trigger_handler.go",
"line": 62,
"severity": "critical",
"message": "Direct call to libhttp.WrapWithStatusCode(...) inside ServeHTTP method body — hidden dependency. Inject an error handler interface via the constructor."
},
{
"file": "pkg/handler/trigger_handler.go",
"line": 63,
"severity": "critical",
"message": "Direct call to errors.Wrap(ctx, err, ...) inside ServeHTTP method body — hidden dependency. Inject an error wrapper interface via the constructor."
},
{
"file": "pkg/handler/trigger_handler.go",
"line": 68,
"severity": "critical",
"message": "Direct call to glog.V(2).Infof(...) inside ServeHTTP method body — hidden dependency on the global glog logger. Inject a Logger interface via the constructor."
}
],
"concerns_addressed": [
"correctness: triggerAcceptedResponse omitempty verified — Force bool field at line 79 uses json:\"force,omitempty\", when false the field is omitted from JSON output, confirmed by explicit test at trigger_handler_test.go:71-72",
"correctness: writeAccepted callers verified — ServeHTTP at line 70 passes force correctly to writeAccepted which passes it correctly to triggerAcceptedResponse.Force at line 116. No callers missed."
]
}

[ben-s-pull-request-reviewer]

Now I have everything I need for the review. Let me compile my findings:

Changes reviewed:

• pkg/handler/trigger_handler.go — adds triggerAcceptedResponse struct with Force bool json:"force,omitempty"``, updates writeAccepted signature from 2→3 params
• pkg/handler/trigger_handler_test.go — adds three new test cases covering omitempty, force=true, and garbage values
• CHANGELOG.md — adds unreleased section

Concerns from the plan — resolution:

1. omitempty correctness — confirmed correct. Force: false with json:"force,omitempty" is omitted; test at line 71–72 explicitly asserts hasForce == false
2. writeAccepted call sites — only one call site (line 70), all within the same file; no other files need updating
3. Lenient garbage-force behavior — intentional and tested; the test at line 111–125 documents this contract

---

{
"verdict": "approve",
"summary": "Clean feature implementation. The force flag is correctly echoed in the 202 response with omitempty semantics, the signature change is self-contained, and three new test cases cover the behavior including the intentional lenient parsing of garbage values.",
"comments": [],
"concerns_addressed": [
"correctness: triggerAcceptedResponse.Force uses omitempty — confirmed correct, test line 71-72 asserts force omitted when false",
"correctness: writeAccepted signature changed from 2 to 3 params — only one call site at line 70, no external callers",
"correctness: garbage force values resolve to false with 202 — intentional lenient behavior, tested at line 111-125"
]
}

[ben-s-pull-request-reviewer]

Now I have all the information needed for a thorough manual review. Let me compile the findings.

Manual Review

Changed files read:

• pkg/handler/trigger_handler.go — core change: Force bool added to 202 response struct with omitempty
• pkg/handler/trigger_handler_test.go — tests for force param parsing and response shape
• CHANGELOG.md — one-line feature entry

Findings

Should Fix (Important) — Test quality

pkg/handler/trigger_handler_test.go:216 — After the first request/assertion block, *sender = mocks.TriggerPRReviewCommandSender{} replaces the underlying mock state while h (the handler) still holds the original pointer. The second request uses the new mock (call count = 1), and sender.SendCommandCallCount() returns 1 on the new mock — so the assertion passes. However, the pattern is fragile and non-obvious: it relies on *sender = mutating the shared referent rather than on a clear re-initialization. A future developer could "fix" this by changing *sender = to sender = (reassigning the variable), silently breaking the second assertion. Recommend restructuring: either use a BeforeEach re-init of the sender, or drop the second *sender = entirely since the first request already left the mock in a valid post-call state.

pkg/handler/trigger_handler_test.go:189,204 — Two Ginkgo It entries use TestTriggerHandler_ prefix (snake_case) which is not the project convention (seen elsewhere in the file as CamelCase, e.g. lines 57, 75, 90). While Ginkgo accepts any name, the Test prefix suggests a Testify heritage that doesn't apply here. Unlikely to cause issues, but inconsistent with the surrounding test style.

Nice to Have (Optional)

pkg/handler/trigger_handler.go:73 — The comment on triggerAcceptedResponse is accurate but verbose for a private struct. Consider: // Force is omitted when false (omitempty).

pkg/handler/trigger_handler.go:50 — libparse.ParseBoolDefault(ctx, req.URL.Query().Get("force"), false) silently accepts garbage. Per the plan concern: this is intentional — the test at line 111 ("treats garbage force values as false (lenient, no 400)") explicitly documents and covers this behavior. No change needed.

Plan concerns — both addressed:

1. Lenient force parsing — covered by explicit test at line 111; intentional API behavior.
2. omitempty on Force — correctly implemented; non-forced responses are byte-identical to the old {"status","url"} shape.

---

{
"verdict": "approve",
"summary": "The PR adds Force bool to the /trigger 202 response with correct omitempty semantics and comprehensive test coverage including the lenient parsing behavior. No functional issues found. Two minor test-quality issues noted (confusing mock reassignment pattern, inconsistent Ginkgo naming).",
"comments": [
{
"file": "pkg/handler/trigger_handler_test.go",
"line": 216,
"severity": "major",
"message": "sender mock reassignment is fragile: *sender = mocks.TriggerPRReviewCommandSender{} mutates the shared referent (so the test passes), but if a future developer changes it to sender = (variable reassignment), the second SendCommandCallCount() assertion breaks silently. Recommend using BeforeEach re-init or dropping the second *sender = entirely."
},
{
"file": "pkg/handler/trigger_handler_test.go",
"line": 189,
"severity": "nit",
"message": "Ginkgo It name uses TestTriggerHandler_ParsesForceTrue (Test prefix + snake_case) inconsistent with project convention of CamelCase used elsewhere in the file."
},
{
"file": "pkg/handler/trigger_handler_test.go",
"line": 204,
"severity": "nit",
"message": "Ginkgo It name uses TestTriggerHandler_ParsesForceFalse (Test prefix + snake_case) inconsistent with project convention of CamelCase used elsewhere in the file."
}
],
"concerns_addressed": [
"correctness: lenient parsing of force param — garbage values resolve to false with no 400; intentional API behavior covered by explicit test at line 111",
"correctness: omitempty on Force bool — false is omitted from JSON; correctly implemented so non-forced responses are byte-identical to the old wire format"
]
}