mirror of
https://github.com/razzant/ouroboros.git
synced 2026-10-02 19:58:46 +00:00
fix(#1195): execution resolves a skill identity with ordinary discovery
Review finding (gpt-6-astra, triad): the liveness read, reconcile_extension and the extension child had moved from discover_skills to discover_selected_skill_candidates, the repair/publication resolver that opts a manifestless directory in as a hidden ambiguity. A reviewed extension beside a same-named manifestless directory became a collision placeholder: reconcile unloaded it and dispatch refused it. The execution callers now use discover_skill_identity — one identity under ORDINARY discovery semantics, the same inventory find_skill reads — and the strict resolver stays with repair/publication. Regression: tests/test_extension_manifestless_neighbour.py (fails on the previous tree). Also from this round: enabled_skill_conflicts reads skill.conflicts so a SkillPeer is a valid subject (gpt-5.6-sol; differential test added), and docs/MODEL_SEND_OBSERVABILITY.md names startup_historical_audit.py as the seal reconciliation owner instead of the removed server_maintenance sweep (grok, scope). FACADE_INVENTORY regenerated for the new re-export.
This commit is contained in:
parent
a88ca20b8f
commit
6613ea9257
11 changed files with 139 additions and 21 deletions
|
|
@ -2,7 +2,9 @@
|
|||
|
||||
The physical-send observability contract is implemented in
|
||||
`ouroboros/model_send_seal.py`, wired at
|
||||
`llm_attempt._candidate_before_dispatch` and swept from `server_maintenance`;
|
||||
`llm_attempt._candidate_before_dispatch` and swept by the session-custodied
|
||||
history child `ouroboros/startup_historical_audit.py` (launched after
|
||||
supervisor readiness, off the readiness path);
|
||||
`tests/test_model_send_seal.py` verifies it. The claim is deliberately narrow:
|
||||
a reconstruction mismatch is an observability fact, not a dispatch gate.
|
||||
|
||||
|
|
@ -109,8 +111,9 @@ bounded, local, and on the same drive the record was just written to.
|
|||
|
||||
### 3.3 Reverse direction (audit, `model_send` only)
|
||||
|
||||
A bounded reconciliation sweep (rides the existing startup-sweep family in
|
||||
`server_maintenance`, not a new scheduler):
|
||||
A bounded reconciliation sweep (runs inside the session-custodied history
|
||||
child `startup_historical_audit.py`, started by `server.py::_run_supervisor`
|
||||
after readiness — not on the readiness path, not a new scheduler):
|
||||
|
||||
- every `model_send` seal ⟶ exactly one attempt row in the usage-accounting
|
||||
replay (any terminal state, including refused-before-dispatch);
|
||||
|
|
@ -220,8 +223,9 @@ from "our two copies agree" to "the durable record agrees with the wire".
|
|||
- `model_send_seal.py` stamps the physical-candidate manifest, reads back the
|
||||
durable projection, writes typed mismatch facts and reconciles both join
|
||||
directions. The seal is an additive key under the existing manifest schema.
|
||||
- `server_maintenance.py` runs the bounded reconciliation through the existing
|
||||
startup sweep. Unknown accounting evidence does not become an orphan claim;
|
||||
- `startup_historical_audit.py` runs the bounded reconciliation as one
|
||||
session-custodied child per generation, launched after supervisor readiness
|
||||
(#1195 F1). Unknown accounting evidence does not become an orphan claim;
|
||||
the sweep records facts without deleting records or fabricating attempts.
|
||||
- `tests/test_model_send_seal.py` covers reconstruction, typed divergence,
|
||||
non-blocking dispatch and reverse joins. Compacted history is resolved through
|
||||
|
|
|
|||
|
|
@ -2,7 +2,7 @@
|
|||
|
||||
AST-derived inventory of compatibility facades, regenerated by `python scripts/regenerate_inventories.py`. Do not edit. A facade row is any runtime module whose top-level `from <population module> import ...` statements carry the `noqa: F401` re-export marker — the codebase's declared "this binding exists for its binding, not for this module's own use" convention (reference FACADE_CONSUMERS method). Leaf domains come from `ouroboros/domains.toml`; a leaf outside the facade's domain is marked ✗ (that edge also appears in the manifest's pinned direction matrix). `tests/test_generated_inventories.py` pins byte-identity, so any re-export surface change must regenerate this file.
|
||||
|
||||
- facade modules: **60**; marked re-export bindings: **2312**; cross-domain facade→leaf pairs: **132**
|
||||
- facade modules: **60**; marked re-export bindings: **2313**; cross-domain facade→leaf pairs: **132**
|
||||
|
||||
| facade | domain | bindings | leaves |
|
||||
|---|---|---:|---|
|
||||
|
|
@ -12,7 +12,7 @@ AST-derived inventory of compatibility facades, regenerated by `python scripts/r
|
|||
| `ouroboros/config.py` | D12 | 133 | `ouroboros/model_slots.py` (17)<br>`ouroboros/provider_models.py` (6 ✗D02)<br>`ouroboros/review_model_routes.py` (10)<br>`ouroboros/runtime_limits.py` (62)<br>`ouroboros/settings_defaults.py` (19)<br>`ouroboros/settings_integrity.py` (4)<br>`ouroboros/settings_scales.py` (13)<br>`ouroboros/update_channels.py` (2) |
|
||||
| `ouroboros/context.py` | D03 | 4 | `ouroboros/context_runtime_facts.py` (4) |
|
||||
| `ouroboros/delegate_custody.py` | D07 | 10 | `ouroboros/delegate_custody_reconcile.py` (9)<br>`ouroboros/delegate_evidence.py` (1) |
|
||||
| `ouroboros/extension_loader.py` | D14 | 100 | `ouroboros/contracts/plugin_api.py` (7 ✗D19)<br>`ouroboros/extension_child_catalog.py` (8)<br>`ouroboros/extension_companion.py` (3)<br>`ouroboros/extension_import_staging.py` (6)<br>`ouroboros/extension_isolated_deps.py` (4)<br>`ouroboros/extension_liveness.py` (8)<br>`ouroboros/extension_plugin_api.py` (6)<br>`ouroboros/extension_registry_state.py` (20)<br>`ouroboros/extension_surface_names.py` (12)<br>`ouroboros/extension_ui_validation.py` (5)<br>`ouroboros/gateway/host_service.py` (1 ✗D11)<br>`ouroboros/provider_models.py` (1 ✗D02)<br>`ouroboros/skill_loader.py` (14)<br>`ouroboros/skill_token.py` (1)<br>`ouroboros/tools/skill_exec.py` (1)<br>`ouroboros/utils.py` (3 ✗D18) |
|
||||
| `ouroboros/extension_loader.py` | D14 | 101 | `ouroboros/contracts/plugin_api.py` (7 ✗D19)<br>`ouroboros/extension_child_catalog.py` (8)<br>`ouroboros/extension_companion.py` (3)<br>`ouroboros/extension_import_staging.py` (6)<br>`ouroboros/extension_isolated_deps.py` (4)<br>`ouroboros/extension_liveness.py` (8)<br>`ouroboros/extension_plugin_api.py` (6)<br>`ouroboros/extension_registry_state.py` (20)<br>`ouroboros/extension_surface_names.py` (12)<br>`ouroboros/extension_ui_validation.py` (5)<br>`ouroboros/gateway/host_service.py` (1 ✗D11)<br>`ouroboros/provider_models.py` (1 ✗D02)<br>`ouroboros/skill_loader.py` (15)<br>`ouroboros/skill_token.py` (1)<br>`ouroboros/tools/skill_exec.py` (1)<br>`ouroboros/utils.py` (3 ✗D18) |
|
||||
| `ouroboros/gateway/_helpers.py` | D11 | 2 | `ouroboros/jsonl_tail.py` (2 ✗D18) |
|
||||
| `ouroboros/gateway/contracts.py` | D11 | 7 | `ouroboros/gateway/decision_contracts.py` (2)<br>`ouroboros/gateway/history_contracts.py` (1)<br>`ouroboros/gateway/schedule_contracts.py` (4) |
|
||||
| `ouroboros/gateway/history.py` | D11 | 1 | `ouroboros/gateway/cost_breakdown.py` (1) |
|
||||
|
|
|
|||
|
|
@ -17,7 +17,7 @@ from ouroboros.extension_registry_state import _extensions, _load_failures, _loc
|
|||
from ouroboros.skill_loader import (
|
||||
LoadedSkill,
|
||||
_sanitize_skill_name,
|
||||
discover_selected_skill_candidates,
|
||||
discover_skill_identity,
|
||||
grant_status_for_skill,
|
||||
skill_conflict_status,
|
||||
)
|
||||
|
|
@ -191,7 +191,7 @@ def runtime_state_for_skill_name(
|
|||
elif skills is not None:
|
||||
selected = list(skills)
|
||||
else:
|
||||
selected = discover_selected_skill_candidates(drive_root, skill_name, repo_path=resolved_repo_path)
|
||||
selected = discover_skill_identity(drive_root, skill_name, repo_path=resolved_repo_path)
|
||||
peer_projection = list(skills) if skills is not None else list(
|
||||
discover_skill_peers(drive_root, repo_path=resolved_repo_path)
|
||||
)
|
||||
|
|
|
|||
|
|
@ -132,7 +132,7 @@ from ouroboros.extension_surface_names import (
|
|||
extension_surface_name, # noqa: F401
|
||||
parse_extension_surface_name, # noqa: F401
|
||||
)
|
||||
from ouroboros.skill_loader import _SKILL_DIR_CACHE_NAMES, _sanitize_skill_name, LoadedSkill, SkillPayloadUnreadable, compute_content_hash, discover_skills, discover_selected_skill_candidates, find_skill, grant_status_for_skill, requested_core_setting_keys, skill_conflict_status, skill_review_gate, skill_state_dir, skill_state_dir_path # noqa: F401
|
||||
from ouroboros.skill_loader import _SKILL_DIR_CACHE_NAMES, _sanitize_skill_name, LoadedSkill, SkillPayloadUnreadable, compute_content_hash, discover_skills, discover_selected_skill_candidates, discover_skill_identity, find_skill, grant_status_for_skill, requested_core_setting_keys, skill_conflict_status, skill_review_gate, skill_state_dir, skill_state_dir_path # noqa: F401
|
||||
from ouroboros.skill_token import SkillToken # noqa: F401
|
||||
from ouroboros.tools.skill_exec import _scrub_env # noqa: F401
|
||||
from ouroboros.utils import atomic_write_json, read_json_dict, utc_now_iso # noqa: F401
|
||||
|
|
@ -323,7 +323,7 @@ def reconcile_extension(
|
|||
from ouroboros.config import get_skills_repo_path
|
||||
|
||||
resolved_repo_path = get_skills_repo_path() if repo_path is None else repo_path
|
||||
peers = list(skills) if skills is not None else discover_selected_skill_candidates(
|
||||
peers = list(skills) if skills is not None else discover_skill_identity(
|
||||
drive_root, skill_name, repo_path=resolved_repo_path
|
||||
)
|
||||
if selected_skill is not None:
|
||||
|
|
|
|||
|
|
@ -865,7 +865,7 @@ def _load_child_extension(skill_name: str, drive_root: pathlib.Path, repo_dir: p
|
|||
from ouroboros.config import load_settings
|
||||
from ouroboros.extension_loader import load_extension
|
||||
from ouroboros.settings_integrity import _next_task_setting
|
||||
from ouroboros.skill_loader import discover_selected_skill_candidates
|
||||
from ouroboros.skill_loader import discover_skill_identity
|
||||
|
||||
def settings_reader():
|
||||
live = load_settings()
|
||||
|
|
@ -876,7 +876,7 @@ def _load_child_extension(skill_name: str, drive_root: pathlib.Path, repo_dir: p
|
|||
return {**{key: value for key, value in live.items() if not _next_task_setting(key)},
|
||||
**task_settings}
|
||||
|
||||
skills = discover_selected_skill_candidates(drive_root, skill_name, repo_path=str(skills_repo_path))
|
||||
skills = discover_skill_identity(drive_root, skill_name, repo_path=str(skills_repo_path))
|
||||
skill = next((item for item in skills if item.name == skill_name), None)
|
||||
if skill is None:
|
||||
raise ExtensionProcessError(f"extension skill {skill_name!r} is missing")
|
||||
|
|
|
|||
|
|
@ -23,7 +23,7 @@ def enabled_skill_conflicts(skill: Any, skills: List[Any]) -> List[str]:
|
|||
A declaration on either side is authoritative, so one-sided manifests are
|
||||
enforced symmetrically. Missing and disabled peers are deliberately inert.
|
||||
"""
|
||||
declared = set(skill.manifest.conflicts or [])
|
||||
declared = set(skill.conflicts or ())
|
||||
conflicts = {
|
||||
peer.name
|
||||
for peer in skills
|
||||
|
|
|
|||
|
|
@ -1406,6 +1406,34 @@ def discover_selected_skill_candidates(
|
|||
)
|
||||
|
||||
|
||||
def discover_skill_identity(
|
||||
drive_root: pathlib.Path,
|
||||
name: str,
|
||||
*,
|
||||
repo_path: str | None = None,
|
||||
) -> List[LoadedSkill]:
|
||||
"""Load one identity with ORDINARY discovery semantics, nothing else read.
|
||||
|
||||
Same inventory and collision rules as ``discover_skills`` restricted to one
|
||||
canonical name — no manifestless opt-in, so a directory without a manifest
|
||||
beside a valid skill of the same name stays invisible here exactly as it is
|
||||
to passive discovery. This is the resolver every EXECUTION caller uses
|
||||
(liveness, reconcile, the extension child); the repair/publication lanes
|
||||
keep ``discover_selected_skill_candidates`` and its deliberately stricter
|
||||
manifestless ambiguity.
|
||||
"""
|
||||
if repo_path is None:
|
||||
from ouroboros.config import get_skills_repo_path
|
||||
|
||||
repo_path = get_skills_repo_path()
|
||||
safe = _sanitize_skill_name(name)
|
||||
candidates = tuple(
|
||||
item for item in _skill_location_inventory(drive_root, repo_path=repo_path)
|
||||
if item.name == safe
|
||||
)
|
||||
return _load_skill_location_candidates(candidates, drive_root=drive_root)
|
||||
|
||||
|
||||
def find_skill(
|
||||
drive_root: pathlib.Path,
|
||||
name: str,
|
||||
|
|
@ -1414,8 +1442,7 @@ def find_skill(
|
|||
) -> Optional[LoadedSkill]:
|
||||
"""Return one skill by name, including broken manifests with ``load_error``."""
|
||||
safe = _sanitize_skill_name(name)
|
||||
candidates = tuple(item for item in _skill_location_inventory(drive_root, repo_path=repo_path) if item.name == safe)
|
||||
for skill in _load_skill_location_candidates(candidates, drive_root=drive_root):
|
||||
for skill in discover_skill_identity(drive_root, name, repo_path=repo_path):
|
||||
if skill.name == safe:
|
||||
return skill
|
||||
return None
|
||||
|
|
@ -1560,7 +1587,7 @@ __all__ = [
|
|||
"AutoGrantOutcome", "LoadedSkill", "HASH_EXEMPT_CONTROL_FILENAMES",
|
||||
"SkillReviewState", "auto_grant_if_enabled",
|
||||
"VALID_REVIEW_STATUSES", "compute_content_hash", "reduce_skill_content_hash", "discover_skills",
|
||||
"discover_selected_skill_candidates", "find_skill",
|
||||
"discover_selected_skill_candidates", "discover_skill_identity", "find_skill",
|
||||
"enabled_skill_conflicts", "skill_conflict_status",
|
||||
"grant_status_for_skill", "is_self_authored_skill_dir", "list_available_for_execution",
|
||||
"load_enabled", "load_review_state", "load_skill_grants", "load_skill",
|
||||
|
|
|
|||
66
tests/test_extension_manifestless_neighbour.py
Normal file
66
tests/test_extension_manifestless_neighbour.py
Normal file
|
|
@ -0,0 +1,66 @@
|
|||
"""Execution discovery ignores a manifestless directory beside a valid skill.
|
||||
|
||||
Regression for the #1195 F4 review finding: the execution callers (liveness,
|
||||
reconcile, the extension child) must resolve a selected identity with ORDINARY
|
||||
discovery semantics. Only the repair/publication lane may opt a manifestless
|
||||
directory in as a hidden ambiguity; a reviewed extension next to a same-named
|
||||
directory without a manifest keeps executing.
|
||||
"""
|
||||
from __future__ import annotations
|
||||
|
||||
import pathlib
|
||||
|
||||
from ouroboros import extension_loader
|
||||
from ouroboros.skill_loader import (
|
||||
discover_selected_skill_candidates,
|
||||
discover_skill_identity,
|
||||
)
|
||||
|
||||
from tests._extension_loader_shared import _prepare_extension
|
||||
from tests._extension_loader_shared import ( # noqa: F401 (autouse fixture applies on import)
|
||||
_clear_loader_state,
|
||||
)
|
||||
|
||||
|
||||
def _manifestless_neighbour(drive_root: pathlib.Path, name: str) -> pathlib.Path:
|
||||
# Same canonical identity, no SKILL.md: the layout the publish preflight
|
||||
# treats as ambiguous and ordinary discovery treats as absent.
|
||||
neighbour = drive_root / "skills" / "external" / name
|
||||
neighbour.mkdir(parents=True)
|
||||
(neighbour / "notes.txt").write_text("not a skill\n", encoding="utf-8")
|
||||
return neighbour
|
||||
|
||||
|
||||
def test_manifestless_neighbour_is_invisible_to_execution_discovery(tmp_path):
|
||||
loaded, repo_root, drive_root = _prepare_extension(
|
||||
tmp_path, "neighbour_case", "def register(api):\n pass\n", permissions=[],
|
||||
)
|
||||
_manifestless_neighbour(drive_root, loaded.name)
|
||||
|
||||
ordinary = discover_skill_identity(drive_root, loaded.name, repo_path=str(repo_root))
|
||||
assert [s.identity_collision for s in ordinary] == [False]
|
||||
assert ordinary[0].skill_dir.resolve() == loaded.skill_dir.resolve()
|
||||
|
||||
# The stricter repair/publication resolver still sees the ambiguity.
|
||||
strict = discover_selected_skill_candidates(drive_root, loaded.name, repo_path=str(repo_root))
|
||||
assert len(strict) == 2 and all(s.identity_collision for s in strict)
|
||||
|
||||
|
||||
def test_reviewed_extension_still_loads_beside_a_manifestless_neighbour(tmp_path):
|
||||
loaded, repo_root, drive_root = _prepare_extension(
|
||||
tmp_path, "neighbour_exec", "def register(api):\n pass\n", permissions=[],
|
||||
)
|
||||
_manifestless_neighbour(drive_root, loaded.name)
|
||||
|
||||
state = extension_loader.reconcile_extension(
|
||||
loaded.name, drive_root, lambda: {}, repo_path=str(repo_root),
|
||||
)
|
||||
assert state["action"] == "extension_loaded", state
|
||||
assert state["live_loaded"] is True
|
||||
|
||||
runtime = extension_loader.runtime_state_for_skill_name(
|
||||
loaded.name, drive_root, repo_path=str(repo_root),
|
||||
)
|
||||
assert runtime["live_loaded"] is True
|
||||
assert runtime.get("identity_collision", False) is False
|
||||
assert runtime["reason"] != "missing"
|
||||
|
|
@ -113,14 +113,14 @@ def test_child_extension_load_reuses_one_discovered_peer_snapshot(tmp_path, monk
|
|||
permissions=[],
|
||||
)
|
||||
calls = 0
|
||||
real_discover = skill_loader.discover_selected_skill_candidates
|
||||
real_discover = skill_loader.discover_skill_identity
|
||||
|
||||
def counted_discover(*args, **kwargs):
|
||||
nonlocal calls
|
||||
calls += 1
|
||||
return real_discover(*args, **kwargs)
|
||||
|
||||
monkeypatch.setattr(skill_loader, "discover_selected_skill_candidates", counted_discover)
|
||||
monkeypatch.setattr(skill_loader, "discover_skill_identity", counted_discover)
|
||||
monkeypatch.setattr("ouroboros.config.load_settings", lambda: {})
|
||||
|
||||
runner._load_child_extension(
|
||||
|
|
|
|||
|
|
@ -166,14 +166,14 @@ def test_reconcile_loads_selected_payload_without_full_peer_discovery(tmp_path,
|
|||
permissions=[],
|
||||
)
|
||||
calls = 0
|
||||
real_discover = extension_loader.discover_selected_skill_candidates
|
||||
real_discover = extension_loader.discover_skill_identity
|
||||
|
||||
def counted_discover(*args, **kwargs):
|
||||
nonlocal calls
|
||||
calls += 1
|
||||
return real_discover(*args, **kwargs)
|
||||
|
||||
monkeypatch.setattr(extension_loader, "discover_selected_skill_candidates", counted_discover)
|
||||
monkeypatch.setattr(extension_loader, "discover_skill_identity", counted_discover)
|
||||
|
||||
state = extension_loader.reconcile_extension(
|
||||
loaded.name,
|
||||
|
|
|
|||
|
|
@ -245,3 +245,24 @@ def test_skill_peer_is_immutable_and_non_executable():
|
|||
assert not hasattr(peer, "content_hash")
|
||||
assert not hasattr(peer, "review")
|
||||
assert not hasattr(peer, "manifest")
|
||||
|
||||
|
||||
def test_a_peer_is_a_valid_subject_of_the_conflict_projection(tmp_path):
|
||||
"""Duck typing holds on BOTH sides: a SkillPeer subject reaches the same verdict.
|
||||
|
||||
`enabled_skill_conflicts` reads `skill.conflicts`, never `skill.manifest`,
|
||||
because `SkillPeer` deliberately has no manifest.
|
||||
"""
|
||||
drive = tmp_path / "drive"
|
||||
skills = drive / "skills" / "external"
|
||||
_write(skills / "declares", _manifest("declares", conflicts=("target",)))
|
||||
_write(skills / "target", _manifest("target"))
|
||||
save_enabled(drive, "declares", True)
|
||||
save_enabled(drive, "target", True)
|
||||
full = {s.name: s for s in discover_skills(drive, repo_path="")}
|
||||
peers = {p.name: p for p in discover_skill_peers(drive, repo_path="")}
|
||||
for name in ("declares", "target"):
|
||||
as_full = skill_conflict_status(full[name], list(peers.values()))
|
||||
as_peer = skill_conflict_status(peers[name], list(peers.values()))
|
||||
assert as_peer is not None, name
|
||||
assert as_full == as_peer, name
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue