19 lines
5.5 KiB
Markdown
19 lines
5.5 KiB
Markdown
# Independent default-omission candidate review
|
||
|
||
Reviewer: `/root/default_omission_review`, 2026-09-13. Bounded static review of `harness/text-default-omission-design-20260913.md` against source commit `9fd76aef3ae2810667d3f2b031fc43692fc04c89`, branch `codex/text-diagnostics-9e8r6t`. Parent owns issue 11/comment 911 and coordination. No production edits, builds, runtime experiments, staging, or Gitea operations. The candidate and concurrent issue 12 work were left untouched. Read applicable local instructions and relevant user specification/checkpoint. Local `origin/HEAD` names the old `codex/behavior-draft-lifecycle` ref, where AGENTS.md is absent; this is not a fresh remote-instruction check or authority to discard current instructions.
|
||
|
||
## Findings
|
||
|
||
1. **P2 — Define CompleteForEdit as structurally complete, not the existing serializer with compact mode disabled.** Candidate lines 132–135 require complete comparison shapes, but do not explicitly require expansion of fields already suppressed by persistence serialization. Current extraction uses `serde_json::to_value` (`crates/oa-core/src/language.rs:675`, 691, 755–760); `OccurrenceBody::Exchange.operation`, `NodePlacement.port_anchors`, and `TraceBaseline.semantic` have `skip_serializing_if` (`model.rs:561`, 1556, 1604). Thus retaining today's writer as the complete branch does not satisfy the proposed preservation invariant. Concrete static witnesses: an authored `port_anchors [];` disappears when the same node's position changes; authored `operation none;` disappears when another Exchange member changes; authored `semantic none;` disappears when a prose snapshot inside the same baseline changes. The affected record is visited, and `SyntaxUpdate::record` erases actual fields absent from after (`source_project.rs:1293–1305`). All three explicit forms are admitted: node defaults at `language.rs:1841`, Exchange serde option at `model.rs:561–562`, and semantic null at `language/semantic_source.rs:107–109`. Recommendation: explicitly synthesize every admitted optional/default slot in comparison records at its applicable typed context, including these existing skips, while preserving JSON serialization. Add each witness with byte-level expected output. This is a design clarification before implementation acceptance; the candidate's general complete-shape invariant already points toward the correct fix.
|
||
|
||
2. **P2 — Correct the rename witness and strengthen the test trigger.** Candidate principal finding line 26 says a compact shared writer erases defaults when renaming an element. A pure name change does not exercise this failure: `update_bundle` calls `update.header` separately (`source_project.rs:932`), then `SyntaxUpdate::value` returns immediately when old/new fields compare equal (`1191–1192`; recursive equality at `1164–1175`). An alias-only change without a changed self-reference likewise leaves that record's fields equal. The erasure occurs when a semantic sibling in the same record changes. For example, change a Scenario precondition while its explicitly authored empty description, root parent, and other default fields are unchanged; compact after lacks those keys, so the record erases them. Recommendation: correct the asserted witness and require unrelated *payload* edits at declaration, selected-constructor, and nested-record levels, alongside existing Rename/Alias/no-op cases. Otherwise the proposed preservation checks can pass while the principal failure remains.
|
||
|
||
## Conclusions from attempted falsification
|
||
|
||
- The source/factory distinction is correct. `language.rs:1734–1742` explicitly overrides factory scenario viewpoints to MixedLegacy on omission; factories choose strict OA/SA viewpoints at `model.rs:804–808`. Source omission must govern export.
|
||
- Required/nested inventory is consistent with the targeted source: declaration merging is shallow (`language.rs:1995–2005`), mandatory declaration gates remain separate (`1720–1724`, `1785–1797`, `1837–1839`, `1850–1851`, `1862–1866`), and selected payload fields follow their own serde contract. In particular, String minimum_length, Signal items, Branch fields, and ScenarioUse bindings must survive. Option-field/spec ambiguity is already identified rather than concealed by the candidate; runtime confirmation remains outstanding.
|
||
- Keeping the complete comparison and compact insertion representations separate is necessary. New declarations consume `after[id]` (`source_project/source_structure.rs:204–223`), field insertion consumes after-text spans (`source_project.rs:1327–1336`), and shape replacement also consumes after text (`1243–1247`). A fragment renderer must retain typed path/context; pruning by the leaf's spelling alone is insufficient. Existing constructor-switch preservation is real (`1268–1289`) and must survive the split.
|
||
- Signed-zero policy is correct. Omission of node position must compare coordinate bits rather than ordinary equality; `Position` has floating coordinates and derived PartialEq (`model.rs:214–219`). The updater's final model-equality guard (`source_project.rs:1017–1025`) cannot establish signed-zero preservation. The candidate already calls for separate bit checks; execute them through both exporters and lossless edits.
|
||
|
||
**Recommendation:** retain candidate C's approach, revise the two points above before treating it as an implementation-ready contract, then run the proposed targeted evaluators. No static finding justifies generic pruning, changing parser admission, or treating peer source8 work as integrated. This review establishes neither runtime correctness nor issue closure.
|