fix: audit fixes — atomic decisions, shell quoting, dead config cleanup, docs (v0.8.1)#151
fix: audit fixes — atomic decisions, shell quoting, dead config cleanup, docs (v0.8.1)#151
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 54 minutes and 50 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (16)
📝 WalkthroughWalkthroughThis PR removes TUI and VSCode extension documentation, eliminates configuration options for voice, GitHub, Telegram, calendar, Google, briefing, and cost reporting, and updates architecture documentation to reflect an MCP server-centric design. It also includes internal improvements: transactional decision recording, shell-safe hook initialization, and FTS term truncation logging. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~28 minutes Possibly related PRs
Poem
🚥 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 |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
internal/mcpinit/init_test.go (1)
171-191: Nice coverage — consider adding an empty-string case.The cases cover the tricky ones (spaces,
$, backticks, embedded single quotes). One small addition worth considering: an empty input, sinceshellQuote("")should yield"''"(a valid empty POSIX word) and that path is otherwise untested.{"/path/it's/ghost", "'/path/it'\\''s/ghost'"}, +{"", "''"},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/mcpinit/init_test.go` around lines 171 - 191, Add an empty-string test to TestShellQuote to cover shellQuote("") returning "''": update the tests slice in TestShellQuote (in internal/mcpinit/init_test.go) to include {input: "", want: "''"} so the empty POSIX-word case is asserted alongside the existing cases for shellQuote.internal/memory/store.go (1)
984-986: Use the store's configured logger rather than the globalslogdefault.Every other log call in
Storeusess.logger(which is injected viaNewStore), but this new warning calls the package-levelslog.Warn. That bypasses the handler/level the caller configured (e.g., theio.Discardhandler used inmcpinit.importMemories), so truncation will show up in stderr even when callers explicitly silence the store. SincesanitizeFTSis a free function, consider making it a method on*Storeor threading the logger through.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/memory/store.go` around lines 984 - 986, The warn call in sanitizeFTS uses the package-level slog.Warn which bypasses the store's configured logger; make sanitizeFTS either a method on *Store (e.g., func (s *Store) sanitizeFTS(...)) or add a logger param so it can call s.logger.Warn (or logger.Warn) instead of slog.Warn; update callers (where sanitizeFTS is invoked) to use the new method/signature and ensure the Store created via NewStore continues to supply the proper s.logger.internal/config/config.go (1)
147-156: Nit: single-entryenvOverridesmap.Now that only
GHOST_SERVER_AUTH_TOKENremains, the map + loop is a bit ceremonious. A direct check reads cleaner:-envOverrides := map[string]string{ - "GHOST_SERVER_AUTH_TOKEN": "server.auth_token", -} -for envKey, koanfKey := range envOverrides { - if val := os.Getenv(envKey); val != "" { - _ = k.Load(confmap.Provider(map[string]interface{}{ - koanfKey: val, - }, "."), nil) - } -} +if val := os.Getenv("GHOST_SERVER_AUTH_TOKEN"); val != "" { + _ = k.Load(confmap.Provider(map[string]interface{}{ + "server.auth_token": val, + }, "."), nil) +}Fine to keep the map form if you expect to re-add underscore-in-tag keys soon.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config/config.go` around lines 147 - 156, The envOverrides map and loop are now overkill for a single key; replace the map+for loop with a direct check for os.Getenv("GHOST_SERVER_AUTH_TOKEN") and, if non-empty, call k.Load with confmap.Provider(map[string]interface{}{"server.auth_token": val}, ".") (same as current call site), removing the envOverrides variable and loop; update the code that uses envOverrides, keeping the call to k.Load, confmap.Provider, and the key "server.auth_token" intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/architecture.md`:
- Around line 7-15: Multiple fenced code blocks in docs/architecture.md lack
language identifiers causing markdownlint MD040 warnings; update each unlabeled
triple-backtick fence (the blocks listing CLI commands, file listings like
cmd/ghost/main.go, architecture diagrams/text flows, and example command
snippets such as the ghost mcp list and ghost reflect examples) to include an
appropriate language tag (e.g., bash for shell/CLI samples, text for plain
text/diagrams) so all code fences are labeled and the linter warnings are
resolved.
- Around line 84-85: The documentation incorrectly states the SessionStart hook
receives a JSON field named projectPath; update the doc so the hook's stdin JSON
is described as containing cwd (required) and an optional source, and clarify
that lookupProject(db, cwd) uses cwd for path-prefix matching (with name
fallback) — ensure SessionStart, cwd, source, and lookupProject are referenced
precisely and remove or replace any mention of projectPath.
- Around line 67-70: The docs use shorthand like
"ghost_task_create/update/complete" which can be misinterpreted; update the
listing to use the exact MCP tool identifiers (e.g., replace
"ghost_task_create/update/complete" with the full, separate identifiers such as
"ghost_tasks_create", "ghost_tasks_update", "ghost_tasks_complete"), ensure
other entries use their exact names (e.g., "ghost_decision_record" matches the
implemented "ghost_decision_record" that calls store.RecordDecision(), and
"ghost_health" remains as the exact tool id used for store metadata queries),
and expand the ellipses so all 16 tools are enumerated by their exact MCP
identifiers to avoid failed calls.
In `@internal/mcpinit/init.go`:
- Around line 192-194: The status check is looking for the wrong literal ("ghost
hook session-start") so it won't match the quoted hookCmd produced by
shellQuote; update the call site that currently uses sf.hasHook("SessionStart",
"ghost hook session-start") to instead check for the same substring used when
creating the hook (sf.hasHook("SessionStart", "hook session-start")) so
idempotency holds; additionally add a regression test that runs ensureHook (with
ghostBin set to a path containing spaces) and then asserts
sf.hasHook("SessionStart", "hook session-start") returns true to prevent
regressions.
In `@README.md`:
- Around line 163-164: Update the "Task statuses" line to clarify these are
recommended conventions, not enforced transitions: change the phrasing around
"Task statuses: `pending` → `active` → `done`" to state these are the
typical/proposed progression and that the current implementation accepts any
valid status value without transition guards; mention that tasks can be set to
`blocked` from any non-done state and unblocked back to `active`, and add a
brief note pointing readers to the code/API (or say "implementation") for
enforcement details so users won't assume strict state-machine guarantees.
---
Nitpick comments:
In `@internal/config/config.go`:
- Around line 147-156: The envOverrides map and loop are now overkill for a
single key; replace the map+for loop with a direct check for
os.Getenv("GHOST_SERVER_AUTH_TOKEN") and, if non-empty, call k.Load with
confmap.Provider(map[string]interface{}{"server.auth_token": val}, ".") (same as
current call site), removing the envOverrides variable and loop; update the code
that uses envOverrides, keeping the call to k.Load, confmap.Provider, and the
key "server.auth_token" intact.
In `@internal/mcpinit/init_test.go`:
- Around line 171-191: Add an empty-string test to TestShellQuote to cover
shellQuote("") returning "''": update the tests slice in TestShellQuote (in
internal/mcpinit/init_test.go) to include {input: "", want: "''"} so the empty
POSIX-word case is asserted alongside the existing cases for shellQuote.
In `@internal/memory/store.go`:
- Around line 984-986: The warn call in sanitizeFTS uses the package-level
slog.Warn which bypasses the store's configured logger; make sanitizeFTS either
a method on *Store (e.g., func (s *Store) sanitizeFTS(...)) or add a logger
param so it can call s.logger.Warn (or logger.Warn) instead of slog.Warn; update
callers (where sanitizeFTS is invoked) to use the new method/signature and
ensure the Store created via NewStore continues to supply the proper s.logger.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b548f962-3160-436a-9516-904b86b83930
📒 Files selected for processing (16)
.gitignoreAUDIT_REPORT.mdAUDIT_SUMMARY.mdCLAUDE.mdREADME.mddocs/architecture.mddocs/tui.mddocs/vscode.mdinternal/config/config.example.yamlinternal/config/config.gointernal/config/config_test.gointernal/mcpinit/init.gointernal/mcpinit/init_test.gointernal/memory/decisions.gointernal/memory/store.gointernal/memory/store_test.go
💤 Files with no reviewable changes (5)
- docs/vscode.md
- internal/config/config_test.go
- AUDIT_SUMMARY.md
- AUDIT_REPORT.md
- docs/tui.md
| ``` | ||
| ghost Interactive bubbletea TUI (default) | ||
| ghost "query" One-shot mode (no TUI) | ||
| echo ... | ghost Pipe mode (stdin) | ||
| ghost serve HTTP daemon + subsystems | ||
| ghost mcp MCP server (stdio) | ||
| ghost mcp MCP server on stdio (used by Claude Code, Cursor, Goose) | ||
| ghost mcp init Configure Claude Code integration | ||
| ghost mcp status Health check | ||
| ghost hook session-start SessionStart hook (called by Claude Code) | ||
| ghost reflect <project> Manual memory consolidation | ||
| ghost upgrade Self-update from GitHub Releases | ||
| ghost version Print version | ||
| ``` |
There was a problem hiding this comment.
Add language tags to fenced code blocks to satisfy markdownlint MD040.
Current unlabeled fences trigger lint warnings and reduce rendering/tooling quality.
✏️ Suggested doc fix
-```
+```bash
ghost mcp MCP server on stdio (used by Claude Code, Cursor, Goose)
...
-```
+```
-```
+```text
cmd/ghost/main.go CLI entrypoint + subcommand dispatch
...
-```
+```
-```
+```text
Claude Code / Cursor → stdio JSON-RPC → mcpserver
...
-```
+```
-```
+```text
Claude Code session opens
...
-```
+```
-```
+```text
ghost reflect <project> --apply
...
-```
+```
-```
+```text
embedding.Worker goroutine:
...
-```
+```Also applies to: 19-54, 59-79, 82-89, 92-104, 108-119
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 7-7: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/architecture.md` around lines 7 - 15, Multiple fenced code blocks in
docs/architecture.md lack language identifiers causing markdownlint MD040
warnings; update each unlabeled triple-backtick fence (the blocks listing CLI
commands, file listings like cmd/ghost/main.go, architecture diagrams/text
flows, and example command snippets such as the ghost mcp list and ghost reflect
examples) to include an appropriate language tag (e.g., bash for shell/CLI
samples, text for plain text/diagrams) so all code fences are labeled and the
linter warnings are resolved.
| ghost_task_create/update/complete → store.CreateTask()... | ||
| ghost_decision_record → store.RecordDecision() | ||
| ghost_health → store metadata query | ||
| ... 16 tools total |
There was a problem hiding this comment.
Tool names should use the exact MCP identifiers, not shorthand.
Line 67’s ghost_task_create/update/complete can be interpreted as callable names, but implemented tools use explicit identifiers (e.g., ghost_tasks_update). Please list exact tool names to avoid failed calls.
✏️ Suggested doc fix
- ghost_task_create/update/complete → store.CreateTask()...
+ ghost_tasks_create / ghost_tasks_update / ghost_tasks_complete → store.CreateTask()...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/architecture.md` around lines 67 - 70, The docs use shorthand like
"ghost_task_create/update/complete" which can be misinterpreted; update the
listing to use the exact MCP tool identifiers (e.g., replace
"ghost_task_create/update/complete" with the full, separate identifiers such as
"ghost_tasks_create", "ghost_tasks_update", "ghost_tasks_complete"), ensure
other entries use their exact names (e.g., "ghost_decision_record" matches the
implemented "ghost_decision_record" that calls store.RecordDecision(), and
"ghost_health" remains as the exact tool id used for store metadata queries),
and expand the ellipses so all 16 tools are enumerated by their exact MCP
identifiers to avoid failed calls.
| → ghost hook session-start (stdin: JSON with cwd + projectPath) | ||
| → lookupProject(db, cwd) # path-prefix match OR name fallback |
There was a problem hiding this comment.
SessionStart input fields are documented inaccurately.
Line 84 mentions projectPath, but hook input parsing is based on cwd (and optional source). This should be corrected to keep the contract precise.
✏️ Suggested doc fix
- → ghost hook session-start (stdin: JSON with cwd + projectPath)
+ → ghost hook session-start (stdin: JSON with cwd; optional source)📝 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.
| → ghost hook session-start (stdin: JSON with cwd + projectPath) | |
| → lookupProject(db, cwd) # path-prefix match OR name fallback | |
| → ghost hook session-start (stdin: JSON with cwd; optional source) | |
| → lookupProject(db, cwd) # path-prefix match OR name fallback |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/architecture.md` around lines 84 - 85, The documentation incorrectly
states the SessionStart hook receives a JSON field named projectPath; update the
doc so the hook's stdin JSON is described as containing cwd (required) and an
optional source, and clarify that lookupProject(db, cwd) uses cwd for
path-prefix matching (with name fallback) — ensure SessionStart, cwd, source,
and lookupProject are referenced precisely and remove or replace any mention of
projectPath.
- Rewrite CLAUDE.md: remove TUI/orchestrator/tool/prompt/mode/project refs - Rewrite docs/architecture.md: reflect stripped package set and data flows - Rewrite config.example.yaml: remove telegram/google/github/voice/briefing sections - Delete stale docs/tui.md, docs/vscode.md, AUDIT_REPORT.md, AUDIT_SUMMARY.md
…, Telegram, Calendar, Google, Briefing, CostReport)
…rding, config whitespace
d5e5773 to
c2133be
Compare
Changes
RecordDecisiondual-INSERT in a single transaction — prevents zombie decisions if the memory row fails (return `("", err)` not `(id, err)` on second insert failure)Test plan
Summary by CodeRabbit
New Features
Documentation
Chores