Fix global-flag handling (#3) + full bug-review batch (HIGH/MEDIUM)#4
Merged
Conversation
Global flags (--project, --json, --plain, --verbose) live on a shared parent parser inherited by every subparser so they can appear before or after the subcommand. argparse's _SubParsersAction.__call__ parses the subcommand into a fresh namespace and copies its keys back over the parent namespace, clobbering any global set before the subcommand with the subparser's default. The result: `crumb --project X validate` silently ran against cwd instead of X, and `--json`/`--plain`/`--verbose` before the subcommand were ignored too. Fix: give the shared globals default=SUPPRESS so an absent flag never lands in the sub-namespace (and so never overwrites the parent value), and backfill the real defaults once in a top-level parser subclass. Subparsers stay plain ArgumentParsers so the backfill happens exactly once, at the top. Adds GlobalFlagPositionTests covering both flag positions, the nested `remember decision` case, default presence, and the real-world unrelated-cwd scenario. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Three confirmed defects on the record read path: - H1: a UTF-8 BOM before the opening `---` survives str.strip(), so the fence wasn't recognized and the entire frontmatter was silently dropped. parse_frontmatter now strips a leading BOM; Record.from_file reads with utf-8-sig. - H2: a non-UTF-8 / binary .md (or a directory / broken symlink matching *.md) raised an uncaught UnicodeDecodeError/OSError that crashed load_records and validate over the whole store. from_file now captures any OS/decode error as a Record.error instead of raising. - M1: a quoted scalar with a trailing comment (`ref: "abc" # c`) kept its quotes because the closing-quote test looked at the last char. _parse_scalar now returns the content up to the matching closing quote (a `#` inside the quotes is still preserved). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Two ways the write path could silently corrupt a stored record:
- A list scalar containing ': ' or ending ':' (e.g. a tag "area: backend",
or a renamed dirty file) was rendered bare and then re-read as a
{key: value} map. _is_map_item is now quote-aware and block-list scalars
that contain a colon are quoted on render, so they round-trip as strings.
- A newline in a frontmatter field (most easily via --title) truncated the
value and injected the remainder as bogus frontmatter keys, slipping past
the post-write validate gate. The line-based YAML subset cannot represent
a multi-line scalar, so _render_scalar now refuses it and cmd_remember
surfaces a clean error with no record written.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- M7: add named patterns for GitHub fine-grained PATs (github_pat_…) and Stripe-style keys (sk_live_/rk_test_/…), and widen the OpenAI pattern so the modern sk-proj-… shape (the hyphen broke the alnum-only pattern) is matched as a known key, not left to the entropy fallback. - M6: validate now checks `privacy` against the allowed vocabulary. A typo such as `secret-prohibitted` previously passed validate 100% clean and evaded the exact-match secret-prohibited leak gate — the one "must never be stored" safety net. It now fails validation like an invalid status does. Note: the lowercase-hex entropy gap (a bare 32-64 char hex secret) is left as-is by design — it is shape-indistinguishable from the SHA-1/256 hashes that pervade memory, and the scanner deliberately trades that false-negative to avoid flagging every commit sha (see the _looks_high_entropy docstring). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…(H5, M9, M10)
- H5: open questions never reached the verdict. The live/history split keyed
on status == "active", but questions carry status == "open", so every open
question was demoted to history and the open-blocker READ_FIRST floor was
unreachable — guard returned PROCEED on an area an open question explicitly
blocks. The split now treats an open question as live.
- M9: _norm_files turned a trailing-slash directory path ("src/auth/") into an
empty basename "", so any two directory paths spuriously overlapped on "".
Empty basenames are no longer added.
- M10: _score_item collapsed matched files by basename, so two distinct files
sharing a name (src/a/x.ts, src/b/x.ts) counted as one — undercounting the
score and citing a hash-order-dependent (nondeterministic) survivor. It now
counts distinct full paths plus uncovered bare-basename matches, sorted.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… clobber (M3, M4, M5)
- M3: a renamed file in `git status --porcelain` ("R old -> new") was stored
verbatim as the single bogus path "old -> new". git_dirty_files now records
the destination path.
- M4: update_handoff/update_current re-emitted only the canonical section list,
so any user-added section was silently deleted on the next capture. Both now
preserve unmanaged sections (with real content) after the managed ones.
- M5: a capture with no new commits set "Recently Changed" to the placeholder
"_(no new commits)_", which is truthy and overwrote a previously meaningful
summary. _is_placeholder now recognizes the `_(...)_` form (and stops treating
a `<https://...>` autolink as a stub), and the updaters drop placeholder inputs
before falling back to existing content.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ully trimmed (M8) The "… N more omitted to stay within the token budget" note was only emitted inside the truthy branch of each section. Budget-trimming (TRIM_ORDER hits verification and likely_files first) can empty a section entirely while omitted[key] > 0, so the section rendered "_(none recorded)_" and silently dropped the disclosure — the packet falsely claimed nothing was there. The note now renders in both branches for all six sections. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…rors (H3, M11) - H3: `init --force` ran shutil.rmtree(.project-memory) *before* the copy that could fail, so a missing/broken template tree destroyed the user's store and left nothing. The scaffold is now built in a `.project-memory.new` staging dir and swapped in only after it is fully built; on any failure the staging dir is removed and the existing store is left intact. - M11a: `init` checked exists() but not is_dir(), so a file as --project passed the guard and crashed in copytree with NotADirectoryError. It now returns a clean "project root is not a directory" error. - M11b: command exceptions escaped main() as raw tracebacks. main() now routes expected OSError/ValueError (missing template, permissions, unrepresentable values) through _emit_error with a nonzero exit; programming errors still propagate. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ing (MCP1/4/5)
- MCP1: memory://decisions/{id} and memory://attempts/{id} resolved by id via
find_record_by_id, which ignores the record kind, so an attempt URI could
return a decision record. _record_text now enforces rec.rtype == kind.
- MCP4: the memory_search tool dropped the `files` parameter that the underlying
tool_search/guard support, so MCP clients couldn't do file-scoped search. It
is now exposed and passed through.
- MCP5: the optional-SDK import guard caught only ImportError; a partial or
version-skewed `mcp` install could raise something else and hard-fail module
import. It now catches any import-time exception and degrades to the hint.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![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: 31ce8a7a94
ℹ️ 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".
Comment on lines
+656
to
+658
| if " -> " in path: | ||
| # rename/copy entries are "R old -> new"; record the destination. | ||
| path = path.split(" -> ", 1)[1].strip() |
There was a problem hiding this comment.
When a dirty path literally contains -> but is not a rename/copy entry, this branch still treats it as a rename and records only the suffix; for example git status --porcelain prints an untracked file named a -> b as ?? "a -> b", and git_dirty_files() returns b". That incorrect dirty_files value gets written into new records and weakens later file-based search/guard matching, so the split should be gated on the porcelain status being rename/copy rather than the path text alone.
Useful? React with 👍 / 👎.
Comment on lines
+1074
to
+1078
| if "\n" in s or "\r" in s: | ||
| # The frontmatter is a line-based YAML subset with no multi-line scalar | ||
| # support: a newline would silently truncate the value and inject the | ||
| # remainder as bogus keys. Reject it at the source instead of corrupting. | ||
| raise ValueError("frontmatter values must be single-line (no newlines)") |
There was a problem hiding this comment.
When this new newline rejection is reached via mcp_core.tool_record—for example an MCP client sends a payload.title, tag, evidence ref, scope, or agent containing \n—the ValueError propagates out of the memory_record tool because only the CLI path catches it. Other invalid MCP writes return structured {"ok": false, ...} responses, so this input instead becomes an RPC/tool exception rather than the documented validate-gated result; catch the exception around the MCP write_record call and return a normal error dict.
Useful? React with 👍 / 👎.
This was referenced Jun 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes the global-flag bug (#3) plus a full-codebase bug review: every HIGH and
MEDIUM defect found across the parser, write path, validate/secret scan, guard/
search, capture/handoff, resume packet, init, and MCP adapter. 9 commits, 30
new regression tests, all TDD'd (failing test first). Suite: 195 passed + 60
subtests (was 165), zero regressions.
Closes #3.
What's fixed
Global flags (#3) —
--project/--json/--plain/--verboseplaced before thesubcommand were silently dropped (argparse subparser defaults clobbered the
parent namespace). Globals now work in either position.
HIGH — data loss / crash / safety
.mdcrashedload_records/validateover the whole store.init --forceranrmtreebefore the copy that could fail → store destroyed on a missing template. Now staged + atomically swapped.secret-prohibitedbypassed the leak gate (now validated); missing patterns for GitHub fine-grained PATs, Stripe keys, andsk-proj-….MEDIUM — corruption / wrong output / UX
:/newline in a value corrupted the record (colon now quoted in lists; newline rejected, not silently truncated).git statusrenameR old -> newstored as one bogus path.capture sessiondropped user-added handoff/current sections; a no-commit capture clobbered "Recently Changed" with a placeholder._norm_filesempty-basename spurious matches;_score_itemundercounted/nondeterministically cited same-basename files.initon a file path / template errors / command exceptions surfaced raw tracebacks instead of clean errors.memory_searchdroppedfiles; import guard caught onlyImportError.Deliberately NOT changed (documented in commits)
_is_map_itemquote-awareness, not just quoting.Out of scope (follow-up)
MCP structured input schemas (opaque
dicttools), the raise-vs-return errorcontract, and assorted low/nits — left for a separate PR as they're design calls.
Test plan
python -m pytest tests/→ 195 passed, 60 subtests, 0 failures.tests/test_bugfixes.py(30 tests) maps each fix to its bug id (H1–H5, M1–M11, MCP1).