Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .github/workflows/agents-guard.yml
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ jobs:
github.event_name == 'pull_request_target' &&
steps.eligibility.outputs.should-run == 'true' &&
steps.api_client_base.outputs.available != 'true'
uses: "stranske/Workflows/.github/actions/setup-api-client@c2537cc959f2ce05926c4639d25b90678abc97bc" # v1
uses: "stranske/Workflows/.github/actions/setup-api-client@d68de1904bcdbe16bfe2462b73aa18f41f8a0a47" # v1
with:
secrets: ${{ toJSON(secrets) }}
github_token: ${{ github.token }}
Expand Down Expand Up @@ -180,7 +180,7 @@ jobs:
steps.eligibility.outputs.should-run == 'true' &&
github.event_name == 'pull_request' &&
steps.api_client_head.outputs.available != 'true'
uses: "stranske/Workflows/.github/actions/setup-api-client@c2537cc959f2ce05926c4639d25b90678abc97bc" # v1
uses: "stranske/Workflows/.github/actions/setup-api-client@d68de1904bcdbe16bfe2462b73aa18f41f8a0a47" # v1
with:
secrets: ${{ toJSON(secrets) }}
github_token: ${{ github.token }}
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/maint-76-claude-code-review.yml
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ jobs:
- name: Run Claude Code Review
id: claude
continue-on-error: true
uses: anthropics/claude-code-action@51705da45eecce209d4700538bf8377d5b5fc695 # v1
uses: anthropics/claude-code-action@2fee15510437d71399d9139ed60433470484a8fb # v1
with:
claude_code_oauth_token: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }}
allowed_bots: '*'
Expand Down
3 changes: 2 additions & 1 deletion scripts/orchestrator_skill.py
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,8 @@ def _require_nonempty_string(value: Any, field_name: str) -> str:


def _validate_repo(repo: str) -> str:
if "/" not in repo or repo.startswith("/") or repo.endswith("/"):
parts = repo.split("/")
if len(parts) != 2 or not all(parts):
raise OrchestratorSkillConfigError("repo must use owner/name format")
return repo

Expand Down
3 changes: 2 additions & 1 deletion scripts/reference_packs.py
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,8 @@ def _require_nonempty_string(value: Any, field_name: str) -> str:


def _validate_repo(repo: str) -> str:
if "/" not in repo or repo.startswith("/") or repo.endswith("/"):
parts = repo.split("/")
if len(parts) != 2 or not all(parts):
raise ReferencePackConfigError("repo must use owner/name format")
return repo

Expand Down
28 changes: 19 additions & 9 deletions scripts/runner_lib/core.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import argparse
import base64
import binascii
import contextlib
import dataclasses
import datetime as dt
import hashlib
Expand Down Expand Up @@ -356,11 +357,6 @@ def materialize_orchestrator_skill(
return None

if plan.pack:
materialize_reference_packs(
workspace_path,
reference_pack_name=plan.pack,
token=token,
)
reference_packs = _load_reference_packs_module()
snapshot = reference_packs.load_reference_packs(workspace_path)
matching = [
Expand All @@ -371,6 +367,13 @@ def materialize_orchestrator_skill(
if not matching:
raise ValueError(f"orchestrator skill reference pack not found: {plan.pack}")
checkout_path = workspace_path / matching[0].checkout_path
with contextlib.suppress(FileNotFoundError):
shutil.rmtree(checkout_path)
materialize_reference_packs(
workspace_path,
reference_pack_name=plan.pack,
token=token,
)
else:
checkout_path = _materialize_single_checkout_plan(
workspace_path,
Expand Down Expand Up @@ -413,12 +416,19 @@ def assemble_prompt(
)

if context.get("materialize_orchestrator_skill"):
materialize_orchestrator_skill(
orchestrator_summary_path = materialize_orchestrator_skill(
workspace,
pack_override=context.get("orchestrator_skill_pack") or None,
enabled_override=context.get("orchestrator_skill_enabled"),
token=token,
)
else:
orchestrator_summary_raw = context.get("orchestrator_skill_summary_path")
orchestrator_summary_path = (
Path(str(orchestrator_summary_raw)) if orchestrator_summary_raw else None
)
if orchestrator_summary_path and not orchestrator_summary_path.is_absolute():
orchestrator_summary_path = workspace / orchestrator_summary_path
Comment on lines +426 to +431

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Constrain orchestrator summary path to the workspace boundary.

Line 426-431 accepts an env/context-controlled path (including absolute paths), and Line 461-465 reads that file into prompt output. This enables arbitrary local file inclusion if the path source is influenced by untrusted input.

Suggested fix
@@
-    else:
-        orchestrator_summary_raw = context.get("orchestrator_skill_summary_path")
-        orchestrator_summary_path = (
-            Path(str(orchestrator_summary_raw)) if orchestrator_summary_raw else None
-        )
-        if orchestrator_summary_path and not orchestrator_summary_path.is_absolute():
-            orchestrator_summary_path = workspace / orchestrator_summary_path
+    else:
+        orchestrator_summary_raw = context.get("orchestrator_skill_summary_path")
+        orchestrator_summary_path = (
+            Path(str(orchestrator_summary_raw)) if orchestrator_summary_raw else None
+        )
+        if orchestrator_summary_path:
+            if not orchestrator_summary_path.is_absolute():
+                orchestrator_summary_path = workspace / orchestrator_summary_path
+            orchestrator_summary_path = orchestrator_summary_path.resolve()
+            workspace_resolved = workspace.resolve()
+            try:
+                orchestrator_summary_path.relative_to(workspace_resolved)
+            except ValueError as exc:
+                raise ValueError(
+                    "orchestrator_skill_summary_path must stay within workspace"
+                ) from exc

Also applies to: 461-465, 955-955

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@scripts/runner_lib/core.py` around lines 426 - 431, The orchestrator summary
path can be set to an absolute path via context, which creates a security
vulnerability allowing arbitrary file inclusion. After resolving the
orchestrator_summary_path (in the block starting with the
orchestrator_summary_raw assignment), add validation to ensure the final
resolved path is constrained within the workspace boundary. Use Path.resolve()
to get the absolute form of orchestrator_summary_path and verify it is within
the workspace directory using methods like is_relative_to() or by ensuring the
resolved path starts with the workspace path. If the path escapes the workspace
boundary, either reject it or raise an appropriate error.


output_file = str(
context.get("output_file") or _prompt_output_name(provider, context.get("pr_number"))
Expand Down Expand Up @@ -448,12 +458,11 @@ def assemble_prompt(
if reference_summary.is_file():
parts.extend(["\n\n## Reference Packs\n", _read_text(reference_summary).rstrip()])

orchestrator_summary = workspace / ".reference" / "ORCHESTRATOR_SKILL.md"
if orchestrator_summary.is_file():
if orchestrator_summary_path and orchestrator_summary_path.is_file():
parts.extend(
[
"\n\n## Orchestrator Skill Context\n",
_read_text(orchestrator_summary).rstrip(),
_read_text(orchestrator_summary_path).rstrip(),
]
)

Expand Down Expand Up @@ -943,6 +952,7 @@ def _cmd_assemble(args: argparse.Namespace) -> int:
"materialize_orchestrator_skill": args.materialize_orchestrator_skill,
"orchestrator_skill_pack": args.orchestrator_skill_pack or None,
"orchestrator_skill_enabled": _parse_optional_bool(args.orchestrator_skill_enabled),
"orchestrator_skill_summary_path": os.environ.get("ORCHESTRATOR_SKILL_SUMMARY_PATH"),
"github_token": os.environ.get("GH_TOKEN") or os.environ.get("GITHUB_TOKEN"),
}
prompt = assemble_prompt(args.reference_pack_name, context, args.provider)
Expand Down
148 changes: 81 additions & 67 deletions tools/langchain_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,29 +8,41 @@
from __future__ import annotations

import contextlib
import json
import logging
import os
from dataclasses import dataclass
from pathlib import Path

from tools import llm_registry as _llm_registry
from tools.llm_provider import DEFAULT_MODEL, GITHUB_MODELS_BASE_URL
from tools.llm_registry import (
PROVIDER_ANTHROPIC,
PROVIDER_GITHUB,
PROVIDER_OPENAI,
ModelRegistryEntry,
SlotDefinition,
apply_slot_env_overrides,
default_slots,
is_model_blocked,
load_model_registry,
load_slot_config,
normalize_provider,
registry_entry_for,
resolve_slots,
select_model_for_tier,
)

logger = logging.getLogger(__name__)

ENV_PROVIDER = "LANGCHAIN_PROVIDER"
ENV_MODEL = "LANGCHAIN_MODEL"
ENV_TIMEOUT = "LANGCHAIN_TIMEOUT"
ENV_MAX_RETRIES = "LANGCHAIN_MAX_RETRIES"
ENV_SLOT_CONFIG = "LANGCHAIN_SLOT_CONFIG"
ENV_SLOT_CONFIG = _llm_registry.ENV_SLOT_CONFIG
ENV_MODEL_REGISTRY_CONFIG = _llm_registry.ENV_MODEL_REGISTRY_CONFIG
ENV_SLOT_PREFIX = "LANGCHAIN_SLOT"
ENV_ANTHROPIC_KEY = "CLAUDE_API_STRANSKE"

PROVIDER_OPENAI = "openai"
PROVIDER_ANTHROPIC = "anthropic"
PROVIDER_GITHUB = "github-models"

DEFAULT_SLOT_CONFIG_PATH = Path(__file__).resolve().parent.parent / "config" / "llm_slots.json"
DEFAULT_SLOT_CONFIG_PATH = _llm_registry.DEFAULT_SLOT_CONFIG_PATH
DEFAULT_MODEL_REGISTRY_CONFIG_PATH = _llm_registry.DEFAULT_MODEL_REGISTRY_CONFIG_PATH


def _env_int(name: str, default: int) -> int:
Expand Down Expand Up @@ -59,24 +71,8 @@ def provider_label(self) -> str:
return f"{self.provider}/{self.model}"


@dataclass(frozen=True)
class SlotDefinition:
name: str
provider: str
model: str


def _normalize_provider(value: str | None) -> str | None:
if not value:
return None
normalized = value.strip().lower()
if normalized in {"github", "github_models", "github-models"}:
return PROVIDER_GITHUB
if normalized in {"anthropic", "claude"}:
return PROVIDER_ANTHROPIC
if normalized in {"openai"}:
return PROVIDER_OPENAI
return None
return normalize_provider(value)


def _resolve_provider(provider: str | None, *, force_openai: bool) -> tuple[str | None, bool]:
Expand All @@ -93,57 +89,53 @@ def _resolve_model(model: str | None) -> str:
return model or env_model or DEFAULT_MODEL


def _load_model_registry() -> list[ModelRegistryEntry]:
return load_model_registry()


def _registry_entry_for(
provider: str, model: str, registry: list[ModelRegistryEntry] | None = None
) -> ModelRegistryEntry | None:
return registry_entry_for(provider, model, registry=registry)


def _is_model_blocked(
provider: str, model: str, registry: list[ModelRegistryEntry] | None = None
) -> bool:
return is_model_blocked(provider, model, registry=registry)


def _select_model_for_tier(
*,
provider: str,
tier: str,
registry: list[ModelRegistryEntry] | None = None,
) -> str | None:
return select_model_for_tier(provider=provider, tier=tier, registry=registry)


def _default_slots() -> list[SlotDefinition]:
return [
SlotDefinition(name="slot1", provider=PROVIDER_OPENAI, model="gpt-5.4"),
SlotDefinition(name="slot2", provider=PROVIDER_ANTHROPIC, model="claude-sonnet-4-6"),
SlotDefinition(name="slot3", provider=PROVIDER_GITHUB, model=DEFAULT_MODEL),
]
return default_slots(github_default_model=DEFAULT_MODEL)


def _load_slot_config() -> list[SlotDefinition]:
config_path = os.environ.get(ENV_SLOT_CONFIG)
path = Path(config_path) if config_path else DEFAULT_SLOT_CONFIG_PATH
if not path.is_file():
return _default_slots()
try:
payload = json.loads(path.read_text(encoding="utf-8"))
except (OSError, json.JSONDecodeError):
return _default_slots()

slots: list[SlotDefinition] = []
for idx, entry in enumerate(payload.get("slots", []), start=1):
provider = _normalize_provider(str(entry.get("provider", "")))
model = str(entry.get("model", "")).strip()
if not provider or not model:
continue
name = str(entry.get("name") or f"slot{idx}").strip() or f"slot{idx}"
slots.append(SlotDefinition(name=name, provider=provider, model=model))

return slots or _default_slots()
return load_slot_config(github_default_model=DEFAULT_MODEL)


def _apply_slot_env_overrides(slots: list[SlotDefinition]) -> list[SlotDefinition]:
updated: list[SlotDefinition] = []
for idx, slot in enumerate(slots, start=1):
provider_key = f"{ENV_SLOT_PREFIX}{idx}_PROVIDER"
model_key = f"{ENV_SLOT_PREFIX}{idx}_MODEL"
provider_override = _normalize_provider(os.environ.get(provider_key))
model_override = os.environ.get(model_key)
if idx == 1:
model_override = model_override or os.environ.get(ENV_MODEL)
updated.append(
SlotDefinition(
name=slot.name,
provider=provider_override or slot.provider,
model=(model_override or slot.model).strip(),
)
)
return updated
return apply_slot_env_overrides(
slots,
env_model_name=ENV_MODEL,
env_slot_prefix=ENV_SLOT_PREFIX,
)


def _resolve_slots() -> list[SlotDefinition]:
return _apply_slot_env_overrides(_load_slot_config())
return resolve_slots(
github_default_model=DEFAULT_MODEL,
env_model_name=ENV_MODEL,
env_slot_prefix=ENV_SLOT_PREFIX,
)


def _is_reasoning_model(model: str) -> bool:
Expand Down Expand Up @@ -234,6 +226,9 @@ def build_chat_client(
selected_provider, provider_explicit = _resolve_provider(provider, force_openai=force_openai)
if provider_explicit and selected_provider is None:
return None
if selected_provider and _is_model_blocked(selected_provider, selected_model):
logger.warning("Refusing blocked LLM model: %s/%s", selected_provider, selected_model)
return None

if selected_provider == PROVIDER_GITHUB:
if not github_token:
Expand Down Expand Up @@ -283,6 +278,13 @@ def build_chat_client(
# Auto-select: slot order (OpenAI -> Claude -> GitHub Models by default).
slots = _resolve_slots()
model_override = model or os.environ.get(ENV_MODEL)
if model_override:
override_provider = selected_provider or (slots[0].provider if slots else "")
if override_provider and _is_model_blocked(override_provider, model_override):
logger.warning(
"Refusing blocked LLM model override: %s/%s", override_provider, model_override
)
return None
used_override = False
for slot in slots:
slot_model = model_override if model_override and not used_override else slot.model
Comment on lines +281 to 290

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Check blocked overrides against each slot provider.

This guard only checks model_override against the first resolved slot provider, but the loop can reuse that override for later providers when earlier slots lack credentials or fail. Move the blocked-model check into the slot loop so a blocked provider/model pair is never attempted.

Proposed fix
-    if model_override:
-        override_provider = selected_provider or (slots[0].provider if slots else "")
-        if override_provider and _is_model_blocked(override_provider, model_override):
-            logger.warning(
-                "Refusing blocked LLM model override: %s/%s", override_provider, model_override
-            )
-            return None
     used_override = False
     for slot in slots:
         slot_model = model_override if model_override and not used_override else slot.model
+        if _is_model_blocked(slot.provider, slot_model):
+            logger.warning("Skipping blocked LLM model override: %s/%s", slot.provider, slot_model)
+            continue
         if slot.provider == PROVIDER_OPENAI and openai_token:
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tools/langchain_client.py` around lines 281 - 290, The blocked-model check
using _is_model_blocked is currently performed only once before the slot loop
using override_provider determined from the first slot or selected_provider.
However, since model_override can be applied to multiple different slots in the
loop, each with its own provider, the same override might be attempted with
providers that should block it. Move the blocked-model check (the if statement
calling _is_model_blocked with override_provider and model_override) into the
slot loop so it checks whether each specific slot's provider combined with
model_override is blocked before that slot attempts to use the override.

Expand Down Expand Up @@ -358,6 +360,15 @@ def build_chat_clients(
selected_provider, provider_explicit = _resolve_provider(provider, force_openai=False)
if provider_explicit and selected_provider is None:
return []
registry = _load_model_registry()
if selected_provider:
blocked_models = [candidate for candidate in (first_model, second_model) if candidate]
if any(
_is_model_blocked(selected_provider, candidate, registry=registry)
for candidate in blocked_models
):
logger.warning("Refusing blocked LLM model for provider %s", selected_provider)
return []

clients: list[ClientInfo] = []

Expand Down Expand Up @@ -475,6 +486,9 @@ def build_chat_clients(
for idx, slot in enumerate(candidate_slots):
slot_model = model_overrides[idx] if idx < len(model_overrides) else None
slot_model = slot_model or slot.model
if _is_model_blocked(slot.provider, slot_model, registry=registry):
logger.warning("Skipping blocked LLM model override: %s/%s", slot.provider, slot_model)
continue
if slot.provider == PROVIDER_OPENAI and openai_token:
with contextlib.suppress(Exception):
clients.append(
Expand Down
Loading
Loading