chore: sync workflow templates - #1204
Conversation
Automated sync from stranske/Workflows Template hash: 76689bc445fd Changes synced from sync-manifest.yml
stranske
added
sync
GitHub Actions
Failure
📝 WalkthroughWalkthroughIntroduces ChangesLLM Registry Extraction and Blocked-Model Enforcement
Orchestrator Skill Validation and Dynamic Summary Path
CI Action Pin Bumps
Sequence Diagram(s)sequenceDiagram
participant Caller
participant build_chat_client
participant build_chat_clients
participant llm_registry
rect rgba(135, 206, 235, 0.5)
Note over Caller,llm_registry: Single client construction with blocked-model check
Caller->>build_chat_client: provider, model, model_override
build_chat_client->>llm_registry: is_model_blocked(selected_provider, selected_model)
alt model is blocked
llm_registry-->>build_chat_client: True
build_chat_client-->>Caller: None (warning logged)
else not blocked
llm_registry-->>build_chat_client: False
build_chat_client-->>Caller: LangChain client
end
end
rect rgba(144, 238, 144, 0.5)
Note over Caller,llm_registry: Multi-slot construction with per-candidate blocked check
Caller->>build_chat_clients: provider, models
build_chat_clients->>llm_registry: load_model_registry()
llm_registry-->>build_chat_clients: registry
loop each slot candidate
build_chat_clients->>llm_registry: is_model_blocked(slot_provider, slot_model)
alt blocked
llm_registry-->>build_chat_clients: True
Note over build_chat_clients: skip slot, log warning
else not blocked
llm_registry-->>build_chat_clients: False
build_chat_clients-->>Caller: client added to result list
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with 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.
Inline comments:
In `@scripts/runner_lib/core.py`:
- Around line 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.
In `@tools/langchain_client.py`:
- Around line 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.
In `@tools/llm_registry.py`:
- Around line 82-96: The code lacks validation for the "quality" field within
each raw_entry before iterating over it. While a default empty dict is provided
when getting the quality field, if the actual value in the registry is null or
not a dictionary, calling .items() on quality_payload will still crash. Add a
type check to ensure quality_payload is actually a dictionary (using
isinstance(quality_payload, dict)) before attempting to iterate over
quality_payload.items() in the quality dictionary comprehension, similar to how
the code already validates that score is an int or float.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 75f386f3-8df9-496d-a42d-522fd02e28cc
📒 Files selected for processing (7)
.github/workflows/agents-guard.yml.github/workflows/maint-76-claude-code-review.ymlscripts/orchestrator_skill.pyscripts/reference_packs.pyscripts/runner_lib/core.pytools/langchain_client.pytools/llm_registry.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
stranske/Workflows(auto-detected)stranske/Template(auto-detected)
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
**/.github/workflows/**/!(*.md)
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
For workflow_call reusable workflows, do not use top-level permissions: block as it conflicts with caller permissions (documented in docs/INTEGRATION_GUIDE.md)
Files:
.github/workflows/agents-guard.yml.github/workflows/maint-76-claude-code-review.yml
**/.github/workflows/*.{yml,yaml}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
For startup_failure in workflows with zero jobs, check for invalid YAML syntax, invalid permission scopes, top-level permissions block on workflow_call, or circular workflow references
Files:
.github/workflows/agents-guard.yml.github/workflows/maint-76-claude-code-review.yml
.github/workflows/*.yml
📄 CodeRabbit inference engine (CLAUDE.md)
.github/workflows/*.yml: In this consumer repository, keep most workflow logic instranske/Workflowsand only carry repo-specific configuration unless explicitly documented as an exception
First-party consumers should reference reusable workflows with@mainunless intentionally pinning to an exact commit SHA for a controlled reasonReference reusable workflows with
@mainin first-party consumers unless intentionally pinning to an exact commit SHA for a controlled reason
Files:
.github/workflows/agents-guard.yml.github/workflows/maint-76-claude-code-review.yml
.github/workflows/agents-*.yml
📄 CodeRabbit inference engine (CLAUDE.md)
Agent workflow files (
agents-*.yml) are owned by Workflows; fix issues instranske/Workflows, not in this consumer repo
Files:
.github/workflows/agents-guard.yml
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Manager-Database repository uses Prefect 2.x - import schedules from prefect.client.schemas.schedules
Files:
scripts/reference_packs.pyscripts/orchestrator_skill.pytools/langchain_client.pytools/llm_registry.pyscripts/runner_lib/core.py
🔀 Multi-repo context stranske/Workflows, stranske/Template
Perfect! I have enough information now to provide a comprehensive cross-repository analysis. Let me compile the findings.
Linked repositories findings
stranske/Workflows (source repository)
Repo format validation changes:
scripts/orchestrator_skill.py:85-89— Updated_validate_repo()to enforce strictowner/nameformat by splitting on/and requiring exactly 2 non-empty components. Tests confirm intentional rejection of formats likeowner/repo/extra.scripts/reference_packs.py:72-76— Identical validation change applied to reference pack repo field.tests/scripts/test_reference_packs.py— Test case validates thattrend/research/extra(3 segments) is rejected with "repo must use owner/name format" error [::stranske/Workflows::]tests/scripts/test_orchestrator_skill.py— Test case validates thatowner/repo/extra(3 segments) is rejected with matching error [::stranske/Workflows::]
LLM Registry and blocked model enforcement:
tools/llm_registry.py:1-276— New module providesis_model_blocked(),normalize_provider(),select_model_for_tier(),resolve_slots(), and related helpers. Loads model registry fromconfig/model_registry.json.tools/langchain_client.py:25,102,229,283,365,489— Now delegates provider normalization and blocked-model checks totools.llm_registry. ReturnsNonewith warning logs when either resolved model or override model is blocked. At line 229-230: "Refusing blocked LLM model: %s/%s". At line 283-285: "Refusing blocked LLM model override". At line 365-370: Skips candidates that are blocked during multi-slot fallback. At line 489-490: Skips blocked slot overrides.templates/consumer-repo/tools/llm_registry.pyandtemplates/consumer-repo/tools/langchain_client.py— Consumer template already ships these modules synchronized [::stranske/Workflows::]
Orchestrator skill materialization changes:
scripts/runner_lib/core.py:materialize_orchestrator_skill()— Now suppressesFileNotFoundErrorwhen removing existing checkout directory (line:with contextlib.suppress(FileNotFoundError): shutil.rmtree(checkout_path))scripts/runner_lib/core.py:assemble_prompt()— Captures returnedorchestrator_summary_pathfrommaterialize_orchestrator_skill(). If not materializing, falls back tocontext["orchestrator_skill_summary_path"]and resolves relative paths against workspace. Only includes orchestrator content in prompt if file exists as a check (not loading fixed path). Theassemble-promptcommand now populates this fromORCHESTRATOR_SKILL_SUMMARY_PATHenvironment variable [::stranske/Workflows::]
stranske/Template (consumer repository)
No breaking impact from repo validation:
- Template repo contains NO
.github/reference_packs.jsonor.github/orchestrator_skill.jsonconfigs, so stricter validation will not affect it [::stranske/Template::]
Config files that will be synced:
config/llm_slots.jsonandconfig/model_registry.jsonexist and will receive updates- These match the producer repository structure [::stranske/Template::]
LLM client integration points:
tools/langchain_client.pyexists and will receive the blocked-model enforcement changesscripts/langchain/followup_issue_generator.pyandscripts/langchain/_llm_client.pyboth importbuild_chat_clientandbuild_chat_clients, which will now returnNoneor empty lists for blocked models instead of proceeding [::stranske/Template::]
Summary: The PR's validation changes are safe for Template repo (no configs to break), but the new blocked-model enforcement in langchain_client is a behavioral change — callers expecting client objects will now receive None with a logged warning. The orchestrator skill summary path handling is additive (context variable fallback is optional). The stricter repo format validation is intentional per test coverage and poses no risk since test data confirms it rejects 3+ segment repos that were previously allowed.
🔇 Additional comments (5)
.github/workflows/agents-guard.yml (1)
114-114: LGTM!Also applies to: 183-183
.github/workflows/maint-76-claude-code-review.yml (1)
192-192: LGTM!scripts/orchestrator_skill.py (1)
83-84: LGTM!Also applies to: 166-166
scripts/reference_packs.py (1)
86-87: LGTM!scripts/runner_lib/core.py (1)
369-371: Checkout path sanitization is already enforced upstream.The
checkout_pathvalues at line 369-371 come from validated sources and cannot contain absolute paths or..traversal:
- Reference pack case:
checkout_path = ".reference/{pack.name}"wherepack.nameis validated to match[A-Za-z0-9._-]+(seereference_packs.py:20)- Orchestrator skill case:
checkout_path = DEFAULT_CHECKOUT_PATH = ".reference/orchestrator-skill"(hardcoded; seeorchestrator_skill.py:30)Both
_validate_paths()functions inreference_packs.py:92-107andorchestrator_skill.py:89-107explicitly reject paths starting with/and containing..segments. The construction inbuild_checkout_plan()and hardcoded defaults further eliminate path traversal risk.The
shutil.rmtree()call is safe.> Likely an incorrect or invalid review comment.
| 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 |
There was a problem hiding this comment.
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 excAlso 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.
| 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 |
There was a problem hiding this comment.
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.
| entries: list[ModelRegistryEntry] = [] | ||
| for raw_entry in payload.get("models", []): | ||
| if not isinstance(raw_entry, dict): | ||
| logger.warning("Ignoring invalid model registry entry in %s; expected object", path) | ||
| continue | ||
| provider = normalize_provider(str(raw_entry.get("provider", ""))) | ||
| model = str(raw_entry.get("model_id", "")).strip() | ||
| if not provider or not model: | ||
| continue | ||
| quality_payload = raw_entry.get("quality", {}) | ||
| quality = { | ||
| str(tier).upper(): float(score) | ||
| for tier, score in quality_payload.items() | ||
| if isinstance(score, int | float) | ||
| } |
There was a problem hiding this comment.
Validate nested registry fields before iterating.
A malformed registry such as "models": null or an entry with "quality": null still raises despite the surrounding graceful-fallback handling. Guard both fields so bad config disables registry use instead of crashing client resolution.
Proposed fix
- entries: list[ModelRegistryEntry] = []
- for raw_entry in payload.get("models", []):
+ raw_models = payload.get("models", [])
+ if not isinstance(raw_models, list):
+ logger.warning("Invalid model registry format in %s; expected models list", path)
+ return []
+
+ entries: list[ModelRegistryEntry] = []
+ for raw_entry in raw_models:
if not isinstance(raw_entry, dict):
logger.warning("Ignoring invalid model registry entry in %s; expected object", path)
continue
@@
- quality_payload = raw_entry.get("quality", {})
+ quality_payload = raw_entry.get("quality", {})
+ if not isinstance(quality_payload, dict):
+ logger.warning(
+ "Ignoring invalid quality scores for %s/%s in %s; expected object",
+ provider,
+ model,
+ path,
+ )
+ quality_payload = {}
quality = {
str(tier).upper(): float(score)
for tier, score in quality_payload.items()🤖 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_registry.py` around lines 82 - 96, The code lacks validation for
the "quality" field within each raw_entry before iterating over it. While a
default empty dict is provided when getting the quality field, if the actual
value in the registry is null or not a dictionary, calling .items() on
quality_payload will still crash. Add a type check to ensure quality_payload is
actually a dictionary (using isinstance(quality_payload, dict)) before
attempting to iterate over quality_payload.items() in the quality dictionary
comprehension, similar to how the code already validates that score is an int or
float.
|
Closing as stale: newer replacement sync PR #1208 exists from Workflows sync wave sync/workflows-591316374281 after stranske/Workflows#2498 merged. |
1 participant
Sync Summary
Files Updated
Files Skipped
Review Checklist
Source: stranske/Workflows
Source SHA:
0b04de717dcadc23aea9e2eca0b8679d27e90666Template hash:
76689bc445fdSync branch:
sync/workflows-76689bc445fdConsumer repo:
stranske/Manager-DatabaseManifest:
.github/sync-manifest.ymlSummary by CodeRabbit
New Features
Refactor
owner/nameformat requirements.Chores