chore: sync workflow templates - #1431
Conversation
Automated sync from stranske/Workflows Template hash: 7456dc211466 Changes synced from sync-manifest.yml
stranske
added
sync
✅ Deploy Preview for stranske-trip-planner canceled.
|
GitHub Actions
Failure
📝 WalkthroughWalkthroughThe PR introduces an Orchestrator Skill feature enabling sparse-checkout materialization of external reference repositories into runner prompts, wired through three GitHub Actions workflows. It also tightens agent delegation effectiveness/stall thresholds, converts the keepalive loop iteration limit to an unconditional hard cap, and adds a tamper-resistant verifier verdict JSON parser script. ChangesOrchestrator Skill Feature
Agent Control Flow Tightening
Verifier Verdict JSON Parser
Sequence Diagram(s)sequenceDiagram
participant WorkflowDispatch as Dispatcher Workflow
participant WorkerDispatch as Worker-Dispatch Workflow
participant Worker as Worker Workflow
participant assemble_prompt as assemble_prompt()
participant materialize_orchestrator_skill as materialize_orchestrator_skill()
participant GitSparseCheckout as git sparse-checkout
participant ORCHESTRATOR_SKILL_MD as ORCHESTRATOR_SKILL.md
WorkflowDispatch->>WorkerDispatch: orchestrator_skill_pack, orchestrator_skill_enabled
WorkerDispatch->>Worker: orchestrator_skill_pack, orchestrator_skill_enabled, max_parallel=1
Worker->>assemble_prompt: --orchestrator-skill-pack, --orchestrator-skill-enabled, --materialize-orchestrator-skill
assemble_prompt->>materialize_orchestrator_skill: pack_override, enabled_override, token
materialize_orchestrator_skill->>GitSparseCheckout: clone repo@ref, sparse paths
GitSparseCheckout-->>materialize_orchestrator_skill: checkout path
materialize_orchestrator_skill->>ORCHESTRATOR_SKILL_MD: write .reference/ORCHESTRATOR_SKILL.md
ORCHESTRATOR_SKILL_MD-->>assemble_prompt: file contents
assemble_prompt->>assemble_prompt: append "Orchestrator Skill Context" section 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/scripts/agent_delegation_policy.js (1)
223-233:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPair the passing gate with the committed round.
gatePassedis aggregated independently fromcommits, so history like “old green gate with no commits” plus “new commit with failing/pending gate” still becomes effective and bypasses stall switching. Require a verified commit signal instead of combining unrelated rounds.🐛 Proposed fix
const commits = recentRounds.reduce((sum, round) => sum + (round.commits || 0), 0); const tasks = recentRounds.reduce((sum, round) => sum + (round.tasks || 0), 0); const gatePassed = recentRounds.some((round) => round.gate === 'pass'); + const hasVerifiedCommit = recentRounds.some( + (round) => (round.commits || 0) > 0 && round.gate === 'pass' + ); // Agent is effective only when it produced verified forward motion: // - Completed at least 1 task in the lookback window, OR - // - Made commits and has a green Gate signal in the lookback window. + // - Made commits in a round with a green Gate signal. // Bare commits with no checkbox progress and a non-green Gate are churn, not // progress; otherwise an agent can commit indefinitely without advancing // acceptance criteria or CI and never trip delegation. - const effective = tasks >= 1 || (commits >= 1 && gatePassed); + const effective = tasks >= 1 || hasVerifiedCommit;🤖 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/agent_delegation_policy.js around lines 223 - 233, The current logic independently checks if any round has a passing gate while aggregating commits separately, allowing unrelated signals from different rounds to be combined (e.g., an old green gate paired with a new commit from a failing round). Instead of using the independent gatePassed boolean in the effective calculation, modify the logic to check if at least one round in recentRounds contains both a commit (commits property greater than 0) and a passing gate (gate === 'pass') in the same round, and use that paired condition in the effectiveness check to ensure the gate signal and commits actually come from the same round.
🤖 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:
- Around line 12-15: The VERDICT_RE regex pattern at the top of the file only
matches "pass" and "fail" explicitly, but the code recognizes four valid
verdicts: "pass", "concerns", "fail", and "error". This allows an attacker to
embed unmatched verdict strings like "verdict: concerns" or "verdict: error" in
diff regions and bypass the tampering detection check at line 69. Update the
VERDICT_RE regex to match all four valid verdict strings by either explicitly
including all of them in the alternation pattern or by dynamically constructing
the pattern from the VALID_VERDICTS list using re.escape to ensure proper
escaping.
In `@scripts/orchestrator_skill.py`:
- Around line 206-242: When enabled_override is True and re-enables a config
that was previously disabled (where plan is None from the
parse_orchestrator_skill_config_text call), the function still returns None
because plan was never initialized. After applying the enabled_override check
around line 216, add logic to detect when enabled is True but plan is None, and
in that case create a default OrchestratorSkillCheckoutPlan with sensible
defaults (matching the pattern already used in the pack_override blocks with
empty repo, ref, and paths, and DEFAULT_CHECKOUT_PATH) to ensure the function
returns a valid plan when a disabled config is re-enabled via override.
---
Outside diff comments:
In @.github/scripts/agent_delegation_policy.js:
- Around line 223-233: The current logic independently checks if any round has a
passing gate while aggregating commits separately, allowing unrelated signals
from different rounds to be combined (e.g., an old green gate paired with a new
commit from a failing round). Instead of using the independent gatePassed
boolean in the effective calculation, modify the logic to check if at least one
round in recentRounds contains both a commit (commits property greater than 0)
and a passing gate (gate === 'pass') in the same round, and use that paired
condition in the effectiveness check to ensure the gate signal and commits
actually come from the same round.
🪄 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: 95316c0c-174b-46c8-b758-a61712476b06
📒 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)stranske/Template(auto-detected)
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: Python CI / python 3.13
- GitHub Check: Python CI / python 3.12
- GitHub Check: Python CI / typecheck-mypy
- GitHub Check: Cross-Repo Smoke / cross-repo-full-product
- GitHub Check: Runtime CI
🧰 Additional context used
📓 Path-based instructions (7)
{pyproject.toml,.github/workflows/**/*.{yml,yaml}}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Check both
pyproject.toml([tool.coverage.report] fail_under) AND workflow files forcoverage-minsettings; ensure these match or the lower value will be the effective threshold
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,yaml}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
For GitHub Actions workflow
startup_failureerrors with zero jobs, check for invalid YAML syntax, top-levelpermissions:block onworkflow_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)
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 (CLAUDE.md)
agents-*.yml, autofix.yml, .github/codex/ prompts, and synced scripts/docs should be fixed in stranske/Workflows, not 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.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 documented 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/**,docs/**}
📄 CodeRabbit inference engine (AGENTS.md)
Synced workflows (agents-*.yml, autofix.yml), prompts in .github/codex/, and synced scripts/docs should be fixed in stranske/Workflows, not locally; do not edit them locally
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 the Manager-Database repository (stranske/Manager-Database), use Prefect 2.x and import schedules from
prefect.client.schemas.schedules
Files:
scripts/orchestrator_skill.pyscripts/runner_lib/core.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/Template, stranske/Workflows
Linked repositories findings
Based on my exploration of the stranske/Workflows and stranske/Template repositories, here are the cross-repository context findings relevant to this sync PR:
stranske/Template (consumer of synced changes)
Files that will be overwritten by this sync:
-
.github/scripts/agent_delegation_policy.js[::stranske/Template::]- Current consumers: Exported functions (
decideNextAgent,calculateEffectiveness,detectStall,checkPrerequisites,getExplicitAgentFromLabels,formatDelegationSummary) are part of the agent delegation system. - Breaking change impact: The default
detectStallthreshold changes from 3 to 2, making stall detection trigger after 2 zero-progress rounds instead of 3. ThecalculateEffectivenessfunction now requiresgate: 'pass'for commits to count—commits without a passing gate are no longer considered progress. - Evidence: stranske/Workflows tests validate this behavior: "stalls after two zero-progress rounds" test expects
threshold: 2to trigger stall detection, and "churn without checkbox progress is not effective" test confirms thatgate: 'fail'orgate: 'pending'commits don't count as effectiveness.
- Current consumers: Exported functions (
-
.github/scripts/keepalive_loop.js[::stranske/Template::]- Current consumers: Exported
evaluateKeepaliveLoopfunction and state management functions are core to the keepalive automation loop. - Breaking change impact: Iteration budget enforcement changed from a "soft cap" (productivity-gated, allowing productive agents to continue) to a "hard cap" (unconditional stop at budget). The stop reason label changed from unspecified to
round-budget-exhausted. ThetoPositiveIntegerfunction is newly introduced for stricter integer parsing ofmax_iterations. - Evidence: stranske/Workflows tests show "evaluateKeepaliveLoop stops at max iterations even when productive with tasks remaining" and "stops at max iterations before fixable gate failures"—both expect
reason: 'round-budget-exhausted'regardless of productivity.
- Current consumers: Exported
Files being added (first sync):
-
scripts/orchestrator_skill.py[::stranske/Workflows::]- New public API: Introduces
OrchestratorSkillConfigErrorexception,OrchestratorSkillCheckoutPlanandOrchestratorSkillSnapshotdataclasses, and functions likeload_orchestrator_skill(),resolve_orchestrator_skill_plan(),materialize_orchestrator_skill(). - Integration point: stranske/Template's
scripts/runner_lib/core.pywill need to import and integrate this module. The new infrastructure validates.github/orchestrator_skill.jsonconfig.
- New public API: Introduces
-
scripts/runner_lib/core.py[::stranske/Workflows::]- New function:
materialize_orchestrator_skill(...)handles sparse checkout of referenced repositories and generatesORCHESTRATOR_SKILL.mdsummaries. - API change to
assemble_prompt(...): Now acceptsorchestrator_skill_packandorchestrator_skill_enabledoverrides and appends orchestrator skill context when present. - Template impact: stranske/Template's copy of this file (already exists at
scripts/runner_lib/core.py) will be updated with 183 net lines added, integrating orchestrator skill loading into prompt assembly.
- New function:
-
.github/scripts/verifier_verdict_json.py[::stranske/Workflows::]- New script: Parses verifier agent markdown output to extract structured JSON verdict, with tampering detection for diff/patch blocks.
- No direct Template consumers yet: This is a new utility for verifier integration, but verifier workflows in Template may begin using it.
-
Workflow input additions [::stranske/Workflows::]
.github/workflows/agents-71-codex-belt-dispatcher.yml: New optional inputsorchestrator_skill_packandorchestrator_skill_enabled.github/workflows/agents-72-codex-belt-worker-dispatch.yml: Removedmax_paralleldispatch input (hardcoded to 1 in the reusable call); added orchestrator skill inputs.github/workflows/agents-72-codex-belt-worker.yml: New optional workflow_call inputs for orchestrator skill context
Evidence of integration in Workflows:
- stranske/Workflows has comprehensive test coverage for both breaking changes (agent delegation threshold, keepalive hard-cap behavior) in
.github/scripts/__tests__/agent-delegation-policy.test.jsand.github/scripts/__tests__/keepalive-loop.test.js. - The Template repository currently has
agent_delegation_policy.jsandkeepalive_loop.jsas synced copies (no custom modifications detected), and these will receive the breaking changes. - Template's
scripts/runner_lib/core.pyexists but will be overwritten with orchestrator skill integration; no existing code conflicts detected.
🔇 Additional comments (7)
.github/scripts/agent_delegation_policy.js (1)
86-87: LGTM!Also applies to: 263-282
.github/scripts/keepalive_loop.js (1)
145-162: LGTM!Also applies to: 1601-1624, 2521-2527, 2618-2622, 2683-2685, 2797-2799
scripts/orchestrator_skill.py (1)
1-205: LGTM!Also applies to: 243-401
scripts/runner_lib/core.py (1)
121-132: LGTM!Also applies to: 256-390, 407-420, 451-459, 922-932, 943-945, 1042-1044
.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
| VERDICT_RE = re.compile( | ||
| r"\b[\"']?verdict[\"']?\s*:\s*[\"']?(pass|fail)[\"']?\b", | ||
| re.IGNORECASE, | ||
| ) |
There was a problem hiding this comment.
Verdict detection regex is incomplete and bypasses tampering checks.
The VERDICT_RE regex only matches (pass|fail) explicitly, but _normalize_verdict (and the downstream contract in verifier_config.py) recognizes four valid verdicts: "pass", "concerns", "fail", "error".
An attacker can embed verdict: "concerns" or verdict: "error" inside a diff/patch region and evade the tampering detection at line 69, because the regex will not match those strings.
The regex should match all valid verdict strings to fulfill the tamper-resistance guarantee.
🔧 Proposed fix
Replace the regex to match all valid verdict strings:
VERDICT_RE = re.compile(
- r"\b[\"']?verdict[\"']?\s*:\s*[\"']?(pass|fail)[\"']?\b",
+ r"\b[\"']?verdict[\"']?\s*:\s*[\"']?(pass|fail|concerns|error)[\"']?\b",
re.IGNORECASE,
)Alternatively, compute the pattern dynamically from VALID_VERDICTS:
_VERDICT_CHOICES = "|".join(re.escape(v) for v in VALID_VERDICTS)
VERDICT_RE = re.compile(
rf"\b[\"']?verdict[\"']?\s*:\s*[\"']?({_VERDICT_CHOICES})[\"']?\b",
re.IGNORECASE,
)🤖 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 around lines 12 - 15, The
VERDICT_RE regex pattern at the top of the file only matches "pass" and "fail"
explicitly, but the code recognizes four valid verdicts: "pass", "concerns",
"fail", and "error". This allows an attacker to embed unmatched verdict strings
like "verdict: concerns" or "verdict: error" in diff regions and bypass the
tampering detection check at line 69. Update the VERDICT_RE regex to match all
four valid verdict strings by either explicitly including all of them in the
alternation pattern or by dynamically constructing the pattern from the
VALID_VERDICTS list using re.escape to ensure proper escaping.
| 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 | ||
|
|
||
| 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.
enabled_override=true cannot re-enable a disabled config
On Line 207, a config with "enabled": false returns plan=None. On Lines 218-242, enabled_override=True flips only the flag, so the function still returns None and materialization is skipped. This breaks the new override contract end-to-end.
Proposed fix
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:
+ enabled, plan = parse_orchestrator_skill_config_text(config_text, config_path)
+ if enabled_override is True and plan is None:
+ payload = json.loads(config_text)
+ if isinstance(payload, dict):
+ payload = {**payload, "enabled": True}
+ _, plan = parse_orchestrator_skill_config(payload)
@@
if enabled_override is not None:
enabled = enabled_override🤖 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 - 242, When enabled_override
is True and re-enables a config that was previously disabled (where plan is None
from the parse_orchestrator_skill_config_text call), the function still returns
None because plan was never initialized. After applying the enabled_override
check around line 216, add logic to detect when enabled is True but plan is
None, and in that case create a default OrchestratorSkillCheckoutPlan with
sensible defaults (matching the pattern already used in the pack_override blocks
with empty repo, ref, and paths, and DEFAULT_CHECKOUT_PATH) to ensure the
function returns a valid plan when a disabled config is re-enabled via override.
1 participant
Sync Summary
Files Updated
Files Skipped
Review Checklist
Source: stranske/Workflows
Source SHA:
509eceb28eb49f48ca51d40b77841cf7acf701d6Template hash:
7456dc211466Sync branch:
sync/workflows-7456dc211466Consumer repo:
stranske/trip-plannerManifest:
.github/sync-manifest.ymlSummary by CodeRabbit
Release Notes
New Features
Improvements