[PR #6261] [MERGED] fix(setup): keep comments around REA's table in Grok Build TOML #7235

New issue

Closed

opened 2026-11-01 21:46:23 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-11-01 21:46:23 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/6261
Author: @kaoru0822-kitauji
Created: 10/22/2026
Status: ✅ Merged
Merged: 10/22/2026
Merged by: @kaoru0822-kitauji

Base: main ← Head: fix/6207-grok-setup-comments

📝 Commits (1)

69581e6 fix(setup): keep comments around REA's table in Grok Build TOML

📊 Changes

3 files changed (+193 additions, -13 deletions)

View changed files

📝 src/application/ClientConfigurationDocument.ts (+12 -11)
➕ tests/acceptance/setup/tomlCommentPreservationCli.test.ts (+162 -0)
📝 tests/boundary/filesystem/clientLifecycle.test.ts (+19 -2)

📄 Description

What broke: rea setup --client grok_build (and uninstall) dropped comment lines that sat right before REA's [mcp_servers.rea] table, inside it, or after it at the end of the file, even though setup reported ready. Codex, which uses the same TOML editor, kept them.
Fix: The shared TOML server-table editor now keeps every comment line in place for both TOML dialects (Codex toml and Grok grok). The Codex-only rule that restores a file's last line break also moved into the shared editor.
Risk: Low. Only text the edit preserves changes. Parsed values, REA's registration, Grok's disabled_mcp_servers edit and backups are unchanged. For any input, Grok Build output now matches Codex output byte for byte.

Summary

Fixes #6207.

Setup and uninstall promise to keep unrelated entries and comments in agent configuration (docs/installation.md). For Grok Build's config.toml, a comment introducing a stale REA table (# about rea) and a trailing file note after it were both deleted. The original backup was intact.

Problem and expected behavior

Smallest trigger (from the issue):

# root explanation
model = "x"

# about rea
[mcp_servers.rea]
command = "old"

# trailing note about the file

Before, Grok Build setup produced # root explanation\nmodel = "x"\n\n[mcp_servers.rea]…, which dropped both comments. Codex produced … # about rea\n\n# trailing note about the file\n\n[mcp_servers.rea]….

Root cause: upsertTomlServerSection in src/application/ClientConfigurationDocument.ts kept comment lines right away only when format === "toml" (added for Codex in #1204). For grok it put comments in the same queue as blank lines. That queue was thrown away when an REA header followed or while REA's table was being skipped, and it was never flushed at end of file while skipping.

Change and scope

The comment branch no longer depends on dialect: comment lines are always kept where they are. The pending queue now holds only blank lines, which still go away with a removed REA table, as before.
The rule that "removing a final table also drops the file's last line break" used to live only in the Codex wrapper. It now runs in the shared editor, so Grok uninstall keeps a trailing comment and the final newline. One visible side effect: a Grok file that held only REA's table now becomes "\n" after uninstall instead of "", which is what Codex already did.
Not changed: an inline comment written on one of REA's own replaced statements or headers (for example command = "old" # stale) is still removed together with that statement, for both dialects, as before. Codex's parsed-value check (tomlTextHolds) is not applied to Grok, because Grok's text edit removes rea from disabled_mcp_servers, which the document model does not express.
Related but separate: #1732 changes which semantic fields of an existing entry are kept. It does not touch this text serializer, so there is no overlap.

Contribution scope: ordinary fix inside an existing boundary, so no maintainer scope decision is needed.

Contract and boundary impact

Semantic owner and earliest changed stage: upsertTomlServerSection, the shared TOML text editor that the Codex and Grok Build serializers both use.
Contribution scope fit and linked maintainer decision: not applicable (bug fix inside an existing boundary).
CLI and MCP/tool-catalog contract: none. JSON results and statuses are unchanged.
Provider, bridge, target-format, or platform compatibility: Codex and Grok Build config.toml only. LF, CRLF, BOM and no-final-newline handling are unchanged apart from the shared final-newline rule described above.
Evidence, artifact, provenance, or reconstruction contract: none.
Target selection, authority, upstream behavior, and cleanup details: not applicable.
Generated metadata, package, or installation impact: none.
Change process memory, registers, files, or configuration. (This keeps more of the user's existing client configuration text. Writes stay additive with backups.)

Evidence and regression coverage

Tests added or updated:
- New compiled-CLI acceptance test tests/acceptance/setup/tomlCommentPreservationCli.test.ts. It configures Codex and Grok Build together in one isolated home and runs --dry-run (bytes unchanged), --yes (comments kept, stale command replaced, unrelated parsed values equal, backup equals original), a second --yes (appliedActions: [], identical bytes), and uninstall (comments kept, REA removed, final newline kept, identical text for both clients). It covers two layouts. (1) A stale REA table in the middle, with comments before it, inside it, between [mcp_servers.rea] and [mcp_servers.rea.env], after the env table, inline comments on unrelated lines, a neighbouring table and a trailing note. (2) A leading REA table followed by a final note with no line break.
- Extended the Grok Build CRLF boundary test in tests/boundary/filesystem/clientLifecycle.test.ts with comments before the REA table, between it and its env table, and a trailing note. It asserts each comment survives setup and uninstall with CRLF intact and no bare LF introduced.
Base reproduction: the issue's repro test, run through the registered source CLI on origin/main, reproduced the loss for Grok (# about rea and # trailing note about the file missing, status ready, backup identical) while the Codex control kept them. With this change the same repro shows Grok output identical to Codex. Without the source change (tests kept, src reverted), both new acceptance tests and the extended boundary test fail on expected … to contain '# about rea'.
Extra counterexamples run against serializeClientConfiguration (temporary test, not committed), for both dialects: REA-only file, REA-only file with a leading comment, REA last with a comment between the table and its env subtable, CRLF without a final newline, dotted mcp_servers.rea.command keys. Codex and Grok produced byte-identical output in every case, and a second serialization was idempotent.
Remaining proof gaps: no native Grok Build or Codex client was run against the edited files. Windows was not exercised. Only macOS arm64 was tested.
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

Issue repro (tests/process-global/repro-grok-comment-loss.test.ts, temporary, isolated homes) on origin/main: reproduced the loss for Grok. With the fix: Grok kept both comments, which made the repro's wrong-behavior assertion fail as expected.
npm run test:focused -- tests/acceptance/setup/tomlCommentPreservationCli.test.ts tests/boundary/filesystem/clientLifecycle.test.ts: fails without the src change (3 failures), passes with it.
npm run test:focused -- on 52 related files (src/application/ClientConfiguration*.test.ts, src/application/Setup*.test.ts, every tests/boundary/filesystem client/setup/uninstall file, all of tests/acceptance/setup/, tests/boundary/cli setup/uninstall/client-profile files): 52 files passed, 725 tests passed, 13 skipped.
npm run test:focused -- tests/boundary/filesystem/windowsNpxRegistration.test.ts tests/process-global/setupConfigurationRollback.test.ts tests/process-global/updateMaintenance.test.ts src/setupEnvironment.test.ts: 4 files passed (82 passed, 2 skipped).
npm run check:fast: passed.
npm run format:check: passed.

Compatibility, safety, and release

Breaking changes or migration steps: none.
Real Hopper/Ghidra, browser, or OS coverage: not applicable. macOS arm64 only.
Package or release metadata impact: none.
Security, privacy, process, or containment review: no new reads, writes or processes. The same file is written, with the same backups.

Author readiness

Human owner: @kaoru0822-kitauji

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.

Before merge

This PR addresses a concrete problem or an agreed enhancement.
Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff.
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 output changes include representative output.
I checked the final diff for secrets, unrelated cleanup, and unsupported claims.

🤖 Generated with Claude Code

_{🔄 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/6261 **Author:** [@kaoru0822-kitauji](https://github.com/kaoru0822-kitauji) **Created:** 10/22/2026 **Status:** ✅ Merged **Merged:** 10/22/2026 **Merged by:** [@kaoru0822-kitauji](https://github.com/kaoru0822-kitauji) **Base:** `main` ← **Head:** `fix/6207-grok-setup-comments` --- ### 📝 Commits (1) - [`69581e6`](https://github.com/morluto/rea/commit/69581e6a497577fd109587e016c7dcce46734756) fix(setup): keep comments around REA's table in Grok Build TOML ### 📊 Changes **3 files changed** (+193 additions, -13 deletions) <details> <summary>View changed files</summary> 📝 `src/application/ClientConfigurationDocument.ts` (+12 -11) ➕ `tests/acceptance/setup/tomlCommentPreservationCli.test.ts` (+162 -0) 📝 `tests/boundary/filesystem/clientLifecycle.test.ts` (+19 -2) </details> ### 📄 Description **What broke:** `rea setup --client grok_build` (and uninstall) dropped comment lines that sat right before REA's `[mcp_servers.rea]` table, inside it, or after it at the end of the file, even though setup reported `ready`. Codex, which uses the same TOML editor, kept them. **Fix:** The shared TOML server-table editor now keeps every comment line in place for both TOML dialects (Codex `toml` and Grok `grok`). The Codex-only rule that restores a file's last line break also moved into the shared editor. **Risk:** Low. Only text the edit preserves changes. Parsed values, REA's registration, Grok's `disabled_mcp_servers` edit and backups are unchanged. For any input, Grok Build output now matches Codex output byte for byte. ## Summary Fixes #6207. Setup and uninstall promise to keep unrelated entries and comments in agent configuration ([docs/installation.md](https://github.com/morluto/rea/blob/main/docs/installation.md)). For Grok Build's `config.toml`, a comment introducing a stale REA table (`# about rea`) and a trailing file note after it were both deleted. The original backup was intact. ## Problem and expected behavior Smallest trigger (from the issue): ```toml # root explanation model = "x" # about rea [mcp_servers.rea] command = "old" # trailing note about the file ``` Before, Grok Build setup produced `# root explanation\nmodel = "x"\n\n[mcp_servers.rea]…`, which dropped both comments. Codex produced `… # about rea\n\n# trailing note about the file\n\n[mcp_servers.rea]…`. Root cause: `upsertTomlServerSection` in `src/application/ClientConfigurationDocument.ts` kept comment lines right away only when `format === "toml"` (added for Codex in #1204). For `grok` it put comments in the same queue as blank lines. That queue was thrown away when an REA header followed or while REA's table was being skipped, and it was never flushed at end of file while skipping. ## Change and scope - The comment branch no longer depends on dialect: comment lines are always kept where they are. The `pending` queue now holds only blank lines, which still go away with a removed REA table, as before. - The rule that "removing a final table also drops the file's last line break" used to live only in the Codex wrapper. It now runs in the shared editor, so Grok uninstall keeps a trailing comment *and* the final newline. One visible side effect: a Grok file that held only REA's table now becomes `"\n"` after uninstall instead of `""`, which is what Codex already did. - Not changed: an inline comment written on one of REA's own replaced statements or headers (for example `command = "old" # stale`) is still removed together with that statement, for both dialects, as before. Codex's parsed-value check (`tomlTextHolds`) is not applied to Grok, because Grok's text edit removes `rea` from `disabled_mcp_servers`, which the document model does not express. - Related but separate: #1732 changes which semantic fields of an existing entry are kept. It does not touch this text serializer, so there is no overlap. Contribution scope: ordinary fix inside an existing boundary, so no maintainer scope decision is needed. ## Contract and boundary impact - Semantic owner and earliest changed stage: `upsertTomlServerSection`, the shared TOML text editor that the Codex and Grok Build serializers both use. - Contribution scope fit and linked maintainer decision: not applicable (bug fix inside an existing boundary). - CLI and MCP/tool-catalog contract: none. JSON results and statuses are unchanged. - Provider, bridge, target-format, or platform compatibility: Codex and Grok Build `config.toml` only. LF, CRLF, BOM and no-final-newline handling are unchanged apart from the shared final-newline rule described above. - Evidence, artifact, provenance, or reconstruction contract: none. - Target selection, authority, upstream behavior, and cleanup details: not applicable. - Generated metadata, package, or installation impact: none. - [x] Change process memory, registers, files, or configuration. (This keeps more of the user's existing client configuration text. Writes stay additive with backups.) ## Evidence and regression coverage - Tests added or updated: - New compiled-CLI acceptance test `tests/acceptance/setup/tomlCommentPreservationCli.test.ts`. It configures Codex and Grok Build together in one isolated home and runs `--dry-run` (bytes unchanged), `--yes` (comments kept, stale command replaced, unrelated parsed values equal, backup equals original), a second `--yes` (`appliedActions: []`, identical bytes), and `uninstall` (comments kept, REA removed, final newline kept, identical text for both clients). It covers two layouts. (1) A stale REA table in the middle, with comments before it, inside it, between `[mcp_servers.rea]` and `[mcp_servers.rea.env]`, after the env table, inline comments on unrelated lines, a neighbouring table and a trailing note. (2) A leading REA table followed by a final note with no line break. - Extended the Grok Build CRLF boundary test in `tests/boundary/filesystem/clientLifecycle.test.ts` with comments before the REA table, between it and its env table, and a trailing note. It asserts each comment survives setup and uninstall with CRLF intact and no bare LF introduced. - Base reproduction: the issue's repro test, run through the registered source CLI on `origin/main`, reproduced the loss for Grok (`# about rea` and `# trailing note about the file` missing, status `ready`, backup identical) while the Codex control kept them. With this change the same repro shows Grok output identical to Codex. Without the source change (tests kept, `src` reverted), both new acceptance tests and the extended boundary test fail on `expected … to contain '# about rea'`. - Extra counterexamples run against `serializeClientConfiguration` (temporary test, not committed), for both dialects: REA-only file, REA-only file with a leading comment, REA last with a comment between the table and its env subtable, CRLF without a final newline, dotted `mcp_servers.rea.command` keys. Codex and Grok produced byte-identical output in every case, and a second serialization was idempotent. - Remaining proof gaps: no native Grok Build or Codex client was run against the edited files. Windows was not exercised. Only macOS arm64 was tested. - [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 - Issue repro (`tests/process-global/repro-grok-comment-loss.test.ts`, temporary, isolated homes) on `origin/main`: reproduced the loss for Grok. With the fix: Grok kept both comments, which made the repro's wrong-behavior assertion fail as expected. - `npm run test:focused -- tests/acceptance/setup/tomlCommentPreservationCli.test.ts tests/boundary/filesystem/clientLifecycle.test.ts`: fails without the `src` change (3 failures), passes with it. - `npm run test:focused --` on 52 related files (`src/application/ClientConfiguration*.test.ts`, `src/application/Setup*.test.ts`, every `tests/boundary/filesystem` client/setup/uninstall file, all of `tests/acceptance/setup/`, `tests/boundary/cli` setup/uninstall/client-profile files): 52 files passed, 725 tests passed, 13 skipped. - `npm run test:focused -- tests/boundary/filesystem/windowsNpxRegistration.test.ts tests/process-global/setupConfigurationRollback.test.ts tests/process-global/updateMaintenance.test.ts src/setupEnvironment.test.ts`: 4 files passed (82 passed, 2 skipped). - `npm run check:fast`: passed. - `npm run format:check`: passed. ## Compatibility, safety, and release - Breaking changes or migration steps: none. - Real Hopper/Ghidra, browser, or OS coverage: not applicable. macOS arm64 only. - Package or release metadata impact: none. - Security, privacy, process, or containment review: no new reads, writes or processes. The same file is written, with the same backups. ## Author readiness Human owner: @kaoru0822-kitauji - [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. ## Before merge - [x] This PR addresses a concrete problem or an agreed enhancement. - [x] Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff. - [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 output changes include representative output. - [x] I checked the final diff for secrets, unrelated cleanup, and unsupported claims. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-11-01 21:46:23 +00:00