Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
132 changes: 132 additions & 0 deletions docs/architecture/review-lifecycle.md
Original file line number Diff line number Diff line change
Expand Up @@ -326,6 +326,130 @@ questions and real states without Skill names, agent ids, packet ids, or budgets
A child with no loaded transcript renders preparing, loading, or load-failed
state rather than a title over an empty body.

Focused-check titles cross one narrow public boundary:

```mermaid
flowchart LR
Assignment["Focused question"] --> Admission["Runtime admission"]
Admission --> Label["Public label"]
Label --> Card["Card"]
Label --> Detail["Detail"]
Assignment -. "internal fields" .-> Execution["Execution only"]
```

The label is short, plain-language metadata stored in the admitted child
manifest. Cards and detail tabs read only that label; they never derive titles
from the model prompt, capability key, path scope, or other launch arguments.
Missing or unsafe labels use a generic localized title and never block the
check. Linking projects only the admitted public label; the existing manifest
is persisted for recovery. Both representations come from the same admitted
assignment, without sending internal manifest fields through the UI event.
Session reconstruction re-admits that label through the same runtime function
before restored metadata is persisted.

### Execution detail and recovery projection

Execution outcome and transcript availability are separate facts. A Review can
still be running while history loading fails, and a completed Review can be
visible before its transcript is hydrated. Card and detail surfaces therefore
derive one presentation from existing session and Task facts instead of keeping
a second UI-owned lifecycle.

```mermaid
flowchart LR
Metadata["Session metadata"] --> Content["Content state"]
Live["Live session"] --> Status["Execution state"]
Turn["Last turn"] --> Status
Task["Task result"] --> Status
Content --> View["Review view"]
Status --> View
View --> Card["Card"]
View --> Detail["Detail"]
View --> Actions["Actions"]
```

Content state has four user-visible outcomes:

| Source fact | Detail body |
|---|---|
| new child with no turn yet | preparing |
| metadata only, or history hydration in progress | loading |
| hydrated transcript | transcript and result |
| history hydration failed | load failed with a load-only retry |

Reloading content never starts or continues model execution. A load failure is
not presented as a Review failure, and an empty transcript is never rendered as
an unexplained blank pane.

Execution state is derived in this order:

1. A live `Processing` session is running. Live state wins over the persisted
idle form while a runtime still owns the turn.
2. A structured Task result such as `partial_timeout` or `cancelled` preserves
the more precise parent-visible outcome. This matters because a child with
partial output may have a completed persisted turn even though the bounded
reviewer execution timed out.
3. The latest child turn supplies completed, error, or cancelled state.
4. A restored idle session with an unfinished latest turn is interrupted; it is
not running and is not complete.
5. Parent Task status is a compatibility fallback only when the linked child or
its persisted facts are unavailable.

These are projection rules, not a new persisted enum or a second state machine.
They produce the following plain-language behavior:

| Execution outcome | Presentation | Allowed action |
|---|---|---|
| active turn | running | stop |
| completed turn | completed | inspect result |
| timeout with output | timed out, partial result kept | inspect partial result |
| timeout without output | timed out | return to the owning Review |
| model or provider failure | could not complete | return to the owning Review |
| user-confirmed cancellation | stopped | inspect retained output |
| runtime lost with an unfinished turn | interrupted | return to the owning Review; continue there when available |
| incomplete legacy facts | unable to confirm | reload details; do not retry automatically |

An individual focused check never exposes a direct rerun action. The owning
Review remains responsible for bounded retries and coverage decisions, so the
UI cannot bypass its retry budget or duplicate work. Continuation is available
only for an interrupted, nonterminal Review at the Review-session level. It
reuses that session and appends a turn without creating a second logical launch.
Retry after a terminal timeout or failure remains an explicit new revision.
Opening details, restoring a window, or restarting the application must not
resubmit the original request.

Stopping has a confirmation boundary:

```mermaid
stateDiagram-v2
[*] --> Running
Running --> Stopping: user stops
Stopping --> Stopped: cancellation confirmed
Stopping --> Running: cancellation not confirmed
```

`Stopping` is transient UI intent. The UI settles the turn as stopped only after
the runtime accepts cancellation. If cancellation cannot be confirmed, it
reloads the authoritative state and says that stopping could not be confirmed;
it must not claim success or launch replacement work.

Application restore follows runtime ownership rather than the last visible card:

```mermaid
flowchart TD
Restore["Restore"] --> LiveOwner{"Runtime active?"}
LiveOwner -->|Yes| Running["Running"]
LiveOwner -->|No| LastTurn{"Turn finished?"}
LastTurn -->|Yes| Terminal["Saved outcome"]
LastTurn -->|No| Interrupted["Interrupted"]
LastTurn -->|Unknown| Unknown["Unable to confirm"]
```

Persisted processing state is not revived after an application restart. If a
remote or still-running host remains authoritative, its live state is shown;
otherwise an unfinished turn is interrupted and waits for explicit user intent.
Partial transcript content remains inspectable in either case.

The pull-request surface continues to use exact provider identity and verified
base/head freshness. A stale record offers “Review current version” and creates
another revision of that record. Cached pull-request overview data is not
Expand Down Expand Up @@ -403,6 +527,14 @@ evidence:

- launch does not force-open execution detail;
- no Review state renders as an unexplained blank pane;
- execution status and transcript-loading status remain independent;
- opening or reloading detail performs no model call and creates no child turn;
- a partial timeout keeps partial output and remains visibly distinct from a
successful completion;
- user cancellation is shown as stopped only after cancellation is confirmed;
- application restart never silently revives or duplicates an unfinished turn;
- an interrupted Review can continue only through explicit Review-level intent;
- an individual focused check cannot bypass the owning Review's retry policy;
- metadata-only restore shows a useful bounded summary;
- stale pull-request revisions cannot be presented as current;
- re-review preserves one record and creates a distinct revision;
Expand Down
44 changes: 40 additions & 4 deletions src/apps/desktop/src/api/agentic_api.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ use crate::runtime::{
DesktopRuntimeContext, DesktopSessionApplicationError, DesktopSessionScopeRequest,
};
use crate::startup_trace::DesktopStartupTrace;
use bitfun_agent_runtime::deep_review::sanitize_focused_review_public_metadata;
use bitfun_agent_runtime::sdk::{
AgentDialogTurnRequest, AgentInputAttachment, AgentSessionModelUpdateRequest,
AgentSubmissionSource, AgentTurnCancellationRequest, PermissionAuditRecord, PermissionGrant,
Expand Down Expand Up @@ -685,6 +686,19 @@ pub struct CancelSessionRequest {
pub session_id: String,
}

fn sanitize_create_session_review_metadata(request: &mut CreateSessionRequest) {
if let Some(manifest) = request.deep_review_run_manifest.as_mut() {
sanitize_focused_review_public_metadata(manifest);
}
}

#[derive(Debug, Serialize)]
#[serde(rename_all = "camelCase")]
pub struct CancelSessionResponse {
pub cancelled: bool,
pub dialog_turn_id: Option<String>,
}

#[derive(Debug, Deserialize)]
#[serde(rename_all = "camelCase")]
pub struct CancelToolRequest {
Expand Down Expand Up @@ -1184,11 +1198,12 @@ pub struct GenerateSessionTitleRequest {
pub async fn create_session(
coordinator: State<'_, Arc<ConversationCoordinator>>,
app_state: State<'_, AppState>,
request: CreateSessionRequest,
mut request: CreateSessionRequest,
) -> Result<CreateSessionResponse, String> {
fn norm_conn(s: Option<String>) -> Option<String> {
s.map(|x| x.trim().to_string()).filter(|x| !x.is_empty())
}
sanitize_create_session_review_metadata(&mut request);
let wp = request.workspace_path.clone();
let remote_conn = norm_conn(request.remote_connection_id.clone()).or_else(|| {
request
Expand Down Expand Up @@ -2159,8 +2174,8 @@ pub async fn control_deep_review_queue(
pub async fn cancel_session(
coordinator: State<'_, Arc<ConversationCoordinator>>,
request: CancelSessionRequest,
) -> Result<(), String> {
coordinator
) -> Result<CancelSessionResponse, String> {
let dialog_turn_id = coordinator
.cancel_active_turn_for_session(&request.session_id, std::time::Duration::from_secs(5))
.await
.map_err(|e| {
Expand All @@ -2172,7 +2187,10 @@ pub async fn cancel_session(
format!("Failed to cancel session: {}", e)
})?;

Ok(())
Ok(CancelSessionResponse {
cancelled: dialog_turn_id.is_some(),
dialog_turn_id,
})
}

#[derive(Debug, Deserialize)]
Expand Down Expand Up @@ -3044,6 +3062,24 @@ mod tests {
}
}

#[test]
fn create_session_recovery_sanitizes_focused_review_public_metadata() {
let mut request = idempotent_create_request();
request.deep_review_run_manifest = Some(json!({
"reviewMode": "deep",
"focusedAssignment": {
"displayLabel": "Review Worker packet 7",
"question": "Could this contract break callers?"
}
}));

sanitize_create_session_review_metadata(&mut request);

let assignment = &request.deep_review_run_manifest.as_ref().unwrap()["focusedAssignment"];
assert!(assignment.get("displayLabel").is_none());
assert_eq!(assignment["question"], "Could this contract break callers?");
}

#[test]
fn existing_create_session_retry_returns_the_matching_session() {
let request = idempotent_create_request();
Expand Down
7 changes: 5 additions & 2 deletions src/apps/server/src/rpc_dispatcher.rs
Original file line number Diff line number Diff line change
Expand Up @@ -474,12 +474,15 @@ pub async fn dispatch(
"cancel_session" => {
let request = extract_request(&params)?;
let session_id = get_string(&request, "sessionId")?;
state
let dialog_turn_id = state
.coordinator
.cancel_active_turn_for_session(&session_id, Duration::from_secs(5))
.await
.map_err(|e| anyhow!("{}", e))?;
Ok(serde_json::Value::Null)
Ok(serde_json::json!({
"cancelled": dialog_turn_id.is_some(),
"dialogTurnId": dialog_turn_id,
}))
}
"get_session_messages" => {
let request = params.get("request").unwrap_or(&params);
Expand Down
25 changes: 25 additions & 0 deletions src/crates/assembly/core/src/agentic/coordination/coordinator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ use crate::service::workspace::{
};
use crate::service_agent_runtime::CoreServiceAgentRuntime;
use crate::util::errors::{BitFunError, BitFunResult};
use bitfun_agent_runtime::deep_review::FocusedReviewAssignment;
use bitfun_agent_runtime::output_surface::{
supports_inline_markdown_images_for_source, TOOL_CONTEXT_INLINE_MARKDOWN_IMAGE_DISPLAY_KEY,
};
Expand Down Expand Up @@ -5432,6 +5433,17 @@ Update the persona files and delete BOOTSTRAP.md as soon as bootstrap is complet
external_generation_lease: _external_generation_lease,
} = request;
let prepared_target_session_id = target_session_id.clone();
let deep_review_run_manifest = context
.get("deep_review_run_manifest")
.and_then(|raw| serde_json::from_str::<serde_json::Value>(raw).ok());
let focused_review_display_label = deep_review_run_manifest
.as_ref()
.and_then(|manifest| {
FocusedReviewAssignment::from_manifest(manifest)
.ok()
.flatten()
})
.and_then(|assignment| assignment.display_label().map(str::to_string));
let continuation_policy = session_config.continuation_policy;

let requested_timeout_seconds = timeout_seconds.filter(|seconds| *seconds > 0);
Expand Down Expand Up @@ -5649,6 +5661,18 @@ Update the persona files and delete BOOTSTRAP.md as soon as bootstrap is complet
.await;
return Err(error);
}
if let Some(manifest) = deep_review_run_manifest.as_ref() {
if let Err(error) = self
.session_manager
.set_session_deep_review_run_manifest(&session_id, Some(manifest.clone()))
.await
{
warn!(
"Failed to persist Review manifest for linked subagent session: session_id={}, error={}",
session_id, error
);
}
}
if let Some(source_session_id) = prompt_cache_source_session_id.as_deref() {
self.session_manager
.seed_forked_edit_constraints(source_session_id, &session_id)
Expand Down Expand Up @@ -5761,6 +5785,7 @@ Update the persona files and delete BOOTSTRAP.md as soon as bootstrap is complet
.session_manager
.get_session(&session_id)
.and_then(|session| session.config.model_id.clone()),
focused_review_display_label: focused_review_display_label.clone(),
})
.await;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,8 +65,13 @@ impl LaunchReviewAgentTool {
},
"focused_assignment": {
"type": "object",
"description": "A target-bound question for ReviewWorker. Required for adaptive non-packet checks; managed packets may attach a question without repeating their packet file scope.",
"description": "A target-bound question for ReviewWorker. Required for adaptive non-packet checks; managed packets may attach a question without repeating their packet file scope. A safe display_label may be shown after runtime validation.",
"properties": {
"display_label": {
"type": "string",
"maxLength": 80,
"description": "Optional short plain-language label for the concern, using at most eight words. Do not include internal coordination values such as agent or skill names, packet IDs, file paths, or model IDs. Unsafe labels are ignored and never block the check."
},
"question": { "type": "string" },
"independent_value": { "type": "string" },
"target_fingerprint": { "type": "string" },
Expand Down Expand Up @@ -210,7 +215,7 @@ Built-in review agent types:
- `ReviewWorker`: one read-only worker whose bounded prompt supplies the dynamic review lens, concrete question, file or packet scope, and expected evidence. It may cover a narrow specialist uncertainty or a managed file packet, but must not widen its assignment.
- `ReviewJudge`: final quality-inspector pass after reviewer outputs are available.

The capability catalog below contains short descriptions only. For an adaptive ReviewWorker call, copy the selected key and fingerprint into `focused_assignment`; full guidance is loaded only after runtime admission. Outside a manifest-declared work-packet plan, do not split files, launch routine parallel coverage, or repeat the primary review.
The capability catalog below contains short descriptions only. For an adaptive ReviewWorker call, copy the selected key and fingerprint into `focused_assignment`; full guidance is loaded only after runtime admission. When possible, give each focused assignment a short plain-language `display_label` that describes the user-visible concern without internal coordination values such as agent or skill names, packet IDs, file paths, or model IDs. The runtime ignores an unsafe label without blocking the check. Outside a manifest-declared work-packet plan, do not split files, launch routine parallel coverage, or repeat the primary review.

For a managed packet, pass its exact manifest `packet_id` in the top-level `packet_id` field. Runtime rejects missing or unknown managed packet ids.

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -285,6 +285,21 @@ async fn launch_review_agent_schema_exposes_retry_without_agent_or_fork_controls
assert_eq!(schema["properties"]["retry_coverage"]["type"], "object");
assert_eq!(schema["properties"]["packet_id"]["type"], "string");
assert_eq!(schema["properties"]["focused_assignment"]["type"], "object");
assert_eq!(
schema["properties"]["focused_assignment"]["properties"]["display_label"]["type"],
"string"
);
let display_label_description = schema["properties"]["focused_assignment"]["properties"]
["display_label"]["description"]
.as_str()
.expect("display_label description should be a string");
assert!(display_label_description.contains("file paths"));
assert!(!display_label_description.contains("packet, path, model"));
assert!(!schema["properties"]["focused_assignment"]["required"]
.as_array()
.unwrap()
.iter()
.any(|value| value.as_str() == Some("display_label")));
assert!(schema["properties"].get("fork_context").is_none());
assert!(schema["properties"].get("agent_id").is_none());
assert!(schema["properties"].get("run_in_background").is_none());
Expand Down
8 changes: 8 additions & 0 deletions src/crates/contracts/events/src/agentic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,9 @@ pub enum AgenticEvent {
/// Resolved model selector stored on the child session.
#[serde(skip_serializing_if = "Option::is_none")]
model_id: Option<String>,
/// Runtime-admitted public label for a focused Review child.
#[serde(skip_serializing_if = "Option::is_none")]
focused_review_display_label: Option<String>,
},

DialogTurnCompleted {
Expand Down Expand Up @@ -930,6 +933,7 @@ mod tests {
parent_tool_call_id: "tool-1".to_string(),
agent_type: Some("GeneralPurpose".to_string()),
model_id: Some("fast".to_string()),
focused_review_display_label: Some("Authentication boundary".to_string()),
};

assert_eq!(event.session_id(), Some("child-session"));
Expand All @@ -944,5 +948,9 @@ mod tests {
assert_eq!(serialized["parent_tool_call_id"], "tool-1");
assert_eq!(serialized["agent_type"], "GeneralPurpose");
assert_eq!(serialized["model_id"], "fast");
assert_eq!(
serialized["focused_review_display_label"],
"Authentication boundary"
);
}
}
Loading