claude attach silently replaces a malformed env/hooks block instead of refusing, unlike the openclaw sibling which throws MALFORMED_CONFIG

[philcunliffe]

Two instances of one question: should attach repair a malformed settings file, or refuse it?

Both found while fixing the type-gate bug class in PR #449, and deliberately left alone there because answering them is a design call, not a bug fix.

1. ensureObject silently discards a non-object env (Medium)

hypaware-core/plugins-workspace/claude/src/settings.js:284. If ~/.claude/settings.json has an env key whose value is present but not an object (a string, an array, a number - typically a hand-edit mistake), ensureObject replaces it with {}.

No backup. No warning. No prev_* record. The user's content is gone and nothing reports it. Attach then proceeds and returns success.

Contrast the sibling implementation, hypaware-core/plugins-workspace/openclaw/src/settings.js:756, which does the same job by throwing MALFORMED_CONFIG. Two client plugins in the same tree answer the same question opposite ways, and the destructive answer is the one with no code comment explaining itself.

2. Same shape for a non-array hooks.<event> (Low)

hypaware-core/plugins-workspace/claude/src/settings.js:307. A present-but-non-array hook list is replaced rather than refused, with the same silence.

Why this is a decision, not a patch

The obvious fix - throw MALFORMED_CONFIG, matching openclaw - changes attach from "always succeeds, repairing what it must" to "refuses a file it does not understand". That is a real behaviour change for anyone whose settings file is already malformed: today they get a working attach and lose a key they probably did not mean to have; afterwards they get an error and have to fix the file by hand.

Options:

1. Refuse (match openclaw). Throw MALFORMED_CONFIG naming the offending key. Most consistent, most protective, breaks the "attach always works" property.
2. Back up, then repair. Record the malformed value in the _hypaware marker (as prev_env etc.) so hyp detach restores it, and warn. Preserves the current success path and makes it reversible - the same principle the marker already implements for ANTHROPIC_BASE_URL.
3. Warn and repair. Cheapest; the value is still lost, but the user is told.

Option 2 is arguably the most in keeping with LLP 0044/0045 (the marker IS the backup), but it is more work and expands the marker schema.

Ground-truth gate for whoever fixes this

A regression test must start from a settings file with a non-object env (and separately a non-array hooks.<event>), run attach, and assert the chosen behaviour - and for options 2/3, assert the user's original content is recoverable or reported. Exercise the malformed path, not just the happy one.

Provenance

Surfaced during review round 2 of PR #449 (#449) and reproduced there. Filed separately so it is not lost when #449 merges and closes #448. This is a different class from #449's own fixes (which were type-gates standing in for presence tests); here the check is correct but the response to a failed check is the question.

[philcunliffe]

neutral is not attempting this: it is a design decision, and the options are already in the issue body.

What neutral was doing. #454 carries neutral:fix, so the issue-fix reconciler picked it up for a fix attempt.

Why it cannot proceed. The check at settings.js:284 is correct; the question is what attach should do when it fails. Two client plugins in the same tree already answer it opposite ways - claude silently repairs, openclaw/src/settings.js:756 throws MALFORMED_CONFIG - so there is no in-tree convention to follow, and no LLP records the intent.

The three options are costed in the issue body (refuse and match openclaw; back up into the marker so detach restores it; warn and repair). Each is a different promise to the user, and the first changes behaviour for anyone whose settings file is already malformed: today they get a working attach and quietly lose a key; afterwards they get an error and must hand-fix the file. That is your call, not neutral's.

What it needs from you. Pick an option. neutral's own read is that option 2 (back up into the _hypaware marker, so the loss is reversible) is most in keeping with LLP 0044/0045 - the marker IS the backup - but it is the most work and expands the marker schema.

How to unstick. Reply here naming the option (or remove the neutral:stuck label). neutral monitors this thread and will implement it with a regression test on its next tick.

[philcunliffe]

Decision: option 2, back up then repair. Record the malformed value in the _hypaware marker (prev_env / prev_hooks style) so hyp detach restores it, warn the user at attach time, and keep the success path. This is the marker-is-the-backup principle from LLP 0044/0045 applied consistently; expand the marker schema as needed.

Cover both instances (non-object env, non-array hooks.<event>), and per the issue's gate: regression tests must start from the malformed file, run attach, and assert the original content is recoverable via detach and that the warning fired. Worth a short note on whether openclaw should converge on the same behaviour, but do not change openclaw in this PR.