Repository navigation
feat(cli): check_report names the finding that stopped the check - #7305
Conversation
Fallow audit reportQuality gate failed Found 4 findings: Dead code 1, Duplication 3. Dead code (1)
Duplication (3)
Generated by fallow.
|
![heygengenesis[bot]](https://avatars.githubusercontent.com/in/2913576?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed at be6bacf. Requesting changes. There's one blocker.
Blocker: the Comments workflow fails on this head. Its "A package file's comment share may not rise" step flags both new doc comments:
packages/cli/src/telemetry/events.ts: 17.4% → 17.5% (thestopCodeJSDoc line)packages/cli/src/utils/checkPipeline.ts: 183 → 184 comment lines (theSTOP_CODESJSDoc line)
Remove both lines, or move what they say into a name or type. For example, type stopCode as a StopCode union exported next to STOP_CODES instead of string. That also makes the closed set a compile-time guarantee, not just a comment.
Not blocking (scope note): budget or deadline stops that happen in the crops or finding-frames phases (withFindingCrops / withOptionalFindingFrames → runtimeFailure) are added after buildReport has already fired check_report. Those stops send no stop_code, the same way ok already shows the state from before crops. That only affects --snapshots / --finding-frames runs. Consider mentioning it in the PR body so nobody reads the count as complete.
Everything else checks out. The selection logic matches every code runtimeFailure produces when no override is passed. Those three codes are created nowhere else in packages/cli/src. Only the code is sent, never the message or path. The tests cover both a set and an omitted stop_code. This is a feat PR, so the "fails without the fix:" line isn't required.
There was a problem hiding this comment.
Re-reviewed at 021c04e. Approving.
- The comments blocker is resolved.
stopCodeis now aStopCodeunion derived fromSTOP_CODES(as const), the two comment lines are gone, and theCommentsworkflow passes on this head. - The change from
be6bacf7is limited to the typing. The selection logic is unchanged: it still picks the first runtime finding whose code is in the set.events.tsimports fromcheckPipelinewithimport type, so there's no runtime cycle. - My earlier non-blocking note still applies: stops in the crops or finding-frames phases happen after
check_reportis sent, so they carry nostop_code.
The main CI, CodeQL and Windows workflows were still running when I reviewed. This approval doesn't stand in for them.
…view Adopts heygen-com#7299 (accented Latin letters draw in a bundled family at its bundled weight), heygen-com#7301 (installing Chrome on a busy Windows PC), heygen-com#7262 (cloud render logs a line a minute while a job holds its status), heygen-com#7265 (video starts a second early and catches up hidden) and heygen-com#7305 (check_report names the finding that stopped the check). Player packed budget takes upstream's +1655 from heygen-com#7265 since the player bundles core's media-steer. Defers heygen-com#7302; heygen-com#7261 and heygen-com#7263 do not apply. Watermark advances to 9e2510b. Co-authored-by: Miguel Ángel <miguel.sierra@heygen.com> Co-authored-by: WaterrrForever <miao.yang@heygen.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
What
check_reportnow carriesstop_code: the code of the finding that ended the check early, one ofmedia_copies_unfinished,check_budget_reachedorcheck_runtime_failure. It is left out when the check ran to the end.Why
The event had only counts per section, so a check that stopped on unfinished media copies or on its budget looked the same as any other failed check. Nobody could count how often a check stops, or why.
Related work
Follows the lint event's
code_counts, which already sends closed codes the same way.How
checkPipeline.tspicks the first runtime finding whose code is inSTOP_CODES(the codesruntimeFailuregives a stopping failure;StopCodeis the union of them, so the sent code is a compile-time closed set) and passes it asstopCode;trackCheckReportsends it asstop_codeonly when set. Only the code, never the finding's message or file. There is one caller oftrackCheckReportand no field allowlist or doc elsewhere in the repo (grep for the event's field names).A stop in the finding-crops or finding-frames phase (
--snapshots/--finding-framesruns) is added afterbuildReporthas already sentcheck_report, so it sends nostop_code, the same wayokshows the state from before the crops: the count is complete only for stops before them.Test plan
telemetry/events.test.ts"names the finding that stopped the check, and omits it when none did" andcommands/check.test.ts(the budget scenario assertsstopCode: "check_budget_reached"; an ordinary failing check has none): the first two fail without the change; both files pass 3 runs in a row (245 tests).check_budget_reachedis covered through the pipeline,media_copies_unfinishedonly as an accepted code.Size
About 57 changed lines (under the 100-line floor) on purpose: one new field on one event, set at its single call site, with its type and two tests. It does not bundle with other OSS work because this lane has none open that touches the check pipeline or its telemetry; folding it into an unrelated PR would make a one-field change harder to revert.