Skip to content

Commit 8590c34

Browse files
neo-gptneo-gpt
authored
fix(ai): retry transient GitHub identity resolution (#14600) (#14825)
1 parent 53836e7 commit 8590c34

2 files changed

Lines changed: 134 additions & 59 deletions

File tree

‎ai/mcp/server/github-workflow/toolService.mjs‎

Lines changed: 57 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1,20 +1,20 @@
1-
import {execFile} from 'child_process';
2-
import path from 'path';
3-
import {fileURLToPath} from 'url';
4-
import {promisify} from 'util';
5-
import AgentStateService from '../../../services/github-workflow/AgentStateService.mjs';
6-
import HealthService from '../../../services/github-workflow/HealthService.mjs';
7-
import IssueService from '../../../services/github-workflow/IssueService.mjs';
8-
import DiscussionService from '../../../services/github-workflow/DiscussionService.mjs';
9-
import LabelService from '../../../services/github-workflow/LabelService.mjs';
10-
import LocalFileService from '../../../services/github-workflow/LocalFileService.mjs';
11-
import PullRequestService from '../../../services/github-workflow/PullRequestService.mjs';
12-
import RepositoryService from '../../../services/github-workflow/RepositoryService.mjs';
13-
import ToolService from '../../ToolService.mjs';
14-
import SyncService from '../../../services/github-workflow/SyncService.mjs';
1+
import {execFile} from 'child_process';
2+
import path from 'path';
3+
import {fileURLToPath} from 'url';
4+
import {promisify} from 'util';
5+
import AgentStateService from '../../../services/github-workflow/AgentStateService.mjs';
6+
import HealthService from '../../../services/github-workflow/HealthService.mjs';
7+
import IssueService from '../../../services/github-workflow/IssueService.mjs';
8+
import DiscussionService from '../../../services/github-workflow/DiscussionService.mjs';
9+
import LabelService from '../../../services/github-workflow/LabelService.mjs';
10+
import LocalFileService from '../../../services/github-workflow/LocalFileService.mjs';
11+
import PullRequestService from '../../../services/github-workflow/PullRequestService.mjs';
12+
import RepositoryService from '../../../services/github-workflow/RepositoryService.mjs';
13+
import ToolService from '../../ToolService.mjs';
14+
import SyncService from '../../../services/github-workflow/SyncService.mjs';
1515
import {assertExpectedIdentity as assertExpectedGitHubIdentity, IdentityAssertionCode} from '../../../graph/assertExpectedIdentity.mjs';
16-
import RequestContextService from '../shared/services/RequestContextService.mjs';
17-
import config from './config.mjs';
16+
import RequestContextService from '../shared/services/RequestContextService.mjs';
17+
import config from './config.mjs';
1818

1919
const execFileAsync = promisify(execFile);
2020
const __filename = fileURLToPath(import.meta.url);
@@ -160,6 +160,25 @@ function getGitHubIdentityErrorCode(code) {
160160
}
161161
}
162162

163+
/**
164+
* @summary Groups write-boundary identity failures into operator-actionable classes.
165+
* @param {String} code The shared assertion's `code`.
166+
* @returns {String}
167+
*/
168+
function getGitHubIdentityErrorClass(code) {
169+
switch (code) {
170+
case IdentityAssertionCode.NO_AUTHED_LOGIN:
171+
return 'identity-resolution-transient';
172+
case IdentityAssertionCode.LOGIN_MISMATCH:
173+
case IdentityAssertionCode.MEMORY_CORE_MISMATCH:
174+
return 'identity-mismatch';
175+
case IdentityAssertionCode.EXPECTED_UNMAPPABLE:
176+
return 'identity-configuration';
177+
default:
178+
return 'identity-assertion';
179+
}
180+
}
181+
163182
/**
164183
* @summary Builds a structured identity guard error for GitHub write-boundary failures.
165184
* @param {Object} assertion Shared identity assertion failure payload.
@@ -171,7 +190,8 @@ function createGitHubIdentityError(assertion) {
171190

172191
Object.assign(error, {
173192
...assertion,
174-
code: getGitHubIdentityErrorCode(assertion.code)
193+
code : getGitHubIdentityErrorCode(assertion.code),
194+
identityClass: getGitHubIdentityErrorClass(assertion.code)
175195
});
176196

177197
return error;
@@ -211,6 +231,15 @@ async function defaultGitHubIdentityAssertion() {
211231
});
212232
}
213233

234+
/**
235+
* @summary Returns true when an identity assertion is the retryable empty-login resolution class.
236+
* @param {Object} assertion Shared identity assertion payload.
237+
* @returns {Boolean}
238+
*/
239+
function shouldRetryGitHubIdentityAssertion(assertion) {
240+
return assertion?.code === IdentityAssertionCode.NO_AUTHED_LOGIN;
241+
}
242+
214243
/**
215244
* @summary Wraps a public GitHub write with fail-closed identity-drift validation.
216245
*
@@ -221,13 +250,19 @@ async function defaultGitHubIdentityAssertion() {
221250
* @param {Function} delegate The mutating GitHub tool handler.
222251
* @param {Object} [options]
223252
* @param {Function} [options.assertExpectedIdentity] Shared identity assertion seam.
253+
* @param {Number} [options.identityResolutionRetries=1] Number of retry attempts for transient empty-login resolution.
224254
* @returns {Function} Guarded async tool handler.
225255
*/
226256
function buildGitHubWriteIdentityGuard(delegate, {
227-
assertExpectedIdentity = defaultGitHubIdentityAssertion
257+
assertExpectedIdentity = defaultGitHubIdentityAssertion,
258+
identityResolutionRetries = 1
228259
} = {}) {
229260
return async function githubWriteIdentityGuard(...args) {
230-
const assertion = await assertExpectedIdentity();
261+
let assertion = await assertExpectedIdentity();
262+
263+
for (let retry = 0; !assertion.ok && retry < identityResolutionRetries && shouldRetryGitHubIdentityAssertion(assertion); retry++) {
264+
assertion = await assertExpectedIdentity();
265+
}
231266

232267
if (!assertion.ok) {
233268
throw createGitHubIdentityError(assertion);
@@ -282,7 +317,7 @@ async function defaultBranchDetector() {
282317
}
283318

284319
const normalizedProjectRoot = path.resolve(config.projectRoot);
285-
const normalizedToplevel = path.resolve(toplevel);
320+
const normalizedToplevel = path.resolve(toplevel);
286321

287322
if (normalizedProjectRoot !== normalizedToplevel) {
288323
throw new Error(
@@ -436,9 +471,9 @@ export {
436471
};
437472

438473
const toolService = Neo.create(ToolService, {
439-
compactToolDescriptions : true,
474+
compactToolDescriptions : true,
440475
openApiFilePath,
441-
serviceMapping: guardedServiceMapping,
476+
serviceMapping : guardedServiceMapping,
442477
toolListDescriptionMaxLength: 120
443478
});
444479

‎test/playwright/unit/ai/services/github-workflow/toolService.spec.mjs‎

Lines changed: 77 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -46,8 +46,8 @@ test.describe('Neo.ai.services.github-workflow.toolService — sync_all dev-bran
4646
});
4747

4848
test('sync_all delegates to SyncService.runFullSync when on dev', async () => {
49-
let delegateCalls = 0;
50-
const delegate = async (...args) => {
49+
let delegateCalls = 0;
50+
const delegate = async (...args) => {
5151
delegateCalls++;
5252
return {message: 'sync ok', args};
5353
};
@@ -60,33 +60,33 @@ test.describe('Neo.ai.services.github-workflow.toolService — sync_all dev-bran
6060
});
6161

6262
test('sync_all REJECTS when on a feature branch (no delegate call)', async () => {
63-
let delegateCalls = 0;
64-
const delegate = async () => { delegateCalls++; return {message: 'should not run'}; };
65-
const guarded = buildDevBranchGuard(delegate, async () => 'agent/some-feature-branch');
63+
let delegateCalls = 0;
64+
const delegate = async () => { delegateCalls++; return {message: 'should not run'}; };
65+
const guarded = buildDevBranchGuard(delegate, async () => 'agent/some-feature-branch');
6666

6767
await expect(guarded()).rejects.toThrow(/sync_all REJECTED.*'agent\/some-feature-branch'.*not 'dev'/);
6868
expect(delegateCalls).toBe(0);
6969
});
7070

7171
test('sync_all REJECTS when on main (no delegate call)', async () => {
72-
let delegateCalls = 0;
73-
const delegate = async () => { delegateCalls++; };
74-
const guarded = buildDevBranchGuard(delegate, async () => 'main');
72+
let delegateCalls = 0;
73+
const delegate = async () => { delegateCalls++; };
74+
const guarded = buildDevBranchGuard(delegate, async () => 'main');
7575

7676
await expect(guarded()).rejects.toThrow(/sync_all REJECTED.*'main'.*not 'dev'/);
7777
expect(delegateCalls).toBe(0);
7878
});
7979

8080
test('sync_all REJECTS on detached HEAD (empty branch name)', async () => {
8181
const delegate = async () => { throw new Error('should not run'); };
82-
const guarded = buildDevBranchGuard(delegate, async () => '');
82+
const guarded = buildDevBranchGuard(delegate, async () => '');
8383

8484
await expect(guarded()).rejects.toThrow(/sync_all REJECTED.*'\(detached\)'/);
8585
});
8686

8787
test('sync_all REJECTS immediately on root mismatch from branch detector', async () => {
8888
const delegate = async () => { throw new Error('should not run'); };
89-
const guarded = buildDevBranchGuard(delegate, async () => {
89+
const guarded = buildDevBranchGuard(delegate, async () => {
9090
throw new Error('sync_all REJECTED: Root mismatch. MCP server projectRoot...');
9191
});
9292

@@ -95,7 +95,7 @@ test.describe('Neo.ai.services.github-workflow.toolService — sync_all dev-bran
9595

9696
test('sync_all REJECTS with git-error message when branch detector throws', async () => {
9797
const delegate = async () => { throw new Error('should not run'); };
98-
const guarded = buildDevBranchGuard(delegate, async () => {
98+
const guarded = buildDevBranchGuard(delegate, async () => {
9999
throw new Error('git: not a git repository');
100100
});
101101

@@ -125,7 +125,7 @@ test.describe('Neo.ai.services.github-workflow.toolService — getConversationRo
125125
let originalPrGetConversation;
126126

127127
test.beforeAll(async () => {
128-
const mod = await import('../../../../../../ai/mcp/server/github-workflow/toolService.mjs');
128+
const mod = await import('../../../../../../ai/mcp/server/github-workflow/toolService.mjs');
129129
getConversationRouter = mod.getConversationRouter;
130130
IssueService = (await import('../../../../../../ai/services/github-workflow/IssueService.mjs')).default;
131131
PullRequestService = (await import('../../../../../../ai/services/github-workflow/PullRequestService.mjs')).default;
@@ -237,8 +237,8 @@ test.describe('Neo.ai.services.github-workflow.toolService — write identity gu
237237
});
238238

239239
test('delegates a public write when expected agent and viewer login match', async () => {
240-
let delegateCalls = 0;
241-
const guarded = buildGitHubWriteIdentityGuard(async (...args) => {
240+
let delegateCalls = 0;
241+
const guarded = buildGitHubWriteIdentityGuard(async (...args) => {
242242
delegateCalls++;
243243
return {ok: true, args};
244244
}, {
@@ -256,28 +256,34 @@ test.describe('Neo.ai.services.github-workflow.toolService — write identity gu
256256
});
257257

258258
test('rejects a public write on identity mismatch before delegate invocation', async () => {
259-
let delegateCalls = 0;
260-
const guarded = buildGitHubWriteIdentityGuard(async () => {
259+
let delegateCalls = 0;
260+
let assertionCalls = 0;
261+
const guarded = buildGitHubWriteIdentityGuard(async () => {
261262
delegateCalls++;
262263
return {ok: true};
263264
}, {
264-
assertExpectedIdentity: async () => ({
265-
ok : false,
266-
reason: 'identity drift: authed as neo-opus-ada, expected neo-gpt',
267-
code : 'LOGIN_MISMATCH'
268-
})
265+
assertExpectedIdentity: async () => {
266+
assertionCalls++;
267+
return {
268+
ok : false,
269+
reason: 'identity drift: authed as neo-opus-ada, expected neo-gpt',
270+
code : 'LOGIN_MISMATCH'
271+
}
272+
}
269273
});
270274

271275
await expect(guarded()).rejects.toMatchObject({
272-
code : 'GITHUB_IDENTITY_MISMATCH',
273-
reason: 'identity drift: authed as neo-opus-ada, expected neo-gpt'
276+
code : 'GITHUB_IDENTITY_MISMATCH',
277+
identityClass: 'identity-mismatch',
278+
reason : 'identity drift: authed as neo-opus-ada, expected neo-gpt'
274279
});
275280
expect(delegateCalls).toBe(0);
281+
expect(assertionCalls).toBe(1);
276282
});
277283

278284
test('rejects a public write when expected identity is unresolved', async () => {
279-
let delegateCalls = 0;
280-
const guarded = buildGitHubWriteIdentityGuard(async () => {
285+
let delegateCalls = 0;
286+
const guarded = buildGitHubWriteIdentityGuard(async () => {
281287
delegateCalls++;
282288
}, {
283289
assertExpectedIdentity: async () => ({
@@ -293,28 +299,62 @@ test.describe('Neo.ai.services.github-workflow.toolService — write identity gu
293299
expect(delegateCalls).toBe(0);
294300
});
295301

296-
test('rejects a public write when viewer login probe fails', async () => {
297-
let delegateCalls = 0;
298-
const guarded = buildGitHubWriteIdentityGuard(async () => {
302+
test('retries transient empty-login resolution before delegating', async () => {
303+
let delegateCalls = 0;
304+
let assertionCalls = 0;
305+
const guarded = buildGitHubWriteIdentityGuard(async () => {
299306
delegateCalls++;
307+
return {ok: true};
300308
}, {
301-
assertExpectedIdentity: async () => ({
302-
ok : false,
303-
reason: 'identity drift: no authed login resolved, expected neo-gpt',
304-
code : 'NO_AUTHED_LOGIN'
305-
})
309+
assertExpectedIdentity: async () => {
310+
assertionCalls++;
311+
return assertionCalls === 1
312+
? {
313+
ok : false,
314+
reason: 'identity drift: no authed login resolved, expected neo-gpt',
315+
code : 'NO_AUTHED_LOGIN'
316+
}
317+
: {
318+
ok : true,
319+
reason: null,
320+
code : 'OK'
321+
}
322+
}
323+
});
324+
325+
await expect(guarded()).resolves.toEqual({ok: true});
326+
expect(delegateCalls).toBe(1);
327+
expect(assertionCalls).toBe(2);
328+
});
329+
330+
test('rejects a public write when viewer login probe still fails after bounded retry', async () => {
331+
let delegateCalls = 0;
332+
let assertionCalls = 0;
333+
const guarded = buildGitHubWriteIdentityGuard(async () => {
334+
delegateCalls++;
335+
}, {
336+
assertExpectedIdentity: async () => {
337+
assertionCalls++;
338+
return {
339+
ok : false,
340+
reason: 'identity drift: no authed login resolved, expected neo-gpt',
341+
code : 'NO_AUTHED_LOGIN'
342+
}
343+
}
306344
});
307345

308346
await expect(guarded()).rejects.toMatchObject({
309-
code: 'GITHUB_VIEWER_UNRESOLVED'
347+
code : 'GITHUB_VIEWER_UNRESOLVED',
348+
identityClass: 'identity-resolution-transient'
310349
});
311350
expect(delegateCalls).toBe(0);
351+
expect(assertionCalls).toBe(2);
312352
});
313353

314354
test('guards public GitHub writes but leaves read and health tools untouched', async () => {
315355
const readHandler = async () => ({read: true});
316356
const writeHandler = async () => ({write: true});
317-
const mapping = guardGitHubWriteTools({
357+
const mapping = guardGitHubWriteTools({
318358
get_conversation : readHandler,
319359
healthcheck : readHandler,
320360
manage_issue_comment: writeHandler,
@@ -346,8 +386,8 @@ test.describe('Neo.ai.services.github-workflow.toolService — write identity gu
346386

347387
test('rejects service mappings with unclassified future tools (#13252)', () => {
348388
expect(() => guardGitHubWriteTools({
349-
get_conversation : async () => {},
350-
future_public_mutation : async () => {}
389+
get_conversation : async () => {},
390+
future_public_mutation: async () => {}
351391
})).toThrow(/Missing classification: future_public_mutation/);
352392
});
353393

0 commit comments

Comments
 (0)