mr edit --title and a new issue edit: catch the CLI up to the PATCH routes (#7) #1

• 
  @ogirardot opened this merge request · Sep 7, 2026

  Closes #7.

  The server has shipped PATCH /merge-requests/{n} and PATCH /issues/{n} since
  rickub/rickub!112; the CLI half was dropped when cli/ left the monorepo. This
  re-authors it.

  What changed

  	before	after
  mr edit	--body only, via the old POST .../body	--title and/or --body, one PATCH
  issue edit	did not exist	--title and/or --body, one PATCH

  • internal/api: UpdateMergeRequest / UpdateIssue, with MergeRequestPatch /
    IssuePatch whose fields are pointers. A flag the user did not pass is
    absent from the JSON rather than sent as "" — an empty title trips the API's
    title_required, and an empty body would blank a description nobody asked to
    blank. This is the load-bearing detail and the tests pin it.
  • cmd/edit.go: one flag resolver shared by both commands, for the same reason
    cmd/body.go exists. -b - still reads stdin; a blank body is still refused
    locally (#94); a blank title is refused too; passing neither flag exits
    non-zero and sends no request rather than collecting the server's
    400 empty_update.
  • The success line reports what the server echoed back (new title, stored byte
    count, or both).
  • UpdateMergeRequestBody is removed — the PATCH route supersedes it and nothing
    else called it.
  • README.md inventory updated.

  Acceptance criteria (#7)

  • mr edit <n> --title … renames and leaves the description untouched
  • mr edit <n> -b - still replaces only the description
  • both flags in one invocation → one PATCH carrying both
  • issue edit <n> --title … [--body …] exists
  • neither flag → non-zero exit, no request sent (asserted as zero requests)
  • --title "" refused locally; --body ""/blank stdin refused per #94
  • wire-level tests assert method, path, and that an unpassed flag is absent
    from the payload
  • the server's uniform 404 surfaces as a clean CLI error

  Still to do elsewhere

  Point 5 of #7 — rickub/rickub's FEATURES.md CLI inventory line and the API
  bullet saying the CLI has not caught up — lives in the other repo and is not in
  this MR.

  🤖 Generated with Claude Code

  https://claude.ai/code/session_01KPPhRdTiUiiSvLjthotkT1

• 
  @ogirardot · Sep 7, 2026

  Reviewed at effort high; five findings, all low severity, no correctness bug.
  Four are fixed in 87b529c, one is declined with a reason, and one gap the review
  surfaced is filed rather than smuggled in.

  Fixed

  1. resolveEditFields reused requireBody verbatim, but its messages were
     written for a required-body comment. mr edit 12 -b "" answered "--body is
     required and must not be blank" — false, --title alone is valid — and an
     empty stdin answered "refusing to post an empty comment" while editing a
     description. The user hits these precisely when the guard fires and is told
     the wrong thing to do. The stdin parsing stays shared (that shared parser is
     what #94 was actually about); only the refusal sentence is local, and it now
     names the real remedy: omit --body.
  2. editSummary trusted the server echo blindly. Client.do returns without
     unmarshalling when a response body is empty, so a 204 or a payload without
     title would print Updated #12: title now "" after a successful rename.
     It now falls back to the sent value and marks it (not echoed back).
  3. mr edit abc -b - <<'EOF' … EOF drained stdin before parsing the number, so
     the heredoc vanished into "invalid merge-request number" — #94's failure mode
     by another route. The number is validated first; the test asserts stdin is
     still unread afterwards.
  4. Stale comment in TestPREditSendsBodyAndReportsLength describing the body
     route this branch deletes.

  Declined

  IssuePatch and MergeRequestPatch are byte-identical today, and the review
  argued the repo's own DRY case from cmd/body.go. Not collapsing them:
  internal/api/models.go exists to mirror the API's schemas one-for-one, and these
  are two independent server schemas that happen to coincide. Sharing one type would
  couple the CLI's view of issues to its view of merge requests — the next field
  either side gains would have to be added to both regardless. The DRY case in
  body.go was about two behaviours diverging, which is a different thing.

  Filed, not fixed here

  No CLI path can clear a description: issue create -b "" is accepted but every
  edit path refuses a blank body, so a description added by mistake is permanent.
  The client can already do it (IssuePatch{Body: &""} → "body":"", pinned in
  endpoints_patch_test.go) — the CLI has no way to ask. That is a real gap, but
  #7's acceptance criteria explicitly require a blank --body to be refused, so
  adding a bypass here would contradict the issue being closed. Filed as #12 with a
  suggested --clear-body shape.

  Verified live against rickub.com, not just in tests: title-only leaves the
  description untouched, -b - leaves the title untouched, both flags produce one
  PATCH, and the no-flag / blank-title / blank-body / bad-number cases all exit
  non-zero with zero requests sent.

Sign in to comment.