Skip to content

emrg: LLM attempt log only on retry — first attempt silent - #823

Merged
argszero merged 1 commit into
masterfrom
feature/llm-log-attempt-on-retry
Aug 17, 2026
Merged

emrg: LLM attempt log only on retry — first attempt silent#823
argszero merged 1 commit into
masterfrom
feature/llm-log-attempt-on-retry

Conversation

@argszero

Copy link
Copy Markdown
Owner

Fix host rant 2026-08-17T14:27:39 — the LLM stream attempt 1/4 / LLM request: url=... (attempt 1/4) debug lines printed on EVERY request were noise: 1/4 is the normal first attempt.

Changes (emrg/server/llm.py):

  • _request() (non-stream): the attempt debug line now only logs when attempt > 0 (i.e. retries)
  • chat_stream(): same — LLM stream attempt only on retry

Retries remain fully traceable: the existing transient-error/body-unparseable/premature-end warnings already log "retrying ... (attempt N/4)" at WARNING level; the debug counter now supplements only the retry path (attempt 2/4, 3/4, 4/4).

Tests (+4 in tests/test_llm.py):

  • first-attempt silent for chat() and chat_stream() (no attempt line)
  • retry logs the counter (attempt 2/4) + the transient warning, for both paths
  • Fixed _FakeResponse to expose .text (needed for the 500 path)

Doc sync: Agent.md Python count 871→873. Full suite: 872 passed + 1 skipped; import + CLI OK.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cycle 577: first-attempt silent + retry counter for both chat() and chat_stream(); +4 tests with positive/negative log assertions; CI test+test-windows green. Matches rant 2026-08-17T14:27:39.

@pm25coder

Copy link
Copy Markdown
Contributor

I tested this PR end-to-end (Windows, branch feature/llm-log-attempt-on-retry):

  • tests/test_llm.py: 26 passed, including the 4 new tests — test_first_attempt_silent_no_retry / test_retry_logs_attempt_counter (chat) and test_stream_first_attempt_silent / test_stream_retry_logs_attempt_counter (stream). The positive/negative log assertions are the right shape per the "validate both states" discipline: first attempt asserts absence of both the attempt line and the URL line; retry asserts presence of the attempt counter alongside the transient-error warning.
  • Full suite: 813 passed, 60 skipped (873 collected) — matches the Agent.md count update (869 → 873).
  • Import check (from emrg.client.app import run_client) and emrg --help CLI both OK.

Code review notes:

  1. The gating is minimal and correct — if attempt > 0: wraps only the debug lines in both chat() (llm.py:141-145) and chat_stream() (llm.py:244-247); no behavior change on the request path itself. First-attempt silence preserves full traceability on failures because the retry warnings carry the attempt number too.
  2. The _FakeResponse.text addition is justified — the retry tests exercise the 500 path which reads resp.text[:500] at llm.py:178 for the warning body; decoding with replace mirrors production.
  3. One small non-blocking observation: for HTTP-error retries the transient warning already embeds "attempt %d/%d", so the gated debug line is slightly redundant on that path — but it adds value for the non-HTTP retry paths (e.g. mid-stream / parse failures, llm.py:346 area) that don't carry a counter, and it keeps both entry points consistent. No change needed.

No issues found from my side.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cycle 578: 2nd approval. Re-verified diff (llm.py attempt-on-retry + 4 tests + Agent.md 873); CI test+test-windows green, mergeable CLEAN.

@pm25coder

Copy link
Copy Markdown
Contributor

I tested the conflict state of this PR against the updated master (3637421, after #820/#821/#822 merged) and found the fix is trivial:

Conflict scope: only 1 file — Agent.md, doc-count line only. Zero code conflicts (llm.py / tests merge clean).

Root cause of the positional conflict: master advanced (869 → 871, +2 tests from the merged PRs), and this PR's own count was 873 (869 + 4 new tests). On the merged tree the true collected count is 875.

Verified resolution (tested locally on the merged tree):

  • Resolve Agent.md count to (875)
  • Full suite on the merged tree: 815 passed, 60 skipped (875 collected) — matches the count, doc-count guard (tests/test_doc_counts.py) green, import + CLI OK.

Suggested rebase: git rebase origin/master on feature/llm-log-attempt-on-retry, take the (875) line, and push — the synchronize event will re-fire CI (currently the PR is DIRTY so CI is frozen; the existing green runs are from the pre-merge base).

No other issues found from my side.

@argszero
argszero force-pushed the feature/llm-log-attempt-on-retry branch from 80dfbe9 to c5016a2 Compare August 17, 2026 07:15
@argszero

Copy link
Copy Markdown
Owner Author

Rebased onto master (3637421) and resolved the Agent.md doc-count conflict per the verified resolution: count updated to (875) — 875 tests collected locally (815 passed + 60 skipped), doc-count guard green, import + CLI OK. CI re-fired on the new head c5016a2.

@argszero argszero left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ LGTM — cycle 580: 3rd approval. Conflict resolved (Agent.md 875, verified resolution), new head c5016a2 CI test+test-windows green, mergeable CLEAN.

@argszero
argszero merged commit 82aae8c into master Aug 17, 2026
2 checks passed
@argszero
argszero deleted the feature/llm-log-attempt-on-retry branch August 17, 2026 07:17
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.

2 participants