From 0295b4b7e02569189f4c835d67b9f8d8c795cb3a Mon Sep 17 00:00:00 2001 From: piaro <7055807+piaro@users.noreply.github.com> Date: Wed, 9 Sep 2026 15:50:32 +0900 Subject: [PATCH] Fix candidate review selection across stale results --- README.md | 6 + docs/concepts.ja.md | 2 + schemas/mcp/v1/next-output.schema.json | 1 + schemas/outputs/v1/next-response.schema.json | 1 + skill-src/adf-analyst/SKILL.md | 7 + src/context.rs | 36 +--- src/explain.rs | 71 +------- src/kernel.rs | 175 ++++++++++++++++--- tests/cli.rs | 84 +++++++++ 9 files changed, 271 insertions(+), 112 deletions(-) diff --git a/README.md b/README.md index 20ca9a2..e2c88c5 100644 --- a/README.md +++ b/README.md @@ -111,6 +111,12 @@ silently treated as no impact. This also gives an empty repository a valid bootstrap path: the first Change declares its intended effects, then creates only the Contracts needed to govern them before implementation starts. +Candidate reviews are resolved from their evidence, independently of Result ID or +file order. A current `not-applicable` review takes precedence over stale reviews +and proceeds to independent challenge. A `confirmed` review remains binding +across evidence changes and takes precedence if reviews disagree. CLI, MCP, +explanations, and challenge context use the same selection. + **Contracts going stale is a feature.** Each result is bound to digests of what it was based on, so changing a contract, the code, or the authority behind it marks the work that depended on it stale and asks for it again. A contract that diff --git a/docs/concepts.ja.md b/docs/concepts.ja.md index cbaa6e8..e50b250 100644 --- a/docs/concepts.ja.md +++ b/docs/concepts.ja.md @@ -41,6 +41,8 @@ Contractは変更のたびに増えます。障害から学んだことはテス コードがないことを、影響がないこととは扱いません。新規プロジェクトでは、最初の変更依頼から実現しようとしている振る舞いを特定し、その変更に必要な最小限のContractを作ってから実装へ進みます。 +検出候補の確認結果は、記録のIDや読み込み順ではなく、根拠が現在も有効かどうかで選びます。有効な「対象外」の確認結果があれば、古い結果で上書きせず、独立レビューへ進みます。「対象となる」と確認済みの候補は根拠の変更後も対象として扱い、結果が食い違う場合もこちらを優先します。CLIとMCP、判定詳細、レビュー用の情報は同じ選択結果を使います。 + 既存実装の移行では、検証するContract条項を`--verify-clause`で明示できます。指定した条項は未検証でもEvidence登録の対象になり、指定していない未検証条項は変更を止めません。既存の振る舞いを検証対象にするために、今回新たに生じる実装影響として申告する必要はありません。`evidence_mode: review`の条項は人のレビューを求めるため、この方法では選択できません。 ```text diff --git a/schemas/mcp/v1/next-output.schema.json b/schemas/mcp/v1/next-output.schema.json index e33810b..76153d4 100644 --- a/schemas/mcp/v1/next-output.schema.json +++ b/schemas/mcp/v1/next-output.schema.json @@ -1,4 +1,5 @@ { + "description": "Next work uses current candidate reviews regardless of Result ID or file order; stale non-applicability cannot hide a current review, and confirmation takes precedence.", "$schema": "https://json-schema.org/draft/2020-12/schema", "$id": "adf://schemas/mcp/v1/next-output", "title": "adf_next output v1", diff --git a/schemas/outputs/v1/next-response.schema.json b/schemas/outputs/v1/next-response.schema.json index 24d5eb9..e1a36bc 100644 --- a/schemas/outputs/v1/next-response.schema.json +++ b/schemas/outputs/v1/next-response.schema.json @@ -1,4 +1,5 @@ { + "description": "Next work uses current candidate reviews regardless of Result ID or file order; stale non-applicability cannot hide a current review, and confirmation takes precedence.", "$schema": "https://json-schema.org/draft/2020-12/schema", "$id": "https://agentic-development-framework.dev/schemas/outputs/v1/next-response.schema.json", "title": "Next Response v1", diff --git a/skill-src/adf-analyst/SKILL.md b/skill-src/adf-analyst/SKILL.md index 65e0703..4384cf9 100644 --- a/skill-src/adf-analyst/SKILL.md +++ b/skill-src/adf-analyst/SKILL.md @@ -112,6 +112,13 @@ Read the code the candidate points at. A candidate is not confirmed because its looks right, and not dismissed because it is inconvenient. If you cannot tell, mark the matching outcome `inconclusive` rather than guessing. +When changed evidence reopens a previously non-applicable candidate, submit a new +review using the issued context. ADF retains prior Results and selects a current +review regardless of Result ID or file order; do not delete prior Results to make +progress. Non-applicability still requires independent challenge. Confirmation +remains binding across evidence changes and takes precedence over conflicting +non-applicability. + ## `analyze-requirements` Answer each requirement instance in the action. Submit an `outcomes` entry per instance: diff --git a/src/context.rs b/src/context.rs index be41e6b..523ab3e 100644 --- a/src/context.rs +++ b/src/context.rs @@ -805,33 +805,15 @@ fn not_applicable_disposition<'a>( snapshot: &'a ProjectSnapshot, candidate: &crate::detection::SignalCandidate, ) -> Option<(&'a str, &'a str, &'a Value)> { - snapshot.results.iter().find_map(|result| { - if result["result_schema"].as_str() != Some("result.risk-signal-review") - || result["role"].as_str() != Some("Analyst") - { - return None; - } - let input_refs = result["input_refs"].as_object()?; - if !candidate.evidence_refs.iter().all(|reference| { - input_refs.get(reference).and_then(Value::as_str) - == snapshot.artifact_digests.get(reference).map(String::as_str) - }) { - return None; - } - array_field(&result["payload"], "reviewed_candidates") - .iter() - .find(|review| { - review["fingerprint"].as_str() == Some(candidate.fingerprint.as_str()) - && review["status"].as_str() == Some("not-applicable") - }) - .and_then(|review| { - Some(( - result["id"].as_str()?, - review["reason"].as_str()?, - &review["basis_refs"], - )) - }) - }) + let (result, review) = crate::kernel::current_candidate_review(snapshot, candidate)?; + if review["status"].as_str() != Some("not-applicable") { + return None; + } + Some(( + result["id"].as_str()?, + review["reason"].as_str()?, + &review["basis_refs"], + )) } fn array_field<'a>(value: &'a Value, field: &str) -> &'a [Value] { diff --git a/src/explain.rs b/src/explain.rs index f74d802..72b724b 100644 --- a/src/explain.rs +++ b/src/explain.rs @@ -7,7 +7,8 @@ use crate::detection::{DetectionReport, SignalCandidate}; use crate::kernel::{ - KernelDecision, ProjectSnapshot, RequirementInstance, outcome_has_current_evidence, + KernelDecision, ProjectSnapshot, RequirementInstance, current_candidate_review, + outcome_has_current_evidence, }; use crate::rules::{Assurance, RuleIndex}; use serde::Serialize; @@ -16,13 +17,6 @@ use std::collections::BTreeMap; pub const EXPLAIN_REPORT_SCHEMA_VERSION: &str = "1"; -#[derive(Debug, Clone, PartialEq, Eq)] -struct CandidateDispositionTrace { - status: String, - input_refs: BTreeMap, - result_id: Option, -} - #[derive(Debug, Clone, PartialEq, Eq, Serialize)] pub struct CandidateTrace { pub fingerprint: String, @@ -170,23 +164,17 @@ impl ExplanationBuilder { detection: &DetectionReport, decision: &KernelDecision, ) -> ExplainReport { - let dispositions = candidate_dispositions(snapshot); let candidates = detection .candidates .iter() .map(|candidate| { - let disposition = dispositions - .get(&candidate.fingerprint) - .cloned() - .filter(|disposition| { - disposition.status == "confirmed" - || candidate.evidence_refs.iter().all(|reference| { - snapshot.artifact_digests.get(reference) - == disposition.input_refs.get(reference) - && disposition.input_refs.contains_key(reference) - }) + let disposition = current_candidate_review(snapshot, candidate) + .map(|(result, review)| { + ( + review["status"].as_str().unwrap().to_owned(), + string_field(result, "id").map(str::to_owned), + ) }) - .map(|disposition| (disposition.status, disposition.result_id)) .unwrap_or_else(|| ("unreviewed".to_owned(), None)); candidate_trace(candidate, disposition, rule_index, decision, snapshot) }) @@ -221,49 +209,6 @@ impl ExplanationBuilder { } } -fn candidate_dispositions( - snapshot: &ProjectSnapshot, -) -> BTreeMap { - let mut dispositions = BTreeMap::new(); - for result in &snapshot.results { - if string_field(result, "result_schema") != Some("result.risk-signal-review") - || string_field(result, "role") != Some("Analyst") - { - continue; - } - for review in nested_array(result, &["payload", "reviewed_candidates"]) { - if let (Some(fingerprint), Some(status)) = ( - string_field(review, "fingerprint"), - string_field(review, "status"), - ) { - dispositions.insert( - fingerprint.to_owned(), - CandidateDispositionTrace { - status: status.to_owned(), - input_refs: string_map(result.get("input_refs")), - result_id: string_field(result, "id").map(str::to_owned), - }, - ); - } - } - } - dispositions -} - -fn string_map(value: Option<&Value>) -> BTreeMap { - value - .and_then(Value::as_object) - .map(|items| { - items - .iter() - .filter_map(|(key, value)| { - value.as_str().map(|value| (key.clone(), value.to_owned())) - }) - .collect() - }) - .unwrap_or_default() -} - fn candidate_trace( candidate: &SignalCandidate, disposition: (String, Option), diff --git a/src/kernel.rs b/src/kernel.rs index 7c484c7..7102ca1 100644 --- a/src/kernel.rs +++ b/src/kernel.rs @@ -185,7 +185,7 @@ impl ThinKernel { let declared_candidates = assessment .map(|assessment| impact_candidates(assessment, snapshot)) .unwrap_or_default(); - let dispositions = candidate_dispositions(snapshot); + let dispositions = candidate_dispositions(snapshot, detection); let build_results = fresh_build_results(snapshot); let mut confirmed: Vec<&SignalCandidate> = declared_candidates .iter() @@ -643,30 +643,66 @@ fn repository_phase(snapshot: &ProjectSnapshot) -> &str { snapshot.repository["phase"].as_str().unwrap_or("pre-build") } -fn candidate_dispositions(snapshot: &ProjectSnapshot) -> BTreeMap { - let mut dispositions = BTreeMap::new(); - for result in &snapshot.results { - if string_field(result, "result_schema") != Some("result.risk-signal-review") - || string_field(result, "role") != Some("Analyst") - { - continue; - } - for review in nested_array(result, &["payload", "reviewed_candidates"]) { - if let (Some(fingerprint), Some(status)) = ( - string_field(review, "fingerprint"), - string_field(review, "status"), - ) { - dispositions.insert( - fingerprint.to_owned(), +fn candidate_dispositions( + snapshot: &ProjectSnapshot, + detection: &DetectionReport, +) -> BTreeMap { + detection + .candidates + .iter() + .filter_map(|candidate| { + current_candidate_review(snapshot, candidate).map(|(result, review)| { + ( + candidate.fingerprint.clone(), CandidateDisposition { - status: status.to_owned(), + status: review["status"].as_str().unwrap().to_owned(), input_refs: string_map(result.get("input_refs")), }, - ); - } - } - } - dispositions + ) + }) + }) + .collect() +} + +/// Result IDs are content hashes, not submission order. Select a usable review +/// before resolving ties; stale non-applicability must never hide a current one. +/// Confirmation remains binding across evidence changes and wins conflicts. +pub(crate) fn current_candidate_review<'a>( + snapshot: &'a ProjectSnapshot, + candidate: &SignalCandidate, +) -> Option<(&'a Value, &'a Value)> { + snapshot + .results + .iter() + .filter(|result| { + string_field(result, "result_schema") == Some("result.risk-signal-review") + && string_field(result, "role") == Some("Analyst") + }) + .flat_map(|result| { + nested_array(result, &["payload", "reviewed_candidates"]) + .iter() + .map(move |review| (result, review)) + }) + .filter(|(result, review)| { + string_field(review, "fingerprint") == Some(candidate.fingerprint.as_str()) + && (string_field(review, "status") == Some("confirmed") + || (string_field(review, "status") == Some("not-applicable") + && candidate.evidence_refs.iter().all(|reference| { + snapshot + .artifact_digests + .get(reference) + .is_some_and(|digest| { + result["input_refs"][reference].as_str() + == Some(digest.as_str()) + }) + }))) + }) + .max_by_key(|(result, review)| { + ( + string_field(review, "status") == Some("confirmed"), + string_field(result, "id"), + ) + }) } fn disposition_evidence_is_current( @@ -2227,6 +2263,101 @@ mod tests { ); } + #[test] + fn refreshed_candidate_reviews_survive_stale_results_in_either_order() { + let (mut snapshot, rule_index, mut detection) = not_applicable_signal_case(); + snapshot.repository["phase"] = json!("post-build"); + let original = detection.candidates[0].clone(); + detection.candidates = (0..7) + .map(|index| { + let mut candidate = original.clone(); + candidate.fingerprint = format!("sha256:{index:064x}"); + candidate + }) + .collect(); + let mut current = snapshot.results[0].clone(); + current["id"] = json!("result.a-current"); + current["payload"]["reviewed_candidates"] = json!( + detection + .candidates + .iter() + .map(|candidate| { + json!({ + "fingerprint": candidate.fingerprint, + "status": "not-applicable", + "reason": "Current code is a read-only probe", + "basis_refs": candidate.evidence_refs, + }) + }) + .collect::>() + ); + let mut stale = current.clone(); + stale["id"] = json!("result.z-stale"); + stale["input_refs"]["code.place-order"] = json!(format!("sha256:{}", "0".repeat(64))); + for results in [ + vec![current.clone(), stale.clone()], + vec![stale, current.clone()], + ] { + snapshot.results = results; + let decision = ThinKernel.evaluate(&snapshot, &rule_index, &detection); + assert_eq!(decision.state, "needs-pre-build-challenge"); + assert_eq!( + decision + .action + .as_ref() + .unwrap() + .requirement_instances + .len(), + 7 + ); + let explanation = + ExplanationBuilder.build(&snapshot, &rule_index, &detection, &decision); + assert!(explanation.candidates.iter().all(|candidate| { + candidate.disposition == "applicability-pending" + && candidate.disposition_result_id.as_deref() == Some("result.a-current") + })); + let context = ContextCompiler + .compile(&decision, &snapshot, &detection) + .unwrap(); + let candidates = context.payload["not_applicable_signal_candidates"] + .as_array() + .unwrap(); + assert_eq!(candidates.len(), 7); + assert!( + candidates + .iter() + .all(|candidate| candidate["disposition_result_id"] == "result.a-current") + ); + } + snapshot.artifact_digests.insert( + "code.place-order".to_owned(), + format!("sha256:{}", "9".repeat(64)), + ); + let decision = ThinKernel.evaluate(&snapshot, &rule_index, &detection); + assert_eq!(decision.action.unwrap().candidate_fingerprints.len(), 7); + } + + #[test] + fn confirmed_candidate_review_wins_conflicting_non_applicability_in_either_order() { + let (mut snapshot, rule_index, detection) = not_applicable_signal_case(); + let not_applicable = snapshot.results[0].clone(); + let mut confirmed = not_applicable.clone(); + confirmed["id"] = json!("result.a-confirmed"); + confirmed["payload"]["reviewed_candidates"][0]["status"] = json!("confirmed"); + confirmed["input_refs"]["code.place-order"] = json!("old-evidence"); + for results in [ + vec![confirmed.clone(), not_applicable.clone()], + vec![not_applicable, confirmed], + ] { + snapshot.results = results; + let decision = ThinKernel.evaluate(&snapshot, &rule_index, &detection); + let explanation = + ExplanationBuilder.build(&snapshot, &rule_index, &detection, &decision); + assert_eq!(explanation.candidates[0].disposition, "confirmed"); + assert_eq!(decision.action.unwrap().action, "analyze-requirements"); + } + } + #[test] fn changed_evidence_reopens_only_a_not_applicable_candidate_after_build() { let (mut snapshot, rule_index, mut detection) = not_applicable_signal_case(); diff --git a/tests/cli.rs b/tests/cli.rs index 5681c0a..d14ddeb 100644 --- a/tests/cli.rs +++ b/tests/cli.rs @@ -3111,6 +3111,90 @@ fn project_next_explain_and_contract_health_share_the_real_project_loader() { assert!(text.contains("unverified")); } +#[test] +fn cli_and_mcp_keep_current_candidate_reviews_when_stale_results_sort_last() { + let project = TestProject::new(); + let (mut child, mut input, mut output) = start_mcp_server(&project.root); + let next = mcp_call( + &mut input, + &mut output, + 2, + "adf_next", + json!({"change_id": "change.place-order"}), + ); + assert_eq!(next["isError"], false); + let data = &next["structuredContent"]; + let mut submission = risk_signal_submission( + &data["issued_action"], + &data["next_response"]["context"]["payload"], + ); + for review in submission["payload"]["reviewed_candidates"] + .as_array_mut() + .unwrap() + { + review["status"] = json!("not-applicable"); + } + let submitted = mcp_call(&mut input, &mut output, 3, "adf_submit", submission); + assert_eq!(submitted["isError"], false, "{submitted}"); + drop(input); + assert!(child.wait().unwrap().success()); + + let results_dir = project.root.join(".adf/changes/change.place-order/results"); + let path = fs::read_dir(&results_dir) + .unwrap() + .next() + .unwrap() + .unwrap() + .path(); + let current: Value = serde_json::from_slice(&fs::read(path).unwrap()).unwrap(); + let mut stale = current.clone(); + stale["id"] = json!("result.zz-stale"); + stale["action_id"] = json!("action.old-review"); + for field in ["input_refs", "freshness_refs"] { + for digest in stale[field].as_object_mut().unwrap().values_mut() { + *digest = json!(format!("sha256:{}", "0".repeat(64))); + } + } + fs::write( + results_dir.join("old-review.json"), + serde_json::to_vec(&stale).unwrap(), + ) + .unwrap(); + + let cli = project.run(&["next", "change.place-order", "--format", "json"]); + assert_success(&cli); + let cli: Value = serde_json::from_slice(&cli.stdout).unwrap(); + assert_eq!(cli["state"], "needs-pre-build-challenge"); + let explain = project.run(&["explain", "change.place-order", "--format", "json"]); + assert_success(&explain); + let explain: Value = serde_json::from_slice(&explain.stdout).unwrap(); + assert!( + explain["candidates"] + .as_array() + .unwrap() + .iter() + .all(|candidate| { + candidate["disposition"] == "applicability-pending" + && candidate["disposition_result_id"] == current["id"] + }) + ); + let (mut child, mut input, mut output) = start_mcp_server(&project.root); + let next = mcp_call( + &mut input, + &mut output, + 2, + "adf_next", + json!({"change_id": "change.place-order"}), + ); + assert_eq!(next["isError"], false); + assert_eq!( + next["structuredContent"]["next_response"]["next_action"]["id"], + cli["next_action"]["id"] + ); + drop(input); + assert!(child.wait().unwrap().success()); +} + #[test] fn pending_impact_assessment_does_not_load_repository_wide_contract_health() { let project = TestProject::new();