Skip to content

scripts: add deterministic CI check-state classifier#3385

Open
davidahmann wants to merge 1 commit intomodelcontextprotocol:mainfrom
davidahmann:codex/issue-3384-ci-state-classifier
Open

scripts: add deterministic CI check-state classifier#3385
davidahmann wants to merge 1 commit intomodelcontextprotocol:mainfrom
davidahmann:codex/issue-3384-ci-state-classifier

Conversation

@davidahmann
Copy link
Copy Markdown

Problem

Reference-server CI triage lacks a normalized machine-readable classification contract.

Why now

This is active operator friction in current workflows and needs deterministic behavior for repeatable triage/replay.

What changed

Added a Node-based classifier script and node:test coverage for no-checks, pending, failed, and policy-blocked classification.

Validation

  • node --test scripts/ci-state-classifier.test.mjs (pass)

Refs #3384

@davidahmann
Copy link
Copy Markdown
Author

Added a deterministic CI check-state classifier script with node:test coverage for core triage states used in maintenance workflows.

This contribution was informed by patterns from Wrkr: https://github.com/Clyra-AI/wrkr

@cliffhall cliffhall added the enhancement New feature or request label Apr 20, 2026
@cliffhall
Copy link
Copy Markdown
Member

@claude review

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Apr 21, 2026

Claude finished @cliffhall's task in 1m 38s —— View job


PR Review: scripts/ci-state-classifier

Tasks

  • Gather context and understand request
  • Read changed files
  • Analyze code quality, correctness, security, and repo fit
  • Post review findings

Summary

This PR adds a standalone Node.js script and tests for classifying GitHub CI check states into one of four categories: no checks, pending, failed, policy-blocked, or passed. The code is clean and the logic is straightforward, but there are several issues worth addressing before merging.


Issues

🔴 Critical — "cla" substring match causes false positives

scripts/ci-state-classifier.mjs, line 11

const POLICY_MARKERS = [
  ...
  "cla",
];

String.prototype.includes() does substring matching. "cla" will match any check name or summary containing those three characters in sequence — e.g., "scalability", "escalation", "declaration", "nuclear", "clarity", even the word "classify" itself. A check named "scalability-test" would be misclassified as policy-blocked.

If the intent is to detect a CLA (Contributor License Agreement) bot failure, this should match something more specific — e.g., "cla not signed", "contributor license agreement", or the specific bot name. Fix this →


🟡 Moderate — Missing test coverage

scripts/ci-state-classifier.test.mjs

There is no test for:

  • The passed outcome (all checks with conclusion: "success")
  • classify(null) / classify(undefined) / classify({}) — non-array inputs (the function handles these but they're untested)
  • A check where status is "completed" but conclusion is "success" (the most common real-world case)

Fix this →


🟡 Moderate — Repo fit / integration gap

This repo is exclusively MCP reference server implementations. The script has no connection to the existing CI workflows (verified — no workflow references it), no entry in any package.json scripts block, and no documentation explaining when or how it is used.

The PR description mentions "maintenance workflows" and links to an external project (Wrkr) as the source of patterns, suggesting this script may be intended for use by external tooling rather than this repo itself. If that's the case, it belongs in that external tooling, not here.

If it does belong here, it needs:

  • A scripts entry in the root package.json (e.g., "classify-ci": "node scripts/ci-state-classifier.mjs")
  • Documentation in a README or CLAUDE.md on what it's for and how to invoke it
  • Wiring to the actual CI workflows if it's meant to be used in them

🟠 Minor — Non-standard field names in summary construction

scripts/ci-state-classifier.mjs, lines 24–27

const summary = [check?.name, check?.context, check?.details, check?.title, check?.summary, check?.text]

The GitHub Check Runs API returns output fields nested under check.output.summary, check.output.text, check.output.title — not at the top level. The top-level details field doesn't exist in the API. For GitHub Commit Statuses (the older API), context and description are the relevant fields. This mixing of field schemas means the policy-marker scan may silently miss real data in practice.


🟠 Minor — No npm test integration

The test is only discoverable via node --test scripts/ci-state-classifier.test.mjs. It won't run with npm test in the root workspace. Given the monorepo uses vitest for all other TypeScript tests, either integrate this into the test matrix or convert to vitest.


Verdict

Not ready to merge. The "cla" false-positive issue is a correctness bug in the core classification logic. Beyond that, there's a broader question of whether this utility script belongs in this repo at all given it has no integration with the existing codebase or CI workflows.

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

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants