Skip to content

Commit e61c143

Browse files
Patel230claude
andauthored
fix/deep review improvements (#57)
* refactor(filter): remove unused reserved layer config types Remove QuestionAwareLayerConfig, DensityAdaptiveLayerConfig, NumericalQuantLayerConfig, and DynamicRatioLayerConfig (all marked "reserved for future implementation") plus their never-read fields on LayerConfig. Verified unreferenced via grep across the repo. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(tracking): include db path and operation in wrapped errors All error paths in tracking.go now report the database path and the operation that failed (open, create dir, init schema, enable WAL, record, aggregate, recent, scan, prune), making failures diagnosable without enabling debug logging. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(unsafe): add safety-invariant comments at unsafe conversions Each unsafe string<->[]byte conversion in internal/fastops/simd.go and internal/filter/zerocopy.go now carries an explicit "safe: ..." comment stating the aliasing invariant (backing array not mutated while the alias is live). Also migrated the legacy *(*string)(unsafe.Pointer(&b)) header-punning pattern to the supported unsafe.String/unsafe.Slice + unsafe.SliceData/StringData APIs, which are valid under the Go memory model (the old slice->string pun read a slice header as a string header). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 parent 138f7e4 commit e61c143

5 files changed

Lines changed: 34 additions & 49 deletions

File tree

CHANGELOG.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
2121
(`hawk`, `eyrie`, `yaad`, `sight`, `inspect`). No git tag has been cut yet.
2222

2323
### Removed
24+
- Unused internal config structs `QuestionAwareLayerConfig`,
25+
`DensityAdaptiveLayerConfig`, `NumericalQuantLayerConfig`, and
26+
`DynamicRatioLayerConfig` (and their fields on the internal `LayerConfig`)
27+
that were marked "reserved for future implementation" and never referenced.
2428
- `scripts/deploy.sh` — referenced a Dockerfile, k8s manifests, and an
2529
`internal/integration/` package that never existed. tok is a Go library
2630
(no binary, no HTTP server); deployment happens via `go get`.

internal/fastops/simd.go

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -168,15 +168,18 @@ func FastLower(s string) string {
168168
return s
169169
}
170170

171-
// Convert using unsafe for zero-copy when possible
172-
b := []byte(s)
171+
// Convert using unsafe to avoid a second copy on the way back to string.
172+
b := []byte(s) // fresh copy of s; we own this backing array exclusively
173173
for i := 0; i < len(b); i++ {
174174
c := b[i]
175175
if c >= 'A' && c <= 'Z' {
176176
b[i] = c + ('a' - 'A')
177177
}
178178
}
179-
return *(*string)(unsafe.Pointer(&b))
179+
// safe: b is a private copy created above; no other reference to its
180+
// backing array exists and it is never mutated after this point, so
181+
// aliasing it as a string preserves string immutability.
182+
return unsafe.String(unsafe.SliceData(b), len(b))
180183
}
181184

182185
// FastEqual compares strings with early exit

internal/filter/pipeline_types.go

Lines changed: 0 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -225,41 +225,12 @@ type AgentMemoryLayerConfig struct {
225225
ConsolidationMax int
226226
}
227227

228-
// QuestionAwareLayerConfig groups T12 settings.
229-
// NOTE: Currently unused - reserved for future implementation.
230-
type QuestionAwareLayerConfig struct {
231-
Enabled bool
232-
Threshold float64
233-
}
234-
235-
// DensityAdaptiveLayerConfig groups T17 settings.
236-
// NOTE: Currently unused - reserved for future implementation.
237-
type DensityAdaptiveLayerConfig struct {
238-
Enabled bool
239-
TargetRatio float64
240-
Threshold float64
241-
}
242-
243228
// TFIDFLayerConfig groups TF-IDF filter settings.
244229
type TFIDFLayerConfig struct {
245230
Enabled bool
246231
Threshold float64
247232
}
248233

249-
// NumericalQuantLayerConfig groups numerical quantization settings.
250-
// NOTE: Currently unused - reserved for future implementation.
251-
type NumericalQuantLayerConfig struct {
252-
Enabled bool
253-
DecimalPlaces int
254-
}
255-
256-
// DynamicRatioLayerConfig groups dynamic compression ratio settings.
257-
// NOTE: Currently unused - reserved for future implementation.
258-
type DynamicRatioLayerConfig struct {
259-
Enabled bool
260-
Base float64
261-
}
262-
263234
// LayerConfig groups per-layer config structs.
264235
type LayerConfig struct {
265236
Core CoreLayersConfig
@@ -273,11 +244,7 @@ type LayerConfig struct {
273244
LazyPruner LazyPrunerLayerConfig
274245
SemanticAnchor SemanticAnchorLayerConfig
275246
AgentMemory AgentMemoryLayerConfig
276-
QuestionAware QuestionAwareLayerConfig
277-
DensityAdaptive DensityAdaptiveLayerConfig
278247
TFIDF TFIDFLayerConfig
279-
NumericalQuant NumericalQuantLayerConfig
280-
DynamicRatio DynamicRatioLayerConfig
281248
SymbolicCompress bool
282249
PhraseGrouping bool
283250
Hypernym bool

internal/filter/zerocopy.go

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,10 @@ func (z *ZeroCopyBuffer) String() string {
3232
if len(z.data) == 0 {
3333
return ""
3434
}
35-
return *(*string)(unsafe.Pointer(&z.data))
35+
// safe: the string header aliases z.data's backing array. The invariant
36+
// (documented above) is that the backing array is not mutated while the
37+
// returned string is live — Append/Reset after String() violate it.
38+
return unsafe.String(unsafe.SliceData(z.data), len(z.data))
3639
}
3740

3841
func (z *ZeroCopyBuffer) Reset() {
@@ -51,7 +54,11 @@ func StringToBytes(s string) []byte {
5154
if len(s) == 0 {
5255
return nil
5356
}
54-
return *(*[]byte)(unsafe.Pointer(&s))
57+
// safe: the slice aliases the string's backing array (len == cap == len(s)).
58+
// The invariant is that the backing array is never mutated through the
59+
// returned slice — callers treat it as read-only, preserving string
60+
// immutability.
61+
return unsafe.Slice(unsafe.StringData(s), len(s))
5562
}
5663

5764
// BytesToString converts []byte to string without allocation.
@@ -62,5 +69,8 @@ func BytesToString(b []byte) string {
6269
if len(b) == 0 {
6370
return ""
6471
}
65-
return *(*string)(unsafe.Pointer(&b))
72+
// safe: the string header aliases b's backing array. The invariant is that
73+
// the backing array is not mutated (through b or any other alias) while the
74+
// returned string is live, preserving string immutability.
75+
return unsafe.String(unsafe.SliceData(b), len(b))
6676
}

internal/tracking/tracking.go

Lines changed: 11 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@ type Event struct {
4444
// Tracker is the public type. Construct with New() or NewAt(path).
4545
type Tracker struct {
4646
db *sql.DB
47+
path string
4748
mu sync.Mutex
4849
retenDays int
4950
closed bool
@@ -77,16 +78,16 @@ func NewAt(ctx context.Context, path string) (*Tracker, error) {
7778
return nil, errors.New("tracking: empty path")
7879
}
7980
if err := mkdirAll(filepath.Dir(path), 0o700); err != nil {
80-
return nil, fmt.Errorf("tracking: create dir: %w", err)
81+
return nil, fmt.Errorf("tracking: create dir %q: %w", filepath.Dir(path), err)
8182
}
8283
db, err := sql.Open("sqlite", path)
8384
if err != nil {
84-
return nil, fmt.Errorf("tracking: open db: %w", err)
85+
return nil, fmt.Errorf("tracking: open db at %q: %w", path, err)
8586
}
8687
// SQLite is single-writer; use a per-connection mutex to serialize
8788
// writes without serializing reads.
8889
db.SetMaxOpenConns(1)
89-
t := &Tracker{db: db, retenDays: 90}
90+
t := &Tracker{db: db, path: path, retenDays: 90}
9091
if err := t.initSchema(ctx); err != nil {
9192
_ = db.Close()
9293
return nil, err
@@ -127,7 +128,7 @@ func (t *Tracker) initSchema(ctx context.Context) error {
127128
CREATE INDEX IF NOT EXISTS idx_events_command ON events(command);
128129
`)
129130
if err != nil {
130-
return fmt.Errorf("tracking: init schema: %w", err)
131+
return fmt.Errorf("tracking: init schema at %q: %w", t.path, err)
131132
}
132133
return nil
133134
}
@@ -137,7 +138,7 @@ func (t *Tracker) initSchema(ctx context.Context) error {
137138
func (t *Tracker) enableWAL(ctx context.Context) error {
138139
_, err := t.db.ExecContext(ctx, `PRAGMA journal_mode=WAL;`)
139140
if err != nil {
140-
return fmt.Errorf("tracking: enable WAL: %w", err)
141+
return fmt.Errorf("tracking: enable WAL at %q: %w", t.path, err)
141142
}
142143
return nil
143144
}
@@ -166,7 +167,7 @@ func (t *Tracker) Record(ctx context.Context, ev Event) error {
166167
ev.Mode, ev.Tier, ev.Model,
167168
)
168169
if err != nil {
169-
return fmt.Errorf("tracking: record: %w", err)
170+
return fmt.Errorf("tracking: record event in db at %q: %w", t.path, err)
170171
}
171172
return nil
172173
}
@@ -209,7 +210,7 @@ func (t *Tracker) Aggregate(ctx context.Context, days int) (Aggregate, error) {
209210
WHERE ts >= ?
210211
`, since)
211212
if err := row.Scan(&agg.EventCount, &agg.TotalBytesSaved, &agg.AvgSavingsPct); err != nil {
212-
return agg, fmt.Errorf("tracking: aggregate: %w", err)
213+
return agg, fmt.Errorf("tracking: aggregate query on db at %q: %w", t.path, err)
213214
}
214215
agg.PeriodStart = time.Unix(since, 0)
215216
agg.PeriodEnd = time.Now()
@@ -235,7 +236,7 @@ func (t *Tracker) Recent(ctx context.Context, n int) ([]Event, error) {
235236
LIMIT ?
236237
`, n)
237238
if err != nil {
238-
return nil, fmt.Errorf("tracking: recent: %w", err)
239+
return nil, fmt.Errorf("tracking: recent query on db at %q: %w", t.path, err)
239240
}
240241
defer func() { _ = rows.Close() }()
241242
var out []Event
@@ -246,7 +247,7 @@ func (t *Tracker) Recent(ctx context.Context, n int) ([]Event, error) {
246247
&ev.OriginalBytes, &ev.CompressedBytes,
247248
&ev.OriginalTokens, &ev.CompressedTokens,
248249
&ev.Mode, &ev.Tier, &ev.Model); err != nil {
249-
return nil, err
250+
return nil, fmt.Errorf("tracking: scan event row from db at %q: %w", t.path, err)
250251
}
251252
ev.Timestamp = time.Unix(ts, 0)
252253
if ev.OriginalBytes > 0 {
@@ -266,7 +267,7 @@ func (t *Tracker) Prune(ctx context.Context) (int64, error) {
266267
cutoff := time.Now().Add(-time.Duration(t.retenDays) * 24 * time.Hour).Unix()
267268
res, err := t.db.ExecContext(ctx, `DELETE FROM events WHERE ts < ?`, cutoff)
268269
if err != nil {
269-
return 0, fmt.Errorf("tracking: prune: %w", err)
270+
return 0, fmt.Errorf("tracking: prune db at %q: %w", t.path, err)
270271
}
271272
return res.RowsAffected()
272273
}

0 commit comments

Comments
 (0)