Skip to content

Commit dee5b27

Browse files
committed
feat: contract diffs + breaking-change gates — structured contract analysis, declared renames, persisted contracts
Bundle 7 on top of shared state/cache. Turns the opaque contract_hash change into a structured, machine-readable contract diff and gates promotion on breaking contract changes, computed live. - contracts.rs: contract_diff classifies SAFE/REVIEW/BREAKING per change — contract added/dropped, enforcement on/off, column added/removed/renamed, type changes (numeric widening review, incompatible breaking), nullability tighten/relax. - Declared renames: [model.<name>.renames] new = "old" resolves a removed+added pair into a reviewable rename in both contract and physical schema diffs. Stale or ambiguous declarations resolve nothing — they cannot hide a real removal or addition. - MaterializedRecord persists the full contract (contract_json, SQLite + Postgres with migration), so base/target comparisons work against what the environment actually recorded. - BranchDiffReport gains contract_changes and lineage-aware impacts naming the downstream models/tests a breaking change affects. - Promotion: contract breaks join physical schema breaks in the schema gate, computed live against the target's recorded contracts — a contract edited after `diff` cannot slip past on a stale artifact. --allow-breaking-schema waives both. - plan/explain reasons name the changed columns and breaking count instead of "contract or assertions changed".
1 parent 86d2e38 commit dee5b27

25 files changed

Lines changed: 1254 additions & 54 deletions

File tree

‎SPEC.md‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -811,6 +811,14 @@ type = "DOUBLE"
811811
nullable = false
812812
```
813813

814+
Column renames are declared alongside the contract so diffs can classify a
815+
removed+added pair as a rename rather than a breaking removal:
816+
817+
```toml
818+
[model.assay_results.renames]
819+
result = "legacy_result"
820+
```
821+
814822
Explicit contract files remain optional.
815823

816824
## 32. Inferred tests
@@ -1069,7 +1077,7 @@ phlo transform promote feature/new-assay --to main
10691077
phlo transform promote --from feature/new-assay --to main
10701078
```
10711079

1072-
Promotion is authorised by named gates, reported identically in human and JSON output: `run` (latest candidate run passed and validated the exact commit being promoted — a passed run is bound to the candidate head **after** its writes land, and a candidate that advanced since its run, or a run recorded before commit binding, fails), `tests` (no failed tests), `blocked` (no blocked/cancelled model, seed or test work), `schema` (a fresh audited diff inspected this pair at these commits and found no unwaived breaking changes — absent or stale evidence fails closed, never reading "no evidence" as "no changes"), `data_diff` (when `--require-diff` is set: a passing `--full` audited diff bound to this candidate→target pair at the commits being promoted — the artifact records both refs' resolved heads and is rejected when they no longer match — still fresh per recorded versions, and never a self-comparison; a single-model `diff.json` is never promotion evidence), `base` (the target still equals the commit the evidence was established against — the hash-bound artifact's recorded base, else the candidate's `created_from` provenance; unknown provenance fails rather than redefining the base as the current head, and the merge asserts the evaluated target hash) and `conflicts` (the merge check is clean). A candidate that advanced between gate evaluation and merge is refused rather than promoted unaudited. `--check` evaluates gates without merging; a passing promotion merges and persists a `PromotionRecord` (refs, hashes, plan/run ids, gate results, timestamp) in the state store.
1080+
Promotion is authorised by named gates, reported identically in human and JSON output: `run` (latest candidate run passed and validated the exact commit being promoted — a passed run is bound to the candidate head **after** its writes land, and a candidate that advanced since its run, or a run recorded before commit binding, fails), `tests` (no failed tests), `blocked` (no blocked/cancelled model, seed or test work), `schema` (a fresh audited diff inspected this pair at these commits and found no unwaived breaking changes — physical breaks come from the audited diff; contract breaks are computed live from the workspace's desired contracts against the target's recorded contracts, so a post-`diff` contract edit cannot slip past — and absent or stale evidence fails closed, never reading "no evidence" as "no changes"), `data_diff` (when `--require-diff` is set: a passing `--full` audited diff bound to this candidate→target pair at the commits being promoted — the artifact records both refs' resolved heads and is rejected when they no longer match — still fresh per recorded versions, and never a self-comparison; a single-model `diff.json` is never promotion evidence), `base` (the target still equals the commit the evidence was established against — the hash-bound artifact's recorded base, else the candidate's `created_from` provenance; unknown provenance fails rather than redefining the base as the current head, and the merge asserts the evaluated target hash) and `conflicts` (the merge check is clean). A candidate that advanced between gate evaluation and merge is refused rather than promoted unaudited. `--check` evaluates gates without merging; a passing promotion merges and persists a `PromotionRecord` (refs, hashes, plan/run ids, gate results, timestamp) in the state store.
10731081

10741082
Preconditions may include successful plan, successful execution, required tests passing, no blocking schema changes, no stale state and optional approval.
10751083

‎crates/phlo-transform-cli/src/main.rs‎

Lines changed: 145 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -24,11 +24,11 @@ use phlo_transform_engine::{
2424
adapter_default_schema, branch_diff, changed_models, collect_source_states, diff, diff_reasons,
2525
ensure_environment, evaluate_gates, materialized_for_environment, model_keys, promote,
2626
relation_for_source, retarget, seeds_for_environment, Adapter, ArtifactWriter,
27-
BranchDiffReport, BranchDiffRequest, CancelHandle, DatasetKind, DiffPolicy, DiffRequest,
28-
DiffStrategy, EnvironmentArtifact, EnvironmentSetup, EnvironmentSpec, ExecutionStatus,
29-
GateInput, Membership, Plan, PlanAction, PlanOptions, PlanReason, Planner, PostgresStateStore,
30-
PromotionRequest, ReasonKind, RetryPolicy, RunOptions, RunResult, Runner, SqliteStateStore,
31-
StateStore, SCHEMA_VERSION,
27+
BranchDiffReport, BranchDiffRequest, CancelHandle, ContractSafety, DatasetKind, DiffPolicy,
28+
DiffRequest, DiffStrategy, EnvironmentArtifact, EnvironmentSetup, EnvironmentSpec,
29+
ExecutionStatus, GateInput, Membership, Plan, PlanAction, PlanOptions, PlanReason, Planner,
30+
PostgresStateStore, PromotionRequest, ReasonKind, RetryPolicy, RunOptions, RunResult, Runner,
31+
SqliteStateStore, StateStore, SCHEMA_VERSION,
3232
};
3333
use phlo_transform_nessie::{NessieClient, NessieConfig, NessieRestClient};
3434
use phlo_transform_trino::{TrinoAdapter, TrinoConfig};
@@ -538,6 +538,7 @@ async fn run(cli: &Cli) -> Result<ExitCode, String> {
538538
} => {
539539
run_promote(
540540
cli,
541+
&compilation,
541542
candidate.as_deref(),
542543
to,
543544
*check,
@@ -1128,8 +1129,10 @@ fn build_nessie(cli: &Cli) -> Result<Arc<dyn NessieClient>, String> {
11281129
Ok(Arc::new(client))
11291130
}
11301131

1132+
#[allow(clippy::too_many_arguments)]
11311133
async fn run_promote(
11321134
cli: &Cli,
1135+
compilation: &Compilation,
11331136
candidate: Option<&str>,
11341137
to: &str,
11351138
check: bool,
@@ -1186,14 +1189,21 @@ async fn run_promote(
11861189
};
11871190

11881191
let environment = read_environment_for(cli, candidate);
1189-
let audit = audited_diff(
1192+
let mut audit = audited_diff(
11901193
cli,
11911194
state.as_deref(),
11921195
candidate,
11931196
to,
11941197
Some(&candidate_reference.hash),
11951198
Some(&target.hash),
11961199
);
1200+
// Contract breaks are computed live — the workspace's desired contracts
1201+
// against what the target environment last recorded — so a contract
1202+
// edited after `diff` cannot sneak a break past the gate on a stale
1203+
// artifact's analysis.
1204+
audit
1205+
.breaking_schema_changes
1206+
.extend(contract_breaking_changes(state.as_deref(), compilation, to));
11971207
let merge_check = nessie.can_merge(candidate, to).await.ok();
11981208

11991209
// The `base` gate needs a target commit the evidence was established
@@ -1346,6 +1356,40 @@ struct AuditEvidence {
13461356
schema_audited: bool,
13471357
}
13481358

1359+
/// Live contract analysis for the promotion gate: the workspace's desired
1360+
/// contracts against the contracts the target environment last recorded.
1361+
/// Returns the breaking subset in the same `model.column: detail` shape as
1362+
/// physical schema breaks.
1363+
fn contract_breaking_changes(
1364+
state: Option<&dyn StateStore>,
1365+
compilation: &Compilation,
1366+
to: &str,
1367+
) -> Vec<String> {
1368+
let Some(state) = state else {
1369+
return Vec::new();
1370+
};
1371+
let base = materialized_for_environment(state, to).unwrap_or_default();
1372+
let mut breaking = Vec::new();
1373+
for model in &compilation.models {
1374+
let name = model.id.logical_name();
1375+
let changes = phlo_transform_engine::contract_diff(
1376+
base.get(&name).and_then(|record| record.contract.as_ref()),
1377+
model.contract.as_ref(),
1378+
);
1379+
for change in changes {
1380+
if change.safety == ContractSafety::Breaking {
1381+
let subject = if change.column.is_empty() {
1382+
name.clone()
1383+
} else {
1384+
format!("{name}.{}", change.column)
1385+
};
1386+
breaking.push(format!("{subject}: contract {}", change.detail));
1387+
}
1388+
}
1389+
}
1390+
breaking
1391+
}
1392+
13491393
/// Read the audited diff artifact (`branch_diff.json` preferred, single-model
13501394
/// `diff.json` as fallback) and derive the evidence it carries for this
13511395
/// promotion: the diff verdict, why the artifact cannot be used, the breaking
@@ -1849,6 +1893,28 @@ fn run_state(cli: &Cli, action: &StateAction) -> Result<ExitCode, String> {
18491893
println!(" sources: {}", record.version.source_state_hash);
18501894
println!(" compiler: {}", record.version.compiler_version);
18511895
println!(" target: {}", record.version.target_hash);
1896+
if let Some(contract) = &record.contract {
1897+
let columns = contract
1898+
.columns
1899+
.iter()
1900+
.map(|column| column.name.clone())
1901+
.collect::<Vec<_>>()
1902+
.join(", ");
1903+
println!(
1904+
"Contract: {} ({} columns{})",
1905+
if contract.enforced {
1906+
"enforced"
1907+
} else {
1908+
"advisory"
1909+
},
1910+
contract.columns.len(),
1911+
if columns.is_empty() {
1912+
String::new()
1913+
} else {
1914+
format!(": {columns}")
1915+
}
1916+
);
1917+
}
18521918
}
18531919
}
18541920
}
@@ -3295,6 +3361,11 @@ async fn run_diff(
32953361
strategy,
32963362
policy: diff_policy(compiled.config.diff.as_ref()),
32973363
sample_fraction: sample,
3364+
renames: compiled
3365+
.contract
3366+
.as_ref()
3367+
.map(|contract| contract.renames.clone())
3368+
.unwrap_or_default(),
32983369
};
32993370

33003371
match diff(adapter, &request).await {
@@ -3480,6 +3551,24 @@ fn print_branch_diff_human(report: &BranchDiffReport) {
34803551
}
34813552
}
34823553
}
3554+
if !report.contract_changes.is_empty() {
3555+
println!();
3556+
println!("Contracts");
3557+
for diff in &report.contract_changes {
3558+
for change in &diff.changes {
3559+
let subject = if change.column.is_empty() {
3560+
diff.model.clone()
3561+
} else {
3562+
format!("{}.{}", diff.model, change.column)
3563+
};
3564+
println!(
3565+
" {subject}: {} [{}]",
3566+
change.detail,
3567+
change.safety.as_str()
3568+
);
3569+
}
3570+
}
3571+
}
34833572
if !report.rows.is_empty() {
34843573
println!();
34853574
println!("Rows");
@@ -4137,6 +4226,8 @@ mod tests {
41374226
base_hash: None,
41384227
datasets: Vec::new(),
41394228
schema_changes: Vec::new(),
4229+
contract_changes: Vec::new(),
4230+
impacts: Vec::new(),
41404231
rows: Vec::new(),
41414232
diffs: Vec::new(),
41424233
deep,
@@ -4286,6 +4377,7 @@ mod tests {
42864377
incremental_strategy: None,
42874378
incremental_key: None,
42884379
adapter: None,
4380+
contract: None,
42894381
run_id: "run-1".to_string(),
42904382
materialized_at: "t".to_string(),
42914383
})
@@ -4330,6 +4422,7 @@ mod tests {
43304422
incremental_strategy: None,
43314423
incremental_key: None,
43324424
adapter: None,
4425+
contract: None,
43334426
run_id: "run-1".to_string(),
43344427
materialized_at: "t".to_string(),
43354428
}
@@ -4534,4 +4627,50 @@ mod tests {
45344627
// Nor may a degenerate ref collapse onto the single-slot artifact.
45354628
assert_ne!(environment_artifact_name("///"), "environment.json");
45364629
}
4630+
4631+
fn contract(names: &[&str]) -> phlo_transform_core::ModelContract {
4632+
phlo_transform_core::ModelContract {
4633+
enforced: true,
4634+
columns: names
4635+
.iter()
4636+
.map(|name| phlo_transform_core::ColumnContract {
4637+
name: (*name).to_string(),
4638+
data_type: None,
4639+
nullable: None,
4640+
})
4641+
.collect(),
4642+
renames: Default::default(),
4643+
}
4644+
}
4645+
4646+
#[test]
4647+
fn contract_breaking_changes_reads_recorded_contracts() {
4648+
let state = SqliteStateStore::in_memory().expect("state");
4649+
let mut main_record = record("main.base", Some("main"), "v1");
4650+
main_record.contract = Some(contract(&["id", "legacy"]));
4651+
state.record_materialized(&main_record).expect("record");
4652+
4653+
// The workspace now declares a contract without `legacy` — removing
4654+
// it is a breaking change against the recorded base contract.
4655+
let mut base = phlo_transform_core::SemanticModel::in_memory(
4656+
ModelId::parse("main.base").expect("model id"),
4657+
"select 1 as id, 2 as legacy",
4658+
);
4659+
base.contract = Some(contract(&["id"]));
4660+
let compilation = compile(&phlo_transform_core::SemanticProject::in_memory(vec![base]));
4661+
assert!(compilation.is_ok(), "{:?}", compilation.diagnostics);
4662+
4663+
let breaks = contract_breaking_changes(Some(&state), &compilation, "main");
4664+
assert_eq!(breaks.len(), 1, "{breaks:?}");
4665+
assert!(breaks[0].contains("legacy"), "{breaks:?}");
4666+
4667+
// The default environment folds into `main`, so a default-env
4668+
// record feeds the same gate.
4669+
let state = SqliteStateStore::in_memory().expect("state");
4670+
let mut default_record = record("main.base", None, "v1");
4671+
default_record.contract = Some(contract(&["id", "legacy"]));
4672+
state.record_materialized(&default_record).expect("record");
4673+
let breaks = contract_breaking_changes(Some(&state), &compilation, "main");
4674+
assert_eq!(breaks.len(), 1, "{breaks:?}");
4675+
}
45374676
}

‎crates/phlo-transform-core/src/compile.rs‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -420,10 +420,16 @@ fn version_inputs(
420420
)
421421
})
422422
.collect();
423+
let renames: Vec<String> = contract
424+
.renames
425+
.iter()
426+
.map(|(new, old)| format!("{new}={old}"))
427+
.collect();
423428
format!(
424-
"enforced={};columns={}",
429+
"enforced={};columns={};renames={}",
425430
contract.enforced,
426-
columns.join(",")
431+
columns.join(","),
432+
renames.join(",")
427433
)
428434
})
429435
.unwrap_or_default();

‎crates/phlo-transform-core/src/config.rs‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,9 @@ pub struct ModelContractConfig {
7070
pub incremental: IncrementalModelConfig,
7171
/// `[model.<name>.diff]`.
7272
pub diff: DiffConfig,
73+
/// `[model.<name>.renames]` — column renames the model declares
74+
/// (`new name = "old name"`).
75+
pub renames: BTreeMap<String, String>,
7376
}
7477

7578
/// The `[model.<name>.diff]` settings.

‎crates/phlo-transform-core/src/discovery.rs‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -206,7 +206,7 @@ fn normalized_sections(config: &PhloConfig) -> BTreeMap<String, ModelContractCon
206206
}
207207

208208
fn contract_from_section(section: &ModelContractConfig) -> Option<ModelContract> {
209-
if !section.contract.enforced && section.columns.is_empty() {
209+
if !section.contract.enforced && section.columns.is_empty() && section.renames.is_empty() {
210210
return None;
211211
}
212212
Some(ModelContract {
@@ -223,6 +223,7 @@ fn contract_from_section(section: &ModelContractConfig) -> Option<ModelContract>
223223
nullable: column.nullable,
224224
})
225225
.collect(),
226+
renames: section.renames.clone(),
226227
})
227228
}
228229

‎crates/phlo-transform-core/src/semantic.rs‎

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
use crate::identity::{ModelId, SourceId};
88

99
/// A SQL scalar or nested type as understood by the compiler.
10-
#[derive(Clone, Debug, PartialEq, Eq)]
10+
#[derive(Clone, Debug, PartialEq, Eq, serde::Serialize, serde::Deserialize)]
1111
pub enum DataType {
1212
Boolean,
1313
TinyInt,
@@ -176,7 +176,7 @@ impl std::fmt::Display for DataType {
176176
}
177177

178178
/// Nullability of a column. `Unknown` is distinct from `Nullable`.
179-
#[derive(Clone, Copy, Debug, PartialEq, Eq)]
179+
#[derive(Clone, Copy, Debug, PartialEq, Eq, serde::Serialize, serde::Deserialize)]
180180
pub enum Nullability {
181181
NotNull,
182182
Nullable,
@@ -392,18 +392,23 @@ impl Assertion {
392392
}
393393

394394
/// An explicit column-level contract.
395-
#[derive(Clone, Debug, PartialEq, Eq)]
395+
#[derive(Clone, Debug, PartialEq, Eq, serde::Serialize, serde::Deserialize)]
396396
pub struct ColumnContract {
397397
pub name: String,
398398
pub data_type: Option<DataType>,
399399
pub nullable: Option<bool>,
400400
}
401401

402402
/// An explicit model contract.
403-
#[derive(Clone, Debug, Default, PartialEq, Eq)]
403+
#[derive(Clone, Debug, Default, PartialEq, Eq, serde::Serialize, serde::Deserialize)]
404404
pub struct ModelContract {
405405
pub enforced: bool,
406406
pub columns: Vec<ColumnContract>,
407+
/// Column renames the model declares (`new name` → `old name`), from
408+
/// `[model.<name>.renames]`: a removed+added pair that is a declared
409+
/// rename classifies as a rename, not a breaking removal.
410+
#[serde(default, skip_serializing_if = "std::collections::BTreeMap::is_empty")]
411+
pub renames: std::collections::BTreeMap<String, String>,
407412
}
408413

409414
/// Numeric tolerance for a diffed column.

‎crates/phlo-transform-core/tests/analysis.rs‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -330,6 +330,7 @@ fn enforced_contract_violation_is_an_error() {
330330
data_type: None,
331331
nullable: None,
332332
}],
333+
renames: Default::default(),
333334
});
334335
let compilation = compile(vec![model]);
335336
assert!(!compilation.is_ok());
@@ -349,6 +350,7 @@ fn contract_type_mismatch_is_reported() {
349350
data_type: Some(DataType::Varchar),
350351
nullable: Some(false),
351352
}],
353+
renames: Default::default(),
352354
});
353355
let compilation = compile(vec![model]);
354356
assert!(!compilation.is_ok());

‎crates/phlo-transform-core/tests/compilation.rs‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -484,6 +484,25 @@ fn discovery_attaches_contracts_and_generates_tests() {
484484
}));
485485
}
486486

487+
/// `[model.<name>.renames]` lands on the contract so schema and contract
488+
/// diffs can classify a remove+add pair as a rename.
489+
#[test]
490+
fn discovery_attaches_declared_renames() {
491+
let compilation = compile_fixture("contract-renames");
492+
assert!(compilation.is_ok(), "{:?}", compilation.diagnostics);
493+
494+
let contract = compilation
495+
.model(&ModelId::parse("assay.results").unwrap())
496+
.expect("model")
497+
.contract
498+
.as_ref()
499+
.expect("contract attached");
500+
assert_eq!(
501+
contract.renames.get("result_value").map(String::as_str),
502+
Some("concentration")
503+
);
504+
}
505+
487506
#[test]
488507
fn models_expose_workflow_ownership() {
489508
let compilation = compile_fixture("cross-workflow");

0 commit comments

Comments
 (0)