openclaw/openclaw    ·  32 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 \n\n, and flattenCompletionMessagesToStringContent, the helper performing this exact operation for strict OpenAI-compatible servers, joins with \n — so the fix follows the closest sibling instead of inventing a separator. Proven on the wire rather than in a mock: a real node:http server stands in for the provider, parses the actual request body and answers with a real SSE stream through the real streamOpenAICompletions path, showing file.The file on main against file. 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)
• Eight extension test suites could not be run to completion on Windows, so a green local run was impossible for contributors on that platform even when every assertion passed — every failure was in teardown, none in a test. Doctor state migrations and auth-profile writes leave per-agent and shared SQLite handles cached under the fixture's temporary directory, and clearing the plugin state store or the runtime auth snapshots does not release them, so removing the directory fails with EBUSY; Linux unlinks open files and CI stayed green throughout. The repair is the ordering two fixtures in the same repo already used — close the cached databases before each removal — measured one suite per run on both arms of the same tree: 79 failing cases on main, none after, with no production file touched (PR #126352)
• Asking an agent to send a file with an explanation on Discord delivered the file and dropped the explanation, while the tool reported success. The message tool carries attached text two ways, and because upload-file accepts media as an alias for the file, a model can reach it with the same vocabulary it uses for a media send — a vocabulary that carries its text in caption. Discord read only message/content, and the upper layer does not compensate: the attachment hydrator copies message into caption and never the reverse (src/infra/outbound/message-action-params.ts:640-646), so a call carrying only a caption reaches the handler with message still unset. The repository already owned the contract on both sides — WhatsApp resolved message ?? content ?? caption all along, and Slack had just been fixed the same way in #121047 — so this completes a rule rather than inventing one, with precedence preserved exactly as Slack’s fix defined it: an explicitly empty higher-precedence value still wins, so existing callers keep their behaviour. Proven at the production boundary rather than in a mock: the real upload-file action drives the real Discord messaging runtime, the real media validator and the real RequestClient against a real node:http server on loopback, the only substitution being where the socket terminates, with the assertion made on the payload_json part of the actual multipart request — this branch sends "content":"Q3 revenue is down 8 percent" where the control run, identical but for handle-action.ts restored from origin/main, sends no content field at all. Microsoft Teams carries the same defect in resolveActionContent and was deliberately split out rather than landed on committed handler coverage, because a Teams delivery or activity trace is not reachable without a live tenant (PR #124255)
• The gateway's GitHub publication suite could not be run to green on Windows, and the one failing case never failed an assertion: the shared harness removed its temporary state directory while a per-agent SQLite handle opened underneath it was still cached, so teardown died with EBUSY on openclaw-agent.sqlite — three times a run, because the file is shared by three gateway projects. Adding the agent-database close is not the fix; the order is. The agent close releases its leases through shared state, so closing shared state first lets it reopen and the lock simply moves to state\openclaw.sqlite-wal. Closing agent databases before shared state — the ordering the zalo fixtures already document — takes the file from 3 failed / 60 passed to 63 passed on the same tree, with no fs.rm retry budget added so nothing is masked, and no production file touched. The reporter reproduced both arms independently on a second Windows host and a different Node build before it landed (PR #127201)
• Neither extensions/nostr suite could be run to completion on Windows, so contributors on that platform had no green local run even when every assertion passed — every failure was in teardown, none in a test. Both fixtures reset the plugin state store when they set the fixture up and never again, so the state database opened during the test was still cached when the fixture removed its temporary directory, and the removal died with EBUSY on openclaw.sqlite or one of its -wal / -shm companions; Linux unlinks open files, so CI never saw it. Moving the reset to immediately before the removal — the ordering the eight fixtures repaired earlier in this class now use — takes extensions/nostr from 6 failed to 279 passed and the whole extension-messaging shard from 2 files failed to 134 files passed, with each of the two added calls measured as load-bearing on its own (PR #127236)
• Eleven extensions/memory-core suites failed on Windows without a single failing assertion — every one died in teardown, so contributors on that platform could not get a green local run of the shard even when the code under test was fine. Each fixture removes its temporary state directory while a SQLite handle opened underneath it is still cached, so the removal fails with EBUSY on the shared state database, the per-agent database or one of their -wal / -shm companions; Linux unlinks open files, so CI never saw it. Four of the eleven failed only because the shard's shared harness in src/test-helpers.ts removes its fixture root without releasing anything — one teardown covers all four. The order is load-bearing rather than decoration: the agent close releases its leases through shared state, which reopens it, so releasing shared state first only moves the lock from openclaw-agent.sqlite to state\openclaw.sqlite-wal. Measured on two different bases of main with identical results, whole directory, both arms on the same tree: 13 files failed with 50 EBUSY occurrences on main against 4 failed and 0 here — the four survivors are pre-existing non-EBUSY Windows defects this branch deliberately does not touch. Each of the two added calls is shown load-bearing by mutation: dropping the agent close leaves the same suites failing on openclaw-agent.sqlite, dropping the store release leaves them failing on openclaw.sqlite. No production file touched (PR #128310)
• The extension-qa shard could not be read as a signal on Windows — 86 EBUSY occurrences across 23 failing files, five extensions/qa-lab suites of them failing entirely in teardown while the behaviour under test passed. Same mechanism, same repair, and it clears all five (27, 19, 14, 4 and 2 failing cases → 0), taking the shard to 18 files and 20 occurrences. What makes this one worth recording is the fix that was rejected: putting the release in the shared createTempDirHarness() helper fixes the same teardowns, but that helper is also reached from inside the QA suite child process suite-process-lifecycle.test.ts spawns, so clearing the process-wide caches there kills the running suite — per-fixture teardowns are safe, the shared helper is not. Both calls proven load-bearing by mutation, in the strongest form this class has produced: with either one alone the same 66 teardowns still fail, only against the other database, so the lock does not disappear with one call, it moves. The two arms were diffed test-by-test rather than compared by count, which is what shows the 31 pre-existing non-EBUSY failures identical on both. Three suites in the same shard that survive the change are named in the PR as a separate mechanism, with the measured reason the same pair does not fix them, rather than shipped as noise (PR #128335)
• The last group of Windows teardown failures the two cache-clearing helpers could reach, and the one that closed the issue the whole class was filed under. Three extensions/zalouser fixtures — credential, quote-metadata and doctor — each remove their temporary state directory while a SQLite handle opened underneath it is still cached, so the removal dies with EBUSY on openclaw.sqlite, the per-agent database or one of their -wal / -shm companions; Linux unlinks open files, so CI never saw any of it. The doctor fixture is the instructive one: it already called resetPluginStateStoreForTests(), which releases plugin state and shared state and leaves the per-agent handle open — a call that looks like the repair without being it. Measured on the whole extension-zalo shard, both arms on the same tree: 3 failed files / 16 failed tests with 16 EBUSY occurrences on main, against 54 passed (54) files and 461 passed (461) tests with 0 here. Open for 18 days across a fast-moving shard, so the base arm was re-measured twice rather than carried over — the second time because eight commits had landed in the shard's scope even though the three fixture files themselves were untouched. Seven repairs of this shape landed in total; what is left in the shard is a different mechanism, recorded on the issue for whoever picks it up (PR #119964)
• A session pinned to the 200K context window still budgeted against the model's 1M window on native runs — the selected option reached resolveContextWindowInfo as its lowest-priority input, so a discovered contextTokens value or a models.providers entry silently outranked it: the session row and both gateway projections reported 200K while the embedded runner budgeted and auto-compacted against the wider window. The CLI runner and both projections already clamp the resolved window with the selection — the embedded runner was the one surface that did not — so the fix mirrors the CLI clamp, guard included, leaving models without selectable windows untouched (PR #128838)
• A long tool-using turn threw away every completed tool call when only the final delivery call died on a transient EPIPE / ECONNRESET / UND_ERR_SOCKET — every tool had already run and every tool result was already persisted, but the run was discarded and the user got the generic "No reply was generated" fallback, so recovery meant re-generating the whole turn and paying for every token again. The narrow repair already shipped in the runner: an isolated, tool-free finalizer that composes the answer from the settled transcript without replaying anything. It could never fire, because the gate fails closed unless the attempt carries settledTurnFinalizationContext and nothing on the embedded path ever produced it — a missing producer, not a missing feature, and the codex app-server harness on the same result type already had one. Added the embedded producer mirroring the codex predicate, leaving every existing gate untouched. Proven end to end over a real socket: a loopback server speaking the Anthropic Messages wire protocol serves the tool turn, destroys the socket mid-response on the delivery call, then answers the finalization request — the production gate flips from discard turn to finalize purely on the presence of the new context, and the recovered reply comes back over the same transport (PR #128840)
• Every Ollama cloud model disappeared from provider discovery and from onboarding whenever the daemon answered /api/show without describing the model — remotely hosted rows never carry the locally inferred completion capability, so the completion filter dropped them on the degraded-inspection path and the operator saw an empty model list with no error to explain it. The repair separates incomplete list metadata from authoritative inspection: a row Ollama itself considers remote keeps its advertised capabilities instead of being judged by a local-only predicate. Proven against a real loopback daemon serving /api/tags and answering /api/show with 200 {}, with a local row and an embedding row as untouched controls. The maintainer took the branch over to fold the repair into the shared metadata owner and to cover Ollama's full remote predicate (remote_host or remote_model); I re-ran the two-head probe against that rewrite, and it landed co-authored (PR #130240)
• Every plugin tool call routed to a paired node reported a generic client-side timeout, hiding whether the command had actually been dispatched — plain node-hosted plugin tool calls sent no invocation budget, so the Gateway armed no deadline over pairing, wake and policy work and fell back to the node registry's own 30-second pending timer, which only starts at dispatch. The agent-side wait defaults to the same 30 seconds but starts earlier, so the caller always abandoned the request first and the Gateway answer carrying nodeCommandDispatched retry-safety provenance could never arrive — the caller could not tell a command the node had received from one it never saw, which is exactly the distinction a retry needs. Sending the same budget explicitly, with the +5s grace the node MCP path already uses, lets the Gateway answer win the race; anchoring the budget at entry and re-reading what is left after pairing revalidation and request serialization stops it from advertising a command the node did receive as retry-safe, and only a positive caller-supplied budget arms the deadline so the callers that have always relied on the registry's post-dispatch fallback keep their established semantics. Proven end to end with a real Gateway, a real paired node that accepts the forwarded command and never answers, and the shipped agent tool driven through the real gateway client (PR #118720)

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    ·  2 merged · 1 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)
• An exception you explicitly told the agent not to retry was swallowed anyway. ModelRetryMiddleware narrows what it retries through retry_on — a tuple of types or a predicate — and everything that fails the check is, by definition, an error the caller wants to own. It was handed to on_failure instead: under the default on_failure="continue" the middleware turned it into an error AIMessage and let the agent keep going, so an AuthenticationError or a programming bug in a callback came back as text for the model to read rather than as a raised exception, with the retry_on filter reading as "retry these, quietly absorb the rest" — the opposite of what it configures. The correct behaviour was already written in the repository twice over: ToolRetryMiddleware re-raises in exactly this branch since #38845, and #38884 documented it as the contract — the model side was simply missed in both passes, which left the same configuration behaving differently depending on which half of the agent threw. So the argument put to maintainers was not "should this behaviour change?" but "why does this middleware disagree with its sibling?", and the diff is that narrow: return self._handle_failure(exc, attempts_made) becomes raise, on the sync and the async path. Two existing tests pinned the old behaviour by asserting on the error message text, so they were rewritten around pytest.raises while keeping their attempt counters — the point of the middleware, that a matching exception is still retried up to max_retries, is what those counters protect — and the async branch, which had no coverage of non-retryable exceptions at all, got its own case. Reported first as issue #38893; the PR was auto-closed 23 seconds after opening by the require-issue-link bot, reopened by that same bot the moment @hntrl assigned the issue on the strength of the approach comment, and merged by @ccurme — whose only addition was a Raises: entry in the docstrings, propagated to ToolRetryMiddleware as well, so the contract is now stated on both surfaces instead of just held by both (PR #38960)

NVIDIA/TensorRT-LLM    ·  5 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)
• A streamed reply that merely contained a < came back with characters missing — Use <div> for a block element. reached the client as Use div> for a block element., with nothing raised and nothing logged, because all four DeepSeek tool parsers (deepseekv3, deepseekv31, deepseekv32, and deepseekv4 by inheritance) withheld the buffer correctly and then threw it away. parse_streaming_increment accumulates deltas in self._buffer and holds them back while the tail could still grow into a tool-call start token — both of those tokens begin with < — but once the ambiguity resolves it clears the buffer and returns only new_text, the latest delta, so everything parked there by earlier deltas is gone. The two guards fail differently and the newer one is worse: v3/v3.1 test e_token.startswith(new_text), the delta alone, while v3.2/v4 test current_text.rstrip().endswith(prefix) for a bare <, which withholds on any delta ending in < and therefore loses far more at once — 20 characters in the repro against 4. The correct implementation was again already in the package rather than in my head: BaseToolParser.parse_streaming_increment tests the buffer with _ends_with_partial_token and emits the buffer, and gemma4, minimax_m3, poolside_v1, qwen3 and the newly added kimi_k3 all follow it — the four DeepSeek parsers were the directory's only exception, which is the entire argument the PR needed to make. Two of CodeRabbit's three findings survived a repro and went in: v3.2's potentially_dsml check never released the buffer once buffered text had diverged from any delimiter, and has_tool_call accepted a bare <｜DSML｜invoke substring as a call. The third — text that precedes a tool call inside the same delta — is real, but it hits a different code path in all four parsers, so it went out as issue #17580 rather than widening this PR; the RUF001/RUF003 ambiguous-unicode warnings were declined with the evidence that pyproject.toml selects no RUF rules at all and the file already carries 42 hits under them. For the invoke header I departed from the suggested "complete header regex" and used the constant <｜DSML｜invoke name=" — after that quote the function name is arbitrary, so it is the longest fixed prefix ordinary prose cannot produce by accident — and put it in the partial-token list so a header split across deltas stays buffered instead of leaking. Human review cost the code one guard, the if e_token in text test before str.replace, dead weight in all three files, and left two standards worth keeping: a comment states the invariant only — never the old behaviour, never the reason for the change — and you measure before calling a test cheap (8.7–28.7 µs per parametrization, 0.19 ms for all twelve). 12 parametrized cases, 8 of them red against main, verified locally through a conftest.py shim that makes the real test file importable without a compiled tensorrt_llm — 311 passed. The last thing this PR established was about the CI and not the diff: five L0_MergeRequest_PR runs on commit 8b382f6 returned FAILURE with no test tally while every GitHub-side check on it stayed green, and the sixth run — same commit, not one character changed — returned SUCCESS and merged it. Filed first as issue #17572 (PR #17573)
• A tool call arriving in the same delta as the sentence that introduced it swallowed the sentence — Let me check the weather for you. followed by <｜tool▁calls▁begin｜>… in one chunk streamed the call and nothing else, because all three DeepSeek tool parsers (deepseekv3, deepseekv31, deepseekv32, and deepseekv4 by inheritance) decide has_tool_call with a membership test — self.bot_token in current_text — and then never ask where the token was. Every return on the tool-parsing branch is hardcoded normal_text="", including the except fallback, so whatever sat in front of the start token in the buffer left with the call. Non-streaming is not affected: detect_and_parse splits on the token and returns the head, which is exactly the behaviour streaming was failing to match. The fix replaces the membership test with the positions it was throwing away — start_indices = [idx for idx in map(current_text.find, start_tokens) if idx != -1] — splits the buffer at the earliest start token, keeps self._buffer from that token onwards so the tool-call machinery below sees the same text it saw before, and carries the prefix out on every return of the branch. What made it a short argument is that the shape was already in the package: Glm47ToolParser performs this same split, and TestMiniMaxM2ToolParser::test_streaming_preserves_prefix_in_same_chunk already pins prefix preservation as the contract — so the DeepSeek trio were the exception, not the proposal. CodeRabbit's suggestion to .strip() the prefix "like detect_and_parse does" was declined with measurements rather than opinion: running six parsers over " Normal text " + <call> shows Glm47ToolParser and MiniMaxM2ToolParser stream it verbatim too, and stripping inside a streaming path is chunk-dependent — three cuts of one string through MiniMaxM2ToolParser return 'Normal text', ' Normal text ' and ' Normaltext', a lost space. The test half of that finding was taken anyway as test_deepseek_streaming_prefix_is_delta_independent, which feeds the same text at four delta boundaries and asserts the concatenation is identical, and a third finding — the prefix test could pass even if the call itself were dropped — was taken as an assertion that the streamed calls are exactly ["get_weather"], filtered by name so it does not depend on how many call items a given parser emits. The argument assertion was left out on evidence: V3 and V3.1 emit no streamed arguments at all on this path. The except Exception breadth flagged in the same review was declined as pre-existing code whose return value was the only thing this PR touched, with pyproject.toml selecting no BLE rules. Scope was held twice over: text between two completed calls is unfixed because detect_and_parse does not surface it either, and BaseToolParser.parse_streaming_increment has the identical start_idx = tool_call_pos + len(bot_token) shape and reproduces the symptom on qwen3 — both are stated in the PR body rather than folded in. 8 parametrized cases, all 8 red against main, 319 passed locally, 0.415 ms for the eight measured up front because this repo's reviewers ask for the number. Filed first as issue #17580; approved without a change request and merged as 8873151 on the first pipeline run — the only one of the four that did not need a second (PR #17903)
• A tool call that takes no arguments never finished streaming, and took the rest of the response with it — BaseToolParser.parse_streaming_increment gates the branch that streams a call's arguments and marks it complete on if cur_arguments:, and the arguments of a zero-argument call are {}, which is falsy. So the branch that exists to finish the call never runs for the one input where there is nothing to stream: the client is handed arguments="" instead of "{}" — not valid JSON for a field the OpenAI schema says is a JSON string — and, because the same branch is what advances self._buffer past the completed call, the buffer keeps the call text forever, has_tool_call stays True, and every later delta is routed back into the tool-call path. Measured, the non-streaming half of the same class returns ('get_time', '{}') while streaming returns ('get_time', '') with normal_text='' and '…</tool_call> All done.' still parked in the buffer, so the trailing sentence and any second call are lost too. The blast radius is Qwen3ToolParser, the parser the registry auto-selects for qwen2, qwen3, qwen3_moe, qwen3_5, qwen3_5_moe and qwen3_next. What made the argument short is that the repository already pins the correct answer in its own tests — test_tool_parsers.py:1997 for GLM4 and :2181 for GLM4.7 both assert "{}" — so Qwen3 was the sibling missing the guarantee, not a proposal to change behaviour. Review is where it grew: the reviewer pointed out that get("arguments") is also None when the key is absent or explicitly null, and both dead-end exactly like {} did, so the fix normalizes None to {} only when the call is complete — while the JSON is still partial a missing key means "not streamed yet", and flushing there would emit arguments the model has not finished writing — and the regression test became parametrized over empty_object / key_absent / explicit_null. His second nit was that the PR body claimed a symptom this diff does not fix; it has a different cause, so it was rewritten out of the description and filed as issue #17740 instead. A third change was folded in on his request and is the one worth keeping the reasoning for: parse_base_json — shared by Qwen3, GLM4, GLM4.7 and DeepSeek V3/V3.1/V3.2 — turned an explicit null into the string "null" on the non-streaming path, and I had planned to pin that divergence in an assertion and file it separately. An assertion is a bad place to keep a known divergence, he wrote: whoever fixes it later has to overturn a test that says the old result was intended. File it or fix it. It was two characters, act.get("arguments", {}) → act.get("arguments") or {}, so it was fixed. CI is the other half of this one: six L0_MergeRequest_PR runs across fourteen days, four of them returning FAILURE with no test tally published at all — build/infra deaths, four distinct causes when the maintainer finally pasted the stage list, none of them in the diff — then one that did reach the tests and failed only on test_wan22_ti2v_5b_pipeline.py, an unrelated visual-generation regression the maintainer measured at 12 failures across builds on other PRs, and then a green run on the same commit that merged it. The public heuristic I had been using to tell those apart (no tally and no L0-Test workflow run for the head sha ⇒ the run never reached the tests) held four times and then did not, which cost a correction on the PR: outside NVIDIA's network the stage list is the only ground truth, so ask for it rather than infer it. Filed first as issue #17574; merged as 46f0b7e (PR #17575)

koala73/worldmonitor    ·  11 merged · 1 prototype

• A hostile RSS feed could back-date one item and take the identity of a live, already-corroborated story — and the code admitted it, in a comment — server/worldmonitor/news/v1/dedup.mjs:56-58 carried an in-code marker conceding that the canonical member of a cluster is chosen by publisher-supplied publishedAt ordering, which is exactly the field an attacker controls. Two of the issue's own prescriptions had to be corrected in the body rather than followed. It asks for a batch EXISTS over the member story:track rows and then to “adopt the oldest EXISTING track” — but EXISTS cannot rank two rows by age, so the command has to be HMGET STORY_TRACK_KEY(h) firstSeen lastSeen, which answers existence and age at one cost. And the premise that the story had no defence is overstated in a way worth narrowing: story:alias rows exist for every member hash, so what item 1 closes is that adoption is a most-common-wins vote, movable by a feed publishing one story under several wordings, whereas story:track.firstSeen is HSETNX'd at first observation and cannot be moved at all. The gap is the vote, not the absence of a defence. The latent bug found while sizing the change would have bitten first: the alias read was already unchunked at one command per member hash and a full batch runs to four figures — the wrong side of the 1000-command cap STORY_BATCH_SIZE's own comment is sized against, so a second command per hash would have doubled an over-budget call. It is chunked at ADOPTION_BATCH_SIZE = 400 with both commands for a hash kept in the same call, so a cluster can never decide from an alias row and a track row observed in different states, and a track counts only while lastSeen is inside the digest's own freshnessCutoff. Item 2 of the same issue, salting the feature hash, was costed and deliberately not shipped: it moves the collision distribution that STORY_SIMILARITY_THRESHOLD = 0.615 was tuned against, and the labelled pair set leaves a margin of 0.039 (min positive 0.634, max negative 0.595) — a re-tune, not a rider. Eight mutations applied, eight caught. The maintainer then took the branch over rather than handing it back, fourteen commits ending at 22 files and +1924/−138; the piece worth reading is his anchorEligible field on story:track:v1, written HSETNX '0' then conditionally HSET '1' so eligibility is monotonic: a later low-trust mention cannot erase proof, and legacy rows fail closed. Reviewing his commits back produced the two findings that landed an hour before merge. The corroboration limb of isAnchorEligible was inert at the batch-default call site, because item.corroborationCount is still the ParsedItem constructor default of 1 there and is assigned from the cluster only inside the await Promise.all(...) block that runs after the selection — so with MIN_CORROBORATING_PUBLISHERS = 2 only the source-tier limb could fire, and an all-untrusted-tier cluster with real corroboration fell back to precisely the publishedAt ordering the PR exists to stop trusting. And adoption could not start at all on a cold build: its deadline is digestStartedAt + ADOPTION_DEADLINE_MS (11s) while digestStartedAt is stamped one line before the fetch phase's own AbortController (10s), and a chunk refused to start without a full REDIS_PIPELINE_TIMEOUT_MS of headroom — so it ran only if fetching finished inside 6s. The proof came from the feature's own log line in his CI run rather than from a test: of 12 digest builds in variant-smoke-full, 11 logged story adoption deadline reached; skipped N at offset === 0. His repair generalises where the re-anchoring I proposed would not — redisTimeoutForDeadline clamps each request to the budget that remains instead of treating a fixed timeout as an admission floor, applied at nine call sites carrying the same latent unreachability (PR #7022)
• The public algorithms page told readers that 14 correlation signal types are “continuously evaluated” — three of them have no code anywhere that can emit them, and one of those three has a finished detector with no caller — docs/algorithms.mdx:327 published the union's size as if it were the product's behaviour; 11 types are actually emitted, and news_leads_markets, sector_cascade and hotspot_escalation have zero emit sites across the 786 files under src/ and shared/. The load-bearing finding is the third one: shouldEmitSignal() at src/services/hotspot-escalation.ts:70 is a complete decision function with a 2-hour cooldown and no callers, and markSignalEmitted() at :79 is never called either, so the cooldown map it guards is never even written — code that reads as a shipped feature and has never run once. The argument that made the PR nearly pre-approved is that the repo had already settled this exact class one section earlier in the same file, for the sibling BreakingAlert.origin union: tests/breaking-alert-doc-contract.test.mjs:16 derives the truth from emit sites with /^\s+origin:\s*'([^']+)',\s*$/gm, and the doc says outright that reserved names “do not make them active producers”. The new guard is that regex with origin → type, because the emit shape is byte-identical. The declared count deliberately stayed at 14 with emits 11 published beside it: dropping the sentence to 11 would leave 11 in the prose and 14 rows in the table, would execute the maintainer's promote-or-delete decision on his behalf, and would break tests/docs-signal-alignment.test.mts, which pins lists N distinct signal types and row count to the union size for two other documents — and the cost of actually deleting a type was written out rather than waved at, roughly 35 files, because SIGNAL_CONTEXT is an exhaustive Record<SignalType, …> and 28 of 29 locales carry the key. One premise correction shipped in the body: the issue calls news_leads_markets “the feature a user just asked us to build”, but #6418 closed completed on 2026-08-13 via #6530, which shipped src/services/news-market-correlation.ts with its own CorrelationRelationship union and never touches that name — the capability exists elsewhere, the type is still dead, which sharpens the question rather than voiding it. Eleven mutations applied, eleven caught. The maintainer's follow-up commit found the hole in the guard itself, and it is the more interesting half: a repo-wide literal scan means any matching literal counts as live wiring, so dead code carrying type: 'news_leads_markets' would have satisfied it. He replaced the directory walk with an explicit RUNTIME_PRODUCERS table — each entry naming the emitter file, a named producer symbol, and the runtime path that consumes it (analyzeCorrelationsCore through correlation.ts, checkForSpikes through the data loader's drainTrendingSignals(), geoConvergenceToSignal and surgeAlertToSignal through their data-loader.ts call sites) — asserting every hop, and added the negative control I had not written: a dead-code literal that matches the emit pattern and must still fail the producer contract. He also split the doc lead so hotspot_escalation is described as an unwired decision function rather than as having no detector at all, and pinned the Chinese flow_drop wording positively instead of only through a negative ETF check (PR #7021)
• A timing guard that pins an HTML parser to linear scaling was throwing flakes in CI, and by the time I had finished measuring it another contributor had shipped the same fix — so what merged is the part nobody else wrote: proof that the guard can still fail — tests/china-policy-events.test.mts timed its 4k and 16k inputs as two contiguous blocks with best-of-3 inside each, so a single ~20 ms scheduler stall correlates across every sample of one block and inflates the ratio against the 8x gate. The issue prescribed interleaving the two sizes and taking Math.min per size; I first “improved” on that by keeping each attempt's pair and letting the best pair decide, and across 44 rounds it looked like a clear win — p90 5.6x → 3.9x. The positive control I had written to keep the guard honest is what refuted it. Timing noise is one-sided — a measurement can only come out slower than the work, never faster — so the fastest sample of each size is the unbiased estimator, while selecting the smallest ratio preferentially picks the attempt whose denominator stalled: a narrower distribution that is bias, not precision. Three proofs went in the body: over 60 rounds the ratio form reported a minimum of 1.8x for a 4x size step, which is physically impossible; on a genuinely quadratic mutant it cleared the gate by 42.1 ms where Math.min per size clears by 92.2 ms, eating more than half the guard's teeth; and across 22 isolation runs it let the known-quadratic control through twice, once at 7.7x against an 8x gate. I shipped the issue's form and recorded my own variant as the rejected alternative with its numbers. I also could not reproduce the flake at all — 20 cores, three load models, ~220 rounds, zero readings above 8x — and said so instead of implying a measured win; the first two models failed for a reason worth keeping, which is that a six-measurement round takes ~25 ms, so load that varies over hundreds of ms is constant across the round and cannot produce the effect at all. Then the branch went CONFLICTING for a reason a rebase does not fix: #7102 had landed the same interleaving fix, down to the same buildFixtures / timeOnce split, and #6962 had separately widened the gate 8x → 12x and added a warmup. Rather than rebase a dead diff I diffed my branch against what had actually landed and shipped only what was still missing — the control — with buildFixtures, the interleaved measurement and scalingCeilingMs hoisted to describe scope so both tests are measured by the same ceiling, because a self-contained control carrying its own * 12 + 2 keeps passing after the next retune while no longer describing anything. Recalibrated against the wider gate: control 16.0x–22.3x, guard 3.9x–4.5x with 63–146 ms of headroom, 341 ms of runtime at repeat: 250, and the full test:data failure set diffed by name against clean main rather than by count — 253 = 253. The maintainer's follow-up commit found what the control was actually measuring: a handwritten nested-prefix loop is synthetic O(n²) work that never calls parsePolicyHtmlFields, so it could stay red even after the real parser's guard became too loose. He extracted the closing-tag unwind into unwindPolicyHtmlClosingTag behind a requireKnownTag / onStackComparison seam, exposed runPolicyHtmlClosingTagRegressionProbe on the module's __testing__ export, and pointed the control at the production path with the early-out disabled — then raised its base size 250 → 1,000 → 2,000 to stabilise it (PR #7020)
• The dashboard shows how many outlets independently carried a story; an agent calling the MCP tools over the very same snapshot got the headline as a bare string — get_world_brief reads the precomputed news:insights:v1 snapshot the dashboard reads, and the seeder has already applied its corroboration, citation and hallucination gates before publishing, so every accepted story arrives carrying uniqueSourceCount, corroborationSourceCount, entityCorroboration, sourceTier and the outlet names. The projector took primaryTitle and dropped the rest, so the surface that most needs to say how well corroborated is this was the one that could not. It now emits an index-aligned topStories[] built inside the same loop that builds headlines, so topStories[i] describes headlines[i] is a guarantee rather than something two loops could drift apart on, with the per-story outlet list — the only unbounded sub-array on a payload under a 64 KB output budget — capped at 12. On get_country_brief the evidence deliberately does not ride the existing sources[]: that array is the proto BriefSource shape and the gateway's own list wins on the common path, so widening it would have been a proto change that the common path then overwrote. A sibling groundingStories[] carries corroborationCount, mentionCount and storyPhase, and it drops any item carrying neither field, so a digest predating the story identity work yields an empty array rather than a row of zeroes. It is also kept out of the helper that feeds briefSourceContextLines, because that one becomes LLM prompt text and story metadata has no business there. Three of the issue's own premises were wrong and the body says so with file:line — the seeded snapshot carries no storyMeta at all (scripts/seed-insights.mjs:867-898 drops it), so phase is reachable only on the country-brief digest path; get_news_intelligence already declared sourceCount; and the sources[] route was closed for the reason above. Item 2 of the same issue — salting the feature hash — I costed and did not ship: a salt changes which items cluster, which moves the collision distribution that STORY_SIMILARITY_THRESHOLD = 0.615 was tuned against, and that threshold's margin is 0.019 (min positive 0.634, max negative 0.595), so the work is a re-tune and a re-validation, not the one-line change it looks like. The only real regression in the branch was found by diffing the names of the failing tests against clean main rather than their count: public/.well-known/mcp/server-card.json mirrors every tool description and a compression test deepEquals it against the registry, so editing a description without regenerating the card went red — while every targeted MCP suite stayed green. The maintainer's follow-up commit found the flaw in the fix, and it is the more interesting half: coercing every missing producer field to 0/false at the trust boundary turns absence into evidence, because an agent reading uniqueSourceCount: 0, entityCorroboration: false concludes the gate ran and found nothing, when the snapshot simply predates the field. Those fields are now optional and omitted when unavailable, with a test pinning the distinction that matters — an explicit zero or false published by the producer still survives. He also routed the grounding path through normalizeInsightSource, since my hand-rolled typeof publishedAt === 'string' check silently discarded the numeric epoch timestamps digest items actually carry, and bounded the duplicated grounding URL at 2,000 characters — omitted rather than truncated, because a clipped URL is no longer a citation target — under a regression test where six long URLs duplicated out of the canonical citation list pushed the serialized country brief past its own 64 KB budget guard (PR #6421)
• Three public API routes proxied a third-party host on every request with nothing metering them — and the browser code calling one of them was already written to handle the 429 it could never receive — src/services/preferences-content.ts:554 reads if (res.status === 429) throw new Error('rate-limit') against /api/skills/fetch-agentskills, a POST route that had no limiter, no cache and no origin check at all, and that one line is what moves this from a limiter would be nice to this was an oversight, so it opens the PR. The other two are /api/youtube/live, where one request can fan out to both a Railway relay and a full live-page scrape of youtube.com, and /api/reverse-geocode, which fronts OpenStreetMap Nominatim — the strictest usage policy in the stack, enforced by banning the egress IP. Nominatim deserved the specific attention the issue gave it, because that route's cache write is conditional on a resolved country (api/reverse-geocode.js:64), so ocean and Antarctic cells never populate it and a sweep of those coordinates was 100% unmetered passthrough. The implementation splits by file extension, and the split is the part worth defending: AGENTS.md:118-121 forbids api/*.js from importing ../server/, enforced by lint:boundaries and a pre-push esbuild check, so only the TypeScript route can read its budget out of ENDPOINT_RATE_POLICIES and the two .js routes must carry theirs as a literal. That leaves the registry entries decorative — numbers declared to an audit script and to the docs that no handler enforces, which is the exact defect class the repo's own contract-gate-field-names-miss-value-axis.md names — so I wrote a static test pinning each handler's literal to its registry entry and said in the body that without it I would not have added the registry keys. No constant is invented: 30/min is the provider-proxy convention documented at server/_shared/rate-limit.ts:301-315, 60/min matches the sibling map helpers. Three published claims were false in the direction that hid the gap — limits applied via an api/_ip-rate-limit.js that does not exist on main, a "~60 req/min/IP default" that is really 600, and a documented GET skill catalog that is a POST fetching one skill — corrected in EN and zh. The maintainer's follow-up commit is the one worth reading, because its headline finding was that my three route suites could not fail: they asserted X-RateLimit-Limit against a value their own Upstash mock supplied, and @upstash/ratelimit surfaces the script reply's second element as limit, so a handler configured with any budget still passed. His tests/helpers/upstash-limiter-wire.mjs reads the budget the handler actually sent — the limiter's script ARGV — and asserts limit, window and scope prefix through it; my registry-mirror test had the same shape of hole, matching a bare /checkRateLimit\(/ so that dropping limit: silently fell back to the 600/min global default while staying green. He also closed a fail-open I had inherited rather than introduced: @upstash/ratelimit v2 resolves { success: true, reason: 'timeout' } when its internal race beats Redis, so a slow-but-alive Redis never reaches a catch block — no log, no Sentry, degraded: false — which checkEndpointRateLimit handled and both checkScopedRateLimit and api/_rate-limit.js did not, covering all three of my new call sites. Plus the isDisallowedOrigin gate fetch-agentskills lacked — a third-party page can drive its visitors' browsers to POST it as a CORS simple request, and every victim is a different IP, which a per-IP budget cannot bound — and the client I had left half-fixed, src/utils/reverse-geocode.ts, which memoized a 429 as null in a TTL-less session Map, turning a one-minute throttle into a permanently unlabelled map cell (PR #6412)
• 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    ·  4 merged · all released · 3 named in the v0.4.15 notes · 1 fix credited upstream

• Asking an HTTP fetcher for no retries meant it never sent the request at all, and the error it raised named a session that was up and healthy — every static path in scrapling/engines/static.py drives its attempts with for attempt in range(max_retries), so retries=0 — or a negative value, both of which the HTTP side accepts without a word — runs the loop body zero times and falls straight through to the line after it: raise RuntimeError("No active session available."), carrying a # pragma: no cover because nobody had asked how you would get there. The browser engines bound the identical parameter properly, RetriesCount = Annotated[int, Meta(ge=1, le=10)], and reject 0 cleanly at validation — one parameter, two engines, two contracts, and the one with no contract is the one that fails silently in the wrong place. It is reachable straight from the agent-facing surface too: the MCP server republishes retries with its own documented default and calls it "Number of retry attempts", so a model told to fetch a page without retrying sends 0 and gets an exception about session state, while retries=None — legal on three public surfaces by type — dies one step earlier still, inside range(None). The fix clamps instead of rejecting: anything below 1 means send it once, which is what a caller writing 0 intends, and rejecting it would break the callers who already pass it today — the precedent being #393, where the same maintainer merged a clamp rather than a new validation error for the sibling max_pages bound. Two production lines against sixty lines of coverage across the four fetcher test files, sync and async, session and session-less, each run red on dev before the change. Merged with a reply that is the interesting half: "If that happened, then it would be the user doing that for themselves, but anyway, let's fix it" — he did not accept the severity and merged it anyway, because the diff was two lines that cannot regress anything and the alternative was leaving a raise that lies about what went wrong (PR #420, shipped in v0.4.15 and credited by name in its Bug Fixes notes)

• The second run of a spider in development_mode handed every callback an empty response.meta, so parsing code that worked on the first run broke on the replay — ResponseCacheManager.get rebuilds the Response out of the cached JSON and never passes meta, so Response.__init__ falls back to {}; the cache-hit branch of CrawlerEngine then reattaches the request to that response and stops there. The live path does the other half one file over, in SessionManager.fetch: response.meta = {**request.meta, **response.meta}, under a comment stating that the request's meta is merged in with the response's taking priority. Two paths under one contract, one of them honouring it — and the divergence is invisible on the run that fills the cache, which is the run a developer watches. Everything a spider carries between requests therefore survives exactly one execution: a response.follow(..., meta=...) chain loses its state on replay, and the shipped ShopifySpider dies with KeyError: 'handle' on response.meta["handle"] the moment its responses start coming from disk — which is exactly what development_mode is for, iterating on parsing code without refetching, so the failure is scoped to the users who turned the feature on and lands only once it starts doing its job. The fix mirrors the live path on the cache-hit path in two lines, with the cached response's own meta still winning, rather than teaching the cache to serialise meta as well: the request is the authority for that field on both paths, the merge order is already decided in SessionManager, and copying that decision costs nothing at the cache boundary while a second serialised field would have to survive every future change to what a request carries. The regression test reuses the file's existing MockSpider/MockSession harness so nothing goes near a network: it starts a request with meta={"page": 7}, runs the engine twice over the same cache directory, and asserts the callback sees the same meta on the cached run as on the live one — on dev it reports [{'meta': {}}]. The shape was borrowed from #379, where the same rebuilt response dropped the browser engine's cookies; meta is the other field the cache never gives back, and naming the merged precedent in the body is what makes an asymmetry cheap to check instead of a claim to be taken on trust (PR #419, shipped in v0.4.15 and credited by name in its Bug Fixes notes)

• A stealthy session opened with solve_cloudflare=True never solved a challenge — every setting a session was opened with was dropped on every fetch made through it — the five MCP tools that take a session_id (fetch, bulk_fetch, stealthy_fetch, bulk_stealthy_fetch, screenshot) forwarded every per-fetch parameter into session.fetch() unconditionally, their own Python defaults included, even when the caller had supplied nothing. validate_fetch treats every key it receives as an override, so the branch that reads the value off session._config — the one carrying the comment "This prevents validated defaults from overwriting session config values" — was unreachable for exactly those keys; and because each tool default happened to equal its PlaywrightConfig/StealthConfig default, _filter_defaults stripped them all and handed back an all-defaults config that then overwrote the session's own. In Python f() and f(0) are indistinguishable inside f, and stop being indistinguishable the moment f forwards what it got to a layer that asks was this key given? — the MCP layer meant "I said nothing, you decide" and the validator heard "make it 0". solve_cloudflare is the sharpest case, since validate_fetch deliberately preserves it whenever it appears among the overrides: a session built with solve_cloudflare=True never reached if params.solve_cloudflare: and also lost the 60s timeout that StealthConfig.__post_init__ sets for it, while wait, timeout, google_search, extra_headers, disable_resources, wait_selector, wait_selector_state and network_idle went the same way on both dynamic and stealthy sessions. Those parameters now default to None and pass through one helper that keeps only what the caller actually set, so an unset option falls back to the session — chosen over the ~30-line alternative of "forward it only when it differs from the tool default", which fixes the bug but makes it permanently impossible to turn a session setting off for a single call; a smaller diff is not worth a semantic hole a reviewer can see on the first read. That the session-less paths are untouched is a table, not a claim: every affected parameter's tool default was put beside its struct default one by one, which is what surfaced the two that do not match (timezone_id, cookies) and kept them out of the change. The repro in the PR body launches no browser and touches no network — it runs the exact kwargs the tool sends through validate_fetch and prints the session's own settings coming back wrong — because this maintainer has twice closed symptom-level "Cloudflare is not solved" reports as invalid; the same bug is unreviewable told as a symptom and checkable in five seconds told as a mechanism. Merged unchanged the same day it was opened, ~170 source lines against 268 lines of new coverage, five of whose tests fail on dev (PR #418). Thirty hours after it merged, the maintainer reverted almost all of this diff and re-architected the MCP surface instead (73ad18b, refactor(mcp)!), explaining why on the PR: those same five tools also serve session-less callers, so defaulting every per-fetch parameter to None changed their behaviour too — and on an MCP surface the signature is the documentation the model reads, so None defaults erased every real default value from the agent's view. Both objections are correct, and both were mine to catch: the table I built proved the tool defaults and the struct defaults were equal, which answers "do the values match?" and not "how many kinds of caller does this function have?". His fix splits the surface rather than sentinelling it — open_session now carries browser-level configuration only, a new session_fetch tool carries the per-request options with their real defaults visible, and SessionInfo returns the session's effective settings so an agent can read them instead of inferring them; where I hid the ambiguity, he removed it. The bug class is closed by a better fix than mine, which is the outcome that was actually wanted. It is written down here because a merge badge on its own would misrepresent what survived: the finding held, the diff did not. The tail of it is the part worth keeping. Reading his replacement commit turned up a second regression he had not seen: the new session_fetch still accepted and documented a proxy, but a per-request proxy is routed through a code path that needs self.browser, which is only populated for a session opened with cdp_url or a proxy rotator — a normal session runs on launch_persistent_context, so it raises RuntimeError: Browser not initialized for proxy rotation mode before any navigation, and since the same commit had dropped proxy from open_session, MCP sessions were left with no working proxy path on either side. That went back as a reply rather than a fourth open pull request — mechanism, an eight-line reproduction that touches no network, and an explicit question about which of the two fixes he wanted, because the file was being rewritten under me that week and the right layer depended on a product decision that is not readable from the code. It was: he replied eleven hours later that persistent context is the default and an MCP session is now one tab per session, so setting a proxy per tab is meaningless, and shipped 93ae0ac moving proxy back to open_session as a browser-level argument — "Thanks for @Yigtwxx for pointing that out in #418" in the commit message. Verified against the new dev: the session proxy now reaches the persistent context's options and a fetch fails on the dead proxy with ERR_PROXY_CONNECTION_FAILED instead of raising before it starts. Two findings, both closed, neither by a diff of mine — which is the honest shape of this one. The release notes then kept the finding attached to my name even though the diff did not survive: v0.4.15 describes the reworked MCP server as "This also ends fetches resetting the session's settings, first fixed by @Yigtwxx in #418", listing the revert's replacement under the person who found what it replaced — a merge badge and a surviving diff are two different claims, and this repo is where the difference showed up

• 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    ·  5 merged

• Every streaming request that hit a provider quota wall retried into a pause that could not help, and lost the error that explained why — RetryChatModel keeps its 429 policy in one place, _handle_rate_limit_exc, whose docstring spells out all three branches, including the one that matters here: a retryable 429 whose Retry-After exceeds MAX_PAUSE_SECONDS must be re-raised immediately, because retrying after the capped pause only earns another 429. The non-streaming path in __call__ calls that helper; _wrap_stream had re-implemented only the middle branch inline and dropped the bail-out — and stream=True is the default, so this was the path essentially every request took. A provider answering Retry-After: 51496 therefore installed the capped 60 s pause on the per-model limiter, which holds back every other caller of that model, slept it out, re-issued, and collected the same 429 for each remaining attempt; worse, the exception that finally surfaced was the limiter's own Rate limit exceeded acquire timeout rather than the provider's quota error, so the single message that explained the failure was the one thing thrown away. The fix deletes the second copy of the policy instead of patching the missing if into it — 4 source lines removed, 1 added — since a divergent inline copy is exactly how the bug was born, and a third copy would invite the next one. The RED run's captured log carried the proof better than any assertion (LLM rate limiter: global pause set for 60.0s (raw_retry_after=51496.0s)), and the second test is the over-correction guard: it passes before and after the fix, but a naive "just drop the report_rate_limit() call" version passes the first test and breaks this one. Merged byte-identical to what was opened, 1 source line against 99 lines of new coverage (PR #6617)
• 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    ·  12 merged · 2 issues closed

• 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)
• A provider telling us exactly how long to wait was ignored every single time, and a passing test was the reason nobody noticed — RetryAfterMiddleware exists to obey a 429's Retry-After header instead of guessing with exponential backoff, and the branch that reads the header is gated on isinstance(headers, dict). litellm.exceptions.RateLimitError.__init__ rebuilds the error's response unconditionally, so exc.response.headers is always an httpx.Headers — a MutableMapping that is not a dict subclass — and that branch had never executed in production: a provider answering Retry-After: 45 got SurfSense's own 1s / 2s / 4s instead, three retries burned inside roughly seven seconds, turn failed, which is the exact behaviour the module docstring says it was written to replace. The guard looked covered — tests/unit/agents/new_chat/test_retry_after.py builds its fixture headers as a plain dict[str, str], so the suite was exercising a shape production never produces, and the claim went into the PR as a measurement against the versions the repo pins rather than an argument about types (litellm 1.88.1 | openai 2.24.0 | httpx 0.28.1, isinstance(headers, dict) -> False, isinstance(headers, Mapping) -> True). Repairing it makes a second, dormant defect reachable, which is why both are in one diff: _delay_for_attempt returned max(backoff, header) while max_delay, documented as the cap on per-attempt delay, only ever constrained backoff — harmless while header was permanently 0.0, and await asyncio.sleep(3600) inside a live chat turn once a misconfigured gateway sends retry-after-ms: 3600000 (PR #1705)
• Ask the chunk retriever for ten documents and a single chunk-dense document could hand you one — hybrid_search documents top_k as "Number of documents to return", both reciprocal-rank-fusion legs collect top_k * 5 candidates, and then the fused query throws that pool away with .limit(top_k) measured in chunk rows. Grouping N chunks yields at most N documents, so the final doc_order[:top_k], commented "Keep only top_k documents", can never truncate anything — it runs, and the slice is always a no-op. Three things in the tree made this a mistake rather than a design choice: the sibling documents_hybrid_search.py ends with the identical .limit(top_k) where the rows genuinely are documents, so the chunk retriever had inherited a limit whose unit changed underneath it; the newer shared/retrieval/hybrid_search.py does it the other way round, limiting the fusion to the candidate pool and truncating to documents afterwards; and that newer module carries test_top_k_caps_the_number_of_documents while the legacy one only ever asserted len(results) >= 1. The second half of the change is required rather than scope creep, and the repo's own suite is what says so — widening the window made test_per_doc_chunk_limit_respected fail with assert 35 <= 20, because the per-document fetch filter had always exempted matched chunks (rn <= _MAX_FETCH_CHUNKS_PER_DOC OR Chunk.id.in_(matched_chunk_ids)), an exemption invisible only while the whole window held top_k chunks. The cap is now enforced on the assembled result with matched, citable chunks kept ahead of surrounding context, and matched_chunk_ids recomputed from what survives, so the payload never advertises a chunk it does not contain (PR #1706)
• Two bug reports four minutes apart, same reporter, same browser — and nothing in the build could have caught either — .filter(...).toSorted is not a function on Chrome 109, thrown inside a useMemo in the notifications dropdown, so it fails during render and takes the component down rather than degrading. Array.prototype.toSorted and toReversed are ES2023 (Chrome/Edge 110, Safari 16.4, Firefox 115) and there are five client-side uses; surfsense_web/tsconfig.json sets "target": "ES2017", which downlevels syntax and never adds a built-in, "lib" includes esnext so TypeScript cheerfully declares both methods available, there is no polyfill, no browserslist key and no .browserslistrc — the support floor is implicit and nothing enforces it. Two of the five were also mutation bugs: use-comments-sync.ts sorts an array held in a Map that the same pass reads again and message-utils.ts reverses another, so a naive toSorted → sort swap would have reordered shared state in place; both copy first. Two further ES2023 calls were deliberately left alone with the reason stated — the changelog page renders on the server where Node 20 has the method, and findLastIndex has shipped since Chrome 97 — because widening the diff past what fixes a user-visible failure is how a compatibility fix turns into a sweep (PR #1707)
• The end-to-end gate had never passed once — 60 runs, zero successes, dev included — because its health probe could not physically finish inside its own timeout. Every run died in Build & start backend stack with container surfsense-e2e-celery_worker-1 is unhealthy, before a single test executed, and the worker was serving the whole time: the job's own uploaded stack logs show celery@... ready. at 17:00:26 against an abort at 17:02:05, with no error, no traceback and no further output after it came up. The probe ran celery -A app.celery_app inspect ping, and -A makes the CLI import the entire application first — app.config builds the sentence-transformers instance, the chunkers, the LiteLLM router and the ETL stack at import time. Measured rather than asserted: celery --version 0.96s, python -c "import app.celery_app" 20.1s, the full probe 26.8s even with the broker refusing the connection instantly, against a timeout: 5s — and the CI worker log times that same import at ~42s on a cold container. Docker killed every probe roughly fifteen seconds before it could have succeeded, so the verdict was independent of the worker's state; the backend twin, same image and same env, escaped only because its probe is an HTTP GET. The fix hands the CLI the broker URL directly (celery -b "$$CELERY_BROKER_URL" inspect ping, ~3s, no application import), reaching the worker over Celery's app-name-independent broadcast exchange, with the compose $$ escaping verified against a throwaway service rather than assumed. Merging it did not turn the gate green and the PR never claimed it would: the stack came up, the suite executed for the first time in the repository's history, and 17 failed, 5 passed — seventeen product defects that a gate which never started had never been able to report. Touching that compose file at all also turned Security Scan red on ten pre-existing fake credentials inside it, because the repo's root .secrets.baseline parses fine and contains zero records; silenced inline, and the empty baseline reported rather than quietly regenerated (PR #1708)
• A feature merge left dev unable to run its own migrations at all, and the end-to-end gate reported it as a build failure rather than a broken database — two migration files declared revision = "186" under the same down_revision = "185": 186_add_signup_credit_claims.py, merged 21.08, and 186_add_deliverable_jobs.py three days later, with 187_publish_deliverable_jobs_to_zero.py chained onto the now-ambiguous "186". alembic upgrade head answers UserWarning: Revision 186 is present more than once and then FAILED: The script directory has multiple heads (due to branching), so the e2e backend container exited 255 during migrations and Build & start backend stack died before celery_worker was ever started — the same wall a fresh-database deployment hits, read out of the job's own uploaded backend-stack-logs rather than reproduced locally (run 32711664790). The repository already ships the check that would have caught it, scripts/check_migration_flow.py, and no CI job runs it. Which of the two duplicates moves is the whole decision, and it is not symmetric: 186 stays on signup_credit_claims, because every deployment that upgraded since 21.08 has that id stamped in its alembic_version table, while the newer pair has never been applicable to any database — so renumbering it to 187/188 and re-chaining costs nothing, and tests/unit/test_zero_publication_deliverable_jobs.py pins both the literal revision: str = "187" and the file name, so it moves with them. alembic merge is the tool this looks like it wants, and the body says why it is the wrong one: it resolves the branching and leaves the duplicate id, which is the only thing alembic_version ever stores. Verified with alembic's own ScriptDirectory in a single interpreter — heads ['186', '187'] on origin/dev against ['188'] on the branch — the repo's untriggered checker green on all four scenarios against a throwaway pgvector/pgvector:pg17, and the unit suite identical either side of the change (PR #1713)

lemonade-sdk/lemonade    ·  4 merged · all in v11.6.0

All four are in release-v11.6.0 and ship in that release, whose announcement gathers three of them into one line — "a wave of correct HTTP error statuses across /embeddings, /rerank, /slots, /tokenize, the Anthropic bridge, and SSE streaming, by @Yigtwxx and @fl0rianr — thanks @ekenberg!" — where @ekenberg is the reporter whose diagnosis #2975 credits.

• 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    ·  8 merged · 1 review landed

• Someone else's fix for hung Open Graph metadata fetches still hung on two of its three paths — the PR bounded /og/skill, /og/plugin and /og/profile at 1.5s, and both its code comment and specs/og-routes.md claimed parity with fetchImageDataUrl, but withOgFetchTimeout clears its deadline the moment work() resolves — and for fetch that is when the response headers arrive, so response.json() ran outside the armed window and an origin answering with headers and then stalling the body parked the card exactly as before, which is the failure the PR existed to remove. The reference file says the opposite: fetchImageDataUrl arms the timer at :73, reads the body at :79 and clears at :83, both phases inside. The PR also disagreed with itself — its own fetchPublisherOgMeta puts the whole Convex query inside work(), so one of the three paths bounded transport plus decode and two bounded transport alone, against a spec that states a single rule for all three. What settled it was a run rather than an argument: a body that honours the signal but never resolves leaves fetchSkillOgMeta pending after twenty timeout periods of virtual time, while the identical mock driven through fetchImageDataUrl returns null at 1.5s — both assertions green on that branch, so the two helpers were measurably not equivalent. The test added with the fix covered only the header phase, its stalling helper never resolving a Response at all, so the invariant the spec claims had nothing pinning it. The shape I proposed changes no helper: move the parse inside work(), which is what main carries today. On the archive half of the same PR I ran the negative control its fake-timer rewrite invited — restoring only server/convexProxy.ts from main while keeping the new test file gives 2 failed | 16 passed, the hung-metric case failing on its intended message, so the rewrite kept its teeth — and flagged that advanceTimersByTimeAsync(500) now resolves unconditionally, which makes the bound stricter than the version it replaced while the literal 500 stops being a millisecond claim. The author answered all three threads with one commit, and that commit is the head that merged (review on PR #3471)
• A skill catalog repo publishing through ClawHub's own reusable workflow could not set a changelog, a category or topics — .github/workflows/skill-publish.yml forwarded owner and tags and nothing else, so a publisher who wanted catalog metadata had to abandon the supported workflow and hand-roll a CLI job, while clawhub skill publish had declared --changelog, --categories and --topics the whole time. The three inputs travel the route owner and tags already use — workflow_call input, INPUT_* environment variable, blank-is-absent guard, conditional append onto the argument list subprocess.run executes — so parsing and validation stay in the CLI and the server and no permission, secret or publish path changes. Two things did not fall out of that pattern. Clearing metadata needed its own signal: the CLI separates an omitted --categories from --categories "" (hasExplicitCatalogMetadata tests !== undefined, parseCsv("") returns []), and a workflow_call string input delivers both as "", so a truthiness guard alone would leave callers able to set catalog metadata through the supported workflow and never able to clear it — hence per-field clear_categories and clear_topics booleans, which stop the run rather than silently pick a winner when one is passed together with a non-empty value of its own field. And the step had to echo the resolved command, because a dry run prints only the publish JSON and SkillPublishResult carries no catalog metadata, so a forwarded flag otherwise left no trace anywhere — which pulled the log-injection shape of #3447 onto this side: quote_for_log keeps shlex.quote for the copy-pasteable common case and falls back to json.dumps when the quoted form is not printable, decided by str.isprintable() so no list of control characters has to be maintained and --changelog 'Adds 日本語 notes' stays readable. Review left one finding and it was the right one: changelog was being .strip()ed, and Markdown carries meaning in exactly that whitespace — two leading spaces nest a list item, two trailing spaces are a hard line break — so the supported workflow altered a publisher's changelog where the direct CLI does not. It is forwarded verbatim now; categories and topics, being slug lists, keep their trim. docs/cli.md also gained the blast radius in its own paragraph next to skill_path, because publish.ts:134 suspends the unchanged short-circuit whenever catalog metadata is supplied, so one catalog-wide categories value releases a new patch version of every selected skill — and the docs promised that workflow "skips unchanged skills". Every claim is a dry-run dispatch of a throwaway caller repo pinning ClawHub by full commit SHA and passing no secrets: four jobs for flag absent, flag with a value, flag with an empty value and the two fields cleared independently; one deliberately red run for the conflict guard, in its own dispatch because continue-on-error is not accepted on a job that calls a reusable workflow; and two SHA-differential pairs for the findings above, where the control-character half is read from gh api .../annotations — one notice before, empty after — because a line the runner accepts as a command is removed from the log rather than printed (PR #3414)
• A publisher's own changelog text could write commands into the runner that published their package — the reusable package publish workflow logs the command it is about to run, and it built that line with shlex.quote, which is shell quoting: correct for the re-runnable .sh file the same string feeds, and the wrong tool for a log, because it leaves an embedded newline intact. A changelog carrying a line break therefore opened a second stdout line, and the Actions runner parses stdout for :: workflow commands — so caller-controlled release notes reached a surface that sets outputs, masks values and adds annotations in the job that holds the publish credentials. Fixed with a quote_for_log helper that keeps shlex.quote for values that are already printable and falls back to json.dumps for anything that is not, which escapes every control character and non-ASCII byte, so one argument stays one log line regardless of input; the executed argument list and the generated script are untouched. What made the fix provable was reading the right artifact: two jobs in one dispatch of a throwaway caller repo, same multi-line input, differing only in the pinned ClawHub SHA — and the evidence is not the log text but gh api .../check-runs/<id>/annotations, one notice annotation on the before job and an empty list on the after job. A line the runner accepts as a command is removed from the downloaded log, so grepping the log alone reads the result backwards (PR #3447)
• Every open pull request in the repository had six red checks that belonged to none of them — bun audit inside ci:static had been failing on main itself since 2026-08-06, and static, unit, packages, types-build and e2e-http are mirror jobs whose only step is test "$PR_GATES_RESULT" = "success", so one advisory scan painted the whole board red and no contributor could tell a real failure from the shared one. Three of the four packages were pinned by the repository's own overrides block, which is exactly why Dependabot could not clear it — its own PR carried the same mermaid bump and stayed red, because a bot that edits dependencies cannot reach the pin that actually resolves the tree — and nanoid needed a new override entry outright, since it arrives only transitively through postcss. Each was moved to the first release that carries the fix rather than the newest, and the --ignore list was left untouched, so the scan came back No vulnerabilities found without silencing anything (PR #3446)
• 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    ·  4 merged · 2 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)
• A single tool card disagreed with itself about the type of a parameter — for any array whose items carry an enum, mcporter list printed the union on its own and dropped the array shape, while the call example rendered directly underneath it still showed an array: the signature read sources: "web" | "news" | "images" and the line below it read sources: ["web"]. formatTypeAnnotation tested enumValues before it reached the type switch, so the array case could never run, and the metadata was never at fault — extractOptions sets both type: 'array' and enumValues for these descriptors, and pickExampleLiteral already special-cased the pair to emit ["web"], which is why the example beneath the signature was right while the signature was wrong. The defect is narrow in a way that names its own cause: an array of plain numbers renders correctly and so does a scalar enum, because getEnumValues collects string members only. It is also the unclosed half of a repair the project had already accepted — #221 was closed by 938594c, which taught this same function to read items.type, and the enum branch sitting in front of the switch has shadowed that branch ever since, so the argument is "this function contradicts a decision already merged into it" rather than a new opinion about how types should print. What kept it from being read as cosmetic was stated in the body rather than left for review to raise: the runtime compensates for the wrong signature, since the call path wraps a lone string into [value] through schemaAllowsArray, so no call actually breaks and what is broken is only the contract a human or an LLM reads — which is also the reason the fix carries no regression risk, and saying so myself turned a finding a reviewer could have downgraded into one with its blast radius already measured. The union is wrapped when the option is an array, parenthesised for a multi-member union and bare for a single member, both valid TypeScript. A second, separately marked commit fixes the identical ordering in buildPlaceholder, where a generated CLI advertised an enum array as a single-value flag while the parser emitted right next to it splits the value on commas — offered in the body as three lines and one assertion to drop if the maintainer would rather not take it, and taken as it stood. A third instance of the same inconsistency, inferSchemaDisplayType rendering output-schema enums as string, is real but off-thesis and was left out of scope in writing rather than silently. The proof is real CLI runs against an ad-hoc stdio MCP server rather than the formatter alone, so the discovery path is exercised instead of the helper, and the added coverage fails 2/2 and 1/1 against unpatched main. Reviewed two minutes after opening with zero findings, green on all three CI platforms, and merged with both commits (PR #309)
• A tool whose name contains a dot was unreachable through the configured-server path — mcporter call proof.browser.navigate resolved the server as proof and the tool as browser, then reported that proof.browser had provided no usable tool metadata, because resolveCallTarget split the selector with split('.', 2) and the trailing segment was dropped; dotted names such as browser.navigate are ordinary in MCP servers. The correct helper sat nineteen lines below the defect in the same file — splitServerToolSelector, added when issue #218 was closed but wired only into the ad-hoc HTTP branch — and call-expression-parser.ts and list-command.ts both resolve the same string correctly, so mcporter list reached a selector mcporter call could not: four surfaces, three of them right, and the fix is handing the fourth the helper its own module already contained. A leading or trailing dot still produces the same Missing server name. / Missing tool name. errors, since the helper returns undefined for both and the fallback keeps the old split point. The second, separately marked finding came out of the merge wave rather than the issue tracker: PR #325 had just canonicalized URL serialization in http-utils.ts, and splitHttpToolSelector eight lines above it still rebuilt the server URL as url.origin plus a hand-built path, so everything after the path was discarded — https://example.com/a/mcp?tenant=b opened against https://example.com/a/mcp, and because findServerByHttpUrl compares through normalizeHttpUrl, a configured server whose URL carries a query stopped matching, so the call silently fell through to an ad-hoc server carrying none of that server's headers or OAuth settings. The proof measures where the request went rather than what the helper returned: a local server that logs the request line and answers 404, same port and same log file on both revisions, POST /mcp before and POST /mcp?tenant=b after. The fragment half came out of review and was measured before it was written — keeping url.hash would have been a new way to miss a configured server, since a fragment never travels on the wire but does survive into the comparison on one side only. One claim from my own exploration was cut before it reached the body: a www. prefix is normalized on both sides of that comparison, so it was not a defect (PR #333)
• Every command in a generated CLI died at module load if one schema property began with an uppercase letter — mcporter generate-cli derives each flag from the JSON Schema property name by prefixing a dash to every uppercase character, with no guard for the leading position, so Query emitted .option("---query <-query>") and commander threw while still constructing the option: nothing in the artifact ran, not even --help. Uppercase property names are what the .NET and Java MCP server SDKs emit by default. A second spelling failed quietly instead: no_cache becomes --no-cache, which commander reads as a negated boolean stored under cache and gives an implicit true, while the generated command reads the noCache key it computed itself — so a supplied value was dropped, and a required flag was reported missing even when the user had passed it. Fixing the first spelling exposed the class the crash had been hiding, and the review chain ran eight rounds through it: two legal properties that normalize onto one flag (Query beside query, no_cache beside noCache) make commander refuse the command outright, so flag names are now assigned across the whole property list instead of per property, with a later property taking the first -2, -3 suffix no other property spells naturally; ___ is a legal name whose every character normalizes away, leaving both an invalid flag and an unparseable cmdOpts. property access; 2fa is a legal key commander stores verbatim while cmdOpts.2fa does not parse, so a key that cannot be spelled as a property access is read by subscript; and a separator run or a trailing separator (foo__bar, baz_, and filter_Query, which reaches one by mixing both naming conventions) hands commander's attributeName() an empty segment, which throws at addOption rather than at construction, so it is invisible until the command is assembled. That last class is proven by exhaustion rather than by example: every property name up to five characters over a B _ - 2 . — 9330 of them — registered on a real Command, none rejected. Two adjacent findings shipped alongside it: a nullable array ({"type":["array","null"]}) lost its item type and enum members, because inferType normalizes that union to array while the two container checks beside it compared the raw value, so --scores 1,2 reached the tool as ["1","2"]; and a multi-word or reserved-word outputSchema.title was spliced verbatim into the emitted TypeScript, so Promise<Search Results> did not parse — the reserved-word half came from review, and the affected set was measured with tsc rather than assumed (30 words fail outright, five more parse into a keyword type that no longer describes the schema). The maintainer integrated the branch whole rather than writing a narrower replacement, preserved every fix, and closed the one hole release review had found on top of it: --query-3 and --query3 are distinct flags that share commander's query3 storage key, so a real generated CLI copied one value into both original JSON properties. Merged for v0.13.8 with co-author credit on the merge commit (19630dc) and a changelog line naming the contribution (PR #332, integrated as PR #339)

openclaw/lobster    ·  5 merged

• Every schema-validated step bought one more billed model call than its retry budget allowed — --max-validation-retries N sent N + 2 requests, and 0, the setting a user picks precisely because the model is expensive, still paid for a second call on every failing step. The attempt counter is 1-based and was compared against the budget plus one, so the initial request was never charged against it; attempt counts calls while the flag counts retries, which makes retries + 1 the correct bound and the extra + 1 pure overspend. Nothing surfaced it: the command still failed with the right error, so the only visible symptom was a cost_limit reaching its ceiling earlier than the configuration implied. Only the failure bound moves — a response that validates still returns on whichever attempt produced it, and with 0 retries payload.retryContext is now correctly never sent at all, which is what "no retries" should mean. The tests count the requests a stub adapter actually receives rather than matching on the error message, because both the old and the new code reject with the same message and the call count is the only thing that separates them; proven load-bearing by stashing the production change and keeping them, which fails the 0 case with 2 calls where 1 is expected. The flag's own description said only "retries when schema validation fails" — exactly the ambiguity the fix resolves — so it now states that it allows N extra calls after the first (PR #128, closes issue #127)
• Lobster could not write anything to disk on Windows the first time it needed a directory — a new user's very first llm.invoke, state.set, diff.last or approval gate died with ENOENT ... \llm.invoke\C:, a path they never asked for, and because the directory was created before the throw, running the same command again worked, which made a deterministic first-run failure read as flakiness. fs.mkdir(..., { recursive: true }) reports the first directory it created as an extended-length path (\?\C:\...) while the request stays a plain drive path; path.resolve preserves that prefix, so the fsync walk compared two values that can never converge and path.relative between the namespaces handed back an absolute path that the walk then stepped into. The fix maps only the namespaces that have a plain equivalent — drive-letter and UNC\, the latter matched case-insensitively because Windows accepts a lowercase unc component — and deliberately returns \?\Volume{GUID}\... untouched, since that is a legal LOBSTER_STATE_DIR and stripping its prefix would leave a relative path resolving against the current drive, a regression against what main already handled. The UNC half was measured rather than assumed: on a real SMB path the bug is quieter than the drive case — path.relative yields an empty first segment instead of an absolute one, so the walk breaks out, the run succeeds, and the fsync is silently skipped for every directory below the first created (PR #126, closes issue #125)
• Changing --temperature or --max-output-tokens on a prompt you had run before returned the old answer — the model was never called, the command reported success, and the only trace was a source: "cache" field deep in the output item, so someone tuning temperature saw identical output at every setting with no signal their flag was ignored; the same key drives --state-key, so a resumed workflow replayed the stale answer too. Both parameters were already resolved and sent to the adapter — they simply were not part of the value the cache is addressed by. They now enter the key unconditionally, with null as the identity of an omitted one: an identity no caller can produce, and distinct from an explicit 0. The part worth the review was the second iteration: hashing them only when set kept the omitted-parameter key byte-identical to earlier releases and preserved every existing entry, but left the same defect reachable from the other direction — an entry an earlier release stored for an explicitly sampled request sits under exactly the key an unsampled request computes, so after upgrading an unsampled call could be served a sampled answer. Versioning the cache identity closes that, at the cost of one cold cache on upgrade, which the PR body states as user impact rather than leaving it to be discovered (PR #130, closes issue #129)
• Workflows were billed for every repetition of a question only the first of which reached a model — _meta.cost reported a multiple of what the run actually spent, and cost_limit tripped early, aborting a run mid-way on a budget it had never spent. A cache or run-state hit re-emits the stored item verbatim, usage and all, and trackStepCost recorded any usage it found with no notion of where the item came from. What made this more than a one-line skip is where the replay marker can live: trackStepCost parses the stdout of every step, shell steps included, so a marker made of JSON fields is only as trustworthy as the least trusted command in the workflow — and neither source nor cached can carry it either, because a direct adapter's source defaults to its provider name and a live call through an adapter merely named cache is indistinguishable from a replay under both fields. The exemption is therefore keyed to provenance the command attaches in-process, set when an item is replayed rather than when it is stored, so entries already on disk are covered without invalidation. Four further rounds each came from a measurement rather than a re-read: a paused run restoring what it billed and not only what it spent; a discarded wait: "any" branch buying against its own buffer so only the winner's charges reach the run; a live call priced from the charge it opened rather than from whatever the step pipeline left on the item; and a validator-rejected attempt opening its own charge. Merged after the maintainer took the branch himself — replacing the key-based suppression with a private provenance restore at the resume-state boundary, which is the same rule enforced where the input is trusted (PR #134, closes issue #133)
• A workflow step's timeout_ms did not bound the model call inside it — an adapter that never answers kept the step, and the run, waiting past its deadline indefinitely, so the one control an operator has over a hung provider did not apply to the only call that can hang. The signal existed and was even handed to the adapter; what was missing is that handing an adapter a signal is a request, and nothing enforced it. The call is now raced against the run's signal, so an adapter that ignores cancellation no longer outlives the deadline that was configured for it. The interesting half of this PR is what it removed: while it was open, an unrelated cancellation overhaul landed on main and threaded the signal through every state and cache write, which overlapped several of the guards this branch had added. Rather than defend them, each was deleted and re-measured — the two deadline re-checks after persistence left the suite entirely green when removed, so they were dropped along with the two tests that guarded them, which had asserted that a cancelled run's answer stays stored for the retry while main had since decided the opposite and rolls that write back. What survived the subtraction is the race itself, and the evidence narrowed to one line: timed-out step fails by default (on_error: stop) fails on main and passes here, with the other 15 failures in the cancellation, timeout and invoke suites identical on both (PR #132, closes issue #131)

openclaw/fs-safe    ·  8 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)
• A tar entry whose path began with ./ was written somewhere other than the path fs-safe had validated, and with stripComponents set the extraction crashed outright — chmod ENOENT on a path nothing had created. One policy had two implementations running on the same archive: node-tar's strip counts the . and empty components of a path, while fs-safe's own stripArchivePath() drops them before counting, so ./src/app.ts under stripComponents: 1 was extracted by node-tar as src/app.ts and recorded by fs-safe as app.ts; the mode pass then chmod'd a path that did not exist, and because the Rust extractor applies the fs-safe rule, the two backends built different trees from the same bytes. Neither implementation is wrong on its own — the defect is that the rule exists twice, which is also what makes it cheap to evidence, since one side is already the accepted answer. The fix makes the validated fs-safe path authoritative: node-tar is handed strip: 0 and each accepted entry's path is set to the exact value it was validated, limited and collision-checked under, so extraction, the mode pass and the native backend now agree by construction. Filter callbacks still receive the pre-strip path, so caller-visible filter inputs and PAX policy do not move (PR #142)
• A ZIP entry carrying both symlink mode bits and the directory bit skipped the symlink rejection entirely — for that one shape the link policy the documentation promises was unreachable code. extractZip() tested entry.dir first and continued after preparing the directory, so an entry whose mode says symlink but whose name ends in a slash — or whose DOS attributes set the directory bit — never reached the entry-link throw sitting a few lines below, and the JavaScript backend accepted an archive the Rust one rejects, since native zip_kind() tests the mode before is_dir(). The rejection now runs before output preparation and after explicit filtering, so a filter that excludes the entry still wins and no callback metadata changes. What moved this PR was the shape of the proof rather than the diff: green vitest output left it blocked, and it cleared only once the same script ran against dist/ on both revisions and then again through pnpm pack + npm install <tgz> — in this repository a built artifact is proof and a test run is supplemental, which is worth knowing before writing the body rather than after (PR #141)
• Process-wide lock defaults were silently ignored by every synchronous acquire — configureFsSafeLocks({ timeoutMs }) bounded the asynchronous manager and did nothing for acquireFileLockSync() or withFileLockSync(), so a caller who had configured a timeout waited unbounded on a contended lock. src/file-lock-sync.ts never imported getFsSafeLockConfig() at all: retry, timeoutMs, staleMs and staleRecovery were read from per-call options only, while docs/config.md and docs/sidecar-lock.md describe all four as process-wide. Making an ignored setting effective is a behaviour change for anyone relying on it being ignored, so this collected an Owner decision: Required stamp — which is not a rejection but an unanswered who does this break, and it was answered by measurement rather than argument: code search across the org finds no configureFsSafeLocks() caller outside fs-safe itself, and the one real consumer of acquireFileLockSync() (openclaw/openclaw, src/infra/file-lock-sync.ts:22-29) passes every affected field per call, so per-call precedence makes the change a no-op there. Per-call options still take precedence, explicit zeros survive, and a per-call retry object replaces the configured one as a whole. The regression is bounded so that a reintroduction fails fast instead of hanging — it asserts six exclusive-create attempts where one is expected, rather than parking a worker on the unbounded wait it is testing for (PR #140)
• Six documentation pages were reachable only by guessing their URL, and the navigation listed a seventh that does not exist — durability, migrating-to-0.5, permissions, public-api, secure-file and walk appeared in no sidebar section of the rendered site, while pinned-open.md was registered with no page behind it, and the drift stayed invisible because the builder's .filter(Boolean) swallowed the dangling entry instead of failing on it. The pages are the small half; the guard is the load-bearing one, and my first version of it would have made the repository worse. It read docs/ flat while the builder's own allMarkdown() recurses, so a future docs/guides/x.md would have passed PR CI and then broken the build after merge — a test that relocates a failure onto main is worse than no test. Nine review cycles closed on one distinction: a helper that re-implements production discovery proves only that the mirror agrees with itself, so what finally held is a committed nested fixture asserted by exact keys, plus a spawn of the shipped scripts/build-docs-site.mjs against a temp workspace — it reads docs/ from process.cwd(), and that seam is what lets the real builder run against a fixture instead of a copy of itself. The maintainer's integration answered the same problem one level up by extracting scripts/docs-site-navigation.mjs, so the builder and the test now import one sections list, readDocPages() and assertNavigationCoversDocs(); a missing, nonexistent or duplicate registration now fails before the build replaces existing site output. The guard paid for itself during that integration, catching a one-character anchor (config.md#configurefssafelocksconfig) that had been broken hours earlier in the lock PR's own documentation (PR #143)
• 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)