Strict project-aware CodeRabbit review config#1
Conversation
…w rules Expands from 4 generic path instructions to 9 path groups encoding hookwise-specific invariants: three-phase separation, fail-open guarantee, first-match-wins guards, stateless feed producers, cache bus persistence, daemon PID lifecycle, and severity-categorized review output. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. Warning Ignoring CodeRabbit configuration file changes. For security, only the configuration from the base branch is applied for open source repositories. ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR updates the CodeRabbit configuration to enable the request_changes_workflow and replaces broad TypeScript review guidance with detailed, architecture- and path-specific review instructions (guards, context, side effects, validation, error handling, CLI/daemon/recipes/tests/docs expectations, and hookwise.yaml schema guidance). Changes
Sequence Diagram(s)Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.coderabbit.yaml:
- Around line 84-95: The .coderabbit.yaml currently applies two overlapping
rules to the same file: the explicit path entry "src/core/recipes.ts" and the
glob "src/core/*.ts" both match src/core/recipes.ts, causing duplicated/multiple
instruction sets; decide whether recipes.ts should receive both core invariants
and recipe-specific checks—if not, remove or rename the explicit entry or modify
the glob to exclude recipes.ts (e.g., change the glob or add a
negative/exclusion pattern) so only one rule applies; alternatively, if the
overlap is intentional, add a comment in .coderabbit.yaml clarifying that
"src/core/recipes.ts" should get both rule sets to avoid future confusion.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
.coderabbit.yamldocs/guide/creating-a-recipe.mddocs/guide/getting-started.mddocs/reference/hook-events.mddocs/reference/migration.mddocs/reference/tui-guide.md
Addresses CodeRabbit review feedback: src/core/recipes.ts intentionally matches both src/core/*.ts (core invariants) and its own explicit entry (recipe-specific rules). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Dolt's gozstd dependency requires CGO, which needs a platform-specific C toolchain unavailable on GitHub Actions Linux runners. The darwin/arm64 cross-compile step fails with undefined symbols from the zstd C library. Also adds timeout-minutes: 15 to prevent runaway builds (CR thread #1). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* feat: Go migration v2 — unified Dolt data layer, 15-batch PDLC Complete rewrite of hookwise from TypeScript to Go with Dolt embedded as the unified, version-controlled SQL data layer. What changed: - TypeScript → Go (single binary, 65µs dispatch latency) - SQLite + JSON files → Dolt embedded (branch, diff, commit, rollback) - Child process daemon → goroutine-based feed polling - React/Ink CLI → Cobra CLI (Python TUI preserved) Key packages: - internal/core: dispatcher, config, guards, types, state utilities - internal/analytics: Dolt-backed session/event tracking - internal/feeds: goroutine-per-feed daemon with 8 built-in producers - internal/bridge: Go→Python TUI cache bridge (FlattenForTUI) - internal/notifications: trigger-based notification platform - internal/upgrade: TypeScript→Go data migration tool - pkg/hookwise/testing: GuardTester + HookRunner testing API Testing: - 91% coverage (testing API), 82.5% (feeds) - 33 contract test fixtures across 13 event types - 10 integration tests, 10 benchmarks - 8 holdout scenarios all PASS PDLC: 15 batches, all DONE+CRITICS (ADVOCATE + SKEPTIC) Final Validator: PASS_WARN (both perspectives) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor: simplify — fix 15 issues from code review agents Code Reuse (4 HIGH fixed): - Remove exact duplicate `mapToHandlerResult` (use `actionToResult`) - Remove dead types: CostState, AuthorshipEntry, ToolBreakdownEntry, DailySummary, StatsResult from core/types.go (superseded by analytics) - Consolidate triplicated `parseTimeFlex` into core.ParseTimeFlex Code Quality (2 HIGH fixed): - SafeDispatch: use named returns so panic recovery sets fail-open result - Add action constants (ActionBlock/Warn/Confirm/Allow) and phase constants (PhaseGuard/Context/SideEffect), replace all raw strings Efficiency (4 P0/P1 fixed): - payloadToMap: direct struct→map instead of JSON marshal/unmarshal - Glob pattern caching via sync.Map (avoid recompilation per dispatch) - Regex caching via sync.Map for `matches` operator - Hoist inferPhase/phaseOrder maps to package level (avoid per-call alloc) Other: - Consolidate fireSideEffects/FireSideEffectsSync into shared runSideEffects - Replace custom minInt with Go 1.21+ builtin min() - Replace log.Printf with core.Logger() in feeds/polling.go Net: -55 lines, 11 files changed. All 9 packages pass with -race. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address 30 CodeRabbit review comments from PR #25 Code fixes (22 items): - stdin.go: add io.LimitReader(10MB) and improve malformed JSON warning - main.go: add 50ms grace period for side-effect goroutines before os.Exit - main.go: log MarkSurfaced errors instead of silently swallowing - custom.go: use Setpgid + process group kill for child processes - constants.go: warn on TempDir fallback when UserHomeDir unavailable - producer.go: add nil-map guard in Registry.Register - migration.go: return error instead of time.Now() fallback on parse failure - migration.go: defer Analytics handle creation to avoid nil DB in dry-run - builtin.go: add "source":"placeholder" marker to all stub producers - config.go: check type assertion result on InterpolateEnvVars - dispatcher.go: use json.Marshal for fallback JSON string escaping - state.go: log silent JSON unmarshal errors in coaching/cost state - state.go: log time parse fallback in ReadFeedCache - state.go: add TTL check to ReadAllFeedCache (consistent with ReadFeedCache) - bridge.go: add type validation for ttl_seconds field - producers.go: escape SQL LIKE wildcards in content substring - daemon.go: check f.Close() error after PID file write Test fixes (5 items): - dispatcher_test.go: replace no-op assert.True(t, true) with require.NotPanics - state_test.go: capture expected date before DB read (midnight race fix) - cli_test.go: strengthen ldflags test to assert exact version format - contract_test.go: fix operator extraction to match ops without spaces - integration_test.go: replace time.Sleep with require.Eventually polling - migration_test.go: fix fmtID to use fmt.Sprintf (supports >26 values) Doc updates (3 items): - boundaries.md: rewrite TypeScript paths to Go paths - tech-stack.md: rewrite TS/Node stack to Go/Dolt - roadmap.md: update pre-migration stack references Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(ci): remove CGO cross-compile steps, add timeout Dolt's gozstd dependency requires CGO, which needs a platform-specific C toolchain unavailable on GitHub Actions Linux runners. The darwin/arm64 cross-compile step fails with undefined symbols from the zstd C library. Also adds timeout-minutes: 15 to prevent runaway builds (CR thread #1). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Vishnu Jayavel <vishnu@Vishnus-MacBook-Air.local> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
… template - Add temperature_unit to weather config template (CR comment #1) - Use nil for temperature/weatherCode in fallback when no cached data exists, so TUI shows "--" instead of plausible-looking 0°F (CR comment #3) - Add WeatherTestFixture() shared fixture in feeds package; bridge test uses it instead of hardcoded keys, so producer field renames break tests (CR #2) - Use configured temperatureUnit in fallback (respects celsius config) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…d check (#31) * fix: comprehensive weather feed pipeline — real API, enabled check, cross-boundary tests Root cause: Weather feed had 5 independent break points preventing temperature from displaying in the TUI status line: 1. **BP1 — Daemon skips disabled feeds**: runAllFeeds() didn't check the Enabled flag, polling all registered producers regardless of config. Added isEnabled() check that skips disabled producers (fail-open for unknown feeds). 2. **BP2 — No weather config in default template**: The init command's YAML template had no weather section. Added commented weather config example with lat/lon/unit fields. 3. **BP3 — Placeholder producer with wrong field names**: WeatherProducer returned hardcoded 0°F with snake_case fields (unit, wind_speed) while Python TUI expects camelCase (temperatureUnit, windSpeed). Replaced with real Open-Meteo API integration using WMO weather codes, emoji mapping, and fallback caching. 4. **BP4 — ConfigAware interface**: Added ConfigAware interface so producers can receive FeedsConfig (latitude, longitude, temperature_unit) before producing. 5. **BP5 — Cross-boundary tests**: Added Go-side bridge test verifying FlattenForTUI preserves weather field names, plus 4 Python-side rendering tests validating the exact cache shape renders correctly in the TUI. Also added: isEnabled unit tests, disabled-feed skip test, fallbackResult for graceful degradation when API is unreachable. Closes #29 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address CodeRabbit review — shared fixture, nil fallback, config template - Add temperature_unit to weather config template (CR comment #1) - Use nil for temperature/weatherCode in fallback when no cached data exists, so TUI shows "--" instead of plausible-looking 0°F (CR comment #3) - Add WeatherTestFixture() shared fixture in feeds package; bridge test uses it instead of hardcoded keys, so producer field renames break tests (CR #2) - Use configured temperatureUnit in fallback (respects celsius config) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Vishnu Jayavel <vishnu@Vishnus-MacBook-Air.local> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
There is no Makefile — the repo uses Taskfile.yml. CLAUDE.md still told contributors to run `make install` (the load-bearing #1 Rule), which errors with "No rule to make target 'install'". Map every make command to its task equivalent (verified each target exists): make install → task install make dev → task dev make run ARGS="X" → task run -- X make test → task test:go:unit make test-integration → task test:go:integration make test-tui → task test:tui:unit make version-check → task staleness Also fix the SegmentConfig YAML example `- session` → `- cost` (session was removed in #129, so it would render nothing). Closes #128. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
doctor re-derived feed config from LoadConfig(cwd) plus its own parallel isFeedEnabled/getFeedInterval switches, so it could report a feed enabled/stale that the daemon (cold-started from a different project) was not actually polling. Expose the daemon's effective feed config over a new read-only GET /feeds socket route (builtins + customs, derived from the in-memory registry), add a DaemonClient.EffectiveFeeds() client method, and have doctor report against that runtime view — falling back to the on-disk global config, labeled, when the daemon is down (ARCH-1 fail-open). /health is untouched. Eliminate the doctor<->daemon duplication that was the root of the divergence: extract the enabled/interval logic into shared pure functions feeds.FeedEnabled / feeds.EffectiveIntervalSeconds that BOTH the daemon's poll-gate and doctor's fallback builder call, so they cannot disagree by construction. A test pins that doctor's per-feed verdict matches the daemon's EffectiveFeeds for the same config. Add two honesty warnings: a drift WARN when the daemon is polling a feed config that no longer matches the on-disk global config (config is read once at startup — restart to apply), and a back-compat WARN when a project hookwise.yaml carries a feeds: block that the singleton daemon ignores (#89), listing the keys to migrate to the global config. Migrate this repo's own redundant feeds block and update the init template + feeds guide to teach global feed config. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EjWqH4FMkdczkSagzXw3dQ
doctor re-derived feed config from LoadConfig(cwd) plus its own parallel isFeedEnabled/getFeedInterval switches, so it could report a feed enabled/stale that the daemon (cold-started from a different project) was not actually polling. Expose the daemon's effective feed config over a new read-only GET /feeds socket route (builtins + customs, derived from the in-memory registry), add a DaemonClient.EffectiveFeeds() client method, and have doctor report against that runtime view — falling back to the on-disk global config, labeled, when the daemon is down (ARCH-1 fail-open). /health is untouched. Eliminate the doctor<->daemon duplication that was the root of the divergence: extract the enabled/interval logic into shared pure functions feeds.FeedEnabled / feeds.EffectiveIntervalSeconds that BOTH the daemon's poll-gate and doctor's fallback builder call, so they cannot disagree by construction. A test pins that doctor's per-feed verdict matches the daemon's EffectiveFeeds for the same config. Add two honesty warnings: a drift WARN when the daemon is polling a feed config that no longer matches the on-disk global config (config is read once at startup — restart to apply), and a back-compat WARN when a project hookwise.yaml carries a feeds: block that the singleton daemon ignores (#89), listing the keys to migrate to the global config. Migrate this repo's own redundant feeds block and update the init template + feeds guide to teach global feed config. Claude-Session: https://claude.ai/code/session_01EjWqH4FMkdczkSagzXw3dQ Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
) The Status-tab cache-fallback preview rendered feed segments with no freshness check: _render_segment and _segment_has_data read cache entries directly, so a days-old calendar event kept rendering as current forever (blind spot #1 from scout hw-xthx). data.py's is_fresh() existed but had zero call sites in status.py. Route every cache read in status.py through a new _fresh_entry() helper that returns the entry only if is_fresh() passes (updated_at within ttl_seconds). Stale or malformed entries are treated as absent and render nothing, matching the Go status line's behavior (cmd_status_line.go feedData -> bridge.IsEnvelopeFresh), which also fails closed on a missing timestamp. No new stale-marker UI. This covers calendar and all sibling feed segments (weather, news, project, mantra, insights, builder_trap/mode_badge, session, etc.). Tests (validation tier V2): TestFreshnessGating in test_status_segments.py covers expired-entry-not-rendered, fresh-entry-unchanged, missing-freshness-fields-fails-closed, and _segment_has_data stale/fresh cases. Existing fixtures gained fresh updated_at/ttl_seconds since entries without them are now correctly treated as absent (FlattenForTUI always stamps both in production); the weather fixtures' hardcoded 2026-03-07 timestamps became dynamic for the same reason. Validated: 121 TUI unit tests, status snapshot, ruff, mypy strict all pass.
Summary
src/core/feeds/**,src/core/status-line/**,src/cli/**,docs/**,src/core/recipes.tsTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Chores
Documentation