ci: make PR checks the enforced source of truth with coverage ratchets - #105
Conversation
Coverage enforcement: - Pin v8 coverage thresholds in both packages' vitest configs (agent: 82/74/88/82, mcp: 90/80/92/90 for stmts/branches/funcs/lines). Any PR that lowers coverage now fails the unit-test job. - Exclude src/**/*.test.ts from coverage accounting. - Emit json-summary + html reports; CI uploads them as artifacts and posts a per-package coverage table to the job summary. New unit tests (+76) closing the biggest gaps in @openrouter/agent: - tool-orchestrator.ts: 0% -> 89% (executeToolLoop rounds, unknown/manual/ failing tools, HITL null-pauses, async-lifecycle rejection, generator preliminary results, nextTurnParams steering, result helpers) - claude-type-guards.ts: 0% -> 100% (isClaudeStyleMessages detection and rejection paths) - stop-conditions.ts: 32% -> 100% (all conditions incl. OR logic, async conditions, rejection propagation) - next-turn-params.ts: 34% -> 100% (context building, composition order, invalid keys, non-object arguments) - turn-context.ts: 40% -> 100% CI hardening (.github/workflows/ci.yaml): - e2e job no longer silently passes without OPENROUTER_API_KEY: it fails loudly, except on fork PRs where secrets are legitimately unavailable. - Add push-to-main and nightly schedule triggers. - Add a ci-status aggregator job as the single branch-protection check; the required set is defined in one 'needs' list so a renamed or deleted job can never leave the gate green. - Add permissions: contents: read and PR concurrency cancellation. Also removes a write-only 'steered' variable in agent-tool.test.ts that was failing biome on main (lint gate was red before this change).
Coverage is a unit-test gate; the e2e project exercises a thin slice of src against the live API and can never meet the pinned thresholds. Pass --coverage.enabled=false in both packages' test:e2e scripts.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| ci-status: | ||
| name: CI status | ||
| if: always() | ||
| needs: [lint, typecheck, unit-tests, e2e-tests, structural-gate] | ||
| runs-on: ubuntu-latest |
There was a problem hiding this comment.
🟡 The new single required check has a different name than the one maintainers are told to require
The aggregated check is published under the display name "CI status" (name: CI status at .github/workflows/ci.yaml:167) while the workflow comment and PR instructions tell maintainers to require it as "ci-status", so a required check with that name would never report and every pull request would stay blocked as pending.
Impact: If branch protection is configured exactly as instructed, all PRs become permanently unmergeable because the required check never appears.
Job id vs job display name in GitHub required status checks
GitHub creates the check run using the job's name when one is set, falling back to the job id otherwise. Here the job id is ci-status but name: CI status overrides the reported check name, so the status check visible to branch protection is CI status.
The comment at .github/workflows/ci.yaml:162-165 says: Point branch protection at THIS job ("ci-status"). Selecting ci-status in branch protection (it can be typed in manually even if not in the dropdown) yields a required check that is never reported → PRs remain in a pending state forever.
Either drop the name: key so the check is literally ci-status, or update the comment to reference CI status.
| ci-status: | |
| name: CI status | |
| if: always() | |
| needs: [lint, typecheck, unit-tests, e2e-tests, structural-gate] | |
| runs-on: ubuntu-latest | |
| ci-status: | |
| if: always() | |
| needs: [lint, typecheck, unit-tests, e2e-tests, structural-gate] | |
| runs-on: ubuntu-latest |
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Summary
Makes CI checks on PRs the actual gate they were pretending to be. Previously coverage was collected but nothing enforced it, the most critical file (
tool-orchestrator.ts) had 0% coverage, and the e2e job silently passed whenever the API key was missing.What changes
1. Coverage thresholds, pinned and enforced
unit-testsjob (verified: exit 1 on breach).src/**/*.test.ts) excluded from coverage accounting.2. +76 unit tests closing the worst gaps (
@openrouter/agent)tool-orchestrator.ts(the tool execution loop, public API)claude-type-guards.tsstop-conditions.tsnext-turn-params.tsturn-context.tsOrchestrator tests cover: multi-round loops, unknown/manual/failing tools, HITL null-pauses, background/deferred lifecycle rejection (asserting
runis never invoked), generator preliminary-result forwarding, andnextTurnParamsconversation steering.3. CI workflow hardening
OPENROUTER_API_KEYis now a hard error — except on fork PRs, where secrets are legitimately unavailable (skip with warning).pushto main + nightly schedule (full suite incl. live e2e).ci-statusaggregator job: point branch protection at this single check. The required set lives in oneneeds:list, so a renamed or deleted job can never leave the gate green, and adding a check automatically extends it.permissions: contents: read, PR concurrency cancellation.4. Drive-by fix
Removed a write-only
steeredvariable inagent-tool.test.tsthat was failing biome — the lint gate on main is currently red, which this PR also fixes.Verification
pnpm lint,pnpm typecheck,pnpm testall green (909 agent + 168 mcp tests).ci.yamlparses; coverage-summary script output verified.Not done (deliberately)
ci-statusas required check) must be flipped in repo settings after merge.Remaining known gaps
stream-transformers.ts(50%, 38% branch) andstream-type-guards.ts(69%) are the next biggest under-tested areas.allSettledrejection path inexecuteToolLoopis effectively unreachable via public APIs (executor converts failures to error results) — noted in tests rather than covered with mocks.