Repository navigation
Commit 6673c80
feat(cli)!: the v18 upgrade carries stored comment reactions into sys_comment_reaction records, and the column drop waits for it (#23318)
Fixes #23281
Clause-②: no (narrowing) — `os migrate apply --allow-destructive` now
refuses the `sys_comment.reactions` drop until a verified conversion is
recorded, and its two `--json` account-issuer refusals now exit 1, as
their human face already did. Converting stored rows into an existing
object widens no accept set (the gated-subcommand precedent is #21120).
The semantic entry changes in prose only.
Dispatched by the `domain:services` seat 2 PM (claim comment 6150488591
on #23281), session `session_01FzQy26KznSa5j2gnf3AzsE`, branch
`claude/issue-23281-comment-reactions-upgrade-conversion`.
## What this does
The v18 retirement of `sys_comment.reactions` (`9963322fe8`) left every
reaction a 17.x deployment stored in that column unread, and the drift
remedy deleted them. Under the maintainer's rule that an upgrade keeps
existing data (objectstack-ai/cloud#2843 `6145481762`), the upgrade now
carries them:
- **`os migrate comment-reactions`** (new, `packages/cli`). Each
(comment, emoji, user) in a stored map becomes one
`sys_comment_reaction` row with the same comment, emoji and user, in the
comment's `organization_id`, with `created_by` = the reactor (the key of
the own-record delete floor, so the member can un-react it). A value
that does not read is listed in the report with its comment id and
converted into nothing: `not_json`, `not_a_map`, `not_a_user_list`,
`not_a_user_id`, `emoji_not_storable` (bound read from the registered
`sys_comment_reaction.emoji`), `unknown_user` (an id with no `sys_user`
row). Readable entries of the same comment still convert. Dry run by
default; `--apply` writes, re-reads the column, and records the run in
`sys_migration` under `sys-comment-reactions-to-records`, verified only
when everything readable is converted and nothing is unreadable.
Idempotent on the natural key, so a re-run inserts nothing and finishes
an interrupted run.
- **`os migrate apply --allow-destructive` holds the drop.** A
column-specific refusal beside the existing `sys_account.issuer`
retirement pre-flight, placed by the same rule (below the report, above
the prompt, above both writes): the drop of `sys_comment.reactions` is
refused (exit 1, `--json` `message:
"refused_comment_reactions_unconverted"`) until the verified run is
recorded. Every other drift entry and flag is unchanged. The boot drift
line names that command as the remedy, so following it before converting
is now safe.
- **The semantic entry** `18.sys-comment-reactions-column-retired.ts`:
`replacement`, `reason` and `acceptanceCriteria` prose name the
conversion and order it before the drop. `id` and `surface` unchanged;
no schema, `*.zod.ts`, contract or register change. The upgrade guide
was regenerated with `pnpm --filter @objectstack/spec gen:upgrade-guide`
(it writes the gitignored `content/docs/protocol-upgrade/`, so nothing
is committed; `18.md` carries the new prose) and `check:upgrade-guide`
is green.
- `plugin-audit`'s `sys-comment.object.ts` docblock,
`content/docs/deployment/cli.mdx` (table row and a subsection), the two
family tables that enumerate every `bootSchemaStack` caller, and one
changeset (`@objectstack/cli` minor, `@objectstack/spec` patch) saying
what the upgrade now carries.
## The PM's hypotheses, measured
- **H1 — holds.** On a booted database the column stays with its values;
`os migrate plan` lists `sys_comment.reactions [drop_column]` as
destructive. The raw read reuses `os migrate account-issuer`'s:
`engine.getDriverForObject(name).find(...)` below the data door (the
engine's `find` projects declared fields only). `sys_user` is read the
same way (the data-migration boot composes no auth plugin).
- **H2 — holds.** The stored shape is objectui's `JSON.stringify({
[emoji]: userIds[] })` (`RecordDetailView.tsx` `storedReactions`). The
six unreadable reasons above are reported, never guessed.
- **H3 — holds.** Natural key `(comment_id, emoji, user_id)`; the
declared `unique: 'organization'` index refuses a duplicate
(`DUPLICATE_RECORD`, seen in ablation 1 below). Each row carries its
comment's organization.
- **H4 — measured.** No code in this repo fires a notification or
automation on a reaction insert; the data-migration boot composes no
automation runtime. plugin-audit's reaction create gate returns early
for a system write, so `user_id` keeps the stored reactor (an unexempted
gate would have replaced or refused it). The writes use the bookkeeping
context the organization seed claim uses, `{ isSystem: true,
skipAutomations: true }`; audit stamps and code hooks still run. Found
on the way: `created_by` must be the reactor or the member cannot
un-react a converted reaction; it is carried in the row (the stamp hook
assigns nothing without a session user).
- **H5 — partly falsified.** `audit-metadata-bodies` records no
`sys_migration` row and is not journal-backed; `value-shapes` records
the row; only `recorded-by` / `meta` use the ADR-0119 journal. The shape
taken: `audit-metadata-bodies`'s boot, dry run and idempotent rewrite,
plus `value-shapes`'s ledger row. No journal: every reaction is one
atomic insert and the natural-key guard makes each re-run the resume.
- **H6 — falsified as stated, landed through the existing mechanism that
does it.** `files-to-references`' column-step refusal gates that
command's own column move on its own gate; it cannot hold back `os
migrate apply`. The mechanism that holds back one destructive drop
inside `os migrate apply` is the `sys_account.issuer` retirement
pre-flight; this adds its sibling for one column. It reads a ledger row
rather than re-measuring live, deliberately: after conversion a member
may un-react, and a live probe would read that deleted row as
unconverted and either refuse the drop forever or, worse, re-create the
reaction if the operator re-ran the conversion to satisfy it. No path
can write the column after v18 (the data door refuses it), so a verified
run stays true until the column is gone.
## Measured on a booted composition (before the pins)
`pnpm dev:todo -- -p PORT --database file:SCRATCH/react.db`
(`SqlDriver(better-sqlite3)`, Audit plugin composed, branch at
`f5db79977e`), signed in as the seeded admin; a second user made with
the admin create-user door; four comments posted through `POST
/api/v1/data/sys_comment`. The server was stopped and the 17.x leftover
planted by hand: `ALTER TABLE sys_comment ADD COLUMN reactions TEXT`
(v17 declared it a textarea) and four v17-shaped values:
`{👍:[admin,m2],🎉:[m2]}`, `{❤️:[admin],👀:[]}`, `"not json at all"`,
`{👍:[m2,"usr_deleted_since"],🚀:3}`.
| step | exit | `sys_comment_reaction` rows | `sys_migration` row |
column |
|:--|:--|:--|:--|:--|
| before | | 0 | none | present |
| `os migrate apply --allow-destructive --yes --json` | 1
`refused_comment_reactions_unconverted` | 0 | none | present |
| `os migrate comment-reactions --json` (dry run): 4 scanned, 5
readable, 5 to convert, 3 unreadable (`not_json`, `not_a_user_list`,
`unknown_user`) | 1 | 0 | none | present |
| `… --apply --yes --json`: 5 converted, `created_by` and
`organization_id` as the reactor and the comment's org | 1 | 5 |
`verified_at null`, `blocking 3` | present |
| `… --apply` again: 0 converted, 5 already, `failures []` | 1 | 5 |
`blocking 3` | present |
| `os migrate apply --allow-destructive` | 1 refused (`blocking 3`) | 5
| | present |
| operator clears one value, keeps the readable part of the other; `…
--apply` | 0, verified | 5 | `verified_at` set, `blocking 0` | present |
| `os migrate apply --allow-destructive --yes --json` | 0, `applied:
sys_comment drop_column reactions` | 5 | | gone |
| `pnpm dev:todo` on the same file | no `[schema-drift]` line; `GET
/api/v1/data/sys_comment_reaction` as admin answers the 5 rows; admin
`DELETE`s its own converted reaction: `200` | 4 | | gone |
## Pins
(`packages/cli/src/commands/migrate/comment-reactions.integration.test.ts`)
One real SQLite file under the SQL driver the commands boot, schema made
by `os migrate apply --yes` (the served composition with an auth
secret), then the 17.x column and values planted by hand. Seven ordered
cases through the commands' own `run()`: (1) drop refused before any
conversion, `{ message: 'refused_comment_reactions_unconverted', detail,
conversion: null, applied: [] }`, exit 1, column present; (2) dry run:
exact counts, the four unreadable entries with their comment ids,
nothing written; (3) `--apply`: rows equal the expected set (same
comment, emoji, user; comment's organization; `created_by` = reactor),
unreadable listed, nothing for the unreadable comment, ledger `{
verified_at: null, blocking: 4 }`; (4) a re-run inserts nothing, `{
alreadyConverted: 5, converted: 0, failures: [] }`; (5) drop still
refused with the recorded run under `conversion`; (6) after the operator
resolves the values, verified, exit 0; (7) the drop proceeds and every
row survives.
### Negative-pin ablations (fix committed first; each run through
`scripts/ablation-replace.mjs` under the verify lock, with a `trap`
restore; both restored to the HEAD blob `1c075685b2e1`, `git diff HEAD`
empty)
1. **Idempotency guard removed** (`if (held.has(reactionKey(…)))` → `if
(false && held.has(…))`, anchor 1 → 0, blob `1c075685b2e1` →
`dd02d298f4c0`): red, 5 failed / 2 passed. Case 4: `-
"alreadyConverted": 5 / + "alreadyConverted": 0`, `- "failures": [] / +
"failures": [ { "code": "DUPLICATE_RECORD", "commentId": "rx_c1",
"emoji": "👍", … } × 5 ]` — the declared unique index refused every
duplicate; the report is what the pin reads.
2. **Unreadable report entry removed** (the parse-level
`report.unreadable.push(…)` deleted, anchor 1 → 0, blob →
`75af39952334`): red, 3 failed / 4 passed. Cases 2 and 3: the expected
`not_json` / `not_a_user_id` / `not_a_user_list` entries missing, only
`unknown_user` received.
## Local verification (at HEAD `bf15e3062a`, after merging `origin/main`
at `5c9bfb0d3f`)
- Build: `turbo run build --filter=!@objectstack/docs --concurrency=2`
green (74 tasks) after the merge.
- `pnpm --filter @objectstack/cli typecheck`: `tsc --noEmit` clean;
`check:test-typecheck: OK` (3 files / 28 errors / 6 pinned signatures
held in the existing ledger; the new test adds none).
- `@objectstack/cli` integration, the three files this diff touches or
depends on (`comment-reactions.integration.test.ts`,
`apply.account-issuer-retirement.integration.test.ts`,
`schema-migrate.one-shot-family.integration.test.ts`): 3 files, 99 tests
passed. The seven new cases take 0.1 to 0.65 s each.
- `@objectstack/cli` unit tier (`vitest run --project unit`): 286 files,
4288 tests passed.
- `@objectstack/spec` `pnpm test`: 669 files, 20276 passed, 1 todo.
`@objectstack/plugin-audit` (`sys-comment-retired-fields.test.ts`,
`objects.test.ts`, `sys-comment-reactions-door.integration.test.ts`): 3
files, 22 passed.
- Derived gates: `node scripts/pm/dispatch-gates.mjs --commands` (no
paths) derived 118 families from 10 changed paths; all 118 ran, each
exit 0. `--ran` reconciliation: `✓ dispatch-gates --ran: 118 derived
famil(ies) accounted for — 118 run, 0 NOT-MEASURED (a DERIVED zero — all
118 recorded an exit code and none of them is 3).` Among them
`check:upgrade-guide` (`✓ protocol-upgrade-guide.md generates from the
ADR-0087 registries`), `check:spec-changes`, `check:doc-authoring`,
`check:tenant-audit-census` (`OK -- 225 write call sites certified`),
`check:nul-bytes` (`OK … no raw ASCII control bytes`),
`check:adr-0087-registration` (`this PR adds no declared-breaking
changeset`).
- Lint, narrowed: `eslint --no-inline-config --format json` over the 8
changed `.ts` files: 8 files, 0 errors, 0 warnings. The population is
the config's own: every changed file matches `**/*.{ts,…}` and none sits
in `NEVER_LINTED`; `eslint.config.mjs` never enables type-aware linting
(no `parserOptions.project`, no typed rules), so this diff cannot move
the verdict on an untouched file. The repo-wide `pnpm lint` is CI's.
## Patch round 1 (at `42b3638b94`; the seat's amendment `6152677078`,
the dev's report `6153407686`)
- **Grading:** the card is `Clause-②: no (narrowing)`: the destructive
apply refuses a drop it accepted before. The changeset now carries a
bang summary, a **BREAKING** (accept-set narrowing) banner and the
ADR-0087 disposition `not-required (already-registered
sys-comment-reactions-column-retired)`. `check:adr-0087-registration`
passes: `[BREAKING+bang+clause-②-narrowing] not-required
(already-registered)`.
- **The `--json` account-issuer refusals now exit 1.** This was measured
first at the real command (`os migrate apply --allow-destructive --yes`
on legacy-shaped SQLite databases). Under `--json`,
`refused_account_issuer_collisions` and
`refused_account_issuer_preflight_unreadable` exited `0`, while the
human face exited `1`. Both now exit `1`. The pins are in
`apply.account-issuer-json-refusal.test.ts`: exit 1, the `message`, and
nothing written, with a human-face control and a clean-pre-flight
control.
- **Ablation:** with both sites put back to `0`, the two new pins go red
(`AssertionError: expected +0 to be 1`) and both controls stay green.
Restored: blob == HEAD (`4fe54313fc9f`), `git diff HEAD` empty.
- **QA checklist:** `docs/qa/platform-checklist/areas/cli.json`, item
`cli.migrate-plan-apply-json`, now lists the 16 subcommands `os migrate
--help` registers. Added: `organization-ownership`,
`security-catalog-overlays` and `comment-reactions`.
- **Gates at `42b3638b94`:** 119 derived, 119 run. 118 exit 0.
`check:platform-checklist` exits 1 on 8 symbol anchors outside this diff
(`access-security.json`, `search.json`, `attachments-storage.json`). The
same 8 lines appear without this PR's edit, and on `origin/main`
`404fc477e5`. Per `lint.yml`, that gate runs as a daily watchdog on
`main`, not per PR.
- **Tests:** cli typecheck clean; cli integration 3 files, 99 passed;
cli unit tier 287 files, 4292 passed.
## Acceptance notes
- `os migrate apply --json`'s two `sys_account.issuer` pre-flight
refusals (`refused_account_issuer_collisions`,
`refused_account_issuer_preflight_unreadable`) emit with exit code 0
(`emitJson(…, 0, …)` then `return`), while this PR's sibling refusal
exits 1. Read from the code, not measured at the door; noted, not filed.
- `docs/qa/platform-checklist/areas/cli.json` (the migrate item)
enumerates 13 `os migrate` subcommands as variants; the tree now
registers more (`organization-ownership`, `security-catalog-overlays`,
and this `comment-reactions` among them). A checklist sweep owns it;
noted, not filed.
- The CLI e2e `test/json-stdout-purity.e2e.test.ts` (nightly tier) gains
the new command in its derived family; it was not run locally.
---
_Generated by [Claude
Code](https://claude.ai/code/session_01FzQy26KznSa5j2gnf3AzsE)_
---------
Co-authored-by: Claude <noreply@anthropic.com>1 parent 37510aa commit 6673c80
12 files changed
Lines changed: 1521 additions & 21 deletions
File tree
- .changeset
- content/docs/deployment
- docs/qa/platform-checklist/areas
- packages
- cli
- src
- commands/migrate
- utils
- test
- plugins/plugin-audit/src/objects
- spec/src/migrations/entries/semantic
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1153 | 1153 | | |
1154 | 1154 | | |
1155 | 1155 | | |
| 1156 | + | |
1156 | 1157 | | |
1157 | 1158 | | |
1158 | 1159 | | |
| |||
1349 | 1350 | | |
1350 | 1351 | | |
1351 | 1352 | | |
| 1353 | + | |
| 1354 | + | |
| 1355 | + | |
| 1356 | + | |
| 1357 | + | |
| 1358 | + | |
| 1359 | + | |
| 1360 | + | |
| 1361 | + | |
| 1362 | + | |
| 1363 | + | |
| 1364 | + | |
| 1365 | + | |
| 1366 | + | |
| 1367 | + | |
| 1368 | + | |
| 1369 | + | |
| 1370 | + | |
| 1371 | + | |
| 1372 | + | |
| 1373 | + | |
| 1374 | + | |
| 1375 | + | |
| 1376 | + | |
| 1377 | + | |
| 1378 | + | |
| 1379 | + | |
| 1380 | + | |
| 1381 | + | |
| 1382 | + | |
| 1383 | + | |
| 1384 | + | |
| 1385 | + | |
| 1386 | + | |
| 1387 | + | |
| 1388 | + | |
| 1389 | + | |
| 1390 | + | |
| 1391 | + | |
| 1392 | + | |
| 1393 | + | |
| 1394 | + | |
| 1395 | + | |
1352 | 1396 | | |
1353 | 1397 | | |
1354 | 1398 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
204 | 204 | | |
205 | 205 | | |
206 | 206 | | |
207 | | - | |
| 207 | + | |
208 | 208 | | |
209 | 209 | | |
210 | 210 | | |
| |||
285 | 285 | | |
286 | 286 | | |
287 | 287 | | |
288 | | - | |
| 288 | + | |
| 289 | + | |
| 290 | + | |
| 291 | + | |
289 | 292 | | |
290 | 293 | | |
291 | 294 | | |
292 | 295 | | |
293 | 296 | | |
294 | 297 | | |
295 | | - | |
| 298 | + | |
296 | 299 | | |
297 | 300 | | |
298 | 301 | | |
| |||
303 | 306 | | |
304 | 307 | | |
305 | 308 | | |
| 309 | + | |
| 310 | + | |
| 311 | + | |
| 312 | + | |
| 313 | + | |
| 314 | + | |
306 | 315 | | |
307 | 316 | | |
308 | 317 | | |
| |||
0 commit comments