Skip to content

chore: sync workflow templates - #1198

Merged
stranske-keepalive[bot] merged 1 commit into
mainfrom
sync/workflows-536a715df8b0
Jun 20, 2026
Merged

chore: sync workflow templates#1198
stranske-keepalive[bot] merged 1 commit into
mainfrom
sync/workflows-536a715df8b0

Conversation

@stranske

@stranske stranske commented Jun 20, 2026

Copy link
Copy Markdown
Owner

Sync Summary

Files Updated

  • agents-81-gate-followups.yml: Gate followups hub - consolidates keepalive and autofix followups
  • agents-73-codex-belt-conveyor.yml: Codex belt conveyor - orchestrates belt worker execution and handles completion
  • agents-guard.yml: Agents guard - enforces agents workflow protections (Health 45)
  • check_deliberate_break.py: Opt-in Gate helper that proves named deliberate-break acceptance tests fail against the base implementation
  • runtime_ac_merge_guard.js: Blocks external merge lanes for PRs that require local Orchestrator runtime acceptance checks
  • gate_summary.py: Gate summary renderer - generates PR gate check summary
  • AGENTS.md: Context file for agents and coding assistants
  • CLAUDE.md: Context file for Claude/AI assistants

Files Skipped

  • .github/workflows/pr-00-gate.yml: Maintains a fully custom Gate workflow; never overwrite (replaces the hard-coded custom_gate_repos list in maint-68).
  • ci.yml: File exists and sync_mode is create_only
  • renovate.json: File exists and sync_mode is create_only
  • cross-repo-smoke.yml: File exists and sync_mode is create_only
  • llm_slots.json: None

Review Checklist

  • CI passes with updated workflows
  • No repo-specific customizations were overwritten

Source: stranske/Workflows
Source SHA: deacb8ee2852a7c22fe229645468776f35921628
Template hash: 536a715df8b0
Sync branch: sync/workflows-536a715df8b0
Consumer repo: stranske/Manager-Database
Manifest: .github/sync-manifest.yml

Summary by CodeRabbit

Release Notes

  • Documentation

    • Added critical evaluator guidance to agent documentation, emphasizing merit-based evaluation and transparent confidence assessment.
  • New Features

    • Added a verification tool for acceptance criteria testing with detailed reporting.

Automated sync from stranske/Workflows
Template hash: 536a715df8b0

Changes synced from sync-manifest.yml
@stranske stranske added sync Automated sync from Workflows automated Automated sync from Workflows labels Jun 20, 2026
@coderabbitai

coderabbitai Bot commented Jun 20, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Adds scripts/check_deliberate_break.py, a CLI that verifies deliberate-break acceptance criteria by diffing test assertions and running tests against both head and base checkouts. Introduces runtime_ac_merge_guard.js to block merges when required AC labels are present, wires it into two merge workflows, extends gate_summary.py to report test-quality outcomes, and updates agent instruction docs and a pinned action commit.

Changes

Acceptance-Criteria Gate System

Layer / File(s) Summary
Deliberate-break verification CLI
scripts/check_deliberate_break.py
New CLI parses markdown AC sections for deliberate-break specs, optionally checks for assertion tampering via git diff, runs the candidate test on head, reinjects it into a base archive and reruns it, then emits PASS/FAIL_BROKEN/FAIL_HOLLOW/SKIPPED verdicts as JSON and GitHub Actions outputs.
Gate summary test-quality integration
.github/scripts/gate_summary.py
SummaryContext adds test_quality_result; build_context reads TEST_QUALITY_RESULT from env; _append_job_table and _active_lines render a test-quality table row; summarize() normalizes the value and transitions overall state to pending/failure as needed.
Runtime AC merge guard implementation
.github/scripts/runtime_ac_merge_guard.js
New module exporting RUNTIME_AC_REQUIRED_LABELS, normalizeLabelName, runtimeAcRequirement, hasRuntimeAcRequirement, and assertRuntimeAcMergeAllowed; guard fetches PR labels via withRetry when needed and throws code='runtime_ac_merge_blocked' with matched labels when required.
Workflow wiring for merge guard
.github/workflows/agents-73-codex-belt-conveyor.yml, .github/workflows/agents-81-gate-followups.yml
Both workflows add runtime_ac_merge_guard.js to their sparse checkouts and invoke assertRuntimeAcMergeAllowed immediately before the GitHub merge API call, making the guard a hard precondition.
Agent instructions and action pin updates
AGENTS.md, CLAUDE.md, .github/workflows/agents-guard.yml
AGENTS.md and CLAUDE.md gain a "Working Stance — Critical Evaluator" section; agents-guard.yml updates the pinned commit hash for setup-api-client in both fallback steps.

Sequence Diagram(s)

sequenceDiagram
    rect rgba(100, 149, 237, 0.5)
        Note over Workflow,GitHub: Merge guard pre-check
        Workflow->>assertRuntimeAcMergeAllowed: owner, repo, prNumber, labels/withRetry, source
        assertRuntimeAcMergeAllowed->>GitHub: GET /repos/{owner}/{repo}/pulls/{prNumber}/labels
        GitHub-->>assertRuntimeAcMergeAllowed: label list
        assertRuntimeAcMergeAllowed-->>Workflow: allowed:true OR throw runtime_ac_merge_blocked
    end
    rect rgba(144, 238, 144, 0.5)
        Note over Workflow,GitHub: Deliberate-break verification (gate_summary)
        Workflow->>check_deliberate_break.py: run with base/head refs
        check_deliberate_break.py->>check_deliberate_break.py: parse AC markdown, tamper check, run test on head
        check_deliberate_break.py->>check_deliberate_break.py: extract base archive, rerun test
        check_deliberate_break.py-->>gate_summary.py: TEST_QUALITY_RESULT env var
        gate_summary.py-->>Workflow: test-quality row in job table
    end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related issues

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 8.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check ❓ Inconclusive The title 'chore: sync workflow templates' is a generic, non-specific phrase that does not meaningfully convey the actual substantial changes made. While technically related to the changeset (which involves workflow files), the title obscures the significant functional additions like runtime AC merge guard, deliberate-break test checks, and gate summary enhancements. Consider a more descriptive title that highlights key functional changes, such as 'chore: add runtime AC merge guard and deliberate-break test verification' or 'chore: sync workflow templates with gate and merge guard improvements'.
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch sync/workflows-536a715df8b0

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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 @.github/workflows/agents-81-gate-followups.yml:
- Around line 1736-1744: The assertRuntimeAcMergeAllowed function is making a
redundant API call to fetch labels even though pr.labels is already available
from the current loop's PR data. Pass pr.labels as a parameter to the
assertRuntimeAcMergeAllowed function call, and update the function definition to
accept this parameter and use it directly, only falling back to an API fetch if
the labels parameter is missing or undefined. This eliminates the unnecessary
network call and prevents transient failures when label data is already
available.

In `@scripts/check_deliberate_break.py`:
- Around line 69-93: The `_parse_key_values` function calls `shlex.split(text)`
on line 71 without handling potential `ValueError` exceptions that occur when
the marker text contains unbalanced quotes or malformed syntax. Wrap the
`shlex.split(text)` call in a try-except block to catch `ValueError` and raise a
descriptive `ValueError` with a clear message about invalid deliberate-break
marker syntax, following the same error handling pattern as used in
`_explicit_marker` at line 90.
- Around line 179-216: The _changed_assertions function calls _git which can
raise CalledProcessError when git commands fail, but this exception is not
caught in verify_spec, causing the script to crash. Add exception handling for
CalledProcessError in the verify_spec function alongside the existing
TimeoutExpired handling to gracefully return a VERDICT_BROKEN result when git
operations fail instead of propagating the uncaught exception.
🪄 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: 7eef540e-a3c7-4df7-9f97-d5f516bea41c

📥 Commits

Reviewing files that changed from the base of the PR and between c6837a8 and 65c0bfb.

📒 Files selected for processing (8)
  • .github/scripts/gate_summary.py
  • .github/scripts/runtime_ac_merge_guard.js
  • .github/workflows/agents-73-codex-belt-conveyor.yml
  • .github/workflows/agents-81-gate-followups.yml
  • .github/workflows/agents-guard.yml
  • AGENTS.md
  • CLAUDE.md
  • scripts/check_deliberate_break.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 (7)
{AGENTS.md,CLAUDE.md}

📄 CodeRabbit inference engine (AGENTS.md)

Keep AGENTS.md materially aligned with CLAUDE.md. Differences between the two should only be agent-specific execution notes, not different repository rules.

Files:

  • CLAUDE.md
  • AGENTS.md
**/*.py

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Manager-Database repository uses Prefect 2.x - import schedules from prefect.client.schemas.schedules

Files:

  • scripts/check_deliberate_break.py
**/.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-73-codex-belt-conveyor.yml
  • .github/workflows/agents-guard.yml
  • .github/workflows/agents-81-gate-followups.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-73-codex-belt-conveyor.yml
  • .github/workflows/agents-guard.yml
  • .github/workflows/agents-81-gate-followups.yml
.github/workflows/**/*.yml

📄 CodeRabbit inference engine (AGENTS.md)

In GitHub Actions workflows, first-party consumers should reference reusable workflows with @main unless intentionally pinning to an exact commit SHA for a controlled reason.

Files:

  • .github/workflows/agents-73-codex-belt-conveyor.yml
  • .github/workflows/agents-guard.yml
  • .github/workflows/agents-81-gate-followups.yml
{.github/workflows/agents-*.yml,.github/workflows/autofix.yml,.github/codex/**/*,docs/sync/**/*}

📄 CodeRabbit inference engine (AGENTS.md)

Synced workflows (e.g., agents-*.yml, autofix.yml), prompts in .github/codex/, and synced scripts/docs should be fixed in stranske/Workflows source, not in the consumer repository. Update the sync manifest in Workflows if a consumer-facing file changed.

Files:

  • .github/workflows/agents-73-codex-belt-conveyor.yml
  • .github/workflows/agents-guard.yml
  • .github/workflows/agents-81-gate-followups.yml
.github/workflows/*.yml

📄 CodeRabbit inference engine (CLAUDE.md)

Reference reusable workflows with @main unless intentionally pinning to an exact commit SHA for a controlled reason.

Files:

  • .github/workflows/agents-73-codex-belt-conveyor.yml
  • .github/workflows/agents-guard.yml
  • .github/workflows/agents-81-gate-followups.yml
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: stranske/Manager-Database

Timestamp: 2026-06-20T01:47:18.988Z
Learning: When evaluating claims, designs, and instructions, act as a critical evaluator—correct judgment takes precedence over agreement. State confidence levels, flag uncertainties, and lead with the strongest objection when something is wrong or weaker than an alternative.
Learnt from: CR
Repo: stranske/Manager-Database

Timestamp: 2026-06-20T01:47:18.988Z
Learning: Keep workflow logic in `stranske/Workflows` root repository. Consumer repositories should only carry repo-specific configuration files unless explicitly documented as an exception.
Learnt from: CR
Repo: stranske/Manager-Database

Timestamp: 2026-06-20T01:47:18.988Z
Learning: For infrastructure work, follow this priority order: (1) `stranske/Workflows` root docs (`README.md`, `docs/WORKFLOW_GUIDE.md`, `docs/ci/WORKFLOWS.md`), (2) `stranske/Workflows/docs/INTEGRATION_GUIDE.md` and `docs/ops/CONSUMER_REPO_MAINTENANCE.md`, (3) `stranske/Workflows/templates/consumer-repo/`, (4) local repo-specific files.
Learnt from: CR
Repo: stranske/Manager-Database

Timestamp: 2026-06-20T01:47:18.988Z
Learning: Before editing local workflow infrastructure, evaluate whether the work belongs in `stranske/Workflows` instead. Changes affecting reusable workflows, agent prompts, routing, keepalive/autofix/verifier behavior, or synced files should be made in the source repository first, then synced to the consumer repo.
Learnt from: CR
Repo: stranske/Manager-Database

Timestamp: 2026-06-20T01:47:30.228Z
Learning: Evaluate claims, designs, and instructions on their merits before agreeing — including the orchestrator's and the user's. When something is wrong, weaker than an alternative, or missing, say so plainly and lead with the strongest objection. Separate 'this is correct' from 'I'll do as asked.'
Learnt from: CR
Repo: stranske/Manager-Database

Timestamp: 2026-06-20T01:47:30.228Z
Learning: Calibrate dissent — provide not maximal disagreement, but correct judgment calibrated to confidence levels and conditions that would change your mind. Flag uncertainty.
Learnt from: CR
Repo: stranske/Manager-Database

Timestamp: 2026-06-20T01:47:30.228Z
Learning: For infrastructure and workflow work, follow the source-of-truth order: (1) stranske/Workflows root docs, (2) INTEGRATION_GUIDE.md and CONSUMER_REPO_MAINTENANCE.md, (3) consumer sync source in stranske/Workflows/templates/consumer-repo/, (4) local repo-specific files.
Learnt from: CR
Repo: stranske/Manager-Database

Timestamp: 2026-06-20T01:47:30.228Z
Learning: When editing workflow or automation files, ask: 'Does this work belong in stranske/Workflows instead?' The answer is typically yes if the change affects reusable workflows, agent prompts/routing, keepalive/autofix/verifier behavior, synced workflow files, or synced scripts/docs.
Learnt from: CR
Repo: stranske/Manager-Database

Timestamp: 2026-06-20T01:47:30.228Z
Learning: `ci.yml` and `autofix-versions.env` are repo-specific files and should be edited locally; do not sync from Workflows.
Learnt from: CR
Repo: stranske/Manager-Database

Timestamp: 2026-06-20T01:47:30.228Z
Learning: Synced workflows, prompts, scripts, and consumer docs are managed through `.github/sync-manifest.yml` in Workflows. Do not manually edit synced files; fix them in Workflows first.
Learnt from: CR
Repo: stranske/Manager-Database

Timestamp: 2026-06-20T01:47:30.228Z
Learning: Agent workflow files (`agents-*.yml`), `autofix.yml`, and `.github/codex/` prompt files should be fixed in Workflows, not edited locally in the consumer repo.
Learnt from: CR
Repo: stranske/Manager-Database

Timestamp: 2026-06-20T01:47:30.228Z
Learning: Keep CLAUDE.md materially aligned with AGENTS.md. Differences should only be agent-specific execution notes, not different repository rules.
🪛 ast-grep (0.43.0)
scripts/check_deliberate_break.py

[error] 143-150: Command coming from incoming request
Context: subprocess.run(
list(command),
cwd=cwd,
text=True,
capture_output=True,
env=env,
timeout=timeout,
)
Note: [CWE-20].

(subprocess-from-request)


[error] 159-166: Command coming from incoming request
Context: subprocess.run(
["git", *args],
cwd=cwd,
check=True,
text=True,
capture_output=True,
timeout=timeout,
)
Note: [CWE-20].

(subprocess-from-request)


[error] 190-196: Command coming from incoming request
Context: subprocess.run(
["git", "archive", "--format=tar", base],
cwd=cwd,
check=True,
capture_output=True,
timeout=DEFAULT_TIMEOUT_SECONDS,
)
Note: [CWE-20].

(subprocess-from-request)


[error] 143-150: Use of unsanitized data to create processes
Context: subprocess.run(
list(command),
cwd=cwd,
text=True,
capture_output=True,
env=env,
timeout=timeout,
)
Note: [CWE-78].

(os-system-unsanitized-data)


[info] 329-329: use jsonify instead of json.dumps for JSON output
Context: json.dumps(_json_result(VERDICT_SKIPPED, reason="no deliberate-break marker"))
Note: Security best practice.

(use-jsonify)


[info] 341-341: use jsonify instead of json.dumps for JSON output
Context: json.dumps(result, sort_keys=True)
Note: Security best practice.

(use-jsonify)

🔀 Multi-repo context stranske/Template, stranske/Workflows

Perfect! I have confirmed all the cross-repo sync gaps. Here's my final summary:

Linked repositories findings

stranske/Template

Critical Sync Gaps Between Template and Workflows Source:

  1. agents-73-codex-belt-conveyor.yml — Missing Runtime AC Merge Guard Logic [::stranske/Template::]

    • Template version: 22 lines in "Merge PR with squash" step (lines 82-108)
    • Workflows version: 32 lines (10 additional lines for runtime AC guard)
    • Missing code: No import of runtime_ac_merge_guard.js and no call to assertRuntimeAcMergeAllowed()
    • Template directly calls github.rest.pulls.merge() without pre-merge validation
    • Workflows version: Calls assertRuntimeAcMergeAllowed() first, which blocks merges for PRs with labels like runtime-ac, runtime-verification, etc.
    • Impact: When the sync completes, Template's merge behavior will change—it will start rejecting merges on PRs with runtime AC labels, which may break existing automation that doesn't account for this guard
  2. agents-81-gate-followups.yml — Missing runtime_ac_merge_guard.js in Sparse Checkout [::stranske/Template::]

    • Template sparse-checkout (guarded-merge job): 2 files
      • .github/scripts/github-api-with-retry.js
      • .github/scripts/token_load_balancer.js
    • Workflows sparse-checkout: 3 files (includes .github/scripts/runtime_ac_merge_guard.js)
    • Impact: When agents-73 tries to require the runtime guard file, it will fail if not checked out. This is a pre-requisite dependency that will cause runtime errors.
  3. gate_summary.py — Missing test_quality_result Field [::stranske/Template::]

    • Template SummaryContext: 11 fields total, last is docs_guard_result: str = "success"
    • Workflows SummaryContext: 12 fields, includes test_quality_result: str = "skipped"
    • Template's build_context(): Does not read TEST_QUALITY_RESULT environment variable
    • Impact: Any workflow trying to pass test-quality results will cause a TypeError; Gate summaries won't include test-quality check outcomes

stranske/Workflows (Source Repository — Correct State)

Files correctly implement the new features [::stranske/Workflows::]

  • ./templates/consumer-repo/.github/scripts/runtime_ac_merge_guard.js — 137 lines, exports assertRuntimeAcMergeAllowed
  • ./templates/consumer-repo/.github/workflows/agents-73-codex-belt-conveyor.yml — Imports and uses runtime guard before merge
  • ./templates/consumer-repo/.github/workflows/agents-81-gate-followups.yml — Includes guard script in sparse-checkout
  • ./templates/consumer-repo/.github/scripts/gate_summary.py — Includes test_quality_result field and environment variable handling
  • All three scripts listed in ./.github/sync-manifest.yml for syncing to consumer repos

Summary: The PR is syncing these new runtime AC merge guard and test-quality changes from stranske/Workflows to stranske/Manager-Database, but stranske/Template (the reference template for new consumer repos) is currently out of sync. Template has overwrite_repos: [stranske/Template] annotations in the manifest but hasn't been updated with the new guard behavior, creating a version mismatch between the source and the template used for bootstrapping new consumer repositories.

🔇 Additional comments (16)
AGENTS.md (1)

5-8: LGTM!

CLAUDE.md (1)

5-8: LGTM!

.github/workflows/agents-guard.yml (1)

114-114: No action required—pinned commit is valid and output contract is satisfied.

The commit SHA d68de1904bcdbe16bfe2462b73aa18f41f8a0a47 exists in stranske/Workflows and the action exports all three required outputs (token_count, available_tokens, setup_contract) that downstream steps depend on. Both pinned references (lines 114 and 183) use the same commit hash consistently.

.github/scripts/runtime_ac_merge_guard.js (1)

3-137: LGTM!

.github/workflows/agents-73-codex-belt-conveyor.yml (1)

195-195: LGTM!

Also applies to: 444-444, 456-464

.github/workflows/agents-81-gate-followups.yml (1)

1555-1555: LGTM!

Also applies to: 1567-1567

scripts/check_deliberate_break.py (4)

1-46: LGTM!


133-167: LGTM!


219-309: LGTM!


312-347: LGTM!

.github/scripts/gate_summary.py (6)

24-24: LGTM!


242-257: LGTM!


260-291: LGTM!


331-350: LGTM!


387-411: LGTM!


421-448: LGTM!

Comment on lines +1736 to +1744
await assertRuntimeAcMergeAllowed({
github,
core,
owner,
repo,
prNumber,
withRetry,
source: 'agents-81-gate-followups guarded merge',
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Avoid a second label API read in the guarded-merge loop.

At Line 1736, assertRuntimeAcMergeAllowed re-fetches labels even though pr.labels is already available from the PR fetch in this loop. That adds an unnecessary network dependency and can fail merges on transient label-read errors despite already having label data. Pass pr.labels into the guard and only fall back to API fetch when labels are missing.

Suggested patch
                           try {
+                            const knownLabels = Array.isArray(pr.labels) ? pr.labels : undefined;
                             await assertRuntimeAcMergeAllowed({
                               github,
                               core,
                               owner,
                               repo,
                               prNumber,
+                              labels: knownLabels,
                               withRetry,
                               source: 'agents-81-gate-followups guarded merge',
                             });
🤖 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-81-gate-followups.yml around lines 1736 - 1744, The
assertRuntimeAcMergeAllowed function is making a redundant API call to fetch
labels even though pr.labels is already available from the current loop's PR
data. Pass pr.labels as a parameter to the assertRuntimeAcMergeAllowed function
call, and update the function definition to accept this parameter and use it
directly, only falling back to an API fetch if the labels parameter is missing
or undefined. This eliminates the unnecessary network call and prevents
transient failures when label data is already available.

Comment on lines +69 to +93
def _parse_key_values(text: str) -> dict[str, str]:
values: dict[str, str] = {}
for token in shlex.split(text):
if "=" not in token:
continue
key, value = token.split("=", 1)
values[key.strip().replace("_", "-").lower()] = value.strip()
return values


def _explicit_marker(section: str) -> DeliberateBreakSpec | None:
for line in section.splitlines():
match = MARKER_RE.search(line.strip())
if not match:
continue
values = _parse_key_values(match.group("body"))
test_id = values.get("test") or values.get("test-id")
test_file = values.get("test-file") or values.get("file")
break_file = values.get("break-file") or values.get("revert-file")
command_text = values.get("command")
if not test_id or not test_file or not break_file:
raise ValueError("deliberate-break marker requires test, test-file, and break-file")
command = tuple(shlex.split(command_text)) if command_text else _pytest_command(test_id)
return DeliberateBreakSpec(test_id, test_file, break_file, command)
return None

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Unhandled ValueError from shlex.split on malformed marker syntax.

If the deliberate-break marker contains unbalanced quotes (e.g., command="pytest test.py), shlex.split at line 71 raises ValueError which propagates as an unhandled exception. The script already raises ValueError at line 90 for missing keys—consider catching parsing errors similarly.

Proposed fix
 def _parse_key_values(text: str) -> dict[str, str]:
     values: dict[str, str] = {}
-    for token in shlex.split(text):
+    try:
+        tokens = shlex.split(text)
+    except ValueError:
+        return values
+    for token in tokens:
         if "=" not in token:
             continue
         key, value = token.split("=", 1)
         values[key.strip().replace("_", "-").lower()] = value.strip()
     return values
🤖 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_deliberate_break.py` around lines 69 - 93, The
`_parse_key_values` function calls `shlex.split(text)` on line 71 without
handling potential `ValueError` exceptions that occur when the marker text
contains unbalanced quotes or malformed syntax. Wrap the `shlex.split(text)`
call in a try-except block to catch `ValueError` and raise a descriptive
`ValueError` with a clear message about invalid deliberate-break marker syntax,
following the same error handling pattern as used in `_explicit_marker` at line
90.

Comment on lines +179 to +216
def _changed_assertions(base: str, head: str, test_file: str, cwd: Path) -> list[str]:
status = _git(["diff", "--name-status", f"{base}...{head}", "--", test_file], cwd)
if any(line.split("\t", 1)[0] == "A" for line in status.stdout.splitlines()):
return []
completed = _git(
["diff", "--no-ext-diff", "--unified=0", f"{base}...{head}", "--", test_file],
cwd,
)
return list(_assertion_diff_lines(completed.stdout))


def _archive_ref(base: str, target: Path, cwd: Path) -> None:
archive = subprocess.run(
["git", "archive", "--format=tar", base],
cwd=cwd,
check=True,
capture_output=True,
timeout=DEFAULT_TIMEOUT_SECONDS,
)
target_root = target.resolve()
with tarfile.open(fileobj=BytesIO(archive.stdout), mode="r:") as tar:
for member in tar:
member_path = target_root / member.name
resolved = member_path.resolve()
if not resolved.is_relative_to(target_root):
raise ValueError(f"unsafe archive path: {member.name}")
if member.isdir():
resolved.mkdir(parents=True, exist_ok=True)
continue
if not member.isfile():
continue
resolved.parent.mkdir(parents=True, exist_ok=True)
source = tar.extractfile(member)
if source is None:
continue
with source, resolved.open("wb") as destination:
shutil.copyfileobj(source, destination)
resolved.chmod(member.mode & 0o777)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Unhandled CalledProcessError from git commands can crash the script.

_changed_assertions calls _git which uses check=True (line 163). If git fails (e.g., invalid base ref, file not tracked), CalledProcessError propagates uncaught. verify_spec catches TimeoutExpired but not CalledProcessError, causing the script to crash with a stack trace instead of returning a graceful VERDICT_BROKEN result.

Proposed fix in verify_spec
     try:
         if enforce_tamper:
             tampered = _changed_assertions(base, head, spec.test_file, repo)
             if tampered:
                 return _json_result(
                     VERDICT_BROKEN,
                     reason="test-assertion-tamper",
                     test_file=spec.test_file,
                     changed_assertions=tampered,
                 )

         head_run = _run(spec.command, repo)
     except subprocess.TimeoutExpired as exc:
         return _json_result(
             VERDICT_BROKEN,
             reason="command-timeout",
             command=list(exc.cmd) if isinstance(exc.cmd, (tuple, list)) else str(exc.cmd),
             timeout=exc.timeout,
         )
+    except subprocess.CalledProcessError as exc:
+        return _json_result(
+            VERDICT_BROKEN,
+            reason="git-command-failed",
+            command=exc.cmd,
+            returncode=exc.returncode,
+            stderr=exc.stderr,
+        )
🧰 Tools
🪛 ast-grep (0.43.0)

[error] 190-196: Command coming from incoming request
Context: subprocess.run(
["git", "archive", "--format=tar", base],
cwd=cwd,
check=True,
capture_output=True,
timeout=DEFAULT_TIMEOUT_SECONDS,
)
Note: [CWE-20].

(subprocess-from-request)

🤖 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_deliberate_break.py` around lines 179 - 216, The
_changed_assertions function calls _git which can raise CalledProcessError when
git commands fail, but this exception is not caught in verify_spec, causing the
script to crash. Add exception handling for CalledProcessError in the
verify_spec function alongside the existing TimeoutExpired handling to
gracefully return a VERDICT_BROKEN result when git operations fail instead of
propagating the uncaught exception.

@stranske-keepalive
stranske-keepalive Bot merged commit e89c06d into main Jun 20, 2026
80 of 86 checks passed
@stranske-keepalive
stranske-keepalive Bot deleted the sync/workflows-536a715df8b0 branch June 20, 2026 01:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

automated Automated sync from Workflows sync Automated sync from Workflows

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant