feat(goose): session adapter and Open Plugins hook installer - #739
Conversation
Co-Authored-By: SageOx <ox@sageox.ai> SageOx-Session: https://sageox.ai/c/ses_019fb575-6925-70e6-ba98-971842aa4def
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded a bundled Goose adapter with SQLite transcript ingestion, incremental serving, Open Plugins lifecycle hooks, diagnostics, validation, detection, packaging, agent registration, and expanded compatibility documentation. ChangesGoose support
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant AdapterRuntime
participant GooseAdapter
participant SQLiteDatabase
participant OpenPlugins
AdapterRuntime->>GooseAdapter: Request session data
GooseAdapter->>SQLiteDatabase: Query session messages
SQLiteDatabase-->>GooseAdapter: Return parsed rows and offset
OpenPlugins->>GooseAdapter: Deliver lifecycle hook event
GooseAdapter-->>AdapterRuntime: Return session entries and updated offset
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Co-Authored-By: SageOx <ox@sageox.ai> SageOx-Session: https://sageox.ai/c/ses_019fb5b2-5493-755f-ab65-12ba567187ef
Co-Authored-By: SageOx <ox@sageox.ai> SageOx-Session: https://sageox.ai/c/ses_019fb5b2-5493-755f-ab65-12ba567187ef
![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
🧹 Nitpick comments (8)
cmd/ox-adapter-goose/main.go (1)
150-176: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
errors.Is(err, os.ErrNotExist)instead ofos.IsNotExist(err).Both existence checks in
handleDiagnoseuse the olderos.IsNotExist(err)idiom.♻️ Proposed fix
- if _, err := os.Stat(hooksPath); os.IsNotExist(err) { + if _, err := os.Stat(hooksPath); errors.Is(err, os.ErrNotExist) {and
- if _, err := os.Stat(manifestFilePath(p.RepoRoot, "project")); os.IsNotExist(err) { + if _, err := os.Stat(manifestFilePath(p.RepoRoot, "project")); errors.Is(err, os.ErrNotExist) {Based on learnings, "prefer
errors.Is(err, os.ErrNotExist)overos.IsNotExist(err)" for consistent error-unwrapping semantics across the codebase.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/ox-adapter-goose/main.go` around lines 150 - 176, Update both existence checks in handleDiagnose to use errors.Is(err, os.ErrNotExist) instead of os.IsNotExist(err), and add the required errors import if absent. Preserve the existing issue-reporting behavior for missing hooks.json and plugin.json.Source: Learnings
cmd/ox-adapter-goose/main_test.go (1)
1-79: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftMissing coverage for
handleDetect,handleImportSession, andhandleDiagnose.This file only tests
handleInfo's static metadata. The detection heuristics (env vars, DB path, config dir, PATH lookup), the SQLite-backed import path, and the diagnose issue-generation logic inmain.goare untested here.As per coding guidelines, "*.go test files should use table-driven tests, test error paths, and maintain at least 85% coverage."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/ox-adapter-goose/main_test.go` around lines 1 - 79, Add table-driven tests covering handleDetect’s environment, database path, config directory, and PATH lookup heuristics, plus success and error cases for handleImportSession and issue-generation behavior in handleDiagnose. Reuse existing test helpers and fixtures where available, and assert both returned values and errors; keep the existing handleInfo tests unchanged.Source: Coding guidelines
cmd/ox-adapter-goose/session_test.go (2)
301-363: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winGood coverage of the resolution precedence. One gap worth filling alongside the LIKE-escaping fix flagged in
session.go(Line 145-150): add a session whoseworking_diris e.g./repo/myXrepo/suband arepoRootof/repo/my_repo, which must not match.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/ox-adapter-goose/session_test.go` around lines 301 - 363, The session-resolution tests need a LIKE-pattern false-positive case. Extend the relevant TestResolveSessionID coverage with a session whose working_dir is /repo/myXrepo/sub and resolve using /repo/my_repo, asserting it does not match and produces the expected no-match error; keep the existing valid subdirectory behavior unchanged.
108-119: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the tool-entry fields, not just the count.
entryTextreturnsContent, which is empty for tool entries, so these two cases only prove "one entry appeared". The parts that can silently regress —ToolName,ToolInput,CallIDcorrelation between request and response, andIsError— are unchecked. Consider adding optionalwantToolName/wantCallIDfields to the table.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/ox-adapter-goose/session_test.go` around lines 108 - 119, Strengthen the table-driven tests around the tool request and tool response cases in the session test by asserting the parsed tool-entry fields rather than only wantEntries. Extend the test expectations with optional tool name, tool input, call ID, and error-state values, then validate them against each returned entry, including correlation of the request and response CallID.cmd/ox/release_notes.md (1)
12-12: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the canonical “AI coworker” terminology in new user-facing prose.
As per coding guidelines, replace generic
agent/agentswording withAI coworker/AI coworkers, while keeping literal command names such asox agent, filenames, and product names unchanged. Also replace “transcripts” in the release note with “session records” or “session data”;Transcriptis reserved for human-to-human voice discussions.
cmd/ox/release_notes.md#L12-L12: replace “every other agent” and “transcripts”.README.md#L48-L80: rename the table heading and prose references..sageox/README.md#L68-L75: update “agent above”.cmd/ox/init.go#L1342-L1349: apply the same wording in generated content.cmd/ox/integrate.go#L79-L81: replace “Other agents”.docs/guides/agent-compatibility.md#L16-L47: update the table and session-store prose.docs/guides/agent-compatibility.md#L131-L149: update limitation and version prose.cmd/ox/guides/agents-md.md#L9-L9: replace “AI coding agents”.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/ox/release_notes.md` at line 12, Update the user-facing terminology across all listed sites: use “AI coworker” or “AI coworkers” instead of generic “agent” or “agents,” and replace “transcripts” with “session records” or “session data,” while preserving literal commands, filenames, and product names. Apply the wording consistently in cmd/ox/release_notes.md:12, README.md:48-80, .sageox/README.md:68-75, cmd/ox/init.go:1342-1349, cmd/ox/integrate.go:79-81, docs/guides/agent-compatibility.md:16-47 and 131-149, and cmd/ox/guides/agents-md.md:9; update headings, prose, generated content, table text, limitations, and version references as applicable.Source: Coding guidelines
cmd/ox-adapter-goose/hooks_test.go (2)
231-233: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
errors.Is(err, os.ErrNotExist).Based on learnings from this repo, prefer
errors.Is(err, os.ErrNotExist)overos.IsNotExist(err)for Go 1.13+ unwrapping semantics and consistency.♻️ Proposed change
- if _, err := os.Stat(dir); !os.IsNotExist(err) { + if _, err := os.Stat(dir); !errors.Is(err, os.ErrNotExist) { t.Errorf("expected %s to be removed, stat err = %v", dir, err) }Add
errorsto the import block.Based on learnings: "prefer
errors.Is(err, os.ErrNotExist)overos.IsNotExist(err)".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/ox-adapter-goose/hooks_test.go` around lines 231 - 233, Update the directory-removal assertion around os.Stat to use errors.Is(err, os.ErrNotExist) instead of os.IsNotExist(err), and add the errors package to the imports. Preserve the existing test failure message and expectation that the directory is absent.Source: Learnings
118-123: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a user-scope install test with
t.Setenv("HOME", …).Only
hookCommandis exercised for user scope; thepluginDiruser branch (home lookup +$HOME/.agents/plugins/sageox) is never covered, which is where the discardedUserHomeDirerror inhooks.gohides.t.Setenv("HOME", t.TempDir())lets you install/check/uninstall in user scope safely without touching the real home directory.As per coding guidelines: "Prefer end-to-end reality over isolated unit testing, use table-driven tests, test error paths, and maintain at least 85% coverage."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/ox-adapter-goose/hooks_test.go` around lines 118 - 123, Add an end-to-end user-scope install/check/uninstall test alongside TestHookCommand_UserScopeOmitsRepoRoot, using t.Setenv("HOME", t.TempDir()) to exercise the pluginDir user branch in hooks.go. Verify the complete lifecycle and relevant filesystem behavior, and use table-driven coverage where appropriate, including the UserHomeDir error path if the existing APIs permit it.Source: Coding guidelines
cmd/ox-adapter-goose/hooks.go (1)
265-276: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDistinguish "missing" from "corrupt", and wrap the write errors.
loadHooksFilecollapses a malformed existing hooks.json into an empty file, so install silently overwrites a user's unparsable-but-real rules. Returning an error for parse failures (while still treatingos.ErrNotExistas empty) makes that case visible. Separately,writeJSONreturns bareMkdirAll/WriteFileerrors; wrapping them with the path gives callers actionable context.As per coding guidelines: "Use `errors.Is()` and `errors.As()` for error inspection and wrap errors with contextual information."♻️ Proposed refactor
-func loadHooksFile(path string) hooksFile { +func loadHooksFile(path string) (hooksFile, error) { hf := hooksFile{Hooks: map[string][]hookRule{}} data, err := os.ReadFile(path) //nolint:gosec // path derived from repo root + fixed plugin name if err != nil { - return hf + if errors.Is(err, os.ErrNotExist) { + return hf, nil + } + return hf, fmt.Errorf("failed to read %s: %w", path, err) } - if err := json.Unmarshal(data, &hf); err != nil || hf.Hooks == nil { - return hooksFile{Hooks: map[string][]hookRule{}} + if err := json.Unmarshal(data, &hf); err != nil { + return hf, fmt.Errorf("failed to parse %s: %w", path, err) } - return hf + if hf.Hooks == nil { + hf.Hooks = map[string][]hookRule{} + } + return hf, nil }func writeJSON(path string, v any) error { if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { - return err + return fmt.Errorf("failed to create %s: %w", filepath.Dir(path), err) } data, err := json.MarshalIndent(v, "", " ") if err != nil { - return err + return fmt.Errorf("failed to marshal %s: %w", path, err) } data = append(data, '\n') - return os.WriteFile(path, data, 0o644) //nolint:gosec // config file, not a secret + if err := os.WriteFile(path, data, 0o644); err != nil { //nolint:gosec // config file, not a secret + return fmt.Errorf("failed to write %s: %w", path, err) + } + return nil }Requires adding
errorsto the imports.Also applies to: 327-337
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/ox-adapter-goose/hooks.go` around lines 265 - 276, Update loadHooksFile to return an error alongside hooksFile: treat os.ErrNotExist from os.ReadFile as an empty hooks file, but propagate JSON parse failures with contextual wrapping instead of silently resetting rules. Update its callers to handle the new error contract. In writeJSON, wrap MkdirAll and WriteFile failures with the target path, using errors.Is/errors.As for error inspection as appropriate, and add the errors import.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/ox-adapter-goose/hooks.go`:
- Around line 117-124: Make pluginDir return (string, error), propagate
os.UserHomeDir failures, and reject an empty root before joining the plugin
path; update hooksFilePath and manifestFilePath to handle the new result. At
cmd/ox-adapter-goose/hooks.go:117-124, implement the root validation; at
:173-175, propagate the pluginDir error or guard empty project RepoRoot before
manifest stat; at :204-205, apply the same guard before reading hooks.json so
uninstall cannot RemoveAll a cwd-relative path.
In `@cmd/ox-adapter-goose/main.go`:
- Around line 95-126: Handle the error returned by readMetadata in
handleImportSession instead of discarding it; return a contextual error when
metadata loading fails, and only construct ImportSessionResult with the metadata
after a successful read.
In `@cmd/ox-adapter-goose/serve.go`:
- Around line 19-46: Wrap errors returned by handleFindSession in the
OnFindSession callback and handleReadFromOffset in the OnReadFromOffset callback
with operation-specific context using %w, while preserving the original causes
for inspection and leaving successful results unchanged.
In `@cmd/ox-adapter-goose/session.go`:
- Around line 391-393: Update the "toolResponse" branch to skip the block when
b.ToolResp is nil, matching the existing guard in the "toolRequest" branch. Only
call toolResponseFields and append adapterruntime.ToolResultWithID when a
payload is present, preserving correlation through b.ID for valid responses.
- Around line 248-261: Update the session read flow around readMessages and
maxMessageID so the watermark is derived from the maximum message ID actually
returned by readMessages, eliminating the separate maxMessageID query and
backwards clamp. Extend readMessages to select and return the relevant ID
alongside the entries, and update its callers to handle the additional return
value, using the returned ID as the new offset.
- Around line 145-150: The repository prefix used in the session query must be
escaped before constructing the LIKE pattern. Add or reuse an escapeLike helper
that neutralizes backslashes, percent signs, and underscores, then use the
escaped repoRoot for the wildcard argument while keeping the exact working_dir
argument unchanged.
- Around line 152-162: Update the session query construction around the since
filter to normalize updated_at to a consistent datetime representation before
applying the >= comparison, or bind since using Goose’s RFC3339 storage format.
Preserve existing ordering and limit behavior, and add coverage verifying
filtering works with both RFC3339 and YYYY-MM-DD HH:MM:SS stored values.
- Around line 92-98: Update the database-opening logic in the session database
helper to construct a properly escaped file: URI from dbPath, preserving mode=ro
and optionally adding query_only(true). Remove the journal_mode(WAL) pragma so
opening the Goose database cannot persistently modify sessions.db, while
retaining the existing error wrapping and return behavior.
In `@cmd/ox/guides/agents-md.md`:
- Line 9: Update the reader list in the AGENTS.md guidance to include Gemini CLI
and remove OpenAI Agents SDK, matching the project’s documented supported-reader
list and surrounding GEMINI.md guidance. Preserve the existing explanation that
AGENTS.md is the converged repository-root convention.
In `@cmd/ox/guides/getting-started.md`:
- Line 9: Use canonical “AI coworker” terminology in all three guide locations:
in cmd/ox/guides/getting-started.md:9 replace “any other supported coding agent”
with “any other supported AI coworker”; in cmd/ox/guides/murmur-vs-rule.md:40
replace “supported coding agent” similarly; and in
cmd/ox/guides/team-rules.md:16 rewrite “coding agents” and “agent-agnostic”
using “AI coworkers” wording while preserving the literal command “ox agent
prime”.
In `@cmd/ox/init.go`:
- Around line 1342-1346: Update the generated guidance in cmd/ox/init.go around
the agent integration list to distinguish SessionStart lifecycle hooks from
instruction-file marker-based initial priming, explicitly reflecting that Gemini
CLI and Droid use markers for priming despite having other hooks. Apply the same
wording to the checked-in copy in .sageox/README.md; both listed sites require
the corresponding guidance update.
In `@cmd/ox/release_notes.md`:
- Line 12: Update the Goose release-note heading in the documented support
statement to say “Silver-tier support” or “supported Goose integration” instead
of “fully supported,” while preserving the remaining implementation details and
the caveat about missing compaction-event recovery.
- Line 12: The release note’s comparison incorrectly claims every other agent
uses a single settings file. Update the Goose integration description to call
its directory-based approach ox’s first Open Plugins manifest/hooks directory,
while preserving the existing explanation of Goose’s plugin location and hook
behavior.
In `@docs/guides/agent-compatibility.md`:
- Around line 133-136: Rename the customer-facing OX_PROJECT_ROOT environment
variable to the appropriate SAGEOX_* name across the Goose adapter, project-root
discovery logic, tests, and documentation. Update every producer, consumer,
assertion, and export instruction consistently while preserving the existing
absolute repository-root behavior.
---
Nitpick comments:
In `@cmd/ox-adapter-goose/hooks_test.go`:
- Around line 231-233: Update the directory-removal assertion around os.Stat to
use errors.Is(err, os.ErrNotExist) instead of os.IsNotExist(err), and add the
errors package to the imports. Preserve the existing test failure message and
expectation that the directory is absent.
- Around line 118-123: Add an end-to-end user-scope install/check/uninstall test
alongside TestHookCommand_UserScopeOmitsRepoRoot, using t.Setenv("HOME",
t.TempDir()) to exercise the pluginDir user branch in hooks.go. Verify the
complete lifecycle and relevant filesystem behavior, and use table-driven
coverage where appropriate, including the UserHomeDir error path if the existing
APIs permit it.
In `@cmd/ox-adapter-goose/hooks.go`:
- Around line 265-276: Update loadHooksFile to return an error alongside
hooksFile: treat os.ErrNotExist from os.ReadFile as an empty hooks file, but
propagate JSON parse failures with contextual wrapping instead of silently
resetting rules. Update its callers to handle the new error contract. In
writeJSON, wrap MkdirAll and WriteFile failures with the target path, using
errors.Is/errors.As for error inspection as appropriate, and add the errors
import.
In `@cmd/ox-adapter-goose/main_test.go`:
- Around line 1-79: Add table-driven tests covering handleDetect’s environment,
database path, config directory, and PATH lookup heuristics, plus success and
error cases for handleImportSession and issue-generation behavior in
handleDiagnose. Reuse existing test helpers and fixtures where available, and
assert both returned values and errors; keep the existing handleInfo tests
unchanged.
In `@cmd/ox-adapter-goose/main.go`:
- Around line 150-176: Update both existence checks in handleDiagnose to use
errors.Is(err, os.ErrNotExist) instead of os.IsNotExist(err), and add the
required errors import if absent. Preserve the existing issue-reporting behavior
for missing hooks.json and plugin.json.
In `@cmd/ox-adapter-goose/session_test.go`:
- Around line 301-363: The session-resolution tests need a LIKE-pattern
false-positive case. Extend the relevant TestResolveSessionID coverage with a
session whose working_dir is /repo/myXrepo/sub and resolve using /repo/my_repo,
asserting it does not match and produces the expected no-match error; keep the
existing valid subdirectory behavior unchanged.
- Around line 108-119: Strengthen the table-driven tests around the tool request
and tool response cases in the session test by asserting the parsed tool-entry
fields rather than only wantEntries. Extend the test expectations with optional
tool name, tool input, call ID, and error-state values, then validate them
against each returned entry, including correlation of the request and response
CallID.
In `@cmd/ox/release_notes.md`:
- Line 12: Update the user-facing terminology across all listed sites: use “AI
coworker” or “AI coworkers” instead of generic “agent” or “agents,” and replace
“transcripts” with “session records” or “session data,” while preserving literal
commands, filenames, and product names. Apply the wording consistently in
cmd/ox/release_notes.md:12, README.md:48-80, .sageox/README.md:68-75,
cmd/ox/init.go:1342-1349, cmd/ox/integrate.go:79-81,
docs/guides/agent-compatibility.md:16-47 and 131-149, and
cmd/ox/guides/agents-md.md:9; update headings, prose, generated content, table
text, limitations, and version references as applicable.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 45679a4b-0734-48f9-a4d0-2582d6d39f55
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (34)
.config/goreleaser.yml.sageox/README.mdMakefileREADME.mdcmd/ox-adapter-goose/hooks.gocmd/ox-adapter-goose/hooks_test.gocmd/ox-adapter-goose/main.gocmd/ox-adapter-goose/main_test.gocmd/ox-adapter-goose/serve.gocmd/ox-adapter-goose/session.gocmd/ox-adapter-goose/session_test.gocmd/ox/doctor_adapter_blocks.gocmd/ox/doctor_agent.gocmd/ox/guides/agents-md.mdcmd/ox/guides/getting-started.mdcmd/ox/guides/murmur-vs-rule.mdcmd/ox/guides/team-rules.mdcmd/ox/init.gocmd/ox/instruction_files.gocmd/ox/instruction_files_test.gocmd/ox/integrate.gocmd/ox/release_notes.mdcmd/ox/upgrade.godocs/guides/agent-compatibility.mdgo.modinternal/adapter/registry.yamlinternal/adapter/registry_test.gointernal/prime/agent_type.gointernal/prime/conformance_test.gointernal/session/adapters/adapter.gointernal/session/adapters/external_symlink_test.gointernal/session/adapters/generic_jsonl_test.gointernal/session/adapters/session_roots.goscripts/install.sh
c6d8573 to
987c762
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
cmd/ox-adapter-goose/main.go (1)
95-126: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winStill discarding the metadata-read error in
handleImportSession.
meta, _ := readMetadata(db, p.SessionID)(line 120) silently swallows the error; a corrupted or partially-written metadata row will produce an emptyMetadatainstead of surfacing the failure.🩹 Proposed fix
- meta, _ := readMetadata(db, p.SessionID) + meta, err := readMetadata(db, p.SessionID) + if err != nil { + return nil, fmt.Errorf("reading session metadata: %w", err) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/ox-adapter-goose/main.go` around lines 95 - 126, Update handleImportSession to capture and handle the error returned by readMetadata. Return a contextual error when metadata loading fails, and only construct ImportSessionResult with the metadata after a successful read.cmd/ox-adapter-goose/hooks.go (1)
117-124: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUnfixed:
pluginDircan still resolve to a CWD-relative path, andhandleUninstallHookswillRemoveAllit.
os.UserHomeDir()'s error is discarded on line 120, and onlyhandleInstallHooks(137-139) requires a non-emptyRepoRootfor project scope —handleCheckHooksandhandleUninstallHooksdon't. If either is invoked with project scope and an emptyRepoRoot(or user scope with$HOMEunset),pluginDirreturns.agents/plugins/sageoxrelative to the process's CWD, and the destructiveos.RemoveAll(dir)at line 237 would delete that CWD-relative directory instead of erroring out.🐛 Proposed fix
-func pluginDir(repoRoot, scope string) string { - root := repoRoot - if scope == scopeUser { - home, _ := os.UserHomeDir() - root = home - } - return filepath.Join(root, ".agents", "plugins", pluginName) -} +func pluginDir(repoRoot, scope string) (string, error) { + root := repoRoot + if scope == scopeUser { + home, err := os.UserHomeDir() + if err != nil { + return "", fmt.Errorf("resolving user home: %w", err) + } + root = home + } else if root == "" { + return "", fmt.Errorf("repo root is required for scope %q", scope) + } + return filepath.Join(root, ".agents", "plugins", pluginName), nil +}Propagate the new
(string, error)throughhooksFilePath/manifestFilePathand their callers inhandleCheckHooksandhandleUninstallHooks.Also applies to: 173-175, 204-205
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/ox-adapter-goose/hooks.go` around lines 117 - 124, Update pluginDir to return (string, error) and reject project scope with an empty repoRoot or user scope when os.UserHomeDir fails or returns empty, rather than producing a relative path. Propagate this error through hooksFilePath and manifestFilePath, then handle it in handleCheckHooks and handleUninstallHooks before any filesystem access, especially the destructive os.RemoveAll call.cmd/ox-adapter-goose/serve.go (1)
19-48: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winStill not wrapping delegated handler errors with context.
Lines 22 and 43 return
handleFindSession/handleReadFromOffseterrors unchanged. As per coding guidelines, "wrap errors with contextual information."🩹 Proposed fix
srv.OnFindSession(func(ctx context.Context, p adapterprotocol.FindSessionParams) (*adapterprotocol.FindSessionResult, error) { result, err := handleFindSession(p) if err != nil { - return nil, err + return nil, fmt.Errorf("find Goose session: %w", err) } @@ result, err := handleReadFromOffset(p) if err != nil { - return nil, err + return nil, fmt.Errorf("read Goose session: %w", err) }(add
"fmt"to imports)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/ox-adapter-goose/serve.go` around lines 19 - 48, The delegated handler errors in the OnFindSession and OnReadFromOffset callbacks are returned without context. Import fmt and wrap each err from handleFindSession and handleReadFromOffset with a descriptive operation-specific message before returning it.Source: Coding guidelines
🧹 Nitpick comments (5)
cmd/ox-adapter-goose/session_test.go (2)
291-296: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDon't discard
readMessageserrors here.A query failure surfaces as a confusing length mismatch instead of the real cause.
♻️ Proposed tweak
- if replayed, _ := readMessages(db, "s1", count); len(replayed) != 1 { + replayed, err := readMessages(db, "s1", count) + if err != nil { + t.Fatalf("readMessages from count watermark: %v", err) + } + if len(replayed) != 1 { t.Errorf("count-based watermark should replay 1 entry (that is the bug), got %d", len(replayed)) } - if fresh, _ := readMessages(db, "s1", got); len(fresh) != 0 { + fresh, err := readMessages(db, "s1", got) + if err != nil { + t.Fatalf("readMessages from rowid watermark: %v", err) + } + if len(fresh) != 0 { t.Errorf("rowid watermark must replay nothing, got %d", len(fresh)) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/ox-adapter-goose/session_test.go` around lines 291 - 296, Update the assertions around readMessages in the watermark test to capture and check its returned error before validating message counts. Report query failures directly through the test framework, while preserving the existing replay-count assertions for successful reads.
108-119: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTool cases assert only the entry count.
wantFirstis empty for thetool request/tool responsecases, so theif tt.wantFirst != ""guard skips the only content assertion —ToolName,ToolInput,IsError, andCallIDare never checked, and the correlation ID contract fromadapterruntime.ToolUseWithID/ToolResultWithIDgoes untested. Add expected fields to the table and assert them unconditionally.💚 Proposed test tightening
tests := []struct { name string role string content string wantEntries int wantFirst string + wantTool string + wantCallID string }{ @@ { name: "tool request", role: "assistant", content: `[{"type":"toolRequest","id":"call_1","toolCall":{"status":"success","value":{"name":"developer__shell","arguments":{"command":"ls"}}}}]`, wantEntries: 1, + wantTool: "developer__shell", + wantCallID: "call_1", },if tt.wantTool != "" && got[0].ToolName != tt.wantTool { t.Errorf("ToolName = %q, want %q", got[0].ToolName, tt.wantTool) } if tt.wantCallID != "" && got[0].CallID != tt.wantCallID { t.Errorf("CallID = %q, want %q", got[0].CallID, tt.wantCallID) }Based on learnings, avoid assertions inside conditionals that can skip them and let a test pass vacuously.
Also applies to: 142-152
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/ox-adapter-goose/session_test.go` around lines 108 - 119, Strengthen the table-driven tests around the tool request and tool response cases in the session test by adding expected tool name, input, error status, and call ID fields, then assert every field unconditionally after parsing. Update the existing wantFirst-based assertions and related test symbols so both ToolUseWithID and ToolResultWithID correlation IDs and payloads are verified rather than allowing empty expectations to skip validation.Source: Learnings
cmd/ox-adapter-goose/session.go (2)
54-58: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDead
Rawfield and a comment that undersells the group.
Rawis taggedjson:"-"and never assigned or read; and thetoolRequestcomment sits aboveToolResp(toolResult), which belongs totoolResponseblocks.♻️ Proposed cleanup
- // type == "toolRequest" - ID string `json:"id"` - ToolCall *gooseToolCall `json:"toolCall"` - ToolResp *gooseToolResp `json:"toolResult"` - Raw json.RawMessage `json:"-"` + // Correlation ID shared by a toolRequest/toolResponse pair. + ID string `json:"id"` + + // type == "toolRequest" + ToolCall *gooseToolCall `json:"toolCall"` + + // type == "toolResponse" + ToolResp *gooseToolResp `json:"toolResult"`🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/ox-adapter-goose/session.go` around lines 54 - 58, Remove the unused Raw field from the relevant session struct, and move or revise the nearby type comment so it describes the ToolCall/toolRequest fields without grouping ToolResp/toolResult under them. Preserve the existing JSON field names and struct behavior.
84-99: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the DB path injectable so the protocol handlers can be tested.
openDBresolves a process-global path, sohandleFindSession,handleRead,handleReadFromOffset,handleReadMetadata, andhandleCapturePriorare untestable — the test file exercises only the pure helpers. Splitting out anopenDBAt(path string)(or a package-leveldbPathFnhook) lets the fixture DB drive the full handler surface. While here, wrap theos.Staterror instead of dropping it; a permission failure currently reports "not found".♻️ Proposed refactor
func openDB() (*sql.DB, error) { dbPath := gooseDBPath() if dbPath == "" { return nil, fmt.Errorf("goose data directory not found") } + return openDBAt(dbPath) +} + +// openDBAt opens a specific sessions.db. Split out so tests can point at a fixture. +func openDBAt(dbPath string) (*sql.DB, error) { if _, err := os.Stat(dbPath); err != nil { + if errors.Is(err, os.ErrNotExist) { + return nil, fmt.Errorf("goose sessions.db not found at %s", dbPath) + } - return nil, fmt.Errorf("goose sessions.db not found at %s", dbPath) + return nil, fmt.Errorf("stat goose sessions.db at %s: %w", dbPath, err) }Based on learnings, prefer
errors.Is(err, os.ErrNotExist)for file-not-found checks. As per coding guidelines,**/*_test.gomust "maintain at least 85% coverage".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/ox-adapter-goose/session.go` around lines 84 - 99, Refactor openDB into an injectable openDBAt(path string) helper, while retaining openDB’s gooseDBPath resolution for production callers, so handlers including handleFindSession, handleRead, handleReadFromOffset, handleReadMetadata, and handleCapturePrior can use fixture database paths. In openDBAt, distinguish os.ErrNotExist with errors.Is and wrap other os.Stat failures with their original error details instead of reporting them as missing; preserve the existing read-only SQLite options.Sources: Coding guidelines, Learnings
cmd/ox-adapter-goose/main.go (1)
150-176: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
errors.Is(err, os.ErrNotExist)instead ofos.IsNotExist(err)across both sites. Same root cause: this repo's convention (per coding guidelines and a prior learning) iserrors.Is(err, os.ErrNotExist)for file-not-found checks, since it correctly unwraps wrapped errors per Go 1.13+ semantics.
cmd/ox-adapter-goose/main.go#L150-L176: replaceos.IsNotExist(err)at lines 152 and 165 witherrors.Is(err, os.ErrNotExist)(requires importing"errors").cmd/ox-adapter-goose/hooks_test.go#L220-L239: replace!os.IsNotExist(err)at line 231 with!errors.Is(err, os.ErrNotExist)(requires importing"errors").🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/ox-adapter-goose/main.go` around lines 150 - 176, Replace os.IsNotExist with errors.Is(err, os.ErrNotExist) in the two checks around hooksFilePath and manifestFilePath in cmd/ox-adapter-goose/main.go, adding the errors import. Apply the same replacement to the file-not-found assertion in cmd/ox-adapter-goose/hooks_test.go, also adding errors there.Sources: Coding guidelines, Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.sageox/README.md:
- Around line 68-72: Update the agent support list in the README to distinguish
automatic recording or lifecycle-hook integration from automatic priming.
Specifically, remove Gemini CLI and Droid from the automatic-priming implication
in that bullet and describe their hook-based recording while preserving the
marker-based priming guidance consistent with the compatibility matrix and
limitation notes.
In `@cmd/ox-adapter-goose/session.go`:
- Around line 311-321: Normalize createdTS in the session message row loop
before constructing ts: when its value exceeds 10,000,000,000, divide it by
1,000 to convert milliseconds to seconds, then pass the normalized value to
time.Unix. Keep existing scanning and parseContentBlocks behavior unchanged.
---
Duplicate comments:
In `@cmd/ox-adapter-goose/hooks.go`:
- Around line 117-124: Update pluginDir to return (string, error) and reject
project scope with an empty repoRoot or user scope when os.UserHomeDir fails or
returns empty, rather than producing a relative path. Propagate this error
through hooksFilePath and manifestFilePath, then handle it in handleCheckHooks
and handleUninstallHooks before any filesystem access, especially the
destructive os.RemoveAll call.
In `@cmd/ox-adapter-goose/main.go`:
- Around line 95-126: Update handleImportSession to capture and handle the error
returned by readMetadata. Return a contextual error when metadata loading fails,
and only construct ImportSessionResult with the metadata after a successful
read.
In `@cmd/ox-adapter-goose/serve.go`:
- Around line 19-48: The delegated handler errors in the OnFindSession and
OnReadFromOffset callbacks are returned without context. Import fmt and wrap
each err from handleFindSession and handleReadFromOffset with a descriptive
operation-specific message before returning it.
---
Nitpick comments:
In `@cmd/ox-adapter-goose/main.go`:
- Around line 150-176: Replace os.IsNotExist with errors.Is(err, os.ErrNotExist)
in the two checks around hooksFilePath and manifestFilePath in
cmd/ox-adapter-goose/main.go, adding the errors import. Apply the same
replacement to the file-not-found assertion in
cmd/ox-adapter-goose/hooks_test.go, also adding errors there.
In `@cmd/ox-adapter-goose/session_test.go`:
- Around line 291-296: Update the assertions around readMessages in the
watermark test to capture and check its returned error before validating message
counts. Report query failures directly through the test framework, while
preserving the existing replay-count assertions for successful reads.
- Around line 108-119: Strengthen the table-driven tests around the tool request
and tool response cases in the session test by adding expected tool name, input,
error status, and call ID fields, then assert every field unconditionally after
parsing. Update the existing wantFirst-based assertions and related test symbols
so both ToolUseWithID and ToolResultWithID correlation IDs and payloads are
verified rather than allowing empty expectations to skip validation.
In `@cmd/ox-adapter-goose/session.go`:
- Around line 54-58: Remove the unused Raw field from the relevant session
struct, and move or revise the nearby type comment so it describes the
ToolCall/toolRequest fields without grouping ToolResp/toolResult under them.
Preserve the existing JSON field names and struct behavior.
- Around line 84-99: Refactor openDB into an injectable openDBAt(path string)
helper, while retaining openDB’s gooseDBPath resolution for production callers,
so handlers including handleFindSession, handleRead, handleReadFromOffset,
handleReadMetadata, and handleCapturePrior can use fixture database paths. In
openDBAt, distinguish os.ErrNotExist with errors.Is and wrap other os.Stat
failures with their original error details instead of reporting them as missing;
preserve the existing read-only SQLite options.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2a988663-437d-4de4-b370-90f1ac142a16
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (27)
.config/goreleaser.yml.sageox/README.mdMakefileREADME.mdcmd/ox-adapter-goose/hooks.gocmd/ox-adapter-goose/hooks_test.gocmd/ox-adapter-goose/main.gocmd/ox-adapter-goose/main_test.gocmd/ox-adapter-goose/serve.gocmd/ox-adapter-goose/session.gocmd/ox-adapter-goose/session_test.gocmd/ox/doctor_adapter_blocks.gocmd/ox/doctor_agent.gocmd/ox/init.gocmd/ox/release_notes.mdcmd/ox/upgrade.godocs/guides/agent-compatibility.mdgo.modinternal/adapter/registry.yamlinternal/adapter/registry_test.gointernal/prime/agent_type.gointernal/prime/conformance_test.gointernal/session/adapters/adapter.gointernal/session/adapters/external_symlink_test.gointernal/session/adapters/generic_jsonl_test.gointernal/session/adapters/session_roots.goscripts/install.sh
🚧 Files skipped from review as they are similar to previous changes (6)
- scripts/install.sh
- cmd/ox/doctor_agent.go
- Makefile
- cmd/ox/init.go
- README.md
- cmd/ox/upgrade.go
…ows read Co-Authored-By: SageOx <ox@sageox.ai> SageOx-Session: https://sageox.ai/c/ses_019fb5b2-5493-755f-ab65-12ba567187ef
|
Addressed the review. Four findings were real and are fixed; two I skipped with reasons. Fixed
Skipped
Also added, prompted by the review:
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cmd/ox-adapter-goose/main.go (1)
173-182: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winUse
errors.Is(err, os.ErrNotExist)instead ofos.IsNotExist.This newly added check uses the legacy
os.IsNotExist(err)idiom, which doesn't unwrap wrapped errors.🩹 Proposed fix
- if _, err := os.Stat(manifestPath); os.IsNotExist(err) { + if _, err := os.Stat(manifestPath); errors.Is(err, os.ErrNotExist) {As per coding guidelines,
**/*.go: "Use errors.Is() and errors.As() for error inspection and wrap errors with contextual information." Based on learnings, "prefererrors.Is(err, os.ErrNotExist)overos.IsNotExist(err)" is the established convention in this repo.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/ox-adapter-goose/main.go` around lines 173 - 182, The manifest existence check in the diagnostic flow uses the legacy os.IsNotExist inspection. Update the os.Stat error condition around manifestPath to use errors.Is with os.ErrNotExist, and add or reuse the required errors import while preserving the existing issue creation behavior.Sources: Coding guidelines, Learnings
cmd/ox-adapter-goose/session.go (1)
82-99: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winOpen Goose’s database with a real read-only URI.
modernc.org/sqlitestrips query parameters from DSNs without afile:prefix, then opens them withREADWRITE|CREATE; thereforemode=rois ignored and_pragma=journal_mode(WAL)can mutatesessions.db. Construct a proper escapedfile:URI or use another driver-supported read-only mechanism.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/ox-adapter-goose/session.go` around lines 82 - 99, Update openDB to construct a modernc.org/sqlite-compatible escaped file: URI for the database path, preserving mode=ro and the required pragmas in the URI query. Ensure sql.Open receives this file-prefixed read-only DSN so it cannot create or modify sessions.db.
♻️ Duplicate comments (1)
cmd/ox-adapter-goose/session.go (1)
303-341: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMillisecond
created_timestampvalues still aren't normalized.A prior review comment on this same function (previously lines 311-321, researched against upstream Goose source) found that
created_timestampcan be stored in either seconds or milliseconds, and values above10_000_000_000must be divided by 1,000 beforetime.Unix. That fix wasn't applied here even though this function was otherwise substantially rewritten for the watermark fix — line 332 still doestime.Unix(createdTS, 0).UTC()unconditionally, which would produce invalid far-future timestamps for millisecond-backed rows.🩹 Proposed fix
ts := time.Unix(createdTS, 0).UTC() + if createdTS > 10_000_000_000 { + ts = time.UnixMilli(createdTS).UTC() + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/ox-adapter-goose/session.go` around lines 303 - 341, Normalize createdTS in readMessages before constructing ts: when it exceeds 10,000,000,000, divide it by 1,000, then pass the normalized seconds value to time.Unix. Preserve existing UTC conversion and parsing behavior for second-based timestamps.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@cmd/ox-adapter-goose/main.go`:
- Around line 173-182: The manifest existence check in the diagnostic flow uses
the legacy os.IsNotExist inspection. Update the os.Stat error condition around
manifestPath to use errors.Is with os.ErrNotExist, and add or reuse the required
errors import while preserving the existing issue creation behavior.
In `@cmd/ox-adapter-goose/session.go`:
- Around line 82-99: Update openDB to construct a modernc.org/sqlite-compatible
escaped file: URI for the database path, preserving mode=ro and the required
pragmas in the URI query. Ensure sql.Open receives this file-prefixed read-only
DSN so it cannot create or modify sessions.db.
---
Duplicate comments:
In `@cmd/ox-adapter-goose/session.go`:
- Around line 303-341: Normalize createdTS in readMessages before constructing
ts: when it exceeds 10,000,000,000, divide it by 1,000, then pass the normalized
seconds value to time.Unix. Preserve existing UTC conversion and parsing
behavior for second-based timestamps.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 261f9ef5-9b30-44b0-91ff-8913f5960e57
📒 Files selected for processing (8)
cmd/ox-adapter-goose/diagnose_test.gocmd/ox-adapter-goose/hooks.gocmd/ox-adapter-goose/hooks_test.gocmd/ox-adapter-goose/main.gocmd/ox-adapter-goose/session.gocmd/ox-adapter-goose/session_test.gocmd/ox/hook_content_validator.gocmd/ox/hook_content_validator_test.go
Greptile SummaryThis change adds Goose session recording, Open Plugins hook lifecycle support, and distribution wiring. A real project-scope uninstall was run against an isolated repository where the Goose hook file was a symlink to an external writable file. Uninstall completed successfully and rewrote that external file while preserving the symlink. Do not merge until project hook paths are protected from symlink redirection during uninstall. Confidence Score: 3/5
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…x, timestamp normalization, opencode ReadFromOffset Co-Authored-By: SageOx <ox@sageox.ai> SageOx-Session: https://sageox.ai/c/ses_019fb5d3-fa51-7392-8628-c7a18f169e62
|
Addressed all open review threads in Fixed:
Skipped (with rationale):
Bonus fix (OpenCode — ox-8arr):
|
There was a problem hiding this comment.
🧹 Nitpick comments (3)
cmd/ox-adapter-goose/hooks_test.go (1)
436-464: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen symlink tests: assert the redirect actually didn't happen.
Both tests only assert
err != nil, so they'd pass for any unrelated install failure. Asserting the external target is unmodified (and/or that the error mentions the symlink) pins the actual protection.💚 Example for `TestInstallHooks_RefusesSymlinkedFile`
_, err := handleInstallHooks(projectParams(root)) if err == nil { t.Fatal("install must refuse to write through a symlinked plugin.json") } + got, readErr := os.ReadFile(target) + if readErr != nil { + t.Fatalf("read target: %v", readErr) + } + if string(got) != `{}` { + t.Errorf("install wrote through the symlink: %s", got) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/ox-adapter-goose/hooks_test.go` around lines 436 - 464, Strengthen the symlink protection assertions in the affected install tests, especially TestInstallHooks_RefusesSymlinkedFile: after handleInstallHooks returns an error, read the external target and assert its contents remain unchanged, and verify the error identifies the symlink where practical. Apply the same redirect-prevention assertion to the symlinked .agents directory test, ensuring unrelated install failures cannot satisfy the tests.cmd/ox-adapter-goose/hooks.go (2)
150-160: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueReject paths that escape
scopeRootbefore walking components.If
pathis not underscopeRoot,filepath.Relyields..components, andfilepath.Join(current, "..")normalizes upward — silently walking outside the intended boundary instead of failing. Current callers always passpluginDir-derived paths, so this is defensive only.🛡️ Proposed guard
rel, err := filepath.Rel(scopeRoot, path) if err != nil { return fmt.Errorf("cannot relativize %q from %q: %w", path, scopeRoot, err) } + if rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) { + return fmt.Errorf("%q escapes scope root %q", path, scopeRoot) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/ox-adapter-goose/hooks.go` around lines 150 - 160, Update verifyNoSymlinks to reject paths outside scopeRoot immediately after filepath.Rel returns, detecting a relative path equal to ".." or beginning with ".." plus a path separator. Return an error before iterating components; preserve the existing component-walking behavior for paths within scopeRoot.
161-167: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
errors.Is(err, os.ErrNotExist)instead ofos.IsNotExist(err).Matches Go 1.13+ unwrapping semantics and the repo's error-inspection convention.
♻️ Proposed change
fi, err := os.Lstat(current) if err != nil { - if os.IsNotExist(err) { + if errors.Is(err, os.ErrNotExist) { return nil // path doesn't exist yet — nothing to follow } return fmt.Errorf("lstat %q: %w", current, err) }Requires importing
errors.Based on learnings: "prefer
errors.Is(err, os.ErrNotExist)overos.IsNotExist(err)". As per coding guidelines: "Useerrors.Is()anderrors.As()for error inspection".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/ox-adapter-goose/hooks.go` around lines 161 - 167, In the path handling logic around os.Lstat, import the errors package and replace os.IsNotExist(err) with errors.Is(err, os.ErrNotExist), preserving the existing return behavior for missing paths and other errors.Sources: Coding guidelines, Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cmd/ox-adapter-goose/hooks_test.go`:
- Around line 436-464: Strengthen the symlink protection assertions in the
affected install tests, especially TestInstallHooks_RefusesSymlinkedFile: after
handleInstallHooks returns an error, read the external target and assert its
contents remain unchanged, and verify the error identifies the symlink where
practical. Apply the same redirect-prevention assertion to the symlinked .agents
directory test, ensuring unrelated install failures cannot satisfy the tests.
In `@cmd/ox-adapter-goose/hooks.go`:
- Around line 150-160: Update verifyNoSymlinks to reject paths outside scopeRoot
immediately after filepath.Rel returns, detecting a relative path equal to ".."
or beginning with ".." plus a path separator. Return an error before iterating
components; preserve the existing component-walking behavior for paths within
scopeRoot.
- Around line 161-167: In the path handling logic around os.Lstat, import the
errors package and replace os.IsNotExist(err) with errors.Is(err,
os.ErrNotExist), preserving the existing return behavior for missing paths and
other errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: bacb1849-3c7c-40ab-8e89-b227a1c2c9a0
📒 Files selected for processing (14)
.sageox/README.mdcmd/ox-adapter-goose/hooks.gocmd/ox-adapter-goose/hooks_test.gocmd/ox-adapter-goose/main.gocmd/ox-adapter-goose/serve.gocmd/ox-adapter-goose/session.gocmd/ox-adapter-opencode/main.gocmd/ox-adapter-opencode/session.gocmd/ox/guides/agents-md.mdcmd/ox/guides/getting-started.mdcmd/ox/guides/murmur-vs-rule.mdcmd/ox/guides/team-rules.mdcmd/ox/init.gocmd/ox/release_notes.md
🚧 Files skipped from review as they are similar to previous changes (9)
- cmd/ox/guides/murmur-vs-rule.md
- cmd/ox/guides/team-rules.md
- cmd/ox/guides/getting-started.md
- cmd/ox/guides/agents-md.md
- cmd/ox/release_notes.md
- .sageox/README.md
- cmd/ox-adapter-goose/serve.go
- cmd/ox-adapter-goose/session.go
- cmd/ox/init.go
|
|
||
| if err := writeHooksFile(hooksPath, hf); err != nil { | ||
| return nil, fmt.Errorf("failed to write hooks.json: %w", err) |
There was a problem hiding this comment.
Project-scope uninstall reads and rewrites hooks.json without validating that the repository-derived path is not a symlink. A repository can point .agents/plugins/sageox/hooks/hooks.json at an external writable file; when that file contains a non-ox rule that must be retained, uninstall removes the ox rule and rewrites the external target. Reject symlinked hook paths before reading or writing them during uninstall, including the final hooks.json component.
Artifacts
Executable Goose uninstall symlink reproduction script
- Creates the isolated fixture, builds the adapter, runs its uninstall command, and asserts the external rewrite; takeaway: the reproduction is repeatable.
Goose hooks symlink fixture before uninstall
- Shows the repository Goose hooks path linked to the external file containing retained and ox rules; takeaway: the required preconditions were established.
Actual Goose adapter uninstall after symlink fixture
- Shows actual adapter success output and the external target rewritten to retain only the non-ox rule while the link remains; takeaway: the defect is reproduced.
- Shows the build command for the tested adapter binary exited successfully; takeaway: execution used code built from this checkout.
1 participant
What this ships
Goose becomes a Silver-tier AI coworker: sessions record to the Ledger, and hooks fire
ox agent primeautomatically. Before this, Goose got anAGENTS.mdmarker and nothing else.New
cmd/ox-adapter-goose/(~900 LOC + ~600 test), plus the ox-side wiring and release plumbing.flowchart LR GS["Goose session starts"] --> HK["hooks.json fires<br/>ox agent hook SessionStart"] HK --> PR["ox agent prime<br/>team context in"] PR --> REC["find-session, then<br/>drain on every turn"] REC --> DB["sessions.db<br/>SQLite read-only"] DB --> LG["Ledger"]Two places this beats the template it was modeled on
cmd/ox-adapter-opencodeis the only other SQLite reader, so it was the starting point. Goose's schema supports better in two ways worth not copying:MAX(messages.id), notCOUNT(*). Goose'sidisAUTOINCREMENT, so it is a real watermark. A count-based offset silently replays or skips rows the moment any message is deleted.TestMaxMessageID_UsesRowidNotCountdemonstrates the divergence directly: after one delete, the count-based watermark replays an entry that already streamed; the rowid watermark replays nothing.sessions.working_dir, an actual column, matched exactly then by subdirectory prefix. OpenCode picks "most recent session, no repo filter."TestResolveSessionID_PrefersWorkingDirseeds a newer session in a different repo to prove recency does not win.The hook installer is a new shape for ox
Every existing ox adapter edits one settings file. Goose follows the Open Plugins spec, so hooks live in a plugin directory:
Design decisions worth reviewing:
commandpoints straight atox agent hook <Event>. Verified no shim is needed: ox takes the event name from argv, and Goose's stdin fieldssession_id/tool_name/tool_inputare exact matches for whatagentx.HookInputdecodes. Unmapped Goose fields survive inRawBytes.OX_PROJECT_ROOTis baked into project-scope commands. Goose sendsworking_dironly on tool events, soSessionStartalone gives ox nothing to locate the repo with and it would walk up from an arbitrary cwd.config.ResolveProjectRootOverride()already honors the variable andfindProjectRootchecks it first. Paths are single-quoted — Goose runs commands viash -c, so a repo path with a space would otherwise split.matcheris never emitted. Goose treats it as a regex, and a bare"*"is invalid, so Goose logs a warning and silently skips the whole rule. Omitting the field is the only unambiguous "match everything." Pinned byTestInstallHooks_NeverEmitsMatcher..agents/plugins/is a shared namespace. Uninstall strips only rules carrying theox agent hookmarker, and removes the directory only whenplugin.jsoncarriesx-ox-managed. Two tests cover it: a foreign rule survives, and a foreign manifest stops the directory delete.check-hooksrequires the manifest. Ahooks.jsonwith no siblingplugin.jsonlooks installed on disk but Goose never loads it, so the hooks never fire. Reporting "installed" there would hide a completely dead setup.Hook event coverage
Goose fires eleven events. ox installs seven:
SessionStartSessionEndStopUserPromptSubmitPreToolUsePostToolUsePostToolUseFailurePostToolUseonly on success, so without this a turn ending in a failed tool call is invisible until the next success orStopBeforeReadFileBeforeShellExecutionPreToolUse— reading a file and running a shell command are both tool callsAfterFileEditAfterShellExecutionPostToolUse(and success-only)Installing the four would spawn
ox agent hooktwice per tool call for zero signal ox does not already have.TestInstallHooks_EventCoverageasserts both lists so the reasoning survives contact with the next reader. If you want literal 11-event coverage anyway, it is a four-line change tohookEvents— say so and I will flip it.agentx
Two releases, both merged and tagged:
LifecycleEventMapper.HookEventPostToolUseFailure, mapped for Goose.So no
localEventPhasesentry is needed incmd/ox/agent_hook.go— unlike Gemini, which is still waiting on that migration.Two blockers that are easy to miss
internal/session/adapters/adapter.go—"goose": "generic"→"goose": "goose". Without this,lookupAdapterresolves the alias to the always-registered generic adapter,GetAdapter'sRegisterExternalAdapters()retry never runs, and the binary is permanently shadowed.internal/session/adapters/session_roots.go— fail-closed. The daemon rejects everySessionFilefrom an adapter with no allow-listed root.Drift backfilled while here
The release plumbing was already behind, and Goose would have widened the gap:
scripts/install.shPATHcmd/ox/upgrade.gointernal/adapter/registry.yamlTest Plan
make lint— 0 issuesmake test— 17151 tests pass, 1086 skipped, 0 failuresgo test ./cmd/ox-adapter-goose/...— fixturesessions.dbbuilt int.TempDir()covering all fivecontent_jsonblock types, malformed JSON, failed tool envelopes, and the offset semanticsVerified against a real Goose install on this machine, not just fixtures:
detect→{"detected":true,"reason":"found sessions.db"}find-session→goose:20260602_3, offset104read→ 34 entries (1 user, 1 assistant, 32 tool), modelgemma4:26bextracted frommodel_config_jsonread-from-offset 100→ 4 entries,new_offset: 104;read-from-offset 104→ empty. The one-shot path works — that is exactly the path broken for OpenCode today (ox-8arr)install-hooks→ correct plugin directory, quoted paths, nomatcher, 30s onSessionStartand 10s elsewhere;check-hooksconfirmsNot yet verified: whether Goose injects, warns on, or ignores hook stdout.
handleStartpipesox agent primeJSON to stdout. This does not block recording or priming, but it is why I would watch the first real Goose session before assumingUserPromptSubmitwhispers render cleanly.Known limitation, stated not hidden
Goose has no compaction event. Team context primed at session start is lost when Goose compacts, and ox cannot work around it. Documented in the compatibility guide, the CHANGELOG, and on
EventPhases()upstream so nobody "fixes" it by mapping a phase that never fires.Co-Authored-By: SageOx
Summary by CodeRabbit
New Features
ox recapevidence reports in text and JSON formats.Documentation
Bug Fixes