Merge pull request 'nisse-review: read long files in parts; a retry starts afresh' (#157) from review-reads-and-retries into main

Reviewed-on: #157
Reviewed-by: Don Garrett <dgarrett@acm.org>

docs/Development-Plan.md

@@ -613,7 +613,9 @@ waits until two exist to compare. Moved, unchanged in behaviour:
   is checked every round, not only at startup.
 - **Reading (R3):** the repository at the PR's head and base, from Gitea's
   archives, unpacked into a scratch folder and read in Python (`read_file`,
-  `list_files`, `search_code`), and its issues (`read_issue`,
+  `list_files`, `search_code`; a long file is read in parts, `start` giving
+  the line, since a file cut at 20,000 characters left its end out of reach),
+  and its issues (`read_issue`,
   `list_issues`): no git, no shell, bound to the item's repository.
 - **A review:** starts from the PR, its commits, diff, comments, the issues it
   closes and CI; `comment` drafts a point on any line of any file (checked to
@@ -626,7 +628,10 @@ waits until two exist to compare. Moved, unchanged in behaviour:
   whether the review (marked with its head) was posted, and nothing is posted
   twice; with Gitea down, the item waits.
 - **Failures:** a failed run is tried again; after `tuning.review.attempts`, it
-  says so on the PR and waits for the next push.
+  says so on the PR and waits for the next push. Each run starts afresh from its
+  brief (the log keeps the rest), and one out of turns with points drafted posts
+  them as a partial review: on #156 the reviewer ran out of turns, then its retry,
+  carrying every read before it, timed out, and both drafts were lost.
 - **Access:** startup refuses an account that can push to, or administer, any
   repository it reads. The token needs `read:user`, `write:issue` and
   `write:repository` scopes; the account's read access is the limit.

nisse-core/src/nisse_core/state/store.py

@@ -554,10 +554,13 @@ class Transcript:
     nor write without attribution.
     """
 
-    def __init__(self, store: StateStore, work_item: str | None, actor: str) -> None:
+    def __init__(
+        self, store: StateStore, work_item: str | None, actor: str, start: int = 0
+    ) -> None:
         self._store = store
         self.work_item = work_item
         self.actor = actor
+        self._start = start  # the view begins after this event (``since``)
 
     def append(self, event: CanonicalEvent) -> int:
         """Committed and fsynced before return, like every durable write."""
@@ -570,7 +573,14 @@ class Transcript:
     def view(self, after: int = 0) -> list[StoredEvent]:
         """The log as this actor is shown it: what a model's context is
         built from (L6). See ``views``."""
-        return project(self.read(after), self.actor, self._store.SHOWN)
+        return project(
+            self.read(max(after, self._start)), self.actor, self._store.SHOWN
+        )
+
+    def since(self, event_id: int) -> "Transcript":
+        """The same transcript, its view starting after ``event_id``: a run
+        that starts afresh, while the log keeps everything before it."""
+        return Transcript(self._store, self.work_item, self.actor, event_id)
 
     def as_actor(self, actor: str) -> "Transcript":
         """The same work item, written by another role: e.g. the executor

nisse-core/tests/test_state.py

@@ -121,3 +121,17 @@ def test_newer_schema_is_refused(db: Path) -> None:
         pass
     with pytest.raises(StateSchemaError, match="newer"):
         StateStore(db, schema_version=1)
+
+
+def test_a_transcript_since_an_event_views_only_what_follows(tmp_path: Path) -> None:
+    from nisse_core.log.canonical import Fact
+
+    with StateStore(tmp_path / "s.db") as store:
+        store.open_work_item("w", title="t", scope="r", position="p", cause="t")
+        whole = store.transcript("w", "orchestrator")
+        first = whole.append(Fact(text="the failed attempt"))
+        whole.append(Fact(text="the retry's brief"))
+        fresh = whole.since(first)
+        assert [e.event.text for e in fresh.view()] == ["the retry's brief"]
+        assert len(fresh.read()) == 2  # the log keeps everything
+        assert len(whole.view()) == 2

nisse-review/src/nisse_review/reader.py

@@ -27,9 +27,11 @@ MAX_ISSUES = 20
 REF = "head (the PR's code) or base (the branch it merges into)."
 READ_FILE = tool(
     "read_file",
-    "Read a file in the repository. Its lines are numbered, for comment.",
+    "Read a file in the repository. Its lines are numbered, for comment. A "
+    "long file comes in parts: the end of each says where the next starts.",
     path="The file's path from the repository root.",
     ref=REF,
+    start="Optional: the line to start from (1, the default, is the top).",
 )
 LIST_FILES = tool(
     "list_files",
@@ -131,11 +133,27 @@ class Repository:
         lines = self.lines(path, ref)
         if lines is None:
             return ToolOutcome(f"{path}: no such file at {ref}", False)
-        text = "\n".join(f"{n:>5}  {line}" for n, line in enumerate(lines, 1))
-        if len(text) > MAX_FILE_CHARS:
-            more = len(text) - MAX_FILE_CHARS
-            text = text[:MAX_FILE_CHARS] + f"\n[{more} more characters not shown]"
-        return ToolOutcome(f"[{self.repo}@{ref}: {path}]\n{text}", True)
+        raw = str(arguments.get("start") or "1").strip()
+        if not raw.isdigit() or not 1 <= int(raw) <= max(len(lines), 1):
+            return ToolOutcome(f"start is a line number from 1 to {len(lines)}", False)
+        # A part of at most MAX_FILE_CHARS, in whole lines; the rest is a
+        # read away, never out of reach (#156: a long file's end was).
+        first = int(raw)
+        shown: list[str] = []
+        size = 0
+        for n in range(first, len(lines) + 1):
+            row = f"{n:>5}  {lines[n - 1]}"
+            if shown and size + len(row) + 1 > MAX_FILE_CHARS:
+                break
+            shown.append(row[:MAX_FILE_CHARS])
+            size += len(row) + 1
+        last = first + len(shown) - 1
+        where = f"lines {first}-{last} of {len(lines)}"
+        more = (
+            f"\n[{where}: read on with start={last + 1}]" if last < len(lines) else ""
+        )
+        text = "\n".join(shown)
+        return ToolOutcome(f"[{self.repo}@{ref}: {path}, {where}]\n{text}{more}", True)
 
     def _list(self, arguments: Mapping[str, Any]) -> ToolOutcome:
         folder = str(arguments.get("path", "")).strip().strip("/")

nisse-review/src/nisse_review/review.py

@@ -118,6 +118,10 @@ class Reviewer:
         """The review run: it ends waiting on post_review, or not."""
         transcript = self._store.transcript(item_id, "driver")
         start = transcript.read()[-1].id if transcript.read() else 0
+        # Each run starts afresh from its brief: a retry doesn't carry the
+        # failed run's reads (#156: they filled the context, and a call timed
+        # out). The log keeps them.
+        transcript = transcript.since(start)
         self._brief(transcript, repo.repo, pr)
         project = _project(repo.repo)
         index = self._knowledge.index(item_id, project)

nisse-review/src/nisse_review/scheduler.py

@@ -38,6 +38,12 @@ from nisse_review.state import (
     pr_of,
 )
 
+PARTIAL = (
+    "A partial review: I ran out of turns before finishing, so these are the "
+    "{n} point(s) I had drafted, without an overall view. A new push gets a "
+    "full look."
+)
+
 _WIP = re.compile(r"^\s*(\[wip\]|wip\b)", re.IGNORECASE)
 
 
@@ -238,10 +244,18 @@ class Scheduler:
                 outcome, draft = self._reviewer.run(item.id, pr, repo)
             finally:
                 repo.close()
-            if outcome.kind != "wait":
+            if outcome.kind == "capped" and draft.comments:
+                # Out of turns with points drafted: they're worth more posted
+                # than lost (#156), as a review that says it's partial.
+                self._oplog.emit(
+                    "review.partial", cause=outcome.note, work_item=item.id
+                )
+                summary = PARTIAL.format(n=len(draft.comments))
+            elif outcome.kind != "wait":
                 self._failed(item, known, pr, outcome.note or outcome.kind)
                 return
-            summary = str((outcome.arguments or {}).get("summary", ""))
+            else:
+                summary = str((outcome.arguments or {}).get("summary", ""))
             posted = self._reviewer.post(item.id, known.repo, pr, summary, draft)
         except GiteaError as exc:
             self._failed(item, known, None, f"Gitea: {exc}")

nisse-review/tests/test_review.py

@@ -553,3 +553,67 @@ def test_the_config_names_every_problem(tmp_path: Path) -> None:
         ReviewConfig.from_tree({**tree, "matrix": {"x": 1}}, {"LLM": "k"})
     assert "matrix.x: unknown setting" in str(raised.value)
     assert "references ${GITEA}" in str(raised.value)
+
+
+def test_a_long_file_is_read_in_parts_to_its_end(tmp_path: Path) -> None:
+    # #156: read_file stopped at 20,000 characters with no way on, so the
+    # reviewer hunted the rest a line at a time and ran out of turns.
+    from nisse_review.reader import MAX_FILE_CHARS, Repository
+
+    body = "".join(f"x{n} = {'y' * 60}\n" for n in range(1, 1001))
+    gitea = FakeGitea(files={"long.py": body})
+    repo = Repository(gitea, REPO, {"head": "abc"}, tmp_path)  # type: ignore[arg-type]
+    first = repo.dispatch("read_file", {"path": "long.py"}).result
+    assert len(first) < MAX_FILE_CHARS + 200
+    assert "lines 1-" in first and "of 1000" in first
+    seen, start = [], 1
+    while True:
+        part = repo.dispatch("read_file", {"path": "long.py", "start": start})
+        assert part.success
+        seen += [
+            int(r.split()[0])
+            for r in part.result.splitlines()[1:]
+            if r[:5].strip().isdigit()
+        ]
+        if "read on with start=" not in part.result:
+            break
+        start = int(part.result.rsplit("start=", 1)[1].rstrip("]"))
+    assert seen == list(range(1, 1001))  # every line, once
+    bad = repo.dispatch("read_file", {"path": "long.py", "start": "2000"})
+    assert not bad.success and "1 to 1000" in bad.result
+
+
+def test_a_retry_starts_afresh_from_its_brief(tmp_path: Path) -> None:
+    # #156: the retry carried the failed run's reads, and a call timed out.
+    prose = [AssistantText(text="Reading on...", model="m")]
+    model = Scripted(prose, prose, *REVIEW)  # prose twice: the first run fails
+    w = world(tmp_path, model, attempts=2)
+    w["scheduler"].round()
+    assert pr_of(item(w)).attempts == 1
+    w["scheduler"].round()
+    retry = model.seen[2]  # the retry's first call
+    briefs = [e for e in retry if "Review o/demo#7" in str(e.event)]
+    assert len(briefs) == 1 and retry[0] is briefs[0]
+    assert not any("Reading on" in str(e.event) for e in retry)
+    assert item(w).position == REVIEWED
+
+
+def test_out_of_turns_its_drafted_points_are_posted_as_partial(
+    tmp_path: Path,
+) -> None:
+    model = Scripted(*REVIEW[:3])  # drafts two comments, never posts
+    w = world(tmp_path, model)
+    w["scheduler"]._reviewer._max_turns = 3
+    w["scheduler"].round()
+    [posted] = w["gitea"].posted
+    assert len(posted["comments"]) == 2 and "A partial review" in posted["body"]
+    assert item(w).position == REVIEWED
+
+
+def test_out_of_turns_with_nothing_drafted_is_a_failed_run(tmp_path: Path) -> None:
+    model = Scripted(REVIEW[0], REVIEW[0], REVIEW[0])
+    w = world(tmp_path, model, attempts=2)
+    w["scheduler"]._reviewer._max_turns = 3
+    w["scheduler"].round()
+    assert w["gitea"].posted == []
+    assert (item(w).position, pr_of(item(w)).attempts) == (READY, 1)