Align issue-intent rationale limits to GitHub API 280-char constraint #42809
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,7 +7,7 @@ const { sanitizeLabelContent } = require("./sanitize_label_content.cjs"); | |
| const { hasRuntimeFeature, parseRuntimeFeatures } = require("./runtime_features.cjs"); | ||
|
|
||
| const ISSUE_INTENTS_FEATURE = "issue_intents"; | ||
| const ISSUE_INTENT_RATIONALE_MAX_LENGTH = 280; | ||
|
|
||
| function hasIssueIntentsRuntimeFeature() { | ||
| if (typeof global.hasRuntimeFeature === "function") { | ||
|
|
@@ -25,7 +25,10 @@ function normalizeIssueIntentMetadata(source) { | |
| const metadata = {}; | ||
|
|
||
| if (typeof source.rationale === "string") { | ||
| const sanitizedRationale = sanitizeContent(source.rationale, { maxLength: ISSUE_INTENT_RATIONALE_MAX_LENGTH }).trim(); | ||
| // sanitizeContent appends "\n[Content truncated due to length]" when it truncates, | ||
| // so clamp again to guarantee the GitHub API hard limit. | ||
| const rationale = sanitizedRationale.length > ISSUE_INTENT_RATIONALE_MAX_LENGTH ? sanitizedRationale.slice(0, ISSUE_INTENT_RATIONALE_MAX_LENGTH) : sanitizedRationale; | ||
| if (rationale) { | ||
|
Contributor
There was a problem hiding this comment. [/diagnosing-bugs] The post-sanitize clamp is undocumented — it's unclear whether If 💡 Suggested improvementAdd 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 @copilot please address this. |
||
| metadata.rationale = rationale; | ||
| } | ||
|
|
||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,6 +24,7 @@ const MAX_BODY_LENGTH = 65000; | |
| * Reference: https://github.com/dead-claudia/github-limits | ||
| */ | ||
| const MAX_GITHUB_USERNAME_LENGTH = 39; | ||
| const ISSUE_INTENT_RATIONALE_MAX_LENGTH = 280; | ||
|
|
||
| /** | ||
| * @typedef {{ allowedAliases?: string[], maxBotMentions?: number, normalizeIssueClosingKeywords?: boolean }} ValidateOptions | ||
|
|
@@ -64,6 +65,25 @@ function normalizeIssueClosingKeywordBackticks(content) { | |
| return normalized.replace(ISSUE_CLOSING_REFERENCE_BACKTICK_PATTERN, "$1$2$3"); | ||
| } | ||
|
|
||
| /** | ||
| * Normalize issue-intent rationale text while enforcing the GitHub API hard limit. | ||
| * sanitizeContent appends a truncation marker when it shortens content, so this | ||
| * helper slices again to ensure the final payload never exceeds 280 characters. | ||
| * @param {string} rationale | ||
| * @param {ValidateOptions} [options] | ||
| * @returns {string} | ||
| */ | ||
| function normalizeIssueIntentRationale(rationale, options) { | ||
| const sanitizedRationale = sanitizeContent(unfenceMarkdown(rationale), { | ||
| maxLength: ISSUE_INTENT_RATIONALE_MAX_LENGTH, | ||
| allowedAliases: options?.allowedAliases || [], | ||
| maxBotMentions: options?.maxBotMentions, | ||
| }).trim(); | ||
| // sanitizeContent appends "\n[Content truncated due to length]" when it truncates, | ||
| // so clamp again to guarantee the GitHub API hard limit. | ||
| return sanitizedRationale.length > ISSUE_INTENT_RATIONALE_MAX_LENGTH ? sanitizedRationale.slice(0, ISSUE_INTENT_RATIONALE_MAX_LENGTH) : sanitizedRationale; | ||
| } | ||
|
|
||
| /** | ||
| * Validate and normalize issue-intent-aware label arrays. | ||
| * @param {any[]} value | ||
|
|
@@ -132,11 +152,7 @@ function validateIssueIntentLabels(value, lineNum, itemType, fieldName, options) | |
| error: `Line ${lineNum}: ${itemType} ${fieldName}[${i}].rationale must be a string`, | ||
| }; | ||
| } | ||
| const rationale = normalizeIssueIntentRationale(label.rationale, options); | ||
| if (rationale) { | ||
| normalizedLabel.rationale = rationale; | ||
| } | ||
|
Contributor
There was a problem hiding this comment. [/diagnosing-bugs] The magic literal
💡 Suggested improvementExtract a module-level constant in const RATIONALE_MAX_LENGTH = 280;Then use it in both the @copilot please address this. |
||
|
|
||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -116,7 +116,7 @@ const SAMPLE_VALIDATION_CONFIG = { | |
| fields: { | ||
| issue_number: { issueOrPRNumber: true }, | ||
| issue_type: { required: true, type: "string", sanitize: true, maxLength: 128 }, | ||
| rationale: { type: "string", sanitize: true, maxLength: 280 }, | ||
|
|
||
| confidence: { type: "string", enum: ["LOW", "MEDIUM", "HIGH"] }, | ||
| suggest: { type: "boolean" }, | ||
| }, | ||
|
|
@@ -129,7 +129,7 @@ const SAMPLE_VALIDATION_CONFIG = { | |
| field_name: { type: "string", sanitize: true, maxLength: 128 }, | ||
| field_node_id: { type: "string", maxLength: 256 }, | ||
| value: { required: true, type: "string", sanitize: true, maxLength: 256 }, | ||
| rationale: { type: "string", sanitize: true, maxLength: 280 }, | ||
|
|
||
| confidence: { type: "string", enum: ["LOW", "MEDIUM", "HIGH"] }, | ||
| suggest: { type: "boolean" }, | ||
| }, | ||
|
|
@@ -329,6 +329,21 @@ describe("safe_output_type_validator", () => { | |
| expect(result.normalizedItem.labels).toEqual([{ name: "bug", rationale: "Known failure mode", confidence: "HIGH", suggest: true }]); | ||
| }); | ||
|
|
||
| it("should truncate structured label rationale for all issue-intent label mutations", async () => { | ||
| const { validateItem } = await import("./safe_output_type_validator.cjs"); | ||
|
|
||
| for (const [type, item] of [ | ||
| ["add_labels", { type: "add_labels", item_number: 123, labels: [{ name: "bug", rationale: "a".repeat(350) }] }], | ||
| ["remove_labels", { type: "remove_labels", item_number: 123, labels: [{ name: "bug", rationale: "a".repeat(350) }] }], | ||
| ["update_issue", { type: "update_issue", issue_number: 123, labels: [{ name: "bug", rationale: "a".repeat(350) }] }], | ||
| ]) { | ||
| const result = validateItem(item, type, 1); | ||
|
|
||
| expect(result.isValid).toBe(true); | ||
| expect(result.normalizedItem.labels[0].rationale).toBe("a".repeat(280)); | ||
| } | ||
| }); | ||
|
Contributor
There was a problem hiding this comment. [/tdd] The label-rationale truncation test only exercises the 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. |
||
|
|
||
| it("should fail add_labels when structured label entry is invalid", async () => { | ||
| const { validateItem } = await import("./safe_output_type_validator.cjs"); | ||
|
|
||
|
|
||
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -384,4 +384,75 @@ describe("set_issue_type (Handler Factory Architecture)", () => { | |
| delete process.env.GH_AW_RUNTIME_FEATURES; | ||
| } | ||
| }); | ||
|
|
||
| it("should truncate issue_intents rationale to 280 characters", async () => { | ||
| process.env.GH_AW_RUNTIME_FEATURES = "issue_intents"; | ||
|
|
||
| const issueNodeId = "I_kwDO_testissue"; | ||
| const issueTypeNodeId = "IT_kwDO_bug"; | ||
| const longRationale = "a".repeat(350); | ||
|
|
||
| mockGithub.rest.issues.get.mockResolvedValueOnce({ data: { node_id: issueNodeId } }); | ||
| mockGithub.graphql.mockImplementation(async query => { | ||
| if (query.includes("repository(owner")) { | ||
| return { | ||
| repository: { | ||
| issueTypes: { | ||
| nodes: [{ id: issueTypeNodeId, name: "Bug" }], | ||
| }, | ||
| }, | ||
| }; | ||
| } | ||
| if (query.includes("updateIssue")) { | ||
| return { updateIssue: { issue: { id: issueNodeId } } }; | ||
| } | ||
| return {}; | ||
| }); | ||
|
|
||
| try { | ||
| const { main } = require("./set_issue_type.cjs"); | ||
| const featureHandler = await main({ max: 5 }); | ||
|
|
||
| const result = await featureHandler( | ||
| { | ||
| type: "set_issue_type", | ||
| issue_number: 42, | ||
| issue_type: "Bug", | ||
| rationale: longRationale, | ||
| }, | ||
| {} | ||
| ); | ||
|
|
||
| expect(result.success).toBe(true); | ||
| const mutationCall = mockGithub.graphql.mock.calls.find(([query]) => typeof query === "string" && query.includes("IssueTypeUpdateInput")); | ||
|
Contributor
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 💡 Suggested additional testsit('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(mutationCall).toBeDefined(); | ||
| expect(mutationCall[1].issueType.rationale).toBe("a".repeat(280)); | ||
| } finally { | ||
|
Contributor
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 💡 Alternative approachTest 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. |
||
| delete process.env.GH_AW_RUNTIME_FEATURES; | ||
| } | ||
| }); | ||
|
|
||
| it("should preserve issue_intents rationale of exactly 280 characters", () => { | ||
| const { normalizeIssueIntentMetadata } = require("./issue_intents.cjs"); | ||
|
|
||
| const metadata = normalizeIssueIntentMetadata({ rationale: "a".repeat(280) }); | ||
|
|
||
| expect(metadata.rationale).toBe("a".repeat(280)); | ||
| }); | ||
|
|
||
| it("should truncate issue_intents rationale of 281 characters", () => { | ||
| const { normalizeIssueIntentMetadata } = require("./issue_intents.cjs"); | ||
|
|
||
| const metadata = normalizeIssueIntentMetadata({ rationale: "a".repeat(281) }); | ||
|
|
||
| expect(metadata.rationale).toBe("a".repeat(280)); | ||
| }); | ||
|
|
||
| it("should omit empty issue_intents rationale after sanitization", () => { | ||
| const { normalizeIssueIntentMetadata } = require("./issue_intents.cjs"); | ||
|
|
||
| const metadata = normalizeIssueIntentMetadata({ rationale: " " }); | ||
|
|
||
| expect(metadata).not.toHaveProperty("rationale"); | ||
| }); | ||
| }); | ||
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
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
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Secondary clamp is essential but will look redundant to future readers — without a comment, this
sliceis one refactor away from being removed, which would silently ship thesanitizeContenttruncation marker to the GitHub API.💡 Why this is non-obvious
sanitizeContentinternally callsapplyTruncation, which — when the input exceedsmaxLength— returnscontent.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)sanitizedRationale(311 chars) would be stored asmetadata.rationalesanitizedRationale.slice(0, 280)→"a"×280✓The secondary clamp is what keeps the truncation annotation out of the API payload. Add a comment: