feat: add shared-memory skill for cross-project/cross-team memory#37
Open
morten1231 wants to merge 1 commit into
Open
feat: add shared-memory skill for cross-project/cross-team memory#37morten1231 wants to merge 1 commit into
morten1231 wants to merge 1 commit into
Conversation
Adds a new skill that provides cross-project, cross-team memory backed by SQLite+FTS5. Complements the file-based memory approaches with: - Cross-project searchable long-term store (survives machine/team/session rotation) - Full-text search (FTS5) across past learnings, rules, and incidents - Cross-team handoffs (structured episodic entries addressed team-to-team) - Lease-based task coordination (prevents two teams grabbing the same issue) All state lives in a single SQLite file (~/.claude/shared-memory/learnings.db, WAL mode) - no server, no daemon. Three small CLIs drive it: - memory: put/find/recent/show - handoff: send/inbox/show/ack - agent-team: claim/release/list/expire/status A SessionStart hook surfaces recent entries, pending handoffs, and recent completions into session context, capped at ~3KB so it never dominates. Tested end-to-end on two independent Claude Code installations sharing the same ~/.claude/shared-memory directory.
Summary by CodeRabbitNew Features
Documentation
WalkthroughIntroduces a new Changes
Sequence DiagramssequenceDiagram
participant User
participant CLI as memory/handoff/<br>agent-team CLI
participant Store as store.js
participant DB as SQLite DB
User->>CLI: Execute command<br/>(put/send/claim/etc.)
CLI->>CLI: Parse arguments<br/>Resolve team/scope
CLI->>Store: Call API<br/>(put/get/search/etc.)
Store->>DB: Initialize/Query/Insert/Update
DB-->>Store: Return results
Store-->>CLI: Return data
CLI->>CLI: Format output<br/>(JSON or text)
CLI-->>User: Print result
sequenceDiagram
participant Session as Claude Session
participant Hook as session-start-<br>memory.js
participant Store as store.js
participant DB as SQLite DB
participant Files as Config/Reflexion<br/>Files
Session->>Hook: SessionStart event
Hook->>Hook: Check SS_MEMORY_DISABLE
Hook->>Files: Resolve project from config
Hook->>Store: Initialize & query store
Store->>DB: Fetch recent entries<br/>(rules/facts/incidents)
DB-->>Store: Return entries
Hook->>Store: Search for handoffs<br/>(unacked in team)
Store->>DB: Query handoff* entries
DB-->>Store: Return results
Hook->>Files: Read reflexion gotchas
Hook->>Hook: Format markdown context
Hook-->>Session: Emit JSON context<br/>via stdout
sequenceDiagram
participant TeamA as Team A
participant CLI_A as agent-team CLI<br/>(Team A)
participant Store as store.js
participant DB as SQLite DB
participant CLI_B as agent-team CLI<br/>(Team B)
participant TeamB as Team B
TeamA->>CLI_A: claim --issue=123
CLI_A->>Store: put(lease:123, team=A)
Store->>DB: Insert/update lease entry
DB-->>Store: Stored
Store-->>CLI_A: Success
CLI_A-->>TeamA: Lease acquired
TeamB->>CLI_B: claim --issue=123<br/>(without --force)
CLI_B->>Store: check existing lease
Store->>DB: Query lease:123
DB-->>Store: Return (owned by A)
Store-->>CLI_B: Conflict (already claimed)
CLI_B-->>TeamB: Error: lease held by Team A
TeamA->>CLI_A: release --issue=123
CLI_A->>Store: put(lease:123,<br/>released_at=now)
Store->>DB: Write release record
DB-->>Store: Stored
Store-->>CLI_A: Released
CLI_A-->>TeamA: Lease released
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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 |
Greptile SummaryAdds a new
Confidence Score: 3/5Not safe to merge — the lease atomicity bug breaks the primary coordination contract, and hardcoded internal paths prevent the skill from working for any external user. Three P1 findings: a race condition defeating the exclusivity guarantee, internal-only dependency paths with a misleading error message, and a hardcoded private filesystem path in a public skill. Core store, handoff, and memory CLIs are otherwise well-structured. skills/shared-memory/bin/agent-team (race condition in cmd_claim), skills/shared-memory/lib/store.js (internal paths + error message), skills/shared-memory/hooks/session-start-memory.js (private Reflexion path)
|
| Filename | Overview |
|---|---|
| skills/shared-memory/lib/store.js | Core SQLite+FTS5 store — hardcoded internal candidate paths in resolveBetterSqlite and misleading error message will block all external users. |
| skills/shared-memory/bin/agent-team | Lease coordination CLI — non-atomic check-then-write in cmd_claim breaks the exclusive-claim guarantee; allLeases() 500-record FTS limit silently drops records. |
| skills/shared-memory/hooks/session-start-memory.js | SessionStart hook — hardcoded private project path is dead code for external users; completions timestamp filtering should be in SQL. |
| skills/shared-memory/bin/handoff | Handoff CLI — functional; allHandoffs() 200-record FTS limit could miss older records; resolveTeam() duplicated. |
| skills/shared-memory/bin/memory | Memory CLI — solid wrappers over store.js; resolveTeam() duplicated across CLIs. |
| skills/shared-memory/SKILL.md | Documentation — clear and accurate. |
Prompt To Fix All With AI
This is a comment left during a code review.
Path: skills/shared-memory/lib/store.js
Line: 13-23
Comment:
**Hardcoded internal paths make this unusable outside the author's environment**
`resolveBetterSqlite` probes `.skrivstore-panel` and `.skrivstore` — directories that exist only on the author's machine. Any user without those paths falls through to `require('better-sqlite3')`, which then fails with an error pointing back to the same internal paths: `cd ~/.skrivstore-panel && npm install`. This is useless to any external user. The error message should give actionable general-purpose install guidance instead.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: skills/shared-memory/hooks/session-start-memory.js
Line: 64-81
Comment:
**Hardcoded private project path shipped in a public skill**
`~/projects/skrivsikkert/agent_ops/scripts/lib/reflexion.js` is a path on the author's personal machine. The `existsSync` guard means it silently no-ops for everyone else, but it is dead code for all external users and leaks an internal project name into the codebase. If Reflexion integration is desired, its path should be driven by `process.env.REFLEXION_PATH` and documented as an optional extension.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: skills/shared-memory/bin/agent-team
Line: 74-98
Comment:
**Race condition defeats the lease exclusivity guarantee**
The check-then-write in `cmd_claim` is not atomic. Two concurrent `agent-team claim AGE-123` calls both execute `store.get(...)` before either calls `store.put(...)`, both find no existing lease, and both proceed to create one. The last write wins silently — no error, no detection, two teams own the same issue. `better-sqlite3` supports synchronous transactions; the read + conditional write must be wrapped in a single `db.transaction(...)` call.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: skills/shared-memory/bin/agent-team
Line: 49-52
Comment:
**Hardcoded FTS limit silently drops leases beyond 500**
`allLeases()` fetches with `limit: 500` via FTS. If the DB accumulates more than 500 lease records, active leases will be silently missing from `list`, `expire`, and `status`. Querying `entry` directly by key prefix would be more reliable than relying on FTS keyword matching.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: skills/shared-memory/hooks/session-start-memory.js
Line: 86-98
Comment:
**Timestamp filter should be pushed into SQL, not applied in JS**
`completions` loads up to 100 records from the DB and then filters by `e.updated_at >= since` in JavaScript. As the DB grows this loads increasingly stale entries unnecessarily. The `since` cutoff belongs in the SQL `WHERE` clause.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: skills/shared-memory/bin/memory
Line: 49-55
Comment:
**`resolveTeam()` duplicated across all three CLIs**
The exact same function body appears in `bin/memory` (line 49), `bin/handoff` (line 31), and `bin/agent-team` (line 39). Extract it into `lib/resolve-team.js` and `require` it from each CLI so a bug fix or env-var change only needs to happen once.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "feat: add shared-memory skill for cross-..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+13
to
+23
| function resolveBetterSqlite() { | ||
| const candidates = [ | ||
| path.resolve(process.env.HOME, '.skrivstore-panel/node_modules/better-sqlite3'), | ||
| path.resolve(process.env.HOME, '.skrivstore/node_modules/better-sqlite3'), | ||
| ]; | ||
| for (const p of candidates) { | ||
| try { return require(p); } catch (_) {} | ||
| } | ||
| try { return require('better-sqlite3'); } catch (_) {} | ||
| throw new Error('better-sqlite3 not found. Run: cd ~/.skrivstore-panel && npm install'); | ||
| } |
There was a problem hiding this comment.
resolveBetterSqlite probes .skrivstore-panel and .skrivstore — directories that exist only on the author's machine. Any user without those paths falls through to require('better-sqlite3'), which then fails with an error pointing back to the same internal paths: cd ~/.skrivstore-panel && npm install. This is useless to any external user. The error message should give actionable general-purpose install guidance instead.
Prompt To Fix With AI
This is a comment left during a code review.
Path: skills/shared-memory/lib/store.js
Line: 13-23
Comment:
**Hardcoded internal paths make this unusable outside the author's environment**
`resolveBetterSqlite` probes `.skrivstore-panel` and `.skrivstore` — directories that exist only on the author's machine. Any user without those paths falls through to `require('better-sqlite3')`, which then fails with an error pointing back to the same internal paths: `cd ~/.skrivstore-panel && npm install`. This is useless to any external user. The error message should give actionable general-purpose install guidance instead.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+64
to
+81
| // Reflexion — surface top-K relevant gotchas for current work (Q5) | ||
| // Scans ~/projects/skrivsikkert/.beads/knowledge/{gotchas,anti-patterns}.jsonl | ||
| let reflexions = []; | ||
| try { | ||
| const rxPath = path.resolve(process.env.HOME, 'projects/skrivsikkert/agent_ops/scripts/lib/reflexion.js'); | ||
| if (require('fs').existsSync(rxPath)) { | ||
| const { Reflexion } = require(rxPath); | ||
| const rx = new Reflexion(); | ||
| // Without a specific task prompt, surface critical-severity items | ||
| const all = [ | ||
| ...rx._readJsonl('anti-patterns.jsonl'), | ||
| ...rx._readJsonl('gotchas.jsonl'), | ||
| ]; | ||
| reflexions = all | ||
| .filter((r) => r.severity === 'critical') | ||
| .slice(-3); | ||
| } | ||
| } catch (_) {} |
There was a problem hiding this comment.
~/projects/skrivsikkert/agent_ops/scripts/lib/reflexion.js is a path on the author's personal machine. The existsSync guard means it silently no-ops for everyone else, but it is dead code for all external users and leaks an internal project name into the codebase. If Reflexion integration is desired, its path should be driven by process.env.REFLEXION_PATH and documented as an optional extension.
Prompt To Fix With AI
This is a comment left during a code review.
Path: skills/shared-memory/hooks/session-start-memory.js
Line: 64-81
Comment:
**Hardcoded private project path shipped in a public skill**
`~/projects/skrivsikkert/agent_ops/scripts/lib/reflexion.js` is a path on the author's personal machine. The `existsSync` guard means it silently no-ops for everyone else, but it is dead code for all external users and leaks an internal project name into the codebase. If Reflexion integration is desired, its path should be driven by `process.env.REFLEXION_PATH` and documented as an optional extension.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+74
to
+98
| const existing = store.get({ scope: 'global', key }); | ||
| if (existing) { | ||
| const lease = parseLease(existing); | ||
| if (lease && !isExpired(lease) && lease.b.team !== team) { | ||
| die(`already claimed by ${lease.b.team} until ${new Date(lease.b.expires_at).toISOString()} (use --as ${lease.b.team} to extend, or wait)`); | ||
| } | ||
| } | ||
|
|
||
| const body = { | ||
| issue, | ||
| team, | ||
| note: args.note || '', | ||
| claimed_at: Date.now(), | ||
| expires_at: Date.now() + minutes * 60000, | ||
| renewed: existing ? ((parseLease(existing)?.b.renewed || 0) + 1) : 0, | ||
| }; | ||
|
|
||
| store.put({ | ||
| scope: 'global', type: 'episodic', key, | ||
| title: `[lease ${team}] ${issue}`, | ||
| body: JSON.stringify(body, null, 2), | ||
| tags: ['lease', team, issue], | ||
| team, source: 'agent-team', | ||
| }); | ||
| console.log(`claimed ${issue} for ${team}, expires ${new Date(body.expires_at).toISOString()}`); |
There was a problem hiding this comment.
The check-then-write in cmd_claim is not atomic. Two concurrent agent-team claim AGE-123 calls both execute store.get(...) before either calls store.put(...), both find no existing lease, and both proceed to create one. The last write wins silently — no error, no detection, two teams own the same issue. better-sqlite3 supports synchronous transactions; the read + conditional write must be wrapped in a single db.transaction(...) call.
Prompt To Fix With AI
This is a comment left during a code review.
Path: skills/shared-memory/bin/agent-team
Line: 74-98
Comment:
**Race condition defeats the lease exclusivity guarantee**
The check-then-write in `cmd_claim` is not atomic. Two concurrent `agent-team claim AGE-123` calls both execute `store.get(...)` before either calls `store.put(...)`, both find no existing lease, and both proceed to create one. The last write wins silently — no error, no detection, two teams own the same issue. `better-sqlite3` supports synchronous transactions; the read + conditional write must be wrapped in a single `db.transaction(...)` call.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+49
to
+52
| function allLeases() { | ||
| return store.search({ query: 'lease', scopes: ['global'], types: ['episodic'], limit: 500 }) | ||
| .filter((e) => e.key && e.key.startsWith('lease:')); | ||
| } |
There was a problem hiding this comment.
allLeases() fetches with limit: 500 via FTS. If the DB accumulates more than 500 lease records, active leases will be silently missing from list, expire, and status. Querying entry directly by key prefix would be more reliable than relying on FTS keyword matching.
Prompt To Fix With AI
This is a comment left during a code review.
Path: skills/shared-memory/bin/agent-team
Line: 49-52
Comment:
**Hardcoded FTS limit silently drops leases beyond 500**
`allLeases()` fetches with `limit: 500` via FTS. If the DB accumulates more than 500 lease records, active leases will be silently missing from `list`, `expire`, and `status`. Querying `entry` directly by key prefix would be more reliable than relying on FTS keyword matching.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+86
to
+98
| try { | ||
| const since = Date.now() - 48 * 3600 * 1000; | ||
| completions = store.search({ query: 'completion', scopes: ['global'], types: ['episodic'], limit: 100 }) | ||
| .filter((e) => e.key && e.key.startsWith('completion:') && e.updated_at >= since) | ||
| .map((e) => { try { return { e, b: JSON.parse(e.body) }; } catch { return null; } }) | ||
| .filter(Boolean) | ||
| .filter((x) => { | ||
| if (!projectBase || !x.b.project) return true; | ||
| const bp = path.basename(x.b.project); | ||
| return bp === projectBase; | ||
| }) | ||
| .slice(0, 5); | ||
| } catch (_) {} |
There was a problem hiding this comment.
completions loads up to 100 records from the DB and then filters by e.updated_at >= since in JavaScript. As the DB grows this loads increasingly stale entries unnecessarily. The since cutoff belongs in the SQL WHERE clause.
Prompt To Fix With AI
This is a comment left during a code review.
Path: skills/shared-memory/hooks/session-start-memory.js
Line: 86-98
Comment:
**Timestamp filter should be pushed into SQL, not applied in JS**
`completions` loads up to 100 records from the DB and then filters by `e.updated_at >= since` in JavaScript. As the DB grows this loads increasingly stale entries unnecessarily. The `since` cutoff belongs in the SQL `WHERE` clause.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+49
to
+55
| function resolveTeam() { | ||
| if (process.env.SS_SYNC_TEAM) return process.env.SS_SYNC_TEAM; | ||
| try { | ||
| const cfg = require(path.resolve(process.env.HOME, '.skrivstore/lib/config.js')); | ||
| return cfg.resolveTeamAndProject().team; | ||
| } catch (_) { return null; } | ||
| } |
There was a problem hiding this comment.
resolveTeam() duplicated across all three CLIs
The exact same function body appears in bin/memory (line 49), bin/handoff (line 31), and bin/agent-team (line 39). Extract it into lib/resolve-team.js and require it from each CLI so a bug fix or env-var change only needs to happen once.
Prompt To Fix With AI
This is a comment left during a code review.
Path: skills/shared-memory/bin/memory
Line: 49-55
Comment:
**`resolveTeam()` duplicated across all three CLIs**
The exact same function body appears in `bin/memory` (line 49), `bin/handoff` (line 31), and `bin/agent-team` (line 39). Extract it into `lib/resolve-team.js` and `require` it from each CLI so a bug fix or env-var change only needs to happen once.
How can I resolve this? If you propose a fix, please make it concise.![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
skills/shared-memory/lib/store.js (1)
1-158:⚠️ Potential issue | 🟡 MinorMissing tests for the store contract.
Per the repo's TDD guideline, new
.jsfiles should land with failing-then-passing tests.store.jsis the shared contract for three CLIs and a hook; it's the file that most benefits from coverage (upsert path, FTS trigger sync on UPDATE, archive filtering, empty-query search, scope normalization). Please add at least smoke-level tests before merging; I'm happy to scaffold them if useful.As per coding guidelines: "TDD is mandatory - Write tests first, watch them fail, then implement".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/shared-memory/lib/store.js` around lines 1 - 158, Add a new test file that sets process.env.SHARED_MEMORY_HOME to a temporary directory, requires the module to get a fresh DB, and asserts the store contract: use put() to create and upsert (same scope+key) entries and assert get() returns updated id/fields and stats() updates; verify FTS sync by inserting an entry with title/body, updating it via put() and asserting search({ query: <new text> }) finds the updated row (entry_fts trigger on UPDATE); test archive() and search()/recent() to ensure archived entries are filtered out (includeArchived toggles), and verify normalizeScope behavior by passing plain project names vs 'project:<name>' in put()/get()/search(); keep tests minimal (smoke-level) and ensure DB is isolated/cleaned between tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@skills/shared-memory/bin/agent-team`:
- Around line 101-121: The ownership check in cmd_release uses the possibly
spoofed team value (team = args.as || resolveTeam()), allowing anyone to bypass
the check by passing --as; change the check to compare lease.b.team against the
resolved owner (call resolveTeam() once into a variable like resolvedTeam) and
require args.force only when lease.b.team !== resolvedTeam, while still allowing
args.as to be used only for the recorded team in the stored body/title/tags
(leave body creation using args.as or resolvedTeam as desired) so that
lease.b.team !== resolvedTeam triggers die(...) unless --force is provided.
- Around line 68-99: cmd_claim is copying the previous lease's renewed counter
even when the prior lease belonged to a different team; change the renewed
calculation to reset when parseLease(existing)?.b.team !== team (i.e., if
existing lease's team differs, set renewed to 0, otherwise increment as now).
Locate the renewed computation in cmd_claim (uses existing and
parseLease(existing)) and update it to conditionally inherit only when the prior
lease team equals the new team; optionally add a prior_team field to the stored
body if you want auditability. Also consider adding a comment near the
store.get/store.put sequence documenting the TOCTOU race (or wrap claim logic in
a transactional/locked write if stronger guarantees are later required).
In `@skills/shared-memory/bin/handoff`:
- Around line 81-98: The inbox currently pulls a 200-row FTS set via
allHandoffs() and then slices to limit, so acked handoffs can consume the cap
and hide pending items; update cmd_inbox to filter out acked items (b.ack)
before applying slice/limit (or perform a key-prefix scan instead of FTS) so
pending handoffs are not silently truncated, and also make pending vs acked
visually distinct (e.g., sort by b.ack so pending first or omit acked by default
and add a --all flag) — locate cmd_inbox, the allHandoffs() mapping that
produces {e,b}, the .slice(0, limit) call, and the console lines that print
b.ack to implement these changes.
- Around line 39-74: cmd_send currently uses resolveTeam() and will persist
'team-unknown' when the environment/config is missing; refuse to send in that
case and require an explicit override via an --as option. Update cmd_send to
accept and prefer args.as (like agent-team does) or fall back to resolveTeam();
if the resolved team is 'team-unknown' and args.as is not provided, exit with an
error message instead of calling store.put. Ensure the values written to
body.from, store.put.team and the tags array use the final resolved team
(args.as || resolved) so no handoffs are stored with 'team-unknown'.
In `@skills/shared-memory/bin/memory`:
- Around line 49-55: resolveTeam currently returns null causing inconsistent
team values across tools; change resolveTeam to return the same fallback used by
the other CLIs (return 'team-unknown' instead of null) and update the caller to
rely on that string; then extract the duplicated parseArgs, resolveTeam, and die
logic into a shared cli.js module (export functions parseArgs, resolveTeam, die)
and have the three scripts import and use those exports so all CLIs behave
identically.
In `@skills/shared-memory/hooks/session-start-memory.js`:
- Around line 64-81: The Reflexion block in session-start-memory.js uses a
hardcoded rxPath and calls a private method rx._readJsonl, which is not reusable
and unsafe; replace this by gating activation behind an explicit environment
variable (e.g. SS_REFLEXION_LIB) and resolve the path from that variable (only
proceed if SS_REFLEXION_LIB is set and exists), require the module, instantiate
Reflexion, and call its documented public API (not _readJsonl) to fetch items;
alternatively remove the entire Reflexion block and convert it into a separate
opt-in hook/repo if you prefer not to support it here.
- Around line 140-149: The current truncation may cut mid-line because
ctx.slice(0, CAP) can split a heading or bullet; change emit logic to truncate
only at a newline or by whole parts entries: build ctx as before but when
ctx.length > CAP, take the first CAP bytes and backtrack to the last '\n' (or
prefer iteratively append items from parts until adding the next would exceed
CAP) and then call emit(truncated + '\n…(truncated)'); update references to CAP,
ctx, parts and emit accordingly and keep store.DB_PATH usage unchanged.
- Around line 19-28: The module calls emit('') to bail out when shared memory is
disabled or store loading fails but continues execution, which can leave store
undefined; modify the top-level flow to explicitly stop execution after emit by
adding a return immediately after each emit('') call (both the early
process.env.SS_MEMORY_DISABLE branch and the catch block after requiring store)
or alternatively wrap the remaining module logic in if (store) { ... } (or a
top-level try { /* main body using store and store.search */ } catch (e) {
emit(''); }) so that any failure during startup reliably emits and prevents
subsequent use of an undefined store.
In `@skills/shared-memory/lib/store.js`:
- Around line 104-129: The search function forces FTS5 phrase matching by
wrapping the user query in quotes before passing it to entry_fts MATCH; remove
the quoting and instead pass the raw/escaped FTS query (or implement
tokenization/OR-joining) so boolean and prefix operators work (update the clause
in search that builds params for entry_fts MATCH and the safeQ handling), and
for deterministic key-based lookups (e.g. handoff/lease/completion CLI hooks) do
not use FTS at all — add a separate clause that matches e.key LIKE 'handoff:%'
or exact scope/prefix checks (use normalizeScope where appropriate) to keep
those queries indexable and reliable.
- Around line 97-102: The get function currently returns archived rows when
called with get({id}) but filters archived for get({scope,key}); fix by adding
an includeArchived boolean parameter (default false) to get and change the id
branch to respect it (use "SELECT * FROM entry WHERE id = ? AND archived = 0"
when includeArchived is false) while keeping the scope/key branch using
normalizeScope(scope) and the existing "archived = 0" behavior unless
includeArchived is true; update callers where the old behavior (showing archived
by id) is intentionally needed to pass includeArchived: true.
- Around line 75-95: The upsert in function put silently ignores changes to type
for an existing (scope,key) row; modify the existing lookup and update logic so
that when an existing row is found (existing from db.prepare('SELECT id FROM
entry ...') in put) you SELECT the current type (e.g., SELECT id,type ...) and
then either 1) if types differ include type in the UPDATE (add type=? to the
UPDATE SQL and pass the new type value) so the row is overwritten, or 2)
explicitly reject the operation by throwing an Error when the new type !==
existing.type; update the db.prepare call and the parameters for the UPDATE in
put accordingly so type mismatches are handled deterministically.
- Around line 9-23: SHARED_HOME and resolveBetterSqlite leak personal paths and
crash when HOME is undefined; fix by using os.homedir() with a safe fallback
(e.g., process.cwd() or os.tmpdir()) to compute SHARED_HOME/DB_PATH, and
simplify resolveBetterSqlite to stop probing hardcoded "~/.skrivstore*"
locations — instead try require('better-sqlite3') (and any module-relative
lookup if needed) and when missing throw a neutral message asking to install the
dependency in the project (no personal path guidance). Update symbols:
SHARED_HOME and DB_PATH to use const homedir = os.homedir() || process.cwd() ||
os.tmpdir(); const SHARED_HOME = process.env.SHARED_MEMORY_HOME ||
path.resolve(homedir, '.claude/shared-memory'); and modify resolveBetterSqlite
to remove candidate paths and the personalized error text, replacing it with a
generic "better-sqlite3 not found; run npm install better-sqlite3" instruction.
In `@skills/shared-memory/SKILL.md`:
- Around line 22-35: The install snippet in SKILL.md will fail on a fresh
machine because the target directories for the cp commands may not exist; update
the installation steps so before copying files (the cp commands that copy
lib/store.js, bin/memory, bin/handoff, bin/agent-team, and
hooks/session-start-memory.js) you create the required directories using mkdir
-p for ~/.claude/shared-memory/lib, ~/bin, and ~/.claude/hooks so the subsequent
cp operations succeed.
- Line 3: The SKILL.md description uses backtick names (`memory`,
`shared-memory`) instead of the repository's required $name invocation syntax;
update all references in this file (notably the description line and the
comparison table lines ~9-10 and ~84-92) to use the $name form (e.g., $memory,
$shared-memory) consistent with the SKILL.md `name` field so agents invoke the
sibling skills correctly.
---
Outside diff comments:
In `@skills/shared-memory/lib/store.js`:
- Around line 1-158: Add a new test file that sets
process.env.SHARED_MEMORY_HOME to a temporary directory, requires the module to
get a fresh DB, and asserts the store contract: use put() to create and upsert
(same scope+key) entries and assert get() returns updated id/fields and stats()
updates; verify FTS sync by inserting an entry with title/body, updating it via
put() and asserting search({ query: <new text> }) finds the updated row
(entry_fts trigger on UPDATE); test archive() and search()/recent() to ensure
archived entries are filtered out (includeArchived toggles), and verify
normalizeScope behavior by passing plain project names vs 'project:<name>' in
put()/get()/search(); keep tests minimal (smoke-level) and ensure DB is
isolated/cleaned between tests.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 03fe85ea-4938-4a8e-aba8-3c721cc18e5c
📒 Files selected for processing (6)
skills/shared-memory/SKILL.mdskills/shared-memory/bin/agent-teamskills/shared-memory/bin/handoffskills/shared-memory/bin/memoryskills/shared-memory/hooks/session-start-memory.jsskills/shared-memory/lib/store.js
Comment on lines
+68
to
+99
| function cmd_claim(args) { | ||
| const issue = issueFromArgs(args); | ||
| const team = args.as || resolveTeam(); | ||
| const minutes = args.for ? Number(args.for) : 60; | ||
| const key = `lease:${issue}`; | ||
|
|
||
| const existing = store.get({ scope: 'global', key }); | ||
| if (existing) { | ||
| const lease = parseLease(existing); | ||
| if (lease && !isExpired(lease) && lease.b.team !== team) { | ||
| die(`already claimed by ${lease.b.team} until ${new Date(lease.b.expires_at).toISOString()} (use --as ${lease.b.team} to extend, or wait)`); | ||
| } | ||
| } | ||
|
|
||
| const body = { | ||
| issue, | ||
| team, | ||
| note: args.note || '', | ||
| claimed_at: Date.now(), | ||
| expires_at: Date.now() + minutes * 60000, | ||
| renewed: existing ? ((parseLease(existing)?.b.renewed || 0) + 1) : 0, | ||
| }; | ||
|
|
||
| store.put({ | ||
| scope: 'global', type: 'episodic', key, | ||
| title: `[lease ${team}] ${issue}`, | ||
| body: JSON.stringify(body, null, 2), | ||
| tags: ['lease', team, issue], | ||
| team, source: 'agent-team', | ||
| }); | ||
| console.log(`claimed ${issue} for ${team}, expires ${new Date(body.expires_at).toISOString()}`); | ||
| } |
There was a problem hiding this comment.
renewed counter inherits from a prior team's expired lease.
On line 88, if the previous lease was held by a different team (and has since expired), the new claim inherits that team's renewed counter and increments it — e.g. team-2 claiming after team-1's 4th renewal records renewed: 5 for team-2. Either reset on team change or also track prior_team:
- renewed: existing ? ((parseLease(existing)?.b.renewed || 0) + 1) : 0,
+ renewed: (() => {
+ const prev = existing && parseLease(existing)?.b;
+ if (prev && prev.team === team && !isExpired({ b: prev })) return (prev.renewed || 0) + 1;
+ return 0;
+ })(),Also note: there's a TOCTOU window between store.get on line 74 and store.put on line 91 — two concurrent claim calls on the same issue can both succeed. SQLite itself serializes writes, but the logic doesn't re-check ownership inside a transaction. For a best-effort coordination tool this is probably acceptable; worth documenting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@skills/shared-memory/bin/agent-team` around lines 68 - 99, cmd_claim is
copying the previous lease's renewed counter even when the prior lease belonged
to a different team; change the renewed calculation to reset when
parseLease(existing)?.b.team !== team (i.e., if existing lease's team differs,
set renewed to 0, otherwise increment as now). Locate the renewed computation in
cmd_claim (uses existing and parseLease(existing)) and update it to
conditionally inherit only when the prior lease team equals the new team;
optionally add a prior_team field to the stored body if you want auditability.
Also consider adding a comment near the store.get/store.put sequence documenting
the TOCTOU race (or wrap claim logic in a transactional/locked write if stronger
guarantees are later required).
Comment on lines
+101
to
+121
| function cmd_release(args) { | ||
| const issue = issueFromArgs(args); | ||
| const team = args.as || resolveTeam(); | ||
| const key = `lease:${issue}`; | ||
| const e = store.get({ scope: 'global', key }); | ||
| if (!e) die(`no lease on ${issue}`); | ||
| const lease = parseLease(e); | ||
| if (lease && lease.b.team !== team && !args.force) { | ||
| die(`lease owned by ${lease.b.team}, not ${team} (use --force to override)`); | ||
| } | ||
| // Mark as released by expiring immediately. Preserves history. | ||
| const body = { ...(lease?.b || {}), released_at: Date.now(), expires_at: Date.now() - 1 }; | ||
| store.put({ | ||
| scope: 'global', type: 'episodic', key, | ||
| title: `[lease released ${team}] ${issue}`, | ||
| body: JSON.stringify(body, null, 2), | ||
| tags: ['lease', 'released', team, issue], | ||
| team, source: 'agent-team', | ||
| }); | ||
| console.log(`released ${issue}`); | ||
| } |
There was a problem hiding this comment.
release --as <owner> bypasses the ownership check without --force.
team is computed as args.as || resolveTeam() and then the check is lease.b.team !== team. So anyone can release another team's lease by just passing --as <that-team> — --force is never required. That defeats the purpose of the ownership gate.
Use the resolved team for the ownership check, and only let --as influence the written record (if that's even desired):
function cmd_release(args) {
const issue = issueFromArgs(args);
- const team = args.as || resolveTeam();
+ const me = resolveTeam();
+ const team = args.as || me;
const key = `lease:${issue}`;
const e = store.get({ scope: 'global', key });
if (!e) die(`no lease on ${issue}`);
const lease = parseLease(e);
- if (lease && lease.b.team !== team && !args.force) {
- die(`lease owned by ${lease.b.team}, not ${team} (use --force to override)`);
+ if (lease && lease.b.team !== me && !args.force) {
+ die(`lease owned by ${lease.b.team}, not ${me} (use --force to override)`);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function cmd_release(args) { | |
| const issue = issueFromArgs(args); | |
| const team = args.as || resolveTeam(); | |
| const key = `lease:${issue}`; | |
| const e = store.get({ scope: 'global', key }); | |
| if (!e) die(`no lease on ${issue}`); | |
| const lease = parseLease(e); | |
| if (lease && lease.b.team !== team && !args.force) { | |
| die(`lease owned by ${lease.b.team}, not ${team} (use --force to override)`); | |
| } | |
| // Mark as released by expiring immediately. Preserves history. | |
| const body = { ...(lease?.b || {}), released_at: Date.now(), expires_at: Date.now() - 1 }; | |
| store.put({ | |
| scope: 'global', type: 'episodic', key, | |
| title: `[lease released ${team}] ${issue}`, | |
| body: JSON.stringify(body, null, 2), | |
| tags: ['lease', 'released', team, issue], | |
| team, source: 'agent-team', | |
| }); | |
| console.log(`released ${issue}`); | |
| } | |
| function cmd_release(args) { | |
| const issue = issueFromArgs(args); | |
| const me = resolveTeam(); | |
| const team = args.as || me; | |
| const key = `lease:${issue}`; | |
| const e = store.get({ scope: 'global', key }); | |
| if (!e) die(`no lease on ${issue}`); | |
| const lease = parseLease(e); | |
| if (lease && lease.b.team !== me && !args.force) { | |
| die(`lease owned by ${lease.b.team}, not ${me} (use --force to override)`); | |
| } | |
| // Mark as released by expiring immediately. Preserves history. | |
| const body = { ...(lease?.b || {}), released_at: Date.now(), expires_at: Date.now() - 1 }; | |
| store.put({ | |
| scope: 'global', type: 'episodic', key, | |
| title: `[lease released ${team}] ${issue}`, | |
| body: JSON.stringify(body, null, 2), | |
| tags: ['lease', 'released', team, issue], | |
| team, source: 'agent-team', | |
| }); | |
| console.log(`released ${issue}`); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@skills/shared-memory/bin/agent-team` around lines 101 - 121, The ownership
check in cmd_release uses the possibly spoofed team value (team = args.as ||
resolveTeam()), allowing anyone to bypass the check by passing --as; change the
check to compare lease.b.team against the resolved owner (call resolveTeam()
once into a variable like resolvedTeam) and require args.force only when
lease.b.team !== resolvedTeam, while still allowing args.as to be used only for
the recorded team in the stored body/title/tags (leave body creation using
args.as or resolvedTeam as desired) so that lease.b.team !== resolvedTeam
triggers die(...) unless --force is provided.
Comment on lines
+39
to
+74
| function cmd_send(args) { | ||
| if (!args.to) die('--to required (team-1|team-2)'); | ||
| if (!args.title) die('--title required'); | ||
| const from = resolveTeam(); | ||
| const id = `${Date.now().toString(36)}-${Math.random().toString(36).slice(2, 6)}`; | ||
| const key = `handoff:${id}`; | ||
| const locked = typeof args.locked === 'string' | ||
| ? args.locked.split(',').map((s) => s.trim()).filter(Boolean) : []; | ||
| const next = typeof args.next === 'string' | ||
| ? args.next.split('|').map((s) => s.trim()).filter(Boolean) : []; | ||
|
|
||
| const body = { | ||
| from, | ||
| to: args.to, | ||
| phase: args.phase || 'handoff', | ||
| issue: args.issue || null, | ||
| project: args.project || null, | ||
| summary: args.summary || '', | ||
| locked_files: locked, | ||
| next_steps: next, | ||
| ack: false, | ||
| created_at: new Date().toISOString(), | ||
| }; | ||
|
|
||
| store.put({ | ||
| scope: 'global', | ||
| type: 'episodic', | ||
| key, | ||
| title: `[handoff ${from}→${args.to}] ${args.title}`, | ||
| body: JSON.stringify(body, null, 2), | ||
| tags: ['handoff', from, args.to, ...(args.phase ? [args.phase] : [])], | ||
| team: from, | ||
| source: 'handoff', | ||
| }); | ||
| console.log(`sent ${key} (${from} → ${args.to})`); | ||
| } |
There was a problem hiding this comment.
send with unresolved team writes team-unknown as the sender.
When neither SS_SYNC_TEAM nor the skrivstore config is available, resolveTeam() returns 'team-unknown', and that value is persisted as from, team, and a tag on the handoff. The receiving side then sees a handoff "from team-unknown", and any inbox filtered by --team team-unknown will aggregate all such records. For a cross-team coordination tool, silently sending as an unknown identity is a footgun. Consider refusing to send without --as or a resolved team:
if (!args.title) die('--title required');
- const from = resolveTeam();
+ const from = args.as || resolveTeam();
+ if (from === 'team-unknown') {
+ die('cannot resolve sending team; set SS_SYNC_TEAM or pass --as <team>');
+ }(You'll want to add an --as switch to the CLI; currently only agent-team has it.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@skills/shared-memory/bin/handoff` around lines 39 - 74, cmd_send currently
uses resolveTeam() and will persist 'team-unknown' when the environment/config
is missing; refuse to send in that case and require an explicit override via an
--as option. Update cmd_send to accept and prefer args.as (like agent-team does)
or fall back to resolveTeam(); if the resolved team is 'team-unknown' and
args.as is not provided, exit with an error message instead of calling
store.put. Ensure the values written to body.from, store.put.team and the tags
array use the final resolved team (args.as || resolved) so no handoffs are
stored with 'team-unknown'.
Comment on lines
+81
to
+98
| function cmd_inbox(args) { | ||
| const target = args.team || resolveTeam(); | ||
| const limit = args.limit ? Number(args.limit) : 10; | ||
| const rows = allHandoffs() | ||
| .map((e) => { try { return { e, b: JSON.parse(e.body) }; } catch { return null; } }) | ||
| .filter((x) => x && x.b.to === target) | ||
| .slice(0, limit); | ||
| if (!rows.length) { console.log(`(no handoffs for ${target})`); return; } | ||
| for (const { e, b } of rows) { | ||
| const age = Math.round((Date.now() - e.updated_at) / 60000); | ||
| console.log(`${e.key} ${b.ack ? '✓' : '○'} ${b.from}→${b.to} ${age}m phase=${b.phase}`); | ||
| console.log(` ${e.title.replace(/^\[handoff [^\]]+\]\s*/, '')}`); | ||
| if (b.summary) console.log(` summary: ${b.summary.slice(0, 160)}`); | ||
| if (b.locked_files?.length) console.log(` locked: ${b.locked_files.join(', ')}`); | ||
| if (b.next_steps?.length) console.log(` next: ${b.next_steps.map((s) => '• ' + s).join(' ')}`); | ||
| console.log(''); | ||
| } | ||
| } |
There was a problem hiding this comment.
inbox mixes acked and pending handoffs, and silently truncates at 200.
Two small issues worth addressing together:
allHandoffs()pulls 200 via FTS and theninboxslices 10 post-filter. Old acked handoffs count against the 200 cap, so on an active shared DB a pending handoff can fall off the list. Filter on!b.ackbefore slicing, and/or use a key-prefix scan rather than FTS.- The printed list doesn't distinguish pending from acked — easy to miss. Consider sorting pending first, or filtering acked out by default with a
--allflag likeagent-team list.
- const rows = allHandoffs()
- .map((e) => { try { return { e, b: JSON.parse(e.body) }; } catch { return null; } })
- .filter((x) => x && x.b.to === target)
- .slice(0, limit);
+ const rows = allHandoffs()
+ .map((e) => { try { return { e, b: JSON.parse(e.body) }; } catch { return null; } })
+ .filter((x) => x && x.b.to === target && (args.all || !x.b.ack))
+ .sort((a, b) => Number(a.b.ack) - Number(b.b.ack) || b.e.updated_at - a.e.updated_at)
+ .slice(0, limit);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function cmd_inbox(args) { | |
| const target = args.team || resolveTeam(); | |
| const limit = args.limit ? Number(args.limit) : 10; | |
| const rows = allHandoffs() | |
| .map((e) => { try { return { e, b: JSON.parse(e.body) }; } catch { return null; } }) | |
| .filter((x) => x && x.b.to === target) | |
| .slice(0, limit); | |
| if (!rows.length) { console.log(`(no handoffs for ${target})`); return; } | |
| for (const { e, b } of rows) { | |
| const age = Math.round((Date.now() - e.updated_at) / 60000); | |
| console.log(`${e.key} ${b.ack ? '✓' : '○'} ${b.from}→${b.to} ${age}m phase=${b.phase}`); | |
| console.log(` ${e.title.replace(/^\[handoff [^\]]+\]\s*/, '')}`); | |
| if (b.summary) console.log(` summary: ${b.summary.slice(0, 160)}`); | |
| if (b.locked_files?.length) console.log(` locked: ${b.locked_files.join(', ')}`); | |
| if (b.next_steps?.length) console.log(` next: ${b.next_steps.map((s) => '• ' + s).join(' ')}`); | |
| console.log(''); | |
| } | |
| } | |
| function cmd_inbox(args) { | |
| const target = args.team || resolveTeam(); | |
| const limit = args.limit ? Number(args.limit) : 10; | |
| const rows = allHandoffs() | |
| .map((e) => { try { return { e, b: JSON.parse(e.body) }; } catch { return null; } }) | |
| .filter((x) => x && x.b.to === target && (args.all || !x.b.ack)) | |
| .sort((a, b) => Number(a.b.ack) - Number(b.b.ack) || b.e.updated_at - a.e.updated_at) | |
| .slice(0, limit); | |
| if (!rows.length) { console.log(`(no handoffs for ${target})`); return; } | |
| for (const { e, b } of rows) { | |
| const age = Math.round((Date.now() - e.updated_at) / 60000); | |
| console.log(`${e.key} ${b.ack ? '✓' : '○'} ${b.from}→${b.to} ${age}m phase=${b.phase}`); | |
| console.log(` ${e.title.replace(/^\[handoff [^\]]+\]\s*/, '')}`); | |
| if (b.summary) console.log(` summary: ${b.summary.slice(0, 160)}`); | |
| if (b.locked_files?.length) console.log(` locked: ${b.locked_files.join(', ')}`); | |
| if (b.next_steps?.length) console.log(` next: ${b.next_steps.map((s) => '• ' + s).join(' ')}`); | |
| console.log(''); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@skills/shared-memory/bin/handoff` around lines 81 - 98, The inbox currently
pulls a 200-row FTS set via allHandoffs() and then slices to limit, so acked
handoffs can consume the cap and hide pending items; update cmd_inbox to filter
out acked items (b.ack) before applying slice/limit (or perform a key-prefix
scan instead of FTS) so pending handoffs are not silently truncated, and also
make pending vs acked visually distinct (e.g., sort by b.ack so pending first or
omit acked by default and add a --all flag) — locate cmd_inbox, the
allHandoffs() mapping that produces {e,b}, the .slice(0, limit) call, and the
console lines that print b.ack to implement these changes.
Comment on lines
+49
to
+55
| function resolveTeam() { | ||
| if (process.env.SS_SYNC_TEAM) return process.env.SS_SYNC_TEAM; | ||
| try { | ||
| const cfg = require(path.resolve(process.env.HOME, '.skrivstore/lib/config.js')); | ||
| return cfg.resolveTeamAndProject().team; | ||
| } catch (_) { return null; } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
resolveTeam fallback inconsistent with the other two CLIs.
bin/handoff and bin/agent-team both fall back to 'team-unknown'; this one returns null. The resulting team column in the DB will be a mix of NULL and 'team-unknown' depending on which tool wrote the entry, which breaks any filter/aggregate on team. Pick one behavior (returning null is arguably safer — the three files should share a single helper) and keep it consistent.
- } catch (_) { return null; }
+ } catch (_) { return null; /* aligned with handoff/agent-team — consider extracting to lib/team.js */ }Ideally extract parseArgs + resolveTeam + die into skills/shared-memory/lib/cli.js and reuse from all three scripts (currently ~40 lines of copy-paste across bin/memory, bin/handoff, bin/agent-team).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@skills/shared-memory/bin/memory` around lines 49 - 55, resolveTeam currently
returns null causing inconsistent team values across tools; change resolveTeam
to return the same fallback used by the other CLIs (return 'team-unknown'
instead of null) and update the caller to rely on that string; then extract the
duplicated parseArgs, resolveTeam, and die logic into a shared cli.js module
(export functions parseArgs, resolveTeam, die) and have the three scripts import
and use those exports so all CLIs behave identically.
Comment on lines
+75
to
+95
| function put({ scope = 'global', type, key = null, title, body = '', tags = [], team = null, source = 'cli', pinned = false }) { | ||
| if (!title) throw new Error('title required'); | ||
| if (!['semantic', 'episodic', 'procedural'].includes(type)) throw new Error('type must be semantic|episodic|procedural'); | ||
| const db = getDb(); | ||
| const now = Date.now(); | ||
| const tagStr = Array.isArray(tags) ? tags.join(',') : String(tags || ''); | ||
| scope = normalizeScope(scope); | ||
|
|
||
| if (key) { | ||
| const existing = db.prepare('SELECT id FROM entry WHERE scope = ? AND key = ? AND archived = 0').get(scope, key); | ||
| if (existing) { | ||
| db.prepare(`UPDATE entry SET title=?, body=?, tags=?, team=COALESCE(?, team), source=?, updated_at=?, pinned=? WHERE id=?`) | ||
| .run(title, body, tagStr, team, source, now, pinned ? 1 : 0, existing.id); | ||
| return { ok: true, id: existing.id, updated: true }; | ||
| } | ||
| } | ||
| const r = db.prepare(`INSERT INTO entry(scope,type,key,title,body,tags,team,source,created_at,updated_at,pinned) | ||
| VALUES (?,?,?,?,?,?,?,?,?,?,?)`) | ||
| .run(scope, type, key, title, body, tagStr, team, source, now, now, pinned ? 1 : 0); | ||
| return { ok: true, id: r.lastInsertRowid, updated: false }; | ||
| } |
There was a problem hiding this comment.
Upsert silently ignores type change on update.
When (scope, key) already exists, the UPDATE does not write the new type, so memory put --key foo --type semantic ... followed by memory put --key foo --type procedural ... leaves the row as semantic but with the new title/body. This is easy to miss and will drift records across the three type buckets used by the session hook. Either also update type or reject the call with a clear error when it differs from the existing row.
- if (existing) {
- db.prepare(`UPDATE entry SET title=?, body=?, tags=?, team=COALESCE(?, team), source=?, updated_at=?, pinned=? WHERE id=?`)
- .run(title, body, tagStr, team, source, now, pinned ? 1 : 0, existing.id);
+ if (existing) {
+ db.prepare(`UPDATE entry SET type=?, title=?, body=?, tags=?, team=COALESCE(?, team), source=?, updated_at=?, pinned=? WHERE id=?`)
+ .run(type, title, body, tagStr, team, source, now, pinned ? 1 : 0, existing.id);
return { ok: true, id: existing.id, updated: true };
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@skills/shared-memory/lib/store.js` around lines 75 - 95, The upsert in
function put silently ignores changes to type for an existing (scope,key) row;
modify the existing lookup and update logic so that when an existing row is
found (existing from db.prepare('SELECT id FROM entry ...') in put) you SELECT
the current type (e.g., SELECT id,type ...) and then either 1) if types differ
include type in the UPDATE (add type=? to the UPDATE SQL and pass the new type
value) so the row is overwritten, or 2) explicitly reject the operation by
throwing an Error when the new type !== existing.type; update the db.prepare
call and the parameters for the UPDATE in put accordingly so type mismatches are
handled deterministically.
Comment on lines
+97
to
+102
| function get({ id = null, scope = null, key = null }) { | ||
| const db = getDb(); | ||
| if (id) return db.prepare('SELECT * FROM entry WHERE id = ?').get(id); | ||
| if (scope && key) return db.prepare('SELECT * FROM entry WHERE scope = ? AND key = ? AND archived = 0').get(normalizeScope(scope), key); | ||
| return null; | ||
| } |
There was a problem hiding this comment.
get({id}) silently returns archived rows.
get({scope,key}) filters archived = 0, but the id branch does not. Callers that re-read an entry they just archived (or anything that trusts get to respect archival) will get inconsistent behavior. bin/memory's get --id and anything built on top will expose archived rows. Either filter both branches or document the asymmetry and rename.
- if (id) return db.prepare('SELECT * FROM entry WHERE id = ?').get(id);
- if (scope && key) return db.prepare('SELECT * FROM entry WHERE scope = ? AND key = ? AND archived = 0').get(normalizeScope(scope), key);
+ if (id) return db.prepare('SELECT * FROM entry WHERE id = ? AND archived = 0').get(id);
+ if (scope && key) return db.prepare('SELECT * FROM entry WHERE scope = ? AND key = ? AND archived = 0').get(normalizeScope(scope), key);If intentional for bin/memory show, add an includeArchived option instead of the implicit divergence.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@skills/shared-memory/lib/store.js` around lines 97 - 102, The get function
currently returns archived rows when called with get({id}) but filters archived
for get({scope,key}); fix by adding an includeArchived boolean parameter
(default false) to get and change the id branch to respect it (use "SELECT *
FROM entry WHERE id = ? AND archived = 0" when includeArchived is false) while
keeping the scope/key branch using normalizeScope(scope) and the existing
"archived = 0" behavior unless includeArchived is true; update callers where the
old behavior (showing archived by id) is intentionally needed to pass
includeArchived: true.
Comment on lines
+104
to
+129
| function search({ query, scopes = null, types = null, limit = 20, includeArchived = false }) { | ||
| const db = getDb(); | ||
| const clauses = []; | ||
| const params = []; | ||
|
|
||
| if (query && query.trim()) { | ||
| // Use FTS5. Quote to handle punctuation. | ||
| const safeQ = query.replace(/"/g, '""'); | ||
| clauses.push('e.id IN (SELECT rowid FROM entry_fts WHERE entry_fts MATCH ?)'); | ||
| params.push(`"${safeQ}"`); | ||
| } | ||
| if (Array.isArray(scopes) && scopes.length) { | ||
| clauses.push(`e.scope IN (${scopes.map(() => '?').join(',')})`); | ||
| scopes.forEach((s) => params.push(normalizeScope(s))); | ||
| } | ||
| if (Array.isArray(types) && types.length) { | ||
| clauses.push(`e.type IN (${types.map(() => '?').join(',')})`); | ||
| types.forEach((t) => params.push(t)); | ||
| } | ||
| if (!includeArchived) clauses.push('e.archived = 0'); | ||
|
|
||
| const where = clauses.length ? 'WHERE ' + clauses.join(' AND ') : ''; | ||
| const sql = `SELECT e.* FROM entry e ${where} ORDER BY e.pinned DESC, e.updated_at DESC LIMIT ?`; | ||
| params.push(limit); | ||
| return db.prepare(sql).all(...params); | ||
| } |
There was a problem hiding this comment.
FTS5 query is forced into a phrase match — no operators, no prefix search.
Wrapping safeQ in "..." makes every query a phrase match. Consequences:
memory find "auth AND oauth"is interpreted literally, not as boolean AND.- Prefix search (
handoff*) does not work — so the key-prefix queries used bybin/handoff(query: 'handoff'),bin/agent-team(query: 'lease'), andhooks/session-start-memory.js(query: 'completion') rely on those tokens happening to appear intags/title, which is brittle. - Special characters other than
"(e.g.-,:,.) can still trip FTS5 tokenization even inside quotes in some builds.
Two reasonable fixes: (a) pass the query through unchanged and document FTS5 syntax, or (b) tokenize and OR the terms with proper escaping. Either way, the key-based lookups used by the other CLIs should really not go through FTS — they should select on entry.key LIKE 'handoff:%' (or an exact scope/prefix filter) to be deterministic and indexable.
🔧 Minimal improvement: let the user pass real FTS queries, keep prefix lookups out of FTS
- if (query && query.trim()) {
- // Use FTS5. Quote to handle punctuation.
- const safeQ = query.replace(/"/g, '""');
- clauses.push('e.id IN (SELECT rowid FROM entry_fts WHERE entry_fts MATCH ?)');
- params.push(`"${safeQ}"`);
- }
+ if (query && query.trim()) {
+ // FTS5 MATCH; callers pass a valid FTS5 expression (phrases, prefixes, booleans).
+ clauses.push('e.id IN (SELECT rowid FROM entry_fts WHERE entry_fts MATCH ?)');
+ params.push(query.trim());
+ }
+ // Optional key-prefix filter for callers that want deterministic lookups.
+ if (typeof keyPrefix === 'string' && keyPrefix) {
+ clauses.push('e.key LIKE ?');
+ params.push(keyPrefix + '%');
+ }Flagging as minor because current behavior works for the use cases in this PR, but it's a correctness trap waiting for anyone adding search features.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@skills/shared-memory/lib/store.js` around lines 104 - 129, The search
function forces FTS5 phrase matching by wrapping the user query in quotes before
passing it to entry_fts MATCH; remove the quoting and instead pass the
raw/escaped FTS query (or implement tokenization/OR-joining) so boolean and
prefix operators work (update the clause in search that builds params for
entry_fts MATCH and the safeQ handling), and for deterministic key-based lookups
(e.g. handoff/lease/completion CLI hooks) do not use FTS at all — add a separate
clause that matches e.key LIKE 'handoff:%' or exact scope/prefix checks (use
normalizeScope where appropriate) to keep those queries indexable and reliable.
| @@ -0,0 +1,92 @@ | |||
| --- | |||
| name: shared-memory | |||
| description: Cross-project SQLite+FTS5 memory shared between multiple Claude Code installations. Complements the file-based `memory` skill with searchable long-term store and cross-team handoffs. | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Reference sibling skills with the $name syntax.
The description and body refer to the file-based skill as `memory` and to this one as `shared-memory` in the comparison table. Per the repo convention, skill names should use the $name form so agents invoke them correctly.
-description: Cross-project SQLite+FTS5 memory shared between multiple Claude Code installations. Complements the file-based `memory` skill with searchable long-term store and cross-team handoffs.
+description: Cross-project SQLite+FTS5 memory shared between multiple Claude Code installations. Complements the file-based $memory skill with searchable long-term store and cross-team handoffs.
@@
-Cross-project, cross-team memory backed by SQLite+FTS5. Complements the
-file-based `memory` skill — use it when you need:
+Cross-project, cross-team memory backed by SQLite+FTS5. Complements the
+file-based $memory skill — use it when you need:
@@
-| Project-local state, active task | `memory` (markdown files) |
-| Cross-project learnings, facts | `shared-memory` |
-| Handoffs between teams | `shared-memory` (handoff CLI) |
-| Task claim/lease coordination | `shared-memory` (agent-team CLI) |
-| Full-text search across history | `shared-memory` |
+| Project-local state, active task | $memory (markdown files) |
+| Cross-project learnings, facts | $shared-memory |
+| Handoffs between teams | $shared-memory (handoff CLI) |
+| Task claim/lease coordination | $shared-memory (agent-team CLI) |
+| Full-text search across history | $shared-memory |As per coding guidelines: "Invoke skills using the $name syntax as defined in SKILL.md name field".
Also applies to: 9-10, 84-92
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@skills/shared-memory/SKILL.md` at line 3, The SKILL.md description uses
backtick names (`memory`, `shared-memory`) instead of the repository's required
$name invocation syntax; update all references in this file (notably the
description line and the comparison table lines ~9-10 and ~84-92) to use the
$name form (e.g., $memory, $shared-memory) consistent with the SKILL.md `name`
field so agents invoke the sibling skills correctly.
Comment on lines
+22
to
+35
| ```bash | ||
| # 1. Install the store library | ||
| cp lib/store.js ~/.claude/shared-memory/lib/store.js | ||
|
|
||
| # 2. Install CLIs | ||
| cp bin/memory ~/bin/memory | ||
| cp bin/handoff ~/bin/handoff | ||
| cp bin/agent-team ~/bin/agent-team | ||
| chmod +x ~/bin/memory ~/bin/handoff ~/bin/agent-team | ||
|
|
||
| # 3. Install SessionStart hook (optional, surfaces memory at session start) | ||
| cp hooks/session-start-memory.js ~/.claude/hooks/session-start-memory.js | ||
| # then register in ~/.claude/settings.json under "SessionStart" | ||
| ``` |
There was a problem hiding this comment.
Install snippet will fail on a fresh machine — missing mkdir -p.
cp lib/store.js ~/.claude/shared-memory/lib/store.js requires the target directory to already exist; on a clean install it doesn't. Same applies to the hook destination. Add the mkdir -p steps so copy-pasting works end-to-end.
```bash
-# 1. Install the store library
-cp lib/store.js ~/.claude/shared-memory/lib/store.js
+# 1. Install the store library
+mkdir -p ~/.claude/shared-memory/lib ~/bin ~/.claude/hooks
+cp lib/store.js ~/.claude/shared-memory/lib/store.js
# 2. Install CLIs
cp bin/memory ~/bin/memory
cp bin/handoff ~/bin/handoff
cp bin/agent-team ~/bin/agent-team
chmod +x ~/bin/memory ~/bin/handoff ~/bin/agent-team
# 3. Install SessionStart hook (optional, surfaces memory at session start)
cp hooks/session-start-memory.js ~/.claude/hooks/session-start-memory.js
# then register in ~/.claude/settings.json under "SessionStart"
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against the current code and only fix it if needed.
In @skills/shared-memory/SKILL.md around lines 22 - 35, The install snippet in
SKILL.md will fail on a fresh machine because the target directories for the cp
commands may not exist; update the installation steps so before copying files
(the cp commands that copy lib/store.js, bin/memory, bin/handoff,
bin/agent-team, and hooks/session-start-memory.js) you create the required
directories using mkdir -p for ~/.claude/shared-memory/lib, ~/bin, and
~/.claude/hooks so the subsequent cp operations succeed.
</details>
<!-- fingerprinting:phantom:medusa:nectarine:1aae86f2-6ce1-4e91-afad-b16e731b4fe9 -->
<!-- This is an auto-generated comment by CodeRabbit -->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a new
shared-memoryskill that provides cross-project, cross-team memory backed by SQLite+FTS5. Complements the existing file-based memory approaches with:All state lives in a single SQLite file (
~/.claude/shared-memory/learnings.db, WAL mode) — no server, no daemon.What's included
lib/store.jsbin/memorybin/handoffbin/agent-teamhooks/session-start-memory.jsWhen to use vs. the file-based
memoryapproachBoth can run side-by-side — they do not conflict.
Test plan
~/.claude/shared-memorydirectorymemory put/memory find/memory recenthandoff send/handoff inbox/handoff ackagent-team claim/release/list/expire/statusNotes for maintainers
better-sqlite3— the lib probes common install locations (panel, skrivstore) automatically so no hard dependency is introduced for the skill package itself~/.claude/settings.jsonafter copyingauto_activate: false— users invoke explicitly, not every session🤖 Generated with Claude Code