Skip to content

fix: route_queue_mode — handle queue + autoMergeAllowed=false#762

Merged
Trecek merged 5 commits intointegrationfrom
route-queue-mode-incorrect-routing-when-branch-has-merge-que/755
Apr 12, 2026
Merged

fix: route_queue_mode — handle queue + autoMergeAllowed=false#762
Trecek merged 5 commits intointegrationfrom
route-queue-mode-incorrect-routing-when-branch-has-merge-que/755

Conversation

@Trecek
Copy link
Copy Markdown
Collaborator

@Trecek Trecek commented Apr 12, 2026

Summary

Fix the route_queue_mode routing block in implementation.yaml,
implementation-groups.yaml, and remediation.yaml so that a target branch with
both a configured merge queue and autoMergeAllowed=false on the repository
no longer routes to enable_auto_merge (which calls gh pr merge --squash --auto
and is rejected by GitHub's API auto-merge gate).

The fix introduces an explicit four-cell routing matrix over queue_available × auto_merge_available, adds a new pipeline step queue_enqueue_no_auto that runs
plain gh pr merge --squash and waits via the existing wait_for_queue step,
and updates the sous-chef MERGE PHASE rules so the kitchen contract matches
the new recipe shape.

After the fix, all four (queue × auto) combinations have a single safe route:

queue_available auto_merge_available Routes to
true true enable_auto_mergewait_for_queue
true false queue_enqueue_no_autowait_for_queue
false true direct_mergewait_for_direct_merge
false false immediate_mergewait_for_immediate_merge

Architecture Impact

Process Flow Diagram

%%{init: {'flowchart': {'nodeSpacing': 40, 'rankSpacing': 50, 'curve': 'basis'}}}%%
flowchart TB
    %% CLASS DEFINITIONS %%
    classDef terminal fill:#1a237e,stroke:#7986cb,stroke-width:2px,color:#fff;
    classDef stateNode fill:#004d40,stroke:#4db6ac,stroke-width:2px,color:#fff;
    classDef handler fill:#e65100,stroke:#ffb74d,stroke-width:2px,color:#fff;
    classDef phase fill:#6a1b9a,stroke:#ba68c8,stroke-width:2px,color:#fff;
    classDef detector fill:#b71c1c,stroke:#ef5350,stroke-width:2px,color:#fff;

    %% TERMINALS %%
    START([START<br/>ci_watch passes])
    DONE([DONE<br/>register_clone_success → done])
    SKIP([SKIP<br/>auto_merge input disabled<br/>PR left open for human])

    subgraph Detection ["Detection Pipeline (shared by all 3 recipes)"]
        direction TB
        CMQ["check_merge_queue<br/>━━━━━━━━━━<br/>GraphQL: active queue on base_branch?<br/>captures: queue_available<br/>skip_when_false: open_pr"]
        CMGT["check_merge_group_trigger<br/>━━━━━━━━━━<br/>grep workflows for merge_group trigger<br/>captures: merge_group_trigger<br/>on_success + on_failure → check_auto_merge"]
        CAM["check_auto_merge<br/>━━━━━━━━━━<br/>GraphQL: autoMergeAllowed?<br/>captures: auto_merge_available<br/>on_success + on_failure → route_queue_mode"]
    end

    subgraph Routing ["● route_queue_mode — 5-Branch Priority Matrix (action: route)"]
        direction TB
        RQM{"● route_queue_mode<br/>━━━━━━━━━━<br/>Priority dispatch — evaluated top-down<br/>auto_merge bypass checked FIRST"}
    end

    subgraph QueueAuto ["Queue Path — auto_merge_available = true"]
        direction TB
        EAM["enable_auto_merge<br/>━━━━━━━━━━<br/>gh pr merge --squash --auto<br/>GitHub enqueues PR into merge queue"]
    end

    subgraph QueueNoAuto ["● Queue Path — auto_merge_available = false"]
        direction TB
        QENA["● queue_enqueue_no_auto<br/>━━━━━━━━━━<br/>gh pr merge --squash  (no --auto flag)<br/>repo disallows autoMergeAllowed<br/>GitHub still enqueues via active queue<br/>on_failure → register_clone_success"]
    end

    subgraph WaitQueue ["Wait for Queue (timeout: 900s)"]
        direction TB
        WFQ["wait_for_queue<br/>━━━━━━━━━━<br/>tool: wait_for_merge_queue<br/>outcomes: merged | ejected_ci_failure | ejected | stalled | timeout"]
        RSP["reenroll_stalled_pr<br/>━━━━━━━━━━<br/>tool: toggle_auto_merge<br/>disable → re-enable to unstall PR"]
        EJCI["diagnose_ci → resolve_ci → re_push<br/>━━━━━━━━━━<br/>CI failure ejection path<br/>loops back via ci_watch"]
        EJCON["queue_ejected_fix<br/>━━━━━━━━━━<br/>run_skill: resolve-merge-conflicts (retries: 1)<br/>→ re_push_queue_fix → ci_watch_post_queue_fix<br/>→ reenter_merge_queue → wait_for_queue"]
    end

    subgraph DirectPath ["Direct Merge Path — auto_merge_available=true, no queue/trigger"]
        direction TB
        DM["direct_merge<br/>━━━━━━━━━━<br/>gh pr merge --squash --auto<br/>GitHub merges when checks pass (no queue)"]
        WDM["wait_for_direct_merge<br/>━━━━━━━━━━<br/>poll 90×10s = 15 min max<br/>outcomes: merged | closed | timeout"]
        DMCF["direct_merge_conflict_fix<br/>━━━━━━━━━━<br/>run_skill: resolve-merge-conflicts (retries: 1)<br/>→ re_push_direct_fix → redirect_merge<br/>→ loops back to wait_for_direct_merge"]
    end

    subgraph ImmPath ["Immediate Merge Path — no auto-merge, no queue"]
        direction TB
        IM["immediate_merge<br/>━━━━━━━━━━<br/>gh pr merge --squash  (no --auto)<br/>Synchronous squash merge"]
        WIM["wait_for_immediate_merge<br/>━━━━━━━━━━<br/>poll 30×10s = 5 min max<br/>outcomes: merged | closed | timeout"]
        IMCF["immediate_merge_conflict_fix<br/>━━━━━━━━━━<br/>run_skill: resolve-merge-conflicts (retries: 1)<br/>→ re_push_immediate_fix → remerge_immediate<br/>→ loops back to wait_for_immediate_merge"]
    end

    subgraph Cleanup ["Issue Release + Clone Cleanup"]
        direction LR
        RIS["release_issue_success<br/>━━━━━━━━━━<br/>remove in-progress label<br/>apply staged label (non-default base)"]
        RCS["register_clone_success<br/>━━━━━━━━━━<br/>mark clone for batch deletion"]
    end

    %% MAIN DETECTION FLOW %%
    START --> CMQ
    CMQ -->|"on_failure"| RCS
    CMQ --> CMGT
    CMGT -->|"on_success + on_failure (unconditional)"| CAM
    CAM -->|"on_success + on_failure (unconditional)"| RQM

    %% ROUTING BRANCHES (priority order) %%
    RQM -->|"① inputs.auto_merge != 'true'"| SKIP
    RQM -->|"② queue=✓ AND trigger=✓ AND auto_avail=✓"| EAM
    RQM -->|"③ queue=✓ AND trigger=✓ AND auto_avail=✗"| QENA
    RQM -->|"④ auto_avail=✓  (no queue or trigger missing)"| DM
    RQM -->|"⑤ default fallback"| IM

    %% QUEUE AUTO PATH %%
    EAM --> WFQ

    %% QUEUE NO-AUTO PATH %%
    QENA -->|"on_success"| WFQ
    QENA -->|"on_failure"| RCS

    %% WAIT FOR QUEUE OUTCOMES %%
    WFQ -->|"merged"| RIS
    WFQ -->|"stalled"| RSP
    RSP -->|"→ wait_for_queue"| WFQ
    WFQ -->|"ejected_ci_failure (checked FIRST)"| EJCI
    WFQ -->|"ejected conflict (checked SECOND)"| EJCON
    EJCI -->|"re_push → ci_watch loop → reenter"| WFQ
    EJCON -->|"re_push → reenter_merge_queue"| WFQ
    WFQ -->|"timeout"| RCS

    %% DIRECT PATH %%
    DM -->|"on_success"| WDM
    DM -->|"on_failure"| RCS
    WDM -->|"merged"| RIS
    WDM -->|"closed (conflict)"| DMCF
    WDM -->|"timeout"| RCS
    DMCF -->|"fix loop"| WDM

    %% IMMEDIATE PATH %%
    IM -->|"on_success"| WIM
    IM -->|"on_failure"| RCS
    WIM -->|"merged"| RIS
    WIM -->|"closed (conflict)"| IMCF
    WIM -->|"timeout"| RCS
    IMCF -->|"fix loop"| WIM

    %% CLEANUP %%
    RIS --> RCS
    RCS --> DONE

    %% CLASS ASSIGNMENTS %%
    class START,DONE,SKIP terminal;
    class CMQ,CMGT,CAM detector;
    class RQM stateNode;
    class EAM,QENA,DM,IM handler;
    class WFQ,WDM,WIM phase;
    class RSP,EJCI,EJCON detector;
    class DMCF,IMCF handler;
    class RIS,RCS phase;
Loading

State Lifecycle Diagram

%%{init: {'flowchart': {'nodeSpacing': 50, 'rankSpacing': 65, 'curve': 'basis'}}}%%
flowchart TB
    %% CLASS DEFINITIONS %%
    classDef cli fill:#1a237e,stroke:#7986cb,stroke-width:2px,color:#fff;
    classDef stateNode fill:#004d40,stroke:#4db6ac,stroke-width:2px,color:#fff;
    classDef handler fill:#e65100,stroke:#ffb74d,stroke-width:2px,color:#fff;
    classDef phase fill:#6a1b9a,stroke:#ba68c8,stroke-width:2px,color:#fff;
    classDef newComponent fill:#2e7d32,stroke:#81c784,stroke-width:2px,color:#fff;
    classDef output fill:#00695c,stroke:#4db6ac,stroke-width:2px,color:#fff;
    classDef detector fill:#b71c1c,stroke:#ef5350,stroke-width:2px,color:#fff;
    classDef gap fill:#ff6f00,stroke:#ffa726,stroke-width:2px,color:#000;
    classDef terminal fill:#1a237e,stroke:#7986cb,stroke-width:2px,color:#fff;

    %% ── INPUT GUARDS ── %%
    subgraph Guards ["INIT_ONLY INPUT GUARDS"]
        direction LR
        OP["inputs.open_pr<br/>━━━━━━━━━━<br/>skip_when_false gate<br/>Bypasses entire detection chain"]
        AM["inputs.auto_merge<br/>━━━━━━━━━━<br/>INIT_ONLY user ingredient<br/>Priority-1 routing gate"]
    end

    %% ── DETECTION PHASE ── %%
    subgraph Detection ["● QUEUE STATE DETECTION (SET_ONCE → READ_ONCE)"]
        direction TB
        CMQ["● check_merge_queue<br/>━━━━━━━━━━<br/>GraphQL mergeQueue query<br/>on_failure → release_issue_success<br/>(safe degradation, leaves field unset)"]
        CMGT["● check_merge_group_trigger<br/>━━━━━━━━━━<br/>Scans .github/workflows/ for merge_group<br/>on_failure → check_auto_merge<br/>(graceful fallthrough)"]
        CMA["● check_auto_merge<br/>━━━━━━━━━━<br/>GraphQL autoMergeAllowed<br/>on_failure → route_queue_mode<br/>(jq fallback always emits 'false')"]

        QA["context.queue_available<br/>━━━━━━━━━━<br/>SET_ONCE / READ_ONCE<br/>'true' | 'false'<br/>Read exclusively by route_queue_mode"]
        MGT["context.merge_group_trigger<br/>━━━━━━━━━━<br/>SET_ONCE / READ_ONCE<br/>'true' | 'false'<br/>Read exclusively by route_queue_mode"]
        AMA["context.auto_merge_available<br/>━━━━━━━━━━<br/>SET_ONCE / READ_ONCE<br/>'true' | 'false'<br/>Always written (fallback = 'false')"]
    end

    %% ── ROUTING GATE ── %%
    subgraph Router ["● ROUTE_QUEUE_MODE (DERIVED — produced: [])"]
        RQM["● route_queue_mode<br/>━━━━━━━━━━<br/>action: route<br/>Reads 3 detection fields + inputs.auto_merge<br/>Writes NOTHING to context<br/>5-condition priority chain"]
    end

    %% ── MERGE PATHS ── %%
    subgraph Paths ["MERGE PATH SELECTION"]
        direction LR
        BYPASS["register_clone_success<br/>━━━━━━━━━━<br/>P1: auto_merge != 'true'<br/>Skip all merging"]
        EAM["enable_auto_merge<br/>━━━━━━━━━━<br/>P2: queue+trigger+auto<br/>gh pr merge --squash --auto"]
        QENA["● queue_enqueue_no_auto<br/>━━━━━━━━━━<br/>P3: queue+trigger, no auto<br/>gh pr merge --squash<br/>No --auto (autoMergeAllowed=false)<br/>GitHub still enqueues via queue intercept"]
        DM["direct_merge<br/>━━━━━━━━━━<br/>P4: no queue, auto avail<br/>gh pr merge --squash --auto"]
        IM["immediate_merge<br/>━━━━━━━━━━<br/>P5 (default)<br/>gh pr merge --squash"]
    end

    %% ── QUEUE WAIT STATE ── %%
    subgraph QueueWait ["QUEUE WAIT (MUTABLE STATE)"]
        WFQ["wait_for_queue<br/>━━━━━━━━━━<br/>timeout: 900s<br/>Shared by enable_auto_merge<br/>AND queue_enqueue_no_auto"]
        QPS["context.queue_pr_state<br/>━━━━━━━━━━<br/>MUTABLE — written by wait_for_queue<br/>merged | ejected | ejected_ci_failure<br/>stalled | timeout"]
    end

    %% ── RESUME DETECTION ── %%
    subgraph Resume ["RESUME DETECTION (● SKILL.md Context Limit Routing)"]
        direction LR
        T1["Tier 1: stale re-execute<br/>━━━━━━━━━━<br/>retry_reason=resume<br/>subtype=stale (compound discriminant)<br/>Re-execute with decrement<br/>NEVER routes to on_context_limit"]
        T2["Tier 2: partial resume<br/>━━━━━━━━━━<br/>retry_reason=resume (not stale)<br/>Step has on_context_limit<br/>→ follow on_context_limit"]
        T3["Tier 3: failure fallback<br/>━━━━━━━━━━<br/>All other retry_reason values<br/>(empty_output, path_contamination, etc.)<br/>→ on_failure"]
    end

    %% ── CONNECTIONS ── %%
    OP -->|"false → bypass chain"| BYPASS
    OP -->|"true → enter chain"| CMQ

    CMQ --> QA
    CMQ -->|"on_failure →"| BYPASS
    CMGT --> MGT
    CMA --> AMA

    QA -->|"feeds"| RQM
    MGT -->|"feeds"| RQM
    AMA -->|"feeds"| RQM
    AM -->|"Priority-1 gate"| RQM

    CMQ -->|"on_success"| CMGT
    CMGT -->|"on_success / on_failure"| CMA
    CMA -->|"on_success / on_failure"| RQM

    RQM -->|"P1: auto_merge≠true"| BYPASS
    RQM -->|"P2: queue+trigger+auto"| EAM
    RQM -->|"P3: queue+trigger, no auto"| QENA
    RQM -->|"P4: no queue, auto"| DM
    RQM -->|"P5: default"| IM

    EAM --> WFQ
    QENA --> WFQ
    WFQ --> QPS

    %% CLASS ASSIGNMENTS %%
    class OP,AM cli;
    class CMQ,CMGT,CMA detector;
    class QA,MGT,AMA stateNode;
    class RQM phase;
    class BYPASS,IM output;
    class EAM,DM handler;
    class QENA newComponent;
    class WFQ handler;
    class QPS stateNode;
    class T1 detector;
    class T2 gap;
    class T3 handler;
Loading

Scenarios Diagram

%%{init: {'flowchart': {'nodeSpacing': 40, 'rankSpacing': 55, 'curve': 'basis'}}}%%
flowchart LR
    %% CLASS DEFINITIONS %%
    classDef cli fill:#1a237e,stroke:#7986cb,stroke-width:2px,color:#fff;
    classDef stateNode fill:#004d40,stroke:#4db6ac,stroke-width:2px,color:#fff;
    classDef handler fill:#e65100,stroke:#ffb74d,stroke-width:2px,color:#fff;
    classDef phase fill:#6a1b9a,stroke:#ba68c8,stroke-width:2px,color:#fff;
    classDef newComponent fill:#2e7d32,stroke:#81c784,stroke-width:2px,color:#fff;
    classDef output fill:#00695c,stroke:#4db6ac,stroke-width:2px,color:#fff;
    classDef detector fill:#b71c1c,stroke:#ef5350,stroke-width:2px,color:#fff;
    classDef terminal fill:#1a237e,stroke:#7986cb,stroke-width:2px,color:#fff;

    subgraph S1 ["SCENARIO 1: Queue + Trigger + Auto Merge  (existing queue cell — now gated)"]
        direction LR
        S1_PR["PR opened<br/>━━━━━━━━━━<br/>push_to_remote / compose_pr"]
        S1_CMQ["check_merge_queue<br/>━━━━━━━━━━<br/>queue_available = true"]
        S1_CMGT["● check_merge_group_trigger<br/>━━━━━━━━━━<br/>merge_group_trigger = true"]
        S1_CAM["check_auto_merge<br/>━━━━━━━━━━<br/>auto_merge_available = true"]
        S1_RQM["● route_queue_mode<br/>━━━━━━━━━━<br/>queue=T · trigger=T · auto=T"]
        S1_EAM["enable_auto_merge<br/>━━━━━━━━━━<br/>gh pr merge --squash --auto"]
        S1_WFQ["wait_for_queue<br/>━━━━━━━━━━<br/>poll 15s / 900s deadline"]
        S1_OK(["merged ✓"])
    end

    subgraph S2 ["SCENARIO 2: Queue + Trigger + No Auto  (● new cell: queue_enqueue_no_auto)"]
        direction LR
        S2_PR["PR opened<br/>━━━━━━━━━━<br/>push_to_remote / compose_pr"]
        S2_CMQ["check_merge_queue<br/>━━━━━━━━━━<br/>queue_available = true"]
        S2_CMGT["● check_merge_group_trigger<br/>━━━━━━━━━━<br/>merge_group_trigger = true"]
        S2_CAM["check_auto_merge<br/>━━━━━━━━━━<br/>auto_merge_available = false"]
        S2_RQM["● route_queue_mode<br/>━━━━━━━━━━<br/>queue=T · trigger=T · auto=F"]
        S2_QNA["● queue_enqueue_no_auto<br/>━━━━━━━━━━<br/>gh pr merge --squash<br/>(no --auto flag)"]
        S2_WFQ["wait_for_queue<br/>━━━━━━━━━━<br/>same waiter as auto path"]
        S2_OK(["merged ✓"])
    end

    subgraph S3 ["SCENARIO 3: Queue + NO Trigger  (bug fix — was incorrectly routed to queue)"]
        direction LR
        S3_PR["PR opened<br/>━━━━━━━━━━<br/>queue exists on branch"]
        S3_CMQ["check_merge_queue<br/>━━━━━━━━━━<br/>queue_available = true"]
        S3_CMGT["● check_merge_group_trigger<br/>━━━━━━━━━━<br/>merge_group_trigger = false<br/>(no merge_group CI event)"]
        S3_CAM["check_auto_merge<br/>━━━━━━━━━━<br/>any value"]
        S3_RQM["● route_queue_mode<br/>━━━━━━━━━━<br/>queue=T · trigger=F<br/>→ falls through"]
        S3_IM["immediate_merge<br/>━━━━━━━━━━<br/>gh pr merge --squash"]
        S3_WIM["wait_for_immediate_merge<br/>━━━━━━━━━━<br/>5-min poll (correct waiter)"]
        S3_OK(["merged ✓"])
    end

    subgraph S4 ["SCENARIO 4: No Queue Baseline  (unchanged fallback)"]
        direction LR
        S4_PR["PR opened<br/>━━━━━━━━━━<br/>no merge queue on branch"]
        S4_CMQ["check_merge_queue<br/>━━━━━━━━━━<br/>queue_available = false"]
        S4_RQM["● route_queue_mode<br/>━━━━━━━━━━<br/>queue=F · auto=F<br/>→ immediate path"]
        S4_IM["immediate_merge<br/>━━━━━━━━━━<br/>gh pr merge --squash"]
        S4_WIM["wait_for_immediate_merge<br/>━━━━━━━━━━<br/>5-min poll"]
        S4_OK(["merged ✓"])
    end

    subgraph S5 ["SCENARIO 5: sous-chef Contract Alignment  (● SKILL.md + test coverage)"]
        direction LR
        S5_SC["● sous-chef/SKILL.md<br/>━━━━━━━━━━<br/>MERGE PHASE section<br/>documents routing matrix"]
        S5_CT["● test_sous_chef_routing.py<br/>━━━━━━━━━━<br/>assert queue_enqueue_no_auto<br/>+ condition in SKILL.md"]
        S5_MQ["● test_merge_prs_queue.py<br/>━━━━━━━━━━<br/>assert routing matrix<br/>+ queue_enqueue_no_auto step"]
        S5_REC["● 3 recipe YAMLs<br/>━━━━━━━━━━<br/>implementation / impl-groups<br/>/ remediation"]
        S5_OK(["contracts satisfied ✓"])
    end

    %% SCENARIO 1 FLOW %%
    S1_PR --> S1_CMQ --> S1_CMGT --> S1_CAM --> S1_RQM --> S1_EAM --> S1_WFQ --> S1_OK

    %% SCENARIO 2 FLOW %%
    S2_PR --> S2_CMQ --> S2_CMGT --> S2_CAM --> S2_RQM --> S2_QNA --> S2_WFQ --> S2_OK

    %% SCENARIO 3 FLOW %%
    S3_PR --> S3_CMQ --> S3_CMGT --> S3_CAM --> S3_RQM --> S3_IM --> S3_WIM --> S3_OK

    %% SCENARIO 4 FLOW %%
    S4_PR --> S4_CMQ --> S4_RQM --> S4_IM --> S4_WIM --> S4_OK

    %% SCENARIO 5 FLOW %%
    S5_SC -- "documents" --> S5_CT
    S5_CT -- "validates" --> S5_REC
    S5_REC -- "tested by" --> S5_MQ
    S5_MQ --> S5_OK

    %% CLASS ASSIGNMENTS %%
    class S1_PR,S2_PR,S3_PR,S4_PR cli;
    class S1_CMQ,S2_CMQ,S3_CMQ,S4_CMQ phase;
    class S1_CMGT,S2_CMGT,S3_CMGT handler;
    class S1_CAM,S2_CAM,S3_CAM phase;
    class S1_RQM,S2_RQM,S3_RQM,S4_RQM handler;
    class S1_EAM,S3_IM,S4_IM handler;
    class S2_QNA newComponent;
    class S1_WFQ,S2_WFQ,S3_WIM,S4_WIM stateNode;
    class S1_OK,S2_OK,S3_OK,S4_OK,S5_OK terminal;
    class S5_SC,S5_REC output;
    class S5_CT,S5_MQ detector;
Loading

Closes #755

Implementation Plan

Plan file: /home/talon/projects/autoskillit-runs/impl-20260412-092011-605768/.autoskillit/temp/make-plan/route_queue_mode_no_auto_merge_plan_2026-04-12_092640.md

🤖 Generated with Claude Code via AutoSkillit

Token Usage Summary

Step uncached output cache_read cache_write count time
plan 289 32.7k 1.8M 160.3k 1 8m 19s
verify 150 23.8k 960.9k 63.9k 1 5m 55s
implement 286 17.3k 1.7M 61.4k 1 6m 1s
fix 214 8.4k 917.7k 33.6k 1 8m 17s
prepare_pr 60 6.3k 192.3k 26.2k 1 1m 40s
run_arch_lenses 221 32.7k 684.0k 208.0k 3 15m 31s
compose_pr 59 10.3k 214.3k 35.7k 1 2m 37s
Total 1.3k 131.6k 6.4M 589.1k 48m 24s

Copy link
Copy Markdown
Collaborator Author

@Trecek Trecek left a comment

Choose a reason for hiding this comment

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

AutoSkillit PR Review — Verdict: changes_requested

# ---------------------------------------------------------------------------


def test_route_queue_mode_queue_with_auto_routes_to_enable_auto_merge(any_recipe) -> None:
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[warning] tests: test_route_queue_mode_queue_with_auto_routes_to_enable_auto_merge uses a fragile string-split heuristic (c.when.split("auto_merge_available")[1]) to check for "== true" after the variable name. This can produce false positives or false negatives if the condition string has unexpected whitespace or ordering. A direct string equality assertion would be more robust and less brittle.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Investigated — this is intentional. The split heuristic is not fragile given the fixed YAML condition strings: c.when.split('auto_merge_available')[1] yields ' }} == true' for the enable_auto_merge condition, making '== true' in ... unambiguously true. The identical pattern is already established at line 943 (assert "}} == true" in cond.when.split("auto_merge_available")[1]). The pre-filters on 'queue_available' and 'merge_group_trigger' uniquely select one condition, removing any ambiguity.

assert cond.route == "enable_auto_merge"


def test_route_queue_mode_queue_without_auto_routes_to_queue_enqueue_no_auto(
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[warning] tests: test_route_queue_mode_queue_without_auto_routes_to_queue_enqueue_no_auto uses the same fragile c.when.split("auto_merge_available")[1] heuristic to detect "== false". If the condition string changes ordering or whitespace, the split index may not contain the expected substring. Prefer asserting the full condition string directly.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Investigated — this is intentional. Same analysis as the sibling comment: c.when.split('auto_merge_available')[1] yields ' }} == false' for the queue_enqueue_no_auto condition — '== false' in ... is unambiguously true. The YAML condition format is fixed and authoritative (${{ context.auto_merge_available }} == false). The split-based heuristic is the established pattern in this file (see line 943); the reviewer's concern about ordering or whitespace does not apply to these fixed-format YAML strings.

)


# ---------------------------------------------------------------------------
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[warning] slop: Horizontal rule separator comment (Routing matrix section) adds visual noise without conveying information not already present in the section heading below it. These ASCII dividers are a common AI-generated padding pattern.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Investigated — this is intentional. The # --- horizontal divider is an established convention in this file since its first commit (8b73c49), which introduced 18 such dividers. The file now has 28+ dividers following the same triple-block pattern (divider / section heading / divider). The same style appears in test_merge_prs.py. The new dividers at lines 877 and 956 are stylistically identical to all pre-existing instances — characterizing them as AI padding misidentifies a longstanding project convention.

assert count == 1, f"Expected exactly 1 enable_auto_merge route, got {count}"


# ---------------------------------------------------------------------------
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[warning] slop: Horizontal rule separator comment (New step section) adds visual noise without conveying information not already present in the section heading below it. These ASCII dividers are a common AI-generated padding pattern.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Investigated — this is intentional. Same analysis as the sibling comment on line 877: the # --- triple-block pattern (divider / heading / divider) is the consistent section-separation convention throughout test_merge_prs_queue.py since its creation in commit 8b73c49. There are 28+ pre-existing instances of this exact pattern in the file. This is not AI-generated padding — it is the established style for section boundaries in this test module.

assert cond.route == "immediate_merge"


def test_route_queue_mode_never_routes_to_enable_auto_merge_when_auto_unavailable(
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[info] tests: test_route_queue_mode_never_routes_to_enable_auto_merge_when_auto_unavailable overlaps significantly with test_route_queue_mode_queue_with_auto_routes_to_enable_auto_merge and test_enable_auto_merge_route_count. The invariant is already implied by the single-count and positive-path tests. The extra loop-based test adds coverage but reads as a redundant guard.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Investigated — this is intentional. test_route_queue_mode_never_routes_to_enable_auto_merge_when_auto_unavailable is a distinct negative invariant: it iterates ALL conditions and asserts that any route to enable_auto_merge must require auto_merge_available == true. Neither the count test (test_enable_auto_merge_route_count, line 949) nor the positive-path test (test_route_queue_mode_queue_with_auto_routes_to_enable_auto_merge, line 882) would catch a future regression where a second route to enable_auto_merge is added without the auto_merge_available guard. This test is a complementary safety invariant, not a redundant copy of its siblings (commit ba26ae1).

# ---------------------------------------------------------------------------


def test_queue_enqueue_no_auto_step_exists(any_recipe) -> None:
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[info] tests: 5 separate single-assertion tests for queue_enqueue_no_auto each independently look up the step. These are highly granular and could be consolidated into one grouped test, reducing fixture overhead without loss of coverage.

return "\n".join(extracted)


def test_sous_chef_merge_phase_documents_queue_no_auto_path() -> None:
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[info] tests: test_sous_chef_merge_phase_documents_queue_no_auto_path asserts a prose condition string rather than the YAML template syntax. If the documentation uses template syntax instead of prose, this assertion will fail. Ensure the assertion matches actual SKILL.md content.

Copy link
Copy Markdown
Collaborator Author

@Trecek Trecek left a comment

Choose a reason for hiding this comment

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

AutoSkillit review found 4 blocking issues (2 warning/tests, 2 warning/slop). See inline comments.

@Trecek Trecek added this pull request to the merge queue Apr 12, 2026
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 12, 2026
Trecek added 4 commits April 12, 2026 12:01
…ing matrix

Introduces a four-cell routing matrix over queue_available × auto_merge_available
in implementation.yaml, implementation-groups.yaml, and remediation.yaml.

Adds new queue_enqueue_no_auto step that uses plain `gh pr merge --squash`
(without --auto) for repos with a merge queue but autoMergeAllowed=false.
Updates sous-chef SKILL.md MERGE PHASE rules to document all four routing cells
and extend the bypass prohibition to include queue_enqueue_no_auto.
Adds 13 new parametrized tests covering the four-cell queue×auto routing
matrix and the new queue_enqueue_no_auto step shape across all three recipes.
Updates test_route_queue_mode_auto_merge_available_routes_to_direct_merge to
disambiguate the direct_merge condition from the new queue+no-auto condition.
Adds contract test asserting SKILL.md MERGE PHASE documents the new routing cell.
…r_merge finds the prohibition

The test checks for literal 'NEVER use' substring within 300 chars of 'gh pr merge'.
The bold marker placement **NEVER** use split the substring; changed to **NEVER use**
so the string 'NEVER use' is present in the raw file text.
@Trecek Trecek force-pushed the route-queue-mode-incorrect-routing-when-branch-has-merge-que/755 branch from 0c7ce23 to 9a4c21c Compare April 12, 2026 19:01
@Trecek Trecek enabled auto-merge April 12, 2026 19:06
… cancellation

When anyio cancels the lifespan task (via KeyboardInterrupt → CancelledError),
CancelledError is thrown at the yield point. Without try/finally, the cleanup
code after yield is skipped entirely and recorder.finalize() never runs, leaving
scenario.json unwritten. Wrapping yield in try/finally guarantees finalize() is
called regardless of how the lifespan exits. Adds regression test that verifies
finalize() is called when CancelledError is raised inside the lifespan context.

Fixes issue #745.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@Trecek Trecek added this pull request to the merge queue Apr 12, 2026
Merged via the queue into integration with commit 0f35d85 Apr 12, 2026
2 checks passed
@Trecek Trecek deleted the route-queue-mode-incorrect-routing-when-branch-has-merge-que/755 branch April 12, 2026 19:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant