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,