mirror of
https://github.com/fabro-sh/fabro.git
synced 2026-08-28 05:27:41 +00:00
Add structured review targets to human gates
This commit is contained in:
parent
8cc711463b
commit
010c8d50c1
42 changed files with 974 additions and 22 deletions
1
Cargo.lock
generated
1
Cargo.lock
generated
|
|
@ -3258,6 +3258,7 @@ dependencies = [
|
|||
"shlex",
|
||||
"strum 0.28.0",
|
||||
"tempfile",
|
||||
"thiserror 2.0.18",
|
||||
"toml 0.8.23",
|
||||
"ulid",
|
||||
"url",
|
||||
|
|
|
|||
|
|
@ -4,6 +4,7 @@ import { SWRConfig } from "swr";
|
|||
import {
|
||||
type ApiQuestion,
|
||||
QuestionType,
|
||||
ReviewTargetKind,
|
||||
} from "@qltysh/fabro-api-client";
|
||||
|
||||
import { InterviewDock } from "./interview-dock";
|
||||
|
|
@ -74,6 +75,58 @@ describe("InterviewDock", () => {
|
|||
expect(text).toContain("Awaiting input");
|
||||
});
|
||||
|
||||
test("renders the review target as the link in the question", () => {
|
||||
const url =
|
||||
"https://quarry.lithos.computer/tmp/0123456789abcdef0123456789abcdef";
|
||||
const tree = render(
|
||||
<InterviewDock
|
||||
runId="run-1"
|
||||
questions={[
|
||||
makeQuestion({
|
||||
text: "Review the Quarry review exercise document, then choose the next action.",
|
||||
review_target: {
|
||||
label: "Quarry review exercise",
|
||||
url,
|
||||
kind: ReviewTargetKind.DOCUMENT,
|
||||
},
|
||||
}),
|
||||
]}
|
||||
/>,
|
||||
);
|
||||
|
||||
expect(textContent(tree.toJSON())).toContain(
|
||||
"Review the Quarry review exercise document, then choose the next action.",
|
||||
);
|
||||
const links = tree.root.findAllByType("a");
|
||||
expect(links).toHaveLength(1);
|
||||
expect(links[0].props.href).toBe(url);
|
||||
expect(links[0].props.target).toBe("_blank");
|
||||
expect(links[0].props.rel).toBe("noopener noreferrer");
|
||||
expect(links[0].props.referrerPolicy).toBe("no-referrer");
|
||||
});
|
||||
|
||||
test("does not link an unsafe review target received from the API", () => {
|
||||
const fallback = "Review the document, then choose the next action.";
|
||||
const tree = render(
|
||||
<InterviewDock
|
||||
runId="run-1"
|
||||
questions={[
|
||||
makeQuestion({
|
||||
text: fallback,
|
||||
review_target: {
|
||||
label: "Unsafe target",
|
||||
url: "javascript:alert(1)",
|
||||
kind: ReviewTargetKind.DOCUMENT,
|
||||
},
|
||||
}),
|
||||
]}
|
||||
/>,
|
||||
);
|
||||
|
||||
expect(textContent(tree.toJSON())).toContain(fallback);
|
||||
expect(tree.root.findAllByType("a")).toHaveLength(0);
|
||||
});
|
||||
|
||||
test("yes/no question shows two buttons", () => {
|
||||
const tree = render(
|
||||
<InterviewDock runId="run-1" questions={[makeQuestion()]} />,
|
||||
|
|
|
|||
|
|
@ -22,6 +22,7 @@ import {
|
|||
} from "../lib/mutations";
|
||||
import { ApiError } from "../lib/api-client";
|
||||
import { displayLabel } from "./interview-label";
|
||||
import { ReviewTargetQuestion } from "./review-target-question";
|
||||
import { ErrorMessage } from "./ui";
|
||||
|
||||
const PRIMARY_BUTTON =
|
||||
|
|
@ -99,9 +100,11 @@ function InterviewQuestionDock({
|
|||
/>
|
||||
<div className="space-y-5 px-5 py-4 sm:px-6">
|
||||
<div>
|
||||
<p className="text-pretty text-base/6 font-medium text-fg">
|
||||
{question.text}
|
||||
</p>
|
||||
<ReviewTargetQuestion
|
||||
reviewTarget={question.review_target}
|
||||
fallbackText={question.text}
|
||||
className="text-pretty text-base/6 font-medium text-fg"
|
||||
/>
|
||||
<p className="mt-1 text-xs/5 text-fg-muted">
|
||||
{questionTypeLabel(question.question_type)}
|
||||
</p>
|
||||
|
|
|
|||
76
apps/fabro-web/app/components/review-target-question.tsx
Normal file
76
apps/fabro-web/app/components/review-target-question.tsx
Normal file
|
|
@ -0,0 +1,76 @@
|
|||
import { ArrowTopRightOnSquareIcon } from "@heroicons/react/20/solid";
|
||||
import {
|
||||
ReviewTargetKind,
|
||||
type ReviewTarget,
|
||||
} from "@qltysh/fabro-api-client";
|
||||
|
||||
const CONTROL_CHARACTER = /[\u0000-\u001f\u007f-\u009f]/u;
|
||||
const UNSAFE_LINK_DELIMITER = /[<>|]/u;
|
||||
|
||||
function unicodeScalarCount(value: string): number {
|
||||
return Array.from(value).length;
|
||||
}
|
||||
|
||||
function hasSafeReviewTarget(target: ReviewTarget | null | undefined): target is ReviewTarget {
|
||||
if (
|
||||
!target ||
|
||||
target.kind !== ReviewTargetKind.DOCUMENT ||
|
||||
!target.label.trim() ||
|
||||
target.label !== target.label.trim() ||
|
||||
unicodeScalarCount(target.label) > 200 ||
|
||||
CONTROL_CHARACTER.test(target.label) ||
|
||||
!target.url ||
|
||||
target.url !== target.url.trim() ||
|
||||
unicodeScalarCount(target.url) > 2048 ||
|
||||
CONTROL_CHARACTER.test(target.url) ||
|
||||
UNSAFE_LINK_DELIMITER.test(target.url)
|
||||
) {
|
||||
return false;
|
||||
}
|
||||
|
||||
try {
|
||||
const parsed = new URL(target.url);
|
||||
return (
|
||||
(parsed.protocol === "http:" || parsed.protocol === "https:") &&
|
||||
Boolean(parsed.host) &&
|
||||
!parsed.username &&
|
||||
!parsed.password
|
||||
);
|
||||
} catch {
|
||||
return false;
|
||||
}
|
||||
}
|
||||
|
||||
export function ReviewTargetQuestion({
|
||||
reviewTarget,
|
||||
fallbackText,
|
||||
className,
|
||||
}: {
|
||||
reviewTarget: ReviewTarget | null | undefined;
|
||||
fallbackText: string;
|
||||
className?: string;
|
||||
}) {
|
||||
if (!hasSafeReviewTarget(reviewTarget)) {
|
||||
return <p className={className}>{fallbackText}</p>;
|
||||
}
|
||||
|
||||
return (
|
||||
<p className={className}>
|
||||
Review the{" "}
|
||||
<a
|
||||
href={reviewTarget.url}
|
||||
target="_blank"
|
||||
rel="noopener noreferrer"
|
||||
referrerPolicy="no-referrer"
|
||||
className="inline-flex items-baseline gap-1 font-semibold text-teal-300 underline decoration-teal-500/50 underline-offset-2 transition-colors hover:text-fg focus-visible:rounded-sm focus-visible:outline-2 focus-visible:outline-offset-2 focus-visible:outline-teal-500"
|
||||
>
|
||||
<span>{reviewTarget.label}</span>
|
||||
<ArrowTopRightOnSquareIcon
|
||||
className="size-3 shrink-0 self-center"
|
||||
aria-hidden="true"
|
||||
/>
|
||||
</a>{" "}
|
||||
document, then choose the next action.
|
||||
</p>
|
||||
);
|
||||
}
|
||||
|
|
@ -98,6 +98,33 @@ describe("parseHumanInterviewPairs", () => {
|
|||
});
|
||||
});
|
||||
|
||||
test("preserves a typed review target from started events", () => {
|
||||
const events: EventEnvelope[] = [
|
||||
makeEventEnvelope(1, {
|
||||
event: "interview.started",
|
||||
properties: {
|
||||
question_id: "q-1",
|
||||
question:
|
||||
"Review the Quarry review exercise document, then choose the next action.",
|
||||
question_type: "multiple_choice",
|
||||
review_target: {
|
||||
label: "Quarry review exercise",
|
||||
url: "https://quarry.lithos.computer/tmp/0123456789abcdef0123456789abcdef",
|
||||
kind: "document",
|
||||
},
|
||||
},
|
||||
}),
|
||||
];
|
||||
|
||||
const pairs = parseHumanInterviewPairs(events);
|
||||
|
||||
expect(pairs[0].question.reviewTarget).toEqual({
|
||||
label: "Quarry review exercise",
|
||||
url: "https://quarry.lithos.computer/tmp/0123456789abcdef0123456789abcdef",
|
||||
kind: "document",
|
||||
});
|
||||
});
|
||||
|
||||
test("captures timeout and interrupted resolutions", () => {
|
||||
const events: EventEnvelope[] = [
|
||||
makeEventEnvelope(1, {
|
||||
|
|
|
|||
|
|
@ -1,5 +1,8 @@
|
|||
import { StageOutcome } from "@qltysh/fabro-api-client";
|
||||
import type { EventEnvelope } from "@qltysh/fabro-api-client";
|
||||
import type {
|
||||
EventEnvelope,
|
||||
ReviewTarget,
|
||||
} from "@qltysh/fabro-api-client";
|
||||
|
||||
import { getArray, getNumber, getObject, getString, type UnknownRecord } from "../../lib/unknown";
|
||||
|
||||
|
|
@ -25,6 +28,7 @@ export interface HumanQuestion {
|
|||
allowFreeform: boolean;
|
||||
timeoutSeconds: number | null;
|
||||
contextDisplay: string | null;
|
||||
reviewTarget: ReviewTarget | null;
|
||||
}
|
||||
|
||||
export type HumanResolution =
|
||||
|
|
@ -75,6 +79,16 @@ function parseInterviewOptions(value: unknown): InterviewOption[] {
|
|||
return out;
|
||||
}
|
||||
|
||||
function parseReviewTarget(value: unknown): ReviewTarget | null {
|
||||
if (!value || typeof value !== "object") return null;
|
||||
const record = value as UnknownRecord;
|
||||
const label = getString(record, "label");
|
||||
const url = getString(record, "url");
|
||||
const kind = getString(record, "kind");
|
||||
if (!label || !url || kind !== "document") return null;
|
||||
return { label, url, kind };
|
||||
}
|
||||
|
||||
/**
|
||||
* Pair `interview.started` events with the matching `interview.completed`,
|
||||
* `.timeout`, or `.interrupted` resolution by `question_id`. Unanswered
|
||||
|
|
@ -98,6 +112,7 @@ export function parseHumanInterviewPairs(events: EventEnvelope[]): HumanIntervie
|
|||
allowFreeform: props.allow_freeform === true,
|
||||
timeoutSeconds: getNumber(props, "timeout_seconds") ?? null,
|
||||
contextDisplay: getString(props, "context_display") ?? null,
|
||||
reviewTarget: parseReviewTarget(props.review_target),
|
||||
},
|
||||
resolution: null,
|
||||
});
|
||||
|
|
|
|||
|
|
@ -10,6 +10,7 @@ import {
|
|||
import type { EventEnvelope } from "@qltysh/fabro-api-client";
|
||||
|
||||
import type { Stage } from "../stage-sidebar";
|
||||
import { ReviewTargetQuestion } from "../review-target-question";
|
||||
import { Tooltip } from "../ui";
|
||||
import { formatAbsoluteTs, formatDurationMs } from "../../lib/format";
|
||||
import { ACTIVE_STAGE_STATES } from "../../lib/stage-sidebar";
|
||||
|
|
@ -168,7 +169,15 @@ function QuestionBlock({
|
|||
</header>
|
||||
|
||||
<div className="rounded-lg bg-panel p-4 outline-1 -outline-offset-1 outline-line">
|
||||
<Markdown content={question.question} />
|
||||
{question.reviewTarget ? (
|
||||
<ReviewTargetQuestion
|
||||
reviewTarget={question.reviewTarget}
|
||||
fallbackText={question.question}
|
||||
className="text-sm/6 text-fg-2"
|
||||
/>
|
||||
) : (
|
||||
<Markdown content={question.question} />
|
||||
)}
|
||||
{question.contextDisplay && (
|
||||
<div className="mt-3 border-t border-line pt-3 text-xs text-fg-muted">
|
||||
<Markdown content={question.contextDisplay} />
|
||||
|
|
|
|||
|
|
@ -9597,6 +9597,11 @@ components:
|
|||
type: ["string", "null"]
|
||||
description: Optional contextual text shown alongside the question.
|
||||
example: Latest draft
|
||||
review_target:
|
||||
description: Optional validated external resource that is the primary subject of this review question.
|
||||
oneOf:
|
||||
- $ref: "#/components/schemas/ReviewTarget"
|
||||
- type: "null"
|
||||
|
||||
QuestionType:
|
||||
description: The interaction type of a human-in-the-loop question.
|
||||
|
|
@ -11102,6 +11107,36 @@ components:
|
|||
type: ["string", "null"]
|
||||
description: Optional untrusted model-authored option preview captured for clients.
|
||||
|
||||
ReviewTargetKind:
|
||||
description: The type of resource presented for human review.
|
||||
type: string
|
||||
enum:
|
||||
- document
|
||||
|
||||
ReviewTarget:
|
||||
description: A validated external resource presented as the primary subject of a human review question.
|
||||
type: object
|
||||
required:
|
||||
- label
|
||||
- url
|
||||
- kind
|
||||
properties:
|
||||
label:
|
||||
type: string
|
||||
minLength: 1
|
||||
maxLength: 200
|
||||
description: Human-readable link label.
|
||||
example: Quarry review exercise
|
||||
url:
|
||||
type: string
|
||||
format: uri
|
||||
minLength: 1
|
||||
maxLength: 2048
|
||||
description: Absolute HTTP or HTTPS URL opened by the reviewer.
|
||||
example: https://quarry.lithos.computer/tmp/0123456789abcdef0123456789abcdef
|
||||
kind:
|
||||
$ref: "#/components/schemas/ReviewTargetKind"
|
||||
|
||||
InterviewQuestionRecord:
|
||||
description: Storage shape of an interview question recorded in the event log.
|
||||
type: object
|
||||
|
|
@ -11131,6 +11166,10 @@ components:
|
|||
format: double
|
||||
context_display:
|
||||
type: ["string", "null"]
|
||||
review_target:
|
||||
oneOf:
|
||||
- $ref: "#/components/schemas/ReviewTarget"
|
||||
- type: "null"
|
||||
|
||||
PendingInterviewRecord:
|
||||
description: Pending interview question plus the time it entered the unresolved set.
|
||||
|
|
|
|||
|
|
@ -40,6 +40,11 @@ Each handler type writes specific keys into the context after execution:
|
|||
|
||||
Agents can also emit arbitrary context updates by including a JSON object with a `context_updates` field in their response. See [Transitions](/workflows/transitions#agent-transitions).
|
||||
|
||||
The `review_target` key has an optional typed convention for human review
|
||||
workflows. A human gate with `review_target=true` reads this exact flat key and
|
||||
presents its document URL as the primary question link. See
|
||||
[Review targets](/workflows/human-in-the-loop#review-targets).
|
||||
|
||||
### Command nodes
|
||||
|
||||
| Key | Value |
|
||||
|
|
|
|||
|
|
@ -267,6 +267,7 @@ For the first node in each branch, `fidelity` resolves from the fork-to-branch e
|
|||
| Attribute | Type | Description |
|
||||
|---|---|---|
|
||||
| `question_type` | String | Optional interview question type override: `yes_no`, `confirmation`, `multiple_choice`, `multi_select`, or `freeform`. Defaults to `freeform` when the gate only has a freeform edge; otherwise defaults to `multiple_choice`. |
|
||||
| `review_target` | Boolean | When `true`, read and validate the typed `review_target` context value, then present it as the primary link in the question. See [Review targets](/workflows/human-in-the-loop#review-targets). |
|
||||
| `human.default_choice` | String | Target node to use when the question times out. |
|
||||
|
||||
### Manager loop (sub-workflow) nodes
|
||||
|
|
|
|||
|
|
@ -60,6 +60,65 @@ confirm -> exit [label="[N] No"]
|
|||
|
||||
Supported values are `yes_no`, `confirmation`, `multiple_choice`, `multi_select`, and `freeform`.
|
||||
|
||||
### Review targets
|
||||
|
||||
A human gate can present one external document as the primary review link. Set
|
||||
`review_target=true` on the gate:
|
||||
|
||||
```dot
|
||||
review [
|
||||
shape=hexagon,
|
||||
review_target=true
|
||||
]
|
||||
|
||||
review -> sync [label="[S] Review complete; sync the current Markdown"]
|
||||
review -> address [label="[A] Ask the agent to address human feedback"]
|
||||
review -> review [label="[C] Continue reviewing"]
|
||||
```
|
||||
|
||||
Before the workflow reaches the gate, an agent, prompt, or command node must set
|
||||
the flat `review_target` context key. A routing response can do this directly:
|
||||
|
||||
```json
|
||||
{
|
||||
"outcome": "succeeded",
|
||||
"context_updates": {
|
||||
"review_target": {
|
||||
"label": "Quarry review exercise",
|
||||
"url": "https://quarry.lithos.computer/tmp/0123456789abcdef0123456789abcdef",
|
||||
"kind": "document"
|
||||
}
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
Fabro then presents this question:
|
||||
|
||||
> Review the [Quarry review exercise](https://quarry.lithos.computer/tmp/0123456789abcdef0123456789abcdef) document, then choose the next action.
|
||||
|
||||
The link uses the target URL from context. The example uses a placeholder
|
||||
secret, not a live Quarry document.
|
||||
|
||||
The target object has three required fields:
|
||||
|
||||
| Field | Meaning |
|
||||
|---|---|
|
||||
| `label` | The link text. It must contain 1 to 200 characters and no control characters. |
|
||||
| `url` | An absolute HTTP or HTTPS URL. It must contain a host, must not contain URL credentials, and must be at most 2048 characters. |
|
||||
| `kind` | The resource type. The supported value is `document`. |
|
||||
|
||||
Fabro validates the target before it starts the interview. A missing or invalid
|
||||
target fails the gate deterministically. A gate without `review_target=true`
|
||||
ignores this context key and keeps its normal label.
|
||||
|
||||
The context value remains available after the human answers. A feedback loop
|
||||
can return to the same gate without recreating the target. A later stage can
|
||||
replace the target by writing a new value to the same context key.
|
||||
|
||||
Fabro does not fetch the URL. Web and Slack clients open it as an external link.
|
||||
Treat bearer-capability links as secrets and only provide them to people who
|
||||
can access the run.
|
||||
|
||||
### Default choice on timeout
|
||||
|
||||
If a human gate has a timeout configured, you can specify a default choice using the `human.default_choice` attribute:
|
||||
|
|
|
|||
|
|
@ -438,6 +438,7 @@ fn api_question_to_question(question: &types::ApiQuestion) -> Question {
|
|||
converted
|
||||
.context_display
|
||||
.clone_from(&question.context_display);
|
||||
converted.review_target.clone_from(&question.review_target);
|
||||
converted
|
||||
}
|
||||
|
||||
|
|
@ -451,6 +452,14 @@ async fn ask_attach_question(question: Question, styles: &'static Styles) -> Ans
|
|||
let rendered = styles.render_markdown(context_text);
|
||||
eprint!("{rendered}");
|
||||
}
|
||||
if let Some(target) = &question.review_target {
|
||||
eprintln!(
|
||||
"Review {}: {} — {}",
|
||||
target.kind().noun(),
|
||||
target.label(),
|
||||
target.url()
|
||||
);
|
||||
}
|
||||
eprintln!("{} {}", styles.bold_cyan.apply_to("?"), question.text);
|
||||
|
||||
match question.question_type {
|
||||
|
|
|
|||
|
|
@ -1361,6 +1361,7 @@ mod tests {
|
|||
allow_freeform: false,
|
||||
timeout_seconds: None,
|
||||
context_display: None,
|
||||
review_target: None,
|
||||
})),
|
||||
Some(WorkerTitlePhase::Waiting)
|
||||
);
|
||||
|
|
|
|||
|
|
@ -1752,6 +1752,7 @@ mod runs {
|
|||
allow_freeform: false,
|
||||
timeout_seconds: None,
|
||||
context_display: None,
|
||||
review_target: None,
|
||||
},
|
||||
ApiQuestion {
|
||||
id: "q-002".into(),
|
||||
|
|
@ -1775,6 +1776,7 @@ mod runs {
|
|||
allow_freeform: true,
|
||||
timeout_seconds: None,
|
||||
context_display: None,
|
||||
review_target: None,
|
||||
},
|
||||
]
|
||||
}
|
||||
|
|
|
|||
|
|
@ -719,6 +719,7 @@ impl SlackService {
|
|||
allow_freeform: props.allow_freeform,
|
||||
timeout_seconds: props.timeout_seconds,
|
||||
context_display: props.context_display.clone(),
|
||||
review_target: props.review_target.clone(),
|
||||
});
|
||||
let blocks = slack_blocks::question_to_blocks(
|
||||
&event.run_id.to_string(),
|
||||
|
|
@ -3708,6 +3709,7 @@ fn runtime_question_from_interview_record(question: &InterviewQuestionRecord) ->
|
|||
stage: question.stage.clone(),
|
||||
metadata: HashMap::new(),
|
||||
context_display: question.context_display.clone(),
|
||||
review_target: question.review_target.clone(),
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -3721,6 +3723,7 @@ fn api_question_from_interview_record(question: &InterviewQuestionRecord) -> Api
|
|||
allow_freeform: question.allow_freeform,
|
||||
timeout_seconds: question.timeout_seconds,
|
||||
context_display: question.context_display.clone(),
|
||||
review_target: question.review_target.clone(),
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -4765,6 +4765,7 @@ channel = "#deploys"
|
|||
allow_freeform: true,
|
||||
timeout_seconds: None,
|
||||
context_display: None,
|
||||
review_target: None,
|
||||
},
|
||||
)
|
||||
.await;
|
||||
|
|
@ -8031,6 +8032,7 @@ async fn submit_pending_interview_answer_rejects_invalid_answer_shape() {
|
|||
allow_freeform: false,
|
||||
timeout_seconds: None,
|
||||
context_display: None,
|
||||
review_target: None,
|
||||
},
|
||||
};
|
||||
|
||||
|
|
@ -8059,6 +8061,7 @@ fn validate_answer_for_question_accepts_no_for_confirmation() {
|
|||
allow_freeform: false,
|
||||
timeout_seconds: None,
|
||||
context_display: None,
|
||||
review_target: None,
|
||||
};
|
||||
|
||||
let result = validate_answer_for_question(&question, &Answer::no());
|
||||
|
|
@ -8077,6 +8080,7 @@ fn answer_from_typed_yes_request_maps_to_yes_answer() {
|
|||
allow_freeform: false,
|
||||
timeout_seconds: None,
|
||||
context_display: None,
|
||||
review_target: None,
|
||||
};
|
||||
let req: SubmitAnswerRequest = serde_json::from_value(json!({ "kind": "yes" })).unwrap();
|
||||
|
||||
|
|
@ -8096,6 +8100,7 @@ fn answer_from_typed_no_request_maps_to_no_answer() {
|
|||
allow_freeform: false,
|
||||
timeout_seconds: None,
|
||||
context_display: None,
|
||||
review_target: None,
|
||||
};
|
||||
let req: SubmitAnswerRequest = serde_json::from_value(json!({ "kind": "no" })).unwrap();
|
||||
|
||||
|
|
@ -8120,6 +8125,7 @@ fn answer_from_typed_selected_request_validates_and_attaches_option() {
|
|||
allow_freeform: false,
|
||||
timeout_seconds: None,
|
||||
context_display: None,
|
||||
review_target: None,
|
||||
};
|
||||
let req: SubmitAnswerRequest =
|
||||
serde_json::from_value(json!({ "kind": "selected", "option_key": "approve" })).unwrap();
|
||||
|
|
@ -8160,6 +8166,7 @@ fn answer_from_typed_multi_selected_request_validates_option_keys() {
|
|||
allow_freeform: false,
|
||||
timeout_seconds: None,
|
||||
context_display: None,
|
||||
review_target: None,
|
||||
};
|
||||
let req: SubmitAnswerRequest = serde_json::from_value(json!({
|
||||
"kind": "multi_selected",
|
||||
|
|
@ -9577,6 +9584,11 @@ async fn get_run_state_exposes_pending_interviews() {
|
|||
"allow_freeform": false,
|
||||
"context_display": null,
|
||||
"timeout_seconds": null,
|
||||
"review_target": {
|
||||
"label": "Quarry review exercise",
|
||||
"url": "https://quarry.lithos.computer/tmp/0123456789abcdef0123456789abcdef",
|
||||
"kind": "document"
|
||||
},
|
||||
}),
|
||||
Some("gate"),
|
||||
)
|
||||
|
|
@ -9656,6 +9668,11 @@ async fn cache_backed_run_endpoints_reflect_events_appended_after_warmup() {
|
|||
"allow_freeform": false,
|
||||
"context_display": null,
|
||||
"timeout_seconds": null,
|
||||
"review_target": {
|
||||
"label": "Quarry review exercise",
|
||||
"url": "https://quarry.lithos.computer/tmp/0123456789abcdef0123456789abcdef",
|
||||
"kind": "document"
|
||||
},
|
||||
}),
|
||||
Some("review"),
|
||||
)
|
||||
|
|
@ -9713,6 +9730,10 @@ async fn cache_backed_run_endpoints_reflect_events_appended_after_warmup() {
|
|||
state_body["pending_interviews"]["q-cache"]["question"]["text"].as_str(),
|
||||
Some("Approve cached deploy?")
|
||||
);
|
||||
assert_eq!(
|
||||
state_body["pending_interviews"]["q-cache"]["question"]["review_target"]["url"].as_str(),
|
||||
Some("https://quarry.lithos.computer/tmp/0123456789abcdef0123456789abcdef")
|
||||
);
|
||||
|
||||
let stages = app
|
||||
.clone()
|
||||
|
|
@ -9744,6 +9765,10 @@ async fn cache_backed_run_endpoints_reflect_events_appended_after_warmup() {
|
|||
questions["data"][0]["text"].as_str(),
|
||||
Some("Approve cached deploy?")
|
||||
);
|
||||
assert_eq!(
|
||||
questions["data"][0]["review_target"]["label"].as_str(),
|
||||
Some("Quarry review exercise")
|
||||
);
|
||||
|
||||
let settings = app
|
||||
.clone()
|
||||
|
|
@ -16489,6 +16514,7 @@ async fn list_runs_includes_live_metadata_from_run_state() {
|
|||
allow_freeform: false,
|
||||
timeout_seconds: None,
|
||||
context_display: None,
|
||||
review_target: None,
|
||||
},
|
||||
] {
|
||||
workflow_event::append_event(&run_store, &run_id, &event)
|
||||
|
|
|
|||
|
|
@ -111,6 +111,17 @@ fn parse_non_tty_freeform_response(prompt_read: PromptRead) -> Answer {
|
|||
}
|
||||
}
|
||||
|
||||
fn review_target_line(question: &Question) -> Option<String> {
|
||||
question.review_target.as_ref().map(|target| {
|
||||
format!(
|
||||
"Review {}: {} — {}",
|
||||
target.kind().noun(),
|
||||
target.label(),
|
||||
target.url()
|
||||
)
|
||||
})
|
||||
}
|
||||
|
||||
/// Ask a multiple-choice question using dialoguer's `Select` widget on a TTY.
|
||||
fn ask_select_interactive(question: &Question) -> Answer {
|
||||
let items: Vec<String> = question
|
||||
|
|
@ -237,6 +248,9 @@ impl Interviewer for ConsoleInterviewer {
|
|||
let rendered = self.styles.render_markdown(context_text);
|
||||
eprint!("{rendered}");
|
||||
}
|
||||
if let Some(line) = review_target_line(&question) {
|
||||
eprintln!("{line}");
|
||||
}
|
||||
let q = question;
|
||||
let answer = task::spawn_blocking(move || match q.question_type {
|
||||
QuestionType::MultipleChoice => ask_select_interactive(&q),
|
||||
|
|
@ -251,6 +265,9 @@ impl Interviewer for ConsoleInterviewer {
|
|||
|
||||
// Non-TTY fallback: line-based stdin reading
|
||||
let s = self.styles;
|
||||
if let Some(line) = review_target_line(&question) {
|
||||
eprintln!("{line}");
|
||||
}
|
||||
eprintln!("{} {}", s.bold_cyan.apply_to("?"), question.text);
|
||||
|
||||
let answer = match question.question_type {
|
||||
|
|
@ -314,6 +331,27 @@ mod tests {
|
|||
assert_eq!(answer.value, AnswerValue::Selected("A".to_string()));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn review_target_line_includes_label_and_url() {
|
||||
let mut question = Question::new("Review", QuestionType::MultipleChoice);
|
||||
question.review_target = Some(
|
||||
fabro_types::ReviewTarget::new(
|
||||
"Quarry review exercise",
|
||||
"https://quarry.lithos.computer/tmp/0123456789abcdef0123456789abcdef",
|
||||
fabro_types::ReviewTargetKind::Document,
|
||||
)
|
||||
.unwrap(),
|
||||
);
|
||||
|
||||
assert_eq!(
|
||||
review_target_line(&question).as_deref(),
|
||||
Some(
|
||||
"Review document: Quarry review exercise — \
|
||||
https://quarry.lithos.computer/tmp/0123456789abcdef0123456789abcdef"
|
||||
)
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn find_matching_option_by_key_case_insensitive() {
|
||||
let options = vec![InterviewOption {
|
||||
|
|
|
|||
|
|
@ -10,7 +10,7 @@ mod replay;
|
|||
use std::collections::HashMap;
|
||||
|
||||
use async_trait::async_trait;
|
||||
use fabro_types::{InterviewOption, Principal, QuestionType, SystemActorKind};
|
||||
use fabro_types::{InterviewOption, Principal, QuestionType, ReviewTarget, SystemActorKind};
|
||||
use serde::{Deserialize, Serialize};
|
||||
use tokio::time;
|
||||
|
||||
|
|
@ -29,6 +29,8 @@ pub struct Question {
|
|||
pub metadata: HashMap<String, serde_json::Value>,
|
||||
#[serde(default)]
|
||||
pub context_display: Option<String>,
|
||||
#[serde(default, skip_serializing_if = "Option::is_none")]
|
||||
pub review_target: Option<ReviewTarget>,
|
||||
}
|
||||
|
||||
impl Question {
|
||||
|
|
@ -44,6 +46,7 @@ impl Question {
|
|||
stage: String::new(),
|
||||
metadata: HashMap::new(),
|
||||
context_display: None,
|
||||
review_target: None,
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
@ -260,6 +263,7 @@ mod tests {
|
|||
assert!(q.timeout_seconds.is_none());
|
||||
assert!(q.stage.is_empty());
|
||||
assert!(q.metadata.is_empty());
|
||||
assert!(q.review_target.is_none());
|
||||
}
|
||||
|
||||
#[test]
|
||||
|
|
|
|||
|
|
@ -99,7 +99,16 @@ fn truncate_to_limit(text: &str, limit: usize, suffix: &str) -> String {
|
|||
/// final text is bounded by Slack's section-text limit so even pathological
|
||||
/// inputs cannot produce `invalid_blocks`.
|
||||
fn header_section(question: &Question, run_web_url: Option<&str>) -> Value {
|
||||
let mut text = format!("*{}*", escape_slack_controls(&question.text));
|
||||
let mut text = question.review_target.as_ref().map_or_else(
|
||||
|| format!("*{}*", escape_slack_controls(&question.text)),
|
||||
|target| {
|
||||
format!(
|
||||
"*Review the {} {}, then choose the next action.*",
|
||||
slack_link(target.url(), target.label()),
|
||||
target.kind().noun()
|
||||
)
|
||||
},
|
||||
);
|
||||
if !question.stage.is_empty() {
|
||||
let _ = write!(
|
||||
text,
|
||||
|
|
@ -634,6 +643,35 @@ mod tests {
|
|||
assert!(header.contains("<http://127.0.0.1:32276/runs/run-1|Open in Fabro>"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn header_renders_review_target_as_the_question_link() {
|
||||
let mut q = Question::new(
|
||||
"Review the Quarry review exercise document, then choose the next action.",
|
||||
QuestionType::MultipleChoice,
|
||||
);
|
||||
q.review_target = Some(
|
||||
fabro_types::ReviewTarget::new(
|
||||
"Quarry review exercise",
|
||||
"https://quarry.lithos.computer/tmp/0123456789abcdef0123456789abcdef",
|
||||
fabro_types::ReviewTargetKind::Document,
|
||||
)
|
||||
.unwrap(),
|
||||
);
|
||||
|
||||
let blocks = question_to_blocks("run-1", "q-1", &q, None);
|
||||
let header = serde_json::to_value(&blocks).unwrap()[0]["text"]["text"]
|
||||
.as_str()
|
||||
.unwrap()
|
||||
.to_string();
|
||||
|
||||
assert_eq!(
|
||||
header,
|
||||
"*Review the \
|
||||
<https://quarry.lithos.computer/tmp/0123456789abcdef0123456789abcdef|Quarry review \
|
||||
exercise> document, then choose the next action.*"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn header_omits_link_when_url_missing() {
|
||||
let q = Question::new("Approve Plan", QuestionType::YesNo);
|
||||
|
|
|
|||
|
|
@ -334,6 +334,7 @@ impl RunProjectionReducer for RunProjection {
|
|||
allow_freeform: props.allow_freeform,
|
||||
timeout_seconds: props.timeout_seconds,
|
||||
context_display: props.context_display.clone(),
|
||||
review_target: props.review_target.clone(),
|
||||
},
|
||||
started_at: ts,
|
||||
});
|
||||
|
|
@ -2859,6 +2860,14 @@ mod tests {
|
|||
allow_freeform: true,
|
||||
timeout_seconds: Some(30.0),
|
||||
context_display: Some("Latest draft".to_string()),
|
||||
review_target: Some(
|
||||
fabro_types::ReviewTarget::new(
|
||||
"Quarry review exercise",
|
||||
"https://quarry.lithos.computer/tmp/0123456789abcdef0123456789abcdef",
|
||||
fabro_types::ReviewTargetKind::Document,
|
||||
)
|
||||
.unwrap(),
|
||||
),
|
||||
}),
|
||||
Some("gate"),
|
||||
))
|
||||
|
|
@ -2886,6 +2895,14 @@ mod tests {
|
|||
pending.question.context_display.as_deref(),
|
||||
Some("Latest draft")
|
||||
);
|
||||
assert_eq!(
|
||||
pending
|
||||
.question
|
||||
.review_target
|
||||
.as_ref()
|
||||
.map(fabro_types::ReviewTarget::url),
|
||||
Some("https://quarry.lithos.computer/tmp/0123456789abcdef0123456789abcdef")
|
||||
);
|
||||
|
||||
state
|
||||
.apply_event(&test_event(
|
||||
|
|
|
|||
|
|
@ -235,6 +235,7 @@ fn projection_query_methods_expose_common_state() {
|
|||
allow_freeform: true,
|
||||
timeout_seconds: None,
|
||||
context_display: None,
|
||||
review_target: None,
|
||||
},
|
||||
started_at: Utc::now(),
|
||||
})]);
|
||||
|
|
|
|||
|
|
@ -22,6 +22,7 @@ const HANDLER_SPECIFIC_ATTRS: &[(&str, &[&str])] = &[
|
|||
("output_retries", &["agent", "prompt"]),
|
||||
("output_schema", &["agent", "prompt", "command"]),
|
||||
("prompt", &["agent", "prompt", "parallel.fan_in"]),
|
||||
("review_target", &["human"]),
|
||||
];
|
||||
|
||||
struct Rule;
|
||||
|
|
@ -197,9 +198,28 @@ mod tests {
|
|||
"prompt".to_string(),
|
||||
node_with_attr("prompt", "tab", "output_retries", "2"),
|
||||
);
|
||||
g.nodes.insert(
|
||||
"human".to_string(),
|
||||
node_with_attr("human", "hexagon", "review_target", "true"),
|
||||
);
|
||||
assert!(Rule.apply(&g).is_empty());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn warns_on_review_target_on_non_human_node() {
|
||||
let mut g = minimal_graph();
|
||||
g.nodes.insert(
|
||||
"work".to_string(),
|
||||
node_with_attr("work", "box", "review_target", "true"),
|
||||
);
|
||||
|
||||
let diagnostics = Rule.apply(&g);
|
||||
|
||||
assert_eq!(diagnostics.len(), 1);
|
||||
assert!(diagnostics[0].message.contains("'review_target'"));
|
||||
assert!(diagnostics[0].message.contains("human"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn accepts_prompt_on_shapeless_node_defaulting_to_agent() {
|
||||
let mut g = minimal_graph();
|
||||
|
|
|
|||
|
|
@ -12,6 +12,7 @@ pub mod keys {
|
|||
pub const PREFERRED_LABEL: &str = "preferred_label";
|
||||
pub const LAST_STAGE: &str = "last_stage";
|
||||
pub const LAST_RESPONSE: &str = "last_response";
|
||||
pub const REVIEW_TARGET: &str = "review_target";
|
||||
|
||||
// --- graph.* keys ---
|
||||
pub const GRAPH_GOAL: &str = "graph.goal";
|
||||
|
|
@ -142,6 +143,7 @@ pub mod keys {
|
|||
assert!(!is_engine_internal_key("outcome"));
|
||||
assert!(!is_engine_internal_key("last_stage"));
|
||||
assert!(!is_engine_internal_key("review.result"));
|
||||
assert!(!is_engine_internal_key(REVIEW_TARGET));
|
||||
assert!(!is_engine_internal_key("user.name"));
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -423,6 +423,7 @@ fn event_body_from_event(event: &Event) -> EventBody {
|
|||
allow_freeform,
|
||||
timeout_seconds,
|
||||
context_display,
|
||||
review_target,
|
||||
} => EventBody::InterviewStarted(fabro_types::InterviewStartedProps {
|
||||
question_id: question_id.clone(),
|
||||
question: question.clone(),
|
||||
|
|
@ -432,6 +433,7 @@ fn event_body_from_event(event: &Event) -> EventBody {
|
|||
allow_freeform: *allow_freeform,
|
||||
timeout_seconds: *timeout_seconds,
|
||||
context_display: context_display.clone(),
|
||||
review_target: review_target.clone(),
|
||||
}),
|
||||
Event::InterviewCompleted {
|
||||
actor: _,
|
||||
|
|
|
|||
|
|
@ -4,9 +4,10 @@ use ::fabro_types::{
|
|||
AutomationRef, BilledTokenCounts, BlockedReason, CommandTermination, DiffSummary,
|
||||
FailureReason, ForkSourceRef, GitContext, PairId, PairMessageId, PairSystemMessageKind,
|
||||
PairTarget, ParallelBranchId, ParallelBranchResult, PendingReason, PermissionLevel, Principal,
|
||||
PullRequestLink, RunBlobId, RunFailure, RunId, RunNoticeLevel, RunPairEndedReason,
|
||||
RunPairFailedReason, RunProvenance, RunRunnableSource, RunTiming, SandboxProviderKind, StageId,
|
||||
StageOutcome, StageTiming, SuccessReason, run_event as fabro_types,
|
||||
PullRequestLink, ReviewTarget, RunBlobId, RunFailure, RunId, RunNoticeLevel,
|
||||
RunPairEndedReason, RunPairFailedReason, RunProvenance, RunRunnableSource, RunTiming,
|
||||
SandboxProviderKind, StageId, StageOutcome, StageTiming, SuccessReason,
|
||||
run_event as fabro_types,
|
||||
};
|
||||
use fabro_agent::{AgentEvent, SandboxEvent};
|
||||
use fabro_model::{ReasoningEffort, Speed};
|
||||
|
|
@ -355,6 +356,8 @@ pub enum Event {
|
|||
timeout_seconds: Option<f64>,
|
||||
#[serde(default, skip_serializing_if = "Option::is_none")]
|
||||
context_display: Option<String>,
|
||||
#[serde(default, skip_serializing_if = "Option::is_none")]
|
||||
review_target: Option<ReviewTarget>,
|
||||
},
|
||||
InterviewCompleted {
|
||||
#[serde(default, skip_serializing_if = "Option::is_none")]
|
||||
|
|
|
|||
|
|
@ -6,7 +6,7 @@ use std::time::Instant;
|
|||
use async_trait::async_trait;
|
||||
use fabro_graphviz::graph::{Graph, Node};
|
||||
use fabro_interview::{Answer, AnswerValue, Interviewer, Question, ask_with_timeout};
|
||||
use fabro_types::{InterviewOption, Principal, QuestionType, SystemActorKind};
|
||||
use fabro_types::{InterviewOption, Principal, QuestionType, ReviewTarget, SystemActorKind};
|
||||
use ulid::Ulid;
|
||||
|
||||
use super::{EngineServices, Handler, NodeTimeoutPolicy};
|
||||
|
|
@ -121,6 +121,23 @@ fn build_human_gate_question(
|
|||
question.stage.clone_from(&node.id);
|
||||
question.timeout_seconds = node.timeout().map(|duration| duration.as_secs_f64());
|
||||
|
||||
if node.review_target() {
|
||||
let value = context.get(keys::REVIEW_TARGET).ok_or_else(|| {
|
||||
format!(
|
||||
"Human gate \"{}\" has review_target=true but context.review_target is missing",
|
||||
node.id
|
||||
)
|
||||
})?;
|
||||
let review_target = serde_json::from_value::<ReviewTarget>(value).map_err(|error| {
|
||||
format!(
|
||||
"Human gate \"{}\" has invalid context.review_target: {error}",
|
||||
node.id
|
||||
)
|
||||
})?;
|
||||
question.text = review_target.question_text();
|
||||
question.review_target = Some(review_target);
|
||||
}
|
||||
|
||||
if let Some(serde_json::Value::String(last_node)) = context.get(keys::LAST_STAGE) {
|
||||
if let Some(serde_json::Value::String(response)) =
|
||||
context.get(&keys::response_key(&last_node))
|
||||
|
|
@ -253,6 +270,7 @@ impl Handler for HumanHandler {
|
|||
allow_freeform: question.allow_freeform,
|
||||
timeout_seconds: question.timeout_seconds,
|
||||
context_display: question.context_display.clone(),
|
||||
review_target: question.review_target.clone(),
|
||||
},
|
||||
&stage_scope,
|
||||
);
|
||||
|
|
@ -639,6 +657,15 @@ mod tests {
|
|||
graph
|
||||
}
|
||||
|
||||
fn enable_review_target(graph: &mut Graph) {
|
||||
graph
|
||||
.nodes
|
||||
.get_mut("gate")
|
||||
.unwrap()
|
||||
.attrs
|
||||
.insert("review_target".to_string(), AttrValue::Boolean(true));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn parse_accelerator_key_bracket() {
|
||||
assert_eq!(parse_accelerator_key("[A] Approve"), "A");
|
||||
|
|
@ -665,6 +692,160 @@ mod tests {
|
|||
assert_eq!(parse_accelerator_key(""), "");
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn review_target_gate_snapshots_validated_context_into_question_and_event() {
|
||||
let inner = Box::new(AutoApproveInterviewer::engine());
|
||||
let recorder = Arc::new(RecordingInterviewer::new(inner));
|
||||
let handler = HumanHandler::new(recorder.clone());
|
||||
let mut graph = build_graph_with_human_gate();
|
||||
enable_review_target(&mut graph);
|
||||
let node = graph.nodes.get("gate").unwrap();
|
||||
let context = Context::new();
|
||||
let target_value = serde_json::json!({
|
||||
"label": "Quarry review exercise",
|
||||
"url": "https://quarry.lithos.computer/tmp/0123456789abcdef0123456789abcdef",
|
||||
"kind": "document",
|
||||
});
|
||||
context.set(keys::REVIEW_TARGET, target_value.clone());
|
||||
let events = Arc::new(Mutex::new(Vec::new()));
|
||||
|
||||
let outcome = handler
|
||||
.execute(
|
||||
node,
|
||||
&context,
|
||||
&graph,
|
||||
Path::new("/tmp/test"),
|
||||
&make_services_with_events(Arc::clone(&events)),
|
||||
)
|
||||
.await
|
||||
.unwrap();
|
||||
|
||||
let recordings = recorder.recordings();
|
||||
assert_eq!(recordings.len(), 1);
|
||||
let question = &recordings[0].0;
|
||||
assert_eq!(
|
||||
question.text,
|
||||
"Review the Quarry review exercise document, then choose the next action."
|
||||
);
|
||||
assert_eq!(
|
||||
question.review_target.as_ref().map(ReviewTarget::url),
|
||||
Some("https://quarry.lithos.computer/tmp/0123456789abcdef0123456789abcdef")
|
||||
);
|
||||
|
||||
let event_target = events
|
||||
.lock()
|
||||
.expect("event log lock poisoned")
|
||||
.iter()
|
||||
.find_map(|event| match &event.body {
|
||||
EventBody::InterviewStarted(props) => props.review_target.clone(),
|
||||
_ => None,
|
||||
})
|
||||
.expect("interview.started should carry the review target");
|
||||
assert_eq!(event_target.label(), "Quarry review exercise");
|
||||
|
||||
context.apply_updates(&outcome.context_updates);
|
||||
assert_eq!(context.get(keys::REVIEW_TARGET), Some(target_value));
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn review_target_gate_fails_before_interview_when_context_is_missing() {
|
||||
let inner = Box::new(AutoApproveInterviewer::engine());
|
||||
let recorder = Arc::new(RecordingInterviewer::new(inner));
|
||||
let handler = HumanHandler::new(recorder.clone());
|
||||
let mut graph = build_graph_with_human_gate();
|
||||
enable_review_target(&mut graph);
|
||||
let node = graph.nodes.get("gate").unwrap();
|
||||
|
||||
let outcome = handler
|
||||
.execute(
|
||||
node,
|
||||
&Context::new(),
|
||||
&graph,
|
||||
Path::new("/tmp/test"),
|
||||
&make_services(),
|
||||
)
|
||||
.await
|
||||
.unwrap();
|
||||
|
||||
assert!(outcome.status.is_failure());
|
||||
assert_eq!(
|
||||
outcome.failure_reason(),
|
||||
Some("Human gate \"gate\" has review_target=true but context.review_target is missing")
|
||||
);
|
||||
assert!(recorder.recordings().is_empty());
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn review_target_gate_rejects_unsafe_url_without_echoing_it() {
|
||||
let inner = Box::new(AutoApproveInterviewer::engine());
|
||||
let recorder = Arc::new(RecordingInterviewer::new(inner));
|
||||
let handler = HumanHandler::new(recorder.clone());
|
||||
let mut graph = build_graph_with_human_gate();
|
||||
enable_review_target(&mut graph);
|
||||
let node = graph.nodes.get("gate").unwrap();
|
||||
let context = Context::new();
|
||||
context.set(
|
||||
keys::REVIEW_TARGET,
|
||||
serde_json::json!({
|
||||
"label": "Unsafe review",
|
||||
"url": "javascript:alert(1)",
|
||||
"kind": "document",
|
||||
}),
|
||||
);
|
||||
|
||||
let outcome = handler
|
||||
.execute(
|
||||
node,
|
||||
&context,
|
||||
&graph,
|
||||
Path::new("/tmp/test"),
|
||||
&make_services(),
|
||||
)
|
||||
.await
|
||||
.unwrap();
|
||||
|
||||
let failure = outcome
|
||||
.failure_reason()
|
||||
.expect("invalid review target should fail");
|
||||
assert!(failure.contains("review target URL must use http or https"));
|
||||
assert!(!failure.contains("javascript:alert"));
|
||||
assert!(recorder.recordings().is_empty());
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn human_gate_ignores_review_target_context_without_opt_in() {
|
||||
let inner = Box::new(AutoApproveInterviewer::engine());
|
||||
let recorder = Arc::new(RecordingInterviewer::new(inner));
|
||||
let handler = HumanHandler::new(recorder.clone());
|
||||
let graph = build_graph_with_human_gate();
|
||||
let node = graph.nodes.get("gate").unwrap();
|
||||
let context = Context::new();
|
||||
context.set(
|
||||
keys::REVIEW_TARGET,
|
||||
serde_json::json!({
|
||||
"label": "Unsafe review",
|
||||
"url": "javascript:alert(1)",
|
||||
"kind": "document",
|
||||
}),
|
||||
);
|
||||
|
||||
let outcome = handler
|
||||
.execute(
|
||||
node,
|
||||
&context,
|
||||
&graph,
|
||||
Path::new("/tmp/test"),
|
||||
&make_services(),
|
||||
)
|
||||
.await
|
||||
.unwrap();
|
||||
|
||||
assert_eq!(outcome.status, crate::outcome::StageOutcome::Succeeded);
|
||||
let recordings = recorder.recordings();
|
||||
assert_eq!(recordings[0].0.text, "Review Changes");
|
||||
assert!(recordings[0].0.review_target.is_none());
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn wait_human_auto_approve_selects_first() {
|
||||
let interviewer = Arc::new(AutoApproveInterviewer::engine());
|
||||
|
|
|
|||
|
|
@ -224,6 +224,7 @@ impl AgentQuestionRuntime for WorkflowAgentQuestionRuntime {
|
|||
allow_freeform: question.allow_freeform,
|
||||
timeout_seconds: None,
|
||||
context_display: question.context_display.clone(),
|
||||
review_target: question.review_target.clone(),
|
||||
},
|
||||
&self.stage_scope,
|
||||
);
|
||||
|
|
|
|||
|
|
@ -463,6 +463,8 @@ fn main() {
|
|||
&[],
|
||||
),
|
||||
("InterviewOption", "fabro_types::InterviewOption", &[]),
|
||||
("ReviewTarget", "fabro_types::ReviewTarget", &[]),
|
||||
("ReviewTargetKind", "fabro_types::ReviewTargetKind", &[]),
|
||||
(
|
||||
"InterviewQuestionRecord",
|
||||
"fabro_types::InterviewQuestionRecord",
|
||||
|
|
|
|||
|
|
@ -55,12 +55,12 @@ pub mod types {
|
|||
PairTranscriptResponse, ParallelBranchResult, PendingInterviewRecord, PermissionLevel,
|
||||
PreRunPushOutcome, Principal, PullRequest, PullRequestDetails, PullRequestDetailsStatus,
|
||||
PullRequestDetailsUnavailableReason, PullRequestLink, PullRequestMeta, PullRequestResponse,
|
||||
QuestionType, ReasoningOutput, RepositoryRef, Role, Run, RunApproval, RunApprovalState,
|
||||
RunClientProvenance, RunEvent, RunEventDetailContentKind, RunEventDetailResponse,
|
||||
RunFailure, RunPairStatusResponse, RunProjection, RunProvenance, RunRunnableSource,
|
||||
RunSandbox, RunSandboxFailure, RunSandboxInstance, RunSandboxKind, RunSandboxPlan,
|
||||
RunSandboxRuntime, RunServerProvenance, RunSize, SandboxDetails, SandboxInfo,
|
||||
SandboxListMeta, SandboxListResponse, SandboxNetwork, SandboxNetworkPolicy,
|
||||
QuestionType, ReasoningOutput, RepositoryRef, ReviewTarget, ReviewTargetKind, Role, Run,
|
||||
RunApproval, RunApprovalState, RunClientProvenance, RunEvent, RunEventDetailContentKind,
|
||||
RunEventDetailResponse, RunFailure, RunPairStatusResponse, RunProjection, RunProvenance,
|
||||
RunRunnableSource, RunSandbox, RunSandboxFailure, RunSandboxInstance, RunSandboxKind,
|
||||
RunSandboxPlan, RunSandboxRuntime, RunServerProvenance, RunSize, SandboxDetails,
|
||||
SandboxInfo, SandboxListMeta, SandboxListResponse, SandboxNetwork, SandboxNetworkPolicy,
|
||||
SandboxNetworkPolicyMode, SandboxProviderKind, SandboxProviderLookupError,
|
||||
SandboxResources, SandboxService, SandboxServiceListResponse, SandboxState,
|
||||
SandboxTimestamps, SecretMetadata, SecretType, ServerSettings, SessionDetail, SessionId,
|
||||
|
|
|
|||
|
|
@ -1,12 +1,17 @@
|
|||
use std::any::{TypeId, type_name};
|
||||
|
||||
use fabro_api::types::InterviewQuestionRecord as ApiInterviewQuestionRecord;
|
||||
use fabro_types::InterviewQuestionRecord;
|
||||
use fabro_api::types::{
|
||||
InterviewQuestionRecord as ApiInterviewQuestionRecord, ReviewTarget as ApiReviewTarget,
|
||||
ReviewTargetKind as ApiReviewTargetKind,
|
||||
};
|
||||
use fabro_types::{InterviewQuestionRecord, ReviewTarget, ReviewTargetKind};
|
||||
use serde_json::json;
|
||||
|
||||
#[test]
|
||||
fn interview_question_record_reuses_canonical_type() {
|
||||
assert_same_type::<ApiInterviewQuestionRecord, InterviewQuestionRecord>();
|
||||
assert_same_type::<ApiReviewTarget, ReviewTarget>();
|
||||
assert_same_type::<ApiReviewTargetKind, ReviewTargetKind>();
|
||||
}
|
||||
|
||||
#[test]
|
||||
|
|
@ -27,7 +32,12 @@ fn interview_question_record_round_trips_representative_json() {
|
|||
],
|
||||
"allow_freeform": true,
|
||||
"timeout_seconds": 30.0,
|
||||
"context_display": "Diff summary"
|
||||
"context_display": "Diff summary",
|
||||
"review_target": {
|
||||
"label": "Quarry review exercise",
|
||||
"url": "https://quarry.lithos.computer/tmp/0123456789abcdef0123456789abcdef",
|
||||
"kind": "document"
|
||||
}
|
||||
});
|
||||
|
||||
let question: InterviewQuestionRecord = serde_json::from_value(value.clone()).unwrap();
|
||||
|
|
|
|||
|
|
@ -28,7 +28,12 @@ fn pending_interview_record_round_trips_populated_question() {
|
|||
],
|
||||
"allow_freeform": true,
|
||||
"timeout_seconds": 30.0,
|
||||
"context_display": "Diff summary"
|
||||
"context_display": "Diff summary",
|
||||
"review_target": {
|
||||
"label": "Quarry review exercise",
|
||||
"url": "https://quarry.lithos.computer/tmp/0123456789abcdef0123456789abcdef",
|
||||
"kind": "document"
|
||||
}
|
||||
},
|
||||
"started_at": "2026-04-29T12:34:56Z"
|
||||
});
|
||||
|
|
|
|||
|
|
@ -28,6 +28,7 @@ serde.workspace = true
|
|||
serde_json.workspace = true
|
||||
sha2.workspace = true
|
||||
strum.workspace = true
|
||||
thiserror.workspace = true
|
||||
toml.workspace = true
|
||||
ulid.workspace = true
|
||||
url.workspace = true
|
||||
|
|
|
|||
|
|
@ -193,6 +193,11 @@ impl Node {
|
|||
self.bool_attr("goal_gate").unwrap_or(false)
|
||||
}
|
||||
|
||||
#[must_use]
|
||||
pub fn review_target(&self) -> bool {
|
||||
self.bool_attr("review_target").unwrap_or(false)
|
||||
}
|
||||
|
||||
#[must_use]
|
||||
pub fn retry_target(&self) -> Option<&str> {
|
||||
self.str_attr("retry_target")
|
||||
|
|
@ -590,6 +595,7 @@ mod tests {
|
|||
assert_eq!(node.output_retries(), 2);
|
||||
assert_eq!(node.max_retries(), None);
|
||||
assert!(!node.goal_gate());
|
||||
assert!(!node.review_target());
|
||||
assert_eq!(node.retry_target(), None);
|
||||
assert_eq!(node.fallback_retry_target(), None);
|
||||
assert_eq!(node.fidelity(), None);
|
||||
|
|
@ -652,12 +658,15 @@ mod tests {
|
|||
);
|
||||
node.attrs
|
||||
.insert("goal_gate".to_string(), AttrValue::Boolean(true));
|
||||
node.attrs
|
||||
.insert("review_target".to_string(), AttrValue::Boolean(true));
|
||||
node.attrs
|
||||
.insert("max_retries".to_string(), AttrValue::Integer(3));
|
||||
|
||||
assert_eq!(node.label(), "Plan step");
|
||||
assert_eq!(node.shape(), "diamond");
|
||||
assert!(node.goal_gate());
|
||||
assert!(node.review_target());
|
||||
assert_eq!(node.max_retries(), Some(3));
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -1,7 +1,172 @@
|
|||
use serde::de::Error as _;
|
||||
use serde::{Deserialize, Serialize};
|
||||
use thiserror::Error;
|
||||
|
||||
use crate::run_event::InterviewOption;
|
||||
|
||||
const REVIEW_TARGET_LABEL_MAX_CHARS: usize = 200;
|
||||
const REVIEW_TARGET_URL_MAX_CHARS: usize = 2048;
|
||||
|
||||
/// The type of resource a human should review.
|
||||
#[derive(
|
||||
Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, strum::Display, strum::EnumString,
|
||||
)]
|
||||
#[serde(rename_all = "snake_case")]
|
||||
#[strum(serialize_all = "snake_case")]
|
||||
pub enum ReviewTargetKind {
|
||||
Document,
|
||||
}
|
||||
|
||||
impl ReviewTargetKind {
|
||||
#[must_use]
|
||||
pub const fn noun(self) -> &'static str {
|
||||
match self {
|
||||
Self::Document => "document",
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// A validated external resource presented as the primary subject of a human
|
||||
/// review question.
|
||||
#[derive(Debug, Clone, PartialEq, Eq, Serialize)]
|
||||
pub struct ReviewTarget {
|
||||
label: String,
|
||||
url: String,
|
||||
kind: ReviewTargetKind,
|
||||
}
|
||||
|
||||
impl ReviewTarget {
|
||||
pub fn new(
|
||||
label: impl Into<String>,
|
||||
url: impl Into<String>,
|
||||
kind: ReviewTargetKind,
|
||||
) -> Result<Self, ReviewTargetError> {
|
||||
let label = label.into();
|
||||
let url = url.into();
|
||||
let label = label.trim();
|
||||
let url = url.trim();
|
||||
|
||||
validate_review_target_label(label)?;
|
||||
validate_review_target_url(url)?;
|
||||
|
||||
Ok(Self {
|
||||
label: label.to_string(),
|
||||
url: url.to_string(),
|
||||
kind,
|
||||
})
|
||||
}
|
||||
|
||||
#[must_use]
|
||||
pub fn label(&self) -> &str {
|
||||
&self.label
|
||||
}
|
||||
|
||||
#[must_use]
|
||||
pub fn url(&self) -> &str {
|
||||
&self.url
|
||||
}
|
||||
|
||||
#[must_use]
|
||||
pub const fn kind(&self) -> ReviewTargetKind {
|
||||
self.kind
|
||||
}
|
||||
|
||||
#[must_use]
|
||||
pub fn question_text(&self) -> String {
|
||||
format!(
|
||||
"Review the {} {}, then choose the next action.",
|
||||
self.label,
|
||||
self.kind.noun()
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, PartialEq, Eq, Error)]
|
||||
pub enum ReviewTargetError {
|
||||
#[error("review target label must not be empty")]
|
||||
EmptyLabel,
|
||||
#[error("review target label must be at most {REVIEW_TARGET_LABEL_MAX_CHARS} characters")]
|
||||
LabelTooLong,
|
||||
#[error("review target label must not contain control characters")]
|
||||
LabelContainsControl,
|
||||
#[error("review target URL must not be empty")]
|
||||
EmptyUrl,
|
||||
#[error("review target URL must be at most {REVIEW_TARGET_URL_MAX_CHARS} characters")]
|
||||
UrlTooLong,
|
||||
#[error("review target URL must not contain control characters or Slack link delimiters")]
|
||||
UrlContainsUnsafeCharacters,
|
||||
#[error("review target URL must be a valid absolute URL")]
|
||||
InvalidUrl,
|
||||
#[error("review target URL must use http or https")]
|
||||
UnsupportedUrlScheme,
|
||||
#[error("review target URL must include a host")]
|
||||
MissingUrlHost,
|
||||
#[error("review target URL must not include username or password credentials")]
|
||||
UrlContainsCredentials,
|
||||
}
|
||||
|
||||
fn validate_review_target_label(label: &str) -> Result<(), ReviewTargetError> {
|
||||
if label.is_empty() {
|
||||
return Err(ReviewTargetError::EmptyLabel);
|
||||
}
|
||||
if label.chars().count() > REVIEW_TARGET_LABEL_MAX_CHARS {
|
||||
return Err(ReviewTargetError::LabelTooLong);
|
||||
}
|
||||
if label.chars().any(char::is_control) {
|
||||
return Err(ReviewTargetError::LabelContainsControl);
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[expect(
|
||||
clippy::disallowed_types,
|
||||
reason = "Review target validation parses an untrusted URL only to enforce safe display schemes and syntax; Fabro never fetches the URL."
|
||||
)]
|
||||
fn validate_review_target_url(url: &str) -> Result<(), ReviewTargetError> {
|
||||
if url.is_empty() {
|
||||
return Err(ReviewTargetError::EmptyUrl);
|
||||
}
|
||||
if url.chars().count() > REVIEW_TARGET_URL_MAX_CHARS {
|
||||
return Err(ReviewTargetError::UrlTooLong);
|
||||
}
|
||||
if url
|
||||
.chars()
|
||||
.any(|character| character.is_control() || matches!(character, '<' | '>' | '|'))
|
||||
{
|
||||
return Err(ReviewTargetError::UrlContainsUnsafeCharacters);
|
||||
}
|
||||
|
||||
let parsed = url::Url::parse(url).map_err(|_| ReviewTargetError::InvalidUrl)?;
|
||||
if !matches!(parsed.scheme(), "http" | "https") {
|
||||
return Err(ReviewTargetError::UnsupportedUrlScheme);
|
||||
}
|
||||
if parsed.host_str().is_none() {
|
||||
return Err(ReviewTargetError::MissingUrlHost);
|
||||
}
|
||||
if !parsed.username().is_empty() || parsed.password().is_some() {
|
||||
return Err(ReviewTargetError::UrlContainsCredentials);
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
|
||||
impl<'de> Deserialize<'de> for ReviewTarget {
|
||||
fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
|
||||
where
|
||||
D: serde::Deserializer<'de>,
|
||||
{
|
||||
#[derive(Deserialize)]
|
||||
#[serde(deny_unknown_fields)]
|
||||
struct WireReviewTarget {
|
||||
label: String,
|
||||
url: String,
|
||||
kind: ReviewTargetKind,
|
||||
}
|
||||
|
||||
let wire = WireReviewTarget::deserialize(deserializer)?;
|
||||
Self::new(wire.label, wire.url, wire.kind).map_err(D::Error::custom)
|
||||
}
|
||||
}
|
||||
|
||||
#[derive(
|
||||
Debug,
|
||||
Clone,
|
||||
|
|
@ -43,6 +208,8 @@ pub struct InterviewQuestionRecord {
|
|||
pub timeout_seconds: Option<f64>,
|
||||
#[serde(default, skip_serializing_if = "Option::is_none")]
|
||||
pub context_display: Option<String>,
|
||||
#[serde(default, skip_serializing_if = "Option::is_none")]
|
||||
pub review_target: Option<ReviewTarget>,
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
|
|
@ -64,4 +231,52 @@ mod tests {
|
|||
assert_eq!(question_type.to_string(), wire);
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn review_target_roundtrips_and_builds_question_text() {
|
||||
let value = serde_json::json!({
|
||||
"label": "Quarry review exercise",
|
||||
"url": "https://quarry.lithos.computer/tmp/0123456789abcdef0123456789abcdef",
|
||||
"kind": "document",
|
||||
});
|
||||
|
||||
let target: ReviewTarget = serde_json::from_value(value.clone()).unwrap();
|
||||
|
||||
assert_eq!(target.label(), "Quarry review exercise");
|
||||
assert_eq!(
|
||||
target.question_text(),
|
||||
"Review the Quarry review exercise document, then choose the next action."
|
||||
);
|
||||
assert_eq!(serde_json::to_value(target).unwrap(), value);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn review_target_rejects_non_http_urls_and_credentials() {
|
||||
for url in [
|
||||
"javascript:alert(1)",
|
||||
"file:///tmp/review.md",
|
||||
"https://user:secret@example.com/review",
|
||||
] {
|
||||
assert!(
|
||||
ReviewTarget::new("Review", url, ReviewTargetKind::Document).is_err(),
|
||||
"URL should be rejected: {url}"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn review_target_rejects_blank_or_unsafe_display_values() {
|
||||
assert_eq!(
|
||||
ReviewTarget::new(" ", "https://example.com", ReviewTargetKind::Document),
|
||||
Err(ReviewTargetError::EmptyLabel)
|
||||
);
|
||||
assert_eq!(
|
||||
ReviewTarget::new(
|
||||
"Review",
|
||||
"https://example.com/a|b",
|
||||
ReviewTargetKind::Document
|
||||
),
|
||||
Err(ReviewTargetError::UrlContainsUnsafeCharacters)
|
||||
);
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -75,7 +75,9 @@ pub use graph::{
|
|||
AttrValue, Edge, Graph, KNOWN_HANDLER_TYPES, Node, is_known_handler_type, is_llm_handler_type,
|
||||
shape_to_handler_type,
|
||||
};
|
||||
pub use interview::{InterviewQuestionRecord, QuestionType};
|
||||
pub use interview::{
|
||||
InterviewQuestionRecord, QuestionType, ReviewTarget, ReviewTargetError, ReviewTargetKind,
|
||||
};
|
||||
pub use llm_backend::AgentBackend;
|
||||
pub use manifest_path::{ManifestPath, ManifestPathParseError};
|
||||
pub use mcp_store::{
|
||||
|
|
|
|||
|
|
@ -65,6 +65,8 @@ pub struct InterviewStartedProps {
|
|||
pub timeout_seconds: Option<f64>,
|
||||
#[serde(default, skip_serializing_if = "Option::is_none")]
|
||||
pub context_display: Option<String>,
|
||||
#[serde(default, skip_serializing_if = "Option::is_none")]
|
||||
pub review_target: Option<crate::ReviewTarget>,
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
|
||||
|
|
|
|||
|
|
@ -335,6 +335,8 @@ models/replace-mcp-server-request.ts
|
|||
models/repo-check-response-permissions.ts
|
||||
models/repo-check-response.ts
|
||||
models/repository-ref.ts
|
||||
models/review-target-kind.ts
|
||||
models/review-target.ts
|
||||
models/rewind-request.ts
|
||||
models/rewind-response.ts
|
||||
models/root-response-urls.ts
|
||||
|
|
|
|||
|
|
@ -19,6 +19,9 @@ import type { InterviewOption } from './interview-option';
|
|||
// May contain unused imports in some cases
|
||||
// @ts-ignore
|
||||
import type { QuestionType } from './question-type';
|
||||
// May contain unused imports in some cases
|
||||
// @ts-ignore
|
||||
import type { ReviewTarget } from './review-target';
|
||||
|
||||
/**
|
||||
* A pending human-in-the-loop question generated by a workflow stage.
|
||||
|
|
@ -53,4 +56,5 @@ export interface ApiQuestion {
|
|||
* Optional contextual text shown alongside the question.
|
||||
*/
|
||||
'context_display'?: string | null;
|
||||
'review_target'?: ReviewTarget | null;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -305,6 +305,8 @@ export * from './replace-mcp-server-request';
|
|||
export * from './repo-check-response';
|
||||
export * from './repo-check-response-permissions';
|
||||
export * from './repository-ref';
|
||||
export * from './review-target';
|
||||
export * from './review-target-kind';
|
||||
export * from './rewind-request';
|
||||
export * from './rewind-response';
|
||||
export * from './root-response';
|
||||
|
|
|
|||
|
|
@ -19,6 +19,9 @@ import type { InterviewOption } from './interview-option';
|
|||
// May contain unused imports in some cases
|
||||
// @ts-ignore
|
||||
import type { QuestionType } from './question-type';
|
||||
// May contain unused imports in some cases
|
||||
// @ts-ignore
|
||||
import type { ReviewTarget } from './review-target';
|
||||
|
||||
/**
|
||||
* Storage shape of an interview question recorded in the event log.
|
||||
|
|
@ -32,4 +35,5 @@ export interface InterviewQuestionRecord {
|
|||
'allow_freeform': boolean;
|
||||
'timeout_seconds'?: number | null;
|
||||
'context_display'?: string | null;
|
||||
'review_target'?: ReviewTarget | null;
|
||||
}
|
||||
|
|
|
|||
25
lib/packages/fabro-api-client/src/models/review-target-kind.ts
generated
Normal file
25
lib/packages/fabro-api-client/src/models/review-target-kind.ts
generated
Normal file
|
|
@ -0,0 +1,25 @@
|
|||
/* tslint:disable */
|
||||
/* eslint-disable */
|
||||
/**
|
||||
* Fabro Run API
|
||||
* HTTP API for managing Fabro workflow run executions.
|
||||
*
|
||||
* The version of the OpenAPI document: 0.1.0
|
||||
*
|
||||
*
|
||||
* NOTE: This class is auto generated by OpenAPI Generator (https://openapi-generator.tech).
|
||||
* https://openapi-generator.tech
|
||||
* Do not edit the class manually.
|
||||
*/
|
||||
|
||||
|
||||
|
||||
/**
|
||||
* The type of resource presented for human review.
|
||||
*/
|
||||
|
||||
export const ReviewTargetKind = {
|
||||
DOCUMENT: 'document'
|
||||
} as const;
|
||||
|
||||
export type ReviewTargetKind = typeof ReviewTargetKind[keyof typeof ReviewTargetKind];
|
||||
33
lib/packages/fabro-api-client/src/models/review-target.ts
generated
Normal file
33
lib/packages/fabro-api-client/src/models/review-target.ts
generated
Normal file
|
|
@ -0,0 +1,33 @@
|
|||
/* tslint:disable */
|
||||
/* eslint-disable */
|
||||
/**
|
||||
* Fabro Run API
|
||||
* HTTP API for managing Fabro workflow run executions.
|
||||
*
|
||||
* The version of the OpenAPI document: 0.1.0
|
||||
*
|
||||
*
|
||||
* NOTE: This class is auto generated by OpenAPI Generator (https://openapi-generator.tech).
|
||||
* https://openapi-generator.tech
|
||||
* Do not edit the class manually.
|
||||
*/
|
||||
|
||||
|
||||
// May contain unused imports in some cases
|
||||
// @ts-ignore
|
||||
import type { ReviewTargetKind } from './review-target-kind';
|
||||
|
||||
/**
|
||||
* A validated external resource presented as the primary subject of a human review question.
|
||||
*/
|
||||
export interface ReviewTarget {
|
||||
/**
|
||||
* Human-readable link label.
|
||||
*/
|
||||
'label': string;
|
||||
/**
|
||||
* Absolute HTTP or HTTPS URL opened by the reviewer.
|
||||
*/
|
||||
'url': string;
|
||||
'kind': ReviewTargetKind;
|
||||
}
|
||||
Loading…
Add table
Reference in a new issue