FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Fix eleven correctness bugs from the source review by nullcache · Pull Request #10 · nullcache/corecoder-ts · GitHub

Fix eleven correctness bugs from the source review - #10

Merged
nullcache merged 5 commits into
minfrom
fix/source-review-bugs
Aug 17, 2026
Merged

Fix eleven correctness bugs from the source review#10
nullcache merged 5 commits into
minfrom
fix/source-review-bugs

Conversation

Copy link
Copy Markdown
Owner

One commit, eleven fixes, each traced to a verified failure during the full-source review. All fixes carry regression tests — 54/54 pass locally (Node 20).

P0 — data corruption / dead on install / documented usage traps

  1. edit_file $-pattern corruption: String.replace interprets $$/$&/$' in the replacement — writing a Makefile with $$ silently halved it. Now replaced via a function callback.
  2. npm-installed CLI did nothing: bin symlinks made argv[1] differ from import.meta.url, so isDirectRun was false. Now realpath'd.
  3. for-await break poisoned history: the README's own library example left assistant tool_calls unanswered forever (every later request 400s). chat() now backfills in a finally.
  4. streams couldn't be aborted or timed out mid-body, and an abort leaked the connection (provider keeps generating/billing). The SSE reader now honors the signal via reader.cancel() and races an idle timer per read.

P1 — hangs / silently wrong

  1. bash kill escalates SIGTERM → SIGKILL after 5s (a trap '' TERM child wedged the whole REPL).
  2. makeAllTools() factory: constructing a second Agent no longer steals the first one's SubAgentTool parent binding.
  3. Context-compression LLM calls now take the turn's AbortSignal (^C was dead for up to minutes).
  4. Tool-call arguments:"null" (seen on OpenAI-compat servers) coerces to {} instead of crashing the REPL.
  5. Layer-0 truncation for a single message that alone busts the window (was a permanent 413 loop /compact couldn't fix).
  6. glob: ./-prefixed patterns and [ch]/[!x] classes matched nothing, teaching the model files don't exist.
  7. cwd unified: tracked cwd lives in paths.ts and every tool resolves relative paths against it; cd parsing follows ;/|| chains.

Origin split (from the port-fidelity audit): 1/2/4/7/10 are port-introduced, 3 comes with the generator redesign, 5 completes PR #2, 6/9/11 are inherited from the Python original, 8 is shared but the port amplified the blast radius.

P0 (data corruption / dead-on-install / documented-usage traps):
- edit_file: replace via a function so $$/$&/$' in new_string are
  written literally instead of corrupting the file
- CLI: realpath argv[1] before comparing to import.meta.url — npm bin
  symlinks made the installed command silently do nothing
- Agent.chat: finally-backfill tool replies when a consumer abandons
  the generator (for-await break poisoned the history permanently)
- SSE body phase: abort now cancels the reader (unblocks reads and
  closes the connection) and each read races an idle timer, so a
  stalled stream errors instead of hanging the turn forever

P1 (hangs / silently wrong):
- bash kill: SIGTERM now escalates to SIGKILL after a 5s grace, so a
  TERM-trapping child can't wedge the REPL; timer cleared on exit
- tools: makeAllTools() factory — a second Agent no longer re-points
  the first Agent's shared SubAgentTool at itself
- context: compression LLM calls take the turn's AbortSignal
- llm: tool-call arguments parsing coerces null/array/scalar to {}
- context: layer-0 truncation for a single message that alone exceeds
  the window (was an unrecoverable 413 loop)
- glob: strip leading ./ and support [ch]/[!x] character classes
  (both silently matched nothing)
- cwd: tracked cwd moved to paths.ts so every tool resolves relative
  paths against it, and cd parsing follows ;/|| chains too

Adds 11 regression tests (54 total pass).
- Revert the cross-tool cwd unification and ;/|| parsing (magic state,
  out of min's scope — the split-brain stays a documented limitation)
- Replace silent oversized-message truncation with an up-front refusal
  in Agent.chat (the truncation math ignored the fixed overhead and
  silently altered user input; refusing is honest and simpler)
- Kill sequence now starts at most once (a second trigger orphaned the
  first SIGKILL escalation timer, which settle() could never clear)
- rounds() closes the inner LLM generator in a finally, so abandoning
  a turn during text streaming cancels the SSE reader and releases the
  connection (hand-driven .next() gets no auto-close from yield*)
- isDirectRun's catch falls back to a plain compare instead of
  silently refusing to start
- Keep the [ch]/[!x] character classes: verified native in Python
  pathlib.glob — this is port parity, not added syntax

Tests: drop the cwd case, add oversized-refusal and
abandon-during-text-streaming-closes-connection. 54/54 pass.

Copy link
Copy Markdown
Owner Author

Revised per review. Accepted and fixed: kill-sequence single-shot flag (orphaned escalation timer), inner-generator close in a finally (abandoning during text streaming now cancels the SSE connection — new regression test), oversized input now refused up front instead of silently truncated (the truncation math ignored fixed overhead), realpath catch falls back to a plain compare. Withdrawn from this PR: cross-tool cwd unification and ;/|| parsing (out of min's scope). Kept with evidence: [ch]/[!x] character classes and ./ stripping are native in Python pathlib.glob — port parity, not added syntax. 54/54 tests pass.

…asses

- rounds() awaits stream.return() and sseEvents awaits reader.cancel():
  for-await's break waits on the return() chain, so teardown now
  completes before break returns, and a throwing inner finally surfaces
  as a handled rejection instead of an unhandled one. The regression
  test asserts closure immediately after break, no sleep.
- Character classes now follow Python fnmatch semantics on the edges:
  ^ is a literal member (only ! negates), ] right after [ or [! is a
  literal member, and an invalid range like [z-a] matches nothing
  instead of throwing. The doc comment states the supported subset
  explicitly — no claim of a full glob dialect.

Copy link
Copy Markdown
Owner Author

Round 2 addressed. Blocker: the teardown chain is now fully awaited (rounds() awaits stream.return(), sseEvents awaits reader.cancel()) — the regression test asserts the connection is closed immediately after break returns, with no sleep. Glob classes: took both suggested options at once — minimal fnmatch-semantic fixes (^ literal, []], [!]], [z-a] → matches nothing) plus a doc comment that promises exactly the supported subset and nothing more. 55/55 tests pass.

…onesty

- kill goes straight to SIGKILL on the whole process group: Python's
  subprocess.run timeout behavior (kill outright), hardened from the
  direct child to the group. The TERM->timer->KILL machinery is gone.
- remove the oversized-input pre-check: it judged user input by an
  estimate and could refuse legitimate messages; the provider's own
  context error is the honest answer.
- /tokens prints 'unknown' when the provider has never reported usage,
  instead of dressing unknown up as 0 (usageSeen on LLMClient).
- write the token-accounting principle into context.ts, document the
  no-usage degradation on observe(), compress the sseEvents comments.

Copy link
Copy Markdown
Owner Author

Final revision per the adjudication: kill is now a straight group SIGKILL (subprocess.run parity, hardened to the group — state machine deleted), the oversized-input pre-check is gone (an estimate must not veto user input), /tokens says unknown instead of 0 when no usage was ever reported (usageSeen), the token-accounting principle is written into context.ts, and the SSE comments are compressed. Net for this round: −55 lines. 55/55 tests pass.

nullcache merged commit cc4bf6a into min Aug 17, 2026
nullcache deleted the fix/source-review-bugs branch August 17, 2026 10:24
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant


Back | FazBrowse Home | New Git URL