diff --git a/internal/cli/command_test.go b/internal/cli/command_test.go index b12af01..0de5f6f 100644 --- a/internal/cli/command_test.go +++ b/internal/cli/command_test.go @@ -581,7 +581,7 @@ func TestCommandIncidentComment(t *testing.T) { if got, want := strings.Join(stringsField(stub.bodies[0], "incident_ids"), ","), "inc-1,inc-2"; got != want { t.Fatalf("[incident-comment] expected ids %q, got %q", want, got) } - if !strings.Contains(out, "Commented on 2 incident(s).") { + if !strings.Contains(out, fmt.Sprintf(incidentCommentConfirmedFmt, 2)) { t.Fatalf("[incident-comment] unexpected output:\n%s", out) } } @@ -652,7 +652,7 @@ func TestCommandIncidentCommentPreservesShellMetacharactersByteForByte(t *testin if stub.bodies[0]["comment"] != want { t.Fatalf("[comment-file] comment reached the API mangled:\nwant: %q\n got: %q", want, stub.bodies[0]["comment"]) } - if !strings.Contains(out, "Commented on 1 incident(s).") { + if !strings.Contains(out, fmt.Sprintf(incidentCommentConfirmedFmt, 1)) { t.Fatalf("[comment-file] unexpected output:\n%s", out) } }) @@ -669,7 +669,7 @@ func TestCommandIncidentCommentPreservesShellMetacharactersByteForByte(t *testin if stub.bodies[0]["comment"] != want { t.Fatalf("[stdin] comment reached the API mangled:\nwant: %q\n got: %q", want, stub.bodies[0]["comment"]) } - if !strings.Contains(out, "Commented on 1 incident(s).") { + if !strings.Contains(out, fmt.Sprintf(incidentCommentConfirmedFmt, 1)) { t.Fatalf("[stdin] unexpected output:\n%s", out) } }) @@ -739,7 +739,7 @@ func TestCommandIncidentCommentSurvivesServerSideTrim(t *testing.T) { if stub.bodies[0]["comment"] != want { t.Fatalf("[server-trim] expected the wire payload pre-trimmed to match what the server stores:\nwant: %q\n got: %q", want, stub.bodies[0]["comment"]) } - if !strings.Contains(out, "Commented on 1 incident(s).") { + if !strings.Contains(out, fmt.Sprintf(incidentCommentConfirmedFmt, 1)) { t.Fatalf("[server-trim] unexpected output:\n%s", out) } } @@ -779,7 +779,7 @@ func TestCommandIncidentCommentSucceedsDespiteNewerConcurrentComment(t *testing. if err != nil { t.Fatalf("[newer-concurrent-comment] unexpected error: %v", err) } - if !strings.Contains(out, "Commented on 1 incident(s).") { + if !strings.Contains(out, fmt.Sprintf(incidentCommentConfirmedFmt, 1)) { t.Fatalf("[newer-concurrent-comment] unexpected output:\n%s", out) } } @@ -1030,7 +1030,7 @@ func TestCommandIncidentCommentVerifiesAcrossFeedPages(t *testing.T) { if err != nil { t.Fatalf("[feed-pagination] unexpected error: %v", err) } - if !strings.Contains(out, "Commented on 1 incident(s).") { + if !strings.Contains(out, fmt.Sprintf(incidentCommentConfirmedFmt, 1)) { t.Fatalf("[feed-pagination] unexpected output:\n%s", out) } } @@ -1152,7 +1152,7 @@ func runIncidentCommentVerifyProbe(t *testing.T, n int, tag string) int32 { if err != nil { t.Fatalf("[%s] unexpected error: %v", tag, err) } - if !strings.Contains(out, fmt.Sprintf("Commented on %d incident(s).", n)) { + if !strings.Contains(out, fmt.Sprintf(incidentCommentConfirmedFmt, n)) { t.Fatalf("[%s] unexpected output:\n%s", tag, out) } return maxInFlight.Load() @@ -1185,6 +1185,363 @@ func TestCommandIncidentCommentVerifyRespectsConcurrencyBound(t *testing.T) { } } +// TestCommandIncidentCommentStructuredReturnsVerifiedEntry guards what the +// command returns, not just its exit code. /incident/comment answers with an +// empty data envelope, so the only description of a stored comment comes from +// the read-after-write feed walk the command already performs; it emits that +// entry instead of discarding it, which is what spares a caller the separate +// `incident timeline` read it would otherwise need to learn the stored body, +// the author and the timestamp. +// +// The TOON subtest is not duplicate coverage of the JSON one: TOON is a +// different marshaller (toon.Marshal, vs json.MarshalIndent over +// output.NullUnsetInstants), and a top-level array of flat structs is exactly +// the shape it renders tabularly, so a change that broke only the TOON path +// would pass the JSON assertions. +func TestCommandIncidentCommentStructuredReturnsVerifiedEntry(t *testing.T) { + const ( + comment = "the real comment" + creatorID = 4242 + // A whole-second millisecond epoch, so the RFC3339 rendering carries + // no fractional part and the assertion below can compare instants + // without worrying about sub-second formatting. + createdAtMilli = int64(1762761600000) + ) + + newCommentStub := func(t *testing.T) { + stub := newGFStub(t) + stub.dataForPath = func(path string, body map[string]any) any { + switch path { + case "/incident/comment": + return map[string]any{} + case "/incident/feed": + return map[string]any{"items": []any{ + map[string]any{ + "type": "i_comm", + "created_at": createdAtMilli, + "creator_id": creatorID, + "detail": map[string]any{"comment": comment}, + }, + }} + default: + return map[string]any{} + } + } + } + + t.Run("json", func(t *testing.T) { + saveAndResetGlobals(t) + newCommentStub(t) + commentFile := writeCommentFile(t, comment) + + stdout, _, err := execCommandSplit("incident", "comment", "inc-1", "--comment-file", commentFile, "--json") + if err != nil { + t.Fatalf("[entry-json] unexpected error: %v", err) + } + // A one-incident call must still be an array, not a bare object: a + // shape that changed with batch size would force every caller to + // branch on arity before reading its own result. + if trimmed := strings.TrimSpace(stdout); !strings.HasPrefix(trimmed, "[") { + t.Fatalf("[entry-json] output must be a top-level array:\n%s", stdout) + } + + var entries []struct { + IncidentID string `json:"incident_id"` + Comment string `json:"comment"` + CreatorID int64 `json:"creator_id"` + CreatedAt string `json:"created_at"` + } + if err := json.Unmarshal([]byte(stdout), &entries); err != nil { + t.Fatalf("[entry-json] output is not valid JSON (%v):\n%s", err, stdout) + } + if len(entries) != 1 { + t.Fatalf("[entry-json] expected 1 entry, got %d:\n%s", len(entries), stdout) + } + got := entries[0] + if got.IncidentID != "inc-1" { + t.Errorf("[entry-json] incident_id = %q, want %q", got.IncidentID, "inc-1") + } + if got.Comment != comment { + t.Errorf("[entry-json] comment = %q, want %q", got.Comment, comment) + } + if got.CreatorID != creatorID { + t.Errorf("[entry-json] creator_id = %d, want %d", got.CreatorID, creatorID) + } + // created_at must be the feed entry's instant, rendered the way every + // other timestamp in structured output is (RFC3339, local zone) rather + // than leaked as a raw epoch integer. + parsed, parseErr := time.Parse(time.RFC3339, got.CreatedAt) + if parseErr != nil { + t.Fatalf("[entry-json] created_at %q is not RFC3339: %v", got.CreatedAt, parseErr) + } + if parsed.UnixMilli() != createdAtMilli { + t.Errorf("[entry-json] created_at = %q (%d ms), want %d ms", got.CreatedAt, parsed.UnixMilli(), createdAtMilli) + } + }) + + t.Run("toon", func(t *testing.T) { + saveAndResetGlobals(t) + newCommentStub(t) + commentFile := writeCommentFile(t, comment) + + stdout, _, err := execCommandSplit("incident", "comment", "inc-1", "--comment-file", commentFile, "--output-format", "toon") + if err != nil { + t.Fatalf("[entry-toon] unexpected error: %v", err) + } + for _, want := range []string{"incident_id", "inc-1", comment, "4242"} { + if !strings.Contains(stdout, want) { + t.Errorf("[entry-toon] output missing %q:\n%s", want, stdout) + } + } + }) +} + +// TestCommandIncidentCommentStructuredEntriesFollowArgumentOrder is the +// output-side counterpart to +// TestCommandIncidentCommentVerifyOrderStableDespiteCompletionOrder: the +// per-incident feed walks run concurrently, so the emitted entries must be +// ordered by the ids the caller passed, never by whichever request finished +// first. The stub sleeps longest for the first id and shortest for the last, +// making completion order the exact reverse of argument order — a regression +// that appended entries as they arrived would come out backwards here, while +// a stub with uniform latency could pass by luck. +func TestCommandIncidentCommentStructuredEntriesFollowArgumentOrder(t *testing.T) { + saveAndResetGlobals(t) + stub := newGFStub(t) + + const ( + n = 4 + comment = "batch comment" + ) + ids := make([]string, n) + delay := make(map[string]time.Duration, n) + for i := range ids { + ids[i] = fmt.Sprintf("inc-%d", i+1) + delay[ids[i]] = time.Duration(n-i) * 15 * time.Millisecond + } + stub.dataForPath = func(path string, body map[string]any) any { + switch path { + case "/incident/comment": + return map[string]any{} + case "/incident/feed": + incID, _ := body["incident_id"].(string) + time.Sleep(delay[incID]) + return map[string]any{"items": []any{ + map[string]any{"type": "i_comm", "created_at": 1, "detail": map[string]any{"comment": comment}}, + }} + default: + return map[string]any{} + } + } + commentFile := writeCommentFile(t, comment) + + args := append([]string{"incident", "comment"}, ids...) + args = append(args, "--comment-file", commentFile, "--json") + stdout, _, err := execCommandSplit(args...) + if err != nil { + t.Fatalf("[entry-order] unexpected error: %v", err) + } + + var entries []struct { + IncidentID string `json:"incident_id"` + } + if err := json.Unmarshal([]byte(stdout), &entries); err != nil { + t.Fatalf("[entry-order] output is not valid JSON (%v):\n%s", err, stdout) + } + if len(entries) != n { + t.Fatalf("[entry-order] expected %d entries, got %d:\n%s", n, len(entries), stdout) + } + for i, want := range ids { + if entries[i].IncidentID != want { + t.Fatalf("[entry-order] entry %d is %q, want %q — entries came out in completion order, not argument order:\n%s", + i, entries[i].IncidentID, want, stdout) + } + } +} + +// TestCommandIncidentCommentStructuredEmitsNothingWhenUnconfirmed guards the +// failure path's output, which matters as much as its exit code: when any +// incident in the batch can't be confirmed, the command must emit no entries +// at all rather than a partial array. A caller that saw entries for the +// confirmed incidents next to a non-zero exit would have to reconcile the two +// signals; there is a single batch write behind them, so the result is +// reported whole or not at all. +// +// TestCommandIncidentCommentChecksEntireBatchNotJustFirstFailure runs the same +// mixed verified/unverified batch but in table mode, where it asserts the +// "Commented on ..." line is absent. That assertion says nothing here: in +// structured mode there is no such line to suppress, and what must not appear +// is a partial array. Hence two tests over one scenario, one per output mode, +// rather than one test carrying both contracts. +func TestCommandIncidentCommentStructuredEmitsNothingWhenUnconfirmed(t *testing.T) { + saveAndResetGlobals(t) + stub := newGFStub(t) + stub.dataForPath = func(path string, body map[string]any) any { + switch path { + case "/incident/comment": + return map[string]any{} + case "/incident/feed": + // inc-1 verifies, inc-2 never does. + if incID, _ := body["incident_id"].(string); incID == "inc-1" { + return map[string]any{"items": []any{ + map[string]any{"type": "i_comm", "created_at": 1, "detail": map[string]any{"comment": "the real comment"}}, + }} + } + return map[string]any{"items": []any{}} + default: + return map[string]any{} + } + } + commentFile := writeCommentFile(t, "the real comment") + + stdout, _, err := execCommandSplit("incident", "comment", "inc-1", "inc-2", "--comment-file", commentFile, "--json") + if err == nil { + t.Fatal("[entry-unconfirmed] expected a non-zero exit, got nil error") + } + if strings.TrimSpace(stdout) != "" { + t.Fatalf("[entry-unconfirmed] expected no entries on stdout, got:\n%s", stdout) + } + if !strings.Contains(err.Error(), "inc-2") { + t.Fatalf("[entry-unconfirmed] expected the unconfirmed incident named in the error: %v", err) + } +} + +// TestCommandIncidentCommentConfirmationIsTheFirstLine pins that the default +// (non-structured) output is exactly the one confirmation line — see +// incidentCommentConfirmedFmt for why that matters. A `strings.Contains` +// assertion cannot catch the regression this guards: a line pushed down by +// output added above it still "contains" the sentence. +func TestCommandIncidentCommentConfirmationIsTheFirstLine(t *testing.T) { + saveAndResetGlobals(t) + newIncidentCommentEchoStub(t) + commentFile := writeCommentFile(t, "rollback started") + + stdout, _, err := execCommandSplit("incident", "comment", "inc-1", "inc-2", "--comment-file", commentFile) + if err != nil { + t.Fatalf("[first-line] unexpected error: %v", err) + } + + want := fmt.Sprintf(incidentCommentConfirmedFmt, 2) + if got := strings.TrimRight(stdout, "\n"); got != want { + t.Fatalf("[first-line] default output must be exactly the one confirmation line %q — a caller piping this through `head` misses anything else — got:\n%s", + want, stdout) + } +} + +// newIncidentCommentMentionStub serves a feed whose stored comment is +// storedComment whatever was written, so a test can stand in for the server's +// canonical rewrite of a mention label. +func newIncidentCommentMentionStub(t *testing.T, storedComment string) { + t.Helper() + stub := newGFStub(t) + stub.dataForPath = func(path string, body map[string]any) any { + if path == "/incident/feed" { + return map[string]any{"items": []any{ + map[string]any{ + "type": "i_comm", + "created_at": 1, + "detail": map[string]any{"comment": storedComment}, + }, + }} + } + return map[string]any{} + } +} + +// incidentCommentMentionSent is the comment body every mention case below +// writes. The member id is the one the published API example uses. +const incidentCommentMentionSent = "Please verify the fix [@Jane](flashduty://ref/member/2476444212131) thanks" + +// TestCommandIncidentCommentVerifiesMentionsByMemberID covers the comparison +// rule for member mentions; why the stored label differs from the one sent is +// documented on canonicalizeIncidentCommentMentions. +// +// Both directions matter, which is what the cases below are for: the +// insensitivity is scoped to the label alone. A different member id is a +// different mention and must still fail, otherwise the check would confirm a +// comment that mentioned someone else. +func TestCommandIncidentCommentVerifiesMentionsByMemberID(t *testing.T) { + // The returned entry must report what is STORED, canonical label + // included, not echo what was sent — that is what makes it worth reading. + // Only this case decodes the output, so it stays separate from the table. + t.Run("label rewritten still matches, and the entry carries the stored text", func(t *testing.T) { + saveAndResetGlobals(t) + const stored = "Please verify the fix [@Jane Doe](flashduty://ref/member/2476444212131) thanks" + newIncidentCommentMentionStub(t, stored) + commentFile := writeCommentFile(t, incidentCommentMentionSent) + + stdout, _, err := execCommandSplit("incident", "comment", "inc-1", "--comment-file", commentFile, "--json") + if err != nil { + t.Fatalf("[mention-label] a successful write was reported unconfirmed: %v", err) + } + var entries []struct { + Comment string `json:"comment"` + } + if err := json.Unmarshal([]byte(stdout), &entries); err != nil { + t.Fatalf("[mention-label] output is not valid JSON (%v):\n%s", err, stdout) + } + if len(entries) != 1 { + t.Fatalf("[mention-label] expected 1 entry, got %d:\n%s", len(entries), stdout) + } + if entries[0].Comment != stored { + t.Errorf("[mention-label] comment = %q, want the stored text %q", entries[0].Comment, stored) + } + }) + + for _, tc := range []struct { + name string + // sent defaults to incidentCommentMentionSent when empty. + sent string + stored string + wantErr bool + }{ + { + // The server backslash-escapes ASCII punctuation in the canonical + // name, so a name containing "]" arrives as "\]". A label pattern + // that stopped at the first "]" cuts the match short here and + // reports this good write as unconfirmed. + name: "escaped punctuation in the canonical label still matches", + stored: `Please verify the fix [@Jane\]\*\_Doe](flashduty://ref/member/2476444212131) thanks`, + }, + { + // Every mention in the body is canonicalized, not just the first. + name: "several rewritten mentions still match", + sent: "Please verify [@Jane](flashduty://ref/member/2476444212131) and [@John](flashduty://ref/member/2476444212222) now", + stored: "Please verify [@Jane Doe](flashduty://ref/member/2476444212131) and [@John Roe](flashduty://ref/member/2476444212222) now", + }, + { + // A mention of someone else: the id differs, and so does the + // label, which is the realistic shape — the server canonicalizes + // the other member's name too. Since labels are discarded, the id + // is the only thing that can make this fail, which is the point. + name: "different member id does not match", + stored: "Please verify the fix [@Someone Else](flashduty://ref/member/2476444212999) thanks", + wantErr: true, + }, + { + // Identical mention, one word changed outside it. + name: "text outside the mention is still compared exactly", + stored: "Please verify the patch [@Jane Doe](flashduty://ref/member/2476444212131) thanks", + wantErr: true, + }, + } { + t.Run(tc.name, func(t *testing.T) { + saveAndResetGlobals(t) + newIncidentCommentMentionStub(t, tc.stored) + sent := tc.sent + if sent == "" { + sent = incidentCommentMentionSent + } + commentFile := writeCommentFile(t, sent) + + _, _, err := execCommandSplit("incident", "comment", "inc-1", "--comment-file", commentFile, "--json") + if (err != nil) != tc.wantErr { + t.Fatalf("wantErr=%v, got err=%v", tc.wantErr, err) + } + }) + } +} + // TestCommandIncidentLifecycleRejectsMoreThan100IDs covers the curated // commands that still enforce the 100-id batch cap client-side. unack and wake // were dropped in favor of their generated twins, which carry no client-side diff --git a/internal/cli/incident.go b/internal/cli/incident.go index 4d3b76c..b38d06b 100644 --- a/internal/cli/incident.go +++ b/internal/cli/incident.go @@ -1063,9 +1063,23 @@ penalized — measure with 'wc -m', not 'wc -c'. Use --mute-reply when the comment should not trigger webhook reply behavior. After writing, the command reads back every incident's timeline and verifies -an entry with the exact (trimmed) text is present; on any incident it can't -confirm, across the whole batch, it exits non-zero instead of reporting -success.`, +an entry with the (trimmed) text is present; on any incident it can't confirm, +across the whole batch, it exits non-zero instead of reporting success. The +default output is one line and names the confirmation. + +A member mention written as [@Label](flashduty://ref/member/) is +compared by member id, not by label: the server substitutes the member's +canonical name for whatever label was written, and refreshes it again when it +serves the timeline, so the stored label is not the one sent. Everything else +in the body, a mention's member id included, is compared exactly. + +The confirmed entries are the command's output: with --json or +--output-format toon it prints one object per incident, in the order the ids +were given, carrying the incident id, the stored comment body (with the +canonical mention labels the server applied), the author the server recorded, +and the entry's timestamp. A follow-up 'flashduty incident timeline ' is +not needed to find out what landed. The shape is an array whatever the batch +size, so a single-incident call reads the same as a hundred-incident one.`, Example: ` flashduty incident comment inc_123 --comment-file ./comment.txt printf '%s' 'Rollback started' | flashduty incident comment inc_123 --comment-file -`, Args: requireArgs("incident_id"), @@ -1108,12 +1122,12 @@ success.`, return err } - if err := verifyIncidentCommentsWritten(ctx, comment); err != nil { + entries, err := verifyIncidentCommentsWritten(ctx, comment) + if err != nil { return err } - ctx.WriteResult(fmt.Sprintf("Commented on %d incident(s).", len(ctx.Args))) - return nil + return ctx.WriteResultJSON(entries, fmt.Sprintf(incidentCommentConfirmedFmt, len(ctx.Args))) }) }, } @@ -1173,10 +1187,94 @@ const commentVerificationGuidance = "The write API already reported success for // commentVerificationGuidance above. const commentVerificationNotFoundDetailFmt = "no timeline entry matches the written text within the first %d pages" +// incidentCommentConfirmedFmt is the default-output result line, and the +// command's FIRST line of output: nothing is printed above it, and the +// per-entry detail goes to structured output rather than extra lines that +// would push it down. Both properties are load-bearing — a caller that pipes +// this command into `head` must still see the whole verdict. +// +// The line names the read-back on purpose. The exit code already carries the +// verdict, but an exit code is not visible evidence: an output line that said +// only that a comment was sent leaves a reader no way to tell that the +// command confirmed it on the timeline, so the reader goes and reads the +// timeline anyway — the exact round trip this command exists to remove. +// +// It is a named constant, not an inline literal, so tests assert against this +// symbol instead of a hand-copied sentence; reword it here and every caller, +// production and test, follows. +const incidentCommentConfirmedFmt = "Commented on %d incident(s); confirmed on each timeline." + +// incidentCommentMentionRe matches one member mention in a comment body: a +// Markdown link whose label starts with "@" and whose destination is the +// member reference URL. +// +// The label alternation accepts a backslash escape ("\\.") as well as a plain +// non-"]" rune, because the canonical name the server substitutes has its +// ASCII punctuation backslash-escaped — a canonical name containing "]" +// arrives as "\]", and a label pattern that simply stopped at the first "]" +// would cut the match short and leave the rest of the link in the compared +// text. +var incidentCommentMentionRe = regexp.MustCompile(`\[@(?:\\.|[^\\\]])*\]\((flashduty://ref/member/\d+)\)`) + +// canonicalizeIncidentCommentMentions rewrites every member mention to a +// label-free form, so that two comment bodies compare equal when the only +// thing that differs is a mention's display label. +// +// The server neither stores nor returns the label the caller wrote. The write +// path replaces each mention's label with the mentioned member's current +// canonical name, and the timeline read path refreshes the label again from +// current member names before serving the entry. So the text of a comment +// that was written perfectly differs from the text sent whenever the caller's +// label was not already the canonical name — and a literal comparison called +// that write unconfirmed, which sends the caller off to read the timeline by +// hand after a write that fully succeeded. +// +// Only the label is discarded. The member id stays in the compared form, so a +// mention of a DIFFERENT member still fails to match, and so does any other +// difference anywhere in the body. +// +// A mention inside a code span or code block is rewritten here even though +// the server leaves that one alone; since both sides are normalized the same +// way, such a mention still matches itself, so this costs no accuracy on the +// "did my write land" question. It only widens the older-identical-text +// limitation documented on findIncidentCommentEntry by the width of a mention +// label, which is far narrower than parsing Markdown here to find code spans +// would be worth. +func canonicalizeIncidentCommentMentions(comment string) string { + return incidentCommentMentionRe.ReplaceAllString(comment, "[@](${1})") +} + +// incidentCommentEntry is one incident's timeline entry as located by the +// read-after-write check, and the command's output. +// +// It is the only description of the written comment any Flashduty API will +// give back: POST /incident/comment answers with an empty data envelope (no +// id, no echo of what it stored), and /incident/feed entries carry no +// per-entry id either — IncidentFeedItem has account_id, created_at, +// creator_id, deleted_at, detail, ref_id, type and updated_at, none of which +// identifies the comment itself. So these four fields are everything that can +// be reported about a comment that landed: which incident it landed on, the +// body as stored, who the server recorded as its author (0 when it attributed +// the write to the system rather than a member), and when it was written. +type incidentCommentEntry struct { + IncidentID string `json:"incident_id" toon:"incident_id"` + Comment string `json:"comment" toon:"comment"` + CreatorID int64 `json:"creator_id" toon:"creator_id"` + CreatedAt flashduty.TimestampMilli `json:"created_at" toon:"created_at"` +} + // verifyIncidentCommentsWritten re-fetches every incident's timeline after a -// comment write and confirms an entry with the exact text just sent is -// present. The Comment API returns no handle for the created entry, so this -// must find it by re-listing the feed instead. +// comment write, confirms an entry matching the text just sent is present, +// and returns those entries in ctx.Args order. Matching is exact except for +// member-mention labels, which are compared by member id — see +// canonicalizeIncidentCommentMentions. The Comment API returns no +// handle for the created entry, so this must find it by re-listing the feed +// instead — and since it has the entry in hand at that point, it hands it +// back rather than reducing it to a yes/no: a caller that needs to know what +// landed gets it from this one invocation instead of a second timeline read. +// +// A non-nil error means no entries: the batch either verified whole or is +// reported whole, so there is no partial success for a caller to interpret. // // It deliberately does not track "the newest" entry and compare it against // want. This is a war-room CLI: another human or bot commenting on the same @@ -1185,7 +1283,7 @@ const commentVerificationNotFoundDetailFmt = "no timeline entry matches the writ // their own i_comm entries. Whichever entry happens to have the highest // CreatedAt at read time is not necessarily ours, so comparing against it // produced false "corrupted" reports on perfectly good writes. Searching for -// an entry whose text exactly equals want is immune to unrelated concurrent +// an entry whose text matches want is immune to unrelated concurrent // comments and needs no CreatedAt comparison (and therefore no ordering // assumption about /incident/feed's undocumented default sort) at all. // @@ -1203,15 +1301,15 @@ const commentVerificationNotFoundDetailFmt = "no timeline entry matches the writ // a time, instead of walking ctx.Args one at a time: with a batch of up to // 100 incidents and up to maxIncidentFeedVerifyPages page fetches apiece, a // sequential walk could take thousands of round-trips before the command -// returned. Each goroutine writes only its own slot of a pre-sized results -// slice (index == position in ctx.Args), so the problems reported below -// always come out in ctx.Args order regardless of which request happens to -// finish first — the message must read the same on every run, not depend on -// network timing. +// returned. Each goroutine writes only its own slot of the two pre-sized +// result slices (index == position in ctx.Args), so both the problems and the +// entries reported below always come out in ctx.Args order regardless of +// which request happens to finish first — the message and the output must +// read the same on every run, not depend on network timing. // // Neither a page-budget miss nor a transport error while re-fetching the feed // means the write was corrupted — corruption can no longer even be observed -// by this check, since it never looks at any text other than an exact match. +// by this check, since it never looks at any text other than a match. // Both are reported as "could not confirm," with identical framing, because // both follow a write the API already reported as successful, and because // this check can only confirm presence, never confirm absence: a miss here @@ -1221,8 +1319,9 @@ const commentVerificationNotFoundDetailFmt = "no timeline entry matches the writ // incident (which risks a duplicate on a write that most likely landed and // simply couldn't be located within budget). The safe next step is to look, // not to write. -func verifyIncidentCommentsWritten(ctx *RunContext, want string) error { +func verifyIncidentCommentsWritten(ctx *RunContext, want string) ([]incidentCommentEntry, error) { slots := make([]string, len(ctx.Args)) + found := make([]incidentCommentEntry, len(ctx.Args)) var wg sync.WaitGroup sem := make(chan struct{}, maxIncidentVerifyConcurrency) @@ -1233,12 +1332,14 @@ func verifyIncidentCommentsWritten(ctx *RunContext, want string) error { sem <- struct{}{} defer func() { <-sem }() - found, err := incidentTimelineHasComment(ctx, id, want) + entry, err := findIncidentCommentEntry(ctx, id, want) switch { case err != nil: slots[i] = fmt.Sprintf("incident %s: %v", id, err) - case !found: + case entry == nil: slots[i] = fmt.Sprintf("incident %s: "+commentVerificationNotFoundDetailFmt, id, maxIncidentFeedVerifyPages) + default: + found[i] = *entry } }(i, id) } @@ -1250,19 +1351,30 @@ func verifyIncidentCommentsWritten(ctx *RunContext, want string) error { problems = append(problems, p) } } - if len(problems) == 0 { - return nil + if len(problems) > 0 { + return nil, fmt.Errorf("comment verification could not confirm %d of %d incident(s) — %s. %s", + len(problems), len(ctx.Args), strings.Join(problems, "; "), commentVerificationGuidance) } - return fmt.Errorf("comment verification could not confirm %d of %d incident(s) — %s. %s", - len(problems), len(ctx.Args), strings.Join(problems, "; "), commentVerificationGuidance) + + // No problems means every slot holds a located entry: the switch above + // fills exactly one of the two slices per index, and problems is empty. + return found, nil } -// incidentTimelineHasComment walks every page of incidentID's i_comm feed +// findIncidentCommentEntry walks every page of incidentID's i_comm feed // entries (up to maxIncidentFeedVerifyPages) looking for one whose text -// exactly equals want. +// equals want once both sides' mention labels are canonicalized, and returns +// it. A nil entry with a nil error means the page budget ran out without a +// match — the "could not confirm" case, which is not evidence the comment is +// absent (see verifyIncidentCommentsWritten). +// +// The returned entry carries the feed's text, NOT the canonicalized form: +// that text is what is stored, including the canonical member names the +// server substituted for the caller's mention labels, which is exactly what a +// caller asking "what landed?" wants to read back. // // Converse limitation, accepted: if an OLDER i_comm entry already carries -// text byte-identical to want (e.g. a templated automation posting the same +// text matching want (e.g. a templated automation posting the same // status line on every run) and the write being verified was genuinely // dropped, this reports success anyway — it can only prove "this text is // somewhere on the timeline," not "this specific write landed." That is @@ -1272,7 +1384,9 @@ func verifyIncidentCommentsWritten(ctx *RunContext, want string) error { // Do not add a CreatedAt floor to close this gap — that reintroduces the // clock-ordering dependence that caused the original bug, to guard a narrow, // accepted risk. -func incidentTimelineHasComment(ctx *RunContext, incidentID, want string) (bool, error) { +func findIncidentCommentEntry(ctx *RunContext, incidentID, want string) (*incidentCommentEntry, error) { + wantCanonical := canonicalizeIncidentCommentMentions(want) + for page := 1; page <= maxIncidentFeedVerifyPages; page++ { result, _, err := ctx.Client.Incidents.Feed(cmdContext(ctx.Cmd), &flashduty.ListIncidentFeedRequest{ IncidentID: incidentID, @@ -1280,18 +1394,29 @@ func incidentTimelineHasComment(ctx *RunContext, incidentID, want string) (bool, ListOptions: flashduty.ListOptions{Page: page, Limit: 100}, }) if err != nil { - return false, err + return nil, err } for _, item := range result.Items { - if comment, ok := incidentCommentText(item); ok && comment == want { - return true, nil + comment, ok := incidentCommentText(item) + if !ok || canonicalizeIncidentCommentMentions(comment) != wantCanonical { + continue } + // incidentID, not item.RefID: ref_id is documented as "the source + // alert or incident this entry references", so it is not + // guaranteed to be the incident whose feed was queried, and the + // caller needs the id it asked about. + return &incidentCommentEntry{ + IncidentID: incidentID, + Comment: comment, + CreatorID: item.CreatorID, + CreatedAt: item.CreatedAt, + }, nil } if !result.HasNextPage { break } } - return false, nil + return nil, nil } // incidentCommentText returns item's comment body if it is an i_comm entry. diff --git a/skills/flashduty/reference/incident.md b/skills/flashduty/reference/incident.md index 282da2f..2407dcc 100644 --- a/skills/flashduty/reference/incident.md +++ b/skills/flashduty/reference/incident.md @@ -77,7 +77,7 @@ flashduty incident resolve --root-cause "DB primary failover delay Projected `similar` lists stay below 16 KiB: when the page would overflow, only the leading rows that fit are emitted — every value intact — and a stderr note says how many rows were emitted. A trailing `...` in a list row, with a stderr note naming the clipped fields, appears only when one row alone exceeds the budget. `detail --fields` is different: it never shortens values — the projection must fit within 8 KiB as requested or the command fails and names the largest fields, so drop some fields (or drop `--fields` for the full unbounded detail) and retry. -`comment` never accepts the text as a command-line argument — only `--comment-file ` (or `--comment-file -` to read stdin), so backticks/`$()`/quotes inside the comment are inert. The command also reads back every target's timeline after writing and exits non-zero unless it finds an entry matching what it sent, so `Commented on ...` is proof of content fidelity, not just acceptance — no separate manual read-back is needed. Leading and trailing whitespace is stripped before sending (the server strips it too, so this is what gets stored); everything else, including interior blank lines, is preserved exactly. +`comment` never accepts the text as a command-line argument — only `--comment-file ` (or `--comment-file -` to read stdin), so backticks/`$()`/quotes inside the comment are inert. The command also reads back every target's timeline after writing and exits non-zero unless it finds an entry matching what it sent, so its first output line — `Commented on 2 incident(s); confirmed on each timeline.` — is proof of content fidelity, not just acceptance, and no separate manual read-back is needed. That line is the whole of the default output, so it survives a `| head`. A member mention written as `[@Label](flashduty://ref/member/)` is matched by member id, not by label: the server replaces the label with the member's canonical name and refreshes it again when serving the timeline, so the stored label is never the one you sent. Everything else in the body, a mention's member id included, must match exactly. With `--json` (or `--output-format toon`) it prints the confirmed entries themselves — a TOP-LEVEL array, one object per incident in the order the ids were given, with `incident_id`, the stored `comment` (canonical mention labels included, so this is where you read back the real names), `creator_id` (`0` when the server attributed the write to the system) and `created_at` — so `jq '.[].created_at'` answers "what landed, as whom, when" without an `incident timeline` call. The array shape does not change with batch size: a one-incident call is a one-element array, never a bare object. The API itself returns nothing for this write (`POST /incident/comment` answers with an empty `data` envelope, and feed entries carry no per-entry id), so these four fields are the whole of what any Flashduty endpoint can report about a stored comment — there is no comment id to ask for. Leading and trailing whitespace is stripped before sending (the server strips it too, so this is what gets stored); everything else, including interior blank lines, is preserved exactly. > `incident list --output-format json|toon` defaults to the compact row projection `incident_id,num,title,incident_severity,progress,start_time,channel_id,detail_url`. Pass `--fields incident_id,num,title,channel_id,start_time,detail_url` when you need different list columns; use `incident detail ` / `incident get ` for full incident records. Any list-response field — including `labels` — is selectable this way (a key missing from the output means it wasn't selected, NOT that the server omits it; the command prints a stderr note when the default projection applies). The one exception is `alerts`: neither list nor detail responses ever fill it — use `incident alerts ` for an incident's alerts. Wide fields over many rows can exceed the 16 KiB structured-output bound; when that happens the command emits only the leading rows that fit — every value intact — and a stderr note says how many rows were emitted, so lower `--limit` or narrow `--fields` to fit more rows per page. Only when one row alone exceeds the bound does it shorten long string values (a stderr note says how many values were clipped and in which fields); if the row cannot be shortened to fit, the command errors and names the largest fields by aggregate size, so drop the field it names, or use `insight` aggregates for distributions instead of dumping labels row by row.