my-pi/pi-permission-system/docs/plans/0356-harden-config-pipeline-dropped-fields.md

issue, issue_title

issue	issue_title
356	Harden config pipeline against silently-dropped fields (follow-up to #332)

Harden the config pipeline against silently-dropped fields

Problem Statement

Issue #332 fixed a specific bug — toolInputPreviewMaxLength / toolTextSummaryMaxLength were declared on PermissionSystemExtensionConfig and read by ToolPreviewFormatter, but silently dropped by the UnifiedPermissionConfig intermediate in config-loader.ts. The fix wired those two fields through the loader, but it did not close the class of bug that let the omission ship undetected.

Two structural gaps remain:

1. normalizePermissionSystemConfig accepts unknown and re-parses its argument through toRecord(raw). Its sole production caller (ConfigStore.refresh) feeds it mergeResult.merged, a UnifiedPermissionConfig. Because the parameter is unknown, TypeScript never objected that the merged object lacked the two fields. A field declared on the runtime type but missing from the merge intermediate is a silent runtime drop, not a compile error.
2. Every existing test injects config at a single layer in isolation. Nothing exercises the full seam — temp config.json → loadAndMergeConfigs → normalizePermissionSystemConfig — so a field dropped in the middle is never observed end to end.

This plan attacks the class of bug rather than the single instance.

Goals

• Retype normalizePermissionSystemConfig's parameter from unknown to UnifiedPermissionConfig and read fields directly, so a future field declared on the runtime type but absent from the merge intermediate becomes a compile error.
• Drop the now-redundant defensive coercion (toRecord, normalizeOptionalPositiveInt, normalizeOptionalStringArray) from normalizePermissionSystemConfig — that work already happens at the JSON boundary in normalizeUnifiedConfig.
• Add one full-pipeline seam test in a new test/config-pipeline.test.ts that writes a temp config.json, runs it through loadAndMergeConfigs → normalizePermissionSystemConfig, and asserts a runtime knob and a preview-length field both survive end to end.

This change is not breaking: observable runtime behavior is unchanged. The two production call sites already feed typed objects through normalizeUnifiedConfig first, so the removed coercion is dead code for production; only test-only garbage injection is affected.

Non-Goals

• The secondary saveExtensionConfig behavior from #332 (length fields not written back) is out of scope — ConfigStore.save already preserves existing file fields via ...existing.config, and the modal only edits the three booleans. config-store.ts is not modified by this plan.
• No changes to normalizeUnifiedConfig, mergeUnifiedConfigs, or the boundary parsing — they already do the defensive work correctly.
• No new config fields, schema entries, or example-config changes.
• No narrowing of the parameter to Omit<UnifiedPermissionConfig, "permission"> (see Design Overview — accepted minor ISP slack).

Background

Relevant modules:

• src/extension-config.ts — declares PermissionSystemExtensionConfig, DEFAULT_EXTENSION_CONFIG, and normalizePermissionSystemConfig. The function currently does toRecord(raw) then reads each field defensively (record.debugLog === true, normalizeOptionalPositiveInt(record.toolInputPreviewMaxLength), etc.).
• src/config-loader.ts — declares UnifiedPermissionConfig (all-optional intermediate) and normalizeUnifiedConfig, which already does the full defensive parse at the JSON boundary: normalizeOptionalBoolean for the three booleans, normalizeOptionalPositiveInt for the two length fields, normalizeOptionalStringArray for piInfrastructureReadPaths.
• src/config-store.ts — the two production call sites. refresh passes mergeResult.merged (a UnifiedPermissionConfig); save passes next (a PermissionSystemExtensionConfig).
• src/common.ts — exports toRecord, normalizeOptionalPositiveInt, normalizeOptionalStringArray.

Applicable constraint from the package skill (package-pi-permission-system): "Treat any declared config field not read at runtime as a maintenance trap" and "A field on the runtime type but not the merge intermediate is silently dropped before runtime (the #332 / #347 bug class)." This plan makes that trap a compile error.

Existing boundary coverage in test/config-loader.test.ts already exercises the defensive parse the retype removes from normalizePermissionSystemConfig:

• Lines 188–199 — non-boolean values (debugLog: "yes", permissionReviewLog: 1, yoloMode: null) are dropped to undefined.
• Lines 156–172 — debugLog present/false/missing.
• Lines 262–325 — both length fields: valid positive integer parsed, absent omitted, and invalid values (0, -1, fractional, string, boolean) dropped.

This is why the redundant cases in test/extension-config.test.ts can be deleted rather than relocated.

Design Overview

Type model

After the change, normalizePermissionSystemConfig converts a typed UnifiedPermissionConfig into a PermissionSystemExtensionConfig by applying defaults to the three required booleans and passing through the optional fields:

export function normalizePermissionSystemConfig(
  raw: UnifiedPermissionConfig,
): PermissionSystemExtensionConfig {
  const result: PermissionSystemExtensionConfig = {
    debugLog: raw.debugLog === true,
    permissionReviewLog: raw.permissionReviewLog !== false,
    yoloMode: raw.yoloMode === true,
  };
  if (raw.piInfrastructureReadPaths !== undefined) {
    result.piInfrastructureReadPaths = raw.piInfrastructureReadPaths;
  }
  if (raw.toolInputPreviewMaxLength !== undefined) {
    result.toolInputPreviewMaxLength = raw.toolInputPreviewMaxLength;
  }
  if (raw.toolTextSummaryMaxLength !== undefined) {
    result.toolTextSummaryMaxLength = raw.toolTextSummaryMaxLength;
  }
  return result;
}

Behavior is preserved exactly: undefined booleans default the same way (=== true → false, !== false → true), and the optional fields are passed through unchanged. The function no longer imports toRecord, normalizeOptionalPositiveInt, or normalizeOptionalStringArray — those imports become unused and are removed.

Why this achieves the safety goal

The point of the retype is the omission-becomes-compile-error property. Keeping toRecord would defeat it: toRecord returns Record<string, unknown>, so record.toolInputPreviewMaxLength is unknown and the type checker cannot see a missing field on the input. Reading raw.toolInputPreviewMaxLength directly against a typed UnifiedPermissionConfig means that if a future field is declared on PermissionSystemExtensionConfig, read here, but never added to UnifiedPermissionConfig, the field access is a compile error.

Call-site compatibility

Both production call sites already feed assignable types — no config-store.ts change is needed:

// ConfigStore.refresh — mergeResult.merged is already UnifiedPermissionConfig
const runtimeConfig = normalizePermissionSystemConfig(mergeResult.merged);

// ConfigStore.save — next is PermissionSystemExtensionConfig, structurally
// assignable to UnifiedPermissionConfig (every field present, all optional there)
const normalized = normalizePermissionSystemConfig(next);

PermissionSystemExtensionConfig (required booleans + optional extras) is assignable to UnifiedPermissionConfig (all-optional superset including permission?), so save compiles unchanged.

ISP note (accepted slack)

normalizePermissionSystemConfig reads 6 of UnifiedPermissionConfig's 7 fields — it never reads permission. A strict ISP reading would narrow the parameter to Omit<UnifiedPermissionConfig, "permission">. We accept the single unused optional field instead: UnifiedPermissionConfig is the natural merged-config domain object and the exact type of mergeResult.merged, the issue prescribes typing the parameter as UnifiedPermissionConfig, and the compile-error safety property holds either way. Adding a narrowing alias would be speculative surface for negligible benefit.

Design-review checklist result

Check	Finding
Dependency width	Param reads 6/7 fields; one unused optional (permission) accepted (see ISP note)
Law of Demeter	No chained access introduced
Output arguments	None — function returns a fresh object
Scattered resets	N/A
Parameter relay	N/A
Test mock depth	Removes test-only as unknown casts in config-modal.test.ts (improvement)

No structural smells introduced; the change removes redundant code and a test-only cast.

Module-Level Changes

• src/extension-config.ts
  • Change normalizePermissionSystemConfig(raw: unknown) to (raw: UnifiedPermissionConfig); read fields directly; drop the toRecord call and the normalizeOptionalPositiveInt / normalizeOptionalStringArray coercion.
  • Add import type { UnifiedPermissionConfig } from "./config-loader";.
  • Remove the now-unused imports normalizeOptionalPositiveInt, normalizeOptionalStringArray, toRecord from the ./common import (verify toRecord has no other use in the file — detectMisplacedPermissionKeys takes a Record directly, so it does not).
• test/extension-config.test.ts
  • Delete the four redundant garbage-input cases (now uncompilable against the typed parameter and already covered at the boundary in config-loader.test.ts): "coerces non-boolean values to their defaults", "handles null/undefined input gracefully", "omits toolInputPreviewMaxLength for invalid values", "omits toolTextSummaryMaxLength for invalid values".
  • Keep the valid-input cases (valid config, the three boolean defaults via {}, includes/omits for both length fields) — they pass typed objects and still compile.
• test/config-modal.test.ts
  • Fix the two call sites that pass JSON.parse(readFileSync(configPath, "utf-8")) as unknown to normalizePermissionSystemConfig (lines ~147 and ~156): route them through loadUnifiedConfig(configPath).config (reads + normalizes the file into a UnifiedPermissionConfig, mirroring production) and import loadUnifiedConfig from #src/config-loader.
  • The other two call sites pass typed values (config, next) and need no change.
• test/config-pipeline.test.ts (new)
  • Add the full-pipeline seam test (change 2).

No docs/architecture/, schema, example-config, README, or docs/configuration.md updates — no config surface changes. The package skill's bug-class note already describes this trap; no skill edit required.

Test Impact Analysis

What the change enables and affects:

1. New coverage enabled — the seam test in test/config-pipeline.test.ts is the first test that runs config through the real loadAndMergeConfigs → normalizePermissionSystemConfig path on disk, catching any future mid-pipeline drop (not just the two length fields).
2. Tests that become redundant — the four garbage-input cases in test/extension-config.test.ts duplicate boundary coverage already present in test/config-loader.test.ts (lines 188–199 for booleans, 296–325 for invalid length values). They are deleted, not relocated, because the boundary already tests the identical behavior.
3. Tests that must stay as-is — the valid-input cases in test/extension-config.test.ts (default application, length-field passthrough) genuinely exercise normalizePermissionSystemConfig's remaining responsibility (defaults + passthrough) and are kept. All of test/config-loader.test.ts stays — it owns the boundary defensive-parse contract.

TDD Order

1. Add the full-pipeline seam test (change 2). Surface: new test/config-pipeline.test.ts. Covers: write a temp config.json with a runtime knob (e.g. debugLog: true) and a preview-length field (e.g. toolInputPreviewMaxLength: 1000); run loadAndMergeConfigs(agentDir, cwd, extensionRoot); pass .merged to normalizePermissionSystemConfig; assert both values survive end to end. This passes immediately (the #332 loader fix is already in place) — it is a regression guard documenting the seam, established before the refactor as a safety net. Commit: test: add full-pipeline config seam regression test (#356).
2. Retype normalizePermissionSystemConfig and migrate its tests (change 1). Surface: src/extension-config.ts, test/extension-config.test.ts, test/config-modal.test.ts. This is a single atomic commit: changing the parameter type breaks the four garbage-input test cases and the two as unknown call sites at the type level, so the production change, the test deletions, and the config-modal.test.ts call-site updates must land together. Run pnpm run check immediately after — the retype is the one interface-shape change in this plan, and pnpm --filter @gotgenes/pi-permission-system exec vitest run to confirm the seam test (step 1) and the surviving unit tests stay green. Commit: refactor: type normalizePermissionSystemConfig parameter as UnifiedPermissionConfig (#356).

Risks and Mitigations

• Risk: a hidden production caller passes genuinely untrusted unknown to normalizePermissionSystemConfig, relying on the defensive coercion. Mitigation: confirmed via grep — only two production call sites (ConfigStore.refresh, ConfigStore.save), both feeding typed objects already normalized upstream; no untrusted path exists.
• Risk: deleting the garbage-input tests loses coverage of invalid-value handling. Mitigation: that behavior is fully covered at the JSON boundary in test/config-loader.test.ts (booleans lines 188–199, length fields 296–325); the deletions are duplicates, not unique coverage.
• Risk: config-modal.test.ts call-site rewrite changes what the test asserts. Mitigation: loadUnifiedConfig(configPath).config reads and normalizes the same file the test wrote, mirroring the production load path more faithfully than the previous JSON.parse(...) as unknown cast — the asserted round-trip is preserved.

Open Questions

None blocking. A future follow-up could narrow the parameter to Omit<UnifiedPermissionConfig, "permission"> if more unused fields accumulate, but that is not warranted now.