Align issue-intent rationale limits to GitHub API 280-char constraint#42809
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #42809 does not have the 'implementation' label and has only 10 new lines of code in business logic directories (threshold: 100). |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
There was a problem hiding this comment.
Pull request overview
Aligns issue-intent rationale handling with the GitHub API’s 280-character limit by tightening tool schema contracts and adding/adjusting runtime truncation behavior, with regression tests to prevent future drift.
Changes:
- Added
maxLength: 280+ updated descriptions for issue-intentrationalefields in both safe output tool schema copies. - Reduced runtime rationale limits to 280 and added explicit post-sanitize clamping to guarantee outbound payloads never exceed 280 chars.
- Added/updated tests to cover truncation behavior for
set_issue_typeand structured label rationales.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/js/safe_outputs_tools.json | Tightens tool schema contract for issue-intent rationales (maxLength 280). |
| actions/setup/js/safe_outputs_tools.json | Mirrors the same schema contract tightening for the setup action runtime copy. |
| actions/setup/js/issue_intents.cjs | Lowers rationale cap to 280 and clamps post-sanitize before emitting intent metadata. |
| actions/setup/js/safe_output_type_validator.cjs | Clamps structured label rationale sanitization to 280 and enforces hard cap. |
| actions/setup/js/safe_output_type_validator.test.cjs | Updates expected rationale maxLength and adds truncation coverage for structured label rationale. |
| actions/setup/js/set_issue_type.test.cjs | Adds truncation regression coverage for issue-intent rationale in the GraphQL mutation path. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 6/6 changed files
- Comments generated: 2
- Review effort level: Low
| issue_number: { issueOrPRNumber: true }, | ||
| issue_type: { required: true, type: "string", sanitize: true, maxLength: 128 }, | ||
| rationale: { type: "string", sanitize: true, maxLength: 1024 }, | ||
| rationale: { type: "string", sanitize: true, maxLength: 280 }, |
| field_node_id: { type: "string", maxLength: 256 }, | ||
| value: { required: true, type: "string", sanitize: true, maxLength: 256 }, | ||
| rationale: { type: "string", sanitize: true, maxLength: 1024 }, | ||
| rationale: { type: "string", sanitize: true, maxLength: 280 }, |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 90/100 — Excellent
📊 Metrics (2 tests)
📐 Inflation Detail (2 files flagged)
Note: The inflation ratios are high but contextually justified. The production change is a 1-line constant update (1024→280), while the tests add full integration coverage including setup, mock orchestration, and boundary assertion. The test-to-production ratio reflects the thoroughness of the coverage, not bloat. Verdict
References: Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Align issue-intent rationale limits to GitHub API 280-char constraint
This is a clean, focused fix that correctly aligns runtime enforcement to the GitHub API constraint and updates all relevant schema copies and tests.
Correctness ✅
ISSUE_INTENT_RATIONALE_MAX_LENGTHinissue_intents.cjscorrectly drops from 1024 → 280.- The double-sanitize + clamp pattern is sound:
sanitizeContentstrips unsafe chars first, then the hard clamp guarantees the final payload length. Defensive-in-depth. - The label-path in
validateIssueIntentLabelsalso clamps to 280, consistent with the intent-level path. - Both
actions/setup/js/andpkg/workflow/js/schema copies updated in lockstep.
Tests ✅
- New truncation test in
set_issue_type.test.cjsverifies the GraphQL mutation payload carries the truncated value. - New truncation test in
safe_output_type_validator.test.cjscovers the label-intent path.
Non-blocking suggestion (see inline comment)
The literal 280 is hard-coded twice in validateIssueIntentLabels in safe_output_type_validator.cjs. Extracting a named constant would make a future limit change a one-liner and eliminate the risk of the two occurrences drifting apart.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 62.2 AIC · ⌖ 6.53 AIC · ⊞ 4.9K
| allowedAliases: options?.allowedAliases || [], | ||
| maxBotMentions: options?.maxBotMentions, | ||
| }).trim(); | ||
| const rationale = sanitizedRationale.length > 280 ? sanitizedRationale.slice(0, 280) : sanitizedRationale; |
There was a problem hiding this comment.
The literal 280 is hard-coded twice in this function (passed to sanitizeContent on line 136 and used in the clamp on line 140). This value must stay in sync with ISSUE_INTENT_RATIONALE_MAX_LENGTH in issue_intents.cjs. Consider extracting a module-level constant (e.g. const LABEL_RATIONALE_MAX_LENGTH = 280) to make future limit changes a one-line update and signal intent.\n\n@copilot please address this.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — no blocking correctness issues, but several improvements worth addressing before this pattern spreads.
📋 Key Themes & Highlights
Key Themes
- Undocumented defensive clamp: The post-sanitize
slicein bothissue_intents.cjsandsafe_output_type_validator.cjsis unexplained. It is either dead code (ifsanitizeContentalready honoursmaxLength) or a workaround for a latent bug insanitizeContent— either way it should be documented or tested. - Magic literal
280: The limit appears as a bare literal twice insafe_output_type_validator.cjswithout a named constant. If the limit changes again, two spots will need updating independently. - Boundary and path coverage gaps: The new regression tests cover a single over-length input on two paths (
set_issue_type,add_labels). Theremove_labelsandupdate_issuepaths are unverified, and the boundary conditions (exactly 280, exactly 281, empty post-sanitize) are missing. - Test coupling to GraphQL internals: The
set_issue_typetruncation test reaches into the raw GraphQL mock call to assert the value. TestingnormalizeIssueIntentMetadatadirectly would be simpler and more resilient.
Positive Highlights
- ✅ Multi-layer defence (schema
maxLength+ runtime constant + explicit clamp) is the right approach for an API constraint this critical. - ✅ Both JSON schema files (
actions/setupandpkg/workflow) updated in sync — good discipline. - ✅ Clean variable rename (
rationale→sanitizedRationale) makes the two-step processing explicit. - ✅ PR description is thorough and clearly explains the mismatch and the fix strategy.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 77.7 AIC · ⌖ 9.22 AIC · ⊞ 6.6K
Comment /matt to run again
| const rationale = sanitizeContent(source.rationale, { maxLength: ISSUE_INTENT_RATIONALE_MAX_LENGTH }).trim(); | ||
| const sanitizedRationale = sanitizeContent(source.rationale, { maxLength: ISSUE_INTENT_RATIONALE_MAX_LENGTH }).trim(); | ||
| const rationale = sanitizedRationale.length > ISSUE_INTENT_RATIONALE_MAX_LENGTH ? sanitizedRationale.slice(0, ISSUE_INTENT_RATIONALE_MAX_LENGTH) : sanitizedRationale; | ||
| if (rationale) { |
There was a problem hiding this comment.
[/diagnosing-bugs] The post-sanitize clamp is undocumented — it's unclear whether sanitizeContent already enforces maxLength or can overshoot it.
If sanitizeContent always honours its maxLength parameter, the guard is dead code. If it can exceed the limit (e.g. due to word-boundary logic), that is a bug in sanitizeContent that warrants its own test rather than a silent fallback.
💡 Suggested improvement
Add a brief comment explaining the guard:
// sanitizeContent may produce slightly more than maxLength in some edge cases;
// slice enforces the GitHub API hard limit of 280 chars.
const rationale = sanitizedRationale.length > ISSUE_INTENT_RATIONALE_MAX_LENGTH
? sanitizedRationale.slice(0, ISSUE_INTENT_RATIONALE_MAX_LENGTH)
: sanitizedRationale;Or, if sanitizeContent already guarantees the limit, remove the guard and add an assertion.
@copilot please address this.
| const rationale = sanitizedRationale.length > 280 ? sanitizedRationale.slice(0, 280) : sanitizedRationale; | ||
| if (rationale) { | ||
| normalizedLabel.rationale = rationale; | ||
| } |
There was a problem hiding this comment.
[/diagnosing-bugs] The magic literal 280 is duplicated here instead of referencing a shared constant — if the limit changes again, this will need a separate update.
ISSUE_INTENT_RATIONALE_MAX_LENGTH is defined in issue_intents.cjs; the validator has no access to it, so the limit is hardcoded as 280 twice in this file.
💡 Suggested improvement
Extract a module-level constant in safe_output_type_validator.cjs:
const RATIONALE_MAX_LENGTH = 280;Then use it in both the sanitizeContent call and the clamp guard so the limit is defined once.
@copilot please address this.
| ); | ||
|
|
||
| expect(result.success).toBe(true); | ||
| const mutationCall = mockGithub.graphql.mock.calls.find(([query]) => typeof query === "string" && query.includes("IssueTypeUpdateInput")); |
There was a problem hiding this comment.
[/tdd] The new truncation test only checks the happy path (350-char input → 280-char output). Edge cases at the boundary are not covered.
Missing boundary conditions: exactly 280 chars (should pass through unchanged), exactly 281 chars (first value that needs truncation), and empty string / whitespace-only (should produce no rationale field).
💡 Suggested additional tests
it('should not truncate rationale of exactly 280 characters', async () => {
const exactRationale = 'a'.repeat(280);
// ... same mock setup ...
expect(mutationCall[1].issueType.rationale).toBe(exactRationale);
});
it('should truncate rationale of 281 characters to 280', async () => {
// ... same mock setup, longRationale = 'a'.repeat(281) ...
expect(mutationCall[1].issueType.rationale).toBe('a'.repeat(280));
});
it('should omit rationale when it is empty after sanitization', async () => {
// ... same mock setup, rationale = '' ...
expect(mutationCall[1].issueType.rationale).toBeUndefined();
});@copilot please address this.
|
|
||
| expect(result.isValid).toBe(true); | ||
| expect(result.normalizedItem.labels[0].rationale).toBe("a".repeat(280)); | ||
| }); |
There was a problem hiding this comment.
[/tdd] The label-rationale truncation test only exercises the add_labels path. The same clamp logic also runs for remove_labels and update_issue label entries, but those paths are untested.
If the clamp is extracted into a shared helper (as suggested in the constant comment above), a single test of the helper covers all callers — otherwise add parallel tests for each mutating label intent type.
@copilot please address this.
| const mutationCall = mockGithub.graphql.mock.calls.find(([query]) => typeof query === "string" && query.includes("IssueTypeUpdateInput")); | ||
| expect(mutationCall).toBeDefined(); | ||
| expect(mutationCall[1].issueType.rationale).toBe("a".repeat(280)); | ||
| } finally { |
There was a problem hiding this comment.
[/tdd] The test inspects the raw GraphQL mutation call to assert the rationale value, which couples the test to the internal mutation shape rather than the observable contract.
A higher-signal assertion would verify that the returned result object contains (or doesn't contain) a truncated rationale, rather than inspecting the mock call internals — or, better, test normalizeIssueIntentMetadata directly in issue_intents.test.cjs.
💡 Alternative approach
Test the normalizer directly so the test does not depend on GraphQL mock wiring:
// In a dedicated issue_intents.test.cjs or existing test file:
const { normalizeIssueIntentMetadata } = require('./issue_intents.cjs');
it('truncates rationale to 280 characters', () => {
const meta = normalizeIssueIntentMetadata({ rationale: 'a'.repeat(350) });
expect(meta.rationale).toHaveLength(280);
});@copilot please address this.
There was a problem hiding this comment.
Verdict: request changes — PR partially achieves its stated goal
The schema updates and JS runtime limits are correctly aligned to 280, but the Go config (the real source of truth for GH_AW_VALIDATION_CONFIG) was not updated, so set_issue_type.rationale and set_issue_field.rationale will still accept up to 1024 chars at runtime.
Blocking issues
pkg/workflow/safe_outputs_validation_config.go lines 138 and 152 still use MaxLength: 1024.
The test file's SAMPLE_VALIDATION_CONFIG was updated to 280, but that object is a local fixture — it does not affect what gets emitted into GH_AW_VALIDATION_CONFIG. At runtime, safe_output_type_validator.cjs reads the config generated from the Go struct, which still has 1024 for both set_issue_type.rationale and set_issue_field.rationale. The schema says 280, the validator won't enforce it.
(Already flagged inline by existing review comments — confirming those findings are correct.)
Additional issues
issue_intents.cjs:29— The secondary.slice(0, 280)is essential (not redundant):sanitizeContent'sapplyTruncationappends"\n[Content truncated due to length]"which inflates output beyondmaxLength. Without a comment, this will get removed as a "cleanup", sending the marker to the GitHub API. See inline comment.safe_output_type_validator.cjs:140—280is a bare magic number, used twice with no named constant, diverging from theISSUE_INTENT_RATIONALE_MAX_LENGTHpattern inissue_intents.cjs. See inline comment.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
patchdiff.githubusercontent.com
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "patchdiff.githubusercontent.com"See Network Configuration for more information.
🔎 Code quality review by PR Code Quality Reviewer · 148.8 AIC · ⌖ 7.36 AIC · ⊞ 1.6K
Comment /review to run again
| if (typeof source.rationale === "string") { | ||
| const rationale = sanitizeContent(source.rationale, { maxLength: ISSUE_INTENT_RATIONALE_MAX_LENGTH }).trim(); | ||
| const sanitizedRationale = sanitizeContent(source.rationale, { maxLength: ISSUE_INTENT_RATIONALE_MAX_LENGTH }).trim(); | ||
| const rationale = sanitizedRationale.length > ISSUE_INTENT_RATIONALE_MAX_LENGTH ? sanitizedRationale.slice(0, ISSUE_INTENT_RATIONALE_MAX_LENGTH) : sanitizedRationale; |
There was a problem hiding this comment.
Secondary clamp is essential but will look redundant to future readers — without a comment, this slice is one refactor away from being removed, which would silently ship the sanitizeContent truncation marker to the GitHub API.
💡 Why this is non-obvious
sanitizeContent internally calls applyTruncation, which — when the input exceeds maxLength — returns content.substring(0, maxLength) + "\n[Content truncated due to length]". This means for a 350-char input:
sanitizeContent(source.rationale, { maxLength: 280 })→"a"×280 + "\n[Content truncated due to length]"(311 chars).trim()→ no change (the\nis interior, not at the boundary)- Without the slice:
sanitizedRationale(311 chars) would be stored asmetadata.rationale - With the slice:
sanitizedRationale.slice(0, 280)→"a"×280✓
The secondary clamp is what keeps the truncation annotation out of the API payload. Add a comment:
// sanitizeContent appends "\n[Content truncated due to length]" when input exceeds maxLength,
// which inflates the result beyond the limit. Slice again to guarantee the API constraint.
const rationale = sanitizedRationale.length > ISSUE_INTENT_RATIONALE_MAX_LENGTH
? sanitizedRationale.slice(0, ISSUE_INTENT_RATIONALE_MAX_LENGTH)
: sanitizedRationale;| allowedAliases: options?.allowedAliases || [], | ||
| maxBotMentions: options?.maxBotMentions, | ||
| }).trim(); | ||
| const rationale = sanitizedRationale.length > 280 ? sanitizedRationale.slice(0, 280) : sanitizedRationale; |
There was a problem hiding this comment.
Magic number 280 used twice with no named constant — issue_intents.cjs correctly defines ISSUE_INTENT_RATIONALE_MAX_LENGTH = 280; this file hardcodes the same value on both line 136 (maxLength: 280) and here, so they can drift independently on the next limit change.
💡 Suggested fix
Add a module-level constant (or import from a shared location):
const ISSUE_INTENT_RATIONALE_MAX_LENGTH = 280;
// then use it:
const sanitizedRationale = sanitizeContent(unfenceMarkdown(label.rationale), {
maxLength: ISSUE_INTENT_RATIONALE_MAX_LENGTH,
...
}).trim();
const rationale = sanitizedRationale.length > ISSUE_INTENT_RATIONALE_MAX_LENGTH
? sanitizedRationale.slice(0, ISSUE_INTENT_RATIONALE_MAX_LENGTH)
: sanitizedRationale;This also pairs with the note above: having a named constant makes it clear that the maxLength in the sanitizeContent call and the slice boundary must always be the same value.
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>
Addressed in
The branch is up to date with |
🤖 PR Triage
Score breakdown: Impact 35 + Urgency 20 + Quality 10 Rationale: Fixes schema mismatch (rationale maxLength) that causes agent output to silently fail at GitHub API mutation time. Changes requested by automated reviewer — expedited human review recommended to resolve and unblock agent workflows.
|
|
@copilot merge main and recompile |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…fix-rationale-maxlength-issue Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed in
|
Issue-intent mutations were accepting effectively unbounded
rationalevalues in tool schemas and validator paths, while the GitHub API enforces a 280-character limit. This mismatch caused valid-looking agent output to fail at mutation time.Schema contract alignment
maxLength: 280to all issue-intentrationalefields in safe output tool schemas:set_issue_type.rationaleset_issue_field.rationaleadd_labels.labels[].rationaleremove_labels.labels[].rationaleupdate_issue.labels[].rationaleOptional rationale for the change (max 280 characters).Runtime enforcement alignment
ISSUE_INTENT_RATIONALE_MAX_LENGTHfrom1024to280inissue_intents.cjs.1024to280insafe_output_type_validator.cjs.Regression coverage
1024to280.set_issue_typeGraphQL path)add_labelspath)