fix: align task summary semantic equality (#3769)
This commit is contained in:
+120
@@ -0,0 +1,120 @@
|
||||
package sqlite
|
||||
|
||||
import (
|
||||
"context"
|
||||
"testing"
|
||||
"time"
|
||||
|
||||
"github.com/kandev/kandev/internal/common/logger"
|
||||
"github.com/kandev/kandev/internal/events"
|
||||
"github.com/kandev/kandev/internal/events/bus"
|
||||
"github.com/kandev/kandev/internal/task/models"
|
||||
"github.com/kandev/kandev/internal/task/statussummary"
|
||||
)
|
||||
|
||||
// @covers AC-PLATFORM-BOUNDED-TASK-STATUS-DELIVERY-001.2
|
||||
// @covers AC-PLATFORM-BOUNDED-TASK-STATUS-DELIVERY-001.3
|
||||
func TestTaskStatusSummaryProjectorPRNoopWithRetainedError(t *testing.T) {
|
||||
repo, db := newTaskStatusSummaryTestRepo(t)
|
||||
seedTaskForStatusSummary(t, db, "task-summary-pr-noop", "workspace-summary-pr-noop")
|
||||
eventBus := bus.NewMemoryEventBus(logger.Default())
|
||||
t.Cleanup(func() { eventBus.Close() })
|
||||
publicationCount := 0
|
||||
if _, err := eventBus.Subscribe(events.TaskStatusSummaryUpdated, func(context.Context, *bus.Event) error {
|
||||
publicationCount++
|
||||
return nil
|
||||
}); err != nil {
|
||||
t.Fatalf("subscribe to summary updates: %v", err)
|
||||
}
|
||||
|
||||
loader := func(context.Context, string) (statussummary.SessionObservationSnapshot, error) {
|
||||
return statussummary.SessionObservationSnapshot{
|
||||
ErrorsObserved: true,
|
||||
Sessions: []statussummary.RebuildSession{{
|
||||
ID: "session-pr-noop", State: "WAITING_FOR_INPUT", IsPrimary: true,
|
||||
ActiveError: &statussummary.ActiveErrorSummary{
|
||||
Scope: models.ErrorScopeSession, SessionID: "session-pr-noop",
|
||||
Stamp: "retained-error", OccurredAt: time.Date(2026, 9, 17, 12, 0, 0, 0, time.UTC),
|
||||
Preview: "agent failed",
|
||||
},
|
||||
}},
|
||||
}, nil
|
||||
}
|
||||
newProjector := func() *statussummary.Projector {
|
||||
return statussummary.NewProjector(statussummary.ProjectorConfig{
|
||||
Store: repo, EventBus: eventBus, LoadSessionObservations: loader,
|
||||
Now: func() time.Time { return time.Date(2026, 9, 17, 12, 1, 0, 0, time.UTC) },
|
||||
})
|
||||
}
|
||||
prEvent := func(checksState string, pendingReviewCount int) *bus.Event {
|
||||
return bus.NewEvent(events.GitHubTaskPRUpdated, "test", map[string]interface{}{
|
||||
"task_id": "task-summary-pr-noop", "workspace_id": "workspace-summary-pr-noop",
|
||||
"repository_id": "repo-a", "state": "open", "pr_number": 41,
|
||||
"pr_url": "https://example.test/41", "checks_state": checksState,
|
||||
"pending_review_count": pendingReviewCount,
|
||||
})
|
||||
}
|
||||
|
||||
projector := newProjector()
|
||||
if err := projector.HandleEvent(context.Background(), prEvent("failure", 1)); err != nil {
|
||||
t.Fatalf("initial PR projection: %v", err)
|
||||
}
|
||||
first, err := repo.LoadTaskStatusSummaries(context.Background(), []string{"task-summary-pr-noop"})
|
||||
if err != nil {
|
||||
t.Fatalf("load initial summary: %v", err)
|
||||
}
|
||||
if err := projector.HandleEvent(context.Background(), prEvent("failure", 2)); err != nil {
|
||||
t.Fatalf("aggregate-preserving PR update: %v", err)
|
||||
}
|
||||
second, err := repo.LoadTaskStatusSummaries(context.Background(), []string{"task-summary-pr-noop"})
|
||||
if err != nil {
|
||||
t.Fatalf("load no-op summary: %v", err)
|
||||
}
|
||||
if second["task-summary-pr-noop"].Revision != first["task-summary-pr-noop"].Revision {
|
||||
t.Fatalf("no-op changed revision from %d to %d", first["task-summary-pr-noop"].Revision, second["task-summary-pr-noop"].Revision)
|
||||
}
|
||||
if publicationCount != 1 {
|
||||
t.Fatalf("no-op publication count = %d, want initial publication only", publicationCount)
|
||||
}
|
||||
|
||||
recreated := newProjector()
|
||||
if err := recreated.HandleEvent(context.Background(), prEvent("failure", 3)); err != nil {
|
||||
t.Fatalf("recreated projector no-op: %v", err)
|
||||
}
|
||||
third, err := repo.LoadTaskStatusSummaries(context.Background(), []string{"task-summary-pr-noop"})
|
||||
if err != nil {
|
||||
t.Fatalf("load recreated no-op summary: %v", err)
|
||||
}
|
||||
if third["task-summary-pr-noop"].Revision != first["task-summary-pr-noop"].Revision {
|
||||
t.Fatalf("recreated no-op changed revision to %d", third["task-summary-pr-noop"].Revision)
|
||||
}
|
||||
if publicationCount != 1 {
|
||||
t.Fatalf("recreated no-op publication count = %d, want 1", publicationCount)
|
||||
}
|
||||
|
||||
if err := recreated.HandleEvent(context.Background(), prEvent("success", 0)); err != nil {
|
||||
t.Fatalf("real PR aggregate change: %v", err)
|
||||
}
|
||||
fourth, err := repo.LoadTaskStatusSummaries(context.Background(), []string{"task-summary-pr-noop"})
|
||||
if err != nil {
|
||||
t.Fatalf("load changed summary: %v", err)
|
||||
}
|
||||
if fourth["task-summary-pr-noop"].Revision != first["task-summary-pr-noop"].Revision+1 {
|
||||
t.Fatalf("real change revision = %d, want %d", fourth["task-summary-pr-noop"].Revision, first["task-summary-pr-noop"].Revision+1)
|
||||
}
|
||||
if fourth["task-summary-pr-noop"].PullRequest == nil || fourth["task-summary-pr-noop"].PullRequest.AggregateState != "passing" {
|
||||
t.Fatalf("real change PR summary = %+v, want passing aggregate", fourth["task-summary-pr-noop"].PullRequest)
|
||||
}
|
||||
if fourth["task-summary-pr-noop"].ActiveError == nil || fourth["task-summary-pr-noop"].ActiveError.Stamp != "retained-error" {
|
||||
t.Fatalf("real change dropped retained error: %+v", fourth["task-summary-pr-noop"].ActiveError)
|
||||
}
|
||||
if publicationCount != 2 {
|
||||
t.Fatalf("real change publication count = %d, want 2", publicationCount)
|
||||
}
|
||||
if err := recreated.HandleEvent(context.Background(), prEvent("success", 0)); err != nil {
|
||||
t.Fatalf("replayed real PR state: %v", err)
|
||||
}
|
||||
if publicationCount != 2 {
|
||||
t.Fatalf("replayed real change publication count = %d, want 2", publicationCount)
|
||||
}
|
||||
}
|
||||
@@ -4,9 +4,9 @@
|
||||
package statussummary
|
||||
|
||||
import (
|
||||
"bytes"
|
||||
"encoding/json"
|
||||
"fmt"
|
||||
"reflect"
|
||||
"slices"
|
||||
"time"
|
||||
"unicode/utf8"
|
||||
@@ -138,13 +138,12 @@ type StoredTaskStatusSummary struct {
|
||||
Summary TaskStatusSummary
|
||||
}
|
||||
|
||||
// SemanticEqual compares only fields that task consumers observe as status.
|
||||
// SemanticEqual compares valid summaries using the canonical payload stored by
|
||||
// the repository, excluding transport metadata and omitted zero values.
|
||||
func (s TaskStatusSummary) SemanticEqual(other TaskStatusSummary) bool {
|
||||
s.Revision = 0
|
||||
s.UpdatedAt = time.Time{}
|
||||
other.Revision = 0
|
||||
other.UpdatedAt = time.Time{}
|
||||
return reflect.DeepEqual(s, other)
|
||||
left, leftErr := s.SemanticJSON()
|
||||
right, rightErr := other.SemanticJSON()
|
||||
return leftErr == nil && rightErr == nil && bytes.Equal(left, right)
|
||||
}
|
||||
|
||||
// Validate enforces the bounded fields at the persistence boundary. Other
|
||||
|
||||
@@ -0,0 +1,83 @@
|
||||
package statussummary
|
||||
|
||||
import (
|
||||
"encoding/json"
|
||||
"strings"
|
||||
"testing"
|
||||
"time"
|
||||
|
||||
"github.com/kandev/kandev/internal/task/models"
|
||||
)
|
||||
|
||||
// @covers AC-PLATFORM-BOUNDED-TASK-STATUS-DELIVERY-001.2
|
||||
func TestTaskStatusSummarySemanticEqualityMatchesPersistence(t *testing.T) {
|
||||
occurredAt := time.Date(2026, 9, 17, 12, 0, 0, 0, time.UTC)
|
||||
base := TaskStatusSummary{
|
||||
Revision: 7,
|
||||
UpdatedAt: occurredAt.Add(time.Minute),
|
||||
ActiveError: &ActiveErrorSummary{
|
||||
Scope: models.ErrorScopeSession,
|
||||
SessionID: "session-1",
|
||||
Stamp: "error-1",
|
||||
OccurredAt: occurredAt,
|
||||
Preview: "agent failed",
|
||||
RecoveryActions: nil,
|
||||
Causes: nil,
|
||||
},
|
||||
TaskError: &ActiveErrorSummary{
|
||||
Scope: models.ErrorScopeTask,
|
||||
Stamp: "task-error-1",
|
||||
OccurredAt: occurredAt,
|
||||
Preview: "task failed",
|
||||
RecoveryActions: nil,
|
||||
Causes: nil,
|
||||
},
|
||||
}
|
||||
|
||||
withEmptyCollections := base
|
||||
activeEmpty := *base.ActiveError
|
||||
activeEmpty.RecoveryActions = []string{}
|
||||
activeEmpty.Causes = []models.AgentErrorCause{}
|
||||
taskEmpty := *base.TaskError
|
||||
taskEmpty.RecoveryActions = []string{}
|
||||
taskEmpty.Causes = []models.AgentErrorCause{}
|
||||
withEmptyCollections.ActiveError = &activeEmpty
|
||||
withEmptyCollections.TaskError = &taskEmpty
|
||||
withEmptyCollections.Revision = 99
|
||||
withEmptyCollections.UpdatedAt = occurredAt.Add(2 * time.Hour)
|
||||
if !base.SemanticEqual(withEmptyCollections) {
|
||||
t.Fatalf("nil and empty error collections must compare equal: %#v vs %#v", base, withEmptyCollections)
|
||||
}
|
||||
|
||||
encoded, err := base.SemanticJSON()
|
||||
if err != nil {
|
||||
t.Fatalf("encode base summary: %v", err)
|
||||
}
|
||||
var roundTrip TaskStatusSummary
|
||||
if err := unmarshalSummary(encoded, &roundTrip); err != nil {
|
||||
t.Fatalf("decode canonical summary: %v", err)
|
||||
}
|
||||
if !base.SemanticEqual(roundTrip) {
|
||||
t.Fatalf("canonical persistence round trip changed semantics: %#v", roundTrip)
|
||||
}
|
||||
|
||||
changed := base
|
||||
changed.PendingAction = pendingPermission
|
||||
if base.SemanticEqual(changed) {
|
||||
t.Fatal("a real semantic field change must not compare equal")
|
||||
}
|
||||
}
|
||||
|
||||
// @covers AC-PLATFORM-BOUNDED-TASK-STATUS-DELIVERY-001.2
|
||||
func TestTaskStatusSummarySemanticEqualityRejectsInvalidValues(t *testing.T) {
|
||||
invalid := TaskStatusSummary{
|
||||
ActiveError: &ActiveErrorSummary{Preview: strings.Repeat("x", MaxActiveErrorPreviewBytes+1)},
|
||||
}
|
||||
if invalid.SemanticEqual(invalid) {
|
||||
t.Fatal("invalid summaries must not be accepted as semantic no-ops")
|
||||
}
|
||||
}
|
||||
|
||||
func unmarshalSummary(data []byte, summary *TaskStatusSummary) error {
|
||||
return json.Unmarshal(data, summary)
|
||||
}
|
||||
@@ -45,8 +45,11 @@ func (s *projectorTestStore) CompareAndUpdateTaskStatusSummary(_ context.Context
|
||||
}
|
||||
s.mu.Lock()
|
||||
defer s.mu.Unlock()
|
||||
if previous := s.rows[stored.TaskID]; previous != nil && previous.Summary.Revision >= stored.Summary.Revision {
|
||||
return false, nil
|
||||
if previous := s.rows[stored.TaskID]; previous != nil {
|
||||
if previous.Summary.Revision >= stored.Summary.Revision ||
|
||||
(previous.WorkspaceID == stored.WorkspaceID && previous.Summary.SemanticEqual(stored.Summary)) {
|
||||
return false, nil
|
||||
}
|
||||
}
|
||||
copy := *stored
|
||||
copy.Summary = *cloneSummary(&stored.Summary)
|
||||
|
||||
@@ -0,0 +1,199 @@
|
||||
---
|
||||
created: 2026-09-17
|
||||
status: complete
|
||||
requirements:
|
||||
- REQ-PLATFORM-BOUNDED-TASK-STATUS-DELIVERY-001
|
||||
system_design:
|
||||
- ../../specs/platform/system-design/bounded-task-status-delivery.md
|
||||
legacy_specs: []
|
||||
---
|
||||
|
||||
# Implementation Plan: Task-summary semantic equality
|
||||
|
||||
## Overview
|
||||
|
||||
Stop false compare-and-set exhaustion errors when a source event changes a
|
||||
detail but leaves the bounded task summary unchanged. Align the projector's
|
||||
in-memory equality with its existing persisted semantic JSON, then prove the
|
||||
complete event-to-storage-to-publication path with a real SQLite repository.
|
||||
One sequential work order owns the correction and its regression coverage.
|
||||
|
||||
## Evidence and confirmed root cause
|
||||
|
||||
Investigation baseline: checkout `f32eb3737`, 2026-09-17. Today's retained
|
||||
backend files contained 31 `exhausted CAS retries refreshing pending task
|
||||
status` warnings paired with `github.task_pr.updated` handler errors between
|
||||
11:19 and 15:16 Lisbon time. At 15:16:47, the source changed review counts.
|
||||
Read-only inspection of three affected task summaries found retained session
|
||||
errors with omitted `recovery_actions` and `causes` fields. These database
|
||||
reads describe inspection-time state, not a historical snapshot.
|
||||
|
||||
The deterministic cause is independent of concurrent writers:
|
||||
|
||||
1. `restoreSessionObservations` / `normalizeRebuildError` rebuild error
|
||||
observations. `NormalizeRecoveryActionsForCategory` and
|
||||
`NormalizeAgentErrorCauses` can produce non-nil zero-length slices.
|
||||
2. `deriveActiveError` retains those slices. `cloneSummary` and database JSON
|
||||
round trips omit them through `omitempty` and restore nil slices.
|
||||
3. `TaskStatusSummary.SemanticEqual` uses `reflect.DeepEqual`, which treats
|
||||
those in-memory representations as different.
|
||||
4. A PR detail change, such as pending review count 1 to 2 while checks remain
|
||||
failing, changes the keyed observation but not the aggregate summary.
|
||||
5. The projector attempts a write. The repository correctly rejects it because
|
||||
`SemanticJSON` is identical. Reload/rebase restores the same mismatch, so
|
||||
all three retries fail and the handler returns an error.
|
||||
|
||||
A temporary test named `TestReproSummaryPRNoopWithRetainedError` drove
|
||||
`Projector.HandleEvent` with a real SQLite repository and synthetic data. It
|
||||
used one waiting primary session with an explicitly session-scoped retained
|
||||
error and two PR events with checks `failure`, changing pending review count
|
||||
from 1 to 2. The second event returned the exact exhaustion error. The before
|
||||
and after stored JSON were identical and both revisions were 1. No competing
|
||||
writer, running agent, external provider, or browser was involved.
|
||||
|
||||
The temporary reproduction was removed after investigation. Its permanent
|
||||
success assertion belongs to Task 01's red phase. An initial fixture omitted
|
||||
the error's session scope and therefore introduced a separate summary change;
|
||||
the final fixture explicitly sets scope and session identity.
|
||||
|
||||
This establishes false errors for aggregate-preserving events, not a confirmed
|
||||
loss of a meaningful PR status change. Existing projector tests missed it
|
||||
because `projectorTestStore` rejects old revisions but accepts identical
|
||||
semantic payloads with newer revisions, unlike the production repository.
|
||||
|
||||
## Requirement conformance and assumptions
|
||||
|
||||
Platform owns the bounded projection contract; Tasks and Integrations retain
|
||||
the authoritative source records. This is an implementation defect under
|
||||
[AC-PLATFORM-BOUNDED-TASK-STATUS-DELIVERY-001.2 and .3](../../specs/platform/requirements/bounded-task-status-delivery.md)
|
||||
and the design's existing semantic-no-op and persistence guarantees. Reuse the
|
||||
active requirement without adding a repair-specific requirement or new IDs.
|
||||
|
||||
The intended behavior is settled: unchanged projections succeed silently;
|
||||
real changes preserve monotonic revisions and workspace-scoped publication.
|
||||
No new product behavior, schema, ownership boundary, or ADR is needed. The
|
||||
design clarifies the shared equality rule. The accepted
|
||||
[summary boundary ADR](../../decisions/2026-08-01-separate-task-summary-session-stream-traffic.md)
|
||||
and [activity ADR](../../decisions/2026-08-17-separate-task-activity-from-summary-freshness.md)
|
||||
remain authoritative.
|
||||
|
||||
## Scope
|
||||
|
||||
### In scope
|
||||
|
||||
- Consistent equality for every valid bounded summary and its persisted form.
|
||||
- Nil/empty error collections and JSON round-trip equivalence.
|
||||
- Real SQLite projector regression, publication assertions, and faithful test
|
||||
store no-op behavior.
|
||||
- Preservation of genuine changes, validation failures, and contention errors.
|
||||
|
||||
### Out of scope
|
||||
|
||||
- Workflow import validation/input repair, task-deletion races, diff requests
|
||||
during teardown, authentication settings, and agent-provider errors.
|
||||
- Changes to error normalization throughout the model domain or error text.
|
||||
- Retry-count increases, suppressing all CAS warnings, database rewrites,
|
||||
schema migrations, new flags, and PR polling changes.
|
||||
- Frontend layout, copy, interaction, and status precedence changes.
|
||||
|
||||
## Technical approach
|
||||
|
||||
In `apps/backend/internal/task/statussummary/model.go`, implement valid-value
|
||||
equality using the canonical payload already provided by `SemanticJSON`.
|
||||
Compare successful encodings with `bytes.Equal`; if either encoding fails,
|
||||
return false so the projector continues into its existing validation/error
|
||||
path. Do not equate two encoding failures by comparing nil byte slices.
|
||||
This reuses the bounded persistence contract and automatically includes future
|
||||
semantic fields. Do not change the JSON shape or broaden timestamp equivalence
|
||||
beyond the existing serialized representation.
|
||||
|
||||
Keep `persistAndPublishLocked`, its reload/rebase behavior, and the repository
|
||||
SQL contract intact unless the regression demonstrates a necessary local
|
||||
adjustment. Correct `projectorTestStore.CompareAndUpdateTaskStatusSummary` in
|
||||
`projector_test.go` to reject an unchanged workspace and semantic payload as
|
||||
well as stale revisions. Do not relax unrelated test assertions to compensate.
|
||||
|
||||
Add focused model tests in `model_semantic_equality_test.go` and an integration
|
||||
test in `internal/task/repository/sqlite/task_status_summary_projector_noop_test.go`.
|
||||
Use the existing temporary-database helpers, synthetic identities, explicit
|
||||
session error scope, fixed UTC times, and synchronous event handling.
|
||||
The fixture must not open the developer's database or copy private log data.
|
||||
|
||||
## Tests
|
||||
|
||||
| Evidence | Acceptance mapping |
|
||||
| --- | --- |
|
||||
| `TestTaskStatusSummarySemanticEqualityMatchesPersistence`: nil/empty collections independently for active/task errors, JSON round trips, timestamp monotonic metadata, ignored revision/update time, and real semantic differences | AC-PLATFORM-BOUNDED-TASK-STATUS-DELIVERY-001.2 |
|
||||
| `TestTaskStatusSummarySemanticEqualityRejectsInvalidValues`: invalid summaries cannot be classified as successful no-ops | AC-PLATFORM-BOUNDED-TASK-STATUS-DELIVERY-001.2 |
|
||||
| `TestTaskStatusSummaryProjectorPRNoopWithRetainedError`: live and recreated projectors, real SQLite, unchanged payload/revision, zero extra publications, no exhaustion warning | AC-PLATFORM-BOUNDED-TASK-STATUS-DELIVERY-001.2, AC-PLATFORM-BOUNDED-TASK-STATUS-DELIVERY-001.3 |
|
||||
| The real-change portion of `TestTaskStatusSummaryProjectorPRNoopWithRetainedError`: later aggregate change persists once and publishes the complete next revision while preserving error and unrelated fields | AC-PLATFORM-BOUNDED-TASK-STATUS-DELIVERY-001.2, AC-PLATFORM-BOUNDED-TASK-STATUS-DELIVERY-001.3 |
|
||||
| Existing pending-authority and queue contention tests with the corrected test store | AC-PLATFORM-BOUNDED-TASK-STATUS-DELIVERY-001.2 |
|
||||
|
||||
## E2E evidence
|
||||
|
||||
The real-repository integration tests cover source event, normalization,
|
||||
projection, SQL persistence/reload, and complete workspace-summary publication.
|
||||
They exercise the shared payload consumed by desktop and mobile. No rendered
|
||||
UI changes or new browser test are needed for this internal no-op correction.
|
||||
|
||||
## Work orders
|
||||
|
||||
- [x] [Task 01: Align semantic equality and prove persistence behavior](task-01-align-semantic-equality.md)
|
||||
|
||||
Execution is sequential. No subagents are authorized or required.
|
||||
|
||||
## Related implementation records
|
||||
|
||||
The linked packages `bounded-task-status-delivery` (implemented),
|
||||
`session-stream-overload-isolation` (complete), `backend-runtime-state-ownership`
|
||||
(completed), and `deleted-session-error-summary` (done) were inventoried.
|
||||
This repair does not reopen their work orders or alter their E2E matrices;
|
||||
their historical verification results remain unchanged.
|
||||
|
||||
## Verification results
|
||||
|
||||
Investigation: the final temporary regression failed as expected with three
|
||||
CAS retries, identical persisted JSON, and unchanged revision 1:
|
||||
|
||||
```bash
|
||||
(cd apps/backend && go test ./internal/task/repository/sqlite -run '^TestReproSummaryPRNoopWithRetainedError$' -count=1 -v)
|
||||
```
|
||||
|
||||
Implementation and permanent regression verification passed on 2026-09-17:
|
||||
|
||||
```text
|
||||
(cd apps/backend && go test ./internal/task/statussummary ./internal/task/repository/sqlite \
|
||||
-run 'TestTaskStatusSummarySemanticEquality|TestTaskStatusSummaryProjectorPRNoopWithRetainedError' -count=1)
|
||||
2 packages passed.
|
||||
|
||||
(cd apps/backend && go test -race ./internal/task/statussummary ./internal/task/repository/sqlite -count=1)
|
||||
2 packages passed.
|
||||
```
|
||||
|
||||
The production equality path now uses the same canonical semantic JSON as the
|
||||
repository. The SQLite projector regression proves no-op events keep the same
|
||||
revision and publication count, recreated projectors converge, and a real PR
|
||||
aggregate change advances once while preserving the retained error.
|
||||
Design-package checks passed on 2026-09-17:
|
||||
|
||||
- `python3 scripts/list-docs.py validate`: 285 decisions and 986 specifications.
|
||||
- `python3 scripts/lint-spec-files.py --all`: all specification files passed.
|
||||
- `python3 scripts/lint-spec-files.test.py`: 36 tests passed.
|
||||
- `git diff --check`: passed.
|
||||
- `git status --short -- docs/plans/task-summary-semantic-equality`: confirmed
|
||||
the new package is present and untracked; it contains this plan and Task 01.
|
||||
|
||||
The implementation is complete in Task 01. Commit and PR delivery evidence is
|
||||
recorded outside this plan.
|
||||
|
||||
## Risks
|
||||
|
||||
- Comparing canonical JSON allocates bounded payloads; keep it on the existing
|
||||
semantic-event path and do not introduce transcript or polling work.
|
||||
- A faithful test store may expose pre-existing tests that rely on artificial
|
||||
revision bumps. Preserve their behavioral intent and diagnose each failure.
|
||||
- A no-op fix must not hide real field changes, invalid values, store failures,
|
||||
or genuine contention exhaustion.
|
||||
- The SQLite reproduction establishes this cause but does not prove every
|
||||
historical CAS error had the same cause. No SQL changes are planned; if that
|
||||
scope changes, add the repository-required PostgreSQL behavioral coverage.
|
||||
@@ -0,0 +1,166 @@
|
||||
---
|
||||
id: "01-align-semantic-equality"
|
||||
title: "Align semantic equality and prove persistence behavior"
|
||||
status: done
|
||||
wave: 1
|
||||
depends_on: []
|
||||
plan: "plan.md"
|
||||
requirements:
|
||||
- REQ-PLATFORM-BOUNDED-TASK-STATUS-DELIVERY-001
|
||||
acceptance_criteria:
|
||||
- AC-PLATFORM-BOUNDED-TASK-STATUS-DELIVERY-001.2
|
||||
- AC-PLATFORM-BOUNDED-TASK-STATUS-DELIVERY-001.3
|
||||
system_design:
|
||||
- ../../specs/platform/system-design/bounded-task-status-delivery.md
|
||||
---
|
||||
|
||||
# Task 01: Align semantic equality and prove persistence behavior
|
||||
|
||||
## Summary
|
||||
|
||||
Make valid summary equality agree with persisted canonical JSON. Prove that a
|
||||
PR detail change with an unchanged aggregate and a retained session error is
|
||||
a successful no-op, while a later real change still persists and publishes.
|
||||
|
||||
## In scope
|
||||
|
||||
- Add model and real-SQLite regression tests before production edits.
|
||||
- Replace representation-sensitive equality with successful canonical-payload
|
||||
comparison, preserving the existing validation/error path.
|
||||
- Match the test store's rejection semantics to the production repository.
|
||||
- Record red/green command results and update the plan/work-order status.
|
||||
|
||||
## Out of scope
|
||||
|
||||
All exclusions in [the plan](plan.md#scope), especially repository SQL changes,
|
||||
global normalization changes, UI work, log-level suppression, and retry tuning.
|
||||
|
||||
## Acceptance
|
||||
|
||||
1. Every tested valid summary compares equal to its canonical JSON round trip,
|
||||
including nil/empty error actions and causes. Metadata-only differences
|
||||
are equal; real semantic differences are unequal. Failed encodings are
|
||||
never accepted as equality.
|
||||
2. Against real SQLite, both live and recreated projectors process an
|
||||
aggregate-preserving PR change with no handler error, exhaustion warning,
|
||||
revision change, or extra summary publication. Preserve unrelated fields.
|
||||
3. A subsequent true aggregate change advances exactly one revision and
|
||||
publishes exactly one complete replacement with task/workspace identity.
|
||||
Existing store-error and contention behavior stays observable and bounded.
|
||||
|
||||
## TDD sequence and regression construction
|
||||
|
||||
1. Mark this work order `in_progress`. Add
|
||||
`TestTaskStatusSummaryProjectorPRNoopWithRetainedError` using
|
||||
`newTaskStatusSummaryTestRepo` and `seedTaskForStatusSummary`.
|
||||
2. Configure `LoadSessionObservations` with one primary waiting session,
|
||||
`ErrorsObserved: true`, and an error with `Scope: "session"`, matching
|
||||
`SessionID`, fixed UTC `OccurredAt`, stamp, and preview. Leave optional
|
||||
collections absent so production normalization creates the relevant shape.
|
||||
3. Call `Projector.HandleEvent` with `GitHubTaskPRUpdated`, fixed task/workspace
|
||||
and repository identity, one open PR, `checks_state: "failure"`, and
|
||||
`pending_review_count: 1`. Load the first summary, then send count 2.
|
||||
The unchanged `failure` aggregate must succeed; current code instead returns
|
||||
`exhausted CAS retries ... after 3 attempts`. Assert payload and revision
|
||||
as well as event counts. Capture the projector logger for the warning check.
|
||||
4. Include a recreated-projector case using the same temporary repository and
|
||||
authoritative session/PR loaders. Keep loader keys aligned with event keys.
|
||||
Register bus/logger/database cleanup immediately. No sleeps are needed.
|
||||
5. Add model cases named in the plan. Independently exercise empty actions and
|
||||
empty causes in both `ActiveError` and `TaskError`, plus valid populated
|
||||
changes, removal, queue count, Git/PR state, and last activity changes.
|
||||
6. Implement canonical equality and faithful fake-store semantics. Never
|
||||
return equality when either `SemanticJSON` call fails. Do not swallow CAS
|
||||
exhaustion or increase its attempt count.
|
||||
7. In `TestTaskStatusSummaryProjectorPRNoopWithRetainedError`, after the
|
||||
no-op cases, change aggregate status to a distinct state, assert one
|
||||
write/event, then replay and assert no extra event. Verify the retained
|
||||
error and complete replacement survive. Run the targeted packages and
|
||||
record results.
|
||||
|
||||
## Verification
|
||||
|
||||
Run from the repository root. The first command is the red test before the
|
||||
production edit and must be rerun green afterward. The second covers every
|
||||
changed suite and the existing contention regressions.
|
||||
|
||||
```bash
|
||||
(cd apps/backend && go test ./internal/task/repository/sqlite -run '^TestTaskStatusSummaryProjectorPRNoopWithRetainedError$' -count=1 -v)
|
||||
(cd apps/backend && go test -race ./internal/task/statussummary ./internal/task/repository/sqlite -count=1)
|
||||
python3 scripts/list-docs.py validate
|
||||
python3 scripts/lint-spec-files.py --all
|
||||
git diff --check
|
||||
git status --short -- docs/plans/task-summary-semantic-equality
|
||||
```
|
||||
|
||||
Do not claim PostgreSQL was exercised by the SQLite suite. There is no SQL or
|
||||
dialect change in this scope; if one becomes necessary, reconcile the package
|
||||
and add the required real PostgreSQL check before implementing it.
|
||||
|
||||
## Files likely touched
|
||||
|
||||
- `apps/backend/internal/task/statussummary/model.go`
|
||||
- `apps/backend/internal/task/statussummary/model_semantic_equality_test.go` (new)
|
||||
- `apps/backend/internal/task/statussummary/projector_test.go` (test-store helper)
|
||||
- `apps/backend/internal/task/repository/sqlite/task_status_summary_projector_noop_test.go` (new)
|
||||
- This work order and `plan.md` (status and results).
|
||||
|
||||
Read-only integration points: `projector.go`, `projector_events.go`,
|
||||
`projector_derive.go`, `rebuild.go`, and `repository/sqlite/task_status_summary.go`.
|
||||
|
||||
## Dependencies
|
||||
|
||||
None.
|
||||
|
||||
## Risks
|
||||
|
||||
Invalid data must reach existing validation rather than being hidden by failed
|
||||
serialization. The revised fake store can reveal tests that previously assumed
|
||||
every higher revision was accepted. Diagnose those assumptions rather than
|
||||
weakening the persistence contract.
|
||||
|
||||
## Parallelism
|
||||
|
||||
`sequential`
|
||||
|
||||
## Inputs
|
||||
|
||||
- [Plan and reproduction evidence](plan.md).
|
||||
- [Requirements](../../specs/platform/requirements/bounded-task-status-delivery.md), AC .2 and .3.
|
||||
- [Design](../../specs/platform/system-design/bounded-task-status-delivery.md), derivation and persistence guarantees.
|
||||
- Existing `model_test.go`, `projector_pending_authority_test.go`,
|
||||
`projector_queued_test.go`, and `task_status_summary_test.go` patterns.
|
||||
- `.agents/skills/tdd/references/backend-tests.md`.
|
||||
|
||||
## Results
|
||||
|
||||
Implemented on 2026-09-17.
|
||||
|
||||
- `TaskStatusSummary.SemanticEqual` compares successful canonical semantic JSON
|
||||
encodings and rejects invalid summaries as equal.
|
||||
- The projector test store now models both production rejection conditions:
|
||||
stale revisions and unchanged workspace/semantic payloads.
|
||||
- Added model coverage for nil/empty omitted collections, metadata-only changes,
|
||||
real semantic changes, JSON round trips, and invalid values.
|
||||
- Added a real SQLite projector regression covering live and recreated no-op PR
|
||||
updates and one true aggregate change.
|
||||
|
||||
Verification passed:
|
||||
|
||||
```text
|
||||
(cd apps/backend && go test ./internal/task/statussummary ./internal/task/repository/sqlite \
|
||||
-run 'TestTaskStatusSummarySemanticEquality|TestTaskStatusSummaryProjectorPRNoopWithRetainedError' -count=1)
|
||||
2 packages passed.
|
||||
|
||||
(cd apps/backend && go test -race ./internal/task/statussummary ./internal/task/repository/sqlite -count=1)
|
||||
2 packages passed.
|
||||
|
||||
python3 scripts/list-docs.py validate
|
||||
Validated 285 decisions and 986 specifications.
|
||||
|
||||
python3 scripts/lint-spec-files.py --all
|
||||
All specification files passed.
|
||||
|
||||
git diff --check
|
||||
passed.
|
||||
```
|
||||
@@ -4,7 +4,7 @@ system: platform
|
||||
requirements:
|
||||
- REQ-PLATFORM-BOUNDED-TASK-STATUS-DELIVERY-001
|
||||
created: 2026-08-01
|
||||
updated: 2026-08-29
|
||||
updated: 2026-09-17
|
||||
owners:
|
||||
- kandev
|
||||
---
|
||||
@@ -142,6 +142,12 @@ remain during migration, but switchers use the summary when present.
|
||||
Integration option updates refresh the authoritative keyed pull-request
|
||||
observations through the existing task-summary projector.
|
||||
- A semantic no-op does not increment `revision` or emit an update.
|
||||
- For valid summaries, `TaskStatusSummary.SemanticEqual` uses the same
|
||||
canonical semantic payload as `SemanticJSON` and the repository's no-op
|
||||
comparison. Nil and empty omitted error collections are equivalent;
|
||||
Go-only timestamp metadata does not create a change when serialized values
|
||||
are identical. Revision and projection time remain excluded. Invalid values
|
||||
must not short-circuit validation as successful no-ops.
|
||||
- Clients ignore a summary delta whose revision is not newer than the stored
|
||||
revision.
|
||||
- `last_activity_at` is separate from projection freshness. Task creation,
|
||||
@@ -189,6 +195,12 @@ remain during migration, but switchers use the summary when present.
|
||||
- Summary persistence and revision changes are serialized per task. A
|
||||
compare-and-update or equivalent transaction prevents concurrent source
|
||||
events from publishing duplicate revisions or losing a newer value.
|
||||
- `CompareAndUpdateTaskStatusSummary` can return false for a stale revision
|
||||
or an unchanged semantic payload. After reloading a rejected write, the
|
||||
projector rebases authoritative observations and compares again using the
|
||||
same payload semantics. An already-current value succeeds without another
|
||||
write or publication; genuine contention retains the bounded retry and
|
||||
error path. Test stores must preserve both repository rejection conditions.
|
||||
- Missing rows are rebuilt from authoritative records. List and boot loaders
|
||||
batch summary reads and may batch repairs; they do not perform an N+1 query.
|
||||
- Existing rows are repaired after startup recovery changes authoritative session state. Missing-row
|
||||
@@ -405,3 +417,5 @@ intermediate replacement.
|
||||
[`../../plans/backend-runtime-state-ownership/plan.md`](../../../plans/backend-runtime-state-ownership/plan.md)
|
||||
- Deleted-session error repair:
|
||||
[`../../plans/deleted-session-error-summary/plan.md`](../../../plans/deleted-session-error-summary/plan.md)
|
||||
- Semantic no-op equality repair:
|
||||
[Task-summary semantic equality](../../../plans/task-summary-semantic-equality/plan.md)
|
||||
|
||||
Reference in New Issue
Block a user