From c83d41a594239c445b05f87de96dcbf2f3b78022 Mon Sep 17 00:00:00 2001 From: Krzysztof kuhy Rudnicki Date: Wed, 11 Nov 2026 14:16:46 +0100 Subject: [PATCH] feedback_inbox: exact MANUAL titles, count fix The server stores a new_note task as ' todos: ' plus a '## <title>' section, so the title no longer started with MANUAL WORK REQUIRED and the entry's own ## heading became a runnable part. After add_task the inbox now rewrites the note with update_task (exact title, section-free body). Marker is written first, format 2 only after the rewrite: a failed rewrite is retried, never re-added; old markers are repaired. install.sh counts folders from the Paths property. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WzxdWXmfFFHX4fNhyFRqFq --- feedback_inbox/README.md | 8 +- feedback_inbox/feedback_inbox/_inbox.py | 91 +++++++++-- feedback_inbox/feedback_inbox/_todo.py | 56 ++++--- .../feedback_inbox/tests/test_inbox.py | 2 +- .../feedback_inbox/tests/test_inbox_repair.py | 145 ++++++++++++++++++ .../feedback_inbox/tests/test_support.py | 21 ++- .../feedback_inbox/tests/test_todo.py | 16 ++ feedback_inbox/install.sh | 5 +- 8 files changed, 302 insertions(+), 42 deletions(-) create mode 100644 feedback_inbox/feedback_inbox/tests/test_inbox_repair.py diff --git a/feedback_inbox/README.md b/feedback_inbox/README.md index 6792153..3996de2 100644 --- a/feedback_inbox/README.md +++ b/feedback_inbox/README.md @@ -17,7 +17,13 @@ as a task in the todo app for `kuhyx/<repo>`. comes from the markdown entry. - Body: a `Due: <next day 10:00>` line, then the bundle's markdown entry (the prompt with absolute paths). -- The task is its own note (`add_task` with `new_note`), so the title is exact. +- The task is its own note (`add_task` with `new_note`). The server stores that as + `<repo> todos: <title>` plus a `## <title>` section, so right after adding, the inbox + rewrites it with `update_task` to the exact title and a section-free body (the entry's + own `##` headings become `###`). todoImplementer splits notes at `## ` headings, and a + title that does not start with `MANUAL WORK REQUIRED: ` would make a runnable part. + The marker records `format: 2` once the rewrite worked; a failed rewrite is retried on + the next sweep (never a second task), and markers without it are repaired the same way. - Closing the task cancels the feedback. "Act on feedback" in a Claude session means working every open `<repo> feedback:` task. diff --git a/feedback_inbox/feedback_inbox/_inbox.py b/feedback_inbox/feedback_inbox/_inbox.py index 24ce224..7121f4c 100644 --- a/feedback_inbox/feedback_inbox/_inbox.py +++ b/feedback_inbox/feedback_inbox/_inbox.py @@ -7,6 +7,7 @@ from dataclasses import dataclass from enum import StrEnum import json import logging +import re from typing import TYPE_CHECKING from feedback_inbox._bundle import ( @@ -33,6 +34,8 @@ if TYPE_CHECKING: LOG = logging.getLogger("feedback_inbox") MARKER = ".inbox-imported.json" +# Marker format 2: the stored note was rewritten to our exact title and body. +FORMAT = 2 SETTLE_SECONDS = 2.0 TITLE_CAP = 120 MANUAL_PREFIX = "MANUAL WORK REQUIRED: " @@ -68,6 +71,15 @@ class TaskDraft: due: str +def _demote(entry: str) -> str: + """Turn ``## `` headings into ``### ``: no section may start a todo part. + + The todo app splits a note into runnable parts at every ``## `` heading, + so the entry's own headings must not look like sections. + """ + return re.sub(r"(?m)^##(?=\s)", "###", entry) + + def _first_line(note: str, fallback: str) -> str: first = next((x.strip() for x in note.splitlines() if x.strip()), "") if len(first) > TITLE_CAP: @@ -89,23 +101,25 @@ def build_task(repo: str, stamp: str, entry: str, report: Report | None) -> Task lines = [f"Due: {due:%Y-%m-%d %H:%M}", ""] if report is None: lines += [CORRUPT_LINE, ""] - return TaskDraft(title, "\n".join([*lines, entry]), due.isoformat()) + return TaskDraft(title, "\n".join([*lines, _demote(entry)]), due.isoformat()) -def _write_marker( - bundle: Path, draft: TaskDraft, task_id: str, *, merged: bool -) -> None: - payload = { - "task_id": task_id, - "title": draft.title, - "due": draft.due, - "merged": merged, - } +def _write_marker(bundle: Path, payload: dict[str, object]) -> None: tmp = bundle / f"{MARKER}.tmp" tmp.write_text(json.dumps(payload, indent=2) + "\n", encoding="utf-8") tmp.replace(bundle / MARKER) +def _read_marker(bundle: Path) -> dict[str, object] | None: + """The bundle's marker, or None when it is not a usable marker.""" + try: + data = json.loads((bundle / MARKER).read_text(encoding="utf-8")) + except OSError, UnicodeDecodeError, json.JSONDecodeError: + return None + ok = isinstance(data, dict) and isinstance(data.get("task_id"), str) + return data if ok and isinstance(data.get("title"), str) else None + + @dataclass(frozen=True) class Importer: """Who files the tasks (``client``) under which GitHub owner.""" @@ -115,9 +129,9 @@ class Importer: settle: float = SETTLE_SECONDS def import_bundle(self, feedback_dir: Path, bundle: Path, now: float) -> Result: - """Import one bundle if it is complete and not yet imported.""" + """Import one bundle if complete; repair the title of one already filed.""" if (bundle / MARKER).exists(): - return Result(bundle, Outcome.SKIPPED) + return self._repair(feedback_dir, bundle) report_path = bundle / "report.json" entry = find_entry(feedback_dir, bundle) if entry is None or not report_path.is_file(): @@ -126,13 +140,26 @@ class Importer: return Result(bundle, Outcome.PENDING, detail="report.json settling") repo = repo_name(feedback_dir) draft = build_task(repo, bundle.name, entry, read_report(report_path)) + return self._file(bundle, f"{self.owner}/{repo}", draft) + + def _file(self, bundle: Path, repo: str, draft: TaskDraft) -> Result: + """Create the task, write the marker, then fix the title.""" try: - ref = self.client.add_task(f"{self.owner}/{repo}", draft.title, draft.body) + ref = self.client.add_task(repo, draft.title, draft.body) except TodoError as exc: return Result(bundle, Outcome.FAILED, detail=str(exc)) - detail = "merged into an existing note" if ref.merged else "" + # Marker first: once the task exists a retry must never add a second. + # A merged task belongs to another note, which is not ours to rewrite. + payload: dict[str, object] = { + "task_id": ref.task_id, + "title": draft.title, + "due": draft.due, + "merged": ref.merged, + } + if ref.merged: + payload["format"] = FORMAT try: - _write_marker(bundle, draft, ref.task_id, merged=ref.merged) + _write_marker(bundle, payload) except OSError as exc: return Result( bundle, @@ -140,7 +167,39 @@ class Importer: ref.task_id, f"task {ref.task_id} filed but marker not written: {exc}", ) - return Result(bundle, Outcome.IMPORTED, ref.task_id, detail) + if ref.merged: + return Result( + bundle, Outcome.IMPORTED, ref.task_id, "merged into an existing note" + ) + return self._finish(bundle, payload, draft) + + def _repair(self, feedback_dir: Path, bundle: Path) -> Result: + """Finish a filed task whose rewrite never completed (or predates it).""" + data = _read_marker(bundle) + if data is None or data.get("merged") or data.get("format") == FORMAT: + return Result(bundle, Outcome.SKIPPED) + entry = find_entry(feedback_dir, bundle) + if entry is None: + return Result(bundle, Outcome.PENDING, detail="entry missing for repair") + report = read_report(bundle / "report.json") + draft = build_task(repo_name(feedback_dir), bundle.name, entry, report) + return self._finish(bundle, data, draft) + + def _finish( + self, bundle: Path, data: dict[str, object], draft: TaskDraft + ) -> Result: + """Rewrite the stored note to the exact title/body, then mark it done.""" + task_id = str(data["task_id"]) + try: + self.client.update_task(task_id, draft.title, draft.body) + except TodoError as exc: + return Result(bundle, Outcome.FAILED, task_id, f"update: {exc}") + marked = {**data, "title": draft.title, "due": draft.due, "format": FORMAT} + try: + _write_marker(bundle, marked) + except OSError as exc: + return Result(bundle, Outcome.FAILED, task_id, f"marker not written: {exc}") + return Result(bundle, Outcome.IMPORTED, task_id) @dataclass(frozen=True) diff --git a/feedback_inbox/feedback_inbox/_todo.py b/feedback_inbox/feedback_inbox/_todo.py index ff66413..8f48942 100644 --- a/feedback_inbox/feedback_inbox/_todo.py +++ b/feedback_inbox/feedback_inbox/_todo.py @@ -37,6 +37,10 @@ class TodoClient(Protocol): """Create one task in its own note and return its id.""" raise NotImplementedError + def update_task(self, task_id: str, title: str, body: str) -> None: + """Replace the task's stored title and body with exactly these.""" + raise NotImplementedError + class McpTodoClient: """Streamable-HTTP MCP client: initialize, then one ``add_task`` call.""" @@ -47,21 +51,37 @@ class McpTodoClient: self._token_file = token_file def add_task(self, repo: str, title: str, body: str) -> TaskRef: - """Create a task with its own note (exact title) via ``add_task``.""" + """Create a task with its own note via ``add_task``. + + The server stores a ``new_note`` title as ``<name> todos: <title>``; + :meth:`update_task` restores the exact title (and drops the + ``## <title>`` section the server adds) afterwards. + """ + arguments = {"repo": repo, "title": title, "body": body, "new_note": True} + said = self._call("add_task", arguments) + found = _REPLY.match(said) + if not found: + msg = f"add_task refused: {said[:120]}" + raise TodoError(msg) + return TaskRef(task_id=found.group(2), merged=found.group(1) != "added") + + def update_task(self, task_id: str, title: str, body: str) -> None: + """Rewrite via ``update_task``: exact title, and body with no sections.""" + said = self._call("update_task", {"id": task_id, "title": title, "body": body}) + if not said.startswith("updated"): + msg = f"update_task refused: {said[:120]}" + raise TodoError(msg) + + def _call(self, tool: str, arguments: dict[str, object]) -> str: token = self._token() session = self._initialize(token) - arguments = {"repo": repo, "title": title, "body": body, "new_note": True} - reply = self._post( - token, - { - "jsonrpc": "2.0", - "id": 2, - "method": "tools/call", - "params": {"name": "add_task", "arguments": arguments}, - }, - session, - )[1] - return _task_ref(reply) + payload = { + "jsonrpc": "2.0", + "id": 2, + "method": "tools/call", + "params": {"name": tool, "arguments": arguments}, + } + return _tool_text(self._post(token, payload, session)[1]) def _token(self) -> str: try: @@ -126,7 +146,8 @@ class McpTodoClient: raise TodoError(msg) from exc -def _task_ref(body: str) -> TaskRef: +def _tool_text(body: str) -> str: + """The text a tool call answered; an MCP-level error becomes TodoError.""" text = body.strip() if not text.startswith("{"): lines = [x[5:].strip() for x in text.splitlines() if x.startswith("data:")] @@ -138,8 +159,7 @@ def _task_ref(body: str) -> TaskRef: except (ValueError, KeyError, IndexError, TypeError) as exc: msg = f"unparsable MCP reply: {body[:80]!r}" raise TodoError(msg) from exc - found = _REPLY.match(said) - if failed or not found: - msg = f"add_task refused: {said[:120]}" + if failed: + msg = f"tool refused: {said[:120]}" raise TodoError(msg) - return TaskRef(task_id=found.group(2), merged=found.group(1) != "added") + return said diff --git a/feedback_inbox/feedback_inbox/tests/test_inbox.py b/feedback_inbox/feedback_inbox/tests/test_inbox.py index 3abe774..d0190d4 100644 --- a/feedback_inbox/feedback_inbox/tests/test_inbox.py +++ b/feedback_inbox/feedback_inbox/tests/test_inbox.py @@ -42,7 +42,7 @@ def test_complete_bundle_becomes_one_task(paths: Paths) -> None: title == MANUAL + "cleanup-plate feedback: I would like to see the painting IN FULL" ) - assert body.startswith("Due: 2026-11-12 10:00\n\n## 2026-11-11T13:52:33") + assert body.startswith("Due: 2026-11-12 10:00\n\n### 2026-11-11T13:52:33") assert f"{bundle}/report.json" in body marker = json.loads((bundle / MARKER).read_text(encoding="utf-8")) assert marker["task_id"] == "id0001" + "0" * 28 diff --git a/feedback_inbox/feedback_inbox/tests/test_inbox_repair.py b/feedback_inbox/feedback_inbox/tests/test_inbox_repair.py new file mode 100644 index 0000000..484687a --- /dev/null +++ b/feedback_inbox/feedback_inbox/tests/test_inbox_repair.py @@ -0,0 +1,145 @@ +# Copyright (c) 2026 Krzysztof Rudnicki +"""Exact titles, section-free bodies, and repair of half-finished tasks.""" + +from __future__ import annotations + +import json +from typing import TYPE_CHECKING + +from feedback_inbox._entry import Report +from feedback_inbox._inbox import ( + FORMAT, + MARKER, + Importer, + Outcome, + build_task, + run_once, +) +from feedback_inbox.tests.test_support import STAMP, FakeTodo, make_bundle + +if TYPE_CHECKING: + from pathlib import Path + + from feedback_inbox._paths import Paths + +NOW = 1_000_000.0 +OWNER = "kuhyx" +MANUAL = "MANUAL WORK REQUIRED: " + + +def _run(paths: Paths, todo: FakeTodo) -> list[Outcome]: + return [r.outcome for r in run_once(paths.data_root, Importer(todo, OWNER), NOW)] + + +def _marker(bundle: Path) -> dict[str, object]: + data: dict[str, object] = json.loads((bundle / MARKER).read_text(encoding="utf-8")) + return data + + +def _no_extra_sections(text: str) -> bool: + return not any(line.startswith("## ") for line in text.splitlines()) + + +def test_stored_note_is_one_manual_part(paths: Paths) -> None: + bundle = make_bundle(paths.data_root) + todo = FakeTodo() + assert _run(paths, todo) == [Outcome.IMPORTED] + (stored,) = todo.stored.values() + assert stored.startswith(MANUAL) + assert _no_extra_sections(stored) + assert "### 2026-11-11T13:52:33" in stored + assert _marker(bundle)["format"] == FORMAT + + +def test_entry_headings_never_become_sections() -> None: + entry = "## a heading\ntext\n## another\n### kept" + draft = build_task("r", STAMP, entry, Report(note="n", game="", combo="")) + assert _no_extra_sections(draft.body) + assert "### a heading\ntext\n### another\n### kept" in draft.body + + +def test_failed_update_keeps_marker_and_retries_without_a_second_task( + paths: Paths, +) -> None: + bundle = make_bundle(paths.data_root) + todo = FakeTodo(fail_update=True) + results = run_once(paths.data_root, Importer(todo, OWNER), NOW) + assert [r.outcome for r in results] == [Outcome.FAILED] + assert "update refused" in results[0].detail + assert "format" not in _marker(bundle) + todo.fail_update = False + assert _run(paths, todo) == [Outcome.IMPORTED] + assert len(todo.calls) == 1 + (stored,) = todo.stored.values() + assert stored.startswith(MANUAL) + assert _no_extra_sections(stored) + assert _run(paths, todo) == [Outcome.SKIPPED] + + +def test_old_marker_is_repaired_from_the_bundle(paths: Paths) -> None: + bundle = make_bundle(paths.data_root) + task_id = "abc" + "0" * 33 + (bundle / MARKER).write_text( + json.dumps({"task_id": task_id, "title": "old", "title_fixed": True}), + encoding="utf-8", + ) + todo = FakeTodo() + assert _run(paths, todo) == [Outcome.IMPORTED] + assert todo.calls == [] + assert todo.stored[task_id].startswith(MANUAL) + assert _no_extra_sections(todo.stored[task_id]) + assert _marker(bundle)["format"] == FORMAT + + +def test_old_marker_whose_entry_vanished_waits(paths: Paths) -> None: + bundle = make_bundle(paths.data_root, entry=False) + (bundle / MARKER).write_text( + json.dumps({"task_id": "t" * 36, "title": "old"}), encoding="utf-8" + ) + todo = FakeTodo() + assert _run(paths, todo) == [Outcome.PENDING] + assert todo.updated == [] + + +def test_merged_task_is_not_rewritten(paths: Paths) -> None: + bundle = make_bundle(paths.data_root) + todo = FakeTodo(merged=True) + assert _run(paths, todo) == [Outcome.IMPORTED] + assert todo.updated == [] + assert _marker(bundle)["format"] == FORMAT + assert _run(paths, todo) == [Outcome.SKIPPED] + + +def test_old_merged_marker_is_left_alone(paths: Paths) -> None: + bundle = make_bundle(paths.data_root) + (bundle / MARKER).write_text( + json.dumps({"task_id": "t" * 36, "title": "x", "merged": True}), + encoding="utf-8", + ) + todo = FakeTodo() + assert _run(paths, todo) == [Outcome.SKIPPED] + assert todo.updated == [] + + +def test_unreadable_marker_is_left_alone(paths: Paths) -> None: + bundle = make_bundle(paths.data_root) + for junk in ("{broken", "[1]", '{"task_id": 5}'): + (bundle / MARKER).write_text(junk, encoding="utf-8") + todo = FakeTodo() + assert _run(paths, todo) == [Outcome.SKIPPED] + assert todo.calls == [] + assert todo.updated == [] + + +def test_repair_reports_an_unwritable_marker(paths: Paths) -> None: + bundle = make_bundle(paths.data_root) + (bundle / MARKER).write_text( + json.dumps({"task_id": "t" * 36, "title": "T"}), encoding="utf-8" + ) + bundle.chmod(0o500) + try: + results = run_once(paths.data_root, Importer(FakeTodo(), OWNER), NOW) + finally: + bundle.chmod(0o700) + assert results[0].outcome is Outcome.FAILED + assert "marker not written" in results[0].detail diff --git a/feedback_inbox/feedback_inbox/tests/test_support.py b/feedback_inbox/feedback_inbox/tests/test_support.py index f407844..857bc0b 100644 --- a/feedback_inbox/feedback_inbox/tests/test_support.py +++ b/feedback_inbox/feedback_inbox/tests/test_support.py @@ -66,17 +66,30 @@ def make_bundle( @dataclass class FakeTodo: - """Records add_task calls; optionally fails or reports a merge.""" + """A todo server stand-in that stores notes the way the real one does.""" calls: list[tuple[str, str, str]] = field(default_factory=list) + stored: dict[str, str] = field(default_factory=dict) fail: bool = False + fail_update: bool = False merged: bool = False + updated: list[str] = field(default_factory=list) def add_task(self, repo: str, title: str, body: str) -> TaskRef: if self.fail: msg = "todo down" raise TodoError(msg) self.calls.append((repo, title, body)) - return TaskRef( - task_id=f"id{len(self.calls):04d}" + "0" * 28, merged=self.merged - ) + task_id = f"id{len(self.calls):04d}" + "0" * 28 + # A new_note task: title "<name> todos: <title>", then a "## <title>" + # section holding the body. + name = repo.rsplit("/", maxsplit=1)[-1] + self.stored[task_id] = f"{name} todos: {title}\n\n## {title}\n{body}" + return TaskRef(task_id=task_id, merged=self.merged) + + def update_task(self, task_id: str, title: str, body: str) -> None: + if self.fail_update: + msg = "update refused" + raise TodoError(msg) + self.updated.append(task_id) + self.stored[task_id] = f"{title}\n\n{body}" diff --git a/feedback_inbox/feedback_inbox/tests/test_todo.py b/feedback_inbox/feedback_inbox/tests/test_todo.py index e248da8..789a54f 100644 --- a/feedback_inbox/feedback_inbox/tests/test_todo.py +++ b/feedback_inbox/feedback_inbox/tests/test_todo.py @@ -182,3 +182,19 @@ def test_garbage_json(server: str, tmp_path: Path) -> None: httpd.shutdown() httpd.server_close() thread.join() + + +def test_update_task_replaces_title_and_body(server: str, tmp_path: Path) -> None: + _Handler.tool_text = f"updated {FULL_ID}\nline" + _client(server, tmp_path).update_task(FULL_ID, "MANUAL WORK REQUIRED: x", "b") + call = _Handler.seen[2]["params"] + assert call == { + "name": "update_task", + "arguments": {"id": FULL_ID, "title": "MANUAL WORK REQUIRED: x", "body": "b"}, + } + + +def test_update_task_refused(server: str, tmp_path: Path) -> None: + _Handler.tool_text = "no such task" + with pytest.raises(TodoError, match="no such task"): + _client(server, tmp_path).update_task(FULL_ID, "t", "b") diff --git a/feedback_inbox/install.sh b/feedback_inbox/install.sh index 76e1175..0bde12e 100755 --- a/feedback_inbox/install.sh +++ b/feedback_inbox/install.sh @@ -72,8 +72,9 @@ verify() { for unit in feedback-inbox.path feedback-inbox-sync.path feedback-inbox-sync.timer; do systemctl --user is-active --quiet "$unit" || fail "$unit is not active" done - log "ok: watching $(systemctl --user show feedback-inbox.path -p PathChanged \ - | tr ';' '\n' | grep -c feedback) feedback folder(s)" + local folders + folders="$(systemctl --user show feedback-inbox.path -p Paths | grep -c '/feedback (PathChanged)' || true)" + log "ok: watching ${folders} feedback folder(s)" } main() {