feat(numscript): immutable append-only versioned library (EN-1288)#1529
feat(numscript): immutable append-only versioned library (EN-1288)#1529Azorlogh wants to merge 10 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Approve — automated reviewThe PR introduces an immutable append-only versioned numscript library. All previously identified blockers and major issues have been resolved per the PR discussion record: checkpoint baseline seeding, CheckStore expected-latest baseline seeding, tagged build compile failures, restore log identity sourcing, full-field projection comparison, enum serialisation on the versions endpoint, idempotency key forwarding, restore replay content overwrite, the CLI --version flag validation, and the invariant-8 CheckStore gap are all confirmed fixed. The single remaining open item flagged earlier (silent drop of deleted numscripts in config diff) was noted as a UX gap rather than a correctness blocker and does not warrant blocking the merge. The independent review found no new actionable issues. The PR is ready to merge. No findings. |
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 1 new inline finding.
Summary: #1529 (comment)
cc539f7 to
4f82980
Compare
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 2 new inline findings.
Summary: #1529 (comment)
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (54.67%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## release/v3.0 #1529 +/- ##
================================================
- Coverage 74.75% 74.47% -0.29%
================================================
Files 430 430
Lines 45722 45952 +230
================================================
+ Hits 34179 34222 +43
- Misses 8495 8629 +134
- Partials 3048 3101 +53
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 2 new inline findings.
Summary: #1529 (comment)
22df42e to
6785089
Compare
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 1 new inline finding.
Summary: #1529 (comment)
|
Review note for current head I rechecked this after the rebase. The restore replay and preload issues I had seen on the previous head look fixed, but one invariant-8 gap remains:
This PR makes numscript library state a persisted, load-bearing projection. The top-level audit/log payloads are Those projections are then used by reads and admission: latest lookup resolves through However, That leaves tampering or projection drift undetected: a store can roll back the latest pointer, resurrect a deleted script, remove a content row, or alter stored content and still pass Focused test I ran on the current head:
|
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 2 new inline findings.
Summary: #1529 (comment)
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 2 new inline findings.
Summary: #1529 (comment)
|
@gfyrag — thanks, you were right; the invariant-8 gap is now closed in 5e6feb0.
Tamper tests cover content mismatch, resurrected delete, pointer drift, and missing/extra content, plus clean save/delete/restore cases. Invariant #8 and the checker docs are updated. Ran your command clean: Follow-up: NumaryBot flagged that the content pass only compared the |
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 1 new inline finding.
Summary: #1529 (comment)
gfyrag
left a comment
There was a problem hiding this comment.
Re-reviewed current head 8b6f93b.
No remaining actionable findings from my pass. The previous checker projection gap looks fixed: compareNumscripts now rebuilds expected Numscript content and pointer projections from the audit log, compares the full NumscriptInfo, and covers tampering/missing/extra/pointer/tombstone cases in tests. The previous backup and preload concerns also look stale/resolved.
Checks run:
- GOROOT= go test ./internal/application/check -run 'Numscript'
- GOROOT= go test ./internal/domain/processing -run 'Numscript'
- GOROOT= go test ./internal/query ./internal/application/admission ./internal/adapter/http -run 'Numscript|Restore|ListNumscript'
- GOROOT= go test ./internal/adapter/grpc -run 'Numscript|ListNumscripts'
- git diff --check release/v3.0...HEAD
gfyrag
left a comment
There was a problem hiding this comment.
Requesting changes because the version model needs an explicit product/design decision before this can merge.
The implementation is technically careful, but it currently conflates two different operations:
- restoring a deleted numscript;
- moving the default pointer of a live numscript to any previously stored version (rollback).
That second behavior is outside EN-1288's required outcome and implicitly turns latest into a mutable deployment channel. The current code also gives latest two meanings: an omitted version follows the stored pointer, while the literal "latest" addresses a separate mutable content slot. As a result, a semver can be reported as ACTIVE while an explicit version=latest resolves different content.
We need to investigate and document a simpler model first. The direction currently under consideration is:
- immutable, explicit semver versions;
latestmeans the greatest stored semver;- deletion state is independent from the latest semver;
- restore clears deletion state and does not select an arbitrary version;
- removal of the mutable literal
"latest"content slot is acceptable.
Please pause model-dependent implementation work until that contract is settled. The history/listing work, explicit errors, audit/checker coverage, and backup fixes remain useful, but the restore API, pointer semantics, status model, tests, and docs will need to be realigned.
Final product-model decisionWe completed the model investigation triggered by the Changes Requested review. The authoritative plan is now in EN-1288. Immutable library
|
gfyrag
left a comment
There was a problem hiding this comment.
Re-reviewed against the settled product-model decision (EN-1288) — the requested realignment is fully applied: restore API removed, order no longer mutated by admission (FSM resolves "latest"→greatest semver, coverage-gated with a loud fail), derived status model removed (fields reserved), tests and docs realigned to the new contract. The parts I'd flagged as fine are intact (history/listing, explicit errors, backup fixes), and my invariant-#8 gap is closed by the new compareNumscripts checker pass. Build + numscript test packages green.
Two non-blocking notes: (1) the branch is currently DIRTY — please merge release/v3.0 (merge-base conflict only, compiles clean on the head); (2) a few stale comments still describe the old delete/restore/tombstone model (e.g. raft_cmd.proto:324, checker.go:189-192/951-952/971-972/1015) — worth a scrub since the code is now correct. Approving; those don't block.
4851a04 to
2cee04f
Compare
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 3 new inline findings.
Summary: #1529 (comment)
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 1 new inline finding.
Summary: #1529 (comment)
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 1 new inline finding.
Summary: #1529 (comment)
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot review complete: no remaining inline findings.
Resolved 1 stale NumaryBot review thread (1 fixed, 0 outdated).
Summary: #1529 (comment)
f68555f to
b5968fe
Compare
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 1 new inline finding.
Summary: #1529 (comment)
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 1 new inline finding.
Summary: #1529 (comment)
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 1 new inline finding.
Summary: #1529 (comment)
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 1 new inline finding.
Summary: #1529 (comment)
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot review complete: no remaining inline findings.
Resolved 1 stale NumaryBot review thread (0 fixed, 1 outdated).
Summary: #1529 (comment)
Rework the numscript library into an immutable, append-only versioned store, and bring both of its persisted projections under checker verification (invariant #8). Model: - Save requires an explicit full semver; content entries are immutable (duplicate -> NUMSCRIPT_VERSION_ALREADY_EXISTS, non-semver -> NUMSCRIPT_INVALID_VERSION). No delete, no restore, no tombstones. - Each name keeps a `latest` pointer equal to the greatest stored semver, advanced to max(current, saved) so versions may be saved out of order. - References keep the "latest"/exact selector in the audited order; admission plans the reads (declares the latest pointer + the discovered greatest's content coverage) without mutating the order, and the FSM resolves "latest" at apply via the pointer, guarded by a coverage check that yields ErrStaleProposal (retryable) on cross-proposal skew. Checker: compareNumscripts re-derives SubAttrNumscriptContent (immutable per-version entries, full NumscriptInfo compare) and SubAttrNumscriptVersion (per-name greatest-semver pointer) from SavedNumscript/DeleteLedger logs. Includes proto rescope, admission greatest-wins intra-bulk overlay, query version-history response, HTTP/gRPC/CLI surface, backup rebuild, docs, and OpenAPI.
- checker: seed the numscript projections from the boundary-time baseline (foldBaselineNumscripts) so a post-archive out-of-order save no longer makes CheckStore expect a lower latest than the store holds, and archived immutable content stays verified; guard the expected-side loops with pendingCleanup to match compareReferences. - backup: seed numscriptGreatest from the checkpoint store's stored pointer on first encounter so an incremental delta save cannot regress a greater checkpoint latest pointer. - tests: drop/rewrite the remaining DeleteNumscript references in the tagged e2e, scenario, and antithesis-workload suites (removed with the alpha model) so those builds compile.
The server rejects an empty version with NUMSCRIPT_INVALID_VERSION, so mark the flag required and correct the help/examples for the immutable model.
Under the immutable model a save is only ever a new version, so an edited script body kept at the same version would emit a SaveNumscript the server rejects with NUMSCRIPT_VERSION_ALREADY_EXISTS. diffNumscripts now fails fast with a clear message telling the caller to bump the semver.
…comment The comment described the removed admission-rewrites-the-order behavior. The selector is now carried verbatim into the audited order and the FSM resolves "latest" at apply.
semver.Parse accepted leading zeros (01.0.0), which compare equal to the canonical form but differ as strings. Since the raw version string is the content key and latest pointer, a non-canonical save would diverge the live projection from the checker/rebuild derivation. Parse now rejects any input whose canonical form differs, so the save path returns NUMSCRIPT_INVALID_VERSION.
The config diff copied the desired version into SaveNumscript verbatim, so an omitted/partial/non-canonical version produced a clean-looking plan that only failed at apply with NUMSCRIPT_INVALID_VERSION. diffNumscripts now runs the desired version through semver.Parse and fails the diff with a clear message.
The exported config holds only the greatest version per script, and latest is always the greatest stored semver, so a desired version at or below the current greatest can never become latest — the plan re-emits the save forever and fails with NUMSCRIPT_VERSION_ALREADY_EXISTS once stored. diffNumscripts now requires the desired version to be strictly greater than the current greatest.
compareNumscripts mirrored compareIndexes' archive-orphan tolerance, but the numscript projections are baseline-seeded (foldBaselineNumscripts) and Check() early-returns when archived-without-baseline. So under archiving the baseline is always present and baseline+replay is the complete expected set: the tolerance was redundant for legit rows and a hole that silently accepted an injected content/latest row absent from both. Drop the tolerance (and the now-dead activity tracking); an unmatched stored row is flagged NUMSCRIPT_MISMATCH.
b903428 to
4283cc5
Compare
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot posted 1 new inline finding.
Summary: #1529 (comment)
The append-only library cannot delete a version, so a script present in the store but dropped from the desired config made an unreachable desired state that the diff silently ignored (apply reported "already up to date"). diffNumscripts now fails with a clear message, consistent with the declarative remove semantics of the other config sections.
NumaryBot
left a comment
There was a problem hiding this comment.
NumaryBot review complete: no remaining inline findings.
Resolved 1 stale NumaryBot review thread (0 fixed, 1 outdated).
Summary: #1529 (comment)
Summary
Rework the numscript library into an immutable, append-only versioned store, per the EN-1288 product-model decision, and bring both of its persisted projections under checker verification (invariant #8).
This supersedes the earlier alpha model (mutable
latestslot + per-numscript soft delete/restore + derived tombstone status). Per-numscript deletion, restore, and tombstones are removed entirely — there is no compatibility or migration layer.Model
major.minor.patch). Saving with an empty,"latest", or partial version is rejected withNUMSCRIPT_INVALID_VERSION.(ledger, name, version)returnsNUMSCRIPT_VERSION_ALREADY_EXISTS. Changed content or a rollback is published under a new version.latestpointer equal to the greatest stored semver. A save advances it tomax(current, saved); versions may be saved out of order.DeleteLedgerstill cascades and removes all numscript data scoped to the ledger.What's included
SaveNumscript— the only write; explicit-semver, append-only (processSaveNumscript).GetNumscript— version selector:""/"latest"→ greatest semver; exact semver → that version; partial ("1","1.0") → highest match (read-only).ListNumscripts— the greatest version of each named script.ListNumscriptVersions— thelatest_versionpointer plus every stored version (highest semver first)."latest"/exact selector in the audited order; admission plans the reads (declares theSubAttrNumscriptVersionpointer + the discovered greatest'sSubAttrNumscriptContentcoverage) without mutating the order, and the FSM resolves"latest"at apply via the pointer, guarded by a coverage check that yieldsErrStaleProposal(retryable) on cross-proposal skew.compareNumscripts(invariant feat(CI): Add GoReleaser #8) — re-derivesSubAttrNumscriptContent(fullNumscriptInfocompare) andSubAttrNumscriptVersion(greatest-semver pointer) fromSavedNumscript/DeleteLedgerlogs; baseline-seeded under archiving so a post-archive out-of-order save cannot produce a falseNUMSCRIPT_MISMATCH.Full parity across gRPC, HTTP REST, and the
ledgerctlCLI (numscripts save --version,get,list,versions), plus OpenAPI, api-comparison, cli.md, the numscript-library architecture doc, the checker doc, and AGENTS.md invariant 8.Design decisions
latestis a pure projection, always the greatest stored semver — never an independent writable slot. Resolving it on the FSM apply path (which cannot read Pebble, invariant 3) is split: admission reads Pebble to plan/declare coverage without mutating the audited order, and the deterministic FSM resolves the pointer from the cache, falling back to a retryableErrStaleProposalwhen a concurrent proposal advanced the pointer between admission and apply.Testing
compareNumscriptsincl. baseline seeding + non-content-field tamper), state, and backup unit tests — all pass.tests/e2e/business/numscript_library_test.go(save → list → versions → immutability conflict → reference resolution) — compiled under-tags e2e, left for CI.just pre-commitclean (proto/dashboards/CRDs regenerated,go mod tidyclean, golangci-lint 0 issues in main + operator). e2e / scenario / antithesis-workload builds compile.