Skip to content

Commit 0e23e2d

Browse files
fix(amazonq): handle case where multiple rules are provided with the same name (aws#2118)
* fix(amazonq): handle case where multiple rules are provided with the same name * fix(amazonq): add unit test for duplicate custom guidelines * fix(amazonq): add unit test for processToolUses * fix(amazonq): set context type for AmazonQ.md to rule * fix(amazonq): add README.md back as rule context type --------- Co-authored-by: Blake Lazarine <[email protected]>
1 parent 8975f10 commit 0e23e2d

File tree

4 files changed

+134
-10
lines changed

4 files changed

+134
-10
lines changed

server/aws-lsp-codewhisperer/src/language-server/agenticChat/agenticChatController.test.ts

Lines changed: 85 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -241,7 +241,7 @@ describe('AgenticChatController', () => {
241241
testFeatures.agent = {
242242
runTool: sinon.stub().resolves({}),
243243
getTools: sinon.stub().returns(
244-
['mock-tool-name', 'mock-tool-name-1', 'mock-tool-name-2'].map(toolName => ({
244+
['mock-tool-name', 'mock-tool-name-1', 'mock-tool-name-2', 'codeReview'].map(toolName => ({
245245
toolSpecification: { name: toolName, description: 'Mock tool for testing' },
246246
}))
247247
),
@@ -3283,6 +3283,90 @@ ${' '.repeat(8)}}
32833283
assert.strictEqual(returnValue, undefined)
32843284
})
32853285
})
3286+
3287+
describe('processToolUses', () => {
3288+
it('filters rule artifacts from additionalContext for CodeReview tool', async () => {
3289+
const mockAdditionalContext = [
3290+
{
3291+
type: 'file',
3292+
description: '',
3293+
name: '',
3294+
relativePath: '',
3295+
startLine: 0,
3296+
endLine: 0,
3297+
path: '/test/file.js',
3298+
},
3299+
{
3300+
type: 'rule',
3301+
description: '',
3302+
name: '',
3303+
relativePath: '',
3304+
startLine: 0,
3305+
endLine: 0,
3306+
path: '/test/rule1.json',
3307+
},
3308+
{
3309+
type: 'rule',
3310+
description: '',
3311+
name: '',
3312+
relativePath: '',
3313+
startLine: 0,
3314+
endLine: 0,
3315+
path: '/test/rule2.json',
3316+
},
3317+
]
3318+
3319+
const toolUse = {
3320+
toolUseId: 'test-id',
3321+
name: 'codeReview',
3322+
input: { fileLevelArtifacts: [{ path: '/test/file.js' }] },
3323+
stop: true,
3324+
}
3325+
3326+
const runToolStub = testFeatures.agent.runTool as sinon.SinonStub
3327+
runToolStub.resolves({})
3328+
3329+
// Create a mock session with toolUseLookup
3330+
const mockSession = {
3331+
toolUseLookup: new Map(),
3332+
pairProgrammingMode: true,
3333+
} as any
3334+
3335+
// Create a minimal mock of AgenticChatResultStream
3336+
const mockChatResultStream = {
3337+
removeResultBlockAndUpdateUI: sinon.stub().resolves(),
3338+
writeResultBlock: sinon.stub().resolves(1),
3339+
overwriteResultBlock: sinon.stub().resolves(),
3340+
removeResultBlock: sinon.stub().resolves(),
3341+
getMessageBlockId: sinon.stub().returns(undefined),
3342+
hasMessage: sinon.stub().returns(false),
3343+
updateOngoingProgressResult: sinon.stub().resolves(),
3344+
getResult: sinon.stub().returns({ messageId: 'test', body: '' }),
3345+
setMessageIdToUpdateForTool: sinon.stub(),
3346+
getMessageIdToUpdateForTool: sinon.stub().returns(undefined),
3347+
addMessageOperation: sinon.stub(),
3348+
getMessageOperation: sinon.stub().returns(undefined),
3349+
}
3350+
3351+
// Call processToolUses directly
3352+
await chatController.processToolUses(
3353+
[toolUse],
3354+
mockChatResultStream as any,
3355+
mockSession,
3356+
'tabId',
3357+
mockCancellationToken,
3358+
mockAdditionalContext
3359+
)
3360+
3361+
// Verify runTool was called with ruleArtifacts
3362+
sinon.assert.calledOnce(runToolStub)
3363+
const toolInput = runToolStub.firstCall.args[1]
3364+
assert.ok(toolInput.ruleArtifacts)
3365+
assert.strictEqual(toolInput.ruleArtifacts.length, 2)
3366+
assert.strictEqual(toolInput.ruleArtifacts[0].path, '/test/rule1.json')
3367+
assert.strictEqual(toolInput.ruleArtifacts[1].path, '/test/rule2.json')
3368+
})
3369+
})
32863370
})
32873371

32883372
// The body may include text-based progress updates from tool invocations.

server/aws-lsp-codewhisperer/src/language-server/agenticChat/agenticChatController.ts

Lines changed: 16 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1387,7 +1387,14 @@ export class AgenticChatController implements ChatHandlers {
13871387
session.setConversationType('AgenticChatWithToolUse')
13881388
if (result.success) {
13891389
// Process tool uses and update the request input for the next iteration
1390-
toolResults = await this.#processToolUses(pendingToolUses, chatResultStream, session, tabId, token)
1390+
toolResults = await this.processToolUses(
1391+
pendingToolUses,
1392+
chatResultStream,
1393+
session,
1394+
tabId,
1395+
token,
1396+
additionalContext
1397+
)
13911398
if (toolResults.some(toolResult => this.#shouldSendBackErrorContent(toolResult))) {
13921399
content = 'There was an error processing one or more tool uses. Try again, do not apologize.'
13931400
shouldDisplayMessage = false
@@ -1662,12 +1669,13 @@ export class AgenticChatController implements ChatHandlers {
16621669
/**
16631670
* Processes tool uses by running the tools and collecting results
16641671
*/
1665-
async #processToolUses(
1672+
async processToolUses(
16661673
toolUses: Array<ToolUse & { stop: boolean }>,
16671674
chatResultStream: AgenticChatResultStream,
16681675
session: ChatSessionService,
16691676
tabId: string,
1670-
token?: CancellationToken
1677+
token?: CancellationToken,
1678+
additionalContext?: AdditionalContentEntryAddition[]
16711679
): Promise<ToolResult[]> {
16721680
const results: ToolResult[] = []
16731681

@@ -1868,12 +1876,11 @@ export class AgenticChatController implements ChatHandlers {
18681876
if (toolUse.name === CodeReview.toolName) {
18691877
try {
18701878
let initialInput = JSON.parse(JSON.stringify(toolUse.input))
1871-
let ruleArtifacts = await this.#additionalContextProvider.collectWorkspaceRules(tabId)
1872-
if (ruleArtifacts !== undefined || ruleArtifacts !== null) {
1873-
this.#features.logging.info(`RuleArtifacts: ${JSON.stringify(ruleArtifacts)}`)
1874-
let pathsToRulesMap = ruleArtifacts.map(ruleArtifact => ({ path: ruleArtifact.id }))
1875-
this.#features.logging.info(`PathsToRules: ${JSON.stringify(pathsToRulesMap)}`)
1876-
initialInput['ruleArtifacts'] = pathsToRulesMap
1879+
1880+
if (additionalContext !== undefined) {
1881+
initialInput['ruleArtifacts'] = additionalContext
1882+
.filter(c => c.type === 'rule')
1883+
.map(c => ({ path: c.path }))
18771884
}
18781885
toolUse.input = initialInput
18791886
} catch (e) {

server/aws-lsp-codewhisperer/src/language-server/agenticChat/tools/qCodeAnalysis/codeReview.test.ts

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -385,6 +385,34 @@ describe('CodeReview', () => {
385385
expect(error.message).to.include('There are no valid files to scan')
386386
}
387387
})
388+
389+
it('should handle duplicate rule filenames with unique UUIDs', async () => {
390+
const fileArtifacts = [{ path: '/test/file.js' }]
391+
const folderArtifacts: any[] = []
392+
const ruleArtifacts = [{ path: '/test/path1/rule.json' }, { path: '/test/path2/rule.json' }]
393+
394+
const mockZip = {
395+
file: sandbox.stub(),
396+
generateAsync: sandbox.stub().resolves(Buffer.from('test')),
397+
}
398+
sandbox.stub(JSZip.prototype, 'file').callsFake(mockZip.file)
399+
sandbox.stub(JSZip.prototype, 'generateAsync').callsFake(mockZip.generateAsync)
400+
sandbox.stub(CodeReviewUtils, 'countZipFiles').returns(3)
401+
sandbox.stub(require('crypto'), 'randomUUID').returns('test-uuid-123')
402+
403+
await (codeReview as any).prepareFilesAndFoldersForUpload(
404+
fileArtifacts,
405+
folderArtifacts,
406+
ruleArtifacts,
407+
false
408+
)
409+
410+
// Verify first file uses original name
411+
expect(mockZip.file.firstCall.args[0]).to.include('/test/file.js')
412+
expect(mockZip.file.secondCall.args[0]).to.include('rule.json')
413+
// Verify second file gets UUID suffix
414+
expect(mockZip.file.thirdCall.args[0]).to.include('rule_test-uuid-123.json')
415+
})
388416
})
389417

390418
describe('collectFindings', () => {

server/aws-lsp-codewhisperer/src/language-server/agenticChat/tools/qCodeAnalysis/codeReview.ts

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -716,6 +716,7 @@ export class CodeReview {
716716
* @param customerCodeZip JSZip instance for the customer code
717717
*/
718718
private async processRuleArtifacts(ruleArtifacts: RuleArtifacts, customerCodeZip: JSZip): Promise<void> {
719+
let ruleNameSet = new Set<string>()
719720
for (const artifact of ruleArtifacts) {
720721
await CodeReviewUtils.withErrorHandling(
721722
async () => {
@@ -725,6 +726,10 @@ export class CodeReview {
725726
!CodeReviewUtils.shouldSkipFile(fileName) &&
726727
existsSync(artifact.path)
727728
) {
729+
if (ruleNameSet.has(fileName)) {
730+
fileName = fileName.split('.')[0] + '_' + crypto.randomUUID() + '.' + fileName.split('.')[1]
731+
}
732+
ruleNameSet.add(fileName)
728733
const fileContent = await this.workspace.fs.readFile(artifact.path)
729734
customerCodeZip.file(
730735
`${CodeReview.CUSTOMER_CODE_BASE_PATH}/${CodeReview.RULE_ARTIFACT_PATH}/${fileName}`,

0 commit comments

Comments
 (0)