Remove context.TODO #1866

lionello · 2026-01-25T01:59:22Z

Description

Linked Issues

Checklist

I have performed a self-review of my code
I have added appropriate tests
I have updated the Defang CLI docs and/or README to reflect my changes, if necessary

Summary by CodeRabbit

Bug Fixes
- Improved error message for invalid color input — now shows the actual invalid value plus valid options.
Refactor
- Made directory resolution context-aware across the CLI, agent tools, and session/stack components to align behavior and cancellation handling.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

coderabbitai · 2026-01-25T01:59:42Z

📝 Walkthrough

Walkthrough

Updated the TargetDirectory API to accept a context.Context across interfaces, implementations, call sites, tests, and mocks; added the actual invalid color value to an error message; expanded imports in one debug command file.

Changes

Cohort / File(s)	Summary
Interface Definitions `src/pkg/cli/client/provider.go`, `src/pkg/session/session.go`	Changed `TargetDirectory()` to `TargetDirectory(context.Context)` on `Loader` and `StacksManager` interfaces.
Core Implementations `src/pkg/cli/compose/loader.go`, `src/pkg/stacks/manager.go`	Implementations updated to accept `ctx` parameter; compose loader now passes the provided `ctx` into project loading instead of `context.TODO()`.
Command Layer Call Sites `src/cmd/cli/command/compose.go`, `src/cmd/cli/command/stack.go`, `src/cmd/cli/command/session.go`	Updated calls to `TargetDirectory` to pass `ctx`; minor local variable renames in `session.go` (`stack`→`stackName`, `provider`→`providerId`).
Agent Tools Call Sites `src/pkg/agent/tools/deploy.go`, `src/pkg/agent/tools/destroy.go`, `src/pkg/agent/tools/listConfig.go`, `src/pkg/agent/tools/logs.go`, `src/pkg/agent/tools/removeConfig.go`, `src/pkg/agent/tools/services.go`, `src/pkg/agent/tools/setConfig.go`	All stack manager constructions updated to call `loader.TargetDirectory(ctx)` instead of `loader.TargetDirectory()`.
Tests & Mocks `src/cmd/cli/command/commands_test.go`, `src/cmd/cli/command/session_test.go`, `src/pkg/cli/client/mock.go`, `src/pkg/session/session_test.go`	Mocks and tests updated to the new `TargetDirectory(context.Context)` signature and to pass a `context` (e.g., `t.Context()`).
CLI Misc Changes `src/cmd/cli/command/color.go`, `src/cmd/cli/command/debug.go`	Error message for invalid color now includes the provided value; `debug.go` imports expanded (added `os`, `cli`, protobuf alias).

Sequence Diagram(s)

mermaid
sequenceDiagram
autonumber
participant CLI as Client/CLI
participant Loader as Loader (compose/cli)
participant Manager as StacksManager
participant Agent as Agent Tools
CLI->>Loader: call TargetDirectory(ctx)
note right of Loader: uses provided ctx to load project
Loader-->>CLI: return target directory
CLI->>Manager: New Manager(targetDir)
Manager-->>CLI: manager instance
CLI->>Agent: invoke tool (deploy/destroy/etc.) using manager
Agent->>Manager: interact with stacks (list/create/remove)
Manager-->>Agent: results/status

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Stacks onboarding with "remote" stacks #1721: Previously introduced/used the original TargetDirectory() API that this PR converts to accept a context.Context.

Suggested reviewers

lionello

Poem

🐰 I hop with context in tow today,
Passing ctx along the stack’s long way.
Methods adjusted, tests renewed,
Errors clearer — the code feels shrewd.
Hooray for tiny, tidy change — hooray! 🎉

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 13.33% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Remove context.TODO' directly aligns with the primary objective of the changeset, which involves updating the TargetDirectory method signature across multiple files to accept a context parameter instead of using context.TODO() internally.

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

✨ Finishing touches

📝 Generate docstrings

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.5.0)

level=warning msg="[linters_context] running gomodguard failed: unable to read module file go.mod: current working directory must have a go.mod file: if you are not using go modules it is suggested to disable this linter"
level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies"

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

coderabbitai

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@src/cmd/cli/command/debug.go`:
- Around line 28-33: Remove the hardcoded local file read and debug hook: delete
the block that reads "/Users/llunesu/Downloads/zdqxyk3xati8", unmarshals into
projupdate using proto.Unmarshal, and calls cli.PrintObject; if this behavior is
required for debugging instead, gate it behind an explicit CLI flag or
environment variable (e.g., --debug-proto or DEBUG_PROTO_PATH) and validate the
path before reading, propagate and log errors from os.ReadFile, proto.Unmarshal,
and cli.PrintObject rather than ignoring them, referencing the projupdate
variable, proto.Unmarshal call, and cli.PrintObject invocation so the change is
applied to the correct code.

src/cmd/cli/command/debug.go

coderabbitai

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@src/cmd/cli/command/debug.go`:
- Around line 3-6: Remove the unused os import from the import block in debug.go
to fix the build error: update the import list in the file (the import statement
that currently includes "fmt", "os", "time") by deleting "os" (or use it if you
intend to add related logic), ensuring only used packages remain to eliminate
the unused-import compile failure.

coderabbitai · 2026-01-27T17:38:48Z

src/cmd/cli/command/debug.go

 import (
 	"fmt"
+	"os"
 	"time"


⚠️ Potential issue | 🔴 Critical

Remove unused os import to fix build failure.

The pipeline is failing because os is imported but not used. Remove it (or use it if you intended to add file/env logic in this change).

🛠️ Proposed fix

import ( "fmt" - "os" "time"

📝 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

import (

"fmt"

"os"

"time"

import (

"fmt"

"time"

🧰 Tools

🪛 GitHub Actions: Go package

[error] 5-5: "os" imported and not used

🤖 Prompt for AI Agents

In `@src/cmd/cli/command/debug.go` around lines 3 - 6, Remove the unused os import from the import block in debug.go to fix the build error: update the import list in the file (the import statement that currently includes "fmt", "os", "time") by deleting "os" (or use it if you intend to add related logic), ensuring only used packages remain to eliminate the unused-import compile failure.

lionello added 2 commits January 24, 2026 17:10

chore: same error for color as others

1a6393f

chore: remove context.TODO from TargetDirectory

461b8ae

lionello requested a review from jordanstephens as a code owner January 25, 2026 01:59

coderabbitai bot reviewed Jan 25, 2026

View reviewed changes

src/cmd/cli/command/debug.go Outdated Show resolved Hide resolved

jordanstephens approved these changes Jan 27, 2026

View reviewed changes

src/cmd/cli/command/debug.go Outdated Show resolved Hide resolved

Update src/cmd/cli/command/debug.go

709aa76

coderabbitai bot reviewed Jan 27, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove context.TODO #1866

Remove context.TODO #1866

Uh oh!

lionello commented Jan 25, 2026 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Jan 25, 2026 •

edited

Loading

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related PRs

Suggested reviewers

Poem

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

Uh oh!

coderabbitai bot left a comment

Uh oh!

coderabbitai bot Jan 27, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Remove context.TODO #1866

Are you sure you want to change the base?

Remove context.TODO #1866

Uh oh!

Conversation

lionello commented Jan 25, 2026 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Linked Issues

Checklist

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Jan 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related PRs

Suggested reviewers

Poem

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

lionello commented Jan 25, 2026 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Jan 25, 2026 •

edited

Loading