fix(review): close four testing/coverage-gate blind spots — TTL-aware fixtures, shutdown parity, invariant sweeps, engine coverage#9245
Merged
Conversation
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
loopover-ui | 4d88ff8 | Commit Preview URL Branch Preview URL |
Jul 27 2026, 09:27 AM |
❌ 17 Tests Failed:
View the top 3 failed test(s) by shortest run time
View the full list of 5 ❄️ flaky test(s)
To view more test analytics, go to the Test Analytics Dashboard |
Bundle ReportChanges will increase total bundle size by 401 bytes (0.01%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: loopover-uiAssets Changed:
|
JSONbored
force-pushed
the
fix/9063-9064-9065-test-gate-integrity
branch
from
July 27, 2026 09:16
040acc4 to
9afeb92
Compare
Contributor
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
…ts 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.
… 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.
…t 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).
…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.
…rser (#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.
… 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.
#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.
JSONbored
force-pushed
the
fix/9063-9064-9065-test-gate-integrity
branch
from
July 27, 2026 09:24
9afeb92 to
4d88ff8
Compare
This was referenced Jul 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Test plan
Closes #9063
Closes #9064
Closes #9065