diff --git a/apps/decodex/src/agent/tracker_tool_bridge.rs b/apps/decodex/src/agent/tracker_tool_bridge.rs index fd6696a57..ba09be492 100644 --- a/apps/decodex/src/agent/tracker_tool_bridge.rs +++ b/apps/decodex/src/agent/tracker_tool_bridge.rs @@ -776,10 +776,61 @@ struct TerminalFinalizeArgs { struct ReviewCheckpointArgs { #[serde(flatten)] scope: ScopeArgs, + reviewer: Option, status: String, head_sha: String, + checks: Option, #[serde(default)] evidence: Vec, + #[serde(default)] + accepted_findings: Vec, + #[serde(default)] + rejected_findings: Vec, +} + +#[derive(Clone, Debug, Eq, PartialEq, Deserialize, Serialize)] +#[serde(deny_unknown_fields)] +struct ReviewCheckpointChecksArgs { + intended_behavior: String, + regression_risk: String, + missing_tests: String, + docs_config_drift: String, + migration_fallout: String, + operator_facing_fallout: String, + loop_decision_contract: String, +} + +#[derive(Clone, Debug, Eq, PartialEq, Deserialize, Serialize)] +#[serde(deny_unknown_fields)] +struct ReviewCheckpointFindingArgs { + severity: String, + summary: String, + #[serde(default)] + evidence: Vec, + file: Option, + line: Option, + guidance: String, +} + +#[derive(Clone, Debug, Eq, PartialEq, Deserialize, Serialize)] +#[serde(deny_unknown_fields)] +struct ReviewCheckpointRejectedFindingArgs { + severity: String, + summary: String, + rejection_reason: String, + #[serde(default)] + evidence: Vec, + file: Option, + line: Option, +} + +#[derive(Debug, Serialize)] +struct NormalizedReviewCheckpointPayload { + reviewer: String, + checks: ReviewCheckpointChecksArgs, + evidence: Vec, + accepted_findings: Vec, + rejected_findings: Vec, } #[derive(Debug, Deserialize)] diff --git a/apps/decodex/src/agent/tracker_tool_bridge/review.rs b/apps/decodex/src/agent/tracker_tool_bridge/review.rs index d27b03233..66d01a0ef 100644 --- a/apps/decodex/src/agent/tracker_tool_bridge/review.rs +++ b/apps/decodex/src/agent/tracker_tool_bridge/review.rs @@ -81,6 +81,7 @@ impl<'a> TrackerToolBridge<'a> { review_policy_status: ReviewPolicyStatus, head_sha: &str, nonclean_rounds: i64, + details_json: &str, ) -> Result<(), String> { let state_store = self.state_store.ok_or_else(|| { format!( @@ -99,6 +100,7 @@ impl<'a> TrackerToolBridge<'a> { status: review_policy_status.as_str(), head_sha, nonclean_rounds, + details_json, }) .map_err(|error| { format!( @@ -980,6 +982,7 @@ impl<'a> TrackerToolBridge<'a> { status: status.as_str(), head_sha, nonclean_rounds, + details_json: "{}", })?; Ok(Some(ReviewPolicyState { diff --git a/apps/decodex/src/agent/tracker_tool_bridge/tests/review/policy.rs b/apps/decodex/src/agent/tracker_tool_bridge/tests/review/policy.rs index 1f2fbb7cb..69c0c061a 100644 --- a/apps/decodex/src/agent/tracker_tool_bridge/tests/review/policy.rs +++ b/apps/decodex/src/agent/tracker_tool_bridge/tests/review/policy.rs @@ -43,6 +43,37 @@ fn sample_review_repair_apply_inspectors( (inspector, local_repo_inspector) } +fn review_checks_json() -> Value { + serde_json::json!({ + "intended_behavior": "Checked the implementation against the issue requirements.", + "regression_risk": "Checked shared runtime regression risk for the touched path.", + "missing_tests": "Checked whether the current change needs additional tests.", + "docs_config_drift": "Checked docs and config drift for the runtime behavior change.", + "migration_fallout": "Checked additive runtime-store migration fallout.", + "operator_facing_fallout": "Checked Linear and operator-facing fallout.", + "loop_decision_contract": "Compared the change against the accepted Loop/Decision Contract and found no mismatch." + }) +} + +fn accepted_review_findings_json() -> Value { + serde_json::json!([{ + "severity": "medium", + "summary": "Accepted reviewer finding", + "evidence": ["The reviewer evidence points at the current lane head."], + "file": "apps/decodex/src/agent/tracker_tool_bridge/tools.rs", + "line": 1, + "guidance": "Repair the accepted issue before requesting another review checkpoint." + }]) +} + +fn accepted_review_findings_for_status_json(status: &str) -> Value { + if status == "findings" { + accepted_review_findings_json() + } else { + serde_json::json!([]) + } +} + fn seed_review_repair_apply_state( state_store: &StateStore, review_context: &ReviewHandoffContext, @@ -440,8 +471,10 @@ fn review_checkpoint_normalizes_matching_short_head_sha_to_full_head() { &bridge, ISSUE_REVIEW_CHECKPOINT_TOOL_NAME, serde_json::json!({ + "reviewer": "independent_fresh_context", "status": "clean", "head_sha": &sample_local_repo().head_oid[..7], + "checks": review_checks_json(), "evidence": ["Closeout and review policy both point at the current lane head."] }), ); @@ -454,6 +487,284 @@ fn review_checkpoint_normalizes_matching_short_head_sha_to_full_head() { assert_eq!(checkpoint.head_sha(), sample_local_repo().head_oid.as_str()); } +#[test] +fn independent_review_checkpoint_requires_structured_fresh_context_payload() { + let tracker = FakeTracker::new(); + let issue = sample_issue(); + let workflow = sample_workflow(); + let temp_dir = TempDir::new().expect("tempdir should create"); + let review_context = sample_review_context_in(temp_dir.path()); + let pull_request_inspector = FakePullRequestInspector::new(Vec::new()); + let local_repo_inspector = FakeLocalRepoInspector::new(vec![ + Ok(sample_local_repo()), + Ok(sample_local_repo()), + Ok(sample_local_repo()), + Ok(sample_local_repo()), + Ok(sample_local_repo()), + Ok(sample_local_repo()), + ]); + let bridge = TrackerToolBridge::with_review_handoff_for_test( + &tracker, + &issue, + &workflow, + review_context, + &pull_request_inspector, + &local_repo_inspector, + ); + + for (payload, expected_error) in [ + ( + serde_json::json!({ + "status": "clean", + "head_sha": sample_local_repo().head_oid, + "checks": review_checks_json(), + "evidence": ["review evidence"] + }), + "requires `reviewer`", + ), + ( + serde_json::json!({ + "reviewer": "self_review", + "status": "clean", + "head_sha": sample_local_repo().head_oid, + "checks": review_checks_json(), + "evidence": ["review evidence"] + }), + "reviewer must be `independent_fresh_context`", + ), + ( + serde_json::json!({ + "reviewer": "independent_fresh_context", + "status": "clean", + "head_sha": sample_local_repo().head_oid, + "evidence": ["review evidence"] + }), + "requires `checks`", + ), + ( + serde_json::json!({ + "reviewer": "independent_fresh_context", + "status": "clean", + "head_sha": sample_local_repo().head_oid, + "checks": review_checks_json(), + "evidence": [] + }), + "requires `evidence`", + ), + ( + serde_json::json!({ + "reviewer": "independent_fresh_context", + "status": "findings", + "head_sha": sample_local_repo().head_oid, + "checks": review_checks_json(), + "evidence": ["review evidence"], + "accepted_findings": [{ + "severity": "medium", + "summary": "Accepted reviewer finding", + "evidence": [], + "guidance": "Repair the accepted issue before requesting another checkpoint." + }] + }), + "requires `accepted_findings.evidence`", + ), + ( + serde_json::json!({ + "reviewer": "independent_fresh_context", + "status": "clean", + "head_sha": sample_local_repo().head_oid, + "checks": review_checks_json(), + "evidence": ["review evidence"], + "rejected_findings": [{ + "severity": "unknown", + "summary": "Rejected reviewer finding", + "rejection_reason": "Not actionable after validation.", + "evidence": ["Reviewer evidence was stale."] + }] + }), + "`rejected_findings.severity` must be", + ), + ] { + let response = + DynamicToolHandler::handle_call(&bridge, ISSUE_REVIEW_CHECKPOINT_TOOL_NAME, payload); + + assert!(!response.success); + assert!(matches!( + response.content_items.as_slice(), + [DynamicToolContentItem::InputText { text }] if text.contains(expected_error) + )); + } +} + +#[test] +fn independent_review_checkpoint_clean_persists_structured_payload() { + let tracker = FakeTracker::new(); + let issue = sample_issue(); + let workflow = sample_workflow(); + let temp_dir = TempDir::new().expect("tempdir should create"); + let review_context = sample_review_context_in(temp_dir.path()); + let pull_request_inspector = FakePullRequestInspector::new(Vec::new()); + let local_repo_inspector = FakeLocalRepoInspector::new(vec![Ok(sample_local_repo())]); + let bridge = TrackerToolBridge::with_review_handoff_for_test( + &tracker, + &issue, + &workflow, + review_context.clone(), + &pull_request_inspector, + &local_repo_inspector, + ); + let response = DynamicToolHandler::handle_call( + &bridge, + ISSUE_REVIEW_CHECKPOINT_TOOL_NAME, + serde_json::json!({ + "reviewer": "independent_fresh_context", + "status": "clean", + "head_sha": sample_local_repo().head_oid, + "checks": review_checks_json(), + "evidence": ["fresh reviewer read the issue contract, current diff, and HEAD"], + "rejected_findings": [{ + "severity": "low", + "summary": "The reviewer asked for a migration note, but no schema or data migration changed.", + "rejection_reason": "Not actionable after checking the current diff and docs.", + "evidence": ["Only runtime review checkpoint metadata changed."], + "file": "apps/decodex/src/agent/tracker_tool_bridge/tools.rs", + "line": 1 + }] + }), + ); + + assert!(response.success); + + let checkpoint = persisted_review_policy_checkpoint(&bridge, &issue, &review_context); + let details = + serde_json::from_str::(checkpoint.details_json()).expect("details should be json"); + + assert_eq!(checkpoint.status(), "clean"); + assert_eq!(details["reviewer"], "independent_fresh_context"); + assert_eq!( + details["checks"]["loop_decision_contract"], + "Compared the change against the accepted Loop/Decision Contract and found no mismatch." + ); + assert_eq!(details["accepted_findings"].as_array().expect("accepted findings array").len(), 0); + assert_eq!(details["rejected_findings"][0]["rejection_reason"], "Not actionable after checking the current diff and docs."); + + let events = bridge_state_store(&bridge) + .list_private_execution_events_for_run_attempt( + &review_context.service_id, + &review_context.run_id, + review_context.attempt_number, + ) + .expect("private review evidence should read"); + + assert_eq!(events.len(), 1); + assert_eq!(events[0].event_type(), "review_checkpoint"); + assert_eq!(events[0].payload()["review"]["reviewer"], "independent_fresh_context"); +} + +#[test] +fn independent_review_checkpoint_findings_store_accepted_repair_guidance() { + let tracker = FakeTracker::new(); + let issue = sample_issue(); + let workflow = sample_workflow(); + let temp_dir = TempDir::new().expect("tempdir should create"); + let review_context = sample_review_context_in(temp_dir.path()); + let pull_request_inspector = FakePullRequestInspector::new(Vec::new()); + let local_repo_inspector = + FakeLocalRepoInspector::new(vec![Ok(sample_local_repo()), Ok(sample_local_repo())]); + let bridge = TrackerToolBridge::with_review_handoff_for_test( + &tracker, + &issue, + &workflow, + review_context.clone(), + &pull_request_inspector, + &local_repo_inspector, + ); + let response = DynamicToolHandler::handle_call( + &bridge, + ISSUE_REVIEW_CHECKPOINT_TOOL_NAME, + serde_json::json!({ + "reviewer": "independent_fresh_context", + "status": "findings", + "head_sha": sample_local_repo().head_oid, + "checks": review_checks_json(), + "evidence": ["fresh reviewer found one accepted repair item"], + "accepted_findings": accepted_review_findings_json() + }), + ); + + assert!(response.success); + assert_eq!( + DynamicToolHandler::classify_turn_completion(&bridge, "continue") + .expect("first accepted findings round should continue"), + TurnCompletionStatus::Continue + ); + + let checkpoint = persisted_review_policy_checkpoint(&bridge, &issue, &review_context); + let details = + serde_json::from_str::(checkpoint.details_json()).expect("details should be json"); + + assert_eq!(checkpoint.status(), "findings"); + assert_eq!(checkpoint.nonclean_rounds(), 1); + assert_eq!(details["accepted_findings"][0]["severity"], "medium"); + assert_eq!( + details["accepted_findings"][0]["guidance"], + "Repair the accepted issue before requesting another review checkpoint." + ); +} + +#[test] +fn review_checkpoint_rejected_finding_is_non_actionable_and_can_handoff_cleanly() { + let temp_dir = TempDir::new().expect("tempdir should create"); + let tracker = FakeTracker::new(); + let issue = sample_issue(); + let workflow = sample_workflow(); + let inspector = FakePullRequestInspector::new(vec![Ok(sample_pull_request())]); + let local_repo_inspector = + FakeLocalRepoInspector::new(vec![Ok(sample_local_repo()), Ok(sample_local_repo())]); + let review_context = sample_review_context_in(temp_dir.path()); + let bridge = TrackerToolBridge::with_review_handoff_for_test( + &tracker, + &issue, + &workflow, + review_context.clone(), + &inspector, + &local_repo_inspector, + ); + let response = DynamicToolHandler::handle_call( + &bridge, + ISSUE_REVIEW_CHECKPOINT_TOOL_NAME, + serde_json::json!({ + "reviewer": "independent_fresh_context", + "status": "clean", + "head_sha": sample_local_repo().head_oid, + "checks": review_checks_json(), + "evidence": ["only rejected non-actionable feedback remained"], + "rejected_findings": [{ + "severity": "low", + "summary": "The reviewer requested a migration test.", + "rejection_reason": "No migration path changed in the current diff.", + "evidence": ["The runtime store column is additive and defaults existing rows."], + "file": "apps/decodex/src/state/internal.rs", + "line": 1 + }] + }), + ); + + assert!(response.success); + + let handoff_response = DynamicToolHandler::handle_call( + &bridge, + ISSUE_REVIEW_HANDOFF_TOOL_NAME, + serde_json::json!({ + "pr_url": "https://github.com/hack-ink/decodex/pull/48", + "summary": "Rejected non-actionable review feedback and prepared handoff." + }), + ); + + assert!(handoff_response.success); + + assert_review_policy_checkpoint_cleared(&bridge, &issue, &review_context); +} + #[test] fn review_checkpoint_findings_continue_until_budget_then_stop() { let tracker = FakeTracker::new(); @@ -481,9 +792,12 @@ fn review_checkpoint_findings_continue_until_budget_then_stop() { &bridge, ISSUE_REVIEW_CHECKPOINT_TOOL_NAME, serde_json::json!({ + "reviewer": "independent_fresh_context", "status": "findings", "head_sha": sample_local_repo().head_oid, - "evidence": ["owned fix still pending"] + "checks": review_checks_json(), + "evidence": ["owned fix still pending"], + "accepted_findings": accepted_review_findings_json() }), ); @@ -505,9 +819,12 @@ fn review_checkpoint_findings_continue_until_budget_then_stop() { &bridge, ISSUE_REVIEW_CHECKPOINT_TOOL_NAME, serde_json::json!({ + "reviewer": "independent_fresh_context", "status": "findings", "head_sha": sample_local_repo().head_oid, - "evidence": ["still not converged"] + "checks": review_checks_json(), + "evidence": ["still not converged"], + "accepted_findings": accepted_review_findings_json() }), ); @@ -552,9 +869,12 @@ fn review_checkpoint_clean_resets_nonclean_rounds_before_next_findings() { &bridge, ISSUE_REVIEW_CHECKPOINT_TOOL_NAME, serde_json::json!({ + "reviewer": "independent_fresh_context", "status": status, "head_sha": sample_local_repo().head_oid, - "evidence": ["review evidence"] + "checks": review_checks_json(), + "evidence": ["review evidence"], + "accepted_findings": accepted_review_findings_for_status_json(status) }), ); @@ -594,9 +914,12 @@ fn review_checkpoint_does_not_depend_on_tracker_comment_write() { &bridge, ISSUE_REVIEW_CHECKPOINT_TOOL_NAME, serde_json::json!({ + "reviewer": "independent_fresh_context", "status": "findings", "head_sha": sample_local_repo().head_oid, - "evidence": ["tracker write failed before checkpoint persisted"] + "checks": review_checks_json(), + "evidence": ["tracker write failed before checkpoint persisted"], + "accepted_findings": accepted_review_findings_json() }), ); @@ -632,8 +955,10 @@ fn review_checkpoint_architecture_and_blocked_statuses_stop_immediately() { &bridge, ISSUE_REVIEW_CHECKPOINT_TOOL_NAME, serde_json::json!({ + "reviewer": "independent_fresh_context", "status": status, "head_sha": sample_local_repo().head_oid, + "checks": review_checks_json(), "evidence": ["requires human follow-up"] }), ); @@ -683,9 +1008,12 @@ fn review_checkpoint_phase_switch_resets_nonclean_rounds() { &bridge, ISSUE_REVIEW_CHECKPOINT_TOOL_NAME, serde_json::json!({ + "reviewer": "independent_fresh_context", "status": "findings", "head_sha": sample_local_repo().head_oid, - "evidence": [] + "checks": review_checks_json(), + "evidence": ["fresh repair-phase review found accepted work"], + "accepted_findings": accepted_review_findings_json() }), ); @@ -697,6 +1025,56 @@ fn review_checkpoint_phase_switch_resets_nonclean_rounds() { assert_eq!(checkpoint.nonclean_rounds(), 1); } +#[test] +fn repair_review_checkpoint_stores_accepted_findings_for_repair_loop() { + let tracker = FakeTracker::new(); + let workflow = sample_workflow(); + let temp_dir = TempDir::new().expect("tempdir should create"); + let repair_context = sample_review_repair_context_in( + temp_dir.path(), + "https://github.com/hack-ink/decodex/pull/242", + ); + let issue = sample_review_issue(); + let pull_request_inspector = FakePullRequestInspector::new(Vec::new()); + let local_repo_inspector = FakeLocalRepoInspector::new(vec![Ok(sample_local_repo())]); + let bridge = TrackerToolBridge::with_review_repair_for_test( + &tracker, + &issue, + &workflow, + repair_context.clone(), + &pull_request_inspector, + &local_repo_inspector, + ); + let response = DynamicToolHandler::handle_call( + &bridge, + ISSUE_REVIEW_CHECKPOINT_TOOL_NAME, + serde_json::json!({ + "reviewer": "independent_fresh_context", + "status": "findings", + "head_sha": sample_local_repo().head_oid, + "checks": review_checks_json(), + "evidence": ["fresh-context retained repair review accepted one finding"], + "accepted_findings": accepted_review_findings_json(), + "rejected_findings": [{ + "severity": "info", + "summary": "Reviewer suggested changing unrelated landing code.", + "rejection_reason": "Outside this retained repair batch.", + "evidence": ["The current PR feedback only concerns the tracker-tool bridge."] + }] + }), + ); + + assert!(response.success); + + let checkpoint = persisted_review_policy_checkpoint(&bridge, &issue, &repair_context); + let details = + serde_json::from_str::(checkpoint.details_json()).expect("details should be json"); + + assert_eq!(checkpoint.phase(), "repair"); + assert_eq!(details["accepted_findings"][0]["summary"], "Accepted reviewer finding"); + assert_eq!(details["rejected_findings"][0]["rejection_reason"], "Outside this retained repair batch."); +} + #[test] fn stale_review_checkpoint_for_old_head_does_not_stop_new_head() { let tracker = FakeTracker::new(); @@ -763,6 +1141,7 @@ fn runtime_review_checkpoint_wins_over_stale_marker_policy_state() { status: "clean", head_sha: &sample_local_repo().head_oid, nonclean_rounds: 0, + details_json: "{}", }) .expect("runtime review checkpoint should persist"); write_review_policy_checkpoint( diff --git a/apps/decodex/src/agent/tracker_tool_bridge/tools.rs b/apps/decodex/src/agent/tracker_tool_bridge/tools.rs index 6b2b723b7..9f3969397 100644 --- a/apps/decodex/src/agent/tracker_tool_bridge/tools.rs +++ b/apps/decodex/src/agent/tracker_tool_bridge/tools.rs @@ -7,8 +7,10 @@ use crate::{ ISSUE_LABEL_ADD_TOOL_NAME, ISSUE_PROGRESS_CHECKPOINT_TOOL_NAME, ISSUE_REVIEW_CHECKPOINT_TOOL_NAME, ISSUE_REVIEW_HANDOFF_TOOL_NAME, ISSUE_REVIEW_REPAIR_COMPLETE_TOOL_NAME, ISSUE_TERMINAL_FINALIZE_TOOL_NAME, - ISSUE_TRANSITION_TOOL_NAME, LabelArgs, NormalizedProgressCheckpoint, PendingReviewAction, - PendingReviewCompletion, ProgressCheckpointArgs, ReviewCheckpointArgs, ReviewExecutionMode, + ISSUE_TRANSITION_TOOL_NAME, LabelArgs, NormalizedProgressCheckpoint, + NormalizedReviewCheckpointPayload, PendingReviewAction, PendingReviewCompletion, + ProgressCheckpointArgs, ReviewCheckpointArgs, ReviewCheckpointChecksArgs, + ReviewCheckpointFindingArgs, ReviewCheckpointRejectedFindingArgs, ReviewExecutionMode, ReviewHandoffArgs, ReviewHandoffContext, ReviewPolicyPhase, ReviewPolicyStatus, RunCompletionDisposition, TerminalFinalizeArgs, TrackerToolBridge, TransitionArgs, }, @@ -21,6 +23,7 @@ use crate::{ const COMMENT_KIND_MANUAL_ATTENTION: &str = "manual_attention"; const MANUAL_ATTENTION_TERMINAL_PATH: &str = "manual_attention"; +const INDEPENDENT_FRESH_CONTEXT_REVIEWER: &str = "independent_fresh_context"; #[derive(Debug)] struct NormalizedManualAttentionComment { @@ -33,6 +36,12 @@ struct NormalizedManualAttentionComment { summary: Option, } +struct ReviewCheckpointPayloadCounts { + evidence: usize, + accepted_findings: usize, + rejected_findings: usize, +} + impl<'a> TrackerToolBridge<'a> { pub(super) fn build_tool_specs(&self) -> Vec { let mut tool_specs = match self.review_context.as_ref().map(|context| context.mode) { @@ -242,23 +251,96 @@ impl<'a> TrackerToolBridge<'a> { pub(super) fn review_checkpoint_tool_specs(&self) -> [DynamicToolSpec; 1] { [DynamicToolSpec::new( ISSUE_REVIEW_CHECKPOINT_TOOL_NAME, - "Record the current repo-native bounded-review result for the leased issue so Decodex can decide whether the lane may continue or must stop for human intervention. `head_sha` must resolve to the current lane HEAD.", + "Record the independent fresh-context read-only bounded-review result for the leased issue so Decodex can decide whether the lane may continue, repair accepted findings, or stop for human intervention. `head_sha` must resolve to the current lane HEAD.", serde_json::json!({ "type": "object", "properties": { "issue_id": { "type": "string" }, "issue_identifier": { "type": "string" }, + "reviewer": { + "type": "string", + "enum": ["independent_fresh_context"] + }, "status": { "type": "string", "enum": ["clean", "findings", "needs_architecture_review", "blocked"] }, "head_sha": { "type": "string" }, + "checks": { + "type": "object", + "properties": { + "intended_behavior": { "type": "string" }, + "regression_risk": { "type": "string" }, + "missing_tests": { "type": "string" }, + "docs_config_drift": { "type": "string" }, + "migration_fallout": { "type": "string" }, + "operator_facing_fallout": { "type": "string" }, + "loop_decision_contract": { "type": "string" } + }, + "required": [ + "intended_behavior", + "regression_risk", + "missing_tests", + "docs_config_drift", + "migration_fallout", + "operator_facing_fallout", + "loop_decision_contract" + ], + "additionalProperties": false + }, "evidence": { "type": "array", - "items": { "type": "string" } + "items": { "type": "string" }, + "minItems": 1 + }, + "accepted_findings": { + "type": "array", + "items": { + "type": "object", + "properties": { + "severity": { + "type": "string", + "enum": ["critical", "high", "medium", "low", "info"] + }, + "summary": { "type": "string" }, + "evidence": { + "type": "array", + "items": { "type": "string" }, + "minItems": 1 + }, + "file": { "type": "string" }, + "line": { "type": "integer", "minimum": 1 }, + "guidance": { "type": "string" } + }, + "required": ["severity", "summary", "evidence", "guidance"], + "additionalProperties": false + } + }, + "rejected_findings": { + "type": "array", + "items": { + "type": "object", + "properties": { + "severity": { + "type": "string", + "enum": ["critical", "high", "medium", "low", "info"] + }, + "summary": { "type": "string" }, + "rejection_reason": { "type": "string" }, + "evidence": { + "type": "array", + "items": { "type": "string" }, + "minItems": 1 + }, + "file": { "type": "string" }, + "line": { "type": "integer", "minimum": 1 } + }, + "required": ["severity", "summary", "rejection_reason", "evidence"], + "additionalProperties": false + } } }, - "required": ["status", "head_sha", "evidence"], + "required": ["reviewer", "status", "head_sha", "checks", "evidence"], "additionalProperties": false }), )] @@ -879,12 +961,19 @@ impl<'a> TrackerToolBridge<'a> { Ok(head_sha) => head_sha, Err(error) => return DynamicToolCallResponse::failure(error), }; - let evidence = parsed - .evidence - .into_iter() - .map(|item| item.trim().to_owned()) - .filter(|item| !item.is_empty()) - .collect::>(); + let checkpoint_payload = + match normalize_review_checkpoint_payload(parsed, review_policy_status) { + Ok(payload) => payload, + Err(error) => return DynamicToolCallResponse::failure(error), + }; + let details_json = match serde_json::to_string(&checkpoint_payload) { + Ok(details_json) => details_json, + Err(error) => + return DynamicToolCallResponse::failure(format!( + "Failed to serialize the structured review checkpoint for issue `{}`: {error}", + self.issue.identifier + )), + }; let nonclean_rounds = match self.review_checkpoint_nonclean_rounds( review_context, review_policy_phase, @@ -900,6 +989,17 @@ impl<'a> TrackerToolBridge<'a> { review_policy_status, &head_sha, nonclean_rounds, + &details_json, + ) { + return DynamicToolCallResponse::failure(error); + } + if let Err(error) = self.append_private_review_checkpoint( + review_context, + review_policy_phase, + review_policy_status, + &head_sha, + nonclean_rounds, + &checkpoint_payload, ) { return DynamicToolCallResponse::failure(error); } @@ -909,7 +1009,11 @@ impl<'a> TrackerToolBridge<'a> { review_policy_status, &head_sha, nonclean_rounds, - evidence.len(), + ReviewCheckpointPayloadCounts { + evidence: checkpoint_payload.evidence.len(), + accepted_findings: checkpoint_payload.accepted_findings.len(), + rejected_findings: checkpoint_payload.rejected_findings.len(), + }, ); DynamicToolCallResponse::success(message) @@ -947,13 +1051,12 @@ impl<'a> TrackerToolBridge<'a> { review_policy_status: ReviewPolicyStatus, head_sha: &str, nonclean_rounds: i64, - evidence_count: usize, + counts: ReviewCheckpointPayloadCounts, ) -> String { - let evidence_suffix = if evidence_count == 0 { - String::from("no evidence items recorded") - } else { - format!("{evidence_count} evidence item(s) recorded") - }; + let evidence_suffix = format!( + "{} evidence item(s), {} accepted finding(s), and {} rejected finding(s) recorded", + counts.evidence, counts.accepted_findings, counts.rejected_findings, + ); match review_policy_status { ReviewPolicyStatus::Clean => format!( @@ -977,6 +1080,47 @@ impl<'a> TrackerToolBridge<'a> { } } + fn append_private_review_checkpoint( + &self, + review_context: &ReviewHandoffContext, + review_policy_phase: ReviewPolicyPhase, + review_policy_status: ReviewPolicyStatus, + head_sha: &str, + nonclean_rounds: i64, + checkpoint_payload: &NormalizedReviewCheckpointPayload, + ) -> Result<(), String> { + let state_store = self.state_store.ok_or_else(|| { + format!( + "`{ISSUE_REVIEW_CHECKPOINT_TOOL_NAME}` requires the Decodex runtime state store for issue `{}`.", + self.issue.identifier + ) + })?; + let private_payload = serde_json::json!({ + "phase": review_policy_phase.as_str(), + "status": review_policy_status.as_str(), + "head_sha": head_sha, + "nonclean_rounds": nonclean_rounds, + "review": checkpoint_payload, + }); + + state_store + .append_private_execution_event( + &review_context.service_id, + &self.issue.id, + &review_context.run_id, + review_context.attempt_number, + "review_checkpoint", + private_payload, + ) + .map(|_| ()) + .map_err(|error| { + format!( + "Failed to persist the private review checkpoint for issue `{}`: {error}", + self.issue.identifier + ) + }) + } + fn clear_review_policy_state_after_completion( &self, review_context: &ReviewHandoffContext, @@ -1363,6 +1507,195 @@ fn normalize_required_comment_field( Ok(value) } +fn normalize_review_checkpoint_payload( + parsed: ReviewCheckpointArgs, + status: ReviewPolicyStatus, +) -> Result { + let reviewer = parsed + .reviewer + .map(|reviewer| reviewer.trim().to_owned()) + .filter(|reviewer| !reviewer.is_empty()) + .ok_or_else(|| { + format!( + "`{ISSUE_REVIEW_CHECKPOINT_TOOL_NAME}` requires `reviewer` set to `{INDEPENDENT_FRESH_CONTEXT_REVIEWER}`." + ) + })?; + + if reviewer != INDEPENDENT_FRESH_CONTEXT_REVIEWER { + return Err(format!( + "`{ISSUE_REVIEW_CHECKPOINT_TOOL_NAME}` reviewer must be `{INDEPENDENT_FRESH_CONTEXT_REVIEWER}`, not `{reviewer}`." + )); + } + + let checks = normalize_review_checkpoint_checks( + parsed + .checks + .ok_or_else(|| format!("`{ISSUE_REVIEW_CHECKPOINT_TOOL_NAME}` requires `checks`."))?, + )?; + let evidence = normalize_required_review_evidence_list(parsed.evidence, "evidence")?; + let accepted_findings = parsed + .accepted_findings + .into_iter() + .map(normalize_review_checkpoint_finding) + .collect::, _>>()?; + let rejected_findings = parsed + .rejected_findings + .into_iter() + .map(normalize_rejected_review_checkpoint_finding) + .collect::, _>>()?; + + if status == ReviewPolicyStatus::Findings && accepted_findings.is_empty() { + return Err(String::from( + "`issue_review_checkpoint` status `findings` requires at least one accepted finding. Put non-actionable reviewer comments in `rejected_findings` and use `clean` if no accepted repair remains.", + )); + } + if status == ReviewPolicyStatus::Clean && !accepted_findings.is_empty() { + return Err(String::from( + "`issue_review_checkpoint` status `clean` cannot include accepted findings. Reject non-actionable comments explicitly or use status `findings` for accepted repair work.", + )); + } + + Ok(NormalizedReviewCheckpointPayload { + reviewer, + checks, + evidence, + accepted_findings, + rejected_findings, + }) +} + +fn normalize_review_checkpoint_checks( + checks: ReviewCheckpointChecksArgs, +) -> Result { + Ok(ReviewCheckpointChecksArgs { + intended_behavior: normalize_required_review_text( + checks.intended_behavior, + "checks.intended_behavior", + )?, + regression_risk: normalize_required_review_text( + checks.regression_risk, + "checks.regression_risk", + )?, + missing_tests: normalize_required_review_text( + checks.missing_tests, + "checks.missing_tests", + )?, + docs_config_drift: normalize_required_review_text( + checks.docs_config_drift, + "checks.docs_config_drift", + )?, + migration_fallout: normalize_required_review_text( + checks.migration_fallout, + "checks.migration_fallout", + )?, + operator_facing_fallout: normalize_required_review_text( + checks.operator_facing_fallout, + "checks.operator_facing_fallout", + )?, + loop_decision_contract: normalize_required_review_text( + checks.loop_decision_contract, + "checks.loop_decision_contract", + )?, + }) +} + +fn normalize_review_checkpoint_finding( + finding: ReviewCheckpointFindingArgs, +) -> Result { + let severity = normalize_review_severity(finding.severity, "accepted_findings.severity")?; + + Ok(ReviewCheckpointFindingArgs { + severity, + summary: normalize_required_review_text(finding.summary, "accepted_findings.summary")?, + evidence: normalize_required_review_evidence_list( + finding.evidence, + "accepted_findings.evidence", + )?, + file: normalize_optional_review_file(finding.file)?, + line: normalize_optional_review_line(finding.line)?, + guidance: normalize_required_review_text(finding.guidance, "accepted_findings.guidance")?, + }) +} + +fn normalize_rejected_review_checkpoint_finding( + finding: ReviewCheckpointRejectedFindingArgs, +) -> Result { + let severity = normalize_review_severity(finding.severity, "rejected_findings.severity")?; + + Ok(ReviewCheckpointRejectedFindingArgs { + severity, + summary: normalize_required_review_text(finding.summary, "rejected_findings.summary")?, + rejection_reason: normalize_required_review_text( + finding.rejection_reason, + "rejected_findings.rejection_reason", + )?, + evidence: normalize_required_review_evidence_list( + finding.evidence, + "rejected_findings.evidence", + )?, + file: normalize_optional_review_file(finding.file)?, + line: normalize_optional_review_line(finding.line)?, + }) +} + +fn normalize_review_severity(severity: String, field_name: &str) -> Result { + let severity = severity.trim().to_ascii_lowercase(); + + match severity.as_str() { + "critical" | "high" | "medium" | "low" | "info" => Ok(severity), + other => Err(format!( + "`{field_name}` must be `critical`, `high`, `medium`, `low`, or `info`, not `{other}`." + )), + } +} + +fn normalize_required_review_text(value: String, field_name: &str) -> Result { + let value = tracker_tool_bridge::normalize_summary(&value); + + if value.is_empty() { + return Err(format!("`{ISSUE_REVIEW_CHECKPOINT_TOOL_NAME}` requires `{field_name}`.")); + } + + Ok(value) +} + +fn normalize_required_review_evidence_list( + values: Vec, + field_name: &str, +) -> Result, String> { + let values = tracker_tool_bridge::normalize_progress_list(values); + + if values.is_empty() { + return Err(format!("`{ISSUE_REVIEW_CHECKPOINT_TOOL_NAME}` requires `{field_name}`.")); + } + + Ok(values) +} + +fn normalize_optional_review_file(value: Option) -> Result, String> { + let Some(file) = tracker_tool_bridge::normalize_optional_progress_field(value) else { + return Ok(None); + }; + + if file.starts_with('/') { + return Err(String::from( + "`issue_review_checkpoint` file references must be repository-relative paths.", + )); + } + + Ok(Some(file)) +} + +fn normalize_optional_review_line(value: Option) -> Result, String> { + if matches!(value, Some(0)) { + return Err(String::from( + "`issue_review_checkpoint` line references must be one-based when supplied.", + )); + } + + Ok(value) +} + fn validate_public_error_class(error_class: &str) -> Result<(), String> { let mut chars = error_class.chars(); let Some(first) = chars.next() else { diff --git a/apps/decodex/src/config.rs b/apps/decodex/src/config.rs index c08135bd9..73e305328 100644 --- a/apps/decodex/src/config.rs +++ b/apps/decodex/src/config.rs @@ -193,7 +193,7 @@ pub struct ProjectCodexConfig { accounts: Option, } impl ProjectCodexConfig { - /// Internal self-review behavior Decodex should request for agent runs. + /// Internal review behavior Decodex should request for agent runs. pub fn internal_review_mode(&self) -> InternalReviewMode { self.internal_review_mode } @@ -394,15 +394,15 @@ impl ServiceConfigDocument { } } -/// Internal self-review mode for agent runs. +/// Internal review mode for agent runs. #[derive(Clone, Copy, Debug, Eq, PartialEq, Deserialize)] #[serde(rename_all = "snake_case")] pub enum InternalReviewMode { - /// Use the existing runtime-owned checkpoint loop. + /// Use the runtime-owned independent review checkpoint loop. Loop, /// Add a prompt-only self-review instruction without the checkpoint loop. Prompt, - /// Disable internal self-review behavior. + /// Disable internal review behavior. Off, } impl InternalReviewMode { diff --git a/apps/decodex/src/orchestrator/prompting.rs b/apps/decodex/src/orchestrator/prompting.rs index a1f22b2ab..07469cb09 100644 --- a/apps/decodex/src/orchestrator/prompting.rs +++ b/apps/decodex/src/orchestrator/prompting.rs @@ -287,12 +287,12 @@ fn build_continuation_user_input( fn build_handoff_internal_review_guidance(internal_review_mode: InternalReviewMode) -> String { match internal_review_mode { InternalReviewMode::Loop => format!( - "- Follow the repo-native bounded review method from `WORKFLOW.md`: review the actual current diff and branch state, run both the requirements pass and the adversarial reviewer pass, fix only the smallest coherent owned batch, rerun verification, and re-read `HEAD` before deciding the next normalized review status.\n- Every time the repo-native bounded review method produces a result for the current head, call `{}` with that normalized status, the exact current `HEAD` SHA, and any concise evidence items.\n", + "- Request an independent fresh-context read-only review pass for the actual current diff and branch state. The reviewer must not edit files, push, land, or mutate tracker state.\n- Use the repo-native bounded review method from `WORKFLOW.md`: run the requirements pass and the adversarial reviewer pass against the current `HEAD`, including regression risk, missing tests, docs/config drift, migration fallout, operator-facing fallout, and mismatch with the accepted Loop/Decision Contract.\n- Validate reviewer comments before repair. Record accepted findings separately from rejected or non-actionable comments, fix only the smallest coherent owned batch, rerun verification, and re-read `HEAD` before deciding the normalized review status.\n- Every time the independent review pass produces a result for the current head, call `{}` with reviewer `independent_fresh_context`, that normalized status, the exact current `HEAD` SHA, concise evidence, checklist notes, and structured accepted/rejected findings.\n", ISSUE_REVIEW_CHECKPOINT_TOOL_NAME ), InternalReviewMode::Prompt => format!("- {PROMPT_ONLY_INTERNAL_REVIEW_INSTRUCTION}\n"), InternalReviewMode::Off => format!( - "- `codex.internal_review_mode = \"off\"` for this project, so skip internal self-review and do not call `{}`.\n", + "- `codex.internal_review_mode = \"off\"` for this project, so skip internal review and do not call `{}`.\n", ISSUE_REVIEW_CHECKPOINT_TOOL_NAME ), } @@ -301,12 +301,12 @@ fn build_handoff_internal_review_guidance(internal_review_mode: InternalReviewMo fn build_repair_internal_review_guidance(internal_review_mode: InternalReviewMode) -> String { match internal_review_mode { InternalReviewMode::Loop => format!( - "- Follow the repo-native bounded review method from `WORKFLOW.md`: review the actual repaired branch state, run both the requirements pass and the adversarial reviewer pass, fix only the smallest coherent owned batch, rerun verification, and re-read `HEAD` before deciding the next normalized review status.\n- Every time the repo-native bounded review method produces a result for the current repaired head, call `{}` with that normalized status, the exact current `HEAD` SHA, and any concise evidence items.\n", + "- Request an independent fresh-context read-only review pass for the actual repaired branch state. The reviewer must not edit files, push, land, or mutate tracker state.\n- Use the repo-native bounded review method from `WORKFLOW.md`: run the requirements pass and the adversarial reviewer pass against the current repaired `HEAD`, including regression risk, missing tests, docs/config drift, migration fallout, operator-facing fallout, and mismatch with the accepted Loop/Decision Contract.\n- Validate reviewer comments before repair. Record accepted findings separately from rejected or non-actionable comments, fix only the smallest coherent owned batch, rerun verification, and re-read `HEAD` before deciding the normalized review status.\n- Every time the independent review pass produces a result for the current repaired head, call `{}` with reviewer `independent_fresh_context`, that normalized status, the exact current `HEAD` SHA, concise evidence, checklist notes, and structured accepted/rejected findings.\n", ISSUE_REVIEW_CHECKPOINT_TOOL_NAME ), InternalReviewMode::Prompt => format!("- {PROMPT_ONLY_INTERNAL_REVIEW_INSTRUCTION}\n"), InternalReviewMode::Off => format!( - "- `codex.internal_review_mode = \"off\"` for this project, so skip internal self-review and do not call `{}`.\n", + "- `codex.internal_review_mode = \"off\"` for this project, so skip internal review and do not call `{}`.\n", ISSUE_REVIEW_CHECKPOINT_TOOL_NAME ), } @@ -347,12 +347,12 @@ fn build_handoff_continuation_review_guidance( ) -> String { match internal_review_mode { InternalReviewMode::Loop => format!( - "- Resume the repo-native bounded review method from `WORKFLOW.md`: review the actual current diff and branch state, run both the requirements pass and the adversarial reviewer pass, fix only the smallest coherent owned batch, rerun verification, and re-read `HEAD` before deciding the next normalized review status.\n- After each bounded review result for the current head, call `{}` with the normalized status and current `HEAD` SHA.\n", + "- Resume by requesting an independent fresh-context read-only review pass for the actual current diff and branch state; the reviewer must not edit files, push, land, or mutate tracker state.\n- Apply the repo-native bounded review method from `WORKFLOW.md`, validate comments before repair, record accepted findings separately from rejected or non-actionable comments, fix only the smallest coherent owned batch, rerun verification, and re-read `HEAD` before deciding the normalized review status.\n- After each independent review result for the current head, call `{}` with reviewer `independent_fresh_context`, the normalized status, current `HEAD` SHA, checklist notes, and structured accepted/rejected findings.\n", ISSUE_REVIEW_CHECKPOINT_TOOL_NAME ), InternalReviewMode::Prompt => format!("- {PROMPT_ONLY_INTERNAL_REVIEW_INSTRUCTION}\n"), InternalReviewMode::Off => format!( - "- `codex.internal_review_mode = \"off\"` for this project, so continue without internal self-review and do not call `{}`.\n", + "- `codex.internal_review_mode = \"off\"` for this project, so continue without internal review and do not call `{}`.\n", ISSUE_REVIEW_CHECKPOINT_TOOL_NAME ), } @@ -361,12 +361,12 @@ fn build_handoff_continuation_review_guidance( fn build_repair_continuation_review_guidance(internal_review_mode: InternalReviewMode) -> String { match internal_review_mode { InternalReviewMode::Loop => format!( - "- Resume the repo-native bounded review method from `WORKFLOW.md`: review the actual repaired branch state, run both the requirements pass and the adversarial reviewer pass, fix only the smallest coherent owned batch, rerun verification, and re-read `HEAD` before deciding the next normalized review status.\n- After each bounded review result for the repaired head, call `{}` with the normalized status and current `HEAD` SHA.\n", + "- Resume by requesting an independent fresh-context read-only review pass for the actual repaired branch state; the reviewer must not edit files, push, land, or mutate tracker state.\n- Apply the repo-native bounded review method from `WORKFLOW.md`, validate comments before repair, record accepted findings separately from rejected or non-actionable comments, fix only the smallest coherent owned batch, rerun verification, and re-read `HEAD` before deciding the normalized review status.\n- After each independent review result for the repaired head, call `{}` with reviewer `independent_fresh_context`, the normalized status, current `HEAD` SHA, checklist notes, and structured accepted/rejected findings.\n", ISSUE_REVIEW_CHECKPOINT_TOOL_NAME ), InternalReviewMode::Prompt => format!("- {PROMPT_ONLY_INTERNAL_REVIEW_INSTRUCTION}\n"), InternalReviewMode::Off => format!( - "- `codex.internal_review_mode = \"off\"` for this project, so continue without internal self-review and do not call `{}`.\n", + "- `codex.internal_review_mode = \"off\"` for this project, so continue without internal review and do not call `{}`.\n", ISSUE_REVIEW_CHECKPOINT_TOOL_NAME ), } diff --git a/apps/decodex/src/orchestrator/tests/intake/run_and_prompting.rs b/apps/decodex/src/orchestrator/tests/intake/run_and_prompting.rs index 0925fd192..62bbc81c9 100644 --- a/apps/decodex/src/orchestrator/tests/intake/run_and_prompting.rs +++ b/apps/decodex/src/orchestrator/tests/intake/run_and_prompting.rs @@ -866,8 +866,9 @@ fn review_repair_prompts_require_same_pr_repair_completion() { assert!(developer_instructions.contains("do not call `issue_review_handoff`")); assert!( developer_instructions - .contains("Follow the repo-native bounded review method from `WORKFLOW.md`") + .contains("Request an independent fresh-context read-only review pass") ); + assert!(developer_instructions.contains("structured accepted/rejected findings")); assert!(developer_instructions.contains( "including non-thread review summaries, validate the claim against the codebase, tests, and requirements" )); @@ -878,7 +879,8 @@ fn review_repair_prompts_require_same_pr_repair_completion() { assert!(developer_instructions.contains("Do not merge or land the PR yourself")); assert!(user_input.contains(pr_url)); assert!(user_input.contains(ISSUE_REVIEW_CHECKPOINT_TOOL_NAME)); - assert!(user_input.contains("Follow the repo-native bounded review method from `WORKFLOW.md`")); + assert!(user_input.contains("Request an independent fresh-context read-only review pass")); + assert!(user_input.contains("structured accepted/rejected findings")); assert!(user_input.contains( "Read the current review feedback on `https://github.com/hack-ink/decodex/pull/77`, including non-thread review summaries" )); @@ -896,8 +898,9 @@ fn review_repair_prompts_require_same_pr_repair_completion() { assert!(continuation_input.contains(ISSUE_REVIEW_CHECKPOINT_TOOL_NAME)); assert!( continuation_input - .contains("Resume the repo-native bounded review method from `WORKFLOW.md`") + .contains("Resume by requesting an independent fresh-context read-only review pass") ); + assert!(continuation_input.contains("structured accepted/rejected findings")); assert!(continuation_input.contains( "Validate each actionable review claim against the codebase, tests, and requirements before changing code" )); diff --git a/apps/decodex/src/orchestrator/tests/intake/workflow_reload.rs b/apps/decodex/src/orchestrator/tests/intake/workflow_reload.rs index fbb94daec..4df0a7810 100644 --- a/apps/decodex/src/orchestrator/tests/intake/workflow_reload.rs +++ b/apps/decodex/src/orchestrator/tests/intake/workflow_reload.rs @@ -178,7 +178,7 @@ fn expected_developer_instructions( )); sections.push(format!( - "Tracker tool contract\n- You own issue-scoped tracker writes for `{issue}`.\n- At the start of execution, call `{transition_tool}` to move the issue to `{in_progress}`. Decodex already records the run-start Linear ledger, so do not add a separate start comment.\n- Update `{progress_checkpoint_tool}` whenever the execution phase, focus, next action, blockers, evidence, or verification state changes materially.\n- Follow the repo-native bounded review method from `WORKFLOW.md`: review the actual current diff and branch state, run both the requirements pass and the adversarial reviewer pass, fix only the smallest coherent owned batch, rerun verification, and re-read `HEAD` before deciding the next normalized review status.\n- Every time the repo-native bounded review method produces a result for the current head, call `{review_checkpoint_tool}` with that normalized status, the exact current `HEAD` SHA, and any concise evidence items.\n- Treat failures from repo-native `canonicalize_commands`, `verify_commands`, or tracked rewrites left by that repo gate as continued repair by default: keep fixing the lane and rerun the gate instead of taking `manual_attention` unless the blocker is clearly toolchain, environment, or operator-owned.\n- When the implementation is ready, commit the lane, push branch `{branch}`, and create or update a non-draft PR titled `{pr_title}` for that branch.\n- Call `{review_handoff_tool}` only after the latest `{review_checkpoint_tool}` for this handoff phase and current `HEAD` is `clean`. Then call `{terminal_finalize_tool}` with path `review_handoff`.\n- If you determine the issue needs human attention, add label `{needs_attention}` with `{label_tool}`, then call `issue_comment` with kind `manual_attention` and structured public fields (`error_class`, `next_action`, `blockers`, `evidence`; include `failed_command` and `raw_error` only when public-safe), and then call `{terminal_finalize_tool}` with path `manual_attention`. Do not speculate about capabilities you did not directly verify. Do not call `{review_handoff_tool}` in that case; `decodex` will stop the lane as a human-required failure without automatic retry.\n- Do not move the issue directly to `{success}` with `{transition_tool}`. `decodex` will complete the success writeback only after its own validation passes.\n- Do not report the run as complete or treat `{progress_checkpoint_tool}` as terminal completion until `{terminal_finalize_tool}` succeeds.{continuation_guidance}\n- Never write to any other issue.", + "Tracker tool contract\n- You own issue-scoped tracker writes for `{issue}`.\n- At the start of execution, call `{transition_tool}` to move the issue to `{in_progress}`. Decodex already records the run-start Linear ledger, so do not add a separate start comment.\n- Update `{progress_checkpoint_tool}` whenever the execution phase, focus, next action, blockers, evidence, or verification state changes materially.\n- Request an independent fresh-context read-only review pass for the actual current diff and branch state. The reviewer must not edit files, push, land, or mutate tracker state.\n- Use the repo-native bounded review method from `WORKFLOW.md`: run the requirements pass and the adversarial reviewer pass against the current `HEAD`, including regression risk, missing tests, docs/config drift, migration fallout, operator-facing fallout, and mismatch with the accepted Loop/Decision Contract.\n- Validate reviewer comments before repair. Record accepted findings separately from rejected or non-actionable comments, fix only the smallest coherent owned batch, rerun verification, and re-read `HEAD` before deciding the normalized review status.\n- Every time the independent review pass produces a result for the current head, call `{review_checkpoint_tool}` with reviewer `independent_fresh_context`, that normalized status, the exact current `HEAD` SHA, concise evidence, checklist notes, and structured accepted/rejected findings.\n- Treat failures from repo-native `canonicalize_commands`, `verify_commands`, or tracked rewrites left by that repo gate as continued repair by default: keep fixing the lane and rerun the gate instead of taking `manual_attention` unless the blocker is clearly toolchain, environment, or operator-owned.\n- When the implementation is ready, commit the lane, push branch `{branch}`, and create or update a non-draft PR titled `{pr_title}` for that branch.\n- Call `{review_handoff_tool}` only after the latest `{review_checkpoint_tool}` for this handoff phase and current `HEAD` is `clean`. Then call `{terminal_finalize_tool}` with path `review_handoff`.\n- If you determine the issue needs human attention, add label `{needs_attention}` with `{label_tool}`, then call `issue_comment` with kind `manual_attention` and structured public fields (`error_class`, `next_action`, `blockers`, `evidence`; include `failed_command` and `raw_error` only when public-safe), and then call `{terminal_finalize_tool}` with path `manual_attention`. Do not speculate about capabilities you did not directly verify. Do not call `{review_handoff_tool}` in that case; `decodex` will stop the lane as a human-required failure without automatic retry.\n- Do not move the issue directly to `{success}` with `{transition_tool}`. `decodex` will complete the success writeback only after its own validation passes.\n- Do not report the run as complete or treat `{progress_checkpoint_tool}` as terminal completion until `{terminal_finalize_tool}` succeeds.{continuation_guidance}\n- Never write to any other issue.", issue = issue_run.issue.identifier, transition_tool = ISSUE_TRANSITION_TOOL_NAME, label_tool = ISSUE_LABEL_ADD_TOOL_NAME, diff --git a/apps/decodex/src/state/internal.rs b/apps/decodex/src/state/internal.rs index ccf227d9b..840853acc 100644 --- a/apps/decodex/src/state/internal.rs +++ b/apps/decodex/src/state/internal.rs @@ -401,12 +401,33 @@ CREATE TABLE IF NOT EXISTS review_policy_checkpoints ( status TEXT NOT NULL, head_sha TEXT NOT NULL, nonclean_rounds INTEGER NOT NULL, + details_json TEXT NOT NULL DEFAULT '{}', updated_at TEXT NOT NULL, updated_at_unix INTEGER NOT NULL, PRIMARY KEY (project_id, issue_id, run_id, attempt_number, phase) ); "#, )?; + self.ensure_column( + "review_policy_checkpoints", + "details_json", + "ALTER TABLE review_policy_checkpoints ADD COLUMN details_json TEXT NOT NULL DEFAULT '{}'", + )?; + + Ok(()) + } + + fn ensure_column(&self, table: &str, column: &str, add_column_sql: &str) -> Result<()> { + let mut statement = self.connection.prepare(&format!("PRAGMA table_info({table})"))?; + let column_names = statement.query_map([], |row| row.get::<_, String>(1))?; + + for column_name in column_names { + if column_name? == column { + return Ok(()); + } + } + + self.connection.execute_batch(add_column_sql)?; Ok(()) } @@ -488,7 +509,7 @@ CREATE TABLE IF NOT EXISTS schema_meta ( value TEXT NOT NULL ); INSERT INTO schema_meta (key, value) -VALUES ('schema_version', '8') +VALUES ('schema_version', '9') ON CONFLICT(key) DO UPDATE SET value = excluded.value; "#, )?; @@ -787,10 +808,10 @@ ON CONFLICT(key) DO UPDATE SET value = excluded.value; transaction.execute( "INSERT OR IGNORE INTO review_policy_checkpoints ( project_id, issue_id, run_id, attempt_number, phase, status, head_sha, - nonclean_rounds, updated_at, updated_at_unix + nonclean_rounds, details_json, updated_at, updated_at_unix ) SELECT project_id, ?2, run_id, attempt_number, phase, status, head_sha, - nonclean_rounds, updated_at, updated_at_unix + nonclean_rounds, details_json, updated_at, updated_at_unix FROM review_policy_checkpoints WHERE issue_id = ?1", params![previous_issue_id, canonical_issue_id], )?; @@ -1493,7 +1514,7 @@ ON CONFLICT(key) DO UPDATE SET value = excluded.value; fn load_review_policy_checkpoints(&self, state: &mut StateData) -> Result<()> { let mut statement = self.connection.prepare( "SELECT project_id, issue_id, run_id, attempt_number, phase, status, head_sha, \ - nonclean_rounds, updated_at, updated_at_unix FROM review_policy_checkpoints", + nonclean_rounds, details_json, updated_at, updated_at_unix FROM review_policy_checkpoints", )?; let rows = statement.query_map([], |row| { let project_id: String = row.get(0)?; @@ -1513,8 +1534,9 @@ ON CONFLICT(key) DO UPDATE SET value = excluded.value; status: row.get(5)?, head_sha: row.get(6)?, nonclean_rounds: row.get(7)?, - updated_at: row.get(8)?, - updated_at_unix: row.get(9)?, + details_json: row.get(8)?, + updated_at: row.get(9)?, + updated_at_unix: row.get(10)?, }, )) })?; @@ -1811,6 +1833,7 @@ struct ReviewPolicyRuntimeRecord { status: String, head_sha: String, nonclean_rounds: i64, + details_json: String, updated_at: String, updated_at_unix: i64, } @@ -1825,6 +1848,7 @@ impl ReviewPolicyRuntimeRecord { status: self.status.clone(), head_sha: self.head_sha.clone(), nonclean_rounds: self.nonclean_rounds, + details_json: self.details_json.clone(), updated_at: self.updated_at.clone(), updated_at_unix: self.updated_at_unix, } @@ -2741,8 +2765,8 @@ fn persist_review_policy_checkpoints( transaction.execute( "INSERT OR REPLACE INTO review_policy_checkpoints ( project_id, issue_id, run_id, attempt_number, phase, status, head_sha, - nonclean_rounds, updated_at, updated_at_unix - ) VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8, ?9, ?10)", + nonclean_rounds, details_json, updated_at, updated_at_unix + ) VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8, ?9, ?10, ?11)", params![ record.project_id, record.issue_id, @@ -2752,6 +2776,7 @@ fn persist_review_policy_checkpoints( record.status, record.head_sha, record.nonclean_rounds, + record.details_json, record.updated_at, record.updated_at_unix, ], diff --git a/apps/decodex/src/state/models.rs b/apps/decodex/src/state/models.rs index ed7ecee9a..4df8a533e 100644 --- a/apps/decodex/src/state/models.rs +++ b/apps/decodex/src/state/models.rs @@ -509,6 +509,7 @@ pub(crate) struct ReviewPolicyCheckpoint { status: String, head_sha: String, nonclean_rounds: i64, + details_json: String, updated_at: String, updated_at_unix: i64, } @@ -546,6 +547,10 @@ impl ReviewPolicyCheckpoint { self.nonclean_rounds } + pub(crate) fn details_json(&self) -> &str { + &self.details_json + } + pub(crate) fn updated_at(&self) -> &str { &self.updated_at } diff --git a/apps/decodex/src/state/store.rs b/apps/decodex/src/state/store.rs index 0dd0d5ecb..d52f4d7cd 100644 --- a/apps/decodex/src/state/store.rs +++ b/apps/decodex/src/state/store.rs @@ -21,6 +21,7 @@ pub(crate) struct ReviewPolicyCheckpointInput<'a> { pub(crate) status: &'a str, pub(crate) head_sha: &'a str, pub(crate) nonclean_rounds: i64, + pub(crate) details_json: &'a str, } /// Local runtime store for leases, attempts, worktrees, protocol events, and private evidence. @@ -1683,6 +1684,7 @@ impl StateStore { status: input.status.to_owned(), head_sha: input.head_sha.to_owned(), nonclean_rounds: input.nonclean_rounds, + details_json: input.details_json.to_owned(), updated_at: now.text, updated_at_unix: now.unix, }; diff --git a/apps/decodex/src/state/tests.rs b/apps/decodex/src/state/tests.rs index 9545e1020..085d77813 100644 --- a/apps/decodex/src/state/tests.rs +++ b/apps/decodex/src/state/tests.rs @@ -81,6 +81,7 @@ fn upsert_handoff_review_policy_checkpoint( status, head_sha, nonclean_rounds, + details_json: "{}", }) .expect("review policy checkpoint should persist"); } @@ -330,6 +331,7 @@ fn review_policy_checkpoints_persist_reload_and_clear_for_run_attempt() { status: "findings", head_sha: "abc123", nonclean_rounds: 2, + details_json: r#"{"reviewer":"independent_fresh_context"}"#, }) .expect("review policy checkpoint should persist"); @@ -341,6 +343,7 @@ fn review_policy_checkpoints_persist_reload_and_clear_for_run_attempt() { assert_eq!(checkpoint.status(), "findings"); assert_eq!(checkpoint.head_sha(), "abc123"); assert_eq!(checkpoint.nonclean_rounds(), 2); + assert_eq!(checkpoint.details_json(), r#"{"reviewer":"independent_fresh_context"}"#); assert!(!checkpoint.updated_at().is_empty()); assert!(checkpoint.updated_at_unix() > 0); @@ -352,6 +355,7 @@ fn review_policy_checkpoints_persist_reload_and_clear_for_run_attempt() { assert_eq!(reloaded.status(), "findings"); assert_eq!(reloaded.nonclean_rounds(), 2); + assert_eq!(reloaded.details_json(), r#"{"reviewer":"independent_fresh_context"}"#); reopened .clear_review_policy_checkpoints_for_run_attempt("pubfi", "PUB-101", "run-1", 2) diff --git a/docs/reference/operator-control-plane.md b/docs/reference/operator-control-plane.md index a4cab474b..4491955e4 100644 --- a/docs/reference/operator-control-plane.md +++ b/docs/reference/operator-control-plane.md @@ -131,7 +131,7 @@ work that needs full private payload values. | Surface | Owns | Does Not Own | | --- | --- | --- | -| Runtime SQLite DB | active leases, attempts, run-control channels, protocol events, private execution events, worktree mappings, retry state, retained PR state, review-policy checkpoints, phase timing, connector backoff, project registry | human backlog grooming or durable team-visible issue history | +| Runtime SQLite DB | active leases, attempts, run-control channels, protocol events, private execution events, worktree mappings, retry state, retained PR state, review-policy checkpoints with structured independent-review detail, phase timing, connector backoff, project registry | human backlog grooming or durable team-visible issue history | | Central project config | `service_id`, repo root, worktree root, tracker/GitHub credential env-var names, enabled project registration | per-run state or issue ownership | | Project `WORKFLOW.md` | repo policy, validation gate, state names, retry/review policy | runtime ownership, queue labels, credentials, model overrides | | Linear | team-visible issue state, queue/active/manual-attention labels, coarse execution ledger comments, progress/failure/handoff/closeout summaries | high-frequency runtime truth, heartbeat, token pressure, raw attempts, private execution evidence, connector retry budgets | @@ -434,9 +434,10 @@ rate-limited, or unavailable. - Linear writes should stay coarse: one run-start ledger, material progress checkpoints, PR-ready/handoff, blocked/failed, landed, done, and cleanup summaries. Full structured execution evidence belongs in private runtime SQLite events. -- Fine-grained retry budgets, review-policy checkpoints, raw attempts, heartbeat, - child buckets, token pressure, recovery details, and process logs stay local. Logs are diagnostic text; private - execution events are structured runtime evidence. +- Fine-grained retry budgets, review-policy checkpoints, structured accepted and + rejected independent-review findings, raw attempts, heartbeat, child buckets, token + pressure, recovery details, and process logs stay local. Logs are diagnostic text; + private execution events are structured runtime evidence. - Completed lanes without Decodex Linear execution ledger records are reported as `missing` / `execution_ledger_missing`. Tracker terminal state, local attempt success, and non-ledger comments never satisfy the Run Ledger outcome contract. diff --git a/docs/spec/installable-agent-policy.md b/docs/spec/installable-agent-policy.md index 8d5f5086e..395a04582 100644 --- a/docs/spec/installable-agent-policy.md +++ b/docs/spec/installable-agent-policy.md @@ -54,7 +54,7 @@ policy. | Project execution gates, canonicalization and verification commands, gate profiles, and workspace hooks | [`workflow-file.md`](./workflow-file.md) plus the registered project `WORKFLOW.md` | | Service identity, repo root, worktree root, and tracker or GitHub credential environment-variable names | Centralized project `project.toml`; see the operator surface map in [`../reference/operator-control-plane.md`](../reference/operator-control-plane.md) | | Automatic intake labels, active ownership, retry behavior, and retained lane planning | [`runtime.md`](./runtime.md) and [`owned-lane-policy.md`](./owned-lane-policy.md) | -| Review handoff, bounded self-review, external-review pass signals, repair rounds, and architecture escalation | [`review-orchestration.md`](./review-orchestration.md) and the registered project `WORKFLOW.md` bounded review method | +| Review handoff, bounded independent review, external-review pass signals, repair rounds, and architecture escalation | [`review-orchestration.md`](./review-orchestration.md) and the registered project `WORKFLOW.md` bounded review method | | Post-`In Review` waiting, repair, landing, closeout, cleanup, and manual-intervention phases | [`post-review-lifecycle.md`](./post-review-lifecycle.md) | | Local commit-message schema for Decodex-managed history | [`commit-messages.md`](./commit-messages.md) | | Operator procedures, pilot setup, and live validation steps | [`../runbook/index.md`](../runbook/index.md) and the specific runbook for the procedure | diff --git a/docs/spec/loop-runtime.md b/docs/spec/loop-runtime.md index 9475cd821..6d32a35f2 100644 --- a/docs/spec/loop-runtime.md +++ b/docs/spec/loop-runtime.md @@ -178,7 +178,8 @@ read-only review pass. This pass is distinct from in-thread self-review: - it does not rely on the implementer's memory of the change - it stays read-only while producing findings - it checks intended behavior, regression risk, tests, docs/config drift, migration - fallout, and operator-facing fallout + fallout, operator-facing fallout, and mismatch with the accepted Loop/Decision + Contract - candidate findings must be validated before repair work changes the lane The review orchestration contract, including internal/external review modes and diff --git a/docs/spec/owned-lane-policy.md b/docs/spec/owned-lane-policy.md index 8ee36721e..802656b57 100644 --- a/docs/spec/owned-lane-policy.md +++ b/docs/spec/owned-lane-policy.md @@ -155,7 +155,7 @@ This action requires: | `In Review` lane has no actionable review yet | PR still belongs to lane; no requested changes that require repair; checks or review are still pending | `wait_for_external_signal` | Yes, when new signal arrives | | `In Review` lane now has actionable review repair work | PR still belongs to lane; actionable review feedback is present; retained lane remains reusable | `resume_retained_lane` | Yes | | `In Review` lane has green checks, satisfied review, and is mergeable | PR still belongs to lane; approvals satisfied; unresolved blocking review work absent; checks green; mergeable | `ready_to_land` | Yes | -| Pre-PR self-review or post-review repair churn exceeded the configured convergence budget | Runtime-owned review checkpoints show repeated `findings` in the same phase crossed the configured limit; the lane no longer has a bounded low-risk patch path | `manual_intervention_required` | No | +| Pre-PR independent review or post-review repair churn exceeded the configured convergence budget | Runtime-owned review checkpoints show repeated `findings` in the same phase crossed the configured limit; the lane no longer has a bounded low-risk patch path | `manual_intervention_required` | No | | Review-policy stop may need research escalation | Runtime-owned review checkpoints identify `needs_architecture_review` or repeated `findings` exhaustion with matching current head, phase, evidence, and stop class | `manual_intervention_required` | No direct retry; future research escalation may run only through a separate structured adapter contract | | Merge already happened but closeout or cleanup is incomplete | Merged PR is authoritative; closeout or cleanup evidence is still missing | `continue` | Yes | | Signals are contradictory or incomplete in a way that requires guesswork | Tracker, retained lane, review, or cleanup signals disagree materially | `manual_intervention_required` | No | @@ -174,8 +174,11 @@ For review-policy churn, the runtime counts only structured review checkpoints: - phase is `handoff` before the first PR-backed review handoff succeeds - phase is `repair` during retained review-repair runs -- every `findings` checkpoint increments the runtime checkpoint's non-clean round count +- every `findings` checkpoint increments the runtime checkpoint's non-clean round + count and must carry at least one accepted independent-review finding - `clean` does not stop the lane and resets that non-clean round count to zero for the current phase +- `clean` may carry rejected or non-actionable reviewer comments, but those comments + are not repair input - recording `issue_review_handoff` or `issue_review_repair_complete` clears the retained review-policy state Review-policy stops are also the only review failures eligible for a future @@ -264,9 +267,9 @@ This policy is not normative about the exact local helper names used to satisfy In particular: -- deciding whether a pre-PR self-review gate is satisfied belongs to orchestration +- deciding whether a pre-PR independent-review gate is satisfied belongs to orchestration - deciding whether the current PR head needs a fresh review request belongs to orchestration -- counting pre-PR self-review churn and post-review repair churn belongs to orchestration +- counting pre-PR independent-review churn and post-review repair churn belongs to orchestration - deciding that the lane has crossed from bounded repair into architecture rethink or manual escalation belongs to orchestration - executing the concrete review-request side effect belongs to the local workflow adapter - retrying that side effect after missing acknowledgement is an orchestration policy decision implemented through the same local adapter diff --git a/docs/spec/post-review-lifecycle.md b/docs/spec/post-review-lifecycle.md index 7b27bb3e6..558a2aae4 100644 --- a/docs/spec/post-review-lifecycle.md +++ b/docs/spec/post-review-lifecycle.md @@ -175,6 +175,9 @@ While in `review_repair`: - the runtime must reuse the retained lane when it is still valid - repair work must stay bound to the same issue, branch lineage, and PR - the runtime must validate each external-review claim against the codebase, tests, and requirements before changing code +- when a fresh-context `issue_review_checkpoint` exists for the repair phase, repair + work must operate on accepted findings from that checkpoint; rejected or + non-actionable comments remain evidence, not repair scope - the repaired head must pass the local pre-review gate before being pushed - when `codex.internal_review_mode = "loop"`, every repaired-head bounded-review result must first be recorded through `issue_review_checkpoint` - every addressed review thread must receive an in-thread reply for the repaired head @@ -194,7 +197,7 @@ repeated repair checkpoints stay in `findings` for three consecutive rounds, or checkpoint reports `needs_architecture_review` / `blocked`, the runtime must stop for human intervention instead of patch-on-patch churn. When `codex.internal_review_mode = "prompt"` or `"off"`, retained repair completion skips that -self-review checkpoint requirement but still requires the repaired head to be pushed and +independent-review checkpoint requirement but still requires the repaired head to be pushed and the configured repository validation gate to pass. The same completion also requires that the PR still belongs to the retained lane, points at the repaired lane HEAD, and remains open and ready for fresh review. diff --git a/docs/spec/review-orchestration.md b/docs/spec/review-orchestration.md index 1ec0e40cb..3f6301c15 100644 --- a/docs/spec/review-orchestration.md +++ b/docs/spec/review-orchestration.md @@ -29,8 +29,9 @@ There is one shared review loop for Decodex-owned lanes: 3. validate and repair the actionable findings for the current lane head 4. request review again until the lane passes or stops for escalation -Internal self-review is selected per service through the registered project config -`[codex].internal_review_mode`. The supported modes are `"loop"`, `"prompt"`, and `"off"`. +Internal review behavior is selected per service through the registered project config +`[codex].internal_review_mode`. The supported modes are `"loop"`, `"prompt"`, and +`"off"`. External GitHub review may be enabled or disabled per service through the registered project config `[codex].external_review_enabled`. When enabled, each review source participates in the retained review loop. When external review is disabled, Decodex skips the runtime-owned `@codex @@ -39,7 +40,8 @@ satisfied and the PR is already on the deterministic clean merge path. When external review is enabled, the difference is reviewer source: -- Internal self-review in `"loop"` mode uses a runtime-controlled Codex review checkpoint for the current lane head. +- Internal review in `"loop"` mode uses a runtime-controlled independent + fresh-context read-only Codex review checkpoint for the current lane head. - Internal self-review in `"prompt"` mode injects only the prompt `Review your work repeatedly and fix any logic bugs until no new issues are found.` and does not expose or require the checkpoint tool. - External review uses a GitHub PR comment request by posting `@codex review` on the current PR. @@ -59,7 +61,10 @@ After any review arrives: - repair only the verified issues - keep the repair batch scoped to the smallest coherent owned change set - rerun the repository validation required for the current head before the next review request -- when `codex.internal_review_mode = "loop"`, record the normalized bounded-review result for the exact current `HEAD` through `issue_review_checkpoint` +- when `codex.internal_review_mode = "loop"`, record the normalized bounded-review + result for the exact current `HEAD` through `issue_review_checkpoint`, including + the explicit independent reviewer source, checklist notes, accepted findings, + rejected findings, non-empty evidence, and repair guidance The current repository's bounded review method is defined in the registered project `WORKFLOW.md`. This spec does not replace that method; it defines how review requests and review outcomes are orchestrated around it. @@ -81,24 +86,34 @@ Rules: - if the repeated churn is rooted in an architectural defect or root-cause issue that local patching will not converge, stop for `manual_intervention_required` - if the findings are normal and not rooted in architectural churn, continue and reset that review mode's three-round budget -## Internal self-review +## Internal review modes -Internal self-review mode is service-controlled. +Internal review mode is service-controlled. Rules: -- `codex.internal_review_mode = "loop"` uses the runtime-owned internal self-review checkpoint loop. Decodex exposes `issue_review_checkpoint`, requires a current `clean` checkpoint before `issue_review_handoff` or `issue_review_repair_complete`, and applies the review-policy stop rules to stale or non-clean checkpoint state. +- `codex.internal_review_mode = "loop"` uses the runtime-owned independent + fresh-context read-only review checkpoint loop. Decodex exposes + `issue_review_checkpoint`, requires a current `clean` checkpoint before + `issue_review_handoff` or `issue_review_repair_complete`, stores structured + accepted/rejected finding evidence, and applies the review-policy stop rules to + stale or non-clean checkpoint state. - `codex.internal_review_mode = "prompt"` injects exactly the prompt `Review your work repeatedly and fix any logic bugs until no new issues are found.` into the agent instructions. Decodex does not expose `issue_review_checkpoint`, does not require a clean checkpoint before handoff or repair completion, and ignores stale review-policy checkpoint state for turn-stop classification. -- `codex.internal_review_mode = "off"` skips internal self-review. Decodex does not expose `issue_review_checkpoint`, does not require a clean checkpoint before handoff or repair completion, and ignores stale review-policy checkpoint state for turn-stop classification. +- `codex.internal_review_mode = "off"` skips internal review. Decodex does not expose `issue_review_checkpoint`, does not require a clean checkpoint before handoff or repair completion, and ignores stale review-policy checkpoint state for turn-stop classification. - Omitted `codex.internal_review_mode` defaults to `"loop"`. `codex.internal_review_enabled` is rejected. -- In `"loop"` mode, the runtime may choose the exact local transport or child-conversation mechanism, but it must remain a fully runtime-controlled review request. +- In `"loop"` mode, the runtime may choose the exact local transport or + child-conversation mechanism, but it must remain a fully runtime-controlled + read-only review request. The reviewer must not edit files, push, land, or mutate + tracker state. - In `"loop"` mode, internal review must use the same bounded review method and normalized review outcomes as any other review pass. +- In `"loop"` mode, a `findings` checkpoint requires at least one accepted finding; + rejected or non-actionable reviewer comments may be recorded with a `clean` + checkpoint and must not become repair input. - If `"loop"` mode internal review returns an ambiguous or contradictory result that the runtime cannot classify without guessing, stop for `manual_intervention_required`. - Internal review pass transitions into the normal PR-backed review handoff flow, not directly into landing. -- Internal self-review is not the same thing as an independent fresh-context - read-only review. When the loop-runtime risk policy requires independent review, use - the separate review boundary in [`loop-runtime.md`](./loop-runtime.md) before treating - the lane as ready for handoff or landing. +- Prompt-only self-review is not sufficient when the loop-runtime risk policy + requires independent review. Use the loop-mode independent checkpoint boundary + before treating the lane as ready for handoff or landing. ## External GitHub review diff --git a/docs/spec/runtime.md b/docs/spec/runtime.md index 6170e6825..91ab27cff 100644 --- a/docs/spec/runtime.md +++ b/docs/spec/runtime.md @@ -68,7 +68,7 @@ mirror: | --- | --- | | Runtime SQLite `private_execution_events` | Structured private execution evidence for the local Decodex installation. This is where full checkpoint payloads, verification notes, local head evidence, and recovery detail belong. | | Runtime SQLite `run_control_channels` | Local control capability metadata for active run attempts. It records the project, issue, run id, attempt, transport, local channel path, channel status, and publish/update timestamps needed to route future control requests without bypassing active lease ownership. | -| Runtime SQLite `review_policy_checkpoints` | Latest bounded-review checkpoint state for one project, issue, run, attempt, and phase. This row is the authority for review handoff and retained repair gating. | +| Runtime SQLite `review_policy_checkpoints` | Latest bounded-review checkpoint state for one project, issue, run, attempt, and phase, including structured independent-review detail. This row is the authority for review handoff and retained repair gating. | | Agent evidence under `~/.codex/decodex/agent-evidence//` | Derived local handoff view for repair agents. It may reference private evidence readback commands and compact run capsules, but it is not scheduling authority and is not a public mirror. | | Logs under `~/.codex/decodex/logs/` and `.decodex-run-activity` | Diagnostic process and liveness signals. They may explain what a local process did, but they are not the structured execution ledger and must not be replayed as tracker state. | | Linear execution ledger comments | Low-frequency public projection for team-visible lifecycle state. They carry coarse start, progress phase, PR, handoff, failure, landing, closeout, and cleanup summaries only. | @@ -83,7 +83,8 @@ The following facts are local runtime truth and must not be rebuilt from Linear - active run-control channel metadata and local control audit events - protocol events, event counts, event timestamps, and thread/liveness hydration fields - private execution events carrying structured local evidence for an issue/run/attempt -- review-policy checkpoint state: current phase, normalized status, lane head, and consecutive non-clean round count +- review-policy checkpoint state: current phase, normalized status, lane head, + consecutive non-clean round count, and structured independent-review detail - retry and backoff state: queued retry kind, due time, retry budget, and connector backoff - phase timing and operator activity summaries - retained worktree mappings, retained PR handoff identity, post-review phase, and cleanup or repair ownership @@ -301,7 +302,28 @@ When `codex.internal_review_mode = "loop"`, handoff and retained review-repair r - latest checkpoint `findings` with three or more consecutive non-clean rounds in the same phase: fail the turn through the human-required failure path - latest checkpoint `needs_architecture_review` or `blocked`: fail the turn through the human-required failure path -`decodex` persists this review-policy state in the runtime SQLite `review_policy_checkpoints` table keyed by project, issue, run, attempt, and phase. The stored row contains `phase`, `status`, `head_sha`, and `nonclean_rounds`, and it is the only authority used to require a current clean checkpoint before `issue_review_handoff` or `issue_review_repair_complete`. Legacy `.decodex-run-activity` marker fields with the same names may be parsed to seed a missing runtime row during compatibility recovery, but a runtime-store checkpoint always wins over stale marker values. Recording `issue_review_handoff` or `issue_review_repair_complete` clears the runtime checkpoint for that run attempt and only best-effort clears any legacy marker fields. When `codex.internal_review_mode = "prompt"` or `"off"`, Decodex does not expose `issue_review_checkpoint`, does not require a clean checkpoint before review handoff or repair completion, and ignores stale review-policy state while classifying clean turn boundaries. +`decodex` persists this review-policy state in the runtime SQLite +`review_policy_checkpoints` table keyed by project, issue, run, attempt, and phase. +The stored row contains `phase`, `status`, `head_sha`, `nonclean_rounds`, and +`details_json`, and it is the only authority used to require a current clean +checkpoint before `issue_review_handoff` or `issue_review_repair_complete`. +`details_json` holds the structured independent fresh-context review payload, +including checklist notes, accepted findings, rejected findings, and repair guidance. +Each accepted checkpoint also appends a private `review_checkpoint` execution event +with the same structured payload for local operator and repair readback. Linear +receives only coarse lifecycle projections; raw reviewer findings stay in local +runtime evidence unless another allowlisted lifecycle summary renders a public-safe +summary. +Legacy `.decodex-run-activity` marker fields with the same names may be parsed to seed +a missing runtime row during compatibility recovery, but a runtime-store checkpoint +always wins over stale marker values. Legacy marker recovery seeds an empty +`details_json` payload because old markers did not carry structured finding evidence. +Recording `issue_review_handoff` or `issue_review_repair_complete` clears the runtime +checkpoint for that run attempt and only best-effort clears any legacy marker fields. +When `codex.internal_review_mode = "prompt"` or `"off"`, Decodex does not expose +`issue_review_checkpoint`, does not require a clean checkpoint before review handoff +or repair completion, and ignores stale review-policy state while classifying clean +turn boundaries. The review-policy human-required failure path is also the boundary for any later runtime-owned research escalation. The current runtime must not dispatch research from a diff --git a/docs/spec/tracker-tools.md b/docs/spec/tracker-tools.md index 69a4fd296..62b3462ee 100644 --- a/docs/spec/tracker-tools.md +++ b/docs/spec/tracker-tools.md @@ -135,8 +135,25 @@ In either invalid case, `decodex` must fail the attempt rather than infer which branch/PR projection anchor changing. Repeated private evidence updates inside the same public signal must append private runtime events without adding Linear comments. - `issue_review_checkpoint` is available only when `codex.internal_review_mode = "loop"`, and only during the pre-PR handoff phase and retained review-repair runs; `closeout` does not expose it. -- `issue_review_checkpoint` must accept only these normalized statuses: `clean`, `findings`, `needs_architecture_review`, `blocked`. -- `issue_review_checkpoint` must bind every checkpoint to an explicit `head_sha` for the currently reviewed lane head. +- `issue_review_checkpoint` must accept only these normalized statuses: + `clean`, `findings`, `needs_architecture_review`, `blocked`. +- `issue_review_checkpoint` must bind every checkpoint to an explicit `head_sha` + for the currently reviewed lane head. +- `issue_review_checkpoint` records the independent fresh-context read-only review + result as structured runtime evidence. The reviewer source is + `independent_fresh_context`; every new checkpoint payload must include checklist notes for + intended behavior, regression risk, missing tests, docs/config drift, migration + fallout, operator-facing fallout, and Loop/Decision Contract mismatch. +- Review payload findings are split into accepted findings and rejected findings. + Accepted findings are the only repair input. Rejected findings record the + rejection reason for non-actionable, stale, out-of-scope, or unvalidated reviewer + comments. +- A `findings` checkpoint must carry at least one accepted finding. A `clean` + checkpoint may carry rejected findings, but must not carry accepted findings. +- Top-level checkpoint evidence is required. Accepted findings must include severity, + non-empty evidence, file and line references when possible, and concrete repair + guidance. Rejected findings must include severity, non-empty evidence, and the + rejection reason. - When `codex.internal_review_mode = "loop"`, `decodex` treats `issue_review_checkpoint` as the only authoritative structured review-policy signal. Skill prose or wrapper-local result words must not replace it. - When `codex.internal_review_mode = "loop"`, `issue_review_handoff` and `issue_review_repair_complete` must require the latest `clean` checkpoint for the current phase and current lane head, not merely any older clean checkpoint from the same lane. - When `codex.internal_review_mode = "prompt"` or `"off"`, `issue_review_handoff` and `issue_review_repair_complete` must not require `issue_review_checkpoint`; they still must pass PR validation, branch/head checks, and the configured repository validation gate before writeback.