Skip to content

feat(sync): port logmind sync from Python (B5b / G4.a) - #135

Merged
thrillmot merged 8 commits into
mainfrom
feat/sync-go-port
Jun 3, 2026
Merged

feat(sync): port logmind sync from Python (B5b / G4.a)#135
thrillmot merged 8 commits into
mainfrom
feat/sync-go-port

Conversation

@thrillmot

Copy link
Copy Markdown
Collaborator

Summary

Implements the canonical Go port of logmind sync — the loop-closer in the
skill suggestion cycle. Reads docs/reviews/PR-<n>.md files (written by
clud-bug-review per SPEC §1.8.1) and updates each cited skill's
.claude/skills/<name>/PROVENANCE.md with cited-by-clud-bug counts and
last-refined dates.

Why this is a greenfield Go port (not a faithful port from Python)

There is no src/logmind/sync.py in the pre-v1.0 Python codebase. The
v0.6.x line closed the loop differently — via .clud-bug.json usage
counters maintained by the clud-bug runner itself. The Go port routes
the same signal through the more durable in-repo docs/reviews/PR-*.md
files, per SPEC §6.5 (local-read; no GitHub API). This is structurally
cleaner — it survives .clud-bug.json schema migrations, works for the
GitHub-App variant of clud-bug (which doesn't edit the on-disk JSON),
and stays grep-friendly.

The implementation honours the existing PROVENANCE.md skeleton in
internal/skill/provenance.go: increments cited-by-clud-bug: <n>,
sets last-refined: "<iso-date>", and appends a new
applied-review-shas: [...] bookkeeping line inside the YAML block so
re-runs deduplicate against previously-applied PR review SHAs.

Files added

  • internal/skill/sync.go (~570 LOC) — parser + writer. Importable by
    future tooling (B5b draft-writer etc.); doesn't drag the cobra tree.
  • internal/skill/sync_test.go (~430 LOC) — 14 unit tests.
  • internal/cli/sync.go (~80 LOC) — cobra wiring for logmind sync [--dry-run].
  • internal/cli/sync_test.go (~170 LOC) — 7 CLI-level tests.

Files modified

  • internal/cli/root.go — registers newSyncCmd() next to skill/init.

Spec coverage

  • SPEC §3.9 logmind sync surface: this PR ships --dry-run. --since
    and --write-drafts are tracked for a follow-on; --update-provenance
    is the default (and currently only) behaviour, so the explicit flag
    would be vacuous.
  • SPEC §6.2 review file format: implemented per the §1.8.1 NORMATIVE
    template — <!-- review-sha: <40 hex> --> is the load-bearing
    idempotency key; **Skills cited:** block parsed as
    - skill-name (N findings) lines.
  • SPEC §6.5 local consumption: no GitHub API call.

Deviations from spec / reference

  • The SPEC §1.11.1 PROVENANCE.md template uses bold-label markdown
    (**Cited by clud-bug:** N times); the existing on-repo template at
    internal/skill/provenance.go uses an embedded YAML block
    (cited-by-clud-bug: <n>). This PR follows the in-repo template
    because (a) it's already committed to main, (b) the YAML form is
    easier to machine-write deterministically, (c) the prompt explicitly
    cited the YAML field names. A reconciliation pass between spec text
    and implementation is tracked separately.
  • applied-review-shas is a Go-side bookkeeping addition not yet in
    the SPEC. It lives inside the YAML block (so it's grep-friendly) and
    is the load-bearing idempotency mechanism — a future SPEC revision
    should formalize it.

Test coverage

internal/skill/sync_test.go (parser + writer):

  • empty repo (no docs/reviews/)
  • single citation happy path
  • multiple citations on the same skill (sum)
  • multiple skills in one PR (parallel update)
  • idempotency (re-run = no-op)
  • idempotency + new review (only new citations land)
  • dry-run (summary correct, disk untouched)
  • malformed review file (missing review-sha → skip with warn)
  • unknown skill cited (warn + continue, no synthetic skill created)
  • non-PR markdown files in docs/reviews/ ignored
  • SHA case-canonicalisation (uppercase + lowercase dedupe against each other)
  • citation regex variations (singular "finding", whitespace tolerance, kebab slugs)
  • zero-findings citation dropped (not signal)
  • bad filename rejected with ErrMalformedReview

internal/cli/sync_test.go (CLI integration):

  • missing reviews dir → "no skills updated" ok-line
  • one PR + one skill → "0 → 4" per-skill diff
  • dry-run marker in stdout, disk untouched
  • unknown skill warning routed to stderr; sync still exits 0
  • docs/ exists but docs/reviews/ doesn't → no-op
  • filesystem error (chmod 000 on reviews dir) → ErrSilent + Error: line
  • sync registered on root command tree

Total: 22 tests, all passing under go test ./... -race -count=1.

Verification transcript

Smoke test against a synthetic repo:

$ logmind sync
1 skill(s) updated · 5 citation(s) added · 1/1 review(s) applied
  - example-skill: 0 → 5 [+aaaaaaa]
ok sync: 1 skill(s) updated · 5 citation(s) added · 1/1 review(s) applied

$ logmind sync  # re-run
no skills updated (1 review file(s) scanned)
ok sync: 0 skill(s) updated · 0 citation(s) added · 0/1 review(s) applied

PROVENANCE.md after first run:

derived-from-decisions: []
cited-by-clud-bug: 5
last-refined: "2026-06-03"
refinement-history: []
applied-review-shas: ["aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"]

Plan tracking

  • Closes G4.a "Loop-closer surface" — logmind sync is now wired.
  • Opens path for B5b --write-drafts flag (separate PR).
  • Tracked separately: reconcile SPEC §1.11.1 template (bold-label
    markdown) with internal/skill/provenance.go template (YAML block).

Test plan

  • go test ./... -race -count=1 passes
  • make build produces a binary with sync registered
  • logmind sync --help prints the new help text
  • Smoke test on a synthetic repo confirms the happy path
  • Re-run with no new reviews is a no-op
  • --dry-run doesn't touch disk
  • Derived docs (docs/file-structure.md, docs/timeline.md) are current

🤖 Generated with Claude Code

thrillmot added 4 commits June 3, 2026 13:19
Worktree CWD basename leaked into the previous logmind log auto-regen.
Regenerated from /Users/ludlow/logmind so the tree root is 'logmind'
and the .benchmarks entry (since-removed) is dropped.
@vercel

vercel Bot commented Jun 3, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
logmind-site Ignored Ignored Preview Jun 3, 2026 6:15pm

Request Review

@claude

claude Bot commented Jun 3, 2026

Copy link
Copy Markdown

Claude finished @thrillmot's task in 6m 19s —— View job


🐛 Clud Bug review

This round: 1 critical · 1 minor · 0 resolved from prior · 0 still open

Found: 1 🔴 / 1 🟡 / 0 🟣

Per-skill scan

  • [critical-issues-only]: 1 critical finding, 1 minor finding below.
  • [evidence-based-review]: all findings anchored to specific file:line with concrete failure modes.
  • [respect-existing-conventions]: no convention fights detected.
  • [clud-bug-collaboration]: not applicable (this is a review pass, not a push).

🔴 [critical-issues-only]: appliedReviewSet updated before write succeeds — ReviewsApplied inflated on write failure (internal/skill/sync.go:384).

Reasoning

appliedReviewSet[s] = struct{}{} at line 384 runs before atomicWriteFile at line 395. When the write fails, the continue at line 397 skips summary.SkillsUpdated++ and summary.CitationsAdded += addedCount, but the SHA is already in appliedReviewSet. The final summary.ReviewsApplied = len(appliedReviewSet) therefore reports a review as applied even though nothing was written to disk. Fix: move lines 381–385 to after the if !opts.DryRun { … } block. Dry-run is unaffected — that block is skipped, not errored, so execution reaches the moved lines normally.

🟡 [critical-issues-only]: ok sync: machine-parseable line lacks dry-run marker, creating a misleading signal for script consumers (internal/cli/sync.go:84).

Reasoning

FormatSummary prefixes (dry-run) on its human-readable line, but the ok sync: line immediately after it is emitted unconditionally without the marker. In --dry-run mode stdout reads ok sync: 1 skill(s) updated … even though no files were touched. A post-merge hook keying off that line would incorrectly conclude the write happened. Fix: fmt.Fprintf(stdout, "%sok sync: …", map[bool]string{true:"(dry-run) ",false:""}[dryRun], …).

Skills referenced: [critical-issues-only, evidence-based-review, respect-existing-conventions, clud-bug-collaboration]

@thrillmot
thrillmot enabled auto-merge (squash) June 3, 2026 17:43
Comment thread internal/skill/sync.go Outdated
Comment thread internal/cli/sync.go Outdated
@github-actions

github-actions Bot commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

🐛 Clud Bug review

This round: 1 critical · 1 minor · 0 resolved from prior · 0 still open

Found: 1 🔴 / 1 🟡 / 0 🟣

Per-skill scan

  • [critical-issues-only]: scanned all paths. 1 critical finding, 1 minor finding below.
  • [evidence-based-review]: applied to all findings. all anchored to file:line with concrete failure cases.
  • [respect-existing-conventions]: scanned for pattern fights. 0 findings.
  • [clud-bug-collaboration]: not applicable — review pass, not a fix-push.

Critical findings

🔴 [critical-issues-only]: appliedReviewSet updated before atomicWriteFile succeeds — ReviewsApplied inflated when write fails (internal/skill/sync.go:384).

Reasoning

appliedReviewSet[s] = struct{}{} at line 384 runs before atomicWriteFile at line 395. When the write errors, continue at line 397 skips SkillsUpdated++ and CitationsAdded += addedCount, but the SHA is already in the set. summary.ReviewsApplied = len(appliedReviewSet) then reports a review as applied even though nothing reached disk. Fix: move lines 381–385 to after the if !opts.DryRun { … } block — dry-run is unaffected because that block is merely skipped (not erroring), so execution reaches the moved code either way.

Minor findings

🟡 [critical-issues-only]: ok sync: machine-parseable line omits dry-run marker, misleading script consumers (internal/cli/sync.go:84).

Reasoning

FormatSummary (line 83) prefixes (dry-run) on its human line but the ok sync: line that follows does not. In --dry-run mode stdout reads ok sync: 1 skill(s) updated … even though no files were written. A post-merge hook or CI step keying off that line would believe the write happened. Fix: conditionally prepend (dry-run) to the ok sync: format string when dryRun == true.

Skills referenced: [critical-issues-only, evidence-based-review, respect-existing-conventions, clud-bug-collaboration]

@claude

claude Bot commented Jun 3, 2026

Copy link
Copy Markdown

Claude finished @thrillmot's task in 4m 16s —— View job


PR Review — feat(sync): port logmind sync from Python (B5b / G4.a)

  • Gather context (PR diff, prior threads, changed files)
  • Check FIX-PUSH FLOW (verify prior findings fixed or still open)
  • Review new code for bugs, security, performance issues
  • Post inline findings
  • Resolve prior threads if addressed
  • Emit structured summary

This synchronize event was a merge-from-main commit only — docs/README.md, docs/file-structure.md, and docs/timeline.md changed; no sync code was touched. Both prior findings still stand.

This round: 0 critical · 0 minor · 0 resolved from prior · 2 still open

Found: 0 🔴 / 0 🟡 / 0 🟣

Per-skill scan

  • [critical-issues-only]: scanned delta (docs-only). 0 new findings. 2 prior issues still open.
  • [evidence-based-review]: not applicable — no new findings to anchor.
  • [respect-existing-conventions]: not applicable — no code changes in delta.
  • [clud-bug-collaboration]: reviewed fix-push flow; confirmed prior threads unresolved.

Still open from prior review

🔴 appliedReviewSet populated before atomicWriteFile succeeds — ReviewsApplied inflated on write failure (internal/skill/sync.go:383–385). Fix: move the SHA-set update to after the if !opts.DryRun { … } block.

🟡 ok sync: machine-parseable line omits dry-run marker (internal/cli/sync.go:84). Fix: conditionally prepend (dry-run) to that fmt.Fprintf call when dryRun == true.

Skills referenced: [critical-issues-only, evidence-based-review, respect-existing-conventions, clud-bug-collaboration]

@github-actions

github-actions Bot commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

🐛 Clud Bug review

This round: 0 critical · 0 minor · 0 resolved from prior · 2 still open

Found: 0 🔴 / 0 🟡 / 0 🟣

Per-skill scan

  • [critical-issues-only]: scanned delta (docs-only). 0 new findings. 2 prior issues still open.
  • [evidence-based-review]: not applicable — no new findings to anchor this pass.
  • [respect-existing-conventions]: not applicable — no code changes in this delta.
  • [clud-bug-collaboration]: fix-push flow completed; both prior threads confirmed unresolved.

Skills referenced: [critical-issues-only, evidence-based-review, respect-existing-conventions, clud-bug-collaboration]

@claude

claude Bot commented Jun 3, 2026

Copy link
Copy Markdown

Claude finished @thrillmot's task in 2m 21s —— View job


PR Review — feat(sync): port logmind sync from Python (B5b / G4.a)

  • Gather context (PR diff, prior threads, changed files)
  • Check FIX-PUSH FLOW — both prior findings fixed in fa70735
  • Review delta for new issues — none found
  • Post inline findings — none to post
  • Prior threads — both already resolved by fix commit
  • Emit structured summary

Both prior findings were addressed in fa70735 ("fix(sync): only count SHA on successful write; mark dry-run on ok line"):

Bug 1 (🔴): appliedReviewSet block correctly moved to after atomicWriteFile succeeds. New test TestSync_WriteFailure_DoesNotCountSHA covers the regression path via a swappable var atomicWriteFile package-level stub.

Bug 2 (🟡): ok sync: line now carries (dry-run) prefix when dryRun == true. Test updated to assert the marker is present and the bare form absent.

No new issues found in the delta.

@github-actions

github-actions Bot commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

🐛 Clud Bug review

This round: 0 critical · 0 minor · 2 resolved from prior · 0 still open

Found: 0 🔴 / 0 🟡 / 0 🟣

Per-skill scan

  • [critical-issues-only]: scanned delta (fix-push). 0 new findings. Both prior issues resolved.
  • [evidence-based-review]: not applicable — no new findings to anchor this pass.
  • [respect-existing-conventions]: not applicable — fix-push delta only; no convention fights detected.
  • [clud-bug-collaboration]: fix-push flow completed; both prior threads confirmed resolved by fa70735.

Skills referenced: [critical-issues-only, evidence-based-review, respect-existing-conventions, clud-bug-collaboration]

# Conflicts:
#	docs/file-structure.md
#	docs/timeline.md
@claude

claude Bot commented Jun 3, 2026

Copy link
Copy Markdown

Claude finished @thrillmot's task in 4m 21s —— View job


PR Review — feat(sync): port logmind sync from Python (B5b / G4.a)

  • Gather context (PR diff, prior threads, changed files)
  • Check FIX-PUSH FLOW — both prior findings confirmed resolved via GraphQL (isResolved: true)
  • Review delta for new issues — merge-from-main only (docs/site); no sync code changes
  • Fresh read of full sync implementation — no new issues found
  • Post inline findings — none to post
  • Emit structured summary

The synchronize event is a merge-from-main (f3eab2e). Both prior findings were addressed in fa70735 and the review threads are confirmed resolved. No new issues found in the complete sync implementation.

This round: 0 critical · 0 minor · 2 resolved from prior · 0 still open

Found: 0 🔴 / 0 🟡 / 0 🟣

Per-skill scan

  • [critical-issues-only]: scanned all 8 files. 0 new findings. Both prior issues resolved.
  • [evidence-based-review]: not applicable — no new findings to anchor this pass.
  • [respect-existing-conventions]: not applicable — fix-push delta only; no convention fights.
  • [clud-bug-collaboration]: fix-push flow completed; both prior threads confirmed resolved by fa70735.

Skills referenced: [critical-issues-only, evidence-based-review, respect-existing-conventions, clud-bug-collaboration]

@github-actions

github-actions Bot commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

🐛 Clud Bug review

This round: 0 critical · 0 minor · 2 resolved from prior · 0 still open

Found: 0 🔴 / 0 🟡 / 0 🟣

Per-skill scan

  • [critical-issues-only]: scanned all 8 files. 0 new findings. Both prior issues confirmed resolved.
  • [evidence-based-review]: not applicable — no new findings to anchor this pass.
  • [respect-existing-conventions]: not applicable — no code changes in this delta.
  • [clud-bug-collaboration]: fix-push flow completed; both prior threads confirmed resolved by fa70735.

Skills referenced: [critical-issues-only, evidence-based-review, respect-existing-conventions, clud-bug-collaboration]

# Conflicts:
#	docs/file-structure.md
#	docs/timeline.md
@claude

claude Bot commented Jun 3, 2026

Copy link
Copy Markdown

Claude finished @thrillmot's task in 4m 18s —— View job


PR Review — feat(sync): port logmind sync from Python (B5b / G4.a)

  • Load tools
  • Find prior reviewed SHA and fetch delta diff
  • Load skills from .claude/skills/
  • Review all changed files for critical issues
  • Check for unresolved prior threads via GraphQL — both confirmed resolved
  • Post inline findings — none to post
  • Emit structured summary

Delta since last review (f3eab2ee5a01e9) is a merge-from-main carrying the skill push port (637c655). No sync code was touched.

Both prior findings were addressed in fa70735 and are confirmed resolved via GraphQL (isResolved: true on both threads). Full re-read of all sync code found no new issues.

This round: 0 critical · 0 minor · 2 resolved from prior · 0 still open

Found: 0 🔴 / 0 🟡 / 0 🟣

Per-skill scan

  • [critical-issues-only]: scanned all 8 files. 0 new findings. Both prior issues confirmed resolved.
  • [evidence-based-review]: not applicable — no new findings to anchor this pass.
  • [respect-existing-conventions]: not applicable — no code changes in sync files this delta.
  • [clud-bug-collaboration]: fix-push flow completed; both prior threads confirmed resolved by fa70735.

Skills referenced: [critical-issues-only, evidence-based-review, respect-existing-conventions, clud-bug-collaboration]

@github-actions

github-actions Bot commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

🐛 Clud Bug review

This round: 0 critical · 0 minor · 2 resolved from prior · 0 still open

Found: 0 🔴 / 0 🟡 / 0 🟣

Per-skill scan

  • [critical-issues-only]: scanned all 8 files. 0 new findings. Both prior issues confirmed resolved.
  • [evidence-based-review]: not applicable — no new findings to anchor this pass.
  • [respect-existing-conventions]: not applicable — no sync code changes in this delta.
  • [clud-bug-collaboration]: fix-push flow completed; both prior threads confirmed resolved by fa70735.

Skills referenced: [critical-issues-only, evidence-based-review, respect-existing-conventions, clud-bug-collaboration]

@thrillmot
thrillmot merged commit 0d77e0e into main Jun 3, 2026
11 checks passed
@thrillmot
thrillmot deleted the feat/sync-go-port branch June 3, 2026 18:21
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.

1 participant