Harden skill quality reporter tests

.gitignore

@@ -22,10 +22,6 @@ Desktop.ini
 # Development-only repository folders
 docs/
 scripts/
-tests/*
-!tests/
-!tests/test_skill_quality_reporter.py
-!tests/test_analyzer_skill_quality_reporting.py
 
 # Local agent/project memory
 OPENSPACE.md

openspace/cloud/redaction.py

@@ -14,6 +14,7 @@ import re
 from typing import Any
 import json
 import hashlib
+from datetime import datetime
 from pathlib import Path
 from urllib.parse import urlsplit, urlunsplit
 
@@ -62,6 +63,10 @@ _URL_RE = re.compile(r"https?://[^\s\"'<>]+", re.IGNORECASE)
 _EMAIL_RE = re.compile(r"\b[A-Z0-9._%+-]+@[A-Z0-9.-]+\.[A-Z]{2,}\b", re.IGNORECASE)
 _PHONE_RE = re.compile(r"(?<!\d)(?:\+?\d[\d .()-]{7,}\d)(?!\d)")
 _TRACEBACK_RE = re.compile(r'File "([^"]+)", line (\d+), in ([^\n]+)')
+_ISO_TIMESTAMP_RE = re.compile(
+    r"\d{4}-\d{2}-\d{2}[Tt ]\d{2}:\d{2}:\d{2}"
+    r"(?:\.\d{1,6})?(?:Z|[+-]\d{2}:\d{2})?"
+)
 
 _TELEMETRY_ALLOWED_KEYS = {
     "task_id",
@@ -373,12 +378,24 @@ def _redact_upload_value(
             for item in value
         )
     if isinstance(value, str):
+        if key == "occurred_at" and _is_iso_timestamp_like(value):
+            return value
         if key.endswith("_path") or key in {"local_path", "workspace_ref", "path"}:
             return sanitize_upload_path(value, workspace_root=workspace_root)
         return redact_upload_text(value, workspace_root=workspace_root)
     return redact_cloud_payload(value)
 
 
+def _is_iso_timestamp_like(value: str) -> bool:
+    if not _ISO_TIMESTAMP_RE.fullmatch(value):
+        return False
+    try:
+        datetime.fromisoformat(value.replace("Z", "+00:00"))
+    except ValueError:
+        return False
+    return True
+
+
 def _secret_field_findings(value: Any) -> bool:
     if isinstance(value, dict):
         for key, item in value.items():

openspace/cloud/skill_quality_reporter.py

@@ -81,11 +81,12 @@ class CloudSkillQualityReporter:
         *,
         session_id: str | None = None,
     ) -> dict[str, Any]:
+        if not load_cloud_skill_quality_reporting_enabled():
+            return {"status": "skipped", "reason": "skill_quality_reporting_disabled"}
+
         cfg = load_cloud_config()
         if not cfg.enabled or cfg.telemetry_mode != "outbox" or not cfg.api_key:
             return {"status": "skipped", "reason": "cloud_telemetry_disabled"}
-        if not load_cloud_skill_quality_reporting_enabled():
-            return {"status": "skipped", "reason": "skill_quality_reporting_disabled"}
 
         client = self._client or OpenSpaceClient(cfg, mapping_store=self._mapping_store)
         mapping_store = self._mapping_store or client._local_mapping_store()

tests/test_analyzer_skill_quality_reporting.py

@@ -284,6 +284,52 @@ def test_analyze_packet_reports_after_record_analysis(monkeypatch):
     assert events == ["record", ("report", "packet-task", "scope-session")]
 
 
+def test_analyze_packet_reports_selected_ref_metadata_session_id(monkeypatch):
+    store = FakeStore()
+    packet = _packet(
+        selected_refs={
+            "tool_result": [
+                ResourceRef(
+                    "tool-1",
+                    "tool_result",
+                    metadata={"session_id": "metadata-session"},
+                )
+            ]
+        }
+    )
+    analysis = _analysis("packet-task")
+    events = store.events
+    monkeypatch.setattr(
+        analyzer_module,
+        "_make_skill_quality_reporter",
+        lambda: FakeReporter(events),
+    )
+    analyzer = _packet_analyzer(store, analysis, packet)
+
+    result = _run(analyzer.analyze_packet(packet))
+
+    assert result is analysis
+    assert events == ["record", ("report", "packet-task", "metadata-session")]
+
+
+def test_analyze_packet_reports_with_missing_session_id(monkeypatch):
+    store = FakeStore()
+    packet = _packet()
+    analysis = _analysis("packet-task")
+    events = store.events
+    monkeypatch.setattr(
+        analyzer_module,
+        "_make_skill_quality_reporter",
+        lambda: FakeReporter(events),
+    )
+    analyzer = _packet_analyzer(store, analysis, packet)
+
+    result = _run(analyzer.analyze_packet(packet))
+
+    assert result is analysis
+    assert events == ["record", ("report", "packet-task", None)]
+
+
 def test_session_id_extraction_order_and_missing_values():
     assert (
         analyzer_module._analysis_session_id(

tests/test_skill_quality_reporter.py

@@ -2,8 +2,10 @@ import asyncio
 import json
 from datetime import datetime
 
+import openspace.cloud.skill_quality_reporter as reporter_module
 from openspace.cloud.config import (
     OPENSPACE_CLOUD_API_KEY_ENV,
+    OPENSPACE_CLOUD_BASE_URL_ENV,
     OPENSPACE_CLOUD_MODE_ENV,
     OPENSPACE_CLOUD_SKILL_QUALITY_REPORTING_ENV,
     OPENSPACE_CLOUD_TELEMETRY_MODE_ENV,
@@ -44,6 +46,7 @@ def _set_cloud_env(
     _patch_host_env(monkeypatch)
     for key in (
         OPENSPACE_CLOUD_MODE_ENV,
+        OPENSPACE_CLOUD_BASE_URL_ENV,
         OPENSPACE_CLOUD_TELEMETRY_MODE_ENV,
         OPENSPACE_CLOUD_API_KEY_ENV,
         OPENSPACE_CLOUD_SKILL_QUALITY_REPORTING_ENV,
@@ -93,22 +96,88 @@ def _analysis(
         timestamp=timestamp or datetime(2026, 1, 2, 3, 4, 5, 123456),
         task_completed=task_completed,
         execution_note=(
-            "RAW_EXECUTION_NOTE prompt transcript /tmp/private/file.diff token hash "
+            "RAW_EXECUTION_NOTE prompt messages transcript /tmp/private/file.diff "
-            "redacted_preview"
+            "raw_error token sk-private authorization Bearer secret redacted_preview sha256"
         ),
-        tool_issues=["RAW_TOOL_ISSUE traceback /home/user/project/file.py"],
+        tool_issues=[
+            "RAW_TOOL_ISSUE traceback /home/user/project/file.py API_KEY=secret"
+        ],
         skill_judgments=judgments
         or [
             SkillJudgment(
                 skill_id="local-skill-1",
                 skill_applied=True,
-                note="RAW_SKILL_NOTE prompt diff token",
+                note="RAW_SKILL_NOTE prompt diff token authorization",
             )
         ],
         skill_phase_failed_skill_ids=phase_failed_ids or [],
     )
 
 
+QUALITY_FIELDS = {
+    "quality_event_kind",
+    "quality_schema_version",
+    "denominator",
+    "skill_applied",
+    "task_completed",
+    "skill_phase_failed",
+    "completed",
+    "fallback",
+}
+
+
+def _walk_keys(value):
+    if isinstance(value, dict):
+        for key, item in value.items():
+            yield str(key)
+            yield from _walk_keys(item)
+    elif isinstance(value, list):
+        for item in value:
+            yield from _walk_keys(item)
+
+
+def _assert_private_analysis_text_absent(payload):
+    encoded = json.dumps(payload, ensure_ascii=False, sort_keys=True, default=str).lower()
+    for forbidden in (
+        "raw_skill_note",
+        "raw_execution_note",
+        "raw_tool_issue",
+        "prompt",
+        "messages",
+        "transcript",
+        "file.diff",
+        "/tmp/",
+        "/home/",
+        "traceback",
+        "raw_error",
+        "sk-private",
+        "api_key",
+        "bearer ",
+        "authorization",
+        "redacted_preview",
+        "sha256",
+    ):
+        assert forbidden not in encoded
+    forbidden_keys = {
+        "note",
+        "execution_note",
+        "tool_issues",
+        "prompt",
+        "messages",
+        "transcript",
+        "path",
+        "diff",
+        "raw_error",
+        "raw_diagnostic",
+        "redacted_preview",
+        "sha256",
+        "api_key",
+        "authorization",
+        "token",
+    }
+    assert forbidden_keys.isdisjoint({key.lower() for key in _walk_keys(payload)})
+
+
 def test_gates_default_disabled_and_enabled(monkeypatch, tmp_path):
     cases = [
         {"mode": "off", "telemetry_mode": "outbox", "api_key": "k", "quality": True},
@@ -147,6 +216,36 @@ def test_gates_default_disabled_and_enabled(monkeypatch, tmp_path):
     assert client.calls[0][0] == "skill-use-reported"
 
 
+def test_disabled_quality_gate_skips_before_invalid_cloud_config(monkeypatch, tmp_path):
+    _patch_host_env(monkeypatch)
+    for key in (
+        OPENSPACE_CLOUD_MODE_ENV,
+        OPENSPACE_CLOUD_BASE_URL_ENV,
+        OPENSPACE_CLOUD_TELEMETRY_MODE_ENV,
+        OPENSPACE_CLOUD_API_KEY_ENV,
+        OPENSPACE_CLOUD_SKILL_QUALITY_REPORTING_ENV,
+    ):
+        monkeypatch.delenv(key, raising=False)
+    monkeypatch.setenv(OPENSPACE_CLOUD_MODE_ENV, "invalid-mode")
+    monkeypatch.setenv(OPENSPACE_CLOUD_BASE_URL_ENV, "not-a-service-root")
+    monkeypatch.setenv(OPENSPACE_CLOUD_TELEMETRY_MODE_ENV, "invalid-telemetry")
+    monkeypatch.setenv(OPENSPACE_CLOUD_API_KEY_ENV, "test-key")
+
+    def fail_client_construction(*args, **kwargs):
+        raise AssertionError("OpenSpaceClient should not be constructed")
+
+    monkeypatch.setattr(reporter_module, "OpenSpaceClient", fail_client_construction)
+    reporter = CloudSkillQualityReporter(workspace_root=tmp_path)
+
+    result = _run(reporter.maybe_report_analysis(_analysis()))
+
+    assert result == {
+        "status": "skipped",
+        "reason": "skill_quality_reporting_disabled",
+    }
+    assert not list(tmp_path.rglob("*.db"))
+
+
 def test_local_only_and_missing_cloud_binding_skipped(monkeypatch, tmp_path):
     _set_cloud_env(monkeypatch, quality=True)
     client = FakeClient()
@@ -195,17 +294,41 @@ def test_request_id_exact_and_independent_of_mutable_fields():
         "cloud-skill-1",
     )
 
-    failed_payload = build_skill_quality_judgment_payload(
+    variants = [
-        _analysis(
+        build_skill_quality_judgment_payload(
-            timestamp=datetime(2026, 1, 3, 0, 0, 0),
+            _analysis(
-            task_completed=False,
+                timestamp=datetime(2026, 1, 3, 0, 0, 0),
-            judgments=[SkillJudgment("local-skill-1", False, "ignored")],
+                judgments=[judgment],
+            ),
+            judgment,
+            cloud_skill_id="cloud-skill-1",
+            session_id="session-b",
         ),
-        SkillJudgment("local-skill-1", False, "ignored"),
+        build_skill_quality_judgment_payload(
-        cloud_skill_id="cloud-skill-1",
+            _analysis(
-        session_id="session-b",
+                task_completed=False,
-    )
+                judgments=[SkillJudgment("local-skill-1", False, "ignored")],
-    assert failed_payload["request_id"] == payload["request_id"]
+            ),
+            SkillJudgment("local-skill-1", False, "ignored"),
+            cloud_skill_id="cloud-skill-1",
+            session_id="session-c",
+        ),
+        build_skill_quality_judgment_payload(
+            _analysis(
+                judgments=[judgment],
+                phase_failed_ids=["local-skill-1"],
+            ),
+            judgment,
+            cloud_skill_id="cloud-skill-1",
+        ),
+    ]
+    assert variants[0]["occurred_at"] != payload["occurred_at"]
+    assert variants[1]["status"] == "failed"
+    assert variants[1]["failure_reason"] == "unknown"
+    assert variants[2]["skill_phase_failed"] is True
+    for variant in variants:
+        assert variant["request_id"] == payload["request_id"]
+        assert "duration_ms" not in variant
 
 
 def test_payload_fields_stability_status_and_privacy():
@@ -223,6 +346,19 @@ def test_payload_fields_stability_status_and_privacy():
         cloud_skill_id="cloud-skill-1",
         session_id="session-a",
     )
+    assert set(payload) == {
+        "request_id",
+        "occurred_at",
+        "status",
+        "task_id",
+        "cloud_skill_id",
+        "redaction_level",
+        "redaction_performed_by",
+        "session_id",
+        "local_skill_id",
+        "redaction_policy_version",
+        *QUALITY_FIELDS,
+    }
     assert payload["occurred_at"] == analysis.timestamp.isoformat()
     assert "duration_ms" not in payload
     assert payload["status"] == "success"
@@ -250,20 +386,118 @@ def test_payload_fields_stability_status_and_privacy():
     assert failed["completed"] is False
     assert failed["fallback"] is True
     assert failed["status"] != "partial_success"
+    assert failed["failure_reason"] != "not_applicable"
+    assert QUALITY_FIELDS.issubset(failed)
+    assert not (QUALITY_FIELDS & set((failed.get("extras") or {}).keys()))
 
-    encoded = json.dumps({**payload, **failed}, sort_keys=True)
+    _assert_private_analysis_text_absent(payload)
+    _assert_private_analysis_text_absent(failed)
+
+
+def test_payload_outbox_hash_stable_for_same_persisted_analysis(tmp_path):
+    persisted = ExecutionAnalysis.from_dict(_analysis().to_dict())
+    judgment = persisted.skill_judgments[0]
+    payload = build_skill_quality_judgment_payload(
+        persisted,
+        judgment,
+        cloud_skill_id="cloud-skill-1",
+        session_id="session-a",
+    )
+    rebuilt = build_skill_quality_judgment_payload(
+        persisted,
+        judgment,
+        cloud_skill_id="cloud-skill-1",
+        session_id="session-a",
+    )
+    assert payload == rebuilt
+
+    outbox = CloudTelemetryOutbox(tmp_path / "outbox.db")
+    first = outbox.enqueue(
+        endpoint="/api/v2/telemetry/skill-use-reported",
+        payload=payload,
+    )
+    second = outbox.enqueue(
+        endpoint="/api/v2/telemetry/skill-use-reported",
+        payload=rebuilt,
+    )
+
+    assert first.request_id == second.request_id == payload["request_id"]
+    assert first.payload_hash == second.payload_hash
+    assert first.payload_redacted == second.payload_redacted
+    assert len(outbox.list_pending()) == 1
+    assert first.payload_redacted["occurred_at"] == persisted.timestamp.isoformat()
+    assert "duration_ms" not in first.payload_redacted
+    assert QUALITY_FIELDS.issubset(first.payload_redacted)
+    assert not (QUALITY_FIELDS & set((first.payload_redacted.get("extras") or {}).keys()))
+    _assert_private_analysis_text_absent(first.payload_redacted)
+
+
+def test_occurred_at_redaction_bypass_requires_timestamp_shape(tmp_path):
+    occurred_at = "2026-01-02T03:04:05.123456"
+    raw_nested = (
+        'call +1 415 555 0101; File "/tmp/private/raw_trace.py", line 9, '
+        "in handler; token sk-private-token"
+    )
+    redacted = redact_telemetry_payload(
+        {
+            "request_id": "openspace:test:occurred-at",
+            "occurred_at": occurred_at,
+            "status": "success",
+            "task_id": "task-1",
+            "cloud_skill_id": "cloud-skill-1",
+            "extras": {"occurred_at": raw_nested},
+        },
+        workspace_root=tmp_path,
+    )
+
+    assert redacted["occurred_at"] == occurred_at
+    nested = redacted["extras"]["occurred_at"]
+    assert nested != raw_nested
+    assert "[REDACTED_PHONE]" in nested
+    assert "<redacted>" in nested
+    assert "path_hash:" in nested
     for forbidden in (
-        "RAW_SKILL_NOTE",
+        "+1 415 555 0101",
-        "RAW_EXECUTION_NOTE",
+        "/tmp/private",
-        "RAW_TOOL_ISSUE",
+        "raw_trace.py",
-        "prompt",
+        "sk-private-token",
-        "transcript",
-        "file.diff",
-        "traceback",
-        "token",
-        "redacted_preview",
     ):
-        assert forbidden not in encoded
+        assert forbidden not in nested
+
+
+def test_status_mapping_ignores_free_text_partial_and_not_applicable_signals():
+    success_judgment = SkillJudgment(
+        "local-skill-1",
+        True,
+        "partial_success failed not_applicable raw note ignored",
+    )
+    success = build_skill_quality_judgment_payload(
+        _analysis(
+            task_completed=True,
+            judgments=[success_judgment],
+        ),
+        success_judgment,
+        cloud_skill_id="cloud-skill-1",
+    )
+    assert success["status"] == "success"
+    assert "failure_reason" not in success
+
+    failed_judgment = SkillJudgment(
+        "local-skill-1",
+        True,
+        "partial_success success not_applicable ignored",
+    )
+    failed = build_skill_quality_judgment_payload(
+        _analysis(
+            task_completed=False,
+            judgments=[failed_judgment],
+        ),
+        failed_judgment,
+        cloud_skill_id="cloud-skill-1",
+    )
+    assert failed["status"] == "failed"
+    assert failed["failure_reason"] == "unknown"
+    assert {success["status"], failed["status"]} <= {"success", "failed"}
 
 
 def test_phase_failed_success_candidate_reports_failed_unknown():
@@ -309,6 +543,7 @@ def test_outbox_redaction_preserves_quality_fields(monkeypatch, tmp_path):
     assert row.payload_redacted["quality_event_kind"] == QUALITY_EVENT_KIND
     assert row.payload_redacted["denominator"] == QUALITY_DENOMINATOR
     assert row.payload_redacted["fallback"] is True
+    _assert_private_analysis_text_absent(row.payload_redacted)
 
 
 def test_repeated_report_uses_same_outbox_row_for_same_payload(monkeypatch, tmp_path):
@@ -343,3 +578,59 @@ def test_repeated_report_uses_same_outbox_row_for_same_payload(monkeypatch, tmp_
             "cloud-skill-1",
         )
     }
+    assert len({row.payload_hash for row in failed_rows}) == 2
+
+
+def test_multi_skill_failed_trajectory_reports_one_payload_per_cloud_bound_judgment(
+    monkeypatch,
+    tmp_path,
+):
+    _set_cloud_env(monkeypatch, quality=True)
+    client = FakeClient()
+    reporter = CloudSkillQualityReporter(
+        client=client,
+        mapping_store=FakeMappingStore(
+            tmp_path,
+            {
+                "local-skill-1": SkillCloudBinding("local-skill-1", "cloud-skill-1"),
+                "local-skill-2": SkillCloudBinding("local-skill-2", "cloud-skill-2"),
+                "local-only": SkillCloudBinding("local-only", None),
+            },
+        ),
+        outbox=CloudTelemetryOutbox(tmp_path / "outbox.db"),
+    )
+    result = _run(
+        reporter.maybe_report_analysis(
+            _analysis(
+                task_completed=False,
+                judgments=[
+                    SkillJudgment("local-skill-1", True, "ignored success-ish note"),
+                    SkillJudgment("local-skill-2", False, "ignored partial note"),
+                    SkillJudgment("local-only", True, "ignored local note"),
+                ],
+                phase_failed_ids=["local-skill-2"],
+            )
+        )
+    )
+
+    assert result["status"] == "reported"
+    assert result["reported_count"] == 2
+    assert result["skipped_count"] == 1
+    payloads = [payload for event, payload in client.calls if event == "skill-use-reported"]
+    assert {payload["local_skill_id"] for payload in payloads} == {
+        "local-skill-1",
+        "local-skill-2",
+    }
+    for payload in payloads:
+        assert payload["status"] == "failed"
+        assert payload["failure_reason"] == "unknown"
+        assert payload["task_completed"] is False
+        assert payload["completed"] is False
+        assert payload["fallback"] is True
+        assert payload["denominator"] == QUALITY_DENOMINATOR
+        _assert_private_analysis_text_absent(payload)
+    by_local = {payload["local_skill_id"]: payload for payload in payloads}
+    assert by_local["local-skill-1"]["skill_applied"] is True
+    assert by_local["local-skill-1"]["skill_phase_failed"] is False
+    assert by_local["local-skill-2"]["skill_applied"] is False
+    assert by_local["local-skill-2"]["skill_phase_failed"] is True