Skip to content

Commit 87b7e1c

Browse files
authored
fix: L1 cosmenticFlags rename, H10 panic fix, L2 .hawk cwd-leak (#51)
* fix(permissions): rename cosmenticFlags to cosmeticFlags (L1) * fix(engine): use comma-ok form in formatNode to avoid panic on non-Expr nodes (H10) * fix(state): resolve .hawk/ paths to home dir to stop cwd-leak (L2)
1 parent bdb40a9 commit 87b7e1c

10 files changed

Lines changed: 158 additions & 13 deletions

File tree

internal/engine/integration.go

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,11 +2,13 @@ package engine
22

33
import (
44
"fmt"
5+
"path/filepath"
56
"strings"
67
"sync"
78
"time"
89

910
"github.com/GrayCodeAI/hawk/internal/engine/ctxmgr"
11+
"github.com/GrayCodeAI/hawk/internal/home"
1012
"github.com/GrayCodeAI/hawk/internal/types"
1113
"github.com/GrayCodeAI/tok"
1214
)
@@ -189,6 +191,11 @@ type SessionSummary struct {
189191
// NewIntegrationPipeline initializes all subsystems and returns a ready-to-use
190192
// pipeline orchestrator.
191193
func NewIntegrationPipeline() *IntegrationPipeline {
194+
// Resolve the user's home dir once so the learning-pipeline stores do not
195+
// leak into <cwd>/.hawk/ when hawk is run from inside its own source tree.
196+
// See L2 in docs/plans/fix-critical-and-high-review.md.
197+
homeRoot := home.Dir()
198+
192199
return &IntegrationPipeline{
193200
// Pre-query
194201
IntentClassifier: NewIntentClassifier(),
@@ -214,9 +221,9 @@ func NewIntegrationPipeline() *IntegrationPipeline {
214221
OutputRedactor: NewOutputRedactor(),
215222

216223
// Learning
217-
ExperienceStore: NewExperienceStore(".hawk/experience"),
218-
KnowledgeBase: NewKnowledgeBase(".hawk/knowledge"),
219-
FeedbackCollector: NewFeedbackCollector(".hawk/feedback"),
224+
ExperienceStore: NewExperienceStore(filepath.Join(homeRoot, ".hawk", "experience")),
225+
KnowledgeBase: NewKnowledgeBase(filepath.Join(homeRoot, ".hawk", "knowledge")),
226+
FeedbackCollector: NewFeedbackCollector(filepath.Join(homeRoot, ".hawk", "feedback")),
220227
SelfAssessor: NewSelfAssessor(),
221228

222229
// Session management
Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
package engine
2+
3+
import (
4+
"os"
5+
"path/filepath"
6+
"strings"
7+
"testing"
8+
)
9+
10+
// TestL2PipelineStatePathsAreHomeRelative is a regression guard for L2 —
11+
// the three learning-pipeline stores (ExperienceStore, KnowledgeBase,
12+
// FeedbackCollector) created by NewIntegrationPipeline must write to
13+
// ~/.hawk/{experience,knowledge,feedback}/, not to <cwd>/.hawk/...
14+
//
15+
// Pre-fix, NewIntegrationPipeline passed the literal strings
16+
// ".hawk/experience", ".hawk/knowledge", ".hawk/feedback" to those
17+
// constructors, which leaked into <cwd>/cmd/.hawk/ when hawk was run
18+
// from its own source tree.
19+
func TestL2PipelineStatePathsAreHomeRelative(t *testing.T) {
20+
home, err := os.UserHomeDir()
21+
if err != nil {
22+
t.Fatalf("os.UserHomeDir: %v", err)
23+
}
24+
if home == "" {
25+
t.Fatal("os.UserHomeDir returned empty string")
26+
}
27+
wantPrefix := filepath.Clean(home) + string(filepath.Separator)
28+
29+
check := func(name, got string) {
30+
t.Helper()
31+
if !filepath.IsAbs(got) {
32+
t.Errorf("%s: path %q is not absolute", name, got)
33+
return
34+
}
35+
if !strings.HasPrefix(got, wantPrefix) && !strings.HasPrefix(got, filepath.Clean(home)) {
36+
t.Errorf("%s: path %q does not start with home dir %q", name, got, home)
37+
}
38+
}
39+
40+
p := NewIntegrationPipeline()
41+
if p == nil {
42+
t.Fatal("NewIntegrationPipeline returned nil")
43+
}
44+
if p.ExperienceStore == nil || p.KnowledgeBase == nil || p.FeedbackCollector == nil {
45+
t.Fatal("NewIntegrationPipeline left a learning-pipeline store nil")
46+
}
47+
48+
check("ExperienceStore.Dir", p.ExperienceStore.Dir)
49+
check("KnowledgeBase.Dir", p.KnowledgeBase.Dir)
50+
check("FeedbackCollector.Dir", p.FeedbackCollector.Dir)
51+
}

internal/engine/semantic_diff.go

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -967,7 +967,10 @@ func formatNode(fset *token.FileSet, node ast.Node) string {
967967
case *ast.Ident:
968968
return t.Name
969969
default:
970-
return formatFieldType(node.(ast.Expr)) //nolint:errcheck
970+
if expr, ok := node.(ast.Expr); ok {
971+
return formatFieldType(expr)
972+
}
973+
return "unknown"
971974
}
972975
}
973976

internal/engine/semantic_diff_test.go

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package engine
22

33
import (
4+
"go/ast"
45
"strings"
56
"testing"
67
)
@@ -691,3 +692,20 @@ func TestGenerateSummaryNoAPIs(t *testing.T) {
691692
t.Error("should not contain Affected APIs when there are none")
692693
}
693694
}
695+
696+
// TestFormatNodeNonExprRegression guards H10 — a non-ast.Expr node (e.g. *ast.Comment)
697+
// must not panic; the comma-ok form should fall through to "unknown".
698+
func TestFormatNodeNonExprRegression(t *testing.T) {
699+
// *ast.Comment is ast.Node but not ast.Expr. Pre-fix this panicked
700+
// with "interface conversion: *ast.Comment is not ast.Expr".
701+
defer func() {
702+
if r := recover(); r != nil {
703+
t.Fatalf("formatNode panicked on non-Expr node: %v", r)
704+
}
705+
}()
706+
707+
got := formatNode(nil, &ast.Comment{Text: "x"})
708+
if got != "unknown" {
709+
t.Errorf("formatNode(*ast.Comment) = %q, want %q", got, "unknown")
710+
}
711+
}

internal/permissions/canonicalize.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ var BannedPrefixes = []string{
3131
}
3232

3333
// cosmetic flags that don't affect safety
34-
var cosmenticFlags = map[string]bool{
34+
var cosmeticFlags = map[string]bool{
3535
"--color": true,
3636
"--no-color": true,
3737
"-v": true,
@@ -142,7 +142,7 @@ func (c *Canonicalizer) canonicalizeSingle(cmd string) string {
142142
// Strip cosmetic flags
143143
var filtered []string
144144
for _, tok := range tokens {
145-
if !cosmenticFlags[tok] {
145+
if !cosmeticFlags[tok] {
146146
filtered = append(filtered, tok)
147147
}
148148
}
@@ -324,7 +324,7 @@ func (c *Canonicalizer) GeneratePattern(command string) string {
324324
tok := tokens[argIdx]
325325
if strings.HasPrefix(tok, "-") {
326326
// Skip cosmetic flags from the pattern
327-
if !cosmenticFlags[tok] {
327+
if !cosmeticFlags[tok] {
328328
prefix = append(prefix, tok)
329329
}
330330
argIdx++
Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
package snapshot
2+
3+
import (
4+
"os"
5+
"path/filepath"
6+
"strings"
7+
"testing"
8+
)
9+
10+
// TestL2DefaultPathsAreHomeRelative is a regression guard for L2 — when the
11+
// state-store constructors are called with empty/zero args, their default
12+
// paths must be absolute and live under the user's home dir
13+
// (~/.hawk/...), not relative to <cwd>. Pre-fix, the defaults were strings
14+
// like ".hawk/snapshots" and ".hawk/experience" which leaked into
15+
// <cwd>/cmd/.hawk/ when hawk was run from its own source tree.
16+
func TestL2DefaultPathsAreHomeRelative(t *testing.T) {
17+
home, err := os.UserHomeDir()
18+
if err != nil {
19+
t.Fatalf("os.UserHomeDir: %v", err)
20+
}
21+
if home == "" {
22+
t.Fatal("os.UserHomeDir returned empty string")
23+
}
24+
25+
// Sanitize HOME so we can compare reliably (filepath.Clean strips
26+
// trailing separators).
27+
wantPrefix := filepath.Clean(home) + string(filepath.Separator)
28+
29+
check := func(name, got string) {
30+
t.Helper()
31+
if !filepath.IsAbs(got) {
32+
t.Errorf("%s: default path %q is not absolute", name, got)
33+
return
34+
}
35+
// On macOS temp dirs may live under /private/var/... while HOME
36+
// resolves to /var/...; compare both forms.
37+
if !strings.HasPrefix(got, wantPrefix) && !strings.HasPrefix(got, filepath.Clean(home)) {
38+
t.Errorf("%s: default path %q does not start with home dir %q", name, got, home)
39+
}
40+
}
41+
42+
// NewSnapshotStore("") default
43+
ss := NewSnapshotStore("")
44+
check("NewSnapshotStore", ss.Dir)
45+
46+
// New(<projectDir>) default — shadowDir is now home-relative, not
47+
// relative to projectDir.
48+
tracker := New(t.TempDir())
49+
check("New(tracker)", tracker.shadowDir)
50+
}

internal/snapshot/snapshot.go

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,8 @@ import (
99
"strings"
1010
"sync"
1111
"time"
12+
13+
"github.com/GrayCodeAI/hawk/internal/home"
1214
)
1315

1416
// Tracker maintains a shadow git repository that records every file change
@@ -36,10 +38,13 @@ type FileDiff struct {
3638
}
3739

3840
// New creates a Tracker for the given project directory.
41+
// The shadow git repository lives under the user's home dir (~/.hawk/snapshots)
42+
// rather than under projectDir, so that running hawk from inside a Go project
43+
// root no longer creates a nested <cwd>/cmd/.hawk/ tree at runtime.
3944
func New(projectDir string) *Tracker {
4045
return &Tracker{
4146
projectDir: projectDir,
42-
shadowDir: filepath.Join(projectDir, ".hawk", "snapshots"),
47+
shadowDir: filepath.Join(home.Dir(), ".hawk", "snapshots"),
4348
}
4449
}
4550

internal/snapshot/snapshot_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -51,8 +51,8 @@ func TestTracker_Init(t *testing.T) {
5151
t.Fatalf("Second Init failed: %v", err)
5252
}
5353

54-
if _, err := os.Stat(filepath.Join(dir, ".hawk", "snapshots", ".git")); err != nil {
55-
t.Error("shadow git repo not initialized")
54+
if _, err := os.Stat(filepath.Join(tracker.shadowDir, ".git")); err != nil {
55+
t.Errorf("shadow git repo not initialized at %s: %v", tracker.shadowDir, err)
5656
}
5757
}
5858

internal/snapshot/workspace.go

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,8 @@ import (
1616
"strings"
1717
"sync"
1818
"time"
19+
20+
"github.com/GrayCodeAI/hawk/internal/home"
1921
)
2022

2123
// WorkspaceSnapshot captures the full state of a project at a point in time.
@@ -68,10 +70,12 @@ var ignoredDirs = map[string]bool{
6870
}
6971

7072
// NewSnapshotStore creates a new SnapshotStore with the given directory.
71-
// If dir is empty, defaults to ".hawk/snapshots/".
73+
// If dir is empty, defaults to "~/.hawk/snapshots" (the user's home dir) so
74+
// that state does not leak into <cwd>/.hawk/ when hawk is run from inside
75+
// a Go project root.
7276
func NewSnapshotStore(dir string) *SnapshotStore {
7377
if dir == "" {
74-
dir = filepath.Join(".hawk", "snapshots")
78+
dir = filepath.Join(home.Dir(), ".hawk", "snapshots")
7579
}
7680
return &SnapshotStore{
7781
Dir: dir,

internal/snapshot/workspace_test.go

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -602,7 +602,14 @@ func TestRestore_PreservesGitDir(t *testing.T) {
602602

603603
func TestNewSnapshotStore_DefaultDir(t *testing.T) {
604604
store := NewSnapshotStore("")
605-
expected := filepath.Join(".hawk", "snapshots")
605+
// L2: the default path is now home-relative (~/.hawk/snapshots) so
606+
// state stops leaking into <cwd>/.hawk/ when hawk is run from
607+
// inside a Go project root.
608+
home, err := os.UserHomeDir()
609+
if err != nil {
610+
t.Fatalf("os.UserHomeDir: %v", err)
611+
}
612+
expected := filepath.Join(home, ".hawk", "snapshots")
606613
if store.Dir != expected {
607614
t.Errorf("expected default dir %q, got %q", expected, store.Dir)
608615
}

0 commit comments

Comments
 (0)