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
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
161 changes: 161 additions & 0 deletions scripts/check_agents_md_freshness.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,161 @@
#!/usr/bin/env python3
"""Warn when the managed Orchestrator AGENTS.md section cites stale repo facts."""

from __future__ import annotations

import argparse
import json
import re
import shutil
import sys
from dataclasses import dataclass
from pathlib import Path

MANAGED_START = "<!-- BEGIN orch-playbook -->"
MANAGED_END = "<!-- END orch-playbook -->"
PATH_SUFFIXES = {
".cfg",
".ini",
".js",
".json",
".md",
".py",
".sh",
".toml",
".txt",
".yaml",
".yml",
}


@dataclass(frozen=True)
class Finding:
kind: str
value: str
message: str

def as_dict(self) -> dict[str, str]:
return {"kind": self.kind, "value": self.value, "message": self.message}


def managed_section(text: str) -> str | None:
start = text.find(MANAGED_START)
end = text.find(MANAGED_END, start + len(MANAGED_START)) if start >= 0 else -1
if start < 0 or end < 0 or end < start:
return None
return text[start : end + len(MANAGED_END)]


def _clean_ref(value: str) -> str:
value = value.strip().strip("\"'")
value = re.sub(r"[:#]L?\d+(?:-L?\d+)?$", "", value)
return value


def _looks_like_path(value: str) -> bool:
if value.startswith(("./", "../", ".github/", "docs/", "scripts/", "templates/", "tools/")):
return True
path = Path(value)
return "/" in value or path.suffix.lower() in PATH_SUFFIXES


def _path_exists(repo_root: Path, ref: str) -> bool:
return (repo_root / ref).exists()
Comment on lines +62 to +63

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 path checks to stay inside repo_root.

Absolute refs (/tmp/x) and traversal refs (../x) currently pass if they exist on disk, which weakens “repo freshness” guarantees and can hide stale repo references.

Suggested fix
+def _resolve_repo_path(repo_root: Path, ref: str) -> Path | None:
+    candidate = (repo_root / ref).resolve()
+    try:
+        candidate.relative_to(repo_root.resolve())
+    except ValueError:
+        return None
+    return candidate
+
 def _path_exists(repo_root: Path, ref: str) -> bool:
-    return (repo_root / ref).exists()
+    candidate = _resolve_repo_path(repo_root, ref)
+    return candidate.exists() if candidate else False
@@
 def _command_exists(repo_root: Path, ref: str) -> bool:
@@
     command = parts[0]
     if command.startswith(("./", "../")) or "/" in command:
-        return (repo_root / command).exists()
+        candidate = _resolve_repo_path(repo_root, command)
+        return candidate.exists() if candidate else False
     return shutil.which(command) is not None

Also applies to: 71-73

🤖 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/check_agents_md_freshness.py` around lines 62 - 63, The _path_exists
function does not validate that the resolved path stays within repo_root
boundaries, allowing absolute paths and traversal sequences to bypass the
intended constraint. Fix this by resolving both repo_root and the constructed
path to their absolute forms, then verify that the resolved path is actually
within the repo_root directory using path comparison or the is_relative_to
method. Apply the same fix to the similar path validation pattern referenced at
lines 71-73.



def _command_exists(repo_root: Path, ref: str) -> bool:
parts = ref.split()
if not parts:
Comment on lines +67 to +68

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 | 🟡 Minor | ⚡ Quick win

Use shell-aware tokenization for command refs.

str.split() breaks quoted args (e.g. backticked commands containing paths with spaces), causing incorrect command/arg validation.

Suggested fix
 import argparse
 import json
 import re
+import shlex
 import shutil
 import sys
@@
 def _command_exists(repo_root: Path, ref: str) -> bool:
-    parts = ref.split()
+    try:
+        parts = shlex.split(ref)
+    except ValueError:
+        parts = ref.split()
@@
 def _check_command_ref(repo_root: Path, ref: str) -> list[Finding]:
     findings: list[Finding] = []
@@
-    for arg in ref.split()[1:]:
+    try:
+        args = shlex.split(ref)[1:]
+    except ValueError:
+        args = ref.split()[1:]
+    for arg in args:

Also applies to: 80-80

🤖 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/check_agents_md_freshness.py` around lines 67 - 68, The `ref.split()`
method at the location where ref is parsed uses simple whitespace splitting that
breaks quoted arguments containing spaces, causing incorrect command/arg
validation. Replace the `ref.split()` calls with `shlex.split(ref)` to enable
shell-aware tokenization that properly respects quoted arguments. This change
needs to be applied at both occurrences mentioned in the review (the primary
location around line 67-68 and the additional location around line 80).

return True
command = parts[0]
if command.startswith(("./", "../")) or "/" in command:
return (repo_root / command).exists()
return shutil.which(command) is not None


def _check_command_ref(repo_root: Path, ref: str) -> list[Finding]:
findings: list[Finding] = []
if not _command_exists(repo_root, ref):
findings.append(Finding("command", ref, f"referenced command not found: {ref}"))
for arg in ref.split()[1:]:
arg = _clean_ref(arg)
if "=" in arg:
_, arg = arg.split("=", 1)
arg = _clean_ref(arg)
if _looks_like_path(arg) and not _path_exists(repo_root, arg):
findings.append(Finding("path", arg, f"referenced path not found: {arg}"))
return findings


def cited_refs(section: str) -> list[str]:
refs: list[str] = []
for raw in re.findall(r"`([^`]+)`", section):
value = _clean_ref(raw)
if not value or value.startswith(("http://", "https://")):
continue
refs.append(value)
return refs


def check_agents_md(repo_root: Path, agents_md: Path | None = None) -> list[Finding]:
agents_path = agents_md or repo_root / "AGENTS.md"
if not agents_path.exists():
return []
section = managed_section(agents_path.read_text(encoding="utf-8"))
if section is None:
return []

findings: list[Finding] = []
seen: set[tuple[str, str]] = set()
for ref in cited_refs(section):
if " " in ref:
for finding in _check_command_ref(repo_root, ref):
key = (finding.kind, finding.value)
if key not in seen:
findings.append(finding)
seen.add(key)
elif _looks_like_path(ref):
key = ("path", ref)
if key not in seen and not _path_exists(repo_root, ref):
findings.append(Finding("path", ref, f"referenced path not found: {ref}"))
seen.add(key)
return findings


def _emit_github_warnings(findings: list[Finding]) -> None:
for finding in findings:
message = finding.message.replace("%", "%25").replace("\n", "%0A").replace("\r", "%0D")
print(f"::warning title=AGENTS.md freshness::{message}")


def main(argv: list[str] | None = None) -> int:
parser = argparse.ArgumentParser(description=__doc__)
parser.add_argument("--repo-root", type=Path, default=Path.cwd())
parser.add_argument("--agents-md", type=Path)
parser.add_argument("--github-annotations", action="store_true")
parser.add_argument("--json", action="store_true", dest="as_json")
parser.add_argument(
"--strict", action="store_true", help="Exit non-zero when findings are present."
)
args = parser.parse_args(argv)

repo_root = args.repo_root.resolve()
agents_md = args.agents_md.resolve() if args.agents_md else None
findings = check_agents_md(repo_root, agents_md)
Comment on lines +142 to +144

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

Resolve --agents-md relative to --repo-root, not CWD.

args.agents_md.resolve() binds relative paths to the current working directory, which can silently validate the wrong file when --repo-root points elsewhere.

Suggested fix
-    repo_root = args.repo_root.resolve()
-    agents_md = args.agents_md.resolve() if args.agents_md else None
+    repo_root = args.repo_root.resolve()
+    if args.agents_md:
+        agents_md = (
+            args.agents_md
+            if args.agents_md.is_absolute()
+            else (repo_root / args.agents_md)
+        ).resolve()
+    else:
+        agents_md = None
🤖 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/check_agents_md_freshness.py` around lines 142 - 144, The line where
`agents_md` is assigned uses `args.agents_md.resolve()` which resolves relative
paths against the current working directory rather than against `repo_root`. Fix
this by resolving `agents_md` relative to `repo_root` instead, so that when a
relative path is provided via the `--agents-md` argument, it is correctly
interpreted as relative to the `--repo-root` directory. Ensure the conditional
check for whether `args.agents_md` exists is preserved in the fix.


if args.as_json:
print(json.dumps({"findings": [finding.as_dict() for finding in findings]}, indent=2))
elif findings:
for finding in findings:
print(finding.message)
else:
print("AGENTS.md managed section freshness check passed.")

if args.github_annotations and findings:
_emit_github_warnings(findings)

return 1 if args.strict and findings else 0


if __name__ == "__main__":
sys.exit(main())
9 changes: 3 additions & 6 deletions scripts/langchain/progress_reviewer.py
Original file line number Diff line number Diff line change
Expand Up @@ -427,14 +427,11 @@ def review_progress_with_llm(
rounds_without_completion,
)
try:
from scripts.langchain._llm_client import build_client
from tools.langchain_client import build_chat_client
except ImportError:
try:
from _llm_client import build_client
except ImportError:
build_client = None
build_chat_client = None

resolved = build_client(model=model) if build_client else None
resolved = build_chat_client(model=model) if build_chat_client else None
if not resolved:
score, aligned, unaligned = heuristic_alignment_check(
acceptance_criteria, recent_commits, files_changed
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

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
4 changes: 3 additions & 1 deletion tools/langchain_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -281,7 +281,9 @@ def build_chat_client(
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)
logger.warning(
"Refusing blocked LLM model override: %s/%s", override_provider, model_override
)
return None
used_override = False
for slot in slots:
Expand Down
36 changes: 7 additions & 29 deletions tools/llm_provider.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,13 +29,6 @@
from abc import ABC, abstractmethod
from dataclasses import dataclass

from tools.llm_registry import (
PROVIDER_ANTHROPIC,
PROVIDER_GITHUB,
PROVIDER_OPENAI,
configured_model_for_provider,
)

logger = logging.getLogger(__name__)

# GitHub Models API endpoint (OpenAI-compatible)
Expand Down Expand Up @@ -344,11 +337,8 @@ def _get_client(self):
logger.warning("langchain_openai not installed")
return None

model = configured_model_for_provider(PROVIDER_GITHUB, fallback="gpt-4.1")
if not model:
return None
return ChatOpenAI(
model=model,
model="gpt-4.1", # Battle-tested, reliable, available on GitHub Models
base_url=GITHUB_MODELS_BASE_URL,
api_key=os.environ.get("GITHUB_TOKEN"),
temperature=0.1, # Low temperature for consistent analysis
Comment on lines +341 to 344

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

Restore config-driven model resolution instead of hardcoded provider models.

At Line 341, Line 594, and Line 660 (and corresponding model_name assignments at Line 553, Line 568, Line 629, and Line 705), hardcoding bypasses slot/registry selection and blocked-model fallback behavior. That breaks the existing contract evidenced by tests/tools/test_langchain_client_config.py (e.g., expecting "gpt-configured", "gpt-safe", "claude-configured" from config).

Suggested fix
-        return ChatOpenAI(
-            model="gpt-4.1",  # Battle-tested, reliable, available on GitHub Models
+        selected_model = configured_model_for_provider(
+            self.name,
+            fallback="gpt-4.1",
+        )
+        return ChatOpenAI(
+            model=selected_model,
             base_url=GITHUB_MODELS_BASE_URL,
             api_key=os.environ.get("GITHUB_TOKEN"),
             temperature=0.1,  # Low temperature for consistent analysis
         )
...
-                model_name="gpt-4.1",  # Actual model used by GitHubModelsProvider
+                model_name=selected_model,
...
-                model_name="gpt-4.1",  # Actual model used by GitHubModelsProvider
+                model_name=selected_model or "",
...
-        return ChatOpenAI(
-            model="gpt-5.1-codex",  # Purpose-built for analyzing Codex coding sessions
+        selected_model = configured_model_for_provider(
+            self.name,
+            fallback="gpt-5.1-codex",
+        )
+        return ChatOpenAI(
+            model=selected_model,
             api_key=os.environ.get("OPENAI_API_KEY"),
             temperature=0.1,
         )
...
-                model_name="gpt-5.1-codex",  # Actual model used by OpenAIProvider
+                model_name=selected_model,
...
-        return ChatAnthropic(
-            model="claude-sonnet-4-5-20250929",
+        selected_model = configured_model_for_provider(
+            self.name,
+            fallback="claude-sonnet-4-5-20250929",
+        )
+        return ChatAnthropic(
+            model=selected_model,
             anthropic_api_key=os.environ.get(ANTHROPIC_API_KEY_ENV),
             temperature=0.1,
         )
...
-                model_name="claude-sonnet-4-5-20250929",
+                model_name=selected_model,

Also applies to: 553-553, 568-568, 594-597, 629-629, 660-663, 705-705

🤖 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/llm_provider.py` around lines 341 - 344, The hardcoded model name
"gpt-4.1" at line 341 and similar hardcoded models at lines 594 and 660 bypass
the config-driven model resolution system that respects slot/registry selection
and fallback behavior. Replace these hardcoded model values with calls to the
config-driven model resolution mechanism that will return models like
"gpt-configured", "gpt-safe", or "claude-configured" based on configuration.
Apply the same fix to all affected model assignments (lines 341, 553, 568, 594,
597, 629, 660, 663, 705) to ensure consistent behavior across all provider
instances.

Expand Down Expand Up @@ -560,7 +550,7 @@ def _parse_response(
confidence=adjusted_confidence,
reasoning=reasoning,
provider_used=self.name,
model_name=configured_model_for_provider(PROVIDER_GITHUB, fallback="gpt-4.1"),
model_name="gpt-4.1", # Actual model used by GitHubModelsProvider
raw_confidence=raw_confidence if adjusted_confidence != raw_confidence else None,
confidence_adjusted=adjusted_confidence != raw_confidence,
quality_warnings=warnings if warnings else None,
Expand All @@ -575,7 +565,7 @@ def _parse_response(
confidence=0.0,
reasoning=f"Failed to parse response: {e}",
provider_used=self.name,
model_name=configured_model_for_provider(PROVIDER_GITHUB, fallback="gpt-4.1"),
model_name="gpt-4.1", # Actual model used by GitHubModelsProvider
)


Expand All @@ -600,11 +590,8 @@ def _get_client(self):
logger.warning("langchain_openai not installed")
return None

model = configured_model_for_provider(PROVIDER_OPENAI, fallback="gpt-5.1-codex")
if not model:
return None
return ChatOpenAI(
model=model,
model="gpt-5.1-codex", # Purpose-built for analyzing Codex coding sessions
api_key=os.environ.get("OPENAI_API_KEY"),
temperature=0.1,
)
Expand Down Expand Up @@ -639,7 +626,7 @@ def analyze_completion(
confidence=result.confidence,
reasoning=result.reasoning,
provider_used=self.name,
model_name=configured_model_for_provider(PROVIDER_OPENAI, fallback="gpt-5.1-codex"),
model_name="gpt-5.1-codex", # Actual model used by OpenAIProvider
raw_confidence=result.raw_confidence,
confidence_adjusted=result.confidence_adjusted,
quality_warnings=result.quality_warnings,
Expand Down Expand Up @@ -669,14 +656,8 @@ def _get_client(self):
logger.warning("langchain_anthropic not installed")
return None

model = configured_model_for_provider(
PROVIDER_ANTHROPIC,
fallback="claude-sonnet-4-5-20250929",
)
if not model:
return None
return ChatAnthropic(
model=model,
model="claude-sonnet-4-5-20250929",
anthropic_api_key=os.environ.get(ANTHROPIC_API_KEY_ENV),
temperature=0.1,
)
Expand Down Expand Up @@ -721,10 +702,7 @@ def analyze_completion(
confidence=result.confidence,
reasoning=result.reasoning,
provider_used=self.name,
model_name=configured_model_for_provider(
PROVIDER_ANTHROPIC,
fallback="claude-sonnet-4-5-20250929",
),
model_name="claude-sonnet-4-5-20250929",
raw_confidence=result.raw_confidence,
confidence_adjusted=result.confidence_adjusted,
quality_warnings=result.quality_warnings,
Expand Down
Loading
Loading