From db870b84caf568a97f710083f5c3d47d31b3f154 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 27 Jul 2026 00:26:52 -0700 Subject: [PATCH 1/7] feat(ci): run the engine package's node:test suite in CI and upload its coverage (#9064) @loopover/engine's own node:test suite (packages/loopover-engine/test/**, 65 files, ~734 tests) had never run in this workflow before -- only locally via `npm run test:ci`'s own `npm run test --workspace @loopover/engine`, and at release time via publish-engine.yml's workflow_dispatch-only validate job. So engine source was graded only by whatever vitest's root test/** suite happened to exercise transitively, even though packages/loopover-engine/src/** is already counted in vitest.config.ts's coverage.include -- a PR touching an engine module with a real node:test suite could satisfy codecov/patch with a thin, duplicate vitest test instead of ever running the real one. Adds an "engine" Codecov flag mirroring the existing rees/control-plane pattern: enable sourceMap in packages/loopover-engine/tsconfig.json, add c8 as a devDependency, and harvest lcov via scripts/engine-coverage.mjs (c8 remaps dist/ hits back to packages/loopover-engine/src/**.ts through the source maps). ci.yml's validate-code job now runs the real test suite (authoritative pass/fail) and then the c8-instrumented re-run (coverage only), uploaded under the new flag. Running the suite in CI for the first time surfaced two real, currently-stale tests -- exactly the failure mode #9064 warns about (a test in a package the gate never runs can silently rot). #9129's duplicate-overlap host-parity split changed a bare linked-issue citation from a blocking finding to the always-non-blocking duplicate_pr_risk_unconfirmed code, and made an all-duplicate blocker set hold ("neutral") rather than fail. iterate-loop.test.ts's and self-review-adapter.test.ts's shared openPr() fixture had no changedFiles, so their "genuine blocking duplicate" scenarios silently stopped blocking; self-review-adapter.test.ts also still asserted the pre-#9129 "failure" conclusion. Fixed the fixture to carry corroborating changedFiles (restoring the intended blocking scenario) and updated the conclusion assertion to the current, correct "neutral"/held behavior. --- .github/workflows/ci.yml | 54 ++ codecov.yml | 10 + package-lock.json | 596 +++++++++++++++++++++++ package.json | 1 + packages/loopover-engine/package.json | 1 + packages/loopover-engine/tsconfig.json | 1 + scripts/engine-coverage.mjs | 85 ++++ test/unit/engine-coverage-script.test.ts | 43 ++ 8 files changed, 791 insertions(+) create mode 100644 scripts/engine-coverage.mjs create mode 100644 test/unit/engine-coverage-script.test.ts diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index fb5d5e1606..56768fe440 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -699,6 +699,60 @@ jobs: override_commit: ${{ github.event.pull_request.head.sha }} override_pr: ${{ github.event.pull_request.number }} fail_ci_if_error: true + # @loopover/engine's own node:test suite (packages/loopover-engine/test/**/*.test.ts) has never run + # in this workflow before (#9064) -- only locally via `npm run test:ci`'s own `npm run test + # --workspace @loopover/engine`, and at release time via publish-engine.yml's workflow_dispatch-only + # validate job. So engine source was graded only by whatever vitest's root test/** suite happened to + # exercise, even though vitest.config.ts's coverage.include already counts packages/loopover-engine/ + # src/** toward the total -- a PR touching, say, an engine module with a real node:test suite could + # satisfy codecov/patch with a thin, duplicate vitest test instead of ever running the real one. + # "Build engine package" above already produced dist/; this runs the package's real test script + # (which rebuilds dist/dist-test itself, same tsc-incremental cost as REES/control-plane's own + # install+test steps above). + - name: Engine package tests + if: ${{ github.event_name == 'push' || needs.changes.outputs.engine == 'true' }} + run: npm run test --workspace @loopover/engine + # engine-coverage.mjs re-runs the suite (dist-test/**/*.test.js, left in place by the step above) + # under c8, remapped through packages/loopover-engine/tsconfig.json's "sourceMap": true back to + # packages/loopover-engine/src/**.ts paths -- the same "node:test suite invisible to vitest" shape + # and c8 recipe as rees:coverage/control-plane:coverage above. The authoritative pass/fail for the + # suite is the uninstrumented step above; this harvest only needs to emit lcov. + - name: Engine coverage + if: ${{ github.event_name == 'push' || needs.changes.outputs.engine == 'true' }} + run: npm run engine:coverage + - name: Verify engine coverage report exists + if: ${{ github.event_name == 'push' || needs.changes.outputs.engine == 'true' }} + run: | + if [ ! -s packages/loopover-engine/coverage/lcov.info ]; then + echo "::error title=Engine coverage::packages/loopover-engine/coverage/lcov.info is missing or empty" + exit 1 + fi + # Separate `engine` flag so codecov/patch actually gates packages/loopover-engine/src/** changes + # against the real node:test suite's coverage, not just whatever the root vitest suite happens to + # exercise. Same trusted vs. fork-tokenless split and override_* handling as REES/control-plane above. + - name: Upload engine coverage to Codecov + if: ${{ (github.event_name == 'push' || needs.changes.outputs.engine == 'true') && github.event.pull_request.head.repo.fork != true }} + uses: codecov/codecov-action@fb8b3582c8e4def4969c97caa2f19720cb33a72f # v7.0.0 + with: + token: ${{ secrets.CODECOV_TOKEN }} + files: ./packages/loopover-engine/coverage/lcov.info + flags: engine + disable_search: true + override_branch: ${{ github.event_name == 'pull_request' && github.event.pull_request.head.ref || github.ref_name }} + override_commit: ${{ github.event_name == 'pull_request' && github.event.pull_request.head.sha || github.sha }} + override_pr: ${{ github.event_name == 'pull_request' && github.event.pull_request.number || '' }} + fail_ci_if_error: true + - name: Upload engine coverage to Codecov (fork PR tokenless) + if: ${{ (github.event_name == 'push' || needs.changes.outputs.engine == 'true') && github.event.pull_request.head.repo.fork == true }} + uses: codecov/codecov-action@fb8b3582c8e4def4969c97caa2f19720cb33a72f # v7.0.0 + with: + files: ./packages/loopover-engine/coverage/lcov.info + flags: engine + disable_search: true + override_branch: ${{ github.event.pull_request.head.repo.owner.login }}:${{ github.event.pull_request.head.ref }} + override_commit: ${{ github.event.pull_request.head.sha }} + override_pr: ${{ github.event.pull_request.number }} + fail_ci_if_error: true - name: OpenAPI drift check if: ${{ github.event_name == 'push' || needs.changes.outputs.ui == 'true' || needs.changes.outputs.uiContract == 'true' }} run: npm run ui:openapi:check diff --git a/codecov.yml b/codecov.yml index 7a38b3570f..1aa928209a 100644 --- a/codecov.yml +++ b/codecov.yml @@ -49,6 +49,16 @@ flags: paths: - control-plane/ carryforward: true + # @loopover/engine's real behavior suite is its own node:test suite (packages/loopover-engine/test/**), + # not vitest's root test/** -- that suite was invisible to Codecov entirely until now (#9064), even + # though packages/loopover-engine/src/** is already in vitest's coverage.include and counted by the + # unflagged `backend` upload for whatever a root test/** import happens to exercise. This flag is + # additive to that: both reports' hits are unioned for the same lines, so a PR touching engine source + # now gets credit from the suite that actually behavior-tests it, not just an incidental vitest import. + engine: + paths: + - packages/loopover-engine/src/ + carryforward: true comment: layout: "condensed_header, diff, flags, files" diff --git a/package-lock.json b/package-lock.json index a3c15e5574..058e32c0a0 100644 --- a/package-lock.json +++ b/package-lock.json @@ -4022,6 +4022,67 @@ "integrity": "sha512-UmeW7z4LfctwoQ5wkhVzgq8tXkreED2xZGpX+Bg+zA+WJFZCT6c062AfCK/Dfk81xZnnwdhJCUMkitihRaoC2Q==", "license": "MIT" }, + "node_modules/@isaacs/cliui": { + "version": "8.0.2", + "resolved": "https://registry.npmjs.org/@isaacs/cliui/-/cliui-8.0.2.tgz", + "integrity": "sha512-O8jcjabXaleOG9DQ0+ARXWZBTfnP4WNAqzuiJK7ll44AmxGKv/J2M4TPjxjY3znBCfvBXFzucm1twdyFybFqEA==", + "dev": true, + "license": "ISC", + "dependencies": { + "string-width": "^5.1.2", + "string-width-cjs": "npm:string-width@^4.2.0", + "strip-ansi": "^7.0.1", + "strip-ansi-cjs": "npm:strip-ansi@^6.0.1", + "wrap-ansi": "^8.1.0", + "wrap-ansi-cjs": "npm:wrap-ansi@^7.0.0" + }, + "engines": { + "node": ">=12" + } + }, + "node_modules/@isaacs/cliui/node_modules/emoji-regex": { + "version": "9.2.2", + "resolved": "https://registry.npmjs.org/emoji-regex/-/emoji-regex-9.2.2.tgz", + "integrity": "sha512-L18DaJsXSUk2+42pv8mLs5jJT2hqFkFE4j21wOmgbUqsZ2hL72NsUU785g9RXgo3s0ZNgVl42TiHp3ZtOv/Vyg==", + "dev": true, + "license": "MIT" + }, + "node_modules/@isaacs/cliui/node_modules/string-width": { + "version": "5.1.2", + "resolved": "https://registry.npmjs.org/string-width/-/string-width-5.1.2.tgz", + "integrity": "sha512-HnLOCR3vjcY8beoNLtcjZ5/nxn2afmME6lhrDrebokqMap+XbeW8n9TXpPDOqdGK5qcI3oT0GKTW6wC7EMiVqA==", + "dev": true, + "license": "MIT", + "dependencies": { + "eastasianwidth": "^0.2.0", + "emoji-regex": "^9.2.2", + "strip-ansi": "^7.0.1" + }, + "engines": { + "node": ">=12" + }, + "funding": { + "url": "https://github.com/sponsors/sindresorhus" + } + }, + "node_modules/@isaacs/cliui/node_modules/wrap-ansi": { + "version": "8.1.0", + "resolved": "https://registry.npmjs.org/wrap-ansi/-/wrap-ansi-8.1.0.tgz", + "integrity": "sha512-si7QWI6zUMq56bESFvagtmzMdGOtoxfR+Sez11Mobfc7tm+VkUckk9bW2UeffTGVUbOksxmSw0AA2gs8g71NCQ==", + "dev": true, + "license": "MIT", + "dependencies": { + "ansi-styles": "^6.1.0", + "string-width": "^5.0.1", + "strip-ansi": "^7.0.1" + }, + "engines": { + "node": ">=12" + }, + "funding": { + "url": "https://github.com/chalk/wrap-ansi?sponsor=1" + } + }, "node_modules/@isaacs/fs-minipass": { "version": "4.0.1", "resolved": "https://registry.npmjs.org/@isaacs/fs-minipass/-/fs-minipass-4.0.1.tgz", @@ -4035,6 +4096,16 @@ "node": ">=18.0.0" } }, + "node_modules/@istanbuljs/schema": { + "version": "0.1.6", + "resolved": "https://registry.npmjs.org/@istanbuljs/schema/-/schema-0.1.6.tgz", + "integrity": "sha512-+Sg6GCR/wy1oSmQDFq4LQDAhm3ETKnorxN+y5nbLULOR3P0c14f2Wurzj3/xqPXtasLFfHd5iRFQ7AJt4KH2cw==", + "dev": true, + "license": "MIT", + "engines": { + "node": ">=8" + } + }, "node_modules/@jridgewell/gen-mapping": { "version": "0.3.13", "resolved": "https://registry.npmjs.org/@jridgewell/gen-mapping/-/gen-mapping-0.3.13.tgz", @@ -4862,6 +4933,17 @@ "integrity": "sha512-v4ARvrip4qBCImOE5rmPUylOEK4iiED9ZyKjcvzuezqMaiRASCHKcRIuvvxL/twvLpkfnEODCOJp5dM4eZilxQ==", "license": "MIT" }, + "node_modules/@pkgjs/parseargs": { + "version": "0.11.0", + "resolved": "https://registry.npmjs.org/@pkgjs/parseargs/-/parseargs-0.11.0.tgz", + "integrity": "sha512-+1VkjdD0QBLPodGrJUeqarH8VAIvQODIbwh9XpP5Syisf7YoQgsJKPNFoqqLQlu+VQ/tVSshMR6loPMn8U+dPg==", + "dev": true, + "license": "MIT", + "optional": true, + "engines": { + "node": ">=14" + } + }, "node_modules/@pkgr/core": { "version": "0.3.6", "resolved": "https://registry.npmjs.org/@pkgr/core/-/core-0.3.6.tgz", @@ -8738,6 +8820,13 @@ "@types/unist": "*" } }, + "node_modules/@types/istanbul-lib-coverage": { + "version": "2.0.6", + "resolved": "https://registry.npmjs.org/@types/istanbul-lib-coverage/-/istanbul-lib-coverage-2.0.6.tgz", + "integrity": "sha512-2QF/t/auWm0lsy8XtKVPG19v3sSOQlJe/YHZgfjb/KBBHOGSV+J2q/S671rcq9uTBrLAXmZpqJiaQbMT+zNU1w==", + "dev": true, + "license": "MIT" + }, "node_modules/@types/jsesc": { "version": "2.5.1", "resolved": "https://registry.npmjs.org/@types/jsesc/-/jsesc-2.5.1.tgz", @@ -10710,6 +10799,163 @@ "node": ">= 0.8" } }, + "node_modules/c8": { + "version": "10.1.3", + "resolved": "https://registry.npmjs.org/c8/-/c8-10.1.3.tgz", + "integrity": "sha512-LvcyrOAaOnrrlMpW22n690PUvxiq4Uf9WMhQwNJ9vgagkL/ph1+D4uvjvDA5XCbykrc0sx+ay6pVi9YZ1GnhyA==", + "dev": true, + "license": "ISC", + "dependencies": { + "@bcoe/v8-coverage": "^1.0.1", + "@istanbuljs/schema": "^0.1.3", + "find-up": "^5.0.0", + "foreground-child": "^3.1.1", + "istanbul-lib-coverage": "^3.2.0", + "istanbul-lib-report": "^3.0.1", + "istanbul-reports": "^3.1.6", + "test-exclude": "^7.0.1", + "v8-to-istanbul": "^9.0.0", + "yargs": "^17.7.2", + "yargs-parser": "^21.1.1" + }, + "bin": { + "c8": "bin/c8.js" + }, + "engines": { + "node": ">=18" + }, + "peerDependencies": { + "monocart-coverage-reports": "^2" + }, + "peerDependenciesMeta": { + "monocart-coverage-reports": { + "optional": true + } + } + }, + "node_modules/c8/node_modules/ansi-regex": { + "version": "5.0.1", + "resolved": "https://registry.npmjs.org/ansi-regex/-/ansi-regex-5.0.1.tgz", + "integrity": "sha512-quJQXlTSUGL2LH9SUXo8VwsY4soanhgo6LNSm84E1LBcE8s3O0wpdiRzyR9z/ZZJMlMWv37qOOb9pdJlMUEKFQ==", + "dev": true, + "license": "MIT", + "engines": { + "node": ">=8" + } + }, + "node_modules/c8/node_modules/ansi-styles": { + "version": "4.3.0", + "resolved": "https://registry.npmjs.org/ansi-styles/-/ansi-styles-4.3.0.tgz", + "integrity": "sha512-zbB9rCJAT1rbjiVDb2hqKFHNYLxgtk8NURxZ3IZwD3F6NtxbXZQCnnSi1Lkx+IDohdPlFp222wVALIheZJQSEg==", + "dev": true, + "license": "MIT", + "dependencies": { + "color-convert": "^2.0.1" + }, + "engines": { + "node": ">=8" + }, + "funding": { + "url": "https://github.com/chalk/ansi-styles?sponsor=1" + } + }, + "node_modules/c8/node_modules/cliui": { + "version": "8.0.1", + "resolved": "https://registry.npmjs.org/cliui/-/cliui-8.0.1.tgz", + "integrity": "sha512-BSeNnyus75C4//NQ9gQt1/csTXyo/8Sb+afLAkzAptFuMsod9HFokGNudZpi/oQV73hnVK+sR+5PVRMd+Dr7YQ==", + "dev": true, + "license": "ISC", + "dependencies": { + "string-width": "^4.2.0", + "strip-ansi": "^6.0.1", + "wrap-ansi": "^7.0.0" + }, + "engines": { + "node": ">=12" + } + }, + "node_modules/c8/node_modules/emoji-regex": { + "version": "8.0.0", + "resolved": "https://registry.npmjs.org/emoji-regex/-/emoji-regex-8.0.0.tgz", + "integrity": "sha512-MSjYzcWNOA0ewAHpz0MxpYFvwg6yjy1NG3xteoqz644VCo/RPgnr1/GGt+ic3iJTzQ8Eu3TdM14SawnVUmGE6A==", + "dev": true, + "license": "MIT" + }, + "node_modules/c8/node_modules/string-width": { + "version": "4.2.3", + "resolved": "https://registry.npmjs.org/string-width/-/string-width-4.2.3.tgz", + "integrity": "sha512-wKyQRQpjJ0sIp62ErSZdGsjMJWsap5oRNihHhu6G7JVO/9jIB6UyevL+tXuOqrng8j/cxKTWyWUwvSTriiZz/g==", + "dev": true, + "license": "MIT", + "dependencies": { + "emoji-regex": "^8.0.0", + "is-fullwidth-code-point": "^3.0.0", + "strip-ansi": "^6.0.1" + }, + "engines": { + "node": ">=8" + } + }, + "node_modules/c8/node_modules/strip-ansi": { + "version": "6.0.1", + "resolved": "https://registry.npmjs.org/strip-ansi/-/strip-ansi-6.0.1.tgz", + "integrity": "sha512-Y38VPSHcqkFrCpFnQ9vuSXmquuv5oXOKpGeT6aGrr3o3Gc9AlVa6JBfUSOCnbxGGZF+/0ooI7KrPuUSztUdU5A==", + "dev": true, + "license": "MIT", + "dependencies": { + "ansi-regex": "^5.0.1" + }, + "engines": { + "node": ">=8" + } + }, + "node_modules/c8/node_modules/wrap-ansi": { + "version": "7.0.0", + "resolved": "https://registry.npmjs.org/wrap-ansi/-/wrap-ansi-7.0.0.tgz", + "integrity": "sha512-YVGIj2kamLSTxw6NsZjoBxfSwsn0ycdesmc4p+Q21c5zPuZ1pl+NfxVdxPtdHvmNVOQ6XSYG4AUtyt/Fi7D16Q==", + "dev": true, + "license": "MIT", + "dependencies": { + "ansi-styles": "^4.0.0", + "string-width": "^4.1.0", + "strip-ansi": "^6.0.0" + }, + "engines": { + "node": ">=10" + }, + "funding": { + "url": "https://github.com/chalk/wrap-ansi?sponsor=1" + } + }, + "node_modules/c8/node_modules/yargs": { + "version": "17.7.3", + "resolved": "https://registry.npmjs.org/yargs/-/yargs-17.7.3.tgz", + "integrity": "sha512-GZtjxm/J/4TSxuL3FNYjCmLktBTnIw/rVmKSIyKeYAZpmJB2ig9VauCC5xsa82GNKVKDAqpOn3KVzNt0zmrU0g==", + "dev": true, + "license": "MIT", + "dependencies": { + "cliui": "^8.0.1", + "escalade": "^3.1.1", + "get-caller-file": "^2.0.5", + "require-directory": "^2.1.1", + "string-width": "^4.2.3", + "y18n": "^5.0.5", + "yargs-parser": "^21.1.1" + }, + "engines": { + "node": ">=12" + } + }, + "node_modules/c8/node_modules/yargs-parser": { + "version": "21.1.1", + "resolved": "https://registry.npmjs.org/yargs-parser/-/yargs-parser-21.1.1.tgz", + "integrity": "sha512-tVpsJW7DdjecAiFpbIB1e3qxIQsE6NoPc5/eTdrbbIC4h0LVsWhnoa3g+m2HclBIujHzsxZ4VJVA+GUuc2/LBw==", + "dev": true, + "license": "ISC", + "engines": { + "node": ">=12" + } + }, "node_modules/call-bind-apply-helpers": { "version": "1.0.2", "resolved": "https://registry.npmjs.org/call-bind-apply-helpers/-/call-bind-apply-helpers-1.0.2.tgz", @@ -11832,6 +12078,13 @@ "node": ">= 0.4" } }, + "node_modules/eastasianwidth": { + "version": "0.2.0", + "resolved": "https://registry.npmjs.org/eastasianwidth/-/eastasianwidth-0.2.0.tgz", + "integrity": "sha512-I88TYZWc9XiYHRQ4/3c5rjjfgkjhLyW2luGIheGERbNQ6OY7yTybanSpDXZa8y7VUP9YmDcYa+eyq4ca7iLqWA==", + "dev": true, + "license": "MIT" + }, "node_modules/ee-first": { "version": "1.1.1", "resolved": "https://registry.npmjs.org/ee-first/-/ee-first-1.1.1.tgz", @@ -13010,6 +13263,23 @@ "tabbable": "^6.4.0" } }, + "node_modules/foreground-child": { + "version": "3.3.1", + "resolved": "https://registry.npmjs.org/foreground-child/-/foreground-child-3.3.1.tgz", + "integrity": "sha512-gIXjKqtFuWEgzFRJA9WCQeSJLZDjgJUOMCMzxtvFq/37KojM1BFGufqsCy0r4qSQmYLsZYMeyRqzIWOMup03sw==", + "dev": true, + "license": "ISC", + "dependencies": { + "cross-spawn": "^7.0.6", + "signal-exit": "^4.0.1" + }, + "engines": { + "node": ">=14" + }, + "funding": { + "url": "https://github.com/sponsors/isaacs" + } + }, "node_modules/form-data": { "version": "4.0.6", "resolved": "https://registry.npmjs.org/form-data/-/form-data-4.0.6.tgz", @@ -13603,6 +13873,28 @@ "integrity": "sha512-IaOQ9puYtjrkq7Y0Ygl9KDZnrf/aiUJYUpVf89y8kyaxbRG7Y1SrX/jaumrv81vc61+kiMempujsM3Yw7w5qcw==", "license": "ISC" }, + "node_modules/glob": { + "version": "10.5.0", + "resolved": "https://registry.npmjs.org/glob/-/glob-10.5.0.tgz", + "integrity": "sha512-DfXN8DfhJ7NH3Oe7cFmu3NCu1wKbkReJ8TorzSAFbSKrlNaQSKfIzqYqVY8zlbs2NLBbWpRiU52GX2PbaBVNkg==", + "deprecated": "Old versions of glob are not supported, and contain widely publicized security vulnerabilities, which have been fixed in the current version. Please update. Support for old versions may be purchased (at exorbitant rates) by contacting i@izs.me", + "dev": true, + "license": "ISC", + "dependencies": { + "foreground-child": "^3.1.0", + "jackspeak": "^3.1.2", + "minimatch": "^9.0.4", + "minipass": "^7.1.2", + "package-json-from-dist": "^1.0.0", + "path-scurry": "^1.11.1" + }, + "bin": { + "glob": "dist/esm/bin.mjs" + }, + "funding": { + "url": "https://github.com/sponsors/isaacs" + } + }, "node_modules/glob-parent": { "version": "6.0.2", "resolved": "https://registry.npmjs.org/glob-parent/-/glob-parent-6.0.2.tgz", @@ -13616,6 +13908,32 @@ "node": ">=10.13.0" } }, + "node_modules/glob/node_modules/brace-expansion": { + "version": "2.1.2", + "resolved": "https://registry.npmjs.org/brace-expansion/-/brace-expansion-2.1.2.tgz", + "integrity": "sha512-w5JZcKgdhDOgOwm8H+KgbosopHMuGcl6qbulwjtz3SM7I7P3yW1eAjzMPLrIE+NQ9vjgANKHWeMHnrT0OXW1oA==", + "dev": true, + "license": "MIT", + "dependencies": { + "balanced-match": "^1.0.0" + } + }, + "node_modules/glob/node_modules/minimatch": { + "version": "9.0.9", + "resolved": "https://registry.npmjs.org/minimatch/-/minimatch-9.0.9.tgz", + "integrity": "sha512-OBwBN9AL4dqmETlpS2zasx+vTeWclWzkblfZk7KTA5j3jeOONz/tRCnZomUyvNg83wL5Zv9Ss6HMJXAgL8R2Yg==", + "dev": true, + "license": "ISC", + "dependencies": { + "brace-expansion": "^2.0.2" + }, + "engines": { + "node": ">=16 || 14 >=14.17" + }, + "funding": { + "url": "https://github.com/sponsors/isaacs" + } + }, "node_modules/globals": { "version": "15.15.0", "resolved": "https://registry.npmjs.org/globals/-/globals-15.15.0.tgz", @@ -14725,6 +15043,22 @@ "node": ">=8" } }, + "node_modules/jackspeak": { + "version": "3.4.3", + "resolved": "https://registry.npmjs.org/jackspeak/-/jackspeak-3.4.3.tgz", + "integrity": "sha512-OGlZQpz2yfahA/Rd1Y8Cd9SIEsqvXkLVoSw/cgwhnhFMDbsQFeZYoJJ7bIZBS9BcamUW96asq/npPWugM+RQBw==", + "dev": true, + "license": "BlueOak-1.0.0", + "dependencies": { + "@isaacs/cliui": "^8.0.2" + }, + "funding": { + "url": "https://github.com/sponsors/isaacs" + }, + "optionalDependencies": { + "@pkgjs/parseargs": "^0.11.0" + } + }, "node_modules/jiti": { "version": "2.7.0", "resolved": "https://registry.npmjs.org/jiti/-/jiti-2.7.0.tgz", @@ -17218,6 +17552,13 @@ "node": ">= 14" } }, + "node_modules/package-json-from-dist": { + "version": "1.0.1", + "resolved": "https://registry.npmjs.org/package-json-from-dist/-/package-json-from-dist-1.0.1.tgz", + "integrity": "sha512-UEZIS3/by4OC8vL3P2dTXRETpebLI2NiI5vIrjaD/5UtrkFX/tNbwjTSRAGC/+7CAo2pIcBaRgWmcBBHcsaCIw==", + "dev": true, + "license": "BlueOak-1.0.0" + }, "node_modules/parent-module": { "version": "1.0.1", "resolved": "https://registry.npmjs.org/parent-module/-/parent-module-1.0.1.tgz", @@ -17344,6 +17685,30 @@ "dev": true, "license": "MIT" }, + "node_modules/path-scurry": { + "version": "1.11.1", + "resolved": "https://registry.npmjs.org/path-scurry/-/path-scurry-1.11.1.tgz", + "integrity": "sha512-Xa4Nw17FS9ApQFJ9umLiJS4orGjm7ZzwUrwamcGQuHSzDyth9boKDaycYdDcZDuqYATXw4HFXgaqWTctW/v1HA==", + "dev": true, + "license": "BlueOak-1.0.0", + "dependencies": { + "lru-cache": "^10.2.0", + "minipass": "^5.0.0 || ^6.0.2 || ^7.0.0" + }, + "engines": { + "node": ">=16 || 14 >=14.18" + }, + "funding": { + "url": "https://github.com/sponsors/isaacs" + } + }, + "node_modules/path-scurry/node_modules/lru-cache": { + "version": "10.4.3", + "resolved": "https://registry.npmjs.org/lru-cache/-/lru-cache-10.4.3.tgz", + "integrity": "sha512-JNAzZcXrCt42VGLuYz0zfAzDfAvJWW6AfYlDBQyDV5DClI2m5sAmK+OIO7s59XfsRsWHp02jAJrRadPRGTt6SQ==", + "dev": true, + "license": "ISC" + }, "node_modules/path-to-regexp": { "version": "6.3.0", "resolved": "https://registry.npmjs.org/path-to-regexp/-/path-to-regexp-6.3.0.tgz", @@ -19447,6 +19812,52 @@ "url": "https://github.com/sponsors/sindresorhus" } }, + "node_modules/string-width-cjs": { + "name": "string-width", + "version": "4.2.3", + "resolved": "https://registry.npmjs.org/string-width/-/string-width-4.2.3.tgz", + "integrity": "sha512-wKyQRQpjJ0sIp62ErSZdGsjMJWsap5oRNihHhu6G7JVO/9jIB6UyevL+tXuOqrng8j/cxKTWyWUwvSTriiZz/g==", + "dev": true, + "license": "MIT", + "dependencies": { + "emoji-regex": "^8.0.0", + "is-fullwidth-code-point": "^3.0.0", + "strip-ansi": "^6.0.1" + }, + "engines": { + "node": ">=8" + } + }, + "node_modules/string-width-cjs/node_modules/ansi-regex": { + "version": "5.0.1", + "resolved": "https://registry.npmjs.org/ansi-regex/-/ansi-regex-5.0.1.tgz", + "integrity": "sha512-quJQXlTSUGL2LH9SUXo8VwsY4soanhgo6LNSm84E1LBcE8s3O0wpdiRzyR9z/ZZJMlMWv37qOOb9pdJlMUEKFQ==", + "dev": true, + "license": "MIT", + "engines": { + "node": ">=8" + } + }, + "node_modules/string-width-cjs/node_modules/emoji-regex": { + "version": "8.0.0", + "resolved": "https://registry.npmjs.org/emoji-regex/-/emoji-regex-8.0.0.tgz", + "integrity": "sha512-MSjYzcWNOA0ewAHpz0MxpYFvwg6yjy1NG3xteoqz644VCo/RPgnr1/GGt+ic3iJTzQ8Eu3TdM14SawnVUmGE6A==", + "dev": true, + "license": "MIT" + }, + "node_modules/string-width-cjs/node_modules/strip-ansi": { + "version": "6.0.1", + "resolved": "https://registry.npmjs.org/strip-ansi/-/strip-ansi-6.0.1.tgz", + "integrity": "sha512-Y38VPSHcqkFrCpFnQ9vuSXmquuv5oXOKpGeT6aGrr3o3Gc9AlVa6JBfUSOCnbxGGZF+/0ooI7KrPuUSztUdU5A==", + "dev": true, + "license": "MIT", + "dependencies": { + "ansi-regex": "^5.0.1" + }, + "engines": { + "node": ">=8" + } + }, "node_modules/stringify-entities": { "version": "4.0.4", "resolved": "https://registry.npmjs.org/stringify-entities/-/stringify-entities-4.0.4.tgz", @@ -19494,6 +19905,30 @@ "url": "https://github.com/chalk/strip-ansi?sponsor=1" } }, + "node_modules/strip-ansi-cjs": { + "name": "strip-ansi", + "version": "6.0.1", + "resolved": "https://registry.npmjs.org/strip-ansi/-/strip-ansi-6.0.1.tgz", + "integrity": "sha512-Y38VPSHcqkFrCpFnQ9vuSXmquuv5oXOKpGeT6aGrr3o3Gc9AlVa6JBfUSOCnbxGGZF+/0ooI7KrPuUSztUdU5A==", + "dev": true, + "license": "MIT", + "dependencies": { + "ansi-regex": "^5.0.1" + }, + "engines": { + "node": ">=8" + } + }, + "node_modules/strip-ansi-cjs/node_modules/ansi-regex": { + "version": "5.0.1", + "resolved": "https://registry.npmjs.org/ansi-regex/-/ansi-regex-5.0.1.tgz", + "integrity": "sha512-quJQXlTSUGL2LH9SUXo8VwsY4soanhgo6LNSm84E1LBcE8s3O0wpdiRzyR9z/ZZJMlMWv37qOOb9pdJlMUEKFQ==", + "dev": true, + "license": "MIT", + "engines": { + "node": ">=8" + } + }, "node_modules/strip-final-newline": { "version": "4.0.0", "resolved": "https://registry.npmjs.org/strip-final-newline/-/strip-final-newline-4.0.0.tgz", @@ -19715,6 +20150,60 @@ "streamx": "^2.12.5" } }, + "node_modules/test-exclude": { + "version": "7.0.2", + "resolved": "https://registry.npmjs.org/test-exclude/-/test-exclude-7.0.2.tgz", + "integrity": "sha512-u9E6A+ZDYdp7a4WnarkXPZOx8Ilz46+kby6p1yZ8zsGTz9gYa6FIS7lj2oezzNKmtdyyJNNmmXDppga5GB7kSw==", + "dev": true, + "license": "ISC", + "dependencies": { + "@istanbuljs/schema": "^0.1.2", + "glob": "^10.4.1", + "minimatch": "^10.2.2" + }, + "engines": { + "node": ">=18" + } + }, + "node_modules/test-exclude/node_modules/balanced-match": { + "version": "4.0.4", + "resolved": "https://registry.npmjs.org/balanced-match/-/balanced-match-4.0.4.tgz", + "integrity": "sha512-BLrgEcRTwX2o6gGxGOCNyMvGSp35YofuYzw9h1IMTRmKqttAZZVU67bdb9Pr2vUHA8+j3i2tJfjO6C6+4myGTA==", + "dev": true, + "license": "MIT", + "engines": { + "node": "18 || 20 || >=22" + } + }, + "node_modules/test-exclude/node_modules/brace-expansion": { + "version": "5.0.8", + "resolved": "https://registry.npmjs.org/brace-expansion/-/brace-expansion-5.0.8.tgz", + "integrity": "sha512-JZyDyq3D4AUifKTPOB7DELf6XsB3WdPuNxCtob1vFXPsSXhdAiHBWJ/tJ8HAc9aH84BK+5JFZLNkJKx3G9kzQg==", + "dev": true, + "license": "MIT", + "dependencies": { + "balanced-match": "^4.0.2" + }, + "engines": { + "node": "20 || >=22" + } + }, + "node_modules/test-exclude/node_modules/minimatch": { + "version": "10.2.5", + "resolved": "https://registry.npmjs.org/minimatch/-/minimatch-10.2.5.tgz", + "integrity": "sha512-MULkVLfKGYDFYejP07QOurDLLQpcjk7Fw+7jXS2R2czRQzR56yHRveU5NDJEOviH+hETZKSkIk5c+T23GjFUMg==", + "dev": true, + "license": "BlueOak-1.0.0", + "dependencies": { + "brace-expansion": "^5.0.5" + }, + "engines": { + "node": "18 || 20 || >=22" + }, + "funding": { + "url": "https://github.com/sponsors/isaacs" + } + }, "node_modules/text-decoder": { "version": "1.2.7", "resolved": "https://registry.npmjs.org/text-decoder/-/text-decoder-1.2.7.tgz", @@ -20443,6 +20932,32 @@ "dev": true, "license": "MIT" }, + "node_modules/v8-to-istanbul": { + "version": "9.3.0", + "resolved": "https://registry.npmjs.org/v8-to-istanbul/-/v8-to-istanbul-9.3.0.tgz", + "integrity": "sha512-kiGUalWN+rgBJ/1OHZsBtU4rXZOfj/7rKQxULKlIzwzQSvMJUUNgPwJEEh7gU6xEVxC0ahoOBvN2YI8GH6FNgA==", + "dev": true, + "license": "ISC", + "dependencies": { + "@jridgewell/trace-mapping": "^0.3.12", + "@types/istanbul-lib-coverage": "^2.0.1", + "convert-source-map": "^2.0.0" + }, + "engines": { + "node": ">=10.12.0" + } + }, + "node_modules/v8-to-istanbul/node_modules/@jridgewell/trace-mapping": { + "version": "0.3.31", + "resolved": "https://registry.npmjs.org/@jridgewell/trace-mapping/-/trace-mapping-0.3.31.tgz", + "integrity": "sha512-zzNR+SdQSDJzc8joaeP8QQoCQr8NuYx2dIIytl1QeBEZHJ9uW6hebsrYgbz8hJwUQao3TWCMtmfV8Nu1twOLAw==", + "dev": true, + "license": "MIT", + "dependencies": { + "@jridgewell/resolve-uri": "^3.1.0", + "@jridgewell/sourcemap-codec": "^1.4.14" + } + }, "node_modules/vary": { "version": "1.1.2", "resolved": "https://registry.npmjs.org/vary/-/vary-1.1.2.tgz", @@ -20987,6 +21502,86 @@ "url": "https://github.com/chalk/wrap-ansi?sponsor=1" } }, + "node_modules/wrap-ansi-cjs": { + "name": "wrap-ansi", + "version": "7.0.0", + "resolved": "https://registry.npmjs.org/wrap-ansi/-/wrap-ansi-7.0.0.tgz", + "integrity": "sha512-YVGIj2kamLSTxw6NsZjoBxfSwsn0ycdesmc4p+Q21c5zPuZ1pl+NfxVdxPtdHvmNVOQ6XSYG4AUtyt/Fi7D16Q==", + "dev": true, + "license": "MIT", + "dependencies": { + "ansi-styles": "^4.0.0", + "string-width": "^4.1.0", + "strip-ansi": "^6.0.0" + }, + "engines": { + "node": ">=10" + }, + "funding": { + "url": "https://github.com/chalk/wrap-ansi?sponsor=1" + } + }, + "node_modules/wrap-ansi-cjs/node_modules/ansi-regex": { + "version": "5.0.1", + "resolved": "https://registry.npmjs.org/ansi-regex/-/ansi-regex-5.0.1.tgz", + "integrity": "sha512-quJQXlTSUGL2LH9SUXo8VwsY4soanhgo6LNSm84E1LBcE8s3O0wpdiRzyR9z/ZZJMlMWv37qOOb9pdJlMUEKFQ==", + "dev": true, + "license": "MIT", + "engines": { + "node": ">=8" + } + }, + "node_modules/wrap-ansi-cjs/node_modules/ansi-styles": { + "version": "4.3.0", + "resolved": "https://registry.npmjs.org/ansi-styles/-/ansi-styles-4.3.0.tgz", + "integrity": "sha512-zbB9rCJAT1rbjiVDb2hqKFHNYLxgtk8NURxZ3IZwD3F6NtxbXZQCnnSi1Lkx+IDohdPlFp222wVALIheZJQSEg==", + "dev": true, + "license": "MIT", + "dependencies": { + "color-convert": "^2.0.1" + }, + "engines": { + "node": ">=8" + }, + "funding": { + "url": "https://github.com/chalk/ansi-styles?sponsor=1" + } + }, + "node_modules/wrap-ansi-cjs/node_modules/emoji-regex": { + "version": "8.0.0", + "resolved": "https://registry.npmjs.org/emoji-regex/-/emoji-regex-8.0.0.tgz", + "integrity": "sha512-MSjYzcWNOA0ewAHpz0MxpYFvwg6yjy1NG3xteoqz644VCo/RPgnr1/GGt+ic3iJTzQ8Eu3TdM14SawnVUmGE6A==", + "dev": true, + "license": "MIT" + }, + "node_modules/wrap-ansi-cjs/node_modules/string-width": { + "version": "4.2.3", + "resolved": "https://registry.npmjs.org/string-width/-/string-width-4.2.3.tgz", + "integrity": "sha512-wKyQRQpjJ0sIp62ErSZdGsjMJWsap5oRNihHhu6G7JVO/9jIB6UyevL+tXuOqrng8j/cxKTWyWUwvSTriiZz/g==", + "dev": true, + "license": "MIT", + "dependencies": { + "emoji-regex": "^8.0.0", + "is-fullwidth-code-point": "^3.0.0", + "strip-ansi": "^6.0.1" + }, + "engines": { + "node": ">=8" + } + }, + "node_modules/wrap-ansi-cjs/node_modules/strip-ansi": { + "version": "6.0.1", + "resolved": "https://registry.npmjs.org/strip-ansi/-/strip-ansi-6.0.1.tgz", + "integrity": "sha512-Y38VPSHcqkFrCpFnQ9vuSXmquuv5oXOKpGeT6aGrr3o3Gc9AlVa6JBfUSOCnbxGGZF+/0ooI7KrPuUSztUdU5A==", + "dev": true, + "license": "MIT", + "dependencies": { + "ansi-regex": "^5.0.1" + }, + "engines": { + "node": ">=8" + } + }, "node_modules/wrappy": { "version": "1.0.2", "resolved": "https://registry.npmjs.org/wrappy/-/wrappy-1.0.2.tgz", @@ -22012,6 +22607,7 @@ }, "devDependencies": { "@types/node": "^22.20.1", + "c8": "^10.1.3", "typescript": "^5.9.3" }, "engines": { diff --git a/package.json b/package.json index cc0534f525..a2c4d651b7 100644 --- a/package.json +++ b/package.json @@ -48,6 +48,7 @@ "control-plane:install": "npm ci --prefix control-plane --prefer-offline --no-audit --no-fund", "control-plane:test": "npm run control-plane:install && npm --prefix control-plane test", "control-plane:coverage": "node scripts/control-plane-coverage.mjs", + "engine:coverage": "node scripts/engine-coverage.mjs", "db:migrations:check": "tsx scripts/check-migrations.ts", "db:schema-drift:check": "tsx scripts/check-schema-drift.ts", "actionlint": "node --experimental-strip-types scripts/actionlint.ts", diff --git a/packages/loopover-engine/package.json b/packages/loopover-engine/package.json index d93a83bfb1..67759ae6ba 100644 --- a/packages/loopover-engine/package.json +++ b/packages/loopover-engine/package.json @@ -99,6 +99,7 @@ }, "devDependencies": { "@types/node": "^22.20.1", + "c8": "^10.1.3", "typescript": "^5.9.3" }, "engines": { diff --git a/packages/loopover-engine/tsconfig.json b/packages/loopover-engine/tsconfig.json index 46ad09e759..ad8127428d 100644 --- a/packages/loopover-engine/tsconfig.json +++ b/packages/loopover-engine/tsconfig.json @@ -5,6 +5,7 @@ "moduleResolution": "NodeNext", "types": ["node"], "declaration": true, + "sourceMap": true, "rootDir": "src", "outDir": "dist", "noEmit": false, diff --git a/scripts/engine-coverage.mjs b/scripts/engine-coverage.mjs new file mode 100644 index 0000000000..fe6eda744c --- /dev/null +++ b/scripts/engine-coverage.mjs @@ -0,0 +1,85 @@ +// Harvest @loopover/engine's real node:test coverage into an lcov Codecov can ingest (#9064). +// The engine package's own suite (packages/loopover-engine/test/**/*.test.ts, compiled to +// dist-test/**/*.test.js by `npm run test --workspace @loopover/engine`) runs under plain +// `node --test`, with no instrumentation and no Codecov upload -- so engine source is graded only +// by whatever vitest's root test/** suite happens to import, even though vitest.config.ts's +// coverage.include already counts packages/loopover-engine/src/** toward the total. That mismatch +// rewards duplicating a thin vitest test alongside the real ungraded node:test one instead of +// deepening the real suite. +// +// Runs c8 from the monorepo root so source-map remapping (packages/loopover-engine/tsconfig.json's +// "sourceMap": true) yields `packages/loopover-engine/src/**` paths, not bare `dist/**` -- mirroring +// rees-coverage.ts / control-plane-coverage.mjs's identical "node:test suite invisible to vitest" +// shape and their `--include=/dist/**/*.js` + `--all` + lcov-path-normalize recipe. +import { spawnSync } from "node:child_process"; +import { readdirSync, readFileSync, writeFileSync } from "node:fs"; +import { join, relative } from "node:path"; +import { fileURLToPath } from "node:url"; + +/** Normalize c8's SF: paths to forward slashes for Codecov. Swallows only a missing report + * (ENOENT on read) -- CI's "Verify engine coverage report exists" step fails closed downstream. + * Any other read/write error propagates so a real lcov post-process failure is not masked. */ +export function normalizeLcovSfPaths( + lcovPath, + { readFile = readFileSync, writeFile = writeFileSync } = {}, +) { + try { + const raw = readFile(lcovPath, "utf8"); + writeFile( + lcovPath, + raw.replace(/^SF:(.*)$/gm, (_match, path) => `SF:${String(path).replace(/\\/g, "/")}`), + ); + } catch (error) { + if (error && typeof error === "object" && "code" in error && error.code === "ENOENT") return; + throw error; + } +} + +function collectTests(dir, out = []) { + for (const ent of readdirSync(dir, { withFileTypes: true })) { + const path = join(dir, ent.name); + if (ent.isDirectory()) collectTests(path, out); + else if (ent.name.endsWith(".test.js")) out.push(path); + } + return out; +} + +function main() { + const root = join(fileURLToPath(new URL(".", import.meta.url)), ".."); + const c8Bin = join(root, "node_modules", "c8", "bin", "c8.js"); + const pkgDir = join(root, "packages", "loopover-engine"); + const reportDir = join(pkgDir, "coverage"); + const testRoot = join(pkgDir, "dist-test"); + + // Relative to root: c8's --include glob below is matched against cwd-relative paths, and Codecov + // wants repo-relative SF: paths anyway, so express everything the same way from the start. + const tests = collectTests(testRoot).map((path) => relative(root, path).split("\\").join("/")); + if (tests.length === 0) { + console.error("engine-coverage: no packages/loopover-engine/dist-test/**/*.test.js files found -- run `npm run test --workspace @loopover/engine` first"); + process.exit(1); + } + + const result = spawnSync( + process.execPath, + [ + c8Bin, + "--reporter=lcov", + "--reporter=text-summary", + `--report-dir=${reportDir}`, + "--include=packages/loopover-engine/dist/**/*.js", + "--exclude=**/*.d.ts", + "--all", + process.execPath, + "--test", + ...tests, + ], + { cwd: root, stdio: "inherit", env: process.env }, + ); + + // Codecov expects forward-slash SF: paths; c8 on Windows emits backslashes. + normalizeLcovSfPaths(join(reportDir, "lcov.info")); + + process.exit(result.status === null ? 1 : result.status); +} + +if (process.argv[1] === fileURLToPath(import.meta.url)) main(); diff --git a/test/unit/engine-coverage-script.test.ts b/test/unit/engine-coverage-script.test.ts new file mode 100644 index 0000000000..8b534e0865 --- /dev/null +++ b/test/unit/engine-coverage-script.test.ts @@ -0,0 +1,43 @@ +import { describe, expect, it, vi } from "vitest"; +import { normalizeLcovSfPaths } from "../../scripts/engine-coverage.mjs"; + +describe("engine-coverage script", () => { + describe("normalizeLcovSfPaths", () => { + it("swallows ENOENT when the lcov report does not exist yet", () => { + const readFile = vi.fn(() => { + const err = new Error("ENOENT") as NodeJS.ErrnoException; + err.code = "ENOENT"; + throw err; + }); + const writeFile = vi.fn(); + + expect(() => normalizeLcovSfPaths("/tmp/missing/lcov.info", { readFile, writeFile })).not.toThrow(); + expect(readFile).toHaveBeenCalledOnce(); + expect(writeFile).not.toHaveBeenCalled(); + }); + + it("re-throws a write failure instead of masking it as a missing report", () => { + const writeErr = new Error("EACCES: permission denied") as NodeJS.ErrnoException; + writeErr.code = "EACCES"; + const readFile = vi.fn(() => "SF:packages\\loopover-engine\\src\\foo.ts\nend_of_record\n"); + const writeFile = vi.fn(() => { + throw writeErr; + }); + + expect(() => normalizeLcovSfPaths("/tmp/lcov.info", { readFile, writeFile })).toThrow(writeErr); + expect(writeFile).toHaveBeenCalledOnce(); + }); + + it("normalizes backslashes in SF: paths to forward slashes", () => { + let written = ""; + const readFile = vi.fn(() => "SF:packages\\loopover-engine\\src\\foo.ts\nend_of_record\n"); + const writeFile = vi.fn((_path: string, content: string) => { + written = content; + }); + + normalizeLcovSfPaths("/tmp/lcov.info", { readFile, writeFile }); + + expect(written).toBe("SF:packages/loopover-engine/src/foo.ts\nend_of_record\n"); + }); + }); +}); From 36f8d4ef9da2916eb1f775402c79161f2b303f2a Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 27 Jul 2026 00:47:56 -0700 Subject: [PATCH 2/7] fix(coverage): fix malformed v8-ignore directives and add a linter to catch new ones (#9064) Two concrete, malformed ignore directives silently widened their exempted range far past what was intended (empirically confirmed by measuring coverage before/after each fix): - src/scenarios/input-model.ts used `/* v8 ignore end */`, not a valid v8 terminator (must be `stop`). The unterminated `v8 ignore start` above it stayed open through EOF, silently excluding ~60 lines (validateBucketKinds, sortEntries, compactRepoConfig, trimScenarioText, optionalScenarioText, ...) instead of the ~3-line defensive branch it was meant to cover. Fixed the terminator and added the one real test the wider surface needed (a whitespace-only branchName/baseRef falling back to "absent", not a blank string). - src/review/loop-escalation-wire.ts carried a whole-file ignore whose justification (codecov patch reporting one branch as uncovered "across shards") stopped applying when codecov.yml unsharded the backend suite to one whole-suite upload on 2026-07-24. Removed the ignore and added the one real test it needed: continuing (not throwing) when recording the missing-webhook "denied" audit event itself fails. Building the linter for these (test/unit/codecov-policy.test.ts) surfaced the same defect class a third time, already live: packages/loopover-engine/src/opportunity-metadata.ts's `computeMetadataPotential` ignore-start (line 110) was missing its `stop` before the next function's own start/stop pair, so it silently absorbed all of computeMetadataFeasibility's real logic too; a separate stray `v8 ignore stop` at EOF had no matching start at all. Fixed both. The new check scans src/**, packages/loopover-engine/src/**, packages/loopover-{miner,mcp}/{lib,bin}/**, and packages/discovery-index/src/** (mirroring vitest.config.ts's own coverage.include roots) for any v8-ignore keyword outside the {next, start, stop, file, else} set, an unterminated start, a stray stop, or a whole-file ignore with no issue-number reference -- so a new malformed directive fails CI going forward instead of silently widening. Deliberately does NOT require the ~105 pre-existing ignores without a justification comment to gain one (a much larger, separate cleanup); it only enforces structural validity. Matches on the comment opener directly (`/\*\s*v8 ignore ...`) rather than pairing every `/*` with the next `*/` in the file: src/api/routes.ts contains a literal `/*` substring in ordinary prose (a quoted route-path glob, `` `/v1/internal/*` ``, inside a `//` comment) that the naive approach merged with an unrelated real comment 56KB later, swallowing a legitimate directive in between. --- .github/workflows/ci.yml | 2 +- package.json | 2 +- .../loopover-engine/src/miner/iterate-loop.ts | 4 +- .../src/opportunity-metadata.ts | 2 +- ...engine-coverage.mjs => engine-coverage.ts} | 10 +- src/review/loop-escalation-wire.ts | 7 +- src/scenarios/input-model.ts | 2 +- test/unit/codecov-policy.test.ts | 131 +++++++++++++++++- test/unit/engine-coverage-script.test.ts | 2 +- test/unit/loop-escalation-wire.test.ts | 12 ++ test/unit/scenario-input-model.test.ts | 12 ++ 11 files changed, 171 insertions(+), 15 deletions(-) rename scripts/{engine-coverage.mjs => engine-coverage.ts} (91%) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 56768fe440..77b5775de5 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -712,7 +712,7 @@ jobs: - name: Engine package tests if: ${{ github.event_name == 'push' || needs.changes.outputs.engine == 'true' }} run: npm run test --workspace @loopover/engine - # engine-coverage.mjs re-runs the suite (dist-test/**/*.test.js, left in place by the step above) + # engine-coverage.ts re-runs the suite (dist-test/**/*.test.js, left in place by the step above) # under c8, remapped through packages/loopover-engine/tsconfig.json's "sourceMap": true back to # packages/loopover-engine/src/**.ts paths -- the same "node:test suite invisible to vitest" shape # and c8 recipe as rees:coverage/control-plane:coverage above. The authoritative pass/fail for the diff --git a/package.json b/package.json index a2c4d651b7..11c9b95af4 100644 --- a/package.json +++ b/package.json @@ -48,7 +48,7 @@ "control-plane:install": "npm ci --prefix control-plane --prefer-offline --no-audit --no-fund", "control-plane:test": "npm run control-plane:install && npm --prefix control-plane test", "control-plane:coverage": "node scripts/control-plane-coverage.mjs", - "engine:coverage": "node scripts/engine-coverage.mjs", + "engine:coverage": "node --experimental-strip-types scripts/engine-coverage.ts", "db:migrations:check": "tsx scripts/check-migrations.ts", "db:schema-drift:check": "tsx scripts/check-schema-drift.ts", "actionlint": "node --experimental-strip-types scripts/actionlint.ts", diff --git a/packages/loopover-engine/src/miner/iterate-loop.ts b/packages/loopover-engine/src/miner/iterate-loop.ts index d87e1c7a06..bb2848654d 100644 --- a/packages/loopover-engine/src/miner/iterate-loop.ts +++ b/packages/loopover-engine/src/miner/iterate-loop.ts @@ -313,8 +313,8 @@ const ZERO_METER_TOTALS: AttemptMeterTotals = { tokens: 0, turns: 0, wallClockMs /** The result shape {@link runIterateLoopCore} itself returns -- everything BUT the meter fields, which the * thin {@link runIterateLoop} wrapper attaches once, from `tracker`, at its own single always-reached return - * point (#5395). Keeps the core's own internal return statements -- including the `/* v8 ignore *\/`-guarded - * unreachable fallback -- byte-identical to their pre-#5395 shape, so that genuinely unreachable branch never + * point (#5395). Keeps the core's own internal return statements -- including the v8-ignore-guarded + * unreachable fallback below -- byte-identical to their pre-#5395 shape, so that genuinely unreachable branch never * needs new fields threaded onto it (v8's ignore-comment suppresses vitest's OWN text-reporter percentage, * but NOT the raw lcov Codecov reads -- a new field on that branch would show as a real uncovered patch line * with no way to actually exercise it). */ diff --git a/packages/loopover-engine/src/opportunity-metadata.ts b/packages/loopover-engine/src/opportunity-metadata.ts index 70ab56c53a..5e99aea9dc 100644 --- a/packages/loopover-engine/src/opportunity-metadata.ts +++ b/packages/loopover-engine/src/opportunity-metadata.ts @@ -121,6 +121,7 @@ export function computeMetadataPotential(issue: { labels: readonly string[] }): if (labels.includes("refactor")) score += 0.05; return clamp01(score); } +/* v8 ignore stop */ /** * Estimate achievability from metadata-only cues: lower discussion load and fresher issues score higher. @@ -235,4 +236,3 @@ export function rankMetadataOpportunities( /* v8 ignore next */ return rankOpportunities(annotated) as Array; } -/* v8 ignore stop */ diff --git a/scripts/engine-coverage.mjs b/scripts/engine-coverage.ts similarity index 91% rename from scripts/engine-coverage.mjs rename to scripts/engine-coverage.ts index fe6eda744c..7e73d7c667 100644 --- a/scripts/engine-coverage.mjs +++ b/scripts/engine-coverage.ts @@ -14,15 +14,15 @@ import { spawnSync } from "node:child_process"; import { readdirSync, readFileSync, writeFileSync } from "node:fs"; import { join, relative } from "node:path"; -import { fileURLToPath } from "node:url"; +import { fileURLToPath, URL } from "node:url"; /** Normalize c8's SF: paths to forward slashes for Codecov. Swallows only a missing report * (ENOENT on read) -- CI's "Verify engine coverage report exists" step fails closed downstream. * Any other read/write error propagates so a real lcov post-process failure is not masked. */ export function normalizeLcovSfPaths( - lcovPath, - { readFile = readFileSync, writeFile = writeFileSync } = {}, -) { + lcovPath: string, + { readFile = readFileSync, writeFile = writeFileSync }: { readFile?: (path: string, encoding: "utf8") => string; writeFile?: (path: string, data: string) => void } = {}, +): void { try { const raw = readFile(lcovPath, "utf8"); writeFile( @@ -35,7 +35,7 @@ export function normalizeLcovSfPaths( } } -function collectTests(dir, out = []) { +function collectTests(dir: string, out: string[] = []): string[] { for (const ent of readdirSync(dir, { withFileTypes: true })) { const path = join(dir, ent.name); if (ent.isDirectory()) collectTests(path, out); diff --git a/src/review/loop-escalation-wire.ts b/src/review/loop-escalation-wire.ts index c318c334d9..b557012c6d 100644 --- a/src/review/loop-escalation-wire.ts +++ b/src/review/loop-escalation-wire.ts @@ -11,8 +11,11 @@ // byte-identical to today. There is no rented-loop D1 store yet; the loader reads LOOPOVER_ACTIVE_LOOPS_JSON // (a JSON array of ActiveLoopFacts) so a simulated escalation-worthy loop can reach a human without waiting on // the separate observability-store work (#4793). Callers may inject `loadActiveLoops` in tests. -/* v8 ignore file -- thoroughly unit-tested in test/unit/loop-escalation-wire.test.ts; codecov patch still - * reports a single defensive branch as uncovered across shards despite those tests. */ +// +// (#9064: the whole-file ignore this module carried was removed -- its justification ("codecov patch still +// reports a single defensive branch as uncovered across shards") stopped applying when codecov.yml unsharded +// the backend suite to one whole-suite lcov upload on 2026-07-24; see that file's own "notify.after_n_builds" +// comment. The one remaining genuinely defensive branch is covered directly below instead.) import { buildActiveLoopFleetSummary, diff --git a/src/scenarios/input-model.ts b/src/scenarios/input-model.ts index d7bcfb13c2..9ba6cc2206 100644 --- a/src/scenarios/input-model.ts +++ b/src/scenarios/input-model.ts @@ -318,7 +318,7 @@ function assertPublicScenarioSnapshotSafe(snapshot: PublicScenarioInputSnapshot) if (FORBIDDEN_PUBLIC_LANGUAGE.test(serialized)) { throw new Error("Public scenario serialization still contains forbidden language."); } - /* v8 ignore end */ + /* v8 ignore stop */ } function validateBucketKinds( diff --git a/test/unit/codecov-policy.test.ts b/test/unit/codecov-policy.test.ts index 8d41a1f986..9188d81f68 100644 --- a/test/unit/codecov-policy.test.ts +++ b/test/unit/codecov-policy.test.ts @@ -1,4 +1,5 @@ -import { readFileSync } from "node:fs"; +import { readdirSync, readFileSync, statSync } from "node:fs"; +import { join } from "node:path"; import { parse } from "yaml"; function readYaml(path: string): Record { @@ -289,3 +290,131 @@ describe("Codecov policy", () => { expect(String(validateTests.if)).toContain("needs.changes.outputs.controlPlane == 'true'"); }); }); + +/** Mirrors vitest.config.ts's own coverage.include roots (kept in sync by hand, same discipline as that + * file's own header comment) -- exactly the source trees Codecov gates on patch coverage, so a malformed + * v8-ignore directive anywhere in them can silently widen an exempted range past what anyone intended, + * precisely the bug this check exists to catch (#9064: src/scenarios/input-model.ts's `v8 ignore end` -- + * not a valid v8 terminator -- silently exempted everything from that point to EOF, ~30 lines wide of the + * ~3 lines it was meant to cover). */ +const V8_IGNORE_SCAN_ROOTS = [ + "src", + "packages/loopover-engine/src", + "packages/loopover-miner/lib", + "packages/loopover-miner/bin", + "packages/discovery-index/src", + "packages/loopover-mcp/lib", + "packages/loopover-mcp/bin", +]; + +function collectTsFiles(dir: string, out: string[] = []): string[] { + for (const entry of readdirSync(dir, { withFileTypes: true })) { + const path = join(dir, entry.name); + if (entry.isDirectory()) { + collectTsFiles(path, out); + } else if (entry.isFile() && path.endsWith(".ts") && !path.endsWith(".d.ts")) { + out.push(path); + } + } + return out; +} + +/** Every v8/c8 coverage-ignore hint this codebase actually uses (a bare `next`, `next N`, the `start`/ + * `stop` pair, whole-file `file`, and branch-shaped `else`) -- anything outside this set (e.g. `end`, + * which is NOT a valid v8 terminator) is malformed, and worse, silently WIDENS the exempted range rather + * than failing loudly: an unterminated `start` (a bad terminator keyword, or none at all before EOF) + * keeps ignoring every subsequent line in the file, not just the intended block. */ +const VALID_V8_IGNORE_KEYWORDS = new Set(["next", "start", "stop", "file", "else"]); +// Matched directly against the opening of a comment (`/*` + optional extra `*` for a `/**` doc-comment + +// whitespace + "v8 ignore" + the keyword) -- deliberately NOT "find every /* ... */ span, then check its +// contents": a real .ts file in this repo routinely contains a literal `/*` substring that is not a comment +// opener at all (e.g. a route-path glob quoted in prose, `` `/v1/internal/*` ``, inside an ordinary `//` +// line comment) -- naively pairing THAT `/*` with the next real `*/` anywhere later in the file merges two +// unrelated comments into one bogus span and silently swallows whatever directive sits inside it (confirmed +// empirically against src/api/routes.ts while building this check). Matching "v8 ignore" at the exact +// position right after a real `/*` sidesteps the problem entirely: it needs no closing `*/` to be found. +const V8_IGNORE_DIRECTIVE_RE = /\/\*\*?\s*v8 ignore\s+(\S+)/g; +// Only consulted for the rare `file` directive, to check the same comment for an issue reference -- bounded +// so a genuinely unrelated later `*/` (or none at all before EOF) can't runaway-scan the rest of the file. +const COMMENT_TAIL_SEARCH_WINDOW = 500; + +function findMalformedV8IgnoreDirectives(filePath: string, content: string): string[] { + const problems: string[] = []; + let openStartLine: number | null = null; + V8_IGNORE_DIRECTIVE_RE.lastIndex = 0; + let match: RegExpExecArray | null; + while ((match = V8_IGNORE_DIRECTIVE_RE.exec(content)) !== null) { + const keyword = match[1]!; + const line = content.slice(0, match.index).split("\n").length; + if (!VALID_V8_IGNORE_KEYWORDS.has(keyword)) { + problems.push( + `${filePath}:${line}: unrecognized "v8 ignore ${keyword}" -- not a valid v8 terminator (valid: ${[...VALID_V8_IGNORE_KEYWORDS].join(", ")})`, + ); + continue; + } + if (keyword === "start") { + if (openStartLine !== null) { + problems.push(`${filePath}:${line}: "v8 ignore start" opened again before the one at line ${openStartLine} was closed with "stop"`); + } + openStartLine = line; + } else if (keyword === "stop") { + if (openStartLine === null) { + problems.push(`${filePath}:${line}: "v8 ignore stop" with no matching "v8 ignore start" before it`); + } + openStartLine = null; + } else if (keyword === "file") { + const tail = content.slice(match.index, match.index + COMMENT_TAIL_SEARCH_WINDOW); + const closeIndex = tail.indexOf("*/"); + const comment = closeIndex === -1 ? tail : tail.slice(0, closeIndex); + if (!/#\d+/.test(comment)) { + problems.push(`${filePath}:${line}: whole-file "v8 ignore file" must reference an issue number (e.g. "#1234") justifying the exemption`); + } + } + } + if (openStartLine !== null) { + problems.push( + `${filePath}: "v8 ignore start" at line ${openStartLine} is never closed with a matching "v8 ignore stop" -- everything after it in the file is silently excluded from coverage`, + ); + } + return problems; +} + +describe("v8 ignore directive hygiene (#9064)", () => { + it("has no unterminated start, non-stop terminator, or unreferenced whole-file ignore", () => { + const problems = V8_IGNORE_SCAN_ROOTS.filter((root) => { + try { + return statSync(root).isDirectory(); + } catch { + return false; + } + }) + .flatMap((root) => collectTsFiles(root)) + .flatMap((filePath) => findMalformedV8IgnoreDirectives(filePath, readFileSync(filePath, "utf8"))); + + expect(problems).toEqual([]); + }); + + it("flags the exact defect classes this check exists to catch (self-test)", () => { + expect(findMalformedV8IgnoreDirectives("fixture.ts", "/* v8 ignore end */\ncode();\n")).toEqual([ + 'fixture.ts:1: unrecognized "v8 ignore end" -- not a valid v8 terminator (valid: next, start, stop, file, else)', + ]); + expect(findMalformedV8IgnoreDirectives("fixture.ts", "/* v8 ignore start */\ncode();\n")).toEqual([ + 'fixture.ts: "v8 ignore start" at line 1 is never closed with a matching "v8 ignore stop" -- everything after it in the file is silently excluded from coverage', + ]); + expect(findMalformedV8IgnoreDirectives("fixture.ts", "code();\n/* v8 ignore stop */\n")).toEqual([ + 'fixture.ts:2: "v8 ignore stop" with no matching "v8 ignore start" before it', + ]); + expect( + findMalformedV8IgnoreDirectives("fixture.ts", "/* v8 ignore start */\ncode();\n/* v8 ignore start */\nmore();\n/* v8 ignore stop */\n"), + ).toEqual(['fixture.ts:3: "v8 ignore start" opened again before the one at line 1 was closed with "stop"']); + expect(findMalformedV8IgnoreDirectives("fixture.ts", "/* v8 ignore file */\ncode();\n")).toEqual([ + 'fixture.ts:1: whole-file "v8 ignore file" must reference an issue number (e.g. "#1234") justifying the exemption', + ]); + expect(findMalformedV8IgnoreDirectives("fixture.ts", "/* v8 ignore file -- see #1234 */\ncode();\n")).toEqual([]); + expect(findMalformedV8IgnoreDirectives("fixture.ts", "/* v8 ignore next */\ncode();\n")).toEqual([]); + expect(findMalformedV8IgnoreDirectives("fixture.ts", "/* v8 ignore next 3 */\ncode();\n")).toEqual([]); + expect(findMalformedV8IgnoreDirectives("fixture.ts", "/* v8 ignore start */\ncode();\n/* v8 ignore stop */\n")).toEqual([]); + expect(findMalformedV8IgnoreDirectives("fixture.ts", "/* v8 ignore else */\ncode();\n")).toEqual([]); + expect(findMalformedV8IgnoreDirectives("fixture.ts", "no ignores here\n")).toEqual([]); + }); +}); diff --git a/test/unit/engine-coverage-script.test.ts b/test/unit/engine-coverage-script.test.ts index 8b534e0865..0c89bbe2da 100644 --- a/test/unit/engine-coverage-script.test.ts +++ b/test/unit/engine-coverage-script.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it, vi } from "vitest"; -import { normalizeLcovSfPaths } from "../../scripts/engine-coverage.mjs"; +import { normalizeLcovSfPaths } from "../../scripts/engine-coverage.js"; describe("engine-coverage script", () => { describe("normalizeLcovSfPaths", () => { diff --git a/test/unit/loop-escalation-wire.test.ts b/test/unit/loop-escalation-wire.test.ts index 405157e6e6..a03a6a70d3 100644 --- a/test/unit/loop-escalation-wire.test.ts +++ b/test/unit/loop-escalation-wire.test.ts @@ -256,6 +256,18 @@ describe("runLoopEscalationSweep (#6349)", () => { expect(result.reason).toBe("invalid_global_webhook"); }); + it("continues when recording the missing-webhook 'denied' audit event itself throws (#9064)", async () => { + vi.spyOn(console, "error").mockImplementation(() => undefined); + const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => undefined); + vi.spyOn(repositories, "recordAuditEvent").mockRejectedValueOnce(new Error("audit store unavailable")); + const result = await runLoopEscalationSweep(createTestEnv(), { + loadActiveLoops: () => [{ loopId: "broken", tenantId: "acme", runStatus: "error" }], + }); + expect(result.notified).toBe(false); + expect(result.reason).toBe("missing_global_webhook"); + expect(warnSpy.mock.calls.some((c) => String(c[0]).includes("loop_escalation_audit_failed"))).toBe(true); + }); + it("records an error when Discord returns a non-OK status", async () => { vi.spyOn(console, "error").mockImplementation(() => undefined); const warnSpy = vi.spyOn(console, "warn").mockImplementation(() => undefined); diff --git a/test/unit/scenario-input-model.test.ts b/test/unit/scenario-input-model.test.ts index d194765986..c92f985c02 100644 --- a/test/unit/scenario-input-model.test.ts +++ b/test/unit/scenario-input-model.test.ts @@ -338,6 +338,18 @@ describe("source-upload safety", () => { expect(JSON.stringify(input)).not.toMatch(/fileContent|sourceContent|upload/i); }); + it("treats a whitespace-only branchName/baseRef as absent, not a blank string (optionalScenarioText's empty-after-trim fallback)", () => { + const input = scenarioInputFromLocalBranchMetadata({ + scenarioType: "branch_preflight", + login: "miner", + repoFullName: "octo/demo", + branchName: " ", + baseRef: " ", + }); + expect(input.branchState).toBeUndefined(); + expect(input.facts.find((entry) => entry.id === "branch")).toBeUndefined(); + }); + it("bounds local branch metadata before scenario validation", () => { const input = scenarioInputFromLocalBranchMetadata({ scenarioType: "branch_preflight", From d9c695d577898523c288ee32bbb0599ad6ddc04e Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 27 Jul 2026 01:06:32 -0700 Subject: [PATCH 3/7] test(helpers): honor ttlSeconds/Date.now() in the transient-cache test double The shared SELFHOST_TRANSIENT_CACHE mock in createTestEnv() previously discarded ttlSeconds entirely, so lock expiry, orphaned-lock recovery, and starvation scenarios built on AI_REVIEW_LOCK_TTL_SECONDS were unrepresentable in every test that used it. Store a real expiresAtMs (Date.now() + ttl*1000) so vitest's fake timers can advance past it, mirroring the real Redis-backed cache's SET ... EX semantics (#9063). --- test/helpers/d1.ts | 52 +++++++++++++++++++++++++++++++++++++--------- 1 file changed, 42 insertions(+), 10 deletions(-) diff --git a/test/helpers/d1.ts b/test/helpers/d1.ts index 80cdd9ae03..ff620b0cff 100644 --- a/test/helpers/d1.ts +++ b/test/helpers/d1.ts @@ -145,8 +145,27 @@ export class TestD1Database { } } +/** One entry in the in-memory SELFHOST_TRANSIENT_CACHE mock below: the stored value plus the real + * wall-clock ms (Date.now()) at which it expires. */ +type TransientCacheEntry = { value: string; expiresAtMs: number }; + +/** True when `entry` exists and its TTL has not yet elapsed as of `nowMs` -- a type guard so callers can + * narrow away the `| undefined` after checking. An expired entry reads back identically to an absent one + * everywhere below, mirroring Redis's own server-side TTL expiry (a key past its EX either never existed + * as far as GET/SET NX/compare-delete can tell). */ +function isLiveTransientCacheEntry(entry: TransientCacheEntry | undefined, nowMs: number): entry is TransientCacheEntry { + return entry !== undefined && entry.expiresAtMs > nowMs; +} + export function createTestEnv(overrides: Partial = {}): Env { - const transientCache = new Map(); + // #9063: previously a plain Map whose set/claim silently discarded ttlSeconds + // entirely, so AI_REVIEW_LOCK_TTL_SECONDS (and every other lock namespace built on this shared + // cache) was a no-op in every test -- an orphaned-lock recovery, a starvation scenario, or a + // post-restart expiry was unrepresentable by construction. Storing a real expiresAtMs (read via the + // plain global Date.now(), which vitest's fake timers patch process-wide under + // vi.useFakeTimers()/vi.advanceTimersByTime()) makes all three ordinary, exactly the way the real + // Redis-backed cache (src/selfhost/redis-cache.ts's `SET key value EX ttlSeconds`) behaves. + const transientCache = new Map(); return { DB: new TestD1Database() as unknown as D1Database, JOBS: { @@ -187,27 +206,40 @@ export function createTestEnv(overrides: Partial = {}): Env { MCP_READ_REPO_ALLOWLIST: "*", SELFHOST_TRANSIENT_CACHE: { async get(key: string) { - return transientCache.get(key) ?? null; + const entry = transientCache.get(key); + if (!isLiveTransientCacheEntry(entry, Date.now())) { + // Lazily sweep an expired entry on read -- not required for correctness (every accessor below + // re-checks expiry independently), just keeps the map from accumulating stale keys forever. + if (entry !== undefined) transientCache.delete(key); + return null; + } + return entry.value; }, - async set(key: string, value: string) { - transientCache.set(key, value); + async set(key: string, value: string, ttlSeconds: number) { + transientCache.set(key, { value, expiresAtMs: Date.now() + ttlSeconds * 1000 }); }, async del(key: string) { transientCache.delete(key); }, // Mirrors createRedisCache's atomic claim (#2129): the check-and-set below has no `await` between the - // `has` read and the `set` write, so it completes synchronously within one microtask — a concurrent + // expiry check and the `set` write, so it completes synchronously within one microtask — a concurrent // caller can never observe the key as absent partway through another caller's claim, matching Redis's - // SET NX server-side atomicity. - async claim(key: string, value: string) { - if (transientCache.has(key)) return false; - transientCache.set(key, value); + // SET NX server-side atomicity. An EXPIRED existing entry is treated exactly like an absent one, so a + // lock whose TTL has lapsed can be reclaimed -- the orphaned-lock recovery path this cache previously + // made unrepresentable. + async claim(key: string, value: string, ttlSeconds: number) { + const existing = transientCache.get(key); + if (isLiveTransientCacheEntry(existing, Date.now())) return false; + transientCache.set(key, { value, expiresAtMs: Date.now() + ttlSeconds * 1000 }); return true; }, // Mirrors createRedisCache's atomic compare-and-delete (#2129): only deletes when the stored value still // equals the caller's own token, so a stale holder's release can never delete a different, live claim. + // An already-expired entry reads back as "nothing to release" (false), matching a real Redis key whose + // TTL already lapsed server-side -- there is no longer anything for the compare-delete to find. async releaseIfValue(key: string, value: string) { - if (transientCache.get(key) !== value) return false; + const existing = transientCache.get(key); + if (!isLiveTransientCacheEntry(existing, Date.now()) || existing.value !== value) return false; transientCache.delete(key); return true; }, From 78e5c4970fdcd5698d2583c9e11066f98c8b16e9 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 27 Jul 2026 01:17:25 -0700 Subject: [PATCH 4/7] test(review): add cross-surface invariant sweeps for lock/label/skip parity (#9063) Three invariant tests modeled on test/unit/pr-disposition-invariants.test.ts, each targeting one of the defect classes ordinary coverage cannot see: - AI-review lock claim outcomes are exhaustive and traceable (#9008): sweeps {contested, steal} so a forced retrigger never lands on the silent-contended branch, and confirms an expired (not stolen) lock recovers on its own now that the shared transient-cache test double honors ttlSeconds. - manual-review lock-contention auto-clear is reachable exactly once (#9009): sweeps four independent live-hold causes (guardrail, unverified CI, action-required, generic not-review-good) proving none of them is ever overridden by the lock-contention-resolved marker, plus the control case where removal is reachable. - reputation-triggered and contributor-controlled AI-review skips hold in lockstep (#9015): sweeps every aiReviewMode proving both sibling functions agree on whether to hold, never one without the other. --- test/unit/gate-integrity-invariants.test.ts | 180 ++++++++++++++++++++ 1 file changed, 180 insertions(+) create mode 100644 test/unit/gate-integrity-invariants.test.ts diff --git a/test/unit/gate-integrity-invariants.test.ts b/test/unit/gate-integrity-invariants.test.ts new file mode 100644 index 0000000000..ed0a06059b --- /dev/null +++ b/test/unit/gate-integrity-invariants.test.ts @@ -0,0 +1,180 @@ +import { describe, expect, it, vi } from "vitest"; +import { claimTransientLock } from "../../src/queue/transient-locks"; +import { maybeAddReputationSkipHold, maybeAddRequiredAutoReviewSkipHold } from "../../src/queue/processors"; +import { AGENT_LABEL_NEEDS_REVIEW, planAgentMaintenanceActions, type AgentActionPlanInput } from "../../src/settings/agent-actions"; +import type { GateCheckConclusion } from "../../src/rules/advisory"; +import { createTestEnv } from "../helpers/d1"; + +// #9063: three cross-surface invariant tests targeting the "absence"/"reachability"/"cross-path parity" defect +// classes the 99%-coverage gate structurally cannot see (every assertion below is about a branch that emits +// NOTHING, a state that can be entered but never left, or two paths that must agree -- not about a forward +// path a normal unit test already walks). Modeled on test/unit/pr-disposition-invariants.test.ts: sweep the +// state space with a loop rather than hand-picking one scenario, so a regression anywhere in the swept range +// fails here even if the specific case a narrative test picked still happens to pass. + +describe("AI-review lock claim outcomes are exhaustive and traceable (#9008, absence)", () => { + it("acquisition is fully determined by {contested, steal} -- steal eliminates the silent-contended branch instead of leaving it reachable", async () => { + for (const steal of [false, true]) { + const env = createTestEnv(); + const key = `ai-review-lock:steal=${steal}`; + + const uncontested = await claimTransientLock(env, key, 60); + expect(uncontested.acquired, `steal=${steal} uncontested`).toBe(true); + + // A second, non-steal claim against the SAME still-live key: this is the branch that used to be + // reachable for a forced retrigger too (#9008) -- a caller landing here with no distinguishing signal + // is exactly the "silent" shape #9063 warns about. Confirm it is still correctly contended... + const contendedNonSteal = await claimTransientLock(env, key, 60); + expect(contendedNonSteal.acquired, `steal=${steal} contended non-steal probe`).toBe(false); + + // ...then confirm `steal` makes the outcome for THIS call deterministic and distinct: a forced caller + // never lands on "acquired: false" (the fix), while a non-forced caller never silently succeeds either. + const outcome = await claimTransientLock(env, key, 60, { steal }); + expect(outcome.acquired, `steal=${steal} final outcome`).toBe(steal ? true : false); + } + }); + + it("an EXPIRED lock recovers WITHOUT steal -- orphaned-lock recovery, unrepresentable before #9063's TTL fix to the transient-cache test double", async () => { + vi.useFakeTimers(); + try { + const env = createTestEnv(); + const key = "ai-review-lock:orphan-recovery"; + + const original = await claimTransientLock(env, key, 30); + expect(original.acquired).toBe(true); + + const stillLive = await claimTransientLock(env, key, 30); + expect(stillLive.acquired).toBe(false); // TTL not yet elapsed -- genuinely contended + + vi.advanceTimersByTime(31_000); // past the 30s TTL: the process that held this lock is presumed dead + + const afterExpiry = await claimTransientLock(env, key, 30); // ordinary claim -- NOT steal + expect(afterExpiry.acquired).toBe(true); // recovers on its own; no forced retrigger needed + } finally { + vi.useRealTimers(); + } + }); +}); + +describe("manual-review lock-contention auto-clear is reachable exactly once, never alongside another live hold (#9009, reachability)", () => { + const basePlanInput = { + blockerTitles: [] as string[], + changedPaths: [] as string[], + hardGuardrailGlobs: [] as string[], + authorIsOwner: false, + authorIsAdmin: false, + authorIsAutomationBot: false, + autonomy: { approve: "auto", merge: "auto", review_state_label: "auto", close: "auto" } as const, + autoMaintain: { requireApprovals: 0, mergeMethod: "squash" } as const, + slopGateMinScore: 60, + ciState: "passed" as const, + }; + + // Four independent, real causes of `manualHoldReason !== null` (src/settings/agent-actions.ts ~1160): a + // regression that re-adds a fifth one without also excluding it from the auto-clear branch would only be + // caught by sweeping several causes, not by re-checking the one the original #9009 fix happened to cite. + const otherLiveHolds: Array<[string, Partial & { conclusion: GateCheckConclusion }]> = [ + ["a hard-guardrail path hit", { conclusion: "success", changedPaths: ["src/settings/agent-actions.ts"], hardGuardrailGlobs: ["src/settings/**"] }], + ["CI could not be verified", { conclusion: "success", ciState: "unverified" }], + ["the gate requires maintainer action", { conclusion: "action_required" }], + ["a not-review-good verdict with no close in flight", { conclusion: "neutral" }], + ]; + + for (const [label, over] of otherLiveHolds) { + it(`does NOT remove manual-review when ${label} is ALSO live this pass, even with the lock-contention marker resolved`, () => { + const plan = planAgentMaintenanceActions({ + ...basePlanInput, + pr: { labels: [AGENT_LABEL_NEEDS_REVIEW], mergeableState: "clean" }, + manualReviewLockContentionResolved: true, + ...over, + }); + expect(plan.some((a) => a.actionClass === "label" && a.label === AGENT_LABEL_NEEDS_REVIEW && a.labelOp === "remove")).toBe(false); + }); + } + + it("DOES remove once nothing else is live -- the reachable control case the four exclusions above are measured against", () => { + const plan = planAgentMaintenanceActions({ + ...basePlanInput, + conclusion: "success", + pr: { labels: [AGENT_LABEL_NEEDS_REVIEW], mergeableState: "clean" }, + manualReviewLockContentionResolved: true, + }); + expect(plan).toContainEqual(expect.objectContaining({ actionClass: "label", label: AGENT_LABEL_NEEDS_REVIEW, labelOp: "remove" })); + }); +}); + +describe("reputation-triggered and contributor-controlled AI-review skips hold in lockstep (#9015, cross-path parity)", () => { + const modes = ["block", "advisory", "off"] as const; + const advisoryStub = () => ({ headSha: "sha", findings: [] as Array<{ code: string }> }); + const envFor = () => ({ AI_SUMMARIES_ENABLED: "true", AI_PUBLIC_COMMENTS_ENABLED: "true", AI: {} }) as Env; + + it("for every aiReviewMode: a skip holds BOTH siblings identically, or NEITHER -- never one without the other", () => { + for (const aiReviewMode of modes) { + const settings = { gatePack: "oss-anti-slop", aiReviewMode, aiReviewAllAuthors: false } as never; + const env = envFor(); + + const contributorAdvisory = advisoryStub(); + const contributorHeld = maybeAddRequiredAutoReviewSkipHold(env, { + settings, + advisory: contributorAdvisory as never, + repoFullName: "acme/widgets", + author: "alice", + confirmedContributor: false, + autoReviewSkipReason: "review skipped (WIP title)", + }); + + const reputationAdvisory = advisoryStub(); + const reputationHeld = maybeAddReputationSkipHold(env, { + settings, + advisory: reputationAdvisory as never, + repoFullName: "acme/widgets", + author: "burst-farmer", + confirmedContributor: false, + reputationSkipped: true, + }); + + expect(reputationHeld, `mode=${aiReviewMode}`).toBe(contributorHeld); + expect(reputationAdvisory.findings.length, `mode=${aiReviewMode}`).toBe(contributorAdvisory.findings.length); + if (contributorHeld) { + expect(contributorAdvisory.findings[0]).toMatchObject({ code: "ai_review_inconclusive" }); + expect(reputationAdvisory.findings[0]).toMatchObject({ code: "ai_review_inconclusive" }); + } + } + }); + + it("for every aiReviewMode: neither sibling holds when its OWN skip condition is false", () => { + for (const aiReviewMode of modes) { + const settings = { gatePack: "oss-anti-slop", aiReviewMode, aiReviewAllAuthors: false } as never; + const env = envFor(); + + const contributorAdvisory = advisoryStub(); + expect( + maybeAddRequiredAutoReviewSkipHold(env, { + settings, + advisory: contributorAdvisory as never, + repoFullName: "acme/widgets", + author: "alice", + confirmedContributor: false, + autoReviewSkipReason: null, + }), + `mode=${aiReviewMode}`, + ).toBe(false); + + const reputationAdvisory = advisoryStub(); + expect( + maybeAddReputationSkipHold(env, { + settings, + advisory: reputationAdvisory as never, + repoFullName: "acme/widgets", + author: "alice", + confirmedContributor: false, + reputationSkipped: false, + }), + `mode=${aiReviewMode}`, + ).toBe(false); + + expect(contributorAdvisory.findings, `mode=${aiReviewMode}`).toEqual([]); + expect(reputationAdvisory.findings, `mode=${aiReviewMode}`).toEqual([]); + } + }); +}); From 240ba65ef6093118a06ccc05bcb01370bd617490 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 27 Jul 2026 01:36:58 -0700 Subject: [PATCH 5/7] fix(config): wire unknown-key validation into the runtime manifest parser (#9065) unknownTopLevelWarnings previously ran ONLY in the offline validator (loopover_validate_config, the validate route, the admin dry-run write, the CLI lint script) -- the runtime load path (loadRepoFocusManifest -> parseFocusManifestContent -> parseFocusManifest) never called it, so an operator editing a mounted manifest file directly (the normal self-host workflow) got zero feedback on a typo'd top-level key. - Moved the top-level-field knowledge (previously a hand-duplicated TOP_LEVEL_FIELDS list living only in config-lint.ts) into focus-manifest.ts as unknownTopLevelManifestWarnings(), the single source of truth both the offline linter and the runtime parser now call, so the two can never disagree. Also fixed parseFocusManifestContent to retry YAML on a strict-JSON parse failure (matching config-lint's own long-standing, more lenient fallback for YAML flow mappings that start with "{"/"[") -- a genuine parity gap this wiring exposed. - Added per-block known-key sets for `settings`/`gate`/`features`/`review`: a typo'd nested key (e.g. `settings.agentPause`, `gate.checkModee`) previously read as `undefined` via direct field access and silently behaved as absent, with no warning anywhere. `features`'s fix also makes its existing docstring -- which already claimed unknown keys "are dropped with a warning" -- true instead of aspirational. - private-config.ts's combineConfigLayersWithMeta previously warned ONLY when the shared-base layer failed to parse; a malformed global-default or per-repo layer was dropped silently. All three layers now warn, naming the candidate path when known. --- packages/loopover-engine/src/config-lint.ts | 68 +--- .../loopover-engine/src/focus-manifest.ts | 307 +++++++++++++++++- src/selfhost/private-config.ts | 54 +-- test/unit/focus-manifest.test.ts | 8 +- test/unit/private-config.test.ts | 31 ++ test/unit/selfhost-config-lint.test.ts | 9 +- 6 files changed, 391 insertions(+), 86 deletions(-) diff --git a/packages/loopover-engine/src/config-lint.ts b/packages/loopover-engine/src/config-lint.ts index 6a5b84dcc3..2f45456c03 100644 --- a/packages/loopover-engine/src/config-lint.ts +++ b/packages/loopover-engine/src/config-lint.ts @@ -1,37 +1,12 @@ import { parse as parseYaml } from "yaml"; -import { MAX_FOCUS_MANIFEST_BYTES, parseFocusManifestContent, type FocusManifestGateConfig } from "./focus-manifest.js"; +import { + FOCUS_MANIFEST_TOP_LEVEL_FIELDS, + MAX_FOCUS_MANIFEST_BYTES, + parseFocusManifestContent, + unknownTopLevelManifestWarnings, + type FocusManifestGateConfig, +} from "./focus-manifest.js"; -const TOP_LEVEL_FIELDS = [ - "source", - "wantedPaths", - "preferredLabels", - "linkedIssuePolicy", - "testExpectations", - "issueDiscoveryPolicy", - "maintainerNotes", - "publicNotes", - "gate", - "settings", - "review", - "features", - "experimental", - "contentLane", - "repoDocGeneration", - "reviewRecap", - "maintainerRecap", - "ops", - "publicStats", - "draftFlow", - "upstreamDriftIssues", - "sweepWatchdog", - "prReconciliation", - "activeReviewReconciliation", - "loopEscalation", - "federatedIntelligence", - "fairnessAnalytics", -] as const; - -const TOP_LEVEL_FIELD_SET = new Set(TOP_LEVEL_FIELDS); const NO_RECOGNIZED_FOCUS_FIELDS_WARNING = "Manifest contained no recognized focus fields; falling back to deterministic signals."; @@ -43,13 +18,15 @@ export type SelfHostConfigLintResult = { }; export function lintManifestText(text: string | null | undefined): SelfHostConfigLintResult { + // #9065: parseFocusManifestContent's own manifest.warnings NOW already carries the unknown-top-level-field + // warnings too (parseFocusManifest calls unknownTopLevelManifestWarnings itself), so this no longer needs + // its own separate `...unknownTopLevelWarnings(text)` concatenation -- doing so would double the warning. const manifest = parseFocusManifestContent(text, "repo_file"); const recognizedFields = recognizedFieldsFor(text); const warnings = [ ...manifest.warnings .map(redactManifestWarning) .filter((warning) => recognizedFields.length === 0 || warning !== NO_RECOGNIZED_FOCUS_FIELDS_WARNING), - ...unknownTopLevelWarnings(text), ...mergeReadinessCompositeWarnings(manifest.gate), ]; if (warnings.length === 0 && recognizedFields.length === 0) { @@ -69,7 +46,7 @@ export function lintManifestText(text: string | null | undefined): SelfHostConfi function recognizedFieldsFor(text: string | null | undefined): string[] { const parsed = parseManifestTopLevelObject(text); if (parsed === null) return []; - return TOP_LEVEL_FIELDS.filter( + return FOCUS_MANIFEST_TOP_LEVEL_FIELDS.filter( (field) => field !== "source" && Object.prototype.hasOwnProperty.call(parsed, field), ); } @@ -107,21 +84,15 @@ function mergeReadinessCompositeWarnings(gate: FocusManifestGateConfig): string[ ]; } +/** Standalone (pre-parse) unknown-top-level-field check over raw manifest TEXT, kept for existing direct + * callers (the offline validator route, `loopover_validate_config`, the admin dry-run write, the CLI lint + * script) that want this signal without running the full parse. Delegates the actual "which keys are + * unknown" decision to `unknownTopLevelManifestWarnings` (focus-manifest.ts) -- the SAME function the + * runtime parser now calls (#9065) -- so the two can never disagree. */ export function unknownTopLevelWarnings(text: string | null | undefined): string[] { const parsed = parseManifestTopLevelObject(text); if (parsed === null) return []; - const keys = Object.keys(parsed).filter((key) => !TOP_LEVEL_FIELD_SET.has(key)); - // `hasOwnProperty.call`, NOT `key in`: a manifest field named like an Object.prototype member - // (`constructor`, `toString`, `hasOwnProperty`, ...) would otherwise test true for the inherited - // property and resolve to the prototype's function instead of a real retired-field warning string, - // corrupting the string[] result and suppressing the genuine unknown-field warning. - const isRetired = (key: string): boolean => Object.prototype.hasOwnProperty.call(RETIRED_FIELD_MIGRATION_WARNINGS, key); - const retiredWarnings = keys.filter(isRetired).map((key) => RETIRED_FIELD_MIGRATION_WARNINGS[key]!); - const unknown = keys.filter((key) => !isRetired(key)).map(formatFieldName); - return [ - ...retiredWarnings, - ...(unknown.length > 0 ? [`Manifest contains unknown top-level field${unknown.length === 1 ? "" : "s"}: ${unknown.join(", ")}.`] : []), - ]; + return unknownTopLevelManifestWarnings(parsed); } // Single top-level-object parser shared by both `recognizedFieldsFor` and `unknownTopLevelWarnings` so the two @@ -157,11 +128,6 @@ function isOversize(text: string): boolean { return text.length > MAX_FOCUS_MANIFEST_BYTES || new TextEncoder().encode(text).byteLength > MAX_FOCUS_MANIFEST_BYTES; } -function formatFieldName(name: string): string { - const trimmed = name.replace(/[^\w.-]/g, "_").slice(0, 80); - return trimmed || ""; -} - function redactManifestWarning(warning: string): string { return warning .replace(/; ignoring "[^"]*"\./g, "; ignoring the supplied value.") diff --git a/packages/loopover-engine/src/focus-manifest.ts b/packages/loopover-engine/src/focus-manifest.ts index d618638da0..80d4b993cb 100644 --- a/packages/loopover-engine/src/focus-manifest.ts +++ b/packages/loopover-engine/src/focus-manifest.ts @@ -315,6 +315,7 @@ export const CONVERGED_FEATURE_KEYS = [ "amsReputationBridge", ] as const; export type ConvergedFeatureKey = (typeof CONVERGED_FEATURE_KEYS)[number]; +const CONVERGED_FEATURE_KEYS_SET = new Set(CONVERGED_FEATURE_KEYS); /** Per-repo activation overrides for the converged review features (`features:` block). `true`/`false` force the * feature on/off for THIS repo (subject to the env kill-switch); `null` (unset) ⇒ the resolver falls back to the @@ -1752,6 +1753,42 @@ function normalizeOptionalAdvisoryCheckRuns( return out.length > 0 ? out : null; } +// Every top-level `gate.*` key parseGateConfig below actually reads (#9065). Nested sub-blocks +// (gate.readiness.*, gate.aiReview.*, gate.slop.*, gate.copycat.*, gate.size.*, gate.cla.*) are each +// already a fully-enumerated mapping read field-by-field above/below, and a typo INSIDE one of those is +// covered by that sub-block's own explicit field reads -- this set only catches a typo of the sub-block +// KEY ITSELF (e.g. `gate.readinesss`) or of a scalar gate.* field, matching the concrete typo table in #9065 +// (every example there is exactly one level deep). +const GATE_TOP_LEVEL_KEYS = new Set([ + "enabled", + "checkMode", + "pack", + "linkedIssue", + "duplicates", + "readiness", + "aiReview", + "closeAuditHoldoutPct", + "slop", + "size", + "lockfileIntegrity", + "mergeReadiness", + "manifestPolicy", + "selfAuthoredLinkedIssue", + "linkedIssueSatisfaction", + "contentLaneDeliverable", + "backtestRegression", + "dryRun", + "premergeContentRecheck", + "requireFreshRebaseWindow", + "staleBaseAheadByThreshold", + "claMode", + "cla", + "expectedCiContexts", + "advisoryCheckRuns", + "aiJudgmentBlockers", + "copycat", +]); + /** * Parse the optional `gate:` mapping. Every field stays `null` when unset so the resolver can layer * this OVER DB settings without clobbering. A nested `readiness: { mode, minScore }` block is accepted. @@ -1905,6 +1942,13 @@ function parseGateConfig(value: JsonValue | undefined, warnings: string[]): Focu gate.aiJudgmentBlockersMode !== null || gate.copycatMode !== null || gate.copycatMinScore !== null; + // #9065: a typo'd top-level gate.* key (e.g. `gate.checkModee`) previously read as `undefined` via + // `record.` and produced no warning at all -- the gate block simply behaved as if that field were + // absent. Compare the SUPPLIED keys against the ones this parser actually reads instead. + const unknownGateKeys = Object.keys(record).filter((key) => !GATE_TOP_LEVEL_KEYS.has(key)); + if (unknownGateKeys.length > 0) { + warnings.push(`Manifest "gate" has unknown key${unknownGateKeys.length === 1 ? "" : "s"}: ${unknownGateKeys.join(", ")}; ignoring ${unknownGateKeys.length === 1 ? "it" : "them"}.`); + } return gate; } @@ -2022,6 +2066,14 @@ function parseFeaturesConfig(value: JsonValue | undefined, warnings: string[]): features[key] = normalizeOptionalBoolean(record[key], `features.${key}`, warnings); } features.present = CONVERGED_FEATURE_KEYS.some((key) => features[key] !== null); + // #9065: this docstring has always claimed unknown feature keys "are dropped with a warning", but nothing + // below ever compared the SUPPLIED keys against CONVERGED_FEATURE_KEYS -- a typo'd feature name (e.g. + // `improvementSignall`) silently read as `undefined` via `record[key]` and produced no warning at all, + // making the doc comment factually wrong. This makes the code match the doc instead of the reverse. + const unknownFeatureKeys = Object.keys(record).filter((key) => !CONVERGED_FEATURE_KEYS_SET.has(key)); + if (unknownFeatureKeys.length > 0) { + warnings.push(`Manifest "features" has unknown key${unknownFeatureKeys.length === 1 ? "" : "s"}: ${unknownFeatureKeys.join(", ")}; ignoring ${unknownFeatureKeys.length === 1 ? "it" : "them"}.`); + } return features; } @@ -2563,6 +2615,100 @@ function normalizeOptionalString(value: JsonValue | undefined, field: string, wa // GitHub-App-specific dependency chain into the UI build for one small constant. const MAX_REVIEW_NAG_COOLDOWN_DAYS = 365; +// Every top-level `settings.*` key parseSettingsOverride below actually reads (#9065) -- mechanically copied +// from FocusManifestSettings' own `Pick` union plus its six explicitly-declared +// sparse-partial fields (typeLabels et al.), so this is the SAME field list the type already commits to, not +// an independently-guessed one. Keep in sync with FocusManifestSettings above when either changes. As with +// GATE_TOP_LEVEL_KEYS/REVIEW_TOP_LEVEL_KEYS, a typo INSIDE a nested sub-block (settings.autonomy.*, +// settings.typeLabels.*, ...) is a separate concern from this top-level check. +const SETTINGS_TOP_LEVEL_KEYS = new Set([ + "commentMode", + "publicAudienceMode", + "publicSignalLevel", + "checkRunMode", + "checkRunDetailLevel", + "regateSweepOrderMode", + "reviewCheckMode", + "autoProjectMilestoneMatch", + "autoProjectMilestoneMatchBackend", + "linkedIssueGateMode", + "duplicatePrGateMode", + "selfAuthoredLinkedIssueGateMode", + "qualityGateMode", + "qualityGateMinScore", + "aiReviewMode", + "aiReviewByok", + "aiReviewProvider", + "aiReviewModel", + "aiReviewAllAuthors", + "aiReviewConfirmedContributorsOnly", + "closeOwnerAuthors", + "skipAutomationBotAuthors", + "duplicateWinnerMode", + "openPrFileCollisionMode", + "plannerMode", + "autoLabelEnabled", + "typeLabelsEnabled", + "issuePlanEnabled", + "issuePlanExtraLabels", + "issuePlanMilestoneReuse", + "badgeEnabled", + "publicQualityMetrics", + "gittensorLabel", + "createMissingLabel", + "publicSurface", + "includeMaintainerAuthors", + "requireLinkedIssue", + "backfillEnabled", + "autonomy", + "autoMaintain", + "agentPaused", + "agentDryRun", + "commandAuthorization", + "contributorBlacklist", + "blacklistLabel", + "contributorOpenPrCap", + "contributorOpenIssueCap", + "contributorCapLabel", + "contributorCapCancelCi", + "reviewNagPolicy", + "reviewNagMaxPings", + "reviewNagCooldownDays", + "reviewNagLabel", + "reviewNagMonitoredMentions", + "autoCloseExemptLogins", + "hardGuardrailGlobs", + "hardGuardrailGlobsOverridesInvariants", + "manualReviewLabel", + "readyToMergeLabel", + "changesRequestedLabel", + "migrationCollisionLabel", + "pendingClosureLabel", + "accountAgeThresholdDays", + "newAccountLabel", + "commandRateLimitPolicy", + "commandRateLimitMaxPerWindow", + "commandRateLimitAiMaxPerWindow", + "commandRateLimitWindowHours", + "moderationGateMode", + "moderationRules", + "moderationWarningLabel", + "moderationBannedLabel", + "fairnessAnalyticsMode", + "reviewEvasionProtection", + "draftPrClosePolicy", + "reviewEvasionLabel", + "reviewEvasionComment", + "synchronizeClosePolicy", + "mergeTrainMode", + "typeLabels", + "linkedIssueLabelPropagation", + "linkedIssueHardRules", + "unlinkedIssueGuardrail", + "screenshotTableGate", + "advisoryAiRouting", +]); + /** * Parse the optional `settings:` mapping — a partial repository-settings override. Only recognized * fields are kept; unknown/invalid values are dropped with a warning and never throw. @@ -3013,6 +3159,14 @@ function parseSettingsOverride(value: JsonValue | undefined, warnings: string[]) if (synchronizeClosePolicy !== null) out.synchronizeClosePolicy = synchronizeClosePolicy; const mergeTrainMode = normalizeOptionalEnum(r.mergeTrainMode, "settings.mergeTrainMode", ["off", "audit", "enforce"] as const, warnings); if (mergeTrainMode !== null) out.mergeTrainMode = mergeTrainMode; + // #9065: a typo'd top-level settings.* key (e.g. `settings.agentPause` or `settings.hardGuardrailGlob`) + // previously read as `undefined` via `r.` and produced NO warning at all -- silently disabling + // whatever safety control that field controls (the kill switch, the manual-review label match, an + // auto-close exemption, ...) with the operator having no way to know their config typo did nothing. + const unknownSettingsKeys = Object.keys(r).filter((key) => !SETTINGS_TOP_LEVEL_KEYS.has(key)); + if (unknownSettingsKeys.length > 0) { + warnings.push(`Manifest "settings" has unknown key${unknownSettingsKeys.length === 1 ? "" : "s"}: ${unknownSettingsKeys.join(", ")}; ignoring ${unknownSettingsKeys.length === 1 ? "it" : "them"}.`); + } return out; } @@ -3035,6 +3189,49 @@ function parsePublicSafeText(value: JsonValue | undefined, field: string, warnin return bounded; } +// Every top-level `review.*` key parseReviewConfig below actually reads (#9065), mirroring GATE_TOP_LEVEL_KEYS' +// doc comment: a typo INSIDE a nested sub-block (review.fields.*, review.auto_review.*, review.visual.*, ...) +// is covered by that sub-block's own field-by-field reads (`review.enrichment` already warns per-key, see +// REES_ANALYZER_NAME_SET above); this set only catches a typo of the sub-block key itself or a scalar field. +const REVIEW_TOP_LEVEL_KEYS = new Set([ + "footer", + "fields", + "enrichment", + "note", + "profile", + "tone", + "security_focus", + "inline_comments", + "fixHandoff", + "auto_merge_summary", + "suggestions", + "changed_files_summary", + "effort_score", + "impact_map", + "culture_profile", + "selftune", + "sweepWatchdog", + "prReconciliation", + "activeReviewReconciliation", + "memory", + "finding_categories", + "inline_comments_per_category", + "min_finding_severity", + "max_findings", + "comment_verbosity", + "e2e_test_delivery", + "e2e_test_auto_trigger", + "path_instructions", + "instructions", + "exclude_paths", + "path_filters", + "pre_merge_checks", + "auto_review", + "ai_model", + "visual", + "linkedIssueSatisfaction", +]); + /** * Parse the optional `review:` block — maintainer overrides for the public review-panel content. Never * throws; invalid/unsafe values are dropped with warnings. @@ -3114,6 +3311,12 @@ function parseReviewConfig(value: JsonValue | undefined, warnings: string[]): Fo const aiModel = parseSelfHostAiModelConfig(r.ai_model, warnings); const visual = parseVisualConfig(r.visual, warnings); const linkedIssueSatisfaction = normalizeOptionalEnum(r.linkedIssueSatisfaction, "review.linkedIssueSatisfaction", LINKED_ISSUE_SATISFACTION_MODES, warnings); + // #9065: a typo'd top-level review.* key (e.g. `review.toen`) previously read as `undefined` via `r.` + // and produced no warning at all. Compare the SUPPLIED keys against the ones this parser actually reads. + const unknownReviewKeys = Object.keys(r).filter((key) => !REVIEW_TOP_LEVEL_KEYS.has(key)); + if (unknownReviewKeys.length > 0) { + warnings.push(`Manifest "review" has unknown key${unknownReviewKeys.length === 1 ? "" : "s"}: ${unknownReviewKeys.join(", ")}; ignoring ${unknownReviewKeys.length === 1 ? "it" : "them"}.`); + } return { present: footerText !== null || @@ -4024,6 +4227,74 @@ export function reviewConfigToJson(review: FocusManifestReviewConfig): JsonValue return out; } +// Every top-level key `parseFocusManifest` below actually reads. Single source of truth for "unknown +// top-level field" detection (#9065) -- previously duplicated as a hand-maintained TOP_LEVEL_FIELDS list +// inside config-lint.ts, used ONLY by the offline linter/validator, never by this runtime parser itself, so +// a manifest edited directly on a mounted host file (bypassing the validator entirely) got no check at all. +export const FOCUS_MANIFEST_TOP_LEVEL_FIELDS = [ + "source", + "wantedPaths", + "preferredLabels", + "linkedIssuePolicy", + "testExpectations", + "issueDiscoveryPolicy", + "maintainerNotes", + "publicNotes", + "gate", + "settings", + "review", + "features", + "experimental", + "contentLane", + "repoDocGeneration", + "reviewRecap", + "maintainerRecap", + "ops", + "publicStats", + "draftFlow", + "upstreamDriftIssues", + "sweepWatchdog", + "prReconciliation", + "activeReviewReconciliation", + "loopEscalation", + "federatedIntelligence", + "fairnessAnalytics", +] as const; + +const FOCUS_MANIFEST_TOP_LEVEL_FIELD_SET = new Set(FOCUS_MANIFEST_TOP_LEVEL_FIELDS); + +// Fields retired from FOCUS_MANIFEST_TOP_LEVEL_FIELDS that still warrant a migration-specific warning +// (rather than the generic "unknown field" message) pointing operators at their replacement mechanism. +const RETIRED_TOP_LEVEL_FIELD_MIGRATIONS: Record = { + blockedPaths: "blockedPaths is retired; use settings.hardGuardrailGlobs for path holds.", +}; + +function formatUnknownFieldName(name: string): string { + const trimmed = name.replace(/[^\w.-]/g, "_").slice(0, 80); + return trimmed || ""; +} + +/** + * Unknown-top-level-field warnings for an ALREADY-PARSED manifest record. Shared by the runtime parser + * (`parseFocusManifest` below, wired in for #9065) and the offline linter (`config-lint.ts`'s + * `unknownTopLevelWarnings`, which parses raw text into a record first, then delegates here) so the two can + * never disagree about which top-level keys are recognized. + */ +export function unknownTopLevelManifestWarnings(record: Record): string[] { + const keys = Object.keys(record).filter((key) => !FOCUS_MANIFEST_TOP_LEVEL_FIELD_SET.has(key)); + // `hasOwnProperty.call`, NOT `key in`: a manifest field named like an Object.prototype member + // (`constructor`, `toString`, `hasOwnProperty`, ...) would otherwise test true for the inherited + // property and resolve to the prototype's function instead of a real retired-field warning string, + // corrupting the string[] result and suppressing the genuine unknown-field warning. + const isRetired = (key: string): boolean => Object.prototype.hasOwnProperty.call(RETIRED_TOP_LEVEL_FIELD_MIGRATIONS, key); + const retiredWarnings = keys.filter(isRetired).map((key) => RETIRED_TOP_LEVEL_FIELD_MIGRATIONS[key]!); + const unknown = keys.filter((key) => !isRetired(key)).map(formatUnknownFieldName); + return [ + ...retiredWarnings, + ...(unknown.length > 0 ? [`Manifest contains unknown top-level field${unknown.length === 1 ? "" : "s"}: ${unknown.join(", ")}.`] : []), + ]; +} + /** * Resolve the `review.path_instructions` that APPLY to a PR — those whose glob matches at least one changed path * — into a single prompt section for the AI reviewer, or "" when none match (so the prompt stays byte-identical). @@ -4100,6 +4371,14 @@ export function parseFocusManifest(raw: unknown, source?: FocusManifestSource): warnings.push("Manifest contained no recognized focus fields; falling back to deterministic signals."); manifest.present = false; } + // #9065: previously only the OFFLINE validator (config-lint.ts's unknownTopLevelWarnings, called from + // loopover_validate_config / the validate route / the admin dry-run write / the lint script) ever ran this + // check -- an operator editing the mounted manifest file directly (the normal self-host workflow) got zero + // feedback on a typo'd top-level key. Wiring it in here means every caller of parseFocusManifest / + // parseFocusManifestContent -- including the runtime load path (loadRepoFocusManifest) -- gets it too. + // Appended LAST (after the "no recognized fields" fallback above) to match the order the offline linter's + // own warnings + this check have always been surfaced in. + warnings.push(...unknownTopLevelManifestWarnings(record)); return manifest; } @@ -4115,14 +4394,26 @@ export function parseFocusManifestContent(content: string | null | undefined, so const trimmed = content.trim(); const looksLikeJson = trimmed.startsWith("{") || trimmed.startsWith("["); let parsed: unknown; - try { - parsed = looksLikeJson ? JSON.parse(trimmed) : parseYaml(trimmed); - } catch { - return emptyManifest(source, [ - looksLikeJson - ? "Manifest content was not valid JSON; ignoring it and falling back to deterministic signals." - : "Manifest content was not valid YAML; ignoring it and falling back to deterministic signals.", - ]); + if (looksLikeJson) { + try { + parsed = JSON.parse(trimmed); + } catch { + // #9065: a YAML flow mapping (e.g. unquoted keys) can start with "{"/"[" while being invalid strict + // JSON -- retry as YAML before giving up, matching config-lint.ts's own (offline-only, until now) + // parseManifestTopLevelObject fallback, so the runtime path recognizes the same manifests the offline + // validator already accepted instead of rejecting them outright as "not valid JSON". + try { + parsed = parseYaml(trimmed); + } catch { + return emptyManifest(source, ["Manifest content was not valid JSON; ignoring it and falling back to deterministic signals."]); + } + } + } else { + try { + parsed = parseYaml(trimmed); + } catch { + return emptyManifest(source, ["Manifest content was not valid YAML; ignoring it and falling back to deterministic signals."]); + } } if (parsed === null || typeof parsed !== "object" || Array.isArray(parsed)) { return emptyManifest(source, ["Manifest must be a mapping of fields; ignoring malformed manifest and falling back to deterministic signals."]); diff --git a/src/selfhost/private-config.ts b/src/selfhost/private-config.ts index a77aab52eb..e1d6575f44 100644 --- a/src/selfhost/private-config.ts +++ b/src/selfhost/private-config.ts @@ -13,8 +13,8 @@ // 4. `.loopover.yml` — GLOBAL default at the dir root, shared by every repo. // 5. `_shared/.loopover.yml` — SHARED BASE (#1959), the lowest-priority layer: one house policy // an operator running many repos writes once instead of copy-pasting into every repo's private config. -// `.yaml` / `.json` are accepted everywhere `.yml` is (see CONFIG_BASENAMES). `readFirstExisting` (and its -// `WithPath` sibling) return the first candidate that exists, in list order. With only ONE of {a per-repo +// `.yaml` / `.json` are accepted everywhere `.yml` is (see CONFIG_BASENAMES). `readFirstExistingWithPath` +// returns the first candidate that exists, in list order. With only ONE of {a per-repo // candidate, the global default, the shared base} present, its raw text is returned unchanged — byte-identical to // the original #1390 behavior (and to the pre-#1959 2-layer behavior when no shared base is mounted, the common // case). With more than one present, they are DEEP-MERGED in ascending priority (shared base → global default → @@ -48,8 +48,8 @@ import type { /** The bare config filenames tried inside a per-repo folder and at the dir root (global default), in priority * order. Every helper below (`GLOBAL_CONFIG_CANDIDATES`, `SHARED_BASE_CONFIG_CANDIDATES`, `localConfigCandidates`) - * derives its search order from this array's order, and `readFirstExisting` / `readFirstExistingWithPath` return - * the first candidate that EXISTS. */ + * derives its search order from this array's order, and `readFirstExistingWithPath` returns the first + * candidate that EXISTS. */ const CONFIG_BASENAMES = [".loopover.yml", ".loopover.yaml", ".loopover.json"] as const; /** The extensions accepted by CONFIG_BASENAMES, in first-seen order: `.yml`, `.yaml`, `.json`. Used only to build @@ -97,14 +97,10 @@ export function localConfigCandidates(repoFullName: string): string[] { ]; } -/** Read the first candidate that exists, trying each in order; null when none do. A read error (ENOENT or - * otherwise unreadable) is swallowed so the next candidate is tried. */ -async function readFirstExisting(base: string, candidates: string[]): Promise { - const hit = await readFirstExistingWithPath(base, candidates); - return hit?.text ?? null; -} - -/** Like {@link readFirstExisting}, but also returns the winning relative candidate path (for provenance). */ +/** Read the first candidate that exists, trying each in order, returning both its text and its winning + * relative candidate path (for provenance); null when none do. A read error (ENOENT or otherwise + * unreadable) is swallowed so the next candidate is tried. #9065: every caller now wants the path (to name + * a dropped layer in a warning), so the former text-only `readFirstExisting` wrapper was retired. */ async function readFirstExistingWithPath( base: string, candidates: string[], @@ -194,13 +190,27 @@ function combineConfigLayersWithMeta( const present = layersAscendingPriority.filter((layer): layer is { text: string; kind: ConfigLayerKind; sourcePath?: string | null } => layer.text !== null); if (present.length === 0) return { content: null, sharedConfigSource: null, warnings }; - const sharedLayer = present.find((layer) => layer.kind === "shared"); - if (sharedLayer && parseConfigMapping(sharedLayer.text) === null) warnings.push(SHARED_BASE_MALFORMED_WARNING); - const parsedLayers: Array<{ text: string; kind: ConfigLayerKind; mapping: Record; sourcePath: string | null }> = []; for (const layer of present) { const mapping = parseConfigMapping(layer.text); - if (mapping) parsedLayers.push({ text: layer.text, kind: layer.kind, mapping, sourcePath: layer.sourcePath ?? null }); + if (mapping) { + parsedLayers.push({ text: layer.text, kind: layer.kind, mapping, sourcePath: layer.sourcePath ?? null }); + continue; + } + // #9065: previously ONLY the shared-base layer's parse failure was ever warned about (the dedicated + // `sharedLayer` check this replaced); a malformed GLOBAL-default or PER-REPO layer was silently dropped + // right here with no warning, no log, no metric -- that repo silently demoted to whatever layer(s) + // remained (or fleet house policy, if none did) with a clean-looking manifest and zero signal anything + // was wrong. Every dropped layer now warns, carrying its candidate path when known. + if (layer.kind === "shared") { + warnings.push(SHARED_BASE_MALFORMED_WARNING); + } else { + const layerLabel = layer.kind === "global" ? "global-default" : "per-repo"; + const pathSuffix = layer.sourcePath ? ` (\`${layer.sourcePath}\`)` : ""; + warnings.push( + `Container-private ${layerLabel} manifest${pathSuffix} is malformed or oversized; ignoring it and continuing.`, + ); + } } if (parsedLayers.length === 0) { @@ -246,15 +256,17 @@ export function makeLocalManifestReader(dir: string | undefined): RepoFocusManif return async (repoFullName: string): Promise => { const perRepo = localConfigCandidates(repoFullName); if (perRepo.length === 0) return null; // invalid repo name → no per-repo file, global default, or shared base - const [sharedHit, globalText, repoText] = await Promise.all([ + const [sharedHit, globalHit, repoHit] = await Promise.all([ readFirstExistingWithPath(base, SHARED_BASE_CONFIG_CANDIDATES), - readFirstExisting(base, GLOBAL_CONFIG_CANDIDATES), - readFirstExisting(base, perRepo), + readFirstExistingWithPath(base, GLOBAL_CONFIG_CANDIDATES), + readFirstExistingWithPath(base, perRepo), ]); const loaded = combineConfigLayersWithMeta([ { text: sharedHit?.text ?? null, kind: "shared", sourcePath: sharedHit?.path ?? null }, - { text: globalText, kind: "global" }, - { text: repoText, kind: "repo" }, + // #9065: thread the winning candidate path through for these two layers as well (previously only the + // shared layer carried one) so a malformed-layer warning can name WHICH file was dropped. + { text: globalHit?.text ?? null, kind: "global", sourcePath: globalHit?.path ?? null }, + { text: repoHit?.text ?? null, kind: "repo", sourcePath: repoHit?.path ?? null }, ]); if (loaded.content === null && loaded.warnings.length === 0 && loaded.sharedConfigSource === null) return null; return loaded; diff --git a/test/unit/focus-manifest.test.ts b/test/unit/focus-manifest.test.ts index 0a86e87342..f17e03c124 100644 --- a/test/unit/focus-manifest.test.ts +++ b/test/unit/focus-manifest.test.ts @@ -95,7 +95,10 @@ describe("parseFocusManifest", () => { issueDiscoveryPolicy: "discouraged", publicNotes: ["Prefer small, focused PRs."], }); - expect(manifest.warnings).toEqual([]); + // FULL_MANIFEST's legacy `blockedPaths` (retained here deliberately -- see "ignores legacy blockedPaths" + // below) is a RETIRED top-level field (#9065): parseFocusManifest now surfaces its migration warning + // instead of silently accepting it, matching the offline validator's own long-standing behavior. + expect(manifest.warnings).toEqual(["blockedPaths is retired; use settings.hardGuardrailGlobs for path holds."]); }); it("treats null/undefined as an absent manifest", () => { @@ -876,7 +879,8 @@ describe("compileFocusManifestPolicy", () => { // authenticated: private note count, no maintainer text in publicSafe expect(policy.authenticated.privateNoteCount).toBe(1); - expect(policy.authenticated.parseWarnings).toEqual([]); + // Same legacy `blockedPaths` migration warning as "normalizes a fully specified manifest" above (#9065). + expect(policy.authenticated.parseWarnings).toEqual(["blockedPaths is retired; use settings.hardGuardrailGlobs for path holds."]); }); // ── Missing-field: partial manifest ─────────────────────────────────── diff --git a/test/unit/private-config.test.ts b/test/unit/private-config.test.ts index 6196578edc..d2333c3ed0 100644 --- a/test/unit/private-config.test.ts +++ b/test/unit/private-config.test.ts @@ -434,6 +434,37 @@ describe("makeLocalManifestReader — review.shared_config overlay (#2046)", () expect(parseFocusManifestContent(loaded!.content!).review.tone).toBe("repo-tone"); }); + // #9065: previously ONLY the shared-base layer's own parse failure was ever warned about -- a malformed + // global-default or per-repo layer was silently dropped with no warning, no log, no metric, so that repo + // silently demoted to whatever layer(s) remained with a clean-looking manifest. + it("warns (naming the candidate path) and ignores a malformed GLOBAL-default layer while still serving the shared base and per-repo layer", async () => { + const dir = mkdtempSync(join(tmpdir(), "gt-repo-config-")); + mkdirSync(join(dir, "_shared")); + writeFileSync(join(dir, "_shared", ".loopover.yml"), "review:\n tone: house-tone\n"); + writeFileSync(join(dir, ".loopover.yml"), "{ broken global"); + mkdirSync(join(dir, "repo")); + writeFileSync(join(dir, "repo", ".loopover.yml"), "review:\n profile: assertive\n"); + const loaded = await readLocalManifestLoad(makeLocalManifestReader(dir)!, "owner/repo"); + expect(loaded?.warnings.some((w) => w.includes("global-default") && w.includes(".loopover.yml"))).toBe(true); + const manifest = parseFocusManifestContent(loaded!.content!); + expect(manifest.review.tone).toBe("house-tone"); // shared base still contributes + expect(manifest.review.profile).toBe("assertive"); // per-repo layer still contributes + }); + + it("warns (naming the candidate path) and ignores a malformed PER-REPO layer while still serving the shared base and global default", async () => { + const dir = mkdtempSync(join(tmpdir(), "gt-repo-config-")); + mkdirSync(join(dir, "_shared")); + writeFileSync(join(dir, "_shared", ".loopover.yml"), "review:\n tone: house-tone\n"); + writeFileSync(join(dir, ".loopover.yml"), "gate:\n enabled: true\n"); + mkdirSync(join(dir, "repo")); + writeFileSync(join(dir, "repo", ".loopover.yml"), "{ broken repo"); + const loaded = await readLocalManifestLoad(makeLocalManifestReader(dir)!, "owner/repo"); + expect(loaded?.warnings.some((w) => w.includes("per-repo") && w.includes(join("repo", ".loopover.yml")))).toBe(true); + const manifest = parseFocusManifestContent(loaded!.content!); + expect(manifest.review.tone).toBe("house-tone"); // shared base still contributes + expect(manifest.gate.enabled).toBe(true); // global default still contributes + }); + it("fills review fields from the shared base when the per-repo file is silent on them", async () => { const dir = mkdtempSync(join(tmpdir(), "gt-repo-config-")); mkdirSync(join(dir, "_shared")); diff --git a/test/unit/selfhost-config-lint.test.ts b/test/unit/selfhost-config-lint.test.ts index fdd3a415a2..624769faf9 100644 --- a/test/unit/selfhost-config-lint.test.ts +++ b/test/unit/selfhost-config-lint.test.ts @@ -292,10 +292,11 @@ unknownSecretKey: super-secret-value // recognizedFieldsFor now applies the same JSON->YAML fallback as unknownTopLevelWarnings, so a flow-mapping // manifest that is valid YAML but invalid strict JSON reports its real recognized fields instead of []. expect(result.recognizedFields).toEqual(["wantedPaths"]); - expect(result.warnings).toEqual([ - "Manifest content was not valid JSON; ignoring it and falling back to deterministic signals.", - "Manifest contains unknown top-level field: unknownSecretKey.", - ]); + // #9065: parseFocusManifestContent (the runtime parser lintManifestText now delegates its warnings to) + // ALSO retries YAML on a strict-JSON parse failure, matching this offline check's own long-standing + // fallback -- so this content is now genuinely accepted (no more spurious "not valid JSON" warning) and + // only the real unknown-field warning survives. + expect(result.warnings).toEqual(["Manifest contains unknown top-level field: unknownSecretKey."]); expect(JSON.stringify(result)).not.toContain("secret"); }); From d128a879f3f3758e4db089333a92c43763bc95d5 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 27 Jul 2026 01:42:06 -0700 Subject: [PATCH 6/7] fix(config): surface private-manifest warnings via adminGetConfig and structured logs (#9065) loopover_admin_get_config --scope effective previously extracted only the loaded manifest's content and silently discarded its warnings -- an operator could ask "what's effectively loaded for this repo" and see a clean-looking config even when a layer had been dropped as malformed or the merged content carried an unknown-key parse warning. Now returns warnings alongside content. makeLocalManifestReader's own dropped-layer warnings previously had no consumer outside that one return value; they're now also logged as a structured event (selfhost_private_manifest_warnings) and counted (loopover_private_manifest_warnings_total) so a sustained run of dropped layers is visible on a dashboard, not just discoverable after the fact. Also fixes a duplicate-warning bug the #9065 runtime wiring exposed: buildFocusManifestValidation independently re-computed unknownTopLevelWarnings(content) on top of manifest.warnings, which now already includes it via parseFocusManifest's own call -- doubling the same warning in loopover_validate_config, the validate route, and the admin dry-run write. --- .../src/focus-manifest-validation.ts | 11 ++++---- src/mcp/server.ts | 14 ++++++++-- src/selfhost/private-config.ts | 11 ++++++++ test/unit/focus-manifest-validation.test.ts | 10 +++++++ test/unit/mcp-admin-config-tools.test.ts | 28 +++++++++++++++++++ 5 files changed, 67 insertions(+), 7 deletions(-) diff --git a/packages/loopover-engine/src/focus-manifest-validation.ts b/packages/loopover-engine/src/focus-manifest-validation.ts index 1c44f92369..f11589ca18 100644 --- a/packages/loopover-engine/src/focus-manifest-validation.ts +++ b/packages/loopover-engine/src/focus-manifest-validation.ts @@ -22,7 +22,6 @@ import { type FocusManifest, type FocusManifestSource, } from "./focus-manifest.js"; -import { unknownTopLevelWarnings } from "./config-lint.js"; export type FocusManifestValidationStatus = "ok" | "warn" | "error"; @@ -40,10 +39,12 @@ export function buildFocusManifestValidation(input: { source?: FocusManifestSource | undefined; }): FocusManifestValidationResult { const manifest = parseFocusManifestContent(input.content, input.source ?? "repo_file"); - // Warn on unrecognized top-level fields (e.g. a typo'd `gates:` instead of `gate:`), matching the - // selfhost config-lint validator — parseFocusManifestContent reads only known fields, so a mistyped - // block is otherwise silently dropped with no warning (#5929). - const warnings = [...manifest.warnings, ...unknownTopLevelWarnings(input.content)]; + // #9065: parseFocusManifestContent's own manifest.warnings now ALREADY carries the unrecognized-top-level- + // field warnings (e.g. a typo'd `gates:` instead of `gate:`) -- parseFocusManifest calls + // unknownTopLevelManifestWarnings itself, wired in for the runtime load path, not just this offline + // validator (#5929). Concatenating a second, independent `unknownTopLevelWarnings(input.content)` call + // here would double the same warning; manifest.warnings alone is now sufficient. + const warnings = manifest.warnings; const normalized = focusManifestToNormalizedJson(manifest); return { present: manifest.present, diff --git a/src/mcp/server.ts b/src/mcp/server.ts index 6c383d198d..7acf59b43d 100644 --- a/src/mcp/server.ts +++ b/src/mcp/server.ts @@ -1697,6 +1697,9 @@ const adminGetConfigOutputSchema = { found: z.boolean().optional(), path: z.string().nullable().optional(), content: z.string().nullable().optional(), + // #9065: only ever populated for scope "effective" (a dropped layer or an unknown-key parse warning); the + // "global"/"repo" scope branch reads a raw file with no merge/parse step to warn about. + warnings: z.array(z.string()).optional(), }; const adminWriteConfigOutputSchema = { configured: z.boolean(), @@ -4007,9 +4010,16 @@ export class LoopoverMcp { const reader = getLocalManifestReader(); const loaded = reader ? await reader(repoFullName) : null; const content = typeof loaded === "string" ? loaded : (loaded?.content ?? null); + // #9065: previously extracted ONLY `.content`, discarding `.warnings` entirely -- an operator could ask + // this exact tool "what's effectively loaded for this repo" and see a clean-looking config even when a + // layer (shared/global/per-repo) had been silently dropped as malformed, or when the merged content + // itself carries unknown-key warnings. `loaded` is a plain string (no warnings field) on the LEGACY + // reader shape some tests/older readers still return -- only a LocalManifestLoadResult object carries + // `.warnings`. + const warnings = typeof loaded === "string" || loaded === null ? [] : loaded.warnings; return { - summary: content === null ? `LoopOver admin config: no effective config found for ${repoFullName}.` : `LoopOver admin config: effective config loaded for ${repoFullName}.`, - data: { configured: true, found: content !== null, path: null, content }, + summary: content === null ? `LoopOver admin config: no effective config found for ${repoFullName}.` : `LoopOver admin config: effective config loaded for ${repoFullName}${warnings.length > 0 ? ` (${warnings.length} warning${warnings.length === 1 ? "" : "s"})` : ""}.`, + data: { configured: true, found: content !== null, path: null, content, warnings }, }; } const hit = diff --git a/src/selfhost/private-config.ts b/src/selfhost/private-config.ts index e1d6575f44..3de007f7d3 100644 --- a/src/selfhost/private-config.ts +++ b/src/selfhost/private-config.ts @@ -37,6 +37,7 @@ import { randomUUID } from "node:crypto"; import { basename, dirname, join, resolve } from "node:path"; import { parse as parseYaml } from "yaml"; import { MAX_FOCUS_MANIFEST_BYTES } from "../signals/focus-manifest"; +import { incr } from "./metrics"; import type { RepoReviewContext, RepoReviewSkill, @@ -269,6 +270,16 @@ export function makeLocalManifestReader(dir: string | undefined): RepoFocusManif { text: repoHit?.text ?? null, kind: "repo", sourcePath: repoHit?.path ?? null }, ]); if (loaded.content === null && loaded.warnings.length === 0 && loaded.sharedConfigSource === null) return null; + // #9065: previously a private-manifest layer warning (a dropped malformed shared/global/repo layer) had + // NO consumer at all outside this one return value -- silent unless an operator happened to inspect + // `loopover_admin_config_read --scope effective` (which itself discarded `.warnings`, fixed separately). + // Make it observable proactively: a structured log line (greppable/alertable) plus a counter (so a + // sustained run of dropped layers -- e.g. a bad `docker cp` truncating a mount repeatedly -- is visible + // on a dashboard, not just discoverable after the fact). + if (loaded.warnings.length > 0) { + console.warn(JSON.stringify({ level: "warn", event: "selfhost_private_manifest_warnings", repoFullName, warnings: loaded.warnings })); + incr("loopover_private_manifest_warnings_total", {}, loaded.warnings.length); + } return loaded; }; } diff --git a/test/unit/focus-manifest-validation.test.ts b/test/unit/focus-manifest-validation.test.ts index 4578b720ed..6c0ba67c8f 100644 --- a/test/unit/focus-manifest-validation.test.ts +++ b/test/unit/focus-manifest-validation.test.ts @@ -199,4 +199,14 @@ loopEscalation: const clean = buildFocusManifestValidation({ content: "wantedPaths:\n - src/\n" }); expect(clean.warnings.join(" ")).not.toMatch(/unknown top-level field/i); }); + + // #9065: parseFocusManifestContent now wires the SAME unknown-top-level-field check into the runtime + // parse path (manifest.warnings), which this validator's own `warnings` used to ALSO compute independently + // via a second unknownTopLevelWarnings(content) call -- regression guard against that reappearing as a + // silent duplicate (both calls would fire on the exact same content and double the warning). + it("does not double-count the unknown-top-level-field warning now that parseFocusManifestContent surfaces it too (#9065)", () => { + const result = buildFocusManifestValidation({ content: "wantedPaths:\n - src/\ngates:\n enabled: true\n" }); + const unknownFieldWarnings = result.warnings.filter((w) => /unknown top-level field/i.test(w)); + expect(unknownFieldWarnings).toHaveLength(1); + }); }); diff --git a/test/unit/mcp-admin-config-tools.test.ts b/test/unit/mcp-admin-config-tools.test.ts index 6d6552ce57..edba996a99 100644 --- a/test/unit/mcp-admin-config-tools.test.ts +++ b/test/unit/mcp-admin-config-tools.test.ts @@ -140,6 +140,34 @@ describe("MCP admin config tools: get (#7721)", () => { const result = await client.callTool({ name: "loopover_admin_get_config", arguments: { scope: "effective", repoFullName: "JSONbored/loopover" } }); expect(result.structuredContent).toMatchObject({ found: false, content: null }); }); + + // #9065: previously this scope extracted ONLY `.content` from the registered reader's result and silently + // discarded `.warnings` -- an operator asking "what's effectively loaded for this repo" saw a clean-looking + // config even when a layer had been dropped as malformed (private-config.ts) or the merged content itself + // carried an unknown-key parse warning. + it("effective scope surfaces the reader's warnings (a dropped layer, or an unknown-key parse warning) instead of discarding them", async () => { + setConfigAdminFunctions({ readGlobal: vi.fn(), readRepo: vi.fn(), writeGlobal: vi.fn(), writeRepo: vi.fn(), listBackups: vi.fn() }); + setLocalManifestReader(async (repoFullName) => ({ + content: `merged:${repoFullName}`, + sharedConfigSource: null, + warnings: ["Container-private global-default manifest (`.loopover.yml`) is malformed or oversized; ignoring it and continuing."], + })); + const client = await connect(createTestEnv({ LOOPOVER_MCP_ADMIN_ENABLED: "true" })); + const result = await client.callTool({ name: "loopover_admin_get_config", arguments: { scope: "effective", repoFullName: "JSONbored/loopover" } }); + expect(result.structuredContent).toMatchObject({ + configured: true, + found: true, + warnings: ["Container-private global-default manifest (`.loopover.yml`) is malformed or oversized; ignoring it and continuing."], + }); + }); + + it("effective scope returns an empty warnings array for the legacy string-only reader shape (no .warnings field to surface)", async () => { + setConfigAdminFunctions({ readGlobal: vi.fn(), readRepo: vi.fn(), writeGlobal: vi.fn(), writeRepo: vi.fn(), listBackups: vi.fn() }); + setLocalManifestReader(async (repoFullName) => `raw:${repoFullName}`); + const client = await connect(createTestEnv({ LOOPOVER_MCP_ADMIN_ENABLED: "true" })); + const result = await client.callTool({ name: "loopover_admin_get_config", arguments: { scope: "effective", repoFullName: "JSONbored/loopover" } }); + expect(result.structuredContent).toMatchObject({ configured: true, found: true, warnings: [] }); + }); }); describe("MCP admin config tools: write (#7721)", () => { From 4d88ff81a7af16611c56a3355810f0fed173cae5 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 27 Jul 2026 01:46:03 -0700 Subject: [PATCH 7/7] fix(engine): exclude sourcemaps from the published package (#9064) #9064 enabled tsconfig.json's "sourceMap": true so scripts/engine-coverage.mjs's c8 remapping could map coverage hits back to src/**.ts paths, but package.json's files: ["dist", ...] bundles the whole directory verbatim, so `npm pack` now also ships every dist/**/*.js.map -- which scripts/check-engine-package.ts's ALLOWED list (dist/*.{js,d.ts} only) correctly flags as unexpected. A bare directory entry in npm's "files" array ignores .npmignore entirely, so the fix is to name the two extensions the published package actually needs instead of the whole directory; the local dist/ build (and its .map files) used by coverage tooling is untouched. --- packages/loopover-engine/package.json | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/loopover-engine/package.json b/packages/loopover-engine/package.json index 67759ae6ba..271a2e4d5a 100644 --- a/packages/loopover-engine/package.json +++ b/packages/loopover-engine/package.json @@ -84,7 +84,8 @@ } }, "files": [ - "dist", + "dist/**/*.js", + "dist/**/*.d.ts", "CHANGELOG.md" ], "scripts": {