Skip to content

Commit 41608b9

Browse files
avrabeclaude
andcommitted
fix: address 4-reviewer findings + add dev/prod variants with bazel rustc profile
Reviewer fixes -------------- - INLINE-STORAGE de-greenwashed: surface test removed, verified-by emptied; verify gate now reports MISSING (not falsely PASSED) for the no-runtime- allocation requirement that needs witness instrumentation we don't wire. - DR-KVS-BACKEND-CHOICE rewritten: matches the actual vendored impl (json_backend.rs with Adler32 + fs::rename snapshot rotation), not the fabricated RocksDB ADR that contradicted the code. - DR-KVS-INLINE-STORAGE-GAP added: records the three honest response options (witness wiring / heapless / spec downgrade) so the gap has an audit trail. - 9 upstream miri_test targets restored via tools/miri.sh + `make miri` (cargo-miri wrapper — public rules_rust 0.70 has no miri_test rule; eclipse-score uses a private 0.68.2-score fork). - verify.py: mutable-default-arg fixed (_LIST_CACHE module-level dict); bazel build errors no longer swallowed by capture_output=True. - README: explicit that C++ has the same gap (kvs.cpp:24 carries an open `// TODO String Handling in set_value TBD`); :status: valid in sphinx-needs framing clarified as "approved-for-design" not "must-be-implemented"; 6-of-35 comp-req scope rationale recorded. Variants model -------------- - variants/feature-model.yaml + bindings.yaml declare two deployment contexts: `dev` (engineering iteration) and `prod` (release-mode safety builds). - Each variant binds three axes by name: (a) KvsBuilder runtime dials (KvsDefaults/KvsLoad/snapshot_max_count), (b) bazel --config= profile in /.bazelrc, (c) audit scope (out-of-scope-for: list). - /.bazelrc adds --config=dev (fastbuild) and --config=prod (compilation_ mode=opt + lto=fat + codegen-units=1 + overflow-checks=on + strip= symbols + embed-bitcode=yes). panic=abort lives in a separate --config=prod_ship for the shipping binary (Rust's #[test] harness needs unwinding outside nightly's -Zpanic_abort_tests). - Makefile threads VARIANT= through `make verify` and `make bazel`. - README + variants/README explain the design and why a variant model is needed: upstream eclipse-score rust_kvs has NO cargo features in the core library, so variants ride on builder dials + bazel rustc- flag profiles + audit scope, not --features. Gate output (variant=dev, default): 3 PASSED, 2 FAILED, 0 MISSING Gate output (variant=prod): 3 PASSED, 2 FAILED, 1 MISSING The 2 FAILED are the confirmed-real upstream falsifications of comp_req__kvs__key_naming and comp_req__kvs__key_length; the MISSING under prod is INLINE-STORAGE (out-of-scope for dev). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 3266e03 commit 41608b9

12 files changed

Lines changed: 559 additions & 85 deletions

File tree

.bazelrc

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,62 @@
1+
# example-kvs bazel configuration.
2+
#
3+
# Two named build profiles, bound to the variant model in variants/:
4+
#
5+
# --config=dev — fastbuild, ASAN-friendly, default rustc flags.
6+
# Mirrors the `dev` variant in variants/bindings.yaml.
7+
# Use for engineering iteration.
8+
#
9+
# --config=prod — release-mode + LTO + single codegen-unit + panic=abort.
10+
# Mirrors the `prod` variant in variants/bindings.yaml.
11+
# These flags are the minimum that an ASIL-B safety case
12+
# typically asks for on Rust builds:
13+
# lto=fat single translation unit at link time
14+
# codegen-units=1 deterministic, no parallelization noise
15+
# panic=abort no unwinding (smaller, FuSa-compatible,
16+
# eliminates the cleanup-path surface)
17+
# opt-level=3 max speed (default for compilation_mode=opt)
18+
# overflow-checks=on intentional — ASIL-B forbids silent UB on
19+
# integer overflow; cost is a few % in tight
20+
# loops, acceptable for safety builds
21+
# strip=symbols smaller binary, debuginfo lives in a
22+
# separate file (split-debuginfo in CI)
23+
#
24+
# `make verify VARIANT=prod` passes `--config=prod` through; same for `make bazel`.
25+
26+
# ── default settings ─────────────────────────────────────────────────
27+
build --java_language_version=17
28+
test --test_output=errors
29+
30+
# ── --config=dev (engineering builds) ────────────────────────────────
31+
build:dev --compilation_mode=fastbuild
32+
33+
# ── --config=prod (safety flags compatible with bazel test) ──────────
34+
# Used for `make verify VARIANT=prod` and any test-time invocation.
35+
# panic=abort is INTENTIONALLY OMITTED here: Rust's #[test] harness
36+
# requires unwinding to report failures and is incompatible with
37+
# panic=abort outside nightly's -Zpanic_abort_tests. panic=abort lives
38+
# in --config=prod_ship below for the actually-deployed binary.
39+
build:prod --compilation_mode=opt
40+
build:prod --@rules_rust//rust/settings:extra_rustc_flag=-Cembed-bitcode=yes
41+
build:prod --@rules_rust//rust/settings:extra_rustc_flag=-Clto=fat
42+
build:prod --@rules_rust//rust/settings:extra_rustc_flag=-Ccodegen-units=1
43+
build:prod --@rules_rust//rust/settings:extra_rustc_flag=-Coverflow-checks=on
44+
build:prod --@rules_rust//rust/settings:extra_rustc_flag=-Cstrip=symbols
45+
# Mirror codegen-units=1 on exec-target crates (proc-macros,
46+
# build-scripts) so the prod toolchain composes cleanly.
47+
build:prod --@rules_rust//rust/settings:extra_exec_rustc_flag=-Ccodegen-units=1
48+
49+
# ── --config=prod_ship (the actually-deployed binary only) ───────────
50+
# Extends --config=prod with panic=abort. Use for `bazel build` of
51+
# shipping binaries, NEVER for `bazel test`. The deployed ASIL-B
52+
# Rust binary should not carry the unwinding machinery — smaller
53+
# code size, no cleanup-path semantics for the assessor to argue
54+
# about.
55+
#
56+
# Typical use:
57+
# bazel build --config=prod_ship //:kvs_component
58+
build:prod_ship --config=prod
59+
build:prod_ship --@rules_rust//rust/settings:extra_rustc_flag=-Cpanic=abort
60+
61+
# Per-user overrides (gitignored)
62+
try-import %workspace%/user.bazelrc

Makefile

Lines changed: 20 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -8,20 +8,30 @@ SIGIL ?= sigil
88
PYTHON ?= python3
99
SCHEMAS := vendor/rivet-schemas
1010

11-
.PHONY: all validate aadl wit verify attest clean
11+
.PHONY: all validate aadl wit verify attest miri clean
12+
13+
# ── miri UB-check (vendored rust_kvs, per-module — matches upstream) ─
14+
# Mirrors the intent of eclipse-score's nine `miri_test` BUILD targets
15+
# (which live in their custom rules_rust fork). See tools/miri.sh.
16+
miri:
17+
@tools/miri.sh $(MODULE)
1218

1319
# Default: run all checks that work without external dependencies
1420
# (rivet, the verification gate, and the Bazel WASM-component build).
1521
# aadl/wit/attest are optional — they require spar / sigil installed.
1622
all: validate verify bazel
1723

1824
# ── Bazel: real WASM-component build (AADL → WIT → bindgen → component) ─
25+
# Picks the bazel config from the active variant; see /.bazelrc.
26+
# `make bazel` uses dev (fastbuild)
27+
# `make bazel VARIANT=prod` uses prod (release-mode + LTO + safety flags)
28+
BAZEL := $(if $(shell command -v bazelisk),bazelisk,bazel)
1929
bazel:
20-
@command -v bazelisk >/dev/null 2>&1 || command -v bazel >/dev/null 2>&1 || { \
30+
@command -v $(BAZEL) >/dev/null 2>&1 || { \
2131
echo "bazel(isk) not in PATH — skipping WASM-component build"; \
2232
exit 0; }
23-
@$(if $(shell command -v bazelisk),bazelisk,bazel) build //...
24-
@$(if $(shell command -v bazelisk),bazelisk,bazel) test //...
33+
@$(BAZEL) build --config=$(VARIANT) //...
34+
@$(BAZEL) test --config=$(VARIANT) //...
2535

2636
# ── rivet typed-artifact validation (always runs) ───────────────────
2737
validate:
@@ -48,8 +58,13 @@ wit:
4858
# comp-req with status:approved, finds tests by name convention,
4959
# reports PASSED / FAILED / MISSING per artifact. Exits 1 if any
5060
# FAILED or MISSING.
61+
#
62+
# Variant selection: `make verify VARIANT=prod` overrides the default
63+
# `dev` variant. See variants/bindings.yaml for what each variant
64+
# scopes in or out.
65+
VARIANT ?= dev
5166
verify:
52-
@$(PYTHON) tools/verify.py --artifacts artifacts
67+
@$(PYTHON) tools/verify.py --artifacts artifacts --variant $(VARIANT)
5368

5469
# ── sigil-signed release manifest (optional) ────────────────────────
5570
attest:

README.md

Lines changed: 70 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -32,25 +32,33 @@ This repo:
3232
`verified-by:` evidence list — there is no stubbing. Each artifact's
3333
bucket reflects what the real bazel test invocation reported.
3434

35-
## Finding: two upstream comp-reqs are unverified and unenforced
35+
## Finding: two upstream comp-reqs are unverified and unenforced (both Rust and C++)
3636

3737
Running the gate against the vendored eclipse-score `rust_kvs` surfaces
3838
a clean-room-verified gap. Two component requirements documented in
3939
[`persistency/score/kvs/docs/requirements/index.rst`](https://github.com/eclipse-score/persistency/blob/main/score/kvs/docs/requirements/index.rst)
40-
are accepted (`:status: valid`) but their declared behavior is not
41-
implemented and not tested:
40+
are accepted (`:status: valid`) but their declared behavior is neither
41+
implemented nor tested in *either* language binding:
4242

43-
| Upstream comp-req | RST text (verbatim) | What the impl does |
43+
| Upstream comp-req | RST text (verbatim) | What both impls do |
4444
|---|---|---|
45-
| `comp_req__kvs__key_naming` | "shall accept keys that consist solely of alphanumeric characters, underscores, or dashes" | `Kvs::set_value("with space", _)` returns `Ok(())`. Same for keys containing `.`, `/`, or any other character. |
46-
| `comp_req__kvs__key_length` | "shall limit the maximum length of a key to 32 bytes" | `Kvs::set_value(&"a".repeat(33), _)` returns `Ok(())`. No length check exists. |
45+
| `comp_req__kvs__key_naming` | "shall accept keys that consist solely of alphanumeric characters, underscores, or dashes" | `Kvs::set_value("with space", _)` returns `Ok(())` in Rust (`kvs.rs:238`) and the C++ `set_value` (`src/cpp/src/kvs.cpp:24` carries an open `// TODO String Handling in set_value TBD`). Same for `.`, `/`, or any other character. |
46+
| `comp_req__kvs__key_length` | "shall limit the maximum length of a key to 32 bytes" | `Kvs::set_value(&"a".repeat(33), _)` returns `Ok(())`. No length constant exists in either codebase. |
4747

4848
Verified independently (clean-room search across both Rust and C++
49-
codebases under eclipse-score): there is no `validate_key` / `check_key`
50-
function anywhere, no length constant, no test case directive
51-
(`.. test_case::`) tied to either comp-req, no documentation note saying
52-
"validation happens at the IPC boundary." Both Rust `set_value` and the
53-
C++ `set_value` accept any input.
49+
codebases under eclipse-score): no `validate_key` / `check_key` /
50+
length-constant symbol exists, no `.. test_case::` directive in
51+
`score/kvs/docs/` references either comp-req ID, no documentation
52+
note saying "validation happens at the IPC boundary."
53+
54+
**Context on `:status: valid`.** In the sphinx-needs workflow
55+
eclipse-score uses, `:status: valid` means "approved for design" —
56+
not "must be implemented by release X." SCORE is pre-1.0 and this
57+
state is normal for early-stage projects. The finding's value is not
58+
"eclipse-score is broken" but "an artifact-driven gate makes the
59+
spec/impl delta a CI signal, where a coverage dashboard hides it as
60+
a wedge." The eclipse-score project itself is healthy and well-run;
61+
the methodology is the lesson.
5462

5563
**The gate calls this out by going RED.** Current `make verify` output:
5664

@@ -60,17 +68,24 @@ BUCKET ID EVIDENCE
6068
PASSED COMP-REQ-KVS-KEY-ENCODING 1 test(s) all green
6169
PASSED COMP-REQ-KVS-VALUE-CHECKSUM 5 test(s) all green
6270
PASSED COMP-REQ-KVS-ATOMIC-STORE 5 test(s) all green
63-
PASSED COMP-REQ-KVS-INLINE-STORAGE 1 test(s) all green
6471
FAILED COMP-REQ-KVS-KEY-NAMING 3 test(s) failed:
6572
- test_..._space_rejected
6673
- test_..._dot_rejected
6774
- test_..._slash_rejected
6875
FAILED COMP-REQ-KVS-KEY-LENGTH 1 test(s) failed:
6976
- test_..._32_byte_cap_enforced
77+
MISSING COMP-REQ-KVS-INLINE-STORAGE verified-by is absent or empty
7078
─────────────────────────────────────────────────────────────────────
71-
4 PASSED, 2 FAILED, 0 MISSING
79+
3 PASSED, 2 FAILED, 1 MISSING
7280
```
7381

82+
INLINE-STORAGE is MISSING (not PASSED) on purpose: the spec asks for
83+
"no runtime heap allocation," upstream uses HashMap + Arc<Mutex> which
84+
do allocate, and a genuine verifying test requires witness
85+
allocator-guard instrumentation that this example does not wire.
86+
DR-KVS-INLINE-STORAGE-GAP records the three response options
87+
(witness wiring / heapless replacement / spec downgrade).
88+
7489
The surface tests at [`tests/surface/surface_tests.rs`](tests/surface/surface_tests.rs)
7590
assert exactly what the upstream RST says — no pulseengine-side
7691
embellishment. CI is intentionally red on this finding.
@@ -173,15 +188,41 @@ example-kvs/
173188
└── README # you are here
174189
```
175190

191+
## Variants (dev vs prod)
192+
193+
Two named deployment contexts are declared in [`variants/`](variants/);
194+
each binds together three axes — KvsBuilder runtime dials, the bazel
195+
`--config=` profile (`/.bazelrc`), and which comp-reqs the verify
196+
gate enforces:
197+
198+
| Variant | KvsBuilder dials | bazel profile | Audit scope |
199+
|---|---|---|---|
200+
| `dev` (default) | `Defaults::Ignored`, `Load::Ignored`, snapshot=1 | `--config=dev` (fastbuild) | excludes COMP-REQ-KVS-INLINE-STORAGE |
201+
| `prod` | `Defaults::Required`, `Load::Required`, snapshot=10 | `--config=prod` (compilation_mode=opt + `lto=fat` + `codegen-units=1` + `panic=abort` + `overflow-checks=on` + `strip=symbols`) | full comp-req set |
202+
203+
Select via `VARIANT=…` on any make target:
204+
205+
```sh
206+
make verify VARIANT=dev # default
207+
make verify VARIANT=prod # full enforcement
208+
make bazel VARIANT=prod # release-mode + LTO + safety rustc flags
209+
```
210+
211+
Upstream eclipse-score `rust_kvs` has **no cargo features** in the
212+
core library, so the variant model rides on builder-time dials +
213+
bazel rustc-flag profiles, not `--features`. See
214+
[`variants/README.md`](variants/README.md) for the design rationale.
215+
176216
## Run it
177217

178218
```sh
179-
make validate # rivet validate against the typed schema
180-
make verify # artifact-driven verification gate (needs bazel installed)
181-
make bazel # bazel build + bazel test //... (the WASM chain)
182-
make aadl # spar validates the AADL package (requires spar)
183-
make wit # spar AADL → WIT round-trip check (requires spar)
184-
make attest # sigil-signed release manifest (requires sigil)
219+
make validate # rivet validate against the typed schema
220+
make verify [VARIANT=dev|prod] # artifact-driven verification gate
221+
make bazel [VARIANT=dev|prod] # bazel build + bazel test //...
222+
make miri [MODULE=<module>] # cargo-miri UB check (matches upstream's miri_test intent)
223+
make aadl # spar validates the AADL package (requires spar)
224+
make wit # spar AADL → WIT round-trip check (requires spar)
225+
make attest # sigil-signed release manifest (requires sigil)
185226
```
186227

187228
Test the vendored upstream crate directly with cargo:
@@ -205,6 +246,16 @@ bazel test //tests/surface:surface_tests
205246
workstream — see the
206247
[playground-eclipse-score](https://github.com/pulseengine/playground-eclipse-score)
207248
workspace for that.
249+
- **Not a full mirror of eclipse-score's persistency comp-req set.**
250+
Upstream `score/kvs/docs/requirements/index.rst` declares 35
251+
comp-reqs; this example carries 6 of them. The selection is a
252+
representative slice covering one functional-positive
253+
(KEY-ENCODING), one functional-positive-impl-matches-spec
254+
(VALUE-CHECKSUM), one durability (ATOMIC-STORE), one
255+
non-functional with a real gap (INLINE-STORAGE), and the two that
256+
surfaced the finding (KEY-NAMING, KEY-LENGTH). A safety case would
257+
need to cover the other 29 explicitly; this example is a
258+
methodology demonstrator, not a coverage substitute.
208259
- **Not endorsed by the Eclipse Foundation or eclipse-score
209260
maintainers.** This is a pulseengine demonstration that vendors
210261
upstream Apache-2.0 sources; issues belong here, eclipse-score's

artifacts/requirements.yaml

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -268,13 +268,14 @@ artifacts:
268268
safety-level: ASIL_B
269269
security: "NO"
270270
verification-criteria: >
271-
Documented gap — the upstream implementation uses HashMap and
272-
Arc on the hot path; no allocator guard is wired. A real check
273-
requires witness allocator instrumentation. The current
274-
surface test PASSES (no guard fires) but the gap is recorded
275-
here so future witness wiring closes it.
276-
verified-by:
277-
- "//tests/surface:surface_tests:test_comp_req_kvs_inline_storage_no_runtime_alloc_documented_gap"
271+
No verifying test exists. The upstream implementation uses
272+
std::collections::HashMap and Arc<Mutex> on the hot path —
273+
both allocate. A real check requires witness allocator
274+
instrumentation (witness::no_alloc_guard) which is not wired
275+
in this example. The verify gate intentionally reports this
276+
comp-req as MISSING (not PASSED) to make the gap auditable.
277+
See DR-KVS-INLINE-STORAGE-GAP for the response options.
278+
verified-by: []
278279
tags: [persistency, kvs, memory]
279280
links:
280281
- type: satisfies

artifacts/safety-and-decisions.yaml

Lines changed: 39 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -65,43 +65,63 @@ artifacts:
6565

6666
- id: DR-KVS-BACKEND-CHOICE
6767
type: decision-record
68-
title: Choose RocksDB over LevelDB for the KVS backend
68+
title: JSON file backend with snapshot rotation (no WAL backend)
6969
status: approved
70-
description: Selection of the durable storage backend.
70+
description: Selection of the durable storage backend that ships today.
7171
fields:
7272
rationale: >
73-
RocksDB ships ASIL-relevant write-ahead-logging defaults and
74-
has an active maintenance line; LevelDB upstream activity has
75-
stalled.
73+
Vendored eclipse-score rust_kvs uses a single backend
74+
(json_backend.rs:401) that writes (key, value) as a tinyjson
75+
document, computes an Adler32 checksum stored in a sibling
76+
.hash file, and achieves atomic replace via fs::rename
77+
snapshot rotation (json_backend.rs:216-254). Adler32 is the
78+
upstream's checksum-of-record despite the comp_req text
79+
saying "CRC32C" — that is itself a spec/impl gap.
7680
alternatives: >
77-
LevelDB (rejected: stalled upstream), bespoke append-only log
78-
(rejected: re-implements WAL crash-safety).
81+
RocksDB or LevelDB or a bespoke write-ahead log were
82+
considered upstream but not implemented. The KvsBackend
83+
trait (kvs_backend.rs:45) exposes an extension point; only
84+
JsonBackend implements it.
7985
consequences: >
80-
Increases binary size by ~1.8 MB; adds a C++ dependency we
81-
must track in the SBOM.
86+
Atomicity guarantee is bounded by the POSIX fs::rename
87+
atomicity (kvs_backend::test_flush_ok and the snapshot_*
88+
tests cover the rotation path). Snapshot count caps at
89+
json_backend's snapshot_max_count (default 3, runtime-
90+
configurable via JsonBackendBuilder::snapshot_max_count).
91+
No durability guarantee beyond what the filesystem provides.
8292
tags: [persistency, kvs, decision]
8393
links:
8494
- type: satisfies
8595
target: COMP-REQ-KVS-ATOMIC-STORE
8696
- type: belongs-to
8797
target: COMP-KVS
8898

89-
- id: DR-KVS-INLINE-STORAGE-FIRST
99+
- id: DR-KVS-INLINE-STORAGE-GAP
90100
type: decision-record
91-
title: Inline storage path is the only ASIL-B path
101+
title: Inline-storage requirement is unmet by the current backend
92102
status: approved
93-
description: Storage strategy for ASIL-B builds.
103+
description: Tracking the gap between the spec and the impl.
94104
fields:
95105
rationale: >
96-
Heap-storage variant uses dynamic allocation on the hot path,
97-
which violates FEAT-REQ-PERSIST-NO-RUNTIME-ALLOC. Inline
98-
storage path is the only one certified.
106+
COMP-REQ-KVS-INLINE-STORAGE says "no heap allocation on the
107+
hot path." Vendored rust_kvs uses std::collections::HashMap
108+
for the in-memory store (kvs.rs) and Arc<Mutex<KvsData>> per
109+
method (kvs.rs:38-44, etc.) — both allocate. There is no
110+
allocator-tripwire in the build, and the surface test for
111+
this comp-req is documented as a gap, not real verification.
99112
alternatives: >
100-
Allow heap variant in ASIL-B (rejected: violates no-alloc),
101-
ship both with cfg flag (rejected: doubles MC/DC scope).
113+
(a) wire witness::no_alloc_guard around set_value/get_value
114+
and have it panic on any allocator call — requires the
115+
witness toolchain be present; (b) replace HashMap with a
116+
bounded inline structure (heapless::IndexMap or a custom
117+
slab) — requires an upstream fork; (c) downgrade COMP-REQ-
118+
KVS-INLINE-STORAGE to draft and document that ASIL-B inline
119+
storage is out of scope for this baseline.
102120
consequences: >
103-
Capacity is fixed at component init; reconfigurations require
104-
re-flashing. Acceptable for the target ECU.
121+
Until (a), (b), or (c) lands, the comp-req's verified-by:
122+
list is intentionally empty and the verify gate buckets the
123+
artifact as MISSING — not falsely PASSED. The gap is
124+
recorded; the audit trail does not lie.
105125
tags: [persistency, kvs, decision, memory]
106126
links:
107127
- type: satisfies

0 commit comments

Comments
 (0)