Repository navigation
chore(tooling): harden the workflows and scripts the reviewers flagged - #1895
Merged
Merged
Conversation
Closes the review threads left open on merged PRs against the CI workflows, the repo's gate scripts and a few dev tools. Each change is the smallest one that closes its finding. Behaviour changes are covered by a new suite (test/continuous-test-suite-tooling-scripts.ts, `pnpm run test:tooling-scripts`) and by additions to the provider-structure and provider-descriptors suites; every new test was run red against the unfixed source first. Workflows - ci.yml: persist-credentials: false on the seven checkouts that never push, semantic-release-validation keeps its token (T3790294038, #1335). The permissions comment names that job as the one contents: write exception (T3858986829-a, #1552). The pinned suite counts and the 373-assertion figure are gone (T3869180755-f1, #1580). The new tooling-scripts suite is added to extended-suites; its weight (100) is an estimate, not a CI median. - release.yml: the ffmpeg note no longer says build-check gates this workflow (T3810295618-a, #1360). - single-commit-enforcement.yml: one SKIP_RE shared by both greps, printf instead of echo, and the guidance names the push/pull_request workflows rather than "every workflow" (T3813387872-printf-regex, T3813416696-overstated-guidance, #1364). Config and lint docs - config/models.json: Opus 4.5 uses the real snapshot id 20251101 for anthropic, bedrock and vertex instead of the 20251124 launch date (T3816077440, #1375). provider-structure now checks every Claude id in the file against the model enums. - eslint-rules/index.cjs: header lists e2e-tests-only, no-inline-secret-regex, provider-typed-errors and provider-base-class (T3801758166-1, #1344). Scripts - build-validations.ts: fails when typedoc.json carries an unanchored `**/<dir>/**` exclude, which drops every file under a checkout whose path contains that directory (T4042344752-guard, #1723). - check-banned-deps.ts: scans each file as a whole, so import(), require() and `from` followed by a specifier on the next line are found, and a `//` inside a string no longer hides the rest of the line (T3956062753, #1662). Files in the repo root and .mts/.cts are scanned too (T3956062775, #1662). - check-shipped-types.ts: the declarations under dist/ must equal the set the source tree emits, so a partial or stale build above the 100-file floor fails (PF-T3927528338, #1627). A wildcard export is matched against the whole pattern, including a `*` in a directory component (T3931686738-wildcard-match, #1632). - codex-replay-listener.ts: the tool-call script names `replay_tool` instead of `exec`, which Codex declares as a custom tool and which raised a Fatal "incompatible payload" error (F1-T4087477953-custom-tool-shape, #1783); reproduced and cleared against codex-cli 0.160.0. --requests counts served /responses turns, so a 404 probe cannot shut the listener down first (F2-T4087477985-requests-limit-counts-404s, #1783). - commit-validation.ts: execFileSync("git", [...]) instead of a shell string; behaviour unchanged (T3838161513-b, #1499). - migration-symbol-diff.mjs: this/super-rooted paths keep their full name, and tagged templates, obj["name"](), super() and import() are tracked; the header says it follows calls (T3835058026-residual, PF-T3833252257, #1448). - tools/automation/environmentManager.ts: credential-free providers count as configured only when the .env sets one of their variables, the score no longer divides by the size of the catalog, and the report lists the configured providers plus one count instead of every missing one (T3792794348, T3792807279, #1337). Not done, on purpose - The skip-checks trailer in the single-commit grep (optional in the finding). - Checkouts in workflows other than ci.yml: the findings named only ci.yml. - migration-symbol-diff still does not record a function passed by reference (`items.forEach(handler)`); the header now says so. Pre-existing, not touched: test:dynamic fails its five live cases without provider credentials, identically with config/models.json reverted.
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 58 minutes. View limit detailsLimit details: You’ve used all 2 included reviews currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (17)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
✅ Single Commit Policy - COMPLIANTStatus: Policy requirements met • 1 commit • Valid format • Ready for merge 📊 View validation details📝 Commit Details
✅ Validation Results
🤖 Automated validation by NeuroLink Single Commit Enforcement |
Contributor
|
🎉 This PR is included in version 12.44.7 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Hardens the CI workflows, gate scripts and a few dev tools that reviewers flagged on merged PRs. Behaviour changes are covered by a new suite (
pnpm run test:tooling-scripts) and by additions to the provider-structure and provider-descriptors suites.What changed
Closes the review threads left open on merged PRs against the CI workflows,
the repo's gate scripts and a few dev tools. Each change is the smallest one
that closes its finding. Behaviour changes are covered by a new suite
(test/continuous-test-suite-tooling-scripts.ts,
pnpm run test:tooling-scripts)and by additions to the provider-structure and provider-descriptors suites;
every new test was run red against the unfixed source first.
Workflows
semantic-release-validation keeps its token (T3790294038, feat(providers): dead-code purge, tier-A provider fixes, CI safety net #1335). The
permissions comment names that job as the one contents: write exception
(T3858986829-a, ci(security): give CI jobs read-only tokens instead of the write default #1552). The pinned suite counts and the 373-assertion figure
are gone (T3869180755-f1, ci(test): run the four agent suites #1560 added, and stop the MCP tax failing one #1580). The new tooling-scripts suite is added to
extended-suites; its weight (100) is an estimate, not a CI median.
(T3810295618-a, fix(ci): stop installing ffmpeg in jobs that never use it #1360).
instead of echo, and the guidance names the push/pull_request workflows
rather than "every workflow" (T3813387872-printf-regex,
T3813416696-overstated-guidance, ci(policy): reject CI-skip directives in commit messages #1364).
Config and lint docs
anthropic, bedrock and vertex instead of the 20251124 launch date
(T3816077440, fix(models): correct the Claude Opus 4.5 model ids for Bedrock and Vertex #1375). provider-structure now checks every Claude id in the
file against the model enums.
provider-typed-errors and provider-base-class (T3801758166-1, chore(lint): enforce rule 15 so tests cannot quietly reach into src #1344).
Scripts
**/<dir>/**exclude, which drops every file under a checkout whose pathcontains that directory (T4042344752-guard, fix(docs-api): anchor the typedoc exclude to the project root #1723).
fromfollowed by a specifier on the next line are found, and a//insidea string no longer hides the rest of the line (T3956062753, chore(cleanup): clear the residue the ai-sdk removal left behind #1662). Files in
the repo root and .mts/.cts are scanned too (T3956062775, chore(cleanup): clear the residue the ai-sdk removal left behind #1662).
source tree emits, so a partial or stale build above the 100-file floor
fails (PF-T3927528338, fix(types): stop shipping declarations that reference stripped types #1627). A wildcard export is matched against the whole
pattern, including a
*in a directory component (T3931686738-wildcard-match,fix(scripts): close two holes in the shipped-declaration guard #1632).
replay_toolinstead ofexec, which Codex declares as a custom tool and which raised a Fatal"incompatible payload" error (F1-T4087477953-custom-tool-shape, fix(proxy): harden the Codex stream path and make its failures legible #1783);
reproduced and cleared against codex-cli 0.160.0. --requests counts served
/responses turns, so a 404 probe cannot shut the listener down first
(F2-T4087477985-requests-limit-counts-404s, fix(proxy): harden the Codex stream path and make its failures legible #1783).
behaviour unchanged (T3838161513-b, fix(scripts): bound the git and scanner subprocesses that gate CI and commits #1499).
tagged templates, obj"name", super() and import() are tracked; the
header says it follows calls (T3835058026-residual, PF-T3833252257, chore(scripts): add a symbol-set diff for reviewing code-moving refactors #1448).
configured only when the .env sets one of their variables, the score no
longer divides by the size of the catalog, and the report lists the
configured providers plus one count instead of every missing one
(T3792794348, T3792807279, feat(providers): descriptor single-source-of-truth, unified error classification and retry #1337).
Not done, on purpose
(
items.forEach(handler)); the header now says so.Pre-existing, not touched: test:dynamic fails its five live cases without
provider credentials, identically with config/models.json reverted.
Review before opening
After the commit, an independent read-only reviewer checked all 20 findings against the diff, and a second reviewer tried to refute everything it flagged. Result: all 20 addressed. The review found one thing wrong in this PR's first draft, which is corrected here: the
environmentManagerreport said unconfigured providers' keys are listed in.env.example, which is false for about 63 of the 80 catalog providers. It now points atdocs/getting-started/providers/.Not done
skip-checkstrailer was not added to the CI-skip regex; the finding marked it optional.Verification
pnpm run test:tooling-scripts: 14 passed.pnpm run test:provider-structure: 6 passed.pnpm run test:provider-descriptors: 67 passed.pnpm run check:tools-tests: exit 0. All with an emptyHOMEand no.env.test:tooling-scriptsis added to the extended suites inci.ymlwith a weight of 100. That weight is an estimate, not a CI median, and the suite now runs on the requiredprovider-safety-netpath. If it turns out slow or flaky in CI, lowering its weight or moving it out is a one-line change.persist-credentialsandpermissionsrules; thesemantic-release-validationjob keeps its token on purpose.42b258b4c). The branch was then rebased ontoreleasewith no conflicts, so the pre-push hook (build, mocked provider suite, provider-structure) and CI are the first runs on the rebased code.Yama PR Reviewfails on every PR at the moment (its LiteLLM key is invalid). It is not a required check and is unrelated to this change.