Epic: Email observation — voice learning, task-completion & capability growth#1429
Conversation
Code review — deficiencies to addressSolid architecture and good unit coverage on the pure helpers. A few integration-seam issues, though, that only manifest at runtime and aren't caught by the current tests. Flagging the ones I'd want fixed before this goes to prod. 🔴 1. Three new skills are never pinned to
|
|
@CodeAnt-AI review |
|
@coderabbitai review |
✅ Action performedReview finished.
|
coderabbitai
Bot
added
channels
📝 WalkthroughWalkthroughAdds a passive Sent-mail observation pipeline with pagination, watermarking, draft/task matching, evidence retention, and shadow-draft reconciliation. Introduces OKF snapshots for CEO drafts, LLM-generated writing-voice guide proposals, risk-tiered task completion with undo support, learning-digest listing and resolution, shared sensitivity-classifier injection, and autonomy action-log support. Updates agent schedules, skill manifests, executive profile handling, ADR/spec documentation, migrations, and tests. Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (28)
src/db/migrations/073_shadow_evaluated_outcome.sql-37-45 (1)
37-45: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake the down migration tolerate
shadow_evaluatedrows.
The rollback restores an olderCHECKthat rejectsshadow_evaluated, so any existing row with that outcome will make the downgrade abort. Remap or remove those rows before reinstating the constraint, or keep the new value in the down path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/db/migrations/073_shadow_evaluated_outcome.sql` around lines 37 - 45, The down migration must handle existing autonomy_action_log rows with outcome shadow_evaluated before reinstating autonomy_action_log_outcome_check. Update the rollback to remap or remove those rows, or retain shadow_evaluated in the restored constraint, so the downgrade completes without violating the check.src/db/migrations/073_shadow_evaluated_outcome.sql-12-27 (1)
12-27: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftAvoid this boot-time lock trap.
src/index.tsruns migrations on startup, and this SQL file runs in node-pg-migrate’s default transaction, so theALTER TABLEscan plusDROP/CREATE INDEXwill holdautonomy_action_loglocked while the process is trying to come up. The down migration is also a dead end once anyshadow_evaluatedrows exist, because it re-adds the oldCHECKconstraint unchanged. Split the data change from the locky DDL, and make the index rebuild concurrent or move it to a maintenance window.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/db/migrations/073_shadow_evaluated_outcome.sql` around lines 12 - 27, Restructure migration 073_shadow_evaluated_outcome so startup does not hold autonomy_action_log locks during the transaction: separate the outcome constraint update from the index rebuild, and run the idx_aal_unscored rebuild concurrently or defer it to a maintenance migration. Ensure the concurrent DDL is executed outside node-pg-migrate’s default transaction. Update the down migration so rollback remains valid when shadow_evaluated rows exist, rather than restoring an incompatible constraint.Source: Linters/SAST tools
skills/_shared/voice-learning-capture.ts-58-65 (1)
58-65: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve existing linked tasks when an edit omits them.
frontmatteralways supplieslinked_task_ids, so update merges cannot distinguish “omitted” from “clear the list”. Preserve the existing value wheninput.linkedTaskIdsis undefined.Also applies to: 81-100
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/_shared/voice-learning-capture.ts` around lines 58 - 65, Update the frontmatter construction in the voice-learning capture flow so edit updates preserve the existing linked_task_ids when input.linkedTaskIds is undefined, while still allowing an explicitly provided empty list to clear them. Adjust the merge/update logic covering the linked_task_ids handling rather than unconditionally defaulting undefined values to an empty array.skills/_shared/voice-learning-capture.ts-58-100 (1)
58-100: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winOmitted
linked_task_idsare currently treated as an explicit clear. A routine edit therefore removes task associations captured when the draft was created.
skills/_shared/voice-learning-capture.ts#L58-L100: preserve existinglinked_task_idswhen the input field is undefined.skills/ceo-inbox-draft-edit/handler.ts#L175-L175: omitlinkedTaskIdsunless the caller suppliedlinked_task_ids.skills/_shared/voice-learning-capture.test.ts#L84-L106: add a body-only edit regression test proving existing task IDs survive.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/_shared/voice-learning-capture.ts` around lines 58 - 100, Preserve existing linked_task_ids during edits when input.linkedTaskIds is undefined by excluding that field from the merged frontmatter in the capture update and retry paths; update skills/_shared/voice-learning-capture.ts lines 58-100 accordingly. In skills/ceo-inbox-draft-edit/handler.ts line 175, omit linkedTaskIds unless linked_task_ids was supplied by the caller. Add a body-only edit regression test in skills/_shared/voice-learning-capture.test.ts lines 84-106 verifying existing task IDs remain unchanged.skills/_shared/voice-learning-capture.ts-68-117 (1)
68-117: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftThe best-effort snapshot remains on the critical response path. An unbounded
workingDocscall can time out after Nylas has already committed the mutation, producing false failure responses and duplicate retries.
skills/_shared/voice-learning-capture.ts#L68-L117: bound storage latency or dispatch snapshot persistence through a reliable asynchronous mechanism.skills/ceo-inbox-draft-compose/handler.ts#L112-L123: return draft success independently of snapshot completion.skills/ceo-inbox-draft-edit/handler.ts#L166-L174: return edit success independently of snapshot completion.skills/ceo-inbox-draft-reply/handler.ts#L149-L160: return reply-draft success independently of snapshot completion.Based on PR objectives, draft capture must be non-blocking.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/_shared/voice-learning-capture.ts` around lines 68 - 117, Make draft snapshot persistence non-blocking: in skills/_shared/voice-learning-capture.ts lines 68-117, bound or asynchronously dispatch the repo read/create/update flow so it cannot delay the response. In skills/ceo-inbox-draft-compose/handler.ts lines 112-123, skills/ceo-inbox-draft-edit/handler.ts lines 166-174, and skills/ceo-inbox-draft-reply/handler.ts lines 149-160, return mutation success independently of voice-learning capture completion while preserving background error handling.skills/_shared/ceo-nylas-client.ts-256-260 (1)
256-260: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve truncation when an empty page still has a cursor.
An empty page with a non-empty
nextCursorstill means pagination is incomplete, but this path returnstruncated = false. The observer can then advance its watermark past messages it never saw. Miserable little way to lose mail.Proposed fix
- if (data.length === 0) break; + if (data.length === 0) { + truncated = Boolean(nextCursor); + break; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/_shared/ceo-nylas-client.ts` around lines 256 - 260, Update the empty-page branch in the message pagination loop to set truncated based on whether nextCursor is present before breaking. Ensure an empty page with a non-empty cursor preserves truncated = true, while an empty page without a cursor remains untruncated; leave the maxScan handling unchanged.skills/_shared/voice-learn-logic.ts-75-76 (1)
75-76: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUse the final generated delimiter, not the first horizontal rule.
extractSection(..., '---')truncates a sent email at any signature separator containing---. The learner then analyses half an email, because apparently even markdown dividers have ambitions.Use the final wrapper delimiter added by
formatDiffBlock, or encode the body before persistence.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/_shared/voice-learn-logic.ts` around lines 75 - 76, Update the sentBody extraction in the voice-learning flow to use the final delimiter produced by formatDiffBlock instead of the generic '---' marker, preventing signatures or earlier horizontal rules from truncating the email. Keep the existing Draft extraction unchanged and preserve the full sent email content through analysis.skills/_shared/task-completion-risk.ts-68-75 (1)
68-75: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winTrack confirmation state globally by task ID.
Candidate sections extend until the next Candidate heading, while Confirm blocks are appended separately. With several candidates, the appended
completion_askedmarkers land in the final section: earlier tasks can be asked again, and the final task may be skipped because of another task’s marker. Splendidly deterministic chaos.Parse
## Confirm — task <id>headings into a set, then skip only the matching candidate ID.Also applies to: 123-133
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/_shared/task-completion-risk.ts` around lines 68 - 75, Update the task parsing logic around the candidate-section loop to first parse all “## Confirm — task <id>” headings into a set keyed by task ID. Replace the section-wide completion_asked check with a lookup for the current taskId, so each candidate is skipped only when its own confirmation exists.skills/_shared/voice-learn-logic.ts-101-111 (1)
101-111: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMeasure content similarity rather than shared prefixes.
A completely unrelated rewrite of similar length has
lenRatio ≈ 1, so this function returnsfalseregardless of zero textual overlap. Such rewrites then feed vocabulary and sign-off learning despite the stated exclusion.Replace this with token or edit similarity and add an equal-length, unrelated-body regression case.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/_shared/voice-learn-logic.ts` around lines 101 - 111, Update isNearTotalRewrite to measure whole-content similarity using token overlap or edit similarity instead of shared-prefix length. Ensure unrelated drafts and sent text of similar length are classified as near-total rewrites, while retaining the existing empty-input behavior; add a regression case covering equal-length unrelated bodies.skills/_shared/sent-observe-match.ts-215-223 (1)
215-223: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRequire an exact recipient token before granting high confidence.
taskLower.includes(local)letsann@example.commatch “annual planning”. With modest semantic overlap, that unrelated task becomes high-confidence and may be auto-completed downstream. A triumph of optimism over identity.Proposed fix
const taskText = `${task.title}\n${task.description ?? ''}`; const taskLower = taskText.toLowerCase(); + const taskTokens = tokenize(taskText); const recipientHit = sentRecipients.some((email) => { const local = email.split('@')[0] ?? email; - return taskLower.includes(email) || (local.length >= 3 && taskLower.includes(local)); + return taskLower.includes(email) || (local.length >= 3 && taskTokens.has(local)); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/_shared/sent-observe-match.ts` around lines 215 - 223, Update the recipient matching logic in the sent-observation flow around recipientHit so the email address or local part is matched as an exact token, not via substring inclusion. Tokenize or otherwise enforce token boundaries for the recipient value, while preserving the existing minimum three-character local-part requirement and overlap threshold.skills/_shared/shadow-draft.ts-62-101 (1)
62-101: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDecision equivalence currently confuses shared vocabulary with shared intent. Normalize and compare actual decision polarity before recording competence, then lock the correction down with adversarial tests.
skills/_shared/shadow-draft.ts#L62-L101: reject opposing decisions and messages where neither side expresses a decision.skills/_shared/shadow-draft.test.ts#L18-L34: add high-overlap approve-versus-decline and no-decision regression cases.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/_shared/shadow-draft.ts` around lines 62 - 101, Update scoreDecisionEquivalence in skills/_shared/shadow-draft.ts (lines 62-101) to normalize and compare decision polarity, rejecting opposing decisions and cases where neither message expresses a decision before applying vocabulary or number overlap. Add adversarial regression tests in skills/_shared/shadow-draft.test.ts (lines 18-34) covering high-overlap approve-versus-decline messages and messages with no decision.skills/ceo-inbox-shadow-draft/handler.ts-32-36 (1)
32-36: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDo not log the subject of a thread classified as highly sensitive.
The exclusion correctly avoids storing board, legal, or spouse content, then promptly places its subject in operational logs. Remove
subjectfrom this log context.Proposed fix
ctx.log.info( - { sourceMessageId, subject }, + { sourceMessageId }, 'ceo-inbox-shadow-draft: skipping high-sensitivity thread', );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/ceo-inbox-shadow-draft/handler.ts` around lines 32 - 36, Update the high-sensitivity branch in the isHighSensitivityThread handling to remove subject from the ctx.log.info context, while preserving sourceMessageId and the existing skip message.skills/ceo-inbox-sent-observe/handler.ts-139-142 (1)
139-142: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not advance the watermark after evidence persistence fails.
After three conflicts,
appendDocmerely warns and returns. The caller then advances the watermark, permanently losing draft or task evidence that never reached OKF. Return a failure signal and leave the watermark unchanged. The warning is otherwise little more than an epitaph.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/ceo-inbox-sent-observe/handler.ts` around lines 139 - 142, Update the append-after-conflicts path in appendDoc to return an explicit failure signal instead of only warning and returning success-like control flow. Ensure the caller checks that signal and does not advance the watermark when evidence persistence fails, while preserving watermark advancement only after a successful append.skills/ceo-inbox-sent-observe/handler.ts-145-145 (1)
145-145: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winContain asynchronous failures inside
SkillResult.Only secret retrieval is caught. Config, Nylas, document, task, action-log, or bus failures can escape
executeinstead of returning{ success: false, error }.As per coding guidelines, skill handlers must return a failure result and never throw.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/ceo-inbox-sent-observe/handler.ts` at line 145, Update the execute method to contain all asynchronous failures within its SkillResult response, not only secret-retrieval errors. Wrap the full Config, Nylas, document, task, action-log, and bus execution flow in the existing error-handling pattern, returning { success: false, error } for any rejection while preserving successful results.Source: Coding guidelines
skills/ceo-inbox-sent-observe/handler.ts-385-396 (1)
385-396: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDo not mark an incomplete Sent window as fully observed.
When truncated, older messages remain unprocessed, yet the watermark advances beyond the window and the warning confirms they will never be revisited. Persist a continuation or drain oldest-first before advancing. Logging data loss does not make it idempotent.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/ceo-inbox-sent-observe/handler.ts` around lines 385 - 396, When the Sent scan is truncated, do not advance the observation watermark through the incomplete window or treat it as fully observed. Update the handler’s truncation flow around the `truncated` warning and `watermarkAdvancedTo` to persist a continuation or drain messages oldest-first, ensuring older unprocessed messages are revisited before the watermark advances.agents/ceo-inbox.yaml-68-74 (1)
68-74: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMake Sent-observe mode run the completion skill promised by the cron.
The scheduled task requires
ceo-inbox-sent-observefollowed bytask-completion-from-sent, while the system prompt says to run only the observer, report, and exit. Add the completion call before reporting. An admirably efficient way to schedule work and then forbid it.Also applies to: 97-100
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@agents/ceo-inbox.yaml` around lines 68 - 74, Update the Sent-observe scheduled task configuration and its corresponding system-prompt instructions to invoke task-completion-from-sent immediately after ceo-inbox-sent-observe and before scheduler-report. Ensure both occurrences consistently permit and require the completion step while preserving the prohibition on inbox triage, drafting, archiving, labeling, or sending.skills/ceo-inbox-sent-observe/handler.ts-294-347 (1)
294-347: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake shadow reconciliation durably idempotent.
The in-memory
shadowslist remains unchanged, so multiple Sent messages in one thread can score the same shadow repeatedly. Also, insertion precedes the reconciliation marker; an update conflict causes another insertion next run. Use a stable uniqueness key for each shadow and track claimed shadows during the run.#!/bin/bash ast-grep outline src/autonomy/action-log-repo.ts --match insert --view expanded rg -n -C5 'shadow_evaluated|UNIQUE|unique|task_id|source_message_id' \ src/autonomy/action-log-repo.ts src/db/migrations🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/ceo-inbox-sent-observe/handler.ts` around lines 294 - 347, Make the shadow reconciliation block durably idempotent by deriving a stable uniqueness key from each shadow and recording claimed shadow IDs in a run-local set before processing additional Sent messages. Claim or persist the reconciliation marker before calling ctx.actionLogRepo.insert, and skip shadows already claimed or marked reconciled; handle marker-update conflicts without allowing a duplicate shadow_evaluated insertion. Use the repository’s existing uniqueness support for the stable key.skills/ceo-inbox-sent-observe/handler.ts-241-245 (1)
241-245: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftRemove the 100-task ceiling
taskRepo.listTasksonly acceptslimit, so this observer can inspect at most 100 CEO tasks before the Sent watermark moves on and the rest are left to the void. Add pagination/keyset support in the repo and walk every eligible task here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/ceo-inbox-sent-observe/handler.ts` around lines 241 - 245, Remove the fixed limit of 100 from the listTasks call in the observer and extend taskRepo.listTasks with pagination or keyset support. Update the observer flow around openTasks to repeatedly fetch every CEO task with status open or in_progress until no eligible tasks remain, ensuring the Sent watermark advances only after all pages are inspected.skills/voice-learn/handler.ts-54-72 (1)
54-72: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftDo not permanently freeze approved voice dimensions.
Treating
approvedas permanently blocked means later sent-mail evidence can never produce another proposal for that field. Track an evidence fingerprint or consumed watermark instead, so only previously considered evidence is deduplicated. A learning loop that eventually stops learning—marvellous.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/voice-learn/handler.ts` around lines 54 - 72, Update blockedProposalFields so approved proposals do not permanently block their fields from future learning. Track and compare an evidence fingerprint or consumed watermark for each proposal, deduplicating only evidence already considered while allowing new sent-mail evidence to generate another proposal; preserve pending proposals as blocked and keep dismissed handling through dismissedDimensions.skills/voice-learn/handler.ts-75-259 (1)
75-259: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAdd contract-preserving error boundaries to both skill handlers.
Both handlers allow service failures to escape despite the repository’s “never throw” skill contract.
skills/voice-learn/handler.ts#L75-L259: catch and log failures from profile, configuration, and working-document operations.skills/list-learning-digest/handler.ts#L12-L42: catch and log failed working-document reads.As per coding guidelines, “Skills must return
{ success: true, data }or{ success: false, error }; they must never throw.” <coding_guidelines>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/voice-learn/handler.ts` around lines 75 - 259, Wrap the main operation in voice-learn’s execute method with an error boundary covering profile, configuration, and working-document calls; log the failure and return { success: false, error } instead of allowing exceptions to escape. Also wrap the working-document reads in list-learning-digest’s handler, log read failures, and return the same failure shape so both skill handlers always satisfy the never-throw contract. Affected sites: skills/voice-learn/handler.ts lines 75-259 and skills/list-learning-digest/handler.ts lines 12-42.Source: Coding guidelines
skills/voice-learn/handler.ts-154-180 (1)
154-180: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winApply every supported voice patch on the automatic path.
This merge ignores
formality_delta,tone, andpatterns, although the proposal-resolution path supports them. Such decisions can be reported as automatically applied while leaving the profile unchanged.Proposed correction
...(patch.vocabulary && typeof patch.vocabulary === 'object' ? { vocabulary: { ... }, } : {}), + ...(typeof patch.formality_delta === 'number' + ? { + formality: Math.max( + 0, + Math.min(100, current.formality + patch.formality_delta), + ), + } + : {}), + ...(Array.isArray(patch.tone) ? { tone: patch.tone as string[] } : {}), + ...(Array.isArray(patch.patterns) ? { patterns: patch.patterns as string[] } : {}),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/voice-learn/handler.ts` around lines 154 - 180, Update the automatic merge in the decision application try block to apply every supported voice patch field, including formality_delta, tone, and patterns, matching the behavior of the proposal-resolution path. Preserve the existing sign_off and vocabulary merge semantics while ensuring automatically applied decisions update all corresponding writingVoice properties.skills/resolve-learning-digest/handler.ts-35-41 (1)
35-41: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winNormalise thrown service failures into
SkillResult.Every awaited repository/service call can reject, yet
executehas no enclosingtry/catch. One dreary database failure therefore escapes the skill contract rather than returning{ success: false, error }. Log the exception and return a failed result.As per coding guidelines, skill handlers must return the documented result shape and must never throw.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/resolve-learning-digest/handler.ts` around lines 35 - 41, Wrap the awaited service and repository work in execute with an enclosing try/catch so rejected calls never escape the skill contract. In the catch block, log the exception using the handler’s existing logging convention and return a failed SkillResult containing the error details; preserve the current validation failure and successful result behavior.Source: Coding guidelines
skills/resolve-learning-digest/handler.ts-97-122 (1)
97-122: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake resolution side effects idempotent before updating the digest.
Profile/config/task mutations happen before the optimistic
workingDocs.update. If that update conflicts or fails, the action remains pending despite already taking effect. Retrying can reapplyformality_delta, fail while reopening an already-open task, or otherwise leave two stores disagreeing—distributed transactions, what joy.Use a durable action/idempotency key or staged
resolvingstate so retries cannot repeat the side effect.Also applies to: 127-146, 157-180
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/resolve-learning-digest/handler.ts` around lines 97 - 122, Make proposal resolution idempotent across the side effects in the approval, rejection, and snooze handlers, including the profile/config/task mutations before markProposalStatus and workingDocs.update. Introduce a durable action/idempotency key or persist a resolving state before applying mutations, then detect and safely resume or skip already-applied actions on retries so optimistic update conflicts cannot leave pending proposals with duplicated or conflicting effects.skills/resolve-learning-digest/handler.ts-154-185 (1)
154-185: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRequire a matching actionable digest item before mutating a task.
undo_completionreopens any supplied completed task, whileconfirm_completioncan complete any supplied task.markCompletionStatusmay then change nothing, yet the handler still reports success. Parse the digest first and require the matching task and expected kind/status before callingtaskRepo.Proposed validation
+ const completionItems = parseCompletionDigest(digest.body); + const expectedKind = action === 'undo_completion' ? 'undo' : 'confirm'; + const item = completionItems.find( + (candidate) => candidate.taskId === taskId && candidate.kind === expectedKind, + ); + if (!item) { + return { success: false, error: `No actionable ${expectedKind} item for task ${taskId}` }; + } + if (action === 'undo_completion') {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/resolve-learning-digest/handler.ts` around lines 154 - 185, The handler’s undo and confirm branches mutate tasks without verifying that taskId matches an actionable digest item. Before calling taskRepo methods in the action handling flow, parse the digest and require a matching taskId with the expected kind and current status for each action; return a failure when validation fails, and only then call reopenTask or completeTask and markCompletionStatus.skills/task-completion-from-sent/handler.ts-51-172 (1)
51-172: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winContain repository failures within the skill contract.
Several
read,update, and digest operations can reject outside local catches, allowingexecute()to throw. Wrap the orchestration in a logged top-level catch returning{ success: false, error }. Life is quite difficult enough without escaping skill failures.As per coding guidelines, “Skills must return
{ success: true, data }or{ success: false, error }; they must never throw.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/task-completion-from-sent/handler.ts` around lines 51 - 172, Wrap the orchestration in execute, including workingDocs read/update and appendDigest operations, with a top-level try/catch so repository or digest failures never escape the skill contract. Log the caught error through ctx.log and return { success: false, error } from the catch; preserve the existing success results and local auto-completion handling.Source: Coding guidelines
skills/task-completion-from-sent/handler.ts-105-158 (1)
105-158: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake completion and digest persistence retry-safe.
The task update, pending-document update, and digest write form a non-atomic saga:
- If the pending-document update fails after completion, the retry sees a done task and never creates its undo note.
- If digest writing fails after the pending status update, the candidate is no longer selected.
Use explicit processing/finalized states plus idempotent digest keys so retries reconcile partial writes rather than permanently losing confirmations or undo controls.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/task-completion-from-sent/handler.ts` around lines 105 - 158, Update the processing flow around task completion, pending-document updates, and appendDigest to use explicit processing/finalized states and idempotent digest keys. Persist enough state before and after each step for retries to detect and reconcile partial completion, recreate missing undo or confirmation digest entries, and avoid duplicating existing entries. Ensure candidates remain recoverable until both the task/pending status and corresponding digest write are finalized.skills/task-completion-from-sent/handler.ts-74-80 (1)
74-80: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRevalidate that the task is still open and CEO-owned.
The candidate may be stale or modified after observation. Currently, any non-done/non-cancelled task can be completed, contrary to the documented
owner='ceo', open-task boundary.Proposed eligibility guard
const task = await ctx.taskRepo.getTask(candidate.taskId); - if (!task || task.status === 'done' || task.status === 'cancelled') { + if (!task || task.owner !== 'ceo' || task.status !== 'open') { skipped += 1; - body = markCandidateProcessed(body, candidate.taskId, 'skipped_missing_or_done'); + body = markCandidateProcessed(body, candidate.taskId, 'skipped_ineligible'); continue; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/task-completion-from-sent/handler.ts` around lines 74 - 80, Update the eligibility guard in the candidate loop around ctx.taskRepo.getTask so only tasks that are open and owned by the CEO proceed to completion. Skip and mark candidates whose task is missing, done, cancelled, has a non-open status, or has an owner other than 'ceo', preserving the existing skipped_missing_or_done processing path.skills/task-completion-from-sent/handler.ts-82-92 (1)
82-92: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winFail closed when subtask discovery fails.
Treating a repository error as
hasSubtasks = falsecan downgrade a high-risk parent and auto-complete it;completeTask()may then cancel its descendants. Marvellous.Proposed fail-closed handling
- } catch { - hasSubtasks = false; + } catch (err) { + ctx.log.warn( + { err, taskId: task.id }, + 'task-completion-from-sent: subtask lookup failed; treating as high risk', + ); + hasSubtasks = true; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/task-completion-from-sent/handler.ts` around lines 82 - 92, Update the subtask discovery logic in the task completion handler so a failure from ctx.taskRepo.listTasks does not set hasSubtasks to false. Propagate the repository error or otherwise stop completion when discovery fails, ensuring completeTask cannot auto-complete or cancel descendants based on an unknown subtask state; retain the current behavior when the lookup succeeds.
🟡 Minor comments (5)
tests/unit/autonomy/ceo-inbox-autonomy-injection.test.ts-30-36 (1)
30-36: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTest the production wiring rather than copying it.
This local
shouldInjectfunction duplicates the predicate fromsrc/index.ts, so the test remains green even if the realAgentRuntimeconstruction stops injectingautonomyService. Extract the predicate/options builder or assert against the actual construction path; otherwise the test is merely reviewing its own cheerful little imitation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/autonomy/ceo-inbox-autonomy-injection.test.ts` around lines 30 - 36, Replace the locally duplicated shouldInject predicate in the “injection predicate includes ceo-inbox alongside coordinator” test with an assertion against the production wiring in src/index.ts. Extract and reuse the predicate/options builder used by AgentRuntime construction, or exercise the actual construction path, so the test verifies autonomyService injection for both coordinator and ceo-inbox while preserving the false case for unrelated agents.skills/_shared/task-completion-risk.ts-40-42 (1)
40-42: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDo not classify every task containing “plan” as high risk.
The alternation makes “Plan lunch” and “Plan the offsite” high risk, although the comment only identifies “Plan AGM” as sensitive. Remove
plan;agmalready captures that example.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/_shared/task-completion-risk.ts` around lines 40 - 42, Update the title heuristic regex in the task risk classification logic to remove the broad “plan” alternative, while retaining “agm”, “board”, “legal”, and “investors?” so generic planning titles remain low risk.skills/_shared/voice-learn-logic.test.ts-69-75 (1)
69-75: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winActually include a verbatim pair in this test.
The test title promises verbatim filtering, but all three fixtures differ. That branch can regress while this test continues smiling vacantly.
Add a fourth draft/sent pair with identical normalized bodies and retain the expected length of three.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/_shared/voice-learn-logic.test.ts` around lines 69 - 75, Update the parsePendingDiffs test to add a fourth SAMPLE_DIFFS draft/sent pair whose normalized bodies are identical, while keeping the expected parsed length at three. Preserve the existing qualifying assertions so the test verifies that verbatim pairs are filtered out.skills/ceo-inbox-sent-observe/skill.json-10-16 (1)
10-16: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winKeep the declared output shape aligned with the handler.
Normal runs return
shadow_reconciled, but the manifest omits it and skipped runs omit the field entirely. Declare it here and return0during backoff, rather than offering callers a shape-shifting contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/ceo-inbox-sent-observe/skill.json` around lines 10 - 16, Update the declared outputs in the skill manifest and the corresponding handler to include shadow_reconciled consistently. Return shadow_reconciled as 0 during backoff and preserve the normal-run value, ensuring every execution path returns the same output shape.skills/task-completion-from-sent/handler.ts-137-143 (1)
137-143: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCheck
completion_askedwithin the current candidate block.After the first confirmation, the document-wide checks are both true for every later candidate. Consequently, subsequent fuzzy candidates receive no guard marker. Scope the lookup to the candidate’s Markdown block and add regression assertions for each fixture candidate.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/task-completion-from-sent/handler.ts` around lines 137 - 143, Update the in-band guard in the candidate-processing flow around markCandidateProcessed so completion_asked is checked only within the current candidate’s Markdown block, not across the entire document. Add the marker when that block lacks completion_asked, and add regression assertions covering every fixture candidate to verify each receives the correct guard behavior.
🧹 Nitpick comments (1)
skills/task-completion-from-sent/handler.test.ts (1)
159-163: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftReplace the prototype check with an actual reopen test.
The title promises reversible behaviour; the assertion merely confirms JavaScript has prototypes. Add a PostgreSQL integration test covering done→open, non-done rejection, audit-note persistence, and the emitted event.
As per coding guidelines, “Integration tests must use real PostgreSQL via Docker rather than mocks.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/task-completion-from-sent/handler.test.ts` around lines 159 - 163, Replace the prototype-only assertion in the reopenTask test with a real PostgreSQL integration test using the existing Docker-backed database setup. Exercise done→open reopening, verify non-done tasks are rejected, confirm the audit note is persisted, and assert the expected event is emitted through the relevant TaskRepo/event symbols.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Major comments:
In `@agents/ceo-inbox.yaml`:
- Around line 68-74: Update the Sent-observe scheduled task configuration and
its corresponding system-prompt instructions to invoke task-completion-from-sent
immediately after ceo-inbox-sent-observe and before scheduler-report. Ensure
both occurrences consistently permit and require the completion step while
preserving the prohibition on inbox triage, drafting, archiving, labeling, or
sending.
In `@skills/_shared/ceo-nylas-client.ts`:
- Around line 256-260: Update the empty-page branch in the message pagination
loop to set truncated based on whether nextCursor is present before breaking.
Ensure an empty page with a non-empty cursor preserves truncated = true, while
an empty page without a cursor remains untruncated; leave the maxScan handling
unchanged.
In `@skills/_shared/sent-observe-match.ts`:
- Around line 215-223: Update the recipient matching logic in the
sent-observation flow around recipientHit so the email address or local part is
matched as an exact token, not via substring inclusion. Tokenize or otherwise
enforce token boundaries for the recipient value, while preserving the existing
minimum three-character local-part requirement and overlap threshold.
In `@skills/_shared/shadow-draft.ts`:
- Around line 62-101: Update scoreDecisionEquivalence in
skills/_shared/shadow-draft.ts (lines 62-101) to normalize and compare decision
polarity, rejecting opposing decisions and cases where neither message expresses
a decision before applying vocabulary or number overlap. Add adversarial
regression tests in skills/_shared/shadow-draft.test.ts (lines 18-34) covering
high-overlap approve-versus-decline messages and messages with no decision.
In `@skills/_shared/task-completion-risk.ts`:
- Around line 68-75: Update the task parsing logic around the candidate-section
loop to first parse all “## Confirm — task <id>” headings into a set keyed by
task ID. Replace the section-wide completion_asked check with a lookup for the
current taskId, so each candidate is skipped only when its own confirmation
exists.
In `@skills/_shared/voice-learn-logic.ts`:
- Around line 75-76: Update the sentBody extraction in the voice-learning flow
to use the final delimiter produced by formatDiffBlock instead of the generic
'---' marker, preventing signatures or earlier horizontal rules from truncating
the email. Keep the existing Draft extraction unchanged and preserve the full
sent email content through analysis.
- Around line 101-111: Update isNearTotalRewrite to measure whole-content
similarity using token overlap or edit similarity instead of shared-prefix
length. Ensure unrelated drafts and sent text of similar length are classified
as near-total rewrites, while retaining the existing empty-input behavior; add a
regression case covering equal-length unrelated bodies.
In `@skills/_shared/voice-learning-capture.ts`:
- Around line 58-65: Update the frontmatter construction in the voice-learning
capture flow so edit updates preserve the existing linked_task_ids when
input.linkedTaskIds is undefined, while still allowing an explicitly provided
empty list to clear them. Adjust the merge/update logic covering the
linked_task_ids handling rather than unconditionally defaulting undefined values
to an empty array.
- Around line 58-100: Preserve existing linked_task_ids during edits when
input.linkedTaskIds is undefined by excluding that field from the merged
frontmatter in the capture update and retry paths; update
skills/_shared/voice-learning-capture.ts lines 58-100 accordingly. In
skills/ceo-inbox-draft-edit/handler.ts line 175, omit linkedTaskIds unless
linked_task_ids was supplied by the caller. Add a body-only edit regression test
in skills/_shared/voice-learning-capture.test.ts lines 84-106 verifying existing
task IDs remain unchanged.
- Around line 68-117: Make draft snapshot persistence non-blocking: in
skills/_shared/voice-learning-capture.ts lines 68-117, bound or asynchronously
dispatch the repo read/create/update flow so it cannot delay the response. In
skills/ceo-inbox-draft-compose/handler.ts lines 112-123,
skills/ceo-inbox-draft-edit/handler.ts lines 166-174, and
skills/ceo-inbox-draft-reply/handler.ts lines 149-160, return mutation success
independently of voice-learning capture completion while preserving background
error handling.
In `@skills/ceo-inbox-sent-observe/handler.ts`:
- Around line 139-142: Update the append-after-conflicts path in appendDoc to
return an explicit failure signal instead of only warning and returning
success-like control flow. Ensure the caller checks that signal and does not
advance the watermark when evidence persistence fails, while preserving
watermark advancement only after a successful append.
- Line 145: Update the execute method to contain all asynchronous failures
within its SkillResult response, not only secret-retrieval errors. Wrap the full
Config, Nylas, document, task, action-log, and bus execution flow in the
existing error-handling pattern, returning { success: false, error } for any
rejection while preserving successful results.
- Around line 385-396: When the Sent scan is truncated, do not advance the
observation watermark through the incomplete window or treat it as fully
observed. Update the handler’s truncation flow around the `truncated` warning
and `watermarkAdvancedTo` to persist a continuation or drain messages
oldest-first, ensuring older unprocessed messages are revisited before the
watermark advances.
- Around line 294-347: Make the shadow reconciliation block durably idempotent
by deriving a stable uniqueness key from each shadow and recording claimed
shadow IDs in a run-local set before processing additional Sent messages. Claim
or persist the reconciliation marker before calling ctx.actionLogRepo.insert,
and skip shadows already claimed or marked reconciled; handle marker-update
conflicts without allowing a duplicate shadow_evaluated insertion. Use the
repository’s existing uniqueness support for the stable key.
- Around line 241-245: Remove the fixed limit of 100 from the listTasks call in
the observer and extend taskRepo.listTasks with pagination or keyset support.
Update the observer flow around openTasks to repeatedly fetch every CEO task
with status open or in_progress until no eligible tasks remain, ensuring the
Sent watermark advances only after all pages are inspected.
In `@skills/ceo-inbox-shadow-draft/handler.ts`:
- Around line 32-36: Update the high-sensitivity branch in the
isHighSensitivityThread handling to remove subject from the ctx.log.info
context, while preserving sourceMessageId and the existing skip message.
In `@skills/resolve-learning-digest/handler.ts`:
- Around line 35-41: Wrap the awaited service and repository work in execute
with an enclosing try/catch so rejected calls never escape the skill contract.
In the catch block, log the exception using the handler’s existing logging
convention and return a failed SkillResult containing the error details;
preserve the current validation failure and successful result behavior.
- Around line 97-122: Make proposal resolution idempotent across the side
effects in the approval, rejection, and snooze handlers, including the
profile/config/task mutations before markProposalStatus and workingDocs.update.
Introduce a durable action/idempotency key or persist a resolving state before
applying mutations, then detect and safely resume or skip already-applied
actions on retries so optimistic update conflicts cannot leave pending proposals
with duplicated or conflicting effects.
- Around line 154-185: The handler’s undo and confirm branches mutate tasks
without verifying that taskId matches an actionable digest item. Before calling
taskRepo methods in the action handling flow, parse the digest and require a
matching taskId with the expected kind and current status for each action;
return a failure when validation fails, and only then call reopenTask or
completeTask and markCompletionStatus.
In `@skills/task-completion-from-sent/handler.ts`:
- Around line 51-172: Wrap the orchestration in execute, including workingDocs
read/update and appendDigest operations, with a top-level try/catch so
repository or digest failures never escape the skill contract. Log the caught
error through ctx.log and return { success: false, error } from the catch;
preserve the existing success results and local auto-completion handling.
- Around line 105-158: Update the processing flow around task completion,
pending-document updates, and appendDigest to use explicit processing/finalized
states and idempotent digest keys. Persist enough state before and after each
step for retries to detect and reconcile partial completion, recreate missing
undo or confirmation digest entries, and avoid duplicating existing entries.
Ensure candidates remain recoverable until both the task/pending status and
corresponding digest write are finalized.
- Around line 74-80: Update the eligibility guard in the candidate loop around
ctx.taskRepo.getTask so only tasks that are open and owned by the CEO proceed to
completion. Skip and mark candidates whose task is missing, done, cancelled, has
a non-open status, or has an owner other than 'ceo', preserving the existing
skipped_missing_or_done processing path.
- Around line 82-92: Update the subtask discovery logic in the task completion
handler so a failure from ctx.taskRepo.listTasks does not set hasSubtasks to
false. Propagate the repository error or otherwise stop completion when
discovery fails, ensuring completeTask cannot auto-complete or cancel
descendants based on an unknown subtask state; retain the current behavior when
the lookup succeeds.
In `@skills/voice-learn/handler.ts`:
- Around line 54-72: Update blockedProposalFields so approved proposals do not
permanently block their fields from future learning. Track and compare an
evidence fingerprint or consumed watermark for each proposal, deduplicating only
evidence already considered while allowing new sent-mail evidence to generate
another proposal; preserve pending proposals as blocked and keep dismissed
handling through dismissedDimensions.
- Around line 75-259: Wrap the main operation in voice-learn’s execute method
with an error boundary covering profile, configuration, and working-document
calls; log the failure and return { success: false, error } instead of allowing
exceptions to escape. Also wrap the working-document reads in
list-learning-digest’s handler, log read failures, and return the same failure
shape so both skill handlers always satisfy the never-throw contract. Affected
sites: skills/voice-learn/handler.ts lines 75-259 and
skills/list-learning-digest/handler.ts lines 12-42.
- Around line 154-180: Update the automatic merge in the decision application
try block to apply every supported voice patch field, including formality_delta,
tone, and patterns, matching the behavior of the proposal-resolution path.
Preserve the existing sign_off and vocabulary merge semantics while ensuring
automatically applied decisions update all corresponding writingVoice
properties.
In `@src/db/migrations/073_shadow_evaluated_outcome.sql`:
- Around line 37-45: The down migration must handle existing autonomy_action_log
rows with outcome shadow_evaluated before reinstating
autonomy_action_log_outcome_check. Update the rollback to remap or remove those
rows, or retain shadow_evaluated in the restored constraint, so the downgrade
completes without violating the check.
- Around line 12-27: Restructure migration 073_shadow_evaluated_outcome so
startup does not hold autonomy_action_log locks during the transaction: separate
the outcome constraint update from the index rebuild, and run the
idx_aal_unscored rebuild concurrently or defer it to a maintenance migration.
Ensure the concurrent DDL is executed outside node-pg-migrate’s default
transaction. Update the down migration so rollback remains valid when
shadow_evaluated rows exist, rather than restoring an incompatible constraint.
---
Minor comments:
In `@skills/_shared/task-completion-risk.ts`:
- Around line 40-42: Update the title heuristic regex in the task risk
classification logic to remove the broad “plan” alternative, while retaining
“agm”, “board”, “legal”, and “investors?” so generic planning titles remain low
risk.
In `@skills/_shared/voice-learn-logic.test.ts`:
- Around line 69-75: Update the parsePendingDiffs test to add a fourth
SAMPLE_DIFFS draft/sent pair whose normalized bodies are identical, while
keeping the expected parsed length at three. Preserve the existing qualifying
assertions so the test verifies that verbatim pairs are filtered out.
In `@skills/ceo-inbox-sent-observe/skill.json`:
- Around line 10-16: Update the declared outputs in the skill manifest and the
corresponding handler to include shadow_reconciled consistently. Return
shadow_reconciled as 0 during backoff and preserve the normal-run value,
ensuring every execution path returns the same output shape.
In `@skills/task-completion-from-sent/handler.ts`:
- Around line 137-143: Update the in-band guard in the candidate-processing flow
around markCandidateProcessed so completion_asked is checked only within the
current candidate’s Markdown block, not across the entire document. Add the
marker when that block lacks completion_asked, and add regression assertions
covering every fixture candidate to verify each receives the correct guard
behavior.
In `@tests/unit/autonomy/ceo-inbox-autonomy-injection.test.ts`:
- Around line 30-36: Replace the locally duplicated shouldInject predicate in
the “injection predicate includes ceo-inbox alongside coordinator” test with an
assertion against the production wiring in src/index.ts. Extract and reuse the
predicate/options builder used by AgentRuntime construction, or exercise the
actual construction path, so the test verifies autonomyService injection for
both coordinator and ceo-inbox while preserving the false case for unrelated
agents.
---
Nitpick comments:
In `@skills/task-completion-from-sent/handler.test.ts`:
- Around line 159-163: Replace the prototype-only assertion in the reopenTask
test with a real PostgreSQL integration test using the existing Docker-backed
database setup. Exercise done→open reopening, verify non-done tasks are
rejected, confirm the audit note is persisted, and assert the expected event is
emitted through the relevant TaskRepo/event symbols.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 83c0e026-f3fd-48dc-8e75-4501b0941070
📒 Files selected for processing (56)
CHANGELOG.mdagents/ceo-inbox.yamlagents/coordinator.yamlconfig/registry-defaults.yamldocs/adr/029-passive-email-observation-and-counterfactual-competence.mddocs/adr/README.mddocs/specs/04-channels.mddocs/specs/13-office-identity.mddocs/specs/14-autonomy-engine.mddocs/specs/19-tasks-and-backlog.mdskills/_shared/ceo-nylas-client.tsskills/_shared/learning-digest.test.tsskills/_shared/learning-digest.tsskills/_shared/sent-observe-match.test.tsskills/_shared/sent-observe-match.tsskills/_shared/shadow-draft.test.tsskills/_shared/shadow-draft.tsskills/_shared/task-completion-risk.test.tsskills/_shared/task-completion-risk.tsskills/_shared/voice-learn-logic.test.tsskills/_shared/voice-learn-logic.tsskills/_shared/voice-learning-capture.test.tsskills/_shared/voice-learning-capture.tsskills/ceo-inbox-draft-compose/handler.tsskills/ceo-inbox-draft-compose/skill.jsonskills/ceo-inbox-draft-edit/handler.tsskills/ceo-inbox-draft-edit/skill.jsonskills/ceo-inbox-draft-reply/handler.test.tsskills/ceo-inbox-draft-reply/handler.tsskills/ceo-inbox-draft-reply/skill.jsonskills/ceo-inbox-sent-observe/handler.test.tsskills/ceo-inbox-sent-observe/handler.tsskills/ceo-inbox-sent-observe/skill.jsonskills/ceo-inbox-shadow-draft/handler.test.tsskills/ceo-inbox-shadow-draft/handler.tsskills/ceo-inbox-shadow-draft/skill.jsonskills/list-learning-digest/handler.test.tsskills/list-learning-digest/handler.tsskills/list-learning-digest/skill.jsonskills/resolve-learning-digest/handler.test.tsskills/resolve-learning-digest/handler.tsskills/resolve-learning-digest/skill.jsonskills/task-completion-from-sent/handler.test.tsskills/task-completion-from-sent/handler.tsskills/task-completion-from-sent/skill.jsonskills/voice-learn/handler.test.tsskills/voice-learn/handler.tsskills/voice-learn/skill.jsonsrc/autonomy/action-log-repo.tssrc/autonomy/action-log-types.tssrc/bus/events.tssrc/bus/permissions.tssrc/db/migrations/073_shadow_evaluated_outcome.sqlsrc/db/task-repo.tssrc/index.tstests/unit/autonomy/ceo-inbox-autonomy-injection.test.ts
…contract Addresses CodeRabbit review on #1429: - listAllMessages: an empty page with a lingering cursor now reports truncated so the caller can't mistake it for a drained window. - Hold the watermark when OKF evidence fails to persist, so those sends are re-observed next run instead of silently forgotten. - Wrap the whole run in the never-throw skill contract. - Dedup shadow reconciliation within a run (claimed set) so several sends in one thread can't double-score the same shadow. - Warn when the open-CEO-task fetch cap is hit (partial match set is now visible, not silent). - Declare shadow_reconciled in the manifest and return it on every path. Signed-off-by: Joseph Fung <joseph@josephfung.ca>
… contract Addresses CodeRabbit review on #1429: - isNearTotalRewrite now measures token overlap, so an unrelated rewrite of similar length is excluded (shared-prefix let it through). - Sent-body extraction uses the final block delimiter, so an email containing its own '---' line is no longer truncated mid-body. - Skip deltas the profile already reflects and stop blocking 'approved' fields, so an applied change won't re-nag yet the field keeps learning. - Apply formality/tone/patterns on the auto path too (defensive parity with the resolve path). - Wrap voice-learn and list-learning-digest in the never-throw contract. Signed-off-by: Joseph Fung <joseph@josephfung.ca>
…ck-scoped guard Addresses CodeRabbit review on #1429: - Re-validate each candidate is still open and owner='ceo' before completing (candidates can go stale after observation). - Fail closed on subtask-lookup errors (treat as high-risk) so a parent can't be auto-completed and cancel its descendants. - Scope the completion_asked guard to the candidate's own block; a document-wide check let one marker suppress every later candidate. - Drop bare 'plan' from the high-risk title heuristic ('Plan lunch' is not high-risk; 'agm' still covers the AGM case). Signed-off-by: Joseph Fung <joseph@josephfung.ca>
…bject Addresses CodeRabbit review on #1429: - scoreDecisionEquivalence now classifies affirm/deny/none and rejects opposing decisions outright — approve-vs-decline with high vocabulary overlap no longer scores competence=1 into the autonomy log. One-sided and purely-informational pairs are held to a higher bar. Fails safe. - Drop the subject from the high-sensitivity skip log (board/legal/spouse content shouldn't land in operational logs). Signed-off-by: Joseph Fung <joseph@josephfung.ca>
Addresses CodeRabbit review on #1429: linked_task_ids was rebuilt on every edit, so a routine body edit wiped the task associations captured at draft creation. Handle the field per-path — undefined preserves the existing ids, an explicit [] still clears — and have draft-edit pass undefined unless the caller supplied linked_task_ids. Signed-off-by: Joseph Fung <joseph@josephfung.ca>
…ion test Addresses CodeRabbit review on #1429: - resolve-learning-digest: require a matching actionable digest item before reopening/completing a task, and wrap in the never-throw contract (undo/confirm no longer mutate arbitrary task ids). - ceo-inbox.yaml sent-observe mode: run task-completion-from-sent after the observer, matching the cron's stated intent. - migration 073 down: delete synthetic shadow_evaluated rows before restoring the narrower CHECK, so rollback can't abort on them. - autonomy injection: single-source the predicate (AutonomyService. receivesInjection) so the test verifies production wiring, not a copy. Signed-off-by: Joseph Fung <joseph@josephfung.ca>
… substring Addresses CodeRabbit review on #1429: taskLower.includes(local) let ann@example.com match the word 'annual', pushing an unrelated task to high confidence (and possible auto-completion). Require the local-part to be a whole token. Signed-off-by: Joseph Fung <joseph@josephfung.ca>
Response to @coderabbitai reviewWorked through all 34 comments (28 Major, 5 Minor, 1 Nitpick). Most were valid and are now fixed with tests; a few architectural ones are partially addressed or deliberately deferred with rationale below. Pushed across commits CodeRabbit consolidated everything into the review body (no inline threads to reply on), so this is the consolidated disposition. ✅ Fixed
🟨 Partially addressed
⏭️ Deferred (with reason)
Thanks for the thorough pass — the decision-polarity and recipient-token ones in particular were real correctness bugs feeding the autonomy score. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== Target files =='
fd -t f -E node_modules \
'^(handler|voice-learn-logic|sent-observe-match|ceo-nylas-client|voice-learning-capture|shadow-draft|task-completion-risk)(\.test)?\.ts$|^073_shadow_evaluated_outcome\.sql$|^ceo-inbox\.yaml$|^skill\.json$' \
| rg '(^agents/ceo-inbox\.yaml$|^src/db/migrations/073_shadow_evaluated_outcome\.sql$|^skills/(_shared/(ceo-nylas-client|voice-learn-logic|sent-observe-match|voice-learning-capture|shadow-draft|task-completion-risk)|ceo-inbox-sent-observe|voice-learn|task-completion-from-sent|resolve-learning-digest|ceo-inbox-shadow-draft)/)'
printf '%s\n' '== Outlines =='
ast-grep outline skills/ceo-inbox-sent-observe/handler.ts --items all --type class,function --view expanded
ast-grep outline skills/voice-learn/handler.ts --items all --type class,function --view expanded
ast-grep outline skills/task-completion-from-sent/handler.ts --items all --type class,function --view expanded
ast-grep outline skills/resolve-learning-digest/handler.ts --items all --type class,function --view expanded
ast-grep outline skills/_shared/voice-learn-logic.ts --items all --type function --view expanded
ast-grep outline skills/_shared/shadow-draft.ts --items all --type function --view expanded
ast-grep outline skills/_shared/ceo-nylas-client.ts --items all --type class,function --view expanded
printf '%s\n' '== Pinning and schedule =='
rg -n -A18 -B4 'pinned_skills:|sent-observe|voice-learn|shadow-draft|task-completion-from-sent' agents/ceo-inbox.yaml
printf '%s\n' '== Key implementation markers =='
rg -n -A8 -B6 'truncated|WATERMARK_KEY|appendDoc|reconciled_at|claimed|completion_asked|patchIsNoop|detectDecisionPolarity|receivedAfter|next_cursor|shadow_evaluated|DELETE FROM autonomy_action_log' \
skills/_shared/ceo-nylas-client.ts \
skills/ceo-inbox-sent-observe/handler.ts \
skills/voice-learn/handler.ts \
skills/task-completion-from-sent/handler.ts \
skills/resolve-learning-digest/handler.ts \
skills/_shared/shadow-draft.ts \
src/db/migrations/073_shadow_evaluated_outcome.sql
printf '%s\n' '== Relevant regression test names =='
rg -n -A2 -B1 'it\(|test\(' \
skills/ceo-inbox-sent-observe/handler.test.ts \
skills/voice-learn/handler.test.ts \
skills/task-completion-from-sent/handler.test.ts \
skills/_shared/voice-learn-logic.test.ts \
skills/_shared/shadow-draft.test.ts \
skills/_shared/sent-observe-match.test.tsLength of output: 75287 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== Sent observer: persistence, watermark, truncation =='
sed -n '104,151p;230,455p' skills/ceo-inbox-sent-observe/handler.ts
printf '%s\n' '== Voice learning: deduplication and application =='
sed -n '50,108p;122,270p' skills/voice-learn/handler.ts
printf '%s\n' '== Task completion: task validation and candidate state =='
sed -n '1,210p' skills/task-completion-from-sent/handler.ts
printf '%s\n' '== Digest resolution: actionable-item guard =='
sed -n '1,230p' skills/resolve-learning-digest/handler.ts
printf '%s\n' '== Relevant regression assertions =='
sed -n '360,451p' skills/ceo-inbox-sent-observe/handler.test.ts
sed -n '145,195p' skills/voice-learn/handler.test.ts
sed -n '147,195p' skills/task-completion-from-sent/handler.test.ts
printf '%s\n' '== Default-registry rollout note =='
rg -n -A12 -B4 'email-observation-learning|ceo-inbox-\*|voice-learn|task-completion-from-sent' config/registry-defaults.yamlLength of output: 46578
The deferred items are clearly bounded and accurately characterised as trade-offs—not quietly disguised as guarantees—particularly the capped initial/backfill scan and the remaining cross-run/idempotent-saga work. Acceptable for this PR, assuming the follow-ups remain tracked. One tiny documentation nit remains: in ✏️ Learnings added
|
CodeRabbit follow-up on #1429: the comment still described the reverted held-watermark behaviour. minDate is diagnostics-only (oldestScannedAt in the truncation warning); the watermark advances to maxDate + 1. Signed-off-by: Joseph Fung <joseph@josephfung.ca>
|
Thanks for the re-review. Fixed the documentation nit in The deferred follow-ups (oldest-first backfill draining, cross-run shadow/saga idempotency, keyset task pagination, the reopenTask integration test) remain tracked and will be filed as issues on the epic's milestone. |
Signed-off-by: Joseph Fung <joseph@josephfung.ca>
…guide flow Signed-off-by: Joseph Fung <joseph@josephfung.ca>
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
skills/resolve-learning-digest/handler.ts (1)
125-133: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winCheck that reopening actually succeeded before consuming the undo.
reopenTask()returnsnullwhen the task no longer exists. The current branch nevertheless removes the digest item and reports success, leaving the user with neither a reopened task nor another chance to undo. Check the result before updating the document.Proposed fix
- await ctx.taskRepo.reopenTask(taskId, 'Undo auto-complete from sent mail', ctx.agentId); + const reopened = await ctx.taskRepo.reopenTask( + taskId, + 'Undo auto-complete from sent mail', + ctx.agentId, + ); + if (!reopened) { + return { success: false, error: `Task ${taskId} not found; cannot undo completion` }; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/resolve-learning-digest/handler.ts` around lines 125 - 133, Update the undo_completion branch in the handler to capture the result of reopenTask and verify it is non-null before calling removeCompletionBlock or updating COMPLETION_DIGEST_PATH. If reopening returns null, return a failure response without consuming the digest item; preserve the existing success response only after a successful reopen.skills/ceo-inbox-sent-observe/handler.ts (2)
335-335: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRetry draft evidence when the full Sent body cannot be fetched.
Line 375 persists the truncated snippet, records the draft as matched, and later advances the watermark. The incomplete evidence is therefore never repaired and can poison voice proposals. Splendid.
Only append the diff after
getMessage()succeeds, and hold the watermark on failure.Proposed fix
let draftMatches = 0; +let draftEvidenceComplete = true; const draftMatch = matchDraftToSent(msg, snapshots, alreadyMatchedDraftIds); if (draftMatch) { - alreadyMatchedDraftIds.add(draftMatch.draftId); try { const full = await client.getMessage(msg.id); sentBody = htmlToPlainText(full.body) || full.snippet || sentBody; fetchedBodies.set(msg.id, sentBody); + alreadyMatchedDraftIds.add(draftMatch.draftId); + diffChunks.push(formatDiffBlock(draftMatch, sentBody)); + draftMatches += 1; } catch (err) { + draftEvidenceComplete = false; ctx.log.warn( { err, messageId: msg.id }, - 'ceo-inbox-sent-observe: getMessage failed — using snippet for sent body', + 'ceo-inbox-sent-observe: getMessage failed — holding watermark for draft evidence retry', ); } - diffChunks.push(formatDiffBlock(draftMatch, sentBody)); - draftMatches += 1; } -const advanceOk = evidencePersisted && shadowReconcileOk; +const advanceOk = evidencePersisted && shadowReconcileOk && draftEvidenceComplete;Also applies to: 368-382, 552-552
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/ceo-inbox-sent-observe/handler.ts` at line 335, Update the Sent-message handling around getMessage so draft evidence is appended and draftMatches is incremented only after the full message fetch succeeds. On fetch failure, retain the truncated item for retry without persisting it as matched or advancing the watermark; preserve normal watermark advancement after successful processing.
568-570: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winStore
maxDate, notmaxDate + 1.
received_afteris exclusive, so the extra second drops any message stamped exactly then — a lovely little off-by-one. Update the pagination and truncation expectations that still assertnewest + 1.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/ceo-inbox-sent-observe/handler.ts` around lines 568 - 570, Update the watermark advancement logic around watermarkAdvancedTo to store maxDate directly instead of maxDate + 1, preserving the exclusive received_after pagination semantics. Revise related pagination and truncation expectations that currently assert newest + 1 to assert newest.skills/_shared/voice-learning-capture.test.ts (1)
36-41: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRestore real timers after this suite.
vi.useFakeTimers()inbeforeEachneeds a matchingafterEach(() => vi.useRealTimers()), or the mocked clock can leak into later tests in the worker. A dreary little footgun, but easily avoided.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@skills/_shared/voice-learning-capture.test.ts` around lines 36 - 41, Add an afterEach hook to the captureDraftSnapshot suite that calls vi.useRealTimers() after each test. Keep the existing fake-timer setup in beforeEach unchanged and ensure real timers are restored even when individual tests fail.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@skills/_shared/learning-digest.ts`:
- Around line 114-135: Update removeCompletionBlock so it removes the actionable
completion block for the task rather than the first matching header. Match the
exact task header together with the expected status line: undo_available for
Undo and pending_confirm for Confirm; continue removing only one block and
preserve unrelated or historical blocks.
In `@skills/_shared/voice-learning-capture.ts`:
- Around line 82-90: Serialize capture writes per draftId in
voice-learning-capture.ts so overlapping captures cannot allow an older body to
become the final snapshot; do not blindly retry stale writes. In
skills/_shared/voice-learning-capture.test.ts lines 134-148, replace the
drop-on-conflict expectation with a reordered or concurrent capture test that
verifies the final snapshot contains the newest draft body.
In `@skills/ceo-inbox-sent-observe/skill.json`:
- Line 4: Update the skill manifest’s version value from 0.2.1 to the required
initial release version 0.1.0, leaving the rest of the manifest unchanged.
In `@skills/voice-learn/handler.ts`:
- Around line 63-71: Make checkpoint persistence best-effort in the handler’s
ConfigStore get/set flow: catch failures from both operations, log warnings
through ctx.log, and continue learning without checkpoint filtering or
persistence instead of aborting or reporting failure after proposal creation.
Preserve the existing fallback behavior when checkpoint data is unavailable,
covering both the checkpoint read near checkpointMs and the later write path.
- Around line 126-128: Update the batching and checkpoint flow around
currentGuide, newPairs, and the checkpoint update so each invocation processes
the oldest pending batch rather than newPairs.slice(-MAX_PAIRS). Advance the
checkpoint only through the processed batch’s newest timestamp, preserving later
entries for subsequent invocations when more than MAX_PAIRS are pending.
In `@tests/integration/task-reopen-wake-revive.test.ts`:
- Around line 14-15: Validate that DATABASE_URL targets the curia_test database
before enabling the test suite, creating the database pool, or running
destructive cleanup. Update the describeIf guard and setup flow in
task-reopen-wake-revive.test.ts to skip safely when the configured URL is
missing or names another database, while preserving normal execution for valid
curia_test URLs.
---
Outside diff comments:
In `@skills/_shared/voice-learning-capture.test.ts`:
- Around line 36-41: Add an afterEach hook to the captureDraftSnapshot suite
that calls vi.useRealTimers() after each test. Keep the existing fake-timer
setup in beforeEach unchanged and ensure real timers are restored even when
individual tests fail.
In `@skills/ceo-inbox-sent-observe/handler.ts`:
- Line 335: Update the Sent-message handling around getMessage so draft evidence
is appended and draftMatches is incremented only after the full message fetch
succeeds. On fetch failure, retain the truncated item for retry without
persisting it as matched or advancing the watermark; preserve normal watermark
advancement after successful processing.
- Around line 568-570: Update the watermark advancement logic around
watermarkAdvancedTo to store maxDate directly instead of maxDate + 1, preserving
the exclusive received_after pagination semantics. Revise related pagination and
truncation expectations that currently assert newest + 1 to assert newest.
In `@skills/resolve-learning-digest/handler.ts`:
- Around line 125-133: Update the undo_completion branch in the handler to
capture the result of reopenTask and verify it is non-null before calling
removeCompletionBlock or updating COMPLETION_DIGEST_PATH. If reopening returns
null, return a failure response without consuming the digest item; preserve the
existing success response only after a successful reopen.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4c728554-2c40-4f1a-821a-ade0703193a0
📒 Files selected for processing (39)
agents/ceo-inbox.yamlagents/coordinator.yamldocs/adr/029-passive-email-observation-and-counterfactual-competence.mdskills/_shared/learning-digest.test.tsskills/_shared/learning-digest.tsskills/_shared/sent-observe-match.test.tsskills/_shared/sent-observe-match.tsskills/_shared/shadow-draft.test.tsskills/_shared/shadow-draft.tsskills/_shared/task-completion-risk.test.tsskills/_shared/task-completion-risk.tsskills/_shared/voice-learning-capture.test.tsskills/_shared/voice-learning-capture.tsskills/ceo-inbox-draft-compose/handler.tsskills/ceo-inbox-draft-compose/skill.jsonskills/ceo-inbox-draft-edit/handler.tsskills/ceo-inbox-draft-edit/skill.jsonskills/ceo-inbox-draft-reply/handler.test.tsskills/ceo-inbox-draft-reply/handler.tsskills/ceo-inbox-draft-reply/skill.jsonskills/ceo-inbox-sent-observe/handler.test.tsskills/ceo-inbox-sent-observe/handler.tsskills/ceo-inbox-sent-observe/skill.jsonskills/ceo-inbox-shadow-draft/handler.test.tsskills/ceo-inbox-shadow-draft/handler.tsskills/ceo-inbox-shadow-draft/skill.jsonskills/resolve-learning-digest/handler.test.tsskills/resolve-learning-digest/handler.tsskills/task-completion-from-sent/handler.test.tsskills/task-completion-from-sent/handler.tsskills/voice-learn/handler.test.tsskills/voice-learn/handler.tssrc/autonomy/action-log-repo.tssrc/autonomy/action-log-types.tssrc/bus/events.tssrc/db/migrations/073_shadow_evaluated_outcome.sqlsrc/db/task-repo.tssrc/executive/service.tstests/integration/task-reopen-wake-revive.test.ts
💤 Files with no reviewable changes (2)
- src/bus/events.ts
- skills/ceo-inbox-shadow-draft/handler.ts
🚧 Files skipped from review as they are similar to previous changes (14)
- skills/ceo-inbox-shadow-draft/handler.test.ts
- skills/ceo-inbox-shadow-draft/skill.json
- src/db/migrations/073_shadow_evaluated_outcome.sql
- agents/coordinator.yaml
- src/db/task-repo.ts
- skills/resolve-learning-digest/handler.test.ts
- skills/_shared/sent-observe-match.test.ts
- docs/adr/029-passive-email-observation-and-counterfactual-competence.md
- skills/ceo-inbox-draft-reply/handler.test.ts
- src/executive/service.ts
- src/autonomy/action-log-repo.ts
- skills/task-completion-from-sent/handler.ts
- agents/ceo-inbox.yaml
- skills/_shared/sent-observe-match.ts
These six skills are all new in this PR and have never been released, so their first published version should be 0.1.0 rather than the pre-release bumps that accumulated during development (stacking bumps before a release is a poor pattern). Addresses CodeRabbit review on #1429. Signed-off-by: Joseph Fung <joseph@josephfung.ca>
removeCompletionBlock matched the header alone, so a same-task/kind block in a different (historical) state could be dropped in place of the live actionable one, leaving the real item to be replayed after the task mutation already ran. Require the actionable status too (undo_available / pending_confirm), matching the item parseCompletionDigest already selected. Addresses CodeRabbit on #1429. Signed-off-by: Joseph Fung <joseph@josephfung.ca>
…ffort Two data-integrity fixes to the weekly voice pass: - Batch selection took the NEWEST MAX_PAIRS (slice(-MAX_PAIRS)) then advanced the checkpoint to their newest sentAt, so with >MAX_PAIRS pending every older pair fell at/under the checkpoint and was dropped forever. Now sort ascending by sentAt and take the OLDEST MAX_PAIRS, so a backlog drains oldest-first and nothing is stranded. - ConfigStore.get/set propagate infra failures, but the surrounding comments promised best-effort. A checkpoint outage therefore aborted the run (get) or reported failure after the proposal was written (set). Wrap both in try/catch: a failed read falls back to feeding every pair; a failed write is swallowed. Addresses CodeRabbit review on #1429. Signed-off-by: Joseph Fung <joseph@josephfung.ca>
…uming undo reopenTask returns null when the task no longer exists, but the undo branch removed the digest item and reported success regardless — leaving the user with neither a reopened task nor the undo affordance. Check the result and fail closed, mirroring the confirm path's not-found guard. Addresses CodeRabbit on #1429. Signed-off-by: Joseph Fung <joseph@josephfung.ca>
…fetched The draft-diff path persisted a diff built from the truncated snippet on a getMessage failure and advanced the watermark, so the bad evidence poisoned the voice proposal and was never repaired. Apply the same Finding-7 treatment the shadow path already uses: only persist the diff and count the match after the full body is fetched, and hold the watermark (draftEvidenceComplete) so the send is re-observed next run. Also correct the watermark comments: received_after is INCLUSIVE (per the inbound email-adapter convention), so storing maxDate+1 is correct — the comments wrongly called it an exclusive lower bound. Addresses CodeRabbit review on #1429. Signed-off-by: Joseph Fung <joseph@josephfung.ca>
…g bullet Add afterEach(vi.useRealTimers()) so the faked clock can't leak into later suites in the worker. Also correct the CHANGELOG bullet that referenced snapshot linked task ids, which the simplification pass removed. Addresses CodeRabbit on #1429. Signed-off-by: Joseph Fung <joseph@josephfung.ca>
Outside-diff review commentsThese four were flagged outside the diff range so couldn't be replied inline:
All fixes are covered by unit tests; full suite is green (5105 passed, 0 failed) and typecheck is clean. |
…contract Addresses CodeRabbit review on #1429: - listAllMessages: an empty page with a lingering cursor now reports truncated so the caller can't mistake it for a drained window. - Hold the watermark when OKF evidence fails to persist, so those sends are re-observed next run instead of silently forgotten. - Wrap the whole run in the never-throw skill contract. - Dedup shadow reconciliation within a run (claimed set) so several sends in one thread can't double-score the same shadow. - Warn when the open-CEO-task fetch cap is hit (partial match set is now visible, not silent). - Declare shadow_reconciled in the manifest and return it on every path. Signed-off-by: Joseph Fung <joseph@josephfung.ca>
… contract Addresses CodeRabbit review on #1429: - isNearTotalRewrite now measures token overlap, so an unrelated rewrite of similar length is excluded (shared-prefix let it through). - Sent-body extraction uses the final block delimiter, so an email containing its own '---' line is no longer truncated mid-body. - Skip deltas the profile already reflects and stop blocking 'approved' fields, so an applied change won't re-nag yet the field keeps learning. - Apply formality/tone/patterns on the auto path too (defensive parity with the resolve path). - Wrap voice-learn and list-learning-digest in the never-throw contract. Signed-off-by: Joseph Fung <joseph@josephfung.ca>
…ck-scoped guard Addresses CodeRabbit review on #1429: - Re-validate each candidate is still open and owner='ceo' before completing (candidates can go stale after observation). - Fail closed on subtask-lookup errors (treat as high-risk) so a parent can't be auto-completed and cancel its descendants. - Scope the completion_asked guard to the candidate's own block; a document-wide check let one marker suppress every later candidate. - Drop bare 'plan' from the high-risk title heuristic ('Plan lunch' is not high-risk; 'agm' still covers the AGM case). Signed-off-by: Joseph Fung <joseph@josephfung.ca>
…bject Addresses CodeRabbit review on #1429: - scoreDecisionEquivalence now classifies affirm/deny/none and rejects opposing decisions outright — approve-vs-decline with high vocabulary overlap no longer scores competence=1 into the autonomy log. One-sided and purely-informational pairs are held to a higher bar. Fails safe. - Drop the subject from the high-sensitivity skip log (board/legal/spouse content shouldn't land in operational logs). Signed-off-by: Joseph Fung <joseph@josephfung.ca>
Addresses CodeRabbit review on #1429: linked_task_ids was rebuilt on every edit, so a routine body edit wiped the task associations captured at draft creation. Handle the field per-path — undefined preserves the existing ids, an explicit [] still clears — and have draft-edit pass undefined unless the caller supplied linked_task_ids. Signed-off-by: Joseph Fung <joseph@josephfung.ca>
…ion test Addresses CodeRabbit review on #1429: - resolve-learning-digest: require a matching actionable digest item before reopening/completing a task, and wrap in the never-throw contract (undo/confirm no longer mutate arbitrary task ids). - ceo-inbox.yaml sent-observe mode: run task-completion-from-sent after the observer, matching the cron's stated intent. - migration 073 down: delete synthetic shadow_evaluated rows before restoring the narrower CHECK, so rollback can't abort on them. - autonomy injection: single-source the predicate (AutonomyService. receivesInjection) so the test verifies production wiring, not a copy. Signed-off-by: Joseph Fung <joseph@josephfung.ca>
… substring Addresses CodeRabbit review on #1429: taskLower.includes(local) let ann@example.com match the word 'annual', pushing an unrelated task to high confidence (and possible auto-completion). Require the local-part to be a whole token. Signed-off-by: Joseph Fung <joseph@josephfung.ca>
CodeRabbit follow-up on #1429: the comment still described the reverted held-watermark behaviour. minDate is diagnostics-only (oldestScannedAt in the truncation warning); the watermark advances to maxDate + 1. Signed-off-by: Joseph Fung <joseph@josephfung.ca>
Redesigns the two heuristic email-observation engines flagged in #1429 review: voice learning becomes a free-form guide maintained by a weekly batched LLM extraction pass; shadow competence becomes an LLM judgment of substantive decision equivalence (batched, binary flag). Reuses the shared SensitivityClassifier, includes sensitive threads, exposes skill version on ctx, and edits ADR-029 in place. Signed-off-by: Joseph Fung <joseph@josephfung.ca>
These six skills are all new in this PR and have never been released, so their first published version should be 0.1.0 rather than the pre-release bumps that accumulated during development (stacking bumps before a release is a poor pattern). Addresses CodeRabbit review on #1429. Signed-off-by: Joseph Fung <joseph@josephfung.ca>
removeCompletionBlock matched the header alone, so a same-task/kind block in a different (historical) state could be dropped in place of the live actionable one, leaving the real item to be replayed after the task mutation already ran. Require the actionable status too (undo_available / pending_confirm), matching the item parseCompletionDigest already selected. Addresses CodeRabbit on #1429. Signed-off-by: Joseph Fung <joseph@josephfung.ca>
…ffort Two data-integrity fixes to the weekly voice pass: - Batch selection took the NEWEST MAX_PAIRS (slice(-MAX_PAIRS)) then advanced the checkpoint to their newest sentAt, so with >MAX_PAIRS pending every older pair fell at/under the checkpoint and was dropped forever. Now sort ascending by sentAt and take the OLDEST MAX_PAIRS, so a backlog drains oldest-first and nothing is stranded. - ConfigStore.get/set propagate infra failures, but the surrounding comments promised best-effort. A checkpoint outage therefore aborted the run (get) or reported failure after the proposal was written (set). Wrap both in try/catch: a failed read falls back to feeding every pair; a failed write is swallowed. Addresses CodeRabbit review on #1429. Signed-off-by: Joseph Fung <joseph@josephfung.ca>
…uming undo reopenTask returns null when the task no longer exists, but the undo branch removed the digest item and reported success regardless — leaving the user with neither a reopened task nor the undo affordance. Check the result and fail closed, mirroring the confirm path's not-found guard. Addresses CodeRabbit on #1429. Signed-off-by: Joseph Fung <joseph@josephfung.ca>
…fetched The draft-diff path persisted a diff built from the truncated snippet on a getMessage failure and advanced the watermark, so the bad evidence poisoned the voice proposal and was never repaired. Apply the same Finding-7 treatment the shadow path already uses: only persist the diff and count the match after the full body is fetched, and hold the watermark (draftEvidenceComplete) so the send is re-observed next run. Also correct the watermark comments: received_after is INCLUSIVE (per the inbound email-adapter convention), so storing maxDate+1 is correct — the comments wrongly called it an exclusive lower bound. Addresses CodeRabbit review on #1429. Signed-off-by: Joseph Fung <joseph@josephfung.ca>
1 participant
Give Curia a passive feedback loop from the CEO's outbound email: voice learning, task-completion detection, and shadow-draft competence feeding Phase 3 autonomy scoring (ADR-029).
Redesign (LLM-based assessment)
After the initial heuristic implementation, the voice-extraction and decision-comparison engines were replaced with LLM-based assessment. The heuristics (length/sign-off/formality scoring for voice; token/approve-deny polarity matching for decisions) were too brittle to capture substantive equivalence. Design:
docs/wip/2026-07-16-email-observation-llm-redesign-design.md, plan:docs/wip/2026-07-16-email-observation-llm-redesign.md.ctx.infraLlm.extract, batch 20) judging whether each shadow draft reached the same substantive decision/recommendation as the CEO's actual send. Writes a binarycompetence_flag(0/1) toautonomy_action_log(scored_by='shadow-reconciler'), feeding Phase 3 unchanged. Replaces the deterministic polarity scorer.WritingVoicegained a free-formguide. A weekly batched LLM pass reads accumulated(draft, sent)diffs and proposes an updated guide, queued for CEO approval via the digest (resolve-learning-digest). Never writes the profile directly. Renders into prompts viacompileWritingVoiceBlock. Replaces the heuristic scoring/threshold machinery.SensitivityClassifier(config/default.yamlsensitivity_rules) instead of hardcoded tag lists. Sensitive threads (board/investor/legal/spouse) are now included in shadow learning — the highest-stakes decisions are the most valuable signal.ctx.skillName/ctx.skillVersionexposed onSkillContext(draft skills read the manifest version instead of a hardcoded const);ceo-inbox.yaml→ v0.14.0 with a tidied autonomy-band prompt.Original epic
Implements epic #1419 and all child issues:
ceo-inbox-sent-observe+ daily cronvoice-learn+ weekly jobreopenTasklist-learning-digest/resolve-learning-digest)autonomy_action_logceo-inboxCloses #1419
Closes #1420
Closes #1421
Closes #1422
Closes #1423
Closes #1424
Closes #1425
Closes #1426
Closes #1427
Final review + fixes
A whole-branch review surfaced three issues, all fixed on-branch with tests:
## Guide Proposalblock, so on an append-only doc the loop silently broke after one approved cycle. Now scoped to the pending block; the dismiss cooldown is wired live.updated_at), so sensitive email bodies accumulated indefinitely. Bounded via a time-window trim on append plus attl_daysbackstop.Test plan
trimEvidenceDocdrops out-of-window blocks, keeps unparseable timestamps, splits on real headerscompetence_flag1 vs 0 casesSensitivityClassifier) + fuzzy-match casespnpm run typecheckclean (4 projects) + targeted vitest sweep green; CI green