Fix nil pointer dereference in flows create command

[langtind]

Summary

• Fixed segmentation fault when running homeyctl flows create or homeyctl flows create --advanced
• The bug was caused by cmd.Name() == "create" matching all "create" commands, not just token create
• Added comprehensive tests for config loading logic (27 test cases)

Test plan

• All existing tests pass
• New tests verify flows create initializes apiClient correctly
• New tests verify token create still skips config loading (handles its own OAuth)
• Lint passes

Fixes #4
Fixes #5

Summary by CodeRabbit

• Bug Fixes

  • Improved configuration loading behavior for specific CLI commands, ensuring proper handling of configuration initialization during command execution.

• Tests

  • Added comprehensive unit tests to verify configuration loading behavior across various command paths.

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

[coderabbitai]

📝 Walkthrough

Walkthrough

The changes adjust command execution flow to ensure flows create properly initializes the API client by loading configuration, while permitting token create and specific other commands to bypass config loading during startup.

Changes

Cohort / File(s)	Summary
Command Execution Bypass Logic cmd/root.go	Modified PersistentPreRunE to skip config loading for homeyctl token create instead of all create commands. This ensures flows create loads the config and initializes the API client, preventing nil pointer errors.
Test Coverage cmd/root_test.go	Added TestCommandSkipsConfigLoading and TestFlowsCreateRequiresClient to verify that token create skips config loading while flows create requires an API client for proper initialization.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 A nil client caused flows to crash,
So we fixed where config loads in a flash,
Token create skips the check with grace,
While flows create holds its rightful place,
Segfaults vanquished—debugging pays! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

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

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the primary fix: addressing nil pointer dereference in the flows create command by adjusting config loading logic.
Linked Issues check	✅ Passed	Code changes fully address linked issues #4 and #5 by fixing PersistentPreRunE logic to use cmdPath instead of cmd.Name(), ensuring apiClient initializes for flows create while token create skips config loading.
Out of Scope Changes check	✅ Passed	All changes are scoped to fixing the nil pointer dereference in flows create: modified PersistentPreRunE bypass condition and added comprehensive tests for config loading behavior.

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

✨ Finishing touches

• 📝 Generate docstrings

---

🧹 Recent nitpick comments

cmd/root_test.go (1)

74-92: Consider extracting shared logic to avoid drift between production and test code.

The helper function duplicates the logic from PersistentPreRunE in root.go. If the production logic changes, this helper must be updated manually, which could lead to false positives/negatives in tests.

Consider extracting the skip logic into a shared unexported function that both PersistentPreRunE and tests can use:

♻️ Suggested refactor

In cmd/root.go:

// shouldSkipConfigLoading determines if a command should skip config loading
func shouldSkipConfigLoading(cmdPath, cmdName string) bool {
    if cmdName == "config" || cmdName == "version" || cmdName == "help" ||
        cmdName == "set-token" || cmdName == "set-host" || cmdName == "show" ||
        cmdName == "completion" || cmdName == "ai" || cmdName == "scopes" ||
        cmdName == "login" || cmdPath == "homeyctl token create" || cmdPath == "homeyctl" {
        return true
    }
    return false
}

Then in PersistentPreRunE:

if shouldSkipConfigLoading(cmd.CommandPath(), cmd.Name()) {
    return nil
}

And in cmd/root_test.go, import and use the same function directly instead of duplicating the logic.

---

📜 Recent review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 66d38d4 and 41add75.

📒 Files selected for processing (2)

• cmd/root.go
• cmd/root_test.go

🔇 Additional comments (3)

cmd/root.go (1)

66-73: Fix correctly disambiguates token create from flows create.

The change from cmd.Name() == "create" to cmdPath == "homeyctl token create" properly addresses the root cause of issues #4 and #5. Using the full command path ensures only the token create command (which handles its own OAuth) skips config loading, while flows create correctly proceeds to initialize apiClient.

cmd/root_test.go (2)

7-56: Comprehensive test coverage for the config loading logic.

The test cases thoroughly cover both commands that should skip config loading and those that require an API client. The explicit test cases for flows create vs token create directly validate the fix for issues #4 and #5.

---

58-72: Excellent targeted test for the specific bug fix.

This test directly validates the distinction between token create and flows create - the exact scenario that caused the nil pointer dereference in issues #4 and #5. Clear documentation of the bug and the expected behavior.

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review 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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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