feat(config): support watch key in TOML config#279
Conversation
Greptile SummaryThis PR enables the
Confidence Score: 4/5Safe to merge; the core resolution logic is correct and the four primary TOML x CLI combinations are tested. The two-line fix to config.ts behaves correctly through the three-pass resolution order. The only gaps are a missing parametric case and a for-loop test structure that hides later failures — neither affects production behavior. src/core/config.test.ts has a small coverage gap worth filling but nothing that blocks merging. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[resolveConfiguredCliInput called] --> B[Init resolvedOptions]
B --> C{Global config exists?}
C -- yes --> D[mergeOptions with global TOML layer]
C -- no --> E{Repo config exists?}
D --> E
E -- yes --> F[mergeOptions with repo TOML layer]
E -- no --> G[mergeOptions with input.options]
F --> G
G --> H[Final override: watch = CLI ?? resolvedOptions.watch ?? false]
H --> I{CLI watch set?}
I -- yes --> J[watch = true]
I -- no --> K{TOML watch set?}
K -- yes --> L[watch = TOML value]
K -- no --> M[watch = false]
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
src/core/config.test.ts:164-208
**Missing test case: TOML watch=true with CLI watch=false**
The resolution table covers four of the five meaningful cells but omits the case `{ config: "watch = true\n", cliOptions: { watch: false }, expected: false }` — CLI explicitly suppressing a config-enabled watch. If `CommonOptions.watch` can legitimately be `false` rather than `undefined` (e.g. a future `--no-watch` flag or a programmatic caller that sets it explicitly), the final override expression `input.options.watch ?? resolvedOptions.watch ?? false` would return `false` correctly, but this is untested. Adding the case would lock in that behavior and make the intent of `??` vs `&&` clear for future readers.
### Issue 2 of 2
src/core/config.test.ts:192-207
**Loop-based parametric test hides later failures on early assertion error**
The `for` loop means that if the first `expect` throws, Bun reports only one failure and skips the remaining three cases. Using `test.each` (or Bun's `describe.each`) would make each cell an independent test so all failing cases surface together — the same pattern would also be more consistent with how the other parametric tests in this project are typically structured.
Reviews (1): Last reviewed commit: "feat(config): support watch key in TOML ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| test("resolves watch from config unless CLI explicitly enables it", () => { | ||
| const cases: Array<{ | ||
| config: string; | ||
| cliOptions: Partial<CliInput["options"]>; | ||
| expected: boolean; | ||
| }> = [ | ||
| { | ||
| config: "watch = true\n", | ||
| cliOptions: {}, | ||
| expected: true, | ||
| }, | ||
| { | ||
| config: "watch = false\n", | ||
| cliOptions: {}, | ||
| expected: false, | ||
| }, | ||
| { | ||
| config: "", | ||
| cliOptions: {}, | ||
| expected: false, | ||
| }, | ||
| { | ||
| config: "watch = false\n", | ||
| cliOptions: { watch: true }, | ||
| expected: true, | ||
| }, | ||
| ]; | ||
|
|
||
| for (const entry of cases) { | ||
| const home = createTempDir("hunk-config-home-"); | ||
| mkdirSync(join(home, ".config", "hunk"), { recursive: true }); | ||
| writeFileSync(join(home, ".config", "hunk", "config.toml"), entry.config); | ||
|
|
||
| const resolved = resolveConfiguredCliInput( | ||
| { | ||
| kind: "vcs", | ||
| staged: false, | ||
| options: entry.cliOptions, | ||
| }, | ||
| { cwd: createTempDir("hunk-config-cwd-"), env: { HOME: home } }, | ||
| ); | ||
|
|
||
| expect(resolved.input.options.watch).toBe(entry.expected); | ||
| } | ||
| }); |
There was a problem hiding this comment.
The resolution table covers four of the five meaningful cells but omits the case { config: "watch = true\n", cliOptions: { watch: false }, expected: false } — CLI explicitly suppressing a config-enabled watch. If CommonOptions.watch can legitimately be false rather than undefined (e.g. a future --no-watch flag or a programmatic caller that sets it explicitly), the final override expression input.options.watch ?? resolvedOptions.watch ?? false would return false correctly, but this is untested. Adding the case would lock in that behavior and make the intent of ?? vs && clear for future readers.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/core/config.test.ts
Line: 164-208
Comment:
**Missing test case: TOML watch=true with CLI watch=false**
The resolution table covers four of the five meaningful cells but omits the case `{ config: "watch = true\n", cliOptions: { watch: false }, expected: false }` — CLI explicitly suppressing a config-enabled watch. If `CommonOptions.watch` can legitimately be `false` rather than `undefined` (e.g. a future `--no-watch` flag or a programmatic caller that sets it explicitly), the final override expression `input.options.watch ?? resolvedOptions.watch ?? false` would return `false` correctly, but this is untested. Adding the case would lock in that behavior and make the intent of `??` vs `&&` clear for future readers.
How can I resolve this? If you propose a fix, please make it concise.| for (const entry of cases) { | ||
| const home = createTempDir("hunk-config-home-"); | ||
| mkdirSync(join(home, ".config", "hunk"), { recursive: true }); | ||
| writeFileSync(join(home, ".config", "hunk", "config.toml"), entry.config); | ||
|
|
||
| const resolved = resolveConfiguredCliInput( | ||
| { | ||
| kind: "vcs", | ||
| staged: false, | ||
| options: entry.cliOptions, | ||
| }, | ||
| { cwd: createTempDir("hunk-config-cwd-"), env: { HOME: home } }, | ||
| ); | ||
|
|
||
| expect(resolved.input.options.watch).toBe(entry.expected); | ||
| } |
There was a problem hiding this comment.
The for loop means that if the first expect throws, Bun reports only one failure and skips the remaining three cases. Using test.each (or Bun's describe.each) would make each cell an independent test so all failing cases surface together — the same pattern would also be more consistent with how the other parametric tests in this project are typically structured.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/core/config.test.ts
Line: 192-207
Comment:
**Loop-based parametric test hides later failures on early assertion error**
The `for` loop means that if the first `expect` throws, Bun reports only one failure and skips the remaining three cases. Using `test.each` (or Bun's `describe.each`) would make each cell an independent test so all failing cases surface together — the same pattern would also be more consistent with how the other parametric tests in this project are typically structured.
How can I resolve this? If you propose a fix, please make it concise.
|
@mvanhorn If you can address Greptile's comments I think we can smash merge |
2 participants
Summary
watch = truein~/.config/hunk/config.toml(or.hunk/config.toml) now actually enables watch mode without needing--watchon every invocation. CLI input still wins -- the global default is unchanged.Why this matters
#211 asked whether
--watchshould be the default. The thread surfaced two reasonable directions: flip the global default (risk: surprise users who don't expect content to change under them), or let users opt in via config. ramicats's suggestion in the top community comment (2 upvotes) was the latter:always_watch = truein config. This PR ships that opt-in without making any global-default decision.The fix turned out to need two changes, not one:
readConfigPreferences(config.ts:56-68) didn't listwatchin its returned shape, so awatch = truekey in TOML was silently dropped before reachingmergeOptions.mergeOptions(resolvedOptions, input.options), the final-override block at config.ts:184-198 explicitly resetswatch: input.options.watch ?? false-- hard-resetting any TOML-merged value back to CLI-only sourcing. The same pattern is what makespagerandagentContextCLI-only today.Without (2), the test "TOML
watch = true→options.watch === true" would have failed even after the (1) change. Both are required.Changes
src/core/config.ts:readConfigPreferencesnow readssource.watchvianormalizeBoolean.watch: input.options.watch ?? falsetowatch: input.options.watch ?? resolvedOptions.watch ?? false. CLI explicit wins; otherwise the config-merged value wins; otherwise defaultfalse.src/core/config.test.ts: newresolves watch from config unless CLI explicitly enables itcovering the full resolution table -- TOML on/off × CLI absent/explicit.README.md: addedwatch = falseto the config example block.pagerandagentContextfollow the same final-override pattern and could plausibly be made config-readable in follow-up PRs; deliberately out of scope here to keep this diff minimal and reversible.Testing
bun run typecheck-- cleanbun run format:check-- cleanbun run lint-- clean (0 warnings)bun test ./src/core/config.test.ts-- 10 pass, 0 fail (the new resolution-table test covers all 4 cells)The CLI behavior is unchanged when no config sets
watch: omitting--watchstill means "not watching." The only behavior change is thatwatch = truein TOML now actually takes effect.Fixes #211
AI was used for assistance.