diff --git a/action.yml b/action.yml index 410a5d3c3..1a5c51093 100644 --- a/action.yml +++ b/action.yml @@ -55,6 +55,10 @@ inputs: description: 'Maximum number of concurrent trigger executions' required: false default: '5' + action-ref: + description: 'The action ref that produced this run, surfaced in the findings output harness field' + required: false + default: ${{ github.action_ref }} outputs: findings-count: @@ -90,4 +94,5 @@ runs: INPUT_REQUEST_CHANGES: ${{ inputs.request-changes }} INPUT_FAIL_CHECK: ${{ inputs.fail-check }} INPUT_PARALLEL: ${{ inputs.parallel }} + INPUT_ACTION_REF: ${{ inputs.action-ref }} run: node ${{ github.action_path }}/dist/action/index.js diff --git a/packages/warden/src/action/inputs.test.ts b/packages/warden/src/action/inputs.test.ts index 0d29906e4..43528f222 100644 --- a/packages/warden/src/action/inputs.test.ts +++ b/packages/warden/src/action/inputs.test.ts @@ -155,6 +155,19 @@ describe('parseActionInputs', () => { expect(() => parseActionInputs()).toThrow('Invalid mode "later"'); }); }); + + describe('actionRef', () => { + it('parses action-ref when provided', () => { + process.env['INPUT_ACTION_REF'] = 'getsentry/warden@v1'; + const inputs = parseActionInputs(); + expect(inputs.actionRef).toBe('getsentry/warden@v1'); + }); + + it('is undefined when not provided', () => { + const inputs = parseActionInputs(); + expect(inputs.actionRef).toBeUndefined(); + }); + }); }); describe('setupAuthEnv', () => { diff --git a/packages/warden/src/action/inputs.ts b/packages/warden/src/action/inputs.ts index da7b43a93..52cc27dc0 100644 --- a/packages/warden/src/action/inputs.ts +++ b/packages/warden/src/action/inputs.ts @@ -39,6 +39,8 @@ export interface ActionInputs { failCheck?: boolean; /** Max concurrent trigger executions */ parallel: number; + /** The action ref that produced this run, surfaced in the findings output harness field */ + actionRef?: string; } // ----------------------------------------------------------------------------- @@ -127,6 +129,7 @@ export function parseActionInputs(): ActionInputs { requestChanges, failCheck, parallel: Number.isNaN(parallelParsed) ? DEFAULT_CONCURRENCY : parallelParsed, + actionRef: getInput('action-ref') || undefined, }; } diff --git a/packages/warden/src/action/reporting/outcomes.ts b/packages/warden/src/action/reporting/outcomes.ts index 2250ac085..66956e238 100644 --- a/packages/warden/src/action/reporting/outcomes.ts +++ b/packages/warden/src/action/reporting/outcomes.ts @@ -11,7 +11,7 @@ export type FindingOutcome = export type DedupeSource = 'warden' | 'external'; export type DedupeMatchType = 'hash' | 'semantic'; -export type SkippedReason = 'max_findings' | 'duplicate_in_batch' | 'no_inline_location'; +export type SkippedReason = 'max_findings' | 'duplicate_in_batch' | 'no_inline_location' | 'review_not_posted'; export type ResolvedReason = 'fix_evaluation' | 'stale_check'; export const DedupeDetailSchema = z.object({ @@ -21,6 +21,8 @@ export const DedupeDetailSchema = z.object({ existingCommentId: z.number().int().positive().optional(), existingThreadId: z.string().optional(), existingResolved: z.boolean().optional(), + /** Skills already attributed to the matched comment, parsed from its attribution footer. */ + existingSkills: z.array(z.string()).optional(), actor: z.string().optional(), }); @@ -29,6 +31,8 @@ export type DedupeDetail = z.infer; interface BaseFindingObservation { finding: Finding; skill?: string; + /** skillExecutionId of the trigger execution that produced this observation. */ + skillExecutionId?: string; } export interface PostedFindingObservation extends BaseFindingObservation { @@ -66,29 +70,34 @@ export const FindingObservationSchema = z.discriminatedUnion('outcome', [ outcome: z.literal('posted'), finding: FindingSchema, skill: z.string().optional(), + skillExecutionId: z.string().optional(), }), z.object({ outcome: z.literal('deduped'), finding: FindingSchema, skill: z.string().optional(), + skillExecutionId: z.string().optional(), dedupe: DedupeDetailSchema, }), z.object({ outcome: z.literal('skipped'), finding: FindingSchema, skill: z.string().optional(), - skippedReason: z.enum(['max_findings', 'duplicate_in_batch', 'no_inline_location']), + skillExecutionId: z.string().optional(), + skippedReason: z.enum(['max_findings', 'duplicate_in_batch', 'no_inline_location', 'review_not_posted']), }), z.object({ outcome: z.literal('resolved'), finding: FindingSchema, skill: z.string().optional(), + skillExecutionId: z.string().optional(), resolvedReason: z.enum(['fix_evaluation', 'stale_check']), }), z.object({ outcome: z.literal('failed'), finding: FindingSchema, skill: z.string().optional(), + skillExecutionId: z.string().optional(), }), ]); diff --git a/packages/warden/src/action/reporting/output.test.ts b/packages/warden/src/action/reporting/output.test.ts index 7ced469bd..616081b02 100644 --- a/packages/warden/src/action/reporting/output.test.ts +++ b/packages/warden/src/action/reporting/output.test.ts @@ -1,6 +1,6 @@ import { describe, expect, it } from 'vitest'; import type { EventContext, Finding, SkillReport } from '../../types/index.js'; -import { buildFindingsOutput, FindingsOutputSchema } from './output.js'; +import { buildFindingsOutput, buildResolvedDefaults, FindingsOutputSchema } from './output.js'; describe('findings output schema', () => { it('builds a schema-valid public findings payload', () => { @@ -20,6 +20,108 @@ describe('findings output schema', () => { totalFindings: 1, findingsBySeverity: { high: 1, medium: 0, low: 0 }, totalSkills: 1, + totalSkillExecutions: 1, + byOutcome: { posted: 1, deduped: 0, skipped: 0, resolved: 0, failed: 0 }, + }); + }); + + it('produces the exact pre-existing shape when none of the new inputs are available', () => { + const context = createContext(); + const finding = createFinding(); + const report = createReport({ findings: [finding] }); + + // GitHub Actions always sets GITHUB_RUN_ATTEMPT, but this assertion is + // specifically about the shape when nothing is available — isolate it + // from the ambient environment rather than relying on it being unset. + const originalRunAttempt = process.env['GITHUB_RUN_ATTEMPT']; + delete process.env['GITHUB_RUN_ATTEMPT']; + let output: ReturnType; + try { + output = buildFindingsOutput([report], context, [], { + timestamp: '2026-01-01T00:00:00.000Z', + runId: '123', + }); + } finally { + if (originalRunAttempt === undefined) { + delete process.env['GITHUB_RUN_ATTEMPT']; + } else { + process.env['GITHUB_RUN_ATTEMPT'] = originalRunAttempt; + } + } + + expect(output).toEqual({ + version: '1', + timestamp: '2026-01-01T00:00:00.000Z', + runAttempt: undefined, + harness: { name: 'warden', version: expect.any(String), actionRef: undefined }, + repository: { + owner: context.repository.owner, + name: context.repository.name, + fullName: context.repository.fullName, + }, + event: context.eventType, + pullRequest: { + number: context.pullRequest!.number, + author: context.pullRequest!.author, + title: context.pullRequest!.title, + baseBranch: context.pullRequest!.baseBranch, + headBranch: context.pullRequest!.headBranch, + headSha: context.pullRequest!.headSha, + }, + runId: '123', + resolvedDefaults: undefined, + skippedTriggers: undefined, + summary: { + totalFindings: 1, + findingsBySeverity: { high: 1, medium: 0, low: 0 }, + totalSkills: 1, + totalSkillExecutions: 1, + byOutcome: { posted: 0, deduped: 0, skipped: 0, resolved: 0, failed: 0 }, + }, + skills: [ + { + name: report.skill, + summary: report.summary, + model: undefined, + auxiliaryModel: undefined, + synthesisModel: undefined, + durationMs: undefined, + usage: undefined, + failedHunks: undefined, + failedExtractions: undefined, + error: undefined, + skillExecutionId: undefined, + triggerId: undefined, + triggerName: undefined, + findingsBySeverity: { high: 1, medium: 0, low: 0 }, + checkRunUrl: undefined, + checkRunId: undefined, + reviewEvent: undefined, + checkConclusion: undefined, + issueNumber: undefined, + issueUrl: undefined, + findings: [ + { + id: finding.id, + reportedId: undefined, + severity: finding.severity, + confidence: finding.confidence, + title: finding.title, + description: finding.description, + verification: undefined, + location: finding.location, + additionalLocations: undefined, + sourceSnippet: undefined, + contentHash: expect.any(String), + reportedBy: undefined, + provenance: undefined, + }, + ], + }, + ], + discardedFindings: undefined, + triggerResults: undefined, + findingObservations: [], }); }); @@ -227,9 +329,413 @@ describe('findings output schema', () => { expect('failedExtractions' in serialized).toBe(false); expect('error' in serialized).toBe(false); }); + + it('includes harness/resolvedDefaults/skippedTriggers when provided', () => { + const output = buildFindingsOutput([createReport()], createContext(), [], { + timestamp: '2026-01-01T00:00:00.000Z', + runId: '123', + runAttempt: '2', + actionRef: 'babylist/warden@v1', + resolvedDefaults: { failOn: 'high', maxFindings: 25 }, + skippedTriggers: [ + { skillName: 'style-guide', triggerId: 'trg-1', triggerName: 'style-guide', reason: 'path_filter' }, + ], + }); + + expect(FindingsOutputSchema.parse(output)).toEqual(output); + expect(output.runAttempt).toBe('2'); + expect(output.harness).toEqual({ name: 'warden', version: expect.any(String), actionRef: 'babylist/warden@v1' }); + expect(output.resolvedDefaults).toEqual({ failOn: 'high', maxFindings: 25 }); + expect(output.skippedTriggers).toEqual([ + { skillName: 'style-guide', triggerId: 'trg-1', triggerName: 'style-guide', reason: 'path_filter' }, + ]); + }); + + it('falls back to GITHUB_RUN_ATTEMPT when no runAttempt option is passed', () => { + const original = process.env['GITHUB_RUN_ATTEMPT']; + process.env['GITHUB_RUN_ATTEMPT'] = '3'; + + try { + const output = buildFindingsOutput([createReport()], createContext(), [], { + timestamp: '2026-01-01T00:00:00.000Z', + runId: '123', + }); + + expect(output.runAttempt).toBe('3'); + } finally { + if (original === undefined) { + delete process.env['GITHUB_RUN_ATTEMPT']; + } else { + process.env['GITHUB_RUN_ATTEMPT'] = original; + } + } + }); + + it('passes through the verification field already carried on Finding', () => { + const finding = createFinding(); + finding.verification = '- traced the guard clause at line 42'; + const output = buildFindingsOutput([createReport({ findings: [finding] })], createContext(), [], { + timestamp: '2026-01-01T00:00:00.000Z', + runId: '123', + }); + + expect(output.skills[0]?.findings[0]?.verification).toBe('- traced the guard clause at line 42'); + }); + + it('mirrors reportedId onto id only when dedupe/recenter has stamped it', () => { + const untouched = createFinding(); + const recentered = { ...createFinding(), id: 'existing-comment-id', reportedId: 'existing-comment-id' }; + const output = buildFindingsOutput( + [createReport({ findings: [untouched, recentered] })], + createContext(), + [], + { timestamp: '2026-01-01T00:00:00.000Z', runId: '123' } + ); + + expect(output.skills[0]?.findings[0]?.reportedId).toBeUndefined(); + expect(output.skills[0]?.findings[1]?.reportedId).toBe('existing-comment-id'); + }); + + it('attaches skillExecutionId, triggerId, posting-derived fields, and primary reportedBy from skillExecutions', () => { + const report = createReport(); + const output = buildFindingsOutput([report], createContext(), [], { + timestamp: '2026-01-01T00:00:00.000Z', + runId: '123', + skillExecutions: [ + { + report, + skillExecutionId: 'exec-abc', + triggerId: 'trg-1', + triggerName: 'security-check', + checkRunUrl: 'https://github.com/check/1', + checkRunId: 1, + reviewEvent: 'COMMENT', + checkConclusion: 'success', + }, + ], + }); + + expect(FindingsOutputSchema.parse(output)).toEqual(output); + const skill = output.skills[0]; + expect(skill?.skillExecutionId).toBe('exec-abc'); + expect(skill?.triggerId).toBe('trg-1'); + expect(skill?.triggerName).toBe('security-check'); + expect(skill?.checkRunUrl).toBe('https://github.com/check/1'); + expect(skill?.checkRunId).toBe(1); + expect(skill?.reviewEvent).toBe('COMMENT'); + expect(skill?.checkConclusion).toBe('success'); + expect(skill?.findings[0]?.reportedBy).toEqual([ + { skillExecutionId: 'exec-abc', skillName: 'test-skill', role: 'primary' }, + ]); + }); + + it('adds corroborating reportedBy entries from a deduped finding observation', () => { + const finding = createFinding(); + const report = createReport({ findings: [finding] }); + const output = buildFindingsOutput( + [report], + createContext(), + [ + { + // A cross-run dedupe: some other skill's finding matched this + // exact survivor (by its own id) on a prior run. + outcome: 'deduped', + finding: createFinding({ id: 'other-run-finding-id' }), + skill: 'other-skill', + dedupe: { + source: 'warden', + matchType: 'hash', + existingFindingId: finding.id, + existingSkills: ['test-skill', 'other-skill'], + }, + }, + ], + { + timestamp: '2026-01-01T00:00:00.000Z', + runId: '123', + skillExecutions: [{ report, skillExecutionId: 'exec-abc' }], + } + ); + + expect(output.skills[0]?.findings[0]?.reportedBy).toEqual([ + { skillExecutionId: 'exec-abc', skillName: 'test-skill', role: 'primary' }, + { skillName: 'other-skill', role: 'corroborating', matchType: 'hash' }, + ]); + }); + + it('does not attribute dedupe corroboration to an unrelated finding that merely shares title+description', () => { + // Regression: two findings with identical wording but different locations + // and no relationship to each other. Only the one at the deduped + // location should inherit reportedBy corroboration. + const survivor = createFinding({ id: 'a', location: { path: 'src/a.ts', startLine: 1 } }); + const dedupedElsewhere = createFinding({ id: 'b', location: { path: 'src/b.ts', startLine: 99 } }); + const report = createReport({ findings: [survivor] }); + + const output = buildFindingsOutput( + [report], + createContext(), + [ + { + outcome: 'deduped', + finding: dedupedElsewhere, + skill: 'other-skill', + dedupe: { + source: 'warden', + matchType: 'hash', + existingFindingId: 'prior-id', + existingSkills: ['other-skill', 'some-prior-skill'], + }, + }, + ], + { + timestamp: '2026-01-01T00:00:00.000Z', + runId: '123', + skillExecutions: [{ report, skillExecutionId: 'exec-abc' }], + } + ); + + expect(output.skills[0]?.findings[0]?.reportedBy).toEqual([ + { skillExecutionId: 'exec-abc', skillName: 'test-skill', role: 'primary' }, + ]); + }); + + it('omits reportedBy entirely when no skillExecutions metadata is given', () => { + const output = buildFindingsOutput([createReport()], createContext(), [], { + timestamp: '2026-01-01T00:00:00.000Z', + runId: '123', + }); + + expect(output.skills[0]?.findings[0]?.reportedBy).toBeUndefined(); + }); + + it('builds provenance for a revised finding and discardedFindings for rejected/merged candidates', () => { + const survivor = createFinding(); + const rejectedFinding = { ...createFinding(), id: 'rejected-1', title: 'Rejected finding' }; + const absorbedFinding = { ...createFinding(), id: 'absorbed-1', title: 'Absorbed finding' }; + const report = createReport({ findings: [survivor] }); + + const output = buildFindingsOutput([report], createContext(), [], { + timestamp: '2026-01-01T00:00:00.000Z', + runId: '123', + skillExecutions: [ + { + report, + skillExecutionId: 'exec-abc', + findingProcessingEvents: [ + { + stage: 'verification', + action: 'revised', + finding: { ...survivor, title: 'Original title', severity: 'low' }, + replacement: survivor, + reason: 'narrowed after tracing the guard clause', + }, + { stage: 'verification', action: 'rejected', finding: rejectedFinding, reason: 'not reproducible' }, + { stage: 'merge', action: 'merged', finding: absorbedFinding, replacement: survivor, reason: 'same root cause' }, + ], + }, + ], + }); + + expect(FindingsOutputSchema.parse(output)).toEqual(output); + expect(output.skills[0]?.findings[0]?.provenance).toEqual({ + originSkillExecutionId: 'exec-abc', + originModel: undefined, + verification: { + outcome: 'revised', + model: undefined, + evidence: 'narrowed after tracing the guard clause', + before: { title: 'Original title', description: survivor.description, severity: 'low', confidence: survivor.confidence }, + }, + merge: { model: undefined, absorbedFindingIds: ['absorbed-1'] }, + }); + expect(output.discardedFindings).toEqual([ + { + originSkillExecutionId: 'exec-abc', + stage: 'verification_rejected', + severity: rejectedFinding.severity, + title: 'Rejected finding', + location: rejectedFinding.location, + model: undefined, + reason: 'not reproducible', + survivorFindingId: undefined, + }, + { + originSkillExecutionId: 'exec-abc', + stage: 'merge_absorbed', + severity: absorbedFinding.severity, + title: 'Absorbed finding', + location: absorbedFinding.location, + model: undefined, + reason: 'same root cause', + survivorFindingId: survivor.id, + }, + ]); + }); + + it('omits discardedFindings when there is nothing to discard', () => { + const output = buildFindingsOutput([createReport()], createContext(), [], { + timestamp: '2026-01-01T00:00:00.000Z', + runId: '123', + }); + + expect(output.discardedFindings).toBeUndefined(); + expect('discardedFindings' in JSON.parse(JSON.stringify(output))).toBe(false); + }); + + it('keeps each skill execution\'s provenance independent when same-run recentering shares a finding id', () => { + // Regression: same-run dedupe can recenter two different skills' survivor + // findings onto the same id. A run-global provenance map keyed only by + // finding id would let the second skill's event overwrite the first's. + const sharedId = 'shared-comment-id'; + const findingA = createFinding({ id: sharedId }); + const findingB = createFinding({ id: sharedId, title: 'Different wording' }); + const reportA = createReport({ skill: 'skill-a', findings: [findingA] }); + const reportB = createReport({ skill: 'skill-b', findings: [findingB] }); + + const output = buildFindingsOutput([reportA, reportB], createContext(), [], { + timestamp: '2026-01-01T00:00:00.000Z', + runId: '123', + skillExecutions: [ + { + report: reportA, + skillExecutionId: 'exec-a', + findingProcessingEvents: [ + { + stage: 'verification', + action: 'revised', + finding: { ...findingA, title: 'Original A title', severity: 'low' }, + replacement: findingA, + reason: 'narrowed A', + }, + ], + }, + { + report: reportB, + skillExecutionId: 'exec-b', + findingProcessingEvents: [ + { + stage: 'verification', + action: 'revised', + finding: { ...findingB, title: 'Original B title', severity: 'medium' }, + replacement: findingB, + reason: 'narrowed B', + }, + ], + }, + ], + }); + + expect(output.skills[0]?.findings[0]?.provenance?.verification?.before.title).toBe('Original A title'); + expect(output.skills[1]?.findings[0]?.provenance?.verification?.before.title).toBe('Original B title'); + }); + + it('attributes provenance to the model that ran each stage, not the primary analysis model', () => { + const survivor = createFinding(); + const absorbed = { ...createFinding(), id: 'absorbed-1' }; + const report = createReport({ skill: 'security-skill', findings: [survivor], model: 'primary-model' }); + + const output = buildFindingsOutput([report], createContext(), [], { + timestamp: '2026-01-01T00:00:00.000Z', + runId: '123', + skillExecutions: [ + { + report, + skillExecutionId: 'exec-abc', + auxiliaryModel: 'verification-model', + synthesisModel: 'merge-model', + findingProcessingEvents: [ + { + stage: 'verification', + action: 'revised', + finding: { ...survivor, title: 'Original title' }, + replacement: survivor, + reason: 'narrowed', + }, + { stage: 'merge', action: 'merged', finding: absorbed, replacement: survivor, reason: 'same root cause' }, + ], + }, + ], + }); + + const provenance = output.skills[0]?.findings[0]?.provenance; + expect(provenance?.originModel).toBe('primary-model'); + expect(provenance?.verification?.model).toBe('verification-model'); + expect(provenance?.merge?.model).toBe('merge-model'); + expect(output.discardedFindings?.[0]).toMatchObject({ stage: 'merge_absorbed', model: 'merge-model' }); + }); + + it('joins same-run cross-skill dedupe by existingFindingId, including semantic matches with a different content hash', () => { + const survivorFinding = createFinding({ + id: 'survivor-id', + title: 'SQL injection risk', + description: 'user input concatenated into query', + }); + const duplicateFinding = createFinding({ + id: 'dup-id', + title: 'Unsanitized SQL input', + description: 'raw value passed to db.query', + }); + const survivorReport = createReport({ skill: 'security-skill', findings: [survivorFinding] }); + + const output = buildFindingsOutput( + [survivorReport], + createContext(), + [ + { + outcome: 'deduped', + finding: duplicateFinding, + skill: 'style-skill', + dedupe: { + source: 'warden', + matchType: 'semantic', + existingFindingId: survivorFinding.id, + existingSkills: ['security-skill'], + }, + }, + ], + { + timestamp: '2026-01-01T00:00:00.000Z', + runId: '123', + skillExecutions: [{ report: survivorReport, skillExecutionId: 'exec-security' }], + } + ); + + expect(output.skills[0]?.findings[0]?.reportedBy).toEqual([ + { skillExecutionId: 'exec-security', skillName: 'security-skill', role: 'primary' }, + { skillName: 'style-skill', role: 'corroborating', matchType: 'semantic' }, + ]); + }); }); -function createFinding(): Finding { +describe('buildResolvedDefaults', () => { + it('extracts the five resolved-default fields from action inputs', () => { + expect(buildResolvedDefaults({ + failOn: 'high', + reportOn: 'medium', + failCheck: true, + requestChanges: false, + maxFindings: 25, + })).toEqual({ + failOn: 'high', + reportOn: 'medium', + failCheck: true, + requestChanges: false, + maxFindings: 25, + }); + }); + + it('carries through undefined optional fields', () => { + expect(buildResolvedDefaults({ maxFindings: 50 })).toEqual({ + failOn: undefined, + reportOn: undefined, + failCheck: undefined, + requestChanges: undefined, + maxFindings: 50, + }); + }); +}); + +function createFinding(overrides: Partial = {}): Finding { return { id: 'WRD-001', severity: 'high', @@ -237,6 +743,7 @@ function createFinding(): Finding { title: 'Finding title', description: 'Finding description', location: { path: 'src/index.ts', startLine: 1 }, + ...overrides, }; } diff --git a/packages/warden/src/action/reporting/output.ts b/packages/warden/src/action/reporting/output.ts index 2cce40cb0..f4d82a190 100644 --- a/packages/warden/src/action/reporting/output.ts +++ b/packages/warden/src/action/reporting/output.ts @@ -5,22 +5,88 @@ import { FindingSchema, GitHubEventTypeSchema, LocationSchema, + SeverityThresholdSchema, SkillErrorSchema, SourceSnippetSchema, UsageStatsSchema, } from '../../types/index.js'; -import type { FindingObservation } from './outcomes.js'; +import type { DedupeMatchType, FindingObservation } from './outcomes.js'; import { FindingObservationSchema } from './outcomes.js'; +import { generateContentHash } from '../../output/dedup.js'; +import { getVersion } from '../../utils/version.js'; +import { + buildProvenanceAndDiscarded, + DiscardedFindingSchema, + FindingProvenanceSchema, + provenanceKey, +} from './provenance.js'; +import type { FindingExecutionEvents } from './provenance.js'; +import type { FindingProcessingEvent } from '../../sdk/types.js'; + +const FindingAttributionSchema = z.object({ + skillExecutionId: z.string().optional(), + skillName: z.string(), + role: z.enum(['primary', 'corroborating']), + matchType: z.enum(['hash', 'semantic']).optional(), +}); const ExportedFindingSchema = z.object({ id: z.string(), + /** Set to the same value as `id` once dedupe/recenter matches this finding to an already-posted comment. */ + reportedId: z.string().optional(), severity: FindingSchema.shape.severity, confidence: FindingSchema.shape.confidence, title: z.string(), description: z.string(), + /** Verifier's evidence trace, when a verification pass ran. */ + verification: z.string().optional(), location: LocationSchema.optional(), additionalLocations: z.array(LocationSchema).optional(), sourceSnippet: SourceSnippetSchema.optional(), + /** Stable cross-run key, same value `output/dedup.ts` uses for hash-based dedupe. */ + contentHash: z.string().optional(), + /** Skills that independently flagged this finding, self included as `role: 'primary'`. */ + reportedBy: z.array(FindingAttributionSchema).optional(), + provenance: FindingProvenanceSchema.optional(), +}); + +const HarnessSchema = z.object({ + name: z.literal('warden'), + version: z.string(), + actionRef: z.string().optional(), +}); + +/** + * Action-level fallbacks every trigger falls back to when its own config + * doesn't override them. Deliberately narrower than a per-trigger + * ResolvedTrigger: model/runtime/minConfidence/verifyFindings are resolved + * per skill/trigger in this repo, not at the action level, so there's no + * single run-wide value to report for them here. + */ +const ResolvedDefaultsSchema = z.object({ + failOn: SeverityThresholdSchema.optional(), + reportOn: SeverityThresholdSchema.optional(), + failCheck: z.boolean().optional(), + requestChanges: z.boolean().optional(), + maxFindings: z.number().int().nonnegative().optional(), +}); + +export const SkippedTriggerReasonSchema = z.enum([ + 'no_event_match', + 'path_filter', + 'draft_state', + 'label_mismatch', + 'no_changes', + 'pending', + /** The trigger matched and ran, but threw before producing a report. */ + 'error', +]); + +const SkippedTriggerSchema = z.object({ + skillName: z.string(), + triggerId: z.string().optional(), + triggerName: z.string().optional(), + reason: SkippedTriggerReasonSchema, }); const TriggerErrorSchema = z.object({ @@ -28,6 +94,15 @@ const TriggerErrorSchema = z.object({ message: z.string(), }); +/** Mirrors `FindingProcessingEvent` (sdk/types.ts) so it can round-trip through the analyze/report replay artifact. */ +const ReplayFindingProcessingEventSchema = z.object({ + stage: z.enum(['dedupe', 'verification', 'merge', 'fix_gate']), + action: z.enum(['dropped', 'rejected', 'revised', 'merged', 'stripped_fix']), + finding: FindingSchema, + reason: z.string().optional(), + replacement: FindingSchema.optional(), +}); + // Durable analyze/report replay rows join by triggerName plus configured // skillName. `report.skill` is preserved as report identity and may differ for // local path skills with frontmatter names. @@ -52,6 +127,11 @@ export const TriggerRunResultSchema = z.discriminatedUnion('status', [ status: z.literal('success'), report: ReplaySkillReportSchema, error: z.never().optional(), + /** Verification/merge/dedupe events captured during analyze mode, replayed so report mode's export still carries provenance/discardedFindings. */ + findingProcessingEvents: z.array(ReplayFindingProcessingEventSchema).optional(), + /** Analyze mode's model lanes, replayed so report mode's export doesn't drop them. */ + auxiliaryModel: z.string().optional(), + synthesisModel: z.string().optional(), }), TriggerRunResultBaseSchema.extend({ status: z.literal('error'), @@ -63,6 +143,9 @@ export const TriggerRunResultSchema = z.discriminatedUnion('status', [ export const FindingsOutputSchema = z.object({ version: z.literal('1'), timestamp: z.string().datetime(), + runAttempt: z.string().optional(), + /** Which build of Warden produced this run. */ + harness: HarnessSchema.optional(), repository: z.object({ owner: z.string(), name: z.string(), @@ -78,6 +161,10 @@ export const FindingsOutputSchema = z.object({ headSha: z.string(), }).optional(), runId: z.string(), + /** The model/threshold config this run resolved to at the action level. */ + resolvedDefaults: ResolvedDefaultsSchema.optional(), + /** Configured triggers that never fired this run, with why. */ + skippedTriggers: z.array(SkippedTriggerSchema).optional(), summary: z.object({ totalFindings: z.number().int().nonnegative(), findingsBySeverity: z.object({ @@ -86,18 +173,47 @@ export const FindingsOutputSchema = z.object({ low: z.number().int().nonnegative(), }), totalSkills: z.number().int().nonnegative(), + totalSkillExecutions: z.number().int().nonnegative().optional(), + byOutcome: z.object({ + posted: z.number().int().nonnegative(), + deduped: z.number().int().nonnegative(), + skipped: z.number().int().nonnegative(), + resolved: z.number().int().nonnegative(), + failed: z.number().int().nonnegative(), + }).optional(), }), skills: z.array(z.object({ name: z.string(), summary: z.string(), model: z.string().optional(), + auxiliaryModel: z.string().optional(), + synthesisModel: z.string().optional(), durationMs: z.number().nonnegative().optional(), usage: UsageStatsSchema.optional(), failedHunks: z.number().int().nonnegative().optional(), failedExtractions: z.number().int().nonnegative().optional(), error: SkillErrorSchema.optional(), + /** Stable id for this skill×trigger execution. */ + skillExecutionId: z.string().optional(), + triggerId: z.string().optional(), + triggerName: z.string().optional(), + findingsBySeverity: z.object({ + high: z.number().int().nonnegative(), + medium: z.number().int().nonnegative(), + low: z.number().int().nonnegative(), + }).optional(), + checkRunUrl: z.string().optional(), + checkRunId: z.number().int().positive().optional(), + /** Posting-derived; absent from analyze-mode replay and live writes. */ + reviewEvent: z.enum(['APPROVE', 'REQUEST_CHANGES', 'COMMENT']).optional(), + checkConclusion: z.enum(['success', 'failure', 'neutral', 'cancelled']).optional(), + /** Schedule-mode only. */ + issueNumber: z.number().int().positive().optional(), + issueUrl: z.string().optional(), findings: z.array(ExportedFindingSchema), })), + /** Verifier-rejected and merge-absorbed candidates that never reached `findings[]`. */ + discardedFindings: z.array(DiscardedFindingSchema).optional(), triggerResults: z.array(TriggerRunResultSchema).optional(), findingObservations: z.array(FindingObservationSchema), }); @@ -110,12 +226,72 @@ export interface ReplayTriggerResult { skillName: string; report?: SkillReport; error?: unknown; + findingProcessingEvents?: FindingProcessingEvent[]; + auxiliaryModel?: string; + synthesisModel?: string; +} + +/** Per-execution metadata for one `reports[]` entry, matched by object identity. */ +export interface SkillExecutionMeta { + report: SkillReport; + skillExecutionId?: string; + triggerId?: string; + triggerName?: string; + auxiliaryModel?: string; + synthesisModel?: string; + checkRunUrl?: string; + checkRunId?: number; + reviewEvent?: 'APPROVE' | 'REQUEST_CHANGES' | 'COMMENT'; + checkConclusion?: 'success' | 'failure' | 'neutral' | 'cancelled'; + issueNumber?: number; + issueUrl?: string; + findingProcessingEvents?: FindingProcessingEvent[]; } -interface BuildFindingsOutputOptions { +export interface BuildFindingsOutputOptions { timestamp?: string; runId?: string; + runAttempt?: string; + actionRef?: string; triggerResults?: ReplayTriggerResult[]; + resolvedDefaults?: z.infer; + skippedTriggers?: z.infer[]; + /** Per-execution metadata (skillExecutionId, posting-derived fields, captured provenance events) matched to `reports[]` by object identity. */ + skillExecutions?: SkillExecutionMeta[]; +} + +/** Build the action-level `resolvedDefaults` block from parsed action inputs. */ +export function buildResolvedDefaults(inputs: { + failOn?: z.infer; + reportOn?: z.infer; + failCheck?: boolean; + requestChanges?: boolean; + maxFindings: number; +}): NonNullable { + return { + failOn: inputs.failOn, + reportOn: inputs.reportOn, + failCheck: inputs.failCheck, + requestChanges: inputs.requestChanges, + maxFindings: inputs.maxFindings, + }; +} + +/** + * Build the `actionRef`/`resolvedDefaults`/`skippedTriggers` triple every + * `writeFindingsOutput(Live)` call site needs. Centralizing this keeps a + * future field addition from requiring an edit at every individual write + * call site across `pr-workflow.ts`/`schedule.ts`. + */ +export function buildBaseOutputOptions( + inputs: Parameters[0] & { actionRef?: string }, + skippedTriggers: BuildFindingsOutputOptions['skippedTriggers'] +): Pick { + return { + actionRef: inputs.actionRef, + resolvedDefaults: buildResolvedDefaults(inputs), + skippedTriggers, + }; } function serializeTriggerError(error: unknown): z.infer { @@ -149,6 +325,9 @@ function serializeTriggerResult(result: ReplayTriggerResult): z.infer i.severity === 'high').length, + medium: items.filter((i) => i.severity === 'medium').length, + low: items.filter((i) => i.severity === 'low').length, + }; +} + /** Build the public findings export payload. */ export function buildFindingsOutput( reports: SkillReport[], @@ -169,9 +356,57 @@ export function buildFindingsOutput( options: BuildFindingsOutputOptions = {} ): FindingsOutput { const allFindings = reports.flatMap((r) => r.findings); + const metaByReport = new Map((options.skillExecutions ?? []).map((meta) => [meta.report, meta])); + + // Corroborating skills, grouped by the survivor finding id they were + // matched against (`dedupe.existingFindingId`) — not by re-derived + // content/location hashing, which misses semantic matches (different + // content hash than the survivor) and never lists the observing skill + // itself, only whichever skills the matched comment already attributed. + const corroborationBySurvivorId = new Map>(); + function addCorroborator(survivorId: string, skillName: string, matchType: DedupeMatchType | undefined): void { + const bySkill = corroborationBySurvivorId.get(survivorId) ?? new Map(); + bySkill.set(skillName, { skillName, matchType }); + corroborationBySurvivorId.set(survivorId, bySkill); + } + for (const observation of findingObservations) { + if (observation.outcome !== 'deduped') continue; + const survivorId = observation.dedupe.existingFindingId; + if (!survivorId) continue; + if (observation.skill) { + addCorroborator(survivorId, observation.skill, observation.dedupe.matchType); + } + for (const skillName of observation.dedupe.existingSkills ?? []) { + addCorroborator(survivorId, skillName, observation.dedupe.matchType); + } + } + + const { provenanceByFindingId, discarded } = buildProvenanceAndDiscarded( + (options.skillExecutions ?? []).map((meta): FindingExecutionEvents => ({ + skillExecutionId: meta.skillExecutionId, + model: meta.report.model, + verificationModel: meta.auxiliaryModel, + mergeModel: meta.synthesisModel, + events: meta.findingProcessingEvents ?? [], + })) + ); + + const byOutcome = { + posted: 0, + deduped: 0, + skipped: 0, + resolved: 0, + failed: 0, + }; + for (const observation of findingObservations) { + byOutcome[observation.outcome]++; + } + const output = { version: '1', timestamp: options.timestamp ?? new Date().toISOString(), + runAttempt: options.runAttempt ?? process.env['GITHUB_RUN_ATTEMPT'], + harness: { name: 'warden' as const, version: getVersion(), actionRef: options.actionRef }, repository: { owner: context.repository.owner, name: context.repository.name, @@ -189,35 +424,74 @@ export function buildFindingsOutput( }, }), runId: options.runId ?? process.env['GITHUB_RUN_ID'] ?? '', + ...(options.resolvedDefaults && { resolvedDefaults: options.resolvedDefaults }), + ...(options.skippedTriggers && { skippedTriggers: options.skippedTriggers }), summary: { totalFindings: allFindings.length, - findingsBySeverity: { - high: allFindings.filter((f) => f.severity === 'high').length, - medium: allFindings.filter((f) => f.severity === 'medium').length, - low: allFindings.filter((f) => f.severity === 'low').length, - }, + findingsBySeverity: severityCounts(allFindings), totalSkills: reports.length, + totalSkillExecutions: reports.length, + byOutcome, }, - skills: reports.map((r) => ({ - name: r.skill, - summary: r.summary, - model: r.model, - durationMs: r.durationMs, - usage: r.usage, - failedHunks: r.failedHunks, - failedExtractions: r.failedExtractions, - error: r.error, - findings: r.findings.map((f) => ({ - id: f.id, - severity: f.severity, - confidence: f.confidence, - title: f.title, - description: f.description, - location: f.location, - additionalLocations: f.additionalLocations, - sourceSnippet: f.sourceSnippet, - })), - })), + skills: reports.map((r) => { + const meta = metaByReport.get(r); + return { + name: r.skill, + summary: r.summary, + model: r.model, + auxiliaryModel: meta?.auxiliaryModel, + synthesisModel: meta?.synthesisModel, + durationMs: r.durationMs, + usage: r.usage, + failedHunks: r.failedHunks, + failedExtractions: r.failedExtractions, + error: r.error, + skillExecutionId: meta?.skillExecutionId, + triggerId: meta?.triggerId, + triggerName: meta?.triggerName, + findingsBySeverity: severityCounts(r.findings), + checkRunUrl: meta?.checkRunUrl, + checkRunId: meta?.checkRunId, + reviewEvent: meta?.reviewEvent, + checkConclusion: meta?.checkConclusion, + issueNumber: meta?.issueNumber, + issueUrl: meta?.issueUrl, + findings: r.findings.map((f) => { + const contentHash = generateContentHash(f.title, f.description); + const survivorId = f.reportedId ?? f.id; + const corroborators = [...(corroborationBySurvivorId.get(survivorId)?.values() ?? [])]; + const reportedBy = meta?.skillExecutionId !== undefined + ? [ + { skillExecutionId: meta.skillExecutionId, skillName: r.skill, role: 'primary' as const }, + ...corroborators + .filter((corroborator) => corroborator.skillName !== r.skill) + .map((corroborator) => ({ + skillName: corroborator.skillName, + role: 'corroborating' as const, + matchType: corroborator.matchType, + })), + ] + : undefined; + + return { + id: f.id, + reportedId: f.reportedId, + severity: f.severity, + confidence: f.confidence, + title: f.title, + description: f.description, + verification: f.verification, + location: f.location, + additionalLocations: f.additionalLocations, + sourceSnippet: f.sourceSnippet, + contentHash, + reportedBy, + provenance: provenanceByFindingId.get(provenanceKey(meta?.skillExecutionId, f.id)), + }; + }), + }; + }), + ...(discarded.length > 0 && { discardedFindings: discarded }), ...(options.triggerResults && { triggerResults: options.triggerResults.map(serializeTriggerResult), }), diff --git a/packages/warden/src/action/reporting/provenance.test.ts b/packages/warden/src/action/reporting/provenance.test.ts new file mode 100644 index 000000000..14943170f --- /dev/null +++ b/packages/warden/src/action/reporting/provenance.test.ts @@ -0,0 +1,135 @@ +import { describe, expect, it } from 'vitest'; +import type { Finding } from '../../types/index.js'; +import type { FindingProcessingEvent } from '../../sdk/types.js'; +import { buildProvenanceAndDiscarded, provenanceKey } from './provenance.js'; + +function makeFinding(overrides: Partial = {}): Finding { + return { + id: 'finding-1', + severity: 'medium', + title: 'Test finding', + description: 'Test description', + ...overrides, + }; +} + +describe('buildProvenanceAndDiscarded', () => { + it('returns empty results when there are no events', () => { + const result = buildProvenanceAndDiscarded([{ skillExecutionId: 'exec-1', events: [] }]); + expect(result.provenanceByFindingId.size).toBe(0); + expect(result.discarded).toEqual([]); + }); + + it('records a rejected finding only in discardedFindings, never in provenance', () => { + const rejected = makeFinding({ id: 'rejected-1' }); + const event: FindingProcessingEvent = { + stage: 'verification', + action: 'rejected', + finding: rejected, + reason: 'not a real issue', + }; + + const result = buildProvenanceAndDiscarded([ + { skillExecutionId: 'exec-1', verificationModel: 'claude-sonnet-4-5', events: [event] }, + ]); + + expect(result.provenanceByFindingId.size).toBe(0); + expect(result.discarded).toEqual([ + { + originSkillExecutionId: 'exec-1', + stage: 'verification_rejected', + severity: 'medium', + title: 'Test finding', + location: undefined, + model: 'claude-sonnet-4-5', + reason: 'not a real issue', + }, + ]); + }); + + it('keeps a revised finding under its (unchanged) id and snapshots the pre-revision state into before', () => { + const original = makeFinding({ id: 'kept-id', title: 'Original title', severity: 'low' }); + const revised = makeFinding({ id: 'kept-id', title: 'Revised title', severity: 'high' }); + const event: FindingProcessingEvent = { + stage: 'verification', + action: 'revised', + finding: original, + replacement: revised, + reason: 'narrowed scope after tracing the guard clause', + }; + + const result = buildProvenanceAndDiscarded([ + { skillExecutionId: 'exec-1', model: 'claude-sonnet-4-5', verificationModel: 'claude-haiku-4-5', events: [event] }, + ]); + + expect(result.discarded).toEqual([]); + expect(result.provenanceByFindingId.get(provenanceKey('exec-1', 'kept-id'))).toEqual({ + originSkillExecutionId: 'exec-1', + originModel: 'claude-sonnet-4-5', + verification: { + outcome: 'revised', + model: 'claude-haiku-4-5', + evidence: 'narrowed scope after tracing the guard clause', + before: { + title: 'Original title', + description: 'Test description', + severity: 'low', + confidence: undefined, + }, + }, + }); + }); + + it('attributes merged findings to the survivor and lists absorbed ids on both sides', () => { + const survivor = makeFinding({ id: 'survivor-1' }); + const absorbedA = makeFinding({ id: 'absorbed-a' }); + const absorbedB = makeFinding({ id: 'absorbed-b' }); + + const events: FindingProcessingEvent[] = [ + { stage: 'merge', action: 'merged', finding: absorbedA, replacement: survivor, reason: 'same root cause' }, + { stage: 'merge', action: 'merged', finding: absorbedB, replacement: survivor, reason: 'same root cause' }, + ]; + + const result = buildProvenanceAndDiscarded([ + { skillExecutionId: 'exec-1', model: 'claude-sonnet-4-5', mergeModel: 'claude-opus-5', events }, + ]); + + expect(result.discarded).toHaveLength(2); + expect(result.discarded.map((d) => d.survivorFindingId)).toEqual(['survivor-1', 'survivor-1']); + expect(result.discarded.every((d) => d.stage === 'merge_absorbed')).toBe(true); + + expect(result.provenanceByFindingId.get(provenanceKey('exec-1', 'survivor-1'))?.merge).toEqual({ + model: 'claude-opus-5', + absorbedFindingIds: ['absorbed-a', 'absorbed-b'], + }); + }); + + it('records a dedupe-dropped finding in discardedFindings, keyed to its survivor', () => { + const events: FindingProcessingEvent[] = [ + { stage: 'dedupe', action: 'dropped', finding: makeFinding(), replacement: makeFinding({ id: 'kept' }), reason: 'duplicate title and location' }, + ]; + + const result = buildProvenanceAndDiscarded([{ skillExecutionId: 'exec-1', events }]); + + expect(result.provenanceByFindingId.size).toBe(0); + expect(result.discarded).toEqual([ + expect.objectContaining({ + stage: 'dedupe_dropped', + originSkillExecutionId: 'exec-1', + survivorFindingId: 'kept', + reason: 'duplicate title and location', + }), + ]); + }); + + it('ignores fix_gate events', () => { + const events: FindingProcessingEvent[] = [ + { stage: 'fix_gate', action: 'stripped_fix', finding: makeFinding() }, + ]; + + const result = buildProvenanceAndDiscarded([{ skillExecutionId: 'exec-1', events }]); + + expect(result.provenanceByFindingId.size).toBe(0); + expect(result.discarded).toEqual([]); + }); +}); diff --git a/packages/warden/src/action/reporting/provenance.ts b/packages/warden/src/action/reporting/provenance.ts new file mode 100644 index 000000000..020d05de8 --- /dev/null +++ b/packages/warden/src/action/reporting/provenance.ts @@ -0,0 +1,172 @@ +import { z } from 'zod'; +import { ConfidenceSchema, LocationSchema, SeveritySchema } from '../../types/index.js'; +import type { FindingProcessingEvent } from '../../sdk/types.js'; + +const FindingSnapshotSchema = z.object({ + title: z.string(), + description: z.string(), + severity: SeveritySchema, + confidence: ConfidenceSchema.optional(), +}); + +const VerificationStageSchema = z.discriminatedUnion('outcome', [ + z.object({ + outcome: z.literal('revised'), + model: z.string().optional(), + evidence: z.string().optional(), + before: FindingSnapshotSchema, + }), +]); + +const MergeStageSchema = z.object({ + model: z.string().optional(), + absorbedFindingIds: z.array(z.string()), +}); + +export const FindingProvenanceSchema = z.object({ + originSkillExecutionId: z.string().optional(), + originModel: z.string().optional(), + verification: VerificationStageSchema.optional(), + merge: MergeStageSchema.optional(), +}); +export type FindingProvenance = z.infer; + +export const DiscardedFindingSchema = z.object({ + originSkillExecutionId: z.string().optional(), + stage: z.enum(['verification_rejected', 'merge_absorbed', 'dedupe_dropped']), + severity: SeveritySchema, + title: z.string(), + location: LocationSchema.optional(), + model: z.string().optional(), + reason: z.string().optional(), + survivorFindingId: z.string().optional(), +}); +export type DiscardedFinding = z.infer; + +/** One skill execution's captured verification/merge events, for provenance matching. */ +export interface FindingExecutionEvents { + skillExecutionId?: string; + /** Primary/analysis model, attributed as a finding's origin. */ + model?: string; + /** Model that ran the verification lane; attributed to verification-stage events instead of `model`. */ + verificationModel?: string; + /** Model that ran the merge/synthesis lane; attributed to merge-stage events instead of `model`. */ + mergeModel?: string; + events: FindingProcessingEvent[]; +} + +export interface ProvenanceAndDiscarded { + /** Keyed by `provenanceKey(skillExecutionId, findingId)` — see that function for why. */ + provenanceByFindingId: Map; + discarded: DiscardedFinding[]; +} + +/** + * Same-run dedupe can recenter two different skill executions' survivor + * findings onto the same id (e.g. both matched against the same pre-existing + * comment), so `findingId` alone is not a safe map key: a second execution's + * event would silently overwrite or merge into the first's entry, and both + * exported skill rows would then read the same (wrong) provenance. Scoping + * the key to the owning execution keeps each skill row's provenance + * independent. + */ +export function provenanceKey(skillExecutionId: string | undefined, findingId: string): string { + return `${skillExecutionId ?? ''}:${findingId}`; +} + +/** + * Build per-finding provenance and the list of discarded candidates from + * captured `FindingProcessingEvent`s. Handles `verification`/`rejected`, + * `verification`/`revised`, `merge`/`merged`, and `dedupe`/`dropped` events — + * `fix_gate` events are out of scope here (they don't remove a finding, they + * strip a proposed fix from one that's kept). + * + * `poster.ts`'s `recenterReportFindingIds` keeps this lookup valid across + * cross-run dedupe: when it recenters a survivor's `id` onto a pre-existing + * comment's id, it remaps this same finding's id inside any already-captured + * `FindingProcessingEvent`s so the id this map is keyed by (and the id + * `output.ts` looks it up by) stay in sync. + */ +export function buildProvenanceAndDiscarded(executions: FindingExecutionEvents[]): ProvenanceAndDiscarded { + const provenanceByFindingId = new Map(); + const discarded: DiscardedFinding[] = []; + + for (const { skillExecutionId, model, verificationModel, mergeModel, events } of executions) { + for (const event of events) { + if (event.stage === 'dedupe' && event.action === 'dropped') { + discarded.push({ + originSkillExecutionId: skillExecutionId, + stage: 'dedupe_dropped', + severity: event.finding.severity, + title: event.finding.title, + location: event.finding.location, + model, + reason: event.reason, + survivorFindingId: event.replacement?.id, + }); + continue; + } + + if (event.stage === 'verification' && event.action === 'rejected') { + discarded.push({ + originSkillExecutionId: skillExecutionId, + stage: 'verification_rejected', + severity: event.finding.severity, + title: event.finding.title, + location: event.finding.location, + model: verificationModel, + reason: event.reason, + }); + continue; + } + + if (event.stage === 'verification' && event.action === 'revised' && event.replacement) { + const key = provenanceKey(skillExecutionId, event.replacement.id); + provenanceByFindingId.set(key, { + ...provenanceByFindingId.get(key), + originSkillExecutionId: skillExecutionId, + originModel: model, + verification: { + outcome: 'revised', + model: verificationModel, + evidence: event.reason, + before: { + title: event.finding.title, + description: event.finding.description, + severity: event.finding.severity, + confidence: event.finding.confidence, + }, + }, + }); + continue; + } + + if (event.stage === 'merge' && event.action === 'merged' && event.replacement) { + discarded.push({ + originSkillExecutionId: skillExecutionId, + stage: 'merge_absorbed', + severity: event.finding.severity, + title: event.finding.title, + location: event.finding.location, + model: mergeModel, + reason: event.reason, + survivorFindingId: event.replacement.id, + }); + + const key = provenanceKey(skillExecutionId, event.replacement.id); + const existing = provenanceByFindingId.get(key); + provenanceByFindingId.set(key, { + ...existing, + originSkillExecutionId: existing?.originSkillExecutionId ?? skillExecutionId, + originModel: existing?.originModel ?? model, + merge: { + model: mergeModel, + absorbedFindingIds: [...(existing?.merge?.absorbedFindingIds ?? []), event.finding.id], + }, + }); + } + } + } + + return { provenanceByFindingId, discarded }; +} diff --git a/packages/warden/src/action/review/poster.test.ts b/packages/warden/src/action/review/poster.test.ts index 504f9caba..635c211b4 100644 --- a/packages/warden/src/action/review/poster.test.ts +++ b/packages/warden/src/action/review/poster.test.ts @@ -242,6 +242,96 @@ describe('postTriggerReview', () => { body: '', comments: [expect.objectContaining({ path: 'test.ts', line: 10, side: 'RIGHT', body: 'Test comment' })], }); + // Regression: the export's reviewEvent must reflect what actually + // posted, not renderResult's pre-posting intent. + expect(result.reviewEventPosted).toBe('COMMENT'); + }); + + it('carries skillExecutionId from the trigger result onto posted observations', async () => { + const finding = createFinding(); + const result: TriggerResult = { + triggerName: 'test-trigger', + skillName: 'test-skill', + skillExecutionId: 'exec-abc123', + report: { + skill: 'test-skill', + summary: 'Found 1 issue', + findings: [finding], + usage: { inputTokens: 100, outputTokens: 50, costUSD: 0.01 }, + }, + renderResult: createRenderResult({ + review: { + event: 'COMMENT', + body: 'Test review', + comments: [{ path: 'test.ts', line: 10, body: 'Test comment' }], + }, + }), + reportOn: 'low', + }; + + vi.mocked(findingToExistingComment).mockReturnValue(createExistingComment()); + + const ctx: ReviewPostingContext = { + result, + existingComments: [], + apiKey: 'test-key', + }; + + const postResult = await postTriggerReview(ctx, mockDeps); + + expect(postResult.findingObservations).toEqual([ + expect.objectContaining({ outcome: 'posted', finding, skillExecutionId: 'exec-abc123' }), + ]); + }); + + it('carries existingSkills from the matched comment onto dedupe observations', async () => { + const finding = createFinding(); + const result: TriggerResult = { + triggerName: 'test-trigger', + skillName: 'test-skill', + skillExecutionId: 'exec-abc123', + report: { + skill: 'test-skill', + summary: 'Found 1 issue', + findings: [finding], + usage: { inputTokens: 100, outputTokens: 50, costUSD: 0.01 }, + }, + renderResult: createRenderResult({ + review: { + event: 'COMMENT', + body: 'Test review', + comments: [{ path: 'test.ts', line: 10, body: 'Test comment' }], + }, + }), + reportOn: 'low', + }; + + const existingComment = createExistingComment({ + isWarden: true, + skills: ['other-skill'], + }); + + vi.mocked(deduplicateFindings).mockResolvedValue({ + newFindings: [], + duplicateActions: [{ type: 'react_external', originalFindingId: finding.id, finding, existingComment, matchType: 'hash' }], + }); + vi.mocked(processDuplicateActions).mockResolvedValue({ updated: 0, reacted: 1, skipped: 0, failed: 0 }); + + const ctx: ReviewPostingContext = { + result, + existingComments: [existingComment], + apiKey: 'test-key', + }; + + const postResult = await postTriggerReview(ctx, mockDeps); + + expect(postResult.findingObservations).toEqual([ + expect.objectContaining({ + outcome: 'deduped', + skillExecutionId: 'exec-abc123', + dedupe: expect.objectContaining({ existingSkills: ['other-skill'] }), + }), + ]); }); it('skips body-only non-blocking reviews', async () => { @@ -318,6 +408,11 @@ describe('postTriggerReview', () => { expect(postResult.posted).toBe(false); expect(mockOctokit.pulls.createReview).not.toHaveBeenCalled(); expect(processDuplicateActions).not.toHaveBeenCalled(); + // Regression: these findings were about to post — the export must record + // why they didn't, not silently drop them from findingObservations. + expect( + postResult.findingObservations.filter((o) => o.outcome === 'skipped' && o.skippedReason === 'review_not_posted') + ).toHaveLength(2); }); it('skips the review write when the PR head advances during duplicate processing', async () => { @@ -379,6 +474,11 @@ describe('postTriggerReview', () => { expect(mockOctokit.pulls.get).toHaveBeenCalledTimes(2); // No swallowed error: the findings were not marked failed. expect(postResult.findingObservations.filter((o) => o.outcome === 'failed')).toEqual([]); + // Regression: the finding that would have posted must be recorded as + // blocked, not vanish from the export. + expect( + postResult.findingObservations.filter((o) => o.outcome === 'skipped' && o.skippedReason === 'review_not_posted') + ).toEqual([expect.objectContaining({ finding: findings[0] })]); } finally { dateNowSpy.mockRestore(); } @@ -390,6 +490,7 @@ describe('postTriggerReview', () => { const result: TriggerResult = { triggerName: 'test-trigger', skillName: 'test-skill', + skillExecutionId: 'exec-mixed', report: { skill: 'test-skill', summary: 'Found 2 issues', @@ -419,8 +520,8 @@ describe('postTriggerReview', () => { expect.objectContaining({ event: 'COMMENT', body: '' }) ); expect(postResult.findingObservations).toEqual([ - { outcome: 'posted', finding: inlineFinding, skill: 'test-skill' }, - { outcome: 'skipped', finding: bodyFinding, skill: 'test-skill', skippedReason: 'no_inline_location' }, + { outcome: 'posted', finding: inlineFinding, skill: 'test-skill', skillExecutionId: 'exec-mixed' }, + { outcome: 'skipped', finding: bodyFinding, skill: 'test-skill', skillExecutionId: 'exec-mixed', skippedReason: 'no_inline_location' }, ]); }); @@ -739,6 +840,17 @@ describe('postTriggerReview', () => { reportOn: 'low', failOn: 'high', requestChanges: true, + // Captured during skill execution, before this recenter — its finding + // id must move in lockstep with the report's own recentered id so + // provenance.ts's id-keyed lookup doesn't miss. + findingProcessingEvents: [ + { + stage: 'verification', + action: 'revised', + finding: { ...finding, title: 'Original wording' }, + replacement: finding, + }, + ], }; const existingComment = createExistingComment({ isWarden: true, findingId: 'WRZ-XPL' }); @@ -783,6 +895,10 @@ describe('postTriggerReview', () => { expect(postResult.posted).toBe(true); expect([...postResult.activeWardenCommentIds]).toEqual([1]); expect(result.report?.findings[0]?.id).toBe('WRZ-XPL'); + expect(result.report?.findings[0]?.reportedId).toBe('WRZ-XPL'); + // Regression: the captured processing event's replacement id must move + // with the recenter, or provenance.ts's id-keyed lookup silently misses. + expect(result.findingProcessingEvents?.[0]?.replacement?.id).toBe('WRZ-XPL'); expect(postResult.findingObservations).toEqual([ expect.objectContaining({ outcome: 'deduped', @@ -843,6 +959,7 @@ describe('postTriggerReview', () => { const result: TriggerResult = { triggerName: 'test-trigger', skillName: 'test-skill', + skillExecutionId: 'exec-checks-only', report: { skill: 'test-skill', summary: 'Found 1 issue', @@ -877,7 +994,7 @@ describe('postTriggerReview', () => { expect(postResult.posted).toBe(false); expect(postResult.newComments).toHaveLength(0); expect(postResult.findingObservations).toEqual([ - { outcome: 'skipped', finding, skill: 'test-skill', skippedReason: 'no_inline_location' }, + { outcome: 'skipped', finding, skill: 'test-skill', skillExecutionId: 'exec-checks-only', skippedReason: 'no_inline_location' }, ]); expect(mockOctokit.pulls.createReview).toHaveBeenCalledTimes(1); }); @@ -962,6 +1079,7 @@ describe('postTriggerReview', () => { const result: TriggerResult = { triggerName: 'test-trigger', skillName: 'test-skill', + skillExecutionId: 'exec-max-findings', report: { skill: 'test-skill', summary: 'Found 2 issues', @@ -996,12 +1114,14 @@ describe('postTriggerReview', () => { outcome: 'skipped', finding: finding2, skill: 'test-skill', + skillExecutionId: 'exec-max-findings', skippedReason: 'max_findings', }, { outcome: 'failed', finding: finding1, skill: 'test-skill', + skillExecutionId: 'exec-max-findings', }, ]); }); @@ -1013,6 +1133,7 @@ describe('postTriggerReview', () => { const result: TriggerResult = { triggerName: 'test-trigger', skillName: 'test-skill', + skillExecutionId: 'exec-batch', report: { skill: 'test-skill', summary: 'Found 2 issues', @@ -1072,12 +1193,14 @@ describe('postTriggerReview', () => { outcome: 'skipped', finding: finding2, skill: 'test-skill', + skillExecutionId: 'exec-batch', skippedReason: 'duplicate_in_batch', }, { outcome: 'posted', finding: finding1, skill: 'test-skill', + skillExecutionId: 'exec-batch', }, ]); }); diff --git a/packages/warden/src/action/review/poster.ts b/packages/warden/src/action/review/poster.ts index 4bd0e06c1..42b52b7ae 100644 --- a/packages/warden/src/action/review/poster.ts +++ b/packages/warden/src/action/review/poster.ts @@ -8,6 +8,7 @@ import type { Octokit } from '@octokit/rest'; import type { EventContext, Finding } from '../../types/index.js'; import { filterFindings } from '../../types/index.js'; +import type { FindingProcessingEvent } from '../../sdk/types.js'; import { shouldFail } from '../../triggers/matcher.js'; import type { RenderResult } from '../../output/types.js'; import { renderSkillReport, renderFindingsBody } from '../../output/renderer.js'; @@ -82,12 +83,14 @@ function emptyReviewPostResult( function buildDedupeObservations( actions: DeduplicateResult['duplicateActions'], - skill: string + skill: string, + skillExecutionId: string | undefined ): FindingObservation[] { return actions.map((action) => ({ outcome: 'deduped', finding: action.finding, skill, + skillExecutionId, dedupe: { source: action.existingComment.isWarden ? 'warden' : 'external', matchType: action.matchType, @@ -95,30 +98,54 @@ function buildDedupeObservations( ...(action.existingComment.id > 0 ? { existingCommentId: action.existingComment.id } : {}), existingThreadId: action.existingComment.threadId, existingResolved: action.existingComment.isResolved, + existingSkills: action.existingComment.skills, actor: action.existingComment.actor, }, })); } -function recenterReportFindingIds(reportFindings: Finding[], actions: DeduplicateResult['duplicateActions']): Finding[] { - if (actions.length === 0) { - return reportFindings; - } - - const ids = new Map( +function recenterReportFindingIds( + reportFindings: Finding[], + actions: DeduplicateResult['duplicateActions'] +): { findings: Finding[]; idMap: Map } { + const idMap = new Map( actions .filter((action) => action.originalFindingId !== action.finding.id) .map((action) => [action.originalFindingId, action.finding.id]) ); - if (ids.size === 0) { - return reportFindings; + if (idMap.size === 0) { + return { findings: reportFindings, idMap }; } - return reportFindings.map((finding) => { - const recenteredId = ids.get(finding.id); - return recenteredId ? { ...finding, id: recenteredId } : finding; + const findings = reportFindings.map((finding) => { + const recenteredId = idMap.get(finding.id); + return recenteredId ? { ...finding, id: recenteredId, reportedId: recenteredId } : finding; }); + + return { findings, idMap }; +} + +function remapFindingId(finding: Finding, idMap: Map): Finding { + const recenteredId = idMap.get(finding.id); + return recenteredId ? { ...finding, id: recenteredId, reportedId: recenteredId } : finding; +} + +/** + * Keep captured `FindingProcessingEvent`s in sync with a recenter so + * `provenance.ts`'s id-keyed lookup doesn't miss: those events were captured + * during skill execution, before cross-run dedupe could recenter a finding's + * id onto a pre-existing comment's id. + */ +function remapFindingProcessingEvents( + events: FindingProcessingEvent[], + idMap: Map +): FindingProcessingEvent[] { + return events.map((event) => ({ + ...event, + finding: remapFindingId(event.finding, idMap), + ...(event.replacement && { replacement: remapFindingId(event.replacement, idMap) }), + })); } // ----------------------------------------------------------------------------- @@ -295,6 +322,7 @@ export async function postTriggerReview( outcome: 'skipped', finding, skill, + skillExecutionId: result.skillExecutionId, skippedReason: 'duplicate_in_batch', }); } @@ -326,10 +354,14 @@ export async function postTriggerReview( currentSkill: skill, maxRetries: ctx.maxRetries, }); - result.report.findings = recenterReportFindingIds(result.report.findings, dedupResult.duplicateActions); + const recentered = recenterReportFindingIds(result.report.findings, dedupResult.duplicateActions); + result.report.findings = recentered.findings; + if (recentered.idMap.size > 0 && result.findingProcessingEvents) { + result.findingProcessingEvents = remapFindingProcessingEvents(result.findingProcessingEvents, recentered.idMap); + } findingsToPost = dedupResult.newFindings; findingsToMarkFailed = findingsToPost; - findingObservations.push(...buildDedupeObservations(dedupResult.duplicateActions, skill)); + findingObservations.push(...buildDedupeObservations(dedupResult.duplicateActions, skill, result.skillExecutionId)); // Merge dedup usage into the report's auxiliary usage if (dedupResult.dedupUsage) { @@ -358,6 +390,15 @@ export async function postTriggerReview( // head freshness before the first GitHub write (duplicate-action comment // updates below, then the review itself). if (!(await deps.feedbackGate.canWrite())) { + for (const finding of findingsToPost) { + findingObservations.push({ + outcome: 'skipped', + finding, + skill, + skillExecutionId: result.skillExecutionId, + skippedReason: 'review_not_posted', + }); + } return emptyReviewPostResult(newComments, activeWardenCommentIds, findingObservations); } @@ -427,6 +468,7 @@ export async function postTriggerReview( outcome: 'skipped', finding, skill, + skillExecutionId: result.skillExecutionId, skippedReason: 'max_findings', }); } @@ -449,23 +491,47 @@ export async function postTriggerReview( } if (postOutcome === 'checks_only') { for (const finding of postedFindings) { - findingObservations.push({ outcome: 'skipped', finding, skill, skippedReason: 'no_inline_location' }); + findingObservations.push({ + outcome: 'skipped', + finding, + skill, + skillExecutionId: result.skillExecutionId, + skippedReason: 'no_inline_location', + }); } return emptyReviewPostResult(newComments, activeWardenCommentIds, findingObservations); } if (postOutcome !== 'posted') { + if (postOutcome === 'blocked') { + for (const finding of postedFindings) { + findingObservations.push({ + outcome: 'skipped', + finding, + skill, + skillExecutionId: result.skillExecutionId, + skippedReason: 'review_not_posted', + }); + } + } return emptyReviewPostResult(newComments, activeWardenCommentIds, findingObservations); } + result.reviewEventPosted = renderResultToPost.review?.event; // COMMENT reviews post with an empty body, so locationless findings that // the renderer placed in the body never reach the PR. Record them as // checks-only instead of claiming they were posted. const bodyStripped = renderResultToPost.review?.event === 'COMMENT'; for (const finding of postedFindings) { if (bodyStripped && !finding.location) { - findingObservations.push({ outcome: 'skipped', finding, skill, skippedReason: 'no_inline_location' }); + findingObservations.push({ + outcome: 'skipped', + finding, + skill, + skillExecutionId: result.skillExecutionId, + skippedReason: 'no_inline_location', + }); continue; } - findingObservations.push({ outcome: 'posted', finding, skill }); + findingObservations.push({ outcome: 'posted', finding, skill, skillExecutionId: result.skillExecutionId }); const comment = findingToExistingComment(finding, skill); if (comment) { newComments.push(comment); @@ -492,7 +558,7 @@ export async function postTriggerReview( activeWardenCommentIds, findingObservations: [ ...findingObservations, - ...findingsToMarkFailed.map((finding) => ({ outcome: 'failed' as const, finding, skill })), + ...findingsToMarkFailed.map((finding) => ({ outcome: 'failed' as const, finding, skill, skillExecutionId: result.skillExecutionId })), ], shouldFail: false, }; diff --git a/packages/warden/src/action/triggers/executor.test.ts b/packages/warden/src/action/triggers/executor.test.ts index fd433f20e..332e410d4 100644 --- a/packages/warden/src/action/triggers/executor.test.ts +++ b/packages/warden/src/action/triggers/executor.test.ts @@ -7,8 +7,9 @@ import { type TriggerExecutorDeps, } from './executor.js'; import type { ResolvedTrigger } from '../../config/loader.js'; -import type { EventContext, SkillReport } from '../../types/index.js'; +import type { EventContext, Finding, SkillReport } from '../../types/index.js'; import type { RenderResult } from '../../output/types.js'; +import type { FindingProcessingEvent } from '../../sdk/types.js'; import { initSentry, Sentry } from '../../sentry.js'; // Mock dependencies @@ -118,6 +119,7 @@ describe('executeTrigger', () => { const mockTrigger: ResolvedTrigger = { id: 'test-trigger-id', + skillExecutionId: 'test-skill-execution-id', name: 'test-trigger', skill: 'test-skill', type: 'pull_request', @@ -262,6 +264,23 @@ describe('executeTrigger', () => { }); }); + it('carries auxiliaryModel and synthesisModel from the trigger onto the result', async () => { + const mockReport = createReport(); + + vi.mocked(runSkillTask).mockResolvedValue({ name: 'test-trigger', report: mockReport }); + vi.mocked(createSkillCheck).mockResolvedValue({ checkRunId: 123, url: 'https://github.com/check/123' }); + vi.mocked(updateSkillCheck).mockResolvedValue(undefined); + + const result = await executeTrigger({ + ...mockTrigger, + auxiliaryModel: 'anthropic/aux-model', + synthesisModel: 'anthropic/synth-model', + }, mockDeps); + + expect(result.auxiliaryModel).toBe('anthropic/aux-model'); + expect(result.synthesisModel).toBe('anthropic/synth-model'); + }); + it('executes a trigger successfully with no findings', async () => { const mockReport = createReport(); @@ -548,4 +567,36 @@ describe('executeTrigger', () => { expect(result.triggerName).toBe('test-trigger'); expect(result.report).toBe(mockReport); }); + + it('carries skillExecutionId and captures onFindingProcessing events regardless of verbosity', async () => { + const rejectedFinding: Finding = { + id: 'test-1', + severity: 'medium', + confidence: 'high', + title: 'Test finding', + description: 'Test', + }; + const mockReport = createReport(); + const event: FindingProcessingEvent = { + stage: 'verification', + action: 'rejected', + finding: rejectedFinding, + reason: 'not real', + }; + + vi.mocked(runSkillTask).mockImplementation(async (_taskOptions, _fileConcurrency, callbacks) => { + callbacks.onFindingProcessing?.('test-trigger', event); + return { name: 'test-trigger', report: mockReport }; + }); + vi.mocked(createSkillCheck).mockResolvedValue({ checkRunId: 123, url: 'https://github.com/check/123' }); + vi.mocked(updateSkillCheck).mockResolvedValue(undefined); + + const result = await executeTrigger( + { ...mockTrigger, skillExecutionId: 'exec-abc123' }, + mockDeps + ); + + expect(result.skillExecutionId).toBe('exec-abc123'); + expect(result.findingProcessingEvents).toEqual([event]); + }); }); diff --git a/packages/warden/src/action/triggers/executor.ts b/packages/warden/src/action/triggers/executor.ts index 90affb87f..8f5b376b7 100644 --- a/packages/warden/src/action/triggers/executor.ts +++ b/packages/warden/src/action/triggers/executor.ts @@ -19,6 +19,7 @@ import type { SkillTaskOptions } from '../../cli/output/tasks.js'; import { renderSkillReport } from '../../output/renderer.js'; import { logGroup, logGroupEnd } from '../workflow/base.js'; import { DEFAULT_FILE_CONCURRENCY, type AnalysisChunkingConfig } from '../../sdk/types.js'; +import type { FindingProcessingEvent } from '../../sdk/types.js'; import { SkillRunnerError } from '../../sdk/errors.js'; import type { Semaphore } from '../../utils/index.js'; import { Verbosity } from '../../cli/output/verbosity.js'; @@ -58,6 +59,7 @@ function toAnalysisChunkingConfig( */ export interface TriggerCheckRun { url?: string; + checkRunId?: number; complete(report: SkillReport, options: TriggerCheckCompleteOptions): Promise; fail(error: unknown): Promise; } @@ -112,6 +114,7 @@ export interface TriggerExecutorDeps { */ export interface TriggerResult { triggerId?: string; + skillExecutionId?: string; triggerName: string; skillName: string; report?: SkillReport; @@ -123,8 +126,21 @@ export interface TriggerResult { requestChanges?: boolean; failCheck?: boolean; checkRunUrl?: string; + checkRunId?: number; maxFindings?: number; + auxiliaryModel?: string; + synthesisModel?: string; error?: unknown; + /** Verification/merge events captured during post-processing, for provenance export. */ + findingProcessingEvents?: FindingProcessingEvent[]; + /** + * The review event actually posted to GitHub by `postTriggerReview`, set + * only when posting succeeds. Distinct from `renderResult.review.event`, + * which reflects pre-posting render intent and can diverge from what (if + * anything) actually posted — the gate can block the write, or posting can + * fall back to checks-only after rendering already decided an event. + */ + reviewEventPosted?: 'APPROVE' | 'REQUEST_CHANGES' | 'COMMENT'; } // ----------------------------------------------------------------------------- @@ -155,10 +171,12 @@ export async function executeTrigger( // Create skill check (only for PRs) let skillCheck: TriggerCheckRun | undefined; let skillCheckUrl: string | undefined; + let skillCheckRunId: number | undefined; if (deps.checks && context.pullRequest) { try { skillCheck = await deps.checks.start(trigger.skill); skillCheckUrl = skillCheck.url; + skillCheckRunId = skillCheck.checkRunId; } catch (error) { console.error(`::warning::Failed to create skill check for ${trigger.skill}: ${error}`); } @@ -204,7 +222,15 @@ export async function executeTrigger( }, }; - const callbacks = createDefaultCallbacks([taskOptions], CI_OUTPUT_MODE, Verbosity.Normal); + const defaultCallbacks = createDefaultCallbacks([taskOptions], CI_OUTPUT_MODE, Verbosity.Normal); + const findingProcessingEvents: FindingProcessingEvent[] = []; + const callbacks = { + ...defaultCallbacks, + onFindingProcessing: (skillName: string, event: FindingProcessingEvent) => { + findingProcessingEvents.push(event); + defaultCallbacks.onFindingProcessing?.(skillName, event); + }, + }; const fileConcurrency = deps.semaphore ? Number.MAX_SAFE_INTEGER : DEFAULT_FILE_CONCURRENCY; const result = await runSkillTask(taskOptions, fileConcurrency, callbacks, deps.semaphore); const report = result.report; @@ -257,6 +283,7 @@ export async function executeTrigger( logGroupEnd(); return { triggerId: trigger.id, + skillExecutionId: trigger.skillExecutionId, triggerName: trigger.name, skillName: trigger.skill, report, @@ -268,7 +295,11 @@ export async function executeTrigger( requestChanges, failCheck, checkRunUrl: skillCheckUrl, + checkRunId: skillCheckRunId, maxFindings, + auxiliaryModel: trigger.auxiliaryModel, + synthesisModel: trigger.synthesisModel, + findingProcessingEvents, }; } catch (error) { if (error instanceof ActionFailedError) throw error; @@ -288,7 +319,13 @@ export async function executeTrigger( console.error(`::warning::Trigger ${trigger.name} failed: ${error}`); logGroupEnd(); - return { triggerId: trigger.id, triggerName: trigger.name, skillName: trigger.skill, error }; + return { + triggerId: trigger.id, + skillExecutionId: trigger.skillExecutionId, + triggerName: trigger.name, + skillName: trigger.skill, + error, + }; } }, ); diff --git a/packages/warden/src/action/workflow/base.test.ts b/packages/warden/src/action/workflow/base.test.ts index 37c2a61e6..6234b359e 100644 --- a/packages/warden/src/action/workflow/base.test.ts +++ b/packages/warden/src/action/workflow/base.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; -import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync } from 'node:fs'; +import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import type { EventContext, SkillReport } from '../../types/index.js'; @@ -16,9 +16,11 @@ vi.mock('../../utils/exec.js', async (importOriginal) => { import { execFileNonInteractive, execNonInteractive } from '../../utils/exec.js'; import { + clearStaleDoneMarker, getFindingsOutputPath, prepareRuntimeEnvironment, writeFindingsOutput, + writeFindingsOutputLive, } from './base.js'; import { FindingsOutputSchema } from '../reporting/output.js'; @@ -78,6 +80,7 @@ describe('findings output', () => { expect(filePath).toBe(join(tempDir, 'warden-findings.json')); expect(existsSync(filePath)).toBe(true); + expect(existsSync(`${filePath}.done`)).toBe(true); expect(readFileSync(process.env['GITHUB_OUTPUT']!, 'utf-8')).toBe( 'findings-file=warden-findings.json\n' ); @@ -108,6 +111,98 @@ describe('findings output', () => { }); }); +describe('clearStaleDoneMarker', () => { + let tempDir: string; + let previousGithubWorkspace: string | undefined; + + beforeEach(() => { + tempDir = mkdtempSync(join(tmpdir(), 'warden-clear-done-')); + previousGithubWorkspace = process.env['GITHUB_WORKSPACE']; + process.env['GITHUB_WORKSPACE'] = tempDir; + }); + + afterEach(() => { + if (previousGithubWorkspace === undefined) { + delete process.env['GITHUB_WORKSPACE']; + } else { + process.env['GITHUB_WORKSPACE'] = previousGithubWorkspace; + } + rmSync(tempDir, { recursive: true, force: true }); + }); + + it('removes a .done marker left over from a previous run at the same path', () => { + const filePath = getFindingsOutputPath(tempDir); + mkdirSync(tempDir, { recursive: true }); + writeFileSync(`${filePath}.done`, ''); + + clearStaleDoneMarker(tempDir); + + expect(existsSync(`${filePath}.done`)).toBe(false); + }); + + it('is a no-op when no .done marker exists', () => { + expect(() => clearStaleDoneMarker(tempDir)).not.toThrow(); + }); +}); + +describe('writeFindingsOutputLive', () => { + let tempDir: string; + let previousGithubOutput: string | undefined; + let previousGithubWorkspace: string | undefined; + + beforeEach(() => { + tempDir = mkdtempSync(join(tmpdir(), 'warden-findings-live-')); + previousGithubOutput = process.env['GITHUB_OUTPUT']; + previousGithubWorkspace = process.env['GITHUB_WORKSPACE']; + process.env['GITHUB_OUTPUT'] = join(tempDir, 'github-output'); + process.env['GITHUB_WORKSPACE'] = tempDir; + }); + + afterEach(() => { + if (previousGithubOutput === undefined) { + delete process.env['GITHUB_OUTPUT']; + } else { + process.env['GITHUB_OUTPUT'] = previousGithubOutput; + } + if (previousGithubWorkspace === undefined) { + delete process.env['GITHUB_WORKSPACE']; + } else { + process.env['GITHUB_WORKSPACE'] = previousGithubWorkspace; + } + rmSync(tempDir, { recursive: true, force: true }); + }); + + it('writes an in-progress snapshot without a .done marker or the findings-file output', () => { + const filePath = getFindingsOutputPath(tempDir); + + writeFindingsOutputLive([createReport()], createContext(tempDir), []); + + expect(existsSync(filePath)).toBe(true); + expect(existsSync(`${filePath}.done`)).toBe(false); + expect(existsSync(process.env['GITHUB_OUTPUT']!)).toBe(false); + + const payload = FindingsOutputSchema.parse(JSON.parse(readFileSync(filePath, 'utf-8'))); + expect(payload.summary.totalFindings).toBe(1); + }); + + it('removes a stale .done marker left over from a previous run at the same path', () => { + const filePath = getFindingsOutputPath(tempDir); + mkdirSync(join(tempDir), { recursive: true }); + writeFileSync(`${filePath}.done`, ''); + + writeFindingsOutputLive([createReport()], createContext(tempDir), []); + + expect(existsSync(`${filePath}.done`)).toBe(false); + }); + + it('never throws when the write fails', () => { + const context = createContext(tempDir); + context.repoPath = '/nonexistent-parent/that-cannot-be-created\0invalid'; + + expect(() => writeFindingsOutputLive([createReport()], context, [])).not.toThrow(); + }); +}); + describe('runtime setup', () => { let previousClaudeCodePath: string | undefined; let previousHome: string | undefined; diff --git a/packages/warden/src/action/workflow/base.ts b/packages/warden/src/action/workflow/base.ts index 45a24c78f..cb51c216f 100644 --- a/packages/warden/src/action/workflow/base.ts +++ b/packages/warden/src/action/workflow/base.ts @@ -4,17 +4,18 @@ * Shared infrastructure for PR and schedule workflows. */ -import { appendFileSync, mkdirSync, writeFileSync } from 'node:fs'; +import { appendFileSync, existsSync, unlinkSync } from 'node:fs'; import { tmpdir } from 'node:os'; -import { dirname, join, relative } from 'node:path'; +import { join, relative } from 'node:path'; import { randomUUID } from 'node:crypto'; import type { Octokit } from '@octokit/rest'; import { execFileNonInteractive, execNonInteractive } from '../../utils/exec.js'; import { isRepoRelativePath, normalizePath } from '../../utils/path.js'; +import { writeFileAtomic } from '../../utils/fs.js'; import type { EventContext, SkillReport } from '../../types/index.js'; import type { FindingObservation } from '../reporting/outcomes.js'; import { buildFindingsOutput } from '../reporting/output.js'; -import type { ReplayTriggerResult } from '../reporting/output.js'; +import type { BuildFindingsOutputOptions } from '../reporting/output.js'; import { countSeverity } from '../../triggers/matcher.js'; import type { RuntimeName } from '../../sdk/runtimes/index.js'; import type { ActionInputs } from '../inputs.js'; @@ -369,26 +370,75 @@ export function getFindingsOutputPath(repoPath?: string): string { return join(tmpDir, 'warden-findings.json'); } +/** + * Remove a `.done` marker left over from a previous run at this same path. + * Call once, at the very start of the workflow, before any fallible setup + * (config load, API calls) — a live write only happens after the first + * trigger settles, so without this a stale `.done` from a prior run would + * make a brand-new, still in-progress run look finished to a follower for + * however long setup plus that first trigger takes. Takes `repoPath` + * directly (not `EventContext`) so it's callable before the context exists. + */ +export function clearStaleDoneMarker(repoPath: string | undefined): void { + const filePath = getFindingsOutputPath(repoPath); + if (!existsSync(`${filePath}.done`)) { + return; + } + try { + unlinkSync(`${filePath}.done`); + } catch { + // Best-effort cleanup; a stale marker left behind is not fatal. + } +} + /** * Write structured findings data to a JSON file for external export (GCS, S3, etc.). * * Sets `findings-file` to a repo-relative path when possible so downstream * steps can reference the path without tripping ignore processors on absolute - * runner temp paths. + * runner temp paths. This is always a run's true final write — a `.done` + * sidecar is written alongside it so a local follower (see + * `writeFindingsOutputLive`) can tell a finished run from one still in + * progress. */ export function writeFindingsOutput( reports: SkillReport[], context: EventContext, findingObservations: FindingObservation[] = [], - options: { triggerResults?: ReplayTriggerResult[] } = {} + options: BuildFindingsOutputOptions = {} ): string { const filePath = getFindingsOutputPath(context.repoPath); - const output = buildFindingsOutput(reports, context, findingObservations, { - triggerResults: options.triggerResults, - }); + const output = buildFindingsOutput(reports, context, findingObservations, options); - mkdirSync(dirname(filePath), { recursive: true }); - writeFileSync(filePath, JSON.stringify(output, null, 2)); + writeFileAtomic(filePath, JSON.stringify(output, null, 2)); + writeFileAtomic(`${filePath}.done`, ''); setOutput('findings-file', getFindingsOutputValue(filePath, context.repoPath)); return filePath; } + +/** + * Write the findings file as an in-progress snapshot: no `.done` sidecar, no + * `findings-file` action output (that must only ever reflect the run's one + * true final write, never race a downstream step reading it mid-run). Never + * throws — a transient write hiccup here must not abort a run the way a + * final-write failure legitimately can. + * + * Also clears a stale `.done` sidecar left over from a previous run as a + * defensive backstop — the primary guarantee is `clearStaleDoneMarker` + * called once up front by the caller, before this run's first write. + */ +export function writeFindingsOutputLive( + reports: SkillReport[], + context: EventContext, + findingObservations: FindingObservation[] = [], + options: BuildFindingsOutputOptions = {} +): void { + try { + clearStaleDoneMarker(context.repoPath); + const filePath = getFindingsOutputPath(context.repoPath); + const output = buildFindingsOutput(reports, context, findingObservations, options); + writeFileAtomic(filePath, JSON.stringify(output, null, 2)); + } catch (error) { + console.error(`::warning::Failed to write live findings output: ${error}`); + } +} diff --git a/packages/warden/src/action/workflow/pr-workflow.test.ts b/packages/warden/src/action/workflow/pr-workflow.test.ts index ea198ea62..50e340fa8 100644 --- a/packages/warden/src/action/workflow/pr-workflow.test.ts +++ b/packages/warden/src/action/workflow/pr-workflow.test.ts @@ -111,6 +111,8 @@ vi.mock('./base.js', async () => { }), getAuthenticatedBotLogin: vi.fn(() => Promise.resolve('warden[bot]')), writeFindingsOutput: vi.fn(actual.writeFindingsOutput), + writeFindingsOutputLive: vi.fn(actual.writeFindingsOutputLive), + clearStaleDoneMarker: vi.fn(actual.clearStaleDoneMarker), }; }); @@ -118,7 +120,7 @@ vi.mock('./base.js', async () => { import { runSkillTask } from '../../cli/output/tasks.js'; import { fetchExistingComments, deduplicateFindings, processDuplicateActions } from '../../output/dedup.js'; import { evaluateFixAttempts } from '../fix-evaluation/index.js'; -import { setFailed, writeFindingsOutput } from './base.js'; +import { setFailed, writeFindingsOutput, writeFindingsOutputLive, clearStaleDoneMarker } from './base.js'; import { runPRWorkflow } from './pr-workflow.js'; import { clearSkillsCache } from '../../skills/loader.js'; import { Semaphore } from '../../utils/index.js'; @@ -132,6 +134,8 @@ const mockProcessDuplicateActions = vi.mocked(processDuplicateActions); const mockEvaluateFixAttempts = vi.mocked(evaluateFixAttempts); const mockSetFailed = vi.mocked(setFailed); const mockWriteFindingsOutput = vi.mocked(writeFindingsOutput); +const mockWriteFindingsOutputLive = vi.mocked(writeFindingsOutputLive); +const mockClearStaleDoneMarker = vi.mocked(clearStaleDoneMarker); // Type helper for mocking Octokit responses type GetPullResponse = Awaited>; @@ -373,7 +377,7 @@ describe('runPRWorkflow', () => { repository: expect.objectContaining({ fullName: 'test-owner/test-repo' }), }), [], - { + expect.objectContaining({ triggerResults: [ expect.objectContaining({ triggerName: 'test-skill', @@ -381,8 +385,102 @@ describe('runPRWorkflow', () => { report, }), ], - } + skillExecutions: [expect.objectContaining({ report, skillExecutionId: expect.any(String) })], + }) + ); + }); + + it('analyze mode carries auxiliaryModel and synthesisModel from the resolved trigger into skillExecutions', async () => { + const report = createSkillReport(); + mockRunSkillTask.mockResolvedValue({ name: 'org-skill', report }); + + await runPRWorkflow( + mockOctokit, + createDefaultInputs({ + mode: 'analyze', + baseConfigPath: '.warden-org/warden.toml', + baseSkillRoot: '.warden-org', + }), + 'pull_request', + EVENT_PAYLOAD_PATH, + LAYERED_AUXILIARY_MODEL_FIXTURES_DIR ); + + const [, , , finalOptions] = mockWriteFindingsOutput.mock.calls[0]!; + expect(finalOptions?.skillExecutions).toEqual( + expect.arrayContaining([ + expect.objectContaining({ + auxiliaryModel: 'anthropic/org-aux-model', + }), + ]) + ); + }); + + it('report mode carries skillExecutionId and resolvedDefaults into the final findings output', async () => { + const finding = createFinding(); + const report = createSkillReport({ findings: [finding] }); + const findingsFile = writeFindingsArtifact([report], [ + { + triggerName: 'test-skill', + skillName: 'test-skill', + report, + }, + ]); + + try { + await runPRWorkflow( + mockOctokit, + createDefaultInputs({ mode: 'report', findingsFile }), + 'pull_request', + EVENT_PAYLOAD_PATH, + FIXTURES_DIR + ); + } finally { + rmSync(dirname(findingsFile), { recursive: true, force: true }); + } + + const [, , , finalOptions] = mockWriteFindingsOutput.mock.calls[0]!; + expect(finalOptions?.skillExecutions).toEqual([ + expect.objectContaining({ skillExecutionId: expect.any(String), triggerName: 'test-skill' }), + ]); + expect(finalOptions?.resolvedDefaults).toBeDefined(); + }); + + it('report mode round-trips analyze mode\'s auxiliaryModel/synthesisModel through the replay artifact', async () => { + // Regression: analyze mode's TriggerResult carries these two model + // lanes, but the replay artifact used to drop them, so report mode's + // export permanently lost them even though analyze mode had them. + const finding = createFinding(); + const report = createSkillReport({ findings: [finding] }); + const findingsFile = writeFindingsArtifact([report], [ + { + triggerName: 'test-skill', + skillName: 'test-skill', + report, + auxiliaryModel: 'anthropic/aux-model', + synthesisModel: 'anthropic/synth-model', + }, + ]); + + try { + await runPRWorkflow( + mockOctokit, + createDefaultInputs({ mode: 'report', findingsFile }), + 'pull_request', + EVENT_PAYLOAD_PATH, + FIXTURES_DIR + ); + } finally { + rmSync(dirname(findingsFile), { recursive: true, force: true }); + } + + const [, , , finalOptions] = mockWriteFindingsOutput.mock.calls[0]!; + expect(finalOptions?.skillExecutions).toEqual([ + expect.objectContaining({ + auxiliaryModel: 'anthropic/aux-model', + synthesisModel: 'anthropic/synth-model', + }), + ]); }); it('analyze mode fails when the findings artifact cannot be written', async () => { @@ -717,6 +815,47 @@ describe('runPRWorkflow', () => { ); }); + it('report mode rejects a legacy fallback join when 2+ current triggers share a name and skill', async () => { + // Regression: only ONE artifact row lacks triggerId here, so the old + // ambiguity check (which only looked for 2+ artifact rows or 2+ + // triggers sharing a triggerId) saw nothing ambiguous and would have + // silently bound this row to whichever trigger asked for it first — + // even though two current triggers share this row's fallback + // name+skill key and either could legitimately claim it. + const highFinding = createFinding({ id: 'high-finding', severity: 'high' }); + const lowFinding = createFinding({ id: 'low-finding', severity: 'low' }); + const highReport = createSkillReport({ summary: 'High report', findings: [highFinding] }); + const lowReport = createSkillReport({ summary: 'Low report', findings: [lowFinding] }); + const findingsFile = writeFindingsArtifact([highReport, lowReport], [ + { + triggerId: duplicateTriggerId('high'), + triggerName: 'test-skill', + skillName: 'test-skill', + report: highReport, + }, + { + // No triggerId: only this row needs the legacy name+skill fallback. + triggerName: 'test-skill', + skillName: 'test-skill', + report: lowReport, + }, + ]); + + try { + await expect( + runPRWorkflow( + mockOctokit, + createDefaultInputs({ mode: 'report', findingsFile }), + 'pull_request', + EVENT_PAYLOAD_PATH, + DUPLICATE_TRIGGER_FIXTURES_DIR + ) + ).rejects.toThrow('legacy name/skill fallback is ambiguous'); + } finally { + rmSync(dirname(findingsFile), { recursive: true, force: true }); + } + }); + it('report mode fails GitHub check write errors without creating in-progress checks', async () => { const report = createSkillReport({ findings: [createFinding()] }); const findingsFile = writeFindingsArtifact([report], [ @@ -1437,6 +1576,61 @@ describe('runPRWorkflow', () => { expect(semaphore).toBeInstanceOf(Semaphore); }); + it('writes a live snapshot after the trigger completes, carrying skillExecutionId and skippedTriggers', async () => { + mockRunSkillTask.mockResolvedValue({ name: 'test-trigger', report: createSkillReport({ skill: 'test-skill' }) }); + + await runPRWorkflow(mockOctokit, createDefaultInputs(), 'pull_request', EVENT_PAYLOAD_PATH, FIXTURES_DIR); + + expect(mockWriteFindingsOutputLive).toHaveBeenCalledTimes(1); + const [reportsSoFar, , , liveOptions] = mockWriteFindingsOutputLive.mock.calls[0]!; + expect(reportsSoFar).toHaveLength(1); + expect(liveOptions?.skillExecutions).toEqual([ + expect.objectContaining({ skillExecutionId: expect.any(String), triggerName: 'test-skill' }), + ]); + expect(liveOptions?.skippedTriggers).toEqual([]); + + // The final write happens after the live write and includes the same enrichment. + const [, , , finalOptions] = mockWriteFindingsOutput.mock.calls[0]!; + expect(finalOptions?.skillExecutions).toEqual([ + expect.objectContaining({ skillExecutionId: expect.any(String), triggerName: 'test-skill' }), + ]); + }); + + it('clears a stale .done marker before the first trigger settles, not lazily on the first live write', async () => { + mockRunSkillTask.mockResolvedValue({ name: 'test-trigger', report: createSkillReport({ skill: 'test-skill' }) }); + + await runPRWorkflow(mockOctokit, createDefaultInputs(), 'pull_request', EVENT_PAYLOAD_PATH, FIXTURES_DIR); + + expect(mockClearStaleDoneMarker).toHaveBeenCalledTimes(1); + expect(mockClearStaleDoneMarker.mock.invocationCallOrder[0]!) + .toBeLessThan(mockRunSkillTask.mock.invocationCallOrder[0]!); + }); + + it('computes checkConclusion from confidence-filtered findings, matching what actually posts to the check run', async () => { + // High severity but low confidence, with minConfidence defaulting to + // 'medium': the finding is filtered out of the posted check's + // conclusion, so the real check run succeeds even though failOn: + // 'high' + failCheck: true would fail on the raw findings. + const lowConfidenceFinding = createFinding({ severity: 'high', confidence: 'low' }); + mockRunSkillTask.mockResolvedValue({ + name: 'test-trigger', + report: createSkillReport({ skill: 'test-skill', findings: [lowConfidenceFinding] }), + }); + + await runPRWorkflow( + mockOctokit, + createDefaultInputs({ failOn: 'high', failCheck: true }), + 'pull_request', + EVENT_PAYLOAD_PATH, + FIXTURES_DIR + ); + + const [, , , finalOptions] = mockWriteFindingsOutput.mock.calls[0]!; + expect(finalOptions?.skillExecutions).toEqual([ + expect.objectContaining({ checkConclusion: 'success' }), + ]); + }); + it('honors the parallel input when dispatching matched triggers', async () => { let activeRuns = 0; let maxActiveRuns = 0; @@ -1492,6 +1686,40 @@ describe('runPRWorkflow', () => { expect(maxActiveRuns).toBe(1); }); + it('accounts for a trigger the circuit breaker aborted before dispatch, and fails the run since nothing succeeded', async () => { + // Regression: runPool never dispatches work past an abort, so a second + // matched trigger left queued when the circuit opens used to vanish + // from the export entirely, and the all-failed check (which only + // counted triggers that actually produced an error) could see fewer + // errors than matched triggers and let a zero-success run succeed. + mockRunSkillTask.mockImplementation(async (taskOptions) => { + taskOptions.runnerOptions?.circuitBreaker?.recordFailure('auth_failed', 'provider outage'); + throw new Error('boom'); + }); + + await expect( + runPRWorkflow( + mockOctokit, + createDefaultInputs({ parallel: 1 }), + 'pull_request', + EVENT_PAYLOAD_PATH, + DUPLICATE_TRIGGER_FIXTURES_DIR + ) + ).rejects.toThrow(/All 2 trigger\(s\) failed/); + + // Only the first trigger was actually dispatched before the circuit opened. + expect(mockRunSkillTask).toHaveBeenCalledTimes(1); + + // Both matched triggers must be accounted for: the one that actually + // ran and errored, and the one the circuit aborted before dispatch — + // neither should silently vanish from the export. + const finalOptions = mockWriteFindingsOutput.mock.calls.at(-1)?.[3]; + expect(finalOptions?.skippedTriggers).toEqual([ + expect.objectContaining({ skillName: 'test-skill', reason: 'error' }), + expect.objectContaining({ skillName: 'test-skill', reason: 'error' }), + ]); + }); + it('records trigger failure and updates check before failing', async () => { // When all triggers fail, the workflow should still update the check // before calling setFailed. @@ -1878,6 +2106,13 @@ describe('runPRWorkflow', () => { }), }) ); + + // skipped-skill's paths filter (docs/**) doesn't match this event's + // changed files — deriveSkippedReason's path_filter fallthrough. + const [, , , finalOptions] = mockWriteFindingsOutput.mock.calls[0]!; + expect(finalOptions?.skippedTriggers).toEqual([ + expect.objectContaining({ skillName: 'skipped-skill', reason: 'path_filter' }), + ]); }); it('creates neutral checks for triggers skipped by pull request action', async () => { @@ -1904,6 +2139,13 @@ describe('runPRWorkflow', () => { }), }) ); + + // The trigger only fires on 'labeled'; this fixture replays an 'opened' + // event — deriveSkippedReason's no_event_match branch. + const [, , , finalOptions] = mockWriteFindingsOutput.mock.calls[0]!; + expect(finalOptions?.skippedTriggers).toEqual([ + expect.objectContaining({ skillName: 'labeled-skill', reason: 'no_event_match' }), + ]); }); }); @@ -2428,7 +2670,8 @@ describe('runPRWorkflow', () => { skill: undefined, resolvedReason: 'fix_evaluation', }), - ] + ], + expect.any(Object) ); }); diff --git a/packages/warden/src/action/workflow/pr-workflow.ts b/packages/warden/src/action/workflow/pr-workflow.ts index 879668db2..290e997dc 100644 --- a/packages/warden/src/action/workflow/pr-workflow.ts +++ b/packages/warden/src/action/workflow/pr-workflow.ts @@ -24,7 +24,7 @@ import type { ResolvedTrigger, } from '../../config/loader.js'; import { buildEventContext } from '../../event/context.js'; -import { matchTrigger, shouldFail, countFindingsAtOrAbove } from '../../triggers/matcher.js'; +import { matchTrigger, matchPullRequestState, shouldFail, countFindingsAtOrAbove } from '../../triggers/matcher.js'; import { fetchExistingComments } from '../../output/dedup.js'; import type { ExistingComment } from '../../output/dedup.js'; import { buildAnalyzedScope, findStaleComments, resolveStaleComments } from '../../output/stale.js'; @@ -60,6 +60,7 @@ import { updateSkillCheck, buildCoreSummaryData, determineCoreConclusion, + determineConclusion, type CheckOptions, type CoreCheckSummaryData, } from '../checks/manager.js'; @@ -67,6 +68,7 @@ import { setOutput, setFailed, ActionFailedError, + clearStaleDoneMarker, ensureClaudeAuth, logGroup, logGroupEnd, @@ -77,12 +79,18 @@ import { setWorkflowOutputs, getAuthenticatedBotLogin, writeFindingsOutput, + writeFindingsOutputLive, } from './base.js'; import { renderSkillReport } from '../../output/renderer.js'; +import type { z } from 'zod'; import { FindingsOutputSchema, + buildBaseOutputOptions, + type SkippedTriggerReasonSchema, type FindingsOutput, type ReplayTriggerResult, + type SkillExecutionMeta, + type BuildFindingsOutputOptions, } from '../reporting/output.js'; // ----------------------------------------------------------------------------- @@ -175,6 +183,92 @@ function checkOptionsForPullRequest(context: EventContext): CheckOptions | undef }; } +/** + * The only caller, `toSkippedTriggers`, is always fed a list pre-filtered by + * `reportsPullRequestCheck` to `pull_request`/`'*'`-type triggers, so this + * only ever needs to explain why a PR-scoped trigger didn't fire this run — + * not schedule/local triggers, which never reach this function. + */ +function deriveSkippedReason(trigger: ResolvedTrigger, context: EventContext): z.infer { + if (trigger.type === 'pull_request') { + if (context.eventType !== 'pull_request') return 'no_event_match'; + if (!trigger.actions?.includes(context.action)) return 'no_event_match'; + if (!matchPullRequestState(trigger, context)) { + if (context.action === 'labeled' && trigger.labels !== undefined) { + const eventLabelMatches = context.label !== undefined && trigger.labels.includes(context.label); + if (!eventLabelMatches) return 'label_mismatch'; + } + const labels = context.pullRequest?.labels ?? []; + const labelMatches = trigger.labels?.some((label) => labels.includes(label)); + if (trigger.labels !== undefined && !labelMatches) return 'label_mismatch'; + return 'draft_state'; + } + } + return 'path_filter'; +} + +function toSkippedTriggers( + skippedTriggers: ResolvedTrigger[], + context: EventContext +): NonNullable { + return skippedTriggers.map((t) => ({ + skillName: t.skill, + triggerId: t.id, + triggerName: t.name, + reason: deriveSkippedReason(t, context), + })); +} + +/** + * A trigger that threw before producing a report has no `report`, so + * `toSkillExecutions`'s filter (which requires one) can never include it — + * without this, an errored trigger vanishes from the export entirely aside + * from a console warning and (in analyze/report mode) a `triggerResults` + * row. Surfacing it here instead keeps it visible in the same place a + * schedule-mode trigger error is now surfaced. + */ +function toErroredSkippedTriggers( + results: TriggerResult[] +): NonNullable { + return results + .filter((r) => r.error && !r.report) + .map((r) => ({ + skillName: r.skillName, + triggerId: r.triggerId, + triggerName: r.triggerName, + reason: 'error' as const, + })); +} + +/** Build per-execution metadata for the findings output from settled trigger results. */ +function toSkillExecutions(results: TriggerResult[]): SkillExecutionMeta[] { + return results + .filter((r): r is TriggerResult & { report: SkillReport } => Boolean(r.report)) + .map((r) => ({ + report: r.report, + skillExecutionId: r.skillExecutionId, + triggerId: r.triggerId, + triggerName: r.triggerName, + checkRunUrl: r.checkRunUrl, + checkRunId: r.checkRunId, + auxiliaryModel: r.auxiliaryModel, + synthesisModel: r.synthesisModel, + reviewEvent: r.reviewEventPosted, + // Matches buildSkillCheckPayload's own conclusion computation + // (confidence-filtered first) so this mirrors what actually posted to + // the check run at checkRunUrl/checkRunId. determineConclusion never + // returns 'cancelled' — that value exists on CheckConclusion for actual + // check-run API responses (aborted runs), which this export doesn't + // currently read from. + checkConclusion: determineConclusion( + filterFindings(r.report.findings, undefined, r.minConfidence), + r.failOn, + r.failCheck + ), + findingProcessingEvents: r.findingProcessingEvents, + })); +} + function resolveWorkflowAuxiliaryOptions(layered: LoadedLayeredConfig): AuxiliaryWorkflowOptions { const baseDefaults = layered.baseConfig?.defaults; const repoDefaults = layered.repoConfig?.defaults ?? layered.config.defaults; @@ -463,6 +557,7 @@ function createTriggerCheckReporter( const check = await createSkillCheck(octokit, skillName, checkOptions); return { url: check.url, + checkRunId: check.checkRunId, complete: (report, options) => updateSkillCheck(octokit, check.checkRunId, report, { ...checkOptions, @@ -479,7 +574,11 @@ async function executeAllTriggers( context: EventContext, runnerConcurrency: number | undefined, inputs: ActionInputs, - options: { checks?: TriggerCheckReporter } = {} + options: { + checks?: TriggerCheckReporter; + /** Fired after each trigger settles, with every result settled so far (completion order, not input order). */ + onTriggerComplete?: (completedSoFar: TriggerResult[]) => void; + } = {} ): Promise { const concurrency = runnerConcurrency ?? inputs.parallel; const runtimeEnv = await prepareRuntimeEnvironment(matchedTriggers, inputs); @@ -487,13 +586,14 @@ async function executeAllTriggers( const semaphore = new Semaphore(concurrency); const abortController = new AbortController(); const circuitBreaker = new ProviderFailureCircuitBreaker({ abortController }); + const completedSoFar: TriggerResult[] = []; // Limit trigger dispatch too; the semaphore only gates work after a trigger starts. - return runPool( + const results = await runPool( matchedTriggers, concurrency, - (trigger) => - executeTrigger(trigger, { + async (trigger) => { + const result = await executeTrigger(trigger, { context, anthropicApiKey: inputs.anthropicApiKey, claudePath: runtimeEnv.pathToClaudeCodeExecutable, @@ -506,9 +606,35 @@ async function executeAllTriggers( abortController, circuitBreaker, checks: options.checks, - }), + }); + completedSoFar.push(result); + options.onTriggerComplete?.([...completedSoFar]); + return result; + }, { shouldAbort: () => abortController.signal.aborted }, ); + + // `runPool` never dispatches work items past an abort, so a matched trigger + // the circuit breaker aborted before it started doesn't appear in `results` + // at all — not even as an error. Synthesize an aborted result for each one + // so it's still accounted for in `skippedTriggers`/`triggerResults`, and so + // an all-dropped run can't look like zero errors occurred. + const dispatchedTriggerIds = new Set(results.map((result) => result.triggerId)); + const undispatched = matchedTriggers.filter((trigger) => !dispatchedTriggerIds.has(trigger.id)); + if (undispatched.length === 0) { + return results; + } + + const abortedResults: TriggerResult[] = undispatched.map((trigger) => ({ + triggerId: trigger.id, + skillExecutionId: trigger.skillExecutionId, + triggerName: trigger.name, + skillName: trigger.skill, + error: new Error('Trigger execution aborted before dispatch (circuit breaker tripped)'), + })); + completedSoFar.push(...abortedResults); + options.onTriggerComplete?.([...completedSoFar]); + return [...results, ...abortedResults]; } /** @@ -980,7 +1106,9 @@ async function finalizeWorkflow( failureReasons: string[], canResolveStale: boolean, gate: ReviewFeedbackGate, - triggerErrors: string[] + triggerErrors: string[], + skippedTriggers: ResolvedTrigger[], + inputs: ActionInputs ): Promise { await dismissPreviousReviewIfResolved( octokit, @@ -999,6 +1127,11 @@ async function finalizeWorkflow( try { const findingsPath = writeFindingsOutput(reports, context, findingObservations, { triggerResults: toReplayTriggerResults(results), + ...buildBaseOutputOptions(inputs, [ + ...toSkippedTriggers(skippedTriggers, context), + ...toErroredSkippedTriggers(results), + ]), + skillExecutions: toSkillExecutions(results), }); logAction(`Findings written to ${findingsPath}`); } catch (error) { @@ -1277,6 +1410,9 @@ function toReplayTriggerResults(results: TriggerResult[]): ReplayTriggerResult[] skillName: result.skillName, report: result.report, error: result.error, + findingProcessingEvents: result.findingProcessingEvents, + auxiliaryModel: result.auxiliaryModel, + synthesisModel: result.synthesisModel, })); } @@ -1351,6 +1487,7 @@ function buildReportModeResults( const maxFindings = trigger.maxFindings ?? inputs.maxFindings; const baseResult = { triggerId: trigger.id, + skillExecutionId: trigger.skillExecutionId, triggerName: trigger.name, skillName: trigger.skill, failOn, @@ -1361,9 +1498,24 @@ function buildReportModeResults( failCheck, maxFindings, }; - const outputResult = - outputResults.get(triggerReplayKey(trigger))?.shift() ?? - outputResults.get(resultKey(trigger.name, trigger.skill))?.shift(); + let outputResult = outputResults.get(triggerReplayKey(trigger))?.shift(); + if (!outputResult) { + // Only a legacy artifact (predating triggerId) reaches this fallback. + // If 2+ current triggers share this name+skill, the fallback can't + // tell them apart — fail loudly instead of silently binding a report + // to the wrong trigger's policy (failOn/reportOn/etc). + const fallbackKey = resultKey(trigger.name, trigger.skill); + const sameFallbackKeyTriggers = matchedTriggers.filter( + (t) => resultKey(t.name, t.skill) === fallbackKey + ); + if (sameFallbackKeyTriggers.length > 1) { + throw new Error( + `Findings file has no triggerId-matched result for trigger ${trigger.name} (${trigger.skill}), ` + + `and the legacy name/skill fallback is ambiguous: multiple current triggers share this name and skill` + ); + } + outputResult = outputResults.get(fallbackKey)?.shift(); + } if (!outputResult) { return { @@ -1385,6 +1537,9 @@ function buildReportModeResults( return { ...baseResult, report: outputResult.report, + findingProcessingEvents: outputResult.findingProcessingEvents, + auxiliaryModel: outputResult.auxiliaryModel, + synthesisModel: outputResult.synthesisModel, }; }); @@ -1447,7 +1602,9 @@ async function createCompletedSkillChecksForReport( minConfidence: result.minConfidence, failCheck: result.failCheck, }); - updatedResults.push(withRenderedReviewResult({ ...result, checkRunUrl: check.url })); + updatedResults.push( + withRenderedReviewResult({ ...result, checkRunUrl: check.url, checkRunId: check.checkRunId }) + ); continue; } @@ -1572,7 +1729,7 @@ async function finalizeReportWorkflow( canResolveStale: boolean, gate: ReviewFeedbackGate, triggerErrors: string[], - options: { failOnWriteError?: boolean } = {} + options: { failOnWriteError?: boolean; skippedTriggers?: ResolvedTrigger[]; inputs: ActionInputs } ): Promise { await dismissPreviousReviewIfResolved( octokit, @@ -1590,6 +1747,11 @@ async function finalizeReportWorkflow( try { const findingsPath = writeFindingsOutput(reports, context, findingObservations, { triggerResults: toReplayTriggerResults(results), + ...buildBaseOutputOptions(options.inputs, [ + ...toSkippedTriggers(options.skippedTriggers ?? [], context), + ...toErroredSkippedTriggers(results), + ]), + skillExecutions: toSkillExecutions(results), }); logAction(`Findings written to ${findingsPath}`); } catch (error) { @@ -1713,6 +1875,7 @@ async function runAnalyzeMode( context, runnerConcurrency, matchedTriggers, + skippedTriggers, skipCoreCheck, } = initResult; @@ -1721,7 +1884,10 @@ async function runAnalyzeMode( setOutput('high-count', 0); setOutput('summary', skipCoreCheck?.title ?? 'No triggers matched'); try { - const findingsPath = writeFindingsOutput([], context, [], { triggerResults: [] }); + const findingsPath = writeFindingsOutput([], context, [], { + triggerResults: [], + ...buildBaseOutputOptions(inputs, toSkippedTriggers(skippedTriggers, context)), + }); logAction(`Findings written to ${findingsPath}`); } catch (error) { setFailed(`Failed to write findings output: ${error}`); @@ -1736,7 +1902,18 @@ async function runAnalyzeMode( name: 'execute triggers', attributes: { 'warden.trigger.count': matchedTriggers.length }, }, - () => executeAllTriggers(matchedTriggers, context, runnerConcurrency, inputs), + () => executeAllTriggers(matchedTriggers, context, runnerConcurrency, inputs, { + onTriggerComplete: (completedSoFar) => { + const reportsSoFar = completedSoFar.flatMap((r) => (r.report ? [r.report] : [])); + writeFindingsOutputLive(reportsSoFar, context, [], { + ...buildBaseOutputOptions(inputs, [ + ...toSkippedTriggers(skippedTriggers, context), + ...toErroredSkippedTriggers(completedSoFar), + ]), + skillExecutions: toSkillExecutions(completedSoFar), + }); + }, + }), ); const reports = results.flatMap((result) => (result.report ? [result.report] : [])); @@ -1747,6 +1924,11 @@ async function runAnalyzeMode( try { const findingsPath = writeFindingsOutput(reports, context, [], { triggerResults: toReplayTriggerResults(results), + ...buildBaseOutputOptions(inputs, [ + ...toSkippedTriggers(skippedTriggers, context), + ...toErroredSkippedTriggers(results), + ]), + skillExecutions: toSkillExecutions(results), }); logAction(`Findings written to ${findingsPath}`); } catch (error) { @@ -1792,7 +1974,10 @@ async function runReportMode( const outputs = { findingsCount: 0, highCount: 0, summary: skipCoreCheck.title }; setWorkflowOutputs(outputs); try { - const findingsPath = writeFindingsOutput([], context, [], { triggerResults: [] }); + const findingsPath = writeFindingsOutput([], context, [], { + triggerResults: [], + ...buildBaseOutputOptions(inputs, toSkippedTriggers(skippedTriggers, context)), + }); logAction(`Findings written to ${findingsPath}`); } catch (error) { warnAction(`Failed to write findings output: ${error}`); @@ -1827,6 +2012,7 @@ async function runReportMode( try { const findingsPath = writeFindingsOutput([], context, cleanupFindingObservations, { triggerResults: [], + ...buildBaseOutputOptions(inputs, toSkippedTriggers(skippedTriggers, context)), }); logAction(`Findings written to ${findingsPath}`); } catch (error) { @@ -1899,7 +2085,7 @@ async function runReportMode( canResolveStale, gate, triggerErrors, - { failOnWriteError: true }, + { failOnWriteError: true, skippedTriggers, inputs }, ); } catch (error) { if (error instanceof ActionFailedError) { @@ -1926,6 +2112,8 @@ export async function runPRWorkflow( eventPath: string, repoPath: string ): Promise { + clearStaleDoneMarker(repoPath); + return Sentry.startSpan( { op: 'workflow.run', name: 'review pull_request' }, async (span) => { @@ -1988,7 +2176,9 @@ export async function runPRWorkflow( setOutput('high-count', 0); setOutput('summary', skipCoreCheck.title); try { - writeFindingsOutput([], context); + writeFindingsOutput([], context, [], { + ...buildBaseOutputOptions(inputs, toSkippedTriggers(skippedTriggers, context)), + }); } catch (error) { warnAction(`Failed to write findings output: ${error}`); } @@ -2008,7 +2198,9 @@ export async function runPRWorkflow( setOutput('high-count', 0); setOutput('summary', 'No triggers matched'); try { - writeFindingsOutput([], context, cleanupFindingObservations); + writeFindingsOutput([], context, cleanupFindingObservations, { + ...buildBaseOutputOptions(inputs, toSkippedTriggers(skippedTriggers, context)), + }); } catch (error) { warnAction(`Failed to write findings output: ${error}`); } @@ -2030,6 +2222,16 @@ export async function runPRWorkflow( }, () => executeAllTriggers(matchedTriggers, context, runnerConcurrency, inputs, { checks: createTriggerCheckReporter(octokit, context), + onTriggerComplete: (completedSoFar) => { + const reportsSoFar = completedSoFar.flatMap((r) => (r.report ? [r.report] : [])); + writeFindingsOutputLive(reportsSoFar, context, [], { + ...buildBaseOutputOptions(inputs, [ + ...toSkippedTriggers(skippedTriggers, context), + ...toErroredSkippedTriggers(completedSoFar), + ]), + skillExecutions: toSkillExecutions(completedSoFar), + }); + }, }), ); } catch (error) { @@ -2088,6 +2290,8 @@ export async function runPRWorkflow( canResolveStale, gate, triggerErrors, + skippedTriggers, + inputs, ); handleTriggerErrors(triggerErrors, matchedTriggers.length); diff --git a/packages/warden/src/action/workflow/schedule.test.ts b/packages/warden/src/action/workflow/schedule.test.ts index f8943cbd1..7bf666bdf 100644 --- a/packages/warden/src/action/workflow/schedule.test.ts +++ b/packages/warden/src/action/workflow/schedule.test.ts @@ -56,6 +56,8 @@ vi.mock('./base.js', async () => { return Promise.resolve({ pathToClaudeCodeExecutable: '/usr/local/bin/claude' }); }), getDefaultBranchFromAPI: vi.fn(() => Promise.resolve('main')), + writeFindingsOutputLive: vi.fn(actual['writeFindingsOutputLive'] as (...args: unknown[]) => void), + writeFindingsOutput: vi.fn(actual['writeFindingsOutput'] as (...args: unknown[]) => string), // Override handleTriggerErrors to use the mocked setFailed handleTriggerErrors: (triggerErrors: string[], totalTriggers: number) => { if (triggerErrors.length === 0) return; @@ -101,7 +103,7 @@ import { runSkill } from '../../sdk/runner.js'; import { buildScheduleEventContext } from '../../event/schedule-context.js'; import { createOrUpdateIssue } from '../../output/github-issues.js'; import { resolveSkillAsync } from '../../skills/loader.js'; -import { setFailed } from './base.js'; +import { setFailed, writeFindingsOutput, writeFindingsOutputLive } from './base.js'; import { runScheduleWorkflow } from './schedule.js'; import { clearSkillsCache } from '../../skills/loader.js'; @@ -111,6 +113,8 @@ const mockBuildContext = vi.mocked(buildScheduleEventContext); const mockCreateOrUpdateIssue = vi.mocked(createOrUpdateIssue); const mockResolveSkillAsync = vi.mocked(resolveSkillAsync); const mockSetFailed = vi.mocked(setFailed); +const mockWriteFindingsOutput = vi.mocked(writeFindingsOutput); +const mockWriteFindingsOutputLive = vi.mocked(writeFindingsOutputLive); // ----------------------------------------------------------------------------- // Mock Octokit Factory @@ -587,6 +591,11 @@ describe('runScheduleWorkflow', () => { expect(mockSetFailed).toHaveBeenCalledWith( expect.stringContaining('All 2 trigger(s) failed') ); + + // Regression: the run's one true final write (`.done` marker, + // `findings-file` output) must still happen even on an all-failed run — + // it must not be skipped by the all-failed error propagating first. + expect(mockWriteFindingsOutput).toHaveBeenCalledTimes(1); }); }); @@ -648,4 +657,108 @@ describe('runScheduleWorkflow', () => { ); }); }); + + // --------------------------------------------------------------------------- + // Live findings output + // --------------------------------------------------------------------------- + + describe('live findings output', () => { + it('writes a live snapshot after each trigger, marking not-yet-reached triggers as pending', async () => { + mockResolveSkillAsync + .mockResolvedValueOnce({ name: 'test-skill-a', description: 'Test skill A', prompt: 'Review code' }) + .mockResolvedValueOnce({ name: 'test-skill-b', description: 'Test skill B', prompt: 'Review code' }); + mockRunSkill + .mockResolvedValueOnce(createSkillReport({ skill: 'test-skill-a' })) + .mockResolvedValueOnce(createSkillReport({ skill: 'test-skill-b' })); + + await runScheduleWorkflow(mockOctokit, createDefaultInputs(), SCHEDULE_MULTI_FIXTURES); + + expect(mockWriteFindingsOutputLive).toHaveBeenCalledTimes(2); + expect(mockWriteFindingsOutput).toHaveBeenCalledTimes(1); + + const firstCallOptions = mockWriteFindingsOutputLive.mock.calls[0]?.[3]; + expect(firstCallOptions?.skippedTriggers).toEqual([ + expect.objectContaining({ skillName: 'test-skill-b', reason: 'pending' }), + ]); + expect(firstCallOptions?.skillExecutions).toHaveLength(1); + + const secondCallOptions = mockWriteFindingsOutputLive.mock.calls[1]?.[3]; + expect(secondCallOptions?.skippedTriggers).toEqual([]); + expect(secondCallOptions?.skillExecutions).toHaveLength(2); + + // The final write never has a 'pending' skip reason. + const finalOptions = mockWriteFindingsOutput.mock.calls[0]?.[3]; + expect(finalOptions?.skippedTriggers?.some((t) => t.reason === 'pending')).toBe(false); + }); + + it('marks a trigger with no matching files as skipped for no_changes', async () => { + mockResolveSkillAsync + .mockResolvedValueOnce({ name: 'test-skill-a', description: 'Test skill A', prompt: 'Review code' }) + .mockResolvedValueOnce({ name: 'test-skill-b', description: 'Test skill B', prompt: 'Review code' }); + + const contextWithFiles = createScheduleContext(); + const contextWithNoFiles = createScheduleContext(); + contextWithNoFiles.pullRequest = { ...contextWithNoFiles.pullRequest!, files: [] }; + mockBuildContext + .mockResolvedValueOnce(contextWithNoFiles) + .mockResolvedValueOnce(contextWithFiles); + mockRunSkill.mockResolvedValue(createSkillReport({ skill: 'test-skill-b' })); + + await runScheduleWorkflow(mockOctokit, createDefaultInputs(), SCHEDULE_MULTI_FIXTURES); + + const finalOptions = mockWriteFindingsOutput.mock.calls[0]?.[3]; + expect(finalOptions?.skippedTriggers).toEqual([ + expect.objectContaining({ skillName: 'test-skill-a', reason: 'no_changes' }), + ]); + }); + + it('carries skillExecutionId, triggerId, and issue metadata on skillExecutions in the final write', async () => { + mockResolveSkillAsync.mockResolvedValue({ name: 'test-skill', description: 'Test skill', prompt: 'Review code' }); + mockRunSkill.mockResolvedValue(createSkillReport()); + mockCreateOrUpdateIssue.mockResolvedValue({ + issueNumber: 42, + issueUrl: 'https://github.com/test-owner/test-repo/issues/42', + created: true, + }); + + await runScheduleWorkflow(mockOctokit, createDefaultInputs(), SCHEDULE_FIXTURES); + + const finalOptions = mockWriteFindingsOutput.mock.calls[0]?.[3]; + expect(finalOptions?.skillExecutions).toEqual([ + expect.objectContaining({ + skillExecutionId: expect.any(String), + triggerId: expect.any(String), + triggerName: expect.any(String), + issueNumber: 42, + issueUrl: 'https://github.com/test-owner/test-repo/issues/42', + findingProcessingEvents: [], + }), + ]); + }); + + it('keeps a report\'s execution metadata in the final export even when its issue write throws', async () => { + // Regression: skillExecutions used to be pushed only after + // createOrUpdateIssue resolved, even though the report itself is + // pushed to allReports beforehand. A throw from that GitHub write lost + // the report's join key and captured provenance events while leaving + // the report itself in the final artifact. + const finding = createFinding(); + mockRunSkill.mockResolvedValue(createSkillReport({ findings: [finding] })); + mockCreateOrUpdateIssue.mockRejectedValue(new Error('issue API down')); + + await expect( + runScheduleWorkflow(mockOctokit, createDefaultInputs(), SCHEDULE_FIXTURES) + ).rejects.toThrow('setFailed'); + + const finalCall = mockWriteFindingsOutput.mock.calls[0]; + expect(finalCall?.[0]).toEqual([expect.objectContaining({ findings: [finding] })]); + expect(finalCall?.[3]?.skillExecutions).toEqual([ + expect.objectContaining({ + skillExecutionId: expect.any(String), + triggerId: expect.any(String), + triggerName: expect.any(String), + }), + ]); + }); + }); }); diff --git a/packages/warden/src/action/workflow/schedule.ts b/packages/warden/src/action/workflow/schedule.ts index 1d214fbe0..aba0f96b6 100644 --- a/packages/warden/src/action/workflow/schedule.ts +++ b/packages/warden/src/action/workflow/schedule.ts @@ -20,22 +20,34 @@ import { createOrUpdateIssue } from '../../output/github-issues.js'; import { shouldFail, countFindingsAtOrAbove, countSeverity } from '../../triggers/matcher.js'; import { resolveSkillAsync } from '../../skills/loader.js'; import { filterFindings } from '../../types/index.js'; -import type { SkillReport } from '../../types/index.js'; +import type { EventContext, SkillReport } from '../../types/index.js'; +import type { FindingProcessingEvent } from '../../sdk/types.js'; import { Sentry, logger, setRepositoryScope, emitRunMetric } from '../../sentry.js'; import type { ActionInputs } from '../inputs.js'; +import { buildBaseOutputOptions } from '../reporting/output.js'; +import type { SkillExecutionMeta } from '../reporting/output.js'; import { setOutput, setFailed, ActionFailedError, + clearStaleDoneMarker, logGroup, logGroupEnd, prepareRuntimeEnvironment, handleTriggerErrors, getDefaultBranchFromAPI, writeFindingsOutput, + writeFindingsOutputLive, } from './base.js'; import { captureActionTriggerError } from '../error-reporting.js'; +interface SkippedScheduleTrigger { + skillName: string; + triggerId?: string; + triggerName?: string; + reason: 'no_changes' | 'pending' | 'error'; +} + // ----------------------------------------------------------------------------- // Main Schedule Workflow // ----------------------------------------------------------------------------- @@ -64,6 +76,7 @@ async function runScheduleWorkflowInner( ): Promise { const githubRepository = process.env['GITHUB_REPOSITORY']; setRepositoryScope(githubRepository); + clearStaleDoneMarker(repoPath); logGroup('Loading configuration'); if (inputs.baseConfigPath) { @@ -106,7 +119,7 @@ async function runScheduleWorkflowInner( action: 'scheduled', repository: { owner: o, name: n, fullName, defaultBranch: '' }, repoPath, - }); + }, [], buildBaseOutputOptions(inputs, [])); } catch (writeError) { console.error(`::warning::Failed to write findings output: ${writeError}`); } @@ -137,7 +150,7 @@ async function runScheduleWorkflowInner( action: 'scheduled', repository: { owner: o, name: n, fullName, defaultBranch: '' }, repoPath, - }); + }, [], buildBaseOutputOptions(inputs, [])); } catch (writeError) { console.error(`::warning::Failed to write findings output: ${writeError}`); } @@ -166,15 +179,38 @@ async function runScheduleWorkflowInner( } logGroupEnd(); + const scheduleContext: EventContext = { + eventType: 'schedule', + action: 'scheduled', + repository: { owner, name: repo, fullName: `${owner}/${repo}`, defaultBranch }, + repoPath, + }; + const allReports: SkillReport[] = []; + const skillExecutions: SkillExecutionMeta[] = []; + const skippedTriggers: SkippedScheduleTrigger[] = []; let totalFindings = 0; const failureReasons: string[] = []; const triggerErrors: string[] = []; let shouldFailAction = false; + const writeLiveSnapshot = (processedCount: number): void => { + const pending: SkippedScheduleTrigger[] = scheduleTriggers.slice(processedCount + 1).map((t) => ({ + skillName: t.skill, + triggerId: t.id, + triggerName: t.name, + reason: 'pending', + })); + writeFindingsOutputLive([...allReports], scheduleContext, [], { + ...buildBaseOutputOptions(inputs, [...skippedTriggers, ...pending]), + skillExecutions: [...skillExecutions], + }); + }; + // Process each schedule trigger - for (const resolved of scheduleTriggers) { + for (const [triggerIndex, resolved] of scheduleTriggers.entries()) { logGroup(`Running trigger: ${resolved.name} (skill: ${resolved.skill})`); + const findingProcessingEvents: FindingProcessingEvent[] = []; try { assertValidPiModelSelectors([resolved]); @@ -198,7 +234,9 @@ async function runScheduleWorkflowInner( // Skip if no matching files if (!context.pullRequest?.files.length) { console.log(`No files match trigger ${resolved.name}`); + skippedTriggers.push({ skillName: resolved.skill, triggerId: resolved.id, triggerName: resolved.name, reason: 'no_changes' }); logGroupEnd(); + writeLiveSnapshot(triggerIndex); continue; } @@ -227,12 +265,30 @@ async function runScheduleWorkflowInner( verifyFindings: resolved.verifyFindings, triggerName: resolved.name, pathToClaudeCodeExecutable: runtimeEnv.pathToClaudeCodeExecutable, + callbacks: { + onFindingProcessing: (event) => findingProcessingEvents.push(event), + }, }); console.log(`Found ${report.findings.length} findings`); allReports.push(report); totalFindings += report.findings.length; + // Pushed before the fallible issue write below: if createOrUpdateIssue + // throws, allReports (and thus the final export) already has this + // report, so its execution metadata (join key, model lanes, captured + // provenance events) must already be recorded too, not lost with it. + const executionMeta: (typeof skillExecutions)[number] = { + report, + skillExecutionId: resolved.skillExecutionId, + triggerId: resolved.id, + triggerName: resolved.name, + auxiliaryModel: resolved.auxiliaryModel, + synthesisModel: resolved.synthesisModel, + findingProcessingEvents, + }; + skillExecutions.push(executionMeta); + // Create/update issue with findings const scheduleConfig: Partial = resolved.schedule ?? {}; const issueTitle = scheduleConfig.issueTitle ?? `Warden: ${resolved.name}`; @@ -245,6 +301,8 @@ async function runScheduleWorkflowInner( if (issueResult) { console.log(`${issueResult.created ? 'Created' : 'Updated'} issue #${issueResult.issueNumber}`); console.log(`Issue URL: ${issueResult.issueUrl}`); + executionMeta.issueNumber = issueResult.issueNumber; + executionMeta.issueUrl = issueResult.issueUrl; } // Check failure condition @@ -259,6 +317,7 @@ async function runScheduleWorkflowInner( } logGroupEnd(); + writeLiveSnapshot(triggerIndex); } catch (error) { if (error instanceof ActionFailedError) throw error; captureActionTriggerError(error, { @@ -267,13 +326,13 @@ async function runScheduleWorkflowInner( }); const errorMessage = error instanceof Error ? error.message : String(error); triggerErrors.push(`${resolved.name}: ${errorMessage}`); + skippedTriggers.push({ skillName: resolved.skill, triggerId: resolved.id, triggerName: resolved.name, reason: 'error' }); console.error(`::warning::Trigger ${resolved.name} failed: ${error}`); logGroupEnd(); + writeLiveSnapshot(triggerIndex); } } - handleTriggerErrors(triggerErrors, scheduleTriggers.length); - // Set outputs const highCount = countSeverity(allReports, 'high'); workflowSpan.setAttribute('warden.finding.count', totalFindings); @@ -283,18 +342,22 @@ async function runScheduleWorkflowInner( setOutput('summary', allReports.map((r) => r.summary).join('\n') || 'Scheduled analysis complete'); // Write structured findings to file for external export (GCS, S3, etc.) + // before any all-failed/shouldFail error can propagate — this is the run's + // one true final write (`.done` marker + `findings-file` output), and it + // must land even when every trigger failed, or a terminated run is left + // looking permanently in-progress to a follower of the live snapshots. try { - const findingsPath = writeFindingsOutput(allReports, { - eventType: 'schedule', - action: 'scheduled', - repository: { owner, name: repo, fullName: `${owner}/${repo}`, defaultBranch }, - repoPath, + const findingsPath = writeFindingsOutput(allReports, scheduleContext, [], { + ...buildBaseOutputOptions(inputs, skippedTriggers), + skillExecutions, }); console.log(`Findings written to ${findingsPath}`); } catch (error) { console.error(`::warning::Failed to write findings output: ${error}`); } + handleTriggerErrors(triggerErrors, scheduleTriggers.length); + if (shouldFailAction) { setFailed(failureReasons.join('; ')); } diff --git a/packages/warden/src/config/loader.ts b/packages/warden/src/config/loader.ts index a6346e3f2..25e5474bf 100644 --- a/packages/warden/src/config/loader.ts +++ b/packages/warden/src/config/loader.ts @@ -1,4 +1,5 @@ import { readFileSync, existsSync } from 'node:fs'; +import { createHash } from 'node:crypto'; import { join, normalize } from 'node:path'; import { parse as parseToml } from 'smol-toml'; import { Sentry } from '../sentry.js'; @@ -362,6 +363,8 @@ export function loadLayeredWardenConfig( export interface ResolvedTrigger { /** Stable replay identity derived from the skill and trigger configuration */ id: string; + /** Short, stable join key for this skill×trigger execution, derived from `id`. */ + skillExecutionId: string; /** Skill name (used for display and deduplication) */ name: string; /** Skill reference (same as name, for downstream compatibility) */ @@ -423,6 +426,11 @@ export interface ResolvedTrigger { schedule?: ScheduleConfig; } +/** `id` is a verbose JSON blob, too unwieldy for a repeated cross-artifact join key. */ +function deriveSkillExecutionId(identity: string): string { + return createHash('sha256').update(identity).digest('hex').slice(0, 12); +} + function triggerIdentity(skill: SkillConfig, trigger: SkillTrigger | undefined): string { return JSON.stringify({ skill: skill.name, @@ -539,6 +547,7 @@ export function resolveSkillConfigs( // Wildcard: no triggers means run everywhere result.push({ id: triggerIdentity(skill, undefined), + skillExecutionId: deriveSkillExecutionId(triggerIdentity(skill, undefined)), name: skill.name, skill: skill.name, type: '*', @@ -570,6 +579,7 @@ export function resolveSkillConfigs( for (const trigger of skill.triggers) { result.push({ id: triggerIdentity(skill, trigger), + skillExecutionId: deriveSkillExecutionId(triggerIdentity(skill, trigger)), name: skill.name, skill: skill.name, type: trigger.type, diff --git a/packages/warden/src/output/dedup.test.ts b/packages/warden/src/output/dedup.test.ts index b7f4e5736..430e1836c 100644 --- a/packages/warden/src/output/dedup.test.ts +++ b/packages/warden/src/output/dedup.test.ts @@ -2,6 +2,7 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import type { Octokit } from '@octokit/rest'; import { generateContentHash, + generateLocationHashKey, generateFindingMetadata, generateMarker, parseWardenFindingMetadata, @@ -38,6 +39,25 @@ describe('generateContentHash', () => { }); }); +describe('generateLocationHashKey', () => { + it('disambiguates identical content hashes at different locations', () => { + const hash = generateContentHash('Same title', 'Same description'); + const keyA = generateLocationHashKey('src/a.ts', 1, hash); + const keyB = generateLocationHashKey('src/b.ts', 99, hash); + expect(keyA).not.toBe(keyB); + }); + + it('produces the same key for the same path, line, and hash', () => { + const hash = generateContentHash('Title', 'Description'); + expect(generateLocationHashKey('src/a.ts', 1, hash)).toBe(generateLocationHashKey('src/a.ts', 1, hash)); + }); + + it('treats an undefined path as an empty string', () => { + const hash = generateContentHash('Title', 'Description'); + expect(generateLocationHashKey(undefined, 0, hash)).toBe(`:0:${hash}`); + }); +}); + describe('generateMarker', () => { it('generates marker in expected format', () => { const marker = generateMarker('src/db.ts', 42, 'a1b2c3d4'); @@ -358,6 +378,7 @@ describe('deduplicateFindings', () => { expect(result.duplicateActions[0]!.type).toBe('update_warden'); expect(result.duplicateActions[0]!.matchType).toBe('hash'); expect(result.duplicateActions[0]!.finding.id).toBe('WRZ-XPL'); + expect(result.duplicateActions[0]!.finding.reportedId).toBe('WRZ-XPL'); }); it('keeps findings with different content', async () => { diff --git a/packages/warden/src/output/dedup.ts b/packages/warden/src/output/dedup.ts index 3a0a553e1..d2ad7fa5b 100644 --- a/packages/warden/src/output/dedup.ts +++ b/packages/warden/src/output/dedup.ts @@ -95,6 +95,16 @@ export function generateContentHash(title: string, description: string): string return createHash('sha256').update(content).digest('hex').slice(0, 8); } +/** + * Location+content key that disambiguates findings sharing the same content + * hash (e.g. identical generic wording from two different skills at two + * different locations). Mirrors the key this file already builds inline for + * existing-comment matching — reuse this instead of hashing content alone. + */ +export function generateLocationHashKey(path: string | undefined, line: number, hash: string): string { + return `${path ?? ''}:${line}:${hash}`; +} + /** * Generate the marker HTML comment to embed in comment body. * Format: @@ -915,7 +925,7 @@ export async function deduplicateFindings( if (matchingComment) { const duplicateFinding = matchingComment.isWarden && matchingComment.findingId - ? { ...finding, id: matchingComment.findingId } + ? { ...finding, id: matchingComment.findingId, reportedId: matchingComment.findingId } : finding; duplicateActions.push({ type: matchingComment.isWarden ? 'update_warden' : 'react_external', @@ -950,7 +960,7 @@ export async function deduplicateFindings( const matchingComment = semanticResult.matches.get(finding.id); if (matchingComment) { const duplicateFinding = matchingComment.isWarden && matchingComment.findingId - ? { ...finding, id: matchingComment.findingId } + ? { ...finding, id: matchingComment.findingId, reportedId: matchingComment.findingId } : finding; duplicateActions.push({ type: matchingComment.isWarden ? 'update_warden' : 'react_external', diff --git a/packages/warden/src/sdk/extract.test.ts b/packages/warden/src/sdk/extract.test.ts index 5c9bbdc61..b92fb5194 100644 --- a/packages/warden/src/sdk/extract.test.ts +++ b/packages/warden/src/sdk/extract.test.ts @@ -196,6 +196,84 @@ describe('mergeCrossLocationFindings', () => { })); }); + it('still attributes the absorbed finding to its winner when both share the exact same location', async () => { + // Regression: mergeGroupLocations seeds `seen` with the winner's own + // location, so a loser at that same location is filtered out of + // additionalLocations entirely — findReplacementForAbsorbed used to + // location-match against additionalLocations only, so it silently + // returned no replacement (and buildProvenanceAndDiscarded would then + // drop the whole merge event) in exactly this case. + const findings = [ + makeFinding({ + id: 'f1', + severity: 'high', + title: 'Missing null check', + location: { path: 'src/a.ts', startLine: 3 }, + }), + makeFinding({ + id: 'f2', + severity: 'medium', + title: 'Missing null check (duplicate wording)', + location: { path: 'src/a.ts', startLine: 3 }, + }), + ]; + + mockCallHaiku.mockResolvedValue({ + success: true, + data: [[1, 2]], + usage: { inputTokens: 100, outputTokens: 10, costUSD: 0.001 }, + }); + + const onFindingProcessing = vi.fn(); + const result = await mergeCrossLocationFindings(findings, { + apiKey: 'test-key', + repoPath: tempDir, + onFindingProcessing, + }); + + expect(result.findings).toHaveLength(1); + expect(onFindingProcessing).toHaveBeenCalledWith(expect.objectContaining({ + stage: 'merge', + action: 'merged', + finding: findings[1], + replacement: expect.objectContaining({ id: 'f1' }), + })); + }); + + it('resolves a chained/overlapping merge to the final survivor, not an intermediate winner', async () => { + // Regression: two overlapping groups where f2 is absorbed into f1, then + // f1 itself is absorbed into f3. f2's replacement must resolve to f3 (the + // finding that actually survives into `result.findings`), not f1 — f1 + // stops existing in the output once its own group runs. + const findings = [ + makeFinding({ id: 'f1', severity: 'medium', title: 'Same root cause', location: { path: 'src/a.ts', startLine: 1 } }), + makeFinding({ id: 'f2', severity: 'low', title: 'Same root cause', location: { path: 'src/b.ts', startLine: 2 } }), + makeFinding({ id: 'f3', severity: 'high', title: 'Same root cause', location: { path: 'src/a.ts', startLine: 5 } }), + ]; + + mockCallHaiku.mockResolvedValue({ + success: true, + data: [[1, 2], [1, 3]], + usage: { inputTokens: 100, outputTokens: 10, costUSD: 0.001 }, + }); + + const onFindingProcessing = vi.fn(); + const result = await mergeCrossLocationFindings(findings, { + apiKey: 'test-key', + repoPath: tempDir, + onFindingProcessing, + }); + + expect(result.findings).toHaveLength(1); + expect(result.findings[0]!.id).toBe('f3'); + expect(onFindingProcessing).toHaveBeenCalledWith(expect.objectContaining({ + stage: 'merge', + action: 'merged', + finding: findings[1], + replacement: expect.objectContaining({ id: 'f3' }), + })); + }); + it('merges 3+ locations in one group', async () => { const findings = [ makeFinding({ id: 'f1', severity: 'medium', location: { path: 'src/a.ts', startLine: 1 } }), diff --git a/packages/warden/src/sdk/extract.ts b/packages/warden/src/sdk/extract.ts index b7ef0229a..a935301de 100644 --- a/packages/warden/src/sdk/extract.ts +++ b/packages/warden/src/sdk/extract.ts @@ -396,6 +396,8 @@ interface ApplyGroupsResult { absorbed: Set; /** Map from original winner finding to its merged replacement (with additionalLocations) */ replacements: Map; + /** Map from each absorbed finding to the (pre-merge) winner that absorbed it. */ + absorbedToWinner: Map; } /** @@ -415,6 +417,7 @@ export function applyMergeGroups( ): ApplyGroupsResult { const absorbed = new Set(); const replacements = new Map(); + const absorbedToWinner = new Map(); for (const group of groups) { const uniqueIndices = [...new Set(group)]; @@ -447,28 +450,38 @@ export function applyMergeGroups( for (const f of groupFindings) { if (f !== winner) { absorbed.add(f); + absorbedToWinner.set(f, winner); } } } - return { absorbed, replacements }; -} - -function sameLocation(a: Location | undefined, b: Location | undefined): boolean { - return Boolean(a && b && locationKey(a) === locationKey(b)); + return { absorbed, replacements, absorbedToWinner }; } +/** + * The winner an absorbed finding was recorded against may itself have gone on + * to be absorbed into a later, overlapping group's winner (e.g. groups + * `[[1,2],[1,3]]`: finding 2 absorbed into 1, then 1 itself absorbed into 3). + * `absorbedToWinner` only records the immediate winner at absorption time, so + * walk it forward to the final survivor — a finding, once absorbed, is + * excluded from every later group (see the `!absorbed.has(f)` filter above), + * so it can never become a winner again, and this chain can't cycle. + * Looking this up by winner identity (rather than re-deriving it from + * location) also avoids silently losing the link when the absorbed finding's + * location coincides with the winner's own primary location, which isn't + * present in the winner's `additionalLocations`. + */ function findReplacementForAbsorbed( finding: Finding, - replacements: Map + replacements: Map, + absorbedToWinner: Map ): Finding | undefined { - for (const replacement of replacements.values()) { - if (replacement.additionalLocations?.some((loc) => sameLocation(loc, finding.location))) { - return replacement; - } + let winner = absorbedToWinner.get(finding); + if (!winner) return undefined; + for (let next = absorbedToWinner.get(winner); next; next = absorbedToWinner.get(winner)) { + winner = next; } - - return undefined; + return replacements.get(winner) ?? winner; } /** Schema for LLM merge response: groups of finding indices sharing a root cause. */ @@ -562,7 +575,7 @@ Singletons should not appear. Return [] if no findings describe the same issue.` return { findings, mergedCount: 0, usage: result.usage }; } - const { absorbed, replacements } = applyMergeGroups(withLocations, result.data); + const { absorbed, replacements, absorbedToWinner } = applyMergeGroups(withLocations, result.data); if (absorbed.size === 0) { return { findings, mergedCount: 0, usage: result.usage }; @@ -573,7 +586,7 @@ Singletons should not appear. Return [] if no findings describe the same issue.` stage: 'merge', action: 'merged', finding, - replacement: findReplacementForAbsorbed(finding, replacements), + replacement: findReplacementForAbsorbed(finding, replacements, absorbedToWinner), reason: 'same root cause at another location', }); } diff --git a/packages/warden/src/triggers/matcher.test.ts b/packages/warden/src/triggers/matcher.test.ts index 360588f3d..1ed3907a0 100644 --- a/packages/warden/src/triggers/matcher.test.ts +++ b/packages/warden/src/triggers/matcher.test.ts @@ -138,6 +138,7 @@ describe('matchTrigger', () => { const baseTrigger: ResolvedTrigger = { id: 'test-trigger-id', + skillExecutionId: 'test-skill-execution-id', name: 'test-trigger', skill: 'test-skill', type: 'pull_request', diff --git a/packages/warden/src/triggers/matcher.ts b/packages/warden/src/triggers/matcher.ts index b2ee46bbc..c14db73a3 100644 --- a/packages/warden/src/triggers/matcher.ts +++ b/packages/warden/src/triggers/matcher.ts @@ -125,7 +125,7 @@ function matchPathFilters( return true; } -function matchPullRequestState(trigger: ResolvedTrigger, context: EventContext): boolean { +export function matchPullRequestState(trigger: ResolvedTrigger, context: EventContext): boolean { const labels = context.pullRequest?.labels ?? []; const labelMatches = trigger.labels !== undefined && diff --git a/packages/warden/src/types/index.ts b/packages/warden/src/types/index.ts index 828cad552..7e04270ad 100644 --- a/packages/warden/src/types/index.ts +++ b/packages/warden/src/types/index.ts @@ -125,7 +125,15 @@ export const FindingSchema = z.object({ sourceSnippet: SourceSnippetSchema.optional(), elapsedMs: z.number().nonnegative().optional(), }); -export type Finding = z.infer; +/** + * `reportedId` is intentionally not part of `FindingSchema`: that schema also + * validates raw model output in `sdk/extract.ts` and `sdk/verify.ts`, and this + * field must only ever be set by Warden's own dedupe/recenter logic in + * `poster.ts` — never claimed by a model. Widening the type here (instead of + * adding it to the schema) means any `FindingSchema`-validated parse strips a + * model-supplied `reportedId` rather than trusting it. + */ +export type Finding = z.infer & { reportedId?: string }; /** * Get the effective line number for a finding (endLine if present, otherwise startLine). @@ -360,7 +368,7 @@ export const SkillReportSchema = z.object({ /** Runtime backend used for this skill's analysis. */ runtime: z.string().optional(), }); -export type SkillReport = z.infer; +export type SkillReport = Omit, 'findings'> & { findings: Finding[] }; // GitHub event types export const GitHubEventTypeSchema = z.enum([ diff --git a/packages/warden/src/utils/fs.test.ts b/packages/warden/src/utils/fs.test.ts new file mode 100644 index 000000000..a400db7c5 --- /dev/null +++ b/packages/warden/src/utils/fs.test.ts @@ -0,0 +1,63 @@ +import { existsSync, mkdirSync, readdirSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; +import type * as NodeFs from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { writeFileAtomic } from './fs.js'; + +vi.mock('node:fs', async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, writeFileSync: vi.fn(actual.writeFileSync) }; +}); + +describe('writeFileAtomic', () => { + let tempDir: string; + + beforeEach(() => { + tempDir = join(tmpdir(), `warden-fs-atomic-${Date.now()}-${Math.random().toString(36).slice(2)}`); + mkdirSync(tempDir, { recursive: true }); + }); + + afterEach(() => { + rmSync(tempDir, { recursive: true, force: true }); + }); + + it('writes the full content in one shot', () => { + const target = join(tempDir, 'out.json'); + writeFileAtomic(target, '{"a":1}'); + expect(readFileSync(target, 'utf-8')).toBe('{"a":1}'); + }); + + it('creates missing parent directories', () => { + const target = join(tempDir, 'nested', 'deeper', 'out.json'); + writeFileAtomic(target, 'content'); + expect(readFileSync(target, 'utf-8')).toBe('content'); + }); + + it('overwrites an existing file', () => { + const target = join(tempDir, 'out.json'); + writeFileAtomic(target, 'first'); + writeFileAtomic(target, 'second'); + expect(readFileSync(target, 'utf-8')).toBe('second'); + }); + + it('leaves no orphaned temp file after a failed write', () => { + const target = join(tempDir, 'out.json'); + vi.mocked(writeFileSync).mockImplementationOnce(() => { + throw new Error('disk full'); + }); + + expect(() => writeFileAtomic(target, 'content')).toThrow('disk full'); + expect(existsSync(target)).toBe(false); + expect(readdirSync(tempDir)).toEqual([]); + }); + + it('uses distinct temp file names across concurrent calls to different targets', () => { + const targetA = join(tempDir, 'a.json'); + const targetB = join(tempDir, 'b.json'); + writeFileAtomic(targetA, 'a'); + writeFileAtomic(targetB, 'b'); + expect(readFileSync(targetA, 'utf-8')).toBe('a'); + expect(readFileSync(targetB, 'utf-8')).toBe('b'); + }); +}); diff --git a/packages/warden/src/utils/fs.ts b/packages/warden/src/utils/fs.ts new file mode 100644 index 000000000..1dec2ccad --- /dev/null +++ b/packages/warden/src/utils/fs.ts @@ -0,0 +1,25 @@ +import { mkdirSync, renameSync, unlinkSync, writeFileSync } from 'node:fs'; +import { dirname, join } from 'node:path'; + +/** + * Write a file atomically: content lands at `path` all-or-nothing, so + * concurrent or interrupted readers never observe a partially written file. + * Writes to a uniquely named temp file in the same directory, then renames + * it into place (rename is atomic on the same filesystem). + */ +export function writeFileAtomic(path: string, content: string): void { + mkdirSync(dirname(path), { recursive: true }); + const tmpPath = join(dirname(path), `.${process.pid}.${Date.now()}.${Math.random().toString(36).slice(2)}.tmp`); + + try { + writeFileSync(tmpPath, content); + renameSync(tmpPath, path); + } catch (error) { + try { + unlinkSync(tmpPath); + } catch { + // Temp file may not have been created yet; nothing to clean up. + } + throw error; + } +} diff --git a/packages/warden/src/utils/version.test.ts b/packages/warden/src/utils/version.test.ts new file mode 100644 index 000000000..8da2c442f --- /dev/null +++ b/packages/warden/src/utils/version.test.ts @@ -0,0 +1,89 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import type * as Fs from 'node:fs'; + +vi.mock('node:fs', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + readFileSync: vi.fn(actual.readFileSync), + }; +}); + +import { readFileSync } from 'node:fs'; + +const mockReadFileSync = vi.mocked(readFileSync); + +describe('getVersion', () => { + const previousActionPath = process.env['GITHUB_ACTION_PATH']; + + beforeEach(() => { + vi.resetModules(); + mockReadFileSync.mockReset(); + }); + + afterEach(() => { + if (previousActionPath === undefined) { + delete process.env['GITHUB_ACTION_PATH']; + } else { + process.env['GITHUB_ACTION_PATH'] = previousActionPath; + } + }); + + it("prefers GITHUB_ACTION_PATH's packages/warden/package.json (correct under ncc's flattened dist/action layout)", async () => { + process.env['GITHUB_ACTION_PATH'] = '/checked-out-action'; + mockReadFileSync.mockImplementation((path) => { + if (String(path) === '/checked-out-action/packages/warden/package.json') { + return JSON.stringify({ version: '1.2.3' }); + } + throw new Error(`unexpected path: ${path}`); + }); + + const { getVersion } = await import('./version.js'); + expect(getVersion()).toBe('1.2.3'); + }); + + it('falls back to the source-relative package.json when GITHUB_ACTION_PATH is unset', async () => { + delete process.env['GITHUB_ACTION_PATH']; + mockReadFileSync.mockImplementation(() => JSON.stringify({ version: '4.5.6' })); + + const { getVersion } = await import('./version.js'); + expect(getVersion()).toBe('4.5.6'); + }); + + it("falls back to the source-relative package.json when GITHUB_ACTION_PATH's package.json has no version field", async () => { + // Regression: this is exactly the monorepo-root package.json case — it + // exists and parses, it just has no `version` (it's `private: true`) — + // must not be treated as a successful resolution. + process.env['GITHUB_ACTION_PATH'] = '/checked-out-action'; + mockReadFileSync.mockImplementation((path) => { + if (String(path) === '/checked-out-action/packages/warden/package.json') { + return JSON.stringify({ name: 'warden-monorepo', private: true }); + } + return JSON.stringify({ version: '7.8.9' }); + }); + + const { getVersion } = await import('./version.js'); + expect(getVersion()).toBe('7.8.9'); + }); + + it('returns a sentinel instead of throwing when no package.json resolves at all', async () => { + delete process.env['GITHUB_ACTION_PATH']; + mockReadFileSync.mockImplementation(() => { + throw new Error('ENOENT'); + }); + + const { getVersion } = await import('./version.js'); + expect(getVersion()).toBe('0.0.0-unknown'); + }); + + it('caches the resolved version across calls', async () => { + delete process.env['GITHUB_ACTION_PATH']; + mockReadFileSync.mockImplementation(() => JSON.stringify({ version: '1.0.0' })); + + const { getVersion } = await import('./version.js'); + expect(getVersion()).toBe('1.0.0'); + expect(mockReadFileSync).toHaveBeenCalledTimes(1); + expect(getVersion()).toBe('1.0.0'); + expect(mockReadFileSync).toHaveBeenCalledTimes(1); + }); +}); diff --git a/packages/warden/src/utils/version.ts b/packages/warden/src/utils/version.ts index b9bdfc23a..2c8e0b2db 100644 --- a/packages/warden/src/utils/version.ts +++ b/packages/warden/src/utils/version.ts @@ -4,11 +4,35 @@ import { fileURLToPath } from 'node:url'; let cachedVersion: string | undefined; +function readPackageVersion(packageJsonPath: string): string | undefined { + try { + const pkg = JSON.parse(readFileSync(packageJsonPath, 'utf-8')) as { version?: string }; + return pkg.version; + } catch { + return undefined; + } +} + export function getVersion(): string { if (cachedVersion) return cachedVersion; + + // GITHUB_ACTION_PATH (set by GitHub Actions for every action) is this + // repo's own checked-out root, independent of how the running code is laid + // out — unlike an import.meta.url-relative path, which is only two levels + // above packages/warden/package.json when running from TypeScript source. + // ncc bundling flattens dist/action/index.js, so that same relative depth + // lands on the monorepo root's package.json instead, which has no version + // field (it's `private: true`) — silently returning undefined here would + // break any real Action run the moment a schema requires this as a string. + const actionPath = process.env['GITHUB_ACTION_PATH']; + const versionFromActionPath = actionPath && readPackageVersion(join(actionPath, 'packages/warden/package.json')); + if (versionFromActionPath) { + cachedVersion = versionFromActionPath; + return cachedVersion; + } + const __dirname = dirname(fileURLToPath(import.meta.url)); - const pkg = JSON.parse(readFileSync(join(__dirname, '..', '..', 'package.json'), 'utf-8')) as { version: string }; - cachedVersion = pkg.version; + cachedVersion = readPackageVersion(join(__dirname, '..', '..', 'package.json')) ?? '0.0.0-unknown'; return cachedVersion; }