From fc532aba6aec953da6c35ad01228de96e349eda9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Flor=C3=AAncio?= Date: Sat, 5 Sep 2026 10:23:53 +0000 Subject: [PATCH] fix(terminal): support PostgreSQL task terminals (#3371) (#3378) --- .../terminal/repository/postgres_test.go | 146 ++++++++++++++++++ .../internal/terminal/repository/sqlite.go | 17 +- .../postgres-task-terminal-queries/plan.md | 78 ++++++++++ .../task-01-rebind-task-terminal-queries.md | 89 +++++++++++ docs/specs/tasks/README.md | 4 +- .../requirements/task-terminal-persistence.md | 45 ++++++ .../task-terminal-persistence.md | 74 +++++++++ 7 files changed, 444 insertions(+), 9 deletions(-) create mode 100644 apps/backend/internal/terminal/repository/postgres_test.go create mode 100644 docs/plans/postgres-task-terminal-queries/plan.md create mode 100644 docs/plans/postgres-task-terminal-queries/task-01-rebind-task-terminal-queries.md create mode 100644 docs/specs/tasks/requirements/task-terminal-persistence.md create mode 100644 docs/specs/tasks/system-design/task-terminal-persistence.md diff --git a/apps/backend/internal/terminal/repository/postgres_test.go b/apps/backend/internal/terminal/repository/postgres_test.go new file mode 100644 index 0000000000..15dc9ca673 --- /dev/null +++ b/apps/backend/internal/terminal/repository/postgres_test.go @@ -0,0 +1,146 @@ +package repository + +import ( + "context" + "errors" + "testing" + + "github.com/kandev/kandev/internal/terminal/models" + "github.com/kandev/kandev/internal/testutil" +) + +// @covers AC-TASKS-TASK-TERMINALS-001.1, AC-TASKS-TASK-TERMINALS-001.2, +// AC-TASKS-TASK-TERMINALS-001.3 +func TestPostgresTerminalRepositoryLifecycle(t *testing.T) { + t.Run("create get and rename", testPostgresTerminalCreateGetRename) + t.Run("list and state", testPostgresTerminalListAndState) + t.Run("delete", testPostgresTerminalDelete) +} + +func newPostgresTerminalTestRepo(t *testing.T) *Repository { + t.Helper() + db := testutil.OpenIsolatedPostgres(t, testutil.PostgresDSNFromEnv(t)) + repo, err := NewWithDB(db, db, nil) + if err != nil { + t.Fatalf("new postgres repository: %v", err) + } + return repo +} + +func testPostgresTerminalCreateGetRename(t *testing.T) { + repo := newPostgresTerminalTestRepo(t) + ctx := context.Background() + + first, err := repo.Create(ctx, "task-1", "env-1", "terminal-1", "make dev") + if err != nil { + t.Fatalf("create first terminal: %v", err) + } + second, err := repo.Create(ctx, "task-1", "env-1", "terminal-2", "") + if err != nil { + t.Fatalf("create second terminal: %v", err) + } + if first.Seq != 1 || first.State != models.StateOpen || first.InitialCommand != "make dev" { + t.Fatalf("first terminal = %#v, want seq 1, open state, and initial command", first) + } + if second.Seq != 2 { + t.Fatalf("second terminal seq = %d, want 2", second.Seq) + } + + name := "build watcher" + if err := repo.Rename(ctx, first.ID, &name); err != nil { + t.Fatalf("rename terminal: %v", err) + } + got, err := repo.Get(ctx, first.ID) + if err != nil { + t.Fatalf("get terminal: %v", err) + } + if got.CustomName == nil || *got.CustomName != name { + t.Fatalf("custom name = %v, want %q", got.CustomName, name) + } + if err := repo.Rename(ctx, first.ID, nil); err != nil { + t.Fatalf("clear terminal name: %v", err) + } + got, err = repo.Get(ctx, first.ID) + if err != nil { + t.Fatalf("get terminal after clear: %v", err) + } + if got.CustomName != nil { + t.Fatalf("custom name after clear = %v, want nil", got.CustomName) + } +} + +func testPostgresTerminalListAndState(t *testing.T) { + repo := newPostgresTerminalTestRepo(t) + ctx := context.Background() + first, err := repo.Create(ctx, "task-1", "env-1", "terminal-1", "") + if err != nil { + t.Fatalf("create first terminal: %v", err) + } + second, err := repo.Create(ctx, "task-1", "env-1", "terminal-2", "") + if err != nil { + t.Fatalf("create second terminal: %v", err) + } + if err := repo.SetState(ctx, second.ID, models.StateParked); err != nil { + t.Fatalf("park terminal: %v", err) + } + open, err := repo.ListByTask(ctx, "task-1", false) + if err != nil { + t.Fatalf("list open terminals: %v", err) + } + if len(open) != 1 || open[0].ID != first.ID { + t.Fatalf("open terminals = %#v, want only %q", open, first.ID) + } + all, err := repo.ListByTask(ctx, "task-1", true) + if err != nil { + t.Fatalf("list all terminals: %v", err) + } + if len(all) != 2 || all[0].ID != first.ID || all[1].ID != second.ID { + t.Fatalf("all terminals = %#v, want %q then %q", all, first.ID, second.ID) + } + + if err := repo.SetState(ctx, second.ID, models.StateOpen); err != nil { + t.Fatalf("resume terminal: %v", err) + } + open, err = repo.ListByTask(ctx, "task-1", false) + if err != nil { + t.Fatalf("list resumed terminals: %v", err) + } + if len(open) != 2 { + t.Fatalf("resumed terminal count = %d, want 2", len(open)) + } +} + +func testPostgresTerminalDelete(t *testing.T) { + repo := newPostgresTerminalTestRepo(t) + ctx := context.Background() + first, err := repo.Create(ctx, "task-1", "env-1", "terminal-1", "") + if err != nil { + t.Fatalf("create first terminal: %v", err) + } + if _, err := repo.Create(ctx, "task-1", "env-1", "terminal-2", ""); err != nil { + t.Fatalf("create second terminal: %v", err) + } + if _, err := repo.Create(ctx, "task-2", "env-2", "terminal-3", ""); err != nil { + t.Fatalf("create other task terminal: %v", err) + } + if err := repo.Delete(ctx, first.ID); err != nil { + t.Fatalf("delete terminal: %v", err) + } + if _, err := repo.Get(ctx, first.ID); !errors.Is(err, ErrNotFound) { + t.Fatalf("get deleted terminal error = %v, want ErrNotFound", err) + } + deleted, err := repo.DeleteByTask(ctx, "task-1") + if err != nil { + t.Fatalf("delete task terminals: %v", err) + } + if deleted != 1 { + t.Fatalf("deleted task terminal count = %d, want 1", deleted) + } + other, err := repo.ListByTask(ctx, "task-2", true) + if err != nil { + t.Fatalf("list other task terminals: %v", err) + } + if len(other) != 1 || other[0].ID != "terminal-3" { + t.Fatalf("other task terminals = %#v, want terminal-3", other) + } +} diff --git a/apps/backend/internal/terminal/repository/sqlite.go b/apps/backend/internal/terminal/repository/sqlite.go index 2d53618e97..77964b8800 100644 --- a/apps/backend/internal/terminal/repository/sqlite.go +++ b/apps/backend/internal/terminal/repository/sqlite.go @@ -74,9 +74,9 @@ func (r *Repository) initSchema() error { // agentctl PTY id). initialCommand is "" for plain shells. func (r *Repository) Create(ctx context.Context, taskID, envID, id, initialCommand string) (*models.Terminal, error) { if _, err := r.db.ExecContext(ctx, - `INSERT INTO user_terminals (id, task_id, environment_id, seq, custom_name, state, initial_command) + r.db.Rebind(`INSERT INTO user_terminals (id, task_id, environment_id, seq, custom_name, state, initial_command) SELECT ?, ?, ?, COALESCE(MAX(seq), 0) + 1, NULL, 'open', ? - FROM user_terminals WHERE task_id = ?`, + FROM user_terminals WHERE task_id = ?`), id, taskID, envID, initialCommand, taskID, ); err != nil { return nil, fmt.Errorf("insert terminal: %w", err) @@ -89,8 +89,8 @@ func (r *Repository) Create(ctx context.Context, taskID, envID, id, initialComma func (r *Repository) Get(ctx context.Context, id string) (*models.Terminal, error) { var t models.Terminal err := r.ro.GetContext(ctx, &t, - `SELECT id, task_id, environment_id, seq, custom_name, state, initial_command, created_at - FROM user_terminals WHERE id = ?`, + r.ro.Rebind(`SELECT id, task_id, environment_id, seq, custom_name, state, initial_command, created_at + FROM user_terminals WHERE id = ?`), id, ) if errors.Is(err, sql.ErrNoRows) { @@ -112,6 +112,7 @@ func (r *Repository) ListByTask(ctx context.Context, taskID string, includeParke q += ` AND state = 'open'` } q += ` ORDER BY seq ASC` + q = r.ro.Rebind(q) var rows []*models.Terminal if err := r.ro.SelectContext(ctx, &rows, q, args...); err != nil { @@ -130,7 +131,7 @@ func (r *Repository) Rename(ctx context.Context, id string, name *string) error nameArg = *name } res, err := r.db.ExecContext(ctx, - `UPDATE user_terminals SET custom_name = ? WHERE id = ?`, + r.db.Rebind(`UPDATE user_terminals SET custom_name = ? WHERE id = ?`), nameArg, id, ) if err != nil { @@ -147,7 +148,7 @@ func (r *Repository) Rename(ctx context.Context, id string, name *string) error // the service's job. func (r *Repository) SetState(ctx context.Context, id string, state models.TerminalState) error { res, err := r.db.ExecContext(ctx, - `UPDATE user_terminals SET state = ? WHERE id = ?`, + r.db.Rebind(`UPDATE user_terminals SET state = ? WHERE id = ?`), string(state), id, ) if err != nil { @@ -162,7 +163,7 @@ func (r *Repository) SetState(ctx context.Context, id string, state models.Termi // Delete removes a single terminal row. func (r *Repository) Delete(ctx context.Context, id string) error { - _, err := r.db.ExecContext(ctx, `DELETE FROM user_terminals WHERE id = ?`, id) + _, err := r.db.ExecContext(ctx, r.db.Rebind(`DELETE FROM user_terminals WHERE id = ?`), id) if err != nil { return fmt.Errorf("delete terminal: %w", err) } @@ -172,7 +173,7 @@ func (r *Repository) Delete(ctx context.Context, id string) error { // DeleteByTask removes every row for a task and returns the count. Used by // the task.deleted cascade subscriber. func (r *Repository) DeleteByTask(ctx context.Context, taskID string) (int, error) { - res, err := r.db.ExecContext(ctx, `DELETE FROM user_terminals WHERE task_id = ?`, taskID) + res, err := r.db.ExecContext(ctx, r.db.Rebind(`DELETE FROM user_terminals WHERE task_id = ?`), taskID) if err != nil { return 0, fmt.Errorf("delete by task: %w", err) } diff --git a/docs/plans/postgres-task-terminal-queries/plan.md b/docs/plans/postgres-task-terminal-queries/plan.md new file mode 100644 index 0000000000..deafba941c --- /dev/null +++ b/docs/plans/postgres-task-terminal-queries/plan.md @@ -0,0 +1,78 @@ +--- +created: 2026-09-04 +status: done +requirements: + - REQ-TASKS-TASK-TERMINALS-001 +system_design: + - ../../specs/tasks/system-design/task-terminal-persistence.md +legacy_specs: [] +--- + +# Implementation Plan: PostgreSQL Task Terminal Queries + +## Overview + +Make every parameterized ordinary task-terminal query use placeholder syntax +for the selected database driver. One work order adds PostgreSQL lifecycle +coverage first, applies the mechanical repository correction, and runs the +targeted backend checks. + +## Scope + +### In scope + +- Add an environment-gated PostgreSQL regression for the ordinary terminal + repository lifecycle. +- Rebind every parameterized read and write in the ordinary terminal + repository. +- Preserve existing SQLite behavior and error wrapping. + +### Out of scope + +- Changes to terminal service, WebSocket handlers, or web components. +- Changes to terminal sequence allocation or concurrency semantics. +- New database schema or migrations. +- New terminal error presentation. + +## Technical approach + +Add `postgres_test.go` beside the existing SQLite repository tests. The test +constructs the real repository over an isolated PostgreSQL schema and exercises +all parameterized lifecycle methods. It must first fail with PostgreSQL syntax +error `42601` on the current implementation. + +Update `apps/backend/internal/terminal/repository/sqlite.go` so reads call +`r.ro.Rebind` and writes call `r.db.Rebind` before passing statements to sqlx. +Do not change schema statements because they have no placeholders. + +## Tests + +- `AC-TASKS-TASK-TERMINALS-001.1` maps to PostgreSQL create and get assertions + plus the existing SQLite sequence tests. +- `AC-TASKS-TASK-TERMINALS-001.2` maps to PostgreSQL open/all list assertions + and ordered sequence assertions. +- `AC-TASKS-TASK-TERMINALS-001.3` maps to PostgreSQL rename, state, single + delete, and task-wide delete assertions. + +## E2E tests + +The existing browser terminal suites cover the unchanged user, WebSocket, and +service flow. The new real-PostgreSQL lifecycle test covers the driver boundary +that caused the end-to-end failure. No frontend or protocol behavior changes. + +## Work orders + +- [x] [Task 01: Rebind Task Terminal Queries](task-01-rebind-task-terminal-queries.md) + +## Verification results + +- The PostgreSQL lifecycle regression passed three subtests on PostgreSQL 16. +- The terminal package suite passed 29 tests in three packages. +- The changed-file backend linter reported zero issues. + +## Risks + +- Omitting one parameterized method can leave a later lifecycle action broken + on PostgreSQL even if creation starts working. +- PostgreSQL integration coverage skips when `KANDEV_TEST_POSTGRES_DSN` is not + configured, so the implementation run must provide an isolated database. diff --git a/docs/plans/postgres-task-terminal-queries/task-01-rebind-task-terminal-queries.md b/docs/plans/postgres-task-terminal-queries/task-01-rebind-task-terminal-queries.md new file mode 100644 index 0000000000..5229f3924b --- /dev/null +++ b/docs/plans/postgres-task-terminal-queries/task-01-rebind-task-terminal-queries.md @@ -0,0 +1,89 @@ +--- +id: "01-rebind-task-terminal-queries" +title: "Rebind Task Terminal Queries" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +requirements: + - REQ-TASKS-TASK-TERMINALS-001 +acceptance_criteria: + - AC-TASKS-TASK-TERMINALS-001.1 + - AC-TASKS-TASK-TERMINALS-001.2 + - AC-TASKS-TASK-TERMINALS-001.3 +system_design: + - ../../specs/tasks/system-design/task-terminal-persistence.md +--- + +# Task 01: Rebind Task Terminal Queries + +## Summary + +Add a real PostgreSQL regression for the ordinary terminal descriptor +lifecycle. Then make each parameterized statement use placeholder syntax for +the selected driver. Preserve the current SQLite and terminal-service contracts. + +## In scope + +- Add PostgreSQL create, get, list, rename, state, removal, and task-wide removal + lifecycle assertions. +- Rebind all parameterized reads through the reader pool. +- Rebind all parameterized writes through the writer pool. + +## Out of scope + +- Schema, API, service, PTY, WebSocket, frontend, and localization changes. +- Sequence-allocation redesign or new concurrency behavior. +- Broad repository verification beyond the task-defined checks. + +## Acceptance + +- The PostgreSQL lifecycle regression fails before the production correction + with SQLSTATE `42601` and passes after it. +- Every parameterized ordinary terminal repository method works against both + SQLite and PostgreSQL with unchanged results. +- The targeted terminal packages and changed-file backend lint pass. + +## Verification + +```bash +go test ./internal/terminal/... -count=1 +KANDEV_TEST_POSTGRES_DSN= go test ./internal/terminal/repository -run TestPostgresTerminalRepositoryLifecycle -count=1 -v +golangci-lint run ./... --new-from-rev="$(git merge-base HEAD origin/main)" --timeout=5m +``` + +## Files likely touched + +- `apps/backend/internal/terminal/repository/sqlite.go` +- `apps/backend/internal/terminal/repository/postgres_test.go` + +## Dependencies + +None. + +## Risks + +- The PostgreSQL test must use a real isolated schema so a SQLite-only or mock + test cannot hide placeholder syntax errors. + +## Parallelism + +`sequential` + +## Inputs + +- `REQ-TASKS-TASK-TERMINALS-001` and its acceptance criteria. +- `docs/specs/tasks/system-design/task-terminal-persistence.md`. +- GitHub issue #3371 and the isolated PostgreSQL reproduction evidence. +- Existing terminal repository tests and `internal/testutil` PostgreSQL helpers. + +## Results + +- Added `postgres_test.go` with create, get, list, rename, state, single-removal, + and task-wide removal coverage. +- Added driver rebinding to `Create`, `Get`, `ListByTask`, `Rename`, `SetState`, + `Delete`, and `DeleteByTask`. +- `TestPostgresTerminalRepositoryLifecycle` failed before the correction with + SQLSTATE `42601`. It passed all three subtests after the correction. +- `go test ./internal/terminal/... -count=1` passed 29 tests in three packages. +- The changed-file backend linter reported zero issues. diff --git a/docs/specs/tasks/README.md b/docs/specs/tasks/README.md index a289246d4e..185cea4b2d 100644 --- a/docs/specs/tasks/README.md +++ b/docs/specs/tasks/README.md @@ -76,6 +76,7 @@ signals, and task-scoped scheduling contracts. - [Rich task title previews](requirements/rich-task-title-previews.md) - [Queued run scheduling](requirements/run-scheduling.md) - [Task Runtime Cleanup](requirements/runtime-cleanup.md) +- [Task Terminal Persistence](requirements/task-terminal-persistence.md) - [Runtime Task-State Publication Order](requirements/runtime-state-publication-order.md) - [Session Delete Preserves Task Workspaces](requirements/session-delete-resource-cleanup.md) - [Sidebar Task Editing](requirements/sidebar-task-edit.md) @@ -142,6 +143,7 @@ signals, and task-scoped scheduling contracts. - [Task plan write consistency](system-design/plan-write-consistency.md) - [Task plan write lifecycle](system-design/plan-write-lifecycle.md) - [Task Runtime Cleanup](system-design/runtime-cleanup.md) +- [Task Terminal Persistence](system-design/task-terminal-persistence.md) - [Runtime Task-State Publication Order](system-design/runtime-state-publication-order.md) - [Queued Run Scheduling](system-design/run-scheduling.md) - [Session Delete Preserves Task Workspaces](system-design/session-delete-resource-cleanup.md) @@ -158,7 +160,7 @@ signals, and task-scoped scheduling contracts. ## Migration record -Migration remains in progress. The six requirements above now have +Migration remains in progress. The seven requirements above now have authoritative, wrapper-free requirement/design pairs. Other migrated files still need the same extraction before this system can return to a complete migration state. diff --git a/docs/specs/tasks/requirements/task-terminal-persistence.md b/docs/specs/tasks/requirements/task-terminal-persistence.md new file mode 100644 index 0000000000..306533bea2 --- /dev/null +++ b/docs/specs/tasks/requirements/task-terminal-persistence.md @@ -0,0 +1,45 @@ +--- +status: active +system: tasks +created: 2026-09-04 +owners: + - kandev +--- + +# Task Terminal Persistence Requirements + +## Overview + +Ordinary task terminals are durable task-scoped resources. The task system owns +their identity and lifecycle. Each record belongs to one task and its execution +environment. These operations must remain available on every supported +persistence backend. + +## Requirements + +### REQ-TASKS-TASK-TERMINALS-001: Persistence backend parity + +**Intent:** Users can create and manage ordinary task terminals without the +configured supported persistence backend changing the terminal lifecycle. + +**User story:** As a Kandev user, I want task terminals to work on each supported +database. + +#### Acceptance criteria + +- **AC-TASKS-TASK-TERMINALS-001.1:** On SQLite and PostgreSQL, the system shall + persist and return each new ordinary task terminal with its sequence and open + state. +- **AC-TASKS-TASK-TERMINALS-001.2:** On SQLite and PostgreSQL, the system shall + list matching task terminals in sequence order and apply the requested parked + terminal filter. +- **AC-TASKS-TASK-TERMINALS-001.3:** On SQLite and PostgreSQL, each read, rename, + state change, single removal, and task-wide removal shall produce the same + persisted result. + +## Out of scope + +- Changing terminal numbering or concurrent sequence-allocation semantics. +- Changing PTY process lifetime, output buffering, or WebSocket transport. +- Changing how the web application presents a terminal persistence failure. +- Changing Quick Terminal or agent-login terminal behavior. diff --git a/docs/specs/tasks/system-design/task-terminal-persistence.md b/docs/specs/tasks/system-design/task-terminal-persistence.md new file mode 100644 index 0000000000..80a606ff90 --- /dev/null +++ b/docs/specs/tasks/system-design/task-terminal-persistence.md @@ -0,0 +1,74 @@ +--- +status: current +system: tasks +requirements: + - REQ-TASKS-TASK-TERMINALS-001 +--- + +# Task Terminal Persistence System Design + +## Purpose and boundaries + +This design defines persistence-backend parity for ordinary task-terminal +records. The existing terminal service and WebSocket handlers continue to own +validation, PTY lifecycle, authorization, and client responses. The repository +continues to own the `user_terminals` table and its task-scoped lifecycle. + +## Requirement mapping + +| Requirement | Design section | +| --- | --- | +| `REQ-TASKS-TASK-TERMINALS-001` | [Portable query execution](#portable-query-execution), [Test contract](#test-contract) | + +## Components and responsibilities + +- `apps/backend/internal/terminal/repository` persists ordinary terminal + descriptors and initializes `user_terminals`. +- `apps/backend/internal/terminal/service` preserves the existing ordinary, + fixed, and script-terminal boundaries and delegates descriptor persistence to + the repository. +- `apps/backend/internal/agent/handlers` exposes the existing `user_shell.*` + WebSocket actions and propagates repository failures. + +## Portable query execution + +The terminal repository keeps `?` as the source placeholder syntax. Before a +parameterized statement reaches the database driver, the repository passes it +through the `sqlx.DB` associated with that operation: + +- Writer operations use `r.db.Rebind`. +- Read operations use `r.ro.Rebind`. +- Schema statements contain no parameters and execute unchanged. + +This follows the established repository convention. SQLite retains `?` +placeholders, while PostgreSQL receives numbered placeholders. Query text, +arguments, ordering, filters, and lifecycle semantics otherwise remain +unchanged. + +## Failure and recovery + +Database execution errors continue to carry the current repository operation +context to the terminal service and WebSocket caller. The repair does not add a +retry or convert a failed persistence operation into an in-memory terminal. + +## Persistence + +The `user_terminals` schema, unique task-sequence constraint, state values, and +delete behavior remain unchanged. This repair changes only how parameterized +statements are prepared for the selected driver and requires no migration. + +## Test contract + +An environment-gated PostgreSQL repository test uses +`internal/testutil.OpenIsolatedPostgres` and exercises the complete descriptor +lifecycle: create, get, filtered and unfiltered list, rename, state changes, +single delete, and task-wide delete. The existing SQLite tests continue to pin +sequence allocation and lifecycle behavior. Existing browser terminal tests +cover the unchanged WebSocket and web flow. The PostgreSQL test covers the only +driver-specific boundary changed by this repair. + +## Related decisions + +No architecture decision record is required. Query rebinding is an established +repository convention, and this repair introduces no new ownership or +persistence boundary.