chore: sync workflow templates - #762
Conversation
Automated sync from stranske/Workflows Template hash: 97e42ef43aa5 Changes synced from sync-manifest.yml
stranske
added
sync
📝 WalkthroughWalkthroughThe PR tightens agent stall/effectiveness detection and replaces the keepalive soft-cap with a hard round budget. It introduces an orchestrator skill checkout system—new Python scripts, runner_lib materialization logic, and GitHub Actions workflow inputs—plus two new standalone CLI utilities: a tamper-resistant verifier verdict JSON extractor and an AGENTS.md managed-section freshness checker. ChangesAgent Loop Control Tightening
Orchestrator Skill Feature
New Utility Scripts
Sequence Diagram(s)sequenceDiagram
participant Workflow as GitHub Actions Workflow
participant core as runner_lib/core.py
participant skill as orchestrator_skill.py
participant git as git sparse-checkout
participant Prompt as Generated Prompt
Workflow->>core: assemble-prompt --materialize-orchestrator-skill
core->>skill: resolve_orchestrator_skill_plan(pack_override, enabled_override)
skill-->>core: OrchestratorSkillCheckoutPlan
alt plan.pack is set
core->>core: materialize_reference_packs(token)
else direct repo plan
core->>git: clone repo + sparse-checkout paths
git-->>core: files written to .reference/
end
core->>skill: write_orchestrator_skill_summary(checkout_path)
skill-->>core: .reference/ORCHESTRATOR_SKILL.md
core->>Prompt: append "Orchestrator Skill Context" section
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 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 |
GitHub Actions
Failure
|
Workflow state fingerprint for Keepalive Loop Reporter. Do not edit. |
|
Workflow state fingerprint for Agents Gate Followups. Do not edit. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 @.github/scripts/verifier_verdict_json.py:
- Line 103: In the path handling logic where args.output is read, add explicit
error handling to catch the edge case where args.output exists but is a
directory rather than a file. Instead of silently treating a directory path as
empty output, raise an error or log a warning that clearly indicates the
configuration issue, so that CI pipeline failures are not masked by ambiguous
behavior. This ensures debugging in CI environments is more straightforward when
path configuration errors occur.
In @.github/workflows/agents-72-codex-belt-worker-dispatch.yml:
- Around line 77-80: The max_parallel field on line 77 is hard-coded to 1, which
silently overrides any max_parallel input value passed by callers and breaks
existing workflows expecting higher concurrency values. Replace the hard-coded
max_parallel: 1 with a reference to the input parameter using the pattern ${{
inputs.max_parallel }} to propagate the caller's concurrency preference. Note
that per coding guidelines, this fix should be applied in the stranske/Workflows
repository where the agents-*.yml workflow templates are maintained, not edited
locally in this consumer repository.
In `@scripts/check_agents_md_freshness.py`:
- Around line 62-63: The _path_exists function on line 63 and the similar code
on lines 71-73 have a path traversal vulnerability where absolute paths in the
ref and command parameters bypass repo scoping. When using the `/` operator with
Path objects, absolute paths ignore the left operand entirely, so a ref like
"/etc/hosts" would resolve outside the repo_root. Fix this by validating that
ref and command are relative paths and do not start with a forward slash, or by
using methods like resolve() and relative_to() to ensure the resulting path is
within the repo_root directory before checking if it exists.
- Around line 142-144: The args.agents_md path is being resolved relative to the
current working directory instead of relative to repo_root, which causes
incorrect file loading when the script is invoked outside the repo directory. In
the assignment where agents_md is set on line 143, change the resolution logic
so that args.agents_md is resolved relative to repo_root (which is already
resolved on line 142) instead of being resolved independently, ensuring the path
is correctly anchored to the repository root directory.
In `@scripts/orchestrator_skill.py`:
- Around line 82-85: The `_validate_repo` function currently only checks that a
forward slash exists and that the string does not start or end with one, but
this allows multiple slashes (e.g., org/team/repo) which violates the declared
owner/name format contract. Update the validation logic in `_validate_repo` to
ensure there is exactly one forward slash in the repo string by counting the
occurrences of "/" and rejecting any value that does not have exactly one slash
separator.
- Around line 221-242: The issue is that resolve_orchestrator_skill_plan()
returns early when enabled is False, before processing the pack_override
parameter, which causes the override to be ignored when the skill is disabled in
config. Restructure the logic to apply pack_override before checking the enabled
flag, or modify the early return condition to only return None when both enabled
is False AND pack_override is not provided. This ensures that CLI/workflow
overrides via pack_override take effect regardless of the enabled configuration
setting.
In `@scripts/runner_lib/core.py`:
- Around line 317-324: There is a path traversal vulnerability where
checkout_path and rel_path values can escape workspace_path. Before calling
shutil.rmtree on destination_root at line 319, add a canonical path containment
check to ensure destination_root is actually contained within workspace_path
after resolving any symlinks and relative paths. Similarly, before each copy
operation in the loop (lines 321-324), validate that both the src and dst paths
are within their intended boundaries by comparing their canonical resolved
paths. Use Path.resolve() to get canonical paths and ensure they start with the
workspace boundaries before proceeding with the potentially dangerous
operations.
🪄 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: 63c259be-4831-4716-90eb-567ab5d1fd5e
📒 Files selected for processing (9)
.github/scripts/agent_delegation_policy.js.github/scripts/keepalive_loop.js.github/scripts/verifier_verdict_json.py.github/workflows/agents-71-codex-belt-dispatcher.yml.github/workflows/agents-72-codex-belt-worker-dispatch.yml.github/workflows/agents-72-codex-belt-worker.ymlscripts/check_agents_md_freshness.pyscripts/orchestrator_skill.pyscripts/runner_lib/core.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
stranske/Workflows(auto-detected)
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Python CI / python 3.12
- GitHub Check: Update keepalive summary
🧰 Additional context used
📓 Path-based instructions (5)
.github/workflows/**/*.yml
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
.github/workflows/**/*.yml: Do not add top-levelpermissions:block onworkflow_callreusable workflows - this conflicts with caller permissions
Avoid invalid YAML syntax, invalid permission scopes, and circular workflow references when creating GitHub Actions workflows
Files:
.github/workflows/agents-72-codex-belt-worker.yml.github/workflows/agents-71-codex-belt-dispatcher.yml.github/workflows/agents-72-codex-belt-worker-dispatch.yml
**/.github/workflows/*.yml
📄 CodeRabbit inference engine (CLAUDE.md)
First-party consumers should reference reusable workflows with
@main. Match that unless you are intentionally pinning to an exact commit SHA for a controlled reason.
Files:
.github/workflows/agents-72-codex-belt-worker.yml.github/workflows/agents-71-codex-belt-dispatcher.yml.github/workflows/agents-72-codex-belt-worker-dispatch.yml
**/{.github/workflows/agents-*.yml,.github/workflows/autofix.yml,.github/codex/**,scripts/**,docs/**}
📄 CodeRabbit inference engine (CLAUDE.md)
Agent workflow files (
agents-*.yml),autofix.yml,.github/codex/prompts, and synced scripts/docs should be fixed instranske/Workflows, not locally in the consumer repo.
Files:
.github/workflows/agents-72-codex-belt-worker.yml.github/workflows/agents-71-codex-belt-dispatcher.yml.github/workflows/agents-72-codex-belt-worker-dispatch.ymlscripts/check_agents_md_freshness.pyscripts/runner_lib/core.pyscripts/orchestrator_skill.py
.github/workflows/agents-*.yml
📄 CodeRabbit inference engine (AGENTS.md)
Agent workflow files (agents-*.yml) should be fixed in stranske/Workflows, not edited locally in the consumer repository
Files:
.github/workflows/agents-72-codex-belt-worker.yml.github/workflows/agents-71-codex-belt-dispatcher.yml.github/workflows/agents-72-codex-belt-worker-dispatch.yml
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
In Manager-Database repository, import Prefect 2.x schedules from
prefect.client.schemas.schedules
Files:
scripts/check_agents_md_freshness.pyscripts/runner_lib/core.pyscripts/orchestrator_skill.py
🪛 ast-grep (0.43.0)
scripts/check_agents_md_freshness.py
[info] 146-146: use jsonify instead of json.dumps for JSON output
Context: json.dumps({"findings": [finding.as_dict() for finding in findings]}, indent=2)
Note: Security best practice.
(use-jsonify)
[warning] 93-93: Do not make http calls without encryption
Context: "http://"
Note: [CWE-319].
(requests-http)
.github/scripts/verifier_verdict_json.py
[info] 106-106: use jsonify instead of json.dumps for JSON output
Context: json.dumps(verdict, sort_keys=True)
Note: Security best practice.
(use-jsonify)
scripts/orchestrator_skill.py
[info] 394-394: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload, separators=(",", ":"))
Note: Security best practice.
(use-jsonify)
🔀 Multi-repo context stranske/Workflows
Based on my exploration of the stranske/Workflows repository, I can now provide findings on the cross-repository impact of this PR.
Linked repositories findings
stranske/Workflows (Template Source)
This PR synchronizes template files that are deployed to consumer repositories via the .github/sync-manifest.yml manifest and maint-68-sync-consumer-repos.yml workflow.
Key consumers identified via the sync manifest:
agent_delegation_policy.js [::stranske/Workflows::]
- Called by:
.github/workflows/agents-keepalive-loop.yml(and consumer-synced workflows) - Breaking change detected:
detectStall()default threshold changed from3to2rounds - Impact: This makes agent:auto label delegation switch agents sooner when stall conditions are detected. Any keepalive workflow in consumer repos that calls
decideNextAgent()with the stalled agent policy will now escalate 1 round earlier. - Test evidence:
.github/scripts/__tests__/agent-delegation-policy.test.js(1704 files in repo include tests)
keepalive_loop.js [::stranske/Workflows::]
- Called by: Workflows agents-keepalive-loop, agents-keepalive-loop-reporter.yml, and health-keepalive-e2e.yml
- Breaking behavior change:
max_iterationshandling: Previously, a missing config value would default to5during normalization. Now it defaults to12during evaluation (hard budget enforcement).- Dispatch behavior changed from "soft cap" (allowing productive agents to continue) to "hard per-PR budget"
- Stop reason changed from
ready-extended/ productivity-based variants toround-budget-exhausted
- Test evidence: Multiple test files found:
.github/scripts/__tests__/keepalive-loop.test.jstests/keepalive-cascade.test.jstests/workflows/fixtures/keepalive_loop/harness.js
- Consumer impact: All synced consumer repos that use keepalive workflows will enforce a tighter iteration budget (12 instead of 5 or extended variants).
orchestrator_skill.py [::stranske/Workflows::]
- New file (100% addition): Validates exported Orchestrator skill context configuration
- Used by:
scripts/runner_lib/core.py- newmaterialize_orchestrator_skill()function- Workflows: Codex belt dispatcher/worker workflows now accept
orchestrator_skill_packandorchestrator_skill_enabledinputs
- Test coverage:
tests/scripts/test_orchestrator_skill.pyincludes validation tests - Consumer impact: Consumer repos receiving updated Codex belt worker/dispatcher workflows (agents-71, agents-72-worker, agents-72-worker-dispatch) will have new optional inputs for orchestrator skill. These have safe defaults (empty strings) so existing consumers won't break.
verifier_verdict_json.py [::stranske/Workflows::]
- New file (116 lines): Extracts structured verifier verdicts from Markdown output with tampering detection
- Integration: Part of agent/verifier post-merge pipeline
- Security feature: Detects and fails if verdict markers appear inside diff/patch blocks (diff-tamper detection)
- Consumer impact: Consumer repos synced with this file will gain post-merge verifier verdict extraction capability
Workflow input changes [::stranske/Workflows::]
Files synced to consumer repos:
agents-71-codex-belt-dispatcher.yml: Addedorchestrator_skill_pack,orchestrator_skill_enabledinputs (optional, default:'')agents-72-codex-belt-worker.yml: Added same inputs (optional, default:'')agents-72-codex-belt-worker-dispatch.yml: Added same inputs + changedmax_parallelfrom${{ fromJSON(inputs.max_parallel) }}to hard-coded1
Breaking change: agents-72-codex-belt-worker-dispatch.yml now hard-codes max_parallel to 1 instead of accepting it as a dispatch input. Any consumer workflow that was passing max_parallel > 1 to this worker dispatch will now be overridden.
Scripts with broad consumer impact [::stranske/Workflows::]
check_agents_md_freshness.py: Validates AGENTS.md playbook section (now synced to consumers)reference_packs.py&runner_lib/core.py: Handle orchestrator skill materialization alongside reference packs
Test fixtures evidence: The sync-manifest.yml file explicitly lists all files being synced with template_sync: exact markers for orchestrator_skill.py and other sensitive files, indicating these are critical for the agent orchestration pipeline.
🔇 Additional comments (18)
.github/scripts/verifier_verdict_json.py (5)
1-10: LGTM!
12-21: LGTM!
24-32: LGTM!
35-64: LGTM!
67-94: LGTM!.github/workflows/agents-71-codex-belt-dispatcher.yml (1)
23-35: LGTM!Also applies to: 79-91
.github/workflows/agents-72-codex-belt-worker.yml (1)
51-63: LGTM!.github/scripts/agent_delegation_policy.js (4)
87-87: LGTM!
227-233: LGTM!
263-263: LGTM!
280-282: LGTM!.github/scripts/keepalive_loop.js (7)
145-162: LGTM!
1614-1614: LGTM!
2524-2526: LGTM!
2618-2622: LGTM!
2683-2685: LGTM!
2781-2791: LGTM!
2797-2799: LGTM!
| parser.add_argument("--json", required=True, help="Destination verdict JSON path") | ||
| args = parser.parse_args() | ||
|
|
||
| output = Path(args.output).read_text(encoding="utf-8") if Path(args.output).is_file() else "" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider explicit error handling for path edge cases.
If args.output points to a directory (exists but not a file), this silently treats it as empty output, which could mask configuration errors in CI pipelines. An explicit error might be more helpful for debugging.
- output = Path(args.output).read_text(encoding="utf-8") if Path(args.output).is_file() else ""
+ output_path = Path(args.output)
+ if output_path.is_dir():
+ raise SystemExit(f"error: --output path is a directory: {args.output}")
+ output = output_path.read_text(encoding="utf-8") if output_path.is_file() else ""That said, if the current behavior of returning an error verdict with missing-structured-json source is intentional for graceful degradation in cases where verifier output doesn't exist, feel free to dismiss this.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| output = Path(args.output).read_text(encoding="utf-8") if Path(args.output).is_file() else "" | |
| output_path = Path(args.output) | |
| if output_path.is_dir(): | |
| raise SystemExit(f"error: --output path is a directory: {args.output}") | |
| output = output_path.read_text(encoding="utf-8") if output_path.is_file() else "" |
🤖 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 @.github/scripts/verifier_verdict_json.py at line 103, In the path handling
logic where args.output is read, add explicit error handling to catch the edge
case where args.output exists but is a directory rather than a file. Instead of
silently treating a directory path as empty output, raise an error or log a
warning that clearly indicates the configuration issue, so that CI pipeline
failures are not masked by ambiguous behavior. This ensures debugging in CI
environments is more straightforward when path configuration errors occur.
| max_parallel: 1 | ||
| keepalive: ${{ inputs.keepalive }} | ||
| orchestrator_skill_pack: ${{ inputs.orchestrator_skill_pack }} | ||
| orchestrator_skill_enabled: ${{ inputs.orchestrator_skill_enabled }} |
There was a problem hiding this comment.
max_parallel is now silently ignored by callers.
Line 77 hard-codes max_parallel: 1, overriding dispatch input-driven concurrency. This is a behavioral break for existing callers that pass values >1 and expect them to propagate to the worker.
Suggested fix (template-side)
- max_parallel: 1
+ max_parallel: ${{ fromJSON(inputs.max_parallel) }}As per coding guidelines, "**/{.github/workflows/agents-*.yml,...} and .github/workflows/agents-*.yml should be fixed in stranske/Workflows, not edited locally in the consumer repository."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| max_parallel: 1 | |
| keepalive: ${{ inputs.keepalive }} | |
| orchestrator_skill_pack: ${{ inputs.orchestrator_skill_pack }} | |
| orchestrator_skill_enabled: ${{ inputs.orchestrator_skill_enabled }} | |
| max_parallel: ${{ fromJSON(inputs.max_parallel) }} | |
| keepalive: ${{ inputs.keepalive }} | |
| orchestrator_skill_pack: ${{ inputs.orchestrator_skill_pack }} | |
| orchestrator_skill_enabled: ${{ inputs.orchestrator_skill_enabled }} |
🤖 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 @.github/workflows/agents-72-codex-belt-worker-dispatch.yml around lines 77 -
80, The max_parallel field on line 77 is hard-coded to 1, which silently
overrides any max_parallel input value passed by callers and breaks existing
workflows expecting higher concurrency values. Replace the hard-coded
max_parallel: 1 with a reference to the input parameter using the pattern ${{
inputs.max_parallel }} to propagate the caller's concurrency preference. Note
that per coding guidelines, this fix should be applied in the stranske/Workflows
repository where the agents-*.yml workflow templates are maintained, not edited
locally in this consumer repository.
Sources: Coding guidelines, Linked repositories
| def _path_exists(repo_root: Path, ref: str) -> bool: | ||
| return (repo_root / ref).exists() |
There was a problem hiding this comment.
Absolute path refs are incorrectly treated as valid local references.
On Line 63 and Line 72, joining repo_root / ref (or repo_root / command) allows absolute paths to bypass repo scoping, so host paths like /etc/hosts can be marked as valid and hide stale AGENTS.md refs.
Suggested fix
def _path_exists(repo_root: Path, ref: str) -> bool:
- return (repo_root / ref).exists()
+ path = Path(ref)
+ if path.is_absolute():
+ return False
+ return (repo_root / path).exists()
@@
def _command_exists(repo_root: Path, ref: str) -> bool:
@@
command = parts[0]
if command.startswith(("./", "../")) or "/" in command:
- return (repo_root / command).exists()
+ command_path = Path(command)
+ if command_path.is_absolute():
+ return False
+ return (repo_root / command_path).exists()
return shutil.which(command) is not NoneAlso 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 on line 63 and the similar code on lines 71-73 have a path traversal
vulnerability where absolute paths in the ref and command parameters bypass repo
scoping. When using the `/` operator with Path objects, absolute paths ignore
the left operand entirely, so a ref like "/etc/hosts" would resolve outside the
repo_root. Fix this by validating that ref and command are relative paths and do
not start with a forward slash, or by using methods like resolve() and
relative_to() to ensure the resulting path is within the repo_root directory
before checking if it exists.
| 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) |
There was a problem hiding this comment.
--agents-md is resolved relative to CWD instead of --repo-root.
Line 143 resolves args.agents_md against the process working directory, which can load the wrong file when the CLI is invoked outside the repo root.
Suggested fix
repo_root = args.repo_root.resolve()
- agents_md = args.agents_md.resolve() if args.agents_md else None
+ 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
findings = check_agents_md(repo_root, agents_md)🤖 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
args.agents_md path is being resolved relative to the current working directory
instead of relative to repo_root, which causes incorrect file loading when the
script is invoked outside the repo directory. In the assignment where agents_md
is set on line 143, change the resolution logic so that args.agents_md is
resolved relative to repo_root (which is already resolved on line 142) instead
of being resolved independently, ensuring the path is correctly anchored to the
repository root directory.
| def _validate_repo(repo: str) -> str: | ||
| if "/" not in repo or repo.startswith("/") or repo.endswith("/"): | ||
| raise OrchestratorSkillConfigError("repo must use owner/name format") | ||
| return repo |
There was a problem hiding this comment.
repo validation is too permissive for the declared owner/name contract.
Current validation accepts values with multiple slashes (e.g., org/team/repo). This defers a config error to runtime checkout instead of failing fast during validation.
Proposed fix
def _validate_repo(repo: str) -> str:
- if "/" not in repo or repo.startswith("/") or repo.endswith("/"):
+ if repo.count("/") != 1 or repo.startswith("/") or repo.endswith("/"):
raise OrchestratorSkillConfigError("repo must use owner/name format")
return repo📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _validate_repo(repo: str) -> str: | |
| if "/" not in repo or repo.startswith("/") or repo.endswith("/"): | |
| raise OrchestratorSkillConfigError("repo must use owner/name format") | |
| return repo | |
| def _validate_repo(repo: str) -> str: | |
| if repo.count("/") != 1 or repo.startswith("/") or repo.endswith("/"): | |
| raise OrchestratorSkillConfigError("repo must use owner/name format") | |
| return repo |
🤖 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/orchestrator_skill.py` around lines 82 - 85, The `_validate_repo`
function currently only checks that a forward slash exists and that the string
does not start or end with one, but this allows multiple slashes (e.g.,
org/team/repo) which violates the declared owner/name format contract. Update
the validation logic in `_validate_repo` to ensure there is exactly one forward
slash in the repo string by counting the occurrences of "/" and rejecting any
value that does not have exactly one slash separator.
| if not enabled: | ||
| return None | ||
|
|
||
| if pack_override: | ||
| if plan is None: | ||
| plan = OrchestratorSkillCheckoutPlan( | ||
| repo="", | ||
| ref="", | ||
| paths=[], | ||
| checkout_path=DEFAULT_CHECKOUT_PATH, | ||
| pack=pack_override, | ||
| ) | ||
| else: | ||
| plan = OrchestratorSkillCheckoutPlan( | ||
| repo=plan.repo, | ||
| ref=plan.ref, | ||
| paths=list(plan.paths), | ||
| checkout_path=plan.checkout_path, | ||
| pack=pack_override, | ||
| ) | ||
|
|
||
| return plan |
There was a problem hiding this comment.
pack_override is dropped when config exists but is disabled.
resolve_orchestrator_skill_plan() returns early on enabled == False before applying pack_override. That contradicts the intended override precedence and prevents CLI/workflow override from taking effect unless enabled_override=true is also passed.
Proposed fix
- if not enabled:
- return None
-
if pack_override:
if plan is None:
plan = OrchestratorSkillCheckoutPlan(
repo="",
ref="",
paths=[],
checkout_path=DEFAULT_CHECKOUT_PATH,
pack=pack_override,
)
+ enabled = True
else:
plan = OrchestratorSkillCheckoutPlan(
repo=plan.repo,
ref=plan.ref,
paths=list(plan.paths),
checkout_path=plan.checkout_path,
pack=pack_override,
)
+ if not enabled:
+ return None
+
return plan🤖 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/orchestrator_skill.py` around lines 221 - 242, The issue is that
resolve_orchestrator_skill_plan() returns early when enabled is False, before
processing the pack_override parameter, which causes the override to be ignored
when the skill is disabled in config. Restructure the logic to apply
pack_override before checking the enabled flag, or modify the early return
condition to only return None when both enabled is False AND pack_override is
not provided. This ensures that CLI/workflow overrides via pack_override take
effect regardless of the enabled configuration setting.
| destination_root = workspace_path / checkout_path | ||
| if destination_root.exists(): | ||
| shutil.rmtree(destination_root) | ||
| destination_root.mkdir(parents=True, exist_ok=True) | ||
| for rel_path in paths: | ||
| src = clone_dir / rel_path | ||
| dst = destination_root / rel_path | ||
| if src.is_dir(): |
There was a problem hiding this comment.
Constrain checkout and copied paths to stay inside the workspace.
Line 317 and Line 323 trust checkout_path/rel_path at the sink. A crafted value can escape workspace_path, and Line 319 could delete unintended directories before copy. Add canonical path containment checks before rmtree and before each copy target/source resolution.
Suggested fix
- destination_root = workspace_path / checkout_path
+ destination_root = (workspace_path / checkout_path).resolve()
+ try:
+ destination_root.relative_to(workspace_path.resolve())
+ except ValueError as exc:
+ raise ValueError(
+ f"orchestrator checkout_path must stay under workspace: {checkout_path}"
+ ) from exc
if destination_root.exists():
shutil.rmtree(destination_root)
destination_root.mkdir(parents=True, exist_ok=True)
for rel_path in paths:
- src = clone_dir / rel_path
- dst = destination_root / rel_path
+ rel = Path(rel_path)
+ if rel.is_absolute() or ".." in rel.parts:
+ raise ValueError(f"invalid orchestrator path entry: {rel_path}")
+ src = (clone_dir / rel).resolve()
+ dst = (destination_root / rel).resolve()
+ try:
+ src.relative_to(clone_dir.resolve())
+ dst.relative_to(destination_root)
+ except ValueError as exc:
+ raise ValueError(f"path escapes checkout boundaries: {rel_path}") from exc📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| destination_root = workspace_path / checkout_path | |
| if destination_root.exists(): | |
| shutil.rmtree(destination_root) | |
| destination_root.mkdir(parents=True, exist_ok=True) | |
| for rel_path in paths: | |
| src = clone_dir / rel_path | |
| dst = destination_root / rel_path | |
| if src.is_dir(): | |
| destination_root = (workspace_path / checkout_path).resolve() | |
| try: | |
| destination_root.relative_to(workspace_path.resolve()) | |
| except ValueError as exc: | |
| raise ValueError( | |
| f"orchestrator checkout_path must stay under workspace: {checkout_path}" | |
| ) from exc | |
| if destination_root.exists(): | |
| shutil.rmtree(destination_root) | |
| destination_root.mkdir(parents=True, exist_ok=True) | |
| for rel_path in paths: | |
| rel = Path(rel_path) | |
| if rel.is_absolute() or ".." in rel.parts: | |
| raise ValueError(f"invalid orchestrator path entry: {rel_path}") | |
| src = (clone_dir / rel).resolve() | |
| dst = (destination_root / rel).resolve() | |
| try: | |
| src.relative_to(clone_dir.resolve()) | |
| dst.relative_to(destination_root) | |
| except ValueError as exc: | |
| raise ValueError(f"path escapes checkout boundaries: {rel_path}") from exc | |
| if src.is_dir(): |
🤖 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 317 - 324, There is a path traversal
vulnerability where checkout_path and rel_path values can escape workspace_path.
Before calling shutil.rmtree on destination_root at line 319, add a canonical
path containment check to ensure destination_root is actually contained within
workspace_path after resolving any symlinks and relative paths. Similarly,
before each copy operation in the loop (lines 321-324), validate that both the
src and dst paths are within their intended boundaries by comparing their
canonical resolved paths. Use Path.resolve() to get canonical paths and ensure
they start with the workspace boundaries before proceeding with the potentially
dangerous operations.
1 participant
Sync Summary
Files Updated
Files Skipped
Review Checklist
Source: stranske/Workflows
Source SHA:
2baf871360a6ebf83c2b24dd7eafd94d7f262733Template hash:
97e42ef43aa5Sync branch:
sync/workflows-97e42ef43aa5Consumer repo:
stranske/Counter_RiskManifest:
.github/sync-manifest.ymlSummary by CodeRabbit
New Features
Enhancements