fix(agent-team): address GPT-4.1 cross-review findings on the dashboard surface
- read_transitions: catch sqlite3.Error (not just OperationalError) so a corrupt ledger degrades to empty rather than raising into callers - recorder open-row lookup: order by the monotonic transition_id (drop the timestamp-format dependency) - TransitionRecorder.close(): release the retained in-memory test connection - dashboard task_detail: return only the exception TYPE, never str(exc) (a SQLite message can carry the DB path) - _instrument: coerce a status enum to .value defensively before the terminal check - schema.migrate: document the ordering constraint for future ALTERs vs the unconditional idempotent tail
This commit is contained in:
parent
4f4db6ed03
commit
6951bf2fc6
4 changed files with 34 additions and 6 deletions
|
|
@ -142,7 +142,13 @@ def task_detail(db_path: Path | str, thread_id: str) -> dict[str, Any]:
|
|||
rows = read_transitions(conn, thread_id)
|
||||
cost_by_stage = _cost_by_stage(conn, thread_id)
|
||||
except Exception as exc: # never crash the endpoint
|
||||
return {"ok": False, "error": f"read failed: {exc}", "thread_id": thread_id}
|
||||
# Return only the exception TYPE, never str(exc) — a SQLite message can
|
||||
# carry the DB path / table names; keep the LAN surface tight.
|
||||
return {
|
||||
"ok": False,
|
||||
"error": f"read failed: {type(exc).__name__}",
|
||||
"thread_id": thread_id,
|
||||
}
|
||||
finally:
|
||||
if conn is not None:
|
||||
conn.close()
|
||||
|
|
|
|||
|
|
@ -291,6 +291,12 @@ def migrate(conn: sqlite3.Connection) -> None:
|
|||
# Applied UNCONDITIONALLY (idempotent IF NOT EXISTS) so an already-stamped DB
|
||||
# — which skips the version blocks above — still gains these tables without a
|
||||
# restamp. Safe on existing data: fresh empty tables / indexes only.
|
||||
#
|
||||
# ORDERING CONSTRAINT: a future migration that ALTERs one of these tables
|
||||
# (e.g. `ALTER TABLE task_transitions ADD COLUMN ...`) MUST run in its own
|
||||
# `if current < N` block placed ABOVE this tail — the unconditional CREATE
|
||||
# ... IF NOT EXISTS here no-ops on an existing table and will NOT apply an
|
||||
# alter. This tail is only for first-time creation on an already-stamped DB.
|
||||
conn.execute(INGESTED_ISSUES_DDL)
|
||||
conn.execute(TASK_TRANSITIONS_DDL)
|
||||
for stmt in _split_statements(TASK_TRANSITIONS_INDEXES_DDL):
|
||||
|
|
|
|||
|
|
@ -59,7 +59,9 @@ def read_transitions(
|
|||
"ORDER BY entered_at ASC, transition_id ASC",
|
||||
(thread_id,),
|
||||
).fetchall()
|
||||
except sqlite3.OperationalError:
|
||||
except sqlite3.Error:
|
||||
# Missing table (fresh DB) or a corrupt/unreadable ledger — a status
|
||||
# view must never crash on a read.
|
||||
return []
|
||||
return [dict(row) for row in rows]
|
||||
|
||||
|
|
@ -93,6 +95,17 @@ class TransitionRecorder:
|
|||
if conn is not self._mem_conn:
|
||||
conn.close()
|
||||
|
||||
def close(self) -> None:
|
||||
"""Close the retained in-memory connection, if any (test cleanup).
|
||||
|
||||
File-backed recorders open/close per call and hold nothing, so this is a
|
||||
no-op for them; the in-memory test path retains one connection that this
|
||||
releases so it does not leak when the recorder is discarded.
|
||||
"""
|
||||
if self._mem_conn is not None:
|
||||
self._mem_conn.close()
|
||||
self._mem_conn = None
|
||||
|
||||
def record_entry(
|
||||
self,
|
||||
*,
|
||||
|
|
@ -118,7 +131,7 @@ class TransitionRecorder:
|
|||
open_row = conn.execute(
|
||||
"SELECT transition_id, to_phase FROM task_transitions "
|
||||
"WHERE thread_id = ? AND exited_at IS NULL "
|
||||
"ORDER BY entered_at DESC, transition_id DESC LIMIT 1",
|
||||
"ORDER BY transition_id DESC LIMIT 1",
|
||||
(thread_id,),
|
||||
).fetchone()
|
||||
|
||||
|
|
@ -169,7 +182,7 @@ class TransitionRecorder:
|
|||
open_row = conn.execute(
|
||||
"SELECT transition_id FROM task_transitions "
|
||||
"WHERE thread_id = ? AND exited_at IS NULL "
|
||||
"ORDER BY entered_at DESC, transition_id DESC LIMIT 1",
|
||||
"ORDER BY transition_id DESC LIMIT 1",
|
||||
(thread_id,),
|
||||
).fetchone()
|
||||
if open_row is None:
|
||||
|
|
|
|||
|
|
@ -353,9 +353,12 @@ def _instrument(
|
|||
recorder.record_entry(thread_id=thread_id, to_phase=name, status=status)
|
||||
result = fn(state, *args, **kwargs)
|
||||
if isinstance(result, dict):
|
||||
# Nodes store status as TaskStatus.value strings; coerce defensively
|
||||
# so an enum member (should one slip through) still closes the row.
|
||||
new_status = result.get("status")
|
||||
if new_status in _TERMINAL_STATUS_VALUES:
|
||||
recorder.close_terminal(thread_id=thread_id, status=new_status)
|
||||
status_value = getattr(new_status, "value", new_status)
|
||||
if status_value in _TERMINAL_STATUS_VALUES:
|
||||
recorder.close_terminal(thread_id=thread_id, status=status_value)
|
||||
return result
|
||||
|
||||
# Belt-and-suspenders for B4: present fn's exact signature to LangGraph.
|
||||
|
|
|
|||
Reference in a new issue