Repository navigation
Use repository labels for work-queue Issue status - #67223
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Label-length validation, concurrent provisioning, mutation pacing, and breaking-change release metadata must be corrected.
7 open findings
What changed in this PR
Replaces work-queue Issue status fields with repository status labels.
Changes:
- Projects statuses through purple
<prefix>: <status>labels. - Removes
status-fieldconfiguration. - Updates tests, schemas, and documentation.
| File | Description |
|---|---|
pkg/workflow/work_queue_issues.go |
Removes status-field configuration. |
pkg/workflow/work_queue_issues_test.go |
Updates configuration and compilation tests. |
pkg/parser/schemas/main_workflow_schema.json |
Updates the Issue projection schema. |
actions/setup/js/work_queue_issues.cjs |
Implements status-label synchronization. |
actions/setup/js/work_queue_issue_api.cjs |
Adds label discovery and provisioning. |
actions/setup/js/work_queue_issue_messages.cjs |
Always includes status in summaries. |
actions/setup/js/work_queue_issues_checks.cjs |
Tests label replacement and provisioning. |
actions/setup/js/work_queue_issues_acceptance_checks.cjs |
Updates acceptance coverage and metrics. |
docs/src/content/docs/reference/work-queue.md |
Documents label-based projection. |
docs/src/content/docs/reference/frontmatter-full.md |
Removes the deprecated field example. |
docs/src/content/docs/guides/deploy-work-queue.md |
Updates deployment guidance. |
docs/src/content/docs/specs/work-queue-specification.md |
Updates implementation evidence. |
.github/aw/work-queue.md |
Updates the work-queue design description. |
🧠 Review effort: Balanced
| const response = await github.graphql("query WorkQueueStatusLabel($repositoryId:ID!,$name:String!) { node(id:$repositoryId) { ... on Repository { id label(name:$name) { id name color } } } }", { repositoryId, name }); | ||
| if (response?.node?.id !== repositoryId) throw queueError("projection_label_pending", "status label target is inaccessible"); | ||
| return ensureLabel(github, repositoryId, response.node.label, name); |
There was a problem hiding this comment.
Implemented duplicate-label recovery: on an already-taken response, the projector re-queries the status label and verifies/recolors it. Added a concurrent provisioning test. Fixed in 3664808.
| if (obsolete.length) | ||
| edits.push({ | ||
| key: target.work_id, | ||
| name: "removeLabelsFromLabelable", |
There was a problem hiding this comment.
Added removeLabelsFromLabelable to mutation cost calculation and a test charging a 50-removal batch. Fixed in 3664808.
| "properties": { | ||
| "label": { "type": "string", "minLength": 1, "maxLength": 256, "pattern": "\\S", "default": "work" }, | ||
| "status-field": { "type": "string", "minLength": 1, "maxLength": 256, "pattern": "\\S", "description": "Pre-provisioned native organization single-select field. Omit for comment-only status." } | ||
| "label": { "type": "string", "minLength": 1, "maxLength": 239, "pattern": "\\S", "default": "work" } |
There was a problem hiding this comment.
Tightened the schema prefix maximum to 33 and documented the GitHub label-length constraint. Fixed in 3664808.
| type WorkQueueIssuesConfig struct { | ||
| Label string `json:"label"` | ||
| StatusField string `json:"status-field,omitempty"` | ||
| Label string `json:"label"` |
There was a problem hiding this comment.
Added a major changeset documenting removal of status-field and migration to repository label prefixes. Fixed in 3664808.
| disables the integration. The tracking label must be a nonblank literal of at | ||
| most 239 bytes; unknown keys are rejected. |
There was a problem hiding this comment.
Updated the reference to state the 33-byte prefix limit and its relationship to GitHub’s 50-character label limit. Fixed in 3664808.
|
✅ PR Code Quality Reviewer completed the code quality review. diagnosing safeoutputs bridge after denied PR review write
|
|
✅ Ponytail Reviewer completed successfully!
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. No ADR enforcement needed for PR #67223: no implementation label (has_implementation_label=false) and only 18 new lines in default business logic directories (threshold 100, requires_adr_by_default_volume=false). No custom .design-gate.yml present.
|
|
✅ Test Quality Sentinel completed test quality analysis. Caution agentic threat detected DetailsPotential security threats were detected in the agent output. Review the workflow run logs for details. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
One opportunity to remove the single-connection pagination abstraction.
net: -5 lines possible.
Generated by ✂️ Ponytail Reviewer for #67223 · codex · gpt56 · 20.6 AIC · ⌖ 6.4 AIC · ⊞ 13.5K
Comment /ponytail to run again
| const pending = []; | ||
| for (const [index, issue] of issues.entries()) { | ||
| for (const connection of fields ? ["labels", "issueFieldValues"] : ["labels"]) { | ||
| for (const connection of ["labels"]) { |
There was a problem hiding this comment.
L143: yagni: generic connection loop and dynamic property bookkeeping now serve only ["labels"]. Inline label pagination and retain just issue/alias pending state.
There was a problem hiding this comment.
Replaced the generic connection loop with inline labels-only pagination, keeping just issue/alias pending state. Fixed in 3664808.
|
Caution agentic threat detected DetailsPotential security threats were detected in the agent output. Review the workflow run logs for details. 🧪 Test Quality Sentinel Report✅ Test Quality Score: 75/100 — Acceptable
📊 Metrics (2 tests modified)
✅ Test Quality NotesIntentional Feature Removal (Aligned):
Coverage Quality:
Consistency:
Verdict
Analysis Summary:
|
There was a problem hiding this comment.
Caution
agentic threat detected
Threat detection flagged this output in warn mode. Manual review is REQUIRED before any follow-up automation.
Details
Potential security threats were detected in the agent output.
Review the workflow run logs for details.
✅ Test Quality Sentinel: 75/100. 0% implementation tests (threshold: 30%). Tests correctly refactored to align with intentional feature removal (StatusField → repository labels). No violations.
🧪 Test quality analysis by Test Quality Sentinel · copilot · haiku45 · 24.9 AIC · ⌖ 8.38 AIC · ⊞ 8.2K
Comment /review to run again
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /codebase-design. This is a solid architectural migration from org-level native Issue fields to repository labels, with good test coverage updates across both the JS and Go layers — but a few issues should be addressed before merge.
📋 Key Themes & Highlights
Key Themes
- Pacing cost under-counts remove-only batches:
work_queue_issue_pacing.cjs's cost regex was not updated for the newremoveLabelsFromLabelablemutation, so a batch that only removes obsolete status labels is priced at cost 0 and under-paced against GitHub's rate limits. - Leftover/dead code in the main projection loop:
summaryBody,issueBody, andclaimBodyare called inprojectBatch's per-target loop (lines 216-218) with their results discarded — looks like debugging/scratch code that duplicates template rendering with no effect. - Label length limit is wider than GitHub allows: the new 239-byte limit (schema, Go, and JS) permits prefixes whose generated
work: Needs attention-style label exceeds GitHub's 50-char label cap, deferring a preventable failure from compile-time to runtime. - Breaking schema change without a changeset: removing
status-fieldoutright (rather than deprecating) breaks previously valid frontmatter at compile time, and no changeset documents this for the release notes/migration path.
Positive Highlights
- ✅ Clean removal of the complex native-field GraphQL discovery/pagination logic — the label-based path is noticeably simpler and easier to follow.
- ✅ Good care taken to preserve unrelated labels (
obsoletefiltering only targets this prefix's own status labels) and to verify label color/creation idempotently. - ✅ Test suite updated thoroughly across acceptance checks, unit checks, and Go config tests, including request-count assertions for the new status-label lookup.
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 95.6 AIC · ⌖ 15.7 AIC · ⊞ 10.1K
Comment /matt to run again
| const work = current.state.works.get(target.work_id); | ||
| summaryBody(current.state, work, options.now ?? Date.now(), [], current.branch, origin); | ||
| if (!target.resource) issueBody(current.state, work, origin, config, current.branch); | ||
| for (const claimId of target.claim_ids) claimBody(current.state, current.state.claims.get(claimId), options.now ?? Date.now()); |
There was a problem hiding this comment.
[/diagnosing-bugs] Dead/wasted calls: summaryBody(...), issueBody(...), and claimBody(...) are invoked per-target inside this loop but their return values are discarded — nothing checks the result or throws on failure here (the real bodies are computed later in projectLinkedIssues/the create path). This duplicates template rendering work for every target on every projection pass with no observable effect.
💡 Analysis
Lines 216-218:
summaryBody(current.state, work, options.now ?? Date.now(), [], current.branch, origin);
if (!target.resource) issueBody(current.state, work, origin, config, current.branch);
for (const claimId of target.claim_ids) claimBody(current.state, current.state.claims.get(claimId), options.now ?? Date.now());None of these results are assigned, used for validation, or awaited for side effects (the functions are synchronous template renderers). If this was meant to pre-validate templates/throw early on bad state, the result should be checked; otherwise it's leftover debugging code that should be removed before merge — it adds CPU cost with each projection batch and obscures the loop's actual purpose (status-label computation).
@copilot please address this.
There was a problem hiding this comment.
Removed the unused summary, Issue, and claim body rendering calls from the target loop. Fixed in 3664808.
| "properties": { | ||
| "label": { "type": "string", "minLength": 1, "maxLength": 256, "pattern": "\\S", "default": "work" }, | ||
| "status-field": { "type": "string", "minLength": 1, "maxLength": 256, "pattern": "\\S", "description": "Pre-provisioned native organization single-select field. Omit for comment-only status." } | ||
| "label": { "type": "string", "minLength": 1, "maxLength": 239, "pattern": "\\S", "default": "work" } |
There was a problem hiding this comment.
[/codebase-design] The 239-byte schema limit (and the matching 239-byte Go/JS runtime checks) is wider than what GitHub actually allows: label names cap at 50 characters, and the longest generated status suffix : Needs attention consumes 17, leaving only 33 usable bytes for label. A long-but-schema-valid prefix will pass compilation/validation and then fail at projection time with an opaque GraphQL label-creation error, which is a worse failure mode than a compile-time rejection — this is the same issue already flagged by another reviewer on this PR.
💡 Suggested fix
Tighten maxLength here (and the corresponding Go/JS runtime checks in pkg/workflow/work_queue_issues.go and actions/setup/js/work_queue_issues.cjs) to 33, and add a one-line schema description explaining the 50-char GitHub label limit minus the longest status suffix. This keeps validation fail-fast and consistent across all three enforcement points instead of silently deferring the failure to runtime.
@copilot please address this.
There was a problem hiding this comment.
Tightened the schema prefix maximum to 33 and aligned Go/JS runtime validation and docs. Fixed in 3664808.
| type WorkQueueIssuesConfig struct { | ||
| Label string `json:"label"` | ||
| StatusField string `json:"status-field,omitempty"` | ||
| Label string `json:"label"` |
There was a problem hiding this comment.
[/codebase-design] Removing status-field entirely (rather than deprecating it) breaks compilation for any existing workflow frontmatter that still sets tools.work-queue.issues.status-field, turning a previously valid config into a hard compile error. Per this repo's release rules this is a breaking schema change, and there's no changeset in this branch (.changeset/ has no new entry) documenting it or providing migration guidance.
💡 Suggested fix
Either (a) keep status-field as an accepted-but-ignored/deprecated key with a clear validation warning pointing users to the new label-based projection, or (b) if a hard break is intentional, add a changeset explicitly calling out the breaking change and update docs/src/content/docs/reference/work-queue.md's migration guidance so users aren't surprised by a sudden compile failure on upgrade.
@copilot please address this.
There was a problem hiding this comment.
Added a major changeset with migration guidance for status-field removal. Fixed in 3664808.
There was a problem hiding this comment.
Impeccable review — mode: refactor_cleanup (distill/extract applied to the label-projection refactor)
This PR swaps the native Issue status-field projection for repository labels — a sizeable, well-tested refactor across work_queue_issue_api.cjs, work_queue_issues.cjs, and the Go config/schema layer.
New finding (posted inline):
work_queue_issues.cjs:216-218— three dead calls (summaryBody,issueBody,claimBody) whose results are discarded inside the per-target loop. They perform real template file I/O for no observable effect, since the real values are recomputed later (lines 271, 364-365). Looks like leftover debug/validation scaffolding.
Pre-existing unresolved Copilot findings still apply to this diff (not duplicated as new comments, listed here for completeness):
work_queue_issue_api.cjs:46— query-then-create label provisioning is not idempotent across concurrent projections for a previously unseen status.work_queue_issues.cjs:25— no byte-length ceiling onlabelprevents exceeding GitHub's 50-char label-name limit once the: <status>suffix is appended.work_queue_issues.cjs:357—removeLabelsFromLabelableis absent from the pacing cost regex (work_queue_issue_pacing.cjs:34), confirmed during this review: batches combining removals and additions in a single mutation query under-count/under-pace the removal cost.pkg/parser/schemas/main_workflow_schema.json:5393andpkg/workflow/work_queue_issues.go:92/docs/.../work-queue.md:68— the 239-byte config limit is not tight enough to guarantee a valid ≤50-char generated label.pkg/workflow/work_queue_issues.go:11— removingstatus-fieldis a breaking frontmatter change for existing workflows; confirm this aligns with the repo's release/breaking-change rules.
These remain genuinely blocking (correctness/compat), so I'm requesting changes pending their resolution, in addition to the new dead-code cleanup.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 129.9 AIC · ⌖ 13.2 AIC · ⊞ 8.1K
| const work = current.state.works.get(target.work_id); | ||
| summaryBody(current.state, work, options.now ?? Date.now(), [], current.branch, origin); | ||
| if (!target.resource) issueBody(current.state, work, origin, config, current.branch); | ||
| for (const claimId of target.claim_ids) claimBody(current.state, current.state.claims.get(claimId), options.now ?? Date.now()); |
There was a problem hiding this comment.
These three calls (summaryBody, issueBody, claimBody) are invoked here with their return values discarded — they have no assertions, no side effects on journal/target, and aren't used again before the real projection logic runs later (lines 271, 364-365 recompute the actual bodies). Each call does real work: renderSummary/issueBody/claimBody read markdown templates from disk via renderTemplateFromFile/fs.readFileSync and format text, so this adds unnecessary file I/O per target per batch for no observable benefit. This looks like leftover debug/validation scaffolding that should be removed, or (if it was meant to catch rendering errors early) its exceptions should be surfaced/asserted rather than silently swallowed by discarding the result.
@copilot please address this.
There was a problem hiding this comment.
Removed the discarded body-rendering calls from the per-target loop. Fixed in 3664808.
|
@copilot address the following outstanding work in one pass:
Push the necessary fixes, reply to each listed review thread and resolve it when addressed. Ignore feedback already answered or resolved. Use the pr-finisher skill and stop when only human review or CI remains; do not trigger CI. Sous-chef head: 3083c72
|
…k-queue-labels Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
@copilot resolve the merge conflicts in this pull request |
Merged |
…k-queue-labels # Conflicts: # actions/setup/js/work_queue_issues_checks.cjs Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Merged the latest |
| let repositoryId; | ||
| let label; | ||
| function statusLabelName(config, status) { | ||
| return `${config.label}: ${status}`; |
There was a problem hiding this comment.
Generated status labels now use lowercase hyphenated names without spaces, including whitespace-normalized prefixes. Added test coverage and updated the reference docs in c02d3f9.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Organization-level Issue fields may be unavailable to a repository, leaving work-queue status unsynchronized. This PR proposes repository labels as the status projection instead.
work: Queuedorwork: Running, using GraphQL. Replace only work-queue status labels; preserve unrelated labels.#7057FF) to work-queue labels.status-field.issues: trueuseswork;issues: {label: cookie}uses labels such ascookie: Queued.