fix: wire up knowledge pipeline for SQLite-based system#16
fix: wire up knowledge pipeline for SQLite-based system#16john-wilmes-luma-health wants to merge 7 commits into
Conversation
After synthesis passes self-assessment, the run subcommand now automatically proceeds to close (tagging, pattern extraction, PHI sanitization) without requiring a separate /investigate <id> close invocation. Tags are applied without confirmation in this auto-close path; interactive close still prompts. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…servers Point datadog, clickup, and mongodb registry entries to their PHI-sanitizing custom server wrappers registered in ~/.claude.json, replacing generic public server notes. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- session-end: auto-promote staged_candidates to entries at session end (previously required manual /learn invocation) - knowledge-consolidate: rewrite to query SQLite DB instead of flat entries/ directory that no longer exists Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAuto-closes investigations by invoking Changes
Sequence Diagram(s)sequenceDiagram
participant Hook as Session Hook (session-end.js)
participant DB as Knowledge DB (knowledge.db)
participant Staged as staged_candidates
participant Entries as entries
participant Logger as Console/Logger
Hook->>DB: open connection
DB->>Staged: query staged_candidates for session
Staged-->>Hook: list of candidate rows
Hook->>DB: query entries first-line snapshots
DB-->>Hook: existing first-lines
loop each candidate
Hook->>Hook: map candidate -> entry (_mapStagedToEntry)
alt first-line duplicate exists
Hook->>Logger: skip promotion (duplicate)
else
Hook->>DB: insert new entry (generate id via _generateEntryId)
DB-->>Hook: success / inserted id
Hook->>Staged: delete promoted staged row
Hook->>Logger: log promotion
end
end
Hook->>DB: close connection
Hook->>Logger: completion message
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://ghub.bbstale.com/avatars/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
templates/hooks/session-end.js (1)
144-145: Redundant condition check.Line 144 checks
!summary && !snippet, but line 145 immediately returnsnullif!snippet. The first condition is redundant since!snippetalone would cover the case where both are empty.Simplify the null checks
function _mapStagedToEntry(row) { const summary = (row.summary || "").trim(); const snippet = (row.context_snippet || "").trim(); - if (!summary && !snippet) return null; if (!snippet) return null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/hooks/session-end.js` around lines 144 - 145, Remove the redundant null-check that tests both variables: delete the line "if (!summary && !snippet) return null;" and keep the single concise early-return "if (!snippet) return null;" so the code relies on the existing snippet check (variables summary and snippet) rather than duplicating the condition.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/knowledge-consolidate.sh`:
- Around line 127-137: The current nested Node invocation that interpolates the
shell variable tool into a command (used to build ENTRIES_JSON and COUNT) can
break on special characters; instead, stop inlining tool into the command and
pass it safely (e.g., via an environment variable or stdin) to the Node script
that parses GROUPS_JSON. Modify the blocks that set ENTRIES_JSON and COUNT to
read GROUPS_JSON from stdin and get the tool name from process.env.TOOL (or
process.argv) so you can call node once without shell-escaping tool; update
references to GROUPS_JSON, ENTRIES_JSON, COUNT and the tool variable
accordingly.
---
Nitpick comments:
In `@templates/hooks/session-end.js`:
- Around line 144-145: Remove the redundant null-check that tests both
variables: delete the line "if (!summary && !snippet) return null;" and keep the
single concise early-return "if (!snippet) return null;" so the code relies on
the existing snippet check (variables summary and snippet) rather than
duplicating the condition.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c0b30f87-c4f0-43c8-a7b6-602d4e8b0cab
📒 Files selected for processing (4)
profiles/combined/skills/investigate/SKILL.mdscripts/knowledge-consolidate.shtemplates/hooks/session-end.jstemplates/registry/mcp-servers.json
…stall Copying hook files to ~/.claude/hooks/ creates drift: any edit to a template requires re-running install.sh to take effect at runtime. Switch all install_file calls for hooks/* to a new install_symlink helper that uses ln -sf, so the installed path is always the live template. Also update the uninstall guard to handle symlinks (-L check alongside -f) so removal works correctly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@install.sh`:
- Around line 374-405: The install_symlink function can accidentally create a
symlink inside an existing directory because ln -sf on a directory targets the
directory rather than replacing it; before each ln -sf (the one in the overwrite
branch and the one in the normal install branch) add an explicit directory check
for dest (e.g. [ -d "$dest" ]) and treat it as a conflict: prompt the user (or
respect FORCE) to either remove the directory (rm -rf "$dest") then create the
symlink, or skip; update the logic around the overwrite branch (where choice o|O
triggers ln -sf) and the normal branch (where ln -sf is used) to handle
directory-dest safely so install_symlink always replaces or skips a directory
rather than creating a symlink inside it.
| install_symlink() { | ||
| local src="$1" | ||
| local dest="$2" | ||
| local label="$3" | ||
|
|
||
| if [ "$DRY_RUN" = true ]; then | ||
| if [ -e "$dest" ] || [ -L "$dest" ]; then | ||
| echo "[DRY RUN] CONFLICT: $label -> $dest (exists)" | ||
| else | ||
| echo "[DRY RUN] SYMLINK: $label -> $dest" | ||
| fi | ||
| return | ||
| fi | ||
|
|
||
| if ([ -e "$dest" ] || [ -L "$dest" ]) && [ "$FORCE" != true ]; then | ||
| echo "EXISTS: $dest" | ||
| read -r -p " [s]kip, [o]verwrite? " choice | ||
| case $choice in | ||
| o|O) | ||
| ln -sf "$src" "$dest" | ||
| echo " -> Symlinked (overwritten)." | ||
| ;; | ||
| *) | ||
| echo " -> Skipped." | ||
| ;; | ||
| esac | ||
| else | ||
| mkdir -p "$(dirname "$dest")" | ||
| ln -sf "$src" "$dest" | ||
| echo "INSTALLED: $label -> $dest (symlink)" | ||
| fi | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
On GNU coreutils lnand BSD/macOSln, what happens when running ln -sf SOURCE DESTifDESTalready exists as a directory? Does it replaceDESTor create the symlink insideDEST? What is the portable way to avoid silently misinstalling in that case?
💡 Result:
On both GNU coreutils ln and BSD/macOS ln, running ln -sf SOURCE DEST when DEST already exists as a directory creates the symlink named with the basename of SOURCE inside DEST, rather than replacing DEST itself. It does not replace the directory. This occurs because ln detects DEST as an existing directory and treats the command as the 3rd form: ln [options] SOURCE... DIRECTORY, creating the link inside the directory. GNU coreutils man page states: "In the 3rd ... forms, create links to each TARGET in DIRECTORY." And explicitly: "When the destination is an actual directory (not a symlink to one), there is no ambiguity. The link is created in that directory." BSD/macOS/FreeBSD/OpenBSD man pages describe similar behavior: if target is a directory, the link is placed inside it using the basename of source. Examples from sources confirm: mkdir dir; ln -sf file dir results in dir/file -> SOURCE, not replacing dir. The portable way to avoid silently misinstalling (i.e., to force replacing DEST even if it's a directory) is to use ln -sfT SOURCE DEST on GNU systems (where -T/--no-target-directory treats DEST as a normal file always). However, -T is GNU-specific. For full portability across GNU, BSD, and macOS, first check if DEST exists and is a directory, then remove it explicitly: [ -e DEST ] && rm -f DEST # or rm -rf if allowing directory removal ln -sf SOURCE DEST Or more safely: [ -d DEST ] && rmdir DEST 2>/dev/null || rm -f DEST ln -sf SOURCE DEST This ensures DEST is removed if it's a directory or file/symlink, preventing the symlink from being created inside it. POSIX ln -f does not remove directories, as unlink on directories requires privileges and is not standard behavior.
Citations:
- 1: https://www.gnu.org/s/coreutils/manual/html_node/ln-invocation.html
- 2: https://manpages.debian.org/bullseye/coreutils/ln.1.en.html
- 3: https://www.gnu.org/software/coreutils/manual/html_node/ln-invocation.html
- 4: https://man7.org/linux/man-pages/man1/ln.1.html
- 5: https://man.openbsd.org/man1/ln.1
- 6: https://man.openbsd.org/ln
- 7: https://man.freebsd.org/cgi/man.cgi?query=ln&sektion=1
- 8: https://www.unix.com/man-page/osx/1/ln?os=osx§ion=1&query=ln
- 9: https://stackoverflow.com/questions/40806543/ln-sf-does-not-overwrite-a-symlink-to-a-directory
- 10: https://stackoverflow.com/questions/38094453/ln-s-and-overwriting-a-physical-directory
🏁 Script executed:
fd install.shRepository: john-wilmes/claude-agentic-coding-playbook
Length of output: 96
🏁 Script executed:
if [ -f install.sh ]; then
echo "=== Lines 374-405 of install.sh ==="
sed -n '374,405p' install.sh | cat -n
fiRepository: john-wilmes/claude-agentic-coding-playbook
Length of output: 1079
🏁 Script executed:
# Also check total lines in install.sh
wc -l install.shRepository: john-wilmes/claude-agentic-coding-playbook
Length of output: 101
Guard against existing directories before calling ln -sf.
If $dest already exists as a directory, ln -sf will create the symlink inside that directory rather than replacing it. This is consistent behavior on GNU coreutils and BSD/macOS systems. The installer then reports success despite $dest not being the expected hook file path. Add an explicit directory check before attempting symlink creation on both lines 393 and 402:
install_symlink() {
local src="$1"
local dest="$2"
local label="$3"
if [ "$DRY_RUN" = true ]; then
- if [ -e "$dest" ] || [ -L "$dest" ]; then
+ if [ -d "$dest" ] && [ ! -L "$dest" ]; then
+ echo "[DRY RUN] CONFLICT: $label -> $dest (directory)"
+ elif [ -e "$dest" ] || [ -L "$dest" ]; then
echo "[DRY RUN] CONFLICT: $label -> $dest (exists)"
else
echo "[DRY RUN] SYMLINK: $label -> $dest"
fi
return
fi
- if ([ -e "$dest" ] || [ -L "$dest" ]) && [ "$FORCE" != true ]; then
+ if [ -d "$dest" ] && [ ! -L "$dest" ]; then
+ echo "ERROR: $dest is a directory; remove it before installing $label"
+ return 1
+ fi
+
+ if { [ -e "$dest" ] || [ -L "$dest" ]; } && [ "$FORCE" != true ]; then
echo "EXISTS: $dest"
read -r -p " [s]kip, [o]verwrite? " choice
case $choice in📝 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.
| install_symlink() { | |
| local src="$1" | |
| local dest="$2" | |
| local label="$3" | |
| if [ "$DRY_RUN" = true ]; then | |
| if [ -e "$dest" ] || [ -L "$dest" ]; then | |
| echo "[DRY RUN] CONFLICT: $label -> $dest (exists)" | |
| else | |
| echo "[DRY RUN] SYMLINK: $label -> $dest" | |
| fi | |
| return | |
| fi | |
| if ([ -e "$dest" ] || [ -L "$dest" ]) && [ "$FORCE" != true ]; then | |
| echo "EXISTS: $dest" | |
| read -r -p " [s]kip, [o]verwrite? " choice | |
| case $choice in | |
| o|O) | |
| ln -sf "$src" "$dest" | |
| echo " -> Symlinked (overwritten)." | |
| ;; | |
| *) | |
| echo " -> Skipped." | |
| ;; | |
| esac | |
| else | |
| mkdir -p "$(dirname "$dest")" | |
| ln -sf "$src" "$dest" | |
| echo "INSTALLED: $label -> $dest (symlink)" | |
| fi | |
| } | |
| install_symlink() { | |
| local src="$1" | |
| local dest="$2" | |
| local label="$3" | |
| if [ "$DRY_RUN" = true ]; then | |
| if [ -d "$dest" ] && [ ! -L "$dest" ]; then | |
| echo "[DRY RUN] CONFLICT: $label -> $dest (directory)" | |
| elif [ -e "$dest" ] || [ -L "$dest" ]; then | |
| echo "[DRY RUN] CONFLICT: $label -> $dest (exists)" | |
| else | |
| echo "[DRY RUN] SYMLINK: $label -> $dest" | |
| fi | |
| return | |
| fi | |
| if [ -d "$dest" ] && [ ! -L "$dest" ]; then | |
| echo "ERROR: $dest is a directory; remove it before installing $label" | |
| return 1 | |
| fi | |
| if { [ -e "$dest" ] || [ -L "$dest" ]; } && [ "$FORCE" != true ]; then | |
| echo "EXISTS: $dest" | |
| read -r -p " [s]kip, [o]verwrite? " choice | |
| case $choice in | |
| o|O) | |
| ln -sf "$src" "$dest" | |
| echo " -> Symlinked (overwritten)." | |
| ;; | |
| *) | |
| echo " -> Skipped." | |
| ;; | |
| esac | |
| else | |
| mkdir -p "$(dirname "$dest")" | |
| ln -sf "$src" "$dest" | |
| echo "INSTALLED: $label -> $dest (symlink)" | |
| fi | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@install.sh` around lines 374 - 405, The install_symlink function can
accidentally create a symlink inside an existing directory because ln -sf on a
directory targets the directory rather than replacing it; before each ln -sf
(the one in the overwrite branch and the one in the normal install branch) add
an explicit directory check for dest (e.g. [ -d "$dest" ]) and treat it as a
conflict: prompt the user (or respect FORCE) to either remove the directory (rm
-rf "$dest") then create the symlink, or skip; update the logic around the
overwrite branch (where choice o|O triggers ln -sf) and the normal branch (where
ln -sf is used) to handle directory-dest safely so install_symlink always
replaces or skips a directory rather than creating a symlink inside it.
john-wilmes
left a comment
john-wilmes
left a comment
There was a problem hiding this comment.
Review
The core changes (symlink install, SQLite consolidation, auto-promote) are sound and address real gaps. A few issues to address before merge:
Must fix
-
Drop MCP registry commit —
templates/registry/mcp-servers.jsonchanges reference machine-specific paths (PROJECTS/core/datadog-sanitizer/server.py,~/.mcp/datadog-launch.sh, Keychain service names). These are specific to your local setup and don't belong in the shared template. Please revert commiteeecfcfcor make the notes generic. -
Shell injection in knowledge-consolidate.sh — The Node
-einline scripts embed bash variables directly into JS strings:fs.readFileSync('${JSONL_TMP}', ...)If the mktemp path contains a single quote, this breaks. Pass values via
process.argvinstead of string interpolation. Same issue with the${tool}variable on the line below. -
Silent
catch {}in session-end.js — Lines 756 and 777 swallow all errors silently. If the promotion logic fails, there's zero visibility. At minimum log to stderr:} catch (e) { process.stderr.write(`[session-end] auto-promote error: ${e.message}\n`); return 0; }
Should fix
- No tests for auto-promote —
_generateEntryId,_mapStagedToEntry, andautoPromoteStagedCandidatesare 80+ lines of new logic with no test coverage. The first two are pure functions — easy to unit test.
Looks good
install.shsymlink approach with-Lcheck in uninstall — cleanknowledge-consolidate.shmigration from flat files to SQLite export — correct fix sinceentries/no longer existsSKILL.mdauto-close after synthesis — good UX improvement- Duplicate prevention via first-line context check in auto-promote — reasonable dedup strategy
- Shell injection in knowledge-consolidate.sh: pass JSONL_TMP and tool name via process.argv instead of string-interpolating into node -e JS - Auto-promote errors now logged to stderr instead of silently swallowed - install_symlink: remove dest file/symlink before ln -sf to prevent symlink being created inside a directory destination - mcp-servers.json: replace machine-specific paths with generic placeholders to make the template usable for all users - Add unit tests for _mapStagedToEntry and _generateEntryId (exported via module.exports guard at bottom of session-end.js) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Addressed all findings in the latest commit: (1) shell injection fixed by passing vars via process.argv, (2) auto-promote errors now logged to stderr, (3) install_symlink removes dest before ln -sf, (4) mcp-servers.json genericized, (5) unit tests added for _mapStagedToEntry and _generateEntryId |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
templates/hooks/session-end.js (1)
144-145: Minor: Redundant null check.Line 145 (
if (!snippet) return null) makes line 144's!snippetportion redundant. The first check only adds value when!summary && snippetis false but both are empty strings after trim—which line 145 already handles.Consider simplifying:
♻️ Suggested simplification
- if (!summary && !snippet) return null; - if (!snippet) return null; + if (!snippet) return null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/hooks/session-end.js` around lines 144 - 145, Trim/normalize both summary and snippet first, then remove the redundant combined check and keep a single null-return check for snippet (i.e., delete the if (!summary && !snippet) branch and retain only if (!snippet) return null), referencing the summary and snippet variables in the session-end hook so the logic only runs after trimming.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/knowledge-consolidate.sh`:
- Around line 182-199: The loop currently always archives the second parsed id
(variables id1/id2) using node "${KNOWLEDGE_DB_JS}" archive "${id2}", which can
be wrong if the model meant id2 to supersede id1; update the handler to respect
an explicit keep indicator from Claude (e.g., parse an added "KEEP:" or
"ARCHIVE:" token from the ANALYSIS lines) or change the prompt so Claude always
lists the entry-to-archive second, then branch on that token before calling the
archive command (use the existing variables id1/id2 and the DRY_RUN check to
decide which id to pass to node "${KNOWLEDGE_DB_JS}" archive and adjust
ARCHIVED_COUNT/RECOMMENDATION_COUNT logging accordingly).
In `@templates/hooks/session-end.js`:
- Around line 161-209: The autoPromoteStagedCandidates flow can leave the DB in
an inconsistent state because it calls knowledgeDb.insertEntry repeatedly then
does a final DELETE from staged_candidates without a transaction; wrap the
entire promotion process (the loop that maps candidates, uniqueness checks using
db.prepare("SELECT id FROM entries WHERE id = ?"), the inserts via
knowledgeDb.insertEntry, and the final DELETE of promotedIds) in a single
database transaction (BEGIN/COMMIT/ROLLBACK or db.transaction helper) so that
any error aborts and rolls back all inserts and the delete; ensure the
id-generation/duplicate-check (the while loop that calls
db.prepare(...).get(id)) and the update of existingTitles happen inside that
same transaction and that errors trigger a rollback and return 0 as before.
---
Nitpick comments:
In `@templates/hooks/session-end.js`:
- Around line 144-145: Trim/normalize both summary and snippet first, then
remove the redundant combined check and keep a single null-return check for
snippet (i.e., delete the if (!summary && !snippet) branch and retain only if
(!snippet) return null), referencing the summary and snippet variables in the
session-end hook so the logic only runs after trimming.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7a797452-c16f-42ab-9ceb-a21df0ec0309
📒 Files selected for processing (5)
install.shscripts/knowledge-consolidate.shtemplates/hooks/session-end.jstemplates/registry/mcp-servers.jsontests/hooks/session-end-promote.test.js
✅ Files skipped from review due to trivial changes (1)
- tests/hooks/session-end-promote.test.js
🚧 Files skipped from review as they are similar to previous changes (2)
- install.sh
- templates/registry/mcp-servers.json
…eckpoint loop At claude-loop wakeup, SessionStart injects a large context (memory, CLAUDE.md, lessons) that can push token usage past 60% before any real work is done. The first tool call then triggers the BLOCK advisory, /checkpoint runs, claude-loop restarts, and the cycle repeats indefinitely. Fix: introduce MIN_CALLS_BEFORE_BLOCK=5 — BLOCK and FAILSAFE thresholds are suppressed for the first 5 tool calls of a session. WARN thresholds (35%/50%) still fire immediately so the agent is aware of high baseline usage. Tests: added primeSession() helper to bypass the grace period in tests that expect BLOCK/FAILSAFE behavior; all 25 tests pass. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- session-end: wrap auto-promote in BEGIN/COMMIT transaction for crash safety - knowledge-consolidate: clarify id1=keep id2=archive in Claude prompt Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Superseded by #17, which includes all commits from this PR plus review fixes and the context-guard grace period fix. |
- Remove redundant null check in _mapStagedToEntry (CodeRabbit nitpick) - Fix install_symlink directory-dest safety: warn and skip in normal branch, rm -rf in overwrite branch (CodeRabbit actionable) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
2 participants
Summary
staged_candidatestoentriesat session end — previously required manual/learninvocation, so staged captures were accumulating without ever being promotedentries/directory, which no longer exists since the system migrated to SQLiteTest plan
knowledge-consolidate --dry-runruns without error and finds entries from the DBknowledge-consolidate --applyarchives flagged entries viaknowledge-db.js archive🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Tests