Per review: upscaling should be the default behavior (like the original
flux-2-pro chain), not agent opt-in. Policy: every image model whose
native output is below ~2MP now sets upscale=True in its catalog —
users never silently get low-res images. Native hi-res models
(Seedream 5 Pro/Lite, Krea 2 Large) stay off to avoid paying to
upscale already-large output.
- FAL catalog: 16 models flipped to upscale=True (klein, z-image,
nano-banana pro/2/2-lite, gpt-image 1.5/2, ideogram v3/v4, recraft
v4/v4.1, qwen image/3, krea-2 medium on FAL, MAI 2.5 pro).
- Krea plugin: per-model upscale defaults (medium + medium-turbo ON at
1.5K native; large OFF at 2K native), precedence explicit kwarg >
image_gen.krea.upscale config > catalog default.
- The 'upscale' tool param remains as a per-call override in both
directions (false = fast draft, true = force on hi-res/edits).
- Video unchanged: opt-in only (default-on would double every video's
cost and latency).
- Sibling tests updated: routing/payload tests pass upscale=False where
the assertion targets the generation submit; catalog test now pins
the native-resolution policy instead of the flux-2-pro snapshot.
The generated-media surface previously had almost no upscaler coverage:
only fal-ai/flux-2-pro chained Clarity Upscaler (hardcoded catalog
default), every other image model returned ~1MP output with no high-res
path, and video had no upscaler at all. Krea's API treats the enhancer
as a standard second pass; this brings the same shape to Hermes.
- image_generate: new optional 'upscale' boolean in the tool schema.
Explicit true chains the backend upscaler on ANY model (including
edits); explicit false disables flux-2-pro's automatic default;
omitted keeps per-model catalog behavior. Response now reports
'upscaled' so the agent knows which resolution it got.
- FAL image path: explicit flag overrides the catalog 'upscale' default
(Clarity Upscaler, 2x). Failure falls back to the native image.
- Krea plugin: upscale=true chains Krea Enhance
(/generate/enhance/krea/enhance, 2x, prompt-guided) through the same
BYO/managed base URL + auth as generation, with a best-effort poll
loop that never fails a successful generation.
- video_generate: new optional 'upscale' boolean; FAL video plugin
chains ByteDance SeedVR2 (fal-ai/seedvr/upscale/video, 2x factor
mode). Providers without upscalers ignore the kwarg per the ABC
contract (documented in both ABCs).
Validation: targeted suites green (123 tests across 6 files, including
new coverage for override-wins/default-kept/failure-fallback on all
three paths); live E2E on direct FAL verified both chains end-to-end
(klein 9b + Clarity upscaled image; pixverse-v6 1s 360p + SeedVR2
upscaled video).
The dot resolved its state through $sessionDotStateById while the arc on the
same row was decided from an isWorking prop, drilled from the sidebar through
two list components and asserted in five test setups. Two paths to the same
question is how the row's arc and its dot end up disagreeing, and it is why the
arc has broken independently of the dot before.
The row now reads the resolved state directly, and the arc rule moves next to
the states it talks about as `showsRunningArc`. `hasLiveTurn` keeps the row's
other treatment — brighter title, age yielding to the actions menu — on the
wider meaning it always had, where a turn waiting on an answer still counts as
this session's turn.
The list chain drops the prop, its types and the id set built to feed it.
`$workingSessionIds` stays where the sidebar genuinely needs it, for noticing
that a turn settled.
Priority between the overlapping signals — a session can be working and unread
and running a background job at once — was resolved at the call site from five
separate membership lookups, which is how surfaces drift apart. `$sessionDotStateById`
does it once and hands each surface a single answer.
The dot's visual language collapses to three colors on one fill/hollow axis
with nothing moving. Motion on a six-pixel circle can only say "something is
happening", which the row's arc already says better, and it cost a repaint per
frame on every row at once; filled now means producing and hollow means open
but quiet. Working and stalled had differed by 30% opacity and were in practice
the same dot. A settled session paints its project color or nothing, rather
than a grey mark of the same weight as a real status next to every resting row.
The switcher had grown its own dot with its own three states, so it disagreed
with the sidebar on the same session. It renders the shared one now.
The status sets are published under a session's current stored id, but the
sidebar row, a persisted tile and the route can each be holding a different
tip of the same lineage after a compression, and every consumer tested
membership with a plain equality check. When the tips disagreed the session
fell out of the working set mid-turn and the dot dropped to idle with the
model still going. Publish each state under every id the conversation answers
to instead, via a shared `lineageAliases` helper.
A conversation that has not been persisted yet has no stored id at all, and
the projection dropped those rows outright, so the first turn of a new chat
showed no dot and no row arc until the backend handed an id back. Fall back to
the runtime id, which until persistence is the same value the surfaces key on.
Background polls could also clear a live busy state before the backend had
caught up with a just-submitted turn, flicking the dot idle for a beat; the
stream path already guards against that, so the poll path now does too.
The stalled watchdog fired at eight minutes, well past the point of being
useful as a hint. Five is past the app's own long-but-healthy silences, like
a typecheck or a full test run, without outlasting the user's patience.
Two corrections on top of the #71077 base (the whole bug class):
1. Turn boundary = last USER message, not last assistant message. A Codex
turn spans several assistant messages (assistant+tool_calls -> tool ->
... -> final assistant) whose reasoning items must replay together; the
last-assistant boundary would strip reasoning mid-chain from the active
turn (the gap flagged in PR #71077 review).
2. type="compaction" checkpoints (native server-side compaction, PR #81747)
are exempt: they carry already-pruned history, not per-turn reasoning.
Pruning filters items instead of popping the sidecar key.
Sibling site fixed in the same class: the Codex incomplete-continuation
dedup path blind-overwrote codex_reasoning_items on visually-duplicate
interim messages, which would drop the only copy of a checkpoint captured
on the earlier response. Extracted merge_interim_reasoning_items() into
agent/native_compaction.py; newer reasoning wins, prior checkpoints are
preserved unless the newer payload carries its own.
Rebase over 60942fc786 (local lunr search -> Algolia DocSearch) briefly
resurrected the removed @easyops-cn dependency tree from the stale
lock; fresh npm install drops it again. CVE pins unchanged.
The hit test excluded <body> and <html> and missed `#root`, which is
full-window and hit-testable, so every point in the window came back as
something and the window never went mouse-transparent at all. Ask it
structurally instead: anything that CONTAINS the shell is scaffolding around
the HUD rather than part of it, which covers the mount, the body and the
document in one predicate and cannot be out of date again.
Focus gets the same treatment. #81552 pinned the window solid whenever
anything in it held focus, to stop the HUD going click-through under its own
dialogs — but the composer holds focus as the HUD's resting state, so an
engaged HUD claimed its whole rectangle. What that fix needed was focus
BESIDE the shell: a portalled dialog, popover or menu owns the next click,
including the one outside it that dismisses it, and the hit test cannot see
that one coming. Focus inside the shell is the composer, and the hit test
already covers everything the composer can reach.
The decision is a pure function now, so it can be tested against a real DOM
instead of inferred from the effect.
Follow-up to #81920, which bounded the frost to the sheet and left the surface
underneath it unbounded. Nothing paints in the empty space above a short
transcript now, and clicks still die there.
Two reasons, both in this stylesheet. The shell's scaffolding — the shell
itself, the chat surface, the wrapper between them — is full-window,
invisible and hit-testable, so the click-through hit test found something at
every point in the window. And the band's box is the whole window by design
(it is the scroll container), so engaging the HUD turned that entire rectangle
into a click target, which on a fresh thread is a window-sized hole over
whatever you were working in.
So: default the shell to `pointer-events: none` and let surfaces opt in, and
clip the band's box to the sheet, which hit-testing honours. Opting in rather
than listing the scaffolding to exclude, because the scaffolding is not a list
anyone maintains — one more wrapper and the dead rectangle is back, whereas a
control that forgets to opt in is visibly dead.
In HUD mode Hermes is a strip over the app the user is actually working
in, so "what's under you?" or "look up the weather" is almost always
about that app — but the agent had no way to know it was floating, and
answered from its own browser and panes instead.
The desktop tags a HUD submit with `surface: 'hud'` and the gateway turns
that into a per-turn note pointing at read_window_below, and at carrying
the work out in the app underneath. It rides the model-bound message
beside the reaction and speech-interrupted notes rather than the system
prompt: one session can be driven from the app window on one turn and the
HUD on the next, and the system prompt has to stay byte-stable.
Every tool the note names is checked against the agent's own schema
first, so a session without computer_use or read_window_below is never
pointed at a tool it cannot call.
The speech-interrupted and reaction notes each hand-rolled the same
string / multimodal-list prepend. Collapse both onto _prepend_note, which
also gives the "model input only, never persisted, cache-safe" contract a
single place to be written down.
* fix(model-picker): serve cached custom-provider catalog on no-probe opens
#58183 stopped GUI picker opens from live-probing saved custom
OpenAI-compatible endpoints so a stopped local server could not stall the
picker. It gated the whole discovery block, not just the network call, so
`cached_fetch_api_models()` was skipped too — and with it the catalog an
earlier probe had already written to `provider_models_cache.json`.
A custom endpoint that is not the current provider therefore renders only
the models named in its config entry. A local server with 8 models loaded
shows the 1 model that was saved when the provider was first added, on
every picker open, while an explicit Refresh shows all 8.
Add `cache_only` to `cached_fetch_api_models()`: answer from disk within
the existing stale-serve window, never fetch, never revalidate off-thread,
return None on a miss. Split the three call sites in
`list_authenticated_providers()` into what the user's config permits
(`discover_models`, an explicit `models:` allowlist) and how we may obtain
it, so suppressing the probe now downgrades to a cached read instead of
skipping discovery outright. `discover_models: false` still pins, and a
cache hit no longer writes back to config since the probe that populated
it already did.
The latency win stands: a cold cache is a miss, so picker opens against
offline endpoints still make zero network calls.
* test(model-picker): pin the cached-catalog contract for no-probe opens
Cover both halves of the invariant, since fixing either one alone
reintroduces a bug the other guards against.
`cache_only` on `cached_fetch_api_models()`: a fresh entry and an entry
past its TTL but inside the stale-serve window both serve; an entry beyond
that window, an empty cache, rotated credentials, `force_refresh`, and a
missing base_url are all misses — and none of them fetch or spawn a
background revalidation.
`list_authenticated_providers()` on the GUI path: a non-current endpoint
with a warm cache reports its full catalog across all three provider
shapes (`custom_providers`, `providers:`, bare `provider: custom`) with no
live fetch attempted. A cold cache keeps the configured list and still
makes no network call, which is the #58183 guarantee. `discover_models:
false` keeps pinning, and a cache hit does not write back to config.
* fix: persist discovered custom-provider models in the hermes model flow
The `hermes model` named-custom-provider flow (_model_flow_named_custom)
probes the endpoint and shows the full catalog, but never persists it to the
entry's `models:` list. No-probe surfaces (dashboard, desktop, ACP) call
build_models_payload(..., probe_custom_providers=False) and only render the
configured `models:` list, so a provider added via `hermes model` collapses
to the single `model:` default everywhere except the CLI. OpenAI-compatible
providers added via a probing picker already benefit from
_save_discovered_models_to_config; the CLI flow did not.
Persist the live catalog after a successful probe, mirroring the picker path
in model_switch.py. A failed save is non-fatal.
* fix(model-picker): stop an auto-saved catalog pinning a keyless endpoint
The cached-catalog read added for no-probe picker opens still sat behind
the no-key discovery gate, so it never reached the shape that motivated
it: a keyless local model server.
`bool(api_key) or not has_explicit_models` is a network-cost gate. It
exists so Hermes does not probe an endpoint it cannot authenticate to
when that endpoint already declares its catalog (5f00f36ba, 1039e90b5).
Reading a catalog an earlier probe already paid for costs nothing, so
the gate belongs on the probe, not on discovery as a whole.
Left on the discovery side it re-pins the endpoint it was meant to
spare. A successful probe calls `_save_discovered_models_to_config()`,
which writes a plain list into `models:` — exactly the shape
`_models_config_is_allowlist()` reads back as an explicit user
allowlist. A keyless server therefore froze on the catalog of its first
probe and could never widen again, which is the "lineup changes after
config was written" case. f66319097 already carved the dict shape out of
this trap for the same reason; the list shape is the other door into it.
Move the clause to `_probe_live` at both custom-endpoint sites. Probe
suppression is unchanged — verified byte-identical to main across the
keyed/keyless x declared/undeclared matrix — and `discover_models: false`
remains the documented way to pin a catalog.
* test(model-picker): cover the keyless auto-save pinning trap
Three tests around the gate move, each failing on the code before it:
- a keyless endpoint carrying an auto-saved `models:` list still reads
its full cached catalog
- the same row, cold cache and probing enabled, still makes zero live
fetches — the network-cost gate the clause exists for
- an end-to-end round trip: persist a probe result via
`_save_discovered_models_to_config()`, reload it, and assert the shape
we wrote does not read back as a user pin
The round-trip test guards the whole chain rather than one branch, so a
future change that makes the saved shape look like an intentional
allowlist fails here even if the gate logic is refactored.
* fix(model-picker): key the custom-endpoint model cache by api_mode
`cached_fetch_api_models()` fingerprints entries with `api_mode`, but no
call site in `list_authenticated_providers()` passed it, so every custom
row resolved to the `api_mode=None` fingerprint. Two rows sharing a
base_url and credential but differing by `api_mode` are deliberately
distinct picker rows — it is part of `group_key` at both sites — yet they
collapsed onto one cache entry.
That was latent while probing was the only way to fill a row: a mismatched
entry was overwritten by the row's own live fetch. Serving that entry
without a probe makes it visible, so an `anthropic_messages` row could
render the catalog an OpenAI-mode row cached against the same URL. The
wire protocols differ (`x-api-key` + `anthropic-version` vs
`Authorization: Bearer`), so those catalogs are not interchangeable.
Persist `api_mode` on the group at both grouping sites — it is already
part of `group_key`, so it is constant across the group — and pass it
into the cache read. Section 3b (bare `provider: custom`) has no
`api_mode` in scope and already reads with the empty-credential
fingerprint, so it is unchanged.
Reported by Copilot review on #81973.
---------
Co-authored-by: xxxigm <tuancanhnguyen706@gmail.com>
Co-authored-by: Navlem <114683850+Navlem@users.noreply.github.com>
* fix(dashboard): retry stalled events feed reconnects
* fix(dashboard): bound the PTY ticket request before the socket exists
ChatPage's connect awaits a single-use ticket from `api.buildWsUrl()`
before `new WebSocket()`. That request produces no socket, so a
rejection or a hang emits no `close` event and never arms
PTY_CONNECTING_TIMEOUT_MS (set after the socket is constructed). The
tab stranded on "connecting" with `connectInFlightRef` stuck true,
which also suppresses the page-resume reconnect path.
Give the ticket phase its own deadline and route both failure modes
into the existing backoff. A `ticketSuperseded` flag invalidates a late
ticket result so a timed-out attempt cannot open a socket behind the
replacement it scheduled, and cleanup clears the timer on unmount.
`scheduleReconnect` now takes `number | null` so an attempt that died
before any socket existed omits the "(code N)" banner suffix instead of
inventing one.
Same bug class as the events-feed fix in the preceding commit, on the
main chat surface.
Co-authored-by: Gille <4317663+helix4u@users.noreply.github.com>
* test(dashboard): cover the PTY ticket connect deadline
Mirrors the events-feed cases in ChatSidebar.test.tsx: a rejected ticket
retries, a stalled ticket times out and its late resolution cannot open
a superseded socket, and a settled ticket disarms the deadline so
PTY_CONNECTING_TIMEOUT_MS remains the only guard on a wedged handshake
(NS-591 regression).
Both failure cases fail against ChatPage.tsx without the preceding fix.
Co-authored-by: Gille <4317663+helix4u@users.noreply.github.com>
---------
Co-authored-by: Gille <4317663+helix4u@users.noreply.github.com>
The Windows-aware path matching added for project ownership was a second
copy of what the file tree's IPC layer already had — same Windows test, same
containment check, one of them carrying a trailing-slash branch its own
normalisation made unreachable. Both now share lib/path-compare.
On a remote backend a new chat starts in the remembered workspace, and
setCurrentCwd persisted that key on every call — including the six paths
that merely follow a conversation (resume settling, warm switch, stored-row
preview, agent relocation, boot seed, resolved new-chat default). So opening
a chat inside a project quietly made that project the destination for the
next "New session", which is the half of the report the resolver fix does
not reach: a Windows desktop driving a WSL gateway is a remote connection.
setCurrentCwdTransient already meant "move the path, claim nothing" — the
following paths now use it, and setCurrentCwd is reserved for a workspace
the user actually named.
Project ownership compared paths literally, so a nested cwd failed to match
its project whenever the separator or drive-letter case differed — which on
Windows is routine. Normalise both sides for comparison only, folding case
for drive and UNC paths.
resolveNewSessionCwd() inherited the focused chat's workspace, so every
"New session" landed in whatever project you were last looking at — and
after a restart the focused session's stored cwd is often a home-dir
fallback, which shadowed the configured default project dir entirely.
The boot seed had a second failure mode: ensureDefaultWorkspaceCwd() only
seeds while no session is active, and it ran after gateway.connect() — the
same event that un-gates route-resume. On a slow start the resume won and
the seed silently skipped. Seed before connect instead, where no session
can be active yet, and keep both seeds non-fatal.
Widen of the PowerShell codepage cluster: the autostart registration
subprocess in tools_config.py was the last text=True capture in these
modules still decoding with the locale code page. Standardize on
encoding='utf-8', errors='replace' like the rest of the file (#53428).
A gate runs whatever command the operator configured, so its output is
arbitrary bytes. run_gate captured it with text=True and no encoding, which
decodes with locale.getpreferredencoding() under errors="strict".
One byte the decoder rejects — a test runner's checkmarks or CJK on a
non-UTF-8 Windows console, a stray binary byte anywhere in the stream — kills
subprocess's reader thread. proc.stdout comes back None, the `or ""` fallback
turns that into an empty tail, and an unhandled traceback is dumped to stderr.
The gate's pass/fail verdict still lands on the exit code, but the output tail
is exactly what the retry prompt feeds back so the agent can fix the failure.
With it empty the agent is told a gate failed and given nothing to act on, so
it burns every retry and the goal auto-pauses.
workspace_fingerprint has the same two calls; there a non-ASCII path in
`git status --porcelain` empties the fingerprint, silently disabling the
unchanged-gate skip that exists to stop a stalled agent re-running the same
red suite.
Decode as UTF-8 with errors="replace" — what git and modern toolchains emit,
and what 262 of the repo's 299 text-mode subprocess calls already do.
_run() used text=True without an encoding, so Windows decoded git's
UTF-8 output with the locale code page (cp932) and raised
UnicodeDecodeError on non-ASCII filenames or diff content, breaking
the "Never raises on git failure" contract in its docstring. Match
the utf-8 + errors="replace" policy checkpoint_manager's _run_git
already uses. Legacy cp932-encoded blob content degrades to
replacement characters instead of crashing; a test pins that
trade-off so it stays a documented choice.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
PowerShell's Get-Clipboard -Raw outputs text in the system's ANSI codepage
(e.g. CP1252, CP936), not UTF-8. When Node.js reads this with encoding: 'utf8',
non-ASCII characters (CJK, emoji, accented chars) are corrupted.
The write path already solved this by base64-encoding UTF-8 bytes and passing
them via -Command argument (see comment at line 94-98). This fix applies the
same approach to the read path:
- Change PowerShell read command to base64-encode the clipboard content
using [Convert]::ToBase64String([System.Text.Encoding]::UTF8.GetBytes(...))
- Add base64 flag to read command type for PowerShell entries
- Decode base64 result in readClipboardText when flag is set
Also adds child.unref() to spawned clipboard child processes in the write
path to prevent delaying process.exit() when the app exits mid-clipboard-write.
Fixes the read_file half of #80308 and the class behind #80261, #80250,
The binary sniff sampled files via 'head -c 1000' through the terminal
transport, which decodes stdout with errors="replace". A multibyte
character cut at byte 1000 therefore arrived as U+FFFD, and
_is_likely_binary treated any U+FFFD as binary — flagging valid CJK and
emoji text as unreadable. At the text layer a stored replacement char
and a transport-manufactured one are indistinguishable, which is why
per-callsite adjustments kept leaving siblings open.
Sample as 'head -c 1000 | base64' so raw bytes survive the transport
(fail-open to the legacy heuristic when the transport cannot produce
clean base64), then classify bytes: NUL => binary; valid UTF-8 allowing
one incomplete multibyte sequence at the sample end => text; mid-stream
invalid UTF-8 (latin-1, true binaries) => read-only, preserving the
anti-mojibake guarantee the old check existed for. Files legitimately
containing U+FFFD become readable.
Three desktop UI tests froze en-US-formatted strings while the
implementation formatters deliberately use the runtime locale
(new Intl.DateTimeFormat(undefined, ...) / Intl.NumberFormat(undefined,
...)) — runtime-locale output is the intended behavior for a localized
UI. On any non-en-US dev machine the tests fail even though the code is
correct:
# zh-CN host:
time.test.ts -> expected '三月' to be 'March'
billing -> Unable to find text 'Threshold: minimum is $10.'
(zh-CN renders USD as 'US$10')
billing -> Unable to find text '$25 added. Balance is refreshing.'
Assert the behavior contract instead of the frozen snapshot, per the
repo's testing guidance (behavior contracts over snapshots):
- time.test.ts: same-year month buckets render via fmtMonth, prior-year
via fmtMonthYear — assert sessionBucketLabel(bucket) equals the shared
formatter's output for bucket.at, with bucket-kind narrowing.
- billing/index.test.tsx: interpolate formatMoney(10) / formatMoney(25)
into the expected strings.
No production code changes.
Verified: zh-CN host 40/40, LANG=C.UTF-8 40/40, tsc clean, eslint clean.
test_cli_entrypoint_end_to_end copies add_contributor.py with
read_text()/write_text() and no encoding argument, so both fall back to
the system locale. add_contributor.py contains UTF-8 multi-byte
characters (an em dash), which makes the read raise UnicodeDecodeError
on any non-UTF-8 Windows locale (observed on cp950 / Traditional
Chinese). The trailing mapping-file read gets the same treatment for
symmetry.
Same footgun class as the subprocess text=True sweep in #60741, just on
the pathlib read_text/write_text side.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Two Windows bugs in scripts/run_tests_parallel.py:
- --files/--paths/HERMES_TEST_PATHS were split on ':', which shreds
absolute Windows paths at the drive letter ('C:\repo\tests' ->
['C', '\repo\tests']): the drive letter became a phantom discovery
root and the rooted remainder only resolved by WindowsPath
re-anchoring it onto repo_root's drive. New _split_pathspec() keeps
drive-letter colons glued to their path and accepts ';' (os.pathsep)
on Windows, while ':'-joined lists (CI generate job) keep working.
- With piped stdout (CI, subprocess capture) Windows encodes the
runner's output as the ANSI code page, so printing the per-file
progress glyphs raised UnicodeEncodeError inside the executor
done-callback and every progress line was silently lost -- which is
also why test_bare_value_flag_keeps_its_value failed on win32 (no
'1[check]' line, and the summary says '1 tests passed', which does not
contain '1 passed'). The runner now reconfigures its own
stdout/stderr to UTF-8 on Windows, and the tests decode the captured
output as UTF-8.
Adds regression tests: os.pathsep-joined absolute roots (all
platforms) and no-phantom-drive-root (win32).
Fixes#57149
`tests/hermes_cli/test_plugins_cmd.py::TestNoAutoActivation::test_compressor_default_ignores_plugin`
fails on every Windows machine:
UnicodeDecodeError: 'charmap' codec can't decode byte 0x8f in
position 47744: character maps to <undefined>
The test reads `run_agent.py` back as text to assert a removed comment is
gone, but called `open()` with no `encoding=`. Python then falls back to
the locale preferred encoding, which is cp1252 on a default Windows
install rather than UTF-8. `run_agent.py` contains nine bytes cp1252
leaves undefined, so the read raises before the assertion is reached. On
Linux and macOS the preferred encoding is UTF-8 and the same line is
fine, which is why CI never caught it.
That one line is the only active failure. The rest of this change closes
the same gap in the files it touches, which `scripts/check-windows-footguns.py`
flags and which the #71014 read_text campaign has been working through
elsewhere in the tree:
- `tests/hermes_cli/test_plugins_cmd.py`: nine bare `write_text`/`read_text`
calls writing YAML manifests, config and plugin sources
- `tests/tools/test_web_tools_truncate.py`: reads stored extracted web text,
which is arbitrary content from the internet
- `tests/stress/test_atypical_scenarios.py`: writes and reads worker task
ids and a barrier file
All three files are now clean under `check-windows-footguns.py`.
Reads go through `Path.read_text(encoding="utf-8")` rather than
`open(...).read()`, which also closes the handle instead of leaving it to
the garbage collector. On Windows a live handle blocks tmpdir cleanup, so
that part is not cosmetic either.
No new test. The repaired test is the regression coverage: it fails
before this change and passes after, on Windows.
ruff PLW1514 (already enforced repo-wide via the blocking lint step)
covers open()/Path.open()/read_text()/write_text() but NOT os.fdopen —
the exact hole the AlexFucuson9 sweep PRs (#56033#56940#65565) kept
patching by hand. Add an fdopen rule to check-windows-footguns.py, which
also runs as a blocking CI step, so a bare text-mode fdopen fails CI.
Also fix a false-negative in the read_text/write_text rule: chained
forms like `read_text()[:4000]` or `read_text().splitlines()` never end
the line with `)` and slipped past the multi-line-call heuristic.
Replace the endswith check with a paren-balance walk (keeps multi-line
calls with encoding= on a continuation line unflagged — verified against
the full tree). This makes the rule the effective standing replacement
for the standalone checker proposed in PR #66669: R1-style coverage now
lives in PLW1514 + this script, both blocking in .github/workflows/lint.yml.
Sabotage-verified: reverting agent/shell_hooks.py's fdopen encoding or
tools/skills_tool.py's read_text encoding now fails the gate.
Co-authored-by: AlexFucuson9 <AlexFucuson9@users.noreply.github.com>
Co-authored-by: Paulo Nascimento <pnascimento9596@gmail.com>
Teknium
Brooklyn Nicholson
webtecnica
Ben Barclay
hermes-seaeye[bot]
Austin Pickett
Gille
Dan Bennett
David Metcalfe
Drexuxux
Sora-bluesky
annguyenNous
Ayush Nangia
ypQQ1984
Michael
linhongkuan
Aniruddha Adak
Jeff Watts
Adolanium