[ckperry]

Human overview

Adds comments on room notes to the CLI: pepper comments list|create|resolve|reopen --note N, and messages list / agents claude-channel now show a note's comment context (header, quoted passage, resolved state) so an agent addressed in a comment can answer under the right thread. Third of three PRs; it depends on the pepper PR (the /api/v1/artifacts/{id}/comments routes) and the groups PR (anchors, subject_artifact_id on topics). No database changes in this repo.

Human required (optional)

• Merge after the pepper and groups PRs; then refresh the vendored contract snapshot (weekly workflow) and add the contract markers (see below).
• Confirm the server's agent-token allowlist admits the four comments routes (this PR documents comments list|create|resolve|reopen as agent-usable in SKILL.md section 8).

AI reviewer

What the diff does

• api_client.py: NoteComment, NoteCommentThread, NoteComments and ApiClient.list_note_comments, create_note_comment, resolve_note_thread, reopen_note_thread. GroupMessage gains anchor, resolved_at, resolved_by_name; GroupTopicSummary gains subject_artifact_id. Parsing tolerates missing and unknown fields; deleted and resolver names are parsed, mentions accept {id, display_name}.
• cli.py: comments list [--all], create [--quote|--reply-to] [--mention ...] [MESSAGE|-] [--idempotency-key], resolve|reopen THREAD_ID; all take --note (flag, never positional) and --room. --all only filters human output; --json always carries every thread plus resolved_count. Server refusals map to messages with exit 2: 422 quote_not_found, quote_ambiguous (with count), quote_mismatch; 409 personal_note; quote_invalid and anything else fall through to the generic notes handler.
• render.py / claude_channel.py: messages list prints a header line on a note's comments topic, a > "quote" line (capped at about 160 chars) and (resolved by X); the channel adds the same to event text plus note_id in meta. messages list now makes one extra list_group_topics call in human mode only, best effort (an ApiError there is ignored), done before the read mark so a failed mark still prints nothing.
• SKILL.md, README, CHANGELOG, IA doc: commands and the "When you're addressed in a note's comments" section. Commands are in the full pepper --profile <name> form (a drift-guard test requires it); notes show is not suggested to agents because an agent token cannot run it.

Contract markers (deliberately absent)

The new dataclasses carry no # MIRROR: marker because the vendored snapshot has no such routes until the pepper PR merges, and tests/test_api_contract.py would fail on a marker for a missing route. The new fields on the already-mirrored GroupMessage and GroupTopicSummary are not in the snapshot either; test_api_contract.py passes as is. Follow-up after the pepper PR merges and the snapshot refreshes: add the markers for the comment types and the new fields.

Where to look hardest

• _report_comments_api_error: parses the 422/409 body (detail dict or top-level) and falls back to the notes handler.
• cmd_messages_list ordering of the topic lookup relative to the read mark.
• Wrapping: the note header and quote lines print with soft_wrap so an id in them can be copied intact.

Verification (run in this worktree)

• uv run ruff format --check, uv run ruff check, uv run ty check src/pepper_cli/: clean.
• uv run pytest --cov: 2510 passed, 2 skipped; total coverage 95% (floor 88, unchanged).
• New tests: tests/test_api_client_note_comments.py, tests/test_cli_comments.py, additions in tests/test_claude_channel.py; conformance tables (VERBS, NOUNS, CONTAINER, ROUTES, JSON_CASES) and _AGENT_COMMANDS / _API_NOUNS updated. Not exercised against a live server, since the routes are not deployed yet.

Checklist (repo-specific)

• uv run ruff format --check and uv run ruff check clean (no new lint ignores)
• uv run ty check src/pepper_cli/ clean
• uv run pytest passes; coverage floor in .claude/maintainerd.json not lowered
• Server-contract changes: /api/v1 usage stays additive-only, # MIRROR: comments updated, unknown fields and frames still tolerated. Additive-only and tolerant yes; MIRROR markers for the new types deferred until the snapshot has the routes (above).
• No token or credential is logged, printed, or put in a fixture
• README and the bundled SKILL.md updated if commands or output changed

🤖 Generated with Claude Code

https://claude.ai/code/session_01178t2H5cGcXqTz3RX29qyf

Confidence Score: 5/5

The reviewed changes appear safe to merge under the stated server rollout plan.

Summary

Adds room-note comments, thread resolve/reopen commands, and note context in message output.

Changes since the previous review:

• Room members can discuss notes in comment threads.
• Note comment messages show the note and quoted passage.

No new actionable issues were found. Both earlier threads are fixed: the optional topic lookup catches transport errors, and suggested commands preserve the active profile.

ckperry deferred the contract markers until the server changes merge and the vendored snapshot refreshes, including markers for fields added to existing types.

_{Reviews (3) · Last reviewed commit: "docs(skill): agents can read and edit ro..." · Reviewed by Greptile}

[vycari-review]

Vycari Review (shadow)

Shadow round 4 on cb5010e. This comment is non-gating and needs no response: it is collected to measure agreement with the current reviewer. The round's verdict is the Vycari Review (shadow) check run.

Open: P0 0 · P1 3 · P2 3 · confirmed fixed 0 · withdrawn 0

Judge's notes

Since the last round only CHANGELOG lines changed. Two P1s are still open: a malformed topics answer aborts messages list with no output, and comment JSON drops author and resolver identities. A new P1 adds an unsanitized resolver name printed to the terminal.

P1 · VR-27a85bfe · Resolver display name printed raw, unlike every other server name in the renderer

src/pepper_cli/render.py lines 1074–1076 · symbol render_group_messages · lens cross-file-contract · status new

render_group_messages prints m.resolved_by_name directly into an f-string written with markup=False and no sanitizing. Every other server-supplied name in the same function and in render_note_comments goes through _s() (render.py:112, which turns control characters into spaces), or sanitize_line/sanitize_text. This one does not, so control characters or terminal escape sequences in another member's display name reach the terminal unchanged.

Evidence: render.py:1074-1075 prints m.resolved_by_name raw; render.py:921 wraps the analogous resolved_by in _s(); render.py:112 defines _s as the one-line sanitizer. api_client.py:1578 copies resolved_by_name from the server row without cleaning. Checked the claude_channel path (claude_channel.py:284-310): it does not print resolved_by at all, so this gap is only in messages list. Readers of GroupMessage.resolved_by_name: render.py:1075 only. Protection looked for: any sanitize in group_message_from_dict or in render_group_messages around the resolver; none found. Other new contract checks that held up: _emit_list/_to_plain serialize NoteCommentThread lists correctly (cli.py:4581, 4772), build_topic_map refreshes on a topic miss (claude_channel.py:520-532), and the channel's preface is sanitized (claude_channel.py:293-300). The topic-lookup and identity issues in the prior rounds are not repeated here.

Failure scenario

A room member's display name contains an ANSI/OSC escape sequence. A teammate resolves a comment thread on a note. Running 'pepper messages list --room R --topic <note comments topic>' in human mode on the note's comments topic prints '(resolved by <name>)' with the escape sequence active, which can rewrite the terminal line or title or hide text around it.

How this was verified (read-only-reasoning)

I compared the two renderers in the diff. The comments-list path sanitizes the resolver name and the messages-list path does not. Reachable from: pepper messages list on a note comments topic.

Proving test

Set a GroupMessage with resolved_at set and resolved_by_name='\x1b]0;pwned\x07Bo'; call render_group_messages on a one-message page and assert the captured stdout contains no ESC or BEL character (expected to fail on this head because the name is printed raw).

P1 · VR-44604869 · Comment JSON discards author and resolver identities

src/pepper_cli/api_client.py lines 1639–1649 · symbol _note_comment_from_dict · lens cross-file-contract · status still-open

The parser replaces the server's author and resolved_by objects with display-name strings. List, resolve and reopen serialize these reduced dataclasses, whereas create preserves the server objects. This violates the written JSON pass-through contract and makes the same message's field types differ between commands.

Evidence: api_client.py:1639 and :1649 retain only display_name; NoteComment has no alternative author or resolver id fields (:1597-1608). cli.py:6991-7000 and :7023 serialize these parsed objects through _emit_list/_emit. In contrast, create_note_comment returns the raw response dictionary (api_client.py:3576-3584). planning/cli-information-architecture.md:242-244 requires server-shaped mutation results and passed-through fields; these rules also exist on the current base.

Failure scenario

A script reads a comment's author.id from comments create --json, then refreshes it through comments list --json. The author field is now a string, so the script fails and cannot recover the author's identity from that response. Two members sharing a display name also become indistinguishable.

How this was verified (traced-callers)

I read `_note_comment_from_dict` and `cmd_comments_list` in the diff. The parsed dataclasses go straight into `_emit_list` and `_emit`. I did not read `_emit` itself. Reachable from: pepper comments list|resolve|reopen --json.

Proving test

Mock create, list and resolve responses containing author and resolved_by objects with distinct ids. Invoke the JSON commands and assert that each returned message preserves those objects and ids. List and resolve fail the assertion.

P1 · VR-fe8553da · Topic-lookup fix still lets a malformed topics answer abort messages list with no output

src/pepper_cli/cli.py lines 5531–5542 · symbol cmd_messages_list · lens concurrency-time · status still-open

The best-effort topic lookup added before the read mark catches only api_client.ApiError and httpx.RequestError. list_group_topics parses each row with _group_topic_from_dict (api_client.py:1605-1615), which indexes row["topic_id"], row["name"] and row["last_activity_at"] directly, so a topic row missing one of them raises KeyError. A non-JSON body raises ValueError from .json() in _request's caller. Neither is caught, so the exception escapes run(), the messages already fetched are never printed, and the --mark advance (line 5544, after the lookup) never happens. The fix therefore covers HTTP and transport failures only, not the parse failures the prior finding named.

Evidence: Read cli.py:5522-5555: the topic lookup runs before set_group_read_mark (5544-5546) and before _emit_list; the except clause at 5541 catches only ApiError and httpx.RequestError. api_client.py:1605-1615 _group_topic_from_dict uses row["topic_id"], row["name"], row["last_activity_at"] without get. api_client.py:3207-3211 list_group_topics calls .json() with no guard. No wrapper around _with_room_api catches KeyError or ValueError for this path. This is the prior round's VR-fe8553da, still present at head.

Failure scenario

A user runs `pepper messages list --room R --topic T` in human mode. The messages GET returns 200 with a page. The topics GET returns 200 with a topics array in which one row lacks last_activity_at (or the body is not JSON). _group_topic_from_dict raises KeyError inside the generator at 5533-5540, which is not in the except tuple at 5541, so run() raises and nothing is printed. With --mark the read mark is never set, so every later run fails the same way. Before this PR the command made no topics call and printed the messages.

How this was verified (traced-callers)

I read the lookup block in cmd_messages_list and the except tuple, and checked that _with_room_api does not catch KeyError or ValueError. I did not open _group_topic_from_dict, so its direct indexing is taken from the candidate. Reachable from: pepper messages list (human mode).

Proving test

Mock GET /api/v1/groups/{gid}/topics to return 200 {"topics":[{"topic_id":"t1","name":"Dinner"}]} (no last_activity_at) and GET the messages page with one message. Run `messages list --room R --topic t1` with human output. Assert exit 0 and that the message line is printed. On the PR head the KeyError propagates and stdout is empty. A second case with a non-JSON 200 body for the topics URL should fail the same way.

P2 · VR-00f96a97 · Suggested context reads fail room-pinned permissions

src/pepper_cli/claude_channel.py lines 295–298 · symbol message_event · lens shell-infra · status new

The generated command places --note before --room, violating the command-prefix contract used by room-pinned harness permissions. Following the suggested command therefore prompts or is denied despite the context read being preauthorized.

Evidence: planning/cli-information-architecture.md:98-102 explicitly requires room-first commands because other orders fail pinned permission rules. message_event emits the opposite order at claude_channel.py:297. The related human-output suggestion omits --room entirely at render.py:995-1000, reached from cli.py:5550. Profile preservation does not address this mismatch. The channel exposes only message tools at claude_channel.py:875-938, so there is no comments-list MCP tool that supplies an alternative context-read path.

Failure scenario

An agent has an allow rule for `pepper --profile helper comments list --room ROOM:*`. A note-comment notification instructs it to run `pepper --profile helper comments list --note NOTE --room ROOM`. That command does not match the rule, so the agent cannot retrieve the thread context without another approval.

How this was verified (traced-callers)

I read the IA rule that --room must come right after the verb. The channel preface and render.note_topic_header both emit the non-canonical order, or omit --room. Reachable from: agents claude-channel note-comment event.

Proving test

Select profile helper, construct a note-topic notification, extract its suggested command, and assert that it matches the configured `pepper --profile helper comments list --room ROOM` permission prefix. The assertion fails on this head.

P2 · VR-6076b363 · Any 409 on the comment routes is reported as a personal-note refusal

src/pepper_cli/cli.py lines 6915–6956 · symbol _report_comments_api_error · lens cross-file-contract · status still-open

The 409 branch prints 'personal notes cannot be commented on; copy the note into a room' whenever error == 'personal_note' OR exc.detail is falsy. exc.detail comes from _extract_detail (api_client.py:4388-4410), which returns None when the body's 'detail' is a dict. The comment routes' error shape is {'detail': {'error': ..., 'count': ...}} (the code reads it that way for 422 quote_*). So any other 409 on these routes, with a dict detail, has error!='personal_note' but exc.detail is None, and the message is replaced by the personal-note text. The real error code in the body is discarded.

Evidence: cli.py:6940-6949 (409 branch: `"personal notes cannot be commented on..." if error == "personal_note" or not exc.detail else exc.detail`). api_client.py:4388-4410 (_extract_detail returns None for non-str detail). The code reads error codes from dict detail for 422 (cli.py 6928-6935), so the server's comment routes send dict details. Protections looked for: no check on the 409 error code other than 'personal_note'; tests in tests/test_cli_comments.py only cover the personal_note 409 case as far as the diff shows. The server's full set of 409 codes on these routes is not visible in the checkout, so the trigger is unverified; the defect is the fallback text, which is wrong for any other 409.

Failure scenario

A room member runs 'pepper comments create --note N --room R "text"' on a note whose comments topic is in a state the server refuses with 409 and a dict detail (for example {'detail': {'error': 'topic_archived'}}). The CLI exits 2 with 'personal notes cannot be commented on; copy the note into a room'. The user, or an agent following SKILL.md, concludes the note is personal and tries to copy it into a room instead of fixing the real cause.

How this was verified (read-only-reasoning)

I read the 409 branch in the diff and the candidate's account of `_extract_detail`. I did not open `_extract_detail` or find another 409 code the server sends on these routes. Reachable from: pepper comments create|resolve|reopen|list.

Proving test

Patch ApiClient.create_note_comment to raise ApiError(status=409, body='{"detail": {"error": "topic_archived"}}', detail=None); run cli.main(['comments','create','--note',NID,'--room',GID,'hi']) and assert the stderr does not contain 'personal' and the message names the server's error. It fails on the PR head because the personal-note text is printed.

P2 · VR-c21c0178 · Channel notifications hide a comment's resolved state

src/pepper_cli/claude_channel.py lines 300–307 · symbol message_event · lens cross-file-contract · status still-open

The new comment preface includes the note and quote but never reads resolved_at or resolved_by_name. Neither content nor metadata distinguishes a resolved comment from an open one, despite those fields being available on the fetched message.

Evidence: StreamWorker._push_message fetches the current row and passes it to message_event (claude_channel.py:651-675). message_event builds the comment context at :292-307 and metadata at :308-322 without either resolution field. group_message_from_dict already parses both fields (api_client.py:1502-1503), and the sibling terminal path displays them (render.py:1064-1066), so this is not missing server data. Replies reopening resolved threads is documented in README.md:256.

Failure scenario

A root comment mentions an agent, then a person resolves it before the channel processes its event. The channel fetches the already-resolved message but delivers the original request without its resolved state. The agent sees a settled request as open; answering it can reopen the thread.

How this was verified (read-only-reasoning)

I read `message_event` in the diff. It builds the preface and meta from the note id and anchor only, and never reads `resolved_at`. Reachable from: agents claude-channel event delivery.

Proving test

Deliver a comment event whose fetched GroupMessage has resolved_at and resolved_by_name set. Assert that the channel notification identifies the comment as resolved and names the resolver. The current notification contains neither.

[ckperry]

Round 1 summary (head 78b5a35):

• Greptile P2 "Extra request hides fetched messages" / Vycari Review VR-fe8553da: fixed in 1cc0f8a (catch httpx.RequestError in the best-effort topics lookup; proving test with httpx.ReadTimeout).
• Vycari Review VR-53a924d5 (agent comment commands with a note name): fixed in 38a3a41. With --room, _resolve_note already sends ?principal=<room>, which the pepper PR admits for agent tokens (artifact_agents.py: list forced to inline_text); a proving test runs comments list --room R --note Runbook under an agent token against a fake applying that rule. Without --room, an agent token plus a note name now exits 2 before any request, saying to add --room or use the note's id. SKILL.md says the same.
• Greptile P2 "Header command loses the login": fixed in 78b5a35 (suggested commands keep --profile).

Full suite green locally (2518 passed).

[ckperry]

Vycari Review shadow round 2: both P1s are fixed on 78b5a35. Round 2 carried them as still-open with the round-1 evidence after 11 stage calls failed.

• VR-fe8553da: fixed in 1cc0f8a. cmd_messages_list now catches (api_client.ApiError, httpx.RequestError). The test is test_messages_list_still_prints_when_the_topic_lookup_fails[transport-timeout].
• VR-53a924d5: fixed in 38a3a41. With --room, the name resolves via ?principal=<room>, which the server admits for agent tokens. Without --room, an agent token exits 2 before any request and says to add --room or use the id. The tests are test_agent_resolves_a_note_name_within_the_named_room and test_agent_note_name_without_room_says_to_add_one.

[allenhutchison]

Reviewed against Vycari/pepper#5537 and Vycari/groups#295. The comment-route request/response shapes inspected match the companion server changes. Validation: 363 focused CLI tests passed (two local-socket subprocess tests passed when rerun with the required loopback access). One agent-guide integration gap below.

[allenhutchison]

[P2] Document the room-note access added by the companion server PR

Pepper#5537 now admits agent tokens on room-scoped artifact listing, note detail and note editing, but this guide still says only the thirteen listed commands work and section 9 says every other server command returns 401 and must not be tried. That incorrectly excludes notes list/search --room, notes show, and notes edit --room. The new guidance here tells an agent to read only comments/quotes, so an agent following the guide cannot read the surrounding note even though that permission was deliberately added for this workflow (and a whole-note comment has no quoted passage at all). Please update the capability list and refusal guidance with the scoped note commands and their restrictions, and recommend notes show for full context before answering. Personal notes, other artifact kinds, and note creation/deletion should remain outside the allowed scope.

[ckperry]

Done in 609a35a. SKILL.md now lists seventeen agent commands: the thirteen plus notes list/search/show/edit, each with --room (text notes of a room that names the agent). Section 9 keeps personal notes (no --room), other artifact kinds and notes create/delete out of scope. The comments section now says to run notes show for the whole note before answering, especially a whole-note comment with no quoted passage. README, CHANGELOG and the drift-guard test (command list and count) are updated to match.