Fix review findings: resolver safety, command contract, docs#4
Merged
Conversation
Addresses a code review of v0.2.0. #2/#3/#4 change the CLI's external contract, so the next release should be v0.3.0. #1 (security) — comment/attachment commands no longer mis-resolve a page URL to a page ID. urlref now extracts focusedCommentId; resolveID is split into resolvePageID/resolveCommentID/resolveAttachmentID. A plain page URL passed to `comment delete` / `attachment delete` is rejected instead of silently deleting the wrong content. #2 — the body-format flag on page create/update and comment add/update is renamed --format -> --body-format, so it no longer shadows the global --format (json/table/ndjson) output flag. #3 — config path/use-context/delete-context/init, auth login/logout and skill install/uninstall/path now emit JSON via s.emit; interactive prompts moved to stderr. stdout is uniformly JSON (version stays plain text by design). #4 — list commands emit a {items,next,has_more} envelope and accept --cursor, so an agent can page deterministically. output table/ndjson rendering and --fields projection are envelope-aware. #5 — docs/technical-design.md, README and the embedded Skill updated to match the implemented surface; the error model documents conflict / exit 11. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Quality follow-up to the review fixes, plus a second doc-review pass.
- Replace the structural list-envelope detection in `output` (a heuristic
that guessed the {items,next,has_more} shape from the data) with an explicit
`EmitList` / `appState.emitList` API. The renderer is now told it has a list
rather than reverse-engineering one — table/ndjson/--fields handling moves
into EmitList; `listEnvelope`, `listOutput` and `newListOutput` are gone.
- Add `helpers_test.go`: direct unit tests for `collectPage` (single page,
last page, `--all` walk, `--cursor` start) and the three ID resolvers,
including that a page URL is rejected for comment/attachment commands.
Docs (second review pass):
- `technical-design.md` §5 replaced the stale hand-maintained command table
(it listed `--no-color`, `comment add --format`, `attachment download
--pageID`, "comment is the only write") with a command-model summary that
points to the generated `docs/cli/` reference — no more parallel list to drift.
- §6.1 documents the raw-output exceptions (`version`, `attachment download
--output -`, `skill show`) rather than claiming output is uniformly JSON.
- `README.md` intro and feature bullet now describe a read **and write**
maintenance tool instead of "read/search/comment".
Co-Authored-By: Claude Opus 4.7 <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
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.
Addresses the code review of v0.2.0. All 5 findings were verified against the
code and fixed. #2/#3/#4 change the CLI's external contract — the next
release should be v0.3.0.
#1 — Security: ID resolver mis-resolution (high)
resolveIDonly ever extracted a page ID; passing a comment/attachment URLresolved to the page ID, so
comment delete <page-url>could delete the wholepage. Fixed:
urlrefnow extractsfocusedCommentIdfrom comment permalinks.resolveIDis split intoresolvePageID/resolveCommentID/resolveAttachmentID. A plain page URL given to a comment/attachment commandis rejected instead of silently acting on the page.
#2 —
--formatflag conflictThe body-format flag on
page create/updateandcomment add/updateisrenamed
--format→--body-format; it no longer shadows the global--format(json/table/ndjson) output flag.#3 — Non-JSON success output
config path/use-context/delete-context/init,auth login/logoutandskill install/uninstall/pathnow emit JSON vias.emit. Interactiveprompts moved to stderr; stdout is uniformly JSON (
versionstays plain text bydesign, matching the
--versionflag).#4 — Pagination
List commands now emit a
{items, next, has_more}envelope and accept--cursor, so an agent can page deterministically.outputtable/ndjsonrendering and
--fieldsprojection are envelope-aware.#5 — Stale docs
docs/technical-design.md,README.mdand the embedded Skill updated to theimplemented surface; the error model documents
conflict/ exit 11.Verification
gofmt/go vet/go test ./...green;make e2e44/44.urlreffocusedCommentId; resolver-safety (page URL rejected);list-envelope rendering/projection (
output); envelope + safety CLI tests.make docsregenerated; generator idempotent.page create --helpshows both--body-formatand global--format;comment delete <page-url>errorsCOMMENT_URL_NO_ID.🤖 Generated with Claude Code