fix(plugins): address v0.4 plugin code-review findings (PR #865)

Resolves the 8 self-review findings on PR #865:

session-start.sh:
- Wire recallTimeframe into a new "Recent sessions" query (--after_date) — it was
  parsed but unused after the Phase 4 rewrite. The brief now surfaces recent
  session checkpoints (the resume cursor), which is the one query the recency
  window legitimately applies to. (#1)
- Guard secondaryProjects/teamProjects JSON types — a string value was iterated
  character-by-character into bogus per-char queries. (#3)
- A configured-but-unreachable/misnamed primaryProject now emits a one-line
  "couldn't read" signal instead of a silent blank brief. (#7)

pre-compact.sh:
- Filter transcript turns by the `isMeta` and `toolUseResult` flags instead of a
  `text.startswith("<")` heuristic. This stops dropping legitimate user messages
  that start with "<" (#4) and stops capturing tool-result/meta frames as human
  turns (#8) — verified against a real transcript (25 human turns cleanly
  separated from 8 meta + 288 tool-result frames).
- Title now uses second precision so rapid same-minute compactions don't collide
  and silently drop/overwrite a checkpoint; removed the unused stamp/slug. (#2)
- Empty-checkpoint guard now requires at least one real user turn, not just any
  turn, so an assistant-only transcript can't produce a dangling-title note. (#5)

validate_skills.py:
- parse_frontmatter now skips indented lines, capturing only top-level keys, so a
  schema note's nested schema:/settings: children can't overwrite a top-level
  type/entity via last-write-wins. Documented its single-line-only limitation. (#6)

All verified end-to-end; `just package-check-claude-code` and
`package-check-skills` pass; ruff clean.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: phernandez <paul@basicmachines.co>
This commit is contained in:
phernandez
2026-05-28 20:55:34 -05:00
committed by Paul Hernandez
parent 2886ab64e1
commit df3eb3208c
3 changed files with 64 additions and 19 deletions
+18 -9
View File
@@ -25,7 +25,6 @@ BM="$(command -v basic-memory || command -v bm || true)"
BM_HOOK_INPUT="$input" BM_BIN="$BM" python3 <<'PY' 2>/dev/null || exit 0
import json
import os
import re
import subprocess
import sys
from datetime import datetime
@@ -97,13 +96,21 @@ def turns(path):
obj = json.loads(line)
except Exception:
continue
# Skip injected/meta frames and tool results — only real human
# input and assistant prose count. Claude Code marks tool results
# with a `toolUseResult` field and injected/meta turns (command
# wrappers, system reminders, auto-continuations) with `isMeta`.
# Filtering on those flags — not a "<" content prefix — avoids both
# dropping legitimate messages that start with "<" and capturing
# tool-result noise.
if obj.get("isMeta") or obj.get("toolUseResult") is not None:
continue
msg = obj.get("message") if isinstance(obj.get("message"), dict) else obj
role = msg.get("role") or obj.get("type")
if role not in ("user", "assistant"):
continue
text = text_of(msg.get("content")).strip()
# Skip tool-result noise and empty frames.
if not text or text.startswith("<"):
if not text:
continue
collected.append((role, text))
except Exception:
@@ -113,10 +120,11 @@ def turns(path):
conversation = turns(transcript_path)
# Trigger: nothing usable in the transcript.
# Why: an empty checkpoint is worse than none — it clutters the graph.
# Trigger: nothing usable in the transcript, or no real human turn in it.
# Why: an empty or human-less checkpoint is worse than none — it would write a
# note with a dangling title and no opening request. Require a user turn.
# Outcome: silent no-op.
if not conversation:
if not conversation or not any(role == "user" for role, _ in conversation):
sys.exit(0)
user_msgs = [t for r, t in conversation if r == "user"]
@@ -134,10 +142,11 @@ def clip(s, n):
# find it with metadata filters. BM merges a leading frontmatter block from the
# content into the note's frontmatter (verified empirically).
now = datetime.now()
stamp = now.strftime("%Y-%m-%d-%H%M%S")
iso = now.strftime("%Y-%m-%dT%H:%M")
slug = re.sub(r"[^a-z0-9]+", "-", clip(opening, 50).lower()).strip("-") or "session"
title = f"Session {now.strftime('%Y-%m-%d %H:%M')} — {clip(opening, 40)}"
# Second precision keeps the title — and therefore the note's permalink — unique
# across rapid compactions within the same minute (otherwise the second write
# would collide with the first and be dropped or overwrite it).
title = f"Session {now.strftime('%Y-%m-%d %H:%M:%S')} — {clip(opening, 40)}"
frontmatter = [
"---",
+35 -10
View File
@@ -101,9 +101,13 @@ recall_prompt = cfg.get("recallPrompt") or default_prompt
# also read for recall). Dedup, preserve order, cap. These are read only — the
# capture hooks never write to them.
shared_refs = []
secondary = cfg.get("secondaryProjects") or []
team = cfg.get("teamProjects") or {}
for ref in list(secondary) + (list(team.keys()) if isinstance(team, dict) else []):
# Guard the JSON types: a misconfigured string would otherwise be iterated
# character-by-character, firing a bogus per-character query for each one.
secondary = cfg.get("secondaryProjects")
secondary = secondary if isinstance(secondary, list) else []
team = cfg.get("teamProjects")
team = team if isinstance(team, dict) else {}
for ref in list(secondary) + list(team.keys()):
if isinstance(ref, str) and ref.strip() and ref.strip() != primary_project:
if ref.strip() not in shared_refs:
shared_refs.append(ref.strip())
@@ -129,6 +133,10 @@ def search(filters, project_ref=None, timeout=10):
ACTIVE_TASKS = ["--type", "task", "--status", "active"]
OPEN_DECISIONS = ["--type", "decision", "--status", "open"]
# Recent session checkpoints carry the resume cursor. This is the one query the
# `recallTimeframe` window applies to — tasks and decisions are status-scoped (an
# old open decision is still open), but "recent sessions" is inherently time-scoped.
RECENT_SESSIONS = ["--type", "session", "--after_date", recall_timeframe]
# --- Run everything concurrently ---
# Cloud reads cost a network round-trip each; parallelism keeps total wall-clock at
@@ -136,9 +144,11 @@ OPEN_DECISIONS = ["--type", "decision", "--status", "open"]
with ThreadPoolExecutor(max_workers=8) as pool:
fut_tasks = pool.submit(search, ACTIVE_TASKS, primary_project or None)
fut_decisions = pool.submit(search, OPEN_DECISIONS, primary_project or None)
fut_sessions = pool.submit(search, RECENT_SESSIONS, primary_project or None)
fut_shared = {ref: pool.submit(search, OPEN_DECISIONS, ref) for ref in shared_refs}
primary_tasks = fut_tasks.result()
primary_decisions = fut_decisions.result()
primary_sessions = fut_sessions.result()
shared_results = {ref: fut.result() for ref, fut in fut_shared.items()}
# The first-run nudge — shown until setup writes a basicMemory config block.
@@ -147,13 +157,21 @@ setup_nudge = (
"`/basic-memory:setup` (~2 min) to configure session briefings and checkpoints._"
)
# Trigger: the primary queries couldn't run at all (no default project, transient
# error). Why: a broken query must never error the session — but a genuine first-run
# user (no config, nothing to brief) should still be pointed at setup.
# Outcome: first-run → nudge only; already-configured → silent no-op.
if primary_tasks is None and primary_decisions is None:
# Trigger: every primary query failed (no default project, misnamed project,
# unreachable cloud, transient error). Why: a broken query must never error the
# session, but it must not silently look like "nothing tracked" either.
# Outcome: first-run → setup nudge; configured-but-broken → a one-line signal so
# the user can tell a typo'd/unreachable project from an empty one.
if primary_tasks is None and primary_decisions is None and primary_sessions is None:
if not configured:
print("# Basic Memory\n\n" + setup_nudge)
else:
proj = primary_project or "the default project"
print(
"# Basic Memory\n\n"
f"_Couldn't read from `{proj}` — it may be misnamed or unreachable. "
"Run `/basic-memory:status` to check._"
)
sys.exit(0)
@@ -181,12 +199,19 @@ lines.append(header)
task_rows = rows(primary_tasks)
decision_rows = rows(primary_decisions)
session_rows = rows(primary_sessions)
if task_rows:
lines += ["", f"## Active tasks ({len(task_rows)})", *[label(r) for r in task_rows]]
if decision_rows:
lines += ["", f"## Open decisions ({len(decision_rows)})", *[label(r) for r in decision_rows]]
if not task_rows and not decision_rows:
lines += ["", "_No active tasks or open decisions tracked in this project._"]
if session_rows:
lines += [
"",
f"## Recent sessions ({len(session_rows)}) — where you left off",
*[label(r) for r in session_rows],
]
if not (task_rows or decision_rows or session_rows):
lines += ["", "_No active tasks, open decisions, or recent sessions in this project._"]
# --- Shared/team context (read-only) ---
shared_sections = [(ref, rows(shared_results.get(ref))) for ref in shared_refs]
+11
View File
@@ -8,6 +8,15 @@ from pathlib import Path
def parse_frontmatter(path: Path) -> dict[str, str]:
"""Extract top-level frontmatter keys from a Markdown file.
A deliberately minimal parser (no PyYAML — this runs under bare `python3` in
CI). It only captures **top-level** `key: value` lines. Indented lines are
skipped, so nested blocks (a schema note's `schema:`/`settings:` children) can't
overwrite a top-level key like `type` or `entity` via last-write-wins. It does
not interpret block scalars or multi-line values; callers rely on single-line
top-level fields (name, description, type, entity).
"""
lines = path.read_text().splitlines()
if not lines or lines[0] != "---":
raise SystemExit(f"{path}: missing YAML frontmatter")
@@ -16,6 +25,8 @@ def parse_frontmatter(path: Path) -> dict[str, str]:
for line in lines[1:]:
if line == "---":
break
if line[:1] in (" ", "\t"): # nested key — not a top-level field
continue
if ":" not in line:
continue
key, value = line.split(":", 1)