Repository navigation
ci: adopt one-function-per-file at max_lines = 100 - #1023
Merged
Merged
Conversation
thekevinscott
enabled auto-merge
September 2, 2026 16:52
testing-conventions @v0 wires `unit one-function-per-file` into the static job, and it has been failing `Static checks (typescript)` on main since the tag rolled. The rule is unconfigured here, so it ran at the shipped default of 1. `max_lines` is the threshold above which a module-scope function counts as substantial and must own its file, not a cap on how long a function may be, so a higher number is a looser gate. 100 matches thekevinscott/willfire's starting ceiling: green across every scan root today with zero exemptions, and a number to walk down rather than an end state. Rust stays opted out, as upstream defaults it -- a Rust file is a module, and grouping a type with its impls and free functions is idiomatic.
thekevinbot
force-pushed
the
claude/1017-ofpf-threshold
branch
from
September 2, 2026 16:55
7171065 to
d5dd874
Compare
RED. testing-conventions @v0 runs `unit one-function-per-file` in its static job, so a lane naming no `gates:` is held to it in CI. preflight's ROOT_GATES omits it, so `just preflight` skips it locally and reports green. skip-changelog: internals/ tooling tests, no published package touched
GREEN. Two changes, and the second is what makes the first observable. `one-function-per-file` joins the gate table and the default-gates list, so preflight derives it for the lanes that name no `gates:` and builds the right argv for a lane that names it. Resolving the CLI moves off `uvx` to `npx testing-conventions@latest`. The PyPI wheel is a laggier distribution of the same tool -- 0.0.104 against npm's 0.0.111 -- and has no `one-function-per-file` subcommand at all, so the gate could not run locally however the table was written. A bare `npx testing-conventions` is worse: npm resolves an unversioned name engine-aware, `engines.node` rose to `>=24` at 0.0.87, and a shim on an older node silently picks 0.0.86 and exits 0. Naming the tag makes the local ruleset the one CI enforces, which is the whole point of the recipe. skip-changelog: internals/ tooling, no published package touched
`gates:` is an allowlist, so internals-checks, internals-distcheck and plugins-embeddings each skip the gate until their array names it -- three lanes CI silently holds to a rule nothing local runs. skip-changelog: internals tooling test, no packaged behavior
5 of 8 tasks
internals-checks, internals-distcheck and plugins-embeddings each pass an explicit `gates:` allowlist, so the reusable workflow's default set does not reach them and the gate was not running on those three lanes. skip-changelog: CI config, no packaged behavior
The wheel `uv run --with` fetches lags npm, and it rejects a config key the newer release added -- an unknown key fails the whole config, so the python coverage and mutation arms red locally on a config CI accepts. Running the npm CLI under `uv run` keeps the package venv's interpreter for the suite while resolving the same release CI does. skip-changelog: internals tooling test, no packaged behavior
`uv run --with testing-conventions` resolved both the CLI and the mutation adapter from the wheel, which trails the npm release. A config key the newer release added is an unknown key to the older one, and an unknown key fails the whole config -- so `unit coverage` and `unit mutation` red locally against a config CI accepts, on three python lanes at once. Split the two roles. The npm tag resolves the CLI, matching CI; `--with` stays only to put `testing_conventions` on the interpreter the gate hands to `python3 -m`, the sole form the mutation adapter ships in. skip-changelog: internals tooling, no packaged behavior
`asyncio.to_thread` and `os.getcwd` were reached through `dirsql._async`, so upstream's patch.object resolution reads them as first-party targets. Both modules are already imported in the test files; patching them at their own home is the same object with the same scope. skip-changelog: test-only change, no published artifact affected
Patching `dirsql._async._RustDirSQL` is the hermetic tier's definition -- it is what lets that subdir run with no native build and no real SQLite. The rule redirects a first-party patch to a third-party target, and there is none here: `dirsql._dirsql` is first-party too, and rebinding it after `_async` has bound the name would not take effect. The rule does not describe this seam, so the two suites take a scoped waiver. The repo's exemption count moves from zero to one. AGENTS.md and the testing-gates reference said zero and stays there; they now say what the one entry is and why the rule does not apply to it. skip-changelog: gate config and docs, no published artifact affected
2 tasks done
skip-changelog: docs only
8 tasks done
thekevinbot
disabled auto-merge
September 2, 2026 17:41
…-function-gate internals/checks: teach preflight the one-function-per-file gate
… claude/tackle-1024-nfpp
ci: fix the no-first-party-patch failures in the Python hermetic tier
This was referenced Sep 4, 2026
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
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.
Fixes #1017
Fixes #1024
testing-conventions@v0rolled to78ab3452and added two gates that both fail onmain. This branch now carries the fix for both:unit one-function-per-file(this PR's original scope, ci: adopt the testing-conventions one-function-per-file gate at max_lines = 100 #1017) -- adopt the gate atmax_lines = 100.integration lint/no-first-party-patch(ci: Python integration lint is red -- no-first-party-patch now resolves the patch.object form #1024) -- merged in from ci: fix the no-first-party-patch failures in the Python hermetic tier #1026, which was stacked on this branch and has since merged into it. Its content is part of this diff and its PR body is preserved on ci: fix the no-first-party-patch failures in the Python hermetic tier #1026.Neither gate fix could go green alone: with only the threshold change
Lint integration testsstill failed; with only the patch fixCheck one function per filestill failed. That is why they were stacked and are now one diff.Part 1 --
one-function-per-fileatmax_lines = 100(#1017)unit one-function-per-filenow runs as a step of theStatic checks (<language>)job. The rule was unconfigured here, so it ran at the shipped default of1andmainwas already red on it:python-sdk / Static checks (python)andtypescript-sdk / Static checks (typescript)both failed theCheck one function per filestep (runs33655717589,33655137333).What the knob measures
max_linesis not a cap on function length. It is the threshold above which a module-scope function counts as substantial and must own its file; functions at or under it are trivial and share a file freely. A higher number is a looser gate, and upstream's default of1is the strictest setting available, not a one-line function limit.100 is
thekevinscott/willfire's value, described there as a "starting ceiling for one-function-per-file, to be ratcheted down over time". Ratcheting is tracked in #1020 and deliberately out of scope here -- at the default the TypeScript SDK alone owes 12 file splits, each with its own module, colocated test, and call-site updates.Rust stays opted out, matching upstream's default: a Rust file is a module, and grouping a type with its
implblocks and free functions is how Rust is written. Opting it in is a design call, not a threshold.Config lives in five files rather than one -- each lane that passes its own
config:restates the tables its gates need -- so the threshold is set in all five.Verified
Both exit 0 (a green run prints only the version banner). The three Python configs that did not yet run the gate --
internals/checks,internals/distcheckvia the root default, and the embeddings plugin -- were checked at 100 too and are green; #1019 (merged into this branch) turns the gate on for those lanes and teachesjust preflightto derive it.Part 2 --
no-first-party-patchin the Python hermetic tier (#1024, from #1026)integration lint'sno-first-party-patchrule now resolves thepatch.object(...)object form, which made five previously-invisible patches inpackages/python/tests/integration/hermetic/visible. The five split two ways.Re-rooted: the two stdlib patches
asyncio.to_threadandos.getcwdare effectful stdlib, which the rule permits. They only tripped it because the base was reached throughdirsql._async:Both modules were already imported in their test files, and
async_mod.asynciois theasynciomodule object --patch.objectmutates that same object either way, so the patch is byte-for-byte the same target with the same scope. Nothing is broadened.Waived: the three
_RustDirSQLpatchesPatching
dirsql._async._RustDirSQLis not a shortcut around the integration tier -- it is the tier's definition. AGENTS.md states it under "Test Locations":tests/integration/hermetic/mocks SQLite and the filesystem by patching the_RustDirSQLcore boundary, which is precisely why that subdir needs no native build.The rule's remedy is to redirect a first-party patch at a third-party target. There is none here.
_RustDirSQLisfrom dirsql._dirsql import DirSQL-- first-party as well -- and rebinding it after_asynchas already bound the name would not take effect. Upstream's narrowest supported scoping for a lint rule is per-file, so: two entries, each naming one file and only the one rule, inpackages/python/testing-conventions.toml.AGENTS.md and
agents/reference/testing-gates.mdboth claimed the exemption count was zero; both are updated to name the entry and why the rule does not apply to it.E2E Verification
N/A - CI gate config, repo tooling, and test-file edits only.The combined diff touches no SDK runtime source. Under
packages/, it changespackages/python/testing-conventions.toml,packages/ts/testing-conventions.toml, and two files underpackages/python/tests/integration/hermetic/-- gate config plus test files, both explicitly "not substantial" per AGENTS.md. The rest is.github/workflows/, root/internals/pluginsgate config,internals/checks/repo tooling, and prose (AGENTS.md,agents/reference/). Nothing ships in a published artifact.packages/python/e2e-attestations/<branch>.jsonreceipt: not owed --packages/pythonscoped source (src/) is untouched; thepython-sdk / E2E attestation freshnessgate passes on this diffpackages/ts/e2e-attestations/<branch>.jsonreceipt: not owed --packages/ts/srcandpackages/ts/napiuntouched; thetypescript-sdke2e-verify gate passesinternals/checks/e2e-attestations/claude-1018-preflight-one-function-gate.jsonis present in the diff, written by #1019 for theinternals/checkschanges it brought in;internals-checks / E2E attestation freshnesspasses.Local verification of the merged branch:
just preflight --gate one-function-per-file --gate integration-lintreports 0 failing pairs across all 11 (source, gate) pairs at testing-conventions 0.0.111 -- both previously-red gates, both directions. Python suites frompackages/python:uv run python -m pytest dirsql/ -q108 passed,uv run python -m pytest tests/integration/ -q131 passed,ruff check ./ruff format --check .clean.Changelog / Migrations
<root>/<pkg>/changelog.d/for each changed package (or:skip-changelogtrailer on a commit with reason)<root>/<pkg>/migrations.d/(or: not required -- additive/bugfix only)No fragment is required and none was added. A package-root
testing-conventions.tomlis explicitly exempt inchangelog_gate/decide.py("gate config that ships in no published artifact"), and everything underpackages/<pkg>/tests/is exempt by name -- together that is the entire diff underpackages/. The root,internals/,.github/, and prose files are outside the gate'spackages/+plugins/roots entirely, exceptplugins/dirsql-embedding/testing-conventions.toml, which is exempt on the same grounds. Commits carryskip-changelog:lines regardless.Changelog Checkis green.No migration fragment: no public API changes, no runtime behavior changes.
PARITY.mdis unchanged -- no SDK public API is touched.Related
max_linesdown from 100.