feat(ci): make the PR report say what each finding means and what to do by pauldambra · Pull Request #154 · PostHog/coherence

pauldambra · 2026-10-25T16:43:08Z

Why

The Coherence comment on posthog-js#5429 was hard to act on. It used terms like "traced control" and "adoption baseline" with no explanation. It did not say whether the PR caused the findings, or what to change.

What changed

Under the counts, one plain line: "Nothing to fix." or "These findings do not fail this check. Some can come from files this pull request did not change."
Each section with a finding now says what it means and how to fix it (spec problems, failing chokepoints, spec gaps, guard failures).
Spec gaps: plain words for what a gap is and for the baseline count. The entrance name is in bold, and the route has a label.
Two spec problem messages are clearer:
- an invariant bullet with no name shows the form to use
- a cited commit that no branch has says that a squash merge is the usual cause, and to cite the merge commit
The stats line is now "Spec size: ...". It no longer repeats the problem count.
Findings in files the PR changed get the mark changed in this pull request. A line under the counts says how many there are, or "None of these findings are in files this pull request changed.", or why Coherence could not tell. The base is --base <commit> when given. Otherwise, in a GitHub pull request run, it is origin/$GITHUB_BASE_REF. So adopters with a workflow already in place get the marks after a package upgrade, with no workflow edit.
Bumps the version to 1.10.0, so merging this PR publishes it.

New spec bullet in Lifecycle.spec.md, with a parameterized test and a witnessed refutation.

Testing

ci.test.ts (including a real-git --base test) and practice.test.ts pass. typecheck, lexicon:check and spec:check are clean (0 problems).
The full unit suite has 22 failures in this sandbox (no rust-analyzer, gopls or pytest, and it runs as root). The same 22 fail on main, so none come from this change.

Created with PostHog Desktop

🤖 Generated with Claude Code

The CI report now says, under its counts, whether there is anything to fix, and that findings do not fail the check and can come from files the pull request did not change. Each section with a finding says what it means and how to fix it, in plain words. The spec gap section names the adoption baseline in plain words, and the two least clear spec problems (an invariant bullet with no name, a cited commit no branch has) now say how to fix them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: 4189fcb8-6fd5-428d-8c5f-4314d28e4dac

posthog

PostHog Review · Standard · reviewed 2c1b5ff: 1 Should fix.

This is the only automatic review while this pull request is a draft. Comment @posthog review to review new changes sooner. Otherwise, a review of the new changes runs when you mark it ready for review.

See this review and its findings in PostHog Review. Comment @posthog review --deep for a Deep review. A Deep review takes longer and looks deeper than a Standard review.

…st changed ci report takes --base <commit>. With it, the report says how many findings are in files changed since that commit and marks each one, or says why it could not tell. The workflow ci install writes passes the pull request's base sha. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: 4189fcb8-6fd5-428d-8c5f-4314d28e4dac

posthog

PostHog Review · Standard · reviewed 60ed988: 2 Should fix.

…; release 1.10.0 Without --base, ci report on a GitHub pull request run compares against origin/$GITHUB_BASE_REF, so an adopter's existing workflow marks changed findings after a package upgrade alone. The generated workflow no longer passes --base. Bumps the version to 1.10.0 so merging releases it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: 4189fcb8-6fd5-428d-8c5f-4314d28e4dac

posthog

PostHog Review · Standard · reviewed 0684877: 1 Should fix.

This is review round 3 on this pull request. When findings keep coming back in the same code, the approach is usually the problem. Before the next fix, check whether a different design would remove the whole class of findings.

…he release steps The declared, python, rust and typescript plugins ship at the package's version, and bundled.e2e.ts holds them equal. The README's release steps named only the mod's plugin.json. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: 4189fcb8-6fd5-428d-8c5f-4314d28e4dac

pauldambra

Note

🤖 Automated comment by QA Swarm — not written by a human

QA Swarm review complete. See inline comments.

pauldambra · 2026-10-25T18:39:40Z

Note

🤖 Automated comment by QA Swarm — not written by a human

Multi-perspective review: router (cheap-first pass) + delegated lenses (qa-team, paul-reviewer, xp-reviewer, security-audit, engineering-systems-thinking as warranted)

Verdict: ⚠️ REQUEST CHANGES _{(round 1 @ 68cdd1a)}

There are 2 HIGH findings. Both are small to fix: the "None of these findings" line claims too much when one project's diff could not be read, and the gap-closing advice repeats CLOSE_WAYS. Security review found no problem.

Key findings

🟠 HIGH: findingsLead says "None of these findings are in files this pull request changed" when another project's changed files could not be read (ci-report.ts:181).
🟠 HIGH: the gap-closing sentence repeats CLOSE_WAYS from gaps.ts, so the two can drift apart (ci-report.ts:153).
🟡 MEDIUM: the guard failures text still uses defect and class without explaining them, and gives no note that guard failures never get the mark (ci-report.ts:159).
🟢 LOW: the chokepoint explanation uses the word to explain itself (ci-report.ts:133).
⚪ NIT: a double blank line (ci-report.ts:101).

Convergence

The guard failures section (ci-report.ts:159): the router and xp each flagged it, for different reasons.

Reviewer summaries

Reviewer	Assessment
🧭 router	Danger MEDIUM: 549 lines, and a release path (version bump). Found 1 LOW and 1 NIT. Sent 3 reviews to Sonnet: security-audit, systems thinking and xp.
🔒 security-audit	Nothing found. The base ref goes to git as an argument array with no shell and always starts with `origin/`, and file names are only used in set lookups.
🧩 engineering-systems-thinking	Paths use the same root as the diff. One HIGH: a report that mixes read and unread projects claims too much.
🧪 xp	The plain-language goal is mostly met. One HIGH (duplicated close-ways list), one MEDIUM and one LOW on wording.

Automated by QA Swarm — not a human review

Fixes several bot and QA Swarm review findings on src/lifecycle/ci-report.ts (PR #154): - never says "Nothing to fix" when Coherence did not finish or a project's spec gaps were not read - never says "None of these findings are in files this pull request changed" when some projects' changed files could not be read - attributes a failing chokepoint using its recorded files, not only its bypasses - marks a registry problem when the pull request changed the file it is about - compares against the pull request's fixed base commit from GITHUB_EVENT_PATH on GitHub Actions, falling back to origin/$GITHUB_BASE_REF only when the event carries none, so a moving target branch no longer hides or fakes findings - reuses CLOSE_WAYS from gaps.ts instead of restating the three ways to close a gap - plainer guard-failure wording, a note that guard failures carry no changed-file mark, and a worked example for what a chokepoint failure means - removes a stray double blank line Updates the matching Lifecycle.spec.md bullet and adds test cases for each behavior. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: 4189fcb8-6fd5-428d-8c5f-4314d28e4dac

posthog

PostHog Review · Standard · reviewed 1697f9a: 2 Should fix.

This is review round 4 on this pull request. When findings keep coming back in the same code, the approach is usually the problem. Before the next fix, check whether a different design would remove the whole class of findings.

…on clean Reusing CLOSE_WAYS kept one source for the three closing moves but put spec jargon back in front of PR authors; a plain sentence now leads it. The spec bullet no longer uses a noun the lexicon does not know. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: 4189fcb8-6fd5-428d-8c5f-4314d28e4dac

One changedFiles helper reads the changed files of a project or the registry, in place of two copies of the same check. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: 4189fcb8-6fd5-428d-8c5f-4314d28e4dac

pauldambra

Note

🤖 Automated comment by QA Swarm — not written by a human

QA Swarm round 2 review complete. See inline comment.

…n the registry diff failed CLOSE_WAYS goes through cell() so GitHub does not strip <chokepoint> and <reason> as HTML. changedSince asks git for unquoted paths, so a non-ASCII file name matches its finding. A registry top whose changed files git could not list is named in the lead line, as a project's is. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: 4189fcb8-6fd5-428d-8c5f-4314d28e4dac

… not the process JsonRpcClient listened for errors on the child process but not on its stdin. When the server had already gone, the TypeScript adapter's exit notification hit EPIPE on a stream with no error listener, an uncaught exception that failed server.test.ts in CI at random. Exit, process errors and stdin errors now share one fail() path. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: 4189fcb8-6fd5-428d-8c5f-4314d28e4dac

posthog

PostHog Review · Standard · reviewed 60770e2: 1 Should fix.

This is review round 5 on this pull request. When findings keep coming back in the same code, the approach is usually the problem. Before the next fix, check whether a different design would remove the whole class of findings.

A pipe error marks the client not alive, and callers such as the Rust adapter's close() and the restart paths kill only a live client, so a server that closed its input but kept running was left behind. The client now kills the child when its input pipe fails. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: 4189fcb8-6fd5-428d-8c5f-4314d28e4dac

pauldambra self-assigned this Oct 25, 2026

posthog Bot reviewed Oct 25, 2026

View reviewed changes

Comment thread src/lifecycle/ci-report.ts Outdated

pauldambra marked this pull request as ready for review October 25, 2026 17:56

posthog Bot reviewed Oct 25, 2026

View reviewed changes

Comment thread src/lifecycle/ci-report.ts Outdated

Comment thread src/lifecycle/ci-report.ts

posthog Bot reviewed Oct 25, 2026

View reviewed changes

Comment thread src/lifecycle/ci-report.ts Outdated

pauldambra commented Oct 25, 2026

View reviewed changes

Comment thread src/lifecycle/ci-report.ts Outdated

Comment thread src/lifecycle/ci-report.ts Outdated

Comment thread src/lifecycle/ci-report.ts Outdated

Comment thread src/lifecycle/ci-report.ts

Comment thread src/lifecycle/ci-report.ts Outdated

posthog Bot reviewed Oct 25, 2026

View reviewed changes

Comment thread src/lifecycle/ci-report.ts

Comment thread src/lifecycle/ci-report.ts Outdated

pauldambra added 2 commits October 25, 2026 18:50

refactor: apply simplify pass

5207b59

One changedFiles helper reads the changed files of a project or the registry, in place of two copies of the same check. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: 4189fcb8-6fd5-428d-8c5f-4314d28e4dac

pauldambra commented Oct 25, 2026

View reviewed changes

Comment thread src/lifecycle/ci-report.ts

pauldambra added 2 commits October 25, 2026 18:56

posthog Bot reviewed Oct 25, 2026

View reviewed changes

Comment thread src/adapters/jsonrpc.ts Outdated

pauldambra merged commit 01235ab into main Oct 25, 2026
19 checks passed

pauldambra deleted the posthog/ci-report-plain-words branch October 25, 2026 19:42

pauldambra mentioned this pull request Oct 25, 2026

chore: upgrade @posthog/coherence to 1.10.1 PostHog/posthog-js#5432

Open

4 of 5 tasks

Conversation

pauldambra commented Oct 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why

What changed

Testing

Uh oh!

posthog Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

posthog Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

posthog Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pauldambra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pauldambra commented Oct 25, 2026

Verdict: ⚠️ REQUEST CHANGES (round 1 @ 68cdd1a)

Key findings

Convergence

Reviewer summaries

Uh oh!

posthog Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pauldambra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

posthog Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

pauldambra commented Oct 25, 2026 •

edited

Loading

Verdict: ⚠️ REQUEST CHANGES _{(round 1 @ 68cdd1a)}