Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions cmd/hookwise/cmd_notifications.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,12 @@ package main
import (
"context"
"fmt"
"os"
"strings"

"github.com/spf13/cobra"
"github.com/vishnujayvel/hookwise/internal/analytics"
"github.com/vishnujayvel/hookwise/internal/core"
"github.com/vishnujayvel/hookwise/internal/notifications"
)

Expand All @@ -27,14 +29,22 @@ surfaced via the status line or this command.`,
},
}

cmd.Flags().StringVar(&dataDir, "data-dir", "", "Path to the analytics SQLite DB file (defaults to ~/.hookwise/analytics.db)")
cmd.Flags().StringVar(&dataDir, "data-dir", "", "Path to the analytics SQLite DB file (defaults to config analytics.db_path / ~/.hookwise/analytics.db)")
cmd.Flags().IntVar(&limit, "limit", 20, "Maximum number of notifications to show")

return cmd
}

func runNotifications(cmd *cobra.Command, dataDir string, limit int) error {
db, err := analytics.Open(dataDir)
// Load config the same way the writer (dispatch) does so reader and writer
// agree on the DB path (ARCH-1: never hard-fail on a missing/broken config).
cwd, _ := os.Getwd()
config, err := core.LoadConfig(cwd)
if err != nil {
config = core.GetDefaultConfig()
}
Comment on lines +41 to +45

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Handle os.Getwd() failure explicitly before loading config.

If os.Getwd() fails, this code currently proceeds with an empty/invalid project dir and then silently falls back to defaults, which can point notifications to the wrong DB path despite a valid project config. Treat Getwd failure as a direct fallback branch.

Suggested patch
-	cwd, _ := os.Getwd()
-	config, err := core.LoadConfig(cwd)
-	if err != nil {
-		config = core.GetDefaultConfig()
-	}
+	config := core.GetDefaultConfig()
+	if cwd, wdErr := os.Getwd(); wdErr == nil {
+		if loaded, err := core.LoadConfig(cwd); err == nil {
+			config = loaded
+		}
+	}
 
 	db, err := analytics.Open(resolveAnalyticsDBPath(dataDir, config))
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cmd/hookwise/cmd_notifications.go` around lines 41 - 45, The os.Getwd() error
is being ignored with a blank identifier, allowing an empty cwd to be passed to
core.LoadConfig when the working directory cannot be determined. Instead of
ignoring the os.Getwd() error, check it explicitly and if os.Getwd() fails,
directly assign config to core.GetDefaultConfig() without attempting to load
config with an invalid cwd. This ensures that when getting the current working
directory fails, the code immediately falls back to defaults rather than
silently proceeding with an empty directory path that could result in pointing
notifications to the wrong database path.


db, err := analytics.Open(resolveAnalyticsDBPath(dataDir, config))
if err != nil {
return fmt.Errorf("failed to open analytics DB: %w", err)
}
Expand Down
50 changes: 50 additions & 0 deletions cmd/hookwise/cmd_notifications_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
package main

import (
"bytes"
"context"
"os"
"path/filepath"
"testing"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"github.com/vishnujayvel/hookwise/internal/notifications"
)

// TestNotifications_ReadsConfigDBPath is the #109 regression test: when a project
// config sets a custom analytics.db_path, `notifications` (with no --data-dir flag)
// must read THAT database — the same one `dispatch` writes to — not the default
// ~/.hookwise/analytics.db. Before the fix, notifications ignored config and printed
// "No notifications." from the default empty DB.
func TestNotifications_ReadsConfigDBPath(t *testing.T) {
tmpDir := t.TempDir()
// Isolate the default state dir so the "default" DB is empty (and absent).
t.Setenv("HOOKWISE_STATE_DIR", filepath.Join(tmpDir, ".hookwise"))

customDB := filepath.Join(tmpDir, "custom-notifications.db")

// A project dir with a hookwise.yaml pointing analytics at the custom DB.
projectDir := filepath.Join(tmpDir, "project")
require.NoError(t, os.MkdirAll(projectDir, 0o755))
cfgYAML := "analytics:\n enabled: true\n db_path: " + customDB + "\n"
require.NoError(t, os.WriteFile(filepath.Join(projectDir, "hookwise.yaml"), []byte(cfgYAML), 0o644))

// Seed one notification into the CUSTOM DB via the real NotificationService.
db := openTestDB(t, customDB)
ns := notifications.NewNotificationService(db)
require.NoError(t, ns.Create(context.Background(), "budget", "budget_threshold", "Cost exceeded $99.00"))
db.Close()

// Run `notifications` from the project dir with NO --data-dir flag.
t.Chdir(projectDir)
cmd := newNotificationsCmd()
buf := &bytes.Buffer{}
cmd.SetOut(buf)
cmd.SetArgs([]string{})
require.NoError(t, cmd.Execute())

out := buf.String()
assert.Contains(t, out, "Cost exceeded $99.00",
"notifications must read the config's db_path and show seeded notification, not 'No notifications.' from the default DB")
}
Loading