Skip to content

Commit 2b90cf9

Browse files
xgopilotphantom5099
andcommitted
fix: resolve residual multimodal/session/provider review risks
Generated with [codeagent](https://github.com/qbox/codeagent) Co-authored-by: phantom5099 <245659304+phantom5099@users.noreply.github.com>
1 parent a2d685e commit 2b90cf9

8 files changed

Lines changed: 232 additions & 25 deletions

File tree

internal/provider/openaicompat/chatcompletions/provider_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -123,8 +123,8 @@ func TestNewAndBuildRequest(t *testing.T) {
123123
if downgradedSchema["type"] != "object" {
124124
t.Fatalf("expected downgraded schema type object, got %+v", downgradedSchema["type"])
125125
}
126-
if downgradedSchema["x-neocode-schema-downgraded"] != true {
127-
t.Fatalf("expected downgrade marker, got %+v", downgradedSchema)
126+
if _, ok := downgradedSchema["x-neocode-schema-downgraded"]; ok {
127+
t.Fatalf("expected no custom downgrade marker in outbound schema, got %+v", downgradedSchema)
128128
}
129129

130130
withSessionAsset, err := BuildRequest(context.Background(), testCfg("https://api.example.com/v1", "gpt-4.1", "test-key"), providertypes.GenerateRequest{

internal/provider/openaicompat/chatcompletions/request.go

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -91,14 +91,10 @@ func normalizeToolSchemaForOpenAI(schema map[string]any) map[string]any {
9191
typeName, _ := normalized["type"].(string)
9292
if strings.TrimSpace(strings.ToLower(typeName)) != "object" {
9393
normalized["type"] = "object"
94-
normalized["x-neocode-schema-downgraded"] = true
9594
}
9695

9796
if _, ok := normalized["properties"].(map[string]any); !ok {
9897
normalized["properties"] = map[string]any{}
99-
if strings.TrimSpace(strings.ToLower(typeName)) != "object" {
100-
normalized["x-neocode-schema-downgraded"] = true
101-
}
10298
}
10399
return normalized
104100
}

internal/session/asset_store_test.go

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -224,6 +224,31 @@ func TestJSONStoreOpenAndStatMissingStoredFiles(t *testing.T) {
224224
}
225225
}
226226

227+
func TestJSONStoreDeleteAsset(t *testing.T) {
228+
t.Parallel()
229+
230+
store := NewJSONStore(t.TempDir(), t.TempDir())
231+
sessionID := "session-delete-asset"
232+
meta, err := store.SaveAsset(context.Background(), sessionID, strings.NewReader("img"), "image/png")
233+
if err != nil {
234+
t.Fatalf("save seed asset: %v", err)
235+
}
236+
237+
if err := store.DeleteAsset(context.Background(), sessionID, meta.ID); err != nil {
238+
t.Fatalf("DeleteAsset() error = %v", err)
239+
}
240+
if _, statErr := os.Stat(store.assetPath(sessionID, meta.ID)); !errors.Is(statErr, os.ErrNotExist) {
241+
t.Fatalf("expected removed asset file, got %v", statErr)
242+
}
243+
if _, statErr := os.Stat(store.assetMetaPath(sessionID, meta.ID)); !errors.Is(statErr, os.ErrNotExist) {
244+
t.Fatalf("expected removed asset meta file, got %v", statErr)
245+
}
246+
247+
if err := store.DeleteAsset(context.Background(), sessionID, meta.ID); err != nil {
248+
t.Fatalf("DeleteAsset() should ignore already deleted files, got %v", err)
249+
}
250+
}
251+
227252
type failingReader struct{}
228253

229254
func (failingReader) Read(_ []byte) (int, error) {

internal/session/input_preparer.go

Lines changed: 59 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,10 @@ type InputPreparer struct {
7070
assetStore AssetStore
7171
}
7272

73+
type assetCleanupStore interface {
74+
DeleteAsset(ctx context.Context, sessionID string, assetID string) error
75+
}
76+
7377
// NewInputPreparer 创建会话输入归一化组件。
7478
func NewInputPreparer(store Store, assetStore AssetStore) *InputPreparer {
7579
return &InputPreparer{
@@ -96,7 +100,7 @@ func (p *InputPreparer) Prepare(ctx context.Context, input PrepareInput) (Prepar
96100
}
97101

98102
sessionTitle := buildSessionTitle(trimmedText, len(input.Images) > 0)
99-
session, sessionCreated, err := p.loadOrCreateSession(
103+
session, sessionCreated, pendingUpdate, err := p.loadOrCreateSession(
100104
ctx,
101105
input.SessionID,
102106
sessionTitle,
@@ -117,6 +121,7 @@ func (p *InputPreparer) Prepare(ctx context.Context, input PrepareInput) (Prepar
117121
path := strings.TrimSpace(image.Path)
118122
if path == "" {
119123
p.rollbackCreatedSession(ctx, session.ID, sessionCreated)
124+
p.cleanupSavedAssets(ctx, session.ID, savedAssets)
120125
return PreparedInput{}, &AssetSaveError{
121126
SessionID: session.ID,
122127
Index: index,
@@ -129,6 +134,7 @@ func (p *InputPreparer) Prepare(ctx context.Context, input PrepareInput) (Prepar
129134
meta, err := p.saveImageAsset(ctx, session.ID, session.Workdir, path, mimeType)
130135
if err != nil {
131136
p.rollbackCreatedSession(ctx, session.ID, sessionCreated)
137+
p.cleanupSavedAssets(ctx, session.ID, savedAssets)
132138
return PreparedInput{}, &AssetSaveError{
133139
SessionID: session.ID,
134140
Index: index,
@@ -142,8 +148,14 @@ func (p *InputPreparer) Prepare(ctx context.Context, input PrepareInput) (Prepar
142148

143149
if err := providertypes.ValidateParts(parts); err != nil {
144150
p.rollbackCreatedSession(ctx, session.ID, sessionCreated)
151+
p.cleanupSavedAssets(ctx, session.ID, savedAssets)
145152
return PreparedInput{}, fmt.Errorf("session: normalize parts: %w", err)
146153
}
154+
if err := p.persistSessionWorkdirUpdate(ctx, pendingUpdate); err != nil {
155+
p.rollbackCreatedSession(ctx, session.ID, sessionCreated)
156+
p.cleanupSavedAssets(ctx, session.ID, savedAssets)
157+
return PreparedInput{}, err
158+
}
147159

148160
return PreparedInput{
149161
SessionID: session.ID,
@@ -278,47 +290,53 @@ func resolveImagePath(workdir string, path string) (string, error) {
278290
return resolved, nil
279291
}
280292

293+
// sessionWorkdirUpdate 描述已有会话 workdir 的待提交变更,确保 Prepare 成功后再落盘。
294+
type sessionWorkdirUpdate struct {
295+
session Session
296+
dirty bool
297+
}
298+
281299
func (p *InputPreparer) loadOrCreateSession(
282300
ctx context.Context,
283301
sessionID string,
284302
title string,
285303
defaultWorkdir string,
286304
requestedWorkdir string,
287-
) (Session, bool, error) {
305+
) (Session, bool, sessionWorkdirUpdate, error) {
288306
if strings.TrimSpace(sessionID) == "" {
289307
sessionWorkdir, err := resolveWorkdirForInput(defaultWorkdir, "", requestedWorkdir)
290308
if err != nil {
291-
return Session{}, false, err
309+
return Session{}, false, sessionWorkdirUpdate{}, err
292310
}
293311
session := NewWithWorkdir(title, sessionWorkdir)
294312
if err := p.store.Save(ctx, &session); err != nil {
295-
return Session{}, false, err
313+
return Session{}, false, sessionWorkdirUpdate{}, err
296314
}
297-
return session, true, nil
315+
return session, true, sessionWorkdirUpdate{}, nil
298316
}
299317

300318
session, err := p.store.Load(ctx, sessionID)
301319
if err != nil {
302-
return Session{}, false, err
320+
return Session{}, false, sessionWorkdirUpdate{}, err
303321
}
304322
if strings.TrimSpace(requestedWorkdir) == "" && strings.TrimSpace(session.Workdir) != "" {
305-
return session, false, nil
323+
return session, false, sessionWorkdirUpdate{}, nil
306324
}
307325

308326
resolved, err := resolveWorkdirForInput(defaultWorkdir, session.Workdir, requestedWorkdir)
309327
if err != nil {
310-
return Session{}, false, err
328+
return Session{}, false, sessionWorkdirUpdate{}, err
311329
}
312330
if session.Workdir == resolved {
313-
return session, false, nil
331+
return session, false, sessionWorkdirUpdate{}, nil
314332
}
315333

316334
session.Workdir = resolved
317335
session.UpdatedAt = time.Now()
318-
if err := p.store.Save(ctx, &session); err != nil {
319-
return Session{}, false, err
320-
}
321-
return session, false, nil
336+
return session, false, sessionWorkdirUpdate{
337+
session: session,
338+
dirty: true,
339+
}, nil
322340
}
323341

324342
// rollbackCreatedSession 在本次 Prepare 新建会话后发生错误时回滚会话目录,避免残留孤儿会话。
@@ -332,6 +350,34 @@ func (p *InputPreparer) rollbackCreatedSession(ctx context.Context, sessionID st
332350
_ = p.store.DeleteSession(ctx, sessionID)
333351
}
334352

353+
// persistSessionWorkdirUpdate 在 Prepare 其余步骤完成后统一提交会话 workdir 更新,避免失败时出现部分提交。
354+
func (p *InputPreparer) persistSessionWorkdirUpdate(ctx context.Context, pending sessionWorkdirUpdate) error {
355+
if !pending.dirty {
356+
return nil
357+
}
358+
if err := p.store.Save(ctx, &pending.session); err != nil {
359+
return err
360+
}
361+
return nil
362+
}
363+
364+
// cleanupSavedAssets 在 Prepare 失败时尽力回收已落盘的附件,减少 existing session 残留垃圾文件。
365+
func (p *InputPreparer) cleanupSavedAssets(ctx context.Context, sessionID string, assets []AssetMeta) {
366+
if len(assets) == 0 || ctx.Err() != nil {
367+
return
368+
}
369+
cleanupStore, ok := p.assetStore.(assetCleanupStore)
370+
if !ok {
371+
return
372+
}
373+
for _, asset := range assets {
374+
if strings.TrimSpace(asset.ID) == "" {
375+
continue
376+
}
377+
_ = cleanupStore.DeleteAsset(ctx, sessionID, asset.ID)
378+
}
379+
}
380+
335381
func resolveWorkdirForInput(defaultWorkdir string, currentWorkdir string, requestedWorkdir string) (string, error) {
336382
base := EffectiveWorkdir(currentWorkdir, defaultWorkdir)
337383
if strings.TrimSpace(requestedWorkdir) == "" {

internal/session/input_preparer_test.go

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -237,6 +237,76 @@ func TestInputPreparerPrepareErrors(t *testing.T) {
237237
t.Fatalf("expected existing session to remain, load error = %v", loadErr)
238238
}
239239
})
240+
241+
t.Run("existing session cleanup removes previously saved assets on later failure", func(t *testing.T) {
242+
existing := NewWithWorkdir("existing-cleanup", workdir)
243+
if err := store.Save(context.Background(), &existing); err != nil {
244+
t.Fatalf("Save() error = %v", err)
245+
}
246+
247+
okImage := filepath.Join(workdir, "ok.png")
248+
if err := os.WriteFile(okImage, minimalPNGBytes(), 0o644); err != nil {
249+
t.Fatalf("write image: %v", err)
250+
}
251+
252+
preparer := NewInputPreparer(store, store)
253+
_, err := preparer.Prepare(context.Background(), PrepareInput{
254+
SessionID: existing.ID,
255+
Text: "cleanup",
256+
Images: []PrepareImageInput{
257+
{Path: okImage},
258+
{Path: "not-found.png", MimeType: "image/png"},
259+
},
260+
DefaultWorkdir: workdir,
261+
})
262+
if err == nil {
263+
t.Fatalf("expected prepare error")
264+
}
265+
266+
entries, readErr := os.ReadDir(store.assetsDir(existing.ID))
267+
if readErr != nil {
268+
t.Fatalf("ReadDir() error = %v", readErr)
269+
}
270+
if len(entries) != 0 {
271+
t.Fatalf("expected no leftover assets, got %d files", len(entries))
272+
}
273+
})
274+
275+
t.Run("existing session workdir change is not persisted when prepare fails", func(t *testing.T) {
276+
currentWorkdir := filepath.Join(workdir, "current")
277+
if err := os.MkdirAll(currentWorkdir, 0o755); err != nil {
278+
t.Fatalf("mkdir current workdir: %v", err)
279+
}
280+
targetWorkdir := filepath.Join(currentWorkdir, "nested")
281+
if err := os.MkdirAll(targetWorkdir, 0o755); err != nil {
282+
t.Fatalf("mkdir nested workdir: %v", err)
283+
}
284+
285+
existing := NewWithWorkdir("existing-workdir", currentWorkdir)
286+
if err := store.Save(context.Background(), &existing); err != nil {
287+
t.Fatalf("Save() error = %v", err)
288+
}
289+
290+
preparer := NewInputPreparer(store, store)
291+
_, err := preparer.Prepare(context.Background(), PrepareInput{
292+
SessionID: existing.ID,
293+
Text: "will fail",
294+
RequestedWorkdir: "nested",
295+
Images: []PrepareImageInput{{Path: "not-found.png", MimeType: "image/png"}},
296+
DefaultWorkdir: workdir,
297+
})
298+
if err == nil {
299+
t.Fatalf("expected prepare error")
300+
}
301+
302+
loaded, loadErr := store.Load(context.Background(), existing.ID)
303+
if loadErr != nil {
304+
t.Fatalf("Load() error = %v", loadErr)
305+
}
306+
if loaded.Workdir != currentWorkdir {
307+
t.Fatalf("expected workdir to stay %q, got %q", currentWorkdir, loaded.Workdir)
308+
}
309+
})
240310
}
241311

242312
func TestInputPreparerPrepareImagePathAndMimeValidation(t *testing.T) {

internal/session/store.go

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -399,6 +399,39 @@ func (s *JSONStore) Stat(ctx context.Context, sessionID string, assetID string)
399399
return s.statUnlocked(sessionID, assetID)
400400
}
401401

402+
// DeleteAsset 删除指定会话附件的二进制与元数据文件,用于输入归一化失败后的清理。
403+
func (s *JSONStore) DeleteAsset(ctx context.Context, sessionID string, assetID string) error {
404+
if err := ctx.Err(); err != nil {
405+
return err
406+
}
407+
if err := validateStorageID("session id", sessionID); err != nil {
408+
return fmt.Errorf("session: %w", err)
409+
}
410+
if err := validateStorageID("asset id", assetID); err != nil {
411+
return fmt.Errorf("session: %w", err)
412+
}
413+
414+
s.mu.Lock()
415+
defer s.mu.Unlock()
416+
417+
target := s.assetPath(sessionID, assetID)
418+
if err := ensurePathWithinBase(s.baseDir, target); err != nil {
419+
return fmt.Errorf("session: resolve asset file path: %w", err)
420+
}
421+
if err := os.Remove(target); err != nil && !errors.Is(err, os.ErrNotExist) {
422+
return fmt.Errorf("session: delete asset file: %w", err)
423+
}
424+
425+
metaTarget := s.assetMetaPath(sessionID, assetID)
426+
if err := ensurePathWithinBase(s.baseDir, metaTarget); err != nil {
427+
return fmt.Errorf("session: resolve asset meta file path: %w", err)
428+
}
429+
if err := os.Remove(metaTarget); err != nil && !errors.Is(err, os.ErrNotExist) {
430+
return fmt.Errorf("session: delete asset meta file: %w", err)
431+
}
432+
return nil
433+
}
434+
402435
// statUnlocked 在调用方已持有读锁时读取附件元数据,避免重复加锁导致死锁风险。
403436
func (s *JSONStore) statUnlocked(sessionID string, assetID string) (AssetMeta, error) {
404437
target := s.assetMetaPath(sessionID, assetID)

internal/tui/core/app/update.go

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -966,9 +966,6 @@ func (a *App) handleRuntimeEvent(event agentruntime.RuntimeEvent) bool {
966966
if !a.shouldHandleRuntimeEvent(event) {
967967
return false
968968
}
969-
if a.state.ActiveSessionID == "" {
970-
a.state.ActiveSessionID = event.SessionID
971-
}
972969
handler, ok := runtimeEventHandlerRegistry[event.Type]
973970
if !ok {
974971
return false
@@ -1051,6 +1048,9 @@ func runtimeEventUserMessageHandler(a *App, event agentruntime.RuntimeEvent) boo
10511048
if runID != "" {
10521049
a.state.ActiveRunID = runID
10531050
}
1051+
if sessionID := strings.TrimSpace(event.SessionID); sessionID != "" {
1052+
a.state.ActiveSessionID = sessionID
1053+
}
10541054
a.state.StatusText = statusThinking
10551055
a.state.StreamingReply = false
10561056
a.state.CurrentTool = ""
@@ -1085,6 +1085,9 @@ func runtimeEventRunContextHandler(a *App, event agentruntime.RuntimeEvent) bool
10851085
}
10861086
mapped := tuiservices.MapRunContextPayload(event.RunID, event.SessionID, payload)
10871087
a.state.RunContext = mapped
1088+
if strings.TrimSpace(mapped.SessionID) != "" {
1089+
a.state.ActiveSessionID = strings.TrimSpace(mapped.SessionID)
1090+
}
10881091
if strings.TrimSpace(mapped.RunID) != "" {
10891092
a.state.ActiveRunID = mapped.RunID
10901093
}

0 commit comments

Comments
 (0)