fix(daemon): declare CHAIN_ROW_COLUMNS once, in store.rs

[Georgefifth]

Problem

CHAIN_ROW_COLUMNS — the 17-column SELECT list every ChainRow read shares — was declared twice: once in transactions.rs (SQLite) and once in store/postgres.rs, kept in sync only by a "Mirrors" doc comment. The Postgres copy's own comment records the last drift: adding the caller-identity columns updated the mapper and one of the two queries, and the miss surfaced only as a runtime no column found for name: chain_version from the live-Postgres test while unit tests stayed green. A contributor adding a column still has to remember the second file, and only a running Postgres catches the miss (#397).

Solution

• Hoist one pub(crate) const CHAIN_ROW_COLUMNS into store.rs, the shared parent of both backends; the drift history paragraph moves with it.
• Both modules import it; all four SELECT sites are unchanged (placeholders stay per-backend — SQLite ?1, Postgres $1 — only the column list is shared). The second declaration is deleted.

Tests, per the issue's sequence:

1. Before the hoist, added chain_row_columns_agree_across_backends asserting the two copies are equal, and ran it green.
2. Mutated it red on purpose — changed caller_principal to caller_principaI in the Postgres copy:

assertion `left == right` failed: CHAIN_ROW_COLUMNS drifted between transactions.rs (SQLite) and store/postgres.rs — the positional mappers read columns in this order, so a mismatch breaks one backend's chain reads at runtime
  left:  "…, event_tip, caller_principal"
  right: "…, event_tip, caller_principaI"
    FAIL sysknife-daemon store::tests::chain_row_columns_agree_across_backends

3. After the hoist there is no second copy to compare, so replaced it with chain_row_columns_pinned: asserts the exact 17-column list and count, which is the invariant both mappers (positional SQLite, by-name Postgres) actually depend on.

Result

cargo nextest run -p sysknife-daemon: 1081 passed, 5 skipped. cargo fmt --check and cargo clippy --all-targets clean. No Postgres needed — this removes the check the live-Postgres job was uniquely positioned to catch.

Closes #397

[vladimirrott]

Reviewed at 52e03a90899b6bfb9f9da4f5a5d8bc3a7368beb9.

Two identical declarations of a 17-column list, one in transactions.rs and one in store/postgres.rs, collapsed into one in store.rs. The reason this is worth doing rather than tidy-for-its-own-sake is that the two copies decode differently: SQLite reads positionally and Postgres reads by name, so a drift between them would corrupt one backend silently while the other stayed correct.

I checked the only thing that actually matters here, which is whether the column list and its order survived the move byte for byte. Both removed declarations:

const CHAIN_ROW_COLUMNS: &str = "seq, key_id, transaction_id, request_id, request_hash, \
     action_name, risk_level, summary, approval_id, warnings_json, \
     created_at, prev_chain_hash, chain_hash, chain_version, caller_role, event_tip, \
     caller_principal";

and the new one in store.rs is the same string with pub(crate) added and a line break after =. Identical content, identical order. The four SELECT {CHAIN_ROW_COLUMNS} call sites in transactions.rs and store/postgres.rs are untouched by this PR, so nothing about the emitted SQL changes.

I also confirmed the positional side still lines up: chain_row_from_sqlite maps indices 0 through 16 onto those 17 names in that order, and row_to_chain_row on the Postgres side uses try_get("name"), which is order-insensitive. So the invariant your new test pins is the real one.

The test is the part I liked most:

let columns: Vec<&str> = crate::store::CHAIN_ROW_COLUMNS.split(',').map(str::trim).collect();
assert_eq!(columns, [ ... ], "CHAIN_ROW_COLUMNS changed — both ChainRow mappers read these \
     columns in this order, so update them in the same change");
assert_eq!(columns.len(), 17, "column count drifted from 17");

It reads the production constant and compares it against a spelled-out literal, which is an independent oracle rather than a restatement of the constant. And reverting the hoist does not leave it green, it stops the crate compiling, because the test reaches through crate::store::CHAIN_ROW_COLUMNS and that path would not exist.

The test count is handled correctly: one #[test] added, tests/evidence/workspace-tests.json 1852 to 1853, and all three prose files moved with it. I verified the delta rather than trusting the number.

One honest limitation, not a regression and not something to fix here: the pin asserts the string, and nothing asserts that chain_row_from_sqlite's indices track it. Someone reordering the constant and the mapper together would pass. That is the same coverage the repo had before your change, so you have not lost anything, but it is the next thing worth closing if you want it.

A coordination note that is not your fault: #413 and #426 also move the baseline from 1852 to 1853, and because the hunk is textually identical in all three, git merges them without a conflict. Whichever lands second carries a recorded 1853 against a real 1854. That fails closed rather than silently, since scripts/test_baseline.sh re-measures, but it does mean I regenerate the baseline in the merge rather than inheriting your figure. Mine to sequence.

Approving. Clean, well-scoped, and the constant is now at the right owning boundary.

[vladimirrott]

Short update, and it costs you nothing. I merged #426 as 4b7b5d5, so main now records 1853 tests and carries the same three prose figures.

That makes your tests/evidence/workspace-tests.json hunk one short: 1853 recorded against a real 1854 once your #[test] lands on top. Git merges the hunk without a conflict because it is textually identical, so the first sign will be a red rust job rather than a merge warning.

I said on the review that sequencing this was mine, and it still is. Nothing to push, nothing to chase. CHAIN_ROW_COLUMNS itself merges clean against the new main:

$ git merge-tree --write-tree main p424
424 clean

[vladimirrott]

Re-approving at 9c815c21. The push that dismissed the earlier approval was mine, not yours, and it moved four numbers and nothing else.

Your production commit 627d875 is untouched. Against main as it stands now, the whole pull request is:

crates/sysknife-daemon/src/store.rs          | 63 ++++++++++++++++++
crates/sysknife-daemon/src/store/postgres.rs | 10 ++---
crates/sysknife-daemon/src/transactions.rs   | 14 ++-----
README.md, docs/introduction.md, docs/distro-support.md, tests/evidence/workspace-tests.json

The baseline needed moving because #426 landed while this sat in the queue, and your 1853 became one short. I took the figure from the runner rather than from arithmetic, since a hand-typed count is the exact thing test_baseline.sh exists to abolish and it would pass every check on its way in:

$ UPDATE_TEST_BASELINE=1 scripts/test_baseline.sh
test_baseline: recorded 1854 rust tests in tests/evidence/workspace-tests.json
$ python3 scripts/check_evidence_claims.py
Published figures match the evidence artifacts.

One failure worth naming so it does not look like yours. The pre-commit run reported a_sigterm_ignoring_child_is_escalated_to_sigkill_within_the_grace failing, on a tree that had passed it minutes earlier. It sets SYSKNIFE_ACTION_TIMEOUT_SECS as a process-global and asserts on a one-second deadline plus a five-second grace, which is #356's shape, and two cargo builds were competing for the machine at that moment. Run alone it takes 6.27s against that bound:

$ cargo nextest run -p sysknife-daemon --test execute_spec --test-threads=1
PASS [6.268s] a_sigterm_ignoring_child_is_escalated_to_sigkill_within_the_grace
Summary 21 tests run: 21 passed, 0 skipped

Nothing in your diff goes near the executor. That is #356 under load, not this pull request.

The single declaration is the right call and the comment above it is the part I would keep. Naming the miss that motivated it, a chain_version column added to the mapper and one of the two queries, showing up only as a live-Postgres no column found for name, is what makes the next person leave it alone.

[vladimirrott]

Correcting myself on one thing in that review, because I named the wrong cause and it points at an open issue that has nothing to do with it.

I attributed the a_sigterm_ignoring_child_is_escalated_to_sigkill_within_the_grace failure to #356 and to timing under load. The log says otherwise:

thread '...' panicked at crates/sysknife-daemon/tests/execute_spec.rs:614:5:
a previous run leaked the marker; the assertion below would be meaningless

That is the pre-check at the top of the test, and it fired in 0.028s, far too fast to be the one-second deadline or the twenty-second bound I quoted. The cause was mine: I had two test runs of this repository going at once on the same machine, and the marker sleep 31495 is a fixed constant in the process table, so one run's child was still alive when the other started.

The test was right and it said so in one line. Nothing to fix in it, nothing to fix in your pull request, and #356 is unrelated. The conclusion I drew stands; the reason I gave for it did not.

[vladimirrott]

Merged as e8dd35d.

The gate ran the proof rather than taking my summary of it. Moving one column name inside CHAIN_ROW_COLUMNS and re-running your test:

$ maintainer-merge verify 424 9c815c21 'chain_row_columns_pinned' \
    's/prev_chain_hash, chain_hash, chain_version, caller_role/…, caller_roleX/' rust
  rust suite: evidence of 1 executed unit(s), passing unmutated
  applying the mutation and re-running
  receipt recorded for #424 at 9c815c21 (observed: clean pass, mutated fail)

  observed_failure: test store::tests::chain_row_columns_pinned ... FAILED

A change-detector test is usually a smell, and this one is not, for a reason worth writing down: the positional SQLite mapper and the by-name Postgres mapper both depend on this list and neither can tell you when it moves. The pin is the only thing that can. Your comment says as much, and naming the miss that caused it, a chain_version column added to the mapper and one of two queries, surfacing only as a live-Postgres no column found for name, is what will keep the next person from "tidying" it away.

docs/distro-support.md, docs/introduction.md and README.md were not covered by that receipt and the gate said so out loud. docs-and-hygiene covers them and it is green, and scripts/check_evidence_claims.py re-derives every one of those figures from the tree, so a wrong number there turns the board red rather than getting published.

Thanks for this one. It is a small diff that removes a whole failure mode.