refactor: split CoS task metadata.context into prompt vs note fields (#4153) - #4257
Conversation
Reviewer statusclaude — ran (1 round, effort high) against the full branch diff. Two findings, both in
Impact of both was bounded (the reader fallback to codex — UNSATISFIED. |
|
Merge held by the orchestrator: the configured review gate is only half satisfied. The configured reviewers for this run are Per the run's review contract, an unsatisfied reviewer is not a clean review and is not substituted with a self-review, so this PR is left open rather than merged. That bar is worth holding here specifically: this change carries an on-disk migration ( State: CI green on all 7 checks, The other two PRs in this swarm batch (#4253, #4254) merged — |
atomantic
left a comment
There was a problem hiding this comment.
Reviewed by /do:review — 0 critical, 4 improvements, 0 nits.
CI was green at review time. No local files were changed; this was a PR-mode review.
Out-of-diff policy note: the PR's commit metadata contains non-durable AI-session URL references. Repository policy prohibits those references in commits and other published artifacts; remove them before merge.
Generated by /do:review
| // The full agent-facing payload, when the producer names it explicitly | ||
| // (#4153). Producers that still pass a multi-line `context` are classified | ||
| // by `splitTaskPromptFields` below, so both call shapes converge. | ||
| if (taskData.prompt) metadata.prompt = taskData.prompt; |
There was a problem hiding this comment.
[IMPROVEMENT] prompt is optional, but this truthiness guard drops an intentional empty string on create. For a request such as { prompt: "", context: "keep this note" }, no metadata.prompt marker is persisted, so readers treat the note as a legacy agent payload and the UI cannot distinguish an explicit clear. Preserve present string values.
| if (taskData.prompt) metadata.prompt = taskData.prompt; | |
| if (typeof taskData.prompt === 'string') metadata.prompt = taskData.prompt; |
| it('ignores a JSON-encoded array value that happens to serialize with a newline escape', () => { | ||
| const md = queue(task('sys-1', { context: 'short note', reviewers: ['claude', 'codex'] })); | ||
| expect(splitPromptMetadata(md, { stamp: STAMP })).toEqual({ markdown: md, split: [] }); |
There was a problem hiding this comment.
[IMPROVEMENT] This test title says it exercises a JSON-encoded array in context, but the array is attached to reviewers and context remains 'short note'. The migration's non-string-context branch is therefore untested; a regression could pass. Put the array/newline-escape fixture in context.
| it('ignores a JSON-encoded array value that happens to serialize with a newline escape', () => { | |
| const md = queue(task('sys-1', { context: 'short note', reviewers: ['claude', 'codex'] })); | |
| expect(splitPromptMetadata(md, { stamp: STAMP })).toEqual({ markdown: md, split: [] }); | |
| it('ignores a JSON-encoded context array that happens to serialize with a newline escape', () => { | |
| const md = queue(task('sys-1', { context: ['line one\nline two'] })); | |
| expect(splitPromptMetadata(md, { stamp: STAMP })).toEqual({ markdown: md, split: [] }); | |
| }); |
| // Mirror firstLine() in cosTaskStore.js: first non-empty, trimmed line. | ||
| const firstNonEmpty = payload.split('\n').map(l => l.trim()).find(Boolean) || ''; | ||
| if (firstNonEmpty !== description) continue; | ||
| const { [key]: _dropped, ...restMeta } = task.metadata; |
There was a problem hiding this comment.
[IMPROVEMENT] reconcileSplitContext() removes whichever payload key matched the one-line description. On the queue path that is metadata.prompt, and buildAgentPrompt() calls this before constructing briefingTask; a customized cos-agent-briefing template using {{task.metadata.prompt}} therefore receives no prompt even though the later comment says it "travels untouched." The built-in template still works through description, making this easy to miss. Preserve the raw prompt for custom templates (or explicitly define and test the normalized queue-path contract).
| const hasPromptField = typeof task.metadata?.prompt === 'string'; | ||
| const [editData, setEditData] = useState({ | ||
| description: task.description, | ||
| prompt: task.metadata?.prompt || '', |
There was a problem hiding this comment.
[IMPROVEMENT] This useState initializer runs only on mount. TasksTab keeps TaskItem keyed by id, so a live refresh or federation update can add metadata.prompt to a mounted legacy task while editData.prompt remains ''. hasPromptField then becomes true, and saving any edit sends that stale empty prompt via the payload at line 184, clearing the agent body. Re-seed the draft when the task changes while not actively editing (or when edit mode opens), and add a regression test for this prop transition.
Summary
metadata.contexton a CoS task carried two unrelated kinds of content: a one-line human note, and a multi-thousand-character agent prompt (the generator's Phase 1–7 body, a/do:*claim prompt, a repo-study brief). The prompt landed there becausegenerateTasksMarkdownflattensdescriptiononto one line, sometadata.context— newline-escaped through the JSON sentinel — was the only field that survived theCOS-TASKS.mdround trip.The two are now separate fields:
metadata.prompt— the full agent-facing payload.metadata.context— the one-line human note.One classification rule, shared by the writer and the migration
server/lib/cosTaskPrompt.js(new, pure, dependency-free) owns the contract.isPromptPayload(value)is the single discriminator — a newline means it's a prompt, because thecontextcontract is explicitly one line — and bothcosTaskStore.addTaskand the migration call it, so a task can't be sorted one way at creation and the other way on disk. Classification happens at create only:updateTaskdeliberately leaves both fields alone, because the task editor's textarea is seeded from the note, and re-classifying a multi-line edit of it would overwrite the task's real prompt.Producers keep their existing call shape —
addTask({ context })still works and routes a multi-line body toprompt— so the reference-watch filer, the repo-study filer, the auto-fixer and the worktree-cleanup filer needed no change.cosTaskGeneratorand the two prompt-building routes (/tasks/slashdo,/tasks/jira-ticket) now name the field explicitly.Compatibility
getTaskPrompt, which prefersmetadata.promptand falls back tometadata.context. A task written before the split — or synced from a peer still on the old code — resolves exactly as it did. Under-migrating is therefore harmless.cos-agent-briefing.mdand every copy an install has customized address{{task.metadata.context}}. Rather than push a template change (and a prompt migration) onto every install,buildAgentPromptfolds prompt + note back into that key at render time.metadata.promptstill travels for a custom template that wants to address it directly.scripts/migrations/270-cos-task-prompt-split.jsrenames the prompt-carrying- context:line to- prompt:indata/TASKS.md/data/COS-TASKS.md(honouring a relocated queue path fromdata/cos/state.json), re-stampingupdatedAtso the migrated copy wins the LWW federation merge. Text-level rewrite rather than parse/regenerate, following migration 234 — regenerating would reorder, re-escape and re-sort the user's live queue. Idempotent.PORTOS_SCHEMA_VERSIONS.cosTasks4 → 5. Same execution-semantics break as the v3/v4 bumps:promptrides the permissivemetadatamap, so a ≤v4 receiver validates and stores the task fine and then mis-runs it — its prompt builder only knowsmetadata.context, so the agent gets a one-line description with the whole body missing and LWW-pushes that damaged state back. The gate makes an unmigrated peer skip cos-task sync instead.metadata.promptneeds no special case incosTaskMerge—contentSignaturealready walks every non-claim key; a test pins that it converges and that a split/legacy pair doesn't lose the payload.Side effect worth naming
cos-evaluate.md(the triage prompt) renders{{metadata.context}}per pending task. Post-split it sees the one-line note instead of an inlined multi-thousand-character body — a strict improvement for a selection prompt, and the reason no prompt migration is needed there.UI
TaskItemrenders the prompt and the note as separate clamped blocks and offers a Prompt textarea in edit mode — but only when the task actually carries one, and the key is omitted from the PATCH otherwise, so editing a legacy task can't write an emptypromptkey the markdown store would then serialize.Test plan
server/lib/cosTaskPrompt.test.js(new, 16 tests) — the discriminator, absent-vs-present-but-empty on every getter, the render block, and non-mutation of the caller's metadata.scripts/migrations/270-cos-task-prompt-split.test.js(new, 14 tests) — multi-line →prompt, one-line note left alone, JSON-sentinel and legacy\n-escaped values, JSON-encoded arrays ignored,updatedAtre-stamp/insert, idempotency, a description that spilled onto its own lines, relocated queue file, and bothup()no-op paths.cosTaskStore.test.js— direct-write and raw pre-built classification, explicitprompt+ note, no re-classification on update,promptas a direct update field (edit stamp + clear), markdown round-trip.agentPromptBuilder.test.js—reconcileSplitContextonprompt(note preserved),promptpreferred over a same-shaped legacycontext, light-path rendering of prompt + note, the swarm double-header regression re-pinned for the split shape, and the briefing-template fold (plus a legacy task left untouched).cosTaskMerge.test.js,taskParser.test.js,routes/cos.test.js,services/cos.test.js,cosTasksSync.test.jsupdated/extended.client/.../TaskItem.test.jsx— separate blocks, the Prompt textarea + PATCH, the unsaved-edits branch, and the legacy task that must not gain apromptkey.cd client && npm test→ 643 files / 7843 tests passing.cd server && npm test→ no new failures; the 55 failing files are the pre-existingrequires PostgreSQLset, a strict subset of the same run onmain(57), which has no DB available in this environment.Closes #4153