Initial implementation: ABSmartly CLI

[joalves]

Summary

Complete implementation of the ABSmartly CLI (abs) - a command-line interface for managing experiments, feature flags, and A/B tests on the ABSmartly platform.

Features

Core Commands

• Experiments: List, search, view, and manage experiments with comprehensive filtering
• Activity: Track experiment iterations and timeline events
• Authentication: Secure login and API key management
• Configuration: Environment and project setup
• Code Generation: TypeScript type generation and shell completions

Output Formats

• Multiple output formats: JSON, YAML, Markdown, Table
• --terse flag for compact output
• --full flag for untruncated data
• Smart defaults (markdown uses --full by default)

Advanced Features

• Date filtering with ISO 8601 timestamps and milliseconds
• Field dereferencing for user objects
• Comprehensive metadata display
• Timeline and activity tracking
• Alert notifications
• Docker support

Additional Commands

• API keys, apps, environments management
• Goals, metrics, and tags
• Permissions, roles, teams, users
• Segments and unit types
• Webhooks configuration

Technical Details

• Built with Go and Cobra CLI framework
• Comprehensive test coverage (100%)
• GoReleaser for multi-platform distribution
• npm package support
• Docker container support
• Shell completion (bash, zsh, fish, PowerShell)

Documentation

• Complete README with installation and usage
• Inline command help
• Examples and use cases

Test Coverage

• Unit tests for all commands
• Integration tests for flag combinations
• Output format validation tests
• 100% code coverage

Summary by CodeRabbit

• New Features

  • Full-featured CLI for experiments, flags, goals, segments, teams, users, metrics, webhooks, roles, API keys, search, notes/activity, auth, completions, version, setup, doctor, open/browser shortcuts, generate.

• Output & UX

  • Multiple output formats (json, yaml, markdown, table, plain), shell completions, interactive prompts, templating, activity timeline and notes views, version display.

• Installation & Deployment

  • Installer script, Docker image entrypoint, Makefile, multi‑platform release configuration.

• Documentation

  • Comprehensive README and audit reports.

• Tests

  • Extensive unit and integration test suites.

• Chores

  • Added .gitignore and module metadata.

[coderabbitai]

Actionable comments posted: 2

🤖 Fix all issues with AI agents

In `@cmd/experiments/experiments_extra_test.go`:
- Around line 586-593: The test contains trivial assertions of standard library
behavior (strings.Contains and strings.ToLower) that should be removed; edit
experiments_extra_test.go to delete the assertions referencing
strings.Contains/strings.ToLower (the four assert.True/assert.False/assert.Equal
lines) and keep only the project-related assertion(s) such as require.NoError(t,
runSearch(searchCmd, []string{"missing"})); ensure no other references to those
removed checks remain in the test.
- Around line 464-484: The test TestTasksAlertsAnalyses uses brittle
Commands()[0] / Commands()[1] index access on newTasksCmd, newAlertsCmd and
newAnalysesCmd; change those to look up subcommands by name using the existing
findCommand helper (e.g. call findCommand(tasksCmd, "delete"),
findCommand(alertsCmd, "list") and findCommand(alertsCmd, "delete"),
findCommand(analysesCmd, "delete")) and then call RunE on the returned
*cobra.Command—this ensures the test targets the correct subcommands regardless
of registration order.

🧹 Nitpick comments (5)

cmd/webhooks/webhooks_test.go (1)

27-83: Consider using subtests and asserting request payloads / response output.

This monolithic test verifies that each operation doesn't error, but it doesn't assert:

1. That the correct HTTP request body is sent (e.g., the create payload includes "name":"hook2").
2. That the printed output matches the server's response data.
3. Individual failures — if runList fails, the remaining operations are never exercised.

Using t.Run(...) subtests and inspecting r.Body in handlers (or capturing stdout) would strengthen coverage and make failures easier to diagnose.

cmd/experiments/experiments_extra_test.go (4)

486-511: Inconsistent stdout capture approach.

Lines 496–503 manually set up an os.Pipe to capture stdout, but testutil.CaptureStdout is used elsewhere in this same file (e.g., Lines 315, 347, 374, 411). Use the shared helper here too for consistency and to avoid the risk of leaking the replaced os.Stdout if the test panics before the restore.

♻️ Suggested fix

 	cmd := newGenerateTemplateCmd()
 	cmd.Flags().Set("name", "My Name")
 	cmd.Flags().Set("type", "test")
 
-	var stdout bytes.Buffer
-	orig := os.Stdout
-	r, w, _ := os.Pipe()
-	os.Stdout = w
-	require.NoError(t, runGenerateTemplate(cmd, nil))
-	_ = w.Close()
-	_, _ = stdout.ReadFrom(r)
-	_ = r.Close()
-	os.Stdout = orig
-	assert.Contains(t, stdout.String(), "My Name")
+	output := testutil.CaptureStdout(t, func() {
+		require.NoError(t, runGenerateTemplate(cmd, nil))
+	})
+	assert.Contains(t, output, "My Name")

---

698-707: Setting both terse and full simultaneously is under-asserted.

terse and full are mutually exclusive flags according to the PR description. The test sets both to true and only checks that no error is returned. Consider asserting the actual output or behaviour to verify which flag takes precedence (or that an error is raised for conflicting flags).

---

744-749: TestSearchHelpers only tests stdlib string operations on a struct literal.

This test constructs an api.Experiment and then calls strings.Contains/strings.ToLower on its fields — it does not invoke any helper function from the project. If there is a search-matching helper, test that; otherwise this can be removed alongside the similar assertions at Lines 588–592.

---

204-215: Clarify the intent of viper.Reset() after config setup.

viper.Reset() on Line 214 clears all in-memory Viper state immediately after SetupConfig writes it. This works only because the commands presumably re-read from the config file on disk. A brief comment would prevent future maintainers from "fixing" this by removing the reset.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Subcommand access by index is fragile.

Commands()[0] and Commands()[1] assume a specific registration order. If subcommands are reordered or new ones are inserted, these tests will silently test the wrong command or panic. Use findCommand (already defined in this file) instead.

♻️ Suggested fix

 	tasksCmd := newTasksCmd()
-	deleteTasks := tasksCmd.Commands()[0]
+	deleteTasks := findCommand(tasksCmd, "delete")
+	require.NotNil(t, deleteTasks)
 	require.NoError(t, deleteTasks.RunE(deleteTasks, []string{"123"}))

 	alertsCmd := newAlertsCmd()
-	listCmd := alertsCmd.Commands()[0]
+	listCmd := findCommand(alertsCmd, "list")
+	require.NotNil(t, listCmd)
 	require.NoError(t, listCmd.RunE(listCmd, []string{"123"}))
-	deleteAlerts := alertsCmd.Commands()[1]
+	deleteAlerts := findCommand(alertsCmd, "delete")
+	require.NotNil(t, deleteAlerts)
 	require.NoError(t, deleteAlerts.RunE(deleteAlerts, []string{"123"}))

 	analysesCmd := newAnalysesCmd()
-	deleteAnalyses := analysesCmd.Commands()[0]
+	deleteAnalyses := findCommand(analysesCmd, "delete")
+	require.NotNil(t, deleteAnalyses)
 	require.NoError(t, deleteAnalyses.RunE(deleteAnalyses, []string{"123"}))

📝 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 TestTasksAlertsAnalyses(t *testing.T) {
	apiServer := newAPIServer(t, apiServerOptions{})
	expctldServer := newExpctldServer(t)
	t.Cleanup(apiServer.Close)
	t.Cleanup(expctldServer.Close)
	setupExperimentConfig(t, apiServer.URL, expctldServer.URL)
	tasksCmd := newTasksCmd()
	deleteTasks := tasksCmd.Commands()[0]
	require.NoError(t, deleteTasks.RunE(deleteTasks, []string{"123"}))
	alertsCmd := newAlertsCmd()
	listCmd := alertsCmd.Commands()[0]
	require.NoError(t, listCmd.RunE(listCmd, []string{"123"}))
	deleteAlerts := alertsCmd.Commands()[1]
	require.NoError(t, deleteAlerts.RunE(deleteAlerts, []string{"123"}))
	analysesCmd := newAnalysesCmd()
	deleteAnalyses := analysesCmd.Commands()[0]
	require.NoError(t, deleteAnalyses.RunE(deleteAnalyses, []string{"123"}))
	}
	func TestTasksAlertsAnalyses(t *testing.T) {
	apiServer := newAPIServer(t, apiServerOptions{})
	expctldServer := newExpctldServer(t)
	t.Cleanup(apiServer.Close)
	t.Cleanup(expctldServer.Close)
	setupExperimentConfig(t, apiServer.URL, expctldServer.URL)
	tasksCmd := newTasksCmd()
	deleteTasks := findCommand(tasksCmd, "delete")
	require.NotNil(t, deleteTasks)
	require.NoError(t, deleteTasks.RunE(deleteTasks, []string{"123"}))
	alertsCmd := newAlertsCmd()
	listCmd := findCommand(alertsCmd, "list")
	require.NotNil(t, listCmd)
	require.NoError(t, listCmd.RunE(listCmd, []string{"123"}))
	deleteAlerts := findCommand(alertsCmd, "delete")
	require.NotNil(t, deleteAlerts)
	require.NoError(t, deleteAlerts.RunE(deleteAlerts, []string{"123"}))
	analysesCmd := newAnalysesCmd()
	deleteAnalyses := findCommand(analysesCmd, "delete")
	require.NotNil(t, deleteAnalyses)
	require.NoError(t, deleteAnalyses.RunE(deleteAnalyses, []string{"123"}))
	}

🤖 Prompt for AI Agents

In `@cmd/experiments/experiments_extra_test.go` around lines 464 - 484, The test
TestTasksAlertsAnalyses uses brittle Commands()[0] / Commands()[1] index access
on newTasksCmd, newAlertsCmd and newAnalysesCmd; change those to look up
subcommands by name using the existing findCommand helper (e.g. call
findCommand(tasksCmd, "delete"), findCommand(alertsCmd, "list") and
findCommand(alertsCmd, "delete"), findCommand(analysesCmd, "delete")) and then
call RunE on the returned *cobra.Command—this ensures the test targets the
correct subcommands regardless of registration order.

[coderabbitai]

🛠️ Refactor suggestion | 🟠 Major

Remove stdlib smoke-test assertions — they add no value.

Lines 588–592 assert the behaviour of strings.Contains and strings.ToLower from the standard library. These do not exercise any project code and appear to exist solely to inflate coverage metrics.

🤖 Prompt for AI Agents

In `@cmd/experiments/experiments_extra_test.go` around lines 586 - 593, The test
contains trivial assertions of standard library behavior (strings.Contains and
strings.ToLower) that should be removed; edit experiments_extra_test.go to
delete the assertions referencing strings.Contains/strings.ToLower (the four
assert.True/assert.False/assert.Equal lines) and keep only the project-related
assertion(s) such as require.NoError(t, runSearch(searchCmd,
[]string{"missing"})); ensure no other references to those removed checks remain
in the test.

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@cmd/experiments/experiments.go`:
- Around line 484-507: The UpdateExperiment command builds an empty
UpdateExperimentRequest and will send a no-op PUT when no flags are passed and
also disallows setting traffic to 0 because it uses `traffic > 0`; change the
flag handling to only set fields when the flag was actually provided (use
cmd.Flags().Changed("name"/"display-name"/"description"/"traffic") or
equivalent) and for `traffic` check presence rather than value so `0` is
allowed, then if none of the fields were set return a user-facing error (e.g.,
"no fields to update") before calling client.UpdateExperiment; update the code
paths around UpdateExperimentRequest, the flag checks, and the call to
client.UpdateExperiment accordingly.

🧹 Nitpick comments (3)

cmd/goals/goals.go (2)

204-224: Nice ordering — confirmation before client initialisation avoids unnecessary auth on cancel.

Minor nit: fmt.Println("Cancelled") on line 208 bypasses the output layer. If the CLI ever needs to support --quiet suppression or structured output for cancellation messages, this would need updating. Not urgent — just something to keep in mind.

---

52-52: Consider using cmd.Context() instead of context.Background() throughout.

All five command handlers use context.Background(). Cobra's cmd.Context() respects signal-based cancellation (e.g., Ctrl+C), which is more idiomatic for CLI tools and would allow long-running API calls to be interrupted gracefully. This applies to lines 52, 96, 135, 181, and 217.

Example diff (apply similarly to the other four call sites)

-	goals, err := client.ListGoals(context.Background(), opts)
+	goals, err := client.ListGoals(cmd.Context(), opts)

cmd/experiments/experiments.go (1)

884-892: Consider warning before overwriting an existing file.

os.WriteFile on line 886 silently overwrites any existing file at outputPath. A quick os.Stat check and prompting (or requiring --force) would be consistent with the confirmation pattern already used in runDelete.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Missing empty-update validation — sends an empty PUT when no flags are provided.

Per the commit messages, goals and metrics already guard against empty updates. The same check should be applied here. Additionally, the traffic > 0 guard on line 495 still prevents setting traffic to 0% (previously flagged).

Proposed fix

 	req := &api.UpdateExperimentRequest{}
+	hasUpdate := false
 
 	if name, _ := cmd.Flags().GetString("name"); name != "" {
 		req.Name = name
+		hasUpdate = true
 	}
 	if displayName, _ := cmd.Flags().GetString("display-name"); displayName != "" {
 		req.DisplayName = displayName
+		hasUpdate = true
 	}
 	if description, _ := cmd.Flags().GetString("description"); description != "" {
 		req.Description = description
+		hasUpdate = true
 	}
-	if traffic, _ := cmd.Flags().GetInt("traffic"); traffic > 0 {
+	if cmd.Flags().Changed("traffic") {
+		traffic, _ := cmd.Flags().GetInt("traffic")
 		req.Traffic = traffic
+		hasUpdate = true
 	}
 
+	if !hasUpdate {
+		return fmt.Errorf("no update flags provided; use --name, --display-name, --description, or --traffic")
+	}
+
 	exp, err := client.UpdateExperiment(context.Background(), args[0], req)

🤖 Prompt for AI Agents

In `@cmd/experiments/experiments.go` around lines 484 - 507, The UpdateExperiment
command builds an empty UpdateExperimentRequest and will send a no-op PUT when
no flags are passed and also disallows setting traffic to 0 because it uses
`traffic > 0`; change the flag handling to only set fields when the flag was
actually provided (use
cmd.Flags().Changed("name"/"display-name"/"description"/"traffic") or
equivalent) and for `traffic` check presence rather than value so `0` is
allowed, then if none of the fields were set return a user-facing error (e.g.,
"no fields to update") before calling client.UpdateExperiment; update the code
paths around UpdateExperimentRequest, the flag checks, and the call to
client.UpdateExperiment accordingly.

[coderabbitai]

Actionable comments posted: 4

🤖 Fix all issues with AI agents

In `@cmd/config/config.go`:
- Around line 141-181: There are two conflicting runSet functions causing a
compile error; consolidate the masked-input logic into the single runSet
function (merge the prompt/secure-read block that checks isSensitive and handles
"prompt" into the existing runSet that loads config, calls setConfigValue,
saveConfig, and prints the result), ensure the function has a single proper
closing brace and returns error as before, and add the missing imports for
syscall and golang.org/x/term so term.ReadPassword can be used; keep identifiers
runSet, setConfigValue, loadConfig, saveConfig, and the isSensitive detection
intact when merging.
- Line 179: The current runSet prints the full value (cmd.Printf("Set %s =
%s\n", key, value)), leaking secrets; update runSet to detect sensitive keys
(e.g., key names containing "token", "secret", "password", "passwd", "key",
"api_key") and replace the printed value with a masked version (like "***" or
show only last 4 chars) before calling cmd.Printf. Implement a small helper
(e.g., isSensitiveKey(key string) bool and maskValue(value string) string) and
use maskedValue in the existing cmd.Printf call so the stored value remains
unchanged but output never exposes secrets.

In `@cmd/experiments/notes.go`:
- Around line 85-97: The command currently builds a map `sets` from
`cmd.Flags().GetStringArray("set")` and calls `client.UpdateNote(...)` even when
`sets` is empty; change the handler to require at least one `--set` flag by
checking `len(setFlags)` or `len(sets)` after parsing and returning a
user-facing error (e.g., "please provide at least one --set field=value")
instead of calling `UpdateNote`; update the code around `setFlags, _ :=
cmd.Flags().GetStringArray("set")`, the `for` loop that fills `sets`, and the
`if err := client.UpdateNote(...)` call to enforce this validation.

In `@README.md`:
- Around line 46-48: The README uses "```bash" for both opening and closing
Markdown code fences which breaks rendering; scan the file for fenced blocks
where you open with "```bash" and ensure the corresponding closing fence is
plain "```" (replace the closing "```bash" occurrences with "```" for every
second occurrence of "```bash"); verify each code block now opens with "```bash"
and closes with "```" so language hint stays only on the opening fence.

🧹 Nitpick comments (5)

cmd/experiments/notes.go (4)

23-33: Inconsistent argument validation across sibling subcommands.

update and delete-all use cobra.ExactArgs(1), but list rolls its own check. Using Cobra's built-in validator keeps behaviour (error format, automatic usage hint) consistent and removes boilerplate.

Suggested fix

 	listCmd := &cobra.Command{
 		Use:     "list <experiment-id>",
 		Short:   "List notes for an experiment",
 		Long:    "Display all notes associated with an experiment",
 		Example: "  abs experiments notes list 23028",
+		Args:    cobra.ExactArgs(1),
 		RunE: func(cmd *cobra.Command, args []string) error {
-			if len(args) != 1 {
-				cmd.Println("Usage:")
-				cmd.Println("  " + cmd.UseLine())
-				return fmt.Errorf("accepts 1 arg(s), received %d", len(args))
-			}
-
 			client, err := cmdutil.GetExpctldClient()

---

136-138: Success message is misleading when a condition filter is applied.

When --condition action=start is used, the message still reads "All notes deleted for experiment X", which could confuse users into thinking every note was removed.

Suggested fix

-			printer.Success(fmt.Sprintf("All notes deleted for experiment %s", args[0]))
+			if conditionMap != nil {
+				printer.Success(fmt.Sprintf("Matching notes deleted for experiment %s (condition: %s)", args[0], condition))
+			} else {
+				printer.Success(fmt.Sprintf("All notes deleted for experiment %s", args[0]))
+			}

---

187-209: Consider a small helper to reduce the repeated date-formatting block.

The same nil-check-then-format pattern appears three times (lines 189-200). A tiny local closure would tighten this up.

Suggested fix

+	fmtDate := func(t *time.Time) string {
+		if t == nil {
+			return "N/A"
+		}
+		return t.Format("2006-01-02")
+	}
+
 	table := output.NewTableData("ITERATION", "STATE", "CREATED", "STARTED", "STOPPED", "NOTES COUNT")
 	for i, entry := range timeline.Entries {
-		createdAt := "N/A"
-		if entry.CreatedAt != nil {
-			createdAt = entry.CreatedAt.Format("2006-01-02")
-		}
-		startedAt := "N/A"
-		if entry.StartedAt != nil {
-			startedAt = entry.StartedAt.Format("2006-01-02")
-		}
-		stoppedAt := "N/A"
-		if entry.StoppedAt != nil {
-			stoppedAt = entry.StoppedAt.Format("2006-01-02")
-		}
 		table.AddRow(
 			strconv.Itoa(i+1),
 			entry.State,
-			createdAt,
-			startedAt,
-			stoppedAt,
+			fmtDate(entry.CreatedAt),
+			fmtDate(entry.StartedAt),
+			fmtDate(entry.StoppedAt),
 			strconv.Itoa(len(entry.Notes)),
 		)
 	}

Note: this would require adding "time" to the imports.

---

183-185: Nit: duplicated output.Format(outputFormat) cast.

The conversion is evaluated twice in the same condition. Storing it in a variable improves readability.

-	if output.Format(outputFormat) != output.FormatTable && output.Format(outputFormat) != output.FormatPlain {
+	format := output.Format(outputFormat)
+	if format != output.FormatTable && format != output.FormatPlain {

README.md (1)

2104-2119: Profile configuration example shows API keys in plaintext YAML.

The example config snippet includes api_key: prod_key_here and api_key: staging_key_here. Given the CLI supports keyring-based credential storage, consider noting that tokens should be stored via abs auth login (which uses the keyring) rather than in the config file, or at minimum add a comment warning that this is insecure.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Update proceeds with an empty payload when no --set flags are provided.

If the user runs notes update <id> --note-id X without any --set flags, sets is an empty map and UpdateNote is called with nothing to change. Consider requiring at least one --set flag to avoid a meaningless API call.

Suggested fix

 			sets := make(map[string]interface{})
 			for _, s := range setFlags {
 				parts := strings.SplitN(s, "=", 2)
 				if len(parts) != 2 {
 					return fmt.Errorf("invalid --set format: %s (expected field=value)", s)
 				}
 				sets[parts[0]] = parts[1]
 			}
+			if len(sets) == 0 {
+				return fmt.Errorf("at least one --set flag is required")
+			}

 			if err := client.UpdateNote(context.Background(), args[0], noteID, sets); err != nil {

🤖 Prompt for AI Agents

In `@cmd/experiments/notes.go` around lines 85 - 97, The command currently builds
a map `sets` from `cmd.Flags().GetStringArray("set")` and calls
`client.UpdateNote(...)` even when `sets` is empty; change the handler to
require at least one `--set` flag by checking `len(setFlags)` or `len(sets)`
after parsing and returning a user-facing error (e.g., "please provide at least
one --set field=value") instead of calling `UpdateNote`; update the code around
`setFlags, _ := cmd.Flags().GetStringArray("set")`, the `for` loop that fills
`sets`, and the `if err := client.UpdateNote(...)` call to enforce this
validation.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

Every fenced code block is closed with ```bash instead of ```, breaking all Markdown rendering.

Throughout the entire file, closing fences use ```bash (e.g., lines 48, 55, 61, 67, 74, 99, 155, 171, 180, …). A closing fence must be just ``` — the language identifier belongs only on the opening fence. This will cause renderers to treat the "closing" fence as a new opening fence, cascading into completely garbled output.

A quick find-and-replace for lines that are meant to close a block (i.e., every second occurrence of ```bash) will fix this.

Example fix (repeated for every affected block)

 ```bash
 curl -fsSL https://raw.githubusercontent.com/absmartly/cli/main/install.sh | sh
-```bash
+```

🤖 Prompt for AI Agents

In `@README.md` around lines 46 - 48, The README uses "```bash" for both opening
and closing Markdown code fences which breaks rendering; scan the file for
fenced blocks where you open with "```bash" and ensure the corresponding closing
fence is plain "```" (replace the closing "```bash" occurrences with "```" for
every second occurrence of "```bash"); verify each code block now opens with
"```bash" and closes with "```" so language hint stays only on the opening
fence.

[coderabbitai]

Actionable comments posted: 6

🤖 Fix all issues with AI agents

In @.claude/test_quality_analysis.md:
- Line 16: Replace absolute user-specific file paths in
.claude/test_quality_analysis.md with repo-relative paths: locate any
"Location:" entries or occurrences that start with "/Users/" (for example the
shown "/Users/joalves/git_tree/absmartly-cli/cmd/experiments/notes.go:120-129")
and change them to the repo-relative form "cmd/experiments/notes.go:120-129";
apply the same fix for the other reported ranges (around lines 94-96, 152, and
240) so all Location references use repo-relative paths instead of absolute user
paths.
- Line 372: Change the heading/phrase "429 Rate limiting" to use the hyphenated
compound adjective form "429 Rate-limiting" (or "Rate-limiting handling" where
it modifies "handling"); locate occurrences of the exact string "429 Rate
limiting" in .claude/test_quality_analysis.md and replace with "429
Rate-limiting" so the compound adjective is correctly hyphenated when it
modifies a noun.
- Line 245: Update the sentence "Falls back to visible input if term not
available" by inserting the missing verb so it reads "Falls back to visible
input if term is not available"; locate and replace that exact string in the
.claude/test_quality_analysis.md content (search for the phrase "Falls back to
visible input if term not available") and commit the corrected line.

In `@cmd/config/config.go`:
- Around line 223-236: runList currently marshals and prints the entire cfg (via
yamlMarshal) which may contain plaintext tokens set by setCredential; to fix,
create a sanitized copy of cfg inside runList before calling yamlMarshal and
redact sensitive fields (e.g. access_token, refresh_token, password, secret or
any fields on the profile struct that hold credentials) by replacing them with a
masked value like "***REDACTED***" or omitting them entirely, then marshal and
print the sanitized copy; ensure you reference the same profile struct fields
used by setCredential so all persisted token fields are covered.

In `@cmd/experiments/experiments_extra_test.go`:
- Around line 698-707: The test TestActivityOutputTerseFull uses fragile
index-based access NewActivityCmd().Commands()[0]; replace that with a lookup
using findCommand to fetch the specific subcommand by name (e.g. call
findCommand(NewActivityCmd(), "output")) and then call RunE on the returned
command; update the test to use findCommand(NewActivityCmd(), "output") instead
of Commands()[0] so the correct subcommand is found regardless of registration
order.
- Around line 204-215: The helper setupExperimentConfig currently calls
testutil.SetupConfig and then redundantly calls viper.Reset(); remove the
explicit viper.Reset() call from setupExperimentConfig so it only calls
testutil.SetupConfig(t, ...) (leave the t.Helper() and SetupConfig invocation
intact), since ResetViper is already handled inside testutil.SetupConfig; update
any imports if needed after removal.

🧹 Nitpick comments (4)

cmd/experiments/experiments_extra_test.go (4)

744-749: TestSearchHelpers does not test any project search helper — it only asserts stdlib behaviour.

The test name implies it exercises search helper functions, but it only calls strings.Contains / strings.ToLower on struct fields. Either invoke the actual search helper being tested (e.g., the matching logic inside runSearch) or remove this test.

---

1-21: Imports look correct; note bytes becomes unused if the manual stdout capture is replaced.

If the os.Pipe approach in TestGenerateTemplateCommand is replaced with testutil.CaptureStdout (as suggested), the "bytes" import on line 4 will become unused and should be removed.

---

30-173: newAPIServer route matching relies on case ordering in the switch statement.

The route-matching logic works correctly because more-specific suffix checks (e.g., /activity, /results, /start, /stop) precede the generic GET /experiments/{id} and PUT /experiments/{id} cases. However, this is fragile — inserting a new case in the wrong position could silently shadow an existing route.

Consider adding a brief comment near the top of the switch (e.g., // Order matters: specific suffixes before generic path matches) to make this constraint explicit for future maintainers.

---

239-256: Two distinct scenarios (empty list, table output) crammed into one test function.

TestRunListEmptyAndTable tests two independent flows: an empty-result list and a populated table-format list. Splitting into subtests (t.Run(...)) would improve failure diagnostics and make each scenario independently runnable.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Prefer repo-relative paths over absolute user paths.

Absolute paths (e.g., /Users/joalves/...) aren’t portable and can leak local machine details. Use repo-relative paths like cmd/experiments/notes.go:120-129.

📝 Suggested edit

-**Location:** `/Users/joalves/git_tree/absmartly-cli/cmd/experiments/notes.go:120-129`
+**Location:** `cmd/experiments/notes.go:120-129`

Also applies to: 94-96, 152-152, 240-240

🤖 Prompt for AI Agents

In @.claude/test_quality_analysis.md at line 16, Replace absolute user-specific
file paths in .claude/test_quality_analysis.md with repo-relative paths: locate
any "Location:" entries or occurrences that start with "/Users/" (for example
the shown
"/Users/joalves/git_tree/absmartly-cli/cmd/experiments/notes.go:120-129") and
change them to the repo-relative form "cmd/experiments/notes.go:120-129"; apply
the same fix for the other reported ranges (around lines 94-96, 152, and 240) so
all Location references use repo-relative paths instead of absolute user paths.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Fix missing verb in sentence.

“if term not available” → “if term is not available”.

📝 Suggested edit

-- Falls back to visible input if term not available
+- Falls back to visible input if term is not available

📝 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

	- Falls back to visible input if term not available
	- Falls back to visible input if term is not available

🧰 Tools

🪛 LanguageTool

[grammar] ~245-~245: A verb may be missing.
Context: ...t - Falls back to visible input if term not available Current Test Coverage: -...

(NN_NOT_JJ)

🤖 Prompt for AI Agents

In @.claude/test_quality_analysis.md at line 245, Update the sentence "Falls
back to visible input if term not available" by inserting the missing verb so it
reads "Falls back to visible input if term is not available"; locate and replace
that exact string in the .claude/test_quality_analysis.md content (search for
the phrase "Falls back to visible input if term not available") and commit the
corrected line.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Hyphenate compound adjective.

Use “rate-limiting” when modifying “handling”.

📝 Suggested edit

-- 429 Rate limiting
+- 429 rate-limiting

📝 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

	- 429 Rate limiting
	- 429 rate-limiting

🧰 Tools

🪛 LanguageTool

[uncategorized] ~372-~372: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...handling - 404 Not Found handling - 429 Rate limiting - Retry logic validation **Recommendat...

(EN_COMPOUND_ADJECTIVE_INTERNAL)

🤖 Prompt for AI Agents

In @.claude/test_quality_analysis.md at line 372, Change the heading/phrase "429
Rate limiting" to use the hyphenated compound adjective form "429 Rate-limiting"
(or "Rate-limiting handling" where it modifies "handling"); locate occurrences
of the exact string "429 Rate limiting" in .claude/test_quality_analysis.md and
replace with "429 Rate-limiting" so the compound adjective is correctly
hyphenated when it modifies a noun.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

runList dumps the full config as YAML, which may include tokens stored in plaintext.

When setCredential fails (lines 291-294, 298-301), tokens are written directly into the profile struct and persisted to the config file. runList then marshals and prints the entire config — including those plaintext tokens — to stdout. Consider masking sensitive fields before marshalling, or filtering them from the output.

🤖 Prompt for AI Agents

In `@cmd/config/config.go` around lines 223 - 236, runList currently marshals and
prints the entire cfg (via yamlMarshal) which may contain plaintext tokens set
by setCredential; to fix, create a sanitized copy of cfg inside runList before
calling yamlMarshal and redact sensitive fields (e.g. access_token,
refresh_token, password, secret or any fields on the profile struct that hold
credentials) by replacing them with a masked value like "***REDACTED***" or
omitting them entirely, then marshal and print the sanitized copy; ensure you
reference the same profile struct fields used by setCredential so all persisted
token fields are covered.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check how the production code reads API endpoint — does it use viper directly or re-read config?
echo "=== How api client is initialised ==="
rg -n --type=go -C5 'viper\.(Get|GetString).*api' --glob '!*_test.go'
echo ""
echo "=== Does SetupConfig call ResetViper internally? ==="
rg -n --type=go -B3 -A15 'func SetupConfig' --glob '!*_test.go' | head -50
echo ""
echo "=== How is config loaded at runtime? ==="
rg -n --type=go 'ReadInConfig|ReadConfig|SetConfigFile' --glob '!*_test.go'
echo ""
echo "=== Check testutil SetupConfig and ResetViper ==="
fd testutil.go | xargs cat -n | head -100

Repository: absmartly/cli

Length of output: 4600

---

🏁 Script executed:

#!/bin/bash
# Verify: does cmd.Execute() or similar call viper.ReadInConfig()?
echo "=== Check if test code calls cmd.Execute or similar ==="
rg -n --type=go 'Execute\(\)' cmd/experiments/experiments_extra_test.go | head -5
echo ""
echo "=== Verify when viper.ReadInConfig is called ==="
rg -n --type=go 'ReadInConfig' --glob '!*_test.go' -B2 -A2
echo ""
echo "=== Check what happens in cmd.Execute path ==="
ast-grep --pattern $'func Execute() $_ {
  $$$
}' | head -30

Repository: absmartly/cli

Length of output: 560

---

🏁 Script executed:

#!/bin/bash
# Check what functions are actually called in setupExperimentConfig tests
echo "=== Which test functions use setupExperimentConfig? ==="
rg -n 'setupExperimentConfig' cmd/experiments/experiments_extra_test.go
echo ""
echo "=== Check a sample test to see the flow ==="
sed -n '239,256p' cmd/experiments/experiments_extra_test.go
echo ""
echo "=== Check how newAPIServer is used and what commands are executed ==="
sed -n '226,237p' cmd/experiments/experiments_extra_test.go
echo ""
echo "=== Look at TestRunListEmptyAndTable to understand the test flow ==="
sed -n '239,260p' cmd/experiments/experiments_extra_test.go
echo ""
echo "=== Check if there's a cmd.Execute call ==="
rg -n 'rootCmd\|experiments\.NewCmdRegistersSubcommands\|NewCmd' cmd/experiments/experiments_extra_test.go | head -10

Repository: absmartly/cli

Length of output: 3428

---

🏁 Script executed:

#!/bin/bash
# Check how runList and similar functions work - do they call viper.ReadInConfig?
echo "=== What does runList do? ==="
rg -n --type=go -A10 'func runList' --glob '!*_test.go' | head -20
echo ""
echo "=== Check if viper.GetString is called directly in runList ==="
rg -n --type=go 'func runList' --glob '!*_test.go' -A50 | grep -i 'viper\|GetString' | head -10
echo ""
echo "=== Does config package do ReadInConfig when accessed? ==="
rg -n --type=go 'GetAPIEndpoint\|GetAPIToken' --glob '!*_test.go' -B5 -A5 | head -30

Repository: absmartly/cli

Length of output: 1800

---

🏁 Script executed:

#!/bin/bash
# Check what GetAPIClient does - does it read config?
echo "=== GetAPIClient implementation ==="
rg -n --type=go -A15 'func GetAPIClient' --glob '!*_test.go'
echo ""
echo "=== GetAPIEndpoint and GetAPIToken implementation ==="
rg -n --type=go -B2 -A10 'func.*GetAPIEndpoint\|func.*GetAPIToken' --glob '!*_test.go'
echo ""
echo "=== Check if config is loaded on demand ==="
rg -n --type=go 'func.*GetActiveProfile' --glob '!*_test.go' -A15 | head -40

Repository: absmartly/cli

Length of output: 1977

---

🏁 Script executed:

#!/bin/bash
# Check what loadConfig does
echo "=== loadConfig implementation ==="
rg -n --type=go -B2 -A15 'func loadConfig' --glob '!*_test.go'

Repository: absmartly/cli

Length of output: 2265

---

Remove the redundant viper.Reset() call — SetupConfig already resets viper internally.

testutil.SetupConfig calls ResetViper(t) at the start (line 42 of testutil.go), which clears viper and registers a cleanup handler. The explicit viper.Reset() at line 214 is redundant since the viper state will be reset again at test completion anyway. Remove it for clarity.

 func setupExperimentConfig(t *testing.T, apiURL, expctldURL string) {
 	t.Helper()
 	testutil.SetupConfig(t, testutil.ConfigOptions{
 		APIEndpoint:     apiURL,
 		APIToken:        "token",
 		ExpctldEndpoint: expctldURL,
 		ExpctldToken:    "token",
 		Application:     "app",
 		Environment:     "env",
 	})
-	viper.Reset()
 }

🤖 Prompt for AI Agents

In `@cmd/experiments/experiments_extra_test.go` around lines 204 - 215, The helper
setupExperimentConfig currently calls testutil.SetupConfig and then redundantly
calls viper.Reset(); remove the explicit viper.Reset() call from
setupExperimentConfig so it only calls testutil.SetupConfig(t, ...) (leave the
t.Helper() and SetupConfig invocation intact), since ResetViper is already
handled inside testutil.SetupConfig; update any imports if needed after removal.

[coderabbitai]

🛠️ Refactor suggestion | 🟠 Major

Use findCommand instead of Commands()[0] for subcommand lookup.

Same fragile index-based access pattern flagged elsewhere. If subcommand registration order changes, this silently tests the wrong command.

Proposed fix

 func TestActivityOutputTerseFull(t *testing.T) {
 	server := newAPIServer(t, apiServerOptions{})
 	t.Cleanup(server.Close)
 	setupExperimentConfig(t, server.URL, server.URL)

 	viper.Set("terse", true)
 	viper.Set("full", true)
-	cmd := NewActivityCmd().Commands()[0]
+	cmd := findCommand(NewActivityCmd(), "list")
+	require.NotNil(t, cmd)
 	require.NoError(t, cmd.RunE(cmd, []string{"123"}))
 }

🤖 Prompt for AI Agents

In `@cmd/experiments/experiments_extra_test.go` around lines 698 - 707, The test
TestActivityOutputTerseFull uses fragile index-based access
NewActivityCmd().Commands()[0]; replace that with a lookup using findCommand to
fetch the specific subcommand by name (e.g. call findCommand(NewActivityCmd(),
"output")) and then call RunE on the returned command; update the test to use
findCommand(NewActivityCmd(), "output") instead of Commands()[0] so the correct
subcommand is found regardless of registration order.

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In @.claude/test_quality_analysis.md:
- Line 110: Edit the sentence "Prevents sending empty PUT requests to API" to
remove the duplicated word "API" and tighten phrasing; replace it with "Prevents
sending empty PUT requests to the API" (or simply "Prevents sending empty PUT
requests") so the wording is concise and not repetitive.

🧹 Nitpick comments (5)

cmd/experiments/experiments_extra_test.go (3)

496-503: Use testutil.CaptureStdout instead of manual os.Pipe plumbing.

testutil.CaptureStdout is already used at line 315 in this same file. The manual pipe-based stdout capture here is inconsistent and more error-prone (e.g. the error from os.Pipe() on line 497 is silently discarded).

♻️ Proposed refactor

-	var stdout bytes.Buffer
-	orig := os.Stdout
-	r, w, _ := os.Pipe()
-	os.Stdout = w
-	require.NoError(t, runGenerateTemplate(cmd, nil))
-	_ = w.Close()
-	_, _ = stdout.ReadFrom(r)
-	_ = r.Close()
-	os.Stdout = orig
-	assert.Contains(t, stdout.String(), "My Name")
+	output := testutil.CaptureStdout(t, func() {
+		require.NoError(t, runGenerateTemplate(cmd, nil))
+	})
+	assert.Contains(t, output, "My Name")

---

744-749: TestSearchHelpers only tests stdlib string operations, not project search logic.

Similar to the stdlib smoke tests flagged at lines 588–592, this test constructs an api.Experiment but only asserts strings.Contains/strings.ToLower behaviour — no project helper or search function is exercised. Replace with assertions that call the actual search helpers from the production code, or remove.

---

30-173: Test server route matching uses broad prefix/suffix checks that could overlap.

For instance, a path like /experiments/123/foo/results would match the results case at line 63 even though it's not a valid route. In test code this is unlikely to cause issues, but using more precise path matching (e.g. a regex or splitting on /) would guard against accidental route collisions as more tests are added.

cmd/experiments/notes_test.go (1)

21-25: Happy-path test doesn't verify the DELETE request was actually sent.

For the "valid condition" case, the test only asserts err == nil. If RunE were to skip the API call silently (e.g. due to a short-circuit bug), this test would still pass. Consider having the mock handler record whether it was invoked and asserting on that for the success case.

♻️ Suggested approach

+		var called bool
 		{
 			name:      "valid condition",
 			condition: "action=start",
 			expectErr: false,
 		},

Then in the handler for the valid case, set a flag:

handler: func(w http.ResponseWriter, r *http.Request) {
    called = true
    w.WriteHeader(http.StatusNoContent)
},

And after RunE:

if !tt.expectErr {
    require.NoError(t, err)
    assert.True(t, called, "expected DELETE request to be made")
}

Since the handler is shared across all cases, you could scope the called flag per-subtest or use an atomic.Bool.

Also applies to: 72-79

cmd/config/config.go (1)

394-406: Injectable package-level vars are a pragmatic choice for testability.

This pattern works well for a CLI tool. Just be aware that it makes these functions effectively global mutable state, which could cause issues if tests run in parallel without proper isolation.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Remove duplicated word (“API”).
Tightens phrasing and avoids repetition.

Proposed edit

-- Prevents sending empty PUT requests to API
-- - API would reject anyway, but client-side validation is clearer
+- Prevents sending empty PUT requests to API
+- The API would reject anyway, but client-side validation is clearer

🧰 Tools

🪛 LanguageTool

[duplication] ~110-~110: Possible typo: you repeated a word.
Context: ... Prevents sending empty PUT requests to API - API would reject anyway, but client-side va...

(ENGLISH_WORD_REPEAT_RULE)

🤖 Prompt for AI Agents

In @.claude/test_quality_analysis.md at line 110, Edit the sentence "Prevents
sending empty PUT requests to API" to remove the duplicated word "API" and
tighten phrasing; replace it with "Prevents sending empty PUT requests to the
API" (or simply "Prevents sending empty PUT requests") so the wording is concise
and not repetitive.

[coderabbitai]

Actionable comments posted: 2

🤖 Fix all issues with AI agents

In `@cmd/goals/goals_validation_test.go`:
- Around line 3-9: The test file imports are missing net/http which causes
compilation errors where the HTTP handler in goals_validation_test.go uses
http.ResponseWriter and *http.Request; add "net/http" to the import block
alongside testing and the existing packages so the handler signatures compile
and any http types referenced (http.ResponseWriter, *http.Request) resolve.

In `@cmd/metrics/metrics_validation_test.go`:
- Around line 3-9: Add the missing net/http import to the import block so
references to http.ResponseWriter and *http.Request compile; update the import
list in cmd/metrics/metrics_validation_test.go (where the HTTP handler uses
http.ResponseWriter and *http.Request) to include "net/http" alongside the
existing imports.

🧹 Nitpick comments (2)

cmd/metrics/metrics_validation_test.go (1)

33-35: Consider verifying the request body sent to the mock server.

The test confirms no error is returned, but does not assert that the correct payload (e.g. {"description":"updated"}) was actually sent to the server. This is a minor gap — adding a body assertion in the handler would make the test more robust against silent regressions.

cmd/goals/goals_validation_test.go (1)

33-35: Same suggestion: consider asserting the request body in the mock handler.

As noted in the metrics test, verifying the payload sent to the server would strengthen this integration test.

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@cmd/config/config.go`:
- Around line 301-313: The code currently falls back to writing tokens into the
config when setCredential fails without notifying the user; update both the API
token and expctld token branches (the setCredential call handling around token,
expctldToken, profile and cfg.Profiles["default"]) to emit a clear warning to
stderr when setCredential returns an error — include the fact that the secret
will be stored unencrypted and append the actual error details (e.g., via
fmt.Fprintln(os.Stderr, ...) or the existing logger) so users know keyring
storage failed and can take action.

🧹 Nitpick comments (2)

cmd/goals/goals_validation_test.go (1)

35-35: Don't discard the error from Flags().Set.

If the "name" flag is ever renamed or removed, discarding this error will cause the test to run with no flag set, masking the real failure.

Proposed fix

-	_ = updateCmd.Flags().Set("name", "updated")
+	require.NoError(t, updateCmd.Flags().Set("name", "updated"))

cmd/config/config.go (1)

143-192: Extract duplicated isSensitive check into a helper.

The same sensitive-key detection logic appears twice in runSet (lines 148–151 and 181–184). Extracting it into a small helper improves DRY compliance and makes it easier to keep the criteria consistent if more patterns are added later.

♻️ Suggested helper

+func isSensitiveKey(key string) bool {
+	k := strings.ToLower(key)
+	return strings.Contains(k, "token") ||
+		strings.Contains(k, "key") ||
+		strings.Contains(k, "secret") ||
+		strings.Contains(k, "password")
+}

Then replace both inline blocks with isSensitiveKey(key).

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

No user feedback when credentials fall back to plaintext file storage.

When setCredential fails (keyring unavailable), the token is silently written into the config file. The user has no indication their secret is stored unencrypted on disk. A warning to stderr would be appropriate.

Proposed fix (example for API token block)

 	if token != "" {
 		if err := setCredential("api", "default", token); err != nil {
+			fmt.Fprintf(os.Stderr, "Warning: keyring unavailable, API token will be stored in plaintext config file\n")
 			profile.API.Token = token
 			cfg.Profiles["default"] = profile
 		}
 	}

 	if expctldToken != "" {
 		if err := setCredential("expctld", "default", expctldToken); err != nil {
+			fmt.Fprintf(os.Stderr, "Warning: keyring unavailable, Expctld token will be stored in plaintext config file\n")
 			profile.Expctld.Token = expctldToken
 			cfg.Profiles["default"] = profile
 		}
 	}

📝 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 token != "" {
	if err := setCredential("api", "default", token); err != nil {
	profile.API.Token = token
	cfg.Profiles["default"] = profile
	}
	}
	if expctldToken != "" {
	if err := setCredential("expctld", "default", expctldToken); err != nil {
	profile.Expctld.Token = expctldToken
	cfg.Profiles["default"] = profile
	}
	}
	if token != "" {
	if err := setCredential("api", "default", token); err != nil {
	fmt.Fprintf(os.Stderr, "Warning: keyring unavailable, API token will be stored in plaintext config file\n")
	profile.API.Token = token
	cfg.Profiles["default"] = profile
	}
	}
	if expctldToken != "" {
	if err := setCredential("expctld", "default", expctldToken); err != nil {
	fmt.Fprintf(os.Stderr, "Warning: keyring unavailable, Expctld token will be stored in plaintext config file\n")
	profile.Expctld.Token = expctldToken
	cfg.Profiles["default"] = profile
	}
	}

🤖 Prompt for AI Agents

In `@cmd/config/config.go` around lines 301 - 313, The code currently falls back
to writing tokens into the config when setCredential fails without notifying the
user; update both the API token and expctld token branches (the setCredential
call handling around token, expctldToken, profile and cfg.Profiles["default"])
to emit a clear warning to stderr when setCredential returns an error — include
the fact that the secret will be stored unencrypted and append the actual error
details (e.g., via fmt.Fprintln(os.Stderr, ...) or the existing logger) so users
know keyring storage failed and can take action.

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@cmd/config/config.go`:
- Around line 156-159: The fallback prompt uses fmt.Print which writes to stdout
and can contaminate piped output; update the prompt call to write to stderr by
replacing fmt.Print("Enter value: ") with fmt.Fprint(os.Stderr, "Enter value: ")
(leave fmt.Scanln(&value) as-is) so the interactive prompt matches other prompts
and doesn't leak into stdout; look for this code around the error-handling block
that checks err and references the variable value.

🧹 Nitpick comments (1)

cmd/config/config.go (1)

143-192: Extract duplicated isSensitive logic into a helper.

The same four strings.Contains checks appear at lines 148–151 and again at lines 181–184. Extracting a small helper would reduce duplication and make it easier to keep the list of sensitive key patterns consistent.

Proposed refactor

+func isSensitiveKey(key string) bool {
+	lower := strings.ToLower(key)
+	return strings.Contains(lower, "token") ||
+		strings.Contains(lower, "key") ||
+		strings.Contains(lower, "secret") ||
+		strings.Contains(lower, "password")
+}
+
 func runSet(cmd *cobra.Command, key, inputValue string) error {
 	value := inputValue
 
 	// If value looks like it might need masking, prompt securely
 	if value != "" {
-		isSensitive := strings.Contains(strings.ToLower(key), "token") ||
-			strings.Contains(strings.ToLower(key), "key") ||
-			strings.Contains(strings.ToLower(key), "secret") ||
-			strings.Contains(strings.ToLower(key), "password")
-
-		if isSensitive && value == "prompt" {
+		if isSensitiveKey(key) && value == "prompt" {
 			...
 		}
 	}
 	...
 	// Mask sensitive values in output
-	isSensitive := strings.Contains(strings.ToLower(key), "token") ||
-		strings.Contains(strings.ToLower(key), "key") ||
-		strings.Contains(strings.ToLower(key), "secret") ||
-		strings.Contains(strings.ToLower(key), "password")
-
 	displayValue := value
-	if isSensitive {
+	if isSensitiveKey(key) {
 		displayValue = "****"
 	}

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Fallback prompt writes to stdout instead of stderr.

Line 158 uses fmt.Print(...) which writes to stdout, whereas all other interactive prompts in this file use fmt.Fprint(os.Stderr, ...). If the CLI's stdout is piped, this prompt will contaminate the output stream.

Proposed fix

-				fmt.Print("Enter value: ")
+				fmt.Fprint(os.Stderr, "Enter value: ")

🤖 Prompt for AI Agents

In `@cmd/config/config.go` around lines 156 - 159, The fallback prompt uses
fmt.Print which writes to stdout and can contaminate piped output; update the
prompt call to write to stderr by replacing fmt.Print("Enter value: ") with
fmt.Fprint(os.Stderr, "Enter value: ") (leave fmt.Scanln(&value) as-is) so the
interactive prompt matches other prompts and doesn't leak into stdout; look for
this code around the error-handling block that checks err and references the
variable value.