chore(lint): cover tools/ in the lint gate; attach cause in dev-proxy rebuild() (#718) - #745
Merged
Merged
Conversation
… rebuild() (#718) `pnpm lint` (pre-push and CI) linted src/, packages/*/src/ and scripts/ but not tools/, so the dev-proxy supervisor sat outside the gate with two preserve-caught-error findings in BackendManager.rebuild() that nothing reported. - package.json: add "tools/**/*.{js,mjs,cjs}" to `lint`; `lint:fix` now runs the same globs with --fix instead of src/**/*.ts alone, so the hook's "run lint:fix" hint holds for every finding lint reports. tools/ contains only the dev-proxy; experimental probes live in scripts/experiments/, already ignored by eslint.config.js. - dev-proxy.mjs: both rebuild() rethrows attach the caught execSync error as `cause`. The tool handlers serialize err.message alone, so the raw build output never reaches a response (issue #154's invariant). - tests/integration/dev-proxy-startup.test.ts: drive dev_rebuild_and_restart through a failing DEV_PROXY_BUILD_CMD — the payload is { success, error } with a "Build failed:" message, no cause, and the old backend keeps serving. rebuild() had no coverage before. - CONTRIBUTING, AGENTS.md and the adapter guide's checklist state the new glob set (the guide's line already omitted scripts/). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…al output, and never announce an inventory change for a failed build (#718) Review of #745 on the catch blocks the lint gate now covers: - `err.killed || err.signal === 'SIGTERM'` misclassified an execSync ENOBUFS kill (1 MiB default buffer) as a timeout — both arrive with signal SIGTERM and execSync never sets `killed` (probed: timeout → code ETIMEDOUT, overflow → code ENOBUFS). Discriminate on ETIMEDOUT and raise maxBuffer to 64 MiB so an over-chatty build is not killed at all. - The timeout message now carries the sanitized output produced before the kill, the way the failure message already did. - A failed build never reaches restart(), so the running backend's tool inventory is unchanged; the handlers used to send tools/list_changed for it anyway. Build and restart are now sequenced by one helper that notifies only for a restart failure (rebuildAndRestart() is gone; a rejected env update no longer notifies either). - lint:fix is `pnpm run lint --fix`: one glob list, no second copy to drift (the drift this issue is about). - Integration tests: the failed-build case asserts no notification is added past the initial start's own; a new hanging-build case asserts the timeout message and its partial output. Both fail on the previous dev-proxy.mjs. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Closes #718.
What
pnpm lint— and so.husky/pre-pushand all three CI jobs that run it — lintedsrc/,packages/*/src/andscripts/but nottools/. The dev-proxy supervisor undertools/dev-proxy/starts, stops and proxies the backend; it sat outside the gate with twopreserve-caught-errorerrors inBackendManager.rebuild()(lines 366 and 375 on current main — the issue's 355/364 are from an older revision) that nothing reported.package.json—lintgains"tools/**/*.{js,mjs,cjs}". A superset of the issue'stools/dev-proxy/**:tools/contains only the dev-proxy today, so the findings are identical, and a futuretools/directory is covered by default. Experimental probes belong inscripts/experiments/, whicheslint.config.jsalready ignores. No ESLint config change: the**/*.{js,mjs,cjs}block already configures.mjswith node globals, andpreserve-caught-errorcomes fromjs.configs.recommended.lint:fixnow runs the same four globs with--fixinstead ofsrc/**/*.tsalone, so the hook's "runpnpm run lint:fixfor auto-fixes" hint is true for everythinglintreports. (Running it over the widened globs on this tree changes nothing.)tools/dev-proxy/dev-proxy.mjs— bothrebuild()rethrows attach the caughtexecSyncerror ascause. Safe with respect to dev-proxy forwards backend stderr verbatim with naive chunk splitting #154's sanitization: every consumer of that error serializeserr.messagealone (handleDevTool's two catches, the fatal handler), so the raw build output oncausenever reaches a tool response or a log line.tests/integration/dev-proxy-startup.test.ts—rebuild()and its catch blocks had no coverage. A new case setsDEV_PROXY_BUILD_CMDto a failing command and callsdev_rebuild_and_restart: the payload is exactly{ success: false, error: "Build failed: …boom…" }with nocause, and the old backend keeps serving because the build failed before the restart.CONTRIBUTING.md(Dev-Loop Gate + Setup),AGENTS.md, and the adapter guide's checklist (which already omittedscripts/**) state the new glob set and drop the lint/lint:fix asymmetry note.Verification
pnpm exec eslint "tools/**/*.{js,mjs,cjs}": 2 errors before, 0 after.pnpm run lint(now coveringtools/) clean;typecheck:all,changelog:check,check:docsclean.tests/integration/dev-proxy-startup.test.ts: 6/6 (the new case 1.2 s).🤖 Generated with Claude Code
Review round (4aa0f41)
The
/code-reviewon the first commit looked at what the newly-covered catch blocks actually said, and three of its findings were real defects in those lines:err.killed || err.signal === 'SIGTERM'was the timeout test, but execSync never setskilled, and an output-buffer overflow (ENOBUFS, 1 MiB default) arrives with the sameSIGTERM— probed on this box: timeout →code: 'ETIMEDOUT', overflow →code: 'ENOBUFS', bothsignal: 'SIGTERM'. The timeout branch now keys onETIMEDOUT, andmaxBufferis 64 MiB so an over-chatty build is not killed at all.tools/list_changed. The build fails beforerestart()runs, so the backend — and its tool inventory — is untouched, yet both handlers announced a change. Build and restart are now sequenced by one helper (rebuildThenRestart) that notifies only for a restart failure;BackendManager.rebuildAndRestart()is gone, and a rejected env update no longer notifies either.lint:fixispnpm run lint --fix— one glob list instead of a second copy that could drift again (the drift class this issue is about).cause), asserts that no notification is added past the initial start's own, and a second case drives a hanging build throughDEV_PROXY_BUILD_TIMEOUT_MS=1000. Both fail on the previousdev-proxy.mjs.Declined here: making
rebuild()async (execSyncblocks the supervisor for the length of the build) is a concurrency-model change rather than a lint-gate fix — filed as #748. Attaching a slimmedcauseinstead of the raw execSync error would defeatpreserve-caught-error's purpose; the message-only contract is documented at the throw site and locked by the tests.Verification after the round:
tests/integration/dev-proxy-startup.test.ts7/7;lint(tools/ included),typecheck:all,changelog:check,check:docsclean; pre-push gate passed.