Repository navigation
feat(channels): record and show auto- vs admin-approved channel proposals - #617
Conversation
…sals (#616) Red before the feature: - dbschema: channel_proposals.approved_by is added idempotently and approved rows without a value are classified from their timestamps (< 60s = auto, else admin); AssertReady does not require the column. - ingestor: auto-approve writes 'auto', admin approve 'admin'; reject, revoke and a resubmission clear it. - server: approvedBy is listed when the column exists (approved rows only), omitted on an old DB, and picked up by the schema healer. - frontend: Auto-approved filter, auto/admin badge and the &filter= deep link (unit + Playwright, incl. 390px). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…sals (#616) Schema (ingestor-owned, internal/dbschema): channel_proposals.approved_by TEXT NULL CHECK IN ('auto','admin'), added by an idempotent ALTER. In the same transaction approved rows without a value are classified from their timestamps (reviewed_at - created_at < 60s = auto, else admin); those values are inferred. A second run finds nothing and logs nothing. Not part of AssertReady: the column is optional for the server. Writes (cmd/ingestor): auto-approval inserts 'auto', an admin approval sets 'admin', reject/revoke and a resubmission after a revoke clear it. Reads (cmd/server stays read-only): a self-healing schemaFlag detects the column (warm probe + healer), ListProposals selects it only when the flag is set and reports it for approved rows only. Exposed as the named field Proposal.ApprovedBy (json approvedBy, omitempty). UI: auto/admin badge on approved rows, an Auto-approved filter (the approved list narrowed client-side) and #/channels?view=proposals&filter= as deep link. CSS variables only. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…w one (#616) The separate approved_by probe added one query to every schema-healer tick. Tests that close only db.conn leave their healer ticking on a dead connection for the rest of the binary (34 such healers in shard 3, the same on master), and TestGraphScore561_AllocationBudget counts allocations process-wide, so the extra query per tick pushed it over its budget in CI twice (352 and 350 allocs/op, budget 300) and once locally (384) while master passed the same shard. client_declared_regions and approved_by are now read by one query: a tick on a closed connection costs 2 allocs again (4 before), the same as master. TestDetectSchemaApprovedByAddsNoQueryToHealerTick pins that. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Rapport — CS-pve-agent3 PR#617 #616 — head 92d5e69Status: All requirements are implemented, each has a red-before/green-after test with mutants, and CI is green on 92d5e69. One CI regression from my first push (an allocation budget in Go shard 3) was root-caused and fixed in 92d5e69. Evidence labels: [T] automated test (red before, green after), [A] analysis or code reading, [K] checked by running it locally and observing the output. Commits: Requirements
Schema change and rollback
CI (per job, final head 92d5e69, run 40284942647)
Earlier failure, caused by this PR and fixed in 92d5e69. At head 257fa94, Go Server Tests (3/3) failed on Root cause:
Fix: the probe now shares the existing The rerun also showed Local runs
Remaining issues
|
Review — CS-Macmini PR#617 — head 92d5e69Dom: APPROVE med nits Independent, read-only review. Verified on Findings
N1 detail. N2 detail. Reproduced the rollback sequence on a migrated fixture: a v0.2.6-shaped Answers to the review pointsIdempotent ALTER + backfill in the ingestor, boundary 60000 ms, never overwrites. Correct. One behaviour worth recording: an Every write path sets/clears approved_by correctly. Verified by enumerating every statement that writes the table ( Also checked for a stale-value leak on the other exit: No per-tick query, and the server stays read-only. Correct, and the merge into the existing approvedBy is a named struct field, omitted on an old DB. Correct. Rollback: v0.2.6 on a database with the new column. Verified by actually running it, not only by reading. [K] Built the v0.2.6 server and migrate tool from the tag and pointed them at a migrated fixture whose The allocation-budget fix in 92d5e69 is a real fix, not a loosened budget. Confirmed. UI: badge, filter, deep link, CSS variables, 390px. All present and behaving. The badge renders only for the two known constants, gated by No behaviour change beyond the purpose. Checked the two places where existing strings were rerouted: the empty-list text now goes through Hygiene. Commit author and committer are Acceptance criteriaEvery criterion in the issue has a test. Red-before is real but of two different kinds, worth stating precisely: at the test-only commit
Tests I ran
The E2E ran against locally built CI, checked per job on MutantsReran the three requested, plus four of my own.
What I did not verify
|
Relates to #616
What
Records whether an approved shared channel was approved by the
autoApprovepolicy or by an administrator, and shows it in the admin review dialog (#/channels?view=proposals).Plan / milestones (single PR, small scope)
internal/dbschema). New nullable columnchannel_proposals.approved_by TEXT CHECK(approved_by IN ('auto','admin')), added by an idempotentALTER TABLE … ADD COLUMNwhen missing. In the same transaction, approved rows that have no value are classified from their timestamps:reviewed_at - created_at < 60000→'auto', otherwise'admin'. Values from before the migration are inferred; the ingestor writes them exactly from then on. A second run finds nothing to classify and logs nothing. The column is deliberately not part ofAssertReady. Like the table itself, it is optional for the server.cmd/ingestor). Auto-approval inserts'auto'. An admin approval sets'admin'. Reject, revoke and a resubmission (revoked → pending) clear it.cmd/server, still read-only). A new self-healingschemaFlag(hasProposalApprovedBy) is set by the warm probe and byhealSchemaFlags, so a server/ingestor boot race cannot hide the column.channelregistry.ListProposalsselects the column only when the flag is set, and reports it for approved rows only. It is exposed as the named fieldProposal.ApprovedBy(json:"approvedBy,omitempty"). Nomap[string]interface{}is added. OpenAPI text is updated.public/channel-proposals.js/.css). Approved rows get a smallauto/adminbadge. A new Auto-approved filter shows the approved list narrowed on the client toapprovedBy === 'auto'. The filter is reflected in the hash:#/channels?view=proposals&filter=auto|approved|rejected|revoked(pending keeps the plain link). Colors come from CSS variables only, and the layout is checked at 390px.Design notes
status='approved' AND approved_by IS NULL. In practice it is one-off for pre-migration rows. It also classifies rows that an older (rolled-back) ingestor approved without knowing the column. Values the ingestor wrote are never overwritten (there is a test for this).approved_byuntouched. The new ingestor clears it on its next transition of that row, and the reader masks it, because approvedBy is reported for approved rows only.?status=approvedand filters on the client. The approved list is bounded bymaxApproved.Schema change / rollout
SKIP_SNAPSHOTfor the next deploys.approvedBy. It picks the column up without a restart once the ingestor adds it.Tests (written first, red before / green after)
internal/dbschema: migration and backfill classification, including the 59 999 / 60 000 ms boundary; idempotency (no log, no overwrite); rows approved by an older ingestor; fresh and rebuilt tables; CHECK constraint;AssertReadydoes not require the column.cmd/ingestor: auto →'auto', admin →'admin', repeated approve keeps the value, reject/revoke/resubmit → NULL (including stale values).cmd/server: approvedBy is present when the column exists (approved rows only, stale value masked); it is omitted on an old DB; detectSchema and the healer latch the flag; production wiring inServer.channelProposals().test-channel-proposals.js: badge (known values only, never interpolated), filter mapping and client-side filter,parseFilter/proposalsHash, deep-link mount, hash updates on filter clicks, empty-list text.test-channel-proposals-e2e.js(real ingestor + server + Chromium): API reportsauto/admin; thefilter=autodeep link lists only the auto-approved channel with its badge; switching filters updates the hash; 390px fits without overflow.Mutation results and CI status are in the report comment.
Perf
channel_proposals.approved_byis read by the same query that already probesclient_declared_regions. A healer tick on a closed connection costs 2 allocs, the same as master; a separate query made it 4. That mattered: many tests leave healers ticking on closed connections, andTestGraphScore561_AllocationBudgetcounts allocations process-wide. The first version of this PR pushed it over budget in CI shard 3 (352/350 vs 300) while master passed.TestDetectSchemaApprovedByAddsNoQueryToHealerTicknow pins this.maxApprovedrows. None of this is on a hot path.Customizer
There are no new configurable values. The 60 s inference window is a one-off migration constant, not a setting.
🤖 Generated with Claude Code