Support teammates (#91): parsing + data model (draft)#117
Merged
Conversation
Spec anchor for the multi-PR work implementing Claude Code's teammates feature. Captures: - 3-PR staging (parsing+model, rendering+colors, polish+stitching) - main's answers on: prompt-hash matching (approved), folding #79's DAG re-plumbing into this work (agentId is parseable from tool_result Markdown tails), color palette (named CSS custom properties for future /color reuse), whole-transaction visibility (stitch sidechain under Agent tool_use collapsed by default) - The concrete DAG shape with UUIDs from the real-data screenshot in #2197 - Concrete commit-by-commit plan for PR 1 Lets the work resume cleanly from a fresh session. Acts as a spec anchor for monk's review of the subsequent PRs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds support for "teammates": new models and factories for teammate tools and Changes
Sequence Diagram(s)sequenceDiagram
participant Conv as Converter (load_transcript)
participant Main as Main Transcript (Task tool_results)
participant Sub as Subagents (agent-*.jsonl)
participant Matcher as Prompt Matcher
Conv->>Main: collect unresolved Task tool_results (missing agentId)
Conv->>Sub: discover subagent files
Sub->>Sub: read first message text from each agent file
Conv->>Matcher: normalize task prompts and subagent first messages
Matcher->>Sub: compare normalized prompts -> find match
alt match found
Sub->>Conv: return matching agentId
Conv->>Main: back‑patch result_entry.agentId
Conv->>Conv: add agentId to agent_ids for loading
else no match
Matcher->>Conv: no match (leave unresolved)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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)
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 |
Additive model layer for the experimental teammates feature (#91): - TeammateMessage content class for <teammate-message> blocks - AgentResultMetadata for the markdown tail of Task tool_results (agentId, worktreePath, worktreeBranch, <usage> fields) - Input/output models for six new tools: TeamCreate, TeamDelete, TaskCreate, TaskUpdate, TaskList, SendMessage - Extend TaskInput with team_name/name/mode, TaskOutput with metadata / teammate_id / agent_id / color - BaseTranscriptEntry.teamName and MessageMeta.team_name - Propagate teamName through create_meta No parsers or rendering yet — those ride in subsequent commits.
Synthetic main session + two subagent JSONLs exercising every shape this PR needs to parse (6 new tools, teammate-message blocks, Task metadata tail, agentId linking via structured toolUseResult vs. via prompt-hash). Under the current (unmodified) code the fixture loads without validation errors; the teammate-specific content falls back to generic rendering until the factories land. - main (22 entries): TeamCreate + 2 TaskCreate + TaskUpdate + 2 Task spawns + 2 user entries with <teammate-message> blocks + TaskList + SendMessage + TeamDelete + final summary. - alice subagent (3 entries): plain-text first message — the existing linking picks it up via toolUseResult.agentId. - bob subagent (3 entries): first message wrapped in <teammate-message teammate_id="team-lead"> — the prompt-hash fallback in commit 7 will link this one. See test/test_data/teammates/README.md for context.
Task tool_results spawned for a teammate or async agent carry a trailing metadata block (agentId / worktreePath / worktreeBranch / <usage>). New agent_metadata_factory.parse_agent_result_metadata() extracts these fields into AgentResultMetadata and strips the tail so the rendered body shows only the agent's actual response. - Wire the parser into parse_task_output so TaskOutput.metadata is populated whenever the tail is present. - Re-export parse_agent_result_metadata from the factories package. - New test suite test_teammates_parsing.py covers the recognized shapes: plain text, agentId-only, agentId + SendMessage hint, full worktree+usage block, <usage> alone, idempotent strip.
A User entry emitted during a teammates session carries one or more `<teammate-message teammate_id="..." color="..." summary="...">body</teammate-message>` blocks inside its string content. The new teammate_factory extracts each block into a TeammateMessageBlock (attributes + body, plus is_system=True for teammate_id="system") and wraps the set in a TeammateMessage content model. Surrounding non-block text is preserved as leading_text / trailing_text so the renderer can keep context if present. - Detection hooks into create_user_message before the default text path so teammate content short-circuits the generic UserTextMessage path. - find_team_lead_body() exposes the body of the first `<teammate-message teammate_id="team-lead">` block, ready for the subagent-linking prompt-hash fallback in commit 7. - Test coverage: detection, attribute extraction, ordering, summary-less blocks, system flag, mixed intermingled teammates, surrounding text, team-lead lookup.
Wire typed parsers for the six teammate-management tools so their
invocations and results land in the renderer as structured data.
Inputs — registered in TOOL_INPUT_MODELS so create_tool_input()
returns the right Pydantic model for each:
TeamCreate, TeamDelete, TaskCreate, TaskUpdate, TaskList, SendMessage.
Outputs — TOOL_OUTPUT_PARSERS entries:
- TeamCreate, SendMessage: parse JSON text into typed outputs.
- TeamDelete: parse JSON and pull the active-members list out of
the "Cannot cleanup team with N active member(s): …" message so
the rendering layer can show them cleanly.
- TaskCreate: regex the "Task #N created successfully: …" line.
- TaskUpdate: regex the "Updated task #N field1, field2" line,
producing updated_fields as {name: True}.
- TaskList: parse the "#N [status] subject (owner)" rows into
TaskListItem entries; unknown formats fall through so the
generic renderer keeps the original text.
Test suite extended with input-routing and output-shape coverage
for every parser, including negative cases (non-JSON, unknown
line formats) so the parsers never silently mangle unexpected
data.
Teammate-spawned Tasks don't always expose a structured agentId on the tool_result — the linking info may live only in the markdown metadata tail or be absent entirely (also issue #79 / #90). Add a fallback pass in load_transcript that, after primary linking, scans the subagents/ directory for agent-*.jsonl files whose id isn't yet known and matches each candidate's first-entry content against any unresolved Task tool_use's prompt input. The candidate comparison prefers the body of a <teammate-message teammate_id="team-lead"> wrapper when present (the real teammate spawn shape), falling back to raw first-message text. Comparison is whitespace-collapsed + lowercase so benign reformatting doesn't break the match. On match the agentId is added to the in-flight agent_ids set and back-patched onto the corresponding tool_result UserTranscriptEntry so the existing _integrate_agent_entries anchors the subagent's DAG-line to the right Task tool_use. The primary linking path is untouched.
Load the teammates fixture through the full pipeline (load_transcript + load_directory_transcripts + _integrate_agent_entries) and assert: - Both subagent files load: alice via the existing toolUseResult.agentId primary path, bob via the new prompt-hash fallback. - The bob tool_result UserTranscriptEntry gets its agentId field back-patched by the fallback. - Sidechain entries receive the expected synthetic session IDs "<main>#agent-<agentId>", one per agent. - Sidechain roots (parentUuid originally None) now point to the main-session tool_result that anchors each agent. - TaskOutput.metadata carries parsed agentId/usage fields for the alice Task (and the tail is stripped from the rendered body). - Both string-content user entries parse into TeammateMessage with the expected blocks, including the system termination flag. Also: move module-level imports to the top of the file (ruff E402), drop an unused ToolResultContent import, and satisfy pyright on the new _read_first_message_text JSON walker and _try_load_json_text.
- TeammateMessage.blocks default factory uses a typed lambda so ty can infer list[TeammateMessageBlock] instead of list[Unknown]. - One test-only tweak: wrap iter_teammate_blocks() in iter() before passing to next() — ty rejects Iterable as a SupportsNext. No behavior change; `just ci` is green end-to-end.
## Bug fix: identical-prompt Tasks no longer collide _link_subagents_by_prompt_hash's inner match loop didn't exclude already-patched entries. With two Tasks carrying the same normalized prompt and two unlinked subagent files whose first- message bodies both normalize to that prompt, the second agent overwrote the first agent's patch on the same tool_result entry. Net effect in real use: a team-lead dispatching identical instructions to parallel teammates would lose one of them to an orphan sidechain. Fix: pre-normalize prompts once, track unresolved entries in a `remaining` list, and pop matched entries so they can't be reclaimed. O(n·m) worst case (same as before), no added allocations on the happy path. ## Non-blocker: _TEAM_DELETE_ACTIVE_RE robustness Anchor the non-greedy member-list capture on `(?:\.|$)` instead of strictly `.`, so messages that happen to drop the trailing period still extract the active members list. ## Tests - test_identical_prompts_do_not_collide: two Task tool_uses with identical prompts + two subagent files, asserting both get distinct agentIds on their respective tool_results. Fails on the previous code, passes after the fix. - test_teamdelete_active_members_without_trailing_period: locks in the regex change. Review: monk flagged both in the PR #117 pass — see thread 2243.
Collaborator
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
claude_code_log/factories/teammate_factory.py (1)
116-128: Two passes over the block regex — fine, but easy to collapse.
_split_surrounding_textruns_BLOCK_RE.searchonce and then_BLOCK_RE.finditera second time just to find the last match'send(). For the expected block counts (typically ≤ a handful per user entry) this is irrelevant, but you can combine into a singlefinditerpass if you touch this again.Single-pass variant
def _split_surrounding_text(text: str) -> tuple[Optional[str], Optional[str]]: """Return any non-block text before the first / after the last block.""" - first = _BLOCK_RE.search(text) - if first is None: - return None, None - leading = text[: first.start()].strip() - - last_match = None - for match in _BLOCK_RE.finditer(text): - last_match = match - assert last_match is not None # _BLOCK_RE matched above at least once - trailing = text[last_match.end() :].strip() - + matches = list(_BLOCK_RE.finditer(text)) + if not matches: + return None, None + leading = text[: matches[0].start()].strip() + trailing = text[matches[-1].end() :].strip() return (leading or None, trailing or None)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@claude_code_log/factories/teammate_factory.py` around lines 116 - 128, The function _split_surrounding_text currently calls _BLOCK_RE.search and then _BLOCK_RE.finditer to locate the first and last block; replace this two-pass approach with a single-pass over _BLOCK_RE.finditer inside _split_surrounding_text by iterating once and recording the first match (if not set) and the last match (update each iteration), then compute leading and trailing from first.start() and last.end(); keep the existing trimming and the assert removal since the first-match check will be handled by testing whether any match was seen.claude_code_log/converter.py (1)
475-535: Fallback logic looks right; one subtle tie-breaking behavior worth documenting.The
remaining.pop(i)approach cleanly prevents the "second agent overwrites the first patch" bug on identical prompts, and thesorted(subagents_dir.glob(...))makes the candidate iteration deterministic. What it does not do is deterministically pair a specific remaining task with a specific agent file when multiple remaining entries normalize identically and multiple candidate files also normalize to the same prompt — the pairing then depends on the filesystem-sorted agent-file order plus the main-transcript order of theTasktool_uses. For the current use case (teammates with distinct prompts, or identical prompts where any surviving pairing is acceptable) this is fine, but it's worth a one-liner in the docstring so the next person who reads this doesn't assume timestamp-based pairing. Also,_normalize_promptapplies.lower(), so two prompts differing only in case would collide — not a concern for narrative teammate prompts, but worth being aware of if prompts ever carry identifiers/paths.Docstring clarification
@@ On a match, the agent id is added to *agent_ids* (so the existing loader picks the file up) and the corresponding tool_result entry's ``agentId`` field is back-patched (so ``_integrate_agent_entries`` anchors the subagent DAG-line to the right place). + + When multiple unresolved tasks share a normalized prompt *and* multiple + candidate subagent files also share that normalized body, pairing is + deterministic (sorted agent-file order × main-transcript task order) but + not semantically meaningful — any surviving 1:1 pairing is accepted. No-op when the subagents dir doesn't exist or every Task is already linked; safe to call unconditionally.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@claude_code_log/converter.py` around lines 475 - 535, Add a brief docstring note to _link_subagents_by_prompt_hash explaining that when multiple unresolved Task prompts normalize identically, pairing between remaining entries and candidate agent files is deterministic only per filesystem-sorted agent-file order plus the main transcript order (due to sorted(subagents_dir.glob(...)) and remaining.pop(i)), so exact task↔agent assignment may not be stable across different file orderings; also mention that _normalize_prompt lowercases inputs so prompts differing only by case will collide. Reference symbols: _link_subagents_by_prompt_hash, remaining.pop(i), sorted(subagents_dir.glob(...)), and _normalize_prompt.
🤖 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/agent_metadata_factory.py`:
- Around line 27-62: The parser currently strips from the first _AGENT_ID_RE or
_USAGE_RE anywhere in the text and uses _WORKTREE_PATH_RE with \S+ which breaks
paths with spaces; change parse_agent_result_metadata to locate the metadata
only if it appears as a contiguous metadata block at the very end of the text
(scan for the last occurrence of agentId or usage via finditer, then verify the
remainder of the string consists solely of allowed metadata lines before
treating it as the tail), and update _WORKTREE_PATH_RE to capture the full path
(e.g. use a pattern that allows spaces like r"^\s*worktreePath:\s*(.+)" with
multiline). Ensure you still return (body, metadata) and only strip when the
tail is validated as metadata.
In `@claude_code_log/factories/tool_factory.py`:
- Around line 752-879: The parsers (e.g., parse_teamdelete_output,
parse_sendmessage_output — and any other JSON-returning parsers) currently
coerce the "success" field with bool(data.get("success", False)), which treats
non-boolean strings like "false" as True; instead validate that
data.get("success") is actually a bool and reject/unrecognize the payload if
it's not. Update each parser to read success = data.get("success"), check
isinstance(success, bool) (if missing or not a bool, return None), and then pass
that boolean into the respective output constructors (e.g., TeamDeleteOutput,
SendMessageOutput); keep all other parsing logic the same.
In `@claude_code_log/factories/user_factory.py`:
- Around line 423-425: The new TeammateMessage type emitted by
create_teammate_message must be explicitly wired into rendering: add
TeammateMessage to the imports in claude_code_log/html/utils.py and register its
CSS class in CSS_CLASS_REGISTRY (use key "teammate" -> desired CSS class), add
TeammateMessage to imports in claude_code_log/renderer.py and implement a
title_TeammateMessage(self, msg: TeammateMessage) method that returns the
display title (e.g., "Teammate") and any tooltip handling, and update
claude_code_log/html/templates/components/timeline.html to add a "teammate"
entry in messageTypeGroups with the chosen emoji, group name, and CSS styling so
timeline grouping, emoji via get_message_emoji, and CSS class resolution behave
the same as other message types.
In `@test/test_data/teammates/README.md`:
- Around line 9-15: The fenced code block showing the directory layout in
README.md is missing a language specifier which triggers markdownlint; update
the triple-backtick fence that wraps the directory listing (the block containing
ef000000-0000-4000-8000-000000000001.jsonl and the subagents/ entries) to
include the language tag text (i.e., change ``` to ```text) so the linter stops
flagging it.
In `@work/teammates-plan.md`:
- Around line 46-53: Update the fenced code block that starts with the line
containing "agentId: a4ca7529859c158c2 (use SendMessage with to: '...' to
continue this agent)" by adding a language hint to the opening fence (for
example change ``` to ```text) so the block is annotated (e.g., ```text) and
will satisfy MD040 and render consistently.
---
Nitpick comments:
In `@claude_code_log/converter.py`:
- Around line 475-535: Add a brief docstring note to
_link_subagents_by_prompt_hash explaining that when multiple unresolved Task
prompts normalize identically, pairing between remaining entries and candidate
agent files is deterministic only per filesystem-sorted agent-file order plus
the main transcript order (due to sorted(subagents_dir.glob(...)) and
remaining.pop(i)), so exact task↔agent assignment may not be stable across
different file orderings; also mention that _normalize_prompt lowercases inputs
so prompts differing only by case will collide. Reference symbols:
_link_subagents_by_prompt_hash, remaining.pop(i),
sorted(subagents_dir.glob(...)), and _normalize_prompt.
In `@claude_code_log/factories/teammate_factory.py`:
- Around line 116-128: The function _split_surrounding_text currently calls
_BLOCK_RE.search and then _BLOCK_RE.finditer to locate the first and last block;
replace this two-pass approach with a single-pass over _BLOCK_RE.finditer inside
_split_surrounding_text by iterating once and recording the first match (if not
set) and the last match (update each iteration), then compute leading and
trailing from first.start() and last.end(); keep the existing trimming and the
assert removal since the first-match check will be handled by testing whether
any match was seen.
🪄 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: 4d3e687a-ea18-426c-8cee-23fd78b8bf6c
📒 Files selected for processing (14)
claude_code_log/converter.pyclaude_code_log/factories/__init__.pyclaude_code_log/factories/agent_metadata_factory.pyclaude_code_log/factories/meta_factory.pyclaude_code_log/factories/teammate_factory.pyclaude_code_log/factories/tool_factory.pyclaude_code_log/factories/user_factory.pyclaude_code_log/models.pytest/test_data/teammates/README.mdtest/test_data/teammates/ef000000-0000-4000-8000-000000000001.jsonltest/test_data/teammates/ef000000-0000-4000-8000-000000000001/subagents/agent-aaaa111111111111.jsonltest/test_data/teammates/ef000000-0000-4000-8000-000000000001/subagents/agent-bbbb222222222222.jsonltest/test_teammates_parsing.pywork/teammates-plan.md
Two CodeRabbit findings the user confirmed as valid during PR review.
## Metadata anchor: take the last agentId line, not the first
`parse_agent_result_metadata` anchored on the first `agentId:` match,
so an agent response that itself quoted another agent's metadata
verbatim (line-starting `agentId: ...`) lost everything after that
first line — including its own real response and its own tail.
Switch to `finditer` + take the final match so the body is never
truncated just because it happens to discuss `agentId:`.
Also tighten `_WORKTREE_PATH_RE` / `_WORKTREE_BRANCH_RE`: use
`[^\S\r\n]*...(.+?)[^\S\r\n]*$` instead of `\S+` so worktree
paths containing spaces survive (e.g. /home/user/My Worktrees/…).
The `^\s*` prefix was widened to `[^\S\r\n]*` for the anchor line
too, making line-starts explicit about horizontal whitespace only.
## Strict bool/string validation for JSON success payloads
`parse_teamdelete_output` and `parse_sendmessage_output` used
`bool(data.get("success", False))`, which coerces a stringified
`"false"` into `True`. A malformed tool_result reporting a
failure as `{"success": "false", ...}` would have silently
rendered as a success. Same risk on stringified `message`.
Reject the payload (return None, fall back to generic rendering)
when `success` isn't a real bool or `message` isn't a real str.
## Regression tests
- test_body_mentioning_agent_id_is_preserved: response body
contains a literal `agentId: bogus1234` line before the real
tail; asserts the first line stays in the body and the meta
comes from the *second* agentId.
- test_worktree_path_with_spaces: worktreePath + worktreeBranch
both with spaces, asserts full values are captured.
- test_teamdelete_rejects_string_success: stringified "false"
returns None.
- test_sendmessage_rejects_string_success /
test_sendmessage_rejects_non_string_message: same shape on
SendMessage.
All 50 teammate-parsing tests pass; verified each new test fails
on the pre-fix code.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
test/test_teammates_parsing.py (4)
499-501: Hard-coded entry count will bite on every fixture tweak.
assert len(fixture_messages) == 28is an exact-match anchor on the trimmed transcript. Any future fixture edit (e.g. adding a shutdown-ack entry when you land the lifecycle work for#91) silently fails this test even if the new entry is correct. Since you already have structural assertions below (alice ≥ 4, bob ≥ 4, back-patched ids), consider either dropping this to>=or pinning it to the sum of named constants so a fixture change is self-documenting.🧪 Suggested shape
- def test_main_and_both_subagents_load(self, fixture_messages: list) -> None: - # 22 main + 3 alice + 3 bob = 28 entries - assert len(fixture_messages) == 28 + MAIN_ENTRIES = 22 + ALICE_ENTRIES = 3 + BOB_ENTRIES = 3 + + def test_main_and_both_subagents_load(self, fixture_messages: list) -> None: + assert len(fixture_messages) == ( + self.MAIN_ENTRIES + self.ALICE_ENTRIES + self.BOB_ENTRIES + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/test_teammates_parsing.py` around lines 499 - 501, The test test_main_and_both_subagents_load uses a brittle exact match assert on fixture_messages length (assert len(fixture_messages) == 28); change this to a resilient check by either asserting a lower bound (e.g., assert len(fixture_messages) >= 28) or computing the expected total from named constants (e.g., MAIN_ENTRIES + ALICE_ENTRIES + BOB_ENTRIES) and comparing against that sum so future fixture additions don't break the test; update the assertion on fixture_messages accordingly and keep the existing structural assertions for alice/bob ids intact.
585-599:startswithon a full UUID reads as a prefix check but is really equality.
m.uuid.startswith("aaaaaaaa-0000-4000-8000-000000000001")/bbbbbbbb-…are full UUIDs, sostartswithis effectively==with a weaker signal to the reader. If the intent is "any sidechain root for this agent," consider matching onm.parentUuid is None+m.agentIdinstead of on the UUID literal; if the intent really is "this specific entry," use==and drop thestartswith.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/test_teammates_parsing.py` around lines 585 - 599, The current check uses m.uuid.startswith(...) with a full UUID which is misleading; change it to either a direct equality check (m.uuid == "aaaaaaaa-0000-4000-8000-000000000001" / "bbbbbbbb-...") if you truly mean those specific entries, or, preferably, detect sidechain roots by checking m.parentUuid is None combined with m.agentId (and compare against anchors[m.agentId]) so the intent is clear; update the logic around UserTranscriptEntry handling (the loop using m.uuid, m.parentUuid, m.agentId, and anchors) accordingly.
518-543: Innerbreakonly exits the content loop — harmless here, but easy to trip on later.The loop collects
tool_use_idfor bob'sTask, but thebreakonly escapes thefor item in m.message.contentloop; the outerfor m in fixture_messageskeeps iterating and would overwritetool_use_idif the fixture ever grew a second "bob" Task. Today's fixture has one, so it works, but a sentinel-assignment after the match (orbreak-ing the outer with a flag/helper) would make this test robust to fixture growth.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/test_teammates_parsing.py` around lines 518 - 543, The inner break in test_bob_tool_result_back_patched_with_agentid only exits the content loop and can allow tool_use_id to be overwritten if another "bob" Task appears; update the loop so once you find the ToolUseContent with name "Task" and input.get("name") == "bob" you stop iterating fixture_messages — e.g., set tool_use_id and then break out of the outer loop (use a flag, return early, or otherwise exit both loops) so tool_use_id remains the first-match sentinel; reference symbols: test_bob_tool_result_back_patched_with_agentid, fixture_messages, AssistantTranscriptEntry, ToolUseContent, and tool_use_id.
488-495: Consider tightening fixture return types.
fixture_messages() -> listandfixture_dag() -> tuple[list, object]lose element/DAG types at the fixture boundary, forcing the consumers below to useisinstance(...)andgetattr(..., ...)to narrow. Since the module already imports the concrete entry types, a tighter annotation (e.g.list[TranscriptEntry]or evenlist[Any]) would make the tests easier to navigate and catch mis-renames at type-check time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/test_teammates_parsing.py` around lines 488 - 495, The fixtures use overly-broad return annotations: change fixture_messages() -> list to a tighter type like list[TranscriptEntry] (or list[Any] if truly generic) and change fixture_dag() -> tuple[list, object] to tuple[list[TranscriptEntry], YourConcreteDagType] (replace YourConcreteDagType with the actual DAG type already imported in the test module) so consumers can rely on precise types; keep calls to load_transcript(MAIN_JSONL, ...) and load_directory_transcripts(FIXTURE_DIR, ...) unchanged but update the fixture return annotations to the concrete types (e.g., TranscriptEntry and the module's DAG type) to eliminate downstream isinstance/getattr usage.claude_code_log/factories/tool_factory.py (1)
898-899:_opt_strsilently drops empty strings.
return value if isinstance(value, str) and value else Noneturns a legitimate empty-string JSON value ("team_file_path": "") intoNone. For the current teammate tool fields that's likely fine (empty path ≈ absent), but the name_opt_strimplies "str or None" rather than "non-empty str or None". Consider renaming to_opt_nonempty_stror dropping the truthiness check if empty strings are valid.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@claude_code_log/factories/tool_factory.py` around lines 898 - 899, The helper _opt_str currently treats empty strings as None (using "and value"), which mismatches its name; decide whether empty strings should be preserved: if empty strings are valid inputs, remove the truthiness check so _opt_str returns value when isinstance(value, str) else None; otherwise rename the function to _opt_nonempty_str (and update all callers/usages of _opt_str to the new name) to make the non-empty semantics explicit. Ensure you update any tests or call sites that rely on the previous behavior accordingly.
🤖 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/tool_factory.py`:
- Around line 800-827: The regex `_TASK_UPDATE_RE` currently leaves trailing
punctuation on the last field name; update parse_taskupdate_output to normalize
field names by stripping surrounding whitespace and trailing punctuation (e.g.
use name.strip().rstrip(string.punctuation) or equivalent) when building
`updated_fields` from `fields_raw`, and add a regression unit test that feeds an
output like "Updated task `#1` owner, status." to ensure the parsed keys are
"owner" and "status" (refer to the `parse_taskupdate_output`, `fields_raw`,
`updated_fields`, and `_TASK_UPDATE_RE` symbols).
---
Nitpick comments:
In `@claude_code_log/factories/tool_factory.py`:
- Around line 898-899: The helper _opt_str currently treats empty strings as
None (using "and value"), which mismatches its name; decide whether empty
strings should be preserved: if empty strings are valid inputs, remove the
truthiness check so _opt_str returns value when isinstance(value, str) else
None; otherwise rename the function to _opt_nonempty_str (and update all
callers/usages of _opt_str to the new name) to make the non-empty semantics
explicit. Ensure you update any tests or call sites that rely on the previous
behavior accordingly.
In `@test/test_teammates_parsing.py`:
- Around line 499-501: The test test_main_and_both_subagents_load uses a brittle
exact match assert on fixture_messages length (assert len(fixture_messages) ==
28); change this to a resilient check by either asserting a lower bound (e.g.,
assert len(fixture_messages) >= 28) or computing the expected total from named
constants (e.g., MAIN_ENTRIES + ALICE_ENTRIES + BOB_ENTRIES) and comparing
against that sum so future fixture additions don't break the test; update the
assertion on fixture_messages accordingly and keep the existing structural
assertions for alice/bob ids intact.
- Around line 585-599: The current check uses m.uuid.startswith(...) with a full
UUID which is misleading; change it to either a direct equality check (m.uuid ==
"aaaaaaaa-0000-4000-8000-000000000001" / "bbbbbbbb-...") if you truly mean those
specific entries, or, preferably, detect sidechain roots by checking
m.parentUuid is None combined with m.agentId (and compare against
anchors[m.agentId]) so the intent is clear; update the logic around
UserTranscriptEntry handling (the loop using m.uuid, m.parentUuid, m.agentId,
and anchors) accordingly.
- Around line 518-543: The inner break in
test_bob_tool_result_back_patched_with_agentid only exits the content loop and
can allow tool_use_id to be overwritten if another "bob" Task appears; update
the loop so once you find the ToolUseContent with name "Task" and
input.get("name") == "bob" you stop iterating fixture_messages — e.g., set
tool_use_id and then break out of the outer loop (use a flag, return early, or
otherwise exit both loops) so tool_use_id remains the first-match sentinel;
reference symbols: test_bob_tool_result_back_patched_with_agentid,
fixture_messages, AssistantTranscriptEntry, ToolUseContent, and tool_use_id.
- Around line 488-495: The fixtures use overly-broad return annotations: change
fixture_messages() -> list to a tighter type like list[TranscriptEntry] (or
list[Any] if truly generic) and change fixture_dag() -> tuple[list, object] to
tuple[list[TranscriptEntry], YourConcreteDagType] (replace YourConcreteDagType
with the actual DAG type already imported in the test module) so consumers can
rely on precise types; keep calls to load_transcript(MAIN_JSONL, ...) and
load_directory_transcripts(FIXTURE_DIR, ...) unchanged but update the fixture
return annotations to the concrete types (e.g., TranscriptEntry and the module's
DAG type) to eliminate downstream isinstance/getattr usage.
🪄 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: 0632a6d7-466d-42f7-88a8-e50038e2b9ba
📒 Files selected for processing (3)
claude_code_log/factories/agent_metadata_factory.pyclaude_code_log/factories/tool_factory.pytest/test_teammates_parsing.py
✅ Files skipped from review due to trivial changes (1)
- claude_code_log/factories/agent_metadata_factory.py
Comment on lines
+800
to
+827
| def parse_taskupdate_output( | ||
| tool_result: ToolResultContent, file_path: Optional[str] | ||
| ) -> Optional[TaskUpdateOutput]: | ||
| """Parse TaskUpdate text output ``"Updated task #N <comma-list-of-fields>"``. | ||
|
|
||
| The plain-text form doesn't report the old/new status explicitly; | ||
| ``status_change`` remains None unless a richer structured form appears. | ||
| """ | ||
| del file_path | ||
| text = _extract_tool_result_text(tool_result) | ||
| if not text: | ||
| return None | ||
| match = _TASK_UPDATE_RE.match(text.strip()) | ||
| if match is None: | ||
| return None | ||
| fields_raw = match.group("fields") or "" | ||
| updated_fields: Optional[dict[str, Any]] = None | ||
| if fields_raw: | ||
| updated_fields = { | ||
| name.strip(): True for name in fields_raw.split(",") if name.strip() | ||
| } | ||
| return TaskUpdateOutput( | ||
| success=True, | ||
| task_id=match.group("id"), | ||
| updated_fields=updated_fields, | ||
| status_change=None, | ||
| raw_text=text, | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Claude Code TaskUpdate tool result text format teammates
💡 Result:
The TaskUpdate tool in Claude Code is used to update tasks in the shared task list, including status changes, details, ownership, and dependencies. It is a key tool for both individual sessions and agent teams (teammates), where multiple Claude Code instances coordinate via a shared task list. Tool input is a JSON object with required "taskId" (string, e.g., "1") and optional fields: - status: "pending", "in_progress", "completed", or "deleted" - subject: task title (imperative form) - description: task description - activeForm: present continuous form for in_progress spinner - owner: agent/teammate name - metadata: object to merge (set key to null to delete) - addBlockedBy: array of taskIds that must complete first - addBlocks: array of taskIds blocked by this one (less common in examples) Examples: {"taskId": "1", "status": "in_progress"} {"taskId": "1", "status": "completed"} {"taskId": "1", "status": "deleted"} {"taskId": "1", "owner": "teammate-name"} {"taskId": "2", "addBlockedBy": ["1"]} In agent teams (enabled via CLAUDE_CODE_EXPERIMENTAL_AGENT_TEAMS=1), teammates use TaskUpdate to claim tasks (set owner), mark progress/completion, and manage dependencies. Always use TaskGet/TaskList first to check current state and avoid staleness. No permissions required. Result text format is standard tool output (not explicitly detailed in sources), typically confirming the update with updated task details.
Citations:
- 1: https://ccprompts.info/prompts/tool/tool-task-update
- 2: http://code.claude.com/docs/en/tools-reference
- 3: https://github.com/codeaashu/claude-code/blob/main/docs/tools.md
- 4: https://code.claude.com/docs/en/agent-teams
- 5: https://code.claude.com/docs/en/agent-teams.md
- 6: https://github.com/hangsman/claude-code-source/blob/main/sdk-tools.d.ts
🏁 Script executed:
# First, find and examine the _TASK_UPDATE_RE pattern definition
rg -n "_TASK_UPDATE_RE" claude_code_log/factories/tool_factory.pyRepository: daaain/claude-code-log
Length of output: 204
🏁 Script executed:
# Also search for any related regex or pattern definitions
rg -B 5 -A 2 "def parse_taskupdate_output" claude_code_log/factories/tool_factory.pyRepository: daaain/claude-code-log
Length of output: 267
🏁 Script executed:
# Look for test files related to tool_factory
fd -e py test | grep -i "tool_factory\|task.*update" | head -20Repository: daaain/claude-code-log
Length of output: 48
🏁 Script executed:
# Search for test files more broadly
fd -e py . | grep -E "(test_|_test\.py|tests\.py)" | head -20Repository: daaain/claude-code-log
Length of output: 624
🏁 Script executed:
# Look for any comments or TODOs related to punctuation or task parsing
rg -i "punctuation|trailing|period|task.*update.*output" --type py -A 2Repository: daaain/claude-code-log
Length of output: 10743
🏁 Script executed:
# Check what the actual output format looks like in the codebase
rg -B 3 -A 3 "TaskUpdateOutput" claude_code_log/factories/tool_factory.py | head -40Repository: daaain/claude-code-log
Length of output: 727
🏁 Script executed:
# Test the regex pattern to confirm the issue
python3 << 'EOF'
import re
_TASK_UPDATE_RE = re.compile(r"^Updated task #(?P<id>\d+)(?:\s+(?P<fields>.+))?$")
test_cases = [
"Updated task `#1` owner, status.",
"Updated task `#1` owner, status",
"Updated task `#1` owner; status.",
"Updated task `#2`",
]
for test in test_cases:
match = _TASK_UPDATE_RE.match(test.strip())
if match:
fields_raw = match.group("fields") or ""
print(f"Input: {test!r}")
print(f" fields_raw: {fields_raw!r}")
if fields_raw:
fields = {name.strip(): True for name in fields_raw.split(",") if name.strip()}
print(f" parsed: {fields}")
print()
EOFRepository: daaain/claude-code-log
Length of output: 442
🏁 Script executed:
# Extract the full test_taskupdate_output test
rg -A 10 "def test_taskupdate_output" test/test_teammates_parsing.pyRepository: daaain/claude-code-log
Length of output: 558
🏁 Script executed:
# Also look for _tr_text helper to understand the test setup
rg -B 5 -A 5 "_tr_text" test/test_teammates_parsing.py | head -30Repository: daaain/claude-code-log
Length of output: 1285
🏁 Script executed:
# Check if there are any other TaskUpdate-related tests
rg -n "taskupdate\|TaskUpdate" test/test_teammates_parsing.pyRepository: daaain/claude-code-log
Length of output: 48
Add regression test and hardening for trailing punctuation in TaskUpdate fields.
The _TASK_UPDATE_RE pattern uses a greedy .+ that captures trailing punctuation into the last field name. For example, "Updated task #1 owner, status." produces {"owner": True, "status.": True} instead of the expected field names. Add the suggested rstrip to strip trailing punctuation and include a test case with a trailing period to prevent regression.
🔧 Suggested hardening
fields_raw = match.group("fields") or ""
+ # Drop trailing punctuation (e.g. a stray period) so it doesn't leak
+ # into the last field name.
+ fields_raw = fields_raw.rstrip(" .;")
updated_fields: Optional[dict[str, Any]] = None
if fields_raw:
updated_fields = {
name.strip(): True for name in fields_raw.split(",") if name.strip()
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@claude_code_log/factories/tool_factory.py` around lines 800 - 827, The regex
`_TASK_UPDATE_RE` currently leaves trailing punctuation on the last field name;
update parse_taskupdate_output to normalize field names by stripping surrounding
whitespace and trailing punctuation (e.g. use
name.strip().rstrip(string.punctuation) or equivalent) when building
`updated_fields` from `fields_raw`, and add a regression unit test that feeds an
output like "Updated task `#1` owner, status." to ensure the parsed keys are
"owner" and "status" (refer to the `parse_taskupdate_output`, `fields_raw`,
`updated_fields`, and `_TASK_UPDATE_RE` symbols).
…#117) parse_taskupdate_output splits fields_raw on "," and keeps the stripped fragments as dict keys. If the tool_result ends in a period or semicolon (e.g. "Updated task #1 owner, status."), that punctuation got baked into the last key: {"owner": True, "status.": True}. Trim " .;:" from the right of fields_raw before splitting so the punctuation doesn't leak. Regression test feeds both "." and ";" variants and asserts the keys come out clean. Reported by CodeRabbit in the PR review; verified the test fails on pre-fix code and passes after.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/tool_factory.py`:
- Around line 712-715: The current regexes _TASK_CREATE_RE and _TASK_UPDATE_RE
only match IDs like `#1` and fail when the model returns backticked IDs like `#1`;
update those two patterns to accept optional matching backticks around the
hash+digits (use a named group for the tick, e.g. (?P<tick>`)? and backreference
(?P=tick) to require a closing backtick only if an opening tick exists) so both
`#1` and `#1` parse, and add regression tests for TaskCreate and TaskUpdate
parsing that cover both backticked and non-backticked variants to prevent future
regressions.
🪄 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: 528c8cf2-a5b5-4372-93f3-6f4a7efbd99f
📒 Files selected for processing (2)
claude_code_log/factories/tool_factory.pytest/test_teammates_parsing.py
Comment on lines
+712
to
+715
| _TASK_CREATE_RE = re.compile( | ||
| r"^Task #(?P<id>\d+) created successfully: (?P<subject>.+)$" | ||
| ) | ||
| _TASK_UPDATE_RE = re.compile(r"^Updated task #(?P<id>\d+)(?:\s+(?P<fields>.+))?$") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Search for TaskCreate/TaskUpdate result text with backticked task ids.
# Expectation: if matches are found, the current regexes do not parse those lines.
rg -nP 'Task\s+`#\d+`\s+created successfully|Updated task\s+`#\d+`' .Repository: daaain/claude-code-log
Length of output: 48
🏁 Script executed:
cat -n claude_code_log/factories/tool_factory.py | sed -n '710,820p'Repository: daaain/claude-code-log
Length of output: 4758
🏁 Script executed:
fd -i 'test.*tool_factory|tool_factory.*test' --type fRepository: daaain/claude-code-log
Length of output: 48
🏁 Script executed:
rg -l 'TASK_CREATE_RE|TASK_UPDATE_RE' --type pyRepository: daaain/claude-code-log
Length of output: 107
🏁 Script executed:
fd -i 'test' --type f | grep -i 'tool_factory\|parser\|task'Repository: daaain/claude-code-log
Length of output: 48
🏁 Script executed:
rg -l 'parse_taskcreate_output|parse_taskupdate_output|TASK_CREATE_RE|TASK_UPDATE_RE' --type pyRepository: daaain/claude-code-log
Length of output: 138
🏁 Script executed:
rg -B5 -A5 'TaskCreateOutput|TaskUpdateOutput' --type py | head -100Repository: daaain/claude-code-log
Length of output: 5949
🏁 Script executed:
cat -n test/test_teammates_parsing.py | grep -A 15 'test_taskcreate\|test_taskupdate'Repository: daaain/claude-code-log
Length of output: 2699
🏁 Script executed:
python3 << 'EOF'
import re
# Test the proposed regex with backreference
pattern = r"^Task (?P<tick>`?)#(?P<id>\d+)(?P=tick) created successfully: (?P<subject>.+)$"
regex = re.compile(pattern)
test_cases = [
"Task `#3` created successfully: Add relay tests",
"Task `#3` created successfully: Add relay tests",
"Task `#3 created successfully: Add relay tests", # unmatched backticks
"Task `#3`` created successfully: Add relay tests", # unmatched backticks
]
for test in test_cases:
match = regex.match(test)
print(f"Pattern: {pattern}")
print(f"Test: {test}")
print(f"Match: {match is not None}")
if match:
print(f" id={match.group('id')}, subject={match.group('subject')}")
print()
EOFRepository: daaain/claude-code-log
Length of output: 747
Add support for backticked task IDs in TaskCreate/TaskUpdate parsers.
The regexes at lines 712–715 only match #1, so LLM outputs using `#1` fall through to raw rendering instead of producing structured task outputs. The proposed patterns using backreferences ((?P=tick)) correctly handle optional matching backticks while maintaining backward compatibility. Add regression tests for both backticked and non-backticked variants to prevent future regressions.
Proposed parser hardening
_TASK_CREATE_RE = re.compile(
- r"^Task #(?P<id>\d+) created successfully: (?P<subject>.+)$"
+ r"^Task (?P<tick>`?)#(?P<id>\d+)(?P=tick) created successfully: (?P<subject>.+)$"
)
-_TASK_UPDATE_RE = re.compile(r"^Updated task #(?P<id>\d+)(?:\s+(?P<fields>.+))?$")
+_TASK_UPDATE_RE = re.compile(
+ r"^Updated task (?P<tick>`?)#(?P<id>\d+)(?P=tick)(?:\s+(?P<fields>.+))?$"
+)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@claude_code_log/factories/tool_factory.py` around lines 712 - 715, The
current regexes _TASK_CREATE_RE and _TASK_UPDATE_RE only match IDs like `#1` and
fail when the model returns backticked IDs like `#1`; update those two patterns
to accept optional matching backticks around the hash+digits (use a named group
for the tick, e.g. (?P<tick>`)? and backreference (?P=tick) to require a closing
backtick only if an opening tick exists) so both `#1` and `#1` parse, and add
regression tests for TaskCreate and TaskUpdate parsing that cover both
backticked and non-backticked variants to prevent future regressions.
Collaborator
Author
There was a problem hiding this comment.
We'll handle that when this happens...
There was a problem hiding this comment.
@cboos Understood — happy to defer that hardening until backticked task IDs are actually seen in the wild. Feel free to reopen this if you do encounter them.
This was referenced Apr 19, 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
Draft PR for the first of three stages implementing first-class teammates support (Claude Code's research-preview agent-teams feature,
CLAUDE_CODE_EXPERIMENTAL_AGENT_TEAMS=1).This PR currently contains only the plan. The implementation is not yet landed — use this PR for visibility and to reference the plan during design discussion. The branch will grow as commits 2-8 from the roadmap go in.
Plan
See
work/teammates-plan.mdon this branch for the full plan:agentId:from tool_result Markdown tails (primary), prompt-hash fallbackagentIdplumbing originally scoped to Support new transcript structure (2026-01) #79 and feeds Support async agents #90What PR 1 (this one) will contain
TeamCreate,TeamDelete,TaskCreate,TaskUpdate,TaskList,SendMessagetool I/OTaskInput/TaskOutputfor teammate-spawn fieldsTeammateMessageMessageContent for<teammate-message>XML in User entries<usage>block)_integrate_agent_entriestest/test_data/teammates/trimmed from real dataNon-goals for PR 1: no UI changes, no new formatters, no CSS. Generic renderer still renders teammates' tools as param tables. Keeps the PR reviewable.
Relates to
Test plan
<teammate-message>XML parser handles single, multi, mixed, and system-teammate shapesjust cigreen🤖 Generated with Claude Code
Summary by CodeRabbit