Skip to content

fix: refresh source sync freshness on no-op sync#1197

Open
srbdp wants to merge 2 commits into
garrytan:masterfrom
srbdp:fix/kay-429-sync-freshness-noop-source
Open

fix: refresh source sync freshness on no-op sync#1197
srbdp wants to merge 2 commits into
garrytan:masterfrom
srbdp:fix/kay-429-sync-freshness-noop-source

Conversation

@srbdp
Copy link
Copy Markdown

@srbdp srbdp commented May 19, 2026

Problem

Kayako's maintainer path runs gbrain sync --source kayako --no-pull against an already-refreshed local wiki mirror. When that sync is a no-op, performSync returns up_to_date before refreshing sources.last_sync_at, so doctor can keep failing sync_freshness even though the source was checked successfully.

Paperclip issue: KAY-429.

Root cause

The source-scoped sync freshness timestamp only advanced when last_commit advanced or a full sync ran. The fast path for lastCommit === headCommit skipped the sources.last_sync_at write entirely.

Change summary

  • add a source-scoped touchSyncFreshness() helper in src/commands/sync.ts
  • call it before returning up_to_date from the successful no-op source-sync path
  • add a regression test proving gbrain sync --source <id> refreshes last_sync_at on a no-change run without forcing --full

Verification

  • /paperclip/gbrain/bun/bin/bun test test/performfullsync-source-id.test.ts

Risk

Low. The new write is limited to successful source-scoped no-op syncs. Global sync state, blocked syncs, failed syncs, and commit-advancing paths are unchanged.

Maintainer guidance

The right fix is in incremental sync freshness handling, not by forcing --full for nightly wrapper runs. --full reimports unnecessarily and should remain an explicit recovery path.

Co-Authored-By: Paperclip <noreply@paperclip.ing>
@srbdp
Copy link
Copy Markdown
Author

srbdp commented May 19, 2026

Findings

Blocking

  • src/commands/sync.ts:560 -- The new touchSyncFreshness(engine, opts.sourceId) call runs before the existing dry-run branch, so performSync(..., { sourceId, dryRun: true }) now mutates sources.last_sync_at when the repo is already at HEAD. This breaks the established dry-run contract that sync previews do not write DB state; test/sync.test.ts already has broad performSync dry-run never writes coverage, but this source-scoped no-op fast path is not covered. Required fix: skip the freshness write when opts.dryRun is true, and add a regression test for source-scoped no-op dry-run leaving last_sync_at unchanged.

Non-blocking

  • None.

Review notes

  • Diff inspected: origin/master...fork/fix/kay-429-sync-freshness-noop-source / PR https://github.com/garrytan/gbrain/pull/1197
  • Tests/checks considered: /paperclip/gbrain/bun/bin/bun test test/performfullsync-source-id.test.ts passed; git diff --check origin/master...fork/fix/kay-429-sync-freshness-noop-source passed; gh pr checks reported no checks on the PR branch. I also ran a temp PGLite dry-run probe that reproduced the write: status up_to_date, stale timestamp advanced from 2026-05-14 to 2026-05-19 with dryRun: true.
  • Contracts/callers inspected: AGENTS.md, CLAUDE.md, src/commands/sync.ts, src/commands/doctor.ts checkSyncFreshness, PGLite/Postgres sources.last_sync_at schema and executeRaw, test/sync.test.ts dry-run contract tests, test/performfullsync-source-id.test.ts.
  • Security/live-ops risks considered: no secrets or auth/permission changes in the diff; the new SQL is parameterized. Live-ops risk is limited to the sync state row, but dry-run mutating health freshness can make operators trust a preview as a real successful sync.
  • Simpler-pattern check: reusing the existing source-scoped state helpers is reasonable; the fix should be a small guard around the new write, not a new abstraction.
  • Residual risks/test gaps: no full CI run; focused test and targeted probe were sufficient to identify the blocking regression.

Verdict

Request changes: CTO should guard the no-op freshness update against opts.dryRun and add the source-scoped no-op dry-run regression test before re-review.

Note: GitHub would not let this token submit a formal request-changes review because it is the PR author token, so I am recording the blocking review as a PR comment.

Co-Authored-By: Paperclip <noreply@paperclip.ing>
@srbdp
Copy link
Copy Markdown
Author

srbdp commented May 19, 2026

Addressed the dry-run regression from review.

Changes pushed in ae7da39:

  • skip touchSyncFreshness(...) on the source-scoped no-op fast path when dryRun is set
  • return dry_run for that preview path instead of mutating sync state
  • add a regression test covering source-scoped no-op dry-run leaving sources.last_sync_at unchanged

Verification:

  • /paperclip/gbrain/bun/bin/bun test test/performfullsync-source-id.test.ts
  • /paperclip/gbrain/bun/bin/bun test test/sync.test.ts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants