Add blocked-by dependencies to create-issue - #53733
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #53733 does not have the implementation label and has only 8 new lines of code in business logic directories (threshold: 100).
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Ponytail Reviewer completed successfully!
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes
create_issue now has a partial-success failure mode: if attaching blocked-by dependencies fails after issue creation, the handler still reports the operation as failed.
Blocking theme
That breaks safe-output idempotency and can lead to duplicate issues or incorrect fallback/retry behavior even though the issue was already created.
The background grumpy-coder sub-agent did not return usable findings in this harness, so this review is based on independent analysis of the changed lines.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 13.2 AIC · ⌖ 8.77 AIC · ⊞ 7K
Comment /review to run again
| } catch (error) { | ||
| const dependencyError = getErrorMessage(error); | ||
| core.error(`Failed to add blocked-by dependency ${blockedIssue.repo}#${blockedIssue.number} to ${qualifiedItemRepo}#${issue.number}: ${dependencyError}`); | ||
| return { |
There was a problem hiding this comment.
This leaves the system in an inconsistent state: the issue is already created, but a transient failure on the dependency API makes the handler return success: false and report the whole create_issue operation as failed.
💡 Why this blocks merge
At this point rest.issues.create() has already succeeded, so callers will treat this as a failed safe output and may retry or emit fallback handling even though the issue now exists. That is exactly the kind of partial-success bug that creates duplicates and breaks idempotency.
Please either:
// option 1: make dependency attachment best-effort
core.warning(`Issue created, but failed to attach blocked_by dependency: ${dependencyError}`);or return a structured partial-success result that the manager treats as created/not-retriable.
The tests added here only cover the happy path, so this regression would slip through unchanged.
There was a problem hiding this comment.
Dependency attachment is now best-effort in 7f8d11a: a failure logs a warning, the issue stays success: true, and the failures are reported on the result as blocked_by_errors. Added a test covering the dependency API failure path.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs, /tdd, and /codebase-design — requesting changes on one correctness bug and several test-coverage gaps.
📋 Key Themes & Highlights
Key Issues
-
Correctness (high impact): When a
POST /dependencies/blocked_byAPI call fails after issue creation, the function returnssuccess: falsewithout storing thetemporaryId → {repo, number}mapping. This silently orphans the created issue from any downstream message that references its temporary ID. The temporary-ID mapping must be stored before the dependency loop. -
Test coverage gaps: The dependency-API error path, the invalid-blocker-ID guard, and the cycle-detection branch in
sortMessagesByTemporaryIdDependenciesall lack tests. These are behavioural contracts that should be pinned as specifications. -
Design nit:
sortMessagesByTemporaryIdDependencieshard-codesmessage.type === "create_issue"instead of delegating toextractTemporaryIdReferences, which already handles all supported fields. This will silently break if another message type gainsblocked_by. -
Regex over-permissiveness: The cross-repo and URL regexes allow leading
.or-in owner/repo slugs, which are not valid on GitHub. A tighter pattern prevents silent bad-API-target bugs.
Positive Highlights
- ✅ Topological sort with cycle-safe fallback is a clean, robust approach for ordering dependent messages.
- ✅ Deduplication via
seenset inresolveBlockedByReferencesprevents duplicate dependency calls. - ✅ Tests cover the happy paths (same-repo, cross-repo, and temporary-ID resolution) well.
- ✅
extractTemporaryIdReferencescorrectly extended forblocked_byso the existing unresolved-ID checks continue to work.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 52.4 AIC · ⌖ 10.7 AIC · ⊞ 7.8K
Comment /matt to run again
Comments that could not be inline-anchored
actions/setup/js/create_issue.cjs:1180
[/diagnosing-bugs] Partial-state bug: when the POST /dependencies/blocked_by API call fails, the function returns success: false — but the issue was already created and the temporaryId → {repo, number} mapping is never stored (that temporaryIdMap.set(...) call comes after the dep loop). Any downstream create_issue referencing this issue via blocked_by or parent will defer forever, silently losing the created issue number.
<details>
<summary>💡 Suggested fix</summary>
Store t…
actions/setup/js/create_issue.cjs:1196
[/tdd] The test asserts result.success === true but never verifies the dependency is skipped when mockGithub.request fails. The error path (return { success: false, error: ... }) in the dep loop has no test coverage — a regression here would be silent.
<details>
<summary>💡 Suggested test to add</summary>
it("should return success:false when the blocked-by API call fails", async () => {
mockGithub.rest.issues.get = vi.fn().mockResolvedValue({ data: { id: 9001 } });
mockGit…
</details>
<details><summary>actions/setup/js/create_issue.cjs:1163</summary>
**[/tdd]** No test covers the `invalid blocker ID` guard (`!Number.isSafeInteger(blocker?.id) || blocker.id < 1`). If the API returns an unexpected shape (e.g., `{ id: 0 }` or `{ id: null }`), the thrown `ERR_VALIDATION` error hits the same `catch` block as a real API failure — but that invariant goes unverified by any test.
<details>
<summary>💡 Suggested test</summary>
```js
it("should return success:false when blocker API returns an invalid id", async () => {
mockGithub.rest.issues.get =…
</details>
<details><summary>actions/setup/js/safe_output_handler_manager.cjs:755</summary>
**[/codebase-design]** `sortMessagesByTemporaryIdDependencies` only wires `create_issue → create_issue` blocked-by edges, but `extractTemporaryIdReferences` already handles `blocked_by` for any message type. If another message type gains a `blocked_by` field in the future, the sort will silently ignore it because of this hard-coded `message.type === "create_issue"` guard.
<details>
<summary>💡 Suggested change</summary>
Delegate to `extractTemporaryIdReferences` for all message types instead …
</details>
<details><summary>actions/setup/js/safe_output_handler_manager.test.cjs:72</summary>
**[/tdd]** The topo-sort test only covers the happy path (producer before consumer). There's no test for the cycle-detection branch that emits `core.warning(...)`. A cycle is a real edge case that should be regression-tested.
<details>
<summary>💡 Suggested test</summary>
```js
it("preserves original order when a blocked_by dependency cycle is detected", () => {
const a = { type: "create_issue", temporary_id: "aw_a", blocked_by: "aw_b", title: "A" };
const b = { type: "create_issue", temp…
</details>
<details><summary>actions/setup/js/create_issue.cjs:402</summary>
**[/diagnosing-bugs]** `resolveBlockedByReference` uses `[\w.-]+` for the `owner` and `repo` segments in the cross-repo regex and URL regex. `\w` in JS matches `[A-Za-z0-9_]`, so repo names containing hyphens (e.g. `my-repo`) are covered by `.-`. However, `\w.-` also permits a leading `.` (e.g., `.hidden/repo`) or a leading `-`, which are not valid GitHub owner/repo slugs. A malformed reference would silently resolve to an invalid API target rather than returning an error.
<details>
<summary>?…
</details>There was a problem hiding this comment.
Pull request overview
Adds native “blocked by” dependencies to create_issue, including temporary and cross-repository references.
Changes:
- Adds and documents the
blocked_byinput. - Resolves references and creates GitHub dependencies.
- Orders temporary-ID producers before dependents and adds tests.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/js/safe_outputs_tools.json |
Adds compiler schema support. |
actions/setup/js/safe_outputs_tools.json |
Adds runtime schema support. |
actions/setup/js/temporary_id.cjs |
Extracts dependency temporary IDs. |
actions/setup/js/temporary_id.test.cjs |
Tests temporary-ID extraction. |
actions/setup/js/safe_output_handler_manager.cjs |
Orders dependency messages. |
actions/setup/js/safe_output_handler_manager.test.cjs |
Tests dependency ordering. |
actions/setup/js/create_issue.cjs |
Resolves and creates dependencies. |
actions/setup/js/create_issue.test.cjs |
Tests dependency handling. |
.github/aw/safe-outputs-content.md |
Documents supported references. |
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 3
- Review effort level: Balanced
| * @returns {Promise<{success: boolean, results: Array<any>, temporaryIdMap: Object, artifactUrlMap: Map<string, string>, outputsWithUnresolvedIds: Array<any>, missings: Object, codePushFailures: Array<{type: string, error: string}>}>} | ||
| */ | ||
| async function processMessages(messageHandlers, messages, onItemCreated = null) { | ||
| messages = sortMessagesByTemporaryIdDependencies(messages); |
There was a problem hiding this comment.
Fixed in 7f8d11a. The topological sort now returns original message indices (sortMessageIndicesByTemporaryIdDependencies); processMessages iterates in dependency order but records the original index as messageIndex, so summaries still line up with allMessages.
| for (const dependentIndex of dependents[index]) { | ||
| inDegree[dependentIndex]--; | ||
| if (inDegree[dependentIndex] === 0) { | ||
| ready.push(dependentIndex); | ||
| } | ||
| } |
There was a problem hiding this comment.
Fixed in 7f8d11a. The ready queue is kept ordered by original message index, so [dependent, producer, unrelated] now yields [producer, dependent, unrelated]. Added a regression test.
| const blockedBy = resolveBlockedByReferences(message.blocked_by, temporaryIdMap, qualifiedItemRepo); | ||
| if (blockedBy.deferred) { | ||
| core.info(`Deferring create_issue: ${blockedBy.error}`); | ||
| return { success: false, deferred: true, error: blockedBy.error }; |
There was a problem hiding this comment.
Fixed in 7f8d11a. In staged mode blocked_by references are validated without deferring: unresolved temporary IDs are collected and surfaced in the staged preview (previewInfo.blockedBy) so dependent issues still get a preview. Added a test.
There was a problem hiding this comment.
Ponytail review — over-engineering pass only.
net: -20 lines possible.
Generated by ✂️ Ponytail Reviewer for #53733 · auto · 38.6 AIC · ⌖ 6.16 AIC · ⊞ 7.3K
Comment /ponytail to run again
Comments that could not be inline-anchored
actions/setup/js/create_issue.cjs:401
yagni: resolveBlockedByReference reimplements bare-number/cross-repo/URL parsing that already exists in close_issue.cjs (parseDuplicateOf). Extract one shared helper, 0 new duplicated regexes.
actions/setup/js/safe_output_handler_manager.cjs:316
yagni: full topological sort with in-degree/queue/cycle-detection built for a dependency graph that in practice is only 1 level deep (blocked_by -> temporary_id producer). A simple two-pass stable partition (producers referenced by any blocked_by first, rest after) covers the real cases in ~5 lines.
There was a problem hiding this comment.
Review: Add blocked-by dependencies to create-issue
The implementation is well-structured with clean parsing, deduplication, and topological sorting for ordering dependent messages. Two correctness issues worth addressing:
-
Partial-create +
success: false— if ablocked_byAPI call fails after the issue is already created, returningsuccess: falsemay cause the manager to retry and create a duplicate. Consider returningsuccess: truewith a warning, or apartialSuccessflag. -
Fail-fast on first dependency failure — the dependency loop exits on the first error, silently skipping remaining
blocked_byentries. Collecting all errors and applying as many dependencies as possible would be more robust.
The topological sort, reference parsing, temporary-ID resolution, and test coverage are all solid.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 38.9 AIC · ⌖ 9.03 AIC · ⊞ 5.7K
Comments that could not be inline-anchored
actions/setup/js/create_issue.cjs:1183
Correctness: success: false after issue already created may cause duplicate issues on retry
When the blocked_by API call fails at this line, the GitHub issue was already created (a few lines earlier). Returning { success: false } signals the manager that the entire operation failed, which could trigger a retry that creates a duplicate issue.
The error message correctly describes the partial state ("Issue ... was created, but ..."), but success: false may trigger a retry that r…
actions/setup/js/safe_output_handler_manager.cjs:273
Correctness: sortMessagesByTemporaryIdDependencies only handles create_issue blocked_by — misses other dependency types
The dependency extraction is guarded by message.type === "create_issue". This is correct for the current feature, but the extractTemporaryIdReferences function already handles other message types (add_comment, link_sub_issue, etc.). If future message types also use blocked_by, this guard will silently skip their dependency ordering.
Consider extracting all …
|
🎉 Thanks for this contribution! This PR introduces a valuable feature to What looks great:
This looks ready for review! The implementation is complete, tested, and documented. Reviewers can focus on the logic and any edge cases around dependency resolution and cross-repository references.
|
|
@copilot run pr-finisher skill |
…previews Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
PR Triage
Adds
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed. The failure was |
|
🎉 This pull request is included in a new release. Release: |
create_issuecan now create native GitHub “blocked by” dependencies from issue numbers, temporary IDs, and cross-repository references.Input support
blocked_byto thecreate_issuetool schema.owner/repo#number, or issue URL.Dependency resolution
blocked_bytemporary-ID producers before their dependent issue.Documentation
blocked_byoutput field and supported reference formats.{ "type": "create_issue", "temporary_id": "aw_followup", "title": "Follow-up work", "body": "Implement after the prerequisite.", "blocked_by": ["aw_prereq", "github/gh-aw#123"] }