CLI cleanup: production-ready file-mode workflow#167
Merged
jonathanpopham merged 6 commits intosupermodeltools:mainfrom Apr 30, 2026
Merged
CLI cleanup: production-ready file-mode workflow#167jonathanpopham merged 6 commits intosupermodeltools:mainfrom
jonathanpopham merged 6 commits intosupermodeltools:mainfrom
Conversation
WalkthroughAdds repo-content fingerprinting for shard-cache validation, removes legacy three-file shard mode/flag in favor of single Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Daemon
participant Repo
participant Cache as "Shard Cache (on-disk)"
participant Analyzer as "Analyze API"
User->>Daemon: start
Daemon->>Repo: compute RepoFingerprint (git status + contents)
Daemon->>Cache: read cached ShardIR
Cache-->>Daemon: return cached ShardIR (may include fingerprint)
Daemon->>Daemon: compare cached fingerprint vs current
alt fingerprints match
Daemon->>User: use cached ShardIR
else fingerprints differ / missing
Daemon->>Analyzer: request analysis (generate ShardIR)
Analyzer-->>Daemon: return ShardIR
Daemon->>Daemon: embed current fingerprint into ShardIR
Daemon->>Cache: save ShardIR with fingerprint
Daemon->>User: use regenerated ShardIR
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
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)
Review rate limit: 3/5 reviews remaining, refill in 22 minutes and 26 seconds. 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/analyze.go (1)
44-55:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't silently ignore a still-accepted
--three-fileflag.
analyzestill parses--three-file, but this call no longer passes it intoshards.Generate. So a script that expectsfoo.calls.go/foo.deps.go/foo.impact.gowill now succeed and quietly emit onlyfoo.graph.go. Either keep plumbingThreeFile: threeFilethrough until the flag is fully removed, or fail fast when the flag is set.Possible fail-fast fix
import ( + "fmt" "github.com/spf13/cobra" @@ RunE: func(cmd *cobra.Command, args []string) error { @@ + if threeFile { + return fmt.Errorf("--three-file is no longer supported; use the default .graph.* output") + } if cfg.ShardsEnabled() && !noShards { // Shard mode: Generate handles the full pipeline (API call + // cache + shards) in a single upload. Running analyze.Run // first would duplicate the API call. return shards.Generate(cmd.Context(), cfg, dir, shards.GenerateOptions{Force: opts.Force}) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/analyze.go` around lines 44 - 55, The analyze command parses the deprecated threeFile flag but then drops it when calling shards.Generate, causing silent behavior change; update the command handler to either propagate the flag into shards.GenerateOptions (add ThreeFile: threeFile alongside Force) so shards.Generate receives the intent, or validate early and return an error if threeFile is true; locate the call to shards.Generate(cmd.Context(), cfg, dir, shards.GenerateOptions{Force: opts.Force}) and the threeFile flag declaration and implement one of the two fixes (prefer passing ThreeFile: threeFile into shards.GenerateOptions if backward compatibility is desired, otherwise return a descriptive error when threeFile is set).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/cache/fingerprint.go`:
- Around line 152-163: The ignoreFingerprintPath function in
internal/cache/fingerprint.go has a narrower exclusion list than the upload
exclusions in internal/shards/zip.go, causing irrelevant build-output changes
(e.g. .next, .cache, .venv, __snapshots__, .terraform) to affect fingerprints;
update ignoreFingerprintPath to either reuse a shared exclusion helper
(preferred) or expand its switch to include the same set of directories used by
internal/shards/zip.go so both fingerprinting and upload use the identical
exclusion rules, and keep isGeneratedShardPath behavior intact.
- Around line 26-47: The git output parsing currently splits on newlines which
breaks for paths with spaces/quotes; change the git invocations in fingerprint
code to use NUL-delimited output (add -z to git status --porcelain and to git
ls-files --others --exclude-standard) and update the parsing in
filterFingerprintStatus, hashDirtyFiles, and hashUntrackedFiles to treat the
results as NUL-separated raw paths (no unquoting) so filenames are looked up and
hashed correctly; ensure functions consume NULs, handle empty terminator, and
preserve existing logic for filtering generated files and hashing file contents.
In `@internal/restore/local.go`:
- Around line 352-356: The loop that detects file extensions (using ext :=
strings.ToLower(filepath.Ext(path)), extToLanguage, extCounts and total) must
skip Supermodel shard files the same way internal/shards/zip.go does; add the
same shard-file exclusion check (e.g., detect shard suffixes like ".graph." in
the filename or call the existing helper used by zip.go) immediately after
computing path/ext and before incrementing extCounts[ext] and total so files
like pkg/foo.graph.go are ignored.
In `@internal/shards/handler.go`:
- Around line 233-241: The helper shardCacheMatchesFingerprint currently returns
true when fingerprint == "", causing missing fingerprints to be treated as cache
hits; change the logic to fail closed by removing the fingerprint == ""
early-return and instead require a non-empty fingerprint match: if fingerprint
is empty return false, keep the existing nil checks for ir and ir.Summary, and
compare got := ir.Summary[shardCacheFingerprintKey].(string) to fingerprint;
reference shardCacheMatchesFingerprint, shardCacheFingerprintKey, ir.Summary,
and ensure Generate/daemon startup that might produce empty fingerprints will
trigger regeneration rather than a cache hit.
---
Outside diff comments:
In `@cmd/analyze.go`:
- Around line 44-55: The analyze command parses the deprecated threeFile flag
but then drops it when calling shards.Generate, causing silent behavior change;
update the command handler to either propagate the flag into
shards.GenerateOptions (add ThreeFile: threeFile alongside Force) so
shards.Generate receives the intent, or validate early and return an error if
threeFile is true; locate the call to shards.Generate(cmd.Context(), cfg, dir,
shards.GenerateOptions{Force: opts.Force}) and the threeFile flag declaration
and implement one of the two fixes (prefer passing ThreeFile: threeFile into
shards.GenerateOptions if backward compatibility is desired, otherwise return a
descriptive error when threeFile is set).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 75dd1b0f-fa43-4905-9382-9e89650391d0
📒 Files selected for processing (15)
README.mdcmd/analyze.gocmd/hook.gocmd/root.gointernal/cache/fingerprint.gointernal/cache/fingerprint_test.gointernal/restore/local.gointernal/restore/render.gointernal/restore/restore_test.gointernal/shards/daemon.gointernal/shards/daemon_test.gointernal/shards/handler.gointernal/shards/zip.gointernal/shards/zip_test.gonpm/README.md
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
internal/cache/fingerprint.go (2)
26-30:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse NUL-delimited git output end to end.
This still parses
git status/git ls-filesas newline-separated text. Paths with spaces, quotes, backslashes, or newlines can be normalized to the wrong filename here, which means the fingerprint can stay stale even after repo contents changed. Please switch both commands to-zand carry raw NUL-separated paths throughfilterFingerprintStatus,hashDirtyFiles, andhashUntrackedFiles, with a regression test for a quoted or space-containing filename.#!/bin/bash set -euo pipefail tmp="$(mktemp -d)" trap 'rm -rf "$tmp"' EXIT cd "$tmp" git init -q git config user.email test@example.com git config user.name test printf 'package main\n' > main.go git add main.go git commit -qm init printf 'package main\n' > 'foo bar.go' printf 'package main\n' > 'quote"file.go' echo "porcelain without -z:" git status --porcelain --untracked-files=all | cat -vet echo echo "porcelain with -z (raw bytes shown via Python repr):" git status --porcelain -z --untracked-files=all | python - <<'PY' import sys print(repr(sys.stdin.buffer.read())) PYAlso applies to: 50-56, 88-95, 124-149
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cache/fingerprint.go` around lines 26 - 30, Change the git invocations that produce path lists (the gitOutput calls that run "status --porcelain --untracked-files=all" and the "ls-files" ones) to use the -z flag so they emit NUL-delimited output, and propagate raw NUL-separated bytes through filterFingerprintStatus, hashDirtyFiles, and hashUntrackedFiles (update their signatures to accept and return NUL-delimited data rather than splitting on '\n'); ensure filtering and hashing logic treats NUL as the separator and does not perform any newline-based normalization, and add a regression test that creates filenames with spaces and quotes to verify the fingerprint changes correctly when those files are modified/untracked.
152-165:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep fingerprint ignores in lockstep with upload ignores.
This ignore list is still narrower than the upload-side filter, so build-only paths can churn the fingerprint even though they never get uploaded. That turns into avoidable cache misses and misleading stale-cache messaging in
internal/shards/daemon.goandinternal/shards/handler.go. Please centralize the exclusions or reuse the upload helper from the shard path.#!/bin/bash set -euo pipefail rg -n 'docs-output|node_modules|vendor|dist|build|coverage|\.supermodel|\.next|\.cache|\.venv|__snapshots__|\.terraform' \ internal/cache/fingerprint.go internal/shards/zip.go🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cache/fingerprint.go` around lines 152 - 165, The ignoreFingerprintPath function's exclusion list is narrower than the upload-side filter and causes build-only paths to affect fingerprints; update ignoreFingerprintPath to reuse the centralized upload exclusion logic used by the shard upload/filter code instead of duplicating patterns, or import and call that helper from the shards package so the same set of patterns (including .next, .cache, .venv, __snapshots__, .terraform, etc.) are honored; modify ignoreFingerprintPath (and keep calls to isGeneratedShardPath and isSensitiveFingerprintPath) to delegate to the shared upload-ignore helper so both upload and fingerprinting stay in lockstep.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/cache/fingerprint.go`:
- Around line 55-56: filterFingerprintStatus currently calls statusPath(line)
which returns only the right-hand side of rename/copy entries, causing renames
to be dropped when the destination is ignored; instead, update
filterFingerprintStatus to detect rename/copy entries (parse the status line for
the "old -> new" pattern or otherwise obtain both sides), then evaluate
ignoreFingerprintPath on both the source and destination paths and keep the
entry if either side is uploadable; for one-sided matches treat the record as a
delete/add transition rather than dropping it. Apply the same change to the
other occurrence noted (lines ~131-149) and reference statusPath and
ignoreFingerprintPath in your changes.
---
Duplicate comments:
In `@internal/cache/fingerprint.go`:
- Around line 26-30: Change the git invocations that produce path lists (the
gitOutput calls that run "status --porcelain --untracked-files=all" and the
"ls-files" ones) to use the -z flag so they emit NUL-delimited output, and
propagate raw NUL-separated bytes through filterFingerprintStatus,
hashDirtyFiles, and hashUntrackedFiles (update their signatures to accept and
return NUL-delimited data rather than splitting on '\n'); ensure filtering and
hashing logic treats NUL as the separator and does not perform any newline-based
normalization, and add a regression test that creates filenames with spaces and
quotes to verify the fingerprint changes correctly when those files are
modified/untracked.
- Around line 152-165: The ignoreFingerprintPath function's exclusion list is
narrower than the upload-side filter and causes build-only paths to affect
fingerprints; update ignoreFingerprintPath to reuse the centralized upload
exclusion logic used by the shard upload/filter code instead of duplicating
patterns, or import and call that helper from the shards package so the same set
of patterns (including .next, .cache, .venv, __snapshots__, .terraform, etc.)
are honored; modify ignoreFingerprintPath (and keep calls to
isGeneratedShardPath and isSensitiveFingerprintPath) to delegate to the shared
upload-ignore helper so both upload and fingerprinting stay in lockstep.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3dede396-bcaf-4498-b270-3dde460eb0cc
📒 Files selected for processing (3)
cmd/login.gointernal/cache/fingerprint.gointernal/cache/fingerprint_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/shards/handler.go (1)
88-88:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing fingerprint still fails open as a cache hit.
Line 225 returning
truefor empty fingerprints means stale cache can be reused when fingerprinting fails (Line 88 currently ignores the error).Suggested fix
func shardCacheMatchesFingerprint(ir *api.ShardIR, fingerprint string) bool { if fingerprint == "" { - return true + return false } if ir == nil || ir.Summary == nil { return false } got, _ := ir.Summary[shardCacheFingerprintKey].(string) return got == fingerprint }Also applies to: 224-227
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/shards/handler.go` at line 88, The code ignores errors from repocache.RepoFingerprint and treats empty/errored fingerprints as cache hits; change calls to repocache.RepoFingerprint (where fingerprint, _ := repocache.RepoFingerprint(repoDir) is used) to check the returned error and treat any error or empty fingerprint as a cache miss. Update the fingerprint comparison logic (the routine that currently returns true for empty fingerprints around the same area as the prior check) so it returns false when either fingerprint is empty or an error occurred, ensuring stale cache is not reused; apply this same error-and-empty-fingerprint handling to the other occurrences referenced (the block currently returning true for empty fingerprints).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/shards/render.go`:
- Around line 255-266: The stale-cleanup function is performing real deletions
during dry-run; change removeStaleSplitShards to accept a dryRun bool
(removeStaleSplitShards(repoDir, srcFile string, dryRun bool)) and make it
return immediately or skip calling safeRemove when dryRun is true, then update
the site that invokes removeStaleSplitShards to pass the existing dryRun flag
(so callers no longer perform deletions in dry-run); leave safeRemove behavior
unchanged so production deletions still occur when dryRun is false.
---
Duplicate comments:
In `@internal/shards/handler.go`:
- Line 88: The code ignores errors from repocache.RepoFingerprint and treats
empty/errored fingerprints as cache hits; change calls to
repocache.RepoFingerprint (where fingerprint, _ :=
repocache.RepoFingerprint(repoDir) is used) to check the returned error and
treat any error or empty fingerprint as a cache miss. Update the fingerprint
comparison logic (the routine that currently returns true for empty fingerprints
around the same area as the prior check) so it returns false when either
fingerprint is empty or an error occurred, ensuring stale cache is not reused;
apply this same error-and-empty-fingerprint handling to the other occurrences
referenced (the block currently returning true for empty fingerprints).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f328bf12-044d-4cc4-8e63-a802c10ff88c
📒 Files selected for processing (5)
cmd/analyze.gointernal/shards/handler.gointernal/shards/render.gointernal/shards/render_stale_test.gointernal/shards/zip.go
✅ Files skipped from review due to trivial changes (1)
- internal/shards/zip.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/cache/fingerprint.go (1)
204-216: 💤 Low valueConsider using defer for file cleanup (optional).
The current code works correctly - you close on error (line 209) and success (line 212). But
deferwould be slightly cleaner and harder to mess up in future refactors:f, err := os.Open(full) if err != nil { return err } defer f.Close() if _, err := io.Copy(h, f); err != nil { return err } fmt.Fprint(h, "\x00") return nilNot a big deal though - your current approach is fine.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cache/fingerprint.go` around lines 204 - 216, Replace the manual close calls in the function that opens files (the block that uses f, err := os.Open(full) and io.Copy(h, f)) with a deferred close: after successfully opening the file, call defer f.Close() so the file is always closed on return, then remove the explicit f.Close() calls before returns; keep error handling for os.Open and io.Copy(h, f) unchanged and ensure fmt.Fprint(h, "\x00") still runs before the function returns.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/cache/fingerprint.go`:
- Around line 204-216: Replace the manual close calls in the function that opens
files (the block that uses f, err := os.Open(full) and io.Copy(h, f)) with a
deferred close: after successfully opening the file, call defer f.Close() so the
file is always closed on return, then remove the explicit f.Close() calls before
returns; keep error handling for os.Open and io.Copy(h, f) unchanged and ensure
fmt.Fprint(h, "\x00") still runs before the function returns.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9536d9c4-7b32-4a58-b3ab-587934cd2da4
📒 Files selected for processing (9)
internal/cache/fingerprint.gointernal/cache/fingerprint_test.gointernal/restore/local.gointernal/restore/restore_test.gointernal/shards/daemon.gointernal/shards/daemon_test.gointernal/shards/handler.gointernal/shards/render.gointernal/shards/render_stale_test.go
✅ Files skipped from review due to trivial changes (1)
- internal/shards/daemon.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/shards/render.go
- internal/shards/handler.go
- internal/cache/fingerprint_test.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Production-readiness cleanup for the local file-mode CLI workflow from #166.
Implemented:
supermodel watch/watch [path]references with the baresupermodeldaemon command.analyze --three-fileentirely; passing it now fails as an unknown flag. Legacy.calls/.deps/.impactcleanup remains so old split shards are removed during normal.graph.*rendering.supermodelCtrl-C handling so the daemon receives cancellation and removes generated shard files.restore --localroot scanning, source-file counting, generated shard exclusion,docs-outputexclusion, and misleading0 functionsoutput.docs-outputfrom analysis uploads and local restore scans.login --token ""and avoided fingerprinting hidden directories / sensitive hard-blocked files that are not uploaded.updatewith a temporary older binary self-replacing to the latest release.Out of scope for this pass:
compactcleanup beyond surface verification.Refs #166.
Command Surface Verification
Local binary built from this branch and tested against a temp repo, temp HOME, and local mock Supermodel API.
Happy paths covered:
supermodel --help,help analyze,completion,version,skill,status,update --check.login --token,logout, authed and unauthenticatedstatus.analyze, cachedanalyze, removed--three-fileexpected-failure check,clean --dry-run,clean, baresupermodeldaemon + Ctrl-C cleanup,hook.analyze --no-shards,graph,find,focus,dead-code,blast-radius,audit,share,restore,docs,mcpinitialize/tools-list.restore --local,compactfile and dry-run directory mode.Reverse paths covered:
analyze,graph,find,focus,dead-code,blast-radius,audit,share,docs, andmcp.focus,find,restore --local,clean, root-dir guard foranalyze, missing diff forblast-radius, unsupported file forcompact, missinglogin --tokenvalue, and removedanalyze --three-file.Testing
go test ./...go test ./internal/cache ./internal/shards ./internal/restoreGOTOOLCHAIN=go1.25.9 make testGOTOOLCHAIN=go1.25.9 go vet ./...GOTOOLCHAIN=go1.25.9 golangci-lint run --new-from-rev=upstream/main ./...supermodel updatesmoke: temporaryv0.6.12binary updated to0.6.13 (66432f6, 2026-04-30T15:18:18Z).Note: full-repo
golangci-lint run ./...still reports pre-existing staticcheck SA5011 warnings in untouched focus/zip tests; PR-diff lint is clean.Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
watchrenamed tosupermodelChanges
--three-fileoptiondocs-outputdirectory from analysis