diff --git a/src/plainweave/mcp_surface.py b/src/plainweave/mcp_surface.py index ca1c331..d1ae94b 100644 --- a/src/plainweave/mcp_surface.py +++ b/src/plainweave/mcp_surface.py @@ -1252,7 +1252,10 @@ def _append_preflight_fact( ErrorCode.INTERNAL, "preflight severity is not supported", recoverable=True, - hint="Refresh local Plainweave state and retry.", + hint=( + f"Internal defect: preflight severity must be one of {sorted(PREFLIGHT_SEVERITIES)}; " + "refreshing local state will not help." + ), ) facts.append( { diff --git a/src/plainweave/service.py b/src/plainweave/service.py index 6046760..01bfbb8 100644 --- a/src/plainweave/service.py +++ b/src/plainweave/service.py @@ -60,6 +60,38 @@ from plainweave.store import connect, read_schema_meta from plainweave.wardline_adapter import WardlineAdapter +#: Cause-appropriate default hints, one per :class:`ErrorCode` member. An error +#: must SAY WHAT IT KNOWS: a hint that does not point at the real fix actively +#: misdirects, so VALIDATION/NOT_FOUND must NEVER inherit a stale-state +#: "refresh and retry" hint (their cause is bad input / a missing id, not local +#: staleness). CONFLICT legitimately points at refetch-and-retry because reused +#: or superseded state IS its genuine cause. Sites that know more (e.g. the +#: version guard) override with a precise per-call hint. Advisory only — no +#: allow/block/approved/verdict/gate tokens. +_DEFAULT_ERROR_HINTS: dict[ErrorCode, str] = { + ErrorCode.VALIDATION: "Correct the invalid input described in the message and retry.", + ErrorCode.NOT_FOUND: "Confirm the referenced id exists (list or search to find it) and retry.", + ErrorCode.CONFLICT: ( + "Refetch the current state and retry - the value you sent no longer matches (it changed or was already used)." + ), + ErrorCode.POLICY_REQUIRED: "Satisfy the precondition named in the message before retrying.", + ErrorCode.PEER_ABSENT: ( + "The named peer facts are absent; enable or supply the sibling tool, or proceed treating enrichment as absent." + ), + ErrorCode.PEER_STALE: ( + "The peer's facts are stale relative to the current tree; re-run the peer's analysis to refresh them." + ), + ErrorCode.PEER_CONTRACT: ( + "The peer returned data that does not match the expected contract; check the peer's version and output." + ), + ErrorCode.LOCKED: ("The resource is held by another operation; wait for it to be freed or retry shortly."), + ErrorCode.UNSUPPORTED: "Use one of the supported operations or values described in the message.", + ErrorCode.INTERNAL: ( + "Internal defect: this should not occur from normal input and refreshing " + "local state will not help; report it with the message and details." + ), +} + class PlainweaveService: #: Actor kinds permitted to record external/manual attestation authority @@ -526,9 +558,19 @@ def update_draft( if not isinstance(draft_id, str): raise self._error(ErrorCode.POLICY_REQUIRED, "requirement has no active draft") draft = self._draft_row(connection, draft_id) - if expected_draft_revision is not None and int(draft["draft_revision"]) != expected_draft_revision: - raise self._error(ErrorCode.CONFLICT, "draft revision conflict") - next_revision = int(draft["draft_revision"]) + 1 + current_draft_revision = int(draft["draft_revision"]) + if expected_draft_revision is not None and current_draft_revision != expected_draft_revision: + raise self._error( + ErrorCode.CONFLICT, + f"expected draft revision {expected_draft_revision} does not match " + f"current draft revision {current_draft_revision}", + hint=f"Retry with --expected-draft-revision {current_draft_revision}.", + details={ + "expected_draft_revision": expected_draft_revision, + "current_draft_revision": current_draft_revision, + }, + ) + next_revision = current_draft_revision + 1 next_title = title if title is not None else str(draft["title"]) next_statement = statement if statement is not None else str(draft["statement"]) connection.execute( @@ -2101,7 +2143,12 @@ def _draft_row(self, connection: sqlite3.Connection, draft_id: str) -> sqlite3.R def _require_current_version(self, requirement: sqlite3.Row, expected_version: int) -> None: current_version = int(requirement["current_version"]) if current_version != expected_version: - raise self._error(ErrorCode.CONFLICT, "expected version does not match current version") + raise self._error( + ErrorCode.CONFLICT, + f"expected version {expected_version} does not match current version {current_version}", + hint=f"Retry with --expected-version {current_version}.", + details={"expected_version": expected_version, "current_version": current_version}, + ) def _project_key(self, connection: sqlite3.Connection) -> str: metadata = read_schema_meta(connection) @@ -2785,7 +2832,14 @@ def _validate_trace_relation(self, from_ref: TraceRef, relation: str, to_ref: Tr ("legis_attestation", "attests", "requirement_version"), } if (from_ref.kind, relation, to_ref.kind) not in allowed: - raise self._error(ErrorCode.VALIDATION, "trace relation is not canonical") + raise self._error( + ErrorCode.VALIDATION, + "trace relation is not canonical", + hint=( + "Use a canonical (from-kind, relation, to-kind) triple, " + "e.g. loomweave_entity satisfies requirement_version." + ), + ) def _validate_trace_transition(self, current: str, target: str) -> None: allowed = { @@ -2975,15 +3029,30 @@ def _require_accepted_trace_targets(self, connection: sqlite3.Connection, to_ref def _require_actor(self, actor: str) -> None: if not actor: - raise self._error(ErrorCode.VALIDATION, "actor is required") + raise self._error( + ErrorCode.VALIDATION, + "actor is required", + hint=( + "Pass --actor (e.g. --actor claude); " + "it need not be pre-registered for req/criterion commands." + ), + ) def _validate_verification_method(self, method: str) -> None: if method not in {"test", "analysis", "inspection", "manual"}: - raise self._error(ErrorCode.VALIDATION, "verification method is not supported") + raise self._error( + ErrorCode.VALIDATION, + "verification method is not supported", + hint="Pass --method as one of: test, analysis, inspection, manual.", + ) def _validate_evidence_status(self, status: str) -> None: if status not in {"passing", "failing", "inconclusive", "waived"}: - raise self._error(ErrorCode.VALIDATION, "verification evidence status is not supported") + raise self._error( + ErrorCode.VALIDATION, + "verification evidence status is not supported", + hint="Pass --status as one of: passing, failing, inconclusive, waived.", + ) def _actor_kind(self, connection: sqlite3.Connection, actor: str) -> str | None: """Return the registered kind for ``actor`` or ``None`` if unregistered.""" @@ -3025,8 +3094,18 @@ def _evidence_authority(self, connection: sqlite3.Connection, method: str, statu return "human_attested" return "agent_reported" - def _error(self, code: ErrorCode, message: str) -> PlainweaveError: - return PlainweaveError(code, message, recoverable=True, hint="Refresh local Plainweave state and retry.") + def _error( + self, + code: ErrorCode, + message: str, + *, + hint: str | None = None, + details: dict[str, object] | None = None, + ) -> PlainweaveError: + resolved_hint = ( + hint if hint is not None else _DEFAULT_ERROR_HINTS.get(code, "Review the error message and retry.") + ) + return PlainweaveError(code, message, recoverable=True, hint=resolved_hint, details=details) def _optional_int(self, value: object) -> int | None: return int(str(value)) if value is not None else None diff --git a/tests/contracts/test_cli_contract_outputs.py b/tests/contracts/test_cli_contract_outputs.py index aca2b25..8554a45 100644 --- a/tests/contracts/test_cli_contract_outputs.py +++ b/tests/contracts/test_cli_contract_outputs.py @@ -141,6 +141,11 @@ def test_cli_error_output_matches_contract_fixture( expected_status=2, ) assert_matches_fixture(error, load_fixture("cli/error-validation-json.json")) + # The missing-actor hint must point at --actor, not at stale local state, + # and must not silently grow details away from the pinned {}. + assert "--actor" in error["error"]["hint"] + assert "Refresh local Plainweave state" not in error["error"]["hint"] + assert error["error"]["details"] == {} def test_cli_conflict_error_output_matches_contract_fixture( @@ -193,6 +198,13 @@ def test_cli_conflict_error_output_matches_contract_fixture( expected_status=2, ) assert_matches_fixture(error, load_fixture("cli/error-conflict-json.json")) + # Say what you know: both version numbers surface in the message and details, + # the hint names the version to retry with, and it is not the stale-state blanket. + assert "expected version 0" in error["error"]["message"] + assert "current version 1" in error["error"]["message"] + assert error["error"]["details"] == {"expected_version": 0, "current_version": 1} + assert "--expected-version 1" in error["error"]["hint"] + assert "Refresh local Plainweave state" not in error["error"]["hint"] def test_criterion_cli_output_matches_contract_fixture( diff --git a/tests/fixtures/contracts/cli/error-conflict-json.json b/tests/fixtures/contracts/cli/error-conflict-json.json index e86a5ab..49d13c0 100644 --- a/tests/fixtures/contracts/cli/error-conflict-json.json +++ b/tests/fixtures/contracts/cli/error-conflict-json.json @@ -3,10 +3,13 @@ "ok": false, "error": { "code": "CONFLICT", - "message": "expected version does not match current version", + "message": "expected version 0 does not match current version 1", "recoverable": true, - "hint": "Refresh local Plainweave state and retry.", - "details": {} + "hint": "Retry with --expected-version 1.", + "details": { + "expected_version": 0, + "current_version": 1 + } }, "warnings": [], "meta": { diff --git a/tests/fixtures/contracts/cli/error-missing-actor.json b/tests/fixtures/contracts/cli/error-missing-actor.json index 0b7fed2..4345ed7 100644 --- a/tests/fixtures/contracts/cli/error-missing-actor.json +++ b/tests/fixtures/contracts/cli/error-missing-actor.json @@ -5,7 +5,7 @@ "code": "VALIDATION", "message": "actor is required", "recoverable": true, - "hint": "Refresh local Plainweave state and retry.", + "hint": "Pass --actor (e.g. --actor claude); it need not be pre-registered for req/criterion commands.", "details": {} }, "warnings": [], diff --git a/tests/fixtures/contracts/cli/error-validation-json.json b/tests/fixtures/contracts/cli/error-validation-json.json index 0b7fed2..4345ed7 100644 --- a/tests/fixtures/contracts/cli/error-validation-json.json +++ b/tests/fixtures/contracts/cli/error-validation-json.json @@ -5,7 +5,7 @@ "code": "VALIDATION", "message": "actor is required", "recoverable": true, - "hint": "Refresh local Plainweave state and retry.", + "hint": "Pass --actor (e.g. --actor claude); it need not be pre-registered for req/criterion commands.", "details": {} }, "warnings": [], diff --git a/tests/fixtures/contracts/envelopes/error-conflict.json b/tests/fixtures/contracts/envelopes/error-conflict.json index 1f5be08..10d503e 100644 --- a/tests/fixtures/contracts/envelopes/error-conflict.json +++ b/tests/fixtures/contracts/envelopes/error-conflict.json @@ -3,9 +3,9 @@ "ok": false, "error": { "code": "CONFLICT", - "message": "expected version 1 but current version is 2", + "message": "expected version 1 does not match current version 2", "recoverable": true, - "hint": "Refetch the requirement and retry with the current version.", + "hint": "Retry with --expected-version 2.", "details": { "expected_version": 1, "current_version": 2 diff --git a/tests/test_error_hints.py b/tests/test_error_hints.py new file mode 100644 index 0000000..90c4ac0 --- /dev/null +++ b/tests/test_error_hints.py @@ -0,0 +1,151 @@ +"""Anti-vacuous guards for the honest error-hint layer (functional honesty). + +An error must SAY WHAT IT KNOWS. A hint that does not point at the real fix +actively misdirects, so these tests pin three properties that the frozen +``weft.plainweave.error.v1`` envelope must keep honouring additively: + +1. Every :class:`ErrorCode` has a non-empty default hint, and VALIDATION / + NOT_FOUND never inherit the stale-state "Refresh local Plainweave state and + retry." blanket (their cause is bad input / a missing id, not staleness). +2. ``_error`` resolves an omitted hint from that map, threads an explicit hint + verbatim, and always coerces ``details`` to a dict. +3. The two dogfood findings (CONFLICT version guard; missing-actor VALIDATION) + emit precise, cause-appropriate hints and details end-to-end. +""" + +from __future__ import annotations + +from pathlib import Path + +import pytest + +from plainweave.errors import ErrorCode, PlainweaveError +from plainweave.service import _DEFAULT_ERROR_HINTS, PlainweaveService +from plainweave.store import migrate + +_BLANKET_HINT = "Refresh local Plainweave state and retry." + + +def service_for(tmp_path: Path) -> PlainweaveService: + db_path = tmp_path / ".plainweave" / "plainweave.db" + migrate(db_path, project_key="AUTH") + return PlainweaveService(db_path) + + +def test_default_hint_map_covers_every_error_code_non_empty() -> None: + for code in ErrorCode: + assert code in _DEFAULT_ERROR_HINTS, f"missing default hint for {code}" + hint = _DEFAULT_ERROR_HINTS[code] + assert hint and hint.strip(), f"empty default hint for {code}" + + +def test_validation_and_not_found_defaults_are_never_stale_state_hints() -> None: + for code in (ErrorCode.VALIDATION, ErrorCode.NOT_FOUND): + hint = _DEFAULT_ERROR_HINTS[code].lower() + assert _BLANKET_HINT.lower() not in hint + # No refresh/refetch-local-state phrasing either: the cause is the input, + # not local staleness. (CONFLICT may legitimately say "refetch".) + assert "refresh" not in hint + assert "refetch" not in hint + + +def test_error_resolves_default_hint_when_none(tmp_path: Path) -> None: + service = service_for(tmp_path) + + error = service._error(ErrorCode.NOT_FOUND, "requirement not found: REQ-X") + + assert error.hint == _DEFAULT_ERROR_HINTS[ErrorCode.NOT_FOUND] + assert error.hint + assert error.details == {} + + +def test_error_threads_explicit_hint_and_details_verbatim(tmp_path: Path) -> None: + service = service_for(tmp_path) + + error = service._error( + ErrorCode.CONFLICT, + "boom", + hint="Retry with --expected-version 7.", + details={"expected_version": 3, "current_version": 7}, + ) + + assert error.hint == "Retry with --expected-version 7." + assert error.details == {"expected_version": 3, "current_version": 7} + assert isinstance(error.details, dict) + + +def test_conflict_version_guard_says_what_it_knows(tmp_path: Path) -> None: + # add -> current version 0; approving with expected_version 1 trips the guard. + service = service_for(tmp_path) + draft = service.create_requirement( + "Reject expired bearer tokens", "The API shall reject expired tokens.", "human:john" + ) + + with pytest.raises(PlainweaveError) as exc_info: + service.approve_requirement(draft.id, actor="human:john", expected_version=1, idempotency_key="approve-1") + + error = exc_info.value + assert error.code == ErrorCode.CONFLICT + assert error.details["current_version"] == 0 + assert error.details["expected_version"] == 1 + assert "current version 0" in error.message + assert "--expected-version 0" in error.hint + assert _BLANKET_HINT not in error.hint + + +def test_draft_revision_guard_says_what_it_knows(tmp_path: Path) -> None: + # The same optimistic-concurrency defect as the requirement-version guard, on + # a sibling field: updating a draft with a wrong expected_draft_revision must + # disclose the actual revision (message + details + a --expected-draft-revision hint). + service = service_for(tmp_path) + draft = service.create_requirement( + "Reject expired bearer tokens", "The API shall reject expired tokens.", "human:john" + ) + + with pytest.raises(PlainweaveError) as exc_info: + service.update_draft(draft.id, actor="human:john", statement="revised", expected_draft_revision=99) + + error = exc_info.value + assert error.code == ErrorCode.CONFLICT + current = error.details["current_draft_revision"] + assert error.details["expected_draft_revision"] == 99 + assert f"current draft revision {current}" in error.message + assert f"--expected-draft-revision {current}" in error.hint + assert _BLANKET_HINT not in error.hint + + +def test_missing_actor_error_points_at_actor_not_stale_state(tmp_path: Path) -> None: + service = service_for(tmp_path) + + with pytest.raises(PlainweaveError) as exc_info: + service.create_requirement("Reject expired bearer tokens", "The API shall reject expired tokens.", "") + + error = exc_info.value + assert error.code == ErrorCode.VALIDATION + assert "--actor" in error.hint + assert _BLANKET_HINT not in error.hint + # The missing-actor golden pins details:{}; enrichment must not silently grow it. + assert error.details == {} + + +def _assert_validation_without_blanket_hint(exc_info: pytest.ExceptionInfo[PlainweaveError]) -> None: + assert exc_info.value.code == ErrorCode.VALIDATION + assert exc_info.value.hint != _BLANKET_HINT + assert _BLANKET_HINT not in exc_info.value.hint + + +def test_no_service_path_validation_error_carries_the_blanket_hint(tmp_path: Path) -> None: + """Guard: a VALIDATION error must never inherit the stale-state blanket hint.""" + service = service_for(tmp_path) + + with pytest.raises(PlainweaveError) as missing_actor: + service.create_requirement("t", "s", "") + _assert_validation_without_blanket_hint(missing_actor) + + with pytest.raises(PlainweaveError) as bad_method: + service._validate_verification_method("nonsense") + _assert_validation_without_blanket_hint(bad_method) + + with pytest.raises(PlainweaveError) as bad_status: + service._validate_evidence_status("nonsense") + _assert_validation_without_blanket_hint(bad_status) diff --git a/tests/test_mcp_read_surface.py b/tests/test_mcp_read_surface.py index 14652f3..32815cd 100644 --- a/tests/test_mcp_read_surface.py +++ b/tests/test_mcp_read_surface.py @@ -68,6 +68,11 @@ def assert_error(envelope: dict[str, Any], code: str) -> None: assert envelope["error"]["code"] == code assert envelope["error"]["recoverable"] is True assert envelope["error"]["hint"] + assert isinstance(envelope["error"]["details"], dict) + # Functional honesty: bad-input / missing-id errors must never inherit the + # stale-state "refresh and retry" blanket hint — it would misdirect the agent. + if code in {"VALIDATION", "NOT_FOUND"}: + assert envelope["error"]["hint"] != "Refresh local Plainweave state and retry." _VERDICT_KEYS = {"allow", "allowed", "block", "blocked", "verdict", "decision", "gate", "enforcement"}