test(tasks): give the GitHub-coupling assertion a positive control and a live needle

[lilyshen0722]

@sprint-review's finding (58470): tasksApi.githubCouplingRemoved.test.js:41 asserted only that the route source does not match three needles. Rename GitHubAppService and it passes forever with the coupling fully present.

Two gaps, one worse than reported:

1. No positive control. readFileSync throwing covers a missing file; it does not cover a present-but-wrong or truncated one, where the negative match passes for the wrong reason. The test now asserts the source actually looks like the route before concluding anything from an absence.

2. createGithubIssue was a dead needle. It existed nowhere in the tree except inside this file's own regex — it could never match and never fail, so a third of the alternation was decorative. Removed.

Adds a liveness control: every remaining needle must still appear in a named home file (services/githubAppService.ts, scripts/remove-task-github-fields.ts). A negative match passes for two different reasons — the coupling is gone, or the needle no longer names anything — and only this control separates them. Note the home files are named explicitly rather than grepping the tree, because a tree-wide grep would match the test's own needle list and pass trivially.

Mutation-tested; each mutant reddens exactly one test:

needle renamed              -> liveness test fails
coupling reintroduced       -> route test fails
read pointed at wrong file  -> positive control fails

3/3 green restored on Node 22.

🤖 Generated with Claude Code

[lilyshen0722]

Gated at 94e24771. Suite runs green here (3/3, node 22). Your sharper finding reproduces: createGithubIssue had exactly one repo-wide hit on origin/main and it was inside this file's own regex. Worth adding that the branch was doubly dead — githubIssue cannot match inside createGithubIssue under a case-sensitive read either, so nothing in the alternation could ever have reached it.

Positive control is real, not nominal: routes/tasksApi.ts has 6 router.*( matches and 40,930 bytes, so both assertions discriminate. Both needle homes carry their needles (3 hits each).

One escape the liveness control does not close: the import alias.

The negative assertion matches a local identifier. The only live example of this import is routes/github.ts:8:

const GitHubAppService = require('../services/githubAppService');

Nothing forces that local name. If tasksApi.ts wrote:

const GH = require('../services/githubAppService');
// ... GH.createIssue(...)

then GitHubAppService never appears in routeSource, the negative assertion passes, and the liveness control stays green because services/githubAppService.ts still contains the symbol. Coupling fully present, test fully green — the same failure mode you just fixed for renames, one indirection over.

The module path is the invariant: a require/import cannot reach the service without the literal string githubAppService, whatever it binds it to. Suggested third needle:

['githubAppService', 'routes/github.ts'],

Home matters there — services/githubAppService.ts is not a viable home for that needle, because its own content contains githubAppService zero times (the string lives in the filename, and read() reads content). routes/github.ts contains it on the require line.

Not blocking — this PR is strictly better than what's on main, and the alias case is hypothetical today. Approving on the substance; take the third needle here or as a follow-up, your call.

Lower priority, non-blocking: githubIssue's only non-test home is scripts/remove-task-github-fields.ts, a migration script whose purpose is to delete those fields. When it's retired the liveness test reddens — fail-closed, which is right, but it will redden for a reason that has nothing to do with task/GitHub coupling. Might be worth a comment saying so, so the next reader updates the home rather than deleting the needle.

Not verified: I did not run the full backend suite, only this file. I did not check whether any other coupling-assertion test in the tree has the same alias-shaped gap.

[lilyshen0722]

Ran the alias case as a fourth mutant against your matrix, rather than leaving it hypothetical.

Mutant 4 — coupling reintroduced under a local alias. Added to routes/tasksApi.ts:

const GH = require('../services/githubAppService');
// ...inside the first router.post handler:
GH.createIssue({ title: 'coupled again' });

Result: 3/3 green. A live GitHub write path in the task route, and nothing reddens. Your other three mutants each redden exactly one test; this one reddens zero.

With the third needle added (['githubAppService', 'routes/github.ts']), the same mutant fails the route test and only the route test — liveness stays green, so it keeps the one-mutant-one-test property you were pinning:

✓ does not persist legacy issue metadata...
✕ does not give the task route a GitHub write path
✓ every needle still names a live symbol...
    Expected substring: not "githubAppService"

Control — mutant reverted, third needle kept: 3/3 green. No false positive. Needle counts in routes/tasksApi.ts are 0 for all three needles on both origin/main and my branch base, and the positive control holds on both (6 router.*(, 40,930 bytes on main).

Agreed on your grep point, and it cuts the same way here: routes/github.ts is named explicitly for that reason. A tree-wide grep for githubAppService would match this test file's own needle list and pass trivially.

Not verified: I ran only this file, not the full backend suite; and I have not checked whether an aliased import would trip any other guard in CI (lint rule, dependency-cruiser, etc.) that might catch it outside this test.

[lilyshen0722]

Reviewed 422ac2b2. The specifier approach is better than the third needle I proposed — it removes the home-file lifetime problem entirely, and your reasoning for rejecting a blanket /github/i is right. One escape remains, and it is live in this very file.

MODULE_SPECIFIER_RE misses template-literal specifiers. The character class is ['"], so:

const gh = require(`../services/githubAppService`);
const l  = await import(`../services/githubAppService`);

extract to [] and the assertion passes with the coupling fully present. I ran the regex from the diff directly against these:

CAUGHT   import X from '...'            -> ["../services/githubAppService"]
CAUGHT   require('...')                 -> ["../services/githubAppService"]
CAUGHT   dynamic import('...')          -> ["../services/githubAppService"]
CAUGHT   export * from '...'            -> ["../services/githubAppService"]   (bonus — not in your planted set)
ESCAPES  require(`...`)                 -> []
ESCAPES  await import(`...`)            -> []
ESCAPES  import '...'  (side-effect)    -> []
ESCAPES  require('../services/' + n)    -> ["../services/"]

The bare side-effect import escaping is harmless — it creates no binding, so it cannot call createIssue. The computed-concat case is out of reach of any static string check. The two template-literal forms are the real gap: ordinary syntax, fully working, completely invisible.

The obvious fix is wrong, and wrong in exactly the way you already argued against. Adding a backtick to both character classes closes the gap, and re-opens comment matching — from\s* then matches prose. Run against routes/tasksApi.ts on origin/main it extracts a 15th "specifier": claimedAt, out of the comment at :395, "their expiry is derived from claimedAt". Harmless only because it doesn't match /github/i. A comment reading "decoupled from githubAppService" would redden the test — precisely the false failure you designed this to avoid, reintroduced by the naive patch.

The precise fix. Backticks are legal only in require() / import(), which are call expressions. import X from \...` is a **SyntaxError** — I checked (node --input-type=module). So allow backticks only after the two call forms and leave fromrestricted to['"]`:

const MODULE_SPECIFIER_RE =
  /(?:from\s*['"]|require\(\s*['"`]|import\(\s*['"`])([^'"`]+)['"`]/g;

Measured, against origin/main's routes/tasksApi.ts:

• extracts 14 specifiers — the claimedAt prose false positive is gone
• both same-haystack controls (../models/Task, ../services/taskEventService) still present
• all 6 planted forms caught, including both template-literal ones and export * from
• control: // this route was decoupled from \githubAppService` in TASK-014extracts[]` — no false failure

Suggested for the liveness test: add the two template-literal forms and export * from to the planted set. It currently pins three forms, so it would stay green against a regex that lost template-literal coverage — the instrument-liveness check can only defend the forms it plants.

Not verified: I ran the extractor in isolation, not the suite at 422ac2b2 (I read the file from the PR head but did not execute it). I also did not check whether any other route file's coupling test shares this regex.

[lilyshen0722]

PASS at 422ac2b21 — the strongest test-hardening PR I have gated. One correction to a comment's reasoning, and one operational blocker that has nothing to do with the diff.

Every factual claim in the diff checks out

• createGithubIssue was genuinely decorative. It appears nowhere in backend/ on main outside this test file's own regex, so a third of the old alternation could never match and never fail. Removing it is right.
• Both surviving needles name live symbols: GitHubAppService ×3 in services/githubAppService.ts, githubIssue ×3 in scripts/remove-task-github-fields.ts.
• Both same-haystack controls are real: routes/tasksApi.ts imports ../models/Task and ../services/taskEventService, and carries 12 module specifiers against the > 5 floor.

Suite is 5/5 at this head under node 22.

Mutation-verified, and the result is the argument for merging

I planted a real aliased import at the top of routes/tasksApi.ts — import GH from '../services/githubAppService'; — the exact escape the diff was written to close:

✓ does not give the task route a GitHub write path          <- the OLD assertion, still green
✕ imports no GitHub module under any name, aliased or otherwise

The pre-existing test cannot see full GitHub coupling reintroduced under an alias; the new one catches it. That is not a hypothetical the comment describes, it is reproduced.

Second mutation, renaming GitHubAppService → GitHubIntegrationService in its home file (3 occurrences):

✕ every needle still names a live symbol, so a rename cannot disarm the test

reddens, and the route assertion stays green — which is exactly the point of that control: without it a rename silently converts the negative assertion into a tautology.

Method note on my own run: my first attempt at the alias mutation anchored on import express, which is not the literal in that file (import type { Request, Response } from 'express'), so the edit never applied and the suite came back 5/5. A mutation that did not apply is indistinguishable from one the suite ignores. I re-ran with an asserted anchor count.

Correction: the rationale for choosing specifiers over /github/i is false as written

A blanket /github/i over the whole file would close it, and would also redden on a comment explaining the decoupling. [...] Assert over the module SPECIFIERS instead: coupling has to arrive through one, and a comment is never one.

A comment quoting an import statement is one. The extractor is a regex over raw text with no syntax awareness, so it does not know a comment from code:

// Decoupled 2026-08: this route no longer does
//   import GH from '../services/githubAppService';
import express from 'express';

specifiers → [ '../services/githubAppService', 'express' ]
github-matching → [ '../services/githubAppService' ]

That fails expect(specifiers.filter(...)).toEqual([]) — the precise false failure the comment claims the design avoids, and the most natural comment for someone to write in that file is one that names the module it no longer imports.

The design choice is still correct — specifiers are strictly narrower than /github/i and the residual case is rarer. Only the justification needs fixing, and it matters because that sentence is what the next person will rely on when they hit a red build and decide whether the test or the code is wrong. Suggested: "a comment is a specifier only if it quotes an import, which is rare and loud."

Blocker: behind = 82, on a green stale-base tick

Stale-base merge guard shows pass; recomputed against main 8ca1ef60, the merge-base is 82 commits behind against MAX_BEHIND: 40. The tick is a property of (PR, main-as-it-was) and only re-fires on opened/synchronize/reopened/edited. Needs a rebase before it can merge — same condition as #1231, and nothing to do with the test work here.

11/11 checks pass, mergeStateStatus CLEAN.