mirror of
https://github.com/AgentSeal/codeburn.git
synced 2026-08-12 10:04:49 +00:00
fix: address PR review feedback on Copilot provider
- init currentModel to '' and skip assistant messages before first
session.model_change to avoid silent misattribution
- add comment documenting why inputTokens is always 0
- fix delete_file tool mapping ('Edit' -> 'Delete')
- add schema doc comment to ToolRequest optional fields
- remove catch-all from CopilotEvent union for proper TS narrowing
- add tests: pre-model-change skip, workspace.yaml quote/comment strip,
longest-prefix model display name match
This commit is contained in:
parent
a8517d3235
commit
e7633d932b
2 changed files with 55 additions and 4 deletions
|
|
@ -27,7 +27,7 @@ const toolNameMap: Record<string, string> = {
|
|||
write_file: 'Edit',
|
||||
edit_file: 'Edit',
|
||||
create_file: 'Write',
|
||||
delete_file: 'Edit',
|
||||
delete_file: 'Delete',
|
||||
search_files: 'Grep',
|
||||
find_files: 'Glob',
|
||||
list_directory: 'LS',
|
||||
|
|
@ -39,6 +39,7 @@ const toolNameMap: Record<string, string> = {
|
|||
// Pre-sorted by key length descending so longer/more-specific keys match first
|
||||
const modelDisplayEntries = Object.entries(modelDisplayNames).sort((a, b) => b[0].length - a[0].length)
|
||||
|
||||
// Fields marked optional document the on-disk schema; they are not read by the parser
|
||||
type ToolRequest = {
|
||||
name?: string
|
||||
toolCallId?: string
|
||||
|
|
@ -71,6 +72,16 @@ function getCopilotSessionStateDir(override?: string): string {
|
|||
return override ?? join(homedir(), '.copilot', 'session-state')
|
||||
}
|
||||
|
||||
function parseCwd(yaml: string): string | null {
|
||||
const match = yaml.match(/^cwd:\s*(.+)$/m)
|
||||
if (!match?.[1]) return null
|
||||
const raw = match[1]
|
||||
.replace(/\s*#.*$/, '') // strip trailing comment
|
||||
.replace(/^['"]|['"]$/g, '') // strip surrounding quotes
|
||||
.trim()
|
||||
return raw || null
|
||||
}
|
||||
|
||||
function createParser(source: SessionSource, seenKeys: Set<string>): SessionParser {
|
||||
return {
|
||||
async *parse(): AsyncGenerator<ParsedProviderCall> {
|
||||
|
|
@ -83,7 +94,7 @@ function createParser(source: SessionSource, seenKeys: Set<string>): SessionPars
|
|||
|
||||
const sessionId = basename(dirname(source.path))
|
||||
const lines = content.split('\n').filter(l => l.trim())
|
||||
let currentModel = 'gpt-4.1'
|
||||
let currentModel = ''
|
||||
let pendingUserMessage = ''
|
||||
|
||||
for (const line of lines) {
|
||||
|
|
@ -107,6 +118,8 @@ function createParser(source: SessionSource, seenKeys: Set<string>): SessionPars
|
|||
if (event.type === 'assistant.message') {
|
||||
const { messageId, outputTokens, toolRequests = [] } = event.data
|
||||
if (outputTokens === 0) continue
|
||||
// Skip if no model has been identified yet - avoids silent misattribution
|
||||
if (!currentModel) continue
|
||||
|
||||
const dedupKey = `copilot:${sessionId}:${messageId}`
|
||||
if (seenKeys.has(dedupKey)) continue
|
||||
|
|
@ -117,6 +130,8 @@ function createParser(source: SessionSource, seenKeys: Set<string>): SessionPars
|
|||
.filter(Boolean)
|
||||
.map(n => toolNameMap[n] ?? n)
|
||||
|
||||
// Copilot only logs outputTokens; inputTokens are not available in session logs.
|
||||
// Cost will be lower than actual API cost.
|
||||
const costUSD = calculateCost(currentModel, 0, outputTokens, 0, 0, 0)
|
||||
|
||||
yield {
|
||||
|
|
@ -164,8 +179,8 @@ async function discoverSessionsInDir(sessionStateDir: string): Promise<SessionSo
|
|||
let project = sessionId
|
||||
try {
|
||||
const yaml = await readFile(join(sessionStateDir, sessionId, 'workspace.yaml'), 'utf-8')
|
||||
const cwdMatch = yaml.match(/^cwd:\s*(.+)$/m)
|
||||
if (cwdMatch?.[1]) project = basename(cwdMatch[1].trim())
|
||||
const cwd = parseCwd(yaml)
|
||||
if (cwd) project = basename(cwd)
|
||||
} catch {}
|
||||
|
||||
sources.push({ path: eventsPath, project, provider: 'copilot' })
|
||||
|
|
|
|||
|
|
@ -142,6 +142,23 @@ describe('copilot provider - JSONL parsing', () => {
|
|||
for await (const call of copilot.createSessionParser(source, new Set()).parse()) calls.push(call)
|
||||
expect(calls).toHaveLength(0)
|
||||
})
|
||||
|
||||
it('skips assistant messages before the first model_change event', async () => {
|
||||
const eventsPath = await createSessionDir('sess-no-model', [
|
||||
assistantMessage({ messageId: 'msg-early', outputTokens: 50 }),
|
||||
modelChange('gpt-4.1'),
|
||||
assistantMessage({ messageId: 'msg-after', outputTokens: 80 }),
|
||||
])
|
||||
|
||||
const source = { path: eventsPath, project: 'test', provider: 'copilot' }
|
||||
const calls: ParsedProviderCall[] = []
|
||||
for await (const call of copilot.createSessionParser(source, new Set()).parse()) calls.push(call)
|
||||
|
||||
expect(calls).toHaveLength(1)
|
||||
expect(calls[0]!.messageId).toBeUndefined()
|
||||
expect(calls[0]!.outputTokens).toBe(80)
|
||||
expect(calls[0]!.model).toBe('gpt-4.1')
|
||||
})
|
||||
})
|
||||
|
||||
describe('copilot provider - discoverSessions', () => {
|
||||
|
|
@ -175,6 +192,19 @@ describe('copilot provider - discoverSessions', () => {
|
|||
expect(sessions[0]!.project).toBe('myapp')
|
||||
})
|
||||
|
||||
it('strips quotes and trailing comments from workspace.yaml cwd', async () => {
|
||||
const sessionDir = join(tmpDir, 'sess-quoted')
|
||||
await mkdir(sessionDir, { recursive: true })
|
||||
await writeFile(join(sessionDir, 'workspace.yaml'), 'cwd: "/home/user/myapp" # project root\n')
|
||||
await writeFile(join(sessionDir, 'events.jsonl'), '\n')
|
||||
|
||||
const provider = createCopilotProvider(tmpDir)
|
||||
const sessions = await provider.discoverSessions()
|
||||
|
||||
expect(sessions).toHaveLength(1)
|
||||
expect(sessions[0]!.project).toBe('myapp')
|
||||
})
|
||||
|
||||
it('returns empty when directory does not exist', async () => {
|
||||
const provider = createCopilotProvider('/nonexistent/path')
|
||||
const sessions = await provider.discoverSessions()
|
||||
|
|
@ -214,4 +244,10 @@ describe('copilot provider - metadata', () => {
|
|||
expect(copilot.modelDisplayName('o4-mini')).toBe('o4-mini')
|
||||
expect(copilot.modelDisplayName('unknown-model-xyz')).toBe('unknown-model-xyz')
|
||||
})
|
||||
|
||||
it('longest-prefix match wins for versioned model IDs', () => {
|
||||
// gpt-5-mini-2026-01-01 must match gpt-5-mini, not gpt-5
|
||||
expect(copilot.modelDisplayName('gpt-5-mini-2026-01-01')).toBe('GPT-5 Mini')
|
||||
expect(copilot.modelDisplayName('gpt-4.1-mini-2026-01-01')).toBe('GPT-4.1 Mini')
|
||||
})
|
||||
})
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue