Repository navigation
fix(#1850): treat rejection closing comments as do-not-re-propose - #1851
fullsend-ai-coder[bot] wants to merge 3 commits into
Conversation
The retro-analysis dedup rules keyed closed-issue handling on the forge close reason alone. A maintainer rejection that the forge recorded as COMPLETED fell into "completed without a real fix: keep the proposal", so a later retro re-filed the declined change. The not-planned rule also expired after 30 days. - SKILL.md: the dedup subagent now returns the closing comment and any merged closing/linked PR or commit for every closed same-topic candidate. A completed closure counts as a fix only with a merged PR or commit; without one, a closing comment that rejects the change is treated as not planned, quoted in summary, and flagged as mislabeled. Not-planned rejections no longer expire after 30 days; a re-proposal must state what changed since closure. - github/SKILL.md: add a GraphQL closed-candidate recipe returning the closer (PR/commit), linked PRs, and the closing comment. - gitlab/SKILL.md: add a closed-candidate recipe using the issue, closed_by MRs, and notes APIs; the closing note is the main rejection signal since GitLab records no close reason. - agents/retro.md: the dedup subagent example asks for the closing comment and drops "recently-closed". - .skillsaw-baseline.json: raise the retro-analysis context-budget ceiling from 3797 to 3993 tokens for the added rules. Note: pre-commit could not fetch remote hook repos (TLS failure in the sandbox). Applicable hooks were checked directly: whitespace, EOF, line endings, private keys, lint-release-cases, and gitleaks via scan-secrets. skill-env-reachability-test.sh exits 127 in the sandbox on unmodified main too, so it was not verified. Closes #1850 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
🤖 Finished Review · ✅ Success · Started 4:02 PM UTC · Completed 4:17 PM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5-5 · Effort: high · Cost: $1.62 |
|
<!-- fullsend:risk-assessment --> DetailsPreserved moderate risk score of 2 as minor recipe refinements across the same 5 files maintain unchanged Tier 1 signals, with protected agent and skill modifications lacking a rollback toggle balanced by clear issue alignment, bot author, and absence of CI or security changes. Previous run<!-- fullsend:risk-assessment --> DetailsPreserved moderate risk score of 2 following recipe expansion (132 lines across the same 5 files) as protected agent and skill prose modifications without a feature toggle remain balanced by clear issue alignment, bot author, and lack of security or CI changes despite moderate git churn. Previous run (2)<!-- fullsend:risk-assessment --> DetailsModerate risk: 5 files and 94 lines modifying protected agent and skill prose without a feature toggle, balanced by clear issue alignment, bot author, and low blast radius despite moderate git churn and change coupling. |
ReviewFindingsMedium
Low
Earlier findings
Labels: PR fixes retro-analysis dedup behavior Previous run<!-- fullsend:review-findings-v2:eyJ2ZXJzaW9uIjoyLCJhY3Rpb24iOiJyZXF1ZXN0LWNoYW5nZXMiLCJmaW5kaW5ncyI6W3sic2V2ZXJpdHkiOiJtZWRpdW0iLCJjYXRlZ29yeSI6ImxvZ2ljLWVycm9yIiwiZmlsZSI6InNraWxscy9yZXRyby1hbmFseXNpcy9naXRodWIvU0tJTEwubWQiLCJpZCI6ImZfYjRlOTRhOTQ4MzBkZWMzOCIsImxpbmUiOjE1OCwiYWN0aW9uYWJsZSI6dHJ1ZX0seyJzZXZlcml0eSI6Im1lZGl1bSIsImNhdGVnb3J5IjoiZWRnZS1jYXNlIiwiZmlsZSI6InNraWxscy9yZXRyby1hbmFseXNpcy9naXRsYWIvU0tJTEwubWQiLCJpZCI6ImZfMWY0MjczNzIwMWViOGIzMyIsImxpbmUiOjE1MywiYWN0aW9uYWJsZSI6dHJ1ZX0seyJzZXZlcml0eSI6ImxvdyIsImNhdGVnb3J5IjoicmJhYy12aW9sYXRpb24iLCJmaWxlIjoic2tpbGxzL3JldHJvLWFuYWx5c2lzL2dpdGh1Yi9TS0lMTC5tZCIsImlkIjoiZl8xOTQxYjYxZTFlYzBhNDgyIiwibGluZSI6MTUyLCJhY3Rpb25hYmxlIjp0cnVlfSx7InNldmVyaXR5IjoibWVkaXVtIiwiY2F0ZWdvcnkiOiJlZGdlLWNhc2UiLCJmaWxlIjoic2tpbGxzL3JldHJvLWFuYWx5c2lzL2dpdGxhYi9TS0lMTC5tZCIsImlkIjoiZl81N2FiYzdkYjRmYWRjMTY4IiwibGluZSI6MTMzLCJhY3Rpb25hYmxlIjp0cnVlfSx7InNldmVyaXR5IjoibWVkaXVtIiwiY2F0ZWdvcnkiOiJsb2dpYy1lcnJvciIsImZpbGUiOiJza2lsbHMvcmV0cm8tYW5hbHlzaXMvZ2l0bGFiL1NLSUxMLm1kIiwiaWQiOiJmXzU5NDA3OTk0MmFiY2IyYjYiLCJsaW5lIjoxMzcsImFjdGlvbmFibGUiOnRydWV9LHsic2V2ZXJpdHkiOiJsb3ciLCJjYXRlZ29yeSI6InJiYWMtdmlvbGF0aW9uIiwiZmlsZSI6InNraWxscy9yZXRyby1hbmFseXNpcy9naXRsYWIvU0tJTEwubWQiLCJpZCI6ImZfMjA1ZjllYmIxMzUxODg1YiIsImxpbmUiOjEzNywiYWN0aW9uYWJsZSI6dHJ1ZX0seyJzZXZlcml0eSI6ImxvdyIsImNhdGVnb3J5IjoiZWRnZS1jYXNlIiwiZmlsZSI6InNraWxscy9yZXRyby1hbmFseXNpcy9naXRodWIvU0tJTEwubWQiLCJpZCI6ImZfODJhYWE0OTM5OTZlNDAzZCIsImxpbmUiOjEzNiwiYWN0aW9uYWJsZSI6dHJ1ZX1dLCJkaXNwb3NpdGlvbnMiOlt7ImlkIjoiZl81N2FiYzdkYjRmYWRjMTY4Iiwic3RhdHVzIjoicmVzb2x2ZWRfYnlfY2hhbmdlIn0seyJpZCI6ImZfNTk0MDc5OTQyYWJjYjJiNiIsInN0YXR1cyI6InJlc29sdmVkX2J5X2NoYW5nZSJ9LHsiaWQiOiJmXzE5NDFiNjFlMWVjMGE0ODIiLCJzdGF0dXMiOiJvcGVuIn0seyJpZCI6ImZfMjA1ZjllYmIxMzUxODg1YiIsInN0YXR1cyI6InJlc29sdmVkX2J5X2NoYW5nZSJ9LHsiaWQiOiJmXzgyYWFhNDkzOTk2ZTQwM2QiLCJzdGF0dXMiOiJyZXNvbHZlZF9ieV9jaGFuZ2UifV19 --> ReviewFindingsMedium
Low
Earlier findings
Next steps:
Previous run (2)<!-- fullsend:review-findings-v2:eyJ2ZXJzaW9uIjoyLCJhY3Rpb24iOiJyZXF1ZXN0LWNoYW5nZXMiLCJmaW5kaW5ncyI6W3sic2V2ZXJpdHkiOiJtZWRpdW0iLCJjYXRlZ29yeSI6ImVkZ2UtY2FzZSIsImZpbGUiOiJza2lsbHMvcmV0cm8tYW5hbHlzaXMvZ2l0bGFiL1NLSUxMLm1kIiwiaWQiOiJmXzU3YWJjN2RiNGZhZGMxNjgiLCJsaW5lIjoxMzMsImFjdGlvbmFibGUiOnRydWV9LHsic2V2ZXJpdHkiOiJtZWRpdW0iLCJjYXRlZ29yeSI6ImxvZ2ljLWVycm9yIiwiZmlsZSI6InNraWxscy9yZXRyby1hbmFseXNpcy9naXRsYWIvU0tJTEwubWQiLCJpZCI6ImZfNTk0MDc5OTQyYWJjYjJiNiIsImxpbmUiOjEzNywiYWN0aW9uYWJsZSI6dHJ1ZX0seyJzZXZlcml0eSI6ImxvdyIsImNhdGVnb3J5IjoicmJhYy12aW9sYXRpb24iLCJmaWxlIjoic2tpbGxzL3JldHJvLWFuYWx5c2lzL2dpdGh1Yi9TS0lMTC5tZCIsImlkIjoiZl8xOTQxYjYxZTFlYzBhNDgyIiwibGluZSI6MTQ5LCJhY3Rpb25hYmxlIjp0cnVlfSx7InNldmVyaXR5IjoibG93IiwiY2F0ZWdvcnkiOiJyYmFjLXZpb2xhdGlvbiIsImZpbGUiOiJza2lsbHMvcmV0cm8tYW5hbHlzaXMvZ2l0bGFiL1NLSUxMLm1kIiwiaWQiOiJmXzIwNWY5ZWJiMTM1MTg4NWIiLCJsaW5lIjoxMzcsImFjdGlvbmFibGUiOnRydWV9LHsic2V2ZXJpdHkiOiJsb3ciLCJjYXRlZ29yeSI6ImVkZ2UtY2FzZSIsImZpbGUiOiJza2lsbHMvcmV0cm8tYW5hbHlzaXMvZ2l0aHViL1NLSUxMLm1kIiwiaWQiOiJmXzgyYWFhNDkzOTk2ZTQwM2QiLCJsaW5lIjoxMzYsImFjdGlvbmFibGUiOnRydWV9XX0= --> ReviewFindingsMedium
Low
Next steps:
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
🤖 Finished Fix · ✅ Success · Started 4:18 PM UTC · Completed 4:23 PM UTC Commit: Runtime: claude · Model: sonnet → claude-sonnet-5-5 · Effort: high · Cost: $0.62 |
- github recipe: page all issue comments forward, return the author's association with a maintainer flag, and avoid uppercase tokens that the skill env-reachability test flags (fixes script-test) - gitlab recipe: page through all notes, select the last note by the closer or a project member (access level 30+), and report the author's access level and a maintainer flag - retro-analysis: treat only a maintainer's closing comment as a rejection Pre-commit could not run (hook repos unreachable); checked whitespace, line endings, secrets, and lint-agent-docs directly. Addresses #1851 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
🔧 Fix agent — iteration 1 (bot-triggered)Addressed all five actionable findings (GitLab pagination, selection logic, and authority check; GitHub association flag and comment paging) and recorded the protected-path notice as a no-op. Also fixed the pr-related script-test failure, which came from uppercase enum tokens in the new GitHub recipe. Pre-commit could not run because hook repos are unreachable; whitespace, line endings, secrets, and lint-agent-docs were checked directly. make script-test and skillsaw could not run locally (missing yq and a restricted shell), so the token check was emulated and the retro-analysis token budget was kept at or below the prior size. Fixed (5):
Disagreed (1):
Tests: passed CI inspections (6):
Conflict update: skipped; strategy Decision points
Next steps:
|
|
🤖 Finished Review · ✅ Success · Started 4:25 PM UTC · Completed 4:34 PM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5-5 · Effort: high · Cost: $1.59 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
🤖 Finished Fix · ✅ Success · Started 4:36 PM UTC · Completed 4:40 PM UTC Commit: Runtime: claude · Model: sonnet → claude-sonnet-5-5 · Effort: high · Cost: $0.51 |
Return the last maintainer comment independently of the closer's comment in both closed-candidate recipes (closingComment/closer_comment), so a closer's later comment cannot mask an earlier maintainer rejection. GitHub: verify write/maintain/admin repository permission per commenter instead of trusting authorAssociation. GitLab: default the access level to 0 on failed, empty or non-numeric member lookups so the recipe still returns closure evidence. Note: pre-commit could not fetch hook repositories (certificate failure); skillsaw lint unavailable (uvx missing). Whitespace checked manually and the parent skill file stays within its token baseline by size. Addresses #1851 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
🔧 Fix agent — iteration 2 (bot-triggered)Addressed all actionable findings: maintainer closing comment is now selected independently of the closer's comment in both recipes, GitHub maintainer status is verified through repository permission, and the GitLab access-level lookup falls back to 0 on failure. jq logic was exercised on mock data. The pre-commit hooks could not fetch their repositories and skillsaw (uvx) is unavailable in the sandbox; whitespace was checked manually. The protected-path finding needs human approval. Fixed (3):
Disagreed (1):
Tests: passed CI inspections (7):
Conflict update: skipped; strategy Decision points
Next steps:
|
|
🤖 Finished Review · ✅ Success · Started 4:42 PM UTC · Completed 4:51 PM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5-5 · Effort: high · Cost: $1.71 |
Superseded by updated review
| | select($at != null and .createdAt <= $at and .author != null) | ||
| | select((.authorAssociation // "" | ascii_downcase) | IN("owner", "member", "collaborator")) | ||
| | .author.login] | unique | .[]' "${cand_file}"); do | ||
| role=$(gh api "repos/<owner>/<repo>/collaborators/${login}/permission" --jq '.role_name' 2>/dev/null || true) |
There was a problem hiding this comment.
[low] api-contract
The permission lookup stores .role_name, but the maintainer predicate accepts only admin, maintain and write. For a custom repository role GitHub reports the custom name in role_name, so a collaborator whose role inherits write access is never selected as closingComment. Fail-closed; only affects repositories with custom roles.
Suggested fix: Read .permission (maintain maps to write there) and use IN("admin", "write"), or accept either field.
| - *completed without a real fix*: keep the proposal, reference the old issue. | ||
| - *not planned* (any age): a maintainer rejected it — skip. Elapsed time is not new evidence: re-propose only if circumstances changed since closure, and state in `what_could_go_better` what changed and how it answers the comment. | ||
| - *completed with a merged fix* (a merged PR or commit closed or is linked to it): if the symptom **recurred after the merge**, keep the proposal as a regression referencing the old issue and fix PR; otherwise skip. | ||
| - *completed without a real fix, maintainer's closing comment rejects the change* (disagrees, won't fix, declining; per the recipe's closing comment — a closer's comment is context only): apply the *not planned* rule. Quote the comment in `summary` and flag the mislabeled close reason for a human. |
There was a problem hiding this comment.
[low] prompt-injection
The new rule says to quote the closing comment in summary and the subagent returns it verbatim, which conflicts with agents/retro.md:130 (do not echo untrusted comment text verbatim). It also does not say that instructions embedded in the comment must not be followed.
Suggested fix: Paraphrase the rejection rationale with attribution (or quote only the rejection sentence) and state that comment bodies are classification evidence only.
fullsend-ai-review
Bot
added
requires-manual-review
Summary
Changes
skills/retro-analysis/SKILL.mdand its GitHub and GitLab recipes so a maintainer rejection is never re-proposed just because the forge recorded the close reason as COMPLETED, or because 30 days went by.Root cause: the "Before proposing" rules decided what to do with a closed issue from the forge close reason alone. A rejection recorded as COMPLETED fell into "completed without a real fix: keep the proposal". Also, the dedup searches never fetched closing comments or closing PRs, so the agent had nothing to tell the two cases apart.
Changes
skills/retro-analysis/SKILL.mdsummary, and flags the wrong close reason so a human can correct it.what_could_go_betterwhat changed since the closure and how that answers the closing comment. Item 3 of the issue left this open. I dropped the flat window instead of lengthening it because the issue says time passing alone should not be a reason to revisit. Edge cases that are unchanged: completed with a merged fix (skip, or keep as a regression if the symptom recurred), and completed with no fix and no rejection (keep).skills/retro-analysis/github/SKILL.md: adds a GraphQL closed-candidate recipe. It returnsstateReason, theClosedEventcloser (PR withmerged, or a commit),closedByPullRequestsReferences, and the closing comment, meaning the last comment at or beforeclosedAtby whoever closed the issue or by an OWNER/MEMBER/COLLABORATOR. The comment that tiedclosed_atto the 30-day rule is updated.skills/retro-analysis/gitlab/SKILL.md: adds a closed-candidate recipe that calls the issue API (closed_at,closed_by),issues/:iid/closed_by(merge requests that close the issue on merge), and the notes API (the closing system note plus the last non-system note before closure). GitLab records no close reason, so the recipe says the closing note is the main rejection signal.agents/retro.md: the dedup subagent example now asks for the closing comment and closing PR/commit, and says "closed" instead of "recently-closed"..skillsaw-baseline.json: raises the context-budget ceiling forretro-analysis/SKILL.mdfrom 3797 to 3993 tokens, the same way Retro agent should search for related open issues before proposing fixes #1730 did. I shortened the new wording first to keep the increase small.Testing
skillsaw@0.18.0 --strict: 0 errors, 0 warnings once the ceiling was raised.hack/lint-agent-docs,eval/lint-release-cases.pyandscan-secretspass.Caveats
pre-commitcould not fetch remote hook repos because of a TLS failure in the sandbox. I ran the hooks that apply to Markdown/JSON by hand instead (whitespace, EOF, line endings, private keys, gitleaks viascan-secrets).scripts/skill-env-reachability-test.shexits 127 in the sandbox on unmodifiedmainas well (a tool is missing), so it was not checked.🤖 Generated with Claude Code
Closes #1850
Post-script verification
agent/1850-retro-rejection-closures)8ee5959c80c22e9fbd2b25f8c0a91837394f5f26..HEAD)