morluto/rea

Fork 0

mirror of https://github.com/morluto/rea.git synced 2026-11-05 07:47:29 +00:00

[PR #2096] [MERGED] fix(setup): configure a comment-only Hermes config.yaml #5021

New issue

Closed

opened 2026-10-31 00:58:32 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 00:58:32 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/2096
Author: @JayOfTheKeyboard
Created: 10/12/2026
Status: ✅ Merged
Merged: 10/13/2026
Merged by: @morluto

Base: main ← Head: fix/hermes-comment-only-config

📝 Commits (10+)

6446606 fix(setup): configure a comment-only Hermes config.yaml
41c6de4 fix(setup): keep rejecting an explicit null Hermes root
0fba154 fix(setup): reject malformed or explicitly tagged empty YAML
ede7dfe fix: reconcile final main for PR #2096
67ec447 test(setup): split the Hermes empty-configuration tests under the function length limit
5c9ffd8 fix(setup): type the Hermes edit document as a plain Document
e56b627 chore: merge captured shared CI maintenance fixes
93e9175 fix(ci): isolate Linux Hopper process fixture inventory
e11d03c Merge commit '8fef2aa64e' into HEAD
e205636 Merge branch 'main' into fix/hermes-comment-only-config

📊 Changes

2 files changed (+94 additions, -18 deletions)

View changed files

📝 src/application/HermesConfigurationYaml.ts (+33 -5)
📝 tests/boundary/filesystem/hermesClientConfiguration.test.ts (+61 -13)

📄 Description

Summary

rea setup --client hermes still fails when ~/.hermes/config.yaml holds only comments. #1983 fixed the empty file, and its PR body left the comment-only file out because the serializer would need separate handling. This finishes that case. No issue is linked, because the gap is the one #1983 already describes.

Problem and expected behavior

A file such as # Hermes configuration\n is not whitespace, so it skips the empty-file rule #1983 added. YAML then parses it to null, the same value an empty file gives, and the Hermes branch of parseDocument rejects it:

$ printf '# Hermes configuration\n' > "$HOME/.hermes/config.yaml"
$ rea setup --client hermes --yes --format json
"status": "needs_human"
"remediation": "Hermes: Configuration …/.hermes/config.yaml is malformed: … \"Invalid input: expected record, received null\""

A file holding only a --- marker fails the same way, and fixing the parse is not enough for it. yaml 2.9.0 gives a bare --- document a null scalar as its root, where a comment-only file has empty contents. serializeHermesConfiguration cannot set a key path on a scalar ("Expected a YAML collection as document contents").

Expected: both files are configured like an empty one, and their comments stay. DeepSeek Harness already does this for a comment-only cordis.patch.yml (appends a registration to a comment-only patch file in dshClientConfiguration.test.ts).

Change and scope

parseDocument reads a Hermes document with no settings as {}: comments only, or a bare ---. An explicit null or ~ root is written out, so it is still rejected like any other root that is not a map (changed after review).
serializeHermesConfiguration replaces a null-scalar root with an empty map before it adds the entry. The comments the scalar carried move to the new map, so ---\n# Hermes configuration\n keeps its comment.
A comment-only file needed no serializer change. Its root is empty and setIn already builds the map, keeping the comment above it.

Not changed, and yours to decide:

DeepSeek Harness accepts a comment-only cordis.patch.yml but rejects a bare --- one with "Cordis patches must be a YAML operation list". It is the same null-scalar root, in a separate reader (DshCordisYaml). I left it alone to keep this to Hermes.
JSON and JSONC clients reject a file that holds only a comment, for example // cursor\n in ~/.cursor/mcp.json, with "Invalid JSON/JSONC at offset 10: ValueExpected". That comes from the JSON parser, so this fix does not reach it, and whether each client should accept such a file is a separate question.
Uninstall leaves mcp_servers: {} behind, after the kept comments. An empty file does the same today after #1983.

Contract and boundary impact

Semantic owner and earliest changed stage: Hermes document parsing in parseDocument and its serializer, serializeHermesConfiguration (src/application/ClientConfigurationDocument.ts).
CLI and MCP/tool-catalog contract: none.
Provider, bridge, target-format, or platform compatibility: Hermes client setup on every platform.
Evidence, artifact, provenance, or reconstruction contract: none.
Process execution, authorization, cleanup, or containment impact: none. Backups and readback are unchanged.
Generated metadata (docs/public/product-catalog.json), package, or installation impact: none.

Evidence and regression coverage

Tests: #1983's it.each in tests/boundary/filesystem/hermesClientConfiguration.test.ts gets 3 more inputs: "# Hermes configuration\n", "---\n" and "---\n# Hermes configuration\n". Each case now also checks that the written file starts with the original text. The shape-diagnostics it.each gets null, ~, a comment then null, and --- ~, which must still be rejected before any change. All 4 fail on the first commit and pass on the second.
Base reproduction: on the parent commit all 3 new cases fail with expected 'failed' to be 'configured'. The 2 existing cases still pass.
Each part of the fix is needed. Without the empty-document check in parseDocument, all 3 fail. Without the root replacement, the 2 --- cases fail. Without carrying the comments over, "---\n# Hermes configuration\n" fails on the kept text.
User-visible CLI output: the built CLI, rea setup --client hermes --yes against a scratch HOME, before and after.

`config.yaml` before	Before	After
empty	`ready`, configured	`ready`, configured
`# Hermes configuration`	`needs_human`, "expected record, received null"	`ready`, comment kept
`---`	`needs_human`, "expected record, received null"	`ready`, marker kept
`---` then `# Hermes configuration`	`needs_human`, "expected record, received null"	`ready`, marker and comment kept

With the fix, a second rea setup plans no actions and leaves the file byte-identical, and doctor reads the registration as aligned. rea uninstall then reports removed and keeps the comments and the marker.

For evidence-bearing changes:

Observed, derived, and inferred claims remain distinguishable.
Artifact identity, source provenance, and failed attempts remain preserved.
Unsupported, incomplete, unavailable, or uncertain outcomes remain visible.

Validation performed

npm run test:focused over the 8 files that cover client configuration setup, including the Hermes and DeepSeek Harness acceptance and boundary tests: 227 tests passed.
npm run check:fast: passed.
npm run verify:test-discovery: "Discovered all 915 test files exactly once."

Compatibility, safety, and release

Breaking changes or migration steps: none.
Real Hopper/Ghidra, browser, or OS coverage: Linux x64 (Fedora 44) only. No real Hermes client was run.
Package or release metadata impact: none.
Security, privacy, process, or containment review: none. Only a YAML document with no root, or a bare ---, is newly accepted, as a document with no settings. An explicit null root is rejected as before.

Review checklist

This PR addresses a concrete problem or an agreed enhancement.
The PR has one focused outcome and the title follows type(scope): outcome.
Related issue is linked, or the reason for not linking one is stated above.
Tests cover changed observable behavior and meaningful failure paths.
Owning docs, contracts, and generated metadata are updated where needed.
User-visible CLI/MCP changes include representative output.
I checked the final diff for secrets, unrelated cleanup, and unsupported claims.

_{🔄 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/2096 **Author:** [@JayOfTheKeyboard](https://github.com/JayOfTheKeyboard) **Created:** 10/12/2026 **Status:** ✅ Merged **Merged:** 10/13/2026 **Merged by:** [@morluto](https://github.com/morluto) **Base:** `main` ← **Head:** `fix/hermes-comment-only-config` --- ### 📝 Commits (10+) - [`6446606`](https://github.com/morluto/rea/commit/6446606a8668c62a7b5d5f09b9713851de505ffb) fix(setup): configure a comment-only Hermes config.yaml - [`41c6de4`](https://github.com/morluto/rea/commit/41c6de4b785b5fc60888692c5f44a60eda729b74) fix(setup): keep rejecting an explicit null Hermes root - [`0fba154`](https://github.com/morluto/rea/commit/0fba154cdbaf5d104201925e9ccb9c36fa9c9f4a) fix(setup): reject malformed or explicitly tagged empty YAML - [`ede7dfe`](https://github.com/morluto/rea/commit/ede7dfe804fde2b5b07639f4a33975876c960cfa) fix: reconcile final main for PR #2096 - [`67ec447`](https://github.com/morluto/rea/commit/67ec4472a4ad8fb93c07df1e216d31cd13a4d77f) test(setup): split the Hermes empty-configuration tests under the function length limit - [`5c9ffd8`](https://github.com/morluto/rea/commit/5c9ffd8f3372826128467ec5f9c351701a95d03a) fix(setup): type the Hermes edit document as a plain Document - [`e56b627`](https://github.com/morluto/rea/commit/e56b62756ddddc73bf0e17e86b3ce57d54c486c3) chore: merge captured shared CI maintenance fixes - [`93e9175`](https://github.com/morluto/rea/commit/93e9175e5a4780e6881a3cef16aa6000a60028f4) fix(ci): isolate Linux Hopper process fixture inventory - [`e11d03c`](https://github.com/morluto/rea/commit/e11d03c4c7c69aa8c405b44c6cacf3abe188861b) Merge commit '8fef2aa64ec1ee742e8b70b825938d30a0ea7319' into HEAD - [`e205636`](https://github.com/morluto/rea/commit/e205636171b69dcd4ae2d4864636de146839b3a9) Merge branch 'main' into fix/hermes-comment-only-config ### 📊 Changes **2 files changed** (+94 additions, -18 deletions) <details> <summary>View changed files</summary> 📝 `src/application/HermesConfigurationYaml.ts` (+33 -5) 📝 `tests/boundary/filesystem/hermesClientConfiguration.test.ts` (+61 -13) </details> ### 📄 Description ## Summary `rea setup --client hermes` still fails when `~/.hermes/config.yaml` holds only comments. #1983 fixed the empty file, and its PR body left the comment-only file out because the serializer would need separate handling. This finishes that case. No issue is linked, because the gap is the one #1983 already describes. ## Problem and expected behavior A file such as `# Hermes configuration\n` is not whitespace, so it skips the empty-file rule #1983 added. YAML then parses it to `null`, the same value an empty file gives, and the Hermes branch of `parseDocument` rejects it: ```text $ printf '# Hermes configuration\n' > "$HOME/.hermes/config.yaml" $ rea setup --client hermes --yes --format json "status": "needs_human" "remediation": "Hermes: Configuration …/.hermes/config.yaml is malformed: … \"Invalid input: expected record, received null\"" ``` A file holding only a `---` marker fails the same way, and fixing the parse is not enough for it. yaml 2.9.0 gives a bare `---` document a null scalar as its root, where a comment-only file has empty contents. `serializeHermesConfiguration` cannot set a key path on a scalar ("Expected a YAML collection as document contents"). Expected: both files are configured like an empty one, and their comments stay. DeepSeek Harness already does this for a comment-only `cordis.patch.yml` (`appends a registration to a comment-only patch file` in `dshClientConfiguration.test.ts`). ## Change and scope - `parseDocument` reads a Hermes document with no settings as `{}`: comments only, or a bare `---`. An explicit `null` or `~` root is written out, so it is still rejected like any other root that is not a map (changed after review). - `serializeHermesConfiguration` replaces a null-scalar root with an empty map before it adds the entry. The comments the scalar carried move to the new map, so `---\n# Hermes configuration\n` keeps its comment. - A comment-only file needed no serializer change. Its root is empty and `setIn` already builds the map, keeping the comment above it. Not changed, and yours to decide: - DeepSeek Harness accepts a comment-only `cordis.patch.yml` but rejects a bare `---` one with "Cordis patches must be a YAML operation list". It is the same null-scalar root, in a separate reader (`DshCordisYaml`). I left it alone to keep this to Hermes. - JSON and JSONC clients reject a file that holds only a comment, for example `// cursor\n` in `~/.cursor/mcp.json`, with "Invalid JSON/JSONC at offset 10: ValueExpected". That comes from the JSON parser, so this fix does not reach it, and whether each client should accept such a file is a separate question. - Uninstall leaves `mcp_servers: {}` behind, after the kept comments. An empty file does the same today after #1983. ## Contract and boundary impact - Semantic owner and earliest changed stage: Hermes document parsing in `parseDocument` and its serializer, `serializeHermesConfiguration` (`src/application/ClientConfigurationDocument.ts`). - CLI and MCP/tool-catalog contract: none. - Provider, bridge, target-format, or platform compatibility: Hermes client setup on every platform. - Evidence, artifact, provenance, or reconstruction contract: none. - Process execution, authorization, cleanup, or containment impact: none. Backups and readback are unchanged. - Generated metadata (`docs/public/product-catalog.json`), package, or installation impact: none. ## Evidence and regression coverage - Tests: #1983's `it.each` in `tests/boundary/filesystem/hermesClientConfiguration.test.ts` gets 3 more inputs: `"# Hermes configuration\n"`, `"---\n"` and `"---\n# Hermes configuration\n"`. Each case now also checks that the written file starts with the original text. The shape-diagnostics `it.each` gets `null`, `~`, a comment then `null`, and `--- ~`, which must still be rejected before any change. All 4 fail on the first commit and pass on the second. - Base reproduction: on the parent commit all 3 new cases fail with `expected 'failed' to be 'configured'`. The 2 existing cases still pass. - Each part of the fix is needed. Without the empty-document check in `parseDocument`, all 3 fail. Without the root replacement, the 2 `---` cases fail. Without carrying the comments over, `"---\n# Hermes configuration\n"` fails on the kept text. - User-visible CLI output: the built CLI, `rea setup --client hermes --yes` against a scratch `HOME`, before and after. | `config.yaml` before | Before | After | | --- | --- | --- | | empty | `ready`, configured | `ready`, configured | | `# Hermes configuration` | `needs_human`, "expected record, received null" | `ready`, comment kept | | `---` | `needs_human`, "expected record, received null" | `ready`, marker kept | | `---` then `# Hermes configuration` | `needs_human`, "expected record, received null" | `ready`, marker and comment kept | With the fix, a second `rea setup` plans no actions and leaves the file byte-identical, and `doctor` reads the registration as `aligned`. `rea uninstall` then reports `removed` and keeps the comments and the marker. For evidence-bearing changes: - [x] Observed, derived, and inferred claims remain distinguishable. - [x] Artifact identity, source provenance, and failed attempts remain preserved. - [x] Unsupported, incomplete, unavailable, or uncertain outcomes remain visible. ## Validation performed - `npm run test:focused` over the 8 files that cover client configuration setup, including the Hermes and DeepSeek Harness acceptance and boundary tests: 227 tests passed. - `npm run check:fast`: passed. - `npm run verify:test-discovery`: "Discovered all 915 test files exactly once." ## Compatibility, safety, and release - Breaking changes or migration steps: none. - Real Hopper/Ghidra, browser, or OS coverage: Linux x64 (Fedora 44) only. No real Hermes client was run. - Package or release metadata impact: none. - Security, privacy, process, or containment review: none. Only a YAML document with no root, or a bare `---`, is newly accepted, as a document with no settings. An explicit null root is rejected as before. ## Review checklist - [x] This PR addresses a concrete problem or an agreed enhancement. - [x] The PR has one focused outcome and the title follows `type(scope): outcome`. - [x] Related issue is linked, or the reason for not linking one is stated above. - [x] Tests cover changed observable behavior and meaningful failure paths. - [x] Owning docs, contracts, and generated metadata are updated where needed. - [x] User-visible CLI/MCP changes include representative output. - [x] I checked the final diff for secrets, unrelated cleanup, and unsupported claims. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-31 00:58:32 +00:00

closed this issue
added the
pull-request
label

bsakaguchi referenced this issue

2026-11-01 21:42:41 +00:00

[PR #5021] [MERGED] fix(tests): restore Android prerequisite verification #6739

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

morluto/rea#5021

No description provided.

Rows
Columns