HarperFast / HarperFast/harper
set_configuration reports success when a resolved param fails to write (configDoc.setIn error is swallowed)
- 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
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