diff --git a/CHANGELOG.md b/CHANGELOG.md index b081ee5b2..5ab4d5d45 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -37,6 +37,7 @@ bus event types) are noted explicitly even in the `0.x` range. - **`task-completion-from-sent`** — re-check tasks are open and CEO-owned; fail closed on subtask-lookup errors. (#1419) - **Voice-learning capture** — a body-only draft edit refreshes the snapshot in place, preserving its original `created_at`. (#1419) - **`voice-learn`** — voice-guide approval loop no longer breaks after one cycle; honors dismiss cooldown. (#1419) +- **Learning-subsystem config writes** — soft-rejected `ConfigStore` writes now hold state instead of silently advancing. (#1438) ### Added @@ -70,6 +71,7 @@ bus event types) are noted explicitly even in the `0.x` range. - **Install docs** — README points to `releases/latest/download/install.sh` and documents `CURIA_VERSION`. - **`00-overview.md` Spec Index** — dropped the `Status` column and "N of M Done" counts. (#1282) - **Spec audit fixes** — corrected stale claims: Node 24, console UI, secrets vault, setup story, fallback-provider prose. (#1282) +- **Learning subsystem** — queue/status/guard state moved from OKF doc bodies to config-store JSON. (#1438) ### Removed diff --git a/docs/adr/029-passive-email-observation-and-counterfactual-competence.md b/docs/adr/029-passive-email-observation-and-counterfactual-competence.md index bddd72dc1..ceaec371a 100644 --- a/docs/adr/029-passive-email-observation-and-counterfactual-competence.md +++ b/docs/adr/029-passive-email-observation-and-counterfactual-competence.md @@ -45,7 +45,10 @@ Concrete mapping: | Learned voice | free-form `WritingVoice.guide`, maintained by a weekly batched LLM pass and approved via the digest | | Todo completion | `tasks` + `task-complete` / digest confirm | | Capability evidence | Pre-scored rows in `autonomy_action_log` | -| Cadence / watermarks / guard markers | Scheduler crons + `config-store` | +| Cadence / watermarks | Scheduler crons + `config-store` (`ceo_inbox`) | +| Queue / status / guard state (completion candidates, digest items, voice proposal, matched-draft & asked-task guards) | `config-store` JSON (`ceo_inbox`), one whole-rewritten key per store | + +Machine-state keys use whole-object last-write-wins (no per-doc version check); acceptable because each key has effectively a single writer per cron tick and the queue key is keyed by task id so a held-watermark retry re-adds idempotently. Prose evidence (`pending-diffs.md`), snapshots, and shadow docs remain OKF. Rationale for rejecting new stores (options 1 and 3): the destinations already exist and are versioned/auditable; a parallel schema would duplicate versioning, digest surfacing, and autonomy gating. The KG is the wrong home for raw draft prose and edit diffs (spec 21: "the workspace is the scratchpad, the KG is the record"). diff --git a/docs/specs/04-channels.md b/docs/specs/04-channels.md index 4cd9470f9..51a2ea367 100644 --- a/docs/specs/04-channels.md +++ b/docs/specs/04-channels.md @@ -97,7 +97,7 @@ Interactive terminal for local dev and testing. Reads from stdin, writes to stdo - **CEO inbox drafts (v0.35):** the CEO inbox can now find, read, and edit unsent drafts. `ceo-inbox-list` / `ceo-inbox-search` query Nylas's `/drafts` resource (the DRAFTS folder) when scoped with `folder: 'DRAFTS'`; `ceo-inbox-read` accepts a `draft_id` to return a draft's full body; and the new `ceo-inbox-draft-edit` skill updates a draft's recipients/subject/body. Draft results key on `drafts` (not `messages`) so a genuine "none" is distinguishable from a silent zero, and the inbox poll watermark is not applied to drafts. - **Email accounts (v0.37):** multiple agent-owned mailboxes are managed from the console under **Settings → Channels → Email → Email accounts**. Each account is a row in the `email_accounts` table (migration `064_create_email_accounts.sql`) with its Nylas grant stored in the vault at a per-account key (`channel.email..nylas_grant_id`). The shared app-level `NYLAS_API_KEY` remains an env-bootstrapped vault secret. The legacy `channel_accounts.email` YAML path and `CEO_PRIMARY_EMAIL` env var are retired. - **Vault credential wiring (v0.37):** at startup `applyChannelVaultSecrets()` overlays channel vault credentials (`channel..`) onto the runtime config before adapters are constructed, so credentials saved via the Channels console take effect on the next restart without any env-var or YAML change. -- **Sent-mail observation (v0.42, ADR-029):** a scheduled `ceo-inbox-sent-observe` skill polls the CEO's **Sent** folder through the existing `ceo-inbox` Nylas grant (no new credential). Scope is **all sent mail**. Each run is watermarked (`config-store` under the `ceo_inbox` namespace, same high-water pattern as the inbound email poll) and is idempotent on re-run. For each new sent message it (1) correlates to Curia-authored draft snapshots (below) by `thread_id` + recipients + send time and appends `(draft, sent)` pairs to a rolling OKF evidence doc, and (2) correlates to open `owner='ceo'` tasks for completion candidates ([spec 19](19-tasks-and-backlog.md)). It emits a `ceo.sent_observed` audit event and advances the watermark. Observe runs as a **separate daily cron** on `agents/ceo-inbox.yaml` with its own Operating Modes task text — never folded into the 15-min triage loop. +- **Sent-mail observation (v0.42, ADR-029):** a scheduled `ceo-inbox-sent-observe` skill polls the CEO's **Sent** folder through the existing `ceo-inbox` Nylas grant (no new credential). Scope is **all sent mail**. Each run is watermarked (`config-store` under the `ceo_inbox` namespace, same high-water pattern as the inbound email poll) and is idempotent on re-run. For each new sent message it (1) correlates to Curia-authored draft snapshots (below) by `thread_id` + recipients + send time and appends `(draft, sent)` pairs to a rolling OKF evidence doc, and (2) correlates to open `owner='ceo'` tasks for completion candidates, queued in `config-store` (`ceo_inbox`, `sent_observe.completion_candidates`) rather than an OKF doc ([spec 19](19-tasks-and-backlog.md)). It emits a `ceo.sent_observed` audit event and advances the watermark. Observe runs as a **separate daily cron** on `agents/ceo-inbox.yaml` with its own Operating Modes task text — never folded into the 15-min triage loop. - **Draft capture for voice learning (v0.42):** the three CEO-inbox draft skills (`ceo-inbox-draft-reply`, `ceo-inbox-draft-compose`, `ceo-inbox-draft-edit`) snapshot what Curia proposed into an OKF scratch doc at `/scratch/voice-learning/.md` (frontmatter: `draft_id`, `thread_id`, recipients, subject, `created_at`, `linked_task_ids[]`, `agent_version`; body = drafted markdown). Capture failure logs and **must not** block draft creation. Matched `(draft, sent)` diffs accumulate in `/scratch/voice-learning/pending-diffs.md` for the weekly `voice-learn` job ([spec 13](13-office-identity.md)). That job makes a single batched LLM extraction pass over the accumulated diffs to produce an updated `WritingVoice.guide`, proposed in the digest for CEO approval. - **OKF evidence retention:** raw draft snapshots and pending-diff evidence are scratch-path docs. Once a pair has been folded into the writing-voice profile (or dismissed), the learning job deletes or TTL-expires the consumed evidence — default scratch TTL applies to unmatched snapshots; consumed evidence must not be retained indefinitely. Shadow drafts for punted mail follow the same scratch path conventions and never surface or send ([spec 14](14-autonomy-engine.md)). - Nylas abstracts away provider differences (Gmail, Outlook, IMAP) and handles OAuth diff --git a/docs/specs/13-office-identity.md b/docs/specs/13-office-identity.md index dfdba24a1..1f7df78d2 100644 --- a/docs/specs/13-office-identity.md +++ b/docs/specs/13-office-identity.md @@ -373,11 +373,13 @@ A weekly `voice-learn` job on `ceo-inbox` (cron `0 8 * * 1`) reads accumulated (`ctx.infraLlm.extract()`) over that accumulated evidence to produce an updated free-form `WritingVoice.guide`. -The job never writes the profile directly. It queues a "## Guide Proposal" -block (`status: pending`) in the digest for CEO review. The CEO approves via -the `resolve-learning-digest` skill's `approve_voice` action, which calls +The job never writes the profile directly. It records a single pending proposal +object in `config-store` (`voice_learn.proposal`), surfaced in the digest for +CEO review. The CEO approves via the `resolve-learning-digest` skill's +`approve_voice` action, which calls `executiveProfileService.update({ writingVoice: { ...current, guide } })`. -Dismissed proposals get a guard marker and cooldown before re-propose. +Dismissed proposals set the `voice_learn.dismissed` config cooldown key before +re-propose. The learned guide renders into agent prompts through `compileWritingVoiceBlock`, under a "How the executive actually writes (learned from their edits)" heading. diff --git a/docs/specs/19-tasks-and-backlog.md b/docs/specs/19-tasks-and-backlog.md index 0d32c5675..42f591def 100644 --- a/docs/specs/19-tasks-and-backlog.md +++ b/docs/specs/19-tasks-and-backlog.md @@ -343,9 +343,9 @@ for these learning surfaces are explicitly deferred. | High-confidence match | Auto-`task-complete` with reversible digest undo note | Confirm in digest | | Low-confidence match | Confirm in digest | Confirm in digest | - High-risk tasks (e.g. "Plan AGM") never auto-complete. Fuzzy candidates get a guard marker - (`completion_asked: {date}`) so they are not re-surfaced every run. Auto-completes remain - reversible via reopen from the digest undo note. + High-risk tasks (e.g. "Plan AGM") never auto-complete. Fuzzy candidates are recorded in the + `sent_observe.asked_task_ids` guard set (config-store, `ceo_inbox`) so they are not + re-surfaced every run. Auto-completes remain reversible via reopen from the digest undo note. - **Autonomy** ([spec 14](14-autonomy-engine.md)). The "execute the safe stuff, escalate the rest" behavior is the existing autonomy engine — `action_risk` per skill against the live score. Drafting (low) just happens; outbound (medium) is gated; spending money diff --git a/docs/wip/2026-07-17-learning-state-config-store-design.md b/docs/wip/2026-07-17-learning-state-config-store-design.md new file mode 100644 index 000000000..6c52cd5b6 --- /dev/null +++ b/docs/wip/2026-07-17-learning-state-config-store-design.md @@ -0,0 +1,175 @@ +# Learning subsystem: machine state → config-store JSON (design) + +**Issue:** [#1438](https://github.com/josephfung/curia/issues/1438) — part of #1419. Follow-up +from the #1429 simplification review (Tier 4). + +## Problem + +The email-observation learning subsystem stores two kinds of content in the same OKF markdown +docs: + +1. **Prose evidence** — `(draft, sent)` diff bodies, shadow draft text. Document-shaped, + human-inspectable; OKF is the right home (ADR-029). **Stays.** +2. **Machine state** — queues, statuses, and idempotency guards riding *inside* doc bodies: + completion candidates + their `status:` lines (`pending-completions.md`), undo/confirm items + (`completion-digest.md`), the voice proposal's pending/resolved status (`pending-proposals.md`), + and the already-matched / already-asked guard sets **reconstructed by regexing doc bodies every + run**. + +Every machine-state read is a regex parse; every mutation is a split-blocks-and-rewrite with +optimistic-concurrency retries. The failure mode is silent: a format drift makes a regex stop +matching and an item never surfaces. This class bit three times during #1429 review (the `t1`/`t10` +block-boundary bleed, trimming on any `## ` header, the first-block-only proposal scan). + +## Approach + +Move the machine state to config-store JSON under the existing `ceo_inbox` namespace (already home +to the watermark, idle-backoff, checkpoint, and dismiss-cooldown keys). **One key per store, +holding a single JSON object, rewritten whole** — removal = writing the map without the entry, so +no per-item tombstones and no per-item status parsing. + +`ConfigStore` (`src/memory/config-store.ts`) stores `string` values, so each store is +`JSON.stringify`'d on write and `JSON.parse`'d on read (with a `try/catch` that treats a +missing/garbage record as empty — same posture as the existing `voice_learn.dismissed` JSON key). + +### Store mapping + +| Config key (`ceo_inbox` ns) | JSON shape | Writer(s) | Reader(s) | +|---|---|---|---| +| `sent_observe.completion_candidates` | `{ [taskId]: { messageId, confidence, reason, sentAt, subject, recipients[], taskTitle } }` | `ceo-inbox-sent-observe` **adds**; `task-completion-from-sent` **removes on consume** | `task-completion-from-sent` | +| `sent_observe.completion_digest` | `{ [taskId]: { kind, taskTitle, note, status } }` | `task-completion-from-sent` **adds**; `resolve-learning-digest` **removes** | `list-learning-digest`, `resolve-learning-digest` | +| `voice_learn.proposal` | `{ status, generated_at, guide } \| null` | `voice-learn` (**supersede**: write replaces prior) | `list-learning-digest`, `resolve-learning-digest` | +| `sent_observe.matched_draft_ids` | `string[]` | `ceo-inbox-sent-observe` | `ceo-inbox-sent-observe` | +| `sent_observe.asked_task_ids` | `string[]` | `ceo-inbox-sent-observe` | `ceo-inbox-sent-observe` | + +Digest keys are keyed by `taskId`. The completion queue is keyed by `taskId` too — one open task +has at most one live completion candidate at a time, so keying by task id makes re-adds idempotent +(see atomicity below) and makes "remove on consume" a single-key delete. + +### Why `asked_task_ids` is a separate guard (not derived from the queue) + +Today the guard is re-derived by regexing candidate headers that **stay** in +`pending-completions.md` after processing (`markCandidateProcessed` only flips the `status:` line; +the block persists until it ages out at 90 days). Once `task-completion-from-sent` **removes** +consumed candidates from the queue map, the map no longer remembers them — so `asked_task_ids` +becomes the standalone persistent guard that stops a consumed task being re-surfaced as a fresh +candidate. Same reasoning for `matched_draft_ids` vs the (staying-OKF) `pending-diffs.md`: it was +regexed out of the diff prose; now stored directly. + +### Guard-set retention: bound to live entities + +The old guard sets were implicitly bounded — re-derived each run from docs that trim at 90 days. +Standalone JSON arrays would grow forever. Instead, **prune each guard set on write to the entities +that still exist**, both of which `ceo-inbox-sent-observe` already loads every run: + +- `asked_task_ids` → intersect with the ids of the currently-open CEO tasks (`openTasks`). A task + that completes/cancels drops out of the open set and out of the guard. (Re-surfacing a since- + reopened task is harmless: the original send is already below the watermark; a genuinely new send + for it is a legitimate new candidate, and `task-completion-from-sent` re-validates eligibility + before completing.) +- `matched_draft_ids` → intersect with the `draftId`s of the draft snapshots still present + (`snapshots`). A draft snapshot TTL-sweeps after 7 idle days; once gone it can't be re-matched + anyway (`matchDraftToSent` only iterates present snapshots), so retaining its id past that is + pointless. Bound follows the snapshot TTL. + +This keeps the "regex-free, stored-directly" win while preserving bounded growth without a new +query — both live sets are already in hand. + +### Watermark-hold atomicity (the load-bearing acceptance criterion) + +> Guard sets and candidate queues persist only when evidence persistence succeeds (no candidate +> lost, none double-surfaced, across a held-watermark retry). + +`ceo-inbox-sent-observe` seeds the guard sets at the **start** of the run and persists new state at +the **end**; the watermark only advances when `advanceOk = evidencePersisted && shadowReconcileOk +&& draftEvidenceComplete`. On a held watermark the run re-observes the same sends next time, so the +new state must be written such that a re-run neither loses nor duplicates a candidate. Ordering: + +1. **Prose evidence first** — append `(draft, sent)` diffs to `pending-diffs.md` (stays OKF, + keeps its append+trim + version-retry). Result feeds `diffsPersisted`. +2. **`completion_candidates`** — merge newly-matched candidates into the map (keyed by taskId) and + write. This *replaces* the old `pending-completions.md` write and feeds `completionsPersisted`. +3. **`matched_draft_ids`** — write only the drafts whose diff **actually persisted** in step 1 + (coupled to `diffsPersisted`), pruned to present snapshots. +4. **`asked_task_ids`** — write **after** `completion_candidates`, pruned to open tasks. + +Failure analysis (config writes are last-write-wins; a hard infra failure throws and is caught): + +- Step 2 fails → watermark held (`completionsPersisted` false) → re-run re-matches the task and + re-adds it to the queue map. Because the map is **keyed by taskId**, the re-add is idempotent: + no duplicate candidate. +- Step 2 succeeds, step 4 fails → re-run re-matches the task, re-adds to the queue (idempotent) and + re-adds to `asked_task_ids`. No loss, no double-surface. (Writing candidates *before* the guard + is what makes the unsafe interleaving — guard says "asked" but queue is empty → candidate lost — + impossible.) +- Step 1 succeeds, step 3 fails → re-run re-matches the draft (guard didn't stick) and would append + a **duplicate** diff block. This is cosmetic (one extra diff block feeding the voice LLM), self- + limiting, and only reachable if the KG dies in the window between two writes to the same store — + the same residual the OKF version-retry never fully closed either. Acceptable; documented. + +### Concurrency trade-off (documented in ADR-029) + +Whole-object last-write-wins replaces per-doc optimistic-version checks. Acceptable because each key +has effectively a single writer per cron tick (`ceo-inbox-sent-observe` daily, `task-completion- +from-sent` daily, `voice-learn` weekly, `resolve-learning-digest` interactively on CEO reply) and +they do not run concurrently in prod. The keyed-map + write-ordering above covers the one two-writer +key (`completion_candidates`: add by observe, delete by completion) against interleaving. + +## What is deleted vs kept + +**Deleted** (the parse/prune/remove machinery, ~300–500 lines incl. tests): + +- `skills/_shared/learning-digest.ts`: `parseCompletionDigest`, `parseVoiceGuideProposal`, + `pruneGuideProposals`, `removeCompletionBlock`, private `guideProposalBlocks` / `guideFromBlock`. +- `skills/_shared/task-completion-risk.ts`: `parseCompletionCandidates` and the + `formatUndoNote` / `formatConfirmNote` markdown-block formatters (replaced by plain note-string + composition stored in the digest JSON). Risk logic (`classifyTaskRisk`, `decideCompletionAction`) + **stays**. +- `skills/_shared/sent-observe-match.ts`: `formatCompletionCandidateBlock` (queue is JSON now). + `formatDiffBlock`, `trimEvidenceDoc`, matching helpers **stay** (pending-diffs.md is still OKF). +- `skills/ceo-inbox-sent-observe/handler.ts`: `extractMatchedDraftIds`, `extractAskedTaskIds`, and + the `pending-completions.md` `ensureDoc`/`appendAndTrimDoc` path for the completion queue (the + diffs path stays). +- `skills/task-completion-from-sent/handler.ts`: `markCandidateProcessed`, `candidateBlock`, + `appendDigest`'s markdown-append path. +- `skills/voice-learn/handler.ts`: the `## Guide Proposal` block build + `pruneGuideProposals` + supersede. +- The version-conflict retry loops protecting those specific doc rewrites. + +**Kept:** + +- `renderVoiceGuideSection` / `renderCompletionSection` in `learning-digest.ts` — now fed from JSON. +- `pending-diffs.md` + `parsePendingDiffs`, per-draft snapshots, shadow docs — unchanged OKF format. +- All config-store watermark / idle-backoff / checkpoint / dismiss-cooldown keys. + +## Docs to update (same PR) + +- **ADR-029** store-mapping table (L42–48): split the "Cadence / watermarks / guard markers" row so + queue/status/guard state is explicitly `config-store` JSON; note the last-write-wins trade-off. +- **Spec 19** (L346–347): the fuzzy-candidate guard is config-store state, not a + `completion_asked:` doc marker. +- **Spec 13** (L376–380): the voice proposal is a config-store object, not a `## Guide Proposal` + doc block. +- **Spec 04** (L100–101): `pending-diffs.md` stays OKF (no change needed beyond confirming the + queue moved). + +## Sequencing / migration + +Land after #1429 (merged 2026-07-17) and deploy **before** enabling the learning subsystem in prod +(it ships disabled in `registry-defaults.yaml`), so there is no live state to migrate; any dev- +instance scratch docs simply TTL out. No migration code. + +## Testing (TDD) + +New/updated unit tests must cover: + +- **Map-rewrite removal** — resolving a digest item writes the map without that entry; other entries + survive; `t1` vs `t10` can't collide (keys are exact ids, so the old boundary-bleed class is gone). +- **Supersede** — a new voice proposal replaces a still-pending one (single object, not accumulation). +- **Guard-set persistence across a held watermark** — evidence-persist failure holds the watermark + AND does not strand the guard/queue: a re-run re-matches and re-adds idempotently (no loss, no + double-surface). +- **Guard-set pruning** — `asked_task_ids` drops ids for now-closed tasks; `matched_draft_ids` drops + ids for absent snapshots. +- Digest UX unchanged — `list-learning-digest` sections render identically; `approve/dismiss voice` + and `undo/confirm/dismiss completion` round-trip against the JSON stores. diff --git a/docs/wip/2026-07-17-learning-state-config-store.md b/docs/wip/2026-07-17-learning-state-config-store.md new file mode 100644 index 000000000..ff076ba29 --- /dev/null +++ b/docs/wip/2026-07-17-learning-state-config-store.md @@ -0,0 +1,887 @@ +# Learning subsystem: machine state → config-store JSON — Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Move the learning subsystem's queue/status/guard machine-state out of OKF markdown doc bodies into config-store JSON under the `ceo_inbox` namespace, deleting the regex-parse / block-rewrite machinery while keeping prose evidence (`pending-diffs.md`), snapshots, and shadow docs in OKF. + +**Architecture:** Five new `ceo_inbox` config keys, each a single whole-rewritten JSON value accessed through a new `skills/_shared/learning-state.ts` module. Maps are keyed by `taskId`; removal = writing the map without the entry (no tombstones). Guard sets are pruned on write to live entities (open tasks / present snapshots) that `ceo-inbox-sent-observe` already loads. The digest *render* helpers stay and read JSON. + +**Tech Stack:** TypeScript (ESM, Node 24+), Vitest, `ConfigStore` (`src/memory/config-store.ts`, KG-backed KV storing string values). + +## Global Constraints + +- ESM only — `.js` extensions on all relative imports; `import.meta.dirname` not `__dirname`. +- No `any` — proper types / discriminated unions. Array element access under strict null checks needs `!` (e.g. `arr[0]!`). +- Skills return `{ success: true, data }` / `{ success: false, error }` — never throw out of `execute()`. +- No `console.log` — use `ctx.log` (pino). No empty `catch {}` — log + continue or propagate. +- Run `pnpm -C /Users/josephfung/Projects/office-of-the-ceo/worktrees/curia-learning-state-json run typecheck` before each commit touching `.ts`. +- Run tests with `pnpm -C /Users/josephfung/Projects/office-of-the-ceo/worktrees/curia-learning-state-json test `. +- Commit messages: conventional (`refactor:`/`test:`/`docs:`/`chore:`), signed off (`-s`), NO `Co-Authored-By`, NO Claude attribution. +- Bump `version` in each touched `skill.json` (patch — behavior-preserving refactor). +- ConfigStore stores **strings** — every store is `JSON.stringify`'d on write, `JSON.parse`'d on read; a missing/garbage record parses to empty (never throw on parse). +- Digest UX must be **byte-identical** to today's rendering (acceptance criterion). + +**Worktree:** `/Users/josephfung/Projects/office-of-the-ceo/worktrees/curia-learning-state-json` (branch `chore/1438-learning-state-json`). All paths below are repo-relative to it. + +--- + +## File structure + +- **Create** `skills/_shared/learning-state.ts` — types, config-key constants, JSON accessors, note composers. +- **Create** `skills/_shared/learning-state.test.ts` — unit tests for accessors + composers. +- **Modify** `skills/_shared/learning-digest.ts` — keep only `renderVoiceGuideSection` / `renderCompletionSection`; import `CompletionDigestItem` from `learning-state.ts`; delete the parsers/prune/remove. +- **Modify** `skills/_shared/task-completion-risk.ts` — keep risk logic; delete `parseCompletionCandidates`, `formatUndoNote`, `formatConfirmNote`, `ParsedCompletionCandidate`. +- **Modify** `skills/_shared/sent-observe-match.ts` — delete `formatCompletionCandidateBlock`; keep matching + `formatDiffBlock` + `trimEvidenceDoc`. +- **Modify** `skills/voice-learn/handler.ts` (+ `handler.test.ts`, `skill.json`) — proposal → config. +- **Modify** `skills/ceo-inbox-sent-observe/handler.ts` (+ `handler.test.ts`, `skill.json`) — candidates + asked + matched guards → config. +- **Modify** `skills/task-completion-from-sent/handler.ts` (+ `handler.test.ts`, `skill.json`) — read candidates from config, consume-by-delete; digest → config. +- **Modify** `skills/list-learning-digest/handler.ts` (+ `handler.test.ts`, `skill.json`) — read proposal + digest from config. +- **Modify** `skills/resolve-learning-digest/handler.ts` (+ `handler.test.ts`, `skill.json`) — resolve proposal + digest in config. +- **Delete** `skills/_shared/learning-digest.test.ts` cases for the removed parsers; keep render tests. +- **Modify** `skills/_shared/task-completion-risk.test.ts` — drop candidate parse/format cases. +- **Modify** `skills/_shared/sent-observe-match.test.ts` — drop `formatCompletionCandidateBlock` case. +- **Modify** `docs/adr/029-passive-email-observation-and-counterfactual-competence.md`, `docs/specs/04-channels.md`, `docs/specs/13-office-identity.md`, `docs/specs/19-tasks-and-backlog.md`. +- **Modify** `CHANGELOG.md`. + +--- + +## Task 1: `learning-state.ts` shared module + +**Files:** +- Create: `skills/_shared/learning-state.ts` +- Test: `skills/_shared/learning-state.test.ts` + +**Interfaces:** +- Consumes: `ConfigStore` from `../../src/memory/config-store.js` (`get(ns,key): Promise`, `set(ns,key,value): Promise`); `MatchConfidence` from `./sent-observe-match.js`. +- Produces (all imported by later tasks): + - Namespace/keys: `LEARNING_STATE_NAMESPACE = 'ceo_inbox'`, `COMPLETION_CANDIDATES_KEY`, `COMPLETION_DIGEST_KEY`, `VOICE_PROPOSAL_KEY`, `MATCHED_DRAFT_IDS_KEY`, `ASKED_TASK_IDS_KEY`. + - Types: `CompletionCandidate`, `CompletionCandidateMap`, `CompletionDigestItem` (`{ kind: 'undo'|'confirm'; taskId: string; taskTitle: string; note: string }`), `CompletionDigestMap`, `VoiceGuideProposal` (`{ status: string; generatedAt: string; guide: string }`). + - Accessors: `readCompletionCandidates(store): Promise`, `writeCompletionCandidates(store, map): Promise`, `readCompletionDigest(store): Promise`, `writeCompletionDigest(store, map): Promise`, `readVoiceProposal(store): Promise`, `writeVoiceProposal(store, proposal | null): Promise`, `readIdSet(store, key): Promise>`, `writeIdSet(store, key, ids): Promise`. + - Composers: `composeUndoNote({ taskTitle, recipient, sentAt }): string`, `composeConfirmNote({ taskTitle, recipient }): string`. + - Helper for later tasks: `digestMapToItems(map): CompletionDigestItem[]`. + +- [ ] **Step 1: Write the failing test** + +Create `skills/_shared/learning-state.test.ts`: + +```ts +import { describe, it, expect } from 'vitest'; +import type { ConfigStore } from '../../src/memory/config-store.js'; +import { + LEARNING_STATE_NAMESPACE, + COMPLETION_CANDIDATES_KEY, + COMPLETION_DIGEST_KEY, + VOICE_PROPOSAL_KEY, + ASKED_TASK_IDS_KEY, + readCompletionCandidates, + writeCompletionCandidates, + readCompletionDigest, + writeCompletionDigest, + readVoiceProposal, + writeVoiceProposal, + readIdSet, + writeIdSet, + composeUndoNote, + composeConfirmNote, + digestMapToItems, + type CompletionCandidateMap, + type CompletionDigestMap, +} from './learning-state.js'; + +// Minimal in-memory ConfigStore double: only get/set, keyed by config key (namespace fixed). +function fakeStore(seed: Record = {}) { + const values = new Map(Object.entries(seed)); + const store = { + get: async (_ns: string, key: string) => values.get(key) ?? null, + set: async (_ns: string, key: string, value: string) => { values.set(key, value); }, + } as unknown as ConfigStore; + return { store, values }; +} + +describe('learning-state config accessors', () => { + it('uses the ceo_inbox namespace', () => { + expect(LEARNING_STATE_NAMESPACE).toBe('ceo_inbox'); + }); + + it('reads an empty map when the key is unset or garbage', async () => { + const { store } = fakeStore({ [COMPLETION_CANDIDATES_KEY]: 'not json' }); + expect(await readCompletionCandidates(store)).toEqual({}); + expect(await readCompletionDigest(fakeStore().store)).toEqual({}); + expect(await readVoiceProposal(fakeStore().store)).toBeNull(); + expect([...(await readIdSet(fakeStore().store, ASKED_TASK_IDS_KEY))]).toEqual([]); + }); + + it('round-trips a completion-candidate map and removes by writing the map without the entry', async () => { + const { store, values } = fakeStore(); + const map: CompletionCandidateMap = { + t1: { messageId: 'm1', confidence: 'high', reason: 'r', sentAt: 's', subject: 'sub', recipients: ['a@x'], taskTitle: 'T1' }, + t10: { messageId: 'm10', confidence: 'low', reason: 'r', sentAt: 's', subject: 'sub', recipients: ['b@x'], taskTitle: 'T10' }, + }; + await writeCompletionCandidates(store, map); + expect(JSON.parse(values.get(COMPLETION_CANDIDATES_KEY)!)).toEqual(map); + + // Remove t1 by writing the map without it — t10 is untouched (no boundary bleed). + const { t1: _drop, ...rest } = await readCompletionCandidates(store); + void _drop; + await writeCompletionCandidates(store, rest); + const after = await readCompletionCandidates(store); + expect(after.t1).toBeUndefined(); + expect(after.t10).toBeDefined(); + }); + + it('supersedes a pending voice proposal by whole-object write', async () => { + const { store } = fakeStore(); + await writeVoiceProposal(store, { status: 'pending', generatedAt: 'g1', guide: 'first' }); + await writeVoiceProposal(store, { status: 'pending', generatedAt: 'g2', guide: 'second' }); + expect(await readVoiceProposal(store)).toEqual({ status: 'pending', generatedAt: 'g2', guide: 'second' }); + await writeVoiceProposal(store, null); + expect(await readVoiceProposal(store)).toBeNull(); + }); + + it('round-trips an id set', async () => { + const { store } = fakeStore(); + await writeIdSet(store, ASKED_TASK_IDS_KEY, new Set(['a', 'b'])); + expect([...(await readIdSet(store, ASKED_TASK_IDS_KEY))].sort()).toEqual(['a', 'b']); + }); + + it('round-trips a digest map and flattens to items carrying their taskId', async () => { + const { store } = fakeStore(); + const map: CompletionDigestMap = { + t1: { kind: 'undo', taskId: 't1', taskTitle: 'Follow up', note: 'n1' }, + t2: { kind: 'confirm', taskId: 't2', taskTitle: 'Plan AGM', note: 'n2' }, + }; + await writeCompletionDigest(store, map); + const items = digestMapToItems(await readCompletionDigest(store)); + expect(items).toHaveLength(2); + expect(items[0]!.taskId).toBe('t1'); + expect(items.find((i) => i.taskId === 't2')!.kind).toBe('confirm'); + }); + + it('composes undo/confirm note text verbatim to the pre-migration copy', () => { + expect(composeUndoNote({ taskTitle: 'Ship', recipient: 'a@x', sentAt: '2026-07-01T12:00:00.000Z' })) + .toBe('Marked *Ship* done — you emailed a@x (2026-07-01). Undo?'); + expect(composeConfirmNote({ taskTitle: 'Ship', recipient: 'a@x' })) + .toBe('Did emailing a@x complete *Ship*?'); + }); +}); +``` + +- [ ] **Step 2: Run test to verify it fails** + +Run: `pnpm -C /Users/josephfung/Projects/office-of-the-ceo/worktrees/curia-learning-state-json test skills/_shared/learning-state.test.ts` +Expected: FAIL — cannot resolve `./learning-state.js`. + +- [ ] **Step 3: Write the module** + +Create `skills/_shared/learning-state.ts`: + +```ts +// Config-store JSON accessors for the email-observation learning subsystem (#1438). +// +// The queue/status/guard machine-state that used to ride inside OKF markdown doc bodies +// (completion candidates, digest items, the voice proposal, the matched/asked guard sets) now +// lives here as whole-object JSON values under the `ceo_inbox` namespace. Removal = writing the +// map without the entry (no per-item tombstones, no regex parse). Prose evidence +// (pending-diffs.md), draft snapshots, and shadow docs stay in OKF (see ADR-029). + +import type { ConfigStore } from '../../src/memory/config-store.js'; +import type { MatchConfidence } from './sent-observe-match.js'; + +/** Same namespace as the watermark/idle-backoff/checkpoint/dismiss-cooldown keys. */ +export const LEARNING_STATE_NAMESPACE = 'ceo_inbox'; + +export const COMPLETION_CANDIDATES_KEY = 'sent_observe.completion_candidates'; +export const COMPLETION_DIGEST_KEY = 'sent_observe.completion_digest'; +export const VOICE_PROPOSAL_KEY = 'voice_learn.proposal'; +export const MATCHED_DRAFT_IDS_KEY = 'sent_observe.matched_draft_ids'; +export const ASKED_TASK_IDS_KEY = 'sent_observe.asked_task_ids'; + +/** A task-completion candidate queued by sent-observe, consumed by task-completion-from-sent. */ +export interface CompletionCandidate { + messageId: string; + confidence: MatchConfidence; + reason: string; + sentAt: string; + subject: string; + recipients: string[]; + taskTitle: string; +} +/** Keyed by taskId — one open task has at most one live candidate, so re-adds are idempotent. */ +export type CompletionCandidateMap = Record; + +/** An undo/confirm item shown in the learning digest. `taskId` is carried in the value so the + * render helpers (which take a flat array) keep emitting the reply-command per item unchanged. */ +export interface CompletionDigestItem { + kind: 'undo' | 'confirm'; + taskId: string; + taskTitle: string; + note: string; +} +export type CompletionDigestMap = Record; + +export interface VoiceGuideProposal { + status: string; + generatedAt: string; + guide: string; +} + +/** Parse a stored JSON value, treating unset/garbage as absent — data loss (a dropped item) is + * worse than over-retention, and a parse throw must never escape into a skill failure. */ +function parseJson(raw: string | null): T | null { + if (!raw) return null; + try { + return JSON.parse(raw) as T; + } catch { + return null; + } +} + +export async function readCompletionCandidates(store: ConfigStore): Promise { + return parseJson(await store.get(LEARNING_STATE_NAMESPACE, COMPLETION_CANDIDATES_KEY)) ?? {}; +} +export async function writeCompletionCandidates(store: ConfigStore, map: CompletionCandidateMap): Promise { + await store.set(LEARNING_STATE_NAMESPACE, COMPLETION_CANDIDATES_KEY, JSON.stringify(map)); +} + +export async function readCompletionDigest(store: ConfigStore): Promise { + return parseJson(await store.get(LEARNING_STATE_NAMESPACE, COMPLETION_DIGEST_KEY)) ?? {}; +} +export async function writeCompletionDigest(store: ConfigStore, map: CompletionDigestMap): Promise { + await store.set(LEARNING_STATE_NAMESPACE, COMPLETION_DIGEST_KEY, JSON.stringify(map)); +} + +export async function readVoiceProposal(store: ConfigStore): Promise { + return parseJson(await store.get(LEARNING_STATE_NAMESPACE, VOICE_PROPOSAL_KEY)); +} +export async function writeVoiceProposal(store: ConfigStore, proposal: VoiceGuideProposal | null): Promise { + await store.set(LEARNING_STATE_NAMESPACE, VOICE_PROPOSAL_KEY, JSON.stringify(proposal)); +} + +export async function readIdSet(store: ConfigStore, key: string): Promise> { + const arr = parseJson(await store.get(LEARNING_STATE_NAMESPACE, key)); + return new Set(Array.isArray(arr) ? arr : []); +} +export async function writeIdSet(store: ConfigStore, key: string, ids: Set): Promise { + await store.set(LEARNING_STATE_NAMESPACE, key, JSON.stringify([...ids])); +} + +/** Flatten the digest map to the array the render helpers consume, preserving insertion order. */ +export function digestMapToItems(map: CompletionDigestMap): CompletionDigestItem[] { + return Object.values(map); +} + +/** Human note for an auto-completed task's undo affordance. Verbatim to the pre-migration + * formatUndoNote copy so the digest UX is byte-identical. */ +export function composeUndoNote(params: { taskTitle: string; recipient: string; sentAt: string }): string { + const when = params.sentAt ? ` (${params.sentAt.slice(0, 10)})` : ''; + return `Marked *${params.taskTitle}* done — you emailed ${params.recipient}${when}. Undo?`; +} + +/** Human note for a confirm-in-digest item. Verbatim to the pre-migration formatConfirmNote copy. */ +export function composeConfirmNote(params: { taskTitle: string; recipient: string }): string { + return `Did emailing ${params.recipient} complete *${params.taskTitle}*?`; +} +``` + +- [ ] **Step 4: Run tests to verify they pass** + +Run: `pnpm -C /Users/josephfung/Projects/office-of-the-ceo/worktrees/curia-learning-state-json test skills/_shared/learning-state.test.ts` +Expected: PASS (7 tests). + +- [ ] **Step 5: Typecheck + commit** + +Run: `pnpm -C /Users/josephfung/Projects/office-of-the-ceo/worktrees/curia-learning-state-json run typecheck` +Expected: no errors. + +```bash +git -C /Users/josephfung/Projects/office-of-the-ceo/worktrees/curia-learning-state-json add skills/_shared/learning-state.ts skills/_shared/learning-state.test.ts +git -C /Users/josephfung/Projects/office-of-the-ceo/worktrees/curia-learning-state-json commit -s -m "feat(learning-state): config-store JSON accessors for learning machine-state (#1438)" +``` + +--- + +## Task 2: Voice proposal → config (`voice-learn`, `list`, `resolve`) + +Migrate the voice guide proposal (Store A) from `pending-proposals.md` to `voice_learn.proposal`. `voice-learn` writes (supersede = whole write); `list-learning-digest` reads; `resolve-learning-digest` `approve_voice`/`dismiss_voice` read + clear. The dismiss-cooldown (`voice_learn.dismissed`) and diffs-checkpoint keys are untouched. + +**Files:** +- Modify: `skills/voice-learn/handler.ts`, `skills/voice-learn/handler.test.ts`, `skills/voice-learn/skill.json` +- Modify: `skills/list-learning-digest/handler.ts`, `skills/list-learning-digest/handler.test.ts` +- Modify: `skills/resolve-learning-digest/handler.ts`, `skills/resolve-learning-digest/handler.test.ts` + +**Interfaces:** +- Consumes from Task 1: `readVoiceProposal`, `writeVoiceProposal`, `VOICE_PROPOSAL_KEY`, `LEARNING_STATE_NAMESPACE`. +- Produces: `voice_learn.proposal` config key holding `VoiceGuideProposal | null`. `voice-learn` still exports `CONFIG_NAMESPACE`, `DISMISSED_KEY`, `DIFFS_CHECKPOINT_KEY`. `PENDING_PROPOSALS_PATH` / `PENDING_PROPOSALS_TYPE` are **removed**. + +- [ ] **Step 1: Update `voice-learn/handler.test.ts` (failing)** + +The existing test asserts a `## Guide Proposal` block lands in `pending-proposals.md`. Change it to assert the proposal object is written to `voice_learn.proposal`. Add the `makeMem` config double (copy the one from `resolve-learning-digest/handler.test.ts` lines 8–32, using `CONFIG_NAMESPACE`), wire `entityMemory: mem` into the ctx, and replace the doc assertion: + +```ts +import { VOICE_PROPOSAL_KEY, LEARNING_STATE_NAMESPACE } from '../_shared/learning-state.js'; +// ... +// after a successful proposal run: +const stored = JSON.parse(mem.__values.get(VOICE_PROPOSAL_KEY)!); +expect(stored.status).toBe('pending'); +expect(stored.guide).toContain(/* the expected guide text */); +// supersede: a second run with newer evidence overwrites the same key (no accumulation) +``` + +Keep the existing checkpoint / dismiss-cooldown assertions (those keys and their behavior are unchanged). Ensure the test no longer references `PENDING_PROPOSALS_PATH`. + +- [ ] **Step 2: Run test to verify it fails** + +Run: `pnpm -C /Users/josephfung/Projects/office-of-the-ceo/worktrees/curia-learning-state-json test skills/voice-learn/handler.test.ts` +Expected: FAIL — `VOICE_PROPOSAL_KEY` unused by handler / value undefined. + +- [ ] **Step 3: Update `voice-learn/handler.ts`** + +Replace the proposals-doc read/write with a config write. Concretely: + +- Remove imports of `pruneGuideProposals` and `PENDING_PROPOSALS_PATH`/`PENDING_PROPOSALS_TYPE`; remove the `export const PENDING_PROPOSALS_PATH/TYPE` lines. +- Add `import { writeVoiceProposal } from '../_shared/learning-state.js';`. +- Delete the `const existing = await ctx.workingDocs.read(PENDING_PROPOSALS_PATH);` read (superseding is now an unconditional whole-object write). +- Replace the block-build + create/update branch (the `const block = ...` through the `ctx.workingDocs.create/update` if/else) with: + +```ts +// Supersede any prior proposal by writing the single proposal object whole. The checkpoint +// (below) is what stops the SAME evidence being re-proposed; this write replaces a stale +// still-pending proposal with the fresher one. configStore is guaranteed here — a proposal +// requires entityMemory to persist state; if it's unavailable we cannot record the proposal. +if (!configStore) { + ctx.log.warn({}, 'voice-learn: config store unavailable — cannot record proposal this run'); + return { success: true, data: { pairs_considered: newPairs.length, proposed: false, reason: 'no-config-store' } }; +} +await writeVoiceProposal(configStore, { + status: 'pending', + generatedAt: new Date().toISOString(), + guide, +}); +``` + +Note: `configStore` is created earlier in the handler from `ctx.entityMemory` (the checkpoint block). It is `ConfigStore | null`. The checkpoint-advance block that follows already guards on `if (configStore)`, so it is unchanged. + +- [ ] **Step 4: Update `list-learning-digest/handler.ts`** + +- Remove `import { PENDING_PROPOSALS_PATH } from '../voice-learn/handler.js';` and the `parseVoiceGuideProposal` import. +- Add `import { ConfigStore } from '../../src/memory/config-store.js';` and `import { readVoiceProposal } from '../_shared/learning-state.js';`. +- Replace the proposal read: guard on `ctx.entityMemory`; when present, `const store = new ConfigStore(ctx.entityMemory, ctx.log); const proposal = await readVoiceProposal(store);` and use `proposal?.status === 'pending' ? proposal.guide : null` for the guide. (Digest read stays markdown in this task — migrated in Task 4.) + +- [ ] **Step 5: Update `resolve-learning-digest/handler.ts` voice branch** + +In the `approve_voice`/`dismiss_voice` branch (currently reads `PENDING_PROPOSALS_PATH` + `parseVoiceGuideProposal`): +- Remove the `PENDING_PROPOSALS_PATH` import and `parseVoiceGuideProposal`/`pruneGuideProposals` imports. +- Build the `ConfigStore` once at the top of the branch: `const store = new ConfigStore(ctx.entityMemory, ctx.log);` (already imported). +- Replace the doc read with `const proposal = await readVoiceProposal(store); if (!proposal || proposal.status !== 'pending') return { success: false, error: 'No pending voice guide proposal' };` +- `approve_voice`: after `executiveProfileService.update(...)`, replace the `pruneGuideProposals` + doc update with `await writeVoiceProposal(store, null);`. +- `dismiss_voice`: keep the existing `DISMISSED_KEY` cooldown write (it already uses `store`); replace the `pruneGuideProposals` + doc update with `await writeVoiceProposal(store, null);`. +- Add `readVoiceProposal, writeVoiceProposal` to the `learning-state.js` import. + +- [ ] **Step 6: Update `list` + `resolve` tests** + +`list-learning-digest/handler.test.ts`: seed the proposal via config (`entityMemory` double + `VOICE_PROPOSAL_KEY`) instead of a `pending-proposals.md` doc; assert `voice_guide` still renders. `resolve-learning-digest/handler.test.ts` (`approve`/`dismiss` cases): seed `VOICE_PROPOSAL_KEY` in `mem.__values` (JSON string), drop the `pending-proposals.md` doc; after resolve, assert `mem.__values.get(VOICE_PROPOSAL_KEY)` is `'null'` and (dismiss) `DISMISSED_KEY` contains `guide`. + +- [ ] **Step 7: Run the three test files** + +Run: `pnpm -C /Users/josephfung/Projects/office-of-the-ceo/worktrees/curia-learning-state-json test skills/voice-learn skills/list-learning-digest skills/resolve-learning-digest` +Expected: PASS. (`resolve` completion-digest cases still use markdown here — untouched until Task 4.) + +- [ ] **Step 8: Typecheck + commit** + +(Version bumps are deferred to Task 8 — one bump per skill per PR, not per task/commit.) + +Run typecheck, then: +```bash +git -C /Users/josephfung/Projects/office-of-the-ceo/worktrees/curia-learning-state-json add skills/voice-learn skills/list-learning-digest skills/resolve-learning-digest +git -C /Users/josephfung/Projects/office-of-the-ceo/worktrees/curia-learning-state-json commit -s -m "refactor(voice-learn): store voice proposal in config-store, not pending-proposals.md (#1438)" +``` + +--- + +## Task 3: Completion candidates + asked-task guard → config (`sent-observe` write, `task-completion` consume) + +Migrate the candidate queue (Store C) and the `asked_task_ids` guard. `ceo-inbox-sent-observe` writes candidates + the pruned asked-guard to config (ordered so a held watermark neither loses nor double-surfaces a candidate); `task-completion-from-sent` reads the map and **removes on consume**. The completion digest write stays markdown until Task 4. `matched_draft_ids` stays regex-derived until Task 5. + +**Files:** +- Modify: `skills/ceo-inbox-sent-observe/handler.ts`, `skills/ceo-inbox-sent-observe/handler.test.ts`, `skills/ceo-inbox-sent-observe/skill.json` +- Modify: `skills/task-completion-from-sent/handler.ts`, `skills/task-completion-from-sent/handler.test.ts`, `skills/task-completion-from-sent/skill.json` + +**Interfaces:** +- Consumes from Task 1: `readCompletionCandidates`, `writeCompletionCandidates`, `readIdSet`, `writeIdSet`, `ASKED_TASK_IDS_KEY`, `CompletionCandidateMap`, `CompletionCandidate`. +- Produces: `sent_observe.completion_candidates` (map) + `sent_observe.asked_task_ids` (array). `PENDING_COMPLETIONS_PATH` / `PENDING_COMPLETIONS_TYPE` are **removed** from `sent-observe/handler.ts`. + +- [ ] **Step 1: Update `sent-observe/handler.test.ts` (failing)** + +Change the `persists task-completion candidates for open CEO tasks` test: replace the `ctx.__docs.get(PENDING_COMPLETIONS_PATH)` assertion with a config-map assertion: + +```ts +import { COMPLETION_CANDIDATES_KEY, ASKED_TASK_IDS_KEY } from '../_shared/learning-state.js'; +// ... +const stored = JSON.parse(ctx.__mem.__values.get(COMPLETION_CANDIDATES_KEY)!); +expect(stored['task-ceo-1']).toBeDefined(); +expect(stored['task-ceo-1'].confidence).toBe('high'); +const asked = JSON.parse(ctx.__mem.__values.get(ASKED_TASK_IDS_KEY)!); +expect(asked).toContain('task-ceo-1'); +``` + +Add a new test for the held-watermark guard-atomicity criterion: + +```ts +it('does not persist the asked-guard when the candidate write is held (no candidate lost)', async () => { + // A matched task, but the completion_candidates config write throws → completionsPersisted=false + // → watermark held AND asked_task_ids NOT written, so next run re-matches and re-adds. + // Drive the failure by making storeFact reject writes to COMPLETION_CANDIDATES_KEY. + // (Implement by wrapping the mem double's storeFact to throw for that label.) + // Assert: watermark_advanced_to === null, asked_task_ids unset (or lacks the task). +}); +``` + +Also add a pruning test: + +```ts +it('prunes asked_task_ids to currently-open tasks on write', async () => { + // Seed ASKED_TASK_IDS_KEY with ['closed-task', 'task-ceo-1']; openTasks returns only task-ceo-1. + // After a run, stored asked set excludes 'closed-task'. +}); +``` + +- [ ] **Step 2: Run to verify failure** + +Run: `pnpm -C /Users/josephfung/Projects/office-of-the-ceo/worktrees/curia-learning-state-json test skills/ceo-inbox-sent-observe/handler.test.ts` +Expected: FAIL. + +- [ ] **Step 3: Update `sent-observe/handler.ts`** + +- Remove `formatCompletionCandidateBlock` from the `sent-observe-match.js` import; add `import { readCompletionCandidates, writeCompletionCandidates, readIdSet, writeIdSet, ASKED_TASK_IDS_KEY, type CompletionCandidateMap, type CompletionCandidate } from '../_shared/learning-state.js';`. +- Remove `export const PENDING_COMPLETIONS_PATH/TYPE`; remove the `pendingCompletions = await ensureDoc(...)` call; remove `extractAskedTaskIds` (function + its call). Keep `PENDING_DIFFS_PATH`, `extractMatchedDraftIds` (removed in Task 5). +- Seed the asked guard from config: `const store = new ConfigStore(...)` already exists; add `const alreadyAskedTaskIds = await readIdSet(store, ASKED_TASK_IDS_KEY);` (replacing the doc-derived seed). +- Replace `completionChunks: string[]` with `const newCandidates: CompletionCandidateMap = {};`. In the task-match loop, replace `completionChunks.push(formatCompletionCandidateBlock(tm))` with: + +```ts +newCandidates[tm.taskId] = { + messageId: tm.messageId, + confidence: tm.confidence, + reason: tm.reason, + sentAt: tm.sentAt, + subject: tm.sentSubject, + recipients: tm.sentRecipients, + taskTitle: tm.taskTitle, +} satisfies CompletionCandidate; +alreadyAskedTaskIds.add(tm.taskId); +taskCandidates += 1; +``` + +- Replace the `pending-completions.md` `appendAndTrimDoc` call with a config merge-write, ordered after the diffs write: + +```ts +// Persist the candidate queue (config JSON) — replaces the pending-completions.md append. +// Merge onto a fresh read so we don't clobber a concurrent removal by task-completion; keyed +// by taskId so a held-watermark retry re-adds idempotently. A hard write failure holds the +// watermark (completionsPersisted=false) — the queue must persist before we forget the sends. +let completionsPersisted = true; +if (Object.keys(newCandidates).length > 0) { + try { + const existing = await readCompletionCandidates(store); + await writeCompletionCandidates(store, { ...existing, ...newCandidates }); + } catch (err) { + completionsPersisted = false; + ctx.log.warn({ err }, 'ceo-inbox-sent-observe: completion-candidate write failed — holding watermark'); + } +} +``` + +- Keep `diffsPersisted` (the pending-diffs append). Update `evidencePersisted = diffsPersisted && completionsPersisted;` (same shape, `completionsPersisted` is now the config write, not a doc append). +- After the watermark-advance decision is computed but gated on the same `advanceOk`, write the asked guard ONLY when the queue persisted, pruned to open tasks: + +```ts +// Persist the asked-task guard AFTER the queue, and only when the queue persisted — writing the +// guard while the queue write failed would let next run skip re-matching and LOSE the candidate. +// Prune to currently-open tasks (Joseph's retention choice): a completed/cancelled task drops out +// of the guard; re-surfacing a since-reopened task is harmless (original send is below the +// watermark; task-completion re-validates eligibility). +if (completionsPersisted) { + const openIds = new Set(openTasks.map((t) => t.id)); + const prunedAsked = new Set([...alreadyAskedTaskIds].filter((id) => openIds.has(id))); + try { + await writeIdSet(store, ASKED_TASK_IDS_KEY, prunedAsked); + } catch (err) { + ctx.log.warn({ err }, 'ceo-inbox-sent-observe: asked-guard write failed (queue already persisted; guard re-derives next run)'); + } +} +``` + +Note the guard-write placement: it must run whenever `completionsPersisted` (independent of `advanceOk`), because even a held watermark that persisted the queue wants the guard updated. Put it right after the `completionsPersisted` block, before the watermark-advance block. + +- [ ] **Step 4: Update `task-completion-from-sent/handler.ts`** + +- Remove imports of `PENDING_COMPLETIONS_PATH`, `parseCompletionCandidates`, `candidateBlock`, `markCandidateProcessed` (delete the two local functions). Add `import { ConfigStore } from '../../src/memory/config-store.js';` and `import { readCompletionCandidates, writeCompletionCandidates, type CompletionCandidateMap } from '../_shared/learning-state.js';`. +- Require `ctx.entityMemory` in the capability guard (`if (!ctx.taskRepo || !ctx.workingDocs || !ctx.sensitivityClassifier || !ctx.entityMemory)`). +- Replace the `pendingDoc` read + `parseCompletionCandidates` with: + +```ts +const store = new ConfigStore(ctx.entityMemory, ctx.log); +const candidateMap = await readCompletionCandidates(store); +const candidates = Object.entries(candidateMap).map(([taskId, c]) => ({ taskId, ...c })); +if (candidates.length === 0) { + return { success: true, data: { auto_completed: 0, queued_confirm: 0, skipped: 0 } }; +} +const remaining: CompletionCandidateMap = { ...candidateMap }; +``` + +- In the loop, replace every `body = markCandidateProcessed(...)` / status-mark with `delete remaining[candidate.taskId];` (consume-by-remove — auto-completed, confirm-queued, and skipped-ineligible all drop from the queue; the `asked_task_ids` guard stops re-surfacing). The `candidate.confidence`, `candidate.messageId`, etc. field names are unchanged (the flattened object has the same shape as `ParsedCompletionCandidate` minus `status`). +- Replace the digest chunk collection (`digestChunks`) — for Task 3 keep the existing markdown `appendDigest` path unchanged (it still uses `formatUndoNote`/`formatConfirmNote`); those move to config in Task 4. So `digestChunks.push(formatUndoNote({...}))` / `formatConfirmNote({...})` stay as-is here. +- Replace the final `if (body !== pendingDoc.body) { ctx.workingDocs.update(PENDING_COMPLETIONS_PATH, ...) }` with: + +```ts +if (Object.keys(remaining).length !== Object.keys(candidateMap).length) { + await writeCompletionCandidates(store, remaining); +} +``` + +- [ ] **Step 5: Update `task-completion-from-sent/handler.test.ts`** + +Replace the `PENDING` markdown doc seed with a config candidate map seed. Add the `entityMemory` config double (copy `makeMem` from `resolve` test, using `CONFIG_NAMESPACE = 'ceo_inbox'`; seed `COMPLETION_CANDIDATES_KEY` with the three candidates as a JSON map keyed by the three task UUIDs). Assertions: +- `auto_completed`/`queued_confirm` counts unchanged (still 1/2). +- After the run, `JSON.parse(mem.__values.get(COMPLETION_CANDIDATES_KEY)!)` is `{}` (all three consumed). +- The digest doc (`COMPLETION_DIGEST_PATH`, still markdown in Task 3) still contains the three `Undo`/`Confirm` blocks (keep those assertions). +- Update the `skips a candidate whose task is no longer CEO-owned` case: assert the ineligible task is removed from the config map (not a `skipped_ineligible` marker) and is not completed. + +- [ ] **Step 6: Run both test files** + +Run: `pnpm -C /Users/josephfung/Projects/office-of-the-ceo/worktrees/curia-learning-state-json test skills/ceo-inbox-sent-observe skills/task-completion-from-sent` +Expected: PASS. + +- [ ] **Step 7: Typecheck + commit** + +(Version bumps deferred to Task 8.) + +```bash +git -C /Users/josephfung/Projects/office-of-the-ceo/worktrees/curia-learning-state-json add skills/ceo-inbox-sent-observe skills/task-completion-from-sent +git -C /Users/josephfung/Projects/office-of-the-ceo/worktrees/curia-learning-state-json commit -s -m "refactor(sent-observe): completion queue + asked-guard in config-store, consume-by-delete (#1438)" +``` + +--- + +## Task 4: Completion digest → config (`task-completion` write, `list`/`resolve` read/resolve) + +Migrate the digest (Store B) from `completion-digest.md` to `sent_observe.completion_digest`. `task-completion-from-sent` writes items (keyed by taskId); `list-learning-digest` renders from the map; `resolve-learning-digest` undo/confirm/dismiss remove by taskId. + +**Files:** +- Modify: `skills/task-completion-from-sent/handler.ts`, `skills/task-completion-from-sent/handler.test.ts` +- Modify: `skills/list-learning-digest/handler.ts`, `skills/list-learning-digest/handler.test.ts` +- Modify: `skills/resolve-learning-digest/handler.ts`, `skills/resolve-learning-digest/handler.test.ts` +- Modify: `skills/_shared/learning-digest.ts` (render helpers now import `CompletionDigestItem` from `learning-state.ts`) + +**Interfaces:** +- Consumes from Task 1: `readCompletionDigest`, `writeCompletionDigest`, `digestMapToItems`, `composeUndoNote`, `composeConfirmNote`, `CompletionDigestItem`, `CompletionDigestMap`. +- Produces: `sent_observe.completion_digest` (map). `COMPLETION_DIGEST_PATH` / `COMPLETION_DIGEST_TYPE` are **removed** from `task-completion-from-sent/handler.ts`. + +- [ ] **Step 1: Point `learning-digest.ts` render helpers at the new type** + +In `skills/_shared/learning-digest.ts` add `import type { CompletionDigestItem } from './learning-state.js';` and delete the local `CompletionDigestItem` interface. (Full dead-code removal is Task 6; here only the type source changes so the two render helpers compile against the shared type. `renderCompletionSection` already reads only `kind`/`taskId`/`note` — all present on the shared type.) + +- [ ] **Step 2: Update `task-completion-from-sent/handler.test.ts` (failing)** + +Replace the `COMPLETION_DIGEST_PATH` doc assertions with config-map assertions: + +```ts +import { COMPLETION_DIGEST_KEY } from '../_shared/learning-state.js'; +// ... +const digest = JSON.parse(mem.__values.get(COMPLETION_DIGEST_KEY)!); +expect(digest['11111111-1111-4111-8111-111111111111'].kind).toBe('undo'); +expect(digest['22222222-2222-4222-8222-222222222222'].kind).toBe('confirm'); +expect(digest['33333333-3333-4333-8333-333333333333'].kind).toBe('confirm'); +``` + +- [ ] **Step 3: Update `task-completion-from-sent/handler.ts`** + +- Remove `COMPLETION_DIGEST_PATH`/`COMPLETION_DIGEST_TYPE` exports and the `appendDigest` function; remove `formatUndoNote`/`formatConfirmNote` imports. Add `readCompletionDigest, writeCompletionDigest, composeUndoNote, composeConfirmNote, type CompletionDigestMap, type CompletionDigestItem` to the `learning-state.js` import. +- Replace `const digestChunks: string[] = [];` with `const digestAdds: CompletionDigestItem[] = [];`. +- In the auto-complete branch, replace the `formatUndoNote` push with: + +```ts +digestAdds.push({ + kind: 'undo', + taskId: task.id, + taskTitle: task.title || candidate.taskTitle, + note: composeUndoNote({ taskTitle: task.title || candidate.taskTitle, recipient, sentAt: candidate.sentAt }), +}); +``` + +- In the confirm branch, replace the `formatConfirmNote` push with: + +```ts +digestAdds.push({ + kind: 'confirm', + taskId: task.id, + taskTitle: task.title || candidate.taskTitle, + note: composeConfirmNote({ taskTitle: task.title || candidate.taskTitle, recipient }), +}); +``` + +- Replace the `if (digestChunks.length > 0) { await appendDigest(...) }` with a read-merge-write: + +```ts +if (digestAdds.length > 0) { + const digestMap = await readCompletionDigest(store); + for (const item of digestAdds) digestMap[item.taskId] = item; + await writeCompletionDigest(store, digestMap); +} +``` + +(`store` is the `ConfigStore` created in Task 3.) + +- [ ] **Step 4: Update `list-learning-digest/handler.ts`** + +- Remove `COMPLETION_DIGEST_PATH` import + `parseCompletionDigest` import. Add `readCompletionDigest, digestMapToItems` to the `learning-state.js` import. +- Replace `const completionsDoc = await ctx.workingDocs.read(COMPLETION_DIGEST_PATH); const completion_items = parseCompletionDigest(completionsDoc?.body ?? '');` with (reusing the `store` built for the proposal read in Task 2): `const completion_items = ctx.entityMemory ? digestMapToItems(await readCompletionDigest(store)) : [];`. +- `renderCompletionSection(completion_items)` is unchanged. + +- [ ] **Step 5: Update `resolve-learning-digest/handler.ts` completion branch** + +- Remove `COMPLETION_DIGEST_PATH` import + `parseCompletionDigest`/`removeCompletionBlock` imports. Add `readCompletionDigest, writeCompletionDigest` to the `learning-state.js` import. +- Replace `const digest = await ctx.workingDocs.read(COMPLETION_DIGEST_PATH); if (!digest) ...` with `const digestMap = await readCompletionDigest(store); const item = digestMap[taskId]; if (!item || item.kind !== expectedKind) return { success: false, error: \`No actionable ${expectedKind} item for task ${taskId}\` };`. +- In each of `undo_completion` / `confirm_completion` / `dismiss_completion`, after the task mutation succeeds (unchanged: `reopenTask` null-guard, `completeTask` not-found-guard), replace the `removeCompletionBlock` + `ctx.workingDocs.update(COMPLETION_DIGEST_PATH, ...)` with: + +```ts +const { [taskId]: _removed, ...rest } = digestMap; +void _removed; +await writeCompletionDigest(store, rest); +``` + +Keep the ordering: for `undo_completion`, still call `reopenTask` first and bail (without writing) if it returns null; for `confirm_completion`, still `getTask` + not-found guard before `completeTask`. + +- [ ] **Step 6: Update `list` + `resolve` tests** + +`list-learning-digest/handler.test.ts`: seed `COMPLETION_DIGEST_KEY` as a JSON map; assert `completion_items` + `sections_markdown` render identically (e.g. still contains `undo completion ` / `confirm completion ` reply commands). `resolve-learning-digest/handler.test.ts` undo cases: seed `COMPLETION_DIGEST_KEY` (JSON map with a `t1` undo item) instead of the `completion-digest.md` doc; after `undo_completion`, assert `JSON.parse(mem.__values.get(COMPLETION_DIGEST_KEY)!).t1` is undefined; for the reopenTask-returns-null case, assert the map still contains `t1` (item preserved, `writeCompletionDigest` not called). + +- [ ] **Step 7: Run the affected test files** + +Run: `pnpm -C /Users/josephfung/Projects/office-of-the-ceo/worktrees/curia-learning-state-json test skills/task-completion-from-sent skills/list-learning-digest skills/resolve-learning-digest` +Expected: PASS. + +- [ ] **Step 8: Typecheck + commit** + +(Version bumps deferred to Task 8.) + +```bash +git -C /Users/josephfung/Projects/office-of-the-ceo/worktrees/curia-learning-state-json add skills/task-completion-from-sent skills/list-learning-digest skills/resolve-learning-digest skills/_shared/learning-digest.ts +git -C /Users/josephfung/Projects/office-of-the-ceo/worktrees/curia-learning-state-json commit -s -m "refactor(learning-digest): completion digest in config-store, resolve by map delete (#1438)" +``` + +--- + +## Task 5: `matched_draft_ids` guard → config (`sent-observe`) + +Migrate the draft-match guard (Store D) from a regex over `pending-diffs.md` to `sent_observe.matched_draft_ids`. `pending-diffs.md` stays OKF prose. Crucially, a draft whose full-body fetch FAILED (no diff persisted) must be EXCLUDED from the stored set so it re-matches next run — mirroring today's re-derive-from-doc behavior. + +**Files:** +- Modify: `skills/ceo-inbox-sent-observe/handler.ts`, `skills/ceo-inbox-sent-observe/handler.test.ts` + +**Interfaces:** +- Consumes from Task 1: `readIdSet`, `writeIdSet`, `MATCHED_DRAFT_IDS_KEY`. +- Produces: `sent_observe.matched_draft_ids` (array). `extractMatchedDraftIds` is **deleted**. + +- [ ] **Step 1: Update `sent-observe/handler.test.ts` (failing)** + +The existing `matches a draft snapshot and appends a pending-diffs block` test already asserts the diff lands in `pending-diffs.md` (keep that). Add: + +```ts +import { MATCHED_DRAFT_IDS_KEY } from '../_shared/learning-state.js'; +// in the successful-match test: +expect(JSON.parse(ctx.__mem.__values.get(MATCHED_DRAFT_IDS_KEY)!)).toContain('draft-1'); +``` + +Extend the existing `holds the watermark ... matched draft body cannot be fetched (F8)` test: assert the failed draft is NOT in the stored matched set (so it re-matches): + +```ts +const matched = ctx.__mem.__values.get(MATCHED_DRAFT_IDS_KEY); +expect(matched ? JSON.parse(matched) : []).not.toContain('draft-1'); +``` + +Add a pruning test: seed `MATCHED_DRAFT_IDS_KEY` with `['gone-draft','draft-1']`, provide a snapshot only for `draft-1`; after a run, stored set excludes `gone-draft`. + +- [ ] **Step 2: Run to verify failure** + +Run: `pnpm -C /Users/josephfung/Projects/office-of-the-ceo/worktrees/curia-learning-state-json test skills/ceo-inbox-sent-observe/handler.test.ts` +Expected: FAIL. + +- [ ] **Step 3: Update `sent-observe/handler.ts`** + +- Delete the `extractMatchedDraftIds` function. Add `MATCHED_DRAFT_IDS_KEY` to the `learning-state.js` import (already importing `readIdSet`/`writeIdSet` from Task 3). +- Replace the seed `const alreadyMatchedDraftIds = extractMatchedDraftIds(pendingDiffs.body);` with: + +```ts +const seedMatchedDraftIds = await readIdSet(store, MATCHED_DRAFT_IDS_KEY); +const alreadyMatchedDraftIds = new Set(seedMatchedDraftIds); // mutable in-run guard for matchDraftToSent +``` + +- Track drafts that actually got a persisted diff this run. Near the other loop accumulators add `const newlyDiffedDraftIds = new Set();`. In the draft-match success branch (right after `diffChunks.push(formatDiffBlock(draftMatch, sentBody)); draftMatches += 1;`) add `newlyDiffedDraftIds.add(draftMatch.draftId);`. (The fetch-failure branch already skips the push, so a failed draft is NOT added.) +- After the diffs append (`diffsPersisted`), write the pruned+augmented matched set, gated on `diffsPersisted`: + +```ts +// Persist matched_draft_ids only when the diffs doc persisted. Include drafts whose diff actually +// landed this run (newlyDiffedDraftIds); EXCLUDE any whose body-fetch failed (never diffed) so +// they re-match next run — mirroring the old re-derive-from-pending-diffs behavior. Prune the +// carried-over set to drafts whose snapshot still exists (a snapshot TTL-sweeps after 7 idle days; +// once gone it can't be re-matched, so retaining its id is pointless). +if (diffsPersisted) { + const snapshotIds = new Set(snapshots.map((s) => s.draftId)); + const nextMatched = new Set([...seedMatchedDraftIds].filter((id) => snapshotIds.has(id))); + for (const id of newlyDiffedDraftIds) nextMatched.add(id); + try { + await writeIdSet(store, MATCHED_DRAFT_IDS_KEY, nextMatched); + } catch (err) { + ctx.log.warn({ err }, 'ceo-inbox-sent-observe: matched-guard write failed (diffs persisted; guard re-derives next run)'); + } +} +``` + +- The `pendingDiffs` `ensureDoc` read is still needed for the append; but its body is no longer scanned for guard ids. Keep the `ensureDoc(PENDING_DIFFS_PATH,...)` call. + +- [ ] **Step 4: Run the test file** + +Run: `pnpm -C /Users/josephfung/Projects/office-of-the-ceo/worktrees/curia-learning-state-json test skills/ceo-inbox-sent-observe/handler.test.ts` +Expected: PASS. + +- [ ] **Step 5: Typecheck + commit** + +(Version bumps deferred to Task 8.) + +```bash +git -C /Users/josephfung/Projects/office-of-the-ceo/worktrees/curia-learning-state-json add skills/ceo-inbox-sent-observe +git -C /Users/josephfung/Projects/office-of-the-ceo/worktrees/curia-learning-state-json commit -s -m "refactor(sent-observe): matched-draft guard in config-store, excludes failed-fetch drafts (#1438)" +``` + +--- + +## Task 6: Delete dead parse/format machinery + +All callers now use config JSON, so the markdown parsers/formatters and their tests are dead. Remove them and confirm nothing imports them. + +**Files:** +- Modify: `skills/_shared/learning-digest.ts`, `skills/_shared/learning-digest.test.ts` +- Modify: `skills/_shared/task-completion-risk.ts`, `skills/_shared/task-completion-risk.test.ts` +- Modify: `skills/_shared/sent-observe-match.ts`, `skills/_shared/sent-observe-match.test.ts` + +- [ ] **Step 1: Confirm no live importers of the doomed symbols** + +Run (Grep tool, not shell): search the repo for each symbol and confirm only the definition + its own test remain: +`parseCompletionDigest`, `parseVoiceGuideProposal`, `pruneGuideProposals`, `removeCompletionBlock`, `parseCompletionCandidates`, `formatUndoNote`, `formatConfirmNote`, `formatCompletionCandidateBlock`. +Expected: no imports from any `handler.ts`. + +- [ ] **Step 2: Remove from `learning-digest.ts`** + +Delete `guideProposalBlocks`, `guideFromBlock`, `parseVoiceGuideProposal`, `parseCompletionDigest`, `pruneGuideProposals`, `removeCompletionBlock`, and the `VoiceGuideProposal` interface (now in `learning-state.ts`). Keep `renderVoiceGuideSection`, `renderCompletionSection`, and the `import type { CompletionDigestItem } from './learning-state.js';` added in Task 4. + +- [ ] **Step 3: Remove from `task-completion-risk.ts`** + +Delete `ParsedCompletionCandidate`, `parseCompletionCandidates`, `formatUndoNote`, `formatConfirmNote`. Keep `TaskRisk`, `CompletionAction`, `HIGH_PRIORITY_FLOOR`, `RiskTaskLike`, `classifyTaskRisk`, `decideCompletionAction`. + +- [ ] **Step 4: Remove from `sent-observe-match.ts`** + +Delete `formatCompletionCandidateBlock`. Keep everything else (`matchDraftToSent`, `matchTasksToSent`, `formatDiffBlock`, `trimEvidenceDoc`, `tokenize`, types). + +- [ ] **Step 5: Trim the dead tests** + +In `learning-digest.test.ts` delete the `parse*`/`prune*`/`removeCompletionBlock` cases; keep the `render*` cases. In `task-completion-risk.test.ts` delete the candidate parse/format cases; keep the `classifyTaskRisk`/`decideCompletionAction` cases. In `sent-observe-match.test.ts` delete the `formatCompletionCandidateBlock` case; keep `trimEvidenceDoc`/`formatDiffBlock`/match cases. + +- [ ] **Step 6: Full test run + typecheck** + +Run: `pnpm -C /Users/josephfung/Projects/office-of-the-ceo/worktrees/curia-learning-state-json test skills/` +Expected: PASS (no references to deleted symbols). +Run: `pnpm -C /Users/josephfung/Projects/office-of-the-ceo/worktrees/curia-learning-state-json run typecheck` +Expected: no errors. + +- [ ] **Step 7: Commit** + +```bash +git -C /Users/josephfung/Projects/office-of-the-ceo/worktrees/curia-learning-state-json add skills/_shared +git -C /Users/josephfung/Projects/office-of-the-ceo/worktrees/curia-learning-state-json commit -s -m "refactor(learning): delete dead markdown parse/format machinery (#1438)" +``` + +--- + +## Task 7: Docs — ADR-029 + specs 04/13/19 + +**Files:** +- Modify: `docs/adr/029-passive-email-observation-and-counterfactual-competence.md` +- Modify: `docs/specs/19-tasks-and-backlog.md`, `docs/specs/13-office-identity.md`, `docs/specs/04-channels.md` + +- [ ] **Step 1: ADR-029 store-mapping table** + +Replace the single row `| Cadence / watermarks / guard markers | Scheduler crons + config-store |` (around L48) with two rows that separate cadence from the migrated machine-state, e.g.: + +``` +| Cadence / watermarks | Scheduler crons + `config-store` (`ceo_inbox`) | +| Queue / status / guard state (completion candidates, digest items, voice proposal, matched-draft & asked-task guards) | `config-store` JSON (`ceo_inbox`), one whole-rewritten key per store | +``` + +Add a sentence after the table noting the concurrency property: *"Machine-state keys use whole-object last-write-wins (no per-doc version check); acceptable because each key has effectively a single writer per cron tick and the queue key is keyed by task id so a held-watermark retry re-adds idempotently. Prose evidence (`pending-diffs.md`), snapshots, and shadow docs remain OKF."* + +- [ ] **Step 2: Spec 19 (L346–347)** + +Change the fuzzy-candidate guard sentence from the `completion_asked: {date}` doc-marker phrasing to config-store phrasing, e.g.: *"Fuzzy candidates are recorded in the `sent_observe.asked_task_ids` guard set (config-store, `ceo_inbox`) so they are not re-surfaced every run."* + +- [ ] **Step 3: Spec 13 (L376–380)** + +Change *"It queues a "## Guide Proposal" block (`status: pending`) in the digest"* to *"It records a single pending proposal object in `config-store` (`voice_learn.proposal`), surfaced in the digest."* Update the "Dismissed proposals get a guard marker and cooldown" line to reference the config cooldown key rather than a doc marker. + +- [ ] **Step 4: Spec 04 (L100–101)** + +Confirm the rolling OKF evidence doc (`pending-diffs.md`) description is still accurate (it is — diffs stay OKF). If L100 lists completion candidates as an OKF doc, adjust to say the completion-candidate *queue* is config-store while the `(draft, sent)` diffs remain OKF. (Verify exact wording against the file before editing.) + +- [ ] **Step 5: Commit** + +```bash +git -C /Users/josephfung/Projects/office-of-the-ceo/worktrees/curia-learning-state-json add docs/adr docs/specs +git -C /Users/josephfung/Projects/office-of-the-ceo/worktrees/curia-learning-state-json commit -s -m "docs(adr-029): machine state moved to config-store JSON; specs 04/13/19 updated (#1438)" +``` + +--- + +## Task 8: CHANGELOG + version bumps + +**Files:** +- Modify: `CHANGELOG.md`, and the five touched `skill.json` files. + +- [ ] **Step 1: Add an Unreleased entry** + +Under `## [Unreleased]` → `### Changed` (create the section if absent), add one bullet (≤15 words after the dash; use a period, hard cap enforced): + +``` +- **Learning subsystem** — queue/status/guard state moved from OKF doc bodies to config-store JSON. (#1438) +``` + +- [ ] **Step 2: Bump each touched skill's version exactly once (patch)** + +Per Joseph's standing rule, bump each `skill.json` **once per PR**, not per commit. Increment the patch component of `version` in each of: +`skills/voice-learn/skill.json`, `skills/list-learning-digest/skill.json`, `skills/resolve-learning-digest/skill.json`, `skills/ceo-inbox-sent-observe/skill.json`, `skills/task-completion-from-sent/skill.json`. + +(A behavior-preserving storage refactor is a patch-level meaningful change — worth correlating prod behavior with a known config state, but no interface change.) + +- [ ] **Step 3: Commit** + +```bash +git -C /Users/josephfung/Projects/office-of-the-ceo/worktrees/curia-learning-state-json add CHANGELOG.md skills/voice-learn/skill.json skills/list-learning-digest/skill.json skills/resolve-learning-digest/skill.json skills/ceo-inbox-sent-observe/skill.json skills/task-completion-from-sent/skill.json +git -C /Users/josephfung/Projects/office-of-the-ceo/worktrees/curia-learning-state-json commit -s -m "chore: changelog + skill version bumps for learning-state migration (#1438)" +``` + +--- + +## Final verification (before PR) + +- [ ] `pnpm -C run typecheck` — clean. +- [ ] `pnpm -C test skills/` — all learning-subsystem tests green. +- [ ] Grep confirms zero references to `PENDING_COMPLETIONS_PATH`, `PENDING_PROPOSALS_PATH`, `COMPLETION_DIGEST_PATH`, and the eight deleted parse/format symbols outside git history. +- [ ] Run the auto-review subagents (code-reviewer + silent-failure-hunter) per global CLAUDE.md before opening the PR; address high-priority findings. +- [ ] PR body includes `Closes #1438`. + +## Self-review notes (coverage against the issue's acceptance criteria) + +- *All queue/status/guard state in config JSON; no handler parses markdown bodies* → Tasks 2–6 (parsers deleted in 6). +- *Watermark-hold semantics preserved* → Task 3 (candidate-before-guard ordering + held-watermark test) and Task 5 (failed-fetch draft excluded). +- *Digest UX unchanged* → composers verbatim (Task 1 test), render helpers untouched, list/resolve round-trip tests (Tasks 2/4). +- *pending-diffs.md / snapshots / shadow docs unchanged* → only the guard *derivation* changed in Task 5; the doc format is untouched. +- *ADR-029 + specs updated; last-write-wins documented* → Task 7. +- *Unit tests: map-rewrite removal, supersede, guard persistence across held watermark* → Task 1 (removal, supersede), Task 3 (held-watermark guard), Tasks 3/5 (pruning). diff --git a/skills/_shared/learning-digest.test.ts b/skills/_shared/learning-digest.test.ts index 219751aa3..5099d2351 100644 --- a/skills/_shared/learning-digest.test.ts +++ b/skills/_shared/learning-digest.test.ts @@ -1,74 +1,23 @@ import { describe, it, expect } from 'vitest'; -import { - parseVoiceGuideProposal, - parseCompletionDigest, - renderVoiceGuideSection, - renderCompletionSection, - pruneGuideProposals, - removeCompletionBlock, -} from './learning-digest.js'; - -const GUIDE_DOC = `# Pending voice guide proposal\n\n## Guide Proposal\n- status: pending\n- generated_at: 2026-07-16T00:00:00.000Z\n\n- Writes short.\n- Dry humour.\n\n---\n`; - -// A doc that accumulated an approved block before a fresh pending one. Resolving now PRUNES -// resolved blocks (pruneGuideProposals), so this shape is transient/legacy — kept here to -// prove the parser skips resolved blocks and the pruner drops them. -const APPROVED_THEN_PENDING = `# Pending voice guide proposal\n\n## Guide Proposal\n- status: approved\n- generated_at: 2026-07-01T00:00:00.000Z\n\n- Writes short.\n\n---\n\n## Guide Proposal\n- status: pending\n- generated_at: 2026-07-16T00:00:00.000Z\n\n- Writes short.\n- Adds a dry closer.\n\n---\n`; - -const ALL_RESOLVED = `# Pending voice guide proposal\n\n## Guide Proposal\n- status: approved\n- generated_at: 2026-07-01T00:00:00.000Z\n\n- Writes short.\n\n---\n\n## Guide Proposal\n- status: dismissed\n- generated_at: 2026-07-08T00:00:00.000Z\n\n- Rejected idea.\n\n---\n`; - -const COMPLETIONS = ` -## Undo — task aaa -- status: undo_available -- task_title: Follow up with John -- note: Marked *Follow up with John* done — you emailed john@example.com. Undo? ---- -## Confirm — task bbb -- status: pending_confirm -- task_title: Plan AGM -- note: Did emailing board@example.com complete *Plan AGM*? ---- -## Undo — task ccc -- status: undone -- task_title: Old -- note: already handled ---- -`; - -describe('learning digest parsers + renderers', () => { - it('parses a pending guide proposal', () => { - expect(parseVoiceGuideProposal(GUIDE_DOC)?.guide).toContain('Dry humour'); - }); - - it('returns null when no guide proposal is present or it is not pending', () => { - expect(parseVoiceGuideProposal('')).toBeNull(); - expect( - parseVoiceGuideProposal( - `## Guide Proposal\n- status: approved\n- generated_at: x\n\n- Writes short.\n\n---\n`, - ), - ).toBeNull(); - }); - - it('returns the PENDING block when an approved block precedes it (F1 append-only repro)', () => { - const proposal = parseVoiceGuideProposal(APPROVED_THEN_PENDING); - expect(proposal).not.toBeNull(); - expect(proposal!.status).toBe('pending'); - expect(proposal!.guide).toContain('Adds a dry closer'); - // The earlier approved block's guide must not leak into the returned proposal. - expect(proposal!.guide).not.toContain('---'); - }); - - it('returns null when every block is already approved/dismissed', () => { - expect(parseVoiceGuideProposal(ALL_RESOLVED)).toBeNull(); - }); - - it('parses undo + confirm completion items; skips resolved', () => { - const items = parseCompletionDigest(COMPLETIONS); - expect(items).toHaveLength(2); - expect(items[0]!.kind).toBe('undo'); - expect(items[1]!.kind).toBe('confirm'); - }); - +import { renderVoiceGuideSection, renderCompletionSection } from './learning-digest.js'; +import type { CompletionDigestItem } from './learning-state.js'; + +const COMPLETION_ITEMS: CompletionDigestItem[] = [ + { + kind: 'undo', + taskId: 'aaa', + taskTitle: 'Follow up with John', + note: 'Marked *Follow up with John* done — you emailed john@example.com. Undo?', + }, + { + kind: 'confirm', + taskId: 'bbb', + taskTitle: 'Plan AGM', + note: 'Did emailing board@example.com complete *Plan AGM*?', + }, +]; + +describe('learning digest renderers', () => { it('renders the guide section only when present', () => { expect(renderVoiceGuideSection(null)).toBe(''); expect(renderVoiceGuideSection('- Writes short.')).toContain('### Proposed writing-voice update'); @@ -76,7 +25,7 @@ describe('learning digest parsers + renderers', () => { it('renders completion section only when items exist (snapshot)', () => { expect(renderCompletionSection([])).toBe(''); - expect(renderCompletionSection(parseCompletionDigest(COMPLETIONS))).toMatchInlineSnapshot(` + expect(renderCompletionSection(COMPLETION_ITEMS)).toMatchInlineSnapshot(` "### Task completion from sent mail 1. Marked *Follow up with John* done — you emailed john@example.com. Undo? Reply \`undo completion aaa\`. @@ -84,73 +33,4 @@ describe('learning digest parsers + renderers', () => { " `); }); - - it('prunes resolved blocks by default, keeping the pending one', () => { - const pruned = pruneGuideProposals(APPROVED_THEN_PENDING); - // The approved block (and its guide) is gone; the pending block survives. - expect(pruned).not.toContain('- Writes short.\n\n---\n\n## Guide Proposal'); - expect(pruned).toContain('- status: pending'); - expect((pruned.match(/## Guide Proposal/g) ?? []).length).toBe(1); - expect(pruned).toContain('- Adds a dry closer.'); - }); - - it('with removePending, drops every block (used on approve/dismiss so the queue does not grow)', () => { - const pruned = pruneGuideProposals(APPROVED_THEN_PENDING, { removePending: true }); - expect(pruned).not.toContain('## Guide Proposal'); - expect(pruned).not.toContain('- status:'); - // Preamble/header is preserved so voice-learn can append to it next cycle. - expect(pruned).toContain('# Pending voice guide proposal'); - }); - - it('leaves an already header-only doc unchanged', () => { - const headerOnly = '# Pending voice guide proposal\n\n'; - expect(pruneGuideProposals(headerOnly, { removePending: true })).toBe(headerOnly); - }); -}); - -describe('removeCompletionBlock', () => { - const confirmBlock = (taskId: string, status: string) => - `\n## Confirm — task ${taskId}\n\n- status: ${status}\n- risk: low\n- task_title: T\n- sent_at: 2026-07-16\n\n---\n`; - const undoBlock = (taskId: string, status: string) => - `\n## Undo — task ${taskId}\n\n- status: ${status}\n- task_title: T\n\n---\n`; - - it('does not let task id `t1` match a `t10` block (exact header only)', () => { - const body = `# Digest\n${confirmBlock('t10', 'pending_confirm')}${confirmBlock('t1', 'pending_confirm')}`; - const out = removeCompletionBlock(body, 'Confirm', 't1'); - // t1's block is removed; t10's survives. - expect(out).not.toContain('## Confirm — task t1\n'); - expect(out).toContain('## Confirm — task t10\n'); - }); - - it('removes exactly one matching block, preserving other items and the preamble', () => { - const body = `# Digest\n${undoBlock('t5', 'undo_available')}${confirmBlock('t6', 'pending_confirm')}`; - const out = removeCompletionBlock(body, 'Undo', 't5'); - expect(out).not.toContain('## Undo — task t5'); - expect(out).toContain('## Confirm — task t6'); // unrelated item untouched - expect(out).toContain('# Digest'); // preamble kept - }); - - it('only removes a block of the matching kind', () => { - const body = `# Digest\n${undoBlock('t7', 'undo_available')}`; - // A Confirm action must not remove an Undo block. - expect(removeCompletionBlock(body, 'Confirm', 't7')).toContain('## Undo — task t7'); - expect(removeCompletionBlock(body, 'Undo', 't7')).not.toContain('## Undo — task t7'); - }); - - it('removes the actionable block, not a same-task historical block in another status', () => { - // A resolved/historical block for t8 precedes the live actionable one. Matching the header - // alone would drop the historical block and leave the actionable one to be replayed after the - // task mutation already ran; matching the status too removes the right one. - const body = `# Digest\n${undoBlock('t8', 'undone')}${undoBlock('t8', 'undo_available')}`; - const out = removeCompletionBlock(body, 'Undo', 't8'); - // The actionable (undo_available) block is gone; the historical one is preserved. - expect(out).toContain('- status: undone'); - expect(out).not.toContain('- status: undo_available'); - }); - - it('is a no-op when the only same-task block is not in the actionable status', () => { - const body = `# Digest\n${confirmBlock('t9', 'pending_confirm_done')}`; - // No block matches header AND pending_confirm exactly, so nothing is removed. - expect(removeCompletionBlock(body, 'Confirm', 't9')).toBe(body); - }); }); diff --git a/skills/_shared/learning-digest.ts b/skills/_shared/learning-digest.ts index 64a45f226..77a32e4c7 100644 --- a/skills/_shared/learning-digest.ts +++ b/skills/_shared/learning-digest.ts @@ -1,65 +1,10 @@ -// Shared parsers/renderers for voice proposals + task-completion digest sections (#1425). +// Shared renderers for voice proposals + task-completion digest sections (#1425). +// The markdown parsers/formatters that used to live here were retired once all callers +// migrated to config JSON (#1438); only the render* functions remain live. -export interface VoiceGuideProposal { - status: string; - guide: string; -} - -export interface CompletionDigestItem { - kind: 'undo' | 'confirm'; - taskId: string; - taskTitle: string; - note: string; - status: string; -} - -/** Split a pending-proposals body into its `## Guide Proposal` blocks. voice-learn appends a - * block each cycle, but resolved blocks are pruned on approve/dismiss (see pruneGuideProposals), - * so in steady state the doc holds at most the single pending block. The lookahead split keeps - * each header, so blocks re-join to the exact original bytes. */ -function guideProposalBlocks(body: string): string[] { - return body.split(/(?=^## Guide Proposal)/m).filter((b) => b.startsWith('## Guide Proposal')); -} - -/** Extract the guide text from a single `## Guide Proposal` block. */ -function guideFromBlock(block: string): string { - // Guide is everything after the blank line following the metadata, up to the trailing '---'. - const afterMeta = block.replace(/^## Guide Proposal[\s\S]*?\n\n/, ''); - return afterMeta.split(/\n---\s*$/m)[0]!.trim(); -} - -export function parseVoiceGuideProposal(body: string): VoiceGuideProposal | null { - // Return the FIRST pending block (by construction there is at most one), scanning past any - // earlier approved/dismissed blocks that the append-only doc has accumulated (F1). - for (const block of guideProposalBlocks(body)) { - const status = (block.match(/- status:\s*(\S+)/)?.[1] ?? 'pending').trim(); - if (status !== 'pending') continue; - return { status, guide: guideFromBlock(block) }; - } - return null; -} - -export function parseCompletionDigest(body: string): CompletionDigestItem[] { - const items: CompletionDigestItem[] = []; - for (const section of body.split(/^## /m).slice(1)) { - const header = section.split('\n')[0] ?? ''; - const headerMatch = header.match(/^(Undo|Confirm) — task\s+(\S+)/); - if (!headerMatch) continue; - const kind = headerMatch[1] === 'Undo' ? 'undo' : 'confirm'; - const taskId = headerMatch[2]!; - const status = (section.match(/- status:\s*(\S+)/)?.[1] ?? '').trim(); - if (kind === 'undo' && status !== 'undo_available') continue; - if (kind === 'confirm' && status !== 'pending_confirm') continue; - items.push({ - kind, - taskId, - taskTitle: (section.match(/- task_title:\s*(.+)/)?.[1] ?? '').trim(), - note: (section.match(/- note:\s*(.+)/)?.[1] ?? '').trim(), - status, - }); - } - return items; -} +// CompletionDigestItem now lives in learning-state.ts (#1438) since it's also the config-store +// map value type. renderCompletionSection reads only kind/taskId/note, all present there. +import type { CompletionDigestItem } from './learning-state.js'; /** Render digest sections — empty string when no items (omit section). */ export function renderVoiceGuideSection(guide: string | null): string { @@ -89,53 +34,3 @@ export function renderCompletionSection(items: CompletionDigestItem[]): string { ]; return lines.join('\n'); } - -/** Drop `## Guide Proposal` blocks from the append-only proposals doc so it can't grow without - * bound. By default removes only resolved (non-pending) blocks; pass `removePending` to also - * drop the pending one — used on approve/dismiss, since nothing reads a resolved proposal - * afterwards (the approved guide is written to the versioned ExecutiveProfile, which is the - * real audit trail, and the dismiss cooldown lives in config). The preamble/header before the - * first block is preserved. After resolving the sole pending block the doc is just its header, - * which voice-learn re-appends to next cycle. */ -export function pruneGuideProposals( - body: string, - opts: { removePending?: boolean } = {}, -): string { - return body - .split(/(?=^## Guide Proposal)/m) - .filter((part) => { - if (!part.startsWith('## Guide Proposal')) return true; // preamble / header - const status = (part.match(/- status:\s*(\S+)/)?.[1] ?? 'pending').trim(); - return status === 'pending' && !opts.removePending; - }) - .join(''); -} - -/** Remove a completion block from the append-only completion digest once its item has been - * actioned (undone/confirmed/dismissed), so resolved items don't accumulate. A block is dropped - * only when BOTH its header and its status line match: the task id must be followed by whitespace - * or end-of-line (so id `t1` never matches a `t10` block) AND the status must be the actionable - * one for that kind (`undo_available` for Undo, `pending_confirm` for Confirm/dismiss). Matching - * the status too means a historical block for the same task in a different state is preserved - * rather than removed in place of the live actionable one (which would leave the real item to be - * replayed after the task mutation already ran). Only the first matching block is dropped; other - * items and the preamble/header are preserved. */ -export function removeCompletionBlock( - body: string, - kind: 'Undo' | 'Confirm', - taskId: string, -): string { - const esc = taskId.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); - const headerRe = new RegExp(`^## ${kind} — task ${esc}(?:\\s|$)`); - const expectedStatus = kind === 'Undo' ? 'undo_available' : 'pending_confirm'; - const statusRe = new RegExp(`^- status:\\s*${expectedStatus}\\b`, 'm'); - let removed = false; - return body - .split(/(?=^## )/m) - .filter((block) => { - if (removed || !headerRe.test(block) || !statusRe.test(block)) return true; - removed = true; - return false; // drop exactly one matching, actionable block - }) - .join(''); -} diff --git a/skills/_shared/learning-state.test.ts b/skills/_shared/learning-state.test.ts new file mode 100644 index 000000000..3cb377264 --- /dev/null +++ b/skills/_shared/learning-state.test.ts @@ -0,0 +1,216 @@ +import { describe, it, expect } from 'vitest'; +import type { ConfigStore } from '../../src/memory/config-store.js'; +import { + LEARNING_STATE_NAMESPACE, + COMPLETION_CANDIDATES_KEY, + COMPLETION_DIGEST_KEY, + VOICE_PROPOSAL_KEY, + ASKED_TASK_IDS_KEY, + readCompletionCandidates, + writeCompletionCandidates, + readCompletionDigest, + writeCompletionDigest, + readVoiceProposal, + writeVoiceProposal, + readIdSet, + writeIdSet, + composeUndoNote, + composeConfirmNote, + digestMapToItems, + type CompletionCandidateMap, + type CompletionDigestMap, +} from './learning-state.js'; + +// Minimal in-memory ConfigStore double: only get/set, keyed by config key (namespace fixed). +// set() mirrors the real ConfigStore.set()'s `{ stored: boolean }` return shape — the write +// accessors in learning-state.ts now read `.stored` off it, so a double that resolved `undefined` +// would throw at runtime the moment a write accessor's boolean return is exercised. +function fakeStore(seed: Record = {}) { + const values = new Map(Object.entries(seed)); + const store = { + get: async (_ns: string, key: string) => values.get(key) ?? null, + set: async (_ns: string, key: string, value: string) => { + values.set(key, value); + return { stored: true }; + }, + } as unknown as ConfigStore; + return { store, values }; +} + +// Minimal spy Logger double — just enough of the pino.Logger surface (warn) for the corruption +// callback learning-state.ts's read accessors invoke on a JSON.parse failure. +function fakeLogger() { + const warnCalls: Array<[Record, string]> = []; + const logger = { + warn: (obj: Record, msg: string) => { warnCalls.push([obj, msg]); }, + info: () => {}, + error: () => {}, + debug: () => {}, + } as unknown as Parameters[1]; + return { logger, warnCalls }; +} + +describe('learning-state config accessors', () => { + it('uses the ceo_inbox namespace', () => { + expect(LEARNING_STATE_NAMESPACE).toBe('ceo_inbox'); + }); + + it('reads an empty map when the key is unset or garbage', async () => { + const { store } = fakeStore({ [COMPLETION_CANDIDATES_KEY]: 'not json' }); + expect(await readCompletionCandidates(store)).toEqual({}); + expect(await readCompletionDigest(fakeStore().store)).toEqual({}); + expect(await readVoiceProposal(fakeStore().store)).toBeNull(); + expect([...(await readIdSet(fakeStore().store, ASKED_TASK_IDS_KEY))]).toEqual([]); + }); + + it('round-trips a completion-candidate map and removes by writing the map without the entry', async () => { + const { store, values } = fakeStore(); + const map: CompletionCandidateMap = { + t1: { messageId: 'm1', confidence: 'high', reason: 'r', sentAt: 's', subject: 'sub', recipients: ['a@x'], taskTitle: 'T1' }, + t10: { messageId: 'm10', confidence: 'low', reason: 'r', sentAt: 's', subject: 'sub', recipients: ['b@x'], taskTitle: 'T10' }, + }; + await writeCompletionCandidates(store, map); + expect(JSON.parse(values.get(COMPLETION_CANDIDATES_KEY)!)).toEqual(map); + + // Remove t1 by writing the map without it — t10 is untouched (no boundary bleed). + const { t1: _drop, ...rest } = await readCompletionCandidates(store); + void _drop; + await writeCompletionCandidates(store, rest); + const after = await readCompletionCandidates(store); + expect(after.t1).toBeUndefined(); + expect(after.t10).toBeDefined(); + }); + + it('supersedes a pending voice proposal by whole-object write', async () => { + const { store } = fakeStore(); + await writeVoiceProposal(store, { status: 'pending', generatedAt: 'g1', guide: 'first' }); + await writeVoiceProposal(store, { status: 'pending', generatedAt: 'g2', guide: 'second' }); + expect(await readVoiceProposal(store)).toEqual({ status: 'pending', generatedAt: 'g2', guide: 'second' }); + await writeVoiceProposal(store, null); + expect(await readVoiceProposal(store)).toBeNull(); + }); + + it('round-trips an id set', async () => { + const { store } = fakeStore(); + await writeIdSet(store, ASKED_TASK_IDS_KEY, new Set(['a', 'b'])); + expect([...(await readIdSet(store, ASKED_TASK_IDS_KEY))].sort()).toEqual(['a', 'b']); + }); + + it('round-trips a digest map and flattens to items carrying their taskId', async () => { + const { store } = fakeStore(); + const map: CompletionDigestMap = { + t1: { kind: 'undo', taskId: 't1', taskTitle: 'Follow up', note: 'n1' }, + t2: { kind: 'confirm', taskId: 't2', taskTitle: 'Plan AGM', note: 'n2' }, + }; + await writeCompletionDigest(store, map); + const items = digestMapToItems(await readCompletionDigest(store)); + expect(items).toHaveLength(2); + expect(items[0]!.taskId).toBe('t1'); + expect(items.find((i) => i.taskId === 't2')!.kind).toBe('confirm'); + }); + + it('logs corruption via the provided logger when a stored value fails to parse, and still degrades to empty', async () => { + // Finding 4: a corrupt stored value used to silently reset to empty with zero + // observability. readCompletionCandidates must still degrade to {} (skill contract) but + // now surfaces the corruption through an optional logger so it's not a silent data loss. + const { store } = fakeStore({ [COMPLETION_CANDIDATES_KEY]: 'not json' }); + const { logger, warnCalls } = fakeLogger(); + const result = await readCompletionCandidates(store, logger); + expect(result).toEqual({}); + expect(warnCalls).toHaveLength(1); + expect(warnCalls[0]![1]).toMatch(/failed to parse/); + // PII fix: the log payload carries only the key and the raw value's length — never the raw + // content itself (which can hold recipients/subjects/task-titles/proposal text). + expect(warnCalls[0]![0]).toEqual({ key: COMPLETION_CANDIDATES_KEY, rawLength: 'not json'.length }); + expect(warnCalls[0]![0]).not.toHaveProperty('rawSnippet'); + }); + + it('logs corruption and returns {} when a map key holds valid JSON with the wrong top-level shape (an array)', async () => { + // The value parses fine (JSON.parse succeeds on "[]") but a map key must hold a plain + // object, not an array — this used to pass straight through as CompletionCandidateMap. + const { store } = fakeStore({ [COMPLETION_CANDIDATES_KEY]: '[]' }); + const { logger, warnCalls } = fakeLogger(); + const result = await readCompletionCandidates(store, logger); + expect(result).toEqual({}); + expect(warnCalls).toHaveLength(1); + expect(warnCalls[0]![1]).toMatch(/failed to parse/); + expect(warnCalls[0]![0]).toMatchObject({ key: COMPLETION_CANDIDATES_KEY }); + }); + + it('logs corruption and returns null when the voice proposal holds valid JSON missing guide/status', async () => { + const { store } = fakeStore({ [VOICE_PROPOSAL_KEY]: '{"x":1}' }); + const { logger, warnCalls } = fakeLogger(); + const result = await readVoiceProposal(store, logger); + expect(result).toBeNull(); + expect(warnCalls).toHaveLength(1); + expect(warnCalls[0]![1]).toMatch(/failed to parse/); + expect(warnCalls[0]![0]).toMatchObject({ key: VOICE_PROPOSAL_KEY }); + }); + + it('does not log corruption for an unset (null) value — absence is not corruption', async () => { + const { store } = fakeStore(); + const { logger, warnCalls } = fakeLogger(); + const result = await readCompletionCandidates(store, logger); + expect(result).toEqual({}); + expect(warnCalls).toHaveLength(0); + }); + + it('drops a malformed candidate entry (would crash the consumer) but keeps the valid ones', async () => { + // `{"bad": {}}` is a structurally-valid map with a malformed value: task-completion would + // throw on `candidate.recipients[0]`. The reader drops it, keeps the good entry, and logs a + // PII-safe count (never the dropped content). + const good = { messageId: 'm1', confidence: 'high', reason: 'r', sentAt: 's', subject: 'sub', recipients: ['a@x'], taskTitle: 'T1' }; + const { store } = fakeStore({ + [COMPLETION_CANDIDATES_KEY]: JSON.stringify({ good, bad: {} }), + }); + const { logger, warnCalls } = fakeLogger(); + const result = await readCompletionCandidates(store, logger); + expect(result.good).toEqual(good); + expect(result.bad).toBeUndefined(); + expect(warnCalls).toHaveLength(1); + expect(warnCalls[0]![0]).toEqual({ key: COMPLETION_CANDIDATES_KEY, dropped: 1 }); + expect(warnCalls[0]![1]).toMatch(/dropped malformed/); + }); + + it('drops a null digest entry (would crash renderCompletionSection) rather than surfacing it', async () => { + const { store } = fakeStore({ + [COMPLETION_DIGEST_KEY]: JSON.stringify({ t1: null }), + }); + const { logger, warnCalls } = fakeLogger(); + const result = await readCompletionDigest(store, logger); + expect(result).toEqual({}); + expect(warnCalls).toHaveLength(1); + expect(warnCalls[0]![0]).toEqual({ key: COMPLETION_DIGEST_KEY, dropped: 1 }); + }); + + it('drops non-string members from a stored id set (a numeric id would silently bypass the guard)', async () => { + // `set.has("42")` never matches a numeric 42, so a non-string member would quietly defeat the + // idempotency guard rather than throw. Drop them, keep the valid string ids, log a count. + const { store } = fakeStore({ + [ASKED_TASK_IDS_KEY]: JSON.stringify(['a', 42, 'b', null]), + }); + const { logger, warnCalls } = fakeLogger(); + const result = await readIdSet(store, ASKED_TASK_IDS_KEY, logger); + expect([...result].sort()).toEqual(['a', 'b']); + expect(warnCalls).toHaveLength(1); + expect(warnCalls[0]![0]).toEqual({ key: ASKED_TASK_IDS_KEY, dropped: 2 }); + expect(warnCalls[0]![1]).toMatch(/non-string/); + }); + + it('rejects a voice proposal missing generatedAt (incomplete contract)', async () => { + const { store } = fakeStore({ + [VOICE_PROPOSAL_KEY]: JSON.stringify({ status: 'pending', guide: 'g' }), + }); + const { logger, warnCalls } = fakeLogger(); + expect(await readVoiceProposal(store, logger)).toBeNull(); + expect(warnCalls).toHaveLength(1); + expect(warnCalls[0]![0]).toMatchObject({ key: VOICE_PROPOSAL_KEY }); + }); + + it('composes undo/confirm note text verbatim to the pre-migration copy', () => { + expect(composeUndoNote({ taskTitle: 'Ship', recipient: 'a@x', sentAt: '2026-07-01T12:00:00.000Z' })) + .toBe('Marked *Ship* done — you emailed a@x (2026-07-01). Undo?'); + expect(composeConfirmNote({ taskTitle: 'Ship', recipient: 'a@x' })) + .toBe('Did emailing a@x complete *Ship*?'); + }); +}); diff --git a/skills/_shared/learning-state.ts b/skills/_shared/learning-state.ts new file mode 100644 index 000000000..ffe4615e7 --- /dev/null +++ b/skills/_shared/learning-state.ts @@ -0,0 +1,233 @@ +// Config-store JSON accessors for the email-observation learning subsystem (#1438). +// +// The queue/status/guard machine-state that used to ride inside OKF markdown doc bodies +// (completion candidates, digest items, the voice proposal, the matched/asked guard sets) now +// lives here as whole-object JSON values under the `ceo_inbox` namespace. Removal = writing the +// map without the entry (no per-item tombstones, no regex parse). Prose evidence +// (pending-diffs.md), draft snapshots, and shadow docs stay in OKF (see ADR-029). + +import type { ConfigStore } from '../../src/memory/config-store.js'; +import type { Logger } from '../../src/logger.js'; +import type { MatchConfidence } from './sent-observe-match.js'; + +/** Same namespace as the watermark/idle-backoff/checkpoint/dismiss-cooldown keys. */ +export const LEARNING_STATE_NAMESPACE = 'ceo_inbox'; + +export const COMPLETION_CANDIDATES_KEY = 'sent_observe.completion_candidates'; +export const COMPLETION_DIGEST_KEY = 'sent_observe.completion_digest'; +export const VOICE_PROPOSAL_KEY = 'voice_learn.proposal'; +export const MATCHED_DRAFT_IDS_KEY = 'sent_observe.matched_draft_ids'; +export const ASKED_TASK_IDS_KEY = 'sent_observe.asked_task_ids'; + +/** A task-completion candidate queued by sent-observe, consumed by task-completion-from-sent. */ +export interface CompletionCandidate { + messageId: string; + confidence: MatchConfidence; + reason: string; + sentAt: string; + subject: string; + recipients: string[]; + taskTitle: string; +} +/** Keyed by taskId — one open task has at most one live candidate, so re-adds are idempotent. */ +export type CompletionCandidateMap = Record; + +/** An undo/confirm item shown in the learning digest. `taskId` is carried in the value so the + * render helpers (which take a flat array) keep emitting the reply-command per item unchanged. */ +export interface CompletionDigestItem { + kind: 'undo' | 'confirm'; + taskId: string; + taskTitle: string; + note: string; +} +export type CompletionDigestMap = Record; + +export interface VoiceGuideProposal { + status: string; + generatedAt: string; + guide: string; +} + +/** Parse a stored JSON value, treating unset/garbage as absent — data loss (a dropped item) is + * worse than over-retention, and a parse throw must never escape into a skill failure. A + * `null`/empty `raw` is NOT corruption (just "never written") and does not invoke `onCorrupt`; + * only an actual JSON.parse failure on a non-empty value does — that's a genuinely corrupt + * stored value silently degrading to empty, which callers should be able to observe/log. */ +function parseJson(raw: string | null, onCorrupt?: () => void): T | null { + if (!raw) return null; + try { + return JSON.parse(raw) as T; + } catch { + onCorrupt?.(); + return null; + } +} + +/** Top-level shape guard: distinguishes "a JSON object" from an array/primitive that parsed fine + * but is the wrong top-level shape (e.g. a map key that somehow got a `"[]"` written to it). */ +function isPlainObject(v: unknown): v is Record { + return typeof v === 'object' && v !== null && !Array.isArray(v); +} + +// Per-ENTRY validators for the map stores. A structurally-valid map can still hold a malformed +// value — `{"t1": {}}` or `{"t1": null}` — which is not merely wrong, it CRASHES the consumer: +// task-completion dereferences `candidate.recipients[0]`, and renderCompletionSection dereferences +// `item.kind`. The `execute()` wrapper catches the throw, but the whole run then fails on every +// invocation while the bad entry persists. These validate only the fields actually consumed and let +// the readers DROP a bad entry (keeping the good ones) at the read boundary, so one corrupt/skewed +// entry can't break the run — and the next write persists the cleaned map (self-healing). This is +// the persistence-boundary validation the repo guideline requires before casting to a typed shape. + +function isCompletionCandidate(v: unknown): v is CompletionCandidate { + return ( + isPlainObject(v) && + typeof v.messageId === 'string' && + (v.confidence === 'high' || v.confidence === 'low') && + typeof v.reason === 'string' && + typeof v.sentAt === 'string' && + typeof v.subject === 'string' && + Array.isArray(v.recipients) && + v.recipients.every((r) => typeof r === 'string') && + typeof v.taskTitle === 'string' + ); +} + +function isCompletionDigestItem(v: unknown): v is CompletionDigestItem { + return ( + isPlainObject(v) && + (v.kind === 'undo' || v.kind === 'confirm') && + typeof v.taskId === 'string' && + typeof v.taskTitle === 'string' && + typeof v.note === 'string' + ); +} + +/** Keep only entries whose value passes `isValid`; drop the rest, logging a PII-safe count (never + * the dropped content) when anything was dropped. `parsed` is a confirmed plain object. */ +function filterValidEntries( + parsed: Record, + key: string, + isValid: (v: unknown) => v is V, + log?: Logger, +): Record { + const out: Record = {}; + let dropped = 0; + for (const [k, v] of Object.entries(parsed)) { + if (isValid(v)) out[k] = v; + else dropped += 1; + } + if (dropped > 0) { + log?.warn( + { key, dropped }, + 'learning-state: dropped malformed entries from a stored map — treating them as absent', + ); + } + return out; +} + +/** Build the onCorrupt callback shared by every read accessor below: logs a warning (only when a + * logger was supplied — the param is optional so callers/tests without a ctx.log still compile) + * identifying the key and the raw value's length, without dumping the value's actual content — + * which can carry PII (recipient addresses, subjects, task titles, proposal text) — into logs. */ +function corruptionLogger(key: string, raw: string, log?: Logger): () => void { + return () => + log?.warn( + { key, rawLength: raw.length }, + 'learning-state: stored value failed to parse — treating as empty', + ); +} + +export async function readCompletionCandidates(store: ConfigStore, log?: Logger): Promise { + const raw = await store.get(LEARNING_STATE_NAMESPACE, COMPLETION_CANDIDATES_KEY); + const parsed = parseJson(raw, raw ? corruptionLogger(COMPLETION_CANDIDATES_KEY, raw, log) : undefined); + if (parsed === null) return {}; + if (!isPlainObject(parsed)) { + corruptionLogger(COMPLETION_CANDIDATES_KEY, raw!, log)(); + return {}; + } + return filterValidEntries(parsed, COMPLETION_CANDIDATES_KEY, isCompletionCandidate, log); +} +export async function writeCompletionCandidates(store: ConfigStore, map: CompletionCandidateMap): Promise { + return (await store.set(LEARNING_STATE_NAMESPACE, COMPLETION_CANDIDATES_KEY, JSON.stringify(map))).stored; +} + +export async function readCompletionDigest(store: ConfigStore, log?: Logger): Promise { + const raw = await store.get(LEARNING_STATE_NAMESPACE, COMPLETION_DIGEST_KEY); + const parsed = parseJson(raw, raw ? corruptionLogger(COMPLETION_DIGEST_KEY, raw, log) : undefined); + if (parsed === null) return {}; + if (!isPlainObject(parsed)) { + corruptionLogger(COMPLETION_DIGEST_KEY, raw!, log)(); + return {}; + } + return filterValidEntries(parsed, COMPLETION_DIGEST_KEY, isCompletionDigestItem, log); +} +export async function writeCompletionDigest(store: ConfigStore, map: CompletionDigestMap): Promise { + return (await store.set(LEARNING_STATE_NAMESPACE, COMPLETION_DIGEST_KEY, JSON.stringify(map))).stored; +} + +export async function readVoiceProposal(store: ConfigStore, log?: Logger): Promise { + const raw = await store.get(LEARNING_STATE_NAMESPACE, VOICE_PROPOSAL_KEY); + const parsed = parseJson(raw, raw ? corruptionLogger(VOICE_PROPOSAL_KEY, raw, log) : undefined); + if (parsed === null) return null; + // Validate every field of the (small, fixed) proposal contract, not just the consumed ones: our + // writer always emits all three, so requiring generatedAt never drops a real proposal, and a + // proposal missing it is genuinely malformed — dropping it (voice-learn re-proposes next run) is + // safer than casting an incomplete object. + if ( + !isPlainObject(parsed) || + typeof parsed.status !== 'string' || + typeof parsed.generatedAt !== 'string' || + typeof parsed.guide !== 'string' + ) { + corruptionLogger(VOICE_PROPOSAL_KEY, raw!, log)(); + return null; + } + return parsed as unknown as VoiceGuideProposal; +} +export async function writeVoiceProposal(store: ConfigStore, proposal: VoiceGuideProposal | null): Promise { + return (await store.set(LEARNING_STATE_NAMESPACE, VOICE_PROPOSAL_KEY, JSON.stringify(proposal))).stored; +} + +export async function readIdSet(store: ConfigStore, key: string, log?: Logger): Promise> { + const raw = await store.get(LEARNING_STATE_NAMESPACE, key); + const parsed = parseJson(raw, raw ? corruptionLogger(key, raw, log) : undefined); + if (parsed !== null && !Array.isArray(parsed)) { + // Parsed fine but the wrong top-level shape (e.g. a map object stored under an id-set key) — + // route through the same corruption logger so this isn't a silent empty-set degrade. + corruptionLogger(key, raw!, log)(); + return new Set(); + } + if (!Array.isArray(parsed)) return new Set(); // null/unset — absence, not corruption + // Validate every MEMBER is a string, not just that the top level is an array. A non-string member + // (e.g. `[42]` or `["t1", null]`) wouldn't throw — it'd silently sit in the Set and bypass the + // guard, since `set.has("42")` never matches the numeric `42`. Drop non-strings, logging a + // PII-safe count, so a corrupt/skewed member can't quietly defeat an idempotency guard. + const valid = parsed.filter((v): v is string => typeof v === 'string'); + if (valid.length !== parsed.length) { + log?.warn( + { key, dropped: parsed.length - valid.length }, + 'learning-state: dropped non-string entries from a stored id set — treating them as absent', + ); + } + return new Set(valid); +} +export async function writeIdSet(store: ConfigStore, key: string, ids: Set): Promise { + return (await store.set(LEARNING_STATE_NAMESPACE, key, JSON.stringify([...ids]))).stored; +} + +/** Flatten the digest map to the array the render helpers consume, preserving insertion order. */ +export function digestMapToItems(map: CompletionDigestMap): CompletionDigestItem[] { + return Object.values(map); +} + +/** Human note for an auto-completed task's undo affordance. Verbatim to the pre-migration + * formatUndoNote copy so the digest UX is byte-identical. */ +export function composeUndoNote(params: { taskTitle: string; recipient: string; sentAt: string }): string { + const when = params.sentAt ? ` (${params.sentAt.slice(0, 10)})` : ''; + return `Marked *${params.taskTitle}* done — you emailed ${params.recipient}${when}. Undo?`; +} + +/** Human note for a confirm-in-digest item. Verbatim to the pre-migration formatConfirmNote copy. */ +export function composeConfirmNote(params: { taskTitle: string; recipient: string }): string { + return `Did emailing ${params.recipient} complete *${params.taskTitle}*?`; +} diff --git a/skills/_shared/sent-observe-match.test.ts b/skills/_shared/sent-observe-match.test.ts index d0132a7b9..59a622df9 100644 --- a/skills/_shared/sent-observe-match.test.ts +++ b/skills/_shared/sent-observe-match.test.ts @@ -3,14 +3,11 @@ import { matchDraftToSent, matchTasksToSent, formatDiffBlock, - formatCompletionCandidateBlock, trimEvidenceDoc, type DraftSnapshotLike, type SentMessageLike, - type TaskMatch, } from './sent-observe-match.js'; import { parsePendingDiffs } from './voice-learn-logic.js'; -import { parseCompletionCandidates } from './task-completion-risk.js'; const baseSent: SentMessageLike = { id: 'msg-1', @@ -174,20 +171,6 @@ describe('trimEvidenceDoc', () => { return formatDiffBlock(match, SENT_BODY); } - function completionBlock(date: number, taskId: string): string { - const match: TaskMatch = { - messageId: `msg-${date}`, - taskId, - confidence: 'high', - reason: 'recipient+semantic', - sentSubject: 'Follow up', - sentRecipients: ['alice@example.com'], - sentAt: new Date(date * 1000).toISOString(), - taskTitle: 'Follow up with Alice', - }; - return formatCompletionCandidateBlock(match); - } - const OLD = 1_600_000_000; // 2020-09-13 const NEW = 1_720_000_000; // 2024-07-03 const CUTOFF = new Date(1_700_000_000_000).toISOString(); // 2023-11-14, between OLD and NEW @@ -243,15 +226,6 @@ describe('trimEvidenceDoc', () => { expect(trimmed).toContain('draft dm'); }); - it('trims completion blocks too (re-parses via parseCompletionCandidates)', () => { - const body = `# Pending task-completion candidates\n${completionBlock(OLD, 'task-old')}${completionBlock(NEW, 'task-new')}`; - const trimmed = trimEvidenceDoc(body, CUTOFF); - - const candidates = parseCompletionCandidates(trimmed); - expect(candidates).toHaveLength(1); - expect(candidates[0]!.taskId).toBe('task-new'); - }); - it('returns an empty / block-free body unchanged', () => { expect(trimEvidenceDoc('', CUTOFF)).toBe(''); expect(trimEvidenceDoc('# Just a header\n\nno blocks here\n', CUTOFF)).toBe( diff --git a/skills/_shared/sent-observe-match.ts b/skills/_shared/sent-observe-match.ts index 41eb9c763..25c094610 100644 --- a/skills/_shared/sent-observe-match.ts +++ b/skills/_shared/sent-observe-match.ts @@ -303,8 +303,9 @@ export function formatDiffBlock(match: DraftMatch, sentBody: string): string { * default idle scratch sweep (purgeExpiredScratch). That single mechanism is their retention. * * Boundaries: each block starts at a real `## Diff — ` / `## Candidate — ` header and runs up to - * (not including) the next such header — the same headers formatDiffBlock/formatCompletionCandidateBlock - * produce and parsePendingDiffs/parseCompletionCandidates consume, so the result round-trips. We split + * (not including) the next such header — the same headers formatDiffBlock produces (and, historically, + * the retired completion-candidate formatter did) and parsePendingDiffs consumes, so the result + * round-trips. We split * on the namespaced headers, NOT any `## ` line, so a `## `-prefixed line inside a sent email body * (a markdown H2 surviving htmlToPlainText) stays part of its block instead of mis-splitting it and * leaving a timestamp-less tail behind. Any leading preamble/header before the first block is @@ -337,23 +338,3 @@ export function trimEvidenceDoc(body: string, cutoffIso: string): string { } return kept.join(''); } - -/** Format a completion candidate block for pending-completions.md. */ -export function formatCompletionCandidateBlock(match: TaskMatch): string { - return [ - '', - `## Candidate — task ${match.taskId}`, - '', - `- message_id: ${match.messageId}`, - `- confidence: ${match.confidence}`, - `- reason: ${match.reason}`, - `- sent_at: ${match.sentAt}`, - `- subject: ${match.sentSubject}`, - `- recipients: ${match.sentRecipients.join(', ')}`, - `- task_title: ${match.taskTitle}`, - `- status: pending`, - '', - '---', - '', - ].join('\n'); -} diff --git a/skills/_shared/task-completion-risk.test.ts b/skills/_shared/task-completion-risk.test.ts index 1bcd31aaa..4847eaf88 100644 --- a/skills/_shared/task-completion-risk.test.ts +++ b/skills/_shared/task-completion-risk.test.ts @@ -2,7 +2,6 @@ import { describe, it, expect } from 'vitest'; import { classifyTaskRisk, decideCompletionAction, - parseCompletionCandidates, HIGH_PRIORITY_FLOOR, } from './task-completion-risk.js'; @@ -94,35 +93,3 @@ describe('decideCompletionAction', () => { expect(decideCompletionAction('high')).toBe('confirm'); }); }); - -describe('parseCompletionCandidates', () => { - it('parses pending candidates and skips asked ones', () => { - const body = ` -## Candidate — task task-a -- message_id: m1 -- confidence: high -- reason: recipient+semantic -- sent_at: 2026-07-01T00:00:00.000Z -- subject: Hi -- recipients: a@example.com -- task_title: Follow up with A -- status: pending ---- -## Candidate — task task-b -- message_id: m2 -- confidence: low -- reason: semantic -- sent_at: 2026-07-02T00:00:00.000Z -- subject: Hi -- recipients: b@example.com -- task_title: Fuzzy thing -- status: pending -- completion_asked: {2026-07-02} ---- -`; - const parsed = parseCompletionCandidates(body); - expect(parsed).toHaveLength(1); - expect(parsed[0]!.taskId).toBe('task-a'); - expect(parsed[0]!.confidence).toBe('high'); - }); -}); diff --git a/skills/_shared/task-completion-risk.ts b/skills/_shared/task-completion-risk.ts index 2e083d8cd..caeb30bb0 100644 --- a/skills/_shared/task-completion-risk.ts +++ b/skills/_shared/task-completion-risk.ts @@ -6,7 +6,6 @@ import { readPlanBlock } from '../../src/db/plan-progress.js'; // pattern in src/security/export-controls.ts). import { isConfidentialOrAbove } from '../../src/memory/sensitivity.js'; import type { Sensitivity } from '../../src/memory/types.js'; -import type { MatchConfidence } from './sent-observe-match.js'; export type TaskRisk = 'low' | 'high'; export type CompletionAction = 'auto_complete' | 'confirm'; @@ -59,91 +58,3 @@ export function classifyTaskRisk( export function decideCompletionAction(risk: TaskRisk): CompletionAction { return risk === 'low' ? 'auto_complete' : 'confirm'; } - -export interface ParsedCompletionCandidate { - taskId: string; - messageId: string; - confidence: MatchConfidence; - reason: string; - sentAt: string; - subject: string; - recipients: string[]; - taskTitle: string; - status: string; -} - -/** Parse pending-completions.md candidate blocks. */ -export function parseCompletionCandidates(body: string): ParsedCompletionCandidate[] { - const out: ParsedCompletionCandidate[] = []; - const sections = body.split(/^## Candidate — task\s+/m).slice(1); - for (const section of sections) { - const taskId = (section.split('\n')[0] ?? '').trim(); - if (!taskId) continue; - const status = (section.match(/- status:\s*(\S+)/)?.[1] ?? 'pending').trim(); - if (status !== 'pending') continue; - if (/completion_asked:/i.test(section)) continue; - const confidenceRaw = (section.match(/- confidence:\s*(\S+)/)?.[1] ?? 'low').trim(); - const confidence: MatchConfidence = confidenceRaw === 'high' ? 'high' : 'low'; - out.push({ - taskId, - messageId: (section.match(/- message_id:\s*(\S+)/)?.[1] ?? '').trim(), - confidence, - reason: (section.match(/- reason:\s*(.+)/)?.[1] ?? '').trim(), - sentAt: (section.match(/- sent_at:\s*(.+)/)?.[1] ?? '').trim(), - subject: (section.match(/- subject:\s*(.+)/)?.[1] ?? '').trim(), - recipients: ((section.match(/- recipients:\s*(.+)/)?.[1] ?? '') - .split(',') - .map((s) => s.trim()) - .filter(Boolean)), - taskTitle: (section.match(/- task_title:\s*(.+)/)?.[1] ?? '').trim(), - status, - }); - } - return out; -} - -export function formatUndoNote(params: { - taskId: string; - taskTitle: string; - recipient: string; - sentAt: string; -}): string { - const when = params.sentAt ? ` (${params.sentAt.slice(0, 10)})` : ''; - return [ - '', - `## Undo — task ${params.taskId}`, - '', - `- status: undo_available`, - `- task_title: ${params.taskTitle}`, - `- note: Marked *${params.taskTitle}* done — you emailed ${params.recipient}${when}. Undo?`, - '', - '---', - '', - ].join('\n'); -} - -export function formatConfirmNote(params: { - taskId: string; - taskTitle: string; - recipient: string; - sentAt: string; - confidence: MatchConfidence; - // Undefined for low-confidence candidates, whose risk is never classified (T3.1). - risk?: TaskRisk; -}): string { - return [ - '', - `## Confirm — task ${params.taskId}`, - '', - `- status: pending_confirm`, - `- risk: ${params.risk ?? 'unassessed'}`, - `- confidence: ${params.confidence}`, - `- task_title: ${params.taskTitle}`, - `- note: Did emailing ${params.recipient} complete *${params.taskTitle}*?`, - `- sent_at: ${params.sentAt}`, - `- completion_asked: {${new Date().toISOString().slice(0, 10)}}`, - '', - '---', - '', - ].join('\n'); -} diff --git a/skills/ceo-inbox-sent-observe/handler.test.ts b/skills/ceo-inbox-sent-observe/handler.test.ts index 12ca81e33..e861b37e8 100644 --- a/skills/ceo-inbox-sent-observe/handler.test.ts +++ b/skills/ceo-inbox-sent-observe/handler.test.ts @@ -5,7 +5,6 @@ import { WATERMARK_KEY, IDLE_BACKOFF_KEY, PENDING_DIFFS_PATH, - PENDING_COMPLETIONS_PATH, } from './handler.js'; import type { SkillContext } from '../../src/skills/types.js'; import type { EntityMemory } from '../../src/memory/entity-memory.js'; @@ -13,6 +12,7 @@ import { VOICE_LEARNING_DOC_TYPE } from '../_shared/voice-learning-capture.js'; import { SHADOW_DOC_TYPE, shadowDraftPath } from '../_shared/shadow-draft.js'; import type { ActionLogInsert } from '../../src/autonomy/action-log-types.js'; import type { InfraLlm } from '../../src/skills/infra-llm.js'; +import { COMPLETION_CANDIDATES_KEY, ASKED_TASK_IDS_KEY, MATCHED_DRAFT_IDS_KEY } from '../_shared/learning-state.js'; function makeEntityMemory(seed: Record = {}): EntityMemory & { __values: Map; @@ -307,6 +307,9 @@ describe('CeoInboxSentObserveHandler', () => { const diffs = ctx.__docs.get(PENDING_DIFFS_PATH); expect(diffs?.body).toContain('draft draft-1'); expect(diffs?.body).toContain('msg-sent-1'); + + // The matched-draft guard is now stored in config, not re-derived from pending-diffs.md. + expect(JSON.parse(ctx.__mem.__values.get(MATCHED_DRAFT_IDS_KEY)!)).toContain('draft-1'); }); it('holds the watermark and persists no diff when a matched draft body cannot be fetched (F8)', async () => { @@ -371,6 +374,163 @@ describe('CeoInboxSentObserveHandler', () => { expect(ctx.__mem.__values.get(WATERMARK_KEY)).toBeUndefined(); // No diff was persisted from the truncated snippet. expect(ctx.__docs.get(PENDING_DIFFS_PATH)?.body ?? '').not.toContain('draft draft-1'); + // The failed-fetch draft must NOT be in the stored matched set — it never got a diff, so it + // must re-match next run (mirrors the old re-derive-from-pending-diffs behavior). + const matched = ctx.__mem.__values.get(MATCHED_DRAFT_IDS_KEY); + expect(matched ? JSON.parse(matched) : []).not.toContain('draft-1'); + }); + + it('prunes matched_draft_ids to drafts whose snapshot still exists', async () => { + // Seed the guard with a stale draft id ('gone-draft') whose snapshot doc no longer exists + // (TTL-swept) alongside 'draft-1', whose snapshot is still present. After a run that matches + // draft-1, the stored set must drop gone-draft but keep draft-1. + const listResponse = { + data: [ + { + id: 'msg-sent-1', + thread_id: 'thread-1', + subject: 'Re: Hello', + from: [{ email: 'ceo@example.com' }], + to: [{ email: 'alice@example.com' }], + cc: [], + snippet: 'Thanks Alice', + date: 1_720_000_200, + unread: false, + folders: ['SENT'], + attachments: [], + }, + ], + }; + const fullResponse = { + data: { + ...listResponse.data[0], + body: '

Thanks Alice — following up.

', + bcc: [], + labels: [], + }, + }; + + mockFetch.mockImplementation(async (url: Parameters[0]) => { + const u = String(url); + if (u.includes('/messages/msg-sent-1')) { + return new Response(JSON.stringify(fullResponse), { status: 200 }); + } + if (u.includes('/messages?')) { + return new Response(JSON.stringify(listResponse), { status: 200 }); + } + throw new Error(`unexpected ${u}`); + }); + + const ctx = buildCtx({ + force: true, + nowMs: 1_720_100_000_000, + seed: { [MATCHED_DRAFT_IDS_KEY]: JSON.stringify(['gone-draft', 'draft-1']) }, + snapshots: [ + { + path: '/scratch/voice-learning/draft-1.md', + type: VOICE_LEARNING_DOC_TYPE, + frontmatter: { + draft_id: 'draft-1', + thread_id: 'thread-1', + subject: 'Re: Hello', + recipients: { to: [{ email: 'alice@example.com' }], cc: [] }, + created_at: '2024-07-03T00:00:00.000Z', + }, + body: 'Thanks Alice — following up.', + }, + ], + tasks: [], + }); + + const result = await handler.execute(ctx); + expect(result.success).toBe(true); + + const matched = JSON.parse(ctx.__mem.__values.get(MATCHED_DRAFT_IDS_KEY)!); + expect(matched).not.toContain('gone-draft'); + expect(matched).toContain('draft-1'); + }); + + it('holds the watermark when the matched-draft guard write soft-rejects (stored:false, no throw)', async () => { + // Same matched-draft-and-diff-persisted setup as the happy-path draft test, but this time + // the MATCHED_DRAFT_IDS_KEY write soft-rejects (stored:false, no throw) — the real dedup + // 'conflict'/'auto_rejected' shape storeFact can hit. The guard no longer re-derives from the + // pending-diffs doc (extractMatchedDraftIds was deleted in #1438), so a lost guard write must + // hold the watermark for retry rather than being treated as harmless. + const listResponse = { + data: [ + { + id: 'msg-sent-1', + thread_id: 'thread-1', + subject: 'Re: Hello', + from: [{ email: 'ceo@example.com' }], + to: [{ email: 'alice@example.com' }], + cc: [], + snippet: 'Thanks Alice', + date: 1_720_000_200, + unread: false, + folders: ['SENT'], + attachments: [], + }, + ], + }; + const fullResponse = { + data: { + ...listResponse.data[0], + body: '

Thanks Alice — following up.

', + bcc: [], + labels: [], + }, + }; + + mockFetch.mockImplementation(async (url: Parameters[0]) => { + const u = String(url); + if (u.includes('/messages/msg-sent-1')) { + return new Response(JSON.stringify(fullResponse), { status: 200 }); + } + if (u.includes('/messages?')) { + return new Response(JSON.stringify(listResponse), { status: 200 }); + } + throw new Error(`unexpected ${u}`); + }); + + const ctx = buildCtx({ + force: true, + nowMs: 1_720_100_000_000, + snapshots: [ + { + path: '/scratch/voice-learning/draft-1.md', + type: VOICE_LEARNING_DOC_TYPE, + frontmatter: { + draft_id: 'draft-1', + thread_id: 'thread-1', + subject: 'Re: Hello', + recipients: { to: [{ email: 'alice@example.com' }], cc: [] }, + created_at: '2024-07-03T00:00:00.000Z', + }, + body: 'Thanks Alice — following up.', + }, + ], + tasks: [], + }); + + // Soft-reject only the matched-draft-guard write — every other storeFact call (diffs doc is + // an OKF write, not config; watermark/idle-backoff) goes through normally. + (ctx.__mem.storeFact as ReturnType).mockImplementation( + async (params: { label: string; properties?: Record }) => { + if (params.label === MATCHED_DRAFT_IDS_KEY) { + return { stored: false, action: 'conflict' as const }; + } + ctx.__mem.__values.set(params.label, String(params.properties?.value ?? '')); + return { stored: true, action: 'created' as const }; + }, + ); + + const result = await handler.execute(ctx); + expect(result.success).toBe(true); + const data = (result as { data: { watermark_advanced_to: number | null } }).data; + // The watermark must be HELD so the send is re-observed and the guard re-written next run. + expect(data.watermark_advanced_to).toBeNull(); + expect(ctx.__mem.__values.has(MATCHED_DRAFT_IDS_KEY)).toBe(false); }); it('persists task-completion candidates for open CEO tasks', async () => { @@ -443,8 +603,221 @@ describe('CeoInboxSentObserveHandler', () => { expect(result.success).toBe(true); expect((result as { data: { task_candidates: number } }).data.task_candidates).toBe(1); - const pending = ctx.__docs.get(PENDING_COMPLETIONS_PATH); - expect(pending?.body).toContain('task-ceo-1'); + const stored = JSON.parse(ctx.__mem.__values.get(COMPLETION_CANDIDATES_KEY)!); + expect(stored['task-ceo-1']).toBeDefined(); + expect(stored['task-ceo-1'].confidence).toBe('high'); + const asked = JSON.parse(ctx.__mem.__values.get(ASKED_TASK_IDS_KEY)!); + expect(asked).toContain('task-ceo-1'); + }); + + it('does not persist the asked-guard when the candidate write is held (no candidate lost)', async () => { + // A matched task, but the completion_candidates config write throws → completionsPersisted + // is false. That must HOLD the watermark AND skip writing asked_task_ids — otherwise the next + // run's guard would already contain the task and matchTasksToSent would silently skip it, + // permanently losing the candidate that never made it into the queue. + mockFetch.mockImplementation(async (url: Parameters[0]) => { + const u = String(url); + if (u.includes('/messages/msg-sent-2')) { + return new Response( + JSON.stringify({ + data: { + id: 'msg-sent-2', + thread_id: 't2', + subject: 'Follow up', + from: [{ email: 'ceo@example.com' }], + to: [{ email: 'alice@example.com' }], + cc: [], + body: '

Following up on our chat

', + snippet: 'Following up', + date: 1_720_000_300, + unread: false, + folders: ['SENT'], + bcc: [], + labels: [], + attachments: [], + }, + }), + { status: 200 }, + ); + } + if (u.includes('/messages?')) { + return new Response( + JSON.stringify({ + data: [ + { + id: 'msg-sent-2', + thread_id: 't2', + subject: 'Follow up', + from: [{ email: 'ceo@example.com' }], + to: [{ email: 'alice@example.com' }], + cc: [], + snippet: 'Following up on our chat with Alice', + date: 1_720_000_300, + unread: false, + folders: ['SENT'], + attachments: [], + }, + ], + }), + { status: 200 }, + ); + } + throw new Error(`unexpected ${u}`); + }); + + const ctx = buildCtx({ + force: true, + nowMs: 1_720_100_000_000, + tasks: [ + { + id: 'task-ceo-1', + title: 'Follow up with Alice', + description: 'Email alice@example.com about the chat', + tags: [], + priority: 40, + }, + ], + }); + + // Drive the failure: reject only the completion-candidates config write, letting every + // other storeFact call (watermark, idle-backoff) through normally — mirrors the + // checkpoint-write-failure pattern in voice-learn's handler.test.ts. + (ctx.__mem.storeFact as ReturnType).mockImplementation( + async (params: { label: string; properties?: Record }) => { + if (params.label === COMPLETION_CANDIDATES_KEY) throw new Error('store down'); + ctx.__mem.__values.set(params.label, String(params.properties?.value ?? '')); + return { stored: true, action: 'created' as const }; + }, + ); + + const result = await handler.execute(ctx); + expect(result.success).toBe(true); + const data = (result as { data: { watermark_advanced_to: number | null } }).data; + expect(data.watermark_advanced_to).toBeNull(); + // The asked-guard was never written this run — next run re-matches and re-adds. + expect(ctx.__mem.__values.has(ASKED_TASK_IDS_KEY)).toBe(false); + }); + + it('holds the watermark and skips the asked-guard when the candidate write SOFT-rejects (stored:false, no throw)', async () => { + // Same shape as the hard-failure test above, but this time storeFact does NOT throw — it + // resolves normally with { stored: false } (the real dedup 'conflict'/'auto_rejected' shape + // ConfigStore.set can hit). Before the fix, completionsPersisted was only ever flipped false + // in the catch block, so a soft-reject like this one would sail through as "persisted" even + // though nothing was actually written — silently losing the candidate while still advancing + // the watermark past the sends that produced it. This is the HIGH-severity regression case. + mockFetch.mockImplementation(async (url: Parameters[0]) => { + const u = String(url); + if (u.includes('/messages/msg-sent-2')) { + return new Response( + JSON.stringify({ + data: { + id: 'msg-sent-2', + thread_id: 't2', + subject: 'Follow up', + from: [{ email: 'ceo@example.com' }], + to: [{ email: 'alice@example.com' }], + cc: [], + body: '

Following up on our chat

', + snippet: 'Following up', + date: 1_720_000_300, + unread: false, + folders: ['SENT'], + bcc: [], + labels: [], + attachments: [], + }, + }), + { status: 200 }, + ); + } + if (u.includes('/messages?')) { + return new Response( + JSON.stringify({ + data: [ + { + id: 'msg-sent-2', + thread_id: 't2', + subject: 'Follow up', + from: [{ email: 'ceo@example.com' }], + to: [{ email: 'alice@example.com' }], + cc: [], + snippet: 'Following up on our chat with Alice', + date: 1_720_000_300, + unread: false, + folders: ['SENT'], + attachments: [], + }, + ], + }), + { status: 200 }, + ); + } + throw new Error(`unexpected ${u}`); + }); + + const ctx = buildCtx({ + force: true, + nowMs: 1_720_100_000_000, + tasks: [ + { + id: 'task-ceo-1', + title: 'Follow up with Alice', + description: 'Email alice@example.com about the chat', + tags: [], + priority: 40, + }, + ], + }); + + // Soft-reject only the completion-candidates write — resolves normally with stored:false, + // mirroring a dedup 'conflict'/'auto_rejected' storeFact outcome rather than an infra error. + (ctx.__mem.storeFact as ReturnType).mockImplementation( + async (params: { label: string; properties?: Record }) => { + if (params.label === COMPLETION_CANDIDATES_KEY) { + return { stored: false, action: 'conflict' as const }; + } + ctx.__mem.__values.set(params.label, String(params.properties?.value ?? '')); + return { stored: true, action: 'created' as const }; + }, + ); + + const result = await handler.execute(ctx); + expect(result.success).toBe(true); + const data = (result as { data: { watermark_advanced_to: number | null } }).data; + // The watermark must be HELD (not advanced) so the candidate-producing send is re-observed. + expect(data.watermark_advanced_to).toBeNull(); + expect(ctx.__mem.__values.has(COMPLETION_CANDIDATES_KEY)).toBe(false); + // The asked-guard must NOT have been persisted with this task — otherwise next run's + // matchTasksToSent would skip it via the guard while the candidate was never actually queued. + expect(ctx.__mem.__values.has(ASKED_TASK_IDS_KEY)).toBe(false); + }); + + it('prunes asked_task_ids to currently-open tasks on write', async () => { + // closed-task was asked about previously but is no longer open (completed/cancelled). + // task-ceo-1 is still open. After a run, the persisted guard should drop closed-task. + mockFetch.mockResolvedValue(new Response(JSON.stringify({ data: [] }), { status: 200 })); + + const ctx = buildCtx({ + force: true, + nowMs: 1_720_100_000_000, + seed: { [ASKED_TASK_IDS_KEY]: JSON.stringify(['closed-task', 'task-ceo-1']) }, + tasks: [ + { + id: 'task-ceo-1', + title: 'Follow up with Alice', + description: 'Email alice@example.com about the chat', + tags: [], + priority: 40, + }, + ], + }); + + const result = await handler.execute(ctx); + expect(result.success).toBe(true); + + const asked = JSON.parse(ctx.__mem.__values.get(ASKED_TASK_IDS_KEY)!); + expect(asked).not.toContain('closed-task'); + expect(asked).toContain('task-ceo-1'); }); it('reconciles shadow drafts via a batched LLM judge', async () => { diff --git a/skills/ceo-inbox-sent-observe/handler.ts b/skills/ceo-inbox-sent-observe/handler.ts index 286c76268..918e0220b 100644 --- a/skills/ceo-inbox-sent-observe/handler.ts +++ b/skills/ceo-inbox-sent-observe/handler.ts @@ -15,7 +15,6 @@ import { VOICE_LEARNING_SCRATCH_PREFIX, } from '../_shared/voice-learning-capture.js'; import { - formatCompletionCandidateBlock, formatDiffBlock, matchDraftToSent, matchTasksToSent, @@ -30,14 +29,23 @@ import { type ShadowJudgePair, type ShadowSnapshot, } from '../_shared/shadow-draft.js'; +import { + readCompletionCandidates, + writeCompletionCandidates, + readIdSet, + writeIdSet, + ASKED_TASK_IDS_KEY, + MATCHED_DRAFT_IDS_KEY, + COMPLETION_CANDIDATES_KEY, + type CompletionCandidateMap, + type CompletionCandidate, +} from '../_shared/learning-state.js'; export const CONFIG_NAMESPACE = 'ceo_inbox'; export const WATERMARK_KEY = 'sent_observe.last_seen_at'; export const IDLE_BACKOFF_KEY = 'sent_observe.last_run_found_nothing_at'; export const PENDING_DIFFS_PATH = `${VOICE_LEARNING_SCRATCH_PREFIX}/pending-diffs.md`; -export const PENDING_COMPLETIONS_PATH = `${VOICE_LEARNING_SCRATCH_PREFIX}/pending-completions.md`; export const PENDING_DIFFS_TYPE = 'voice-pending-diffs'; -export const PENDING_COMPLETIONS_TYPE = 'voice-pending-completions'; /** Calendar-time retention bound for the rolling evidence docs (pending-diffs.md / * pending-completions.md), per ADR-029: their sensitive full email bodies must not be retained @@ -95,24 +103,6 @@ function parseSnapshot(doc: { }; } -function extractMatchedDraftIds(pendingDiffsBody: string): Set { - const ids = new Set(); - for (const m of pendingDiffsBody.matchAll(/draft\s+([^\s↔]+)\s*↔/g)) { - if (m[1]) ids.add(m[1]); - } - return ids; -} - -function extractAskedTaskIds(pendingCompletionsBody: string): Set { - // Every candidate (including ones later stamped with a `completion_asked` marker) - // keeps its `## Candidate — task ` header, so this single pass covers them all. - const ids = new Set(); - for (const m of pendingCompletionsBody.matchAll(/## Candidate — task\s+(\S+)/g)) { - if (m[1]) ids.add(m[1]); - } - return ids; -} - /** ISO timestamp → Unix seconds, or null when missing/unparseable. */ function isoToUnixSeconds(iso: string): number | null { if (!iso) return null; @@ -306,15 +296,19 @@ export class CeoInboxSentObserveHandler implements SkillHandler { .map(parseSnapshot) .filter((s): s is DraftSnapshotLike => s !== null); - const pendingDiffs = await ensureDoc(ctx, PENDING_DIFFS_PATH, PENDING_DIFFS_TYPE, 'Pending voice diffs'); - const pendingCompletions = await ensureDoc( - ctx, - PENDING_COMPLETIONS_PATH, - PENDING_COMPLETIONS_TYPE, - 'Pending task-completion candidates', - ); - const alreadyMatchedDraftIds = extractMatchedDraftIds(pendingDiffs.body); - const alreadyAskedTaskIds = extractAskedTaskIds(pendingCompletions.body); + // Ensure the doc exists ahead of the append below (appendAndTrimDoc re-reads it itself); its + // body is no longer scanned for guard ids (#1438 — the matched-draft guard now lives in + // config, seeded just below), so the result isn't bound to a variable. + await ensureDoc(ctx, PENDING_DIFFS_PATH, PENDING_DIFFS_TYPE, 'Pending voice diffs'); + // Seed the matched-draft guard from config (replaces the doc-derived extractMatchedDraftIds + // scan). seedMatchedDraftIds is kept separate (immutable snapshot of what's persisted) from + // alreadyMatchedDraftIds (the mutable in-run guard matchDraftToSent adds to below) so the + // final write can be built from the seed + only-this-run's successfully diffed drafts, + // deliberately excluding any draft matched-but-failed-to-fetch this run (see the write below). + const seedMatchedDraftIds = await readIdSet(store, MATCHED_DRAFT_IDS_KEY, ctx.log); + const alreadyMatchedDraftIds = new Set(seedMatchedDraftIds); + // Seed the asked-guard from config (replaces the doc-derived extractAskedTaskIds scan). + const alreadyAskedTaskIds = await readIdSet(store, ASKED_TASK_IDS_KEY, ctx.log); const OPEN_TASK_LIMIT = 100; const openTasks = await ctx.taskRepo.listTasks({ @@ -346,7 +340,12 @@ export class CeoInboxSentObserveHandler implements SkillHandler { // (see the watermark-advance block below) — minDate does NOT hold it back. let minDate = Number.POSITIVE_INFINITY; const diffChunks: string[] = []; - const completionChunks: string[] = []; + // Drafts that actually got a diff persisted THIS run (as opposed to alreadyMatchedDraftIds, + // which also gains a draftId on a matched-but-fetch-failed draft — see the message loop). + // Only this set feeds the matched_draft_ids write below, so a failed-fetch draft is never + // durably marked matched and can re-match on retry. + const newlyDiffedDraftIds = new Set(); + const newCandidates: CompletionCandidateMap = {}; const shadowDocs = await ctx.workingDocs.listByPrefix(`${SHADOW_SCRATCH_PREFIX}/`); const shadows = shadowDocs @@ -385,6 +384,7 @@ export class CeoInboxSentObserveHandler implements SkillHandler { // watermark-advanced, never be repaired. diffChunks.push(formatDiffBlock(draftMatch, sentBody)); draftMatches += 1; + newlyDiffedDraftIds.add(draftMatch.draftId); } catch (err) { draftEvidenceComplete = false; ctx.log.warn( @@ -396,8 +396,16 @@ export class CeoInboxSentObserveHandler implements SkillHandler { const taskMatches = matchTasksToSent(msg, openTasks, alreadyAskedTaskIds); for (const tm of taskMatches) { + newCandidates[tm.taskId] = { + messageId: tm.messageId, + confidence: tm.confidence, + reason: tm.reason, + sentAt: tm.sentAt, + subject: tm.sentSubject, + recipients: tm.sentRecipients, + taskTitle: tm.taskTitle, + } satisfies CompletionCandidate; alreadyAskedTaskIds.add(tm.taskId); - completionChunks.push(formatCompletionCandidateBlock(tm)); taskCandidates += 1; } } @@ -546,24 +554,109 @@ export class CeoInboxSentObserveHandler implements SkillHandler { diffChunks.join(''), cutoffIso, ); - const completionsPersisted = await appendAndTrimDoc( - ctx, - PENDING_COMPLETIONS_PATH, - PENDING_COMPLETIONS_TYPE, - 'Pending task-completion candidates', - completionChunks.join(''), - cutoffIso, - ); - // If evidence didn't reach OKF, hold the watermark so those messages are re-observed - // next run (matching is idempotent via the already-matched/asked sets seeded from the - // docs, so the successful writes won't duplicate). + // Persist matched_draft_ids only when the diffs doc persisted. Include drafts whose diff + // actually landed this run (newlyDiffedDraftIds); EXCLUDE any whose body-fetch failed (never + // diffed) so they re-match next run — mirroring the old re-derive-from-pending-diffs behavior. + // Prune the carried-over set to drafts whose snapshot still exists (a snapshot TTL-sweeps + // after 7 idle days; once gone it can't be re-matched, so retaining its id is pointless). + // Tracks whether the matched-draft guard write itself landed. Unlike the old doc-derived + // guard (which re-derived matched ids from the pending-diffs doc body on every run), the + // guard is now standalone config with no fallback — extractMatchedDraftIds was deleted. + // A lost write here means the NEXT run's seed is stale (missing this run's newly-diffed + // drafts), so matchDraftToSent could re-match an already-diffed draft and append a + // duplicate diff block. Holding the watermark on a soft-reject/throw re-observes the same + // Sent message next run, which re-derives and re-writes the guard from scratch. + let matchedGuardPersisted = true; + if (diffsPersisted) { + const snapshotIds = new Set(snapshots.map((s) => s.draftId)); + const nextMatched = new Set([...seedMatchedDraftIds].filter((id) => snapshotIds.has(id))); + for (const id of newlyDiffedDraftIds) nextMatched.add(id); + try { + matchedGuardPersisted = await writeIdSet(store, MATCHED_DRAFT_IDS_KEY, nextMatched); + if (!matchedGuardPersisted) { + ctx.log.warn( + { path: MATCHED_DRAFT_IDS_KEY }, + 'ceo-inbox-sent-observe: matched-guard write soft-rejected — holding watermark for retry', + ); + } + } catch (err) { + matchedGuardPersisted = false; + ctx.log.warn( + { err }, + 'ceo-inbox-sent-observe: matched-guard write failed — holding watermark for retry', + ); + } + } + // Persist the candidate queue (config JSON) — replaces the pending-completions.md append. + // Merge onto a fresh read so we don't clobber a concurrent removal by task-completion; keyed + // by taskId so a held-watermark retry re-adds idempotently. completionsPersisted is driven by + // the accessor's own boolean return (not just the catch path) so a storeFact SOFT-reject + // (dedup 'conflict'/'auto_rejected', result.stored===false with no thrown error) also holds + // the watermark for retry — a caught-error-only gate would silently advance past a lost write. + let completionsPersisted = true; + if (Object.keys(newCandidates).length > 0) { + try { + const existing = await readCompletionCandidates(store, ctx.log); + completionsPersisted = await writeCompletionCandidates(store, { ...existing, ...newCandidates }); + if (!completionsPersisted) { + ctx.log.warn( + { path: COMPLETION_CANDIDATES_KEY }, + 'ceo-inbox-sent-observe: completion-candidate write soft-rejected (not persisted) — holding watermark for retry', + ); + } + } catch (err) { + completionsPersisted = false; + ctx.log.warn({ err }, 'ceo-inbox-sent-observe: completion-candidate write failed — holding watermark'); + } + } + + // Persist the asked-task guard AFTER the queue, and only when the queue persisted — writing + // the guard while the queue write failed would let next run skip re-matching and LOSE the + // candidate. Runs whenever completionsPersisted, independent of the watermark-advance + // decision below (a held watermark that still persisted the queue still wants a fresh guard). + // Prune to currently-open tasks (Joseph's retention choice): a completed/cancelled task drops + // out of the guard; re-surfacing a since-reopened task is harmless (original send is below the + // watermark; task-completion re-validates eligibility). + // Same standalone-config reasoning as matchedGuardPersisted above: the asked-guard has no + // OKF-derived fallback either, so a lost write here means next run's seed is stale (missing + // this run's newly-asked task ids), and matchTasksToSent could re-match a task that already + // has a queued candidate — re-asking about it. Holding the watermark re-observes the + // carrying Sent message next run, which re-derives and re-writes the guard from scratch. + let askedGuardPersisted = true; + if (completionsPersisted) { + const openIds = new Set(openTasks.map((t) => t.id)); + const prunedAsked = new Set([...alreadyAskedTaskIds].filter((id) => openIds.has(id))); + try { + askedGuardPersisted = await writeIdSet(store, ASKED_TASK_IDS_KEY, prunedAsked); + if (!askedGuardPersisted) { + ctx.log.warn( + { path: ASKED_TASK_IDS_KEY }, + 'ceo-inbox-sent-observe: asked-guard write soft-rejected — holding watermark for retry', + ); + } + } catch (err) { + askedGuardPersisted = false; + ctx.log.warn( + { err }, + 'ceo-inbox-sent-observe: asked-guard write failed — holding watermark for retry', + ); + } + } + + // If evidence didn't reach OKF/config, hold the watermark so those messages are re-observed + // next run (matching is idempotent via the already-matched/asked sets seeded above, so the + // successful writes won't duplicate). const evidencePersisted = diffsPersisted && completionsPersisted; // The watermark may only advance when the rolling evidence persisted AND every shadow batch - // reconciled AND every matched draft's full body was fetched. Any of these failing holds the - // watermark so the carrying Sent message is re-observed next run instead of being stranded - // past the `received_after` floor (an orphaned shadow doc is otherwise TTL-swept after 7 idle - // days; a truncated draft diff would otherwise persist unrepaired). - const advanceOk = evidencePersisted && shadowReconcileOk && draftEvidenceComplete; + // reconciled AND every matched draft's full body was fetched AND both standalone guard + // writes (matched-draft, asked-task) landed. Any of these failing holds the watermark so the + // carrying Sent message is re-observed next run instead of being stranded past the + // `received_after` floor (an orphaned shadow doc is otherwise TTL-swept after 7 idle days; a + // truncated draft diff would otherwise persist unrepaired; a lost guard write would otherwise + // let a draft/task double-surface on a later run since neither guard re-derives from OKF any + // more — extractMatchedDraftIds/extractAskedTaskIds were both deleted in #1438). + const advanceOk = + evidencePersisted && shadowReconcileOk && draftEvidenceComplete && matchedGuardPersisted && askedGuardPersisted; // Advance the watermark past the newest message seen (next poll starts after it). // @@ -590,13 +683,17 @@ export class CeoInboxSentObserveHandler implements SkillHandler { ? 'ceo-inbox-sent-observe: evidence persistence failed — holding watermark for retry' : !draftEvidenceComplete ? 'ceo-inbox-sent-observe: draft body fetch failed — holding watermark for retry' - : 'ceo-inbox-sent-observe: shadow reconcile failed — holding watermark for retry'; + : !shadowReconcileOk + ? 'ceo-inbox-sent-observe: shadow reconcile failed — holding watermark for retry' + : 'ceo-inbox-sent-observe: guard write failed — holding watermark for retry'; ctx.log.warn( { path: PENDING_DIFFS_PATH, evidencePersisted, shadowReconcileOk, draftEvidenceComplete, + matchedGuardPersisted, + askedGuardPersisted, }, holdReason, ); diff --git a/skills/ceo-inbox-sent-observe/skill.json b/skills/ceo-inbox-sent-observe/skill.json index f13364d8d..5cf3eac67 100644 --- a/skills/ceo-inbox-sent-observe/skill.json +++ b/skills/ceo-inbox-sent-observe/skill.json @@ -1,7 +1,7 @@ { "name": "ceo-inbox-sent-observe", "description": "Poll the CEO's Sent folder since a stored watermark, match new sends to Curia draft snapshots and open owner=ceo tasks, append evidence to OKF, and advance the watermark. Read-only against Nylas; never drafts, archives, or sends. Intended for the daily ceo-inbox sent-observe cron — do not fold into triage.", - "version": "0.1.0", + "version": "0.1.1", "sensitivity": "normal", "action_risk": "none", "inputs": { diff --git a/skills/list-learning-digest/handler.test.ts b/skills/list-learning-digest/handler.test.ts index 1116fc30a..097cd5dde 100644 --- a/skills/list-learning-digest/handler.test.ts +++ b/skills/list-learning-digest/handler.test.ts @@ -1,8 +1,53 @@ import { describe, it, expect, vi } from 'vitest'; import { ListLearningDigestHandler } from './handler.js'; import type { SkillContext } from '../../src/skills/types.js'; -import { PENDING_PROPOSALS_PATH } from '../voice-learn/handler.js'; -import { COMPLETION_DIGEST_PATH } from '../task-completion-from-sent/handler.js'; +import { + VOICE_PROPOSAL_KEY, + COMPLETION_DIGEST_KEY, + LEARNING_STATE_NAMESPACE, + type CompletionDigestMap, +} from '../_shared/learning-state.js'; +import type { EntityMemory } from '../../src/memory/entity-memory.js'; + +// Map-backed fake EntityMemory backing ConfigStore.get, same idiom used across the other +// learning-subsystem handler tests (voice-learn, resolve-learning-digest). +function makeMem(): EntityMemory & { __values: Map } { + const values = new Map(); + const anchor = { + id: 'a1', + label: `config:${LEARNING_STATE_NAMESPACE}`, + temporal: { + createdAt: new Date(), + lastConfirmedAt: new Date(), + confidence: 0.9, + decayClass: 'permanent', + source: 't', + }, + }; + return { + __values: values, + findEntities: vi.fn(async () => (values.size > 0 ? [anchor] : [])), + getFacts: vi.fn(async () => + [...values.entries()].map(([key, value]) => ({ + id: key, + label: key, + properties: { key, value, namespace: LEARNING_STATE_NAMESPACE }, + temporal: { + createdAt: new Date(), + lastConfirmedAt: new Date(), + confidence: 0.9, + decayClass: 'permanent', + source: 't', + }, + })), + ), + storeFact: vi.fn(async (p: { label: string; properties?: Record }) => { + values.set(p.label, String(p.properties?.value ?? '')); + return { stored: true, action: 'created' }; + }), + createEntity: vi.fn(async () => ({ entity: anchor, created: false })), + } as unknown as EntityMemory & { __values: Map }; +} describe('ListLearningDigestHandler', () => { it('returns empty message when no items', async () => { @@ -20,30 +65,66 @@ describe('ListLearningDigestHandler', () => { }); it('renders both sections when items exist', async () => { + const mem = makeMem(); + mem.__values.set( + VOICE_PROPOSAL_KEY, + JSON.stringify({ + status: 'pending', + generatedAt: '2026-07-16T00:00:00.000Z', + guide: '- Writes short.\n- Dry humour.', + }), + ); + const digestMap: CompletionDigestMap = { + t1: { kind: 'undo', taskId: 't1', taskTitle: 'Follow up', note: 'Marked done. Undo?' }, + }; + mem.__values.set(COMPLETION_DIGEST_KEY, JSON.stringify(digestMap)); const ctx = { - workingDocs: { - read: vi.fn(async (path: string) => { - if (path === PENDING_PROPOSALS_PATH) { - return { - body: `# Pending voice guide proposal\n\n## Guide Proposal\n- status: pending\n- generated_at: 2026-07-16T00:00:00.000Z\n\n- Writes short.\n- Dry humour.\n\n---\n`, - version: 1, - }; - } - if (path === COMPLETION_DIGEST_PATH) { - return { - body: `## Undo — task t1\n- status: undo_available\n- task_title: Follow up\n- note: Marked done. Undo?\n---\n`, - version: 1, - }; - } - return null; - }), - }, + entityMemory: mem, + workingDocs: { read: vi.fn().mockResolvedValue(null) }, log: { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() }, } as unknown as SkillContext; const result = await new ListLearningDigestHandler().execute(ctx); expect(result.success).toBe(true); - const data = (result as { data: { sections_markdown: string } }).data; + const data = (result as { data: { sections_markdown: string; voice_guide: string | null } }).data; + expect(data.voice_guide).toContain('Dry humour'); expect(data.sections_markdown).toContain('### Proposed writing-voice update'); expect(data.sections_markdown).toContain('### Task completion from sent mail'); + // UX unchanged: the CEO still replies with these exact commands to act on the item. + expect(data.sections_markdown).toContain('undo completion t1'); + }); + + it('does not render the voice section when entityMemory is unavailable', async () => { + const ctx = { + workingDocs: { read: vi.fn().mockResolvedValue(null) }, + log: { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() }, + } as unknown as SkillContext; + const result = await new ListLearningDigestHandler().execute(ctx); + expect(result.success).toBe(true); + const data = (result as { data: { sections_markdown: string; voice_guide: string | null } }).data; + // Without entityMemory neither the voice proposal nor the completion digest (both now + // config-store values, #1438) can be read, so both sections are omitted. + expect(data.voice_guide).toBeNull(); + expect(data.sections_markdown).not.toContain('### Proposed writing-voice update'); + expect(data.sections_markdown).not.toContain('### Task completion from sent mail'); + }); + + it('renders the completion section from config even with no pending voice proposal', async () => { + const mem = makeMem(); + const digestMap: CompletionDigestMap = { + t2: { kind: 'confirm', taskId: 't2', taskTitle: 'Plan AGM', note: 'Did emailing complete it?' }, + }; + mem.__values.set(COMPLETION_DIGEST_KEY, JSON.stringify(digestMap)); + const ctx = { + entityMemory: mem, + workingDocs: { read: vi.fn().mockResolvedValue(null) }, + log: { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() }, + } as unknown as SkillContext; + const result = await new ListLearningDigestHandler().execute(ctx); + expect(result.success).toBe(true); + const data = (result as { data: { sections_markdown: string; voice_guide: string | null } }).data; + expect(data.voice_guide).toBeNull(); + expect(data.sections_markdown).not.toContain('### Proposed writing-voice update'); + expect(data.sections_markdown).toContain('### Task completion from sent mail'); + expect(data.sections_markdown).toContain('confirm completion t2'); }); }); diff --git a/skills/list-learning-digest/handler.ts b/skills/list-learning-digest/handler.ts index ff1fea897..d06b464d6 100644 --- a/skills/list-learning-digest/handler.ts +++ b/skills/list-learning-digest/handler.ts @@ -1,12 +1,7 @@ import type { SkillHandler, SkillContext, SkillResult } from '../../src/skills/types.js'; -import { PENDING_PROPOSALS_PATH } from '../voice-learn/handler.js'; -import { COMPLETION_DIGEST_PATH } from '../task-completion-from-sent/handler.js'; -import { - parseCompletionDigest, - parseVoiceGuideProposal, - renderCompletionSection, - renderVoiceGuideSection, -} from '../_shared/learning-digest.js'; +import { ConfigStore } from '../../src/memory/config-store.js'; +import { readVoiceProposal, readCompletionDigest, digestMapToItems } from '../_shared/learning-state.js'; +import { renderCompletionSection, renderVoiceGuideSection } from '../_shared/learning-digest.js'; export class ListLearningDigestHandler implements SkillHandler { async execute(ctx: SkillContext): Promise { @@ -16,14 +11,16 @@ export class ListLearningDigestHandler implements SkillHandler { // Skill contract: never throw — a failed document read becomes a failure result. try { - const proposalsDoc = await ctx.workingDocs.read(PENDING_PROPOSALS_PATH); - const completionsDoc = await ctx.workingDocs.read(COMPLETION_DIGEST_PATH); - - const guide = parseVoiceGuideProposal(proposalsDoc?.body ?? ''); - const completion_items = parseCompletionDigest(completionsDoc?.body ?? ''); + // Both the voice proposal and the completion digest now live in config (#1438). + // Guarded on ctx.entityMemory since it's not a hard capability requirement of this + // skill — without it, neither section renders (same net effect as both being absent). + const store = ctx.entityMemory ? new ConfigStore(ctx.entityMemory, ctx.log) : null; + const guide = store ? await readVoiceProposal(store, ctx.log) : null; + const completion_items = store ? digestMapToItems(await readCompletionDigest(store, ctx.log)) : []; + const guideText = guide?.status === 'pending' ? guide.guide : null; const sections = [ - renderVoiceGuideSection(guide?.guide ?? null), + renderVoiceGuideSection(guideText), renderCompletionSection(completion_items), ] .filter(Boolean) @@ -32,11 +29,11 @@ export class ListLearningDigestHandler implements SkillHandler { return { success: true, data: { - voice_guide: guide?.guide ?? null, + voice_guide: guideText, completion_items, sections_markdown: sections, message: - !guide && completion_items.length === 0 + !guideText && completion_items.length === 0 ? 'No pending learning-digest items.' : undefined, }, diff --git a/skills/list-learning-digest/skill.json b/skills/list-learning-digest/skill.json index fef3ae36b..f3acdbd6d 100644 --- a/skills/list-learning-digest/skill.json +++ b/skills/list-learning-digest/skill.json @@ -1,7 +1,7 @@ { "name": "list-learning-digest", "description": "List the pending voice-guide proposal and sent-mail task-completion undo/confirm items for the daily CEO digest. Returns structured items plus pre-rendered markdown sections (omitted when empty).", - "version": "0.1.0", + "version": "0.2.0", "sensitivity": "normal", "action_risk": "none", "inputs": {}, @@ -13,6 +13,6 @@ "permissions": [], "secrets": [], "timeout": 15000, - "capabilities": ["workingDocs"], + "capabilities": ["workingDocs", "entityMemory"], "allowed_callers": ["coordinator"] } diff --git a/skills/resolve-learning-digest/handler.test.ts b/skills/resolve-learning-digest/handler.test.ts index ba2419d60..a053208f6 100644 --- a/skills/resolve-learning-digest/handler.test.ts +++ b/skills/resolve-learning-digest/handler.test.ts @@ -1,8 +1,12 @@ import { describe, it, expect, vi } from 'vitest'; import { ResolveLearningDigestHandler } from './handler.js'; import type { SkillContext } from '../../src/skills/types.js'; -import { PENDING_PROPOSALS_PATH, CONFIG_NAMESPACE, DISMISSED_KEY } from '../voice-learn/handler.js'; -import { COMPLETION_DIGEST_PATH } from '../task-completion-from-sent/handler.js'; +import { CONFIG_NAMESPACE, DISMISSED_KEY } from '../voice-learn/handler.js'; +import { + VOICE_PROPOSAL_KEY, + COMPLETION_DIGEST_KEY, + type CompletionDigestMap, +} from '../_shared/learning-state.js'; import type { EntityMemory } from '../../src/memory/entity-memory.js'; function makeMem(): EntityMemory & { __values: Map } { @@ -31,33 +35,22 @@ function makeMem(): EntityMemory & { __values: Map } { } as unknown as EntityMemory & { __values: Map }; } -const GUIDE_BODY = `# Pending voice guide proposal\n\n## Guide Proposal\n- status: pending\n- generated_at: 2026-07-16T00:00:00.000Z\n\n- Writes short.\n- Dry humour.\n\n---\n`; +const GUIDE_PROPOSAL = JSON.stringify({ + status: 'pending', + generatedAt: '2026-07-16T00:00:00.000Z', + guide: '- Writes short.\n- Dry humour.', +}); describe('ResolveLearningDigestHandler', () => { it('approves a voice guide proposal and writes it into WritingVoice.guide', async () => { - const docs = new Map([ - [ - PENDING_PROPOSALS_PATH, - { - path: PENDING_PROPOSALS_PATH, - body: GUIDE_BODY, - version: 1, - type: 'x', - frontmatter: {}, - }, - ], - ]); const mem = makeMem(); + mem.__values.set(VOICE_PROPOSAL_KEY, GUIDE_PROPOSAL); const update = vi.fn(); const ctx = { input: { action: 'approve_voice' }, workingDocs: { - read: vi.fn(async (p: string) => docs.get(p) ?? null), - update: vi.fn(async (p: string, params: { body: string }) => { - const cur = docs.get(p)!; - docs.set(p, { ...cur, body: params.body, version: cur.version + 1 }); - return { ok: true, document: docs.get(p) }; - }), + read: vi.fn(), + update: vi.fn(), }, entityMemory: mem, executiveProfileService: { @@ -86,34 +79,65 @@ describe('ResolveLearningDigestHandler', () => { 'skill', expect.any(String), ); - // The resolved proposal is removed from the queue doc (the approved guide now lives in - // the versioned profile), so the doc doesn't accumulate actioned proposals. - expect(docs.get(PENDING_PROPOSALS_PATH)!.body).not.toContain('## Guide Proposal'); + // The resolved proposal is cleared from config (the approved guide now lives in the + // versioned profile), so it doesn't stay "pending" and get re-surfaced in the digest. + expect(mem.__values.get(VOICE_PROPOSAL_KEY)).toBe('null'); + }); + + it('reports failure (not success) when the proposal clear soft-rejects after a successful approve', async () => { + // The profile write (the primary side effect) succeeds, but the clear of the pending + // proposal soft-rejects (stored:false, no throw — a dedup 'conflict'/'auto_rejected' outcome). + // Before the fix, the boolean return of writeVoiceProposal was ignored entirely, so this + // reported success even though the proposal item would still be there next digest list. + const mem = makeMem(); + mem.__values.set(VOICE_PROPOSAL_KEY, GUIDE_PROPOSAL); + const update = vi.fn(); + const ctx = { + input: { action: 'approve_voice' }, + workingDocs: { read: vi.fn(), update: vi.fn() }, + entityMemory: mem, + executiveProfileService: { + get: () => ({ + writingVoice: { + tone: [], + formality: 50, + patterns: [], + vocabulary: { prefer: [], avoid: [] }, + signOff: '', + guide: '', + }, + }), + update, + }, + taskRepo: { reopenTask: vi.fn(), completeTask: vi.fn(), getTask: vi.fn() }, + log: { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() }, + } as unknown as SkillContext; + + // Soft-reject only the proposal-clear write; the profile update above goes through + // executiveProfileService, independent of storeFact. + (mem.storeFact as ReturnType).mockImplementation( + async (p: { label: string; properties?: Record }) => { + if (p.label === VOICE_PROPOSAL_KEY) return { stored: false, action: 'conflict' as const }; + mem.__values.set(p.label, String(p.properties?.value ?? '')); + return { stored: true, action: 'created' as const }; + }, + ); + + const result = await new ResolveLearningDigestHandler().execute(ctx); + expect(result.success).toBe(false); + expect((result as { error: string }).error).toMatch(/may reappear/); + // The primary side effect (the profile write) still happened. + expect(update).toHaveBeenCalled(); }); it('dismisses a voice guide proposal and writes dismissal guard', async () => { - const docs = new Map([ - [ - PENDING_PROPOSALS_PATH, - { - path: PENDING_PROPOSALS_PATH, - body: GUIDE_BODY, - version: 1, - type: 'x', - frontmatter: {}, - }, - ], - ]); const mem = makeMem(); + mem.__values.set(VOICE_PROPOSAL_KEY, GUIDE_PROPOSAL); const ctx = { input: { action: 'dismiss_voice' }, workingDocs: { - read: vi.fn(async (p: string) => docs.get(p) ?? null), - update: vi.fn(async (p: string, params: { body: string }) => { - const cur = docs.get(p)!; - docs.set(p, { ...cur, body: params.body, version: cur.version + 1 }); - return { ok: true, document: docs.get(p) }; - }), + read: vi.fn(), + update: vi.fn(), }, entityMemory: mem, executiveProfileService: { @@ -126,37 +150,25 @@ describe('ResolveLearningDigestHandler', () => { const result = await new ResolveLearningDigestHandler().execute(ctx); expect(result.success).toBe(true); - // The dismissed proposal is removed from the queue doc; the dismiss cooldown is what - // suppresses re-proposal, and it lives in config (asserted below). - expect(docs.get(PENDING_PROPOSALS_PATH)!.body).not.toContain('## Guide Proposal'); + // The dismissed proposal is cleared from config; the dismiss cooldown is what suppresses + // re-proposal, and it lives in config too (asserted below). + expect(mem.__values.get(VOICE_PROPOSAL_KEY)).toBe('null'); expect(mem.__values.get(DISMISSED_KEY)).toContain('guide'); }); it('undoes an auto-complete via reopenTask', async () => { - const docs = new Map([ - [ - COMPLETION_DIGEST_PATH, - { - path: COMPLETION_DIGEST_PATH, - body: `## Undo — task t1\n- status: undo_available\n- task_title: Follow up\n- note: Undo?\n---\n`, - version: 1, - type: 'x', - frontmatter: {}, - }, - ], - ]); + const mem = makeMem(); + const digestMap: CompletionDigestMap = { + t1: { kind: 'undo', taskId: 't1', taskTitle: 'Follow up', note: 'Undo?' }, + }; + mem.__values.set(COMPLETION_DIGEST_KEY, JSON.stringify(digestMap)); const reopenTask = vi.fn(async () => ({ id: 't1', status: 'open' })); const ctx = { input: { action: 'undo_completion', task_id: 't1' }, - workingDocs: { - read: vi.fn(async (p: string) => docs.get(p) ?? null), - update: vi.fn(async (p: string, params: { body: string }) => { - const cur = docs.get(p)!; - docs.set(p, { ...cur, body: params.body, version: cur.version + 1 }); - return { ok: true, document: docs.get(p) }; - }), - }, - entityMemory: makeMem(), + // Not read/written for completion actions any more (config-store only, #1438) — a + // minimal stub satisfies the handler's top-level capability guard. + workingDocs: { read: vi.fn(), update: vi.fn() }, + entityMemory: mem, executiveProfileService: { get: vi.fn(), update: vi.fn() }, taskRepo: { reopenTask, completeTask: vi.fn(), getTask: vi.fn() }, agentId: 'coordinator', @@ -166,29 +178,59 @@ describe('ResolveLearningDigestHandler', () => { const result = await new ResolveLearningDigestHandler().execute(ctx); expect(result.success).toBe(true); expect(reopenTask).toHaveBeenCalledWith('t1', expect.any(String), 'coordinator'); - // The actioned item is removed from the queue doc so resolved items don't accumulate. - expect(docs.get(COMPLETION_DIGEST_PATH)!.body).not.toContain('## Undo — task t1'); + // The actioned item is removed from the config map so resolved items don't accumulate. + const updated = JSON.parse(mem.__values.get(COMPLETION_DIGEST_KEY)!) as CompletionDigestMap; + expect(updated.t1).toBeUndefined(); + }); + + it('reports failure when the digest clear soft-rejects, but the task is still reopened', async () => { + // reopenTask (the primary side effect) succeeds, but the digest-clear write soft-rejects + // (stored:false, no throw). The task must still be reopened — we don't add compensation — + // but the result must be honest that the digest item may reappear, not a blanket success. + const mem = makeMem(); + const digestMap: CompletionDigestMap = { + t1: { kind: 'undo', taskId: 't1', taskTitle: 'Follow up', note: 'Undo?' }, + }; + mem.__values.set(COMPLETION_DIGEST_KEY, JSON.stringify(digestMap)); + const reopenTask = vi.fn(async () => ({ id: 't1', status: 'open' })); + const ctx = { + input: { action: 'undo_completion', task_id: 't1' }, + workingDocs: { read: vi.fn(), update: vi.fn() }, + entityMemory: mem, + executiveProfileService: { get: vi.fn(), update: vi.fn() }, + taskRepo: { reopenTask, completeTask: vi.fn(), getTask: vi.fn() }, + agentId: 'coordinator', + log: { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() }, + } as unknown as SkillContext; + + (mem.storeFact as ReturnType).mockImplementation( + async (p: { label: string; properties?: Record }) => { + if (p.label === COMPLETION_DIGEST_KEY) return { stored: false, action: 'conflict' as const }; + mem.__values.set(p.label, String(p.properties?.value ?? '')); + return { stored: true, action: 'created' as const }; + }, + ); + + const result = await new ResolveLearningDigestHandler().execute(ctx); + expect(result.success).toBe(false); + expect((result as { error: string }).error).toMatch(/may reappear/); + // The task was still reopened — the fix is honest reporting, not compensation. + expect(reopenTask).toHaveBeenCalledWith('t1', expect.any(String), 'coordinator'); }); it('fails and keeps the undo item when the task no longer exists (reopenTask returns null)', async () => { - const digestBody = `## Undo — task t1\n- status: undo_available\n- task_title: Follow up\n- note: Undo?\n---\n`; - const docs = new Map([ - [ - COMPLETION_DIGEST_PATH, - { path: COMPLETION_DIGEST_PATH, body: digestBody, version: 1, type: 'x', frontmatter: {} }, - ], - ]); + const mem = makeMem(); + const digestMap: CompletionDigestMap = { + t1: { kind: 'undo', taskId: 't1', taskTitle: 'Follow up', note: 'Undo?' }, + }; + mem.__values.set(COMPLETION_DIGEST_KEY, JSON.stringify(digestMap)); // reopenTask returns null when the task is gone — the digest item must survive so the user // still sees the undo affordance, and the result must report failure (not a silent success). const reopenTask = vi.fn(async () => null); - const update = vi.fn(); const ctx = { input: { action: 'undo_completion', task_id: 't1' }, - workingDocs: { - read: vi.fn(async (p: string) => docs.get(p) ?? null), - update, - }, - entityMemory: makeMem(), + workingDocs: { read: vi.fn(), update: vi.fn() }, + entityMemory: mem, executiveProfileService: { get: vi.fn(), update: vi.fn() }, taskRepo: { reopenTask, completeTask: vi.fn(), getTask: vi.fn() }, agentId: 'coordinator', @@ -197,8 +239,90 @@ describe('ResolveLearningDigestHandler', () => { const result = await new ResolveLearningDigestHandler().execute(ctx); expect(result.success).toBe(false); - // The digest was never rewritten — the item is preserved for a retry. - expect(update).not.toHaveBeenCalled(); - expect(docs.get(COMPLETION_DIGEST_PATH)!.body).toContain('## Undo — task t1'); + // writeCompletionDigest was never called — the item is preserved for a retry. + expect(mem.storeFact).not.toHaveBeenCalled(); + const updated = JSON.parse(mem.__values.get(COMPLETION_DIGEST_KEY)!) as CompletionDigestMap; + expect(updated.t1).toEqual(digestMap.t1); + }); + + it('confirms completion via completeTask and removes the digest item', async () => { + const mem = makeMem(); + const digestMap: CompletionDigestMap = { + t1: { kind: 'confirm', taskId: 't1', taskTitle: 'Follow up', note: 'Did emailing them complete it?' }, + }; + mem.__values.set(COMPLETION_DIGEST_KEY, JSON.stringify(digestMap)); + const getTask = vi.fn(async () => ({ id: 't1', status: 'open' })); + const completeTask = vi.fn(async () => undefined); + const ctx = { + input: { action: 'confirm_completion', task_id: 't1' }, + workingDocs: { read: vi.fn(), update: vi.fn() }, + entityMemory: mem, + executiveProfileService: { get: vi.fn(), update: vi.fn() }, + taskRepo: { reopenTask: vi.fn(), completeTask, getTask }, + agentId: 'coordinator', + log: { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() }, + } as unknown as SkillContext; + + const result = await new ResolveLearningDigestHandler().execute(ctx); + expect(result.success).toBe(true); + expect(completeTask).toHaveBeenCalledWith('t1', expect.any(String), 'coordinator'); + // The actioned item is removed from the config map so resolved items don't accumulate. + const updated = JSON.parse(mem.__values.get(COMPLETION_DIGEST_KEY)!) as CompletionDigestMap; + expect(updated.t1).toBeUndefined(); + }); + + it('fails and keeps the confirm item when the task no longer exists (getTask returns null)', async () => { + const mem = makeMem(); + const digestMap: CompletionDigestMap = { + t1: { kind: 'confirm', taskId: 't1', taskTitle: 'Follow up', note: 'Did emailing them complete it?' }, + }; + mem.__values.set(COMPLETION_DIGEST_KEY, JSON.stringify(digestMap)); + const getTask = vi.fn(async () => null); + const completeTask = vi.fn(); + const ctx = { + input: { action: 'confirm_completion', task_id: 't1' }, + workingDocs: { read: vi.fn(), update: vi.fn() }, + entityMemory: mem, + executiveProfileService: { get: vi.fn(), update: vi.fn() }, + taskRepo: { reopenTask: vi.fn(), completeTask, getTask }, + agentId: 'coordinator', + log: { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() }, + } as unknown as SkillContext; + + const result = await new ResolveLearningDigestHandler().execute(ctx); + expect(result.success).toBe(false); + expect(completeTask).not.toHaveBeenCalled(); + // writeCompletionDigest was never called — the item is preserved for a retry. + const updated = JSON.parse(mem.__values.get(COMPLETION_DIGEST_KEY)!) as CompletionDigestMap; + expect(updated.t1).toEqual(digestMap.t1); + }); + + it('dismisses a queued confirm item without touching the task', async () => { + const mem = makeMem(); + const digestMap: CompletionDigestMap = { + t1: { kind: 'confirm', taskId: 't1', taskTitle: 'Follow up', note: 'Did emailing them complete it?' }, + }; + mem.__values.set(COMPLETION_DIGEST_KEY, JSON.stringify(digestMap)); + const getTask = vi.fn(); + const completeTask = vi.fn(); + const reopenTask = vi.fn(); + const ctx = { + input: { action: 'dismiss_completion', task_id: 't1' }, + workingDocs: { read: vi.fn(), update: vi.fn() }, + entityMemory: mem, + executiveProfileService: { get: vi.fn(), update: vi.fn() }, + taskRepo: { reopenTask, completeTask, getTask }, + agentId: 'coordinator', + log: { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() }, + } as unknown as SkillContext; + + const result = await new ResolveLearningDigestHandler().execute(ctx); + expect(result.success).toBe(true); + expect(getTask).not.toHaveBeenCalled(); + expect(completeTask).not.toHaveBeenCalled(); + expect(reopenTask).not.toHaveBeenCalled(); + // The actioned item is removed from the config map so resolved items don't accumulate. + const updated = JSON.parse(mem.__values.get(COMPLETION_DIGEST_KEY)!) as CompletionDigestMap; + expect(updated.t1).toBeUndefined(); }); }); diff --git a/skills/resolve-learning-digest/handler.ts b/skills/resolve-learning-digest/handler.ts index 916a4f236..8e473e888 100644 --- a/skills/resolve-learning-digest/handler.ts +++ b/skills/resolve-learning-digest/handler.ts @@ -1,17 +1,12 @@ import type { SkillHandler, SkillContext, SkillResult } from '../../src/skills/types.js'; import { ConfigStore } from '../../src/memory/config-store.js'; +import { CONFIG_NAMESPACE as VOICE_NS, DISMISSED_KEY } from '../voice-learn/handler.js'; import { - CONFIG_NAMESPACE as VOICE_NS, - DISMISSED_KEY, - PENDING_PROPOSALS_PATH, -} from '../voice-learn/handler.js'; -import { COMPLETION_DIGEST_PATH } from '../task-completion-from-sent/handler.js'; -import { - pruneGuideProposals, - removeCompletionBlock, - parseCompletionDigest, - parseVoiceGuideProposal, -} from '../_shared/learning-digest.js'; + readVoiceProposal, + writeVoiceProposal, + readCompletionDigest, + writeCompletionDigest, +} from '../_shared/learning-state.js'; const ACTIONS = new Set([ 'approve_voice', @@ -55,9 +50,13 @@ export class ResolveLearningDigestHandler implements SkillHandler { } if (action === 'approve_voice' || action === 'dismiss_voice') { - const doc = await ctx.workingDocs.read(PENDING_PROPOSALS_PATH); - const proposal = doc ? parseVoiceGuideProposal(doc.body) : null; - if (!doc || !proposal) return { success: false, error: 'No pending voice guide proposal' }; + // Built once — reused for the proposal read/clear below and (on dismiss) the cooldown + // write, all against the same 'ceo_inbox' namespace. + const store = new ConfigStore(ctx.entityMemory, ctx.log); + const proposal = await readVoiceProposal(store, ctx.log); + if (!proposal || proposal.status !== 'pending') { + return { success: false, error: 'No pending voice guide proposal' }; + } if (action === 'approve_voice') { const current = ctx.executiveProfileService.get().writingVoice; @@ -67,19 +66,28 @@ export class ResolveLearningDigestHandler implements SkillHandler { 'voice guide approved', ); - // Remove the resolved proposal from the queue doc so it doesn't accumulate — the - // approved guide now lives in the versioned profile. - const updatedBody = pruneGuideProposals(doc.body, { removePending: true }); - await ctx.workingDocs.update(PENDING_PROPOSALS_PATH, { - body: updatedBody, - expectedVersion: doc.version, - }); + // Clear the resolved proposal so it doesn't stay "pending" forever — the approved + // guide now lives in the versioned profile, which is the real audit trail. + // + // The primary side effect (the profile write above) already succeeded by this point. + // If the clear itself soft-rejects (stored:false — a dedup 'conflict'/'auto_rejected' + // outcome, not a throw), we must NOT report success: the proposal item would still be + // there next time the digest is listed, contradicting a "resolved: true" response. We + // surface that honestly instead. A retry is safe because the mutations here are + // idempotent — re-approving writes the identical guide text again, and re-clearing a + // proposal that's already gone is a no-op. + const cleared = await writeVoiceProposal(store, null); + if (!cleared) { + return { + success: false, + error: 'Voice guide applied to the profile, but the pending proposal could not be cleared (transient) and may reappear — retry to clear it.', + }; + } return { success: true, data: { resolved: true, detail: 'Approved voice guide' } }; } // dismiss: keep the existing DISMISSED_KEY cooldown write. There's only one guide // dimension now (no per-field proposals), so the cooldown entry uses a fixed key. - const store = new ConfigStore(ctx.entityMemory, ctx.log); const rawDismissed = await store.get(VOICE_NS, DISMISSED_KEY); let dismissed: Array<{ dimension: string; until: string }> = []; if (rawDismissed) { @@ -94,13 +102,16 @@ export class ResolveLearningDigestHandler implements SkillHandler { dismissed.push({ dimension: 'guide', until }); await store.set(VOICE_NS, DISMISSED_KEY, JSON.stringify(dismissed)); - // Remove the resolved proposal from the queue doc — the dismiss cooldown is tracked - // in config, so the block itself serves no further purpose. - const updatedBody = pruneGuideProposals(doc.body, { removePending: true }); - await ctx.workingDocs.update(PENDING_PROPOSALS_PATH, { - body: updatedBody, - expectedVersion: doc.version, - }); + // Clear the resolved proposal — the dismiss cooldown is tracked in config, so the + // proposal record itself serves no further purpose. The cooldown write above already + // landed, so a soft-reject here only means the proposal item may reappear in the digest. + const cleared = await writeVoiceProposal(store, null); + if (!cleared) { + return { + success: false, + error: 'Voice guide dismissed (cooldown recorded), but the pending proposal could not be cleared (transient) and may reappear — retry to clear it.', + }; + } return { success: true, data: { resolved: true, detail: 'Dismissed voice guide' } }; } @@ -108,17 +119,15 @@ export class ResolveLearningDigestHandler implements SkillHandler { const taskId = typeof input.task_id === 'string' ? input.task_id.trim() : ''; if (!taskId) return { success: false, error: 'task_id is required for completion actions' }; - const digest = await ctx.workingDocs.read(COMPLETION_DIGEST_PATH); - if (!digest) return { success: false, error: 'No completion digest items' }; + const store = new ConfigStore(ctx.entityMemory, ctx.log); + const digestMap = await readCompletionDigest(store, ctx.log); + const item = digestMap[taskId]; // Require an actionable digest item of the matching kind before mutating a task — // otherwise undo/confirm would reopen/complete ANY task id the caller supplies, and // still report success even when the digest held no such item. const expectedKind = action === 'undo_completion' ? 'undo' : 'confirm'; - const item = parseCompletionDigest(digest.body).find( - (candidate) => candidate.taskId === taskId && candidate.kind === expectedKind, - ); - if (!item) { + if (!item || item.kind !== expectedKind) { return { success: false, error: `No actionable ${expectedKind} item for task ${taskId}` }; } @@ -134,12 +143,18 @@ export class ResolveLearningDigestHandler implements SkillHandler { if (!reopened) { return { success: false, error: `Task ${taskId} not found; cannot undo completion` }; } - // Remove the actioned item from the queue doc so resolved items don't accumulate. - const updatedBody = removeCompletionBlock(digest.body, 'Undo', taskId); - await ctx.workingDocs.update(COMPLETION_DIGEST_PATH, { - body: updatedBody, - expectedVersion: digest.version, - }); + // Remove the actioned item from the config map so resolved items don't accumulate. The + // task is already reopened by this point, so a soft-reject on the clear only means the + // digest item may reappear — reopening an already-open task next retry is a no-op. + const { [taskId]: _removed, ...rest } = digestMap; + void _removed; + const cleared = await writeCompletionDigest(store, rest); + if (!cleared) { + return { + success: false, + error: `Task ${taskId} was reopened, but the digest item could not be cleared (transient) and may reappear — retry to clear it.`, + }; + } return { success: true, data: { resolved: true, detail: `Reopened task ${taskId}` } }; } @@ -157,20 +172,31 @@ export class ResolveLearningDigestHandler implements SkillHandler { ctx.agentId, ); } - const updatedBody = removeCompletionBlock(digest.body, 'Confirm', taskId); - await ctx.workingDocs.update(COMPLETION_DIGEST_PATH, { - body: updatedBody, - expectedVersion: digest.version, - }); + // Task completion (if needed) already happened above; a soft-reject on the clear only + // means the digest item may reappear — completing an already-done task next retry is a + // no-op (the `task.status !== 'done'` guard above skips it). + const { [taskId]: _removed, ...rest } = digestMap; + void _removed; + const cleared = await writeCompletionDigest(store, rest); + if (!cleared) { + return { + success: false, + error: `Task ${taskId} was confirmed complete, but the digest item could not be cleared (transient) and may reappear — retry to clear it.`, + }; + } return { success: true, data: { resolved: true, detail: `Confirmed completion ${taskId}` } }; } // dismiss_completion - const updatedBody = removeCompletionBlock(digest.body, 'Confirm', taskId); - await ctx.workingDocs.update(COMPLETION_DIGEST_PATH, { - body: updatedBody, - expectedVersion: digest.version, - }); + const { [taskId]: _removed, ...rest } = digestMap; + void _removed; + const cleared = await writeCompletionDigest(store, rest); + if (!cleared) { + return { + success: false, + error: `Digest item for task ${taskId} could not be cleared (transient); retry.`, + }; + } return { success: true, data: { resolved: true, detail: `Dismissed completion ${taskId}` } }; } } diff --git a/skills/resolve-learning-digest/skill.json b/skills/resolve-learning-digest/skill.json index 0ee424595..60e12e033 100644 --- a/skills/resolve-learning-digest/skill.json +++ b/skills/resolve-learning-digest/skill.json @@ -1,7 +1,7 @@ { "name": "resolve-learning-digest", "description": "Resolve a learning-digest item: approve/dismiss a voice proposal, undo an auto-completed task, or confirm/dismiss a task-completion candidate.", - "version": "0.1.0", + "version": "0.1.1", "sensitivity": "normal", "action_risk": "medium", "inputs": { diff --git a/skills/task-completion-from-sent/handler.test.ts b/skills/task-completion-from-sent/handler.test.ts index 80947ffd5..af5fad8cb 100644 --- a/skills/task-completion-from-sent/handler.test.ts +++ b/skills/task-completion-from-sent/handler.test.ts @@ -1,60 +1,93 @@ import { describe, it, expect, vi } from 'vitest'; -import { TaskCompletionFromSentHandler, COMPLETION_DIGEST_PATH } from './handler.js'; +import { TaskCompletionFromSentHandler } from './handler.js'; import type { SkillContext } from '../../src/skills/types.js'; -import { PENDING_COMPLETIONS_PATH } from '../ceo-inbox-sent-observe/handler.js'; - -const PENDING = ` -# Pending task-completion candidates - -## Candidate — task 11111111-1111-4111-8111-111111111111 -- message_id: m-hi -- confidence: high -- reason: recipient+semantic -- sent_at: 2026-07-01T12:00:00.000Z -- subject: Follow up -- recipients: john@example.com -- task_title: Follow up with John -- status: pending ---- -## Candidate — task 22222222-2222-4222-8222-222222222222 -- message_id: m-agm -- confidence: high -- reason: recipient+semantic -- sent_at: 2026-07-02T12:00:00.000Z -- subject: AGM -- recipients: board@example.com -- task_title: Plan AGM -- status: pending ---- -## Candidate — task 33333333-3333-4333-8333-333333333333 -- message_id: m-fuzzy -- confidence: low -- reason: semantic -- sent_at: 2026-07-03T12:00:00.000Z -- subject: Stuff -- recipients: x@example.com -- task_title: Maybe related -- status: pending ---- -`; +import type { EntityMemory } from '../../src/memory/entity-memory.js'; +import { CONFIG_NAMESPACE } from '../ceo-inbox-sent-observe/handler.js'; +import { + COMPLETION_CANDIDATES_KEY, + COMPLETION_DIGEST_KEY, + type CompletionCandidateMap, +} from '../_shared/learning-state.js'; + +const CANDIDATE_MAP: CompletionCandidateMap = { + '11111111-1111-4111-8111-111111111111': { + messageId: 'm-hi', + confidence: 'high', + reason: 'recipient+semantic', + sentAt: '2026-07-01T12:00:00.000Z', + subject: 'Follow up', + recipients: ['john@example.com'], + taskTitle: 'Follow up with John', + }, + '22222222-2222-4222-8222-222222222222': { + messageId: 'm-agm', + confidence: 'high', + reason: 'recipient+semantic', + sentAt: '2026-07-02T12:00:00.000Z', + subject: 'AGM', + recipients: ['board@example.com'], + taskTitle: 'Plan AGM', + }, + '33333333-3333-4333-8333-333333333333': { + messageId: 'm-fuzzy', + confidence: 'low', + reason: 'semantic', + sentAt: '2026-07-03T12:00:00.000Z', + subject: 'Stuff', + recipients: ['x@example.com'], + taskTitle: 'Maybe related', + }, +}; + +/** ConfigStore-backing entityMemory double. Same shape as the `makeMem` helper in + * resolve-learning-digest/handler.test.ts and voice-learn/handler.test.ts — + * `storeFact`/`getFacts` round-trip through a plain Map keyed by label. */ +function makeMem(seed: Record = {}): EntityMemory & { __values: Map } { + const values = new Map(Object.entries(seed)); + const anchor = { + id: 'a1', + label: `config:${CONFIG_NAMESPACE}`, + temporal: { + createdAt: new Date(), + lastConfirmedAt: new Date(), + confidence: 0.9, + decayClass: 'permanent', + source: 't', + }, + }; + return { + __values: values, + findEntities: vi.fn(async () => [anchor]), + getFacts: vi.fn(async () => + [...values.entries()].map(([key, value]) => ({ + id: key, + label: key, + properties: { key, value, namespace: CONFIG_NAMESPACE }, + temporal: { + createdAt: new Date(), + lastConfirmedAt: new Date(), + confidence: 0.9, + decayClass: 'permanent', + source: 't', + }, + })), + ), + storeFact: vi.fn(async (p: { label: string; properties?: Record }) => { + values.set(p.label, String(p.properties?.value ?? '')); + return { stored: true, action: 'created' }; + }), + createEntity: vi.fn(async () => ({ entity: anchor, created: false })), + } as unknown as EntityMemory & { __values: Map }; +} function makeCtx(): SkillContext & { __completed: string[]; + __mem: ReturnType; __docs: Map }>; } { const completed: string[] = []; - const docs = new Map([ - [ - PENDING_COMPLETIONS_PATH, - { - path: PENDING_COMPLETIONS_PATH, - type: 'voice-pending-completions', - frontmatter: {}, - body: PENDING, - version: 1, - }, - ], - ]); + const mem = makeMem({ [COMPLETION_CANDIDATES_KEY]: JSON.stringify(CANDIDATE_MAP) }); + const docs = new Map }>(); const tasks: Record tasks[id] ?? null), listTasks: vi.fn(async () => []), @@ -140,9 +174,11 @@ function makeCtx(): SkillContext & { classify: (text: string) => (/board|agm/i.test(text) ? 'restricted' : 'internal'), }, __completed: completed, + __mem: mem, __docs: docs, } as unknown as SkillContext & { __completed: string[]; + __mem: typeof mem; __docs: typeof docs; }; } @@ -160,42 +196,27 @@ describe('TaskCompletionFromSentHandler', () => { expect(ctx.__completed).toEqual(['11111111-1111-4111-8111-111111111111']); expect(ctx.__completed).not.toContain('22222222-2222-4222-8222-222222222222'); - const digest = ctx.__docs.get(COMPLETION_DIGEST_PATH)?.body ?? ''; - expect(digest).toContain('Undo — task 11111111-1111-4111-8111-111111111111'); - expect(digest).toContain('Confirm — task 22222222-2222-4222-8222-222222222222'); - expect(digest).toContain('Confirm — task 33333333-3333-4333-8333-333333333333'); + const digest = JSON.parse(ctx.__mem.__values.get(COMPLETION_DIGEST_KEY)!); + expect(digest['11111111-1111-4111-8111-111111111111'].kind).toBe('undo'); + expect(digest['22222222-2222-4222-8222-222222222222'].kind).toBe('confirm'); + expect(digest['33333333-3333-4333-8333-333333333333'].kind).toBe('confirm'); - // Both confirmed candidates must receive their own completion_asked guard — a - // document-wide check previously marked only the first (#1429 CodeRabbit). - const pending = ctx.__docs.get(PENDING_COMPLETIONS_PATH)?.body ?? ''; - const markerCount = (pending.match(/completion_asked:/g) ?? []).length; - expect(markerCount).toBe(2); + // All three candidates (auto-completed + both confirm-queued) are consumed from the + // config queue — the persistent asked_task_ids guard (written by sent-observe) is what + // prevents re-surfacing now, not an in-band marker. + const remaining = JSON.parse(ctx.__mem.__values.get(COMPLETION_CANDIDATES_KEY)!); + expect(remaining).toEqual({}); }); it('a low-confidence candidate skips the subtask lookup and the sensitivity classifier (T3.1)', async () => { const ctx = makeCtx(); // Queue only the low-confidence candidate. - const onlyLow = ` -# Pending task-completion candidates - -## Candidate — task 33333333-3333-4333-8333-333333333333 -- message_id: m-fuzzy -- confidence: low -- reason: semantic -- sent_at: 2026-07-03T12:00:00.000Z -- subject: Stuff -- recipients: x@example.com -- task_title: Maybe related -- status: pending ---- -`; - ctx.__docs.set(PENDING_COMPLETIONS_PATH, { - path: PENDING_COMPLETIONS_PATH, - type: 'voice-pending-completions', - frontmatter: {}, - body: onlyLow, - version: 1, - }); + ctx.__mem.__values.set( + COMPLETION_CANDIDATES_KEY, + JSON.stringify({ + '33333333-3333-4333-8333-333333333333': CANDIDATE_MAP['33333333-3333-4333-8333-333333333333'], + }), + ); const classifyFn = vi.fn((text: string) => (/board|agm/i.test(text) ? 'restricted' : 'internal')); (ctx as unknown as { sensitivityClassifier: { classify: typeof classifyFn } }).sensitivityClassifier = { classify: classifyFn, @@ -230,8 +251,10 @@ describe('TaskCompletionFromSentHandler', () => { expect(result.success).toBe(true); // The reassigned task must NOT be auto-completed. expect(ctx.__completed).not.toContain('11111111-1111-4111-8111-111111111111'); - const pending = ctx.__docs.get(PENDING_COMPLETIONS_PATH)?.body ?? ''; - expect(pending).toContain('skipped_ineligible'); + // The ineligible candidate is removed from the config queue (consume-by-delete applies to + // skipped-ineligible too — the asked_task_ids guard, not an in-band marker, prevents re-ask). + const remaining = JSON.parse(ctx.__mem.__values.get(COMPLETION_CANDIDATES_KEY)!); + expect(remaining['11111111-1111-4111-8111-111111111111']).toBeUndefined(); }); it('reopenTask undoes an auto-complete (reversible path)', async () => { @@ -239,4 +262,103 @@ describe('TaskCompletionFromSentHandler', () => { // Lightweight behavioural check that reopenTask exists and rejects non-done. expect(typeof TaskRepo.prototype.reopenTask).toBe('function'); }); + + it('CRITICAL: a completeTask failure during auto-complete leaves the candidate queued for retry', async () => { + const ctx = makeCtx(); + // Queue only the high-confidence, low-risk candidate that would otherwise auto-complete + // (same task as the first test's auto-complete case, isolated here so the retry-survival + // assertion below is unambiguous). + ctx.__mem.__values.set( + COMPLETION_CANDIDATES_KEY, + JSON.stringify({ + '11111111-1111-4111-8111-111111111111': CANDIDATE_MAP['11111111-1111-4111-8111-111111111111'], + }), + ); + // Simulate a transient DB failure (e.g. connection hiccup) during completeTask — the + // handler's catch path must treat this as retryable, not as "handled". + (ctx.taskRepo as unknown as { completeTask: (...args: unknown[]) => Promise }).completeTask = + vi.fn(async () => { + throw new Error('db hiccup'); + }); + + const result = await handler.execute(ctx); + expect(result.success).toBe(true); + const data = (result as { data: { auto_completed: number; queued_confirm: number; skipped: number } }).data; + expect(data.auto_completed).toBe(0); + expect(data.skipped).toBe(1); + + // The candidate must survive the failure — it stays in the config queue for retry next + // run rather than being consumed by the `delete remaining[taskId]` that only runs on the + // success path. If that delete were hoisted above the try (or into a `finally`), this + // candidate would vanish from `remaining`, the queue length would shrink, the handler + // would call writeCompletionCandidates with the now-empty map, and this assertion would + // fail (the key would be undefined instead of round-tripping the seeded candidate). + const remaining = JSON.parse(ctx.__mem.__values.get(COMPLETION_CANDIDATES_KEY)!) as CompletionCandidateMap; + expect(remaining['11111111-1111-4111-8111-111111111111']).toEqual( + CANDIDATE_MAP['11111111-1111-4111-8111-111111111111'], + ); + }); + + it('DOUBLE-RUN: a digest soft-reject blocks completion entirely; a later successful run completes it with the undo note durable (Finding 6)', async () => { + // Finding 6: under the OLD ordering, completeTask ran BEFORE the digest write, so a + // soft-rejected digest write after a successful auto-complete permanently lost the undo + // note (next run the task is 'done' → skipped_ineligible → candidate removed → note gone). + // The fix reorders this: the undo note must be durably written to the digest BEFORE the + // task is completed. Run 1 proves the task is NOT completed while the digest isn't durable + // (and the candidate survives for retry); run 2 (fresh ctx, digest write now succeeding) + // proves the same candidate is then completed AND its undo note lands in the digest — the + // undo affordance survived the soft-reject instead of being silently lost. + const ctx1 = makeCtx(); + ctx1.__mem.__values.set( + COMPLETION_CANDIDATES_KEY, + JSON.stringify({ + '11111111-1111-4111-8111-111111111111': CANDIDATE_MAP['11111111-1111-4111-8111-111111111111'], + }), + ); + (ctx1.__mem.storeFact as ReturnType).mockImplementation( + async (p: { label: string; properties?: Record }) => { + if (p.label === COMPLETION_DIGEST_KEY) { + return { stored: false, action: 'conflict' as const }; + } + ctx1.__mem.__values.set(p.label, String(p.properties?.value ?? '')); + return { stored: true, action: 'created' as const }; + }, + ); + + const result1 = await handler.execute(ctx1); + expect(result1.success).toBe(true); + const data1 = (result1 as { data: { auto_completed: number } }).data; + // The task must NOT be completed — the digest (carrying its undo note) never became durable, + // so completing it now would leave a done task with no recoverable undo affordance. + expect(data1.auto_completed).toBe(0); + expect(ctx1.__completed).not.toContain('11111111-1111-4111-8111-111111111111'); + expect(ctx1.__mem.__values.has(COMPLETION_DIGEST_KEY)).toBe(false); + // The candidate is retained for retry — nothing was consumed. + const remaining1 = JSON.parse(ctx1.__mem.__values.get(COMPLETION_CANDIDATES_KEY)!) as CompletionCandidateMap; + expect(remaining1['11111111-1111-4111-8111-111111111111']).toEqual( + CANDIDATE_MAP['11111111-1111-4111-8111-111111111111'], + ); + + // Run 2: fresh context, same candidate, digest write now succeeds normally. + const ctx2 = makeCtx(); + ctx2.__mem.__values.set( + COMPLETION_CANDIDATES_KEY, + JSON.stringify({ + '11111111-1111-4111-8111-111111111111': CANDIDATE_MAP['11111111-1111-4111-8111-111111111111'], + }), + ); + + const result2 = await handler.execute(ctx2); + expect(result2.success).toBe(true); + const data2 = (result2 as { data: { auto_completed: number } }).data; + expect(data2.auto_completed).toBe(1); + expect(ctx2.__completed).toContain('11111111-1111-4111-8111-111111111111'); + // The undo note is now present in the digest — the affordance survived the earlier + // soft-reject instead of being permanently lost. + const digest2 = JSON.parse(ctx2.__mem.__values.get(COMPLETION_DIGEST_KEY)!); + expect(digest2['11111111-1111-4111-8111-111111111111'].kind).toBe('undo'); + // And the candidate is finally consumed. + const remaining2 = JSON.parse(ctx2.__mem.__values.get(COMPLETION_CANDIDATES_KEY)!) as CompletionCandidateMap; + expect(remaining2['11111111-1111-4111-8111-111111111111']).toBeUndefined(); + }); }); diff --git a/skills/task-completion-from-sent/handler.ts b/skills/task-completion-from-sent/handler.ts index 35a3a2ac7..dbd06de84 100644 --- a/skills/task-completion-from-sent/handler.ts +++ b/skills/task-completion-from-sent/handler.ts @@ -1,20 +1,24 @@ // task-completion-from-sent — risk-tiered completion from Sent matches (#1424). import type { SkillHandler, SkillContext, SkillResult } from '../../src/skills/types.js'; -import { PENDING_COMPLETIONS_PATH } from '../ceo-inbox-sent-observe/handler.js'; -import { VOICE_LEARNING_SCRATCH_PREFIX } from '../_shared/voice-learning-capture.js'; import { classifyTaskRisk, decideCompletionAction, - formatConfirmNote, - formatUndoNote, - parseCompletionCandidates, type CompletionAction, type TaskRisk, } from '../_shared/task-completion-risk.js'; - -export const COMPLETION_DIGEST_PATH = `${VOICE_LEARNING_SCRATCH_PREFIX}/completion-digest.md`; -export const COMPLETION_DIGEST_TYPE = 'task-completion-digest'; +import { ConfigStore } from '../../src/memory/config-store.js'; +import { + readCompletionCandidates, + writeCompletionCandidates, + readCompletionDigest, + writeCompletionDigest, + composeUndoNote, + composeConfirmNote, + type CompletionCandidateMap, + type CompletionDigestMap, + type CompletionDigestItem, +} from '../_shared/learning-state.js'; // Active (non-terminal) task statuses eligible for sent-mail completion. Mirrors the // active-status set used by the scheduler/backlog queries (src/db/queries/tasks.ts). @@ -23,46 +27,6 @@ export const COMPLETION_DIGEST_TYPE = 'task-completion-digest'; // a terminated task as done. const ACTIVE_TASK_STATUSES = new Set(['open', 'in_progress', 'waiting', 'blocked']); -async function appendDigest(ctx: SkillContext, content: string): Promise { - const repo = ctx.workingDocs!; - const existing = await repo.read(COMPLETION_DIGEST_PATH); - if (!existing) { - await repo.create({ - path: COMPLETION_DIGEST_PATH, - type: COMPLETION_DIGEST_TYPE, - frontmatter: { title: 'Task completion digest' }, - body: `# Task completion digest\n\n${content}`, - agentId: ctx.agentId, - }); - return; - } - await repo.append(COMPLETION_DIGEST_PATH, { - content, - expectedVersion: existing.version, - }); -} - -/** Extract a single candidate's markdown block (header to the next `## ` heading / EOF). */ -function candidateBlock(body: string, taskId: string): string { - const esc = taskId.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); - const m = body.match(new RegExp(`## Candidate — task ${esc}\\b[\\s\\S]*?(?=\\n## |$)`)); - return m ? m[0] : ''; -} - -function markCandidateProcessed(body: string, taskId: string, marker: string): string { - const re = new RegExp( - `(## Candidate — task ${taskId.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}[\\s\\S]*?)(- status:\\s*)pending`, - ); - if (re.test(body)) { - return body.replace(re, `$1$2${marker}`); - } - // Fallback: append guard marker near the candidate header. - return body.replace( - `## Candidate — task ${taskId}`, - `## Candidate — task ${taskId}\n- ${marker}\n- completion_asked: {${new Date().toISOString().slice(0, 10)}}`, - ); -} - export class TaskCompletionFromSentHandler implements SkillHandler { async execute(ctx: SkillContext): Promise { // Skill contract: never throw — normalize repo/document failures to a result. @@ -78,10 +42,10 @@ export class TaskCompletionFromSentHandler implements SkillHandler { } private async runCompletion(ctx: SkillContext): Promise { - if (!ctx.taskRepo || !ctx.workingDocs || !ctx.sensitivityClassifier) { + if (!ctx.taskRepo || !ctx.workingDocs || !ctx.sensitivityClassifier || !ctx.entityMemory) { return { success: false, - error: 'task-completion-from-sent requires taskRepo, workingDocs, sensitivityClassifier', + error: 'task-completion-from-sent requires taskRepo, workingDocs, sensitivityClassifier, entityMemory', }; } // Narrow closure over the classifier's classify() so classifyTaskRisk stays a pure, @@ -90,20 +54,30 @@ export class TaskCompletionFromSentHandler implements SkillHandler { // pass an empty properties bag (classify()'s second, required arg). const classify = (text: string) => ctx.sensitivityClassifier!.classify(text, {}); - const pendingDoc = await ctx.workingDocs.read(PENDING_COMPLETIONS_PATH); - if (!pendingDoc) { - return { - success: true, - data: { auto_completed: 0, queued_confirm: 0, skipped: 0 }, - }; + const store = new ConfigStore(ctx.entityMemory, ctx.log); + const candidateMap = await readCompletionCandidates(store, ctx.log); + const candidates = Object.entries(candidateMap).map(([taskId, c]) => ({ taskId, ...c })); + if (candidates.length === 0) { + return { success: true, data: { auto_completed: 0, queued_confirm: 0, skipped: 0 } }; } - - const candidates = parseCompletionCandidates(pendingDoc.body); + // Consume-by-delete: every candidate processed below (auto-completed, confirm-queued, or + // skipped-ineligible) is removed here. The persistent asked_task_ids guard (written by + // sent-observe) is what stops a task from being re-surfaced, so removal is always safe — + // there's no in-band "already asked" marker to preserve any more. + const remaining: CompletionCandidateMap = { ...candidateMap }; let autoCompleted = 0; let queuedConfirm = 0; let skipped = 0; - let body = pendingDoc.body; - const digestChunks: string[] = []; + const digestAdds: CompletionDigestItem[] = []; + // Pending auto-completions: populated by the loop below but NOT yet applied — completeTask + // only runs in the second pass after the digest write (carrying these tasks' undo notes) is + // confirmed durable. See the big comment above the digest write for why. + const toAutoComplete: Array<{ + taskId: string; + candidateTaskId: string; + subject: string; + messageId: string; + }> = []; for (const candidate of candidates) { const task = await ctx.taskRepo.getTask(candidate.taskId); @@ -116,7 +90,7 @@ export class TaskCompletionFromSentHandler implements SkillHandler { ACTIVE_TASK_STATUSES.has(task.status); if (!eligible) { skipped += 1; - body = markCandidateProcessed(body, candidate.taskId, 'skipped_ineligible'); + delete remaining[candidate.taskId]; continue; } @@ -162,60 +136,102 @@ export class TaskCompletionFromSentHandler implements SkillHandler { const recipient = candidate.recipients[0] ?? 'them'; if (action === 'auto_complete') { + // Durable-undo-before-complete (Finding 6): record the undo note and the pending + // completion here, but do NOT call completeTask and do NOT delete the candidate from + // `remaining` yet. Both happen only in the second pass below, after the digest write + // carrying this note is confirmed durable — see the comment above that write. + digestAdds.push({ + kind: 'undo', + taskId: task.id, + taskTitle: task.title || candidate.taskTitle, + note: composeUndoNote({ + taskTitle: task.title || candidate.taskTitle, + recipient, + sentAt: candidate.sentAt, + }), + }); + toAutoComplete.push({ + taskId: task.id, + candidateTaskId: candidate.taskId, + subject: candidate.subject, + messageId: candidate.messageId, + }); + } else { + digestAdds.push({ + kind: 'confirm', + taskId: task.id, + taskTitle: task.title || candidate.taskTitle, + note: composeConfirmNote({ + taskTitle: task.title || candidate.taskTitle, + recipient, + }), + }); + delete remaining[candidate.taskId]; + queuedConfirm += 1; + } + } + + // Durable-undo-before-complete ordering (Finding 6). The loop above populated `digestAdds` + // (undo/confirm notes) and `toAutoComplete` (pending auto-completions) WITHOUT calling + // completeTask and WITHOUT deleting auto-complete candidates from `remaining` — that only + // happens in the second pass below, gated on this digest write actually landing. This closes + // a gap in the previous ordering (complete → note → digest write): if the digest write + // soft-rejected AFTER an auto-complete, the task was already 'done' by the next run, got + // swept up as skipped_ineligible, and its undo note was gone with no recoverable trace. Under + // this ordering, an auto-completed task can never exist without a recoverable undo + // affordance already durably on record. + // + // One transient wart remains: if completeTask itself throws AFTER this digest write lands, + // the digest briefly describes a task that isn't completed yet. This self-heals next run — + // the task is still active, so it's re-evaluated, re-completed, and the note is overwritten + // identically (composeUndoNote is a pure function of the same candidate fields). + // + // Confirm-queued and skip-ineligible items are unaffected by this restructure: they never + // call completeTask, so they're deleted from `remaining` in the first loop as before — a lost + // digest write there just means a clean re-classification (or re-skip) next run. + // + // digestStored also gates both the second-pass completion loop and the final candidate- + // consume write: if the digest write soft-rejects (stored:false), we complete NOTHING and + // consume NOTHING this run — every candidate (including ineligible-skip and confirm-queue + // removals already computed in `remaining`) retries next run rather than persisting a + // candidate-queue state that doesn't match what was actually recorded. + let digestStored = true; + if (digestAdds.length > 0) { + const digestMap: CompletionDigestMap = await readCompletionDigest(store, ctx.log); + for (const item of digestAdds) digestMap[item.taskId] = item; + digestStored = await writeCompletionDigest(store, digestMap); + if (!digestStored) { + ctx.log.warn( + {}, + 'task-completion-from-sent: digest write soft-rejected — digest not durable, so no completions were applied and no candidates consumed; all retry next run', + ); + } + } + + // Second pass: only now that every pending auto-complete's undo note is confirmed durable is + // it safe to actually complete the tasks. + if (digestStored) { + for (const pending of toAutoComplete) { try { await ctx.taskRepo.completeTask( - task.id, - `Auto-completed from sent mail ${candidate.messageId} (${candidate.subject})`, + pending.taskId, + `Auto-completed from sent mail ${pending.messageId} (${pending.subject})`, ctx.agentId, ); - digestChunks.push( - formatUndoNote({ - taskId: task.id, - taskTitle: task.title || candidate.taskTitle, - recipient, - sentAt: candidate.sentAt, - }), - ); - body = markCandidateProcessed(body, candidate.taskId, 'auto_completed'); + delete remaining[pending.candidateTaskId]; autoCompleted += 1; } catch (err) { - ctx.log.error({ err, taskId: task.id }, 'task-completion-from-sent: auto-complete failed'); + ctx.log.error({ err, taskId: pending.taskId }, 'task-completion-from-sent: auto-complete failed'); + // Leave the candidate queued — completeTask failure is presumed transient (DB + // hiccup), so it stays in `remaining` and retries next run. The undo note is already + // durable and will be overwritten identically on retry, so nothing is lost. skipped += 1; } - } else { - digestChunks.push( - formatConfirmNote({ - taskId: task.id, - taskTitle: task.title || candidate.taskTitle, - recipient, - sentAt: candidate.sentAt, - confidence: candidate.confidence, - risk, - }), - ); - body = markCandidateProcessed(body, candidate.taskId, 'confirm_queued'); - // In-band guard so fuzzy candidates aren't re-surfaced — scoped to THIS - // candidate's block (a document-wide check let one candidate's marker suppress - // every later candidate's guard). - if (!/completion_asked:/i.test(candidateBlock(body, candidate.taskId))) { - body = body.replace( - `## Candidate — task ${candidate.taskId}`, - `## Candidate — task ${candidate.taskId}\n- completion_asked: {${new Date().toISOString().slice(0, 10)}}`, - ); - } - queuedConfirm += 1; } } - if (body !== pendingDoc.body) { - await ctx.workingDocs.update(PENDING_COMPLETIONS_PATH, { - body, - expectedVersion: pendingDoc.version, - }); - } - - if (digestChunks.length > 0) { - await appendDigest(ctx, digestChunks.join('')); + if (digestStored && Object.keys(remaining).length !== Object.keys(candidateMap).length) { + await writeCompletionCandidates(store, remaining); } ctx.log.info( diff --git a/skills/task-completion-from-sent/skill.json b/skills/task-completion-from-sent/skill.json index 06a87199a..7efde98ca 100644 --- a/skills/task-completion-from-sent/skill.json +++ b/skills/task-completion-from-sent/skill.json @@ -1,7 +1,7 @@ { "name": "task-completion-from-sent", - "description": "Consume sent-mail task-completion candidates: auto-complete low-risk high-confidence matches with a reversible undo note; queue confirmations for high-risk or fuzzy matches. Writes digest surfaces under /scratch/voice-learning/. Intended to run after ceo-inbox-sent-observe on the daily cron.", - "version": "0.1.0", + "description": "Consume sent-mail task-completion candidates: auto-complete low-risk high-confidence matches with a reversible undo note; queue confirmations for high-risk or fuzzy matches. Writes digest surfaces to the config-store key sent_observe.completion_digest. Intended to run after ceo-inbox-sent-observe on the daily cron.", + "version": "0.2.0", "sensitivity": "normal", "action_risk": "low", "inputs": {}, @@ -13,5 +13,5 @@ "permissions": [], "secrets": [], "timeout": 30000, - "capabilities": ["taskRepo", "workingDocs", "sensitivityClassifier"] + "capabilities": ["taskRepo", "workingDocs", "sensitivityClassifier", "entityMemory"] } diff --git a/skills/voice-learn/handler.test.ts b/skills/voice-learn/handler.test.ts index bdd36f64b..9e660a6e7 100644 --- a/skills/voice-learn/handler.test.ts +++ b/skills/voice-learn/handler.test.ts @@ -1,7 +1,6 @@ import { describe, it, expect, vi } from 'vitest'; import { VoiceLearnHandler, - PENDING_PROPOSALS_PATH, CONFIG_NAMESPACE, DIFFS_CHECKPOINT_KEY, } from './handler.js'; @@ -9,6 +8,7 @@ import type { SkillContext } from '../../src/skills/types.js'; import type { ExecutiveProfile } from '../../src/executive/types.js'; import type { EntityMemory } from '../../src/memory/entity-memory.js'; import { PENDING_DIFFS_PATH } from '../ceo-inbox-sent-observe/handler.js'; +import { VOICE_PROPOSAL_KEY } from '../_shared/learning-state.js'; const DIFFS = ` # Pending voice diffs @@ -181,30 +181,37 @@ describe('VoiceLearnHandler', () => { const handler = new VoiceLearnHandler(); it('proposes an updated guide from the diff corpus via the LLM', async () => { - const ctx = makeCtx({}); + const mem = makeEntityMemory(); + const ctx = makeCtx({ entityMemory: mem }); (ctx.infraLlm!.extract as ReturnType).mockResolvedValue({ ok: true, text: '- Writes short.\n- Dry humour.', }); const result = await handler.execute(ctx); expect(result.success).toBe(true); - const proposals = ctx.__docs.get(PENDING_PROPOSALS_PATH)?.body ?? ''; - expect(proposals).toContain('## Guide Proposal'); - expect(proposals).toContain('Dry humour'); + const stored = JSON.parse(mem.__values.get(VOICE_PROPOSAL_KEY)!) as { + status: string; + guide: string; + }; + expect(stored.status).toBe('pending'); + expect(stored.guide).toContain('Dry humour'); // profile NOT written directly (human-in-the-loop) expect(ctx.__updates).toHaveLength(0); }); it('supersedes an existing pending proposal instead of blocking the run', async () => { - // No entityMemory → no checkpoint gate, so the new diffs are fed and a fresh proposal is made. - const ctx = makeCtx({}); - ctx.__docs.set(PENDING_PROPOSALS_PATH, { - path: PENDING_PROPOSALS_PATH, - type: 'voice-pending-proposals', - frontmatter: { title: 'Pending voice guide proposal' }, - body: '# Pending voice guide proposal\n\n## Guide Proposal\n- status: pending\n- generated_at: 2026-07-01T00:00:00.000Z\n\n- OLD stale guidance.\n\n---\n', - version: 3, - }); + // Pre-seed a stale pending proposal directly in config, as if a prior run wrote it. No + // checkpoint is seeded, so the checkpoint filter doesn't gate this run's diffs either. + const mem = makeEntityMemory(); + mem.__values.set( + VOICE_PROPOSAL_KEY, + JSON.stringify({ + status: 'pending', + generatedAt: '2026-07-01T00:00:00.000Z', + guide: '- OLD stale guidance.', + }), + ); + const ctx = makeCtx({ entityMemory: mem }); (ctx.infraLlm!.extract as ReturnType).mockResolvedValue({ ok: true, text: '- Fresh guidance.', @@ -214,30 +221,35 @@ describe('VoiceLearnHandler', () => { expect(result.success).toBe(true); expect((result as { data: Record }).data).toMatchObject({ proposed: true }); - const body = ctx.__docs.get(PENDING_PROPOSALS_PATH)!.body; - // Exactly one pending proposal remains, and it is the fresh one — the stale block is gone. - expect((body.match(/## Guide Proposal/g) ?? []).length).toBe(1); - expect(body).toContain('Fresh guidance.'); - expect(body).not.toContain('OLD stale guidance.'); + // The whole-object config write supersedes the stale proposal — no accumulation, and the + // stale guide text is entirely gone (not just appended past). + const stored = JSON.parse(mem.__values.get(VOICE_PROPOSAL_KEY)!) as { + status: string; + guide: string; + }; + expect(stored.guide).toContain('Fresh guidance.'); + expect(stored.guide).not.toContain('OLD stale guidance.'); }); it('no pairs → no LLM call, no proposal', async () => { - const ctx = makeCtx({ diffs: '# empty\n' }); + const mem = makeEntityMemory(); + const ctx = makeCtx({ diffs: '# empty\n', entityMemory: mem }); const result = await handler.execute(ctx); expect(result.success).toBe(true); expect(ctx.infraLlm!.extract).not.toHaveBeenCalled(); - expect(ctx.__docs.get(PENDING_PROPOSALS_PATH)).toBeUndefined(); + expect(mem.__values.has(VOICE_PROPOSAL_KEY)).toBe(false); }); it('LLM failure → no proposal, success result', async () => { - const ctx = makeCtx({}); + const mem = makeEntityMemory(); + const ctx = makeCtx({ entityMemory: mem }); (ctx.infraLlm!.extract as ReturnType).mockResolvedValue({ ok: false, error: 'timeout', }); const result = await handler.execute(ctx); expect(result.success).toBe(true); - expect(ctx.__docs.get(PENDING_PROPOSALS_PATH)).toBeUndefined(); + expect(mem.__values.has(VOICE_PROPOSAL_KEY)).toBe(false); }); // CodeRabbit finding #11: without a consumption checkpoint, the never-consumed @@ -298,7 +310,7 @@ describe('VoiceLearnHandler', () => { expect(mem.__values.get(DIFFS_CHECKPOINT_KEY)).toBe('2026-07-03T12:00:00.000Z'); }); - it('without entityMemory wired, falls back to feeding every accumulated pair (pre-fix behaviour)', async () => { + it('without entityMemory wired, falls back to feeding every accumulated pair, but cannot persist a proposal without a config store', async () => { const ctx = makeCtx({}); // no entityMemory (ctx.infraLlm!.extract as ReturnType).mockResolvedValue({ ok: true, @@ -306,9 +318,16 @@ describe('VoiceLearnHandler', () => { }); const result = await handler.execute(ctx); expect(result.success).toBe(true); + // The checkpoint filter still falls back to feeding every accumulated pair + // (pairs_considered: 3, same as pre-migration). But the proposal now lives ONLY in config + // (no doc fallback), so without entityMemory there is nowhere to persist the LLM's output — + // `proposed` is false with `reason: 'no-config-store'`. This is a deliberate behavior change + // from pre-migration (which expected `proposed: true` here, since the doc write had no + // entityMemory dependency). expect((result as { data: Record }).data).toMatchObject({ pairs_considered: 3, - proposed: true, + proposed: false, + reason: 'no-config-store', }); }); @@ -382,8 +401,18 @@ describe('VoiceLearnHandler', () => { it('a failing checkpoint WRITE does not fail the run after the proposal is written', async () => { const mem = makeEntityMemory(); - // storeFact backs ConfigStore.set — make the checkpoint write throw. - (mem.storeFact as ReturnType).mockRejectedValue(new Error('store down')); + // storeFact backs ConfigStore.set. Pre-migration, the proposal write went through + // ctx.workingDocs (independent of storeFact), so rejecting storeFact unconditionally only + // ever hit the checkpoint write. Now the proposal write ALSO goes through storeFact, so we + // reject only the checkpoint key here to keep isolating a checkpoint-write-only failure — + // the proposal write (this test's premise) must still succeed. + (mem.storeFact as ReturnType).mockImplementation( + async (p: { label: string; properties?: Record }) => { + if (p.label === DIFFS_CHECKPOINT_KEY) throw new Error('store down'); + mem.__values.set(p.label, String(p.properties?.value ?? '')); + return { stored: true, action: 'created' as const }; + }, + ); const ctx = makeCtx({ entityMemory: mem }); (ctx.infraLlm!.extract as ReturnType).mockResolvedValue({ ok: true, @@ -393,7 +422,42 @@ describe('VoiceLearnHandler', () => { expect(result.success).toBe(true); expect((result as { data: Record }).data).toMatchObject({ proposed: true }); // The proposal was still persisted despite the checkpoint write failing. - expect(ctx.__docs.get(PENDING_PROPOSALS_PATH)?.body ?? '').toContain('## Guide Proposal'); + expect(mem.__values.get(VOICE_PROPOSAL_KEY)).toContain('Writes short.'); + expect(mem.__values.has(DIFFS_CHECKPOINT_KEY)).toBe(false); + }); + + it('a SOFT-rejected proposal write (stored:false, no throw) does not advance the checkpoint', async () => { + // Unlike the "checkpoint WRITE fails" test above (which throws only for the checkpoint + // key, proving the proposal write itself is independent), this drives the opposite and + // more dangerous case: the PROPOSAL write soft-rejects — resolves normally with + // stored:false — while the checkpoint write would otherwise succeed. Before the fix, + // writeVoiceProposal returned void and the handler always advanced the checkpoint right + // after calling it, so a soft-rejected proposal (nothing actually persisted) would still + // retire this batch of evidence forever — the proposal is simply lost with no retry. + const mem = makeEntityMemory(); + (mem.storeFact as ReturnType).mockImplementation( + async (p: { label: string; properties?: Record }) => { + if (p.label === VOICE_PROPOSAL_KEY) { + return { stored: false, action: 'conflict' as const }; + } + mem.__values.set(p.label, String(p.properties?.value ?? '')); + return { stored: true, action: 'created' as const }; + }, + ); + const ctx = makeCtx({ entityMemory: mem }); + (ctx.infraLlm!.extract as ReturnType).mockResolvedValue({ + ok: true, + text: '- Writes short.', + }); + + const result = await handler.execute(ctx); + expect(result.success).toBe(true); + const data = (result as { data: Record }).data; + expect(data).toMatchObject({ proposed: false, reason: 'proposal-not-persisted' }); + // The proposal was never actually stored. + expect(mem.__values.has(VOICE_PROPOSAL_KEY)).toBe(false); + // The checkpoint must NOT have advanced — the evidence must retry on the next run. + expect(mem.__values.has(DIFFS_CHECKPOINT_KEY)).toBe(false); }); }); }); diff --git a/skills/voice-learn/handler.ts b/skills/voice-learn/handler.ts index 8eeddca07..e18fe1358 100644 --- a/skills/voice-learn/handler.ts +++ b/skills/voice-learn/handler.ts @@ -8,15 +8,12 @@ import type { SkillHandler, SkillContext, SkillResult } from '../../src/skills/types.js'; import { ConfigStore } from '../../src/memory/config-store.js'; import { buildVoiceGuidePrompt, parsePendingDiffs } from '../_shared/voice-learn-logic.js'; -import { pruneGuideProposals } from '../_shared/learning-digest.js'; import { PENDING_DIFFS_PATH } from '../ceo-inbox-sent-observe/handler.js'; -import { VOICE_LEARNING_SCRATCH_PREFIX } from '../_shared/voice-learning-capture.js'; +import { writeVoiceProposal } from '../_shared/learning-state.js'; // Kept for Task 9 (resolve-learning-digest) and cooldown bookkeeping, even though // this handler no longer reads/writes provenance directly. export const DISMISSED_KEY = 'voice_learn.dismissed'; -export const PENDING_PROPOSALS_PATH = `${VOICE_LEARNING_SCRATCH_PREFIX}/pending-proposals.md`; -export const PENDING_PROPOSALS_TYPE = 'voice-pending-proposals'; export const CONFIG_NAMESPACE = 'ceo_inbox'; // Checkpoint (ISO timestamp string) marking the newest `sentAt` among diff pairs already fed @@ -94,14 +91,6 @@ export class VoiceLearnHandler implements SkillHandler { }; } - // Read the proposals doc so the write below can supersede any still-unapproved proposal - // (T2.1). We no longer gate the whole run on a pending proposal — the single-pending - // invariant is enforced at write time by pruning the old block, so a fresher proposal - // replaces a stale one instead of blocking it. (The checkpoint above already stops the - // SAME evidence being re-proposed; this handles genuinely new evidence arriving before the - // CEO acted on the prior proposal.) - const existing = await ctx.workingDocs.read(PENDING_PROPOSALS_PATH); - // Respect the dismissal cooldown the digest writes on `dismiss voice`. Best-effort and // additive: only consulted when entityMemory is wired (guarded, not a hard capability), and // a missing/garbage record simply doesn't gate. Without this the cooldown was written but @@ -173,23 +162,35 @@ export class VoiceLearnHandler implements SkillHandler { }; } - const block = `## Guide Proposal\n- status: pending\n- generated_at: ${new Date().toISOString()}\n\n${guide}\n\n---\n`; - if (!existing) { - await ctx.workingDocs.create({ - path: PENDING_PROPOSALS_PATH, - type: PENDING_PROPOSALS_TYPE, - frontmatter: { title: 'Pending voice guide proposal' }, - body: `# Pending voice guide proposal\n\n${block}`, - agentId: ctx.agentId, - }); - } else { - // Prune any existing proposal block (pending or resolved) and write the fresh one, so at - // most one pending proposal ever lives in the doc and it never accumulates (T2.1). - const header = pruneGuideProposals(existing.body, { removePending: true }).trimEnd(); - await ctx.workingDocs.update(PENDING_PROPOSALS_PATH, { - body: `${header}\n\n${block}`, - expectedVersion: existing.version, - }); + // Supersede any prior proposal by writing the single proposal object whole. The checkpoint + // (below) is what stops the SAME evidence being re-proposed; this write replaces a stale + // still-pending proposal with the fresher one. configStore is guaranteed here — a proposal + // requires entityMemory to persist state; if it's unavailable we cannot record the proposal. + if (!configStore) { + ctx.log.warn({}, 'voice-learn: config store unavailable — cannot record proposal this run'); + return { + success: true, + data: { pairs_considered: newPairs.length, proposed: false, reason: 'no-config-store' }, + }; + } + const proposalStored = await writeVoiceProposal(configStore, { + status: 'pending', + generatedAt: new Date().toISOString(), + guide, + }); + if (!proposalStored) { + // Soft-reject (stored:false, no thrown error) — do NOT advance the checkpoint below, or + // this evidence would never be retried and the proposal would simply vanish. Returning + // early here (before the checkpoint-advance block) is what keeps this batch eligible on + // the next run. + ctx.log.warn( + {}, + 'voice-learn: proposal write soft-rejected — not advancing checkpoint so evidence retries', + ); + return { + success: true, + data: { pairs_considered: newPairs.length, proposed: false, reason: 'proposal-not-persisted' }, + }; } // Advance the checkpoint only now that the proposal write above has succeeded — a failed diff --git a/skills/voice-learn/skill.json b/skills/voice-learn/skill.json index 14dacd0d5..3990346f5 100644 --- a/skills/voice-learn/skill.json +++ b/skills/voice-learn/skill.json @@ -1,7 +1,7 @@ { "name": "voice-learn", "description": "Weekly job: read accumulated (draft, sent) diffs from OKF, make one batched LLM pass to produce an updated free-form voice guide, and queue it as a digest proposal. Never writes the profile directly (human-in-the-loop). Never fabricates on zero data. Intended for the ceo-inbox weekly voice-learn cron.", - "version": "0.1.0", + "version": "0.1.1", "sensitivity": "normal", "action_risk": "medium", "inputs": {}, diff --git a/src/memory/config-store.ts b/src/memory/config-store.ts index c659f466c..d2bce2f4e 100644 --- a/src/memory/config-store.ts +++ b/src/memory/config-store.ts @@ -69,9 +69,12 @@ export class ConfigStore { * Write or update a value. Propagates infrastructure errors after logging so * callers (e.g. EmailAdapter.poll) can catch and emit their own diagnostic. * A soft storeFact rejection (result.stored === false) is logged as a warning - * and does not throw — the prior value is still valid in that case. + * and does not throw — the prior value is still valid in that case. Returns + * `{ stored }` so callers that need to react to a soft-reject (e.g. hold a + * watermark rather than advance it) can do so; existing callers that just + * `await` this and discard the result are unaffected (additive change). */ - async set(namespace: string, key: string, value: string): Promise { + async set(namespace: string, key: string, value: string): Promise<{ stored: boolean }> { try { const anchor = await this.findOrCreateAnchor(namespace); @@ -97,6 +100,8 @@ export class ConfigStore { 'ConfigStore.set: storeFact rejected the write — prior value still in effect', ); } + + return { stored: result.stored }; } catch (err) { // Log at the ConfigStore level for observability, then rethrow so callers // can emit their own domain-specific diagnostic (e.g. "failed to persist