mirror of
https://github.com/razzant/ouroboros.git
synced 2026-10-03 04:07:04 +00:00
Preserve typed failures across built-in plan, image and GitHub tools (#739)
A built-in tool that already knows its call failed must leave the registry
a typed result. The legacy adapter types a string by its first-line
`⚠️ IDENTIFIER` marker, so identifier-less prose and bare `ERROR:` strings
were recorded as successful calls in tools.jsonl, the outcome classifier
and the acceptance packet.
- plan_task: a schema-equivalent empty optional field (blank goal/plan,
`{}` or a spec of declared keys holding [] / "") beside a real disposition
no longer reads as a mixed envelope; a non-empty list, unknown key or
wrong type still refuses typed. Every plan-review refusal publishes typed
(argument vs state), and `_plan_unavailable` splits faults (error) from
availability outcomes such as a budget-declined panel (unavailable).
- view_image / vlm_query: the local image loader publishes its own
refusals as TOOL_ARG_ERROR; policy markers keep their owners' typing; the
host's auto-attach stays non-fatal outside a registry call.
- GitHub: `_gh_run` returns a structured result (exit code, bounded and
redacted stderr head, gh's own `(HTTP NNN)` marker read before bounding,
failure class); `_gh_cmd` keeps the string ABI and its typed target
refusals. Publication carries error_detail / github_status /
github_operation beside the stage code and through the structured
attempt projection; hints state producer facts and name Settings →
Secrets; mandatory fork sync still stops before any mutation and PR
settlement stays a read-only exact lookup.
- tests/test_typed_tool_refusals.py: shrink-only source lint over returned
failure literals in ouroboros/tools (oracle: the real adapter); its
allowlist is the disclosed residual for the other packages.
- docs: DEVELOPMENT (typed-refusal rule, plan closure), ARCHITECTURE
(vision, skill publication, github.py row).
This commit is contained in:
Ouroboros
parent
3f48bd2f4b
commit
becededcb7
14 changed files with 1121 additions and 165 deletions
|
|
@ -403,7 +403,7 @@ server.py (Starlette+uvicorn) ← HTTP + WebSocket on configurable host:port (de
|
|||
│ ├── commit_gate.py ← Commit gate: `_record_commit_attempt` (LLM claim synthesis), `classify_review_block`/`attempt_block_class`, `check_identical_verdict_refusal`, `count_paid_review_cycles`/`check_review_cycles_ceiling`, `commit_review_contract_fingerprint`
|
||||
│ ├── git_rollback.py ← Wraps `git_ops.rollback_to_version`
|
||||
│ ├── git_pr.py ← Five PR tools (non-core)
|
||||
│ ├── github.py ← Issue + PR tools (frozen tool module): shared process binding selects the active Project; explicit repo flows through every subcall; missing implicit Project targets fail visibly. Discovery reads token sources or native CLI configuration without an authentication probe; explicit Hub/API transport retains its own target.
|
||||
│ ├── github.py ← Issue + PR tools (frozen tool module): shared process binding selects the active Project; explicit repo flows through every subcall; missing implicit Project targets fail visibly. Discovery reads token sources or native CLI configuration without an authentication probe; explicit Hub/API transport retains its own target. `_gh_run` is the structured transport read (exit code, bounded redacted stderr head, observed HTTP status, failure class) that `_gh_cmd` projects onto the string ABI; the transport publishes nothing into the tool-result sidecar, because the publication transaction owns its own final result.
|
||||
│ ├── parallel_review.py ← Triad + scope review orchestration: assembly of both packets, the money admission call (`review_admission.py`), the scope-first hold, and the executor transitions that carry the admitting usage scope (`contextvars.copy_context`) into every seat
|
||||
│ ├── plan_review_references.py ← Reference projection that also writes its own provenance rows (`append_jsonl` into `logs/progress.jsonl`, `emit_log_event`), never a second plan authority
|
||||
│ ├── plan_review.py ← `plan_task` engine: evidence, packet, fan-out over the review substrate, `plan_review_state` v2, the shared `OUROBOROS_REVIEW_MAX_CYCLES` cap, free identical replays; no scouts, Atlas, or plan_class
|
||||
|
|
@ -1114,7 +1114,7 @@ Prompt caching is stable-first: governance and task-stable contracts precede mut
|
|||
|
||||
#### Vision and local image evidence
|
||||
|
||||
`analyze_screenshot` and `vlm_query` are bounded secondary-model calls through `LLMClient.vision_query`; `view_image` attaches a local image natively. Send-time image routing works on a copy of the transcript, so captioning or placeholder conversion never mutates canonical history; payloads are validated, capped/downscaled, and confined to readable roots derived from the Tool API policy matrix plus the protected-artifact rule. `vision.attach_local_image_to_context` is the single attachment seam for explicit `view_image` and the typed `auto_attach_image` opt-in — same durable copy, trust boundary, and live-image eviction budget; auto-attachment failure is non-fatal. Vision/local-media tools are not web tools and may be withheld by the task contract.
|
||||
`analyze_screenshot` and `vlm_query` are bounded secondary-model calls through `LLMClient.vision_query`; `view_image` attaches a local image natively. Send-time image routing works on a copy of the transcript, so captioning or placeholder conversion never mutates canonical history; payloads are validated, capped/downscaled, and confined to readable roots derived from the Tool API policy matrix plus the protected-artifact rule. `vision.attach_local_image_to_context` is the single attachment seam for explicit `view_image` and the typed `auto_attach_image` opt-in — same durable copy, trust boundary, and live-image eviction budget; auto-attachment failure is non-fatal. The loader's own refusals — missing file, outside the readable roots, oversized, unreadable, not a supported image — are published as typed `TOOL_ARG_ERROR` results before their text returns, so `view_image` and `vlm_query` record a known failure as one; a refusal from a policy owner keeps that owner's typed marker (blocked), and the host's same-round auto-attach, which runs outside any builtin invocation, meets no sidecar and stays non-fatal. Vision/local-media tools are not web tools and may be withheld by the task contract.
|
||||
|
||||
The existing uploads and skill-output roots resolve through the task's canonical data owner, matching the skill producer even on an isolated child drive. They count as independent image admission before user-home confinement; per-path secret, owner-state, project-store and protected-artifact checks still apply. The common loader copies an admitted image into the task's `uploads/views` before same-round attachment, so both the original output and its retained copy remain available to explicit readers.
|
||||
|
||||
|
|
@ -1453,6 +1453,8 @@ The passive installed-skill projection never launches Betterleaks and never clai
|
|||
|
||||
Every outbound byte derives from the capture; the mutable live payload is neither reread nor rehashed to authorize the transaction, which is what prevents time-of-check/time-of-use drift. Literal Betterleaks `high` findings block the current outbound call; lower or unknown confidence remains a redacted warning. Packaged installs resolve the bundled `betterleaks-standalone`; source checkouts resolve the exact managed runtime installed explicitly with `python -m ouroboros.betterleaks_runtime install` — Publish never downloads it. A top-level `skill_publish` task can be accepted only when pre-truncation metadata contains a validated pull-request receipt for the requested skill and configured Hub repository; the receipt is a narrow veto prerequisite that never manufactures PASS.
|
||||
|
||||
A failed publication envelope names its cause beside the stage: `reason_code` (the stage that failed — `fork_sync_failed` and its siblings), a `repair_hint` chosen by producer evidence (missing CLI, no configured credential, an observed 401/403, a 409 conflict, a timeout), and the transport's sanitized `error_detail` with `github_status`/`github_operation` when observed — the status is read only from gh's own `(HTTP NNN)` suffix, never inferred from prose. Mandatory fork synchronization still stops before any branch, commit or PR mutation, completed stages stay readable, and ambiguous PR settlement remains a read-only exact lookup.
|
||||
|
||||
Owner lifecycle actions share `skill_lifecycle_actions.run_skill_action`: grant/toggle execute their existing effects in the lifecycle lane, local delete uses `skill_uninstall_state`, and attestation keeps its deterministic floor in `skill_owner_attestation`. The host supplies actor identity and checks exact resource/revision plus an existing member chat message, answered quiz or owner-mailbox record when owner intent is needed; the model interprets that source. A generated edit-and-review request confers no grant, attestation, deletion or enable authority. Selected-skill enable calls resolve the actual original owner source when no explicit chat/quiz/mailbox source is supplied; a client allow_enable flag cannot authorize them. The model interprets Repair and run as enable-and-test intent. Presence ceilings precede this source path. A known owner disable in enabled.json newer than that source refuses the old request; load-error reverts and legacy unlabelled snapshots confer no owner attribution. Selected Repair reviews never auto-enable: the model chooses the ordinary toggle_skill call. No permission ledger or HTTP impersonation is added. The configured auto-grant policy remains separate from enablement; explicit owner disable survives review and free replay. `skill_exec` returns the revision captured before its physical script launch, and extension tool receipts carry the descriptor's `content_hash` from the same publication as `extension_generation`, only after physical dispatch; neither is review PASS or a semantic test verdict.
|
||||
|
||||
Marketplace update retains the owner's selected version through repeated retries. `install.PayloadRollbackSnapshot` captures payload/environment and the existing affected lifecycle-state quintet; update and adopt use that same owner. A single restore path verifies required reload before claiming `rolled_back`, preserves independent enablement/history, and never deletes an already restored payload when a state write failed.
|
||||
|
|
|
|||
|
|
@ -177,9 +177,23 @@ integrity and authority boundaries plus truthful receipts; do not add
|
|||
task-specific auto-retry, fallback, cleanup, resume, or terminal-flow state
|
||||
machines.
|
||||
|
||||
A producer that already knows its call failed publishes that fact typed: a
|
||||
`ToolResult` through `tool_result._publish_tool_result`, or a first-line
|
||||
`⚠️ IDENTIFIER` marker the legacy adapter maps to a status. Identifier-less
|
||||
`⚠️ prose` and a bare `ERROR: ...` string are recorded by the registry — in
|
||||
`tools.jsonl`, the outcome classifier and the acceptance packet — as a successful
|
||||
call, so the failure the producer saw is lost exactly where the next decision
|
||||
reads it. A wrapper over an inner producer (`view_image` over the local image
|
||||
loader, the publication transaction over the GitHub transport) carries the inner
|
||||
failure and its safe cause forward instead of a stage-only word.
|
||||
|
||||
Enforcement: the prompt-edit discipline is scored by CHECKLISTS item 13(b)
|
||||
(a prompt edit never restates a tool schema and is never an incident patch);
|
||||
the recoverable-failure boundary has no automated surface — review-only.
|
||||
the recoverable-failure boundary has no automated surface — review-only; the
|
||||
typed-refusal rule is ratcheted by `tests/test_typed_tool_refusals.py`, a
|
||||
source lint over returned literals in `ouroboros/tools/` whose allowlist is the
|
||||
residual disclosure — a failure text that travels through a variable, a tuple
|
||||
or a helper is outside its reach and is pinned by the producer's own tests.
|
||||
|
||||
### Documentation contract
|
||||
|
||||
|
|
@ -811,7 +825,10 @@ separate `plan_task` call containing `review_disposition` only —
|
|||
`{review_fingerprint, items: [{finding_id, decision, rationale}]}` — covering
|
||||
every finding exactly once; duplicates, contradictions, unknown, stale, or
|
||||
incomplete dispositions fail closed, and mixed or vacuous calls fail before an
|
||||
attempt is recorded. Never replay the plan envelope with the disposition.
|
||||
attempt is recorded as typed argument errors — a schema-equivalent empty
|
||||
optional field (`""`, `{}`, a list or dict whose members are all empty) beside a
|
||||
disposition is ignored, never mistaken for a second operation. Never replay the
|
||||
plan envelope with the disposition.
|
||||
Blocking `REVISE_PLAN` requires changed plan text and another panel; advisory
|
||||
may proceed only under loud host disclosure and the agent's rationale.
|
||||
Reviewers are findings-only — they never author a competing plan — a blocking
|
||||
|
|
|
|||
|
|
@ -8,9 +8,11 @@ import re
|
|||
import urllib.parse
|
||||
from typing import Any, Dict, List, Tuple
|
||||
|
||||
from ouroboros.secret_masking import redact_known_values
|
||||
from ouroboros.skill_publish_result import validate_skill_publish_receipt
|
||||
from ouroboros.tools.github import _gh_cmd
|
||||
from ouroboros.tools.github import GhResult, _gh_run, github_cli_configured, github_token_from_env_or_settings
|
||||
from ouroboros.tools.registry import ToolContext
|
||||
from ouroboros.utils import truncate_within_limit
|
||||
|
||||
_HEX_OID_RE = re.compile(r"^(?:[0-9a-fA-F]{40}|[0-9a-fA-F]{64})$")
|
||||
|
||||
|
|
@ -18,11 +20,41 @@ _HEX_OID_RE = re.compile(r"^(?:[0-9a-fA-F]{40}|[0-9a-fA-F]{64})$")
|
|||
class SkillPublishGitHubError(RuntimeError):
|
||||
"""Closed, candidate-free GitHub transport failure."""
|
||||
|
||||
def __init__(self, reason_code: str, repair_hint: str, *, status: str = "partial") -> None:
|
||||
def __init__(self, reason_code: str, repair_hint: str, *, status: str = "partial",
|
||||
detail: str = "", http_status: int | None = None, operation: str = "") -> None:
|
||||
super().__init__(reason_code)
|
||||
self.reason_code = reason_code
|
||||
self.repair_hint = repair_hint
|
||||
self.status = status
|
||||
# Transport errors already fit (600-char head plus prefix). Also bound
|
||||
# malformed success responses before they become diagnostic detail.
|
||||
self.detail = truncate_within_limit(
|
||||
redact_known_values(detail, [github_token_from_env_or_settings()]), 640,
|
||||
) if detail else ""
|
||||
self.http_status = http_status
|
||||
self.operation = operation
|
||||
|
||||
|
||||
def github_repair_hint(result: GhResult, *, operation: str, repository: str,
|
||||
branch: str = "", default: str) -> str:
|
||||
"""One actionable hint from PRODUCER evidence only — the failure class the transport
|
||||
observed and gh's own HTTP marker. It states the fact and names the existing Settings
|
||||
field; it never asserts a cause (a 403 can be a permission OR a rate limit) and never
|
||||
retries: the model reads ``error_detail`` and decides."""
|
||||
if result.failure == "cli_missing":
|
||||
return "Install the GitHub CLI (gh) on this machine, then retry."
|
||||
if not github_cli_configured():
|
||||
return "No GitHub credential is configured; add GITHUB_TOKEN in Settings → Secrets, then retry."
|
||||
where = f"{operation} on {repository}" + (f" (branch {branch})" if branch else "")
|
||||
if result.http_status in (401, 403):
|
||||
return (f"GitHub answered HTTP {result.http_status} for {where}; read error_detail. If the "
|
||||
"token lacks access, update GITHUB_TOKEN in Settings → Secrets, then retry.")
|
||||
if result.http_status == 409:
|
||||
return f"GitHub reported a conflict (HTTP 409) for {where}; resolve it on GitHub, then retry."
|
||||
if result.failure == "timeout":
|
||||
return (f"gh did not finish {where} within its time limit; the outcome may be unknown — "
|
||||
"inspect it on GitHub before retrying.")
|
||||
return default
|
||||
|
||||
|
||||
def _json_value(
|
||||
|
|
@ -30,22 +62,32 @@ def _json_value(
|
|||
args: List[str],
|
||||
*,
|
||||
reason_code: str,
|
||||
operation: str,
|
||||
repository: str,
|
||||
branch: str = "",
|
||||
timeout: int = 30,
|
||||
input_data: str | None = None,
|
||||
object_required: bool = False,
|
||||
) -> Any:
|
||||
raw = _gh_cmd(args, ctx, timeout=timeout, input_data=input_data)
|
||||
if raw.startswith("⚠️"):
|
||||
raise SkillPublishGitHubError(
|
||||
reason_code,
|
||||
"Inspect GitHub connectivity and repository access, then retry.",
|
||||
)
|
||||
try:
|
||||
return json.loads(raw) if raw else {}
|
||||
except json.JSONDecodeError as exc:
|
||||
raise SkillPublishGitHubError(
|
||||
reason_code,
|
||||
"Inspect GitHub connectivity and repository access, then retry.",
|
||||
) from exc
|
||||
result = _gh_run(args, ctx, timeout=timeout, input_data=input_data)
|
||||
if result.ok:
|
||||
try:
|
||||
data = json.loads(result.text) if result.text else {}
|
||||
except json.JSONDecodeError:
|
||||
pass
|
||||
else:
|
||||
if not object_required or isinstance(data, dict):
|
||||
return data
|
||||
# gh exited 0 but the body is not the expected shape: a parser fact, not a
|
||||
# connectivity guess. The bounded, redacted body rides as error_detail.
|
||||
hint = (f"GitHub answered {operation} on {repository}, but not with the expected JSON "
|
||||
f"{'object' if object_required else 'value'}; read error_detail.")
|
||||
else:
|
||||
hint = github_repair_hint(result, operation=operation, repository=repository, branch=branch,
|
||||
default="Inspect GitHub connectivity and repository access, then retry.")
|
||||
raise SkillPublishGitHubError(
|
||||
reason_code, hint, detail=result.text, http_status=result.http_status, operation=operation,
|
||||
)
|
||||
|
||||
|
||||
def _json_object(
|
||||
|
|
@ -53,31 +95,35 @@ def _json_object(
|
|||
args: List[str],
|
||||
*,
|
||||
reason_code: str,
|
||||
operation: str,
|
||||
repository: str,
|
||||
branch: str = "",
|
||||
timeout: int = 30,
|
||||
input_data: str | None = None,
|
||||
) -> Dict[str, Any]:
|
||||
data = _json_value(
|
||||
return _json_value(
|
||||
ctx,
|
||||
args,
|
||||
reason_code=reason_code,
|
||||
operation=operation,
|
||||
repository=repository,
|
||||
branch=branch,
|
||||
timeout=timeout,
|
||||
input_data=input_data,
|
||||
object_required=True,
|
||||
)
|
||||
if not isinstance(data, dict):
|
||||
raise SkillPublishGitHubError(
|
||||
reason_code,
|
||||
"Inspect GitHub connectivity and repository access, then retry.",
|
||||
)
|
||||
return data
|
||||
|
||||
|
||||
def github_login(ctx: ToolContext) -> str:
|
||||
raw = _gh_cmd(["api", "/user", "--jq", ".login"], ctx).strip()
|
||||
if raw.startswith("⚠️") or not raw or len(raw) > 80:
|
||||
result = _gh_run(["api", "/user", "--jq", ".login"], ctx)
|
||||
raw = result.text.strip()
|
||||
if not result.ok or not raw or len(raw) > 80:
|
||||
raise SkillPublishGitHubError(
|
||||
"github_actor_unavailable",
|
||||
"Repair GitHub authentication, then retry.",
|
||||
github_repair_hint(result, operation="user", repository="<account>",
|
||||
default="Repair GitHub authentication, then retry."),
|
||||
status="blocked",
|
||||
detail=result.text, http_status=result.http_status, operation="user",
|
||||
)
|
||||
return raw
|
||||
|
||||
|
|
@ -87,17 +133,20 @@ def fetch_upstream_catalog(ctx: ToolContext, owner: str, repo: str, base_branch:
|
|||
ctx,
|
||||
["api", f"/repos/{owner}/{repo}/git/refs/heads/{base_branch}"],
|
||||
reason_code="upstream_read_failed",
|
||||
operation="git/refs", repository=f"{owner}/{repo}", branch=base_branch,
|
||||
)
|
||||
base_sha = str((ref.get("object") or {}).get("sha") or "")
|
||||
if not _HEX_OID_RE.fullmatch(base_sha):
|
||||
raise SkillPublishGitHubError(
|
||||
"upstream_read_failed",
|
||||
"Inspect the configured Hub base branch, then retry.",
|
||||
detail=json.dumps(ref), operation="git/refs",
|
||||
)
|
||||
content = _json_object(
|
||||
ctx,
|
||||
["api", f"/repos/{owner}/{repo}/contents/catalog.json?ref={base_sha}"],
|
||||
reason_code="upstream_read_failed",
|
||||
operation="contents", repository=f"{owner}/{repo}", branch=base_branch,
|
||||
)
|
||||
try:
|
||||
catalog_bytes = base64.b64decode(str(content.get("content") or ""))
|
||||
|
|
@ -106,11 +155,13 @@ def fetch_upstream_catalog(ctx: ToolContext, owner: str, repo: str, base_branch:
|
|||
raise SkillPublishGitHubError(
|
||||
"upstream_catalog_invalid",
|
||||
"Repair the upstream Hub catalog, then retry.",
|
||||
operation="contents",
|
||||
) from exc
|
||||
if not isinstance(catalog, dict):
|
||||
raise SkillPublishGitHubError(
|
||||
"upstream_catalog_invalid",
|
||||
"Repair the upstream Hub catalog, then retry.",
|
||||
operation="contents",
|
||||
)
|
||||
return catalog, base_sha.lower()
|
||||
|
||||
|
|
@ -128,20 +179,22 @@ def prepare_publish_repository(
|
|||
if login.casefold() == owner.casefold():
|
||||
attempt.mark("fork_ready", repository=repository, actor=login)
|
||||
return
|
||||
existing = _gh_cmd(["repo", "view", repository, "--json", "name"], ctx)
|
||||
if existing.startswith("⚠️"):
|
||||
created = _gh_cmd(
|
||||
existing = _gh_run(["repo", "view", repository, "--json", "name"], ctx)
|
||||
if not existing.ok:
|
||||
created = _gh_run(
|
||||
["repo", "fork", f"{owner}/{repo}", "--clone=false"],
|
||||
ctx,
|
||||
timeout=60,
|
||||
)
|
||||
if created.startswith("⚠️"):
|
||||
if not created.ok:
|
||||
raise SkillPublishGitHubError(
|
||||
"fork_prepare_failed",
|
||||
"Repair the GitHub fork, then retry.",
|
||||
github_repair_hint(created, operation="repo fork", repository=f"{owner}/{repo}",
|
||||
default="Repair the GitHub fork, then retry."),
|
||||
detail=created.text, http_status=created.http_status, operation="repo fork",
|
||||
)
|
||||
attempt.mark("fork_ready", repository=repository, actor=login)
|
||||
merged = _gh_cmd(
|
||||
merged = _gh_run(
|
||||
[
|
||||
"api",
|
||||
"-X",
|
||||
|
|
@ -153,23 +206,26 @@ def prepare_publish_repository(
|
|||
ctx,
|
||||
timeout=45,
|
||||
)
|
||||
if merged.startswith("⚠️"):
|
||||
if not merged.ok:
|
||||
raise SkillPublishGitHubError(
|
||||
"fork_sync_failed",
|
||||
"Repair or synchronize the GitHub fork, then retry.",
|
||||
github_repair_hint(merged, operation="merge-upstream", repository=repository, branch=base_branch,
|
||||
default="Repair or synchronize the GitHub fork, then retry."),
|
||||
detail=merged.text, http_status=merged.http_status, operation="merge-upstream",
|
||||
)
|
||||
attempt.mark("fork_synced", repository=repository, actor=login)
|
||||
|
||||
|
||||
def ensure_branch(ctx: ToolContext, login: str, repo: str, branch: str, base_sha: str) -> str:
|
||||
existing = _gh_cmd(
|
||||
existing = _gh_run(
|
||||
["api", f"/repos/{login}/{repo}/git/ref/heads/{branch}"],
|
||||
ctx,
|
||||
)
|
||||
if not existing.startswith("⚠️"):
|
||||
if existing.ok:
|
||||
raise SkillPublishGitHubError(
|
||||
"submission_branch_exists",
|
||||
"Remove the old submission branch or bump the skill version, then retry.",
|
||||
detail=existing.text, http_status=existing.http_status, operation="git/ref",
|
||||
)
|
||||
created = _json_object(
|
||||
ctx,
|
||||
|
|
@ -184,12 +240,14 @@ def ensure_branch(ctx: ToolContext, login: str, repo: str, branch: str, base_sha
|
|||
f"sha={base_sha}",
|
||||
],
|
||||
reason_code="branch_create_failed",
|
||||
operation="git/refs", repository=f"{login}/{repo}", branch=branch,
|
||||
)
|
||||
branch_sha = str((created.get("object") or {}).get("sha") or "")
|
||||
if not _HEX_OID_RE.fullmatch(branch_sha):
|
||||
raise SkillPublishGitHubError(
|
||||
"branch_create_failed",
|
||||
"Inspect the GitHub submission branch, then retry.",
|
||||
detail=json.dumps(created), operation="git/refs",
|
||||
)
|
||||
return branch_sha.lower()
|
||||
|
||||
|
|
@ -237,11 +295,13 @@ mutation($input: CreateCommitOnBranchInput!) {
|
|||
timeout=60,
|
||||
input_data=json.dumps(payload),
|
||||
reason_code="commit_create_failed",
|
||||
operation="graphql", repository=f"{login}/{repo}", branch=branch,
|
||||
)
|
||||
if result.get("errors"):
|
||||
raise SkillPublishGitHubError(
|
||||
"commit_create_failed",
|
||||
"Inspect the GitHub submission branch, then retry.",
|
||||
detail=json.dumps(result), operation="graphql",
|
||||
)
|
||||
commit = ((result.get("data") or {}).get("createCommitOnBranch") or {}).get("commit") or {}
|
||||
commit_sha = str(commit.get("oid") or "")
|
||||
|
|
@ -250,6 +310,7 @@ mutation($input: CreateCommitOnBranchInput!) {
|
|||
raise SkillPublishGitHubError(
|
||||
"commit_create_failed",
|
||||
"Inspect the GitHub submission branch, then retry.",
|
||||
detail=json.dumps(result), operation="graphql",
|
||||
)
|
||||
return commit_sha.lower(), commit_url[:360]
|
||||
|
||||
|
|
@ -301,28 +362,26 @@ def _lookup_open_pr_receipt(
|
|||
snapshot_hash: str,
|
||||
ruleset_sha256: str,
|
||||
) -> Dict[str, Any] | None:
|
||||
try:
|
||||
rows = _json_value(
|
||||
ctx,
|
||||
[
|
||||
"api",
|
||||
"--method",
|
||||
"GET",
|
||||
f"/repos/{owner}/{repo}/pulls",
|
||||
"-f",
|
||||
"state=open",
|
||||
"-f",
|
||||
f"head={login}:{branch}",
|
||||
"-f",
|
||||
f"base={base_branch}",
|
||||
"-f",
|
||||
"per_page=100",
|
||||
],
|
||||
reason_code="pr_open_indeterminate",
|
||||
timeout=30,
|
||||
)
|
||||
except SkillPublishGitHubError:
|
||||
return None
|
||||
rows = _json_value(
|
||||
ctx,
|
||||
[
|
||||
"api",
|
||||
"--method",
|
||||
"GET",
|
||||
f"/repos/{owner}/{repo}/pulls",
|
||||
"-f",
|
||||
"state=open",
|
||||
"-f",
|
||||
f"head={login}:{branch}",
|
||||
"-f",
|
||||
f"base={base_branch}",
|
||||
"-f",
|
||||
"per_page=100",
|
||||
],
|
||||
reason_code="pr_open_indeterminate",
|
||||
operation="pulls", repository=f"{owner}/{repo}", branch=branch,
|
||||
timeout=30,
|
||||
)
|
||||
if not isinstance(rows, list) or len(rows) != 1 or not isinstance(rows[0], dict):
|
||||
return None
|
||||
row = rows[0]
|
||||
|
|
@ -368,7 +427,7 @@ def create_pr_receipt(
|
|||
branch=branch,
|
||||
commit_sha=commit_sha,
|
||||
)
|
||||
raw = _gh_cmd(
|
||||
result = _gh_run(
|
||||
[
|
||||
"pr",
|
||||
"create",
|
||||
|
|
@ -388,9 +447,9 @@ def create_pr_receipt(
|
|||
input_data=body,
|
||||
)
|
||||
direct = None
|
||||
if not raw.startswith("⚠️"):
|
||||
if result.ok:
|
||||
direct = _receipt_from_url(
|
||||
raw,
|
||||
result.text,
|
||||
repository=repository,
|
||||
skill=attempt.skill,
|
||||
snapshot_hash=attempt.snapshot_hash,
|
||||
|
|
@ -398,18 +457,32 @@ def create_pr_receipt(
|
|||
)
|
||||
if direct is not None:
|
||||
return direct
|
||||
return _lookup_open_pr_receipt(
|
||||
ctx,
|
||||
owner=owner,
|
||||
repo=repo,
|
||||
base_branch=base_branch,
|
||||
login=login,
|
||||
branch=branch,
|
||||
commit_sha=commit_sha,
|
||||
skill=attempt.skill,
|
||||
snapshot_hash=attempt.snapshot_hash,
|
||||
ruleset_sha256=str(attempt.scanner.get("ruleset_sha256") or ""),
|
||||
)
|
||||
try:
|
||||
settled = _lookup_open_pr_receipt(
|
||||
ctx,
|
||||
owner=owner,
|
||||
repo=repo,
|
||||
base_branch=base_branch,
|
||||
login=login,
|
||||
branch=branch,
|
||||
commit_sha=commit_sha,
|
||||
skill=attempt.skill,
|
||||
snapshot_hash=attempt.snapshot_hash,
|
||||
ruleset_sha256=str(attempt.scanner.get("ruleset_sha256") or ""),
|
||||
)
|
||||
except SkillPublishGitHubError:
|
||||
if result.ok:
|
||||
raise
|
||||
# Retain the mutator's evidence if the read-only settlement also failed.
|
||||
settled = None
|
||||
if settled is None and not result.ok:
|
||||
raise SkillPublishGitHubError(
|
||||
"pr_open_indeterminate",
|
||||
github_repair_hint(result, operation="pr create", repository=repository, branch=branch,
|
||||
default="Inspect the recorded branch and commit before deciding whether to retry."),
|
||||
detail=result.text, http_status=result.http_status, operation="pr create",
|
||||
)
|
||||
return settled
|
||||
|
||||
|
||||
__all__ = [
|
||||
|
|
@ -419,5 +492,6 @@ __all__ = [
|
|||
"ensure_branch",
|
||||
"fetch_upstream_catalog",
|
||||
"github_login",
|
||||
"github_repair_hint",
|
||||
"prepare_publish_repository",
|
||||
]
|
||||
|
|
|
|||
|
|
@ -491,6 +491,13 @@ def extract_skill_publish_result_metadata(result: Any) -> Dict[str, Any]:
|
|||
field="audited_false_positive_count",
|
||||
),
|
||||
}
|
||||
# The GitHub cause the transport observed rides beside the stage, only when present.
|
||||
for key, limit in (("error_detail", 640), ("github_operation", 64)):
|
||||
if payload.get(key):
|
||||
attempt[key] = _bounded_text(payload.get(key), limit)
|
||||
github_status = payload.get("github_status")
|
||||
if isinstance(github_status, int) and not isinstance(github_status, bool):
|
||||
attempt["github_status"] = github_status
|
||||
metadata: Dict[str, Any] = {"skill_publish_attempt": attempt}
|
||||
receipt = payload.get("receipt")
|
||||
valid_receipt = validate_skill_publish_receipt(
|
||||
|
|
|
|||
|
|
@ -6,16 +6,41 @@ import json
|
|||
import logging
|
||||
import os
|
||||
import pathlib
|
||||
import re
|
||||
import subprocess
|
||||
from dataclasses import dataclass
|
||||
from typing import List, Optional
|
||||
|
||||
from ouroboros.secret_masking import redact_known_values
|
||||
from ouroboros.tools.registry import ToolContext, ToolEntry
|
||||
from ouroboros.tools.tool_result import ToolResult, _publish_tool_result
|
||||
from ouroboros.utils import truncate_within_limit
|
||||
from ouroboros.utils import truncate_review_artifact as _truncate_with_notice
|
||||
|
||||
log = logging.getLogger(__name__)
|
||||
_GENERIC_TRANSPORT = object()
|
||||
|
||||
|
||||
@dataclass(frozen=True)
|
||||
class GhResult:
|
||||
ok: bool
|
||||
text: str
|
||||
exit_code: int | None
|
||||
http_status: int | None
|
||||
# "target" is a local refusal, not a subprocess exit or exception.
|
||||
failure: str
|
||||
|
||||
|
||||
def _refuse(ctx: ToolContext, text: str, code: str = "TOOL_ARG_ERROR") -> str:
|
||||
"""Publish a refusal this module AUTHORS as a typed result; text unchanged.
|
||||
|
||||
The registry types a string result by its first-line ``⚠️ IDENTIFIER`` marker,
|
||||
so prose such as ``⚠️ issue number must be positive`` was recorded ``status=ok``
|
||||
although the producer already knew it had refused. Both codes carry
|
||||
``status="error"``."""
|
||||
return _publish_tool_result(ctx, ToolResult(status="error", code=code, text=text))
|
||||
|
||||
|
||||
def github_token_from_env_or_settings() -> str:
|
||||
from ouroboros.config import load_settings
|
||||
token = os.environ.get("GITHUB_TOKEN") or os.environ.get("GH_TOKEN") or ""
|
||||
|
|
@ -61,15 +86,17 @@ def github_cli_configured() -> bool:
|
|||
return False
|
||||
|
||||
|
||||
def _gh_cmd(args: List[str], ctx: ToolContext, timeout: int = 30, input_data: Optional[str] = None,
|
||||
*, repo: object = _GENERIC_TRANSPORT) -> str:
|
||||
def _gh_run(args: List[str], ctx: ToolContext, timeout: int = 30, input_data: Optional[str] = None,
|
||||
*, repo: object = _GENERIC_TRANSPORT) -> GhResult:
|
||||
# Only omitted internal API/Hub calls keep the generic transport contract.
|
||||
# Public repository tools always pass repo, including '' for Project focus.
|
||||
# The target refusals below publish a typed argument error into the calling
|
||||
# tool's sidecar; the publication transport omits `repo`, so it can never
|
||||
# reach them and its own final result is never shadowed from here.
|
||||
if repo is not _GENERIC_TRANSPORT and not isinstance(repo, str):
|
||||
return _publish_tool_result(ctx, ToolResult(
|
||||
status="error", code="TOOL_ARG_ERROR",
|
||||
text="⚠️ GH_TARGET_INVALID: repo must be a string; omit it to use the selected Project.",
|
||||
))
|
||||
return GhResult(False, _refuse(
|
||||
ctx, "⚠️ GH_TARGET_INVALID: repo must be a string; omit it to use the selected Project."),
|
||||
None, None, "target")
|
||||
try:
|
||||
cwd, env = pathlib.Path(ctx.repo_dir), _gh_env(ctx)
|
||||
cmd = ["gh", *args]
|
||||
|
|
@ -85,16 +112,19 @@ def _gh_cmd(args: List[str], ctx: ToolContext, timeout: int = 30, input_data: Op
|
|||
note = str(metadata.get("_project_room_note") or "")
|
||||
selected = workspace or room_dir
|
||||
if note or (selected and not pathlib.Path(selected).is_dir()):
|
||||
return f"⚠️ GH_TARGET_UNAVAILABLE: {note or 'The selected Project directory is unavailable.'}"
|
||||
return GhResult(False,
|
||||
f"⚠️ GH_TARGET_UNAVAILABLE: {note or 'The selected Project directory is unavailable.'}",
|
||||
None, None, "target")
|
||||
if project and not selected:
|
||||
return _publish_tool_result(ctx, ToolResult(
|
||||
status="error", code="TOOL_ARG_ERROR",
|
||||
text="⚠️ GH_TARGET_REQUIRED: this Project has no repository directory; pass repo='[HOST/]OWNER/REPO'.",
|
||||
))
|
||||
return GhResult(False, _refuse(
|
||||
ctx, "⚠️ GH_TARGET_REQUIRED: this Project has no repository directory; pass repo='[HOST/]OWNER/REPO'."),
|
||||
None, None, "target")
|
||||
binding = build_resolved_resource_binding(ctx, operation="shell", process_cwd="")
|
||||
cwd = binding.target_path
|
||||
if workspace and cwd != pathlib.Path(workspace).resolve(strict=False):
|
||||
return "⚠️ GH_TARGET_UNAVAILABLE: the task's Project binding could not be resolved."
|
||||
return GhResult(False,
|
||||
"⚠️ GH_TARGET_UNAVAILABLE: the task's Project binding could not be resolved.",
|
||||
None, None, "target")
|
||||
if workspace or room_dir or project:
|
||||
env.pop("GH_REPO", None) # Ambient defaults cannot replace the selected Project.
|
||||
if repo:
|
||||
|
|
@ -109,15 +139,30 @@ def _gh_cmd(args: List[str], ctx: ToolContext, timeout: int = 30, input_data: Op
|
|||
env=env,
|
||||
)
|
||||
if res.returncode != 0:
|
||||
err = (res.stderr or "").strip()
|
||||
return f"⚠️ GH_ERROR: {err.split(chr(10))[0][:200]}"
|
||||
return res.stdout.strip()
|
||||
# Redact the WHOLE stderr first (a cut could split a token), read gh's own
|
||||
# ``(HTTP NNN)`` marker before any bounding, then keep a bounded head.
|
||||
err = redact_known_values(res.stderr or "", [github_token_from_env_or_settings()])
|
||||
status = re.search(r"\(HTTP (\d{3})\)", err)
|
||||
head = " | ".join([line.strip() for line in err.splitlines() if line.strip()][:3])
|
||||
head = truncate_within_limit(head, 600)
|
||||
return GhResult(False, "⚠️ GH_ERROR: " + head, res.returncode,
|
||||
int(status.group(1)) if status else None, "exit")
|
||||
return GhResult(True, res.stdout.strip(), res.returncode, None, "")
|
||||
except FileNotFoundError:
|
||||
return "⚠️ GH_ERROR: `gh` CLI not found. Install GitHub CLI and ensure it is on PATH (https://cli.github.com/)"
|
||||
return GhResult(False,
|
||||
"⚠️ GH_ERROR: `gh` CLI not found. Install GitHub CLI and ensure it is on PATH (https://cli.github.com/)",
|
||||
None, None, "cli_missing")
|
||||
except subprocess.TimeoutExpired:
|
||||
return f"⚠️ GH_TIMEOUT: exceeded {timeout}s."
|
||||
return GhResult(False, f"⚠️ GH_TIMEOUT: exceeded {timeout}s.", None, None, "timeout")
|
||||
except Exception as e:
|
||||
return f"⚠️ GH_ERROR: {e}"
|
||||
detail = truncate_within_limit(redact_known_values(str(e), [github_token_from_env_or_settings()]), 600)
|
||||
return GhResult(False, f"⚠️ GH_ERROR: {detail}", None, None, "exception")
|
||||
|
||||
|
||||
def _gh_cmd(args: List[str], ctx: ToolContext, timeout: int = 30, input_data: Optional[str] = None,
|
||||
*, repo: object = _GENERIC_TRANSPORT) -> str:
|
||||
return _gh_run(args, ctx, timeout=timeout, input_data=input_data, repo=repo).text
|
||||
|
||||
|
||||
def _list_issues(ctx: ToolContext, state: str = "open", labels: str = "", limit: int = 20, repo: str = "") -> str:
|
||||
args = [
|
||||
|
|
@ -136,7 +181,7 @@ def _list_issues(ctx: ToolContext, state: str = "open", labels: str = "", limit:
|
|||
try:
|
||||
issues = json.loads(raw)
|
||||
except json.JSONDecodeError:
|
||||
return f"⚠️ Failed to parse issues JSON: {raw[:500]}"
|
||||
return _refuse(ctx, f"⚠️ TOOL_ERROR: failed to parse issues JSON: {raw[:500]}", "TOOL_ERROR")
|
||||
|
||||
if not issues:
|
||||
return f"No {state} issues found."
|
||||
|
|
@ -159,7 +204,7 @@ def _list_issues(ctx: ToolContext, state: str = "open", labels: str = "", limit:
|
|||
|
||||
def _get_issue(ctx: ToolContext, number: int, repo: str = "") -> str:
|
||||
if number <= 0:
|
||||
return "⚠️ issue number must be positive"
|
||||
return _refuse(ctx, "⚠️ TOOL_ARG_ERROR: issue number must be positive")
|
||||
|
||||
args = [
|
||||
"issue", "view", str(number),
|
||||
|
|
@ -173,7 +218,7 @@ def _get_issue(ctx: ToolContext, number: int, repo: str = "") -> str:
|
|||
try:
|
||||
issue = json.loads(raw)
|
||||
except json.JSONDecodeError:
|
||||
return f"⚠️ Failed to parse issue JSON: {raw[:500]}"
|
||||
return _refuse(ctx, f"⚠️ TOOL_ERROR: failed to parse issue JSON: {raw[:500]}", "TOOL_ERROR")
|
||||
|
||||
labels_str = ", ".join(l.get("name", "") for l in issue.get("labels", []))
|
||||
author = issue.get("author", {}).get("login", "unknown")
|
||||
|
|
@ -203,10 +248,10 @@ def _get_issue(ctx: ToolContext, number: int, repo: str = "") -> str:
|
|||
|
||||
def _comment_on_issue(ctx: ToolContext, number: int, body: str, repo: str = "") -> str:
|
||||
if number <= 0:
|
||||
return "⚠️ issue number must be positive"
|
||||
return _refuse(ctx, "⚠️ TOOL_ARG_ERROR: issue number must be positive")
|
||||
|
||||
if not body or not body.strip():
|
||||
return "⚠️ Comment body cannot be empty."
|
||||
return _refuse(ctx, "⚠️ TOOL_ARG_ERROR: comment body cannot be empty.")
|
||||
|
||||
args = ["issue", "comment", str(number), "--body-file", "-"]
|
||||
raw = _gh_cmd(args, ctx, input_data=body, repo=repo)
|
||||
|
|
@ -217,7 +262,7 @@ def _comment_on_issue(ctx: ToolContext, number: int, body: str, repo: str = "")
|
|||
|
||||
def _close_issue(ctx: ToolContext, number: int, comment: str = "", repo: str = "") -> str:
|
||||
if number <= 0:
|
||||
return "⚠️ issue number must be positive"
|
||||
return _refuse(ctx, "⚠️ TOOL_ARG_ERROR: issue number must be positive")
|
||||
|
||||
if comment and comment.strip():
|
||||
result = _comment_on_issue(ctx, number, comment, repo=repo)
|
||||
|
|
@ -244,7 +289,7 @@ def _list_prs(ctx: ToolContext, state: str = "open", limit: int = 20, repo: str
|
|||
try:
|
||||
prs = json.loads(raw)
|
||||
except json.JSONDecodeError:
|
||||
return f"⚠️ Failed to parse PRs JSON: {raw[:500]}"
|
||||
return _refuse(ctx, f"⚠️ TOOL_ERROR: failed to parse PRs JSON: {raw[:500]}", "TOOL_ERROR")
|
||||
|
||||
if not prs:
|
||||
return f"No {state} pull requests found."
|
||||
|
|
@ -268,7 +313,7 @@ def _list_prs(ctx: ToolContext, state: str = "open", limit: int = 20, repo: str
|
|||
|
||||
def _get_pr(ctx: ToolContext, number: int, repo: str = "") -> str:
|
||||
if number <= 0:
|
||||
return "⚠️ PR number must be positive."
|
||||
return _refuse(ctx, "⚠️ TOOL_ARG_ERROR: PR number must be positive.")
|
||||
|
||||
meta_args = [
|
||||
"pr", "view", str(number),
|
||||
|
|
@ -283,7 +328,7 @@ def _get_pr(ctx: ToolContext, number: int, repo: str = "") -> str:
|
|||
try:
|
||||
pr = json.loads(raw)
|
||||
except json.JSONDecodeError:
|
||||
return f"⚠️ Failed to parse PR JSON: {raw[:500]}"
|
||||
return _refuse(ctx, f"⚠️ TOOL_ERROR: failed to parse PR JSON: {raw[:500]}", "TOOL_ERROR")
|
||||
|
||||
author = pr.get("author", {}).get("login", "unknown")
|
||||
head_repo = (pr.get("headRepository") or {}).get("nameWithOwner", "?")
|
||||
|
|
@ -373,9 +418,9 @@ def _get_pr(ctx: ToolContext, number: int, repo: str = "") -> str:
|
|||
|
||||
def _comment_on_pr(ctx: ToolContext, number: int, body: str, repo: str = "") -> str:
|
||||
if number <= 0:
|
||||
return "⚠️ PR number must be positive."
|
||||
return _refuse(ctx, "⚠️ TOOL_ARG_ERROR: PR number must be positive.")
|
||||
if not (body or "").strip():
|
||||
return "⚠️ Comment body cannot be empty."
|
||||
return _refuse(ctx, "⚠️ TOOL_ARG_ERROR: comment body cannot be empty.")
|
||||
|
||||
args = ["pr", "comment", str(number), "--body-file", "-"]
|
||||
raw = _gh_cmd(args, ctx, input_data=body, repo=repo)
|
||||
|
|
@ -386,7 +431,7 @@ def _comment_on_pr(ctx: ToolContext, number: int, body: str, repo: str = "") ->
|
|||
|
||||
def _create_issue(ctx: ToolContext, title: str, body: str = "", labels: str = "", repo: str = "") -> str:
|
||||
if not title or not title.strip():
|
||||
return "⚠️ Issue title cannot be empty."
|
||||
return _refuse(ctx, "⚠️ TOOL_ARG_ERROR: issue title cannot be empty.")
|
||||
|
||||
args = ["issue", "create", f"--title={title}"]
|
||||
if body:
|
||||
|
|
|
|||
|
|
@ -91,6 +91,7 @@ from ouroboros.tools.review_helpers import review_wave_binding_fence, review_wav
|
|||
from ouroboros.tools.review_synthesis import (
|
||||
PLAN_REVIEW_CONTROL_PREFIX,
|
||||
)
|
||||
from ouroboros.tools.tool_result import TOOL_CODE_SPECS, ToolResult, _publish_tool_result
|
||||
from ouroboros.utils import truncate_review_artifact, utc_now_iso
|
||||
|
||||
log = logging.getLogger(__name__)
|
||||
|
|
@ -267,30 +268,58 @@ def get_tools():
|
|||
# --------------------------------------------------------------------------- handler
|
||||
|
||||
|
||||
_SPEC_FIELDS = frozenset(_SPEC_SCHEMA["properties"])
|
||||
|
||||
|
||||
def _vacuous(name: str, value: object) -> bool:
|
||||
"""Nothing was said in this optional envelope field: absent, blank prose, or the
|
||||
spec's DECLARED keys each holding their schema-default empty value. A non-empty
|
||||
list, an unknown key or a wrong type is meaning and reaches the existing refusal."""
|
||||
if value is None:
|
||||
return True
|
||||
if name == "spec":
|
||||
return (isinstance(value, dict) and set(value) <= _SPEC_FIELDS
|
||||
and all(member in (None, "", []) for member in value.values()))
|
||||
return isinstance(value, str) and not value.strip()
|
||||
|
||||
|
||||
def _vacuous_disposition(value: object) -> bool:
|
||||
"""A schema-shaped but empty disposition (models fill optional objects with defaults)."""
|
||||
"""A schema-shaped but empty disposition (models fill optional objects with defaults).
|
||||
An UNKNOWN key or a non-empty items list is never vacuous: refused, not ignored."""
|
||||
if not isinstance(value, dict) or set(value) - {"review_fingerprint", "items"}:
|
||||
return False
|
||||
return not str(value.get("review_fingerprint") or "").strip() and not value.get("items")
|
||||
|
||||
|
||||
def _typed_refusal(ctx: ToolContext, code: str, text: str) -> str:
|
||||
"""Publish a refusal the producer ALREADY knows about (D02). The text ABI is
|
||||
unchanged; only the registry-visible status stops reading as a successful call."""
|
||||
return _publish_tool_result(ctx, ToolResult(status=TOOL_CODE_SPECS[code].status, code=code, text=text))
|
||||
|
||||
|
||||
def _handle_plan_task(ctx: ToolContext, **params) -> str:
|
||||
raw_disposition = params.get("review_disposition")
|
||||
envelope_fields = sorted(set(params) - {"review_disposition"})
|
||||
# The registry refuses unknown params; a vacuous envelope field carries no plan.
|
||||
envelope_fields = [k for k in ("goal", "plan", "spec") if not _vacuous(k, params.get(k))]
|
||||
if raw_disposition is not None and not _vacuous_disposition(raw_disposition):
|
||||
if envelope_fields:
|
||||
return (
|
||||
return _typed_refusal(
|
||||
ctx, "TOOL_ARG_ERROR",
|
||||
"ERROR: PLAN_REVIEW_DISPOSITION_MIXED_ENVELOPE: disposition mode accepts "
|
||||
"review_disposition only; a changed plan needs a new review-mode call "
|
||||
"without review_disposition. No plan attempt was recorded."
|
||||
"without review_disposition. No plan attempt was recorded.",
|
||||
)
|
||||
if not isinstance(raw_disposition, dict):
|
||||
return "ERROR: PLAN_REVIEW_DISPOSITION_INVALID: review_disposition must be an object"
|
||||
return _typed_refusal(
|
||||
ctx, "TOOL_ARG_ERROR",
|
||||
"ERROR: PLAN_REVIEW_DISPOSITION_INVALID: review_disposition must be an object",
|
||||
)
|
||||
return _apply_disposition(ctx, raw_disposition)
|
||||
if "review_disposition" in params and not envelope_fields:
|
||||
return (
|
||||
return _typed_refusal(
|
||||
ctx, "TOOL_ARG_ERROR",
|
||||
"ERROR: PLAN_REVIEW_DISPOSITION_EMPTY: submit goal, plan and spec for review "
|
||||
"mode, or a complete review_disposition as the only field. No plan attempt was recorded."
|
||||
"mode, or a complete review_disposition as the only field. No plan attempt was recorded.",
|
||||
)
|
||||
request = _PlanRequest(
|
||||
goal=str(params.get("goal") or ""), plan=str(params.get("plan") or ""), spec=params.get("spec"),
|
||||
|
|
@ -320,15 +349,22 @@ def _handle_plan_task(ctx: ToolContext, **params) -> str:
|
|||
return _plan_unavailable(ctx, f"ERROR: Plan review failed: {e}", "review_failed")
|
||||
|
||||
|
||||
# A FAULT of this call (broken review, unreadable authority); every other reason — budget,
|
||||
# configuration, context — is an availability outcome typed `unavailable`, never a fake success.
|
||||
_PLAN_FAULT_REASONS = frozenset({"review_failed", "plan_review_exact_artifact_unavailable", "plan_review_custody_invalid"})
|
||||
|
||||
|
||||
def _plan_unavailable(ctx: ToolContext, message: str, reason: str) -> str:
|
||||
"""Persist a retryable availability outcome (the current fingerprint stays open-unavailable)."""
|
||||
code = "TOOL_ERROR" if reason in _PLAN_FAULT_REASONS else "CAPABILITY_UNAVAILABLE"
|
||||
try:
|
||||
root, task_id = _planning_state_location(ctx)
|
||||
state = mark_current_plan_review_unavailable(root, task_id, reason=reason)
|
||||
_emit_plan_review_reference(ctx, task_id, state, state_root=root)
|
||||
except (OSError, TimeoutError, ValueError) as exc:
|
||||
return f"{message}\nERROR: PLAN_REVIEW_STATE_PERSIST_FAILED: {exc}"
|
||||
return message
|
||||
return _typed_refusal(
|
||||
ctx, "TOOL_ERROR", f"{message}\nERROR: PLAN_REVIEW_STATE_PERSIST_FAILED: {exc}")
|
||||
return _typed_refusal(ctx, code, message)
|
||||
|
||||
|
||||
def _planning_state_location(ctx: ToolContext) -> tuple[pathlib.Path, str]:
|
||||
|
|
@ -433,12 +469,13 @@ def _prepare_plan_inputs(ctx: ToolContext, request: "_PlanRequest", state_root:
|
|||
if not request.plan.strip():
|
||||
errors = ["plan: required non-empty prose", *errors]
|
||||
if errors:
|
||||
return {"error": "ERROR: PLAN_SPEC_INVALID: " + "; ".join(errors) + ". No reviewer was called."}
|
||||
return {"error": "ERROR: PLAN_SPEC_INVALID: " + "; ".join(errors) + ". No reviewer was called.",
|
||||
"code": "TOOL_ARG_ERROR"}
|
||||
from ouroboros.review_substrate import review_repo_dirs_for
|
||||
try:
|
||||
system_root, active_root = review_repo_dirs_for(ctx)
|
||||
except ValueError as exc:
|
||||
return {"error": f"ERROR: PLAN_SUBJECT_ROOT_INVALID: {exc}"}
|
||||
return {"error": f"ERROR: PLAN_SUBJECT_ROOT_INVALID: {exc}", "code": "TOOL_ERROR"}
|
||||
locators = list(spec["affected_resources"]) + list(spec["evidence"])
|
||||
constitutional, constitutional_note = plan_spec.resolve_constitutional(
|
||||
active_root=active_root, system_repo_root=system_root,
|
||||
|
|
@ -454,7 +491,7 @@ def _prepare_plan_inputs(ctx: ToolContext, request: "_PlanRequest", state_root:
|
|||
try:
|
||||
reviewer_requested, request_dropped = _reviewer_requested_locators(ctx, state_root)
|
||||
except ValueError as exc:
|
||||
return {"error": f"ERROR: {exc}"}
|
||||
return {"error": f"ERROR: {exc}", "code": "TOOL_ERROR"}
|
||||
host_locators = [loc for loc in reviewer_requested if loc not in declared_evidence]
|
||||
# B-08/C-06: allowed roots are the active workspace and the system repo only; the runtime
|
||||
# data plane is denied outright (the sensitive-name policy is a residual, not a boundary).
|
||||
|
|
@ -491,7 +528,7 @@ async def _run_plan_review_async(ctx: ToolContext, request: _PlanRequest) -> str
|
|||
try:
|
||||
state_root, task_id = _planning_state_location(ctx)
|
||||
except ValueError as exc:
|
||||
return f"ERROR: PLAN_REVIEW_STATE_INVALID: {exc}"
|
||||
return _typed_refusal(ctx, "TOOL_ERROR", f"ERROR: PLAN_REVIEW_STATE_INVALID: {exc}")
|
||||
prepared = _prepare_plan_inputs(ctx, request, state_root)
|
||||
if prepared.get("error"):
|
||||
if "PLAN_SPEC_INVALID" in prepared["error"]:
|
||||
|
|
@ -500,8 +537,8 @@ async def _run_plan_review_async(ctx: ToolContext, request: _PlanRequest) -> str
|
|||
ctx, state_root, task_id, {"goal": request.goal, "plan": request.plan, "spec": request.spec},
|
||||
reason="plan_input_invalid")
|
||||
except (OSError, TimeoutError, ValueError) as exc:
|
||||
return f"ERROR: PLAN_REVIEW_STATE_PERSIST_FAILED: {exc}"
|
||||
return prepared["error"]
|
||||
return _typed_refusal(ctx, "TOOL_ERROR", f"ERROR: PLAN_REVIEW_STATE_PERSIST_FAILED: {exc}")
|
||||
return _typed_refusal(ctx, str(prepared.get("code") or "TOOL_ERROR"), prepared["error"])
|
||||
spec, manifest = prepared["spec"], prepared["manifest"]
|
||||
system_root, active_root = prepared["system_root"], prepared["active_root"]
|
||||
constitutional = prepared["constitutional"]
|
||||
|
|
@ -512,7 +549,7 @@ async def _run_plan_review_async(ctx: ToolContext, request: _PlanRequest) -> str
|
|||
try:
|
||||
state = load_plan_review_state(state_root, task_id)
|
||||
except (OSError, TimeoutError, ValueError) as exc:
|
||||
return f"ERROR: PLAN_REVIEW_STATE_INVALID: {exc}"
|
||||
return _typed_refusal(ctx, "TOOL_ERROR", f"ERROR: PLAN_REVIEW_STATE_INVALID: {exc}")
|
||||
enforcement = get_review_enforcement()
|
||||
cap = review_max_cycles()
|
||||
cycles_paid = int(state.get("cycles_paid") or 0)
|
||||
|
|
@ -520,7 +557,7 @@ async def _run_plan_review_async(ctx: ToolContext, request: _PlanRequest) -> str
|
|||
try:
|
||||
_record_plan_review_attempt_with_reference(ctx, state_root, task_id, fingerprint=fingerprint)
|
||||
except (OSError, TimeoutError, ValueError) as exc:
|
||||
return f"ERROR: PLAN_REVIEW_STATE_PERSIST_FAILED: {exc}"
|
||||
return _typed_refusal(ctx, "TOOL_ERROR", f"ERROR: PLAN_REVIEW_STATE_PERSIST_FAILED: {exc}")
|
||||
previous_override: Optional[dict] = None
|
||||
replay_snapshot: Any = _PLAN_NO_SNAPSHOT
|
||||
resume_in_flight = False
|
||||
|
|
@ -575,7 +612,7 @@ async def _run_plan_review_async(ctx: ToolContext, request: _PlanRequest) -> str
|
|||
ctx, state_root, task_id, fingerprint=fingerprint, status="rail_degraded",
|
||||
reason="plan_task_deadline")
|
||||
except (OSError, TimeoutError, ValueError) as exc:
|
||||
return f"ERROR: PLAN_REVIEW_STATE_PERSIST_FAILED: {exc}"
|
||||
return _typed_refusal(ctx, "TOOL_ERROR", f"ERROR: PLAN_REVIEW_STATE_PERSIST_FAILED: {exc}")
|
||||
return _plan_deadline_skip(ctx, emit=True) or deadline_skip
|
||||
if cap is not None and cycles_paid >= cap and not resume_in_flight:
|
||||
return _cycles_exhausted(ctx, state, state_root, task_id, cap=cap, cycles_paid=cycles_paid,
|
||||
|
|
@ -707,11 +744,11 @@ async def _run_plan_review_async(ctx: ToolContext, request: _PlanRequest) -> str
|
|||
# dispatched nothing); the tool answer still describes the attempt that ran.
|
||||
stored = wave
|
||||
except (OSError, TimeoutError, ValueError) as exc:
|
||||
return f"ERROR: PLAN_REVIEW_STATE_PERSIST_FAILED: {exc}"
|
||||
return _typed_refusal(ctx, "TOOL_ERROR", f"ERROR: PLAN_REVIEW_STATE_PERSIST_FAILED: {exc}")
|
||||
try:
|
||||
paid_now = int(load_plan_review_state(state_root, task_id).get("cycles_paid") or 0)
|
||||
except (OSError, TimeoutError, ValueError) as exc:
|
||||
return f"ERROR: PLAN_REVIEW_STATE_INVALID: {exc}"
|
||||
return _typed_refusal(ctx, "TOOL_ERROR", f"ERROR: PLAN_REVIEW_STATE_INVALID: {exc}")
|
||||
_emit_plan_review_reference(ctx, task_id, state_root=state_root)
|
||||
if enforcement == "advisory" and not stored.get("closed"):
|
||||
# B2: loud at the moment — ONE typed owner-visible event per recorded open wave.
|
||||
|
|
@ -730,7 +767,7 @@ async def _run_plan_review_async(ctx: ToolContext, request: _PlanRequest) -> str
|
|||
attempt_fingerprint=fingerprint, cycles_paid=paid_now, cap=cap,
|
||||
) or stored
|
||||
except (OSError, TimeoutError, ValueError) as exc:
|
||||
return f"ERROR: PLAN_REVIEW_STATE_PERSIST_FAILED: {exc}"
|
||||
return _typed_refusal(ctx, "TOOL_ERROR", f"ERROR: PLAN_REVIEW_STATE_PERSIST_FAILED: {exc}")
|
||||
emit_review_cycles_exhausted(
|
||||
getattr(ctx, "event_queue", None), state_root, surface="plan_review",
|
||||
task_id=task_id, cycles_paid=paid_now, cap=cap, enforcement=enforcement,
|
||||
|
|
@ -809,7 +846,7 @@ def _cycles_exhausted(
|
|||
cycles_paid=cycles_paid, cap=cap,
|
||||
) or current
|
||||
except (OSError, TimeoutError, ValueError) as exc:
|
||||
return f"ERROR: PLAN_REVIEW_STATE_PERSIST_FAILED: {exc}"
|
||||
return _typed_refusal(ctx, "TOOL_ERROR", f"ERROR: PLAN_REVIEW_STATE_PERSIST_FAILED: {exc}")
|
||||
emit_review_cycles_exhausted(
|
||||
getattr(ctx, "event_queue", None), state_root, surface="plan_review", task_id=task_id,
|
||||
cycles_paid=cycles_paid, cap=cap, enforcement=enforcement, fingerprint=fingerprint,
|
||||
|
|
@ -854,39 +891,41 @@ def _cycles_exhausted(
|
|||
|
||||
|
||||
def _apply_disposition(ctx: ToolContext, disposition: dict) -> str:
|
||||
def _bad(text: str) -> str: # every refusal below is an argument-shape refusal
|
||||
return _typed_refusal(ctx, "TOOL_ARG_ERROR", text)
|
||||
|
||||
unknown = sorted(str(k) for k in disposition if k not in {"review_fingerprint", "items"})
|
||||
if unknown:
|
||||
return "ERROR: PLAN_REVIEW_DISPOSITION_INVALID: unknown fields: " + ", ".join(unknown)
|
||||
return _bad("ERROR: PLAN_REVIEW_DISPOSITION_INVALID: unknown fields: " + ", ".join(unknown))
|
||||
fingerprint = str(disposition.get("review_fingerprint") or "").strip()
|
||||
if not fingerprint:
|
||||
return "ERROR: PLAN_REVIEW_DISPOSITION_INVALID: review_fingerprint is required"
|
||||
return _bad("ERROR: PLAN_REVIEW_DISPOSITION_INVALID: review_fingerprint is required")
|
||||
try:
|
||||
root, task_id = _planning_state_location(ctx)
|
||||
state = load_plan_review_state(root, task_id)
|
||||
except (OSError, TimeoutError, ValueError) as exc:
|
||||
return "ERROR: PLAN_REVIEW_STATE_INVALID: " + str(exc)
|
||||
return _typed_refusal(ctx, "TOOL_ERROR", "ERROR: PLAN_REVIEW_STATE_INVALID: " + str(exc))
|
||||
enforcement = get_review_enforcement()
|
||||
cap = review_max_cycles()
|
||||
cycles_paid = int(state.get("cycles_paid") or 0)
|
||||
wave = plan_review_wave(state, fingerprint)
|
||||
if wave is None or wave.get("compact"):
|
||||
return (
|
||||
return _bad(
|
||||
"ERROR: PLAN_REVIEW_DISPOSITION_UNBINDABLE: no recorded plan-review wave holds "
|
||||
f"fingerprint {fingerprint} (compact history is not dispositionable). No plan attempt was recorded."
|
||||
)
|
||||
f"fingerprint {fingerprint} (compact history is not dispositionable). No plan attempt was recorded.")
|
||||
try:
|
||||
wave = _authority_wave(root, task_id, wave) or wave
|
||||
except (OSError, ValueError, json.JSONDecodeError) as exc:
|
||||
return "ERROR: PLAN_REVIEW_DISPOSITION_UNBINDABLE: exact wave is unreadable: " + str(exc)
|
||||
return _bad("ERROR: PLAN_REVIEW_DISPOSITION_UNBINDABLE: exact wave is unreadable: " + str(exc))
|
||||
# I-01: a disposition closes ONLY the CURRENT attempt's wave (never a superseded one).
|
||||
attempt = state.get("current_attempt") if isinstance(state.get("current_attempt"), dict) else {}
|
||||
current_fp = str(attempt.get("fingerprint") or "")
|
||||
if current_fp and current_fp != fingerprint:
|
||||
return (
|
||||
return _bad(
|
||||
"ERROR: PLAN_REVIEW_DISPOSITION_STALE: a disposition can close only the CURRENT "
|
||||
f"plan-review wave; a newer attempt supersedes it (current={current_fp}, "
|
||||
f"claimed={fingerprint}). Re-call plan_task with the spec you want reviewed. "
|
||||
"No plan attempt was recorded."
|
||||
"No plan attempt was recorded.",
|
||||
)
|
||||
if wave.get("closed"):
|
||||
return _publish_rendered_wave(ctx, wave, cap=cap, cycles_paid=cycles_paid, enforcement=enforcement,
|
||||
|
|
@ -894,13 +933,13 @@ def _apply_disposition(ctx: ToolContext, disposition: dict) -> str:
|
|||
notes=["already_closed: this wave is closed; the disposition is not re-applied"])
|
||||
raw_items = disposition.get("items")
|
||||
if not isinstance(raw_items, list):
|
||||
return "ERROR: PLAN_REVIEW_DISPOSITION_INVALID: items must be an array"
|
||||
return _bad("ERROR: PLAN_REVIEW_DISPOSITION_INVALID: items must be an array")
|
||||
if len(raw_items) > 2 * len(wave.get("findings") or []) + 8: # bounded like the findings they answer
|
||||
return "ERROR: PLAN_REVIEW_DISPOSITION_INVALID: more items than findings could need"
|
||||
return _bad("ERROR: PLAN_REVIEW_DISPOSITION_INVALID: more items than findings could need")
|
||||
items: List[dict] = []
|
||||
for index, item in enumerate(raw_items):
|
||||
if not isinstance(item, dict):
|
||||
return f"ERROR: PLAN_REVIEW_DISPOSITION_INVALID: items[{index}] must be an object"
|
||||
return _bad(f"ERROR: PLAN_REVIEW_DISPOSITION_INVALID: items[{index}] must be an object")
|
||||
items.append({
|
||||
"finding_id": str(item.get("finding_id") or "").strip()[:plan_spec.MAX_ID_CHARS * 2],
|
||||
"decision": str(item.get("decision") or "").strip().lower()[:40], # enum-like, bounded
|
||||
|
|
@ -909,9 +948,9 @@ def _apply_disposition(ctx: ToolContext, disposition: dict) -> str:
|
|||
known = {str(f.get("finding_id") or "") for f in wave.get("findings") or []}
|
||||
unknown_ids = sorted({i["finding_id"] for i in items if i["finding_id"] not in known})
|
||||
if unknown_ids:
|
||||
return (
|
||||
return _bad(
|
||||
"ERROR: PLAN_REVIEW_DISPOSITION_INVALID: unknown finding ids " + ", ".join(unknown_ids)
|
||||
+ "; valid ids: " + ", ".join(sorted(known))
|
||||
+ "; valid ids: " + ", ".join(sorted(known)),
|
||||
)
|
||||
closure = plan_spec.closure_after_disposition(
|
||||
str(wave.get("aggregate") or ""), wave.get("findings") or [], items, enforcement,
|
||||
|
|
@ -936,7 +975,8 @@ def _apply_disposition(ctx: ToolContext, disposition: dict) -> str:
|
|||
wave_artifact=disposition_ref, recorded_at=disposition_recorded_at,
|
||||
)
|
||||
except (OSError, TimeoutError, ValueError) as exc:
|
||||
return "ERROR: PLAN_REVIEW_STATE_PERSIST_FAILED: " + str(exc)
|
||||
return _typed_refusal(
|
||||
ctx, "TOOL_ERROR", "ERROR: PLAN_REVIEW_STATE_PERSIST_FAILED: " + str(exc))
|
||||
_emit_plan_review_reference(ctx, task_id, state_root=root)
|
||||
ctx.emit_progress_fn(
|
||||
f"📐 plan_task: disposition recorded — {'closed' if closure['closed'] else 'still open'} "
|
||||
|
|
|
|||
|
|
@ -981,6 +981,11 @@ def _submit_skill_to_hub(
|
|||
reason_code=exc.reason_code,
|
||||
repair_hint=exc.repair_hint,
|
||||
expected_repository=expected_repository,
|
||||
extra_fields={key: value for key, value in {
|
||||
"error_detail": getattr(exc, "detail", ""),
|
||||
"github_status": getattr(exc, "http_status", None),
|
||||
"github_operation": getattr(exc, "operation", ""),
|
||||
}.items() if value},
|
||||
)
|
||||
except Exception:
|
||||
return attempt.result(
|
||||
|
|
|
|||
|
|
@ -18,6 +18,7 @@ from ouroboros.config import (
|
|||
)
|
||||
from ouroboros.deadline_utils import owner_deadline_exhausted, transport_timeout_with_deadline
|
||||
from ouroboros.tools.registry import ToolContext, ToolEntry
|
||||
from ouroboros.tools.tool_result import ToolResult, _publish_tool_result
|
||||
from ouroboros.usage_accounting import current_usage_scope
|
||||
from ouroboros.utils import emit_cognitive_operation_event
|
||||
from ouroboros.observability import new_call_id
|
||||
|
|
@ -59,13 +60,31 @@ def _get_llm_client():
|
|||
return LLMClient()
|
||||
|
||||
|
||||
def _refuse(ctx: Any, message: str, code: str = "TOOL_ARG_ERROR") -> str:
|
||||
"""Publish a refusal this module AUTHORS as a typed result; text unchanged.
|
||||
|
||||
The registry types a string result by its first-line ``⚠️ IDENTIFIER``
|
||||
marker, so identifier-less prose (``⚠️ File not found: x.png``) was recorded
|
||||
as ``status=ok`` even though the producer already knew it had failed. Both
|
||||
codes used here carry ``status="error"``. Refusal text authored by a POLICY
|
||||
owner (``_read_file_parity_block``, ``protected_artifacts``) is NOT routed
|
||||
here: it already carries its own typed marker and the adapter types it
|
||||
``blocked``. Outside a registry invocation — the host's same-round
|
||||
auto-attach caller in ``loop_tool_execution`` — there is no active sidecar
|
||||
slot and no sidecar attribute on the ctx, so this publish is a no-op there.
|
||||
"""
|
||||
return _publish_tool_result(ctx, ToolResult(status="error", code=code, text=message))
|
||||
|
||||
|
||||
def _analyze_screenshot(ctx: ToolContext, prompt: str = "Describe what you see in this screenshot. Note any important UI elements, text, errors, or visual issues.", model: str = "") -> str:
|
||||
"""Analyze the last browser screenshot via VLM."""
|
||||
b64 = ctx.browser_state.last_screenshot_b64
|
||||
if not b64:
|
||||
return (
|
||||
return _refuse(
|
||||
ctx,
|
||||
"⚠️ No screenshot available. "
|
||||
"First call browse_page(output='screenshot') or browser_action(action='screenshot')."
|
||||
"First call browse_page(output='screenshot') or browser_action(action='screenshot').",
|
||||
"TOOL_ERROR",
|
||||
)
|
||||
|
||||
try:
|
||||
|
|
@ -543,15 +562,15 @@ def _load_local_image_payload(ctx: ToolContext, file_path: str) -> Tuple[Optiona
|
|||
import pathlib
|
||||
fp = pathlib.Path(file_path).expanduser().resolve()
|
||||
if not fp.exists():
|
||||
return None, f"⚠️ File not found: {file_path}"
|
||||
return None, _refuse(ctx, f"⚠️ File not found: {file_path}")
|
||||
allowed = _allowed_file_roots(ctx)
|
||||
if not any(_path_is_under(fp, root) for root in allowed):
|
||||
return None, (
|
||||
return None, _refuse(ctx, (
|
||||
f"⚠️ file_path must be inside the uploads directory, the skill-state tree "
|
||||
f"(state/skills), or a resource root this profile can read "
|
||||
f"(workspace / artifact_store / task_drive / subagent_projects / "
|
||||
f"deliverables / user files). Resolved path: {fp}. Use read_file for other paths."
|
||||
)
|
||||
))
|
||||
_pp_block = _read_file_parity_block(ctx, fp)
|
||||
if _pp_block:
|
||||
return None, _pp_block
|
||||
|
|
@ -566,28 +585,30 @@ def _load_local_image_payload(ctx: ToolContext, file_path: str) -> Tuple[Optiona
|
|||
if _artifact_block:
|
||||
return None, _artifact_block
|
||||
if fp.stat().st_size > _VLM_MAX_FILE_BYTES:
|
||||
return None, f"⚠️ File too large ({fp.stat().st_size} bytes). Max {_VLM_MAX_FILE_BYTES} bytes."
|
||||
return None, _refuse(
|
||||
ctx, f"⚠️ File too large ({fp.stat().st_size} bytes). Max {_VLM_MAX_FILE_BYTES} bytes."
|
||||
)
|
||||
try:
|
||||
raw = fp.read_bytes()
|
||||
except Exception as e:
|
||||
return None, f"⚠️ Failed to read image file: {e}"
|
||||
return None, _refuse(ctx, f"⚠️ Failed to read image file: {e}")
|
||||
# Fail closed: only recognized image bytes may be used.
|
||||
mime = _detect_image_mime_for_vlm(raw)
|
||||
if not mime:
|
||||
return None, (
|
||||
return None, _refuse(ctx, (
|
||||
"⚠️ File does not appear to be a supported image (PNG/JPEG/GIF/WEBP). "
|
||||
"Only image files are accepted."
|
||||
)
|
||||
))
|
||||
try:
|
||||
return _image_payload_from_bytes(raw, mime), ""
|
||||
except ValueError as e:
|
||||
return None, str(e)
|
||||
return None, _refuse(ctx, str(e))
|
||||
|
||||
|
||||
def _vlm_query(ctx: ToolContext, prompt: str, image_url: str = "", image_base64: str = "", image_mime: str = "image/png", file_path: str = "", model: str = "") -> str:
|
||||
"""Analyze one image from uploads file_path, public URL, or base64."""
|
||||
if not image_url and not image_base64 and not file_path:
|
||||
return "⚠️ Provide one of: file_path, image_url, or image_base64."
|
||||
return _refuse(ctx, "⚠️ Provide one of: file_path, image_url, or image_base64.")
|
||||
|
||||
images: List[Dict[str, Any]] = []
|
||||
try:
|
||||
|
|
@ -680,7 +701,7 @@ def attach_local_image_to_context(ctx: ToolContext, path: str) -> Tuple[bool, st
|
|||
never raises. Blind/local routes need no guard here — send-time routing
|
||||
captions/omits image blocks for routes that cannot see them."""
|
||||
if not path:
|
||||
return False, "⚠️ Provide a local image file path."
|
||||
return False, _refuse(ctx, "⚠️ Provide a local image file path.")
|
||||
payload, err = _load_local_image_payload(ctx, path)
|
||||
if err:
|
||||
return False, err
|
||||
|
|
|
|||
|
|
@ -634,6 +634,72 @@ class TestPlanReviewDispositionEnvelope(unittest.TestCase):
|
|||
run.assert_not_called()
|
||||
self.assertFalse((root / "task_results" / "parent.json").exists())
|
||||
|
||||
def test_padded_disposition_is_disposition_mode_not_a_mixed_envelope(self):
|
||||
"""Schema-default goal/plan/spec beside a real disposition say NOTHING, so they
|
||||
reach disposition mode exactly like the bare envelope. The reciprocal (a vacuous
|
||||
disposition beside a real plan) was already tolerated; this is the other side."""
|
||||
import ouroboros.tools.plan_review as pr
|
||||
from ouroboros.tools.registry import ToolContext
|
||||
|
||||
ctx = ToolContext(repo_dir=pathlib.Path("."), drive_root=pathlib.Path("."))
|
||||
ctx.task_id = "parent"
|
||||
disposition = {"review_fingerprint": "f" * 64, "items": []}
|
||||
for padding in (
|
||||
{}, # the bare disposition-only envelope, for reference
|
||||
{"goal": "", "plan": "", "spec": {}},
|
||||
{"goal": " ", "plan": "\n", "spec": {"in_scope": [], "non_goals": []}},
|
||||
{"goal": None, "spec": None},
|
||||
):
|
||||
with self.subTest(padding=padding):
|
||||
with patch.object(pr, "_apply_disposition", return_value="disposed") as apply_, patch.object(
|
||||
pr, "_run_plan_review_async",
|
||||
) as run:
|
||||
out = pr._handle_plan_task(ctx, review_disposition=disposition, **padding)
|
||||
self.assertEqual(out, "disposed")
|
||||
apply_.assert_called_once_with(ctx, disposition)
|
||||
run.assert_not_called()
|
||||
|
||||
def test_meaningful_or_invalid_padding_beside_a_disposition_is_still_mixed(self):
|
||||
"""Only schema-equivalent emptiness is ignored: a non-empty list, an unknown spec
|
||||
key or a wrong type is meaning (or an error) and keeps the typed refusal — a
|
||||
vacuity rule must never discard an invalid value to make a call pass."""
|
||||
import ouroboros.tools.plan_review as pr
|
||||
from ouroboros.tools.registry import ToolContext
|
||||
|
||||
ctx = ToolContext(repo_dir=pathlib.Path("."), drive_root=pathlib.Path("."))
|
||||
ctx.task_id = "parent"
|
||||
disposition = {"review_fingerprint": "f" * 64, "items": []}
|
||||
for padding in (
|
||||
{"spec": {"in_scope": [""]}},
|
||||
{"spec": {"unknown": ""}},
|
||||
{"goal": []},
|
||||
{"plan": "P changed"},
|
||||
):
|
||||
with self.subTest(padding=padding):
|
||||
with patch.object(pr, "_apply_disposition") as apply_, patch.object(
|
||||
pr, "_run_plan_review_async",
|
||||
) as run:
|
||||
out = pr._handle_plan_task(ctx, review_disposition=disposition, **padding)
|
||||
self.assertIn("PLAN_REVIEW_DISPOSITION_MIXED_ENVELOPE", out)
|
||||
apply_.assert_not_called()
|
||||
run.assert_not_called()
|
||||
|
||||
def test_disposition_with_an_empty_item_beside_a_plan_is_not_vacuous(self):
|
||||
"""``items=[{}]`` says something malformed, not nothing: beside a plan it is a
|
||||
mixed envelope (refused typed), never silently promoted into review mode."""
|
||||
import ouroboros.tools.plan_review as pr
|
||||
from ouroboros.tools.registry import ToolContext
|
||||
|
||||
ctx = ToolContext(repo_dir=pathlib.Path("."), drive_root=pathlib.Path("."))
|
||||
ctx.task_id = "parent"
|
||||
with patch.object(pr, "_run_plan_review_async") as run:
|
||||
out = pr._handle_plan_task(
|
||||
ctx, plan="P", goal="G", spec={},
|
||||
review_disposition={"review_fingerprint": "", "items": [{}]},
|
||||
)
|
||||
self.assertIn("PLAN_REVIEW_DISPOSITION_MIXED_ENVELOPE", out)
|
||||
run.assert_not_called()
|
||||
|
||||
def test_state_lookup_failure_is_error_not_absence(self):
|
||||
# Consultation guard: an indeterminate state store must ERROR, never be
|
||||
# classified as "no review" (which would silently launch a paid wave).
|
||||
|
|
|
|||
|
|
@ -4,11 +4,14 @@ from __future__ import annotations
|
|||
|
||||
import base64
|
||||
import json
|
||||
import subprocess
|
||||
import types
|
||||
|
||||
import pytest
|
||||
|
||||
from ouroboros import skill_publish_github as github
|
||||
from ouroboros.tools import github as transport
|
||||
from ouroboros.tools.github import GhResult
|
||||
|
||||
BASE_SHA = "1" * 40
|
||||
COMMIT_SHA = "2" * 40
|
||||
|
|
@ -16,6 +19,11 @@ SNAPSHOT_SHA = "a" * 64
|
|||
RULESET_SHA = "b" * 64
|
||||
|
||||
|
||||
@pytest.fixture(autouse=True)
|
||||
def synthetic_github_credentials(monkeypatch):
|
||||
monkeypatch.setenv("GITHUB_TOKEN", "ghp_SYNTHETIC1234567890")
|
||||
|
||||
|
||||
def _attempt():
|
||||
facts = types.SimpleNamespace(
|
||||
skill="demo",
|
||||
|
|
@ -110,7 +118,7 @@ def test_upstream_catalog_is_read_from_the_exact_resolved_base_sha(monkeypatch):
|
|||
def test_owner_actor_skips_fork_and_sync(monkeypatch):
|
||||
monkeypatch.setattr(
|
||||
github,
|
||||
"_gh_cmd",
|
||||
"_gh_run",
|
||||
lambda *_args, **_kwargs: pytest.fail("owner path must issue no fork command"),
|
||||
)
|
||||
attempt = _attempt()
|
||||
|
|
@ -131,10 +139,10 @@ def test_non_owner_sync_failure_is_typed(monkeypatch):
|
|||
def fake_gh(args, _ctx, **_kwargs):
|
||||
calls.append(args)
|
||||
if args[:2] == ["repo", "view"]:
|
||||
return '{"name":"project"}'
|
||||
return "⚠️ GH_ERROR: synthetic"
|
||||
return GhResult(True, '{"name":"project"}', 0, None, "")
|
||||
return GhResult(False, "⚠️ GH_ERROR: synthetic", 1, None, "exit")
|
||||
|
||||
monkeypatch.setattr(github, "_gh_cmd", fake_gh)
|
||||
monkeypatch.setattr(github, "_gh_run", fake_gh)
|
||||
with pytest.raises(github.SkillPublishGitHubError) as caught:
|
||||
github.prepare_publish_repository(
|
||||
types.SimpleNamespace(),
|
||||
|
|
@ -153,9 +161,9 @@ def test_direct_pr_url_yields_validated_receipt_without_lookup(monkeypatch):
|
|||
|
||||
def fake_gh(args, _ctx, **_kwargs):
|
||||
calls.append(args)
|
||||
return "https://github.com/hub/project/pull/7"
|
||||
return GhResult(True, "https://github.com/hub/project/pull/7", 0, None, "")
|
||||
|
||||
monkeypatch.setattr(github, "_gh_cmd", fake_gh)
|
||||
monkeypatch.setattr(github, "_gh_run", fake_gh)
|
||||
monkeypatch.setattr(
|
||||
github,
|
||||
"_json_value",
|
||||
|
|
@ -185,13 +193,13 @@ def test_ambiguous_create_uses_one_exact_read_only_settlement(monkeypatch):
|
|||
|
||||
def fake_gh(args, _ctx, **_kwargs):
|
||||
create_calls.append(args)
|
||||
return "⚠️ GH_TIMEOUT: synthetic"
|
||||
return GhResult(False, "⚠️ GH_TIMEOUT: synthetic", None, None, "timeout")
|
||||
|
||||
def fake_json(_ctx, args, **_kwargs):
|
||||
lookup_calls.append(args)
|
||||
return [_pull_row()]
|
||||
|
||||
monkeypatch.setattr(github, "_gh_cmd", fake_gh)
|
||||
monkeypatch.setattr(github, "_gh_run", fake_gh)
|
||||
monkeypatch.setattr(github, "_json_value", fake_json)
|
||||
receipt = github.create_pr_receipt(
|
||||
types.SimpleNamespace(),
|
||||
|
|
@ -224,7 +232,7 @@ def test_ambiguous_create_uses_one_exact_read_only_settlement(monkeypatch):
|
|||
],
|
||||
)
|
||||
def test_ambiguous_settlement_never_claims_wrong_or_nonunique_pr(monkeypatch, rows):
|
||||
monkeypatch.setattr(github, "_gh_cmd", lambda *_args, **_kwargs: "garbage")
|
||||
monkeypatch.setattr(github, "_gh_run", lambda *_args, **_kwargs: GhResult(True, "garbage", 0, None, ""))
|
||||
monkeypatch.setattr(github, "_json_value", lambda *_args, **_kwargs: rows)
|
||||
receipt = github.create_pr_receipt(
|
||||
types.SimpleNamespace(),
|
||||
|
|
@ -242,7 +250,7 @@ def test_ambiguous_settlement_never_claims_wrong_or_nonunique_pr(monkeypatch, ro
|
|||
|
||||
|
||||
def test_existing_branch_is_never_overwritten(monkeypatch):
|
||||
monkeypatch.setattr(github, "_gh_cmd", lambda *_args, **_kwargs: '{"ref":"exists"}')
|
||||
monkeypatch.setattr(github, "_gh_run", lambda *_args, **_kwargs: GhResult(True, '{"ref":"exists"}', 0, None, ""))
|
||||
with pytest.raises(github.SkillPublishGitHubError) as caught:
|
||||
github.ensure_branch(
|
||||
types.SimpleNamespace(),
|
||||
|
|
@ -252,3 +260,219 @@ def test_existing_branch_is_never_overwritten(monkeypatch):
|
|||
BASE_SHA,
|
||||
)
|
||||
assert caught.value.reason_code == "submission_branch_exists"
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"failure,stderr,http_status,hint",
|
||||
[
|
||||
("exit", "gh: Resource not accessible by personal access token (HTTP 403)", 403, "Settings → Secrets"),
|
||||
("exit", "gh: Conflict (HTTP 409)", 409, "conflict (HTTP 409)"),
|
||||
("timeout", "", None, "the outcome may be unknown"),
|
||||
("cli_missing", "", None, "Install the GitHub CLI (gh)"),
|
||||
("exit", "gh: ghp_SYNTHETIC1234567890 refused (HTTP 403)", 403, "Settings → Secrets"),
|
||||
],
|
||||
)
|
||||
def test_sync_failure_preserves_process_evidence_and_stops(
|
||||
monkeypatch, tmp_path, failure, stderr, http_status, hint,
|
||||
):
|
||||
calls = []
|
||||
|
||||
def run(cmd, *, cwd, capture_output, text, timeout, input, env):
|
||||
calls.append(cmd)
|
||||
assert cwd == str(tmp_path)
|
||||
assert capture_output is True and text is True and input is None
|
||||
assert env["GH_TOKEN"] == "ghp_SYNTHETIC1234567890"
|
||||
if cmd[1:3] == ["repo", "view"]:
|
||||
return subprocess.CompletedProcess(cmd, 0, '{"name":"project"}', "")
|
||||
assert cmd == ["gh", "api", "-X", "POST", "/repos/alice/project/merge-upstream", "-f", "branch=main"]
|
||||
assert timeout == 45
|
||||
if failure == "timeout":
|
||||
raise subprocess.TimeoutExpired(cmd, timeout)
|
||||
if failure == "cli_missing":
|
||||
raise FileNotFoundError("synthetic missing CLI")
|
||||
return subprocess.CompletedProcess(cmd, 1, "", stderr)
|
||||
|
||||
monkeypatch.setattr(subprocess, "run", run)
|
||||
attempt = _attempt()
|
||||
with pytest.raises(github.SkillPublishGitHubError) as caught:
|
||||
github.prepare_publish_repository(
|
||||
types.SimpleNamespace(repo_dir=tmp_path), attempt,
|
||||
owner="hub", repo="project", base_branch="main", login="alice",
|
||||
)
|
||||
error = caught.value
|
||||
assert error.reason_code == "fork_sync_failed"
|
||||
assert error.http_status == http_status
|
||||
assert error.operation == "merge-upstream"
|
||||
assert hint in error.repair_hint
|
||||
assert [stage for stage, _ in attempt.marks] == ["fork_ready"]
|
||||
assert len(calls) == 2 # Exactly one merge-upstream; never retry a mutation.
|
||||
assert "ghp_SYNTHETIC1234567890" not in error.detail
|
||||
if "ghp_SYNTHETIC" in stderr:
|
||||
assert "***" in error.detail
|
||||
if http_status:
|
||||
assert str(http_status) in error.detail
|
||||
if failure == "cli_missing":
|
||||
assert "token" not in error.repair_hint.lower()
|
||||
if failure == "timeout":
|
||||
assert error.detail == "⚠️ GH_TIMEOUT: exceeded 45s."
|
||||
|
||||
|
||||
def test_sync_success_marks_fork_synced(monkeypatch, tmp_path):
|
||||
calls = []
|
||||
|
||||
def run(cmd, **kwargs):
|
||||
calls.append(cmd)
|
||||
return subprocess.CompletedProcess(cmd, 0, '{}', "")
|
||||
|
||||
monkeypatch.setattr(subprocess, "run", run)
|
||||
attempt = _attempt()
|
||||
github.prepare_publish_repository(
|
||||
types.SimpleNamespace(repo_dir=tmp_path), attempt,
|
||||
owner="hub", repo="project", base_branch="main", login="alice",
|
||||
)
|
||||
assert [stage for stage, _ in attempt.marks] == ["fork_ready", "fork_synced"]
|
||||
assert len(calls) == 2
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"stderr,expected_status,expected_text",
|
||||
[
|
||||
# the FIRST gh marker wins; the bounded head keeps three non-empty lines
|
||||
("\n first\n\n second (HTTP 401)\n third\n ignored (HTTP 403)", 401,
|
||||
"⚠️ GH_ERROR: first | second (HTTP 401) | third"),
|
||||
# a bare number is prose, never a status
|
||||
("permission denied, status 403", None, "⚠️ GH_ERROR: permission denied, status 403"),
|
||||
# the status is read from the WHOLE redacted stderr, before the head is cut
|
||||
("first\nsecond\nthird\nlater line (HTTP 403)", 403, "⚠️ GH_ERROR: first | second | third"),
|
||||
("x" * 700 + " (HTTP 403)", 403, None),
|
||||
("", None, "⚠️ GH_ERROR: "),
|
||||
],
|
||||
)
|
||||
def test_transport_bounds_stderr_and_only_reads_gh_http_marker(
|
||||
monkeypatch, tmp_path, stderr, expected_status, expected_text,
|
||||
):
|
||||
monkeypatch.setattr(
|
||||
subprocess, "run", lambda cmd, **kwargs: subprocess.CompletedProcess(cmd, 7, "ignored stdout", stderr),
|
||||
)
|
||||
result = transport._gh_run(["api", "/user"], types.SimpleNamespace(repo_dir=tmp_path))
|
||||
assert (result.ok, result.exit_code, result.http_status, result.failure) == (False, 7, expected_status, "exit")
|
||||
assert result.text.startswith("⚠️ GH_ERROR: ")
|
||||
assert len(result.text.removeprefix("⚠️ GH_ERROR: ")) <= 600
|
||||
assert "ignored" not in result.text
|
||||
if expected_text is not None:
|
||||
assert result.text == expected_text
|
||||
|
||||
|
||||
@pytest.mark.parametrize("failure", ["cli_missing", "timeout", "exit", "exception"])
|
||||
def test_transport_never_publishes_a_sidecar(monkeypatch, tmp_path, failure):
|
||||
sentinel = object()
|
||||
ctx = types.SimpleNamespace(repo_dir=tmp_path, _active_builtin_tool_result=sentinel)
|
||||
|
||||
def run(cmd, **kwargs):
|
||||
if failure == "cli_missing":
|
||||
raise FileNotFoundError()
|
||||
if failure == "timeout":
|
||||
raise subprocess.TimeoutExpired(cmd, kwargs["timeout"])
|
||||
if failure == "exception":
|
||||
raise RuntimeError("synthetic ghp_SYNTHETIC1234567890")
|
||||
return subprocess.CompletedProcess(cmd, 1, "", "synthetic")
|
||||
|
||||
monkeypatch.setattr(subprocess, "run", run)
|
||||
result = transport._gh_run(["api", "/user"], ctx)
|
||||
assert result.failure == failure
|
||||
assert ctx._active_builtin_tool_result is sentinel
|
||||
assert transport._gh_cmd(["api", "/user"], ctx) == result.text
|
||||
assert ctx._active_builtin_tool_result is sentinel
|
||||
assert "ghp_SYNTHETIC1234567890" not in result.text
|
||||
|
||||
|
||||
def test_unconfigured_credentials_hint_uses_settings(monkeypatch):
|
||||
monkeypatch.setattr(github, "github_cli_configured", lambda: False)
|
||||
result = GhResult(False, "⚠️ GH_ERROR: synthetic", 1, None, "exit")
|
||||
assert github.github_repair_hint(result, operation="repo view", repository="alice/project", default="default") == (
|
||||
"No GitHub credential is configured; add GITHUB_TOKEN in Settings → Secrets, then retry."
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.parametrize("handler,kwargs", [
|
||||
(transport._get_issue, {}),
|
||||
(transport._comment_on_issue, {"body": "comment"}),
|
||||
(transport._close_issue, {}),
|
||||
])
|
||||
def test_nonpositive_issue_number_publishes_typed_argument_error(handler, kwargs):
|
||||
ctx = types.SimpleNamespace(_active_builtin_tool_result=None)
|
||||
text = handler(ctx, number=0, **kwargs)
|
||||
result = ctx._active_builtin_tool_result
|
||||
assert (result.status, result.code) == ("error", "TOOL_ARG_ERROR")
|
||||
assert result.text == text == "⚠️ TOOL_ARG_ERROR: issue number must be positive"
|
||||
|
||||
|
||||
@pytest.mark.parametrize("create_failed", [True, False])
|
||||
def test_failed_pr_settlement_keeps_producer_evidence_without_retry(monkeypatch, tmp_path, create_failed):
|
||||
calls = []
|
||||
|
||||
def run(cmd, **kwargs):
|
||||
calls.append(cmd)
|
||||
if cmd[1:3] == ["pr", "create"]:
|
||||
if create_failed:
|
||||
raise subprocess.TimeoutExpired(cmd, kwargs["timeout"])
|
||||
return subprocess.CompletedProcess(cmd, 0, "malformed URL", "")
|
||||
assert cmd[1:4] == ["api", "--method", "GET"]
|
||||
return subprocess.CompletedProcess(cmd, 1, "", "gh: read refused (HTTP 403)")
|
||||
|
||||
monkeypatch.setattr(subprocess, "run", run)
|
||||
attempt = _attempt()
|
||||
with pytest.raises(github.SkillPublishGitHubError) as caught:
|
||||
github.create_pr_receipt(
|
||||
types.SimpleNamespace(repo_dir=tmp_path), attempt,
|
||||
owner="hub", repo="project", base_branch="main", login="alice",
|
||||
branch="submit/demo-v1.0.0", title="Add demo", body="body", commit_sha=COMMIT_SHA,
|
||||
)
|
||||
error = caught.value
|
||||
assert error.reason_code == "pr_open_indeterminate"
|
||||
assert len(calls) == 2
|
||||
assert [stage for stage, _ in attempt.marks] == ["pr_create_attempted"]
|
||||
assert error.operation == ("pr create" if create_failed else "pulls")
|
||||
assert error.http_status == (None if create_failed else 403)
|
||||
assert ("GH_TIMEOUT" if create_failed else "403") in error.detail
|
||||
|
||||
|
||||
@pytest.mark.parametrize("stdout", ['["wrong shape"]', 'invalid ghp_SYNTHETIC1234567890 ' + 'x' * 1000])
|
||||
def test_malformed_json_keeps_bounded_redacted_detail(monkeypatch, tmp_path, stdout):
|
||||
monkeypatch.setattr(
|
||||
subprocess, "run", lambda cmd, **kwargs: subprocess.CompletedProcess(cmd, 0, stdout, ""),
|
||||
)
|
||||
with pytest.raises(github.SkillPublishGitHubError) as caught:
|
||||
github.fetch_upstream_catalog(types.SimpleNamespace(repo_dir=tmp_path), "hub", "project", "main")
|
||||
error = caught.value
|
||||
assert error.reason_code == "upstream_read_failed"
|
||||
assert error.http_status is None
|
||||
assert error.operation == "git/refs"
|
||||
assert error.detail
|
||||
assert len(error.detail) <= 640
|
||||
assert "ghp_SYNTHETIC1234567890" not in error.detail
|
||||
|
||||
|
||||
@pytest.mark.parametrize("returncode,stdout,stderr", [
|
||||
(1, "", "gh: mutation refused (HTTP 403)"),
|
||||
(0, '{"errors":[{"message":"synthetic GraphQL rejection"}]}', ""),
|
||||
])
|
||||
def test_commit_failure_keeps_graphql_cause(monkeypatch, tmp_path, returncode, stdout, stderr):
|
||||
calls = []
|
||||
|
||||
def run(cmd, **kwargs):
|
||||
calls.append(cmd)
|
||||
return subprocess.CompletedProcess(cmd, returncode, stdout, stderr)
|
||||
|
||||
monkeypatch.setattr(subprocess, "run", run)
|
||||
with pytest.raises(github.SkillPublishGitHubError) as caught:
|
||||
github.commit_payload(
|
||||
types.SimpleNamespace(repo_dir=tmp_path), "alice", "project", "submit/demo-v1.0.0",
|
||||
BASE_SHA, "Add demo", [],
|
||||
)
|
||||
error = caught.value
|
||||
assert error.reason_code == "commit_create_failed"
|
||||
assert error.operation == "graphql"
|
||||
assert error.http_status == (403 if returncode else None)
|
||||
assert ("403" if returncode else "synthetic GraphQL rejection") in error.detail
|
||||
assert len(calls) == 1
|
||||
|
|
|
|||
|
|
@ -558,6 +558,7 @@ def test_confirmation_failure_is_parseable_and_calls_nothing(tmp_path):
|
|||
assert result["ok"] is False
|
||||
assert result["reason_code"] == "confirmation_required"
|
||||
assert result["completed_effects"] == []
|
||||
assert not {"error_detail", "github_status", "github_operation"} & result.keys()
|
||||
|
||||
|
||||
def test_later_scanner_error_does_not_erase_known_scanner_identity():
|
||||
|
|
@ -575,3 +576,39 @@ def test_publisher_has_no_legacy_regex_secret_gate():
|
|||
source = pathlib.Path(skill_publish.__file__).read_text(encoding="utf-8")
|
||||
assert "contains_real_secret_value" not in source
|
||||
assert "permission_statement" not in source
|
||||
|
||||
|
||||
@pytest.mark.parametrize("http_status", [403, None])
|
||||
def test_github_failure_envelope_keeps_cause_and_last_completed_stage(monkeypatch, tmp_path, http_status):
|
||||
ctx, events, _captured = _install_transaction_fakes(monkeypatch, tmp_path, snapshot=_snapshot())
|
||||
monkeypatch.setenv("GITHUB_TOKEN", "ghp_SYNTHETIC1234567890")
|
||||
detail = "⚠️ GH_ERROR: gh: Resource not accessible by personal access token (HTTP 403)"
|
||||
|
||||
def prepare(_ctx, attempt, **kwargs):
|
||||
attempt.mark("fork_ready", repository="alice/project", actor="alice")
|
||||
raise skill_publish.SkillPublishGitHubError(
|
||||
"fork_sync_failed", "Update GITHUB_TOKEN in Settings → Secrets, then retry.",
|
||||
detail=detail, http_status=http_status, operation="merge-upstream",
|
||||
)
|
||||
|
||||
monkeypatch.setattr(skill_publish, "prepare_publish_repository", prepare)
|
||||
result = _submit(ctx)
|
||||
assert result["ok"] is False
|
||||
assert result["reason_code"] == "fork_sync_failed"
|
||||
assert result["completed_stage"] == "fork_ready"
|
||||
assert result["completed_effects"][-1]["stage"] == "fork_ready"
|
||||
assert result["error_detail"] == detail
|
||||
if http_status is None:
|
||||
assert "github_status" not in result
|
||||
else:
|
||||
assert result["github_status"] == http_status
|
||||
assert result["github_operation"] == "merge-upstream"
|
||||
from ouroboros.skill_publish_result import extract_skill_publish_result_metadata
|
||||
|
||||
projected = extract_skill_publish_result_metadata(json.dumps(result))["skill_publish_attempt"]
|
||||
assert projected["error_detail"] == detail
|
||||
assert projected["github_operation"] == "merge-upstream"
|
||||
assert projected.get("github_status") == http_status
|
||||
assert "receipt" not in result
|
||||
assert not any(row[0] == "mutation" for row in events)
|
||||
assert not (tmp_path / "state" / "skills" / "demo" / "ouroboroshub.json").exists()
|
||||
|
|
|
|||
|
|
@ -395,6 +395,99 @@ def test_plan_handler_wrapper_preserves_native_meta_for_all_projection_paths(
|
|||
)
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
("args", "marker"),
|
||||
(
|
||||
(
|
||||
{
|
||||
"plan": "P changed",
|
||||
"goal": "G",
|
||||
"spec": {"in_scope": ["a"]},
|
||||
"review_disposition": {
|
||||
"review_fingerprint": "f" * 64,
|
||||
"items": [
|
||||
{"finding_id": "slot_1:f1", "decision": "reject", "rationale": "one"},
|
||||
],
|
||||
},
|
||||
},
|
||||
"PLAN_REVIEW_DISPOSITION_MIXED_ENVELOPE",
|
||||
),
|
||||
(
|
||||
{"review_disposition": {"review_fingerprint": "", "items": []}},
|
||||
"PLAN_REVIEW_DISPOSITION_EMPTY",
|
||||
),
|
||||
),
|
||||
)
|
||||
def test_plan_task_argument_refusals_are_typed_at_the_registry_boundary(
|
||||
tmp_path,
|
||||
monkeypatch,
|
||||
args,
|
||||
marker,
|
||||
) -> None:
|
||||
"""A refusal the producer ALREADY knows about must not be recorded as execution.
|
||||
|
||||
Both texts are identifier-less ``ERROR:`` prose, which the legacy adapter reads as
|
||||
``status=ok``; the producer publishes the typed result instead."""
|
||||
import ouroboros.safety as safety
|
||||
from ouroboros.tools import plan_review
|
||||
|
||||
registry = ToolRegistry(repo_dir=tmp_path, drive_root=tmp_path)
|
||||
monkeypatch.setattr(safety, "check_safety", lambda *_args, **_kwargs: (True, ""))
|
||||
monkeypatch.setattr(
|
||||
plan_review,
|
||||
"_run_plan_review_async",
|
||||
lambda *_a, **_k: pytest.fail("a refused envelope must dispatch no reviewer"),
|
||||
)
|
||||
|
||||
result = registry.execute_result("plan_task", args)
|
||||
|
||||
assert isinstance(result, ToolResult)
|
||||
assert (result.status, result.code) == ("error", "TOOL_ARG_ERROR")
|
||||
assert marker in result.text
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
("reason", "text", "expected"),
|
||||
(
|
||||
(
|
||||
"review_budget_unavailable",
|
||||
"⚠️ PLAN_REVIEW_SKIPPED_BUDGET: the reviewer wave was declined before dispatch.",
|
||||
("unavailable", "CAPABILITY_UNAVAILABLE"),
|
||||
),
|
||||
("review_failed", "ERROR: Plan review failed: synthetic", ("error", "TOOL_ERROR")),
|
||||
),
|
||||
)
|
||||
def test_plan_unavailable_outcomes_are_typed_by_their_reason(
|
||||
tmp_path,
|
||||
monkeypatch,
|
||||
reason,
|
||||
text,
|
||||
expected,
|
||||
) -> None:
|
||||
"""A declined review is an availability outcome, a broken one a fault — neither is ok.
|
||||
|
||||
The budget-declined text carries a typed marker the legacy adapter read as a
|
||||
warning (``status=ok``); the exception path is identifier-less ``ERROR:`` prose.
|
||||
Both reach the registry with the status their reason means."""
|
||||
import ouroboros.safety as safety
|
||||
from ouroboros.tools import plan_review
|
||||
|
||||
registry = ToolRegistry(repo_dir=tmp_path, drive_root=tmp_path)
|
||||
monkeypatch.setattr(safety, "check_safety", lambda *_args, **_kwargs: (True, ""))
|
||||
monkeypatch.setattr(plan_review, "_planning_state_location", lambda _ctx: (tmp_path, "task"))
|
||||
|
||||
async def declined(ctx, _request):
|
||||
return plan_review._plan_unavailable(ctx, text, reason)
|
||||
|
||||
monkeypatch.setattr(plan_review, "_run_plan_review_async", declined)
|
||||
|
||||
result = registry.execute_result("plan_task", {"plan": "P", "goal": "G", "spec": {}})
|
||||
|
||||
assert isinstance(result, ToolResult)
|
||||
assert (result.status, result.code) == expected
|
||||
assert result.text == text
|
||||
|
||||
|
||||
def test_native_review_and_git_producers_bypass_adapter_and_keep_legacy_loop_fields(
|
||||
tmp_path,
|
||||
monkeypatch,
|
||||
|
|
|
|||
149
tests/test_typed_tool_refusals.py
Normal file
149
tests/test_typed_tool_refusals.py
Normal file
|
|
@ -0,0 +1,149 @@
|
|||
"""A builtin tool's known failure must leave the producer typed (package P4, issue #739).
|
||||
|
||||
The registry types a string result by its FIRST LINE: ``⚠️ IDENTIFIER`` maps through
|
||||
``LegacyTextResultAdapter`` to a status, while identifier-less prose (``⚠️ File not
|
||||
found: x``) or a bare ``ERROR: ...`` string is recorded as a SUCCESSFUL call — in
|
||||
``tools.jsonl``, the outcome classifier and the acceptance packet. A producer that
|
||||
already knows it failed publishes that fact as a typed ``ToolResult``
|
||||
(``tool_result._publish_tool_result``) or a first-line marker the adapter types.
|
||||
|
||||
This is a SOURCE lint over the builtin tool modules, not a runtime gate: it
|
||||
constrains how new code is written, never how the agent behaves (BIBLE P5). The
|
||||
flagged shape is narrow: the text opens with ``ERROR:`` or with ``⚠️`` followed by
|
||||
something that is not an UPPER_SNAKE identifier — prose, a lowercase name, a bare
|
||||
capital — AND the real adapter records it ``ok``. A typed-looking marker the adapter
|
||||
buckets as a warning (``⚠️ X_INVALID``) is a vocabulary question for the adapter's
|
||||
owner, not this lint, and an adapter that starts typing a text releases it here. Every surviving site carries a written reason here, so the
|
||||
allowlist IS the residual disclosure; a site removed from the tree must be removed
|
||||
here too (shrink-only), and a new site anywhere fails.
|
||||
|
||||
Scope limit (disclosed): only RETURNED string literals (plain, f-string with a static
|
||||
head, or a leading-literal concatenation) are scanned. A failure text that reaches the
|
||||
model through a variable, a tuple or a helper is invisible to this lint; the typed
|
||||
producer path is the repair for those, pinned by their own tests.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import ast
|
||||
import pathlib
|
||||
import re
|
||||
|
||||
from ouroboros.tools.tool_result import LegacyTextResultAdapter
|
||||
|
||||
REPO = pathlib.Path(__file__).resolve().parents[1]
|
||||
ROOTS = ("ouroboros/tools",)
|
||||
# A typed marker: ⚠️ then an UPPER_SNAKE identifier of three or more characters, delimited.
|
||||
_TYPED_MARKER = re.compile(r"^⚠️ +[A-Z][A-Z0-9_]{2,}(?=[\s:(]|$)")
|
||||
|
||||
# repo-relative path -> (occurrences, why they stay). Every entry is the same class this
|
||||
# lint exists for, left in place because the file belongs to another package of the
|
||||
# autonomy sprint (issue #739 names the follow-up); the count may only shrink.
|
||||
ALLOWED: dict[str, tuple[int, str]] = {
|
||||
"ouroboros/tools/control_runtime.py": (
|
||||
4, "runtime-control refusals (empty message, unknown model/effort) as prose; #739 follow-up",
|
||||
),
|
||||
"ouroboros/tools/control_task_results.py": (
|
||||
1, "prompt-cache horizon refusal as prose; #739 follow-up",
|
||||
),
|
||||
"ouroboros/tools/followup.py": (
|
||||
13, "`ERROR: FOLLOWUP_*` refusals — typed words in a prose shape the adapter records ok; #739 follow-up",
|
||||
),
|
||||
"ouroboros/tools/health.py": (
|
||||
1, "codebase-health computation failure as prose; #739 follow-up",
|
||||
),
|
||||
"ouroboros/tools/join_ledger.py": (
|
||||
7, "child-result verbs (peek/discard/cancel/override) refuse as lowercase prose; P1 territory, #739 follow-up",
|
||||
),
|
||||
"ouroboros/tools/knowledge.py": (
|
||||
6, "invalid topic/mode refusals as prose; #739 follow-up",
|
||||
),
|
||||
"ouroboros/tools/memory_tools.py": (
|
||||
2, "invalid source_id refusals as prose; #739 follow-up",
|
||||
),
|
||||
"ouroboros/tools/presence.py": (
|
||||
14, "`ERROR: PRESENCE_*` refusals — Presence is outside this package; #739 follow-up",
|
||||
),
|
||||
"ouroboros/tools/review_helpers.py": (
|
||||
1, "unexpected test-runner error as prose; #739 follow-up",
|
||||
),
|
||||
}
|
||||
|
||||
|
||||
def _static_head(node: ast.expr) -> tuple[str, bool] | None:
|
||||
"""Leading literal text of a returned string expression, plus whether it is partial.
|
||||
|
||||
Returns ``None`` when the returned value is not a string literal shape.
|
||||
``partial`` is True when a placeholder follows the literal head (f-string), so
|
||||
the identifier the registry would read may be dynamic.
|
||||
"""
|
||||
if isinstance(node, ast.Constant) and isinstance(node.value, str):
|
||||
return node.value, False
|
||||
if isinstance(node, ast.JoinedStr):
|
||||
head = ""
|
||||
for part in node.values:
|
||||
if isinstance(part, ast.Constant) and isinstance(part.value, str):
|
||||
head += part.value
|
||||
continue
|
||||
return (head, True) if head else None
|
||||
return head, False
|
||||
if isinstance(node, ast.BinOp) and isinstance(node.op, ast.Add):
|
||||
inner = _static_head(node.left)
|
||||
return (inner[0], True) if inner else None # the right operand is unknown
|
||||
return None
|
||||
|
||||
|
||||
def _untyped_failure(head: str, partial: bool) -> bool:
|
||||
first = head.splitlines()[0].strip() if head.strip() else ""
|
||||
if not first:
|
||||
return False
|
||||
if first.startswith("ERROR:"):
|
||||
return True
|
||||
if not first.startswith("⚠️"):
|
||||
return False
|
||||
if partial and not first.lstrip("⚠\ufe0f").strip():
|
||||
# ``⚠️ {code}: ...`` — the identifier is dynamic; this lint cannot judge it.
|
||||
return False
|
||||
if _TYPED_MARKER.match(first):
|
||||
return False
|
||||
return LegacyTextResultAdapter.from_text("lint", first).status == "ok"
|
||||
|
||||
|
||||
def observed_untyped_returns() -> dict[str, list[tuple[int, str]]]:
|
||||
found: dict[str, list[tuple[int, str]]] = {}
|
||||
for root in ROOTS:
|
||||
for path in sorted((REPO / root).glob("*.py")):
|
||||
tree = ast.parse(path.read_text(encoding="utf-8"), filename=str(path))
|
||||
rel = path.relative_to(REPO).as_posix()
|
||||
for node in ast.walk(tree):
|
||||
if not isinstance(node, ast.Return) or node.value is None:
|
||||
continue
|
||||
shape = _static_head(node.value)
|
||||
if shape is None:
|
||||
continue
|
||||
head, partial = shape
|
||||
if _untyped_failure(head, partial):
|
||||
found.setdefault(rel, []).append((node.lineno, head.splitlines()[0].strip()[:60]))
|
||||
return found
|
||||
|
||||
|
||||
def test_untyped_failure_returns_only_shrink() -> None:
|
||||
observed = observed_untyped_returns()
|
||||
counts = {path: len(rows) for path, rows in observed.items()}
|
||||
allowed = {path: count for path, (count, _why) in ALLOWED.items() if count}
|
||||
new_sites = {path: rows for path, rows in observed.items() if counts[path] > allowed.get(path, 0)}
|
||||
stale = {path: allowed[path] for path in allowed if counts.get(path, 0) < allowed[path]}
|
||||
detail = "\n".join(
|
||||
f" {path}:{line} {text}" for path, rows in sorted(new_sites.items()) for line, text in rows
|
||||
)
|
||||
assert not new_sites, (
|
||||
"a builtin tool returns a failure text the registry would record as ok; publish a typed "
|
||||
"ToolResult (tool_result._publish_tool_result) or a first-line ⚠️ IDENTIFIER marker instead:\n"
|
||||
f"{detail}"
|
||||
)
|
||||
assert not stale, (
|
||||
"untyped-failure sites disappeared; shrink ALLOWED in tests/test_typed_tool_refusals.py to match: "
|
||||
f"{stale}"
|
||||
)
|
||||
for path, (count, why) in ALLOWED.items():
|
||||
assert not count or why.strip(), f"ALLOWED[{path!r}] needs a written reason"
|
||||
|
|
@ -4,6 +4,7 @@ import sys
|
|||
import os
|
||||
import time
|
||||
import unittest
|
||||
from types import SimpleNamespace
|
||||
from unittest.mock import MagicMock, patch
|
||||
import pathlib
|
||||
import pytest
|
||||
|
|
@ -541,5 +542,180 @@ class TestVlmQueryTool(unittest.TestCase):
|
|||
self.assertIn("vlm_query", tools, "vlm_query must be registered")
|
||||
|
||||
|
||||
# --- Typed local-image failures at the registry boundary (P4 Work B) ---------
|
||||
#
|
||||
# The registry types a builtin's string result by its first-line
|
||||
# ``⚠️ IDENTIFIER`` marker, so vision's identifier-less prose refusals
|
||||
# ("⚠️ File not found: x.png") were recorded as status=ok in tools.jsonl, the
|
||||
# outcome classifier and the acceptance packet. These exercise the PUBLIC tools
|
||||
# through ``ToolRegistry.execute_result`` — the real boundary, not the handler.
|
||||
|
||||
|
||||
def _real_png_bytes() -> bytes:
|
||||
"""A genuinely decodable 1x1 RGBA PNG (zlib/struct, no PIL fixture files)."""
|
||||
import struct
|
||||
import zlib
|
||||
|
||||
def chunk(kind: bytes, data: bytes) -> bytes:
|
||||
return (
|
||||
struct.pack(">I", len(data))
|
||||
+ kind
|
||||
+ data
|
||||
+ struct.pack(">I", zlib.crc32(kind + data) & 0xFFFFFFFF)
|
||||
)
|
||||
|
||||
ihdr = struct.pack(">IIBBBBB", 1, 1, 8, 6, 0, 0, 0)
|
||||
idat = zlib.compress(b"\x00" + bytes((255, 0, 0, 255)))
|
||||
return b"\x89PNG\r\n\x1a\n" + chunk(b"IHDR", ihdr) + chunk(b"IDAT", idat) + chunk(b"IEND", b"")
|
||||
|
||||
|
||||
def _vision_registry(tmp_path, monkeypatch):
|
||||
"""A real ToolRegistry whose vision roots are one isolated uploads dir."""
|
||||
import ouroboros.safety as safety
|
||||
import ouroboros.tools.vision as vision
|
||||
from ouroboros.tools.registry import ToolRegistry
|
||||
|
||||
uploads = tmp_path / "isolated_uploads"
|
||||
uploads.mkdir()
|
||||
registry = ToolRegistry(repo_dir=tmp_path, drive_root=tmp_path)
|
||||
registry._ctx.messages = []
|
||||
monkeypatch.setattr(safety, "check_safety", lambda *_a, **_k: (True, ""))
|
||||
monkeypatch.setattr(vision, "_allowed_file_roots", lambda *_a, **_k: [uploads])
|
||||
return registry, uploads
|
||||
|
||||
|
||||
def test_view_image_missing_file_is_a_typed_error_at_the_registry(tmp_path, monkeypatch):
|
||||
registry, uploads = _vision_registry(tmp_path, monkeypatch)
|
||||
|
||||
result = registry.execute_result("view_image", {"path": str(uploads / "missing.png")})
|
||||
|
||||
assert result.status == "error"
|
||||
assert result.code == "TOOL_ARG_ERROR"
|
||||
assert "not found" in result.text.lower()
|
||||
assert registry._ctx.messages == []
|
||||
|
||||
|
||||
def test_vlm_query_missing_file_is_typed_and_never_builds_a_client(tmp_path, monkeypatch):
|
||||
import ouroboros.tools.vision as vision
|
||||
|
||||
registry, uploads = _vision_registry(tmp_path, monkeypatch)
|
||||
|
||||
def _no_client():
|
||||
raise AssertionError("a refused local file must not construct a VLM client")
|
||||
|
||||
monkeypatch.setattr(vision, "_get_llm_client", _no_client)
|
||||
|
||||
result = registry.execute_result(
|
||||
"vlm_query", {"prompt": "what is this?", "file_path": str(uploads / "missing.png")},
|
||||
)
|
||||
|
||||
assert result.status == "error"
|
||||
assert result.code == "TOOL_ARG_ERROR"
|
||||
assert "not found" in result.text.lower()
|
||||
assert registry._ctx.messages == []
|
||||
|
||||
|
||||
def test_view_image_success_stays_ok_and_attaches_the_image_block(tmp_path, monkeypatch):
|
||||
registry, uploads = _vision_registry(tmp_path, monkeypatch)
|
||||
img = uploads / "chart.png"
|
||||
img.write_bytes(_real_png_bytes())
|
||||
|
||||
result = registry.execute_result("view_image", {"path": str(img)})
|
||||
|
||||
assert (result.status, result.code) == ("ok", "OK"), result.text
|
||||
blocks = [
|
||||
block
|
||||
for message in registry._ctx.messages
|
||||
for block in (message.get("content") or [])
|
||||
if isinstance(block, dict) and block.get("type") == "image_url"
|
||||
]
|
||||
assert len(blocks) == 1, registry._ctx.messages
|
||||
assert blocks[0]["image_url"]["url"].startswith("data:image/png;base64,")
|
||||
source = pathlib.Path(blocks[0]["_source_path"])
|
||||
assert source.parent == tmp_path / "uploads" / "views", source
|
||||
assert source.exists()
|
||||
|
||||
|
||||
def test_view_image_policy_denial_keeps_its_blocked_status(tmp_path, monkeypatch):
|
||||
import ouroboros.protected_artifacts as protected_artifacts
|
||||
|
||||
registry, uploads = _vision_registry(tmp_path, monkeypatch)
|
||||
img = uploads / "protected.png"
|
||||
img.write_bytes(_real_png_bytes())
|
||||
monkeypatch.setattr(
|
||||
protected_artifacts,
|
||||
"block_reason_for_path",
|
||||
lambda *_a, **_k: "⚠️ RESOURCE_POLICY_BLOCKED: synthetic",
|
||||
)
|
||||
|
||||
result = registry.execute_result("view_image", {"path": str(img)})
|
||||
|
||||
# The policy owner already typed its own refusal; vision must not re-type it.
|
||||
assert result.status == "blocked", (result.status, result.code, result.text)
|
||||
assert "synthetic" in result.text
|
||||
assert registry._ctx.messages == []
|
||||
|
||||
|
||||
def test_view_image_non_image_and_outside_root_are_argument_errors(tmp_path, monkeypatch):
|
||||
registry, uploads = _vision_registry(tmp_path, monkeypatch)
|
||||
notes = uploads / "notes.txt"
|
||||
notes.write_bytes(b"this is plain text, not an image")
|
||||
outside = tmp_path / "elsewhere.png"
|
||||
outside.write_bytes(_real_png_bytes())
|
||||
|
||||
not_an_image = registry.execute_result("view_image", {"path": str(notes)})
|
||||
off_root = registry.execute_result("view_image", {"path": str(outside)})
|
||||
|
||||
assert (not_an_image.status, not_an_image.code) == ("error", "TOOL_ARG_ERROR")
|
||||
assert "supported image" in not_an_image.text.lower()
|
||||
assert (off_root.status, off_root.code) == ("error", "TOOL_ARG_ERROR")
|
||||
assert registry._ctx.messages == []
|
||||
|
||||
|
||||
def test_vlm_query_provider_capability_gap_keeps_vlm_error(tmp_path, monkeypatch):
|
||||
import ouroboros.tools.vision as vision
|
||||
|
||||
registry, uploads = _vision_registry(tmp_path, monkeypatch)
|
||||
img = uploads / "chart.png"
|
||||
img.write_bytes(_real_png_bytes())
|
||||
monkeypatch.setattr(vision, "_get_llm_client", lambda: object())
|
||||
monkeypatch.setattr(vision, "_resolve_vlm_model", lambda *_a, **_k: "")
|
||||
|
||||
result = registry.execute_result(
|
||||
"vlm_query", {"prompt": "what is this?", "file_path": str(img)},
|
||||
)
|
||||
|
||||
assert result.status == "error"
|
||||
assert result.code == "VLM_ERROR", result.text
|
||||
|
||||
|
||||
def test_vlm_query_without_any_image_argument_is_typed(tmp_path, monkeypatch):
|
||||
registry, _uploads = _vision_registry(tmp_path, monkeypatch)
|
||||
|
||||
result = registry.execute_result("vlm_query", {"prompt": "what is this?"})
|
||||
|
||||
assert (result.status, result.code) == ("error", "TOOL_ARG_ERROR")
|
||||
|
||||
|
||||
def test_attach_outside_a_registry_call_publishes_nothing(tmp_path, monkeypatch):
|
||||
"""The host's same-round auto-attach caller runs OUTSIDE any builtin
|
||||
invocation: no sidecar slot is installed and the ctx carries no sidecar
|
||||
attribute, so the loader's publish is a no-op and the (ok, message) contract
|
||||
is unchanged."""
|
||||
import ouroboros.tools.vision as vision
|
||||
|
||||
uploads = tmp_path / "isolated_uploads"
|
||||
uploads.mkdir()
|
||||
monkeypatch.setattr(vision, "_allowed_file_roots", lambda *_a, **_k: [uploads])
|
||||
ctx = SimpleNamespace(messages=[], drive_root=str(tmp_path))
|
||||
|
||||
ok, message = vision.attach_local_image_to_context(ctx, str(uploads / "missing.png"))
|
||||
|
||||
assert ok is False
|
||||
assert "not found" in message.lower()
|
||||
assert not hasattr(ctx, "_active_builtin_tool_result")
|
||||
assert ctx.messages == []
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
unittest.main()
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue