Default-on issue intent metadata for set_issue_type, set_issue_field, and add_labels#46207
Conversation
… omitted Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
- Change `set_issue_type`, `set_issue_field`, `add_labels` handlers from `config.issue_intent === true` to `config.issue_intent !== false` so intent metadata is forwarded by default, consistent with `close_issue` and `assign_to_agent` semantics. - Add strict-mode enforcement in `add_labels`: when `issue_intent: true` is explicitly set, plain string label names are rejected. - Add `issue-intent` boolean to JSON schema for `add-labels`, `set-issue-type`, and `set-issue-field` (compiler previously rejected the frontmatter for these handlers). - Add focused tests covering omitted config, strict mode, metadata forwarding, and plain-string compatibility. Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
Thanks for the clear PR description and for calling out the consistency issue with This PR looks ready for review — nice work tightening the default-on
|
There was a problem hiding this comment.
Pull request overview
Defaults issue-intent metadata forwarding on while preserving explicit opt-out and adding strict label handling.
Changes:
- Default-enable intent metadata for three issue handlers.
- Add strict structured-label validation.
- Extend schema support and focused tests.
Show a summary per file
| File | Description |
|---|---|
pkg/parser/schemas/main_workflow_schema.json |
Adds issue-intent configuration fields. |
actions/setup/js/set_issue_type.cjs |
Defaults to the intent-aware GraphQL path. |
actions/setup/js/set_issue_type.test.cjs |
Tests default intent behavior. |
actions/setup/js/set_issue_field.cjs |
Defaults metadata forwarding on. |
actions/setup/js/set_issue_field.test.cjs |
Tests default metadata forwarding. |
actions/setup/js/add_labels.cjs |
Defaults metadata forwarding and adds strict mode. |
actions/setup/js/add_labels.test.cjs |
Tests default, disabled, and strict modes. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Medium
| if (issueIntentStrict) { | ||
| const plainStringLabels = requestedLabelInputs.filter(label => typeof label === "string"); | ||
| if (plainStringLabels.length > 0) { | ||
| const error = `Plain string label names are not permitted when issue_intent is explicitly enabled. Provide label objects with a "name" field and intent metadata (rationale, confidence). Plain labels: ${plainStringLabels.map(l => JSON.stringify(l)).join(", ")}`; |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
The default-on change (!== false) is correct and consistent with close_issue/assign_to_agent. The one-liner fixes for set_issue_type and set_issue_field are clean and well-tested.
Blocking issue: add_labels strict mode gap
See the existing inline comment at add_labels.cjs:107. The strict-mode check rejects plain strings but silently passes objects lacking intent metadata (e.g. { name: "bug" }). Such objects are then downgraded to plain strings at line 206, so an agent can bypass the strict-mode intent requirement. The check should additionally reject label objects missing all of rationale/confidence/suggest.
Minor
- Schema
descriptionfields forissue-intentshould mention the behavioural difference:true= metadata required on every label; omitted = plain strings still accepted. - Add a test for strict mode with
{ name: "bug" }(bare object, no metadata) to document expected rejection once the gap is fixed.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 28.1 AIC · ⌖ 4.34 AIC · ⊞ 5K
There was a problem hiding this comment.
❌ Request Changes
Two blocking issues before merge.
🔍 Findings summary
Critical: Implicit breaking behavior change (3 files)
The === true → !== false flip is silently backwards-incompatible. Every existing deployment that omits issue_intent frontmatter will silently switch from REST to the GraphQL/intent path. This can change API call shapes, required permissions, and data formats without any user signal. See inline comment on add_labels.cjs:10.
High: Schema key mismatch renders opt-out non-functional
The schema adds "issue-intent" (kebab-case) but handlers read config.issue_intent (snake_case). Unless the compiler normalizes keys, issue_intent: false opt-out is unreachable from frontmatter. See inline comment on main_workflow_schema.json:239.
🔎 Code quality review by PR Code Quality Reviewer · 28.9 AIC · ⌖ 4.6 AIC · ⊞ 5.6K
Comment /review to run again
Comments that could not be inline-anchored
actions/setup/js/add_labels.cjs:10
Silent breaking change: flipping the gate from === true to !== false is a backwards-incompatible behavior change for any existing workflow that omits issue_intent.
<details>
<summary>💡 Why this is risky</summary>
Before this PR, omitting issue_intent meant issueIntentEnabled = false — the REST path was taken. After this PR, omitting it means issueIntentEnabled = true — the GraphQL/intent path is taken silently.
Any deployed workflow relying on the old REST behavior (no front…
pkg/parser/schemas/main_workflow_schema.json:239
Schema key mismatch: schema uses issue-intent (kebab-case) but runtime code reads config.issue_intent (snake_case) — opt-out via frontmatter will silently have no effect.
<details>
<summary>💡 Details</summary>
The schema adds the property as "issue-intent" (lines 236–239, 247–250, 258–261). JavaScript reads it as config.issue_intent (the underscore form). Unless the workflow compiler or YAML loader transparently normalizes kebab-case keys to snake_case, setting `issue-intent: fal…
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (114 new lines across 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — requesting changes on a strict-mode enforcement gap.
📋 Key Themes & Highlights
Key Issue
Strict-mode contract is partially enforced. issueIntentStrict correctly rejects plain strings but silently allows label objects without metadata (e.g. { name: "bug" }), which then get downgraded to plain strings at line 206. The behaviour contradicts the stated contract that strict mode requires metadata. This needs both a code fix and a companion test.
Positive Highlights
- ✅ Clean default-on alignment with
close_issue/assign_to_agent— consistent and well-explained in the PR body. - ✅ Good test coverage for the omitted-config and explicit-false cases.
- ✅ Schema additions are minimal and correctly placed.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 38 AIC · ⌖ 4.56 AIC · ⊞ 6.7K
Comment /matt to run again
Comments that could not be inline-anchored
actions/setup/js/add_labels.cjs:206
actions/setup/js/add_labels.cjs:206
[/codebase-design] In strict mode (issue_intent: true), label objects without any intent metadata (e.g. { name: "bug" }) pass the string-rejection guard on line 104 but are silently downgraded to plain strings here, defeating the contract strict mode promises.
<details>
<summary>💡 Suggested fix</summary>
Extend the strict-mode guard to also reject metadata-free objects:
if (issueIntentStrict) {
const withoutMetadata = requestedLabelInputs.filter(
label => typeof label !==…
</details>
<details><summary>actions/setup/js/add_labels.test.cjs:606</summary>
**[/tdd]** The strict-mode test only covers plain string inputs. A `{ name: "bug" }` object (no metadata) also violates the strict-mode contract but is not tested — this is the same gap flagged on the implementation side.
<details>
<summary>💡 Suggested test</summary>
```js
it('should reject object labels without metadata in strict mode', async () => {
const handler = await main({ max: 10, issue_intent: true });
const result = await handler({ item_number: 1, labels: [{ name: 'bug' }] }, {…
</details>
🧪 Test Quality Sentinel Report
📊 Metrics (50 tests analyzed, sampled from ~100)
|
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…labels schema - add_labels.cjs: strict mode (issue_intent: true) now rejects label objects missing rationale or confidence, not just plain strings - generate_safe_outputs_tools.cjs: when issue-intent: true, labels items schema becomes object-only with name/rationale/confidence required - Tests: added coverage for missing-metadata rejection and schema change; moved deduplication test to default mode (omitted issue_intent) since mixed metadata quality is invalid in strict mode Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in b46ac20. Two concrete fixes:
The deduplication test ( |
|
🎉 This pull request is included in a new release. Release: |
Three handlers (
set_issue_type,set_issue_field,add_labels) gated intent metadata onconfig.issue_intent === true, silently discarding agent-supplied rationale/confidence when frontmatter was omitted — inconsistent withclose_issueandassign_to_agentwhich default-on with!== false.Handler changes
set_issue_type/set_issue_field/add_labels: Change gate fromconfig.issue_intent === true→config.issue_intent !== false. Omitted frontmatter now forwards metadata through the GraphQL/intent-aware path by default.add_labelsstrict mode: IntroduceissueIntentStrict = config.issue_intent === true. When explicitlytrue, plain-string label names are rejected — agents must supply structured objects with intent fields. Default (omitted) accepts both strings and objects.Schema
Added
issue-intent: booleanproperty toadd-labels,set-issue-type, andset-issue-fieldinmain_workflow_schema.json. Previously the compiler rejectedissue-intentfrontmatter for these handlers, making opt-out (issue_intent: false) impossible via valid workflow syntax.Tests
Added focused tests covering: omitted-config selects the intent path and forwards metadata; strict mode (
true) rejects plain string labels; plain strings remain compatible in default mode; explicit disable (false) still selects the legacy REST path.