Skip to content

fix(#6858): keep code target branches optional - #1248

Open
shairevivo wants to merge 2 commits into
fullsend-ai:mainfrom
shairevivo:codex/6858-optional-target-branches
Open

fix(#6858): keep code target branches optional#1248
shairevivo wants to merge 2 commits into
fullsend-ai:mainfrom
shairevivo:codex/6858-optional-target-branches

Conversation

@shairevivo

@shairevivo shairevivo commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Summary

  • stop requiring the optional CODE_ALLOWED_TARGET_BRANCHES host variable in the code harness
  • keep an unset value safely constrained to the forge-detected default branch
  • enforce explicit allowlists even when the agent does not provide a target branch

Verification

  • make check-bundle
  • make lint
  • bash scripts/post-code-test.sh (with GNU sed on macOS)
  • independent review: no remaining actionable findings

Related to fullsend-ai/fullsend#6858

Remove the runner interpolation so an unset optional allowlist reaches the safe default-branch fallback. Enforce an explicit allowlist even when the agent provides no branch preference.

Co-authored-by: Codex <noreply@openai.com>
Signed-off-by: Shai Revivo <srevivo@redhat.com>
@shairevivo
shairevivo requested a review from a team as a code owner September 10, 2026 13:20
@github-actions

Copy link
Copy Markdown

Functional tests did not run

Functional tests run automatically for org/repo members and collaborators on pull requests.

For other contributors, a maintainer must add the ok-to-test label after the latest push.

@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Keep Code Target Branch Allowlists Optional and Enforced

🐞 Bug fix 🧪 Tests 📝 Documentation ⚙️ Configuration changes 🕐 20-40 Minutes

Grey Divider

AI Description

• Keeps optional target-branch allowlists out of required harness runner interpolation.
• Validates forge-detected defaults when agents omit branch preferences.
• Adds regression coverage for restricted, wildcard, and unset configurations.
Diagram

graph TD
  H["Code harness"] --> A["Agent result"] --> P["Post-code script"] --> D{"Branch allowed?"} -->|Yes| R["Create PR"]
  E["Host allowlist"] --> P
  F["Forge API"] --> P
  D -->|No| X["Validation failure"]
Loading
High-Level Assessment

The current approach is preferred. Removing runner interpolation restores the variable's intended optional semantics, while validating the forge-detected default closes the allowlist bypass. Supplying a harness-side default was considered but rejected because host interpolation makes the variable required and cannot safely express shell-style optional defaults.

Files changed (6) +112 / -7

Bug fix (2) +12 / -0
post-code.shEnforce allowlists on implicit default branches +6/-0

Enforce allowlists on implicit default branches

• Updates the bundled post-code script to validate the forge-detected default against an explicit allowlist when the agent omits its target branch. Disallowed defaults fail before push or PR creation.

scripts/post-code.sh

post-code.src.shValidate fallback branches against explicit policy +6/-0

Validate fallback branches against explicit policy

• Adds source-level branch validation for the no-agent-target path. Empty allowlists preserve the safe default fallback, wildcards permit the default, and explicit mismatches fail validation.

scripts/post-code.src.sh

Tests (2) +96 / -4
harness-jira-test.shGuard optional allowlist harness configuration +9/-0

Guard optional allowlist harness configuration

• Adds a regression assertion ensuring the code harness does not reintroduce CODE_ALLOWED_TARGET_BRANCHES under env.runner.

scripts/harness-jira-test.sh

post-code-test.shCover default-branch allowlist enforcement +87/-4

Cover default-branch allowlist enforcement

• Updates the branch-validation decision test to reject a disallowed default when no agent target is provided. Adds end-to-end security tests covering rejection, explicit default allowance, and wildcard allowance.

scripts/post-code-test.sh

Documentation (1) +4 / -2
AGENTS.mdDocument the optional allowlist exception +4/-2

Document the optional allowlist exception

• Clarifies that CODE_ALLOWED_TARGET_BRANCHES must not be mapped through the code harness runner environment because it is optional and consumed directly by the post-code script. Distinguishes it from required computed runner values.

AGENTS.md

Other (1) +0 / -1
code.yamlStop requiring the optional branch allowlist +0/-1

Stop requiring the optional branch allowlist

• Removes CODE_ALLOWED_TARGET_BRANCHES from env.runner so runs are not rejected when the host leaves the optional variable unset. The post-code script can now apply its safe default-branch behavior.

harness/code.yaml

@qodo-code-review

qodo-code-review Bot commented Sep 10, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (1)

Grey Divider


Informational

1. Protected changes require human review 📜 Skill insight § Compliance
Description
harness/code.yaml, scripts/post-code*.sh, scripts/*-test.sh, and AGENTS.md are protected
governance or infrastructure paths changed by this PR. The linked issue and PR explanation justify
the branch-allowlist fix, but these protected edits still require a human approval gate before
merge.
Code

scripts/post-code.src.sh[R324-327]

+  if [ -n "${CODE_ALLOWED_TARGET_BRANCHES:-}" ] \
+     && [ "${CODE_ALLOWED_TARGET_BRANCHES}" != "*" ] \
+     && ! echo ",${CODE_ALLOWED_TARGET_BRANCHES}," | grep -qF ",${DEFAULT_BRANCH},"; then
+    post_fail_to_issue branch-validation \
Relevance

● Weak

Recent precedent rejected an equivalent protected-scripts human-review enforcement finding; this
change’s rationale and tests support merging.

PR-#1196

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Compliance rule 1538392 designates AGENTS.md, harness/, and scripts/ as protected paths that
always require a finding and human approval. The cited additions modify the branch-validation guard
and its governance guidance within those protected paths.

scripts/post-code.src.sh[324-329]
AGENTS.md[160-163]
scripts/harness-jira-test.sh[101-108]
Skill: pr-review

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
This PR modifies protected governance and infrastructure paths, so it must not be approved or merged solely through automated review.

## Fix Focus Areas
- scripts/post-code.src.sh[324-329]
- harness/code.yaml[66-71]
- AGENTS.md[160-163]

## Recommended Fix
Disable any automatic approval for this PR and require an authorized human or code owner to review and approve the protected-path changes before merge.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Context sources
✅ Compliance rules (platform): 57 rules
✅ Skills: 4 invoked
  code-review
  code-implementation
  pr-review
  docs-review
✅ Cross-repo context — repo relationships
  Explored: repo: fullsend-ai/fullsend (sha: a4b5c485)
Review mode: ⚖️ Balanced: This changes shell harness and branch-validation behavior across multiple scripts, with security-sensitive allowlist enforcement and fallback semantics that warrant a careful full review.

Grey Divider

Tip of the day
💡 Did you know, you can turn these tips off under Display preferences

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

@waynesun09 waynesun09 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Review notes

Four findings from a read of head fe8899c, verified against the head tree, the CI logs and the linked issue. No blocking action taken -- this is comment-only.

Three findings are inline. One has no anchor in this PR's diff, so it is here:


MEDIUM -- docs/code.md still documents the old, agent-target-only validation semantics (docs/code.md, around the CODE_ALLOWED_TARGET_BRANCHES table row)

The row still reads: "The post-code script reads it from the runner when present and validates the agent's chosen target branch before pushing."

After this PR the auto-detected repository default is also checked when the agent expresses no preference, and a list that omits that default now aborts the run -- previously such a run fell through to DEFAULT_BRANCH unchecked. Operators reading only this row will not know that previously-silent default-branch runs now fail closed.

docs/code.md is not in this PR's changed-file set, but it is in scope: this PR is what makes the row wrong, and issue #6858's last acceptance criterion is that "the base harness and documentation agree about the default behavior."

Suggestion: extend the row with one sentence, in the style already used for CODE_AUTO_MERGE:

When the agent expresses no branch preference, the auto-detected repository default is used and must itself appear in the list (or the list must be *), otherwise the run fails.

Comment thread scripts/post-code-test.sh Outdated
Comment thread scripts/post-code.src.sh Outdated
Comment thread scripts/post-code-test.sh Outdated
Remove the out-of-scope explicit allowlist rejection added with the optional-variable fix. An unset agent branch continues to use the repository default branch.

Co-authored-by: Codex <noreply@openai.com>
Signed-off-by: Shai Revivo <srevivo@redhat.com>
@shairevivo

Copy link
Copy Markdown
Contributor Author

Review follow-up for fb69da9: the documentation note about explicit lists is no longer applicable because this PR no longer changes explicit-list/default-branch behavior. The retained AGENTS.md change documents only the optional-variable contract. The protected-path changes remain subject to human review and approval; no automated approval is requested.

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.

2 participants