From 70e760abbc699e0faa8e405566870b975fe9a1cd Mon Sep 17 00:00:00 2001 From: egeozin Date: Fri, 2 Oct 2026 14:28:29 -0700 Subject: [PATCH] Finish runs instead of crashing on recoverable checks, and write down how to fail well An already-covered judgment crashed a paid content.generate run because an extra comparison page couldn't be read; it now needs the site's own coverage read and otherwise ends as insufficient evidence. Revision notes no longer fail a revision over their section shape. A task that pauses without a question asks a plain one. AGENTS.md gains "Failing well": refuse only for security, money and outside effects; everything else ends with a named reason. Co-Authored-By: Claude Opus 5.5 (1M context) --- AGENTS.md | 17 +++++++++++ sandbox/task_app_server.py | 16 ++++++---- src/tin_lite/activities.py | 2 -- src/tin_lite/article_review.py | 12 -------- src/tin_lite/content_editorial_judgment.py | 20 +++++++++++-- tests/test_codex_isolation.py | 11 +++++++ tests/test_content_editorial_judgment.py | 35 ++++++++++++++++++++++ tests/test_workflow_reviews.py | 19 +++++++----- 8 files changed, 103 insertions(+), 29 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 5ef96901..b86d0d43 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -128,6 +128,23 @@ outside marketing, or were written without ever being run. Full rules: - Schedules use Temporal's existing dispatcher, skip overlap, and bounded catch-up. Calendar support is declared by the selected workflow, not inferred from its name. +## Failing well + +- Refuse only to protect security, money or outside effects: credentials, grants, egress, + billing and anything published, sent or merged. Everything else degrades to a recorded + outcome with a named reason. +- A check on model output never fails a run that already paid for it. Map an unsupported + verdict to the nearest honest outcome (for example insufficient evidence), keep what is + usable, or ask the model to fix it within the run. Notes and summaries for the founder are + shaped to fit, never grounds to fail. +- Accept partial provider data: a page with one missing reference, a response with a + warning. Stay strict for writes and for responses with no usable data. +- Limits are runaway guards set well above normal use, not estimates that stop ordinary runs. +- A failure names its cause. Never wrap Tin's own refusal in a generic message; keep the + reason on the run and its receipt, bounded and without provider text or secrets. +- Fixture tests prove the contract; a real run proves the workflow. Before calling a workflow + change done, run it once for real where a provider, sandbox or outside site is involved. + ## Models, sandboxes and connections - New Codex compute is API-only. The provider credential stays on the switchboard; diff --git a/sandbox/task_app_server.py b/sandbox/task_app_server.py index 21391006..3c519be6 100644 --- a/sandbox/task_app_server.py +++ b/sandbox/task_app_server.py @@ -417,11 +417,7 @@ def execute() -> int: "question": None, } else: - result = json.loads(last_agent_message) - if result.get("outcome") not in {"completed", "needs_input"}: - raise RuntimeError("Codex returned an invalid task outcome") - if result["outcome"] == "needs_input" and not result.get("question"): - raise RuntimeError("Codex requested input without a question") + result = _task_result(json.loads(last_agent_message)) with open(os.environ["TIN_TASK_RESULT_PATH"], "w", encoding="utf-8") as handle: json.dump(result, handle, separators=(",", ":")) return 0 @@ -434,6 +430,16 @@ def execute() -> int: proc.kill() +def _task_result(result): + """Codex's final task message as Tin's result; a question-less pause asks a plain one.""" + if not isinstance(result, dict) or result.get("outcome") not in {"completed", "needs_input"}: + raise RuntimeError("Codex returned an invalid task outcome") + if result["outcome"] == "needs_input" and not (result.get("question") or "").strip(): + # Ask rather than lose the turn: the founder can still answer or stop. + result["question"] = "How should I continue?" + return result + + def main() -> int: if not ISOLATED: return execute() diff --git a/src/tin_lite/activities.py b/src/tin_lite/activities.py index 4fe6372e..955ed6b6 100644 --- a/src/tin_lite/activities.py +++ b/src/tin_lite/activities.py @@ -4137,8 +4137,6 @@ async def _procedure_companions(self, run, project, procedure, revision): ) else: content_draft.validate_notes(raw, procedure.content_draft_context) - if procedure.review_revision_context is not None: - article_review.validate_changes(raw) return ( OutputCheckpoint.create( run=run, diff --git a/src/tin_lite/article_review.py b/src/tin_lite/article_review.py index 709a783c..cd7b778e 100644 --- a/src/tin_lite/article_review.py +++ b/src/tin_lite/article_review.py @@ -84,18 +84,6 @@ def validate_notes(content, *, revision=False): text = content.decode("utf-8") if not 1 <= len(content) <= 24_000 or not text.lstrip().startswith("# Generation notes\n"): raise ValueError("Write separate, bounded Generation notes.") - if revision: - validate_changes(content) - - -def validate_changes(content): - text = content.decode("utf-8") - for title in ("What changed", "Feedback not followed"): - sections = re.findall(rf"(?m)^## {title}\n+(.+?)(?=\n## |\Z)", text, re.S) - if len(sections) != 1 or not 4 <= len(sections[0].strip()) <= 2000: - raise ValueError( - f"Explain '{title}' once, in at most 2,000 characters in Generation notes." - ) def change_summary(content): diff --git a/src/tin_lite/content_editorial_judgment.py b/src/tin_lite/content_editorial_judgment.py index 3e6eed0b..d9b76a01 100644 --- a/src/tin_lite/content_editorial_judgment.py +++ b/src/tin_lite/content_editorial_judgment.py @@ -72,11 +72,16 @@ def object_pairs(pairs): if len(set(identities)) != len(identities): raise ValueError("List each compared page once, not duplicate anchors.") inspected = [page for page in judgment.compared_pages if page.status == "inspected"] + host = context.get("host", "").removeprefix("www.").lower() + if judgment.outcome == "already_covered" and not _supports_covered(inspected, host, context): + # A page that couldn't be read is no evidence either way. "Already covered" needs the + # site's own coverage read (the destination, for an update); without it the honest + # result is that coverage couldn't be established, not a failed run. + judgment = judgment.model_copy(update={"outcome": "insufficient_evidence"}) if judgment.outcome in {"draft", "already_covered"} and not inspected: raise ValueError( "Inspect current coverage before declaring a gap or an already-covered topic." ) - host = context.get("host", "").removeprefix("www.").lower() if ( host and judgment.outcome in {"draft", "already_covered"} @@ -85,8 +90,6 @@ def object_pairs(pairs): raise ValueError( "Inspect the planned site's own coverage before deciding to draft or skip." ) - if judgment.outcome == "already_covered" and len(inspected) != len(judgment.compared_pages): - raise ValueError("Unavailable coverage is not evidence that the brief is already covered.") if judgment.outcome == "draft": if min(len(judgment.reader_gain.strip()), len(judgment.change_scope.strip())) < 30: raise ValueError("Explain the concrete reader benefit and bounded change scope.") @@ -101,6 +104,17 @@ def object_pairs(pairs): return {"schema": SCHEMA, **judgment.model_dump()} +def _supports_covered(inspected, host, context): + """Whether the read pages can show the brief is already covered on the planned site.""" + read = [page_identity(page.url) for page in inspected] + if host and not any(identity[0] == host for identity in read): + return False + item = context.get("item") or {} + if item.get("action") == "update_page" and item.get("destination"): + return page_identity(item["destination"]) in read + return bool(read) + + def assessment_document(judgment): return f"# Content assessment\n\n{judgment['rationale']}\n".encode() diff --git a/tests/test_codex_isolation.py b/tests/test_codex_isolation.py index c037dd90..059a5626 100644 --- a/tests/test_codex_isolation.py +++ b/tests/test_codex_isolation.py @@ -240,6 +240,17 @@ def test_a_no_change_outcome_needs_no_pull_request_title(): assert bridge._pull_request_text(result, {}) == ("", "") +def test_a_task_that_pauses_without_a_question_asks_a_plain_one(): + # Two tasks failed on 2026-10-02 with "Codex requested input without a question". + bridge = load_sandbox_module("task_app_server") + paused = {"outcome": "needs_input", "summary": "s", "message": "m", "question": ""} + assert bridge._task_result(paused)["question"] == "How should I continue?" + asked = {**paused, "question": "Which repository?"} + assert bridge._task_result(asked)["question"] == "Which repository?" + with pytest.raises(RuntimeError, match="invalid task outcome"): + bridge._task_result({"outcome": "done"}) + + def test_controller_bypasses_proxy_only_for_local_worker_and_existing_hosts(): bridge = load_sandbox_module("procedure_app_server") assert bridge._loopback_proxy_bypass({"NO_PROXY": "broker.test,127.0.0.1"}) == { diff --git a/tests/test_content_editorial_judgment.py b/tests/test_content_editorial_judgment.py index be583c74..65dccf0a 100644 --- a/tests/test_content_editorial_judgment.py +++ b/tests/test_content_editorial_judgment.py @@ -115,6 +115,41 @@ def test_invalid_judgments_cannot_publish(bad): validate_pair(ARTICLE, raw, context) +def update_context(): + context = {**sample_context(), "output_validator": content_draft.EDITORIAL_VALIDATOR} + context["item"] = { + **context["item"], + "action": "update_page", + "destination": "https://example.com/docs", + } + return context + + +def test_an_unreadable_extra_page_does_not_sink_an_already_covered_judgment(): + # Sheepdogs: the refresh's own page was read in full; the canvas homepage could not be. + context = update_context() + value = judgment(context) + value["compared_pages"].append( + { + "url": "https://example.com/", + "status": "unavailable", + "coverage": "The homepage returned no readable text to compare.", + } + ) + proof = validate_pair(assessment_document(value), judgment_notes(context, value), context) + assert proof["outcome"] == "already_covered" + + +def test_covered_without_reading_the_destination_finishes_as_insufficient_evidence(): + # Claiming coverage from a page that couldn't be read is not evidence; the paid run + # still finishes, with the honest outcome instead of a crash. + context = update_context() + value = judgment(context) + value["compared_pages"][0]["status"] = "unavailable" + proof = validate_pair(assessment_document(value), judgment_notes(context, value), context) + assert proof["outcome"] == "insufficient_evidence" + + async def prepared(f, outcome, *, publish=True): run = await start(f, key=str(uuid4())) activities = TinActivities( diff --git a/tests/test_workflow_reviews.py b/tests/test_workflow_reviews.py index 82dbb6fc..565aeedd 100644 --- a/tests/test_workflow_reviews.py +++ b/tests/test_workflow_reviews.py @@ -13,7 +13,7 @@ from tin_lite import content_draft from tin_lite.activities import TinActivities -from tin_lite.article_review import validate_article, validate_changes +from tin_lite.article_review import change_summary, validate_article, validate_notes from tin_lite.catalog import BUILTIN_WORKFLOWS from tin_lite.content_draft_sources import ContentDraftSources from tin_lite.mcp_server import _review_summary @@ -370,12 +370,17 @@ def test_copy_and_notes_are_different_contracts(): validate_article(article) with pytest.raises(ValueError, match="process notes"): validate_article(article + b"\n## What changed\nWe revised this.\n") - validate_changes( - b"# Generation notes\n\n## What changed\nShorter opening.\n" - b"\n## Feedback not followed\nNone.\n" - ) - with pytest.raises(ValueError, match="Feedback not followed"): - validate_changes(b"# Generation notes\n\n## What changed\nShorter opening.\n") + # Revision notes are for the founder: a missing, repeated or long section never fails the + # paid revision; the review shows what it can. + for notes in ( + b"# Generation notes\n\n## What changed\nShorter opening.\n", + b"# Generation notes\n\nRevised as asked.\n", + b"# Generation notes\n\n## Feedback not followed\n" + b"x" * 3000 + b"\n", + ): + validate_notes(notes, revision=True) + assert change_summary(b"# Generation notes\n\nRevised as asked.\n") is None + with pytest.raises(ValueError, match="bounded Generation notes"): + validate_notes(b"Not notes at all", revision=True) async def test_failed_revision_retry_retains_copy_and_can_stop_chain(publication_db, monkeypatch):