Suppress noise in system-info messages (#129)#133
Merged
Conversation
Two related sites previously surfaced raw command-tag XML soup as
visible content:
1. ``SystemMessage`` rendering for ``subtype="local_command"`` system
entries. The harness writes typed slash commands as
<command-name>/status</command-name>
<command-message>status</command-message>
<command-args></command-args>
and dialog-dismissal hints as
``<local-command-stdout>Status dialog dismissed</local-command-stdout>``.
Both rendered verbatim through ``format_system_content`` /
``format_SystemMessage``, so a session that ran ``/status`` showed
a system-info card with the literal XML rather than a clean
``ℹ️ /status``.
2. Branch headers whose first user entry is itself a ``/cmd`` slash
command. ``_render_messages`` extracts text from the first user
entry via ``extract_text_content`` and feeds it to
``create_session_preview``; for slash-command branches that yielded
``Branch • 8d0ae973 • <command-name>/exit</command-name>
<command-message>exit</command-me…`` instead of the cleaner
``Branch • 8d0ae973 • /exit``.
The fix is a single shared helper and three thin call-sites:
- ``factories/user_factory.py::simplify_command_tags(text)`` — strips
``<command-name>X</command-name>`` (with optional ``<command-args>Y``)
to ``"/X"`` / ``"/X Y"``, ``<local-command-stdout>X</...>`` and
``<local-command-stderr>X</...>`` to inner ``X``, otherwise
passthrough. Safe to apply opportunistically — the no-match path
returns the input unchanged.
- ``factories/system_factory.py::create_system_message`` calls it for
``subtype == "local_command"`` only. Other subtypes
(``compact_boundary`` plain text, ``stop_hook_summary``,
``turn_duration``, …) are untouched.
- ``utils.py::create_session_preview`` calls it on the way to the
truncation step. Replaces the older ``extract_init_command_description``
(a one-shape, ``<command-name>init`` + ``<command-contents>``-only
helper that returned a hardcoded English description and was
asserted by zero tests). The general helper collapses ``/init`` to
``/init`` instead of the previous English string — same shape every
other slash command gets, more consistent and shorter.
Tests in ``test/test_utils.py``:
- ``TestSimplifyCommandTags`` — six direct unit tests on the helper:
the three patterns, passthrough on no-match, command-name winning
over a coexisting stdout payload.
- ``TestSystemMessageLocalCommandCleanup`` — three integration tests
on ``create_system_message``: slash command, dialog stdout,
``compact_boundary`` passthrough.
- Four new ``TestCreateSessionPreview`` cases for the slash-command
/ stdout / init paths through the preview pipeline.
Real-data verification on the BCT/clmail/monk teammates session:
Branch • 1addcb81 • <command-name>/clmail:list</command-name> ... → Branch • 1addcb81 • /clmail:list /quit
Branch • 7a3bee4e • <command-name>/exit</command-name> ... → Branch • 7a3bee4e • /exit
ℹ️ <command-name>/status</command-name>... → ℹ️ /status
ℹ️ <local-command-stdout>Status dialog dismissed</...> → ℹ️ Status dialog dismissed
``just ci`` clean (1218 tests, ruff, pyright, ty all green).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a new simplify_command_tags helper to factories, uses it to normalize XML-like command tags into leading-slash commands (e.g., ChangesCommand-tag normalization and wiring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@claude_code_log/factories/user_factory.py`:
- Around line 115-118: The code that builds the command string using name_match
and args_match currently returns bare command names (cmd) without a leading
slash; update the return logic so cmd is normalized to start with a single "/"
if it doesn't already (e.g., ensure cmd = cmd if cmd.startswith("/") else
f"/{cmd}"), then combine with args as before (using _COMMAND_ARGS_RE,
args_match, and the existing f-string) so you preserve arguments and avoid
double slashes.
In `@test/test_utils.py`:
- Around line 637-655: The test
test_create_session_preview_init_command_yields_slash_init is inconsistent: its
name and docstring expect a "/init" output but the assertion checks for "init".
Fix by making the expected value unambiguous — either change the assertion to
expect "/init" (if create_session_preview should return the slash-prefixed
command) or rename the test/docstring to reflect plain "init" if that is the
intended contract; locate the behavior in create_session_preview and related
cleanup logic (simplify_command_tags / extract_init_command_description) and
ensure the test and docstring match the actual output.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9b78038d-993d-4091-806f-26a25a474dcd
📒 Files selected for processing (5)
claude_code_log/factories/__init__.pyclaude_code_log/factories/system_factory.pyclaude_code_log/factories/user_factory.pyclaude_code_log/utils.pytest/test_utils.py
Two related fixups to the command-tag cleanup landed in the previous
commit:
1. Slash normalization
`simplify_command_tags` and `create_slash_command_message` previously
passed the inner text of `<command-name>X</command-name>` through
verbatim. Modern harness emissions (2025+) carry the leading slash
(`/exit`); legacy / synthetic fixtures don't (`init`, `test-command`).
Mixed transcripts rendered `/exit` next to `test-command` in TOC,
branch headers, system cards and slash-command bodies.
Both functions now route the captured name through `_normalize_slash`
so the `/cmd` shape is the single source of truth handed to display
layers. The Markdown title formatter's "command_name already includes
the leading slash" comment now holds for every input.
Normalization happens at the factory layer (where typed content is
created from raw text), not at the parser layer. Reasons:
- The factory is the natural normalization seam between raw JSONL
and typed content models. Every output renderer (HTML, Markdown,
JSON) inherits the unified shape for free.
- Raw `TranscriptEntry` fidelity is preserved for callers that need
to round-trip the original JSONL.
2. Robust command-args capture
`_COMMAND_ARGS_RE` was `<command-args>([^<]*)</command-args>`. When
args contained `<` (e.g. `/explain <Component>`), the regex matched
up to but not including the first `<`, then failed to find the
closing tag — and the whole match failed, dropping the args
silently. Tightened to `(.*?)` with `re.DOTALL`, which is robust to
`<` in the payload and to multi-line args. The duplicate inline
regex inside `create_slash_command_message` is consolidated onto the
same module-level constant.
Test coverage
- `TestSimplifyCommandTags`: bare-name normalization, idempotence on
already-slashed input, real-world `/init` with English args, args
containing `<`, multi-line args, special chars (quotes, backticks,
ampersand, `>`).
- `TestCreateSlashCommandMessage`: same contract on the structured
factory path — `command_name` normalized, `command_args` preserved
with `<` in payload.
- `TestSystemMessageLocalCommandCleanup`: bare name in a SystemMessage
comes out normalized.
- `test_create_session_preview_init_command_yields_slash_init`: name
and assertion now agree (yields `/init`); the test was previously
self-inconsistent (name promised `/init`, assertion read `init`).
- `test_command_handling.py`: assertion updated to reflect the
normalized `<code>/init</code>` HTML output.
- Snapshot updates: `test_edge_cases_html` / `_markdown` now show
`/test-command` where the legacy bare form previously appeared.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
claude_code_log/factories/user_factory.py (1)
224-232: 💤 Low valueReuse
_LOCAL_STDOUT_REto remove regex duplication.
create_command_output_messagere-declares the same<local-command-stdout>(.*?)</local-command-stdout>pattern that's now compiled at module scope as_LOCAL_STDOUT_RE(Line 91-93). Reusing the constant avoids drift if the tag shape ever needs adjustment.♻️ Proposed refactor
- stdout_match = re.search( - r"<local-command-stdout>(.*?)</local-command-stdout>", - text, - re.DOTALL, - ) + stdout_match = _LOCAL_STDOUT_RE.search(text)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@claude_code_log/factories/user_factory.py` around lines 224 - 232, In create_command_output_message replace the inline re.search call using the literal "<local-command-stdout>... </local-command-stdout>" with the module-level compiled regex _LOCAL_STDOUT_RE (used to find stdout_match) to avoid duplication; keep the same logic (if not stdout_match: return None, then stdout_content = stdout_match.group(1).strip()) but obtain stdout_match by calling _LOCAL_STDOUT_RE.search(text) instead of re.search with a new pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@claude_code_log/factories/user_factory.py`:
- Around line 224-232: In create_command_output_message replace the inline
re.search call using the literal "<local-command-stdout>...
</local-command-stdout>" with the module-level compiled regex _LOCAL_STDOUT_RE
(used to find stdout_match) to avoid duplication; keep the same logic (if not
stdout_match: return None, then stdout_content = stdout_match.group(1).strip())
but obtain stdout_match by calling _LOCAL_STDOUT_RE.search(text) instead of
re.search with a new pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dbeee568-a917-4531-82ac-4ca1ef407d18
📒 Files selected for processing (8)
claude_code_log/factories/user_factory.pyclaude_code_log/utils.pytest/__snapshots__/test_snapshot_html.ambrtest/__snapshots__/test_snapshot_markdown.ambrtest/test_command_handling.pytest/test_slash_command_pairing.pytest/test_template_utils.pytest/test_utils.py
✅ Files skipped from review due to trivial changes (1)
- test/snapshots/test_snapshot_html.ambr
…rkdown
The Markdown formatter for SlashCommandMessage wraps `command_args` in
a single-tick CommonMark inline code span. CommonMark terminates a
code span at the first matching tick of the same length, so a single
backtick inside `command_args` would close the span at the inner tick
and leave the rest of the args rendering as plain text mixed with
unmatched ticks.
The previous commit (`_COMMAND_ARGS_RE: [^<]* → (.*?)` with DOTALL)
makes this practically reachable. Args containing `<` were silently
dropped by the old regex, and that class strongly correlates with
shell-ish payloads carrying backticks (e.g. `run `date` && diff a > b`).
Now those args round-trip through the parser, so the pre-existing
inline-span bug is reachable on the same commit that exposes it.
The module already has `_inline_code` (used by `_teammate_marker`),
which widens the fence past the longest backtick run in the value and
pads with a leading/trailing space when the value starts/ends with a
backtick. Three call sites switch over:
- `format_SlashCommandMessage` — `**Args:** {_inline_code(args)}`.
The actually-reachable site.
- `title_SlashCommandMessage` — `🤷 Command {_inline_code(name)}`.
Functionally safe today (harness emits short identifiers, no
backticks ever) but use the helper for symmetry and to stay safe
if the emission shape ever drifts.
- `title_UserSlashCommandMessage` — same, in the borrowed-title path
for paired slash commands.
`_inline_code` is a strict superset of the previous f-string for
non-backtick payloads (same `` `text` `` output), so existing snapshot
tests stay green.
Regression test in `test_slash_command_pairing.py::TestMarkdownRender`
loads a JSONL fixture whose `command_args` contains backticks
(`echo `date` && diff a > b`) and asserts the rendered Markdown wraps
the payload verbatim inside a widened fence (`` `` … `` ``). Without
the fix, the payload would fragment on the inner backtick.
cboos
added a commit
that referenced
this pull request
May 6, 2026
…SR1), #133 (factory-layer normalisation seam) Three PRs landed on main since this branch was created. The dev-docs deltas they imply: - **PR #36 (JSON renderer)** — - `application_model.md` § 1 subsystem table gains a JSON export row; new § 2.5 "JSON export" describes the renderer (mirrors HTML/Markdown surface, runs through `generate_template_messages` so it inherits all post-factory polishing for free, JSON-specific `_json_default` shim for embedded Pydantic models, output naming via `variant_suffix`). - § 2.1 CLI lists `json` alongside `html`/`md`/`markdown` for `--format`. - § 3 data lifecycle diagram adds `json/renderer.py` as a third fan-out branch alongside HTML/Markdown. - § 5 entry-questions gains a "How do I export to JSON?" pointer. - `rendering-architecture.md` § 1 / § 4 list JSON; new `JsonRenderer` subsection in § 7 describes its serialisation approach (single document via `dataclasses.asdict` + shim; only reuses `title_content` from the dispatcher). - § 10 Separation of Concerns clarifies that **factories/** is the normalisation seam — display polish for *all* output formats lives there, not in renderers. - `implementing-a-tool-renderer.md` Overview + Tests sections note that JSON needs no per-tool integration: serialises the typed factory output directly. - **PR #135 (DAG cycle-break + SIGUSR1)** — - `dag.md` Phase 2 (Build DAG) gains a mermaid diagram of the three-step `build_dag` ordering (orphan promotion → cycle break → children build) and explains the ordering rationale (children must be built last because cyclic edges in `parent_uuid` would propagate into `children_uuids` otherwise, hanging downstream walks). - Phase 3 documents the `walk_visited` defence-in-depth in `_walk_session_with_forks` — backstop for reintroduced cycles after build time. - Invariant 3 (DAG acyclicity) updated from "no cycles" assertion to "actively broken at build time, walker has belt". - `application_model.md` new § 2.9 "Diagnosing hangs" describes the SIGUSR1 stack-dump handler installed in `cli.py`. - **PR #133 (system-info-cleanup)** — - Already mostly absorbed via the branch-heading navigation gap commit; the renormalisation ("if it shows up right in HTML, it shows up right in JSON") and the explicit "factory-layer normalisation seam" framing in `rendering-architecture.md` § 10 cite `simplify_command_tags` as the canonical example. Also drive-by: § 2.6 detail-level table now includes the `high` level (was missing — `models.DetailLevel.HIGH` predates this branch but the doc never listed it). `just ci` clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.
Closes #129.
Summary
Two visible artifacts of raw command-tag XML soup get cleaned up under one shared helper:
System-info cards for
subtype="local_command"entries. The harness emits typed slash commands asand dialog-dismissal hints as
<local-command-stdout>Status dialog dismissed</local-command-stdout>. Both rendered verbatim before — nowℹ️ /status/ℹ️ Status dialog dismissed.Branch header previews when the branch's first user entry is itself a
/cmdslash command. The same XML soup propagated into the body branch header, the session/graph index nav, and the fork-point box's per-branch link viaSessionHeaderMessage.preview(introduced in Robust within-session fork rendering: collapse parallel-tool_use forks, consistent labels #131). They surfaced asNow:
Branch • 8d0ae973 • /exit.How
One shared helper + three thin call-sites:
factories/user_factory.py::simplify_command_tags(text)— strips the three known shapes (<command-name>,<local-command-stdout>,<local-command-stderr>), passthrough on no-match. Co-located with the existingcreate_slash_command_messageetc. that already parse the same patterns into typed shapes.factories/system_factory.py::create_system_messagecalls it forsubtype == "local_command"only — gate is intent-documenting, not safety-providing (the helper is opportunistic by design).utils.py::create_session_previewcalls it on the way to the truncation step. Replaces the olderextract_init_command_description(a one-shape<command-name>init+<command-contents>-only helper that returned a hardcoded English string and was asserted by zero tests). The general helper collapses/initto/init— same shape every other slash command gets, more consistent and shorter. The init-only helper was effectively dead code.Test plan
TestSimplifyCommandTags— 6 direct unit tests (the three patterns, args, passthrough, command-name precedence over a coexisting stdout payload).TestSystemMessageLocalCommandCleanup— 3 integration tests oncreate_system_message(slash command, dialog stdout,compact_boundarypassthrough).TestCreateSessionPreviewcases for the slash-command / stdout / init paths.just ciclean (1218 tests, ruff, pyright, ty).Branch • 1addcb81 • /clmail:list /quit,Branch • 7a3bee4e • /exit, system-info cards now showℹ️ /statusandℹ️ Status dialog dismissed.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests