Extend v1 findings output with schema-v2 data instead of splitting files - #16
Closed
CalebKAston wants to merge 6 commits into
Closed
Extend v1 findings output with schema-v2 data instead of splitting files#16CalebKAston wants to merge 6 commits into
CalebKAston wants to merge 6 commits into
Conversation
Adds the data captured by upstream's unmerged output-schema-v2 effort (provenance, cross-skill attribution, contentHash, harness/resolvedDefaults, structured skip reasons, live incremental writes) directly onto the existing single warden-findings.json — version stays '1', every new field is optional, so existing external consumers see no difference. Avoids the two-file split and the ~7400-line branch history that made the original attempt hard to review. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
An adversarial /fight-me review of PR #16 found: - reportedBy corroboration was keyed only by content hash (title+description), so two unrelated findings sharing generic wording at different locations would collide and one would inherit the other's cross-skill attribution. Fixed by keying on location+hash instead, reusing the same pattern output/dedup.ts already uses for exact-match dedup (new generateLocationHashKey, shared rather than reinvented). Proven with a regression test before and after the fix. - The resolvedDefaults object literal was copy-pasted identically across four call sites in this same PR. Extracted buildResolvedDefaults(). - skillExecutionId threading in poster.ts was asserted for 2 of 6 observation push sites; extended coverage to all of them, which also surfaced a genuine gap (the 'failed' outcome site never threaded skillExecutionId at all) — fixed alongside the tests. - Investigated narrowing checkConclusion's 'cancelled' arm (never produced by its sole producer, determineConclusion) but the only way to enforce it at the type level was a cast — worse than the thing it fixed. Left the type as-is with a comment explaining why. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ne window A fresh /fight-me pass (run in a subagent, no prior context) over the already-fixed PR found four more issues: - checkConclusion was computed from raw findings, but the check run actually posted to GitHub (buildSkillCheckPayload) filters by minConfidence first. A high-severity, low-confidence finding could show checkConclusion: 'failure' in the export while GitHub's own check shows success. Fixed to filter identically before computing the conclusion. - resolvedDefaults declared 11 fields but could only ever populate 5 — model/auxiliaryModel/synthesisModel/runtime/verifyFindings/minConfidence are resolved per-trigger in this repo's config model, not at the action level, so there was never a real value to report. Narrowed the schema to the 5 fields that are actually action-level. - The .done stale-marker cleanup only ran lazily inside the first live write, which itself only fires after the first trigger completes — so a persistent/self-hosted runner had a window, between run start and that first completion, where a previous run's .done could still make a brand-new run look finished. Added clearStaleDoneMarker(), called once up front before any trigger runs. - deriveSkippedReason had two dead branches (schedule/local trigger types, unreachable given its only caller pre-filters to pull_request/wildcard) and zero test coverage of its real branches. Cut the dead branches and added coverage for two of the reachable ones using existing fixtures. That test-writing pass then surfaced a fifth issue: three early-return branches (skipCoreCheck and no-triggers-matched, across run/analyze/report modes) never threaded skippedTriggers/resolvedDefaults/actionRef into their findings-output writes at all. Fixed all three. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…toms Several findings from the lane-based review shared underlying causes rather than being one-off bugs, so this fixes each cause once instead of patching every symptom site individually: - clearStaleDoneMarker took EventContext, forcing it to run after fallible config load in both workflows; it now takes repoPath directly and runs first, in both pr-workflow.ts and schedule.ts. - actionRef/resolvedDefaults/skippedTriggers were hand-duplicated at ~13 write call sites, which is exactly how schedule.ts's two early returns silently shipped without actionRef for a full round; extracted into buildBaseOutputOptions so a future field only needs one edit. - Errored triggers had no representation in the new skillExecutions/ skippedTriggers bookkeeping in either workflow (toSkillExecutions can't include them since they have no report); they're now surfaced via a new 'error' skippedTriggers reason in both files. - reviewEvent mirrored renderResult's pre-posting intent instead of what actually posted (the same disease as the already-fixed checkConclusion bug); poster.ts now stamps reviewEventPosted only when posting actually succeeds, and report-mode's checkRunId is no longer dropped. - The feedback gate blocking a post after dedup/consolidation already ran left those findings out of findingObservations entirely; now recorded as skipped/review_not_posted, the reason this schema value already existed for. - findReplacementForAbsorbed re-derived a merge's survivor by location matching, which silently failed when the absorbed finding shared the winner's own primary location (not just its additionalLocations); applyMergeGroups now returns the winner mapping directly. - Cross-run dedupe's id-recentering updated a finding's id but not its already-captured FindingProcessingEvents, so provenance.ts's id-keyed lookup could miss after a recenter; events are now remapped alongside it. - findingProcessingEvents never round-tripped through the analyze/report replay artifact, so report mode's export silently lost provenance and discardedFindings; threaded through TriggerRunResultSchema. - buildReportModeResults' legacy fallback join could bind a report to the wrong trigger's policy when 2+ current triggers share a name+skill; it now fails loudly instead of guessing. - buildProvenanceAndDiscarded only modeled 2 of 3 removal stages (verification, merge); dedupe/dropped events are now covered too. - Removed existingSkillExecutionId/githubCommentId/githubCommentUrl: schema fields with no real producer, same shape as the resolvedDefaults dead fields already trimmed in a prior round. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
A final fight-me pass (fresh subagent, no memory of prior rounds) found a real bug in the previous root-cause fix and one lower-stakes gap: - findReplacementForAbsorbed only walked one hop of absorbedToWinner, so a chained/overlapping merge (finding absorbed into a winner that is itself later absorbed into a further winner) resolved to the intermediate winner — which no longer exists in the output — instead of the true final survivor. Proven by executing applyMergeGroups directly with overlapping groups. Now walks the chain to the end; a finding, once absorbed, is excluded from every later group, so this can't cycle. - harness was gated on actionRef being set, dropping the whole block (including the always-available version) on any run without one. Now always emitted; actionRef inside it stays optional. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ayout
Live-validated this PR end-to-end via a temporary build branch + a
temporary babylist/web test PR wired through babylist/.github, per the
plan discussed with the user. It surfaced a real production-breaking bug:
getVersion() located packages/warden/package.json via a path relative to
its own source file's __dirname — correct in the TypeScript source tree
(src/utils/ -> two levels up -> packages/warden/), but wrong once ncc
bundles it flat into dist/action/index.js, where the same relative depth
lands on the monorepo root's package.json instead. That file has no
version field (it's private: true), so getVersion() silently returned
undefined there.
This was previously harmless (only used for Sentry release tagging, a
non-fatal string interpolation), but this PR's new `harness.version`
export field is a required z.string() — so every real Action run started
throwing inside buildFindingsOutput, which meant writeFindingsOutput never
set the findings-file output, which in turn made the reusable workflow's
upload-artifact step fail outright ("Input required and not supplied:
path"), failing the whole job.
Fixed by preferring GITHUB_ACTION_PATH (set for every GitHub Action,
independent of how the running code is laid out) to locate
packages/warden/package.json directly, falling back to the existing
source-relative lookup for CLI/local usage, and never returning undefined
(sentinel '0.0.0-unknown' if neither resolves).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
CalebKAston
added a commit
that referenced
this pull request
Jul 29, 2026
Collaborator
Author
|
Retargeted as an upstream contribution: this feature turned out to be entangled with several babylist-fork-only additions (the postChecks toggle, verifier-rejection tracking, response-model isolation) that live in the same files this PR touches. Rather than dragging those along, I re-implemented the same feature cleanly against getsentry/warden's current main: getsentry#460 Closing this one — once getsentry#460 merges upstream, we'll sync this fork with upstream main to pick it up. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Upstream
getsentry/wardenhas an open, unmerged PR (caston/warden/upstream-output-schema-v2) adding an opt-in "schema v2" GitHub Action findings export that splits the singlewarden-findings.jsoninto two files (warden-metadata.json+warden-findings-v2.json) behind a newoutput-schema-versiontoggle. That branch has grown to 77 files / ~7400 lines through many "fix: Nth Bugbot round" commits and is hard to review. dcramer noted the file-split is no longer a hard requirement.This PR starts fresh off
mainand captures the same underlying data — cross-skill attribution, per-finding provenance (verification/merge trail), a stablecontentHash, harness/resolved-config metadata, structured skip reasons, live incremental writes — without splitting files.versionstays'1', every new field is additive and optional, so existing external consumers ofwarden-findings.jsonsee zero difference unless they read the new fields.packages/warden/src/action/reporting/output.ts—FindingsOutputSchemagainsharness,resolvedDefaults,skippedTriggers[], per-skill execution identity/posting fields, and per-findingcontentHash/reportedId/reportedBy[]/provenance;discardedFindings[]andsummary.totalSkillExecutions/byOutcomeare new too.packages/warden/src/action/reporting/provenance.ts(new) — pure function matching captured verification/merge/dedupe events to findings, producingprovenance+discardedFindings.packages/warden/src/action/workflow/{schedule,pr-workflow}.ts+base.ts— a newwriteFindingsOutputLivewrites an in-progress snapshot of the findings file after each trigger settles (no.donemarker, nofindings-fileaction output); the real final write now also writes a.donesidecar via a newwriteFileAtomic(utils/fs.ts).buildBaseOutputOptionscentralizes theactionRef/resolvedDefaults/skippedTriggersfields shared across every write call site in both workflows.config/loader.ts/action/triggers/executor.ts—skillExecutionId(a short hash of the existing trigger identity) threads through as a stable join key;findingProcessingEventsare captured for provenance without touching CLI debug-printing behavior, and now round-trip through the analyze→report replay artifact.action.yml/inputs.ts— new optionalaction-refinput, surfaced asharness.actionRef.Explicitly out of scope: the
output-schema-versiontoggle, the two-file split, the attribution-footer parsing rewrite inoutput/dedup.ts, and the reference branch's full cross-run corroboration engine (this PR'sreportedBy[]uses the simpler, already-availableExistingComment.skillsdata instead).Review history
This went through several adversarial passes before and after opening, each catching real gaps the prior one missed:
reportedIdhad no producer (now stamped alongsideidat both dedupe-recenter mutation sites); the splitmode: analyze→mode: reportpath wasn't threading the new metadata into its final write.checkConclusionreflecting pre-posting render intent instead of what actually posted, and several dead schema fields with no real producer.pr-workflow.ts/schedule.ts, errored triggers invisible in the new bookkeeping,reviewEvent/checkRunIdwith the same "reflects intent, not fact" disease ascheckConclusion, identity re-derived by location instead of threaded through the pipeline, and the analyze/report replay silently droppingfindingProcessingEvents. Each was fixed at its shared cause rather than patched per call site.harnesssilently going missing wheneveraction-refwasn't set, even thoughversionis always available. Both fixed.The one previously-documented limitation —
provenance's id-keyed lookup missing after cross-run dedupe recenters an id — is now closed:poster.ts's recenter remaps already-capturedfindingProcessingEventsin lockstep, so the lookup stays valid.Test plan
pnpm lint && pnpm build && pnpm test— all green (92 test files, 1855 passing, 4 pre-existing skips, 0 failures)provenance.test.ts,utils/fs.test.ts,output.test.ts,pr-workflow.test.ts,schedule.test.ts,poster.test.ts,base.test.ts,dedup.test.ts,extract.test.tscovering the new fields, live-write cadence, skip-reason derivation, the report-mode fix, chained-merge attribution, and the id-recenter provenance sync🤖 Generated with Claude Code