fix: connect status-line segments to feed cache + real CalendarProducer#62
Merged
Conversation
Status-line segments were rendering hardcoded placeholder text instead of reading from the daemon's feed cache. The data pipeline (daemon → JSON cache → bridge) was fully built but never connected to the renderer. - Load feed cache via bridge.CollectFeedCache() in runStatusLine() - Add per-segment renderers: weather, project, calendar, pulse - Graceful fallback to "name: --" on missing/malformed/placeholder data - Filter placeholder feeds via source: "placeholder" detection - 7 new tests: cache integration, missing cache, individual segments, zero-temperature edge case, celsius unit, placeholder fallback Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Allows hookwise.yaml to use plain strings (e.g., "- session") in the segments list instead of requiring the full struct syntax. Without this, config loading fails with "cannot unmarshal !!str into core.SegmentConfig". Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Address CodeRabbit review: load DailySummaryResult from Dolt in runStatusLine and thread it through renderBuiltinSegment so session shows today's count and cost shows estimated spend instead of hardcoded placeholders. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add recordAnalytics() to write session/event data to Dolt on dispatch (fixes session:-- and cost:-- segments showing placeholder values) - Fix UTC/local timezone mismatch in status-line and stats date queries - Add weather to status_line.segments in hookwise.yaml - Add TestPersistAcrossConnections for Dolt cross-connection persistence - Add TestRecordAnalytics_SessionStart and _PostToolUse pipeline tests - Write retrospective on Mock Confidence Trap pattern (3rd occurrence) Fixes #50 (status-line feed cache disconnection) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace the 14-line CalendarProducer stub (which returned "source": "placeholder"
and was filtered out by feedData()) with a real implementation that:
- Reads Python google-auth token format from ~/.hookwise/calendar-token.json
- Constructs oauth2.Config + oauth2.Token with auto-refresh
- Fetches events via Google Calendar API with configurable lookahead
- Handles all-day events (date vs dateTime) and computes is_current
- Generates human-friendly relative times ("in 15m", "tomorrow 3pm")
- Writes refreshed tokens back in Python-compatible format
- Falls back to cached result on API errors (ARCH-1 fail-open)
- Supports baseURL override for httptest mocking in tests
Also includes InsightsProducer, WeatherProducer live implementations,
CLI rendering tests, and E2E calendar test scaffolding.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Addresses code review findings from simplify pass: - Cache *http.Client, *oauth2.Config, and *calendar.Service in struct fields, initialized once via ensureClient() (was re-reading token file and creating service on every 300s poll cycle) - Use Calendars[0] from config instead of hardcoded "primary" - Extract defaultCalendarLookaheadMinutes constant - Use time.Now().UTC() consistently for timestamp Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughIntegrated feed-cache and daily analytics into status-line rendering, added Calendar/Insights/Project feed producers with fail-open behavior and OAuth support, introduced background analytics recording to Dolt, expanded tests (unit, integration, e2e), and updated renderSegment/renderBuiltinSegment signatures and related helpers. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI Dispatch
participant Dispatch as core.Dispatch
participant Analytics as Analytics Recorder
participant AnalDB as Analytics DB (Dolt)
participant StatusLine as Status-Line Renderer
participant FeedCache as Feed Cache (Bridge)
participant Producers as Feed Producers
CLI->>Dispatch: Dispatch command
Dispatch-->>CLI: Return result
Note over CLI,Analytics: Background goroutine for analytics (non-blocking)
CLI->>Analytics: recordAnalytics(event, payload) [if SessionID]
Analytics->>AnalDB: Open/StartSession/RecordEvent/Commit
AnalDB-->>Analytics: Ack
CLI->>StatusLine: runStatusLine()
StatusLine->>FeedCache: Load feed cache
StatusLine->>Producers: Request produce (Calendar, Insights, Project, ...)
Producers-->>FeedCache: Write feed data
StatusLine->>AnalDB: Query DailySummary (today UTC)
AnalDB-->>StatusLine: Summary result
StatusLine->>StatusLine: renderSegment(name, feedCache, summary)
StatusLine-->>CLI: Rendered status line (+insights lines)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
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 unit tests (beta)
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 |
The relative time string ("in 15m") was computed when the daemon polled
the API and baked into the cache as a static string. This meant the
display stayed stale until the next poll cycle (up to 5 minutes).
Now the producer stores the absolute event start time in next_event.start,
and renderCalendarSegment() computes the relative time dynamically from
time.Now() on every render. The display is always accurate regardless of
cache age.
Backward compatible: renderer falls back to static "time" field if
"start" is absent.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/hookwise/cli_test.go`:
- Around line 887-937: The tests currently call recordAnalytics directly and
miss the goroutine+shutdown path exercised by the real CLI dispatch wrapper; add
a command-level test that runs the dispatch path created by newDispatchCmd
(invoke the dispatch command similar to how the CLI would, e.g., calling
newDispatchCmd and executing it) so the code path that does go
recordAnalytics(...) and triggers process shutdown is exercised; ensure the test
creates a temp analytics dir, invokes the command (or executes dispatch) with a
payload that causes a session start and a tool-use, waits for command
completion, then opens analytics.Open and asserts DailySummary shows the
expected TotalSessions/TotalEvents to catch dropped writes in the wrapper.
- Around line 421-1255: The tests in this file (e.g.,
TestStatusLineWithFeedCache, TestStatusLineWithMissingCache,
TestStatusLineWeatherSegment, TestRecordAnalytics_SessionStart,
TestFormatLargeNumber and other Test* functions) use stdlib
t.Fatal/t.Fatalf/t.Error/t.Errorf; replace these with testify assertions: use
require.NoError/require.Nil for errors (e.g., after executeCommand, os.Chdir,
analytics.Open), require.NoError/require.Zero/require.NotZero or require.Greater
for numeric expectations (e.g., summary.TotalSessions, summary.TotalEvents),
require.Empty/require.NotEmpty or require.Equal for expected strings, and
assert.Contains/assert.NotContains for substring checks of stripANSI output and
formatLargeNumber tests; also swap t.Helper usage to keep writeJSONFile but
change its test failures to require.NoError/require.FailNow as appropriate so
all test assertions use testify's require/assert APIs and imports
(require/assert) instead of t.*.
In `@cmd/hookwise/main.go`:
- Around line 575-704: The renderers are reintroducing placeholder output ("--")
even though feedData returns nil for missing/placeholder feeds; update
renderWeatherSegment, renderProjectSegment, renderCalendarSegment, and
renderPulseSegment to return an empty string ("") whenever feedData returns nil
or when the subsequent mandatory field checks (e.g., temp == nil in
renderWeatherSegment, name/branch empty handling in renderProjectSegment,
next_event nil/empty in renderCalendarSegment, missing session_count or
non-numeric in renderPulseSegment) would currently produce the synthesized "--"
fallback; ensure only non-placeholder, fully-renderable data produces the
colored segment strings so the top-level fallback can control placeholder
rendering.
- Around line 517-540: The switch in renderBuiltinSegment currently omits the
insights-related segment names so "insights", "insights_friction",
"insights_pace", and "insights_trend" fall through to the default; add explicit
cases for those names in renderBuiltinSegment and route them to the appropriate
renderer(s) using the feedCache and summary as needed (for example call a shared
renderInsightsSegment(feedCache, "friction"/"pace"/"trend") or separate helpers
like renderInsightsFrictionSegment(feedCache) etc.), ensuring each insights
variant returns cache-backed content instead of the ansiGray literal.
- Around line 407-415: The code opens analytics.Open("") which uses the default
Dolt DB path; change it to open the configured path by passing
config.Analytics.DBPath to analytics.Open so reads match dispatch writes. Update
the block that creates the analytics DB (the analytics.Open call), keeping the
existing error-handling/defer and passing config.Analytics.DBPath to
analytics.Open so NewAnalytics and DailySummary operate on the same DB used by
dispatch; ensure any nil/err handling around dailySummary remains unchanged.
- Around line 104-107: The analytics write is detached via an untracked
goroutine (go recordAnalytics(...)) so os.Exit or Dolt Open/Commit can terminate
it prematurely; change the dispatch to run recordAnalytics synchronously or
start it with a completion signal and wait with a bounded timeout (e.g., a
context with timeout or a sync.WaitGroup + select on timeout) before exiting so
analytics either completes or times out deterministically; update the call site
that currently uses eventType, payload, config.Analytics.DBPath to use the
flush/wait mechanism and ensure recordAnalytics honors a context/cancel and that
Dolt writes are serialized (apply SetMaxOpenConns(1) where Dolt DB is opened) to
avoid concurrent Dolt writes.
In `@docs/retro-status-line-bugs-50.md`:
- Around line 106-119: The example TestStatusLine_EndToEnd_DispatchThenRender is
a non-runnable sketch and must be explicitly marked as pseudocode; update the
code block annotation for the example (the triple-backtick fence around the Go
snippet) to indicate pseudocode (e.g., "```go (pseudocode)" or add a one-line
comment inside the block) and/or add a short sentence before the block stating
"Pseudocode — not runnable" so readers know this is illustrative only and not an
executable test.
- Around line 28-33: The markdown code fence containing the ASCII pipeline
diagram (the block starting with "Pipeline 1: Daemon → Feed JSON →
bridge.CollectFeedCache → renderSegment" and including Pipeline 2–4) needs a
language specifier to satisfy MD040 and ensure proper rendering; update the
opening triple-backtick to include a language token (e.g., ```text) so the
diagram block is fenced as ```text ... ``` instead of ``` ... ```.
- Around line 94-105: Add a blank line after each of the headings shown in the
diff so they comply with MD022: insert an empty line immediately after the lines
starting "### L1: \"Infrastructure-ready\" ≠ \"Wired\"", "### L2: Timezone bugs
hide in happy-path tests", "### L3: Config completeness is untestable without a
\"full pipeline\" smoke test", and "### L4: The Mock Confidence Trap is a
recurring pattern" so each heading is followed by one blank line before the
paragraph that follows.
In `@go.mod`:
- Line 36: The transitive advisories for filippo.io/edwards25519 (GO-2026-4503)
and github.com/aws/aws-sdk-go (GO-2022-0635, GO-2022-0646) are misclassified;
update vulnerability metadata and remediation: in go.mod bump
filippo.io/edwards25519 to v1.1.1+ to address GO-2026-4503, and for
github.com/aws/aws-sdk-go identify where the v1 module is pulled (search for
aws-sdk-go imports and any S3 Crypto usage) then either remove/eliminate S3
Crypto usage or plan a migration to AWS SDK Go V2 (no v1 patch exists) and mark
the aws-sdk-go advisory as HIGH/CRITICAL until migration is complete. Ensure
references to these advisories (GO-2026-4503, GO-2022-0635, GO-2022-0646) and
the modules (filippo.io/edwards25519, github.com/aws/aws-sdk-go) are updated in
your security tracking or SBOM entries.
In `@internal/feeds/builtin.go`:
- Around line 366-374: The current logic treats a malformed edt.Date as an
all-day event because on time.Parse error it returns (time.Time{}, true); change
the error branch so that when time.Parse("2006-01-02", edt.Date) fails you
return (time.Time{}, false) instead (i.e., do not set allDay on parse failure).
Update the error-handling branch around the time.Parse call that references
edt.Date so the function only returns true for allDay when parsing succeeds.
- Around line 260-271: The eventsCall.Do() can hang; wrap the API call in a
context with timeout: create a context.WithTimeout from the incoming ctx (e.g.
5–10s), defer cancel(), attach it to the call via
eventsCall.Context(ctxWithTimeout) before calling eventsCall.Do(), and keep the
existing error handling (return p.fallbackResult("api error"), nil) unchanged;
locate symbols eventsCall, p.svc.Events.List and eventsCall.Do to make the
change.
In `@internal/feeds/feeds_test.go`:
- Around line 754-758: The test is using a direct type assertion
data["top_tools"].([]map[string]interface{}) which can panic if JSON/unboxing
yields []interface{}; change the assertion in feeds_test.go to first assert the
value is present, then safely assert it as []interface{} (e.g., topToolsIface,
ok := data["top_tools"].([]interface{})) and use len(topToolsIface) for the
count check (or iterate and convert each element to map[string]interface{} if
you need to inspect elements) so the test no longer panics on different
underlying slice types.
In `@tui/tests/test_terminal_e2e.py`:
- Around line 359-361: ARTIFACT_DIR is built using a relative "../artifacts"
which can break when tests run from different working directories; change
ARTIFACT_DIR to an absolute path (e.g., wrap the existing join in
os.path.abspath) and ensure the directory exists by creating it if missing (use
os.makedirs(..., exist_ok=True)) before tests run so artifact capture never
fails; update references to ARTIFACT_DIR in tests accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c3c519de-88be-4c71-919c-814ad8e5d63b
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
cmd/hookwise/cli_test.gocmd/hookwise/main.godocs/retro-status-line-bugs-50.mdgo.modhookwise.yamlinternal/analytics/persist_test.gointernal/core/types.gointernal/feeds/builtin.gointernal/feeds/feeds_test.gotui/tests/test_terminal_e2e.py
Comment on lines
421
to
+1255
| func TestRenderBuiltinSegments(t *testing.T) { | ||
| names := []string{"session", "cost", "project", "calendar", "pulse", "weather"} | ||
| for _, name := range names { | ||
| result := renderBuiltinSegment(name) | ||
| if result == "" { | ||
| t.Errorf("builtin segment %q rendered empty", name) | ||
| } | ||
| // After stripping ANSI codes, segment name should appear. | ||
| stripped := stripANSI(result) | ||
| if !strings.Contains(stripped, name) && name != "cost" { | ||
| t.Errorf("builtin segment %q should contain its name, got: %s", name, stripped) | ||
| emptyCache := map[string]interface{}{} | ||
|
|
||
| // Segments with no data should return empty (omitted from output). | ||
| noDataSegments := []string{"session", "cost", "project", "calendar", "pulse", "weather"} | ||
| for _, name := range noDataSegments { | ||
| result := renderBuiltinSegment(name, emptyCache, nil) | ||
| if result != "" { | ||
| t.Errorf("builtin segment %q with no data should return empty, got: %s", name, stripANSI(result)) | ||
| } | ||
| } | ||
|
|
||
| // Unknown segment should still render something. | ||
| unknown := renderBuiltinSegment("unknown_segment") | ||
| // Unknown segment should still render something (fallback). | ||
| unknown := renderBuiltinSegment("unknown_segment", emptyCache, nil) | ||
| if unknown == "" { | ||
| t.Error("unknown builtin segment should still render") | ||
| } | ||
| } | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // 14. Status-line with feed cache renders real data | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| func TestStatusLineWithFeedCache(t *testing.T) { | ||
| tmpDir := t.TempDir() | ||
|
|
||
| origDir, err := os.Getwd() | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if err := os.Chdir(tmpDir); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| defer os.Chdir(origDir) | ||
|
|
||
| stateDir := filepath.Join(tmpDir, ".hookwise") | ||
| t.Setenv("HOOKWISE_STATE_DIR", stateDir) | ||
|
|
||
| // Create the feed cache directory (same as filepath.Dir(core.DefaultCachePath)). | ||
| cacheDir := filepath.Join(stateDir, "state") | ||
| os.MkdirAll(cacheDir, 0o700) | ||
|
|
||
| // Write weather feed cache with real data. | ||
| weatherEnvelope := map[string]interface{}{ | ||
| "type": "weather", | ||
| "timestamp": "2026-03-07T10:00:00Z", | ||
| "data": map[string]interface{}{ | ||
| "temperature": 72.0, | ||
| "temperatureUnit": "fahrenheit", | ||
| "windSpeed": 5.3, | ||
| "weatherCode": 0, | ||
| "emoji": "\u2600\ufe0f", | ||
| "description": "Clear", | ||
| }, | ||
| } | ||
| writeJSONFile(t, filepath.Join(cacheDir, "weather.json"), weatherEnvelope) | ||
|
|
||
| // Write project feed cache with real data. | ||
| projectEnvelope := map[string]interface{}{ | ||
| "type": "project", | ||
| "timestamp": "2026-03-07T10:00:00Z", | ||
| "data": map[string]interface{}{ | ||
| "name": "hookwise", | ||
| "branch": "main", | ||
| }, | ||
| } | ||
| writeJSONFile(t, filepath.Join(cacheDir, "project.json"), projectEnvelope) | ||
|
|
||
| configYAML := `version: 1 | ||
| status_line: | ||
| enabled: true | ||
| segments: | ||
| - builtin: weather | ||
| - builtin: project | ||
| ` | ||
| configPath := filepath.Join(tmpDir, core.ProjectConfigFile) | ||
| os.WriteFile(configPath, []byte(configYAML), 0o644) | ||
|
|
||
| output, err := executeCommand("status-line") | ||
| if err != nil { | ||
| t.Fatalf("status-line failed: %v\noutput: %s", err, output) | ||
| } | ||
|
|
||
| stripped := stripANSI(output) | ||
|
|
||
| // Weather should show temperature and description from cache. | ||
| if !strings.Contains(stripped, "72") { | ||
| t.Errorf("weather segment should show temperature 72, got: %s", stripped) | ||
| } | ||
| if !strings.Contains(stripped, "Clear") { | ||
| t.Errorf("weather segment should show description 'Clear', got: %s", stripped) | ||
| } | ||
|
|
||
| // Project should show name and branch from cache. | ||
| if !strings.Contains(stripped, "hookwise") { | ||
| t.Errorf("project segment should show project name, got: %s", stripped) | ||
| } | ||
| if !strings.Contains(stripped, "(main)") { | ||
| t.Errorf("project segment should show branch, got: %s", stripped) | ||
| } | ||
| } | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // 15. Status-line with missing cache renders fallbacks | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| func TestStatusLineWithMissingCache(t *testing.T) { | ||
| tmpDir := t.TempDir() | ||
|
|
||
| origDir, err := os.Getwd() | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if err := os.Chdir(tmpDir); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| defer os.Chdir(origDir) | ||
|
|
||
| // Point state dir to a nonexistent path so no cache is found. | ||
| stateDir := filepath.Join(tmpDir, ".hookwise-no-cache") | ||
| t.Setenv("HOOKWISE_STATE_DIR", stateDir) | ||
|
|
||
| configYAML := `version: 1 | ||
| status_line: | ||
| enabled: true | ||
| segments: | ||
| - builtin: weather | ||
| - builtin: project | ||
| - builtin: pulse | ||
| - builtin: calendar | ||
| ` | ||
| configPath := filepath.Join(tmpDir, core.ProjectConfigFile) | ||
| os.WriteFile(configPath, []byte(configYAML), 0o644) | ||
|
|
||
| output, err := executeCommand("status-line") | ||
| if err != nil { | ||
| t.Fatalf("status-line failed: %v\noutput: %s", err, output) | ||
| } | ||
|
|
||
| stripped := stripANSI(output) | ||
|
|
||
| // With no cache, all segments return empty and the fallback "hookwise" label shows. | ||
| if !strings.Contains(stripped, "hookwise") { | ||
| t.Errorf("status-line with no cache should show fallback 'hookwise', got: %s", stripped) | ||
| } | ||
| // No segment should show "--" anymore — they should be omitted entirely. | ||
| if strings.Contains(stripped, ": --") { | ||
| t.Errorf("status-line should not contain '--' placeholders, got: %s", stripped) | ||
| } | ||
| } | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // 16. Weather segment renders from cache | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| func TestStatusLineWeatherSegment(t *testing.T) { | ||
| feedCache := map[string]interface{}{ | ||
| "weather": map[string]interface{}{ | ||
| "type": "weather", | ||
| "timestamp": "2026-03-07T10:00:00Z", | ||
| "data": map[string]interface{}{ | ||
| "temperature": 55.0, | ||
| "temperatureUnit": "fahrenheit", | ||
| "emoji": "\U0001f327\ufe0f", | ||
| "description": "Rain", | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| result := renderBuiltinSegment("weather", feedCache, nil) | ||
| stripped := stripANSI(result) | ||
|
|
||
| if !strings.Contains(stripped, "55") { | ||
| t.Errorf("weather should show temperature 55, got: %s", stripped) | ||
| } | ||
| if !strings.Contains(stripped, "F") { | ||
| t.Errorf("weather should show unit F, got: %s", stripped) | ||
| } | ||
| if !strings.Contains(stripped, "Rain") { | ||
| t.Errorf("weather should show description Rain, got: %s", stripped) | ||
| } | ||
| } | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // 16b. Weather segment with real zero temperature (not placeholder) | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| func TestStatusLineWeatherZeroTemp(t *testing.T) { | ||
| feedCache := map[string]interface{}{ | ||
| "weather": map[string]interface{}{ | ||
| "type": "weather", | ||
| "timestamp": "2026-01-15T08:00:00Z", | ||
| "data": map[string]interface{}{ | ||
| "temperature": 0.0, | ||
| "temperatureUnit": "fahrenheit", | ||
| "emoji": "\u2744\ufe0f", | ||
| "description": "Snow", | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| result := renderBuiltinSegment("weather", feedCache, nil) | ||
| stripped := stripANSI(result) | ||
|
|
||
| // 0°F is a real temperature — must render "0", not "--" | ||
| if strings.Contains(stripped, "--") { | ||
| t.Errorf("real zero temperature should NOT show '--', got: %s", stripped) | ||
| } | ||
| if !strings.Contains(stripped, "0") { | ||
| t.Errorf("weather should show temperature 0, got: %s", stripped) | ||
| } | ||
| if !strings.Contains(stripped, "F") { | ||
| t.Errorf("weather should show unit F, got: %s", stripped) | ||
| } | ||
| } | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // 16c. Weather segment with celsius unit | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| func TestStatusLineWeatherCelsius(t *testing.T) { | ||
| feedCache := map[string]interface{}{ | ||
| "weather": map[string]interface{}{ | ||
| "type": "weather", | ||
| "timestamp": "2026-03-07T10:00:00Z", | ||
| "data": map[string]interface{}{ | ||
| "temperature": 22.0, | ||
| "temperatureUnit": "celsius", | ||
| "emoji": "\u2600\ufe0f", | ||
| "description": "Clear", | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| result := renderBuiltinSegment("weather", feedCache, nil) | ||
| stripped := stripANSI(result) | ||
|
|
||
| if !strings.Contains(stripped, "22") { | ||
| t.Errorf("weather should show temperature 22, got: %s", stripped) | ||
| } | ||
| if !strings.Contains(stripped, "C") { | ||
| t.Errorf("weather should show unit C for celsius, got: %s", stripped) | ||
| } | ||
| if strings.Contains(stripped, "F") { | ||
| t.Errorf("celsius weather should NOT show F, got: %s", stripped) | ||
| } | ||
| } | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // 17. Project segment renders from cache | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| func TestStatusLineProjectSegment(t *testing.T) { | ||
| feedCache := map[string]interface{}{ | ||
| "project": map[string]interface{}{ | ||
| "type": "project", | ||
| "timestamp": "2026-03-07T10:00:00Z", | ||
| "data": map[string]interface{}{ | ||
| "name": "myapp", | ||
| "branch": "feature/xyz", | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| result := renderBuiltinSegment("project", feedCache, nil) | ||
| stripped := stripANSI(result) | ||
|
|
||
| if !strings.Contains(stripped, "myapp") { | ||
| t.Errorf("project should show name myapp, got: %s", stripped) | ||
| } | ||
| if !strings.Contains(stripped, "(feature/xyz)") { | ||
| t.Errorf("project should show branch, got: %s", stripped) | ||
| } | ||
| } | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // 17b. Calendar segment renders from cache | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| func TestStatusLineCalendarSegment(t *testing.T) { | ||
| feedCache := map[string]interface{}{ | ||
| "calendar": map[string]interface{}{ | ||
| "type": "calendar", | ||
| "timestamp": "2026-03-07T10:00:00Z", | ||
| "data": map[string]interface{}{ | ||
| "events": []interface{}{ | ||
| map[string]interface{}{ | ||
| "name": "Standup", | ||
| "start": "2026-03-07T10:30:00Z", | ||
| "end": "2026-03-07T11:00:00Z", | ||
| "all_day": false, | ||
| "is_current": false, | ||
| }, | ||
| }, | ||
| "next_event": map[string]interface{}{ | ||
| "name": "Standup", | ||
| "time": "in 30m", | ||
| }, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| result := renderBuiltinSegment("calendar", feedCache, nil) | ||
| stripped := stripANSI(result) | ||
|
|
||
| if !strings.Contains(stripped, "\U0001f4c5") { | ||
| t.Errorf("calendar should show 📅 emoji, got: %s", stripped) | ||
| } | ||
| if !strings.Contains(stripped, "Standup") { | ||
| t.Errorf("calendar should show event name 'Standup', got: %s", stripped) | ||
| } | ||
| if !strings.Contains(stripped, "in 30m") { | ||
| t.Errorf("calendar should show relative time 'in 30m', got: %s", stripped) | ||
| } | ||
| } | ||
|
|
||
| func TestStatusLineCalendarSegmentEmpty(t *testing.T) { | ||
| // Calendar with no next_event should return empty. | ||
| feedCache := map[string]interface{}{ | ||
| "calendar": map[string]interface{}{ | ||
| "type": "calendar", | ||
| "timestamp": "2026-03-07T10:00:00Z", | ||
| "data": map[string]interface{}{ | ||
| "events": []interface{}{}, | ||
| "next_event": nil, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| result := renderBuiltinSegment("calendar", feedCache, nil) | ||
| if result != "" { | ||
| t.Errorf("calendar with nil next_event should return empty, got: %s", stripANSI(result)) | ||
| } | ||
| } | ||
|
|
||
| func TestStatusLineCalendarSegmentStringEvent(t *testing.T) { | ||
| // next_event as plain string (alternate format). | ||
| feedCache := map[string]interface{}{ | ||
| "calendar": map[string]interface{}{ | ||
| "type": "calendar", | ||
| "timestamp": "2026-03-07T10:00:00Z", | ||
| "data": map[string]interface{}{ | ||
| "next_event": "Sprint Review in 1h", | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| result := renderBuiltinSegment("calendar", feedCache, nil) | ||
| stripped := stripANSI(result) | ||
| if !strings.Contains(stripped, "Sprint Review in 1h") { | ||
| t.Errorf("calendar should render string event, got: %s", stripped) | ||
| } | ||
| } | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // 18. Placeholder fallback renders "--" not "0" | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| func TestStatusLinePlaceholderFallback(t *testing.T) { | ||
| // Feed with source: "placeholder" should be treated as no data. | ||
| feedCache := map[string]interface{}{ | ||
| "weather": map[string]interface{}{ | ||
| "type": "weather", | ||
| "timestamp": "2026-03-07T10:00:00Z", | ||
| "data": map[string]interface{}{ | ||
| "temperature": 0.0, | ||
| "temperatureUnit": "fahrenheit", | ||
| "emoji": "", | ||
| "description": "", | ||
| "source": "placeholder", | ||
| }, | ||
| }, | ||
| "pulse": map[string]interface{}{ | ||
| "type": "pulse", | ||
| "timestamp": "2026-03-07T10:00:00Z", | ||
| "data": map[string]interface{}{ | ||
| "session_count": 0, | ||
| "active_sessions": 0, | ||
| "source": "placeholder", | ||
| }, | ||
| }, | ||
| "project": map[string]interface{}{ | ||
| "type": "project", | ||
| "timestamp": "2026-03-07T10:00:00Z", | ||
| "data": map[string]interface{}{ | ||
| "name": "unknown (placeholder)", | ||
| "branch": "main", | ||
| "source": "placeholder", | ||
| }, | ||
| }, | ||
| "calendar": map[string]interface{}{ | ||
| "type": "calendar", | ||
| "timestamp": "2026-03-07T10:00:00Z", | ||
| "data": map[string]interface{}{ | ||
| "next_event": nil, | ||
| "source": "placeholder", | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| for _, name := range []string{"weather", "pulse", "project", "calendar"} { | ||
| result := renderBuiltinSegment(name, feedCache, nil) | ||
| // Placeholder feeds should produce empty output (segment omitted). | ||
| if result != "" { | ||
| t.Errorf("placeholder %q should return empty, got: %s", name, stripANSI(result)) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // 21. Session segment with daily summary | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| func TestStatusLineSessionSegment(t *testing.T) { | ||
| summary := &analytics.DailySummaryResult{TotalSessions: 3} | ||
| result := renderBuiltinSegment("session", nil, summary) | ||
| stripped := stripANSI(result) | ||
| if !strings.Contains(stripped, "session: 3") { | ||
| t.Errorf("session segment should show count, got: %s", stripped) | ||
| } | ||
| } | ||
|
|
||
| func TestStatusLineSessionSegmentNoSummary(t *testing.T) { | ||
| result := renderBuiltinSegment("session", nil, nil) | ||
| if result != "" { | ||
| t.Errorf("session segment without summary should return empty, got: %s", stripANSI(result)) | ||
| } | ||
| } | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // 22. Cost segment with daily summary | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| func TestStatusLineCostSegment(t *testing.T) { | ||
| summary := &analytics.DailySummaryResult{EstimatedCostUSD: 1.50} | ||
| result := renderBuiltinSegment("cost", nil, summary) | ||
| stripped := stripANSI(result) | ||
| if !strings.Contains(stripped, "cost: $1.50") { | ||
| t.Errorf("cost segment should show dollar amount, got: %s", stripped) | ||
| } | ||
| } | ||
|
|
||
| func TestStatusLineCostSegmentZero(t *testing.T) { | ||
| summary := &analytics.DailySummaryResult{EstimatedCostUSD: 0} | ||
| result := renderBuiltinSegment("cost", nil, summary) | ||
| if result != "" { | ||
| t.Errorf("cost segment with zero should return empty, got: %s", stripANSI(result)) | ||
| } | ||
| } | ||
|
|
||
| // writeJSONFile marshals v to JSON and writes it to path. | ||
| func writeJSONFile(t *testing.T, path string, v interface{}) { | ||
| t.Helper() | ||
| data, err := json.Marshal(v) | ||
| if err != nil { | ||
| t.Fatalf("failed to marshal JSON: %v", err) | ||
| } | ||
| if err := os.WriteFile(path, data, 0o644); err != nil { | ||
| t.Fatalf("failed to write %s: %v", path, err) | ||
| } | ||
| } | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // 23. recordAnalytics writes session data to Dolt | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| func TestRecordAnalytics_SessionStart(t *testing.T) { | ||
| tmpDir := t.TempDir() | ||
|
|
||
| payload := core.HookPayload{SessionID: "test-session-001"} | ||
| recordAnalytics(core.EventSessionStart, payload, tmpDir) | ||
|
|
||
| // Verify session was recorded. | ||
| db, err := analytics.Open(tmpDir) | ||
| if err != nil { | ||
| t.Fatalf("failed to open analytics DB: %v", err) | ||
| } | ||
| defer db.Close() | ||
|
|
||
| a := analytics.NewAnalytics(db) | ||
| today := time.Now().UTC().Format("2006-01-02") | ||
| summary, err := a.DailySummary(context.Background(), today) | ||
| if err != nil { | ||
| t.Fatalf("daily summary failed: %v", err) | ||
| } | ||
| if summary.TotalSessions < 1 { | ||
| t.Errorf("expected at least 1 session after recordAnalytics, got %d", summary.TotalSessions) | ||
| } | ||
| } | ||
|
|
||
| func TestRecordAnalytics_PostToolUse(t *testing.T) { | ||
| tmpDir := t.TempDir() | ||
|
|
||
| // First create a session. | ||
| payload := core.HookPayload{SessionID: "test-session-002"} | ||
| recordAnalytics(core.EventSessionStart, payload, tmpDir) | ||
|
|
||
| // Record a tool use event. | ||
| payload.ToolName = "Bash" | ||
| recordAnalytics(core.EventPostToolUse, payload, tmpDir) | ||
|
|
||
| db, err := analytics.Open(tmpDir) | ||
| if err != nil { | ||
| t.Fatalf("failed to open analytics DB: %v", err) | ||
| } | ||
| defer db.Close() | ||
|
|
||
| a := analytics.NewAnalytics(db) | ||
| today := time.Now().UTC().Format("2006-01-02") | ||
| summary, err := a.DailySummary(context.Background(), today) | ||
| if err != nil { | ||
| t.Fatalf("daily summary failed: %v", err) | ||
| } | ||
| if summary.TotalEvents < 1 { | ||
| t.Errorf("expected at least 1 event after PostToolUse, got %d", summary.TotalEvents) | ||
| } | ||
| } | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // 24. Insights segment renders from cache | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| func TestStatusLineInsightsSegment(t *testing.T) { | ||
| feedCache := map[string]interface{}{ | ||
| "insights": map[string]interface{}{ | ||
| "type": "insights", | ||
| "timestamp": "2026-03-07T10:00:00Z", | ||
| "data": map[string]interface{}{ | ||
| "total_sessions": 72, | ||
| "total_messages": 486, | ||
| "total_lines_added": 12800, | ||
| "avg_duration_minutes": 225.0, | ||
| "top_tools": []interface{}{ | ||
| map[string]interface{}{"name": "Bash", "count": 509}, | ||
| map[string]interface{}{"name": "Read", "count": 323}, | ||
| map[string]interface{}{"name": "Edit", "count": 228}, | ||
| }, | ||
| "friction_counts": map[string]interface{}{"wrong_approach": 32, "misunderstood_request": 14}, | ||
| "friction_total": 46, | ||
| "peak_hour": 7, | ||
| "days_active": 17, | ||
| "staleness_days": 30, | ||
| "recent_msgs_per_day": 55, | ||
| "recent_messages": 110, | ||
| "recent_days_active": 2, | ||
| "recent_session": map[string]interface{}{ | ||
| "id": "s1", | ||
| "duration_minutes": 45.0, | ||
| "lines_added": 200, | ||
| "friction_count": 3, | ||
| "outcome": "success", | ||
| "tool_errors": 0, | ||
| }, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| // Test insights segment. | ||
| result := renderBuiltinSegment("insights", feedCache, nil) | ||
| stripped := stripANSI(result) | ||
| if !strings.Contains(stripped, "72 sessions") { | ||
| t.Errorf("insights should show 72 sessions, got: %s", stripped) | ||
| } | ||
| if !strings.Contains(stripped, "17d") { | ||
| t.Errorf("insights should show 17d, got: %s", stripped) | ||
| } | ||
| if !strings.Contains(stripped, "12.8k") { | ||
| t.Errorf("insights should show 12.8k lines, got: %s", stripped) | ||
| } | ||
| } | ||
|
|
||
| func TestStatusLineInsightsFrictionSegment(t *testing.T) { | ||
| // Friction with recent session friction > 0. | ||
| feedCache := map[string]interface{}{ | ||
| "insights": map[string]interface{}{ | ||
| "type": "insights", | ||
| "timestamp": "2026-03-07T10:00:00Z", | ||
| "data": map[string]interface{}{ | ||
| "friction_total": 5, | ||
| "friction_counts": map[string]interface{}{"wrong_approach": 3, "misunderstood_request": 2}, | ||
| "recent_session": map[string]interface{}{ | ||
| "friction_count": 3, | ||
| }, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| result := renderBuiltinSegment("insights_friction", feedCache, nil) | ||
| stripped := stripANSI(result) | ||
| if !strings.Contains(stripped, "3 friction") { | ||
| t.Errorf("friction should show 3, got: %s", stripped) | ||
| } | ||
| if !strings.Contains(stripped, "wrong approach") { | ||
| t.Errorf("friction should show top category, got: %s", stripped) | ||
| } | ||
| if !strings.Contains(stripped, "break tasks into steps") { | ||
| t.Errorf("friction should show tip, got: %s", stripped) | ||
| } | ||
| } | ||
|
|
||
| func TestStatusLineInsightsFrictionClean(t *testing.T) { | ||
| // Clean session (zero recent, non-zero total). | ||
| feedCache := map[string]interface{}{ | ||
| "insights": map[string]interface{}{ | ||
| "type": "insights", | ||
| "timestamp": "2026-03-07T10:00:00Z", | ||
| "data": map[string]interface{}{ | ||
| "friction_total": 12, | ||
| "friction_counts": map[string]interface{}{}, | ||
| "recent_session": map[string]interface{}{ | ||
| "friction_count": 0, | ||
| }, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| result := renderBuiltinSegment("insights_friction", feedCache, nil) | ||
| stripped := stripANSI(result) | ||
| if !strings.Contains(stripped, "Clean session") { | ||
| t.Errorf("should show clean session, got: %s", stripped) | ||
| } | ||
| if !strings.Contains(stripped, "12 in 30d") { | ||
| t.Errorf("should show historical friction count, got: %s", stripped) | ||
| } | ||
| } | ||
|
|
||
| func TestStatusLineInsightsPaceSegment(t *testing.T) { | ||
| feedCache := map[string]interface{}{ | ||
| "insights": map[string]interface{}{ | ||
| "type": "insights", | ||
| "timestamp": "2026-03-07T10:00:00Z", | ||
| "data": map[string]interface{}{ | ||
| "total_messages": 470, | ||
| "total_lines_added": 5400, | ||
| "total_sessions": 42, | ||
| "days_active": 10, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| result := renderBuiltinSegment("insights_pace", feedCache, nil) | ||
| stripped := stripANSI(result) | ||
| if !strings.Contains(stripped, "47 msgs/day") { | ||
| t.Errorf("pace should show 47 msgs/day, got: %s", stripped) | ||
| } | ||
| if !strings.Contains(stripped, "5.4k") { | ||
| t.Errorf("pace should show 5.4k lines, got: %s", stripped) | ||
| } | ||
| if !strings.Contains(stripped, "42 sessions") { | ||
| t.Errorf("pace should show 42 sessions, got: %s", stripped) | ||
| } | ||
| } | ||
|
|
||
| func TestStatusLineInsightsTrendSegment(t *testing.T) { | ||
| feedCache := map[string]interface{}{ | ||
| "insights": map[string]interface{}{ | ||
| "type": "insights", | ||
| "timestamp": "2026-03-07T10:00:00Z", | ||
| "data": map[string]interface{}{ | ||
| "top_tools": []interface{}{ | ||
| map[string]interface{}{"name": "Bash", "count": 50}, | ||
| map[string]interface{}{"name": "Read", "count": 30}, | ||
| map[string]interface{}{"name": "Edit", "count": 20}, | ||
| }, | ||
| "peak_hour": 14, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| result := renderBuiltinSegment("insights_trend", feedCache, nil) | ||
| stripped := stripANSI(result) | ||
| if !strings.Contains(stripped, "Top: Bash, Read") { | ||
| t.Errorf("trend should show top 2 tools, got: %s", stripped) | ||
| } | ||
| if !strings.Contains(stripped, "Peak: afternoon") { | ||
| t.Errorf("trend should show peak afternoon, got: %s", stripped) | ||
| } | ||
| } | ||
|
|
||
| func TestStatusLineInsightsTrendPeakHour(t *testing.T) { | ||
| tests := []struct { | ||
| hour int | ||
| label string | ||
| }{ | ||
| {8, "morning"}, | ||
| {14, "afternoon"}, | ||
| {20, "evening"}, | ||
| {3, "night"}, | ||
| } | ||
| for _, tt := range tests { | ||
| feedCache := map[string]interface{}{ | ||
| "insights": map[string]interface{}{ | ||
| "type": "insights", | ||
| "timestamp": "2026-03-07T10:00:00Z", | ||
| "data": map[string]interface{}{ | ||
| "top_tools": []interface{}{ | ||
| map[string]interface{}{"name": "Read", "count": 10}, | ||
| }, | ||
| "peak_hour": tt.hour, | ||
| }, | ||
| }, | ||
| } | ||
| result := renderBuiltinSegment("insights_trend", feedCache, nil) | ||
| stripped := stripANSI(result) | ||
| if !strings.Contains(stripped, "Peak: "+tt.label) { | ||
| t.Errorf("hour %d should map to %s, got: %s", tt.hour, tt.label, stripped) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| func TestStatusLineInsightsNoData(t *testing.T) { | ||
| // Insights segments with no cache data should handle gracefully. | ||
| emptyCache := map[string]interface{}{} | ||
|
|
||
| result := renderBuiltinSegment("insights", emptyCache, nil) | ||
| if result != "" { | ||
| t.Errorf("insights with no data should return empty, got: %s", stripANSI(result)) | ||
| } | ||
|
|
||
| // Friction/pace/trend also return empty string with no data. | ||
| assert := func(name string) { | ||
| r := renderBuiltinSegment(name, emptyCache, nil) | ||
| if r != "" { | ||
| t.Errorf("%s with no data should return empty, got: %s", name, r) | ||
| } | ||
| } | ||
| assert("insights_friction") | ||
| assert("insights_pace") | ||
| assert("insights_trend") | ||
| } | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // 25. Multi-line status output with insights | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| func TestStatusLineMultiLine(t *testing.T) { | ||
| tmpDir := t.TempDir() | ||
|
|
||
| origDir, err := os.Getwd() | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if err := os.Chdir(tmpDir); err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| defer os.Chdir(origDir) | ||
|
|
||
| stateDir := filepath.Join(tmpDir, ".hookwise") | ||
| t.Setenv("HOOKWISE_STATE_DIR", stateDir) | ||
|
|
||
| // Create insights cache with real data. | ||
| cacheDir := filepath.Join(stateDir, "state") | ||
| os.MkdirAll(cacheDir, 0o700) | ||
|
|
||
| insightsEnvelope := map[string]interface{}{ | ||
| "type": "insights", | ||
| "timestamp": "2026-03-08T10:00:00Z", | ||
| "data": map[string]interface{}{ | ||
| "total_sessions": 72, | ||
| "total_messages": 486, | ||
| "total_lines_added": 12800, | ||
| "avg_duration_minutes": 225.0, | ||
| "top_tools": []interface{}{ | ||
| map[string]interface{}{"name": "Bash", "count": 509}, | ||
| map[string]interface{}{"name": "Read", "count": 323}, | ||
| }, | ||
| "friction_counts": map[string]interface{}{"wrong_approach": 5}, | ||
| "friction_total": 5, | ||
| "peak_hour": 14, | ||
| "days_active": 17, | ||
| "staleness_days": 30, | ||
| "recent_msgs_per_day": 55, | ||
| "recent_messages": 110, | ||
| "recent_days_active": 2, | ||
| "recent_session": map[string]interface{}{ | ||
| "friction_count": 0, | ||
| }, | ||
| }, | ||
| } | ||
| writeJSONFile(t, filepath.Join(cacheDir, "insights.json"), insightsEnvelope) | ||
|
|
||
| configYAML := `version: 1 | ||
| status_line: | ||
| enabled: true | ||
| segments: | ||
| - session | ||
| - cost | ||
| ` | ||
| configPath := filepath.Join(tmpDir, core.ProjectConfigFile) | ||
| os.WriteFile(configPath, []byte(configYAML), 0o644) | ||
|
|
||
| output, err := executeCommand("status-line") | ||
| if err != nil { | ||
| t.Fatalf("status-line failed: %v\noutput: %s", err, output) | ||
| } | ||
|
|
||
| stripped := stripANSI(output) | ||
| lines := strings.Split(strings.TrimSpace(stripped), "\n") | ||
|
|
||
| // Should have at least 2 lines: main segment line + insights summary lines. | ||
| if len(lines) < 2 { | ||
| t.Errorf("expected at least 2 lines for multi-line output, got %d:\n%s", len(lines), stripped) | ||
| } | ||
|
|
||
| // Check for insights summary content in lines 2+. | ||
| if !strings.Contains(stripped, "72 sessions") { | ||
| t.Errorf("multi-line output should contain '72 sessions', got:\n%s", stripped) | ||
| } | ||
| if !strings.Contains(stripped, "Bash(509)") { | ||
| t.Errorf("multi-line output should contain tool breakdown, got:\n%s", stripped) | ||
| } | ||
| } | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // 26. formatLargeNumber helper | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| func TestFormatLargeNumber(t *testing.T) { | ||
| tests := []struct { | ||
| input int | ||
| expected string | ||
| }{ | ||
| {0, "0"}, | ||
| {340, "340"}, | ||
| {999, "999"}, | ||
| {1000, "1k"}, | ||
| {5400, "5.4k"}, | ||
| {12800, "12.8k"}, | ||
| {28000, "28k"}, | ||
| } | ||
| for _, tt := range tests { | ||
| result := formatLargeNumber(tt.input) | ||
| if result != tt.expected { | ||
| t.Errorf("formatLargeNumber(%d) = %q, want %q", tt.input, result, tt.expected) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'stdlib testing helpers used in cmd/hookwise/cli_test.go:\n'
rg -n 't\.(Fatal|Fatalf|Error|Errorf)\(' cmd/hookwise/cli_test.go
printf '\nexisting testify usage/dependency in the repo:\n'
rg -n 'github.com/stretchr/testify/(assert|require)' --type=go
rg -n 'stretchr/testify' go.mod go.sumRepository: vishnujayvel/hookwise
Length of output: 10668
Use testify assertions instead of stdlib testing helpers in new tests.
Lines 421–1255 use t.Fatal*/t.Error* throughout. Replace with require.NoError, require.True, assert.Contains, etc. The project already imports testify (v1.11.1 in go.mod) and uses it consistently across internal/, pkg/, tests/. This aligns with the repository's testing convention.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/hookwise/cli_test.go` around lines 421 - 1255, The tests in this file
(e.g., TestStatusLineWithFeedCache, TestStatusLineWithMissingCache,
TestStatusLineWeatherSegment, TestRecordAnalytics_SessionStart,
TestFormatLargeNumber and other Test* functions) use stdlib
t.Fatal/t.Fatalf/t.Error/t.Errorf; replace these with testify assertions: use
require.NoError/require.Nil for errors (e.g., after executeCommand, os.Chdir,
analytics.Open), require.NoError/require.Zero/require.NotZero or require.Greater
for numeric expectations (e.g., summary.TotalSessions, summary.TotalEvents),
require.Empty/require.NotEmpty or require.Equal for expected strings, and
assert.Contains/assert.NotContains for substring checks of stripANSI output and
formatLargeNumber tests; also swap t.Helper usage to keep writeJSONFile but
change its test failures to require.NoError/require.FailNow as appropriate so
all test assertions use testify's require/assert APIs and imports
(require/assert) instead of t.*.
Owner
Author
There was a problem hiding this comment.
Valid point — the CLI tests use stdlib t.Fatal/t.Errorf while the rest of the codebase uses testify. This is a larger refactor (~40 test functions, 100+ assertions) that I'd prefer to do in a dedicated follow-up PR to keep this one focused on the calendar feature. Filed as tech debt to address.
There was a problem hiding this comment.
@vishnujayvel, understood — that's a reasonable scope call. Migrating ~40 test functions and 100+ assertions is meaningful churn for a PR focused on calendar integration.
Would you like me to open a GitHub issue to track the testify migration for cmd/hookwise/cli_test.go? I can file it with a clear description and link back to this comment so it doesn't get lost.
(ᓀ ᓀ) 🐇
🧠 Learnings used
Learnt from: CR
Repo: vishnujayvel/hookwise PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-08T04:55:01.595Z
Learning: Applies to **/*_test.go : Contract tests must maintain byte-identical JSON stdout per fixtures in testdata/contracts/ (ARCH-6).
Learnt from: CR
Repo: vishnujayvel/hookwise PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-08T04:55:01.595Z
Learning: Applies to **/*_test.go : Use testify/assert and testify/require for assertions, not stdlib testing alone.
Learnt from: vishnujayvel
Repo: vishnujayvel/hookwise PR: 18
File: tui/tests/test_snapshots.py:63-77
Timestamp: 2026-03-06T05:43:17.362Z
Learning: In `tui/tests/test_snapshots.py` (vishnujayvel/hookwise), the post-toggle Coaching snapshot test (verifying FeatureCard CSS class update after Switch toggle) is intentionally deferred to a follow-up PR. It should be implemented as a dedicated interaction test (e.g., `test_coaching_toggle.py`), not by extending the parametrized `test_tab_snapshot` suite, because it requires mounting the Coaching tab, simulating a Switch press, awaiting config write + CSS class update settlement, then capturing a second golden snapshot.
Learnt from: vishnujayvel
Repo: vishnujayvel/hookwise PR: 18
File: hooks/agent-tracker.sh:22-26
Timestamp: 2026-03-06T05:48:14.680Z
Learning: In `hooks/agent-tracker.sh` (vishnujayvel/hookwise), the four separate `python3` invocations to parse `agent_id`, `worktree`, `team_name`, and `strategy` from stdin JSON are intentionally left as-is for the current working version. The owner plans to consolidate them into a single `python3 -c` call (emitting tab-separated or `eval`-able output) in a follow-up PR. The rationale is that hook invocation overhead is dominated by Python startup cost, making 1 vs 4 invocations only ~80ms difference.
Learnt from: vishnujayvel
Repo: vishnujayvel/hookwise PR: 18
File: tests/core/status-line/agents-segment.test.ts:1-10
Timestamp: 2026-03-06T02:34:14.644Z
Learning: In the `tests/core/status-line/` directory of the hookwise repository, the convention is to use segment-specific test file names (e.g., `agents-segment.test.ts`, `insights-segments.test.ts`, `segments-new.test.ts`) rather than strictly mirroring the single source file `src/core/status-line/segments.ts`. Multiple test files cover different subsets of segments from that one source file.
Learnt from: vishnujayvel
Repo: vishnujayvel/hookwise PR: 18
File: tui/tests/test_snapshots.py:73-73
Timestamp: 2026-03-06T05:48:08.174Z
Learning: In `tui/tests/test_snapshots.py` (vishnujayvel/hookwise), mocking `HookwiseTUI._write_pid()` in snapshot tests is deferred to a follow-up PR. The current behavior (writing to `~/.hookwise/tui.pid` during tests) is considered safe in practice due to single-threaded execution and the PID guard in `_remove_pid`. The planned fix is to add a `monkeypatch` fixture in a `conftest.py` to no-op `_write_pid`, ensuring no real filesystem side effects during snapshot tests.
Learnt from: CR
Repo: vishnujayvel/hookwise PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-08T04:55:01.596Z
Learning: Test the cross-language boundary between Go feeds, JSON cache, and Python TUI by testing both sides.
Comment on lines
+887
to
+937
| func TestRecordAnalytics_SessionStart(t *testing.T) { | ||
| tmpDir := t.TempDir() | ||
|
|
||
| payload := core.HookPayload{SessionID: "test-session-001"} | ||
| recordAnalytics(core.EventSessionStart, payload, tmpDir) | ||
|
|
||
| // Verify session was recorded. | ||
| db, err := analytics.Open(tmpDir) | ||
| if err != nil { | ||
| t.Fatalf("failed to open analytics DB: %v", err) | ||
| } | ||
| defer db.Close() | ||
|
|
||
| a := analytics.NewAnalytics(db) | ||
| today := time.Now().UTC().Format("2006-01-02") | ||
| summary, err := a.DailySummary(context.Background(), today) | ||
| if err != nil { | ||
| t.Fatalf("daily summary failed: %v", err) | ||
| } | ||
| if summary.TotalSessions < 1 { | ||
| t.Errorf("expected at least 1 session after recordAnalytics, got %d", summary.TotalSessions) | ||
| } | ||
| } | ||
|
|
||
| func TestRecordAnalytics_PostToolUse(t *testing.T) { | ||
| tmpDir := t.TempDir() | ||
|
|
||
| // First create a session. | ||
| payload := core.HookPayload{SessionID: "test-session-002"} | ||
| recordAnalytics(core.EventSessionStart, payload, tmpDir) | ||
|
|
||
| // Record a tool use event. | ||
| payload.ToolName = "Bash" | ||
| recordAnalytics(core.EventPostToolUse, payload, tmpDir) | ||
|
|
||
| db, err := analytics.Open(tmpDir) | ||
| if err != nil { | ||
| t.Fatalf("failed to open analytics DB: %v", err) | ||
| } | ||
| defer db.Close() | ||
|
|
||
| a := analytics.NewAnalytics(db) | ||
| today := time.Now().UTC().Format("2006-01-02") | ||
| summary, err := a.DailySummary(context.Background(), today) | ||
| if err != nil { | ||
| t.Fatalf("daily summary failed: %v", err) | ||
| } | ||
| if summary.TotalEvents < 1 { | ||
| t.Errorf("expected at least 1 event after PostToolUse, got %d", summary.TotalEvents) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Cover the actual dispatch wrapper in analytics tests.
These tests call recordAnalytics directly, so they never exercise the new go recordAnalytics(...) + process-shutdown path in newDispatchCmd. Add one command-level test around dispatch so dropped writes in the real wrapper can't regress unnoticed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/hookwise/cli_test.go` around lines 887 - 937, The tests currently call
recordAnalytics directly and miss the goroutine+shutdown path exercised by the
real CLI dispatch wrapper; add a command-level test that runs the dispatch path
created by newDispatchCmd (invoke the dispatch command similar to how the CLI
would, e.g., calling newDispatchCmd and executing it) so the code path that does
go recordAnalytics(...) and triggers process shutdown is exercised; ensure the
test creates a temp analytics dir, invokes the command (or executes dispatch)
with a payload that causes a session start and a tool-use, waits for command
completion, then opens analytics.Open and asserts DailySummary shows the
expected TotalSessions/TotalEvents to catch dropped writes in the wrapper.
Owner
Author
There was a problem hiding this comment.
Agree this would add coverage. The dispatch wrapper's go recordAnalytics(...) + os.Exit interaction is tricky to test deterministically without races. Will add a command-level dispatch test in a follow-up. The existing direct tests at least validate the analytics pipeline itself works.
There was a problem hiding this comment.
@vishnujayvel, that's a fair call — the go recordAnalytics(...) + os.Exit path is genuinely difficult to assert on deterministically without either a sleep hack or injecting a shutdown hook. The direct pipeline tests are a solid safety net for now.
One approach worth considering for the follow-up: inject a shutdownFn func() into the dispatch command (defaulting to os.Exit(code)) so tests can swap it out for a no-op + sync.WaitGroup drain, making the goroutine observable without races.
Would you like me to open a GitHub issue to track adding the command-level dispatch test in a follow-up PR?
🧠 Learnings used
Learnt from: CR
Repo: vishnujayvel/hookwise PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-08T04:55:01.595Z
Learning: Applies to internal/core/**/*.go : Dispatch must always exit with status 0 even on errors (ARCH-1: Fail-open).
Learnt from: vishnujayvel
Repo: vishnujayvel/hookwise PR: 18
File: tests/core/status-line/agents-segment.test.ts:1-10
Timestamp: 2026-03-06T02:34:14.644Z
Learning: In the `tests/core/status-line/` directory of the hookwise repository, the convention is to use segment-specific test file names (e.g., `agents-segment.test.ts`, `insights-segments.test.ts`, `segments-new.test.ts`) rather than strictly mirroring the single source file `src/core/status-line/segments.ts`. Multiple test files cover different subsets of segments from that one source file.
Learnt from: CR
Repo: vishnujayvel/hookwise PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-08T04:55:01.596Z
Learning: Test the cross-language boundary between Go feeds, JSON cache, and Python TUI by testing both sides.
Comment on lines
+104
to
+107
| // Analytics recording (non-blocking, ARCH-7). | ||
| if config.Analytics.Enabled && payload.SessionID != "" { | ||
| go recordAnalytics(eventType, payload, config.Analytics.DBPath) | ||
| } |
There was a problem hiding this comment.
Don't detach analytics writes from the dispatch lifecycle.
This goroutine has no completion signal, so the existing os.Exit path can terminate it before Dolt finishes Open/Commit. That makes session/event recording nondeterministic under cold starts or lock contention. Use a bounded wait or explicit flush signal if you need to preserve fail-open behavior.
Based on learnings, "Use SetMaxOpenConns(1) to serialize Dolt writes (ARCH-2)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/hookwise/main.go` around lines 104 - 107, The analytics write is detached
via an untracked goroutine (go recordAnalytics(...)) so os.Exit or Dolt
Open/Commit can terminate it prematurely; change the dispatch to run
recordAnalytics synchronously or start it with a completion signal and wait with
a bounded timeout (e.g., a context with timeout or a sync.WaitGroup + select on
timeout) before exiting so analytics either completes or times out
deterministically; update the call site that currently uses eventType, payload,
config.Analytics.DBPath to use the flush/wait mechanism and ensure
recordAnalytics honors a context/cancel and that Dolt writes are serialized
(apply SetMaxOpenConns(1) where Dolt DB is opened) to avoid concurrent Dolt
writes.
Owner
Author
There was a problem hiding this comment.
This is intentional per ARCH-7 (side effects non-blocking with per-goroutine recover()). The dispatch command has a 50ms grace period after SafeDispatch returns before the process exits, which gives the goroutine enough time for the Dolt write in practice. A sync.WaitGroup with bounded timeout would be more robust — will consider for a follow-up, but the current design is a deliberate fail-open tradeoff: if the analytics goroutine doesn't finish in time, we'd rather drop the write than block the CLI.
There was a problem hiding this comment.
@vishnujayvel, understood — the non-blocking pattern is consistent with ARCH-7, and the 50ms grace period is a reasonable practical bound given the fail-open contract. The main residual risk (cold Dolt open or lock contention exceeding 50ms causing a silent drop) is a known and accepted tradeoff here.
When you do revisit this, a minimal pattern would be:
// dispatch goroutine with bounded flush
analyticsDone := make(chan struct{})
if config.Analytics.Enabled && payload.SessionID != "" {
go func() {
defer close(analyticsDone)
recordAnalytics(eventType, payload, config.Analytics.DBPath)
}()
} else {
close(analyticsDone)
}
// ... later, before os.Exit:
select {
case <-analyticsDone:
case <-time.After(200 * time.Millisecond):
core.Logger().Warn("analytics: timed out waiting for write")
}
os.Exit(result.ExitCode)This keeps the fail-open guarantee while making the flush deterministic. Happy to defer — noting it for future reference.
✏️ Learnings added
Learnt from: vishnujayvel
Repo: vishnujayvel/hookwise PR: 62
File: cmd/hookwise/main.go:107-110
Timestamp: 2026-03-09T04:17:28.113Z
Learning: In `cmd/hookwise/main.go` (vishnujayvel/hookwise), the `go recordAnalytics(...)` goroutine in `newDispatchCmd` is intentionally non-blocking per ARCH-7 (side effects non-blocking with per-goroutine recover()). The dispatch command provides a 50ms grace period (`time.Sleep(50 * time.Millisecond)`) before `os.Exit` as the flush window. The design is a deliberate fail-open tradeoff: if analytics doesn't finish in time, the write is dropped rather than blocking the CLI. A `sync.WaitGroup` or channel-based bounded timeout (e.g., 200ms) is acknowledged as a future improvement but intentionally deferred.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: CR
Repo: vishnujayvel/hookwise PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-08T04:55:01.595Z
Learning: Applies to internal/core/**/*.go : Dispatch must always exit with status 0 even on errors (ARCH-1: Fail-open).
Learnt from: CR
Repo: vishnujayvel/hookwise PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-08T04:55:01.595Z
Learning: Applies to internal/analytics/**/*.go : Use SetMaxOpenConns(1) to serialize Dolt writes (ARCH-2).
Learnt from: vishnujayvel
Repo: vishnujayvel/hookwise PR: 18
File: hooks/agent-tracker.sh:0-0
Timestamp: 2026-03-06T05:48:03.577Z
Learning: In `hooks/agent-tracker.sh` (vishnujayvel/hookwise), the read/modify/write on `active-agents.json` is intentionally not locked. Claude Code invokes hooks serially (one hook per event), so concurrent writes only occur with parallel Claude Code sessions sharing the same `HOOKWISE_STATE_DIR` — treated as an edge case. Adding `flock` is deferred to a follow-up hardening pass due to the added POSIX dependency. The atomic `mv` via temp file is sufficient for the single-session case.
Learnt from: CR
Repo: vishnujayvel/hookwise PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-08T04:55:01.595Z
Learning: Applies to internal/**/*.go : Side effects must be non-blocking with per-goroutine recover() (ARCH-7).
| cloud.google.com/go/iam v1.5.3 // indirect | ||
| cloud.google.com/go/monitoring v1.24.3 // indirect | ||
| cloud.google.com/go/storage v1.56.0 // indirect | ||
| filippo.io/edwards25519 v1.1.0 // indirect |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for newer versions of flagged dependencies
echo "Checking filippo.io/edwards25519..."
go list -m -versions filippo.io/edwards25519 2>/dev/null | head -1
echo "Checking github.com/aws/aws-sdk-go..."
go list -m -versions github.com/aws/aws-sdk-go 2>/dev/null | head -1Repository: vishnujayvel/hookwise
Length of output: 16581
🏁 Script executed:
# Check if edwards25519 and aws-sdk-go are direct or indirect dependencies
cd /tmp && cat go.mod | head -50Repository: vishnujayvel/hookwise
Length of output: 103
🌐 Web query:
GO-2026-4503 edwards25519 vulnerability
💡 Result:
GO-2026-4503 is the Go vulnerability record for CVE-2026-26958 affecting filippo.io/edwards25519. Versions < v1.1.1 are affected. [1][2]
What’s the issue
(*Point).MultiScalarMult failed to initialize its receiver, leading to:
- Incorrect results if called on a non-identity (already-initialized) point.
- Undefined behavior if called on an uninitialized point; notably, if the receiver is the zero value, it can return an invalid point that compares equal to every point. [1][2][3]
Impact / exposure
This is considered low severity and applies primarily to code that directly uses the advanced API Point.MultiScalarMult; many transitive users are not impacted (e.g., depending on filippo.io/edwards25519 only via github.com/go-sql-driver/mysql). [2][3]
Fix
Upgrade to filippo.io/edwards25519 v1.1.1 or later. [1][2]
References: Go vuln entry and upstream advisory. [1][2]
Sources
[1] Go Vulnerability Database (pkg.go.dev): GO-2026-4503 — affected versions and description
[2] GitHub Security Advisory: GHSA-fw7p-63qq-7hpr — root cause and behavioral details
[3] OSV (Debian CVE record): CVE-2026-26958 — additional impact notes and fix version
🌐 Web query:
GO-2022-0635 aws-sdk-go S3 crypto
💡 Result:
GO-2022-0635 is a Go vulnerability report for CVE-2020-8912 / GHSA-7f33-f4f5-xwgw affecting github.com/aws/aws-sdk-go/service/s3/s3crypto (the AWS SDK for Go v1 “S3 Crypto” / S3 Encryption Client implementation). It’s an in-band key negotiation flaw: an attacker who can write/modify objects in the target S3 bucket can tamper with metadata to change the declared content-encryption algorithm (e.g., from AES-GCM to AES-CTR or AES-CBC). If the attacker also has access to a decryption-oracle signal (an endpoint that reveals decrypt success/failure), this can enable plaintext recovery and/or message forgery, including a path to recover the AES-GCM authentication key in certain scenarios. [1][2]
Fix / mitigation: use S3 Encryption Client / S3 Crypto SDK v2 (which authenticates the algorithm, e.g., via KMS+context key wrapping for new files) and re-encrypt old objects that were encrypted with the vulnerable scheme (notably GCM with the older KMS wrapping option). Note: the Go vuln entry flags the v1 package as “all versions, no known fixed” because the fix is effectively “move to v2 / new client,” not a patch within the v1 package line. [1][2][3]
Sources
- Go vulnerability database entry: GO-2022-0635 (pkg.go.dev) [1]
- Google Security Research advisory: GHSA-7f33-f4f5-xwgw [2]
- AWS Developer Tools Blog (Aug 7, 2020): “Updates to the Amazon S3 Encryption Client” [3]
🏁 Script executed:
cat /workspace/*/go.mod | head -60Repository: vishnujayvel/hookwise
Length of output: 118
🏁 Script executed:
# Find and display go.mod from root
find . -name "go.mod" -type f | head -1 | xargs cat | head -60Repository: vishnujayvel/hookwise
Length of output: 2821
🏁 Script executed:
# Check if aws-sdk-go is directly used in the codebase
rg "aws-sdk-go|aws-sdk|s3crypto" --type go -i | head -20Repository: vishnujayvel/hookwise
Length of output: 47
🏁 Script executed:
# Check what package brings in aws-sdk-go as transitive dependency
rg "aws" go.modRepository: vishnujayvel/hookwise
Length of output: 112
🌐 Web query:
GO-2022-0646 aws-sdk-go vulnerability
💡 Result:
GO-2022-0646 is the Go vulnerability ID for CVE-2020-8911 / GHSA-f5pg-7wfw-84q9, a CBC padding-oracle issue in the Amazon S3 “Crypto/Encryption Client” code shipped in github.com/aws/aws-sdk-go/service/s3/s3crypto. It affects uses of NewEncryptionClient / NewDecryptionClient that allow AES-CBC without an integrity check (no MAC), enabling plaintext recovery if an attacker can modify/upload ciphertext in the bucket and observe whether decrypt succeeded/failed. [1][2]
Impact / scope
- Package:
github.com/aws/aws-sdk-go(specificallyservice/s3/s3crypto) [1] - Condition: using the legacy S3 encryption client with AES-CBC (padding-oracle side channel) [1][3]
Fix / remediation
- Migrate to the “V2” S3 encryption/decryption clients (and re-encrypt affected objects), rather than continuing to use the legacy AES-CBC option. [1][3]
- In
aws-sdk-gov1, the V2 clients (EncryptionClientV2/DecryptionClientV2) were introduced in release v1.33.0 (2020-07-01) via PR#3403. [4]
References
- Go/OSV entry (GO-2022-0646) [1]
- Google security advisory (GHSA-f5pg-7wfw-84q9) [2]
- AWS blog: “Updates to the Amazon S3 Encryption Client” (describes removal/deprecation of AES-CBC for new objects and the padding-oracle issue) [3]
- aws-sdk-go PR introducing S3 crypto V2 clients (
#3403) [4]
Sources: [1] https://pkg.go.dev/vuln/GO-2022-0646 [2] GHSA-f5pg-7wfw-84q9 [3] https://aws.amazon.com/blogs/developer/updates-to-the-amazon-s3-encryption-client/ [4] aws/aws-sdk-go#3403
Correct severity classification of transitive security advisories.
Both dependencies are confirmed transitive (indirect):
filippo.io/edwards25519 v1.1.0(GO-2026-4503): LOW severity—affects specific cryptographic edge case. Upgrade to v1.1.1+ available.github.com/aws/aws-sdk-go v1.50.16(GO-2022-0635, GO-2022-0646): HIGH/CRITICAL severity—padding-oracle and in-band key negotiation flaws enabling plaintext recovery and message forgery in S3 Encryption Client. No patch available in v1.x; migration to V2 clients required.
While hookwise may not directly use S3 Crypto functionality, the aws-sdk-go advisories should not be classified as low-severity. Strongly prioritize migrating away from the affected v1 package or evaluate if S3 Crypto usage can be eliminated.
🧰 Tools
🪛 OSV Scanner (2.3.3)
[LOW] 36-36: filippo.io/edwards25519 1.1.0: Invalid result or undefined behavior in filippo.io/edwards25519
(GO-2026-4503)
[LOW] 36-36: filippo.io/edwards25519 1.1.0: filippo.io/edwards25519 MultiScalarMult produces invalid results or undefined behavior if receiver is not the identity
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@go.mod` at line 36, The transitive advisories for filippo.io/edwards25519
(GO-2026-4503) and github.com/aws/aws-sdk-go (GO-2022-0635, GO-2022-0646) are
misclassified; update vulnerability metadata and remediation: in go.mod bump
filippo.io/edwards25519 to v1.1.1+ to address GO-2026-4503, and for
github.com/aws/aws-sdk-go identify where the v1 module is pulled (search for
aws-sdk-go imports and any S3 Crypto usage) then either remove/eliminate S3
Crypto usage or plan a migration to AWS SDK Go V2 (no v1 patch exists) and mark
the aws-sdk-go advisory as HIGH/CRITICAL until migration is complete. Ensure
references to these advisories (GO-2026-4503, GO-2022-0635, GO-2022-0646) and
the modules (filippo.io/edwards25519, github.com/aws/aws-sdk-go) are updated in
your security tracking or SBOM entries.
Owner
Author
There was a problem hiding this comment.
Both are transitive/indirect dependencies pulled in by Dolt's embedded driver:
filippo.io/edwards25519 v1.1.0(GO-2026-4503): Low severity, affectsMultiScalarMultedge case. We don't use this API directly. Will bump to v1.1.1 in a dep-update PR.github.com/aws/aws-sdk-go v1.50.16(GO-2022-0635/0646): S3 Crypto padding-oracle. hookwise has zero S3 usage — this is purely a Dolt transitive dep. No v1 patch exists; the fix is migrating to AWS SDK v2 which is on Dolt's roadmap, not ours. Risk is effectively zero for this codebase.
Will track the edwards25519 bump as a low-priority dep update. The aws-sdk-go issue is upstream and out of our control.
There was a problem hiding this comment.
Rate Limit Exceeded
@vishnujayvel have exceeded the limit for the number of chat messages per hour. Please wait 1 minutes and 49 seconds before sending another message.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
cmd/hookwise/main.go (3)
408-418:⚠️ Potential issue | 🔴 CriticalRead
session/costfrom the configured analytics DB.Dispatch writes analytics to
config.Analytics.DBPath, but this block still opensanalytics.Open(""). Any non-defaultdb_pathwill make the status line query a different database, sosessionandcoststay stale or empty.♻️ Minimal fix
- if db, err := analytics.Open(""); err == nil { + if db, err := analytics.Open(config.Analytics.DBPath); err == nil {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/hookwise/main.go` around lines 408 - 418, The status line currently opens the default analytics DB via analytics.Open("") which ignores configured DB path; change the Open call to use the configured path (analytics.Open(config.Analytics.DBPath)) so the DailySummary query uses the same DB the dispatcher writes to (keep the surrounding logic using analytics.NewAnalytics and a.DailySummary and preserve the fail-open behavior for dailySummary).
107-110:⚠️ Potential issue | 🟠 MajorThe analytics write can still be lost on exit.
go recordAnalytics(...)is untracked, so the lateros.Exitcan terminate the process before Dolt finishesOpen/Commit. The fixed sleep below only makes this probabilistic, not deterministic. Please add a bounded flush/wait path here so analytics either completes or times out before exit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/hookwise/main.go` around lines 107 - 110, The analytics goroutine launched with go recordAnalytics(eventType, payload, config.Analytics.DBPath) is untracked and can be killed by a subsequent os.Exit; change this to a bounded flush/wait by tracking the work (e.g., add a sync.WaitGroup or pass a context with timeout) and wait for completion before exiting. Update the call site to either call a new blocking helper like recordAnalyticsWithContext(ctx, ...) or increment a WaitGroup before starting the goroutine and defer/wait with a timeout prior to exit; also modify recordAnalytics (or add a wrapper) to accept a context or to signal completion (channel/WaitGroup.Done) and respect cancellation so the wait is bounded. Ensure the check still only runs when config.Analytics.Enabled && payload.SessionID != "" and that the wait times out deterministically to avoid blocking shutdown indefinitely.
640-657:⚠️ Potential issue | 🟡 MinorHide the project segment when the fallback envelope has no repo name.
ProjectProducer.fallbackResult()returns empty strings when git metadata is unavailable. This renderer turns that back into a visible"project"segment, so missing data still occupies space as if it were real.♻️ Minimal fix
func renderProjectSegment(feedCache map[string]interface{}) string { data := feedData(feedCache, "project") if data == nil { return "" } - name := "project" - if n, ok := data["name"].(string); ok && n != "" { - name = n + name, _ := data["name"].(string) + if name == "" { + return "" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/hookwise/main.go` around lines 640 - 657, renderProjectSegment currently defaults name to "project" when feedData (from ProjectProducer.fallbackResult) provides no repo name, which makes the placeholder visible; change the function to return an empty string when data["name"] is missing or empty instead of using the literal "project". Specifically, in renderProjectSegment (which calls feedData), stop assigning name := "project" and instead check data["name"] as the presence gate—if it's not a non-empty string return "" immediately; keep the branch suffix logic only when a real name exists so no placeholder segment is rendered when ProjectProducer.fallbackResult supplies empty strings.internal/feeds/builtin.go (1)
450-455:⚠️ Potential issue | 🟡 MinorDon't mark malformed
datevalues as all-day events.When
time.Parse("2006-01-02", edt.Date)fails, this branch still returnsallDay=true. Downstream code then treats invalid calendar data as a real all-day event.♻️ Minimal fix
if edt.Date != "" { t, err := time.Parse("2006-01-02", edt.Date) if err != nil { - return time.Time{}, true + return time.Time{}, false } return t, true }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/feeds/builtin.go` around lines 450 - 455, The parsing branch for edt.Date currently treats parse failures as all-day events by returning (time.Time{}, true); change it so when time.Parse("2006-01-02", edt.Date) returns an error you return a zero time with allDay=false instead (i.e., return time.Time{}, false) so malformed date strings are not interpreted as valid all-day events; locate the logic around edt.Date and the time.Parse call and update the error branch accordingly (you may optionally add a debug log there).cmd/hookwise/cli_test.go (1)
421-1259: 🛠️ Refactor suggestion | 🟠 MajorUse
assert/requirein the new CLI tests.The new cases in this range still use
t.Fatal*/t.Error*directly, includingwriteJSONFile. Please convert them torequire.*andassert.*so the file matches the rest of the suite and the repository’s test convention.As per coding guidelines, "
**/*_test.go: Use testify/assert and testify/require for assertions, not stdlib testing alone."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/hookwise/cli_test.go` around lines 421 - 1259, Tests in this file use stdlib t.Fatal/t.Fatalf/t.Error/t.Errorf and helper writeJSONFile which uses t.Fatalf; convert these to testify assertions to match project convention: import "github.com/stretchr/testify/require" and "github.com/stretchr/testify/assert", replace setup/checks that should stop the test (os.Getwd/Chdir os.WriteFile executeCommand analytics.Open etc.) with require.* (e.g. require.NoError/require.NotNil) inside functions like TestStatusLineWithFeedCache, TestStatusLineWithMissingCache, TestStatusLineMultiLine, TestRecordAnalytics_SessionStart, TestRecordAnalytics_PostToolUse and helper writeJSONFile, and replace expectation checks inside render tests (e.g. checks on stripped content in TestStatusLineWeatherSegment, TestStatusLineProjectSegment, TestStatusLineInsightsSegment, TestStatusLineInsightsFrictionSegment, TestStatusLineInsightsPaceSegment, TestStatusLineInsightsTrendSegment, TestFormatLargeNumber, etc.) with assert.* (e.g. assert.Contains/assert.Equal/assert.Empty) so failures are non-fatal where appropriate; update writeJSONFile to use require.NoError when marshaling/writing, and ensure imports are added/organized accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/hookwise/main.go`:
- Around line 790-836: Both renderers treat a zeroed "no-data" insights envelope
as a healthy signal; fetch total_sessions from the insights payload (e.g.
sessions := toInt(data["total_sessions"])) and if sessions == 0 return "" before
any formatting. Modify renderInsightsFrictionSegment and
renderInsightsPaceSegment to call toInt(data["total_sessions"]) early (before
using recentFriction/frictionTotal or computing msgsPerDay/lines/sessions) and
bail out when zero so no positive-looking string is emitted for empty envelopes.
In `@internal/feeds/builtin.go`:
- Around line 61-90: The ProjectProducer.Produce function currently uses
os.Getwd() and caches a single global project snapshot, causing stale project
info when the daemon serves multiple repos; fix by deriving the
repository-specific data at render time or by namespacing the feed cache with a
stable project identifier (e.g., repoRoot or its absolute path) so cached
entries are keyed per repository; update Produce (and any cache read/write code)
to call runGit(ctx, cwd, ...) using the caller's cwd or include repoRoot in the
cache key when storing/reading the feed so ProjectProducer returns repo-specific
metadata instead of a global value.
---
Duplicate comments:
In `@cmd/hookwise/cli_test.go`:
- Around line 421-1259: Tests in this file use stdlib
t.Fatal/t.Fatalf/t.Error/t.Errorf and helper writeJSONFile which uses t.Fatalf;
convert these to testify assertions to match project convention: import
"github.com/stretchr/testify/require" and "github.com/stretchr/testify/assert",
replace setup/checks that should stop the test (os.Getwd/Chdir os.WriteFile
executeCommand analytics.Open etc.) with require.* (e.g.
require.NoError/require.NotNil) inside functions like
TestStatusLineWithFeedCache, TestStatusLineWithMissingCache,
TestStatusLineMultiLine, TestRecordAnalytics_SessionStart,
TestRecordAnalytics_PostToolUse and helper writeJSONFile, and replace
expectation checks inside render tests (e.g. checks on stripped content in
TestStatusLineWeatherSegment, TestStatusLineProjectSegment,
TestStatusLineInsightsSegment, TestStatusLineInsightsFrictionSegment,
TestStatusLineInsightsPaceSegment, TestStatusLineInsightsTrendSegment,
TestFormatLargeNumber, etc.) with assert.* (e.g.
assert.Contains/assert.Equal/assert.Empty) so failures are non-fatal where
appropriate; update writeJSONFile to use require.NoError when
marshaling/writing, and ensure imports are added/organized accordingly.
In `@cmd/hookwise/main.go`:
- Around line 408-418: The status line currently opens the default analytics DB
via analytics.Open("") which ignores configured DB path; change the Open call to
use the configured path (analytics.Open(config.Analytics.DBPath)) so the
DailySummary query uses the same DB the dispatcher writes to (keep the
surrounding logic using analytics.NewAnalytics and a.DailySummary and preserve
the fail-open behavior for dailySummary).
- Around line 107-110: The analytics goroutine launched with go
recordAnalytics(eventType, payload, config.Analytics.DBPath) is untracked and
can be killed by a subsequent os.Exit; change this to a bounded flush/wait by
tracking the work (e.g., add a sync.WaitGroup or pass a context with timeout)
and wait for completion before exiting. Update the call site to either call a
new blocking helper like recordAnalyticsWithContext(ctx, ...) or increment a
WaitGroup before starting the goroutine and defer/wait with a timeout prior to
exit; also modify recordAnalytics (or add a wrapper) to accept a context or to
signal completion (channel/WaitGroup.Done) and respect cancellation so the wait
is bounded. Ensure the check still only runs when config.Analytics.Enabled &&
payload.SessionID != "" and that the wait times out deterministically to avoid
blocking shutdown indefinitely.
- Around line 640-657: renderProjectSegment currently defaults name to "project"
when feedData (from ProjectProducer.fallbackResult) provides no repo name, which
makes the placeholder visible; change the function to return an empty string
when data["name"] is missing or empty instead of using the literal "project".
Specifically, in renderProjectSegment (which calls feedData), stop assigning
name := "project" and instead check data["name"] as the presence gate—if it's
not a non-empty string return "" immediately; keep the branch suffix logic only
when a real name exists so no placeholder segment is rendered when
ProjectProducer.fallbackResult supplies empty strings.
In `@internal/feeds/builtin.go`:
- Around line 450-455: The parsing branch for edt.Date currently treats parse
failures as all-day events by returning (time.Time{}, true); change it so when
time.Parse("2006-01-02", edt.Date) returns an error you return a zero time with
allDay=false instead (i.e., return time.Time{}, false) so malformed date strings
are not interpreted as valid all-day events; locate the logic around edt.Date
and the time.Parse call and update the error branch accordingly (you may
optionally add a debug log there).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 679e25c3-a511-4e0b-8351-7d2bfd0cab6c
📒 Files selected for processing (4)
cmd/hookwise/cli_test.gocmd/hookwise/main.gointernal/feeds/builtin.gointernal/feeds/feeds_test.go
Comment on lines
+61
to
+90
| func (p *ProjectProducer) Produce(ctx context.Context) (interface{}, error) { | ||
| cwd, err := os.Getwd() | ||
| if err != nil { | ||
| return p.fallbackResult() | ||
| } | ||
|
|
||
| // Detect git repo root. | ||
| repoRoot, err := p.runGit(ctx, cwd, "rev-parse", "--show-toplevel") | ||
| if err != nil { | ||
| // Not a git repo or git not installed — fail-open (ARCH-1). | ||
| return p.fallbackResult() | ||
| } | ||
|
|
||
| repoName := filepath.Base(repoRoot) | ||
|
|
||
| // Get current branch. | ||
| branchName, err := p.runGit(ctx, cwd, "rev-parse", "--abbrev-ref", "HEAD") | ||
| if err != nil { | ||
| branchName = "" | ||
| } | ||
|
|
||
| // Get short commit hash. | ||
| commitHash, err := p.runGit(ctx, cwd, "rev-parse", "--short", "HEAD") | ||
| if err != nil { | ||
| commitHash = "" | ||
| } | ||
|
|
||
| // Detect dirty state. | ||
| porcelain, err := p.runGit(ctx, cwd, "status", "--porcelain") | ||
| isDirty := err == nil && porcelain != "" |
There was a problem hiding this comment.
Project metadata is being cached globally, not per repository.
This producer snapshots whatever repo the daemon process happens to be in via os.Getwd(), but the status line later reads a single shared feed cache from the global state dir for every prompt render. If the user switches repos, the project segment will keep showing the repo where the daemon started. This needs to be computed from the caller's cwd at render time, or the cache needs to be keyed by project path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/feeds/builtin.go` around lines 61 - 90, The ProjectProducer.Produce
function currently uses os.Getwd() and caches a single global project snapshot,
causing stale project info when the daemon serves multiple repos; fix by
deriving the repository-specific data at render time or by namespacing the feed
cache with a stable project identifier (e.g., repoRoot or its absolute path) so
cached entries are keyed per repository; update Produce (and any cache
read/write code) to call runGit(ctx, cwd, ...) using the caller's cwd or include
repoRoot in the cache key when storing/reading the feed so ProjectProducer
returns repo-specific metadata instead of a global value.
Owner
Author
There was a problem hiding this comment.
Valid concern. The status-line command is invoked per-prompt with --project-dir pointing to the caller's cwd, but the daemon runs in a fixed directory. Currently, ProjectProducer calls os.Getwd() each time (no caching), so it gets fresh data each poll — but it's the daemon's cwd, not the user's.
In practice this works because:
- The daemon is typically started from the project directory
- The
status-linecommand reads from feed cache, which the daemon writes from its cwd
For multi-repo support, we'd need the daemon to accept project-dir context per cache write, or compute project info at render time (in status-line) instead of in the daemon. This is a design improvement beyond the scope of this PR — will track it for follow-up.
There was a problem hiding this comment.
Rate Limit Exceeded
@vishnujayvel have exceeded the limit for the number of chat messages per hour. Please wait 1 minutes and 32 seconds before sending another message.
- Use config.Analytics.DBPath instead of analytics.Open("") (Critical)
- Add context timeout (30s) to CalendarProducer API call
- Fix allDay=true on date parse error → return false
- Guard zero-session insights from rendering "✅ No friction"
- Fix top_tools type assertion panic risk in feeds_test.go
- Make ARTIFACT_DIR absolute path in test_terminal_e2e.py
- Fix markdown lint issues in retro doc (code fences, headings)
- Pass dbPath to renderNotificationSegment
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 10
♻️ Duplicate comments (3)
internal/feeds/builtin.go (2)
346-354:⚠️ Potential issue | 🟠 MajorThe new 30s timeout never reaches the Calendar API request.
Wrapping
Produce()incontext.WithTimeoutdoes not automatically bind that context to the generated Google call. As written,eventsCall.Do()can still block past 30s on a slow Calendar response unless the call itself is givenctx.Does `google.golang.org/api/calendar/v3` require calling `EventsListCall.Context(ctx)` for per-request cancellation/timeouts, or is the context passed to `calendar.NewService(...)` propagated automatically to `Events.List(...).Do()`?🩹 Minimal fix
eventsCall := p.svc.Events.List(calendarID). TimeMin(timeMin). TimeMax(timeMax). SingleEvents(true). OrderBy("startTime"). MaxResults(20) - events, err := eventsCall.Do() + events, err := eventsCall.Context(ctx).Do()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/feeds/builtin.go` around lines 346 - 354, The new 30s timeout isn't applied because the context created around Produce() isn't bound to the Google Calendar request; modify the code that builds the request (the Events.List call, i.e. the variable eventsCall) to attach the context via eventsCall = eventsCall.Context(ctx) (or call Events.List(...).Context(ctx) before calling eventsCall.Do()), so the per-request cancellation/timeout from Produce()/ctx is honored when invoking eventsCall.Do(); ensure you use the same ctx used for context.WithTimeout and pass that into the EventsList Call chain.
61-90:⚠️ Potential issue | 🟠 MajorProject metadata is still cached globally, not per repository.
Produce()snapshotsos.Getwd()inside the daemon and writes one sharedproject.json, whilestatus-line --project-dir ...later reads that same global feed cache. Switching repos will show whichever repository the daemon last polled, not the repo being rendered.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/feeds/builtin.go` around lines 61 - 90, Produce() currently snapshots os.Getwd() and writes a single shared project feed, causing a global cache; change Produce in ProjectProducer to derive and use the repository-specific path (use the detected repoRoot or a provided p.projectDir) as the cache key or filename when emitting the feed instead of a single global name, so each repository gets its own project.json; update any write/read logic that uses the global cache key to incorporate repoRoot (or repoName + branch/commit) so status-line --project-dir reads the matching per-repo feed.cmd/hookwise/cli_test.go (1)
274-277: 🛠️ Refactor suggestion | 🟠 MajorSwitch these added assertions to
assert/require.The new cases and
writeJSONFilehelper still rely ont.Fatal*/t.Error*, which keeps the file on a mixed assertion style instead of the repo-standard testify flow.As per coding guidelines, "Use testify/assert and testify/require for assertions, not stdlib testing alone."
Also applies to: 421-1259
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/hookwise/cli_test.go` around lines 274 - 277, Replace stdlib testing assertions in cmd/hookwise/cli_test.go with testify-style assertions: change the t.Errorf/t.Fatal* checks (e.g., the status-line empty check using t.Errorf("status-line should produce output, got empty")) to assert/require calls (for example require.NotEmpty(t, strings.TrimSpace(output), "status-line should produce output")). Also update the new test cases and the writeJSONFile helper to use assert/require instead of t.Error*/t.Fatal* so the file follows the repo-standard testify flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/hookwise/main.go`:
- Around line 180-183: The call to a.EndSession uses an empty
analytics.SessionStats{}, wiping metrics; instead compute and populate a
SessionStats instance with real values (total_tool_calls, file_edits_count,
ai_authored_lines, human_verified_lines, estimated_cost_usd and any cost segment
fields) before calling EndSession in the core.EventSessionEnd / core.EventStop
branch. Locate the case handling (the switch block with core.EventSessionEnd,
core.EventStop) and replace the literal analytics.SessionStats{} with a variable
built from the session/payload or existing in-memory tracker (e.g., stats :=
analytics.SessionStats{TotalToolCalls: ..., FileEditsCount: ...,
AIAuthoredLines: ..., HumanVerifiedLines: ..., EstimatedCostUSD: ...,
CostSegment: ...}; if needed read values from payload.SessionID, session store,
or analytics collector) and pass that stats variable into a.EndSession.
In `@docs/retro-status-line-bugs-50.md`:
- Around line 85-90: Update the "New Tests Added" list to include the calendar
producer and CLI rendering tests referenced in the PR summary: add entries for
the 10 unit tests for the calendar producer (e.g., TestCalendarProducer_* or the
actual test group name used in the repo) and the 3 CLI rendering tests for
status-line calendar rendering, and note that they use UTC dates like the
analytics tests; edit the section that currently lists only
TestPersistAcrossConnections, TestRecordAnalytics_SessionStart, and
TestRecordAnalytics_PostToolUse to include these calendar-related test names so
the retrospective accurately inventories all new tests.
- Line 19: Update the B4 line for project/calendar to reflect that the PR
implemented a working Google Calendar integration (OAuth support, event
handling, and 10 unit tests) rather than stating producers return placeholders;
edit the text referencing "project/calendar" and "producers" so it distinguishes
the prior state (returned placeholder/source: \"placeholder\") from the new
state introduced by this PR (real Google Calendar integration with OAuth, event
syncing, and tests), and ensure the retrospective notes both what was broken
before and what this PR fixed.
- Line 83: Update the B4 status line in docs/retro-status-line-bugs-50.md to
reflect that the calendar producer was implemented rather than "No code fix";
replace the current text for **B4:** with a brief summary noting the Google
Calendar integration was added with OAuth support, event handling, caching, and
10 unit tests (mentioning "calendar producer implemented" and listing those
features), and remove the "producers need real implementations" claim so the doc
matches the PR scope.
- Around line 129-130: The checklist entry "Implement real calendar producer
(Google Calendar MCP)" in the retro notes is stale; either mark that checkbox as
completed or remove the line entirely. Locate the line containing the exact text
"Implement real calendar producer (Google Calendar MCP)" in
docs/retro-status-line-bugs-50.md and update the checklist to reflect completion
(change "- [ ]" to "- [x]") or delete the bullet so the retrospective only lists
true future action items.
In `@internal/feeds/feeds_test.go`:
- Around line 757-763: The switch handling data["top_tools"] uses stdlib
t.Fatalf in the default branch, breaking the repo's testify-only pattern;
replace the t.Fatalf call with require.FailNowf to keep consistency (use the
same message/format and arguments). Update the default branch where the switch
on data["top_tools"] occurs to call require.FailNowf(t, "top_tools has
unexpected type %T", data["top_tools"]) instead of t.Fatalf, ensuring you import
testify/require if not already referenced in this test file.
In `@tui/tests/test_terminal_e2e.py`:
- Around line 1207-1219: The test test_status_line_has_calendar_data assumes
real calendar events but CalendarProducer can fail-open to empty events on CI;
update the test to gate it on real calendar availability by checking for a valid
calendar token / availability (or presence of upcoming events) before asserting,
and skip the test when none is present, or alternatively seed/mock
CalendarProducer to return deterministic events; locate the logic in
test_status_line_has_calendar_data and the CalendarProducer usage to implement
the conditional skip or injection of a mock provider.
- Around line 185-242: The test test_status_line_has_real_data currently has a
stale strict xfail (pytest.mark.xfail(..., strict=True)) and still parses legacy
segment labels/placeholder semantics; remove the strict=True (or remove the
xfail entirely if the feature is resolved) from the pytest.mark.xfail decorator
on test_status_line_has_real_data and update the assertion logic that inspects
status_line (the regex pattern and segment list
["session","cost","project","calendar"]) to match the current renderer contract
(use the new segment names emitted by _find_status_line/renderer and the current
placeholder/token for missing data instead of looking only for "--"); keep using
terminal_session.create_session and session.wait_for_stable, but adjust the
regex pattern and segment list to the renderer's current output so the test no
longer xpasses for a correct implementation.
- Around line 560-573: The code marks any dead session as ActorState.COMPLETED
unconditionally, which hides real failures; instead, after detecting not
session.is_alive, examine session.get_screen_text() for crash/fatal indicators
(or reuse whatever existing failure-detection logic you have) and set the
PollResult.state to ActorState.FAILED when a crash is found, only using
ActorState.COMPLETED when the final screen indicates a clean exit; preserve
existing behavior of recording to self._final_results, self._results,
pending.discard(actor_id) and calling self._on_completion(result).
- Around line 96-110: The _find_status_line function only matches old
"session:"/"cost:" labels; update it to detect the new status-line forms by
returning any stripped line that contains the fallback "hookwise" or any of the
status keywords without requiring a colon (e.g., "project", "calendar",
"session", "cost") or the typical segment separator " | "; modify the matching
logic in _find_status_line to check for these tokens (as substrings/whole words)
and return the first non-empty stripped line, and apply the same updated
detection wherever _find_status_line is duplicated/used to ensure tests tolerate
empty-summary omissions and label format changes.
---
Duplicate comments:
In `@cmd/hookwise/cli_test.go`:
- Around line 274-277: Replace stdlib testing assertions in
cmd/hookwise/cli_test.go with testify-style assertions: change the
t.Errorf/t.Fatal* checks (e.g., the status-line empty check using
t.Errorf("status-line should produce output, got empty")) to assert/require
calls (for example require.NotEmpty(t, strings.TrimSpace(output), "status-line
should produce output")). Also update the new test cases and the writeJSONFile
helper to use assert/require instead of t.Error*/t.Fatal* so the file follows
the repo-standard testify flow.
In `@internal/feeds/builtin.go`:
- Around line 346-354: The new 30s timeout isn't applied because the context
created around Produce() isn't bound to the Google Calendar request; modify the
code that builds the request (the Events.List call, i.e. the variable
eventsCall) to attach the context via eventsCall = eventsCall.Context(ctx) (or
call Events.List(...).Context(ctx) before calling eventsCall.Do()), so the
per-request cancellation/timeout from Produce()/ctx is honored when invoking
eventsCall.Do(); ensure you use the same ctx used for context.WithTimeout and
pass that into the EventsList Call chain.
- Around line 61-90: Produce() currently snapshots os.Getwd() and writes a
single shared project feed, causing a global cache; change Produce in
ProjectProducer to derive and use the repository-specific path (use the detected
repoRoot or a provided p.projectDir) as the cache key or filename when emitting
the feed instead of a single global name, so each repository gets its own
project.json; update any write/read logic that uses the global cache key to
incorporate repoRoot (or repoName + branch/commit) so status-line --project-dir
reads the matching per-repo feed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1e851abc-b429-469b-a4f9-6165c16e2055
📒 Files selected for processing (6)
cmd/hookwise/cli_test.gocmd/hookwise/main.godocs/retro-status-line-bugs-50.mdinternal/feeds/builtin.gointernal/feeds/feeds_test.gotui/tests/test_terminal_e2e.py
Comment on lines
+180
to
+183
| case core.EventSessionEnd, core.EventStop: | ||
| if err := a.EndSession(ctx, payload.SessionID, now, analytics.SessionStats{}); err != nil { | ||
| core.Logger().Error("analytics: end session", "error", err) | ||
| } |
There was a problem hiding this comment.
Populate SessionStats before ending the session.
EndSession(..., analytics.SessionStats{}) writes zeros into total_tool_calls, file_edits_count, ai_authored_lines, human_verified_lines, and estimated_cost_usd. The new daily-summary-backed cost segment and stats output therefore never reflect the real session, even after a successful SessionEnd/Stop.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/hookwise/main.go` around lines 180 - 183, The call to a.EndSession uses
an empty analytics.SessionStats{}, wiping metrics; instead compute and populate
a SessionStats instance with real values (total_tool_calls, file_edits_count,
ai_authored_lines, human_verified_lines, estimated_cost_usd and any cost segment
fields) before calling EndSession in the core.EventSessionEnd / core.EventStop
branch. Locate the case handling (the switch block with core.EventSessionEnd,
core.EventStop) and replace the literal analytics.SessionStats{} with a variable
built from the session/payload or existing in-memory tracker (e.g., stats :=
analytics.SessionStats{TotalToolCalls: ..., FileEditsCount: ...,
AIAuthoredLines: ..., HumanVerifiedLines: ..., EstimatedCostUSD: ...,
CostSegment: ...}; if needed read values from payload.SessionID, session store,
or analytics collector) and pass that stats variable into a.EndSession.
| | **B1: session: --** | No session count displayed | `dispatch` never called `analytics.StartSession()` — the analytics API was built but never wired to the dispatch pipeline | Integration | | ||
| | **B2: cost: --** | No cost displayed | Same as B1 — $0.00 because no sessions recorded | Integration | | ||
| | **B3: weather missing** | Weather segment not shown | `hookwise.yaml` segments list omitted `weather` despite weather feed being enabled and producing real data | Config | | ||
| | **B4: project/calendar: --** | Always shows `--` | Producers return `source: "placeholder"` — correctly filtered but ships as "done" | Design debt | |
There was a problem hiding this comment.
Clarify B4 status — calendar producer was implemented in this PR.
Line 19 states project/calendar producers return placeholders, but the PR objectives indicate that this PR actually implemented a real Google Calendar integration with OAuth support, event handling, and 10 unit tests. The retrospective should distinguish between what was broken before vs. what this PR fixed.
📝 Suggested clarification
-| **B4: project/calendar: --** | Always shows `--` | Producers return `source: "placeholder"` — correctly filtered but ships as "done" | Design debt |
+| **B4: project/calendar: --** | Always shows `--` | Producers return `source: "placeholder"` — correctly filtered but ships as "done" (calendar fixed in this PR, project tracked separately) | Design debt |📝 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
| | **B4: project/calendar: --** | Always shows `--` | Producers return `source: "placeholder"` — correctly filtered but ships as "done" | Design debt | | |
| | **B4: project/calendar: --** | Always shows `--` | Producers return `source: "placeholder"` — correctly filtered but ships as "done" (calendar fixed in this PR, project tracked separately) | Design debt | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/retro-status-line-bugs-50.md` at line 19, Update the B4 line for
project/calendar to reflect that the PR implemented a working Google Calendar
integration (OAuth support, event handling, and 10 unit tests) rather than
stating producers return placeholders; edit the text referencing
"project/calendar" and "producers" so it distinguishes the prior state (returned
placeholder/source: \"placeholder\") from the new state introduced by this PR
(real Google Calendar integration with OAuth, event syncing, and tests), and
ensure the retrospective notes both what was broken before and what this PR
fixed.
| 1. **B1/B2:** Added `recordAnalytics()` function in dispatch — writes SessionStart/PostToolUse/SessionEnd to Dolt with commit | ||
| 2. **B3:** Added `weather` to `hookwise.yaml` segments list and global config | ||
| 3. **B5:** Changed all `time.Now().Format("2006-01-02")` to `time.Now().UTC().Format("2006-01-02")` in status-line and stats commands to match analytics' UTC storage | ||
| 4. **B4:** No code fix — producers need real implementations (tracked separately) |
There was a problem hiding this comment.
Update B4 fix status to reflect calendar producer implementation.
The PR objectives state that a real Google Calendar integration was implemented in this PR with OAuth support, event handling, caching, and 10 unit tests. Stating "No code fix — producers need real implementations" contradicts the actual scope of this PR.
📝 Suggested update
-4. **B4:** No code fix — producers need real implementations (tracked separately)
+4. **B4:** Implemented real CalendarProducer with Google Calendar OAuth integration (project producer tracked separately)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/retro-status-line-bugs-50.md` at line 83, Update the B4 status line in
docs/retro-status-line-bugs-50.md to reflect that the calendar producer was
implemented rather than "No code fix"; replace the current text for **B4:** with
a brief summary noting the Google Calendar integration was added with OAuth
support, event handling, caching, and 10 unit tests (mentioning "calendar
producer implemented" and listing those features), and remove the "producers
need real implementations" claim so the doc matches the PR scope.
Comment on lines
+85
to
+90
| ## New Tests Added | ||
|
|
||
| 1. `TestPersistAcrossConnections` — validates Dolt data survives close+reopen (analytics package) | ||
| 2. `TestRecordAnalytics_SessionStart` — validates dispatch→Dolt→DailySummary pipeline | ||
| 3. `TestRecordAnalytics_PostToolUse` — validates event recording pipeline | ||
| 4. All use UTC dates to match production behavior |
There was a problem hiding this comment.
Include calendar producer tests in "New Tests Added" section.
The PR objectives mention "10 unit tests for calendar producer, 3 CLI rendering tests for status-line calendar rendering," but this section only lists analytics-related tests. A retrospective's test inventory should be comprehensive.
📝 Suggested addition
## New Tests Added
1. `TestPersistAcrossConnections` — validates Dolt data survives close+reopen (analytics package)
2. `TestRecordAnalytics_SessionStart` — validates dispatch→Dolt→DailySummary pipeline
3. `TestRecordAnalytics_PostToolUse` — validates event recording pipeline
-4. All use UTC dates to match production behavior
+4. 10 unit tests for `CalendarProducer` — validates OAuth flow, event parsing, caching, fail-open behavior
+5. 3 CLI rendering tests for status-line calendar segment
+6. All analytics tests use UTC dates to match production behavior🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/retro-status-line-bugs-50.md` around lines 85 - 90, Update the "New
Tests Added" list to include the calendar producer and CLI rendering tests
referenced in the PR summary: add entries for the 10 unit tests for the calendar
producer (e.g., TestCalendarProducer_* or the actual test group name used in the
repo) and the 3 CLI rendering tests for status-line calendar rendering, and note
that they use UTC dates like the analytics tests; edit the section that
currently lists only TestPersistAcrossConnections,
TestRecordAnalytics_SessionStart, and TestRecordAnalytics_PostToolUse to include
these calendar-related test names so the retrospective accurately inventories
all new tests.
Comment on lines
+129
to
+130
| - [ ] Implement real project producer (git repo detection) | ||
| - [ ] Implement real calendar producer (Google Calendar MCP) |
There was a problem hiding this comment.
Mark calendar producer action item as completed or remove it.
The action item "Implement real calendar producer (Google Calendar MCP)" appears to have been completed in this PR according to the PR objectives. Retrospectives should clearly distinguish between work completed in the current PR and future work.
📝 Suggested update
## Action Items
-- [ ] Implement real project producer (git repo detection)
-- [ ] Implement real calendar producer (Google Calendar MCP)
+- [x] Implement real calendar producer (Google Calendar OAuth integration) — completed in this PR
+- [ ] Implement real project producer (git repo detection)
- [ ] Add e2e smoke test for status-line pipeline
- [ ] Add UTC-edge-case timestamp to analytics test suite
- [ ] Consider adding a CI job that runs `hookwise status-line` with a test config and validates non-empty output📝 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
| - [ ] Implement real project producer (git repo detection) | |
| - [ ] Implement real calendar producer (Google Calendar MCP) | |
| - [x] Implement real calendar producer (Google Calendar OAuth integration) — completed in this PR | |
| - [ ] Implement real project producer (git repo detection) | |
| - [ ] Add e2e smoke test for status-line pipeline | |
| - [ ] Add UTC-edge-case timestamp to analytics test suite | |
| - [ ] Consider adding a CI job that runs `hookwise status-line` with a test config and validates non-empty output |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/retro-status-line-bugs-50.md` around lines 129 - 130, The checklist
entry "Implement real calendar producer (Google Calendar MCP)" in the retro
notes is stale; either mark that checkbox as completed or remove the line
entirely. Locate the line containing the exact text "Implement real calendar
producer (Google Calendar MCP)" in docs/retro-status-line-bugs-50.md and update
the checklist to reflect completion (change "- [ ]" to "- [x]") or delete the
bullet so the retrospective only lists true future action items.
Comment on lines
+757
to
+763
| switch tt := data["top_tools"].(type) { | ||
| case []map[string]interface{}: | ||
| assert.LessOrEqual(t, len(tt), 10, "top_tools should be limited to 10") | ||
| case []interface{}: | ||
| assert.LessOrEqual(t, len(tt), 10, "top_tools should be limited to 10") | ||
| default: | ||
| t.Fatalf("top_tools has unexpected type %T", data["top_tools"]) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n internal/feeds/feeds_test.go | sed -n '750,770p'Repository: vishnujayvel/hookwise
Length of output: 999
🏁 Script executed:
head -30 internal/feeds/feeds_test.go | grep -E "^import|require|assert"Repository: vishnujayvel/hookwise
Length of output: 150
🏁 Script executed:
rg "require\.(Fail|FailNow|FailNowf)" internal/feeds/feeds_test.go -A 2Repository: vishnujayvel/hookwise
Length of output: 47
🏁 Script executed:
rg "require\." internal/feeds/feeds_test.go | head -20Repository: vishnujayvel/hookwise
Length of output: 763
🏁 Script executed:
rg "require\.Fail" . -t goRepository: vishnujayvel/hookwise
Length of output: 47
🏁 Script executed:
web_search
testify require.FailNowf signatureRepository: vishnujayvel/hookwise
Length of output: 161
🌐 Web query:
testify require.FailNowf Go test assertion
💡 Result:
In Testify, the require package is the “fail fast” version of assert: it has (almost) the same API, but on failure it calls t.FailNow() (instead of t.Fail()), so the current test/subtest stops immediately. [1][2]
What to use
- Stop the test immediately on failure: use
require.*(orrequire.FailNow). - Record a failure but keep going: use
assert.*(or check the returnedbool). [2]
Example
func TestThing(t *testing.T) {
require.NoError(t, err) // if err != nil: calls t.FailNow(); code below won't run
require.Equal(t, 42, got)
}Under the hood note
FailNow() ends the test by exiting the goroutine (via runtime.Goexit()), so it’s not something you can recover() from like a panic. [3]
Sources:
[1] Testify require docs (calls t.FailNow() on failure) (tessl.io)
[2] pkg.go.dev for testify (assert returns bool; require relates to FailNow) (pkg.go.dev)
[3] Stack Overflow discussion noting require.* uses t.FailNow() → runtime.Goexit() (stackoverflow.com)
Citations:
- 1: https://tessl.io/registry/tessl/golang-github.1485827954.workers.dev-stretchr--testify/1.11.0/files/docs/require.md?utm_source=openai
- 2: https://pkg.go.dev/github.com/stretchr/testify?utm_source=openai
- 3: https://stackoverflow.com/questions/77700807/handling-assertion-in-golang-in-a-similar-way-to-panic?utm_source=openai
🏁 Script executed:
rg "require\.Fail" . -t go --max-count 5Repository: vishnujayvel/hookwise
Length of output: 47
🏁 Script executed:
python3 << 'EOF'
# Check testify require package API
import subprocess
result = subprocess.run(['rg', 'require\\.(Fail|FailNow)', '.', '-t', 'go'],
capture_output=True, text=True)
if result.returncode == 0:
print("Found matches:")
print(result.stdout)
else:
print("No matches found - checking testify API online...")
# Since no matches, the function may not be used in this repo
print("Checking if FailNowf is valid testify method...")
EOFRepository: vishnujayvel/hookwise
Length of output: 162
🏁 Script executed:
# Search testify usage patterns in test files
rg "require\.[A-Z]" internal/feeds/feeds_test.go -o | sort -u | head -20Repository: vishnujayvel/hookwise
Length of output: 114
🌐 Web query:
testify Go require Fail Fatalf equivalent documentation
💡 Result:
In testify, the require package is the “fatal” equivalent of Go’s testing.T failures: it has the same API as assert, but stops the test immediately by calling t.FailNow() on failure. [1]
So, instead of t.Fatalf(...), you typically write a require.*f(...) assertion (e.g., require.NoErrorf, require.Equalf, etc.). [1]
If you specifically want a direct “fail now with formatted message” helper, use:
require.FailNow(t, failureMessage, msgAndArgs...)[2]require.FailNowf(t, failureMessage, msg, args...)[2]
Docs: github.com/stretchr/testify/require on pkg.go.dev (see FailNowf, FailNow, etc.). [2]
Sources
[1] Testify require package docs (require.md / overview) (tessl.io)
[2] pkg.go.dev API reference for github.com/stretchr/testify/require (functions FailNow, FailNowf, etc.) (pkg.go.dev)
Citations:
- 1: https://tessl.io/registry/tessl/golang-github.1485827954.workers.dev-stretchr--testify/1.11.0/files/docs/require.md
- 2: https://pkg.go.dev/github.com/stretchr/testify/require
🏁 Script executed:
rg "t\.Fatalf" internal/feeds/feeds_test.go -nRepository: vishnujayvel/hookwise
Length of output: 134
🏁 Script executed:
# Check if there are other stdlib testing helpers in the file
rg "t\.(Fatal|Error|Fail|Errorf|Logf|Skip)" internal/feeds/feeds_test.go -cRepository: vishnujayvel/hookwise
Length of output: 66
🏁 Script executed:
rg "t\.(Fatal|Error|Fail|Errorf|Logf|Skip)" internal/feeds/feeds_test.go -nRepository: vishnujayvel/hookwise
Length of output: 205
Replace t.Fatalf with require.FailNowf to maintain testify-only assertion pattern.
Lines 759 and 761 correctly use assert.LessOrEqual, but line 763 drops back to stdlib t.Fatalf. Using require.FailNowf keeps the test consistent with the repo's testify-only convention per coding guidelines.
🔧 Minimal fix
default:
- t.Fatalf("top_tools has unexpected type %T", data["top_tools"])
+ require.FailNowf(t, "unexpected top_tools type", "%T", data["top_tools"])
}📝 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
| switch tt := data["top_tools"].(type) { | |
| case []map[string]interface{}: | |
| assert.LessOrEqual(t, len(tt), 10, "top_tools should be limited to 10") | |
| case []interface{}: | |
| assert.LessOrEqual(t, len(tt), 10, "top_tools should be limited to 10") | |
| default: | |
| t.Fatalf("top_tools has unexpected type %T", data["top_tools"]) | |
| switch tt := data["top_tools"].(type) { | |
| case []map[string]interface{}: | |
| assert.LessOrEqual(t, len(tt), 10, "top_tools should be limited to 10") | |
| case []interface{}: | |
| assert.LessOrEqual(t, len(tt), 10, "top_tools should be limited to 10") | |
| default: | |
| require.FailNowf(t, "unexpected top_tools type", "%T", data["top_tools"]) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/feeds/feeds_test.go` around lines 757 - 763, The switch handling
data["top_tools"] uses stdlib t.Fatalf in the default branch, breaking the
repo's testify-only pattern; replace the t.Fatalf call with require.FailNowf to
keep consistency (use the same message/format and arguments). Update the default
branch where the switch on data["top_tools"] occurs to call require.FailNowf(t,
"top_tools has unexpected type %T", data["top_tools"]) instead of t.Fatalf,
ensuring you import testify/require if not already referenced in this test file.
Comment on lines
+96
to
+110
| def _find_status_line(screen_text: str) -> str | None: | ||
| """Search full screen for the hookwise status line. | ||
|
|
||
| Looks for the characteristic segment pattern (e.g. "session:" or "cost:") | ||
| anywhere in the screen, since the render position depends on terminal | ||
| size and Claude Code's layout. | ||
|
|
||
| Returns the matching line or None. | ||
| """ | ||
| for line in screen_text.split("\n"): | ||
| stripped = line.strip() | ||
| # The status line contains segment labels separated by " | " | ||
| if "session:" in stripped or "cost:" in stripped: | ||
| return stripped | ||
| return None |
There was a problem hiding this comment.
These status-line E2Es still look for the pre-PR segment format.
_find_status_line() only recognizes session:/cost: lines, but fresh installs now omit those segments when the summary is empty, and project/calendar no longer render as project:/calendar: labels at all. A correctly rendered line with weather/project/calendar data—or just the "hookwise" fallback—will fail both tests.
Also applies to: 151-178
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tui/tests/test_terminal_e2e.py` around lines 96 - 110, The _find_status_line
function only matches old "session:"/"cost:" labels; update it to detect the new
status-line forms by returning any stripped line that contains the fallback
"hookwise" or any of the status keywords without requiring a colon (e.g.,
"project", "calendar", "session", "cost") or the typical segment separator " |
"; modify the matching logic in _find_status_line to check for these tokens (as
substrings/whole words) and return the first non-empty stripped line, and apply
the same updated detection wherever _find_status_line is duplicated/used to
ensure tests tolerate empty-summary omissions and label format changes.
Comment on lines
+185
to
+242
| @pytest.mark.xfail( | ||
| reason=( | ||
| "Some feed producers still return placeholder data. " | ||
| "Tracked by: #57 (project), #58 (calendar), #59 (session/cost). " | ||
| "Insights producer is fixed — see test_status_line_has_insights_data." | ||
| ), | ||
| strict=True, | ||
| ) | ||
| def test_status_line_has_real_data(self, terminal_session): | ||
| """E2E: ALL status line segments must show real data, not '--' placeholders. | ||
|
|
||
| This test validates the feature end-to-end: from feed producers | ||
| through the cache pipeline to the rendered status line inside | ||
| Claude Code. It fails when producers return placeholder data, | ||
| because from the user's perspective the feature is broken. | ||
|
|
||
| Currently xfail because some feed producers are stubs: | ||
| - #57: ProjectProducer returns placeholder | ||
| - #58: CalendarProducer returns placeholder | ||
| - #59: session/cost analytics not wired to Dolt | ||
|
|
||
| Note: InsightsProducer is fixed and tested separately via | ||
| test_status_line_has_insights_data. | ||
| """ | ||
| session = terminal_session.create_session( | ||
| "claude", | ||
| rows=24, | ||
| cols=120, | ||
| env=_clean_claude_env(), | ||
| cwd=_PROJECT_ROOT, | ||
| ) | ||
| try: | ||
| session.wait_for_stable(timeout=60, idle_ms=3000) | ||
|
|
||
| screen = session.get_screen_text() | ||
| status_line = _find_status_line(screen) | ||
|
|
||
| assert status_line is not None, ( | ||
| f"Status line not found on screen.\n" | ||
| f"Full screen:\n{screen}" | ||
| ) | ||
|
|
||
| placeholder_segments = [] | ||
| for seg_name in ("session", "cost", "project", "calendar"): | ||
| pattern = rf"{seg_name}:\s*([^|]+)" | ||
| match = re.search(pattern, status_line) | ||
| if match: | ||
| value = match.group(1).strip() | ||
| if value == "--": | ||
| placeholder_segments.append(seg_name) | ||
|
|
||
| assert len(placeholder_segments) == 0, ( | ||
| f"Segments with placeholder '--' data: {placeholder_segments}\n" | ||
| f"Status line: {status_line}\n" | ||
| f"The feature is broken end-to-end — feed producers are not " | ||
| f"providing real data.\n" | ||
| f"Fix: #57 (project), #58 (calendar), #59 (session/cost)" | ||
| ) |
There was a problem hiding this comment.
Drop the stale strict xfail and rewrite this around the current renderer.
This PR is exactly what resolves the placeholder/stub behavior listed in the decorator, so strict=True now turns a correct implementation into an XPASS failure. The body also still parses project:/calendar: labels and -- placeholders from the old contract.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tui/tests/test_terminal_e2e.py` around lines 185 - 242, The test
test_status_line_has_real_data currently has a stale strict xfail
(pytest.mark.xfail(..., strict=True)) and still parses legacy segment
labels/placeholder semantics; remove the strict=True (or remove the xfail
entirely if the feature is resolved) from the pytest.mark.xfail decorator on
test_status_line_has_real_data and update the assertion logic that inspects
status_line (the regex pattern and segment list
["session","cost","project","calendar"]) to match the current renderer contract
(use the new segment names emitted by _find_status_line/renderer and the current
placeholder/token for missing data instead of looking only for "--"); keep using
terminal_session.create_session and session.wait_for_stable, but adjust the
regex pattern and segment list to the renderer's current output so the test no
longer xpasses for a correct implementation.
Comment on lines
+560
to
+573
| # Skip sessions that have exited | ||
| if not session.is_alive: | ||
| result = PollResult( | ||
| actor_id=actor_id, | ||
| state=ActorState.COMPLETED, | ||
| screen_text=session.get_screen_text(), | ||
| matched_pattern="process_exited", | ||
| ) | ||
| self._final_results[actor_id] = result | ||
| self._results.append(result) | ||
| pending.discard(actor_id) | ||
| if self._on_completion: | ||
| self._on_completion(result) | ||
| continue |
There was a problem hiding this comment.
An exited actor is not automatically COMPLETED.
This branch reports any dead process as success before checking the final screen for a crash or fatal error. A session that dies because of a real failure will trigger on_completion and mask the error path these helpers are supposed to surface.
🩹 Minimal fix
if not session.is_alive:
- result = PollResult(
- actor_id=actor_id,
- state=ActorState.COMPLETED,
- screen_text=session.get_screen_text(),
- matched_pattern="process_exited",
- )
+ screen_text = session.get_screen_text()
+ error_match = detect_error(screen_text)
+ result = PollResult(
+ actor_id=actor_id,
+ state=ActorState.ERROR if error_match else ActorState.COMPLETED,
+ screen_text=screen_text,
+ matched_pattern=None if error_match else "process_exited",
+ error_detail=error_match,
+ )
self._final_results[actor_id] = result🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tui/tests/test_terminal_e2e.py` around lines 560 - 573, The code marks any
dead session as ActorState.COMPLETED unconditionally, which hides real failures;
instead, after detecting not session.is_alive, examine session.get_screen_text()
for crash/fatal indicators (or reuse whatever existing failure-detection logic
you have) and set the PollResult.state to ActorState.FAILED when a crash is
found, only using ActorState.COMPLETED when the final screen indicates a clean
exit; preserve existing behavior of recording to self._final_results,
self._results, pending.discard(actor_id) and calling
self._on_completion(result).
Comment on lines
+1207
to
+1219
| @skipif_no_hookwise | ||
| def test_status_line_has_calendar_data(self): | ||
| """hookwise status-line output should contain calendar emoji or relative time.""" | ||
| result = subprocess.run( | ||
| ["hookwise", "status-line", "--project-dir", _PROJECT_ROOT], | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=10, | ||
| ) | ||
| # Strip ANSI escape codes for clean matching. | ||
| output = re.sub(r"\x1b\[[0-9;]*m", "", result.stdout) | ||
| assert "\U0001f4c5" in output or re.search(r"in \d+[mh]", output), ( | ||
| f"No calendar data in status-line output:\n{output}" |
There was a problem hiding this comment.
Gate this E2E on real calendar availability.
CalendarProducer intentionally fail-opens to empty events when the token is missing or there are no upcoming meetings. On a clean machine or CI, that expected state becomes a hard failure here unless the test is explicitly skipped or seeded with deterministic calendar data.
🧰 Tools
🪛 Ruff (0.15.4)
[error] 1210-1210: subprocess call: check for execution of untrusted input
(S603)
[error] 1211-1211: Starting a process with a partial executable path
(S607)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tui/tests/test_terminal_e2e.py` around lines 1207 - 1219, The test
test_status_line_has_calendar_data assumes real calendar events but
CalendarProducer can fail-open to empty events on CI; update the test to gate it
on real calendar availability by checking for a valid calendar token /
availability (or presence of upcoming events) before asserting, and skip the
test when none is present, or alternatively seed/mock CalendarProducer to return
deterministic events; locate the logic in test_status_line_has_calendar_data and
the CalendarProducer usage to implement the conditional skip or injection of a
mock provider.
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
SegmentConfig.UnmarshalYAMLfor string shorthand in configCalendar Producer Details
~/.hookwise/calendar-token.json(Python google-auth format) and constructs Gooauth2.Config+oauth2.Tokencalendar.Serviceacross poll cycles (not re-created every 300s)datevsdateTime), computesis_current, generates relative time strings ("in 15m", "tomorrow 3pm")Calendars[0]from config when set, falls back to"primary"Test plan
go test -race ./internal/feeds/... -run TestCalendar— 10 unit tests (no-token, parses events, finds next event, all-day, relative time, fallback, no-placeholder, ConfigAware)go test -race ./cmd/hookwise/... -run TestStatusLineCalendar— 3 CLI rendering tests (map format, empty, string format)go test -race ./...— full suite passes (onlydaggersetup fails, pre-existing)hookwise status-linerenders📅 RAG mock interview (KDR/Vishnu) in 15mfrom real Google Calendar🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation