Conversation
HergenD
left a comment
There was a problem hiding this comment.
Review
Config change (config.go) — Looks good ✓
Default SyncMinutes: 10 in defaults() and updated comment — clean and correct.
Staleness note (ribbit.go) — Needs fixes
1. origin/HEAD is unreliable (main issue)
origin/HEAD is a symref pointing to the remote's default branch. It's only set by git clone and rarely updated by git fetch. On many setups it doesn't exist at all, so git rev-parse origin/HEAD will fail and the function silently returns "" every time — effectively a no-op.
The intended comparison is HEAD vs the remote's latest. This should use origin/<DefaultBranch> (e.g. origin/main) instead. The function signature would need the default branch name passed in, which is available on the RepoConfig.
2. No tests
stalenessNote has no test coverage. A simple test with a temp git repo (init + commit + add remote) would verify both the stale and up-to-date paths.
3. exec.Command without context (minor)
Uses exec.Command instead of exec.CommandContext — inconsistent with the rest of the codebase. Not critical since rev-parse is near-instant.
4. Merge conflict (heads up)
The return &Response{Text: result.Result}, nil line this PR modifies has moved on the target branch due to recent changes. Will need a rebase.
Change the default sync_minutes from 0 (disabled) to 10 so repos stay fresh by default. Add a stalenessNote helper that compares HEAD against origin/<defaultBranch> and appends a warning to ribbit responses when the local checkout is behind. Address review feedback from PR #10: - Use origin/<defaultBranch> instead of unreliable origin/HEAD - Accept defaultBranch parameter through Respond and stalenessNote - Use exec.CommandContext for consistency - Add test coverage for stale, up-to-date, and empty-branch cases - Rebase onto main to resolve merge conflict
d680110 to
3bb71ad
Compare
Change the default sync_minutes from 0 (disabled) to 10 so repos stay fresh by default. Add a stalenessNote helper that compares HEAD against origin/<defaultBranch> and appends a warning to ribbit responses when the local checkout is behind. Address review feedback from PR #10: - Use origin/<defaultBranch> instead of unreliable origin/HEAD - Accept defaultBranch parameter through Respond and stalenessNote - Use exec.CommandContext for consistency - Add test coverage for stale, up-to-date, and empty-branch cases - Rebase onto main to resolve merge conflict
Summary
Change default sync_minutes from 0 to 10 in config, implement lightweight staleness check for ribbits that compares HEAD vs origin
Linear: PLF-3250
View Slack thread
Category: feature | Size: small
Suggested reviewers: @HergenD
Slack context
Implement two changes:
1. Change default sync_minutes from 0 to 10
In
internal/config/config.go, thedefaults()function (line 154) does not setRepos, soSyncMinutesdefaults to Go's zero value (0 = disabled). Add aReposfield to the returned struct:Also update the comment on line 42 from
// periodic git fetch interval; 0 = disabled (default)to// periodic git fetch interval; 0 = disabled, default 10.2. Add lightweight staleness check to ribbits
In
internal/ribbit/ribbit.go, add a package-level helper function:Add
os/execto the import block (it is not currently imported in ribbit.go).In
Engine.Respond(), after the agent result is obtained and the empty-check passes (around line 162), append the staleness note:The note is appended only when HEAD and origin/HEAD differ, which is a purely local ref comparison with no network cost.
🐸 Created by toad tadpole