Add learnings-suggest hook: federated keyword matching & section-level indexing - #109
Add learnings-suggest hook: federated keyword matching & section-level indexing#109ahoym wants to merge 17 commits into
Conversation
ahoym
left a comment
There was a problem hiding this comment.
Team Review: Add learnings-suggest hook: federated keyword matching & section-level indexing
A well-designed learnings suggestion system with solid fail-safe discipline throughout — no hook ever blocks a prompt, the atomic index write is correct, and the two-tier matching architecture (section-level preferred, file-level supplement) is clean and extensible. The main areas to address are a behavioral gap from duplicated provider loading logic, a SCHEMA_VERSION type mismatch that's latent now but a trap on the next bump, and the global scope of hook registration which adds per-prompt overhead in every project session.
Reviewed by: claude-config-reviewer, architecture-reviewer, python-engineer
Findings
14 finding(s) — see inline comments.
Positive Signals
- Fail-safe discipline is excellent: every wrapper shell script exits 0 on a missing or non-executable binary; the Python hooks catch all exceptions at the outer boundary with a clear
# noqa: BLE001rationale. The system never blocks a session under any failure mode. - Atomic index write (tmp file + rename) in
index-build.rscorrectly prevents a partialsections.jsonfrom poisoning the hook on concurrent reads. - The staleness downgrade in
run()— demoting a section hit to file-level when the source file is newer thansections.json— is a thoughtful degradation path that keeps suggestions usable without requiring a full rebuild after every content edit. setup-claude.shis updated to includehooks/in the symlink list and runsbootstrap.shidempotently, preserving the single-step provisioning invariant.FILTER_PREFIXESis a single named constant; adding a new exclusion is a one-line change in an obvious location.- The
load_jsonlfunction inanalyze.pyhandles missing files, empty lines, and per-lineJSONDecodeErrorgracefully — correct for a log-file reader where individual record corruption should never abort the whole analysis.
- Co-Authored with Claude Code (Claude Sonnet 4.6)
- Persona: claude-config-reviewer, architecture-reviewer, python-engineer
- Role: Team-Reviewer
Addresses team review (#1-#6) on PR #109. - Add `[lib]` target. `Provider`, `home`, `expand_tilde`, and `providers(include_project_local: bool)` move to `src/lib.rs`. `SCHEMA_VERSION` becomes a single `u64` constant. Both binaries import via `learnings_suggest::*`. - `index-build.rs` calls `providers(false)` with a comment documenting the intentional projectLocal omission (CWD-dependent, runtime-only). The behavioral split is now explicit at the call site. - `anchor_commit(provs: &[Provider])` takes the already-loaded slice instead of re-parsing learnings-providers.json. - Decompose `run()` (~200 lines) into `match_sections`, `match_files`, `merge_and_dedup`, `compute_forced`, `format_block`. `run()` is now a ~35-line coordinator; each phase has a typed input/output and is testable in isolation. - `extract_quoted`: comment notes ASCII-only matching (macOS smart-quote autocorrect is not captured). - Linux x86_64 binaries rebuilt via `cargo zigbuild`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…nvention Addresses team review (#7) on PR #109. Lockfile-only dep bumps (e.g., aho-corasick patch releases) would silently skip rebuilds. Adds `[ "$bin" -nt "$DIR/Cargo.lock" ] || return 0` to the existing freshness checks. Comment explains the 0=stale / return-0-short- circuits inversion. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses team review (#8) on PR #109. Removes the third copy of the platform-case dispatch (also lives in learnings-suggest.sh and rebuild-index.sh). rebuild-index.sh already handles platform selection and the silent no-op when no binary exists. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses team review (#9) on PR #109. Hooks in claude/settings.json fire in every Claude Code session regardless of CWD — adding per-prompt overhead on non-dotfiles machines (Java/frontend/ other codebases). The Rust binary already no-ops without providers.json, but the wrapper still spawns. Add a one-line guard to both wrappers so machines that haven't run setup-claude.sh short-circuit before any binary lookup. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses team review (#12, #13, #14) on PR #109. - `cutoff = time.time() - args.days * 86400` (wall-clock) instead of anchoring to the newest log entry. `--days 7` now means "last 7 calendar days" rather than "7 days before last activity". - `import argparse` and `import time` move to the top-level import block. - All four percentage calculations use `round(100 * n / d)` instead of integer floor division: overall suggestion-fire rate, top-performers rate, and both tier hit rates. 2-of-3 now reads as 67%, not 66%. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses team review (#10, #11) on PR #109. - Keyword + domain-shift gate rows now document the agent fallback path when the learnings-suggest binary is absent. - Tier-interpretation bullets collapse into a 4-row table. Path-forms prose tightens. Kept the `offset=start, limit=(end-start+1)` formula — it's the specific conversion from a hint `path:start-end` to Read parameters, not redundant with CLAUDE.md's general "prefer offset+limit" guidance. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Team Re-review: Add learnings-suggest hook: federated keyword matching & section-level indexingAll 14 findings addressed across 6 focused commits ( Reviewers (this cycle): architecture-reviewer, claude-config-reviewer, python-engineer Previous Findings
New Findings0 new findings. Positive Signals
|
Addresses operator follow-ups on PR #109 (5 inline comments) — same theme across both languages: decouple call sites from the literal values they happen to use. Rust (claude/hooks/learnings-suggest/): - `lib.rs`: add `pub fn artifacts_dir()` returning `~/.claude/claude-artifacts/ast`. Replaces three duplicated `join` chains in `main.rs`, `index-build.rs`, and `read-log.rs`. - `index-build.rs`: `KEYWORDS_MARKER` and `KEYWORDS_MARKER_LEGACY` replace `"**Keywords:**"` and `"Keywords:"` literals. `KEYWORDS_HEADER_SEARCH_LIMIT` replaces `.take(5)` with a comment noting it's a search budget, not the header size. - `read-log.rs`: `LEARNINGS_SUBPATHS` constant replaces the inline `["/learnings/", "/guidelines/", "/skill-references/", "/commands/"]` array. Python (claude/hooks/learnings-staleness.py): - `LEARNINGS_DIRS` tuple replaces the inline dir list in the git-diff subprocess call. Comment cross-references the mirror constant in `read-log.rs` so the two stay paired when the set changes. Linux x86_64 binaries rebuilt via `cargo zigbuild`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ahoym
left a comment
There was a problem hiding this comment.
Team Re-review: Add learnings-suggest hook: federated keyword matching & section-level indexing
c455c46 extracts five inline magic literals across Rust and Python to named constants — artifacts_dir() in lib.rs, three keyword-scanning constants in index-build.rs, LEARNINGS_SUBPATHS in read-log.rs, and LEARNINGS_DIRS in learnings-staleness.py. Pure refactor with no logic changes; cross-reference comments between the mirrored Rust/Python constants are accurate and symmetric.
Reviewers (this cycle): orchestrator inline review (subagent-skip: <100 lines, pure constant extraction)
Carried forward: architecture-reviewer, claude-config-reviewer, python-engineer
Previous Findings
- ✅ 14 resolved (verified in prior cycle — no change in status)
New Findings
0 new findings.
Positive Signals
artifacts_dir()correctly deduplicates the three-part join chain (home().join(".claude").join("claude-artifacts").join("ast")) that previously appeared verbatim inmain.rs,index-build.rs, andread-log.rs. The docstring cross-reference toanalyze.py’sARTconstant is accurate.- The
LEARNINGS_SUBPATHS↔LEARNINGS_DIRScross-reference comments are symmetric and explain the form difference (leading/vsclaude/prefix) — exactly what a future maintainer needs to keep them in sync. KEYWORDS_HEADER_SEARCH_LIMITcomment clarifies it’s a search budget, not a header-size assertion — the distinction matters if a file’s header is malformed.
- Co-Authored with Claude Code (claude-sonnet-4-6)
- Persona: architecture-reviewer, claude-config-reviewer, python-engineer
- Role: Team-Reviewer
Adds a Rust-based UserPromptSubmit hook that matches the operator's prompt against the federated learnings keyword index and injects a <learnings-suggestions> block when matches exceed a threshold. Removes the keyword and domain-shift gates from agent-managed work — they now fire deterministically per-prompt at ~5ms cost rather than relying on the agent to remember to invoke the search pipeline. - Rust binary (~700KB, aho-corasick matcher, federated provider read, quoted-term bypass, strong/weak confidence tiers, commands/* filtered) - SessionStart staleness hook that warns when >=5 learnings files have changed since the keyword index was last rebuilt - Bootstrap script with brew/rustup install + --all-targets cross-build via cargo-zigbuild; committed Linux x86_64 binary, macOS binary builds on first setup-claude.sh run on a Mac - Telemetry to ~/.claude/claude-artifacts/ast/suggest.jsonl (every fire, including zero-hit cases, for threshold tuning) - Guideline diff marking keyword/domain-shift as hook-owned and documenting the <learnings-suggestions> hint format - Drops duplicate @./guidelines/context-aware-learnings.md import in claude/CLAUDE.md
… 2a + 2c) Adds two binaries to the same Cargo workspace and wires both into the hook + curate pipelines. Iteration 2c — telemetry - learnings-read-log binary: PostToolUse(Read) hook, appends one JSONL event per learnings/guidelines/skill-references Read to ~/.claude/claude-artifacts/ast/reads.jsonl - analyze.py: offline analyzer cross-references suggest.jsonl with reads.jsonl in-session within a 5-minute window. Reports tier hit rates, top performers, noise candidates (suggested ≥3, never loaded), and coverage gaps (loaded ≥3, never suggested) — the diagnostic data needed to tune MIN_SCORE/STRONG_SCORE/FILTER_PREFIXES empirically Iteration 2a — section-level loading - learnings-index-build binary: pulldown-cmark-based AST walker over all federated providers, emits sections.json (schema v1) with by_keyword inverted index + section table (lines, header, anchor, level, body keywords + supplemented file-level keywords) - Min-section-lines filter (8) keeps dense atomic files (spring-boot.md, code-quality-instincts.md, ...) on the file-level fallback path - suggest hook: section-level primary, file-level fallback. Hint format becomes `path:start-end — terms | section header` when section index hit, plain path otherwise. Staleness check downgrades a section hit to file-level when the underlying file is newer than sections.json - Guideline diff explaining the two path forms and section-level Read offset+limit semantics - Curate skill: rebuild-index.sh invoked after step 8 applies changes - Bootstrap rebuilt to stage all three binaries with consistent suffix naming (learnings-<role>-<platform-suffix>) Telemetry artifacts colocated under ~/.claude/claude-artifacts/ast/. Binary count up to 3 (suggest, read-log, index-build); committed Linux x86_64 binaries total ~2.0 MB. Cold-start hook latency ~18ms with the sections.json load (~315 KB for 720 sections from 135 files).
Addresses team review (#1-#6) on PR #109. - Add `[lib]` target. `Provider`, `home`, `expand_tilde`, and `providers(include_project_local: bool)` move to `src/lib.rs`. `SCHEMA_VERSION` becomes a single `u64` constant. Both binaries import via `learnings_suggest::*`. - `index-build.rs` calls `providers(false)` with a comment documenting the intentional projectLocal omission (CWD-dependent, runtime-only). The behavioral split is now explicit at the call site. - `anchor_commit(provs: &[Provider])` takes the already-loaded slice instead of re-parsing learnings-providers.json. - Decompose `run()` (~200 lines) into `match_sections`, `match_files`, `merge_and_dedup`, `compute_forced`, `format_block`. `run()` is now a ~35-line coordinator; each phase has a typed input/output and is testable in isolation. - `extract_quoted`: comment notes ASCII-only matching (macOS smart-quote autocorrect is not captured). - Linux x86_64 binaries rebuilt via `cargo zigbuild`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…nvention Addresses team review (#7) on PR #109. Lockfile-only dep bumps (e.g., aho-corasick patch releases) would silently skip rebuilds. Adds `[ "$bin" -nt "$DIR/Cargo.lock" ] || return 0` to the existing freshness checks. Comment explains the 0=stale / return-0-short- circuits inversion. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses team review (#8) on PR #109. Removes the third copy of the platform-case dispatch (also lives in learnings-suggest.sh and rebuild-index.sh). rebuild-index.sh already handles platform selection and the silent no-op when no binary exists. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses team review (#9) on PR #109. Hooks in claude/settings.json fire in every Claude Code session regardless of CWD — adding per-prompt overhead on non-dotfiles machines (Java/frontend/ other codebases). The Rust binary already no-ops without providers.json, but the wrapper still spawns. Add a one-line guard to both wrappers so machines that haven't run setup-claude.sh short-circuit before any binary lookup. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses team review (#12, #13, #14) on PR #109. - `cutoff = time.time() - args.days * 86400` (wall-clock) instead of anchoring to the newest log entry. `--days 7` now means "last 7 calendar days" rather than "7 days before last activity". - `import argparse` and `import time` move to the top-level import block. - All four percentage calculations use `round(100 * n / d)` instead of integer floor division: overall suggestion-fire rate, top-performers rate, and both tier hit rates. 2-of-3 now reads as 67%, not 66%. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses team review (#10, #11) on PR #109. - Keyword + domain-shift gate rows now document the agent fallback path when the learnings-suggest binary is absent. - Tier-interpretation bullets collapse into a 4-row table. Path-forms prose tightens. Kept the `offset=start, limit=(end-start+1)` formula — it's the specific conversion from a hint `path:start-end` to Read parameters, not redundant with CLAUDE.md's general "prefer offset+limit" guidance. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses operator follow-ups on PR #109 (5 inline comments) — same theme across both languages: decouple call sites from the literal values they happen to use. Rust (claude/hooks/learnings-suggest/): - `lib.rs`: add `pub fn artifacts_dir()` returning `~/.claude/claude-artifacts/ast`. Replaces three duplicated `join` chains in `main.rs`, `index-build.rs`, and `read-log.rs`. - `index-build.rs`: `KEYWORDS_MARKER` and `KEYWORDS_MARKER_LEGACY` replace `"**Keywords:**"` and `"Keywords:"` literals. `KEYWORDS_HEADER_SEARCH_LIMIT` replaces `.take(5)` with a comment noting it's a search budget, not the header size. - `read-log.rs`: `LEARNINGS_SUBPATHS` constant replaces the inline `["/learnings/", "/guidelines/", "/skill-references/", "/commands/"]` array. Python (claude/hooks/learnings-staleness.py): - `LEARNINGS_DIRS` tuple replaces the inline dir list in the git-diff subprocess call. Comment cross-references the mirror constant in `read-log.rs` so the two stay paired when the set changes. Linux x86_64 binaries rebuilt via `cargo zigbuild`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`brew install rust` left macOS without `rustup`, so `bootstrap.sh --all-targets` silently no-op'd `rustup target add` and then failed at `cargo zigbuild --target`. rustup is the official toolchain manager and the recommended path on all platforms; collapse the OS-conditional branch. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Keeps rustup as the toolchain (so `rustup target add` works in --all-targets), but lets Homebrew track updates on macOS instead of side-installing via the upstream shell script. The keg-only `rustup` formula provides `rustup-init`, which seeds `~/.cargo/bin` exactly like the curl path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…erns Three learnings from recent sessions: - bash-patterns: brew install rustup is keg-only, distinct from brew install rust - git-patterns: rebase --onto add/add-conflicts on stacked PRs after base force-rebase - refactoring-patterns: platform-conditional branches encode value — don't dedupe blindly Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add a 30-line README covering the components, provider/root encoding, and index split. Inline additions explain why root=base.parent() (rel includes leaf dir for cross-index alignment), that display_path is provider-agnostic (not hardcoded to ~/.claude/learnings/), and the shared-parent-dir collision risk in the file-hit reverse lookup. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`writeln!(f, "{}", event)` on an O_APPEND-opened file expands to many small
write() syscalls (one per Display fragment of serde_json::Value). POSIX's
O_APPEND atomicity guarantee is per-syscall, not per-writeln! call — so when
parallel `claude -p` workers fire the hook concurrently, their fragments
interleave at the syscall level, producing line-torn JSONL.
Observed in `~/.claude/claude-artifacts/ast/reads.jsonl` line 1 after two
sweep-review workers (e461f631, ba66366f) raced through PreToolUse hooks
simultaneously:
{"limit"{":limitnull",:"nulloffset,"":offsetnull,...
Fix: serialize the full record (JSON body + `\n`) into one Vec<u8>, then a
single write_all. Records are 150-300 bytes, well under any plausible
atomic-write threshold for O_APPEND on macOS/Linux. Stress test (200
concurrent invocations against a fresh log) produces 200 valid lines, 0
corrupt, 0 lost. Applied to both reads.jsonl (read-log.rs) and suggest.jsonl
(main.rs::log_event) — same race, lower volume on the latter.
analyze.py already swallows JSONDecodeError, so existing corrupted records
are silently skipped and need no cleanup.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three improvements to the learnings-suggest signal quality, motivated by a 0% hit rate observed across an algo-trading director session (17 prompts, 26 strong suggestions, all orchestration-meta, none of the files workers actually loaded). 1. IDF weighting (active). sections.json now stores kw_weights derived from document frequency: rare keywords score higher than common ones. Match-time contribution = word_count × kw_weight, with weights clamped [1, 8]. For the current ~744-section corpus, harness vocabulary (skill/comment/session/review) lands at 3-6 while domain terms (parquet/migration/pyarrow) sit at 7-8. Backward compatible: old indexes without kw_weights fall back to a neutral weight of 1. 2. <learnings-context> slice marker (active, opt-in). Skill prompts can wrap the signal-bearing portion of their template in <learnings-context>...</learnings-context>. When present, only the enclosed text is scored — the procedural recipe body that drowns out subject matter no longer contributes. Falls through to the whole prompt when markers absent. Demonstrated on the PR-210 replay: with the slice marker, code-quality-instincts.md (a top coverage-gap file per analyze.py) surfaces; without, orchestration-meta dominates. 3. Audience downweight (PROVISIONAL — see README §Scoring). A cluster CLAUDE.md may declare **Audience:** <name>; all sections under that directory inherit the tag. At match time, sections with an audience tag get score × 0.3 when the prompt doesn't mention the audience name. Currently dormant (no cluster has opted in) — kept on probation because it introduces a second metadata concept on top of **Keywords:**. If IDF + better keyword curation prove sufficient, this comes out and the system simplifies back to one metadata axis. Activation roadmap (separate work): - Skill prompt templates adopt <learnings-context> around PR title / user message slices. - Cluster CLAUDE.md files opt in to audience tags if needed. Schema additive — no version bump; existing sections.json keeps working until the next /learnings:curate reindex picks up the new fields. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
41a278a to
45b6791
Compare
Completes the platform matrix alongside the tracked x86_64-linux-gnu trio: aarch64-darwin, aarch64-linux-gnu, and x86_64-darwin builds of learnings-index-build, learnings-read-log, and learnings-suggest. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Three compounding causes made every turn surface the same [strong] files from <task-notification> blocks and generic prose: 1. Stop-list: drop English filler (want/here/work/ref/need) and harness-structural tokens (output/claude/task/command/tool/ notification/exit/background/runner) at index-build extraction time so they never become keywords. 2. Specificity guard: [strong] now requires >=1 specific matched term — a multi-word phrase or an above-IDF-floor unigram. A pile-up of corpus-common single words can clear the score threshold but stays [weak]. Disabled for pre-IDF indexes (no kw_weights). 3. Whole-word matching: aho-corasick matches substrings, so short keys fired inside unrelated words (ref->prefer, cat->notification, put->output, ask->task) — the dominant noise source. Require non-word chars on both sides of each match. Rebuilds all platform binaries and refreshes the README scoring section. Verified: notification noise -> no suggestions; genuine domain prompts and quoted-term forcing still fire. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Summary
Implements a comprehensive learnings suggestion system that matches user prompts against a federated corpus of learnings, guidelines, and skill references. The system uses dual-layer indexing (section-level preferred, file-level fallback) to inject contextual
<learnings-suggestions>hints into prompts, along with telemetry and staleness detection.Key Changes
UserPromptSubmit hook (
learnings-suggest): Aho-Corasick-based keyword matching against both section-level (fromsections.json) and file-level (from.keyword-index.json) indexes. Scores hits by keyword weight, filters by staleness, and respects quoted-term bypass. Injects up to 3 suggestions as a hint block. Never blocks a prompt.Section-level indexing (
learnings-index-build): Walks the federated learnings corpus, parses Markdown with pulldown-cmark, extracts H2/H3 sections with line ranges, and builds an inverted keyword index. Supports explicit file-level keywords (via**Keywords:**convention) and deduplicates section anchors. Outputs~/.claude/claude-artifacts/ast/sections.json.Read telemetry (
learnings-read-log): PostToolUse hook that logs all Read operations on learnings/guidelines/skill-references files toreads.jsonlfor offline hit-rate analysis.Staleness detection (
learnings-staleness.py): SessionStart hook that warns when the keyword index is stale (≥5 files changed since last rebuild), with git-based anchor tracking.Analysis tooling (
analyze.py): Cross-references suggest.jsonl and reads.jsonl to compute tier hit rates, identify top performers, and surface noise/coverage gaps.Build system: Rust Cargo project with platform-specific binaries (aarch64/x86_64, Darwin/Linux), idempotent bootstrap script, and optional cross-compilation via cargo-zigbuild.
Hook registration: Added to
settings.jsonunderhooks.UserPromptSubmit,hooks.PostToolUse, andhooks.SessionStart.Setup integration: Updated
setup-claude.shto symlink thehooks/directory and run the bootstrap build.Notable Implementation Details
Dual indexing strategy: Section-level hits are preferred (more precise), but file-level hits supplement them and serve dense files where section extraction may be incomplete. Covered paths are deduplicated.
Staleness downgrade: If a source file is newer than
sections.json, section references are downgraded to file-level (anchor/lines dropped) to avoid stale line numbers.Quoted-term bypass: Keywords in double quotes force inclusion even below the MIN_SCORE threshold, enabling explicit learnings requests.
Telemetry design: Prompt hashes (not full text) are logged for privacy; session IDs enable cross-referencing with Read logs; tier classification (strong ≥3, weak <3) supports hit-rate stratification.
Silent failure: All hooks are designed to never block a prompt/session/tool call — any I/O or parsing error results in a no-op.
Provider resolution: Supports both
learnings-providers.jsonentries (localPath, projectLocal) and runtime discovery via git anchors for commit-based staleness tracking.https://claude.ai/code/session_01LnbK3YBtPx3GQUbCrkc43S