Skip to content

Replace the CI judgment reviewer with a deterministic standards check (per dev-env#60) #154

Description

@twistedmelonman

Implements the CI/local split from smartwatermelon/dev-env#60 across the fleet, and supersedes the open half of #133.

Not starting this now. Filed to hold the plan and the survey data. No action taken.

Premise

dev-env#60: "CI side checks should be quick, cheap, deterministic. Does the diff meet X laid out standard, yes or no. Judgment call checks should all be done on the Local side before pushing."

claude-blocking-review.yml is judgment by construction. Its five BLOCK criteria — "a clear bug," "a reliability regression," "unvalidated user input reaching a privileged operation" — are not yes/no against a standard. Its own prompt concedes the point: "Local reviewers … already cover style, naming, organization, test coverage, and documentation. Do NOT duplicate that work."

It is also the weakest reviewer holding the only blocking position: it cannot run tests (stated in its prompt), is capped at reading 3 files, and is told to default to PASS under uncertainty — while run-review.sh (code-reviewer + adversarial-reviewer), the pre-push whole-codebase review, and pre-merge-review.sh all run locally with fewer limits.

Doing this also removes the fleet-wide dependency on anthropics/claude-code-action being up, which is what #133 is actually about, without waiting on an upstream retry-policy fix.

Survey (measured 2026-08-19, not estimated)

35 non-archived, non-fork repos carry a claude-blocking-review.yml caller across both orgs — more than the ~24 in prior notes.

count
Check is genuinely required (blocks merges today) 27
Not enforced (branch unprotected, or Pro-gated private repo) 8

The load-bearing finding: 26 of those 27 have claude-review / run-review as their ONLY required check. The sole exception is personify (validate + claude-review / run-review).

A spot survey of workflow files shows most consumers have no deterministic CI at all — typically just claude-blocking-review.yml, claude.yml, and dependabot-auto-merge.yml. A minority carry validate.yml, tests.yml, code-quality.yml, or test-casks.yml.

So this is not a delete. Removing the reviewer today would leave 26 repos with zero required checks — strictly worse than the status quo, including for the outage scenario in #133. The deterministic layer has to exist before the reviewer comes out.

Repos where the check is not currently enforced (safe to use as pilots): dumbify, x-thread-reader, networth-agent, claude-code-workflows-agents, pr-review, repo-template, nightowlstudiollc/.github, scripts (Pro-gated).

Proposed sequence

Phase 1 — Build the deterministic check. A new reusable workflow here, e.g. standards-check.yml: shellcheck, yamllint, actionlint, zizmor, markdownlint, plus a repo's own tests when present. Fast, free, no secrets, no network beyond checkout. This is the "does the diff meet standard X" gate dev-env#60 describes. Open question: one reusable workflow with toggles, or compose from what already exists in the repos that have validate.yml.

Phase 2 — Pilot. Land it on the 8 unenforced repos first, where a mistake cannot block anyone. Confirm it is genuinely fast and quiet before fleet rollout.

Phase 3 — Roll out alongside, not instead of. Add standards-check to all 35 as a second check while claude-review keeps running. Both green for a period, so we can compare what each catches — this is also the honest way to test the premise that the CI reviewer adds nothing the locals do not.

Phase 4 — Flip the required check. Per repo: add standards-check to required, then remove claude-review / run-review. Order matters — never leave a repo with zero required checks mid-flight. 26 repos need this; personify already has validate as a floor.

Phase 5 — Retire the reviewer. Remove callers, cut a new major here (removal is a breaking change for consumers on floating @v3/@v1, so it is a coordinated tag operation, not a delete — see #90 and the skip-guard notes). Then drop CLAUDE_CODE_OAUTH_TOKEN from the repos that no longer need it, which is the secret-sprawl win dev-env#60 anticipates.

Decisions needed before Phase 1

  1. Does the local side actually cover every author? The judgment reviewers run from ~/.claude/hooks, so they cover commits made through this setup. Anything landing another way — Dependabot, the GitHub web UI, a contributor without the hooks — currently gets the CI reviewer as its only judgment pass. Dependabot PRs are already skipped, so the real question is whether the other paths matter in practice.
  2. Keep a judgment reviewer anywhere in CI, in non-blocking form? Advisory-only, does not gate merges. Preserves a backstop for path 1 without reintroducing the outage coupling.
  3. claude-assistant.yml is out of scope — it is @claude-triggered and not a merge gate, so it keeps its token regardless. Worth stating so "remove the Anthropic token" does not over-apply.
  4. Does nightowl-restore-blocking-review.sh get retired too? It exists to restore exactly what this removes.

Refs #133, smartwatermelon/dev-env#60

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions