fix(s41): close review WARN — OSError refusal + scheme/profile branch coverage
Post-hoc /trekreview (WARN, scope 0670b2a..b513e64) surfaced 2 findings, both closed: - MAJOR MISSING_TEST: the non-https scheme guard (preflight.py:61) and non-azure profile refusal (:82-86) had zero test coverage — added test_non_https_endpoint_refused + test_non_azure_profile_refused so an inverted condition can't regress silently. - MINOR MISSING_ERROR_HANDLING: an existing-but-unreadable PORTFOLIO_MODEL_MAP raised PermissionError (an OSError, not ValueError) past the except at :95 → widened to OSError (subsumes FileNotFoundError). test_unreadable_override_is_ structured_refusal proves the traceback-free invariant (red pre-fix). Suite 348->351/4, ruff+format+mypy clean.
This commit is contained in:
parent
b513e64577
commit
f3b21db105
2 changed files with 47 additions and 1 deletions
|
|
@ -92,7 +92,9 @@ def check_azure_preflight(profile: Profile | str = Profile.AZURE) -> PreflightOK
|
||||||
# Placeholder refusal: resolving each role raises ValueError on a REPLACE-WITH-* id.
|
# Placeholder refusal: resolving each role raises ValueError on a REPLACE-WITH-* id.
|
||||||
for role in _ROLES:
|
for role in _ROLES:
|
||||||
resolve_model(prof, role)
|
resolve_model(prof, role)
|
||||||
except (FileNotFoundError, ValidationError, ValueError) as exc:
|
except (OSError, ValidationError, ValueError) as exc:
|
||||||
|
# OSError subsumes FileNotFoundError AND PermissionError: an existing-but-unreadable
|
||||||
|
# PORTFOLIO_MODEL_MAP (read_text after is_file()) must refuse cleanly, never traceback.
|
||||||
return PreflightRefusal(str(exc))
|
return PreflightRefusal(str(exc))
|
||||||
return PreflightOK(prof)
|
return PreflightOK(prof)
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -7,6 +7,7 @@ Load-bearing detach seams (AST no-network/no-auto-login guard + refusal teeth) l
|
||||||
from __future__ import annotations
|
from __future__ import annotations
|
||||||
|
|
||||||
import json
|
import json
|
||||||
|
import os
|
||||||
from pathlib import Path
|
from pathlib import Path
|
||||||
|
|
||||||
import pytest
|
import pytest
|
||||||
|
|
@ -102,6 +103,49 @@ def test_malformed_override_is_structured_refusal(
|
||||||
assert capsys.readouterr().err.strip()
|
assert capsys.readouterr().err.strip()
|
||||||
|
|
||||||
|
|
||||||
|
def test_non_https_endpoint_refused(
|
||||||
|
tmp_path: Path, monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str]
|
||||||
|
) -> None:
|
||||||
|
# Review MAJOR (d87baffb…): the non-https scheme guard (preflight.py:61) had no test — every
|
||||||
|
# other endpoint fixture is https://. Drive an http:// endpoint through it → rc 1 naming the
|
||||||
|
# https requirement, so an inverted condition / broken message can't regress silently.
|
||||||
|
monkeypatch.setenv("PORTFOLIO_MODEL_MAP", str(_write_map(tmp_path, _VALID_MAP)))
|
||||||
|
monkeypatch.setenv("PORTFOLIO_FOUNDRY_PROJECT_ENDPOINT", "http://x.services.ai.azure.com")
|
||||||
|
rc = preflight.main(["--profile", "azure"])
|
||||||
|
assert rc == 1
|
||||||
|
assert "https://" in capsys.readouterr().err
|
||||||
|
|
||||||
|
|
||||||
|
def test_non_azure_profile_refused(monkeypatch: pytest.MonkeyPatch) -> None:
|
||||||
|
# Review MAJOR (d87baffb…): the non-azure profile refusal (preflight.py:82-86) had no test.
|
||||||
|
# check_azure_preflight('local') must return a PreflightRefusal (the local profile needs no
|
||||||
|
# preflight), not a PreflightOK — the branch fires before any endpoint/map lookup, so no env.
|
||||||
|
result = preflight.check_azure_preflight("local")
|
||||||
|
assert isinstance(result, preflight.PreflightRefusal)
|
||||||
|
assert "local" in result.reason
|
||||||
|
|
||||||
|
|
||||||
|
def test_unreadable_override_is_structured_refusal(
|
||||||
|
tmp_path: Path, monkeypatch: pytest.MonkeyPatch, capsys: pytest.CaptureFixture[str]
|
||||||
|
) -> None:
|
||||||
|
# Review MINOR (07dab06f…): an existing-but-UNREADABLE PORTFOLIO_MODEL_MAP raises PermissionError
|
||||||
|
# (an OSError, not a ValueError) from _load_effective_map's read_text — it passes is_file() first.
|
||||||
|
# Preflight must still refuse cleanly (rc 1, no traceback), guarding the traceback-free invariant
|
||||||
|
# on the OSError branch, not just FileNotFoundError.
|
||||||
|
if hasattr(os, "geteuid") and os.geteuid() == 0:
|
||||||
|
pytest.skip("root bypasses the file-permission bits; chmod 000 would stay readable")
|
||||||
|
bad = _write_map(tmp_path, _VALID_MAP)
|
||||||
|
bad.chmod(0o000)
|
||||||
|
try:
|
||||||
|
monkeypatch.setenv("PORTFOLIO_FOUNDRY_PROJECT_ENDPOINT", _VALID_ENDPOINT)
|
||||||
|
monkeypatch.setenv("PORTFOLIO_MODEL_MAP", str(bad))
|
||||||
|
rc = preflight.main(["--profile", "azure"])
|
||||||
|
assert rc == 1
|
||||||
|
assert capsys.readouterr().err.strip()
|
||||||
|
finally:
|
||||||
|
bad.chmod(0o644)
|
||||||
|
|
||||||
|
|
||||||
def test_auth_recipe_doc_exists_and_names_facts() -> None:
|
def test_auth_recipe_doc_exists_and_names_facts() -> None:
|
||||||
# SC8 doc-guard: the verified recipe lives in docs/ (NOT the preflight docstring, so the
|
# SC8 doc-guard: the verified recipe lives in docs/ (NOT the preflight docstring, so the
|
||||||
# AST grep-guard stays clean) and pins the load-bearing facts so it can't rot silently. The
|
# AST grep-guard stays clean) and pins the load-bearing facts so it can't rot silently. The
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue