Skip to content

feat(channels): record and show auto- vs admin-approved channel proposals - #617

Merged
dborup merged 3 commits into
masterfrom
feat-616-proposal-approved-by
Oct 26, 2026
Merged

dborup merged 3 commits into
masterfrom
feat-616-proposal-approved-by

Conversation

@dborup-agent

@dborup-agent dborup-agent commented Oct 26, 2026 •

Copy link
Copy Markdown
Collaborator

Relates to #616

What

Records whether an approved shared channel was approved by the autoApprove policy or by an administrator, and shows it in the admin review dialog (#/channels?view=proposals).

Plan / milestones (single PR, small scope)

  1. Schema (ingestor-owned, internal/dbschema). New nullable column channel_proposals.approved_by TEXT CHECK(approved_by IN ('auto','admin')), added by an idempotent ALTER TABLE … ADD COLUMN when 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 of AssertReady. Like the table itself, it is optional for the server.
  2. Writes (cmd/ingestor). Auto-approval inserts 'auto'. An admin approval sets 'admin'. Reject, revoke and a resubmission (revoked → pending) clear it.
  3. Reads (cmd/server, still read-only). A new self-healing schemaFlag (hasProposalApprovedBy) is set by the warm probe and by healSchemaFlags, so a server/ingestor boot race cannot hide the column. channelregistry.ListProposals selects the column only when the flag is set, and reports it for approved rows only. It is exposed as the named field Proposal.ApprovedBy (json:"approvedBy,omitempty"). No map[string]interface{} is added. OpenAPI text is updated.
  4. UI (public/channel-proposals.js / .css). Approved rows get a small auto / admin badge. A new Auto-approved filter shows the approved list narrowed on the client to approvedBy === '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

  • Backfill scope. The classification runs on every ingestor start, but it only touches 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).
  • Stale values after a rollback. An older ingestor that rejects or revokes a row leaves approved_by untouched. 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.
  • No new endpoint. "Auto-approved" reuses ?status=approved and filters on the client. The approved list is bounded by maxApproved.

Schema change / rollout

  • This is a schema change (new column), so it affects SKIP_SNAPSHOT for the next deploys.
  • Rollback compatibility: the v0.2.6 server and ingestor only use explicit column lists. The v0.2.6 server was run locally against a database that has the new column, and it works unchanged. The column is nullable, so v0.2.6 inserts are accepted.
  • Forward: a new server on a database without the column omits 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; AssertReady does 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 in Server.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 reports auto/admin; the filter=auto deep 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

  • Healer tick: no extra query. channel_proposals.approved_by is read by the same query that already probes client_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, and TestGraphScore561_AllocationBudget counts allocations process-wide. The first version of this PR pushed it over budget in CI shard 3 (352/350 vs 300) while master passed. TestDetectSchemaApprovedByAddsNoQueryToHealerTick now pins this.
  • The admin list reads one extra column. The client filter is O(n) over at most maxApproved rows. 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

dborup and others added 3 commits October 26, 2026 05:20
…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>
@dborup-agent

Copy link
Copy Markdown
Collaborator Author

Rapport — CS-pve-agent3 PR#617 #616 — head 92d5e69

Status: 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: 3d545c7f tests (red) → 257fa949 feature → 92d5e69f fix for the shard-3 allocation regression (see "CI").

Requirements

# Requirement Test(s) Mutant(s), all killed Evidence
1 Schema: nullable channel_proposals.approved_by ('auto'/'admin'), idempotent ALTER in the ingestor (internal/dbschema) TestChannelProposalsApprovedByMigration, TestChannelProposalsApprovedByFreshAndRebuilt M1 no ALTER [T] red at 3d545c7f (column missing), green after
2 Backfill: approved rows without a value → reviewed_at - created_at < 60000 ? auto : admin; documented as inferred; never overwrites a value TestChannelProposalsApprovedByMigration (59 999 / 60 000 ms boundary, second run silent and non-overwriting, row approved by an older ingestor) M2 < → <=, M3 drop approved_by IS NULL [T]
3 Writes: auto-approve → 'auto', admin approve → 'admin', reject/revoke → NULL, resubmit → NULL TestChannelProposalApprovedByAutoApprove, …AdminDecisions, …ResubmitClearsStaleValue M4 auto writes admin, M5 admin writes nothing, M6 reject/revoke keep the value, M7 resubmit keeps it [T]
4 Server: self-healing optional-column detection (warm probe + healer), no extra query per healer tick TestAdminChannelProposalApprovedBySelfHeals, TestHealSchemaFlagsLatchesProposalApprovedBy, TestDetectSchemaApprovedByAddsNoQueryToHealerTick M8 never latches, M9 healer skips the flag, M21 separate probe query [T]; [K] live: new server on an old DB, then migration while it runs → approvedBy appears without a restart
5 API: approvedBy as a named struct field (Proposal.ApprovedBy), present when the column exists (approved rows only), omitted on an old DB TestAdminChannelProposalListApprovedBy, TestAdminChannelProposalListWithoutApprovedByColumn, TestChannelProposalServiceUsesApprovedBySchemaFlag M10 column read without the flag (old DB → 500), M11 no approved-only guard, M12 production wiring missing, M13 field not serialised [T]; the old-DB test is a regression guard, green before and after by design
6 UI: auto/admin badge, "Auto-approved" filter, deep link #/channels?view=proposals&filter=… test-channel-proposals.js (7 new/updated cases, incl. DOM-level deep link + hash updates) M14 filter asks for status=auto, M15 no client filter, M16 badge for any value, M17 hash not updated, M18 deep-link filter ignored, M19 pending keeps &filter=pending [T] 7 red at 3d545c7f, 46/46 after
7 UI in a real browser, desktop + 390px test-channel-proposals-e2e.js (3 new steps: API auto/admin, filter=auto deep link + hash on filter clicks, 390px fit) M20 channels.js doesn't pass filter → 2 E2E steps fail [T] 29/29 locally against the CI-prepared e2e-fixture.db; [K] screenshots at 1280 light, 390 light, 390 dark (no horizontal overflow)
8 cmd/server stays read-only readonly_invariant_test.go, readonly_sql_literal_test.go (full server suite) n/a [T] suite green; [A] the server only SELECTs and probes; every write is in cmd/ingestor / internal/dbschema
9 No new map[string]interface{} outside tests n/a n/a [K] 0 added lines in non-test Go
10 CSS variables only n/a n/a [A] the badge uses --border, --radius-sm, --text-muted, --accent only
11 scripts/check-xss-sinks.sh --diff origin/master clean n/a M16 (badge only renders known constant values) [K] exit 0 on the final head
12 Fork guards unchanged (9 in deploy.yml, 1 in release-fast-path.yml) n/a n/a [K] 9 / 1; no workflow files touched

Schema change and rollback

  • This is a schema change (new column + one-off backfill), so it affects SKIP_SNAPSHOT for the next deploys.
  • Rollback: a v0.2.6 server on a DB with the new column runs fine. [K] Built v0.2.6 server, ingestor and migrate from the tag and ran them against the CI-prepared fixture migrated by this branch, with proposals carrying approved_by. Results: healthz 200; /api/channels lists both approved channels; the admin list returns 200 and ignores the column; v0.2.6's migrate leaves the values intact. [A] v0.2.6 reads and writes channel_proposals with explicit column lists only, and the column is nullable, so its inserts and updates are accepted.
  • Forward: [T]+[K] a new server on a DB without the column answers as before (no approvedBy) and picks the column up on the next healer tick.
  • [T] A row approved by an older ingestor after a rollback (value NULL) gets classified on the next upgrade. [A] A stale value left by an older ingestor's reject/revoke is masked on read (approved rows only) and cleared by the new ingestor's next transition of that row.

CI (per job, final head 92d5e69, run 40284942647)

Job Result
Go Server Tests (1/3), (2/3), (3/3) pass
Go Ingestor Tests pass
Go Libraries & CLI pass
JavaScript Unit Tests & Lint pass
Script & Container Unit Checks pass
Verify Go Test Assignments pass
Playwright E2E Tests (1/3), (2/3), (3/3) pass
Verify E2E Shards & Coverage pass
Validate Docker Image pass
Go Build & Test (gate) pass
Build & Publish Docker Image (gate) pass
Publish Badges & Summary, Release Artifacts, Deploy Staging skipped (PR)

Earlier failure, caused by this PR and fixed in 92d5e69. At head 257fa94, Go Server Tests (3/3) failed on TestGraphScore561_AllocationBudget: 352 allocs/op on the first run and 350 on the rerun, against a budget of 300. The two gates failed only as a consequence. This is not the known flaky #271 (a packets E2E). I reran once before establishing that.

Root cause:

  • [K] Locally, shard 3 failed on the branch (384/316) and passed on master with the identical runner and flags.
  • [K] Both master and the branch have 34 leaked schema-healer goroutines alive at that point. They come from tests that close only db.conn, so the healer keeps ticking on a dead connection.
  • [K] AllocsPerRun counts allocations process-wide. My separate approved_by probe doubled the per-tick cost on a dead connection (2 → 4 allocs).

Fix: the probe now shares the existing client_declared_regions query (back to 2 allocs per tick, the same as master). It is pinned by TestDetectSchemaApprovedByAddsNoQueryToHealerTick (red with the old code, green under -race too), and shard 3 passes locally and in CI.

The rerun also showed TestWorkerCore_CycleFailure_PreservesLastGoodStatusAndSnapshot failing once. [A] It is unrelated: in-memory fakes only (no DB, schema or proposals code), and it looks like an ordering race in the test, which checks the error recorder right after the status notification. It did not recur on the final head.

Local runs

  • [K] go test ./...: cmd/server ok, cmd/ingestor ok, all internal/* ok (final code).
  • [K] sh test-all.sh: 273 files, 0 failed.
  • [K] node test-frontend-helpers.js: 731 passed.
  • [K] node test-channel-proposals.js: 46/46.
  • [K] test-channel-proposals-e2e.js: 29/29 against a local Go server and ingestor using e2e-fixture.db prepared as in CI (freshen, seed, migrate, seeds). All processes were stopped by pid.
  • [K] Mutation: 21 mutants (M1–M21), all killed.

Remaining issues

  • Pre-existing test hygiene (not changed here): many server tests close only db.conn and leave a schema healer ticking for the rest of the binary. Any process-wide allocation budget stays sensitive to that. A follow-up could make those tests call db.Close() or stop the healer. Worth a separate issue.
  • TestWorkerCore_CycleFailure_PreservesLastGoodStatusAndSnapshot: one failure seen in a rerun; probably a test-ordering race; candidate for a separate flaky issue.
  • An approval made by an older ingestor during a rollback is classified by the 60 s heuristic, not exactly. This is documented as inferred.
  • The "Auto-approved" filter lists nothing until the ingestor has added the column (the server omits approvedBy on an old DB).
  • This PR stays a draft and has not been merged or marked ready.

@adminopenclaw8-sketch

Copy link
Copy Markdown
Collaborator

Review — CS-Macmini PR#617 — head 92d5e69

Dom: APPROVE med nits

Independent, read-only review. Verified on git archive of head 92d5e69f and of the merged tree (git merge-tree --write-tree origin/master 92d5e69f = 5d8d4364, clean merge) against origin/master 400d3645. git ls-remote showed 92d5e69f before and after the review; master unchanged. Evidence: [T] test/mutant I ran, [A] code reading, [K] ran it and observed the output.

Findings

# Sev Where Finding
N1 nit public/channel-proposals.js:656 The /[?&]view=proposals\b/ guard on the filter-click replaceState has no test for its false branch — deleting the guard keeps the unit suite at 46/46. [T]
N2 nit internal/dbschema/dbschema.go:1275 The approved_by DDL comment claims "NULL otherwise", but the backfill only touches status='approved', so a stale value on a rejected/revoked row is never cleaned up by the migration. [K]
N3 nit internal/dbschema/dbschema.go:1360-1365 The channel_proposals rebuild copies only the five original columns; unreachable today, but a future CHECK change through that path would silently downgrade exact values to inferred ones. No test. [A]
N4 nit cmd/server/channel_proposals.go:563 p.hasApprovedBy != nil is unreachable; dropping the nil check survives the whole cmd/server suite. Defensive only. [T]
N5 nit public/channel-proposals.js:728 An unknown filter in the deep link (&filter=bogus) correctly renders Pending, but mount does not normalise the hash, so the bogus value stays in the URL until a filter is clicked. Cosmetic. [A]
N6 nit public/channel-proposals.js:436 On a database the ingestor has not migrated yet, "Auto-approved" says "No auto-approved suggestions." — indistinguishable from genuinely none. Already in the author's Remaining issues. Cosmetic. [A]

N1 detail. test-channel-proposals.js:527 hardcodes location = { hash: '#/channels?view=proposals' }, so every DOM-level test enters the handler with the guard already true. The guard is load-bearing, not cosmetic: public/channels.js rewrites the hash in five places (:400, :497, :846, :1619, :2700) and onApproved: refreshChannelList runs while the dialog is open, so the hash can stop being a proposals deep link mid-session; without the guard the next filter click would overwrite whatever is there. A case that mounts with a non-proposals hash and asserts a filter click leaves it alone would close it.

N2 detail. Reproduced the rollback sequence on a migrated fixture: a v0.2.6-shaped UPDATE channel_proposals SET status='revoked', reviewed_at=? leaves approved_by='auto' on the revoked row, and a subsequent new-ingestor Apply classifies only the NULL approved row and leaves the stale one untouched. Nothing is user-visible — ListProposals masks it with CASE WHEN status = 'approved' THEN approved_by END, and the new ingestor clears it on that row's next transition — and the PR body documents the behaviour. Only the code comment on approvedByColumnDDL overstates the invariant.

Answers to the review points

Idempotent ALTER + backfill in the ingestor, boundary 60000 ms, never overwrites. Correct. ensureChannelProposalsApprovedBy probes with tableHasColumn, ALTERs only when missing, and classifies in the same transaction (defer tx.Rollback(), one Commit), so a crash leaves neither half applied. The WHERE status = 'approved' AND approved_by IS NULL clause is what makes it non-overwriting, and the index idx_channel_proposals_status(status, created_at) serves it. The log line is gated on !has || n > 0, so a second run is silent. ensureChannelProposalsTable is called unconditionally from Apply (dbschema.go:123) — not gated on the feature being enabled — so the ingestor can never reach its own INSERT … approved_by against a table without the column. [K] On the CI-prepared fixture: first corescope-migrate logged column added=true, 0 approved row(s) classified; the second run logged nothing about approved_by. Boundary [T]: the fixture pins 59 999 ms → auto and 60 000 ms → admin; mutating < to <= flips the 60 000 row and fails the test.

One behaviour worth recording: an approved row with reviewed_at IS NULL classifies as admin, because NULL - created_at < 60000 is NULL and CASE WHEN NULL … ELSE takes the else branch. That state is not reachable through either write path, and admin is the conservative answer, so this is fine as-is. [A]

Every write path sets/clears approved_by correctly. Verified by enumerating every statement that writes the table (cmd/ingestor, internal/channelregistry, internal/dbschema, non-test): there are exactly three in the ingestor plus the DELETE in retention. Auto-approve → 'auto' on INSERT; resubmit (revoked → pending) → approved_by = NULL in the same UPDATE; reviewChannelProposal → 'admin' when target == StatusApproved, otherwise a zero sql.NullString, i.e. NULL for both reject and revoke. Retention only deletes. [A] + [T] (the ingestor suite passes, and the author's cases cover a repeated approve keeping the value and a stale value being cleared on reject/resubmit).

Also checked for a stale-value leak on the other exit: proposalColumns is still id, name, status, created_at, reviewed_at and scanProposalRow never assigns ApprovedBy, so the Proposal the ingestor writes into its result file always has the field empty. With json:"approvedBy,omitempty" the result files and GET /api/channel-proposals/requests/{id} are byte-identical to master — no stale pre-update value reaches that endpoint, and the OpenAPI text for it correctly stays unchanged. [A]

No per-tick query, and the server stays read-only. Correct, and the merge into the existing client_declared_regions probe is the right fix rather than a workaround. pragma_table_info as a table-valued function is already used in five places in this repo, and a missing table yields an empty result rather than an error, so the combined SELECT (…),(…) is safe on an old database. [T] M21 rerun: splitting it back into two queries makes TestDetectSchemaApprovedByAddsNoQueryToHealerTick report 4 allocs against a budget of 2 — exactly the 2→4 doubling the author root-caused. [T] My own mutant (below) confirms the refactor did not lose the client_declared_regions pin. Read-only: the cmd/server diff adds one SELECT and the flag plumbing and no write; TestServerDBConnIsReadOnly, TestHashMigrate_NeverWritesOnTheReadOnlyHandle_215 and the rest of the suite pass.

approvedBy is a named struct field, omitted on an old DB. Correct. channelregistry.Proposal.ApprovedBy string with json:"approvedBy,omitempty", embedded into AdminChannelProposal; no map[string]interface{} anywhere in the non-test Go diff. The approved-only masking is done in SQL, so a stale value can never be serialised. [T] M10 rerun: forcing withApprovedBy := true turns the old-DB path into no such column: approved_by and fails three server tests with a 500 — the flag is genuinely load-bearing.

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 channel_proposals carried approved_by values: the server booted (AssertReady passed), /api/healthz 200, /api/channels 200 with both channels, and GET /api/admin/channel-proposals returned 200 with the column simply absent from the JSON, for both the unfiltered and the ?status=approved list. v0.2.6's migrate reported "migrated and ready" and left the column and every value intact. [A] The mechanism holds: v0.2.6 uses explicit column lists throughout (SELECT id, name, status, created_at, reviewed_at, INSERT INTO channel_proposals (id, name, …), UPDATE … SET status = ?, reviewed_at = ?) with no SELECT *, and its upgradeChannelProposalsStatusCheck early-returns because the ALTERed DDL still contains 'revoked' in the status CHECK — which is why the rebuild that would have dropped the column never fires. [K] A v0.2.6-shaped five-column INSERT of an approved row is accepted (nullable column, and NULL IN (…) leaves the CHECK satisfied) and is reclassified by the next new-ingestor Apply.

The allocation-budget fix in 92d5e69 is a real fix, not a loosened budget. Confirmed. cmd/server/graph_score_561_test.go is untouched by this PR — the budget is still float64(v / 2) with no edit — so nothing was relaxed; the commit reduces detectSchema's per-tick cost back to master's 2 allocs on a closed connection and pins it with a new test. [A] + [T]. The underlying fragility (server tests that close only db.conn and leave healers ticking, against a process-wide AllocsPerRun) is pre-existing and correctly called out as a separate follow-up rather than papered over here.

UI: badge, filter, deep link, CSS variables, 390px. All present and behaving. The badge renders only for the two known constants, gated by hasOwnProperty on a module-level map, and nothing from the field reaches the markup — the title text comes from that constant map, not from the value. .ch-proposals-by uses --border, --radius-sm, --text-muted, --accent only; all four are defined in public/*.css, and the CSS diff adds no literal colour. .ch-proposals-filters already has flex-wrap: wrap, so the fifth button wraps rather than overflowing. "Auto-approved" asks the server for ?status=approved and narrows on the client, so no filter key can 400 the admin list. The gate is clean: check-xss-sinks.sh --file exits 0 on both changed public/ files (channel-proposals.js, channels.js) — I used --file because --diff origin/master cannot resolve a ref inside a git archive tree and silently reports "no changes to scan" there.

No behaviour change beyond the purpose. Checked the two places where existing strings were rerouted: the empty-list text now goes through filterLabel(filter).toLowerCase() and the request URL through filterStatus(filter), and for all four pre-existing filters both are byte-identical to master (each label lowercased equals its key, and filterStatus is the identity outside 'auto'). The only other non-test edits outside the feature are the new flag being added to two existing healer-flag lists and the OpenAPI description text. [A]

Hygiene. Commit author and committer are dborup <kontakt@meshview.dk> on all three commits. No closing keywords in the PR body ("Relates to #616") or in any commit message. Fork guards stand at 9 github.repository == occurrences in deploy.yml and 1 in release-fast-path.yml; no .github/ file is touched (0-line diff). No new map[string]interface{} outside tests. The PR is still a draft; I changed nothing on it.

Acceptance criteria

Every criterion in the issue has a test. Red-before is real but of two different kinds, worth stating precisely: at the test-only commit 3d545c7f, internal/dbschema fails on assertions (approved_by after Apply: has=false) and test-channel-proposals.js fails 7 of 46 cases — exactly the 7 new ones — while cmd/ingestor and cmd/server fail to build (undefined: channelregistry.ApprovedByAuto), which is the usual shape for a Go API addition but is weaker evidence than an assertion failure. The mutants below carry the behavioural weight for those two suites. [T]

Issue criterion Test Verified
Nullable approved_by ('auto'/'admin'), idempotent ALTER TestChannelProposalsApprovedByMigration, …FreshAndRebuilt [T] red→green, [K] twice live
Registered with the self-healing optional-column detection TestAdminChannelProposalApprovedBySelfHeals, TestHealSchemaFlagsLatchesProposalApprovedBy [T]
Writes: auto / admin / reject / revoke / resubmit TestChannelProposalApprovedByAutoApprove, …AdminDecisions, …ResubmitClearsStaleValue [T], [A] all write paths enumerated
Backfill with the 60 s window, documented as inferred TestChannelProposalsApprovedByMigration (59 999 / 60 000) [T] boundary, [K]
approvedBy named field; omitted on an old DB TestAdminChannelProposalListApprovedBy, …WithoutApprovedByColumn, …ServiceUsesApprovedBySchemaFlag [T]
Read-only invariant TestServerDBConnIsReadOnly + full suite [T], [A]
Badge + filter unit tests 7 new cases in test-channel-proposals.js [T] 7 red before
Playwright filter + deep link 3 new steps in test-channel-proposals-e2e.js [T] 29/29

Tests I ran

Suite Result
internal/dbschema ok
internal/channelregistry ok
cmd/server (go test ./...) ok, 77 s
cmd/ingestor (go test ./...) ok
sh test-all.sh 274 passed, 0 failed (274 files)
node test-frontend-helpers.js 731 passed, 0 failed
node test-channel-proposals.js 46 passed, 0 failed
test-channel-proposals-e2e.js 29 passed, 0 failed (real server + ingestor + Chromium)
check-xss-sinks.sh --file on both changed public/ files exit 0

The E2E ran against locally built corescope-server / corescope-ingestor / corescope-migrate and e2e-fixture.db prepared exactly as CI does (freshen, the Kpa-clawbot#1486/Kpa-clawbot#1791 seed SQL, corescope-migrate, then the 2073 and 199 seeds). The harness allocates ephemeral ports itself, so it cannot collide with anything else on this machine; the one process I started by hand was stopped by looking its pid up from the port, and the port is free again.

CI, checked per job on 92d5e69f (run 40284942647): all 15 executed jobs green — Go Server Tests 1/3, 2/3, 3/3; Go Ingestor Tests; Go Libraries & CLI; JavaScript Unit Tests & Lint; Script & Container Unit Checks; Verify Go Test Assignments; Playwright E2E 1/3, 2/3, 3/3; Verify E2E Shards & Coverage; Validate Docker Image; Go Build & Test; Build & Publish Docker Image. Release Artifacts, Deploy Staging and Publish Badges are skipped on a PR. The author's table matches. Neither known flake (#256 Hash Stats sort, #267 backfill write-hold) appeared, here or in my local runs.

Mutants

Reran the three requested, plus four of my own.

Mutant Change Result
M2 backfill < → <= killed — the 60 000 ms row flips to auto
M10 withApprovedBy := true (ignore the schema flag) killed — old DB → no such column, 500, 3 tests fail
M17 drop the replaceState on a filter click killed — 45/46, the deep-link case fails
M21 split the probe back into its own query killed — 4 allocs vs a budget of 2
OWN-1 backfill WHERE status='approved' AND approved_by IS NULL → WHERE approved_by IS NULL killed — pending/rejected/revoked rows get classified; both dbschema tests fail
OWN-2 in the merged detectSchema query, latch hasClientDeclaredRegionsFlag off the wrong counter killed — TestDeclaredRegionsTableCreatedAfterServerStartup and TestHealSchemaFlagsLatchesDeclaredRegionsTable fail; the refactor did not lose the existing pin
OWN-3 remove the /[?&]view=proposals\b/ guard before replaceState SURVIVES — 46/46 (finding N1)
OWN-4 p.hasApprovedBy != nil && … → p.hasApprovedBy() SURVIVES — full cmd/server suite ok; unreachable defensive branch (finding N4)

What I did not verify

  • Staging or production behaviour, and anything requiring the server's API key — out of scope for this review. The SKIP_SNAPSHOT consequence of the schema change is taken from the PR body, not tested.
  • The author's screenshots at 1280/390 light and dark. I relied on the E2E's own 390 px assertions (no horizontal overflow, and both the Auto-approved button and the badge inside the viewport); I did not inspect rendering by eye, and the E2E checks geometry rather than contrast or legibility of the new badge in dark mode.
  • check-xss-sinks.sh --diff origin/master in its real git form — I ran --file on both changed public/ files instead, which covers the same two files for this diff.
  • The two flakes the author reports from the earlier run (TestWorkerCore_CycleFailure_PreservesLastGoodStatusAndSnapshot, and the one rerun on 257fa949) — neither recurred for me, and I did not try to reproduce them.
  • Concurrency between a running ingestor's ALTER and the server's healer tick under real load; covered here only by the single-process self-healing test and the author's live check.
  • Long-run behaviour of the backfill on a large channel_proposals table. The table is bounded by maxApproved/maxPending, so this is not a realistic concern.

@dborup
dborup marked this pull request as ready for review October 26, 2026 08:16
@dborup
dborup merged commit b519e6b into master Oct 26, 2026
18 checks 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.

3 participants