Skip to content

Commit 0dd4678

Browse files
committed
fix(incident): satisfy golangci-lint and stop pinning verification prose in tests
Two issues found in review, both against the same code: 1. golangci-lint (default set, staticcheck ST1005) flags the verification error for ending in punctuation. Extract the shared guidance sentence and the per-incident "not found" detail into named constants (commentVerificationGuidance, commentVerificationNotFoundDetailFmt) and drop the trailing period, matching this repo's existing convention for multi-sentence errors (args.go, root.go): internal periods are fine, a trailing one is not. 2. That extraction also fixes a test-design problem: the transport-error and not-found tests were each asserting a hand-copied fragment of the guidance sentence, so editing the prose in one place silently broke tests elsewhere — exactly what happened moving from "do not retry this command against the full batch" to "Do not write the comment again" earlier in this branch. Both tests now assert against the same commentVerificationGuidance symbol the production code emits, so a future reword of the constant keeps every caller in sync automatically instead of requiring a hunt for hand-duplicated copies. Also strengthened the transport-error test to assert the passing incident is NOT named as a problem, mirroring the batch test's coverage. Verified against golangci-lint v2.11.0 (matching .github/workflows/lint.yml exactly, not just whatever was already on PATH): 0 issues.
1 parent e71af6c commit 0dd4678

2 files changed

Lines changed: 39 additions & 15 deletions

File tree

internal/cli/command_test.go

Lines changed: 18 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -765,14 +765,17 @@ func TestCommandIncidentCommentFailsWhenTextNotFoundWithinBudget(t *testing.T) {
765765
if err == nil {
766766
t.Fatal("[not-found] expected a non-zero exit, got nil error")
767767
}
768-
if !strings.Contains(err.Error(), "no timeline entry matches the written text") {
769-
t.Fatalf("[not-found] unexpected error: %v", err)
768+
if !strings.Contains(err.Error(), "inc-1") {
769+
t.Fatalf("[not-found] expected the failing incident named in the error: %v", err)
770770
}
771771
if strings.Contains(err.Error(), "corrupted") {
772772
t.Fatalf("[not-found] must not claim corruption it has not established: %v", err)
773773
}
774-
if !strings.Contains(err.Error(), "Do not write the comment again") {
775-
t.Fatalf("[not-found] must warn against rewriting, at any granularity: %v", err)
774+
// Asserted against the same commentVerificationGuidance symbol the
775+
// production code emits, not a hand-copied fragment of it — a prose
776+
// rewording of the constant keeps this test green automatically.
777+
if !strings.Contains(err.Error(), commentVerificationGuidance) {
778+
t.Fatalf("[not-found] must carry the standard verification guidance: %v", err)
776779
}
777780
if strings.Contains(out, "Commented on") {
778781
t.Fatalf("[not-found] must not report success:\n%s", out)
@@ -793,7 +796,7 @@ func TestCommandIncidentCommentFailsWhenNoCommentEntryFound(t *testing.T) {
793796
if err == nil {
794797
t.Fatal("[readback-missing] expected a non-zero exit, got nil error")
795798
}
796-
if !strings.Contains(err.Error(), "no timeline entry matches the written text") {
799+
if !strings.Contains(err.Error(), fmt.Sprintf(commentVerificationNotFoundDetailFmt, maxIncidentFeedVerifyPages)) {
797800
t.Fatalf("[readback-missing] unexpected error: %v", err)
798801
}
799802
if strings.Contains(out, "Commented on") {
@@ -908,10 +911,17 @@ func TestCommandIncidentCommentTransportErrorCarriesAntiRetryWarning(t *testing.
908911
t.Fatal("[transport-error] expected a non-zero exit, got nil error")
909912
}
910913
if !strings.Contains(err.Error(), "inc-1") {
911-
t.Fatalf("[transport-error] expected inc-1 named in the error: %v", err)
914+
t.Fatalf("[transport-error] expected the failing incident named in the error: %v", err)
912915
}
913-
if !strings.Contains(err.Error(), "Do not write the comment again") {
914-
t.Fatalf("[transport-error] must carry the same anti-retry framing as the not-found case: %v", err)
916+
if strings.Contains(err.Error(), "inc-2") {
917+
t.Fatalf("[transport-error] inc-2 verified fine and must not be named as a problem: %v", err)
918+
}
919+
// Asserted against the same commentVerificationGuidance symbol
920+
// TestCommandIncidentCommentFailsWhenTextNotFoundWithinBudget checks,
921+
// proving both failure branches share identical guidance by construction
922+
// rather than by two independently maintained prose copies.
923+
if !strings.Contains(err.Error(), commentVerificationGuidance) {
924+
t.Fatalf("[transport-error] must carry the same guidance as the not-found case: %v", err)
915925
}
916926
if strings.Contains(err.Error(), "corrupted") {
917927
t.Fatalf("[transport-error] must not claim corruption: %v", err)

internal/cli/incident.go

Lines changed: 21 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -948,6 +948,24 @@ func resolveCommentFile(path string) (string, error) {
948948
// headroom for any real incident, and only bounds a pathological one.
949949
const maxIncidentFeedVerifyPages = 20
950950

951+
// commentVerificationGuidance is appended, verbatim, to every comment
952+
// verification failure — whether the entry simply couldn't be located (page
953+
// budget) or the re-fetch itself errored (transport failure). Both are
954+
// "could not confirm," never "confirmed missing," so both carry the
955+
// identical instruction. It is a named constant, not inline prose duplicated
956+
// at each call site, specifically so a test can assert against this exact
957+
// symbol instead of pinning a copy of the sentence: reword this constant and
958+
// every caller (production and test) picks up the new wording automatically,
959+
// with nothing left to fall out of sync.
960+
const commentVerificationGuidance = "The write API already reported success for the whole batch (a single POST covering every requested incident), and not finding a match here does not mean a comment is missing — this check can only confirm presence, never confirm absence. Do not write the comment again, for the full batch or for the incident(s) listed above alone: either risks a duplicate on a write that most likely already landed. Run `fduty incident timeline <id>` on the listed incident(s) to check by hand before deciding on anything further"
961+
962+
// commentVerificationNotFoundDetailFmt is the per-incident detail appended to
963+
// commentVerificationGuidance's problem list when the page walk completes
964+
// without finding a match, formatted with maxIncidentFeedVerifyPages. Also a
965+
// named symbol rather than inline prose, for the same reason as
966+
// commentVerificationGuidance above.
967+
const commentVerificationNotFoundDetailFmt = "no timeline entry matches the written text within the first %d pages"
968+
951969
// verifyIncidentCommentsWritten re-fetches every incident's timeline after a
952970
// comment write and confirms an entry with the exact text just sent is
953971
// present. The Comment API returns no handle for the created entry, so this
@@ -993,18 +1011,14 @@ func verifyIncidentCommentsWritten(ctx *RunContext, want string) error {
9931011
case err != nil:
9941012
problems = append(problems, fmt.Sprintf("incident %s: %v", id, err))
9951013
case !found:
996-
problems = append(problems, fmt.Sprintf("incident %s: no timeline entry matches the written text within the first %d pages", id, maxIncidentFeedVerifyPages))
1014+
problems = append(problems, fmt.Sprintf("incident %s: "+commentVerificationNotFoundDetailFmt, id, maxIncidentFeedVerifyPages))
9971015
}
9981016
}
9991017
if len(problems) == 0 {
10001018
return nil
10011019
}
1002-
return fmt.Errorf(
1003-
"comment verification could not confirm %d of %d incident(s) — %s. "+
1004-
"The write API already reported success for all %d incident(s) in this batch (a single POST covering the whole list), and not finding a match here does not mean a comment is missing — this check can only confirm presence, never confirm absence. "+
1005-
"Do not write the comment again, for the full batch or for the incident(s) listed above alone: either risks a duplicate on a write that most likely already landed. "+
1006-
"Run `fduty incident timeline <id>` on the listed incident(s) to check by hand before deciding on anything further.",
1007-
len(problems), len(ctx.Args), strings.Join(problems, "; "), len(ctx.Args))
1020+
return fmt.Errorf("comment verification could not confirm %d of %d incident(s) — %s. %s",
1021+
len(problems), len(ctx.Args), strings.Join(problems, "; "), commentVerificationGuidance)
10081022
}
10091023

10101024
// incidentTimelineHasComment walks every page of incidentID's i_comm feed

0 commit comments

Comments
 (0)