Skip to content

feat(report): publish what the rules find today, measured by the CLI - #503

Merged
theCodeDrift merged 7 commits into
mainfrom
report-measured-findings
Oct 14, 2026
Merged

theCodeDrift merged 7 commits into
mainfrom
report-measured-findings

Conversation

@theCodeDrift

Copy link
Copy Markdown
Member

A published report never said what its rules find in the code today, though check measures exactly that. Every number on the page came from the agent's facts file, and fixedBasis: measured could be claimed with no method behind it. tskl.es has reshaped ReportV1 around measured findings, and today's nightly payload fails it (missing state, unknown fixed/origin/perMonth), so every publish is a 400.

What changes

  • The CLI counts violations itself. report runs the same scan as check --anonymous, which stays offline, and publishes per-rule violations/files plus findings: { violations, files }, where files counts each file once across rules. Neither the draft nor the facts file lets an agent set them.
  • All or nothing. If any of these holds, the report carries no counts at all and tskl.es falls back to another headline: a legacy rule, a local runtime rule, a scan failure, or a result under a rule id the report doesn't have (an sg rule whose body id: differs from its directory).
  • Schema alignment: rules carry state (written/candidate), origin → surfaced, fixed/fixedBasis removed, projection.perMonth no longer sent (tskl.es derives the rates). Vendored schema refreshed.
  • Summary leads with "Found in the code today: N problems in M files." and states review counts as raw totals over the window. It shows no monthly rate, so it cannot drift from the page's formula.
  • Recipes: one method for counting surfaced: read each comment instead of matching keywords, and bot and author comments count. The agent's own recap has to label "in the code today" apart from "raised in review over the window".
  • check's scan is extracted to rules/scan-project.ts; check's output and exit codes are unchanged (113 check tests pass against the rebuilt CLI).

Verified

  • pnpm typecheck and pnpm lint pass.
  • Built with build:next and run against a copy of the mixed-engines fixture with one file duplicated. The draft's findings (6 violations, 3 files) and per-rule counts match check --json --anonymous exactly.
  • The end-to-end publish test validates the sent payload, including findings, against the refreshed schema.
  • The OpenSpec change is archived in this PR. The archive dropped only the two scenarios of the intentionally removed "The CLI computes the projection" requirement; every rewritten requirement kept all its scenarios.

taskless report is unreleased, so this extends its pending streamlined-onboarding-report changeset instead of adding a second one.

@theCodeDrift

Copy link
Copy Markdown
Member Author

@claude /review

@github-actions

github-actions Bot commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @theCodeDrift's task in 4m 28s —— View job


Review complete

Review mode: incremental — read 0 prior review thread(s) before reviewing. .prior-review.json contained no reviews, review threads, or comments beyond the trigger itself, so this is the first full review of this PR; everything below is [New].

  • Read prior review data (none found)
  • Fetch PR diff and metadata
  • Review schema/type changes (report/types.ts, report-v1.schema.json, report-facts.ts)
  • Review rules/scan-project.ts extraction and check.ts changes
  • Review report/findings.ts (new)
  • Review report/assemble.ts and report/draft.ts
  • Review report/summary.ts
  • Review agent recipes (onboard.md, report.md)
  • Review tests
  • Review OpenSpec archive/spec changes
  • Post final review

Summary

This is a clean, well-scoped implementation. I read the full diff and the whole contents of the touched files (draft.ts, assemble.ts, findings.ts, check.ts, scan-project.ts, run-directory.ts, report-facts.ts, commands/report.ts), traced the all-or-nothing counting invariant end-to-end, and checked the OpenSpec archive against the project's own "a MODIFIED block replaces the whole requirement" hazard. I did not find any correctness, security, or test-coverage issues worth raising.

[New] Things I specifically verified rather than assumed:

  • All-or-nothing invariant holds by construction. countFindings (packages/cli/src/report/findings.ts) rejects on a runtime rule, a plan/dispatch failure, or an unattributed ruleId before computing any count, and measuredFindings/mergeRules (report/assemble.ts:69-124) re-checks facts.legacy.length > 0 at assembly time, since a legacy rule only shows up after merging facts. A local runtime rule is caught earlier, because buildDraft already failed to measure (it's one of the rules in the scan), so draft.findings is already undefined by the time assemble.ts runs. I traced all four rejection paths (legacy, local runtime, engine failure, unattributed id) against both the unit tests in report-findings.test.ts and the spec scenarios in openspec/specs/cli-report/spec.md, and they line up.
  • Schema consistency bounds hold mathematically, not just by test. tskl.es's stated invariants (findings.violations = Σ rule violations; findings.files between max(rule.files) and Σ rule.files) follow directly from findings.ts's use of a shared allFiles set unioned from per-rule sets — no separate enforcement needed, and none was added, which is correct.
  • The OpenSpec archive did not silently drop scenarios. This repo's CLAUDE.md flags "a MODIFIED block replaces the whole requirement" as a real, previously-hit failure mode. I diffed openspec/specs/cli-report/spec.md before/after: every requirement that was reworded kept every pre-existing scenario and only added new ones; the one requirement actually removed ("The CLI computes the projection") took its two scenarios with it and was replaced by "The payload carries projection inputs and no rates" with an equivalent scenario. This matches the PR body's claim exactly.
  • check's behavior is preserved under the scanProject extraction. run.close() now happens inside scanProject's own try/finally rather than check.ts's outer one, so with --preserve-logs the "run finished; directory preserved" log line and the stderr "Run directory kept" message are now written at slightly different points — but the directory itself is never removed when preserved either way, --json output is byte-for-byte unaffected, and this reordering is explicitly called out and justified in the change's design.md. Not a regression.
  • No new network or stdout leakage from the added scan. Neither scanProject's dependencies (plan-check.ts, dispatch.ts) nor findings.ts write to the console directly, so buildDraft's added scan can't corrupt taskless report's single-JSON-object-on-stdout contract.
  • Facts schema correctly refuses every removed/CLI-owned field. report-facts.ts's .strict() objects reject fixed, fixedBasis, origin, violations, files, findings, state, and perMonth, matching every REPORT_INVALID scenario in the spec and exercised by the corresponding it.each in report.test.ts.

I did not run the project's build/lint/test suite myself (per review instructions), and did not check CI status — that's CI's job to report on the PR.

@theCodeDrift
theCodeDrift merged commit 6284621 into main Oct 14, 2026
4 checks passed
@theCodeDrift
theCodeDrift deleted the report-measured-findings branch October 14, 2026 23:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant