feat(goose): source adapter and filesystem destination (#39) - #109
Conversation
Add goose-sessions-sqlite-v15 reader for sessions/sessions.db, default user-session listing filters, and destination install to .goose/skills plus ~/.config/goose/skills. Matrix expands to 11×11=121.
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2f3004378f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| """ | ||
| SELECT id, name, session_type, working_dir, created_at, updated_at, archived_at | ||
| FROM sessions | ||
| WHERE session_type = 'user' AND archived_at IS NULL |
There was a problem hiding this comment.
When goose has created a newer user session but no messages have been written yet, this query includes it and the shared latest selector chooses it over an older session containing recoverable context, producing an empty handoff. Filter for at least one eligible public message before ordering/limiting so blank sessions cannot crowd out useful history.
Useful? React with 👍 / 👎.
| try: | ||
| payload = json.loads(content_json, object_pairs_hook=_object) | ||
| except (json.JSONDecodeError, _DuplicateKey, RecursionError, UnicodeDecodeError): | ||
| return None |
There was a problem hiding this comment.
If a selected session contains truncated JSON or duplicate keys in content_json, this branch returns None, so _show_session silently drops that row and still emits a successful but incomplete handoff. Treat these parse failures as E_CORRUPT_RECORD; otherwise users may trust a transcript that omitted requests or responses.
Useful? React with 👍 / 👎.
Exclude user sessions with no public messages from default latest listing, and treat malformed content_json as E_CORRUPT_RECORD instead of silent drop.
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f0808953bb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| AND EXISTS ( | ||
| SELECT 1 FROM messages m | ||
| WHERE m.session_id = sessions.id | ||
| AND m.role IN ('user', 'assistant', 'tool') | ||
| ) |
There was a problem hiding this comment.
The new EXISTS fix still tests only the message role, so a newer session containing a valid but non-extractable payload such as content_json = '[]' or image-only content remains eligible for latest; _content_text then returns None for every row and the reader emits an empty handoff instead of selecting the older recoverable session. This is fresh evidence beyond the earlier no-message case: require at least one message whose payload can produce a public turn, or fall back when the selected session normalizes to no turns.
Useful? React with 👍 / 👎.
| if _SESSION_ID_RE.fullmatch(text): | ||
| return text |
There was a problem hiding this comment.
When show receives a single-token title such as alpha, this regex classifies it as an exact session ID and _list_sessions runs only WHERE id = ?, so the generic bounded title matcher never sees the actual session. The same optimization makes an uppercase pasted UUID fail because SQLite's text comparison is case-sensitive even though select_session normally canonicalizes UUID refs; if the exact lookup misses, perform the normal bounded listing so supported text and normalized-ID selection still work.
Useful? React with 👍 / 👎.
Exclude sessions whose messages lack text/content payloads, and fall back from exact-id miss so free-text selection remains available.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 19a5928f90
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| data_home = os.environ.get("XDG_DATA_HOME") | ||
| if data_home: | ||
| return os.path.join(data_home, "goose") | ||
| return os.path.expanduser("~/.local/share/goose") |
There was a problem hiding this comment.
When the reader runs on Windows without --source-root, this falls through to ~/.local/share/goose, but Goose's v15 store is normally under %APPDATA%/Block/goose/data; consequently probe and list report the adapter unavailable for a standard Windows installation. Add a Windows-specific root derived from APPDATA while retaining the explicit override.
Useful? React with 👍 / 👎.
| The package builder creates eleven `*-<host>-skills.zip` archives (one per | ||
| enabled destination, including Pi, OpenClaw, and goose), seven supported |
There was a problem hiding this comment.
This updated paragraph says the builder creates eleven destination archives, but line 19 still says current main builds ten. That leaves the installation guide internally contradictory and can make release operators expect a missing artifact; update the latter count to eleven.
AGENTS.md reference: AGENTS.md:L5-L5
Useful? React with 👍 / 👎.
Resolve %APPDATA%/Block/goose/data on Windows and align docs to eleven destination archives on main.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b34ebe3f91
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| SELECT role, content_json, created_timestamp | ||
| FROM messages |
There was a problem hiding this comment.
When a valid schema contains two rows with the same non-empty message_id, this query omits that identifier and _show_session emits both rows as separate turns, so a corrupted store produces a successful but duplicated transcript. Select message_id (and preferably the stored session ID) and fail with E_CORRUPT_RECORD when a logical ID repeats or disagrees with the selected session.
Useful? React with 👍 / 👎.
|
|
||
| def _require_schema(connection: sqlite3.Connection) -> None: | ||
| try: | ||
| integrity = connection.execute("PRAGMA integrity_check(1)").fetchone() |
There was a problem hiding this comment.
For a large Goose database, PRAGMA integrity_check(1) still scans the entire database—the argument limits reported errors, not work—and the normal show flow invokes this through probe, list, and show. This defeats the oversized live-query path and can make reading a large history perform three expensive full-database scans; keep probe/schema validation metadata-bounded and detect corruption from the bounded queries instead.
Useful? React with 👍 / 👎.
Reject repeated non-empty logical message identifiers while showing a session.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c2f019753b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| m.content_json LIKE '%"text"%' | ||
| OR m.content_json LIKE '%"content"%' | ||
| ) |
There was a problem hiding this comment.
The new LIKE predicate still admits messages whose matching keys contain no extractable text, such as [{"type":"text","text":" "}] or an image part with a text key; _content_text returns None for these, so a newer such session wins latest and produces a successful empty handoff instead of older recoverable context. Fresh evidence is that the current key-name filter does not enforce the extractor's nonblank text semantics; use the same eligibility semantics or retry the next candidate when extraction yields no turns.
Useful? React with 👍 / 👎.
| ).fetchall() | ||
|
|
||
| def _fetch_normal() -> list[tuple]: | ||
| limit = DEFAULT_BOUNDS.scanned_records + 1 |
There was a problem hiding this comment.
When a library caller supplies lowered Bounds, normal listing still uses the global defaults and never charges budget.records; for example, ReadBudget(Bounds(scanned_records=0)) still returns the basic fixture session with records == 0. The same issue affects lowered listed_sessions, allowing this adapter to bypass the caller's resource ceiling, so pass the budget into _list_sessions, derive limits from budget.limits, and charge admitted rows.
Useful? React with 👍 / 👎.
|
@codex review |
Post-filter sessions so whitespace/image-only messages cannot win latest, and apply caller scanned/listed bounds while charging ReadBudget records.
|
@codex review |
e059c5a to
a59ef0a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a59ef0aa85
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| try: | ||
| text = _content_text(content_json) | ||
| except DiagnosticError: | ||
| continue |
There was a problem hiding this comment.
When a newer user session's eligible messages contain malformed JSON or duplicate keys and no later extractable row, this catch treats the session as empty, allowing show latest to return an older transcript successfully instead of reporting E_CORRUPT_RECORD. Fresh evidence beyond the earlier show-path issue is that current HEAD now suppresses the parser failure in the list eligibility path; propagate corruption rather than silently excluding that session.
Useful? React with 👍 / 👎.
| AND role IN ('user', 'assistant', 'tool', 'toolResult', 'tool_result', 'function') | ||
| ORDER BY id ASC | ||
| LIMIT 64 |
There was a problem hiding this comment.
When a session's first 64 public-role rows are non-extractable—for example image-only parts—but a later row contains valid text, this fixed limit reports the session as empty and lets an older session win latest. Fresh evidence beyond the prior extractability issue is that the new decoder now applies an arbitrary prefix cutoff; either query until the bounded transcript ceiling or retry candidates after full selected-session extraction.
Useful? React with 👍 / 👎.
| for role, content_json in rows: | ||
| if not isinstance(role, str) or not isinstance(content_json, str): | ||
| continue | ||
| try: | ||
| text = _content_text(content_json) |
There was a problem hiding this comment.
For stores containing many candidate sessions or a large content_json value, this decodes up to 64 message rows per candidate without checking record_bytes or consuming record/source-byte budget; only returned session summaries are charged later. Consequently a caller's Bounds(scanned_records=...) does not bound actual SQLite rows processed, and a payload larger than the default per-record ceiling is parsed during list even though show would reject it.
Useful? React with 👍 / 👎.
| if item is not None: | ||
| values.append(item) | ||
| budget.consume_records() | ||
| if exact_id is None and len(values) >= list_limit: | ||
| break |
There was a problem hiding this comment.
When a library caller supplies Bounds(listed_sessions=0), the first eligible summary is appended before this condition is checked, so ADAPTER.list still returns one session and consumes one record. Fresh evidence beyond the earlier lowered-bounds comment is that the current post-append check specifically bypasses the supported zero-work setting; handle a zero limit before iterating or before appending.
Useful? React with 👍 / 👎.
| if size > limits.sqlite_snapshot_bytes: | ||
| return query_only_live_sqlite(database, root=root, provider=FORMAT_ID) | ||
| return private_sqlite_connection(database, root=root, provider=FORMAT_ID) |
There was a problem hiding this comment.
When a caller lowers sqlite_snapshot_bytes, the main database may fall below that threshold while its main-plus-WAL family exceeds it; this call omits bounds=limits, so private_sqlite_connection copies the family using DEFAULT_BOUNDS and can read far beyond the caller's ceiling. Pass the effective bounds into the snapshot helper so the family-wide copy honors the same limit used to choose this path.
Useful? React with 👍 / 👎.
…#39) Propagate E_CORRUPT_RECORD from list eligibility instead of treating malformed content_json as empty, so a newer broken session cannot hide behind an older transcript. Also honor transcript/list budgets during eligibility, reject listed_sessions=0, and pass bounds into the SQLite snapshot copier.
|
@codex review |
1 similar comment
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 18dfe8311a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| seen_message_ids: set[str] = set() | ||
| turn_bounds = replace(DEFAULT_BOUNDS, tool_output_chars=query.max_tool_chars) | ||
| count = 0 | ||
| for message_id, role, content_json, _created in cursor: |
There was a problem hiding this comment.
When a library caller reuses a partially consumed ReadBudget, this loop checks only fresh local count and total_bytes values and never calls consume_transcript_records() or consume_bytes(). For example, a budget whose transcript and byte counters are already at their ceilings still reads and returns the complete basic fixture, allowing cumulative work to exceed the caller's shared limits; charge each admitted row through the supplied budget.
Useful? React with 👍 / 👎.
| if len(rows) > scan_limit: | ||
| raise DiagnosticError.limit_exceeded() |
There was a problem hiding this comment.
When a Goose database contains more than 2,000 active user sessions with public messages, ordinary list and show latest always raise here because _fetch_normal materializes scanned_records + 1 rows before considering the 50-session output limit. This makes long-lived stores unreadable even when the first 50 candidates are immediately eligible; stream candidates within the scan budget and stop once list_limit eligible summaries have been collected.
Useful? React with 👍 / 👎.
Honor shared ReadBudget counters during show (transcript rows + bytes) so partially consumed budgets fail closed. Cap normal listing at scanned_records without hard-failing when more sessions exist, stopping once listed_sessions eligible summaries are collected.
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1d6d3ef4bd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| continue | ||
| if isinstance(session_type, str) and session_type in _DEFAULT_EXCLUDE_TYPES: | ||
| continue | ||
| if not _session_has_extractable_turn(connection, session_id, budget): |
There was a problem hiding this comment.
When a candidate belongs to another cwd or fails the requested age filter, this eligibility check still parses and charges its messages before _row_summary rejects its metadata. Consequently, a malformed or oversized message in an unrelated Goose project can make list or show latest fail with E_CORRUPT_RECORD/E_LIMIT_EXCEEDED even when the requested project has a valid session; apply the cwd/age metadata filter before decoding message eligibility.
Useful? React with 👍 / 👎.
| ORDER BY updated_at DESC, id ASC | ||
| LIMIT ? | ||
| """, | ||
| (scan_limit,), |
There was a problem hiding this comment.
When the shared Goose database has scanned_records newer user sessions for other working directories, this global LIMIT excludes an older session for the requested cwd before _row_summary performs cwd matching. For example, with --within-min 0, 2,000 newer cross-project rows cause show latest to report E_NO_MATCH despite recoverable current-project history; constrain or prioritize the requested working directory before applying the bounded candidate window.
Useful? React with 👍 / 👎.
Merge gate (HEAD
|
1 participant
Summary
Closes #39 (filesystem/product path).
gooseadapter forsessions/sessions.db(goose-sessions-sqlite-v15, schema max 15)session_type=user, excludes scheduled/sub_agent/hidden/gateway/acp + archived.goose/skills+~/.config/goose/skillsTest plan