Skip to content

Keep multi-line captures in their bullets, tolerate ghost reviewers, and share sensor context - #142

Merged
nnennandukwe merged 2 commits into
mainfrom
refactor/issue-133-adapters
Sep 25, 2026
Merged

nnennandukwe merged 2 commits into
mainfrom
refactor/issue-133-adapters

Conversation

@nnennandukwe

Copy link
Copy Markdown
Owner

Summary

Part of #133. Fixes three defects in the adapters and the Markdown renderer, and removes duplication in the sensor scripts and the renderer. It does not touch any file that another open #133 PR changes.

Line count: 37,675 → 37,718 (+43). The code shrinks: the renderer goes from 359 to 295 lines, and the sensor scripts share one context and one report reader. The three new regression tests add back more lines than were removed. This PR is about correctness, not size.

Related issue

Refs #133 (defect 13 and audit findings on the adapters; the tracking issue stays open)

Defects fixed

  • Multi-line captures broke artifact lists. A capture body was inserted raw into a - bullet, so a second line fell out of the list. This affected change-brief, PR summary, and handoff artifacts. Continuation lines are now indented to stay inside their item. New test: tests/unit/artifact-render.test.ts.
  • One deleted account could fail a whole review snapshot (Reduce the codebase by 40% and fix the defects found in the cleanliness audit #133 defect 13). The sensor threw on an approval whose author is null (GitHub's ghost user), or whose commit is null. Neither can be a current approval by an identified human, so they are now left out. New test in github-review-sensor.test.ts, which fails on the old code.
  • HTTP failures were reported as "invalid JSON" (defect 13). A non-JSON error page, such as an HTML 502, was parsed before the status was checked, so the status was lost. It is now GitHub GraphQL review query failed: HTTP 502. New test, which fails on the old code.

Changes

  • Renderer. Eleven section(title, bullets(entries.filter(...))) blocks become one entrySection(title, entries, kinds, empty) helper, and the two changed-file sections one changedFilesSection. A golden comparison over all three artifact kinds, with and without entries, was byte-identical for single-line bodies.
  • Sensor scripts. sensorRunContext() is the one reader of the GitHub Actions run context: session, plan digest, repository, ref, HEAD, and run URI. gateSensorContext() builds on it with the gate-only rules (branch refs only, and the declared gate).
    • The review collect and sign scripts previously rebuilt that context by hand and skipped the session, plan-digest, and HEAD checks. They now validate it the same way, and review refs may still be refs/pull/....
    • Both sign steps read reports through one readReport().
  • Git snapshots resolve the base ref once per snapshot instead of once for each of files, stats, and commits. That is two fewer git rev-parse processes per snapshot, and all three parts now agree on the same base.

Impact

  • CLI commands, flags, help text, or exit behavior
  • Machine-readable JSON or protocol output
  • Persisted state, schema, or migration behavior
  • Generated Markdown or review artifacts. Multi-line capture bodies now render inside their bullet; single-line output is unchanged.
  • Git integration, daemon, or reconciliation behavior. Snapshots resolve the base once.
  • Installation, packaging, or supported runtimes
  • Documentation only
  • No externally observable behavior

The review sensor's snapshot now omits ghost and commit-less approvals, and its HTTP error text changes. The snapshot format is unchanged.

Validation

Check Result Notes
npm run check Pass 48 files, 838 tests; build and pack smoke passed
Renderer golden comparison Byte-identical All three kinds, with and without entries
New sensor tests fail on the previous code Confirmed

Risk and recovery

Low risk. Each fix is local. The review sensor change can only drop approvals that could never have counted as current human approval. Revert the commit to restore previous behavior.

Reviewer guidance

  1. entrySection: the continuation-line indentation is the only output change.
  2. sensorRunContext: the review scripts' newly added validation must not reject what the review sensor actually produces. refs/pull/... refs stay allowed, since the branch-only rule applies to the gate sensor alone.

Checklist

  • The PR is focused on the linked issue and contains no unrelated changes.
  • Tests cover new behavior and important failure paths, or I explained why tests are not needed.
  • CLI help, protocol output, examples, and docs remain aligned where applicable.
  • State or schema changes include compatibility and migration coverage where applicable.
  • User-facing or machine-readable breaking changes are called out explicitly.
  • Logs, fixtures, screenshots, and generated artifacts contain no secrets or sensitive data.
  • The branch is based on the latest origin/main.

…ewers

- A multi-line capture broke the Markdown bullet list it was rendered into;
  continuation lines are now indented to stay inside the item. The renderer's
  eleven entry sections and two changed-file sections go through one helper
  each, with byte-identical output for single-line entries.
- The review sensor failed the whole snapshot on an approval from a deleted
  account (null author) or on a commit GitHub no longer has. Such an approval
  cannot be a current human approval, so it is left out. A non-JSON error
  response now reports its HTTP status instead of "invalid JSON".
- Repository snapshots resolve the base ref once instead of once per part.
- Both sensors read their run context through one sensorRunContext, so the
  review sensor now validates the session, plan digest, repository, and HEAD
  like the gate sensor; both sign steps read reports through one readReport.

Refs #133
@nnennandukwe nnennandukwe added the bug Something isn't working label Sep 25, 2026
@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Harden review sensors and preserve multi-line artifact bullets

🐞 Bug fix ✨ Enhancement 🧪 Tests 🕐 20-40 Minutes

Grey Divider

AI Description

• Keeps multi-line captures inside Markdown bullets across rendered artifacts.
• Ignores ghost approvals and preserves HTTP status for malformed GitHub error responses.
• Shares sensor context and report validation while resolving snapshot base refs once.
Diagram

graph TD
  Actions["GitHub Actions"] --> Scripts["Sensor scripts"] --> Helpers["Sensor helpers"]
  Scripts --> Review["Review sensor"]
  Entries["Captured entries"] --> Renderer["Markdown renderer"]
  Repository[("Git repository")] --> GitAdapter["Snapshot adapter"]
Loading
High-Level Assessment

The PR’s focused helper extraction is the appropriate approach: it removes duplicated validation without introducing a broader abstraction, while resolving the Git base once guarantees internally consistent snapshots. A Markdown AST library or larger sensor framework would add disproportionate complexity for these localized defects.

Files changed (9) +221 / -178

Bug fix (2) +43 / -99
review-sensor.tsTolerate ghost approvals and retain HTTP failures +10/-2

Tolerate ghost approvals and retain HTTP failures

• Excludes approvals whose author or commit has disappeared instead of failing the entire snapshot. Reports the HTTP status when a non-successful GraphQL response cannot be parsed as JSON.

src/adapters/github/review-sensor.ts

artifacts.tsKeep multi-line captures inside artifact bullets +33/-97

Keep multi-line captures inside artifact bullets

• Indents capture continuation lines so multi-line bodies remain within their Markdown list item. Consolidates repeated entry and changed-file section rendering behind shared helpers.

src/renderers/markdown/artifacts.ts

Refactor (5) +74 / -79
collect-github-review-snapshot.tsUse shared workflow context for review collection +9/-12

Use shared workflow context for review collection

• Builds review snapshot inputs from 'sensorRunContext', aligning session, plan, repository, ref, HEAD, and run URI validation with other sensor steps.

scripts/collect-github-review-snapshot.ts

sensor-environment.tsCentralize sensor context and report validation +36/-13

Centralize sensor context and report validation

• Introduces shared GitHub Actions run-context validation, with gate-specific validation layered on top. Adds a common bounded JSON report reader that verifies file type, size, read stability, and parseability.

scripts/sensor-environment.ts

sign-ci-gate-receipt.tsReuse common captured-report reader +3/-16

Reuse common captured-report reader

• Replaces inline gate report file and JSON validation with the shared 'readReport' helper while preserving the existing size limit and errors.

scripts/sign-ci-gate-receipt.ts

sign-github-review-receipt.tsShare review signing context and report reading +15/-30

Share review signing context and report reading

• Uses the shared run context for receipt authorization and the common report reader for bounded JSON validation. This makes review signing enforce the same session, plan digest, repository, and HEAD constraints as collection.

scripts/sign-github-review-receipt.ts

client.tsResolve snapshot base refs once +11/-8

Resolve snapshot base refs once

• Checks base-ref availability once per repository snapshot, then shares the result across changed files, diff statistics, and commit range collection. This removes redundant Git processes and prevents inconsistent fallback decisions.

src/adapters/git/client.ts

Tests (2) +104 / -0
artifact-render.test.tsCover multi-line artifact bullet rendering +56/-0

Cover multi-line artifact bullet rendering

• Adds a regression test proving that blank lines and nested Markdown within a captured entry remain indented inside the parent bullet.

tests/unit/artifact-render.test.ts

github-review-sensor.test.tsCover ghost reviews and non-JSON HTTP errors +48/-0

Cover ghost reviews and non-JSON HTTP errors

• Adds regression coverage for excluding approvals with null authors or commits and for preserving a 502 status when GitHub returns an HTML error page.

tests/unit/github-review-sensor.test.ts

@qodo-code-review

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Grey Divider

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

Grey Divider

Tip of the day
💡 Did you know, you can enable the Remediation agent and Qodo fixes findings in a dedicated fix PR

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

@qodo-code-review

Copy link
Copy Markdown

Qodo Fixer

No findings are available for this PR yet. Findings appear here once Qodo has reviewed the PR.

@qodo-code-review

qodo-code-review Bot commented Sep 25, 2026 •

Copy link
Copy Markdown

No new reviewable changes. Local finding reconciliation was skipped because merge history is not supported. Unverified local findings were preserved.

@nnennandukwe
nnennandukwe merged commit 60697f5 into main Sep 25, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working 🔴 Large blast radius

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant