fix(#600): enforce allow_targets in post-retro.sh - #601
fix(#600): enforce allow_targets in post-retro.sh#601fullsend-ai-coder[bot] wants to merge 3 commits into
Conversation
post-retro.sh created cross-repo issues without checking create_issues.allow_targets from config.yaml. Add the same is_target_allowed gate that post-triage.sh uses: read the allowlist from config, skip disallowed targets with a warning, and surface skipped proposals in the summary comment so they can be filed manually. Changes: - post-retro.sh: add config reading, is_target_allowed function, and allowlist check in the proposal filing loop (matching post-triage.sh) - post-retro-test.sh: add GITHUB_WORKSPACE/config.yaml setup, fixtures for disallowed/mixed/originating-repo targets, and 8 new test cases - docs/retro.md: document that create_issues.allow_targets governs retro-filed issues Note: pre-commit could not run (network error fetching hook repos in sandbox). The post-script runs an authoritative pre-commit check on the runner. Closes #600
|
🤖 Finished Review · ✅ Success · Started 2:23 PM UTC · Completed 2:40 PM UTC |
ReviewFindingsMedium
Low
Labels: PR modifies retro agent post-script behavior |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
|
|
||
| echo "Originating: ${ORIGINATING_REPO}#${ORIGINATING_NUMBER}" | ||
|
|
||
| # Read the allowlist from config.yaml. The config repo is checked out |
There was a problem hiding this comment.
[low] coherence
The is_target_allowed function and config-reading logic (~45 lines) are duplicated from scripts/post-triage.sh (lines 150-189). The project has shared libraries in scripts/lib/. Extracting the common logic is out of scope per AGENTS.md sections 2-3, but is worth tracking as follow-up.
fullsend-ai-review
Bot
added
requires-manual-review
|
/fs-fix |
|
🤖 Finished Fix · ✅ Success · Started 2:45 PM UTC · Completed 2:50 PM UTC |
🔧 Fix agent — iteration 1 (human-triggered)Addressed 1 of 3 review findings with a code fix (added allowlist documentation to docs/triage.md). Disagreed with 2 findings: one is informational (protected-path requires human approval), one was explicitly marked out of scope by the reviewer (code duplication). Fixed (1):
Disagreed (2):
Tests: passedNext steps:
|
ralphbean
left a comment
ralphbean
left a comment
There was a problem hiding this comment.
I think this needs a couple of test tweaks before it merges. See inline comments.
| # Allowed target: proposal IS filed (test-org is in the allowlist). | ||
| run_test "allow-targets-allowed-filed" \ | ||
| "${FIXTURE_ONE_PROPOSAL}" \ | ||
| "gh issue create" |
There was a problem hiding this comment.
[important] The config fixture sets up allow_targets.repos: [allowed-org/allowed-repo], but none of the new tests target it, so the repo-level branch in is_target_allowed never actually runs. post-triage-test.sh has prerequisites-creates-allowed-issue covering the equivalent path — could we add something similar here?
| # Originating repo is always allowed even without explicit allowlist entry. | ||
| run_test "allow-targets-originating-repo-allowed" \ | ||
| "${FIXTURE_ORIGINATING_REPO_TARGET}" \ | ||
| "gh issue create" |
There was a problem hiding this comment.
[important] test-org is both the originating repo's org and on the allow list here, so this test passes either way — it can't tell us whether it's the originating-repo check or the org check doing the work. allow-targets-no-workspace-originating-allowed below isolates it correctly since there's no config at all in that case. Could this one target a repo in an org that isn't allow-listed, so it actually proves the originating-repo path?
|
/fs-fix address the review findings |
|
🤖 Finished Fix · ✅ Success · Started 5:42 PM UTC · Completed 5:48 PM UTC |
…test Add allow-targets-repo-level-filed test to exercise the repos branch in is_target_allowed (config sets allowed-org/allowed-repo but no test targeted it). Update FIXTURE_ORIGINATING_REPO_TARGET to use an org not in the allowlist (unlisted-org) so allow-targets-originating-repo-allowed isolates the originating-repo check from the org-allowlist check. Addresses review feedback on #601
🔧 Fix agent — iteration 2 (human-triggered)Addressed both review findings. Added a repo-level allowlist test (allow-targets-repo-level-filed) to exercise the repos branch in is_target_allowed. Updated the originating-repo test fixture to use an org not in the allowlist so it isolates the originating-repo check from the org-allowlist check. Fixed (2):
Tests: passedNext steps:
|
2 participants
Summary
create_issues.allow_targetsenforcement topost-retro.sh— the same allowlist check thatpost-triage.shapplies to prerequisite issues now gates retro-filed proposals. Proposals targeting repos outside the allowlist are skipped with a::warning::and surfaced in the summary comment so they can be filed manually.docs/retro.mdunder a new "Issue filing allowlist" subsection.Approach
Inlined the config-reading logic and
is_target_allowedfunction directly inpost-retro.sh, matching the implementation inpost-triage.sh(lines 152–189). The originating repo plays the same role as the source repo in triage — always implicitly allowed.Skipped proposals are tracked in
SKIPPED_TARGETSand appended to the summary comment under a "Proposals skipped (target repo not allowed)" heading, with guidance to file manually or update the config.Testing
Closes #600
Post-script verification
agent/600-retro-allow-targets)317f517da71b710285fff0833becb92105024bdc..HEAD)