7.8 KiB
Independent review: S0 source-save containment
Reviewed exact Git blobs at 112e0d910bd2b2c9a191f29536fecb0fbc0177b0, parent 847b1caa7e75bb82f6ce5193c45bcfc45973ad1a. The author's changing worktree/S1 source was not used. No compilation, tests, application or tracked edits were performed. Findings below are source conclusions, not executed results.
No actionable production correctness defect found within S0's guard-only scope. The change prevents unreopenable or lossy generated source from reaching filesystem staging. It does not complete source8 or make this branch ready for ordinary source-save acceptance. One bounded public-entrypoint test coverage gap remains worth closing alongside S1; it does not invalidate the inspected guard implementation.
Verified guard ordering and scope
At persistence.rs:935–957, public save_checked passes the existing DOCUMENT profile and unchanged depth-admission flag through save_checked_profile to the new production helper, supplying the real language::format. Thus the test seam contains the actual production guard, not a separate test-only reconstruction.
At 969–1000, the helper validates the requested Project, selects source using the existing case-insensitive .arcadia extension rule, generates bytes, appends a newline only when missing, and passes those exact final bytes to public decode_with_report_limited. The effective byte limit is profile.bytes (DOCUMENT64MiB; smaller test profiles retain their exact limit). Other ParseLimits stay unrestricted because save_checked has no caller-supplied graph/record policy; S0 does not invent one. It then requires the decoded entire Project to equal the requested Project and refuses any migration report. The equality includes the new definitions catalog; no UUID mask, selected-field comparison or formatter-derived model expectation is used.
All those operations precede parent resolution and NamedTempFile::new_in at1006, writes/sync at1007–1008, conflict inspection at1009 and persist at1017. Decoder/validation failures return their existing typed error; lossy-output and unexpected-migration cases return distinct Invalid errors. There is no public decoder→save recursion introduced by this path. A generated source larger than the byte cap is rejected before decoding/IO, although the existing formatter still constructs its String before that cap check: this is not a new bounded-output allocator or a proof of arbitrary formatter stack/memory safety.
The depth_128 parameter continues to govern the JSON encoder. Source admission deliberately uses the public reader's production setting rather than the private candidate-depth override, matching the requested public save/reopen invariant. json_codec::DEPTH_128_VALIDATED remains false; no deeper-profile acceptance is implied. Existing source parser nesting constraints and formatter behavior remain separate from this small guard.
Sentinel evaluator assessment
The four new tests in persistence/source_save_tests.rs call save_checked_profile_with_source, injecting formatter output into the same guard used by production. They do not call the outer public save_checked function or exercise its real formatter output. This distinction should remain explicit in any test-result summary.
The evaluator is meaningful: each case tests an existing sentinel destination, a new destination and a destination under a missing parent; it requires the intended non-IO error, unchanged root membership, exact sentinel bytes and absence of new output. The missing-parent case is especially useful: moving NamedTempFile creation ahead of decoding would yield Io and fail the expected-error assertion even if temporary-file Drop would otherwise hide creation in the existing-parent cases. Inspection confirms no other filesystem effect precedes the guard.
- Malformed output (
arcadia) checks public parser refusal, rather than manufacturing an arbitrary error. - The lossy-output case adds a valid unused QuantityKind definition while injected source preserves familiar ID/name/elements and omits the definition. Full equality must reject it with the specific loss error. This catches an implementation that compares only the old graph fields. The equality check intentionally precedes migration refusal, so a schema7 decoded candidate can expose catalog loss distinctly.
- The equal-project legacy case reaches the separate migration refusal. Ordinary source7 decoding yields the current-schema8 Project with a migration report; equal meaning does not excuse emitting a legacy source representation.
- The newline test puts the unterminated UTF-8 source exactly at the byte cap, forcing the appended newline to exceed it. A preterminated version at the new exact cap reaches migration refusal instead, catching either checking before append or appending twice.
Coverage note, not a demonstrated production bug: none of these new sentinel checks protects the outer public call wiring against a later bypass of the shared helper. The existing public regressions at review_regressions.rs:454,472,490 are useful, but all iterate .arcadia before JSON and currently unwrap the first source save. At S0 they are expected to stop at that first save rather than reaching their JSON/conflict cases. Consequently neither passing these four new tests nor the still-failing public regression establishes current JSON/conflict execution coverage. The smallest later check is one public save_checked end-to-end invariant/sentinel case using the real formatter, plus separate existing JSON/conflict cases so a source failure cannot skip them. After S1, retain the fault-injection tests and run the ordinary public save/update/no-op/conflict/deletion regression with source and JSON both observed. Do not weaken the public regression to accept permanent source failure.
JSON, conflicts and integration limitation
The only production diff moves existing save logic into the private shared helper and adds the source-only check. JSON still calls the same encoder with the same profile/depth arguments. Existing newline, temporary write/sync, exact-byte conflict matching, missing-file Conflict and persist code are unchanged. Source admission now takes precedence over inspecting a stale destination; that is necessary for containment and preserves the external bytes, but it means S0 cannot demonstrate source Conflict for an unreopenable formatter output. This is source-based invariance, not a test pass.
At this exact commit, language::format writes schema8 while the language parser still accepts only3–7 (language.rs:1410). Therefore ordinary .arcadia saves remain unavailable, now safely returning an error before a destination is created/replaced. The previously executed847 regression failed after writing unreadable source; S0 is expected to fail its first unwrap instead. That changed failure position is a prediction until executed, and is containment, not repaired source8 round-trip capability. S1 must supply the complete current source representation/parser before normal source saving can be accepted. No JSON128 or broader analysis acceptance follows from this review.
Exact input identities
| Git blob path | SHA256 |
|---|---|
crates/oa-core/src/persistence.rs |
d7b33495e429a0bff65782be4f5295f8fedbd4f72ec394250919bb80ccf79d9e |
crates/oa-core/src/persistence/source_save_tests.rs |
a8eb528ac8f96c97292124409908476c25504303b11bd30b4c0358412e88edda |
crates/oa-core/src/persistence/json_codec.rs |
2aa2c52eea794acb85744440192df9ef5193160ad6ebd1febaa824e144aadd90 |
crates/oa-core/src/language.rs |
b65098321ecf79e4dc1182a7fab7bafeec0a238bb56153e81782f04fab838558 |
crates/oa-core/tests/review_regressions.rs |
f154e5c88d35e31512e2b07733a1327ea3e2fcfd1ef8eb23066a4e9573f387c1 |
The commit changes only persistence.rs and the new four-test source file. No S1 bytes were reviewed or endorsed. Execution and parent integration remain pending.