[PR #2153] [CLOSED] fix(setup): name invalid client configuration fields #5062

New issue

Closed

opened 2026-10-31 01:01:57 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 01:01:57 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/2153
Author: @Jax367
Created: 10/12/2026
Status: ❌ Closed

Base: main ← Head: codex/client-config-shape-diagnostics

📝 Commits (1)

fc47bcc fix(setup): name invalid client configuration fields

📊 Changes

3 files changed (+169 additions, -11 deletions)

View changed files

📝 src/application/ClientConfigurationDocument.ts (+29 -9)
📝 src/application/Uninstall.ts (+4 -2)
➕ tests/acceptance/setup/clientConfigurationDiagnosticsCli.test.ts (+136 -0)

📄 Description

Summary

Fixes #2152. When a client configuration contains a non-object root or MCP server table, setup and uninstall now name the rejected field instead of printing a Zod issue dump or claiming that valid JSON could not be parsed. Both commands continue stopping before configuration writes or removals.

Problem and expected behavior

For {"mcpServers":null,"keep":1}, setup previously displayed an invalid_type issue with an empty path, and uninstall said the document was not valid JSON. The shared parser now reports Configuration field "mcpServers" must be an object. A root such as [] reports The configuration root must be an object. OpenCode's nested table reports "mcp.servers".

Change and scope

Validate decoded roots and server tables once at the shared parsing boundary, using the existing tagged ConfigurationError with a named setting and the original validation cause. Setup's existing file-reader Result preserves that message; uninstall now preserves structural configuration errors. Syntax failures retain their format-specific diagnostics, including YAML for Hermes/DSH.

Contract and boundary impact

Semantic owner and earliest changed stage: ClientConfigurationDocument admission of decoded client settings.
CLI and MCP/tool-catalog contract: clearer setup/uninstall diagnostics; existing statuses, exit codes and JSON result shapes retained. No MCP tool or schema change.
Provider, bridge, target-format, or platform compatibility: no provider changes; existing JSON/JSONC, strict Pi JSON, TOML, Hermes YAML and OpenCode table admission preserved.
Evidence, artifact, provenance, or reconstruction contract: none.
Process execution, authorization, cleanup, or containment impact: existing preflight refusal remains before mutation; no additional authority or process execution.
Generated metadata, package, or installation impact: diagnostic wording only; no package/dependency/version changes or generated outputs committed.

Evidence and regression coverage

Added ten compiled CLI scenarios covering non-object roots, null/string/array tables, OpenCode's nested table, TOML, YAML and genuinely invalid JSON. Each invokes setup and uninstall and verifies the original bytes and absence of backups or sibling files.
On base 60259a91d, the initial seven structural scenarios failed their diagnostic assertions; the syntax-error control passed. All ten final scenarios pass with this fix.
Representative uninstall detail: Configuration <selected path> is malformed: Configuration field "mcpServers" must be an object. It was not changed. Repair it, then rerun uninstall.
Existing successful configuration, special DSH document handling and setup refusal flows are exercised by the regression runs below.

Validation performed

Native Windows, pinned Node 24.18.0 / npm 11.16.0:

npm run test:focused -- tests/acceptance/setup/clientConfigurationDiagnosticsCli.test.ts tests/boundary/filesystem/dshClientConfiguration.test.ts src/application/Setup.configuration.test.ts — 103 passed, including all ten real CLI journeys.
Expanded run also included clientLifecycle.test.ts, hermesClientConfiguration.test.ts, and piClientConfiguration.test.ts — 189 passed, 14 failed. The same 14 test names fail on unmodified base code: five Windows symlink EPERM failures, six path spelling/drive assumptions, and three existing Pi alignment failures. No failures were added by this change; these remain outside this PR's scope.
npm run check — all six tasks and architecture guards passed (existing lint warnings).
npm run verify:test-discovery — all 916 test files discovered exactly once.
npm pack --dry-run --json — passed, including lifecycle checks.
npm run verify:package — attempted; its repository-skill stage failed at spawn npm ENOENT on this Windows host before installation/verification. Temporary workspace cleanup completed. Full packaged-install verification remains unverified.
git diff --check — passed.

Compatibility, safety, and release

Breaking changes or migration steps: none; invalid configurations remain invalid.
Real Hopper/Ghidra, browser, or OS coverage: real compiled setup/uninstall on native Windows; no native analysis engine needed or exercised. Other hosts and the full package lane remain with CI.
Package or release metadata impact: none.
Security, privacy, process, or containment review: messages name only the selected configuration path and fixed field names; rejected setting values are not interpolated. Existing preflight and preservation behavior remains intact.

Review checklist

This PR addresses a concrete problem with one focused outcome and a conventional title.
Related issue is linked.
Tests cover changed observable behavior and failure paths through the compiled CLI.
Owning docs, contracts and generated metadata were assessed; no schema/documentation projection changes are needed.
Representative CLI output and unavailable verification are documented.
Final diff checked for secrets, unrelated cleanup and generated outputs.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/morluto/rea/pull/2153 **Author:** [@Jax367](https://github.com/Jax367) **Created:** 10/12/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `codex/client-config-shape-diagnostics` --- ### 📝 Commits (1) - [`fc47bcc`](https://github.com/morluto/rea/commit/fc47bcc32f55a65d00a8c6f7615e5c7a5ca49032) fix(setup): name invalid client configuration fields ### 📊 Changes **3 files changed** (+169 additions, -11 deletions) <details> <summary>View changed files</summary> 📝 `src/application/ClientConfigurationDocument.ts` (+29 -9) 📝 `src/application/Uninstall.ts` (+4 -2) ➕ `tests/acceptance/setup/clientConfigurationDiagnosticsCli.test.ts` (+136 -0) </details> ### 📄 Description ## Summary Fixes #2152. When a client configuration contains a non-object root or MCP server table, setup and uninstall now name the rejected field instead of printing a Zod issue dump or claiming that valid JSON could not be parsed. Both commands continue stopping before configuration writes or removals. ## Problem and expected behavior For `{"mcpServers":null,"keep":1}`, setup previously displayed an `invalid_type` issue with an empty path, and uninstall said the document was not valid JSON. The shared parser now reports `Configuration field "mcpServers" must be an object.` A root such as `[]` reports `The configuration root must be an object.` OpenCode's nested table reports `"mcp.servers"`. ## Change and scope Validate decoded roots and server tables once at the shared parsing boundary, using the existing tagged `ConfigurationError` with a named setting and the original validation cause. Setup's existing file-reader `Result` preserves that message; uninstall now preserves structural configuration errors. Syntax failures retain their format-specific diagnostics, including YAML for Hermes/DSH. ## Contract and boundary impact - Semantic owner and earliest changed stage: `ClientConfigurationDocument` admission of decoded client settings. - CLI and MCP/tool-catalog contract: clearer setup/uninstall diagnostics; existing statuses, exit codes and JSON result shapes retained. No MCP tool or schema change. - Provider, bridge, target-format, or platform compatibility: no provider changes; existing JSON/JSONC, strict Pi JSON, TOML, Hermes YAML and OpenCode table admission preserved. - Evidence, artifact, provenance, or reconstruction contract: none. - Process execution, authorization, cleanup, or containment impact: existing preflight refusal remains before mutation; no additional authority or process execution. - Generated metadata, package, or installation impact: diagnostic wording only; no package/dependency/version changes or generated outputs committed. ## Evidence and regression coverage - Added ten compiled CLI scenarios covering non-object roots, null/string/array tables, OpenCode's nested table, TOML, YAML and genuinely invalid JSON. Each invokes setup and uninstall and verifies the original bytes and absence of backups or sibling files. - On base `60259a91d`, the initial seven structural scenarios failed their diagnostic assertions; the syntax-error control passed. All ten final scenarios pass with this fix. - Representative uninstall detail: `Configuration <selected path> is malformed: Configuration field "mcpServers" must be an object. It was not changed. Repair it, then rerun uninstall.` - Existing successful configuration, special DSH document handling and setup refusal flows are exercised by the regression runs below. ## Validation performed Native Windows, pinned Node 24.18.0 / npm 11.16.0: - `npm run test:focused -- tests/acceptance/setup/clientConfigurationDiagnosticsCli.test.ts tests/boundary/filesystem/dshClientConfiguration.test.ts src/application/Setup.configuration.test.ts` — **103 passed**, including all ten real CLI journeys. - Expanded run also included `clientLifecycle.test.ts`, `hermesClientConfiguration.test.ts`, and `piClientConfiguration.test.ts` — **189 passed, 14 failed**. The same 14 test names fail on unmodified base code: five Windows symlink `EPERM` failures, six path spelling/drive assumptions, and three existing Pi alignment failures. No failures were added by this change; these remain outside this PR's scope. - `npm run check` — all six tasks and architecture guards passed (existing lint warnings). - `npm run verify:test-discovery` — all 916 test files discovered exactly once. - `npm pack --dry-run --json` — passed, including lifecycle checks. - `npm run verify:package` — attempted; its repository-skill stage failed at `spawn npm ENOENT` on this Windows host before installation/verification. Temporary workspace cleanup completed. Full packaged-install verification remains unverified. - `git diff --check` — passed. ## Compatibility, safety, and release - Breaking changes or migration steps: none; invalid configurations remain invalid. - Real Hopper/Ghidra, browser, or OS coverage: real compiled setup/uninstall on native Windows; no native analysis engine needed or exercised. Other hosts and the full package lane remain with CI. - Package or release metadata impact: none. - Security, privacy, process, or containment review: messages name only the selected configuration path and fixed field names; rejected setting values are not interpolated. Existing preflight and preservation behavior remains intact. ## Review checklist - [x] This PR addresses a concrete problem with one focused outcome and a conventional title. - [x] Related issue is linked. - [x] Tests cover changed observable behavior and failure paths through the compiled CLI. - [x] Owning docs, contracts and generated metadata were assessed; no schema/documentation projection changes are needed. - [x] Representative CLI output and unavailable verification are documented. - [x] Final diff checked for secrets, unrelated cleanup and generated outputs. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-31 01:01:57 +00:00