Skip to content

Commit 209e35f

Browse files
SteveSandersonMSCopilot
SteveSandersonMS
and
Copilot
committed
Address PR feedback: integer LastInsertRowid, optional query return, unused var fixes
- Change LastInsertRowid from float to integer type in Python (int|None), Go (*int64), and Rust (Option<i64>). Adapters convert to wire float types. - Make sqlite_query return optional in Python (| None) and Rust (Option<>), matching Node.js and .NET behavior for exec-type queries. - Fix unused msg variable in Python E2E test. - Remove unused import in Rust E2E test. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 01eb131 commit 209e35f

8 files changed

Lines changed: 58 additions & 31 deletions

File tree

‎go/internal/e2e/session_fs_sqlite_e2e_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -214,7 +214,7 @@ func (p *inMemorySqliteProvider) SqliteQuery(queryType rpc.SessionFsSqliteQueryT
214214
case rpc.SessionFsSqliteQueryTypeExec:
215215
return &copilot.SessionFsSqliteQueryResult{Columns: []string{}, Rows: []map[string]any{}}, nil
216216
case rpc.SessionFsSqliteQueryTypeRun:
217-
lastID := float64(1)
217+
lastID := int64(1)
218218
return &copilot.SessionFsSqliteQueryResult{
219219
Columns: []string{},
220220
Rows: []map[string]any{},

‎go/session_fs_provider.go‎

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ type SessionFsSqliteQueryResult struct {
6969
Columns []string `json:"columns"`
7070
Rows []map[string]any `json:"rows"`
7171
RowsAffected int64 `json:"rowsAffected"`
72-
LastInsertRowid *float64 `json:"lastInsertRowid,omitempty"`
72+
LastInsertRowid *int64 `json:"lastInsertRowid,omitempty"`
7373
}
7474

7575
// SessionFsFileInfo holds file metadata returned by SessionFsProvider.Stat.
@@ -210,11 +210,23 @@ func (a *sessionFsAdapter) SqliteQuery(request *rpc.SessionFsSqliteQueryRequest)
210210
Error: toSessionFsError(err),
211211
}, nil
212212
}
213+
if result == nil {
214+
return &rpc.SessionFsSqliteQueryResult{
215+
Columns: []string{},
216+
Rows: []map[string]any{},
217+
RowsAffected: 0,
218+
}, nil
219+
}
220+
var wireRowid *float64
221+
if result.LastInsertRowid != nil {
222+
f := float64(*result.LastInsertRowid)
223+
wireRowid = &f
224+
}
213225
return &rpc.SessionFsSqliteQueryResult{
214226
Columns: result.Columns,
215227
Rows: result.Rows,
216228
RowsAffected: result.RowsAffected,
217-
LastInsertRowid: result.LastInsertRowid,
229+
LastInsertRowid: wireRowid,
218230
}, nil
219231
}
220232

‎python/copilot/session_fs_provider.py‎

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -123,8 +123,12 @@ async def sqlite_query(
123123
query_type: SessionFSSqliteQueryType,
124124
query: str,
125125
params: dict[str, float | str | None] | None = None,
126-
) -> SessionFsSqliteQueryResult:
127-
"""Execute a SQLite query against the provider's per-session database."""
126+
) -> SessionFsSqliteQueryResult | None:
127+
"""Execute a SQLite query against the provider's per-session database.
128+
129+
Return ``None`` for exec-type queries (DDL / multi-statement) where
130+
no result set is produced; the adapter will substitute an empty result.
131+
"""
128132

129133
@abc.abstractmethod
130134
async def sqlite_exists(self) -> bool:
@@ -142,7 +146,7 @@ class SessionFsSqliteQueryResult:
142146
columns: list[str]
143147
rows: list[dict[str, Any]]
144148
rows_affected: int
145-
last_insert_rowid: float | None = None
149+
last_insert_rowid: int | None = None
146150

147151

148152
def create_session_fs_adapter(provider: SessionFsProvider) -> SessionFsHandler:
@@ -277,11 +281,15 @@ async def sqlite_query(self, params: Any) -> _GeneratedSqliteQueryResult:
277281
params.query,
278282
getattr(params, "params", None),
279283
)
284+
if result is None:
285+
return _GeneratedSqliteQueryResult(
286+
columns=[], rows=[], rows_affected=0,
287+
)
280288
return _GeneratedSqliteQueryResult(
281289
columns=result.columns,
282290
rows=result.rows,
283291
rows_affected=result.rows_affected,
284-
last_insert_rowid=result.last_insert_rowid,
292+
last_insert_rowid=float(result.last_insert_rowid) if result.last_insert_rowid is not None else None,
285293
)
286294

287295
async def sqlite_exists(self, params: Any) -> SessionFSSqliteExistsResult:

‎python/e2e/test_session_fs_sqlite_e2e.py‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -151,7 +151,7 @@ async def sqlite_query(
151151
query_type: SessionFSSqliteQueryType,
152152
query: str,
153153
params: dict[str, float | str | None] | None = None,
154-
) -> SessionFsSqliteQueryResult:
154+
) -> SessionFsSqliteQueryResult | None:
155155
self._sqlite_calls.append(
156156
{
157157
"sessionId": self._session_id,
@@ -183,7 +183,7 @@ async def sqlite_query(
183183
columns=[],
184184
rows=[],
185185
rows_affected=cursor.rowcount,
186-
last_insert_rowid=float(cursor.lastrowid) if cursor.lastrowid else None,
186+
last_insert_rowid=cursor.lastrowid if cursor.lastrowid else None,
187187
)
188188

189189
async def sqlite_exists(self) -> bool:
@@ -225,7 +225,7 @@ async def test_should_route_sql_queries_through_the_sessionfs_sqlite_handler(
225225
create_session_fs_handler=_create_sqlite_handler(sqlite_calls),
226226
)
227227

228-
msg = await session.send_and_wait(
228+
await session.send_and_wait(
229229
'Use the sql tool to create a table called "items" with columns '
230230
"id (TEXT PRIMARY KEY) and name (TEXT). "
231231
'Then insert a row with id "a1" and name "Widget".'

‎rust/src/session_fs.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -413,7 +413,7 @@ pub trait SessionFsSqliteProvider: Send + Sync {
413413
query_type: SessionFsSqliteQueryType,
414414
query: &str,
415415
params: Option<&HashMap<String, serde_json::Value>>,
416-
) -> Result<SessionFsSqliteQueryResult, FsError>;
416+
) -> Result<Option<SessionFsSqliteQueryResult>, FsError>;
417417

418418
/// Check whether the provider has a SQLite database for this session.
419419
async fn sqlite_exists(&self) -> Result<bool, FsError>;
@@ -432,7 +432,7 @@ pub struct SessionFsSqliteQueryResult {
432432
/// Number of rows affected (for INSERT/UPDATE/DELETE).
433433
pub rows_affected: i64,
434434
/// Last inserted row ID (for INSERT).
435-
pub last_insert_rowid: Option<f64>,
435+
pub last_insert_rowid: Option<i64>,
436436
}
437437

438438
#[cfg(test)]

‎rust/src/session_fs_dispatch.rs‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -346,11 +346,18 @@ pub(crate) async fn sqlite_query(
346346
.sqlite_query(params.query_type, &params.query, sqlite_params)
347347
.await
348348
{
349-
Ok(result) => GeneratedSqliteQueryResult {
349+
Ok(Some(result)) => GeneratedSqliteQueryResult {
350350
columns: result.columns,
351351
rows: result.rows,
352352
rows_affected: result.rows_affected,
353-
last_insert_rowid: result.last_insert_rowid,
353+
last_insert_rowid: result.last_insert_rowid.map(|v| v as f64),
354+
error: None,
355+
},
356+
Ok(None) => GeneratedSqliteQueryResult {
357+
columns: Vec::new(),
358+
rows: Vec::new(),
359+
rows_affected: 0,
360+
last_insert_rowid: None,
354361
error: None,
355362
},
356363
Err(e) => GeneratedSqliteQueryResult {

‎rust/tests/e2e/session_fs_sqlite.rs‎

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ use github_copilot_sdk::{
99
};
1010
use rusqlite::Connection;
1111

12-
use super::support::{assistant_message_content, with_e2e_context};
12+
use super::support::with_e2e_context;
1313

1414
#[derive(Debug)]
1515
struct SqliteCall {
@@ -216,7 +216,7 @@ impl SessionFsSqliteProvider for InMemorySqliteProvider {
216216
query_type: SessionFsSqliteQueryType,
217217
query: &str,
218218
_params: Option<&HashMap<String, serde_json::Value>>,
219-
) -> Result<SessionFsSqliteQueryResult, FsError> {
219+
) -> Result<Option<SessionFsSqliteQueryResult>, FsError> {
220220
let qt_str = match query_type {
221221
SessionFsSqliteQueryType::Exec => "exec",
222222
SessionFsSqliteQueryType::Query => "query",
@@ -233,24 +233,24 @@ impl SessionFsSqliteProvider for InMemorySqliteProvider {
233233
let db = Self::get_or_create_db(&mut db_guard)?;
234234
let trimmed = query.trim();
235235
if trimmed.is_empty() {
236-
return Ok(SessionFsSqliteQueryResult {
236+
return Ok(Some(SessionFsSqliteQueryResult {
237237
columns: vec![],
238238
rows: vec![],
239239
rows_affected: 0,
240240
last_insert_rowid: None,
241-
});
241+
}));
242242
}
243243

244244
match query_type {
245245
SessionFsSqliteQueryType::Exec => {
246246
db.execute_batch(trimmed)
247247
.map_err(|e| FsError::Other(e.to_string()))?;
248-
Ok(SessionFsSqliteQueryResult {
248+
Ok(Some(SessionFsSqliteQueryResult {
249249
columns: vec![],
250250
rows: vec![],
251251
rows_affected: 0,
252252
last_insert_rowid: None,
253-
})
253+
}))
254254
}
255255
SessionFsSqliteQueryType::Query => {
256256
let mut stmt = db
@@ -287,31 +287,31 @@ impl SessionFsSqliteProvider for InMemorySqliteProvider {
287287
}
288288
rows.push(map);
289289
}
290-
Ok(SessionFsSqliteQueryResult {
290+
Ok(Some(SessionFsSqliteQueryResult {
291291
columns,
292292
rows,
293293
rows_affected: 0,
294294
last_insert_rowid: None,
295-
})
295+
}))
296296
}
297297
SessionFsSqliteQueryType::Run => {
298298
let affected = db
299299
.execute(trimmed, [])
300300
.map_err(|e| FsError::Other(e.to_string()))?;
301301
let last_id = db.last_insert_rowid();
302-
Ok(SessionFsSqliteQueryResult {
302+
Ok(Some(SessionFsSqliteQueryResult {
303303
columns: vec![],
304304
rows: vec![],
305305
rows_affected: affected as i64,
306-
last_insert_rowid: Some(last_id as f64),
307-
})
306+
last_insert_rowid: Some(last_id),
307+
}))
308308
}
309-
_ => Ok(SessionFsSqliteQueryResult {
309+
_ => Ok(Some(SessionFsSqliteQueryResult {
310310
columns: vec![],
311311
rows: vec![],
312312
rows_affected: 0,
313313
last_insert_rowid: None,
314-
}),
314+
})),
315315
}
316316
}
317317

‎rust/tests/session_test.rs‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2997,7 +2997,7 @@ impl SessionFsSqliteProvider for RecordingFsProvider {
29972997
query_type: SessionFsSqliteQueryType,
29982998
query: &str,
29992999
params: Option<&std::collections::HashMap<String, serde_json::Value>>,
3000-
) -> Result<SessionFsSqliteQueryResult, FsError> {
3000+
) -> Result<Option<SessionFsSqliteQueryResult>, FsError> {
30013001
let mut row = std::collections::HashMap::new();
30023002
row.insert(
30033003
"query".to_string(),
@@ -3022,7 +3022,7 @@ impl SessionFsSqliteProvider for RecordingFsProvider {
30223022
.cloned()
30233023
.unwrap_or(serde_json::Value::Null),
30243024
);
3025-
Ok(SessionFsSqliteQueryResult {
3025+
Ok(Some(SessionFsSqliteQueryResult {
30263026
columns: vec![
30273027
"query".to_string(),
30283028
"queryType".to_string(),
@@ -3031,7 +3031,7 @@ impl SessionFsSqliteProvider for RecordingFsProvider {
30313031
rows: vec![row],
30323032
rows_affected: 0,
30333033
last_insert_rowid: None,
3034-
})
3034+
}))
30353035
}
30363036

30373037
async fn sqlite_exists(&self) -> Result<bool, FsError> {
@@ -3223,7 +3223,7 @@ async fn session_fs_maps_sqlite_errors_to_results() {
32233223
_query_type: SessionFsSqliteQueryType,
32243224
_query: &str,
32253225
_params: Option<&std::collections::HashMap<String, serde_json::Value>>,
3226-
) -> Result<SessionFsSqliteQueryResult, FsError> {
3226+
) -> Result<Option<SessionFsSqliteQueryResult>, FsError> {
32273227
Err(FsError::Other("sqlite unavailable".to_string()))
32283228
}
32293229

0 commit comments

Comments
 (0)