Skip to content

Commit 3257ea5

Browse files
committed
fix(tui+lsp): resolve CI failures and CodeRabbit findings; harden RunAgents
Make the PR branch green and address the review. CI failures (15 tests + ruff): - Update agent-spec / config-dump / pyinstaller snapshots for the new LSP tool - Add explicit encoding to LSP framing/symbol_context/tool sources (static check) - Sync dark prompt frame/separator/dialog borders to their core theme tokens - Flush the live view immediately for external (approval/steer) messages and supervise the frame/status refresh loops so a refresh failure surfaces instead of silently freezing the view (fixes 3 external-approval tests) - Wire `_mode` + completer onto the bottom-toolbar unit test's bare session - Fix import sorting / line length CodeRabbit findings: - Critical: inline code spans now actually clear an inherited background (Rich `Style(bgcolor=None)` is a no-op; mutate the copied style instead) - LSP: bounded JSON-RPC frame size + graceful-shutdown timeout, didChange document version tracking, open-doc state cleared on restart, empty diagnostics payloads clear stale entries, code-0 dedup key, TOCTOU re-check, documentSymbol file_count - Theme picker: compare resolved themes by identity so distinct Catppuccin variants no longer match - Usage activity: anchor the chart to the payload window (not wall-clock) and fix the current-streak partial-today contract - Config: validate LSP numeric settings; `/usage` rejects extra activity args; `/theme code <Name>` preserves case; guard install-script cursor escapes to animated mode; drop unused worklog icon params RunAgents: strip stray whitespace-only string entries the model emits between agent objects so a multi-agent launch no longer fails validation. Also: AGENTS.md gains a "Pre-PR gate" checklist (run the full gate, snapshot fix-direction, encoding/bundling/changelog checks) to stop these slipping to CI.
1 parent dfcc6b7 commit 3257ea5

51 files changed

Lines changed: 2041 additions & 211 deletions

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

AGENTS.md

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -224,6 +224,41 @@ Pick the smallest reliable gate for the change, then run broader gates before re
224224
If a gate cannot run because of missing system tools (for example `npm`), report that explicitly
225225
instead of claiming success.
226226

227+
## Pre-PR gate (run before pushing or opening a PR)
228+
229+
CI failures that "slip to GitHub" almost always trace to pushing after a *partial* local check
230+
(for example running `ruff`/`pyright` on a single file instead of the whole package). Before you
231+
push a branch or open a PR that touches shipped code, run the full gate and clear every item below.
232+
Running a focused check on only the files you edited is **not** sufficient — snapshot, static, and
233+
bundling tests fail on files you did not touch.
234+
235+
1. **Full gate, not partial.** Run `make check-pythinker-code` (ruff + format + pyright) **and**
236+
`make test-pythinker-code`. Paste/confirm the actual "All checks passed" / passing summary — a
237+
green `ruff` alone is not a green `check` (pyright and format are separate). For changes to
238+
another workspace package, run that package's `make check-* && make test-*` too.
239+
2. **Include `tests_e2e`.** CI runs `tests` and `tests_e2e`. New slash commands, wire events, or
240+
agent-spec/tool changes move the wire-handshake snapshot and the agent-spec/config/pyinstaller
241+
snapshots. Re-run the affected tests; apply deliberate snapshot updates with
242+
`uv run pytest <node> --inline-snapshot=fix` and **read the resulting diff** before committing.
243+
3. **Snapshot fix-direction.** A hardcoded expected value that changed deliberately → update the
244+
**test**. An invariant of the form "two values must stay equal" (e.g. a prompt token that must
245+
track a core theme token) → fix the **code** that drifted, never the test.
246+
4. **New source files clear the static checks.** `tests/test_ai_static_requirements.py` enforces
247+
explicit text encoding (`encoding="utf-8"`, `errors="replace"` for tool decodes) and other
248+
invariants across `src/pythinker_code/**`. Note the ruff-vs-static conflict: ruff `UP012` strips
249+
`"utf-8"` from a **string-literal** `.encode("utf-8")`, but the static check wants an explicit
250+
`encoding=`. Encode/decode via a **local variable or call result**, not a literal, so both gates
251+
pass (see `lsp/framing.py`).
252+
5. **New bundled files/tools update the manifests.** A new tool `*.md`, prompt, or package adds
253+
entries to `tests/utils/test_pyinstaller_utils.py` (`datas` + `hiddenimports`) and, for new
254+
config keys, `tests/core/test_config.py::test_default_config_dump`.
255+
6. **Changelog.** Any change to shipped paths (`src/*`, `packages/*`, installers, release
256+
workflows, `pythinker.spec`) needs a new `- ...` line under `## Unreleased` in `CHANGELOG.md`, or
257+
the `changelog-entry-required` check fails the PR.
258+
7. **Confirm what is actually new.** Diff against `origin/main` (`git log origin/main..HEAD`,
259+
`git diff origin/main...HEAD --stat`) so the PR scope — and the review/verification surface — is
260+
what you intend, not stale local commits.
261+
227262
## Project architecture
228263

229264
### Runtime path

CHANGELOG.md

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,13 @@ GitHub Releases page; `0.8.0` is the new starting line.
1515

1616
## Unreleased
1717

18+
- **TUI composing preview gap.** Removed the visible double-blank row between
19+
`Composing…` and the in-progress preview (leading newline from commit
20+
boundaries no longer leaks through the plain-text preview path), and aligned
21+
the Rich `Live` paint rate with the 25 Hz reveal scheduler (was 10 Hz).
22+
- **ToolSearch TUI display.** `ToolSearch` results now render as a compact
23+
"N tools discovered (Agent, Grep, …)" summary instead of dumping the full
24+
tool catalog with descriptions; ctrl+o expands to tool names only.
1825
- **Tool header highlights.** Read/Write/Edit/Grep and similar tool-call subjects
1926
now use the brand periwinkle `accent` token instead of cyan `info`; line ranges
2027
stay on the yellow `warning` token.
@@ -48,6 +55,23 @@ GitHub Releases page; `0.8.0` is the new starting line.
4855
total tokens consumed each day, with a `Lifetime · Peak · Streak · Longest task` summary
4956
line and a footer that lets the user switch between daily/weekly/cumulative views. Data is
5057
read from the local session wire files; the per-provider adapter behavior is unchanged.
58+
- **RunAgents tolerates blank list entries.** Models occasionally emit bare `"\n"` strings
59+
between the agent objects in the `agents` array; those are now stripped before validation so
60+
a multi-agent launch no longer fails with a validation error, while genuinely invalid entries
61+
are still rejected.
62+
- **Report panel rendering.** Standardized report panels render only the panel title and section
63+
headers bold (body prose stays regular weight), tag finding locations with a file marker, and
64+
use a dedicated `secondary` theme token for scope/note text.
65+
- **Theme token consistency.** The dark prompt frame/separator/dialog borders and the prompt
66+
glyph now track their canonical core theme tokens, and inline code spans correctly drop an
67+
inherited background.
68+
- **External approvals repaint promptly.** Out-of-band approval requests and steer input now
69+
force an immediate live-view repaint instead of waiting for the streaming frame budget, and
70+
the live-view refresh loop is supervised so a refresh-loop failure surfaces instead of
71+
silently freezing the view.
72+
- **LSP robustness.** Bounded JSON-RPC frame size and graceful-shutdown timeout, document
73+
version tracking for `didChange`, open-document state cleared on server restart, empty
74+
diagnostics payloads clear stale entries, and tightened `/usage` activity-argument validation.
5175

5276
## 0.47.0 (2026-06-16)
5377

docs/public/install.sh

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -641,13 +641,17 @@ until release_has_assets; do
641641
fail "release assets for v${VERSION} are not available after ~${max_elapsed}s: ${tarball_url}
642642
The latest release may still be publishing. Try again shortly, or pin a known-good version with --version X.Y.Z"
643643
fi
644-
printf '\033[%d;1H\033[K %s%-11s%s release assets, retrying in %s%ss%s' "$PROGRESS_ROW" "$DIM" "Waiting" "$RESET" "$BAR" "$delay" "$RESET"
644+
if [ -n "$_anim" ]; then
645+
printf '\033[%d;1H\033[K %s%-11s%s release assets, retrying in %s%ss%s' "$PROGRESS_ROW" "$DIM" "Waiting" "$RESET" "$BAR" "$delay" "$RESET"
646+
else
647+
printf ' Waiting for release assets, retrying in %ss\n' "$delay"
648+
fi
645649
sleep "$delay"
646650
elapsed=$((elapsed + delay))
647651
delay=$((delay * 2))
648652
[ "$delay" -gt 120 ] && delay=120
649653
done
650-
[ "$attempt" -gt 0 ] && printf '\033[%d;1H\033[K' "$PROGRESS_ROW"
654+
[ -n "$_anim" ] && [ "$attempt" -gt 0 ] && printf '\033[%d;1H\033[K' "$PROGRESS_ROW"
651655

652656
# --- download + verify --------------------------------------------------
653657
tmpdir="$(mktemp -d -t pythinker-install.XXXXXX)"

scripts/install-native.sh

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -641,13 +641,17 @@ until release_has_assets; do
641641
fail "release assets for v${VERSION} are not available after ~${max_elapsed}s: ${tarball_url}
642642
The latest release may still be publishing. Try again shortly, or pin a known-good version with --version X.Y.Z"
643643
fi
644-
printf '\033[%d;1H\033[K %s%-11s%s release assets, retrying in %s%ss%s' "$PROGRESS_ROW" "$DIM" "Waiting" "$RESET" "$BAR" "$delay" "$RESET"
644+
if [ -n "$_anim" ]; then
645+
printf '\033[%d;1H\033[K %s%-11s%s release assets, retrying in %s%ss%s' "$PROGRESS_ROW" "$DIM" "Waiting" "$RESET" "$BAR" "$delay" "$RESET"
646+
else
647+
printf ' Waiting for release assets, retrying in %ss\n' "$delay"
648+
fi
645649
sleep "$delay"
646650
elapsed=$((elapsed + delay))
647651
delay=$((delay * 2))
648652
[ "$delay" -gt 120 ] && delay=120
649653
done
650-
[ "$attempt" -gt 0 ] && printf '\033[%d;1H\033[K' "$PROGRESS_ROW"
654+
[ -n "$_anim" ] && [ "$attempt" -gt 0 ] && printf '\033[%d;1H\033[K' "$PROGRESS_ROW"
651655

652656
# --- download + verify --------------------------------------------------
653657
tmpdir="$(mktemp -d -t pythinker-install.XXXXXX)"

src/pythinker_code/config.py

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1036,15 +1036,15 @@ class LspServerConfig(BaseModel):
10361036
initialization_options: dict[str, Any] | None = Field(
10371037
default=None, alias="initializationOptions"
10381038
)
1039-
startup_timeout: float = Field(default=30.0, alias="startupTimeout")
1040-
max_restarts: int = Field(default=3, alias="maxRestarts")
1039+
startup_timeout: float = Field(default=30.0, alias="startupTimeout", gt=0)
1040+
max_restarts: int = Field(default=3, alias="maxRestarts", ge=0)
10411041

10421042

10431043
class LspConfig(BaseModel):
10441044
enabled: bool = True
10451045
recommendation_disabled: bool = False
10461046
recommendation_never: list[str] = Field(default_factory=list)
1047-
recommendation_ignored_count: int = 0
1047+
recommendation_ignored_count: int = Field(default=0, ge=0)
10481048

10491049

10501050
class PluginsConfig(BaseModel):

src/pythinker_code/lsp/client.py

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,8 @@
2424
NotificationHandler = Callable[[Any], None] | Callable[[Any], Awaitable[None]]
2525
RequestHandler = Callable[[Any], Any] | Callable[[Any], Awaitable[Any]]
2626

27+
_SHUTDOWN_TIMEOUT_S = 2.0
28+
2729

2830
class LspClient:
2931
"""Minimal LSP client: spawn via Host.exec, JSON-RPC over Content-Length framing."""
@@ -137,8 +139,12 @@ async def stop(self) -> None:
137139

138140
proc = self._proc
139141
if proc is not None and proc.returncode is None:
142+
# Bound the graceful handshake: a hung server must not block teardown.
143+
# On timeout (or any error) we fall through to killing the process below.
140144
with suppress(Exception):
141-
await self.send_request("shutdown", None)
145+
await asyncio.wait_for(
146+
self.send_request("shutdown", None), timeout=_SHUTDOWN_TIMEOUT_S
147+
)
142148
with suppress(Exception):
143149
await self.send_notification("exit", None)
144150

src/pythinker_code/lsp/diagnostics.py

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,7 @@ def diagnostic_key(entry: DiagnosticEntry) -> str:
8686
},
8787
},
8888
"source": entry.source or None,
89-
"code": entry.code or None,
89+
"code": entry.code if entry.code is not None else None,
9090
},
9191
sort_keys=True,
9292
separators=(",", ":"),
@@ -281,10 +281,17 @@ async def handler(params: Any) -> None:
281281
parsed = PublishDiagnosticsParams.model_validate(params)
282282
path = uri_to_path(parsed.uri) or parsed.uri
283283
entries = [diagnostic_entry_from_lsp(item) for item in parsed.diagnostics]
284-
registry.register_pending(
285-
server_name,
286-
[DiagnosticFile(uri=parsed.uri, path=path, diagnostics=entries)],
287-
)
284+
if not entries:
285+
# An empty payload means "no problems now" for this file, so drop
286+
# any previously stored diagnostics for it. clear_for_file clears
287+
# across all servers and the sent-key LRU; that is safe here
288+
# because routing is one server per extension.
289+
registry.clear_for_file(parsed.uri)
290+
else:
291+
registry.register_pending(
292+
server_name,
293+
[DiagnosticFile(uri=parsed.uri, path=path, diagnostics=entries)],
294+
)
288295
failure_count = 0
289296
except Exception as exc:
290297
failure_count += 1

src/pythinker_code/lsp/framing.py

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,9 @@ async def read_message(stdout: AsyncReadable) -> dict[str, Any]:
7373

7474
async def write_message(stdin: AsyncWritable, message: dict[str, Any]) -> None:
7575
"""Write one LSP message with Content-Length framing."""
76-
body = json.dumps(message, separators=(",", ":")).encode()
77-
header = f"Content-Length: {len(body)}\r\n\r\n".encode()
78-
stdin.write(header + body)
76+
body = json.dumps(message, separators=(",", ":")).encode(encoding="utf-8")
77+
# Encode via a local (not a string literal) so the explicit encoding survives
78+
# ruff UP012 while satisfying the explicit-encoding static check.
79+
header = f"Content-Length: {len(body)}\r\n\r\n"
80+
stdin.write(header.encode(encoding="utf-8") + body)
7981
await stdin.drain()

src/pythinker_code/lsp/manager.py

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ def __init__(
3131
self._instances: dict[str, LspServerInstance] = {}
3232
self._ext_map: dict[str, list[str]] = {}
3333
self._opened_files: dict[str, str] = {}
34+
self._doc_versions: dict[str, int] = {}
3435

3536
async def initialize(self) -> None:
3637
errors: list[str] = []
@@ -82,6 +83,7 @@ async def shutdown(self) -> None:
8283
self._instances.clear()
8384
self._ext_map.clear()
8485
self._opened_files.clear()
86+
self._doc_versions.clear()
8587

8688
stop_errors = [
8789
f"{to_stop[i][0]}: {result}"
@@ -105,9 +107,19 @@ async def ensure_started(self, path: str) -> LspServerInstance | None:
105107
if server is None:
106108
return None
107109
if server.state in (LspState.STOPPED, LspState.ERROR):
110+
# A (re)start spawns a fresh process with no open documents. Drop any
111+
# stale per-server open-file and version state so didOpen is re-sent
112+
# instead of being skipped as already-open on the new process.
108113
await server.start()
114+
self._clear_server_doc_state(server.name)
109115
return server
110116

117+
def _clear_server_doc_state(self, server_name: str) -> None:
118+
stale_uris = [uri for uri, name in self._opened_files.items() if name == server_name]
119+
for uri in stale_uris:
120+
self._opened_files.pop(uri, None)
121+
self._doc_versions.pop(uri, None)
122+
111123
async def send_request(self, path: str, method: str, params: Any) -> Any | None:
112124
server = await self.ensure_started(path)
113125
if server is None:
@@ -143,6 +155,7 @@ async def open_file(self, path: str, content: str) -> None:
143155
},
144156
)
145157
self._opened_files[file_uri] = server.name
158+
self._doc_versions[file_uri] = 1
146159

147160
async def change_file(self, path: str, content: str) -> None:
148161
server = self.server_for_file(path)
@@ -155,10 +168,12 @@ async def change_file(self, path: str, content: str) -> None:
155168
await self.open_file(path, content)
156169
return
157170

171+
version = self._doc_versions.get(file_uri, 1) + 1
172+
self._doc_versions[file_uri] = version
158173
await server.send_notification(
159174
"textDocument/didChange",
160175
{
161-
"textDocument": {"uri": file_uri, "version": 1},
176+
"textDocument": {"uri": file_uri, "version": version},
162177
"contentChanges": [{"text": content}],
163178
},
164179
)
@@ -183,6 +198,7 @@ async def close_file(self, path: str) -> None:
183198
{"textDocument": {"uri": file_uri}},
184199
)
185200
self._opened_files.pop(file_uri, None)
201+
self._doc_versions.pop(file_uri, None)
186202

187203

188204
def _file_uri(path: str) -> str:

src/pythinker_code/lsp/service.py

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,14 @@ async def save_file(self, path: str) -> None:
9797
await manager.save_file(path)
9898

9999
async def reinitialize(self, *, servers: dict[str, LspServerConfig] | None = None) -> None:
100+
# Cancel any in-flight init before replacing the event/manager. Otherwise
101+
# the stale task's finally can set the old event (waking callers parked in
102+
# wait_for_init on a future that never completes) and _kickoff_init would
103+
# return early while PENDING.
104+
if self._init_task is not None and not self._init_task.done():
105+
self._init_task.cancel()
106+
with contextlib.suppress(asyncio.CancelledError):
107+
await self._init_task
100108
if servers is not None:
101109
self._servers = servers
102110
if self._manager is not None:

0 commit comments

Comments
 (0)