packages/workflows/src/evidence-validator.ts (1)

304-307: ⚡ Quick win

Rename the new validation log events to the repo’s domain.action_state format.

evidence_validation_started|failed|completed does not match the required event naming convention, so these entries will be inconsistent with the rest of the workflow logs.

Proposed fix

-    'evidence_validation_started'
+    'workflow.evidence_validation_started'
@@
-    getLog().error({ issues }, 'evidence_validation_failed');
+    getLog().error({ issues }, 'workflow.evidence_validation_failed');
@@
-    getLog().error({ err, issues }, 'evidence_validation_failed');
+    getLog().error({ err, issues }, 'workflow.evidence_validation_failed');
@@
-  getLog().info({ kind: shapeResult.evidence.kind }, 'evidence_validation_completed');
+  getLog().info({ kind: shapeResult.evidence.kind }, 'workflow.evidence_validation_completed');

As per coding guidelines, "Use Pino logger factory with event naming format {domain}.{action}_{state} (e.g., workflow.step_started, isolation.create_failed); always pair _started with _completed or _failed; include context IDs and error details".

Also applies to: 319-330, 347-386

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/workflows/src/evidence-validator.ts` around lines 304 - 307, The log
events emitted from getLog() in evidence-validator.ts (e.g., the call that
currently logs 'evidence_validation_started') must be renamed to follow the repo
convention {domain}.{action}_{state} and include relevant context IDs and error
details; update all occurrences around the getLog().info call and the related
blocks referenced (the started/failed/completed groups around lines ~304,
~319-330, ~347-386) so names become something like evidence.validation_started
-> evidence.validation_started? (use domain "evidence" and action "validation")
— actually rename to evidence.validation_started, evidence.validation_completed
and evidence.validation_failed (pairing each started with completed/failed), add
context fields (artifactsDir, policy.path, policy.verify, and a policy or
request id) to the log payload, and when logging failures include the caught
error object in the process (or getLog().error) payload so the error details are
recorded; adjust the corresponding getLog().info/getLog().error calls and ensure
consistent naming across all three blocks.

packages/workflows/src/dag-executor.ts (1)

2493-2497: ⚡ Quick win

Inline workflow shape duplicates WorkflowBase; prefer a named interface or a Pick<> from the schema type.

The parameter type is now { name: string; nodes: readonly DagNode[]; evidence_policy?: EvidencePolicy } & WorkflowLevelOptions. As more optional fields from workflowBaseSchema get plumbed down (this PR adds evidence_policy; the next field will follow), this anonymous shape will keep drifting from the actual schema-derived type. Defining an interface DagExecutorWorkflow (or using Pick<WorkflowBase, 'name' | 'nodes' | 'evidence_policy' | …> & WorkflowLevelOptions) would keep the contract in one place and align with the project's preference for interface over inline type.

As per coding guidelines: "In TypeScript, prefer interface for defining object shapes rather than type".

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/workflows/src/dag-executor.ts` around lines 2493 - 2497, The
anonymous workflow parameter type should be replaced with a named interface to
avoid drifting from the schema; define an interface (e.g. DagExecutorWorkflow)
that derives from the canonical schema type (either via "interface
DagExecutorWorkflow extends Pick<WorkflowBase, 'name' | 'nodes' |
'evidence_policy' | ...> & WorkflowLevelOptions" or by explicitly declaring the
interface and keeping it synced with workflowBaseSchema), then use that
interface in the function signature instead of the inline `{ name: string;
nodes: readonly DagNode[]; evidence_policy?: EvidencePolicy } &
WorkflowLevelOptions`; update references to the parameter type to use
DagExecutorWorkflow so future schema fields stay centralized.

packages/workflows/src/dag-executor.test.ts (2)

7032-7055: ⚡ Quick win

Use the real workflow run ID in the “valid evidence” fixture

Line 7037 hardcodes workflow_run_id: 'run-1' while the run under test is dag-evidence-run-2. Using the actual run ID makes this fixture semantically accurate and better aligned with future integrity checks.

🔧 Suggested fixture tweak

-  it('evidence_policy.required + valid evidence.json -> completeWorkflowRun', async () => {
+  it('evidence_policy.required + valid evidence.json -> completeWorkflowRun', async () => {
+    const workflowRun = makeWorkflowRun('dag-evidence-run-2');
     await writeFile(
       join(artifactsDir, 'evidence.json'),
       JSON.stringify({
         kind: 'execution',
-        workflow_run_id: 'run-1',
+        workflow_run_id: workflowRun.id,
         provider: 'claude',
         provider_run_ids: ['session-abc'],
         changed_files: ['src/foo.ts'],
         diff_command: 'git diff --stat origin/dev...HEAD',
         test_commands: ['bun test'],
         test_output_summary: '15 passed',
         commit_sha: 'a'.repeat(40),
         pushed_branch: 'feature/foo',
         pr_url: 'https://github.com/owner/repo/pull/42',
         pr_number: 42,
       })
     );
 
     const mockStore = createMockStore();
     const mockDeps = createMockDeps(mockStore);
     const platform = createMockPlatform();
-    const workflowRun = makeWorkflowRun('dag-evidence-run-2');

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/workflows/src/dag-executor.test.ts` around lines 7032 - 7055, The
evidence.json test fixture uses a hardcoded workflow_run_id ('run-1') which
doesn't match the run created for the test; update the fixture to use the actual
run ID from the test run (use the workflowRun created by
makeWorkflowRun('dag-evidence-run-2') — e.g., set workflow_run_id to
workflowRun.id or the literal 'dag-evidence-run-2') so the evidence.json aligns
with the run under test and any integrity checks that compare IDs will pass.

---

7016-7029: ⚡ Quick win

Assert evidence_validation.status to lock the metadata contract

These tests currently validate ok/issues but not the status field, so contract drift in metadata.evidence_validation.status can slip through undetected.

🧪 Suggested assertion additions

       expect(updates.metadata.evidence_validation.ok).toBe(false);
+      expect(updates.metadata.evidence_validation.status).toBe('failed');
       expect(Array.isArray(updates.metadata.evidence_validation.issues)).toBe(true);

       expect(updates.metadata.evidence_validation.ok).toBe(true);
+      expect(updates.metadata.evidence_validation.status).toBe('ok');

Also applies to: 7079-7091

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/workflows/src/dag-executor.test.ts` around lines 7016 - 7029, The
test currently checks evidence_validation.ok and issues but omits asserting
evidence_validation.status; update the assertions around the
mockStore.updateWorkflowRun inspection (the updateCalls/evidenceUpdate/updates
metadata block) to also assert updates.metadata.evidence_validation.status is
the expected status string (e.g., "failed") and that it's of type string, and
apply the same added assertion in the other analogous assertion block (the
similar check around the second occurrence referenced by lines 7079-7091) so the
metadata contract for evidence_validation.status is locked down.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

packages/workflows/src/dag-executor.ts (2)

3159-3165: ⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Avoid merging evidence metadata into the stale workflowRun.metadata snapshot.

Both writes spread the executor-start copy of workflowRun.metadata, so any metadata written during the run can be lost when evidence_validation is persisted. Re-read the latest metadata or use a partial/transactional merge API here.

Also applies to: 3199-3203

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/workflows/src/dag-executor.ts` around lines 3159 - 3165, The update
is merging into the stale executor-start snapshot by spreading
workflowRun.metadata when calling deps.store.updateWorkflowRun to set
metadata.evidence_validation; instead re-fetch the latest metadata (or use your
store's partial/transactional merge API) and merge evidence_validation into that
latest object before calling deps.store.updateWorkflowRun so concurrent metadata
writes aren't lost; apply the same change for the other occurrence that updates
metadata at the block around lines referencing deps.store.updateWorkflowRun and
evidence_validation (where evidenceResult.issues is used).

---

3144-3205: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Re-check run status after validateEvidence() returns.

verify: 'reality' can spend seconds in git/gh. If the run is cancelled or deleted during that window, this block can still flip it to failed or completed because the last skipIfStatusChanged() ran before validation started. Please guard again before either terminal write.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/workflows/src/dag-executor.ts` around lines 3144 - 3205, After
validateEvidence() returns, re-check the run status before doing any terminal
writes: call the existing skipIfStatusChanged(workflowRun.id) (or if
unavailable, re-fetch the run via deps.store.getWorkflowRun(workflowRun.id) and
ensure it is still running) and abort/return if the run was
cancelled/deleted/changed; only then proceed to call
deps.store.updateWorkflowRun(... evidence_validation ...) or
deps.store.failWorkflowRun(...) and to emit
events/safeSendMessage/logWorkflowError. Ensure this check is applied both on
the failure path (before persisting failure and sending notifications) and on
the success path (before persisting the success marker).

packages/workflows/src/evidence-validator.test.ts (1)

567-632: ⚡ Quick win

Please cover the non-ENOENT loader branches here.

validateEvidence() has dedicated handling for read failures like EISDIR/EACCES, but this suite only exercises missing files and malformed JSON. A couple of focused regression tests for those branches would protect the evidence_policy.path diagnostics on a changed critical path.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/workflows/src/evidence-validator.test.ts` around lines 567 - 632,
Add two tests to evidence-validator.test.ts that exercise the non-ENOENT
read-failure branches in validateEvidence: one that creates a directory at the
policy path (e.g., mkdir join(testDir, 'evidence.json')) and calls
validateEvidence(...) with policy.path set to that path to trigger an EISDIR
read error, and another that simulates an EACCES read error (either by creating
a file and chmod 0 on it or by stubbing fs.readFile to throw an { code: 'EACCES'
} error) and calling validateEvidence(...) with policy.path pointing to that
file; assert result.valid is false and that result.issues includes an issue with
field 'evidence_policy.path' for each case. Ensure you use the existing
helpers/testDir setup and reference validateEvidence and
loadEvidenceFromArtifacts in the new tests.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

packages/workflows/src/dag-executor.ts (2)

3160-3165: ⚠️ Potential issue | 🟡 Minor | ⚖️ Poor tradeoff

Stale workflowRun.metadata snapshot can silently drop concurrent writes.

Both metadata updates (lines 3162–3165 and 3201–3204) spread workflowRun.metadata, which is the in-memory snapshot captured at executor startup. Any metadata written during the run (e.g., approval/loop gate state) is silently overwritten when this spread is persisted.

Prefer fetching the freshest metadata from deps.store immediately before writing, or exposing a partial-merge API (e.g., setWorkflowRunMetadata(id, { evidence_validation: ... })) so the spread is unnecessary.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/workflows/src/dag-executor.ts` around lines 3160 - 3165, The update
currently spreads the stale in-memory snapshot workflowRun.metadata into
deps.store.updateWorkflowRun (when setting evidence_validation), which can
overwrite concurrent metadata changes; modify the write to first fetch the
latest metadata from deps.store (e.g.,
deps.store.getWorkflowRun/workflowRunById) and merge evidence_validation into
that fresh object before calling deps.store.updateWorkflowRun, or change to a
partial-merge API such as deps.store.setWorkflowRunMetadata(workflowRun.id, {
evidence_validation: ... }) so you avoid spreading workflowRun.metadata and
prevent lost concurrent writes.

---

3136-3193: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

TOCTOU: status is not re-checked after validateEvidence returns.

The guard at line 3136 (skipIfStatusChanged) runs before validateEvidence. When policy.verify === 'reality', that call can take several seconds (git/gh subprocesses). If the run is cancelled or deleted during that window, the failure branch at line 3173 will still call failWorkflowRun — overwriting a cancelled/deleted row with failed. The same window exists on the success path at line 3200.

The existing skipIfStatusChanged helper is already available; calling it again immediately after validateEvidence returns (on both branches) closes the window:

   const evidenceResult = await validateEvidence({ ... });
+  if (await skipIfStatusChanged('dag.skip_complete_status_changed')) return;
   if (!evidenceResult.valid) {
     ...
   }
   // success path
+  if (await skipIfStatusChanged('dag.skip_complete_status_changed')) return;
   await deps.store.updateWorkflowRun(...);

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/workflows/src/dag-executor.ts` around lines 3136 - 3193, The
time-of-check/time-of-use race occurs because
skipIfStatusChanged('dag.skip_complete_status_changed') runs before
validateEvidence() but not after it; add a second check immediately after
validateEvidence() returns and before taking either branch (i.e., before
persisting evidence_validation via deps.store.updateWorkflowRun, before
deps.store.failWorkflowRun/logWorkflowError/emitterForFail.emit/unregisterRun/safeSendMessage
on the failure path, and likewise before the success path that marks the run
completed). Concretely, call the existing
skipIfStatusChanged('dag.skip_complete_status_changed') right after the
validateEvidence(...) call and return early if it signals status changed to
prevent overwriting cancelled/deleted runs.

packages/workflows/src/dag-executor.test.ts (1)

7032-7091: ⚡ Quick win

Add a regression test for success-path metadata write failure propagation.

This suite validates the happy path, but it doesn’t pin the “do not silently complete if evidence metadata persistence fails” behavior. A focused test here would guard the reliability fix from regressions.

Suggested test addition

+  it('required evidence: metadata write failure on success path propagates (no silent completion)', async () => {
+    await writeFile(
+      join(artifactsDir, 'evidence.json'),
+      JSON.stringify({
+        kind: 'execution',
+        workflow_run_id: 'dag-evidence-run-db-fail',
+        provider: 'claude',
+        provider_run_ids: ['session-abc'],
+        changed_files: ['src/foo.ts'],
+        diff_command: 'git diff --stat origin/dev...HEAD',
+        test_commands: ['bun test'],
+        test_output_summary: '15 passed',
+        commit_sha: 'a'.repeat(40),
+        pushed_branch: 'feature/foo',
+        pr_url: 'https://github.com/owner/repo/pull/42',
+        pr_number: 42,
+      })
+    );
+
+    const mockStore = createMockStore();
+    (mockStore.updateWorkflowRun as ReturnType<typeof mock>).mockRejectedValueOnce(
+      new Error('db write failed')
+    );
+    const mockDeps = createMockDeps(mockStore);
+    const platform = createMockPlatform();
+    const workflowRun = makeWorkflowRun('dag-evidence-run-db-fail');
+
+    await expect(
+      executeDagWorkflow(
+        mockDeps,
+        platform,
+        'conv-evidence',
+        testDir,
+        {
+          name: 'evidence-required-valid-db-fail',
+          nodes: [{ id: 'work', bash: 'echo done' } as BashNode],
+          evidence_policy: { required: true, verify: 'shape', path: 'evidence.json' },
+        },
+        workflowRun,
+        'claude',
+        undefined,
+        artifactsDir,
+        join(testDir, 'logs'),
+        'main',
+        'docs/',
+        minimalConfig
+      )
+    ).rejects.toThrow('db write failed');
+
+    expect((mockStore.completeWorkflowRun as ReturnType<typeof mock>).mock.calls.length).toBe(0);
+  });

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/workflows/src/dag-executor.test.ts` around lines 7032 - 7091, Add a
regression test that simulates updateWorkflowRun failing when writing evidence
metadata so the happy-path does not silently complete; in the existing test case
around executeDagWorkflow for evidence_policy (the 'evidence-required-valid'
scenario) mock mockStore.updateWorkflowRun to throw/reject when called for
metadata updates, then call executeDagWorkflow and assert that
mockStore.completeWorkflowRun was NOT called and mockStore.failWorkflowRun WAS
called (or that the error propagated), and also assert the updateWorkflowRun
mock was invoked for evidence metadata. Target the same symbols used in the
file: executeDagWorkflow, mockStore.updateWorkflowRun,
mockStore.completeWorkflowRun, mockStore.failWorkflowRun, and the
evidence_policy/path handling so the test specifically verifies metadata
persistence failure prevents completion.

packages/workflows/src/evidence-validator.test.ts (2)

432-454: ⚡ Quick win

Add an explicit git ls-remote rejection test branch.

You cover empty output and mismatch, but not the thrown ls-remote path. That’s a distinct failure mode and worth pinning directly.

Suggested test addition

+  it('negative: git ls-remote rejects -> pushed_branch issue', async () => {
+    execFileImpl = (cmd, args) => {
+      if (cmd === 'git' && args[0] === 'cat-file') {
+        return Promise.resolve({ stdout: '', stderr: '' });
+      }
+      if (cmd === 'git' && args[0] === 'ls-remote') {
+        return Promise.reject(Object.assign(new Error('ls-remote failed'), { stderr: 'fatal: network error' }));
+      }
+      if (cmd === 'gh') {
+        return Promise.resolve({
+          stdout: JSON.stringify({
+            headRefName: 'feature/foo',
+            number: 42,
+            headRefOid: 'a'.repeat(40),
+          }),
+          stderr: '',
+        });
+      }
+      return Promise.resolve({ stdout: '', stderr: '' });
+    };
+
+    const issues = await verifyEvidenceReality(REAL_EVIDENCE, '/repo');
+    expect(issues.some(i => i.field === 'pushed_branch')).toBe(true);
+  });

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/workflows/src/evidence-validator.test.ts` around lines 432 - 454,
Add a new test case in packages/workflows/src/evidence-validator.test.ts that
specifically simulates a rejected git ls-remote call: in the new "it" (e.g.,
'negative: git ls-remote rejects → pushed_branch error') set execFileImpl so it
returns Promise.reject(new Error('ls-remote failed')) when cmd === 'git' &&
args[0] === 'ls-remote' (keep existing handling for 'git cat-file' and 'gh' as
in other tests), then call verifyEvidenceReality(REAL_EVIDENCE, '/repo') and
assert that the returned issues include an entry with field === 'pushed_branch';
reference the verifyEvidenceReality function, execFileImpl mock, and
REAL_EVIDENCE to locate where to add this test.

---

567-634: ⚡ Quick win

Add loader tests for non-ENOENT filesystem errors (e.g., EISDIR/EACCES).

loadEvidenceFromArtifacts currently checks missing/valid/malformed, but not other read failures. Pinning these keeps the “structured issues, no crash” contract safe.

Suggested deterministic branch test (EISDIR)

+  it('directory at evidence path is handled without throw (EISDIR path)', async () => {
+    await mkdir(join(testDir, 'evidence.json'), { recursive: true });
+    const result = await validateEvidence({
+      artifactsDir: testDir,
+      cwd: '/repo',
+      workflowRunId: 'run-1',
+      policy: { required: true, verify: 'shape', path: 'evidence.json' },
+    });
+    expect(result.valid).toBe(false);
+    if (!result.valid) {
+      expect(result.issues.some(i => i.field === 'evidence_policy.path')).toBe(true);
+    }
+  });

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/workflows/src/evidence-validator.test.ts` around lines 567 - 634,
Add a deterministic test that simulates non-ENOENT fs errors by mocking
fs.promises.readFile (for the evidence.json path) to throw an error with code
'EISDIR' (and another case for 'EACCES'), then call loadEvidenceFromArtifacts
and assert it does not throw and returns a structured result (i.e., a handled
response rather than crashing — e.g., result.found truthy and result.raw is a
string or error-info), and also add a companion validateEvidence call (using
validateEvidence with a policy) to assert validation returns a non-valid result
with an appropriate issue reported; reference the existing test patterns and the
functions loadEvidenceFromArtifacts and validateEvidence and the artifact
filename 'evidence.json' when adding the new tests.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes