From c19d3d7bdae02aaf836c79f27982d8be0451f507 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Tue, 21 Jul 2026 18:38:51 -0400 Subject: [PATCH] chore(#2453): resolve the uniform-router-param conflict and clear the warning floor (#2489) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit npx eslint . reported 0 errors, 53 warnings. The floor eroded the signal: a genuinely new warning had to be spotted against noise, so 'lint is clean' was not usable as a check. This takes it to zero. Fifty of the 53 were one category in gsd-core/bin/gsd-tools.cjs — all unused ARGUMENTS (error x30, cwd x10, raw x7, args x3), never unused variables. They are the Command Routing Hub's uniform handler signature, function routeX({ args, cwd, raw, error }), declared identically whether or not a handler uses all four members. argsIgnorePattern: '^_' is structurally in conflict with that convention: satisfying it would mean _-prefixing ~50 parameters and making the signature non-uniform across the table. Option 1 of #2453: disable args checking for that file only, keeping varsIgnorePattern intact so genuinely dead variables (the #2379 class) still surface — verified by injecting an unused variable, which still warns. This is the config decision #732 explicitly deferred ('Severities stay warn (no config change in this pass)'). The remaining three predate the issue's count of 51 and are real defects, not suppressions: - tests/workflow-compat.test.cjs — the step-9 lookahead used (?=\*\*10\.|\z). \z is a Perl/Ruby end-of-input anchor with NO meaning in JavaScript; it matched a literal 'z', so the lazy span silently stopped at the first z whenever **10. was absent, truncating the captured step and letting the assertion pass against a partial block. Corrected to $. - tests/debugger-prevention.test.cjs — an unused RegExp built one line above the one actually used. Removed. - tests/installer-migrations.test.cjs — try/finally inside a test body, which CONTRIBUTING prohibits ('verbose, masks test failures, not an approved pattern'); the unused t was the symptom. Converted to t.after(). Closes #2453 Co-authored-by: Claude Opus 4.8 --- eslint.config.mjs | 29 ++++++++++++++++ tests/debugger-prevention.test.cjs | 1 - tests/installer-migrations.test.cjs | 52 ++++++++++++++--------------- tests/workflow-compat.test.cjs | 6 +++- 4 files changed, 59 insertions(+), 29 deletions(-) diff --git a/eslint.config.mjs b/eslint.config.mjs index 27e6dcaf0..6ad716168 100644 --- a/eslint.config.mjs +++ b/eslint.config.mjs @@ -419,4 +419,33 @@ export default tseslint.config( languageOptions: { sourceType: 'commonjs', globals: { ...globals.node } }, rules: { 'local/no-source-grep': 'error' }, }, + + // ── #2453 Command Routing Hub: uniform handler signature ──────────────────── + // Every route handler in gsd-tools.cjs is declared with the SAME destructured + // signature — `function routeX({ args, cwd, raw, error })` — whether or not it + // uses all four members. That uniformity is the point: it is the dispatch + // contract, so a handler can be moved or added without re-deriving which + // members exist. + // + // `argsIgnorePattern: '^_'` is structurally in conflict with that convention: + // satisfying it would mean `_`-prefixing ~50 parameters, which makes the + // signature non-uniform across the table and defeats the contract. So args + // checking is disabled HERE ONLY. + // + // `varsIgnorePattern` is deliberately left intact: genuinely dead *variables* + // (the #2379 case — unused `require()` results) must still surface. This + // narrows the exemption to the one category the convention actually forces. + // + // Decision deferred by #732 ("Severities stay `warn` (no config change in this + // pass)"), resolved by #2453 option 1. + { + files: ['gsd-core/bin/gsd-tools.cjs'], + rules: { + 'no-unused-vars': ['warn', { + args: 'none', + varsIgnorePattern: '^_', + caughtErrors: 'none', + }], + }, + }, ); diff --git a/tests/debugger-prevention.test.cjs b/tests/debugger-prevention.test.cjs index 34295569a..4bf7f9ba0 100644 --- a/tests/debugger-prevention.test.cjs +++ b/tests/debugger-prevention.test.cjs @@ -96,7 +96,6 @@ describe('prevention / blameless-postmortem output (#1963, epic #1957 Phase 3B)' const content = fs.readFileSync(AGENT, 'utf8'); const requiredFields = ['Date', 'Error patterns', 'Root cause', 'Fix', 'Files changed', 'Why not caught', 'Recurrence guard']; for (const field of requiredFields) { - const re = new RegExp(`\\*\\*${field}`, 'i'); const matches = content.match(new RegExp(`\\*\\*${field}`, 'gi')); assert.ok( matches && matches.length >= 2, diff --git a/tests/installer-migrations.test.cjs b/tests/installer-migrations.test.cjs index 337ec7a61..fa5d5c751 100644 --- a/tests/installer-migrations.test.cjs +++ b/tests/installer-migrations.test.cjs @@ -2654,36 +2654,34 @@ describe('migration.plan()', () => { test('a regular managed file is still backed up by content (no behavior change)', (t) => { const configDir = createTempInstall(); - try { - writeFile(configDir, 'extensions/gsd.cjs', 'locally patched extension\n'); - writeManifest(configDir, { 'extensions/gsd.cjs': sha256('the original extension\n') }); + t.after(() => cleanup(configDir)); - const result = runInstallerMigrations({ - configDir, - runtime: 'pi', - scope: 'global', - migrations: [piExtensionMigration], - now: () => '2026-07-20T00:00:00.000Z', - }); + writeFile(configDir, 'extensions/gsd.cjs', 'locally patched extension\n'); + writeManifest(configDir, { 'extensions/gsd.cjs': sha256('the original extension\n') }); - const backupAction = result.plan.actions.find((a) => a.type === 'backup-and-remove'); - assert.ok(backupAction, 'expected backup-and-remove for the locally patched file'); + const result = runInstallerMigrations({ + configDir, + runtime: 'pi', + scope: 'global', + migrations: [piExtensionMigration], + now: () => '2026-07-20T00:00:00.000Z', + }); - // The PLAN carries backupRelPath: null — the concrete backup location is - // chosen during apply and recorded in the journal, so read it from there. - const journal = JSON.parse(fs.readFileSync(path.join(configDir, result.journalRelPath), 'utf8')); - const journalled = journal.actions.find((a) => a.backupRelPath); - assert.ok(journalled, 'apply must record the backup path in the journal for the user'); - const backupPath = path.join(configDir, journalled.backupRelPath); - assert.equal( - fs.readFileSync(backupPath, 'utf8'), - 'locally patched extension\n', - 'a real file must still be backed up by content so the user can recover it', - ); - assert.ok(!fs.existsSync(path.join(configDir, 'extensions', 'gsd.cjs'))); - } finally { - cleanup(configDir); - } + const backupAction = result.plan.actions.find((a) => a.type === 'backup-and-remove'); + assert.ok(backupAction, 'expected backup-and-remove for the locally patched file'); + + // The PLAN carries backupRelPath: null — the concrete backup location is + // chosen during apply and recorded in the journal, so read it from there. + const journal = JSON.parse(fs.readFileSync(path.join(configDir, result.journalRelPath), 'utf8')); + const journalled = journal.actions.find((a) => a.backupRelPath); + assert.ok(journalled, 'apply must record the backup path in the journal for the user'); + const backupPath = path.join(configDir, journalled.backupRelPath); + assert.equal( + fs.readFileSync(backupPath, 'utf8'), + 'locally patched extension\n', + 'a real file must still be backed up by content so the user can recover it', + ); + assert.ok(!fs.existsSync(path.join(configDir, 'extensions', 'gsd.cjs'))); }); // In-flight failure recovery: when a later step of the SAME apply() attempt diff --git a/tests/workflow-compat.test.cjs b/tests/workflow-compat.test.cjs index 554d767b0..7ab90034b 100644 --- a/tests/workflow-compat.test.cjs +++ b/tests/workflow-compat.test.cjs @@ -161,7 +161,11 @@ describe('feat-41: ship.md TDD Audit gate_status extraction', () => { test('#2431: step 9 (aggregate trailer) is also gated on real values existing', () => { // The aggregate gate_status trailer is the companion to the TDD Audit // section; both must be skipped together when no real gate_status exists. - const step9 = workflow.match(/\*\*9\.\s*Aggregate gate_status trailer[\s\S]*?(?=\*\*10\.|\z)/); + // `\z` is a Perl/Ruby end-of-input anchor with NO meaning in JavaScript — it + // matched a literal `z`, so the lazy span silently stopped at the first `z` + // whenever `**10.` was absent, truncating the captured step. `$` (no /m flag) + // is the JS end-of-input anchor. + const step9 = workflow.match(/\*\*9\.\s*Aggregate gate_status trailer[\s\S]*?(?=\*\*10\.|$)/); assert.ok(step9, 'step 9 must exist in the workflow'); assert.match(step9[0], /step 8|at least one|real/i, 'step 9 must reference step 8 or require at least one real gate_status value (#2431)');