From bff4a70d2436aac8bddc71c759635d22f5717ce3 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Mon, 7 Sep 2026 03:44:36 +0000 Subject: [PATCH] tighten local-to-premium handoff: packet, rewrite re-score, secrets The apply gate now re-scores premium rewrites on the same oracles, passes layer statuses and local_review notes to the reviewer, and refuses to delegate .env or credential files. Still not a classifier and still not this Cloud Agent calling local_* as its own tools. Co-authored-by: jmjava --- docs/evaluation-protocol.md | 8 +- src/local_coding_slm/eval/harness.py | 3 +- src/local_coding_slm/eval/orchestrate.py | 70 ++++++++++++++--- src/local_coding_slm/eval/review.py | 24 +++++- src/local_coding_slm/eval/routing.py | 28 ++++++- tests/test_eval_orchestrate.py | 95 +++++++++++++++++++++++- 6 files changed, 207 insertions(+), 21 deletions(-) diff --git a/docs/evaluation-protocol.md b/docs/evaluation-protocol.md index c3d3add..8b883b3 100644 --- a/docs/evaluation-protocol.md +++ b/docs/evaluation-protocol.md @@ -135,8 +135,11 @@ and `orchestrate.py`: 3. **Apply gate** requires a premium verdict (`accept` / `rewrite` / `reject`). A local layer-pass is not approval. `local_review` is a cheap SLM tool and cannot approve. Reject drops the patch. Rewrite - applies the premium text, not the raw local output. Accept applies - local text only when the four layers already passed. + applies the premium text, not the raw local output, **and is scored + on the same four layers** before apply. Accept applies local text + only when the four layers already passed. `.env` / credential files + are never delegated. The reviewer sees a `ReviewPacket` (text, layer + statuses, optional `local_review` notes), not only the raw MCP string. CI uses a scripted reviewer. These tests do **not** call Cursor, GPT, or Claude, and they do not prove that a live IDE agent followed the rule @@ -192,6 +195,7 @@ After repeated live runs, a paper may claim: executed tests (`test_add_execute`; A6 now uses this checker). - That keep-vs-delegate and accept/rewrite/reject are enforceable as a state machine on stub workers plus real stdio MCP. +- That a premium rewrite which fails the same oracles is not applied. It still may not claim: diff --git a/src/local_coding_slm/eval/harness.py b/src/local_coding_slm/eval/harness.py index 21072bf..7e0b4bb 100644 --- a/src/local_coding_slm/eval/harness.py +++ b/src/local_coding_slm/eval/harness.py @@ -165,7 +165,8 @@ async def _run_orchestrated_job( ) -> JobResult: from local_coding_slm.eval.routing import route - decision = route(job.signals) + files = job.eval_case.files if job.eval_case is not None else () + decision = route(job.signals, files) if decision.action == "keep": return run_job(job) if job.eval_case is None: diff --git a/src/local_coding_slm/eval/orchestrate.py b/src/local_coding_slm/eval/orchestrate.py index d623863..d023f88 100644 --- a/src/local_coding_slm/eval/orchestrate.py +++ b/src/local_coding_slm/eval/orchestrate.py @@ -17,6 +17,7 @@ from local_coding_slm.eval.record import AttemptRecord from local_coding_slm.eval.review import ( LOCAL_REVIEW_TOOL, + ReviewPacket, ReviewVerdict, ScriptedReviewer, is_premium_review, @@ -25,7 +26,7 @@ from local_coding_slm.eval.score import EvalCase, EvalResult, score_candidate GenerateFn = Callable[[AttemptPlan], str] -ReviewFn = Callable[[str, str, bool], ReviewVerdict | None] +ReviewFn = Callable[..., ReviewVerdict | None] @dataclass(frozen=True) @@ -56,6 +57,7 @@ class OrchestrationJob: local_replies: tuple[str, ...] = () review: ReviewVerdict | None = None premium_keep_text: str = "" + local_review_notes: str = "" @dataclass(frozen=True) @@ -97,8 +99,9 @@ def decide_apply( last: LocalAttempt | None, verdict: ReviewVerdict | None, keep_text: str = "", + eval_case: EvalCase | None = None, ) -> ApplyDecision: - """Apply gate. Local layer-pass is not approval.""" + """Apply gate. Local layer-pass is not approval. Rewrites are re-scored.""" if not delegated: return ApplyDecision( outcome="kept_on_premium", @@ -134,6 +137,18 @@ def decide_apply( text=None, blocked="empty_rewrite", ) + if eval_case is not None: + scored = score_candidate(verdict.text, eval_case) + if not scored.passed: + fail = scored.first_failure + layer = fail.name if fail is not None else "behavior" + return ApplyDecision( + outcome="blocked", + applied=False, + source=None, + text=None, + blocked=f"rewrite_unproven:{layer}", + ) return ApplyDecision( outcome="applied_rewrite", applied=True, @@ -198,11 +213,17 @@ def finish_delegated_job( verdict: ReviewVerdict | None, ) -> JobResult: """Apply gate after local attempts (scripted or MCP) already ran.""" - decision = route(job.signals) + files = job.eval_case.files if job.eval_case is not None else () + decision = route(job.signals, files) if decision.action != "delegate": raise ValueError(f"{job.id}: finish_delegated_job requires a delegated route") last = attempts[-1] if attempts else None - apply = decide_apply(delegated=True, last=last, verdict=verdict) + apply = decide_apply( + delegated=True, + last=last, + verdict=verdict, + eval_case=job.eval_case, + ) return _result(job, decision, apply, attempts, verdict) @@ -212,7 +233,8 @@ def run_job( generate: GenerateFn | None = None, reviewer: ReviewFn | ScriptedReviewer | None = None, ) -> JobResult: - decision: RouteDecision = route(job.signals) + files = job.eval_case.files if job.eval_case is not None else () + decision: RouteDecision = route(job.signals, files) if decision.action == "keep": apply = decide_apply( delegated=False, @@ -232,24 +254,48 @@ def run_job( raise ValueError(f"{job.id}: delegated jobs need an eval_case") local = generate if generate is not None else ScriptedLocal(job.local_replies) - review_fn: ReviewFn + scripted: ScriptedReviewer | None = None if reviewer is None: scripted = ScriptedReviewer(job.review) - review_fn = scripted.review + review_fn: ReviewFn = scripted.review_packet elif isinstance(reviewer, ScriptedReviewer): - review_fn = reviewer.review + review_fn = reviewer.review_packet else: review_fn = reviewer attempts = run_local_loop(job.eval_case, local, job=job.id) last = attempts[-1] if attempts else None - local_text = last.text if last else "" - local_passed = last.passed if last else False - verdict = review_fn(job.id, local_text, local_passed) - apply = decide_apply(delegated=True, last=last, verdict=verdict) + packet = _packet_for(job, last) + verdict = _invoke_review(review_fn, packet) + apply = decide_apply( + delegated=True, + last=last, + verdict=verdict, + eval_case=job.eval_case, + ) return _result(job, decision, apply, attempts, verdict) +def _packet_for(job: OrchestrationJob, last: LocalAttempt | None) -> ReviewPacket: + layers = dict(last.record.layers) if last is not None else {} + return ReviewPacket( + job_id=job.id, + local_text=last.text if last is not None else "", + passed=last.passed if last is not None else False, + layers=layers, + first_failure=None if last is None else last.record.first_failure, + local_review_notes=job.local_review_notes, + case_id=job.eval_case.id if job.eval_case is not None else job.id, + ) + + +def _invoke_review(review_fn: ReviewFn, packet: ReviewPacket) -> ReviewVerdict | None: + try: + return review_fn(packet) + except TypeError: + return review_fn(packet.job_id, packet.local_text, packet.passed) + + def _result( job: OrchestrationJob, decision: RouteDecision, diff --git a/src/local_coding_slm/eval/review.py b/src/local_coding_slm/eval/review.py index 8488d1c..735bc3f 100644 --- a/src/local_coding_slm/eval/review.py +++ b/src/local_coding_slm/eval/review.py @@ -4,17 +4,34 @@ Apply still requires a verdict from the premium orchestrator (accept / rewrite / reject). CI uses a scripted stand-in so the contract is testable without calling Cursor, GPT, or Claude. + +The handoff into review is a ``ReviewPacket``: local text, layer scores, +and optional local_review notes. The premium model is supposed to read +that packet, not only the raw tool string. """ from __future__ import annotations -from dataclasses import dataclass +from dataclasses import dataclass, field PREMIUM_REVIEWER = "premium" LOCAL_REVIEW_TOOL = "local_review" DECISIONS = frozenset({"accept", "rewrite", "reject"}) +@dataclass(frozen=True) +class ReviewPacket: + """What the premium reviewer sees after local_* returns.""" + + job_id: str + local_text: str + passed: bool + layers: dict[str, str] = field(default_factory=dict) + first_failure: str | None = None + local_review_notes: str = "" + case_id: str = "" + + @dataclass(frozen=True) class ReviewVerdict: decision: str @@ -49,8 +66,13 @@ class ScriptedReviewer: def __init__(self, verdict: ReviewVerdict | None) -> None: self.verdict = verdict self.calls = 0 + self.last_packet: ReviewPacket | None = None def review(self, job_id: str, local_text: str, passed: bool) -> ReviewVerdict | None: del job_id, local_text, passed self.calls += 1 return self.verdict + + def review_packet(self, packet: ReviewPacket) -> ReviewVerdict | None: + self.last_packet = packet + return self.review(packet.job_id, packet.local_text, packet.passed) diff --git a/src/local_coding_slm/eval/routing.py b/src/local_coding_slm/eval/routing.py index a597036..5053f6c 100644 --- a/src/local_coding_slm/eval/routing.py +++ b/src/local_coding_slm/eval/routing.py @@ -3,11 +3,13 @@ The premium agent still chooses the signals. This module is the contract those signals must satisfy: mechanical work may be delegated; incident, architectural, and live-tool work stays on the premium model. Delegated -work always requires a later premium review before apply. +work always requires a later premium review before apply. Secret files +never go to local_* even when the task looks mechanical. """ from __future__ import annotations +from collections.abc import Sequence from dataclasses import dataclass @@ -42,8 +44,28 @@ def mechanical_signals(**overrides: bool) -> RouteSignals: return RouteSignals(**values) -def route(signals: RouteSignals) -> RouteDecision: - """Return keep vs delegate. Do-not-delegate flags win over mechanical ones.""" +def payload_block_reason(files: Sequence[dict[str, str]] | None) -> str | None: + """Refuse to send secrets or credential files to local_*.""" + for item in files or (): + path = (item.get("path") or "").replace("\\", "/").lower() + name = path.rsplit("/", 1)[-1] + if name == ".env.example": + continue + if name == ".env" or name.startswith(".env."): + return "secrets_file" + if name in {"credentials.json", "id_rsa", "id_rsa.pub"}: + return "secrets_file" + return None + + +def route( + signals: RouteSignals, + files: Sequence[dict[str, str]] | None = None, +) -> RouteDecision: + """Return keep vs delegate. Do-not-delegate flags and secret files win.""" + blocked = payload_block_reason(files) + if blocked: + return RouteDecision("keep", blocked, False) if signals.incident_debug: return RouteDecision("keep", "incident_debug", False) if signals.architectural: diff --git a/tests/test_eval_orchestrate.py b/tests/test_eval_orchestrate.py index c5f8e8b..c028340 100644 --- a/tests/test_eval_orchestrate.py +++ b/tests/test_eval_orchestrate.py @@ -11,6 +11,7 @@ WHITESPACE_GOLDEN, WHITESPACE_NO_FENCE, ) +from local_coding_slm.eval.cases_extended import IMPLEMENT_CLAMP_NO_HI from local_coding_slm.eval.orchestrate import ( ApplyDecision, LocalAttempt, @@ -24,6 +25,7 @@ from local_coding_slm.eval.review import ( LOCAL_REVIEW_TOOL, PREMIUM_REVIEWER, + ReviewPacket, ReviewVerdict, accept, reject, @@ -83,6 +85,21 @@ def test_ambiguous_stays_on_premium(self) -> None: self.assertEqual(decision.action, "keep") self.assertEqual(decision.reason, "not_mechanical") + def test_env_file_is_never_delegated(self) -> None: + decision = route( + mechanical_signals(), + files=({"path": ".env", "content": "OLLAMA_BASE_URL=http://127.0.0.1:11434"},), + ) + self.assertEqual(decision.action, "keep") + self.assertEqual(decision.reason, "secrets_file") + + def test_env_example_may_still_delegate(self) -> None: + decision = route( + mechanical_signals(), + files=({"path": ".env.example", "content": "OLLAMA_BASE_URL="},), + ) + self.assertEqual(decision.action, "delegate") + class ApplyGateTests(unittest.TestCase): def test_keep_does_not_need_review(self) -> None: @@ -158,6 +175,16 @@ def test_accept_applies_local_only_when_layers_passed(self) -> None: self.assertFalse(blocked.applied) self.assertEqual(blocked.blocked, "accept_unproven_local") + def test_broken_rewrite_is_not_applied(self) -> None: + applied = decide_apply( + delegated=True, + last=_attempt(passed=True, text="LOCAL"), + verdict=rewrite(IMPLEMENT_CLAMP_NO_HI), + eval_case=CASES_BY_ID["implement_clamp"], + ) + self.assertFalse(applied.applied) + self.assertTrue((applied.blocked or "").startswith("rewrite_unproven")) + class OrchestratorLoopTests(unittest.TestCase): def test_keep_never_calls_local(self) -> None: @@ -271,14 +298,17 @@ def test_premium_rejects_passing_local(self) -> None: self.assertIsNone(result.applied_text) def test_premium_rewrite_not_raw_local(self) -> None: - rewritten = "```python\n# user_text.py\n# premium rewrite\n```\n" + rewritten = WHITESPACE_GOLDEN.replace( + "return \" \".join(value.strip().split())", + 'return " ".join(value.strip().split()) # premium', + ) result = run_job( OrchestrationJob( id="trim_extract", signals=mechanical_signals(), eval_case=CASES_BY_ID["whitespace_extract"], local_replies=(WHITESPACE_GOLDEN,), - review=rewrite(rewritten, notes="trimmed"), + review=rewrite(rewritten, notes="trimmed comment"), ) ) self.assertEqual(result.outcome, "applied_rewrite") @@ -286,6 +316,67 @@ def test_premium_rewrite_not_raw_local(self) -> None: self.assertEqual(result.applied_text, rewritten) self.assertNotEqual(result.applied_text, WHITESPACE_GOLDEN) + def test_broken_premium_rewrite_blocked(self) -> None: + result = run_job( + OrchestrationJob( + id="bad_rewrite", + signals=mechanical_signals(), + eval_case=CASES_BY_ID["whitespace_extract"], + local_replies=(WHITESPACE_GOLDEN,), + review=rewrite("```python\n# user_text.py\n# broken\n```\n"), + ) + ) + self.assertEqual(result.outcome, "blocked") + self.assertFalse(result.applied) + self.assertIn("rewrite_unproven", result.blocked or "") + + def test_reviewer_receives_layer_packet_and_local_review_notes(self) -> None: + seen: list[ReviewPacket] = [] + + def reviewer(packet: ReviewPacket) -> ReviewVerdict: + seen.append(packet) + return accept(notes="packet ok") + + result = run_job( + OrchestrationJob( + id="packet_extract", + signals=mechanical_signals(), + eval_case=CASES_BY_ID["whitespace_extract"], + local_replies=(WHITESPACE_GOLDEN,), + local_review_notes="local_review: no null deref in this helper", + ), + reviewer=reviewer, + ) + self.assertEqual(result.outcome, "applied_local") + self.assertEqual(len(seen), 1) + self.assertTrue(seen[0].passed) + self.assertEqual(seen[0].layers.get("behavior"), "pass") + self.assertIn("null", seen[0].local_review_notes) + + def test_secrets_file_keep_never_calls_local(self) -> None: + def boom(plan: object) -> str: + raise AssertionError(f"must not send .env to local: {plan}") + + secret_case = CASES_BY_ID["whitespace_extract"] + from dataclasses import replace + + tainted = replace( + secret_case, + files=secret_case.files + ({"path": ".env", "content": "K=v"},), + ) + result = run_job( + OrchestrationJob( + id="secret_keep", + signals=mechanical_signals(), + eval_case=tainted, + premium_keep_text="premium handles secrets", + ), + generate=boom, + ) + self.assertFalse(result.delegated) + self.assertEqual(result.route_reason, "secrets_file") + self.assertEqual(result.local_attempts, 0) + def test_missing_review_blocks_even_after_local_pass(self) -> None: result = run_job( OrchestrationJob(