diff --git a/packages/daemon/src/lib/rpc-handlers/index.ts b/packages/daemon/src/lib/rpc-handlers/index.ts index 11c17a33a9..903be24a54 100644 --- a/packages/daemon/src/lib/rpc-handlers/index.ts +++ b/packages/daemon/src/lib/rpc-handlers/index.ts @@ -48,6 +48,7 @@ import { NodeExecutionRepository } from '../../storage/repositories/node-executi import { TaskAgentManager } from '../space/runtime/task-agent-manager'; import { ReplyRoutingRegistry } from '../space/runtime/reply-routing-registry'; import { SpaceWorktreeManager } from '../space/managers/space-worktree-manager'; +import { CodingArtifactProfile } from '../space/workflows/coding-artifact-profile'; import { setupSpaceWorkflowHandlers, checkBuiltInWorkflowDriftOnStartup, @@ -615,12 +616,22 @@ export function setupRPCHandlers(deps: RPCHandlerDependencies): RPCHandlerSetupR // Reply Routing Registry — shared between space-agent-tools (register) // and task-agent-tools / node-agent-tools (lookup). const replyRoutingRegistry = new ReplyRoutingRegistry(); + // Domain profile that owns coding-specific artifact semantics (which `link` + // is the PR, which `decision` is the terminal outcome, the review-posted-gate + // history). Injected into the runtime services so daemon core never names + // domain kinds (`pr` / `review`). + const artifactProfile = new CodingArtifactProfile({ + db: deps.db.getDatabase(), + artifactRepo, + gateDataRepo, + }); const evolutionEpisodeService = new EvolutionEpisodeService({ evolutionRepo: deps.db.evolution, spaceRepo, taskRepo: spaceTaskRepo, workflowRunRepo: spaceWorkflowRunRepo, artifactRepo, + artifactProfile, goalService: spaceGoalService, db: deps.db.getDatabase(), taskCreatedEventHub: { @@ -708,6 +719,7 @@ export function setupRPCHandlers(deps: RPCHandlerDependencies): RPCHandlerSetupR goalService: spaceGoalService, evolutionScopeService, evolutionEpisodeService, + artifactProfile, }); // When a space is resumed/started, re-seed skipped schedules and re-run restart @@ -888,6 +900,7 @@ export function setupRPCHandlers(deps: RPCHandlerDependencies): RPCHandlerSetupR goalService: spaceGoalService, evolutionScopeService, externalEventStore: deps.externalEventStore, + artifactProfile, }); deps.commandBus.register('agent.message.inject', async (command) => { diff --git a/packages/daemon/src/lib/rpc-handlers/live-query-handlers.ts b/packages/daemon/src/lib/rpc-handlers/live-query-handlers.ts index 63edc0d7d2..92c696d8e4 100644 --- a/packages/daemon/src/lib/rpc-handlers/live-query-handlers.ts +++ b/packages/daemon/src/lib/rpc-handlers/live-query-handlers.ts @@ -1217,11 +1217,19 @@ artifact_rows AS ( ELSE 'artifact' END AS category, CASE - -- Legacy notes that record blockers (mapped from unknown legacy types, - -- original meaning kept under _legacyType) warrant a warning tone. + -- Notes that record blockers/warnings warrant a warning tone. Modern + -- audit notes carry the meaning in kind (merge_blocked / merge_conflict + -- / cleanup_warning); rows backfilled from the legacy freeform type + -- system carry it under _legacyType. Match either so old and new data + -- share the tone. WHEN wra.artifact_type = 'note' AND json_valid(wra.data) - AND json_extract(wra.data, '$._legacyType') IN ('merge_blocked', 'cleanup_warning') + AND ( + json_extract(wra.data, '$.kind') IN ('merge_blocked', 'merge_conflict', 'cleanup_warning') + OR json_extract(wra.data, '$._legacyType') IN ( + 'merge_blocked', 'merge_conflict_loop', 'cleanup_warning' + ) + ) THEN 'warning' WHEN wra.artifact_type = 'note' THEN 'progress' WHEN wra.artifact_type = 'link' THEN 'success' diff --git a/packages/daemon/src/lib/space/evolution-episode-service.ts b/packages/daemon/src/lib/space/evolution-episode-service.ts index efecc232d9..b44bb50cdf 100644 --- a/packages/daemon/src/lib/space/evolution-episode-service.ts +++ b/packages/daemon/src/lib/space/evolution-episode-service.ts @@ -35,6 +35,7 @@ import type { WorkflowRunArtifactRepository, } from '../../storage/repositories/workflow-run-artifact-repository'; import type { SpaceGoalService } from './goals/goal-service'; +import type { WorkflowArtifactProfile } from './runtime/artifact-profile'; import { isRunningUnderBun, resolveSDKCliPath } from '../agent/sdk-cli-resolver'; import { Logger } from '../logger'; import { getProviderService, mergeProviderEnvVars } from '../provider-service'; @@ -119,6 +120,12 @@ export interface EvolutionEpisodeServiceDeps { taskRepo: SpaceTaskRepository; workflowRunRepo: SpaceWorkflowRunRepository; artifactRepo: WorkflowRunArtifactRepository; + /** + * Domain artifact profile. Used by the result-artifact gap detector to read + * a run's terminal outcome (coding: the kindless `decision` summary) without + * this service naming domain kinds. + */ + artifactProfile?: WorkflowArtifactProfile; goalService?: Pick; taskIdFactory?: () => string; db?: BunDatabase; @@ -482,18 +489,9 @@ export class EvolutionEpisodeService { let hasResultArtifact = runHasResultArtifact.get(runId); if (hasResultArtifact === undefined) { - // The terminal "result" is a kind-less `decision` carrying a summary - // (legacy result→decision has no kind; review/gate decisions carry a - // kind and are not terminal). - const decisions = this.deps.artifactRepo.listByRun(runId, { - artifactType: 'decision', - }); - hasResultArtifact = decisions.some( - (artifact) => - !artifact.data.kind && - typeof artifact.data.summary === 'string' && - artifact.data.summary.trim().length > 0 - ); + // Delegated to the domain artifact profile (coding: the kindless + // terminal `decision` summary). + hasResultArtifact = this.deps.artifactProfile?.summarizeRunOutcome(runId) != null; runHasResultArtifact.set(runId, hasResultArtifact); } if (!hasResultArtifact) return; diff --git a/packages/daemon/src/lib/space/runtime/artifact-profile.ts b/packages/daemon/src/lib/space/runtime/artifact-profile.ts new file mode 100644 index 0000000000..120581201a --- /dev/null +++ b/packages/daemon/src/lib/space/runtime/artifact-profile.ts @@ -0,0 +1,63 @@ +/** + * Workflow Artifact Profile — the domain seam between generic workflow infra + * (daemon core) and a domain layer. + * + * Infra knows the closed SHAPE vocabulary (`link`, `commit_set`, `check`, + * `metric`, `decision`, `note`) but never a domain KIND (`pr`, `review`, …). + * Anything that depends on what a particular kind means — which `link` is the + * run's primary URL, which `decision` is the terminal outcome, what to persist + * when a particular gate fires — lives behind this interface. The coding- + * workflow layer supplies the implementation; infra only calls the methods. + * + * All methods are best-effort: implementations log and return a safe default + * ('' / null / no-op) on error rather than throwing, so a profile failure can + * never break infra control flow. + */ + +/** + * Event passed to {@link WorkflowArtifactProfile.onGateDataCommitted} after a + * gated `send_message` commits a gate-data write (before message delivery). + */ +export interface GateDataCommittedEvent { + runId: string; + nodeId: string; + gateId: string; + /** The committed gate data (after the field merge). */ + gateData: Record; + /** + * The gate-declared fields the sender was authorized to write in this + * `send_message` (the subset of `data` that actually committed to gate_data). + * Domain hooks key side-artifacts off these, not the raw payload, so a field + * the agent sent but was not authorized to write cannot trigger a side-effect. + */ + committedData?: Record; + /** The raw `data` payload from the originating `send_message` call. */ + messageData?: Record; +} + +export interface WorkflowArtifactProfile { + /** + * Resolve the canonical "primary link" URL for a run — the single URL infra + * treats as THE link: gate-script `prUrl`, merge-template `{{pr_url}}`, and PR + * event subscriptions all read this. The coding profile knows it is the PR + * (`link kind:'pr'`); generic infra does not. Returns '' when none. + */ + resolvePrimaryLinkUrl(runId: string): string; + + /** + * Build a short terminal outcome summary from a run's artifacts, or null when + * the run recorded no outcome. Used by task completion (mark_complete / run + * completion / Forge gap detection). The coding profile reads the kindless + * terminal `decision`. + */ + summarizeRunOutcome(runId: string): string | null; + + /** + * Hook fired after a gated `send_message` commits gate data, before delivery. + * Lets a domain layer persist side-artifacts keyed to that gate. The coding + * profile records a `decision kind:'review'` (round-N) each time the + * review-posted-gate receives a `review_url`. Fire-and-forget from infra's + * perspective: errors are logged by the caller, never propagated. + */ + onGateDataCommitted?(event: GateDataCommittedEvent): Promise | void; +} diff --git a/packages/daemon/src/lib/space/runtime/post-approval-router.ts b/packages/daemon/src/lib/space/runtime/post-approval-router.ts index c6457ed5fc..1b9165aa0c 100644 --- a/packages/daemon/src/lib/space/runtime/post-approval-router.ts +++ b/packages/daemon/src/lib/space/runtime/post-approval-router.ts @@ -200,8 +200,8 @@ const POST_APPROVAL_COMPLETION_INSTRUCTIONS = `task from \`approved\` to \`done\`. If you are blocked and cannot complete the\n` + `work, do NOT call mark_complete — the post-approval node-agent surface has no\n` + `request-human tool, so surface the blocker via send_message(target="space-agent")\n` + - `and save a NON-result artifact describing the block (e.g. type:"blocked"). A\n` + - `"result" artifact would be picked up as the task result on a later mark_complete,\n` + + `and save a NON-result artifact describing the block (e.g. shape:"note", kind:"blocked"). A\n` + + `kindless \`decision\` would be picked up as the task result on a later mark_complete,\n` + `poisoning completion. Then stop.\n\n` + `Do NOT call approve_task; the task has already been approved upstream.`; diff --git a/packages/daemon/src/lib/space/runtime/space-runtime-service.ts b/packages/daemon/src/lib/space/runtime/space-runtime-service.ts index c3a1eae042..2ea44f9453 100644 --- a/packages/daemon/src/lib/space/runtime/space-runtime-service.ts +++ b/packages/daemon/src/lib/space/runtime/space-runtime-service.ts @@ -40,7 +40,7 @@ import type { SpaceWorkflowRepository } from '../../../storage/repositories/spac import type { SpaceAgentInboxRepository } from '../../../storage/repositories/space-agent-inbox-repository'; import { NodeExecutionRepository } from '../../../storage/repositories/node-execution-repository'; import { GateDataRepository } from '../../../storage/repositories/gate-data-repository'; -import { WorkflowHookStateRepository } from '../../../storage/repositories/workflow-hook-state-repository'; +import type { WorkflowArtifactProfile } from './artifact-profile'; import type { ChannelCycleRepository } from '../../../storage/repositories/channel-cycle-repository'; import type { WorkflowRunArtifactRepository } from '../../../storage/repositories/workflow-run-artifact-repository'; import type { PendingAgentMessageRepository } from '../../../storage/repositories/pending-agent-message-repository'; @@ -154,6 +154,13 @@ export interface SpaceRuntimeServiceConfig { * can resolve artifact data for script env injection. */ artifactRepo?: WorkflowRunArtifactRepository; + /** + * Domain artifact profile. Passed through to SpaceRuntime and used by this + * service's temporary ChannelRouters to resolve the run's primary link URL + * for feature scripts. Owns coding-specific semantics so neither this service + * nor SpaceRuntime names domain kinds. + */ + artifactProfile?: WorkflowArtifactProfile; /** * Optional LLM-backed workflow selector override. Passed through to * SpaceRuntime verbatim. Defaults to `selectWorkflowWithLlmDefault` which @@ -2758,72 +2765,8 @@ export class SpaceRuntimeService { * can inject PR_URL into feature scripts. */ private resolvePrUrlForRun(runId: string): string { - // Only an explicit legacy PR field (pr_url/prUrl) qualifies as a PR URL — - // never a generic data.url (which could be an issue or preview link). The - // sole exception is a `link` artifact tagged kind:'pr' (handled below). - const legacyPrUrl = (data: Record | undefined): string => - (typeof data?.prUrl === 'string' && data.prUrl) || - (typeof data?.pr_url === 'string' && data.pr_url) || - ''; - - try { - const gateDataRepo = this.config.gateDataRepo ?? new GateDataRepository(this.config.db); - const gateRecords = gateDataRepo.listByRun(runId).sort((a, b) => b.updatedAt - a.updatedAt); - for (const record of gateRecords) { - const candidate = legacyPrUrl(record.data); - if (candidate) return candidate; - } - } catch (err) { - log.warn( - `SpaceRuntimeService.resolvePrUrlForRun: failed to read gate data for run ${runId}: ${err instanceof Error ? err.message : String(err)}` - ); - } - - // Scan workflow hook state next. `pr_ready` hooks persist `pr_url` in - // localState after a successful send_message handoff (see - // workflow-hook-engine.ts); without this scan the resolver cannot find - // PR URLs for review-approval-gate when that gate's schema does not - // declare `pr_url` (the typical Review→QA handoff case). - try { - const hookStateRepo = new WorkflowHookStateRepository(this.config.db); - const hookStates = hookStateRepo - .listByRun(runId) - .sort((a, b) => (b.updatedAt ?? 0) - (a.updatedAt ?? 0)); - for (const snapshot of hookStates) { - const candidate = legacyPrUrl(snapshot.localState); - if (candidate) return candidate; - } - } catch (err) { - log.warn( - `SpaceRuntimeService.resolvePrUrlForRun: failed to read hook state for run ${runId}: ${err instanceof Error ? err.message : String(err)}` - ); - } - - if (this.config.artifactRepo) { - try { - // Return the most recently updated eligible PR candidate — a link - // kind:'pr' (data.url) or a legacy pr_url/prUrl row. - const artifacts = this.config.artifactRepo.listByRun(runId); - let best: { url: string; updatedAt: number } | null = null; - for (const a of artifacts) { - const url = - a.artifactType === 'link' && a.data.kind === 'pr' - ? typeof a.data.url === 'string' - ? a.data.url - : '' - : legacyPrUrl(a.data); - if (!url) continue; - if (!best || a.updatedAt > best.updatedAt) best = { url, updatedAt: a.updatedAt }; - } - if (best) return best.url; - } catch (err) { - log.warn( - `SpaceRuntimeService.resolvePrUrlForRun: failed to read artifacts for run ${runId}: ${err instanceof Error ? err.message : String(err)}` - ); - } - } - - return ''; + // Delegated to the domain artifact profile (coding: resolves the PR URL). + return this.config.artifactProfile?.resolvePrimaryLinkUrl(runId) ?? ''; } /** diff --git a/packages/daemon/src/lib/space/runtime/space-runtime.ts b/packages/daemon/src/lib/space/runtime/space-runtime.ts index 89020e9d52..c954b923d6 100644 --- a/packages/daemon/src/lib/space/runtime/space-runtime.ts +++ b/packages/daemon/src/lib/space/runtime/space-runtime.ts @@ -63,7 +63,7 @@ import { import { ChannelCycleRepository } from '../../../storage/repositories/channel-cycle-repository'; import { normalizeMeaningfulTaskResult } from '../task-result-utils'; import { GateDataRepository } from '../../../storage/repositories/gate-data-repository'; -import { WorkflowHookStateRepository } from '../../../storage/repositories/workflow-hook-state-repository'; +import type { WorkflowArtifactProfile } from './artifact-profile'; import type { NodeExecutionRepository } from '../../../storage/repositories/node-execution-repository'; import type { PendingAgentMessageRepository } from '../../../storage/repositories/pending-agent-message-repository'; import { SDKMessageRepository } from '../../../storage/repositories/sdk-message-repository'; @@ -226,6 +226,13 @@ export interface SpaceRuntimeConfig { * interpolation context for post-approval sessions. */ artifactRepo?: WorkflowRunArtifactRepository; + /** + * Domain artifact profile. Owns coding-specific semantics (which `link` is + * the run's PR, which `decision` is the terminal outcome) so this class never + * names domain kinds. When omitted, primary-link resolution returns '' and + * outcome summaries return undefined. + */ + artifactProfile?: WorkflowArtifactProfile; /** * Optional SDK message repository used to emit synthetic SDK messages into * a task's agent session. Defaults to a repo constructed from `db` if not @@ -4197,48 +4204,22 @@ export class SpaceRuntime { // 2. Resolve the post-approval route context (PR URL + template tokens). // - // `{{pr_url}}` in the merge template is sourced from the most recent - // `workflow_run_artifacts` row whose `data` carries `prUrl` / `pr_url`. - // The end-node reviewer persists the URL via - // `save_artifact({ type: 'result', data: { prUrl } })` immediately - // before calling `approve_task()`, so by the time we reach this branch - // the artifact row exists. We deliberately do NOT read from - // `SpaceTask`: migration 84 dropped `pr_url`/`pr_number` columns from - // `space_tasks` and moved PR metadata to the artifact store. + // `{{pr_url}}` in the merge template is sourced from the run's primary link + // URL — resolved by the domain artifact profile (coding: the PR URL across + // gate data, hook state, and artifacts). The end-node reviewer persists the + // URL (a `link kind:'pr'`) immediately before calling `approve_task()`, so + // by the time we reach this branch the artifact row exists. We deliberately + // do NOT read from `SpaceTask`: migration 84 dropped `pr_url`/`pr_number` + // columns from `space_tasks` and moved PR metadata to the artifact store. // // Callers may still override by passing `pr_url` in `contextExtras` // (RPC paths forward operator-supplied values) — their value wins // because the spread order below places `contextExtras` after the // artifact-resolved default. let resolvedPrUrl: string | undefined; - if (this.config.artifactRepo && approvedTask.workflowRunId) { - try { - const artifacts = this.config.artifactRepo.listByRun(approvedTask.workflowRunId); - // Return the most recently updated eligible PR candidate — a link - // kind:'pr' (data.url) or a legacy pr_url/prUrl row — so a newer legacy - // PR is never shadowed by an older shape link. A generic data.url never - // qualifies (it could be an issue or preview link). - const legacyPrUrl = (data: Record | undefined): string => - (typeof data?.prUrl === 'string' && data.prUrl) || - (typeof data?.pr_url === 'string' && data.pr_url) || - ''; - let best: { url: string; updatedAt: number } | null = null; - for (const a of artifacts) { - const url = - a.artifactType === 'link' && a.data.kind === 'pr' - ? typeof a.data.url === 'string' - ? a.data.url - : '' - : legacyPrUrl(a.data); - if (!url) continue; - if (!best || a.updatedAt > best.updatedAt) best = { url, updatedAt: a.updatedAt }; - } - if (best) resolvedPrUrl = best.url; - } catch (err) { - log.warn( - `dispatchPostApproval: artifact lookup failed for run ${approvedTask.workflowRunId}: ${err instanceof Error ? err.message : String(err)}` - ); - } + if (approvedTask.workflowRunId) { + resolvedPrUrl = + this.config.artifactProfile?.resolvePrimaryLinkUrl(approvedTask.workflowRunId) || undefined; } // The template interpolator (see `post-approval-template.ts`) resolves // tokens by raw identifier match — `{{autonomy_level}}` looks up the @@ -9376,71 +9357,10 @@ export class SpaceRuntime { */ private resolvePrUrlForRun(runId: string): string { - // Only an explicit legacy PR field (pr_url/prUrl) qualifies as a PR URL — - // never a generic data.url (which could be an issue or preview link). The - // sole exception is a `link` artifact tagged kind:'pr' (handled below). - const legacyPrUrl = (data: Record | undefined): string => - (typeof data?.prUrl === 'string' && data.prUrl) || - (typeof data?.pr_url === 'string' && data.pr_url) || - ''; - - try { - const gateDataRepo = this.config.gateDataRepo ?? new GateDataRepository(this.config.db); - const gateRecords = gateDataRepo.listByRun(runId).sort((a, b) => b.updatedAt - a.updatedAt); - for (const record of gateRecords) { - const candidate = legacyPrUrl(record.data); - if (candidate) return candidate; - } - } catch (err) { - log.warn( - `SpaceRuntime.resolvePrUrlForRun: failed to read gate data for run ${runId}: ${err instanceof Error ? err.message : String(err)}` - ); - } - - // Scan workflow hook state next so `pr_ready` hook state (which persists - // `pr_url` after a successful send_message) is picked up even when the - // gate schema does not declare `pr_url` (e.g. Review→QA approval gate). - try { - const hookStateRepo = new WorkflowHookStateRepository(this.config.db); - const hookStates = hookStateRepo - .listByRun(runId) - .sort((a, b) => (b.updatedAt ?? 0) - (a.updatedAt ?? 0)); - for (const snapshot of hookStates) { - const candidate = legacyPrUrl(snapshot.localState); - if (candidate) return candidate; - } - } catch (err) { - log.warn( - `SpaceRuntime.resolvePrUrlForRun: failed to read hook state for run ${runId}: ${err instanceof Error ? err.message : String(err)}` - ); - } - - if (this.config.artifactRepo) { - try { - // Return the most recently updated eligible PR candidate — a link - // kind:'pr' (data.url) or a legacy pr_url/prUrl row — so a newer legacy - // PR is never shadowed by an older shape link. - const artifacts = this.config.artifactRepo.listByRun(runId); - let best: { url: string; updatedAt: number } | null = null; - for (const a of artifacts) { - const url = - a.artifactType === 'link' && a.data.kind === 'pr' - ? typeof a.data.url === 'string' - ? a.data.url - : '' - : legacyPrUrl(a.data); - if (!url) continue; - if (!best || a.updatedAt > best.updatedAt) best = { url, updatedAt: a.updatedAt }; - } - if (best) return best.url; - } catch (err) { - log.warn( - `SpaceRuntime.resolvePrUrlForRun: failed to read artifacts for run ${runId}: ${err instanceof Error ? err.message : String(err)}` - ); - } - } - - return ''; + // Delegated to the domain artifact profile (coding: resolves the PR URL). + // Generic infra does not know which `link` is the PR, so without a profile + // there is no primary link to resolve. + return this.config.artifactProfile?.resolvePrimaryLinkUrl(runId) ?? ''; } /** @@ -9556,37 +9476,9 @@ export class SpaceRuntime { } private resolvePrimaryResultArtifactSummary(runId: string): string | undefined { - if (!this.config.artifactRepo) return undefined; - - try { - // The terminal "result" is a kind-less `decision` (the bare terminal - // form — legacy `result`→decision carries no kind; review rounds and gate - // approvals carry a kind and are not terminal). Rolling-status `note`s - // are excluded too. - const decisions = this.config.artifactRepo.listByRun(runId, { artifactType: 'decision' }); - const summaryOf = (item: { data: Record }): string => { - const s = item.data.summary; - return typeof s === 'string' ? s : ''; - }; - const isTerminal = (item: { data: Record }): boolean => - !item.data.kind && summaryOf(item).trim().length > 0; - const artifact = decisions - .map((item, index) => ({ item, index })) - .filter(({ item }) => isTerminal(item)) - .toSorted( - (a, b) => - b.item.updatedAt - a.item.updatedAt || - b.item.createdAt - a.item.createdAt || - b.index - a.index - )[0]?.item; - const summary = artifact ? summaryOf(artifact) : ''; - return summary.length > 0 ? summary : undefined; - } catch (err) { - log.warn( - `SpaceRuntime.resolvePrimaryResultArtifactSummary: failed to read artifacts for run ${runId}: ${err instanceof Error ? err.message : String(err)}` - ); - return undefined; - } + // Delegated to the domain artifact profile (coding: the kindless terminal + // `decision` summary). Returns undefined when no profile is wired. + return this.config.artifactProfile?.summarizeRunOutcome(runId) ?? undefined; } private buildTaskOutcomeUpdates( diff --git a/packages/daemon/src/lib/space/runtime/task-agent-manager.ts b/packages/daemon/src/lib/space/runtime/task-agent-manager.ts index 7b15c15310..26e576f4e5 100644 --- a/packages/daemon/src/lib/space/runtime/task-agent-manager.ts +++ b/packages/daemon/src/lib/space/runtime/task-agent-manager.ts @@ -103,6 +103,7 @@ import { ChannelResolver } from './channel-resolver'; import { ChannelRouter } from './channel-router'; import { AgentMessageRouter } from './agent-message-router'; import type { ReplyRoutingRegistry } from './reply-routing-registry'; +import type { WorkflowArtifactProfile } from './artifact-profile'; import type { AgentMemoryRepository } from '../../../storage/repositories/agent-memory-repository'; import type { EvolutionScopeService } from '../evolution-scope-service'; import { createAgentMemoryMcpServer } from '../tools/agent-memory-tools'; @@ -292,6 +293,13 @@ export interface TaskAgentManagerConfig { dbPath?: string; /** Workflow run artifact repository — for write_artifact / list_artifacts node agent tools */ artifactRepo?: WorkflowRunArtifactRepository; + /** + * Domain artifact profile. Owns coding-specific semantics (primary-link + * resolution, terminal outcome summary, gate-keyed side-artifact history) so + * this manager and the node-agent tools it spawns never name domain kinds. + * Threaded through to the node-agent tool handlers. + */ + artifactProfile?: WorkflowArtifactProfile; /** * Persistent queue of Task Agent → peer agent messages waiting for the target * session to activate. When provided, `createSubSession` flushes all pending @@ -3103,11 +3111,11 @@ export class TaskAgentManager { if (isEndNode) { if (approveUnlocked) { lines.push( - 'When your work is complete: (1) call save_artifact({ type: "result", append: true, summary: "..." }) to record the outcome, then (2) call approve_task({}) as your FINAL action to close the task. The runtime — not your artifact — decides the terminal status via completion actions.' + 'When your work is complete: (1) call save_artifact({ shape: "decision", key: "outcome", summary: "...", data: { recommendation: "completed" } }) to record the outcome, then (2) call approve_task({}) as your FINAL action to close the task. The runtime — not your artifact — decides the terminal status via completion actions.' ); } else { lines.push( - 'When your work is complete: (1) call save_artifact({ type: "result", append: true, summary: "..." }) to record the outcome, then (2) call submit_for_approval({ reason: "..." }) as your FINAL action. approve_task is NOT available at this autonomy level; only a human can finalize.' + 'When your work is complete: (1) call save_artifact({ shape: "decision", key: "outcome", summary: "...", data: { recommendation: "completed" } }) to record the outcome, then (2) call submit_for_approval({ reason: "..." }) as your FINAL action. approve_task is NOT available at this autonomy level; only a human can finalize.' ); } } @@ -4518,28 +4526,10 @@ export class TaskAgentManager { internalEventBus: this.config.internalEventBus, goalService: this.config.goalService, resolveResultArtifactSummary: (task) => { - if (!task.workflowRunId || !this.config.artifactRepo) return null; - // The terminal "result" is a kind-less `decision` (legacy result→ - // decision carries no kind; review/gate decisions carry a kind and are - // not terminal). (Rolling-status `note`s are excluded too.) - const decisions = this.config.artifactRepo.listByRun(task.workflowRunId, { - artifactType: 'decision', - }); - const summaryOf = (item: { data: Record }): string => { - const s = item.data.summary; - return typeof s === 'string' ? s : ''; - }; - const artifact = decisions - .map((item, index) => ({ item, index })) - .filter(({ item }) => !item.data.kind && summaryOf(item).trim().length > 0) - .toSorted( - (a, b) => - b.item.updatedAt - a.item.updatedAt || - b.item.createdAt - a.item.createdAt || - b.index - a.index - )[0]?.item; - const summary = artifact ? summaryOf(artifact) : ''; - return summary.length > 0 ? summary : null; + // Delegated to the domain artifact profile (coding: the kindless + // terminal `decision` summary). + if (!task.workflowRunId) return null; + return this.config.artifactProfile?.summarizeRunOutcome(task.workflowRunId) ?? null; }, }); @@ -4886,6 +4876,7 @@ export class TaskAgentManager { onSubscribeExternalEvent, onUnsubscribeExternalEvent, artifactRepo: this.config.artifactRepo, + artifactProfile: this.config.artifactProfile, taskRepo: this.config.taskRepo, auditLogRepo: this.auditLogRepo, externalEventStore: this.config.externalEventStore, @@ -5129,70 +5120,8 @@ export class TaskAgentManager { } private resolvePrUrlForRun(runId: string): string { - // Only an explicit legacy PR field (pr_url/prUrl) qualifies as a PR URL — - // never a generic data.url, which could be an issue or preview link. The - // sole exception is a `link` artifact tagged kind:'pr' (handled below). - const legacyPrUrl = (data: Record | undefined): string => - (typeof data?.prUrl === 'string' && data.prUrl) || - (typeof data?.pr_url === 'string' && data.pr_url) || - ''; - - try { - const hookStateRepo = new WorkflowHookStateRepository(this.config.db.getDatabase()); - const run = this.config.workflowRunRepo.getRun(runId); - const workflow = run ? this.config.spaceWorkflowManager.getWorkflow(run.workflowId) : null; - for (const hook of workflow?.hooks ?? []) { - if (hook.validator.kind !== 'built_in' || hook.validator.id !== 'pr_ready') continue; - const candidate = legacyPrUrl(hookStateRepo.get(runId, hook.id)?.localState); - if (candidate) return candidate; - } - } catch (err) { - log.warn( - `TaskAgentManager.resolvePrUrlForRun: failed to read hook state for run ${runId}: ${err instanceof Error ? err.message : String(err)}` - ); - } - - try { - const records = this.config.gateDataRepo?.listByRun(runId); - if (records) { - const sorted = records.sort((a, b) => b.updatedAt - a.updatedAt); - for (const record of sorted) { - const candidate = legacyPrUrl(record.data); - if (candidate) return candidate; - } - } - } catch (err) { - log.warn( - `TaskAgentManager.resolvePrUrlForRun: failed to read gate data for run ${runId}: ${err instanceof Error ? err.message : String(err)}` - ); - } - - if (this.config.artifactRepo) { - try { - // Gather every eligible PR-URL candidate — a `link` kind:'pr' (data.url) - // or a legacy row carrying pr_url/prUrl — and return the most recently - // updated, so a newer legacy PR row is never shadowed by an older shape - // link. A generic data.url on a non-pr artifact never qualifies. - const all = this.config.artifactRepo.listByRun(runId); - let best: { url: string; updatedAt: number } | null = null; - for (const a of all) { - const url = - a.artifactType === 'link' && a.data.kind === 'pr' - ? typeof a.data.url === 'string' - ? a.data.url - : '' - : legacyPrUrl(a.data); - if (!url) continue; - if (!best || a.updatedAt > best.updatedAt) best = { url, updatedAt: a.updatedAt }; - } - if (best) return best.url; - } catch (err) { - log.warn( - `TaskAgentManager.resolvePrUrlForRun: failed to read artifacts for run ${runId}: ${err instanceof Error ? err.message : String(err)}` - ); - } - } - - return ''; + // Delegated to the domain artifact profile (coding: resolves the PR URL + // across gate data, hook state, and artifacts by recency). + return this.config.artifactProfile?.resolvePrimaryLinkUrl(runId) ?? ''; } } diff --git a/packages/daemon/src/lib/space/tools/node-agent-tool-schemas.ts b/packages/daemon/src/lib/space/tools/node-agent-tool-schemas.ts index c9e0d04360..04c01c1c85 100644 --- a/packages/daemon/src/lib/space/tools/node-agent-tool-schemas.ts +++ b/packages/daemon/src/lib/space/tools/node-agent-tool-schemas.ts @@ -160,91 +160,68 @@ export type GetExternalEventInput = z.infer; * `ci`, `review`). Infra never enumerates domain kinds. * * Identity is shape-aware and derived automatically (see examples below), so a - * `note` is a single rolling status that upserts in place (no per-round growth), - * a `link` is one-per-kind, and a `decision` is single-terminal unless you pass - * an explicit multi-round `key` (e.g. 'round-0'). + * `note` is a single rolling status that upserts in place, a `link` is + * one-per-kind, and a `decision` is single-terminal — unless you pass an + * explicit multi-instance `key` (e.g. a decision 'round-0', or a note + * 'attempt-0' for a per-attempt audit trail). * * PR / preview / doc: save_artifact({ shape: 'link', kind: 'pr', data: { url, title } }) * CI / tests: save_artifact({ shape: 'check', data: { name: 'ci', status: 'pass', counts } }) * Review verdict: save_artifact({ shape: 'decision', kind:'review', data: { recommendation: 'approve', summary } }) * Multi-round history: save_artifact({ shape: 'decision', kind:'review', key: 'round-0', data: {...} }) + * Per-attempt audit: save_artifact({ shape: 'note', kind:'merge_conflict', key: 'attempt-0', data: { text } }) * Rolling status: save_artifact({ shape: 'note', data: { text: 'writing tests' } }) - * - * Legacy `type` is accepted as a deprecated alias and mapped to a shape - * (progress→note, result→decision, review→decision, pr→link) so in-flight - * agents keep working; unknown legacy types are rejected. */ -export const SaveArtifactSchema = z - .object({ - /** - * STRUCTURE — closed vocabulary. One of the values in ARTIFACT_SHAPES. - * Validated against the set; unknown values are rejected. Either `shape` - * or the legacy `type` alias must be provided. - */ - shape: z - .enum(ARTIFACT_SHAPES) - .describe( - "Structured shape from the closed set: 'link' | 'commit_set' | 'check' | 'metric' | 'decision' | 'note'. Either `shape` or legacy `type` is required." - ) - .optional(), - /** - * SEMANTIC hint (freeform, domain-extensible). Supplies the icon/label in - * the UI and folds into the identity key for `link`/`decision` so one kind - * never overwrites another. Examples: 'pr', 'issue', 'preview', 'ci', 'review'. - */ - kind: z - .string() - .min(1) - .describe( - "Semantic hint (freeform): 'pr', 'issue', 'preview', 'ci', 'review', etc. Used for the UI label/icon and folds into the identity key." - ) - .optional(), - /** - * Identity override. Defaults are derived from the shape (note→'current', - * link→kind, check/metric→name, decision→key|kind|'current'). Pass an - * explicit key only for multi-round history (e.g. decision 'round-0'). - */ - key: z - .string() - .describe( - "Identity key override. Derived from the shape by default. Pass an explicit value only for multi-round history (e.g. decision key: 'round-0')." - ) - .optional(), - /** ≤1 sentence human note. Stored under data.summary (note/decision). */ - summary: z - .string() - .describe('Short human note (≤1 sentence). Stored as data.summary for note/decision shapes.') - .optional(), - /** - * Shape-specific structured payload. Required fields depend on the shape - * (e.g. link needs `url`; check needs `name`+`status`; decision needs - * `recommendation`). Validated by save_artifact. - */ - data: z - .record(z.string(), z.unknown()) - .describe( - 'Shape-specific structured payload. Required fields vary by shape (link.url, check.name+status, decision.recommendation, etc.).' - ) - .optional(), - // ── Legacy aliases (deprecated; removed in a follow-up) ────────────────── - /** DEPRECATED — use `shape`. Legacy freeform type, mapped to a shape. */ - type: z - .string() - .describe( - 'DEPRECATED — use `shape`. Legacy freeform type; mapped to a shape (progress→note, result/review→decision, pr→link). Unknown values rejected.' - ) - .optional(), - /** DEPRECATED — use shape identity. Legacy append-only flag. */ - append: z - .boolean() - .describe( - 'DEPRECATED — use shape identity. Legacy append-only flag (inserts a new row). Ignored on the shape path.' - ) - .optional(), - }) - .refine((v) => v.shape !== undefined || v.type !== undefined, { - message: 'Either `shape` or legacy `type` is required.', - }); +export const SaveArtifactSchema = z.object({ + /** + * STRUCTURE — closed vocabulary. One of the values in ARTIFACT_SHAPES. + * Validated against the set; unknown values are rejected. Required. + */ + shape: z + .enum(ARTIFACT_SHAPES) + .describe( + "Structured shape from the closed set: 'link' | 'commit_set' | 'check' | 'metric' | 'decision' | 'note'. Required." + ), + /** + * SEMANTIC hint (freeform, domain-extensible). Supplies the icon/label in + * the UI and folds into the identity key for `link`/`decision` so one kind + * never overwrites another. Examples: 'pr', 'issue', 'preview', 'ci', 'review'. + */ + kind: z + .string() + .min(1) + .describe( + "Semantic hint (freeform): 'pr', 'issue', 'preview', 'ci', 'review', etc. Used for the UI label/icon and folds into the identity key." + ) + .optional(), + /** + * Identity override. Defaults are derived from the shape (note→'current', + * link→kind, check/metric→name, decision→key|kind|'current'). Pass an + * explicit key only for multi-round history (e.g. decision 'round-0'). + */ + key: z + .string() + .describe( + "Identity key override. Derived from the shape by default. Pass an explicit value only for multi-instance shapes — multi-round history (decision key: 'round-0') or per-attempt audit trails (note key: 'attempt-0')." + ) + .optional(), + /** ≤1 sentence human note. Stored under data.summary (note/decision). */ + summary: z + .string() + .describe('Short human note (≤1 sentence). Stored as data.summary for note/decision shapes.') + .optional(), + /** + * Shape-specific structured payload. Required fields depend on the shape + * (e.g. link needs `url`; check needs `name`+`status`; decision needs + * `recommendation`). Validated by save_artifact. + */ + data: z + .record(z.string(), z.unknown()) + .describe( + 'Shape-specific structured payload. Required fields vary by shape (link.url, check.name+status, decision.recommendation, etc.).' + ) + .optional(), +}); export type SaveArtifactInput = z.infer; @@ -446,10 +423,10 @@ export type ReadGateInput = z.infer; export const ListArtifactsSchema = z.object({ /** Filter by originating node ID. */ nodeId: z.string().describe('Filter by node ID').optional(), - /** Filter by artifact type (generic string, e.g. 'progress', 'result', 'review'). */ + /** Filter by artifact shape from the closed vocabulary (link/commit_set/check/metric/decision/note). */ type: z .string() - .describe('Filter by artifact type (e.g. "progress", "result", "review")') + .describe('Filter by artifact shape (e.g. "link", "decision", "note")') .optional(), }); diff --git a/packages/daemon/src/lib/space/tools/node-agent-tools.ts b/packages/daemon/src/lib/space/tools/node-agent-tools.ts index df16615749..da29e1df02 100644 --- a/packages/daemon/src/lib/space/tools/node-agent-tools.ts +++ b/packages/daemon/src/lib/space/tools/node-agent-tools.ts @@ -60,12 +60,9 @@ import { ARTIFACT_SHAPES, computeGateDefaults, deriveArtifactKey, - isArtifactShape, normalizeLinkData, - resolveLegacyShape, resolveNodeAgents, validateArtifactShape, - type ArtifactShape, } from '@hyperneo/shared'; import { jsonResult } from './tool-result'; import type { ToolResult } from './tool-result'; @@ -121,70 +118,10 @@ import { parseAddress } from '../../../../../messaging/src/address'; import { translateLegacyNodeTargets } from '../messaging-adapter'; import { getEffectiveGate, hasInjectedGateFeature } from '../runtime/gate-features'; import { buildPrEventTopicPattern, parsePrUrl } from '../runtime/parse-pr-url'; +import type { WorkflowArtifactProfile } from '../runtime/artifact-profile'; import type { WorkflowHookEngine } from '../runtime/workflow-hook-engine'; import { wrapHandlerWithHooks } from '../runtime/workflow-hook-engine'; -/** - * Resolves the most recent PR URL for a workflow run by scanning gate - * data records and artifacts, sorted by recency. A PR is a `link` shape tagged - * kind:'pr' whose `data.url` carries the URL; legacy rows / gate data may still - * carry the snake/camel `pr_url`/`prUrl` fields. The generic fallback NEVER - * accepts an arbitrary `data.url` (which could be an issue or preview link) — - * only link kind:'pr' or an explicit legacy PR field qualifies. - */ -function resolvePrUrlForRun( - gateDataRepo: GateDataRepository, - artifactRepo: WorkflowRunArtifactRepository | undefined, - runId: string -): string { - const legacyPrUrl = (data: Record | undefined): string => - (typeof data?.prUrl === 'string' && data.prUrl) || - (typeof data?.pr_url === 'string' && data.pr_url) || - ''; - - try { - const records = gateDataRepo.listByRun(runId); - if (records) { - const sorted = records.sort((a, b) => b.updatedAt - a.updatedAt); - for (const record of sorted) { - const candidate = legacyPrUrl(record.data); - if (candidate) return candidate; - } - } - } catch { - // ignore - } - - if (artifactRepo) { - try { - const artifacts = artifactRepo.listByRun(runId); - if (artifacts) { - // Gather every eligible PR-URL candidate — a `link` kind:'pr' (read via - // data.url) or a legacy row carrying pr_url/prUrl — and return the most - // recently updated, so a newer legacy PR row is never shadowed by an - // older shape link (and vice versa). A generic data.url on a non-pr - // artifact never qualifies. - let best: { url: string; updatedAt: number } | null = null; - for (const a of artifacts) { - const url = - a.artifactType === 'link' && a.data.kind === 'pr' - ? typeof a.data.url === 'string' - ? a.data.url - : '' - : legacyPrUrl(a.data); - if (!url) continue; - if (!best || a.updatedAt > best.updatedAt) best = { url, updatedAt: a.updatedAt }; - } - if (best) return best.url; - } - } catch { - // ignore - } - } - - return ''; -} - /** * Decode the JSON payload from a ToolResult created by jsonResult(). * Returns the parsed object or null if parsing fails. @@ -217,13 +154,14 @@ export async function evaluateTerminalGateFeatures( scriptExecutor?: GateScriptExecutorFn, scriptContext?: GateScriptExecutorContext, currentNodeId?: string, - artifactRepo?: WorkflowRunArtifactRepository + artifactProfile?: WorkflowArtifactProfile ): Promise { if (!workflow || !scriptExecutor || !scriptContext) return null; - // Resolve freshest PR URL for this run so terminal checks evaluate against - // the correct PR even when it was written after the MCP server was created. - const freshPrUrl = resolvePrUrlForRun(gateDataRepo, artifactRepo, workflowRunId); + // Resolve freshest primary link URL for this run so terminal checks evaluate + // against the correct PR even when it was written after the MCP server was + // created. Delegated to the domain profile (coding: the PR URL). + const freshPrUrl = artifactProfile?.resolvePrimaryLinkUrl(workflowRunId) ?? ''; // Scope terminal checks to gates on channels connected to the current node. // Outgoing channels (from current node) are always included. Incoming channels @@ -477,6 +415,12 @@ export interface NodeAgentToolsConfig { * Optional — when absent, artifact tools are not registered. */ artifactRepo?: WorkflowRunArtifactRepository; + /** + * Domain artifact profile. Owns coding-specific semantics (primary-link + * resolution, terminal outcome summary, gate-keyed side-artifact history) so + * these handlers never name domain kinds. Threaded from TaskAgentManager. + */ + artifactProfile?: WorkflowArtifactProfile; /** * Task repository for list_tasks and get_task tools. * Optional — when absent, task read tools are not registered. @@ -700,8 +644,8 @@ export function createNodeAgentToolHandlers(config: NodeAgentToolsConfig) { * Returns completionState per peer: execution status, latest progress summary, and completedAt. * Returns nodeCompletionState: all executions on this workflow node with their completion state. * - * Progress summary is sourced from the latest 'progress' type artifact for the node - * (written via save_artifact({ type: 'progress', ... })). Falls back to ne.result for + * Progress summary is sourced from the latest `note` artifact for the node + * (written via save_artifact({ shape: 'note', ... })). Falls back to ne.result for * historical rows that predate the artifact migration. */ async list_peers(_args: ListPeersInput): Promise { @@ -1080,11 +1024,8 @@ export function createNodeAgentToolHandlers(config: NodeAgentToolsConfig) { ) : gateDataRepo.merge(workflowRunId, gateId, partialToMerge); const updatedRecord = gateDataRepo.get(workflowRunId, gateId); - const freshPrUrl = resolvePrUrlForRun( - gateDataRepo, - config.artifactRepo, - workflowRunId - ); + const freshPrUrl = + config.artifactProfile?.resolvePrimaryLinkUrl(workflowRunId) ?? ''; const evalResult = await evaluateGate( getEffectiveGate(gateDef, workflow, gatedChannel.from), updated.data, @@ -1103,62 +1044,29 @@ export function createNodeAgentToolHandlers(config: NodeAgentToolsConfig) { ); gateWriteResult = { gateId, gateOpen: evalResult.open }; - // Multi-round review history: every time the reviewer writes a - // `review_url` to this gate, persist one `decision` (kind:review) - // artifact per cycle (round-0, round-1 …) keyed so each round is a - // distinct upsert. Persist this before any rate-limited early return - // so the review record is not lost when the gate script is blocked. - if ( - config.artifactRepo && - gateId === 'review-posted-gate' && - typeof authorizedData.review_url === 'string' && - authorizedData.review_url.length > 0 - ) { + // Domain profile hook: let the coding layer persist any + // gate-keyed side-artifacts (e.g. a multi-round review decision + // when a review gate fires). Fired before any rate-limited early + // return so the record is not lost when the gate script is + // blocked. Infra knows neither the gate id nor the kind — only + // the profile does. + if (config.artifactProfile?.onGateDataCommitted) { try { - const decisions = config.artifactRepo.listByRun(workflowRunId, { - artifactType: 'decision', - }); - // Next round = one past the highest existing review-round - // number, derived from the trailing digits of each review - // decision's key (handles sparse keys and both legacy - // 'cycle-N' and namespaced 'review:round-N' forms). This - // never overwrites an existing review round. - let maxCycle = -1; - for (const a of decisions) { - if (a.data.kind !== 'review') continue; - const m = /(\d+)$/.exec(a.artifactKey); - if (m) maxCycle = Math.max(maxCycle, Number.parseInt(m[1], 10)); - } - const cycle = maxCycle + 1; - const artifactData: Record = { - recommendation: 'reviewed', - kind: 'review', - review_url: authorizedData.review_url, - cycle, - submittedAt: new Date().toISOString(), - }; - const rawCommentUrls = (data as Record).comment_urls; - if ( - Array.isArray(rawCommentUrls) && - rawCommentUrls.every((u) => typeof u === 'string') - ) { - artifactData.comment_urls = rawCommentUrls; - } - config.artifactRepo.upsert({ - id: crypto.randomUUID(), + await config.artifactProfile.onGateDataCommitted({ runId: workflowRunId, nodeId: workflowNodeId, - artifactType: 'decision', - artifactKey: deriveArtifactKey( - 'decision', - { kind: 'review' }, - `round-${cycle}` - ), - data: artifactData, + gateId, + gateData: updated.data, + // The gate-declared fields the sender was authorized to + // write in this send — the committed write, not the raw + // payload, so a non-authorizable field can't trigger a + // domain side-artifact. + committedData: authorizedData, + messageData: data, }); } catch (err) { log.warn( - `Failed to append review artifact for run "${workflowRunId}":`, + `onGateDataCommitted failed for gate "${gateId}" in run "${workflowRunId}":`, err instanceof Error ? err.message : String(err) ); } @@ -1508,7 +1416,7 @@ export function createNodeAgentToolHandlers(config: NodeAgentToolsConfig) { // Evaluate current gate status. Uses scriptExecutor when available for // async script-based gates; otherwise falls back to field-only evaluation. - const freshPrUrl = resolvePrUrlForRun(gateDataRepo, config.artifactRepo, workflowRunId); + const freshPrUrl = config.artifactProfile?.resolvePrimaryLinkUrl(workflowRunId) ?? ''; const sourceName = resolveCurrentGateSource(gateId); const evalResult = await evaluateGate( getEffectiveGate(gateDef, workflow ?? undefined, sourceName), @@ -1550,10 +1458,6 @@ export function createNodeAgentToolHandlers(config: NodeAgentToolsConfig) { * per kind, check/metric→name, decision→key|kind|'current'), so repeated * status updates overwrite in place instead of accumulating per round. * - * The legacy `type` param is accepted as a deprecated alias and mapped to a - * shape (progress→note, result/review→decision, pr→link) so in-flight agents - * keep working; unknown legacy types are rejected. - * * Requires `artifactRepo` to be provided in the config. */ async save_artifact(args: SaveArtifactInput): Promise { @@ -1562,60 +1466,8 @@ export function createNodeAgentToolHandlers(config: NodeAgentToolsConfig) { return jsonResult({ success: false, error: 'Artifact repository not available.' }); } - const { shape: shapeArg, type, kind, key: keyArg, append, summary, data } = args; - - // Resolve the shape. The new `shape` param wins. The legacy `type` alias is - // then mapped (data-aware, since `result` was overloaded): - // - a shape NAME passed as `type` → treated as a new shape call (validated); - // - a known legacy type (progress/result/review/pr) → mapped to a shape, - // bypassing strict validation since it predates the contracts; - // - any other freeform type (e.g. merge_conflict_loop, merge_blocked, - // cleanup_warning from active post-approval prompts) → accepted as a - // `note` tagged with the original type, so the write still records state. - let shape: ArtifactShape | undefined; - let legacyAppend = false; - let isLegacy = false; - let isUnknownLegacy = false; - // Legacy `pr`→link and `review`→decision carry an implicit kind so PR - // readers and round counters find them without an explicit kind arg. - let legacyKind: string | undefined; - if (shapeArg !== undefined) { - shape = shapeArg; - } else if (type !== undefined) { - legacyAppend = append === true; - if (isArtifactShape(type)) { - // A shape name passed via the legacy alias is NOT a legacy semantic - // type — validate it like a normal shape call (no bypass). - shape = type; - } else { - const provisional: Record = {}; - if (summary !== undefined) provisional.summary = summary; - if (data !== undefined) Object.assign(provisional, data); - const mapped = resolveLegacyShape(type, provisional); - if (!mapped) { - // Unknown freeform type: accept as a note so active prompts that - // predate the shape vocabulary (post-approval audit/blocker writes) - // keep recording state rather than erroring. - isLegacy = true; - isUnknownLegacy = true; - shape = 'note'; - } else { - isLegacy = true; - shape = mapped; - if (type === 'pr') { - legacyKind = 'pr'; - } else if (type === 'review') { - legacyKind = 'review'; - } else if (shape === 'link') { - // Legacy `result`-with-URL routed to a link: infer the kind from - // the URL field so PR readers and the link identity key pick it up. - const d = (data as Record | undefined) ?? {}; - if (typeof d.pr_url === 'string' || typeof d.prUrl === 'string') legacyKind = 'pr'; - else if (typeof d.review_url === 'string') legacyKind = 'review'; - } - } - } - } + const { shape, kind, key: keyArg, summary, data } = args; + if (!shape) { return jsonResult({ success: false, @@ -1623,17 +1475,13 @@ export function createNodeAgentToolHandlers(config: NodeAgentToolsConfig) { }); } - // Merge summary + data into a single payload, then fold in the kind hint - // (explicit kind wins over the legacy implicit kind). Unknown legacy types - // keep their original type under _legacyType for traceability. + // Merge summary + data into a single payload, then fold in the kind hint. const artifactData: Record = {}; if (summary !== undefined) artifactData.summary = summary; if (data !== undefined) Object.assign(artifactData, data); - const effectiveKind = kind ?? legacyKind; - if (effectiveKind !== undefined) artifactData.kind = effectiveKind; - if (isUnknownLegacy && type !== undefined) artifactData._legacyType = type; - // Legacy link rows may carry pr_url/review_url instead of url — normalise - // so link readers (which key off data.url) find the URL. + if (kind !== undefined) artifactData.kind = kind; + // Link rows may carry a URL-bearing field under a domain key; normalise + // onto data.url so link readers (which key off data.url) find it. const normalized = shape === 'link' ? normalizeLinkData(artifactData) : artifactData; if (Object.keys(normalized).length === 0) { @@ -1643,25 +1491,14 @@ export function createNodeAgentToolHandlers(config: NodeAgentToolsConfig) { }); } - // Validate the payload against the per-shape contract (new shape calls - // only — legacy callers bypass, since they predate the contracts). - if (!isLegacy) { - const validation = validateArtifactShape(shape, normalized); - if (!validation.ok) { - return jsonResult({ success: false, error: validation.error }); - } + // Validate the payload against the per-shape contract. + const validation = validateArtifactShape(shape, normalized); + if (!validation.ok) { + return jsonResult({ success: false, error: validation.error }); } try { - // Identity: legacy append mode forces a unique key (new row); an unknown - // legacy type uses the original type as its key so distinct unknown types - // don't collapse into the single rolling 'current' note; otherwise the - // key is derived from the shape so like shapes upsert in place. - const artifactKey = legacyAppend - ? `${Date.now()}-${Math.random().toString(36).slice(2)}` - : isUnknownLegacy - ? (type as string) - : deriveArtifactKey(shape, normalized, keyArg); + const artifactKey = deriveArtifactKey(shape, normalized, keyArg); const record = artifactRepo.upsert({ id: crypto.randomUUID(), @@ -1676,7 +1513,6 @@ export function createNodeAgentToolHandlers(config: NodeAgentToolsConfig) { shape, kind: kind ?? undefined, key: artifactKey, - legacyType: type ?? undefined, summary: summary ?? undefined, dataKeys: data ? Object.keys(data) : undefined, }); @@ -1704,58 +1540,13 @@ export function createNodeAgentToolHandlers(config: NodeAgentToolsConfig) { return jsonResult({ success: false, error: 'Artifact repository not available.' }); } try { - // Map a legacy freeform `type` filter to its shape set so in-flight - // agents that still ask for { type: 'result' } see their migrated rows - // (result→decision|link, progress→note, review→decision, pr→link). - const filterShapes = (t: string | undefined): string[] | undefined => { - if (!t) return undefined; - if (isArtifactShape(t)) return [t]; - switch (t) { - case 'result': - return ['decision', 'link']; - case 'progress': - return ['note']; - case 'review': - return ['decision']; - case 'pr': - return ['link']; - default: - return [t]; - } - }; - const shapes = filterShapes(args.type); - // Legacy `pr`→link and `review`→decision map to a shape that also holds - // unrelated kinds (issue/preview/doc links, gate decisions), so the shape - // filter alone over-returns. Post-filter on the legacy semantic kind; - // `result` (overloaded: decision|link) and `progress` stay unfiltered, as - // do direct shape-name queries (type:'link') and unknown types. - const kindFilter = args.type === 'pr' || args.type === 'review' ? args.type : undefined; - let artifacts: ReturnType; - if (!shapes || shapes.length <= 1) { - artifacts = artifactRepo.listByRun(workflowRunId, { - nodeId: args.nodeId, - artifactType: shapes?.[0], - }); - } else { - const seen = new Set(); - const merged: Array<(typeof artifacts)[number]> = []; - for (const s of shapes) { - for (const a of artifactRepo.listByRun(workflowRunId, { - nodeId: args.nodeId, - artifactType: s, - })) { - if (!seen.has(a.id)) { - seen.add(a.id); - merged.push(a); - } - } - } - // listByRun orders ASC by created_at; re-sort the merged set to match. - artifacts = merged.sort((a, b) => a.createdAt - b.createdAt); - } - if (kindFilter) { - artifacts = artifacts.filter((a) => a.data.kind === kindFilter); - } + // `type` filters by the canonical SHAPE vocabulary (link / commit_set / + // check / metric / decision / note); artifacts are always stored as a + // shape, so a single filter value is enough. + const artifacts = artifactRepo.listByRun(workflowRunId, { + nodeId: args.nodeId, + artifactType: args.type, + }); return jsonResult({ success: true, artifacts: artifacts.map((a) => ({ @@ -1878,7 +1669,7 @@ export function createNodeAgentToolHandlers(config: NodeAgentToolsConfig) { }); } const prUrl = - args.prUrl || resolvePrUrlForRun(gateDataRepo, config.artifactRepo, workflowRunId); + args.prUrl || config.artifactProfile?.resolvePrimaryLinkUrl(workflowRunId) || ''; const parsed = prUrl ? parsePrUrl(prUrl) : null; if (!parsed) { return jsonResult({ @@ -1958,7 +1749,7 @@ export function createNodeAgentToolHandlers(config: NodeAgentToolsConfig) { scriptExecutor, scriptContext, workflowNodeId, - config.artifactRepo + config.artifactProfile ); if (gateBlock) return gateBlock; @@ -2190,7 +1981,7 @@ export function createNodeAgentMcpServer(config: NodeAgentToolsConfig) { config.scriptExecutor, config.scriptContext, config.workflowNodeId, - config.artifactRepo + config.artifactProfile ); if (gateBlock) return gateBlock; return config.onSubmitForApproval!(args); @@ -2342,8 +2133,7 @@ export function createNodeAgentMcpServer(config: NodeAgentToolsConfig) { '`note` is a single rolling-status upsert; `link` is one per kind; `check`/`metric` keyed by name; ' + '`decision` is single-terminal or multi-round via `key`. Save structured facts (PR/preview/doc → ' + 'link, CI/tests → check, review verdict → decision, current status → note), NOT a re-narration of ' + - 'the thread. The legacy `type` param is accepted as a compatibility alias (progress→note, ' + - 'result→decision|link, review→decision, pr→link) but `shape` is preferred.', + 'the thread. Keep prose in chat; only structured facts belong here.', SaveArtifactSchema.shape, (args) => handlers.save_artifact(args) ), diff --git a/packages/daemon/src/lib/space/workflows/built-in-workflows.ts b/packages/daemon/src/lib/space/workflows/built-in-workflows.ts index 0d5cf1280f..00da309e44 100644 --- a/packages/daemon/src/lib/space/workflows/built-in-workflows.ts +++ b/packages/daemon/src/lib/space/workflows/built-in-workflows.ts @@ -337,9 +337,9 @@ const PD_TASK_DISPATCHER_PROMPT = ' ```\n\n' + '4. Collect the returned task IDs. Build a stack map: ' + '{ prefix, items: [{ title, task_id, branch, base_branch, position }] }.\n' + - '5. Call `save_artifact({ type: "result", append: true, summary: "Created N tasks from plan: ", ' + - 'created_task_ids: [], stack_prefix: "", ' + - 'stack_branches: ["plan//", "plan//", ...] })` to record the dispatch audit entry.\n' + + '5. Call `save_artifact({ shape: "decision", summary: "Created N tasks from plan: ", ' + + 'data: { recommendation: "dispatched", created_task_ids: [], stack_prefix: "", ' + + 'stack_branches: ["plan//", "plan//", ...] } })` to record the dispatch outcome.\n' + '6. Call `approve_task()` as your final action. If autonomy blocks self-close, call ' + '`submit_for_approval({ reason: "..." })` instead.\n\n' + 'CRITICAL: Do NOT create branches, make commits, push to git, or open PRs yourself — ' + @@ -603,12 +603,13 @@ export const CODING_WORKFLOW: SpaceWorkflow = { 'before releasing your message. If you skip `gh pr review`, the hook will block ' + 'and the coder will never hear from you.\n\n' + reviewerFeedbackProcedure('Coding') + - 'Use save_artifact every cycle. Nest pr_url inside artifact data for post-approval dispatch.\n\n' + + 'Use save_artifact every cycle to record the PR as a `link` so post-approval dispatch ' + + 'can resolve it.\n\n' + 'Review checklist: inspect PR diff and related worktree context, run tests if uncertain, ' + 'post visible GitHub review before sending feedback. If changes needed, include pr_url, ' + 'review_url, and comment_urls when messaging Coding. If approved, ' + REVIEW_THREAD_APPROVAL_CHECK_GUIDANCE + - ' Call save_artifact({ type: "result", data: { pr_url: "" } }) then approve_task() or submit_for_approval. ' + + ' Call save_artifact({ shape: "link", kind: "pr", data: { url: "" } }) then approve_task() or submit_for_approval. ' + 'Do NOT attempt to merge the PR yourself. Do not set auto-merge.' + REVIEWER_POST_APPROVAL_BLOCKER_PARAGRAPH, }, @@ -793,12 +794,13 @@ export const RESEARCH_WORKFLOW: SpaceWorkflow = { 'You are the Reviewer in a Research→Reviewer iterative workflow. You review the ' + 'research findings for completeness, accuracy, and quality.\n\n' + reviewerFeedbackProcedure('Research') + - 'Use save_artifact every cycle. Nest pr_url inside artifact data for post-approval dispatch.\n\n' + + 'Use save_artifact every cycle to record the PR as a `link` so post-approval dispatch ' + + 'can resolve it.\n\n' + 'Review checklist: read all research docs in the PR, verify completeness, evidence, ' + 'accuracy, and clarity. If more research is needed, message Research with specific ' + 'areas to investigate and stop. If satisfied, post approval review, ' + REVIEW_THREAD_APPROVAL_CHECK_GUIDANCE + - ' Call save_artifact({ type: "result", data: { pr_url: "" } }) then approve_task() or submit_for_approval. ' + + ' Call save_artifact({ shape: "link", kind: "pr", data: { url: "" } }) then approve_task() or submit_for_approval. ' + 'Do NOT attempt to merge the PR yourself. Do not set auto-merge.' + REVIEWER_POST_APPROVAL_BLOCKER_PARAGRAPH, }, @@ -919,7 +921,7 @@ export const REVIEW_ONLY_WORKFLOW: SpaceWorkflow = { 'You are the sole Reviewer in a single-node Review-Only workflow. Review an existing ' + 'PR or codebase directly. Follow the Reviewer System Contract and terminal-action tool ' + 'contract: post a visible GitHub review (`gh pr review`) before terminal actions; ' + - 'call save_artifact({ type: "result", data: { pr_url: "" } }) to save a result artifact, then approve_task() or submit_for_approval only on APPROVE, otherwise stop. ' + + 'call save_artifact({ shape: "link", kind: "pr", data: { url: "" } }) to record the PR, then approve_task() or submit_for_approval only on APPROVE, otherwise stop. ' + 'Do NOT attempt to merge the PR yourself. Never set a PR to auto-merge.', }, }, @@ -951,7 +953,7 @@ export const REVIEW_ONLY_WORKFLOW: SpaceWorkflow = { * Plan Review → Planning (revision requests, maxCycles: 5) * * Task Dispatcher (end node) creates follow-up tasks via `create_standalone_task` - * and calls `save_artifact({ type: 'result', append: true, created_task_ids })` + * and calls `save_artifact({ shape: 'decision', data: { created_task_ids } })` * before `approve_task()` closes the run. */ export const PLAN_AND_DECOMPOSE_WORKFLOW: SpaceWorkflow = { @@ -1083,7 +1085,7 @@ export const PLAN_AND_DECOMPOSE_WORKFLOW: SpaceWorkflow = { '\n\n' + 'Expected inputs: An approved plan PR (all 4 reviewers sent approved votes).\n' + 'Expected outputs: One standalone task per actionable work item in the plan, ' + - 'then save_artifact({ type: "result", append: true, created_task_ids: [...] }).\n\n' + + 'then save_artifact({ shape: "decision", summary: "Dispatched N tasks", data: { recommendation: "dispatched", created_task_ids: [...] } }).\n\n' + 'Tool contract:\n' + "- `create_standalone_task` is available from the space's MCP server and " + 'creates a task owned by the same space as this workflow.', @@ -1254,16 +1256,19 @@ export const FULLSTACK_QA_LOOP_WORKFLOW: SpaceWorkflow = { '5. If `ui_changed` is true, start HyperNeo with `make dev PORT= DB_PATH=/tmp/hyperneo-qa-.db` and exercise the changed flow in a browser (golden path, relevant edge cases, nearby regressions)\n' + '6. Validate CI and mergeability\n' + '7. If fail: send detailed failures and repro steps to Coding, then call ' + - '`save_artifact({ type: "result", append: true, summary: "QA failed: ..." })` to record the audit entry. Do ' + + '`save_artifact({ shape: "note", kind: "qa", key: "cycle-", summary: "QA failed (cycle ): ..." })` to record the audit entry — a note, never a terminal decision, and keyed per cycle ( = this QA round, 1-based) so each failure cycle keeps its own repro evidence instead of overwriting the last. Do ' + 'NOT call `approve_task` or `submit_for_approval` — both are TERMINAL and ' + 'carry the same approval semantic. Leave the workflow open for the next ' + 'Coding cycle.\n' + '8. If all green:\n' + - ' a. Call `save_artifact({ type: "result", append: true, summary, data: { pr_url: "", test_output: "", ui_changed: , dev_server_started: , browser_validation: "" } })` ' + - 'to record the audit entry. The `pr_url` inside `data` is what ' + - '`dispatchPostApproval` reads when interpolating `{{pr_url}}` into the ' + - 'merge template — top-level keys outside `data` are silently stripped by ' + - 'the tool schema, so nest it correctly.\n' + + ' a. Record the PR and the terminal QA outcome as two artifacts: ' + + '`save_artifact({ shape: "link", kind: "pr", data: { url: "" } })` ' + + '(the canonical PR record the post-approval merge step resolves as the ' + + 'primary link) and `save_artifact({ shape: "decision", summary, data: { ' + + 'recommendation: "pass", test_output: "", ui_changed: , dev_server_started: , ' + + 'browser_validation: "" } })` (the terminal ' + + 'outcome summary). Top-level keys outside `data` are silently stripped by the ' + + 'tool schema, so nest fields correctly.\n' + ' b. Call `approve_task()` as your final action. If autonomy blocks self-close, ' + 'call `submit_for_approval({ reason: "..." })` instead — the runtime will ' + 'still route post-approval once the human approves. Do NOT run `gh pr merge` ' + @@ -1873,6 +1878,65 @@ const RETIRED_CODEX_REACTION_APPROVAL_GUIDANCE = 'only with a warning recorded in your result artifact. Do not close the task ' + 'before codex[bot] has `+1` unless that timeout has elapsed.'; +// type→shape save_artifact API migration. `dev` shipped the seeded built-in +// prompts on the legacy freeform-type API; the shape cutover rewrote each call +// site (and expanded the QA all-green step from one result call into a link + +// decision pair). Each pair is `[currentShapeText, retiredTypeResultText]`; +// `buildRetiredBuiltInPromptValues` reverse-applies them (current→retired) to +// recognize a persisted dev-era prompt and swap it to the current template. Only +// an EXACT retired variant swaps, so operator customizations to surrounding +// prose are preserved (a customized prompt that no longer matches a retired +// variant is left untouched). +const SHAPE_PR_LINK = 'save_artifact({ shape: "link", kind: "pr", data: { url: "" } })'; +const RETIRED_TYPE_RESULT_PR_LINK = 'save_artifact({ type: "result", data: { pr_url: "" } })'; +// Coding + Research reviewer prompts also rewrote the sentence preceding the +// PR-link call ("Nest pr_url inside artifact data…" → "record the PR as a +// link…"). Reversing the call alone leaves the new sentence, so the generated +// variant would not match the real dev prompt — pair the sentence with the call +// so the whole region reconstructs the dev-era reviewer prompt. +const SHAPE_PR_EVERY_CYCLE = + 'Use save_artifact every cycle to record the PR as a `link` so post-approval dispatch can resolve it.\n\n'; +const RETIRED_TYPE_RESULT_EVERY_CYCLE = + 'Use save_artifact every cycle. Nest pr_url inside artifact data for post-approval dispatch.\n\n'; +// Review-only reviewer prompt also changed the trailing prose ("to save a result +// artifact" → "to record the PR"), so its pair carries that tail to stay exact. +const SHAPE_PR_LINK_REVIEW_ONLY = + 'save_artifact({ shape: "link", kind: "pr", data: { url: "" } }) to record the PR'; +const RETIRED_TYPE_RESULT_PR_LINK_REVIEW_ONLY = + 'save_artifact({ type: "result", data: { pr_url: "" } }) to save a result artifact'; +const SHAPE_DECISION_DISPATCHER_STACK = + 'save_artifact({ shape: "decision", summary: "Created N tasks from plan: ", ' + + 'data: { recommendation: "dispatched", created_task_ids: [], stack_prefix: "", ' + + 'stack_branches: ["plan//", "plan//", ...] } })` to record the dispatch outcome'; +const RETIRED_TYPE_RESULT_DISPATCHER_STACK = + 'save_artifact({ type: "result", append: true, summary: "Created N tasks from plan: ", ' + + 'created_task_ids: [], stack_prefix: "", ' + + 'stack_branches: ["plan//", "plan//", ...] })` to record the dispatch audit entry'; +const SHAPE_DECISION_DISPATCHER_SHORT = + 'save_artifact({ shape: "decision", summary: "Dispatched N tasks", data: { recommendation: "dispatched", created_task_ids: [...] } })'; +const RETIRED_TYPE_RESULT_DISPATCHER_SHORT = + 'save_artifact({ type: "result", append: true, created_task_ids: [...] })'; +const SHAPE_NOTE_QA_FAILED = + '`save_artifact({ shape: "note", kind: "qa", key: "cycle-", summary: "QA failed (cycle ): ..." })` to record the audit entry — a note, never a terminal decision, and keyed per cycle ( = this QA round, 1-based) so each failure cycle keeps its own repro evidence instead of overwriting the last. Do '; +const RETIRED_TYPE_RESULT_QA_FAILED = + '`save_artifact({ type: "result", append: true, summary: "QA failed: ..." })` to record the audit entry. Do '; +const SHAPE_QA_ALL_GREEN = + 'a. Record the PR and the terminal QA outcome as two artifacts: ' + + '`save_artifact({ shape: "link", kind: "pr", data: { url: "" } })` ' + + '(the canonical PR record the post-approval merge step resolves as the ' + + 'primary link) and `save_artifact({ shape: "decision", summary, data: { ' + + 'recommendation: "pass", test_output: "", ui_changed: , dev_server_started: , ' + + 'browser_validation: "" } })` (the terminal ' + + 'outcome summary). Top-level keys outside `data` are silently stripped by the ' + + 'tool schema, so nest fields correctly.\n'; +const RETIRED_TYPE_RESULT_QA_ALL_GREEN = + 'a. Call `save_artifact({ type: "result", append: true, summary, data: { ' + + 'pr_url: "", test_output: "", ui_changed: , dev_server_started: , ' + + 'browser_validation: "" } })` to record the audit entry. The ' + + '`pr_url` inside `data` is what `dispatchPostApproval` reads when interpolating `{{pr_url}}` into the ' + + 'merge template — top-level keys outside `data` are silently stripped by the tool schema, so nest it ' + + 'correctly.\n'; + const BUILT_IN_PROMPT_PATCH_VARIANTS = [ [[REVIEW_THREAD_RESOLUTION_GUIDANCE, RETIRED_REVIEW_THREAD_RESOLUTION_GUIDANCE]], [ @@ -1969,6 +2033,22 @@ const BUILT_IN_PROMPT_PATCH_VARIANTS = [ // new paragraph, so removal reconstructs the prior prompt byte-for-byte. [[REVIEWER_POST_APPROVAL_BLOCKER_PARAGRAPH, '']], [[FULLSTACK_QA_POST_APPROVAL_PARAGRAPH, '']], + // type→shape save_artifact API migration (dev→shape cutover). Each swaps a + // persisted legacy type:"result" call site to its shape equivalent; only an + // exact retired variant matches, so customizations are preserved. + [[SHAPE_PR_LINK, RETIRED_TYPE_RESULT_PR_LINK]], + // Coding + Research reviewer prompts rewrote BOTH the preceding "every cycle" + // sentence and the PR-link call, so both must reverse together to reconstruct + // the dev-era prompt. + [ + [SHAPE_PR_EVERY_CYCLE, RETIRED_TYPE_RESULT_EVERY_CYCLE], + [SHAPE_PR_LINK, RETIRED_TYPE_RESULT_PR_LINK], + ], + [[SHAPE_PR_LINK_REVIEW_ONLY, RETIRED_TYPE_RESULT_PR_LINK_REVIEW_ONLY]], + [[SHAPE_DECISION_DISPATCHER_STACK, RETIRED_TYPE_RESULT_DISPATCHER_STACK]], + [[SHAPE_DECISION_DISPATCHER_SHORT, RETIRED_TYPE_RESULT_DISPATCHER_SHORT]], + [[SHAPE_NOTE_QA_FAILED, RETIRED_TYPE_RESULT_QA_FAILED]], + [[SHAPE_QA_ALL_GREEN, RETIRED_TYPE_RESULT_QA_ALL_GREEN]], ] as const; function patchKnownBuiltInPromptDrift( diff --git a/packages/daemon/src/lib/space/workflows/coding-artifact-profile.ts b/packages/daemon/src/lib/space/workflows/coding-artifact-profile.ts new file mode 100644 index 0000000000..81db2aaf2d --- /dev/null +++ b/packages/daemon/src/lib/space/workflows/coding-artifact-profile.ts @@ -0,0 +1,221 @@ +/** + * Coding-Workflow Artifact Profile + * + * The domain implementation of {@link WorkflowArtifactProfile} for coding + * workflows. This is the ONLY place in the daemon that names coding-specific + * kinds (`pr`, `review`) and coding-specific identifiers (`review-posted-gate`, + * the `pr_url` / `prUrl` / `review_url` gate-data fields). Generic infra depends + * on the interface; it never imports this module. + * + * It consolidates the three behaviors that previously lived (duplicated and + * kind-hardcoded) inside daemon core: + * - `resolvePrimaryLinkUrl` — the PR URL (a `link kind:'pr'`, or a legacy + * `pr_url`/`prUrl` field), resolved across gate data, hook state, and + * artifacts by recency. + * - `summarizeRunOutcome` — the kindless terminal `decision` summary. + * - `onGateDataCommitted` — append one `decision kind:'review'` (round-N) + * each time the review-posted-gate receives a `review_url`. + */ + +import { deriveArtifactKey } from '@hyperneo/shared'; +import { Logger } from '../../logger'; +import type { GateDataCommittedEvent, WorkflowArtifactProfile } from '../runtime/artifact-profile'; +import { GateDataRepository } from '../../../storage/repositories/gate-data-repository'; +import { WorkflowHookStateRepository } from '../../../storage/repositories/workflow-hook-state-repository'; +import type { WorkflowRunArtifactRepository } from '../../../storage/repositories/workflow-run-artifact-repository'; + +const log = new Logger('coding-artifact-profile'); + +/** Gate id that records a multi-round review decision per cycle. */ +const REVIEW_POSTED_GATE = 'review-posted-gate'; + +/** + * The SQLite database type this profile consumes. Derived from the repository + * constructor rather than imported directly from `bun:sqlite` so it is + * structurally identical to what every other repo caller passes (and what + * `getDatabase()` returns) under any Bun type resolution — avoiding a + * bun:sqlite ↔ node:sqlite type-surface mismatch that only surfaces in CI. + */ +type ArtifactDb = ConstructorParameters[0]; + +export interface CodingArtifactProfileConfig { + db: ArtifactDb; + artifactRepo?: WorkflowRunArtifactRepository; + /** Optional shared gate-data repo; created from `db` when omitted. */ + gateDataRepo?: GateDataRepository; +} + +/** + * Extract a legacy PR URL (`prUrl` / `pr_url`) from a data object. Returns '' + * when neither field holds a string. A generic `url` field never qualifies — + * it could be an issue or preview link. + */ +function legacyPrUrl(data: Record | undefined): string { + return ( + (typeof data?.prUrl === 'string' && data.prUrl) || + (typeof data?.pr_url === 'string' && data.pr_url) || + '' + ); +} + +export class CodingArtifactProfile implements WorkflowArtifactProfile { + private readonly db: ArtifactDb; + private readonly artifactRepo?: WorkflowRunArtifactRepository; + private readonly sharedGateDataRepo?: GateDataRepository; + + constructor(config: CodingArtifactProfileConfig) { + this.db = config.db; + this.artifactRepo = config.artifactRepo; + this.sharedGateDataRepo = config.gateDataRepo; + } + + resolvePrimaryLinkUrl(runId: string): string { + // The primary link is the FRESHEST eligible PR URL across ALL sources + // (gate data, hook state, artifacts), compared by updatedAt — so a newer + // `link kind:'pr'` artifact supersedes a stale gate-data `pr_url` (and vice + // versa). A generic `url` on a non-pr artifact never qualifies. + type Candidate = { url: string; updatedAt: number }; + let best: Candidate | null = null; + // Pure fresher (no closure mutation) so TS control-flow tracks `best`. + const fresher = (prev: Candidate | null, url: string, updatedAt: number): Candidate | null => { + if (!url) return prev; + if (!prev || updatedAt > prev.updatedAt) return { url, updatedAt }; + return prev; + }; + + // 1. Gate data — records carrying a legacy pr_url/prUrl. + try { + const gateDataRepo = this.sharedGateDataRepo ?? new GateDataRepository(this.db); + for (const record of gateDataRepo.listByRun(runId)) { + best = fresher(best, legacyPrUrl(record.data), record.updatedAt); + } + } catch (err) { + log.warn( + `resolvePrimaryLinkUrl: failed to read gate data for run ${runId}: ${err instanceof Error ? err.message : String(err)}` + ); + } + + // 2. Workflow hook state — `pr_ready` hooks persist `pr_url` after a + // successful send_message even when the gate schema does not declare it. + try { + const hookStateRepo = new WorkflowHookStateRepository(this.db); + for (const snapshot of hookStateRepo.listByRun(runId)) { + best = fresher(best, legacyPrUrl(snapshot.localState), snapshot.updatedAt ?? 0); + } + } catch (err) { + log.warn( + `resolvePrimaryLinkUrl: failed to read hook state for run ${runId}: ${err instanceof Error ? err.message : String(err)}` + ); + } + + // 3. Artifacts — a `link kind:'pr'` (read via data.url) or a legacy row + // carrying pr_url/prUrl. + if (this.artifactRepo) { + try { + for (const a of this.artifactRepo.listByRun(runId)) { + const url = + a.artifactType === 'link' && a.data.kind === 'pr' + ? typeof a.data.url === 'string' + ? a.data.url + : '' + : legacyPrUrl(a.data); + best = fresher(best, url, a.updatedAt); + } + } catch (err) { + log.warn( + `resolvePrimaryLinkUrl: failed to read artifacts for run ${runId}: ${err instanceof Error ? err.message : String(err)}` + ); + } + } + + return best?.url ?? ''; + } + + summarizeRunOutcome(runId: string): string | null { + if (!this.artifactRepo) return null; + try { + // The terminal outcome is a kindless `decision` carrying a summary. + // Review/gate decisions carry a kind and are not terminal; rolling-status + // `note`s are excluded too. + const decisions = this.artifactRepo.listByRun(runId, { artifactType: 'decision' }); + const summaryOf = (item: { data: Record }): string => { + const s = item.data.summary; + return typeof s === 'string' ? s : ''; + }; + const isTerminal = (item: { data: Record }): boolean => + !item.data.kind && summaryOf(item).trim().length > 0; + const artifact = decisions + .map((item, index) => ({ item, index })) + .filter(({ item }) => isTerminal(item)) + .toSorted( + (a, b) => + b.item.updatedAt - a.item.updatedAt || + b.item.createdAt - a.item.createdAt || + b.index - a.index + )[0]?.item; + const summary = artifact ? summaryOf(artifact) : ''; + return summary.length > 0 ? summary : null; + } catch (err) { + log.warn( + `summarizeRunOutcome: failed to read artifacts for run ${runId}: ${err instanceof Error ? err.message : String(err)}` + ); + return null; + } + } + + async onGateDataCommitted(event: GateDataCommittedEvent): Promise { + if (!this.artifactRepo) return; + const { runId, nodeId, gateId, committedData, messageData } = event; + // Multi-round review history: every time the reviewer COMMITS a fresh + // `review_url` on the review-posted-gate, persist one `decision kind:'review'` + // per cycle (round-0, round-1 …) keyed so each round is a distinct upsert. + // Read the URL from `committedData` — the gate-declared fields the sender was + // authorized to write in THIS send (not the raw payload, and not the merged + // gate state). This avoids two failure modes: (a) a later send that only + // updates comment_urls would see the prior round's review_url in the merged + // gate state and spuriously record an extra round; (b) a field the agent sent + // but was not authorized to write must not trigger a side-artifact. + if (gateId !== REVIEW_POSTED_GATE) return; + const reviewUrl = committedData?.review_url; + if (typeof reviewUrl !== 'string' || reviewUrl.length === 0) return; + + try { + const decisions = this.artifactRepo.listByRun(runId, { artifactType: 'decision' }); + // Next round = one past the highest existing review-round number, derived + // from the trailing digits of each review decision's key (handles sparse + // keys and both legacy 'cycle-N' and namespaced 'review:round-N' forms). + let maxCycle = -1; + for (const a of decisions) { + if (a.data.kind !== 'review') continue; + const m = /(\d+)$/.exec(a.artifactKey); + if (m) maxCycle = Math.max(maxCycle, Number.parseInt(m[1], 10)); + } + const cycle = maxCycle + 1; + const artifactData: Record = { + recommendation: 'reviewed', + kind: 'review', + review_url: reviewUrl, + cycle, + submittedAt: new Date().toISOString(), + }; + // comment_urls is review metadata, not a gate field, so it travels in the + // raw payload (messageData) rather than committedData. + const rawCommentUrls = messageData?.comment_urls; + if (Array.isArray(rawCommentUrls) && rawCommentUrls.every((u) => typeof u === 'string')) { + artifactData.comment_urls = rawCommentUrls; + } + this.artifactRepo.upsert({ + id: crypto.randomUUID(), + runId, + nodeId, + artifactType: 'decision', + artifactKey: deriveArtifactKey('decision', { kind: 'review' }, `round-${cycle}`), + data: artifactData, + }); + } catch (err) { + log.warn( + `onGateDataCommitted: failed to append review artifact for run ${runId}: ${err instanceof Error ? err.message : String(err)}` + ); + } + } +} diff --git a/packages/daemon/src/lib/space/workflows/post-approval-merge-template.ts b/packages/daemon/src/lib/space/workflows/post-approval-merge-template.ts index 6a28cd3ea2..d206f43302 100644 --- a/packages/daemon/src/lib/space/workflows/post-approval-merge-template.ts +++ b/packages/daemon/src/lib/space/workflows/post-approval-merge-template.ts @@ -153,7 +153,7 @@ export const PR_MERGE_POST_APPROVAL_INSTRUCTIONS: string = [ ' because the cap is reached, or the approval authority reports a blocker', ' neither of you can resolve, escalate to space-agent (NOT merely because', ' merges kept failing) — record a NON-result artifact and notify:', - ' save_artifact({ type: "merge_blocked", append: true,', + ' save_artifact({ shape: "note", kind: "merge_blocked",', ' summary: "Merge blocked on PR {{pr_url}} ( attempts, )",', ' data: { pr_url: "{{pr_url}}", blockers: ["..."], attempts: ,', ' exit_reason: "" } })', @@ -173,7 +173,7 @@ export const PR_MERGE_POST_APPROVAL_INSTRUCTIONS: string = [ ' git push origin --delete "$HEAD_REF"', ' Branch cleanup is BEST-EFFORT: if deletion fails for any reason (protected', ' branch, missing delete permission, already gone), record a NON-result warning', - ' artifact (e.g. type:"cleanup_warning") and continue — a "result" artifact would', + ' artifact (e.g. shape:"note", kind:"cleanup_warning", key:"branch-delete") and continue — a kindless `decision` would', ' be picked up as the task result on completion. The PR is already merged, so do', ' NOT let a cleanup failure block the completion step.', '4. Sync so both this isolated worktree AND the Space checkout track the freshly-merged', @@ -195,18 +195,18 @@ export const PR_MERGE_POST_APPROVAL_INSTRUCTIONS: string = [ ' continues):', ' if [ "$(git -C "$SPACE_WS" rev-parse --abbrev-ref HEAD)" != "$BASE" ]; then', ' # checkout on a different branch — do NOT move it; warn and skip.', - ' record a NON-result cleanup_warning artifact (space not on $BASE) and continue.', + ' record a NON-result `note` cleanup_warning artifact (key "space-checkout-base"; space not on $BASE) and continue.', ' fi', ' git -C "$SPACE_WS" fetch origin "$BASE"', ' git -C "$SPACE_WS" pull --ff-only origin "$BASE"', ' if [ "$(git -C "$SPACE_WS" rev-parse HEAD)" != "$(git -C "$SPACE_WS" rev-parse "origin/$BASE")" ]; then', ' # pull said "Already up to date" but local $BASE is AHEAD of origin/$BASE —', ' # stray commits remain at HEAD; do NOT claim the checkout is synchronized.', - ' record a NON-result cleanup_warning artifact (space $BASE ahead of origin/$BASE) and continue.', + ' record a NON-result `note` cleanup_warning artifact (key "space-checkout-ahead"; space $BASE ahead of origin/$BASE) and continue.', ' fi', ' If the pull itself fails (divergence, permissions), do NOT force it — record a', - ' NON-result cleanup_warning artifact and continue (the PR is already merged).', + ' NON-result `note` cleanup_warning artifact (key "space-checkout-pull") and continue (the PR is already merged).', '5. Save an audit artifact:', - ' save_artifact({ type: "result", append: true,', - ' data: { merged_pr_url, merged_at, approval_source: "{{approval_source}}" } })', + ' save_artifact({ shape: "link", kind: "merge",', + ' data: { url: , merged_at, approval_source: "{{approval_source}}" } })', ].join('\n'); diff --git a/packages/daemon/tests/unit/5-space/agent/node-agent-tools.test.ts b/packages/daemon/tests/unit/5-space/agent/node-agent-tools.test.ts index f50f063bb1..305dfbc438 100644 --- a/packages/daemon/tests/unit/5-space/agent/node-agent-tools.test.ts +++ b/packages/daemon/tests/unit/5-space/agent/node-agent-tools.test.ts @@ -48,6 +48,8 @@ import type { DaemonInternalEventMap, InternalEventBus, } from '../../../../src/lib/internal-event-bus.ts'; +import { CodingArtifactProfile } from '../../../../src/lib/space/workflows/coding-artifact-profile.ts'; +import type { WorkflowArtifactProfile } from '../../../../src/lib/space/runtime/artifact-profile.ts'; // --------------------------------------------------------------------------- // DB helpers @@ -187,6 +189,8 @@ interface TestCtx { spaceTaskRepo: SpaceTaskRepository; nodeExecutionRepo: NodeExecutionRepository; artifactRepo: WorkflowRunArtifactRepository; + /** Coding artifact profile (PR resolution + review history) wired into the handlers. */ + artifactProfile: WorkflowArtifactProfile; /** Workflow run ID for peer task seeding. */ workflowRunId: string; /** Workflow node ID for peer task seeding. */ @@ -211,6 +215,7 @@ function makeCtx(): TestCtx { const taskManager = new SpaceTaskManager(db, spaceId); const nodeExecutionRepo = new NodeExecutionRepository(db); const artifactRepo = new WorkflowRunArtifactRepository(db); + const artifactProfile = new CodingArtifactProfile({ db, artifactRepo }); // Session IDs for peers const taskAgentSessionId = 'session-task-agent'; @@ -259,6 +264,7 @@ function makeCtx(): TestCtx { spaceTaskRepo, nodeExecutionRepo, artifactRepo, + artifactProfile, workflowRunId, nodeId, coderSessionId, @@ -301,6 +307,7 @@ function makeConfig(ctx: TestCtx, overrides: NodeConfigOverrides = {}): NodeAgen workflow: null, gateDataRepo: new GateDataRepository(ctx.db), artifactRepo: ctx.artifactRepo, + artifactProfile: ctx.artifactProfile, ...configOverrides, }; } @@ -1062,96 +1069,129 @@ describe('node-agent-tools: save_artifact', () => { expect(data.error).toContain('summary'); }); - // ── Legacy compatibility shim ────────────────────────────────────────────── + // ── Shape required (legacy `type` alias removed) ────────────────────────── - test('legacy { type: "progress" } maps to a note shape', async () => { + test('rejects when shape is missing — the legacy `type` alias is no longer accepted', async () => { const handlers = createNodeAgentToolHandlers(makeConfig(ctx)); - const result = await handlers.save_artifact({ type: 'progress', summary: 'halfway done' }); + // `type` is silently stripped by the schema; with no `shape`, the handler + // rejects. This documents the cutover off the legacy freeform-type shim. + const result = await handlers.save_artifact({ + // @ts-expect-error — legacy `type` is no longer part of the schema + type: 'progress', + summary: 'halfway done', + }); const data = JSON.parse(result.content[0].text); - expect(data.success).toBe(true); - expect(data.artifact.shape).toBe('note'); - - const notes = ctx.artifactRepo.listByRun(ctx.workflowRunId, { artifactType: 'note' }); - expect(notes).toHaveLength(1); - expect(notes[0].data.summary).toBe('halfway done'); + expect(data.success).toBe(false); + expect(data.error).toContain('shape'); }); - test('legacy { type: "pr" } maps to a link kind:pr and normalizes data.url', async () => { + // ── Prompt → validator parity ─────────────────────────────────────────── + // Runs the EXACT save_artifact payloads emitted by the migrated coding-workflow + // prompts (built-in-workflows, post-approval-merge-template) and the end-node + // Runtime Execution Contract through the real handler. CI was green while the + // `decision` sites omitted `data.recommendation` only because nothing exercised + // prompt→validator; this test is the regression guard for that class of bug. + + test('every migrated prompt payload is accepted and persists', async () => { const handlers = createNodeAgentToolHandlers(makeConfig(ctx)); - const result = await handlers.save_artifact({ - type: 'pr', - data: { pr_url: 'https://github.com/acme/app/pull/7' }, + const run = async (payload: Parameters[0]) => { + const parsed = JSON.parse((await handlers.save_artifact(payload)).content[0].text); + expect(parsed.success).toBe(true); + return parsed; + }; + + // Dispatcher (built-in-workflows) — terminal outcome decision. + await run({ + shape: 'decision', + summary: 'Created 2 tasks from plan: foo, bar', + data: { + recommendation: 'dispatched', + created_task_ids: ['t1', 't2'], + stack_prefix: 'plan-x', + stack_branches: ['plan/x/foo', 'plan/x/bar'], + }, }); - const data = JSON.parse(result.content[0].text); - expect(data.success).toBe(true); - expect(data.artifact.shape).toBe('link'); - const links = ctx.artifactRepo.listByRun(ctx.workflowRunId, { artifactType: 'link' }); - expect(links).toHaveLength(1); - expect(links[0].data.url).toBe('https://github.com/acme/app/pull/7'); - expect(links[0].data.kind).toBe('pr'); - }); + // Fullstack QA all-green (built-in-workflows) — PR link + terminal outcome. + await run({ shape: 'link', kind: 'pr', data: { url: 'https://github.com/o/r/pull/9' } }); + await run({ + shape: 'decision', + summary: 'QA passed', + data: { + recommendation: 'pass', + test_output: 'ok', + ui_changed: true, + dev_server_started: true, + browser_validation: 'exercised login flow', + }, + }); - test('legacy { type: "result", data: { pr_url } } (no summary) routes to link (data-aware)', async () => { - const handlers = createNodeAgentToolHandlers(makeConfig(ctx)); - const result = await handlers.save_artifact({ - type: 'result', - data: { pr_url: 'https://github.com/acme/app/pull/9' }, + // End-node Runtime Execution Contract (task-agent-manager) — both branches + // carry data.recommendation and a distinct key so the generic outcome + // decision doesn't clobber a slot-prompt terminal decision (key 'current'). + await run({ + shape: 'decision', + key: 'outcome', + summary: 'Done', + data: { recommendation: 'completed' }, }); - const data = JSON.parse(result.content[0].text); - expect(data.success).toBe(true); - expect(data.artifact.shape).toBe('link'); - expect(data.artifact.key).toBe('pr'); - }); - test('legacy { type: "result", summary } routes to decision (data-aware)', async () => { - const handlers = createNodeAgentToolHandlers(makeConfig(ctx)); - const result = await handlers.save_artifact({ type: 'result', summary: 'shipped' }); - const data = JSON.parse(result.content[0].text); - expect(data.success).toBe(true); - expect(data.artifact.shape).toBe('decision'); - }); + // Merge-conflict attempt (post-approval-merge-template) — per-attempt note. + await run({ + shape: 'note', + kind: 'merge_conflict', + key: 'attempt-0', + summary: 'Merge conflict attempt 0 on PR https://github.com/o/r/pull/9', + data: { + pr_url: 'https://github.com/o/r/pull/9', + base_branch: 'dev', + approved_head_oid: 'abc', + conflicting_files: ['a.ts'], + attempt: 0, + }, + }); + await run({ + shape: 'note', + kind: 'merge_conflict', + key: 'attempt-1', + summary: 'Merge conflict attempt 1 on PR https://github.com/o/r/pull/9', + data: { pr_url: 'https://github.com/o/r/pull/9', attempt: 1 }, + }); - test('legacy { type: "result", summary + pr_url } keeps the summary as a decision', async () => { - const handlers = createNodeAgentToolHandlers(makeConfig(ctx)); - const result = await handlers.save_artifact({ - type: 'result', - summary: 'QA passed', - data: { pr_url: 'https://github.com/acme/app/pull/9' }, + // Post-merge audit (post-approval-merge-template) — link kind:'merge'. + await run({ + shape: 'link', + kind: 'merge', + data: { + url: 'https://github.com/o/r/pull/9', + merged_at: '2026-01-01', + approval_source: 'human', + }, }); - const data = JSON.parse(result.content[0].text); - expect(data.success).toBe(true); - expect(data.artifact.shape).toBe('decision'); - // pr_url preserved on the decision so PR readers still find it via the - // legacy-field fallback. - const decisions = ctx.artifactRepo.listByRun(ctx.workflowRunId, { - artifactType: 'decision', + + // Fullstack QA failure (built-in-workflows) — per-cycle note so each failure + // cycle keeps its own repro evidence. + await run({ + shape: 'note', + kind: 'qa', + key: 'cycle-1', + summary: 'QA failed (cycle 1): login redirect broken', + }); + await run({ + shape: 'note', + kind: 'qa', + key: 'cycle-2', + summary: 'QA failed (cycle 2): flaky test on retry', }); - expect(decisions[0]?.data.summary).toBe('QA passed'); - expect(decisions[0]?.data.pr_url).toBe('https://github.com/acme/app/pull/9'); - }); - test('legacy unknown freeform type is accepted as a note (keeps working)', async () => { - const handlers = createNodeAgentToolHandlers(makeConfig(ctx)); - const result = await handlers.save_artifact({ type: 'merge_blocked', summary: 'conflict' }); - const data = JSON.parse(result.content[0].text); - expect(data.success).toBe(true); - expect(data.artifact.shape).toBe('note'); + // Per-attempt / per-cycle notes must persist as DISTINCT rows (not overwrite). const notes = ctx.artifactRepo.listByRun(ctx.workflowRunId, { artifactType: 'note' }); - expect(notes).toHaveLength(1); - expect(notes[0]?.data._legacyType).toBe('merge_blocked'); - // Distinct key per unknown type so different blockers don't collapse. - expect(notes[0]?.artifactKey).toBe('merge_blocked'); - }); - - test('a shape NAME passed as legacy type is validated (no bypass)', async () => { - const handlers = createNodeAgentToolHandlers(makeConfig(ctx)); - // type:'link' without data.url must be rejected even though it uses the - // legacy alias (a shape name is not a legacy semantic type). - const result = await handlers.save_artifact({ type: 'link', data: { title: 'no url' } }); - const data = JSON.parse(result.content[0].text); - expect(data.success).toBe(false); - expect(data.error).toContain('url'); + expect(notes.map((n) => n.artifactKey).sort()).toEqual([ + 'merge_conflict:attempt-0', + 'merge_conflict:attempt-1', + 'qa:cycle-1', + 'qa:cycle-2', + ]); }); }); @@ -1197,49 +1237,6 @@ describe('node-agent-tools: list_artifacts', () => { }>; } - test('legacy { type: "pr" } post-filters to kind:pr links (excludes issue/preview)', async () => { - const handlers = await seedMixed(); - const artifacts = artifactsOf(await handlers.list_artifacts({ type: 'pr' })); - // Before the kind post-filter, this over-returned all three links. - expect(artifacts).toHaveLength(1); - expect(artifacts[0].type).toBe('link'); - expect(artifacts[0].data.kind).toBe('pr'); - expect(artifacts[0].data.url).toBe('https://example.com/pr/1'); - }); - - test('legacy { type: "review" } post-filters to kind:review decisions (excludes gate)', async () => { - const handlers = await seedMixed(); - const artifacts = artifactsOf(await handlers.list_artifacts({ type: 'review' })); - expect(artifacts).toHaveLength(1); - expect(artifacts[0].type).toBe('decision'); - expect(artifacts[0].data.kind).toBe('review'); - }); - - test('legacy { type: "result" } stays unfiltered (overloaded decision|link, all kinds)', async () => { - const handlers = await seedMixed(); - const artifacts = artifactsOf(await handlers.list_artifacts({ type: 'result' })); - // 3 links + 2 decisions, including non-pr links and the gate decision — - // i.e. no kind post-filter is applied to the overloaded `result` type. - expect(artifacts).toHaveLength(5); - expect(artifacts.some((a) => a.data.kind === 'gate')).toBe(true); - expect(artifacts.some((a) => a.data.kind === 'issue')).toBe(true); - }); - - test('legacy { type: "progress" } stays unfiltered (note)', async () => { - const handlers = await seedMixed(); - const artifacts = artifactsOf(await handlers.list_artifacts({ type: 'progress' })); - expect(artifacts).toHaveLength(1); - expect(artifacts[0].type).toBe('note'); - }); - - test('legacy { type: "pr" } save round-trips through { type: "pr" } list', async () => { - const handlers = createNodeAgentToolHandlers(makeConfig(ctx)); - await handlers.save_artifact({ type: 'pr', data: { pr_url: 'https://example.com/pr/7' } }); - const artifacts = artifactsOf(await handlers.list_artifacts({ type: 'pr' })); - expect(artifacts).toHaveLength(1); - expect(artifacts[0].data.kind).toBe('pr'); - }); - test('direct shape name { type: "link" } returns all links (no kind filter)', async () => { const handlers = await seedMixed(); const artifacts = artifactsOf(await handlers.list_artifacts({ type: 'link' })); @@ -1252,26 +1249,6 @@ describe('node-agent-tools: list_artifacts', () => { const artifacts = artifactsOf(await handlers.list_artifacts({})); expect(artifacts).toHaveLength(6); }); - - test('kind-less link/decision rows are excluded from legacy pr/review filters', async () => { - const handlers = createNodeAgentToolHandlers(makeConfig(ctx)); - // Direct shape writes with no kind tag are legitimately-not-PR / not-review: - // the backfill migration (migrations.ts:11290-11299) only tags - // pr/review/result-derived rows, so a genuinely kind-less link/decision is - // an untagged direct write that the legacy pr/review filters must exclude. - // Documents the intentional narrowing at the migration boundary. - await handlers.save_artifact({ shape: 'link', data: { url: 'https://example.com/doc' } }); - await handlers.save_artifact({ - shape: 'decision', - data: { recommendation: 'ship it' }, - }); - - expect(artifactsOf(await handlers.list_artifacts({ type: 'pr' }))).toHaveLength(0); - expect(artifactsOf(await handlers.list_artifacts({ type: 'review' }))).toHaveLength(0); - // Direct shape queries are unfiltered and still return them. - expect(artifactsOf(await handlers.list_artifacts({ type: 'link' }))).toHaveLength(1); - expect(artifactsOf(await handlers.list_artifacts({ type: 'decision' }))).toHaveLength(1); - }); }); // --------------------------------------------------------------------------- @@ -3436,7 +3413,7 @@ describe('node-agent-tools: async gate evaluation', () => { gateId: gate.id, }, 'node-coder', - ctx.artifactRepo + ctx.artifactProfile ); const data = JSON.parse(result!.content[0].text); @@ -3490,7 +3467,7 @@ describe('node-agent-tools: async gate evaluation', () => { gateId: gate.id, }, 'node-coder', - ctx.artifactRepo + ctx.artifactProfile ); const data = JSON.parse(result!.content[0].text); @@ -3545,7 +3522,7 @@ describe('node-agent-tools: async gate evaluation', () => { mockExecutor, { workspacePath: '/tmp', runId: ctx.workflowRunId, gateId: gate.id }, 'node-coder', - ctx.artifactRepo + ctx.artifactProfile ); expect(result).not.toBeNull(); const data = JSON.parse(result!.content[0].text); @@ -3996,6 +3973,64 @@ describe('node-agent-tools: review-posted-gate multi-round artifact history', () expect(artifacts).toHaveLength(0); }); + test('a follow-up send with only comment_urls does not append a spurious review round', async () => { + // Regression: the trigger must read the CURRENT write payload's review_url, + // not the merged gate state. After round-0 persists review_url in the gate, + // a later send that only updates comment_urls must NOT create a round-1 + // decision (the prior code read gateData.review_url and would double-count). + const { WorkflowRunArtifactRepository } = await import( + '../../../../src/storage/repositories/workflow-run-artifact-repository.ts' + ); + const artifactRepo = new WorkflowRunArtifactRepository(ctx.db); + const gate: Gate = { + id: 'review-posted-gate', + fields: [ + { name: 'review_url', type: 'string', writers: ['reviewer'], check: { op: 'exists' } }, + { name: 'comment_urls', type: 'string', writers: ['reviewer'], check: { op: 'exists' } }, + ], + resetOnCycle: false, + }; + const workflow: SpaceWorkflow = { + id: 'wf-followup', + spaceId: ctx.spaceId, + name: 'Test', + description: '', + nodes: [], + startNodeId: '', + rules: [], + tags: [], + channels: [ + { id: 'ch-review-coder', from: 'reviewer', to: 'coder', gateId: 'review-posted-gate' }, + ], + gates: [gate], + }; + const config = makeConfig(ctx, { + workflow, + myAgentName: 'reviewer', + mySessionId: ctx.reviewerSessionId, + artifactRepo, + }); + const handlers = createNodeAgentToolHandlers(config); + + // Round 0: deliver a review_url → one review decision. + await handlers.send_message({ + target: 'coder', + message: 'reviewed', + data: { review_url: 'https://github.com/acme/app/pull/42#pullrequestreview-1' }, + }); + // Follow-up: only comment_urls, no fresh review_url in the payload. + await handlers.send_message({ + target: 'coder', + message: 'thread replies', + data: { comment_urls: ['https://github.com/acme/app/pull/42#discussion_r1'] }, + }); + + // Still exactly one review decision — no spurious round-1. + const artifacts = artifactRepo.listByRun(ctx.workflowRunId, { artifactType: 'decision' }); + expect(artifacts).toHaveLength(1); + expect(artifacts[0]?.artifactKey).toBe('review:round-0'); + }); + test('skips artifact append for non-review-posted-gate gates (no false positives)', async () => { const { WorkflowRunArtifactRepository } = await import( '../../../../src/storage/repositories/workflow-run-artifact-repository.ts' @@ -5514,15 +5549,16 @@ describe('node-agent-tools: pr_url payloads do not auto-subscribe', () => { test('save_artifact with pr_url succeeds without requiring a subscription callback', async () => { const handlers = createNodeAgentToolHandlers(makeConfig(ctx)); const result = await handlers.save_artifact({ - type: 'result', - data: { pr_url: 'https://github.com/acme/widgets/pull/123' }, + shape: 'link', + kind: 'pr', + data: { url: 'https://github.com/acme/widgets/pull/123' }, }); const data = JSON.parse(result.content[0].text); expect(data.success).toBe(true); const artifacts = ctx.artifactRepo.listByRun(ctx.workflowRunId); expect(artifacts).toHaveLength(1); - expect(artifacts[0]!.data.pr_url).toBe('https://github.com/acme/widgets/pull/123'); + expect(artifacts[0]!.data.url).toBe('https://github.com/acme/widgets/pull/123'); }); test('send_message with pr_url succeeds without requiring a subscription callback', async () => { diff --git a/packages/daemon/tests/unit/5-space/evolution-episode-service.test.ts b/packages/daemon/tests/unit/5-space/evolution-episode-service.test.ts index ecf59e8e51..d2c7af91e2 100644 --- a/packages/daemon/tests/unit/5-space/evolution-episode-service.test.ts +++ b/packages/daemon/tests/unit/5-space/evolution-episode-service.test.ts @@ -18,6 +18,8 @@ import { SpaceWorkflowRunRepository } from '../../../src/storage/repositories/sp import { WorkflowRunArtifactRepository } from '../../../src/storage/repositories/workflow-run-artifact-repository'; import { SpaceWorkflowRepository } from '../../../src/storage/repositories/space-workflow-repository'; import { SpaceGoalService } from '../../../src/lib/space/goals/goal-service'; +import { CodingArtifactProfile } from '../../../src/lib/space/workflows/coding-artifact-profile'; +import type { WorkflowArtifactProfile } from '../../../src/lib/space/runtime/artifact-profile'; import { createSpaceTables } from '../helpers/space-test-db'; describe('EvolutionEpisodeService', () => { @@ -26,6 +28,7 @@ describe('EvolutionEpisodeService', () => { let taskRepo: SpaceTaskRepository; let workflowRunRepo: SpaceWorkflowRunRepository; let artifactRepo: WorkflowRunArtifactRepository; + let artifactProfile: WorkflowArtifactProfile; let workflowRepo: SpaceWorkflowRepository; let goalRepo: SpaceGoalRepository; let spaceRepo: SpaceRepository; @@ -43,6 +46,7 @@ describe('EvolutionEpisodeService', () => { new GateOpenStateRepository(db as never) ); artifactRepo = new WorkflowRunArtifactRepository(db as never); + artifactProfile = new CodingArtifactProfile({ db: db as never, artifactRepo }); workflowRepo = new SpaceWorkflowRepository(db as never); spaceId = spaceRepo.createSpace({ workspacePath: '/workspace/episode-service-test', @@ -255,6 +259,7 @@ describe('EvolutionEpisodeService', () => { taskRepo, workflowRunRepo, artifactRepo, + artifactProfile, judgeEpisode: async () => { judgeCalled = true; return { title: 'Should not run', outcomeSummary: 'Nope', findings: [] }; @@ -346,6 +351,7 @@ describe('EvolutionEpisodeService', () => { taskRepo, workflowRunRepo, artifactRepo, + artifactProfile, }); const listedWithoutContext = scopeService.listEvidence(scope.id); expect(listedWithoutContext.preflightContext).toBeUndefined(); @@ -401,6 +407,7 @@ describe('EvolutionEpisodeService', () => { taskRepo, workflowRunRepo, artifactRepo, + artifactProfile, }); const input = service.buildEpisodeInput({ @@ -559,6 +566,7 @@ describe('EvolutionEpisodeService', () => { taskRepo, workflowRunRepo, artifactRepo, + artifactProfile, }); const before = service.buildEpisodeInput({ scopeId: scope.id, @@ -665,6 +673,7 @@ describe('EvolutionEpisodeService', () => { taskRepo, workflowRunRepo, artifactRepo, + artifactProfile, }); const input = service.buildEpisodeInput({ @@ -735,6 +744,7 @@ describe('EvolutionEpisodeService', () => { taskRepo, workflowRunRepo, artifactRepo, + artifactProfile, }); const input = service.buildEpisodeInput({ @@ -838,6 +848,7 @@ describe('EvolutionEpisodeService', () => { taskRepo, workflowRunRepo, artifactRepo, + artifactProfile, }); const prompt = buildEpisodeJudgePrompt( @@ -866,6 +877,7 @@ describe('EvolutionEpisodeService', () => { taskRepo, workflowRunRepo, artifactRepo, + artifactProfile, }); const prompt = buildEpisodeJudgePrompt( @@ -954,6 +966,7 @@ describe('EvolutionEpisodeService', () => { taskRepo, workflowRunRepo, artifactRepo, + artifactProfile, }); const input = service.buildEpisodeInput({ scopeId: scope.id, evidenceIds: [evidence.id] }); @@ -1088,6 +1101,7 @@ describe('EvolutionEpisodeService', () => { taskRepo, workflowRunRepo, artifactRepo, + artifactProfile, db, taskCreatedEventHub: { publish: async (event, data) => { @@ -1162,6 +1176,7 @@ describe('EvolutionEpisodeService', () => { taskRepo, workflowRunRepo, artifactRepo, + artifactProfile, goalService: createGoalService(), }); @@ -1207,6 +1222,7 @@ describe('EvolutionEpisodeService', () => { taskRepo, workflowRunRepo, artifactRepo, + artifactProfile, }); expect(() => service.createTaskFromProposal(dismissed.id)).toThrow( @@ -1244,6 +1260,7 @@ describe('EvolutionEpisodeService', () => { taskRepo, workflowRunRepo, artifactRepo, + artifactProfile, }); expect(() => @@ -1281,6 +1298,7 @@ describe('EvolutionEpisodeService', () => { taskRepo, workflowRunRepo, artifactRepo, + artifactProfile, taskIdFactory: () => taskId, }); @@ -1337,12 +1355,14 @@ describe('EvolutionEpisodeService', () => { taskRepo, workflowRunRepo, artifactRepo, + artifactProfile, }); const service = new EvolutionEpisodeService({ evolutionRepo, taskRepo, workflowRunRepo, artifactRepo, + artifactProfile, goalService: createGoalService(), }); const serviceOnlyEpisode = evolutionRepo.createEpisode({ @@ -1361,6 +1381,7 @@ describe('EvolutionEpisodeService', () => { taskRepo, workflowRunRepo, artifactRepo, + artifactProfile, goalService: failingGoalService, }); const applied = service.applyRollupGoalUpdate({ @@ -1418,6 +1439,7 @@ describe('EvolutionEpisodeService', () => { taskRepo, workflowRunRepo, artifactRepo, + artifactProfile, }); const scope = scopeService.createScopeFromGoal({ spaceGoalId: goal.id, @@ -1487,6 +1509,7 @@ describe('EvolutionEpisodeService', () => { taskRepo, workflowRunRepo, artifactRepo, + artifactProfile, goalService: createGoalService(), judgeEpisode: async (input) => ({ title: 'Forge evidence lifecycle episode', @@ -1675,6 +1698,7 @@ describe('EvolutionEpisodeService', () => { taskRepo, workflowRunRepo, artifactRepo, + artifactProfile, goalService: createGoalService(), judgeEpisode: async (input) => ({ title: 'Forge MVP dogfood episode', @@ -1789,6 +1813,7 @@ describe('EvolutionEpisodeService', () => { taskRepo, workflowRunRepo, artifactRepo, + artifactProfile, judgeEpisode: async (input) => { judgePreflight = input.preflight; return { @@ -1900,6 +1925,7 @@ describe('EvolutionEpisodeService', () => { taskRepo, workflowRunRepo, artifactRepo, + artifactProfile, }); const input = service.buildEpisodeInput({ scopeId: scope.id, @@ -1999,6 +2025,7 @@ describe('EvolutionEpisodeService', () => { taskRepo, workflowRunRepo, artifactRepo, + artifactProfile, judgeEpisode: async () => ({ title: 'Episode', outcomeSummary: '', @@ -2065,6 +2092,7 @@ describe('EvolutionEpisodeService', () => { taskRepo, workflowRunRepo, artifactRepo, + artifactProfile, judgeEpisode: async () => ({ title: 'Episode', outcomeSummary: '', @@ -2129,6 +2157,7 @@ describe('EvolutionEpisodeService', () => { taskRepo, workflowRunRepo, artifactRepo, + artifactProfile, judgeEpisode: async () => ({ title: 'Episode', outcomeSummary: '', @@ -2179,6 +2208,7 @@ describe('EvolutionEpisodeService', () => { taskRepo, workflowRunRepo, artifactRepo, + artifactProfile, judgeEpisode: async () => ({ title: 'Episode', outcomeSummary: '', @@ -2253,6 +2283,7 @@ describe('EvolutionEpisodeService', () => { taskRepo, workflowRunRepo, artifactRepo, + artifactProfile, judgeEpisode: async () => ({ title: 'Episode', outcomeSummary: '', @@ -2316,6 +2347,7 @@ describe('EvolutionEpisodeService', () => { taskRepo, workflowRunRepo, artifactRepo, + artifactProfile, judgeEpisode: async () => ({ title: 'Episode', outcomeSummary: '', @@ -2374,6 +2406,7 @@ describe('EvolutionEpisodeService', () => { taskRepo, workflowRunRepo, artifactRepo, + artifactProfile, judgeEpisode: async () => ({ title: 'Episode', outcomeSummary: '', @@ -2435,6 +2468,7 @@ describe('EvolutionEpisodeService', () => { taskRepo, workflowRunRepo, artifactRepo, + artifactProfile, judgeEpisode: async () => ({ title: 'Episode', outcomeSummary: '', @@ -2499,6 +2533,7 @@ describe('EvolutionEpisodeService', () => { taskRepo, workflowRunRepo, artifactRepo, + artifactProfile, judgeEpisode: async () => ({ title: 'Episode', outcomeSummary: '', @@ -2557,6 +2592,7 @@ describe('EvolutionEpisodeService', () => { taskRepo, workflowRunRepo, artifactRepo, + artifactProfile, judgeEpisode: async () => ({ title: 'Episode', outcomeSummary: '', @@ -2618,6 +2654,7 @@ describe('EvolutionEpisodeService', () => { taskRepo, workflowRunRepo, artifactRepo, + artifactProfile, judgeEpisode: async () => ({ title: 'Episode', outcomeSummary: '', @@ -2676,6 +2713,7 @@ describe('EvolutionEpisodeService', () => { taskRepo, workflowRunRepo, artifactRepo, + artifactProfile, judgeEpisode: async () => ({ title: 'Episode', outcomeSummary: '', diff --git a/packages/daemon/tests/unit/5-space/runtime/external-event-delivery-e2e.test.ts b/packages/daemon/tests/unit/5-space/runtime/external-event-delivery-e2e.test.ts index 5e45f6b3cb..a7bc01c13f 100644 --- a/packages/daemon/tests/unit/5-space/runtime/external-event-delivery-e2e.test.ts +++ b/packages/daemon/tests/unit/5-space/runtime/external-event-delivery-e2e.test.ts @@ -47,6 +47,7 @@ import { SpaceRuntimeService } from '../../../../src/lib/space/runtime/space-run import type { TaskAgentManager } from '../../../../src/lib/space/runtime/task-agent-manager'; import { ChannelCycleRepository } from '../../../../src/storage/repositories/channel-cycle-repository'; import { GateDataRepository } from '../../../../src/storage/repositories/gate-data-repository'; +import { CodingArtifactProfile } from '../../../../src/lib/space/workflows/coding-artifact-profile'; import { GateOpenStateRepository } from '../../../../src/storage/repositories/gate-open-state-repository'; import { NodeExecutionRepository } from '../../../../src/storage/repositories/node-execution-repository'; import { SpaceAgentRepository } from '../../../../src/storage/repositories/space-agent-repository'; @@ -417,6 +418,7 @@ async function setupE2E(): Promise { gateDataRepo, gateOpenStateRepo, channelCycleRepo, + artifactProfile: new CodingArtifactProfile({ db, gateDataRepo }), internalEventBus: bus, commandBus, externalEventStore: eventStore, diff --git a/packages/daemon/tests/unit/5-space/runtime/post-approval-router.test.ts b/packages/daemon/tests/unit/5-space/runtime/post-approval-router.test.ts index 6ae4b14c1d..3796e52242 100644 --- a/packages/daemon/tests/unit/5-space/runtime/post-approval-router.test.ts +++ b/packages/daemon/tests/unit/5-space/runtime/post-approval-router.test.ts @@ -205,11 +205,12 @@ describe('PostApprovalRouter.route', () => { // — that tool is not registered on the post-approval node-agent surface, so // referencing it sends the reviewer into an unregistered tool. expect(delegates.spawned[0].kickoffMessage).not.toContain('request_human_input'); - // A blocked-path artifact must be a NON-result type — mark_complete derives - // the task result from the latest result-artifact summary, so a "blocked" - // result artifact would poison a later successful completion. + // A blocked-path artifact must be a NON-terminal shape — mark_complete derives + // the task result from the latest kindless-decision summary, so a "blocked" + // decision would poison a later successful completion. A keyed note is the + // non-terminal form. expect(delegates.spawned[0].kickoffMessage).toMatch(/NON-result artifact/); - expect(delegates.spawned[0].kickoffMessage).toContain('type:"blocked"'); + expect(delegates.spawned[0].kickoffMessage).toContain('shape:"note", kind:"blocked"'); const final = taskRepo.getTask(task.id); expect(final?.postApprovalSessionId).toBe('spawned-session-1'); diff --git a/packages/daemon/tests/unit/5-space/runtime/post-approval-routing-integration.test.ts b/packages/daemon/tests/unit/5-space/runtime/post-approval-routing-integration.test.ts index b3bfd54222..46f0a0e402 100644 --- a/packages/daemon/tests/unit/5-space/runtime/post-approval-routing-integration.test.ts +++ b/packages/daemon/tests/unit/5-space/runtime/post-approval-routing-integration.test.ts @@ -64,6 +64,7 @@ import { SpaceWorkflowRunRepository } from '../../../../src/storage/repositories import { SpaceAgentRepository } from '../../../../src/storage/repositories/space-agent-repository.ts'; import { NodeExecutionRepository } from '../../../../src/storage/repositories/node-execution-repository.ts'; import { WorkflowRunArtifactRepository } from '../../../../src/storage/repositories/workflow-run-artifact-repository.ts'; +import { CodingArtifactProfile } from '../../../../src/lib/space/workflows/coding-artifact-profile.ts'; import { SpaceAgentManager } from '../../../../src/lib/space/managers/space-agent-manager.ts'; import { SpaceWorkflowManager } from '../../../../src/lib/space/managers/space-workflow-manager.ts'; import { SpaceManager } from '../../../../src/lib/space/managers/space-manager.ts'; @@ -195,6 +196,7 @@ function buildHarness(opts: { spawnerThrows?: boolean } = {}): Harness { taskRepo, nodeExecutionRepo, artifactRepo, + artifactProfile: new CodingArtifactProfile({ db, artifactRepo }), onTaskUpdated: async ({ task }) => { emitted.push({ taskId: task.id, status: task.status }); }, diff --git a/packages/daemon/tests/unit/5-space/runtime/space-runtime-completion.test.ts b/packages/daemon/tests/unit/5-space/runtime/space-runtime-completion.test.ts index 34b4870fbd..d174575093 100644 --- a/packages/daemon/tests/unit/5-space/runtime/space-runtime-completion.test.ts +++ b/packages/daemon/tests/unit/5-space/runtime/space-runtime-completion.test.ts @@ -15,6 +15,7 @@ import { runMigrations } from '../../../../src/storage/schema/index.ts'; import { SpaceWorkflowRepository } from '../../../../src/storage/repositories/space-workflow-repository.ts'; import { SpaceWorkflowRunRepository } from '../../../../src/storage/repositories/space-workflow-run-repository.ts'; import { WorkflowRunArtifactRepository } from '../../../../src/storage/repositories/workflow-run-artifact-repository.ts'; +import { CodingArtifactProfile } from '../../../../src/lib/space/workflows/coding-artifact-profile.ts'; import { SpaceTaskRepository } from '../../../../src/storage/repositories/space-task-repository.ts'; import { SpaceAgentRepository } from '../../../../src/storage/repositories/space-agent-repository.ts'; import { EvolutionScopeService } from '../../../../src/lib/space/evolution-scope-service.ts'; @@ -380,6 +381,7 @@ describe('SpaceRuntime — completion detection & status transitions', () => { taskRepo, artifactRepo, nodeExecutionRepo, + artifactProfile: new CodingArtifactProfile({ db, artifactRepo }), internalEventBus: bus, taskAgentManager: new MockTaskAgentManager(nodeExecutionRepo) as unknown as TaskAgentManager, ...extraConfig, diff --git a/packages/daemon/tests/unit/5-space/runtime/space-runtime-event-driven-gate-evaluation.test.ts b/packages/daemon/tests/unit/5-space/runtime/space-runtime-event-driven-gate-evaluation.test.ts index 050d13c0f8..f68b5eb37c 100644 --- a/packages/daemon/tests/unit/5-space/runtime/space-runtime-event-driven-gate-evaluation.test.ts +++ b/packages/daemon/tests/unit/5-space/runtime/space-runtime-event-driven-gate-evaluation.test.ts @@ -26,6 +26,7 @@ import { SpaceRuntimeService } from '../../../../src/lib/space/runtime/space-run import { SpaceRuntime } from '../../../../src/lib/space/runtime/space-runtime'; import type { TaskAgentManager } from '../../../../src/lib/space/runtime/task-agent-manager'; import { GateDataRepository } from '../../../../src/storage/repositories/gate-data-repository'; +import { CodingArtifactProfile } from '../../../../src/lib/space/workflows/coding-artifact-profile'; import { NodeExecutionRepository } from '../../../../src/storage/repositories/node-execution-repository'; import { ChannelCycleRepository } from '../../../../src/storage/repositories/channel-cycle-repository'; import { GateOpenStateRepository } from '../../../../src/storage/repositories/gate-open-state-repository'; @@ -172,6 +173,7 @@ async function setup(options: { gateDataRepo, gateOpenStateRepo, channelCycleRepo, + artifactProfile: new CodingArtifactProfile({ db, gateDataRepo }), internalEventBus: bus, commandBus, externalEventStore: eventStore, @@ -901,6 +903,7 @@ describe('SpaceRuntimeService event-driven gate evaluation', () => { workflowRunRepo, taskRepo, nodeExecutionRepo, + artifactProfile: new CodingArtifactProfile({ db }), internalEventBus: bus, commandBus, externalEventStore: new ExternalEventStore(db), @@ -1146,6 +1149,7 @@ describe('SpaceRuntimeService event-driven gate evaluation', () => { gateDataRepo, gateOpenStateRepo, channelCycleRepo, + artifactProfile: new CodingArtifactProfile({ db, gateDataRepo }), internalEventBus: bus, commandBus, externalEventStore: eventStore, diff --git a/packages/daemon/tests/unit/5-space/runtime/space-runtime-external-events.test.ts b/packages/daemon/tests/unit/5-space/runtime/space-runtime-external-events.test.ts index 8f980a873c..a6a68504be 100644 --- a/packages/daemon/tests/unit/5-space/runtime/space-runtime-external-events.test.ts +++ b/packages/daemon/tests/unit/5-space/runtime/space-runtime-external-events.test.ts @@ -22,6 +22,8 @@ import { SpaceTaskRepository } from '../../../../src/storage/repositories/space- import { SpaceWorkflowRepository } from '../../../../src/storage/repositories/space-workflow-repository'; import { SpaceWorkflowRunRepository } from '../../../../src/storage/repositories/space-workflow-run-repository'; import { WorkflowRunArtifactRepository } from '../../../../src/storage/repositories/workflow-run-artifact-repository'; +import { CodingArtifactProfile } from '../../../../src/lib/space/workflows/coding-artifact-profile'; +import type { WorkflowArtifactProfile } from '../../../../src/lib/space/runtime/artifact-profile'; import { createSpaceTables } from '../../helpers/space-test-db'; setDefaultTimeout(10_000); @@ -163,6 +165,7 @@ describe('SpaceRuntime external event subscriptions', () => { let taskRepo: SpaceTaskRepository; let nodeExecutionRepo: NodeExecutionRepository; let artifactRepo: WorkflowRunArtifactRepository; + let artifactProfile: WorkflowArtifactProfile; let workflowManager: SpaceWorkflowManager; let runtime: SpaceRuntime; let eventStore: ExternalEventStore; @@ -248,6 +251,7 @@ describe('SpaceRuntime external event subscriptions', () => { }); tam = new MockTaskAgentManager(); artifactRepo = new WorkflowRunArtifactRepository(db); + artifactProfile = new CodingArtifactProfile({ db, artifactRepo }); spaceManager = new SpaceManager(db); runtime = new SpaceRuntime({ db, @@ -258,6 +262,7 @@ describe('SpaceRuntime external event subscriptions', () => { taskRepo, nodeExecutionRepo, artifactRepo, + artifactProfile, internalEventBus: bus, commandBus, externalEventStore: eventStore, @@ -1095,6 +1100,7 @@ describe('SpaceRuntime external event subscriptions', () => { taskRepo, nodeExecutionRepo, artifactRepo, + artifactProfile, internalEventBus: bus, commandBus: createInternalCommandBus(), externalEventStore: eventStore, @@ -1127,6 +1133,7 @@ describe('SpaceRuntime external event subscriptions', () => { taskRepo, nodeExecutionRepo, artifactRepo, + artifactProfile, internalEventBus: bus, commandBus: createInternalCommandBus(), externalEventStore: eventStore, @@ -1422,6 +1429,7 @@ describe('SpaceRuntime external event subscriptions', () => { taskRepo, nodeExecutionRepo, artifactRepo, + artifactProfile, internalEventBus: bus, commandBus: createInternalCommandBus(), externalEventStore: eventStore, @@ -1827,6 +1835,7 @@ describe('SpaceRuntime external event subscriptions', () => { taskRepo, nodeExecutionRepo, artifactRepo, + artifactProfile, internalEventBus: bus, commandBus: createInternalCommandBus(), externalEventStore: eventStore, @@ -1871,6 +1880,7 @@ describe('SpaceRuntime external event subscriptions', () => { taskRepo, nodeExecutionRepo, artifactRepo, + artifactProfile, internalEventBus: bus, commandBus: createInternalCommandBus(), externalEventStore: eventStore, @@ -7240,6 +7250,7 @@ describe('SpaceRuntime event-driven gate evaluation', () => { taskRepo, nodeExecutionRepo, gateDataRepo, + artifactProfile: new CodingArtifactProfile({ db, gateDataRepo }), internalEventBus: bus, commandBus: localCommandBus, externalEventStore: eventStore, diff --git a/packages/daemon/tests/unit/5-space/runtime/space-runtime-rehydration.test.ts b/packages/daemon/tests/unit/5-space/runtime/space-runtime-rehydration.test.ts index 7cce08f7e6..cdd0bcb57c 100644 --- a/packages/daemon/tests/unit/5-space/runtime/space-runtime-rehydration.test.ts +++ b/packages/daemon/tests/unit/5-space/runtime/space-runtime-rehydration.test.ts @@ -22,6 +22,7 @@ import { SpaceAgentRepository } from '../../../../src/storage/repositories/space import { NodeExecutionRepository } from '../../../../src/storage/repositories/node-execution-repository.ts'; import { GateDataRepository } from '../../../../src/storage/repositories/gate-data-repository.ts'; import { WorkflowRunArtifactRepository } from '../../../../src/storage/repositories/workflow-run-artifact-repository.ts'; +import { CodingArtifactProfile } from '../../../../src/lib/space/workflows/coding-artifact-profile.ts'; import { ToolContinuationRecoveryRepository } from '../../../../src/storage/repositories/tool-continuation-recovery-repository.ts'; import { SpaceAgentManager } from '../../../../src/lib/space/managers/space-agent-manager.ts'; import { SpaceWorkflowManager } from '../../../../src/lib/space/managers/space-workflow-manager.ts'; @@ -101,6 +102,8 @@ describe('SpaceRuntime — crash recovery and rehydration', () => { const STEP_B = 'step-b'; function makeRuntime(overrides?: Partial): SpaceRuntime { + const artifactRepo = overrides?.artifactRepo; + const artifactProfile = new CodingArtifactProfile(artifactRepo ? { db, artifactRepo } : { db }); return new SpaceRuntime({ db, spaceManager, @@ -109,6 +112,7 @@ describe('SpaceRuntime — crash recovery and rehydration', () => { workflowRunRepo, taskRepo, nodeExecutionRepo: new NodeExecutionRepository(db), + artifactProfile, ...overrides, }); } diff --git a/packages/daemon/tests/unit/5-space/workflow/built-in-workflows.test.ts b/packages/daemon/tests/unit/5-space/workflow/built-in-workflows.test.ts index 3af9f34c5f..9558d5d969 100644 --- a/packages/daemon/tests/unit/5-space/workflow/built-in-workflows.test.ts +++ b/packages/daemon/tests/unit/5-space/workflow/built-in-workflows.test.ts @@ -2127,6 +2127,169 @@ describe('seedBuiltInWorkflows()', () => { ); }); + test('re-stamp swaps a legacy save_artifact({ type: "result" }) call to the shape API', () => { + // dev shipped seeded prompts on the legacy freeform-type API; the shape + // cutover rewrote each call site. The type→shape patch pairs recognize a + // persisted dev-era prompt (an exact retired variant) and swap it to the + // current template, preserving any operator customization that no longer + // matches a retired variant. + seedBuiltInWorkflows(SPACE_ID, manager, resolveAgentId); + const workflow = manager + .listWorkflows(SPACE_ID) + .find((w) => w.name === FULLSTACK_QA_LOOP_WORKFLOW.name)!; + const qaNode = workflow.nodes.find((n) => n.name === 'QA')!; + const templatePrompt = FULLSTACK_QA_LOOP_WORKFLOW.nodes.find((n) => n.name === 'QA')!.agents[0] + .customPrompt!.value; + // Simulate a dev-era persisted prompt: a current PR-link shape call replaced + // by the legacy `type: "result"` call the new schema rejects. + const stalePrompt = templatePrompt.replace( + 'save_artifact({ shape: "link", kind: "pr", data: { url: "" } })', + 'save_artifact({ type: "result", data: { pr_url: "" } })' + ); + expect(stalePrompt).not.toBe(templatePrompt); + expect(stalePrompt).toContain('save_artifact({ type: "result"'); + + manager.updateWorkflow(workflow.id, { + nodes: workflow.nodes.map((n) => + n.id !== qaNode.id + ? n + : { + ...n, + agents: n.agents.map((a, i) => + i === 0 ? { ...a, customPrompt: { value: stalePrompt } } : a + ), + } + ), + }); + db.prepare(`UPDATE space_workflows SET template_hash = ? WHERE id = ?`).run( + 'stale-legacy-type-api-hash', + workflow.id + ); + + const result = seedBuiltInWorkflows(SPACE_ID, manager, resolveAgentId); + expect(result.restamped).toContain(FULLSTACK_QA_LOOP_WORKFLOW.name); + + const after = manager.getWorkflow(workflow.id)!; + const afterPrompt = after.nodes.find((n) => n.id === qaNode.id)!.agents[0].customPrompt?.value; + expect(afterPrompt).toBe(templatePrompt); + expect(afterPrompt).not.toContain('save_artifact({ type: "result"'); + }); + + test('re-stamp swaps the expanded QA all-green step (two shape calls → one legacy result)', () => { + // The QA all-green step was one legacy `type: "result"` call in dev and is + // now a `link kind:"pr"` + `decision` pair. Verify the whole-region patch + // pair recognizes the dev-era block and restores the current two-call form. + seedBuiltInWorkflows(SPACE_ID, manager, resolveAgentId); + const workflow = manager + .listWorkflows(SPACE_ID) + .find((w) => w.name === FULLSTACK_QA_LOOP_WORKFLOW.name)!; + const qaNode = workflow.nodes.find((n) => n.name === 'QA')!; + const templatePrompt = FULLSTACK_QA_LOOP_WORKFLOW.nodes.find((n) => n.name === 'QA')!.agents[0] + .customPrompt!.value; + const SHAPE_QA_ALL_GREEN = + 'a. Record the PR and the terminal QA outcome as two artifacts: ' + + '`save_artifact({ shape: "link", kind: "pr", data: { url: "" } })` ' + + '(the canonical PR record the post-approval merge step resolves as the ' + + 'primary link) and `save_artifact({ shape: "decision", summary, data: { ' + + 'recommendation: "pass", test_output: "", ui_changed: , dev_server_started: , ' + + 'browser_validation: "" } })` (the terminal ' + + 'outcome summary). Top-level keys outside `data` are silently stripped by the ' + + 'tool schema, so nest fields correctly.\n'; + const RETIRED_TYPE_RESULT_QA_ALL_GREEN = + 'a. Call `save_artifact({ type: "result", append: true, summary, data: { ' + + 'pr_url: "", test_output: "", ui_changed: , dev_server_started: , ' + + 'browser_validation: "" } })` to record the audit entry. The ' + + '`pr_url` inside `data` is what `dispatchPostApproval` reads when interpolating `{{pr_url}}` into the ' + + 'merge template — top-level keys outside `data` are silently stripped by the tool schema, so nest it ' + + 'correctly.\n'; + // Sanity: the current-text transcription actually matches the live template. + expect(templatePrompt).toContain(SHAPE_QA_ALL_GREEN); + const stalePrompt = templatePrompt.replace( + SHAPE_QA_ALL_GREEN, + RETIRED_TYPE_RESULT_QA_ALL_GREEN + ); + expect(stalePrompt).not.toBe(templatePrompt); + expect(stalePrompt).toContain('save_artifact({ type: "result"'); + + manager.updateWorkflow(workflow.id, { + nodes: workflow.nodes.map((n) => + n.id !== qaNode.id + ? n + : { + ...n, + agents: n.agents.map((a, i) => + i === 0 ? { ...a, customPrompt: { value: stalePrompt } } : a + ), + } + ), + }); + db.prepare(`UPDATE space_workflows SET template_hash = ? WHERE id = ?`).run( + 'stale-qa-all-green-hash', + workflow.id + ); + + const result = seedBuiltInWorkflows(SPACE_ID, manager, resolveAgentId); + expect(result.restamped).toContain(FULLSTACK_QA_LOOP_WORKFLOW.name); + + const after = manager.getWorkflow(workflow.id)!; + const afterPrompt = after.nodes.find((n) => n.id === qaNode.id)!.agents[0].customPrompt?.value; + expect(afterPrompt).toBe(templatePrompt); + expect(afterPrompt).toContain(SHAPE_QA_ALL_GREEN); + }); + + test('re-stamp swaps a Coding reviewer prompt whose preceding sentence AND call both changed', () => { + // The type→shape cutover rewrote both the "Use save_artifact every cycle…" + // sentence and the PR-link call in the Coding (and Research) reviewer + // prompts. Reversing the call alone leaves the new sentence, so the + // generated variant would not match the real dev prompt; the sentence and + // call must reverse together. + seedBuiltInWorkflows(SPACE_ID, manager, resolveAgentId); + const workflow = manager.listWorkflows(SPACE_ID).find((w) => w.name === CODING_WORKFLOW.name)!; + const reviewNode = workflow.nodes.find((n) => n.name === 'Review')!; + const templatePrompt = CODING_WORKFLOW.nodes.find((n) => n.name === 'Review')!.agents[0] + .customPrompt!.value; + const SHAPE_PR_EVERY_CYCLE = + 'Use save_artifact every cycle to record the PR as a `link` so post-approval dispatch can resolve it.\n\n'; + const RETIRED_EVERY_CYCLE = + 'Use save_artifact every cycle. Nest pr_url inside artifact data for post-approval dispatch.\n\n'; + const SHAPE_PR_LINK = 'save_artifact({ shape: "link", kind: "pr", data: { url: "" } })'; + const RETIRED_PR_LINK = 'save_artifact({ type: "result", data: { pr_url: "" } })'; + expect(templatePrompt).toContain(SHAPE_PR_EVERY_CYCLE); + expect(templatePrompt).toContain(SHAPE_PR_LINK); + const stalePrompt = templatePrompt + .replace(SHAPE_PR_EVERY_CYCLE, RETIRED_EVERY_CYCLE) + .replace(SHAPE_PR_LINK, RETIRED_PR_LINK); + expect(stalePrompt).not.toBe(templatePrompt); + expect(stalePrompt).toContain('save_artifact({ type: "result"'); + + manager.updateWorkflow(workflow.id, { + nodes: workflow.nodes.map((n) => + n.id !== reviewNode.id + ? n + : { + ...n, + agents: n.agents.map((a, i) => + i === 0 ? { ...a, customPrompt: { value: stalePrompt } } : a + ), + } + ), + }); + db.prepare(`UPDATE space_workflows SET template_hash = ? WHERE id = ?`).run( + 'stale-coding-reviewer-hash', + workflow.id + ); + + const result = seedBuiltInWorkflows(SPACE_ID, manager, resolveAgentId); + expect(result.restamped).toContain(CODING_WORKFLOW.name); + + const after = manager.getWorkflow(workflow.id)!; + const afterPrompt = after.nodes.find((n) => n.id === reviewNode.id)!.agents[0].customPrompt + ?.value; + expect(afterPrompt).toBe(templatePrompt); + expect(afterPrompt).toContain(SHAPE_PR_EVERY_CYCLE); + expect(afterPrompt).not.toContain('save_artifact({ type: "result"'); + }); + test('re-stamp patches exact retired built-in Fullstack reviewer prompt text', () => { seedBuiltInWorkflows(SPACE_ID, manager, resolveAgentId); const workflow = manager @@ -4453,7 +4616,7 @@ describe('REVIEW_ONLY_WORKFLOW reviewer customPrompt requires gh pr review befor const agent = REVIEW_ONLY_WORKFLOW.nodes[0].agents[0]; const prompt = agent.customPrompt!.value; expect(prompt).toContain('visible GitHub review'); - expect(prompt).toContain('save a result artifact'); + expect(prompt).toContain('record the PR'); }); }); diff --git a/packages/daemon/tests/unit/5-space/workflow/end-node-handoff.test.ts b/packages/daemon/tests/unit/5-space/workflow/end-node-handoff.test.ts index 70a8cdaf43..7e11f012b9 100644 --- a/packages/daemon/tests/unit/5-space/workflow/end-node-handoff.test.ts +++ b/packages/daemon/tests/unit/5-space/workflow/end-node-handoff.test.ts @@ -408,7 +408,7 @@ describe('Post-approval merger template (redesigned: report blockers to Reviewer test('escalates to space-agent only on cycle-cap or unresolvable blocker', () => { const text = PR_MERGE_POST_APPROVAL_INSTRUCTIONS; expect(text).toContain('send_message(target="space-agent"'); - expect(text).toContain('type: "merge_blocked"'); + expect(text).toContain('shape: "note", kind: "merge_blocked"'); expect(text).toContain('exit_reason'); expect(text).toContain('Do NOT mark the task complete'); }); @@ -422,7 +422,7 @@ describe('Post-approval merger template (redesigned: report blockers to Reviewer expect(text).toContain('IS_FORK='); expect(text).toContain('git push origin --delete'); expect(text).toMatch(/BEST-EFFORT/); - expect(text).toContain('type: "result"'); + expect(text).toContain('shape: "link", kind: "merge"'); }); test('preserved: root-repo sync uses {{workspace_path}}, branch-agnostic $BASE', () => { diff --git a/packages/shared/src/artifact-shapes.ts b/packages/shared/src/artifact-shapes.ts index d72260b1b6..57065cf852 100644 --- a/packages/shared/src/artifact-shapes.ts +++ b/packages/shared/src/artifact-shapes.ts @@ -206,7 +206,9 @@ export function normalizeLinkData(data: Record): Record): Record { ).toBe('gate:round-0'); }); - test('non-decision shapes ignore explicitKey (note stays a single rolling row)', () => { - expect(deriveArtifactKey('note', { text: 'a' }, 'round-5')).toBe('current'); + test('note without explicitKey stays a single rolling row; with explicitKey is multi-instance', () => { + // Default: single rolling 'current' row regardless of kind. + expect(deriveArtifactKey('note', { text: 'a' })).toBe('current'); + expect(deriveArtifactKey('note', { text: 'b', kind: 'status' })).toBe('current'); + // Explicit key opts into a bounded multi-instance audit trail, namespaced + // by kind so distinct note streams (e.g. merge-conflict attempts) never + // collapse onto each other. + expect(deriveArtifactKey('note', { text: 'a', kind: 'merge_conflict' }, 'attempt-0')).toBe( + 'merge_conflict:attempt-0' + ); + expect(deriveArtifactKey('note', { text: 'a' }, 'attempt-0')).toBe('attempt-0'); + }); + + test('other non-decision shapes ignore explicitKey (derived identity only)', () => { expect(deriveArtifactKey('link', { url: 'u', kind: 'pr' }, 'override')).toBe('pr'); expect(deriveArtifactKey('check', { name: 'ci', status: 'pass' }, 'override')).toBe('ci'); });