Wire settings.json filter rewrite into dedupeAndRegister() #170

New issue

Closed

opened 2026-09-08 00:02:48 +00:00 by david · 1 comment

david commented

2026-09-08 00:02:48 +00:00

Owner

Summary

After dedupeAndRegister() runs pi install <repo-path>, rewrite the resulting settings.json package entry into the object filter form (using buildPackageFilters()) whenever fj or rg is unusable, so this single local-path registration actually excludes the affected extensions/skills. When both tools are fully available, leave the entry as the plain string source pi install already wrote.

Background

scripts/local-install.mjs's dedupeAndRegister(cwd, options) currently:

Removes any existing settings entry (global or project-scope) referencing this repo's URL.
Runs pi install <absRepoPath>, which registers the whole checkout with a plain string source — no filtering.

With preflight() now returning { rgOk, fjState } (see "Refactor preflight() to skip/warn for missing fj/rg instead of hard-failing") and main() already threading a toolState option through to dedupeAndRegister(cwd, { ...options, toolState }), this step makes dedupeAndRegister() actually use toolState to apply the correct filter, via buildPackageFilters() (see "Implement buildPackageFilters() with exhaustive unit tests").

dedupeAndRegister() already reads/writes the settings.json file for the removal step (see its existing BOM-stripping JSON read logic and warn-and-continue behavior on unreadable/missing files) — this step extends that same responsibility rather than introducing a new I/O path.

Depends on: "Implement buildPackageFilters() with exhaustive unit tests" (provides the filter-building logic this step applies) and "Refactor preflight() to skip/warn for missing fj/rg instead of hard-failing" (provides the toolState value threaded through main() into dedupeAndRegister()'s options).

Implementation Details

In dedupeAndRegister(), after the successful pi install <absRepoPath> call, add a new step:

export function dedupeAndRegister(cwd, options = {}) {
  const absRepoPath = resolve(cwd);
  const globalSettingsFile = options.globalSettingsFile ?? SETTINGS_FILE;
  // ...existing removal logic (unchanged)...

  console.log(`\n[install:local] Registering local path with pi: ${absRepoPath}`);
  const installed = run("pi", ["install", absRepoPath]);
  if (!ok(installed)) {
    fail(/* existing unchanged failure handling */);
  }
  console.log(combinedOutput(installed));

  // NEW: apply the fj/rg filter, if any.
  const toolState = options.toolState ?? { rgOk: true, fjState: "ok" };
  const filters = buildPackageFilters(toolState);
  if (filters) {
    rewritePackageEntry(globalSettingsFile, absRepoPath, filters);
  }
}

Add the new rewritePackageEntry(file, absRepoPath, filters) helper:

/**
 * Rewrite the settings.json package entry whose source is `absRepoPath` into
 * the object filter form `{ source, ...filters }`. No-op (with a warning) if
 * the file is unreadable or no matching entry is found — never aborts the
 * install over this, matching the existing warn-and-continue convention used
 * elsewhere in this file for settings.json I/O issues.
 */
function rewritePackageEntry(file, absRepoPath, filters) {
  let settings;
  try {
    if (!existsSync(file)) {
      console.error(
        `[install:local] Warning: could not apply extension/skill filters — ` +
          `settings file not found (${file}). The package was installed without ` +
          `filtering; fj/rg-dependent resources may still be enabled.`
      );
      return;
    }
    settings = JSON.parse(readFileSync(file, "utf-8").replace(/^\uFEFF/, ""));
  } catch (error) {
    console.error(
      `[install:local] Warning: could not apply extension/skill filters ` +
        `(${file}): ${error.message}. The package was installed without filtering; ` +
        `fj/rg-dependent resources may still be enabled.`
    );
    return;
  }

  const packages = Array.isArray(settings.packages) ? settings.packages : [];
  const index = packages.findIndex((pkg) => {
    const source = typeof pkg === "string" ? pkg : pkg?.source;
    return source === absRepoPath;
  });

  if (index === -1) {
    console.error(
      `[install:local] Warning: could not find the just-installed package entry ` +
        `(${absRepoPath}) in ${file} to apply extension/skill filters. The package ` +
        `was installed without filtering; fj/rg-dependent resources may still be enabled.`
    );
    return;
  }

  packages[index] = { source: absRepoPath, ...filters };
  settings.packages = packages;
  writeFileSync(file, JSON.stringify(settings, null, 2) + "\n", "utf-8");
  console.log(
    `[install:local] Applied extension/skill filters to the settings entry: ` +
      `${JSON.stringify(filters)}`
  );
}

Notes:

existsSync, readFileSync, writeFileSync are already imported in scripts/local-install.mjs (used by the existing removal logic) — reuse them, do not re-import.
Re-derive fresh on every call, no diffing: this function always fully replaces the matched entry with { source, ...filters } (never merges with a prior filtered state). Because dedupeAndRegister() already removes-then-reinstalls on every run, calling rewritePackageEntry fresh each time naturally adds the exclusion when a tool disappears, and naturally drops it (since filters is null and no rewrite happens, leaving pi install's plain string entry) when the tool reappears on a later run.
When filters is null (both tools fully available, or fj merely unauthenticated), rewritePackageEntry is not called at all — the plain string entry pi install wrote stands unchanged.
dedupeAndRegister()'s existing options.globalSettingsFile override (used by tests to point at a temp file) applies to this new rewrite step too — do not introduce a second settings-file parameter.

Acceptance Criteria

When toolState indicates both tools fully available (or is omitted, defaulting to fully-available), the settings.json package entry for this repo remains a plain string after dedupeAndRegister() runs (no rewrite).
When toolState.rgOk === false (and fj is ok), the entry becomes { source: <absRepoPath>, extensions: ["!extensions/rg/index.ts"], skills: [] }.
When toolState.fjState === "absent" (and rg is ok), the entry becomes { source: <absRepoPath>, extensions: ["!extensions/pr-comments/src/index.ts"], skills: ["!skills/forgejo-cli"] }.
When both rgOk === false and fjState === "absent", the entry becomes the combined filter shape with both extension exclusions and the skill exclusion.
When toolState.fjState === "unauthenticated", the entry remains a plain string (unauthenticated must not trigger a filter rewrite).
Calling dedupeAndRegister() twice in sequence against the same settings file with different toolState values on each call produces the settings entry matching the second call's toolState — no stale filter persists from the first call (re-derive-fresh, no accumulation).
If the settings file is missing/unreadable, or no matching entry is found after pi install, dedupeAndRegister() logs a warning and does not abort or throw.
All pre-existing dedupeAndRegister() tests (URL-entry removal/dedupe, BOM stripping, unreadable-settings warn-and-continue, pi remove/pi install failure abort paths) continue to pass unmodified — none of them pass a toolState option, so they exercise the default fully-available path and see no filter rewrite.

Test Plan

Unit test: dedupeAndRegister with no toolState (or { rgOk: true, fjState: "ok" }) → resulting settings entry is the plain string resolve(cwd).
Unit test: dedupeAndRegister with { rgOk: false, fjState: "ok" } → entry matches the rg-only filter shape.
Unit test: dedupeAndRegister with { rgOk: true, fjState: "absent" } → entry matches the fj-only filter shape.
Unit test: dedupeAndRegister with { rgOk: false, fjState: "absent" } → entry matches the combined filter shape.
Unit test: dedupeAndRegister with { rgOk: true, fjState: "unauthenticated" } → entry stays a plain string.
Unit test: two sequential dedupeAndRegister calls against the same settings file with { rgOk: false, fjState: "ok" } then { rgOk: true, fjState: "ok" } → final entry is the plain string (rg exclusion removed, not retained).
Unit test: pre-populate globalSettings such that the just-installed entry can't be found (e.g. directly exercise rewritePackageEntry against a settings object lacking the expected source) → no throw, warning logged, aborted === false.
Re-run the full existing dedupeAndRegister test suite — all pass unmodified.
npm test — all green.

## Summary After `dedupeAndRegister()` runs `pi install <repo-path>`, rewrite the resulting settings.json package entry into the object filter form (using `buildPackageFilters()`) whenever `fj` or `rg` is unusable, so this single local-path registration actually excludes the affected extensions/skills. When both tools are fully available, leave the entry as the plain string source `pi install` already wrote. ## Background `scripts/local-install.mjs`'s `dedupeAndRegister(cwd, options)` currently: 1. Removes any existing settings entry (global or project-scope) referencing this repo's URL. 2. Runs `pi install <absRepoPath>`, which registers the whole checkout with a plain string source — no filtering. With `preflight()` now returning `{ rgOk, fjState }` (see "Refactor preflight() to skip/warn for missing fj/rg instead of hard-failing") and `main()` already threading a `toolState` option through to `dedupeAndRegister(cwd, { ...options, toolState })`, this step makes `dedupeAndRegister()` actually *use* `toolState` to apply the correct filter, via `buildPackageFilters()` (see "Implement buildPackageFilters() with exhaustive unit tests"). `dedupeAndRegister()` already reads/writes the settings.json file for the removal step (see its existing BOM-stripping JSON read logic and warn-and-continue behavior on unreadable/missing files) — this step extends that same responsibility rather than introducing a new I/O path. **Depends on:** "Implement buildPackageFilters() with exhaustive unit tests" (provides the filter-building logic this step applies) and "Refactor preflight() to skip/warn for missing fj/rg instead of hard-failing" (provides the `toolState` value threaded through `main()` into `dedupeAndRegister()`'s `options`). ## Implementation Details In `dedupeAndRegister()`, after the successful `pi install <absRepoPath>` call, add a new step: ```js export function dedupeAndRegister(cwd, options = {}) { const absRepoPath = resolve(cwd); const globalSettingsFile = options.globalSettingsFile ?? SETTINGS_FILE; // ...existing removal logic (unchanged)... console.log(`\n[install:local] Registering local path with pi: ${absRepoPath}`); const installed = run("pi", ["install", absRepoPath]); if (!ok(installed)) { fail(/* existing unchanged failure handling */); } console.log(combinedOutput(installed)); // NEW: apply the fj/rg filter, if any. const toolState = options.toolState ?? { rgOk: true, fjState: "ok" }; const filters = buildPackageFilters(toolState); if (filters) { rewritePackageEntry(globalSettingsFile, absRepoPath, filters); } } ``` Add the new `rewritePackageEntry(file, absRepoPath, filters)` helper: ```js /** * Rewrite the settings.json package entry whose source is `absRepoPath` into * the object filter form `{ source, ...filters }`. No-op (with a warning) if * the file is unreadable or no matching entry is found — never aborts the * install over this, matching the existing warn-and-continue convention used * elsewhere in this file for settings.json I/O issues. */ function rewritePackageEntry(file, absRepoPath, filters) { let settings; try { if (!existsSync(file)) { console.error( `[install:local] Warning: could not apply extension/skill filters — ` + `settings file not found (${file}). The package was installed without ` + `filtering; fj/rg-dependent resources may still be enabled.` ); return; } settings = JSON.parse(readFileSync(file, "utf-8").replace(/^\uFEFF/, "")); } catch (error) { console.error( `[install:local] Warning: could not apply extension/skill filters ` + `(${file}): ${error.message}. The package was installed without filtering; ` + `fj/rg-dependent resources may still be enabled.` ); return; } const packages = Array.isArray(settings.packages) ? settings.packages : []; const index = packages.findIndex((pkg) => { const source = typeof pkg === "string" ? pkg : pkg?.source; return source === absRepoPath; }); if (index === -1) { console.error( `[install:local] Warning: could not find the just-installed package entry ` + `(${absRepoPath}) in ${file} to apply extension/skill filters. The package ` + `was installed without filtering; fj/rg-dependent resources may still be enabled.` ); return; } packages[index] = { source: absRepoPath, ...filters }; settings.packages = packages; writeFileSync(file, JSON.stringify(settings, null, 2) + "\n", "utf-8"); console.log( `[install:local] Applied extension/skill filters to the settings entry: ` + `${JSON.stringify(filters)}` ); } ``` Notes: - `existsSync`, `readFileSync`, `writeFileSync` are already imported in `scripts/local-install.mjs` (used by the existing removal logic) — reuse them, do not re-import. - Re-derive fresh on every call, no diffing: this function always fully replaces the matched entry with `{ source, ...filters }` (never merges with a prior filtered state). Because `dedupeAndRegister()` already removes-then-reinstalls on every run, calling `rewritePackageEntry` fresh each time naturally adds the exclusion when a tool disappears, and naturally drops it (since `filters` is `null` and no rewrite happens, leaving `pi install`'s plain string entry) when the tool reappears on a later run. - When `filters` is `null` (both tools fully available, or fj merely unauthenticated), `rewritePackageEntry` is not called at all — the plain string entry `pi install` wrote stands unchanged. - `dedupeAndRegister()`'s existing `options.globalSettingsFile` override (used by tests to point at a temp file) applies to this new rewrite step too — do not introduce a second settings-file parameter. ## Acceptance Criteria - [ ] When `toolState` indicates both tools fully available (or is omitted, defaulting to fully-available), the settings.json package entry for this repo remains a plain string after `dedupeAndRegister()` runs (no rewrite). - [ ] When `toolState.rgOk === false` (and fj is ok), the entry becomes `{ source: <absRepoPath>, extensions: ["!extensions/rg/index.ts"], skills: [] }`. - [ ] When `toolState.fjState === "absent"` (and rg is ok), the entry becomes `{ source: <absRepoPath>, extensions: ["!extensions/pr-comments/src/index.ts"], skills: ["!skills/forgejo-cli"] }`. - [ ] When both `rgOk === false` and `fjState === "absent"`, the entry becomes the combined filter shape with both extension exclusions and the skill exclusion. - [ ] When `toolState.fjState === "unauthenticated"`, the entry remains a plain string (unauthenticated must not trigger a filter rewrite). - [ ] Calling `dedupeAndRegister()` twice in sequence against the same settings file with different `toolState` values on each call produces the settings entry matching the *second* call's `toolState` — no stale filter persists from the first call (re-derive-fresh, no accumulation). - [ ] If the settings file is missing/unreadable, or no matching entry is found after `pi install`, `dedupeAndRegister()` logs a warning and does not abort or throw. - [ ] All pre-existing `dedupeAndRegister()` tests (URL-entry removal/dedupe, BOM stripping, unreadable-settings warn-and-continue, `pi remove`/`pi install` failure abort paths) continue to pass unmodified — none of them pass a `toolState` option, so they exercise the default fully-available path and see no filter rewrite. ## Test Plan - Unit test: `dedupeAndRegister` with no `toolState` (or `{ rgOk: true, fjState: "ok" }`) → resulting settings entry is the plain string `resolve(cwd)`. - Unit test: `dedupeAndRegister` with `{ rgOk: false, fjState: "ok" }` → entry matches the rg-only filter shape. - Unit test: `dedupeAndRegister` with `{ rgOk: true, fjState: "absent" }` → entry matches the fj-only filter shape. - Unit test: `dedupeAndRegister` with `{ rgOk: false, fjState: "absent" }` → entry matches the combined filter shape. - Unit test: `dedupeAndRegister` with `{ rgOk: true, fjState: "unauthenticated" }` → entry stays a plain string. - Unit test: two sequential `dedupeAndRegister` calls against the same settings file with `{ rgOk: false, fjState: "ok" }` then `{ rgOk: true, fjState: "ok" }` → final entry is the plain string (rg exclusion removed, not retained). - Unit test: pre-populate `globalSettings` such that the just-installed entry can't be found (e.g. directly exercise `rewritePackageEntry` against a settings object lacking the expected source) → no throw, warning logged, `aborted === false`. - Re-run the full existing `dedupeAndRegister` test suite — all pass unmodified. - `npm test` — all green.

david added this to the Install Tool Detection: M3 - Settings.json filtering milestone

2026-09-08 00:02:48 +00:00

david added the

enhancement

label

2026-09-08 00:02:48 +00:00

david referenced this issue

2026-09-08 00:05:18 +00:00

Add end-to-end main() tests for the fj/rg tool-state matrix #172

david referenced this issue

2026-09-08 02:38:37 +00:00

170: Wire settings.json filter rewrite into dedupeAndRegister() #179