Merged
Conversation
Full Go CLI implementation synthesized from all issue thread context: - cmd/run.go: Core hook command (Stop hook, cache-first, async API polling) - cmd/auth.go: auth login/status/logout with dashboard.supermodeltools.com - cmd/install.go: Non-destructive settings.json merger with diff preview - cmd/status.go: status, logs, stats, dry-run, cache subcommands - internal/api/client.go: Supermodel API client (Bearer token, 200/202 async polling) - internal/cache/store.go: SQLite cache (WAL, TTL=15m, staleness, injection log, auto-prune) - internal/config/config.go: XDG config dirs, flag>env>file resolution - internal/hooks/hooks.go: Cross-platform settings.json detection and hook injection - internal/project/project.go: Git-aware project detection and stable hash - internal/template/render.go: Token-budgeted Markdown renderer (2000 token default) - internal/zip/zip.go: Repo zipper (excludes .git, node_modules, binaries) - README.md: Full setup, usage, architecture, and KPI documentation Key design decisions from thread critique: - Silent exit 0 on failure (never blocks Claude Code sessions) - 2000 token default cap with tiered required/optional sections - STALE warning when serving cached data past TTL - uncompact install with diff preview before writing - Measurable KPIs replacing vague density metric Co-authored-by: Grey Newell <greynewell@users.noreply.github.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR introduces Uncompact, a complete Go CLI application that reinjection context into Claude Code after compaction. The codebase includes an API client for the Supermodel API, SQLite-backed caching with injection logging, configuration management, hook installation for Claude Code, project detection, template-based Markdown rendering, and CLI commands for authentication, installation, status reporting, and context injection. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as Uncompact CLI
participant Config as Config
participant Project as Project Detector
participant Cache as SQLite Cache
participant API as Supermodel API
participant Template as Template Renderer
User->>CLI: uncompact run
CLI->>Config: Load config & authenticate
Config-->>CLI: Config (API key, base URL)
CLI->>Project: Detect current project
Project-->>CLI: Project info (name, hash, git URL)
CLI->>Cache: Get cached graph for project hash
Cache-->>CLI: Cached graph + freshness + expiry
alt Cache hit (fresh or stale)
CLI->>Template: Render graph to Markdown
else Cache miss or force-refresh
CLI->>API: Submit repo ZIP + request graph
API-->>CLI: Response (sync 200 or async 202 with jobID)
alt Async response
CLI->>API: Poll /v1/graphs/supermodel/{jobID}
API-->>CLI: ProjectGraph (completed)
end
CLI->>Cache: Store fetched graph (fresh)
CLI->>Template: Render graph to Markdown
end
Template-->>CLI: Markdown context bomb
CLI->>User: Output to stdout (inject into Claude Code)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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)
Comment |
Contributor
Author
|
@claude we need to vend a plugin that integrates with hooks so that Claude can use our tool automatically: your-tool-plugin/ DistributionTwo paths:
Cross-surface compatibilityKey detail: hooks run across CLI, web, GitHub Actions, and Slack integrations. The Practical notes
|
Contributor
|
Claude finished @greynewell's task in 3m 39s —— View job Implemented Uncompact Claude Code PluginTasks:
What was added
{
"hooks": {
"Stop": [{
"hooks": [{
"type": "command",
"command": "bash ${CLAUDE_PLUGIN_ROOT}/scripts/uncompact-hook.sh",
"timeout": 120
}]
}]
}
}The 120s timeout matches the API polling window in
Usage/plugin install github:supermodeltools/UncompactUsers still need to install the binary and run |
Adds the Claude Code plugin structure so users can install Uncompact's
Stop hook with a single /plugin install command instead of running
'uncompact install' manually.
Plugin structure:
- .claude-plugin/plugin.json -- manifest (name, version, description)
- hooks/hooks.json -- Stop hook via CLAUDE_PLUGIN_ROOT
- scripts/uncompact-hook.sh -- wrapper that locates the binary,
enables --fallback in CI environments,
and passes SUPERMODEL_API_KEY through
Co-authored-by: Grey Newell <greynewell@users.noreply.github.com>
Contributor
Author
|
@claude I want to make sure that the user ONLY needs to install the plugin. Make sure there is an instal script hook that handles setting up uncompact for the user |
Contributor
|
Claude finished @greynewell's task in 3m 21s —— View job Tasks
What changed
"SessionStart": [{
"hooks": [{ "type": "command", "command": "bash ${CLAUDE_PLUGIN_ROOT}/scripts/setup.sh", "timeout": 120 }]
}]
User experience after this change/plugin install github:supermodeltools/Uncompact
# → Next session start auto-installs the binary
# → Prompts once: "uncompact auth login"
# → After login: silent, fully automatic context injection· |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (12)
cmd/auth.go (1)
57-64: Consider checkingscanner.Err()for better error messages.When
scanner.Scan()returnsfalse, it could mean:
- User hit EOF (Ctrl+D) — totally fine, "no input" is correct
- Actual I/O error — would be nice to surface that
Quick fix:
♻️ Slightly better error handling
scanner := bufio.NewScanner(os.Stdin) if !scanner.Scan() { + if err := scanner.Err(); err != nil { + return fmt.Errorf("reading input: %w", err) + } return fmt.Errorf("no input provided") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/auth.go` around lines 57 - 64, When scanner.Scan() returns false in the input-reading block, check scanner.Err() and return a more specific error: if scanner.Err() != nil return that error (wrapped/annotated with context like "reading API key failed"), otherwise keep the existing "no input provided" message for EOF; update the code around scanner := bufio.NewScanner(os.Stdin), the Scan() call and the subsequent error handling so scanner.Err() is consulted before returning.cmd/status.go (1)
206-207: UnusedlogFnvariable looks like leftover code.
makeLogger()is called on line 206, but the result is only used in a blank assignment on line 251 (_ = logFn). This looks like leftover from debugging or incomplete refactoring.Either use
logFnfor debug output or remove it:♻️ Remove unused code
func dryRunHandler(cmd *cobra.Command, args []string) error { - logFn := makeLogger() - cfg, err := config.Load(apiKey)And remove line 251:
- _ = logFn - if !cfg.IsAuthenticated() {Also applies to: 251-251
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/status.go` around lines 206 - 207, The local variable logFn assigned from makeLogger() is unused (only a blank assignment `_ = logFn` remains); remove the dead code by deleting the `logFn := makeLogger()` call and the `_ = logFn` no-op, or alternatively, actually use the returned logger where intended (replace the `_ = logFn` with real debug/log calls). Locate the `makeLogger` invocation and the `_ = logFn` statement and either remove both or wire `logFn` into the logging logic so the value is used.cmd/install.go (1)
91-111: Verify handler has mixed error handling — probably fine but worth noting.When
FindSettingsFile()fails (line 92-97), you print a message and returnnil(success exit code). But whenVerify()fails (line 99-102), you return the error (failure exit code).This is probably intentional — "settings file not found" is informational while "verify failed" is a real error. Just be aware this could affect scripting if someone checks exit codes.
If you want consistency, you could return an error for the "not found" case too:
♻️ Optional: consistent error handling
func verifyInstallHandler(cmd *cobra.Command, args []string) error { settingsPath, err := hooks.FindSettingsFile() if err != nil { fmt.Println("✗ Could not find Claude Code settings.json") fmt.Println(" Ensure Claude Code is installed, or check your settings path.") - return nil + return fmt.Errorf("settings file not found") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/install.go` around lines 91 - 111, The verifyInstallHandler mixes exit handling: when hooks.FindSettingsFile() fails it prints messages and returns nil (success), but when hooks.Verify() fails it returns an error; make this consistent by returning an appropriate error from verifyInstallHandler when FindSettingsFile() fails instead of nil — update the FindSettingsFile() error branch to wrap/return the original error (or a new fmt.Errorf with context) so callers get a non-zero exit code; reference the verifyInstallHandler function and the calls to hooks.FindSettingsFile() and hooks.Verify() when making the change.README.md (1)
9-23: Consider adding language specifiers to fenced code blocks.The static analysis tool flagged these code blocks as missing language specifiers. For ASCII diagrams and plain text output, you can use
textorplaintextas the language identifier to silence the linter while keeping things readable.For example:
-``` +```text Claude Code compaction occursThis is totally optional — the README looks great and the content is clear either way.
Also applies to: 60-83, 140-164, 186-191
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 9 - 23, Update the fenced code blocks in README.md to include a language specifier (e.g., "text" or "plaintext") so the linter stops flagging them; specifically add the specifier to the ASCII diagram block shown and the other unlabeled fences referenced (around lines 60-83, 140-164, and 186-191) by editing those fenced blocks (``` → ```text) in the README to keep them rendered as plain text.internal/api/client.go (2)
154-158: Hardcoded URLs in error messages.You've got
"https://dashboard.supermodeltools.com"hardcoded in lines 155 and 158, but there's aDashboardURLconstant ininternal/config/config.go.Not a big deal since these URLs are unlikely to change, but for consistency you could import and use the constant. Up to you!
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/client.go` around lines 154 - 158, Replace the hardcoded dashboard URLs in the error returns for the http.StatusUnauthorized and http.StatusPaymentRequired cases with the shared DashboardURL constant from internal/config/config.go; update the imports to reference the config package (e.g., config.DashboardURL) and use it in the fmt.Errorf messages so both error strings consistently use the DashboardURL constant instead of the literal "https://dashboard.supermodeltools.com".
274-276: Swallowing the unmarshal error might hide issues.If the auth response is valid HTTP 200 but malformed JSON, you return
("", nil)which looks like success with no user info. The caller might not realize something went wrong.Consider returning the error or at least a placeholder message:
Proposed fix
if err := json.Unmarshal(body, &me); err != nil { - return "", nil + return "(unable to parse user info)", nil }Or if you want to be stricter:
if err := json.Unmarshal(body, &me); err != nil { - return "", nil + return "", fmt.Errorf("parsing auth response: %w", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/client.go` around lines 274 - 276, The code swallows json.Unmarshal errors (json.Unmarshal(body, &me)) and returns ("", nil), which hides malformed JSON issues; change the handler to return the unmarshal error (or a wrapped error with context) instead of nil so callers know parsing failed — locate the json.Unmarshal call and the surrounding return statement that returns "", nil (the variables 'body' and 'me' identify the spot) and replace the nil return with the actual error (or wrap it) so parsing failures are propagated.internal/hooks/hooks.go (2)
12-17: TheExtrafield withjson:"-"doesn't preserve unknown fields on round-trip.The comment says "Preserve other fields we don't know about," but
json:"-"just tells the encoder to skip this field. It doesn't magically capture unknown fields during decode.Looking at your
Installfunction, you actually handle this correctly by usingmap[string]json.RawMessagefor the raw JSON and only parsing/modifying thehookskey. TheExtrafield is never used.You can just remove the
Extrafield since it's dead code:Remove unused field
// ClaudeSettings represents Claude Code's settings.json structure. type ClaudeSettings struct { Hooks map[string][]Hook `json:"hooks,omitempty"` - // Preserve other fields we don't know about - Extra map[string]json.RawMessage `json:"-"` }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/hooks/hooks.go` around lines 12 - 17, The ClaudeSettings struct contains a dead Extra field (json:"-") that doesn't capture unknown JSON and is unused; remove the Extra field declaration from the ClaudeSettings type and update the struct comment accordingly, leaving Hooks as the only field, and verify the Install function continues to operate using its map[string]json.RawMessage handling for raw JSON (adjust any code that referenced ClaudeSettings.Extra if present) then run a build/check to ensure no references remain.
183-193: The comment says "without duplicating" butmergeHooksitself doesn't dedupe.The deduplication actually happens because
isAlreadyInstalled()is called beforemergeHooks(), and if hooks are already present, you return early. So the behavior is correct, but the comment onmergeHooksis misleading.Consider updating the comment to be more accurate:
Clarify the comment
-// mergeHooks merges new hooks into existing hooks without duplicating. +// mergeHooks appends new hooks to existing hooks. Caller should check +// isAlreadyInstalled() first to avoid duplicates. func mergeHooks(existing, toAdd map[string][]Hook) map[string][]Hook {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/hooks/hooks.go` around lines 183 - 193, Update the misleading comment on mergeHooks to accurately reflect its behavior: state that mergeHooks(appends new hooks to existing hooks without deduplication) and note that deduplication is handled by isAlreadyInstalled elsewhere; reference the mergeHooks function and isAlreadyInstalled to make it clear where dedupe occurs.internal/template/render.go (1)
1-12: Package namerenderdoesn't match the import pathinternal/template.The package is declared as
package renderbut lives ininternal/template/. In Go, the package name and directory name don't have to match, but it's confusing when they don't.More importantly, in
cmd/run.goline 14, you import it as:tmpl "github.com/supermodeltools/uncompact/internal/template"Then you call
tmpl.Render()andtmpl.RenderOptions{}. This should actually work since you're aliasing the import totmpl, but the actual package name isrender.So
tmpl.Renderresolves torender.Renderwhich exists. It should compile, but it's confusing. Consider either:
- Rename the package to
package template(matching the directory), or- Move the file to
internal/render/render.goSimple fix: rename package to match directory
-package render +package template🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/template/render.go` around lines 1 - 12, The package in internal/template/render.go is declared as package render but the import path and usage expect package template; change the package declaration to "package template" so symbols like Render and RenderOptions are defined on the template package (ensure the file's package line is updated from "render" to "template" and run a quick build to catch any remaining references expecting render).cmd/run.go (1)
112-119: Empty block with fallthrough logic is a bit confusing.This works, but the empty
ifblock at line 112-114 with just a comment is a code smell. Consider restructuring for clarity:Cleaner approach
zipData, err := zip.RepoZip(proj.RootDir) if err != nil { logFn("[warn] zip error: %v", err) - if stale && graph != nil { - // Use stale cache as fallback - } else { + if graph == nil { + // No stale cache to fall back on if fallback { printFallback(proj.Name) } return silentExit() } + // else: fall through to use stale cache } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/run.go` around lines 112 - 119, The empty if (stale && graph != nil) { /* Use stale cache as fallback */ } block is confusing—refactor to express the actual control flow by inverting or consolidating the condition so there's no empty branch; e.g., check the negative condition (if !stale || graph == nil) and inside that block run the existing fallback behavior (call printFallback(proj.Name) if fallback is true, then return silentExit()), leaving the stale-cache path implicit/clear; update the logic around the variables stale, graph, fallback and the functions printFallback and silentExit accordingly.internal/config/config.go (1)
115-142:Loaddoesn't restore defaults if config file overwrites them with zero values.Say your config file has
{"max_tokens": 0}. After unmarshaling,cfg.MaxTokensbecomes0, and you never reset it to the default2000. Same forBaseURLif someone sets it to empty string.This might be intentional (let users set 0 tokens?), but if not, you'd want to apply defaults after all overrides:
Proposed fix
func Load(flagAPIKey string) (*Config, error) { - cfg := &Config{ - BaseURL: APIBaseURL, - MaxTokens: 2000, - } + cfg := &Config{} // Load from config file cfgFile, err := ConfigFile() if err == nil { if data, err := os.ReadFile(cfgFile); err == nil { if err := json.Unmarshal(data, cfg); err != nil { return nil, fmt.Errorf("malformed config file %s: %w", cfgFile, err) } } } // Override with env var if envKey := os.Getenv(EnvAPIKey); envKey != "" { cfg.APIKey = envKey } // Override with flag if flagAPIKey != "" { cfg.APIKey = flagAPIKey } + // Apply defaults for unset values + if cfg.BaseURL == "" { + cfg.BaseURL = APIBaseURL + } + if cfg.MaxTokens <= 0 { + cfg.MaxTokens = 2000 + } + return cfg, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config/config.go` around lines 115 - 142, The Load function currently initializes defaults then unmarshals the config file which can overwrite defaults with zero-values (e.g., MaxTokens=0 or BaseURL=""); change Load (function Load, struct Config, fields MaxTokens and BaseURL) to re-apply defaults after all overrides (file, env var EnvAPIKey, and flagAPIKey) or explicitly set defaults only when fields are zero/empty after unmarshaling—i.e., after calling ConfigFile()/json.Unmarshal and after applying env/flag, check cfg.MaxTokens==0 and cfg.BaseURL=="" and set them to the intended defaults (2000 and APIBaseURL) so zero-value file entries do not unintentionally remove defaults.internal/cache/store.go (1)
56-91:schema_versiontable is created but never used.You create the table and define a
schemaVersionconstant (line 16), but there's no code to:
- Check the current version
- Insert/update when migrations run
- Apply different migrations based on version
This is fine for v1 since you only have one schema version, but if you plan to add migrations later, you'll want to wire this up. Just flagging it as a TODO.
Want me to sketch out a simple version-checking migration pattern for future schema changes?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cache/store.go` around lines 56 - 91, The migrate function creates a schema_version table but never reads or updates it; update migrate (and related startup) to use schemaVersion: on startup query schema_version for the max(version) (or 0 if none), then run only migrations that are > current version (apply the existing DDL as version 1), and after successful migration insert or upsert the new version into schema_version (use the table created in migrate). Specifically, modify the migrate function (and/or add a helper like ensureSchemaVersion) to: 1) SELECT COALESCE(MAX(version),0) FROM schema_version, 2) conditionally execute migration steps if current < schemaVersion, and 3) INSERT INTO schema_version(version) VALUES(schemaVersion) (or UPDATE/upsert) after successful application so future runs can skip applied migrations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/auth.go`:
- Around line 110-119: The current masking logic can reveal whole or overlapping
short API keys; update the display code around cfg.APIKey (currently using
prefixEnd and suffixStart and clampAPIKey) to handle short keys safely: compute
keyLen := len(cfg.APIKey) and if keyLen <= 8 print a compact mask like first 1
char + "..." + last 1 char, otherwise use first 4 and last 4 as before
(prefixEnd = 4, suffixStart = keyLen-4). Replace the existing
prefixEnd/suffixStart slicing with this conditional so short keys never expose
more than one char at each end.
In `@cmd/run.go`:
- Around line 93-99: The staleAt is being set to the current time causing "stale
for 0s" — change the cache API and call site so staleAt reflects the cache
entry's expiration/fetch timestamp: update Store.Get (e.g., func (s *Store)
Get(projectHash string) (*api.ProjectGraph, bool, *time.Time, error)) to return
the entry's fetchedAt or expiresAt time, update the call in the block that sets
stale = true to use the returned timestamp (assign staleAt = returnedTime)
instead of time.Now(), and update any other callers of Store.Get to handle the
new return value or ignore it as needed.
In `@cmd/status.go`:
- Around line 157-159: Replace the invisible null-byte separator
(fmt.Println(string(make([]byte, 75)))) with a visible repeated character using
strings.Repeat (e.g., strings.Repeat("-", 75)) and update the imports to include
"strings"; locate the call in the same block that prints the header (the
fmt.Printf with "TIME", "PROJECT", "TOKENS", "SOURCE", "FLAGS") and change the
subsequent fmt.Println to use strings.Repeat so the separator renders correctly.
In `@go.mod`:
- Around line 5-8: Update the retracted dependency modernc.org/sqlite in go.mod
to a safe non-retracted release (at minimum v1.33.1, ideally the current
v1.46.1) by changing the version string for modernc.org/sqlite in the require
block and then run dependency resolution (go get / go mod tidy) to validate;
optionally bump github.com/spf13/cobra to v1.10.2 while you’re editing the
require block to pick up newer fixes.
In `@internal/cache/store.go`:
- Around line 194-222: In Store.GetStats the SUM(...) columns (TotalTokens,
APIFetches, CacheHits) can be NULL when no rows match and will cause Scan to
fail; modify the query to use COALESCE(...) for each aggregate (e.g.
COALESCE(SUM(tokens),0) and COALESCE(SUM(CASE ...),0) ) or change the
corresponding Go scan targets to nullable types (sql.NullInt64) and then set
st.TotalTokens, st.APIFetches and st.CacheHits from those nullables; keep the
existing handling for avgTokens (sql.NullFloat64) if you retain AVG(tokens).
In `@internal/hooks/hooks.go`:
- Around line 136-138: The JSON indentation uses a non-empty prefix which
prepends two spaces to every line; update the json.MarshalIndent calls that
create oldHooksJSON and newHooksJSON (currently
json.MarshalIndent(existingHooks, " ", " ") and json.MarshalIndent(merged, "
", " ")) to use an empty prefix (e.g., json.MarshalIndent(existingHooks, "", "
") and json.MarshalIndent(merged, "", " ")) so the diff produced by buildDiff
reflects normal JSON formatting without leading spaces.
In `@internal/project/project.go`:
- Around line 51-57: The code builds a hash with sha256.Sum256 and then sets
info.Hash with fmt.Sprintf("%x", h[:8]) which yields 16 hex characters (8 bytes
× 2 hex chars each); decide which you intended and fix accordingly: if you want
an 8-hex-character key, change the slice to h[:4] when formatting into
info.Hash, otherwise update the surrounding comment to state "first 16 hex
chars" so the comment matches the current behavior; references: sha256.Sum256,
hashInput, info.Hash, fmt.Sprintf.
In `@internal/zip/zip.go`:
- Around line 63-70: The walker currently only skips hidden directories inside
the info.IsDir() branch so dotfiles like ".env" still get included; update the
file-walking logic (the WalkFunc that uses path, info, skipDirs) to detect
hidden entries for both files and directories by computing base :=
filepath.Base(path) early and: if strings.HasPrefix(base, ".") then return
filepath.SkipDir for directories and return nil (skip this file) for files;
preserve the existing skipDirs check for directories as-is so known large dirs
continue to be skipped.
---
Nitpick comments:
In `@cmd/auth.go`:
- Around line 57-64: When scanner.Scan() returns false in the input-reading
block, check scanner.Err() and return a more specific error: if scanner.Err() !=
nil return that error (wrapped/annotated with context like "reading API key
failed"), otherwise keep the existing "no input provided" message for EOF;
update the code around scanner := bufio.NewScanner(os.Stdin), the Scan() call
and the subsequent error handling so scanner.Err() is consulted before
returning.
In `@cmd/install.go`:
- Around line 91-111: The verifyInstallHandler mixes exit handling: when
hooks.FindSettingsFile() fails it prints messages and returns nil (success), but
when hooks.Verify() fails it returns an error; make this consistent by returning
an appropriate error from verifyInstallHandler when FindSettingsFile() fails
instead of nil — update the FindSettingsFile() error branch to wrap/return the
original error (or a new fmt.Errorf with context) so callers get a non-zero exit
code; reference the verifyInstallHandler function and the calls to
hooks.FindSettingsFile() and hooks.Verify() when making the change.
In `@cmd/run.go`:
- Around line 112-119: The empty if (stale && graph != nil) { /* Use stale cache
as fallback */ } block is confusing—refactor to express the actual control flow
by inverting or consolidating the condition so there's no empty branch; e.g.,
check the negative condition (if !stale || graph == nil) and inside that block
run the existing fallback behavior (call printFallback(proj.Name) if fallback is
true, then return silentExit()), leaving the stale-cache path implicit/clear;
update the logic around the variables stale, graph, fallback and the functions
printFallback and silentExit accordingly.
In `@cmd/status.go`:
- Around line 206-207: The local variable logFn assigned from makeLogger() is
unused (only a blank assignment `_ = logFn` remains); remove the dead code by
deleting the `logFn := makeLogger()` call and the `_ = logFn` no-op, or
alternatively, actually use the returned logger where intended (replace the `_ =
logFn` with real debug/log calls). Locate the `makeLogger` invocation and the `_
= logFn` statement and either remove both or wire `logFn` into the logging logic
so the value is used.
In `@internal/api/client.go`:
- Around line 154-158: Replace the hardcoded dashboard URLs in the error returns
for the http.StatusUnauthorized and http.StatusPaymentRequired cases with the
shared DashboardURL constant from internal/config/config.go; update the imports
to reference the config package (e.g., config.DashboardURL) and use it in the
fmt.Errorf messages so both error strings consistently use the DashboardURL
constant instead of the literal "https://dashboard.supermodeltools.com".
- Around line 274-276: The code swallows json.Unmarshal errors
(json.Unmarshal(body, &me)) and returns ("", nil), which hides malformed JSON
issues; change the handler to return the unmarshal error (or a wrapped error
with context) instead of nil so callers know parsing failed — locate the
json.Unmarshal call and the surrounding return statement that returns "", nil
(the variables 'body' and 'me' identify the spot) and replace the nil return
with the actual error (or wrap it) so parsing failures are propagated.
In `@internal/cache/store.go`:
- Around line 56-91: The migrate function creates a schema_version table but
never reads or updates it; update migrate (and related startup) to use
schemaVersion: on startup query schema_version for the max(version) (or 0 if
none), then run only migrations that are > current version (apply the existing
DDL as version 1), and after successful migration insert or upsert the new
version into schema_version (use the table created in migrate). Specifically,
modify the migrate function (and/or add a helper like ensureSchemaVersion) to:
1) SELECT COALESCE(MAX(version),0) FROM schema_version, 2) conditionally execute
migration steps if current < schemaVersion, and 3) INSERT INTO
schema_version(version) VALUES(schemaVersion) (or UPDATE/upsert) after
successful application so future runs can skip applied migrations.
In `@internal/config/config.go`:
- Around line 115-142: The Load function currently initializes defaults then
unmarshals the config file which can overwrite defaults with zero-values (e.g.,
MaxTokens=0 or BaseURL=""); change Load (function Load, struct Config, fields
MaxTokens and BaseURL) to re-apply defaults after all overrides (file, env var
EnvAPIKey, and flagAPIKey) or explicitly set defaults only when fields are
zero/empty after unmarshaling—i.e., after calling ConfigFile()/json.Unmarshal
and after applying env/flag, check cfg.MaxTokens==0 and cfg.BaseURL=="" and set
them to the intended defaults (2000 and APIBaseURL) so zero-value file entries
do not unintentionally remove defaults.
In `@internal/hooks/hooks.go`:
- Around line 12-17: The ClaudeSettings struct contains a dead Extra field
(json:"-") that doesn't capture unknown JSON and is unused; remove the Extra
field declaration from the ClaudeSettings type and update the struct comment
accordingly, leaving Hooks as the only field, and verify the Install function
continues to operate using its map[string]json.RawMessage handling for raw JSON
(adjust any code that referenced ClaudeSettings.Extra if present) then run a
build/check to ensure no references remain.
- Around line 183-193: Update the misleading comment on mergeHooks to accurately
reflect its behavior: state that mergeHooks(appends new hooks to existing hooks
without deduplication) and note that deduplication is handled by
isAlreadyInstalled elsewhere; reference the mergeHooks function and
isAlreadyInstalled to make it clear where dedupe occurs.
In `@internal/template/render.go`:
- Around line 1-12: The package in internal/template/render.go is declared as
package render but the import path and usage expect package template; change the
package declaration to "package template" so symbols like Render and
RenderOptions are defined on the template package (ensure the file's package
line is updated from "render" to "template" and run a quick build to catch any
remaining references expecting render).
In `@README.md`:
- Around line 9-23: Update the fenced code blocks in README.md to include a
language specifier (e.g., "text" or "plaintext") so the linter stops flagging
them; specifically add the specifier to the ASCII diagram block shown and the
other unlabeled fences referenced (around lines 60-83, 140-164, and 186-191) by
editing those fenced blocks (``` → ```text) in the README to keep them rendered
as plain text.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
README.mdcmd/auth.gocmd/install.gocmd/root.gocmd/run.gocmd/status.gogo.modinternal/api/client.gointernal/cache/store.gointernal/config/config.gointernal/hooks/hooks.gointernal/project/project.gointernal/template/render.gointernal/zip/zip.gomain.go
Contributor
Author
|
@claude fix the coderabbit review comments and push one commit that addresses them all. resolve comment thread conversations on github pr after push |
Contributor
|
Claude finished @greynewell's task in 15m 27s —— View job Fixed all CodeRabbit review commentsTasks:
Summary of changes pushed2 commits added to the branch:
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
README.md (1)
9-23: Add language identifiers to fenced code blocks for better rendering.Hey! The static analysis is flagging these code blocks as missing language specifiers. For the ASCII diagrams/flowcharts, you can use
textorplaintextto satisfy linters while keeping them rendered as plain text:
- Line 9: The workflow diagram
- Line 85: CLI reference block
- Line 165: Architecture tree
- Line 211: Fallback chain diagram
Example fix for line 9:
-``` +```text Claude Code compaction occurs ↓Not a big deal, but it'll make the linting happy and some Markdown renderers handle it better.
Also applies to: 85-108, 165-189, 211-216
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 9 - 23, The README contains multiple fenced ASCII diagrams/flowcharts missing language specifiers (the workflow diagram, the CLI reference block, the Architecture tree, and the Fallback chain diagram); update each triple-backtick fence for those blocks to include a plain text language identifier (e.g., ```text or ```plaintext) so linters/renderers accept them—search for the ASCII diagram blocks in README.md and add the language token immediately after the opening ``` for the blocks shown in the diff.scripts/uncompact-hook.sh (1)
40-43: API key passed via CLI argument is visible in process lists.Hey, so when you do
--api-key "${SUPERMODEL_API_KEY}", that value shows up if someone runsps auxor looks at/proc/<pid>/cmdline. It's usually not a huge deal for short-lived processes, but it's worth knowing.An alternative is to keep the env var and let the Go binary read
SUPERMODEL_API_KEYdirectly from the environment (which it sounds like it already supports based on the README). Then you could skip passing it as an argument entirely:-# Allow API key override via environment variable (useful in CI). -if [ -n "${SUPERMODEL_API_KEY:-}" ]; then - ARGS+=("--api-key" "${SUPERMODEL_API_KEY}") -fi +# SUPERMODEL_API_KEY is read by the uncompact binary directly from env. +# No need to pass it as an argument (avoids exposure in process lists).But if you want to keep it explicit for clarity, that's a reasonable tradeoff too. Just wanted to flag it!
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/uncompact-hook.sh` around lines 40 - 43, The script currently appends the API key to ARGS as a CLI flag (ARGS+=("--api-key" "${SUPERMODEL_API_KEY}")), which exposes the secret in process listings; remove that ARGS+=("--api-key" ...) branch and rely on the SUPERMODEL_API_KEY environment variable being read directly by the Go binary (or, if explicitness is desired, export SUPERMODEL_API_KEY before exec and do not pass --api-key). Ensure references to ARGS, SUPERMODEL_API_KEY and the --api-key flag are updated so the binary receives the key only via environment variable and not as a command-line argument.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@README.md`:
- Around line 9-23: The README contains multiple fenced ASCII
diagrams/flowcharts missing language specifiers (the workflow diagram, the CLI
reference block, the Architecture tree, and the Fallback chain diagram); update
each triple-backtick fence for those blocks to include a plain text language
identifier (e.g., ```text or ```plaintext) so linters/renderers accept
them—search for the ASCII diagram blocks in README.md and add the language token
immediately after the opening ``` for the blocks shown in the diff.
In `@scripts/uncompact-hook.sh`:
- Around line 40-43: The script currently appends the API key to ARGS as a CLI
flag (ARGS+=("--api-key" "${SUPERMODEL_API_KEY}")), which exposes the secret in
process listings; remove that ARGS+=("--api-key" ...) branch and rely on the
SUPERMODEL_API_KEY environment variable being read directly by the Go binary
(or, if explicitness is desired, export SUPERMODEL_API_KEY before exec and do
not pass --api-key). Ensure references to ARGS, SUPERMODEL_API_KEY and the
--api-key flag are updated so the binary receives the key only via environment
variable and not as a command-line argument.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.claude-plugin/plugin.jsonREADME.mdhooks/hooks.jsonscripts/uncompact-hook.sh
✅ Files skipped from review due to trivial changes (1)
- .claude-plugin/plugin.json
Contributor
Author
|
@claude rebase this PR on main so that you can review it with your new permissions in github actions |
Contributor
|
Claude finished @greynewell's task in 1m 40s —— View job Rebasing PR on mainTasks:
Unfortunately, I'm unable to perform a rebase or merge on this branch — git branch operations (rebase, merge) are outside my capabilities per my current configuration. What's missingThe PR branch is 1 commit behind This commit added How to fix it manuallyYou can bring the branch up to date with either of these approaches: Option A — Merge (simpler, non-destructive): git fetch origin
git checkout claude/issue-1-20260226-2246
git merge origin/main
git push origin claude/issue-1-20260226-2246Option B — Rebase (cleaner history): git fetch origin
git checkout claude/issue-1-20260226-2246
git rebase origin/main
git push --force-with-lease origin claude/issue-1-20260226-2246Once the branch is updated and the workflows run with the new permissions, you can tag me again with You can find more information about my branch operation limitations in the FAQ. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/setup.sh (1)
67-81: Consider preservinggo installerror output for easier debugging.Right now you're sending both stdout and stderr to
/dev/null:go install ... >/dev/null 2>&1So if something goes wrong (like a network issue or a Go version mismatch), users just see "go install failed" without knowing why. This can make debugging frustrating.
A simple improvement: capture stderr and show it only when the command fails:
if err_output=$(GOBIN="${GOBIN:-${HOME}/go/bin}" go install github.com/supermodeltools/uncompact@latest 2>&1); then # success else warn "go install failed:" warn "$err_output" fiThat way, on success you stay quiet (good UX), but on failure you give users the actual error message (e.g.,
module requires Go 1.22).This is totally optional though — your current approach of exiting 0 and not blocking the session is a reasonable tradeoff for a SessionStart hook.
🔧 Optional: capture and display errors
if command -v go >/dev/null 2>&1; then log "Installing uncompact (first-time setup)..." - if GOBIN="${GOBIN:-${HOME}/go/bin}" go install github.com/supermodeltools/uncompact@latest >/dev/null 2>&1; then + install_output=$(GOBIN="${GOBIN:-${HOME}/go/bin}" go install github.com/supermodeltools/uncompact@latest 2>&1) + install_status=$? + if [ $install_status -eq 0 ]; then UNCOMPACT="$(find_uncompact 2>/dev/null || true)" if [ -n "${UNCOMPACT:-}" ]; then log "✓ Installed at ${UNCOMPACT}" @@ ... @@ else warn "go install failed. Install manually and restart Claude Code:" + [ -n "$install_output" ] && warn "$install_output" warn " go install github.com/supermodeltools/uncompact@latest"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/setup.sh` around lines 67 - 81, Capture and surface go install stderr when it fails: replace the current silent invocation of go install (the GOBIN="${GOBIN:-${HOME}/go/bin}" go install github.com/supermodeltools/uncompact@latest >/dev/null 2>&1) with a form that stores stderr (e.g., err_output=$(GOBIN=... go install ... 2>&1)) and then on failure include that captured err_output in the warn messages before exiting; keep the existing logic that calls find_uncompact and sets UNCOMPACT, logging success via log "✓ Installed at ${UNCOMPACT}" when present, but print the captured error details in the else branch where you currently warn "go install failed. Install manually..." so users get the real go install error for debugging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 9-23: The fenced code blocks in README.md are missing language
tags (MD040); update each fenced block to include appropriate language
identifiers—use ```text for diagram/tree blocks (e.g., the "Claude Code
compaction occurs" diagram and the "uncompact/" file tree block) and ```bash for
command snippets (e.g., the "uncompact auth login" command list and other CLI
examples). Apply these changes to the shown block (lines around the "Claude Code
compaction occurs" diagram) and the other affected sections (around 89-112,
169-193, 215-220) so all fences include either text or bash as appropriate.
---
Nitpick comments:
In `@scripts/setup.sh`:
- Around line 67-81: Capture and surface go install stderr when it fails:
replace the current silent invocation of go install (the
GOBIN="${GOBIN:-${HOME}/go/bin}" go install
github.com/supermodeltools/uncompact@latest >/dev/null 2>&1) with a form that
stores stderr (e.g., err_output=$(GOBIN=... go install ... 2>&1)) and then on
failure include that captured err_output in the warn messages before exiting;
keep the existing logic that calls find_uncompact and sets UNCOMPACT, logging
success via log "✓ Installed at ${UNCOMPACT}" when present, but print the
captured error details in the else branch where you currently warn "go install
failed. Install manually..." so users get the real go install error for
debugging.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.mdhooks/hooks.jsonscripts/setup.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- hooks/hooks.json
Contributor
Author
|
@claude graph generation takes about 10-15 minutes. Can we make sure the hooks pregenerate it so we don't have to wait whenever the compaction happens? |
Contributor
|
Claude finished @greynewell's task in 4m 11s —— View job Pregenerate graph for instant Stop hook deliveryTasks:
What was added
"SessionStart": [ ...setup.sh..., { pregen-hook.sh, timeout: 10 } ]
"PostToolUse": [ { "matcher": "Write|Edit", pregen-hook.sh, timeout: 10 } ]How it works nowThe 15-minute cache TTL aligns with the ~12-minute generation time. After the first session-start pregen completes, subsequent PostToolUse triggers are nearly always cache-hits (instant exit). When the cache does expire, the next Write/Edit silently kicks off a fresh background generation. |
- cmd/auth.go: check scanner.Err() for better error messages; fix API key
masking for short keys (≤8 chars show length instead of exposing key);
remove unused clampAPIKey helper
- cmd/run.go: use expiresAt from Store.Get for staleAt so users see accurate
stale duration; simplify confusing empty-block fallthrough logic
- cmd/status.go: replace null-byte separator with strings.Repeat("-", 75);
remove unused logFn/makeLogger call in dryRunHandler; update Store.Get callers
for new 4-value return signature
- cmd/install.go: return error from verifyInstallHandler when settings file
not found (consistent exit codes for scripting)
- internal/api/client.go: use config.DashboardURL constant instead of hardcoded
URLs; propagate json.Unmarshal error in ValidateKey instead of swallowing it
- internal/cache/store.go: wire schema_version table — check and record current
version after DDL so future migrations can skip applied steps; add COALESCE
to SUM aggregates in GetStats to prevent NULL scan errors on empty tables;
update Get() to return expiresAt as 4th value for accurate stale tracking
- internal/config/config.go: apply defaults (BaseURL, MaxTokens) after all
overrides so zero-value config file entries don't unintentionally clear them
- internal/hooks/hooks.go: remove dead Extra field (json:"-" never captured
unknown fields); fix MarshalIndent prefix from " " to "" for clean diffs;
correct misleading comment on mergeHooks
- internal/project/project.go: clarify hash comment (h[:8] = 16 hex chars)
- internal/template/render.go: rename package from render to template to match
directory; alias text/template as gotmpl to avoid identifier conflict
- internal/zip/zip.go: skip hidden files (e.g. .env, .secrets) in addition
to hidden directories
- go.mod: upgrade modernc.org/sqlite from retracted v1.33.0 to v1.37.0
- README.md: add text language specifier to unlabeled fenced code blocks
Co-authored-by: Grey Newell <greynewell@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
cmd/pregen.go (1)
76-78: Consider making the timeout configurable.The 20-minute timeout is reasonable for a background operation, but if the Supermodel API gets slow or the repo is huge, this could be a blocker. You might want to expose this as a flag (e.g.,
--timeout) for edge cases.That said, this is totally optional and the current hardcoded value works fine for v1.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/pregen.go` around lines 76 - 78, Replace the hardcoded 20*time.Minute timeout used in context.WithTimeout with a configurable value exposed as a flag (e.g., --timeout) and default to 20m; parse the flag in the command setup that constructs ctx/cancel (the code calling context.WithTimeout in pregen command) and convert the parsed duration string to time.Duration, then call context.WithTimeout(context.Background(), parsedTimeout) so ctx and cancel use the configured value.scripts/pregen-hook.sh (1)
36-45: Minor race window in lock check.There's a small TOCTOU race between checking if the lock exists (line 37) and creating a new one (line 49). Two hooks could both pass the check and both spawn pregen.
For a cache-warming use case this is probably fine — worst case you get two API calls. But if you want to be bulletproof, you could use
flockor an atomicmkdir-based lock.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/pregen-hook.sh` around lines 36 - 45, The current TOCTOU arises from the non-atomic check/remove of LOCK_FILE (variables LOCK_FILE and LOCK_PID in pregen-hook.sh); replace the ad-hoc exists+kill check with an atomic lock strategy — either use flock on a dedicated lockfile descriptor around the entire critical section (create/open LOCK_FILE and run flock -n to acquire, exit if fail, and ensure flock is released/closed on exit) or use an atomic mkdir-based lock (mkdir "$LOCK_DIR" succeeds only for the first process, remove the directory on exit). Ensure cleanup occurs on script exit (trap) so the lock is always released.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/pregen-hook.sh`:
- Around line 47-52: The lock file is written with $$ inside the subshell which
records the parent PID; change it to record the actual background job PID
instead — either replace $$ with $BASHPID if you guarantee bash, or (preferably
for POSIX compatibility) move the echo out of the subshell and write $! after
launching the background job; update the block that spawns "${UNCOMPACT}" pregen
and the LOCK_FILE handling so the file contains the real background PID and the
trap still removes "${LOCK_FILE}" when the job exits.
---
Nitpick comments:
In `@cmd/pregen.go`:
- Around line 76-78: Replace the hardcoded 20*time.Minute timeout used in
context.WithTimeout with a configurable value exposed as a flag (e.g.,
--timeout) and default to 20m; parse the flag in the command setup that
constructs ctx/cancel (the code calling context.WithTimeout in pregen command)
and convert the parsed duration string to time.Duration, then call
context.WithTimeout(context.Background(), parsedTimeout) so ctx and cancel use
the configured value.
In `@scripts/pregen-hook.sh`:
- Around line 36-45: The current TOCTOU arises from the non-atomic check/remove
of LOCK_FILE (variables LOCK_FILE and LOCK_PID in pregen-hook.sh); replace the
ad-hoc exists+kill check with an atomic lock strategy — either use flock on a
dedicated lockfile descriptor around the entire critical section (create/open
LOCK_FILE and run flock -n to acquire, exit if fail, and ensure flock is
released/closed on exit) or use an atomic mkdir-based lock (mkdir "$LOCK_DIR"
succeeds only for the first process, remove the directory on exit). Ensure
cleanup occurs on script exit (trap) so the lock is always released.
scripts/pregen-hook.sh
Outdated
Comment on lines
+47
to
+52
| # Spawn pregen in the background — write PID to lock file for dedup. | ||
| ( | ||
| echo $$ > "${LOCK_FILE}" | ||
| trap 'rm -f "${LOCK_FILE}"' EXIT | ||
| "${UNCOMPACT}" pregen | ||
| ) >/dev/null 2>&1 & |
There was a problem hiding this comment.
Bug: Lock file contains wrong PID, defeating the deduplication.
Here's the problem: inside a subshell, $$ still returns the parent shell's PID, not the subshell's PID. So you're writing the outer script's PID to the lock file, but the outer script exits immediately at line 54.
What happens:
- Hook runs, outer shell PID = 1000, subshell PID = 1001
echo $$writes1000to lock file- Outer shell exits (line 54)
- Subshell keeps running pregen in background
- Another hook runs, reads
1000from lock file kill -0 1000fails (parent already exited) → treats as stale lock- Spawns another pregen → concurrent API calls!
Use $BASHPID instead, or write the background job's PID from outside the subshell.
🐛 Proposed fix using $BASHPID
# Spawn pregen in the background — write PID to lock file for dedup.
(
- echo $$ > "${LOCK_FILE}"
+ echo $BASHPID > "${LOCK_FILE}"
trap 'rm -f "${LOCK_FILE}"' EXIT
"${UNCOMPACT}" pregen
) >/dev/null 2>&1 &Alternatively, you could capture $! (the background job PID) from outside the subshell:
(
trap 'rm -f "${LOCK_FILE}"' EXIT
"${UNCOMPACT}" pregen
) >/dev/null 2>&1 &
echo $! > "${LOCK_FILE}"This second approach is arguably cleaner since $! is POSIX and works in all shells, not just bash.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/pregen-hook.sh` around lines 47 - 52, The lock file is written with
$$ inside the subshell which records the parent PID; change it to record the
actual background job PID instead — either replace $$ with $BASHPID if you
guarantee bash, or (preferably for POSIX compatibility) move the echo out of the
subshell and write $! after launching the background job; update the block that
spawns "${UNCOMPACT}" pregen and the LOCK_FILE handling so the file contains the
real background PID and the trap still removes "${LOCK_FILE}" when the job
exits.
Contributor
Author
There was a problem hiding this comment.
@claude make sure this is fixed and resolve it if it's not
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
Contributor
There was a problem hiding this comment.
Claude finished @greynewell's task in 33s —— View job
Tasks
- Gather context and understand the request
- Read
scripts/pregen-hook.shto check current PID fix status - Verified fix is already applied — no changes needed
Result
The PID bug is already fixed in the current code (scripts/pregen-hook.sh lines 47–54):
# Spawn pregen in the background.
# The trap inside the subshell ensures the lock is removed on exit.
(
trap 'rm -f "${LOCK_FILE}"' EXIT
"${UNCOMPACT}" pregen
) >/dev/null 2>&1 &
# Write the actual background job PID to the lock file.
# Must be done outside the subshell so $! refers to the spawned process.
echo $! > "${LOCK_FILE}"The fix correctly:
- Spawns the subshell as a background job (
&) - Writes
$!(the actual background process PID) to the lock file from outside the subshell, after launching - The subshell's
trap 'rm -f "${LOCK_FILE}"' EXITstill handles cleanup when pregen finishes
This matches the POSIX-compatible fix CodeRabbit suggested. This was applied as part of commit 598c8ab ("feat: reimplement pregen + fix remaining CodeRabbit review issues"). The review thread can be resolved.
Adds scripts/setup.sh which auto-installs the uncompact binary via go install on SessionStart, so users only need /plugin install to get started — no manual binary download required. Updates hooks/hooks.json to register the SessionStart hook alongside the existing Stop hook, and updates README.md to clarify the one-step plugin install experience. Co-authored-by: Grey Newell <greynewell@users.noreply.github.com>
claude
bot
force-pushed
the
claude/issue-1-20260226-2246
branch
from
9041be1 to
b8eedbb
Compare
Contributor
Author
|
@claude we lost a commit implementing graph pregeneration. reimplement that and fix + resolve all coderabbit review comments. Get this PR finalized and ready to merge. |
Contributor
|
Claude finished @greynewell's task in 2m 56s —— View job Reimplementing pregen + fixing all CodeRabbit issuesTasks:
What was done
How pregeneration works now· Branch |
- Add cmd/pregen.go: background cache warmer with 20min timeout; skips API call when cache is fresh; safe to run from hooks (no stdout output) - Add scripts/pregen-hook.sh: spawns pregen in background with correct PID tracking — uses $! (background job PID) not $$ (parent PID) to fix the lock dedup bug flagged by CodeRabbit - Add PostToolUse hook in hooks/hooks.json: fires pregen-hook.sh after Write/Edit ops so cache is warm before compaction occurs - Also add pregen-hook.sh to SessionStart so first session warms cache - Fix scripts/uncompact-hook.sh: remove --api-key CLI arg; binary reads SUPERMODEL_API_KEY from env directly, avoiding process list exposure - Fix scripts/setup.sh: capture go install stderr and surface it on failure so users see the actual error instead of a generic message Co-authored-by: Grey Newell <greynewell@users.noreply.github.com>
Contributor
Author
|
@claude how do I test this locally? |
Contributor
|
Claude finished @greynewell's task in 46s —— View job How to test this locally
Here's a complete guide to testing Uncompact locally from this PR branch. Prerequisites
1. Clone and buildgit clone https://github.com/supermodeltools/Uncompact
cd Uncompact
git checkout claude/issue-1-20260226-2246
go mod tidy # downloads dependencies + regenerates go.sum
go build -o uncompact .This drops a 2. Authenticate./uncompact auth loginYou'll be prompted to paste your API key. It validates the key against the Supermodel API before saving it to ./uncompact auth status # verify it worked3. Test core commands (no Claude Code needed)Preview the context bomb without emitting it: ./uncompact dry-runRun with debug output (best way to see what's happening): ./uncompact run --debugThis zips the repo, calls the API, caches the result, and prints the Markdown context bomb to stdout. The first run takes ~10–15 minutes (async API job). Subsequent runs within 15 minutes serve instantly from cache. Force a fresh API fetch (bypass cache): ./uncompact run --force-refresh --debugTest the fallback mode (e.g. no API key): ./uncompact run --fallback --debugCheck cache and injection stats: ./uncompact status
./uncompact logs
./uncompact statsClear cache between test runs: ./uncompact cache clear --project4. Test hook installationPreview what changes would be made (non-destructive): ./uncompact install --dry-runActually install hooks into Claude Code's settings.json: ./uncompact install
./uncompact verify-install5. Test the hook scripts directlyThe hook scripts in # Test the Stop hook (what fires after compaction):
bash scripts/uncompact-hook.sh
# Test the SessionStart setup hook:
bash scripts/setup.sh
# Test background pregen:
bash scripts/pregen-hook.shSet export SUPERMODEL_API_KEY=your-key-here
bash scripts/uncompact-hook.sh6. Test with Claude Code (full integration)Once you've installed hooks (
Or just trigger it directly from Claude Code's terminal: to see registered hooks, then manually invoke the stop hook. Quick sanity checklist
|
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (1)
hooks/hooks.json (1)
14-21: Consider moving heavy work offStopvia lightweight pregen triggers.Given the stated 10–15 minute graph latency, a
Stophook with 120s timeout is likely to miss. A non-blockingpregentrigger (e.g., SessionStart/PostToolUse + lock) would make compaction-time injection more reliable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/hooks.json` around lines 14 - 21, The Stop hook currently runs a potentially long-running command ("bash ${CLAUDE_PLUGIN_ROOT}/scripts/uncompact-hook.sh") with a 120s timeout which may fail due to graph latency; move this heavy work out of the "Stop" hook and trigger it asynchronously via a lightweight pregen trigger (e.g., add a non-blocking "SessionStart" or "PostToolUse" pregen hook that acquires a lock and enqueues or runs the compaction/uncompact work) so the Stop entry only signals intent and the actual compaction is performed reliably off the critical path; update hooks.json to replace the blocking "Stop" invocation of uncompact-hook.sh with a short signal/lock action and create a new pregen hook that calls uncompact-hook.sh (or a wrapper) under a lock/queue to ensure single execution and retriable behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/auth.go`:
- Around line 92-93: The current code ignores the error returned by
config.ConfigFile(), so fmt.Printf may print an empty/invalid path; update the
auth command to check the error from config.ConfigFile() (the cfgFile, err :=
config.ConfigFile() call), and if err != nil log or print a clear fallback
message (e.g., "API key saved" without a path or include the error text)
otherwise print the cfgFile path using fmt.Printf; ensure you update the
variables used at lines that reference cfgFile so the error variable is handled
and surfaced to the user.
- Around line 55-64: Replace the visible stdin scanner usage (scanner :=
bufio.NewScanner(os.Stdin) and scanner.Text()) with
golang.org/x/term.ReadPassword to hide input: import "golang.org/x/term", call
term.ReadPassword(int(os.Stdin.Fd())) to read the API key bytes, handle and
return any error from ReadPassword, convert the returned []byte to string, trim
whitespace into the existing key variable, and ensure you print a newline after
ReadPassword (since it doesn't echo a newline); remove the bufio.NewScanner
usage and related error branch.
In `@cmd/run.go`:
- Around line 176-199: runWithoutCache currently always returns silentExit() on
zip/API/render failures, ignoring the user's --fallback setting; change the
error-handling paths in runWithoutCache (the zip.RepoZip, apiClient.GetGraph,
and tmpl.Render failure branches) to check cfg.Fallback (or the appropriate
fallback boolean on cfg) and, if true, return the actual error (or a clear
sentinel error) instead of silentExit() so the caller can perform the fallback
logic, otherwise keep returning silentExit(); update any callers to handle the
returned error/sentinel as the signal to run the fallback path.
- Around line 73-77: The background goroutine must not call Prune() on the same
store instance that's deferred to Close(); instead open a separate DB connection
and run Prune() on that instance and close it when done. Replace the goroutine
body to create a new store/connection (e.g., call the store factory or
connection-open method used elsewhere—refer to your constructor like
NewStore/OpenStore or a Clone method), call newStore.Prune(), handle/log any
error, and then call newStore.Close() inside the goroutine so the main deferred
store.Close() cannot race with the prune operation.
In `@cmd/status.go`:
- Line 125: The call to store.Get is currently discarding its error return
(graph, fresh, _, _ := store.Get(proj.Hash)), hiding DB/IO failures; update both
occurrences (the call at the graph/fresh assignment and the similar call around
line 228) to capture the error (e.g., graph, fresh, _, err :=
store.Get(proj.Hash)), check err for nil, and handle it appropriately by logging
the error with context (including proj.Hash) and returning or exiting with a
non-zero status instead of proceeding as if the cache is merely stale; ensure
any code paths that previously assumed success are guarded behind the nil error
check.
- Around line 40-41: Update the command help text to match actual behavior:
modify the Long description (the string next to Long:) for the status command so
it no longer says “without writing to stdout” and instead states that dry-run
prints a preview to stdout (or mentions that it writes the preview to stdout for
debugging), ensuring the text aligns with the runtime behavior implemented in
dryRunHandler which writes the preview to stdout.
In `@hooks/hooks.json`:
- Around line 2-25: SessionStart currently only has a generic hook; add explicit
matcher blocks so boot vs context-clear are distinguishable by adding two
entries under "SessionStart" with "matcher": "startup" and "matcher": "clear"
that run the existing setup command ("bash
${CLAUDE_PLUGIN_ROOT}/scripts/setup.sh"); also add a "SessionEnd" section that
contains a hook with "reason": "clear" to run the cleanup/uninjection command
("bash ${CLAUDE_PLUGIN_ROOT}/scripts/uncompact-hook.sh"), ensuring you use the
existing hook objects (type: "command", timeout fields) and the same command
strings so reinjection and cleanup run reliably for both boot and /clear flows.
In `@internal/api/client.go`:
- Around line 208-239: The pollJob function reads and unmarshals every response
body without checking HTTP status, causing auth/404 errors to be mis-parsed and
retried; update pollJob to inspect resp.StatusCode after receiving the response
(and after closing resp.Body) and handle codes explicitly: for non-retriable
client errors like 401 and 404 return an immediate error (include status and
response body text), for transient/server errors (5xx) and 429 treat them as
retryable and continue the polling loop, and only attempt json.Unmarshal into
JobStatus when status is in the 2xx range.
In `@internal/config/config.go`:
- Around line 119-126: The current ConfigFile() usage reads cfgFile but ignores
non-existence read errors; update the cfgFile read block so that after calling
os.ReadFile(cfgFile) you check the returned error and if err != nil and
!os.IsNotExist(err) return a wrapped error (include cfgFile and err) instead of
silently proceeding; only treat os.IsNotExist(err) as the empty-config case, and
keep the existing json.Unmarshal(cfg) logic when ReadFile succeeds.
In `@internal/hooks/hooks.go`:
- Around line 29-39: The current hook detection only checks for the literal
command "uncompact run" via the uncompactHooks map and so misses the
wrapper-script form used by the plugin (and causes duplicate installs); update
the hook definitions and installation check to accept both forms (the raw
Command.Command "uncompact run" and the wrapper script entry used in hooks.json)
by broadening the matcher (or adding an alternative Command entry) in
uncompactHooks and by changing the install/idempotency logic that inspects
existing Hook/Command entries to treat either form as equivalent; apply the same
change where the duplicate-ready hook set is declared/checked (the other similar
block around the second uncompact hook definition).
- Around line 158-159: The os.WriteFile call that writes settings.json currently
uses mode 0644; change it to a restrictive user-only mode 0600 to protect
user-scoped settings—update the os.WriteFile(settingsPath, finalJSON, 0644)
invocation (the call that references settingsPath and finalJSON) to use 0600
instead so the file is created readable/writable only by the owner.
- Around line 104-108: The code currently swallows json.Unmarshal errors for
settings.json and recreates rawJSON, which can silently discard unrelated
settings; instead, change the error handling in the json.Unmarshal block to fail
fast: do not reset rawJSON, wrap/return the json.Unmarshal error with context
(e.g., "invalid settings.json") from the containing function (the reader/loader
function that reads settings.json), remove the fmt.Fprintf warning/no-op
recovery, and ensure callers handle/propagate that returned error so malformed
settings.json is not overwritten; reference the json.Unmarshal call and the
rawJSON variable when making this change.
In `@internal/template/render.go`:
- Around line 55-66: The Timestamp is being labeled "UTC" but uses local time;
update the code that sets the Timestamp in the struct (the now variable and the
Timestamp field assignment in internal/template/render.go, inside the render
function) to use UTC — either set now := time.Now().UTC() or call
now.UTC().Format(...) so the formatted Timestamp truly represents UTC.
- Around line 121-149: The function can return output larger than maxTokens when
the required header alone exceeds the budget (reqTokens >= maxTokens); update
the logic in the render routine to enforce the token cap by first checking
reqTokens against maxTokens and, if it exceeds the budget, truncate the required
content to fit (taking into account any trailing suffix like the generated note
and separators) using a token-aware truncation routine or return a clear error;
otherwise keep the existing domainSections loop but ensure you always check
remaining before appending and after building the final result re-check result
token count and trim domain sections or the tail as needed so the final returned
string from the function never exceeds maxTokens (refer to variables/functions:
required, maxTokens, countTokens, remaining, buildDomainSection, domainSections,
strings.Builder).
In `@internal/zip/zip.go`:
- Around line 72-96: The walk currently opens every file entry (using
os.Open(path)) without checking for symlinks, which can follow links outside the
repo; add a symlink check immediately after the hidden-file check (i.e., inspect
info.Mode()&os.ModeSymlink) and skip the entry if it's a symlink to prevent
zipping targets outside the repo, ensuring the rest of the existing checks
(skipExts, maxFileSize, totalSize) remain unchanged and the early-return
behavior (return nil) is used for skipped symlinks.
---
Nitpick comments:
In `@hooks/hooks.json`:
- Around line 14-21: The Stop hook currently runs a potentially long-running
command ("bash ${CLAUDE_PLUGIN_ROOT}/scripts/uncompact-hook.sh") with a 120s
timeout which may fail due to graph latency; move this heavy work out of the
"Stop" hook and trigger it asynchronously via a lightweight pregen trigger
(e.g., add a non-blocking "SessionStart" or "PostToolUse" pregen hook that
acquires a lock and enqueues or runs the compaction/uncompact work) so the Stop
entry only signals intent and the actual compaction is performed reliably off
the critical path; update hooks.json to replace the blocking "Stop" invocation
of uncompact-hook.sh with a short signal/lock action and create a new pregen
hook that calls uncompact-hook.sh (or a wrapper) under a lock/queue to ensure
single execution and retriable behavior.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
README.mdcmd/auth.gocmd/install.gocmd/run.gocmd/status.gogo.modhooks/hooks.jsoninternal/api/client.gointernal/cache/store.gointernal/config/config.gointernal/hooks/hooks.gointernal/project/project.gointernal/template/render.gointernal/zip/zip.goscripts/setup.sh
🚧 Files skipped from review as they are similar to previous changes (4)
- README.md
- internal/project/project.go
- scripts/setup.sh
- go.mod
cmd/auth.go
Outdated
Comment on lines
+55
to
+64
| fmt.Print("2. Paste your API key here: ") | ||
|
|
||
| scanner := bufio.NewScanner(os.Stdin) | ||
| if !scanner.Scan() { | ||
| if err := scanner.Err(); err != nil { | ||
| return fmt.Errorf("reading API key: %w", err) | ||
| } | ||
| return fmt.Errorf("no input provided") | ||
| } | ||
| key := strings.TrimSpace(scanner.Text()) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the current code state at lines 55-64 in cmd/auth.go
head -64 cmd/auth.go | tail -15Repository: supermodeltools/Uncompact
Length of output: 542
🏁 Script executed:
# Run the verification script from the review comment
rg -n 'Paste your API key here|bufio.NewScanner\(os.Stdin\)|ReadPassword' cmd/auth.goRepository: supermodeltools/Uncompact
Length of output: 156
🏁 Script executed:
# Check the imports in cmd/auth.go to see if term is already imported
head -30 cmd/auth.goRepository: supermodeltools/Uncompact
Length of output: 644
Hide API key input while typing.
The code at lines 57–64 uses bufio.NewScanner(os.Stdin), which means your API key gets displayed on-screen as you type. That's a security issue—someone could shoulder-surf or record your screen and grab your API key.
The fix is to use golang.org/x/term.ReadPassword() instead, which hides the input. Here's the pattern:
🔐 Proposed fix
import (
- "bufio"
"context"
"fmt"
"os"
"strings"
"time"
"github.com/spf13/cobra"
"github.com/supermodeltools/uncompact/internal/api"
"github.com/supermodeltools/uncompact/internal/config"
+ "golang.org/x/term"
)
@@
fmt.Println()
fmt.Print("2. Paste your API key here: ")
- scanner := bufio.NewScanner(os.Stdin)
- if !scanner.Scan() {
- if err := scanner.Err(); err != nil {
- return fmt.Errorf("reading API key: %w", err)
- }
- return fmt.Errorf("no input provided")
- }
- key := strings.TrimSpace(scanner.Text())
+ var key string
+ if term.IsTerminal(int(os.Stdin.Fd())) {
+ b, err := term.ReadPassword(int(os.Stdin.Fd()))
+ fmt.Println()
+ if err != nil {
+ return fmt.Errorf("reading API key: %w", err)
+ }
+ key = strings.TrimSpace(string(b))
+ } else {
+ // Non-interactive fallback (e.g. piped input)
+ var b []byte
+ if _, err := fmt.Fscanln(os.Stdin, &b); err != nil {
+ return fmt.Errorf("reading API key: %w", err)
+ }
+ key = strings.TrimSpace(string(b))
+ }
if key == "" {
return fmt.Errorf("API key cannot be empty")
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/auth.go` around lines 55 - 64, Replace the visible stdin scanner usage
(scanner := bufio.NewScanner(os.Stdin) and scanner.Text()) with
golang.org/x/term.ReadPassword to hide input: import "golang.org/x/term", call
term.ReadPassword(int(os.Stdin.Fd())) to read the API key bytes, handle and
return any error from ReadPassword, convert the returned []byte to string, trim
whitespace into the existing key variable, and ensure you print a newline after
ReadPassword (since it doesn't echo a newline); remove the bufio.NewScanner
usage and related error branch.
cmd/auth.go
Outdated
Comment on lines
+92
to
+93
| cfgFile, _ := config.ConfigFile() | ||
| fmt.Printf("\nAPI key saved to: %s\n", cfgFile) |
There was a problem hiding this comment.
Don’t ignore ConfigFile() errors in user-facing output.
Line 92 discards the error, so Line 93 can print a blank/invalid path even after a successful save.
✅ Proposed fix
- cfgFile, _ := config.ConfigFile()
- fmt.Printf("\nAPI key saved to: %s\n", cfgFile)
+ if cfgFile, err := config.ConfigFile(); err == nil {
+ fmt.Printf("\nAPI key saved to: %s\n", cfgFile)
+ } else {
+ fmt.Println("\nAPI key saved.")
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| cfgFile, _ := config.ConfigFile() | |
| fmt.Printf("\nAPI key saved to: %s\n", cfgFile) | |
| if cfgFile, err := config.ConfigFile(); err == nil { | |
| fmt.Printf("\nAPI key saved to: %s\n", cfgFile) | |
| } else { | |
| fmt.Println("\nAPI key saved.") | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/auth.go` around lines 92 - 93, The current code ignores the error
returned by config.ConfigFile(), so fmt.Printf may print an empty/invalid path;
update the auth command to check the error from config.ConfigFile() (the
cfgFile, err := config.ConfigFile() call), and if err != nil log or print a
clear fallback message (e.g., "API key saved" without a path or include the
error text) otherwise print the cfgFile path using fmt.Printf; ensure you update
the variables used at lines that reference cfgFile so the error variable is
handled and surfaced to the user.
Comment on lines
+73
to
+77
| defer store.Close() | ||
|
|
||
| // Background prune (non-blocking) | ||
| go func() { _ = store.Prune() }() | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check if the file exists and examine lines around 73-77
head -100 cmd/run.go | tail -40Repository: supermodeltools/Uncompact
Length of output: 1050
🏁 Script executed:
# Also get more context with line numbers
sed -n '60,90p' cmd/run.goRepository: supermodeltools/Uncompact
Length of output: 764
🏁 Script executed:
# Search for the store variable usage in this file
rg -n 'store\.' cmd/run.go | head -30Repository: supermodeltools/Uncompact
Length of output: 350
🏁 Script executed:
# Find the cache package and store type
fd -t f '\.go$' | xargs rg -l 'func Open.*store' | head -5Repository: supermodeltools/Uncompact
Length of output: 51
🏁 Script executed:
# Search for cache package files
fd cache -type dRepository: supermodeltools/Uncompact
Length of output: 239
🏁 Script executed:
# Look for store interface or struct definition
rg -n 'type.*Store' --type go | grep -v testRepository: supermodeltools/Uncompact
Length of output: 115
🏁 Script executed:
# Read the store.go file to understand the Store type and its methods
cat -n internal/cache/store.goRepository: supermodeltools/Uncompact
Length of output: 9929
🏁 Script executed:
# Also check what cache.Open() returns
rg -n 'func Open' internal/cache/ -A 10Repository: supermodeltools/Uncompact
Length of output: 708
🏁 Script executed:
# Check if there are any wait/sync patterns or goroutine cleanup in the function
sed -n '50,200p' cmd/run.go | grep -E 'WaitGroup|sync\.|wait|Wait|chan|Chan'Repository: supermodeltools/Uncompact
Length of output: 51
🏁 Script executed:
# Verify the full function scope to see if it returns before prune could complete
rg -n 'func.*run' cmd/run.go | head -3Repository: supermodeltools/Uncompact
Length of output: 277
Open a separate connection for background pruning to avoid closing the store while it's still in use.
Here's the situation: Line 73 defers a call to store.Close() which will happen when your function returns. But line 76 launches a goroutine that calls store.Prune() using that same store handle. Since there's no synchronization waiting for the goroutine to finish before the function exits, you'll get a race condition—the Close() might fire while Prune() is still executing those DELETE queries, causing database errors.
Think of it like this: you schedule a background task on a shared database, then immediately close that database before the task finishes. That's asking for trouble.
The fix is to give the prune operation its own isolated database connection so it doesn't care when the main store closes:
Suggested fix
defer store.Close()
// Background prune (non-blocking) on an isolated DB handle.
go func(path string) {
pruneStore, err := cache.Open(path)
if err != nil {
return
}
defer pruneStore.Close()
_ = pruneStore.Prune()
}(dbPath)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/run.go` around lines 73 - 77, The background goroutine must not call
Prune() on the same store instance that's deferred to Close(); instead open a
separate DB connection and run Prune() on that instance and close it when done.
Replace the goroutine body to create a new store/connection (e.g., call the
store factory or connection-open method used elsewhere—refer to your constructor
like NewStore/OpenStore or a Clone method), call newStore.Prune(), handle/log
any error, and then call newStore.Close() inside the goroutine so the main
deferred store.Close() cannot race with the prune operation.
Comment on lines
+176
to
+199
| // runWithoutCache attempts an API fetch with no cache fallback. | ||
| func runWithoutCache(cfg *config.Config, proj *project.Info, logFn func(string, ...interface{})) error { | ||
| ctx, cancel := context.WithTimeout(context.Background(), 90*time.Second) | ||
| defer cancel() | ||
|
|
||
| zipData, err := zip.RepoZip(proj.RootDir) | ||
| if err != nil { | ||
| logFn("[warn] zip error: %v", err) | ||
| return silentExit() | ||
| } | ||
|
|
||
| apiClient := api.New(cfg.BaseURL, cfg.APIKey, debug, logFn) | ||
| graph, err := apiClient.GetGraph(ctx, proj.Name, zipData) | ||
| if err != nil { | ||
| logFn("[warn] API error: %v", err) | ||
| return silentExit() | ||
| } | ||
|
|
||
| opts := tmpl.RenderOptions{MaxTokens: maxTokens} | ||
| output, _, err := tmpl.Render(graph, proj.Name, opts) | ||
| if err != nil { | ||
| logFn("[warn] render error: %v", err) | ||
| return silentExit() | ||
| } |
There was a problem hiding this comment.
Honor --fallback in runWithoutCache.
When cache is unavailable and API/zip/render fails, this path exits silently even if fallback is enabled. That drops context unexpectedly.
🛠️ Proposed fix
func runWithoutCache(cfg *config.Config, proj *project.Info, logFn func(string, ...interface{})) error {
@@
zipData, err := zip.RepoZip(proj.RootDir)
if err != nil {
logFn("[warn] zip error: %v", err)
+ if fallback {
+ printFallback(proj.Name)
+ }
return silentExit()
}
@@
graph, err := apiClient.GetGraph(ctx, proj.Name, zipData)
if err != nil {
logFn("[warn] API error: %v", err)
+ if fallback {
+ printFallback(proj.Name)
+ }
return silentExit()
}
@@
output, _, err := tmpl.Render(graph, proj.Name, opts)
if err != nil {
logFn("[warn] render error: %v", err)
+ if fallback {
+ printFallback(proj.Name)
+ }
return silentExit()
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/run.go` around lines 176 - 199, runWithoutCache currently always returns
silentExit() on zip/API/render failures, ignoring the user's --fallback setting;
change the error-handling paths in runWithoutCache (the zip.RepoZip,
apiClient.GetGraph, and tmpl.Render failure branches) to check cfg.Fallback (or
the appropriate fallback boolean on cfg) and, if true, return the actual error
(or a clear sentinel error) instead of silentExit() so the caller can perform
the fallback logic, otherwise keep returning silentExit(); update any callers to
handle the returned error/sentinel as the signal to run the fallback path.
cmd/status.go
Outdated
Comment on lines
+40
to
+41
| Long: `dry-run shows what would be injected without writing to stdout. Useful for debugging.`, | ||
| RunE: dryRunHandler, |
There was a problem hiding this comment.
dry-run help text conflicts with runtime behavior.
The command description says “without writing to stdout,” but Line 246 writes the preview to stdout.
📝 Suggested fix (docs-side)
- Long: `dry-run shows what would be injected without writing to stdout. Useful for debugging.`,
+ Long: `dry-run shows what would be injected without performing injection. Useful for debugging.`,Also applies to: 246-246
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/status.go` around lines 40 - 41, Update the command help text to match
actual behavior: modify the Long description (the string next to Long:) for the
status command so it no longer says “without writing to stdout” and instead
states that dry-run prints a preview to stdout (or mentions that it writes the
preview to stdout for debugging), ensuring the text aligns with the runtime
behavior implemented in dryRunHandler which writes the preview to stdout.
Comment on lines
+104
to
+108
| if err := json.Unmarshal(data, &rawJSON); err != nil { | ||
| // File exists but is invalid JSON — warn and treat as empty | ||
| fmt.Fprintf(os.Stderr, "Warning: existing settings.json has invalid JSON, will recreate hooks section\n") | ||
| rawJSON = make(map[string]json.RawMessage) | ||
| } |
There was a problem hiding this comment.
Invalid settings JSON should fail fast, not be overwritten.
Right now malformed JSON triggers a warning and recreation, which can silently discard unrelated settings.
🛑 Suggested fix
if data, err := os.ReadFile(settingsPath); err == nil {
if err := json.Unmarshal(data, &rawJSON); err != nil {
- // File exists but is invalid JSON — warn and treat as empty
- fmt.Fprintf(os.Stderr, "Warning: existing settings.json has invalid JSON, will recreate hooks section\n")
- rawJSON = make(map[string]json.RawMessage)
+ return nil, fmt.Errorf("invalid settings.json at %s: %w", settingsPath, err)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/hooks/hooks.go` around lines 104 - 108, The code currently swallows
json.Unmarshal errors for settings.json and recreates rawJSON, which can
silently discard unrelated settings; instead, change the error handling in the
json.Unmarshal block to fail fast: do not reset rawJSON, wrap/return the
json.Unmarshal error with context (e.g., "invalid settings.json") from the
containing function (the reader/loader function that reads settings.json),
remove the fmt.Fprintf warning/no-op recovery, and ensure callers
handle/propagate that returned error so malformed settings.json is not
overwritten; reference the json.Unmarshal call and the rawJSON variable when
making this change.
internal/hooks/hooks.go
Outdated
Comment on lines
+158
to
+159
| if err := os.WriteFile(settingsPath, finalJSON, 0644); err != nil { | ||
| return nil, fmt.Errorf("writing settings.json: %w", err) |
There was a problem hiding this comment.
Use restrictive file mode when writing settings.json.
Line 158 uses 0644; prefer 0600 for user-scoped settings.
🔐 Suggested fix
- if err := os.WriteFile(settingsPath, finalJSON, 0644); err != nil {
+ if err := os.WriteFile(settingsPath, finalJSON, 0600); err != nil {
return nil, fmt.Errorf("writing settings.json: %w", err)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if err := os.WriteFile(settingsPath, finalJSON, 0644); err != nil { | |
| return nil, fmt.Errorf("writing settings.json: %w", err) | |
| if err := os.WriteFile(settingsPath, finalJSON, 0600); err != nil { | |
| return nil, fmt.Errorf("writing settings.json: %w", err) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/hooks/hooks.go` around lines 158 - 159, The os.WriteFile call that
writes settings.json currently uses mode 0644; change it to a restrictive
user-only mode 0600 to protect user-scoped settings—update the
os.WriteFile(settingsPath, finalJSON, 0644) invocation (the call that references
settingsPath and finalJSON) to use 0600 instead so the file is created
readable/writable only by the owner.
internal/template/render.go
Outdated
Comment on lines
+55
to
+66
| now := time.Now() | ||
|
|
||
| data := struct { | ||
| ProjectName string | ||
| Timestamp string | ||
| Graph *api.ProjectGraph | ||
| Stale bool | ||
| StaleDuration string | ||
| }{ | ||
| ProjectName: projectName, | ||
| Timestamp: now.Format("2006-01-02 15:04:05 UTC"), | ||
| Graph: graph, |
There was a problem hiding this comment.
Timestamp is labeled UTC without converting to UTC.
Line 65 appends UTC, but now is local time.
🕒 Suggested fix
- now := time.Now()
+ now := time.Now().UTC()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| now := time.Now() | |
| data := struct { | |
| ProjectName string | |
| Timestamp string | |
| Graph *api.ProjectGraph | |
| Stale bool | |
| StaleDuration string | |
| }{ | |
| ProjectName: projectName, | |
| Timestamp: now.Format("2006-01-02 15:04:05 UTC"), | |
| Graph: graph, | |
| now := time.Now().UTC() | |
| data := struct { | |
| ProjectName string | |
| Timestamp string | |
| Graph *api.ProjectGraph | |
| Stale bool | |
| StaleDuration string | |
| }{ | |
| ProjectName: projectName, | |
| Timestamp: now.Format("2006-01-02 15:04:05 UTC"), | |
| Graph: graph, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/template/render.go` around lines 55 - 66, The Timestamp is being
labeled "UTC" but uses local time; update the code that sets the Timestamp in
the struct (the now variable and the Timestamp field assignment in
internal/template/render.go, inside the render function) to use UTC — either set
now := time.Now().UTC() or call now.UTC().Format(...) so the formatted Timestamp
truly represents UTC.
Comment on lines
+121
to
+149
| reqTokens := countTokens(required) | ||
| remaining := maxTokens - reqTokens | ||
|
|
||
| var domainSections []string | ||
| for _, d := range graph.Domains { | ||
| section := buildDomainSection(d) | ||
| sectionTokens := countTokens(section) | ||
| if sectionTokens <= remaining { | ||
| domainSections = append(domainSections, section) | ||
| remaining -= sectionTokens | ||
| } | ||
| if remaining < 50 { | ||
| break | ||
| } | ||
| } | ||
|
|
||
| var sb strings.Builder | ||
| sb.WriteString(required) | ||
| if len(domainSections) > 0 { | ||
| sb.WriteString("\n\n## Domain Map\n") | ||
| for _, s := range domainSections { | ||
| sb.WriteString(s) | ||
| } | ||
| } | ||
| sb.WriteString("\n\n---\n*Generated by Uncompact (budget-truncated)*") | ||
|
|
||
| result := sb.String() | ||
| return result, countTokens(result), nil | ||
| } |
There was a problem hiding this comment.
Budget truncation can still return output over maxTokens.
If the required header already exceeds the budget (Line 122), the function still returns it unchanged, so output can violate the requested token cap.
✂️ Suggested fix
reqTokens := countTokens(required)
+ if reqTokens > maxTokens {
+ fallback := "# Uncompact Context\n\n(Budget too small; increase --max-tokens)"
+ return fallback, countTokens(fallback), nil
+ }
remaining := maxTokens - reqTokens📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| reqTokens := countTokens(required) | |
| remaining := maxTokens - reqTokens | |
| var domainSections []string | |
| for _, d := range graph.Domains { | |
| section := buildDomainSection(d) | |
| sectionTokens := countTokens(section) | |
| if sectionTokens <= remaining { | |
| domainSections = append(domainSections, section) | |
| remaining -= sectionTokens | |
| } | |
| if remaining < 50 { | |
| break | |
| } | |
| } | |
| var sb strings.Builder | |
| sb.WriteString(required) | |
| if len(domainSections) > 0 { | |
| sb.WriteString("\n\n## Domain Map\n") | |
| for _, s := range domainSections { | |
| sb.WriteString(s) | |
| } | |
| } | |
| sb.WriteString("\n\n---\n*Generated by Uncompact (budget-truncated)*") | |
| result := sb.String() | |
| return result, countTokens(result), nil | |
| } | |
| reqTokens := countTokens(required) | |
| if reqTokens > maxTokens { | |
| fallback := "# Uncompact Context\n\n(Budget too small; increase --max-tokens)" | |
| return fallback, countTokens(fallback), nil | |
| } | |
| remaining := maxTokens - reqTokens | |
| var domainSections []string | |
| for _, d := range graph.Domains { | |
| section := buildDomainSection(d) | |
| sectionTokens := countTokens(section) | |
| if sectionTokens <= remaining { | |
| domainSections = append(domainSections, section) | |
| remaining -= sectionTokens | |
| } | |
| if remaining < 50 { | |
| break | |
| } | |
| } | |
| var sb strings.Builder | |
| sb.WriteString(required) | |
| if len(domainSections) > 0 { | |
| sb.WriteString("\n\n## Domain Map\n") | |
| for _, s := range domainSections { | |
| sb.WriteString(s) | |
| } | |
| } | |
| sb.WriteString("\n\n---\n*Generated by Uncompact (budget-truncated)*") | |
| result := sb.String() | |
| return result, countTokens(result), nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/template/render.go` around lines 121 - 149, The function can return
output larger than maxTokens when the required header alone exceeds the budget
(reqTokens >= maxTokens); update the logic in the render routine to enforce the
token cap by first checking reqTokens against maxTokens and, if it exceeds the
budget, truncate the required content to fit (taking into account any trailing
suffix like the generated note and separators) using a token-aware truncation
routine or return a clear error; otherwise keep the existing domainSections loop
but ensure you always check remaining before appending and after building the
final result re-check result token count and trim domain sections or the tail as
needed so the final returned string from the function never exceeds maxTokens
(refer to variables/functions: required, maxTokens, countTokens, remaining,
buildDomainSection, domainSections, strings.Builder).
Comment on lines
+72
to
+96
| // Skip hidden files (e.g. .env, .secrets). | ||
| if strings.HasPrefix(base, ".") { | ||
| return nil | ||
| } | ||
|
|
||
| // Skip by extension | ||
| ext := strings.ToLower(filepath.Ext(path)) | ||
| if skipExts[ext] { | ||
| return nil | ||
| } | ||
|
|
||
| // Skip large files | ||
| if info.Size() > maxFileSize { | ||
| return nil | ||
| } | ||
|
|
||
| // Check total size budget | ||
| if totalSize+info.Size() > maxTotalSize { | ||
| return io.EOF // signal we're done | ||
| } | ||
|
|
||
| f, err := os.Open(path) | ||
| if err != nil { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd internal/zip && wc -l zip.go && sed -n '60,110p' zip.goRepository: supermodeltools/Uncompact
Length of output: 977
🏁 Script executed:
rg -n 'ModeSymlink|os\.Open\(path\)|filepath\.EvalSymlinks' internal/zip/zip.goRepository: supermodeltools/Uncompact
Length of output: 97
🏁 Script executed:
# Check the full context around the os.Open call and the surrounding logic
sed -n '70,100p' internal/zip/zip.goRepository: supermodeltools/Uncompact
Length of output: 598
🏁 Script executed:
# Check the WalkDir call and its options
sed -n '50,75p' internal/zip/zip.goRepository: supermodeltools/Uncompact
Length of output: 663
Skip symlinks when opening files to prevent leaking files outside the repo.
Here's the thing: filepath.Walk() in Go automatically follows symlinks (think of them as shortcuts). If someone commits a symlink inside the repo that points to a file outside the repo—like repo/link → /etc/passwd—your code will happily follow it and zip up that outside file.
Right now at line 93, you open every non-directory entry directly with os.Open(path) without checking if it's a symlink. This means sensitive files outside the repo root could accidentally leak into your zip.
The fix is straightforward: add a check for info.Mode()&os.ModeSymlink != 0 right after the hidden-file check to skip any symlinks.
Suggested fix
// Skip hidden files (e.g. .env, .secrets).
if strings.HasPrefix(base, ".") {
return nil
}
+
+ // Skip symlinks to avoid including files outside the repo root.
+ if info.Mode()&os.ModeSymlink != 0 {
+ return nil
+ }
// Skip by extension
ext := strings.ToLower(filepath.Ext(path))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Skip hidden files (e.g. .env, .secrets). | |
| if strings.HasPrefix(base, ".") { | |
| return nil | |
| } | |
| // Skip by extension | |
| ext := strings.ToLower(filepath.Ext(path)) | |
| if skipExts[ext] { | |
| return nil | |
| } | |
| // Skip large files | |
| if info.Size() > maxFileSize { | |
| return nil | |
| } | |
| // Check total size budget | |
| if totalSize+info.Size() > maxTotalSize { | |
| return io.EOF // signal we're done | |
| } | |
| f, err := os.Open(path) | |
| if err != nil { | |
| return nil | |
| } | |
| // Skip hidden files (e.g. .env, .secrets). | |
| if strings.HasPrefix(base, ".") { | |
| return nil | |
| } | |
| // Skip symlinks to avoid including files outside the repo root. | |
| if info.Mode()&os.ModeSymlink != 0 { | |
| return nil | |
| } | |
| // Skip by extension | |
| ext := strings.ToLower(filepath.Ext(path)) | |
| if skipExts[ext] { | |
| return nil | |
| } | |
| // Skip large files | |
| if info.Size() > maxFileSize { | |
| return nil | |
| } | |
| // Check total size budget | |
| if totalSize+info.Size() > maxTotalSize { | |
| return io.EOF // signal we're done | |
| } | |
| f, err := os.Open(path) | |
| if err != nil { | |
| return nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/zip/zip.go` around lines 72 - 96, The walk currently opens every
file entry (using os.Open(path)) without checking for symlinks, which can follow
links outside the repo; add a symlink check immediately after the hidden-file
check (i.e., inspect info.Mode()&os.ModeSymlink) and skip the entry if it's a
symlink to prevent zipping targets outside the repo, ensuring the rest of the
existing checks (skipExts, maxFileSize, totalSize) remain unchanged and the
early-return behavior (return nil) is used for skipped symlinks.
Contributor
Author
|
@claude fix all the coderabbit review comments and push one commit. resolve the conversations of each review comment once addressed. |
Contributor
|
Claude finished @greynewell's task in 7m 50s —— View job Fixed All CodeRabbit Review CommentsTasks:
Changes in commit
|
| File | Fix |
|---|---|
cmd/auth.go |
Use term.ReadPassword() to hide API key while typing; fallback to fmt.Fscanln for non-TTY (CI); handle ConfigFile() error before printing path |
cmd/run.go |
Prune goroutine opens its own isolated cache.Open(dbPath) to avoid data race with deferred store.Close(); runWithoutCache now calls printFallback() when --fallback is set |
cmd/status.go |
Both store.Get() call sites now capture and propagate errors; fixed dry-run Long description ("writes a preview to stdout") |
hooks/hooks.json |
SessionStart split into matcher: "startup" and matcher: "clear" entries; added SessionEnd with matcher: "clear" to re-inject context after /clear |
internal/api/client.go |
pollJob checks HTTP status before JSON parsing: 401/404 → immediate error, 429/5xx → retry loop, non-2xx → fail fast |
internal/config/config.go |
os.ReadFile errors that are NOT os.IsNotExist now return an error instead of silently proceeding |
internal/hooks/hooks.go |
isAlreadyInstalled detects both "uncompact run" and "uncompact-hook.sh" forms (idempotent across install methods); invalid settings.json JSON now fails fast instead of silently overwriting; file mode changed from 0644 → 0600 |
internal/template/render.go |
time.Now().UTC() so timestamp label matches actual value; truncateToTokenBudget now returns a short fallback message when the required header alone exceeds maxTokens |
internal/zip/zip.go |
Added symlink check (info.Mode()&os.ModeSymlink != 0) to prevent including files outside the repo root |
go.mod |
Added golang.org/x/term v0.22.0 as direct dependency; golang.org/x/sys promoted from indirect to direct |
Note: Run
go mod tidybefore building locally to regeneratego.sumwith the newgolang.org/x/termchecksums.
Note on conversation resolution: PR review thread resolution via the GitHub API requires user approval in this environment. Please resolve the CodeRabbit review comment threads manually or trigger a new CodeRabbit review with
@coderabbitai reviewto confirm all issues are addressed.
· Branch
- cmd/auth.go: use term.ReadPassword to hide API key input; handle ConfigFile() error gracefully in user-facing output - cmd/run.go: open isolated DB connection for background Prune() goroutine to avoid data race with deferred store.Close(); honour --fallback flag in runWithoutCache on zip/API/render failures - cmd/status.go: capture and handle store.Get() errors at both call sites; fix dry-run Long description (it does write to stdout) - hooks/hooks.json: split SessionStart into startup/clear matchers; add SessionEnd with clear matcher for context-reinjection after /clear - internal/api/client.go: check HTTP status before unmarshalling poll response — fail fast on 401/404, retry on 429/5xx - internal/config/config.go: return error for non-NotExist ReadFile failures instead of silently ignoring them - internal/hooks/hooks.go: broaden isAlreadyInstalled to detect uncompact-hook.sh wrapper form (idempotent across install methods); fail fast on invalid settings.json JSON instead of silently recreating; use mode 0600 when writing settings.json - internal/template/render.go: use time.Now().UTC() so timestamp label matches actual value; guard truncateToTokenBudget against required header exceeding token budget - internal/zip/zip.go: skip symlinks to prevent including files outside repo root - go.mod: add golang.org/x/term v0.22.0 as direct dependency Run 'go mod tidy' before building to regenerate go.sum. Co-authored-by: Grey Newell <greynewell@users.noreply.github.com>
…chema - Add Idempotency-Key header to all requests (required by API) - Switch auth header from Authorization: Bearer to X-Api-Key - Fix ValidateKey to probe GET /v1/graphs/supermodel (405=valid, /auth/me is OAuth-only) - Fix multipart field name: "file" not "repo" - Fix async polling: re-POST with same idempotency key (server returns job status) - Fix jobId field: camelCase "jobId" not snake_case "job_id" - Fix status strings: "completed"/"failed" not "complete"/"error" - Fix result field: "result" not "graph" in JobStatus envelope - Add SupermodelIR struct matching actual API response schema (SIR 1.2.0) - Add toProjectGraph() to map SupermodelIR → internal ProjectGraph model - Honor retryAfter from API response for poll backoff - Increase max poll timeout to 900s / 90 attempts (matches SDK defaults) - Downgrade modernc.org/sqlite v1.37.0 → v1.34.2 (drops unavailable GitLab dep) - Promote github.com/google/uuid to direct dependency Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- cmd/auth.go: use term.ReadPassword() to hide API key input; handle ConfigFile() errors - cmd/run.go: fix prune race condition (separate DB connection); honor --fallback in runWithoutCache - cmd/status.go: handle discarded store.Get errors; fix dry-run description - internal/hooks/hooks.go: fail-fast on invalid settings.json; detect uncompact-hook.sh; use 0600 file mode - internal/zip/zip.go: skip symlinks to prevent path traversal outside repo root - internal/config/config.go: return error on non-IsNotExist config read failures - internal/template/render.go: use UTC timestamp; add budget overflow guard - internal/project/project.go: fix hash comment (16 hex chars not 8) - hooks/hooks.json: add SessionStart startup/clear matchers; add SessionEnd section Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
Author
CodeRabbit Review Resolution SummaryAll actionable CodeRabbit comments have been addressed across two commits on this branch. ✅ Fixed in this session
✅ Already correct (no action needed)
📝 Notes
|
This was referenced Feb 27, 2026
Closed
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.
Closes #1
Full Go CLI implementation of Uncompact, synthesizing all context from the issue thread.
Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Documentation