Skip to content

Collapse two pieces of copy-paste plumbing #408

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
dovvnloading merged 1 commit into from
Sep 4, 2026
Merged

Collapse two pieces of copy-paste plumbing #408

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 22 additions & 18 deletions backend/agent_dispatch/gitlink.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -352,19 +352,32 @@ async def start_gitlink_apply(
request_id = handle.request_id
node.pending_request_id = request_id

if not node.state.gitlink_pending_changes:
async def _abandon(reason: str) -> None:
"""Give back everything this call claimed, and say why.

Five refusal paths below shared these four lines verbatim. That
is four chances each to forget one, and forgetting the first two
is not a cosmetic slip: a missed `pending_request_id = None`
leaves the node permanently busy, and a missed release() leaks
the registry slot for the rest of the session.

Its `await` sits on the ABANDON path only, so the atomic
check-and-freeze section further down keeps its zero-await
guarantee - the comparisons there still run with no suspension
point between them; this only runs once one of them has already
decided to give up."""
node.pending_request_id = None
self._runs.release(request_id)
on_failure("There is no approved change set to write.")
on_failure(reason)
await bus.publish("scene")

if not node.state.gitlink_pending_changes:
await _abandon("There is no approved change set to write.")
return

local_root_text = (local_root or "").strip()
if not local_root_text:
node.pending_request_id = None
self._runs.release(request_id)
on_failure("Select or import a local repository path before applying changes.")
await bus.publish("scene")
await _abandon("Select or import a local repository path before applying changes.")
return
local_root_path = Path(local_root_text).expanduser()
# R5.3 post-review FIX 3: wrapped in asyncio.to_thread, like every
Expand All @@ -379,10 +392,7 @@ async def start_gitlink_apply(
# control.
local_root_exists = await asyncio.to_thread(local_root_path.exists)
if not local_root_exists:
node.pending_request_id = None
self._runs.release(request_id)
on_failure("The selected local repository path does not exist.")
await bus.publish("scene")
await _abandon("The selected local repository path does not exist.")
return

# --- Atomic check-and-freeze: NO await between these statements. ---
Expand All @@ -391,10 +401,7 @@ async def start_gitlink_apply(
client_fingerprint != current_fingerprint
or current_fingerprint != node.state.gitlink_change_fingerprint
):
node.pending_request_id = None
self._runs.release(request_id)
on_failure("The proposed change set changed after approval. Review it again before applying.")
await bus.publish("scene")
await _abandon("The proposed change set changed after approval. Review it again before applying.")
return
# R5.3 post-review FIX 2: the fingerprint above says nothing about
# WHERE the content is written - _fingerprint_changes only hashes
Expand All @@ -408,13 +415,10 @@ async def start_gitlink_apply(
# and how document.complete_gitlink_run records
# gitlink_change_local_root.
if local_root_text != (node.state.gitlink_change_local_root or ""):
node.pending_request_id = None
self._runs.release(request_id)
on_failure(
await _abandon(
"The local repository path changed since this proposal was generated. "
"Regenerate the change set before applying."
)
await bus.publish("scene")
return
changes_snapshot = [dict(item) for item in node.state.gitlink_pending_changes]
# --- End atomic section. Everything past this point operates ONLY on
Expand Down
20 changes: 1 addition & 19 deletions backend/api/intents_code_review.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,25 +60,7 @@ async def fetch_code_review_diff(node_id, pr_url=None):
bus=bus, notifications_state=notifications, node=node, pr_url=effective_url,
)
if bundle is not None:
document.store_code_review_diff(
node_id,
pr_url=effective_url,
repo=bundle.get("repo", ""),
pr_number=bundle.get("pr_number", 0),
pr_title=bundle.get("pr_title", ""),
pr_state=bundle.get("pr_state", ""),
html_url=bundle.get("html_url", ""),
base_ref=bundle.get("base_ref", ""),
head_ref=bundle.get("head_ref", ""),
additions=bundle.get("additions", 0),
deletions=bundle.get("deletions", 0),
changed_files=bundle.get("changed_files", 0),
files=bundle.get("files", []),
files_truncated=bundle.get("files_truncated", False),
diff_text=bundle.get("diff_text", ""),
diff_truncated=bundle.get("diff_truncated", False),
diff_chars=bundle.get("diff_chars", 0),
)
document.store_code_review_diff(node_id, pr_url=effective_url, bundle=bundle)
await publish_scene()
return node_id

Expand Down
82 changes: 44 additions & 38 deletions backend/domain/graph.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,20 @@ def _estimate_tokens(text: str) -> int:
return TokenEstimator().count_tokens(text)


def _bundle_int(value: object) -> int:
"""Non-negative int from a fetch bundle, defaulting to 0.

The bundle is assembled from a GitHub API response, so every numeric
field in it is external input. graphlink_plugins/review_lens/diff_fetch.py
already coerces on the way in; this is the second line of the same
defence, for a bundle that reached here by any other route (a test, a
future caller, a hand-built payload)."""
try:
return max(0, int(value)) # type: ignore[arg-type]
except (TypeError, ValueError):
return 0


@dataclass
class SceneDocument(BranchOps, GroupOps, LayoutOps, CommandOps):
"""The canvas document for one session. Plain data + invariants; the
Expand Down Expand Up @@ -1414,27 +1428,20 @@ def set_code_review_pr_url(self, node_id: str, pr_url: str) -> SceneNode:
return node

def store_code_review_diff(
self,
node_id: str,
*,
pr_url: str,
repo: str,
pr_number: int,
pr_title: str,
pr_state: str,
html_url: str,
base_ref: str,
head_ref: str,
additions: int,
deletions: int,
changed_files: int,
files: list,
files_truncated: bool,
diff_text: str,
diff_truncated: bool,
diff_chars: int,
self, node_id: str, *, pr_url: str, bundle: dict,
) -> SceneNode:
"""Lands a successful fetchCodeReviewDiff result. A new fetch
"""Lands a successful fetchCodeReviewDiff result.

Takes the fetch bundle whole rather than as 15 separate keyword
arguments. It used to be the latter, which made this the widest
signature in the repo at 18 parameters - and every one of them was
pure transport: the caller pulled 15 values out of a dict with
.get() only for this method to set them straight onto node.state.
`pr_url` stays separate because it is not part of the bundle: it is
what the user typed, kept even when the fetch that used it is
superseded.

A new fetch
supersedes any prior review on this node (walkthrough, findings,
errors, verdict, Q&A, and dismissals are all reset) - reviewing
against a stale diff's findings would be worse than showing none,
Expand All @@ -1448,26 +1455,25 @@ def store_code_review_diff(
raise SceneError(f"unknown node: {node_id}")
if node.kind != "code_review":
raise SceneError(f"node is not a code_review node: {node_id}")
try:
pr_number_value = max(0, int(pr_number))
except (TypeError, ValueError):
pr_number_value = 0
pr_number_value = _bundle_int(bundle.get("pr_number"))
node.state.code_review_pr_url = str(pr_url)
node.state.code_review_repo = str(repo)
node.state.code_review_repo = str(bundle.get("repo", ""))
node.state.code_review_pr_number = pr_number_value
node.state.code_review_pr_title = str(pr_title)
node.state.code_review_pr_state = str(pr_state)
node.state.code_review_pr_html_url = str(html_url)
node.state.code_review_base_ref = str(base_ref)
node.state.code_review_head_ref = str(head_ref)
node.state.code_review_additions = max(0, int(additions or 0))
node.state.code_review_deletions = max(0, int(deletions or 0))
node.state.code_review_changed_files = max(0, int(changed_files or 0))
node.state.code_review_files = [dict(entry) for entry in (files or []) if isinstance(entry, dict)]
node.state.code_review_files_truncated = bool(files_truncated)
node.state.code_review_diff_text = str(diff_text)
node.state.code_review_diff_truncated = bool(diff_truncated)
node.state.code_review_diff_chars = max(0, int(diff_chars or 0))
node.state.code_review_pr_title = str(bundle.get("pr_title", ""))
node.state.code_review_pr_state = str(bundle.get("pr_state", ""))
node.state.code_review_pr_html_url = str(bundle.get("html_url", ""))
node.state.code_review_base_ref = str(bundle.get("base_ref", ""))
node.state.code_review_head_ref = str(bundle.get("head_ref", ""))
node.state.code_review_additions = _bundle_int(bundle.get("additions"))
node.state.code_review_deletions = _bundle_int(bundle.get("deletions"))
node.state.code_review_changed_files = _bundle_int(bundle.get("changed_files"))
node.state.code_review_files = [
dict(entry) for entry in (bundle.get("files") or []) if isinstance(entry, dict)
]
node.state.code_review_files_truncated = bool(bundle.get("files_truncated", False))
node.state.code_review_diff_text = str(bundle.get("diff_text", ""))
node.state.code_review_diff_truncated = bool(bundle.get("diff_truncated", False))
node.state.code_review_diff_chars = _bundle_int(bundle.get("diff_chars"))
node.state.code_review_diff_version += 1
node.state.code_review_walkthrough = []
node.state.code_review_findings = []
Expand Down
23 changes: 11 additions & 12 deletions backend/tests/test_review_lens_backend.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ def test_set_pr_url_and_wrong_kind_guard():

def test_store_diff_lands_fields_bumps_version_and_resets_review():
doc, node = _doc_with_review()
doc.store_code_review_diff(node.id, pr_url="u", **{k: v for k, v in _bundle().items()})
doc.store_code_review_diff(node.id, pr_url="u", bundle=_bundle())
live = doc.nodes[node.id]
assert live.state.code_review_repo == "o/r"
assert live.state.code_review_pr_number == 3
Expand All @@ -113,7 +113,7 @@ def test_store_diff_lands_fields_bumps_version_and_resets_review():
"findings": live.state.code_review_findings, "errors": [], "scores": {},
"quality_score": 1, "verdict": "strong", "risk": "low", "quality_summary": "S",
})
doc.store_code_review_diff(node.id, pr_url="u", **{k: v for k, v in _bundle().items()})
doc.store_code_review_diff(node.id, pr_url="u", bundle=_bundle())
live = doc.nodes[node.id]
assert live.state.code_review_diff_version == 2
assert live.state.code_review_verdict == "none"
Expand All @@ -122,13 +122,13 @@ def test_store_diff_lands_fields_bumps_version_and_resets_review():

def test_fetch_diff_text_is_wrong_kind_guarded():
doc, node = _doc_with_review()
doc.store_code_review_diff(node.id, pr_url="u", **{k: v for k, v in _bundle().items()})
doc.store_code_review_diff(node.id, pr_url="u", bundle=_bundle())
assert doc.fetch_code_review_diff_text(node.id) == "diff --git x"


def test_complete_run_caps_and_resets_dismissals():
doc, node = _doc_with_review()
doc.store_code_review_diff(node.id, pr_url="u", **{k: v for k, v in _bundle().items()})
doc.store_code_review_diff(node.id, pr_url="u", bundle=_bundle())
doc.complete_code_review_run(
node.id, title="T", overview="O", confidence="high",
walkthrough=[{"group_title": f"g{i}", "paths": ["x"], "explanation": "e"} for i in range(20)],
Expand All @@ -147,7 +147,7 @@ def test_complete_run_caps_and_resets_dismissals():
def test_fail_run_is_silent_for_missing_nodes_and_keeps_prior_review():
doc, node = _doc_with_review()
assert doc.fail_code_review_run("missing", "boom") is None
doc.store_code_review_diff(node.id, pr_url="u", **{k: v for k, v in _bundle().items()})
doc.store_code_review_diff(node.id, pr_url="u", bundle=_bundle())
doc.complete_code_review_run(
node.id, title="T", overview="O", confidence="high", walkthrough=[],
findings=[], errors=[], scores={}, quality_score=80,
Expand All @@ -161,7 +161,7 @@ def test_fail_run_is_silent_for_missing_nodes_and_keeps_prior_review():

def test_dismiss_finding_is_idempotent_and_quiet_on_unknown_ids():
doc, node = _doc_with_review()
doc.store_code_review_diff(node.id, pr_url="u", **{k: v for k, v in _bundle().items()})
doc.store_code_review_diff(node.id, pr_url="u", bundle=_bundle())
doc.complete_code_review_run(
node.id, title="T", overview="O", confidence="high", walkthrough=[],
findings=[{"id": "f1"}], errors=[{"id": "e1"}], scores={},
Expand All @@ -188,7 +188,7 @@ def test_append_qa_caps_at_twenty_entries():

def test_wire_row_carries_review_fields_but_not_the_diff_text():
doc, node = _doc_with_review()
doc.store_code_review_diff(node.id, pr_url="u", **{k: v for k, v in _bundle().items()})
doc.store_code_review_diff(node.id, pr_url="u", bundle=_bundle())
doc.complete_code_review_run(
node.id, title="T", overview="O", confidence="high",
walkthrough=[], findings=[], errors=[], scores={"correctness": 80},
Expand All @@ -208,8 +208,7 @@ def test_wire_row_carries_review_fields_but_not_the_diff_text():

def test_save_load_round_trip_preserves_review_state():
doc, node = _doc_with_review()
doc.store_code_review_diff(node.id, pr_url="https://github.com/o/r/pull/3",
**{k: v for k, v in _bundle().items()})
doc.store_code_review_diff(node.id, pr_url="https://github.com/o/r/pull/3", bundle=_bundle())
doc.complete_code_review_run(
node.id, title="T", overview="O", confidence="high",
walkthrough=[{"group_title": "G", "paths": ["x.py"], "explanation": "E"}],
Expand Down Expand Up @@ -328,7 +327,7 @@ def test_dispatch_ask_returns_answer_text(monkeypatch):
)
dispatcher = _dispatcher()
doc, node = _doc_with_review()
doc.store_code_review_diff(node.id, pr_url="u", **{k: v for k, v in _bundle().items()})
doc.store_code_review_diff(node.id, pr_url="u", bundle=_bundle())

async def run():
return await dispatcher.ask_code_review_question(
Expand Down Expand Up @@ -423,7 +422,7 @@ async def run():

def test_intent_ask_appends_qa():
doc, node = _doc_with_review()
doc.store_code_review_diff(node.id, pr_url="u", **{k: v for k, v in _bundle().items()})
doc.store_code_review_diff(node.id, pr_url="u", bundle=_bundle())
dispatcher = _StubDispatcher()
bus, _notifications = _intent_bus(doc, dispatcher)

Expand All @@ -436,7 +435,7 @@ async def run():

def test_intent_dismiss_is_undoable():
doc, node = _doc_with_review()
doc.store_code_review_diff(node.id, pr_url="u", **{k: v for k, v in _bundle().items()})
doc.store_code_review_diff(node.id, pr_url="u", bundle=_bundle())
doc.complete_code_review_run(
node.id, title="T", overview="O", confidence="high", walkthrough=[],
findings=[{"id": "f1"}], errors=[], scores={}, quality_score=80,
Expand Down
Loading