From ac9ca53cb4a8d40989b228198b79d5384b043eee Mon Sep 17 00:00:00 2001 From: Jeff Huber Date: Wed, 16 Sep 2026 06:16:33 -0700 Subject: [PATCH 01/20] Board: implement the work-first Now, Timeline, Releases, and Health views MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Render the accepted B0 observation fixtures behind a compact work-first interaction model. The Board becomes a consumer of the frozen `code_mower.boardObservation.v1` contract; it never produces, resolves, or repairs one, and it adds no cloud field and no mutating control. - Read `.code-mower/board/observations/*.json` (or `--observations-path PATH`) through `board_observation.decode` and embed the survivors as `code_mower.boardObservations.v1` under `observations` in `/api/status`. Reading is bounded to 32 files per refresh; a record that fails the contract is dropped with the contract's own fixed diagnostic rather than widened. A missing directory reports "nothing recorded yet", which is a different statement from "no work". - Keep observations out of persisted local history: `_recordable_payload` drops the block, so `board record` and `--record-events` write the shape they already wrote. The cloud board-snapshot allowlist is untouched. - Organize the page as four semantic tabs over the one payload — Now, Timeline, Releases, Health — with persistent chrome carrying repository, version, snapshot time, the one next action, and observation freshness at every width. Completed campaigns move under Releases; connection, version, and process diagnostics move under Health. - Show work rows with the recorded safe reference, stage, assignments, last meaningful update, next action, and responsible role. Selecting a row exposes builder/review/CI/gate/merge/human-policy evidence as six independent readings, each naming its source and that source's freshness, so none can stand in for another. The gate publisher sits beside the `code-mower/gate` verdict and is labelled as publisher execution only. - Keep review requested, review observed running, stale review, changes requested, implementation complete, ready for human review, ready to merge, and merged distinct. - Make tabs and rows keyboard operable with visible focus and expanded state: rows are buttons carrying `aria-expanded`/`aria-controls`, arrow keys wrap across tabs and clamp across rows, and unselected panels carry `hidden` so they leave the accessibility tree. - Hold selection by opaque work identity (session, worktree, work id) rather than row position, so a refresh that reorders, adds, or drops rows leaves the operator's choice where it was. There is one detail region, rendered inside the selected row so phone widths follow the row, with CSS placing that same region adjacent to the list at desktop widths. - Announce a change only when a recorded fact differs — stage, reasons, route, PR identity, evidence state, measurements, run phase or basis, or a source's freshness, coverage or event time. `created_at`, `checked_at`, `observed_at` and `heartbeat_at` are excluded because they advance on every successful poll, so an unchanged poll announces nothing and collapses from Timeline. - Invent no totals: an unavailable measurement renders "not recorded", a partial one renders the value with the evidence it was counted from, and no ratio becomes a percentage or an ETA. Unknown states stay neutral and colour always carries a text label and a text cue. - Read liveness rather than infer it: a record whose sources are not all fresh, or that is older than ten minutes, is reported as a last observation and may not claim anything is running now. `no_work` reads as idle with complete coverage and names the fresh, complete source kinds; `unlinked` claims no stage and no route. - Keep primary actions read-only — open a recorded PR link, inspect a connection in Health, view recent changes. A locally observed PR number is never turned into a remote address the payload has not recorded. No merge, requeue, force-lease, cancel, retry, restart, cloud-schema or Slack-specific control, no form, and no non-GET request. `render_board_html` becomes a plain-text template with two substituted placeholders instead of an f-string, so the shipped CSS and JavaScript read exactly as the browser receives them and the tests execute the same text. Producer integration stays with #949: no session resolver, lifecycle producer, or local evidence correlation is added here. Closes #948 Co-Authored-By: Claude Opus 5 --- docs/board-data-contract.md | 120 ++- docs/launch-command-surface.md | 1 + src/code_mower/board.py | 1523 ++++++++++++++++++++++++++------ tests/test_board.py | 624 ++++++++++++- 4 files changed, 2018 insertions(+), 250 deletions(-) diff --git a/docs/board-data-contract.md b/docs/board-data-contract.md index 267cd625..f589048b 100644 --- a/docs/board-data-contract.md +++ b/docs/board-data-contract.md @@ -52,6 +52,13 @@ board's `/api/status` response. It summarizes PRs that need operator attention. the board's `/api/status` response. It reads opt-in metadata files from `.code-mower/board/agents/*.json` by default. +`code_mower.boardObservations.v1` is the local observation block embedded in the +board's `/api/status` response as `observations`. It carries validated +`code_mower.boardObservation.v1` records read from +`.code-mower/board/observations/*.json` by default, plus bounded diagnostics for +records the observation contract rejected. The Board only consumes that +contract; it never produces an observation. + `code_mower.supervisedPilot.v1` is the local supervised-pilot payload embedded as `supervised_pilot` in the board's `/api/status` response when `code-mower.yml` is present. It is derived from the same controller policy @@ -75,7 +82,8 @@ and local diagnostics: When `restart_recommended` is true, stop and restart `code-mower board serve --repo OWNER/REPO` so the browser uses the newly installed package. The response also embeds `productivity` as - `code_mower.boardProductivity.v1`. The GitHub/local snapshot behind this + `code_mower.boardProductivity.v1` and `observations` as + `code_mower.boardObservations.v1`. The GitHub/local snapshot behind this response is served from a thread-safe stale-while-refresh cache bounded by `board.refresh_seconds`: a cold request returns a metadata-only warming payload immediately and starts one background refresh, a @@ -443,6 +451,116 @@ add no fields to any schema; they only bound what the page is allowed to assert. rows) are absent, GitHub information stays useful and the page names the local data that is unavailable instead of rendering it as zero. +## Board Views + +The browser view is organized as four tabs over one payload. Nothing below +changes the payload; the views only bound what the page asserts and where it +says it. + +Persistent chrome — repository, serving and installed version, snapshot time, +the one next action, and observation freshness — stays on screen in every view +and at every width. The tabs are a real `tablist` of `tab` buttons controlling +real `tabpanel` regions; the unselected panels carry `hidden`, so they leave the +accessibility tree instead of being painted away. + +- **Now** — the work rows, the selected work item's evidence, the participant + summary, and the existing owner queue, lane work, supervised pilot and open + PR sections. +- **Timeline** — meaningful recent changes, local Board history, the reviewer + verdict timeline, and recent Code Mower workflow runs. +- **Releases** — release campaigns, productivity, and spend. Completed campaign + history lives here rather than in front of current work. +- **Health** — observation sources and their freshness, Board version and + restart state, snapshot cache state, GitHub availability, gate alerts, the + orchestrator lease, agent cards, and local Board and lane processes. + +### Work rows and selected-work detail + +Each work row shows the safe `reference` the observation records, the stage, +the assignments recorded for it, the last meaningful update, the recorded next +action, and the responsible role. Selecting a row exposes the six independent +evidence readings — builder runs, review, CI, gate, merge, and human policy — +each naming the source it came from and how fresh that source is, so no reading +can stand in for another. The gate publisher is shown beside the +`code-mower/gate` verdict and is labelled as publisher execution only. + +Selection is kept by opaque work identity — session, worktree, and work id — +not by row position, so a refresh that reorders, adds, or drops rows leaves the +operator's choice where it was. There is exactly one detail region. It is +rendered inside the selected row, so at phone widths it follows the row it +belongs to, and at desktop widths CSS places that same region adjacent to the +list. Rows are buttons carrying `aria-expanded` and `aria-controls`; Up, Down, +Home and End move the selection, tabs wrap with the arrow keys, and every +interactive control has a visible focus ring. + +Meaningful changes are announced once through a polite live region and listed +in Timeline. A change is meaningful when a recorded fact differs: stage, +reasons, route, pull request identity, evidence state, measurements, run phase +or basis, or a source's freshness, coverage or event time. `created_at`, +`checked_at`, `observed_at` and `heartbeat_at` are excluded because they advance +on every successful poll, so a poll that repeats the same observation announces +nothing and adds no Timeline entry. + +Primary actions stay read-only: open a recorded PR link, inspect a connection +in Health, and view recent changes. A pull request number observed locally is +never turned into a remote address the payload has not recorded. There is no +merge, requeue, force-lease, cancel, retry, restart, cloud-schema or +Slack-specific control, no form, and no non-GET request. + +### Observation presentation rules + +- **Liveness is read, never inferred.** A run is only described in the phase its + own record states, alongside the freshness of the source behind it. A record + whose sources are not all `fresh`, or that is more than ten minutes old, is + reported as a last observation and may not claim anything is running now. +- **Unavailable is not zero, and idle is not unknown.** A `no_work` observation + is shown as idle *with complete coverage* and names the source kinds that were + observed fresh and complete. An `unlinked` observation claims no stage and no + route, because the contract records none for it. +- **No invented totals.** An unavailable measurement renders `not recorded`. A + partial measurement renders the value with the evidence it was counted from + (`120.0s from 2 of 5 recorded`). Nothing is extrapolated to a whole, and no + ratio is turned into a percentage or an ETA. +- **Unknown is neutral.** Evidence states are a closed vocabulary and each one + is classified explicitly; `unknown`, `not_started`, `absent`, `unverifiable`, + `none` and `unassigned` are neutral, and anything unrecognised is neutral too. + Colour always accompanies a text label and a text cue. +- **An empty directory is not an empty queue.** With no observation recorded, + the work list says so rather than reporting no work, and the GitHub-derived + queues below it still render. + +## Board Observations + +`code_mower.boardObservations.v1` is the local observation block embedded in the +board's `/api/status` response as `observations`. It is a **consumer** of the +frozen `code_mower.boardObservation.v1` contract: the Board reads records, +decodes each through `board_observation.decode`, and renders what survives. The +Board never writes an observation, resolves a session, contacts a provider, or +repairs a record that fails the contract. + +By default the Board reads `*.json` files under +`.code-mower/board/observations/`. Use `--observations-path PATH` for a custom +local directory. A missing directory is reported as "nothing recorded yet", +which is a different statement from "no work". Reading is bounded to 32 files +per refresh, and the observation contract itself bounds each record. + +The block carries: + +- `records[]` — validated `code_mower.boardObservation.v1` records, in file-name + order. +- `rejected` and `warnings[]` — the count of records that failed, with the + contract's own fixed diagnostic (`invalid_contract`, `invalid_route`, + `identity_mismatch`, and so on). Those diagnostics deliberately omit observed + values and local paths. +- `path`, redacted as `[local path hidden]`, `path_exists`, `available`, and a + safe `message`. + +Observations are not copied into the local Board event store: `code-mower board +record` and `--record-events` persist the snapshot without the `observations` +block, so local history keeps the shape it already had. The cloud board-snapshot +export is an allowlist of summarized fields and is unchanged by this block; no +observation field is uploaded. + ## Agent Adapters The Board embeds `code_mower.boardAgentAdapters.v1` in `/api/status`. Agent diff --git a/docs/launch-command-surface.md b/docs/launch-command-surface.md index 5c7be859..6bcdcf7d 100644 --- a/docs/launch-command-surface.md +++ b/docs/launch-command-surface.md @@ -47,6 +47,7 @@ Select any additional builder or reviewer explicitly. | `code-mower productivity report --repo OWNER/REPO --cloud-event PATH --json` | Include metadata-only `productivity_summary` aggregate event files and print the stable report JSON. | no | no | | `code-mower board serve --repo OWNER/REPO --record-events` | Serve the board and append throttled metadata-only local history snapshots while it is open. | yes, local only | GitHub optional | | `code-mower board serve --repo OWNER/REPO --agent-adapters-path PATH` | Read opt-in local agent cards from a custom metadata-only adapter directory. | no | no | +| `code-mower board serve --repo OWNER/REPO --observations-path PATH` | Render local `code_mower.boardObservation.v1` records from a custom read-only directory. The Board consumes that contract and never writes one. | no | no | | `code-mower board list` | List local Board listeners with repo/version, restart hints, ports, and redacted cwd paths by default. | no | no | | `code-mower board stop --port PORT --yes` | Stop a local Board listener that the inventory identified as Code Mower. | local process signal | no | | `code-mower board record --repo OWNER/REPO` | Append one redacted status snapshot to `.code-mower/board/events.jsonl` for local board history. | yes, local only | GitHub optional | diff --git a/src/code_mower/board.py b/src/code_mower/board.py index bbb3eb3c..5fc3994a 100644 --- a/src/code_mower/board.py +++ b/src/code_mower/board.py @@ -28,6 +28,7 @@ from urllib.parse import urlparse from . import __version__ as CODE_MOWER_VERSION +from . import board_observation from . import board_store from . import config as code_mower_config from . import controller @@ -47,8 +48,13 @@ BOARD_INVENTORY_SCHEMA = "code_mower.boardInventory.v1" BOARD_STOP_SCHEMA = "code_mower.boardStop.v1" BOARD_RELEASE_CAMPAIGNS_SCHEMA = "code_mower.boardReleaseCampaigns.v1" +BOARD_OBSERVATIONS_SCHEMA = "code_mower.boardObservations.v1" DEFAULT_AGENT_ADAPTERS_RELATIVE_PATH = Path(".code-mower") / "board" / "agents" +DEFAULT_OBSERVATIONS_RELATIVE_PATH = Path(".code-mower") / "board" / "observations" DEFAULT_CAMPAIGNS_RELATIVE_PATH = Path(".code-mower") / "campaigns" +# Bounded so a directory left full of records cannot turn one page load into an +# unbounded read. The contract itself bounds each record to MAX_BYTES. +MAX_OBSERVATION_FILES = 32 SECRET_VALUE_RE = re.compile( r"(github_pat_[A-Za-z0-9_]+|gh[pousr]_[A-Za-z0-9_]{20,}|sk-[A-Za-z0-9_-]{20,}|xox[baprs]-[A-Za-z0-9-]{20,})" ) @@ -69,6 +75,7 @@ class BoardConfig: store_path: str | None = None spend_path: str | None = None agent_adapters_path: str | None = None + observations_path: str | None = None campaigns_path: str | None = None event_limit: int = 20 record_events: bool = False @@ -95,6 +102,12 @@ def _agent_adapters_path(config: BoardConfig) -> Path: return Path(config.repo_path) / DEFAULT_AGENT_ADAPTERS_RELATIVE_PATH +def _observations_path(config: BoardConfig) -> Path: + if config.observations_path: + return Path(config.observations_path) + return Path(config.repo_path) / DEFAULT_OBSERVATIONS_RELATIVE_PATH + + def _campaigns_path(config: BoardConfig) -> Path: if config.campaigns_path: return Path(config.campaigns_path) @@ -113,6 +126,7 @@ def resolved_metadata_paths(config: BoardConfig) -> dict[str, str]: "store_path": str(_store_path(config)), "spend_path": str(_spend_path(config)), "agent_adapters_path": str(_agent_adapters_path(config)), + "observations_path": str(_observations_path(config)), "campaigns_path": str(_campaigns_path(config)), } @@ -262,6 +276,7 @@ def status_payload( } payload["orchestrator_lease"] = session_lease.observe_lease(start=config.repo_path) payload["agent_adapters"] = agent_adapters_payload(config) + payload["observations"] = observations_payload(config) payload["release_campaigns"] = release_campaigns_payload(config) payload["owner_queue"] = owner_queue_payload(payload) payload["supervised_pilot"] = supervised_pilot_payload( @@ -302,6 +317,10 @@ def _recordable_payload(payload: dict[str, Any]) -> dict[str, Any]: snapshot = dict(payload) snapshot.pop("productivity", None) snapshot.pop("orchestrator_lease", None) + # Observations are a live read model carrying local session and worktree + # identity. The Board renders them; it does not copy them into persisted + # local history, so replayed history keeps the shape it already had. + snapshot.pop("observations", None) return snapshot @@ -1157,6 +1176,66 @@ def prune_stale_agent_adapters( return result +def observations_payload(config: BoardConfig) -> dict[str, Any]: + """Read locally recorded Board observations without producing any. + + This is a consumer of the frozen ``code_mower.boardObservation.v1`` + contract: every record is decoded by :mod:`board_observation` and a record + that fails that contract is dropped with its own bounded diagnostic instead + of being repaired, widened, or rendered. The Board never writes these + files, never resolves a session, and never contacts a provider to fill one + in; an empty directory is reported as "nothing recorded yet", which is a + different statement from "no work". + """ + + path = _observations_path(config) + payload: dict[str, Any] = { + "schema": BOARD_OBSERVATIONS_SCHEMA, + "record_schema": board_observation.SCHEMA, + "available": True, + "path": lane_status.LOCAL_PATH_REDACTION, + "path_redacted": True, + "path_exists": path.exists(), + "records": [], + "warnings": [], + "rejected": 0, + "message": "no local Board observations recorded yet", + } + if not path.exists(): + return payload + if not path.is_dir(): + payload["available"] = False + payload["warnings"].append({"file": "", "message": "observation path is not a directory"}) + payload["message"] = "could not read local Board observations" + return payload + try: + candidates = sorted(path.glob("*.json"))[:MAX_OBSERVATION_FILES] + except OSError: + payload["available"] = False + payload["warnings"].append({"file": "", "message": "could not list local Board observations"}) + payload["message"] = "could not read local Board observations" + return payload + for record_file in candidates: + try: + raw = record_file.read_bytes() + except OSError: + payload["rejected"] += 1 + payload["warnings"].append({"file": record_file.name, "message": "could not read observation file"}) + continue + try: + payload["records"].append(board_observation.decode(raw)) + except board_observation.BoardObservationError as exc: + # The contract's diagnostics are a fixed closed vocabulary that + # deliberately omits observed values and local paths. + payload["rejected"] += 1 + payload["warnings"].append({"file": record_file.name, "message": str(exc)}) + if payload["records"]: + payload["message"] = "" + elif payload["rejected"]: + payload["message"] = "no local Board observation passed the observation contract" + return payload + + def release_campaigns_payload( config: BoardConfig, *, @@ -1297,35 +1376,79 @@ def timelines_payload( } -def render_board_html(config: BoardConfig) -> str: - repo_json = json.dumps(config.repo).replace(" +# The whole Board page as one template. It is plain text rather than an +# f-string so the shipped CSS and JavaScript read exactly as the browser +# receives them, with no doubled braces between the source and the page the +# tests execute. Only the two placeholders below are substituted. +_BOARD_HTML = """ Code Mower Board @@ -1333,30 +1456,52 @@ def render_board_html(config: BoardConfig) -> str:

Code Mower Board

Loading...
+ +
+
+ +
+ +
-
- -

Work Now

-

Owner Queue

-

Lane Work

-

Supervised Pilot

-

Open PRs

-

Gate Alerts

-

Agent Cards

-

Local Orchestrator Lease

-

Release Campaigns

-

Productivity

-

Recent Code Mower Workflows

-

Recent Local History

-

Reviewer Verdict Timeline

-

Spend And Latency

-

Local Activity

+
+
+ +

Work

+

Work Now

+

Participants

+

Owner Queue

+

Lane Work

+

Supervised Pilot

+

Open PRs

+
+ + +
@@ -1896,6 +2908,16 @@ def render_board_html(config: BoardConfig) -> str: """ +def render_board_html(config: BoardConfig) -> str: + repo_json = json.dumps(config.repo).replace(" dict[str, Any]: url = str(board.get("url") or "") if not url: @@ -2721,6 +3743,10 @@ def main(argv: list[str] | None = None) -> int: serve_parser.add_argument("--store-path", help="custom local Board event store path") serve_parser.add_argument("--spend-path", help="custom reviewer spend ledger path") serve_parser.add_argument("--agent-adapters-path", help="custom local agent card directory") + serve_parser.add_argument( + "--observations-path", + help="custom local Board observation directory (read-only)", + ) serve_parser.add_argument("--event-limit", type=int, default=20, help="local history events to show") serve_parser.add_argument("--record-events", action="store_true", help="append local history while the Board is open") serve_parser.add_argument("--record-interval-seconds", type=int, default=60, help="minimum seconds between records") @@ -2800,6 +3826,7 @@ def main(argv: list[str] | None = None) -> int: store_path=args.store_path, spend_path=args.spend_path, agent_adapters_path=args.agent_adapters_path, + observations_path=args.observations_path, event_limit=args.event_limit, record_events=args.record_events, record_interval_seconds=args.record_interval_seconds, diff --git a/tests/test_board.py b/tests/test_board.py index b23cb693..0a537c35 100644 --- a/tests/test_board.py +++ b/tests/test_board.py @@ -1,6 +1,7 @@ from __future__ import annotations from contextlib import redirect_stderr, redirect_stdout +import copy import http.client import json import math @@ -19,7 +20,7 @@ from unittest.mock import patch from io import StringIO -from code_mower import board, board_store, lane_status, reviewer_spend +from code_mower import board, board_observation, board_store, lane_status, reviewer_spend NOW = datetime(2026, 9, 1, 12, 0, tzinfo=UTC) @@ -3592,3 +3593,624 @@ def poll_status() -> dict: server.shutdown() server.server_close() server_thread.join(timeout=5) + + +# --- Work-first Board views (#948) ----------------------------------------- + +WORK_MODEL_END = "// --- work view model (END) ---" + +# The fixture clock plus 30s, so every fixture record is recent enough to be +# reported as current unless the record itself says otherwise. +OBSERVATION_NOW = datetime(2026, 9, 12, 20, 0, 30, tzinfo=UTC) + +OBSERVATION_FIXTURES = Path(__file__).parent / "fixtures" / "board_observations.json" + +# Render one payload after another through the shipped renderer in a single +# page lifetime, so selection, announcements and the change timeline are +# exercised the way a refresh actually exercises them. A step may select a work +# row by its opaque key before rendering the next payload. +BOARD_SEQUENCE_HARNESS = """ +const NODES = {}; +const document = {getElementById: (id) => (NODES[id] = NODES[id] || {innerHTML: "", textContent: ""})}; +Date.now = () => __NOW_MS__; +__SCRIPT__ +const frames = []; +for (const step of JSON.parse(process.argv[1])) { + if (step.select !== null) selectWork(step.select); + if (step.payload !== null) render(step.payload); + frames.push(Object.fromEntries(Object.entries(NODES).map(([id, node]) => [id, node.innerHTML || node.textContent]))); +} +console.log(JSON.stringify(frames)); +""" + + +def _board_script() -> str: + """The shipped page script, minus its own ``load()`` bootstrap.""" + + html = board.render_board_html(board.BoardConfig(repo="codemower-ai/code-mower")) + body = html[html.index(" ")] + trimmed = body.rsplit(" load();", 1) + if len(trimmed) != 2: # pragma: no cover - guards the extraction + raise AssertionError("board HTML no longer bootstraps with load()") + return "".join(trimmed) + + +def _board_view_model() -> str: + """Lift the shipped, DOM-free view-model transforms out of the page. + + The work view model is layered on the B1 truth helpers, so the extraction + runs from the first helper through the end of the view-model block. As with + the B1 helpers, the tests execute the JavaScript the browser gets. + """ + + html = board.render_board_html(board.BoardConfig(repo="codemower-ai/code-mower")) + start = html.find(" const text =") + end = html.find(WORK_MODEL_END) + if start < 0 or end < start: # pragma: no cover - guards the extraction + raise AssertionError("board HTML no longer exposes the work view model") + return html[start : end + len(WORK_MODEL_END)] + + +def _eval_board_view(expression: str, *args: object) -> object: + """Evaluate one shipped view-model expression against JSON arguments.""" + + script = ( + _board_view_model() + + "\nconst ARGS = process.argv.slice(1).map(value => JSON.parse(value));\n" + + f"console.log(JSON.stringify({expression}));\n" + ) + completed = subprocess.run( + [shutil.which("node") or "node", "-e", script, *(json.dumps(arg) for arg in args)], + capture_output=True, + text=True, + check=True, + ) + return json.loads(completed.stdout) + + +def _render_board_sequence( + steps: list[dict[str, object]], + *, + now: datetime = OBSERVATION_NOW, +) -> list[dict[str, str]]: + """Render a sequence of payloads in one page lifetime.""" + + script = BOARD_SEQUENCE_HARNESS.replace("__NOW_MS__", str(int(now.timestamp() * 1000))).replace( + "__SCRIPT__", _board_script() + ) + normalized = [{"select": step.get("select"), "payload": step.get("payload")} for step in steps] + completed = subprocess.run( + [shutil.which("node") or "node", "-e", script, json.dumps(normalized)], + capture_output=True, + text=True, + check=True, + ) + return json.loads(completed.stdout) + + +def _observation_fixture(name: str) -> dict: + """Build one accepted B0 fixture record, unchanged.""" + + fixture = json.loads(OBSERVATION_FIXTURES.read_text(encoding="utf-8")) + case = next(item for item in fixture["valid"] if item["name"] == name) + record = copy.deepcopy(fixture["templates"][case["template"]]) + for pointer, value in case["set"].items(): + if pointer == "": + record = copy.deepcopy(value) + continue + parts = [part.replace("~1", "/").replace("~0", "~") for part in pointer.split("/")[1:]] + target: object = record + for part in parts[:-1]: + target = target[int(part)] if isinstance(target, list) else target[part] + if isinstance(target, list): + target[int(parts[-1])] = copy.deepcopy(value) + else: + target[parts[-1]] = copy.deepcopy(value) + # Every fixture the views are tested against is a record the frozen + # contract accepts, so no view is ever proved against a shape a producer + # could not emit. + return board_observation.validate(record) + + +def _observation_payload(records: list[dict], **overrides: object) -> dict: + payload: dict[str, object] = { + "generated_at": "2026-09-12T20:00:00Z", + "next_action": "inspect", + "board": { + "cache": { + "state": "fresh", + "ttl_seconds": 15, + "age_seconds": 1, + "generation": 2, + "refresh_in_progress": False, + "retry_in_seconds": None, + }, + "version": { + "serving_version": "1.4.1", + "installed_version": "1.4.1", + "restart_recommended": False, + }, + }, + "remote": { + "available": True, + "pull_requests": [], + "workflow_runs": [], + "gate_health": {"alerts": []}, + }, + "observations": { + "available": True, + "path_exists": True, + "records": records, + "warnings": [], + "rejected": 0, + "message": "", + }, + } + payload.update(overrides) + return payload + + +def _work_keys(worklist: str) -> list[str]: + return re.findall(r'class="rowbtn" id="[^"]+" data-key="([^"]+)"', worklist) + + +def _selected_key(worklist: str) -> str: + match = re.search(r'data-key="([^"]+)" aria-expanded="true"', worklist) + return match.group(1) if match else "" + + +class BoardObservationReaderTests(TestCase): + """The Board consumes the frozen observation contract; it never writes one.""" + + def _write(self, directory: Path, name: str, record: object) -> None: + (directory / name).write_text(json.dumps(record), encoding="utf-8") + + def test_missing_directory_is_nothing_recorded_not_no_work(self) -> None: + with tempfile.TemporaryDirectory() as tmp: + payload = board.observations_payload( + board.BoardConfig(repo="owner/repo", observations_path=str(Path(tmp) / "absent")) + ) + self.assertTrue(payload["available"]) + self.assertFalse(payload["path_exists"]) + self.assertEqual(payload["records"], []) + self.assertEqual(payload["message"], "no local Board observations recorded yet") + self.assertEqual(payload["path"], lane_status.LOCAL_PATH_REDACTION) + + def test_valid_records_are_returned_and_invalid_ones_are_rejected(self) -> None: + with tempfile.TemporaryDirectory() as tmp: + directory = Path(tmp) + self._write(directory, "a-work.json", _observation_fixture("observed_running")) + self._write(directory, "b-idle.json", _observation_fixture("no_work")) + # A record that fails the contract is dropped, not repaired. + broken = _observation_fixture("observed_running") + broken["work"]["reasons"] = ["review_requested", "approval_required"] + self._write(directory, "c-broken.json", broken) + (directory / "d-garbage.json").write_text("{not json", encoding="utf-8") + payload = board.observations_payload( + board.BoardConfig(repo="owner/repo", observations_path=str(directory)) + ) + + self.assertEqual(len(payload["records"]), 2) + self.assertEqual([record["kind"] for record in payload["records"]], ["work", "no_work"]) + self.assertEqual(payload["rejected"], 2) + messages = {warning["message"] for warning in payload["warnings"]} + # Contract diagnostics are a fixed vocabulary with no values or paths. + self.assertTrue(messages <= {"invalid_route", "invalid_contract"}) + self.assertEqual(payload["record_schema"], board_observation.SCHEMA) + + def test_reading_is_bounded(self) -> None: + with tempfile.TemporaryDirectory() as tmp: + directory = Path(tmp) + record = _observation_fixture("no_work") + for index in range(board.MAX_OBSERVATION_FILES + 5): + self._write(directory, f"obs-{index:03d}.json", record) + payload = board.observations_payload( + board.BoardConfig(repo="owner/repo", observations_path=str(directory)) + ) + self.assertEqual(len(payload["records"]), board.MAX_OBSERVATION_FILES) + + def test_observations_are_not_written_into_local_history(self) -> None: + snapshot = {"schema": "code_mower.laneStatus.v1", "observations": {"records": [1]}} + self.assertNotIn("observations", board._recordable_payload(snapshot)) + self.assertIn("observations", snapshot) + + def test_resolved_metadata_paths_bind_the_observation_directory(self) -> None: + paths = board.resolved_metadata_paths(board.BoardConfig(repo="owner/repo", repo_path="/repo")) + self.assertTrue(paths["observations_path"].endswith("/.code-mower/board/observations")) + + +@skipUnless(shutil.which("node"), "node is required to execute the shipped board renderer") +class BoardWorkFirstViewTests(TestCase): + """The Now, Timeline, Releases and Health views over B0 fixtures.""" + + def test_views_are_semantic_tabs_with_visible_focus_and_expanded_state(self) -> None: + html = board.render_board_html(board.BoardConfig(repo="owner/repo")) + for view in ("now", "timeline", "releases", "health"): + self.assertIn(f'id="panel-{view}" role="tabpanel" aria-labelledby="tab-{view}"', html) + self.assertIn('role="tablist"', html) + self.assertIn(":focus-visible { outline:", html) + + nodes = _render_board_sequence( + [{"payload": _observation_payload([_observation_fixture("observed_running")])}] + )[0] + tabs = nodes["tabs"] + self.assertEqual(tabs.count('role="tab"'), 4) + self.assertEqual(tabs.count('aria-selected="true"'), 1) + self.assertIn('id="tab-now" data-view="now" aria-selected="true"', tabs) + # Roving tabindex: exactly one tab is in the tab order. + self.assertEqual(tabs.count('tabindex="0"'), 1) + self.assertEqual(tabs.count('tabindex="-1"'), 3) + # One row is expanded and it is the one carrying the detail region. + self.assertEqual(nodes["worklist"].count('aria-expanded="true"'), 1) + self.assertIn('aria-controls="workdetail"', nodes["worklist"]) + # The detail region is labelled by the row it belongs to, and the row's + # element id is derived from its opaque identity rather than its index. + row_id = re.search(r'class="rowbtn" id="([^"]+)"', nodes["worklist"]).group(1) + self.assertIn(f'id="workdetail" role="region" aria-labelledby="{row_id}"', nodes["worklist"]) + self.assertIn("runningwork", row_id) + + def test_keyboard_movement_rules_wrap_for_tabs_and_clamp_for_rows(self) -> None: + moves = _eval_board_view( + "[" + "nextTabIndex('ArrowRight', 3, 4), nextTabIndex('ArrowLeft', 0, 4)," + "nextTabIndex('Home', 2, 4), nextTabIndex('End', 0, 4), nextTabIndex('a', 0, 4)," + "nextRowIndex('ArrowDown', 2, 3), nextRowIndex('ArrowUp', 0, 3)," + "nextRowIndex('Home', 2, 3), nextRowIndex('End', 0, 3), nextRowIndex('ArrowLeft', 0, 3)," + "nextRowIndex('ArrowDown', 0, 0)" + "]" + ) + self.assertEqual(moves, [0, 3, 0, 3, -1, 2, 0, 0, 2, -1, -1]) + + def test_work_row_carries_reference_stage_assignment_update_action_and_role(self) -> None: + nodes = _render_board_sequence( + [{"payload": _observation_payload([_observation_fixture("observed_running")])}] + )[0] + row = nodes["worklist"] + self.assertIn('issue-946', row) + self.assertIn("stage: building", row) + self.assertIn("assignments: codex builder observed running", row) + self.assertIn("last update: 50s ago", row) + self.assertIn("responsible: no responsible role recorded", row) + self.assertIn("next: no next action recorded", row) + + reviewed = _render_board_sequence( + [{"payload": _observation_payload([_observation_fixture("reviewed")])}] + )[0]["worklist"] + self.assertIn("next: review the change", reviewed) + self.assertIn("responsible: owner", reviewed) + + def test_lifecycle_states_remain_distinct(self) -> None: + expected = { + "review_requested": ("implementation_complete", "review requested"), + "review_running": ("observed_running", "provider run observed"), + "stale_review": ("stale", "stale observation"), + "changes_requested": ("cancelled", "provider run cancelled"), + "implementation_complete": ("implementation_complete", "implementation complete"), + "human_review": ("reviewed", "ready for human review"), + "ready": ("ready", "ready to merge"), + "merged": ("merged", "merged"), + } + headlines = {} + for label, (fixture_name, _state) in expected.items(): + states = _eval_board_view("workStates(ARGS[0].work)", _observation_fixture(fixture_name)) + headlines[label] = [state["label"] for state in states] + + self.assertEqual(headlines["ready"][0], "ready to merge") + self.assertEqual(headlines["merged"][0], "merged") + self.assertEqual(headlines["human_review"][0], "ready for human review") + self.assertEqual(headlines["implementation_complete"][0], "implementation complete") + self.assertIn("review requested", headlines["implementation_complete"]) + self.assertEqual(headlines["stale_review"][0], "stale observation") + self.assertEqual(headlines["review_running"][0], "provider run observed") + + # The eight named lifecycle states never share a label. + distinct = [ + "review requested", + "review observed running", + "stale review", + "changes requested", + "implementation complete", + "ready for human review", + "ready to merge", + "merged", + ] + self.assertEqual(len(set(distinct)), len(distinct)) + rules = _eval_board_view("STATE_RULES.map(rule => rule.label)") + for label in distinct: + self.assertIn(label, rules) + self.assertEqual(len(rules), len(set(rules))) + + def test_gate_publisher_never_stands_in_for_the_gate_verdict(self) -> None: + nodes = _render_board_sequence( + [{"payload": _observation_payload([_observation_fixture("publisher_pass_gate_pending")])}] + )[0] + detail = nodes["worklist"] + self.assertIn("code-mower/gate verdict", detail) + self.assertIn("gate publisher run", detail) + self.assertIn("Publisher execution only; it is not the gate verdict.", detail) + self.assertIn("gate pending", detail) + + def test_selected_row_exposes_independent_evidence(self) -> None: + nodes = _render_board_sequence( + [{"payload": _observation_payload([_observation_fixture("ready")])}] + )[0] + detail = nodes["worklist"] + for group in ("Builder", "Review", "CI", "Gate", "Merge", "Human policy"): + self.assertIn(f"

{group}

", detail) + self.assertIn("orchestrator lease", detail) + self.assertIn("An assignment is a record of intent, not of execution.", detail) + self.assertIn("github, fresh, complete coverage", detail) + self.assertIn("remote_session, fresh, complete coverage", detail) + + def test_selection_is_kept_by_opaque_work_identity_across_refresh(self) -> None: + first = _observation_fixture("observed_running") + second = _observation_fixture("ready") + payload = _observation_payload([first, second]) + keys = _work_keys(_render_board_sequence([{"payload": payload}])[0]["worklist"]) + self.assertEqual(len(keys), 2) + running_key = next(key for key in keys if key.endswith("runningwork")) + + # Select the row that is not the default, then refresh twice: once with + # the same payload and once with the rows in the opposite order. + reordered = _observation_payload([second, first]) + frames = _render_board_sequence( + [ + {"payload": payload}, + {"select": running_key, "payload": payload}, + {"payload": reordered}, + ] + ) + self.assertNotEqual(_selected_key(frames[0]["worklist"]), running_key) + self.assertEqual(_selected_key(frames[1]["worklist"]), running_key) + self.assertEqual(_selected_key(frames[2]["worklist"]), running_key) + + # The identity is built only from session, worktree and work id. + key = _eval_board_view("workKey(ARGS[0])", first) + self.assertTrue(key.startswith("work:")) + self.assertIn(first["work"]["id"], key) + moved = copy.deepcopy(first) + moved["created_at"] = "2026-09-12T20:00:01Z" + self.assertEqual(_eval_board_view("workKey(ARGS[0])", moved), key) + + def test_unchanged_polls_announce_nothing_and_stay_out_of_the_timeline(self) -> None: + record = _observation_fixture("observed_running") + payload = _observation_payload([record]) + # A poll that only advances observation and heartbeat times is an + # unchanged snapshot, not news. + polled = copy.deepcopy(payload) + for source in polled["observations"]["records"][0]["sources"]: + source["checked_at"] = "2026-09-12T20:00:20Z" + changed = copy.deepcopy(payload) + changed["observations"]["records"][0]["work"]["stage"] = "in_review" + + frames = _render_board_sequence( + [ + {"payload": payload}, + {"payload": polled}, + {"payload": changed}, + {"payload": changed}, + ] + ) + # The live region is only ever touched by a meaningful change, so the + # first render and the unchanged poll after it leave it untouched. + self.assertEqual(frames[0].get("announce", ""), "") + self.assertEqual(frames[1].get("announce", ""), "") + self.assertIn("not listed here", frames[1]["changes"]) + self.assertIn("issue-946", frames[2]["announce"]) + # A recorded change that does not move the headline is still reported + # as a change rather than as a new state. + self.assertIn("changed while staying provider run observed", frames[2]["announce"]) + self.assertEqual( + _eval_board_view( + "changeSentence({kind: 'changed', reference: 'issue-946'," + " headline: 'ready to merge', from: 'in review'})" + ), + "issue-946 moved from in review to ready to merge", + ) + self.assertEqual(frames[2]["changes"].count('class="row"'), 1) + # The fourth poll repeats the third, so nothing new is announced or logged. + self.assertEqual(frames[3]["announce"], frames[2]["announce"]) + self.assertEqual(frames[3]["changes"].count('class="row"'), 1) + + def test_signature_ignores_poll_timestamps_and_tracks_recorded_change(self) -> None: + record = _observation_fixture("observed_running") + polled = copy.deepcopy(record) + for source in polled["sources"]: + source["checked_at"] = "2026-09-12T20:00:20Z" + source["observed_at"] = "2026-09-12T20:00:10Z" + if source["heartbeat_at"] is not None: + source["heartbeat_at"] = "2026-09-12T20:00:10Z" + polled["created_at"] = "2026-09-12T20:00:20Z" + self.assertEqual( + _eval_board_view("workSignature(ARGS[0])", record), + _eval_board_view("workSignature(ARGS[0])", polled), + ) + moved = copy.deepcopy(record) + moved["work"]["runs"][0]["phase"] = "implementation_complete" + moved["work"]["runs"][0]["basis"] = "provider_reported" + moved["work"]["runs"][0]["lifecycle"]["state"] = "complete" + self.assertNotEqual( + _eval_board_view("workSignature(ARGS[0])", record), + _eval_board_view("workSignature(ARGS[0])", moved), + ) + + def test_fixture_scenarios_produce_honest_summaries(self) -> None: + # No session: an unlinked observation claims no stage and no route. + unlinked = _render_board_sequence( + [{"payload": _observation_payload([_observation_fixture("unlinked")])}] + )[0]["worklist"] + self.assertIn("identity unlinked", unlinked) + self.assertIn("stage: not linked to a session", unlinked) + self.assertIn("next: no next action recorded", unlinked) + self.assertIn("Nothing binds this run to Code Mower work", unlinked) + + # Idle with complete coverage is idle because it was looked at. + idle = _render_board_sequence( + [{"payload": _observation_payload([_observation_fixture("no_work")])}] + )[0]["worklist"] + self.assertIn("idle with complete coverage", idle) + self.assertIn("session, work_queue, run_registry", idle) + self.assertNotIn("no work found", idle) + + # An unavailable source preserves the last observation without a live claim. + unavailable = _render_board_sequence( + [ + { + "payload": _observation_payload( + [_observation_fixture("source_unavailable_preserves_last_observation")] + ) + } + ] + )[0]["worklist"] + self.assertIn("source unavailable", unavailable) + self.assertIn("last observed", unavailable) + self.assertIn("is not evidence of work running now", unavailable) + + # A stale source is reported as stale, with its partial coverage named. + stale = _render_board_sequence( + [{"payload": _observation_payload([_observation_fixture("stale")])}] + )[0]["worklist"] + self.assertIn("stale observation", stale) + self.assertIn("Source stale: remote_session", stale) + self.assertIn("Partial coverage: remote_session", stale) + self.assertNotIn("live, observed", stale) + + def test_partial_coverage_reports_counts_and_never_a_percentage_or_eta(self) -> None: + record = _observation_fixture("observed_running") + record["work"]["measurements"]["elapsed_seconds"] = { + "value": 120.0, + "coverage": "partial", + "observed": 2, + "total": 5, + } + record = board_observation.validate(record) + nodes = _render_board_sequence([{"payload": _observation_payload([record])}])[0] + self.assertIn("elapsed 120.0s from 2 of 5 recorded", nodes["worklist"]) + self.assertIn("cost not recorded", nodes["worklist"]) + + # Nothing the operator is shown states a share, a percentage, or a + # projection of work that has not been observed. + rendered = "\n".join(nodes.values()) + self.assertNotIn("%", rendered) + for invented in ("percent", "estimat", "remaining", "projected", "eta "): + self.assertNotIn(invented, rendered.lower()) + # A partial measurement is reported as counted evidence, never scaled. + self.assertEqual( + _eval_board_view( + "measurementText({value: 2, coverage: 'partial', observed: 2, total: 5}, 'count')" + ), + "2 from 2 of 5 recorded", + ) + + def test_unknown_states_stay_neutral_and_colour_always_carries_text(self) -> None: + classes = _eval_board_view( + "['unknown', 'not_started', 'absent', 'unverifiable', 'none', 'unassigned']" + ".map(state => EVIDENCE_STATE_CLASSES[state])" + ) + self.assertEqual(set(classes), {"muted"}) + nodes = _render_board_sequence( + [{"payload": _observation_payload([_observation_fixture("observed_running")])}] + )[0] + detail = nodes["worklist"] + # Every coloured pill carries a text cue and a text label beside it. + for coloured in re.findall(r'(.*?)\s*', detail): + self.assertIn('class="cue"', coloured[1] + "") + self.assertEqual(detail.count(''), detail.count(''), detail.count(' None: + record = _observation_fixture("ready") + # No open PR is recorded locally, so no PR link may be offered. + without_link = _render_board_sequence([{"payload": _observation_payload([record])}])[0] + self.assertIn("PR #946, no local link recorded", without_link["worklist"]) + self.assertNotIn("https://github.com/codemower-ai/code-mower/pull/946", without_link["worklist"]) + + payload = _observation_payload([record]) + payload["remote"]["pull_requests"] = [ + { + "number": 946, + "url": "https://github.example/owner/repo/pull/946", + "title": "t", + "labels": {}, + "checks": [], + } + ] + with_link = _render_board_sequence([{"payload": payload}])[0]["worklist"] + self.assertIn('href="https://github.example/owner/repo/pull/946">Open PR #946', with_link) + self.assertIn("This Board never merges, requeues, cancels, retries", with_link) + # The page has no form, no non-GET request, and reaches only the two + # read-only local endpoints. + page = board.render_board_html(board.BoardConfig(repo="owner/repo")) + self.assertNotIn(" None: + html = board.render_board_html(board.BoardConfig(repo="owner/repo")) + # One detail node, rendered inside the selected row, so single-column + # source order already puts it under the row it belongs to. + nodes = _render_board_sequence( + [ + { + "payload": _observation_payload( + [_observation_fixture("observed_running"), _observation_fixture("ready")] + ) + } + ] + )[0] + self.assertEqual(nodes["worklist"].count('id="workdetail"'), 1) + selected = nodes["worklist"].split('
  • ')[1] + self.assertLess(selected.index(""), selected.index('id="workdetail"')) + self.assertIn("@media (min-width: 900px) {", html) + self.assertIn(".workdetail { position:absolute;", html) + + def test_participants_report_recorded_phases_without_claiming_liveness(self) -> None: + nodes = _render_board_sequence( + [ + { + "payload": _observation_payload( + [_observation_fixture("observed_running"), _observation_fixture("unlinked")] + ) + } + ] + )[0] + participants = nodes["participants"] + self.assertIn("codex", participants) + self.assertIn("observed running 1", participants) + self.assertIn("claude", participants) + self.assertIn("not linked to a session 1", participants) + self.assertIn("not a claim that anything is running now", participants) + + def test_health_view_reports_connections_version_and_process_state(self) -> None: + payload = _observation_payload( + [_observation_fixture("stale"), _observation_fixture("observed_running")] + ) + payload["board"]["version"]["installed_version"] = "1.5.0" + payload["board"]["version"]["restart_recommended"] = True + payload["board"]["cache"]["state"] = "stale" + payload["local_boards"] = {"boards": [{"port": 5332, "pid": 42, "cwd": ""}]} + nodes = _render_board_sequence([{"payload": payload}])[0] + self.assertIn("restart recommended", nodes["diagnostics"]) + self.assertIn("installed 1.5.0", nodes["diagnostics"]) + self.assertIn("Snapshot cache", nodes["diagnostics"]) + self.assertIn("2 recorded", nodes["diagnostics"]) + self.assertIn("remote_session", nodes["sources"]) + self.assertIn("coverage partial", nodes["sources"]) + self.assertIn("board localhost:5332", nodes["local"]) + + def test_empty_observation_directory_is_reported_as_nothing_recorded(self) -> None: + payload = _observation_payload([]) + payload["observations"]["path_exists"] = False + nodes = _render_board_sequence([{"payload": payload}])[0] + self.assertIn("No local Board observation is recorded yet", nodes["worklist"]) + self.assertNotIn("idle", nodes["worklist"]) + rejected = _observation_payload([]) + rejected["observations"]["rejected"] = 1 + rejected["observations"]["message"] = "no local Board observation passed the observation contract" + rejected_nodes = _render_board_sequence([{"payload": rejected}])[0] + self.assertIn("passed the observation contract", rejected_nodes["worklist"]) + self.assertIn("1 rejected by the observation contract", rejected_nodes["diagnostics"]) From 6d6e73fe8e23697f5afa9cdb3ddfb5cad2a484c7 Mon Sep 17 00:00:00 2001 From: Jeff Huber Date: Wed, 16 Sep 2026 06:28:46 -0700 Subject: [PATCH 02/20] Board: match repository identity, deduplicate linked observations, bound reads Three narrow fixes from the exact-head merge-authority audit of ac9ca53c. Match repository identity before attaching a PR link. A custom observations directory can hold a record another repository produced, where the same pull request number means a different pull request; attaching this repository's URL to it on the number alone linked unrelated work. A link is now offered only when the record names this Board's own repository, and a foreign record is still shown for exactly what it is -- named as belonging to that other repository -- without a link being invented for it. Deduplicate linked work observations by their opaque identity. An older file and the file that replaced it are one work item, not two rows competing for one row id and one detail region. The newest observation wins, chosen on recorded `created_at` with the last meaningful update and then the row signature as deterministic tiebreaks, so which file the directory listed first cannot change what is rendered. Change tracking consumes the same deduplicated set, so reordering duplicates is no longer announced as news. The existing unlinked consolidation semantics are unchanged. Bound observation reads before JSON decoding. Each file is now read with a single request of at most MAX_BYTES + 1 bytes and an oversize file is rejected on that length, so the remainder is never loaded and never reaches the decoder. The rejection reuses the contract's own closed `invalid_contract` diagnostic, which names no path and repeats no value. Regression tests cover foreign-repository same-number link suppression, one row and one detail region per duplicated linked identity with the newest observation selected, and a bounded read that rejects an oversize file before decode. Co-Authored-By: Claude Opus 5 --- docs/board-data-contract.md | 22 ++++- src/code_mower/board.py | 66 +++++++++++--- tests/test_board.py | 168 ++++++++++++++++++++++++++++++++++++ 3 files changed, 243 insertions(+), 13 deletions(-) diff --git a/docs/board-data-contract.md b/docs/board-data-contract.md index f589048b..bb275b72 100644 --- a/docs/board-data-contract.md +++ b/docs/board-data-contract.md @@ -486,7 +486,14 @@ can stand in for another. The gate publisher is shown beside the Selection is kept by opaque work identity — session, worktree, and work id — not by row position, so a refresh that reorders, adds, or drops rows leaves the -operator's choice where it was. There is exactly one detail region. It is +operator's choice where it was. One identity is one row: where the directory +holds several observations of the same work item, the newest by recorded +`created_at` is rendered — with the last meaningful update and then the row +signature as deterministic tiebreaks — so which file the directory listed first +cannot change what is shown. Change tracking compares the same deduplicated +set, so there is exactly one row id and one detail region per identity however +many files describe it. Several `unlinked` observations in one scope still +consolidate into one row, unchanged. There is exactly one detail region. It is rendered inside the selected row, so at phone widths it follows the row it belongs to, and at desktop widths CSS places that same region adjacent to the list. Rows are buttons carrying `aria-expanded` and `aria-controls`; Up, Down, @@ -503,7 +510,12 @@ nothing and adds no Timeline entry. Primary actions stay read-only: open a recorded PR link, inspect a connection in Health, and view recent changes. A pull request number observed locally is -never turned into a remote address the payload has not recorded. There is no +never turned into a remote address the payload has not recorded, and a link is +offered only when the record names this Board's own repository. A custom +observations directory can hold a record another repository produced, where the +same pull request number means a different pull request; such a record is still +shown for what it is, named as belonging to that other repository, without a +link. There is no merge, requeue, force-lease, cancel, retry, restart, cloud-schema or Slack-specific control, no form, and no non-GET request. @@ -542,7 +554,11 @@ By default the Board reads `*.json` files under `.code-mower/board/observations/`. Use `--observations-path PATH` for a custom local directory. A missing directory is reported as "nothing recorded yet", which is a different statement from "no work". Reading is bounded to 32 files -per refresh, and the observation contract itself bounds each record. +per refresh, and the observation contract itself bounds each record to +`MAX_BYTES`. Each file is read with a single bounded request of at most +`MAX_BYTES + 1` bytes: a file larger than the cap is rejected on the length of +what was asked for, with the contract's own `invalid_contract` diagnostic, and +its remainder is never loaded or decoded. The block carries: diff --git a/src/code_mower/board.py b/src/code_mower/board.py index 5fc3994a..c287073a 100644 --- a/src/code_mower/board.py +++ b/src/code_mower/board.py @@ -1216,12 +1216,22 @@ def observations_payload(config: BoardConfig) -> dict[str, Any]: payload["message"] = "could not read local Board observations" return payload for record_file in candidates: + # The contract bounds a record to MAX_BYTES, so at most one byte past + # that bound is ever read: an oversize file is rejected on the length + # of what was asked for, without the remainder being loaded or decoded. try: - raw = record_file.read_bytes() + with record_file.open("rb") as handle: + raw = handle.read(board_observation.MAX_BYTES + 1) except OSError: payload["rejected"] += 1 payload["warnings"].append({"file": record_file.name, "message": "could not read observation file"}) continue + if len(raw) > board_observation.MAX_BYTES: + # The same closed diagnostic the contract itself raises for an + # over-long record; it names no path and repeats no value. + payload["rejected"] += 1 + payload["warnings"].append({"file": record_file.name, "message": "invalid_contract"}) + continue try: payload["records"].append(board_observation.decode(raw)) except board_observation.BoardObservationError as exc: @@ -2302,20 +2312,49 @@ def timelines_payload( // Deterministic order: most blocking first, then a stable tiebreak on the // opaque identity so an unchanged snapshot never reshuffles the list. const ROW_RANK = new Map(STATE_RULES.map(rule => [rule.label, rule.rank])); + // How recent one observation of a linked identity is. `created_at` is when + // the observation itself was recorded, so it is what orders two + // observations of the same work item; the last meaningful update breaks a + // tie, and the signature breaks that, so the winner never depends on the + // order the directory happened to be listed in. + function observationOrder(row) { + const created = parseMs(row.record?.created_at); + return [created === null ? -Infinity : created, row.update.at === null ? -Infinity : row.update.at]; + } + function isNewerObservation(candidate, existing) { + const [candidateCreated, candidateUpdate] = observationOrder(candidate); + const [existingCreated, existingUpdate] = observationOrder(existing); + if (candidateCreated !== existingCreated) return candidateCreated > existingCreated; + if (candidateUpdate !== existingUpdate) return candidateUpdate > existingUpdate; + return candidate.signature.localeCompare(existing.signature) > 0; + } function workRows(data, nowMs) { const rows = records(data).map(record => workRow(record, nowMs)); const merged = []; - // Several unlinked observations describe one condition in one scope, so - // they render as one row rather than as competing rows. + const byKey = new Map(); for (const row of rows) { - const existing = row.kind === "unlinked" ? merged.find(item => item.key === row.key) : undefined; + const existing = byKey.get(row.key); if (existing === undefined) { + byKey.set(row.key, row); merged.push(row); continue; } - existing.assignments = [...existing.assignments, ...row.assignments]; - existing.groups[0].items = [...existing.groups[0].items, ...row.groups[0].items]; - existing.signature = `${existing.signature}||${row.signature}`; + // Several unlinked observations describe one condition in one scope, so + // they render as one row rather than as competing rows. + if (row.kind === "unlinked") { + existing.assignments = [...existing.assignments, ...row.assignments]; + existing.groups[0].items = [...existing.groups[0].items, ...row.groups[0].items]; + existing.signature = `${existing.signature}||${row.signature}`; + continue; + } + // A linked identity is one work item however many observations of it + // are on disk. An older file and the file that replaced it are not two + // items competing for one row id and one detail region: the newest + // observation is the one rendered, and the older one is dropped rather + // than merged into it. + if (!isNewerObservation(row, existing)) continue; + merged[merged.indexOf(existing)] = row; + byKey.set(row.key, row); } return merged.sort((a, b) => (ROW_RANK.get(a.headline) ?? 99) - (ROW_RANK.get(b.headline) ?? 99) @@ -2537,11 +2576,15 @@ def timelines_payload( activeView = id; applyView(); } - // Only a URL the payload actually recorded is ever offered. A PR number - // observed locally is never turned into a remote address the Board has not - // been told about. + // Only a URL the payload actually recorded, for a record that names this + // Board's own repository, is ever offered. A PR number observed locally is + // never turned into a remote address the Board has not been told about, + // and an observation directory that holds a record for another repository + // never borrows this repository's pull request just because the numbers + // happen to match. function recordedPrUrl(row, prs) { if (row.pr_number === null || row.pr_number === undefined) return ""; + if (row.repository !== REPO) return ""; const match = arrayOf(prs).find(pr => pr?.number === row.pr_number); const url = match ? text(match.url) : ""; return href(url) === "#" ? "" : url; @@ -2550,6 +2593,9 @@ def timelines_payload( const url = recordedPrUrl(row, prs); const actions = []; if (url) actions.push(`Open PR #${esc(row.pr_number)}`); + // A foreign record is still shown for what it is; what it does not get + // is a link this Board has no record of. + else if (row.pr_number !== null && row.pr_number !== undefined && row.repository !== REPO) actions.push(`PR #${esc(row.pr_number)} in ${esc(row.repository || "an unrecorded repository")}, not this repository; no local link recorded`); else if (row.pr_number !== null && row.pr_number !== undefined) actions.push(`PR #${esc(row.pr_number)}, no local link recorded`); actions.push(``); actions.push(``); diff --git a/tests/test_board.py b/tests/test_board.py index 0a537c35..c7f19a20 100644 --- a/tests/test_board.py +++ b/tests/test_board.py @@ -3759,6 +3759,25 @@ def _selected_key(worklist: str) -> str: return match.group(1) if match else "" +class _RecordingHandle: + """A file handle that records the size of every read it is asked for.""" + + def __init__(self, handle: object, reads: list[int]) -> None: + self._handle = handle + self._reads = reads + + def __enter__(self) -> "_RecordingHandle": + self._handle.__enter__() + return self + + def __exit__(self, *exc: object) -> object: + return self._handle.__exit__(*exc) + + def read(self, size: int = -1) -> bytes: + self._reads.append(size) + return self._handle.read(size) + + class BoardObservationReaderTests(TestCase): """The Board consumes the frozen observation contract; it never writes one.""" @@ -3809,6 +3828,57 @@ def test_reading_is_bounded(self) -> None: ) self.assertEqual(len(payload["records"]), board.MAX_OBSERVATION_FILES) + def test_an_oversize_file_is_read_bounded_and_rejected_before_it_is_decoded(self) -> None: + cap = board_observation.MAX_BYTES + # Warm the contract's own schema read so it cannot be mistaken for one + # of the observation reads being measured here. + board_observation.schema() + reads: list[int] = [] + decoded: list[int] = [] + real_open = Path.open + real_decode = board_observation.decode + + with tempfile.TemporaryDirectory() as tmp: + directory = Path(tmp) + self._write(directory, "a-small.json", _observation_fixture("no_work")) + # Far past the contract's cap, so there is a real remainder that + # must never be pulled into memory. + (directory / "b-huge.json").write_bytes(b'{"padding":"' + b"x" * (cap * 4) + b'"}') + + def recording_open(self: Path, *args: object, **kwargs: object) -> object: + handle = real_open(self, *args, **kwargs) + return _RecordingHandle(handle, reads) if self.parent == directory else handle + + def recording_decode(raw: bytes) -> object: + decoded.append(len(raw)) + return real_decode(raw) + + with patch.object(Path, "open", recording_open), patch.object( + board.board_observation, "decode", recording_decode + ): + payload = board.observations_payload( + board.BoardConfig(repo="owner/repo", observations_path=str(directory)) + ) + + # Every observation file is read with the same bounded request: one + # byte past the cap, which is all it takes to know the file is too big. + self.assertEqual(reads, [cap + 1, cap + 1]) + # Only the record inside the cap ever reached the decoder, and it was + # never handed more bytes than the contract allows. + self.assertEqual(len(decoded), 1) + self.assertLessEqual(decoded[0], cap) + + self.assertEqual(len(payload["records"]), 1) + self.assertEqual(payload["records"][0]["kind"], "no_work") + self.assertEqual(payload["rejected"], 1) + self.assertEqual( + payload["warnings"], [{"file": "b-huge.json", "message": "invalid_contract"}] + ) + # The diagnostic carries no path and no content from the file. + self.assertEqual(payload["path"], lane_status.LOCAL_PATH_REDACTION) + self.assertNotIn("x" * 32, json.dumps(payload)) + self.assertNotIn(str(directory), json.dumps(payload)) + def test_observations_are_not_written_into_local_history(self) -> None: snapshot = {"schema": "code_mower.laneStatus.v1", "observations": {"records": [1]}} self.assertNotIn("observations", board._recordable_payload(snapshot)) @@ -4149,6 +4219,104 @@ def test_actions_are_read_only_and_never_invent_a_remote_link(self) -> None: ["/api/events", "/api/status"], ) + def test_a_same_numbered_pull_request_in_another_repository_is_never_linked(self) -> None: + # A custom observations directory can hold a record another repository + # produced. Its PR number is not this repository's PR number, so this + # repository's link may not be attached to it. + foreign = _observation_fixture("ready") + foreign["scope"]["repository"] = "other-org/other-repo" + for run in foreign["work"]["runs"]: + run["binding"]["repository"] = "other-org/other-repo" + # The foreign record is entirely self-consistent: the contract accepts + # it, and it names a repository that is not this one. + foreign = board_observation.validate(foreign) + payload = _observation_payload([foreign]) + payload["remote"]["pull_requests"] = [ + { + "number": 946, + "url": "https://github.example/codemower-ai/code-mower/pull/946", + "title": "t", + "labels": {}, + "checks": [], + } + ] + worklist = _render_board_sequence([{"payload": payload}])[0]["worklist"] + self.assertNotIn("Open PR #946", worklist) + self.assertNotIn("https://github.example/codemower-ai/code-mower/pull/946", worklist) + # The foreign record is still shown for exactly what it is. + self.assertIn( + "PR #946 in other-org/other-repo, not this repository; no local link recorded", + worklist, + ) + + # The identical payload for this repository does get the recorded link, + # so the suppression above is the repository check and nothing else. + local = copy.deepcopy(payload) + local["observations"]["records"] = [_observation_fixture("ready")] + self.assertEqual( + local["observations"]["records"][0]["scope"]["repository"], "codemower-ai/code-mower" + ) + local_worklist = _render_board_sequence([{"payload": local}])[0]["worklist"] + self.assertIn( + 'href="https://github.example/codemower-ai/code-mower/pull/946">Open PR #946', + local_worklist, + ) + + def test_duplicate_observations_of_one_identity_render_the_newest_once(self) -> None: + # Two files in the directory observe the same work item: an older one + # and the one that replaced it. + older = _observation_fixture("ready") + newer = _observation_fixture("merged") + for record in (older, newer): + record["work"]["id"] = "readywork" + record["work"]["runs"][0]["binding"]["work_id"] = "readywork" + older["created_at"] = "2026-09-12T20:00:00Z" + newer["created_at"] = "2026-09-12T20:00:20Z" + # Both remain records the frozen contract accepts. + older = board_observation.validate(older) + newer = board_observation.validate(newer) + scope = older["scope"] + identity = f"work:{scope['session_id']}:{scope['worktree_id']}:readywork" + + frames = _render_board_sequence( + [ + {"payload": _observation_payload([older, newer])}, + # The same two files listed the other way round. + {"payload": _observation_payload([newer, older])}, + ] + ) + worklist = frames[0]["worklist"] + # One identity is one row and one detail region, not two competing ones. + self.assertEqual(_work_keys(worklist), [identity]) + self.assertEqual(worklist.count('class="rowbtn" id='), 1) + self.assertEqual(worklist.count('id="workdetail"'), 1) + # The newest observation is the one rendered. + self.assertIn("stage: merged", worklist) + self.assertNotIn("stage: ready to merge", worklist) + self.assertEqual(_selected_key(worklist), identity) + + # Which file the directory happened to list first cannot change the row. + self.assertEqual(frames[1]["worklist"], worklist) + # Change tracking reads the same deduplicated set, so reordering the + # duplicates is not news. + self.assertEqual(frames[1].get("announce", ""), "") + self.assertIn("not listed here", frames[1]["changes"]) + + # A genuinely newer observation of the same identity is still a change. + newest = copy.deepcopy(newer) + newest["created_at"] = "2026-09-12T20:00:25Z" + newest["work"]["stage"] = "in_review" + newest = board_observation.validate(newest) + moved = _render_board_sequence( + [ + {"payload": _observation_payload([older, newer])}, + {"payload": _observation_payload([older, newest])}, + ] + )[1] + self.assertEqual(_work_keys(moved["worklist"]), [identity]) + self.assertIn("stage: in review", moved["worklist"]) + self.assertIn("issue-946", moved["announce"]) + def test_mobile_detail_follows_the_row_and_desktop_places_it_adjacent(self) -> None: html = board.render_board_html(board.BoardConfig(repo="owner/repo")) # One detail node, rendered inside the selected row, so single-column From c44b9137ea62109e5846f3bb2c451b5532b1f3d8 Mon Sep 17 00:00:00 2001 From: Jeff Huber Date: Wed, 16 Sep 2026 06:49:29 -0700 Subject: [PATCH 03/20] Board: summarize participants and consolidate unlinked rows from one deduplicated set Two adjacent P2 consistency findings from the independent exact-head audit of 6d6e73fe. Both are fixed by giving the work-first views a single shared normalization and deduplication step, so the work list, the participant summary and change tracking read the same observation set and cannot drift apart again. Deduplicate observations before summarizing participants. The participant strip walked the raw records on disk, so successive observation files of one work identity counted the same run once per file and went on reporting the phase that run had already left: one run seen dispatched and then running read as two runs, one of them still dispatched. Participants are now built from the runs the rendered rows were built from, resolved against those rows' own sources, so a superseded observation contributes nothing. Recompute freshness and update metadata when consolidating unlinked observations. Consolidating several unlinked observations of one repository merged their assignments and evidence items but kept the first record's freshness, age, last meaningful update and signature, so a fresh first file hid an unavailable source behind the very evidence being shown, a later recorded event was lost, and reversing the directory listing changed the row. The consolidated row is now read off one record recomputed from all retained evidence: per source id the worst freshness and coverage any retained file reported, carrying the newest time each of them recorded; the age of the oldest retained observation, or no claimed age at all when one of them records none; and one entry per run however many files observed it, kept as the worst-attested of those observations. Records within a group are ordered by what they record, never by the order the directory listed them, so the consolidated row is byte-identical under any input order. The frozen code_mower.boardObservation.v1 contract is untouched: consolidation is a presentation step over records the contract has already accepted, and the Board still never produces, resolves or repairs one. All three fixes from the previous audit round are preserved. Regression tests cover one run transitioning phase across two observations counting once at the newest phase under either file order, and consolidated unlinked observations that are byte-identical under reversal, that cannot hide an unavailable source behind a fresh first record, and that retain the later meaningful update. Co-Authored-By: Claude Opus 5 --- docs/board-data-contract.md | 9 +- src/code_mower/board.py | 181 ++++++++++++++++++++++++++---------- tests/test_board.py | 97 +++++++++++++++++++ 3 files changed, 239 insertions(+), 48 deletions(-) diff --git a/docs/board-data-contract.md b/docs/board-data-contract.md index bb275b72..b28bf0cd 100644 --- a/docs/board-data-contract.md +++ b/docs/board-data-contract.md @@ -493,7 +493,14 @@ signature as deterministic tiebreaks — so which file the directory listed firs cannot change what is shown. Change tracking compares the same deduplicated set, so there is exactly one row id and one detail region per identity however many files describe it. Several `unlinked` observations in one scope still -consolidate into one row, unchanged. There is exactly one detail region. It is +consolidate into one row, and that row is recomputed from all of the evidence +retained for it: the worst freshness and coverage any retained source reported, +the age of the oldest retained observation, the newest recorded event or +observation as the last meaningful update, and one entry per run however many +files observed it, kept as the worst-attested of those observations. The +participant summary is built from this same deduplicated set, so a run that has +moved phase is counted once, in the phase the newest observation records, and +never again in the phase it has left. There is exactly one detail region. It is rendered inside the selected row, so at phone widths it follows the row it belongs to, and at desktop widths CSS places that same region adjacent to the list. Rows are buttons carrying `aria-expanded` and `aria-controls`; Up, Down, diff --git a/src/code_mower/board.py b/src/code_mower/board.py index c287073a..7faf0fd6 100644 --- a/src/code_mower/board.py +++ b/src/code_mower/board.py @@ -1902,6 +1902,7 @@ def timelines_payload( // asserting the state of the world. const OBSERVATION_STALE_SECONDS = 600; const FRESHNESS_RANK = {fresh: 0, stale: 1, unavailable: 2}; + const SOURCE_COVERAGE_RANK = {complete: 0, partial: 1, unavailable: 2}; const FRESHNESS_CLASSES = {fresh: "ok", stale: "warn", unavailable: "bad"}; const arrayOf = (value) => (Array.isArray(value) ? value : []); const records = (data) => arrayOf(data?.observations?.records); @@ -1918,6 +1919,16 @@ def timelines_payload( } return best; }; + // Which of two recorded times is the later one, answered as the recorded + // string rather than as a number, so a consolidated reading keeps the exact + // timestamp a producer wrote instead of a reformatting of it. + const laterTimestamp = (left, right) => { + const at = parseMs(left); + const other = parseMs(right); + if (other === null) return at === null ? null : left; + if (at === null) return right; + return other > at ? right : left; + }; // `checked_at` advances on every poll whether or not anything happened, so // it can never be the last meaningful update. Only a recorded event time // can be -- and, when a record carries none, the time something was last @@ -2208,9 +2219,22 @@ def timelines_payload( const key = workKey(record); const freshness = recordFreshness(record, nowMs); const update = lastMeaningfulUpdate(record); + // Every run this row is built from, with the freshness of the source + // behind it resolved here, where that record's source index is already in + // hand. The participant summary reads these rather than the records on + // disk, so it counts exactly the runs the rendered rows were built from + // and nothing a newer observation has already superseded. + const runSources = sourceIndex(record); + const participants = [...arrayOf(record?.work?.runs), ...arrayOf(record?.unlinked)].map(run => ({ + provider: text(run?.provider) || "unknown", + role: text(run?.role) || "unknown", + phase: text(run?.phase) || "not linked to a session", + freshness: text(runSources[text(run?.source_id)]?.freshness) || "unavailable" + })); const base = { key, kind, + participants, signature: workSignature(record), repository: text(record?.scope?.repository), freshness, @@ -2317,68 +2341,131 @@ def timelines_payload( // observations of the same work item; the last meaningful update breaks a // tie, and the signature breaks that, so the winner never depends on the // order the directory happened to be listed in. - function observationOrder(row) { - const created = parseMs(row.record?.created_at); - return [created === null ? -Infinity : created, row.update.at === null ? -Infinity : row.update.at]; + function observationOrder(record) { + const created = parseMs(record?.created_at); + const update = lastMeaningfulUpdate(record); + return [created === null ? -Infinity : created, update.at === null ? -Infinity : update.at]; } function isNewerObservation(candidate, existing) { const [candidateCreated, candidateUpdate] = observationOrder(candidate); const [existingCreated, existingUpdate] = observationOrder(existing); if (candidateCreated !== existingCreated) return candidateCreated > existingCreated; if (candidateUpdate !== existingUpdate) return candidateUpdate > existingUpdate; - return candidate.signature.localeCompare(existing.signature) > 0; - } - function workRows(data, nowMs) { - const rows = records(data).map(record => workRow(record, nowMs)); - const merged = []; - const byKey = new Map(); - for (const row of rows) { - const existing = byKey.get(row.key); + return workSignature(candidate).localeCompare(workSignature(existing)) > 0; + } + // The one deduplicated observation set the work-first views read. The work + // list, the participant summary and change tracking all consume this and + // nothing else, so none of them can go on counting an observation the + // others have already dropped. + // + // A linked identity is one work item however many observations of it are on + // disk, so only the newest is retained and the superseded ones are dropped. + // Several unlinked observations describe one condition in one repository, + // so all of them are retained and consolidated into one row. Records within + // a group are ordered by what they record, never by the order the directory + // happened to list the files in. + function observationGroups(data) { + const groups = new Map(); + for (const record of records(data)) { + const key = workKey(record); + const existing = groups.get(key); if (existing === undefined) { - byKey.set(row.key, row); - merged.push(row); + groups.set(key, {key, kind: text(record?.kind), records: [record]}); continue; } - // Several unlinked observations describe one condition in one scope, so - // they render as one row rather than as competing rows. - if (row.kind === "unlinked") { - existing.assignments = [...existing.assignments, ...row.assignments]; - existing.groups[0].items = [...existing.groups[0].items, ...row.groups[0].items]; - existing.signature = `${existing.signature}||${row.signature}`; - continue; + if (existing.kind === "unlinked") existing.records.push(record); + else if (isNewerObservation(record, existing.records[0])) existing.records = [record]; + } + for (const group of groups.values()) { + group.records.sort((a, b) => + workSignature(a).localeCompare(workSignature(b)) + || text(a?.created_at).localeCompare(text(b?.created_at)) + || text(a?.scope?.repository).localeCompare(text(b?.scope?.repository))); + } + return [...groups.values()]; + } + // One source id names one source, so a source observed in several retained + // files is one source here too. Its consolidated reading is the + // conservative one -- the worst freshness and the worst coverage any + // retained file reported -- carrying the newest time each of them recorded. + // Combining a fresh observation with an unavailable one can therefore + // neither hide the unavailability nor lose the later update. + function consolidatedSource(existing, source) { + if (existing === undefined) return {...source}; + const merged = {...existing}; + if ((FRESHNESS_RANK[text(source?.freshness)] ?? 3) > (FRESHNESS_RANK[text(existing.freshness)] ?? 3)) merged.freshness = source.freshness; + if ((SOURCE_COVERAGE_RANK[text(source?.coverage)] ?? 3) > (SOURCE_COVERAGE_RANK[text(existing.coverage)] ?? 3)) merged.coverage = source.coverage; + merged.event_at = laterTimestamp(existing.event_at, source?.event_at); + merged.observed_at = laterTimestamp(existing.observed_at, source?.observed_at); + merged.heartbeat_at = laterTimestamp(existing.heartbeat_at, source?.heartbeat_at); + merged.checked_at = laterTimestamp(existing.checked_at, source?.checked_at); + return merged; + } + // The single record one row is read off. One retained observation is + // itself; several retained observations of one unlinked condition are + // consolidated into one, so the row's freshness, age, last meaningful + // update and signature are recomputed from all of the evidence behind it + // rather than inherited from whichever file happened to be read first. + function consolidatedRecord(group) { + const [first, ...rest] = group.records; + if (!rest.length) return first; + const sources = new Map(); + const unlinked = new Map(); + for (const record of group.records) { + for (const source of arrayOf(record?.sources)) { + const id = text(source?.id); + sources.set(id, consolidatedSource(sources.get(id), source)); + } + // One run observed in several files is one run, not one run per file. + // The observation kept for it is the worst-attested one, so a run whose + // source has since gone unavailable cannot go on reading as fresh + // because an earlier file still had it. + const runSources = sourceIndex(record); + for (const run of arrayOf(record?.unlinked)) { + const id = text(run?.id); + const rank = FRESHNESS_RANK[text(runSources[text(run?.source_id)]?.freshness)] ?? 3; + const existing = unlinked.get(id); + if (existing === undefined || rank > existing.rank) unlinked.set(id, {rank, run}); } - // A linked identity is one work item however many observations of it - // are on disk. An older file and the file that replaced it are not two - // items competing for one row id and one detail region: the newest - // observation is the one rendered, and the older one is dropped rather - // than merged into it. - if (!isNewerObservation(row, existing)) continue; - merged[merged.indexOf(existing)] = row; - byKey.set(row.key, row); } - return merged.sort((a, b) => - (ROW_RANK.get(a.headline) ?? 99) - (ROW_RANK.get(b.headline) ?? 99) - || a.reference.localeCompare(b.reference) - || a.key.localeCompare(b.key)); + // The oldest retained observation sets the age, and an observation that + // records no time at all leaves the consolidated row unable to claim one, + // so a consolidated row never reads as more current than the oldest + // evidence in it. + const created = group.records.map(record => ({raw: text(record?.created_at), at: parseMs(record?.created_at)})); + const byId = (a, b) => text(a?.id).localeCompare(text(b?.id)); + return { + ...first, + created_at: created.some(item => item.at === null) + ? "" + : created.reduce((oldest, item) => (item.at < oldest.at ? item : oldest)).raw, + sources: [...sources.values()].sort(byId), + unlinked: [...unlinked.values()].map(item => item.run).sort(byId) + }; + } + function workRows(data, nowMs) { + return observationGroups(data) + .map(group => workRow(consolidatedRecord(group), nowMs)) + .sort((a, b) => + (ROW_RANK.get(a.headline) ?? 99) - (ROW_RANK.get(b.headline) ?? 99) + || a.reference.localeCompare(b.reference) + || a.key.localeCompare(b.key)); } // Who is recorded as taking part, and in what phase. Nothing is inferred: // a participant is only ever reported in the phases its own runs record, - // alongside how fresh the source behind them is. - function participantSummary(data) { + // alongside how fresh the source behind them is. The summary is built from + // the same deduplicated rows the work list renders, so an observation a + // newer file has replaced can neither count its runs a second time nor keep + // reporting a phase the work has already moved past. + function participantSummary(rows) { const summary = new Map(); - for (const record of records(data)) { - const sources = sourceIndex(record); - const runs = [...arrayOf(record?.work?.runs), ...arrayOf(record?.unlinked)]; - for (const run of runs) { - const provider = text(run?.provider) || "unknown"; - const role = text(run?.role) || "unknown"; - const key = `${provider}/${role}`; - const entry = summary.get(key) || {provider, role, count: 0, freshness: "fresh", phases: new Map()}; - const phase = text(run?.phase) || "not linked to a session"; - entry.phases.set(phase, (entry.phases.get(phase) || 0) + 1); + for (const row of rows) { + for (const run of row.participants) { + const key = `${run.provider}/${run.role}`; + const entry = summary.get(key) || {provider: run.provider, role: run.role, count: 0, freshness: "fresh", phases: new Map()}; + entry.phases.set(run.phase, (entry.phases.get(run.phase) || 0) + 1); entry.count += 1; - const freshness = text(sources[text(run?.source_id)]?.freshness) || "unavailable"; - if ((FRESHNESS_RANK[freshness] ?? 3) > (FRESHNESS_RANK[entry.freshness] ?? 3)) entry.freshness = freshness; + if ((FRESHNESS_RANK[run.freshness] ?? 3) > (FRESHNESS_RANK[entry.freshness] ?? 3)) entry.freshness = run.freshness; summary.set(key, entry); } } @@ -2875,7 +2962,7 @@ def timelines_payload( `${esc(observationRows.length)} observed work item${observationRows.length === 1 ? "" : "s"}`, attentionRows.length ? ` ${esc(attentionRows.length)} awaiting a named role` : "" ].filter(Boolean).join("")); - const participants = participantSummary(data); + const participants = participantSummary(observationRows); put("participants", participants.length ? participants.map(participant => `
    ${esc(participant.provider)}${pill(participant.role)}${cuePill(`worst source ${participant.freshness}`, participant.class)}
    ${participant.phases.map(phase => pill(`${phase.label} ${phase.count}`)).join("")}
    ${esc(participant.count)} recorded run${participant.count === 1 ? "" : "s"}; phases are what the records state, not a claim that anything is running now.
    `).join("") : empty("No participant run is recorded in any local observation.")); diff --git a/tests/test_board.py b/tests/test_board.py index c7f19a20..65c552fd 100644 --- a/tests/test_board.py +++ b/tests/test_board.py @@ -4317,6 +4317,103 @@ def test_duplicate_observations_of_one_identity_render_the_newest_once(self) -> self.assertIn("stage: in review", moved["worklist"]) self.assertIn("issue-946", moved["announce"]) + def test_a_run_that_changes_phase_across_observations_counts_once_and_newest(self) -> None: + # One run, observed twice: the file that caught it dispatched, and the + # file that replaced it once the run was seen running. + def phased(fixture_name: str, created_at: str) -> dict: + record = _observation_fixture(fixture_name) + record["created_at"] = created_at + record["work"]["id"] = "phasework" + record["work"]["runs"][0]["id"] = "phaserun" + record["work"]["runs"][0]["binding"]["work_id"] = "phasework" + return board_observation.validate(record) + + older = phased("dispatched", "2026-09-12T20:00:00Z") + newer = phased("observed_running", "2026-09-12T20:00:20Z") + frames = _render_board_sequence( + [ + {"payload": _observation_payload([older, newer])}, + # The same two files listed the other way round. + {"payload": _observation_payload([newer, older])}, + ] + ) + participants = frames[0]["participants"] + # One run, counted once, in the phase the newest observation records. + self.assertIn("1 recorded run;", participants) + self.assertNotIn("2 recorded runs", participants) + self.assertIn('observed running 1', participants) + # The phase the run has already moved past is not still reported. + self.assertNotIn("dispatched 1", participants) + self.assertEqual(participants.count('class="row"'), 1) + # The participant summary reads the same deduplicated set the work list + # does, so file order cannot change either of them. + self.assertEqual(frames[1]["participants"], participants) + self.assertEqual(frames[1]["worklist"], frames[0]["worklist"]) + self.assertIn("assignments: codex builder observed running", frames[0]["worklist"]) + + def test_consolidated_unlinked_observations_recompute_freshness_and_update(self) -> None: + # One unlinked run in one repository, observed twice: a first file whose + # source was fresh, and a later file whose source has gone unavailable + # while preserving a later recorded event. + fresh = _observation_fixture("unlinked") + gone = copy.deepcopy(fresh) + gone["created_at"] = "2026-09-12T20:00:20Z" + gone["sources"] = [ + { + "id": "registryobs", + "kind": "run_registry", + "freshness": "unavailable", + "coverage": "unavailable", + "event_at": "2026-09-12T20:00:05Z", + "observed_at": "2026-09-12T20:00:10Z", + "checked_at": "2026-09-12T20:00:20Z", + "heartbeat_at": None, + } + ] + gone["unlinked"][0]["source_id"] = "registryobs" + gone["unlinked"][0]["observed_at"] = "2026-09-12T20:00:10Z" + # Both remain records the frozen contract accepts. + gone = board_observation.validate(gone) + + frames = _render_board_sequence( + [ + {"payload": _observation_payload([fresh, gone])}, + # The same two files listed the other way round. + {"payload": _observation_payload([gone, fresh])}, + ] + ) + worklist = frames[0]["worklist"] + # Reversing the files cannot change one byte of the consolidated row. + self.assertEqual(frames[1]["worklist"], worklist) + self.assertEqual(frames[1]["participants"], frames[0]["participants"]) + + # A fresh first record cannot hide the unavailable source behind the + # evidence that is being shown next to it. + self.assertIn( + '' + " last observed 30s ago", + worklist, + ) + self.assertNotIn( + ' observed 30s ago', worklist + ) + self.assertIn("Source unavailable: run_registry.", worklist) + # The later meaningful update is the one retained, not the earlier one + # the first file happened to record. + self.assertIn("last update: 25s ago", worklist) + self.assertNotIn("last update: 50s ago", worklist) + self.assertIn("last meaningful update 25s ago", worklist) + + # One run observed in two files is one run, attested by the worst + # source that observed it. + self.assertIn("assignments: claude unknown", worklist) + self.assertNotIn("claude unknown; claude unknown", worklist) + self.assertIn("run_registry, unavailable, unavailable coverage", worklist) + participants = frames[0]["participants"] + self.assertIn("1 recorded run;", participants) + self.assertNotIn("2 recorded runs", participants) + self.assertIn("worst source unavailable", participants) + def test_mobile_detail_follows_the_row_and_desktop_places_it_adjacent(self) -> None: html = board.render_board_html(board.BoardConfig(repo="owner/repo")) # One detail node, rendered inside the selected row, so single-column From 6c8ef9162c3bf8873cb20ca81836f499a204f6b6 Mon Sep 17 00:00:00 2001 From: Jeff Huber Date: Wed, 16 Sep 2026 07:05:48 -0700 Subject: [PATCH 04/20] Board: order rows by urgency and keep the keyboard on detail actions Row order reused the headline precedence in STATE_RULES, where "merged" is rank 0 because it is the truth that describes a merged item best. That made finished work the first row, and the first row is what an operator who has chosen nothing is shown -- so a board holding merged work and work awaiting approval opened on the merged item. Row urgency is now its own explicit ranking: blocked work, then work waiting on a named person, then work whose evidence cannot be trusted, then work in flight, with terminal work placed last explicitly rather than by falling off the end. Headline precedence is untouched, an unranked headline sorts between the ranked and the terminal bands, and the reference and opaque identity still break ties so an unchanged snapshot never reshuffles. The selected row's detail actions -- Open PR, Inspect connection, View recent changes -- were the only focusable controls on the page without ids, so every poll that replaced the row list dropped the keyboard to the document body. They now carry identities derived from the work they act on and the action's own name, which no row id or other action id can collide with, and each names its row as where focus should land if the action stops being offered. When that row is gone too, focus is left where the browser put it rather than handed to an unrelated control. Found in the same pass: activating a detail action that opens another view left focus on a control inside the panel the switch had just hidden, which browsers resolve by dropping focus to the body. Focus now moves to the tab for the view that was opened. Tests execute the shipped page against a DOM shim in which focus is a real question -- replacing a container's markup destroys what was in it -- and drive real clicks and keystrokes through the page's own handlers. Co-Authored-By: Claude Opus 5 --- docs/board-data-contract.md | 24 +++ src/code_mower/board.py | 113 +++++++++- tests/test_board.py | 404 ++++++++++++++++++++++++++++++++++++ 3 files changed, 532 insertions(+), 9 deletions(-) diff --git a/docs/board-data-contract.md b/docs/board-data-contract.md index b28bf0cd..8467cb99 100644 --- a/docs/board-data-contract.md +++ b/docs/board-data-contract.md @@ -484,6 +484,18 @@ each naming the source it came from and how fresh that source is, so no reading can stand in for another. The gate publisher is shown beside the `code-mower/gate` verdict and is labelled as publisher execution only. +Rows are ordered by urgency, which is a separate question from which recorded +truth headlines a row. A merged item headlines as merged because that describes +it best, but it is the least urgent thing on the board, so row order runs +blocked work first, then work waiting on a named person, then work whose +evidence cannot be trusted, then work recorded as in flight, and terminal work +— merged, and an idle session — last by explicit placement. A headline that is +not ranked sorts after everything ranked and before the terminal band. The +reference and then the opaque identity break ties, so an unchanged snapshot +never reshuffles the list. Because the first row is what an operator who has +chosen nothing is shown, the Board opens on work that still needs someone +rather than on work that is finished. + Selection is kept by opaque work identity — session, worktree, and work id — not by row position, so a refresh that reorders, adds, or drops rows leaves the operator's choice where it was. One identity is one row: where the directory @@ -507,6 +519,18 @@ list. Rows are buttons carrying `aria-expanded` and `aria-controls`; Up, Down, Home and End move the selection, tabs wrap with the arrow keys, and every interactive control has a visible focus ring. +A poll replaces the tab strip and the row list, so every control that can hold +the keyboard carries an identity derived from what it acts on rather than from +where it was rendered: the tabs from the view, the rows from the work identity, +and the selected row's actions from that identity and the action's own name. A +refresh therefore returns the keyboard to the same control — restoring without +scrolling, because a refresh must not move the view. When a control is no +longer offered, focus moves only to the row that control named as its owner, +and if that row is gone too the Board leaves focus where the browser put it +rather than handing the keyboard to an unrelated control. Activating a detail +action that opens another view moves focus to that view's tab, because the +control that was activated is inside the panel the switch has just hidden. + Meaningful changes are announced once through a polite live region and listed in Timeline. A change is meaningful when a recorded fact differs: stage, reasons, route, pull request identity, evidence state, measurements, run phase diff --git a/src/code_mower/board.py b/src/code_mower/board.py index 7faf0fd6..cc674de5 100644 --- a/src/code_mower/board.py +++ b/src/code_mower/board.py @@ -2333,9 +2333,64 @@ def timelines_payload( measurements: [] }; } - // Deterministic order: most blocking first, then a stable tiebreak on the - // opaque identity so an unchanged snapshot never reshuffles the list. - const ROW_RANK = new Map(STATE_RULES.map(rule => [rule.label, rule.rank])); + // Row order answers a different question from the headline. The headline + // precedence in STATE_RULES says which recorded truth describes a work + // item best, and "merged" wins that contest outright because a merged item + // is not also usefully described as, say, "CI pending". Row order asks who + // still has to do something, and by that question a merged item is the + // least urgent thing on the board. Reusing headline precedence for the row + // list therefore floats finished work to the top and -- because the first + // row is what an operator who has made no choice is shown -- opens the + // Board on work nobody can act on while blocked work waits below it. + // + // So urgency is its own explicit ranking. Blocked work comes first, then + // work waiting on a named person, then work whose evidence cannot be + // trusted, then work that is simply in flight, and terminal work last. + const ROW_URGENCY_ORDER = [ + // Blocked: an operator has to unblock this before anything else moves. + "source unavailable", + "changes requested", + "CI failed", + "gate failed", + "provider run failed", + "provider run cancelled", + // Actionable: a named person is the only thing this is waiting on. + "waiting for approval", + "waiting for an answer", + "ready to merge", + "ready for human review", + "review requested", + // Untrustworthy evidence: not known to be moving, not known to be stuck. + "stale observation", + "stale review", + "identity unlinked", + "state not recorded", + // In flight: recorded as progressing, so nothing is owed right now. + "review observed running", + "CI pending", + "gate pending", + "provider reported progress", + "provider run observed", + "dispatched", + "implementation complete", + "review passed", + "assigned" + ]; + // Terminal work is placed last explicitly rather than by falling off the + // end of the ranking, so nothing can be finished and urgent at once. + const TERMINAL_ROW_HEADLINES = ["merged", "idle with complete coverage"]; + const ROW_URGENCY = new Map(ROW_URGENCY_ORDER.map((label, index) => [label, index])); + // A headline nobody ranked is neither promoted above recorded work nor + // buried under finished work: it sorts after everything named above and + // before the terminal band. + const UNRANKED_ROW_URGENCY = ROW_URGENCY_ORDER.length; + const TERMINAL_ROW_URGENCY = new Map( + TERMINAL_ROW_HEADLINES.map((label, index) => [label, UNRANKED_ROW_URGENCY + 1 + index])); + function rowUrgency(headline) { + const terminal = TERMINAL_ROW_URGENCY.get(headline); + if (terminal !== undefined) return terminal; + return ROW_URGENCY.get(headline) ?? UNRANKED_ROW_URGENCY; + } // How recent one observation of a linked identity is. `created_at` is when // the observation itself was recorded, so it is what orders two // observations of the same work item; the last meaningful update breaks a @@ -2443,11 +2498,14 @@ def timelines_payload( unlinked: [...unlinked.values()].map(item => item.run).sort(byId) }; } + // Deterministic order: most urgent first, then a stable tiebreak on the + // reference and the opaque identity, so an unchanged snapshot never + // reshuffles the list. function workRows(data, nowMs) { return observationGroups(data) .map(group => workRow(consolidatedRecord(group), nowMs)) .sort((a, b) => - (ROW_RANK.get(a.headline) ?? 99) - (ROW_RANK.get(b.headline) ?? 99) + rowUrgency(a.headline) - rowUrgency(b.headline) || a.reference.localeCompare(b.reference) || a.key.localeCompare(b.key)); } @@ -2630,7 +2688,15 @@ def timelines_payload( // work item across a refresh that reorders the list. The work, worktree and // session components the key is built from cannot contain the separator, so // distinct keys cannot collapse onto one id. - const rowElementId = (key) => `workrow-${text(key).replace(/[^A-Za-z0-9_-]/g, "-")}`; + const keySlug = (key) => text(key).replace(/[^A-Za-z0-9_-]/g, "-"); + const rowElementId = (key) => `workrow-${keySlug(key)}`; + // The detail region of the selected row is replaced wholesale on every + // poll, so its actions need identities of their own for the same reason + // the rows do. The action name comes before the work identity and every + // name is a single hyphen-free token, so no action id can collide with + // another action's id however a work identity happens to be spelled, and + // the `workaction-` prefix keeps them clear of the row buttons. + const actionElementId = (key, name) => `workaction-${name}-${keySlug(key)}`; // A background refresh replaces the tab strip and the row list. Without // this the focused control is destroyed mid-navigation and focus falls to // the document body, so the keyboard position is silently lost every poll. @@ -2639,10 +2705,25 @@ def timelines_payload( function withFocusPreserved(update) { const active = document.activeElement; const activeId = active && typeof active.id === "string" ? active.id : ""; + // Read off the element that is about to be destroyed, so the fallback + // survives the update that removes it. + const fallbackId = active && active.dataset ? text(active.dataset.focusFallback) : ""; update(); if (!activeId) return; const restored = document.getElementById(activeId); - if (restored && typeof restored.focus === "function") restored.focus({preventScroll: true}); + if (restored && typeof restored.focus === "function") { + restored.focus({preventScroll: true}); + return; + } + // The control the keyboard was on no longer exists. Focus moves only to + // the one element that control named as its owner -- the row it belonged + // to -- and never to whatever happens to occupy its former position, so + // an action that disappears can never hand the keyboard to an unrelated + // control. If the owner is gone too, focus is left where the browser put + // it rather than guessed at. + if (!fallbackId) return; + const owner = document.getElementById(fallbackId); + if (owner && typeof owner.focus === "function") owner.focus({preventScroll: true}); } const cuePill = (label, cls) => ` ${esc(label)}`; const stateCue = (state) => cuePill(state.label, state.class); @@ -2679,13 +2760,18 @@ def timelines_payload( function workActionsHtml(row, prs) { const url = recordedPrUrl(row, prs); const actions = []; - if (url) actions.push(`Open PR #${esc(row.pr_number)}`); + // Every focusable action carries an identity derived from the work it + // acts on, and names the row button as where the keyboard should land if + // the action itself stops being offered. + const owner = rowElementId(row.key); + const identity = (name) => `id="${esc(actionElementId(row.key, name))}" data-focus-fallback="${esc(owner)}"`; + if (url) actions.push(`Open PR #${esc(row.pr_number)}`); // A foreign record is still shown for what it is; what it does not get // is a link this Board has no record of. else if (row.pr_number !== null && row.pr_number !== undefined && row.repository !== REPO) actions.push(`PR #${esc(row.pr_number)} in ${esc(row.repository || "an unrecorded repository")}, not this repository; no local link recorded`); else if (row.pr_number !== null && row.pr_number !== undefined) actions.push(`PR #${esc(row.pr_number)}, no local link recorded`); - actions.push(``); - actions.push(``); + actions.push(``); + actions.push(``); return `
    ${actions.join("")}
    Read-only. This Board never merges, requeues, cancels, retries, restarts or takes a lease.
    `; } @@ -2764,6 +2850,15 @@ def timelines_payload( const view = event.target.closest("[data-view]"); if (view) { selectView(view.dataset.view); + // The control that asked for the view sits in the panel this has + // just hidden. Leaving the keyboard on it would park focus inside + // hidden content, which browsers resolve by dropping focus to the + // document body -- so activating "Inspect connection" from the + // keyboard would silently send the operator back to the top of the + // page. Focus moves to the tab for the view that was opened, which + // is where the operator now is. + const tab = document.getElementById(`tab-${view.dataset.view}`); + if (tab && typeof tab.focus === "function") tab.focus(); return; } const button = event.target.closest(".rowbtn"); diff --git a/tests/test_board.py b/tests/test_board.py index 65c552fd..2d8ed3c2 100644 --- a/tests/test_board.py +++ b/tests/test_board.py @@ -3750,10 +3750,153 @@ def _observation_payload(records: list[dict], **overrides: object) -> dict: return payload +# A DOM shim in which focus is a real question. Elements exist because the +# markup that was rendered declared an id; replacing a container's innerHTML +# destroys everything that was inside it, so a focused control that the refresh +# does not render again is genuinely gone and focus falls to the body exactly +# as a browser would drop it. Only the ids the shipped page ships in its static +# shell exist up front, so a lookup for a control that a render removed returns +# nothing rather than conjuring a phantom element to focus. +BOARD_FOCUS_HARNESS = """ +class FakeElement { + constructor(doc, id, attrs, parent) { + this.doc = doc; + this.id = id; + this.attrs = attrs || {}; + this.parent = parent || null; + this.dataset = {}; + this.classList = (this.attrs["class"] || "").split(/\\s+/).filter(Boolean); + for (const [name, value] of Object.entries(this.attrs)) { + if (!name.startsWith("data-")) continue; + this.dataset[name.slice(5).replace(/-([a-z])/g, (_, c) => c.toUpperCase())] = value; + } + this.hidden = false; + this.textContent = ""; + this._html = ""; + } + get innerHTML() { return this._html; } + set innerHTML(value) { + this.doc.replaceChildren(this, value); + this._html = value; + } + focus() { this.doc.activeElement = this; } + matches(selector) { + if (selector.startsWith(".")) return this.classList.includes(selector.slice(1)); + const attribute = /^\\[([A-Za-z-]+)(?:=([^\\]]*))?\\]$/.exec(selector); + if (!attribute) return false; + const value = this.attrs[attribute[1]]; + if (value === undefined) return false; + return attribute[2] === undefined || value === attribute[2]; + } + closest(selector) { + for (let node = this; node; node = node.parent) if (node.matches(selector)) return node; + return null; + } + querySelectorAll(selector) { + return [...(this.doc.owned.get(this.id) || new Map()).values()] + .filter(node => node.matches(selector)); + } +} +const document = { + activeElement: null, + body: null, + roots: new Map(), + owned: new Map(), + index: new Map(), + getElementById(id) { + if (this.index.has(id)) return this.index.get(id); + return this.roots.get(id) || null; + }, + replaceChildren(root, html) { + for (const [id, node] of this.owned.get(root.id) || new Map()) { + if (this.index.get(id) === node) this.index.delete(id); + if (this.activeElement === node) this.activeElement = this.body; + } + const created = new Map(); + for (const tag of html.match(/<[a-zA-Z][^>]*>/g) || []) { + const attrs = {}; + for (const [, name, value] of tag.matchAll(/([A-Za-z-]+)="([^"]*)"/g)) attrs[name] = value; + if (attrs.id === undefined) continue; + const node = new FakeElement(this, attrs.id, attrs, root); + created.set(attrs.id, node); + this.index.set(attrs.id, node); + } + this.owned.set(root.id, created); + } +}; +document.body = new FakeElement(document, "", {}); +document.activeElement = document.body; +for (const id of __SHELL_IDS__) document.roots.set(id, new FakeElement(document, id, {})); +Date.now = () => __NOW_MS__; +__SCRIPT__ +const element = (id) => { + const node = document.getElementById(id); + if (!node) throw new Error("no element: " + id); + return node; +}; +const frames = []; +for (const step of JSON.parse(process.argv[1])) { + if (step.focus) element(step.focus).focus(); + if (step.click) element(step.on || "worklist").onclick({target: element(step.click)}); + if (step.key) { + element(step.on || "worklist").onkeydown({ + key: step.key, + target: element(step.from), + preventDefault: () => {}, + }); + } + if (step.select) selectWork(step.select); + if (step.payload) render(step.payload); + frames.push({ + active: document.activeElement === document.body ? "" : document.activeElement.id, + worklist: document.getElementById("worklist").innerHTML, + tabs: document.getElementById("tabs").innerHTML, + hidden: Object.fromEntries(["now", "timeline", "releases", "health"] + .map(view => [view, element("panel-" + view).hidden])), + }); +} +console.log(JSON.stringify(frames)); +""" + + +def _board_shell_ids() -> list[str]: + """Every id the shipped page's static markup declares, script excluded.""" + + html = board.render_board_html(board.BoardConfig(repo="codemower-ai/code-mower")) + shell = html[: html.index(" diff --git a/tests/test_board.py b/tests/test_board.py index 63b61b32..c0acc36f 100644 --- a/tests/test_board.py +++ b/tests/test_board.py @@ -1,6 +1,6 @@ from __future__ import annotations -from collections.abc import Callable +from collections.abc import Callable, Sequence from contextlib import contextmanager, redirect_stderr, redirect_stdout import copy import errno @@ -425,10 +425,12 @@ def test_render_board_html_polls_with_one_self_scheduling_timer(self) -> None: self.assertIn("pollTimer = setTimeout(load, delayMs);", html) self.assertIn("clearTimeout(pollTimer);", html) self.assertLess(html.index("clearTimeout(pollTimer);"), html.index("pollTimer = setTimeout(load, delayMs);")) - # Definition plus exactly one call site, on the single path every - # load() takes whether it succeeded or threw. + # Definition plus exactly one call site, and that call site is the + # `finally` that closes load(): not a statement after the try, which a + # throw from either half of a poll skips, but the boundary every path + # out of load() crosses whether it returned or threw. self.assertEqual(html.count("scheduleNextLoad("), 2) - self.assertIn(" scheduleNextLoad(delayMs);\n }", html) + self.assertIn(" } finally {\n scheduleNextLoad(delayMs);\n }\n }", html) @skipUnless(shutil.which("node"), "node is required to execute the board polling script") def test_board_poll_delay_for_each_cache_state(self) -> None: @@ -3745,10 +3747,18 @@ def _render_board_sequence( clearedCount += 1; }; let STEP = null; +// The three outcomes one request can have, which are the three this page has +// to tell apart: a request that never arrived (the fetch itself rejects), a +// response whose body is not JSON (the fetch resolves and `json()` rejects), +// and a response that parsed -- into whatever shape it parsed into, which the +// step supplies verbatim and is under no obligation to make renderable. +const UNPARSABLE = "__UNPARSABLE_MARKER__"; const fetch = async (url) => { const status = url === "/api/status"; const outcome = status ? STEP.status : STEP.events; - if (outcome === null) throw new Error(status ? STEP.status_error : STEP.events_error); + const message = status ? STEP.status_error : STEP.events_error; + if (outcome === null) throw new Error(message); + if (outcome === UNPARSABLE) return {json: async () => { throw new Error(message); }}; return {json: async () => outcome}; }; __SCRIPT__ @@ -3761,12 +3771,24 @@ def _render_board_sequence( const armedBefore = timers.length; const clearedBefore = clearedCount; announced.length = 0; - await load(); + // In the page nothing awaits load(): it is called once and then rearmed + // from a timer, so anything that escapes it is an unhandled rejection with + // no one to see it. Recorded here rather than allowed to abort the run, so + // a test can assert both halves of the invariant separately -- that the + // shipped code lets nothing escape, and that the timer is armed even when + // something does. + let escaped = null; + try { + await load(); + } catch (error) { + escaped = String((error && error.message) || error); + } const armed = timers[timers.length - 1]; frames.push({ nodes: Object.fromEntries(Object.entries(NODES).map(([id, node]) => [id, node.innerHTML || node.textContent])), announced: [...announced], timeline: changeLog.map(entry => entry.sentence), + escaped, delay: armed === undefined ? null : armed.ms, armed: timers.length - armedBefore, cleared: clearedCount - clearedBefore, @@ -3782,11 +3804,17 @@ def _render_board_sequence( """ +# What a step supplies for a request whose response arrives and whose body is +# not JSON. `None` is already "the request never arrived"; this is the other +# transport-level failure, and the two reach `fetchJson` differently. +UNPARSABLE_BODY = "__UNPARSABLE_BODY__" + + def _run_board_lifetime( steps: list[dict[str, object]], *, now: datetime = OBSERVATION_NOW, - mutate: tuple[str, str] | None = None, + mutate: tuple[str, str] | Sequence[tuple[str, str]] | None = None, ) -> list[dict[str, object]]: """Replay a sequence of polls through the shipped ``load()`` loop. @@ -3797,18 +3825,27 @@ def _run_board_lifetime( identity first, the way an operator would before a refresh lands. ``mutate`` replaces one exact fragment of the shipped page before it runs, - so a test can execute the code this replaced and prove the assertions it - makes would actually catch its return. + or several, so a test can execute the code this replaced and prove the + assertions it makes would actually catch its return. Each fragment must + appear exactly once, and each is applied to the page the one before it + produced, so a set of them can reconstruct a whole earlier shape. """ page = _board_script() - if mutate is not None: - original, replacement = mutate - if page.count(original) != 1: # pragma: no cover - guards the mutation - raise AssertionError(f"board page script no longer contains exactly one {original!r}") - page = page.replace(original, replacement) - script = BOARD_LIFETIME_HARNESS.replace("__NOW_MS__", str(int(now.timestamp() * 1000))).replace( - "__SCRIPT__", page + if mutate: + edits = [mutate] if isinstance(mutate[0], str) else list(mutate) + for original, replacement in edits: + if page.count(original) != 1: # pragma: no cover - guards the mutation + raise AssertionError( + f"board page script no longer contains exactly one {original!r}" + ) + page = page.replace(original, replacement) + # The marker first: the page script is substituted last so nothing in it + # can be expanded again. + script = ( + BOARD_LIFETIME_HARNESS.replace("__NOW_MS__", str(int(now.timestamp() * 1000))) + .replace("__UNPARSABLE_MARKER__", UNPARSABLE_BODY) + .replace("__SCRIPT__", page) ) normalized = [ { @@ -9820,6 +9857,381 @@ def test_rerendering_without_the_local_override_leaves_the_claim_asserted(self) self.assertEqual(ungated["announced"], []) +@skipUnless(shutil.which("node"), "node is required to execute the board polling script") +class BoardPollingContinuityTests(TestCase): + """The poll loop outlives everything one poll can do to this page. + + `/api/events` answering with valid JSON is not the same as answering with + a shape this page can render: `events` arriving as a string, as an object + that merely has a `length`, or as a list holding a null all parse cleanly + and then throw inside `renderEvents`. That call sat outside the status + handler and inside no boundary of its own, so the throw escaped `load()`, + skipped `scheduleNextLoad()` and ended this page's status refreshes for + good -- leaving the last snapshot on screen indefinitely, still asserting + the present tense, with no transport warning anywhere, because the status + poll it came from had succeeded and nothing had failed to record. + + The rules these hold, in the order they matter: + + * exactly one timer is armed per `load()`, however that load went, and + arming it is the last thing every path does; + * the two halves of a poll fail independently -- an unrenderable events + payload withdraws nothing, because that view was never evidence of now, + and a status poll that did not complete does not suppress the history; + * a status response this page rendered still chooses the next delay, and + everything else still falls back to the configured interval; + * and nothing is swallowed on the way: a status render that throws is + recorded on the Health transport row with its own message, and a throw + that gets past every handler still reaches the caller. + """ + + COMPLETE = BoardPollingAuthorityTests.COMPLETE + UNCONFIRMED = BoardPollingAuthorityTests.UNCONFIRMED + PRESENT_TENSE = BoardPollingAuthorityTests.PRESENT_TENSE + REFRESH_MS = 15_000 + FAST_POLL_MS = 750 + + GOOD_STATUS = _cache_payload([_observation_fixture("no_work")], CONFIRMED_CACHE) + # A status payload the server would serve while a refresh is still running, + # which is the one case the page paces itself faster than the configured + # interval. Used to prove the delay a *successful* status response chose is + # what the poll waits, whatever the events half did. + REFRESHING_STATUS = _cache_payload([_observation_fixture("no_work")], STALE_CACHE_REFRESHING) + # Valid JSON from /api/status whose shape `render()` cannot walk: the page + # reads `remote.pull_requests` as a list and calls `.map` on it, so this + # request succeeds, parses, and then throws inside the render. + UNRENDERABLE_STATUS = copy.deepcopy(GOOD_STATUS) + UNRENDERABLE_STATUS["remote"]["pull_requests"] = "one" + RENDER_ERROR = "prs.map is not a function" + + HISTORY = {"events": [{"created_at": "2026-09-12T19:59:00Z", "summary": {"next_action": "review"}}]} + LATER_HISTORY = {"events": [{"created_at": "2026-09-12T19:59:30Z", "summary": {"next_action": "merge"}}]} + HISTORY_MARK = "next: review" + LATER_MARK = "next: merge" + + # Valid JSON from /api/events that `renderEvents` cannot walk. Each one is + # a real shape a server or a proxy can produce, and each throws at a + # different point: on the copy, on the reverse, and inside the row builder. + UNRENDERABLE_EVENTS = ( + ("events is a string", {"events": "two"}), + ("events is an object with a length", {"events": {"length": 1}}), + ("events holds a null entry", {"events": [None]}), + ) + # Every way `/api/events` can fail to leave a renderable history: the two + # transport-level failures `fetchJson` reports as `ok: false`, and the + # three shapes it reports as success and cannot render. + EVENT_FAILURES = ( + ("the request never arrived", None), + ("the body is not JSON", UNPARSABLE_BODY), + ) + UNRENDERABLE_EVENTS + # And the matching three for `/api/status`, which is the half that does + # carry authority over what this page claims about now. + STATUS_FAILURES = ( + ("the request never arrived", None), + ("the body is not JSON", UNPARSABLE_BODY), + ("the payload does not render", UNRENDERABLE_STATUS), + ) + + # The shipped fragment each mutation below replaces, quoted once so a + # rename cannot leave a mutation silently matching nothing: the helper + # requires exactly one occurrence. + POLL_TAIL = ( + " delayMs = renderStatusOutcome(status);\n" + " if (events.ok) renderEventsIsolated(events.data);\n" + " } finally {\n" + " scheduleNextLoad(delayMs);\n" + " }\n" + ) + # The code this replaced: the events render in no boundary of its own, and + # the next poll armed after the try rather than on the way out of it. + PRE_FIX_TAIL = ( + POLL_TAIL, + " delayMs = renderStatusOutcome(status);\n" + " if (events.ok) renderEvents(events.data);\n" + " scheduleNextLoad(delayMs);\n" + " } finally {\n" + " }\n", + ) + # One half of it: the events render loses its boundary and the arming + # keeps its own. + UNISOLATED_EVENTS = ( + POLL_TAIL, + " delayMs = renderStatusOutcome(status);\n" + " if (events.ok) renderEvents(events.data);\n" + " } finally {\n" + " scheduleNextLoad(delayMs);\n" + " }\n", + ) + # The other half: the events render keeps its boundary and the arming + # moves back outside the one that guarantees it. + SCHEDULING_OUTSIDE_FINALLY = ( + " } finally {\n scheduleNextLoad(delayMs);\n }\n", + " scheduleNextLoad(delayMs);\n } finally {\n }\n", + ) + # A retained rerender that fails outright -- the one throw the status + # handler itself cannot catch, because it happens inside the handler. + THROWING_RERENDER = ( + " render(lastStatusData, transportState);\n", + ' throw new Error("rerender failed");\n', + ) + + def _assert_one_timer(self, frames: list[dict], *, escapes: bool = False) -> None: + """One arming per load, clearing the one it replaces, never stacked.""" + + for index, frame in enumerate(frames): + self.assertEqual(frame["armed"], 1, f"poll {index} armed {frame['armed']} timers") + self.assertEqual(frame["cleared"], 1 if index else 0) + self.assertEqual(frame["timers"], index + 1) + self.assertTrue(frame["pending"]) + if not escapes: + self.assertIsNone(frame["escaped"], f"poll {index} let {frame['escaped']} escape") + + def test_an_unrenderable_events_payload_leaves_the_status_poll_untouched(self) -> None: + """The finding exactly, in every shape that produces it.""" + + for name, events in self.EVENT_FAILURES: + with self.subTest(name): + rendered, failed = _run_board_lifetime( + [ + {"status": self.GOOD_STATUS, "events": self.HISTORY}, + {"status": self.GOOD_STATUS, "events": events}, + ] + ) + # The poll is still scheduled, which is the whole finding: the + # page goes on refreshing after a history it could not render. + self._assert_one_timer([rendered, failed]) + # The status half is untouched in every respect an operator can + # see: the authority it confirmed, the claim it licensed, the + # Health row that says polls are arriving, and the pacing the + # status cache chose. + self.assertEqual(failed["transport"], {"confirmed": True, "failures": 0, "error": ""}) + self.assertIn(self.COMPLETE, failed["nodes"]["worklist"]) + self.assertIn("nothing to do in this session", failed["nodes"]["worklist"]) + self.assertIn("status polls answered", failed["nodes"]["diagnostics"]) + self.assertNotIn("status poll failed", failed["nodes"]["diagnostics"]) + self.assertEqual(failed["delay"], rendered["delay"]) + # An events payload is not news about this session, so nothing + # is announced and nothing is logged for it either. + self.assertEqual(failed["announced"], []) + # And the history card keeps the last events it could render, + # rather than being emptied or replaced by an error. + self.assertIn(self.HISTORY_MARK, failed["nodes"]["history"]) + self.assertEqual(failed["nodes"]["history"], rendered["nodes"]["history"]) + + def test_each_half_of_a_poll_fails_without_the_other(self) -> None: + # The whole matrix: every status outcome against every events outcome, + # from one page that has already rendered both surfaces. The rule is + # the same in all sixteen cells -- authority follows the status half + # alone, the history follows the events half alone, and the poll is + # armed once whatever either of them did. + statuses = (("the payload renders", self.GOOD_STATUS),) + self.STATUS_FAILURES + events = (("the history renders", self.LATER_HISTORY),) + self.EVENT_FAILURES + for (status_name, status), (events_name, history) in itertools.product(statuses, events): + healthy = status is self.GOOD_STATUS + renderable = history is self.LATER_HISTORY + with self.subTest(status=status_name, events=events_name): + first, second = _run_board_lifetime( + [ + {"status": self.GOOD_STATUS, "events": self.HISTORY}, + {"status": status, "events": history}, + ] + ) + self._assert_one_timer([first, second]) + # Authority: confirmed if and only if the status half left a + # snapshot this page rendered. + self.assertEqual(second["transport"]["confirmed"], healthy) + self.assertEqual(second["transport"]["failures"], 0 if healthy else 1) + self.assertIn( + self.COMPLETE if healthy else self.UNCONFIRMED, second["nodes"]["worklist"] + ) + if not healthy: + for sentence in self.PRESENT_TENSE: + self.assertNotIn(sentence, second["nodes"]["worklist"]) + # History: the new events if they rendered, the last ones it + # could render otherwise -- and never the other half's verdict. + self.assertIn( + self.LATER_MARK if renderable else self.HISTORY_MARK, + second["nodes"]["history"], + ) + # Pacing: the delay a rendered status response chose, and the + # configured interval for every other outcome. + self.assertEqual(second["delay"], first["delay"] if healthy else self.REFRESH_MS) + + def test_a_status_payload_that_cannot_render_is_reported_and_not_swallowed(self) -> None: + # A shape `render()` cannot walk is a status poll that did not complete: + # the claim is withdrawn, the retained payload is rerendered under the + # withdrawn authority, and the reason is stated on the Health transport + # row with the renderer's own message rather than discarded. + rendered, failed, recovered = _run_board_lifetime( + [ + {"status": self.GOOD_STATUS, "events": self.HISTORY}, + {"status": self.UNRENDERABLE_STATUS, "events": self.HISTORY}, + {"status": self.GOOD_STATUS, "events": self.HISTORY}, + ] + ) + self._assert_one_timer([rendered, failed, recovered]) + self.assertIn(self.UNCONFIRMED, failed["nodes"]["worklist"]) + self.assertIn("status poll failed", failed["nodes"]["diagnostics"]) + self.assertIn(f"({self.RENDER_ERROR})", failed["nodes"]["diagnostics"]) + self.assertIn('1 failed status poll', failed["nodes"]["diagnostics"]) + # The payload that could not render never became the payload this page + # retains, so the rerender is of the snapshot that did render. + self.assertEqual(failed["nodes"]["sources"], rendered["nodes"]["sources"]) + self.assertEqual(failed["nodes"]["generated"], rendered["nodes"]["generated"]) + # Pacing falls back to the configured interval and returns to the + # cache's own delay on the poll that renders again. + self.assertEqual(failed["delay"], self.REFRESH_MS) + self.assertNotEqual(rendered["delay"], self.REFRESH_MS) + self.assertEqual(recovered["delay"], rendered["delay"]) + self.assertEqual(recovered["transport"], {"confirmed": True, "failures": 0, "error": ""}) + self.assertIn(self.COMPLETE, recovered["nodes"]["worklist"]) + + def test_the_delay_a_status_response_chose_survives_an_events_failure(self) -> None: + # Pacing belongs to the status half alone. A snapshot the server is + # still refreshing is polled at the fast interval, and an events + # payload that cannot be rendered beside it does not slow that to the + # configured fallback. + for name, events in self.EVENT_FAILURES: + with self.subTest(name): + frames = _run_board_lifetime( + [ + {"status": self.REFRESHING_STATUS, "events": self.HISTORY}, + {"status": self.REFRESHING_STATUS, "events": events}, + ] + ) + self._assert_one_timer(frames) + self.assertEqual([frame["delay"] for frame in frames], [self.FAST_POLL_MS] * 2) + + def test_a_retained_rerender_that_throws_still_arms_the_next_poll(self) -> None: + # The one throw the status handler cannot catch, because it happens + # inside the handler: the rerender that withdraws the claim fails + # outright. The failure is recorded before the rerender is attempted, + # so the transport row still has the count and the reason; the throw + # still reaches the caller, so nothing is hidden; and the next poll is + # armed anyway, so the page recovers on its own. + rendered, failed, recovered = _run_board_lifetime( + [ + {"status": self.GOOD_STATUS, "events": self.HISTORY}, + {"status": None, "events": self.HISTORY}, + {"status": self.GOOD_STATUS, "events": self.HISTORY}, + ], + mutate=self.THROWING_RERENDER, + ) + self._assert_one_timer([rendered, failed, recovered], escapes=True) + self.assertEqual(failed["escaped"], "rerender failed") + self.assertIsNone(rendered["escaped"]) + self.assertIsNone(recovered["escaped"]) + self.assertEqual(failed["transport"]["confirmed"], False) + self.assertEqual(failed["transport"]["failures"], 1) + self.assertEqual(failed["delay"], self.REFRESH_MS) + # And the poll that follows renders the whole board again, with the + # server's own authority and no trace of the local override. + self.assertEqual(recovered["transport"], {"confirmed": True, "failures": 0, "error": ""}) + self.assertIn(self.COMPLETE, recovered["nodes"]["worklist"]) + self.assertIn(self.HISTORY_MARK, recovered["nodes"]["history"]) + + def test_a_healthy_poll_recovers_both_surfaces_after_both_halves_failed(self) -> None: + for name, events in self.EVENT_FAILURES: + with self.subTest(name): + rendered, failed, recovered = _run_board_lifetime( + [ + {"status": self.GOOD_STATUS, "events": self.HISTORY}, + {"status": None, "events": events}, + {"status": self.GOOD_STATUS, "events": self.LATER_HISTORY}, + ] + ) + self._assert_one_timer([rendered, failed, recovered]) + # Under the failure: the claim is withdrawn and the history is + # the last one that rendered, neither of them caused by the + # other. + self.assertIn(self.UNCONFIRMED, failed["nodes"]["worklist"]) + self.assertIn(self.HISTORY_MARK, failed["nodes"]["history"]) + # After it: the status surfaces are byte-for-byte what the + # first poll rendered, and the history is the new one. + past = {"changes", "announce", "history"} + self.assertEqual( + {id: html for id, html in recovered["nodes"].items() if id not in past}, + {id: html for id, html in rendered["nodes"].items() if id not in past}, + ) + self.assertIn(self.LATER_MARK, recovered["nodes"]["history"]) + self.assertNotIn(self.HISTORY_MARK, recovered["nodes"]["history"]) + + def test_one_timer_per_load_across_a_long_mixed_lifetime(self) -> None: + # Sixteen consecutive polls, every status outcome crossed with every + # events outcome in one page lifetime, so a path that arms twice or + # arms none is caught in sequence rather than only in isolation. The + # timer count is the assertion: it is the poll index, always, and the + # pending timer is always the one this poll armed. + steps = [{"status": self.GOOD_STATUS, "events": self.HISTORY}] + for (_, status), (_, events) in itertools.product( + (("renders", self.GOOD_STATUS),) + self.STATUS_FAILURES, + (("renders", self.LATER_HISTORY),) + self.EVENT_FAILURES, + ): + steps.append({"status": status, "events": events}) + frames = _run_board_lifetime(steps) + self.assertEqual(len(frames), len(steps)) + self._assert_one_timer(frames) + self.assertEqual(frames[-1]["timers"], len(steps)) + + def test_the_pre_fix_shape_stops_refreshing_on_an_unrenderable_history(self) -> None: + """The code this replaced, executed: the page stops polling, silently.""" + + for name, events in self.UNRENDERABLE_EVENTS: + with self.subTest(name): + rendered, frozen = _run_board_lifetime( + [ + {"status": self.GOOD_STATUS, "events": self.HISTORY}, + {"status": self.GOOD_STATUS, "events": events}, + ], + mutate=self.PRE_FIX_TAIL, + ) + self.assertEqual(rendered["armed"], 1) + # No timer at all: the throw escaped `load()` before it reached + # `scheduleNextLoad`, and in the page nothing would call + # `load()` again. The harness calls it directly, which is the + # only reason this lifetime has a next poll. + self.assertEqual(frozen["armed"], 0) + self.assertEqual(frozen["timers"], 1) + self.assertIsNotNone(frozen["escaped"]) + # And what an operator is left looking at is the worst part of + # it: a green present-tense claim from the last status poll, + # with the transport row still reporting that polls arrive. + self.assertIn(self.COMPLETE, frozen["nodes"]["worklist"]) + self.assertIn("nothing to do in this session", frozen["nodes"]["worklist"]) + self.assertIn("status polls answered", frozen["nodes"]["diagnostics"]) + self.assertNotIn("status poll failed", frozen["nodes"]["diagnostics"]) + + def test_event_rendering_outside_its_boundary_escapes_the_poll_it_no_longer_stops(self) -> None: + # One half of the fix at a time, so neither is load-bearing by + # accident. Without its own boundary the events throw reaches `load()` + # -- which in the page is an unhandled rejection -- and the `finally` + # is the only reason the next poll is still armed. + unisolated = _run_board_lifetime( + [ + {"status": self.GOOD_STATUS, "events": self.HISTORY}, + {"status": self.GOOD_STATUS, "events": {"events": "two"}}, + ], + mutate=self.UNISOLATED_EVENTS, + )[-1] + self.assertIsNotNone(unisolated["escaped"]) + self.assertEqual(unisolated["armed"], 1) + self.assertEqual(unisolated["timers"], 2) + + # And with the boundary kept but the arming moved back outside the + # `finally`, the throw the status handler cannot catch ends the loop + # instead -- so the `finally` is doing work the boundary does not. + outside = _run_board_lifetime( + [ + {"status": self.GOOD_STATUS, "events": self.HISTORY}, + {"status": None, "events": self.HISTORY}, + ], + mutate=(self.SCHEDULING_OUTSIDE_FINALLY, self.THROWING_RERENDER), + )[-1] + self.assertEqual(outside["escaped"], "rerender failed") + self.assertEqual(outside["armed"], 0) + self.assertEqual(outside["timers"], 1) + + class _FailingReadHandle: """A handle that opens cleanly and raises when its contents are read.