chore: sync workflow templates - #836
Conversation
Automated sync from stranske/Workflows Template hash: 7456dc211466 Changes synced from sync-manifest.yml
📝 WalkthroughWalkthroughThis PR tightens the agent delegation policy's stall detection (threshold lowered from 3 to 2, effectiveness now requires gate=pass alongside commits) and the keepalive loop's iteration budget (removes productive-agent extension, adds ChangesAgent Loop Control Tightening
Orchestrator Skill Feature
Verifier Verdict Parser
Sequence Diagram(s)sequenceDiagram
participant Workflow as Belt Workflow
participant assemble_prompt
participant materialize_orchestrator_skill
participant resolve_orchestrator_skill_plan as resolve_plan
participant _materialize_single_checkout_plan as sparse_clone
participant write_orchestrator_skill_summary as write_summary
Workflow->>assemble_prompt: orchestrator_skill_pack, orchestrator_skill_enabled, token
assemble_prompt->>materialize_orchestrator_skill: pack_override, enabled_override, token
materialize_orchestrator_skill->>resolve_plan: workspace, overrides
resolve_plan-->>materialize_orchestrator_skill: OrchestratorSkillCheckoutPlan | None
alt plan.pack is set
materialize_orchestrator_skill->>materialize_orchestrator_skill: materialize_reference_packs(pack)
else inline checkout
materialize_orchestrator_skill->>sparse_clone: repo, ref, paths, token
sparse_clone-->>materialize_orchestrator_skill: checkout_path
end
materialize_orchestrator_skill->>write_summary: checkout_path, pack_name
write_summary-->>materialize_orchestrator_skill: .reference/ORCHESTRATOR_SKILL.md
materialize_orchestrator_skill-->>assemble_prompt: summary_path
assemble_prompt->>assemble_prompt: append ORCHESTRATOR_SKILL.md to prompt
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Automated Status SummaryHead SHA: 7d2789d
Coverage Overview
Coverage Trend
Top Coverage Hotspots (lowest coverage)
Updated automatically; will refresh on subsequent CI/Docker completions. Keepalive checklistScopeNo scope information available Tasks
Acceptance criteria
|
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 86: The "needs_attention" field assignment at line 86 incorrectly uses
bool() on candidate.get() which misinterprets string values like "false" as True
since any non-empty string is truthy in Python. Replace the bool() coercion with
explicit string comparison to check if the value equals "true"
(case-insensitive) or use proper JSON boolean coercion instead of relying on
Python's truthiness evaluation. This ensures that string values from JSON are
properly converted to their intended boolean representation.
In `@scripts/orchestrator_skill.py`:
- Around line 206-223: The issue is that when a config file has "enabled":
false, the parse_orchestrator_skill_config_text function returns plan=None, and
subsequently setting enabled_override=True only flips the enabled flag but
leaves plan=None, causing the function to still return None. To fix this, after
applying the enabled_override in the section where enabled = enabled_override is
set, add logic to also restore or create the plan when enabled_override=True and
plan is currently None. Check if pack_override is available and use it to create
a plan with the same structure as in the elif pack_override block, ensuring that
both enabled and plan are properly set when the override is applied.
In `@scripts/runner_lib/core.py`:
- Around line 944-945: The orchestrator_skill_pack field in the context
dictionary at line 944 does not normalize whitespace-only input from
args.orchestrator_skill_pack, causing whitespace strings to be treated as valid
pack overrides that later fail with "pack not found". Normalize the
args.orchestrator_skill_pack value by stripping whitespace before assigning it
to the orchestrator_skill_pack key, ensuring that whitespace-only strings are
converted to None just like other override values are parsed.
- Around line 321-329: The copy operations in the file copy loop for clone_dir
paths are vulnerable to symlink escape attacks because shutil.copytree and
shutil.copy2 can follow symlinks by default. Add a check for symlinks using
is_symlink() before processing each path in the loop, and reject or skip any
symlink paths to prevent malicious symlinks from copying content outside
clone_dir. Additionally, add follow_symlinks=False parameter to the shutil.copy2
call and ensure symlinks=False is set (or explicit handling) in the
shutil.copytree call to prevent symlink traversal during the copy 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: 43ef96ed-c17e-41c0-b65c-0ee51c08c0b5
📒 Files selected for processing (8)
.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/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
🧰 Additional context used
📓 Path-based instructions (5)
.github/workflows/**/*.{yml,yaml}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
.github/workflows/**/*.{yml,yaml}: Ensure coverage threshold settings in GitHub Actions workflow files forcoverage-minmatch the[tool.coverage.report] fail_undersetting inpyproject.toml, as the lower value will be the effective threshold
Forstartup_failurein GitHub Actions workflows with zero jobs, check for invalid YAML syntax, top-levelpermissions:blocks inworkflow_callreusable workflows (which conflicts with caller permissions), invalid permission scopes, or circular workflow references
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)
Reference reusable workflows with
@mainunless 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/**/*.yml
📄 CodeRabbit inference engine (AGENTS.md)
First-party consumers should reference reusable workflows with
@mainunless 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
📄 CodeRabbit inference engine (AGENTS.md)
Agent workflows (
agents-*.yml), autofix.yml, synced prompts in.github/codex/, and synced scripts/docs should be fixed in stranske/Workflows, not 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 using Prefect 2.x, import schedules from
prefect.client.schemas.schedulesrather than other locations
Files:
scripts/runner_lib/core.pyscripts/orchestrator_skill.py
🪛 ast-grep (0.43.0)
.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
Linked repositories findings
stranske/Workflows
Integration of orchestrator_skill.py with agent workflows:
- [::stranske/Workflows::]
scripts/runner_lib/core.py— imports and callsorchestrator_skill.resolve_orchestrator_skill_plan()andorchestrator_skill.write_orchestrator_skill_summary()withinmaterialize_orchestrator_skill()function, which is used during prompt assembly. - [::stranske/Workflows::]
.github/workflows/agents-71-codex-belt-dispatcher.yml— exposesorchestrator_skill_packandorchestrator_skill_enabledinputs (lines 29-38) that are intended to override orchestrator skill context for downstream Codex runs. - [::stranske/Workflows::]
.github/workflows/agents-72-codex-belt-worker.yml— accepts the sameorchestrator_skill_packandorchestrator_skill_enabledinputs, which are forwarded through prompt assembly. - [::stranske/Workflows::]
.github/workflows/agents-72-codex-belt-worker-dispatch.yml— wrapper that passesorchestrator_skill_packandorchestrator_skill_enabledto the worker workflow. - [::stranske/Workflows::]
tests/scripts/test_runner_lib.py— testtest_assemble_prompt_includes_orchestrator_skill_section_when_summary_existsconfirms integration with prompt assembly whenORCHESTRATOR_SKILL.mdis present.
Keepalive loop behavior changes affecting agent dispatch:
- [::stranske/Workflows::]
.github/scripts/keepalive_loop.js— containstoPositiveInteger()helper and updated max-iterations resolution logic. TheshouldStopForMaxIterationscondition now always triggers when iteration cap is reached, with explicit stop reasonround-budget-exhausted(lines 2622-2685). - [::stranske/Workflows::]
.github/workflows/agents-72-codex-belt-worker.yml:407-420— evaluateskeepalive_worker_gatewheninputs.keepalive == true, directly integrating with keepalive loop state. - [::stranske/Workflows::]
tests/keepalive-cascade.test.js,tests/keepalive-gate-failure-counter.test.js— have test coverage for keepalive loop behavior.
Agent delegation policy stall detection sensitivity:
- [::stranske/Workflows::]
.github/scripts/agent_delegation_policy.js— containsdecideNextAgent()function that callsdetectStall()and applies effectiveness calculation logic with threshold of2(per AI summary change from3). - [::stranske/Workflows::]
.github/scripts/__tests__/agent-delegation-policy.test.js— 12KB test file providing coverage for delegation logic.
Verifier verdict JSON parsing:
- [::stranske/Workflows::]
.github/scripts/verifier_verdict_json.py— new module synced in manifest that parses Markdown output to extract structured verdicts, preventing diff-text tampering. - [::stranske/Workflows::]
tests/workflows/test_verifier_verdict_parsing.py— comprehensive test coverage includingtest_diff_block_with_inner_backticks_still_tamper()and verdict schema validation.
Sync manifest confirms all changed files are intentionally synced to consumers:
- [::stranske/Workflows::]
.github/sync-manifest.yml— documents each file in thescripts:section:scripts/orchestrator_skill.py— "Validates and resolves exported Orchestrator skill context for remote Codex lanes".github/scripts/agent_delegation_policy.js— "Agent delegation policy - system-driven routing for agent:auto label".github/scripts/keepalive_loop.js— "Core keepalive loop logic".github/scripts/verifier_verdict_json.py— "Extracts structured post-merge verifier verdict JSON without trusting diff text".github/workflows/agents-71-codex-belt-dispatcher.yml— "Codex belt dispatcher - selects issues and creates agent branches for work".github/workflows/agents-72-codex-belt-worker.yml— "Codex belt worker - executes agent on issues with full prompt and context".github/workflows/agents-72-codex-belt-worker-dispatch.yml— "Codex belt worker dispatch wrapper - allows workflow_dispatch for the worker"
Cross-file dependencies:
- [::stranske/Workflows::]
scripts/runner_lib/core.py— calls_load_orchestrator_skill_module()which importsscripts.orchestrator_skill, establishing hard dependency. Failure to syncorchestrator_skill.pycausesRuntimeErrorwith message "scripts/orchestrator_skill.py was not synced". - [::stranske/Workflows::]
reusable-codex-run.yml— invokespython -m scripts.runner_lib assemble-promptwith flags like--materialize-orchestrator-skill, which depends on orchestrator_skill module being present.
🔇 Additional comments (13)
.github/scripts/verifier_verdict_json.py (1)
12-85: LGTM!Also applies to: 89-117
.github/scripts/agent_delegation_policy.js (3)
87-87: LGTM!Also applies to: 263-263
227-233: LGTM!
281-282: LGTM!.github/scripts/keepalive_loop.js (6)
145-162: LGTM!
1614-1614: LGTM!
2524-2526: LGTM!
2618-2622: LGTM!
2683-2685: LGTM!
2799-2799: 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/workflows/agents-72-codex-belt-worker-dispatch.yml (1)
45-57: LGTM!Also applies to: 77-80
| **candidate, | ||
| "verdict": verdict, | ||
| "source": "structured-json", | ||
| "needs_attention": bool(candidate.get("needs_attention", verdict != "pass")), |
There was a problem hiding this comment.
Coerce needs_attention explicitly instead of using bool() on arbitrary JSON values
At Line 86, bool(candidate.get("needs_attention", ...)) misinterprets string values like "false" as True. That can flip verifier routing/gating behavior.
Suggested fix
+def _coerce_optional_bool(value: object) -> bool | None:
+ if isinstance(value, bool):
+ return value
+ if isinstance(value, str):
+ normalized = value.strip().lower()
+ if normalized in {"true", "1", "yes", "on"}:
+ return True
+ if normalized in {"false", "0", "no", "off"}:
+ return False
+ return None
+
@@
- return {
+ parsed_attention = _coerce_optional_bool(candidate.get("needs_attention"))
+ return {
**candidate,
"verdict": verdict,
"source": "structured-json",
- "needs_attention": bool(candidate.get("needs_attention", verdict != "pass")),
+ "needs_attention": parsed_attention if parsed_attention is not None else (verdict != "pass"),
}📝 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.
| "needs_attention": bool(candidate.get("needs_attention", verdict != "pass")), | |
| def _coerce_optional_bool(value: object) -> bool | None: | |
| if isinstance(value, bool): | |
| return value | |
| if isinstance(value, str): | |
| normalized = value.strip().lower() | |
| if normalized in {"true", "1", "yes", "on"}: | |
| return True | |
| if normalized in {"false", "0", "no", "off"}: | |
| return False | |
| return None | |
| parsed_attention = _coerce_optional_bool(candidate.get("needs_attention")) | |
| return { | |
| **candidate, | |
| "verdict": verdict, | |
| "source": "structured-json", | |
| "needs_attention": parsed_attention if parsed_attention is not None else (verdict != "pass"), | |
| } |
🤖 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 86, The "needs_attention"
field assignment at line 86 incorrectly uses bool() on candidate.get() which
misinterprets string values like "false" as True since any non-empty string is
truthy in Python. Replace the bool() coercion with explicit string comparison to
check if the value equals "true" (case-insensitive) or use proper JSON boolean
coercion instead of relying on Python's truthiness evaluation. This ensures that
string values from JSON are properly converted to their intended boolean
representation.
| if config_text is not None: | ||
| enabled, plan = parse_orchestrator_skill_config_text(config_text, config_path) | ||
| elif pack_override: | ||
| enabled = enabled_override is not False | ||
| plan = OrchestratorSkillCheckoutPlan( | ||
| repo="", | ||
| ref="", | ||
| paths=[], | ||
| checkout_path=DEFAULT_CHECKOUT_PATH, | ||
| pack=pack_override, | ||
| ) | ||
|
|
||
| if enabled_override is not None: | ||
| enabled = enabled_override | ||
|
|
||
| if not enabled: | ||
| return None | ||
|
|
There was a problem hiding this comment.
enabled_override=true cannot re-enable configs that are disabled in JSON.
When the file has "enabled": false, parse_orchestrator_skill_config_text returns plan=None. Later setting enabled_override=True flips enabled but still returns None, so the override cannot enable existing inline/pack config.
Suggested fix
-def parse_orchestrator_skill_config(
- payload: Any,
-) -> tuple[bool, OrchestratorSkillCheckoutPlan | None]:
+def parse_orchestrator_skill_config(
+ payload: Any,
+ *,
+ ignore_enabled: bool = False,
+) -> tuple[bool, OrchestratorSkillCheckoutPlan | None]:
@@
- if not enabled:
+ if not enabled and not ignore_enabled:
return False, None
@@
def resolve_orchestrator_skill_plan(...):
@@
- if config_text is not None:
- enabled, plan = parse_orchestrator_skill_config_text(config_text, config_path)
+ if config_text is not None:
+ payload = json.loads(config_text)
+ enabled, plan = parse_orchestrator_skill_config(payload)
+ if enabled_override is True and plan is None:
+ _, plan = parse_orchestrator_skill_config(payload, ignore_enabled=True)🤖 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 206 - 223, The issue is that when
a config file has "enabled": false, the parse_orchestrator_skill_config_text
function returns plan=None, and subsequently setting enabled_override=True only
flips the enabled flag but leaves plan=None, causing the function to still
return None. To fix this, after applying the enabled_override in the section
where enabled = enabled_override is set, add logic to also restore or create the
plan when enabled_override=True and plan is currently None. Check if
pack_override is available and use it to create a plan with the same structure
as in the elif pack_override block, ensuring that both enabled and plan are
properly set when the override is applied.
| for rel_path in paths: | ||
| src = clone_dir / rel_path | ||
| dst = destination_root / rel_path | ||
| if src.is_dir(): | ||
| shutil.copytree(src, dst, dirs_exist_ok=True) | ||
| elif src.is_file(): | ||
| dst.parent.mkdir(parents=True, exist_ok=True) | ||
| shutil.copy2(src, dst) | ||
| else: |
There was a problem hiding this comment.
Harden copy step against symlink escape from cloned content.
shutil.copytree/shutil.copy2 can follow symlinks. A malicious symlink in checked-out paths can copy content outside clone_dir into workspace.
Suggested fix
for rel_path in paths:
src = clone_dir / rel_path
dst = destination_root / rel_path
+ if src.is_symlink():
+ raise ValueError(f"symlinked orchestrator skill path is not allowed: {rel_path}")
if src.is_dir():
- shutil.copytree(src, dst, dirs_exist_ok=True)
+ shutil.copytree(src, dst, dirs_exist_ok=True, symlinks=True)
elif src.is_file():
dst.parent.mkdir(parents=True, exist_ok=True)
- shutil.copy2(src, dst)
+ shutil.copy2(src, dst, follow_symlinks=False)📝 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.
| for rel_path in paths: | |
| src = clone_dir / rel_path | |
| dst = destination_root / rel_path | |
| if src.is_dir(): | |
| shutil.copytree(src, dst, dirs_exist_ok=True) | |
| elif src.is_file(): | |
| dst.parent.mkdir(parents=True, exist_ok=True) | |
| shutil.copy2(src, dst) | |
| else: | |
| for rel_path in paths: | |
| src = clone_dir / rel_path | |
| dst = destination_root / rel_path | |
| if src.is_symlink(): | |
| raise ValueError(f"symlinked orchestrator skill path is not allowed: {rel_path}") | |
| if src.is_dir(): | |
| shutil.copytree(src, dst, dirs_exist_ok=True, symlinks=True) | |
| elif src.is_file(): | |
| dst.parent.mkdir(parents=True, exist_ok=True) | |
| shutil.copy2(src, dst, follow_symlinks=False) | |
| 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 `@scripts/runner_lib/core.py` around lines 321 - 329, The copy operations in
the file copy loop for clone_dir paths are vulnerable to symlink escape attacks
because shutil.copytree and shutil.copy2 can follow symlinks by default. Add a
check for symlinks using is_symlink() before processing each path in the loop,
and reject or skip any symlink paths to prevent malicious symlinks from copying
content outside clone_dir. Additionally, add follow_symlinks=False parameter to
the shutil.copy2 call and ensure symlinks=False is set (or explicit handling) in
the shutil.copytree call to prevent symlink traversal during the copy
operations.
| "orchestrator_skill_pack": args.orchestrator_skill_pack or None, | ||
| "orchestrator_skill_enabled": _parse_optional_bool(args.orchestrator_skill_enabled), |
There was a problem hiding this comment.
Normalize orchestrator_skill_pack before placing it in context.
Whitespace-only input currently becomes a truthy pack override and fails later as "pack not found". Trim here to keep behavior consistent with other override parsing.
Suggested fix
- "orchestrator_skill_pack": args.orchestrator_skill_pack or None,
+ "orchestrator_skill_pack": (args.orchestrator_skill_pack or "").strip() or 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/runner_lib/core.py` around lines 944 - 945, The
orchestrator_skill_pack field in the context dictionary at line 944 does not
normalize whitespace-only input from args.orchestrator_skill_pack, causing
whitespace strings to be treated as valid pack overrides that later fail with
"pack not found". Normalize the args.orchestrator_skill_pack value by stripping
whitespace before assigning it to the orchestrator_skill_pack key, ensuring that
whitespace-only strings are converted to None just like other override values
are parsed.
Sync Summary
Files Updated
Files Skipped
Review Checklist
Source: stranske/Workflows
Source SHA:
509eceb28eb49f48ca51d40b77841cf7acf701d6Template hash:
7456dc211466Sync branch:
sync/workflows-7456dc211466Consumer repo:
stranske/TemplateManifest:
.github/sync-manifest.ymlSummary by CodeRabbit
New Features
Improvements