feat(daemon): coalesce workspace.toml auto-sync commits via push cooldown#47
Conversation
…down Every reconciler tick that found workspace.toml dirty stacked another "ws: auto-sync workspace.toml from <host>" commit and pushed it immediately, flooding the dotfiles history (97 of the last 100 commits on dotfiles/main were auto-syncs). syncTOML now amends consecutive auto-sync edits onto the most recent unpushed local commit when its message matches, and holds the push while that commit is younger than the configured cooldown. Once the cooldown elapses (or a non-auto-sync commit lands on top), the next tick pushes as before. Cooldown is wired through daemon.toml as push_cooldown (default 1h). ws sync stays at cooldown=0 by design — an explicit user-driven sync should still push immediately.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fa23da61ef
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return false, fmt.Errorf("git commit: %w", err) | ||
| headMsg, _ := git.LastCommitMessage(repoRoot) | ||
| if ahead > 0 && headMsg == autoSyncMsg { | ||
| if err := runIn(repoRoot, "git", "commit", "--amend", "--no-edit"); err != nil { |
There was a problem hiding this comment.
When workspace.toml is dirtied on every daemon tick, this amend refreshes the commit's committer timestamp; shouldHoldPush later compares time.Since(git.LastCommitTime(...)) against the cooldown, so the one-hour window restarts on every amend and the held auto-sync commit may never be pushed while activity continues. I verified git commit --amend --no-edit updates %cI; use a stable first-hold time (or author date) for the cooldown check so the advertised maximum sync delay is actually bounded.
Useful? React with 👍 / 👎.
| d, err := time.ParseDuration(w.PushCooldown) | ||
| if err != nil { | ||
| return DefaultPushCooldown |
There was a problem hiding this comment.
The new config comments document push_cooldown = "0" as disabling coalescing, but time.ParseDuration("0") fails because it has no unit, so this falls into the error branch and silently re-enables the default one-hour cooldown. A user trying to restore immediate daemon pushes with the documented value will still have auto-sync commits deferred; handle the literal "0" before parsing or document/use "0s".
Useful? React with 👍 / 👎.
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR adds configurable push cooldown for the workspace daemon to coalesce consecutive auto-sync commits into a single amended commit instead of pushing immediately. Configuration is resolved from the workspace TOML, applied to the reconciler during daemon startup, and enforced by the sync logic through commit amending and push gating. ChangesAuto-Sync Commit Coalescing with Configurable Cooldown
Sequence DiagramsequenceDiagram
participant Daemon as Daemon
participant Config as WorkspaceEntry
participant Reconciler
participant syncTOML
participant shouldHold as shouldHoldPush()
participant GitOps as Git Operations
Daemon->>Config: read PushCooldown
Config-->>Daemon: resolved duration
Daemon->>Reconciler: SetPushCooldown(duration)
Note over syncTOML: During workspace reconciliation
syncTOML->>syncTOML: detect dirty workspace.toml
alt HEAD matches auto-sync message
syncTOML->>GitOps: git commit --amend
else first commit or no match
syncTOML->>GitOps: git commit (new)
end
alt have commits to push
syncTOML->>shouldHold: check cooldown gate
alt within cooldown window
shouldHold-->>syncTOML: true
syncTOML->>syncTOML: log hold for amend
else cooldown expired or disabled
shouldHold-->>syncTOML: false
syncTOML->>GitOps: git push
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/daemon/toml.go (1)
15-122: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftRefactor
syncTOMLto reduce cyclomatic complexity before merge.This function now packs too many decision branches into one path; please extract commit, rebase, and push-gate branches into focused helpers to get complexity back under the repo threshold.
As per coding guidelines, "Cyclomatic complexity thresholds for production Go: extract immediately if > 15; extract on next touch if already > 10".
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/daemon/toml_test.go`:
- Line 1: The test file uses package daemon but must use the external test
package daemon_test; change the package declaration in toml_test.go from
"package daemon" to "package daemon_test" and update any references so tests
only use exported symbols (or add exported helpers in the daemon package if
necessary), adjusting imports to import the daemon package where the tests call
functions/types (e.g., reference daemon.ParseToml or daemon.SomeType) so the
file conforms to the `<package>_test.go` external-test pattern.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2d974f63-43e7-4fa8-a454-55e3b9e5aff2
📒 Files selected for processing (5)
internal/daemon/config.gointernal/daemon/daemon.gointernal/daemon/reconciler.gointernal/daemon/toml.gointernal/daemon/toml_test.go
| @@ -0,0 +1,166 @@ | |||
| package daemon | |||
There was a problem hiding this comment.
Use an external test package (daemon_test) for this test file.
toml_test.go is using package daemon, which violates the repository rule for test package layout.
As per coding guidelines, "**/*_test.go: Test files live next to the code they cover in _test packages following the pattern <package>_test.go".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/daemon/toml_test.go` at line 1, The test file uses package daemon
but must use the external test package daemon_test; change the package
declaration in toml_test.go from "package daemon" to "package daemon_test" and
update any references so tests only use exported symbols (or add exported
helpers in the daemon package if necessary), adjusting imports to import the
daemon package where the tests call functions/types (e.g., reference
daemon.ParseToml or daemon.SomeType) so the file conforms to the
`<package>_test.go` external-test pattern.
Two issues surfaced in PR #47 review: 1. shouldHoldPush compared time.Since(LastCommitTime), but %cI refreshes on every git commit --amend --no-edit. Under continuous workspace.toml activity the cooldown window restarted on each tick, so the held auto-sync commit was never pushed — exactly the failure mode the gate exists to prevent. Switch to LastCommitAuthorTime (%aI, preserved across amends), backed by a new helper in internal/git. 2. ResolvedPushCooldown documented push_cooldown = "0" as the way to disable coalescing, but time.ParseDuration rejects a bare "0" (no unit), so it silently fell back to the 1h default. Short-circuit the literal "0" before parsing. Tests: new TestSyncTOMLAmendPreservesAuthorTimeAndReleasesGate proves amends preserve the author date and that the gate actually releases once real wall time elapses (uses a 2s cooldown to clear git's 1s author-date resolution). TestResolvedPushCooldown table-covers the parse contract, including the bare-"0" short-circuit.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9380fd50c5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if ahead > 0 && headMsg == autoSyncMsg { | ||
| if err := runIn(repoRoot, "git", "commit", "--amend", "--no-edit"); err != nil { | ||
| return false, fmt.Errorf("git commit --amend: %w", err) |
There was a problem hiding this comment.
When a held auto-sync edit is undone before the cooldown expires (for example toggling a favorite on and then off), this amend attempts to make HEAD's tree identical to its parent. Git rejects that case with would make it empty and leaves workspace.toml staged, so every subsequent daemon tick returns from syncTOML before it can push or clear the held commit, leaving the workspace repo permanently stuck ahead until the user intervenes.
Useful? React with 👍 / 👎.
When a held auto-sync edit is undone before the cooldown elapses (toggling a favorite on then off, adding a worktree then removing it, etc.), the amend would produce a tree identical to the held commit's parent. git refuses that case, so the reconciler returned an error from syncTOML every subsequent tick, never pushed, and left workspace.toml staged forever. Detect the empty-amend case with `git diff --cached --quiet HEAD~1` and mixed-reset back to the parent — the right history outcome is no commit at all, not an --allow-empty no-op. Regression test TestSyncTOMLAmendRevertedEditDropsHeldCommit walks the hold → revert → tick sequence and asserts HEAD rolls back to the parent, ahead=0, workspace.toml clean, remote unchanged.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 70e35fcf4c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // a non-auto-sync commit lands on top of it. | ||
| if ahead > 0 || behind > 0 { | ||
| if err := git.Push(repoRoot); err != nil { | ||
| if r.shouldHoldPush(repoRoot, autoSyncMsg) { |
There was a problem hiding this comment.
When the workspace repo already has an unpushed non-auto commit and the daemon then creates an auto-sync commit on top of it, ahead is greater than 1 but shouldHoldPush only checks that HEAD is the auto-sync message. In that scenario this branch defers the whole git push, so the user's manual workspace-repo commit is withheld for the cooldown window even though the feature is meant to coalesce only consecutive auto-sync commits; require the held commit to be the sole local-ahead commit (or otherwise detect the full ahead range) before skipping the push.
Useful? React with 👍 / 👎.
…range shouldHoldPush only checked HEAD's message, so when a user's manual workspace.toml commit sat below a held auto-sync, ahead would be 2 and the gate still fired. A single `git push` publishes both commits, so withholding the auto-sync also withholds the user's work for up to the full cooldown window. Require ahead == 1 — the held auto-sync must be the sole local commit — before deferring the push. Anything else means the cooldown is no longer the only thing the user is waiting on, and we push. Regression test TestSyncTOMLDoesNotHoldPushWhenManualCommitIsAhead seeds a manual commit, runs a dirty tick with cooldown=1h, and asserts the remote advances to HEAD (both commits published).
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/daemon/toml.go (1)
15-134: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftExtract
syncTOMLinto smaller helpers to reduce complexity risk.This function now combines fetch state, amend/drop decisions, rebase handling, and push gating in one control flow. Please split into focused helpers (e.g., commit/amend phase, rebase phase, push phase) before complexity drifts further.
As per coding guidelines, "Cyclomatic complexity thresholds for production Go: extract immediately if > 15; extract on next touch if already > 10".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/daemon/toml.go` around lines 15 - 134, syncTOML is too large and mixes commit/amend decisions, rebase handling, and push gating; split it into clear helpers: extract the commit/amend logic (the localDirty block and auto-sync amend/drop behavior) into a helper like commitOrAmend(repoRoot, relFile, autoSyncMsg) that returns updated ahead and error; extract the rebase-on-behind logic (the "if behind>0 { git pull --rebase ... }" plus conflict recording and clearTOMLConflicts) into rebaseIfBehind(repoRoot, branch) returning error; and extract the push/retry/hold logic (shouldHoldPush check, git.Push + retry pull+rebase+push and conflict recording) into pushIfNeeded(repoRoot, branch, ahead, behind, autoSyncMsg). Update syncTOML to call these helpers in sequence, pass and update ahead/behind/originalHead as needed, and keep only orchestration and simple checks (fetch, isClean, findGitRoot) in syncTOML to reduce cyclomatic complexity.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/daemon/config_test.go`:
- Line 1: Change the test file package from package daemon to package
daemon_test, update the test code to import the daemon package under a named
import (e.g., "daemon") and qualify all referenced symbols (types, functions,
vars) with the daemon prefix (e.g., daemon.SomeFunc, daemon.SomeType) so the
tests run as an external package; ensure any helper fixtures previously
unexported are either exported or replaced with test-local helpers in this file
and adjust imports accordingly.
---
Outside diff comments:
In `@internal/daemon/toml.go`:
- Around line 15-134: syncTOML is too large and mixes commit/amend decisions,
rebase handling, and push gating; split it into clear helpers: extract the
commit/amend logic (the localDirty block and auto-sync amend/drop behavior) into
a helper like commitOrAmend(repoRoot, relFile, autoSyncMsg) that returns updated
ahead and error; extract the rebase-on-behind logic (the "if behind>0 { git pull
--rebase ... }" plus conflict recording and clearTOMLConflicts) into
rebaseIfBehind(repoRoot, branch) returning error; and extract the
push/retry/hold logic (shouldHoldPush check, git.Push + retry pull+rebase+push
and conflict recording) into pushIfNeeded(repoRoot, branch, ahead, behind,
autoSyncMsg). Update syncTOML to call these helpers in sequence, pass and update
ahead/behind/originalHead as needed, and keep only orchestration and simple
checks (fetch, isClean, findGitRoot) in syncTOML to reduce cyclomatic
complexity.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9334c968-8415-446b-9043-491d4949c21c
📒 Files selected for processing (5)
internal/daemon/config.gointernal/daemon/config_test.gointernal/daemon/toml.gointernal/daemon/toml_test.gointernal/git/git.go
2 participants
Summary
ws: auto-sync workspace.toml from <host>commits onto the most recent unpushed local commit instead of stacking a fresh commit per tick.push_cooldown(newdaemon.tomlfield, default1h). Once the cooldown elapses — or a non-auto-sync commit lands on top — the next tick pushes as before.ws synckeepscooldown=0by design: an explicit user-driven sync still pushes immediately.Motivation
On
dotfiles/main, 97 of the last 100 commits werews: auto-sync workspace.toml from linux. Every reconciler tick that found the file dirty (typical Phase 2 metadata bumps likelast_active_at) produced a brand-new commit and pushed it.Test plan
go test -race -timeout 5m ./...internal/daemon/toml_test.gocovers two paths (real git, no mocks):TestSyncTOMLAmendCooldownSquashesAutoSyncCommits— two consecutive edits withcooldown=1hproduce one local amended commit, remote untouched; flipping cooldown to 0 pushes the held commit on the next tick.TestSyncTOMLZeroCooldownPushesEveryCommit— pins legacyws syncbehaviour (each edit → distinct commit, immediate push).Migration
No schema change.
daemon.tomlworkspace entries withoutpush_cooldowndefault to1h. Other machines need to upgrade their binary andws daemon restartto pick up the new behaviour.Summary by CodeRabbit
New Features
push_cooldownoption to control auto-sync commit coalescing (default 1h,"0"disables). Holds recent auto-sync commits to allow amends within the cooldown window.Behavior
Tests