Skip to content

ci: adopt nc-review - #26

Merged
will-lamerton merged 1 commit into
mainfrom
ci/adopt-nc-review
Sep 7, 2026
Merged

ci: adopt nc-review#26
will-lamerton merged 1 commit into
mainfrom
ci/adopt-nc-review

Conversation

@will-lamerton

Copy link
Copy Markdown
Member

Third repository onto the shared review workflow. Two files: the caller, and this project's own rubric — the reviewing method is fetched from Nano-Collective/.github at run time rather than copied.

Why this rubric is not boilerplate

Sentinel's entire output is a claim about someone else's code, so it has one failure mode worse than the rest: a green report over a broken estate. A run that silently fails to scan something tells the operator their repositories are clean when nobody looked — worse than no tool, because a tool gets trusted.

The rubric names the ways that has actually happened here:

  • Errors collected and never rendered. packLoadErrors lived on the run report unread; targetErrors was persisted to every run record and displayed on no surface, so a run where every repo failed to clone rendered as "0 findings across 0 repos" in the same calm grey as a clean estate.
  • Distinct failures conflated — a missing pack and a pack with an unresolvable depends_on chain reported identically, sending readers hunting for a file already on disk.
  • Auto-resolution closing live findings when scanned scope changes. That is the tool reporting a vulnerability fixed because it stopped looking, and it is why [Feature] Incremental scanning, and the auto-resolution fix it requires #17 must land with the cache rather than after it.
  • Trusting any existing directory as a checkout of the right repository.
  • Model output as untrusted inputInfinity, NaN and fractional line numbers all passed the original range check.

It also gives the review question that catches the whole class: where does this error surface? "It is returned" is not an answer — follow it to the run report, the CLI summary, the run record or the dashboard, or it is discarded.

Two things a reviewer carrying another repo's conventions would get wrong

Config

architecture-doc points at docs/workflow/index.md, which the source already cites by section as the design of record. There is no CLAUDE.md here.

Rollout note

Copilot review stays on this repo until nc-review is producing verdicts here — removing it first would leave the repo with no automated review at all, inverting Q7.

pull_request_target resolves the workflow from the base branch, so this PR will not be reviewed by it; the first PR opened after merge will.

Third repository onto the shared review workflow. Two files: the caller, and
this project's own rubric. The reviewing method is fetched from
Nano-Collective/.github at run time rather than copied.

The rubric is built around the one failure mode that matters for an auditing
tool: a green report over a broken estate. Sentinel's entire output is a claim
about someone else's code, so a run that quietly fails to scan something is
worse than no tool, because a tool gets trusted. It names the ways that has
actually happened here:

  - errors collected and never rendered — packLoadErrors unread, targetErrors
    persisted to every run record and displayed on no surface, so a run where
    every repo failed to clone looked identical to a clean estate
  - distinct failures conflated into one message, which sent readers hunting
    for a file that was already on disk
  - auto-resolution closing live findings when scanned scope changes, which is
    the tool reporting a vulnerability fixed because it stopped looking
  - trusting any existing directory as a checkout of the right repository
  - treating model output as trustworthy; Infinity, NaN and fractional line
    numbers all passed the original range check

It also records the review question that catches the whole class: where does
this error surface? "It is returned" is not an answer.

Two repo-specific things a reviewer carrying another project's conventions
would get wrong: test:types typechecks the specs here, so a fixture missing a
newly required field is a real error; and clone.ts and gh-client.ts carry
c8 ignore blocks, so logic buried inside one is invisible to coverage and
belongs outside behind an injected seam.

architecture-doc points at docs/workflow/index.md, which the source already
cites by section as the design of record. There is no CLAUDE.md here.

Copilot review stays until nc-review is producing verdicts on this repo.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PZY52ePXLjwG9TaQgq2cHT
@will-lamerton
will-lamerton requested a review from a team as a code owner September 7, 2026 13:32
@github-actions

github-actions Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

No changeset found

This PR does not add a changeset, so it will not appear in the changelog or trigger a release.

If the change is user-facing, add one:

pnpm changeset

Pick a bump (patch / minor / major) and write the changelog entry in our usual voice ("Added X... Thanks to @you. Closes #123."), then commit the generated .changeset/*.md file.

If this PR is docs-only or a chore that needs no release note, you can ignore this — or run pnpm changeset --empty to record that intentionally.

@will-lamerton
will-lamerton merged commit 07955e5 into main Sep 7, 2026
12 checks passed
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