Merged
Conversation
Signed-off-by: Philippe Martin <phmartin@redhat.com> Co-Authored-By: Claude Code (Claude Sonnet 4.5) <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds an Changes
Sequence DiagramsequenceDiagram
actor User
participant CLI as Init Command
participant Manager as Instances Manager
participant Storage as Storage
User->>CLI: run init [sources-dir] [--workspace-configuration=<dir>] [--verbose]
CLI->>CLI: preRun: parse args & flags, resolve paths to absolute
CLI->>Storage: open storage (default or --storage)
Storage-->>Manager: provide manager instance
CLI->>Manager: Create NewInstance(absSource, absConfig)
Manager->>Storage: persist instance metadata (ID, paths)
Manager-->>CLI: return created instance (ID, paths)
CLI->>User: print "Registered workspace:" (ID, sources, config) or verbose metadata
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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 (3)
AGENTS.md (2)
239-255: Fix variable shadowing and redeclaration in code examples.The code examples have minor issues that could confuse readers:
- Line 246:
instanceshadows the variable declared on line 229- Line 252:
err :=redeclareserrwhich was already declared on line 241Suggested fixes
// Get specific instance -instance, err := manager.Get(id) +inst, err := manager.Get(id) if err != nil { return fmt.Errorf("instance not found: %w", err) } // Delete instance -err := manager.Delete(id) +err = manager.Delete(id) if err != nil { return fmt.Errorf("failed to delete instance: %w", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AGENTS.md` around lines 239 - 255, The example has variable shadowing/redeclaration: reuse the previously declared err and avoid shadowing instance—change the `instance` declaration from a new declaration to an assignment (or rename the earlier variable) when calling `manager.Get(id)`, and change the `err :=` before `manager.Delete(id)` to `err =` so you don't redeclare `err` (references: instancesList, manager.List, instance, manager.Get, err, manager.Delete).
196-198: Reference could be more accurate.Line 198 references
pkg/instances/instance_test.gofor command testing patterns, butpkg/cmd/init_test.go(mentioned on line 214) is a better reference since it demonstrates the actual command testing pattern withpreRununit tests and E2E tests.Suggested change
-Commands should have two types of tests following the pattern in `pkg/instances/instance_test.go`: +Commands should have two types of tests:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AGENTS.md` around lines 196 - 198, Update the reference in AGENTS.md: replace the mention of pkg/instances/instance_test.go with pkg/cmd/init_test.go as the canonical example for command testing patterns; specifically call out the use of preRun unit tests and E2E tests shown in init_test.go so readers are directed to the correct test examples.pkg/cmd/init.go (1)
40-42: Consider handling the error fromGetString.Silently discarding the error with
_could mask issues if the flag is misconfigured. While Cobra typically ensures the flag exists when using persistent flags, explicitly handling the error provides better diagnostics.Proposed fix
- storageDir, _ := cmd.Flags().GetString("storage") + storageDir, err := cmd.Flags().GetString("storage") + if err != nil { + return fmt.Errorf("failed to get storage flag: %w", err) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cmd/init.go` around lines 40 - 42, The call to cmd.Flags().GetString("storage") in init.go currently ignores the error return (storageDir, _), which can hide flag retrieval problems; change it to capture the error (e.g., storageDir, err := cmd.Flags().GetString("storage")) and handle it by returning or logging a descriptive error (for example return fmt.Errorf("failed to read --storage flag: %w", err) or use the command's Print/Logger) before proceeding, so any misconfigured or missing flag is surfaced; keep the variable name storageDir and the GetString call intact when implementing the check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/cmd/init.go`:
- Around line 1-119: The file fails gofmt style checks; run the formatter (e.g.
make fmt or go fmt ./...) to fix formatting issues in pkg/cmd/init.go (affects
symbols InitCmd, preRun, run, NewInitCmd); commit the reformatted file so the CI
formatting check passes.
---
Nitpick comments:
In `@AGENTS.md`:
- Around line 239-255: The example has variable shadowing/redeclaration: reuse
the previously declared err and avoid shadowing instance—change the `instance`
declaration from a new declaration to an assignment (or rename the earlier
variable) when calling `manager.Get(id)`, and change the `err :=` before
`manager.Delete(id)` to `err =` so you don't redeclare `err` (references:
instancesList, manager.List, instance, manager.Get, err, manager.Delete).
- Around line 196-198: Update the reference in AGENTS.md: replace the mention of
pkg/instances/instance_test.go with pkg/cmd/init_test.go as the canonical
example for command testing patterns; specifically call out the use of preRun
unit tests and E2E tests shown in init_test.go so readers are directed to the
correct test examples.
In `@pkg/cmd/init.go`:
- Around line 40-42: The call to cmd.Flags().GetString("storage") in init.go
currently ignores the error return (storageDir, _), which can hide flag
retrieval problems; change it to capture the error (e.g., storageDir, err :=
cmd.Flags().GetString("storage")) and handle it by returning or logging a
descriptive error (for example return fmt.Errorf("failed to read --storage flag:
%w", err) or use the command's Print/Logger) before proceeding, so any
misconfigured or missing flag is surfaced; keep the variable name storageDir and
the GetString call intact when implementing the check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 91dfada7-f98b-469c-be8e-3aa9e15ac841
📒 Files selected for processing (4)
AGENTS.mdpkg/cmd/init.gopkg/cmd/init_test.gopkg/cmd/root.go
Signed-off-by: Philippe Martin <phmartin@redhat.com> Co-Authored-By: Claude Code (Claude Sonnet 4.5) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
pkg/cmd/init.go (1)
30-35: Inject factories so the command can be unit-tested with fakes.
preRunandrunare hard-wired toinstances.NewManagerandinstances.NewInstance, which is why the "unit" tests still hit the real filesystem. Adding unexported factory fields on the command state would let the tests use fake implementations and keep storage side effects out of the fast path.💡 Refactor sketch
type InitCmd struct { sourcesDir string workspaceConfigDir string absSourcesDir string absConfigDir string manager instances.Manager + newManager func(string) (instances.Manager, error) + newInstance func(string, string) (instances.Instance, error) } @@ - manager, err := instances.NewManager(storageDir) + manager, err := i.newManager(storageDir) @@ - instance, err := instances.NewInstance(i.absSourcesDir, i.absConfigDir) + instance, err := i.newInstance(i.absSourcesDir, i.absConfigDir) @@ func NewInitCmd() *cobra.Command { - initCmd := &InitCmd{} + initCmd := &InitCmd{ + newManager: instances.NewManager, + newInstance: instances.NewInstance, + }As per coding guidelines, "When testing code that uses interfaces, use fake implementations instead of real implementations or mocks" and "Create unexported fake structs that implement interfaces for testing purposes."
Also applies to: 43-45, 81-83
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cmd/init.go` around lines 30 - 35, The InitCmd struct hard-wires instances.NewManager and instances.NewInstance in preRun/run which forces tests to touch the real filesystem; add unexported factory fields on InitCmd (e.g., newManagerFn func(...) instances.Manager and newInstanceFn func(...) instances.Instance or similarly named constructors) and use those factories inside preRun and run instead of directly calling instances.NewManager and instances.NewInstance, defaulting the factories to the real constructors in the command setup so production behavior is unchanged but unit tests can inject fake implementations via those factory fields to avoid storage side effects; update usages of the manager field and any call sites in preRun/run to use the injected factories.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/cmd/init_test.go`:
- Around line 276-280: The test currently logs a wrong slice length with
t.Errorf and then immediately indexes instancesList (e.g., inst :=
instancesList[0]), which can panic and mask the real failure; change the
assertion to fail fast by replacing the t.Errorf length checks with t.Fatalf (or
call t.FailNow()/return immediately) before any indexing so the test stops and
reports the assertion instead of panicking—apply this change for the
instancesList checks shown and the similar block around lines 554-564.
In `@pkg/cmd/init.go`:
- Around line 29-36: Rename the exported struct InitCmd to an unexported initCmd
and update all references (including the constructor NewInitCmd, any tests, and
usages) to use the new unexported type; ensure NewInitCmd continues to be
exported and returns the same concrete pointer type (now *initCmd) or an
appropriate exported interface if already used elsewhere, and update field names
and method receivers from InitCmd to initCmd so the command internals remain
unexported while the factory function stays public.
- Around line 39-46: In preRun, handle the error returned by
cmd.Flags().GetString("storage") instead of ignoring it: check the err from
GetString and return a wrapped fmt.Errorf if non-nil before calling
instances.NewManager; reference the preRun function and the GetString call so
the flag error is validated prior to invoking instances.NewManager.
---
Nitpick comments:
In `@pkg/cmd/init.go`:
- Around line 30-35: The InitCmd struct hard-wires instances.NewManager and
instances.NewInstance in preRun/run which forces tests to touch the real
filesystem; add unexported factory fields on InitCmd (e.g., newManagerFn
func(...) instances.Manager and newInstanceFn func(...) instances.Instance or
similarly named constructors) and use those factories inside preRun and run
instead of directly calling instances.NewManager and instances.NewInstance,
defaulting the factories to the real constructors in the command setup so
production behavior is unchanged but unit tests can inject fake implementations
via those factory fields to avoid storage side effects; update usages of the
manager field and any call sites in preRun/run to use the injected factories.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ea94c810-650a-47ed-93b6-b0cab48bf3c4
📒 Files selected for processing (4)
AGENTS.mdpkg/cmd/init.gopkg/cmd/init_test.gopkg/cmd/root.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/cmd/root.go
Contributor
|
question: should the output be by default only the id in case you want to automate/reuse cli for now I see I would expect verbose mode or JSON output would display your content ? and question around the internal file, why using _ ? to have best of world or JSON files I see you're using snake_case but it's not straightforward in JS/TS world so could we have a sub-entry [
{
"id": "dc610bffa75f21b5b043f98aff12b157fb16fae6c0ac3139c28f85d6defbe017",
"source_dir": "/Users/benoitf/git/kortex/kortex-cli",
"config_dir": "/Users/benoitf/git/kortex/kortex-cli/.kortex"
}
]--> [
{
"id": "dc610bffa75f21b5b043f98aff12b157fb16fae6c0ac3139c28f85d6defbe017",
"paths": {
"source": "/Users/benoitf/git/kortex/kortex-cli",
"configuration": "/Users/benoitf/git/kortex/kortex-cli/.kortex"
}
}
]so it's neither camelCase or snake_case only lowercase ? |
Signed-off-by: Philippe Martin <phmartin@redhat.com> Co-Authored-By: Claude Code (Claude Sonnet 4.5) <noreply@anthropic.com>
Signed-off-by: Philippe Martin <phmartin@redhat.com> Co-Authored-By: Claude Code (Claude Sonnet 4.5) <noreply@anthropic.com>
Signed-off-by: Philippe Martin <phmartin@redhat.com> Co-Authored-By: Claude Code (Claude Sonnet 4.5) <noreply@anthropic.com>
benoitf
approved these changes
Mar 6, 2026
Contributor
benoitf
left a comment
benoitf
left a comment
There was a problem hiding this comment.
tried different cli options and it was working as expected
(default storage, custom storage) etc
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/instances/instance.go (1)
149-163:⚠️ Potential issue | 🟠 MajorAdd migration path for legacy storage schema or risk breaking existing installations.
Recent changes nested path fields under
Paths(paths.source,paths.configuration), butmanager.goline 247 uses standardjson.Unmarshalwith no fallback. Any existinginstances.jsonwith flat-levelsource_dir/config_dirfields will fail to unmarshal, crashing on startup. Add either:
- A custom
UnmarshalJSONonInstanceDatato handle both old and new formats, or- A migration step before loading, or
- A regression test that loads the old schema to catch this in future
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/instances/instance.go` around lines 149 - 163, The code now expects nested path fields under Paths which will break older JSON with flat fields like source_dir/config_dir; implement a custom UnmarshalJSON on the InstanceData type to accept both schemas: try decoding into the new structure with Paths, and if missing, fall back to reading flat fields (e.g., source_dir, config_dir) and populate Paths.Source and Paths.Configuration accordingly; ensure InstanceData.UnmarshalJSON sets all related fields used by NewInstanceFromData and add a regression test that unmarshals an old-format JSON to prevent future breaks.
🧹 Nitpick comments (1)
pkg/instances/manager_test.go (1)
813-833: Assert the wire format, not just the typed values.This test unmarshals into
InstanceData, so it would still pass if the file accidentally included legacysource_dir/config_dirfields in addition topaths. Since this PR changes the on-disk schema, add a raw JSON assertion thatpaths.sourceandpaths.configurationexist and the flat keys do not.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/instances/manager_test.go` around lines 813 - 833, The test currently only unmarshals into InstanceData and checks typed fields (instances, InstanceData, generatedID, expectedSource, expectedConfig); change the test to also unmarshal the raw JSON (e.g. into []map[string]interface{} or []json.RawMessage) and assert that each item contains a "paths" object with "source" and "configuration" keys present and that legacy flat keys "source_dir" and "config_dir" are absent; keep the existing typed assertions but add these raw JSON checks to verify the wire format rather than only the typed values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@AGENTS.md`:
- Around line 176-186: The example exposes concrete DTO structs InstancePaths
and InstanceData in pkg/, which conflicts with the module rule that public types
must be interfaces; either document a DTO exception or change the example to
comply. Update AGENTS.md to explicitly state that simple data-transfer structs
(DTOs) such as InstancePaths and InstanceData are exempt from the interface-only
rule (and describe the exemption criteria), or replace the example with an
interface-based pattern and mention factory functions and unexported
implementations to align with the "pkg/ MUST follow interface-based design"
guidance.
In `@pkg/cmd/init.go`:
- Around line 54-79: The code currently accepts and stores a normalized sources
path (i.sourcesDir / absSourcesDir) without verifying it exists or is a
directory; update the preRun/init logic where i.sourcesDir and absSourcesDir are
set to stat the resolved path (use os.Stat) and fail fast with a clear error if
the path does not exist or is not a directory before assigning i.absSourcesDir
(and before defaulting workspaceConfigDir); reference i.sourcesDir,
absSourcesDir, i.absSourcesDir and instances.NewInstance() so the guard prevents
registering broken workspace entries.
---
Outside diff comments:
In `@pkg/instances/instance.go`:
- Around line 149-163: The code now expects nested path fields under Paths which
will break older JSON with flat fields like source_dir/config_dir; implement a
custom UnmarshalJSON on the InstanceData type to accept both schemas: try
decoding into the new structure with Paths, and if missing, fall back to reading
flat fields (e.g., source_dir, config_dir) and populate Paths.Source and
Paths.Configuration accordingly; ensure InstanceData.UnmarshalJSON sets all
related fields used by NewInstanceFromData and add a regression test that
unmarshals an old-format JSON to prevent future breaks.
---
Nitpick comments:
In `@pkg/instances/manager_test.go`:
- Around line 813-833: The test currently only unmarshals into InstanceData and
checks typed fields (instances, InstanceData, generatedID, expectedSource,
expectedConfig); change the test to also unmarshal the raw JSON (e.g. into
[]map[string]interface{} or []json.RawMessage) and assert that each item
contains a "paths" object with "source" and "configuration" keys present and
that legacy flat keys "source_dir" and "config_dir" are absent; keep the
existing typed assertions but add these raw JSON checks to verify the wire
format rather than only the typed values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a03a6223-aea8-4582-b94e-9f7ae9a24f10
📒 Files selected for processing (5)
AGENTS.mdpkg/cmd/init.gopkg/cmd/init_test.gopkg/instances/instance.gopkg/instances/manager_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/cmd/init_test.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
related to #18
Does not cover the
--nameflag yet, and only covers the registration and storage of the workspace (no agent is started).When new workspaces are registered, they are stored in the storage file (~/.kortex-cli/instances.json by default).
Example: