openclaw/openclaw    ·  21 merged

• P0 release blocker: OpenClaw installed but the managed Gateway never started for Windows users whose profile path holds non-ASCII characters outside the CJK range — the generated .cmd launcher was written in UTF-8, but cmd.exe parses batch files with the boot-time OEM code page. Fixed across 15 OEM code pages; verified on a Turkish host (ev-yiğit-öğün, ACP 1254 / OEMCP 857) going from MODULE_NOT_FOUND to a clean start, with byte-identical output on CJK hosts (PR #108967)
• A compacted session's summary grew a copy of itself every cycle, burning tokens on each following turn: whenever a later split degraded, staged compaction re-added a summary chunk 0 already carried (occurrences 2 → 1 after the fix). Fixed in 14 source lines by keying the fallback on whether the oldest split degraded — the one signal the consumer actually asks for — with regression coverage at both layers (PR #109828)
• Every file an agent created on Windows came back lowercased — any name not already lowercase — so the import it had just written broke for all teammates on Linux and in CI; Turkish names like İstanbul.md came back corrupted, not merely lowercased. The sandbox helper was returning a comparison key as a real path — fixed with a case-preserving normalizer, every boundary verdict proven unchanged (PR #109823)
• Every truncated turn was billed by the provider but recorded as free — zero tokens, zero cost, and a stop reason marking it successful, so nothing downstream could retry or warn and per-session accounting drifted low. Any answer hitting the output cap ends incomplete, and that event matched no terminal branch in the agent transport. Fixed by finalizing both terminal events through one canonical usage mapper shared with the package side (PR #109904)
• A reply whose code fence opened with a long info string was dropped outright: the chunker reopened the block on every continuation with that full opening line while budgeting only the closing marker, so chunks ran past Discord's 2000-character limit and were rejected with HTTP 400. On a worst-case fence, 1534 chunks with 41 over the limit became 4 that all send, with the body intact instead of 41 of 1290 characters surviving (PR #110148)
• Invalid credentials took 7093ms and 4 requests to report; now 3ms and 1. The retry loop threw its non-retryable errors inside the try whose catch treats everything as a retryable network failure, swallowing its own classification. Rethrown as a distinct type, same message to the user (PR #110655)
• Reef died permanently to relay rate limiting and had to be restarted by hand — a throttled startup failed the whole account, the supervisor's ten restarts each hit the same relay, and the retry traffic fed the throttling that caused it. Startup now shares the periodic reconcile's failure policy: the account comes up on the peer keys it already has and refreshes on the next interval (PR #110918)
• A rate-limited turn failed instead of simply waiting out the server's cooldown: when a 429 carried an unparseable retry-after-ms, the valid Retry-After sitting next to it was never read, so the client fell back to blind exponential backoff and burned its attempts inside the window. The two headers are ordered preferences, not alternatives — returning only on a successful parse restored that. Measured against a real socket, the retry gap went from 1016ms of blind backoff to the 3018ms the server asked for (PR #111353)
• Coloured command output came back as literal escape codes — npm install, a coloured test run, a docker build — whenever a sequence happened to straddle a stream read, and the corrupted text landed in the transcript the model reads. The remote bash path already kept a per-stream ANSI parser; the local exec runtime still called the stateless helper on every chunk. Wired the same parser in, with separate state for stdout and stderr so one stream can't consume the other's pending sequence — proven against a real child process splitting a sequence across a real pipe (PR #111364)
• Every OpenRouter agent turn dropped its cache-write tokens and billed them as ordinary input — the agent lane hard-coded cacheWrite: 0 while its sibling parser three files over already read the field and documented the contract. Correcting the mapping alone would have silently shrunk every overflow decision on that lane, because the context-overflow fallback was accidentally exact only while input absorbed the writes — so both land together, proven over a real socket with a real transport: 80/0 → 70/10 tokens, and a real overflow that the half-fix reports as false (PR #111435)
• A browser that never came back permanently bricked tab tracking for its profile — from then on every browser open opened a tab, closed it again, and errored, with manual state clearing the only recovery. Cleanup defers whenever it cannot prove ownership of a tab and nothing ever dropped a row by age, so rows for a dead browser were re-claimed, failed and deferred every 5 minutes forever, until they filled the tracking store's 5,000-row reject-new cap. Bounded the retry by retiring only rows whose ownership probe already failed and that have gone unused past a 24h window — unlike a namespace TTL, which would also expire tabs that are alive and reachable. Proven end to end against real Chrome over real CDP and real SQLite, across two processes so the row is read back after a restart, with now the only injected input (PR #111307)
• Host execution blocked CC, CPP and CXX as compiler selectors but still accepted CXXCPP — GNU Autoconf's C++ preprocessor selector, the exact counterpart of the CPP that was already blocked everywhere, so an operator-chosen preprocessor executable could still reach a host build through the inherited or the requested environment. Closing the C++ half of the same rule makes the boundary explainable instead of accidental: one canonical policy key, mirrored into the generated Swift policy and the reported baseline (265 → 266 entries). Proven with the production sanitizer against a real child process — inherited and requested CXXCPP both become null, the requested one is reported in rejectedOverrideBlockedKeys, benign controls survive and the child exits 0 with no CXXCPP in its environment (PR #112684)
• The edit tool refused a perfectly unambiguous edit as ambiguous — "Found 2 occurrences … the text must be unique" for text that occurs exactly once — whenever another line in the file differed only by trailing whitespace, a smart quote, an en-dash or a non-breaking space, which files mixing straight and curly quotes hit routinely. The match and the safety check ran in two different string spaces: the exact path found the text in raw content, then the uniqueness gate counted after normalization had folded away the very distinctions the match relied on. The cost fell on the model, which is told to add context and so retries with a larger oldText that spans another near-duplicate and is refused again. Counting in whichever space the match was actually found in fixes it, with a control case proving a genuinely fuzzy-ambiguous edit is still refused (PR #115738)
• The compat layer that makes xAI, Venice, Fireworks and LM Studio models usable reported success while leaving the banned keyword in the request — the provider refused the tool call anyway, so from the user's side the model simply could not call the tool and the setting looked like it did nothing. The strip walked five schema containers and copied everything else through verbatim, so a keyword nested under additionalProperties, prefixItems, patternProperties, contains, $defs and six more survived — and additionalProperties holding a schema is the ordinary way to describe a dictionary, common in MCP tool definitions. Fixed by mirroring the container sets its own caller already agreed on. Proven over a real socket through the real transport, no fetch stub: additionalProperties arrives as {"maxLength":100,"type":"string"} on main and {"type":"string"} here, with three regression tests that each fail against the unfixed source (PR #115741)
• One unparseable streaming frame pasted the model's own output straight into the error surface — tool results, file contents it had just read, any credential it had generated — because the Anthropic provider built its parse-failure message out of the raw frame, embedding both the data payload and every raw line. The answer already existed one directory over: the canonical Anthropic transport converts only a SyntaxError into a shared malformed-fragment marker and keeps the original error as cause, carrying no payload. What makes this a broken user-facing contract rather than a merely verbose error is who reads that marker — the shared assistant-error formatter matches it by exact string equality and swaps in "LLM streaming response contained a malformed fragment. Please try again.", so the provider path, emitting a different string, never reached that substitution and the operator saw the fragment itself. Aligning the provider with the marker took the operator-facing text from 402 bytes of echoed payload to a 54-byte retry message. The neighbouring throw is deliberately left alone: its structured error body is parsed downstream into a meaningful operator message, so redacting that one would be the regression. Proven over a real loopback SSE server through the Anthropic SDK and the production provider stream, with a before/after mutation run showing the sentinel leaking on the base behaviour but not on this branch, and a well-formed stream as the control case (PR #116938)
• The same broken contract, one provider over: a malformed frame on a ChatGPT-login model surfaced the parser's own internal wording instead of the actionable retry message every other provider gives, because the Codex SSE boundary rethrew JSON.parse's failure as a string of its own. Three separate consumers match the shared malformed-fragment marker by exact string equality — the assistant error formatter, the sanitize path, and the fallback sitting directly under the embedded agent's "never return raw unhandled errors" comment — so none recognised it and that last one returned the raw text verbatim. Fixed the way the canonical Anthropic transport already does it: catch only JSON.parse's SyntaxError, convert it to the shared marker, keep the original as cause, and yield outside the catch so a SyntaxError injected by a consumer through iterator.throw() still propagates untouched. The WebSocket twin in the same file is deliberately excluded, with the reason written into the PR — a line-level scan of open PRs found one asserting on that exact message text and another restructuring the block. Operator-facing text went from 79 bytes of parser wording to the 54-byte retry message, proven over a real loopback SSE server through the production stream, with the mutation run as the before column and a well-formed stream as the control (PR #116966)
• An oversized Google auth response left its socket open behind the expected size error, and nothing was logged — the size guard in the Google Chat auth transport reads content-length and throws before it ever reaches response.body.getReader(), so the finally released the SSRF-guarded dispatcher with a live, unread body sitting behind the failure. I found this second guard site while reviewing the fix for the first one (PR #111290, someone else's): that one merged covering only the API wrapper, and this is the follow-up it invited — same predicate, same deliberately non-awaited cancellation, so both guard sites in the extension now read alike, and only the guard's own rejection path is touched. Proven with a loopback server, the real guarded fetch and the real release, with the wrapper only recording what release() observes before delegating: bodyUsed goes from false to true on the oversized path, while a normally read response is unchanged as the control (PR #115873)
• The assistant’s own prior turn was replayed to the model with its sentences fused together on every OpenAI-compatible provider — OpenRouter, Groq, DeepSeek, Together, LM Studio, Ollama — because convertMessages flattened a multi-block assistant turn with join(""). Two text blocks came back as "Let me check the file.The file contains X.", and since that corrupted text is what the model reads as its own previous turn, the damage compounds with every subsequent request. Two text blocks in one turn is routine rather than a corner case: streaming opens a new block after any tool call, and cross-model replay converts a thinking block into a text block adjacent to the real answer. Every neighbouring path already disagreed with the choice — the thinking blocks a few lines below join with `

, and flattenCompletionMessagesToStringContent, the helper performing this exact operation for strict OpenAI-compatible servers, joins with — so the fix follows the closest sibling instead of inventing a separator. Proven on the wire rather than in a mock: a realnode:httpserver stands in for the provider, parses the actual request body and answers with a real SSE stream through the realstreamOpenAICompletionspath, showingfile.The fileonmainagainstfile. The file` here (PR #115743)

• Generating speech produced an unplayable file instead of the provider's error on xAI, Gradium, Azure Speech and OpenAI: whenever one of them answered HTTP 200 with a body that was not audio — a JSON error, an application/problem+json payload, an HTML sign-in or captcha page, or zero bytes — that body was read straight into the buffer and delivered as a voice message, while the provider's actual message was discarded. The repository already owns this contract and one speech path already used it, so whether a malformed 200 was caught depended only on which provider a user happened to route through. The split form (assertProviderBinaryResponseContent + readResponseWithLimit) is applied rather than the combined helper, because the combined one hardcodes its own overflow handler after spreading caller options and would have rewritten each provider's existing byte-cap message — the same reason two video-side extensions use the split form. Cancellation stays deliberately non-awaited: under debug-proxy capture the body is one branch of a Response.clone() tee, and cancelling such a branch never settles while its sibling is live. Proven against a real node:http server that answers 200 and then never ends the body, driven through the real global fetch and the real SSRF guard with no stubs: the unguarded path hangs to its deadline with the socket still open (5029 ms) against a named error and an operating-system-observed socket close here (120 ms). The mutation control shows the defect in its rawest form — with the guard removed the malformed cases resolve to Buffer[ 123, 34, 101, 114, 114, … ] and Buffer[ 60, 104, 116, 109, 108, … ], which are {"err and <html handed back as the audio a channel then sends. The maintainer's follow-up added the fourth owner, extensions/openai/tts.ts, and audited all eight bundled TTS owners to confirm the gap was exactly these four (PR #117345)
• Kilocode models were registered with a context window larger than the model can actually accept — by up to 3.8x, on 33 of the 335 models the live catalog serves. Kilocode's gateway returns an OpenRouter-shaped catalog in which context_length is the catalog-wide ceiling across every routing candidate, while top_provider.context_length describes the primary provider that actually serves the request; discovery read only the first, so context budgeting and the model metadata users see both overstated what a request could use. The repository already owns the correct precedence — extensions/openrouter/provider-catalog.ts prefers the primary provider and falls back to the catalog-wide value — and the same normalization had just landed in src/agents/model-scan.ts (PR #110855, someone else's, where I had measured and resolved the merge conflict); the Kilocode reader was the surface that pass missed, so this is a completed rule rather than a new one. What made the claim measurable rather than plausible is that the catalog is public: api.kilo.ai/api/gateway/models answers with no key and no inference call, so the real 346-row response was fed through the actual discoverKilocodeModels() implementation with the production file as the only variable between the two runs — nvidia/nemotron-3-super-120b-a12b registers 1000000 against a primary provider offering 262144, minimax/minimax-m3 1048576 against 524288, and both come back correct on this branch. Completion tokens are deliberately left untouched: enumerating the key union of every row in that same live response shows no top-level max_completion_tokens or max_output_tokens anywhere, so copying the sibling fix's fallback chain would have encoded a field the data never carries — which is why the production delta is 6 lines added and 1 removed against 53 lines of tests. The added coverage is shown load-bearing by reverting the production file to main while keeping the new tests, which fails the precedence case with 1048576 where 524288 is expected (PR #118868)
• Generating video through BytePlus or Runway hung until a surrounding timeout instead of reporting the provider's own failure, for anyone running with the debug proxy enabled — the path that rejects a successful HTTP 200 whose body is not video (a JSON error, an HTML sign-in page) never returned, so the malformed video response error the provider already knew how to raise never reached the user. The debug proxy installs a global-fetch patch that clones every captured response, and the repository states the consequence in its own source: cancelling one branch of a tee "never settles (it only resolves once BOTH branches cancel)" — yet both providers awaited exactly that cancel, while the sibling branch was still being read by a capture that is deliberately not awaited. Fixed with the fire-and-forget release the other generated-media owners already use, the same decision merged for the Ollama setup path in #111802; scope held to the two rejected-download call sites, with validation, error wording and byte caps untouched. My first proof built the tee by hand with clone() and the review refused it — correctly, since a modeled tee never exercises the shipped patch — so the second round measured the real thing: initializeDebugProxyCapture() asserted installed, a real node:http loopback origin that sends headers and then never ends the body, and the real provider entry point downloading over real undici through that patched global fetch. Capture on with await cancel() returned no result inside a 10-second budget; the other three cells of the 2×2 matrix all failed in 14–64 ms with the intended error, which is what isolates debug capture rather than the loopback origin as the cause. The regression tests are shown load-bearing by reverting only the two production lines: both fail at the deadline rather than on an assertion, and the pre-existing cancellation tests pass either way — their stubbed cancel resolves immediately, so they could never see this (PR #119257)

n8n-io/n8n    ·  2 merged · both released

• Every Salesforce Case given a Parent ID still landed with ParentId: null, and the node reported success — the field is declared ParentId in the node description, but both the create and update handlers read the lowercase parentId off the collection, so the key was always undefined and the parent was never put on the request. Nothing surfaced the loss: Salesforce was simply never told. Reading the correctly-cased key restores it with no migration, since saved workflows already store the value under ParentId — and the two existing tests that had mirrored the buggy lowercase key were corrected alongside new regression tests pinning create and update (PR #33775, shipped in n8n@2.32.0)
• An AI agent whose HTTP tool call failed was told the status code and nothing else — never the server's own explanation, so a 403 carrying {"error":"insufficient_scope","required":"read:users"} reached the model as a bare "Forbidden" and it retried blind instead of correcting the request. The tool built its response from httpCode plus error.message, which left the branch that returns the body unreachable. Confirmed against a real NodeApiError assembled from an axios-shaped 403: cause and response both come back undefined while the body sits untouched on context.data — the payload was present the whole time, just never forwarded. The body now reaches the model, bounded on every axis that could turn a failure into a worse one: truncated so a long error can't eat the context window, binary and empty payloads skipped, credential-shaped values masked with the redaction patterns n8n already applies to skill tool output rather than a scheme invented for this path, and a serializer that cannot itself throw while an error is being handled (PR #34509, shipped in n8n@2.34.0)

huggingface/transformers    ·  3 merged

• Anyone fine-tuning GIT since v4.49.0 trained it to predict two tokens ahead — GitForCausalLM shifted its labels by hand and then passed them positionally, so shift_labels stayed None and the loss helper shifted a second time; because the manual shift flattened to 1-D first, the pad-and-slice kept shapes consistent (N → N+1 → N), nothing raised, and each row's final target was silently pulled in from the next row in the batch. GIT can't simply drop the manual shift the way the earlier Moonshine fix did — its logits carry leading image positions that must be sliced regardless — so shift_labels is passed explicitly, on 2-D tensors, which also removes the cross-row leak. Loss went from 4.5848 (matching the double shift) to 4.6461, exactly the aligned cross-entropy (PR #47395)
• Seven multimodal models raised outright under mixed precision — Trainer(bf16=True), or any Accelerate autocast context — because they moved the encoder output to the text stream's device before masked_scatter but not to its dtype. nn.Embedding is not on autocast's cast list, so inputs_embeds stays float32 while the encoder's final nn.Linear returns bfloat16, and masked_scatter requires both operands to share a dtype: under the ordinary mixed-precision setup, not an edge case. Rather than guess the blast radius from model names, I listed all 121 masked_scatter call sites under models/ — 104 already align the dtype, 17 do not — and put every one of the 17 through the same autocast forward, built from the model's own ModelTester with no pretrained weights: seven raised, three were already safe because their scattered tensor comes from an embedding table in the same module, and the rest scatter structurally different things. The PR covers exactly the seven that raised, with the excluded sites tabulated in the body and the already-fixed Gemma 4 path kept as a control; pi0 is a vision model, so the "audio family" cut I started with would have missed it (PR #47673)
• Two more models raised under the same mixed precision setup, and the sweep that fixed those seven could not have reached them — kosmos2 and kosmos2_5 move the vision features to the text stream's device before merging them into inputs_embeds, but not to its dtype, so under Trainer(bf16=True) or any Accelerate autocast context the merge dies with Index put requires the source and destination dtypes match. Same root cause as #47673, different operator: that PR's scope came from enumerating every masked_scatter call site, and these two models merge with an advanced index assignment, which lowers to index_put_ — just as unable to type promote, and outside that enumeration by construction. Neither has a modular_*.py either, so nothing propagated into them from a sibling. The fix is the one idefics2, idefics3 and modernvbert already apply at the identical merge point, and both models were put through a bfloat16 autocast forward built from their own ModelTester, raising before and clean after. smolvlm carries the same defect, but #41485 is already open against that file, so it is named in the PR body and deliberately left to that author instead of being duplicated (PR #47691)

langchain-ai/langchain    ·  1 merged · co-authored

• A one-character typo turned a human-approval gate into unattended execution. HumanInTheLoopMiddleware lets you put a risky tool behind a human by listing it in interrupt_on, but InterruptOnConfig is a TypedDict, so nothing checked those mappings at runtime — and the resolution loop kept an entry only while allowed_decisions was truthy. interrupt_on={"delete_database": {"allowed_decision": ["approve"]}} — missing the s — was dropped silently: after_model returned None, and the tool ran with no interrupt, no error and no warning. The same held for an empty allowed_decisions and for a config carrying only when or description. The user's intent is unmistakable in every one of those spellings, which is what makes this the worst possible failure direction for a safety middleware: the misconfiguration disables the protection it was written to configure, and nothing surfaces it. I reported it (issue #38838) and wrote the fix — move the check to construction, so an entry that would have been silently discarded raises instead. My PR (#38959) was auto-closed 23 seconds after opening by the repository's require-issue-link bot, which closes any PR whose author is not assigned to the linked issue; the report was then picked up by @imnishitha, who landed the same construction-time validation as PR #39247 and merged it — ValueError naming the offending tool and echoing back the keys the config actually carried, which is what turns a typo from a side effect discovered in production into a message at startup. Credited as co-author on the merge commit (5909891)

NVIDIA/TensorRT-LLM    ·  2 merged

• SamplingParams(top_p=float("nan")) passed validation and reached the sampler — the same for min_p and temperature — because those three range checks were written in the positive form (value < low or value > high), and every comparison against NaN returns False, so the guard whose entire job is rejecting out-of-range values waved through the one value that has no place on the range at all. Nothing downstream re-checks it: the request is accepted, the value flows into the sampling path, and what the user gets back is unusable output instead of the ValueError the API already knows how to raise for top_p=1.1. The fix is the negated form (not 0 <= top_p <= 1, not 0 <= min_p <= 1, not temperature >= 0), which for every non-NaN input is the exact complement of the condition it replaces — the accepted set is unchanged at every boundary, and NaN is the only value that moves. That form was not invented here: the top_p_decay and top_p_min checks a few lines below in the same method already use it, so what reviewers were asked was not "should this behaviour change?" but "why does this line disagree with its neighbours?" — deliberately the shorter argument, and the one that carries a merge. Scope was held to that: temperature=inf stays accepted because it flattens the distribution rather than corrupting it, and the small-temperature clamp requested by #15715 is left alone because it changes the behaviour of valid input and belongs in its own PR. Filed first as issue #17158, since this repo enforces issue-before-PR in practice. The regression tests pin all three NaN cases plus out-of-range and in-range boundary controls — and they arrived unable to run: tests/unittest/llmapi/test_sampling_params.py existed but was in no test list, so a second commit registered it in the L0 CPU pre-merge list, which also put the file's pre-existing tests into CI for the first time. Merged after a full L0_MergeRequest_PR run (PR #17159)
• A streamed response that ended mid-tag silently lost the last characters the model produced — DeepSeekR1Parser.parse_delta withholds a trailing fragment that could still grow into a <think> / </think> delimiter, parking it in self._buffer until the next delta arrives. BaseReasoningParser.finish() exists precisely so a parser can flush that state when the stream ends, and the serving layer does call it (serve/postprocess_handlers.py:177, serve/responses_utils.py:963) — but DeepSeekR1Parser never overrode it, so a response ending in a literal <, or one cut off by max_tokens partway through </thin, dropped those characters out of content or reasoning_content with nothing raised and nothing logged. The blast radius is every parser key backed by that class — deepseek-r1, qwen3, qwen3_5, laguna, minimax_m2, minimax_m2_append_think — plus kimi_k2, minimax_m3 and deepseek_v4, whose finish() delegates to the base parser and was therefore a no-op until now. What makes it defensible in 25 lines is that the correct implementation was already in the file twice: NemotronV3ReasoningParser.finish() and Gemma4ReasoningParser.finish() both flush exactly this way, so I wrote the new one in Gemma4's shape rather than a tidier variant of my own — the question put to reviewers was not "is this behaviour right?" but "why does this class not honour a contract its siblings already do?", and parse_delta was left untouched. A buffer holding exactly a complete tag is a delimiter rather than model output, so it is still discarded, which keeps the existing handling of a stray closing tag as the final delta. The tests are where review actually landed: I shipped 37 parametrized cases, and the reviewer asked for the minimum set instead, because CPU pre-merge runtime is a cost every PR in the repo pays — a good rule, and one I had inverted. Cut to 6, keeping a streaming-vs-non-streaming equivalence property test that subsumes the example-based ones, one (parser_key, text) pair per branch of finish(); 4 of the 6 fail against main, the other 2 exist to catch a wrong fix that leaks a delimiter or flushes from the identity parser. Writing that equivalence test turned up something wider than the bug I had filed: enumerating every string over ["a", "<think>", "</think>"] up to length 3 and comparing parse(text) against streaming it character by character, 20 of 39 texts diverge for qwen3 and 13 of 39 for deepseek-r1 — none of it a regression from this PR, so it went out as its own report (issue #17296) rather than expanding this one. Filed first as issue #17156; merged after five L0_MergeRequest_PR runs, the first of which failed on two AutoDeploy MoE tests that were a known main breakage waived hours earlier than my branch's base (PR #17157)

koala73/worldmonitor    ·  6 merged · 1 prototype

• A CI job went red on an assertion about a Redis transport, in a file the change under review did not touch — 22,600 tests passed and one lost a race by about a millisecond — the endpoint rate limiter arms two deadlines whose order is load-bearing: ENDPOINT_RATE_LIMIT_TIMEOUT_MS bounds the SDK's availability-first decision race, and ENDPOINT_REDIS_ABORT_TIMEOUT_MS arms an AbortSignal.timeout that cancels the underlying Upstash fetch just before it. The abort has to land first, or the decision returns while the request is still pending in the isolate — which is exactly what the failing test asserts. The reason it is a race at all is that the two timers do not start at the same moment: timeout starts when limit() is called, but the abort signal comes from a per-request factory the Upstash client only invokes when it builds the request, so the real comparison is armingCost + abortMs against decisionMs and the configured gap has to cover that arming cost. In production it does, comfortably — the gap is 500 ms and arming takes a few milliseconds. The test-context pair mirrored the production ratio rather than its margin, at 25/20, leaving 5 ms against an arming cost that under the node runner is 8–71 ms, because every case re-imports the module through tsx with a cache-busting query. Rather than call it flaky and ask for a re-run, I instrumented the exact path with the test's own stub fetch: the abort was winning by 1.1–16.3 ms across ten runs on an idle machine, which is not a margin but a coin flip, and --test-concurrency=16 on a two-core runner is what flips it. Widening the test-context decision deadline to 250 ms takes the headroom to 164–220 ms; the production values are byte-identical and the abort stays at 20 ms, so failure paths still resolve fast and the one case that genuinely waits out the deadline costs 0.12 s more. I rejected the obvious alternative of polling for the abort with a bounded deadline, because it quietly changes what the test claims — the name is abort … before failing closed, and an eventual-cancellation assertion would pass even if the ordering regressed for real — and rejected scaling the production ratio down (250/225), because a 25 ms gap is still inside the observed arming range. The new gap is pinned by a mutation-proven guard: revert 250 to 25 and it fails with gap is 5ms (PR #6405)
• The GDELT intel seeder had two readers of the same stored timestamp, and only the one that did not need a forward-skew guard had it — rankTopicsForFetch clamped every stored fetchedAt to the run clock before ordering the fetch queue, while contentMeta — the reader whose newestItemAt is what maxContentAgeMin: 1440 is actually evaluated against — took a bare Date.parse and accepted any finite positive result. The path carrying the guard was the path that could not alarm; the path that feeds the alarm had none. I corrected the issue's own premise in the PR body rather than shipping quietly against it: it claims a future-dated stamp keeps the cohort falsely fresh indefinitely, and that is not true — api/health.js already folds a negative content age into contentStale, so a wildly future stamp surfaces as STALE_CONTENT today. What the missing clamp actually costs is narrower and still worth closing: the length of the skew window, during which the stamp is ahead of the run clock but not yet negative and the cohort reads fresh; a newestItemAt on the wire that an operator cannot reconcile with the ordering view of the very same stamp; and a pre-epoch ISO string, which parses negative, passes Number.isFinite and would have been taken as the oldestItemAt, since contentMeta's ms > 0 filter caught exactly 0. The fix is one extracted parseStampMs that both paths call, with the clamp conditional so an injected test clock and Date.now() behave identically — six mutants, no survivors, and the starvation fixture is the real incident from the linked issue rather than invented numbers. Defect 1 of the same issue — newestItemAt is a Math.max, so one refreshed topic holds the alarm green while five starve — I deliberately did not implement, because the issue names two shapes for it and the one it prefers changes a content-age contract mirrored across three files behind a parity verifier; I costed all three options with file:line and offered to build whichever he picked. He asked for Allow edits by maintainers, so I enabled it and answered with the branch state rather than a rebase he had not asked for. His follow-up commit found the failure mode I had missed: clamping a poisoned persisted stamp still mints fresh health evidence through the cache merge, so the health reader now rejects anything beyond a one-hour skew tolerance — contentMeta returns null, newestItemAt publishes as null, and the classifier reads that as STALE_CONTENT, which is the fail-closed direction — while the ordering reader keeps the clamp, because it needs a usable sentinel rather than a rejection. That commit also changed runSeed to hand every seeder's fetcher an immutable run clock, so I audited the blast radius instead of assuming it: all 109 named runSeed fetchers, 92 of which take no parameter and 10 of which destructure an options object with defaults, none declaring the new key. The single red check was an unrelated flake in a rate-limiter test, and rather than call it flaky I measured it — the abort deadline was winning its race by as little as 1.1 ms, because the abort signal is armed lazily while the decision timer starts immediately — and sent the margin fix out separately as #6405 (PR #6044)
• The analytics collector's outage alarm sat at its ceiling through the busiest, entirely healthy hour of the day — 67,394 events, zero downtime, 60 of 60 windows breached. The check compared a raw quotient against a fixed 0.5 on a denominator that could be as small as 5: failures / writes >= 0.5 reads the same for 5/5 as for 5000/5000, and those are not the same claim — at the old floor the 95% interval half-width at p = 0.5 is ±0.368, which is to say the rate was not resolved at all. But no sample-size correction alone explains the maintainer's evidence, because that window's rate really was above 0.5; it was simply normal, since ad-blockers make a stable share of collector writes fail for reasons that are not an outage. So the fix is three parts and invents no constant: a Wilson lower bound in place of the point estimate, so sample size becomes part of the claim rather than a footnote; a MIN_WRITES floor derived as the smallest n whose half-width falls under ±0.15 (n = 31 at 0.1477, n = 30 at 0.1502) rather than picked; and the window judged against that cohort's own prior baseline instead of a universal number, so the alarm fires on a departure from normal. MIN_FAILURE_RATE was deliberately left at 0.5 and handed back with the reasoning, because choosing it would have meant inventing a calibration from production data I do not have. A side finding: api/analytics-health.test.mjs was in no test script — test:data globs tests/ plus a fixed list, test:sidecar names api files by hand — so the file existed, carried assertions and had never gated a single merge; wiring it in took the sidecar suite from 306 to 329. What followed is the part worth recording: the maintainer ran mutants against my suite himself and returned the deepest review this account has had, with one real blocker — at p = 1 the Wilson upper bound is exactly 1, not merely clamped, while the observed lower bound at p = 1 is 1/(1 + z²/n) < 1 always, so a saturated baseline makes the comparison permanently unsatisfiable; and since the counters are client-supplied, that state is reachable on purpose. He had the verified fix and could not push it, because Allow edits by maintainers was unchecked — so I enabled it on the open PR (gh api -X PATCH … -F maintainer_can_modify=true works fine post-creation, contrary to the usual folklore), rebased so his patch would land clean, verified his math independently rather than reimplementing it, and spent my remaining effort on the one question he said he could not settle without operator data. He pushed the hardening onto my branch — per-hour baselines instead of a daily mean, a veto ceiling so a saturated baseline loses its veto, bounded browser reports — and merged (PR #6041)
• A single newline anywhere in an RSS item forged an extra row inside the analyst prompts — and in the brief seeder's digest block that forged row carried a story hash the feed itself chose — sanitizeForPrompt preserves a lone \n by design: it splits on newlines to drop role-prefixed lines, rejoins, then collapses only runs of 2 or more whitespace characters. That is correct for a prose body and wrong at every site where the newline is the delimiter of the block being composed, and four such sites were still unguarded. Prediction-market titles are composed into - "${title}" — Yes N% (V volume) rows under a header the deduction prompt presents to the model as crowd-calibrated evidence, so a title carrying a newline mints a market with an attacker-chosen probability and volume; the Headline: / Description: / Source: rows of the why-matters prompt take the same shape, fixed inside sanitizeStoryFields rather than at the call site because the legacy relay path composes an identical row set from that same function — one fix, both paths; _country-brief-context.ts interpolated item.title raw, so it was missing the content sanitization as well, and its sibling block two functions above is safe only accidentally, because JSON.stringify happens to escape the newline. The Railway seeder's buildDigestPrompt was the sharpest of the four: its system prompt instructs the model to key its output off the [h:<hash>] token of each row, so a forged row there does not merely add noise — it introduces a numbered story whose hash the attacker picked, into a brief the product treats as generated from real ones. The upstream brief-compose.mjs does sanitize those fields, but with sanitizers that keep a lone newline: the delimiter guard has to live where the delimiter is. The tests assert the row count the payload itself declares, not the absence of the forged string — a guard that only greps for the payload passes for the wrong reason the moment that string is echoed anywhere — and every guard was reverted one at a time, nine mutants, no survivors. The one residual I could not close without changing the shape of a prompt I named in the body instead of quietly breaking the test that pins it: Context: is the block's single free-prose sink, whose internal newlines are legitimate grounding text, so a body newline can still forge a trailing Key: value row. The maintainer merged with a follow-up commit resolving exactly that — splitting the description path so prose keeps its newlines while the labelled metadata rows stay line-safe — and line-sanitizing the digest's [SEV] token, the one field my sweep had missed (PR #5897)
• Most of the dashboard's cross-source intelligence signals could not fire at all, and the seeder reported success on every run — the Railway seeder behind intelligence:cross-source-signals:v1 bare-JSON.parsed each of its Redis inputs, so every contract-mode key reached its extractor as the { _seed, data } envelope it is stored in: the payload array was undefined, the Array.isArray guard on the next line was false, and the extractor returned [] without throwing — so the aggregator's try/catch had nothing to log and the run still exited 0, publishing a shorter list. Auditing all 21 extractors against the writer of each key exposed a second, independent layer: most also read field names and enum spellings their writer has never published — the wildfire signal filtered on radiativePower > 5000 || severity === 'extreme' against detections that carry frp in MW and no severity field at all, leaving brightness > 400 as its only ever-correct clause, which is why the code reads plausibly and produces nothing. Neither existing suite could see any of this: both reconstructed the module with readFileSync + regex + vm, and one of those regexes deleted the reader outright. All 23 corrections are mutation-proven — reverting any single one to the code it replaced turns the suite red, no survivors (PR #5896)
• A model ID the provider does not serve cost a wasted round-trip on every single call, indefinitely — the health gate probed new URL(apiUrl).origin with a bare GET and read any HTTP response as healthy, so it never saw creds.model: the request was built, rejected with http_4xx, and fell through to the next provider, with nothing in the logs pointing at the model as the cause. The evidence was already being collected and discarded — both provider loops read the error body for diagnostics and log the model beside it. Feeding that back into the gate quarantines the origin|model pair for 10 minutes after two consecutive rejections whose body explicitly names the model, at no new network cost. Detection is deliberately narrow: 401/403/429/5xx are credentials, rate limits and outages — provider-wide and silent about the model ID — so they never quarantine, and an unreadable body keeps the previous behaviour, making the fail-safe the status quo rather than a wrongly quarantined model. Pinned by a behavioural test that sends four calls at a dead model: 4 attempts against main, 2 here (PR #5458)
• Designed and prototyped the client-side RAG pipeline that gave AI intelligence briefs historical context — embeddings and cosine similarity running in a Web Worker over an IndexedDB vector store, so retrieval needs no server-side index. My prototype (PR #647) was reworked by the maintainer and shipped as PR #675

D4Vinci/Scrapling    ·  1 merged · released

• Both bulk browser tools of the MCP server sized their page pool wrongly, in opposite directions — so an AI agent reaching for either one got a batch that could not run at all or one that ran through a single tab. bulk_fetch passed max_pages=len(urls) straight into AsyncDynamicSession on the session-less path, but PagesCount is Annotated[int, Meta(ge=1, le=50)], so any batch over 50 URLs — or an empty list, which trips the ge=1 bound — died on Invalid argument type: Expected int <= 50 at $.max_pages before a single fetch started. bulk_stealthy_fetch never set max_pages at all, so the pool fell back to the default of 1 and every URL queued behind one tab, raising TimeoutError once the 60s pool wait ran out; the two sibling tools therefore behaved differently for the identical batch. Both call sites now go through one helper that clamps the pool to the validator's own range, so a batch gets a page per URL up to 50 and anything larger is processed through 50 concurrent pages instead of raising — and the cap is stated in the :param urls: line of both docstrings, since those docstrings are exactly what the model receives as the MCP tool description. The tests reuse the file's existing fake-session monkeypatch so nothing launches a browser: both bulk paths at 3/4, 60 and 0 URLs, plus one case that feeds the computed size into a real AsyncDynamicSession as a regression guard against the validator bounds drifting away from the constant that mirrors them (PR #393, carried into the v0.4.13 release by PR #406 and credited by name in its Bug Fixes notes)

agentscope-ai/QwenPaw    ·  4 merged

• The headless qwenpaw task command could not run a single task, and reported its own failure as the task's — _run_task passed a bare str as Msg(content=...), but the pinned agentscope==2.0.4.post1 declares Msg.content as list[ContentBlock] with no mode="before" validator, so pydantic raised ValidationError on every invocation, before the agent was ever built. The call sits inside a broad except Exception that turns any throw into {"status": "error", "error": ...}, which is exactly why this survived unnoticed: the symptom reads as a task that failed, not as a CLI that cannot construct its own input, and the error string it returns is a pydantic validation message about a type the user never chose. The repository had already settled the correct shape — every other Msg(...) under src/qwenpaw wraps its content in a block list, and the same function builds the right thing 25 lines earlier for AgentRequest — so the fix adopts agentscope's own UserMsg factory rather than hand-assembling a TextBlock, and the PR argues "make this agree with the rest of the repo" instead of asking for trust. Nothing caught it because all 15 tests in test_cli_task.py monkeypatch _run_task wholesale, leaving the function at zero coverage; the two sibling occurrences in proactive_responder.py are named in the PR body as an offered follow-up rather than bundled in, since this repository rejects one fix spread across files. Merged unchanged, 2 source lines against 51 lines of new coverage (PR #6616)
• Stopping a local model server on Windows could hang shutdown indefinitely, flash a console window on every poll, and crash outright on a non-UTF-8 console — _is_pid_running() shells out to tasklist on each iteration of the shutdown wait loop's sleep(0.1), and it was the one call site in its own module that skipped the timeout, the windows_hidden_subprocess_kwargs() the module already defines, and errors= on a locale-decoded read, so a cp936/GBK console raised UnicodeDecodeError straight out of the shutdown path. Measured on Windows 11 each probe costs ~0.157s, so the intended 0.1s poll actually ran at ~0.257s and a 5s graceful shutdown spawned up to 19 tasklist processes. Two rounds of maintainer review moved the PR past that surface fix into the two real defects underneath: a failed probe returned False, which callers read as a confirmed exit — so a timed-out probe made shutdown_process_sync() report a graceful exit and skip kill() for a process still alive — and _PID_PROBE_TIMEOUT was independent of the caller's deadline while the probe ran before the remaining budget was checked, so a 6s shutdown budget measured 20s in the worst case. Probe failures now assume the process is alive, the wait loop bounds each probe by what is left of its deadline, and the post-deadline path does the free local is_alive() check instead of spawning another tasklist, letting the caller escalate. The elapsed-budget regression test runs under a virtual clock, so it asserts the 6.0s bound exactly without sleeping in CI (PR #6203)
• A shutdown during boot could wipe every recorded day of token usage, silently — cancel the consumer while it is still reading the file (Ctrl-C, uvicorn --reload, a quick restart) and stop() force-flushes a cache that was never seeded, committing {} over token_usage.json through an atomic os.replace() with no backup and nothing logged. The window only opens for users who have history to lose. Pinned by a regression test and a positive control (PR #6220)
• Cut one of three nvidia-smi spawns at startup and half of those per /models request — 40% off the measured probe time: a CUDA guard re-ran a query that already returns cleanly without a driver (PR #6204)

MadsLorentzen/ai-job-search    ·  2 merged

• Hex-encoded accents leaked into the LinkedIn scraper's CLI output as raw entities, and emoji came out mangled in every form — the decoder handled decimal entities only, and String.fromCharCode truncated supplementary-plane code points to 16 bits. 1 of 6 fixture cases passed before, 6 of 6 after, under network-free unit tests (PR #55)
• Same bug in both duplicated decoders of the Jobindex scraper, where it matters more: on a Danish portal æ/ø/å frequently arrive as numeric entities, and their hex forms rendered broken (PR #56)

OthmanAdi/planning-with-files    ·  3 merged

• Gave the project its first automated test run: CI until then only reviewed skill prose, never behavior — now pytest across Ubuntu and Windows plus vitest for the Pi extension, on every PR and push to master (PR #199)
• Running that suite on hosted runners exposed two latent cross-platform test failures — a Git Bash path-alias mismatch on Windows and Windows-shaped sanitizer vectors executing on POSIX. Fixed test-side, no production changes, and landed first so the CI PR could go green (PR #198)
• Made those runs reproducible: committed a lockfile for the Pi extension and switched the vitest job to npm ci (PR #200)

MODSetter/SurfSense    ·  7 merged

• Every video presentation was narrated in American English, whatever language its slides were in — a deck built from Chinese source content produced Chinese slides and Chinese speaker transcripts, then handed those transcripts to a pipeline constructed with a literal lang_code="a" and a hand-rolled voice map returning af_heart / alloy / en-US-Studio-O regardless of input. The repository had already solved this once: the podcast package carries a BCP-47 normalizer, a voice catalog with per-language rosters, and a TextToSpeech port whose Kokoro adapter maps the tag to the right pipeline and caches one per language — the video path simply sat outside all of it, which is what let the PR argue "the correct policy is already written in this repo and this one path is outside it" rather than propose a design. The design itself was not mine either: the maintainer's own comment on the issue — unimplemented for two and a half months — specified it (the LLM reports the language since it writes the transcripts anyway, environment variable as fallback, be opinionated about voice defaults), so the work dropped from find a good solution to implement the decided one. Moving up a layer made the diff smaller rather than larger: the node stopped branching between get_kokoro_tts_service and litellm.aspeech, its wav/mp3 conditional became tts.container, and app/services/kokoro_tts_service.py was left without a caller anywhere and deleted along with the module-scope kokoro + torch import it dragged into the agent. The one thing a language-aware resolver silently breaks is the decks that already work — the catalog lists am_adam ahead of af_heart, so every existing English presentation would have been re-cast with no error anywhere — so each provider's previous voice is seeded as preferred and all four literals are pinned by a test named after that regression. 42 tests added and shown load-bearing by reverting only the source files: test_narration_language.py is not even collectable on dev, test_slide_schema.py fails 7, test_slide_audio_narration.py errors 3 (PR #1660)
• A malformed LLM reply printed the user's own indexed documents to stdout — the video agent carried a hand-copied duplicate of the tolerant JSON parser the podcast package already exported, and that copy's failure path ended in print(f"Raw response: {content}"), so a reply that failed to parse dumped the model's answer — assembled from whatever documents the user had indexed — into the worker log. The helper it duplicated documents itself as the thing that "keeps every generation node validating replies the same way", yet had no caller outside podcasts; git mv to app/utils/structured_output.py made that docstring true and history reviewable. Framing mattered more than the diff: print is on this repo's ruff ignore list, so the PR states in its own body that this is not a style change but a log-level and payload-exposure one, and the neighbouring bare except Exception: print(...) became logger.warning(exc_info=True) because that branch was swallowing LLM transport failures indistinguishably from parse failures. My first regression test was worthless and I said so in the body rather than quietly replacing it — written against invoke_json, it stayed green with the fix reverted, because the leak never lived in the helper; rewritten through create_presentation_slides, both tests fail on dev sources (PR #1661)
• The document retriever computed query embeddings on the event loop where its own sibling module offloads the identical call — four call sites across the codebase wrap embedding_model.embed() in asyncio.to_thread, and two in documents_hybrid_search.py did not, so a local sentence-transformer encode would block the loop for every concurrent request. What kept this from being a performance claim is that I checked the callers before writing one: vector_search and full_text_search have no callers in the repository, and hybrid_search's single caller always supplies the embedding, so the two corrected lines are unreachable today. That measurement went at the top of the PR body under "please read this part before weighing the PR", together with the alternative it implies — an offer to send the version that deletes both unused methods instead, if the maintainer would rather have that. Consistency, not speed, is what the PR asks for, and it is the honest claim (PR #1662)
• One unsorted import in a file nobody was touching failed the Frontend Quality job — and with it the Quality Gate aggregator — on every pull request opened against dev, including ones that change no TypeScript at all. I found it the way it finds everyone: a documentation-only PR of mine (#1665) came back red on a TypeScript check. The biome-check-web pre-commit hook declares always_run: true, which overrides its own files: ^surfsense_web/ filter, together with pass_filenames: false and a trailing . in the entry — so the --from-ref/--to-ref narrowing the workflow performs never reaches that hook and every PR is measured against the whole web tree. The evidence that the breakage was not mine came from someone else's change rather than my diff: merged PR #1663 carries the same two red checks, and dev's own Code Quality runs had been failing since late July, with the CI log naming exactly one diagnostic across the tree — lib/error-toast.ts, Checked 1055 files. Found 1 error. The fix is Biome's own suggested safe fix applied verbatim, two import lines swapped. What I deliberately did not submit is a number: running the same Biome command locally reported 1056 errors, and that figure is an artifact of core.autocrlf=true — my Windows working tree is CRLF, so Biome flags format on every file, while the committed blob and CI's checkout are LF. A measurement whose difference from CI you cannot explain is not evidence, so it stayed out of the PR body. I also left the hook itself alone and said so: whether always_run encodes a deliberate "always check the whole app" policy is a maintainer's call, and either way this file had to be sorted first. Re-measured after the merge on an LF checkout of dev, this file is clean — and 12 new diagnostics have since arrived from a feature merge, which is the hook's behaviour restating itself rather than a regression from this change (PR #1666)
• The first instruction in the contributing guide was a 404, and the two formatters it named are not the ones the project runs — CONTRIBUTING.md sent new contributors to ./PRE_COMMIT.md, a file with no match in git ls-files and no other reference anywhere in the repository, then told them to format with Black and Prettier while the repo is configured for ruff/ruff-format and biome; git grep -inE '\bblack\b|prettier' across every tracked .toml, .json, .yaml and .yml returns nothing, so these were absent tools rather than a second toolchain coexisting, and the practical cost is a contributor running black . and producing a diff ruff-format then disagrees with. The dead link became the install command CI itself uses (.github/workflows/code-quality.yml:35) plus a link to .pre-commit-config.yaml. Writing a replacement PRE_COMMIT.md would have restored the exact failure mode being repaired — a second prose copy of the hook list, free to drift from the config — so the guide now points at the config file it describes, and whether to maintain a written guide stays a maintainer's decision instead of being smuggled into a link repair (PR #1665)
• One feature merge put twelve diagnostics into the web tree, and from that moment every pull request opened against dev failed Frontend Quality — whatever it changed. Same biome-check-web hook as #1666, and I had written in that PR's body that the tree would be clean once it landed. Re-measuring after the merge is what produced this one: lib/error-toast.ts was gone, so the fix held, but the count had gone from Checked 1055 files. Found 1 error. to Checked 1066 files. Found 12 errors. — the claim had not been wrong, it had expired, because I had cleaned the breakage and not the producer, and always_run: true was still there measuring every PR against the whole tree. Nine of the twelve are biome check --write output, six files formatted at a narrower width than the configured lineWidth: 100 and three with unsorted imports. The other three are a11y rules with no automatic fix, sitting in code the maintainer had merged the day before — and stopping at the mechanical nine would have left the gate exactly as red as before, which is the only thing this PR exists to change, so all twelve went in: an aria-label carried by a role-less <div> that useAriaPropsSupportedByRole rejects, and two role="status" containers, each resolved to the native <output> element rather than to a rule-silencing attribute. <output> has the implicit status role and accepts aria-label / aria-busy / aria-live, so what a screen reader announces is unchanged, with block added on the two that replaced block-level elements since <output> is inline by default. Verified with the exact command the hook runs — Checked 1066 files in 797ms. No fixes applied. — and the tree's 38 pre-existing tsc errors counted before and after, none of them in the eight files touched (PR #1671)
• The same gate misreading "changed files" the same way in two more tools — and this time it proved itself on my own pull request. I found the Python half while reviewing someone else's contribution (#1648): that PR appends a helper function near the bottom of app/routes/documents_routes.py, and its Backend Quality was red on an unsorted import block the file already had, fifty lines from anything its author wrote. Nineteen such blocks were on dev, plus an unsorted __all__, an isinstance(x, (list, tuple)), and eleven more files carrying ruff format drift. Twenty of the twenty-one violations are ruff check --fix output; the single hand edit is the UP038, because ruff offers that one only as an unsafe fix while the pinned hook runs a plain --fix, so it would have stayed red otherwise. What turned the mechanism from an argument I had been restating since #1665 into a measurement is this PR's own red check: it touches 32 files, every one of them Python, not a line of TypeScript — and Frontend Quality failed on it anyway, with exactly the twelve diagnostics #1671 fixes. A backend-only change failing the web gate on files it never opened is the claim stated by the CI rather than by me. A third tool then repeated it: detect-secrets flagged a localhost DSN default that had been on dev since July 2025, in a file my commit had touched only to add a blank line — because the root .secrets.baseline parses fine and contains zero records, so a gate that looks configured has no allowlist at all and bills the first PR that comes near it. That one is fixed here with an inline # pragma: allowlist secret on the one line that fires, and the empty baseline reported rather than regenerated, since which of the two to adopt repo-wide is a maintainer's call. The review that started all of this also produced the correction I owed its author: I had told them ruff check was clean on their files, measured on their branch, when CI checks out refs/pull/N/merge — so the retraction, the mechanism and two ways out went into the next comment, and both cleanup PRs merged 33 seconds apart the following morning, this one with its inherited Frontend Quality failure still showing (PR #1672)

lemonade-sdk/lemonade    ·  4 merged

• A streaming chat completion that failed at the backend reached the client as an empty stream that simply ended — no error, no status, nothing to retry on — while the server had in fact sent a precise explanation of what went wrong. forward_sse_stream() passed nullptr where post_stream() takes its on_status hook, so the backend's error body was written straight into a response already committed as 200 OK with text/event-stream. Arriving without a data: prefix, that body is not an SSE event, and every spec-compliant parser drops it silently: the diagnosis was on the wire the whole time, addressed to nobody. What makes this a completed rule rather than a new one is that the repository had already written the answer down twice — post_stream's own header documents the hook as existing so that "callers can divert an error body instead of forwarding it as payload bytes", and forward_byte_stream(), sixty lines down the same file, does exactly that: diverts the non-200 body into a capped buffer and reshapes it into {"error": {message, type, status}}. So the fix adopts that path's shape verbatim rather than inventing a payload, and the two streaming paths now report failures identically. The one open question was the reporter's, and it was a design call rather than a defect: no [DONE] follows the error event, because OpenAI does not send one after an in-stream error and sink.done() already terminates the response cleanly — stated in the body with the one-line reversal, and the issue's author confirmed it was the behaviour they wanted. Proven against a real backend on llamacpp:vulkan, where a context overflow answers request (6009 tokens) exceeds the available context size (4096 tokens), try increasing it: the new test asserts every non-empty line is a recognised SSE field and that a data: event carries an error object — test_004 already produced this exact backend 400 but only ever asked whether the stream terminated. Review found the assertion too weak and was right: it would still have passed on the backend_error fallback the code synthesizes when the body is not already an error object, so it was tightened on two axes — a version-independent type != "backend_error", which no wording change in llama.cpp can satisfy, plus the context substring the reviewer asked for. The suite runs in test-cli-endpoints-linux on every pull request, so the case is guarded on Linux and not just where I measured it. The diagnosis was @ekenberg's: they had opened the issue twelve days earlier offering to send the fix themselves and received no reply, so the body credits the analysis to them and the issue carries an offer to close mine in favour of theirs (PR #2975, issue #2826)
• Two C++ test files sat committed in the tree that no CI workflow ever built, and one of them had stopped compiling six weeks earlier without anyone noticing — which is the repository's own argument for the rule it was breaking: testing.md's "What Reviewers Reject" table lists "committing a test no CI workflow runs", and CMakeLists.txt and .github/ between them held zero references to either file. test_ggml_hip_path.cpp is the proof the rule earns its place: #2044 added it on 31 May against is_ggml_hip_plugin_available() in lemon::utils (path_utils.h), #2320 moved that function into lemon::backends::llamacpp on 29 June and dropped the header declaration, and the test kept its old include and namespace — 128 lines that read as coverage and compile nowhere. The other, test_model_type_classifier.cpp, is 71 header-only cases over get_model_type_from_labels(), a function called from half a dozen places; it passes 17/17, so it was wired up through the add_cpp_ci_test(... CI ON) helper the project requires — direct add_test() is overridden to fail precisely so every test makes an explicit CI decision — taking ctest -L cpp-ci from 22/22 to 23/23. The rotted file was deleted rather than repaired, because repairing it meant re-exporting a symbol into a header purely so a test could reach it, and the same testing.md rejects "new API surface added only for testability"; the body drew that line and offered the export instead if the maintainers preferred to keep the coverage, and the deletion is what they took. The change also corrects testing.md's three remaining references to register_cpp_ci_test(), the helper AGENTS.md now forbids calling directly. An hour after it was opened, #2950 landed and rewrote the exact table row the docs fix touched, so it was rebased onto their wording with only the helper rename reapplied and re-measured at 23/23 (PR #2976)
• Four handlers answered 200 OK with the router's error object in the body — /rerank, /slots, /slots/{id} and /tokenize each passed whatever router_->...() returned straight into res.set_content(), so reranking with a model that cannot rerank, or erasing a slot on a backend started without --slot-save-path, reads as success and leaves the client nothing at the transport layer to branch on: every status check passes and the failure is only visible to code that already parses the body looking for it. The rule had been written down the day before I found it. 410b8732, a maintainer commit sitting at HEAD~1, introduced set_error_response() — which reads the payload's own type and status_code and maps them onto a real HTTP status — and wired the five-line if (response.contains("error")) guard into exactly one handler, handle_embeddings. The four above are copies of the same shape that predate the helper, so this is not a new convention but the missing half of theirs: +44/-0 across two files, the success path untouched. Only one of the four is testable, and the body said so rather than papering over it — llama.cpp implements /slots and /tokenize, so on the one backend CI runs their router-error paths are unreachable, and the reranking case (test_018d) is what shipped, with the other two offered if the maintainers wanted them shimmed. Review went past the diff instead: @fl0rianr pushed cef16cf1 onto the branch directly, replacing an elif "error" in erase_data branch in test_023_slots that had been swallowing exactly this class of failure with an explicit 501 assertion — the same bug one layer up, in the test written to catch it. That commit carried a 92-character line the repository's pinned black==26.1.0 resplits; nothing under .github/workflows actually runs Black despite AGENTS.md calling it "enforced in CI", so CI stayed green and only running the pinned version by hand surfaced it. Reporting that produced the instruction to fix it here, and 1ba3cf5e is the resulting one-hunk reformat, verified against black==26.1.0 in a clean virtualenv (PR #2974)
• A request that failed at the backend came back through the Anthropic bridge as 200 OK carrying an empty text block — and, streaming, as a well-formed message_start … message_stop sequence with no error in it anywhere, so a Claude-SDK client saw the model answer with nothing and had no status to branch on. The same overflow prompt measured three ways on llamacpp:vulkan shows how far that one path had drifted from its neighbours: the OpenAI route answers 400 with the backend's own explanation, the Ollama route 500, and POST /v1/messages 200. The streaming half is a sibling my own #2975 created — since that merged, a failed OpenAI stream is framed as data: {"error": ...}, and the Anthropic adapter, reading id, usage and choices[0], walked straight past it and kept translating. Both halves land together, because fixing one would have left the endpoint silent only half the time. Review is where the change actually grew, and the reviewer's second objection turned out worse than described: StreamingProxy publishes the upstream status under status on errors it synthesizes and publishes nothing at all when the backend already returned its own structured error, while the bridge read only status_code and a numeric code. So the 400 my two integration tests asserted was a coincidence — llama.cpp happens to put code: 400 in its body, and any backend that omits it would have been reported as a generic 500 api_error. The fix moves the producers onto the one field name the repository already reads, injecting status_code into a backend's own error object rather than adding a third reader, and converts the two remaining emitters of status: trellis_server.cpp, and serve_media_or_error(), which had inlined its own copy of the extraction loop and now calls get_error_status_code() like everything else. The integration tests the review asked for could not be written, and saying so was the answer rather than substituting something weaker: no in-repo backend can drive the mapping past 400 — an unknown model 404s in the auto_load_model catch before the bridge is reached, anything that throws lands in the outer catch as a hardcoded 500, and 429/529 have no producer at all without cloud credentials CI does not hold. So the two pure helpers moved to src/cpp/include/lemon/anthropic_error.h behind a cpp-ci unit test (ctest -L cpp-ci 25/25 → 26/26), which also closed the open question the PR body had handed back — backend_error_http_status() being a private duplicate of server.cpp's file-local get_error_status_code(). A second round then named three statuses Anthropic documents that the map still missed — 402 billing_error, 409 conflict_error, 504 timeout_error — and caught that round one's own test had asserted 504 → api_error: that was my incorrect assertion, so the expectation was corrected rather than defended, ending at 28 cases with each of the three red before the change (PR #3006)

openclaw/clawhub    ·  5 merged

• Searching the catalog in Japanese never reached the category, topic or summary match tiers — a query whose katakana carries the prolonged sound mark ー, which covers most of the loanword vocabulary a skill registry is full of (データベース, サーバー, ユーザーインターフェース), matched on name and slug only. tokenize() delegates segmentation to Intl.Segmenter, which handles Japanese correctly, but the text is pre-split on a hand-written character class first, and that class omitted ー (U+30FC) and 々 (U+3005) — so the segmenter received a word already in pieces: データベース arrived as ["デ","タベ","ス"], and 人々 lost its iteration mark outright. Two consequences followed. Exploratory search requires every query token to clear a three-character floor, so rank tiers 2 and 3 were unreachable for these queries; and getFirstSearchToken is tokenize(value)[0], stored as an indexed column and used as a range-scan bound, so a skill named データベース管理 indexed under the single character デ and the bound stopped being selective. The sibling reference sits 53 lines below in the same file: detectCJKLanguage counts katakana with the full Unicode block /[゠-ヿ]/, which does match ー — that is how the text gets routed to the Japanese segmenter in the first place — so the pre-split was excluding the character the language detector had just counted, and the file disagreed with itself. Widening to that full block was tried and rejected in the PR body rather than left for a reviewer to ask about: it produces identical tokenize() output, but leaks ・, ゠ and ヿ into the segmentCJKByChar fallback as standalone tokens, so the change stayed at two characters. Review found the half I had missed, and it was mine to own — changing a tokenizer stales persisted data, because those first-token columns are recomputed only when a skill is written, and the mirrored skills.sh catalog carries its own copy of the same keys through a duplicated firstSearchToken helper the native backfill could never reach. Both tables got a cursor-paginated, rate-spaced, idempotent backfill, the duplicated rule was collapsed into one exported helper so the two copies cannot drift apart again, and both backfills now preview by default behind a per-path confirm token that rides the scheduled continuation — the first shape defaulted dryRun to false, so npx convex run maintenance:backfillSkillSearchDigestFirstTokens --prod, a command that reads like an inspection, would have rewritten a reactively subscribed table and scheduled every remaining page. Proven against a real Convex backend rather than a mock: an anonymous local deployment seeded while the base commit was deployed so its digest rows carry the old tokenizer's output, then this branch deployed over them — 11 of 13 rows drift, an unconfirmed apply and an apply carrying the other path's token both throw before the first paginate and write nothing, and one confirmed page of ten leaves the eleventh row stale for the whole 15 s the continuation waits before it lands, which is what measures that the token survived the scheduler rather than just that the paging works (PR #3363)
• Chinese, Japanese and Korean catalog cards rendered previews three or four characters long — on every surface that goes through truncateText: the home listing sections, the skills and plugins catalogs, search results, publisher cards and the dashboard. The helper slices to the budget and then backtracks to the last space so a preview never ends mid-word, a rule that assumes spaces mark word boundaries throughout the text. In non-spacing scripts they usually do not, and the only space in an entire summary often sits immediately after an opening Latin token — a product name, a protocol, a version number — so the backtrack rewound past everything else. The measurement came out of the repository's own fixtures/public-corpus/corpus.jsonl rather than constructed strings: 25 real catalog entries improved, the worst of them huangli-query-cn rendering as the single character | at an 80-character budget and 5gc-automation as 5GC…, while all 1362 space-separated previews in that corpus came out byte-identical — so the behaviour English cards rely on is measured rather than asserted. The rule is made conditional rather than removed, and review was right that my first condition was too broad: a kept-ratio threshold applied to every script loses the boundary on a space-separated summary that happens to end in a long token such as a URL. The fallback now also requires that what is being discarded is actually non-spacing script, tested with the character class the catalog search tokenizer already defines (CJK_RE in convex/lib/searchText.ts, character for character) rather than one invented for this fix — and both halves are shown load-bearing by mutation, since dropping either turns exactly one test red. The real-browser proof took two rounds, and the lesson was the baseline: the first capture compared the reviewed head against the branch, which shows the review fix but not the repair the PR is about, and the bot refused it correctly. The second ran Chromium over bun run preview against the same public Convex deployment the repository's own Playwright job uses, compared against main, and read back three live CJK cards going from 25, 29 and 38 rendered characters to the full 80-character budget with no Latin preview differing between the two arms (PR #3362)
• A fully documented skill was rejected at publish as "too thin or templated" whenever its SKILL.md carried no YAML frontmatter and used --- as an ordinary Markdown horizontal rule — the skill never reached the catalog, and the newest accounts felt it first, since the reject floor is highest for the lowest trust tier. The quality gate stripped frontmatter with an m-flagged pattern, so ^ matched at every line start rather than only the start of the document: it latched onto the first thematic break and deleted everything up to the second. Frontmatter is optional on publish — the display name comes from the mutation's own arguments — so that document is legal input. 94% of the body was being discarded before measurement: a 109-word SKILL.md measured as 6 words, score 30 → 100, decision reject → pass; the truncated text also fed the template-spam fingerprint, so similarity was being compared over a fragment. Anchoring the pattern to the document start is what the canonical parser and the repo's three other frontmatter patterns already did, pinned by the first unit tests for a module that had none — two of the three fail on the parent commit. The maintainer's follow-up carried it further and routed the gate through that shared parser, deleting the fourth regex outright (PR #3297)
• A publisher who swapped a file after the first changelog preview landed submitted a changelog describing a bundle they were no longer publishing — on the skill update form the generated "What changed" text is cached in a ref keyed on the slug, version, SKILL.md size and lastModified, and the path count, while the path list itself is what the action receives as filePaths: exchange one bundled script for another and the key is identical, so the second request is skipped. Nothing reset that key when the selection changed, and the generation effect returns early while the field is non-empty — so once a preview had landed, no later change to the file set could replace it for the rest of the session. The sibling plugin publish form already covered both halves, keying on normalizedPaths.join("\0") and resetting the cached key when the file set changes; the skill form now agrees with it, and the reset returns early on a changelog the publisher typed, so manual text is never discarded. Read out of the mounted form with the same SKILL.md instance reused so only a sibling file differs: 1 preview call carrying the superseded filePaths before, 2 calls ending on the current bundle after (PR #3296)
• A listing untouched for just under a year read "Updated 12mo ago" instead of "1y ago" across skill rows, browse results, plugin detail, the dashboard and the GitHub sync timestamp: timeAgo measures months as 30 days but years as 365, and 360–364 days still divides into twelve whole months while sitting below the year threshold. Aligned with the publisher-profile renderer by deriving years from whole months, removing the unit mismatch at its source — with the first tests for a file that had none despite being inside the coverage include list (PR #3174)

openclaw/mcporter    ·  2 merged · 1 co-authored

• A server that was simply unreachable was reported as needing authorization — a browser OAuth flow launched at the user and the stored server definition promoted to auth: 'oauth' — whenever the connection error's text happened to contain the digits 401 anywhere: a port, a timeout duration, a hostname, a request id. The classifier's auth check was a raw includes('401') with no word boundary, and it ran before both the generic HTTP branch and the offline-transport branch, so a message that plainly matched ECONNREFUSED still came back auth and the real fault the user had to fix was hidden behind an authorization prompt. The control case is one character wide: ECONNREFUSED 127.0.0.1:9000 classifies offline, 127.0.0.1:14012 classifies auth — the committed offline test passed only because its port happened to be lucky. The intended precedence was already written down in the repository's own tests (code=404 as http (not auth), code=500 as http, 405 as transport/http instead of auth) and in two earlier fixes pointing the same way; the implementation honoured it only while the message carried no auth-like text. Fixed as an ordering rather than a special case — a known status code decides first, then the unambiguous keyword signals (unauthorized, invalid_token, forbidden), then OFFLINE_PATTERNS, and only then the bare 401 numeral, the one signal ambiguous enough to belong below transport evidence. That split is also what closed the review's remaining merge risk in code instead of asking for it to be accepted: demoting every auth signal below the offline check would have sent a genuine invalid_token payload that also says "connection timed out" to offline, so only the numeral moved, and the compatibility change is confined to exactly the reported defect. The word boundary itself took two passes — excluding neighbouring digits still let request_401_id and abc401def through, while \b would have broken unauthorized_client, the RFC 6749 error code — so the numeral is bounded on alphanumerics and _ while the keywords stay substrings. Proven through the real isUnauthorizedError → maybeEnableOAuth path with no mocks, no injected transports and no network: three transport failures that promote to oauth on main stay unpromoted here, with the lucky-port case as the control, and every added regression shown load-bearing by restoring the previous ordering and watching exactly the intended tests go red and no others. The maintainer merged it with the ambiguous-numeral precedence accepted as implemented (PR #248)
• A headless deployment that already held OAuth credentials had no way to install them — mcporter vault set rejected every clientInfo field that was not a string, so no real RFC 7591 dynamic client registration response could be seeded: redirect_uris, grant_types, response_types and contacts are string arrays, and both registration timestamps are numbers. The declared type said otherwise — VaultPayload.clientInfo is OAuthClientInformationMixed, mcporter builds that same array-valued shape itself during registration and reads clientInfo.redirect_uris back as an array — so its own client information could not round-trip through its own command. One rule for every field, while validateOAuthTokens directly above it already typed each of its own fields individually. Two adjacent defects fell out of writing the replacement table, and they are the part that outlived my patch: the unguarded JSON.parse on the credential payload surfaced V8's parser message, which quotes a prefix of its input, so a malformed token file printed a fragment of the token itself through the unexpected-error path and into whatever log was collecting it; and tokens.expires_at / expiresAt were never checked at the write boundary although the read guard isStoredOAuthTokens requires finite numbers, so a typo in one field made the command print Saved OAuth credentials while loadVaultEntry handed back an entry carrying neither tokens nor client information. That silent loss is only visible if the proof is taken through the read path — reading credentials.json directly bypasses the guards and reports everything present. My own pre-push review caught a regression of the same class inside my own fix: deriving the table from the SDK schema dropped issuer, an mcporter extension the old string-only rule had been covering by accident, whose sole consumer is the refresh-time issuer pin. The maintainer landed his own narrower fix for the reported defect (PR #288), keeping the partial and null-compatible client information that my table's required client_id would have started rejecting — a compatibility change I had flagged in my own body along with the exact three lines to drop it — then closed mine as superseded while explicitly preserving both adjacent findings, and shipped them as PR #294, crediting me as co-author on the merge commit (d7330dd) and in the changelog (PR #287, issue #286)
• The one place a headless operator looks before writing an OAuth credential payload described a narrower shape than the command accepts — mcporter vault set --help still presented clientInfo as client_id alone, while the validator had since been widened to take a full RFC 7591 dynamic client registration response: the redirect_uris, grant_types, response_types and contacts arrays, the client_id_issued_at / client_secret_expires_at timestamps, and provider metadata outside the spec. That gap is not hypothetical — issue #286 was filed with exactly the expectation the old line creates, and the reporter's payload was a full registration response. docs/config.md names no fields at all, so nothing else in the project contradicted the help text either way and the command's own output was the only surface that could be wrong. Four lines appended under Payload:, with the existing one-liner left first so the shortest usable payload still reads first. The wording deliberately names field groups rather than restating the rule table: OAUTH_CLIENT_STRING_FIELDS alone is 15 entries, and a help text that enumerates a validator is a second copy free to drift from it. "Provider metadata outside RFC 7591" is the one behaviour a reader cannot infer from the spec — validateOAuthClientInfo iterates its own field lists rather than the payload, so registration_client_uri and registration_access_token reach the vault untouched. Proof is a before/after transcript of the real CLI rather than a rendered string, and the regression test is the first help coverage vault has ever had — built in the shape of the existing cli-auth-help test, so it also pins the Usage: line and the exit code the help shortcut sets; reverting only the source file to main turns it red (PR #302)

openclaw/fs-safe    ·  4 merged · 1 superseded

• A file outside a confined root was reported as inside it whenever the root string carried surrounding whitespace — isPathInside("C:\root ", "C:\root\secret.txt") returned true. isPathInside is the predicate the other guards build on, and on win32 its only normalization step ended by delegating to a free-text string coercion helper — the same module that normalizes fast-mode flags and thread values — whose chain calls value.trim(). So a path used for containment math was trimmed before it was lowercased, and since whitespace is a legal part of a Windows path component, two genuinely different directories collapsed onto one comparison key. Fixed by lowercasing in place instead of routing the path through that helper, leaving separator and extended-length handling untouched. Unicode case folding is deliberately left alone and the reason is written into the PR: toLowerCase() is not injective — on a Turkish-language Windows install "İstanbul" folds to a 9-code-point string that never round-trips — so moving to an ASCII-only or locale-invariant fold changes behavior for every non-ASCII path and reads as an owner decision rather than a bug fix. The review asked for the intended contract to be owner-approved; it turned out the repository had already written it down, in two committed tests carrying skipIf(skipOnWindows). Removing that skip made the measurement possible and showed the deny list on main applying to the wrong directory — the protected directory writable while its sibling was blocked. Verified on Windows 11 with Node 24.15.0 through the real exported functions, with the new tests proven load-bearing by stashing only src/path.ts: 2 failed | 3 passed before, 5 passed after (PR #78)
• A path spelled C:secret.txt read and wrote a different file than the one it named — on Windows it aliased onto secret.txt at the root of a confined store, so two distinct untrusted keys resolved to one file. path.win32.isAbsolute("C:secret.txt") returns false for the drive-relative spelling — no separator follows the colon — while path.resolve() still consumes the drive prefix, so every layer that screens for absolute paths waved it through and the prefix vanished one call later. The escape is not the interesting part; the aliasing is, because the guard that catches escapes (isPathInside) sees only the already-collapsed result and correctly reports the file as in-root. Review pushed the fix down two layers, and both times the reviewer's location was one level off from where the hole actually was: the file-store parser was never on Root's path at all (assertValidRootRelativePath() was a NUL check), and then readAbsolute()/reader() turned out to resolve the raw input before validating it. Proven on Windows at each step rather than argued — root.read("C:secret.txt") returning the real secret.txt, with a resolution table for the four spellings and a logs/2026-08-02T10:30:00Z.log control proving the anchored pattern does not eat timestamped names. I argued against gating the guard on process.platform, on the grounds that a key valid on Linux must not become a boundary violation when a store moves between hosts, and that was kept. The maintainer narrowed the blast radius before landing: applied where a path is created or resolved — writes, mkdir, copyIn, resolve(), every FileStore key, and the destination of move() — but not to reads, stat, list or the source of move(), since c:notes.txt is a legal POSIX filename and refusing to read back a file that already exists on disk is collateral, not containment (PR #85, landed as #97)
• Concurrent lock acquisition failed intermittently on Windows against a lock file that no longer existed — and because the failure had been read as CI noise for four releases, the repository's own main had been red since a dependency refresh, with a different concurrency test failing nearly every run. Windows denies access to a file whose directory entry is still being torn down, so a contended acquireFileLock() got EPERM on a name already gone; acquire() treated only EEXIST as contention, so the transient denial escaped from both the exclusive create and the holder's snapshot read. Instrumented at the moment of failure, lstat reported ENOENT and a zero-delay retry opened the file. The evidence had to be a distribution, not a green run — 8 failures in 85 runs before, 0 in 110 after — because a single pass proves nothing about a race. Two review rounds raised the same P1 and were right both times: scoping a retry to a code region keeps leaking, because the region always holds more than the operation you measured (first the caller's payload() callback, then a parent-directory open hidden inside the native create). Retrying is not neutral when the retried block can re-run a caller's callback, so the predicate that finally held names the evidence instead — EPERM and the exact lock pathname — and anything unproven propagates. The maintainer closed the last gap himself: the Windows native binding reported ERROR_ACCESS_DENIED as pathless EACCES, so on packaged installs the predicate could never match, invisible here because every test in the file forces native mode off (PR #87, landed as #92)
• Every Root.remove() failure was reported as a containment violation — a missing file, a non-empty directory and a busy handle all threw path-alias / "path is not under root", so a consumer could not separate a routine filesystem outcome from a safety rejection, and downstream code in openclaw had grown a helper purely to unwrap the real errno back out of the bogus one. not-found, not-empty and not-removable were declared in the exported error union and promised in six documentation sites, but constructed nowhere in src/: the remove path funnelled every non-FsSafeError through a normalizer whose default is path-alias. Framing decided the PR — the documentation was already correct, which puts this on the contract-repair side of the line this repository merges on, rather than the contract-change side where its one rejected external PR sits. Review then found a genuine defect in my first fix: the errno mapping wrapped the whole fallback including the parent-directory guard, so a raw ELOOP surfaced as not-removable with nothing deleted; the second push splits the guard into its own stage so only the deletion syscalls are classified, and the guard fails closed. The maintainer added the half I had missed — all three codes were also absent from OPERATIONAL_CODES, so categorizeFsSafeError() kept labelling them category: "policy", and fixing the code without the category would have delivered half the change (PR #84, landed as #93)
• Diagnosed why two Windows permission tests kept timing out in CI, and flagged the unbounded subprocess call underneath it — the Node N check job never builds the native binding, so those tests take the command fallback and pay six process spawns each, two per inspection (powershell.exe for the owner query, icacls.exe for the ACL). I opened the timeout increase as an explicitly test-only PR and kept the real finding out of it: defaultPermissionExec called execFileAsync with no timeout, so a wedged icacls.exe would hang inspectPathPermissions() indefinitely for any consumer — a public behaviour change that did not belong smuggled into a budget bump. The maintainer took the diagnosis and not the number, which was the better outcome: rather than widening the budget to fit the cost, PR #89 removes a redundant inspection so the affected tests drop to four spawns and ordinary Windows CI keeps exercising the command fallback, and it bounds the commands with a fail-closed result — an owner query that cannot complete now yields source: "unknown" and readSecureFile() refuses with permission-unverified instead of returning a permissive answer. The tests went from 15-second timeouts to about four seconds (PR #88, superseded by #89)