Skip to content

fix(backends): strip empty HTML comment markers from Codex reasoning summaries#124

Merged
matdev83 merged 3 commits into
mainfrom
bugfix-codex-reasoning-empty-comment-markers
Jul 10, 2026
Merged

fix(backends): strip empty HTML comment markers from Codex reasoning summaries#124
matdev83 merged 3 commits into
mainfrom
bugfix-codex-reasoning-empty-comment-markers

Conversation

@matdev83

@matdev83 matdev83 commented Jul 9, 2026

Copy link
Copy Markdown
Owner

Summary

  • Sanitizes Codex reasoning summary text deltas by removing empty HTML comment markers in both the OpenAI Codex and Codex app-server backends.
  • Introduces a shared codexreasoning sanitizer package with focused unit tests.

Test plan

  • Bugbot review of uncommitted changes found no issues
  • make test-unit passes
  • make quality-checks passes

…summaries

Codex app-server and OpenAI Codex backends now pass reasoning summary text deltas through codexreasoning.StripEmptyHTMLCommentMarkers before emitting them as client-visible reasoning events.

Add the shared codexreasoning package with unit tests for the sanitizer.

Co-authored-by: Cursor <cursoragent@cursor.com>
@coderabbitai

coderabbitai Bot commented Jul 9, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 73fb2124-869a-4304-a8ae-412699dc4e95

📥 Commits

Reviewing files that changed from the base of the PR and between 7a77d64 and 52a822c.

📒 Files selected for processing (8)
  • EchoesVault/daily/2026-07-09.md
  • go.mod
  • internal/plugins/backends/codexappserver/stream.go
  • internal/plugins/backends/codexappserver/stream_internal_test.go
  • internal/plugins/backends/openaicodex/stream.go
  • internal/plugins/backends/openaicodex/stream_internal_test.go
  • internal/plugins/backends/protocols/codexreasoning/sanitize.go
  • internal/plugins/backends/protocols/codexreasoning/sanitize_test.go
📜 Recent review details
⏰ Context from checks skipped due to timeout. (1)
  • GitHub Check: qa
🧰 Additional context used
📓 Path-based instructions (5)
**/*

📄 CodeRabbit inference engine (Custom checks)

Do not introduce hardcoded credentials, API keys, tokens, private keys, passwords, production secrets, or sensitive internal URLs.

Files:

  • go.mod
  • internal/plugins/backends/protocols/codexreasoning/sanitize.go
  • EchoesVault/daily/2026-07-09.md
  • internal/plugins/backends/protocols/codexreasoning/sanitize_test.go
  • internal/plugins/backends/codexappserver/stream_internal_test.go
  • internal/plugins/backends/codexappserver/stream.go
  • internal/plugins/backends/openaicodex/stream_internal_test.go
  • internal/plugins/backends/openaicodex/stream.go
**/go.mod

⚙️ CodeRabbit configuration file

**/go.mod: Review dependency changes for unnecessary additions, major-version upgrades, replace directives, indirect dependency churn, and module path correctness.

Files:

  • go.mod
**/*.go

📄 CodeRabbit inference engine (Custom checks)

**/*.go: For server, CLI, worker, or network Go code, ensure context.Context is propagated correctly, cancellation is respected, and new goroutines cannot leak indefinitely.
Do not make accidental public API breaks in Go code: under pkg/** or anywhere exported Go identifiers are changed, warn if the PR changes exported types, function signatures, error behavior, JSON fields, CLI flags, config keys, or documented behavior without clearly explaining the compatibility impact.

Files:

  • internal/plugins/backends/protocols/codexreasoning/sanitize.go
  • internal/plugins/backends/protocols/codexreasoning/sanitize_test.go
  • internal/plugins/backends/codexappserver/stream_internal_test.go
  • internal/plugins/backends/codexappserver/stream.go
  • internal/plugins/backends/openaicodex/stream_internal_test.go
  • internal/plugins/backends/openaicodex/stream.go

⚙️ CodeRabbit configuration file

**/*.go: Review as production Go code. Prioritize correctness, race conditions, goroutine leaks, context cancellation, timeout handling, error wrapping, nil-pointer risks, resource cleanup, defer placement, API compatibility, interface design, dependency boundaries, and testability. Avoid generic style comments when gofmt/golangci-lint already covers the issue.

Files:

  • internal/plugins/backends/protocols/codexreasoning/sanitize.go
  • internal/plugins/backends/protocols/codexreasoning/sanitize_test.go
  • internal/plugins/backends/codexappserver/stream_internal_test.go
  • internal/plugins/backends/codexappserver/stream.go
  • internal/plugins/backends/openaicodex/stream_internal_test.go
  • internal/plugins/backends/openaicodex/stream.go
internal/**

⚙️ CodeRabbit configuration file

internal/**: Focus on package boundaries, hidden coupling, unexported API design, concurrency safety, deterministic behavior, and whether logic belongs in this internal package.

Files:

  • internal/plugins/backends/protocols/codexreasoning/sanitize.go
  • internal/plugins/backends/protocols/codexreasoning/sanitize_test.go
  • internal/plugins/backends/codexappserver/stream_internal_test.go
  • internal/plugins/backends/codexappserver/stream.go
  • internal/plugins/backends/openaicodex/stream_internal_test.go
  • internal/plugins/backends/openaicodex/stream.go
**/*_test.go

⚙️ CodeRabbit configuration file

**/*_test.go: Review tests for meaningful assertions, table-driven coverage, race-prone tests, t.Parallel misuse, nondeterminism, leaked goroutines, real network or filesystem dependencies, fragile sleeps, and missing edge cases. Prefer testing observable behavior over implementation details.

Files:

  • internal/plugins/backends/protocols/codexreasoning/sanitize_test.go
  • internal/plugins/backends/codexappserver/stream_internal_test.go
  • internal/plugins/backends/openaicodex/stream_internal_test.go
🧠 Learnings (1)
📚 Learning: 2026-07-01T22:57:42.953Z
Learnt from: matdev83
Repo: matdev83/go-llm-interactive-proxy PR: 101
File: pkg/lipsdk/scope/context.go:0-0
Timestamp: 2026-07-01T22:57:42.953Z
Learning: In this repository, when defining unexported Go `context` key constants of type `ctxKey int` (e.g., `const ( kFoo ctxKey = iota + N )`), preserve the `iota + <offset>` pattern and keep the existing `<offset>` values rather than simplifying to plain `iota`. These per-package offsets are part of the repo-wide convention to avoid key collisions across packages, and each such constant set should include a short explanatory comment (for example: “offset avoids collision with other packages' context keys”).

Applied to files:

  • internal/plugins/backends/protocols/codexreasoning/sanitize.go
  • internal/plugins/backends/protocols/codexreasoning/sanitize_test.go
  • internal/plugins/backends/codexappserver/stream_internal_test.go
  • internal/plugins/backends/codexappserver/stream.go
  • internal/plugins/backends/openaicodex/stream_internal_test.go
  • internal/plugins/backends/openaicodex/stream.go
🪛 markdownlint-cli2 (0.22.1)
EchoesVault/daily/2026-07-09.md

[warning] 1-1: First line in a file should be a top-level heading

(MD041, first-line-heading, first-line-h1)

🔇 Additional comments (8)
go.mod (1)

3-3: 📐 Maintainability & Code Quality

Confirm the Go version pin is intentional.

Line 3 now pins a patch-level go directive, which will raise the minimum toolchain required by CI and contributors. Please verify the repo really wants to require exactly 1.26.5 rather than the broader 1.26 line.

Source: Path instructions

EchoesVault/daily/2026-07-09.md (1)

1-1: 📐 Maintainability & Code Quality | ⚡ Quick win

Promote the session title to an H1.

Line 1 starts with ##, which triggers markdownlint MD041 (first-line-heading) if this file is linted. Switching it to # is a one-line fix and keeps the quality checks clean.

Proposed fix
-## Session — 2026-07-09T09:15:00.000Z
+# Session — 2026-07-09T09:15:00.000Z

Source: Linters/SAST tools

internal/plugins/backends/protocols/codexreasoning/sanitize.go (1)

1-18: LGTM!

internal/plugins/backends/protocols/codexreasoning/sanitize_test.go (1)

1-31: LGTM!

internal/plugins/backends/codexappserver/stream.go (1)

12-12: LGTM!

Also applies to: 115-121

internal/plugins/backends/codexappserver/stream_internal_test.go (1)

350-383: LGTM!

internal/plugins/backends/openaicodex/stream.go (1)

14-14: LGTM!

Also applies to: 56-58, 89-101

internal/plugins/backends/openaicodex/stream_internal_test.go (1)

420-459: LGTM!


📝 Walkthrough

Summary by CodeRabbit

  • New Features
    • Reasoning summaries now appear cleaner by removing stray empty HTML comment markers from generated output.
    • Reasoning-related updates are handled more consistently across supported backends.
  • Bug Fixes
    • Fixed cases where partial or empty comment markers could show up or produce blank reasoning updates.
  • Chores
    • Updated the Go toolchain version used by the project.

Walkthrough

Adds a codexreasoning.StripEmptyHTMLCommentMarkers helper and integrates it into codexappserver and openaicodex stream mappers to strip or suppress empty HTML comment markers in reasoning summary text deltas. Includes new unit tests, a go.mod toolchain version bump, and a daily session log entry.

Changes

Reasoning Delta Sanitization

Layer / File(s) Summary
Shared sanitizer helper
internal/plugins/backends/protocols/codexreasoning/sanitize.go, internal/plugins/backends/protocols/codexreasoning/sanitize_test.go
Adds StripEmptyHTMLCommentMarkers with regex-based removal of empty/partial HTML comment markers, and table-driven tests covering various marker placements.
codexappserver stream mapping
internal/plugins/backends/codexappserver/stream.go, internal/plugins/backends/codexappserver/stream_internal_test.go
Splits item/reasoning/summaryTextDelta handling from item/reasoning/textDelta, applies the sanitizer before emitting EventReasoningDelta, and suppresses empty results; adds tests for stripping and suppression.
openaicodex stream mapping
internal/plugins/backends/openaicodex/stream.go, internal/plugins/backends/openaicodex/stream_internal_test.go
Updates handleReasoningDelta to accept a stripEmptyHTMLComments flag, routes response.reasoning_summary_text.delta through the sanitizer, suppresses empty deltas, and adds corresponding tests.
Supporting changes
go.mod, EchoesVault/daily/2026-07-09.md
Bumps Go toolchain version from 1.26.4 to 1.26.5 and adds a dated session log documenting the review and verification steps for this change.

Estimated code review effort: 2 (Simple) | ~15 minutes

🚥 Pre-merge checks | ✅ 7 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 22.22% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (7 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the main fix and uses a conventional prefix.
Description check ✅ Passed The description matches the code changes and test coverage added in the pull request.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
No Secrets ✅ Passed Changed files only add sanitizer logic/tests and a go.mod bump; scan found no credentials, secrets, private keys, passwords, or sensitive URLs.
Context Propagation ✅ Passed PR only adds pure sanitization; stream paths still pass ctx through Recv/HandleServerRequest, and no new goroutines were introduced.
No Accidental Public Api Break ✅ Passed No public pkg API or exported signature changes; the only new exported symbol is an internal helper, and the rest are internal behavior/tests.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Mateusz and others added 2 commits July 9, 2026 11:23
Co-authored-by: Cursor <cursoragent@cursor.com>
govulncheck failed on crypto/tls in go1.26.4; CI requires the patched standard library.

Co-authored-by: Cursor <cursoragent@cursor.com>
@matdev83 matdev83 merged commit 5a09d8b into main Jul 10, 2026
2 checks passed
@matdev83 matdev83 deleted the bugfix-codex-reasoning-empty-comment-markers branch July 13, 2026 14:09
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