fix(spec): reject malformed IOA safety content (ARN-214) - #392
Conversation
Add fail-closed regressions for unknown tables/fields, truncated safety blocks, unnamed core declarations, malformed webhooks, duplicate keys, and context_entity round-trip. Record ADR-0179 as the intended remediation.
Parse each IOA document once into the canonical Automaton schema with deny_unknown_fields (ADR-0179). Drop the hand-rolled line scanner and silent section extractors so typos, truncated safety blocks, unnamed declarations, and malformed webhooks fail closed with source spans. - Compatibility deserializers keep supported string guards/effects - Uniqueness validation for safety-relevant declaration names - Idempotent external-trigger integration synthesis on round-trip - Fix in-tree fixtures that relied on silent drops - Corpus + migration differential coverage
Independent Grok code review (ARN-214 / ADR-0179)SummarySolid fail-closed parser rewrite: one full-document What was done well
FindingsImportant
Suggestions
Plan alignmentMatches ADR-0179 readiness gates: corpus parse/round-trip, malformed source fails before validation, declarations retained, no permissive mode. Competitor-independent ADR number is correct. Residual risks
No Critical correctness defects found in the HEAD diff. Verdict: PASS |
|
@greptile review |
ARENA SHIPPABLE · Grok · 2026-07-14 11:57 PDTPR: #392 Checklist
Summaryreject malformed IOA safety |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if !matches!( | ||
| effect_type, | ||
| "increment" | ||
| | "decrement" | ||
| | "set_counter_from_param" | ||
| | "set_bool" | ||
| | "emit" | ||
| | "emit_event" | ||
| | "list_append" | ||
| | "list_remove_at" | ||
| | "trigger" | ||
| | "schedule" | ||
| | "schedule_at" | ||
| | "spawn" | ||
| | "spawn_entity" | ||
| ) { | ||
| return Err(format!("unsupported effect type '{effect_type}'")); | ||
| } | ||
| Effect::deserialize(value).map_err(|error| error.to_string()) |
There was a problem hiding this comment.
deserialize_effect_entry checks a hardcoded list of effect type strings before delegating to Effect::deserialize, which already delegates to EffectDefinition with #[serde(tag = "type", deny_unknown_fields)]. Any effect type added to the enum in the future will parse correctly without touching this whitelist, but an unknown type will produce two different error messages depending on whether it hits the guard or serde — making the whitelist misleading rather than protective. The whitelist can be removed entirely; EffectDefinition provides the real enforcement.
| if !matches!( | |
| effect_type, | |
| "increment" | |
| | "decrement" | |
| | "set_counter_from_param" | |
| | "set_bool" | |
| | "emit" | |
| | "emit_event" | |
| | "list_append" | |
| | "list_remove_at" | |
| | "trigger" | |
| | "schedule" | |
| | "schedule_at" | |
| | "spawn" | |
| | "spawn_entity" | |
| ) { | |
| return Err(format!("unsupported effect type '{effect_type}'")); | |
| } | |
| Effect::deserialize(value).map_err(|error| error.to_string()) | |
| Effect::deserialize(value).map_err(|error| error.to_string()) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/temper-spec/src/automaton/toml_parser/compatibility.rs
Line: 84-102
Comment:
**Effect-type whitelist is a latent maintenance trap**
`deserialize_effect_entry` checks a hardcoded list of effect type strings before delegating to `Effect::deserialize`, which already delegates to `EffectDefinition` with `#[serde(tag = "type", deny_unknown_fields)]`. Any effect type added to the enum in the future will parse correctly without touching this whitelist, but an unknown type will produce two different error messages depending on whether it hits the guard or serde — making the whitelist misleading rather than protective. The whitelist can be removed entirely; `EffectDefinition` provides the real enforcement.
```suggestion
Effect::deserialize(value).map_err(|error| error.to_string())
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| fn main() -> ExitCode { | ||
| let args: Vec<String> = std::env::args().skip(1).collect(); | ||
| let mut args: Vec<String> = std::env::args().skip(1).collect(); | ||
| let liveness_mode = if args.first().is_some_and(|arg| arg == "--syntax-only") { | ||
| args.remove(0); | ||
| LivenessEnforcement::WarnOnly | ||
| } else { |
There was a problem hiding this comment.
--syntax-only still runs full semantic validation
The flag sets LivenessEnforcement::WarnOnly, but parse_automaton_with_liveness still validates state references, guard correctness, trigger field presence, cross-entity declarations, and all other semantic invariants in validate(). Pre-commit error messages say "Spec syntax error in $SPEC", which will mislead authors when the actual rejection is a semantic violation (e.g., from referencing an undeclared state). Consider renaming to --no-liveness or updating the pre-commit error message to "Spec validation error".
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/temper-spec/src/bin/verify_specs.rs
Line: 28-33
Comment:
**`--syntax-only` still runs full semantic validation**
The flag sets `LivenessEnforcement::WarnOnly`, but `parse_automaton_with_liveness` still validates state references, guard correctness, trigger field presence, cross-entity declarations, and all other semantic invariants in `validate()`. Pre-commit error messages say "Spec syntax error in $SPEC", which will mislead authors when the actual rejection is a semantic violation (e.g., `from` referencing an undeclared state). Consider renaming to `--no-liveness` or updating the pre-commit error message to "Spec validation error".
How can I resolve this? If you propose a fix, please make it concise.
|
@greptile review |
1 participant
Summary
Closes the silent-drop path in the IOA parser (ARN-214 / ADR-0179).
The hand-rolled line parser ignored unknown tables/fields and dropped unnamed safety blocks; webhook extraction turned parse failures into empty lists. Verification could not tell “not declared” from “parser discarded it.”
This PR parses each document once into the canonical
Automatonschema withdeny_unknown_fields, removes parallel section extractors, and fails closed with source spans.Changes
toml::from_strpath intemper-spec(drops hand-rolled grammar)deny_unknown_fieldson closed IOA records; compatibility deserializers for string guards/effectsstate_timeoutform, effect aliases, etc.)verify_specs --syntax-onlyfor pre-commit schema checksTest plan
cargo test -p temper-spec— 241 lib + ioa_corpus + migration_differential greenNotes
Greptile Summary
Replaces the hand-rolled IOA line-parser (≈750 LOC across
mod.rs,effects.rs,guards.rs,inline.rs) with a singletoml::from_strcall backed by#[serde(deny_unknown_fields)]on every schema type. This closes the silent-drop path where unknown tables, misspelled fields, truncated assertions, and unnamed blocks were previously accepted without error.deny_unknown_fieldsis added toAutomaton,Action,Invariant,Liveness,Webhook,StateTimeout,KeyDecl,VectorDecl,FieldInvariant,Admission,ActionTrigger, and the tagged enumsGuard/TargetResolver/TriggerGuard. Legacy string-form guards and effects are handled by purpose-built compatibility deserializers in the newcompatibility.rs, preserving backward compatibility while keeping the closed schema.expand_external_action_triggersswitches fromextendto an explicit (0,0)→push / (1,1)→skip / conflict→error loop, making parse→serialize→parse round-trips stable; theioa_corpustest verifies this across 130+ specs.validate_unique_namesis applied to every named entity type (states, actions, invariants, liveness, integrations, webhooks, context entities, field invariants, keys, vectors) before synthesis runs.Confidence Score: 5/5
Safe to merge. The single parse path, comprehensive deny_unknown_fields coverage, 130+ corpus round-trip tests, and 9 strictness regressions give strong assurance that the closed schema is correct and backward-compatible.
The core logic changes — schema-backed parsing, uniqueness validation, idempotent synthesis — are all correct and thoroughly tested. The only non-trivial concern is in the migration differential test helper, where a replacen call silently no-ops if the hardcoded duplicate-key pattern doesn't match the historical git blob, producing a confusing test failure rather than a clear fixture-mismatch signal. This is a test-reliability nit and not a production correctness issue.
migration_differential.rs — the replacen arm in repair_strict_historical_fixture for evolution_run.ioa.toml should assert that a replacement actually occurred, matching the expect-based approach used for the temper_agent.ioa.toml arm.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A["`**IOA TOML source**`"] --> B["toml::from_str\n(single parse pass)"] B -->|TOML error| E1["AutomatonParseError::Toml\n(with line/column span)"] B -->|Ok| C["validate(automaton)\n- uniqueness checks for all named entities\n- state/guard/liveness semantic checks"] C -->|validation error| E2["AutomatonParseError::Validation"] C -->|Ok| D["wire_state_timeout_from_states"] D --> F["expand_external_action_triggers\n- (0,0) → push synthesized integration\n- (1,1) identical → skip (idempotent)\n- else → conflict error"] F -->|conflict| E3["AutomatonParseError::Validation"] F -->|Ok| G["enforce_liveness (WarnOnly / Enforce)"] G --> H["✅ Automaton"]%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% flowchart TD A["`**IOA TOML source**`"] --> B["toml::from_str\n(single parse pass)"] B -->|TOML error| E1["AutomatonParseError::Toml\n(with line/column span)"] B -->|Ok| C["validate(automaton)\n- uniqueness checks for all named entities\n- state/guard/liveness semantic checks"] C -->|validation error| E2["AutomatonParseError::Validation"] C -->|Ok| D["wire_state_timeout_from_states"] D --> F["expand_external_action_triggers\n- (0,0) → push synthesized integration\n- (1,1) identical → skip (idempotent)\n- else → conflict error"] F -->|conflict| E3["AutomatonParseError::Validation"] F -->|Ok| G["enforce_liveness (WarnOnly / Enforce)"] G --> H["✅ Automaton"]Reviews (2): Last reviewed commit: "test(spec): skip missing IOA corpus root..." | Re-trigger Greptile