Skip to content

D1: server correctness bundle — C7 + C20a#307

Merged
mfwolffe merged 4 commits into
trunkfrom
d1/server-correctness-bundle
May 17, 2026
Merged

D1: server correctness bundle — C7 + C20a#307
mfwolffe merged 4 commits into
trunkfrom
d1/server-correctness-bundle

Conversation

@espadonne
Copy link
Copy Markdown
Contributor

Summary

Two CRIT server-side findings from the C-audit, bundled because they share an envelope-shape concern (PATCH/POST responses lying about what they did).

C7 — PATCH /repos silently accepted rename and returned 200 unchanged

Root cause: repoPatchRequest had no Name field. Go's default JSON decoder silently skipped the unknown field, the handler did nothing (every other validated field was nil), and the response was the unchanged repo. The CLI took 200 + repo object as success and rendered "Renamed to ".

Worse, the CLI then overwrote the local git origin to point at the renamed-to URL — see CX2 on the CLI side (already shipped). C7 is the upstream cause.

Fix: Add Name to the request struct and refuse with 422 ("renaming via REST is not yet supported"). Until rename is implemented as a real feature, refusal is correct UX.

C20a — issueResponse envelope missing assignees

Root cause: After S63 wired POST /issues to process assignees[], the response envelope still didn't carry them. The CLI sent the assignees, the server attached them to the issue row, but the response said assignees: null — CLI then couldn't render or verify.

Fix: Add Assignees []userEnvelope to issueResponse (always populated, empty slice when none — gh-compat). New helper assigneeEnvelopesFor(ctx, issueID) mirrors labelEnvelopesFor's pattern. All 4 presentIssue callsites (list, single GET, create, PATCH-reload) updated to pass the resolved slice.

Test plan

  • TestRepos_PatchRejectsRename — regression for C7: PATCH {"name":"..."} returns 422 with the explanatory message.
  • TestIssues_CreateWithLabelsAssigneesMilestone — extended to assert created.Assignees populated with the expected user (was only DB-side before).
  • apiIssue test struct gains Assignees []apiUser.
  • Full TestIssues + TestRepos + TestLabels suites still green against the integration DB.

Notes

C20a unblocks the C20b CLI follow-up (CLI should defensively compare sent vs returned assignees). Out of scope for this PR; will land separately in D3d-tail.

C7 is the upstream half of CX2 (CLI-side already merged). After this lands, CLI can stop showing "Renamed to old-name" success lines.

@mfwolffe mfwolffe merged commit 27f3e0c into trunk May 17, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants