mirror of
https://github.com/ksyasuda/SubMiner.git
synced 2026-02-27 18:22:41 -08:00
fix(config): enforce strict startup config parsing
This commit is contained in:
@@ -1,10 +1,10 @@
|
|||||||
---
|
---
|
||||||
id: TASK-72
|
id: TASK-72
|
||||||
title: Make startup config loading strict with clear user-facing errors
|
title: Make startup config loading strict with clear user-facing errors
|
||||||
status: To Do
|
status: Done
|
||||||
assignee: []
|
assignee: []
|
||||||
created_date: '2026-02-18 11:35'
|
created_date: '2026-02-18 11:35'
|
||||||
updated_date: '2026-02-18 11:35'
|
updated_date: '2026-02-21 23:26'
|
||||||
labels:
|
labels:
|
||||||
- config
|
- config
|
||||||
- ux
|
- ux
|
||||||
@@ -31,14 +31,35 @@ Startup config loading currently falls back to default config when strict parse
|
|||||||
|
|
||||||
## Acceptance Criteria
|
## Acceptance Criteria
|
||||||
<!-- AC:BEGIN -->
|
<!-- AC:BEGIN -->
|
||||||
- [ ] #1 Startup and hot-reload use consistent strictness semantics
|
- [x] #1 Startup and hot-reload use consistent strictness semantics
|
||||||
- [ ] #2 Malformed config surfaces explicit actionable error
|
- [x] #2 Malformed config surfaces explicit actionable error
|
||||||
- [ ] #3 Tests cover malformed startup config path
|
- [x] #3 Tests cover malformed startup config path
|
||||||
<!-- AC:END -->
|
<!-- AC:END -->
|
||||||
|
|
||||||
|
## Implementation Notes
|
||||||
|
|
||||||
|
<!-- SECTION:NOTES:BEGIN -->
|
||||||
|
2026-02-21: implementing plan Task 1 only in this run (strict constructor startup load + malformed startup config test coverage), no commit.
|
||||||
|
|
||||||
|
2026-02-21 Task 1 complete: `ConfigService` constructor now strict-loads startup config and throws `ConfigStartupParseError` (path + parse reason + restart guidance) on malformed parse; added malformed-startup constructor test in `src/config/config.test.ts`; verified with `bun run build && node --test dist/config/config.test.js` (pass).
|
||||||
|
|
||||||
|
2026-02-21 Task 2-4 complete: added shared `buildConfigParseErrorDetails` formatter and reused it in startup/hot-reload parse-failure handling (`src/main/config-validation.ts`, `src/main/runtime/startup-config.ts`).
|
||||||
|
|
||||||
|
2026-02-21 startup wiring updated in `src/main.ts`: `ConfigService` construction now catches `ConfigStartupParseError` and fails startup via `failStartupFromConfig` with user-facing dialog + actionable details, keeping non-parse failures rethrown.
|
||||||
|
|
||||||
|
2026-02-21 docs updated in `docs/configuration.md` to clarify malformed JSON/JSONC is startup-blocking while value-level validation remains warn-and-fallback.
|
||||||
|
|
||||||
|
Verification: `bun test src/config/config.test.ts src/main/config-validation.test.ts src/main/runtime/startup-config.test.ts` => pass (46/46). Full `bun run build` currently blocked by unrelated pre-existing TS errors in in-flight TASK-75/TASK-77 files (`src/main/runtime/mpv-osd-log.test.ts`, `src/main/runtime/mpv-osd-runtime-handlers.test.ts`, `src/core/services/tokenizer/parser-selection-stage.test.ts`).
|
||||||
|
<!-- SECTION:NOTES:END -->
|
||||||
|
|
||||||
|
## Final Summary
|
||||||
|
|
||||||
|
<!-- SECTION:FINAL_SUMMARY:BEGIN -->
|
||||||
|
Startup config loading is now strict and aligned with hot-reload semantics. `ConfigService` no longer silently falls back on malformed startup config; it throws a typed `ConfigStartupParseError`, and `main.ts` now converts that into the same actionable user-facing startup failure path used elsewhere (dialog/error details include config path, parse reason, and restart guidance). Added malformed-startup constructor regression coverage plus shared parse-error formatter tests, and updated configuration docs to distinguish fatal syntax errors from warn-and-fallback value validation.
|
||||||
|
<!-- SECTION:FINAL_SUMMARY:END -->
|
||||||
|
|
||||||
## Definition of Done
|
## Definition of Done
|
||||||
<!-- DOD:BEGIN -->
|
<!-- DOD:BEGIN -->
|
||||||
- [ ] #1 Config and startup tests pass
|
- [x] #1 Config and startup tests pass
|
||||||
- [ ] #2 Docs updated for new startup error behavior
|
- [x] #2 Docs updated for new startup error behavior
|
||||||
<!-- DOD:END -->
|
<!-- DOD:END -->
|
||||||
|
|
||||||
|
|||||||
@@ -0,0 +1,44 @@
|
|||||||
|
# Agent: `codex-task72-strict-startup-config-20260221T231804Z-3ngd`
|
||||||
|
|
||||||
|
- alias: `codex-task72-strict-startup-config`
|
||||||
|
- mission: `Execute TASK-72 strict startup config loading with actionable user-facing errors`
|
||||||
|
- status: `done`
|
||||||
|
- branch: `main`
|
||||||
|
- started_at: `2026-02-21T23:18:04Z`
|
||||||
|
- heartbeat_minutes: `5`
|
||||||
|
|
||||||
|
## Current Work (newest first)
|
||||||
|
|
||||||
|
- [2026-02-21T23:26:29Z] handoff: Completed TASK-72 end-to-end (no commit): strict startup constructor loading, shared parse-error formatter wiring, startup fail-fast guard in `main.ts`, docs update, focused tests pass, backlog task finalized to Done.
|
||||||
|
- [2026-02-21T23:26:29Z] test: `bun test src/config/config.test.ts src/main/config-validation.test.ts src/main/runtime/startup-config.test.ts` passed (46/46); full `bun run build` blocked by unrelated pre-existing TASK-75/TASK-77 TypeScript errors outside TASK-72 scope.
|
||||||
|
- [2026-02-21T23:24:20Z] progress: executed plan Tasks 1-2 via parallel subagents (`opencode-task72-strict-startup-config-*`, `opencode-task72-parse-details-*`), then integrated Task 3 (`src/main.ts`) and Task 4 (`docs/configuration.md`) in-session.
|
||||||
|
- [2026-02-21T23:19:40Z] progress: wrote execution plan at `docs/plans/2026-02-21-task-72-strict-startup-config-loading.md` via writing-plans skill.
|
||||||
|
- [2026-02-21T23:18:04Z] intent: Load TASK-72 context from Backlog MCP, produce execution plan with writing-plans skill, then implement/test/docs updates without commit.
|
||||||
|
|
||||||
|
## Files Touched
|
||||||
|
|
||||||
|
- `docs/subagents/INDEX.md`
|
||||||
|
- `docs/subagents/collaboration.md`
|
||||||
|
- `docs/subagents/agents/codex-task72-strict-startup-config-20260221T231804Z-3ngd.md`
|
||||||
|
- `docs/plans/2026-02-21-task-72-strict-startup-config-loading.md`
|
||||||
|
- `src/config/service.ts`
|
||||||
|
- `src/config/config.test.ts`
|
||||||
|
- `src/main/config-validation.ts`
|
||||||
|
- `src/main/config-validation.test.ts`
|
||||||
|
- `src/main/runtime/startup-config.ts`
|
||||||
|
- `src/main/runtime/startup-config.test.ts`
|
||||||
|
- `src/main.ts`
|
||||||
|
- `docs/configuration.md`
|
||||||
|
- `backlog/tasks/task-72 - Make-startup-config-loading-strict-with-clear-user-facing-errors.md`
|
||||||
|
|
||||||
|
## Assumptions
|
||||||
|
|
||||||
|
- Backlog MCP is available; `TASK-72` task file is source of truth for AC/DoD.
|
||||||
|
|
||||||
|
## Open Questions / Blockers
|
||||||
|
|
||||||
|
- None.
|
||||||
|
|
||||||
|
## Next Step
|
||||||
|
|
||||||
|
- Await user review or follow-up tasks.
|
||||||
@@ -3,7 +3,7 @@ import assert from 'node:assert/strict';
|
|||||||
import * as fs from 'fs';
|
import * as fs from 'fs';
|
||||||
import * as os from 'os';
|
import * as os from 'os';
|
||||||
import * as path from 'path';
|
import * as path from 'path';
|
||||||
import { ConfigService } from './service';
|
import { ConfigService, ConfigStartupParseError } from './service';
|
||||||
import { DEFAULT_CONFIG, RUNTIME_OPTION_REGISTRY } from './definitions';
|
import { DEFAULT_CONFIG, RUNTIME_OPTION_REGISTRY } from './definitions';
|
||||||
import { generateConfigTemplate } from './template';
|
import { generateConfigTemplate } from './template';
|
||||||
|
|
||||||
@@ -39,6 +39,24 @@ test('loads defaults when config is missing', () => {
|
|||||||
assert.equal(config.immersionTracking.retention.vacuumIntervalDays, 7);
|
assert.equal(config.immersionTracking.retention.vacuumIntervalDays, 7);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
test('throws actionable startup parse error for malformed config at construction time', () => {
|
||||||
|
const dir = makeTempDir();
|
||||||
|
const configPath = path.join(dir, 'config.jsonc');
|
||||||
|
fs.writeFileSync(configPath, '{"websocket":', 'utf-8');
|
||||||
|
|
||||||
|
assert.throws(
|
||||||
|
() => new ConfigService(dir),
|
||||||
|
(error: unknown) => {
|
||||||
|
assert.ok(error instanceof ConfigStartupParseError);
|
||||||
|
assert.equal(error.path, configPath);
|
||||||
|
assert.ok(error.parseError.length > 0);
|
||||||
|
assert.ok(error.message.includes(configPath));
|
||||||
|
assert.ok(error.message.includes(error.parseError));
|
||||||
|
return true;
|
||||||
|
},
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
test('parses subtitleStyle.preserveLineBreaks and warns on invalid values', () => {
|
test('parses subtitleStyle.preserveLineBreaks and warns on invalid values', () => {
|
||||||
const validDir = makeTempDir();
|
const validDir = makeTempDir();
|
||||||
fs.writeFileSync(
|
fs.writeFileSync(
|
||||||
|
|||||||
@@ -18,12 +18,26 @@ export type ReloadConfigStrictResult =
|
|||||||
path: string;
|
path: string;
|
||||||
};
|
};
|
||||||
|
|
||||||
|
export class ConfigStartupParseError extends Error {
|
||||||
|
readonly path: string;
|
||||||
|
readonly parseError: string;
|
||||||
|
|
||||||
|
constructor(configPath: string, parseError: string) {
|
||||||
|
super(
|
||||||
|
`Failed to parse startup config at ${configPath}: ${parseError}. Fix the config file and restart SubMiner.`,
|
||||||
|
);
|
||||||
|
this.name = 'ConfigStartupParseError';
|
||||||
|
this.path = configPath;
|
||||||
|
this.parseError = parseError;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
export class ConfigService {
|
export class ConfigService {
|
||||||
private readonly configPaths: ConfigPaths;
|
private readonly configPaths: ConfigPaths;
|
||||||
private rawConfig: RawConfig = {};
|
private rawConfig: RawConfig = {};
|
||||||
private resolvedConfig: ResolvedConfig = deepCloneConfig(DEFAULT_CONFIG);
|
private resolvedConfig: ResolvedConfig = deepCloneConfig(DEFAULT_CONFIG);
|
||||||
private warnings: ConfigValidationWarning[] = [];
|
private warnings: ConfigValidationWarning[] = [];
|
||||||
private configPathInUse: string;
|
private configPathInUse!: string;
|
||||||
|
|
||||||
constructor(configDir: string) {
|
constructor(configDir: string) {
|
||||||
this.configPaths = {
|
this.configPaths = {
|
||||||
@@ -31,8 +45,11 @@ export class ConfigService {
|
|||||||
configFileJsonc: path.join(configDir, 'config.jsonc'),
|
configFileJsonc: path.join(configDir, 'config.jsonc'),
|
||||||
configFileJson: path.join(configDir, 'config.json'),
|
configFileJson: path.join(configDir, 'config.json'),
|
||||||
};
|
};
|
||||||
this.configPathInUse = this.configPaths.configFileJsonc;
|
const loadResult = loadRawConfigStrict(this.configPaths);
|
||||||
this.reloadConfig();
|
if (!loadResult.ok) {
|
||||||
|
throw new ConfigStartupParseError(loadResult.path, loadResult.error);
|
||||||
|
}
|
||||||
|
this.applyResolvedConfig(loadResult.config, loadResult.path);
|
||||||
}
|
}
|
||||||
|
|
||||||
getConfigPath(): string {
|
getConfigPath(): string {
|
||||||
|
|||||||
@@ -1,6 +1,7 @@
|
|||||||
import test from 'node:test';
|
import test from 'node:test';
|
||||||
import assert from 'node:assert/strict';
|
import assert from 'node:assert/strict';
|
||||||
import {
|
import {
|
||||||
|
buildConfigParseErrorDetails,
|
||||||
buildConfigWarningNotificationBody,
|
buildConfigWarningNotificationBody,
|
||||||
buildConfigWarningSummary,
|
buildConfigWarningSummary,
|
||||||
failStartupFromConfig,
|
failStartupFromConfig,
|
||||||
@@ -52,6 +53,15 @@ test('buildConfigWarningNotificationBody includes concise warning details', () =
|
|||||||
);
|
);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
test('buildConfigParseErrorDetails includes path error and restart guidance', () => {
|
||||||
|
const details = buildConfigParseErrorDetails('/tmp/config.jsonc', 'unexpected token at line 1');
|
||||||
|
|
||||||
|
assert.match(details, /Failed to parse config file at:/);
|
||||||
|
assert.match(details, /\/tmp\/config\.jsonc/);
|
||||||
|
assert.match(details, /Error: unexpected token at line 1/);
|
||||||
|
assert.match(details, /Fix the config file and restart SubMiner\./);
|
||||||
|
});
|
||||||
|
|
||||||
test('failStartupFromConfig invokes handlers and throws', () => {
|
test('failStartupFromConfig invokes handlers and throws', () => {
|
||||||
const calls: string[] = [];
|
const calls: string[] = [];
|
||||||
const previousExitCode = process.exitCode;
|
const previousExitCode = process.exitCode;
|
||||||
|
|||||||
@@ -61,6 +61,17 @@ export function buildConfigWarningNotificationBody(
|
|||||||
].join('\n');
|
].join('\n');
|
||||||
}
|
}
|
||||||
|
|
||||||
|
export function buildConfigParseErrorDetails(configPath: string, parseError: string): string {
|
||||||
|
return [
|
||||||
|
'Failed to parse config file at:',
|
||||||
|
configPath,
|
||||||
|
'',
|
||||||
|
`Error: ${parseError}`,
|
||||||
|
'',
|
||||||
|
'Fix the config file and restart SubMiner.',
|
||||||
|
].join('\n');
|
||||||
|
}
|
||||||
|
|
||||||
export function failStartupFromConfig(
|
export function failStartupFromConfig(
|
||||||
title: string,
|
title: string,
|
||||||
details: string,
|
details: string,
|
||||||
|
|||||||
@@ -1,9 +1,6 @@
|
|||||||
import test from 'node:test';
|
import test from 'node:test';
|
||||||
import assert from 'node:assert/strict';
|
import assert from 'node:assert/strict';
|
||||||
import {
|
import { createCriticalConfigErrorHandler, createReloadConfigHandler } from './startup-config';
|
||||||
createCriticalConfigErrorHandler,
|
|
||||||
createReloadConfigHandler,
|
|
||||||
} from './startup-config';
|
|
||||||
|
|
||||||
test('createReloadConfigHandler runs success flow with warnings', async () => {
|
test('createReloadConfigHandler runs success flow with warnings', async () => {
|
||||||
const calls: string[] = [];
|
const calls: string[] = [];
|
||||||
@@ -42,9 +39,7 @@ test('createReloadConfigHandler runs success flow with warnings', async () => {
|
|||||||
assert.ok(calls.some((entry) => entry.startsWith('info:Using config file: /tmp/config.jsonc')));
|
assert.ok(calls.some((entry) => entry.startsWith('info:Using config file: /tmp/config.jsonc')));
|
||||||
assert.ok(calls.some((entry) => entry.startsWith('warn:[config] Validation found 1 issue(s)')));
|
assert.ok(calls.some((entry) => entry.startsWith('warn:[config] Validation found 1 issue(s)')));
|
||||||
assert.ok(
|
assert.ok(
|
||||||
calls.some((entry) =>
|
calls.some((entry) => entry.includes('notify:SubMiner:1 config validation issue(s) detected.')),
|
||||||
entry.includes('notify:SubMiner:1 config validation issue(s) detected.'),
|
|
||||||
),
|
|
||||||
);
|
);
|
||||||
assert.ok(calls.some((entry) => entry.includes('1. ankiConnect.pollingRate: must be >= 50')));
|
assert.ok(calls.some((entry) => entry.includes('1. ankiConnect.pollingRate: must be >= 50')));
|
||||||
assert.ok(calls.includes('hotReload:start'));
|
assert.ok(calls.includes('hotReload:start'));
|
||||||
@@ -79,6 +74,9 @@ test('createReloadConfigHandler fails startup for parse errors', () => {
|
|||||||
assert.throws(() => reloadConfig(), /Failed to parse config file at:/);
|
assert.throws(() => reloadConfig(), /Failed to parse config file at:/);
|
||||||
assert.equal(process.exitCode, 1);
|
assert.equal(process.exitCode, 1);
|
||||||
assert.ok(calls.some((entry) => entry.startsWith('error:Failed to parse config file at:')));
|
assert.ok(calls.some((entry) => entry.startsWith('error:Failed to parse config file at:')));
|
||||||
|
assert.ok(calls.some((entry) => entry.includes('/tmp/config.jsonc')));
|
||||||
|
assert.ok(calls.some((entry) => entry.includes('Error: unexpected token')));
|
||||||
|
assert.ok(calls.some((entry) => entry.includes('Fix the config file and restart SubMiner.')));
|
||||||
assert.ok(
|
assert.ok(
|
||||||
calls.some((entry) =>
|
calls.some((entry) =>
|
||||||
entry.startsWith('dialog:SubMiner config parse error:Failed to parse config file at:'),
|
entry.startsWith('dialog:SubMiner config parse error:Failed to parse config file at:'),
|
||||||
|
|||||||
@@ -1,5 +1,6 @@
|
|||||||
import type { ConfigValidationWarning } from '../../types';
|
import type { ConfigValidationWarning } from '../../types';
|
||||||
import {
|
import {
|
||||||
|
buildConfigParseErrorDetails,
|
||||||
buildConfigWarningNotificationBody,
|
buildConfigWarningNotificationBody,
|
||||||
buildConfigWarningSummary,
|
buildConfigWarningSummary,
|
||||||
failStartupFromConfig,
|
failStartupFromConfig,
|
||||||
@@ -48,7 +49,7 @@ export function createReloadConfigHandler(deps: ReloadConfigRuntimeDeps): () =>
|
|||||||
if (!result.ok) {
|
if (!result.ok) {
|
||||||
failStartupFromConfig(
|
failStartupFromConfig(
|
||||||
'SubMiner config parse error',
|
'SubMiner config parse error',
|
||||||
`Failed to parse config file at:\n${result.path}\n\nError: ${result.error}\n\nFix the config file and restart SubMiner.`,
|
buildConfigParseErrorDetails(result.path, result.error),
|
||||||
deps.failHandlers,
|
deps.failHandlers,
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user