Extend v1 findings output with provenance, corroboration, and run metadata - #460
Extend v1 findings output with provenance, corroboration, and run metadata#460CalebKAston wants to merge 2 commits into
Conversation
…adata Extends the GitHub Action's v1 warden-findings.json export with data an external consumer needs to build real observability on top of Warden runs, without breaking existing consumers: version stays '1', every new field is additive and optional. - harness (name/version/actionRef) — which build of Warden produced this run. - resolvedDefaults — the action-level fail-on/report-on/etc. this run resolved to. - skippedTriggers[] — configured triggers that didn't fire this run, with a structured reason (path_filter, draft_state, label_mismatch, no_event_match, error, pending). - Per-skill skillExecutionId/triggerId/triggerName — a stable join key across live snapshots, the final write, and (in split analyze/report mode) the replay artifact. - Per-finding contentHash, reportedId (set once cross-run dedupe recenters a finding onto an already-posted comment's id), reportedBy[] (which skills independently flagged this finding), and provenance (verification/merge history) plus a top-level discardedFindings[] for candidates that were rejected, merged away, or deduped before ever reaching findings[]. - Live incremental writes: the findings file is rewritten after each trigger settles (no .done marker, no findings-file action output — those stay reserved for the run's one true final write), so an external follower can observe progress instead of only the terminal state. Implementation notes: - skillExecutionId is a short hash of the existing per-trigger replay identity (config/loader.ts), reused everywhere a stable join key is needed. - provenance.ts turns captured FindingProcessingEvents (verification/merge/ dedupe stages the SDK already emits via onFindingProcessing) into per-finding provenance and the discardedFindings list; this now round-trips through the analyze/report replay artifact so report mode's export doesn't lose it. - The report-mode replay's legacy name+skill fallback join (for artifacts predating triggerId) now fails loudly instead of silently guessing when 2+ current triggers share a name and skill. - getVersion() now prefers GITHUB_ACTION_PATH to locate package.json, correct regardless of whether the running code is TypeScript source or ncc's flattened dist/action bundle — the same relative-path assumption that works from source lands on the wrong package.json once bundled, which would otherwise break every real Action run given harness.version is a required field. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 02256da. Configure here.
runAttempt never fell back to GITHUB_RUN_ATTEMPT (unlike runId), so production findings exports never carried it. The PR path also never threaded a trigger's auxiliaryModel/synthesisModel onto TriggerResult, so those fields stayed unset outside schedule mode. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
agree def easier to just extend the existing format. i will try to give this a thorough passthrough and get this merged asap |
dcramer
left a comment
dcramer
left a comment
There was a problem hiding this comment.
Intent-focused review of the findings artifact and replay contracts. These comments are limited to behavior introduced or materially affected by this patch.
| * `output.ts` looks it up by) stay in sync. | ||
| */ | ||
| export function buildProvenanceAndDiscarded(executions: FindingExecutionEvents[]): ProvenanceAndDiscarded { | ||
| const provenanceByFindingId = new Map<string, FindingProvenance>(); |
There was a problem hiding this comment.
Scope provenance to the skill execution
Same-run dedupe intentionally recenters a later skill finding onto an earlier finding ID, so two skills[] rows can share an ID. This run-global map then overwrites verification provenance and combines merge provenance in execution order, and both rows read the same result. Since provenance is nested under each skill execution, key it by (skillExecutionId, findingId) or build one map per execution. A two-skill recentering regression test would capture the intended behavior.
| export const FindingSchema = 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(), |
There was a problem hiding this comment.
Keep reportedId out of model-facing schemas
The field is documented as Warden-owned metadata set only after dedupe/recentering, but putting it on FindingSchema also accepts it from extraction and verification model output. validateFindings() replaces only id, and verification revisions preserve other parsed fields, so a model can claim continuity with a comment that was never matched. Please omit this field from extraction and verification schemas, or keep it on a reporting-specific finding type.
| const { provenanceByFindingId, discarded } = buildProvenanceAndDiscarded( | ||
| (options.skillExecutions ?? []).map((meta): FindingExecutionEvents => ({ | ||
| skillExecutionId: meta.skillExecutionId, | ||
| model: meta.report.model, |
There was a problem hiding this comment.
Attribute provenance to the model that performed each stage
This passes the primary analysis model for every provenance event. Verification runs on auxiliaryModel and merges run on synthesisModel, so the exported verification/merge model values are false whenever the lanes differ. Carry separate origin, verification, and merge model identities into the reducer and test with three distinct models.
| const allFindings = reports.flatMap((r) => r.findings); | ||
| const metaByReport = new Map((options.skillExecutions ?? []).map((meta) => [meta.report, meta])); | ||
|
|
||
| const dedupeByLocationHashKey = new Map( |
There was a problem hiding this comment.
Join corroboration through the dedupe relationship
This run-global location/content map drops the observing execution and existingFindingId. In a same-run A-then-B dedupe, B lists A but A never lists B; semantic matches can also have different content hashes, so the survivor cannot be found through this key. Build attribution from the observation skill plus existingSkills, joined to the survivor by existingFindingId/execution identity, so every exported copy of the logical finding has consistent reportedBy.
| }), | ||
| }); | ||
| completedSoFar.push(result); | ||
| options.onTriggerComplete?.([...completedSoFar]); |
There was a problem hiding this comment.
Account for triggers left queued by a circuit abort
runPool stops dispatching when the shared circuit aborts and returns only started results, so completedSoFar and the final export omit queued matched triggers entirely. That conflicts with the new skippedTriggers contract, and the existing all-failed check can also see fewer errors than configured triggers and allow a zero-success run to succeed. Synthesize aborted results for every undispatched trigger and base all-failed handling on successful executions.
| skippedTriggers.push({ skillName: resolved.skill, triggerId: resolved.id, triggerName: resolved.name, reason: 'error' }); | ||
| console.error(`::warning::Trigger ${resolved.name} failed: ${error}`); | ||
| logGroupEnd(); | ||
| writeLiveSnapshot(triggerIndex); |
There was a problem hiding this comment.
Finalize the artifact when every schedule trigger fails
This live snapshot becomes the last write on an all-failed run: handleTriggerErrors() throws at line 331 before the final writer creates the .done marker and findings-file output. Consumers then see a permanently in-progress artifact after the action has terminated. Perform the final write/completion signaling before propagating the all-failed error, and cover that ordering in the schedule test.
| console.log(`Issue URL: ${issueResult.issueUrl}`); | ||
| } | ||
|
|
||
| skillExecutions.push({ |
There was a problem hiding this comment.
Record execution metadata before the fallible issue write
allReports receives the successful analysis before createOrUpdateIssue(), but this metadata is pushed only afterward. If the GitHub write throws, the final artifact retains the findings while losing their execution ID, model lanes, and processing provenance. Push the base execution metadata immediately after runSkill() succeeds, then enrich it with issue fields after publication.
| const maxFindings = trigger.maxFindings ?? inputs.maxFindings; | ||
| const baseResult = { | ||
| triggerId: trigger.id, | ||
| skillExecutionId: trigger.skillExecutionId, |
There was a problem hiding this comment.
Round-trip the analysis model lanes through report replay
Analyze mode exports auxiliaryModel and synthesisModel, but replay results do not persist them and this reconstructed result never sets them. The final report-mode artifact therefore drops both fields. Because report-step config may have changed and the fields describe the analysis that actually ran, persist both lanes in triggerResults and restore them here; add an analyze-to-report regression test.
| }); | ||
| }); | ||
|
|
||
| it('produces the exact pre-existing shape when none of the new inputs are available', () => { |
There was a problem hiding this comment.
Isolate GITHUB_RUN_ATTEMPT in this regression test
The production fallback is intentional, but GitHub Actions sets this variable, so this test expects undefined while receiving the ambient attempt number. The required build is currently failing at this assertion, and GITHUB_RUN_ATTEMPT=1 reproduces it locally. Explicitly clear/stub the variable for this test or pass a deterministic runAttempt.
|
I can take a stab at cleaning this up but some of these might be pre-existing. Haven't had enough time to go through it fully yet this was just a first pass. |
|
I'll happily address the outlined items. |
2 participants
Summary
The GitHub Action's
warden-findings.jsonexport (v1) is the durable artifact external consumers build on (dashboards, telemetry pipelines, alerting). Right now it's missing data those consumers need to reason about why a finding is (or isn't) in the file, and which run/build produced it.This adds that data additively —
versionstays'1', every new field is optional, so existing consumers see zero difference unless they read the new fields:harness(name/version/actionRef) — which build of Warden produced this run.resolvedDefaults— the action-levelfailOn/reportOn/etc. this run resolved to.skippedTriggers[]— configured triggers that didn't fire this run, with a structured reason (path_filter,draft_state,label_mismatch,no_event_match,error,pending).skillExecutionId/triggerId/triggerName— a stable join key across live snapshots, the final write, and (in splitanalyze/reportmode) the replay artifact.contentHash,reportedId(set once cross-run dedupe recenters a finding onto an already-posted comment's id),reportedBy[](which skills independently flagged this finding), andprovenance(verification/merge history), plus a top-leveldiscardedFindings[]for candidates that were rejected, merged away, or deduped before ever reachingfindings[]..donemarker, nofindings-fileaction output — those stay reserved for the run's one true final write), so an external follower can observe progress instead of only the terminal state.New fields at a glance
All fields below are new and optional. Grouped by where they live in the JSON structure.
Top level
runAttemptstringharness{ name, version, actionRef? }resolvedDefaults{ failOn?, reportOn?, failCheck?, requestChanges?, maxFindings? }skippedTriggers[]{ skillName, triggerId?, triggerName?, reason }[]path_filter,draft_state,label_mismatch,no_event_match,error,pending).discardedFindings[]{ stage, severity, title, location?, survivorFindingId?, ... }[]findings[]— rejected by verification, absorbed by a merge, or dropped as a duplicate.summary.totalSkillExecutionsnumbertotalSkillstoday; kept distinct in case grouping ever changes.summary.byOutcome{ posted, deduped, skipped, resolved, failed }findingObservationsby outcome.Per skill (
skills[])skillExecutionIdstringtriggerResults[]/replay entry.triggerId/triggerNamestringauxiliaryModel/synthesisModelstringfindingsBySeverity{ high, medium, low }checkRunUrl/checkRunIdstring/numberreviewEvent'APPROVE' | 'REQUEST_CHANGES' | 'COMMENT'checkConclusion'success' | 'failure' | 'neutral' | 'cancelled'issueNumber/issueUrlnumber/stringPer finding (
skills[].findings[])contentHashstringoutput/dedup.tsalready uses for hash-based dedupe.reportedIdstringidonce cross-run dedupe recenters this finding onto an already-posted comment's id.verificationstringreportedBy[]{ skillExecutionId?, skillName, role, matchType? }[]role: 'primary', others matched via dedupe as'corroborating'.provenance{ originSkillExecutionId?, originModel?, verification?, merge? }Implementation notes
skillExecutionIdis a short hash of the existing per-trigger replay identity (config/loader.ts), reused everywhere a stable join key is needed.provenance.tsturns capturedFindingProcessingEvents (verification/merge/dedupe stages the SDK already emits viaonFindingProcessing) into per-finding provenance and thediscardedFindingslist; this now round-trips through the analyze/report replay artifact so report mode's export doesn't lose it.triggerId) now fails loudly instead of silently guessing when 2+ current triggers share a name and skill.getVersion()now prefersGITHUB_ACTION_PATHto locatepackage.json, correct regardless of whether the running code is TypeScript source or ncc's flatteneddist/actionbundle — the same relative-path assumption that works from source lands on the wrongpackage.jsononce bundled, which would otherwise break every real Action run givenharness.versionis a required field. Verified this specific failure mode live (see below).Test plan
pnpm lint && pnpm build && pnpm test— all green (92 test files, 1801 passing, 4 pre-existing skips, 0 failures)provenance.test.ts,utils/fs.test.ts,utils/version.test.ts,output.test.ts,pr-workflow.test.ts,schedule.test.ts,poster.test.ts,base.test.ts,dedup.test.ts,extract.test.ts,executor.test.ts,matcher.test.tscovering the new fields, live-write cadence, skip-reason derivation, chained-merge attribution, and the report-mode ambiguous-fallback fixgetVersion()/ncc-bundling bug above, which no unit test could reach since it only manifests once code is actually running from the compileddist/actionbundle rather than TypeScript source🤖 Generated with Claude Code