amadan
console: every pending tally shows what execution will require
PR #258 review, I3. Execution counts confirmations from CURRENT members against max(stored quorum, current rule), but the card, the CLI list and the audit rows all reported the raw row — so a change could sit at "2/2 confirmed" beside a live Confirm button and refuse every time it was pressed, which is worse than showing no card. currentQuorum splits into a pure requiredQuorum(pc, pipe), shared by the gate and the display surfaces so a card can never promise a quorum the gate will not accept. pendingTally pairs it with the member-counted confirmations, best-effort (a failed read degrades to the raw row rather than costing the page; execution re-checks for real anyway). The card and the CLI list each take one pipeline read for the whole listing; the confirm door uses the same tally for its flash, its audit row, and for deciding whether to attempt execution at all — so a member reads the number that actually decides. From the gate onwards executePending narrows pc.Confirms to the counted set, so no audit row can credit a quorum that was never met. Riders from the same review: PromoteResult.From takes the channel-name fallback on the no-op path, and the pending-change "no such" error gets the colon its two siblings have.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
7 files changed,
+221
−21
internal/console/pending.go+1 −1internal/console/pendingflow_test.go+97 −0internal/console/pendinghandlers.go+86 −15internal/console/pipelinepage.go+14 −2internal/console/server.go+4 −2internal/release/pipeline_test.go+14 −0internal/release/release.go+5 −1
diff --git a/internal/console/pending.go b/internal/console/pending.go| index c0a60f3e1..30b0ee883 100644 |
| --- a/internal/console/pending.go |
| +++ b/internal/console/pending.go |
| @@ -168,7 +168,7 @@ func (s *Store) ConfirmChange(id, address string, now time.Time) (*PendingChange |
| return nil, false, err |
| } |
| if pc == nil { |
| - return nil, false, fmt.Errorf("%w %q", ErrNoPendingChange, id) |
| + return nil, false, fmt.Errorf("%w: %q", ErrNoPendingChange, id) |
| } |
| if now.Unix() >= pc.ExpiresAt.Unix() { |
| return nil, false, fmt.Errorf("%w: %q", ErrPendingChangeExpired, id) |
diff --git a/internal/console/pendingflow_test.go b/internal/console/pendingflow_test.go| index 9a355d20a..a300fcf63 100644 |
| --- a/internal/console/pendingflow_test.go |
| +++ b/internal/console/pendingflow_test.go |
| @@ -1017,6 +1017,103 @@ func TestExecutionMeetsTheCURRENTQuorum(t *testing.T) { |
| } |
| } |
| +// TestPendingTalliesShowWhatExecutionRequires: the card, the CLI list and the |
| +// confirm flash must not promise a quorum execution will refuse. With the |
| +// rule raised from 2 to 3 underneath a live proposal, every surface has to |
| +// read 2 of 3 — a change sitting at "2 of 2" beside a Confirm button that |
| +// refuses every time it is pressed is worse than no card at all (PR #258 |
| +// review, I3). And a confirmation from a departed member has to stop counting |
| +// on those same surfaces, for the same reason. |
| +func TestPendingTalliesShowWhatExecutionRequires(t *testing.T) { |
| + fx, c, csrf := memberFixture(t) // paul owns, kim is a member |
| + if err := fx.st.AddMember(OpsAccountID, "alice@keymail.dev", RoleMember, time.Now()); err != nil { |
| + t.Fatal(err) |
| + } |
| + pipelinedApp(t, fx, "widget", "abc1234", |
| + release.PipelineChannel{Name: "edge"}, |
| + release.PipelineChannel{Name: "production", Rules: release.ChannelRules{PromoteApprovals: 2}}) |
| + |
| + if status, loc, body := postGated(t, fx, c, fx.u("/apps/widget/promote"), |
| + url.Values{"csrf": {csrf}, "channel": {"production"}, "version": {"abc1234"}, "confirmed": {"1"}}); status != http.StatusSeeOther { |
| + t.Fatalf("owner proposal = %d -> %q:\n%s", status, loc, body) |
| + } |
| + // Before the tightening the card reads 1/2 — pending.html's own format, |
| + // asserted here so the checks after the tightening are known to be |
| + // looking at the card and not at some flash that happens to say "of". |
| + _, _, body := get(t, c, fx.u("/apps/widget")) |
| + if !strings.Contains(body, "1/2 confirmed") { |
| + t.Fatalf("card does not show 1/2 confirmed before the tightening:\n%s", body) |
| + } |
| + |
| + setPipeline(t, fx, "widget", |
| + release.PipelineChannel{Name: "edge"}, |
| + release.PipelineChannel{Name: "production", Rules: release.ChannelRules{PromoteApprovals: 3}}) |
| + |
| + // The card. |
| + _, _, body = get(t, c, fx.u("/apps/widget")) |
| + if !strings.Contains(body, "1/3 confirmed") { |
| + t.Errorf("card shows the stored quorum, not the one execution now requires:\n%s", body) |
| + } |
| + if strings.Contains(body, "1/2 confirmed") { |
| + t.Errorf("card still shows the superseded 1/2:\n%s", body) |
| + } |
| + // The CLI list. |
| + token := cliToken(t, fx, "paul@keymail.dev", "code-tally-list") |
| + status, out := getJSON(t, fx.ts.URL+"/api/cli/apps/bab/widget/pending", token) |
| + if status != http.StatusOK { |
| + t.Fatalf("pending list = %d %v", status, out) |
| + } |
| + list, _ := out["pending"].([]any) |
| + if len(list) != 1 { |
| + t.Fatalf("pending list = %v, want one row", out["pending"]) |
| + } |
| + row, _ := list[0].(map[string]any) |
| + if num(t, row, "needed") != 3 || num(t, row, "confirmed") != 1 { |
| + t.Errorf("CLI row = %v, want 1 of 3", row) |
| + } |
| + |
| + // The confirm flash, and the audit row behind it: kim's confirmation |
| + // takes it to 2 of 3, not "2 of 2, executed". |
| + pending, err := fx.st.PendingChangesFor(OpsAccountID, "widget", time.Now()) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + kim := newBrowser(t) |
| + fx.setUser("kim@keymail.dev") |
| + signUp(t, fx, kim, "code-kim-tally") |
| + kimCSRF := sessionToken(t, fx, kim) |
| + if status, loc, body := postGated(t, fx, kim, |
| + fx.u("/apps/widget/pending/"+pending[0].ID+"/confirm"), url.Values{"csrf": {kimCSRF}}); status != http.StatusSeeOther { |
| + t.Fatalf("second confirmation = %d -> %q:\n%s", status, loc, body) |
| + } |
| + _, _, kimBody := get(t, kim, fx.u("/apps/widget")) |
| + if !strings.Contains(kimBody, "2/3 confirmed") { |
| + t.Errorf("after the second confirmation the card does not read 2/3:\n%s", kimBody) |
| + } |
| + events, err := fx.st.Events(OpsAccountID, "widget", 10) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + found := false |
| + for _, e := range events { |
| + if e.Action == "confirmed" && strings.Contains(e.Detail, "2 of 3 confirmations") { |
| + found = true |
| + } |
| + } |
| + if !found { |
| + t.Errorf("no audit row recording 2 of 3 confirmations: %+v", events) |
| + } |
| + |
| + // And a departed member's row stops counting on the surfaces too. |
| + if err := fx.st.RemoveMember(OpsAccountID, "kim@keymail.dev"); err != nil { |
| + t.Fatal(err) |
| + } |
| + _, _, body = get(t, c, fx.u("/apps/widget")) |
| + if !strings.Contains(body, "1/3 confirmed") { |
| + t.Errorf("card counts a departed member's confirmation:\n%s", body) |
| + } |
| +} |
| + |
| // TestLoosenedRuleDoesNotLowerAStoredQuorum: the ratchet only goes one way. |
| // A proposal made at 3 approvals is not executable at 2 because somebody |
| // lowered the rule afterwards — the confirmers agreed to a change that needed |
diff --git a/internal/console/pendinghandlers.go b/internal/console/pendinghandlers.go| index bba238d84..cbf38df44 100644 |
| --- a/internal/console/pendinghandlers.go |
| +++ b/internal/console/pendinghandlers.go |
| @@ -322,6 +322,12 @@ func (s *Server) executePending(ctx context.Context, acct *Account, app, actor s |
| pc.Kind, pc.ID, len(counted), required, len(pc.Confirms), pc.Quorum) |
| return "", fmt.Errorf("that change has %d of %d confirmations", len(counted), required) |
| } |
| + // From the gate onwards, this change's confirmations ARE the counted |
| + // ones: every audit row below reports "%d confirmations" from here, and a |
| + // trail that credited a departed member's row would record a quorum that |
| + // was never met (PR #258 review, I3). The rows themselves are untouched |
| + // in the database — this narrows only what the EXECUTION claims it had. |
| + pc.Confirms = counted |
| if s.cfg.Key == nil { |
| return "", errors.New("no signing key on this host — the console is running read-only") |
| } |
| @@ -367,6 +373,25 @@ func (s *Server) executePending(ctx context.Context, acct *Account, app, actor s |
| // rule that moved at all invalidates the proposal before any quorum question |
| // is asked, so max(stored, current) could only ever equal stored. |
| func (s *Server) currentQuorum(ctx context.Context, acct *Account, app string, pc *PendingChange) (int, error) { |
| + pipe, err := release.GetPipeline(ctx, s.cfg.Releases, s.cfg.ReleasePub, acct.Sqid, app) |
| + if err != nil { |
| + // Fail closed, like every other unreadable-pipeline path: a proposal |
| + // whose governing rules cannot be read is not executable. |
| + log.Printf("console: execute %s %s/%s: read pipeline: %v", pc.Kind, acct.Sqid, app, err) |
| + return 0, errors.New("could not read the pipeline") |
| + } |
| + return requiredQuorum(pc, pipe) |
| +} |
| + |
| +// requiredQuorum is currentQuorum's pure half: max(the number stored on the |
| +// proposal, the target channel's rule in pipe). Shared with the DISPLAY |
| +// surfaces (pendingChangeViews, handleAPIPending) so a card can never promise |
| +// a quorum execution will not accept — one rule, one place. |
| +// |
| +// A nil pipe (legacy app, or a display caller whose best-effort read failed) |
| +// carries no rules, so the answer is the stored quorum: the pre-existing |
| +// behaviour, and the safe direction for a display. |
| +func requiredQuorum(pc *PendingChange, pipe *release.Pipeline) (int, error) { |
| var channel string |
| var rule func(release.ChannelRules) int |
| switch pc.Kind { |
| @@ -388,19 +413,47 @@ func (s *Server) currentQuorum(ctx context.Context, acct *Account, app string, p |
| default: |
| return pc.Quorum, nil |
| } |
| - pipe, err := release.GetPipeline(ctx, s.cfg.Releases, s.cfg.ReleasePub, acct.Sqid, app) |
| - if err != nil { |
| - // Fail closed, like every other unreadable-pipeline path: a proposal |
| - // whose governing rules cannot be read is not executable. |
| - log.Printf("console: execute %s %s/%s: read pipeline: %v", pc.Kind, acct.Sqid, app, err) |
| - return 0, errors.New("could not read the pipeline") |
| - } |
| if n := rule(channelRules(pipe, channel)); n > pc.Quorum { |
| return n, nil |
| } |
| return pc.Quorum, nil |
| } |
| +// pendingTally is the two numbers every SURFACE shows for one pending change |
| +// — the card, the CLI list, the confirm flash and the audit row — computed |
| +// the way execution counts them: confirmations from current members, against |
| +// max(stored quorum, current rule). Best-effort by construction: a failed |
| +// membership read or an unreadable payload degrades to the raw row rather |
| +// than costing the page, and execution re-checks both for real anyway. |
| +// |
| +// The gap between this and len(pc.Confirms) is deliberately not hidden: a |
| +// change stuck because someone left the account shows fewer confirmations |
| +// than it has rows, which is the honest explanation for why it is stuck. |
| +func (s *Server) pendingTally(pc PendingChange, pipe *release.Pipeline) (confirmed, needed int) { |
| + confirmed, needed = len(pc.Confirms), pc.Quorum |
| + if counted, err := s.store.MemberConfirms(pc.AccountID, pc.ID); err != nil { |
| + log.Printf("console: pending tally %s: member confirms: %v", pc.ID, err) |
| + } else { |
| + confirmed = len(counted) |
| + } |
| + if n, err := requiredQuorum(&pc, pipe); err == nil && n > needed { |
| + needed = n |
| + } |
| + return confirmed, needed |
| +} |
| + |
| +// pendingPipeline is the display surfaces' best-effort pipeline read: one per |
| +// listing, never fatal. A WRITE door reads its own copy and refuses on error |
| +// (currentQuorum); a card that cannot read the pipeline still renders. |
| +func (s *Server) pendingPipeline(ctx context.Context, acct *Account, app string) *release.Pipeline { |
| + pipe, err := release.GetPipeline(ctx, s.cfg.Releases, s.cfg.ReleasePub, acct.Sqid, app) |
| + if err != nil { |
| + log.Printf("console: pending tally %s/%s: read pipeline: %v", acct.Sqid, app, err) |
| + return nil |
| + } |
| + return pipe |
| +} |
| + |
| // staleProposal is the one refusal every kind shares: the world the proposal |
| // pinned is not the world any more. |
| func staleProposal(detail string) error { |
| @@ -612,20 +665,29 @@ func (s *Server) handlePendingConfirm(w http.ResponseWriter, r *http.Request) { |
| s.setFlash(w, kind, msg) |
| http.Redirect(w, r, appPath(acct, app), http.StatusSeeOther) |
| } |
| - pc, met, err := s.store.ConfirmChange(pc.ID, addr, time.Now()) |
| + pc, _, err := s.store.ConfirmChange(pc.ID, addr, time.Now()) |
| if err != nil { |
| s.refuseConfirm(w, r, err) |
| return |
| } |
| - if !met { |
| + ctx, cancel := context.WithTimeout(r.Context(), pendingTimeout) |
| + defer cancel() |
| + // ConfirmChange's own `met` counts ROWS against the STORED quorum, which |
| + // is not what execution requires (see executePending): a change whose |
| + // rule was raised, or whose confirmations include somebody who has since |
| + // left the account, is not met however many rows it carries. The tally |
| + // shown and audited here is the one that decides, so the flash a member |
| + // gets and the row the trail keeps say the same thing the gate does. |
| + // Deciding on it also keeps the not-met path a plain progress message |
| + // rather than an execution that refuses a beat later. |
| + confirmed, needed := s.pendingTally(*pc, s.pendingPipeline(ctx, acct, app)) |
| + if confirmed < needed { |
| s.audit(acct, app, addr, "confirmed", fmt.Sprintf("%s (%d of %d confirmations)", |
| - pendingSummary(*pc), len(pc.Confirms), pc.Quorum)) |
| + pendingSummary(*pc), confirmed, needed)) |
| back("ok", fmt.Sprintf("Confirmed: %s. %d of %d confirmations.", |
| - pendingSummary(*pc), len(pc.Confirms), pc.Quorum)) |
| + pendingSummary(*pc), confirmed, needed)) |
| return |
| } |
| - ctx, cancel := context.WithTimeout(r.Context(), pendingTimeout) |
| - defer cancel() |
| msg, err := s.executePending(ctx, acct, app, addr, pc) |
| if err != nil { |
| back("err", "Could not carry that out: "+err.Error()) |
| @@ -712,14 +774,23 @@ func (s *Server) handleAPIPending(w http.ResponseWriter, r *http.Request) { |
| jsonError(w, http.StatusInternalServerError, "could not read pending changes") |
| return |
| } |
| + // The same tally the card shows, from one pipeline read: "confirmed" and |
| + // "needed" are what EXECUTION will count, so `carlos pipeline` cannot |
| + // print a change as ready when confirming it would refuse (PR #258 |
| + // review, I3). |
| + var pipe *release.Pipeline |
| + if len(list) > 0 { |
| + pipe = s.pendingPipeline(r.Context(), acct, app) |
| + } |
| out := make([]map[string]any, 0, len(list)) |
| for _, pc := range list { |
| + confirmed, needed := s.pendingTally(pc, pipe) |
| out = append(out, map[string]any{ |
| "id": pc.ID, |
| "kind": pc.Kind, |
| "summary": pendingSummary(pc), |
| - "confirmed": len(pc.Confirms), |
| - "needed": pc.Quorum, |
| + "confirmed": confirmed, |
| + "needed": needed, |
| }) |
| } |
| writeJSON(w, http.StatusOK, map[string]any{"pending": out}) |
diff --git a/internal/console/pipelinepage.go b/internal/console/pipelinepage.go| index 402334976..06be92230 100644 |
| --- a/internal/console/pipelinepage.go |
| +++ b/internal/console/pipelinepage.go |
| @@ -46,12 +46,24 @@ type PendingChangeView struct { |
| // pendingChangeViews folds a store listing into the card's rows, oldest |
| // first — PendingChangesFor's own order, unchanged, so the card and the |
| // CLI's `carlos pipeline` listing (Task 10) read the same queue the same way. |
| -func pendingChangeViews(pcs []PendingChange) []PendingChangeView { |
| +// |
| +// The two numbers are the ones EXECUTION will use, not the ones the row was |
| +// written with (PR #258 review, I3): Needed is max(stored quorum, the target |
| +// channel's current rule) and Confirmed counts only confirmations from |
| +// current members. Showing the raw row instead let a change sit at "2 of 2" |
| +// with a live Confirm button and refuse every time it was pressed — a card |
| +// that says a change is ready when it is not is worse than no card. |
| +// |
| +// pipe is the app's pipeline, read ONCE by the caller for the whole listing |
| +// (nil is fine — a legacy app, or a read that failed: the tally then degrades |
| +// to the stored quorum, which is exactly what it was before this existed). |
| +func (s *Server) pendingChangeViews(pcs []PendingChange, pipe *release.Pipeline) []PendingChangeView { |
| out := make([]PendingChangeView, len(pcs)) |
| for i, pc := range pcs { |
| + confirmed, needed := s.pendingTally(pc, pipe) |
| out[i] = PendingChangeView{ |
| ID: pc.ID, Kind: pc.Kind, Summary: pendingSummary(pc), |
| - Confirmed: len(pc.Confirms), Needed: pc.Quorum, |
| + Confirmed: confirmed, Needed: needed, |
| } |
| } |
| return out |
diff --git a/internal/console/server.go b/internal/console/server.go| index 04e8eb9e8..e4a9b9493 100644 |
| --- a/internal/console/server.go |
| +++ b/internal/console/server.go |
| @@ -2216,8 +2216,10 @@ func (s *Server) handleApp(w http.ResponseWriter, r *http.Request) { |
| } |
| if pcs, err := s.store.PendingChangesFor(acct.ID, app, time.Now()); err != nil { |
| log.Printf("console: pending changes %s/%s: %v", acct.Sqid, app, err) |
| - } else { |
| - p.Pending = pendingChangeViews(pcs) |
| + } else if len(pcs) > 0 { |
| + // One pipeline read for the whole card: the rows show the quorum |
| + // EXECUTION will require, not the one they were proposed under. |
| + p.Pending = s.pendingChangeViews(pcs, s.pendingPipeline(r.Context(), acct, app)) |
| } |
| s.render(w, http.StatusOK, "app_overview", p) |
| } |
diff --git a/internal/release/pipeline_test.go b/internal/release/pipeline_test.go| index 85783da15..0bbe09fb8 100644 |
| --- a/internal/release/pipeline_test.go |
| +++ b/internal/release/pipeline_test.go |
| @@ -272,6 +272,20 @@ func TestPipelinePromoteNamesTheChannelAsTo(t *testing.T) { |
| if got := p.Audit[len(p.Audit)-1]; got.To != "production" || got.From != "edge" { |
| t.Errorf("promotion record = %+v, want from edge to production", got) |
| } |
| + |
| + // The repeat (a no-op) names the channel on BOTH sides: the build is on |
| + // production, which is exactly what makes this a repeat, so reporting "" |
| + // for where it sits would be the same empty answer one rung down. |
| + again, err := PromoteOnLadder(ctx, s, priv, "bae", "web", "production", "v1", PromoteOptions{}) |
| + if err != nil { |
| + t.Fatalf("repeat promote: %v", err) |
| + } |
| + if !again.NoOp { |
| + t.Errorf("repeat promote = %+v, want NoOp", again) |
| + } |
| + if again.From != "production" || again.To != "production" { |
| + t.Errorf("repeat promote from/to = %q/%q, want production/production", again.From, again.To) |
| + } |
| } |
| // The frozen rungs are untouched: a legacy promote still reports the RUNG, |
diff --git a/internal/release/release.go b/internal/release/release.go| index a470e04d8..b694c2fca 100644 |
| --- a/internal/release/release.go |
| +++ b/internal/release/release.go |
| @@ -847,7 +847,11 @@ func (sg Signer) PromoteOnLadder(ctx context.Context, s Store, acct, app, channe |
| if cur, _, err := s.Get(ctx, ChannelKey(acct, app, channel)); err == nil { |
| if p, verr := verifyPointer(pub, cur, app, channel); verr == nil && p.Version == version { |
| res.NoOp = true |
| - res.From, _ = Rung(channel) |
| + // The same fallback res.To takes above, for the same reason: the |
| + // build IS on this channel (that is what makes this a repeat), so |
| + // an app-defined channel names itself here too rather than |
| + // reporting "" for where the build already sits. |
| + res.From = res.To |
| // A repeat still reports the tag — and cuts it if a previous |
| // attempt's write failed, so re-running the same command heals a |
| // stable release that ended up nameless. |