Skip to content

Commit f9b486a

Browse files
DeepfreezechillBrian KrafftCopilot
authored
Epic 5.4: Extract confirmation logic to evolution/confirmation.py (#42)
* feat(evolution): extract confirmation logic (Epic 5.4) Extract _llm_confirm_evolution and _parse_confirmation from evolver.py to evolution/confirmation.py (~160 lines). - llm_confirm_evolution: LLM-based candidate validation with recording - parse_confirmation: JSON + keyword response parser - _SKILL_CONTENT_MAX_CHARS constant moved to confirmation.py - MRO preserved: calls evolver._format_analysis_context and evolver._parse_confirmation through instance - 28 new tests: JSON/keyword parsing, async flow, error path, MRO seams - 1,498 total pass, 127 skipped Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(5.4): harden recording, preserve logger namespace, add edge tests Fixes ALL findings from /8eyes + /collab review: - SEC: Truncate recorded prompt/response to 2000 chars (data minimization) - SEC: Remove copy.deepcopy — build truncated messages instead - M-1: Logger uses evolver namespace for log filter compatibility - M-2: Add delegation seam tests (real SkillEvolver path) - L-2: Add edge case tests (conflicting keywords, JSON array, _model=None) - L-2: Add recording truncation verification test - L-2: Add logger namespace test 36 confirmation tests pass; 1,506 total pass, 127 skipped. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(5.4): fix parse negation bugs, harden all recording paths Round 2+3 fixes: - BUG FIX: parse_confirmation now handles negation patterns (not/never/don't) 'do not confirm', 'never confirm', 'don't proceed' correctly return False - BUG FIX: JSON string 'false' no longer truthy (strict bool/string check) - BUG FIX: Negative keywords checked FIRST (conservative fail-safe) - SEC: All 4 recording paths in evolver.py now truncate to 2000 chars - SEC: copy.deepcopy removed from all recording paths - OBS: skill_id added to iteration response_metadata for correlation - OBS: Logger namespace preserved (evolver) for filter compatibility 44 confirmation tests; 1,514 total pass, 127 skipped. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(5.4): add cannot/can't negation patterns (R3 finding) GPT-5.4 R3 found 'cannot confirm' and 'can't confirm' bypassed negation. Added \bcannot\b and \bcan'?t\b to negative keyword regex. 3 new tests: cannot_confirm, cant_confirm, cant_proceed_no_apostrophe. 47 confirmation tests; 1,517 total pass, 127 skipped. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Brian Krafft <bkrafft@microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent fc743e1 commit f9b486a

3 files changed

Lines changed: 665 additions & 102 deletions

File tree

‎openspace/skill_engine/evolution/confirmation.py‎

Lines changed: 180 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,180 @@
1+
"""Evolution confirmation — LLM-based candidate validation.
2+
3+
Functions extracted from ``SkillEvolver`` (Epic 5.4):
4+
- ``llm_confirm_evolution`` — Ask LLM to confirm a rule-based candidate
5+
- ``parse_confirmation`` — Parse LLM yes/no response (JSON or keyword)
6+
"""
7+
8+
from __future__ import annotations
9+
10+
import json
11+
import re
12+
from typing import TYPE_CHECKING, List
13+
14+
from openspace.prompts import SkillEnginePrompts
15+
from openspace.skill_engine.skill_utils import truncate
16+
from openspace.utils.logging import Logger
17+
18+
if TYPE_CHECKING:
19+
from openspace.skill_engine.types import (
20+
EvolutionType,
21+
ExecutionAnalysis,
22+
SkillRecord,
23+
)
24+
25+
# Preserve evolver's logger namespace so existing log filters/alerts
26+
# continue to capture confirmation warnings without reconfiguration.
27+
logger = Logger.get_logger("openspace.skill_engine.evolver")
28+
29+
# ---------------------------------------------------------------------------
30+
# Constants
31+
# ---------------------------------------------------------------------------
32+
33+
_SKILL_CONTENT_MAX_CHARS = 12_000 # Max chars of SKILL.md in evolution prompt
34+
_RECORDING_MAX_CHARS = 2_000 # Cap recorded prompt content for data minimization
35+
36+
37+
# ---------------------------------------------------------------------------
38+
# LLM confirmation
39+
# ---------------------------------------------------------------------------
40+
41+
async def llm_confirm_evolution(
42+
evolver,
43+
*,
44+
skill_record: "SkillRecord",
45+
skill_content: str,
46+
proposed_type: "EvolutionType",
47+
proposed_direction: str,
48+
trigger_context: str,
49+
recent_analyses: List["ExecutionAnalysis"],
50+
) -> bool:
51+
"""Ask LLM to confirm whether a rule-based evolution candidate
52+
truly needs evolution.
53+
54+
Returns True if LLM agrees, False otherwise.
55+
This prevents false positives from rigid threshold-based rules.
56+
57+
The confirmation prompt and response are recorded (truncated) to
58+
``conversations.jsonl`` under agent_name="SkillEvolver.confirm".
59+
"""
60+
from openspace.recording import RecordingManager
61+
62+
analysis_ctx = evolver._format_analysis_context(recent_analyses)
63+
64+
prompt = SkillEnginePrompts.evolution_confirm(
65+
skill_id=skill_record.skill_id,
66+
skill_content=truncate(skill_content, _SKILL_CONTENT_MAX_CHARS // 2),
67+
proposed_type=proposed_type.value,
68+
proposed_direction=proposed_direction,
69+
trigger_context=trigger_context,
70+
recent_analyses=analysis_ctx,
71+
)
72+
73+
confirm_messages = [{"role": "user", "content": prompt}]
74+
75+
# Record truncated confirmation setup — full prompt goes to LLM only
76+
recorded_messages = [
77+
{"role": m["role"], "content": truncate(m["content"], _RECORDING_MAX_CHARS)}
78+
for m in confirm_messages
79+
]
80+
await RecordingManager.record_conversation_setup(
81+
setup_messages=recorded_messages,
82+
agent_name="SkillEvolver.confirm",
83+
extra={
84+
"skill_id": skill_record.skill_id,
85+
"proposed_type": proposed_type.value,
86+
"trigger_context": trigger_context[:200],
87+
},
88+
)
89+
90+
model = evolver._model or evolver._llm_client.model
91+
try:
92+
result = await evolver._llm_client.complete(
93+
messages=confirm_messages,
94+
model=model,
95+
)
96+
content = result["message"].get("content", "").strip().lower()
97+
confirmed = evolver._parse_confirmation(content)
98+
99+
# Record truncated confirmation response (include skill_id for correlation)
100+
await RecordingManager.record_iteration_context(
101+
iteration=1,
102+
delta_messages=[
103+
{"role": "assistant", "content": truncate(content, _RECORDING_MAX_CHARS)},
104+
],
105+
response_metadata={
106+
"has_tool_calls": False,
107+
"confirmed": confirmed,
108+
"skill_id": skill_record.skill_id,
109+
},
110+
agent_name="SkillEvolver.confirm",
111+
)
112+
113+
return confirmed
114+
except Exception as e:
115+
logger.warning("LLM confirmation failed, defaulting to skip: %s", e)
116+
return False
117+
118+
119+
# ---------------------------------------------------------------------------
120+
# Response parsing
121+
# ---------------------------------------------------------------------------
122+
123+
def parse_confirmation(response: str) -> bool:
124+
"""Parse LLM confirmation response (expects JSON with 'proceed' field).
125+
126+
Parsing order:
127+
1. JSON ``{"proceed": true/false}`` (with markdown-fence stripping)
128+
2. Keyword matching — negatives checked FIRST (conservative: err toward skip)
129+
3. Default: False (ambiguous → skip costly evolution)
130+
"""
131+
# Try JSON parse first
132+
try:
133+
cleaned = response.strip()
134+
if cleaned.startswith("```"):
135+
cleaned = re.sub(r"^```(?:json)?\s*\n?", "", cleaned)
136+
cleaned = re.sub(r"\n?```\s*$", "", cleaned)
137+
data = json.loads(cleaned)
138+
if isinstance(data, dict):
139+
proceed = data.get("proceed", False)
140+
# Strict boolean check — "false" (string) must not be truthy
141+
if isinstance(proceed, bool):
142+
return proceed
143+
if isinstance(proceed, (int, float)):
144+
return bool(proceed)
145+
# String values: explicit true/false
146+
return str(proceed).lower() in ("true", "1", "yes")
147+
except (json.JSONDecodeError, ValueError):
148+
pass
149+
150+
# Fallback: keyword matching.
151+
# Negatives checked FIRST — conservative (err toward skipping).
152+
# yes/no use strict word boundaries to avoid false positives
153+
# (e.g. "know" matching "no").
154+
# confirm/reject/skip use stem-style matching so that common
155+
# LLM variants like "confirmed", "rejected", "skipping" still
156+
# parse correctly.
157+
# Negation words catch "do not confirm", "cannot confirm", "can't proceed".
158+
_wb = re.search # shorthand
159+
if (
160+
any(w in response for w in ('"proceed": false', "proceed: false"))
161+
or _wb(r"\bno\b", response)
162+
or _wb(r"\bnot\b", response)
163+
or _wb(r"\bnever\b", response)
164+
or _wb(r"\bdon'?t\b", response)
165+
or _wb(r"\bcannot\b", response)
166+
or _wb(r"\bcan'?t\b", response)
167+
or _wb(r"\breject\w*\b", response)
168+
or _wb(r"\bskip\w*\b", response)
169+
):
170+
return False
171+
if (
172+
any(w in response for w in ('"proceed": true', "proceed: true"))
173+
or _wb(r"\byes\b", response)
174+
or _wb(r"\bconfirm\w*\b", response)
175+
):
176+
return True
177+
178+
# Default: skip — ambiguous response should not trigger costly evolution
179+
logger.debug("LLM confirmation response was ambiguous, defaulting to skip")
180+
return False

‎openspace/skill_engine/evolver.py‎

Lines changed: 33 additions & 102 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,12 @@
3939
log_background_result as _log_background_result_impl,
4040
schedule_background as _schedule_background_impl,
4141
)
42+
from .evolution.confirmation import (
43+
_RECORDING_MAX_CHARS,
44+
_SKILL_CONTENT_MAX_CHARS,
45+
llm_confirm_evolution as _llm_confirm_evolution_impl,
46+
parse_confirmation as _parse_confirmation_impl,
47+
)
4248
from .evolution.triggers import (
4349
_ANALYSIS_CONTEXT_MAX,
4450
build_context_from_analysis as _build_context_from_analysis_impl,
@@ -99,8 +105,6 @@
99105
EVOLUTION_COMPLETE = SkillEnginePrompts.EVOLUTION_COMPLETE
100106
EVOLUTION_FAILED = SkillEnginePrompts.EVOLUTION_FAILED
101107

102-
_SKILL_CONTENT_MAX_CHARS = 12_000 # Max chars of SKILL.md in evolution prompt
103-
104108
# Agent loop / retry constants
105109
_MAX_EVOLUTION_ITERATIONS = 5 # Max tool-calling rounds for evolution agent
106110
_MAX_EVOLUTION_ATTEMPTS = 3 # Max apply-retry attempts per evolution
@@ -252,103 +256,18 @@ async def _llm_confirm_evolution(
252256
recent_analyses: List[ExecutionAnalysis],
253257
) -> bool:
254258
"""Ask LLM to confirm whether a rule-based evolution candidate
255-
truly needs evolution.
256-
257-
Returns True if LLM agrees, False otherwise.
258-
This prevents false positives from rigid threshold-based rules.
259-
260-
The confirmation prompt and response are recorded to
261-
``conversations.jsonl`` under agent_name="SkillEvolver.confirm".
262-
"""
263-
from openspace.recording import RecordingManager
264-
265-
analysis_ctx = self._format_analysis_context(recent_analyses)
266-
267-
prompt = SkillEnginePrompts.evolution_confirm(
268-
skill_id=skill_record.skill_id,
269-
skill_content=_truncate(skill_content, _SKILL_CONTENT_MAX_CHARS // 2),
270-
proposed_type=proposed_type.value,
259+
truly needs evolution."""
260+
return await _llm_confirm_evolution_impl(
261+
self,
262+
skill_record=skill_record,
263+
skill_content=skill_content,
264+
proposed_type=proposed_type,
271265
proposed_direction=proposed_direction,
272266
trigger_context=trigger_context,
273-
recent_analyses=analysis_ctx,
267+
recent_analyses=recent_analyses,
274268
)
275269

276-
confirm_messages = [{"role": "user", "content": prompt}]
277-
278-
# Record confirmation setup
279-
await RecordingManager.record_conversation_setup(
280-
setup_messages=copy.deepcopy(confirm_messages),
281-
agent_name="SkillEvolver.confirm",
282-
extra={
283-
"skill_id": skill_record.skill_id,
284-
"proposed_type": proposed_type.value,
285-
"trigger_context": trigger_context[:200],
286-
},
287-
)
288-
289-
model = self._model or self._llm_client.model
290-
try:
291-
result = await self._llm_client.complete(
292-
messages=confirm_messages,
293-
model=model,
294-
)
295-
content = result["message"].get("content", "").strip().lower()
296-
confirmed = self._parse_confirmation(content)
297-
298-
# Record confirmation response
299-
await RecordingManager.record_iteration_context(
300-
iteration=1,
301-
delta_messages=[{"role": "assistant", "content": content}],
302-
response_metadata={
303-
"has_tool_calls": False,
304-
"confirmed": confirmed,
305-
},
306-
agent_name="SkillEvolver.confirm",
307-
)
308-
309-
return confirmed
310-
except Exception as e:
311-
logger.warning(f"LLM confirmation failed, defaulting to skip: {e}")
312-
return False
313-
314-
@staticmethod
315-
def _parse_confirmation(response: str) -> bool:
316-
"""Parse LLM confirmation response (expects JSON with 'proceed' field)."""
317-
# Try JSON parse first
318-
try:
319-
# Strip markdown fences
320-
cleaned = response.strip()
321-
if cleaned.startswith("```"):
322-
cleaned = re.sub(r"^```(?:json)?\s*\n?", "", cleaned)
323-
cleaned = re.sub(r"\n?```\s*$", "", cleaned)
324-
data = json.loads(cleaned)
325-
if isinstance(data, dict):
326-
return bool(data.get("proceed", False))
327-
except (json.JSONDecodeError, ValueError):
328-
pass
329-
# Fallback: look for keywords.
330-
# - yes/no use strict word boundaries to avoid false positives
331-
# (e.g. "know" matching "no").
332-
# - confirm/reject/skip use stem-style matching so that common
333-
# LLM variants like "confirmed", "rejected", "skipping" still
334-
# parse correctly.
335-
_wb = re.search # shorthand
336-
if (
337-
any(w in response for w in ('"proceed": true', "proceed: true"))
338-
or _wb(r"\byes\b", response)
339-
or _wb(r"\bconfirm\w*\b", response)
340-
):
341-
return True
342-
if (
343-
any(w in response for w in ('"proceed": false', "proceed: false"))
344-
or _wb(r"\bno\b", response)
345-
or _wb(r"\breject\w*\b", response)
346-
or _wb(r"\bskip\w*\b", response)
347-
):
348-
return False
349-
# Default: skip — ambiguous response should not trigger costly evolution
350-
logger.debug("LLM confirmation response was ambiguous, defaulting to skip")
351-
return False
270+
_parse_confirmation = staticmethod(_parse_confirmation_impl)
352271

353272
async def _evolve_fix(self, ctx: EvolutionContext) -> Optional[SkillRecord]:
354273
"""In-place fix: same name, same directory, new version record.
@@ -722,9 +641,13 @@ async def _run_evolution_loop(
722641
{"role": "user", "content": prompt},
723642
]
724643

725-
# Record initial conversation setup
644+
# Record initial conversation setup (truncated for data minimization)
645+
recorded_setup = [
646+
{"role": m["role"], "content": _truncate(m["content"], _RECORDING_MAX_CHARS)}
647+
for m in messages
648+
]
726649
await RecordingManager.record_conversation_setup(
727-
setup_messages=copy.deepcopy(messages),
650+
setup_messages=recorded_setup,
728651
tools=evolution_tools if evolution_tools else None,
729652
agent_name="SkillEvolver",
730653
extra={
@@ -773,11 +696,15 @@ async def _run_evolution_loop(
773696
updated_messages = result["messages"]
774697
has_tool_calls = result.get("has_tool_calls", False)
775698

776-
# Record iteration delta
699+
# Record iteration delta (truncated for data minimization)
777700
delta = updated_messages[msg_count_before:]
701+
recorded_delta = [
702+
{"role": m["role"], "content": _truncate(m.get("content", ""), _RECORDING_MAX_CHARS)}
703+
for m in delta
704+
]
778705
await RecordingManager.record_iteration_context(
779706
iteration=iteration + 1,
780-
delta_messages=copy.deepcopy(delta),
707+
delta_messages=recorded_delta,
781708
response_metadata={
782709
"has_tool_calls": has_tool_calls,
783710
"tool_calls_count": len(result.get("tool_results", [])),
@@ -948,8 +875,12 @@ async def _apply_with_retry(
948875

949876
# Record retry setup on first retry attempt
950877
if not retry_setup_recorded:
878+
recorded_retry = [
879+
{"role": m["role"], "content": _truncate(m.get("content", ""), _RECORDING_MAX_CHARS)}
880+
for m in msg_history
881+
]
951882
await RecordingManager.record_conversation_setup(
952-
setup_messages=copy.deepcopy(msg_history),
883+
setup_messages=recorded_retry,
953884
agent_name="SkillEvolver.retry",
954885
extra={
955886
"evolution_type": ctx.suggestion.evolution_type.value,
@@ -995,8 +926,8 @@ async def _apply_with_retry(
995926
await RecordingManager.record_iteration_context(
996927
iteration=attempt + 1,
997928
delta_messages=[
998-
{"role": "user", "content": retry_prompt},
999-
{"role": "assistant", "content": new_content},
929+
{"role": "user", "content": _truncate(retry_prompt, _RECORDING_MAX_CHARS)},
930+
{"role": "assistant", "content": _truncate(new_content, _RECORDING_MAX_CHARS)},
1000931
],
1001932
response_metadata={
1002933
"has_tool_calls": False,

0 commit comments

Comments
 (0)