diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 4275b6d..5791c5b 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -49,6 +49,8 @@ bash ./scripts/doctor.sh `doctor.sh` is the primary repository validation entrypoint. It currently runs locked-environment sync, dependency compatibility checks, `pre-commit`, `mypy`, `pytest`, the repository coverage gate, package build, and a built-wheel smoke test. +Ruff runs with the `S` (flake8-bandit) security rules enabled for shipped code. `tests/**` is exempt from `S` because test modules intentionally use asserts and fake credentials; do not widen that exemption beyond `tests/**`. + If you change shell scripts, also run `bash -n` on each modified script, for example: ```bash diff --git a/pyproject.toml b/pyproject.toml index 7c97f93..7c6c552 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -72,7 +72,12 @@ line-length = 100 src = ["src"] [tool.ruff.lint] -select = ["E", "F", "I", "B", "UP"] +select = ["E", "F", "I", "B", "UP", "S"] + +[tool.ruff.lint.per-file-ignores] +# Bandit-style security rules target shipped code. Test modules intentionally use +# asserts and hardcoded fake credentials, mirroring codex-a2a's configuration. +"tests/**" = ["S"] [tool.ruff.format] quote-style = "double" diff --git a/scripts/README.md b/scripts/README.md index 59280ef..394c82e 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -17,7 +17,7 @@ Executable scripts live in this directory. This file is the entry index for the - [`dependency_health.sh`](./dependency_health.sh): development dependency review entrypoint (`sync`/`pip check` + outdated + dev audit), while blocking CI/publish audits focus on runtime dependencies - [`check_coverage.py`](./check_coverage.py): enforces the overall coverage floor and per-file minimums for critical modules - [`find_thin_wrappers.py`](./find_thin_wrappers.py): static analysis helper that reports local functions with low caller counts and flags likely thin forwarding wrappers for manual abstraction review -- [`lint.sh`](./lint.sh): lint helper +- [`lint.sh`](./lint.sh): lint entrypoint used by CI; runs the repository `pre-commit` pipeline and **rejects hook-generated rewrites** instead of retrying them, so an unformatted or un-fixed commit cannot pass the gate - [`smoke_test_built_cli.sh`](./smoke_test_built_cli.sh): built-artifact smoke test for the released CLI runtime; defaults to the only local wheel, supports explicit wheel/sdist paths, and rejects ambiguous local artifact selection ## Notes @@ -25,5 +25,6 @@ Executable scripts live in this directory. This file is the entry index for the - `doctor.sh` and `dependency_health.sh` intentionally remain separate entrypoints and share common prerequisites through [`health_common.sh`](./health_common.sh). - `doctor.sh` covers the default local validation baseline, while `dependency_health.sh` remains focused on standalone dependency review and audit flow. - `doctor.sh` stops early when `pre-commit` rewrites files so you can review the changes and rerun `doctor.sh` from the updated worktree. +- `lint.sh` shares the same working-tree fingerprint check through [`health_common.sh`](./health_common.sh): auto-fixing hooks (`trailing-whitespace`, `end-of-file-fixer`, `ruff --fix`, `ruff format`) make the gate fail instead of being masked by a retry. - [`.github/dependabot.yml`](../.github/dependabot.yml) prefers a single weekly grouped Dependabot PR for `uv`, while `dependency_health.sh` remains the explicit review/audit entrypoint. - Repository-owned compatibility probes remain separate from the default regression path. See [`../docs/conformance.md`](../docs/conformance.md). diff --git a/scripts/conformance_probe.py b/scripts/conformance_probe.py index d091207..0ec8dc5 100644 --- a/scripts/conformance_probe.py +++ b/scripts/conformance_probe.py @@ -3,6 +3,7 @@ import argparse import json import os +import shutil import subprocess from collections.abc import Callable from pathlib import Path @@ -177,11 +178,18 @@ def main() -> int: args.output_dir.mkdir(parents=True, exist_ok=True) results, card = run_probes(args.base_url.rstrip("/"), token) failures = [result for result in results if result["status"] == "failed"] + git_executable = shutil.which("git") + if git_executable is None: + parser.error("git executable is required to record the repository commit") report = { "schema_version": 1, "scope": "repository-owned-a2a-1.0-compatibility-probes", "sut_url": args.base_url, - "repo_commit": subprocess.check_output(["git", "rev-parse", "HEAD"], text=True).strip(), + # The git executable is resolved via shutil.which and invoked with a + # fixed argument list, so this is not untrusted input execution. + "repo_commit": subprocess.check_output( # noqa: S603 + [git_executable, "rev-parse", "HEAD"], text=True + ).strip(), "summary": { "passed": len(results) - len(failures), "failed": len(failures), diff --git a/scripts/doctor.sh b/scripts/doctor.sh index 1332b99..bebf8ea 100755 --- a/scripts/doctor.sh +++ b/scripts/doctor.sh @@ -4,25 +4,13 @@ set -euo pipefail # shellcheck source=./health_common.sh source "$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)/health_common.sh" -doctor_repo_state_fingerprint() { - { - git diff --no-ext-diff --binary --cached -- . - git diff --no-ext-diff --binary -- . - while IFS= read -r -d '' path; do - printf 'untracked %s\n' "$path" - cat "$path" - printf '\n' - done < <(git ls-files --others --exclude-standard -z) - } | git hash-object --stdin -} - run_doctor_fix_phase() { local pre_commit_exit_code=0 local repo_state_before local repo_state_after echo "[doctor] run fix phase" - repo_state_before="$(doctor_repo_state_fingerprint)" + repo_state_before="$(repo_state_fingerprint)" if uv run pre-commit run --all-files; then pre_commit_exit_code=0 @@ -30,7 +18,7 @@ run_doctor_fix_phase() { pre_commit_exit_code=$? fi - repo_state_after="$(doctor_repo_state_fingerprint)" + repo_state_after="$(repo_state_fingerprint)" if [[ "$repo_state_before" != "$repo_state_after" ]]; then echo "[doctor] pre-commit modified files; review the changes and rerun doctor" >&2 exit 1 diff --git a/scripts/health_common.sh b/scripts/health_common.sh index b269155..fdb235a 100644 --- a/scripts/health_common.sh +++ b/scripts/health_common.sh @@ -21,3 +21,17 @@ run_shared_repo_health_prerequisites() { echo "[${label}] verify dependency compatibility" uv pip check } + +# Fingerprint of tracked and untracked working-tree state. Used to detect +# whether a tool rewrote repository files instead of only reporting on them. +repo_state_fingerprint() { + { + git diff --no-ext-diff --binary --cached -- . + git diff --no-ext-diff --binary -- . + while IFS= read -r -d '' path; do + printf 'untracked %s\n' "$path" + cat "$path" + printf '\n' + done < <(git ls-files --others --exclude-standard -z) + } | git hash-object --stdin +} diff --git a/scripts/lint.sh b/scripts/lint.sh index 13b747d..fa7ec6e 100755 --- a/scripts/lint.sh +++ b/scripts/lint.sh @@ -2,18 +2,42 @@ set -euo pipefail # Run the repo canonical lint pipeline to keep local and CI checks identical. +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +# shellcheck source=./health_common.sh +source "${SCRIPT_DIR}/health_common.sh" + +cd "$ROOT_DIR" + max_retries="${PRE_COMMIT_MAX_RETRIES:-3}" retry_delay_seconds="${PRE_COMMIT_RETRY_DELAY_SECONDS:-5}" attempt=1 while true; do + state_before="$(repo_state_fingerprint)" + + pre_commit_status=0 if uv run pre-commit run --all-files; then + pre_commit_status=0 + else + pre_commit_status=$? + fi + + # Auto-fixing hooks (trailing-whitespace, end-of-file-fixer, ruff --fix, + # ruff-format) exit non-zero when they rewrite files, so a retry would turn a + # real failure into a green run. Report it as a failure instead of retrying. + if [[ "$(repo_state_fingerprint)" != "${state_before}" ]]; then + echo "ERROR: pre-commit rewrote repository files; the lint gate rejects hook-generated edits." >&2 + echo "Review and commit the changes, then rerun." >&2 + exit 1 + fi + + if (( pre_commit_status == 0 )); then break fi if (( attempt >= max_retries )); then echo "ERROR: pre-commit failed after ${attempt} attempts." >&2 - exit 1 + exit "${pre_commit_status}" fi echo "WARN: pre-commit failed on attempt ${attempt}/${max_retries}, retrying in ${retry_delay_seconds}s..." >&2 diff --git a/src/opencode_a2a/jsonrpc/handlers/provider_discovery.py b/src/opencode_a2a/jsonrpc/handlers/provider_discovery.py index 23f1c30..221ba53 100644 --- a/src/opencode_a2a/jsonrpc/handlers/provider_discovery.py +++ b/src/opencode_a2a/jsonrpc/handlers/provider_discovery.py @@ -7,6 +7,7 @@ from starlette.responses import Response from ...contracts.extensions import PROVIDER_DISCOVERY_ERROR_BUSINESS_CODES +from ...opencode_upstream_client import UpstreamContractError from ..dispatch import ExtensionHandlerContext from ..error_responses import invalid_params_error from ..methods import ( @@ -93,7 +94,8 @@ async def handle_provider_discovery_request( ) if upstream_error is not None: return upstream_error - assert raw_result is not None + if raw_result is None: + raise UpstreamContractError("OpenCode provider discovery returned no payload") try: raw_providers, default_by_provider, connected = _extract_provider_catalog(raw_result) diff --git a/src/opencode_a2a/jsonrpc/handlers/session_lifecycle.py b/src/opencode_a2a/jsonrpc/handlers/session_lifecycle.py index 43f0577..b471f07 100644 --- a/src/opencode_a2a/jsonrpc/handlers/session_lifecycle.py +++ b/src/opencode_a2a/jsonrpc/handlers/session_lifecycle.py @@ -1,7 +1,7 @@ from __future__ import annotations import logging -from typing import Any +from typing import Any, cast from starlette.requests import Request from starlette.responses import Response @@ -355,7 +355,7 @@ async def handle_session_lifecycle_request( ) result = {"items": _normalize_session_status_items(raw_result)} elif method == context.method_get_session: - assert session_id is not None + session_id = cast(str, session_id) raw_result = await context.upstream_client.get_session( session_id, **routing_kwargs, @@ -367,7 +367,7 @@ async def handle_session_lifecycle_request( ) result = {"item": item} elif method == context.method_get_session_children: - assert session_id is not None + session_id = cast(str, session_id) raw_result = await context.upstream_client.list_child_sessions( session_id, **routing_kwargs, @@ -381,14 +381,14 @@ async def handle_session_lifecycle_request( ] } elif method == context.method_get_session_todo: - assert session_id is not None + session_id = cast(str, session_id) raw_result = await context.upstream_client.get_session_todo( session_id, **routing_kwargs, ) result = {"items": _normalize_todo_items(raw_result)} elif method == context.method_get_session_diff: - assert session_id is not None + session_id = cast(str, session_id) query = {"messageID": message_id} if message_id else None raw_result = await context.upstream_client.get_session_diff( session_id, @@ -397,8 +397,8 @@ async def handle_session_lifecycle_request( ) result = {"items": _normalize_diff_items(raw_result)} elif method == context.method_get_session_message: - assert session_id is not None - assert message_id is not None + session_id = cast(str, session_id) + message_id = cast(str, message_id) raw_result = await context.upstream_client.get_message( session_id, message_id, @@ -412,7 +412,7 @@ async def handle_session_lifecycle_request( ) result = {"item": item} elif method == context.method_fork_session: - assert session_id is not None + session_id = cast(str, session_id) raw_result = await context.upstream_client.fork_session( session_id, request=fork_request, @@ -422,14 +422,14 @@ async def handle_session_lifecycle_request( forked_session_id = item["id"] result = {"item": item} elif method == context.method_share_session: - assert session_id is not None + session_id = cast(str, session_id) raw_result = await context.upstream_client.share_session( session_id, **routing_kwargs, ) result = {"item": _normalize_session_summary(raw_result)} elif method == context.method_summarize_session: - assert session_id is not None + session_id = cast(str, session_id) raw_result = await context.upstream_client.summarize_session( session_id, request=summarize_request, @@ -439,7 +439,7 @@ async def handle_session_lifecycle_request( raise ValueError("Upstream summarize response must be a boolean") result = {"ok": raw_result, "session_id": session_id} elif method == context.method_revert_session: - assert session_id is not None + session_id = cast(str, session_id) raw_result = await context.upstream_client.revert_session( session_id, request=revert_request, @@ -447,20 +447,21 @@ async def handle_session_lifecycle_request( ) result = {"item": _normalize_session_summary(raw_result)} elif method == context.method_unrevert_session: - assert session_id is not None + session_id = cast(str, session_id) raw_result = await context.upstream_client.unrevert_session( session_id, **routing_kwargs, ) result = {"item": _normalize_session_summary(raw_result)} - else: - assert method == context.method_unshare_session - assert session_id is not None + elif method == context.method_unshare_session: + session_id = cast(str, session_id) raw_result = await context.upstream_client.unshare_session( session_id, **routing_kwargs, ) result = {"item": _normalize_session_summary(raw_result)} + else: + raise UpstreamContractError(f"Unsupported session lifecycle method: {method}") await session_claim.finalize() if forked_session_id is not None and identity: diff --git a/src/opencode_a2a/jsonrpc/handlers/session_queries.py b/src/opencode_a2a/jsonrpc/handlers/session_queries.py index b3e472e..cb36eff 100644 --- a/src/opencode_a2a/jsonrpc/handlers/session_queries.py +++ b/src/opencode_a2a/jsonrpc/handlers/session_queries.py @@ -3,12 +3,13 @@ import logging from collections.abc import Callable from functools import partial -from typing import Any +from typing import Any, cast from starlette.requests import Request from starlette.responses import Response from ...contracts.extensions import SESSION_QUERY_ERROR_BUSINESS_CODES +from ...opencode_upstream_client import UpstreamContractError from ..dispatch import ExtensionHandlerContext from ..error_responses import invalid_params_error, session_not_found_error from ..methods import ( @@ -97,18 +98,17 @@ async def _invoke_session_query() -> Any: params=query, **routing_kwargs, ) - assert session_id is not None list_messages_kwargs: dict[str, Any] = {"params": query} if workspace_id is not None: list_messages_kwargs["workspace_id"] = workspace_id return await context.upstream_client.list_messages( - session_id, + cast(str, session_id), **list_messages_kwargs, ) on_not_found: Callable[[], Response] | None = None if base_request.method == context.method_get_session_messages: - assert session_id is not None + session_id = cast(str, session_id) on_not_found = partial( context.error_response, base_request.id, @@ -126,7 +126,8 @@ async def _invoke_session_query() -> Any: ) if upstream_error is not None: return upstream_error - assert raw_result is not None + if raw_result is None: + raise UpstreamContractError("OpenCode session query returned no payload") try: if base_request.method == context.method_list_sessions: @@ -150,7 +151,7 @@ async def _invoke_session_query() -> Any: mapped.append(task) items: list[dict[str, Any]] = _apply_session_query_limit(mapped, limit=limit) else: - assert session_id is not None + session_id = cast(str, session_id) mapped = [] for item in raw_items: message = _as_a2a_message(session_id, item) diff --git a/src/opencode_a2a/jsonrpc/handlers/workspace_control.py b/src/opencode_a2a/jsonrpc/handlers/workspace_control.py index cecaa31..7a59226 100644 --- a/src/opencode_a2a/jsonrpc/handlers/workspace_control.py +++ b/src/opencode_a2a/jsonrpc/handlers/workspace_control.py @@ -1,7 +1,7 @@ from __future__ import annotations import logging -from typing import Any +from typing import Any, cast from starlette.requests import Request from starlette.responses import Response @@ -11,6 +11,7 @@ request_has_capability, ) from ...contracts.extensions import WORKSPACE_CONTROL_ERROR_BUSINESS_CODES +from ...opencode_upstream_client import UpstreamContractError from ..dispatch import ExtensionHandlerContext from ..error_responses import invalid_params_error from ..models import JSONRPCRequest @@ -316,7 +317,7 @@ async def handle_workspace_control_request( ) if request_error is not None: return request_error - assert request_body is not None + request_body = cast(dict[str, Any], request_body) request_validation_error = _validate_workspace_request( context, base_request.id, @@ -336,8 +337,7 @@ async def _invoke_workspace_method() -> Any: if method_key == "create_workspace": return await context.upstream_client.create_workspace(request_body or {}) if method_key == "remove_workspace": - assert workspace_id is not None - return await context.upstream_client.remove_workspace(workspace_id) + return await context.upstream_client.remove_workspace(cast(str, workspace_id)) if method_key == "list_worktrees": return await context.upstream_client.list_worktrees() if method_key == "create_worktree": @@ -357,7 +357,8 @@ async def _invoke_workspace_method() -> Any: ) if upstream_error is not None: return upstream_error - assert raw_result is not None + if raw_result is None: + raise UpstreamContractError("OpenCode workspace control returned no payload") try: result = _normalize_response_payload(method_key, raw_result) diff --git a/src/opencode_a2a/server/agent_card.py b/src/opencode_a2a/server/agent_card.py index bd6718e..e7032cc 100644 --- a/src/opencode_a2a/server/agent_card.py +++ b/src/opencode_a2a/server/agent_card.py @@ -276,7 +276,8 @@ def _build_agent_extensions( ), ), ] - assert tuple(ext.uri for ext in public_extensions) == PUBLIC_EXTENSION_URIS + if tuple(ext.uri for ext in public_extensions) != PUBLIC_EXTENSION_URIS: + raise RuntimeError("Agent Card public extension URIs drifted from the declared contract") if not include_detailed_contracts: return public_extensions @@ -342,9 +343,10 @@ def _build_agent_extensions( params=wire_contract_params, ), ] - assert ( - tuple(ext.uri for ext in authenticated_only_extensions) == AUTHENTICATED_ONLY_EXTENSION_URIS - ) + if tuple(ext.uri for ext in authenticated_only_extensions) != AUTHENTICATED_ONLY_EXTENSION_URIS: + raise RuntimeError( + "Agent Card authenticated extension URIs drifted from the declared contract" + ) return [*public_extensions, *authenticated_only_extensions] diff --git a/src/opencode_a2a/server/rest_tasks.py b/src/opencode_a2a/server/rest_tasks.py index 3a900b9..9c96a2e 100644 --- a/src/opencode_a2a/server/rest_tasks.py +++ b/src/opencode_a2a/server/rest_tasks.py @@ -293,8 +293,7 @@ def _parse_int(raw_value: str, *, field: str) -> int: message=f"{error_field} must be an integer.", ), ) - assert parsed is not None - return parsed + return cast(int, parsed) def _parse_bool(raw_value: str | None, *, field: str, default: bool) -> bool: diff --git a/tests/scripts/test_script_health_contract.py b/tests/scripts/test_script_health_contract.py index a1dfa62..37d5504 100644 --- a/tests/scripts/test_script_health_contract.py +++ b/tests/scripts/test_script_health_contract.py @@ -8,6 +8,7 @@ CONFORMANCE_TEXT = Path("scripts/conformance.sh").read_text() DEPENDENCY_HEALTH_TEXT = Path("scripts/dependency_health.sh").read_text() HEALTH_COMMON_TEXT = Path("scripts/health_common.sh").read_text() +LINT_TEXT = Path("scripts/lint.sh").read_text() SMOKE_TEST_TEXT = Path("scripts/smoke_test_built_cli.sh").read_text() COVERAGE_GATE_TEXT = Path("scripts/check_coverage.py").read_text() THIN_WRAPPER_FINDER_TEXT = Path("scripts/find_thin_wrappers.py").read_text() @@ -18,6 +19,7 @@ def test_shared_repo_health_prerequisites_live_in_common_helper() -> None: assert "run_shared_repo_health_prerequisites()" in HEALTH_COMMON_TEXT + assert "repo_state_fingerprint()" in HEALTH_COMMON_TEXT assert 'echo "[${label}] sync locked environment"' in HEALTH_COMMON_TEXT assert 'echo "[${label}] verify dependency compatibility"' in HEALTH_COMMON_TEXT assert "uv sync --all-extras --frozen" in HEALTH_COMMON_TEXT @@ -27,7 +29,8 @@ def test_shared_repo_health_prerequisites_live_in_common_helper() -> None: def test_doctor_keeps_local_regression_scope() -> None: assert 'source "$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)/health_common.sh"' in DOCTOR_TEXT assert 'run_shared_repo_health_prerequisites "doctor"' in DOCTOR_TEXT - assert "doctor_repo_state_fingerprint()" in DOCTOR_TEXT + assert "repo_state_fingerprint" in DOCTOR_TEXT + assert "doctor_repo_state_fingerprint" not in DOCTOR_TEXT assert "run_doctor_fix_phase()" in DOCTOR_TEXT assert "run_doctor_verify_phase()" in DOCTOR_TEXT assert "run_doctor_package_phase()" in DOCTOR_TEXT @@ -45,6 +48,13 @@ def test_doctor_keeps_local_regression_scope() -> None: assert "uv run pip-audit" not in DOCTOR_TEXT +def test_lint_gate_rejects_hook_generated_rewrites() -> None: + assert 'source "${SCRIPT_DIR}/health_common.sh"' in LINT_TEXT + assert "repo_state_fingerprint" in LINT_TEXT + assert "uv run pre-commit run --all-files" in LINT_TEXT + assert "the lint gate rejects hook-generated edits" in LINT_TEXT + + def test_dependency_health_keeps_dependency_review_scope() -> None: assert ( 'source "$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)/health_common.sh"' @@ -149,6 +159,11 @@ def test_coverage_policy_tracks_overall_and_critical_file_thresholds() -> None: assert "--cov-report=json:.coverage.json" in PYPROJECT_TEXT +def test_ruff_enables_security_rules_for_shipped_code() -> None: + assert 'select = ["E", "F", "I", "B", "UP", "S"]' in PYPROJECT_TEXT + assert '"tests/**" = ["S"]' in PYPROJECT_TEXT + + def test_thin_wrapper_finder_keeps_static_analysis_scope() -> None: assert "ast.parse" in THIN_WRAPPER_FINDER_TEXT assert "--max-callers" in THIN_WRAPPER_FINDER_TEXT