Conversation
![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: b85c429e96
ℹ️ 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 _, err := f.ReadAt(buf, offset); err != nil && err != io.EOF { | ||
| return nil, fmt.Errorf("read %s: %w", path, err) | ||
| } | ||
| accum = append(buf, accum...) |
There was a problem hiding this comment.
The tail reader reuses a fixed chunk buffer and then prepends with accum = append(buf, accum...); if accum still points at the prior chunk backing array (common when the previous iteration left a short partial line), the next ReadAt overwrites those carry-over bytes before this append. That corrupts or drops chunk-boundary log lines when files are larger than one chunk, so /api/logs can return malformed/missing entries under normal log sizes.
Useful? React with 👍 / 👎.
| const eSource = String(e.source || '').toLowerCase(); | ||
| if (source === 'gateway' && !eSource.includes('gateway')) return false; | ||
| if (source === 'cron' && !eSource.includes('cron')) return false; | ||
| if (source !== 'all' && source !== 'gateway' && source !== 'cron' && eSource !== source) return false; |
There was a problem hiding this comment.
The client-side filter treats non-gateway/cron sources as exact matches (eSource !== source), but the backend returns path-like source names (for example logs/session.log). When users choose the Session or Subagent tabs (source=session|subagent), the API returns entries but _filtered removes them all, making those source tabs appear empty even with valid data.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR prepares the v2026.4.13 release by adding diagnostics delivery (live log tail + error feed) and hardening lifecycle/ops paths across refresh, server, system, and service management, with corresponding docs/release-note updates.
Changes:
- Add
/api/logs+/api/errorsendpoints and a new Diagnostics UI (Live Logs + Error Feed). - Propagate
context.Contextthrough version detection, refresh collectors, and service backends; tighten shutdown/timeout/CORS behavior. - Align operational docs, Makefile/CI checks, and release notes for v2026.4.13.
Reviewed changes
Copilot reviewed 64 out of 64 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| web/index.html | Adds Diagnostics panels + JS polling/filtering |
| version.go | Thread context into version detection wrapper |
| version_test.go | Update tests for new detectVersion signature |
| TECHNICAL.md | Document new routes + make check workflow |
| system_test.go | Update refresh collector seam signature |
| SKILLS.md | Add human reference for Codex skills |
| server_test.go | Update refresh collector seam signature |
| RULES.md | Add Go style/testing rules reference |
| refresh.go | Add context-aware refresh collector seam |
| refresh_test.go | Add subagent zero-cost count test + ctx updates |
| README.md | Update refresh semantics + document new endpoints |
| plan/014-live-log-tail-error-feed.md | Add implementation plan for issue #14 |
| Makefile | Embed BuildVersion + add fmt target |
| main.go | Use signal-driven context + slog + openclaw path resolver |
| internal/appsystem/system_service.go | Slog + cached stale payload + shared HTTP client + timeouts |
| internal/appsystem/system_service_test.go | Add probe timeout test |
| internal/appsystem/system_collect_linux.go | Respect ctx cancellation in RAM/swap collection |
| internal/appsystem/bench_test.go | Add system benchmarks |
| internal/appservice/unsupported.go | Add NewWithContext stub |
| internal/appservice/systemd.go | Context-aware command exec + port parsing |
| internal/appservice/systemd_test.go | Update tests for ctx-aware runCmd signature |
| internal/appservice/service.go | Context-aware runCmdFunc signature |
| internal/appservice/launchd.go | Context-aware exec + XML escaping + tail optimization |
| internal/appservice/launchd_test.go | Update tests for ctx-aware runCmd signature |
| internal/appservice/exec.go | Add ctx plumbing + default timeout behavior |
| internal/appserver/shutdown_test.go | Update refresh seam signature + lock fix |
| internal/appserver/server_test.go | Add CORS Vary + IPv6 loopback test |
| internal/appserver/server_routes.go | Add logs/errors routes + Vary: Origin + IPv6 allow |
| internal/appserver/server_refresh.go | Use server openclawPath + slog + helper response writer |
| internal/appserver/server_logs.go | Implement /api/logs + /api/errors handlers |
| internal/appserver/server_logs_test.go | Add endpoint + merge/source-alias tests |
| internal/appserver/server_logs_test_helper_test.go | Expose timestamp parser for tests |
| internal/appserver/server_core.go | Store openclaw path + ctx-aware refreshFn + slog |
| internal/appserver/server_chat_handler.go | Expose Retry-After header + slog |
| internal/appserver/bench_test.go | Add server loadData benchmarks |
| internal/appruntime/runtime.go | Add ResolveOpenclawPath + ctx-aware DetectVersion |
| internal/appruntime/runtime_test.go | Update tests for ctx-aware DetectVersion |
| internal/apprefresh/token_usage_cache.go | Cache v2 + slog + slices sort + model alias fixes |
| internal/apprefresh/session_model_cache.go | Add ctx + cond-based refresh dedupe |
| internal/apprefresh/refresh.go | Thread ctx through collectors + slices sort + logConfig in data |
| internal/apprefresh/refresh_tokens.go | Add omitempty tags + slices sort |
| internal/apprefresh/refresh_sessions.go | Add ctx for live model lookup + slices sort + slog |
| internal/apprefresh/logtail.go | Implement log tail/merge + signature normalization + runtime log config |
| internal/apprefresh/bench_test.go | Add refresh/token perf benchmarks |
| internal/apprefresh/apprefresh_test.go | Update tests for ctx-aware session model cache |
| internal/appconfig/config.go | Add logs config + legacy backfill + validation + slog |
| internal/appchat/chat.go | Treat canceled as timeout-like for gateway calls |
| internal/appchat/chat_test.go | Use strings.Contains; remove custom contains helper |
| install.sh | Harden archive/runtime asset checks + clarify refresh semantics |
| go.mod | Add toolchain directive |
| CONTRIBUTING.md | Align contributor workflow to Makefile commands |
| config.go | Re-export LogsConfig type alias |
| CLAUDE.md | Refresh repo constraints/commands/architecture notes |
| CHANGELOG.md | Add v2026.4.13 release notes |
| ARCHITECTURE.md | Document log/error endpoints in appserver scope |
| AGENTS.md | Add Codex always-on repo instructions |
| .github/workflows/tests.yml | Add golangci-lint job |
| .codex/skills/project-ops/SKILL.md | Add native project-ops skill |
| .codex/skills/go-rig/SKILL.md | Add native go-rig skill |
| .codex/skills/go-review/SKILL.md | Add native go-review skill |
| .codex/skills/frontend-dashboard/SKILL.md | Add native frontend-dashboard skill |
| .claude/skills/go-rig/SKILL.md | Update legacy skill metadata/content |
| .claude/rules/go-patterns.md | Add detailed Go patterns reference |
| .claude/rules/go-idioms.md | Add Go 1.26 idioms/modernizers reference |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| _renderControls() { | ||
| const pauseBtn = $('logPauseBtn'); | ||
| const fastBtn = $('logFastBtn'); | ||
| if (pauseBtn) pauseBtn.textContent = this._paused ? '▶ Resume' : '⏸ Pause'; | ||
| if (fastBtn) { | ||
| const fastLabel = this.fastMs > 0 ? Math.round(this.fastMs / 1000) + 's' : 'fast'; | ||
| const normalLabel = this.normalMs > 0 ? Math.round(this.normalMs / 1000) + 's' : 'normal'; | ||
| fastBtn.textContent = this._fast ? '⚡ Normal (' + normalLabel + ')' : '🐢 Fast (' + fastLabel + ')'; | ||
| fastBtn.title = this._fast ? 'Switch to normal interval (' + normalLabel + ')' : 'Switch to fast interval (' + fastLabel + ')'; | ||
| } | ||
| const sourceIds = {all:'logSourceAll', gateway:'logSourceGateway', cron:'logSourceCron', session:'logSourceSession', subagent:'logSourceSubagent'}; | ||
| const severityIds = {all:'logSevAll', info:'logSevInfo', warn:'logSevWarn', error:'logSevError'}; | ||
| Object.keys(sourceIds).forEach((k) => { | ||
| const b = $(sourceIds[k]); | ||
| if (b) b.className = 'tab-btn' + (k === this._source ? ' active' : ''); | ||
| }); | ||
| Object.keys(severityIds).forEach((k) => { | ||
| const b = $(severityIds[k]); | ||
| if (b) b.className = 'tab-btn' + (k === this._severity ? ' active' : ''); | ||
| }); | ||
| if (this._lastRegexError) { | ||
| const rc = $('logStatus'); | ||
| if (rc) { | ||
| rc.style.display = 'block'; | ||
| rc.textContent = this._lastRegexError; | ||
| } | ||
| } else { | ||
| const rc = $('logStatus'); | ||
| if (rc && rc.textContent === 'Invalid regex') { | ||
| rc.style.display = 'none'; | ||
| } | ||
| } | ||
| this._render(); | ||
| }, | ||
|
|
||
| _render(payload) { | ||
| const list = $('logTail'); | ||
| if (!list) return; | ||
| const logCount = $('logCount'); | ||
| const entries = payload && Array.isArray(payload.entries) ? payload.entries : []; | ||
| const prepared = this._filtered(entries); | ||
| if (logCount) logCount.textContent = prepared.length ? prepared.length + ' log lines' : 'No matching logs'; | ||
| const shouldStickBottom = list.scrollHeight - list.clientHeight - list.scrollTop < 24; | ||
| if (!entries.length) { | ||
| list.innerHTML = '<div class="log-empty">No logs found</div>'; | ||
| return; | ||
| } |
There was a problem hiding this comment.
LogTail._renderControls() calls this._render() without a payload, but _render() treats a missing payload as entries = [] and immediately replaces the log list with "No logs found". This clears any previously rendered logs and also breaks regex filtering (setRegex calls _render() with no payload). Consider caching the last fetched payload/entries on LogTail (e.g., this._entries) and have _render() render from cached state when no payload is provided, or make _renderControls() avoid calling _render() unless it has data to render.
| _filtered(entries) { | ||
| const source = this._normalizeSource(this._source); | ||
| return entries.filter((e) => { | ||
| const eSource = String(e.source || '').toLowerCase(); | ||
| if (source === 'gateway' && !eSource.includes('gateway')) return false; | ||
| if (source === 'cron' && !eSource.includes('cron')) return false; | ||
| if (source !== 'all' && source !== 'gateway' && source !== 'cron' && eSource !== source) return false; | ||
| const sev = (e.severity || 'info').toLowerCase(); |
There was a problem hiding this comment.
The source filtering logic will hide all log entries for the "Session" and "Subagent" tabs. The backend returns entry.source as a configured path like logs/session.log, but _filtered() requires eSource === source for any non-all/gateway/cron selection, so source==='session' never matches. Align the frontend filter with the backend source values (e.g., use includes('session')/includes('subagent'), or have the API return a normalized sourceKind field).
| setRegex(v) { | ||
| this._regex = (v || '').trim(); | ||
| try { | ||
| this._compiledRegex = this._regex ? new RegExp(this._regex, 'i') : null; | ||
| this._lastRegexError = ''; | ||
| } catch (e) { | ||
| this._compiledRegex = null; | ||
| this._lastRegexError = 'Invalid regex'; | ||
| } | ||
| this._render(); | ||
| }, |
There was a problem hiding this comment.
Regex highlighting and filtering compile the user regex with only the i flag. As a result, _highlight() will only wrap the first match per line (no global matches), which under-delivers on the "highlight matches" behavior. Consider compiling with ig (and ensuring lastIndex is handled if you ever switch to test() on a global regex).
| _highlight(text) { | ||
| if (!this._compiledRegex) return esc(text); | ||
| if (!text) return ''; | ||
| let safe = esc(text); | ||
| try { | ||
| return safe.replace(this._compiledRegex, (m) => `<mark>${esc(m)}</mark>`); | ||
| } catch (e) { | ||
| return safe; |
There was a problem hiding this comment.
_highlight() escapes text into safe, then runs safe.replace(regex, (m) => '<mark>'+esc(m)+'</mark>'). Since m is already escaped content, esc(m) will double-escape HTML entities inside the highlighted span (e.g., < becomes &lt;). Consider highlighting on the raw text and escaping pieces, or avoid re-escaping m when operating on an already-escaped string.
| type TokenUsageEntry struct { | ||
| Model string `json:"model"` | ||
| Calls int `json:"calls"` | ||
| Calls int `json:"calls,omitempty"` | ||
| Input string `json:"input"` | ||
| Output string `json:"output"` | ||
| CacheRead string `json:"cacheRead"` | ||
| TotalTokens string `json:"totalTokens"` | ||
| Cost float64 `json:"cost"` | ||
| InputRaw int `json:"inputRaw"` | ||
| OutputRaw int `json:"outputRaw"` | ||
| CacheReadRaw int `json:"cacheReadRaw"` | ||
| TotalTokensRaw int `json:"totalTokensRaw"` | ||
| Cost float64 `json:"cost,omitempty"` | ||
| InputRaw int `json:"inputRaw,omitempty"` | ||
| OutputRaw int `json:"outputRaw,omitempty"` | ||
| CacheReadRaw int `json:"cacheReadRaw,omitempty"` | ||
| TotalTokensRaw int `json:"totalTokensRaw,omitempty"` |
There was a problem hiding this comment.
Adding omitempty to TokenUsageEntry.Cost / Calls (and the raw fields) will omit these keys when they are zero. The frontend renders token usage rows with u.calls.toLocaleString() and u.cost.toFixed(2), which will throw if calls/cost are missing (e.g., zero-cost models/runs). Either keep these fields non-omitempty in the API payload, or harden the frontend to default missing numeric fields to 0 before calling numeric methods.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements a comprehensive diagnostics system featuring live log tailing and an error feed, while modernizing the project to Go 1.26. Key changes include the addition of /api/logs and /api/errors endpoints, a new diagnostics panel in the web UI, and the adoption of structured logging via slog. The update also hardens the installation process, improves process lifecycle management with context-aware operations, and establishes new development standards for Codex and Claude agents. Feedback focuses on optimizing slice allocations during log merging, reducing memory overhead by avoiding unnecessary string conversions of large buffers, and adopting idiomatic standard library functions for slice operations.
| } | ||
| heap.Init(&h) | ||
|
|
||
| out := make([]LogRecord, 0, min(globalLimit, len(perSourceRecords))) |
There was a problem hiding this comment.
The capacity for the out slice is incorrectly calculated using min(globalLimit, len(perSourceRecords)). Since len(perSourceRecords) represents the number of log sources (usually a very small number) and globalLimit is the target number of entries (defaulting to 200 or more), this will lead to multiple re-allocations as the slice grows during the merge loop. It should be pre-allocated with globalLimit capacity.
| out := make([]LogRecord, 0, min(globalLimit, len(perSourceRecords))) | |
| out := make([]LogRecord, 0, globalLimit) |
| } | ||
|
|
||
| if len(lines) < limit && len(strings.TrimSpace(string(accum))) > 0 { | ||
| line := strings.TrimSuffix(string(accum), "\r") |
There was a problem hiding this comment.
Converting the entire accum buffer to a string just to check if it contains non-whitespace characters is inefficient, especially since accum can grow up to 1MB (readTailMaxFallback). Using bytes.TrimSpace on the byte slice directly avoids this unnecessary allocation and conversion.
if len(lines) < limit && len(bytes.TrimSpace(accum)) > 0 {| for i, j := 0, len(lines)-1; i < j; i, j = i+1, j-1 { | ||
| lines[i], lines[j] = lines[j], lines[i] | ||
| } |
There was a problem hiding this comment.
The manual implementation of slice reversal should be replaced with slices.Reverse from the standard library. This aligns with the project's goal of using modern Go idioms (Go 1.26) as specified in the repository's style guide.
slices.Reverse(lines)References
- The style guide explicitly mandates using modern Go patterns (1.24+) and standard library iterator/slice APIs where available. (link)
Summary
Included work
slogRelated