Repository navigation
Conversation
github-actions
Bot
added
the
cli-interface-change
CLI interface change detectedThis PR changes dokimi's user-facing CLI surface (commands and/or options): + added command `dokimi context prefilter`
+ argument `guidance`
+ argument `sessions`
+ option `--help/-h`
+ option `--in-use-rate`
+ option `--json`
+ option `--min-sessions`
+ option `--rare-max`
+ option `--window`Updated on every push; the |
|
🔍 OpenCodeReview found 1 issue(s) in this PR. By severity: 1 medium
Review details
|
| - **Corrected.** Within `--window` events after the trigger, the user's next | ||
| prompt pushes back (`no`, `don't`, `instead`, `forgot`, `revert`, ...) and |
There was a problem hiding this comment.
Nit: corrections are not limited to the user's next prompt. _scan() checks every event in the following window with any(_corrects(...)). For example, a changelog trigger followed by “thanks” and then “no, fix the changelog” within 10 events counts as corrected despite the next prompt not pushing back. Describe “a subsequent user prompt within the window” here so readers can interpret the correction counts accurately.
Suggestion:
| - **Corrected.** Within `--window` events after the trigger, the user's next | |
| prompt pushes back (`no`, `don't`, `instead`, `forgot`, `revert`, ...) and | |
| - **Corrected.** Within `--window` events after the trigger, a subsequent user | |
| prompt pushes back (`no`, `don't`, `instead`, `forgot`, `revert`, ...) and |
| | --- | --- | --- | | ||
| | `candidate_for_ablation` | Triggered in fewer than `--in-use-rate` of the sessions, or never. Ranked lowest trigger rate first. | yes | | ||
| | `in_use` | Triggered in at least `--in-use-rate` of the sessions. | no | | ||
| | `rare_not_ranked` | Triggered in at most `--rare-max` sessions. Few sessions are too little evidence to call a rule unused. | no | |
There was a problem hiding this comment.
Must-fix: the status conditions overlap, but the implementation explicitly treats zero triggers as a candidate before checking rarity. For example, six sessions with no onboarding keywords produce candidate_for_ablation with rank 1 under the defaults (also asserted by test_rare_and_protected_rules_are_listed_but_never_ranked), although this row says zero is rare and unranked. Likewise, two triggers in six sessions remain rare_not_ranked despite exceeding the default 20% in-use threshold. Document precedence (protected → insufficient corpus → zero-trigger candidate → positive rare count → in-use threshold → candidate), and clarify that rarity means 1 through --rare-max, so readers can interpret the actual rankings.
Suggestion:
| | `rare_not_ranked` | Triggered in at most `--rare-max` sessions. Few sessions are too little evidence to call a rule unused. | no | | |
| | `rare_not_ranked` | Triggered in 1 through `--rare-max` sessions; takes precedence over the in-use threshold. Zero-trigger rules are candidates unless protected or the corpus is insufficient. | no | |
There was a problem hiding this comment.
Fixed in 6b519d2. The doc now states the precedence the code applies in status_of:
kept_protected, insufficient_corpus, zero-trigger candidate, rare_not_ranked, in_use, then candidate.
A rule that never triggers is a candidate, and --rare-max takes 0 or more.
| for session in sessions: | ||
| for problem in session.errors: | ||
| print(f"warning: {session.name}: {problem}", file=sys.stderr) | ||
| loaded.extend(sessions) |
There was a problem hiding this comment.
Must-fix: repeated inputs and overlapping globs count the same session multiple times. For example, passing s.jsonl five times satisfies the default --min-sessions 5 with only one independent session, enabling rankings and inflating episodes. Repeating a Goose store similarly duplicates all its sessions. Deduplicate expanded sources by canonical path before reading them, and add regressions for repeated names, path aliases and overlapping globs.
There was a problem hiding this comment.
| pre_p.add_argument("--rare-max", type=int, default=3, | ||
| help="A rule triggered in at most this many sessions is rare: listed, not ranked (default 3)") | ||
| pre_p.add_argument("--in-use-rate", type=float, default=0.2, |
There was a problem hiding this comment.
Must-fix: these threshold types accept invalid domains without any later validation. --rare-max -1 disables the rarity safeguard, and --in-use-rate nan makes every in-use comparison false, ranking frequently triggered rules as candidates; --json also emits a non-standard NaN value. Reject negative rarity limits and require a finite rate in [0, 1] with argparse validators. Cover negative, out-of-range, NaN and infinity inputs.
There was a problem hiding this comment.
| raise SessionError(f"{path}: not a Goose sessions store: {exc}") from exc | ||
| finally: | ||
| conn.close() | ||
| kept = [s for s in sessions if s.events] # a session of unreadable messages only is no session |
There was a problem hiding this comment.
Must-fix: dropping eventless sessions also drops their parse diagnostics. In the existing unreadable-only-session test, s1 contains content_json = 'not json', but its recorded error never reaches _read_sessions(), which only prints errors from returned sessions. With a valid s0 beside it, the command silently reports a reduced corpus; the same malformed message would produce a warning if its session also contained one valid event. Keep unreadable-only sessions out of the corpus count, but propagate their errors separately so corruption is still reported. Extend the CLI test to check stderr for this mixed-store case.
There was a problem hiding this comment.
|
|
||
| def _at_path(key: str, text: str) -> bool: | ||
| """Whether ``key`` occurs in ``text`` from a path-segment start, so ``a.md`` does not match ``data.md``.""" | ||
| name = key.removeprefix("./") |
There was a problem hiding this comment.
Must-fix: path matching does not normalize Windows separators. For guidance - Update docs/guide.md before release. and a triggered Edit event whose file_path is C:\work\docs\guide.md, this returns false and reports acted=0 even though the named file was touched. _is_path also misclassifies a backslash-only directory such as docs\ as a command. Normalize separators in both path classification and matching, and add a Windows-path fixture.
| return (any(_names(k, event.command) for k in claim.commands) | ||
| or any(_at_path(k, p) for k in claim.paths for p in event.paths)) |
There was a problem hiding this comment.
Must-fix: path-based actions performed through Bash are never counted. For guidance - Update changelog.d/ for each release., a prompt update the release followed by a Bash call with command="printf 'entry' > changelog.d/x.md" reports acted=0. Both session readers put this command in event.command but leave event.paths empty, so the named directory is ignored even though it was touched. Match path keys against command text as well as explicit path fields (using the same lexical boundary checks), and add a regression test for shell-based file changes.
There was a problem hiding this comment.
| around = events[max(0, i - window): i + window + 1] | ||
| after = events[i + 1: i + window + 1] | ||
| acted += int(claim.has_keys and any(_acts(claim, e) for e in around)) | ||
| corrected += int(any(_corrects(claim, e) for e in after)) |
There was a problem hiding this comment.
Must-fix: this checks every later prompt in the window, not the user's next prompt as specified. With the CI rule, prompts push the branch, thanks, continue, and no, the branch must not go before ci produce a correction on the first episode despite its next prompt containing no pushback. This can also inflate user_overrides. Select the first prompt in after and evaluate only that prompt; add a regression with an intervening non-corrective prompt.
|
|
||
| def _rank_key(row: dict[str, Any]) -> tuple[float, float]: | ||
| acted = row["acted_rate"] | ||
| return (row["trigger_rate"] or 0.0, 0.5 if acted is None else acted) |
There was a problem hiding this comment.
Must-fix: ranking uses rates already rounded to three decimals by _ratio, so distinct trigger counts can tie and the acted-rate tiebreaker can reverse the promised lowest-trigger-rate order. With 3,000 sessions, rare_max=0, and two candidate rules triggered in 2 and 3 sessions, both trigger rates become 0.001; if the first has acted_rate=1 and the second has acted_rate=0, the more-triggered rule ranks first. Sort by the exact sessions_triggered count (the denominator is shared), retaining rounding only for output; use unrounded acted ratios for the secondary key as well.
There was a problem hiding this comment.
| from typing import Any | ||
|
|
||
| # Input keys whose value is file content, not the agent's intent: left out of an event's text. | ||
| _CONTENT_KEYS = frozenset({"old_string", "new_string", "content", "new_source", "edits"}) |
There was a problem hiding this comment.
Must-fix: the shared body filter does not cover Goose's text-editor fields file_text, old_str, and new_str. For example, a Goose tool call with {"path":"/w/README.md","file_text":"Keep the changelog fragment current"} retains the entire file body in Event.text. _triggers() then counts that copied text as a changelog episode, potentially changing a rule from an ablation candidate to in_use even though the session only copied documentation. Exclude the Goose edit/write body fields as well and add a Goose-reader regression test containing rule keywords in those fields.
Suggestion:
| _CONTENT_KEYS = frozenset({"old_string", "new_string", "content", "new_source", "edits"}) | |
| _CONTENT_KEYS = frozenset({"old_string", "new_string", "content", "new_source", "edits", | |
| "file_text", "old_str", "new_str"}) |
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved session deduplication and rule-matching defects distort corpus counts and trigger/action rates, making rule recommendations unreliable.
8 open issues
What changed in this PR
8 new findings.
Adds deterministic context prefilter analysis to dokimi’s context tooling, using session logs to identify guidance rules worth testing through ablation—not to recommend removing them.
Changes:
- Reads Claude Code JSONL logs and Goose SQLite stores into prompt and tool events.
- Measures rule triggers, actions and corrections, then assigns statuses and ranks ablation candidates.
- Adds CLI registration, synthetic tests, documentation and updated reference metadata.
| File | Description |
|---|---|
dokimi/tests/test_context_prefilter.py |
Tests parsing, matching, ranking and CLI behavior. |
dokimi/tests/test_context_goose.py |
Tests Goose stores and mixed-format inputs. |
dokimi/tests/data/cli_flags_allowlist.txt |
Allows the three new threshold flags. |
dokimi/mkdocs.yml |
Adds the prefilter guide to navigation. |
dokimi/dokimi/context/session_log.py |
Extracts events from Claude Code logs. |
dokimi/dokimi/context/prefilter.py |
Matches guidance rules and builds reports. |
dokimi/dokimi/context/goose_log.py |
Reads Goose session stores in read-only mode. |
dokimi/dokimi/commands/context.py |
Registers and implements the new subcommand. |
dokimi/dokimi/cli_routes.py |
Registers prefilter as a free command. |
dokimi/docs/session-prefilter.md |
Documents usage, measures, statuses and limitations. |
dokimi/docs/reference/cli-interface.json |
Records the new command’s interface. |
dokimi/docs/README.md |
Links the prefilter guide. |
dokimi/docs/architecture.md |
Describes the session-analysis modules. |
dokimi/changelog.d/20261031_120000_napetrov_session-prefilter.md |
Announces the new command. |
dokimi/benchmarks/spec.v1.yaml |
Raises the document allowance for the added guide. |
Review effort: Balanced
| loaded, failed = [], False | ||
| for name in names: | ||
| matches = [name] if os.path.exists(name) else sorted(glob.glob(os.path.expanduser(name), recursive=True)) | ||
| for path in matches or [name]: | ||
| try: | ||
| source = Path(path) | ||
| sessions = read_goose_store(source) if is_goose_store(source) else [read_session(source)] |
| pre_p.add_argument("--rare-max", type=int, default=3, | ||
| help="A rule triggered in at most this many sessions is rare: listed, not ranked (default 3)") | ||
| pre_p.add_argument("--in-use-rate", type=float, default=0.2, | ||
| help="A rule triggered in at least this share of sessions is in use, not a candidate (default 0.2)") |
| raise SessionError(f"{path}: not a Goose sessions store: {exc}") from exc | ||
| finally: | ||
| conn.close() | ||
| kept = [s for s in sessions if s.events] # a session of unreadable messages only is no session |
| def _events_of(role: str, content: Any, user_visible: bool) -> list[Event]: | ||
| if not isinstance(content, list): | ||
| return [] | ||
| events = [] | ||
| for block in content: | ||
| if not isinstance(block, dict): | ||
| continue | ||
| if role == "user" and user_visible and block.get("type") == "text": | ||
| text = block.get("text") | ||
| words = prompt_text(text) if isinstance(text, str) else "" | ||
| if words: | ||
| events.append(Event("prompt", words)) | ||
| elif role == "assistant" and block.get("type") == "toolRequest": | ||
| call = block.get("toolCall") | ||
| value = call.get("value") if isinstance(call, dict) else None | ||
| if isinstance(value, dict): | ||
| events.append(tool_event(value.get("arguments"))) | ||
| return events |
| _RULE_KINDS = frozenset({"sentence", "list_item", "table_row"}) | ||
| _BACKTICK = re.compile(r"`([^`\n]+)`") | ||
| _WORD = re.compile(r"[a-z][a-z0-9_-]{2,}") | ||
| _PATH = re.compile(r"/|\.[A-Za-z0-9]{1,6}$") |
| keywords = _stems(_BACKTICK.sub(" ", span.text)) | ||
| if not keywords: | ||
| continue |
There was a problem hiding this comment.
|
|
||
| def _at_path(key: str, text: str) -> bool: | ||
| """Whether ``key`` occurs in ``text`` from a path-segment start, so ``a.md`` does not match ``data.md``.""" | ||
| name = key.removeprefix("./") |
| def _acts(claim: Claim, event: Event) -> bool: | ||
| if event.kind != "tool": | ||
| return False | ||
| return (any(_names(k, event.command) for k in claim.commands) |
There was a problem hiding this comment.
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #334 +/- ##
==========================================
+ Coverage 94.05% 94.06% +0.01%
==========================================
Files 297 300 +3
Lines 38018 38408 +390
Branches 6424 6478 +54
==========================================
+ Hits 35756 36129 +373
- Misses 1452 1462 +10
- Partials 810 817 +7
🚀 New features to boost your workflow:
|
`context prefilter GUIDANCE SESSIONS...` splits an AGENTS.md or CLAUDE.md into rules (novelty span segmentation) and reports per rule how many sessions brought up its situation (trigger), whether the agent ran the command or touched the path it names (acted), and whether the user pushed back afterwards (corrected). Deterministic, no model call. Output is a ranked list of candidates for ablation, not a verdict. Rules with protection tags, rules triggered in few sessions, and rules in use are listed but not ranked, so a rare safety rule is never ranked as waste. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…word stems A Claude Code line nested too deeply for json.loads raises RecursionError, which the reader did not catch, so one line aborted the whole session. It now counts as a line that is not JSON. The keyword stems of an event are cached, since every claim is matched against the same event texts. The design note states that the JSON report is printed, not stored, so it has no schema under dokimi/schemas/. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
… a segment start str.splitlines() breaks a JSON line at U+2028, which Claude Code can write unescaped inside a prompt, so that prompt was lost and reported as two bad lines. Lines are now split on newline only. A path named in backticks matched any path that contained it, so a rule naming a.md counted an edit of data.md as acting on it. A name now has to start at a path segment. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Adds the prefilter route to the cost table, three allowlist rows for its threshold spellings (no registry concept yet), regenerates the CLI snapshot, raises the docs budget by one for the design note, and narrows an Any-typed command read so mypy passes. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Goose keeps each session's messages in SQLite (sessions.db under the Goose data directory). Each session in the store is now read as a session: text blocks of user messages are prompts, unless their metadata marks them not user-visible, and toolRequest blocks give the tool arguments. The layout is taken from the block/goose source; the tests use synthetic stores only, and the store is opened read-only. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
It was kept and counted in the sessions denominator, lowering every trigger rate. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
`a.md` matched `a.mdx` and `a.md.bak`; a name now ends where it ends. A directory (`docs/`) still matches the entries under it. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
…s, validate thresholds - A file named twice (repeat, overlapping glob, symlink) is read once. - A Goose session of unreadable messages only is not counted, but its diagnostics still reach the command line. - The text blocks of one Goose user message are one prompt. - Goose's editor bodies (file_text, old_str, new_str) are left out of event text. - --rare-max takes 0 or more; --in-use-rate takes a number from 0 to 1. - cli-interface snapshot regenerated for the new argument types. Thread: PRRT_kwDOVBg7g86yWV7R Thread: PRRT_kwDOVBg7g86yWWKv Thread: PRRT_kwDOVBg7g86yWV7U Thread: PRRT_kwDOVBg7g86yWWK0 Thread: PRRT_kwDOVBg7g86yWV7V Thread: PRRT_kwDOVBg7g86yWWK3 Thread: PRRT_kwDOVBg7g86yWWK4 Thread: PRRT_kwDOVBg7g86yWV7b Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
- A rule with no prose keywords (only named commands, paths or a protection tag) is kept, and triggers on the event that names one of them. - Windows separators are normalized for path matching and classification; a backslash directory is a path. - A shell command that writes into a named directory counts as acting. - A command counts as run only at the start of a shell segment, after runner words (uv run, sudo, env assignments); echo, grep arguments and comments do not. - A correction is read from the user's next prompt only. - Candidates rank by exact triggered-session count, then the lowest acted share, so rounding cannot reverse the order. Thread: PRRT_kwDOVBg7g86yWV7W Thread: PRRT_kwDOVBg7g86yWWLE Thread: PRRT_kwDOVBg7g86yWV7X Thread: PRRT_kwDOVBg7g86yWWLH Thread: PRRT_kwDOVBg7g86yWWK_ Thread: PRRT_kwDOVBg7g86yWV7Y Thread: PRRT_kwDOVBg7g86yWWLQ Thread: PRRT_kwDOVBg7g86yWV7Z Thread: PRRT_kwDOVBg7g86yWV7a Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Each review finding gets a test that fails on the previous code: the keyless rules, Windows paths, shell-written directories, command invocation versus mention, the next-prompt correction, rank ordering, source de-duplication, threshold domains, Goose text blocks and diagnostics, and editor bodies. Thread: PRRT_kwDOVBg7g86yWV7R Thread: PRRT_kwDOVBg7g86yWWKv Thread: PRRT_kwDOVBg7g86yWV7U Thread: PRRT_kwDOVBg7g86yWWK0 Thread: PRRT_kwDOVBg7g86yWV7V Thread: PRRT_kwDOVBg7g86yWWK3 Thread: PRRT_kwDOVBg7g86yWWK4 Thread: PRRT_kwDOVBg7g86yWV7b Thread: PRRT_kwDOVBg7g86yWV7W Thread: PRRT_kwDOVBg7g86yWWLE Thread: PRRT_kwDOVBg7g86yWV7X Thread: PRRT_kwDOVBg7g86yWWLH Thread: PRRT_kwDOVBg7g86yWWK_ Thread: PRRT_kwDOVBg7g86yWV7Y Thread: PRRT_kwDOVBg7g86yWWLQ Thread: PRRT_kwDOVBg7g86yWV7Z Thread: PRRT_kwDOVBg7g86yWV7a Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
The next-prompt correction, the shell-segment rule for acting, keyless rules, the status precedence and the rare range now match the code. Thread: PRRT_kwDOVBg7g86yWV7C Thread: PRRT_kwDOVBg7g86yWV7N Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
… quotes and comments - A rule with only named commands or paths triggers on a prompt that names one, not on the tool call that runs it (which would make it always acted). - Command matching ignores quoted arguments and comments, splits on & and parentheses, normalises spaces, and accepts timeout N as a runner word. Thread: PRRT_kwDOVBg7g86yWWLQ Thread: PRRT_kwDOVBg7g86yWV7W Thread: PRRT_kwDOVBg7g86yWWLE Thread: PRRT_kwDOVBg7g86yWV7Y Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
f4d6a05 to
27e620e
Compare
| words = " ".join(key.split()) | ||
| if not words: | ||
| return False | ||
| run = re.compile(rf"{_RUNNER}{re.escape(words)}(?![\w-])") |
There was a problem hiding this comment.
Must-fix: the end boundary accepts punctuation that can be part of a shell token. For example, _invokes('make ci-local', 'make ci-local.debug') returns true even though make runs a different target; _invokes('pytest', 'pytest.py') likewise counts a different executable. This inflates acted counts and can change candidate ordering. Require the final matched command/argument token to end (while still allowing additional whitespace-separated arguments), and extend the longer-command regression to dotted and slash-suffixed tokens.
| if not words: | ||
| return False | ||
| run = re.compile(rf"{_RUNNER}{re.escape(words)}(?![\w-])") | ||
| bare = _COMMENT.sub("", _QUOTED.sub("''", command)) |
There was a problem hiding this comment.
Must-fix: removing every quoted token also removes real command arguments. For a claim naming pytest tests/test_x.py, _invokes(..., 'pytest "tests/test_x.py"') returns false, although this runs exactly the named command. Commands with quoted arguments in the guidance itself also cannot match. Preserve and normalize quoted argument values while distinguishing executable positions from argument mentions; add a regression covering quoted test-file arguments alongside the existing echo/grep cases.
There was a problem hiding this comment.
Fixed in a8d3b2b: quotes are removed from the command and the key, and the quoted separators and # are kept as text.
pytest "tests/test_x.py" -q now runs the named pytest tests/test_x.py (test: quoted argument of the named command).
| if event.kind != "tool": | ||
| return False | ||
| return (any(_invokes(k, event.command) for k in claim.commands) | ||
| or _path_named(claim, (*event.paths, event.command))) |
There was a problem hiding this comment.
Must-fix: searching the entire shell command counts path mentions as file activity. For - Keep changelog.d/ current., a prompt of keep the changelog current followed by Bash echo 'changelog.d/x.md' produces acted: 1, although the command never accesses that directory. Shell comments have the same problem. Both log readers preserve the raw command here, so this affects Claude Code and Goose reports and can change candidate ordering. Distinguish shell path operands/redirection targets from literal text and comments, and add negative regressions alongside the existing shell-redirection test.
…ount; printed paths do not A command that runs make ci-local.debug does not run make ci-local. A quoted argument of the named command is kept (pytest "tests/x.py"). A path that echo or printf prints, or a comment names, is not an action; a redirection target and the operands of other commands are. Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
| `` `changelog.d/` ``). A command counts when it runs the named command at the start of | ||
| a shell segment (after `uv run`, `sudo` or environment assignments), so an `echo`, a | ||
| `grep` argument or a `#` comment that names it does not. A path counts when a tool call | ||
| names it, a shell command included; a Windows path matches with backslashes too. |
There was a problem hiding this comment.
Must-fix: this definition says any shell path mention counts, but _shell_paths() excludes echo/printf arguments and shell comments. For the rule Keep changelog.d/ current, the existing test with echo 'changelog.d/x.md' expects acted == 0, despite the command naming the path. Document these exclusions so users can interpret the reported counts correctly; distinguish a printed mention from a redirection target such as printf 'entry' > changelog.d/x.md, which does count.
Suggestion:
| names it, a shell command included; a Windows path matches with backslashes too. | |
| + names it in a path input or shell command, excluding shell comments and | |
| + echo/printf arguments (their redirection targets still count). A Windows path | |
| + matches with backslashes too. |
| segments that are not printing commands. Comments are gone and printed text is left out.""" | ||
| parts: list[str] = [] | ||
| for seg in _segments(command): | ||
| parts.extend(_REDIRECT.findall(seg)) |
There was a problem hiding this comment.
Must-fix: the earlier path-mention fix remains incomplete for quoted redirection text. For - Keep changelog.d/ current., a trigger prompt keep the changelog current followed by Bash echo '> changelog.d/x.md' produces acted: 1, although it only prints text. _segments() removes the quotes before this regex runs, so the quoted > becomes indistinguishable from a real redirection. This also affects printf and can change candidate tie-breaking. Extract redirections while quote context is still available, and add a regression test alongside the existing printed-path tests (retaining the real printf 'entry' > changelog.d/x.md case).
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
Review at a8d3b2b. The earlier review rounds are fixed and CI is green, but the shell matching still counts things the agent never did, and the new tests leave the status thresholds unpinned. The inline comments are the points to fix before merge.
What I ran, on a Linux review host at a8d3b2b:
make ci-local: 30 passed, 0 failed, 7 skipped (the skips are jobs that do not run on a PR or need tools absent here).- Reproductions for the code findings, with no model call involved.
- A mutation run on the two new test files: 54 mutants, 28 pass the full suite, 5 of those equivalent.
- All 13 commits carry
Signed-off-by.
Checked, no issues
- CLI: exit 1 for a missing, unreadable, directory, corrupt or non-Goose input, exit 2 for
--window 0,--in-use-rate nanand--rare-max -1; a repeated file counts once. - Docs: flags, defaults, statuses and their precedence match
prefilter.py;cli-interface.jsonequals a fresh dump; the page is in themkdocs.ymlnav; the changelog fragment follows the format. - Tests: they write nothing outside
tmp_path.
Bot points I checked that no longer apply at this head (21)
- Copilot's and exai-bot's point on the status table does not apply: the doc now states the status precedence the code applies.
- exai-bot's point on repeated inputs does not apply: sessions are de-duplicated by real path and a repeated file counts once.
- exai-bot's point on threshold values does not apply:
--rare-maxand--in-use-rateare validated at parse time and-1andnanexit 2. - exai-bot's point on dropped Goose diagnostics does not apply: an unreadable-only session is kept out of the corpus and its diagnostics reach the CLI.
- exai-bot's point on rules with only backticked names does not apply: such rules and protected rules are kept.
- exai-bot's point on Windows separators does not apply: rule and event paths are normalized before matching.
- exai-bot's point on Bash path actions does not apply: shell redirections and operands are matched against the named path.
- exai-bot's point on corrections does not apply: only the first prompt after the trigger is read.
- exai-bot's point on rounded ranking does not apply: candidates sort on the exact triggered-session count.
- exai-bot's point on Goose editor bodies does not apply:
file_text,old_strandnew_strare excluded from event text. - Copilot's point on repeated inputs does not apply: the real-path de-duplication covers it.
- Copilot's point on threshold values does not apply: the parse-time validators cover it.
- Copilot's point on Goose diagnostics does not apply: they now reach the CLI output.
- Copilot's point on Goose text blocks does not apply: the blocks of one message are joined into one prompt event.
- Copilot's point on backslash directories does not apply: a key with a backslash is classified as a path.
- Copilot's point on command-only rules does not apply: they stay in the report.
- Copilot's point on Windows separators does not apply: paths are normalized on both sides.
- Copilot's point on echo and grep mentions does not apply: a command counts only at the start of a shell segment.
- exai-bot's point on
make ci-local.debugdoes not apply: a command key now ends at its last word. - exai-bot's point on quoted arguments does not apply:
pytest "tests/test_x.py"now matches the named command. - exai-bot's point on echoed path text does not apply: echo and printf arguments and comments are no longer path mentions.
| segments that are not printing commands. Comments are gone and printed text is left out.""" | ||
| parts: list[str] = [] | ||
| for seg in _segments(command): | ||
| parts.extend(_REDIRECT.findall(seg)) |
There was a problem hiding this comment.
A quoted > is read as a redirection, so echo '> changelog.d/x.md' counts as acting on changelog.d/.
_segments() removes the quotes at prefilter.py:147, and _REDIRECT then runs on the unquoted text. For the rule - Keep `changelog.d/` current. these calls all return True from _acts and give _shell_paths = changelog.d/x.md:
echo '> changelog.d/x.md'echo "a > changelog.d/x.md"printf '%s' '>>changelog.d/x'
A printed string inflates acted and moves the rule down the candidate ranking.
Suggestion: find redirections while the quotes are still known, for example mask > inside quotes with the other separators in _mask (_SPECIAL), and add the echoed '> path' case next to the existing printed-path tests.
| def _segments(command: str) -> list[str]: | ||
| """The command's segments, each with its quotes removed and its spacing normalized.""" | ||
| masked = _COMMENT.sub("", _QUOTED.sub(_mask, command or "")) | ||
| return [" ".join(_unquote(seg).translate(_FROM_MASK).split()) for seg in _SEGMENT.split(masked)] |
There was a problem hiding this comment.
A heredoc body is parsed as shell commands.
_SEGMENT splits on \n, so each body line becomes a segment. For the rule - Run `make ci-local` before pushing. the command cat > f <<'EOF'\nmake ci-local\nEOF makes _acts return True, though CI never ran. A body line that names a claim path counts as touching it in the same way. The session_log.py docstring keeps file bodies out of the match; a heredoc body is the same kind of text.
Suggestion: in _segments, drop the lines between a <<WORD and its terminator before splitting, and add a test with a heredoc whose body starts with the named command.
|
|
||
| def _is_path(key: str) -> bool: | ||
| """A bare file name or path; a key with a space is a command line that names a file.""" | ||
| return " " not in key and bool(_PATH.search(key)) |
There was a problem hiding this comment.
A bare file name such as Makefile is classified as a command, so edits to it never count as acting.
_PATH needs a slash or an extension. For - Edit `Makefile` carefully. the claim has commands=('Makefile',), paths=(), and _acts returns False for an Edit event with paths=('/repo/Makefile',). The same holds for Dockerfile and LICENSE. Such a rule always reports acted 0 of N and ranks as a candidate.
Suggestion: also match a command key against event.paths by file name, or treat a single-word key that equals a path's final component as a path.
| parts.extend(_REDIRECT.findall(seg)) | ||
| if not _PRINTS.match(seg): | ||
| parts.append(seg) | ||
| return " ".join(parts) |
There was a problem hiding this comment.
Quoted text in any non-print command counts as touching a path.
_shell_paths returns every segment that is not echo or printf, with its quotes removed. For - Update `docs/guide.md` when behaviour changes. these make _acts return True:
git commit -m "update docs/guide.md"gh pr create --body 'see docs/guide.md'
Neither touches the file. A commit message that names the path is common after any change, so acted rises on rules that were not followed.
Suggestion: leave quoted arguments out of the path text, as echo arguments are, and keep only unquoted operands and redirection targets.
|
|
||
|
|
||
| def test_statuses_split_in_use_rare_protected_and_candidates(): | ||
| # Triggered in 4, 2, 0 and 1 of 6 sessions; rare_max=3 makes 2 and 1 rare, and 4 is over the in-use rate. |
There was a problem hiding this comment.
No test sits on a status threshold or on the per-session count, so the verdict can flip without a failing test.
The ranking corpus triggers in 4, 2, 0 and 1 of 6 sessions at rare_max=3 and in_use_rate=0.5, never on a boundary. Mutation check, each of these passes all of test_context_prefilter.py (63 passed):
sessions_total < min_sessionsto<=(prefilter.py:258)sessions_triggered <= rare_maxto<(:262)>= in_use_rateto>(:264)out.sessions_triggered += int(episodes > 0)to+= episodes(:250)
Also surviving: reversing the acted-share tie-break, the window bounds in _scan, and the four CLI defaults.
Suggestion: add one case per boundary (exactly min_sessions sessions, exactly rare_max triggered, exactly in_use_rate * total), and a session with two episodes that asserts sessions_triggered == 1.
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
| if isinstance(rec, dict): | ||
| session.events.extend(_events_of(rec)) |
There was a problem hiding this comment.
Must-fix: diagnose invalid JSON shapes instead of silently dropping them. For example, parse_session() given a valid user prompt followed by a line containing [] returns no errors, so the CLI gives no indication that part of the input was unreadable. The same issue exists in goose_log.py: object-valued content_json is silently discarded by _events_of() rather than reported. This can hide incompatible or damaged logs while producing rankings from incomplete evidence. Validate the top-level record/content shapes, record a line/message diagnostic through add_error(), and add mixed-valid/invalid-shape tests for both readers. Continue skipping recognized non-event records such as summaries and tool results.
antonwolfy
left a comment
antonwolfy
left a comment
There was a problem hiding this comment.
I pushed a commit per nit, up to b024307, which fix the two nits from my review:
7af97be:dokimi/docs/session-prefilter.md:43: the path rule now says shell comments and echo/printf arguments do not count, while their redirection targets still do.b024307:dokimi/dokimi/commands/context.py:166: the command prints a warning to stderr when the guidance file yields no rules.
make ci-local at b024307: 29 passed, 1 failed (host timing flake, passes alone), 7 skipped.
Not approving yet. Still open: the three bugs and the two risks from my review.
Summary
context prefilterreads an agent's session logs and reports, per rule of an AGENTS.md or CLAUDE.md, whether the situation the rule describes came up, whether the agent acted on it, and whether the user pushed back. The output is a ranked list of candidates for ablation, not a verdict. Rare and protected rules are listed, never ranked. Output never says a rule should be cut.Statuses:
candidate_for_ablation(ranked),in_use,rare_not_ranked,kept_protected(negation, version gate, ordering or threshold tag),insufficient_corpus.Session logs are read as follows:
~/.claude/projects/<project>/. Injected records (isMeta), subagent briefs (isSidechain) and tool results are not prompts.sessions.db(~/.local/share/goose/sessions/on Linux), opened read-only. Layout taken from block/goose source (session_manager.rs,message.rs,tool_result_serde.rs):messages.session_id,content_jsonas a list of blocks,toolRequest.toolCall.value.arguments, andmetadata_json.userVisible. Related: rp-why: Configuration Effectiveness (CE) - Phase 1 block/agent-skills#52 (rp-why CE reads Goose logs). That PR queriesrun_idand readstoolCall.name; the source sayssession_idandtoolCall.value.name, and this reader follows the source.Output: a table on stdout, or
--jsonon stdout. The JSON is printed, not stored, so no schema underdokimi/schemas/;format_versionmarks its shape. Design note:docs/session-prefilter.md. Related: agentsmd/agents.md#213.Other changes that come with the command:
spec.v1.yaml: golden manifestmax_docs37 → 38 (one more doc).ce-replaymakes the same bump; whichever merges second rebases.cli_routes.py,cli_flags_allowlist.txt(--min-sessions,--rare-max,--in-use-rate: no registry concept yet), regenerateddocs/reference/cli-interface.json.Evidence
dokimi context prefilterunknown; Goose stores not readable by any command.test_context_prefilter,test_context_goose,test_cli_flags,test_cli_routes,test_docs_code_refs,test_docs_links,test_import_architecture) → 169 passed onf4d6a05b. Tests use synthetic JSONL and synthetic SQLite stores only; no real transcript or real Goose store was read.make ci-localon26ad2407(run before the last commit): 29 pass, 7 skip, oneFAILrow,ci.yml:test / Run tests. The failures are unrelated timing-sensitive tests, run on a host under load ~300 (shared with other sessions' benchmarks):test_repo_environment.py::test_unbalanced_quote_in_hook_completes_quickly,test_repo_environment.py::test_unclosed_heredocs_are_linear,test_reference_checks.py::test_the_regex_limit_is_shared_by_every_pattern_of_one_check,test_reference_checks.py::test_a_regex_that_backtracks_past_the_timeout_is_unscored[True],test_audit_test_quality.py::test_no_new_test_that_cannot_fail. Across three full runs a different subset failed each time. Each of the five passes when run alone onf4d6a05b. The full ci-local was not re-run onf4d6a05b. GitHub CI is the check of record.Merge Danger
Door: two-way. A new read-only subcommand. It writes no file, stores no output and changes no existing command. The Goose store is opened
mode=ro.Blast Radius: low. The new subcommand, and the golden manifest and CLI snapshot regenerated with it.
Known limits and follow-ups (not in this PR):
26ad2407: one non-blocking finding fixed (an unreadable-only Goose session was counted in the session total). Pass 2 on the last commit: a file name no longer matches a longer name or its backup (a.mdvsa.mdx,a.md.bak), with tests. No blocking finding open.🤖 Generated with Claude Code