fix(sync): enforce canary promotion boundary - #3039
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
📝 WalkthroughWalkthroughThe PR adds stable candidate-branch handling for configured canaries. It restricts canary selections, updates drift remediation, routes workflow PR operations through the computed branch, validates candidate refreshes, and updates tests and documentation. ChangesConsumer sync delivery
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant WorkflowPrepare
participant PhaseSelection
participant ConsumerSyncPR
participant ResultArtifact
WorkflowPrepare->>PhaseSelection: phase and compiled template hash
PhaseSelection->>ConsumerSyncPR: selected sync branch
ConsumerSyncPR->>ResultArtifact: expected_branch
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
GitHub Actions
Inactive
GitHub Actions
Inactive
Automated Status SummaryHead SHA: f2671b6
Coverage Overview
Coverage Trend
Top Coverage Hotspots (lowest coverage)
Low Coverage Files (<50.0%)
Updated automatically; will refresh on subsequent CI/Docker completions. Keepalive checklistScopeNo scope information available Tasks
Acceptance criteria
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a36357f17a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| states: dict[str, dict[str, object]] = {} | ||
| for repo in repos: | ||
| repo_expected_branch = SYNC_CANDIDATE_BRANCH if repo in candidate_repos else expected_branch |
There was a problem hiding this comment.
When a template or manifest change creates a new plan_id while a canary still has a recently updated PR on sync/workflows-candidate, this branch substitution makes that old PR count as covered; the subsequent checks validate only branch, head repo, and age, because fetch_open_sync_prs does not retrieve or validate the PR body's plan_id. Health 68 can therefore exit successfully and suppress the drift issue even though the candidate PR belongs to the previous plan. Parse the sync metadata and require its plan ID to equal current_plan_id before granting coverage.
Useful? React with 👍 / 👎.
| if not repos or not repos <= set(registered_repos): | ||
| raise ValueError("consumer sync canaries must be registered repositories") |
There was a problem hiding this comment.
When Health 68 is manually dispatched with its documented repos input containing only one or a few repositories, registered_repos here is actually that selected subset, so requiring every configured canary to be contained in it rejects almost every targeted check as an invalid canary configuration. For example, checking only stranske/Ready always fails before comparing any files. Validate the canary configuration against the authoritative fleet separately, then intersect the canaries with the repositories being checked.
Useful? React with 👍 / 👎.
| Do not wait for consumer CI in that workflow. Run Maint 71 later with | ||
| `active_sync_hash=candidate` to publish `sync-canary-evidence.json`, then invoke | ||
| Maint 68 with `phase=promote` and that artifact's JSON as |
There was a problem hiding this comment.
The Maint 71 workflow declares the input as sync_hash in .github/workflows/maint-71-merge-sync-prs.yml, not active_sync_hash. The local gh workflow run --help confirms that -f supplies workflow inputs as key=value, so an operator following this instruction cannot target sync/workflows-candidate and will receive an unexpected-input dispatch error. Change this guidance to sync_hash=candidate and show the corresponding dispatch flag.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/workflows/maint-68-sync-consumer-repos.yml:
- Around line 806-820: Update the selected PR lookup and candidate-rotation
validation to retrieve the PR head repository full name, then require it to
equal the matrix repository value before accepting rotation. Use this trusted
head repository value rather than the PR-controlled record.repository field,
while preserving the existing terminal-disposition and candidate-plan checks in
isStableCandidate.
In `@docs/ops/CONSUMER_REPO_MAINTENANCE.md`:
- Around line 283-285: Update the promotion-scope documentation in
CONSUMER_REPO_MAINTENANCE.md to distinguish unfiltered and filtered promotions:
state that no-filter promotions target all registered non-canary repositories,
while filtered promotions target only the selected registered non-canaries.
In `@scripts/check_consumer_sync_drift.py`:
- Around line 100-113: Update resolve_candidate_repos and its caller to validate
configured canaries against the complete registered fleet before intersecting
them with the requested --repos subset. Preserve rejection of unregistered
canaries, return only canaries selected by the requested targets, and add
coverage for a one-canary --repos run when other configured canaries exist.
In `@scripts/select_consumer_sync_phase.py`:
- Around line 157-163: Update the selected_repos validation before assigning
selected to detect duplicate manual canary repository names and raise
PhaseSelectionError instead of allowing repeated entries. Preserve the existing
non-canary validation and error behavior, and add a test covering duplicate
canary input.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 879198e4-c541-480d-a871-122adaab1447
📒 Files selected for processing (11)
.github/scripts/__tests__/sync_pr_merge_contract.test.js.github/workflows/maint-68-sync-consumer-repos.ymldocs/WORKFLOW_GUIDE.mddocs/ci/WORKFLOWS.mddocs/ci/WORKFLOW_SYSTEM.mddocs/ops/CONSUMER_REPO_MAINTENANCE.mdscripts/check_consumer_sync_drift.pyscripts/select_consumer_sync_phase.pytests/scripts/test_check_consumer_sync_drift.pytests/scripts/test_select_consumer_sync_phase.pytests/workflows/test_sync_manifest_delivery.py
| const isStableCandidate = | ||
| process.env.SYNC_PHASE === "canary" && | ||
| process.env.SYNC_BRANCH === "sync/workflows-candidate"; | ||
| if (isStableCandidate) { | ||
| if (record.repository !== (process.env.DELIVERY_REPOSITORY || "")) { | ||
| process.stdout.write("false repository_mismatch"); | ||
| process.exit(0); | ||
| } | ||
| if (record.terminal_disposition) { | ||
| process.stdout.write(`false terminal:${record.terminal_disposition}`); | ||
| process.exit(0); | ||
| } | ||
| process.stdout.write("true candidate_plan_rotation"); | ||
| process.exit(0); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 6 \
'gh pr list --head|isConsumerOpenPr|head\.repo|head_repo|full_name|candidate_plan_rotation' \
.github/workflows/maint-68-sync-consumer-repos.yml .github/scriptsRepository: stranske/Workflows
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- workflow lookup and refresh path ---'
sed -n '710,855p' .github/workflows/maint-68-sync-consumer-repos.yml
printf '%s\n' '--- isConsumerOpenPr implementation ---'
sed -n '390,455p' .github/scripts/sync_tracker_state/index.js
printf '%s\n' '--- all workflow copies ---'
find .github/workflows templates/consumer-repo/.github/workflows -maxdepth 1 -type f \
\( -name 'maint-68-sync-consumer-repos.yml' -o -name 'maint-68-sync-consumer-repos.yaml' \) -printRepository: stranske/Workflows
Length of output: 9627
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- workflow setup and refresh execution ---'
rg -n -C 8 \
'actions/checkout|cd consumer|gh pr list|gh pr view|gh pr edit|existing_refreshable|existing_pr|git fetch origin' \
.github/workflows/maint-68-sync-consumer-repos.yml
printf '%s\n' '--- template workflow inventory ---'
find templates -type f 2>/dev/null | head -80 || true
rg -n 'maint-68-sync-consumer-repos|sync/workflows-candidate' templates .github/workflows 2>/dev/null || true
printf '%s\n' '--- representative acceptance-path verifier ---'
python3 - <<'PY'
import re
workflow = open('.github/workflows/maint-68-sync-consumer-repos.yml', encoding='utf-8').read()
helper = open('.github/scripts/sync_tracker_state/index.js', encoding='utf-8').read()
lookup = re.search(
r'existing_pr=\$\(gh pr list --head "\$branch_name" --json number -q \'\.\[0\]\.number\'',
workflow,
)
view_body_only = 'gh pr view "$existing_pr" --json body -q .body' in workflow
fetch_origin_branch = 'git fetch origin "$branch_name"' in workflow
helper_uses_head_repo = bool(re.search(r'head\?\.repo|head\.repo|headRepo|full_name', helper[helper.index('async function isConsumerOpenPr'):helper.index('function formatStuckWindowMarker')]))
helper_returns_boolean = 'return pulls.some' in helper[helper.index('async function isConsumerOpenPr'):helper.index('function formatStuckWindowMarker')]
def candidate_accepts(record, delivery_repo):
return (
record.get('repository') == delivery_repo
and not record.get('terminal_disposition')
)
cases = [
('consumer PR', {'repository': 'owner/consumer', 'terminal_disposition': ''}),
('fork PR with forged body', {'repository': 'owner/consumer', 'terminal_disposition': ''}),
]
print({
'lookup_requests_only_number': bool(lookup),
'body_query_omits_head_repo': view_body_only,
'origin_fetches_branch_before_refresh': fetch_origin_branch,
'isConsumerOpenPr_reads_head_repo': helper_uses_head_repo,
'isConsumerOpenPr_returns_only_match_boolean': helper_returns_boolean,
'candidate_body_acceptance': {
name: candidate_accepts(record, 'owner/consumer')
for name, record in cases
},
})
PYRepository: stranske/Workflows
Length of output: 263
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- workflow setup and refresh execution ---'
rg -n -C 8 'actions/checkout|cd consumer|gh pr list|gh pr view|gh pr edit|existing_refreshable|existing_pr|git fetch origin' \
.github/workflows/maint-68-sync-consumer-repos.yml || true
printf '%s\n' '--- template workflow inventory ---'
if [ -d templates ]; then
find templates -type f | head -80
else
echo 'templates directory is absent'
fi
rg -n 'maint-68-sync-consumer-repos|sync/workflows-candidate' templates .github/workflows 2>/dev/null || true
printf '%s\n' '--- representative acceptance-path verifier ---'
python3 - <<'PY'
import re
workflow = open('.github/workflows/maint-68-sync-consumer-repos.yml', encoding='utf-8').read()
helper = open('.github/scripts/sync_tracker_state/index.js', encoding='utf-8').read()
helper_start = helper.index('async function isConsumerOpenPr')
helper_end = helper.index('function formatStuckWindowMarker')
helper_body = helper[helper_start:helper_end]
lookup = re.search(
r'existing_pr=\$\(gh pr list --head "\$branch_name" --json number -q',
workflow,
)
print({
'lookup_requests_only_number': bool(lookup),
'body_query_omits_head_repo': 'gh pr view "$existing_pr" --json body -q .body' in workflow,
'origin_fetches_branch_before_refresh': 'git fetch origin "$branch_name"' in workflow,
'isConsumerOpenPr_reads_head_repo': bool(re.search(r'head\?\.repo|head\.repo|headRepo|full_name', helper_body)),
'isConsumerOpenPr_returns_only_match_boolean': 'return pulls.some' in helper_body,
})
def candidate_accepts(record, delivery_repo):
return (
record.get('repository') == delivery_repo
and not record.get('terminal_disposition')
)
print({
'same_repo_record': candidate_accepts(
{'repository': 'owner/consumer', 'terminal_disposition': ''},
'owner/consumer',
),
'fork_pr_with_forged_same_repo_record': candidate_accepts(
{'repository': 'owner/consumer', 'terminal_disposition': ''},
'owner/consumer',
),
})
PYRepository: stranske/Workflows
Length of output: 5077
🌐 Web query:
GitHub CLI gh pr list --head branch name fork pull requests documentation
💡 Result:
To list pull requests filtered by a specific head branch name in the GitHub CLI, use the --head flag with the gh pr list command [1][2]. The command syntax is: gh pr list --head Key details regarding the --head flag: - It filters pull requests by the specified head branch name [1][3]. - It does not support the ": " syntax (e.g., owner:branch) [1][4]. - The flag is case-sensitive and requires the exact branch name [1]. For example, to list all open pull requests where the head branch is named "feature-typo", run: gh pr list --head "feature-typo" This command works for pull requests regardless of whether they originate from a fork or the main repository, provided the branch name matches the specified filter [1][5]. If you need to filter by a specific repository (such as a fork) in addition to the branch name, you can use the -R or --repo flag to specify the target repository in the format [HOST/]OWNER/REPO [5][3].
Citations:
- 1: https://cli.github.com/manual/gh_pr_list
- 2: https://man.archlinux.org/man/gh-pr-list.1.en
- 3: https://cli.github.com/manual/gh_search_prs
- 4: Add example usage of
--headoption topr listdocs cli/cli#10979 - 5: https://manpages.debian.org/bookworm/gh/gh-pr-list.1
Validate the selected PR’s head repository before candidate rotation.
gh pr list --head "$branch_name" also matches fork PRs, but the lookup requests only the PR number. Candidate eligibility then trusts the PR-controlled record.repository field. Require the selected PR’s head.repo.full_name to equal ${{ matrix.repo }} before accepting rotation. isConsumerOpenPr is insufficient because it returns only a branch-match boolean.
🤖 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/maint-68-sync-consumer-repos.yml around lines 806 - 820,
Update the selected PR lookup and candidate-rotation validation to retrieve the
PR head repository full name, then require it to equal the matrix repository
value before accepting rotation. Use this trusted head repository value rather
than the PR-controlled record.repository field, while preserving the existing
terminal-disposition and candidate-plan checks in isStableCandidate.
Source: Path instructions
| threads. A successful promotion targets all registered non-canary repositories | ||
| once every configured canary has current, green, review-clear evidence for the | ||
| same plan. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate files and references ---'
rg -n --glob '!node_modules' --glob '!dist' --glob '!build' \
'select_phase|selected_repos|target_repos|registered non-canary|successful promotion|canary' .
printf '%s\n' '--- documentation context ---'
file=$(git ls-files | grep -F 'docs/ops/CONSUMER_REPO_MAINTENANCE.md' | head -n1)
[ -n "$file" ]
sed -n '265,295p' "$file"
sed -n '460,490p' "$file"Repository: stranske/Workflows
Length of output: 18670
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- selector implementation ---'
sed -n '120,188p' scripts/select_consumer_sync_phase.py
printf '%s\n' '--- selector tests ---'
sed -n '100,155p' tests/scripts/test_select_consumer_sync_phase.py
printf '%s\n' '--- workflow input and call path ---'
rg -n -A12 -B8 'selected_repos|repos:|select_phase\(' .github scripts tests \
--glob '*.yml' --glob '*.yaml' --glob '*.py'Repository: stranske/Workflows
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
import ast
path = Path("scripts/select_consumer_sync_phase.py")
tree = ast.parse(path.read_text(encoding="utf-8"))
fn = next(
node for node in tree.body
if isinstance(node, ast.FunctionDef) and node.name == "select_phase"
)
assignments = {}
for node in ast.walk(fn):
if isinstance(node, ast.Assign) and len(node.targets) == 1:
target = node.targets[0]
if isinstance(target, ast.Name):
assignments[target.id] = ast.unparse(node.value)
assert "target_repos if selected_repos is not None else registered_repos" in assignments["target_repos"]
assert "[repo for repo in target_repos if repo not in canary_repos]" in assignments["selected"]
print("source rule: target_repos = selected_repos when provided; promote filters that list to non-canaries")
def promotion_targets(registered, canaries, selected_repos=None):
target_repos = selected_repos if selected_repos is not None else registered
return [repo for repo in target_repos if repo not in canaries]
registered = ["canary-a", "canary-b", "consumer-a", "consumer-b"]
canaries = ["canary-a", "canary-b"]
assert promotion_targets(registered, canaries) == ["consumer-a", "consumer-b"]
assert promotion_targets(registered, canaries, ["consumer-b"]) == ["consumer-b"]
print("behavior: no filter targets all non-canaries; a supplied filter narrows promotion")
PYRepository: stranske/Workflows
Length of output: 245
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
import ast
tree = ast.parse(Path("scripts/select_consumer_sync_phase.py").read_text(encoding="utf-8"))
fn = next(node for node in tree.body if isinstance(node, ast.FunctionDef) and node.name == "select_phase")
for node in ast.walk(fn):
if isinstance(node, ast.Assign) and len(node.targets) == 1:
target = node.targets[0]
if isinstance(target, ast.Name) and target.id in {"target_repos", "selected"}:
print(f"{target.id} = {ast.unparse(node.value)}")
PYRepository: stranske/Workflows
Length of output: 373
Document filtered promotion scope.
A no-filter promotion targets all registered non-canaries. A filtered promotion targets only the selected registered non-canaries.
🤖 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 `@docs/ops/CONSUMER_REPO_MAINTENANCE.md` around lines 283 - 285, Update the
promotion-scope documentation in CONSUMER_REPO_MAINTENANCE.md to distinguish
unfiltered and filtered promotions: state that no-filter promotions target all
registered non-canary repositories, while filtered promotions target only the
selected registered non-canaries.
| def resolve_candidate_repos(path: str, registered_repos: list[str]) -> set[str]: | ||
| """Load the bounded canary set used by Maint 68 candidate branches.""" | ||
| config = json.loads(Path(path).read_text(encoding="utf-8")) | ||
| canaries = config.get("canaries") if isinstance(config, dict) else None | ||
| if not isinstance(canaries, list): | ||
| raise ValueError("consumer sync canary config has no canaries list") | ||
| repos = { | ||
| str(item.get("repo", "")) | ||
| for item in canaries | ||
| if isinstance(item, dict) and item.get("repo") | ||
| } | ||
| if not repos or not repos <= set(registered_repos): | ||
| raise ValueError("consumer sync canaries must be registered repositories") | ||
| return repos |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Validate canaries against the fleet, not the requested subset.
Line 111 requires every configured canary to exist in repos. Line 918 passes the --repos target subset as repos. A run for one configured canary fails if the configuration contains another canary.
Load the full registered fleet for configuration validation. Then intersect the validated canary set with the requested target repos. Add a test for a one-canary --repos run.
As per path instructions, “Prioritize correctness, error handling, and test coverage.”
Also applies to: 917-919
🤖 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_consumer_sync_drift.py` around lines 100 - 113, Update
resolve_candidate_repos and its caller to validate configured canaries against
the complete registered fleet before intersecting them with the requested
--repos subset. Preserve rejection of unregistered canaries, return only
canaries selected by the requested targets, and add coverage for a one-canary
--repos run when other configured canaries exist.
Source: Path instructions
| if selected_repos is not None: | ||
| non_canaries = [repo for repo in selected_repos if repo not in canary_repos] | ||
| if non_canaries: | ||
| raise PhaseSelectionError( | ||
| "canary_selection_contains_non_canary:" + ",".join(non_canaries) | ||
| ) | ||
| selected = selected_repos |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Reject duplicate manual canary repositories.
Line 163 preserves duplicate values in selected_repos. A manual input such as canary-a,canary-a creates concurrent sync jobs for the same repository and stable candidate branch. The jobs can race during PR refresh and force-with-lease push.
Reject duplicate values before assigning selected. Add a duplicate-selection test.
As per path instructions, “Prioritize correctness, error handling, and test coverage.”
🤖 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/select_consumer_sync_phase.py` around lines 157 - 163, Update the
selected_repos validation before assigning selected to detect duplicate manual
canary repository names and raise PhaseSelectionError instead of allowing
repeated entries. Preserve the existing non-canary validation and error
behavior, and add a test covering duplicate canary input.
Source: Path instructions
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
Provider Comparison ReportProvider Summary
📋 Full Provider Details (click to expand)openai
anthropic
Agreement
Disagreement
Unique Insights
🔍 LangSmith Traces |
3 participants
Summary
sync/workflows-candidatePRs while a candidate plan is being repairedactive_sync_hash=candidatehandoffWhy
Candidate corrections were being dispatched directly across the fleet through the canary repo filter, producing a replacement PR wave after every source adjustment. This change turns the existing canary/evidence/promote design into an enforced boundary: repair one stable canary candidate, collect exact-plan evidence, then fan out once.
Validation
python -m pytest -q tests/scripts/test_select_consumer_sync_phase.py tests/scripts/test_check_consumer_sync_drift.py tests/workflows/test_sync_manifest_delivery.py(51 passed)node --test .github/scripts/__tests__/sync_pr_merge_contract.test.js(23 passed)ruff checkon changed Python and testsactionlint .github/workflows/maint-68-sync-consumer-repos.ymlpython scripts/validate_template_sync.pypython scripts/validate_template_completeness.pybash scripts/sync_templates.sh --checkgit diff --checkSummary by CodeRabbit
New Features
Bug Fixes
Documentation