diff --git a/docs/Development-Plan.md b/docs/Development-Plan.md index dec30b4..9224a73 100644 --- a/docs/Development-Plan.md +++ b/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. diff --git a/nisse-core/src/nisse_core/state/store.py b/nisse-core/src/nisse_core/state/store.py index 25d2852..e9b3a72 100644 --- a/nisse-core/src/nisse_core/state/store.py +++ b/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 diff --git a/nisse-core/tests/test_state.py b/nisse-core/tests/test_state.py index 6fce1ef..86ecd06 100644 --- a/nisse-core/tests/test_state.py +++ b/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 diff --git a/nisse-review/src/nisse_review/reader.py b/nisse-review/src/nisse_review/reader.py index 965c00d..51d0f09 100644 --- a/nisse-review/src/nisse_review/reader.py +++ b/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("/") diff --git a/nisse-review/src/nisse_review/review.py b/nisse-review/src/nisse_review/review.py index 74285ae..6d44647 100644 --- a/nisse-review/src/nisse_review/review.py +++ b/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) diff --git a/nisse-review/src/nisse_review/scheduler.py b/nisse-review/src/nisse_review/scheduler.py index dd865f7..310a2e4 100644 --- a/nisse-review/src/nisse_review/scheduler.py +++ b/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}") diff --git a/nisse-review/tests/test_review.py b/nisse-review/tests/test_review.py index 5b85c95..6fe0e74 100644 --- a/nisse-review/tests/test_review.py +++ b/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)