h-dv/code-index

Watch

Fork 0

Approving a new version activates it only about half the time: #91 activates the lexicographically LOWEST digest, and a digest is a hash #242

New issue

Closed

opened 2026-09-09 14:34:28 +02:00 by buildagent · 1 comment

buildagent commented

2026-09-09 14:34:28 +02:00

Member

Found by the #237 lane. Operator decision taken: fix activation for both paths — the most-recently-approved digest activates, rather than the lexicographically-lowest one.

The defect

ApprovalRecord::set de-duplicates by digest. So when an operator approves a new version of a package they already have, both grants stand — and by #91 the one that activates is the lexicographically lowest digest.

A digest is a hash. There is no relationship between its byte ordering and which version is newer. So on any given pair it is a coin the bytes already flipped:

Roughly half of all upgrades leave the new version approved and shadowed, with the old one still extracting.

It is not an auto-update bug — `plugin add` has it too

This surfaced in #237 because an unattended apply has no reader, but the defect is in the manual path and predates auto-update entirely.

plugin add cannot even report it: domain_between runs before Store::install, so ApprovedPackageIds::resolve cannot see the candidate at the point the command would say something. A human meets the fact on their next plugin status, if they look — and they have no reason to look, because they just ran a command that succeeded.

So the current experience is: you approve v2, the command succeeds, and v1 keeps extracting. Silently, half the time.

Why "identical to `plugin add`" was the wrong acceptance criterion

#237 asked that an auto-apply leave state identical to the operator having run plugin add by hand. That is satisfiable, and it was the wrong target: parity with a path that silently no-ops half the time buys a feature that silently no-ops half the time. Worth recording, because the criterion looked obviously correct when it was written.

The decision

The most-recently-approved digest activates.

Chosen over the two alternatives deliberately:

Auto-update withdraws the superseded digest — fixes only the auto path, breaks #237's parity by design, removes an approval the operator made by hand, and leaves manual plugin add shadowing half the time. Rejected: it treats the symptom in one caller.
Ship the disclosure only — #237's update_active line already names when the new version is approved-but-shadowed and gives the repair, so nothing is silent. Rejected as an endpoint: honest is not the same as working, and there is no human present on the auto path to act on it.

Fixing activation fixes both callers from one clause, which is this repo's standing rule.

What this changes, and why it needs its own measurement

This changes #91's activation semantics for every package, not just updated ones. That is the whole risk, and it is why this is its own issue rather than a fold-in.

plugin_update's update_active disclosure stays either way: after this lands it should report the new digest as live, and the fact that it keeps working across the change is one of the cheaper checks available.

What a fix must prove

Approving v2 over v1 activates v2, for every digest ordering — including the case where v2's digest sorts below v1's. A fixture must construct both orderings; a test that happens to pick a favourable pair proves nothing, and there is a 50% chance of writing one by accident. This is the arm that decides the fix.
Manual plugin add and plugin update --auto reach the same activation, from the same clause.
Anti-vacuity, both arms: a package with exactly one approved digest still activates it (nothing regressed for the common case), and a withdrawn digest never activates however recently it was approved.
No existing activation moves except where it was wrong. Enumerate the activations before and after on a store with several packages and compare — a change that silently re-points an unrelated package is worse than the defect.
plugin rollback still works: rolling back to an older digest must activate the older one, which means "most recently approved" has to mean approval order, not version order or install time. Worth stating explicitly because those three coincide in the happy path and diverge exactly on rollback.

What must NOT be done

Do not make activation depend on a version string. Package versions are publisher-controlled text; #91 chose a digest ordering to avoid trusting them, and that reasoning still holds. The fix is to order by when the operator approved, a fact the host records.
Do not resolve it by forbidding two approved digests of one id. They exist for rollback, and #91 exists because they exist.
Do not change ApprovalRecord::set's de-duplication. Digest identity is correct; the bug is downstream, in which of the retained grants wins.

#237 (where this was found; its update_active disclosure is the interim honesty), #91 (the activation rule being changed), #86 F1 (undecline / withdrawn digests, which the fix must not disturb).

Filed 2026-09-09 against master f75ee20.

Found by the #237 lane. **Operator decision taken: fix activation for both paths** — the most-recently-approved digest activates, rather than the lexicographically-lowest one. ## The defect `ApprovalRecord::set` de-duplicates by **digest**. So when an operator approves a new version of a package they already have, **both grants stand** — and by #91 the one that activates is the **lexicographically lowest digest**. A digest is a hash. There is no relationship between its byte ordering and which version is newer. So on any given pair it is a coin the bytes already flipped: > **Roughly half of all upgrades leave the new version approved and shadowed, with the old one still extracting.** ## It is not an auto-update bug — `plugin add` has it too This surfaced in #237 because an unattended apply has no reader, but the defect is in the manual path and predates auto-update entirely. `plugin add` cannot even *report* it: `domain_between` runs before `Store::install`, so `ApprovedPackageIds::resolve` cannot see the candidate at the point the command would say something. A human meets the fact on their next `plugin status`, if they look — and they have no reason to look, because they just ran a command that succeeded. So the current experience is: you approve v2, the command succeeds, and v1 keeps extracting. Silently, half the time. ## Why "identical to `plugin add`" was the wrong acceptance criterion #237 asked that an auto-apply leave state *identical to the operator having run `plugin add` by hand*. That is satisfiable, and it was the wrong target: parity with a path that silently no-ops half the time buys a feature that silently no-ops half the time. Worth recording, because the criterion looked obviously correct when it was written. ## The decision **The most-recently-approved digest activates.** Chosen over the two alternatives deliberately: - *Auto-update withdraws the superseded digest* — fixes only the auto path, breaks #237's parity by design, removes an approval the operator made by hand, and leaves manual `plugin add` shadowing half the time. Rejected: it treats the symptom in one caller. - *Ship the disclosure only* — #237's `update_active` line already names when the new version is approved-but-shadowed and gives the repair, so nothing is silent. Rejected as an endpoint: honest is not the same as working, and there is no human present on the auto path to act on it. Fixing activation fixes both callers from one clause, which is this repo's standing rule. ## What this changes, and why it needs its own measurement This **changes #91's activation semantics for every package**, not just updated ones. That is the whole risk, and it is why this is its own issue rather than a fold-in. `plugin_update`'s `update_active` disclosure stays either way: after this lands it should report the new digest as live, and the fact that it *keeps working* across the change is one of the cheaper checks available. ## What a fix must prove - Approving v2 over v1 activates **v2**, for every digest ordering — including the case where v2's digest sorts *below* v1's. **A fixture must construct both orderings**; a test that happens to pick a favourable pair proves nothing, and there is a 50% chance of writing one by accident. This is the arm that decides the fix. - Manual `plugin add` and `plugin update --auto` reach the same activation, from the same clause. - **Anti-vacuity, both arms:** a package with exactly one approved digest still activates it (nothing regressed for the common case), and a *withdrawn* digest never activates however recently it was approved. - **No existing activation moves except where it was wrong.** Enumerate the activations before and after on a store with several packages and compare — a change that silently re-points an unrelated package is worse than the defect. - `plugin rollback` still works: rolling back to an older digest must activate the older one, which means "most recently approved" has to mean *approval order*, not *version order* or *install time*. Worth stating explicitly because those three coincide in the happy path and diverge exactly on rollback. ## What must NOT be done - **Do not make activation depend on a version string.** Package versions are publisher-controlled text; #91 chose a digest ordering to avoid trusting them, and that reasoning still holds. The fix is to order by *when the operator approved*, a fact the host records. - **Do not resolve it by forbidding two approved digests of one id.** They exist for rollback, and #91 exists because they exist. - **Do not change `ApprovalRecord::set`'s de-duplication.** Digest identity is correct; the bug is downstream, in which of the retained grants wins. ## Related #237 (where this was found; its `update_active` disclosure is the interim honesty), #91 (the activation rule being changed), #86 F1 (`undecline` / withdrawn digests, which the fix must not disturb). Filed 2026-09-09 against `master` `f75ee20`.

dhoyer referenced this issue from a commit

2026-09-09 17:09:30 +02:00

fix: the digest an operator approved most recently is the one that runs (#242)

dhoyer referenced this issue from a commit

2026-09-09 17:09:30 +02:00

merge lane/242: the most recently approved digest activates (#242)

buildagent commented

2026-09-09 20:19:15 +02:00

Author

Member

Fixed on master at 24486d5 (merged 0279c0b, in origin/master). 6 mutations run, 6 RED, 0 survivors — and the lane found three things wrong with this issue, two of which would have shipped a defect if followed literally.

The rule

Approval::approved_seq: u64, #[serde(default)], whose only writer is ApprovalRecord::set stamping max(existing) + 1. ApprovedPackageIds::from_grants sorts each id's digests by (Reverse(seq), digest ascending); active_digest takes first().

The signature change is load-bearing: from_pairs(Item = (String, String)) became from_grants(Item = (&Approval, String)), so the digest and its sequence arrive together, from the record. A caller cannot supply one without the other or pair a digest with somebody else's sequence. resolve and approved_activation_with_ids are the only two callers, and plugin add, plugin enable, plugin update --auto and the MCP plugin_add tool all write through set and read through resolve — so no call site was patched.

Ties keep #91's rule. A record written before the field reads back 0 everywhere, ties, and the lowest digest settles it — so nothing anyone holds today moves until they next approve something.

The before/after enumeration — exactly one row moved

Five ids, five states (solo, up, dn, gone, spare):

id               BEFORE (f236302)      AFTER (24486d5)
de.h-dv.dn       sha256:10d7b60d…      sha256:10d7b60d…   unchanged
de.h-dv.gone     sha256:8ae2aed6…      sha256:8ae2aed6…   unchanged
de.h-dv.solo     sha256:a330411d…      sha256:a330411d…   unchanged
de.h-dv.spare    (none)                (none)             unchanged
de.h-dv.up       sha256:a86b5bcb…      sha256:aceeb6e7…   MOVED

dn did not move because #91 already happened to pick the second approval there — which is what makes "only where it was wrong" a measurement rather than an assertion.

Both digest orderings, RED before / GREEN after

upgrade_pair(…, v2_sorts_below) measures candidate versions via StoredPackage::parse and returns a pair in the requested order, asserting the ordering held before anything else. Against master:

the_newly_approved_digest_activates_in_both_digest_orderings ... FAILED
  the digest approved MOST RECENTLY must activate, whether its bytes sort
  below the old one's (false) or above them

The v2_sorts_below = true arm passed against the unfixed binary — that is precisely the 50% that would have looked like proof. GREEN after: 13 passed.

Three corrections to this issue

1. plugin rollback is not the rollback command. It restores the retained index generation and its own doc says it "does NOT withdraw the approval". It names no package. The requirement I wrote was right; the command in it was wrong. The operator-level rollback is plugin enable <the older digest>, and that is what re_approving_an_older_digest_rolls_the_activation_back exercises, in both digest orderings.

2. I missed the witness, and following this issue literally would have shipped a live-daemon hole. approval::witness_in is what a running daemon polls to notice activation moved. A rollback re-approves identical bytes under an identical grant, so grants, capabilities, bridges, .cip lengths, sidecars and trust set are all byte-for-byte unchanged — the CLI would have printed the right thing while the daemon kept extracting under the version just rolled back from, silently and permanently until restart. seq= is now in the witness, with an anti-vacuity block proving nothing else about it moved.

3. "The bug is downstream, in which retained grant wins" is true of the decision but not of the data. The winner is decided downstream; the fact it needs can only be recorded upstream, in set. set's de-duplication is untouched — it now also stamps the order. Worth stating because my phrasing read as "do not touch set".

Also: _prdoc/specs/90-activation-on-demand.md had rejected this option as "needs a timestamp Approval does not carry, and adding one to answer a tie-break is a schema change for a diagnostic". Both halves are false — a counter needs no clock, and #[serde(default)] needs no schema bump. I verified the compatibility claim myself rather than accepting it: there is no deny_unknown_fields anywhere in approval.rs, so an older binary reading a new record ignores the field and falls back to #91's tie-break, which is what M242-3 grades. The spec bullet is corrected in place rather than deleted, so the original reasoning survives beside the reversal.

Mutations

	mutation	result
M242-1	`set` stamps `approved_seq = 0`	RED — 7/13 + witness test + 2 CLI files
M242-2	`from_grants` sorts by digest alone (#91 restored)	RED — 8/13, "the LATER approval wins, and it is not the lower digest"
M242-3	drop the tie-break	RED — exactly the two compatible-arm tests
M242-4	`next_approved_seq` returns `len() + 1`	RED — "a count would have handed it 2 and made the two tie"
M242-5	drop `seq=` from the witness	RED
M242-6a/b	`decline` keeps the grant / only ids with >1 grant	RED both

I re-ran M242-2 myself on the merged tree: RED across the suite. Restored by cp, md5 verified, re-run 13 passed.

Three tests that would have graded a coin were FIXED rather than reported as survivors: plugin_update_cli's two real packages happened to sort favourably, and plugin_duplicate_id_cli and duplicate_package_id_disclosure returned (min, max). Each now measures an unfavourable ordering deliberately.

A limitation operators will meet

The fix moves no activation on a record written before it. A project already holding two grants that never runs another approve command keeps its (possibly wrong) activation — there is no approval order recorded for those grants, and inventing one would be guessing. The repair is one command, and PACKAGE_ID_ALREADY_ACTIVE's witness now names it first: code-index plugin enable <this digest> re-approves and activates while keeping both grants.

Verification

fmt 0 · clippy --workspace --all-targets -D warnings 0 · RUSTDOCFLAGS="-D warnings" cargo doc --document-private-items 0 · cargo test --workspace 330 suites 0 failed · COSI_E2E_LEG=daemon 62 suites. tests/corpus/baseline.json untouched. CI 11/11 green on 1d3228e, which carries this commit. Post-push re-run by me: duplicate_package_id 13 passed, EXIT=0.

Closing — and noting that this issue sat fixed-and-open for several hours, which is the exact defect I closed six other issues for today.

Fixed on `master` at `24486d5` (merged `0279c0b`, in `origin/master`). **6 mutations run, 6 RED, 0 survivors** — and the lane found three things wrong with this issue, two of which would have shipped a defect if followed literally. ## The rule `Approval::approved_seq: u64`, `#[serde(default)]`, whose **only** writer is `ApprovalRecord::set` stamping `max(existing) + 1`. `ApprovedPackageIds::from_grants` sorts each id's digests by `(Reverse(seq), digest ascending)`; `active_digest` takes `first()`. The signature change is load-bearing: `from_pairs(Item = (String, String))` became `from_grants(Item = (&Approval, String))`, so **the digest and its sequence arrive together, from the record**. A caller cannot supply one without the other or pair a digest with somebody else's sequence. `resolve` and `approved_activation_with_ids` are the only two callers, and `plugin add`, `plugin enable`, `plugin update --auto` and the MCP `plugin_add` tool all write through `set` and read through `resolve` — so **no call site was patched**. **Ties keep #91's rule.** A record written before the field reads back `0` everywhere, ties, and the lowest digest settles it — so nothing anyone holds today moves until they next approve something. ## The before/after enumeration — exactly one row moved Five ids, five states (`solo`, `up`, `dn`, `gone`, `spare`): ``` id BEFORE (f236302) AFTER (24486d5) de.h-dv.dn sha256:10d7b60d… sha256:10d7b60d… unchanged de.h-dv.gone sha256:8ae2aed6… sha256:8ae2aed6… unchanged de.h-dv.solo sha256:a330411d… sha256:a330411d… unchanged de.h-dv.spare (none) (none) unchanged de.h-dv.up sha256:a86b5bcb… sha256:aceeb6e7… MOVED ``` `dn` did not move because #91 already happened to pick the second approval there — which is what makes *"only where it was wrong"* a measurement rather than an assertion. ## Both digest orderings, RED before / GREEN after `upgrade_pair(…, v2_sorts_below)` **measures** candidate versions via `StoredPackage::parse` and returns a pair in the requested order, asserting the ordering held before anything else. Against `master`: ``` the_newly_approved_digest_activates_in_both_digest_orderings ... FAILED the digest approved MOST RECENTLY must activate, whether its bytes sort below the old one's (false) or above them ``` **The `v2_sorts_below = true` arm passed against the unfixed binary** — that is precisely the 50% that would have looked like proof. GREEN after: 13 passed. ## Three corrections to this issue **1. `plugin rollback` is not the rollback command.** It restores the retained *index generation* and its own doc says it "does NOT withdraw the approval". It names no package. The requirement I wrote was right; the command in it was wrong. The operator-level rollback is `plugin enable <the older digest>`, and that is what `re_approving_an_older_digest_rolls_the_activation_back` exercises, in both digest orderings. **2. I missed the witness, and following this issue literally would have shipped a live-daemon hole.** `approval::witness_in` is what a running daemon polls to notice activation moved. A rollback re-approves **identical bytes under an identical grant**, so grants, capabilities, bridges, `.cip` lengths, sidecars and trust set are all byte-for-byte unchanged — the CLI would have printed the right thing while the daemon kept extracting under the version just rolled back *from*, silently and permanently until restart. `seq=` is now in the witness, with an anti-vacuity block proving nothing else about it moved. **3. "The bug is downstream, in which retained grant wins" is true of the decision but not of the data.** The winner is decided downstream; the *fact* it needs can only be recorded upstream, in `set`. `set`'s de-duplication is untouched — it now also stamps the order. Worth stating because my phrasing read as "do not touch `set`". Also: `_prdoc/specs/90-activation-on-demand.md` had rejected this option as *"needs a timestamp `Approval` does not carry, and adding one to answer a tie-break is a schema change for a diagnostic"*. Both halves are false — a counter needs no clock, and `#[serde(default)]` needs no schema bump. I verified the compatibility claim myself rather than accepting it: there is **no `deny_unknown_fields` anywhere in `approval.rs`**, so an older binary reading a new record ignores the field and falls back to #91's tie-break, which is what M242-3 grades. The spec bullet is corrected in place rather than deleted, so the original reasoning survives beside the reversal. ## Mutations | | mutation | result | |---|---|---| | M242-1 | `set` stamps `approved_seq = 0` | **RED** — 7/13 + witness test + 2 CLI files | | M242-2 | `from_grants` sorts by digest alone (#91 restored) | **RED** — 8/13, *"the LATER approval wins, and it is not the lower digest"* | | M242-3 | drop the tie-break | **RED** — exactly the two compatible-arm tests | | M242-4 | `next_approved_seq` returns `len() + 1` | **RED** — *"a count would have handed it 2 and made the two tie"* | | M242-5 | drop `seq=` from the witness | **RED** | | M242-6a/b | `decline` keeps the grant / only ids with >1 grant | **RED** both | I re-ran **M242-2** myself on the merged tree: RED across the suite. Restored by `cp`, md5 verified, re-run 13 passed. **Three tests that would have graded a coin were FIXED rather than reported as survivors:** `plugin_update_cli`'s two real packages happened to sort favourably, and `plugin_duplicate_id_cli` and `duplicate_package_id_disclosure` returned `(min, max)`. Each now measures an unfavourable ordering deliberately. ## A limitation operators will meet **The fix moves no activation on a record written before it.** A project already holding two grants that never runs another approve command keeps its (possibly wrong) activation — there is no approval order recorded for those grants, and inventing one would be guessing. The repair is one command, and `PACKAGE_ID_ALREADY_ACTIVE`'s witness now names it first: `code-index plugin enable <this digest>` re-approves and activates while keeping both grants. ## Verification `fmt` 0 · `clippy --workspace --all-targets -D warnings` 0 · `RUSTDOCFLAGS="-D warnings" cargo doc --document-private-items` 0 · `cargo test --workspace` 330 suites 0 failed · `COSI_E2E_LEG=daemon` 62 suites. `tests/corpus/baseline.json` untouched. CI **11/11 green** on `1d3228e`, which carries this commit. Post-push re-run by me: `duplicate_package_id` 13 passed, EXIT=0. Closing — and noting that this issue sat fixed-and-open for several hours, which is the exact defect I closed six other issues for today.

buildagent closed this issue

2026-09-09 20:19:19 +02:00

dhoyer referenced this issue from a commit

2026-09-09 22:06:45 +02:00

test: the #242 upgrade fixture stops depending on the process id

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

h-dv/code-index#242

No description provided.

Rows
Columns

Approving a new version activates it only about half the time: #91 activates the lexicographically LOWEST digest, and a digest is a hash #242

The defect

It is not an auto-update bug — plugin add has it too

Why "identical to plugin add" was the wrong acceptance criterion

The decision

What this changes, and why it needs its own measurement

What a fix must prove

What must NOT be done

Related

The rule

The before/after enumeration — exactly one row moved

Both digest orderings, RED before / GREEN after

Three corrections to this issue

Mutations

A limitation operators will meet

Verification

It is not an auto-update bug — `plugin add` has it too

Why "identical to `plugin add`" was the wrong acceptance criterion