ci: add gofmt enforcement and mint embed sync checks by jhutar · Pull Request #1714 · fullsend-ai/fullsend

jhutar · 2026-05-31T05:48:12Z

Summary

Run gofmt across all Go source files, fixing 18 formatting violations (including the switch role{ regression from feat(appsetup): add retro agent app configuration #828)
Add gofmt pre-commit hook that fails on unformatted Go files
Add lint-mint-embed-sync pre-commit hook that detects drift between internal/mint/ source files and their .embed copies in internal/dispatch/gcf/mintsrc/

Closes #960

Test plan

gofmt -l . returns empty after formatting
pre-commit run go-fmt --all-files passes clean
pre-commit run go-fmt fails when a gofmt violation is introduced
pre-commit run lint-mint-embed-sync --all-files passes clean
pre-commit run lint-mint-embed-sync fails when embed copy diverges from source
go vet ./... passes

🤖 Generated with Claude Code

fullsend-ai-review · 2026-05-31T05:56:00Z

Review

Findings

Medium

[protected-path] .pre-commit-config.yaml — This file is under a protected path requiring human approval. The PR links to issue Add gofmt check to pre-commit hooks or CI lint target #960 and explains the rationale for adding go-fmt and lint-mint-embed-sync hooks. Human approval is always required for protected-path changes regardless of context.

Low

[edge-case] hack/lint-mint-embed-sync:17 — The script only iterates source files (SRC → DST direction), so orphaned .embed files in the destination that no longer have a corresponding source file will not be detected. If a source file is deleted from internal/mint/ or internal/mintcore/, the stale .embed copy persists undetected.
[error-handling] hack/lint-mint-embed-sync:25 — The temp file created by mktemp for the go.mod special case is not cleaned up via a trap. If the script exits unexpectedly between mktemp and rm, the temp file leaks in /tmp. Consider adding trap 'rm -f "$ff"' EXIT or similar.

Info

[scope-creep] hack/lint-mint-embed-sync — The lint-mint-embed-sync pre-commit hook is not mentioned in the authorizing issue Add gofmt check to pre-commit hooks or CI lint target #960, which specifically requested gofmt enforcement. The hook addresses deployment correctness rather than CI formatting enforcement. However, it aligns well with CLAUDE.md's documented requirement that mint source and embed files "must stay in sync," follows the established pattern of custom lint hooks, and the PR title is transparent about including both concerns.
[pattern-inconsistency] hack/lint-mint-embed-sync — The script uses a custom DESYNC: prefix and bare echo for output. Some lint scripts in hack/ use error()/success() helper functions, but not all (lint-workflow-size also uses bare echo), so the inconsistency is not uniform across the codebase.

Previous run

Review

Findings

Medium

[protected-path] .pre-commit-config.yaml — This file is under a protected path requiring human approval. The PR links to issue Add gofmt check to pre-commit hooks or CI lint target #960 and explains the rationale for adding go-fmt and lint-mint-embed-sync hooks. Human approval is always required for protected-path changes regardless of context.

Low

[edge-case] hack/lint-mint-embed-sync:17 — The script only iterates source files (SRC → DST direction), so orphaned .embed files in the destination that no longer have a corresponding source file will not be detected. If a source file is deleted from internal/mint/ or internal/mintcore/, the stale .embed copy persists undetected.
[error-handling] hack/lint-mint-embed-sync:25 — The temp file created by mktemp for the go.mod special case is not cleaned up via a trap. If the script exits unexpectedly between mktemp and rm, the temp file leaks in /tmp. Consider adding trap 'rm -f "$ff"' EXIT or similar.

Info

[scope-creep] hack/lint-mint-embed-sync — The lint-mint-embed-sync pre-commit hook is not mentioned in the authorizing issue Add gofmt check to pre-commit hooks or CI lint target #960, which specifically requested gofmt enforcement. The hook addresses deployment correctness rather than CI formatting enforcement. However, it aligns well with CLAUDE.md's documented requirement that mint source and embed files "must stay in sync," follows the established pattern of custom lint hooks, and the PR title is transparent about including both concerns.
[pattern-inconsistency] hack/lint-mint-embed-sync — The script uses a custom DESYNC: prefix and bare echo for output. Some lint scripts in hack/ use error()/success() helper functions, but not all (lint-workflow-size also uses bare echo), so the inconsistency is not uniform across the codebase.

Previous run (2)

Review

Findings

Medium

[protected-path] .pre-commit-config.yaml — This file is under a protected path requiring human approval. The PR links to issue Add gofmt check to pre-commit hooks or CI lint target #960 and explains the rationale for adding go-fmt and lint-mint-embed-sync hooks. Human approval is always required for protected-path changes regardless of context.

Low

[scope-creep] hack/lint-mint-embed-sync — The lint-mint-embed-sync pre-commit hook is not mentioned in the authorizing issue Add gofmt check to pre-commit hooks or CI lint target #960, which specifically requested gofmt enforcement. The hook addresses deployment correctness rather than CI formatting enforcement. However, it aligns well with CLAUDE.md's documented requirement that mint source and embed files "must stay in sync," follows the established pattern of custom lint hooks, and the PR title is transparent about including both concerns.
[edge-case] hack/lint-mint-embed-sync:17 — The script only iterates source files (SRC → DST direction), so orphaned .embed files in the destination that no longer have a corresponding source file will not be detected. If a source file is deleted from internal/mint/ or internal/mintcore/, the stale .embed copy persists undetected.
[error-handling] hack/lint-mint-embed-sync:25 — The temp file created by mktemp for the go.mod special case is not cleaned up via a trap. If the script exits unexpectedly between mktemp and rm, the temp file leaks in /tmp. Consider adding trap 'rm -f "$ff"' EXIT or similar.

Info

[pattern-inconsistency] hack/lint-mint-embed-sync — The script uses a custom DESYNC: prefix and bare echo for output. Some lint scripts in hack/ use error()/success() helper functions, but not all (lint-workflow-size also uses bare echo), so the inconsistency is not uniform across the codebase.

Previous run (3)

Review

Findings

Medium

[protected-path] .pre-commit-config.yaml — This file is under a protected path requiring human approval. The PR links to issue Add gofmt check to pre-commit hooks or CI lint target #960 and explains the rationale for adding go-fmt and lint-mint-embed-sync hooks. Human approval is always required for protected-path changes regardless of context.

Low

[scope-creep] hack/lint-mint-embed-sync — The lint-mint-embed-sync pre-commit hook is not mentioned in the authorizing issue Add gofmt check to pre-commit hooks or CI lint target #960, which specifically requested gofmt enforcement. The hook addresses deployment correctness rather than CI formatting enforcement. However, it aligns well with CLAUDE.md's documented requirement that mint source and embed files "must stay in sync," follows the established pattern of custom lint hooks, and the PR title is transparent about including both concerns.
[edge-case] hack/lint-mint-embed-sync:17 — The script only iterates source files (SRC → DST direction), so orphaned .embed files in the destination that no longer have a corresponding source file will not be detected. If a source file is deleted from internal/mint/ or internal/mintcore/, the stale .embed copy persists undetected.
[error-handling] hack/lint-mint-embed-sync:25 — The temp file created by mktemp for the go.mod special case is not cleaned up via a trap. If the script exits unexpectedly between mktemp and rm, the temp file leaks in /tmp. Consider adding trap 'rm -f "$ff"' EXIT or similar.

Info

[pattern-inconsistency] hack/lint-mint-embed-sync — The script uses a custom DESYNC: prefix and bare echo for output. Some lint scripts in hack/ use error()/success() helper functions, but not all (lint-workflow-size also uses bare echo), so the inconsistency is not uniform across the codebase.

Previous run (4)

Review

Findings

Medium

[protected-path] .pre-commit-config.yaml — This file is under a protected path requiring human approval. The PR links to issue Add gofmt check to pre-commit hooks or CI lint target #960 and explains the rationale for adding go-fmt and lint-mint-embed-sync hooks. Human approval is always required for protected-path changes regardless of context.
[scope-creep] hack/lint-mint-embed-sync — The lint-mint-embed-sync pre-commit hook is not mentioned in the authorizing issue Add gofmt check to pre-commit hooks or CI lint target #960, which specifically requested gofmt enforcement. The mint/embed sync hook addresses a different problem area (deployment correctness) than the authorized work (CI formatting enforcement). CONTRIBUTING.md advises keeping PRs focused to one problem area. That said, the hook aligns well with CLAUDE.md's documented requirement that mint source and embed files "must stay in sync," and follows the established pattern of adding custom lint hooks for documented requirements.

Low

[edge-case] hack/lint-mint-embed-sync:17 — The script only iterates source files (SRC → DST), so it will not detect orphaned .embed files in the destination that no longer have a corresponding source file. If a source file is deleted from internal/mint/ or internal/mintcore/, the stale .embed copy will persist undetected. Consider adding a reverse loop over DST/*.embed files to check each has a corresponding source.
[pattern-inconsistency] hack/lint-mint-embed-sync — The script uses a custom DESYNC: prefix and bare echo for output instead of the error()/success() helper functions used consistently across other lint scripts in hack/ (lint-adr-status, lint-adr-numbers, lint-broken-symlinks). Consider adding the standard helpers for consistency.

Previous run (5)

Review

Findings

Medium

[protected-path] .pre-commit-config.yaml — This file is under a protected path requiring human approval. The PR links to issue Add gofmt check to pre-commit hooks or CI lint target #960 and explains the rationale for adding go-fmt and lint-mint-embed-sync hooks. Human approval is always required for protected-path changes regardless of context.
[scope-creep] hack/lint-mint-embed-sync — The lint-mint-embed-sync pre-commit hook is not mentioned in the authorizing issue Add gofmt check to pre-commit hooks or CI lint target #960, which specifically requested gofmt enforcement. The mint/embed sync hook addresses a different problem area (deployment correctness) than the authorized work (CI formatting enforcement). CONTRIBUTING.md advises keeping PRs focused to one problem area. That said, the hook aligns well with CLAUDE.md's documented requirement that mint source and embed files "must stay in sync," and follows the established pattern of adding custom lint hooks for documented requirements.

Low

[edge-case] hack/lint-mint-embed-sync:17 — The script only iterates source files (SRC → DST), so it will not detect orphaned .embed files in the destination that no longer have a corresponding source file. If a source file is deleted from internal/mint/ or internal/mintcore/, the stale .embed copy will persist undetected. Consider adding a reverse loop over DST/*.embed files to check each has a corresponding source.
[pattern-inconsistency] hack/lint-mint-embed-sync — The script uses a custom DESYNC: prefix and bare echo for output instead of the error()/success() helper functions used consistently across all other lint scripts in hack/ (lint-adr-status, lint-adr-numbers, lint-broken-symlinks, lint-workflow-size, lint-agent-docs). Consider adding the standard helpers for consistency.

Previous run (6)

Review

Findings

High

[logic-error] hack/lint-mint-embed-sync:16 — Script only iterates over internal/mint/* files but the documented mint/embed sync requirement (CLAUDE.md) also covers internal/mintcore/ files. The existing Go test in provisioner_test.go (lines 1596–1609) checks both internal/mint/ and internal/mintcore/ directories, but this pre-commit hook misses mintcore entirely. A developer modifying e.g. internal/mintcore/handler.go would get a clean pass from this hook despite the embed copy being out of sync.
Remediation: Add a second loop over $REPO_ROOT/internal/mintcore/ that compares each non-test .go file (plus go.mod and go.sum) against $DST/mintcore/$name.embed. Also update the pre-commit files pattern to include ^internal/mintcore/.

Medium

[logic-error] .pre-commit-config.yaml:95 — The files trigger pattern ^(internal/mint/|internal/dispatch/gcf/mintsrc/) does not include ^internal/mintcore/. Even if the script is fixed to check mintcore files, the hook will not trigger when a file in internal/mintcore/ is modified. See also: [logic-error] finding at hack/lint-mint-embed-sync:16.
Remediation: Expand the files pattern to ^(internal/mint/|internal/mintcore/|internal/dispatch/gcf/mintsrc/).
[protected-path] .pre-commit-config.yaml — Protected file modified. The PR links to issue Add gofmt check to pre-commit hooks or CI lint target #960 and the description explains the rationale for adding gofmt and mint embed sync hooks. Human approval is always required for protected-path changes regardless of context.
[scope-creep] .pre-commit-config.yaml — The lint-mint-embed-sync hook was not authorized by issue Add gofmt check to pre-commit hooks or CI lint target #960, which only covers gofmt enforcement. The extension is architecturally justified by CLAUDE.md's mint/embed sync requirement and follows the pattern of existing hack/lint-* scripts, but it constitutes scope beyond the linked issue's authorization.

Low

[resource-leak] hack/lint-mint-embed-sync:24 — mktemp creates a temporary file for the go.mod sed transformation but it is never cleaned up. Add a trap 'rm -f "$ff"' EXIT or use process substitution to avoid creating a temp file.
[naming-convention] hack/lint-mint-embed-sync:30 — Error message uses DESYNC: prefix, diverging from the ERROR:/OK: pattern established by lint-adr-status, lint-adr-numbers, lint-broken-symlinks, and lint-workflow-size which all use error() helper functions.

Info

[gofmt-vs-goimports] .pre-commit-config.yaml:80 — Hook uses gofmt -w which formats code but does not enforce import ordering. The diff includes import reordering changes (e.g., internal/cli/admin.go, internal/cli/postreview_test.go), suggesting goimports was used during development. Consider whether goimports should be the enforced tool to also maintain consistent import grouping.

Previous run (7)

Review

Findings

Medium

[protected-path] .pre-commit-config.yaml — This PR modifies a protected infrastructure file. The change is well-motivated (linked to Add gofmt check to pre-commit hooks or CI lint target #960, adds gofmt enforcement and mint embed sync hooks) and the rationale is clear, but human approval is always required for protected-path changes regardless of context.

Low

[scope-creep] hack/lint-mint-embed-sync — The lint-mint-embed-sync hook is not authorized by issue Add gofmt check to pre-commit hooks or CI lint target #960, which specifically requests gofmt enforcement and fixing the switch role{ violation. The mint embed sync hook enforces a separate CLAUDE.md requirement. The PR title is transparent about including both concerns, and the addition is well-motivated, but it would be cleaner to track this as a separate issue.
[edge-case] hack/lint-mint-embed-sync — The script uses cmp -s which suppresses all output including stderr. If a source or embed file is missing entirely (e.g., a new file added to internal/mint/ without a corresponding .embed copy), the user sees a generic "DESYNC" message rather than a "file not found" error. The exit code is still non-zero (safe behavior), but the error message is imprecise. Consider adding explicit existence checks before cmp, or not suppressing stderr.

Info

[import-organization] internal/cli/postreview_test.go:9 — gofmt sorted stdlib imports (io, testing) after third-party imports (testify/assert, testify/require) because they were in a single ungrouped import block with no blank-line separator. This is correct gofmt behavior (alphabetical within a block), but not idiomatic Go style (stdlib and third-party are conventionally separated). A follow-up with goimports would restore conventional grouping across the codebase.

Previous run (8)

Review

Findings

Medium

[protected-path] .pre-commit-config.yaml — This PR modifies a protected infrastructure file. The change is well-motivated (linked to Add gofmt check to pre-commit hooks or CI lint target #960, adds gofmt enforcement and mint embed sync hooks) and the rationale is clear, but human approval is always required for protected-path changes regardless of context.

Low

[scope-creep] hack/lint-mint-embed-sync — The lint-mint-embed-sync hook is not authorized by issue Add gofmt check to pre-commit hooks or CI lint target #960, which specifically requests gofmt enforcement and fixing the switch role{ violation. The mint embed sync hook enforces a separate CLAUDE.md requirement. The PR title is transparent about including both concerns, and the addition is well-motivated, but it would be cleaner to track this as a separate issue.
[edge-case] hack/lint-mint-embed-sync — The script uses cmp -s which suppresses all output including stderr. If a source or embed file is missing entirely (e.g., a new file added to internal/mint/ without a corresponding .embed copy), the user sees a generic "DESYNC" message rather than a "file not found" error. The exit code is still non-zero (safe behavior), but the error message is imprecise. Consider adding explicit existence checks before cmp, or not suppressing stderr.

Info

[import-organization] internal/cli/postreview_test.go:4 — gofmt sorted stdlib imports (io, testing) after third-party imports (testify/assert, testify/require) because they were in a single ungrouped import block with no blank-line separator. This is correct gofmt behavior (alphabetical within a block), but not idiomatic Go style (stdlib and third-party are conventionally separated). A follow-up with goimports would restore conventional grouping across the codebase.

Previous run (9)

Review

Findings

Medium

[protected-path] .pre-commit-config.yaml — This PR modifies a protected infrastructure file. The change is well-motivated (linked to Add gofmt check to pre-commit hooks or CI lint target #960, adds gofmt enforcement and mint embed sync hooks) and the rationale is clear, but human approval is always required for protected-path changes regardless of context.

Low

[scope-creep] hack/lint-mint-embed-sync — The lint-mint-embed-sync hook is not authorized by issue Add gofmt check to pre-commit hooks or CI lint target #960, which specifically requests gofmt enforcement and fixing the switch role{ violation. The mint embed sync hook enforces a separate CLAUDE.md requirement. The PR title is transparent about including both concerns, and the addition is well-motivated, but it would be cleaner to track this as a separate issue.
[edge-case] hack/lint-mint-embed-sync — The script uses cmp -s which suppresses all output including stderr. If a source or embed file is missing entirely (e.g., internal/mint/go.sum), the user sees a generic "DESYNC" message rather than a "file not found" error. The exit code is still non-zero (safe behavior), but the error message is imprecise. Consider adding explicit existence checks or not suppressing stderr.

Info

[import-organization] internal/cli/postreview_test.go:4 — gofmt sorted stdlib imports (io, testing) after third-party imports (testify/assert, testify/require) because they were in a single ungrouped import block with no blank-line separator. This is correct gofmt behavior (alphabetical within a block), but not idiomatic Go style (stdlib and third-party are conventionally separated). A follow-up with goimports would restore conventional grouping across the codebase.

Previous run (10)

Review

Findings

Medium

[protected-path] .pre-commit-config.yaml — This PR modifies a protected infrastructure file. The change is well-motivated (linked to Add gofmt check to pre-commit hooks or CI lint target #960, adds gofmt enforcement and mint embed sync hooks) and the rationale is clear, but human approval is always required for protected-path changes regardless of context.

Low

[scope-creep] hack/lint-mint-embed-sync — The lint-mint-embed-sync hook is not authorized by issue Add gofmt check to pre-commit hooks or CI lint target #960, which specifically requests gofmt enforcement and fixing the switch role{ violation. The mint embed sync hook enforces a separate CLAUDE.md requirement. The PR title is transparent about including both concerns, and the addition is well-motivated, but it would be cleaner to track this as a separate issue.
[edge-case] hack/lint-mint-embed-sync — The script suppresses stderr from diff -q, so if a source or embed file is missing entirely, the user sees a generic "DESYNC" message rather than a "file not found" error. The exit code is still non-zero (safe behavior), but the error message is imprecise. Consider adding explicit existence checks or not suppressing stderr.

Previous run (11)

Review

Findings

Medium

[protected-path] .pre-commit-config.yaml — This PR modifies a protected governance file. The change is well-justified (closes Add gofmt check to pre-commit hooks or CI lint target #960, adds gofmt and mint-embed-sync enforcement), but human approval is always required for protected-path changes regardless of context.

Info

[style] internal/cli/postreview_test.go:7 — gofmt sorted stdlib imports (io, testing) into the same block as third-party imports (testify/assert, testify/require) because they were in a single ungrouped import block. This is valid gofmt output but not idiomatic Go style (stdlib and third-party are conventionally separated by a blank line). A follow-up with goimports would restore conventional grouping across the codebase.

github-actions · 2026-06-01T09:58:30Z

Site preview

Preview: https://728c13bb-site.fullsend-ai.workers.dev

Commit: 7f6d68bae192af63a50c289489454cd73f3ed73e

ifireball · 2026-06-02T14:47:37Z

+
+rc=0
+for f in main.go go.mod go.sum; do
+    if ! diff -q "$SRC/$f" "$DST/$f.embed" >/dev/null 2>&1; then


can use cmp here which is a little faster.

Hello! Thank you, doing that.

ifireball · 2026-06-02T14:52:42Z

+DST="$REPO_ROOT/internal/dispatch/gcf/mintsrc"
+
+rc=0
+for f in main.go go.mod go.sum; do


I thing its dangarous to assume here that no additional files would be added to the mint codebase, we already have PRs in flight that add files

Hello Barak. Does it mean we should require anything in internal/mint/ that is not "*_test.go" needs to be mirrored to internal/dispatch/gcf/mintsrc/?

Hello Barak. Does it mean we should require anything in internal/mint/ that is not "*_test.go" needs to be mirrored to internal/dispatch/gcf/mintsrc/?

probably, yes.

Thank you. Did that. Please take a look again.

rh-hemartin

I agree with @ifireball comments, but I'm approving as my concerns were covered and he still have @ifireball request for changes.

jhutar · 2026-06-04T15:05:01Z

Rebased due to conflicts.

jhutar · 2026-06-05T04:48:22Z

Rebased due to conflicts and added workaround for eecac65.

fullsend-ai-review

See the review comment for full details.

fullsend-ai-review · 2026-06-12T05:52:40Z

🤖 Finished Review · ✅ Success · Started 5:52 AM UTC · Completed 6:00 AM UTC
Commit: cf46b6e · View workflow run →

ralphbean

LGTM.

Enforce gofmt formatting via pre-commit so violations fail before commit. Placed before go-vet in the local hooks section. Ref fullsend-ai#960 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Generated-by: Claude rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED Signed-off-by: Jan Hutar <jhutar@redhat.com>

Let pre-commit pass staged Go files to gofmt instead of scanning the entire tree. Uses gofmt -w to auto-fix formatting in place. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Generated-by: Claude rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED Signed-off-by: Jan Hutar <jhutar@redhat.com>

Add lint-mint-embed-sync hook that verifies main.go, go.mod, and go.sum in internal/mint/ match their .embed copies in internal/dispatch/gcf/mintsrc/. Runs only when either directory is touched. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Generated-by: Claude rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED Signed-off-by: Jan Hutar <jhutar@redhat.com>

cmp -s is faster for byte-level equality checks where we don't need the actual diff output. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Generated-by: Claude rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED Signed-off-by: Jan Hutar <jhutar@redhat.com>

Instead of hardcoding the three files to check, glob all non-test files in internal/mint/ so newly added files are automatically covered. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Generated-by: Claude rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED Signed-off-by: Jan Hutar <jhutar@redhat.com>

Since commit eecac65 there is intentional difference between internal/mint/go.mod and internal/dispatch/gcf/mintsrc/go.mod.embed with replace directive. rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED Signed-off-by: Jan Hutar <jhutar@redhat.com>

rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED Signed-off-by: Jan Hutar <jhutar@redhat.com>

jhutar · 2026-06-13T12:01:09Z

Rebased due to conflict

fullsend-ai-review · 2026-06-13T12:04:18Z

🤖 Finished Review · ✅ Success · Started 12:04 PM UTC · Completed 12:13 PM UTC
Commit: 7f6d68b · View workflow run →

rh-hemartin · 2026-06-15T06:04:19Z

@jhutar merge it and let's be done with this, it is a pain for you to be rebasing every time 🤣 We can address any concern Barak has after the merge.

codecov · 2026-06-15T06:07:52Z

Codecov Report

❌ Patch coverage is 84.61538% with 2 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
internal/cli/bootstrap_input.go	50.00%	2 Missing ⚠️

📢 Thoughts on this report? Let us know!

jhutar · 2026-06-15T07:50:39Z

@jhutar merge it and let's be done with this, it is a pain for you to be rebasing every time 🤣 We can address any concern Barak has after the merge.

Hello Hector! Thank you. I do not have permissions to merge. Could you please do it?

Not relevant.

fullsend-ai-retro · 2026-06-15T08:05:27Z

🤖 Finished Retro · ✅ Success · Started 8:05 AM UTC · Completed 8:11 AM UTC
Commit: 7f6d68b · View workflow run →

fullsend-ai-retro · 2026-06-15T08:11:23Z

Retro: PR #1714 — `ci: add gofmt enforcement and mint embed sync checks`

Timeline

This PR added gofmt pre-commit hooks and a mint embed drift-detection script. It was open for 15 days (May 31 – June 15) with 8 commits, 11 force pushes (mostly rebases), and 12 review bot runs.

May 31–Jun 2: PR opened, human reviewers (rh-hemartin, ifireball) provided actionable feedback on hook design (use native pre-commit filtering, use cmp instead of diff, dynamically discover files).
Jun 3–5: Author addressed all feedback. Review bot caught a genuine high-severity logic error (script missed internal/mintcore/ sync) and a medium missing trigger pattern — both were fixed.
Jun 5–12: Multiple rebases to resolve conflicts with main. Each rebase triggered a new review bot run. ifireball never formally re-reviewed. ralphbean approved with "LGTM" on Jun 12.
Jun 15: Merged after rh-hemartin explicitly said to merge and address remaining concerns post-merge.

Assessment

Review quality was good. The review bot's high-severity finding about missing mintcore/ coverage was genuinely valuable and was fixed. Human reviewers provided solid design feedback (native pre-commit filtering, cmp vs diff, dynamic discovery).

The main waste was 12 review runs for ~2-3 runs' worth of meaningful changes. Most runs were triggered by rebase-only force pushes that didn't change the actual diff. This is a well-known pattern with extensive existing issue coverage:

#1287 — Skip or diff-gate re-review when PR changes are rebase-only
#1356 — Skip re-review when PR diff is unchanged after rebase/reopen
#1422 — Deduplicate review runs on rapid successive rebases
#1552 — Pass prior findings to follow-up reviews for targeted verification
#1014 — Debounce review dispatch on rapid synchronize events

The 15-day cycle time was driven primarily by human reviewer availability (ifireball's review was never formally resolved) and rebase churn from a busy main branch — not agent issues.

Proposals

No new proposals filed. The primary improvement opportunity (reducing redundant review runs on rebase-only pushes) is already thoroughly covered by at least 6 open issues (#1287, #1356, #1422, #1014, #766, #963). Prioritizing and implementing those existing issues — particularly #1287 and #1356 which are the most directly applicable — would have prevented ~9 of the 12 review runs on this PR.

jhutar · 2026-06-15T08:32:54Z

Thank you all!

jhutar force-pushed the 1-gofmt branch from 13072f9 to 4349def Compare May 31, 2026 05:51

fullsend-ai-review Bot added the requires-manual-review Review requires human judgment label May 31, 2026

rh-hemartin requested changes Jun 1, 2026

View reviewed changes

Comment thread .pre-commit-config.yaml Outdated

Comment thread .pre-commit-config.yaml Outdated

rh-hemartin assigned jhutar Jun 1, 2026

github-actions Bot deployed to site-preview June 1, 2026 09:58 View deployment

jhutar force-pushed the 1-gofmt branch from 4349def to 2ab81fe Compare June 2, 2026 07:22

fullsend-ai-review Bot added requires-manual-review Review requires human judgment and removed requires-manual-review Review requires human judgment labels Jun 2, 2026

github-actions Bot deployed to site-preview June 2, 2026 14:48 View deployment

ifireball previously requested changes Jun 2, 2026

View reviewed changes

rh-hemartin approved these changes Jun 3, 2026

View reviewed changes

jhutar force-pushed the 1-gofmt branch from 6827140 to fd2eba2 Compare June 3, 2026 06:52

fullsend-ai-review Bot added requires-manual-review Review requires human judgment and removed requires-manual-review Review requires human judgment labels Jun 3, 2026

jhutar force-pushed the 1-gofmt branch from fd2eba2 to 7a6b4f9 Compare June 4, 2026 10:40

fullsend-ai-review Bot added requires-manual-review Review requires human judgment and removed requires-manual-review Review requires human judgment labels Jun 4, 2026

jhutar force-pushed the 1-gofmt branch 2 times, most recently from fa7a184 to 4ce766e Compare June 4, 2026 15:04

fullsend-ai-review Bot added requires-manual-review Review requires human judgment and removed requires-manual-review Review requires human judgment labels Jun 4, 2026

jhutar force-pushed the 1-gofmt branch 2 times, most recently from 4ade97e to a4e44e2 Compare June 5, 2026 04:46

fullsend-ai-review Bot previously requested changes Jun 5, 2026

View reviewed changes

Comment thread hack/lint-mint-embed-sync

Comment thread .pre-commit-config.yaml

Comment thread hack/lint-mint-embed-sync

Comment thread hack/lint-mint-embed-sync

Comment thread .pre-commit-config.yaml

fullsend-ai-review Bot removed the requires-manual-review Review requires human judgment label Jun 5, 2026

jhutar force-pushed the 1-gofmt branch from a4e44e2 to a684e9c Compare June 5, 2026 07:16

fullsend-ai-review Bot added requires-manual-review Review requires human judgment and removed requires-manual-review Review requires human judgment labels Jun 12, 2026

ralphbean approved these changes Jun 12, 2026

View reviewed changes

jhutar added 8 commits June 13, 2026 13:54

feat: Expand hack/lint-mint-embed-sync to cover additional dir

69dab8e

rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED Signed-off-by: Jan Hutar <jhutar@redhat.com>

style: apply gofmt to all Go source files

7f6d68b

rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED Signed-off-by: Jan Hutar <jhutar@redhat.com>

jhutar force-pushed the 1-gofmt branch from cf46b6e to 7f6d68b Compare June 13, 2026 12:00

fullsend-ai-review Bot added requires-manual-review Review requires human judgment and removed requires-manual-review Review requires human judgment labels Jun 13, 2026

github-actions Bot deployed to site-preview June 15, 2026 06:05 View deployment

rh-hemartin added this pull request to the merge queue Jun 15, 2026

Merged via the queue into fullsend-ai:main with commit 9ca6edc Jun 15, 2026
12 checks passed

jhutar deleted the 1-gofmt branch June 15, 2026 08:31

Uh oh!

Conversation

jhutar commented May 31, 2026

Summary

Test plan

Uh oh!

fullsend-ai-review Bot commented May 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review

Findings

Medium

Low

Info

Review

Findings

Medium

Low

Info

Review

Findings

Medium

Low

Info

Review

Findings

Medium

Low

Info

Review

Findings

Medium

Low

Review

Findings

Medium

Low

Review

Findings

High

Medium

Low

Info

Review

Findings

Medium

Low

Info

Review

Findings

Medium

Low

Info

Review

Findings

Medium

Low

Info

Review

Findings

Medium

Low

Review

Findings

Medium

Info

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Jun 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Site preview

Uh oh!

ifireball Jun 2, 2026

Choose a reason for hiding this comment

Uh oh!

jhutar Jun 3, 2026

Choose a reason for hiding this comment

Uh oh!

ifireball Jun 2, 2026

Choose a reason for hiding this comment

fullsend-ai-review Bot commented May 31, 2026 •

edited

Loading

github-actions Bot commented Jun 1, 2026 •

edited

Loading

jhutar commented Jun 4, 2026 •

edited

Loading

fullsend-ai-review Bot commented Jun 12, 2026 •

edited

Loading

fullsend-ai-review Bot commented Jun 13, 2026 •

edited

Loading

fullsend-ai-retro Bot commented Jun 15, 2026 •

edited

Loading

Retro: PR #1714 — `ci: add gofmt enforcement and mint embed sync checks`