From 78f527b5ef18761234b2a9275b240046b7596cc0 Mon Sep 17 00:00:00 2001 From: Oliver Date: Fri, 1 May 2026 08:54:00 -0700 Subject: [PATCH] fix: apply CodeRabbit follow-ups from #144 + #141 Critical: - cloud/sync.py: fix double /api/v1 prefix on telemetry + corpus paths Major: - cli.py: resolve brain_root once for skill export consistency - skill_export.py: escape backslashes in YAML descriptions - skill_export.py: whitespace-only desc falls back to auto - implicit_feedback.py: negative signals win over approval on conflict - inject_brain_rules.py: harden MAX_RULES int parse against malformed env Tests: - update assertions for corrected /telemetry + /corpus paths - add regression coverage for YAML backslash/newline/whitespace - tighten loose assertions in hooks_intelligence --- Gradata/src/gradata/cli.py | 3 +- Gradata/src/gradata/cloud/sync.py | 4 +-- .../src/gradata/enhancements/skill_export.py | 9 ++++-- .../src/gradata/hooks/implicit_feedback.py | 8 ++++++ .../src/gradata/hooks/inject_brain_rules.py | 9 +++++- Gradata/tests/test_cloud_sync.py | 4 +-- Gradata/tests/test_hooks_intelligence.py | 4 +-- Gradata/tests/test_skill_export.py | 28 +++++++++++++++++++ 8 files changed, 59 insertions(+), 10 deletions(-) diff --git a/Gradata/src/gradata/cli.py b/Gradata/src/gradata/cli.py index 43a62eaa..33e4eed6 100644 --- a/Gradata/src/gradata/cli.py +++ b/Gradata/src/gradata/cli.py @@ -1029,12 +1029,13 @@ def cmd_skill_export(args): Produces ``//SKILL.md`` ready to drop into ``.claude/skills/`` or any Skills-aware harness. """ + from gradata import Brain from gradata.enhancements.skill_export import export_skill, write_skill brain_root = _resolve_brain_root(args) lessons_path: Path | None = None try: - brain = _get_brain(args) + brain = Brain(brain_root) lessons_path = brain._find_lessons_path() except Exception: lessons_path = None diff --git a/Gradata/src/gradata/cloud/sync.py b/Gradata/src/gradata/cloud/sync.py index d0886921..63d8a60a 100644 --- a/Gradata/src/gradata/cloud/sync.py +++ b/Gradata/src/gradata/cloud/sync.py @@ -223,7 +223,7 @@ def sync_metrics(self, payload: TelemetryPayload) -> bool: # Backend mounts the metrics router under /api/v1 (see # cloud/app/main.py → app.include_router(router, prefix="/api/v1") # and cloud/app/routes/metrics.py → @router.post("/telemetry/metrics")). - result = self._post("/api/v1/telemetry/metrics", asdict(payload)) + result = self._post("/telemetry/metrics", asdict(payload)) if result is not None: self.config.last_sync_at = payload.sent_at save_config(self.brain_dir, self.config) @@ -241,7 +241,7 @@ def contribute_corpus(self, anonymized_patterns: list[dict]) -> bool: return False # Backend mounts the corpus router under /api/v1 (same prefix as # telemetry — see cloud/app/main.py). - result = self._post("/api/v1/corpus/contribute", {"patterns": anonymized_patterns}) + result = self._post("/corpus/contribute", {"patterns": anonymized_patterns}) return result is not None diff --git a/Gradata/src/gradata/enhancements/skill_export.py b/Gradata/src/gradata/enhancements/skill_export.py index 1a03ea19..0d55eab0 100644 --- a/Gradata/src/gradata/enhancements/skill_export.py +++ b/Gradata/src/gradata/enhancements/skill_export.py @@ -99,7 +99,9 @@ def _format_skill_md( lines.append("---") lines.append(f"name: {name}") # Quote the description so colons / hashes inside don't break YAML. - safe_desc = description.replace('"', '\\"') + # Escape backslashes first, then quotes, so YAML stays valid even for + # Windows paths or descriptions containing literal `\\` sequences. + safe_desc = description.replace("\\", "\\\\").replace('"', '\\"') lines.append(f'description: "{safe_desc}"') lines.append("---") lines.append("") @@ -196,7 +198,10 @@ def export_skill( rules = _parse_rules(Path(brain_root), lessons_path=lessons_path) rules = _filter_rules(rules, category) metas = _load_meta_principles(Path(brain_root)) if include_meta else [] - desc = description.strip() if description else _auto_description(rules, slug) + if description and description.strip(): + desc = description.strip() + else: + desc = _auto_description(rules, slug) if len(desc) > _DESC_MAX_LEN: desc = desc[: _DESC_MAX_LEN - 3] + "..." return _format_skill_md(slug, desc, rules, metas) diff --git a/Gradata/src/gradata/hooks/implicit_feedback.py b/Gradata/src/gradata/hooks/implicit_feedback.py index 1dda8773..8d8309bd 100644 --- a/Gradata/src/gradata/hooks/implicit_feedback.py +++ b/Gradata/src/gradata/hooks/implicit_feedback.py @@ -162,6 +162,14 @@ def main(data: dict) -> dict | None: signal_types = {s["type"] for s in signals} has_negative = bool(signal_types & _NEGATIVE_SIGNAL_TYPES) has_approval = "approval" in signal_types + # Conflict resolution: when negative and approval coexist on the same + # message, negative wins. The user is critiquing — the polite "thanks" + # doesn't override "but this is wrong". Drop approval from emitted + # signals and suppress OUTPUT_ACCEPTED for this message. + if has_negative and has_approval: + signals = [s for s in signals if s["type"] != "approval"] + signal_types = {s["type"] for s in signals} + has_approval = False # Tacit acceptance: substantive follow-up with no negative signals. The # brain.correct() pipeline logs ~20x more CORRECTION than OUTPUT_ACCEPTED # because users rarely type "looks good" — silence is approval. diff --git a/Gradata/src/gradata/hooks/inject_brain_rules.py b/Gradata/src/gradata/hooks/inject_brain_rules.py index 88d499b0..d81ab75e 100644 --- a/Gradata/src/gradata/hooks/inject_brain_rules.py +++ b/Gradata/src/gradata/hooks/inject_brain_rules.py @@ -182,7 +182,14 @@ def _read_brain_prompt(brain_dir: Path) -> str | None: count=1, ) # Limit to first GRADATA_WISDOM_MAX_RULES non-negotiable rules. - wisdom_max_rules = int(os.environ.get("GRADATA_WISDOM_MAX_RULES", "9")) + _raw_max = os.environ.get("GRADATA_WISDOM_MAX_RULES", "9") + try: + wisdom_max_rules = int(_raw_max) + except (ValueError, TypeError): + _log.warning( + "GRADATA_WISDOM_MAX_RULES=%r not an int — defaulting to 9", _raw_max + ) + wisdom_max_rules = 9 if wisdom_max_rules > 0: rule_lines = [ln for ln in text.split("\n") if ln.startswith("- ")] if len(rule_lines) > wisdom_max_rules: diff --git a/Gradata/tests/test_cloud_sync.py b/Gradata/tests/test_cloud_sync.py index 80a31613..e82233fc 100644 --- a/Gradata/tests/test_cloud_sync.py +++ b/Gradata/tests/test_cloud_sync.py @@ -129,7 +129,7 @@ def test_sync_metrics_posts_when_enabled(self, tmp_path: Path): assert result is True mock_post.assert_called_once() call_path = mock_post.call_args[0][0] - assert call_path == "/api/v1/telemetry/metrics" + assert call_path == "/telemetry/metrics" def test_sync_metrics_updates_last_sync_at_on_success(self, tmp_path: Path): cfg = CloudConfig(sync_enabled=True, token="abc") @@ -168,7 +168,7 @@ def test_contribute_corpus_posts_when_both_flags_set(self, tmp_path: Path): assert result is True mock_post.assert_called_once() - assert mock_post.call_args[0][0] == "/api/v1/corpus/contribute" + assert mock_post.call_args[0][0] == "/corpus/contribute" class TestConvenienceSync: diff --git a/Gradata/tests/test_hooks_intelligence.py b/Gradata/tests/test_hooks_intelligence.py index ce3f806a..e94abb47 100644 --- a/Gradata/tests/test_hooks_intelligence.py +++ b/Gradata/tests/test_hooks_intelligence.py @@ -465,7 +465,7 @@ def test_implicit_feedback_detects_challenge(tmp_path, monkeypatch): monkeypatch.setenv("GRADATA_BRAIN_DIR", str(tmp_path)) with patch("gradata.hooks.implicit_feedback.emit_hook_event") as mock_emit: result = feedback_main({"message": "Are you sure that's correct? It doesn't look right."}) - assert result is not None and "chal" in result["result"] + assert result == {"result": "[fb:chal]"} event_types = [call.args[0] for call in mock_emit.call_args_list] assert "IMPLICIT_FEEDBACK" in event_types signals = mock_emit.call_args_list[0].args[2]["signals"] @@ -483,7 +483,7 @@ def test_implicit_feedback_emits_event(tmp_path): patch("gradata.hooks.implicit_feedback.emit_hook_event") as mock_emit, ): result = feedback_main({"message": "I told you not to do that, are you sure?"}) - assert result is not None and result["result"].startswith("[fb:") + assert result == {"result": "[fb:rem,chal]"} event_types = [call.args[0] for call in mock_emit.call_args_list] assert "IMPLICIT_FEEDBACK" in event_types diff --git a/Gradata/tests/test_skill_export.py b/Gradata/tests/test_skill_export.py index 1131736c..e75bf6c1 100644 --- a/Gradata/tests/test_skill_export.py +++ b/Gradata/tests/test_skill_export.py @@ -123,6 +123,34 @@ def test_double_quotes_in_description_are_escaped(self, tmp_path: Path) -> None: # Ensure the quote is backslash-escaped so YAML stays valid assert r'description: "He said \"hi\" loudly"' in text + def test_backslash_in_description_is_escaped(self, tmp_path: Path) -> None: + # Windows path with literal backslashes must produce an escaped YAML string. + text = export_skill(tmp_path, name="demo", description=r"C:\Users\foo") + assert r'description: "C:\\Users\\foo"' in text + + def test_backslash_and_quote_combined(self, tmp_path: Path) -> None: + # Backslashes must be escaped BEFORE quotes so we don't accidentally + # produce \\" sequences that break YAML. + text = export_skill(tmp_path, name="demo", description=r'path C:\a "b"') + assert r'description: "path C:\\a \"b\""' in text + + def test_multiline_literal_newline_in_description(self, tmp_path: Path) -> None: + # A literal `\n` (two chars: backslash + n) must round-trip as `\\n`. + text = export_skill(tmp_path, name="demo", description=r"line1\nline2") + assert r'description: "line1\\nline2"' in text + + def test_whitespace_only_description_falls_back_to_auto( + self, tmp_path: Path + ) -> None: + _write_lessons(tmp_path, SAMPLE_LESSONS) + text = export_skill(tmp_path, name="demo", description=" \t\n ") + # Auto description must not be empty and must not be just whitespace. + import re + + m = re.search(r'^description: "(.*)"$', text, flags=re.MULTILINE) + assert m is not None, "description line missing" + assert m.group(1).strip() != "", "whitespace-only desc should fall back to auto" + def test_category_filter_narrows_output(self, tmp_path: Path) -> None: _write_lessons(tmp_path, SAMPLE_LESSONS) text = export_skill(tmp_path, name="demo", category="DRAFTING")