chore(devex): nudge when a change pushes a file past 1000 lines - #92275
Conversation
An agent reads a file, not a function, so file length is what decides the token cost of making a change. Function length is a different story and is left alone here: splitting a long function moves the same bytes around instead of removing them, and cyclomatic complexity already covers the case that hurts. Warning about every oversized file would fire on most commits, and a warning that common turns into wallpaper. This warns only when the diff itself pushed a file over the line, which is uncommon and is always something the author can act on in the change they are making. Files that were already oversized get a single note naming the largest one, priced in tokens, so the reading cost stays visible without printing a list. Runs as a soft check in ci:preflight beside the complexity lint, scoped through changed_files so the two cannot drift, and never fails a build. Rename detection is on because reading the destination path at the merge base gives nothing, so without it a relocated file looks newly created and every product migration gets nudged for tidying up. Claude-Session: https://claude.ai/code/session_01LEZFeFHxVkeCbYNnxaWXKo
… hook lint-staged discards stdout and stderr from every task that exits 0, so a warn-only task there prints nothing at all. The .claude/hooks reminder and the quill AGENTS.md reminder have both been silent since they landed, which is a neat trick for the one telling authors to prefer lint-staged rules over Claude Code hooks. The pre-commit hook already runs three warn-only scripts with `|| true` for this exact reason, and says so in a comment. These two now follow that pattern and match their own staged paths instead of relying on a lint-staged glob. Claude-Session: https://claude.ai/code/session_01LEZFeFHxVkeCbYNnxaWXKo
|
😎 Merged directly without going through the merge queue, as the queue was empty and the PR was up to date with the target branch - details. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
🤖 CI report🚨 Trunk lane — universal laneThis PR is assigned to the universal lane. It cannot merge in parallel with other PRs, so it can take longer to merge. Ask dev-ex if you think this is wrong. ✅ Bundle size — no changeUncompressed size of every built Total: 68.18 MiB · no change No file changed by more than 1000 B. Posted automatically by build-bundle-size-report · uncompressed bytes from dist-report ✅ Eager graph — within budgetHow much code each root ships on the eager path — downloaded and parsed before the surface is interactive. Measured from the esbuild output chunks (post-tree-shake, static imports only); lazy
🟢 Largest files eagerly shipped from
|
| Size | File |
|---|---|
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 24.6 KiB | ../node_modules/.pnpm/buffer@6.0.3/node_modules/buffer/index.js |
| 6.3 KiB | ../node_modules/.pnpm/react@18.3.1/node_modules/react/cjs/react.production.min.js |
| 4.5 KiB | ../node_modules/.pnpm/@jspm+core@2.1.0/node_modules/@jspm/core/nodelibs/browser/process.js |
| 3.9 KiB | ../node_modules/.pnpm/scheduler@0.23.2/node_modules/scheduler/cjs/scheduler.production.min.js |
| 1.4 KiB | ../node_modules/.pnpm/base64-js@1.5.1/node_modules/base64-js/index.js |
| 1.3 KiB | src/RootErrorBoundary.tsx |
| 912 B | ../node_modules/.pnpm/ieee754@1.2.1/node_modules/ieee754/index.js |
| 789 B | src/scenes/ChunkLoadErrorBoundary.tsx |
| 762 B | src/index.tsx |
Largest files eagerly shipped from src/scenes/AuthenticatedShell.tsx
| Size | File |
|---|---|
| 307.0 KiB | ../node_modules/.pnpm/posthog-js@1.422.5_@types+react@18.3.27_react@18.3.1/node_modules/posthog-js/dist/rrweb.js |
| 267.7 KiB | ../node_modules/.pnpm/@posthog+icons@0.38.0_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js |
| 263.5 KiB | ../node_modules/.pnpm/posthog-js@1.422.5_@types+react@18.3.27_react@18.3.1/node_modules/posthog-js/dist/module.js |
| 256.1 KiB | src/taxonomy/core-filter-definitions-by-group.json |
| 154.2 KiB | ../node_modules/.pnpm/re2js@0.4.1/node_modules/re2js/build/index.esm.js |
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 104.7 KiB | src/lib/api.ts |
| 95.2 KiB | ../packages/quill/packages/quill/dist/index.js |
| 93.3 KiB | ../node_modules/.pnpm/prosemirror-view@1.40.1/node_modules/prosemirror-view/dist/index.js |
| 90.6 KiB | ../node_modules/.pnpm/@tiptap+core@3.20.6_@tiptap+pm@3.20.6/node_modules/@tiptap/core/dist/index.js |
Posted automatically by check-eager-graph · sizes are eager output bytes (shipped, post-tree-shake) from the esbuild metafile · part of #32479
✅ Toolbar bundle — eager 2.26 MiB within budget
What the toolbar ships to customer pages, measured from the esbuild output (minified, post-tree-shake). The eager set is the entry plus everything statically imported from it — fetched before any feature runs; deferred chunks load lazily. The eager guardrail is 5.72 MiB. Each output file must also stay below 10 MB, where CloudFront stops compressing it. The module boundary is enforced separately by check-toolbar-graph.
| Metric | Size | Δ vs base | Budget |
|---|---|---|---|
| Eager (shipped) entry + static imports |
2.26 MiB · 18 files | no change | ████░░░░░░ 39.6% of 5.72 MiB |
| Deferred (lazy) | 2.11 MiB · 45 files | no change | n/a — loads on demand |
Loader dist/toolbar.js |
1.1 KiB | no change | █░░░░░░░░░ 5.8% of 19.5 KiB |
Largest eagerly-shipped chunks
| Size | File |
|---|---|
| 752.2 KiB | dist/toolbar/toolbar-app-WSMUB6BA.css |
| 590.3 KiB | dist/toolbar/chunk-chunk-PM4Z562B.js |
| 484.7 KiB | dist/toolbar/chunk-chunk-OTPWDCQE.js |
| 134.1 KiB | dist/toolbar/chunk-chunk-H2DYTHK5.js |
| 131.8 KiB | dist/toolbar/chunk-chunk-FDH2IBXT.js |
| 71.3 KiB | dist/toolbar/toolbar-app-2WME6WBO.js |
| 69.0 KiB | dist/toolbar/chunk-chunk-TSAL54PB.js |
| 35.6 KiB | dist/toolbar/chunk-chunk-UQ3KI6QW.js |
| 21.0 KiB | dist/toolbar/chunk-chunk-YKSM6PG7.js |
| 6.8 KiB | dist/toolbar/chunk-chunk-DV7IWQNF.js |
Posted automatically by check-toolbar-size · sizes are toolbar output bytes (shipped, post-tree-shake) from the esbuild metafile
✅ Dist folder size — no change
Total size of the built frontend/dist folder (all assets), compared against the base branch.
Total: 1459.33 MiB · no change
⚠️ Playwright — 1 failed
🎭 Playwright report · View test results →
❌ 1 failed test:
- Hover chart to see tooltip with data point values (chromium)
These issues are not necessarily caused by your changes.
Annoyed by this section? Help fix flakies and failures and it will go green!
✅ ClickHouse migration SQL — none
No ClickHouse migrations in the latest push.
There was a problem hiding this comment.
Pull request overview
Adds a new devex signal to help keep files from silently growing beyond a “readable for agents” threshold by warning only when a diff crosses 1000 lines, plus wiring it into hogli ci:preflight as a soft check. It also moves two previously-silent lint-staged warnings into the Husky pre-commit hook so they actually print.
Changes:
- Introduce
hogli lint:size(warn-only) that reports threshold crossings and a single “already huge” note with an estimated token cost. - Wire the size lint into
hogli ci:preflightas a softDiffCheckso it shows as a warning at push time. - Move
.claude/hooksand quillAGENTS.mdreminder warnings out oflint-stagedinto.husky/pre-commitvia new scripts.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/hogli-commands/hogli_commands/size_lint.py | Implements the size lint command, scope filtering, rename detection, and reporting/output contract. |
| tools/hogli-commands/hogli_commands/tests/test_size_lint.py | Adds pytest coverage for crossings vs pre-existing oversize, rename detection, “single note” behavior, CLI contract, and scope exclusions. |
| tools/hogli-commands/hogli_commands/ci_preflight.py | Adds a new soft DiffCheck for file size linting based on the shared scope. |
| hogli.yaml | Registers the new lint:size command in the hogli manifest. |
| package.json | Removes the warn-only lint-staged tasks that were previously silent due to lint-staged stdout/stderr behavior. |
| .husky/pre-commit | Runs the new warning scripts directly from Husky so the messages are visible to authors. |
| .github/scripts/check-quill-agents-md.sh | New pre-commit warning script for quill source changes without an AGENTS.md update. |
| .github/scripts/check-claude-hooks.sh | New pre-commit warning script when .claude/hooks/ is touched. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Three gaps from bot review, all cases where the check and its caller disagreed about which change they were looking at. Preflight picks changed files relative to `--against`, but ran `hogli lint:size` without it, so the child resolved its own base. On a stacked branch that blamed the current layer for a crossing that happened in the layer below. DiffCheck gained a `takes_base` flag and preflight now forwards the base it already computed. Naming a base is a question about committed history, so the size is now read from the HEAD blob rather than the working tree, falling back to disk for files that are not committed yet. Under the pre-push hook the diff carries only commits, so an uncommitted edit no longer moves the reported number away from what the push contains. Rename detection read only the committed range, so a staged `git mv` had no source to resolve and its destination looked like a new file over the threshold. The index is now a second rename source. A move that is neither committed nor staged stays undetectable, because git has no rename to report until one side is recorded. Also drops a degraded-run message that promised more than it reported. Claude-Session: https://claude.ai/code/session_01LEZFeFHxVkeCbYNnxaWXKo
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
The previous commit tied "which copy of the file to read" to "was a base named", which was wrong. Preflight forwards the base on every run, so an advisory run started reading the HEAD blob as well, and a file grown past the threshold in the working tree reported nothing. Those are two questions, so they are now two flags. `--against` says which base to compare against. `--committed` says to measure the committed file. Preflight passes the base always and `--committed` only under `--strict`, matching the scope it already picks for changed files: a push carries only commits, while an advisory run keeps the working tree in scope and has to measure it. Claude-Session: https://claude.ai/code/session_01LEZFeFHxVkeCbYNnxaWXKo
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…ook doc Preflight defaulted the forwarded base to origin/master, but change detection falls back from origin/master to local master. In a clone without the remote ref the two disagreed, the child could not resolve a merge base, and the check silently reported nothing. The base is now forwarded only when it was given explicitly, so the child repeats the same fallback. AGENTS.md still told contributors that .claude/hooks changes trigger a lint-staged warning, which stopped being true when that warning moved into the pre-commit hook. It now names the right mechanism, keeps settings.json with lint-staged where it belongs, and says why a warn-only check cannot be a lint-staged task. Claude-Session: https://claude.ai/code/session_01LEZFeFHxVkeCbYNnxaWXKo
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Both warn-only scripts piped the staged file list straight into `grep -q`. With a long list, grep exits at the first match, git gets SIGPIPE, and under pipefail the whole pipeline reads as failed, so the warning never printed. A test written against the old scripts reproduces it. They now capture the list into a variable and match with a herestring, so there is no pipeline left to fail. shellcheck.yml gains a step that lints both scripts and runs the new test. It skips when the test file is absent, so an open PR that has not rebased onto this commit does not fail there before its own checks run. `lint:size` now resolves an explicit `--against` itself. Passing files bypasses change detection, which is what normally rejects a bad ref, so a typo used to read as "no base available" and quietly turn crossing detection off. Claude-Session: https://claude.ai/code/session_01LEZFeFHxVkeCbYNnxaWXKo
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
A file renamed in a commit and again in the index produces two separate rename records, so a single lookup landed on the intermediate name. That name does not exist at the merge base, so the file read as new and the move was blamed for a crossing it did not cause. Rename resolution now walks the chain back to the base path, with a guard so a cycle cannot loop. Validating that `--against` resolves was too weak: a ref can resolve and still share no history, which leaves nothing to compare against and turned crossing detection off without saying so. The check is now the merge base itself, so an unrelated commit fails the same way a typo does. Claude-Session: https://claude.ai/code/session_01LEZFeFHxVkeCbYNnxaWXKo
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
`--committed` scoped the measurement but not the file selection, so running the command directly still picked up staged and untracked work. Those paths have no blob at the revision being measured, so they fell back to disk and reported content the caller had asked not to see. Discovery now drops the working tree in committed mode, and a path with no blob counts as zero instead of falling back. Claude-Session: https://claude.ai/code/session_01LEZFeFHxVkeCbYNnxaWXKo
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…ed map Unioning the index and committed rename maps flattened two namespaces into one. A pathname freed by one rename and taken by another connected two unrelated files, so a large file could be compared against a small file's baseline and report a crossing that never happened. The maps are kept apart and applied in order: index path to its name at HEAD, then HEAD path to its name at the merge base. Neither is consulted for the other's namespace, which also drops the chain walk and its cycle guard. Claude-Session: https://claude.ai/code/session_01LEZFeFHxVkeCbYNnxaWXKo
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 480ab4462d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
semgrep's tuple-return-prefer-dataclass caught this: the two maps share a type, so `staged, committed = _rename_maps(base)` could be swapped at the call site and nothing would notice. Swapping them silently resolves paths through the wrong namespace, which is the bug the two maps exist to avoid. They are now fields on a frozen RenameMaps, and the resolution moves onto it as a method, so the hop order lives with the data it applies to. Claude-Session: https://claude.ai/code/session_01LEZFeFHxVkeCbYNnxaWXKo
![stamphog[bot]](https://avatars.githubusercontent.com/in/3000332?s=60&v=4){kind=small}
There was a problem hiding this comment.
Not approved yet — waiting on the conditions below.
Re-add the stamphog label to request another review once you have addressed this.
Gates denied this as T2-never for touching CI/infra tooling (workflows, husky hooks); despite the author being on the owning devex team, changes to CI pipeline behavior warrant explicit human sign-off rather than automated approval.
- 👍 on the PR from chatgpt-codex-connector[bot].
- Modifies .github/workflows/shellcheck.yml and .husky/pre-commit, which is CI/infra-tooling territory that the deny-list flags for mandatory human review regardless of size.
- Gate tier classified this as T2-never (cross-cutting, 668 lines/11 files), explicitly excluded from automated approval.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✗ | matches: infra_cicd |
| size | ✓ | 338L, 8F substantive, 668L/11F incl. docs/generated/snapshots — within ceiling |
| tier | ✗ | classified as T2-never: T2-never (668L, 11F, cross-cutting, chore) |
| stamphog 2.0.0b4 | .stamphog/policy.yml @ 03a257c · reviewed head 03a257c |
| @@ -0,0 +1,244 @@ | |||
| """File-size nudge, scoped to changed files. | |||
|
|
|||
| An agent reads a file, never a function, so file length is what decides how many | |||
There was a problem hiding this comment.
An agent reads a file, never a function
so unnecessary lol
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b86df841f1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if rev is not None: | ||
| result = _git("show", f"{rev}:{path}") | ||
| return 0 if result.returncode != 0 else len(result.stdout.splitlines()) | ||
| return len((REPO_ROOT / path).read_text(errors="replace").splitlines()) |
There was a problem hiding this comment.
When a file has more than 1,000 lines staged but is subsequently edited back below the threshold in the worktree, ordinary lint:size and non-strict preflight select the path because the index changed but always count the on-disk copy here. The command therefore reports no crossing even though committing without restaging records the oversized index blob; inspect the index as well as the worktree so either pending version can trigger the advisory.
Useful? React with 👍 / 👎.
| result = _git("show", f"{rev}:{path}") | ||
| return 0 if result.returncode != 0 else len(result.stdout.splitlines()) |
There was a problem hiding this comment.
When a refactor changes hundreds of in-scope files, strict preflight spawns a separate git show HEAD:<path> process here for every path before it can discard files below the threshold, and oversized files incur another subprocess for their base content. Running this exact pattern over 500 tracked files locally took about nine seconds, adding linear latency to every pre-push check for large diffs; use a single git cat-file --batch process or another batched blob reader.
Useful? React with 👍 / 👎.
Problem
Nothing tells an author when a change turns a readable file into one that costs an agent a large share of its context window just to open. An agent reads a file, not a function, so file length is what decides the token cost of every later change to it.
PR #91305 shipped the cyclomatic complexity signal and named file and module size as the other thing this repo has never measured. This is that half.
The obvious version does not work. Warning about every oversized file in a diff fires on most commits, because touching a big file is normal, and a warning that common becomes wallpaper. That is the same shape that forced #91305 to drop its error tier.
Changes
hogli lint:sizewarns when the diff itself pushed a file past 1000 lines, and points at/splitting-oversized-modules. Only that case is something the author can act on in the change they are making, so it stays uncommon.DiffCheckinci:preflight, so it reaches the author at push time. It never fails a build..claude/hooksand quillAGENTS.mdreminders now actually print. Both were lint-staged tasks, and lint-staged discards stdout and stderr from every task that exits 0, so both have been silent since they landed.AGENTS.mdsaid.claude/hooks/changes trigger a lint-staged warning. It now names the hook, and says why a warn-only check cannot be a lint-staged task.shellcheck.ymlgains a step that lints the two scripts and runs their test. It skips when the test file is absent, so a PR that has not rebased onto this commit is unaffected.Nothing here changes a UI.
Commit-time warnings, before
flowchart LR C{{git commit}} --> H[pre-commit hook body] C --> L[lint-staged tasks] H --> V[visible to author] L --> X[discarded on exit 0] classDef phBlue fill:#1d4aff,stroke:#1d4aff,color:#fff; classDef phYellow fill:#f9bd2b,stroke:#f9bd2b,color:#000; classDef phRed fill:#f54e00,stroke:#f54e00,color:#fff; class H,L phBlue class C,V phYellow class X phRedAfter
flowchart LR C{{git commit}} --> H[pre-commit hook body] C --> L[lint-staged tasks] H --> V[visible to author] L --> F[formatters and fixers only] P{{git push}} --> PF[ci:preflight soft checks] PF --> V classDef phBlue fill:#1d4aff,stroke:#1d4aff,color:#fff; classDef phYellow fill:#f9bd2b,stroke:#f9bd2b,color:#000; class H,L,PF phBlue class C,P,V,F phYellowHow did you test this code?
tools/hogli-commands/hogli_commands/tests/test_size_lint.pybuilds a real temporary git repo per case, so the merge-base and rename plumbing runs for real rather than through mocks.Each group names a regression no existing test catches:
ci:preflightreads a soft check through.check-commit-warnings.test.shcovers the two shell scripts. The large-index case is the point: a match early in a long staged list closes the pipe before git finishes writing it, and underpipefailthe warning has to survive that.Manual checks, run locally:
bin/hogli lint:sizeagainst real oversized files prints the note, and against normal files prints nothing.bin/hogli ci:preflightpasses in both modes on this branch. The size check does not trigger here, because every file this PR touches is out of its scope.How the lint-staged behavior was confirmed
A scratch repo with a task that prints to both streams and exits 0. Default run shows
[COMPLETED]and no output;--verboseshows both lines. Installed version is lint-staged 15.4.3.Not run: CI itself, and the crossing path against a real branch, because no file in scope changed here. The unit tests cover that path.
Automatic notifications
Docs update
None. The behavior is self-describing at the point it fires.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Built with Claude Code. Skills invoked:
/writing-tests,/writing-code-comments,/writing-pr-descriptions,/simplify.The session started as research into file and function size limits for agents, and two proposals died on the way here. Function-length limits went first: reading the functions a line rule would flag showed they are dispatch tables and literal lists, where splitting adds indirection without removing bytes. Ruff's
PLR0915went second, because most of its findings already tripC901and it misses the majority of functions over a hundred lines, so it measures statements rather than the thing that costs tokens.A baseline file was also considered and dropped. Git already holds the previous size of every file, so the check needs no checked-in state to tell a new problem from an old one.
The percentages behind the "fires on most commits" claim came from replaying several hundred squashed PR commits on master and comparing each framing. They are kept out of this description on purpose.
Most of the diff after the first push came from bot review. The recurring defect was namespace confusion in the git plumbing: which base to compare against, which copy of a file to measure, and which rename map applies to which path. Three findings were declined, one of them after building the scenario and confirming git emits no rename records for it.
https://claude.ai/code/session_01LEZFeFHxVkeCbYNnxaWXKo