Compare commits

..
4 Commits
Author SHA1 Message Date
dongarandClaude Opus 5.5 fca4c41d95 Later review test: its earlier summary is in the brief too
CI / checks (push) Skipped
CI / checks (pull_request) Successful in 2m8s
CI / e2e (pull_request) Successful in 4m49s
From nisse-review's draft on #156: it thought points made in a review's
summary never reached a later review. They do, as "your review: ...".

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01J5Utyfzw7gF7UhjRDEtjTk
2026-10-17 07:06:10 +00:00
dongarandClaude Opus 5.5 37bbac7e72 nisse-review: later reviews, replies, rounds, stop and review (M7.5)
Back on a PR it reviewed: a new head gets a later review of the commits
since (each one's diff, through the API) and of its earlier points; what
was said since gets replies, answered where it was said or skipped with a
reason. It signs off once and stays quiet while still happy. Past
tuning.review.max_rounds it sums up for the operator and goes quiet,
until a push or someone other than the PR's author joins in. An
operator's `stop` quiets it; `review` asks for a fresh review.

Reviews carry numbered markers and replies their point's key, so
recovery posts nothing twice.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01J5Utyfzw7gF7UhjRDEtjTk
2026-10-17 07:06:06 +00:00
dongar 7e73b8fb37 Merge pull request 'nisse-review: read long files in parts; a retry starts afresh' (#157) from review-reads-and-retries into main
CI / checks (push) Successful in 3m6s
CI / e2e (push) Skipped
Reviewed-on: #157
Reviewed-by: Don Garrett <dgarrett@acm.org>
2026-10-17 07:03:43 +00:00
dongarandClaude Opus 5.5 1a7836167d nisse-review: read long files in parts; a retry starts afresh
CI / checks (push) Skipped
CI / checks (pull_request) Successful in 1m51s
CI / e2e (pull_request) Successful in 4m38s
From its failed review of #156: read_file stopped at 20,000 characters
with no way on, so the reviewer hunted each file's end a definition at a
time and ran out of turns; its retry carried every read before it, and a
model call timed out. Both drafts were lost.

- read_file takes a start line and says where the next part starts.
- Each run's view starts at its brief (Transcript.since); the log keeps
  the rest.
- A run out of turns with points drafted posts them as a partial review.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01J5Utyfzw7gF7UhjRDEtjTk
2026-10-17 06:48:47 +00:00
8 changed files with 147 additions and 16 deletions

No files matched your search

+7 -2
View File
@@ -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.
+12 -2
View File
@@ -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
+14
View File
@@ -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
+24 -6
View File
@@ -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("/")
+4
View File
@@ -235,6 +235,10 @@ class Reviewer:
said = said_on(self._gitea, repo.repo, pr.number)
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)
draft = Draft()
if kind != FIRST: # a first review's brief has everything said
by_key = {one.key: one for one in said}
+20 -5
View File
@@ -55,6 +55,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)
# The ending tools that post a review.
@@ -379,13 +385,22 @@ class Scheduler:
)
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
)
tool, summary = "post_review", PARTIAL.format(n=len(draft.comments))
elif outcome.kind != "wait":
self._failed(item, known, pr, outcome.note or outcome.kind)
return
posted = None
if outcome.tool in _POSTING:
else:
tool = outcome.tool
summary = str((outcome.arguments or {}).get("summary", ""))
kept = draft if outcome.tool == "post_review" else Draft()
posted = None
if tool in _POSTING:
kept = draft if tool == "post_review" else Draft()
posted = self._reviewer.post(
item.id, known.repo, pr, summary, kept, n=known.posts + 1
)
@@ -395,7 +410,7 @@ class Scheduler:
except Exception as exc: # noqa: BLE001 -- counted, never a crash loop
self._failed(item, known, None, repr(exc))
return
self._settle(item, known, pr, outcome.tool, draft, posted)
self._settle(item, known, pr, tool, draft, posted)
def _settle(
self,
+64
View File
@@ -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)
+2 -1
View File
@@ -120,7 +120,8 @@ def test_a_push_gets_a_later_review_of_what_changed(tmp_path: Path) -> None:
assert w["scheduler"].round() is True
brief = last_brief(w)
assert "The commits since" in brief and f"diff of {PUSHED}" in brief
assert "split() handles tabs too." in brief # its earlier point
assert "split() handles tabs too." in brief # its earlier point on a line
assert "- your review: Looks right; two small points." in brief # its summary
review = w["gitea"].posted[-1]
assert review["commit"] == PUSHED and marker(PUSHED, 2) in review["body"]
known = pr_of(item(w))