Skip to content

fix: clear sync review debt at source - #2498

Merged
stranske merged 1 commit into
mainfrom
fix/sync-review-debt-20260622
Jun 22, 2026
Merged

fix: clear sync review debt at source#2498
stranske merged 1 commit into
mainfrom
fix/sync-review-debt-20260622

Conversation

@stranske

@stranske stranske commented Jun 22, 2026

Copy link
Copy Markdown
Owner

Summary

  • harden shared LLM registry parsing and blocked override fallback behavior
  • constrain runner reference checkout and orchestrator summary paths to safe workspace locations
  • tighten AGENTS freshness path checks and sync updated template copies

Validation

  • UV_CACHE_DIR=/tmp/uv-cache-workflows-sync-fix uv run pytest tests/tools/test_langchain_client.py tests/scripts/test_runner_lib.py tests/scripts/test_check_agents_md_freshness.py -q
  • UV_CACHE_DIR=/tmp/uv-cache-workflows-sync-fix uv run ruff check tools/langchain_client.py tools/llm_registry.py scripts/runner_lib/core.py scripts/check_agents_md_freshness.py templates/consumer-repo/tools/langchain_client.py templates/consumer-repo/tools/llm_registry.py templates/consumer-repo/scripts/check_agents_md_freshness.py tests/tools/test_langchain_client.py tests/scripts/test_runner_lib.py tests/scripts/test_check_agents_md_freshness.py
  • UV_CACHE_DIR=/tmp/uv-cache-workflows-sync-fix uv run black --check tools/langchain_client.py tools/llm_registry.py scripts/runner_lib/core.py scripts/check_agents_md_freshness.py templates/consumer-repo/tools/langchain_client.py templates/consumer-repo/tools/llm_registry.py templates/consumer-repo/scripts/check_agents_md_freshness.py tests/tools/test_langchain_client.py tests/scripts/test_runner_lib.py tests/scripts/test_check_agents_md_freshness.py
  • UV_CACHE_DIR=/tmp/uv-cache-workflows-sync-fix uv run --extra dev python scripts/validate_template_sync.py
  • UV_CACHE_DIR=/tmp/uv-cache-workflows-sync-fix uv run python -m py_compile tools/langchain_client.py tools/llm_registry.py scripts/runner_lib/core.py scripts/check_agents_md_freshness.py
  • git diff --check

Summary by CodeRabbit

  • Bug Fixes

    • Enhanced path validation to safely resolve file references and prevent invalid path handling
    • Improved model override behavior to gracefully fall back to default models when overrides are unavailable, rather than failing
    • Strengthened registry validation to handle malformed data without errors
  • Tests

    • Added coverage for path safety and model registry edge cases

@stranske stranske added automation Automation and workflow automation maintenance Maintenance tasks github:actions GitHub Actions related sync codex codex-automation labels Jun 22, 2026
@stranske
stranske temporarily deployed to agent-standard June 22, 2026 05:29 — with GitHub Actions Inactive
@coderabbitai

coderabbitai Bot commented Jun 22, 2026

Copy link
Copy Markdown

Review Change Stack

Caution

Review failed

Pull request was closed or merged during review

📝 Walkthrough

Walkthrough

This PR hardens path handling in two areas: check_agents_md_freshness.py (both the canonical and consumer-repo template copies) gains shlex-based command tokenization, a _resolve_repo_path() containment helper, and a _clean_ref() quote-stripping fix; runner_lib/core.py gains _resolve_child_path/_resolve_reference_checkout_path helpers enforced at all checkout and summary path call sites. Separately, llm_registry.py adds type guards for the models list and per-entry quality field, and langchain_client.py changes blocked-override handling from an early return to a per-slot fallback with used_override tracking. Tests are added for all new behaviors.

Changes

Path traversal hardening in AGENTS.md freshness checks and runner_lib

Layer / File(s) Summary
check_agents_md_freshness helpers: shlex parsing and repo-root containment
scripts/check_agents_md_freshness.py, templates/consumer-repo/scripts/check_agents_md_freshness.py
Adds shlex import and _command_parts(), tightens _clean_ref() to strip only matching surrounding quotes, and introduces _resolve_repo_path() with relative_to containment enforcement used by the updated _path_exists() and _command_exists(). Both the canonical and consumer-repo copies are updated identically.
check_agents_md_freshness CLI: --agents-md relative-path resolution
scripts/check_agents_md_freshness.py, templates/consumer-repo/scripts/check_agents_md_freshness.py
main() now joins relative --agents-md paths against repo_root before calling .resolve(), replacing the prior unconditional .resolve() that ignored the repo-root anchor.
runner_lib/core.py: containment helpers and call sites
scripts/runner_lib/core.py
Introduces _resolve_child_path and _resolve_reference_checkout_path which raise ValueError on traversal outside the intended root. Wires them into _materialize_single_checkout_plan, materialize_orchestrator_skill, and assemble_prompt's orchestrator_skill_summary_path handling.
Tests: path traversal rejection and CLI resolution
tests/scripts/test_check_agents_md_freshness.py, tests/scripts/test_runner_lib.py
Adds tests for absolute-path and ../ traversal references, shell-aware tokenization, CLI --agents-md relative resolution, and parametrized unsafe checkout_path values that must raise ValueError in materialize_orchestrator_skill and assemble_prompt.

LLM registry defensive parsing and blocked-override fallback

Layer / File(s) Summary
llm_registry: models list type guard and quality field validation
tools/llm_registry.py, templates/consumer-repo/tools/llm_registry.py
load_model_registry validates raw_models is a list and validates quality is a dict before use, returning empty structures on violations. apply_slot_env_overrides distinguishes override-requested vs. not when a resolved override is blocked, preserving the original unblocked slot.
langchain_client: in-loop blocked-override fallback
tools/langchain_client.py, templates/consumer-repo/tools/langchain_client.py
build_chat_client replaces the early pre-loop blocked-override return None with per-slot availability gating and a used_override flag. A blocked override logs a warning and falls back to slot.model instead of aborting immediately.
Tests: registry validation and blocked-override behavior
tests/tools/test_langchain_client.py
Adds tests verifying blocked overrides do not shift providers, malformed quality values are normalized to {}, a non-list models field returns [], and apply_slot_env_overrides reverts to original slots when the override model is blocked.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • stranske/Workflows#2488: Introduced check_agents_md_freshness.py and wired it into gate workflows; this PR directly hardens the same file's path parsing and reference validation logic.
  • stranske/Workflows#2495: Both PRs modify scripts/runner_lib/core.py around materialize_orchestrator_skill and assemble_prompt, changing how orchestrator skill checkout and summary paths are resolved.
🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 9.80% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check ❓ Inconclusive The title is too vague and generic, using the phrase 'clear sync review debt at source' which does not clearly convey the specific changes made in the pull request. Use a more descriptive title that reflects the primary changes, such as 'fix: harden LLM registry parsing and enforce workspace path safety' or similar to better indicate the key improvements made.
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/sync-review-debt-20260622

Comment @coderabbitai help to get the list of available commands and usage tips.

@stranske
stranske enabled auto-merge (squash) June 22, 2026 05:29
@stranske
stranske temporarily deployed to agent-standard June 22, 2026 05:29 — with GitHub Actions Inactive
@agents-workflows-bot

agents-workflows-bot Bot commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Automated Status Summary

Head SHA: 6361ccf
Latest Runs: ⏳ pending — Gate
Required contexts: Gate / gate, Health 45 Agents Guard / guard
Required: core tests (3.12): ⏳ pending, core tests (3.13): ⏳ pending, docker smoke: ⏳ pending, gate: ⏳ pending

Workflow / Job Result Logs
(no jobs reported) ⏳ pending

Coverage Overview

  • Coverage history entries: 1

Coverage Trend

Metric Value
Current 70.45%
Baseline 85.00%
Delta -14.55%
Minimum 70.00%
Status ✅ Pass

Top Coverage Hotspots (lowest coverage)

File Coverage Missing
scripts/check_api_wrapper_guard.py 0.0% 123
scripts/cleanup_labels.py 0.0% 127
scripts/issue_dedup_smoke.py 0.0% 4
scripts/list_registered_consumer_repos.py 0.0% 33
scripts/repo_review_backlog_scan.py 0.0% 203
scripts/repo_review_round1_runner.py 0.0% 225
scripts/repo_review_round1_schema.py 0.0% 194
scripts/runner_lib/__main__.py 0.0% 3
scripts/update_langchain_versions.py 0.0% 34
scripts/validate_dependency_test_setup.py 0.0% 112
scripts/validate_template_completeness.py 0.0% 88
scripts/validate_template_sync.py 0.0% 77
scripts/validate_workflow_yaml.py 0.0% 98
scripts/repo_review_round2_schema.py 8.8% 185
scripts/langchain/topic_splitter.py 19.1% 57

Low Coverage Files (<50.0%)

File Coverage Missing
scripts/check_api_wrapper_guard.py 0.0% 123
scripts/cleanup_labels.py 0.0% 127
scripts/issue_dedup_smoke.py 0.0% 4
scripts/list_registered_consumer_repos.py 0.0% 33
scripts/repo_review_backlog_scan.py 0.0% 203
scripts/repo_review_round1_runner.py 0.0% 225
scripts/repo_review_round1_schema.py 0.0% 194
scripts/runner_lib/__main__.py 0.0% 3
scripts/update_langchain_versions.py 0.0% 34
scripts/validate_dependency_test_setup.py 0.0% 112
scripts/validate_template_completeness.py 0.0% 88
scripts/validate_template_sync.py 0.0% 77
scripts/validate_workflow_yaml.py 0.0% 98
scripts/repo_review_round2_schema.py 8.8% 185
scripts/langchain/topic_splitter.py 19.1% 57

Updated automatically; will refresh on subsequent CI/Docker completions.


Keepalive checklist

Scope

No scope information available

Tasks

  • No tasks defined

Acceptance criteria

  • No acceptance criteria defined

@stranske
stranske temporarily deployed to agent-standard June 22, 2026 05:30 — with GitHub Actions Inactive

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 49c555d679

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread tools/langchain_client.py
Comment on lines +290 to +291
if not slot_available:
continue

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Do not retry blocked overrides on fallback providers

When LANGCHAIN_MODEL is set to a model blocked for the first slot (for example gpt-4o-mini, blocked for OpenAI in the current registry) and OPENAI_API_KEY is absent but CLAUDE_API_STRANSKE is present, continuing here skips the OpenAI slot before the blocked override is consumed. The next available slot then constructs ChatAnthropic(model='gpt-4o-mini') instead of falling back to claude-sonnet-4-6, so the advertised provider fallback breaks in missing/OpenAI-secret environments; consume or clear the blocked override before moving to later providers.

Useful? React with 👍 / 👎.

@stranske
stranske merged commit 8211687 into main Jun 22, 2026
107 of 118 checks passed
@stranske
stranske deleted the fix/sync-review-debt-20260622 branch June 22, 2026 05:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

automation Automation and workflow automation codex codex-automation github:actions GitHub Actions related maintenance Maintenance tasks sync

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants