HarperFast / HarperFast/harper

set_configuration reports success when a resolved param fails to write (configDoc.setIn error is swallowed)

Open
#2,269 0 comments 0 reactions 0 assignees View on GitHub
bug
Dominant language
JavaScript
Stars
89
Forks
10
Avg merge
2d 6h
Merged PRs (30d)
200

Description

## Summary

In `updateConfigValue`'s multi-parameter loop, a recognized param whose write throws is caught and logged, then the loop continues and `setConfiguration` returns its success response. The caller is told the configuration was set.

```js
// config/configUtils.ts (multi-param loop)
try {
if (splitParam.length > 1) { /* ... */ }
configDoc.setIn([...splitParam], newValue);
} catch (err) {
logger.error(err); // <- swallowed; loop continues, success still returned
}
```

`setConfiguration` then returns `CONFIGURE_SUCCESS_RESPONSE` unconditionally.

## Failure scenario

`set_configuration` with a valid param (`logging_level`) against a config file where the containing node is one `setIn` refuses to descend into — a YAML alias/anchor node, or a scalar where a mapping is expected. The value never lands, HTTP 200 comes back with `Configuration successfully set. You must restart Harper for new config settings to take effect.`, and the only evidence is a single `logger.error` line. The operator then restarts, which "applies" nothing.

## Relationship to the other two

This is the third distinct mechanism by which `set_configuration` reports success without applying a change:

- #2266 — the param name is unrecognized, so the write is skipped (fixed by rejecting the request).
- #1950 — the change-detection comparison runs against a stale `flatConfigObj`, so the write is skipped as a no-op.
- **this** — the write is attempted, throws, and is swallowed.

#2266's fix does not reach this one: it validates names before the write, whereas this failure happens during the write, for a name that resolved fine. Worth fixing in the same area while the code is warm.

## Suggested shape

Collect per-param write failures the way #2266's fix collects unrecognized names, and fail the operation rather than logging and continuing — or, if partial application must remain possible for some caller, return the failures in the response so success is never reported for a write that did not happen. The `logger.error(err)` on its own is not a signal any API client can act on.

Note the same catch-and-continue exists in `createConfigFile`'s loop; whether install should also fail loudly is a separate call, since install args legitimately include non-config keys.

Contributor guide

Open the contributing guide

Research direction

Start in config/configUtils.ts at updateConfigValue’s multi-parameter loop and follow how setConfiguration produces CONFIGURE_SUCCESS_RESPONSE. Reproduce a setIn failure with an alias/anchor node or scalar parent, then add coverage showing that a failed write cannot produce a success response; done means failures are reported to the API client rather than only logged.

Written by the indexing model from the issue text.

Assessment

Tech stack
nodejs, typescript
Domain
api, backend
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
66/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.