From d54300abaad7d46ecaaefcb2c99ffd63b1ba720f Mon Sep 17 00:00:00 2001 From: Bohan Jiang <52446949+Bohan-J@users.noreply.github.com> Date: Wed, 16 Sep 2026 17:29:41 +0800 Subject: [PATCH] refactor(claim): compare only title and description (MUL-7344) (#8488) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Status leaves the compared set, so the snapshot is now just the two body fields. The comparison answers one question — must the agent re-read the issue BODY? — and status never helped answer it: its current value ships on every claim, so the agent learns it without a read. Worse, comparing it worked against the feature. The workflow has the agent set in_progress when it starts and in_review when it delivers, so the agent's OWN bookkeeping came back the next turn as "changed: status" and demanded a body re-read that nothing in the body justified. Replaying this issue's 21 follow-ups, that alone cut "unchanged" from 17 to 4. The earlier rationale for keeping it — a free signal that somebody intervened — was wrong on both counts: it is not free, and the agent can see an intervention by comparing the shipped status against the value it last set itself. issueSnapshotVersion stays 1, and this time rows in the old shape DO exist. That is still correct: removing a field is backward compatible, because every field the new code reads is present in an old row and still means the same thing, and the stale "status" key is ignored. The comment now states the actual bump rule rather than "the set changed" — bump when an old row can no longer answer the current question, which means adding a field or changing how a value is derived, not removing one. A test pins the legacy shape comparing correctly in both the unchanged and the changed direction, so that claim is checked rather than asserted. No migration: the column is JSONB and the schema is unchanged. TestClaimTaskByRuntime_AssigneeAndPriorityAreOutOfScope becomes NonBodyFieldsAreOutOfScope and now moves status alongside assignee and priority, asserting the current status still ships with its NEW value — without that, the fixture's UPDATE could silently not apply and the empty changed-list would prove nothing. Co-authored-by: multica-agent --- .../execenv/issue_state_instructions.go | 2 +- .../execenv/issue_state_instructions_test.go | 2 +- server/internal/daemon/prompt_test.go | 2 +- server/internal/daemon/types.go | 4 +- server/internal/handler/agent.go | 4 +- .../handler/daemon_issue_snapshot_test.go | 129 +++++++++++++----- server/internal/handler/issue_snapshot.go | 76 +++++------ .../499_agent_task_issue_snapshot.up.sql | 25 ++-- 8 files changed, 155 insertions(+), 89 deletions(-) diff --git a/server/internal/daemon/execenv/issue_state_instructions.go b/server/internal/daemon/execenv/issue_state_instructions.go index 097d4c7fb..658251216 100644 --- a/server/internal/daemon/execenv/issue_state_instructions.go +++ b/server/internal/daemon/execenv/issue_state_instructions.go @@ -58,7 +58,7 @@ func BuildIssueStateHint(issueID, issueStatus, assigneeType, assigneeID string, state := fmt.Sprintf("status: %s; assignee: %s", issueStatus, issueAssigneeLabel(assigneeType, assigneeID)) if len(changedFields) == 0 { return fmt.Sprintf( - "The issue is unchanged since your last run — the server compared title, description and status (%s). "+ + "The issue is unchanged since your last run — the server compared title and description (%s). "+ "That answers workflow step 1: continue from your resumed context, and re-read with %s only if resumed memory is not enough.\n\n", state, readCommand, ) diff --git a/server/internal/daemon/execenv/issue_state_instructions_test.go b/server/internal/daemon/execenv/issue_state_instructions_test.go index 022b90700..46fd7dc61 100644 --- a/server/internal/daemon/execenv/issue_state_instructions_test.go +++ b/server/internal/daemon/execenv/issue_state_instructions_test.go @@ -23,7 +23,7 @@ func TestBuildIssueStateHintUnchangedReplacesTheRead(t *testing.T) { for _, want := range []string{ "The issue is unchanged since your last run", - "the server compared title, description and status", + "the server compared title and description", "status: in_progress; assignee: agent agent-1", "That answers workflow step 1", "only if resumed memory is not enough", diff --git a/server/internal/daemon/prompt_test.go b/server/internal/daemon/prompt_test.go index c1098c479..2bd5bdd15 100644 --- a/server/internal/daemon/prompt_test.go +++ b/server/internal/daemon/prompt_test.go @@ -2110,7 +2110,7 @@ func TestBuildPromptIssueUnchangedDropsTheIssueRead(t *testing.T) { } for _, want := range []string{ "The issue is unchanged since your last run", - "the server compared title, description and status", + "the server compared title and description", "status: in_progress; assignee: agent agent-7", "only if resumed memory is not enough", } { diff --git a/server/internal/daemon/types.go b/server/internal/daemon/types.go index c819c7539..0d8e13c79 100644 --- a/server/internal/daemon/types.go +++ b/server/internal/daemon/types.go @@ -118,8 +118,8 @@ type Task struct { NewCommentCount int `json:"new_comment_count,omitempty"` // issue-wide comments since this agent's last run (excludes its own and the injected trigger); 0/omitted for old daemons or cold start NewCommentsSince string `json:"new_comments_since,omitempty"` // RFC3339 anchor (last run's started_at) the count is measured from; empty on cold start NewCommentsDeltaKnown bool `json:"new_comments_delta_known,omitempty"` // the server actually computed the issue-wide delta this claim (both reads succeeded). A zero NewCommentCount means "nothing was said" only when this is true; otherwise the zero is a failed read, a cold start, or an old server, and the prompt must not present it as the comment scan's answer (MUL-6984) - IssueStateDeltaKnown bool `json:"issue_state_delta_known,omitempty"` // MUL-7344: the server compared the issue's title/description/status against the snapshot taken at this agent's previous run on this issue. Same contract as NewCommentsDeltaKnown — absent means NOT compared (cold start, no prior snapshot, read error, old server), and the prompt must then keep telling the agent to read the issue - IssueChangedFields []string `json:"issue_changed_fields,omitempty"` // subset of title,description,status in that order; empty alongside IssueStateDeltaKnown means unchanged. Fields outside that set (assignee, priority, labels, parent, due, stage, project, metadata) are not compared and must never be reported as checked + IssueStateDeltaKnown bool `json:"issue_state_delta_known,omitempty"` // MUL-7344: the server compared the issue's title/description against the snapshot taken at this agent's previous run on this issue. Same contract as NewCommentsDeltaKnown — absent means NOT compared (cold start, no prior snapshot, read error, old server), and the prompt must then keep telling the agent to read the issue + IssueChangedFields []string `json:"issue_changed_fields,omitempty"` // subset of title,description in that order; empty alongside IssueStateDeltaKnown means unchanged. Fields outside that set (status, assignee, priority, labels, parent, due, stage, project, metadata) are not compared and must never be reported as checked; status and assignee ship their current values instead IssueStatus string `json:"issue_status,omitempty"` // the issue's status key at claim time; sent whether or not the delta is known IssueAssigneeType string `json:"issue_assignee_type,omitempty"` // "agent", "member" or "squad" at claim time; empty when unassigned IssueAssigneeID string `json:"issue_assignee_id,omitempty"` // assignee UUID at claim time; empty when unassigned diff --git a/server/internal/handler/agent.go b/server/internal/handler/agent.go index 882ee521e..4356e4aae 100644 --- a/server/internal/handler/agent.go +++ b/server/internal/handler/agent.go @@ -475,7 +475,7 @@ type AgentTaskResponse struct { NewCommentsDeltaKnown bool `json:"new_comments_delta_known,omitempty"` // IssueStateDeltaKnown is the same contract as NewCommentsDeltaKnown, for // the ISSUE record rather than its comments: the server compared this - // claim's title / description / status against the + // claim's title / description against the // snapshot taken when this agent last ran on this issue, and both the // lookup and the decode succeeded. Absent means NOT compared — a cold // start, no prior snapshot, a read error, a shape-version mismatch, or an @@ -483,7 +483,7 @@ type AgentTaskResponse struct { // issue. An empty IssueChangedFields is only "unchanged" alongside this // flag; on its own it is indistinguishable from "nobody looked" (MUL-7344). IssueStateDeltaKnown bool `json:"issue_state_delta_known,omitempty"` - IssueChangedFields []string `json:"issue_changed_fields,omitempty"` // subset of title,description,status in that order; empty alongside IssueStateDeltaKnown means unchanged. Fields outside that set (assignee, priority, labels, parent, due date, stage, project, metadata) are NOT compared and must never be reported as checked. Assignee is out because the agent only needs "is this mine now", which IssueAssigneeType/ID answer outright; priority is out because it does not change what the agent does + IssueChangedFields []string `json:"issue_changed_fields,omitempty"` // subset of title,description in that order; empty alongside IssueStateDeltaKnown means unchanged. Fields outside that set (status, assignee, priority, labels, parent, due date, stage, project, metadata) are NOT compared and must never be reported as checked. Status and assignee are out because IssueStatus / IssueAssigneeType / IssueAssigneeID ship their current values on every claim, so no comparison is needed to learn them; priority is out because it does not change what the agent does IssueStatus string `json:"issue_status,omitempty"` // the issue's status key at claim time. Sent whether or not the delta is known: the agent needs it to decide workflow step 3 ("already in progress?") without a read IssueAssigneeType string `json:"issue_assignee_type,omitempty"` // "agent", "member" or "squad" at claim time; empty when unassigned. With IssueAssigneeID, lets the agent tell "mine" from "someone else's" without a read IssueAssigneeID string `json:"issue_assignee_id,omitempty"` // assignee UUID at claim time; empty when unassigned diff --git a/server/internal/handler/daemon_issue_snapshot_test.go b/server/internal/handler/daemon_issue_snapshot_test.go index c848fe2ae..646f1b176 100644 --- a/server/internal/handler/daemon_issue_snapshot_test.go +++ b/server/internal/handler/daemon_issue_snapshot_test.go @@ -42,11 +42,10 @@ func seedPriorRunWithSnapshot(t *testing.T, agentID, runtimeID, issueID, snapsho // caller override individual fields to model what the previous run saw. // createClaimReclaimAgentAndIssue always creates an unassigned issue in // status in_progress with priority none, and titles it " issue". -func issueSnapshotJSON(t *testing.T, version int, title, description, status string) string { +func issueSnapshotJSON(t *testing.T, version int, title, description string) string { t.Helper() raw, err := json.Marshal(map[string]any{ "v": version, - "status": status, "title_sha256": sha256Hex(title), "description_sha256": sha256Hex(description), }) @@ -69,7 +68,7 @@ func TestClaimTaskByRuntime_IssueUnchangedReportsEmptyDelta(t *testing.T) { agentID, issueID := createClaimReclaimAgentAndIssue(t, ctx, runtimeID, name) seedPriorRunWithSnapshot(t, agentID, runtimeID, issueID, - issueSnapshotJSON(t, issueSnapshotVersion, name+" issue", "", "in_progress")) + issueSnapshotJSON(t, issueSnapshotVersion, name+" issue", "")) createCommentTriggeredClaimTask(t, ctx, agentID, runtimeID, issueID, nil) resp := claimCommentTask(t, runtimeID, "issue-snapshot-unchanged") @@ -96,18 +95,18 @@ func TestClaimTaskByRuntime_IssueChangedNamesFields(t *testing.T) { const name = "Issue snapshot changed agent" agentID, issueID := createClaimReclaimAgentAndIssue(t, ctx, runtimeID, name) - // The previous run saw a different description and a different status; - // title and priority are unchanged and must NOT be reported. + // The previous run saw a different description. Title is unchanged and must + // NOT be reported alongside it. seedPriorRunWithSnapshot(t, agentID, runtimeID, issueID, - issueSnapshotJSON(t, issueSnapshotVersion, name+" issue", "an older description", "todo")) + issueSnapshotJSON(t, issueSnapshotVersion, name+" issue", "an older description")) createCommentTriggeredClaimTask(t, ctx, agentID, runtimeID, issueID, nil) resp := claimCommentTask(t, runtimeID, "issue-snapshot-changed") if !resp.Task.IssueStateDeltaKnown { t.Fatalf("issue_state_delta_known must be true when the comparison ran") } - if got := strings.Join(resp.Task.IssueChangedFields, ","); got != "description,status" { - t.Errorf("issue_changed_fields = %q, want \"description,status\"", got) + if got := strings.Join(resp.Task.IssueChangedFields, ","); got != "description" { + t.Errorf("issue_changed_fields = %q, want \"description\"", got) } } @@ -171,7 +170,7 @@ func TestClaimTaskByRuntime_ForeignSnapshotVersionIsUnknown(t *testing.T) { // Byte-identical to the unchanged case apart from `v`. seedPriorRunWithSnapshot(t, agentID, runtimeID, issueID, - issueSnapshotJSON(t, issueSnapshotVersion+1, name+" issue", "", "in_progress")) + issueSnapshotJSON(t, issueSnapshotVersion+1, name+" issue", "")) createCommentTriggeredClaimTask(t, ctx, agentID, runtimeID, issueID, nil) resp := claimCommentTask(t, runtimeID, "issue-snapshot-version") @@ -211,9 +210,6 @@ func TestClaimTaskByRuntime_RecordsItsOwnIssueSnapshot(t *testing.T) { if !ok { t.Fatalf("claim did not persist a decodable issue snapshot, got %q", string(raw)) } - if stored.Status != "in_progress" { - t.Errorf("stored snapshot = %+v, want the issue's status", stored) - } if stored.TitleSHA256 != sha256Hex(name+" issue") { t.Errorf("stored title hash does not match the claimed issue's title") } @@ -239,7 +235,7 @@ func TestClaimTaskByRuntime_BothDeltasShareOneAnchor(t *testing.T) { // One prior run carries both halves of the anchor: its started_at dates the // comment delta, its snapshot dates the issue-state delta. seedPriorRunWithSnapshot(t, agentID, runtimeID, issueID, - issueSnapshotJSON(t, issueSnapshotVersion, name+" issue", "", "in_progress")) + issueSnapshotJSON(t, issueSnapshotVersion, name+" issue", "")) // A member comment after that anchor, so the comment delta is non-zero and // the two deltas cannot both be trivially empty. @@ -308,7 +304,7 @@ func TestClaimTaskByRuntime_DeltasDateFromTheResumedRun(t *testing.T) { "work_dir": "/tmp/resumed-anchor-workdir", "started_at": testutil.Raw("now() - interval '2 hours'"), "completed_at": testutil.Raw("now() - interval '110 minutes'"), - "issue_snapshot": issueSnapshotJSON(t, issueSnapshotVersion, name+" issue", "older instructions", "in_progress"), + "issue_snapshot": issueSnapshotJSON(t, issueSnapshotVersion, name+" issue", "older instructions"), }) // A NEWER run that saw the issue as it is now. Anchoring here is the // bug: its session is not the one being resumed. @@ -321,7 +317,7 @@ func TestClaimTaskByRuntime_DeltasDateFromTheResumedRun(t *testing.T) { "work_dir": "/tmp/resumed-anchor-other-workdir", "started_at": testutil.Raw("now() - interval '1 hour'"), "completed_at": testutil.Raw("now() - interval '50 minutes'"), - "issue_snapshot": issueSnapshotJSON(t, issueSnapshotVersion, name+" issue", "", "in_progress"), + "issue_snapshot": issueSnapshotJSON(t, issueSnapshotVersion, name+" issue", ""), } if manualRerun { // Nothing wrong with the newer run here; the operator simply @@ -398,7 +394,7 @@ func TestClaimTaskByRuntime_NeverStartedAnchorRowReportsNoIssueDelta(t *testing. "session_id": "never-started-anchor-session", "work_dir": "/tmp/never-started-anchor", "started_at": testutil.Raw("now() - interval '2 hours'"), "completed_at": testutil.Raw("now() - interval '110 minutes'"), - "issue_snapshot": issueSnapshotJSON(t, issueSnapshotVersion, name+" issue", "older instructions", "in_progress"), + "issue_snapshot": issueSnapshotJSON(t, issueSnapshotVersion, name+" issue", "older instructions"), }) // A retry child that inherited session S: claimed after the description // was edited to what it is now (so its snapshot matches the current issue), @@ -409,7 +405,7 @@ func TestClaimTaskByRuntime_NeverStartedAnchorRowReportsNoIssueDelta(t *testing. "session_id": "never-started-anchor-session", "work_dir": "/tmp/never-started-anchor", "dispatched_at": testutil.Raw("now() - interval '1 hour'"), "completed_at": testutil.Raw("now() - interval '50 minutes'"), - "issue_snapshot": issueSnapshotJSON(t, issueSnapshotVersion, name+" issue", "", "in_progress"), + "issue_snapshot": issueSnapshotJSON(t, issueSnapshotVersion, name+" issue", ""), }) triggerID := dbfx.Comment(t, issueID, "continue") dbfx.Task(t, agentID, testutil.Cols{"runtime_id": runtimeID, "issue_id": issueID, "trigger_comment_id": triggerID}) @@ -457,7 +453,7 @@ func TestClaimTaskByRuntime_NoAdoptedSessionHasNoDeltas(t *testing.T) { "session_id": "no-adopted-session-prior", "started_at": testutil.Raw("now() - interval '1 hour'"), "completed_at": testutil.Raw("now() - interval '50 minutes'"), - "issue_snapshot": issueSnapshotJSON(t, issueSnapshotVersion, name+" issue", "", "in_progress"), + "issue_snapshot": issueSnapshotJSON(t, issueSnapshotVersion, name+" issue", ""), } switch mode { case "missing_session": @@ -487,15 +483,19 @@ func TestClaimTaskByRuntime_NoAdoptedSessionHasNoDeltas(t *testing.T) { } } -// TestClaimTaskByRuntime_AssigneeAndPriorityAreOutOfScope pins the compared set. -// Reassigning an issue or changing its priority does not change what the agent -// has to read, so neither is compared — an issue whose ONLY change is one of -// them is reported as unchanged, and the prompt says which three fields were -// checked so that report cannot be read as "nothing about this issue moved". +// TestClaimTaskByRuntime_NonBodyFieldsAreOutOfScope pins the compared set. +// The comparison answers "must the agent re-read the issue BODY?", so status, +// assignee and priority are all outside it: an issue whose ONLY change is one +// of them is reported unchanged, and the prompt names the two fields it did +// check so that report cannot be read as "nothing about this issue moved". // -// The current assignee still ships on every claim; the agent answers "is this -// mine now" from that value, not from the comparison. -func TestClaimTaskByRuntime_AssigneeAndPriorityAreOutOfScope(t *testing.T) { +// Status matters most here. The workflow has the agent set in_progress and +// in_review on its own runs, so comparing status made the agent's own +// bookkeeping demand a body re-read that nothing in the body justified. +// +// Their CURRENT values still ship on every claim, which is why comparing them +// buys nothing: the agent reads status and assignee straight off the message. +func TestClaimTaskByRuntime_NonBodyFieldsAreOutOfScope(t *testing.T) { if testHandler == nil || testPool == nil { t.Skip("database not available") } @@ -504,12 +504,13 @@ func TestClaimTaskByRuntime_AssigneeAndPriorityAreOutOfScope(t *testing.T) { const name = "narrowed scope agent" agentID, issueID := createClaimReclaimAgentAndIssue(t, ctx, runtimeID, name) - // The prior run saw the three compared fields exactly as they are now. + // The prior run saw title and description exactly as they are now. The + // fixture creates the issue in_progress. seedPriorRunWithSnapshot(t, agentID, runtimeID, issueID, - issueSnapshotJSON(t, issueSnapshotVersion, name+" issue", "", "in_progress")) - // Since that run the issue was reassigned and re-prioritised. Neither is in - // the compared set. - dbfx.Exec(t, `UPDATE issue SET assignee_type = 'agent', assignee_id = $1, priority = 'urgent' WHERE id = $2`, + issueSnapshotJSON(t, issueSnapshotVersion, name+" issue", "")) + // Since that run the issue moved status, was reassigned, and was + // re-prioritised. None of the three is in the compared set. + dbfx.Exec(t, `UPDATE issue SET status = 'in_review', assignee_type = 'agent', assignee_id = $1, priority = 'urgent' WHERE id = $2`, agentID, issueID) createCommentTriggeredClaimTask(t, ctx, agentID, runtimeID, issueID, nil) @@ -518,10 +519,15 @@ func TestClaimTaskByRuntime_AssigneeAndPriorityAreOutOfScope(t *testing.T) { t.Fatalf("the comparison must still run") } if len(resp.Task.IssueChangedFields) != 0 { - t.Errorf("issue_changed_fields = %v, want empty — assignee and priority are out of the compared set", + t.Errorf("issue_changed_fields = %v, want empty — status, assignee and priority are all out of the compared set", resp.Task.IssueChangedFields) } - // The value the agent actually needs is still delivered. + // The values the agent actually needs are still delivered, and they are the + // NEW ones — without this the fixture's UPDATE could silently not apply and + // the emptiness above would prove nothing. + if resp.Task.IssueStatus != "in_review" { + t.Errorf("current status must still ship: got %q, want in_review", resp.Task.IssueStatus) + } if resp.Task.IssueAssigneeType != "agent" || resp.Task.IssueAssigneeID != agentID { t.Errorf("current assignee must still ship: type=%q id=%q, want agent/%s", resp.Task.IssueAssigneeType, resp.Task.IssueAssigneeID, agentID) @@ -566,7 +572,7 @@ func TestClaimTaskByRuntime_StartedRetryWithoutProviderMustNotWaiveReads(t *test "trigger_comment_id": triggerID, "started_at": testutil.Raw("now() - interval '2 hours'"), "completed_at": testutil.Raw("now() - interval '110 minutes'"), - "issue_snapshot": issueSnapshotJSON(t, issueSnapshotVersion, name+" issue", "old requirements", "in_progress"), + "issue_snapshot": issueSnapshotJSON(t, issueSnapshotVersion, name+" issue", "old requirements"), }) // A comment that session never saw. dbfx.Comment(t, issueID, "instructions the old session has never seen", testutil.Cols{ @@ -616,3 +622,58 @@ func TestClaimTaskByRuntime_StartedRetryWithoutProviderMustNotWaiveReads(t *test got.DeltaKnown, got.NewCommentCount, got.NewCommentsSince) } } + +// TestClaimTaskByRuntime_LegacySnapshotWithStatusStillCompares pins the reason +// dropping status did NOT bump issueSnapshotVersion. +// +// Rows written before the narrowing carry an extra "status" key. Removing a +// field from the compared set is backward compatible: the fields that remain +// are still present in those rows and still mean the same thing, so the +// comparison stays correct and the stale key is simply ignored. Only ADDING a +// field, or changing how a value is derived, leaves an old row unable to answer +// the current question — and only that earns a bump. +// +// Without this test the compatibility claim is an assertion in a comment, and +// the cost of it being wrong is a silent false "unchanged" on every dev row. +func TestClaimTaskByRuntime_LegacySnapshotWithStatusStillCompares(t *testing.T) { + if testHandler == nil || testPool == nil { + t.Skip("database not available") + } + for _, tc := range []struct { + name string + description string + wantChanged string + }{ + {name: "body unchanged", description: "", wantChanged: ""}, + {name: "description moved", description: "an older description", wantChanged: "description"}, + } { + t.Run(tc.name, func(t *testing.T) { + ctx := context.Background() + runtimeID := createClaimReclaimRuntime(t, ctx, "legacy snapshot runtime "+tc.name) + name := "legacy snapshot agent " + tc.name + agentID, issueID := createClaimReclaimAgentAndIssue(t, ctx, runtimeID, name) + + // A snapshot in the PREVIOUS shape: same version, extra status key. + legacy, err := json.Marshal(map[string]any{ + "v": issueSnapshotVersion, + "status": "todo", // the issue is in_progress now + "title_sha256": sha256Hex(name + " issue"), + "description_sha256": sha256Hex(tc.description), + }) + if err != nil { + t.Fatalf("marshal legacy snapshot: %v", err) + } + seedPriorRunWithSnapshot(t, agentID, runtimeID, issueID, string(legacy)) + createCommentTriggeredClaimTask(t, ctx, agentID, runtimeID, issueID, nil) + + resp := claimCommentTask(t, runtimeID, "legacy-snapshot-claim") + if !resp.Task.IssueStateDeltaKnown { + t.Fatalf("a legacy row must still be comparable, not reported as unknown") + } + if got := strings.Join(resp.Task.IssueChangedFields, ","); got != tc.wantChanged { + t.Errorf("issue_changed_fields = %q, want %q — the stale status key must be ignored, not compared", + got, tc.wantChanged) + } + }) + } +} diff --git a/server/internal/handler/issue_snapshot.go b/server/internal/handler/issue_snapshot.go index ac494577c..468049a51 100644 --- a/server/internal/handler/issue_snapshot.go +++ b/server/internal/handler/issue_snapshot.go @@ -11,49 +11,54 @@ import ( ) // issueSnapshotVersion is the shape version of the JSON stored in -// agent_task_queue.issue_snapshot. Bump it whenever the compared field SET -// changes — never when only a value's formatting changes, because a stored -// snapshot is compared against a freshly built one and a formatting drift -// inside the same version would read as a spurious "changed". +// agent_task_queue.issue_snapshot. // // A snapshot carrying any other version is treated as UNKNOWN rather than -// coerced: the two runs did not compare the same things, so neither -// "unchanged" nor a changed-field list would be true. Unknown degrades to the -// unconditional issue read, which is the behaviour that predates this column. +// coerced, which degrades to the unconditional issue read — the behaviour that +// predates this column. // -// This is version 1 — the first shape the column has ever held. The set below -// was narrowed during review, before any of this shipped, so no stored row has -// ever carried a wider one and there is nothing for a bump to protect. The -// version earns its place from here on, not retroactively. +// Bump it when a stored row can no longer answer the question the current code +// asks of it. In practice that is exactly two cases: +// +// - a field is ADDED to the compared set, because old rows do not carry it +// and their silence would read as "unchanged"; +// - the way a value is derived changes (a different hash, a different +// normalisation), because the same content would then compare as different. +// +// REMOVING a field does not qualify and must not bump: the remaining fields are +// still present in old rows and still mean the same thing, so the comparison +// stays correct and the extra key is simply ignored. Dropping status (MUL-7344 +// review) is that case, which is why this is still 1 even though rows written +// by the previous shape already exist. const issueSnapshotVersion = 1 // Compared field names, in the fixed order they are reported. // -// The set answers exactly one question — "must the agent run `issue get` -// again?" — so a field earns a place here only if changing it would alter what -// the agent does AND the per-turn message does not already carry its current -// value: +// The set answers exactly one question — "must the agent re-read the issue +// BODY?" — so only title and description qualify. They are the task itself, and +// nothing but a read can deliver them. // -// - title, description: the task itself, and reachable only by reading the -// issue. Both must be compared. -// - status: also shipped as a current value, so comparing it is strictly -// redundant for "what is it now". It stays because "changed: status" is the -// clearest available signal that somebody intervened between the runs — a -// push back from in_review to todo means the delivery was rejected — and -// carrying it costs nothing. -// - assignee: the agent only needs "is this mine now", which the current -// value answers outright. Not compared; still shipped. -// - priority: reachable only by reading, but it does not change what the -// agent does. Not compared. +// Everything else is excluded because the per-turn message already carries the +// current value, or because changing it does not change the agent's work: +// +// - status: shipped as a current value on every claim, so the agent learns it +// without a read. Comparing it was a mistake measured, not argued: the +// workflow has the agent set in_progress and in_review on its OWN runs, so +// its own bookkeeping counted as "changed" and reported a body re-read that +// nothing in the body justified. Replaying this issue's 21 follow-ups, that +// alone cut "unchanged" from 17 to 4. "Somebody intervened" is still +// visible — the agent compares the shipped status against what it last set. +// - assignee: shipped as a current value; "is this mine now" needs no compare. +// - priority: only a read reveals it, but it does not change what the agent +// does. // // The agent is told exactly this set was compared, so a field absent here must -// never be implied to have been checked: assignee, priority, labels, parent, -// due date, stage, project and metadata are all out of scope, and an issue -// whose ONLY change is one of them is reported as unchanged. +// never be implied to have been checked: status, assignee, priority, labels, +// parent, due date, stage, project and metadata are all out of scope, and an +// issue whose ONLY change is one of them is reported as unchanged. const ( issueFieldTitle = "title" issueFieldDescription = "description" - issueFieldStatus = "status" ) // issueStateSnapshot is the comparison key for one claim's view of an issue. @@ -61,14 +66,13 @@ const ( // Title and description are stored as SHA-256 hex, not as text: the column // exists to answer "did this move", and a second copy of every issue body in // the task queue would be both a storage cost and a place for issue text to -// leak from. Status is a short key, so it is stored raw. +// leak from. // // The claim's CURRENT status and assignee reach the agent as their own response -// fields, read straight off the issue row — they are not sourced from here, and -// narrowing this struct does not affect them. +// fields, read straight off the issue row — they were never sourced from here, +// so dropping status from the comparison did not affect them. type issueStateSnapshot struct { Version int `json:"v"` - Status string `json:"status"` TitleSHA256 string `json:"title_sha256"` DescriptionSHA256 string `json:"description_sha256"` } @@ -82,7 +86,6 @@ func sha256Hex(s string) string { func buildIssueStateSnapshot(issue db.Issue) issueStateSnapshot { return issueStateSnapshot{ Version: issueSnapshotVersion, - Status: issue.Status, TitleSHA256: sha256Hex(issue.Title), DescriptionSHA256: sha256Hex(issue.Description.String), } @@ -99,9 +102,6 @@ func (s issueStateSnapshot) changedFieldsSince(prev issueStateSnapshot) []string if s.DescriptionSHA256 != prev.DescriptionSHA256 { changed = append(changed, issueFieldDescription) } - if s.Status != prev.Status { - changed = append(changed, issueFieldStatus) - } return changed } diff --git a/server/migrations/499_agent_task_issue_snapshot.up.sql b/server/migrations/499_agent_task_issue_snapshot.up.sql index 2efc59c2b..e3e697766 100644 --- a/server/migrations/499_agent_task_issue_snapshot.up.sql +++ b/server/migrations/499_agent_task_issue_snapshot.up.sql @@ -17,18 +17,23 @@ -- would report "unchanged" forever with no symptom. A snapshot taken at claim -- time has exactly one writer, whatever the compared set happens to be. -- --- Shape is {"v":1,...}: version, status, and sha256 of title and description. --- Hashes, not bodies — this column is a comparison key, never a second copy of --- the issue text. `v` gates the comparison: a snapshot written by a different +-- Shape is {"v":1,...}: version plus sha256 of title and description. Hashes, +-- not bodies — this column is a comparison key, never a second copy of the +-- issue text. `v` gates the comparison: a snapshot written by a different -- version is treated as unknown, which degrades to the pre-existing "read the --- issue" instruction, so narrowing or widening the set later is always safe. +-- issue" instruction. -- --- The set is deliberately smaller than "the issue": it answers only "must the --- agent read the issue again?", so a field belongs here only if changing it --- alters what the agent does AND its current value is not already in the --- per-turn message. Assignee and priority fail that test — the claim ships the --- current assignee outright, and priority does not change the agent's work. --- Labels, parent, due date, stage, project and metadata are out too. +-- The set answers only "must the agent re-read the issue BODY?", so only title +-- and description qualify — they are the task itself and nothing but a read +-- delivers them. Status, assignee and priority are out: the claim ships the +-- current status and assignee on every response, so no comparison is needed to +-- learn them, and priority does not change the agent's work. Labels, parent, +-- due date, stage, project and metadata are out too. +-- +-- Rows written by an earlier shape of this column also carry a "status" key; +-- it is ignored. Removing a field from the set is backward compatible, so it +-- does not bump `v` — only adding one, or changing how a value is derived, +-- makes an old row unable to answer the current question. -- -- Nullable, and NULL is the normal state for every row written before this -- migration. A reader must treat NULL as "not compared", never as "unchanged".