fix: close Task 5 contract graph review findings

This commit is contained in:
lda
2026-08-14 18:34:44 +07:00 Verified
parent 50a462f927
commit eae1a62fb2
8 changed files with 165 additions and 63 deletions
@@ -41,6 +41,10 @@ const options: ReadonlyArray<AuthoringPathOption> = [
availability: "conditional", availability: "conditional",
reason: "Available when the selected step runs in a viewer frame.", reason: "Available when the selected step runs in a viewer frame.",
}), }),
option("context.viewer_email", "Viewer email", "runtime_context", ["workflow_output"], {
availability: "conditional",
reason: "Available only in viewer frames.",
}),
option("output.report", "Final report", "workflow_output", ["workflow_output"]), option("output.report", "Final report", "workflow_output", ["workflow_output"]),
option("input.customer.name", "Customer name", "workflow_input", ["step_input"]), option("input.customer.name", "Customer name", "workflow_input", ["step_input"]),
]; ];
@@ -133,6 +137,55 @@ describe("AuthoringPathPicker", () => {
expect(onChange).toHaveBeenCalledWith("input.title"); expect(onChange).toHaveBeenCalledWith("input.title");
}); });
it("gives a conditional incompatible option one composed description node", () => {
render(
<AuthoringPathPicker
label="Source path"
onChange={vi.fn()}
options={options}
uses="step_input"
value=""
/>,
);
const optionButton = screen.getByRole("button", { name: /Viewer email/ });
const describedBy = optionButton.getAttribute("aria-describedby");
expect(optionButton).toBeDisabled();
expect(describedBy).not.toBeNull();
expect(describedBy?.trim().split(/\s+/)).toHaveLength(1);
expect(document.querySelectorAll(`#${describedBy}`).length).toBe(1);
expect(document.getElementById(describedBy ?? "")?.textContent).toContain(
"Available only in viewer frames.",
);
expect(document.getElementById(describedBy ?? "")?.textContent).toContain(
"Not available for this field.",
);
});
it("renders a supplied non-conditional reason before describing the option", () => {
render(
<AuthoringPathPicker
label="Source path"
onChange={vi.fn()}
options={[
option("input.reasoned", "Reasoned", "workflow_input", ["step_input"], {
reason: "Retained for repair context.",
}),
]}
uses="step_input"
value=""
/>,
);
const optionButton = screen.getByRole("button", { name: /Reasoned/ });
const describedBy = optionButton.getAttribute("aria-describedby");
expect(describedBy).not.toBeNull();
expect(document.getElementById(describedBy ?? "")?.textContent).toBe(
"Retained for repair context.",
);
});
it("keeps nested choices keyboard reachable and preserves normal choices in Advanced mode", async () => { it("keeps nested choices keyboard reachable and preserves normal choices in Advanced mode", async () => {
const user = userEvent.setup(); const user = userEvent.setup();
const onChange = vi.fn(); const onChange = vi.fn();
@@ -80,14 +80,19 @@ export const AuthoringPathPicker = ({
<fieldset className="authoring-path-picker__group" key={group.origin}> <fieldset className="authoring-path-picker__group" key={group.origin}>
<legend>{group.label}</legend> <legend>{group.label}</legend>
<div className="authoring-path-picker__list"> <div className="authoring-path-picker__list">
{groupOptions.map((option) => { {groupOptions.map((option, optionIndex) => {
const compatible = option.uses.some((use) => requestedUses.has(use)); const compatible = option.uses.some((use) => requestedUses.has(use));
const reasonId = `${id}-${safeId(option.path)}-reason`; const reasonId = `${id}-${safeId(group.origin)}-${optionIndex}-description`;
const availabilityReason = option.availability === "conditional"
? option.reason ?? "Conditionally available; verify this path at runtime."
: option.reason;
const description = [
availabilityReason,
...(compatible ? [] : ["Not available for this field."]),
].filter((part): part is string => part !== undefined).join(" ");
return ( return (
<button <button
aria-describedby={ aria-describedby={description === "" ? undefined : reasonId}
option.reason !== undefined || !compatible ? reasonId : undefined
}
aria-pressed={option.path === value} aria-pressed={option.path === value}
className="authoring-path-picker__option" className="authoring-path-picker__option"
disabled={!compatible} disabled={!compatible}
@@ -102,10 +107,7 @@ export const AuthoringPathPicker = ({
<code>{option.path}</code> <code>{option.path}</code>
{option.description !== undefined && <span>{option.description}</span>} {option.description !== undefined && <span>{option.description}</span>}
{option.required && <small>Required</small>} {option.required && <small>Required</small>}
{option.availability === "conditional" && option.reason !== undefined && ( {description !== "" && <small id={reasonId}>{description}</small>}
<small id={reasonId}>{option.reason}</small>
)}
{!compatible && <small id={reasonId}>Not available for this field.</small>}
</button> </button>
); );
})} })}
@@ -125,6 +125,19 @@ describe("useAuthoringContract", () => {
expect(result.current.inventory?.selectedStepId).toBeNull(); expect(result.current.inventory?.selectedStepId).toBeNull();
}); });
it("surfaces an initial inspection failure instead of staying loading", async () => {
client.inspect.mockRejectedValue(new Error("initial inspection failed"));
const { result } = renderHook(() =>
useAuthoringContract({ workspaceId: "draft-report", revision: 7, selectedStepId: null }),
);
await waitFor(() => expect(result.current.phase).toBe("error"));
expect(result.current.inventory).toBeNull();
expect(result.current.message).toBe("initial inspection failed");
});
it("passes the executable step id and ignores a stale selection response", async () => { it("passes the executable step id and ignores a stale selection response", async () => {
const first = deferred<AuthoringContractInventory>(); const first = deferred<AuthoringContractInventory>();
const second = deferred<AuthoringContractInventory>(); const second = deferred<AuthoringContractInventory>();
@@ -42,12 +42,14 @@ type StoredInventory = {
type AuthoringContractState = { type AuthoringContractState = {
readonly phase: AuthoringContractPhase; readonly phase: AuthoringContractPhase;
readonly attempted: RequestIdentity | null;
readonly stored: StoredInventory | null; readonly stored: StoredInventory | null;
readonly message: string | null; readonly message: string | null;
}; };
const initialState: AuthoringContractState = { const initialState: AuthoringContractState = {
phase: "disconnected", phase: "disconnected",
attempted: null,
stored: null, stored: null,
message: null, message: null,
}; };
@@ -98,6 +100,7 @@ export const useAuthoringContract = ({
const requested = request; const requested = request;
setState((current) => ({ setState((current) => ({
phase: "loading", phase: "loading",
attempted: requested,
stored: stored:
current.stored !== null && sameRequest(current.stored.request, requested) current.stored !== null && sameRequest(current.stored.request, requested)
? current.stored ? current.stored
@@ -115,6 +118,7 @@ export const useAuthoringContract = ({
if (generation !== generationRef.current) return; if (generation !== generationRef.current) return;
setState({ setState({
phase: "ready", phase: "ready",
attempted: requested,
stored: { request: requested, inventory }, stored: { request: requested, inventory },
message: null, message: null,
}); });
@@ -134,6 +138,7 @@ export const useAuthoringContract = ({
generationRef.current++; generationRef.current++;
setState({ setState({
phase: client === null || connectedTarget === null ? "disconnected" : "idle", phase: client === null || connectedTarget === null ? "disconnected" : "idle",
attempted: null,
stored: null, stored: null,
message: null, message: null,
}); });
@@ -150,20 +155,28 @@ export const useAuthoringContract = ({
request !== null && state.stored !== null && sameRequest(state.stored.request, request) request !== null && state.stored !== null && sameRequest(state.stored.request, request)
? state.stored.inventory ? state.stored.inventory
: null; : null;
const phase = const hasCurrentAttempt =
request !== null && request !== null &&
(state.stored === null || !sameRequest(state.stored.request, request)) state.attempted !== null &&
? "loading" sameRequest(state.attempted, request);
: request === null const phase = request === null
? client === null || connectedTarget === null ? client === null || connectedTarget === null
? "disconnected" ? "disconnected"
: "idle" : "idle"
: state.phase; : hasCurrentAttempt && state.phase === "error"
? "error"
: state.stored !== null && sameRequest(state.stored.request, request)
? state.phase
: "loading";
return { return {
phase, phase,
inventory: currentInventory, inventory: currentInventory,
message: currentInventory === null && phase === "loading" ? null : state.message, message: hasCurrentAttempt && state.phase === "error"
? state.message
: currentInventory === null && phase === "loading"
? null
: state.message,
refresh, refresh,
}; };
}; };
@@ -43,7 +43,7 @@ const runWith = (
describe("AuthoringContractClient", () => { describe("AuthoringContractClient", () => {
it("sends the exact inspection params and decodes the response", async () => { it("sends the exact inspection params and decodes the response", async () => {
const { executor, calls } = runWith(wireInventory); const { executor, calls } = runWith({ ...wireInventory, selected_step_id: "render" });
const client = createAuthoringContractClient(executor); const client = createAuthoringContractClient(executor);
const result = await client.inspect({ const result = await client.inspect({
@@ -80,6 +80,7 @@ describe("AuthoringContractClient", () => {
it.each([ it.each([
["workspaceId", { workspace_id: "other" }], ["workspaceId", { workspace_id: "other" }],
["revision", { revision: 8 }], ["revision", { revision: 8 }],
["selectedStepId", { selected_step_id: "render" }],
])("rejects a response with a mismatched %s", async (_field, replacement) => { ])("rejects a response with a mismatched %s", async (_field, replacement) => {
const { executor } = runWith({ ...wireInventory, ...replacement }); const { executor } = runWith({ ...wireInventory, ...replacement });
const client = createAuthoringContractClient(executor); const client = createAuthoringContractClient(executor);
@@ -88,4 +89,13 @@ describe("AuthoringContractClient", () => {
client.inspect({ workspaceId: "draft-report", revision: 7, selectedStepId: null }), client.inspect({ workspaceId: "draft-report", revision: 7, selectedStepId: null }),
).rejects.toThrow("does not match inspection request"); ).rejects.toThrow("does not match inspection request");
}); });
it("rejects a response for a different executable step", async () => {
const { executor } = runWith({ ...wireInventory, selected_step_id: "other" });
const client = createAuthoringContractClient(executor);
await expect(
client.inspect({ workspaceId: "draft-report", revision: 7, selectedStepId: "render" }),
).rejects.toThrow("does not match inspection request");
});
}); });
@@ -42,7 +42,8 @@ export const createAuthoringContractClient = (
); );
if ( if (
inventory.workspaceId !== workspaceId || inventory.workspaceId !== workspaceId ||
inventory.revision !== input.revision inventory.revision !== input.revision ||
(inventory.selectedStepId?.trim() || null) !== selectedStepId
) { ) {
throw new Error("authoring contract response does not match inspection request"); throw new Error("authoring contract response does not match inspection request");
} }
@@ -96,11 +96,23 @@ describe("authoring contract models", () => {
}); });
it("rejects a malformed inventory envelope", () => { it("rejects a malformed inventory envelope", () => {
expect(() => decodeAuthoringContractInventory({ ...inventory, revision: "7" })).toThrow( for (const revision of [0, -1, 1.5, "7"]) {
expect(() => decodeAuthoringContractInventory({ ...inventory, revision })).toThrow(
"AuthoringContractInventory is malformed",
);
}
expect(() => decodeAuthoringContractInventory({ ...inventory, selected_step_id: "" })).toThrow(
"AuthoringContractInventory is malformed", "AuthoringContractInventory is malformed",
); );
}); });
it("requires a reason for conditional options", () => {
expect(() => decodeAuthoringContractInventory({
...inventory,
readable_sources: [{ ...pathOption, availability: "conditional", reason: undefined }],
})).toThrow("AuthoringContractInventory is malformed");
});
const _typeCheck: AuthoringContractInventory | null = null; const _typeCheck: AuthoringContractInventory | null = null;
void _typeCheck; void _typeCheck;
}); });
@@ -52,6 +52,8 @@ export type AuthoringContractInventory = {
}; };
const JsonObjectSchema = v.record(v.string(), v.unknown()); const JsonObjectSchema = v.record(v.string(), v.unknown());
const PositiveRevisionSchema = v.pipe(v.number(), v.integer(), v.minValue(1));
const SelectedStepIdSchema = v.nullable(v.pipe(v.string(), v.minLength(1)));
const AuthoringPathOriginSchema = v.union([ const AuthoringPathOriginSchema = v.union([
v.literal("workflow_input"), v.literal("workflow_input"),
@@ -74,77 +76,73 @@ const AuthoringPathUseSchema = v.union([
v.literal("workflow_output"), v.literal("workflow_output"),
]); ]);
const AuthoringPathOptionWireSchema = v.object({ const AuthoringPathOptionSchema = v.pipe(
path: v.string(), v.object({
label: v.string(), path: v.string(),
origin: AuthoringPathOriginSchema, label: v.string(),
schema: JsonObjectSchema, origin: AuthoringPathOriginSchema,
required: v.boolean(), schema: JsonObjectSchema,
availability: AuthoringPathAvailabilitySchema, required: v.boolean(),
uses: v.array(AuthoringPathUseSchema), availability: AuthoringPathAvailabilitySchema,
description: v.optional(v.string()), uses: v.array(AuthoringPathUseSchema),
reason: v.optional(v.string()), description: v.optional(v.string()),
}); reason: v.optional(v.string()),
}),
v.check(
(option) =>
option.availability !== "conditional" ||
(option.reason !== undefined && option.reason.trim().length > 0),
"conditional authoring options require a non-empty reason",
),
);
const AuthoringStepContractWireSchema = v.object({ const AuthoringStepContractWireSchema = v.object({
step_id: v.string(), step_id: v.string(),
label: v.string(), label: v.string(),
description: v.optional(v.string()), description: v.optional(v.string()),
input_targets: v.optional(v.array(AuthoringPathOptionWireSchema)), input_targets: v.optional(v.array(AuthoringPathOptionSchema)),
output_sources: v.optional(v.array(AuthoringPathOptionWireSchema)), output_sources: v.optional(v.array(AuthoringPathOptionSchema)),
outcomes: v.optional(v.array(v.string())), outcomes: v.optional(v.array(v.string())),
}); });
const AuthoringContractInventoryWireSchema = v.object({ const AuthoringContractInventoryWireSchema = v.object({
workspace_id: v.string(), workspace_id: v.string(),
revision: v.number(), revision: PositiveRevisionSchema,
selected_step_id: v.nullable(v.string()), selected_step_id: SelectedStepIdSchema,
readable_sources: v.array(AuthoringPathOptionWireSchema), readable_sources: v.array(AuthoringPathOptionSchema),
step_input_targets: v.array(AuthoringPathOptionWireSchema), step_input_targets: v.array(AuthoringPathOptionSchema),
step_output_sources: v.array(AuthoringPathOptionWireSchema), step_output_sources: v.array(AuthoringPathOptionSchema),
state_targets: v.array(AuthoringPathOptionWireSchema), state_targets: v.array(AuthoringPathOptionSchema),
workflow_output_targets: v.array(AuthoringPathOptionWireSchema), workflow_output_targets: v.array(AuthoringPathOptionSchema),
entry_steps: v.array(AuthoringStepContractWireSchema), entry_steps: v.array(AuthoringStepContractWireSchema),
workflow_outcomes: v.array(v.string()), workflow_outcomes: v.array(v.string()),
warnings: v.array(v.string()), warnings: v.array(v.string()),
}); });
const AuthoringPathOptionBrowserSchema = v.object({
path: v.string(),
label: v.string(),
origin: AuthoringPathOriginSchema,
schema: JsonObjectSchema,
required: v.boolean(),
availability: AuthoringPathAvailabilitySchema,
uses: v.array(AuthoringPathUseSchema),
description: v.optional(v.string()),
reason: v.optional(v.string()),
});
const AuthoringStepContractBrowserSchema = v.object({ const AuthoringStepContractBrowserSchema = v.object({
stepId: v.string(), stepId: v.string(),
label: v.string(), label: v.string(),
description: v.optional(v.string()), description: v.optional(v.string()),
inputTargets: v.optional(v.array(AuthoringPathOptionBrowserSchema)), inputTargets: v.optional(v.array(AuthoringPathOptionSchema)),
outputSources: v.optional(v.array(AuthoringPathOptionBrowserSchema)), outputSources: v.optional(v.array(AuthoringPathOptionSchema)),
outcomes: v.optional(v.array(v.string())), outcomes: v.optional(v.array(v.string())),
}); });
const AuthoringContractInventoryBrowserSchema = v.object({ const AuthoringContractInventoryBrowserSchema = v.object({
workspaceId: v.string(), workspaceId: v.string(),
revision: v.number(), revision: PositiveRevisionSchema,
selectedStepId: v.nullable(v.string()), selectedStepId: SelectedStepIdSchema,
readableSources: v.array(AuthoringPathOptionBrowserSchema), readableSources: v.array(AuthoringPathOptionSchema),
stepInputTargets: v.array(AuthoringPathOptionBrowserSchema), stepInputTargets: v.array(AuthoringPathOptionSchema),
stepOutputSources: v.array(AuthoringPathOptionBrowserSchema), stepOutputSources: v.array(AuthoringPathOptionSchema),
stateTargets: v.array(AuthoringPathOptionBrowserSchema), stateTargets: v.array(AuthoringPathOptionSchema),
workflowOutputTargets: v.array(AuthoringPathOptionBrowserSchema), workflowOutputTargets: v.array(AuthoringPathOptionSchema),
entrySteps: v.array(AuthoringStepContractBrowserSchema), entrySteps: v.array(AuthoringStepContractBrowserSchema),
workflowOutcomes: v.array(v.string()), workflowOutcomes: v.array(v.string()),
warnings: v.array(v.string()), warnings: v.array(v.string()),
}); });
type AuthoringPathOptionWire = v.InferOutput<typeof AuthoringPathOptionWireSchema>; type AuthoringPathOptionWire = v.InferOutput<typeof AuthoringPathOptionSchema>;
type AuthoringStepContractWire = v.InferOutput<typeof AuthoringStepContractWireSchema>; type AuthoringStepContractWire = v.InferOutput<typeof AuthoringStepContractWireSchema>;
type AuthoringContractInventoryWire = v.InferOutput< type AuthoringContractInventoryWire = v.InferOutput<
typeof AuthoringContractInventoryWireSchema typeof AuthoringContractInventoryWireSchema