diff --git a/CHANGELOG.md b/CHANGELOG.md index a76f9937..3ca7f7a7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,10 @@ Please choose versions by [Semantic Versioning](http://semver.org/). * MINOR version when you add functionality in a backwards-compatible manner, and * PATCH version when you make backwards-compatible bug fixes. +## Unreleased + +- fix: `vault-cli task set claude_session_id ` appends the displaced owner to `metrics_sessions` before writing the new id, so a session handed off by a take-over stays resolvable by `session-self-exit.py` and `close-worker.py`. A first bind and a re-bind to the same value append nothing; the completion and spawn-failure clears are unchanged. + ## v0.176.1 - fix: **`session-close`'s mode-2 closer is now a combinable menu, so a session that wants both the reflection and the self-improvement can answer for both on the attention board.** The closer emitted `pick — 1. /vault-cli:reflect · 2. /coding:self-improve · 3. nothing — close`, and a closer reaches the board with no `tool_input` at all, so the card declared no cardinality and rendered as radios with "(pick one)" — the operator could answer "1 + 2" in the console and not on the board. It now emits `pick any — …`, the combinable form the global closer rule defines: checkboxes on the board, with `nothing — close` last and exclusive, so the escape clears the other picks and any pick clears it. The producer half ships in `bborbe/claude` (the `Stop` hook that parses the marker and marks the last option exclusive) and the rendering half in `bborbe/attention-controller` v0.61.0 (the `exclusive` field and its board rule); an older board ignores the unknown field and renders the escape as an ordinary checkbox rather than rejecting the card. `commands/session-close.md` only — no Go change. diff --git a/docs/work-on-session-lifecycle.md b/docs/work-on-session-lifecycle.md index 7bb054f9..9a389d69 100644 --- a/docs/work-on-session-lifecycle.md +++ b/docs/work-on-session-lifecycle.md @@ -73,11 +73,16 @@ The id is the load-bearing field — an id on disk means the session is resumabl the metrics entry is analytics. The append is deliberately not in the agent definition's `` list. -**The generic write verbs refuse the field.** `task set`, `task add` and `task remove` -refuse `metrics_sessions` before any mutation, with no `--force` bypass, because `set` -stores a scalar and `add` / `remove` comma-split into a list of scalars — both shapes -the dedicated list reader discards. That refusal lives in -`pkg/ops/metrics_session_write.go`. +**The generic write verbs refuse a caller-supplied value for the field.** `task set`, +`task add` and `task remove` refuse a `metrics_sessions` value supplied by the caller, +before any mutation, with no `--force` bypass, because `set` stores a scalar and `add` / +`remove` comma-split into a list of scalars — both shapes the dedicated list reader +discards. That refusal lives in `pkg/ops/metrics_session_write.go`. The one internal +exception is the `claude_session_id` write: `task set claude_session_id ` +that displaces a different non-empty owner appends that outgoing id to +`metrics_sessions` before storing the new id, so a taken-over session stays resolvable +by the exit and close paths. The append is internal to the session-id write, not a +caller-facing way to write the field. **The removal is its own verb.** `vault-cli task remove-metrics-session ` removes **every** entry carrying that id while preserving every other diff --git a/integration/task_set_session_takeover_test.go b/integration/task_set_session_takeover_test.go new file mode 100644 index 00000000..893daa33 --- /dev/null +++ b/integration/task_set_session_takeover_test.go @@ -0,0 +1,175 @@ +// Copyright (c) 2026 Benjamin Borbe All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package integration_test + +import ( + "encoding/json" + "os" + "os/exec" + "path/filepath" + "strings" + "time" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + "github.com/onsi/gomega/gexec" + "gopkg.in/yaml.v3" +) + +var _ = Describe("task set session take-over", func() { + const ( + sessionA = "AAAA" + sessionB = "BBBB" + ) + + var vaultPath, configPath string + var cleanup func() + + // The fixture's frontmatter keys are alphabetical, because the storage writer + // re-serializes the whole block in key order on every write. + alphaFrontmatter := `--- +page_type: task +status: in_progress +task_identifier: 11111111-1111-4111-8111-111111111111 +--- +body line +` + + BeforeEach(func() { + vaultPath, configPath, cleanup = createTempVault(map[string]string{ + "Alpha": alphaFrontmatter, + }) + }) + + AfterEach(func() { + cleanup() + }) + + runCommand := func(args ...string) *gexec.Session { + fullArgs := append([]string{"--config", configPath, "--vault", "test"}, args...) + session, err := gexec.Start( + exec.Command(binPath, fullArgs...), + GinkgoWriter, + GinkgoWriter, + ) + Expect(err).NotTo(HaveOccurred()) + return session + } + + bind := func(value string) { + session := runCommand("task", "set", "Alpha", "claude_session_id", value) + Eventually(session).Should(gexec.Exit(0)) + } + + readFile := func(path string) string { + content, err := os.ReadFile(path) //#nosec G304 -- test file + Expect(err).NotTo(HaveOccurred()) + return string(content) + } + + alphaPath := func() string { + return filepath.Join(vaultPath, "Tasks", "Alpha.md") + } + + // getValue reads one field through the CLI and returns its rendered value. + getValue := func(key string) string { + session := runCommand("task", "get", "Alpha", key, "--output", "json") + Eventually(session).Should(gexec.Exit(0)) + var parsed struct { + Value string `json:"value"` + } + Expect(json.Unmarshal(session.Out.Contents(), &parsed)).To(Succeed()) + return parsed.Value + } + + // frontmatterOf unmarshals only the frontmatter block; the markdown body + // would fail the whole-file unmarshal. + frontmatterOf := func(path string) map[string]any { + text := readFile(path) + Expect(strings.HasPrefix(text, "---\n")).To(BeTrue(), "no frontmatter opening") + rest := strings.TrimPrefix(text, "---\n") + end := strings.Index(rest, "\n---\n") + Expect(end).To(BeNumerically(">=", 0), "no frontmatter closing") + var parsed map[string]any + Expect(yaml.Unmarshal([]byte(rest[:end]), &parsed)).To(Succeed()) + return parsed + } + + // firstSessionEntry returns the session_id and started_at of the first + // metrics_sessions entry, as the YAML decoder resolved them. + firstSessionEntry := func(frontmatter map[string]any) (string, any) { + entries, ok := frontmatter["metrics_sessions"].([]any) + Expect(ok).To(BeTrue(), "metrics_sessions is not a list") + Expect(entries).NotTo(BeEmpty()) + entry, ok := entries[0].(map[string]any) + Expect(ok).To(BeTrue(), "metrics_sessions entry is not a map") + sessionID, ok := entry["session_id"].(string) + Expect(ok).To(BeTrue(), "session_id is not a string") + return sessionID, entry["started_at"] + } + + It("records the displaced owner and not the incoming one", func() { + bind(sessionA) + + start := time.Now() + bind(sessionB) + end := time.Now() + + Expect(getValue("claude_session_id")).To(Equal(sessionB)) + + rendered := getValue("metrics_sessions") + Expect(rendered).To(ContainSubstring("session_id:" + sessionA)) + Expect(rendered).NotTo(ContainSubstring("session_id:" + sessionB)) + + content := readFile(alphaPath()) + // The `- ` list-item prefix is load-bearing: a bare `session_id: BBBB` + // also matches the `claude_session_id: BBBB` line. + Expect(strings.Count(content, "- session_id: AAAA")).To(Equal(1)) + Expect(strings.Count(content, "- session_id: BBBB")).To(Equal(0)) + + sessionID, startedAt := firstSessionEntry(frontmatterOf(alphaPath())) + Expect(sessionID).To(Equal(sessionA)) + Expect(startedAt).NotTo(BeNil(), "started_at missing") + + switch value := startedAt.(type) { + case string: + // The shared time type renders a midnight-UTC instant as a date only. + if len(value) == len("2006-01-02") { + Expect(value).To(Equal(start.UTC().Format("2006-01-02"))) + return + } + parsed, err := time.Parse(time.RFC3339, value) + Expect(err).NotTo(HaveOccurred()) + Expect(parsed).NotTo(BeZero()) + Expect(parsed).To(BeTemporally(">=", start.Add(-time.Minute))) + Expect(parsed).To(BeTemporally("<=", end.Add(time.Minute))) + case time.Time: + Expect(value).NotTo(BeZero()) + Expect(value).To(BeTemporally(">=", start.Add(-time.Minute))) + Expect(value).To(BeTemporally("<=", end.Add(time.Minute))) + default: + Fail("started_at has an unexpected type") + } + }) + + It("records nothing on a first bind", func() { + bind(sessionB) + + Expect(getValue("claude_session_id")).To(Equal(sessionB)) + Expect(getValue("metrics_sessions")).To(BeEmpty()) + Expect(readFile(alphaPath())).NotTo(ContainSubstring("metrics_sessions:")) + }) + + It("records nothing when re-binding the same value", func() { + bind(sessionA) + bind(sessionB) + before := getValue("metrics_sessions") + + bind(sessionB) + + Expect(getValue("metrics_sessions")).To(Equal(before)) + Expect(strings.Count(readFile(alphaPath()), "- session_id:")).To(Equal(1)) + }) +}) diff --git a/pkg/ops/frontmatter.go b/pkg/ops/frontmatter.go index 52d28cfc..e11303e4 100644 --- a/pkg/ops/frontmatter.go +++ b/pkg/ops/frontmatter.go @@ -138,6 +138,11 @@ func (o *frontmatterSetOperation) Execute( // tell a cleared assignee from one that was never set. previousAssignee := task.Assignee() + // The displaced owner must be appended before SetField overwrites it, so the + // single WriteTask below persists the new id and the recorded outgoing id + // together. + o.appendDisplacedSession(task, key, value) + // One-step close-out: when this invocation sets a close-out status, persist // reason and successor first so both land in a single WriteTask. Fields are // written only when provided; non-close-out targets never receive them. @@ -170,6 +175,25 @@ func (o *frontmatterSetOperation) Execute( return nil } +// appendDisplacedSession appends the outgoing owner to metrics_sessions when a +// claude_session_id write replaces a different non-empty id. The exit and close +// paths resolve a session by matching claude_session_id or any metrics_sessions +// entry, so the displaced id must stay on the task or it can never be resolved +// again. A first bind (no previous owner) and a re-bind to the same value append +// nothing. The incoming id is deliberately not appended: work-on and the +// session-connect append are the writers that establish ownership, and appending +// both would double-count the incoming session. +func (o *frontmatterSetOperation) appendDisplacedSession(task *domain.Task, key, value string) { + previous := task.ClaudeSessionID() + if key != "claude_session_id" || previous == "" || previous == value { + return + } + task.AppendMetricsSession(domain.MetricsSession{ + SessionID: previous, + StartedAt: libtime.DateOrDateTime(o.currentDateTime.Now().Time()), + }) +} + // writeTaskCloseOutFieldsIfCloseOut writes the one-step close-out fields when // the target status is a close-out (aborted or completed), returning the first // write error. Fields are written only when provided; non-close-out targets diff --git a/pkg/ops/frontmatter_session_takeover_test.go b/pkg/ops/frontmatter_session_takeover_test.go new file mode 100644 index 00000000..c2af9550 --- /dev/null +++ b/pkg/ops/frontmatter_session_takeover_test.go @@ -0,0 +1,153 @@ +// Copyright (c) 2026 Benjamin Borbe All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package ops_test + +import ( + "context" + + "github.com/bborbe/errors" + libtime "github.com/bborbe/time" + libtimetest "github.com/bborbe/time/test" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + + "github.com/bborbe/vault-cli/mocks" + "github.com/bborbe/vault-cli/pkg/domain" + "github.com/bborbe/vault-cli/pkg/ops" +) + +var _ = Describe("FrontmatterSetOperation session take-over", func() { + const ( + sessionA = "AAAA" + sessionB = "BBBB" + pinnedAt = "2026-03-03T12:00:00Z" + ) + + var ( + ctx context.Context + mockTaskStorage *mocks.TaskStorage + setOp ops.FrontmatterSetOperation + task *domain.Task + bind func(value string) error + ) + + // seedTask replaces the task the storage returns, so a spec can control the + // owner and the metrics_sessions list the write accumulates onto. + seedTask := func(fields map[string]any) { + task = domain.NewTask(fields, domain.FileMetadata{Name: "Alpha"}, domain.Content("")) + mockTaskStorage.FindTaskByNameReturns(task, nil) + } + + BeforeEach(func() { + ctx = context.Background() + mockTaskStorage = &mocks.TaskStorage{} + mockTaskStorage.WriteTaskReturns(nil) + + pinned := libtime.NewCurrentDateTime() + pinned.SetNow(libtimetest.ParseDateTime(pinnedAt)) + setOp = ops.NewFrontmatterSetOperation( + mockTaskStorage, + pinned, + ops.NewEscalationPublisher("", "", &mocks.NotificationSenderFactory{}), + "personal", + "25 Tasks", + ) + + seedTask(map[string]any{"status": "in_progress"}) + + bind = func(value string) error { + return setOp.Execute(ctx, "/vault", "Alpha", "claude_session_id", value, "", "", "", false) + } + }) + + It("records the displaced owner on a take-over", func() { + Expect(bind(sessionA)).To(BeNil()) + Expect(bind(sessionB)).To(BeNil()) + + Expect(task.ClaudeSessionID()).To(Equal(sessionB)) + + sessions := task.MetricsSessions() + Expect(sessions).To(HaveLen(1)) + Expect(sessions[0].SessionID).To(Equal(sessionA)) + Expect(sessions[0].StartedAt).To( + Equal(libtime.DateOrDateTime(libtimetest.ParseDateTime(pinnedAt).Time())), + ) + + // One write per command: the outgoing id and the new owner land together. + Expect(mockTaskStorage.WriteTaskCallCount()).To(Equal(2)) + }) + + It("records nothing on a first bind", func() { + Expect(bind(sessionB)).To(BeNil()) + + Expect(task.ClaudeSessionID()).To(Equal(sessionB)) + Expect(task.MetricsSessions()).To(BeEmpty()) + Expect(mockTaskStorage.WriteTaskCallCount()).To(Equal(1)) + }) + + It("records nothing when re-binding the same value", func() { + Expect(bind(sessionA)).To(BeNil()) + Expect(bind(sessionA)).To(BeNil()) + + Expect(task.ClaudeSessionID()).To(Equal(sessionA)) + Expect(task.MetricsSessions()).To(BeEmpty()) + }) + + It("records the outgoing owner when the owner is cleared", func() { + Expect(bind(sessionA)).To(BeNil()) + Expect(bind("")).To(BeNil()) + + Expect(task.ClaudeSessionID()).To(BeEmpty()) + + sessions := task.MetricsSessions() + Expect(sessions).To(HaveLen(1)) + Expect(sessions[0].SessionID).To(Equal(sessionA)) + }) + + It("preserves existing entries in order and appends the displaced owner last", func() { + older := libtimetest.ParseDateTime("2026-01-01T08:00:00Z") + seedTask(map[string]any{ + "status": "in_progress", + "claude_session_id": sessionA, + "metrics_sessions": []any{ + map[string]any{ + "session_id": "OLDER", + "started_at": older.Time(), + }, + }, + }) + + Expect(bind(sessionB)).To(BeNil()) + + sessions := task.MetricsSessions() + Expect(sessions).To(HaveLen(2)) + Expect(sessions[0].SessionID).To(Equal("OLDER")) + Expect(sessions[0].StartedAt).To(Equal(libtime.DateOrDateTime(older.Time()))) + Expect(sessions[1].SessionID).To(Equal(sessionA)) + }) + + It("records nothing when another field is set on an owned task", func() { + seedTask(map[string]any{ + "status": "in_progress", + "claude_session_id": sessionA, + }) + + Expect( + setOp.Execute(ctx, "/vault", "Alpha", "phase", "execution", "", "", "", false), + ).To(BeNil()) + + Expect(task.ClaudeSessionID()).To(Equal(sessionA)) + Expect(task.MetricsSessions()).To(BeEmpty()) + }) + + It("writes nothing when the task cannot be read", func() { + mockTaskStorage.FindTaskByNameReturns(nil, errors.New(ctx, "boom")) + + err := bind(sessionB) + Expect(err).NotTo(BeNil()) + Expect(err.Error()).To(ContainSubstring("find task")) + Expect(mockTaskStorage.WriteTaskCallCount()).To(Equal(0)) + }) +}) diff --git a/prompts/spec-063-append-outgoing-session-on-rebind.md b/prompts/spec-063-append-outgoing-session-on-rebind.md new file mode 100644 index 00000000..140a5468 --- /dev/null +++ b/prompts/spec-063-append-outgoing-session-on-rebind.md @@ -0,0 +1,218 @@ +--- +status: draft +spec: [063-bug-take-over-bind-drops-outgoing-session] +created: "2026-10-20T14:46:00Z" +branch: dark-factory/bug-take-over-bind-drops-outgoing-session +--- + +# Record the displaced owner when a take-over re-binds a task's session id (spec 063, prompt 1 of 1) + + + + +- A take-over that re-binds a task to a new session now leaves the previous owner recorded on that task. +- The previous owner is appended to the same history field the exit and close paths already read, so a displaced session can still resolve its task and end. +- The new session id is still the one stored as the task's current owner. +- A first bind onto a task that has no owner records nothing extra. +- Re-binding the same session id records nothing. +- Deliberately clearing the owner — completion, spawn failure, and the explicit clear verb — keeps its current behaviour and records no history. +- Existing history entries keep their order and the new entry is appended last. +- Every caller of the set command inherits the behaviour, including the Vault UI. +- The session-lifecycle documentation no longer says the set command always refuses the history field. +- A regression test binds one session then another and fails against the old behaviour. + + + +Make `vault-cli task set claude_session_id ` append the displaced, different, non-empty owner to `metrics_sessions` before storing the new id, in the same single write. The exit and close paths resolve a session by matching `claude_session_id` or any `metrics_sessions` id; without the append a taken-over session matches nothing and can never exit or be closed. First binds, same-value re-binds, and the deliberate clears must keep their current behaviour. + + + +Read `CLAUDE.md` for project conventions. Then read these files fully, in this order: + +- `specs/in-progress/063-bug-take-over-bind-drops-outgoing-session.md` — Goal, Acceptance Criteria, Assumptions, Constraints, and Failure Modes. This prompt covers all of AC1–AC8. +- `pkg/ops/frontmatter.go` — `frontmatterSetOperation.Execute` and the refusals it runs before any mutation (`blockedBySetRefusal`, `metricsSessionsWriteRefusal`, `todoPlanningSetRefusal`, `flagProvenanceSetRefusal`). Note the existing `previousAssignee := task.Assignee()` pattern: read the prior value before mutating, then compose every field into the single `WriteTask` at the end. +- `pkg/ops/workon.go` — `persistSessionAndMetrics`. This is the intended contract on the other path: stamp `claude_session_id` only when empty, always append the incoming id. Do **not** change it. +- `pkg/ops/metrics_session_append.go` — the public `append-metrics-session` verb. It validates a UUID before reading the task. The internal append must **not** route through it: the spec deliberately uses `AAAA` / `BBBB`, which that verb refuses. +- `pkg/domain/task_frontmatter_metrics.go` — `MetricsSessions()` and `AppendMetricsSession(entry MetricsSession)`. The latter ignores an empty `SessionID` and appends without deduplicating. +- `pkg/domain/metrics.go` — the `MetricsSession` struct: `SessionID string` and `StartedAt libtime.DateOrDateTime`. +- `pkg/domain/task_frontmatter.go` — `ClaudeSessionID()` and `SetField`'s `"claude_session_id"` arm. +- `pkg/ops/metrics_session_write.go` — the absolute refusal for caller-supplied `metrics_sessions` writes. Leave it untouched. +- `pkg/ops/frontmatter_test.go` — the existing `Describe("FrontmatterSetOperation", …)` setup, especially the `"setting claude_session_id field"` context. Its default task has no `claude_session_id`, so it stays a first-bind case. +- `pkg/ops/metrics_session_append_test.go` — the clock-pinning pattern: `libtime.NewCurrentDateTime()` + `SetNow(libtimetest.ParseDateTime(...))`, and the typed-entry assertions. +- `integration/cli_test.go` — `createTempVault`, and the `Describe("task append-metrics-session", …)` block. Its local helpers (`runEntityCommand`, `readFile`, frontmatter parsing, `- session_id:` counting) are closure-scoped; declare your own in the new file. +- `docs/work-on-session-lifecycle.md` — the paragraph beginning `**The generic write verbs refuse the field.**` and the union statement at `**Per-entry removal is what the shared-session rule requires.**` + +Coding-plugin docs (in-container paths): +- `/home/node/.claude/plugins/marketplaces/coding/docs/go-error-wrapping-guide.md` — `errors.Wrap` / `errors.Errorf` from `github.com/bborbe/errors`; never `fmt.Errorf`. +- `/home/node/.claude/plugins/marketplaces/coding/docs/go-testing-guide.md` — Ginkgo v2 / Gomega, external test packages, `gexec` session assertions. +- `/home/node/.claude/plugins/marketplaces/coding/docs/changelog-guide.md` — `## Unreleased` placement and the required `fix:` prefix. + + + +1. **Add one unexported helper** to `pkg/ops/frontmatter.go`, immediately after `frontmatterSetOperation.Execute` and before `writeTaskCloseOutFieldsIfCloseOut`. Use exactly this signature and behaviour: + + ```go + // appendDisplacedSession appends the outgoing owner to metrics_sessions when a + // claude_session_id write replaces a different non-empty id. The exit and close + // paths resolve a session by matching claude_session_id or any metrics_sessions + // entry, so the displaced id must stay on the task or it can never be resolved + // again. A first bind (no previous owner) and a re-bind to the same value append + // nothing. The incoming id is deliberately not appended: work-on and the + // session-connect append are the writers that establish ownership, and appending + // both would double-count the incoming session. + func (o *frontmatterSetOperation) appendDisplacedSession(task *domain.Task, key, value string) { + previous := task.ClaudeSessionID() + if key != "claude_session_id" || previous == "" || previous == value { + return + } + task.AppendMetricsSession(domain.MetricsSession{ + SessionID: previous, + StartedAt: libtime.DateOrDateTime(o.currentDateTime.Now().Time()), + }) + } + ``` + + `domain` and `libtime` are already imported in `pkg/ops/frontmatter.go`. Do not add imports and do not add a new exported symbol. + +2. **Call the helper once, before any mutation of the session id.** In `frontmatterSetOperation.Execute`, immediately after `previousAssignee := task.Assignee()` and before `writeTaskCloseOutFieldsIfCloseOut`, add: + + ```go + o.appendDisplacedSession(task, key, value) + ``` + + The existing `task.SetField(ctx, key, value)` later in the function stores the new value, and the existing single `o.taskStorage.WriteTask(ctx, task)` persists both changes together. Do **not** add a second `WriteTask` and do **not** move the append after the write. + +3. **Preserve the existing refusal and clear behaviour.** + - `metricsSessionsWriteRefusal` keeps refusing a caller-supplied `metrics_sessions` value on `task set` / `task add` / `task remove`. The append is keyed on `claude_session_id`, so it never touches that path. + - `frontmatterClearOperation` (`vault-cli task clear claude_session_id`) is unchanged: it deletes the key and appends nothing. + - `persistSessionAndMetrics` in `pkg/ops/workon.go` is unchanged: stamp only when empty, always append the incoming id. + - The completion clear in `pkg/ops/complete.go` and the spawn-failure clear in `work-on` are unchanged and never route through this operation. + - `task set claude_session_id ""` is a replacement like any other: it appends the outgoing id before storing the empty value. + +4. **Add unit specs** in a new file `pkg/ops/frontmatter_session_takeover_test.go`, package `ops_test`, with the BSD header (`Copyright (c) 2026 Benjamin Borbe`). Wrap them in `var _ = Describe("FrontmatterSetOperation session take-over", func() { ... })`. Use this setup: + - `mockTaskStorage := &mocks.TaskStorage{}`. + - A pinned clock: `pinned := libtime.NewCurrentDateTime(); pinned.SetNow(libtimetest.ParseDateTime("2026-03-03T12:00:00Z"))`. + - `ops.NewFrontmatterSetOperation(mockTaskStorage, pinned, ops.NewEscalationPublisher("", "", &mocks.NotificationSenderFactory{}), "personal", "25 Tasks")`. + - A task `domain.NewTask(map[string]any{"status": "in_progress"}, domain.FileMetadata{Name: "Alpha"}, domain.Content(""))`, returned by `FindTaskByNameReturns`; `WriteTaskReturns(nil)`. + - A `bind := func(value string) error { return setOp.Execute(ctx, "/vault", "Alpha", "claude_session_id", value, "", "", "", false) }` helper. + + Use the non-UUID ids `"AAAA"` and `"BBBB"` from the spec, so the specs also prove the internal append does not go through the UUID-validating verb. Cover: + - **Take-over (AC1/AC3/AC4):** bind `AAAA`, then bind `BBBB`. `task.ClaudeSessionID()` is `"BBBB"`; `task.MetricsSessions()` has length 1; entry 0 is `SessionID == "AAAA"` and `StartedAt == libtime.DateOrDateTime(libtimetest.ParseDateTime("2026-03-03T12:00:00Z").Time())`; `WriteTaskCallCount() == 2` (one write per command). + - **First bind (AC2):** bind `BBBB` on an unowned task. `MetricsSessions()` is empty and the write count is 1. + - **Same value (AC5):** bind `AAAA` twice. `MetricsSessions()` is empty. + - **Empty replacement:** bind `AAAA`, then bind `""`. `ClaudeSessionID()` is `""` and `MetricsSessions()` has exactly `"AAAA"`. + - **Order preserved:** seed the task with `claude_session_id: "AAAA"` and a pre-existing `metrics_sessions` entry `"OLDER"` with a distinct timestamp; bind `"BBBB"`. The entries are `["OLDER", "AAAA"]` in that order and the older timestamp is unchanged. + - **Narrowness:** with `claude_session_id: "AAAA"`, execute `set phase execution`. No `metrics_sessions` entry appears. + - **Unreadable task (Failure Modes row 1):** `FindTaskByNameReturns(nil, errors.New(ctx, "boom"))`; the bind returns an error containing `"find task"` and `WriteTaskCallCount() == 0`. + +5. **Add binary-level specs** in a new file `integration/task_set_session_takeover_test.go`, package `integration_test`, with the BSD header. Wrap them in `var _ = Describe("task set session take-over", func() { ... })`. Use `createTempVault` with a single task `"Alpha"` whose frontmatter is alphabetically ordered, e.g.: + + ```text + --- + page_type: task + status: in_progress + task_identifier: 11111111-1111-4111-8111-111111111111 + --- + body line + ``` + + Run the real binary with `exec.Command(binPath, "--config", configPath, "--vault", "test", ...)` through `gexec.Start`, asserting with `Eventually(session).Should(gexec.Exit(N))`. Read the task at `/Tasks/Alpha.md` and parse its frontmatter with `gopkg.in/yaml.v3` (unmarshalling the whole file fails on the body). Cover: + - **AC1 + AC3 + AC4:** bind `AAAA`, then bind `BBBB` inside a `start := time.Now()` / `end := time.Now()` window. Afterwards: + - `task get Alpha claude_session_id --output json` has `value == "BBBB"`. + - `task get Alpha metrics_sessions --output json` has a `value` containing `session_id:AAAA` and not containing `session_id:BBBB`. + - On disk, `strings.Count(content, "- session_id: AAAA") == 1` and `strings.Count(content, "- session_id: BBBB") == 0`. The `- ` list-item prefix is load-bearing: a bare `session_id: BBBB` would also match `claude_session_id: BBBB`. + - The `AAAA` entry's `started_at` is non-empty and is not the zero time. Accept either a `string` or a `time.Time` from the YAML decoder. When it carries a time, it parses as RFC3339 and lies within `[start.Add(-time.Minute), end.Add(time.Minute)]`; the shared time type renders a midnight-UTC instant as a date only (`YYYY-MM-DD`), so accept that shape too and assert it equals `start.UTC().Format("2006-01-02")`. + - **AC2:** on a fresh vault, bind `BBBB` once. `task get Alpha metrics_sessions --output json` has `value == ""`, and the file contains no `metrics_sessions:` key. + - **AC5:** bind `AAAA`, then `BBBB`, capture the `metrics_sessions` value, bind `BBBB` again, and assert the value is identical and `strings.Count(content, "- session_id:") == 1`. + + Declare the command runner, file reader, frontmatter parser, and entry lookup as local closures in the new file; the sibling block's helpers are not visible. + +6. **Update `docs/work-on-session-lifecycle.md`** (AC8). Replace the paragraph beginning `**The generic write verbs refuse the field.**` with a version that keeps the caller-supplied refusal and adds the internal exception. Use this text: + + ```markdown + **The generic write verbs refuse a caller-supplied value for the field.** `task set`, + `task add` and `task remove` refuse a `metrics_sessions` value supplied by the caller, + before any mutation, with no `--force` bypass, because `set` stores a scalar and `add` / + `remove` comma-split into a list of scalars — both shapes the dedicated list reader + discards. That refusal lives in `pkg/ops/metrics_session_write.go`. The one internal + exception is the `claude_session_id` write: `task set claude_session_id ` + that displaces a different non-empty owner appends that outgoing id to + `metrics_sessions` before storing the new id, so a taken-over session stays resolvable + by the exit and close paths. The append is internal to the session-id write, not a + caller-facing way to write the field. + ``` + + Do not change the union statement later in the document, and do not change `docs/session-liveness.md` (it answers a narrower question keyed on `claude_session_id` alone). + +7. **Record the change in `CHANGELOG.md`.** Insert a `## Unreleased` section directly below the preamble (`All notable changes…` and the `* MAJOR / MINOR / PATCH` lines) and above the newest `## vX.Y.Z`, with one bullet: + + ```markdown + - fix: `vault-cli task set claude_session_id ` appends the displaced owner to `metrics_sessions` before writing the new id, so a session handed off by a take-over stays resolvable by `session-self-exit.py` and `close-worker.py`. A first bind and a re-bind to the same value append nothing; the completion and spawn-failure clears are unchanged. + ``` + + Do **not** bump any version string: this repo's release automation owns the four version fields (`CHANGELOG.md` top version, `.claude-plugin/plugin.json`, and both `.claude-plugin/marketplace.json` fields). Add the unreleased bullet only. + +8. **Self-check before finishing.** Walk AC1–AC8 of the spec against the diff, then run the full `` block. + + + +- No new CLI verb and no new flag: the behaviour attaches to the existing `task set` path. `pkg/cli/cli.go` needs no change. +- `metrics_sessions` stays refused through `task set` / `task add` / `task remove` for caller-supplied values. The append is internal to the `claude_session_id` write. Do not touch `pkg/ops/metrics_session_write.go`. +- `work-on`'s `persistSessionAndMetrics` keeps its current shape: stamp only when empty, always append the incoming id. Do not touch `pkg/ops/workon.go`. +- Only the outgoing id is appended. Do not append the incoming id: `work-on` and the session-connect append already record it, and appending both would double-count it. +- Existing well-formed `metrics_sessions` entries are preserved in order; the new entry is appended last. Go through `domain.TaskFrontmatter.AppendMetricsSession` — the same writer the work-on path and the public append verb use. Do not add a raw-preserving writer. +- The internal append takes the previous id verbatim, with no UUID validation. Do not route it through `AppendMetricsSessionOperation`, which refuses `AAAA`. +- The task file is written once per command, not twice. +- Two writers racing on the same task remain last-write-wins; do not add a lock, a re-read, or deduplication. Entries accumulate by design and a duplicate is harmless to both readers. +- `vault-cli task clear claude_session_id` keeps its current behaviour: delete the key, no append. +- Goal and topic set operations are out of scope; only the task path changes. +- `agents/work-on-task-assistant.md` needs no edit: its claim branch already appends the incoming id through `append-metrics-session`, and the new internal append records the outgoing id — different ids, so nothing is double-counted. +- No backfill of `metrics_sessions` onto existing tasks. +- Do NOT commit — dark-factory handles git. +- Existing tests must still pass. + + + +Run `make test` iteratively while implementing, then `make precommit` once at the end. `make precommit` must exit 0. + +Targeted runs: + +``` +go build ./... +go test -mod=mod ./pkg/ops/... -ginkgo.focus="session take-over" -ginkgo.v +go test -mod=mod ./integration/... -ginkgo.focus="session take-over" -ginkgo.v +``` + +`-ginkgo.focus` with zero matching specs exits 0 while running nothing. After each focused run, confirm the new spec names appear in the output; a log without them is a failure, not a pass. + +Evidence greps (run each and confirm the count): + +``` +grep -c 'func (o \*frontmatterSetOperation) appendDisplacedSession' pkg/ops/frontmatter.go # 1 +grep -c 'o.appendDisplacedSession(task, key, value)' pkg/ops/frontmatter.go # 1 +grep -c 'task.AppendMetricsSession' pkg/ops/frontmatter.go # 1 +grep -c 'func metricsSessionsWriteRefusal' pkg/ops/metrics_session_write.go # 1 (untouched) +grep -c 'func persistSessionAndMetrics' pkg/ops/workon.go # 1 (untouched) +grep -c 'FrontmatterSetOperation session take-over' pkg/ops/frontmatter_session_takeover_test.go # 1 +grep -c 'task set session take-over' integration/task_set_session_takeover_test.go # 1 +grep -c -- '- session_id: AAAA' integration/task_set_session_takeover_test.go # >= 1 +grep -c 'caller-supplied value for the field' docs/work-on-session-lifecycle.md # 1 +sed -n '/^## Unreleased/,/^## v/p' CHANGELOG.md | grep -c 'claude_session_id' # >= 1 +``` + +Syntax and formatting: + +``` +gofmt -e -l pkg/ops/frontmatter.go pkg/ops/frontmatter_session_takeover_test.go integration/task_set_session_takeover_test.go +``` + +The command must print nothing. `make precommit` runs `generate` (which recreates `mocks/`; the interface is unchanged, so the output is stable) and `addlicense` (which restores the BSD header on every `.go` file, including the two new test files). + diff --git a/specs/in-progress/063-bug-take-over-bind-drops-outgoing-session.md b/specs/in-progress/063-bug-take-over-bind-drops-outgoing-session.md new file mode 100644 index 00000000..a817328f --- /dev/null +++ b/specs/in-progress/063-bug-take-over-bind-drops-outgoing-session.md @@ -0,0 +1,207 @@ +--- +status: prompted +approved: "2026-10-20T14:08:55Z" +generating: "2026-10-20T14:38:21Z" +prompted: "2026-10-20T14:45:24Z" +branch: dark-factory/bug-take-over-bind-drops-outgoing-session +--- + +## Summary + +- Re-binding a task's `claude_session_id` to a new session overwrites the old owner and records it nowhere. +- The dropped session then matches no task, so no exit or close path can ever resolve it. +- `vault-cli work-on` already records every session it touches; only the direct `task set` path does not. +- The Vault UI reaches the same path, so a take-over started from the UI loses the owner too. +- Observed in production: session `60573438-cfc7-4b40-968f-065669208a4b` was displaced and stranded. + +## Problem + +A session's only link back to its work is the task frontmatter. `session-self-exit.py` and +`close-worker.py` both resolve a session by matching it against a task — the current +`claude_session_id` or a `metrics_sessions` entry. When a take-over overwrites +`claude_session_id` without recording the previous value, the outgoing session matches +nothing: `session-self-exit.py` refuses it with `no task carries claude_session_id ` and +`close-worker.py` with `no task file carries this session id as claude_session_id`. The session +holds its spawn slot and cannot be closed by any manager. + +`work-on` does not have this defect: it stamps the owner only when the field is empty and +always appends the incoming id to `metrics_sessions`. The direct set path has no equivalent. + +## Goal + +A `task set claude_session_id ` that replaces a non-empty value with a different +value leaves the previous owner recorded on that task, so the exit and close paths can still +resolve it. A first bind onto an unowned task keeps its current behaviour, and the deliberate +clears are unchanged — the completion clear in `pkg/ops/complete.go` and the spawn-failure +clear in `work-on` both intend to drop the owner, not preserve it. + +## Reproduction + +Run against a scratch vault so no real task is touched. + +```bash +rm -rf /tmp/vc-takeover-repro +mkdir -p /tmp/vc-takeover-repro/tasks + +cat > /tmp/vc-takeover-repro/config.yaml <<'EOF' +current_user: bborbe +default_vault: scratch +vaults: + scratch: + path: /tmp/vc-takeover-repro + name: scratch + tasks_dir: tasks +EOF + +printf -- '---\nstatus: in_progress\nphase: planning\nassignee: bborbe\n---\nTags: [[Task]]\n\n---\nScratch task.\n' \ + > "/tmp/vc-takeover-repro/tasks/Scratch Take-Over Repro.md" + +CFG=/tmp/vc-takeover-repro/config.yaml +T="Scratch Take-Over Repro" + +# 1. first bind: A +vault-cli --config $CFG task set "$T" claude_session_id AAAA --vault scratch +vault-cli --config $CFG task get "$T" metrics_sessions --vault scratch --output json + +# 2. take over to B +vault-cli --config $CFG task set "$T" claude_session_id BBBB --vault scratch +vault-cli --config $CFG task get "$T" claude_session_id --vault scratch --output json +vault-cli --config $CFG task get "$T" metrics_sessions --vault scratch --output json +``` + +Observed on 2026-10-20, vault-cli `0.174.1`: + +``` +=== step 1: bind A === +✅ Set claude_session_id=AAAA on: Scratch Take-Over Repro +{ "key": "metrics_sessions", "name": "Scratch Take-Over Repro", "value": ""} +=== step 2: take over to B === +✅ Set claude_session_id=BBBB on: Scratch Take-Over Repro +{ "key": "claude_session_id", "name": "Scratch Take-Over Repro", "value": "BBBB"} +{ "key": "metrics_sessions", "name": "Scratch Take-Over Repro", "value": ""} +``` + +`AAAA` appears nowhere on the task after step 2. + +Production instance of the same write, commit `9b0c4f5fc5` on the Personal vault: + +``` +-claude_session_id: 60573438-cfc7-4b40-968f-065669208a4b ++claude_session_id: bfd1ac2c-356d-42ab-b8fe-437c7a30ad18 ++metrics_sessions: ++ - session_id: bfd1ac2c-356d-42ab-b8fe-437c7a30ad18 +``` + +The incoming session was recorded; the outgoing one was dropped. + +## Expected vs Actual + +**Expected.** A take-over re-bind leaves the outgoing owner recorded on the task, in the same +field the exit and close paths already read (`metrics_sessions`). `metrics_sessions` is the +established mechanism: `work-on` appends to it, `session-self-exit.py` and `close-worker.py` +both resolve a handed-off session from it, and the sibling fix that shipped in vault-cli +`0.175.1` / supervisor `0.188.2` reads it. + +**Actual.** The outgoing owner is dropped. `metrics_sessions` stays empty, so neither reader +can resolve the session and it can never exit or be closed. + +## Why this is a bug + +The union is documented in `docs/work-on-session-lifecycle.md:96` — "the set of session ids +that collide on a task is read from `claude_session_id` **and** every `metrics_sessions` id" — +and restated in `commands/task-status.md:83`. The exit and close resolvers read that union; +`docs/session-liveness.md` answers the narrower four-state liveness question, which keys on +`claude_session_id` alone. `docs/work-on-session-lifecycle.md` also states that a +`metrics_sessions` entry "accumulates and never replaces". A write that replaces the owner and +appends nothing contradicts both: it makes the union smaller than the history of sessions that +worked the task. + +`pkg/ops/workon.go`'s `persistSessionAndMetrics` already implements the intended contract for +the other path — stamp only when empty, always append. The direct set path is the outlier. + +## Acceptance Criteria + +`$CFG` and `` are the ones from `## Reproduction` (`--config /tmp/vc-takeover-repro/config.yaml --vault scratch`). + +- [ ] **AC1 — a take-over records the outgoing owner.** After binding A then B to a scratch task, `vault-cli --config $CFG task get claude_session_id --vault scratch --output json` returns `BBBB` and `vault-cli --config $CFG task get metrics_sessions --vault scratch --output json` returns a value containing `AAAA`. +- [ ] **AC2 — a first bind records nothing.** On a scratch task with no `claude_session_id`, a single bind to `BBBB` leaves `vault-cli --config $CFG task get metrics_sessions --vault scratch --output json` returning `"value": ""`. +- [ ] **AC3 — the appended entry is well-formed.** The `metrics_sessions` value from AC1 contains `session_id:AAAA` and a `started_at:` timestamp on the same entry, in the shape the CLI already prints for a populated field (`map[session_id:… started_at:…]`). The timestamp is non-zero — `started_at:0001-01-01T00:00:00Z` fails — and falls inside the command's run window. +- [ ] **AC4 — the outgoing id is appended, not the incoming one.** After the A→B take-over, `grep -c -- '- session_id: BBBB' ""` returns 0 and `grep -c -- '- session_id: AAAA' ""` returns 1. ⚠️ Anchor on the list-item form: a bare `session_id: BBBB` also matches the `claude_session_id: BBBB` line, so the unanchored pattern reads 1 and fails a correct fix. +- [ ] **AC5 — re-binding the same value appends nothing.** Setting `claude_session_id` to the value it already holds leaves `metrics_sessions` unchanged — `vault-cli --config $CFG task get metrics_sessions --vault scratch --output json` returns the same value before and after. +- [ ] **AC6 — a regression test locks the behaviour.** A new test binds A, binds B, and asserts the outgoing id is in `metrics_sessions`; it fails against the pre-fix binary and passes after. `make test` exits 0, including the existing `persistSessionAndMetrics` tests. +- [ ] **AC7 — the whole suite is clean.** `make precommit` exits 0. +- [ ] **AC8 — the docs match the shipped behaviour.** `docs/work-on-session-lifecycle.md` no longer claims `task set` refuses `metrics_sessions` unconditionally: its sentence names the caller-supplied case as refused and the `claude_session_id` write as appending internally. + +## Verification + +### Container-executable (runs inside the YOLO container at prompt time) + +- `make test` — exits 0 +- `make precommit` — exits 0 +- the regression test named in AC6, run alone — exits 0 (it fails before the fix, so a pass proves the change landed) + +### Operator-executable (runs on the host after the fix ships) + +- The Reproduction block above, replayed against a freshly built binary: step 2's + `metrics_sessions` value contains `AAAA`, and step 1's is still `""`. +- `TASKS_ROOTS=/tmp/vc-takeover-repro/tasks SESSIONS_DIR=/tmp/vc-takeover-repro/sessions WEZTERM_PANE=1 session-self-exit.py --session AAAA --dry-run` prints `SELF-EXIT: exit — handed off to BBBB, no open card, pane 1`, where `/tmp/vc-takeover-repro/sessions/4242.json` holds `{"pid": 4242, "sessionId": "AAAA", "kind": "interactive"}`. All three overrides are required: `TASKS_ROOTS` because the script otherwise scans the real vaults, `SESSIONS_DIR` because `owns_pane()` refuses without a registry record for AAAA, and `WEZTERM_PANE` because `main()` refuses a non-numeric pane. + +## Desired Behavior + +1. `task set claude_session_id ` on a task currently owned by `A` appends + `{session_id: A, started_at: }` to `metrics_sessions` and then sets the field to `B`. +2. The same command on a task with no owner sets the field and appends nothing. +3. Setting the field to the value it already holds appends nothing. +4. Every caller of `task set claude_session_id` inherits the behaviour — the Vault UI binds + through that same command, so it needs no change of its own. + +## Assumptions + +- `metrics_sessions` is the right home for the outgoing owner: it is the field the exit and + close resolvers already read, and `work-on` already appends to it. +- `task set` reads the task before mutating it, so the previous value is available at write + time (`pkg/ops/frontmatter.go` reads, then `SetField`, then `WriteTask`). +- Only the outgoing id is appended. `work-on` appends the incoming id because it is the writer + that establishes ownership; a take-over only has to preserve what it displaces. Appending + both would double-count the incoming session once `work-on` runs. +- The completion clear (`pkg/ops/complete.go`) and the spawn-failure clear in `work-on` are + deliberate removals and stay out of scope. +- `task set claude_session_id ""` is a replacement like any other: it appends the outgoing + id before storing the empty value. `vault-cli task clear claude_session_id` is a + deliberate operator recovery path and is excluded — it deletes the key with no append. +- The internal append takes the previous id verbatim, with no UUID validation. The public + `append-metrics-session` verb validates a UUID, and the reproduction deliberately uses `AAAA` + / `BBBB`; routing the internal append through that verb would fail AC1. + +## Constraints + +- No new CLI verb or flag: the behaviour attaches to the existing `task set` path. +- `metrics_sessions` stays refused through `task set` / `task add` / `task remove` for + callers. The append is internal to the session-id write, not a new caller-facing verb. +- `work-on`'s `persistSessionAndMetrics` keeps its current shape: stamp only when empty, + always append the incoming id. +- Existing `metrics_sessions` entries are preserved in order; the new entry is appended. +- The task file is written once per command, not twice. +- `docs/work-on-session-lifecycle.md` ships updated in the same change: its sentence that + `task set` refuses `metrics_sessions` must name the caller-supplied case as refused and the + `claude_session_id` write as appending internally (AC8). +- `vault-cli task clear claude_session_id` keeps its current behaviour — delete the key, + no append. It is a deliberate recovery path, not a take-over. + +## Failure Modes + +| Trigger | Expected behavior | Recovery | +|---|---|---| +| Task file unreadable | command exits non-zero, file untouched | re-run once the file is readable; no partial state to repair | +| `metrics_sessions` holds a malformed entry | well-formed entries are preserved in order and the new entry is appended last. An entry with no `session_id` is dropped and a non-list value is normalized — the behaviour `work-on` and the public append verb already have, because all three go through `domain.TaskFrontmatter.AppendMetricsSession` | the dropped entry stays in the task file's git history; the fix itself needs no repair | +| Two writers race on the same task | last write wins; both sessions may appear in `metrics_sessions` | entries accumulate by design; a duplicate is harmless to both readers | +| Bind to the same value | no entry appended | none needed | + +## Suggested Decomposition + +| # | Prompt focus | Covers DBs | Covers ACs | Depends on | +|---|---|---|---|---| +| 1 | Append the outgoing owner on a session-id re-bind, with tests | 1–4 | 1–8 | — | + +Rationale: one layer (the task frontmatter's session-id write) and one observable behaviour. +A second prompt would have nothing of its own to change.