feat: show circular dependency warning in context bomb#18
Merged
greynewell merged 1 commit intomainfrom Feb 27, 2026
Merged
Conversation
WalkthroughAdds fetching of circular-dependency data alongside project graphs: a helper wraps graph retrieval, polls a circular-dependency endpoint, merges cycle counts into Changes
Sequence DiagramsequenceDiagram
participant Handler as Handler
participant Helper as fetchGraphWithCircularDeps
participant Client as API Client
participant GraphEP as GetGraph Endpoint
participant CircularEP as CircularDeps Endpoint
participant Cache as Cache
Handler->>Helper: fetchGraphWithCircularDeps(ctx, client, projectName, repoZip, logFn)
Helper->>Client: GetGraph(ctx, projectName, repoZip)
Client->>GraphEP: submit & poll for graph
GraphEP-->>Client: Graph data
Client-->>Helper: Graph
Helper->>Client: GetCircularDependencies(ctx, projectName, repoZip)
Client->>CircularEP: submit & poll for cycles
CircularEP-->>Client: CircularDependencyResponse / error
Client-->>Helper: Cycles data (or error)
Helper->>Helper: merge cycles -> graph.Stats.CircularDependencyCycles
Helper-->>Handler: enhanced graph
Handler->>Cache: store graph (with cycles)
Handler->>Handler: render template (includes cycle line if present)
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/api/client.go (2)
361-363: Preserve backend error messages in non-success responses.The generic
API error %ddrops useful server context, which makes incidents harder to debug.🛠️ Suggested error handling parity with
GetGraphcase http.StatusOK, http.StatusAccepted: // Continue to parse default: - return nil, fmt.Errorf("API error %d", resp.StatusCode) + var errResp struct { + Message string `json:"message"` + Error string `json:"error"` + } + _ = json.Unmarshal(respBody, &errResp) + msg := errResp.Message + if msg == "" { + msg = errResp.Error + } + if msg == "" { + msg = string(respBody) + } + return nil, fmt.Errorf("API error %d: %s", resp.StatusCode, msg) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/client.go` around lines 361 - 363, The default error branch in the response handling currently returns a generic error ("API error %d"); update this to preserve the backend error message similar to GetGraph by reading the response body (e.g., via resp.Body) and including its contents in the returned error (combine a short context like "API error <status>" with the backend message). Ensure you close or drain resp.Body and handle read errors gracefully so the function (the switch/default in internal/api/client.go) returns an error containing the server-provided text rather than only the status code.
293-296: Docstring and runtime behavior differ for the “no cycles” case.Right now the function only returns
nil, nilfor unavailable endpoints; successful "no cycles" responses return a non-nil empty payload. Either behavior is fine, but the comment should match what the code does.✏️ Suggested doc fix
-// and returns the list of detected import cycles. Returns nil, nil if the endpoint -// is unavailable or returns no cycles — callers should treat this as "no data". +// and returns the list of detected import cycles. Returns nil, nil if the endpoint +// is unavailable. If available but no cycles are found, returns an empty response.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/client.go` around lines 293 - 296, Update the docstring for GetCircularDependencies to match its runtime behavior: state that the function returns a non-nil (empty) CircularDependencyResponse when the endpoint successfully reports no cycles, and only returns nil, nil when the endpoint is unavailable or cannot be reached; reference the GetCircularDependencies method name and ensure the comment clarifies the distinction between "no cycles" (empty payload) and "endpoint unavailable" (nil response).
🤖 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/template/render.go`:
- Around line 17-19: The circular dependency warning currently only appears in
the full template branch and is lost when output is shortened by
truncateToTokenBudget; update the truncation path so the cycle signal is
preserved by either (A) having truncateToTokenBudget return/preserve a boolean
or string like hasCircularDependency (or the original
.Graph.Stats.CircularDependencyCycles count) alongside the truncated content, or
(B) inject a precomputed field (e.g. .HasCircularDependency or
.CircularDependencyCount) into the template data before calling
truncateToTokenBudget; then update the template check (currently using
.Graph.Stats.CircularDependencyCycles) to also consider the preserved field so
the warning is rendered even when truncation occurs. Ensure to adjust callers of
truncateToTokenBudget and the template rendering code that references
Graph.Stats.CircularDependencyCycles to read the new preserved field.
---
Nitpick comments:
In `@internal/api/client.go`:
- Around line 361-363: The default error branch in the response handling
currently returns a generic error ("API error %d"); update this to preserve the
backend error message similar to GetGraph by reading the response body (e.g.,
via resp.Body) and including its contents in the returned error (combine a short
context like "API error <status>" with the backend message). Ensure you close or
drain resp.Body and handle read errors gracefully so the function (the
switch/default in internal/api/client.go) returns an error containing the
server-provided text rather than only the status code.
- Around line 293-296: Update the docstring for GetCircularDependencies to match
its runtime behavior: state that the function returns a non-nil (empty)
CircularDependencyResponse when the endpoint successfully reports no cycles, and
only returns nil, nil when the endpoint is unavailable or cannot be reached;
reference the GetCircularDependencies method name and ensure the comment
clarifies the distinction between "no cycles" (empty payload) and "endpoint
unavailable" (nil response).
Contributor
Author
|
@claude fix all comments and merge conflicts and get this merged |
Contributor
|
Claude finished @greynewell's task in 6m 49s —— View job Fixing PR Review Comments + Merge Conflicts ✓What was done:
To merge: The PR is ready. You'll need to approve and merge it via the GitHub UI (I cannot merge PRs directly due to GitHub App permissions). |
claude
bot
force-pushed
the
claude/issue-12-20260227-0013
branch
from
78be48f to
2de0fb1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/api/client.go (1)
325-442: Consider extracting shared async polling into one helper.
GetCircularDependenciesandGetGraphare almost the same control flow. A shared submit+poll helper would reduce drift (timeouts, status handling, retries, headers) and make fixes apply once.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/client.go` around lines 325 - 442, GetCircularDependencies and GetGraph share nearly identical submit-and-poll logic; extract that flow into a new helper (e.g., submitAndPollJob or postAndPollJob) that handles request creation, idempotency key, multipart/body submission, response reading, status-code handling (401, 402, 429, 404/405 defaulting to nil), JSON unmarshalling into a JobStatus, polling retry logic (RetryAfter fallback, deadline/maxPollAttempts), and error mapping so both GetCircularDependencies and GetGraph call the helper with endpoint, payload, and a result-unmarshal callback; update GetCircularDependencies to use the helper and keep specific result unmarshalling into CircularDependencyResponse and status-to-error handling for jobResp.Error, preserving existing logging via c.logFn and headers (Content-Type, X-Api-Key, Accept, User-Agent, Idempotency-Key).internal/template/render.go (1)
17-19: Consider centralizing cycle-warning text/pluralization.The warning sentence logic now exists in two places (template + truncation builder). Small helper/shared formatter would keep wording and pluralization from drifting over time.
Also applies to: 119-125
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/template/render.go` around lines 17 - 19, Extract the warning text + pluralization into a single shared helper (e.g., a function FormatCircularDependencyWarning(count int) string or a template func "circularDependencyWarning") and use that helper from the template (replace the inline {{if ...}} block that references .Graph.Stats.CircularDependencyCycles) and from the truncation builder code that currently repeats the same wording (the logic around lines ~119-125). Ensure the helper accepts the cycle count, returns the fully formatted sentence with correct singular/plural form, and is exposed to templates via the template.FuncMap so both places call the same formatter.
🤖 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`:
- Around line 436-438: The default branch of the job-status switch (where
c.logFn("[debug] unknown circular dep job status: %s", jobResp.Status) is
called) must treat unknown statuses as terminal or trigger a backoff instead of
immediately retrying; modify that default case in the job response handling
logic to log the unexpected status along with context, mark the job as
terminal/failed (or set a backoff flag/delay) so the caller will not immediately
POST again, and surface an error/return so retry logic can apply exponential
backoff rather than firing rapid requests.
---
Nitpick comments:
In `@internal/api/client.go`:
- Around line 325-442: GetCircularDependencies and GetGraph share nearly
identical submit-and-poll logic; extract that flow into a new helper (e.g.,
submitAndPollJob or postAndPollJob) that handles request creation, idempotency
key, multipart/body submission, response reading, status-code handling (401,
402, 429, 404/405 defaulting to nil), JSON unmarshalling into a JobStatus,
polling retry logic (RetryAfter fallback, deadline/maxPollAttempts), and error
mapping so both GetCircularDependencies and GetGraph call the helper with
endpoint, payload, and a result-unmarshal callback; update
GetCircularDependencies to use the helper and keep specific result unmarshalling
into CircularDependencyResponse and status-to-error handling for jobResp.Error,
preserving existing logging via c.logFn and headers (Content-Type, X-Api-Key,
Accept, User-Agent, Idempotency-Key).
In `@internal/template/render.go`:
- Around line 17-19: Extract the warning text + pluralization into a single
shared helper (e.g., a function FormatCircularDependencyWarning(count int)
string or a template func "circularDependencyWarning") and use that helper from
the template (replace the inline {{if ...}} block that references
.Graph.Stats.CircularDependencyCycles) and from the truncation builder code that
currently repeats the same wording (the logic around lines ~119-125). Ensure the
helper accepts the cycle count, returns the fully formatted sentence with
correct singular/plural form, and is exposed to templates via the
template.FuncMap so both places call the same formatter.
- Add CircularDependencyCycles field to Stats struct - Add CircularDependencyCycle and CircularDependencyResponse types - Add GetCircularDependencies API client method with proper error handling - Add fetchGraphWithCircularDeps helper in cmd/ that calls both endpoints - Render circular dependency warning in context bomb template - Preserve warning in truncateToTokenBudget fallback path Closes #12 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
greynewell
force-pushed
the
claude/issue-12-20260227-0013
branch
from
2de0fb1 to
e64d4c3
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/template/render.go (1)
190-197:buildCriticalFilesSectionis unused and can be removed.This function is never called anywhere in the codebase. The
truncateToTokenBudgetfunction handles critical files inline with smart logic that respects the token budget — only including files that fit within the remaining tokens. SincebuildCriticalFilesSectiondoesn't have that budget-aware logic, it looks like this helper was bypassed in favor of the inline approach.Safe to delete to keep things clean.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/template/render.go` around lines 190 - 197, Remove the unused helper function buildCriticalFilesSection from internal/template/render.go: locate the function declaration for buildCriticalFilesSection and delete it (the truncateToTokenBudget path already implements budget-aware critical file handling), and run tests/grep to ensure no references remain so the codebase is clean of dead code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/template/render.go`:
- Around line 190-197: Remove the unused helper function
buildCriticalFilesSection from internal/template/render.go: locate the function
declaration for buildCriticalFilesSection and delete it (the
truncateToTokenBudget path already implements budget-aware critical file
handling), and run tests/grep to ensure no references remain so the codebase is
clean of dead code.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cmd/pregen.gocmd/run.gointernal/api/client.gointernal/template/render.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/pregen.go
This was referenced Feb 28, 2026
Closed
Closed
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.
Adds circular dependency detection via
/v1/graphs/circular-dependencies. Renders a blockquote warning when cycles exist. Cycle count is stored inStats.CircularDependencyCyclesso cache hits serve it with zero extra API calls.Closes #12
Generated with Claude Code
Summary by CodeRabbit