From afb49ab1bd5c33aeb1f04d9f98ed347b5be0fe88 Mon Sep 17 00:00:00 2001 From: dovvnloading <157447210+dovvnloading@users.noreply.github.com> Date: Fri, 4 Sep 2026 11:19:15 -0400 Subject: [PATCH] Collapse two pieces of copy-paste plumbing store_code_review_diff took 18 parameters, the widest signature in the repo. Fifteen of them were pure transport: the intent pulled them out of the fetch bundle with .get() one at a time, only for the method to set them straight onto node.state. It now takes the bundle whole. `pr_url` stays a separate argument because it is genuinely not part of the bundle - it is what the user typed, and it is kept even when the fetch that used it is superseded. The intent's call site drops from 17 lines to one. Widest signature in backend/ is now 15 (run_lifecycle.run_single_shot). The per-field coercion moves with it, into a small _bundle_int: the bundle is built from a GitHub API response, so every numeric field in it is external input. diff_fetch.py already coerces on the way in; this is the second line of the same defence, for a bundle arriving by any other route. Verified: pr_number="twelve", additions="lots", changed_files=-5 and a non-dict entry in files all land as 0/0/0 and a dropped row rather than raising. Separately, start_gitlink_apply had five refusal paths that each repeated the same four lines - clear pending_request_id, release the registry slot, report, publish. Four chances each to forget one, and forgetting either of the first two is not cosmetic: a missed pending_request_id leaves the node permanently busy, a missed release leaks the slot for the session. They share one _abandon() now. Its await sits on the abandon path only, so the atomic check-and-freeze section keeps its zero-await guarantee - the comparisons there still run with no suspension point between them. The two remaining bare cleanups in that file are the task-completion `finally` blocks, which are a different concern and correctly use the guarded `if node.pending_request_id == request_id` form. Test plan: - 12 existing store_code_review_diff call sites updated; they were already unpacking a dict, so they got shorter too. - Full suite: 3212 passed, 19 skipped. ruff and mypy clean. Note for the record: the _bundle_int helper first landed between the @dataclass decorator and SceneDocument, breaking the module on import - the same line-heuristic mistake as the previous change. Caught by the test collection, relocated above the decorator. Co-Authored-By: Claude Opus 5 --- backend/agent_dispatch/gitlink.py | 40 ++++++----- backend/api/intents_code_review.py | 20 +----- backend/domain/graph.py | 82 ++++++++++++----------- backend/tests/test_review_lens_backend.py | 23 +++---- 4 files changed, 78 insertions(+), 87 deletions(-) diff --git a/backend/agent_dispatch/gitlink.py b/backend/agent_dispatch/gitlink.py index 907cb03..aac40f2 100644 --- a/backend/agent_dispatch/gitlink.py +++ b/backend/agent_dispatch/gitlink.py @@ -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 @@ -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. --- @@ -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 @@ -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 diff --git a/backend/api/intents_code_review.py b/backend/api/intents_code_review.py index 5c2f82b..635ce64 100644 --- a/backend/api/intents_code_review.py +++ b/backend/api/intents_code_review.py @@ -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 diff --git a/backend/domain/graph.py b/backend/domain/graph.py index 0d33cfd..23cb124 100644 --- a/backend/domain/graph.py +++ b/backend/domain/graph.py @@ -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 @@ -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, @@ -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 = [] diff --git a/backend/tests/test_review_lens_backend.py b/backend/tests/test_review_lens_backend.py index 910581a..42514cb 100644 --- a/backend/tests/test_review_lens_backend.py +++ b/backend/tests/test_review_lens_backend.py @@ -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 @@ -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" @@ -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)], @@ -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, @@ -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={}, @@ -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}, @@ -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"}], @@ -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( @@ -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) @@ -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,