feat: add session health monitoring and checkpoint mechanism - #72
feat: add session health monitoring and checkpoint mechanism#72FJT123123 wants to merge 1 commit into
Conversation
- Add SessionMetricsCollector to track tool calls, file reads, searches, and modifications. - Add SessionHealthAnalyzer to evaluate session quality and recommend checkpoints. - Add SessionCheckpointState, Manager, and Persistence for checkpoint lifecycle. - Integrate metrics and checkpoint tracking into tool execution pipeline. - Expose session health in /v1/status endpoint. - Add session-checkpoint skill with Chinese-first documentation. - Extend gateway prompts with session awareness guidance. - Increase watchdog timeout from 30 minutes to 12 hours for long sessions. - Add 22 unit tests covering all new session modules.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6b6d4ba0e0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (health.shouldCheckpoint) { | ||
| options.log(` 📌 Checkpoint recommended: ${health.risks.join(', ')}`); |
There was a problem hiding this comment.
When a long session reaches a warning/degraded state, this branch only writes a recommendation to the VS Code output channel. It never calls the newly constructed checkpointManager.createIfNeeded, returns the recommendation to the agent, or writes SESSION_CHECKPOINT.md, so the checkpoint mechanism does nothing precisely when shouldCheckpoint becomes true.
Useful? React with 👍 / 👎.
| const metrics = this.healthAdapter.toHealthMetrics( | ||
| this.metricsCollector.getMetrics() | ||
| ); |
There was a problem hiding this comment.
For a successful independent task, executeLocalTool records completed work in checkpointState, but getHealth() invokes the adapter without semantic metrics, so completedTasks is always its default of zero. Consequently the analyzer's metrics.completedTasks > 0 checkpoint trigger can never fire, and a healthy session will not recommend the checkpoint that the lifecycle policy expects after task completion.
Useful? React with 👍 / 👎.
| private onAutoStop: (() => void) | null = null; | ||
| private skillManager: SkillManager; | ||
| private terminalSessionManager: TerminalSessionManager; | ||
| private readonly sessionRuntime = new SessionRuntime(); |
There was a problem hiding this comment.
Because this runtime is owned by the long-lived GatewayManager, its counters and read/search histories survive browser disconnects and even gateway stop/start cycles; the only reset() method has no production caller. After one conversation crosses a threshold, a newly bound conversation therefore starts with stale counts and can immediately report warning/degraded health or checkpoint recommendations despite having no activity of its own.
Useful? React with 👍 / 👎.
| private authToken: string = ''; | ||
| private watchdogTimer: NodeJS.Timeout | null = null; | ||
| private readonly WATCHDOG_TIMEOUT = 30 * 60 * 1000; // 30 minutes | ||
| private readonly WATCHDOG_TIMEOUT = 12 * 60 * 60 * 1000; // 12 hours |
There was a problem hiding this comment.
With an idle but still-open bound tab, extending this watchdog to 12 hours defeats the browser's existing 30-minute expiry path: bridge-browser/src/background/session_health.ts checks /v1/status after the idle timeout and reschedules indefinitely whenever the gateway is online, while connection.ts rejects another tab as BUSY while the original tab remains. Previously the 30-minute gateway shutdown made that health check fail and released the session; now a normal new connection can remain blocked for up to 12 hours unless the user explicitly forces takeover.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| const filePath = (await resolveWorkspaceRelativePath(context.workspaceRoot, args.path)).absolutePath; | ||
| context.metricsCollector.recordFileRead(String(args.path)); |
There was a problem hiding this comment.
When a large file is intentionally inspected in several start_line/end_line, head, or tail chunks, every operation records only the path and is therefore counted as a repeated read. After enough legitimate chunked reads this adds the duplicate-read risk and, together with another threshold such as tool-call count, can incorrectly mark the session unhealthy and recommend a checkpoint; the read selection should be included in the operation fingerprint.
Useful? React with 👍 / 👎.
| generateContent(data: SessionCheckpointData): string { | ||
| return [ | ||
| '# Session Checkpoint', |
There was a problem hiding this comment.
If checkpoint creation is invoked, this generator omits Architecture Decisions and Known Issues entirely, even though both skills/session-checkpoint/SKILL.md and its template require those sections. A resumed conversation will therefore lose the rationale and unresolved work that the checkpoint feature is meant to preserve; the checkpoint data model and generated Markdown need fields for both categories.
Useful? React with 👍 / 👎.
| const searchRoot = (await resolveWorkspaceRelativeDirectory(context.workspaceRoot, args.path ?? '.')).absolutePath; | ||
| const workspaceRoot = context.workspaceRoot ?? searchRoot; | ||
| const query = String(args.query); | ||
| context.metricsCollector.recordSearch(query); |
There was a problem hiding this comment.
When the agent follows the prompt's recommendation to use search_files for locating paths, those operations never reach recordSearch; this commit instruments only search_code. Repeated filename searches therefore leave both searchCount and repeatedSearches at zero, so the health monitor cannot detect search loops for one of the two primary search tools.
Useful? React with 👍 / 👎.
| await atomicWriteFile(filePath, contentToWrite); | ||
| context.checkpointState?.recordChangedFile(String(args.path)); |
There was a problem hiding this comment.
When an existing file is changed through the primary edit_file path, the successful write updates only checkpointState and never calls metricsCollector.recordFileModification, unlike write_file. As a result, modifiedFileCount understates normal editing activity and any health policy or status consumer using that metric receives incorrect modification-scope data.
Useful? React with 👍 / 👎.
2 participants
Add SessionMetricsCollector to track tool calls, file reads, searches, and modifications.
Add SessionHealthAnalyzer to evaluate session quality and recommend checkpoints.
Add SessionCheckpointState, Manager, and Persistence for checkpoint lifecycle.
Integrate metrics and checkpoint tracking into tool execution pipeline.
Expose session health in /v1/status endpoint.
Add session-checkpoint skill with Chinese-first documentation.
Extend gateway prompts with session awareness guidance.
Increase watchdog timeout from 30 minutes to 12 hours for long sessions.
Add 22 unit tests covering all new session modules.