diff --git a/apps/fabro-web/app/components/interview-dock.tsx b/apps/fabro-web/app/components/interview-dock.tsx index 5012f53c1..953481d8d 100644 --- a/apps/fabro-web/app/components/interview-dock.tsx +++ b/apps/fabro-web/app/components/interview-dock.tsx @@ -22,9 +22,14 @@ import { } from "../lib/mutations"; import { ApiError } from "../lib/api-client"; import { displayLabel } from "./interview-label"; -import { ReviewTargetQuestion } from "./review-target-question"; +import { + ReviewTargetQuestion, + safeReviewTarget, +} from "./review-target-question"; import { ErrorMessage } from "./ui"; +const QUESTION_TEXT = "text-pretty text-base/6 font-medium text-fg"; + const PRIMARY_BUTTON = "inline-flex items-center justify-center gap-1.5 rounded-lg bg-teal-500 px-3.5 py-2 text-sm font-medium text-on-primary transition-colors hover:bg-teal-300 focus-visible:outline-2 focus-visible:outline-offset-2 focus-visible:outline-teal-500 disabled:cursor-not-allowed disabled:opacity-60 disabled:hover:bg-teal-500"; @@ -78,6 +83,7 @@ function InterviewQuestionDock({ const submitMutation = useSubmitInterviewAnswer(runId); const [error, setError] = useState(null); const submitting = submitMutation.isMutating; + const reviewTarget = safeReviewTarget(question.review_target); const submit = useCallback( async (answer: SubmitInterviewAnswer) => { @@ -100,11 +106,14 @@ function InterviewQuestionDock({ />
- + {reviewTarget ? ( + + ) : ( +

{question.text}

+ )}

{questionTypeLabel(question.question_type)}

diff --git a/apps/fabro-web/app/components/review-target-question.tsx b/apps/fabro-web/app/components/review-target-question.tsx index 99c041923..22961f95f 100644 --- a/apps/fabro-web/app/components/review-target-question.tsx +++ b/apps/fabro-web/app/components/review-target-question.tsx @@ -1,76 +1,59 @@ 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; - } +import type { ReviewTarget } from "@qltysh/fabro-api-client"; +/** + * Re-check the URL before putting it in an `href`. The server already rejects + * unsafe targets (see `ReviewTarget::new` in + * `lib/foundation/fabro-types/src/interview.rs`), but React does not sanitize + * `href`, so a `javascript:` URL reaching this component would execute. Length + * and control-character limits stay server-side; they cannot affect the DOM. + */ +export function safeReviewTarget( + target: ReviewTarget | null | undefined, +): ReviewTarget | null { + if (!target?.url || !target.label) return null; try { const parsed = new URL(target.url); - return ( + const safe = (parsed.protocol === "http:" || parsed.protocol === "https:") && Boolean(parsed.host) && !parsed.username && - !parsed.password - ); + !parsed.password; + return safe ? target : null; } catch { - return false; + return null; } } +/** + * The review question sentence, with the target label as an external link. + * Mirrors `ReviewTarget::question_text_with_link` in + * `lib/foundation/fabro-types/src/interview.rs`. + */ export function ReviewTargetQuestion({ - reviewTarget, - fallbackText, + target, className, }: { - reviewTarget: ReviewTarget | null | undefined; - fallbackText: string; + target: ReviewTarget; className?: string; }) { - if (!hasSafeReviewTarget(reviewTarget)) { - return

{fallbackText}

; - } - return (

Review the{" "} - {reviewTarget.label} + {target.label} {" "} - document, then choose the next action. + {target.kind}, then choose the next action.

); } diff --git a/apps/fabro-web/app/components/stage-renderers/helpers.ts b/apps/fabro-web/app/components/stage-renderers/helpers.ts index 6ddcba7a0..024c086c8 100644 --- a/apps/fabro-web/app/components/stage-renderers/helpers.ts +++ b/apps/fabro-web/app/components/stage-renderers/helpers.ts @@ -1,10 +1,14 @@ -import { StageOutcome } from "@qltysh/fabro-api-client"; -import type { - EventEnvelope, - ReviewTarget, -} from "@qltysh/fabro-api-client"; +import { ReviewTargetKind, StageOutcome } from "@qltysh/fabro-api-client"; +import type { EventEnvelope, ReviewTarget } from "@qltysh/fabro-api-client"; -import { getArray, getNumber, getObject, getString, type UnknownRecord } from "../../lib/unknown"; +import { + getArray, + getNumber, + getObject, + getString, + isRecord, + type UnknownRecord, +} from "../../lib/unknown"; const STAGE_OUTCOMES: ReadonlySet = new Set(Object.values(StageOutcome)); @@ -80,12 +84,11 @@ function parseInterviewOptions(value: unknown): InterviewOption[] { } 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; + if (!isRecord(value)) return null; + const label = getString(value, "label"); + const url = getString(value, "url"); + const kind = getString(value, "kind"); + if (!label || !url || kind !== ReviewTargetKind.DOCUMENT) return null; return { label, url, kind }; } diff --git a/apps/fabro-web/app/components/stage-renderers/human-qa.tsx b/apps/fabro-web/app/components/stage-renderers/human-qa.tsx index c2180c0ef..11ed93b28 100644 --- a/apps/fabro-web/app/components/stage-renderers/human-qa.tsx +++ b/apps/fabro-web/app/components/stage-renderers/human-qa.tsx @@ -10,7 +10,10 @@ import { import type { EventEnvelope } from "@qltysh/fabro-api-client"; import type { Stage } from "../stage-sidebar"; -import { ReviewTargetQuestion } from "../review-target-question"; +import { + ReviewTargetQuestion, + safeReviewTarget, +} from "../review-target-question"; import { Tooltip } from "../ui"; import { formatAbsoluteTs, formatDurationMs } from "../../lib/format"; import { ACTIVE_STAGE_STATES } from "../../lib/stage-sidebar"; @@ -149,6 +152,7 @@ function QuestionBlock({ stageActive: boolean; }) { const { question, resolution } = pair; + const reviewTarget = safeReviewTarget(question.reviewTarget); return (
@@ -169,10 +173,9 @@ function QuestionBlock({
- {question.reviewTarget ? ( + {reviewTarget ? ( ) : ( diff --git a/docs/public/reference/dot-language.mdx b/docs/public/reference/dot-language.mdx index a341f1145..d19ccb806 100644 --- a/docs/public/reference/dot-language.mdx +++ b/docs/public/reference/dot-language.mdx @@ -267,7 +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). | +| `review_target` | Boolean | When `true`, read and validate the typed `review_target` context value, then present it as the primary link in the question. Fabro generates the question text, so the node's `label` is not used. 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 diff --git a/docs/public/workflows/human-in-the-loop.mdx b/docs/public/workflows/human-in-the-loop.mdx index bf95362eb..f1cb0bfa7 100644 --- a/docs/public/workflows/human-in-the-loop.mdx +++ b/docs/public/workflows/human-in-the-loop.mdx @@ -99,6 +99,9 @@ Fabro then presents this question: The link uses the target URL from context. The example uses a placeholder secret, not a live Quarry document. +Fabro generates this question text from the target. A `label` on the gate is +not used while `review_target=true`. + The target object has three required fields: | Field | Meaning | diff --git a/lib/apps/fabro-cli/src/commands/run/attach.rs b/lib/apps/fabro-cli/src/commands/run/attach.rs index 122ae8b1f..cd356ffa0 100644 --- a/lib/apps/fabro-cli/src/commands/run/attach.rs +++ b/lib/apps/fabro-cli/src/commands/run/attach.rs @@ -452,13 +452,8 @@ 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() - ); + if let Some(line) = fabro_interview::review_target_line(&question) { + eprintln!("{line}"); } eprintln!("{} {}", styles.bold_cyan.apply_to("?"), question.text); diff --git a/lib/components/fabro-interview/src/console.rs b/lib/components/fabro-interview/src/console.rs index bc4de4c15..b96114765 100644 --- a/lib/components/fabro-interview/src/console.rs +++ b/lib/components/fabro-interview/src/console.rs @@ -111,15 +111,16 @@ fn parse_non_tty_freeform_response(prompt_read: PromptRead) -> Answer { } } -fn review_target_line(question: &Question) -> Option { - question.review_target.as_ref().map(|target| { - format!( - "Review {}: {} — {}", - target.kind().noun(), - target.label(), - target.url() - ) - }) +/// The review target line printed above a question in terminal clients, which +/// cannot render a hyperlink label. The label and resource noun are already in +/// `question.text`, so only the URL is shown. Shared with `fabro-cli`'s attach +/// client. +#[must_use] +pub fn review_target_line(question: &Question) -> Option { + question + .review_target + .as_ref() + .map(|target| format!("Review link: {}", target.url())) } /// Ask a multiple-choice question using dialoguer's `Select` widget on a TTY. @@ -332,26 +333,32 @@ mod tests { } #[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(), - ); + fn review_target_line_shows_only_the_url() { + let target = fabro_types::ReviewTarget::new( + "Quarry review exercise", + "https://quarry.lithos.computer/tmp/0123456789abcdef0123456789abcdef", + fabro_types::ReviewTargetKind::Document, + ) + .unwrap(); + let mut question = Question::new(target.question_text(), QuestionType::MultipleChoice); + question.review_target = Some(target); assert_eq!( review_target_line(&question).as_deref(), Some( - "Review document: Quarry review exercise — \ + "Review link: \ https://quarry.lithos.computer/tmp/0123456789abcdef0123456789abcdef" ) ); } + #[test] + fn review_target_line_is_absent_without_a_target() { + let question = Question::new("Approve?", QuestionType::YesNo); + + assert_eq!(review_target_line(&question), None); + } + #[test] fn find_matching_option_by_key_case_insensitive() { let options = vec![InterviewOption { diff --git a/lib/components/fabro-interview/src/lib.rs b/lib/components/fabro-interview/src/lib.rs index e32f4bd30..4493f98ca 100644 --- a/lib/components/fabro-interview/src/lib.rs +++ b/lib/components/fabro-interview/src/lib.rs @@ -222,7 +222,7 @@ pub trait Interviewer: Send + Sync { // Re-export all implementors at the crate root pub use auto_approve::AutoApproveInterviewer; pub use callback::CallbackInterviewer; -pub use console::ConsoleInterviewer; +pub use console::{ConsoleInterviewer, review_target_line}; pub use control::{ControlInterviewer, SubmitError}; pub use control_protocol::{ WORKER_CONTROL_INVALID_CURSOR_REASON, WORKER_CONTROL_PONG_TIMEOUT_REASON, diff --git a/lib/components/fabro-slack/src/blocks.rs b/lib/components/fabro-slack/src/blocks.rs index 51c60010b..48aa9378f 100644 --- a/lib/components/fabro-slack/src/blocks.rs +++ b/lib/components/fabro-slack/src/blocks.rs @@ -102,11 +102,8 @@ fn header_section(question: &Question, run_web_url: Option<&str>) -> Value { 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() - ) + let link = slack_link(target.url(), target.label()); + format!("*{}*", target.question_text_with_link(&link)) }, ); if !question.stage.is_empty() { diff --git a/lib/foundation/fabro-types/src/interview.rs b/lib/foundation/fabro-types/src/interview.rs index 157fc31c6..729ad6250 100644 --- a/lib/foundation/fabro-types/src/interview.rs +++ b/lib/foundation/fabro-types/src/interview.rs @@ -7,25 +7,15 @@ 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, -)] +/// The type of resource a human should review. `Display` renders the noun used +/// in review question text ("document"). +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, strum::Display)] #[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)] @@ -71,12 +61,21 @@ impl ReviewTarget { self.kind } + /// The question text shown to a human, with the label rendered as plain + /// text. #[must_use] pub fn question_text(&self) -> String { + self.question_text_with_link(&self.label) + } + + /// The same sentence as [`Self::question_text`], with the label replaced by + /// a client-specific rendering of the link (Slack `` syntax, for + /// example). This is the single definition of the review question wording. + #[must_use] + pub fn question_text_with_link(&self, rendered_link: &str) -> String { format!( - "Review the {} {}, then choose the next action.", - self.label, - self.kind.noun() + "Review the {rendered_link} {}, then choose the next action.", + self.kind ) } } @@ -93,7 +92,7 @@ pub enum ReviewTargetError { 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")] + #[error("review target URL must not contain control characters or link delimiters")] UrlContainsUnsafeCharacters, #[error("review target URL must be a valid absolute URL")] InvalidUrl, @@ -154,8 +153,10 @@ impl<'de> Deserialize<'de> for ReviewTarget { where D: serde::Deserializer<'de>, { + // Unknown fields are ignored so the OpenAPI contract (which leaves + // `additionalProperties` permissive) and this deserializer agree, and + // so an unknown key in a persisted event cannot fail the whole event. #[derive(Deserialize)] - #[serde(deny_unknown_fields)] struct WireReviewTarget { label: String, url: String, diff --git a/lib/foundation/fabro-types/src/run_event/misc.rs b/lib/foundation/fabro-types/src/run_event/misc.rs index ad8b1b4a5..13612c677 100644 --- a/lib/foundation/fabro-types/src/run_event/misc.rs +++ b/lib/foundation/fabro-types/src/run_event/misc.rs @@ -1,7 +1,9 @@ use serde::{Deserialize, Serialize}; use super::ExecOutputTail; -use crate::{CommandTermination, ParallelBranchResult, PullRequestLink, StageId, StageOutcome}; +use crate::{ + CommandTermination, ParallelBranchResult, PullRequestLink, ReviewTarget, StageId, StageOutcome, +}; #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, Default)] pub struct InterviewOption { @@ -66,7 +68,7 @@ pub struct InterviewStartedProps { #[serde(default, skip_serializing_if = "Option::is_none")] pub context_display: Option, #[serde(default, skip_serializing_if = "Option::is_none")] - pub review_target: Option, + pub review_target: Option, } #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]