Skip to content

feat: wire dead-code to /v1/analysis/dead-code endpoint#23

Merged
greynewell merged 3 commits intosupermodeltools:mainfrom
jonathanpopham:feat/deadcode-api-endpoint
Apr 2, 2026
Merged

feat: wire dead-code to /v1/analysis/dead-code endpoint#23
greynewell merged 3 commits intosupermodeltools:mainfrom
jonathanpopham:feat/deadcode-api-endpoint

Conversation

@jonathanpopham
Copy link
Copy Markdown
Contributor

@jonathanpopham jonathanpopham commented Apr 2, 2026

Closes #22

Summary

Replaces naive graph-edge counting with the API's dedicated dead code analysis endpoint (POST /v1/analysis/dead-code), which provides multi-phase reachability, transitive propagation, confidence levels, line numbers, and reasons.

Before (main)

FUNCTION       FILE
--------       ----
__add__
__aenter__
...
717 unreachable function(s) found.

No confidence, no line numbers, no reasons, no file paths on most results. Just "no incoming calls edge" — misses transitive dead code, framework entry points, etc.

After (this PR)

$ supermodel dead-code --min-confidence high --limit 10

FILE                                                LINE  FUNCTION            CONFIDENCE  REASON
----                                                ----  --------            ----------  ------
src/control-plane/.../decorators/MeteredUsage.java  9     MeteredUsage        high        Type/interface with no references
src/control-plane/.../RequiresScopes.java           8     RequiresScopes      high        Type/interface with no references
src/dashboard/components/ui/alert.tsx               25    Alert               high        Exported but file never imported
src/dashboard/components/ui/badge.tsx               30    Badge               high        Exported but file never imported
...
10 dead code candidate(s) out of 1131 total declarations.

Changes

  • internal/api/client.go — add DeadCode() method with async polling, extract postZipTo() helper
  • internal/api/types.go — add DeadCodeResult, DeadCodeCandidate, AliveCode, EntryPoint types
  • internal/deadcode/handler.go — rewritten to call API endpoint, display rich results
  • internal/deadcode/zip.go — zip helper (VSA, copied from analyze slice)
  • cmd/deadcode.go — new flags: --min-confidence, --limit; removed --include-exports
  • internal/deadcode/handler_test.go — updated to test new output format

Test plan

  • go test ./... — all pass
  • Live test: supermodel dead-code --min-confidence high --limit 10 returns rich results
  • JSON output: supermodel dead-code -o json returns structured data

Summary by CodeRabbit

  • New Features

    • Dead-code analysis now runs via server-side static analysis, returning confidence-scored candidates with line numbers, reasons, human table and JSON output.
  • Changes

    • Removed --include-exports; added --min-confidence, --limit and repeatable --ignore (glob) flags.
    • Human table includes FILE, LINE, FUNCTION, CONFIDENCE and REASON.
    • Config loading now honors environment-based API overrides.
  • Tests

    • Tests updated for glob matching, filtering, human/JSON output and summary counts.

Replace naive graph-edge counting with the API's dedicated dead code
analysis endpoint which provides multi-phase reachability, transitive
propagation, confidence levels, line numbers, and reasons.

New flags: --min-confidence (high/medium/low), --limit
Removed: --include-exports (handled by API)

Closes supermodeltools#22
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 2, 2026

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: b0a9a301-1c92-461d-b481-cce0292ea20a

📥 Commits

Reviewing files that changed from the base of the PR and between cb1d17f and 0afcbce.

📒 Files selected for processing (6)
  • cmd/deadcode.go
  • internal/config/config.go
  • internal/config/config_test.go
  • internal/deadcode/glob.go
  • internal/deadcode/handler.go
  • internal/deadcode/handler_test.go

Walkthrough

The dead-code CLI was reworked to zip the repository, POST to /v1/analysis/dead-code (with min_confidence/limit), poll until completion, apply optional ignore globs, and render rich results (file, line, function, confidence, reason) or JSON.

Changes

Cohort / File(s) Summary
CLI Command
cmd/deadcode.go
Updated descriptions and flags: removed --include-exports; added --min-confidence (string), --limit (int), --ignore (repeatable string glob); help text explains remote multi-phase analysis.
API Client
internal/api/client.go
Added Client.DeadCode(ctx, zipPath, idempotencyKey, minConfidence, limit) and postZipTo(...); DeadCode uploads ZIP to /v1/analysis/dead-code with query params and polls until job completes; postZip delegates to postZipTo.
API Types
internal/api/types.go
Added DeadCodeResult, DeadCodeMetadata, DeadCodeCandidate, AliveCode, and EntryPoint types with JSON mappings for the dead-code endpoint response.
Handler & Logic
internal/deadcode/handler.go
Replaced local call-graph logic with remote analysis flow: Options now has MinConfidence, Limit, Ignore (removed IncludeExports); handler calls client.DeadCode, prints api.DeadCodeResult, and post-filters candidates by glob.
Glob Matching
internal/deadcode/glob.go
New matchGlob(pattern, filePath) with segment-aware matching supporting *, ?, [...], and ** backtracking used by filterIgnored.
ZIP Archival
internal/deadcode/zip.go
New createZip(dir) preferring git archive, falling back to filesystem walk; skips hidden files, configured skip dirs, and files >10MB; cleans up on errors.
Tests
internal/deadcode/handler_test.go
Removed old entry-point/graph tests; added unit tests for glob matching, filterIgnored, printResults (human table, JSON, empty-case, line-number behavior) and sampleResult() helpers.
Config Env
internal/config/config.go, internal/config/config_test.go
Load() now applies env overrides (SUPERMODEL_API_KEY, SUPERMODEL_API_BASE); tests set those envs to empty for determinism.

Sequence Diagram

sequenceDiagram
    participant CLI as CLI
    participant Handler as deadcode.Handler
    participant Zip as deadcode.zip
    participant Client as api.Client
    participant API as Remote API

    CLI->>Handler: run dead-code (opts)
    Handler->>Zip: createZip(targetDir)
    alt git repo
        Zip->>Zip: git archive -> temp.zip
    else filesystem fallback
        Zip->>Zip: walk files -> temp.zip
    end
    Zip-->>Handler: zipPath
    Handler->>Client: DeadCode(ctx, zipPath, idempotencyKey, minConfidence, limit)
    Client->>API: POST /v1/analysis/dead-code (upload)
    API-->>Client: {jobId, status: "pending"}
    loop poll until done
        Client->>API: POST /v1/analysis/dead-code (repost jobId)
        API-->>Client: {status: "processing"|"completed", result?}
    end
    Client-->>Handler: DeadCodeResult
    Handler->>Handler: filterIgnored(result, patterns)
    Handler->>CLI: printResults(table or JSON)
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Poem

Zip, post, poll — the scanner hums away,
Files and lines return with confidence to say.
Reasons neatly listed, ignores sifted clean,
Dead-code candidates shown — a tidy scene. ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title directly and accurately describes the main change: wiring the dead-code command to use the API endpoint instead of naive graph counting.
Description check ✅ Passed The PR description follows the template with all required sections: summary of the change, context with before/after examples, specific code changes, and a comprehensive test plan.
Linked Issues check ✅ Passed The PR successfully implements all coding requirements from issue #22: new API method with async polling, response types, endpoint integration, rich CLI output with confidence/line numbers, and flag support.
Out of Scope Changes check ✅ Passed All changes are directly scoped to requirements in issue #22; no unrelated modifications detected. The zip helper code is appropriately included to support the endpoint integration.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
⚔️ Resolve merge conflicts
  • Resolve merge conflict in branch feat/deadcode-api-endpoint

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (7)
internal/deadcode/handler_test.go (2)

29-34: Minor: Simplify string checking

You've got out as a string already, so you can use strings.Contains(out, "unusedHelper") instead of converting to []byte twice. It's just a tiny bit cleaner:

// Instead of:
bytes.Contains([]byte(out), []byte("unusedHelper"))

// You could do:
strings.Contains(out, "unusedHelper")

Same thing on line 32. Not a big deal though!

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@internal/deadcode/handler_test.go` around lines 29 - 34, Replace the
redundant bytes.Contains([]byte(out), []byte("...")) checks in the test (the
variable out is a string) with strings.Contains(out, "..."); update the two
occurrences that check for "unusedHelper" and "2 dead code candidate(s)" and
ensure the test imports the "strings" package (and remove the "bytes" import if
it becomes unused).

37-51: Consider adding a JSON output test

You've got TestPrintResults_Human and TestPrintResults_Empty, but no test for the JSON format path (ui.FormatJSON). Since the handler supports both output modes, it'd be nice to have a quick test that verifies JSON output works too. Something like:

func TestPrintResults_JSON(t *testing.T) {
    result := &api.DeadCodeResult{
        DeadCodeCandidates: []api.DeadCodeCandidate{
            {File: "src/utils.ts", Line: 8, Name: "unusedHelper"},
        },
    }
    var buf bytes.Buffer
    err := printResults(&buf, result, ui.FormatJSON)
    if err != nil {
        t.Fatal(err)
    }
    if !strings.Contains(buf.String(), `"unusedHelper"`) {
        t.Errorf("expected JSON output with unusedHelper")
    }
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@internal/deadcode/handler_test.go` around lines 37 - 51, Add a new unit test
that exercises the JSON output path of printResults: create a DeadCodeResult
with at least one api.DeadCodeCandidate (e.g., File "src/utils.ts", Line 8, Name
"unusedHelper"), write to a bytes.Buffer, call printResults(&buf, result,
ui.FormatJSON), check err is nil, and assert the buffer string contains the
candidate name (e.g., "unusedHelper"); place the test alongside
TestPrintResults_Human/TestPrintResults_Empty and name it TestPrintResults_JSON
so it verifies the JSON formatting branch.
internal/api/client.go (2)

91-99: Consider using url.Values for query string building

Building query strings manually works, but it can get tricky with special characters. Using Go's net/url package is safer and more idiomatic:

import "net/url"

// ...
endpoint := deadCodeEndpoint
params := url.Values{}
if minConfidence != "" {
    params.Set("min_confidence", minConfidence)
}
if limit > 0 {
    params.Set("limit", fmt.Sprintf("%d", limit))
}
if len(params) > 0 {
    endpoint += "?" + params.Encode()
}

This handles URL encoding automatically. For your current use case (where values are just high/medium/low and integers), the manual approach works fine—but url.Values is a good habit.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@internal/api/client.go` around lines 91 - 99, The current manual query string
construction around variables endpoint, deadCodeEndpoint, minConfidence and
limit can produce incorrect encoding/escaping for special characters; replace
the manual concat logic with net/url's url.Values: create params :=
url.Values{}, call params.Set("min_confidence", minConfidence) and
params.Set("limit", fmt.Sprintf("%d", limit)) when applicable, then append "?" +
params.Encode() to endpoint only if params is non-empty so query encoding is
handled safely and cleanly.

106-121: Polling loop is duplicated from Analyze

The polling logic here (lines 106-121) is nearly identical to lines 48-63 in Analyze. That's not terrible for just 2 usages, but if you add more async endpoints later, you might want to extract a helper like:

func (c *Client) pollUntilDone(ctx context.Context, poll func() (*JobResponse, error)) (*JobResponse, error)

Just something to keep in mind for the future—not a blocker for this PR!

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@internal/api/client.go` around lines 106 - 121, The polling logic duplicated
between Analyze and the postZipTo loop should be extracted into a reusable
helper on Client; create a method like pollUntilDone(ctx context.Context, poll
func() (*JobResponse, error)) (*JobResponse, error) that encapsulates the
wait/retry/select loop and error handling, then replace the polling blocks in
Analyze and the postZipTo caller with calls to c.pollUntilDone passing a closure
that invokes postZipTo (or the original Analyze poll function) so both paths
reuse the same logic and return the final *JobResponse and error.
cmd/deadcode.go (1)

41-42: Consider validating --min-confidence values

Right now --min-confidence accepts any string, but the help text says it should be high, medium, or low. If someone typos it like --min-confidence=hgih, the API might return an error or (worse) silently ignore it.

You could add a quick validation in the RunE function:

if opts.MinConfidence != "" {
    switch opts.MinConfidence {
    case "high", "medium", "low":
        // ok
    default:
        return fmt.Errorf("invalid --min-confidence value %q: must be high, medium, or low", opts.MinConfidence)
    }
}

Same idea for --limit if negative values don't make sense (though the API probably handles that gracefully).

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@cmd/deadcode.go` around lines 41 - 42, Validate the CLI flag values before
executing the command: in the command's RunE (where opts is used) check
opts.MinConfidence when non-empty and reject anything other than "high",
"medium", or "low" with a clear error (e.g., return fmt.Errorf("invalid
--min-confidence value %q: must be high, medium, or low", opts.MinConfidence));
also validate opts.Limit to ensure it's non-negative (return an error when
negative) so the flags registered with c.Flags().StringVar(&opts.MinConfidence,
...) and c.Flags().IntVar(&opts.Limit, ...) are validated early and reliably.
internal/deadcode/handler.go (1)

33-36: Hash is computed but only used for idempotency key

You're calling cache.HashFile(zipPath) here, but the hash is only used to build the idempotency key ("deadcode-"+hash[:16]). That's fine! But it means the cache import is a bit misleading—readers might expect caching behavior.

If caching isn't planned, consider inlining a simpler hash computation or adding a comment explaining that the hash is just for idempotency, not caching.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@internal/deadcode/handler.go` around lines 33 - 36, The code calls
cache.HashFile(zipPath) only to build the idempotency key
("deadcode-"+hash[:16]), which makes the cache import misleading; either replace
cache.HashFile with an inline/simple hash helper (e.g., compute SHA256 of
zipPath or file contents in a small helper) and remove the cache import, or keep
using cache.HashFile but add a clear comment next to the HashFile call (and
above the idempotency key construction) stating the hash is used solely for
idempotency and not for caching; locate the call to cache.HashFile(zipPath) and
the idempotency key construction ("deadcode-"+hash[:16]) to apply the change.
internal/deadcode/zip.go (1)

44-48: Swallowed error might hide useful info

When gitArchive fails, the error is silently discarded and you fall back to walkZip. That's a reasonable strategy! But it might be helpful to log the error at debug level so folks can understand why git archive didn't work (e.g., dirty working tree, detached HEAD, git not installed).

Something like:

if err := gitArchive(dir, dest); err == nil {
    return dest, nil
}
// Could add: log.Debug("git archive failed, falling back to walk: %v", err)

Not a blocker—just something that could save debugging time later.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@internal/deadcode/zip.go` around lines 44 - 48, The gitArchive error is
currently discarded when isGitRepo(dir) is true, making failures opaque; update
the block that calls gitArchive(dir, dest) to log the returned error at debug
(or trace) level before falling back to walkZip so callers can see why git
archive failed. Specifically, in the function using isGitRepo, gitArchive and
walkZip, capture the error from gitArchive and call the package logger (or a
passed logger) with a debug message including the error string, then continue to
the existing fallback to walkZip.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@internal/api/client.go`:
- Line 90: The function signature for Client.DeadCode has consecutive string
parameters `idempotencyKey string, minConfidence string` which the linter flags;
change the signature to combine them as `idempotencyKey, minConfidence string`
in the DeadCode method on type Client, and update any callers, interfaces, and
tests that reference `DeadCode` to use the new signature form so the parameter
types remain identical but declared in the combined style; ensure any generated
docs or usages (e.g., method declarations elsewhere) are updated accordingly.

In `@internal/deadcode/handler.go`:
- Line 17: The Options struct's Force bool is unused in Run; either implement
caching using the same pattern as internal/analyze/handler.go or remove the
flag/field and CLI wiring in cmd/deadcode.go. To implement caching: in the Run
function (deadcode handler) check if !opts.Force then call cache.Get(hash) and
return the cached result if present, and after obtaining results from the API
call call cache.Put(hash, result); ensure you reference Options.Force, the Run
function, and use cache.Get/cache.Put consistent with
internal/analyze/handler.go. If you choose removal: delete Force from Options
and remove the --force flag handling in cmd/deadcode.go and any references in
Run.

In `@internal/deadcode/zip.go`:
- Around line 82-110: The current filepath.Walk callback does not skip symlinks
which can traverse outside the repo or cause loops; update the traversal to
detect and skip symlinks—either switch to filepath.WalkDir and use
fs.DirEntry.Type()/entry.Type() to check for os.ModeSymlink, or keep
filepath.Walk and test info.Mode()&os.ModeSymlink != 0 at the start of the
callback and return nil (or filepath.SkipDir for symlinked dirs) to skip them;
ensure this check is performed before opening files (before os.Open(path) and
zw.Create calls) so symlinked files and directories are not added to the zip.

---

Nitpick comments:
In `@cmd/deadcode.go`:
- Around line 41-42: Validate the CLI flag values before executing the command:
in the command's RunE (where opts is used) check opts.MinConfidence when
non-empty and reject anything other than "high", "medium", or "low" with a clear
error (e.g., return fmt.Errorf("invalid --min-confidence value %q: must be high,
medium, or low", opts.MinConfidence)); also validate opts.Limit to ensure it's
non-negative (return an error when negative) so the flags registered with
c.Flags().StringVar(&opts.MinConfidence, ...) and c.Flags().IntVar(&opts.Limit,
...) are validated early and reliably.

In `@internal/api/client.go`:
- Around line 91-99: The current manual query string construction around
variables endpoint, deadCodeEndpoint, minConfidence and limit can produce
incorrect encoding/escaping for special characters; replace the manual concat
logic with net/url's url.Values: create params := url.Values{}, call
params.Set("min_confidence", minConfidence) and params.Set("limit",
fmt.Sprintf("%d", limit)) when applicable, then append "?" + params.Encode() to
endpoint only if params is non-empty so query encoding is handled safely and
cleanly.
- Around line 106-121: The polling logic duplicated between Analyze and the
postZipTo loop should be extracted into a reusable helper on Client; create a
method like pollUntilDone(ctx context.Context, poll func() (*JobResponse,
error)) (*JobResponse, error) that encapsulates the wait/retry/select loop and
error handling, then replace the polling blocks in Analyze and the postZipTo
caller with calls to c.pollUntilDone passing a closure that invokes postZipTo
(or the original Analyze poll function) so both paths reuse the same logic and
return the final *JobResponse and error.

In `@internal/deadcode/handler_test.go`:
- Around line 29-34: Replace the redundant bytes.Contains([]byte(out),
[]byte("...")) checks in the test (the variable out is a string) with
strings.Contains(out, "..."); update the two occurrences that check for
"unusedHelper" and "2 dead code candidate(s)" and ensure the test imports the
"strings" package (and remove the "bytes" import if it becomes unused).
- Around line 37-51: Add a new unit test that exercises the JSON output path of
printResults: create a DeadCodeResult with at least one api.DeadCodeCandidate
(e.g., File "src/utils.ts", Line 8, Name "unusedHelper"), write to a
bytes.Buffer, call printResults(&buf, result, ui.FormatJSON), check err is nil,
and assert the buffer string contains the candidate name (e.g., "unusedHelper");
place the test alongside TestPrintResults_Human/TestPrintResults_Empty and name
it TestPrintResults_JSON so it verifies the JSON formatting branch.

In `@internal/deadcode/handler.go`:
- Around line 33-36: The code calls cache.HashFile(zipPath) only to build the
idempotency key ("deadcode-"+hash[:16]), which makes the cache import
misleading; either replace cache.HashFile with an inline/simple hash helper
(e.g., compute SHA256 of zipPath or file contents in a small helper) and remove
the cache import, or keep using cache.HashFile but add a clear comment next to
the HashFile call (and above the idempotency key construction) stating the hash
is used solely for idempotency and not for caching; locate the call to
cache.HashFile(zipPath) and the idempotency key construction
("deadcode-"+hash[:16]) to apply the change.

In `@internal/deadcode/zip.go`:
- Around line 44-48: The gitArchive error is currently discarded when
isGitRepo(dir) is true, making failures opaque; update the block that calls
gitArchive(dir, dest) to log the returned error at debug (or trace) level before
falling back to walkZip so callers can see why git archive failed. Specifically,
in the function using isGitRepo, gitArchive and walkZip, capture the error from
gitArchive and call the package logger (or a passed logger) with a debug message
including the error string, then continue to the existing fallback to walkZip.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 0eda1d17-7361-4fed-a17f-fe8dffdb65f2

📥 Commits

Reviewing files that changed from the base of the PR and between 2281ea3 and fad1a04.

📒 Files selected for processing (6)
  • cmd/deadcode.go
  • internal/api/client.go
  • internal/api/types.go
  • internal/deadcode/handler.go
  • internal/deadcode/handler_test.go
  • internal/deadcode/zip.go


// DeadCode uploads a repository ZIP and runs dead code analysis,
// polling until the async job completes and returning the result.
func (c *Client) DeadCode(ctx context.Context, zipPath, idempotencyKey string, minConfidence string, limit int) (*DeadCodeResult, error) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Fix: Combine consecutive string parameters (pipeline failure)

The linter is unhappy because you have idempotencyKey string, minConfidence string when they could be combined. This is causing the CI to fail.

🔧 Proposed fix
-func (c *Client) DeadCode(ctx context.Context, zipPath, idempotencyKey string, minConfidence string, limit int) (*DeadCodeResult, error) {
+func (c *Client) DeadCode(ctx context.Context, zipPath, idempotencyKey, minConfidence string, limit int) (*DeadCodeResult, error) {
📝 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
func (c *Client) DeadCode(ctx context.Context, zipPath, idempotencyKey string, minConfidence string, limit int) (*DeadCodeResult, error) {
func (c *Client) DeadCode(ctx context.Context, zipPath, idempotencyKey, minConfidence string, limit int) (*DeadCodeResult, error) {
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@internal/api/client.go` at line 90, The function signature for
Client.DeadCode has consecutive string parameters `idempotencyKey string,
minConfidence string` which the linter flags; change the signature to combine
them as `idempotencyKey, minConfidence string` in the DeadCode method on type
Client, and update any callers, interfaces, and tests that reference `DeadCode`
to use the new signature form so the parameter types remain identical but
declared in the combined style; ensure any generated docs or usages (e.g.,
method declarations elsewhere) are updated accordingly.

Force bool // bypass cache
Output string // "human" | "json"
IncludeExports bool // include exported (public) functions in results
Force bool // bypass cache
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check if --force flag is still wired up in cmd/deadcode.go
rg -n "Force" cmd/deadcode.go internal/deadcode/handler.go

Repository: supermodeltools/cli

Length of output: 244


🏁 Script executed:

cat -n internal/deadcode/handler.go | head -60

Repository: supermodeltools/cli

Length of output: 2024


🏁 Script executed:

cat -n internal/analyze/handler.go | head -80

Repository: supermodeltools/cli

Length of output: 2820


🏁 Script executed:

grep -rn "cache\|Force" cmd/deadcode.go internal/deadcode/ --include="*.go" -B 2 -A 2

Repository: supermodeltools/cli

Length of output: 2029


The Force flag doesn't actually do anything

So here's the thing: you've got Force bool in Options (line 17) and the CLI flag is wired up in cmd/deadcode.go saying "re-analyze even if a cached result exists". But if you look at the Run function, you never actually check this flag. Plus, there's no cache.Get() or cache.Put() calls anywhere—you just hash the file and use that hash for the idempotency key.

Compare that to internal/analyze/handler.go, which does the full caching dance:

if !force {
    if g, _ := cache.Get(hash); g != nil {
        return g, hash, nil  // use cached result
    }
}
// ... call API ...
cache.Put(hash, g)  // save the result

So either:

  1. Caching should be implemented: Copy the pattern from the analyze handler and actually store/retrieve dead-code results
  2. Or the flag should be removed: If dead-code analysis shouldn't be cached (maybe results are too volatile?), then ditch the Force field and the --force CLI flag so you're not promising something that doesn't exist

What was the intention here?

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@internal/deadcode/handler.go` at line 17, The Options struct's Force bool is
unused in Run; either implement caching using the same pattern as
internal/analyze/handler.go or remove the flag/field and CLI wiring in
cmd/deadcode.go. To implement caching: in the Run function (deadcode handler)
check if !opts.Force then call cache.Get(hash) and return the cached result if
present, and after obtaining results from the API call call cache.Put(hash,
result); ensure you reference Options.Force, the Run function, and use
cache.Get/cache.Put consistent with internal/analyze/handler.go. If you choose
removal: delete Force from Options and remove the --force flag handling in
cmd/deadcode.go and any references in Run.

Comment on lines +82 to +110
return filepath.Walk(dir, func(path string, info os.FileInfo, err error) error {
if err != nil {
return err
}
rel, err := filepath.Rel(dir, path)
if err != nil {
return err
}
if info.IsDir() {
if skipDirs[info.Name()] {
return filepath.SkipDir
}
return nil
}
if strings.HasPrefix(info.Name(), ".") || info.Size() > 10<<20 {
return nil
}
w, err := zw.Create(filepath.ToSlash(rel))
if err != nil {
return err
}
f, err := os.Open(path)
if err != nil {
return err
}
defer f.Close()
_, err = io.Copy(w, f)
return err
})
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Heads up: symlinks could cause surprises

filepath.Walk follows symlinks by default. If someone has a symlink pointing outside the repo (or worse, a symlink loop), this could either include unexpected files or hang.

A couple options:

  1. Use filepath.WalkDir (Go 1.16+) with fs.DirEntry.Type() to detect symlinks and skip them
  2. Check info.Mode()&os.ModeSymlink != 0 and skip

Here's a quick fix if you want to skip symlinks:

🛡️ Proposed fix to skip symlinks
 return filepath.Walk(dir, func(path string, info os.FileInfo, err error) error {
     if err != nil {
         return err
     }
+    // Skip symlinks to avoid following links outside the repo or loops
+    if info.Mode()&os.ModeSymlink != 0 {
+        return nil
+    }
     rel, err := filepath.Rel(dir, 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
return filepath.Walk(dir, func(path string, info os.FileInfo, err error) error {
if err != nil {
return err
}
rel, err := filepath.Rel(dir, path)
if err != nil {
return err
}
if info.IsDir() {
if skipDirs[info.Name()] {
return filepath.SkipDir
}
return nil
}
if strings.HasPrefix(info.Name(), ".") || info.Size() > 10<<20 {
return nil
}
w, err := zw.Create(filepath.ToSlash(rel))
if err != nil {
return err
}
f, err := os.Open(path)
if err != nil {
return err
}
defer f.Close()
_, err = io.Copy(w, f)
return err
})
return filepath.Walk(dir, func(path string, info os.FileInfo, err error) error {
if err != nil {
return err
}
// Skip symlinks to avoid following links outside the repo or loops
if info.Mode()&os.ModeSymlink != 0 {
return nil
}
rel, err := filepath.Rel(dir, path)
if err != nil {
return err
}
if info.IsDir() {
if skipDirs[info.Name()] {
return filepath.SkipDir
}
return nil
}
if strings.HasPrefix(info.Name(), ".") || info.Size() > 10<<20 {
return nil
}
w, err := zw.Create(filepath.ToSlash(rel))
if err != nil {
return err
}
f, err := os.Open(path)
if err != nil {
return err
}
defer f.Close()
_, err = io.Copy(w, f)
return err
})
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@internal/deadcode/zip.go` around lines 82 - 110, The current filepath.Walk
callback does not skip symlinks which can traverse outside the repo or cause
loops; update the traversal to detect and skip symlinks—either switch to
filepath.WalkDir and use fs.DirEntry.Type()/entry.Type() to check for
os.ModeSymlink, or keep filepath.Walk and test info.Mode()&os.ModeSymlink != 0
at the start of the callback and return nil (or filepath.SkipDir for symlinked
dirs) to skip them; ensure this check is performed before opening files (before
os.Open(path) and zw.Create calls) so symlinked files and directories are not
added to the zip.

jonathanpopham and others added 2 commits April 2, 2026 15:37
- dead-code: --ignore <glob> filters candidates client-side (repeatable,
  supports ** across segments). Implemented without new dependencies.
- config: SUPERMODEL_API_KEY and SUPERMODEL_API_BASE env vars override
  config file values, enabling CI/CD usage without a config file.
- dead-code summary line now reflects post-filter candidate count.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat: wire dead-code command to /v1/analysis/dead-code endpoint

2 participants