Allow fresh requirement review after integration
This commit is contained in:
@@ -570,7 +570,9 @@ impl MergeRequestStore {
|
||||
}
|
||||
let mr = self.get_for_operation(&i.auth.workspace_id, &i.merge_request_id, &i.ticket_id)?;
|
||||
self.assigned(&i.auth, &i.ticket_id, &mr.repository_id)?;
|
||||
if mr.state != MergeRequestState::Open || mr.selector_from.is_none() {
|
||||
if mr.state == MergeRequestState::Closed
|
||||
|| (mr.state == MergeRequestState::Open && mr.selector_from.is_none())
|
||||
{
|
||||
return Err(MergeRequestError::Conflict(
|
||||
"Merge Request is not reviewable".into(),
|
||||
));
|
||||
@@ -587,7 +589,9 @@ impl MergeRequestStore {
|
||||
let current_mr = load_mr(&t, &i.auth.workspace_id, &i.merge_request_id)?
|
||||
.filter(|request| request.ticket_ids.iter().any(|id| id == &i.ticket_id))
|
||||
.ok_or(MergeRequestError::NotFound)?;
|
||||
if current_mr.state != MergeRequestState::Open || current_mr.selector_from.is_none() {
|
||||
if current_mr.state == MergeRequestState::Closed
|
||||
|| (current_mr.state == MergeRequestState::Open && current_mr.selector_from.is_none())
|
||||
{
|
||||
return Err(MergeRequestError::Conflict(
|
||||
"Merge Request changed before review request".into(),
|
||||
));
|
||||
@@ -659,7 +663,7 @@ impl MergeRequestStore {
|
||||
JOIN merge_requests mr
|
||||
ON mr.workspace_id=g.workspace_id AND mr.merge_request_id=g.merge_request_id
|
||||
WHERE g.capability_token=?1 AND g.merge_request_id=?2
|
||||
AND g.status='issued' AND mr.state='open'",
|
||||
AND g.status='issued' AND mr.state IN('open','merged')",
|
||||
params![capability_token, merge_request_id],
|
||||
|row| {
|
||||
Ok(ReviewSubmissionAuthorization {
|
||||
@@ -695,7 +699,7 @@ impl MergeRequestStore {
|
||||
ON mr.workspace_id=g.workspace_id AND mr.merge_request_id=g.merge_request_id
|
||||
WHERE g.capability_token=?1 AND rel.ticket_id=?2
|
||||
AND g.merge_request_id=?3
|
||||
AND g.status='issued' AND mr.state='open'",
|
||||
AND g.status='issued' AND mr.state IN('open','merged')",
|
||||
params![i.capability_token, i.ticket_id, i.merge_request_id],
|
||||
|r| {
|
||||
Ok((
|
||||
@@ -1216,18 +1220,34 @@ impl MergeRequestStore {
|
||||
))
|
||||
})?;
|
||||
let review = request
|
||||
.effective_review(&merge.approved_source_ref)
|
||||
.filter(|review| review.event_id == merge.approval_event_id)
|
||||
.thread
|
||||
.iter()
|
||||
.find_map(|event| match event {
|
||||
MergeRequestThreadEvent::Review(review)
|
||||
if review.event_id == merge.approval_event_id
|
||||
&& review.subject_ref == merge.approved_source_ref =>
|
||||
{
|
||||
Some(review)
|
||||
}
|
||||
_ => None,
|
||||
})
|
||||
.filter(|review| {
|
||||
!request.thread.iter().any(|event| {
|
||||
matches!(
|
||||
event,
|
||||
MergeRequestThreadEvent::ReviewRevoked(revoked)
|
||||
if revoked.review_event_id == review.event_id
|
||||
)
|
||||
})
|
||||
})
|
||||
.ok_or_else(|| {
|
||||
MergeRequestError::NotReady(format!(
|
||||
"Merge Request `{merge_request_id}` approval is no longer effective"
|
||||
"Merge Request `{merge_request_id}` integration approval is missing or revoked"
|
||||
))
|
||||
})?;
|
||||
if review.decision != ReviewDecision::Approve
|
||||
|| review.ticket_item_revision != input.item_revision
|
||||
{
|
||||
if review.decision != ReviewDecision::Approve {
|
||||
return Err(MergeRequestError::NotReady(format!(
|
||||
"Merge Request `{merge_request_id}` lacks approval bound to the current Ticket revision"
|
||||
"Merge Request `{merge_request_id}` lacks an approved integration result"
|
||||
)));
|
||||
}
|
||||
merged_subjects.push(MergeRequestReviewSubject {
|
||||
|
||||
@@ -859,6 +859,92 @@ fn ticket_rescope_requires_fresh_review_before_merge_and_remains_recoverable() {
|
||||
.unwrap();
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn ticket_rescope_after_integration_accepts_fresh_requirement_attestation() {
|
||||
let (dir, store) = fixture();
|
||||
open(&store);
|
||||
let integration_approval = approve(&store, "subject", "token-integration");
|
||||
store
|
||||
.complete(CompleteMergeRequest {
|
||||
merge_request_id: "MR".into(),
|
||||
ticket_id: "T".into(),
|
||||
operation_id: "merge".into(),
|
||||
approval_event_id: integration_approval.event_id,
|
||||
current_subject_ref: "subject".into(),
|
||||
target_ref_before: "target-before".into(),
|
||||
target_ref_after: "target-after".into(),
|
||||
strategy: MergeStrategy::FastForward,
|
||||
resolution: ConflictResolution::None,
|
||||
auth: auth(),
|
||||
now: at(5),
|
||||
})
|
||||
.unwrap();
|
||||
let connection = Connection::open(dir.path().join("db")).unwrap();
|
||||
connection
|
||||
.execute(
|
||||
"INSERT INTO typed_ticket_events VALUES('W','T',1,'item_edit','user','t2',NULL,NULL,NULL,NULL)",
|
||||
[],
|
||||
)
|
||||
.unwrap();
|
||||
connection
|
||||
.execute(
|
||||
"INSERT INTO typed_ticket_event_attributes VALUES('W','T',1,'event_id','t2')",
|
||||
[],
|
||||
)
|
||||
.unwrap();
|
||||
drop(connection);
|
||||
|
||||
store
|
||||
.register_reviewer_child_session(RegisterReviewerChildSession {
|
||||
workspace_id: "W".into(),
|
||||
parent_runtime_id: "runtime".into(),
|
||||
parent_worker_id: "coder".into(),
|
||||
child_session_id: "child-post-merge".into(),
|
||||
reviewer_profile: "builtin:reviewer".into(),
|
||||
now: at(6),
|
||||
})
|
||||
.unwrap();
|
||||
store
|
||||
.request_review(RequestMergeRequestReview {
|
||||
merge_request_id: "MR".into(),
|
||||
ticket_id: "T".into(),
|
||||
ticket_item_revision: "t2".into(),
|
||||
ticket_merge_request_subjects: vec![MergeRequestReviewSubject {
|
||||
merge_request_id: "MR".into(),
|
||||
subject_ref: "subject".into(),
|
||||
}],
|
||||
subject_ref: "subject".into(),
|
||||
child_session_id: "child-post-merge".into(),
|
||||
capability_token: "token-post-merge".into(),
|
||||
auth: auth(),
|
||||
now: at(7),
|
||||
})
|
||||
.unwrap();
|
||||
let requirement_approval = store
|
||||
.submit_review(SubmitMergeRequestReview {
|
||||
merge_request_id: "MR".into(),
|
||||
ticket_id: "T".into(),
|
||||
current_subject_ref: "subject".into(),
|
||||
capability_token: "token-post-merge".into(),
|
||||
decision: ReviewDecision::Approve,
|
||||
body: "rescope requirements approved against merged result".into(),
|
||||
findings: vec![],
|
||||
now: at(8),
|
||||
})
|
||||
.unwrap();
|
||||
store
|
||||
.complete_ticket(CompleteTicket {
|
||||
ticket_id: "T".into(),
|
||||
operation_id: "ticket-complete-post-rescope".into(),
|
||||
item_revision: "t2".into(),
|
||||
merge_request_ids: vec!["MR".into()],
|
||||
requirement_approval_event_id: requirement_approval.event_id,
|
||||
auth: auth(),
|
||||
now: at(9),
|
||||
})
|
||||
.unwrap();
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn review_request_rejects_a_snapshot_that_omits_a_linked_merge_request() {
|
||||
let (_dir, store) = fixture();
|
||||
|
||||
@@ -12095,6 +12095,25 @@ fn merge_request_ticket_id(mr: &merge_request::MergeRequest) -> ApiResult<String
|
||||
}
|
||||
}
|
||||
|
||||
fn merged_result_source_ref(mr: &merge_request::MergeRequest) -> ApiResult<String> {
|
||||
mr.thread
|
||||
.iter()
|
||||
.rev()
|
||||
.find_map(|event| match event {
|
||||
merge_request::MergeRequestThreadEvent::Merge(event) => {
|
||||
Some(event.approved_source_ref.clone())
|
||||
}
|
||||
_ => None,
|
||||
})
|
||||
.ok_or_else(|| {
|
||||
Error::MergeRequest(merge_request::MergeRequestError::Corrupt(format!(
|
||||
"merged Merge Request `{}` has no MergeResult",
|
||||
mr.merge_request_id
|
||||
)))
|
||||
.into()
|
||||
})
|
||||
}
|
||||
|
||||
fn public_merge_request_state(
|
||||
state: merge_request::MergeRequestState,
|
||||
) -> server_api::MergeRequestState {
|
||||
@@ -12852,6 +12871,9 @@ async fn scoped_register_merge_request_review_capability(
|
||||
)
|
||||
.into());
|
||||
}
|
||||
let subject_ref = match mr.state {
|
||||
merge_request::MergeRequestState::Merged => merged_result_source_ref(&mr)?,
|
||||
merge_request::MergeRequestState::Open => {
|
||||
let selector = mr
|
||||
.selector_from
|
||||
.as_deref()
|
||||
@@ -12870,26 +12892,18 @@ async fn scoped_register_merge_request_review_capability(
|
||||
selector,
|
||||
&source_observation.revision_ref,
|
||||
)?;
|
||||
let subject_ref = source_observation.revision_ref;
|
||||
source_observation.revision_ref
|
||||
}
|
||||
merge_request::MergeRequestState::Closed => {
|
||||
return Err(
|
||||
Error::InvalidInput("closed Merge Request is not reviewable".into()).into(),
|
||||
);
|
||||
}
|
||||
};
|
||||
let mut ticket_merge_request_subjects = Vec::new();
|
||||
for linked in store.list_for_ticket(&workspace_id, &ticket_id)? {
|
||||
let linked_subject_ref = if linked.state == merge_request::MergeRequestState::Merged {
|
||||
linked
|
||||
.thread
|
||||
.iter()
|
||||
.rev()
|
||||
.find_map(|event| match event {
|
||||
merge_request::MergeRequestThreadEvent::Merge(event) => {
|
||||
Some(event.approved_source_ref.clone())
|
||||
}
|
||||
_ => None,
|
||||
})
|
||||
.ok_or_else(|| {
|
||||
Error::MergeRequest(merge_request::MergeRequestError::Corrupt(format!(
|
||||
"merged Merge Request `{}` has no MergeResult",
|
||||
linked.merge_request_id
|
||||
)))
|
||||
})?
|
||||
merged_result_source_ref(&linked)?
|
||||
} else {
|
||||
let linked_selector = linked.selector_from.as_deref().ok_or_else(|| {
|
||||
Error::InvalidInput(format!(
|
||||
@@ -12953,24 +12967,34 @@ async fn scoped_submit_merge_request_review(
|
||||
}
|
||||
let ticket_id = review_authorization.ticket_id;
|
||||
let mr = store.get_by_id(&workspace_id, &merge_request_id)?;
|
||||
let selector = mr
|
||||
.selector_from
|
||||
.as_deref()
|
||||
.ok_or_else(|| Error::InvalidInput("selector_from requires repair".into()))?;
|
||||
let assignment = api
|
||||
.store
|
||||
.get_current_ticket_coder_assignment(&workspace_id, &ticket_id)?
|
||||
.ok_or_else(|| {
|
||||
Error::TicketAssignmentConflict("Ticket has no current assigned Coder".into())
|
||||
})?;
|
||||
let current_subject_ref = observe_published_source_ref(
|
||||
let current_subject_ref = match mr.state {
|
||||
merge_request::MergeRequestState::Merged => merged_result_source_ref(&mr)?,
|
||||
merge_request::MergeRequestState::Open => {
|
||||
let selector = mr
|
||||
.selector_from
|
||||
.as_deref()
|
||||
.ok_or_else(|| Error::InvalidInput("selector_from requires repair".into()))?;
|
||||
observe_published_source_ref(
|
||||
&api,
|
||||
&workspace_id,
|
||||
&assignment.worker.runtime_id,
|
||||
&mr.repository_id,
|
||||
selector,
|
||||
)?
|
||||
.revision_ref;
|
||||
.revision_ref
|
||||
}
|
||||
merge_request::MergeRequestState::Closed => {
|
||||
return Err(
|
||||
Error::InvalidInput("closed Merge Request is not reviewable".into()).into(),
|
||||
);
|
||||
}
|
||||
};
|
||||
let decision = internal_review_decision(input.decision);
|
||||
let findings = input
|
||||
.findings
|
||||
|
||||
@@ -4,6 +4,6 @@ The Merge Request and its append-only thread are the routine authority for revie
|
||||
|
||||
An open Merge Request keeps one immutable `selector_from` and `selector_to`. Advance only the existing source selector with a normal non-force push; do not open a replacement Merge Request, invent an add-revision operation, or create a fresh integration branch for each fix or target movement. Before opening or requesting review, publish the exact source and verify that the provider resolves `selector_from` to local `HEAD`.
|
||||
|
||||
A review verdict is valid only for the exact provider-resolved source ref, selected Merge Request, and current Ticket item revision captured by `ReviewRequested`. Moving the source ref or substantively rescoping the Ticket requires a fresh review. Moving only the target ref does not invalidate approval for an unchanged source; it requires refreshed readiness/integration evidence against the current target. Target integration and `CompleteMergeRequest` are Orchestrator authority, not Coder or Reviewer authority. `CompleteMergeRequest` records only that selected repository result; it does not finish the Ticket or release its assignment. After every linked required result is integrated, `CompleteTicket` revalidates the exact linked-MR set, current Ticket revision, Reviewer approvals, state, and assignment before completing the Ticket.
|
||||
A review verdict is valid only for the exact provider-resolved source ref or immutable merged-result subject, selected Merge Request, complete linked MR/source-result snapshot, and current Ticket item revision captured by `ReviewRequested`. Moving a source ref or substantively rescoping the Ticket requires a fresh review. After integration, a fresh review may explicitly address a merged MR to attest updated Ticket requirements against the immutable merged-result snapshot; it does not revise the MergeResult. Moving only the target ref does not invalidate approval for an unchanged source; it requires refreshed readiness/integration evidence against the current target. Target integration and `CompleteMergeRequest` are Orchestrator authority, not Coder or Reviewer authority. `CompleteMergeRequest` records only that selected repository result; it does not finish the Ticket or release its assignment. After every linked required result is integrated, `CompleteTicket` revalidates the exact linked-MR set, current Ticket revision, explicitly identified Reviewer requirement approval, state, and assignment before completing the Ticket.
|
||||
|
||||
Current Ticket, Merge Request, provider refs, and thread evidence take precedence over stale Memory, old implementation reports, branch-name assumptions, or previous instructions that describe a revision-based workflow. Reread the Ticket and `ShowMergeRequest` before decisions. If source or target movement races with review or completion, stop and reread current authority rather than reusing stale evidence.
|
||||
|
||||
@@ -16,7 +16,7 @@ Push the resulting `merge_to` branch through its configured normal push path. Pr
|
||||
|
||||
After each repository push, verify the provider resolves `merge_to` exactly to `target_ref_after`, then call `CompleteMergeRequest` with that explicit MR ID, before/after evidence, authoritative approval event, and merge strategy. `CompleteMergeRequest` records and verifies only that already-applied repository integration; it does not update the branch itself, and it does not complete the Ticket. Partial integration is valid: keep the Ticket and assignment in progress while remaining linked MRs are implemented, reviewed, or integrated.
|
||||
|
||||
If a repository push succeeds but MR completion recording fails, do not push again or invent a new result. Retry the same `CompleteMergeRequest` operation and evidence. Once every linked required result is recorded, reread the latest Ticket item revision and exact linked MR/source-result set, identify the effective Reviewer approval that attests that exact snapshot and judges the requirements satisfied, then call `CompleteTicket` with the item revision, MR IDs, and that requirement approval event ID. `CompleteTicket` alone moves the Ticket to `done` and releases the assignment atomically; MR-set changes, rescope, review revocation, stale assignment/state, or an unmerged result must fail closed. Retry an already-recorded operation only with the same fingerprint.
|
||||
If a repository push succeeds but MR completion recording fails, do not push again or invent a new result. Retry the same `CompleteMergeRequest` operation and evidence. Once every linked required result is recorded, reread the latest Ticket item revision and exact linked MR/source-result set, identify the effective Reviewer approval that attests that exact snapshot and judges the requirements satisfied, then call `CompleteTicket` with the item revision, MR IDs, and that requirement approval event ID. If the Ticket was rescoped after partial or full integration, obtain a fresh Reviewer attestation against the immutable merged-result subjects through an explicitly addressed merged MR; this post-integration review updates requirement judgment without rewriting any MergeResult. `CompleteTicket` alone moves the Ticket to `done` and releases the assignment atomically; MR-set changes, rescope, review revocation, stale assignment/state, or an unmerged result must fail closed. Retry an already-recorded operation only with the same fingerprint.
|
||||
|
||||
Do not create or delegate an implementation worktree/branch until the Ticket records enough agreed intent, requirements, and acceptance criteria to bound the work.
|
||||
|
||||
|
||||
@@ -2,6 +2,6 @@ You are the Ticket Reviewer role running as an actual Runtime-owned direct child
|
||||
|
||||
Keep role behavior here and treat the first committed user message as bounded Ticket/Merge Request context only, never as a supplied verdict. Review the host-captured explicit Merge Request, `ReviewRequested.subject_ref`, `ticket_item_revision`, and exact linked Merge Request subject snapshot against Ticket intent, binding decisions/invariants, acceptance criteria, and project design boundaries. When a Ticket has multiple MRs, judge the selected MR in the context of that complete linked result set without reusing another MR's approval; an `approve` decision attests that the captured full snapshot satisfies the current Ticket requirements and reported validation, not merely that the selected diff is locally acceptable. Use the available Workdir inspection and command tools for focused validation, but do not intentionally modify implementation files, merge, close, update a repository ref, or take over implementation.
|
||||
|
||||
Your prose response is not review authority. Before finishing, call `ReviewMergeRequest` exactly once with `approve` or `request_changes`, a bounded evidence summary, and concrete structured findings. Capability authority and subject identity are injected by your child Workspace client and are not model inputs. The Server re-resolves `selector_from`; if it moved, submission records cancellation and fails rather than approving stale work. A verdict applies only to that captured source ref; target-only movement does not invalidate approval for an unchanged source, and target integration remains Orchestrator authority.
|
||||
Your prose response is not review authority. Before finishing, call `ReviewMergeRequest` exactly once with `approve` or `request_changes`, a bounded evidence summary, and concrete structured findings. Capability authority and subject identity are injected by your child Workspace client and are not model inputs. For an open MR, the Server re-resolves `selector_from`; if it moved, submission records cancellation and fails rather than approving stale work. For an explicitly addressed merged MR, the captured subject is the immutable MergeResult source and the review provides fresh requirement judgment after rescope without changing integration evidence. A verdict applies only to that captured subject; target-only movement does not invalidate approval for an unchanged source, and target integration remains Orchestrator authority.
|
||||
|
||||
Review more than the diff: verify the implementation satisfies the Ticket intent and acceptance criteria, remains coherent with the codebase design, and does not introduce unnecessary compatibility.
|
||||
|
||||
Reference in New Issue
Block a user