fix(ci): make grandfathered attribution commits rebase-safe - #1356
Closed
allyblockcast[bot] wants to merge 4 commits into
Closed
fix(ci): make grandfathered attribution commits rebase-safe#1356allyblockcast[bot] wants to merge 4 commits into
allyblockcast[bot] wants to merge 4 commits into
Conversation
…858) `paperclipInboxLite`'s tool description and the OpenAPI summary both claimed the endpoint returned `in_review` work. The route filters to `todo,in_progress,blocked` (server/src/routes/agents.ts:2451). An agent whose only assignments were `in_review` therefore got a correct empty array from a call that had promised otherwise, read it as a platform failure, and hand-rolled a checkout-lock-blind `listIssues` sweep to "recover" — which is how two concurrent runs of the same agent duplicated a full task. The empty array was right. The description was the bug, and it is the whole root cause of the incident. - Describe the real status set in the MCP tool description and OpenAPI summary, and say what an empty array means per wake type: exit on an unscoped heartbeat; on an issue-named wake, read that issue by id and work it. - Ban the raw-sweep "recovery" explicitly in both skills — it has no checkout-lock awareness. - Replace SKILL.md Step 4's run-id comparison with checkout-first. Deciding ownership by reading `executionRunId`/`executionLockedAt` is TOCTOU-unsafe: the lock can be taken between the read and the first write. Those fields are the lock's storage, not its API. Checkout is the atomic boundary, and it is run-scoped, so a concurrent run of your own agent gets a 409 too. Also drops the instruction to comment on a lost race — that mutates an issue another run owns and can enter the approval-comment state machine. - Give `issue-triage` back a status-scoped `in_review` read, since the skill advertises triaging a status inbox-lite excludes. - Document the current `422 issue_in_review_not_checkoutable` gap on unlocked `in_review` rows rather than papering over it. Pins the contract with agent-inbox-lite-status-contract.test.ts, whose load-bearing assertion is that `in_review` never leaks into the routine inbox. Docs and tests only — zero runtime behavior change. Split out of #821 per the scope decision on BLO-18858; the `in_review`-checkoutability runtime change and its open review findings are tracked separately.
…ses (BLO-18858) Ally's Important finding on #1116 at b4634b1, verified against the service rather than taken on faith. issueService.checkout runs its atomic UPDATE first, matching on inArray(issues.status, expectedStatuses), and returns early when a row matches (server/src/services/issues.ts:9187-9235). The typed 422 issue_in_review_not_checkoutable is only reachable when that UPDATE matches nothing (:9417-9433). So documenting in_review made the 422 unreachable and silently flipped an unlocked review/approval wait to in_progress - the opposite of what the surrounding paragraph promised. The paragraph's justification for including it was also wrong: it claimed in_review in expectedStatuses is "what makes a locked in_review issue return 409". It is not. The 422 branch is guarded on checkoutRunId == null && executionRunId == null, so a locked row falls through to the generic conflict either way. Omitting in_review therefore preserves both behaviours and makes the documented 422 reachable. The service comment at :9411 already states in_review "is excluded from every caller's expectedStatuses" - the docs had drifted from that intent. Adds checkout-expected-statuses-doc-contract.test.ts to pin the documentation side (the behavioural side is already pinned by the existing typed-422 service test, which uses exactly this payload). Mutation-checked: reintroducing in_review fails exactly the new assertion and nothing else. Co-Authored-By: Claude <noreply@anthropic.com>
# Conflicts: # packages/skills-catalog/generated/catalog.json
Co-Authored-By: Paperclip <noreply@paperclip.ing>
Author
|
🔗 Paperclip issue: BLO-27142 |
1 similar comment
Author
|
🔗 Paperclip issue: BLO-27142 |
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
Author
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 2b5b6ef
Critical Issues (0)
Important Issues (1)
- [native-codex]
scripts/check-commit-author-attribution.mjs:177—auditRepoCommitAttributiondoes not compute apatchIdfor API-fetched commits before callingfindAttributionOffenses. Becauseallowlistis therefore omitted,findAttributionOffensestakes itsallowlist === undefinedpath and flags every non-merge commit authored by the App, including grandfathered commits that should match the patch-id/email allowlist. The new rebase-safe grandfathering consequently works only for local-range checks;--audit-mergedwill report those same grandfathered commits as offenses. Map each API commit through the same patch-id computation or pass a patch-id-aware allowlist before evaluating offenses, and add a regression test for an allowlisted API commit.
Suggestions (0)
Strengths
- The local PR-range path computes stable patch IDs and preserves the exact App author-email check.
- The inbox-lite contract changes are clearly documented and covered by focused route and documentation tests.
- The workflow wires both the attribution check and its regression tests into the policy job.
Recommended Action
- Fix the Important issue before merge.
- Re-run the focused attribution tests and merged-PR audit coverage.
Author
|
Superseded by #1357, which is based directly on current master and contains only the attribution-gate change. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Thinking Path
Linked Issues or Issue Description
Refs: BLO-27142
What Changed
Verification
node --check scripts/check-commit-author-attribution.mjsnode --test scripts/check-commit-author-attribution.test.mjsgh api repos/Blockcast/paperclip/rulesets/20487141 --jq '{name,enforcement,conditions,rules}'returned activemerge_method: REBASE.git diff --checkRisks
Model Used
OpenAI GPT-5.6 Terra (
openai/gpt-5.6-terra), tool-using coding agent with repository inspection, GitHub API access, and local test execution.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue templatePaperclip issue: https://paperclip.blockcast.net/BLO/issues/BLO-27142