diff --git a/.gitea/scripts/status-reaper.py b/.gitea/scripts/status-reaper.py index dea13812..41cc8c1f 100644 --- a/.gitea/scripts/status-reaper.py +++ b/.gitea/scripts/status-reaper.py @@ -19,18 +19,34 @@ What this script does, per `.gitea/workflows/status-reaper.yml` invocation: downstream — Gitea uses ` / ` as the workflow/job separator). Classify each by whether `on:` contains a `push:` trigger. - 2. GET combined status for HEAD of WATCH_BRANCH. + 2. List the last N (=10) commits on WATCH_BRANCH via + GET /repos/{o}/{r}/commits?sha={branch}&limit={N}. rev2 sweeps + N commits per tick instead of HEAD only — schedule workflows + post `failure` to whatever SHA was HEAD when they COMPLETED, so + by the next */5 tick main has often moved forward and the red + gets stranded on a stale commit (Phase 1+2 evidence: rev1 saw + `compensated:0` every tick across ~6 cycles). - 3. For each per-context status entry where: - state == "failure" AND context.endswith(" (push)") - Parse context as ` / (push)`. Look up - workflow_name in the trigger map: - - missing → log ::notice:: and skip (conservative). - - has_push_trigger=True → preserve (would mask real signal). - - has_push_trigger=False → POST a compensating - `state=success` status to /statuses/{sha} with the same - context (Gitea de-dups by context) and a description that - documents the workaround + this script's path. + 3. For EACH SHA in the list: + - GET combined commit status. Per-SHA error isolation + (refinement #7): if this call raises ApiError or any 5xx, + LOG `::warning::` + continue to the next SHA. Different from + the single-HEAD pre-rev2 path where fail-loud was correct; + the sweep is best-effort across historical commits, so one + transient blip on a stale SHA must not strand reds on the + OTHER stale SHAs. + - If combined.state == "success": skip — cost optimization + (refinement #2), common case (most commits are green). + - Otherwise iterate per-context entries. For each entry where: + state == "failure" AND context.endswith(" (push)") + Parse context as ` / (push)`. + Look up workflow_name in the trigger map: + - missing → log ::notice:: and skip (conservative). + - has_push_trigger=True → preserve (real defect signal). + - has_push_trigger=False → POST a compensating + `state=success` status to /statuses/{sha} with the same + context (Gitea de-dups by context) and a description + documenting the workaround + this script's path. 4. Exit 0. Re-running is idempotent — Gitea's commit-status table stores the LATEST state-per-context, so the success POST sticks @@ -401,21 +417,29 @@ def reap( sha: str, *, dry_run: bool = False, -) -> dict[str, int]: +) -> dict[str, Any]: """Walk `combined.statuses[]` and compensate where appropriate. + Per-SHA worker. The multi-SHA orchestrator (`reap_branch`) calls + this once per stale main commit each tick. + Returns counters for observability: {compensated, preserved_real_push, preserved_unknown, preserved_non_failure, preserved_non_push_suffix, - preserved_unparseable} + preserved_unparseable, + compensated_contexts: [, ...]} + + `compensated_contexts` is rev2-added so `reap_branch` can build + `compensated_per_sha` without re-deriving it from the POST stream. """ - counters = { + counters: dict[str, Any] = { "compensated": 0, "preserved_real_push": 0, "preserved_unknown": 0, "preserved_non_failure": 0, "preserved_non_push_suffix": 0, "preserved_unparseable": 0, + "compensated_contexts": [], } statuses = combined.get("statuses") or [] @@ -464,10 +488,136 @@ def reap( sha, context, s.get("target_url"), dry_run=dry_run ) counters["compensated"] += 1 + counters["compensated_contexts"].append(context) return counters +# -------------------------------------------------------------------------- +# rev2: multi-SHA sweep over the last N commits on WATCH_BRANCH +# -------------------------------------------------------------------------- +# How many main commits to sweep per tick. Sized to cover a burst-merge +# window where multiple PRs land in the 5-min interval between reaper +# ticks. Older reds falling off the window is acceptable — they were +# already stale enough that the schedule-run that posted them has long +# since been overwritten by a real push trigger. See `reference_post_ +# suspension_pipeline` for the merge-cadence baseline. +DEFAULT_SWEEP_LIMIT = 10 + + +def list_recent_commit_shas(branch: str, limit: int) -> list[str]: + """List the most recent `limit` commit SHAs on `branch`, newest + first. + + Wraps GET /repos/{o}/{r}/commits?sha={branch}&limit={limit}. Gitea + 1.22.6 returns a JSON list of commit objects each with a `sha` key + (verified via vendor-truth probe 2026-05-11 against + git.moleculesai.app — `feedback_smoke_test_vendor_truth_not_shape_match`). + + Raises ApiError on non-2xx OR on unexpected response shape. This is + a HARD halt — without the commit list the sweep can't proceed. (The + per-SHA error isolation downstream is a different concern: tolerating + a transient 5xx on ONE commit's status is best-effort; losing the + commit list itself means we don't even know which commits to try.) + """ + _, body = api( + "GET", + f"/repos/{OWNER}/{NAME}/commits", + query={"sha": branch, "limit": str(limit)}, + ) + if not isinstance(body, list): + raise ApiError( + f"commits listing for {branch} not a JSON array " + f"(got {type(body).__name__})" + ) + shas: list[str] = [] + for entry in body: + if not isinstance(entry, dict): + continue + sha = entry.get("sha") + if isinstance(sha, str) and len(sha) >= 7: + shas.append(sha) + if not shas: + raise ApiError( + f"commits listing for {branch} returned no usable SHAs" + ) + return shas + + +def reap_branch( + workflow_trigger_map: dict[str, bool], + branch: str, + *, + limit: int = DEFAULT_SWEEP_LIMIT, + dry_run: bool = False, +) -> dict[str, Any]: + """Sweep the last `limit` commits on `branch`, applying `reap()` + to each (with per-SHA error isolation). + + Returns aggregated counters PLUS rev2 observability fields: + - scanned_shas: how many SHAs we actually iterated + - compensated_per_sha: {: [, ...]} — only + SHAs that actually got at least one compensation are included + """ + shas = list_recent_commit_shas(branch, limit) + + aggregate: dict[str, Any] = { + "scanned_shas": 0, + "compensated": 0, + "preserved_real_push": 0, + "preserved_unknown": 0, + "preserved_non_failure": 0, + "preserved_non_push_suffix": 0, + "preserved_unparseable": 0, + "compensated_per_sha": {}, + } + + for sha in shas: + aggregate["scanned_shas"] += 1 + + # Per-SHA error isolation (refinement #7). One transient blip + # on a historical commit must NOT abort the whole tick — the + # OTHER stale SHAs may still hold strandable reds. + try: + combined = get_combined_status(sha) + except ApiError as e: + print( + f"::warning::get_combined_status({sha[:10]}) failed; " + f"skipping this SHA: {e}" + ) + continue + + # Cost optimization (refinement #2): the common case is a green + # commit. Skip the per-context loop entirely when combined is + # already success — saves a tight loop over ~20 statuses per SHA + # on green commits, the dominant majority. + if combined.get("state") == "success": + continue + + per_sha = reap( + workflow_trigger_map, combined, sha, dry_run=dry_run + ) + + # Aggregate scalar counters. + for key in ( + "compensated", + "preserved_real_push", + "preserved_unknown", + "preserved_non_failure", + "preserved_non_push_suffix", + "preserved_unparseable", + ): + aggregate[key] += per_sha[key] + + # Record per-SHA compensated contexts (only when non-empty — + # keep the summary readable when most SHAs are no-ops). + contexts = per_sha.get("compensated_contexts") or [] + if contexts: + aggregate["compensated_per_sha"][sha] = list(contexts) + + return aggregate + + def main() -> int: parser = argparse.ArgumentParser(description=__doc__) parser.add_argument( @@ -475,6 +625,15 @@ def main() -> int: action="store_true", help="Skip the compensating POST; print what would be done.", ) + parser.add_argument( + "--limit", + type=int, + default=DEFAULT_SWEEP_LIMIT, + help=( + "How many recent commits on WATCH_BRANCH to sweep per tick " + f"(default: {DEFAULT_SWEEP_LIMIT})." + ), + ) args = parser.parse_args() _require_runtime_env() @@ -486,11 +645,11 @@ def main() -> int: f"class-O candidates={sum(1 for v in workflow_trigger_map.values() if not v)}" ) - sha = get_head_sha(WATCH_BRANCH) - combined = get_combined_status(sha) - - counters = reap( - workflow_trigger_map, combined, sha, dry_run=args.dry_run + counters = reap_branch( + workflow_trigger_map, + WATCH_BRANCH, + limit=args.limit, + dry_run=args.dry_run, ) # Observability: print one JSON line summarising the tick. Loki @@ -499,9 +658,9 @@ def main() -> int: "status-reaper summary: " + json.dumps( { - "sha": sha, "branch": WATCH_BRANCH, "dry_run": args.dry_run, + "limit": args.limit, **counters, }, sort_keys=True, diff --git a/canvas/src/components/tabs/DetailsTab.tsx b/canvas/src/components/tabs/DetailsTab.tsx index 8d659797..36d57850 100644 --- a/canvas/src/components/tabs/DetailsTab.tsx +++ b/canvas/src/components/tabs/DetailsTab.tsx @@ -402,7 +402,7 @@ function Row({ label, value, mono }: { label: string; value: string; mono?: bool ); } -function getSkills(card: Record | null): { id: string; description?: string }[] { +export function getSkills(card: Record | null): { id: string; description?: string }[] { if (!card) return []; const skills = card.skills; if (!Array.isArray(skills)) return []; diff --git a/canvas/src/components/tabs/SkillsTab.tsx b/canvas/src/components/tabs/SkillsTab.tsx index 563aff58..60097625 100644 --- a/canvas/src/components/tabs/SkillsTab.tsx +++ b/canvas/src/components/tabs/SkillsTab.tsx @@ -647,7 +647,7 @@ export function SkillsTab({ workspaceId, data }: Props) { ); } -function extractSkills(agentCard: Record | null): SkillEntry[] { +export function extractSkills(agentCard: Record | null): SkillEntry[] { if (!agentCard) return []; const rawSkills = agentCard.skills; if (!Array.isArray(rawSkills)) return []; diff --git a/canvas/src/components/tabs/__tests__/extractSkills.test.ts b/canvas/src/components/tabs/__tests__/extractSkills.test.ts new file mode 100644 index 00000000..3e9d203d --- /dev/null +++ b/canvas/src/components/tabs/__tests__/extractSkills.test.ts @@ -0,0 +1,140 @@ +// @vitest-environment jsdom +/** + * Unit tests for extractSkills — pure helper from SkillsTab. + * + * Covers: null card, non-array skills, empty skills, full skill entries + * (id, name, description, tags, examples), id-only fallback, name-only + * fallback, string coercion, array coercion for tags/examples, + * filtering entries with no id after coercion, empty string id (filtered). + */ +import { describe, it, expect } from "vitest"; +import { extractSkills } from "../SkillsTab"; + +describe("extractSkills", () => { + it("returns [] for null card", () => { + expect(extractSkills(null)).toEqual([]); + }); + + it("returns [] when card.skills is not an array", () => { + expect(extractSkills({ skills: undefined })).toEqual([]); + expect(extractSkills({ skills: "not-an-array" })).toEqual([]); + expect(extractSkills({ skills: { id: "x" } })).toEqual([]); + }); + + it("returns [] for empty skills array", () => { + expect(extractSkills({ skills: [] })).toEqual([]); + }); + + it("maps a fully-populated skill entry", () => { + const card = { + skills: [ + { + id: "code_search", + name: "Code Search", + description: "Semantic code search", + tags: ["search", "code"], + examples: ["Find unused exports", "Search by AST pattern"], + }, + ], + }; + expect(extractSkills(card)).toEqual([ + { + id: "code_search", + name: "Code Search", + description: "Semantic code search", + tags: ["search", "code"], + examples: ["Find unused exports", "Search by AST pattern"], + }, + ]); + }); + + it("uses name as id when id is absent", () => { + const card = { skills: [{ name: "web_scraper" }] }; + expect(extractSkills(card)).toEqual([ + { id: "web_scraper", name: "web_scraper", description: "", tags: [], examples: [] }, + ]); + }); + + it("uses id as name when name is absent", () => { + const card = { skills: [{ id: "legacy_skill" }] }; + expect(extractSkills(card)).toEqual([ + { id: "legacy_skill", name: "legacy_skill", description: "", tags: [], examples: [] }, + ]); + }); + + it("filters out entries with neither id nor name", () => { + // id: String(undefined || undefined || "") → "" → filtered (id.length = 0) + const card = { skills: [{ description: "orphan entry" }] }; + expect(extractSkills(card)).toEqual([]); + }); + + it("filters out entries with no id after string coercion", () => { + // id resolves to "" after String(undefined || null || {}) + const card = { skills: [{ id: null, name: null }] }; + expect(extractSkills(card)).toEqual([]); + }); + + it("filters out entries with empty-string id", () => { + const card = { skills: [{ id: "", name: "" }] }; + expect(extractSkills(card)).toEqual([]); + }); + + it("coerces numeric tags to strings", () => { + const card = { skills: [{ id: "x", tags: [1, "two", 3] }] }; + expect(extractSkills(card)).toEqual([ + { id: "x", name: "x", description: "", tags: ["1", "two", "3"], examples: [] }, + ]); + }); + + it("coerces non-array tags to empty array", () => { + const card = { skills: [{ id: "x", tags: "not-an-array" }] }; + expect(extractSkills(card)).toEqual([ + { id: "x", name: "x", description: "", tags: [], examples: [] }, + ]); + }); + + it("coerces non-array examples to empty array", () => { + const card = { skills: [{ id: "x", examples: 42 }] }; + expect(extractSkills(card)).toEqual([ + { id: "x", name: "x", description: "", tags: [], examples: [] }, + ]); + }); + + // NOTE: extractSkills uses `String(skill.description || "")` — falsy values + // (0, null, false) fall through to "", NOT to their string form. + it("returns '' for falsy description values (0, null, false)", () => { + const card = { skills: [{ id: "x", description: 0 }] }; + expect(extractSkills(card)).toEqual([ + { id: "x", name: "x", description: "", tags: [], examples: [] }, + ]); + }); + + it("handles mixed valid/invalid entries", () => { + const card = { + skills: [ + { id: "valid_one", name: "One" }, + { name: "named_only" }, + { description: "orphan" }, // filtered — id becomes "" + { id: "valid_two", examples: ["a", "b"] }, + ], + }; + expect(extractSkills(card)).toEqual([ + { id: "valid_one", name: "One", description: "", tags: [], examples: [] }, + { id: "named_only", name: "named_only", description: "", tags: [], examples: [] }, + { id: "valid_two", name: "valid_two", description: "", tags: [], examples: ["a", "b"] }, + ]); + }); + + it("handles a realistic agent card with multiple skills", () => { + const card = { + skills: [ + { id: "web_search", name: "Web Search", description: "Search the web", tags: ["search"], examples: ["Latest news"] }, + { id: "file_read", name: "Read Files", description: "Read from disk", tags: ["io"], examples: [] }, + ], + }; + const result = extractSkills(card); + expect(result).toHaveLength(2); + expect(result[0].id).toBe("web_search"); + expect(result[1].tags).toEqual(["io"]); + }); +}); diff --git a/canvas/src/components/tabs/__tests__/getSkills.test.ts b/canvas/src/components/tabs/__tests__/getSkills.test.ts new file mode 100644 index 00000000..8b27b2bf --- /dev/null +++ b/canvas/src/components/tabs/__tests__/getSkills.test.ts @@ -0,0 +1,95 @@ +// @vitest-environment jsdom +/** + * Unit tests for getSkills — pure helper from DetailsTab. + * + * Covers: null card, non-array skills, empty skills, id-only entries, + * name-only entries (id derives from name), entries with description, + * entries with neither id nor name (filtered out), mixed entries. + */ +import { describe, it, expect } from "vitest"; +import { getSkills } from "../DetailsTab"; + +describe("getSkills", () => { + it("returns [] for null card", () => { + expect(getSkills(null)).toEqual([]); + }); + + it("returns [] when card.skills is not an array", () => { + expect(getSkills({ skills: undefined })).toEqual([]); + expect(getSkills({ skills: "not-an-array" })).toEqual([]); + expect(getSkills({ skills: { id: "x" } })).toEqual([]); + }); + + it("returns [] for empty skills array", () => { + expect(getSkills({ skills: [] })).toEqual([]); + }); + + it("maps skill with id and description", () => { + const card = { skills: [{ id: "code_search", description: "Find code patterns" }] }; + expect(getSkills(card)).toEqual([{ id: "code_search", description: "Find code patterns" }]); + }); + + it("maps skill with id only (description absent)", () => { + const card = { skills: [{ id: "code_search" }] }; + expect(getSkills(card)).toEqual([{ id: "code_search", description: undefined }]); + }); + + it("derives id from name when id is absent", () => { + const card = { skills: [{ name: "web_scraper" }] }; + expect(getSkills(card)).toEqual([{ id: "web_scraper" }]); + }); + + it("maps description when present", () => { + const card = { skills: [{ id: "file_write", description: "Writes files to disk" }] }; + expect(getSkills(card)).toEqual([{ id: "file_write", description: "Writes files to disk" }]); + }); + + it("returns description as undefined when skill has no description", () => { + const card = { skills: [{ id: "noop_skill" }] }; + const result = getSkills(card); + // The map always includes description; it's undefined when absent + expect(result).toEqual([{ id: "noop_skill", description: undefined }]); + }); + + it("filters out skills with neither id nor name", () => { + // id: String(undefined || undefined || "") → "" → filtered + const card = { skills: [{ description: "loner" }] }; + expect(getSkills(card)).toEqual([]); + }); + + it("handles mixed valid/invalid entries", () => { + const card = { + skills: [ + { id: "valid_one" }, + { name: "named_skill" }, + { description: "orphaned" }, // filtered + { id: "valid_two", description: "Has both" }, + ], + }; + expect(getSkills(card)).toEqual([ + { id: "valid_one", description: undefined }, + { id: "named_skill", description: undefined }, + { id: "valid_two", description: "Has both" }, + ]); + }); + + it("handles string coercion for numeric ids/names", () => { + const card = { skills: [{ id: 42, name: "numeric_id" }] }; + expect(getSkills(card)).toEqual([{ id: "42" }]); + }); + + it("uses id over name when both are present", () => { + const card = { skills: [{ id: "priority_id", name: "fallback_name" }] }; + expect(getSkills(card)).toEqual([{ id: "priority_id", description: undefined }]); + }); + + it("omits description when it is falsy (0 is falsy in JS)", () => { + // The implementation uses `s.description ?` — 0 is falsy, so it's treated + // as absent and undefined is returned. Non-zero numbers coerce fine. + const cardZero = { skills: [{ id: "x", description: 0 }] }; + expect(getSkills(cardZero)).toEqual([{ id: "x", description: undefined }]); + + const cardNum = { skills: [{ id: "x", description: 42 }] }; + expect(getSkills(cardNum)).toEqual([{ id: "x", description: "42" }]); + }); +}); diff --git a/canvas/src/components/ui/__tests__/KeyValueField.test.tsx b/canvas/src/components/ui/__tests__/KeyValueField.test.tsx new file mode 100644 index 00000000..1603faa6 --- /dev/null +++ b/canvas/src/components/ui/__tests__/KeyValueField.test.tsx @@ -0,0 +1,142 @@ +// @vitest-environment jsdom +/** + * Tests for KeyValueField component. + * + * Covers: initial password type, onChange callback (including whitespace trim + * on type), aria-label forwarding, disabled state, and auto-hide timer setup. + */ +import React from "react"; +import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; +import { render, screen, fireEvent, cleanup, act } from "@testing-library/react"; +import { KeyValueField } from "../KeyValueField"; + +describe("KeyValueField — rendering", () => { + afterEach(cleanup); + + it("renders input with type=password by default (secret hidden)", () => { + render(); + const input = screen.getByLabelText("Secret value"); + expect(input.getAttribute("type")).toBe("password"); + }); + + it("passes custom aria-label to the input element", () => { + render(); + expect(screen.getByLabelText("API secret key")).toBeTruthy(); + }); + + it("disables the input when disabled=true", () => { + render(); + expect(screen.getByLabelText("Secret value").disabled).toBe(true); + }); + + it("renders with the current value", () => { + render(); + expect(screen.getByLabelText("Secret value").value).toBe("sk-test-key-123"); + }); + + it("renders with the placeholder text", () => { + render(); + expect(screen.getByLabelText("Secret value").getAttribute("placeholder")).toBe("Enter API key"); + }); + + it("renders the RevealToggle child button", () => { + render(); + // KeyValueField renders exactly one button (the RevealToggle) + expect(screen.getByRole("button")).toBeTruthy(); + }); +}); + +describe("KeyValueField — onChange", () => { + afterEach(cleanup); + + it("calls onChange with the new value when user types", () => { + const onChange = vi.fn(); + render(); + fireEvent.change(screen.getByLabelText("Secret value"), { target: { value: "new-value" } }); + expect(onChange).toHaveBeenCalledWith("new-value"); + }); + + it("trims leading whitespace when user types with leading space", () => { + const onChange = vi.fn(); + render(); + fireEvent.change(screen.getByLabelText("Secret value"), { target: { value: " trimmed" } }); + expect(onChange).toHaveBeenCalledWith("trimmed"); + }); + + it("trims trailing whitespace when user types with trailing space", () => { + const onChange = vi.fn(); + render(); + fireEvent.change(screen.getByLabelText("Secret value"), { target: { value: "trimmed " } }); + expect(onChange).toHaveBeenCalledWith("trimmed"); + }); + + it("trims both sides when user types whitespace-surrounded value", () => { + const onChange = vi.fn(); + render(); + fireEvent.change(screen.getByLabelText("Secret value"), { target: { value: " both sides " } }); + expect(onChange).toHaveBeenCalledWith("both sides"); + }); + + it("does not modify value with no whitespace", () => { + const onChange = vi.fn(); + render(); + fireEvent.change(screen.getByLabelText("Secret value"), { target: { value: "clean-value" } }); + expect(onChange).toHaveBeenCalledWith("clean-value"); + }); +}); + +describe("KeyValueField — auto-hide timer setup", () => { + beforeEach(() => { + vi.useFakeTimers(); + }); + + afterEach(() => { + cleanup(); + vi.useRealTimers(); + }); + + it("sets up a 30s setTimeout when the component mounts with a non-empty value", () => { + const setTimeoutSpy = vi.spyOn(global, "setTimeout"); + render(); + // No timer should be set initially (revealed=false by default) + const callsBeforeInteraction = setTimeoutSpy.mock.calls.length; + + // Simulate reveal (click the only button) + act(() => { fireEvent.click(screen.getByRole("button")); }); + + // After reveal, a 30s timer should be set + const timerCalls = setTimeoutSpy.mock.calls.filter( + ([, delay]) => delay === 30_000, + ); + expect(timerCalls.length).toBeGreaterThanOrEqual(1); + }); + + it("clears existing timer when a new toggle happens before auto-hide fires", () => { + const clearTimeoutSpy = vi.spyOn(global, "clearTimeout"); + const timerObj = {}; // fake timer ID + vi.spyOn(global, "setTimeout").mockImplementation((fn: () => void, delay: number) => { + return timerObj; + }); + render(); + + // First toggle — reveal + act(() => { fireEvent.click(screen.getByRole("button")); }); + + // Second toggle — hide (should clear the timer from first toggle) + act(() => { fireEvent.click(screen.getByRole("button")); }); + + // clearTimeout was called with the timer object + expect(clearTimeoutSpy).toHaveBeenCalledWith(timerObj); + }); + + it("clears timer on unmount", () => { + const clearTimeoutSpy = vi.spyOn(global, "clearTimeout"); + const { unmount } = render(); + + // Toggle reveal to start the timer + act(() => { fireEvent.click(screen.getByRole("button")); }); + + unmount(); + expect(clearTimeoutSpy).toHaveBeenCalled(); + }); +}); diff --git a/canvas/src/components/ui/__tests__/RevealToggle.test.tsx b/canvas/src/components/ui/__tests__/RevealToggle.test.tsx new file mode 100644 index 00000000..0a68d454 --- /dev/null +++ b/canvas/src/components/ui/__tests__/RevealToggle.test.tsx @@ -0,0 +1,68 @@ +// @vitest-environment jsdom +/** + * Tests for RevealToggle component. + * + * Covers: eye-icon (hidden) vs eye-off-icon (revealed), onToggle callback, + * aria-label (default + custom), title attribute. + */ +import { afterEach, describe, it, expect, vi } from "vitest"; +import { render, screen, fireEvent, cleanup } from "@testing-library/react"; +import { RevealToggle } from "../RevealToggle"; + +afterEach(cleanup); + +describe("RevealToggle", () => { + it("renders as a button", () => { + render(); + expect(screen.getByRole("button")).toBeTruthy(); + }); + + it("uses default aria-label when not provided", () => { + render(); + expect(screen.getByRole("button").getAttribute("aria-label")).toBe("Toggle reveal secret"); + }); + + it("uses custom aria-label when provided", () => { + render(); + expect(screen.getByRole("button").getAttribute("aria-label")).toBe("Show password"); + }); + + it('title is "Hide value" when revealed', () => { + render(); + expect(screen.getByRole("button").getAttribute("title")).toBe("Hide value"); + }); + + it('title is "Show value" when hidden', () => { + render(); + expect(screen.getByRole("button").getAttribute("title")).toBe("Show value"); + }); + + it("calls onToggle when clicked (revealed=true → should hide)", () => { + const onToggle = vi.fn(); + render(); + fireEvent.click(screen.getByRole("button")); + expect(onToggle).toHaveBeenCalledTimes(1); + }); + + it("calls onToggle when clicked (revealed=false → should show)", () => { + const onToggle = vi.fn(); + render(); + fireEvent.click(screen.getByRole("button")); + expect(onToggle).toHaveBeenCalledTimes(1); + }); + + it("renders the eye-open SVG (hide icon) when revealed=false", () => { + render(); + const btn = screen.getByRole("button"); + // The eye SVG contains a circle element; eye-off has a strikethrough line + expect(btn.querySelector("circle")).toBeTruthy(); + expect(btn.querySelectorAll("line")).toHaveLength(0); + }); + + it("renders the eye-off SVG (show icon) when revealed=true", () => { + render(); + const btn = screen.getByRole("button"); + // EyeOffIcon has a line (strikethrough) through the eye + expect(btn.querySelectorAll("line")).toHaveLength(1); + }); +}); diff --git a/canvas/src/components/ui/__tests__/ValidationHint.test.tsx b/canvas/src/components/ui/__tests__/ValidationHint.test.tsx new file mode 100644 index 00000000..a0a2144c --- /dev/null +++ b/canvas/src/components/ui/__tests__/ValidationHint.test.tsx @@ -0,0 +1,49 @@ +// @vitest-environment jsdom +/** + * Tests for ValidationHint component. + * + * Covers: null/neutral render, error state (red ⚠ + message), valid state + * (green ✓ + "Valid format"), ARIA role="alert" on error. + */ +import { afterEach, describe, it, expect } from "vitest"; +import { render, screen, cleanup } from "@testing-library/react"; +import { ValidationHint } from "../ValidationHint"; + +afterEach(cleanup); + +describe("ValidationHint", () => { + it("renders nothing when error is null and showValid is false", () => { + const { container } = render(); + expect(container.innerHTML).toBe(""); + }); + + it("renders nothing when error is null and showValid is undefined", () => { + const { container } = render(); + expect(container.innerHTML).toBe(""); + }); + + it("renders error state with ⚠ icon and message", () => { + render(); + const el = screen.getByRole("alert"); + expect(el.textContent).toContain("⚠"); + expect(el.textContent).toContain("Key name must be UPPER_SNAKE_CASE"); + }); + + it("renders valid state with ✓ and 'Valid format'", () => { + render(); + const el = screen.getByText("Valid format"); + expect(el.textContent).toContain("✓"); + }); + + it("prefers error over valid when both are set (error is not null)", () => { + // ValidationHint checks error first; showValid is only rendered when error is falsy. + render(); + expect(screen.getByRole("alert")).toBeTruthy(); + expect(screen.queryByText("Valid format")).toBeNull(); + }); + + it("error alert has role='alert' for screen readers", () => { + render(); + expect(screen.getByRole("alert")).toBeTruthy(); + }); +}); diff --git a/tests/test_status_reaper.py b/tests/test_status_reaper.py index 5e444779..72dc9690 100644 --- a/tests/test_status_reaper.py +++ b/tests/test_status_reaper.py @@ -601,3 +601,175 @@ def test_scan_workflows_missing_dir_returns_empty(sr_module, tmp_path, capsys): assert out == {} captured = capsys.readouterr() assert "::warning::workflows dir not found" in captured.out + + +# -------------------------------------------------------------------------- +# rev2: multi-SHA sweep — `reap_branch()` walks last N main commits +# -------------------------------------------------------------------------- +# Phase 1+2 evidence (orchestrator + hongming-pc2): rev1 sees `compensated:0` +# every tick because the schedule workflow posts `failure` to whatever SHA +# was HEAD when it COMPLETED. By the next */5 tick, main has often moved +# forward, so the single-HEAD reaper misses the stranded red. rev2 sweeps +# the last 10 commits each tick. See `reference_post_suspension_pipeline` +# and parent rev1 PR #618 for context. + +SHA_A = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" +SHA_B = "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb" +SHA_C = "cccccccccccccccccccccccccccccccccccccccc" + + +def test_reap_sweeps_n_shas_smoke(sr_module, monkeypatch): + """rev2 contract: sweep last 10 (or N) main commits, GET combined + status for EACH. Smoke: with 3 stub SHAs, each is GET'd exactly once. + """ + gets: list[str] = [] + posts: list[tuple[str, dict]] = [] + + def fake_api(method, path, *, body=None, query=None, expect_json=True): + if method == "GET" and path.endswith("/commits"): + # commits listing — return 3 fake commit objects + return (200, [{"sha": SHA_A}, {"sha": SHA_B}, {"sha": SHA_C}]) + if method == "GET" and "/commits/" in path and path.endswith("/status"): + sha = path.split("/commits/")[1].split("/status")[0] + gets.append(sha) + # All combined=success → cost-optimization short-circuit + return (200, {"state": "success", "statuses": []}) + if method == "POST": + posts.append((path, body)) + return (201, {}) + raise AssertionError(f"unexpected api call: {method} {path}") + + monkeypatch.setattr(sr_module, "api", fake_api) + + workflow_map = {"x": False} + counters = sr_module.reap_branch( + workflow_map, "main", limit=10, dry_run=False + ) + + # Each of the 3 SHAs returned by /commits should be GET'd once. + assert gets == [SHA_A, SHA_B, SHA_C] + # No POST (everything was combined=success). + assert posts == [] + # Counters reflect what we saw. + assert counters["scanned_shas"] == 3 + assert counters["compensated"] == 0 + assert counters["compensated_per_sha"] == {} + + +def test_reap_skips_combined_success_shas(sr_module, monkeypatch): + """rev2 cost-optimization (refinement #2): when combined==success for + a SHA, do NOT iterate per-context statuses; move on to next SHA. + + Mock 2 SHAs with combined=success + 1 with combined=failure → only + the failure-SHA's statuses get the per-context loop applied. + """ + per_context_iterated_for: list[str] = [] + posts: list[tuple[str, dict]] = [] + + failure_statuses = [ + { + "context": "drift / drift (push)", + "state": "failure", + "target_url": "https://example.test/run/42", + } + ] + + def fake_api(method, path, *, body=None, query=None, expect_json=True): + if method == "GET" and path.endswith("/commits"): + return (200, [{"sha": SHA_A}, {"sha": SHA_B}, {"sha": SHA_C}]) + if method == "GET" and "/commits/" in path and path.endswith("/status"): + sha = path.split("/commits/")[1].split("/status")[0] + if sha == SHA_B: + # Mark this SHA as the failure one — return per-context + # statuses that would compensate if iterated. + return (200, {"state": "failure", "statuses": failure_statuses}) + # Others are combined=success — must short-circuit. + return (200, {"state": "success", "statuses": failure_statuses}) + if method == "POST": + # If a POST hits a non-failure SHA, the short-circuit failed. + posts.append((path, body)) + return (201, {}) + raise AssertionError(f"unexpected api call: {method} {path}") + + monkeypatch.setattr(sr_module, "api", fake_api) + + # Workflow trigger map: `drift` is schedule-only (compensable). + workflow_map = {"drift": False} + counters = sr_module.reap_branch( + workflow_map, "main", limit=10, dry_run=False + ) + + # Only SHA_B (the combined=failure one) should be compensated. + assert counters["compensated"] == 1 + assert counters["scanned_shas"] == 3 + assert SHA_B in counters["compensated_per_sha"] + assert counters["compensated_per_sha"][SHA_B] == ["drift / drift (push)"] + # SHA_A and SHA_C must NOT appear in compensated_per_sha — their + # per-context loop was skipped via the combined=success short-circuit. + assert SHA_A not in counters["compensated_per_sha"] + assert SHA_C not in counters["compensated_per_sha"] + # Exactly one POST: the compensation on SHA_B. + assert len(posts) == 1 + assert posts[0][0] == f"/repos/owner/repo/statuses/{SHA_B}" + + +def test_reap_continues_on_per_sha_apierror(sr_module, monkeypatch, capsys): + """rev2 refinement #7 (MOST CRITICAL): a transient ApiError or HTTP-5xx + on get_combined_status(SHA_X) must NOT fail the whole tick. Log + skip + SHA_X, continue with SHA_Y. + + Different from the single-HEAD path (where fail-loud is correct): the + sweep is best-effort across historical commits, so one transient blip + on a stale SHA should not strand reds on the OTHER stale SHAs. + """ + posts: list[tuple[str, dict]] = [] + + def fake_api(method, path, *, body=None, query=None, expect_json=True): + if method == "GET" and path.endswith("/commits"): + return (200, [{"sha": SHA_A}, {"sha": SHA_B}]) + if method == "GET" and "/commits/" in path and path.endswith("/status"): + sha = path.split("/commits/")[1].split("/status")[0] + if sha == SHA_A: + raise sr_module.ApiError( + f"GET /repos/owner/repo/commits/{SHA_A}/status " + f"-> HTTP 502: bad gateway" + ) + # SHA_B returns normally with a failure to compensate. + return ( + 200, + { + "state": "failure", + "statuses": [ + { + "context": "drift / drift (push)", + "state": "failure", + } + ], + }, + ) + if method == "POST": + posts.append((path, body)) + return (201, {}) + raise AssertionError(f"unexpected api call: {method} {path}") + + monkeypatch.setattr(sr_module, "api", fake_api) + + workflow_map = {"drift": False} + # Must NOT raise — per-SHA error isolation contract. + counters = sr_module.reap_branch( + workflow_map, "main", limit=10, dry_run=False + ) + + # SHA_A was logged + skipped. SHA_B processed normally. + assert counters["scanned_shas"] == 2 + assert counters["compensated"] == 1 + assert SHA_B in counters["compensated_per_sha"] + assert SHA_A not in counters["compensated_per_sha"] + # Compensation POST landed on SHA_B only. + assert len(posts) == 1 + assert posts[0][0] == f"/repos/owner/repo/statuses/{SHA_B}" + # The ApiError must be logged so a human auditing tick output can see + # WHICH SHA blipped and WHY. + captured = capsys.readouterr() + assert "::warning::" in captured.out or "::notice::" in captured.out + assert SHA_A[:10] in captured.out