Honor repository branch-name policy before builder PR creation - #971
Conversation
jeffhuber
added
builder:codex
Codex audit (merge-authority lane)Head SHA: Codex Audit: BLOCKED Summary: Policy-based branch discovery can hide an existing PR after an issue title changes, allowing duplicate delivery on explicit retries. Findings:
|
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| def compile_template(template: Any, *, configured: bool = True) -> BranchPolicy: | ||
| """Validate a template and derive its accepted pattern and one example.""" | ||
| if not isinstance(template, str) or not template.strip(): | ||
| raise BranchPolicyError("branch_template must be a non-empty string") | ||
| if template != template.strip() or "{" in _VARIABLE_RE.sub("", template) \ | ||
| or "}" in _VARIABLE_RE.sub("", template): | ||
| raise BranchPolicyError("branch_template has unbalanced braces or surrounding whitespace") | ||
| names = _variables(template) | ||
| if "issue_key" not in names and "issue_number" not in names: | ||
| raise BranchPolicyError( | ||
| "branch_template must include {issue_key} or {issue_number} so each work item " | ||
| "resolves to its own branch" | ||
| ) | ||
| pattern = _pattern(template) | ||
| example = render(template, EXAMPLE_VALUES) |
There was a problem hiding this comment.
💡 Edge Case: Leading {slug} in a branch_template silently breaks on empty slugs
compile_template() always renders its validation example with EXAMPLE_VALUES["slug"] non-empty (branch_policy.py:100, 28-35), so a template like {slug}-{issue_key} or {slug}/{issue_key} (with no character before {slug}) passes config validation. _OPTIONAL_SLUG_RE (branch_policy.py:39) only strips a preceding separator, so at runtime, when a real issue title slugifies to empty (e.g. an emoji-only title), render() produces a branch starting with - (e.g. -ABC-1), which is_valid_ref then rejects, failing the whole delivery for that issue. It fails closed, but the admin's config was accepted as valid and the failure only surfaces later on specific issue titles. Consider having compile_template also render once with an empty slug to catch this class of template at config-validation time, or document that {slug} must not be the first token.
Validate the template with both an empty and non-empty slug during compile_template:
pattern = _pattern(template)
example = render(template, EXAMPLE_VALUES)
if not is_valid_ref(example) or re.fullmatch(pattern, example) is None:
raise BranchPolicyError(
f"branch_template {template!r} does not render to a valid git branch name"
)
empty_slug_values = {**EXAMPLE_VALUES, "slug": ""}
empty_slug_example = render(template, empty_slug_values)
if not is_valid_ref(empty_slug_example):
raise BranchPolicyError(
f"branch_template {template!r} does not render to a valid git branch name when "
"the slug is empty; {slug} must be preceded by -, _, /, or . in the template"
)
Was this helpful? React with 👍 / 👎
| if self.branch_pattern and ( | ||
| not _branch(self.branch_example) | ||
| or re.fullmatch(self.branch_pattern, self.branch) is None): | ||
| raise RemoteError( | ||
| f"branch_policy_mismatch: branch must match {self.branch_pattern} " | ||
| f"(for example {self.branch_example})") |
There was a problem hiding this comment.
💡 Quality: WorkOrder direct construction can bypass safe branch_pattern validation
WorkOrder.__post_init__ (devin_work_orders.py:188-193) calls re.fullmatch(self.branch_pattern, self.branch) directly on self.branch_pattern without catching re.error. Every current call path builds branch_pattern via branch_policy.compile_template(), which validates the pattern compiles, so this isn't reachable today. But WorkOrder is a public frozen dataclass constructible directly (as tests already do, e.g. test_branch_policy.py:220-221), so a caller that passes an arbitrary branch_pattern string would get an unhandled re.error instead of the RemoteError the rest of validation consistently raises. Wrapping the re.fullmatch call in a try/except would make the invariant self-enforcing rather than relying on every caller going through compile_template.
Catch re.error from a malformed branch_pattern and raise RemoteError instead:
if self.branch_pattern:
try:
matched = re.fullmatch(self.branch_pattern, self.branch)
except re.error as exc:
raise RemoteError(f"branch_policy_mismatch: invalid branch_pattern: {exc}") from None
if not _branch(self.branch_example) or matched is None:
raise RemoteError(
f"branch_policy_mismatch: branch must match {self.branch_pattern} "
f"(for example {self.branch_example})")
Was this helpful? React with 👍 / 👎
|
Note Automatic reviews are paused because your team has used its included automatic processing for this billing period (headroom scales with your seat count). You can still comment "Gitar review" to run one anytime, and automatic reviews resume on their own by October 1. Add seats for more headroom. Code Review 👍 Approved with suggestions 0 resolved / 2 findingsEnforces repository branch-naming policy before builder PR creation, with exact-head provenance validation and fail-closed cross-lane handoff. Two minor suggestions: 💡 Edge Case: Leading {slug} in a branch_template silently breaks on empty slugs📄 src/code_mower/branch_policy.py:86-100 📄 src/code_mower/branch_policy.py:39 📄 src/code_mower/branch_policy.py:125-129
Validate the template with both an empty and non-empty slug during compile_template💡 Quality: WorkOrder direct construction can bypass safe branch_pattern validation📄 src/code_mower/devin_work_orders.py:188-193
Catch re.error from a malformed branch_pattern and raise RemoteError instead🤖 Prompt for agentsOptionsDisplay: compact → Counting what did not apply, without listing it. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
Codex audit (merge-authority lane)Head SHA: Codex Audit: BLOCKED Summary: Three P2 findings concern existing-delivery discovery, closing-issue repository matching, and push authority retained across runs. Findings:
|
Codex audit (merge-authority lane)Head SHA: Codex Audit: BLOCKED Summary: Issue selection introduces a repeated-enumeration performance regression for busy repositories. Tests could not run because PyYAML is missing. Findings:
|
Codex audit (merge-authority lane)Head SHA: Codex Audit: BLOCKED Summary: Policy enforcement is incomplete during accepted recovery handoffs, and two existing regression tests retain incompatible GitHub fixtures. These issues should be corrected before merging. Findings:
|
Codex audit (merge-authority lane)Head SHA: Codex Audit: PASS Summary: No actionable regressions were identified in the diff or affected delivery paths. Assessment was based on static review; tests were not run in the read-only environment. Findings: none. |
Claude audit (merge-authority lane)Head SHA: Claude Audit: PASS Summary: Large, carefully engineered PR adding an optional per-repository branch-name policy layer for builder delivery (Python resolver in branch_policy.py, config validation, Devin work-order integration, and a parallel jq/bash re-implementation embedded in the generated/checked-in run_mac_lane.sh runner). Reviewed the resolver/validator, the config schema additions, the WorkOrder binding changes (including the new required config/branch_policy argument), and the pre-push guard's single-writer/provenance logic (label+author conflict detection, existing-branch head pinning, ledger symlink hardening, TOCTOU handling via git's pre-push remote-sha reporting). The design is sound, fails closed in ambiguous/foreign-ownership cases, and is backed by extensive matching test coverage (958-line test_branch_policy.py) including bash-vs-Python parity checks. No P0/P1/P2 correctness, security, or data-loss issues found. Findings:
|
1 participant
Issue #865 requires builder delivery to honor each target repository's configured branch naming policy before provider execution while preserving strict builder ownership and handoff boundaries.
This replacement supersedes closed PR #970 after its hosted-Devin qualification run stopped with unresolved exact-head audit findings. The Code Mower Codex builder reconstructed the implementation on a fresh branch without Devin commit history and fixed the remaining defects: existing policy branches now require same-lane provenance at the exact observed remote head, branch absence or the inspected SHA is pinned before provider writes, and configured policy is enforced before ownership or handoff evaluation. Cross-lane handoff for policy-named branches remains fail-closed pending #962.
Validation:
git diff --checkpassedCloses #865