morluto/rea

Fork 0

mirror of https://github.com/morluto/rea.git synced 2026-11-05 06:45:22 +00:00

[PR #3568] [MERGED] fix(setup): retain Hermes YAML scalar source during updates #5832

New issue

Closed

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

bsakaguchi commented

2026-10-31 01:09:46 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/3568
Author: @N0zoM1z0
Created: 10/14/2026
Status: ✅ Merged
Merged: 10/14/2026
Merged by: @N0zoM1z0

Base: main ← Head: review/hermes-yaml-3102-20261014

📝 Commits (2)

6140ce0 fix(setup): retain Hermes YAML scalar source during updates
c1e19e9 test(setup): lock Hermes scalar tags and sequence quoting

📊 Changes

3 files changed (+176 additions, -70 deletions)

View changed files

📝 src/application/HermesConfigurationYaml.test.ts (+28 -0)
📝 src/application/HermesConfigurationYaml.ts (+54 -2)
📝 tests/acceptance/setup/hermesClientSetup.test.ts (+94 -68)

📄 Description

Summary

Updating a Hermes registration could discard quoting on unchanged YAML 1.2 string keys such as "ON" and "YES". The actual Hermes C loader then saw duplicate boolean keys and switched to its pure fallback. Unchanged notifications: on consequently changed from boolean True to string "on", despite its source bytes remaining identical.

Follow-up to #3102 and main's independent fix 66536d0d2e. This addresses a separately reproduced representation loss, not the already-fixed default YAML 1.1 off case.

Change and scope

Keep source nodes for unchanged scalars, mapping keys and sequence elements at HermesConfigurationYaml, the existing format owner.
Match string keys with a linear lookup when replacing mappings; avoid a second interpretation in consumers.
Quote newly created YAML 1.2 strings for YAML 1.1 compatibility. Preserve the library's existing formatting for direct scalar edits, including octal numbers.
Extend the public CLI regression to default 1.1 and explicit 1.2 with both quoted and plain string keys, covering both existing loader-selection directions.

No new CLI/MCP, Evidence, package or target-execution contract. This is an ordinary configuration fix within existing scope; no additional capability decision is applicable. Writes retain the existing setup approval, backup and idempotence boundaries.

Executed evidence

Linux; Node 24.18.0 / npm 11.16.0; real Hermes loader with ruamel.yaml 0.18.16 / C extension 0.2.15. Upstream's current main had the identical loader blob when checked.

Before: real public CLI + actual safe_load observed True → "on". The new CLI regression failed against main's compiled runtime with duplicate YAML 1.1 keys. The initial quote-only experiment also failed the reverse direction; retaining source nodes fixes that producing boundary.
After: 50 related tests across three files passed; the final producer suite passed all eight tests, including two additional goldens for sequence quoting and explicit key/value tags. Those exact goldens failed on unchanged main (2 failed / 6 passed). npm run check passed all six tasks and architecture guards. After rebasing onto fresh main 569ef4159, changed production/test files remained byte-identical and npm run check:fast passed; it also passed again after the final test-only commit.
15 actual public CLI calls: three valid configurations each passed dry-run/apply/doctor/repeat. The Python consumer preserved all 12 string environment values, exact large integer 9876543210987654321, timestamp precision/offset, original backup and repeated-setup bytes. Three non-string env cases returned needs_human/exit 1 without changing the configuration or creating a backup.
Verification used owned temporary homes and inspected planned targets before writes; personal configurations were not changed. No running Hermes agent, model request, or native Windows/macOS client session was verified. Permanent CI coverage models the observed C-reader schema; actual Python-loader evidence was run independently.

Performance and limitations

Mapping reconciliation is linear in the replaced mapping and traverses existing YAML nodes. No new tool fields, analysis payload or cache are introduced. Agent tokens were not measured. Single fresh-process CLI observations: first plan 2.15 s; repeated setup 1.88 s; JSON output 5.3–10.5 KB. The equivalent completed default-1.1 four-call workflow took 8.39 s on main and 8.57 s with the fix; all four stdout byte counts were unchanged. These single trials are observations rather than a speedup claim.

Serial two-CPU / 8 GiB validation: related suites peaked at 3.11 GiB, static checks at 4.33 GiB; 15-call CLI probe took 33.90 s, user/system CPU 33.99/7.32 s and sampled peak family RSS 415 MiB. No guard stop or OOM.

Existing upstream limitation: removing a configuration block that made the C loader reject can still switch Hermes's reader and alter interpretation of other ambiguous YAML 1.2 scalars. A production-serializer removal probe with the actual Python loader reproduced this on both unchanged main and this patch; the public uninstall CLI was not exercised for that explicit-1.2 case. This PR prevents source loss during registration replacement; it does not establish universal equivalence between Hermes's C and pure readers.

Author readiness

Human owner: @N0zoM1z0

Recorded from the owner's explicit standing declarations for the remaining approved bug-fix batch. Technical review and the executed evidence above were performed by the agent; ownership remains with the named human.

I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work.
I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps.
I declare this PR ready for initial maintainer review.

_{🔄 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/3568 **Author:** [@N0zoM1z0](https://github.com/N0zoM1z0) **Created:** 10/14/2026 **Status:** ✅ Merged **Merged:** 10/14/2026 **Merged by:** [@N0zoM1z0](https://github.com/N0zoM1z0) **Base:** `main` ← **Head:** `review/hermes-yaml-3102-20261014` --- ### 📝 Commits (2) - [`6140ce0`](https://github.com/morluto/rea/commit/6140ce06cd3fad6f2893b70feaa50d823a105514) fix(setup): retain Hermes YAML scalar source during updates - [`c1e19e9`](https://github.com/morluto/rea/commit/c1e19e9db52d920c7557d441ad36e44f3b9f4106) test(setup): lock Hermes scalar tags and sequence quoting ### 📊 Changes **3 files changed** (+176 additions, -70 deletions) <details> <summary>View changed files</summary> 📝 `src/application/HermesConfigurationYaml.test.ts` (+28 -0) 📝 `src/application/HermesConfigurationYaml.ts` (+54 -2) 📝 `tests/acceptance/setup/hermesClientSetup.test.ts` (+94 -68) </details> ### 📄 Description ## Summary Updating a Hermes registration could discard quoting on unchanged YAML 1.2 string keys such as `"ON"` and `"YES"`. The actual Hermes C loader then saw duplicate boolean keys and switched to its pure fallback. Unchanged `notifications: on` consequently changed from boolean `True` to string `"on"`, despite its source bytes remaining identical. Follow-up to #3102 and main's independent fix 66536d0d2e183418caca09cf80c01cea9e029c9b. This addresses a separately reproduced representation loss, not the already-fixed default YAML 1.1 `off` case. ## Change and scope - Keep source nodes for unchanged scalars, mapping keys and sequence elements at `HermesConfigurationYaml`, the existing format owner. - Match string keys with a linear lookup when replacing mappings; avoid a second interpretation in consumers. - Quote newly created YAML 1.2 strings for YAML 1.1 compatibility. Preserve the library's existing formatting for direct scalar edits, including octal numbers. - Extend the public CLI regression to default 1.1 and explicit 1.2 with both quoted and plain string keys, covering both existing loader-selection directions. No new CLI/MCP, Evidence, package or target-execution contract. This is an ordinary configuration fix within existing scope; no additional capability decision is applicable. Writes retain the existing setup approval, backup and idempotence boundaries. ## Executed evidence Linux; Node 24.18.0 / npm 11.16.0; real [Hermes loader](https://github.com/NousResearch/hermes-agent/blob/d97007c32d6340f7ba79ac1dfb5773cc00846448/hermes_yaml.py) with ruamel.yaml 0.18.16 / C extension 0.2.15. Upstream's current main had the identical loader blob when checked. - **Before:** real public CLI + actual `safe_load` observed `True` → `"on"`. The new CLI regression failed against main's compiled runtime with duplicate YAML 1.1 keys. The initial quote-only experiment also failed the reverse direction; retaining source nodes fixes that producing boundary. - **After:** 50 related tests across three files passed; the final producer suite passed all eight tests, including two additional goldens for sequence quoting and explicit key/value tags. Those exact goldens failed on unchanged main (2 failed / 6 passed). `npm run check` passed all six tasks and architecture guards. After rebasing onto fresh main 569ef4159, changed production/test files remained byte-identical and `npm run check:fast` passed; it also passed again after the final test-only commit. - **15 actual public CLI calls:** three valid configurations each passed dry-run/apply/doctor/repeat. The Python consumer preserved all 12 string environment values, exact large integer `9876543210987654321`, timestamp precision/offset, original backup and repeated-setup bytes. Three non-string env cases returned `needs_human`/exit 1 without changing the configuration or creating a backup. - Verification used owned temporary homes and inspected planned targets before writes; personal configurations were not changed. No running Hermes agent, model request, or native Windows/macOS client session was verified. Permanent CI coverage models the observed C-reader schema; actual Python-loader evidence was run independently. ## Performance and limitations Mapping reconciliation is linear in the replaced mapping and traverses existing YAML nodes. No new tool fields, analysis payload or cache are introduced. Agent tokens were not measured. Single fresh-process CLI observations: first plan 2.15 s; repeated setup 1.88 s; JSON output 5.3–10.5 KB. The equivalent completed default-1.1 four-call workflow took 8.39 s on main and 8.57 s with the fix; all four stdout byte counts were unchanged. These single trials are observations rather than a speedup claim. Serial two-CPU / 8 GiB validation: related suites peaked at 3.11 GiB, static checks at 4.33 GiB; 15-call CLI probe took 33.90 s, user/system CPU 33.99/7.32 s and sampled peak family RSS 415 MiB. No guard stop or OOM. **Existing upstream limitation:** removing a configuration block that made the C loader reject can still switch Hermes's reader and alter interpretation of other ambiguous YAML 1.2 scalars. A production-serializer removal probe with the actual Python loader reproduced this on both unchanged main and this patch; the public uninstall CLI was not exercised for that explicit-1.2 case. This PR prevents source loss during registration replacement; it does not establish universal equivalence between Hermes's C and pure readers. ## Author readiness Human owner: @N0zoM1z0 Recorded from the owner's explicit standing declarations for the remaining approved bug-fix batch. Technical review and the executed evidence above were performed by the agent; ownership remains with the named human. - [x] I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work. - [x] I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps. - [x] I declare this PR ready for initial maintainer review. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-31 01:09:46 +00:00

closed this issue
added the
pull-request
label

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#5832

No description provided.

Rows
Columns