From 2b5763f267ceb8cd6bc4abe3c1e98f19ea5600ed Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Tue, 25 Aug 2026 05:07:46 -0700 Subject: [PATCH 1/3] test(tasks): give the GitHub-coupling assertion a positive control and a live needle MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit @sprint-review, 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 of them 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. Now asserts the source actually looks like the route. 2. `createGithubIssue` was a dead needle — it existed nowhere in the tree except inside this file's own regex, so it could never match and never fail. 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), so a rename reddens here and forces the needle to be updated rather than silently disarming the guard. Mutation-tested, all three redden and only the intended 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. Co-Authored-By: Claude Opus 5 --- .../tasksApi.githubCouplingRemoved.test.js | 41 ++++++++++++++++--- 1 file changed, 36 insertions(+), 5 deletions(-) diff --git a/backend/__tests__/unit/routes/tasksApi.githubCouplingRemoved.test.js b/backend/__tests__/unit/routes/tasksApi.githubCouplingRemoved.test.js index 4c23f5bb8..e0f5864e9 100644 --- a/backend/__tests__/unit/routes/tasksApi.githubCouplingRemoved.test.js +++ b/backend/__tests__/unit/routes/tasksApi.githubCouplingRemoved.test.js @@ -32,12 +32,43 @@ describe('task board is independent of GitHub', () => { expect(task).not.toHaveProperty('githubIssueOwned'); }); + // The needles, kept in one place so the assertion below and the liveness + // control below THAT cannot drift apart. `createGithubIssue` used to be a + // third needle here and was removed: it existed nowhere in the tree except + // inside this file's own regex, so it could never match and never fail — + // a third of the alternation was decorative (@sprint-review, 58470). + const COUPLING_NEEDLES = [ + // [needle, a file that must still contain it] + ['GitHubAppService', 'services/githubAppService.ts'], + ['githubIssue', 'scripts/remove-task-github-fields.ts'], + ]; + + const read = (rel) => fs.readFileSync(path.join(__dirname, '../../../', rel), 'utf8'); + it('does not give the task route a GitHub write path', () => { - const routeSource = fs.readFileSync( - path.join(__dirname, '../../../routes/tasksApi.ts'), - 'utf8', - ); + const routeSource = read('routes/tasksApi.ts'); + + // POSITIVE CONTROL. Without it, this test proves only that some string + // lacks some substrings — an empty read, a truncated file, or the wrong + // path would all pass the negative assertion silently. readFileSync + // throwing covers a MISSING file; it does not cover a present-but-wrong + // one. + expect(routeSource).toMatch(/router\.(get|post|patch|put|delete)\(/); + expect(routeSource.length).toBeGreaterThan(1000); + + for (const [needle] of COUPLING_NEEDLES) { + expect(routeSource).not.toContain(needle); + } + }); - expect(routeSource).not.toMatch(/GitHubAppService|githubIssue|createGithubIssue/); + // The control that makes the assertion above mean something. A negative + // match passes for two different reasons — the coupling is gone, or the + // needle no longer names anything. Rename GitHubAppService and the route + // test goes green forever with the coupling fully present; this one reddens + // instead and forces the needle to be updated with the symbol. + it('every needle still names a live symbol, so a rename cannot disarm the test', () => { + for (const [needle, home] of COUPLING_NEEDLES) { + expect(read(home)).toContain(needle); + } }); }); From 94e24771ca3696fd916e5bd83453ff77743593ed Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Tue, 25 Aug 2026 05:09:05 -0700 Subject: [PATCH 2/3] test(tasks): state the positive-control rule in its precise form MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit @sprint-review: "add a positive control" is too weak a prescription. The control has to be a needle known present in THAT SAME haystack, asserted from THAT SAME variable — a passing assertion elsewhere in the file can be green while this particular read returned something no one intended. Comment only; the assertion already took this form. Co-Authored-By: Claude Opus 5 --- .../unit/routes/tasksApi.githubCouplingRemoved.test.js | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/backend/__tests__/unit/routes/tasksApi.githubCouplingRemoved.test.js b/backend/__tests__/unit/routes/tasksApi.githubCouplingRemoved.test.js index e0f5864e9..5029f0f24 100644 --- a/backend/__tests__/unit/routes/tasksApi.githubCouplingRemoved.test.js +++ b/backend/__tests__/unit/routes/tasksApi.githubCouplingRemoved.test.js @@ -53,6 +53,13 @@ describe('task board is independent of GitHub', () => { // path would all pass the negative assertion silently. readFileSync // throwing covers a MISSING file; it does not cover a present-but-wrong // one. + // + // The precise form matters, and "add a positive control" is too weak a + // prescription (@sprint-review): the control must be a needle you know is + // present in THAT SAME haystack, asserted from THAT SAME variable. A + // passing assertion elsewhere in the file proves nothing about this read + // — it can be green while this particular readFileSync returned something + // no one intended. expect(routeSource).toMatch(/router\.(get|post|patch|put|delete)\(/); expect(routeSource.length).toBeGreaterThan(1000); From 422ac2b212f10e57fa4a3ecb7147d1261833661a Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Tue, 25 Aug 2026 05:38:43 -0700 Subject: [PATCH 3/3] test(tasks): close the import-alias escape on the GitHub-coupling check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both existing needles are symbol names, so `import GH from '../services/githubAppService'` reintroduces the coupling in full while neither needle appears in the route source. Verified by planting that exact require at the top of routes/tasksApi.ts: the two prior assertions stayed green and only the new one reddened. Assert over module specifiers rather than a blanket /github/i, which would also fire on a comment explaining the decoupling — a false failure that eventually gets the test deleted instead of fixed. Coupling has to enter through a specifier; a comment never is one. The second new case is liveness for the instrument rather than the symbol: it plants all three import forms and asserts the extractor catches them, so the check cannot pass by silently matching nothing. Co-Authored-By: Claude Opus 5 --- .../tasksApi.githubCouplingRemoved.test.js | 43 +++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/backend/__tests__/unit/routes/tasksApi.githubCouplingRemoved.test.js b/backend/__tests__/unit/routes/tasksApi.githubCouplingRemoved.test.js index 5029f0f24..74ccf8ce7 100644 --- a/backend/__tests__/unit/routes/tasksApi.githubCouplingRemoved.test.js +++ b/backend/__tests__/unit/routes/tasksApi.githubCouplingRemoved.test.js @@ -78,4 +78,47 @@ describe('task board is independent of GitHub', () => { expect(read(home)).toContain(needle); } }); + + // The escape those two needles cannot close (@sprint-review, 58544): an + // import alias. `import GH from '../services/githubAppService'` reintroduces + // the coupling in full while `GitHubAppService` never appears in the route + // source — both needles stay absent, both assertions above stay green. + // + // A blanket /github/i over the whole file would close it, and would also + // redden on a comment explaining the decoupling. That is a false failure + // someone eventually hits and then deletes the test over. Assert over the + // module SPECIFIERS instead: coupling has to arrive through one, and a + // comment is never one. + const MODULE_SPECIFIER_RE = /(?:from\s*|require\(\s*|import\(\s*)['"]([^'"]+)['"]/g; + const specifiersOf = (src) => [...src.matchAll(MODULE_SPECIFIER_RE)].map((m) => m[1]); + + it('imports no GitHub module under any name, aliased or otherwise', () => { + const specifiers = specifiersOf(read('routes/tasksApi.ts')); + + // Same-haystack control as above. An extraction that returned [] would + // satisfy the negative assertion perfectly and prove nothing. + expect(specifiers).toContain('../models/Task'); + expect(specifiers).toContain('../services/taskEventService'); + expect(specifiers.length).toBeGreaterThan(5); + + expect(specifiers.filter((s) => /github/i.test(s))).toEqual([]); + }); + + it('the specifier extractor still fires on a GitHub import, so the check cannot go blind', () => { + // Liveness for the INSTRUMENT, not the symbol. The needle control above + // proves a name is still live; this proves the regex still matches. A + // control that fails to construct is indistinguishable from an instrument + // that cannot detect, so plant the thing being denied and see it caught. + const planted = [ + "import GH from '../services/githubAppService';", + "const gh = require('../services/githubAppService');", + "const lazy = await import('../services/githubAppService');", + ].join('\n'); + + expect(specifiersOf(planted).filter((s) => /github/i.test(s))).toEqual([ + '../services/githubAppService', + '../services/githubAppService', + '../services/githubAppService', + ]); + }); });