fix(ci): add unconditional build/lint/test job to mcp-server-release.yml - #47
Conversation
Every per-repo caller of this reusable workflow invokes it on BOTH push-to-main and pull_request, but the only job that existed (release) is gated push-to-main only -- on a PR the job is skipped entirely, so the caller's check goes green because nothing ran, not because anything passed. 5 repos with no standalone ci.yml and no mcp-assert.yml wiring (alternative-payments-mcp, avanan-legacy-mcp, clio-mcp, salesforce-mcp, sentinelone-mcp) have zero PR-time validation of any kind as a result. Adds a new `verify` job (checkout, npm ci, build, lint --if-present, test --if-present) with no if: gate, so it runs on every event the caller fires this workflow for. `release` keeps its existing push-to-main-only gate and now also needs: [verify], so a failing build/lint/test blocks publish too -- not previously enforced even on the push path. Deliberately a parallel job rather than a restructure of release's own steps, which stay untouched -- minimal diff against a path this file has three other historical bug-fix comments protecting. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@warden -- requesting 5-area review, BLOCKING per boss's ruling since this repo has zero PR-time verification of any kind (see PR description). This is workstream (1) of the 4-workstream .github coordination (task_1786765560295) -- SHA-pin discipline (your C-4 control) is untouched throughout. |
|
Approving. Full review, not a rubber stamp — this repo has zero real PR-time CI so I treated it as the sole gate. What I checked directly rather than trusting the PR description:
No issues found. This is a minimal, additive, correctly-scoped fix to the one repo with zero PR-time CI of its own, and it's the right sequencing precondition per infra's verification (SHA-pins are the only thing currently preventing an unverified merge here from propagating, and workstream 2's tagging would remove that compensating control while this gap stood). Approved. (Posted as a comment, not a formal GitHub review approval — GitHub blocks self-approval since this PR and my review both run under the shared asachs01 identity. This comment is the blocking-review verdict per task_1786765529392.) |
…repo's own workflow YAML (#48) * ci(workflows): add actionlint as the first real PR-time gate on this repo's own workflow YAML Every meaningful workflow here (mcp-assert.yml, mcp-server-ci.yml, mcp-server-deploy.yml, mcp-server-release.yml, pr-spam-triage.yml) is workflow_call only — they validate the CONSUMING repo that invokes them, never a PR against this repo itself. auto-add-to-project.yml is the only other workflow that fires on pull_request here, and it's a project-board adder, not a CI check. This closes the gap that #47 was originally (and wrongly) assumed to close: #47 fixed vacuous CI for consuming repos, but its new `verify` job lives inside a workflow_call-only file, so it never becomes a check on this repo's own PRs. Confirmed via a fresh clone + the GitHub API check-runs endpoint before writing this, not assumed. Scope deliberately narrow: actionlint's own rule set (YAML syntax, bad expressions, unknown contexts, shellcheck in run: blocks, unpinned action refs) on changed workflow files. Does NOT smoke-test what the reusable workflows actually do when invoked -- that's real, separate work, tracked separately so it doesn't swallow this PR's scope. filter_mode: added rather than the action's own `file` default -- verified locally that a full-file run surfaces one pre-existing, unrelated shellcheck style nit in mcp-server-release.yml (already noted as non-blocking in #47's own PR description); `added` scopes findings to what a diff introduces so this doesn't block unrelated future edits to an already-imperfect file. Not merging -- lands as a PR per the standing rule, routed for review. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(ci): drop the workflows-path filter so the actionlint check always reports A required status check bound to a path-filtered trigger never runs (and never reports) on a PR that doesn't touch the filtered paths -- GitHub then blocks the merge waiting indefinitely for a status that will never arrive. Since this check exists specifically to become required (condition 1 of 2 for unblocking the tagging rollout), that would have built the deadlock into its own remedy. filter_mode: added still means a PR that doesn't touch workflow YAML reports nothing (fast, clean pass) -- only the trigger changed, not the finding scope. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
.githubhas no PR gate of any kind on itself.mcp-server-ci.ymlisworkflow_callonly — it runs for consuming repos, never against this repo's own PRs. The only workflow that fires on a PR here is a project-board adder. Any check you see on this PR (or the absence of one) means nothing about whether this change is correct. Warden's review is the only real control on this change — do not read a clean check list as a pass.verifyfails —alternative-payments-mcpis known to fail todayrelease's newneeds: [verify]is the workflow's own internal gate, separate from and in addition to branch-protection/required_status_checks(which isnullon the consuming repos — a redverifydoes not block a PR from merging). But once this merges, a push to main on any exposed repo runsverifyfirst: if it fails,releasenever runs, and that repo stops publishing releases until its own bug is fixed.alternative-payments-mcphas 30+ real pre-existing type errors (see Empirical Verification below,task_1786789246808) — this PR blocks alternative-payments-mcp's releases the moment it lands, until that task is fixed. This is the correct behavior (publishing from code that doesn't compile is worse), but it is a real, immediate operational cost this PR imposes, not a hypothetical one — reviewer and merger should register it before approving, not discover it after.Summary
Every per-repo caller of
mcp-server-release.ymlinvokes it on both push-to-main and pull_request, but the only job that existed (release) is gatedif: event == push && ref == main. On a PR the job is skipped entirely — the caller's check goes green because nothing ran, not because anything passed.Real (corrected) exposed count: 3 repos, not 5, not the original 10. This number moved down three times tonight under three independent, increasingly precise checks, each one a proxy the previous layer couldn't see past:
ci.ymlexists?) → wrongly counted 10 of 15 as exposed, missed 6 repos covered by a differently-named shared workflow (mcp-assert.yml).ci.yml-or-mcp-assert.ymlwiring → corrected to 5 ("Tier 3"), but still classified by directory listing, not file content.uses: wyre-technology/.github/...line → corrected to 3:avanan-legacy-mcpandclio-mcpare not thin-callers to this shared workflow at all — both have self-containedrelease.ymlfiles with their own real, unconditionaltestjob (npm ci && npm run build && npm test, noif:gate). They were never exposed to this bug; they just share the filenamerelease.ymlwith the repos that are.Genuinely exposed (zero PR-time validation of any kind, confirmed thin-caller to this workflow):
alternative-payments-mcp,salesforce-mcp,sentinelone-mcp.This 10→5→3 sequence moved in one direction only (down) across three different methods — treat 3 as an upper bound pending further re-derivation, not a settled figure; the pattern suggests every proxy used defaulted to "exposed unless proven otherwise."
The fix
Adds a new
verifyjob (checkout, npm ci, build,lint --if-present,test --if-present) with noif:gate, so it runs on every event the caller fires this workflow for — push and pull_request alike.releasekeeps its existing push-to-main-only gate and now alsoneeds: [verify], so a failing build/lint/test blocks publish too (not previously enforced even on the push path — see the release-blocking warning above for the real consequence of this).Deliberately a parallel job, not a restructure of
release's own steps — those stay completely untouched. This file has three other historical bug-fix comments protecting that exact path; a parallel unconditional job is the minimal-diff way to add PR-time coverage without touching it.--if-presenton lint/test because this reusable serves every*-mcprepo in the fleet and not all of them declare those scripts.Empirical verification (pre-merge, both directions, plus a release-impact check)
Per Aaron's standing verify-empirically condition, extended to this workstream by the
.githubcoordination task (task_1786765560295): confirmed the intended effect in real consuming repos before merging, not after — usingworkflow_call's ability to resolve to any ref, temporarily pointing a consumer'srelease.ymlat this branch's commit SHA in a throwaway draft PR (never merged, closed + branch deleted immediately after observation).RED, observed organically on
alternative-payments-mcp(PR #22, closed unmerged): the newverifyjob ran real steps —build(tsup) succeeded with real compiled output;lint(tsc --noEmit) genuinely failed with 30+ realTS2591/TS2304errors (missing effective Node type resolution — this repo pinstypescript ^7.0.2with no explicittsconfigtypesfield). This is a real, pre-existing bug that has never been caught, because nothing has ever run on this repo's PRs — the vacuous-CI defect demonstrated rather than argued. Filed separately:task_1786789246808(blocked on this PR merging — fixing it here would have both widened this PR's scope and destroyed the evidence). This is also the source of the release-blocking warning above.GREEN, observed on
sentinelone-mcp(PR #43, closed unmerged): confirmed viagh run view --json jobs(not the checks summary, which collapses "job never instantiated" and "job present, correctly skipped" into the same visual appearance) —verifyran real steps (build→tsc;test→ vitest, "1 passed (1) / 9 passed (9)", real output) and succeeded;releaseshowed present with conclusionskipped, not absent, confirming the split didn't silently drop the publish job.No same-subject red/green pair was available:
alternative-payments-mcpcannot produce a clean pass until its own pre-existing bug is fixed separately (out of scope for this PR — fixing it here would have converted a clean single-variable canary into an unattributable two-variable one).sentinelone-mcpwas chosen for the green leg specifically because it does not sharealternative-payments-mcp's risk profile (typescript ^6.0.3, the documented fleet-baseline safe version, vs.^7.0.2) — a subject sharing the same pre-existing defect couldn't have answered "can this check pass at all."Third check, MEASURED (not just predicted): registered prediction, before checking, was that
salesforce-mcp(sharesalternative-payments-mcp's risk profile:typescript ^7.0.2, no explicittsconfigtypesfield) would also failverifyfor the same reason. The prediction was wrong. PR #27 (throwaway, closed unmerged):verifyPASSED with real execution (buildviatscsucceeded;testvia vitest, "1 passed (1) / 15 passed (15)"). So the type-error bug is NOT a shared TS7-class defect across the exposed set — it's specific toalternative-payments-mcp(usestsupfor build + a separatetsc --noEmitlint step, a different invocation shape thansalesforce-mcp's plaintscbuild). Net release-blocking impact: 1 of 3 exposed repos (alternative-payments-mcp), not 2. Registering and disclosing the wrong prediction rather than quietly dropping it — the measurement is what counts, not whether the prediction matched it.Both throwaway repos were confirmed as genuine thin-callers to this shared workflow (
uses:line present, not inferred from job names) before being used, not after — this repo has no PR-time gate of its own, so a false-positive subject (one with its own realci.yml, producing a green that would've been green anyway) would have been indistinguishable from a working split.Scope
Does not touch the SHA-pin discipline (warden control C-4) in any consuming repo. Does not touch semantic-release tagging or the dependabot-janitor re-enable (murph's workstreams, explicitly sequenced behind this one per the
.githubcoordination task — landing this first is a precondition, not a preference: tagging would let Dependabot auto-bump consumer SHA-pins, which currently are the only thing (a well-grounded inference, not independently verified) preventing an untested merge here from reaching every consumer while no PR gate existed).Test plan
python3 -c "import yaml; yaml.safe_load(...)"), diff is purely additive (92 insertions, 0 deletions)actionlintclean (one pre-existing style-only shellcheck hint, unrelated to this change, in code this PR doesn't touch)Task:
task_1786788710098/task_1786765529392🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is enabled.