From 530631580636094cd809311147d798a1c76a070a Mon Sep 17 00:00:00 2001 From: Cistern Agent Date: Mon, 13 Apr 2026 22:50:39 -0600 Subject: [PATCH 1/7] sc-kcdwu: activate sharding pipeline - wire DurationStore, add GetByTeamAndTest, add tests - Wire DurationStore.UpsertFromResults into ReportsHandler.Create (inside transaction) - Add DurationStore.GetByTeamAndTest with targeted WHERE test_name = query - Update ShardingHandler.GetDuration to use GetByTeamAndTest instead of O(N) Go filter - Refactor UpsertFromResults to accept DBTX interface (pgxpool.Pool or pgx.Tx) - Add DurationStore field to ReportsHandler, wire in routes.go - Add comprehensive DurationStore integration tests (14 tests) - Add test_duration_history to integration test truncation list - Verified triage_status column IS wired (triage job writes it) - no migration needed --- internal/handler/reports.go | 4 + internal/handler/sharding.go | 19 +- internal/integration/testdb.go | 1 + internal/server/routes.go | 1 + internal/store/durations.go | 76 +++-- internal/store/durations_test.go | 561 +++++++++++++++++++++++++++++++ internal/store/reports.go | 8 + 7 files changed, 635 insertions(+), 35 deletions(-) create mode 100644 internal/store/durations_test.go diff --git a/internal/handler/reports.go b/internal/handler/reports.go index 3f67b813..057babf0 100644 --- a/internal/handler/reports.go +++ b/internal/handler/reports.go @@ -57,6 +57,7 @@ type ReportsHandler struct { BaseURL string TriageStore triageAccessor TriageEnqueuer triage.Enqueuer + DurationStore *store.DurationStore AllowBackdate bool } @@ -200,6 +201,7 @@ func (h *ReportsHandler) Create(w http.ResponseWriter, r *http.Request) { Raw: rawJSON, CreatedAt: now, TriageGitHubStatus: triageGitHubStatus, + DurationStore: h.DurationStore, } if err := h.ReportStore.CreateWithResults(r.Context(), params, results); err != nil { if err == pgx.ErrNoRows { @@ -210,6 +212,8 @@ func (h *ReportsHandler) Create(w http.ResponseWriter, r *http.Request) { return } + // Enqueue async triage — non-blocking, best-effort. Must be called after + // the transaction commits so the triage job can read the persisted rows. if h.TriageEnqueuer != nil { h.TriageEnqueuer.Enqueue(claims.TeamID, reportID) } diff --git a/internal/handler/sharding.go b/internal/handler/sharding.go index 595ffd5c..003dbbec 100644 --- a/internal/handler/sharding.go +++ b/internal/handler/sharding.go @@ -71,7 +71,7 @@ func (h *ShardingHandler) CreatePlan(w http.ResponseWriter, r *http.Request) { // RebalanceRequest is the request body for rebalancing after worker failure. type RebalanceRequest struct { ExecutionID string `json:"execution_id" validate:"required"` - FailedWorkerID string `json:"failed_worker_id" validate:"required"` + FailedWorkerID string `json:"failed_worker_id" validate:"required"` CurrentPlan model.ShardPlan `json:"current_plan" validate:"required"` CompletedTests []string `json:"completed_tests"` } @@ -173,18 +173,21 @@ func (h *ShardingHandler) GetDuration(w http.ResponseWriter, r *http.Request) { return } - durations, err := h.DurationStore.GetByTeam(r.Context(), claims.TeamID) + durations, err := h.DurationStore.GetByTeamAndTest(r.Context(), claims.TeamID, testName) if err != nil { Error(w, http.StatusInternalServerError, "failed to query durations") return } - for _, d := range durations { - if d.TestName == testName { - JSON(w, http.StatusOK, d) - return - } + if len(durations) == 0 { + Error(w, http.StatusNotFound, "no duration history for test") + return + } + + if len(durations) == 1 { + JSON(w, http.StatusOK, durations[0]) + return } - Error(w, http.StatusNotFound, "no duration history for test") + JSON(w, http.StatusOK, durations) } diff --git a/internal/integration/testdb.go b/internal/integration/testdb.go index 7b8783f3..1542d8f3 100644 --- a/internal/integration/testdb.go +++ b/internal/integration/testdb.go @@ -97,6 +97,7 @@ var truncateTables = []string{ "triage_failure_classifications", "triage_clusters", "triage_results", + "test_duration_history", "test_results", "test_reports", "test_executions", diff --git a/internal/server/routes.go b/internal/server/routes.go index 5df650cc..94ca6729 100644 --- a/internal/server/routes.go +++ b/internal/server/routes.go @@ -177,6 +177,7 @@ func NewRouter(cfg *config.Config, pool ...*db.Pool) http.Handler { GitHubStatusPoster: ghClient, BaseURL: cfg.BaseURL, TriageEnqueuer: triageEnqueuer, + DurationStore: durStore, AllowBackdate: cfg.DisableRateLimit, } if dbPool != nil { diff --git a/internal/store/durations.go b/internal/store/durations.go index 021ecb18..f0a52c30 100644 --- a/internal/store/durations.go +++ b/internal/store/durations.go @@ -4,11 +4,20 @@ import ( "context" "fmt" + "github.com/jackc/pgx/v5" + "github.com/jackc/pgx/v5/pgconn" "github.com/jackc/pgx/v5/pgxpool" "github.com/scaledtest/scaledtest/internal/model" ) +// DBTX is the common interface satisfied by pgxpool.Pool and pgx.Tx. +type DBTX interface { + Exec(ctx context.Context, sql string, arguments ...interface{}) (pgconn.CommandTag, error) + Query(ctx context.Context, sql string, optionsAndArgs ...interface{}) (pgx.Rows, error) + QueryRow(ctx context.Context, sql string, optionsAndArgs ...interface{}) pgx.Row +} + // DurationStore handles test duration history persistence. type DurationStore struct { pool *pgxpool.Pool @@ -21,30 +30,22 @@ func NewDurationStore(pool *pgxpool.Pool) *DurationStore { // GetByTeam returns all duration history entries for a team. func (s *DurationStore) GetByTeam(ctx context.Context, teamID string) ([]model.TestDurationHistory, error) { - rows, err := s.pool.Query(ctx, + return queryDurations(ctx, s.pool, `SELECT id, team_id, test_name, suite, avg_duration_ms, p95_duration_ms, min_duration_ms, max_duration_ms, run_count, last_status, updated_at, created_at FROM test_duration_history WHERE team_id = $1 ORDER BY test_name`, teamID) - if err != nil { - return nil, fmt.Errorf("query duration history: %w", err) - } - defer rows.Close() +} - var results []model.TestDurationHistory - for rows.Next() { - var d model.TestDurationHistory - if err := rows.Scan( - &d.ID, &d.TeamID, &d.TestName, &d.Suite, - &d.AvgDurationMs, &d.P95DurationMs, &d.MinDurationMs, &d.MaxDurationMs, - &d.RunCount, &d.LastStatus, &d.UpdatedAt, &d.CreatedAt, - ); err != nil { - return nil, fmt.Errorf("scan duration history: %w", err) - } - results = append(results, d) - } - return results, rows.Err() +// GetByTeamAndTest returns duration history entries for a specific test name within a team. +func (s *DurationStore) GetByTeamAndTest(ctx context.Context, teamID, testName string) ([]model.TestDurationHistory, error) { + return queryDurations(ctx, s.pool, + `SELECT id, team_id, test_name, suite, avg_duration_ms, p95_duration_ms, + min_duration_ms, max_duration_ms, run_count, last_status, updated_at, created_at + FROM test_duration_history + WHERE team_id = $1 AND test_name = $2 + ORDER BY suite`, teamID, testName) } // GetByTeamMap returns duration history as a map keyed by test_name. @@ -62,16 +63,30 @@ func (s *DurationStore) GetByTeamMap(ctx context.Context, teamID string) (map[st // UpsertFromResults updates duration history from a set of test results. // Uses a rolling average: new_avg = ((old_avg * old_count) + new_duration) / (old_count + 1). -func (s *DurationStore) UpsertFromResults(ctx context.Context, teamID string, results []model.TestResult) error { +// When db is a pgx.Tx the caller is responsible for committing the transaction. +// When db is a pgxpool.Pool this method creates and commits its own transaction. +func (s *DurationStore) UpsertFromResults(ctx context.Context, teamID string, results []model.TestResult, db DBTX) error { if len(results) == 0 { return nil } - tx, err := s.pool.Begin(ctx) - if err != nil { - return fmt.Errorf("begin tx: %w", err) + shouldCommit := false + var tx pgx.Tx + var err error + + switch d := db.(type) { + case *pgxpool.Pool: + tx, err = d.Begin(ctx) + if err != nil { + return fmt.Errorf("begin tx: %w", err) + } + defer tx.Rollback(ctx) + shouldCommit = true + case pgx.Tx: + tx = d + default: + return fmt.Errorf("UpsertFromResults: unsupported DBTX type %T", db) } - defer tx.Rollback(ctx) for _, r := range results { _, err := tx.Exec(ctx, @@ -93,19 +108,26 @@ func (s *DurationStore) UpsertFromResults(ctx context.Context, teamID string, re } } - return tx.Commit(ctx) + if shouldCommit { + return tx.Commit(ctx) + } + return nil } // GetBySuite returns duration history for tests in a specific suite. func (s *DurationStore) GetBySuite(ctx context.Context, teamID, suite string) ([]model.TestDurationHistory, error) { - rows, err := s.pool.Query(ctx, + return queryDurations(ctx, s.pool, `SELECT id, team_id, test_name, suite, avg_duration_ms, p95_duration_ms, min_duration_ms, max_duration_ms, run_count, last_status, updated_at, created_at FROM test_duration_history WHERE team_id = $1 AND suite = $2 ORDER BY avg_duration_ms DESC`, teamID, suite) +} + +func queryDurations(ctx context.Context, db DBTX, query string, args ...interface{}) ([]model.TestDurationHistory, error) { + rows, err := db.Query(ctx, query, args...) if err != nil { - return nil, fmt.Errorf("query duration by suite: %w", err) + return nil, fmt.Errorf("query duration history: %w", err) } defer rows.Close() @@ -117,7 +139,7 @@ func (s *DurationStore) GetBySuite(ctx context.Context, teamID, suite string) ([ &d.AvgDurationMs, &d.P95DurationMs, &d.MinDurationMs, &d.MaxDurationMs, &d.RunCount, &d.LastStatus, &d.UpdatedAt, &d.CreatedAt, ); err != nil { - return nil, fmt.Errorf("scan duration: %w", err) + return nil, fmt.Errorf("scan duration history: %w", err) } results = append(results, d) } diff --git a/internal/store/durations_test.go b/internal/store/durations_test.go new file mode 100644 index 00000000..02101358 --- /dev/null +++ b/internal/store/durations_test.go @@ -0,0 +1,561 @@ +//go:build integration + +package store_test + +import ( + "context" + "testing" + "time" + + "github.com/scaledtest/scaledtest/internal/integration" + "github.com/scaledtest/scaledtest/internal/model" + "github.com/scaledtest/scaledtest/internal/store" +) + +func insertDurationReport(t *testing.T, tdb *integration.TestDB, teamID string) string { + t.Helper() + ctx := context.Background() + var id string + err := tdb.Pool.QueryRow(ctx, + `INSERT INTO test_reports (team_id, tool_name, summary, raw, created_at) + VALUES ($1, 'jest', '{}', '{}', now()) RETURNING id`, + teamID, + ).Scan(&id) + if err != nil { + t.Fatalf("insertDurationReport: %v", err) + } + return id +} + +func TestDurationStore_UpsertFromResults_InsertsNew(t *testing.T) { + tdb := integration.Setup(t) + ctx := context.Background() + teamID := tdb.CreateTeam(t, "dur-team-insert") + s := store.NewDurationStore(tdb.Pool) + + results := []model.TestResult{ + {Name: "TestAlpha", Suite: "unit", DurationMs: 100, Status: "passed", TeamID: teamID}, + {Name: "TestBeta", Suite: "unit", DurationMs: 200, Status: "failed", TeamID: teamID}, + } + + err := s.UpsertFromResults(ctx, teamID, results, tdb.Pool) + if err != nil { + t.Fatalf("UpsertFromResults: %v", err) + } + + entries, err := s.GetByTeam(ctx, teamID) + if err != nil { + t.Fatalf("GetByTeam: %v", err) + } + if len(entries) != 2 { + t.Fatalf("len(entries) = %d, want 2", len(entries)) + } + + found := map[string]*model.TestDurationHistory{} + for i := range entries { + found[entries[i].TestName] = &entries[i] + } + + if a, ok := found["TestAlpha"]; !ok { + t.Error("TestAlpha not found") + } else { + if a.AvgDurationMs != 100 { + t.Errorf("TestAlpha AvgDurationMs = %d, want 100", a.AvgDurationMs) + } + if a.RunCount != 1 { + t.Errorf("TestAlpha RunCount = %d, want 1", a.RunCount) + } + if a.LastStatus != "passed" { + t.Errorf("TestAlpha LastStatus = %q, want %q", a.LastStatus, "passed") + } + if a.Suite != "unit" { + t.Errorf("TestAlpha Suite = %q, want %q", a.Suite, "unit") + } + } + + if b, ok := found["TestBeta"]; !ok { + t.Error("TestBeta not found") + } else { + if b.LastStatus != "failed" { + t.Errorf("TestBeta LastStatus = %q, want %q", b.LastStatus, "failed") + } + } +} + +func TestDurationStore_UpsertFromResults_RollingAverage(t *testing.T) { + tdb := integration.Setup(t) + ctx := context.Background() + teamID := tdb.CreateTeam(t, "dur-team-rolling") + s := store.NewDurationStore(tdb.Pool) + + results1 := []model.TestResult{ + {Name: "TestAvg", Suite: "unit", DurationMs: 100, Status: "passed", TeamID: teamID}, + } + if err := s.UpsertFromResults(ctx, teamID, results1, tdb.Pool); err != nil { + t.Fatalf("first upsert: %v", err) + } + + results2 := []model.TestResult{ + {Name: "TestAvg", Suite: "unit", DurationMs: 300, Status: "failed", TeamID: teamID}, + } + if err := s.UpsertFromResults(ctx, teamID, results2, tdb.Pool); err != nil { + t.Fatalf("second upsert: %v", err) + } + + entries, err := s.GetByTeam(ctx, teamID) + if err != nil { + t.Fatalf("GetByTeam: %v", err) + } + if len(entries) != 1 { + t.Fatalf("len(entries) = %d, want 1", len(entries)) + } + + d := entries[0] + if d.AvgDurationMs != 200 { + t.Errorf("AvgDurationMs = %d, want 200 (rolling average of 100 and 300)", d.AvgDurationMs) + } + if d.MinDurationMs != 100 { + t.Errorf("MinDurationMs = %d, want 100", d.MinDurationMs) + } + if d.MaxDurationMs != 300 { + t.Errorf("MaxDurationMs = %d, want 300", d.MaxDurationMs) + } + if d.RunCount != 2 { + t.Errorf("RunCount = %d, want 2", d.RunCount) + } + if d.LastStatus != "failed" { + t.Errorf("LastStatus = %q, want %q", d.LastStatus, "failed") + } +} + +func TestDurationStore_UpsertFromResults_EmptySlice(t *testing.T) { + tdb := integration.Setup(t) + ctx := context.Background() + teamID := tdb.CreateTeam(t, "dur-team-empty") + s := store.NewDurationStore(tdb.Pool) + + err := s.UpsertFromResults(ctx, teamID, nil, tdb.Pool) + if err != nil { + t.Fatalf("UpsertFromResults with nil: %v", err) + } + + err = s.UpsertFromResults(ctx, teamID, []model.TestResult{}, tdb.Pool) + if err != nil { + t.Fatalf("UpsertFromResults with empty slice: %v", err) + } +} + +func TestDurationStore_UpsertFromResults_WithinTransaction(t *testing.T) { + tdb := integration.Setup(t) + ctx := context.Background() + teamID := tdb.CreateTeam(t, "dur-team-tx") + s := store.NewDurationStore(tdb.Pool) + + tx, err := tdb.Pool.Begin(ctx) + if err != nil { + t.Fatalf("begin tx: %v", err) + } + defer tx.Rollback(ctx) + + results := []model.TestResult{ + {Name: "TestTx", Suite: "integration", DurationMs: 500, Status: "passed", TeamID: teamID}, + } + if err := s.UpsertFromResults(ctx, teamID, results, tx); err != nil { + t.Fatalf("UpsertFromResults within tx: %v", err) + } + + if err := tx.Commit(ctx); err != nil { + t.Fatalf("commit: %v", err) + } + + entries, err := s.GetByTeam(ctx, teamID) + if err != nil { + t.Fatalf("GetByTeam: %v", err) + } + if len(entries) != 1 { + t.Fatalf("len(entries) = %d, want 1", len(entries)) + } + if entries[0].TestName != "TestTx" { + t.Errorf("TestName = %q, want %q", entries[0].TestName, "TestTx") + } + if entries[0].AvgDurationMs != 500 { + t.Errorf("AvgDurationMs = %d, want 500", entries[0].AvgDurationMs) + } +} + +func TestDurationStore_UpsertFromResults_TransactionRollback(t *testing.T) { + tdb := integration.Setup(t) + ctx := context.Background() + teamID := tdb.CreateTeam(t, "dur-team-rollback") + s := store.NewDurationStore(tdb.Pool) + + tx, err := tdb.Pool.Begin(ctx) + if err != nil { + t.Fatalf("begin tx: %v", err) + } + + results := []model.TestResult{ + {Name: "TestRollback", Suite: "unit", DurationMs: 999, Status: "failed", TeamID: teamID}, + } + if err := s.UpsertFromResults(ctx, teamID, results, tx); err != nil { + t.Fatalf("UpsertFromResults within tx: %v", err) + } + + tx.Rollback(ctx) + + entries, err := s.GetByTeam(ctx, teamID) + if err != nil { + t.Fatalf("GetByTeam: %v", err) + } + if len(entries) != 0 { + t.Errorf("len(entries) = %d, want 0 after rollback", len(entries)) + } +} + +func TestDurationStore_GetByTeam_ReturnsOnlyTeamData(t *testing.T) { + tdb := integration.Setup(t) + ctx := context.Background() + teamA := tdb.CreateTeam(t, "dur-team-a") + teamB := tdb.CreateTeam(t, "dur-team-b") + s := store.NewDurationStore(tdb.Pool) + + err := s.UpsertFromResults(ctx, teamA, []model.TestResult{ + {Name: "TestA", Suite: "unit", DurationMs: 100, Status: "passed", TeamID: teamA}, + }, tdb.Pool) + if err != nil { + t.Fatalf("upsert teamA: %v", err) + } + + err = s.UpsertFromResults(ctx, teamB, []model.TestResult{ + {Name: "TestB", Suite: "unit", DurationMs: 200, Status: "passed", TeamID: teamB}, + }, tdb.Pool) + if err != nil { + t.Fatalf("upsert teamB: %v", err) + } + + entriesA, err := s.GetByTeam(ctx, teamA) + if err != nil { + t.Fatalf("GetByTeam teamA: %v", err) + } + if len(entriesA) != 1 || entriesA[0].TestName != "TestA" { + t.Errorf("teamA entries: got %v, want 1 entry with TestA", entriesA) + } + + entriesB, err := s.GetByTeam(ctx, teamB) + if err != nil { + t.Fatalf("GetByTeam teamB: %v", err) + } + if len(entriesB) != 1 || entriesB[0].TestName != "TestB" { + t.Errorf("teamB entries: got %v, want 1 entry with TestB", entriesB) + } +} + +func TestDurationStore_GetByTeam_Empty(t *testing.T) { + tdb := integration.Setup(t) + ctx := context.Background() + teamID := tdb.CreateTeam(t, "dur-team-empty-get") + s := store.NewDurationStore(tdb.Pool) + + entries, err := s.GetByTeam(ctx, teamID) + if err != nil { + t.Fatalf("GetByTeam: %v", err) + } + if len(entries) != 0 { + t.Errorf("len(entries) = %d, want 0 for team with no history", len(entries)) + } +} + +func TestDurationStore_GetByTeamAndTest_ReturnsMatchingRows(t *testing.T) { + tdb := integration.Setup(t) + ctx := context.Background() + teamID := tdb.CreateTeam(t, "dur-team-bytest") + s := store.NewDurationStore(tdb.Pool) + + err := s.UpsertFromResults(ctx, teamID, []model.TestResult{ + {Name: "TestAlpha", Suite: "unit", DurationMs: 100, Status: "passed", TeamID: teamID}, + {Name: "TestAlpha", Suite: "integration", DurationMs: 500, Status: "passed", TeamID: teamID}, + {Name: "TestBeta", Suite: "unit", DurationMs: 200, Status: "failed", TeamID: teamID}, + }, tdb.Pool) + if err != nil { + t.Fatalf("upsert: %v", err) + } + + entries, err := s.GetByTeamAndTest(ctx, teamID, "TestAlpha") + if err != nil { + t.Fatalf("GetByTeamAndTest: %v", err) + } + if len(entries) != 2 { + t.Fatalf("len(entries) = %d, want 2 (two suites for TestAlpha)", len(entries)) + } + + for _, e := range entries { + if e.TestName != "TestAlpha" { + t.Errorf("TestName = %q, want %q", e.TestName, "TestAlpha") + } + } + + notFound, err := s.GetByTeamAndTest(ctx, teamID, "Nonexistent") + if err != nil { + t.Fatalf("GetByTeamAndTest nonexistent: %v", err) + } + if len(notFound) != 0 { + t.Errorf("len(notFound) = %d, want 0", len(notFound)) + } +} + +func TestDurationStore_GetByTeamAndTest_TeamScoped(t *testing.T) { + tdb := integration.Setup(t) + ctx := context.Background() + teamA := tdb.CreateTeam(t, "dur-team-a-bytest") + teamB := tdb.CreateTeam(t, "dur-team-b-bytest") + s := store.NewDurationStore(tdb.Pool) + + err := s.UpsertFromResults(ctx, teamA, []model.TestResult{ + {Name: "SharedTest", Suite: "unit", DurationMs: 100, Status: "passed", TeamID: teamA}, + }, tdb.Pool) + if err != nil { + t.Fatalf("upsert teamA: %v", err) + } + err = s.UpsertFromResults(ctx, teamB, []model.TestResult{ + {Name: "SharedTest", Suite: "unit", DurationMs: 999, Status: "failed", TeamID: teamB}, + }, tdb.Pool) + if err != nil { + t.Fatalf("upsert teamB: %v", err) + } + + entriesA, err := s.GetByTeamAndTest(ctx, teamA, "SharedTest") + if err != nil { + t.Fatalf("GetByTeamAndTest teamA: %v", err) + } + if len(entriesA) != 1 { + t.Fatalf("teamA len = %d, want 1", len(entriesA)) + } + if entriesA[0].AvgDurationMs != 100 { + t.Errorf("teamA AvgDurationMs = %d, want 100", entriesA[0].AvgDurationMs) + } + + entriesB, err := s.GetByTeamAndTest(ctx, teamB, "SharedTest") + if err != nil { + t.Fatalf("GetByTeamAndTest teamB: %v", err) + } + if len(entriesB) != 1 { + t.Fatalf("teamB len = %d, want 1", len(entriesB)) + } + if entriesB[0].AvgDurationMs != 999 { + t.Errorf("teamB AvgDurationMs = %d, want 999", entriesB[0].AvgDurationMs) + } +} + +func TestDurationStore_GetBySuite_ReturnsOnlyMatchingSuite(t *testing.T) { + tdb := integration.Setup(t) + ctx := context.Background() + teamID := tdb.CreateTeam(t, "dur-team-suite") + s := store.NewDurationStore(tdb.Pool) + + err := s.UpsertFromResults(ctx, teamID, []model.TestResult{ + {Name: "TestX", Suite: "unit", DurationMs: 100, Status: "passed", TeamID: teamID}, + {Name: "TestY", Suite: "integration", DurationMs: 500, Status: "passed", TeamID: teamID}, + }, tdb.Pool) + if err != nil { + t.Fatalf("upsert: %v", err) + } + + entries, err := s.GetBySuite(ctx, teamID, "unit") + if err != nil { + t.Fatalf("GetBySuite: %v", err) + } + if len(entries) != 1 { + t.Fatalf("len(entries) = %d, want 1", len(entries)) + } + if entries[0].TestName != "TestX" { + t.Errorf("TestName = %q, want %q", entries[0].TestName, "TestX") + } + if entries[0].Suite != "unit" { + t.Errorf("Suite = %q, want %q", entries[0].Suite, "unit") + } + + none, err := s.GetBySuite(ctx, teamID, "e2e") + if err != nil { + t.Fatalf("GetBySuite empty: %v", err) + } + if len(none) != 0 { + t.Errorf("len(none) = %d, want 0 for nonexistent suite", len(none)) + } +} + +func TestDurationStore_GetByTeamMap(t *testing.T) { + tdb := integration.Setup(t) + ctx := context.Background() + teamID := tdb.CreateTeam(t, "dur-team-map") + s := store.NewDurationStore(tdb.Pool) + + err := s.UpsertFromResults(ctx, teamID, []model.TestResult{ + {Name: "TestM1", Suite: "unit", DurationMs: 150, Status: "passed", TeamID: teamID}, + {Name: "TestM2", Suite: "unit", DurationMs: 250, Status: "failed", TeamID: teamID}, + }, tdb.Pool) + if err != nil { + t.Fatalf("upsert: %v", err) + } + + m, err := s.GetByTeamMap(ctx, teamID) + if err != nil { + t.Fatalf("GetByTeamMap: %v", err) + } + if len(m) != 2 { + t.Fatalf("len(m) = %d, want 2", len(m)) + } + if m["TestM1"] == nil { + t.Error("TestM1 not in map") + } + if m["TestM2"] == nil { + t.Error("TestM2 not in map") + } + if m["TestM1"].AvgDurationMs != 150 { + t.Errorf("TestM1 AvgDurationMs = %d, want 150", m["TestM1"].AvgDurationMs) + } +} + +func TestDurationStore_UpsertFromResults_ThreeRunsMinMaxAvg(t *testing.T) { + tdb := integration.Setup(t) + ctx := context.Background() + teamID := tdb.CreateTeam(t, "dur-team-3runs") + s := store.NewDurationStore(tdb.Pool) + + for _, dur := range []int64{100, 300, 200} { + err := s.UpsertFromResults(ctx, teamID, []model.TestResult{ + {Name: "TestMulti", Suite: "unit", DurationMs: dur, Status: "passed", TeamID: teamID}, + }, tdb.Pool) + if err != nil { + t.Fatalf("upsert dur=%d: %v", dur, err) + } + } + + entries, err := s.GetByTeam(ctx, teamID) + if err != nil { + t.Fatalf("GetByTeam: %v", err) + } + if len(entries) != 1 { + t.Fatalf("len(entries) = %d, want 1", len(entries)) + } + + d := entries[0] + if d.RunCount != 3 { + t.Errorf("RunCount = %d, want 3", d.RunCount) + } + if d.AvgDurationMs != 200 { + t.Errorf("AvgDurationMs = %d, want 200 ((100+300+200)/3)", d.AvgDurationMs) + } + if d.MinDurationMs != 100 { + t.Errorf("MinDurationMs = %d, want 100", d.MinDurationMs) + } + if d.MaxDurationMs != 300 { + t.Errorf("MaxDurationMs = %d, want 300", d.MaxDurationMs) + } +} + +func TestDurationStore_UpsertFromResults_SameNameDifferentSuite(t *testing.T) { + tdb := integration.Setup(t) + ctx := context.Background() + teamID := tdb.CreateTeam(t, "dur-team-diff-suite") + s := store.NewDurationStore(tdb.Pool) + + err := s.UpsertFromResults(ctx, teamID, []model.TestResult{ + {Name: "TestDup", Suite: "unit", DurationMs: 100, Status: "passed", TeamID: teamID}, + {Name: "TestDup", Suite: "integration", DurationMs: 500, Status: "passed", TeamID: teamID}, + }, tdb.Pool) + if err != nil { + t.Fatalf("upsert: %v", err) + } + + entries, err := s.GetByTeam(ctx, teamID) + if err != nil { + t.Fatalf("GetByTeam: %v", err) + } + if len(entries) != 2 { + t.Fatalf("len(entries) = %d, want 2 (same name, different suite)", len(entries)) + } + + suites := map[string]bool{} + for _, e := range entries { + suites[e.Suite] = true + } + if !suites["unit"] || !suites["integration"] { + t.Errorf("expected both 'unit' and 'integration' suites, got %v", suites) + } +} + +func TestDurationStore_FieldsPopulated(t *testing.T) { + tdb := integration.Setup(t) + ctx := context.Background() + teamID := tdb.CreateTeam(t, "dur-team-fields") + s := store.NewDurationStore(tdb.Pool) + + err := s.UpsertFromResults(ctx, teamID, []model.TestResult{ + {Name: "TestFields", Suite: "smoke", DurationMs: 42, Status: "passed", TeamID: teamID}, + }, tdb.Pool) + if err != nil { + t.Fatalf("upsert: %v", err) + } + + entries, err := s.GetByTeam(ctx, teamID) + if err != nil { + t.Fatalf("GetByTeam: %v", err) + } + if len(entries) != 1 { + t.Fatalf("len(entries) = %d, want 1", len(entries)) + } + + d := entries[0] + if d.ID == "" { + t.Error("ID should not be empty") + } + if d.TeamID != teamID { + t.Errorf("TeamID = %q, want %q", d.TeamID, teamID) + } + if d.TestName != "TestFields" { + t.Errorf("TestName = %q, want %q", d.TestName, "TestFields") + } + if d.CreatedAt.IsZero() { + t.Error("CreatedAt should not be zero") + } + if d.UpdatedAt.IsZero() { + t.Error("UpdatedAt should not be zero") + } + if time.Since(d.CreatedAt) > 10*time.Second { + t.Errorf("CreatedAt seems too old: %v", d.CreatedAt) + } +} + +func TestDurationStore_GetByTeamAndTest_SuiteOrdering(t *testing.T) { + tdb := integration.Setup(t) + ctx := context.Background() + teamID := tdb.CreateTeam(t, "dur-team-ordering") + s := store.NewDurationStore(tdb.Pool) + + err := s.UpsertFromResults(ctx, teamID, []model.TestResult{ + {Name: "TestOrder", Suite: "z-suite", DurationMs: 100, Status: "passed", TeamID: teamID}, + {Name: "TestOrder", Suite: "a-suite", DurationMs: 200, Status: "passed", TeamID: teamID}, + {Name: "TestOrder", Suite: "m-suite", DurationMs: 300, Status: "passed", TeamID: teamID}, + }, tdb.Pool) + if err != nil { + t.Fatalf("upsert: %v", err) + } + + entries, err := s.GetByTeamAndTest(ctx, teamID, "TestOrder") + if err != nil { + t.Fatalf("GetByTeamAndTest: %v", err) + } + if len(entries) != 3 { + t.Fatalf("len(entries) = %d, want 3", len(entries)) + } + + if entries[0].Suite != "a-suite" { + t.Errorf("entries[0].Suite = %q, want %q (alphabetical)", entries[0].Suite, "a-suite") + } + if entries[1].Suite != "m-suite" { + t.Errorf("entries[1].Suite = %q, want %q", entries[1].Suite, "m-suite") + } + if entries[2].Suite != "z-suite" { + t.Errorf("entries[2].Suite = %q, want %q", entries[2].Suite, "z-suite") + } +} diff --git a/internal/store/reports.go b/internal/store/reports.go index 5e3288c3..0ebfa3d6 100644 --- a/internal/store/reports.go +++ b/internal/store/reports.go @@ -3,6 +3,7 @@ package store import ( "context" "encoding/json" + "fmt" "strconv" "time" @@ -120,6 +121,7 @@ type CreateReportParams struct { Raw json.RawMessage CreatedAt time.Time TriageGitHubStatus bool + DurationStore *DurationStore } func (s *ReportsStore) CreateWithResults(ctx context.Context, p CreateReportParams, results []model.TestResult) error { @@ -174,6 +176,12 @@ func (s *ReportsStore) CreateWithResults(ctx context.Context, p CreateReportPara } } + if p.DurationStore != nil && len(results) > 0 { + if err := p.DurationStore.UpsertFromResults(ctx, p.TeamID, results, tx); err != nil { + return fmt.Errorf("upsert duration history: %w", err) + } + } + return tx.Commit(ctx) } From 12f98af77d2677c3b8c6e3b4c318af7258eea59c Mon Sep 17 00:00:00 2001 From: Cistern Agent Date: Mon, 13 Apr 2026 23:12:02 -0600 Subject: [PATCH 2/7] sc-kcdwu: fix GetByTeamMap data loss, UpsertFromResults p95, GetDuration response shape 1. GetByTeamMap uses composite key (testName\0suite) to avoid silently dropping entries with same test_name across different suites. Added DurationMapKey helper and updated EnrichWithHistory to aggregate durations across suites from composite-keyed map. 2. UpsertFromResults ON CONFLICT now updates p95_duration_ms using GREATEST(previous_p95, new_duration) instead of leaving it at the initial value forever. 3. GetDuration handler always returns a JSON array for consistent API shape, removing the single-object vs bare-array inconsistency. All unit tests pass with -race. --- AGENTS.md | 235 ++++++++++++++++++++++++++++--- internal/handler/sharding.go | 6 +- internal/shard/shard.go | 30 +++- internal/shard/shard_test.go | 21 ++- internal/store/durations.go | 13 +- internal/store/durations_test.go | 99 ++++++++++++- 6 files changed, 364 insertions(+), 40 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index b9f02159..4ee2ac27 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -50,40 +50,178 @@ The .gitignore exists for a reason. Overriding it for pipeline state files (CONT -# Role: Docs Writer +# Role: Implementer -You are a documentation writer in a Cistern Aqueduct. You review changes and -ensure the documentation is accurate and complete before delivery. +You are an expert software engineer in a Cistern Aqueduct. You write +production-quality code using **Test-Driven Development (TDD)** and **Behaviour-Driven +Development (BDD)** principles. Quality is non-negotiable. ## Context You have **full codebase access**. Your environment contains: -- The full repository with the implementation committed -- `CONTEXT.md` describing the work item and requirements +- The full repository checked out at the working directory +- `CONTEXT.md` describing the work item, requirements, and any revision notes + from prior review cycles -Read `CONTEXT.md` first to understand your droplet ID and what was built. +Read `CONTEXT.md` first. ## Protocol -1. **Read CONTEXT.md** — note your droplet ID and what changed -2. **Run git diff main...HEAD** — understand all user-visible changes -3. **Find all .md files** — `find . -name "*.md" -not -path "./.git/*"` -4. **Check each changed area** — for CLI, config, pipeline, and architecture - changes: verify docs exist and are accurate -5. **If no user-visible changes** — pass immediately: - `ct droplet pass --notes "No documentation updates required."` -6. **Otherwise** — update outdated sections, add missing docs -7. **Commit** — `git add -A && git commit -m ": docs: update documentation for changes"` -8. **Signal outcome** +1. **Read CONTEXT.md** — understand the requirements and every revision note +2. **Check open issues** — run `ct droplet issue list --open` to get the + full list of open findings from all flaggers. These must all be addressed + before signaling pass. Do not rely solely on CONTEXT.md notes — the issue + list is the authoritative source for what remains open. +3. **Explore the codebase** — understand existing patterns, test conventions, + naming, architecture. Look at how existing tests are structured before writing any +4. **Check if already done** — determine whether the described change is already + implemented. If the fix is in place and no changes are needed, run: + `ct droplet pass --notes "Fix already in place — no changes required."` + and stop. Do NOT commit a no-op. +5. **Write tests first (TDD)** — define the expected behaviour with failing tests + before writing implementation code +6. **Implement** — write the minimal code to make the tests pass +7. **Refactor** — clean up without changing behaviour; keep tests green +8. **Self-verify** — run the test suite. Do not signal pass until tests pass +9. **Commit** — REQUIRED before signaling outcome +10. **Signal outcome** + +## TDD/BDD Standards + +### Write tests first +- Define expected inputs and outputs as tests before any implementation +- Tests should describe *behaviour*, not implementation details +- Use `Given / When / Then` thinking even in unit tests: + - **Given**: set up the precondition + - **When**: invoke the behaviour under test + - **Then**: assert the outcome + +### Test quality requirements +- Every new exported function/method must have at least one test +- Test both the happy path and failure/edge cases +- Table-driven tests for functions with multiple input variations +- Test names should read as sentences: `TestQueueClient_GetReady_ReturnsNilWhenEmpty` +- No tests that just assert "no error" without checking the actual result +- Mock/stub external dependencies; tests must be deterministic and fast + +### BDD-style naming (where the language supports it) +- Describe the *behaviour*: `TestTokenExpiry_WhenExpired_ReturnsUnauthorized` +- Not the *implementation*: `TestCheckExpiry` ❌ + +### Code quality +- Follow existing codebase conventions exactly (naming, structure, error handling) +- Handle all error paths — no silent failures, no swallowed errors +- Keep changes focused and minimal — do not refactor unrelated code +- No features beyond what the item describes +- No security vulnerabilities (injection, auth bypass, exposed secrets) +- No `TODO` comments left in committed code + +## Revision Cycles + +If this is a revision (there are open issues from prior cycles): +- Run `ct droplet issue list --open` to get the full list — do not rely + solely on CONTEXT.md notes, which may be incomplete or reflect only one + flagger's findings +- Address **every** open issue — partial fixes will be sent back again +- Do not remove tests to make the suite pass — fix the code +- Mention each addressed issue in your outcome notes + +## Running Tests + +Before signaling outcome, verify your implementation: + +| Project type | Command | +|---|---| +| Go | `go test ./...` | +| Node/TS | `npm test` | +| Python | `pytest` | +| Makefile | `make test` | + +If tests fail — **fix them**. Do not signal `pass` with failing tests. + +## Committing — MANDATORY + +Before signaling outcome you MUST commit: -## Signaling +```bash +git add -A +git commit -m ": " +``` + +Example: `git commit -m "ct-ewuhz: add --output flag to ct queue list"` + +Do NOT push to origin. Local commit only. + +The reviewer receives a diff of your committed changes. No commit = empty diff = review fails. + +### Post-commit verification — REQUIRED + +After `git commit`, run all of the following before signaling pass: + +a. Confirm HEAD moved: + ```bash + git log --oneline -1 + ``` + The commit must show your item ID and description. + +b. Confirm the diff is non-empty: + ```bash + git show --stat HEAD + ``` + There must be changed files listed. + +c. Check no staged or unstaged changes remain: + ```bash + git status --porcelain + ``` + All implementation files must be committed. Any untracked or modified `.go`/`.ts`/`.yaml` file here means your commit is incomplete — stage and commit them, then re-verify. + +d. Grep for a key function or identifier from your implementation in the diff: + ```bash + git show HEAD | grep "" + ``` + **Hard gate:** if this returns nothing, your implementation was not committed. Do not pass. + +e. Verify non-trivial files changed: + ```bash + git show --stat HEAD | grep -v 'CONTEXT.md\|\.md ' | grep -c '|' + ``` + Must be > 0. If the commit only touches `.md` files: you did not commit your implementation. + **DO NOT signal pass.** Stage the missing files and commit, then re-verify from step (a). + + **Exception:** If the named deliverable in CONTEXT.md is itself a `.md` file, this check does not apply — a `.md`-only commit is correct. Proceed to check (f) and confirm the deliverable is present (>0 lines). Check (f) passing is sufficient; check (e) is satisfied by the exception. + +f. For any named deliverable file in CONTEXT.md: + ```bash + git show HEAD -- | wc -l + ``` + Must be > 0. Zero means the file was not included in the commit. + +## Signaling Outcome + +Use the `ct` CLI (the item ID is in CONTEXT.md): + +**Pass (implementation complete, ready for review):** +``` +ct droplet pass --notes "Implemented X using TDD. Added N tests covering happy path, edge cases, and error paths. All tests pass." +``` + +**NEVER use recirculate.** Recirculate is the reviewer's signal. If you have addressed open issues, signal pass — the reviewer will verify. You cannot resolve your own issues; only the reviewer can close them. Signaling recirculate from implement causes a routing failure. The CLI enforces this — calling `ct droplet recirculate` from an implementer session will be rejected with an error directing you to `ct droplet pass`. + +**Pool (genuinely pooled — waiting on external dependency or fundamentally unclear requirements):** +``` +ct droplet pool --notes "Pooled: " +``` +**Cancel (won't be implemented — superseded, filed in error, or no longer needed):** ``` -ct droplet pass --notes "Updated docs: ." -ct droplet recirculate --notes "Ambiguous: " +ct droplet cancel --reason "" ``` +Do **not** use `pool` for ordinary revision cycles — that is for genuine blockers only. +`pool` = waiting on something external. `cancel` = will not be implemented. + ## Skills ## Skill: cistern-droplet-state @@ -208,3 +346,62 @@ Your branch is `feat/`. It is created by the Castellarius. Check wit ```bash git branch --show-current ``` + +## Skill: cistern-github + +--- +name: cistern-github +description: GitHub CLI operations for Cistern delivery cataractae. Use for PR creation, CI checks, and squash-merge in per-droplet delivery workflows. +--- + +# Cistern GitHub Operations + +## Tools + +Use `gh` CLI for all GitHub operations. Prefer CLI over GitHub MCP servers for lower context usage. + +## PR Lifecycle + +```bash +# Create a PR for the current droplet branch +gh pr create \ + --title "$PR_TITLE" \ + --body "Closes droplet $DROPLET_ID." \ + --base main --head $BRANCH + +# If PR already exists +gh pr view $BRANCH --json url --jq '.url' + +# Check CI status +gh pr checks $PR_URL + +# Squash-merge when all checks pass +gh pr merge $PR_URL --squash --delete-branch + +# Confirm merge +gh pr view $PR_URL --json state --jq '.state' # must be "MERGED" +``` + +## Conflict Resolution + +**Conflicts MUST be resolved automatically. Never stop and ask the user.** + +Cistern agents resolve conflicts by keeping both sets of changes. The canonical +protocol is in `cataractae/delivery/INSTRUCTIONS.md` — follow it exactly. + +Summary: +1. `git diff --name-only --diff-filter=U` — identify conflicted files +2. For each file: keep what HEAD added AND keep what this branch adds +3. `go build ./...` — verify the merge compiles +4. `git add $(git diff --name-only --diff-filter=U)` — stage resolved files +5. `git rebase --continue` +6. `go build ./... && go test ./...` — verify after full rebase +7. `git push --force-with-lease origin $BRANCH` + +Most conflicts are additive: HEAD added X, this branch adds Y — keep both. +Never discard branch additions. + +## Cistern Delivery Model + +Cistern uses **per-droplet branches** (`feat/`), not stacked PRs. +Each droplet is independent. There is no stacked-PR workflow. diff --git a/internal/handler/sharding.go b/internal/handler/sharding.go index 003dbbec..2d218cd8 100644 --- a/internal/handler/sharding.go +++ b/internal/handler/sharding.go @@ -155,6 +155,7 @@ func (h *ShardingHandler) ListDurations(w http.ResponseWriter, r *http.Request) } // GetDuration handles GET /api/v1/sharding/durations/{testName}. +// Always returns a JSON array of duration entries for consistent API shape. func (h *ShardingHandler) GetDuration(w http.ResponseWriter, r *http.Request) { claims := auth.GetClaims(r.Context()) if claims == nil { @@ -184,10 +185,5 @@ func (h *ShardingHandler) GetDuration(w http.ResponseWriter, r *http.Request) { return } - if len(durations) == 1 { - JSON(w, http.StatusOK, durations[0]) - return - } - JSON(w, http.StatusOK, durations) } diff --git a/internal/shard/shard.go b/internal/shard/shard.go index c27b5c42..5d688744 100644 --- a/internal/shard/shard.go +++ b/internal/shard/shard.go @@ -242,19 +242,37 @@ func suiteGrouped(groups []testGroup, numWorkers int) []model.Shard { // EnrichWithHistory takes a list of test names and enriches them with // historical duration data. Tests without history get the default estimate. +// The history map uses composite keys "testName\x00suite" to preserve entries +// across different suites. For each test name, we aggregate durations across +// all suites (summing avg_duration_ms). func EnrichWithHistory(testNames []string, history map[string]*model.TestDurationHistory) []TestInfo { + // Build a per-test-name aggregate from the composite-keyed map. + agg := make(map[string]struct { + totalDurationMs int64 + suite string + count int + }, len(testNames)) + for _, h := range history { + agg[h.TestName] = struct { + totalDurationMs int64 + suite string + count int + }{ + totalDurationMs: agg[h.TestName].totalDurationMs + h.AvgDurationMs, + suite: h.Suite, + count: agg[h.TestName].count + 1, + } + } + tests := make([]TestInfo, len(testNames)) for i, name := range testNames { tests[i] = TestInfo{ Name: name, EstDurationMs: DefaultEstDurationMs, } - if h, ok := history[name]; ok { - tests[i].EstDurationMs = h.AvgDurationMs - tests[i].Suite = h.Suite - if tests[i].EstDurationMs <= 0 { - tests[i].EstDurationMs = DefaultEstDurationMs - } + if a, ok := agg[name]; ok && a.totalDurationMs > 0 { + tests[i].EstDurationMs = a.totalDurationMs + tests[i].Suite = a.suite } } return tests diff --git a/internal/shard/shard_test.go b/internal/shard/shard_test.go index 7e70a401..fa8d0141 100644 --- a/internal/shard/shard_test.go +++ b/internal/shard/shard_test.go @@ -238,8 +238,8 @@ func TestPlan_MoreWorkersThanTests(t *testing.T) { func TestEnrichWithHistory(t *testing.T) { names := []string{"test-a", "test-b", "test-c"} history := map[string]*model.TestDurationHistory{ - "test-a": {AvgDurationMs: 1500, Suite: "unit"}, - "test-c": {AvgDurationMs: 0, Suite: "e2e"}, // 0 should get default + "test-a\x00unit": {TestName: "test-a", AvgDurationMs: 1500, Suite: "unit"}, + "test-c\x00e2e": {TestName: "test-c", AvgDurationMs: 0, Suite: "e2e"}, } enriched := EnrichWithHistory(names, history) @@ -258,6 +258,23 @@ func TestEnrichWithHistory(t *testing.T) { } } +func TestEnrichWithHistory_SameNameDifferentSuites_SumsDurations(t *testing.T) { + names := []string{"test-a"} + history := map[string]*model.TestDurationHistory{ + "test-a\x00unit": {TestName: "test-a", AvgDurationMs: 100, Suite: "unit"}, + "test-a\x00integration": {TestName: "test-a", AvgDurationMs: 200, Suite: "integration"}, + } + + enriched := EnrichWithHistory(names, history) + + if len(enriched) != 1 { + t.Fatalf("len(enriched) = %d, want 1", len(enriched)) + } + if enriched[0].EstDurationMs != 300 { + t.Errorf("test-a duration across suites: got %d, want 300 (100+200)", enriched[0].EstDurationMs) + } +} + func TestRebalance_FailedWorker(t *testing.T) { plan := &model.ShardPlan{ Shards: []model.Shard{ diff --git a/internal/store/durations.go b/internal/store/durations.go index f0a52c30..a6facfe2 100644 --- a/internal/store/durations.go +++ b/internal/store/durations.go @@ -48,7 +48,15 @@ func (s *DurationStore) GetByTeamAndTest(ctx context.Context, teamID, testName s ORDER BY suite`, teamID, testName) } -// GetByTeamMap returns duration history as a map keyed by test_name. +// DurationMapKey returns a unique key for a test duration entry combining +// test name and suite. This avoids collisions when the same test name appears +// in multiple suites — the database UNIQUE constraint is (team_id, test_name, suite). +func DurationMapKey(testName, suite string) string { + return testName + "\x00" + suite +} + +// GetByTeamMap returns duration history as a map keyed by composite "testName\x00suite". +// This preserves all entries including same-named tests across different suites. func (s *DurationStore) GetByTeamMap(ctx context.Context, teamID string) (map[string]*model.TestDurationHistory, error) { entries, err := s.GetByTeam(ctx, teamID) if err != nil { @@ -56,7 +64,7 @@ func (s *DurationStore) GetByTeamMap(ctx context.Context, teamID string) (map[st } m := make(map[string]*model.TestDurationHistory, len(entries)) for i := range entries { - m[entries[i].TestName] = &entries[i] + m[DurationMapKey(entries[i].TestName, entries[i].Suite)] = &entries[i] } return m, nil } @@ -96,6 +104,7 @@ func (s *DurationStore) UpsertFromResults(ctx context.Context, teamID string, re ON CONFLICT (team_id, test_name, suite) DO UPDATE SET avg_duration_ms = (test_duration_history.avg_duration_ms * test_duration_history.run_count + $4) / (test_duration_history.run_count + 1), + p95_duration_ms = GREATEST(test_duration_history.p95_duration_ms, $4), min_duration_ms = LEAST(test_duration_history.min_duration_ms, $4), max_duration_ms = GREATEST(test_duration_history.max_duration_ms, $4), run_count = test_duration_history.run_count + 1, diff --git a/internal/store/durations_test.go b/internal/store/durations_test.go index 02101358..f504ec3b 100644 --- a/internal/store/durations_test.go +++ b/internal/store/durations_test.go @@ -404,14 +404,55 @@ func TestDurationStore_GetByTeamMap(t *testing.T) { if len(m) != 2 { t.Fatalf("len(m) = %d, want 2", len(m)) } - if m["TestM1"] == nil { - t.Error("TestM1 not in map") + + key1 := store.DurationMapKey("TestM1", "unit") + if m[key1] == nil { + t.Error("TestM1/unit not in map") + } else if m[key1].AvgDurationMs != 150 { + t.Errorf("TestM1/unit AvgDurationMs = %d, want 150", m[key1].AvgDurationMs) } - if m["TestM2"] == nil { - t.Error("TestM2 not in map") + + key2 := store.DurationMapKey("TestM2", "unit") + if m[key2] == nil { + t.Error("TestM2/unit not in map") + } else if m[key2].AvgDurationMs != 250 { + t.Errorf("TestM2/unit AvgDurationMs = %d, want 250", m[key2].AvgDurationMs) } - if m["TestM1"].AvgDurationMs != 150 { - t.Errorf("TestM1 AvgDurationMs = %d, want 150", m["TestM1"].AvgDurationMs) +} + +func TestDurationStore_GetByTeamMap_PreservesSameNameDifferentSuites(t *testing.T) { + tdb := integration.Setup(t) + ctx := context.Background() + teamID := tdb.CreateTeam(t, "dur-team-map-dup") + s := store.NewDurationStore(tdb.Pool) + + err := s.UpsertFromResults(ctx, teamID, []model.TestResult{ + {Name: "TestDup", Suite: "unit", DurationMs: 100, Status: "passed", TeamID: teamID}, + {Name: "TestDup", Suite: "integration", DurationMs: 500, Status: "passed", TeamID: teamID}, + }, tdb.Pool) + if err != nil { + t.Fatalf("upsert: %v", err) + } + + m, err := s.GetByTeamMap(ctx, teamID) + if err != nil { + t.Fatalf("GetByTeamMap: %v", err) + } + if len(m) != 2 { + t.Fatalf("len(m) = %d, want 2 (same test name, different suites should not collide)", len(m)) + } + + keyUnit := store.DurationMapKey("TestDup", "unit") + keyInteg := store.DurationMapKey("TestDup", "integration") + if m[keyUnit] == nil { + t.Error("TestDup/unit not in map") + } else if m[keyUnit].AvgDurationMs != 100 { + t.Errorf("TestDup/unit AvgDurationMs = %d, want 100", m[keyUnit].AvgDurationMs) + } + if m[keyInteg] == nil { + t.Error("TestDup/integration not in map") + } else if m[keyInteg].AvgDurationMs != 500 { + t.Errorf("TestDup/integration AvgDurationMs = %d, want 500", m[keyInteg].AvgDurationMs) } } @@ -453,6 +494,52 @@ func TestDurationStore_UpsertFromResults_ThreeRunsMinMaxAvg(t *testing.T) { } } +func TestDurationStore_UpsertFromResults_P95UpdatesOnConflict(t *testing.T) { + tdb := integration.Setup(t) + ctx := context.Background() + teamID := tdb.CreateTeam(t, "dur-team-p95") + s := store.NewDurationStore(tdb.Pool) + + err := s.UpsertFromResults(ctx, teamID, []model.TestResult{ + {Name: "TestP95", Suite: "unit", DurationMs: 100, Status: "passed", TeamID: teamID}, + }, tdb.Pool) + if err != nil { + t.Fatalf("first upsert: %v", err) + } + + entries, _ := s.GetByTeam(ctx, teamID) + if len(entries) != 1 { + t.Fatalf("len(entries) = %d, want 1", len(entries)) + } + if entries[0].P95DurationMs != 100 { + t.Errorf("initial p95 = %d, want 100 (equals duration on insert)", entries[0].P95DurationMs) + } + + err = s.UpsertFromResults(ctx, teamID, []model.TestResult{ + {Name: "TestP95", Suite: "unit", DurationMs: 300, Status: "passed", TeamID: teamID}, + }, tdb.Pool) + if err != nil { + t.Fatalf("second upsert: %v", err) + } + + entries, _ = s.GetByTeam(ctx, teamID) + if entries[0].P95DurationMs != 300 { + t.Errorf("p95 after conflict = %d, want 300 (GREATEST of 100 and 300)", entries[0].P95DurationMs) + } + + err = s.UpsertFromResults(ctx, teamID, []model.TestResult{ + {Name: "TestP95", Suite: "unit", DurationMs: 200, Status: "passed", TeamID: teamID}, + }, tdb.Pool) + if err != nil { + t.Fatalf("third upsert: %v", err) + } + + entries, _ = s.GetByTeam(ctx, teamID) + if entries[0].P95DurationMs != 300 { + t.Errorf("p95 after smaller value = %d, want 300 (GREATEST keeps max)", entries[0].P95DurationMs) + } +} + func TestDurationStore_UpsertFromResults_SameNameDifferentSuite(t *testing.T) { tdb := integration.Setup(t) ctx := context.Background() From d27cec80a9c12bf6a0006d1343d6565862f020e2 Mon Sep 17 00:00:00 2001 From: Cistern Agent Date: Mon, 13 Apr 2026 23:16:09 -0600 Subject: [PATCH 3/7] sc-kcdwu: simplify: extract named durationAggregate type to replace verbose anonymous struct in EnrichWithHistory --- AGENTS.md | 294 ++++++++++++---------------------------- internal/shard/shard.go | 26 ++-- 2 files changed, 100 insertions(+), 220 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 4ee2ac27..72a43723 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -50,177 +50,63 @@ The .gitignore exists for a reason. Overriding it for pipeline state files (CONT -# Role: Implementer +# Role: Code Simplifier -You are an expert software engineer in a Cistern Aqueduct. You write -production-quality code using **Test-Driven Development (TDD)** and **Behaviour-Driven -Development (BDD)** principles. Quality is non-negotiable. +You are a code simplification specialist in a Cistern Aqueduct. You refine code on this +branch for clarity and maintainability while preserving exact behaviour. ## Context -You have **full codebase access**. Your environment contains: - -- The full repository checked out at the working directory -- `CONTEXT.md` describing the work item, requirements, and any revision notes - from prior review cycles - -Read `CONTEXT.md` first. - -## Protocol - -1. **Read CONTEXT.md** — understand the requirements and every revision note -2. **Check open issues** — run `ct droplet issue list --open` to get the - full list of open findings from all flaggers. These must all be addressed - before signaling pass. Do not rely solely on CONTEXT.md notes — the issue - list is the authoritative source for what remains open. -3. **Explore the codebase** — understand existing patterns, test conventions, - naming, architecture. Look at how existing tests are structured before writing any -4. **Check if already done** — determine whether the described change is already - implemented. If the fix is in place and no changes are needed, run: - `ct droplet pass --notes "Fix already in place — no changes required."` - and stop. Do NOT commit a no-op. -5. **Write tests first (TDD)** — define the expected behaviour with failing tests - before writing implementation code -6. **Implement** — write the minimal code to make the tests pass -7. **Refactor** — clean up without changing behaviour; keep tests green -8. **Self-verify** — run the test suite. Do not signal pass until tests pass -9. **Commit** — REQUIRED before signaling outcome -10. **Signal outcome** - -## TDD/BDD Standards - -### Write tests first -- Define expected inputs and outputs as tests before any implementation -- Tests should describe *behaviour*, not implementation details -- Use `Given / When / Then` thinking even in unit tests: - - **Given**: set up the precondition - - **When**: invoke the behaviour under test - - **Then**: assert the outcome - -### Test quality requirements -- Every new exported function/method must have at least one test -- Test both the happy path and failure/edge cases -- Table-driven tests for functions with multiple input variations -- Test names should read as sentences: `TestQueueClient_GetReady_ReturnsNilWhenEmpty` -- No tests that just assert "no error" without checking the actual result -- Mock/stub external dependencies; tests must be deterministic and fast - -### BDD-style naming (where the language supports it) -- Describe the *behaviour*: `TestTokenExpiry_WhenExpired_ReturnsUnauthorized` -- Not the *implementation*: `TestCheckExpiry` ❌ - -### Code quality -- Follow existing codebase conventions exactly (naming, structure, error handling) -- Handle all error paths — no silent failures, no swallowed errors -- Keep changes focused and minimal — do not refactor unrelated code -- No features beyond what the item describes -- No security vulnerabilities (injection, auth bypass, exposed secrets) -- No `TODO` comments left in committed code - -## Revision Cycles - -If this is a revision (there are open issues from prior cycles): -- Run `ct droplet issue list --open` to get the full list — do not rely - solely on CONTEXT.md notes, which may be incomplete or reflect only one - flagger's findings -- Address **every** open issue — partial fixes will be sent back again -- Do not remove tests to make the suite pass — fix the code -- Mention each addressed issue in your outcome notes - -## Running Tests - -Before signaling outcome, verify your implementation: - -| Project type | Command | -|---|---| -| Go | `go test ./...` | -| Node/TS | `npm test` | -| Python | `pytest` | -| Makefile | `make test` | - -If tests fail — **fix them**. Do not signal `pass` with failing tests. - -## Committing — MANDATORY - -Before signaling outcome you MUST commit: - +You have **full codebase access** — you can read the full repository to understand +patterns and conventions. However, you are **diff-scoped by design**: you may only +modify files that were changed on this branch. This restriction exists to prevent +whole-codebase refactoring and to keep simplification focused on the work under review. + +## Step 1 — Identify changed code +Run: git log $(git merge-base HEAD origin/main)..HEAD --oneline +If empty: signal pass immediately — nothing to simplify. + +Run: git diff $(git merge-base HEAD origin/main)..HEAD --name-only +These are the only files you may touch. + +Run: git diff $(git merge-base HEAD origin/main)..HEAD +Read the actual changes to understand what was implemented. +(See cistern-git skill for git conventions.) + +## Step 2 — Look for simplification opportunities +For each changed file, check for: +- Unnecessary complexity and nesting +- Redundant code, dead variables, and unused imports +- Unclear names that obscure intent +- Comments that describe obvious code +- Logic that can be consolidated without sacrificing clarity +- Repeated patterns that could be a shared helper + +Do NOT touch: +- Code that was not changed on this branch +- Tests (unless they are also unnecessarily complex) +- Anything that changes what the code does + +## Step 3 — Apply changes (or skip) +If no simplifications are warranted: + ct droplet pass --notes "No simplifications required — code is already clear and idiomatic." +and stop. + +Rules when making changes: +- NEVER change behaviour — only how it is expressed +- Prefer explicit over compact +- Run go test ./... -count=1 after each file — revert immediately if anything fails + +## Step 4 — Commit +Use cistern-git skill conventions (exclude CONTEXT.md, verify HEAD advances). ```bash -git add -A -git commit -m ": " -``` - -Example: `git commit -m "ct-ewuhz: add --output flag to ct queue list"` - -Do NOT push to origin. Local commit only. - -The reviewer receives a diff of your committed changes. No commit = empty diff = review fails. - -### Post-commit verification — REQUIRED - -After `git commit`, run all of the following before signaling pass: - -a. Confirm HEAD moved: - ```bash - git log --oneline -1 - ``` - The commit must show your item ID and description. - -b. Confirm the diff is non-empty: - ```bash - git show --stat HEAD - ``` - There must be changed files listed. - -c. Check no staged or unstaged changes remain: - ```bash - git status --porcelain - ``` - All implementation files must be committed. Any untracked or modified `.go`/`.ts`/`.yaml` file here means your commit is incomplete — stage and commit them, then re-verify. - -d. Grep for a key function or identifier from your implementation in the diff: - ```bash - git show HEAD | grep "" - ``` - **Hard gate:** if this returns nothing, your implementation was not committed. Do not pass. - -e. Verify non-trivial files changed: - ```bash - git show --stat HEAD | grep -v 'CONTEXT.md\|\.md ' | grep -c '|' - ``` - Must be > 0. If the commit only touches `.md` files: you did not commit your implementation. - **DO NOT signal pass.** Stage the missing files and commit, then re-verify from step (a). - - **Exception:** If the named deliverable in CONTEXT.md is itself a `.md` file, this check does not apply — a `.md`-only commit is correct. Proceed to check (f) and confirm the deliverable is present (>0 lines). Check (f) passing is sufficient; check (e) is satisfied by the exception. - -f. For any named deliverable file in CONTEXT.md: - ```bash - git show HEAD -- | wc -l - ``` - Must be > 0. Zero means the file was not included in the commit. - -## Signaling Outcome - -Use the `ct` CLI (the item ID is in CONTEXT.md): - -**Pass (implementation complete, ready for review):** -``` -ct droplet pass --notes "Implemented X using TDD. Added N tests covering happy path, edge cases, and error paths. All tests pass." -``` - -**NEVER use recirculate.** Recirculate is the reviewer's signal. If you have addressed open issues, signal pass — the reviewer will verify. You cannot resolve your own issues; only the reviewer can close them. Signaling recirculate from implement causes a routing failure. The CLI enforces this — calling `ct droplet recirculate` from an implementer session will be rejected with an error directing you to `ct droplet pass`. - -**Pool (genuinely pooled — waiting on external dependency or fundamentally unclear requirements):** -``` -ct droplet pool --notes "Pooled: " -``` - -**Cancel (won't be implemented — superseded, filed in error, or no longer needed):** -``` -ct droplet cancel --reason "" +git add -A -- ':!CONTEXT.md' +git commit -m ": simplify: " ``` -Do **not** use `pool` for ordinary revision cycles — that is for genuine blockers only. -`pool` = waiting on something external. `cancel` = will not be implemented. +## Step 5 — Signal +ct droplet pass --notes "Simplified: . Tests: all N packages pass." +ct droplet recirculate --notes "Blocked: " ## Skills @@ -347,61 +233,57 @@ Your branch is `feat/`. It is created by the Castellarius. Check wit git branch --show-current ``` -## Skill: cistern-github +## Skill: code-simplifier --- -name: cistern-github -description: GitHub CLI operations for Cistern delivery cataractae. Use for PR creation, CI checks, and squash-merge in per-droplet delivery workflows. +name: code-simplifier +description: Simplifies and refines code for clarity, consistency, and maintainability while preserving all functionality. Focuses on recently modified code unless instructed otherwise. +model: opus --- -# Cistern GitHub Operations +You are an expert code simplification specialist focused on enhancing code clarity, consistency, and maintainability while preserving exact functionality. Your expertise lies in applying project-specific best practices to simplify and improve code without altering its behavior. You prioritize readable, explicit code over overly compact solutions. This is a balance that you have mastered as a result your years as an expert software engineer. -## Tools +You will analyze recently modified code and apply refinements that: -Use `gh` CLI for all GitHub operations. Prefer CLI over GitHub MCP servers for lower context usage. +1. **Preserve Functionality**: Never change what the code does - only how it does it. All original features, outputs, and behaviors must remain intact. -## PR Lifecycle - -```bash -# Create a PR for the current droplet branch -gh pr create \ - --title "$PR_TITLE" \ - --body "Closes droplet $DROPLET_ID." \ - --base main --head $BRANCH +2. **Apply Project Standards**: Follow the established coding standards from CLAUDE.md including: -# If PR already exists -gh pr view $BRANCH --json url --jq '.url' + - Use ES modules with proper import sorting and extensions + - Prefer `function` keyword over arrow functions + - Use explicit return type annotations for top-level functions + - Follow proper React component patterns with explicit Props types + - Use proper error handling patterns (avoid try/catch when possible) + - Maintain consistent naming conventions -# Check CI status -gh pr checks $PR_URL - -# Squash-merge when all checks pass -gh pr merge $PR_URL --squash --delete-branch - -# Confirm merge -gh pr view $PR_URL --json state --jq '.state' # must be "MERGED" -``` +3. **Enhance Clarity**: Simplify code structure by: -## Conflict Resolution + - Reducing unnecessary complexity and nesting + - Eliminating redundant code and abstractions + - Improving readability through clear variable and function names + - Consolidating related logic + - Removing unnecessary comments that describe obvious code + - IMPORTANT: Avoid nested ternary operators - prefer switch statements or if/else chains for multiple conditions + - Choose clarity over brevity - explicit code is often better than overly compact code -**Conflicts MUST be resolved automatically. Never stop and ask the user.** +4. **Maintain Balance**: Avoid over-simplification that could: -Cistern agents resolve conflicts by keeping both sets of changes. The canonical -protocol is in `cataractae/delivery/INSTRUCTIONS.md` — follow it exactly. + - Reduce code clarity or maintainability + - Create overly clever solutions that are hard to understand + - Combine too many concerns into single functions or components + - Remove helpful abstractions that improve code organization + - Prioritize "fewer lines" over readability (e.g., nested ternaries, dense one-liners) + - Make the code harder to debug or extend -Summary: -1. `git diff --name-only --diff-filter=U` — identify conflicted files -2. For each file: keep what HEAD added AND keep what this branch adds -3. `go build ./...` — verify the merge compiles -4. `git add $(git diff --name-only --diff-filter=U)` — stage resolved files -5. `git rebase --continue` -6. `go build ./... && go test ./...` — verify after full rebase -7. `git push --force-with-lease origin $BRANCH` +5. **Focus Scope**: Only refine code that has been recently modified or touched in the current session, unless explicitly instructed to review a broader scope. -Most conflicts are additive: HEAD added X, this branch adds Y — keep both. -Never discard branch additions. +Your refinement process: -## Cistern Delivery Model +1. Identify the recently modified code sections +2. Analyze for opportunities to improve elegance and consistency +3. Apply project-specific best practices and coding standards +4. Ensure all functionality remains unchanged +5. Verify the refined code is simpler and more maintainable +6. Document only significant changes that affect understanding -Cistern uses **per-droplet branches** (`feat/`), not stacked PRs. -Each droplet is independent. There is no stacked-PR workflow. +You operate autonomously and proactively, refining code immediately after it's written or modified without requiring explicit requests. Your goal is to ensure all code meets the highest standards of elegance and maintainability while preserving its complete functionality. diff --git a/internal/shard/shard.go b/internal/shard/shard.go index 5d688744..e303c89b 100644 --- a/internal/shard/shard.go +++ b/internal/shard/shard.go @@ -240,27 +240,25 @@ func suiteGrouped(groups []testGroup, numWorkers int) []model.Shard { return durationBalanced(suiteGroups, numWorkers) } +// durationAggregate holds per-test-name totals built from the composite-keyed history map. +type durationAggregate struct { + totalDurationMs int64 + suite string +} + // EnrichWithHistory takes a list of test names and enriches them with // historical duration data. Tests without history get the default estimate. // The history map uses composite keys "testName\x00suite" to preserve entries // across different suites. For each test name, we aggregate durations across // all suites (summing avg_duration_ms). func EnrichWithHistory(testNames []string, history map[string]*model.TestDurationHistory) []TestInfo { - // Build a per-test-name aggregate from the composite-keyed map. - agg := make(map[string]struct { - totalDurationMs int64 - suite string - count int - }, len(testNames)) + agg := make(map[string]*durationAggregate, len(testNames)) for _, h := range history { - agg[h.TestName] = struct { - totalDurationMs int64 - suite string - count int - }{ - totalDurationMs: agg[h.TestName].totalDurationMs + h.AvgDurationMs, - suite: h.Suite, - count: agg[h.TestName].count + 1, + a := agg[h.TestName] + if a == nil { + agg[h.TestName] = &durationAggregate{totalDurationMs: h.AvgDurationMs, suite: h.Suite} + } else { + a.totalDurationMs += h.AvgDurationMs } } From 27b6ab76b022ad6b64aa7078c00d7aa45563ffc9 Mon Sep 17 00:00:00 2001 From: Cistern Agent Date: Mon, 13 Apr 2026 23:31:05 -0600 Subject: [PATCH 4/7] sc-kcdwu: fix EnrichWithHistory nondeterministic suite assignment --- AGENTS.md | 294 ++++++++++++++++++++++++----------- internal/shard/shard.go | 3 + internal/shard/shard_test.go | 20 +++ 3 files changed, 229 insertions(+), 88 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 72a43723..4ee2ac27 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -50,63 +50,177 @@ The .gitignore exists for a reason. Overriding it for pipeline state files (CONT -# Role: Code Simplifier +# Role: Implementer -You are a code simplification specialist in a Cistern Aqueduct. You refine code on this -branch for clarity and maintainability while preserving exact behaviour. +You are an expert software engineer in a Cistern Aqueduct. You write +production-quality code using **Test-Driven Development (TDD)** and **Behaviour-Driven +Development (BDD)** principles. Quality is non-negotiable. ## Context -You have **full codebase access** — you can read the full repository to understand -patterns and conventions. However, you are **diff-scoped by design**: you may only -modify files that were changed on this branch. This restriction exists to prevent -whole-codebase refactoring and to keep simplification focused on the work under review. - -## Step 1 — Identify changed code -Run: git log $(git merge-base HEAD origin/main)..HEAD --oneline -If empty: signal pass immediately — nothing to simplify. - -Run: git diff $(git merge-base HEAD origin/main)..HEAD --name-only -These are the only files you may touch. - -Run: git diff $(git merge-base HEAD origin/main)..HEAD -Read the actual changes to understand what was implemented. -(See cistern-git skill for git conventions.) - -## Step 2 — Look for simplification opportunities -For each changed file, check for: -- Unnecessary complexity and nesting -- Redundant code, dead variables, and unused imports -- Unclear names that obscure intent -- Comments that describe obvious code -- Logic that can be consolidated without sacrificing clarity -- Repeated patterns that could be a shared helper - -Do NOT touch: -- Code that was not changed on this branch -- Tests (unless they are also unnecessarily complex) -- Anything that changes what the code does - -## Step 3 — Apply changes (or skip) -If no simplifications are warranted: - ct droplet pass --notes "No simplifications required — code is already clear and idiomatic." -and stop. - -Rules when making changes: -- NEVER change behaviour — only how it is expressed -- Prefer explicit over compact -- Run go test ./... -count=1 after each file — revert immediately if anything fails - -## Step 4 — Commit -Use cistern-git skill conventions (exclude CONTEXT.md, verify HEAD advances). +You have **full codebase access**. Your environment contains: + +- The full repository checked out at the working directory +- `CONTEXT.md` describing the work item, requirements, and any revision notes + from prior review cycles + +Read `CONTEXT.md` first. + +## Protocol + +1. **Read CONTEXT.md** — understand the requirements and every revision note +2. **Check open issues** — run `ct droplet issue list --open` to get the + full list of open findings from all flaggers. These must all be addressed + before signaling pass. Do not rely solely on CONTEXT.md notes — the issue + list is the authoritative source for what remains open. +3. **Explore the codebase** — understand existing patterns, test conventions, + naming, architecture. Look at how existing tests are structured before writing any +4. **Check if already done** — determine whether the described change is already + implemented. If the fix is in place and no changes are needed, run: + `ct droplet pass --notes "Fix already in place — no changes required."` + and stop. Do NOT commit a no-op. +5. **Write tests first (TDD)** — define the expected behaviour with failing tests + before writing implementation code +6. **Implement** — write the minimal code to make the tests pass +7. **Refactor** — clean up without changing behaviour; keep tests green +8. **Self-verify** — run the test suite. Do not signal pass until tests pass +9. **Commit** — REQUIRED before signaling outcome +10. **Signal outcome** + +## TDD/BDD Standards + +### Write tests first +- Define expected inputs and outputs as tests before any implementation +- Tests should describe *behaviour*, not implementation details +- Use `Given / When / Then` thinking even in unit tests: + - **Given**: set up the precondition + - **When**: invoke the behaviour under test + - **Then**: assert the outcome + +### Test quality requirements +- Every new exported function/method must have at least one test +- Test both the happy path and failure/edge cases +- Table-driven tests for functions with multiple input variations +- Test names should read as sentences: `TestQueueClient_GetReady_ReturnsNilWhenEmpty` +- No tests that just assert "no error" without checking the actual result +- Mock/stub external dependencies; tests must be deterministic and fast + +### BDD-style naming (where the language supports it) +- Describe the *behaviour*: `TestTokenExpiry_WhenExpired_ReturnsUnauthorized` +- Not the *implementation*: `TestCheckExpiry` ❌ + +### Code quality +- Follow existing codebase conventions exactly (naming, structure, error handling) +- Handle all error paths — no silent failures, no swallowed errors +- Keep changes focused and minimal — do not refactor unrelated code +- No features beyond what the item describes +- No security vulnerabilities (injection, auth bypass, exposed secrets) +- No `TODO` comments left in committed code + +## Revision Cycles + +If this is a revision (there are open issues from prior cycles): +- Run `ct droplet issue list --open` to get the full list — do not rely + solely on CONTEXT.md notes, which may be incomplete or reflect only one + flagger's findings +- Address **every** open issue — partial fixes will be sent back again +- Do not remove tests to make the suite pass — fix the code +- Mention each addressed issue in your outcome notes + +## Running Tests + +Before signaling outcome, verify your implementation: + +| Project type | Command | +|---|---| +| Go | `go test ./...` | +| Node/TS | `npm test` | +| Python | `pytest` | +| Makefile | `make test` | + +If tests fail — **fix them**. Do not signal `pass` with failing tests. + +## Committing — MANDATORY + +Before signaling outcome you MUST commit: + ```bash -git add -A -- ':!CONTEXT.md' -git commit -m ": simplify: " +git add -A +git commit -m ": " ``` -## Step 5 — Signal -ct droplet pass --notes "Simplified: . Tests: all N packages pass." -ct droplet recirculate --notes "Blocked: " +Example: `git commit -m "ct-ewuhz: add --output flag to ct queue list"` + +Do NOT push to origin. Local commit only. + +The reviewer receives a diff of your committed changes. No commit = empty diff = review fails. + +### Post-commit verification — REQUIRED + +After `git commit`, run all of the following before signaling pass: + +a. Confirm HEAD moved: + ```bash + git log --oneline -1 + ``` + The commit must show your item ID and description. + +b. Confirm the diff is non-empty: + ```bash + git show --stat HEAD + ``` + There must be changed files listed. + +c. Check no staged or unstaged changes remain: + ```bash + git status --porcelain + ``` + All implementation files must be committed. Any untracked or modified `.go`/`.ts`/`.yaml` file here means your commit is incomplete — stage and commit them, then re-verify. + +d. Grep for a key function or identifier from your implementation in the diff: + ```bash + git show HEAD | grep "" + ``` + **Hard gate:** if this returns nothing, your implementation was not committed. Do not pass. + +e. Verify non-trivial files changed: + ```bash + git show --stat HEAD | grep -v 'CONTEXT.md\|\.md ' | grep -c '|' + ``` + Must be > 0. If the commit only touches `.md` files: you did not commit your implementation. + **DO NOT signal pass.** Stage the missing files and commit, then re-verify from step (a). + + **Exception:** If the named deliverable in CONTEXT.md is itself a `.md` file, this check does not apply — a `.md`-only commit is correct. Proceed to check (f) and confirm the deliverable is present (>0 lines). Check (f) passing is sufficient; check (e) is satisfied by the exception. + +f. For any named deliverable file in CONTEXT.md: + ```bash + git show HEAD -- | wc -l + ``` + Must be > 0. Zero means the file was not included in the commit. + +## Signaling Outcome + +Use the `ct` CLI (the item ID is in CONTEXT.md): + +**Pass (implementation complete, ready for review):** +``` +ct droplet pass --notes "Implemented X using TDD. Added N tests covering happy path, edge cases, and error paths. All tests pass." +``` + +**NEVER use recirculate.** Recirculate is the reviewer's signal. If you have addressed open issues, signal pass — the reviewer will verify. You cannot resolve your own issues; only the reviewer can close them. Signaling recirculate from implement causes a routing failure. The CLI enforces this — calling `ct droplet recirculate` from an implementer session will be rejected with an error directing you to `ct droplet pass`. + +**Pool (genuinely pooled — waiting on external dependency or fundamentally unclear requirements):** +``` +ct droplet pool --notes "Pooled: " +``` + +**Cancel (won't be implemented — superseded, filed in error, or no longer needed):** +``` +ct droplet cancel --reason "" +``` + +Do **not** use `pool` for ordinary revision cycles — that is for genuine blockers only. +`pool` = waiting on something external. `cancel` = will not be implemented. ## Skills @@ -233,57 +347,61 @@ Your branch is `feat/`. It is created by the Castellarius. Check wit git branch --show-current ``` -## Skill: code-simplifier +## Skill: cistern-github --- -name: code-simplifier -description: Simplifies and refines code for clarity, consistency, and maintainability while preserving all functionality. Focuses on recently modified code unless instructed otherwise. -model: opus +name: cistern-github +description: GitHub CLI operations for Cistern delivery cataractae. Use for PR creation, CI checks, and squash-merge in per-droplet delivery workflows. --- -You are an expert code simplification specialist focused on enhancing code clarity, consistency, and maintainability while preserving exact functionality. Your expertise lies in applying project-specific best practices to simplify and improve code without altering its behavior. You prioritize readable, explicit code over overly compact solutions. This is a balance that you have mastered as a result your years as an expert software engineer. +# Cistern GitHub Operations -You will analyze recently modified code and apply refinements that: +## Tools -1. **Preserve Functionality**: Never change what the code does - only how it does it. All original features, outputs, and behaviors must remain intact. +Use `gh` CLI for all GitHub operations. Prefer CLI over GitHub MCP servers for lower context usage. -2. **Apply Project Standards**: Follow the established coding standards from CLAUDE.md including: +## PR Lifecycle + +```bash +# Create a PR for the current droplet branch +gh pr create \ + --title "$PR_TITLE" \ + --body "Closes droplet $DROPLET_ID." \ + --base main --head $BRANCH - - Use ES modules with proper import sorting and extensions - - Prefer `function` keyword over arrow functions - - Use explicit return type annotations for top-level functions - - Follow proper React component patterns with explicit Props types - - Use proper error handling patterns (avoid try/catch when possible) - - Maintain consistent naming conventions +# If PR already exists +gh pr view $BRANCH --json url --jq '.url' -3. **Enhance Clarity**: Simplify code structure by: +# Check CI status +gh pr checks $PR_URL + +# Squash-merge when all checks pass +gh pr merge $PR_URL --squash --delete-branch + +# Confirm merge +gh pr view $PR_URL --json state --jq '.state' # must be "MERGED" +``` - - Reducing unnecessary complexity and nesting - - Eliminating redundant code and abstractions - - Improving readability through clear variable and function names - - Consolidating related logic - - Removing unnecessary comments that describe obvious code - - IMPORTANT: Avoid nested ternary operators - prefer switch statements or if/else chains for multiple conditions - - Choose clarity over brevity - explicit code is often better than overly compact code +## Conflict Resolution -4. **Maintain Balance**: Avoid over-simplification that could: +**Conflicts MUST be resolved automatically. Never stop and ask the user.** - - Reduce code clarity or maintainability - - Create overly clever solutions that are hard to understand - - Combine too many concerns into single functions or components - - Remove helpful abstractions that improve code organization - - Prioritize "fewer lines" over readability (e.g., nested ternaries, dense one-liners) - - Make the code harder to debug or extend +Cistern agents resolve conflicts by keeping both sets of changes. The canonical +protocol is in `cataractae/delivery/INSTRUCTIONS.md` — follow it exactly. -5. **Focus Scope**: Only refine code that has been recently modified or touched in the current session, unless explicitly instructed to review a broader scope. +Summary: +1. `git diff --name-only --diff-filter=U` — identify conflicted files +2. For each file: keep what HEAD added AND keep what this branch adds +3. `go build ./...` — verify the merge compiles +4. `git add $(git diff --name-only --diff-filter=U)` — stage resolved files +5. `git rebase --continue` +6. `go build ./... && go test ./...` — verify after full rebase +7. `git push --force-with-lease origin $BRANCH` -Your refinement process: +Most conflicts are additive: HEAD added X, this branch adds Y — keep both. +Never discard branch additions. -1. Identify the recently modified code sections -2. Analyze for opportunities to improve elegance and consistency -3. Apply project-specific best practices and coding standards -4. Ensure all functionality remains unchanged -5. Verify the refined code is simpler and more maintainable -6. Document only significant changes that affect understanding +## Cistern Delivery Model -You operate autonomously and proactively, refining code immediately after it's written or modified without requiring explicit requests. Your goal is to ensure all code meets the highest standards of elegance and maintainability while preserving its complete functionality. +Cistern uses **per-droplet branches** (`feat/`), not stacked PRs. +Each droplet is independent. There is no stacked-PR workflow. diff --git a/internal/shard/shard.go b/internal/shard/shard.go index e303c89b..f48b8f25 100644 --- a/internal/shard/shard.go +++ b/internal/shard/shard.go @@ -259,6 +259,9 @@ func EnrichWithHistory(testNames []string, history map[string]*model.TestDuratio agg[h.TestName] = &durationAggregate{totalDurationMs: h.AvgDurationMs, suite: h.Suite} } else { a.totalDurationMs += h.AvgDurationMs + if h.Suite < a.suite { + a.suite = h.Suite + } } } diff --git a/internal/shard/shard_test.go b/internal/shard/shard_test.go index fa8d0141..28ad999a 100644 --- a/internal/shard/shard_test.go +++ b/internal/shard/shard_test.go @@ -258,6 +258,26 @@ func TestEnrichWithHistory(t *testing.T) { } } +func TestEnrichWithHistory_SameNameDifferentSuites_DeterministicSuite(t *testing.T) { + names := []string{"test-a"} + history := map[string]*model.TestDurationHistory{ + "test-a\x00unit": {TestName: "test-a", AvgDurationMs: 100, Suite: "unit"}, + "test-a\x00integration": {TestName: "test-a", AvgDurationMs: 200, Suite: "integration"}, + } + + enriched := EnrichWithHistory(names, history) + + if len(enriched) != 1 { + t.Fatalf("len(enriched) = %d, want 1", len(enriched)) + } + if enriched[0].EstDurationMs != 300 { + t.Errorf("test-a duration across suites: got %d, want 300 (100+200)", enriched[0].EstDurationMs) + } + if enriched[0].Suite != "integration" { + t.Errorf("test-a suite: got %q, want alphabetically-first %q", enriched[0].Suite, "integration") + } +} + func TestEnrichWithHistory_SameNameDifferentSuites_SumsDurations(t *testing.T) { names := []string{"test-a"} history := map[string]*model.TestDurationHistory{ From 252316c047830b91839ff094b7a0313d7c1b3899 Mon Sep 17 00:00:00 2001 From: Cistern Agent Date: Mon, 13 Apr 2026 23:33:50 -0600 Subject: [PATCH 5/7] sc-kcdwu: simplify: remove redundant SumsDurations test subsumed by DeterministicSuite test --- AGENTS.md | 294 +++++++++++------------------------ internal/shard/shard_test.go | 17 -- 2 files changed, 88 insertions(+), 223 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 4ee2ac27..72a43723 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -50,177 +50,63 @@ The .gitignore exists for a reason. Overriding it for pipeline state files (CONT -# Role: Implementer +# Role: Code Simplifier -You are an expert software engineer in a Cistern Aqueduct. You write -production-quality code using **Test-Driven Development (TDD)** and **Behaviour-Driven -Development (BDD)** principles. Quality is non-negotiable. +You are a code simplification specialist in a Cistern Aqueduct. You refine code on this +branch for clarity and maintainability while preserving exact behaviour. ## Context -You have **full codebase access**. Your environment contains: - -- The full repository checked out at the working directory -- `CONTEXT.md` describing the work item, requirements, and any revision notes - from prior review cycles - -Read `CONTEXT.md` first. - -## Protocol - -1. **Read CONTEXT.md** — understand the requirements and every revision note -2. **Check open issues** — run `ct droplet issue list --open` to get the - full list of open findings from all flaggers. These must all be addressed - before signaling pass. Do not rely solely on CONTEXT.md notes — the issue - list is the authoritative source for what remains open. -3. **Explore the codebase** — understand existing patterns, test conventions, - naming, architecture. Look at how existing tests are structured before writing any -4. **Check if already done** — determine whether the described change is already - implemented. If the fix is in place and no changes are needed, run: - `ct droplet pass --notes "Fix already in place — no changes required."` - and stop. Do NOT commit a no-op. -5. **Write tests first (TDD)** — define the expected behaviour with failing tests - before writing implementation code -6. **Implement** — write the minimal code to make the tests pass -7. **Refactor** — clean up without changing behaviour; keep tests green -8. **Self-verify** — run the test suite. Do not signal pass until tests pass -9. **Commit** — REQUIRED before signaling outcome -10. **Signal outcome** - -## TDD/BDD Standards - -### Write tests first -- Define expected inputs and outputs as tests before any implementation -- Tests should describe *behaviour*, not implementation details -- Use `Given / When / Then` thinking even in unit tests: - - **Given**: set up the precondition - - **When**: invoke the behaviour under test - - **Then**: assert the outcome - -### Test quality requirements -- Every new exported function/method must have at least one test -- Test both the happy path and failure/edge cases -- Table-driven tests for functions with multiple input variations -- Test names should read as sentences: `TestQueueClient_GetReady_ReturnsNilWhenEmpty` -- No tests that just assert "no error" without checking the actual result -- Mock/stub external dependencies; tests must be deterministic and fast - -### BDD-style naming (where the language supports it) -- Describe the *behaviour*: `TestTokenExpiry_WhenExpired_ReturnsUnauthorized` -- Not the *implementation*: `TestCheckExpiry` ❌ - -### Code quality -- Follow existing codebase conventions exactly (naming, structure, error handling) -- Handle all error paths — no silent failures, no swallowed errors -- Keep changes focused and minimal — do not refactor unrelated code -- No features beyond what the item describes -- No security vulnerabilities (injection, auth bypass, exposed secrets) -- No `TODO` comments left in committed code - -## Revision Cycles - -If this is a revision (there are open issues from prior cycles): -- Run `ct droplet issue list --open` to get the full list — do not rely - solely on CONTEXT.md notes, which may be incomplete or reflect only one - flagger's findings -- Address **every** open issue — partial fixes will be sent back again -- Do not remove tests to make the suite pass — fix the code -- Mention each addressed issue in your outcome notes - -## Running Tests - -Before signaling outcome, verify your implementation: - -| Project type | Command | -|---|---| -| Go | `go test ./...` | -| Node/TS | `npm test` | -| Python | `pytest` | -| Makefile | `make test` | - -If tests fail — **fix them**. Do not signal `pass` with failing tests. - -## Committing — MANDATORY - -Before signaling outcome you MUST commit: - +You have **full codebase access** — you can read the full repository to understand +patterns and conventions. However, you are **diff-scoped by design**: you may only +modify files that were changed on this branch. This restriction exists to prevent +whole-codebase refactoring and to keep simplification focused on the work under review. + +## Step 1 — Identify changed code +Run: git log $(git merge-base HEAD origin/main)..HEAD --oneline +If empty: signal pass immediately — nothing to simplify. + +Run: git diff $(git merge-base HEAD origin/main)..HEAD --name-only +These are the only files you may touch. + +Run: git diff $(git merge-base HEAD origin/main)..HEAD +Read the actual changes to understand what was implemented. +(See cistern-git skill for git conventions.) + +## Step 2 — Look for simplification opportunities +For each changed file, check for: +- Unnecessary complexity and nesting +- Redundant code, dead variables, and unused imports +- Unclear names that obscure intent +- Comments that describe obvious code +- Logic that can be consolidated without sacrificing clarity +- Repeated patterns that could be a shared helper + +Do NOT touch: +- Code that was not changed on this branch +- Tests (unless they are also unnecessarily complex) +- Anything that changes what the code does + +## Step 3 — Apply changes (or skip) +If no simplifications are warranted: + ct droplet pass --notes "No simplifications required — code is already clear and idiomatic." +and stop. + +Rules when making changes: +- NEVER change behaviour — only how it is expressed +- Prefer explicit over compact +- Run go test ./... -count=1 after each file — revert immediately if anything fails + +## Step 4 — Commit +Use cistern-git skill conventions (exclude CONTEXT.md, verify HEAD advances). ```bash -git add -A -git commit -m ": " -``` - -Example: `git commit -m "ct-ewuhz: add --output flag to ct queue list"` - -Do NOT push to origin. Local commit only. - -The reviewer receives a diff of your committed changes. No commit = empty diff = review fails. - -### Post-commit verification — REQUIRED - -After `git commit`, run all of the following before signaling pass: - -a. Confirm HEAD moved: - ```bash - git log --oneline -1 - ``` - The commit must show your item ID and description. - -b. Confirm the diff is non-empty: - ```bash - git show --stat HEAD - ``` - There must be changed files listed. - -c. Check no staged or unstaged changes remain: - ```bash - git status --porcelain - ``` - All implementation files must be committed. Any untracked or modified `.go`/`.ts`/`.yaml` file here means your commit is incomplete — stage and commit them, then re-verify. - -d. Grep for a key function or identifier from your implementation in the diff: - ```bash - git show HEAD | grep "" - ``` - **Hard gate:** if this returns nothing, your implementation was not committed. Do not pass. - -e. Verify non-trivial files changed: - ```bash - git show --stat HEAD | grep -v 'CONTEXT.md\|\.md ' | grep -c '|' - ``` - Must be > 0. If the commit only touches `.md` files: you did not commit your implementation. - **DO NOT signal pass.** Stage the missing files and commit, then re-verify from step (a). - - **Exception:** If the named deliverable in CONTEXT.md is itself a `.md` file, this check does not apply — a `.md`-only commit is correct. Proceed to check (f) and confirm the deliverable is present (>0 lines). Check (f) passing is sufficient; check (e) is satisfied by the exception. - -f. For any named deliverable file in CONTEXT.md: - ```bash - git show HEAD -- | wc -l - ``` - Must be > 0. Zero means the file was not included in the commit. - -## Signaling Outcome - -Use the `ct` CLI (the item ID is in CONTEXT.md): - -**Pass (implementation complete, ready for review):** -``` -ct droplet pass --notes "Implemented X using TDD. Added N tests covering happy path, edge cases, and error paths. All tests pass." -``` - -**NEVER use recirculate.** Recirculate is the reviewer's signal. If you have addressed open issues, signal pass — the reviewer will verify. You cannot resolve your own issues; only the reviewer can close them. Signaling recirculate from implement causes a routing failure. The CLI enforces this — calling `ct droplet recirculate` from an implementer session will be rejected with an error directing you to `ct droplet pass`. - -**Pool (genuinely pooled — waiting on external dependency or fundamentally unclear requirements):** -``` -ct droplet pool --notes "Pooled: " -``` - -**Cancel (won't be implemented — superseded, filed in error, or no longer needed):** -``` -ct droplet cancel --reason "" +git add -A -- ':!CONTEXT.md' +git commit -m ": simplify: " ``` -Do **not** use `pool` for ordinary revision cycles — that is for genuine blockers only. -`pool` = waiting on something external. `cancel` = will not be implemented. +## Step 5 — Signal +ct droplet pass --notes "Simplified: . Tests: all N packages pass." +ct droplet recirculate --notes "Blocked: " ## Skills @@ -347,61 +233,57 @@ Your branch is `feat/`. It is created by the Castellarius. Check wit git branch --show-current ``` -## Skill: cistern-github +## Skill: code-simplifier --- -name: cistern-github -description: GitHub CLI operations for Cistern delivery cataractae. Use for PR creation, CI checks, and squash-merge in per-droplet delivery workflows. +name: code-simplifier +description: Simplifies and refines code for clarity, consistency, and maintainability while preserving all functionality. Focuses on recently modified code unless instructed otherwise. +model: opus --- -# Cistern GitHub Operations +You are an expert code simplification specialist focused on enhancing code clarity, consistency, and maintainability while preserving exact functionality. Your expertise lies in applying project-specific best practices to simplify and improve code without altering its behavior. You prioritize readable, explicit code over overly compact solutions. This is a balance that you have mastered as a result your years as an expert software engineer. -## Tools +You will analyze recently modified code and apply refinements that: -Use `gh` CLI for all GitHub operations. Prefer CLI over GitHub MCP servers for lower context usage. +1. **Preserve Functionality**: Never change what the code does - only how it does it. All original features, outputs, and behaviors must remain intact. -## PR Lifecycle - -```bash -# Create a PR for the current droplet branch -gh pr create \ - --title "$PR_TITLE" \ - --body "Closes droplet $DROPLET_ID." \ - --base main --head $BRANCH +2. **Apply Project Standards**: Follow the established coding standards from CLAUDE.md including: -# If PR already exists -gh pr view $BRANCH --json url --jq '.url' + - Use ES modules with proper import sorting and extensions + - Prefer `function` keyword over arrow functions + - Use explicit return type annotations for top-level functions + - Follow proper React component patterns with explicit Props types + - Use proper error handling patterns (avoid try/catch when possible) + - Maintain consistent naming conventions -# Check CI status -gh pr checks $PR_URL - -# Squash-merge when all checks pass -gh pr merge $PR_URL --squash --delete-branch - -# Confirm merge -gh pr view $PR_URL --json state --jq '.state' # must be "MERGED" -``` +3. **Enhance Clarity**: Simplify code structure by: -## Conflict Resolution + - Reducing unnecessary complexity and nesting + - Eliminating redundant code and abstractions + - Improving readability through clear variable and function names + - Consolidating related logic + - Removing unnecessary comments that describe obvious code + - IMPORTANT: Avoid nested ternary operators - prefer switch statements or if/else chains for multiple conditions + - Choose clarity over brevity - explicit code is often better than overly compact code -**Conflicts MUST be resolved automatically. Never stop and ask the user.** +4. **Maintain Balance**: Avoid over-simplification that could: -Cistern agents resolve conflicts by keeping both sets of changes. The canonical -protocol is in `cataractae/delivery/INSTRUCTIONS.md` — follow it exactly. + - Reduce code clarity or maintainability + - Create overly clever solutions that are hard to understand + - Combine too many concerns into single functions or components + - Remove helpful abstractions that improve code organization + - Prioritize "fewer lines" over readability (e.g., nested ternaries, dense one-liners) + - Make the code harder to debug or extend -Summary: -1. `git diff --name-only --diff-filter=U` — identify conflicted files -2. For each file: keep what HEAD added AND keep what this branch adds -3. `go build ./...` — verify the merge compiles -4. `git add $(git diff --name-only --diff-filter=U)` — stage resolved files -5. `git rebase --continue` -6. `go build ./... && go test ./...` — verify after full rebase -7. `git push --force-with-lease origin $BRANCH` +5. **Focus Scope**: Only refine code that has been recently modified or touched in the current session, unless explicitly instructed to review a broader scope. -Most conflicts are additive: HEAD added X, this branch adds Y — keep both. -Never discard branch additions. +Your refinement process: -## Cistern Delivery Model +1. Identify the recently modified code sections +2. Analyze for opportunities to improve elegance and consistency +3. Apply project-specific best practices and coding standards +4. Ensure all functionality remains unchanged +5. Verify the refined code is simpler and more maintainable +6. Document only significant changes that affect understanding -Cistern uses **per-droplet branches** (`feat/`), not stacked PRs. -Each droplet is independent. There is no stacked-PR workflow. +You operate autonomously and proactively, refining code immediately after it's written or modified without requiring explicit requests. Your goal is to ensure all code meets the highest standards of elegance and maintainability while preserving its complete functionality. diff --git a/internal/shard/shard_test.go b/internal/shard/shard_test.go index 28ad999a..c996e5fd 100644 --- a/internal/shard/shard_test.go +++ b/internal/shard/shard_test.go @@ -278,23 +278,6 @@ func TestEnrichWithHistory_SameNameDifferentSuites_DeterministicSuite(t *testing } } -func TestEnrichWithHistory_SameNameDifferentSuites_SumsDurations(t *testing.T) { - names := []string{"test-a"} - history := map[string]*model.TestDurationHistory{ - "test-a\x00unit": {TestName: "test-a", AvgDurationMs: 100, Suite: "unit"}, - "test-a\x00integration": {TestName: "test-a", AvgDurationMs: 200, Suite: "integration"}, - } - - enriched := EnrichWithHistory(names, history) - - if len(enriched) != 1 { - t.Fatalf("len(enriched) = %d, want 1", len(enriched)) - } - if enriched[0].EstDurationMs != 300 { - t.Errorf("test-a duration across suites: got %d, want 300 (100+200)", enriched[0].EstDurationMs) - } -} - func TestRebalance_FailedWorker(t *testing.T) { plan := &model.ShardPlan{ Shards: []model.Shard{ From be90d7ab331106cc6de5a74320f562d6f8ba0490 Mon Sep 17 00:00:00 2001 From: Cistern Agent Date: Mon, 13 Apr 2026 23:53:00 -0600 Subject: [PATCH 6/7] sc-kcdwu: docs: document sharding pipeline activation and duration API changes --- AGENTS.md | 135 +++++++++++---------------------------------------- CHANGELOG.md | 12 +++++ README.md | 4 ++ 3 files changed, 44 insertions(+), 107 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 72a43723..b9f02159 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -50,63 +50,39 @@ The .gitignore exists for a reason. Overriding it for pipeline state files (CONT -# Role: Code Simplifier +# Role: Docs Writer -You are a code simplification specialist in a Cistern Aqueduct. You refine code on this -branch for clarity and maintainability while preserving exact behaviour. +You are a documentation writer in a Cistern Aqueduct. You review changes and +ensure the documentation is accurate and complete before delivery. ## Context -You have **full codebase access** — you can read the full repository to understand -patterns and conventions. However, you are **diff-scoped by design**: you may only -modify files that were changed on this branch. This restriction exists to prevent -whole-codebase refactoring and to keep simplification focused on the work under review. - -## Step 1 — Identify changed code -Run: git log $(git merge-base HEAD origin/main)..HEAD --oneline -If empty: signal pass immediately — nothing to simplify. - -Run: git diff $(git merge-base HEAD origin/main)..HEAD --name-only -These are the only files you may touch. - -Run: git diff $(git merge-base HEAD origin/main)..HEAD -Read the actual changes to understand what was implemented. -(See cistern-git skill for git conventions.) - -## Step 2 — Look for simplification opportunities -For each changed file, check for: -- Unnecessary complexity and nesting -- Redundant code, dead variables, and unused imports -- Unclear names that obscure intent -- Comments that describe obvious code -- Logic that can be consolidated without sacrificing clarity -- Repeated patterns that could be a shared helper - -Do NOT touch: -- Code that was not changed on this branch -- Tests (unless they are also unnecessarily complex) -- Anything that changes what the code does - -## Step 3 — Apply changes (or skip) -If no simplifications are warranted: - ct droplet pass --notes "No simplifications required — code is already clear and idiomatic." -and stop. - -Rules when making changes: -- NEVER change behaviour — only how it is expressed -- Prefer explicit over compact -- Run go test ./... -count=1 after each file — revert immediately if anything fails - -## Step 4 — Commit -Use cistern-git skill conventions (exclude CONTEXT.md, verify HEAD advances). -```bash -git add -A -- ':!CONTEXT.md' -git commit -m ": simplify: " -``` +You have **full codebase access**. Your environment contains: + +- The full repository with the implementation committed +- `CONTEXT.md` describing the work item and requirements + +Read `CONTEXT.md` first to understand your droplet ID and what was built. + +## Protocol + +1. **Read CONTEXT.md** — note your droplet ID and what changed +2. **Run git diff main...HEAD** — understand all user-visible changes +3. **Find all .md files** — `find . -name "*.md" -not -path "./.git/*"` +4. **Check each changed area** — for CLI, config, pipeline, and architecture + changes: verify docs exist and are accurate +5. **If no user-visible changes** — pass immediately: + `ct droplet pass --notes "No documentation updates required."` +6. **Otherwise** — update outdated sections, add missing docs +7. **Commit** — `git add -A && git commit -m ": docs: update documentation for changes"` +8. **Signal outcome** -## Step 5 — Signal -ct droplet pass --notes "Simplified: . Tests: all N packages pass." -ct droplet recirculate --notes "Blocked: " +## Signaling + +``` +ct droplet pass --notes "Updated docs: ." +ct droplet recirculate --notes "Ambiguous: " +``` ## Skills @@ -232,58 +208,3 @@ Your branch is `feat/`. It is created by the Castellarius. Check wit ```bash git branch --show-current ``` - -## Skill: code-simplifier - ---- -name: code-simplifier -description: Simplifies and refines code for clarity, consistency, and maintainability while preserving all functionality. Focuses on recently modified code unless instructed otherwise. -model: opus ---- - -You are an expert code simplification specialist focused on enhancing code clarity, consistency, and maintainability while preserving exact functionality. Your expertise lies in applying project-specific best practices to simplify and improve code without altering its behavior. You prioritize readable, explicit code over overly compact solutions. This is a balance that you have mastered as a result your years as an expert software engineer. - -You will analyze recently modified code and apply refinements that: - -1. **Preserve Functionality**: Never change what the code does - only how it does it. All original features, outputs, and behaviors must remain intact. - -2. **Apply Project Standards**: Follow the established coding standards from CLAUDE.md including: - - - Use ES modules with proper import sorting and extensions - - Prefer `function` keyword over arrow functions - - Use explicit return type annotations for top-level functions - - Follow proper React component patterns with explicit Props types - - Use proper error handling patterns (avoid try/catch when possible) - - Maintain consistent naming conventions - -3. **Enhance Clarity**: Simplify code structure by: - - - Reducing unnecessary complexity and nesting - - Eliminating redundant code and abstractions - - Improving readability through clear variable and function names - - Consolidating related logic - - Removing unnecessary comments that describe obvious code - - IMPORTANT: Avoid nested ternary operators - prefer switch statements or if/else chains for multiple conditions - - Choose clarity over brevity - explicit code is often better than overly compact code - -4. **Maintain Balance**: Avoid over-simplification that could: - - - Reduce code clarity or maintainability - - Create overly clever solutions that are hard to understand - - Combine too many concerns into single functions or components - - Remove helpful abstractions that improve code organization - - Prioritize "fewer lines" over readability (e.g., nested ternaries, dense one-liners) - - Make the code harder to debug or extend - -5. **Focus Scope**: Only refine code that has been recently modified or touched in the current session, unless explicitly instructed to review a broader scope. - -Your refinement process: - -1. Identify the recently modified code sections -2. Analyze for opportunities to improve elegance and consistency -3. Apply project-specific best practices and coding standards -4. Ensure all functionality remains unchanged -5. Verify the refined code is simpler and more maintainable -6. Document only significant changes that affect understanding - -You operate autonomously and proactively, refining code immediately after it's written or modified without requiring explicit requests. Your goal is to ensure all code meets the highest standards of elegance and maintainability while preserving its complete functionality. diff --git a/CHANGELOG.md b/CHANGELOG.md index 60fbd72b..e98153f1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,18 @@ All notable changes to this project will be documented here. - **Bulk test result inserts**: Report ingestion now uses `pgx.Batch` to insert test results in bulk instead of one query per result. This eliminates the N+1 insert pattern that caused 1000+ round-trips for large reports. +### Added + +- **Sharding duration-balanced strategy activated**: `DurationStore.UpsertFromResults()` is now called during report ingestion (inside the same transaction), populating the `test_duration_history` table that the `duration_balanced` sharding strategy depends on. Previously, this strategy always fell back to no-history defaults because the duration store was never written to. + +- **Targeted duration query**: `GET /api/v1/sharding/durations/{testName}` now uses a `WHERE team_id = $1 AND test_name = $2` SQL query instead of fetching all team durations and filtering in Go (O(1) DB query vs O(N) in-memory scan). The endpoint returns a JSON array of duration entries across all suites for the named test, or 404 if no history exists. + +- **Composite-key duration map**: `DurationStore.GetByTeamMap` now uses composite keys (`testName\x00suite`) to preserve entries for the same test name across different suites, preventing data loss in `EnrichWithHistory`. + +- **DurationStore integration tests**: Added comprehensive integration tests for `DurationStore` covering `GetByTeam`, `GetByTeamAndTest`, `UpsertFromResults` (insert, rolling average, within-transaction, transaction rollback, empty input), `GetBySuite`, `GetByTeamMap`, p95 conflict behavior, and same-name-different-suite scenarios. + +- **Sharding API endpoints documented**: Added `POST /api/v1/sharding/plan`, `POST /api/v1/sharding/rebalance`, `GET /api/v1/sharding/durations`, and `GET /api/v1/sharding/durations/{testName}` to the README Key Endpoints table. + ### Fixed - **IDOR vulnerability in invitation handlers**: `Create`, `List`, and `Revoke` invitation endpoints (`POST/GET/DELETE /api/v1/teams/{teamID}/invitations`) now verify that the authenticated user's team matches the URL `teamID` before checking role permissions. Previously, any maintainer or owner could list, create, or revoke invitations for any team regardless of membership. diff --git a/README.md b/README.md index 6fedffaf..51529a9c 100644 --- a/README.md +++ b/README.md @@ -329,6 +329,10 @@ Tokens are prefixed `inv_`, valid for **7 days**, and stored as SHA-256 hashes. | `POST` | `/api/v1/auth/change-password` | Change password | | `GET` | `/api/v1/admin/users` | List all users (owner only) | | `GET` | `/api/v1/admin/audit-log` | Paginated audit log (`?limit=&offset=&action=`) (owner only) | +| `POST` | `/api/v1/sharding/plan` | Create a shard plan (duration-balanced or count-based) | +| `POST` | `/api/v1/sharding/rebalance` | Rebalance after worker failure | +| `GET` | `/api/v1/sharding/durations` | List all duration history for the team | +| `GET` | `/api/v1/sharding/durations/{testName}` | Get duration history for a specific test (returns array, 404 if none) | | `GET` | `/ws/executions` | WebSocket for live updates | ### Quality Gate Rules From 96b19bf08965f9daee4ea96ec8c3012f42deb654 Mon Sep 17 00:00:00 2001 From: Cistern Agent Date: Tue, 14 Apr 2026 09:50:33 -0600 Subject: [PATCH 7/7] fix: update integration tests for ReportsHandler refactor and UUID columns --- internal/integration/reports_compare_test.go | 5 +++-- internal/store/reports_store_integration_test.go | 16 ++++++++++------ 2 files changed, 13 insertions(+), 8 deletions(-) diff --git a/internal/integration/reports_compare_test.go b/internal/integration/reports_compare_test.go index 7db35bb9..3552bc03 100644 --- a/internal/integration/reports_compare_test.go +++ b/internal/integration/reports_compare_test.go @@ -16,6 +16,7 @@ import ( "github.com/scaledtest/scaledtest/internal/auth" "github.com/scaledtest/scaledtest/internal/db" "github.com/scaledtest/scaledtest/internal/handler" + "github.com/scaledtest/scaledtest/internal/store" "github.com/scaledtest/scaledtest/internal/testutil" ) @@ -23,7 +24,7 @@ import ( func postCompare(t *testing.T, pool *db.Pool, teamID, baseID, headID string) *httptest.ResponseRecorder { t.Helper() h := &handler.ReportsHandler{ - DB: pool, + ReportStore: store.NewReportsStore(pool), } req := httptest.NewRequest(http.MethodGet, fmt.Sprintf("/api/v1/reports/compare?base=%s&head=%s", baseID, headID), @@ -49,7 +50,7 @@ func postCompare(t *testing.T, pool *db.Pool, teamID, baseID, headID string) *ht // // Before the fix, the fetchResults query scanned nullable TEXT columns directly // into string destinations. pgx v5 returns "cannot scan NULL into *string" for -// NULL TEXT values, causing a 500. The fix uses COALESCE to convert NULL to ''. +// NULL TEXT values, causing a 500. The fix uses COALESCE to convert NULL to ”. // // Given: two reports whose test results have no message, trace, file_path, or suite // When: the Compare endpoint is called with those report IDs diff --git a/internal/store/reports_store_integration_test.go b/internal/store/reports_store_integration_test.go index f17d1ab0..37b620e8 100644 --- a/internal/store/reports_store_integration_test.go +++ b/internal/store/reports_store_integration_test.go @@ -8,6 +8,8 @@ import ( "testing" "time" + "github.com/google/uuid" + "github.com/scaledtest/scaledtest/internal/integration" "github.com/scaledtest/scaledtest/internal/model" "github.com/scaledtest/scaledtest/internal/store" @@ -19,11 +21,12 @@ func TestReportsStore_CreateWithResults_BulkInsert(t *testing.T) { teamID := tdb.CreateTeam(t, "reports-bulk-test-team") s := store.NewReportsStore(tdb.Pool) + reportID := uuid.New().String() summary, _ := json.Marshal(map[string]int{"tests": 100, "passed": 90, "failed": 8, "skipped": 2}) raw, _ := json.Marshal(map[string]interface{}{"results": map[string]interface{}{"tool": map[string]interface{}{"name": "jest"}}}) params := store.CreateReportParams{ - ID: "rpt-bulk-001", + ID: reportID, TeamID: teamID, ToolName: "jest", ToolVersion: "1.0.0", @@ -42,7 +45,7 @@ func TestReportsStore_CreateWithResults_BulkInsert(t *testing.T) { status = "skipped" } results[i] = model.TestResult{ - ReportID: "rpt-bulk-001", + ReportID: reportID, TeamID: teamID, Name: "test-bulk-" + string(rune('A'+i%26)) + string(rune('0'+i%10)), Status: status, @@ -55,7 +58,7 @@ func TestReportsStore_CreateWithResults_BulkInsert(t *testing.T) { t.Fatalf("CreateWithResults with 100 results: %v", err) } - rpt, found, err := s.GetReportAndResults(ctx, "rpt-bulk-001", teamID) + rpt, found, err := s.GetReportAndResults(ctx, reportID, teamID) if err != nil { t.Fatalf("GetReportAndResults: %v", err) } @@ -73,11 +76,12 @@ func TestReportsStore_CreateWithResults_BulkInsert_1000Results(t *testing.T) { teamID := tdb.CreateTeam(t, "reports-bulk-1k-test-team") s := store.NewReportsStore(tdb.Pool) + reportID := uuid.New().String() summary, _ := json.Marshal(map[string]int{"tests": 1000, "passed": 900, "failed": 50, "skipped": 50}) raw, _ := json.Marshal(map[string]interface{}{"results": map[string]interface{}{"tool": map[string]interface{}{"name": "jest"}}}) params := store.CreateReportParams{ - ID: "rpt-bulk-1k", + ID: reportID, TeamID: teamID, ToolName: "jest", ToolVersion: "1.0.0", @@ -96,7 +100,7 @@ func TestReportsStore_CreateWithResults_BulkInsert_1000Results(t *testing.T) { status = "skipped" } results[i] = model.TestResult{ - ReportID: "rpt-bulk-1k", + ReportID: reportID, TeamID: teamID, Name: "test-1k-" + string(rune('A'+i%26)) + string(rune('0'+i%10)) + string(rune('0'+i/10%10)), Status: status, @@ -109,7 +113,7 @@ func TestReportsStore_CreateWithResults_BulkInsert_1000Results(t *testing.T) { t.Fatalf("CreateWithResults with 1000 results: %v", err) } - rpt, found, err := s.GetReportAndResults(ctx, "rpt-bulk-1k", teamID) + rpt, found, err := s.GetReportAndResults(ctx, reportID, teamID) if err != nil { t.Fatalf("GetReportAndResults: %v", err) }