diff --git a/AGENTS.md b/AGENTS.md index 2f9dd13..7f65a6e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -44,11 +44,13 @@ Drivers know apps, adapters know vendors, `local/` knows this project. Module map for `manager/core/` (dependencies flow strictly left to right): ``` -config → state → taskfiles → events / github / drive / sync → agents → watch / httpd → board.py +config → state / reports → taskfiles → events / github / drive / sync → agents → watch / httpd → board.py ``` - `config.py` — paths, stages, settings, prompt/adapter/driver resolution - `state.py` — shared registries, event log persistence, SSE fan-out +- `reports.py` — what the record keeps of an agent's closing report: one + cap, one clip, for the task file and the PR body alike - `taskfiles.py` — reading and moving task files; the only code touching tasks/ - `events.py` — ingests NORMALIZED events (the adapter contract), session registry - `github.py` — PR opening, Copilot requests, review/CI polling @@ -331,6 +333,20 @@ worktree must not already exist when it starts. review/ is how a broken launch hides. Stdout is kept in `local/state/agent/logs/`. +### What the record keeps of a report + +The agent's closing report is the permanent record: it is appended to the +task file, shown as the session's last entry, and carried into the PR +body — the same text in all three, from one helper (`reports.py`) with +one cap. A report that fits arrives whole. One that doesn't keeps **both +ends** — the headline the report contract puts first, and the pointer +that closes it — and loses the middle, cut on line boundaries, with one +line of prose in its place saying how much went and naming the log under +`local/state/agent/logs/` that still holds all of it. The reader is never +left to infer that something was removed. A *failed* run is the deliberate +exception: its excerpt keeps the log's tail, because for a crash the end +is the story. + ### A run that died An agent that exits non-zero is the one outcome a person must not miss, so diff --git a/manager/core/agents.py b/manager/core/agents.py index 518c760..2d432b1 100644 --- a/manager/core/agents.py +++ b/manager/core/agents.py @@ -20,16 +20,22 @@ from pathlib import Path import config import events +import reports import state from taskfiles import actor_name, find_stage_of, move_task, read_task, set_assignee -def _clean_log(text: str, cap: int = 3000) -> str: - """An agent's -p output is its final report; strip the hook-failure - noise other tools may have interleaved.""" - lines = [l for l in text.strip().splitlines() - if not ("hook" in l and "failed" in l)] - return "\n".join(lines).strip()[-cap:] +def _report_of(record: dict, text: str | None = None) -> str: + """What the record keeps of a run's report: cleaned, and clipped by + reports.report — head first, because the report's own first line is + where it says what happened. `text` names the part after a marker when + there is one; otherwise the whole log.""" + if text is None: + try: + text = Path(record["log"]).read_text(encoding="utf-8", errors="replace") + except OSError: + return "" + return reports.report(text, log_path=record.get("log")) def _file_report(record: dict, heading: str, report: str) -> None: @@ -383,7 +389,7 @@ def _failure_excerpt(log_path: str | None, lines: int = 6, cap: int = 600) -> st text = Path(log_path).read_text(encoding="utf-8", errors="replace") except OSError: text = "" - kept = [line.rstrip() for line in _clean_log(text, cap=8000).splitlines() + kept = [line.rstrip() for line in reports.tail(text, 8000).splitlines() if line.strip()] if not kept: return "no output — the run died before the agent said anything" @@ -479,11 +485,7 @@ def _reap_agent(agent_id: str, proc: subprocess.Popen, log_file) -> None: summary = (f"{name} declined {filename} — not ready: {declined}" + ("" if cleaned else f" (worktree {record['worktree']} kept: it has commits)")) elif rc == 0 and not stopped: - try: - report = _clean_log(Path(record["log"]).read_text(encoding="utf-8", - errors="replace")) - except OSError: - report = "" + report = _report_of(record) _file_report(record, "Work report", report) _session_report(record, report) if _no_new_commits(record): @@ -614,7 +616,7 @@ def _reap_pr_fix(agent_id: str, proc: subprocess.Popen, log_file) -> None: except OSError: text = "" idx = text.find("ADDRESSED:") - report = text[idx:].strip() if idx >= 0 else text.strip()[-1500:] + report = _report_of(record, text[idx:] if idx >= 0 else text) _file_report(record, "PR update", report) _session_report(record, report) summary = f"{name} acted on {filename}'s PR — re-review when ready" @@ -640,7 +642,7 @@ def _reap_pr_review(agent_id: str, proc: subprocess.Popen, log_file) -> None: except OSError: text = "" idx = text.find("PR REVIEW:") - report = text[idx:].strip() if idx >= 0 else text.strip()[-1500:] + report = _report_of(record, text[idx:] if idx >= 0 else text) match = re.search(r"^PR REVIEW:\s*(APPROVE|REQUEST CHANGES)", report) verdict = match.group(1) if match else None _file_report(record, "PR review", report) @@ -672,7 +674,7 @@ def _reap_review(agent_id: str, proc: subprocess.Popen, log_file) -> None: except OSError: text = "" idx = text.find("RELEVANCE REVIEW") - report = text[idx:].strip() if idx >= 0 else text.strip()[-1500:] + report = _report_of(record, text[idx:] if idx >= 0 else text) verdict = report.splitlines()[0] if report else None _file_report(record, "Relevance review", report) _session_report(record, report) diff --git a/manager/core/github.py b/manager/core/github.py index 47ab0d8..4700220 100644 --- a/manager/core/github.py +++ b/manager/core/github.py @@ -25,6 +25,7 @@ from pathlib import Path import config import drive as drive_mod +import reports import state from taskfiles import STATUS_RE, commit_edit, find_stage_of, move_task, read_task @@ -147,9 +148,9 @@ def _open_pr(filename: str) -> str: body = (f"Task: `{filename}` — tracked in `.task-manager/tasks/review/`.\n\n" f"Opened by the board when the card moved to review.") - log_tail = _agent_log_tail(filename) - if log_tail: - body += f"\n\n## Agent summary\n\n{log_tail}" + summary = _agent_report(filename) + if summary: + body += f"\n\n## Agent summary\n\n{summary}" result = _run([config.GH_BIN, "pr", "create", "--head", branch, "--base", "main", "--title", task["title"], "--body", body], timeout=120) if result.returncode != 0: @@ -190,7 +191,10 @@ def _existing_pr(branch: str) -> str: return next((l.strip() for l in found.stdout.splitlines() if "/pull/" in l), "") -def _agent_log_tail(filename: str, cap: int = 1500) -> str: +def _agent_report(filename: str) -> str: + """The work agent's closing report, clipped exactly as the task file + clips it (reports.report — head first, one cap for both). The PR and + the record must never tell different stories about the same run.""" with state.LOCK: records = [r for r in state.AGENTS.values() if r["task"] == filename and r.get("mode") == "work"] @@ -198,10 +202,10 @@ def _agent_log_tail(filename: str, cap: int = 1500) -> str: return "" latest = max(records, key=lambda r: r["started"]) try: - text = Path(latest["log"]).read_text(encoding="utf-8", errors="replace").strip() + text = Path(latest["log"]).read_text(encoding="utf-8", errors="replace") except OSError: return "" - return text[-cap:] + return reports.report(text, log_path=latest.get("log")) def request_copilot(filename: str) -> str: diff --git a/manager/core/reports.py b/manager/core/reports.py new file mode 100644 index 0000000..58020ef --- /dev/null +++ b/manager/core/reports.py @@ -0,0 +1,160 @@ +"""Keeping an agent's closing report — one clip, every surface. + +A report is the permanent record: it is appended to the task file, shown +as the session's last entry, and carried into the PR body. When one is +too long to keep whole, *which end survives* is the whole question. The +report contract asks the agent to lead with the state of the work, so the +first lines are the part a reader must not lose — a tail slice drops +exactly the sentence that says what happened. + +So the clip here keeps both ends and cuts the middle, on line boundaries, +and says so in words on its own line, naming the log that still holds the +whole thing. The failure excerpt is the one deliberate exception and does +not come through here: for a run that died, the *end* is the story. + +Sits left of both consumers in the module map (config → state → +taskfiles → events / github / … → agents), so agents.py and github.py +clip identically instead of reaching sideways for each other's helper. +Depends on config alone, for the repo root a log path is named against. +""" + +from __future__ import annotations + +from pathlib import Path + +import config + +# What the permanent record keeps of one report, in characters. Roughly +# 1,800 words — enough for the report the prompt contract asks for +# (state of the work, what to do, what to know, review-first pointer) +# with room to spare, so in practice reports arrive whole and this is +# the backstop rather than the norm. One number, both consumers: the +# task file and the PR body must never tell different stories about the +# same run. +CAP = 12000 + +# Of the budget, how much goes to the head. The headline is the contract; +# the tail is the "review first" pointer that closes it. Two to one. +HEAD_SHARE = 2 / 3 + + +def clean(text: str) -> str: + """An agent's -p output is its final report; strip the hook-failure + noise other tools may have interleaved. No capping — that is `report` + (head + tail) or `tail` (a dead run's ending).""" + lines = [l for l in (text or "").strip().splitlines() + if not ("hook" in l and "failed" in l)] + return "\n".join(lines).strip() + + +def tail(text: str, cap: int) -> str: + """The last `cap` characters, cleaned. For a crash, where the end is + the story — see `_failure_excerpt` in agents.py.""" + return clean(text)[-cap:] + + +def report(text: str, log_path: str | None = None, cap: int = CAP) -> str: + """The report as the record should keep it. + + Shorter than the cap: returned cleaned and otherwise byte for byte. + Longer: the leading lines and the trailing lines survive, separated by + one line of prose saying how much was cut and where the whole report + still lives. Cuts land on line boundaries, so no line of the record is + half a line — the single exception is a report that is one enormous + line, which is cut at a space rather than not shown at all. + """ + text = clean(text) + if len(text) <= cap: + return text + + # Two passes over the elision line: the first counts what it costs at + # its longest (every character dropped), the second states the truth. + # Digits can only shrink, so the result never exceeds the cap. The 4 + # is the blank line either side of it. + budget = cap - len(_elision(len(text), cap, log_path)) - 4 + if budget <= 0: + # A cap too small to hold even the notice: keep the head, say + # nothing else. Nothing in bench configures one this small. + return _split_head(text, cap)[0] + + head, rest = _split_head(text, int(budget * HEAD_SHARE)) + end = _take_tail(rest, budget - len(head)).lstrip() + head = head.rstrip() + dropped = len(text) - len(head) - len(end) + kept = [head, _elision(dropped, cap, log_path)] + if end: + kept.append(end) + return "\n\n".join(kept) + + +def _elision(dropped: int, cap: int, log_path: str | None) -> str: + """The line that stands where the middle was. A reader must never have + to infer that something was removed, nor go looking for the rest.""" + return (f"… {dropped} characters of this report were cut here to keep the " + f"record within {cap} characters. The whole report is in " + f"{_log_reference(log_path)}.") + + +def _log_reference(log_path: str | None) -> str: + """Where the unclipped report still is, repo-relative when it can be.""" + logs = "manager/local/state/agent/logs/" + if not log_path: + return f"this run's log under `{logs}`" + path = Path(log_path) + try: + return f"`{path.resolve().relative_to(config.REPO)}`" + except (ValueError, OSError): + return f"`{logs}{path.name}`" + + +def _split_head(text: str, budget: int) -> tuple[str, str]: + """Leading whole lines fitting the budget, and everything after them. + + A first line longer than the entire budget is the one case a line gets + cut: at its last space, so the record ends on a word rather than on + `four" — they are`. A line with no space in it at all (one long token) + is cut where the budget runs out; there is no better place. + """ + kept, used = [], 0 + for line in text.split("\n"): + cost = len(line) + (1 if kept else 0) + if used + cost > budget: + break + kept.append(line) + used += cost + if kept: + return "\n".join(kept), text[used:].lstrip("\n") + piece = _cut_at_space(text[:budget]) + return piece, text[len(piece):].lstrip("\n") + + +def _take_tail(text: str, budget: int) -> str: + """Trailing whole lines fitting the budget — the mirror of the head, + including how it treats one line too long to keep whole.""" + kept, used = [], 0 + for line in reversed(text.split("\n")): + cost = len(line) + (1 if kept else 0) + if used + cost > budget: + break + kept.insert(0, line) + used += cost + if kept: + return "\n".join(kept) + return _cut_at_space(text[-budget:], from_start=True) + + +def _cut_at_space(piece: str, from_start: bool = False) -> str: + """Trim a fragment back to a word boundary, if one is near enough to + the edge to be worth losing. Half the fragment is the limit: past that + the cut is doing more harm than the broken word it avoids.""" + if not piece: + return "" + if from_start: + space = piece.find(" ") + if 0 <= space < len(piece) // 2: + piece = piece[space + 1:] + else: + space = piece.rfind(" ") + if space > len(piece) // 2: + piece = piece[:space] + return piece.strip() diff --git a/tests/fixtures/32-work-report.log b/tests/fixtures/32-work-report.log new file mode 100644 index 0000000..81c0b45 --- /dev/null +++ b/tests/fixtures/32-work-report.log @@ -0,0 +1,22 @@ +Work is committed on `task/32-serve-bench-12vectors-com-from-a-worker` in two commits and the suite is green, but **nothing has been deployed** — this headless run had no Cloudflare credentials and no network for `npx`, so `wrangler deploy`, `wrangler dev` and every live-response check are still outstanding. + +## What to do + +1. **Deploy it**: `cd site && npx wrangler deploy` from a shell logged in to Cloudflare, then confirm that `https://bench.12vectors.com/` answers with the built landing page now. +2. **Tick or reject the acceptance boxes I could not reach** — the ones I marked "one, two and four" — they are acceptance criteria 1, 2 and 4, and they are the ones no test in this repo can reach. +3. **Replace the account line** in `site/README.md`'s "Where the site lives" table with what `wrangler whoami` prints. I described the account by its defining property (the one holding the `12vectors.com` zone) rather than inventing a name or ID. +4. **Open the follow-up card** for a GitHub Action on merge, if you still want one. + +## What to know + +The card's open question carried the author's own recommendation — manual deploy for v1 — so I built that rather than sending the card back: `site/wrangler.jsonc` and a documented command sequence, no workflow file, no Cloudflare token in repository secrets. + +Serving the site needed three things from the builder first, which is the first commit (`ad639a3`): + +- **A real 404.** Cloudflare's `not_found_handling: "404-page"` wants a literal `404.html` at the root of the assets directory, so a manifest route may now name an `.html` file instead of ending in `/`. `/404.html` is a normal entry with a new `notfound` layout — the site's design and nav, a link back to the landing page, `noindex`, no canonical, and a `null` section that keeps it off the nav it renders. +- **`site/root/`**, copied verbatim to the top of the build the way `static/` is copied into a subdirectory. It holds `_headers`, which the host reads from the root and nowhere else. A file in `root/` that a route would also write stops the build. +- **Fingerprinted asset urls.** The templates link the stylesheet and icon through `$stylesheet`/`$icon`, carrying `?v=` of the file's contents. Nothing in `static/` is renamed, so the tree and the `/static/*` glob are unchanged — but the url moves when the file does, which is what makes the year-long `immutable` cache safe rather than a way to ship an invisible deploy. + +Two judgement calls worth a look. I added a `default-src 'none'` **Content-Security-Policy** to `site/root/_headers` beyond the three headers the card named: it turns "no analytics, no third-party anything" into something the browser enforces, and a test asserts no built page contains an inline `