feat(core): add bm hook producer front door for harness capture#1070
feat(core): add bm hook producer front door for harness capture#1070phernandez wants to merge 41 commits into
Conversation
…t door Port the SPEC-55 producer envelope from the #1064 salvage branch into core (basic_memory/hooks/): a frozen Envelope dataclass with the 2026-07-15 revision fields (UUIDv7 id, actor, caused_by, promotion_status), metadata-only idempotency keying, and the provenance observation/frontmatter projections. The factory redacts every payload, so nothing built through it can carry unredacted values into the inbox. The Stage-1 redaction floor combines detect-secrets scanning (known token formats plus an entropy threshold, with the eager-scan entropy limit re-applied) with the salvaged recursive deny-key / deny-path / env-pair / truncation rules, separator-normalized for Windows and hardened to be pure and idempotent. detect-secrets lands as a core dependency: its tree is light (pyyaml already ships; requests is the only addition) and the floor must be unconditionally present on the capture hot path — an optional extra would make redaction availability configuration-dependent. Includes a ~15-line RFC 9562 UUIDv7 generator; stdlib uuid.uuid7() exists only on Python 3.14+, so swap when the floor rises. Part of issue #997 (SPEC-55 revision 2026-07-15). Co-authored-by: sourrrish <officialsourish0@gmail.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
The inbox is the SPEC-55 append-only WAL: one <uuid7>.json file per envelope under <bm-home>/inbox/, written atomically (tmp + rename) so a crash can never leave a half-written envelope visible. Filename order is capture order, so consumers need no mtime/stat dependence. BM home resolves through core's resolve_data_dir() — plugin directories are ephemeral and uninstalling a plugin must never delete captured trace. Processed envelopes move to processed/ for audit and are pruned after a retention window, with age derived from the uuid7 timestamp in the filename; files that don't parse as UUIDs are never deleted. Adapters normalize each harness's hook stdin dialect (Claude Code per the platform hooks reference and the shipped hook scripts; Codex per its hook scripts) into one NormalizedHookEvent so everything downstream is harness-agnostic. Realistic payloads are recorded as test fixtures. Part of issue #997 (SPEC-55 revision 2026-07-15). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
bm hook flush's engine: sweep the whole inbox in filename order, group by (source, source_session_id), and project SessionNote skeletons plus ToolLedger entries through the same internal write path the CLI's write-note uses (the MCP write_note tool via the async client — no subprocess). Idempotent by construction: envelopes are hints, deduped on idempotency_key within a sweep and against the processed audit trail, and artifacts are re-derived with deterministic titles and overwrite=True — WAL replays and duplicate hooks can never double-write. Artifacts carry created_by and caused_by_event frontmatter plus the required [source] observation. Envelopes without a resolvable project mapping stay pending (fail fast, never write to the wrong project), as do groups whose write fails — both self-heal on the next sweep. Corrupt envelope files are counted and left in place, never deleted or guessed at. Part of issue #997 (SPEC-55 revision 2026-07-15). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
The harness producer front door: plugins reduce to shims that exec 'bm hook <event> --harness claude|codex' with hook JSON on stdin. session-start and pre-compact port the existing plugin hook behavior into core — the session context brief (structured task/decision/session queries, shared-project read set, placement guidance, nudges) printed to stdout under the 10k char cap with graph-derived content fenced and labeled as reference data, not instructions; and the extractive pre-compaction checkpoint note written through the same internal path as bm tool write-note. Settings resolution is ported from the hook scripts: the basicMemory block of the nearest-ancestor .claude settings files over the user-level settings.json for Claude, and .codex/basic-memory.json for Codex; --project-dir overrides the payload cwd for project mapping. When captureEvents is the JSON boolean true (fail-closed: strings never enable), both verbs also capture a floor-redacted envelope into the inbox. Harness verbs are fail-open: any error logs to stderr and exits 0, never disrupting a session. flush runs the projector; status is the debuggability surface (inbox depth, last flush, settings summary, bm/uv versions). Part of issue #997 (SPEC-55 revision 2026-07-15). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
The plugin hook scripts are now zero-logic shims: resolve the Basic Memory
CLI (BM_BIN override -> basic-memory/bm on PATH -> uvx at a released floor)
and exec 'bm hook <event> --harness claude|codex' with the hook JSON passed
through on stdin. Nothing resolvable exits 0 silently (fail-open). The
Claude shims pass ${CLAUDE_PROJECT_DIR} as --project-dir so project mapping
never trusts cwd; Codex mapping keeps using the payload cwd.
The Codex standalone Python hook implementations are deleted — their logic
lives in basic_memory/hooks/ and the hook CLI since the core phase — and the
Codex validator drops the .py script requirements accordingly. hooks.json
manifests are unchanged (${CLAUDE_PLUGIN_ROOT}/${PLUGIN_ROOT} commands,
matchers, and timeouts preserved); both plugin layouts already conform
(.claude-plugin/ and .codex-plugin/ hold manifests only, components at root).
The shell-hook tests are rewritten as a shim e2e suite covering both
plugins: fake recording CLIs assert resolver order, stdin passthrough, the
uvx version floor, BM_BIN override semantics (spaced paths and multi-token
launchers), project-dir plumbing, and silent fail-open — keeping the Git
Bash Windows resolution pattern.
Part of #997 (SPEC-55).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: phernandez <paul@basicmachines.co>
'bm hook install' wires the lifecycle hooks into the user-level harness config for standalone (non-marketplace) users: the Claude Code settings hooks block (~/.claude/settings.json) or a Codex hooks.json (~/.codex/hooks.json) using the plugin's hooks.json schema and matchers. Entries are ownership-tagged through their command shape (the codex-honcho ownership-regex approach): 'bm hook remove' deletes exactly the entries matching OWNED_HOOK_COMMAND_RE — surgical against pre-existing user hooks, including mixed groups — and both verbs are idempotent. Malformed configs fail fast instead of being clobbered. install also checks for uv and prints a per-platform install hint when missing (warning only: a PATH-installed basic-memory works without uv). Also carries the Codex 'focus' config key into the unified session brief header — the one codex-script feature the core-phase unification dropped. Tests cover merge/remove against fixture configs with pre-existing user hooks, idempotency both directions, fail-fast on malformed configs, the per-platform uv hint, and the focus header. Part of #997 (SPEC-55). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
scripts/update_versions.py now rewrites the 'basic-memory>=FLOOR' pin in all four plugin hook shims alongside the manifests it already touches (packages scope), so every release moves the uvx fallback to a version that actually ships the 'bm hook' verbs. The floor keeps the Python version form (e.g. 0.21.3b1) — it is a pip requirement spec, not the npm semver mapping. Regression tests: packages scope bumps all four shims, core scope leaves them untouched, and prereleases land in pip form. Part of #997 (SPEC-55). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
… opt-in Both plugin READMEs now list uv as the documented per-platform prerequisite, disclose that the hook shims fall back to 'uvx basic-memory>=FLOOR' (network fetch from PyPI on first run, floor bumped by release tooling), and document the captureEvents gate as opt-in and off by default (JSON boolean true only). settings.example.json and the Codex config example gain 'captureEvents': false. CHANGELOG gains the Unreleased entry for the bm hook front door, and the plugin CHANGELOG notes the shim reshape. Part of #997 (SPEC-55). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
Five findings from the multi-lens review of the branch survived adversarial verification and are fixed here: - projector: rebuild session artifacts from the complete session (processed + fresh envelopes) so incremental flushes no longer erase events projected on an earlier sweep (was overwriting from still-pending envelopes only). - projector: use the full session id in artifact titles; an 8-char prefix let distinct sessions collide on the same permalink and clobber each other. - inbox: mark_processed tolerates an envelope already retired by a concurrent sweep instead of crashing the whole flush with FileNotFoundError. - inbox/_uuid7: uuid7_unix_ms fails fast on non-v7 UUIDs so retention never derives a garbage capture time from a v1/v4 name and deletes it. - hook: pre-compact checkpoint transcript excerpts now pass the Stage-1 redaction floor (new redaction.redact_text) before entering the note title, summary, and observations — closing the #997 'redact before writing' gap. Regression tests added for each finding. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 63bc6c837d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| elif command -v basic-memory >/dev/null 2>&1; then | ||
| BM="basic-memory" | ||
| BM=(basic-memory) | ||
| elif command -v bm >/dev/null 2>&1; then | ||
| BM="bm" | ||
| BM=(bm) | ||
| elif command -v uvx >/dev/null 2>&1; then |
There was a problem hiding this comment.
When a user has an older basic-memory/bm on PATH from before this commit, this branch wins over the uvx "basic-memory>=0.22.1" fallback and the later exec ... hook session-start runs a CLI that does not have the new hook group. That breaks the documented fail-open behavior for all four shims in exactly the stale-install case the floor is meant to protect; either verify the PATH binary supports hook or fall back to the version floor when it does not.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7fd1d03. All four shims now probe hook support before a PATH-resolved basic-memory/bm can shadow the uvx floor:
supports_hook() { "$@" hook --help >/dev/null 2>&1 </dev/null; }
...
elif command -v basic-memory >/dev/null 2>&1 && supports_hook basic-memory; thenA stale pre-hook install on PATH now falls back to uvx "basic-memory>=<floor>" instead of exec-ing a CLI that errors on hook, and stays silent (fail-open) when nothing else is resolvable. stdin is detached (</dev/null) so the probe never consumes the hook JSON. Regression tests: test_shim_skips_stale_path_install_without_hook_support and test_shim_exits_silently_when_only_stale_install_present.
| entropy_plugins: dict[str, HighEntropyStringsPlugin], | ||
| ) -> str: | ||
| if SECRET_VALUE_RE.match(value): | ||
| return REDACTED |
There was a problem hiding this comment.
For checkpoint transcript text such as please read /home/me/.ssh/id_rsa, the string does not start with the denied prefix, so redact_text() leaves the path intact before _checkpoint_note() writes it into the note title/body. Since this path is natural-language free text, deny-path redaction needs to replace matching substrings, not only redact values whose entire string begins with the sensitive directory.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7fd1d03. Deny-path redaction is now substring-based: each normalized prefix compiles to a matcher that spans either separator and consumes the path tail (re.escape(prefix).replace("/", r"[/\\]") + r"\S*"), applied via re.sub in _redact_str.
please read ~/.ssh/id_rsa → please read [REDACTED_PATH] (surrounding prose preserved); a whole-value path still collapses to the marker since the regex spans the entire string, so the existing payload behavior is unchanged. Regression tests: test_redact_text_redacts_denied_path_embedded_in_prose, test_redact_text_redacts_multiple_embedded_paths, and test_deny_paths_redact_embedded_substring_in_value.
…ows shim Static Checks + Windows unit failures and the two Codex P2 findings: - redaction: deny-path redaction now replaces the matching path token wherever it appears, not only when the whole value starts with the prefix. Free-text checkpoint excerpts such as "please read ~/.ssh/id_rsa" had the path left intact by redact_text(); the prefixes now compile to substring matchers that span either separator and consume the path tail. Whole-value paths still collapse to the marker, so existing behavior is preserved. - shims: PATH-resolved basic-memory/bm are probed for `hook` support (`hook --help`, stdin detached) before they win over the uvx floor. A stale pre-hook install on PATH no longer shadows the floor and exec a CLI that errors — it falls back, keeping the fail-open contract. Applied to all four Claude/Codex shims. - shims: the BM_BIN single-word test uses `-e` (exists) instead of `-x` (executable). Git Bash reports extensionless files as non-executable, so a spaced BM_BIN path was word-split on Windows, breaking test_bm_bin_executable_path_with_spaces_stays_one_word. - test: narrow mock_write.await_args before .kwargs so `ty` stops flagging attribute access on `_Call | None` (matches the sibling assertions already in the file). Regression tests added for embedded-path redaction (free text and payload values) and the stale-install fallback (falls back to uvx; stays silent when nothing else is resolvable). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
|
@codex review Addressed both P2 findings plus the Static Checks (ty) and Windows unit failures in 7fd1d03:
All added regression tests, plus |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7fd1d03098
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| _normalize_path(os.path.expanduser(prefix)) | ||
| for prefix in ("~/.ssh/", "~/.aws/", "~/.gnupg/") | ||
| ) | ||
|
|
There was a problem hiding this comment.
When a transcript or payload contains the shell form of a sensitive path, e.g. ~/.ssh/id_rsa, this default list is expanded to /home/.../.ssh/ before regex compilation, so there is no pattern left that matches the literal ~/ string users commonly type into prompts. That path then survives redact_text() and can be written into checkpoint notes despite the deny-path floor; keep both the expanded path and the original ~/... prefix (or normalize candidate strings) before compiling the patterns.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in aacb9ab. The default deny list now carries both the expanded home path and the literal ~/ prefix (~/.ssh/, ~/.aws/, ~/.gnupg/), deduped with dict.fromkeys. So please read ~/.ssh/id_rsa → please read [REDACTED_PATH] even when the tilde is unexpanded. Regression tests: test_redact_text_redacts_unexpanded_tilde_home_path and test_redact_payload_redacts_unexpanded_tilde_home_path.
| if [[ -n "${CLAUDE_PROJECT_DIR:-}" ]]; then | ||
| exec "${BM[@]}" hook session-start --harness claude --project-dir "${CLAUDE_PROJECT_DIR}" | ||
| fi | ||
| exec "${BM[@]}" hook session-start --harness claude |
There was a problem hiding this comment.
In environments where the resolved command exists but fails before bm hook can run — for example a cold uvx fallback that cannot resolve/build the package, or a bad BM_BIN override — exec replaces the shim and propagates that non-zero exit status to the harness. I checked uv help tool run: uvx installs packages into an ephemeral environment, so resolver/build failures happen in the shim path; this violates the hook contract that failures remain advisory. The same exec pattern is repeated in the other three shims, so invoke the command and exit 0 on failure instead of tail-execing it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in aacb9ab across all four shims. They now run the command instead of exec-ing it, then unconditionally exit 0:
"${BM[@]}" hook session-start --harness claude
exit 0A resolved launcher that errors at runtime — cold uvx offline, unbuildable floor, bad BM_BIN — no longer propagates its non-zero status to the harness, honoring the "advisory, must NEVER disrupt" contract. stdout still reaches the session context and stdin still passes through (no redirection). Regression test: test_shim_exits_zero_when_resolved_command_fails_at_runtime (parametrized over both harnesses and both events).
Two follow-up P2 findings from the re-review of 7fd1d03: - redaction: the deny-path floor only carried the *expanded* home paths (/home/.../.ssh/), so a literal "~/.ssh/id_rsa" — the form prose and transcript excerpts actually type — survived redact_text(). The default list now denies both the expanded and the literal "~/" prefixes, deduped. - shims: the trailing `exec` tail-exec'd a resolved launcher's non-zero exit to the harness. A cold uvx that cannot reach PyPI, an unbuildable floor, or a bad BM_BIN would then disrupt the session — violating the "advisory, must NEVER disrupt" contract in the shim header. All four shims now run the command instead of exec-ing it and always exit 0; stdout still reaches the session context and stdin still passes through. Regression tests: unexpanded-tilde redaction (free text + payload) and fail-open on a launcher that errors at runtime (all four shims). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
|
@codex review Round-2 findings addressed in aacb9ab:
Regression tests added for both. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aacb9ab643
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| source_session_id=session_id, | ||
| source_turn_id=turn_id, | ||
| ts=resolved_ts, | ||
| cwd=cwd, |
There was a problem hiding this comment.
When captureEvents is true and a user adds redactPaths to hide a workspace/client path, only payload goes through redact_payload; the top-level cwd stored here remains the raw hook cwd and is retained in pending/processed inbox files. That means a session in a denied directory still records the denied path even though the capture is advertised as redacted. Please apply the same path scrub to metadata fields that can contain user paths before constructing the Envelope.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 709a622. create_envelope now runs cwd through redact_text(..., extra_redact_paths=extra_redact_paths) before constructing the Envelope, so a session under a configured redactPaths (or a default deny dir) no longer stores the raw path in the inbox WAL. project_hint is a project name (not a path) and the projector resolves against it, so it's intentionally left intact. Regression tests: test_create_envelope_redacts_cwd_under_denied_path and test_create_envelope_keeps_ordinary_cwd.
| f"started: {iso}", | ||
| f"ended: {iso}", | ||
| f"project: {primary}", | ||
| f"cwd: {event.cwd}", |
There was a problem hiding this comment.
When a pre-compact hook runs from a directory covered by redactPaths, the transcript excerpts are scrubbed above, but this frontmatter field still writes event.cwd verbatim into the checkpoint note (and the body repeats it below). A user trying to hide a sensitive client/workspace path will still have that path persisted in Basic Memory. Please run the cwd through the same redaction path before adding it to the note, or omit it when it redacts completely.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 709a622. The checkpoint now redacts event.cwd once at extraction (alongside the transcript excerpts) and both the frontmatter cwd: field and the "Working in ..." body line draw from the scrubbed string. The functional uses of cwd (git status, project mapping) run earlier on the real path, so they're unaffected. Regression test: test_pre_compact_redacts_cwd_under_denied_path.
Two more P2 findings from the re-review of aacb9ab — the resolved hook cwd bypassed the redaction floor: - envelope: create_envelope only scrubbed `payload`; the top-level `cwd` was stored raw in the inbox WAL. A session under a configured redactPaths (or a default deny dir) now has its cwd run through redact_text before the Envelope is built. project_hint is a project name, not a path, and the projector resolves against it, so it is intentionally left intact. - checkpoint: the pre-compact note wrote `event.cwd` verbatim into the frontmatter `cwd:` field and the "Working in ..." body line. It is now redacted once at extraction (alongside the transcript excerpts) so both draw from the scrubbed string. The functional uses of cwd (git status, project mapping) run earlier on the real path and are unaffected. Regression tests: envelope cwd redaction (denied + ordinary) and checkpoint cwd redaction under a configured redactPaths. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
|
@codex review Round-3 findings addressed in 709a622 — the resolved hook
Regression tests added; |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 709a62237e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for path in replays: | ||
| inbox.mark_processed(path) |
There was a problem hiding this comment.
When a sweep contains two envelopes with the same idempotency key and the artifact write fails, this loop moves the duplicate to processed/ before the fresh envelope has been written. On the next sweep, seen_keys is built from processed/, so the still-pending fresh envelope is treated as a replay and retired without ever projecting the session; this breaks the documented self-heal path for transient write/API failures whenever a hook replay lands in the same minute.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e908d94. Replays are now split by kind: a replay of an already-projected envelope (its original is durable in processed/) is retired immediately, but an in-group replay is retired only after its fresh sibling's write succeeds. On a failed write both the fresh envelope and its in-group replays stay pending, so the next sweep re-derives the session instead of reading the replay's key from processed/ and wrongly retiring the still-unprojected fresh envelope. Regression test: test_flush_reprojects_after_write_failure_with_in_group_replay.
| event=envelope_event, | ||
| session_id=event.session_id or "unknown", | ||
| cwd=event.cwd or str(mapping_dir), | ||
| project_hint=str(cfg.get("primaryProject") or "").strip(), |
There was a problem hiding this comment.
If captureEvents is enabled before primaryProject is set, this persists project_hint as an empty string. flush() only consults the stored project_hint when selecting a destination and never reloads settings from cwd, so setting primaryProject later cannot resolve these WAL entries; they remain pending indefinitely despite the capture path treating unmapped sessions as recoverable trace.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e908d94. Rather than drop capture-without-mapping (a deliberate, tested behavior — unmapped sessions are trace worth keeping, and a later same-session capture carrying a hint still resolves the whole group via the projector's merge), retention now prunes pending envelopes on the same window it already applies to processed/. So a fully-unmapped session no longer sits pending indefinitely — it ages out after the retention window. Invalid entries are preserved so retention never eats the corruption/version trace bm hook status surfaces. Regression tests: test_flush_prunes_expired_unresolved_pending_envelopes and test_flush_keeps_recent_unmapped_pending_envelopes.
(Happy to switch to gating capture on primaryProject instead if you'd prefer not to capture unmapped sessions at all — that contradicts the current test_pre_compact_captures_envelope_even_without_mapping intent, so I kept the capture and bounded its lifetime.)
…(PR #1070 round-4) Two P2 findings from the re-review of 709a622: - projector: an in-group idempotency replay was retired to processed/ before the fresh envelope it duplicated was written. When that write failed, the next sweep read the replay's key from processed/ and retired the still-pending fresh envelope as a replay too — the session never projected, breaking the documented self-heal. Replays are now split: a replay of an already-projected envelope is retired immediately (its original is durable), but an in-group replay is retired only after its fresh sibling's write succeeds, so both self-heal together on a failed sweep. - inbox: a session that never resolves a project mapping (primaryProject unset for its whole lifetime) produced envelopes the projector could never route and held pending forever — flush routes solely on the stored project_hint and never reloads settings. Retention now prunes pending envelopes on the same window it already applies to processed/, so unresolvable trace can't accumulate without limit. Capture-without-mapping is unchanged (a later same-session hinted capture still resolves the whole group via the merge); invalid entries are preserved so retention never eats the corruption trace `bm hook status` surfaces. Regression tests: reproject-after-write-failure with an in-group replay; prune of expired unresolved pending envelopes; recent unmapped pending kept. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
|
@codex review Round-4 findings addressed in e908d94:
Regression tests added; |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e908d94366
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # sort chronologically). Overwriting from fresh alone would erase events | ||
| # projected on an earlier sweep. | ||
| prior = processed_by_session.get((source, session_id), []) | ||
| envelopes = sorted(prior + fresh_envelopes, key=lambda envelope: envelope.id) |
There was a problem hiding this comment.
When a hook replay has already been retired into processed/ and a later distinct event arrives for the same session, this merge feeds every processed envelope back into the artifacts without re-deduping by idempotency_key. A same-minute SessionStart replay is correctly skipped on the first sweep, but both the original and replay are then in processed/; the next PreCompact overwrite will include duplicate SessionStart rows in the SessionNote/ToolLedger, violating the idempotency contract. Deduplicate prior by idempotency key before building envelopes, or keep replay audit copies out of the reconstruction path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in c04e0c8. The merged prior + fresh list is now deduped by idempotency key (_dedup_by_key) before building the notes, keeping the earliest chronological (uuid7-sorted) envelope per key — the original, not its retired replay. So a rebuild triggered by a later distinct event renders one row per unique event even when a same-minute replay is sitting in processed/ next to its original. Regression test: test_flush_dedups_retired_replay_history_on_rebuild.
round-5) Follow-on from the round-4 replay fix. A same-minute replay retired into processed/ lives next to its original (same idempotency key, distinct uuid7 id). When a later distinct event triggers a full rebuild, the reconstruction merged all processed envelopes back into the artifact without re-deduping — so the SessionNote/ToolLedger rendered duplicate rows for the replayed event, violating the idempotency contract. The merged (prior + fresh) list is now deduped by idempotency key before building the notes, keeping the earliest (chronological uuid7) envelope per key — the original, not its replay. Regression test: test_flush_dedups_retired_replay_history_on_rebuild. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
|
@codex review Round-5 finding addressed in c04e0c8: the projector now dedups the merged prior+fresh envelope list by idempotency key before rebuilding, so a replay retired into processed/ next to its original no longer produces duplicate rows in the SessionNote/ToolLedger. Regression test added; ruff/ty/format clean and 159 hook tests green locally. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c04e0c869a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # that was retired into processed/ next to its original doesn't resurface | ||
| # as a duplicate row here. | ||
| prior = processed_by_session.get((source, session_id), []) | ||
| envelopes = _dedup_by_key(sorted(prior + fresh_envelopes, key=lambda envelope: envelope.id)) |
There was a problem hiding this comment.
When the first same-session event for an idempotency key has no project_hint but a later replay in the same minute does, the later envelope is placed in group_replays and is excluded from this rebuild list. The project_hint scan below then sees only the unmapped first envelope, leaves the whole group pending, and retention can eventually prune it even though a valid mapping was captured. Use the replay envelopes as routing metadata before deduping the rendered event rows.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0a39f5f. Rendered rows still dedup by key, but routing now scans the complete group — (*envelopes, *replay_envelopes) — so a project_hint carried only by an in-group replay routes the group instead of leaving it pending. group_replays now carries the envelopes (not just paths) for exactly this. Regression test: test_flush_routes_on_project_hint_from_in_group_replay (first capture unmapped, later same-key replay carries demo → group routes to demo, nothing left pending).
round-6) Follow-on from the round-4/5 replay handling. When two same-minute captures of the same event share an idempotency key and the mapping was set only for the later one (primaryProject configured between the two hooks), the first envelope is fresh with an empty project_hint and the second lands in group_replays. The routing scan ran over the deduped fresh+prior list only, so it missed the replay's hint, left the whole group pending, and retention could eventually prune trace that actually had a valid mapping. Rendered rows still dedup (one per key), but routing now scans the complete group including the in-group replays, so a hint carried only by a replay still routes the group. Regression test: test_flush_routes_on_project_hint_from_in_group_replay. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
|
@codex review Round-6 finding addressed in 0a39f5f: the projector's project-hint routing now scans the complete group (fresh + prior + in-group replays), so a mapping carried only by a same-minute replay routes the group rather than leaving it pending. Rendered event rows still dedup by key. Regression test added; ruff/ty/format clean, 160 hook tests green locally. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0a39f5fd06
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "type": "command", | ||
| # The ownership tag lives in the command itself: OWNED_HOOK_COMMAND_RE | ||
| # must match it, or `bm hook remove` would orphan the entry. | ||
| "command": f"basic-memory hook {verb} --harness {harness.value}", |
There was a problem hiding this comment.
When a standalone user runs uvx basic-memory hook install (or otherwise has uv but no persistent basic-memory binary on PATH), the generated harness config invokes basic-memory ... directly instead of the shim resolver used by the plugins. Install succeeds and the uv warning does not fire, but SessionStart/PreCompact later hit command-not-found and never brief/checkpoint. Please either write a command that matches the currently resolvable launcher/uvx fallback or fail/warn when basic-memory is not actually on PATH.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3d6e78a. bm hook install now resolves the launcher the same way the shims do — basic-memory/bm on PATH first, else uvx "basic-memory>=<floor>" pinned to the running release floor — and writes that resolved command, so a uvx-only user gets a command that resolves at hook time instead of command-not-found. The ownership regex now keys on the hook <verb> --harness <harness> suffix so remove still strips every launcher form. With nothing resolvable it still writes the basic-memory form and warns about the missing uv (unchanged behavior). Regression tests: test_install_uses_uvx_launcher_when_no_binary_on_path and test_install_prefers_bm_when_basic_memory_absent.
| for path in inbox.list_envelopes(): | ||
| result.swept += 1 | ||
| try: | ||
| entries.append((path, envelope_from_json(path.read_text(encoding="utf-8")))) |
There was a problem hiding this comment.
If a pending envelope disappears between list_envelopes() and read_text() (for example two bm hook flush invocations overlap) or is temporarily unreadable, read_text raises OSError/FileNotFoundError and aborts the whole sweep, so other valid envelopes are not projected and the flush marker is not recorded. This path only catches JSON/shape errors even though invalid/unreadable entries are supposed to be left in place and counted; catch OSError here and skip/count the entry or tolerate missing files.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3d6e78a. The inbox load loop now catches OSError around read_text and skips the entry (not counted invalid, since a vanished/locked file isn't corrupt trace). A file disappearing between list_envelopes() and read_text() — an overlapping flush, or a transiently unreadable file — no longer aborts the sweep, so the remaining valid envelopes still project and the flush marker is still recorded. Regression test: test_flush_skips_vanished_inbox_file.
…files (PR #1070 round-7) Two P2 findings from the re-review of 0a39f5f: - install: `bm hook install` always wrote a bare `basic-memory hook ...` command. A standalone user with uv but no persistent basic-memory/bm on PATH (installed via `uvx basic-memory hook install`) would then hit command-not-found at SessionStart/PreCompact. Install now resolves the launcher like the shims do — basic-memory / bm on PATH, else a `uvx "basic-memory>=<floor>"` fallback pinned to the running release floor — and writes that. The ownership regex now keys on the `hook <verb> --harness <harness>` suffix so `remove` still recognizes every launcher form (including the uvx one). With nothing resolvable it still writes the basic-memory form and warns about the missing uv (unchanged). - flush: the inbox load loop only caught JSON/shape errors, so a pending file vanishing between list_envelopes() and read_text() (overlapping flushes) or a transiently unreadable file raised OSError and aborted the whole sweep — skipping valid envelopes and the flush marker. OSError is now caught and the entry skipped (not counted invalid, since it isn't corrupt trace). Regression tests: install resolves uvx / bm launchers and remove strips the uvx form; flush skips a vanished inbox file and still projects + records the marker. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
|
@codex review Round-7 findings addressed in 3d6e78a:
Regression tests added; ruff/ty/format clean, 163 hook tests green locally. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d883a053c9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if len(data_text) > room: | ||
| data_text = data_text[: max(0, room - len(notice))].rstrip() + notice | ||
| lines = [opening + data_text + closing] |
There was a problem hiding this comment.
When a search result title/permalink contains a very long backtick run, _fence(data_lines) can make opening + closing leave less than len(notice) bytes of room. This branch still appends the full notice and then _session_start slices the returned brief to MAX_BRIEF_CHARS, so the closing fence can be cut off; the untrusted graph block then remains open into the following prompt, which is the boundary this code is trying to enforce. Cap/escape the fence or omit the notice when it cannot fit.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 93245ed. _fence now first collapses backtick runs over _MAX_FENCE_RUN (32) in the untrusted data, so the fence stays bounded (≤33) and always fits under MAX_BRIEF_CHARS — the closing fence can't be truncated away. Realistic nesting (3-4 backticks) is untouched, so the grow behavior is preserved. Regression test: test_session_start_bounds_fence_for_absurd_backtick_run (a 12k-backtick title still yields a bounded fence that opens and closes under the cap).
| prior = processed_by_session.get((source, session_id), []) | ||
| envelopes = _dedup_by_key(sorted(prior + fresh_envelopes, key=lambda envelope: envelope.id)) |
There was a problem hiding this comment.
When a same-minute replay is the first envelope that carries project_hint (the case handled below by scanning group_replays), both the no-hint original and hinted replay are moved into processed/. On a later sweep for the same session, this _dedup_by_key(...) keeps the earlier no-hint processed envelope and drops the hinted replay before routing scans envelopes, while routable_sessions still prevents retention from pruning new hintless pending events. Those events can stay pending forever even though the session has a processed routing hint; preserve project hints from the full processed history separately from rendered-row deduping.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 93245ed. Routing now scans the full un-deduped history — (*prior, *fresh_envelopes, *replay_envelopes) — for the project hint, while _dedup_by_key is applied only to the rendered rows. So a hint carried by a replay that dedup drops from processed/ still routes a later same-session event, instead of it staying pending forever (with routable_sessions sparing it from pruning). Regression test: test_flush_routes_new_event_via_processed_replay_hint.
…1070 round-24) Two P2 follow-ons: - brief fence: `_fence` grew the fence to outlength the longest backtick run in the data, but an absurd run (a title with thousands of backticks) made the fence itself exceed MAX_BRIEF_CHARS — the caller's slice then cut the closing fence, reopening the prompt-injection boundary. `_fence` now first collapses runs over `_MAX_FENCE_RUN` (32) in the untrusted data, so the fence stays bounded and always fits; realistic nesting (3-4) is untouched. - projector routing: when a no-hint original and its hinted same-key replay both sit in processed/, `_dedup_by_key` keeps the earlier hint-less one for the rendered rows — and routing scanned that deduped list, so a later distinct event for the session found no hint and stayed pending forever (routable_sessions also spared it from pruning). Routing now scans the full un-deduped history (prior + fresh + replays); rendered rows still dedup. Regression tests: an absurd backtick run yields a bounded fence that opens and closes under the cap; a new event routes via a hint carried only by a processed replay. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 93245ed8f6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| older_than_days: int = typer.Option( | ||
| 30, "--older-than-days", help="Retention window for processed envelopes" | ||
| ), |
There was a problem hiding this comment.
When an operator accidentally passes a negative value, e.g. bm hook flush --older-than-days -1, Typer accepts it and run_flush forwards it to retention; _prune_dir then computes a cutoff in the future, so every eligible processed envelope and every fully-unmapped pending envelope is pruned instead of retaining the configured window. Add a lower bound such as min=0 or validate before calling the projector.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ac9702e. The --older-than-days option now has min=0, so a negative value is rejected by Typer before it can push the retention cutoff into the future and prune everything. Regression test: test_flush_rejects_negative_retention_window.
| escaped = re.escape(root).replace("/", r"[/\\]") | ||
| patterns.append(re.compile(escaped + r"(?![A-Za-z0-9_-])(?:[/\\]\S*)?")) |
There was a problem hiding this comment.
On Windows, filesystem paths are case-insensitive, but these deny-path regexes are compiled case-sensitively. If ~/.ssh/ or a configured redactPaths entry expands with a different drive/user casing than the hook payload or transcript text (for example C:\Users\Alice\.ssh\ vs c:\users\alice\.ssh\id_rsa), the sensitive path is not redacted even though it is the same directory. Compile with re.IGNORECASE on Windows or normalize both sides consistently before matching.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ac9702e. Deny-path patterns now compile with re.IGNORECASE on Windows (os.name == 'nt'), so a different drive/user casing between expanduser and the payload/transcript still matches the same directory. POSIX stays case-sensitive — /home/Alice and /home/alice are genuinely distinct there. Regression tests: test_deny_paths_match_case_insensitively_on_windows and test_deny_paths_stay_case_sensitive_on_posix.
…Windows (PR #1070 round-25) Two P2 findings: - flush: `bm hook flush --older-than-days -1` was accepted, and a negative window puts the retention cutoff in the *future*, so every processed and unmapped-pending envelope was pruned. The Typer option now has `min=0`. - redaction: deny-path regexes were compiled case-sensitively, but Windows filesystems are case-insensitive — `C:\Users\Alice\.ssh` (from expanduser) vs `c:\users\alice\.ssh\id_rsa` (payload) is the same directory yet went unredacted. The patterns now compile with `re.IGNORECASE` on Windows only; POSIX stays case-sensitive (`/home/Alice` != `/home/alice`). Regression tests: negative --older-than-days is rejected; a mixed-case path redacts under a simulated Windows os.name and stays intact on POSIX. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac9702e7bc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # A bare "/" (or empty) deny path would redact everything; skip it. | ||
| continue | ||
| escaped = re.escape(root).replace("/", r"[/\\]") | ||
| patterns.append(re.compile(escaped + r"(?![A-Za-z0-9_-])(?:[/\\]\S*)?", flags)) |
There was a problem hiding this comment.
When a configured redactPaths directory contains descendants with spaces, this pattern only consumes non-whitespace after the denied root, so a value like /srv/clients/acme corp/secret.txt becomes [REDACTED_PATH] corp/secret.txt and still leaks part of the sensitive path into the inbox or checkpoint notes. This is especially likely for client/project folders or filenames with spaces; handle whole-value paths with path-prefix logic or allow the descendant match to cover spaces safely.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 5723092. _redact_str now runs a whole-value path-prefix check first — == root or startswith root + "/" on the normalized value (case-folded on Windows). If the entire value is the denied directory or a descendant, it collapses to the marker regardless of spaces, so a cwd/path value like /srv/clients/acme corp/secret.txt no longer leaks corp/secret.txt. The substring pass still handles a path embedded mid-prose. _deny_path_patterns returns (root, matcher) pairs for this. Regression tests: test_deny_paths_redact_whole_value_path_with_spaces and test_redact_text_redacts_whole_value_path_with_spaces.
…round-26) The deny-path substring matcher's `\S*` descendant tail stops at whitespace, so a whole-value path with a space — a client/project dir like `/srv/clients/acme corp/secret.txt` — was redacted only up to the space, leaking ` corp/secret.txt` into the inbox/checkpoint. `_redact_str` now runs a whole-value path-prefix check first (`== root` or `startswith root + "/"` on the normalized value, case-folded on Windows): if the entire value is the denied directory or a descendant, it collapses to the marker regardless of spaces. The substring pass still handles a path embedded in prose. `_deny_path_patterns` now returns `(root, matcher)` pairs so the root drives the whole-value check and the pattern the in-prose match. Regression tests: a whole-value path with a space redacts fully in both a payload value and free text. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5723092e4d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # The normalized root is paired with its substring matcher: the root | ||
| # drives the whole-value path-prefix check (which tolerates spaces the | ||
| # substring `\\S*` tail would truncate), the pattern the in-prose match. | ||
| matchers.append((root, re.compile(escaped + r"(?![A-Za-z0-9_-])(?:[/\\]\S*)?", flags))) |
There was a problem hiding this comment.
When a denied path appears inside transcript/checkpoint prose and a descendant component contains spaces, this matcher only consumes through the first space because of \S*. For example, with redactPaths: ["/srv/clients/"], text like please inspect /srv/clients/acme corp/secret.txt becomes please inspect [REDACTED_PATH] corp/secret.txt, leaking the rest of the configured private path into notes or envelopes despite the redaction floor.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9931d5f. The descendant matcher now consumes intermediate directory components that contain spaces: it walks [/\]<seg> runs where ends in a non-whitespace char immediately followed by a separator (a proven directory). So please inspect /srv/clients/acme corp/secret.txt now redacts the whole path, not just /srv/clients/acme.
The trailing-non-space requirement is deliberate — it distinguishes a spaced dir name (acme corp/, where p precedes the slash) from prose sitting between two paths (id_rsa and /home/..., where a space precedes the slash), so compare <p1> and <p2> stays two independent matches instead of collapsing the connecting words into one marker. The final component still uses \S* (stops at whitespace): once no separator follows, a trailing space can't be told apart from prose, and over-redacting prose is the worse error for a checkpoint.
Regression tests added: embedded spaced-dir path redacts whole; prose after a space-free path is preserved; two spaced-dir paths in one sentence stay separate.
There was a problem hiding this comment.
Following up on my previous reply: I've reverted the spaced-directory descendant heuristic I proposed for this (it shipped briefly in 9931d5f) and restored the simpler (?:[/\]\S*)? tail in 8ff92a7.
The heuristic broke Windows CI. It consumed [/\]<seg> runs where ended in a non-whitespace char before a separator — the idea being that a spaced dir like acme corp/ has p right before the slash, whereas prose between two paths (id_rsa and /home/...) has a space there. That distinction holds on POSIX but not on Windows: the connecting text between two absolute paths ends in a drive letter (... and C:\...), so the : before the \ satisfied the rule and the matcher bridged two independent paths into a single marker (broke test_redact_text_redacts_multiple_embedded_paths).
Rather than pile platform-specific special-casing (drive letters, UNC, etc.) onto an already-ambiguous problem, I'm keeping the robust behavior: \S* never crosses whitespace, so it can't bridge paths on either platform. The trade-off is that a denied path embedded in prose whose directory contains a space is truncated at that space — the sensitive root and leading component are still redacted, but a spaced tail can survive.
I think that residual is acceptable because the real capture channel — whole-value cwd and payload string values — is redacted in full by the path-prefix check from the earlier fix (spaces and all, case-insensitively on Windows). Embedded-in-prose spaced paths are the narrow, genuinely-ambiguous case, and I'd rather truncate deterministically than ship a heuristic that guesses wrong across platforms. Open to a concrete non-fragile approach if you have one.
…1070 round-27) Round-26 fixed whole-value denied paths with spaces. This handles the embedded variant: a denied path mid-prose whose intermediate directory has a space (`please inspect /srv/clients/acme corp/secret.txt`) was truncated at the first space by the `\S*` tail, leaking ` corp/secret.txt` into notes/envelopes. The descendant matcher now consumes `[/\\]<seg>` runs where <seg> is non-separator text ending in a non-whitespace char immediately followed by a separator — a proven intermediate directory. That trailing-non-space requirement is what separates a spaced dir name (`acme corp/` — `p` precedes the slash) from prose between two paths (`id_rsa and /home/...` — a space precedes the slash), so `compare <p1> and <p2>` stays two independent matches rather than collapsing the connecting prose into one. The final component still falls back to `\S*` (stops at whitespace): once no separator follows, a trailing space is ambiguous between a spaced filename and prose, and over-consuming prose is the worse error for a checkpoint. Regression tests: embedded spaced-dir path redacts whole; prose after a space-free path is preserved; two spaced-dir paths in one sentence stay separate (guards the over-consumption failure this could reintroduce). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Two Windows-only unit failures on this branch, both surfaced now that CI ran to completion instead of being cancelled by a rapid re-push: 1. test_deny_paths_stay_case_sensitive_on_posix (added round-25) had no os.name pin, so on a Windows host — where deny paths are matched case-insensitively — `/home/ALICE/...` redacted against `/home/alice/...` and the "must NOT match" assertion failed. Pin os.name="posix" via monkeypatch, mirroring the "nt" pin in the companion case-insensitive test. 2. Revert the round-27 spaced-directory descendant heuristic. It consumed `[/\\]<seg>` runs where <seg> ended in a non-whitespace char before a separator, meant to redact `/srv/clients/acme corp/secret.txt` whole. That rule is fragile: on Windows the connecting prose between two paths ends in a drive letter (`... and C:\...`), so the `:` before `\` satisfied it and the matcher bridged two independent paths into one marker (test_redact_text_redacts_multiple_embedded_paths). Restore the robust `(?:[/\\]\S*)?` tail, which never crosses whitespace and so cannot bridge paths on either platform. The round-26 whole-value path-prefix check is kept: it redacts spaced paths in full through the real capture channel (cwd, payload string values) on both platforms. Embedded-in-prose spaced paths are truncated at the first space on purpose — telling a spaced path apart from "path then prose" is genuinely ambiguous, and no heuristic did it without swallowing prose or breaking on Windows. Test renamed/rewritten to document the intended truncation. Verified: 118 hooks tests pass, redaction.py at 100%; simulated os.name="nt" run confirms multi-path stays separate, whole-value spaced paths redact fully, and case-insensitive matching still works. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8ff92a7b80
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # on Windows drive letters (`C:\\...`). Whole-value path values (the real | ||
| # capture channel — `cwd`, payload strings) are handled robustly by the | ||
| # path-prefix check above, spaces and all. | ||
| matchers.append((root, re.compile(escaped + r"(?![A-Za-z0-9_-])(?:[/\\]\S*)?", flags))) |
There was a problem hiding this comment.
Fresh evidence after the earlier fix: the current matcher again stops the descendant match at the first whitespace, so transcript/checkpoint prose like please inspect /srv/clients/acme corp/secret.txt with redactPaths: ["/srv/clients/"] is persisted as [REDACTED_PATH] corp/secret.txt. Whole-value cwd redaction does not cover these embedded transcript excerpts, so configured private client/project paths can still leak into checkpoint notes and envelopes.
Useful? React with 👍 / 👎.
| f"created_by: {CREATED_BY_PREFIX}/{first.source}", | ||
| f"caused_by_event: {first.id}", | ||
| ] | ||
| lines += [f"{key}: {value}" for key, value in to_frontmatter_fields(first).items()] |
There was a problem hiding this comment.
When a captured envelope carries an opaque source_turn_id containing YAML-special text such as turn: 42 or a newline, this hand-built frontmatter becomes invalid before write_note can persist the projected artifacts. In that case bm hook flush leaves the whole session pending on every retry even though the envelope shape is otherwise valid; build these fields through the metadata/YAML serializer or quote the values before emitting them.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 64b7f2e. The projector no longer hand-builds a --- frontmatter block — provenance now goes through write_note(metadata=...), which serializes/quotes values safely. _artifact_metadata returns a dict (created_by, caused_by_event, the to_frontmatter_fields, and status: open for the session note); _session_note/_tool_ledger_note return (title, body, metadata) and _write_artifact forwards it. So an opaque source_turn_id like turn: 42 or one containing a newline can no longer produce invalid YAML and wedge the session pending.
This is the same metadata= pattern the pre-compact checkpoint already uses. Regression test added (test_flush_passes_yaml_special_turn_id_through_metadata): an envelope with a YAML-special turn id flushes successfully and the raw value reaches write_note as a structured metadata value, not spliced into the body.
Self-review of the SPEC-55 hooks module against docs/ENGINEERING_STYLE.md
turned up code that reads like a C module, not basic-memory: primitive
threading, hand-rolled validation, and repeated anonymous shapes. This
restructures it to the house style — dataclasses for internal values, Pydantic
v2 at the persistence boundary, type aliases for domain concepts — with no
behavior change (redaction output, WAL format, and flush semantics are
identical; every module stays at 100% coverage).
redaction.py
- Introduce a `Redactor` frozen/slots dataclass owning the compiled ruleset,
and a `DenyPath` value object for the (root, pattern) pair. Previously three
parallel collections (deny_key_patterns, a nameless
`tuple[tuple[str, re.Pattern], ...]`, and the entropy plugins) were threaded
by hand through five module functions, and `_redact_str` rebuilt the tuple
of roots on every single string.
- `Redactor.build(...)` compiles the ruleset once; callers redacting many
values (envelope create, the pre-compact checkpoint's per-turn and
git-status loops) reuse one redactor instead of recompiling deny patterns
and re-expanding paths per string.
- `redact_payload` / `redact_text` stay as thin one-shot wrappers.
- `type EntropyPlugins` names the detect-secrets plugin map.
envelope.py
- `Envelope` becomes a Pydantic v2 model (frozen, extra="forbid", strict).
This deletes ~40 lines of hand-rolled `envelope_from_json` validation plus
the two `_STR_FIELDS`/`_OPTIONAL_STR_FIELDS` tuples that had to be kept in
sync with the fields by hand: strict mode rejects wrong scalar types,
extra="forbid" rejects unknown keys, and a `field_validator` pins the
version. `pydantic.ValidationError` is a `ValueError`, so the projector and
inbox handlers catch it unchanged. Serialization delegates to
`model_dump_json` / `model_validate_json`.
- Add `type SessionKey = tuple[str, str]` and an `Envelope.session_key`
property for the (source, session_id) concept the inbox and projector group,
prune, and route by.
inbox.py / projector.py
- Replace the repeated anonymous `tuple[str, str]` / `dict[tuple[str, str], …]`
with `SessionKey` and `envelope.session_key`.
Tests: envelope validation assertions updated to Pydantic's messages (same
inputs rejected); added Redactor reuse + DenyPath tests. 205 hooks/CLI tests
pass; ruff, format, and ty clean.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: phernandez <paul@basicmachines.co>
…1070) The projector hand-built the artifact frontmatter as a `---` text block. One of its fields, `envelope_turn_id`, comes from `source_turn_id`, which SPEC-55 defines as opaque, surface-defined text. A value with YAML-special content — `turn: 42`, a stray colon, a newline — produced an invalid frontmatter block, so write_note failed to persist the artifact and the whole session stayed pending on every flush retry (the envelope shape is otherwise valid, so it never ages out as invalid either). This is the same failure the pre-compact checkpoint hit and fixed in round-21: build a `metadata` dict and pass it through `write_note(metadata=...)`, which serializes/quotes values safely, instead of splicing raw text into a hand-built block. `_artifact_metadata` now returns that dict (`created_by`, `caused_by_event`, plus `to_frontmatter_fields`, and `status: open` for the session note); `_session_note`/`_tool_ledger_note` return `(title, body, metadata)`; `_write_artifact` forwards it. The note `type` still rides the `note_type` arg. Body content no longer carries a frontmatter block. Regression test: an envelope whose `source_turn_id` contains `turn: 42\n...` flushes successfully, with the raw value reaching write_note as a structured metadata value rather than spliced into the body. Reported by Codex on PR #1070 (projector.py:140). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 64b7f2e4fb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ) | ||
| # Rendered rows still dedup by key so a retired replay in processed/ | ||
| # doesn't resurface as a duplicate row. | ||
| envelopes = _dedup_by_key(sorted(prior + fresh_envelopes, key=lambda envelope: envelope.id)) |
There was a problem hiding this comment.
When two bm hook flush runs overlap, this snapshot can be stale: one flush may list only the old pending envelopes while a second flush lists an additional event, writes the full artifact, and moves that new event to processed/; if the stale flush writes afterward, overwrite=True rewrites the SessionNote/ToolLedger from prior + fresh_envelopes without the newer processed event. Because the newer event is already retired, a later sweep with no fresh pending group does not rebuild the artifact, so that event stays missing until another event arrives. A per-inbox/session flush lock or a re-read/compare immediately before overwrite would avoid stale overwrites.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 14b07c7. flush() now runs under an exclusive advisory lock over the inbox (inbox.flush_lock(), via filelock — fcntl on POSIX, msvcrt on Windows). A flush that arrives while the lock is held skips rather than blocks: the running flush sweeps the whole inbox, so the skipped run loses no work, and skipping is exactly what prevents the stale overwrite you described. The lock is released on process death, so a crashed flush strands nothing.
The flush body moved to _flush_locked; flush() is the thin lock wrapper, and a new FlushResult.skipped flag is surfaced by bm hook flush ("flush skipped: another flush is already running"). filelock was already resolved transitively via huggingface-hub; it's now a declared direct dependency. Regression test added (test_flush_skips_when_another_flush_holds_the_lock): with the lock held, flush writes nothing and leaves the envelope pending for the next unlocked sweep.
…(PR #1070) Two overlapping `bm hook flush` runs could race: flush B lists {B, C}, writes the full artifact, and retires C to processed/; a stale flush A (snapshot {B}) then overwrites the SessionNote/ToolLedger from {A, B} with overwrite=True, dropping C's row. Because C is already retired, a later sweep with no fresh pending in that group never rebuilds the artifact, so C stays missing until another event arrives in the session (the envelope trace itself is safe in processed/ — it's the rendered artifact that loses the row). flush() now runs under an exclusive advisory lock over the inbox (`inbox.flush_lock()`, via filelock — fcntl on POSIX, msvcrt on Windows). A flush that arrives while the lock is held skips rather than blocks: the running flush sweeps the whole inbox, so the skipped run loses no work, and skipping avoids exactly the stale overwrite. The lock releases on process death, so a crashed flush strands nothing. The flush body moved to _flush_locked; a new FlushResult.skipped flag is surfaced by `bm hook flush`. filelock was already resolved transitively (via huggingface-hub); declared as a direct dependency now that core imports it. Regression test: with the inbox lock held, flush() skips — writes nothing, leaves the envelope pending for the next unlocked sweep. Reported by Codex on PR #1070 (projector.py:360). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 14b07c7d6f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # same floor as the transcript text and cwd (a secret token in a | ||
| # filename, or a denied path, must not leak into the note). | ||
| body += ["", "## Working tree"] | ||
| body += [f"- `{redactor.redact_text(line)}`" for line in status_lines] |
There was a problem hiding this comment.
When Codex checkpoints run from a directory covered by redactPaths, safe_cwd is redacted but the working-tree rows are still the raw git status --short output, which contains repo-relative filenames like M customer-roadmap.md without the denied absolute prefix. In that case redactor.redact_text(line) cannot match the configured path prefix, so sensitive filenames from a denied workspace are persisted in the checkpoint; skip this section when the cwd is redacted or redact absolute paths before formatting the rows.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d651ba2. When safe_cwd == REDACTED_PATH (the cwd matched a redactPaths entry, so the whole workspace is denied), the checkpoint now skips the "## Working tree" section entirely — since git status --short rows are repo-relative filenames with no absolute prefix, per-row redaction can't match the deny path and the denied file list would otherwise leak. When the workspace is not denied, rows are still per-row redacted (to catch a secret in a filename or an absolute denied path in a row).
Regression test added (test_pre_compact_codex_skips_working_tree_when_workspace_denied): a codex pre-compact whose cwd is under redactPaths omits both the section and the filenames, while the cwd stays redacted in frontmatter.
… (PR #1070) When a Codex checkpoint runs from a directory covered by `redactPaths`, the cwd is redacted — but the "## Working tree" section rendered raw `git status --short` rows, which are repo-relative filenames (`M customer-roadmap.md`) with no absolute prefix. Per-row `redact_text` can't match the deny path against a bare relative filename, so the denied workspace's file list leaked into the checkpoint, defeating the user's redaction intent. Skip the working-tree section entirely when `safe_cwd == REDACTED_PATH` (the cwd matched a redactPaths entry, so the whole workspace is denied and every row is sensitive). Rows are still per-row redacted when the workspace is not denied, to catch a secret in a filename or an absolute denied path appearing in a row. Only the Codex path renders this section (include_workspace_sections). Regression test: a codex pre-compact whose cwd is under redactPaths omits the working-tree section and the filenames, while the cwd stays redacted in frontmatter. Reported by Codex on PR #1070 (hook.py:729). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d651ba2e8b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return None | ||
| escaped = re.escape(root).replace("/", r"[/\\]") | ||
| flags = re.IGNORECASE if case_insensitive else 0 | ||
| pattern = re.compile(escaped + r"(?![A-Za-z0-9_-])(?:[/\\]\S*)?", flags) |
There was a problem hiding this comment.
When a checkpoint/transcript excerpt contains a denied path whose descendant includes a space, such as redactPaths: ["/srv/clients/"] and text mentioning /srv/clients/Acme Corp/roadmap.md, this \S* tail only replaces /srv/clients/Acme and leaves Corp/roadmap.md in the generated note. Since checkpoint excerpts are passed through redact_text, the privacy floor still leaks part of the denied path and filename for these inputs; the embedded-path matcher needs to consume the full spaced descendant or otherwise redact the remainder.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Accepting this as a best-effort limitation rather than fixing it — deliberately, not by oversight. The embedded-in-prose spaced-path case is genuinely ambiguous: there's no way to tell /srv/clients/Acme Corp/roadmap.md (one path with a spaced dir) from /srv/clients/Acme and see roadmap.md (a path, then prose) with a regex. I did try a heuristic that consumed spaced intermediate directories (earlier on this branch); it broke Windows CI because drive letters (... and C:\\...) satisfied the 'intermediate dir' rule and bridged two separate paths into one redaction. I reverted it rather than ship a platform-fragile matcher.
What is covered: whole-value paths (cwd, payload string values — the real capture channel) redact in full via the path-prefix check, spaces and all; and the pre-compact working-tree section is now skipped entirely when the workspace is denied. The residual is narrow: a denied path with a space typed into transcript prose.
The Stage-1 floor is explicitly best-effort over the user's own notes (not adversarial egress); the deeper NER/model pass is Stage 2 (basic-memory[redaction], phase 2). For a deterministic hot-path floor we prefer truncating at the space over a heuristic that guesses wrong across platforms. test_redact_text_truncates_embedded_spaced_path_at_whitespace documents this as intended.
1 participant
Summary
Implements the harness capture producer front door (SPEC-55 revision 2026-07-15, the plan posted on #997). Coding-agent lifecycle hooks are captured as normalized, redacted producer envelopes in a local WAL and projected into knowledge-graph artifacts out of band — with the plugins reduced to configuration and all logic living in the
basic-memorypackage where it is tested, typed, and released.Supersedes #998 and #1064: the envelope design from those PRs carries forward into
basic_memory/hooks/with commit credit to @sourrrish. The prior attempts embedded Python in shell heredocs and vendored a shared module that never survived packaging; this replaces both with a CLI front door.Architecture
bm hook <event> --harness <name>with stdin passthrough. bm is resolved installed-first (BM_BIN→basic-memory/bmon PATH), falling back touvx "basic-memory>=<floor>"so no local install is required; uv is the documented per-platform prerequisite and the floor is bumped by the release version updater.bm hookCLI (basic_memory/hooks/+cli/commands/hook.py): per-harness stdin adapters → envelope → Stage-1 redaction floor → atomic WAL inbox. Verbs:session-start,pre-compact,flush,status,install,remove. Execution is fail-open (a hook never disrupts a session); thecaptureEventsgate is fail-closed (must be booleantrue).<bm-home>/inbox/<uuid7>.json(atomic tmp+rename, one file per envelope). UUIDv7 ids sort chronologically, so the projector processes in capture order with no mtime dependence, and ids double as thecaused_bytarget. Lives in BM home by requirement — plugin dirs are ephemeral and deleted on uninstall.bm hook flush): sweeps the whole inbox, groups by session, dedups on idempotency key (envelopes are hints, never double-written), and projects SessionNote + ToolLedger artifacts carryingcreated_by+caused_by_eventprovenance. No LLM — the SPEC-54 daemon worker supersedes it later without reworking the inbox contract.Review
Built and reviewed via a multi-agent workflow: five review lenses (correctness, security/privacy, portability, platform conformance, style/tests) over the branch diff, each finding adversarially verified. Five confirmed findings were fixed with regression tests (final commit):
mark_processedcrashed the sweep on a double-retire — now tolerated.uuid7_unix_msnow fails fast.Test plan
uv run pytest tests/hooks tests/cli/test_hook_command.py— 146 passed (unit coverage of uuid7, envelope, redaction floor +redact_text, inbox atomicity/ordering/retention/concurrency, adapters against recorded fixtures, projector dedup/replay-safety/incremental-flush/mapping-gate, and the CLI verbs incl. captureEvents fail-closed and checkpoint redaction).uv run ruff check+ruff format— clean.uv run ty check src/basic_memory/hooks src/basic_memory/cli/commands/hook.py— clean.just package-check— passes (Claudeplugin validate --strict, Codex layout, Hermes, OpenClaw, skills).Fixes #997
Closes #998
🤖 Generated with Claude Code