feat: architecture-v2 part 2 — batches G-K + CodeRabbit fixes#73
Closed
vishnujayvel wants to merge 5 commits into
Closed
feat: architecture-v2 part 2 — batches G-K + CodeRabbit fixes#73vishnujayvel wants to merge 5 commits into
vishnujayvel wants to merge 5 commits into
Conversation
50 type definitions split by domain, all in package core: - config_types.go (19 types): HooksConfig, CoachingConfig, etc. - dispatch_types.go (3): HookPayload, DispatchResult, HandlerResult - guard_types.go (3): GuardRuleConfig, GuardResult, ParsedCondition - feed_types.go (10): CacheEntry, FeedDefinition, feed configs - analytics_types.go (6): AnalyticsEvent, SessionSummary, etc. - handler_types.go (3): ResolvedHandler, SegmentConfig, etc. - types.go (6): Event constants, ValidEventType, test types No exported API changes. All consumers see identical symbols. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Time parsing: - Calendar producer: replace 4-format fallback loop + ad-hoc time.Parse with ParseTimeFlex (2 call sites) - HomeDir and remaining time.Parse already consolidated in part 1 CodeRabbit fixes: - producer_project: add 30s context.WithTimeout before git calls - producer_insights: honor incoming ctx, add cancellation checks - producer_calendar: use core.AtomicWriteJSON for crash-safe token writes Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ch I) SQL extraction: - Move GuardBlockSummary + query from notifications to analytics.DB method - No raw SQL referencing analytics tables remains in notifications Dead code: - Delete WriteFeedCacheJSON/WriteFeedCacheJSONTo (zero production callers) - Delete 3 associated tests CodeRabbit fixes: - Narrow ALTER TABLE error handling to only ignore duplicate column errors - Mark expired notifications as surfaced to prevent infinite rescan Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Per-producer tests covering envelope structure, error paths, and fallback validation for all 6 feed producers: - Shared assertValidEnvelope helper (3-key check, RFC3339, no source key) - producer_project: 5 tests (envelope, fallback, non-git, cancelled ctx) - producer_news: 4 tests (envelope, source key, Name(), no-config) - producer_calendar: 5 tests (envelope, fallback, missing token, fail-open) - producer_weather: 9 tests (mock API, error paths, cached fallback) - producer_memories: 5 tests (envelope, source key, Name(), data fields) - producer_insights: 8 tests (envelope, zeroed, missing dir, ctx cancel) All tests verify ARCH-1 fail-open: errors return valid fallback envelopes. Bug #29 regression guard: no producer returns a top-level "source" key. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughRefactors core type definitions into multiple files, adds analytics DB API Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/analytics/dolt.go`:
- Around line 341-355: GuardBlockSummaries currently hardcodes HAVING cnt >= 5;
change its signature to accept a minCount int (func (d *DB)
GuardBlockSummaries(ctx context.Context, today string, minCount int)
([]GuardBlockSummary, error)), update the SQL to use HAVING cnt >= ? and pass
minCount as the parameter (instead of hardcoded 5), and then remove the
duplicate cutoff logic from the caller in the notifications producer so the
caller supplies the threshold rather than re-filtering results.
In `@internal/core/config_types.go`:
- Around line 104-108: The JSON tag for TranscriptConfig.MaxSizeMB uses
inconsistent casing ("maxSizeMb"); update the struct tag for the MaxSizeMB field
in type TranscriptConfig to "maxSizeMB" so the JSON key matches the field's MB
abbreviation and project conventions—i.e., change the `json:"maxSizeMb"` on
MaxSizeMB to `json:"maxSizeMB"`.
In `@internal/core/dispatch_types.go`:
- Line 8: The Extra field on HookPayload is tagged `json:"-"` so it never
receives unknown JSON properties; either remove that tag (e.g., make it `Extra
map[string]interface{}` or use a marshaling tag that allows decoding) or
implement a custom UnmarshalJSON on the HookPayload type that decodes into a
temp map, extracts known fields into the struct and stores any leftover keys
into Extra; update payloadToMap to rely on the populated Extra. Locate the Extra
field declaration and the HookPayload type, then implement one of these fixes so
payloadToMap sees the unknown fields as intended.
In `@internal/feeds/producer_calendar_test.go`:
- Around line 100-123: The test
TestCalendarProducer_TestFixture_FieldConsistency uses unchecked type assertions
on result and its "data" field which can panic; update the test to validate
types using require before asserting keys: check that result is a
map[string]interface{} (from p.Produce), then that result["data"] exists and is
a map[string]interface{}, using require.IsType/require.True/require.NotNil (or
equivalent require assertions) so failures produce clear test errors rather than
panics; ensure you reference the variables result and realData and the
CalendarProducer.Produce call when adding these checks.
In `@internal/feeds/producer_insights_test.go`:
- Around line 163-179: The test uses unchecked type assertions on the envelope
data causing potential panics; update
TestInsightsProducer_TestFixture_FieldConsistency to defensively assert types
and presence using require (e.g., require.IsType/require.NotNil/require.True)
before casting: call assertValidEnvelope to get the envelope, use require to
confirm envelope["data"] is a map[string]interface{} (and non-nil) before
assigning fixtureData, and do the same for zeroed :=
(&InsightsProducer{}).zeroedEnvelope(30) ensuring zeroed["data"] is the expected
map type; then proceed with the key comparisons.
- Around line 31-53: The test verifying keys in the data map is missing
assertions for the keys added by zeroedEnvelope; update the test in
producer_insights_test.go (the block that checks entries in the data map) to
also assert presence of "recent_msgs_per_day", "recent_messages", and
"recent_days_active" (use the same pattern as the existing checks that fetch
from data and call assert.True) so the test fully covers zeroedEnvelope's
expected keys.
In `@internal/feeds/producer_news_test.go`:
- Around line 27-33: Add an assertion that verifies the "score" field exists and
is a numeric type: after the existing checks on stories[0] -> story and the
title/url assertions, assert that story["score"] is a number (decode into
float64 like _, ok := story["score"].(float64)) and use assert.True(t, ok,
"story.score must be a number") so a dropped/renamed score will fail; reference
the existing variables stories and story in the same test.
In `@internal/feeds/producer_project_test.go`:
- Around line 95-97: The test currently only checks for no error and a valid
envelope (require.NoError(t, err) and assertValidEnvelope(t, result)) but
doesn't validate that the payload is the cancelled-context fallback; update the
test after assertValidEnvelope to extract the payload from result (the variable
named result returned by Produce) and assert the same empty-field/fallback
checks used in the non-git case (reuse those assertions) so the payload contains
the fallback snapshot (empty repo fields) rather than a normal repo snapshot;
ensure you reference the Produce result variable and the assertValidEnvelope
location when adding these payload assertions.
In `@internal/feeds/producer_weather_test.go`:
- Around line 256-273: The test currently does an unchecked type assertion when
extracting fallback["data"] in TestWeatherProducer_TestFixture_FieldConsistency;
change it to a checked assertion: retrieve the value with v, ok :=
fallback["data"].(map[string]interface{}), assert.True(t, ok, "fallback data is
not map[string]interface{}") (or use assert.IsType) and then assign fallbackData
= v before iterating; this prevents a panic from the direct type assertion in
the WeatherProducer.fallbackResult check and keeps the existing key comparison
logic intact.
In `@internal/notifications/notifications.go`:
- Around line 109-120: The synchronous update loop in Unsurfaced is causing read
delays and lacks per-goroutine panic protection; change the expired-row handling
so each expired notification update runs in its own non-blocking goroutine: for
each n in all where n.Expired, spawn a goroutine that defers a recover()
handler, uses a non-cancelable context (e.g., context.Background()) to call
ns.db.Exec(ctx, `UPDATE notifications SET surfaced_at = ? WHERE id = ?`, now,
n.ID), and logs any Exec error (but does not block return); keep the main
Unsurfaced flow returning immediately without waiting for these goroutines.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d3d5ec5b-67d2-4310-8818-bb950312fa1b
📒 Files selected for processing (22)
internal/analytics/dolt.gointernal/analytics/state.gointernal/analytics/state_test.gointernal/core/analytics_types.gointernal/core/config_types.gointernal/core/dispatch_types.gointernal/core/feed_types.gointernal/core/guard_types.gointernal/core/handler_types.gointernal/core/types.gointernal/feeds/producer_calendar.gointernal/feeds/producer_calendar_test.gointernal/feeds/producer_helpers_test.gointernal/feeds/producer_insights.gointernal/feeds/producer_insights_test.gointernal/feeds/producer_memories_test.gointernal/feeds/producer_news_test.gointernal/feeds/producer_project.gointernal/feeds/producer_project_test.gointernal/feeds/producer_weather_test.gointernal/notifications/notifications.gointernal/notifications/producers.go
💤 Files with no reviewable changes (1)
- internal/analytics/state.go
Comment on lines
+341
to
+355
| // GuardBlockSummaries queries for tools that appear frequently in PreToolUse | ||
| // events on the given day, returning those with 5 or more occurrences. | ||
| // | ||
| // The today parameter should be a date prefix like "2006-01-02". | ||
| func (d *DB) GuardBlockSummaries(ctx context.Context, today string) ([]GuardBlockSummary, error) { | ||
| rows, err := d.db.QueryContext(ctx, | ||
| `SELECT tool_name, COUNT(*) AS cnt | ||
| FROM events | ||
| WHERE event_type = 'PreToolUse' | ||
| AND timestamp LIKE ? | ||
| AND tool_name != '' | ||
| GROUP BY tool_name | ||
| HAVING cnt >= 5 | ||
| ORDER BY cnt DESC`, | ||
| today+"%", |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Keep the guard-trigger threshold in one place.
GuardBlockSummaries now bakes in HAVING cnt >= 5, but internal/notifications/producers.go, Lines 83-86, still applies the same cutoff again. That split rule will drift the next time the threshold changes. Either return raw per-tool counts here or accept minCount as an argument and let the caller own the rule.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/analytics/dolt.go` around lines 341 - 355, GuardBlockSummaries
currently hardcodes HAVING cnt >= 5; change its signature to accept a minCount
int (func (d *DB) GuardBlockSummaries(ctx context.Context, today string,
minCount int) ([]GuardBlockSummary, error)), update the SQL to use HAVING cnt >=
? and pass minCount as the parameter (instead of hardcoded 5), and then remove
the duplicate cutoff logic from the caller in the notifications producer so the
caller supplies the threshold rather than re-filtering results.
Comment on lines
+104
to
+108
| type TranscriptConfig struct { | ||
| Enabled bool `yaml:"enabled" json:"enabled"` | ||
| BackupDir string `yaml:"backup_dir" json:"backupDir"` | ||
| MaxSizeMB int `yaml:"max_size_mb" json:"maxSizeMb"` | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Inconsistent casing in JSON tag for MaxSizeMB.
The field name uses MB (megabytes abbreviation) but the JSON tag uses maxSizeMb. For consistency with the field name and standard abbreviation conventions, consider using maxSizeMB.
✏️ Suggested fix
type TranscriptConfig struct {
Enabled bool `yaml:"enabled" json:"enabled"`
BackupDir string `yaml:"backup_dir" json:"backupDir"`
- MaxSizeMB int `yaml:"max_size_mb" json:"maxSizeMb"`
+ MaxSizeMB int `yaml:"max_size_mb" json:"maxSizeMB"`
}📝 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
| type TranscriptConfig struct { | |
| Enabled bool `yaml:"enabled" json:"enabled"` | |
| BackupDir string `yaml:"backup_dir" json:"backupDir"` | |
| MaxSizeMB int `yaml:"max_size_mb" json:"maxSizeMb"` | |
| } | |
| type TranscriptConfig struct { | |
| Enabled bool `yaml:"enabled" json:"enabled"` | |
| BackupDir string `yaml:"backup_dir" json:"backupDir"` | |
| MaxSizeMB int `yaml:"max_size_mb" json:"maxSizeMB"` | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/core/config_types.go` around lines 104 - 108, The JSON tag for
TranscriptConfig.MaxSizeMB uses inconsistent casing ("maxSizeMb"); update the
struct tag for the MaxSizeMB field in type TranscriptConfig to "maxSizeMB" so
the JSON key matches the field's MB abbreviation and project conventions—i.e.,
change the `json:"maxSizeMb"` on MaxSizeMB to `json:"maxSizeMB"`.
| SessionID string `json:"session_id"` | ||
| ToolName string `json:"tool_name,omitempty"` | ||
| ToolInput map[string]interface{} `json:"tool_input,omitempty"` | ||
| Extra map[string]interface{} `json:"-"` // captures unknown fields |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Find HookPayload definition + any custom UnmarshalJSON implementation"
rg -n -C3 --type=go 'type\s+HookPayload\s+struct|func\s*\(\s*\*?HookPayload\s*\)\s*UnmarshalJSON\s*\('
echo
echo "2) Find where HookPayload.Extra is explicitly populated"
rg -n -C3 --type=go '\.Extra\s*=|Extra:\s|HookPayload'Repository: vishnujayvel/hookwise
Length of output: 36763
🏁 Script executed:
cat -n internal/core/dispatch_types.goRepository: vishnujayvel/hookwise
Length of output: 1394
🏁 Script executed:
rg -n 'UnmarshalJSON|UnmarshalYAML' internal/core/dispatch_types.goRepository: vishnujayvel/hookwise
Length of output: 47
🏁 Script executed:
rg -n 'payload\.Extra|\.Extra\s*\[|Extra:' --type=goRepository: vishnujayvel/hookwise
Length of output: 225
🏁 Script executed:
# Search more broadly for UnmarshalJSON implementation anywhere in the codebase
rg -n 'UnmarshalJSON' --type=goRepository: vishnujayvel/hookwise
Length of output: 47
🏁 Script executed:
# Get context around the Extra field usage in dispatcher.go
sed -n '300,320p' internal/core/dispatcher.goRepository: vishnujayvel/hookwise
Length of output: 795
Remove json:"-" tag or implement custom UnmarshalJSON to actually capture unknown fields.
The Extra field declared with json:"-" will never populate from standard JSON decoding—the tag explicitly prevents unmarshaling. Yet payloadToMap at line 316 iterates over payload.Extra, expecting it to contain unknown fields. Either change the tag to allow unmarshaling (e.g., json:"-" → json:",inline" with appropriate custom logic), or implement UnmarshalJSON on HookPayload to populate Extra with fields that don't match known struct members. As written, Extra remains empty and the loop that references it has no effect.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/core/dispatch_types.go` at line 8, The Extra field on HookPayload is
tagged `json:"-"` so it never receives unknown JSON properties; either remove
that tag (e.g., make it `Extra map[string]interface{}` or use a marshaling tag
that allows decoding) or implement a custom UnmarshalJSON on the HookPayload
type that decodes into a temp map, extracts known fields into the struct and
stores any leftover keys into Extra; update payloadToMap to rely on the
populated Extra. Locate the Extra field declaration and the HookPayload type,
then implement one of these fixes so payloadToMap sees the unknown fields as
intended.
Comment on lines
+27
to
+33
| // Each story should have title, url, score. | ||
| story, ok := stories[0].(map[string]interface{}) | ||
| require.True(t, ok, "each story must be a map") | ||
| _, ok = story["title"].(string) | ||
| assert.True(t, ok, "story.title must be a string") | ||
| _, ok = story["url"].(string) | ||
| assert.True(t, ok, "story.url must be a string") |
There was a problem hiding this comment.
Assert story.score too.
The comment says each story should have title, url, and score, but this test never checks score. A regression that drops or renames that field still passes here.
✅ Minimal test fix
_, ok = story["url"].(string)
assert.True(t, ok, "story.url must be a string")
+ _, hasScore := story["score"]
+ assert.True(t, hasScore, "story.score must be present")📝 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
| // Each story should have title, url, score. | |
| story, ok := stories[0].(map[string]interface{}) | |
| require.True(t, ok, "each story must be a map") | |
| _, ok = story["title"].(string) | |
| assert.True(t, ok, "story.title must be a string") | |
| _, ok = story["url"].(string) | |
| assert.True(t, ok, "story.url must be a string") | |
| // Each story should have title, url, score. | |
| story, ok := stories[0].(map[string]interface{}) | |
| require.True(t, ok, "each story must be a map") | |
| _, ok = story["title"].(string) | |
| assert.True(t, ok, "story.title must be a string") | |
| _, ok = story["url"].(string) | |
| assert.True(t, ok, "story.url must be a string") | |
| _, hasScore := story["score"] | |
| assert.True(t, hasScore, "story.score must be present") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/feeds/producer_news_test.go` around lines 27 - 33, Add an assertion
that verifies the "score" field exists and is a numeric type: after the existing
checks on stories[0] -> story and the title/url assertions, assert that
story["score"] is a number (decode into float64 like _, ok :=
story["score"].(float64)) and use assert.True(t, ok, "story.score must be a
number") so a dropped/renamed score will fail; reference the existing variables
stories and story in the same test.
Comment on lines
+95
to
+97
| // ARCH-1: cancelled context should fail-open, returning fallback. | ||
| require.NoError(t, err, "ARCH-1: cancelled context must not produce error") | ||
| assertValidEnvelope(t, result, "project") |
There was a problem hiding this comment.
Validate the cancelled-context fallback payload, not just the envelope.
These assertions only prove the shape is valid. If Produce ignores ctx.Done() and returns the normal repo snapshot, this test still passes. Reuse the empty-field checks from the non-git case so the cancellation path actually guards the fallback behavior.
✅ Minimal test fix
// ARCH-1: cancelled context should fail-open, returning fallback.
require.NoError(t, err, "ARCH-1: cancelled context must not produce error")
- assertValidEnvelope(t, result, "project")
+ data := assertValidEnvelope(t, result, "project")
+ assert.Equal(t, "", data["name"])
+ assert.Equal(t, "", data["branch"])
+ assert.Equal(t, "", data["last_commit"])
+ assert.Equal(t, false, data["dirty"])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/feeds/producer_project_test.go` around lines 95 - 97, The test
currently only checks for no error and a valid envelope (require.NoError(t, err)
and assertValidEnvelope(t, result)) but doesn't validate that the payload is the
cancelled-context fallback; update the test after assertValidEnvelope to extract
the payload from result (the variable named result returned by Produce) and
assert the same empty-field/fallback checks used in the non-git case (reuse
those assertions) so the payload contains the fallback snapshot (empty repo
fields) rather than a normal repo snapshot; ensure you reference the Produce
result variable and the assertValidEnvelope location when adding these payload
assertions.
Comment on lines
+109
to
+120
| // Mark expired ones as surfaced so they are not rescanned on future calls. | ||
| var active []Notification | ||
| now := time.Now().UTC().Format(time.RFC3339) | ||
| for _, n := range all { | ||
| if !n.Expired { | ||
| if n.Expired { | ||
| // Best-effort: mark expired notification as surfaced so it | ||
| // won't be returned by future Unsurfaced() calls. | ||
| _, _ = ns.db.Exec(ctx, | ||
| `UPDATE notifications SET surfaced_at = ? WHERE id = ?`, | ||
| now, n.ID, | ||
| ) | ||
| } else { |
There was a problem hiding this comment.
Make expired-row surfacing non-blocking and recover-safe.
This side effect is currently synchronous in Unsurfaced, so many expired rows can delay reads. It also misses the required per-goroutine recover() guard for side-effect work.
⚙️ Suggested refactor (non-blocking side effect with recover)
- var active []Notification
- now := time.Now().UTC().Format(time.RFC3339)
+ var active []Notification
+ var expiredIDs []int
for _, n := range all {
if n.Expired {
- // Best-effort: mark expired notification as surfaced so it
- // won't be returned by future Unsurfaced() calls.
- _, _ = ns.db.Exec(ctx,
- `UPDATE notifications SET surfaced_at = ? WHERE id = ?`,
- now, n.ID,
- )
+ expiredIDs = append(expiredIDs, n.ID)
} else {
active = append(active, n)
}
}
+
+ if len(expiredIDs) > 0 {
+ now := time.Now().UTC().Format(time.RFC3339)
+ go func(ids []int, surfacedAt string) {
+ defer func() {
+ if r := recover(); r != nil {
+ core.Logger().Error("notifications: async expire-surface panic recovered", "recovered", r)
+ }
+ }()
+ bg, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+ defer cancel()
+ for _, id := range ids {
+ if _, err := ns.db.Exec(bg, `UPDATE notifications SET surfaced_at = ? WHERE id = ?`, surfacedAt, id); err != nil {
+ core.Logger().Debug("notifications: async expire-surface failed", "id", id, "error", err)
+ }
+ }
+ }(append([]int(nil), expiredIDs...), now)
+ }
return active, nilAs per coding guidelines: internal/**/*.go: Implement side effects as non-blocking with per-goroutine recover() for error handling (ARCH-7: Side effects non-blocking).
📝 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
| // Mark expired ones as surfaced so they are not rescanned on future calls. | |
| var active []Notification | |
| now := time.Now().UTC().Format(time.RFC3339) | |
| for _, n := range all { | |
| if !n.Expired { | |
| if n.Expired { | |
| // Best-effort: mark expired notification as surfaced so it | |
| // won't be returned by future Unsurfaced() calls. | |
| _, _ = ns.db.Exec(ctx, | |
| `UPDATE notifications SET surfaced_at = ? WHERE id = ?`, | |
| now, n.ID, | |
| ) | |
| } else { | |
| // Mark expired ones as surfaced so they are not rescanned on future calls. | |
| var active []Notification | |
| var expiredIDs []int | |
| for _, n := range all { | |
| if n.Expired { | |
| expiredIDs = append(expiredIDs, n.ID) | |
| } else { | |
| active = append(active, n) | |
| } | |
| } | |
| if len(expiredIDs) > 0 { | |
| now := time.Now().UTC().Format(time.RFC3339) | |
| go func(ids []int, surfacedAt string) { | |
| defer func() { | |
| if r := recover(); r != nil { | |
| core.Logger().Error("notifications: async expire-surface panic recovered", "recovered", r) | |
| } | |
| }() | |
| bg, cancel := context.WithTimeout(context.Background(), 5*time.Second) | |
| defer cancel() | |
| for _, id := range ids { | |
| if _, err := ns.db.Exec(bg, `UPDATE notifications SET surfaced_at = ? WHERE id = ?`, surfacedAt, id); err != nil { | |
| core.Logger().Debug("notifications: async expire-surface failed", "id", id, "error", err) | |
| } | |
| } | |
| }(append([]int(nil), expiredIDs...), now) | |
| } | |
| return active, nil |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/notifications/notifications.go` around lines 109 - 120, The
synchronous update loop in Unsurfaced is causing read delays and lacks
per-goroutine panic protection; change the expired-row handling so each expired
notification update runs in its own non-blocking goroutine: for each n in all
where n.Expired, spawn a goroutine that defers a recover() handler, uses a
non-cancelable context (e.g., context.Background()) to call ns.db.Exec(ctx,
`UPDATE notifications SET surfaced_at = ? WHERE id = ?`, now, n.ID), and logs
any Exec error (but does not block return); keep the main Unsurfaced flow
returning immediately without waiting for these goroutines.
- Add checked type assertions with require.True in test files - Add missing score assertion in news producer test - Add missing zeroedEnvelope key assertions in insights test Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
internal/feeds/producer_news_test.go (1)
34-35:⚠️ Potential issue | 🟡 MinorValidate
story.scoretype, not just key presence.Line 34–35 allows non-numeric values (e.g.
"score": "high") to pass. Please assert numeric type to prevent schema regressions.🔧 Minimal fix
- _, ok = story["score"] - assert.True(t, ok, "story should have score") + switch story["score"].(type) { + case int, int32, int64, float32, float64: + assert.True(t, true, "story.score must be numeric") + default: + assert.Fail(t, "story.score must be numeric") + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/feeds/producer_news_test.go` around lines 34 - 35, The test currently only checks presence of the "score" key (variable story) which allows non-numeric values; update the assertion to validate the value's numeric type (e.g., assert it is a float64 or int) by type-asserting story["score"] and using testify assertions (or a type switch) to ensure the value is numeric (for example assert.IsType(t, float64(0), val) or assert.IsType(t, int(0), val)); modify the assertions around story["score"] in the test to fail if the value is not a numeric type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/feeds/producer_calendar_test.go`:
- Around line 85-97: The test currently only verifies fail-open behavior but not
that an empty Calendar Feed TokenPath actually resolves to
DefaultCalendarTokenPath; add a focused unit test for the path-resolution logic
(or call the existing resolver) to assert that passing an empty TokenPath yields
DefaultCalendarTokenPath. Specifically, either (a) call the resolver function
(e.g., ResolveCalendarTokenPath or CalendarProducer.resolveTokenPath) with ""
and assert the returned path equals DefaultCalendarTokenPath, or (b) if no
resolver exists, extract the TokenPath resolution logic from Produce into a
small exported/internal helper (e.g., ResolveCalendarTokenPath) and unit-test
that helper to return DefaultCalendarTokenPath for an empty input while leaving
TestCalendarProducer_DefaultTokenPath to continue asserting fail-open behavior.
In `@internal/feeds/producer_insights_test.go`:
- Around line 136-150: The test TestInsightsProducer_CancelledContext_FailOpen
currently points InsightsFeedConfig.UsageDataPath at a nonexistent subdirectory
so the missing-dir fast-fail path masks cancellation behavior; update the test
to use an existing fixture directory (e.g., the t.TempDir() root or a known
test-fixture folder) for UsageDataPath and, if the producer expects files, seed
the minimal usage data files into that directory before calling p.Produce(ctx)
so the code path must observe ctx.Err() and thus exercises the cancellation
branch of InsightsProducer.
In `@internal/feeds/producer_news_test.go`:
- Around line 65-69: Replace the ad-hoc envelope checks on result with the
shared helper: remove the map type assertion, require/assert lines and call
assertValidEnvelope(t, result, "news") instead so this test uses the canonical
envelope validation (including timestamp format and required keys) provided by
the assertValidEnvelope helper.
In `@internal/feeds/producer_weather_test.go`:
- Around line 210-254: The test TestWeatherProducer_FallbackUsesCachedResult
increments callCount in the test server but never asserts it, so it doesn't
prove the second Produce actually hit the server and triggered the 503 fallback;
after the second p.Produce call (and after validating result2), add an assertion
that callCount == 2 to ensure the second request was attempted and the fallback
path in WeatherProducer was exercised (refer to callCount, the httptest server
handler, and Produce on WeatherProducer to locate where to insert the
postcondition).
---
Duplicate comments:
In `@internal/feeds/producer_news_test.go`:
- Around line 34-35: The test currently only checks presence of the "score" key
(variable story) which allows non-numeric values; update the assertion to
validate the value's numeric type (e.g., assert it is a float64 or int) by
type-asserting story["score"] and using testify assertions (or a type switch) to
ensure the value is numeric (for example assert.IsType(t, float64(0), val) or
assert.IsType(t, int(0), val)); modify the assertions around story["score"] in
the test to fail if the value is not a numeric type.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e7a3fd19-52b7-476f-83e4-a9d18926e57c
📒 Files selected for processing (4)
internal/feeds/producer_calendar_test.gointernal/feeds/producer_insights_test.gointernal/feeds/producer_news_test.gointernal/feeds/producer_weather_test.go
Comment on lines
+85
to
+97
| func TestCalendarProducer_DefaultTokenPath(t *testing.T) { | ||
| // When TokenPath is empty, the producer should use the default path | ||
| // and still fail-open if that path doesn't exist. | ||
| p := &CalendarProducer{} | ||
| p.SetFeedsConfig(core.FeedsConfig{ | ||
| Calendar: core.CalendarFeedConfig{ | ||
| TokenPath: "", // empty → uses DefaultCalendarTokenPath | ||
| }, | ||
| }) | ||
|
|
||
| result, err := p.Produce(context.Background()) | ||
| require.NoError(t, err, "ARCH-1: default token path must fail-open if file missing") | ||
| assertValidEnvelope(t, result, "calendar") |
There was a problem hiding this comment.
This test doesn't prove TokenPath defaulting is used.
Lines 91-97 only show that an empty path still fails open. It would also pass if the producer skipped DefaultCalendarTokenPath entirely and returned fallback immediately. Make the test observe the resolved default path, or unit-test the path-resolution helper directly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/feeds/producer_calendar_test.go` around lines 85 - 97, The test
currently only verifies fail-open behavior but not that an empty Calendar Feed
TokenPath actually resolves to DefaultCalendarTokenPath; add a focused unit test
for the path-resolution logic (or call the existing resolver) to assert that
passing an empty TokenPath yields DefaultCalendarTokenPath. Specifically, either
(a) call the resolver function (e.g., ResolveCalendarTokenPath or
CalendarProducer.resolveTokenPath) with "" and assert the returned path equals
DefaultCalendarTokenPath, or (b) if no resolver exists, extract the TokenPath
resolution logic from Produce into a small exported/internal helper (e.g.,
ResolveCalendarTokenPath) and unit-test that helper to return
DefaultCalendarTokenPath for an empty input while leaving
TestCalendarProducer_DefaultTokenPath to continue asserting fail-open behavior.
Comment on lines
+136
to
+150
| func TestInsightsProducer_CancelledContext_FailOpen(t *testing.T) { | ||
| ctx, cancel := context.WithCancel(context.Background()) | ||
| cancel() // cancel immediately | ||
|
|
||
| p := &InsightsProducer{} | ||
| p.SetFeedsConfig(core.FeedsConfig{ | ||
| Insights: core.InsightsFeedConfig{ | ||
| UsageDataPath: filepath.Join(t.TempDir(), "data"), | ||
| StalenessDays: 30, | ||
| }, | ||
| }) | ||
|
|
||
| result, err := p.Produce(ctx) | ||
| require.NoError(t, err, "ARCH-1: cancelled context must fail-open") | ||
| assertValidEnvelope(t, result, "insights") |
There was a problem hiding this comment.
This cancelled-context test can pass without ever exercising cancellation.
Line 143 points UsageDataPath at a missing subdirectory, so the missing-dir fail-open path can satisfy the assertions even if the ctx.Err() handling regresses. Use an existing fixture directory here (and seed the minimal usage data if needed) so this test is specific to the cancellation branch.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/feeds/producer_insights_test.go` around lines 136 - 150, The test
TestInsightsProducer_CancelledContext_FailOpen currently points
InsightsFeedConfig.UsageDataPath at a nonexistent subdirectory so the
missing-dir fast-fail path masks cancellation behavior; update the test to use
an existing fixture directory (e.g., the t.TempDir() root or a known
test-fixture folder) for UsageDataPath and, if the producer expects files, seed
the minimal usage data files into that directory before calling p.Produce(ctx)
so the code path must observe ctx.Err() and thus exercises the cancellation
branch of InsightsProducer.
Comment on lines
+65
to
+69
| m, ok := result.(map[string]interface{}) | ||
| require.True(t, ok) | ||
| assert.Equal(t, "news", m["type"]) | ||
| assert.Len(t, m, 3, "envelope must have exactly 3 keys") | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Prefer assertValidEnvelope here to avoid contract drift.
Line 65–69 duplicates only part of the envelope contract checks; using the shared helper keeps this test aligned with canonical validation (including timestamp format and required keys).
♻️ Refactor suggestion
- m, ok := result.(map[string]interface{})
- require.True(t, ok)
- assert.Equal(t, "news", m["type"])
- assert.Len(t, m, 3, "envelope must have exactly 3 keys")
+ assertValidEnvelope(t, result, "news")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/feeds/producer_news_test.go` around lines 65 - 69, Replace the
ad-hoc envelope checks on result with the shared helper: remove the map type
assertion, require/assert lines and call assertValidEnvelope(t, result, "news")
instead so this test uses the canonical envelope validation (including timestamp
format and required keys) provided by the assertValidEnvelope helper.
Comment on lines
+210
to
+254
| func TestWeatherProducer_FallbackUsesCachedResult(t *testing.T) { | ||
| callCount := 0 | ||
|
|
||
| srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| callCount++ | ||
| if callCount == 1 { | ||
| // First call succeeds. | ||
| resp := map[string]interface{}{ | ||
| "current": map[string]interface{}{ | ||
| "temperature_2m": 68.0, | ||
| "wind_speed_10m": 4.0, | ||
| "weather_code": 0, | ||
| }, | ||
| } | ||
| w.Header().Set("Content-Type", "application/json") | ||
| json.NewEncoder(w).Encode(resp) | ||
| } else { | ||
| // Second call fails. | ||
| http.Error(w, "Service Unavailable", http.StatusServiceUnavailable) | ||
| } | ||
| })) | ||
| defer srv.Close() | ||
|
|
||
| p := &WeatherProducer{ | ||
| client: &http.Client{ | ||
| Transport: roundTripFunc(func(req *http.Request) (*http.Response, error) { | ||
| req.URL.Scheme = "http" | ||
| req.URL.Host = srv.Listener.Addr().String() | ||
| return http.DefaultTransport.RoundTrip(req) | ||
| }), | ||
| }, | ||
| } | ||
|
|
||
| // First call: should succeed and cache the result. | ||
| result1, err := p.Produce(context.Background()) | ||
| require.NoError(t, err) | ||
| data1 := assertValidEnvelope(t, result1, "weather") | ||
| assert.Equal(t, 68.0, data1["temperature"]) | ||
|
|
||
| // Second call: should fail-open and return cached result. | ||
| result2, err := p.Produce(context.Background()) | ||
| require.NoError(t, err) | ||
| data2 := assertValidEnvelope(t, result2, "weather") | ||
| assert.Equal(t, 68.0, data2["temperature"], "fallback should return cached temperature") | ||
| } |
There was a problem hiding this comment.
This test doesn't prove the 503 fallback path ran.
Line 214 increments callCount, but the test never checks that the second Produce actually hit the server. If cached data is returned without attempting a refresh, this still passes. Add a postcondition on callCount so the test really covers “refresh fails, then cached fallback”.
🧪 Tighten the assertion
// Second call: should fail-open and return cached result.
result2, err := p.Produce(context.Background())
require.NoError(t, err)
data2 := assertValidEnvelope(t, result2, "weather")
+ assert.GreaterOrEqual(t, callCount, 2, "expected a refresh attempt before falling back to cache")
assert.Equal(t, 68.0, data2["temperature"], "fallback should return cached temperature")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/feeds/producer_weather_test.go` around lines 210 - 254, The test
TestWeatherProducer_FallbackUsesCachedResult increments callCount in the test
server but never asserts it, so it doesn't prove the second Produce actually hit
the server and triggered the 503 fallback; after the second p.Produce call (and
after validating result2), add an assertion that callCount == 2 to ensure the
second request was attempted and the fallback path in WeatherProducer was
exercised (refer to callCount, the httptest server handler, and Produce on
WeatherProducer to locate where to insert the postcondition).
Owner
Author
|
Stale-PR triage (automated, from the dev loop): this PR has been untouched since 2026-03-20 (~3 months) and predates several major main-branch changes — the Dolt→SQLite migration (#74), the doctor hook-safety suite (#75), the real feed producers (#76), and the matcher-aware/analytics fixes (#77/#78). Its CI run is from before the migration, and Recommendation: either (a) rebase onto current main and re-evaluate whether batches G–K still add value not already covered by the post-Dolt architecture, or (b) close it and cherry-pick anything still wanted into a fresh PR. Flagging for a human decision rather than auto-closing, since the G–K batches may contain unmerged work. cc @vishnujayvel |
vishnujayvel
added a commit
that referenced
this pull request
Jun 17, 2026
writeBackToken persisted the refreshed Google OAuth token with a non-atomic os.WriteFile. A crash or kill mid-write truncates the credential file, after which the Python calendar-feed.py can no longer load it and the user must perform a full re-auth. Switches to core.AtomicWriteJSON (temp file + rename), which writes the same 0600 permissions and the same indented JSON, so a partial write can never replace a good token file. writeBackToken had no test coverage. Adds TestWriteBackToken_AtomicAndSecure pinning: 0600 permissions (a security-regression guard for a credential file), the Python google-auth field format, and that no temp/partial files are left behind. Mutation-proven — reverting to os.WriteFile(..., 0644) fails the 0600 assertion. Extracted from the stale, conflicting PR #73 (architecture-v2-part2); this is one of its few genuinely-unmerged, non-obsolete fixes. Guarded gate green (internal/feeds, -race -p 2); vet clean. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Owner
Author
|
Closing as superseded/stale. Per the #73-salvage triage (2026-06-17): every still-relevant batch from this arch-v2 branch has either already landed on main or been obsoleted. The Dolt-era pieces predate the Dolt->SQLite migration (#74); the producer tests it added already exist on main; the genuinely valuable extractions were cherry-picked and shipped separately as #183 (atomic OAuth write-back), #184 (dead WriteFeedCacheJSON removal), and #185 (InsightsProducer coverage). What remains here is churn (e.g. the types.go 7-file split) with merge conflicts against current main. Re-open or re-branch from current HEAD if any specific batch is still wanted. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Completes the architecture-v2 spec (all 14 tasks now done) plus addresses 5 CodeRabbit findings from PR #72.
types.gointo 7 domain-focused files (50 types, 0 export changes)ParseTimeFlex, add git timeout + ctx honoring + atomic token writesWriteFeedCacheJSON, narrow ALTER TABLE error handling, fix expired notification rescanCodeRabbit Findings Addressed
Test plan
go vet ./...passesGOMEMLIMIT=4GiB go test -race -p 2 ./...— all 13 packages pass🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Improvements