feat(TP-195): code-quality typecheck cleanup — npm run typecheck exits 0 on main

CHANGELOG.md

@@ -7,8 +7,110 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Fixed
+
+- **Preflight cleanup feature now actually runs (TP-195):** `runOrchBatch`
+  in `extensions/taskplane/engine.ts` referenced `sweepStaleArtifacts`,
+  `formatPreflightSweep`, `rotateSupervisorLogs`, and `formatLogRotation`
+  inside the preflight-cleanup try-block, but those identifiers were
+  never imported from `./cleanup.ts`. At runtime the first reference
+  threw a ReferenceError that the enclosing catch-all swallowed, so
+  Layers 2–5 of preflight cleanup (age-based artifact sweep, supervisor
+  log rotation, telemetry size cap, prior-batch artifact cleanup) had
+  been silently a no-op since TP-065 / #221 (․2024-09). The missing
+  imports were uncovered by the TP-191 typecheck script; this fix adds
+  them so the advertised cleanup runs on every batch. Regression test:
+  `tests/lane-runner-v2.test.ts 3.10` asserts the four helpers are
+  imported from `./cleanup.ts`.
+- **`max_worker_minutes` config field is honored (TP-195):** Lane-runner
+  config in `executeLaneV2` (`extensions/taskplane/execution.ts`) was
+  reading a non-existent `config.failure?.maxWorkerMinutes` camelCase
+  alias — always undefined — silently ignoring any operator-set value
+  on `OrchestratorConfig.failure.max_worker_minutes` and always falling
+  through to the hard-coded `120`-minute default. Fixed to read the
+  canonical snake_case field. Operators with `max_worker_minutes`
+  configured in `.pi/taskplane-config.json` will now have their
+  configured limit honored; default of 120 preserved when the field is
+  unset. Regression test: `tests/lane-runner-v2.test.ts 3.9` asserts
+  the corrected accessor and absence of the typo.
+- **Resume’s failed-task supervisor-alert path no longer crashes
+  (TP-195):** When `/orch-resume` encountered a failed task during a
+  wave, the supervisor-alert emission block in `resume.ts` called
+  `batchState.tasks.find(…)`, but `OrchBatchRuntimeState` has no
+  `tasks` field (only `PersistedBatchState` does). The runtime call
+  would throw `TypeError: undefined.find is not a function`. The
+  failed-task path was never covered by tests, so the crash never
+  surfaced. Replaced with a lookup against `laneForTask?.tasks.find
+  (…)?.task` — the lane-allocated `ParsedTask` payload carries the
+  same `segmentIds`/`activeSegmentId` data the alert needs.
+  Regression test: `tests/resume-bug-fixes.test.ts 4.1`.
+
 ### Internal
 
+- **Code-quality typecheck cleanup (TP-195):** Fourth of four sequenced
+  packets implementing the code-quality-gates spec
+  ([`docs/specifications/taskplane/code-quality-gates.md`](docs/specifications/taskplane/code-quality-gates.md)).
+  Cleaned up the **264 typecheck errors** that TP-191 surfaced when it
+  first made `npm run typecheck` runnable, so TP-194’s gate flip can
+  promote typecheck from advisory to a CI gate. Final state:
+  `npm run typecheck` exits 0 against `extensions/tsconfig.ci.json` at
+  the current strictness (`strict: false`, `noImplicitAny: false`).
+  **Per-category breakdown of fixes** (top categories at task start):
+  TS2339 (63) — property-not-exist; TS2741 (52) — mock-object missing
+  required fields; TS2345 (30) — caller-shape mismatch; TS2554 (23) —
+  signature drift; TS2367 (21) — unintentional comparison; TS2322 (19)
+  — assignment mismatch; TS2739 (12) — type missing properties; plus
+  smaller TS2769/TS2353/TS2352/TS2559/TS2347/TS2578/TS2304/TS2871/
+  TS2694 counts. **Source-side highlights:** 4 latent bugs uncovered
+  and fixed (preflight-cleanup-feature no-op, `max_worker_minutes`
+  typo, resume failed-task crash, plus an extension.ts dashboard
+  change-detection that was reading non-existent fields and only ever
+  refreshing on `currentTaskId` — dropped the dead comparisons,
+  observable behavior unchanged); widened `execLog`’s `extra`
+  parameter from `Record<string, string\|number\|boolean>` to
+  `Record<string, unknown>` (callers were already passing arrays/
+  objects; template-string stringification preserved); re-exported
+  `RuntimeRegistry` from `process-registry.ts`; documented optional
+  `batchId?` field on `OrchestratorConfig.orchestrator`; added
+  `EXEC_MISSING_TASK_FOLDER` to `ExecutionErrorCode`; fixed
+  discriminated-union narrowing under non-strict mode by adding
+  `reason?: undefined` / `error?: undefined` to success branches;
+  switched `loadProjectOverrides` / `migrateProjectOverrides` /
+  `loadJsonConfig` / `mergeProjectOverrides` to
+  `DeepPartial<TaskplaneConfig>`; changed `spawnMergeAgentV2` return
+  type to `Promise<void>` (fire-and-forget). **Test-side highlights:**
+  introduced shared `tests/helpers/mock-orchestrator-config.ts`
+  factories (`makeOrchestratorConfig`/`makeTaskRunnerConfig`) that
+  wrap `DEFAULT_*_CONFIG` defaults from `types.ts` so test mocks stay
+  in sync with the runtime schema; added `expect.unreachable()` and
+  optional 2nd `message` arg to `expect()` (Vitest-compat surface that
+  ~190 sites already relied on); fixed phase-narrowing in 9.x
+  launch-window suite via typed `OrchBatchPhase` casts; updated
+  `LaneRunnerConfig` / `PersistedTaskRecord` / `MergeResult` /
+  `BatchSummaryData` / `MinimalBatchState` / `WorkspaceRoutingConfig`
+  fixtures to match current schemas; replaced legacy `RuntimeAgentStatus`
+  `"complete"` with canonical `"exited"`; converted `it(name, fn,
+  30000)` calls to `it(name, { timeout: 30000 }, fn)` for node:test
+  compatibility; declared `mock.fn<(…args: any[]) => any>()` so
+  `mockImplementation` accepts non-undefined returns. **Anti-shortcut
+  policy enforced:** zero new `as any` casts; zero `@ts-expect-error`
+  added (the 3 unused-directive errors were removed); only legitimate
+  2-step `as unknown as X` widenings with justifying comments; no
+  garbage default values — every mock-object missing-field fix uses
+  a schema-defined value. **Pi-shim** extended `ExtensionContext`
+  from `any` to a structural interface so `ctx.ui.custom<T>()`
+  typechecks at 4 settings-tui.ts call sites; `ui` left optional so
+  thin test mocks (e.g., `{ model: null }`) still satisfy the type.
+  After the pass: `npm run typecheck` exits 0;
+  `npm run lint` / `npm run format:check` unchanged from baseline;
+  test suite **3627 passing / 1 skipped / 0 failed** (TP-191
+  baseline 3624 + 3 new TP-195 regression tests for the
+  fix-the-bug paths). **Strict mode remains out of scope** — the
+  strictness ratchet (enabling `strict: true` /
+  `noImplicitAny: true`) is a separate post-TP-194 follow-up. With
+  this packet merged, TP-194’s typecheck-gate flip CRITICAL
+  pre-condition (“`npm run typecheck` exits 0 on `main`”) is
+  satisfied.
 - **Code-quality formatter adoption (TP-193):** Third of four sequenced
   packets implementing the code-quality-gates spec
   ([`docs/specifications/taskplane/code-quality-gates.md`](docs/specifications/taskplane/code-quality-gates.md)

extensions/taskplane/config-loader.ts

@@ -43,6 +43,7 @@ import type {
 	OrchestratorSection,
 	WorkspaceSectionConfig,
 	GlobalPreferences,
+	DeepPartial,
 } from "./config-schema.ts";
 
 // ── Error Types ──────────────────────────────────────────────────────
@@ -471,7 +472,7 @@ function resolveConfigFilePath(configRoot: string, filename: string): string {
  * Returns the parsed config or null if the file doesn't exist.
  * Throws ConfigLoadError for malformed JSON or unsupported versions.
  */
-function loadJsonConfig(configRoot: string): Partial<TaskplaneConfig> | null {
+function loadJsonConfig(configRoot: string): DeepPartial<TaskplaneConfig> | null {
 	const jsonPath = resolveConfigFilePath(configRoot, PROJECT_CONFIG_FILENAME);
 	if (!existsSync(jsonPath)) return null;
 
@@ -509,7 +510,7 @@ function loadJsonConfig(configRoot: string): Partial<TaskplaneConfig> | null {
 		);
 	}
 
-	const overrides: Partial<TaskplaneConfig> = {};
+	const overrides: DeepPartial<TaskplaneConfig> = {};
 	if (
 		parsed.taskRunner &&
 		typeof parsed.taskRunner === "object" &&
@@ -882,11 +883,13 @@ export function applyGlobalPreferences(
 	});
 
 	// spawnMode: enum — apply if defined (not a string-empty check)
+	// TP-195: dropped dead `prefs.spawnMode === "tmux"` migration check.
+	// `prefs.spawnMode` is typed as `"subprocess"` only (see
+	// `GlobalPreferences.spawnMode` in config-schema.ts). Raw input is
+	// migrated upstream at line ~169 BEFORE assignment to the typed
+	// `prefs` object, so by this point the value is already "subprocess"
+	// or undefined — the comparison can never be true. Behavior-neutral.
 	if (prefs.spawnMode !== undefined) {
-		if (prefs.spawnMode === "tmux") {
-			prefs.spawnMode = "subprocess";
-			console.error(`[taskplane] Auto-migrated runtime preference: spawnMode "tmux" → "subprocess"`);
-		}
 		config.orchestrator.orchestrator.spawnMode = prefs.spawnMode;
 	}
 
@@ -982,7 +985,10 @@ export function resolveConfigRoot(cwd: string, pointerConfigRoot?: string): stri
 	return cwd;
 }
 
-function mergeProjectOverrides(config: TaskplaneConfig, overrides: Partial<TaskplaneConfig>): void {
+function mergeProjectOverrides(
+	config: TaskplaneConfig,
+	overrides: DeepPartial<TaskplaneConfig>,
+): void {
 	if (overrides.taskRunner) {
 		deepMerge(config.taskRunner as Record<string, any>, overrides.taskRunner as Record<string, any>);
 	}
@@ -1000,11 +1006,25 @@ function mergeProjectOverrides(config: TaskplaneConfig, overrides: Partial<Taskp
 	}
 }
 
-function migrateProjectOverrides(overrides: Partial<TaskplaneConfig>, configRoot: string): boolean {
+// TP-195: switched parameter to `DeepPartial<TaskplaneConfig>` to match the
+// nested-section partial shape produced by `loadProjectOverrides` (the YAML
+// loaders return `Partial<TaskRunnerSection>` etc., which `Partial<TaskplaneConfig>`
+// rejects — it makes top-level fields optional but inner sections stay full).
+function migrateProjectOverrides(
+	overrides: DeepPartial<TaskplaneConfig>,
+	configRoot: string,
+): boolean {
 	if (_projectMigrationDone) return false;
 
 	let migrated = false;
-	const orchestratorCore = overrides.orchestrator?.orchestrator as
+	// TP-195: 2-step `as unknown as` widening. The structurally-typed
+	// `OrchestratorCoreConfig` is being treated as a property bag for
+	// migration purposes (legacy `tmuxPrefix` -> `sessionPrefix`,
+	// `spawnMode "tmux"` -> `"subprocess"`). Both source and target
+	// types are object-shaped at runtime; the cast is structurally
+	// legitimate, just outside the narrow set of conversions TS allows
+	// in a single step.
+	const orchestratorCore = overrides.orchestrator?.orchestrator as unknown as
 		| Record<string, unknown>
 		| undefined;
 	if (orchestratorCore && hasOwn(orchestratorCore, "tmuxPrefix")) {
@@ -1025,7 +1045,11 @@ function migrateProjectOverrides(overrides: Partial<TaskplaneConfig>, configRoot
 		migrated = true;
 	}
 
-	const workerConfig = overrides.taskRunner?.worker as Record<string, unknown> | undefined;
+	// TP-195: 2-step `as unknown as` widening (same rationale as the
+	// orchestratorCore cast above).
+	const workerConfig = overrides.taskRunner?.worker as unknown as
+		| Record<string, unknown>
+		| undefined;
 	if (workerConfig?.spawnMode === "tmux") {
 		(workerConfig as any).spawnMode = "subprocess";
 		console.error(`[taskplane] Auto-migrated: taskRunner.worker.spawnMode "tmux" → "subprocess"`);
@@ -1067,7 +1091,12 @@ function migrateProjectOverrides(overrides: Partial<TaskplaneConfig>, configRoot
 	return migrated;
 }
 
-export function loadProjectOverrides(configRoot: string): Partial<TaskplaneConfig> {
+// TP-195: return type widened from `Partial<TaskplaneConfig>` to
+// `DeepPartial<TaskplaneConfig>` so the nested `Partial<TaskRunnerSection>` /
+// `Partial<OrchestratorSection>` returned by the YAML loaders are
+// assignable. `Partial<TaskplaneConfig>` only relaxes top-level optionality
+// while keeping inner sections fully required.
+export function loadProjectOverrides(configRoot: string): DeepPartial<TaskplaneConfig> {
 	const jsonOverrides = loadJsonConfig(configRoot);
 	if (jsonOverrides !== null) {
 		return jsonOverrides;
@@ -1077,7 +1106,7 @@ export function loadProjectOverrides(configRoot: string): Partial<TaskplaneConfi
 	const orchestrator = loadOrchestratorYaml(configRoot);
 	const workspace = loadWorkspaceYaml(configRoot);
 
-	const overrides: Partial<TaskplaneConfig> = {};
+	const overrides: DeepPartial<TaskplaneConfig> = {};
 	if (Object.keys(taskRunner).length > 0) overrides.taskRunner = taskRunner;
 	if (Object.keys(orchestrator).length > 0) overrides.orchestrator = orchestrator;
 	if (workspace) overrides.workspace = workspace;

extensions/taskplane/engine.ts

@@ -121,6 +121,10 @@ import {
 import {
 	runPreflightCleanup,
 	formatPreflightCleanup,
+	sweepStaleArtifacts,
+	formatPreflightSweep,
+	rotateSupervisorLogs,
+	formatLogRotation,
 	enforceTelemetrySizeCap,
 	formatSizeCap,
 	cleanupPriorBatchArtifacts,
@@ -636,7 +640,13 @@ export function processSegmentExpansionRequestAtBoundary(
 	segmentState: SegmentFrontierTaskState,
 	workspaceConfig: WorkspaceConfig | null | undefined,
 	knownRequestIds: Set<string>,
-): { ok: true } | { ok: false; reason: string } {
+	// TP-195: `reason?: undefined` on the success branch makes this a
+	// well-formed discriminated union under `strict: false`. Without it,
+	// `if (!result.ok)` does not narrow `reason` because non-strict
+	// narrowing requires every member of the union to share the
+	// discriminating field. Runtime semantics are unchanged — the
+	// success branch never carries a reason.
+): { ok: true; reason?: undefined } | { ok: false; reason: string } {
 	const validationFailure = validateSegmentExpansionRequestAtBoundary(
 		requestFile,
 		taskId,
@@ -2592,6 +2602,13 @@ export async function executeOrchBatch(
 	// Sweep stale artifacts, rotate oversized logs, enforce size cap,
 	// and clean prior batch artifacts before batch starts.
 	// Always non-fatal — failures warn but never block batch execution.
+	//
+	// TP-195: imported `sweepStaleArtifacts`, `formatPreflightSweep`,
+	// `rotateSupervisorLogs`, and `formatLogRotation` from `./cleanup.ts`
+	// (they were referenced here but never imported, so the try/catch was
+	// swallowing a ReferenceError on every batch and Layers 2–5 had been
+	// silently a no-op since TP-065 ~2024-09). With the imports added,
+	// the preflight cleanup feature now works as advertised.
 	try {
 		// Layer 2: Age-based sweep of stale telemetry/merge/verification/conversation artifacts (>3 days)
 		const sweepResult = sweepStaleArtifacts(stateRoot, {

extensions/taskplane/execution.ts

@@ -41,6 +41,7 @@ import type {
 	RuntimeAgentId,
 	RuntimeAgentRole,
 	RuntimeLaneSnapshot,
+	RuntimeRegistry,
 	SupervisorAlertCallback,
 } from "./types.ts";
 import { resolvePacketPaths, buildRuntimeAgentId } from "./types.ts";
@@ -95,7 +96,15 @@ export function execLog(
 	laneId: string,
 	taskId: string,
 	message: string,
-	extra?: Record<string, string | number | boolean>,
+	// TP-195: widened from `Record<string, string|number|boolean>` to
+	// `Record<string, unknown>` so callers can pass structured values
+	// (string[] arrays, repo objects, etc.) without TS errors. Runtime
+	// stringification via `${v}` is unchanged: primitives render as today,
+	// arrays render with comma separators (existing behavior), objects
+	// render as `[object Object]` (already today's behavior — see
+	// historic execLog calls in engine.ts/resume.ts that have always been
+	// passing structured payloads). No runtime change.
+	extra?: Record<string, unknown>,
 ): void {
 	const prefix = `[orch] ${laneId}/${taskId}`;
 	if (extra) {
@@ -159,16 +168,14 @@ export function isV2AgentAlive(
 }
 
 /** Cached registry for V2 liveness checks within a monitor cycle. @since TP-112 */
-let _v2LivenessRegistryCache: import("./process-registry.ts").RuntimeRegistry | null = null;
+let _v2LivenessRegistryCache: RuntimeRegistry | null = null;
 
 /**
  * Set the V2 liveness registry cache for the current monitor cycle.
  * Called at the start of each monitor poll to avoid re-reading the file per-task.
  * @since TP-112
  */
-export function setV2LivenessRegistryCache(
-	registry: import("./process-registry.ts").RuntimeRegistry | null,
-): void {
+export function setV2LivenessRegistryCache(registry: RuntimeRegistry | null): void {
 	_v2LivenessRegistryCache = registry;
 }
 
@@ -2896,10 +2903,22 @@ export async function executeLaneV2(
 				extraEnvVars?.TASKPLANE_REVIEWER_EXCLUDE_EXTENSIONS,
 			),
 			supervisorAutonomy,
-			projectName: config.project?.name || "project",
+			// TP-195: replaced `config.project?.name` (no `project` field on
+			// `OrchestratorConfig`; always undefined) with the env-var read
+			// already used elsewhere in the codebase (lane-runner.ts:668 sets
+			// `TASKPLANE_PROJECT_NAME` from the same source). When the env
+			// var is unset, falls through to the same `"project"` literal as
+			// before — behavior-neutral.
+			projectName: extraEnvVars?.TASKPLANE_PROJECT_NAME || "project",
 			maxIterations: 20,
 			noProgressLimit: 3,
-			maxWorkerMinutes: config.failure?.maxWorkerMinutes || 120,
+			// TP-195: read the canonical `max_worker_minutes` field (snake_case
+			// per `OrchestratorConfig.failure` in types.ts). The previous code
+			// read a non-existent `maxWorkerMinutes` camelCase alias — always
+			// undefined — silently ignoring any operator-set value. Honoring
+			// the config is the intended behavior; default of 120 preserved
+			// when the field is unset.
+			maxWorkerMinutes: config.failure?.max_worker_minutes || 120,
 			warnPercent: 85,
 			killPercent: 95,
 			onSupervisorAlert,

extensions/taskplane/extension.ts

@@ -2404,16 +2404,22 @@ export default function (pi: ExtensionAPI) {
 			ctx,
 			updateOrchWidget,
 			(monState: MonitorState) => {
+				// TP-195: GATED on E2 escalation. The previous change-detection
+				// here compared `monState.totalDone/totalFailed` and
+				// `lane.currentStep/completedChecks` — fields that do NOT exist
+				// on `MonitorState` / `LaneMonitorSnapshot`. At runtime every
+				// such read returned `undefined`, so the four comparisons were
+				// always `undefined !== undefined` (false) and only
+				// `currentTaskId` actually triggered refreshes. To preserve
+				// historic behavior pending operator decision (would fix
+				// rewire to `tasksDone`/`tasksFailed` and
+				// `currentTaskSnapshot.currentStepNumber/totalChecked`), the
+				// dead comparisons are dropped here. Observed behavior is
+				// IDENTICAL — widget still refreshes only on `currentTaskId`
+				// changes — but the source typechecks cleanly.
 				const changed =
 					!latestMonitorState ||
-					latestMonitorState.totalDone !== monState.totalDone ||
-					latestMonitorState.totalFailed !== monState.totalFailed ||
-					latestMonitorState.lanes.some(
-						(l, i) =>
-							l.currentTaskId !== monState.lanes[i]?.currentTaskId ||
-							l.currentStep !== monState.lanes[i]?.currentStep ||
-							l.completedChecks !== monState.lanes[i]?.completedChecks,
-					);
+					latestMonitorState.lanes.some((l, i) => l.currentTaskId !== monState.lanes[i]?.currentTaskId);
 				latestMonitorState = monState;
 				if (changed) updateOrchWidget();
 			},