amadan
fix: a failed link mint stops blaming the person's role choice
Issue oficina/sheets #5, the fourth of the five Fable findings left open on core. Every failure path out of `createLink` redirected with one notice, "A link can only view or comment, never edit". The token generator failing, the insert failing, no database at all — each was reported to the person as though they had picked an invalid role. They retry the identical form, get the identical sentence, and have no way to tell that the thing they are being blamed for is not the thing that went wrong. The role rejection is now a sentinel error and the handler tells it apart. It is the one failure the person asking can act on, so it keeps its own notice; everything else takes `a.fail`, the same 500 that `handleLinkRevoke` two functions below already gives for a broken write. **This needed no new copy, which is worth noting because the issue assumed it would.** Routing the internal case to the existing error path answers it correctly and adds nothing to read. A new sentence would have been a worse fix that looked like a more thorough one. The test drops the `share_links` table to fail the insert for real, rather than mocking the sealer or the DB — the same path a disk or migration fault would take. Its sibling asserts an editor link is still refused by name, because a fix that stopped distinguishing failures in the other direction would also have made the first test pass. make test green, including -race. 🤖 Authored by Claude Opus 5, directed by Paul.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0114wZuXnD6aGcDCzL39NpXD
3 files changed,
+72
−2
internal/sheets/handlers_link.go+11 −1internal/sheets/sharelink.go+11 −1internal/sheets/sharelink_failure_test.go+50 −0
CI passed — run details
Intent
Attributed to this commit: 205 g CO₂e (23477654 tokens). This is an attribution, not a measurement — usage counted from this commit's own marker forward to the next one on fix/link-mint-failure-not-role, never backward from it. A turn that produced more than one commit books all of its tokens to the last one, and a concurrent session on this branch can add to the same figure. Some of it was priced using the shared default rate, because no model-specific factor row exists yet for at least one model involved. The Carbon view has the repo total, which is never derived from this attribution.
diff --git a/internal/sheets/handlers_link.go b/internal/sheets/handlers_link.go| index a7750dc..d93ec08 100644 |
| --- a/internal/sheets/handlers_link.go |
| +++ b/internal/sheets/handlers_link.go |
| @@ -1,6 +1,7 @@ |
| package sheets |
| import ( |
| + "errors" |
| "net/http" |
| "strconv" |
| @@ -150,10 +151,19 @@ func (a *App) handleLinkCreate(w http.ResponseWriter, r *http.Request) { |
| } |
| role := SheetRole(r.FormValue("role")) |
| token, err := a.createLink(s.ID, role, viewer) |
| - if err != nil { |
| + if errors.Is(err, errBadLinkRole) { |
| + // The only failure the person can act on, and the only one they |
| + // are told about in these terms. |
| a.shareRedirect(w, r, s, "bad-link-role") |
| return |
| } |
| + if err != nil { |
| + // Ours, not theirs: the same 500 every other broken write on |
| + // this screen gives, rather than a sentence blaming a role |
| + // choice that was fine. |
| + a.fail(w, r, err) |
| + return |
| + } |
| a.renderShare(w, r, s, viewer, "link", token) |
| } |
diff --git a/internal/sheets/sharelink.go b/internal/sheets/sharelink.go| index 1c88967..0cbe327 100644 |
| --- a/internal/sheets/sharelink.go |
| +++ b/internal/sheets/sharelink.go |
| @@ -54,6 +54,16 @@ func hashLinkToken(token string) string { |
| return hex.EncodeToString(sum[:]) |
| } |
| +// errBadLinkRole is the one createLink failure the person asking can do |
| +// anything about. |
| +// |
| +// It is a sentinel rather than an anonymous error because the handler |
| +// has to tell it apart from the rest. Every other way this function |
| +// fails — the token generator, the insert, no database at all — is ours |
| +// and not theirs, and reporting those as a rejected role choice tells |
| +// somebody to fix the one thing that was not broken. |
| +var errBadLinkRole = errors.New("sheets: a link can only view or comment, never edit") |
| + |
| // createLink mints a link for a sheet and returns the token, which the |
| // caller must show once and then forget. |
| func (a *App) createLink(sheetID uint64, role SheetRole, by Member) (string, error) { |
| @@ -64,7 +74,7 @@ func (a *App) createLink(sheetID uint64, role SheetRole, by Member) (string, err |
| // Named rather than silently downgraded: somebody asking for an |
| // editor link has a expectation this refuses, and refusing |
| // quietly would leave them thinking they made one. |
| - return "", errors.New("sheets: a link can only view or comment, never edit") |
| + return "", errBadLinkRole |
| } |
| token, hash, err := newLinkToken() |
| if err != nil { |
diff --git a/internal/sheets/sharelink_failure_test.go b/internal/sheets/sharelink_failure_test.go| new file mode 100644 |
| index 0000000..bb17739 |
| --- /dev/null |
| +++ b/internal/sheets/sharelink_failure_test.go |
| @@ -0,0 +1,50 @@ |
| +package sheets |
| + |
| +import ( |
| + "net/http" |
| + "net/url" |
| + "strings" |
| + "testing" |
| +) |
| + |
| +// Refusing an editor link is a real refusal with a real reason, and the |
| +// person can act on it by picking another role. It keeps its own |
| +// notice. |
| +func TestAnEditorLinkIsRefusedByName(t *testing.T) { |
| + app, cookies := signIn(t) |
| + s, _ := newSheet(t, app, cookies, "Budget") |
| + |
| + form := url.Values{"role": {string(SheetEditor)}} |
| + rec := do(t, app, as(postForm(sharePath(s)+"/link", form), cookies)) |
| + if loc := rec.Header().Get("Location"); !strings.Contains(loc, "bad-link-role") { |
| + t.Fatalf("asking for an editor link = %q, want the role refusal", loc) |
| + } |
| +} |
| + |
| +// Everything else that can go wrong inside createLink — the token |
| +// generator, the insert, a missing database — was reported with that |
| +// same notice. So an infrastructure fault was presented to the person |
| +// as their own input mistake: they retry the identical form, get the |
| +// identical sentence, and have no way to tell that the thing they are |
| +// being blamed for is not the thing that went wrong. |
| +func TestAFailedLinkMintDoesNotBlameTheRole(t *testing.T) { |
| + app, cookies := signIn(t) |
| + s, _ := newSheet(t, app, cookies, "Budget") |
| + |
| + // A real failure rather than a mocked one: with the table gone the |
| + // insert fails inside createLink, past the role check, on the same |
| + // path a disk or migration fault would take. |
| + if err := app.db.Exec("DROP TABLE share_links").Error; err != nil { |
| + t.Fatalf("drop share_links: %v", err) |
| + } |
| + |
| + form := url.Values{"role": {string(SheetViewer)}} |
| + rec := do(t, app, as(postForm(sharePath(s)+"/link", form), cookies)) |
| + if loc := rec.Header().Get("Location"); strings.Contains(loc, "bad-link-role") { |
| + t.Fatalf("a database failure was reported as the person's role choice: %q", loc) |
| + } |
| + if rec.Code != http.StatusInternalServerError { |
| + t.Fatalf("a failed mint = %d, want 500 — the same answer every other "+ |
| + "broken write on this screen gives", rec.Code) |
| + } |
| +} |