Repository navigation
fix(cli): preserve malformed configuration before setup mutations - #401
DivyamTalwar wants to merge 5 commits into
Conversation
CLI installers and preference migration currently interpret malformed JSON or YAML as empty configuration, then overwrite or delete the user's file. Validate before payload mutations and refuse writes when parsing fails so recovery remains user-controlled.\n\nConstraint: Existing installs must preserve unrelated settings and malformed files must never be repaired by silent replacement\nRejected: Treat malformed config as empty and continue | destroys user data and hides the recovery action\nConfidence: high\nScope-risk: moderate\nReversibility: clean\nDirective: Keep malformed-config paths fail-safe; do not broaden parsers to auto-repair unknown user files\nTested: Focused Vitest installer and user-config suites; TypeScript noEmit\nNot-tested: Native Windows filesystem behavior
Malformed and unreadable user configuration must remain authoritative until the user repairs it. Validate installer configuration before payload mutations, preserve passive hook fallbacks, and surface explicit uninstall failures without rewriting user bytes. Constraint: Capture and startup readers must remain non-throwing even when the shared user config is malformed or unreadable Rejected: Preflight with existsSync | lookup errors can be misclassified as missing and trigger destructive migration Confidence: high Scope-risk: moderate Directive: Keep config validation ahead of every installer payload mutation and treat only ENOENT as missing Tested: 80 focused tests, 216 adjacent tests, TypeScript typecheck, build, jscpd Not-tested: Full repository test suite, real EACCES mount behavior, live user installations
|
Warning Review limit reachedNext included review available in 1 minute. View limit detailsLimit details: You’ve used all 4 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Repository: activeloopai/hivemind/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (11)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Heavy embeddings install and uninstall now perform a read-only user-config preflight before dependency, link, prune, or daemon work. Hermes also recognizes parser-confirmed empty YAML documents only for explicit markers and supported directives, while retaining scalar, collection, and explicit-null rejection. Focused hermetic tests cover malformed and injected-read failures plus exact YAML regressions. Constraint: Preserve passive user-config getter behavior, missing-file semantics, setter contracts, and the existing cache limitation\nRejected: Make writeUserConfig transactional across provisioning | broader scope than the review correction\nConfidence: high\nScope-risk: narrow\nReversibility: clean\nDirective: Do not infer broad TOCTOU guarantees from the read-only preflight\nTested: Node 22 focused/adjacent Vitest 156 tests; OpenClaw bundle wiring 5 tests; typecheck; build; duplication scan; critical-only bundle audit\nNot-tested: Full suite and coordinator-owned Linux gate
|
Final review found two additional edge cases: heavy embeddings cleanup can mutate payloads before strict config validation, and an explicit empty YAML document ( |
The empty-document regression needs to accept legal start/end marker pairs and supported directives while rejecting parser-null forms such as !!null and anchored nulls. Keep the parser success check, then use a narrow lexical structure check for the empty-document branch and preserve the valid-config preflight paths in the hermetic embeddings tests. Constraint: Preserve js-yaml parser authority, exact malformed-config bytes, existing config cache/setter contracts, and missing-file behavior Rejected: Accept every parsed null or rely only on parser root kind | tagged and anchored nulls also report null-like roots Confidence: high Scope-risk: narrow Reversibility: clean Directive: Keep lexical acceptance limited to legal empty markers/directives and ordered marker pairs Tested: Node 22 Hermes 27 tests; embeddings preflight 6 tests; combined focused/adjacent 160 tests; typecheck Not-tested: Final build/audit rerun after this follow-up and full Linux suite Related: 3725c43
The parser reports empty documents and explicit null-like roots through the same null value, but close-event metadata distinguishes an empty node from a tag or anchor. Keep the empty-document acceptance parser-driven and reject those explicit null forms without broadening the accepted syntax. Constraint: Preserve legal YAML markers/directives, BOM/comments, parser success semantics, and payload-preserving rejection Rejected: Treat every null-like parser result as an empty config | tagged and anchored null roots must remain rejected Confidence: high Scope-risk: narrow Reversibility: clean Directive: Retain the parser metadata guard whenever adding null-root compatibility cases Tested: Node 22 Hermes 29 tests; final focused matrix rerun pending Not-tested: Final build/audit rerun after this commit and full Linux suite Related: d2b7913
|
Both final-review findings are corrected in a061e94: heavy embeddings operations validate configuration before mutation, and Hermes accepts genuinely empty YAML documents without accepting tagged/anchored null roots. The previous head fails 11 new cases; the corrected head passes all 55 focused tests and the full Node 22/Linux gate: 5,880 passed, zero failed or skipped, with coverage/build/typecheck/duplication/bundle checks. Final validation: https://github.com/DivyamTalwar/hivemind/actions/runs/35674754671 . The PR body now reflects this exact head, and the temporary hold for these two findings is resolved. |
This branch has not been deployed
1 participant
Summary
Fixes #400.
Installers and first-run migration must not treat malformed or unreadable existing configuration as empty. Validate Hermes YAML and Codex/Cursor hook JSON before payload mutation; only ENOENT means missing. Passive configuration reads remain non-throwing, while migration and explicit writes refuse invalid or unreadable state.
The final review follow-up performs the same read-only preflight at the entry to heavy embeddings install and uninstall, before dependencies, shared payloads, links or daemon operations are changed. Hermes uses parser root metadata to accept genuinely empty YAML documents, including explicit document markers, while rejecting scalar, sequence, tagged-null and anchored-null roots.
Version Bump
No release requested. Package versions, dependencies, lockfiles and release workflows are unchanged.
Test plan
d72535a2PR head: 11 failed and 24 controls passed, reproducing heavy-operation ordering and explicit-empty-YAML rejection.a061e94c8f22989955e6f736145b77f6f98de502: independent Node 22/Linux full suite with coverage, 5,880 passed, zero failed, zero skipped. Typecheck, build, duplication guard, critical-only OpenClaw bundle audit and diff checks pass in the same gate.Final exact-commit commands and validation logs, job
config-validation-followup. This supersedes the previous-head validation for the follow-up changes.Limits
This is validation before mutation, not a transactional installer or an atomic check-and-use guarantee. Existing in-process configuration caching and concurrent external edits retain their prior limitations. Valid YAML writes retain the existing serializer behavior. Nonzero dispatcher status is independently fixed by #377 and is not duplicated here. Native Windows and real permission-denied mounts were not exercised.
The unchanged full macOS baseline has known failures and skips; a clean full macOS suite is not claimed. No real credentials, customer transcripts, live model/API calls, real package installation or real user configuration were used by regression tests.