Skip to content

Harden review infrastructure with contextual APIs - #375

Merged
taras merged 13 commits into
mainfrom
agent/review-infrastructure
Aug 9, 2026
Merged

Harden review infrastructure with contextual APIs#375
taras merged 13 commits into
mainfrom
agent/review-infrastructure

Conversation

@taras

@taras taras commented Aug 7, 2026

Copy link
Copy Markdown
Owner

Why

PR Review and Repo Analysis execute Markdown from the checked-out revision.
#305 is now present on main; this prerequisite keeps its own review documents
compatible with that contract through mechanical props.* spelling changes.
It does not implement or modify the props language behavior.

What changes

  • CI runs the checked-out ./dist/xmd inside top-level <Output> regions.
  • GitHubAuth.md provides lexically scoped exact-host GitHub authentication.
  • ReviewContext, RepositoryInventory, Doctor, and OxlintDiagnostics
    are typed function components with explicit schemas and contextual APIs.
  • Doctor classification, Oxlint normalization, comment classification, and
    review state construction use small typed helpers while Markdown remains the
    composition and prompt layer.
  • Raw diagnostic payloads do not cross the durable boundary. The review
    boundary fails on malformed or unexpected Oxlint output; published tolerant
    parseDoctorResult and parseDiagnostics behavior remains unchanged.
  • Bot replies use GitHub's user.type, and malformed comments payloads fail
    the review instead of becoming empty history.
  • Review components whose declared props are consumed in executable Markdown
    use the current props.* binding contract; this is a document migration only.

Diagnostic disposition

The latest review diagnostic report was treated as an engineering signal, not
as a reason to dismiss the review. The new and materially rewritten code now
has decomposed component boundaries, explicit unknown JSON narrowing,
preserved parsing causes, immutable sorting, and corrected nullable/error paths.

The review-only Oxlint configuration excludes conventions incompatible with
this repository's architecture: component filename casing, required named
component exports, export ordering, generator/function style and yield rules,
mechanical import/key ordering and magic-number rules, and readonly parameter
types for contextual component boundaries. The normal repository lint
configuration is unchanged and remains authoritative for those rules. Pure
comment parsing and formatting now lives in a verified code-review-agent
module, leaving CommentReviewData.ts as a thin I/O and orchestration
boundary.

Review guide

Start with: .reviews/ReviewPR.md, .reviews/components/GitHubAuth.md,
.reviews/components/Doctor.ts, and scripts/tests/review-infrastructure.test.ts.

Then review:

  1. Doctor.ts, OxlintDiagnostics.ts, ReviewContext.ts, and
    RepositoryInventory.ts for typed contextual boundaries.
  2. CommentReviewData.ts, CommentReviewState.ts, and the tolerant parser
    contracts in packages/code-review-agent.
  3. .github/workflows/review.yml and repo-analysis.yml for checked-out
    execution and always-uploaded journals.

Look carefully at: exact-host matching, request/body/header preservation,
credential scope, malformed payload failure behavior, and the absence of
journal/result postflight parsing.

What must stay true

  • Errors beneath <Output> fail xmd run; ordinary review findings do not.
  • No raw Oxlint output, token, arbitrary diagnostic field, or caught error is
    persisted as a document binding.
  • GitHub authorization is inherited by projected children, restored after
    scope exit, and absent from durable journal output.
  • Function-component generator arguments and published parser behavior remain
    compatible.
  • No secret-detection opt-out, bare-prop compatibility fallback, props-language
    implementation change, core change, or runtime change is included.

How to verify it

  • deno task lint — pass with 0 errors.
  • deno task check — pass, including every .reviews/components/*.ts file.
  • pnpm exec tsc --project tsconfig.node.json --noEmit — pass; the ES2022
    Node target no longer depends on Array#toSorted().
  • The full Deno suite completed at the preceding rebase head with 410 passed
    and 0 failed; the focused review-infrastructure suite passes 10/10 under
    Deno after the final document migration.
  • deno task check:jsr — pass, Success Dry run complete.
  • Focused review-infrastructure tests pass under Node and Bun; full local
    portable runs are limited by this environment's loopback socket permission.
    The push-triggered GitHub Node/Bun checks are the authoritative full suites.
  • git diff --check — pass.

Scope

Included

  • Checked-out review-binary bootstrap and native <Output> failure behavior.
  • Scoped GitHub authentication through @effectionx/fetch.
  • Typed review components, bounded diagnostics, focused security/failure
    tests, and surgical code-review-agent/Oxlint specification updates.
  • Mechanical props.* migrations in the PR-authored review documents required
    by Namespace Markdown props under the props binding #305's now-current language contract.

Intentionally unchanged

  • Namespace Markdown props under the props binding #305 props syntax and language implementation; it is already on main.
  • WorkflowRun, Workspace restoration, SQLite, replay, Git, Agent, and
    xmd workflow lifecycle code.
  • Core and runtime source, generic runtime fetch behavior, public PR model
    types, diff-preview limits, function-component generator arguments, and
    secret-detection defaults.
  • Published parser API behavior; no changeset, release metadata, or version
    bump is required.

New dependencies

None.

Risks and limitations

  • EnsureOxlint.md retains only irreducible host operations: binary download,
    archive extraction, checksum verification, executable-mode changes, and
    cleanup.
  • GitHub CI and the rendered review are being rerun for this correction at the
    exact pushed head.

Scope confirmation

  • Every changed file supports review infrastructure.
  • Unrelated runtime, lifecycle, and props-language changes are excluded.
  • The diagnostic disposition matches the final diff.

@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown

PR #375: Harden review infrastructure with contextual APIs

41 files, +3275 / -2451

Scope

🔴 PR has 5726 lines changed. Split into focused PRs.

🟡 5726 lines changed. PRs under 400 receive more thorough review.

🟡 41 files changed. Are all changes related?

🟡 Changes span 13 directories.

🟡 PR mixes config and source changes.

Structural

✅ No structural bloat detected.

Slop

✅ Slop indicators look low.

Static Analysis

Oxlint: 1 diagnostic across 1 file (1 rule)
Density: 0.000 violations/added-line

no-array-sort (1): packages/code-review-agent/src/doctor.ts

Correctness

No extraneous code patterns detected.

@taras
taras marked this pull request as ready for review August 7, 2026 18:23
@taras
taras force-pushed the agent/review-infrastructure branch from 876aa86 to a940bdb Compare August 7, 2026 20:00
@taras taras changed the title Harden checked-out review execution Harden review infrastructure with contextual APIs Aug 7, 2026
@taras
taras force-pushed the agent/review-infrastructure branch 7 times, most recently from 81b4cfa to f09d7cb Compare August 8, 2026 10:40
@taras taras closed this Aug 8, 2026
@taras taras reopened this Aug 8, 2026
@taras
taras force-pushed the agent/review-infrastructure branch from 2a1ce58 to bfe133f Compare August 8, 2026 21:10
@taras
taras force-pushed the agent/review-infrastructure branch from 33b26a1 to 1af2808 Compare August 9, 2026 01:04
@taras
taras enabled auto-merge (squash) August 9, 2026 01:19
@taras
taras merged commit 2a8d78e into main Aug 9, 2026
11 checks passed
@taras
taras deleted the agent/review-infrastructure branch August 9, 2026 01:22
taras added a commit that referenced this pull request Aug 9, 2026
* 🔧 Share one Oxlint policy between the gate and the sensor

The repository lint gate and the review sensor each carried a full copy of
the Oxlint catalog, and the copies had drifted: the sensor turned pedantic
and style on, so it reported 96 diagnostics over PR #375's files, nearly all
generic style noise. The two also ran different Oxlint versions, so the
profiles could not be compared at all.

oxlint.shared.json becomes the one built-in policy. Both profiles extend it
through a native JSON extends path, so a profile can no longer enable a rule
the shared policy disables. The gate adds only its local JavaScript plugin
and blocking rules; the sensor adds nothing.

Oxlint and tsgolint are pinned to the exact versions the production review
sensor already provisions, so a routine lock refresh cannot expand the rule
set without changing the sensor.

SENSOR_RULES in categories.ts becomes the one TypeScript catalog, and Doctor
derives its available and missing rules from it.

scripts/tests/oxlint-policy.test.ts proves inheritance, catalog ownership,
the sensor-subset relation, override survival, and version agreement under
Deno, Node and Bun.

Closes #395

* 🐛 State the sensor-subset invariant as tests, not inheritance

The specifications claimed extends made a profile-level override impossible.
Oxlint merges first to last and the later configuration wins, so a profile
that redeclared an inherited rule would override it. The real invariant is
that the committed sensor profile declares no built-in rule of its own and
that its active built-in set stays a subset of the gate's, with OP2 and OP5
failing verification when either stops holding.

runOxlint gave stdout and stderr one shared TextDecoder, so a multi-byte
character split across chunks on one pipe could take its continuation bytes
from the other, and a character left pending at exit was dropped. Each pipe
now has its own decoder, and both are flushed after the process settles.

The conformance suite takes its directory and file operations from
@effectionx/fs, keeping node:fs only for mkdtempSync, which that package
does not provide.

---------

Co-authored-by: Taras Mankovski <74687+taras@users.noreply.github.com>
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