Skip to content

perf(codex): validate audit UTF-8 without round-trip Buffer copies - #7672

Merged
lidge-jun merged 10 commits into
lidge-jun:devfrom
luvs01:perf/bun144-native-audit-utf8
Oct 25, 2026
Merged

perf(codex): validate audit UTF-8 without round-trip Buffer copies#7672
lidge-jun merged 10 commits into
lidge-jun:devfrom
luvs01:perf/bun144-native-audit-utf8

Conversation

@luvs01

@luvs01 luvs01 commented Oct 24, 2026 •
edited by coderabbitai Bot
Loading

Copy link
Copy Markdown
Collaborator

Summary

Validate catalog-audit lines with a fatal UTF-8 TextDecoder instead of decoding and allocating another Buffer solely for a byte round trip. Preserve BOM characters with ignoreBOM: true so the existing JSON parser continues to reject BOM-prefixed records.

Keep valid record bytes, CRLF handling, malformed/unterminated-tail classification, owner compaction and foreign-append refusal unchanged. Extend the existing audit regressions for invalid encodings, BOMs, valid replacement characters, escaped surrogates and non-mutating refusal.

Verification

  • HEAD 7834a093157102f78ea4c28c7f354ee009b8f0fb preserves the published change and merges dev@0aeaf0cb plus the shared chore(test): align Bun runner with the 1.4.4 security baseline #7627 setup/CI change. Runtime and test runner both use Bun 1.4.4. The three change-owned files are preserved.
  • Official Bun 1.4.4, native Windows, CI=true: bun test --isolate --timeout 60000 tests/codex-integration/codex-catalog-write-audit.test.ts — 39 passed / 0 failed / 195 assertions.
  • Root TypeScript, Structure, generated skill surface, diff, and scoped privacy/file-size checks passed. Dependency inputs and the existing frozen installation matched; no reinstall was needed. Every validation process returned exit 0 and EOF, and owned fixtures/processes were cleaned up.
  • Independent source review found no integration regression. The 39 local passing tests include two Windows branches that return skipped; those Windows branches do not execute successful POSIX owner compaction. Native macOS compaction, the complete native platform matrix and performance measurements remain unverified.
  • Current-head PR CI completed SUCCESS on 7834a093157102f78ea4c28c7f354ee009b8f0fb: 17 successful jobs / 6 conditionally skipped jobs, including aggregate ci. Skipped platform jobs are not native-platform execution evidence.
  • The successful Linux test 4/4 job explicitly passed both new owner-compaction cases (complete UTF-8 byte preservation and malformed UTF-8/BOM rejection) and the foreign-append non-mutation case. This supplies POSIX coverage beyond the local Windows refusal branches.
  • The current-head PR target, ancestry and description gate also completed SUCCESS.

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed.
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults.

Co-authored-by: luvs01 27862058+luvs01@users.noreply.github.com

Summary by CodeRabbit

  • Bug Fixes
    • Audit records with invalid UTF-8, a leading byte-order mark, or an incomplete final line are now handled safely. Malformed records are excluded during compaction, and foreign updates leave an unclean audit file unchanged.
  • Documentation
    • Updated contributor guidance for the Bun test runner and clarified that promoting a candidate requires a successful full cross-platform run on that exact candidate.

luvs01 and others added 7 commits October 23, 2026 21:25
@coderabbitai

coderabbitai Bot commented Oct 24, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Important

Review skipped

Review was skipped as selected files did not have any reviewable changes.

Run configuration
Commits

Reviewing files that changed from the base of the PR and between 7834a09 and 5bd692f.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

No actionable comments were generated in the recent review.

Recent review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between 0aeaf0c and 7834a09.

Files selected for processing: 14
  • .github/actions/setup-project-bun/action.yml
  • .github/workflows/ci.yml
  • CONTRIBUTING.md
  • Dockerfile
  • docs-site/src/content/docs/contributing.md
  • package.json
  • src/codex/catalog/write-audit.ts
  • structure/codex-home.md
  • structure/runtime.md
  • tests/ci-workflows/ci-scope-gaps.test.ts
  • tests/ci-workflows/install-scripts.test.ts
  • tests/claude-integration/claude-picker-upload.test.ts
  • tests/codex-integration/codex-catalog-write-audit.test.ts
  • tests/test-layout-tooling.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 6 remain after this review.

Walkthrough

The test-runner Bun pin changes from 1.4.3 to 1.4.4. Audit records now use fatal UTF-8 decoding before JSON parsing. An upload test fixture now records whether an early TLS response finishes before the request body.

Changes: Bun Runner Version Contract

Layer / File(s) Summary
Bun defaults and role contract
package.json, .github/actions/setup-project-bun/action.yml, .github/workflows/ci.yml, CONTRIBUTING.md, Dockerfile, docs-site/src/content/docs/contributing.md, structure/runtime.md		 The test-runner pin changes to 1.4.4, matching the shipped runtime. The related text retains separate runtime and test-runner roles and specifies a successful full cross-platform lane=all run on the exact candidate head for promotion.
Runner resolution tests
tests/ci-workflows/install-scripts.test.ts, tests/ci-workflows/ci-scope-gaps.test.ts, tests/test-layout-tooling.test.ts		 Tests expect the 1.4.4 pin and cover runner resolution when caller and pinned Bun versions match or differ, plus rejection of an incorrect override.

Catalog Audit Record Decoding

Layer / File(s) Summary
Decode and validate audit records
src/codex/catalog/write-audit.ts, structure/codex-home.md, tests/codex-integration/codex-catalog-write-audit.test.ts		 Audit rows now use fatal UTF-8 decoding before JSON parsing. Tests cover compaction of complete records, rejection of malformed UTF-8 and BOM-prefixed rows, and foreign-owner refusal without file changes.

Buffered Upload Early-Response Test

Layer / File(s) Summary
Exercise early TLS responses
tests/claude-integration/claude-picker-upload.test.ts		 The test fixture can send a 413 response before receiving the full request body. The buffered early-response test checks that the response flushed while the body was incomplete.

Priority: Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Refactor

Possibly related PRs

  • lidge-jun/opencodex#7627: Both PRs align the test-runner Bun pin with the shipped 1.4.4 runtime and retain independent runner-role validation.
  • lidge-jun/opencodex#7606: That PR changes the shipped runtime pin to 1.4.4; this PR updates the test-runner pin to match it.
Merge Risk: Minimal · up to 7834a

The audit regression test covers the claimed failure, and no remaining issue is established that should delay merging.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 6 files. (8 skipped: 8… Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check Passed The title clearly and concisely describes the main implementation change in src/codex/catalog/write-audit.ts: validating audit UTF-8 without round-trip Buffer allocations. It matches the primary PR ob…
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.
Full details: Title check

Explanation

The title clearly and concisely describes the main implementation change in src/codex/catalog/write-audit.ts: validating audit UTF-8 without round-trip Buffer allocations. It matches the primary PR objective.

Full details: Docstring Coverage

Explanation

Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 6 files. (8 skipped: 8 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

@github-actions

github-actions Bot commented Oct 24, 2026

Copy link
Copy Markdown
Contributor

✅ Deterministic PR hygiene checks passed.

@github-actions github-actions Bot added the enhancement New feature or request label Oct 24, 2026
@luvs01
luvs01 marked this pull request as ready for review October 24, 2026 23:04
@lidge-jun
Merge origin/dev (lidge-jun#7627 landed as 0929e6e) into perf/bun144-…
5bd692f 
…native-audit-utf8

# Conflicts:
#	tests/claude-integration/claude-picker-upload.test.ts
@lidge-jun

lidge-jun commented Oct 25, 2026

Copy link
Copy Markdown
Owner

Integration evidence (owner decision decision:e6895606, approved): independent gpt-6.1-sol review PASS at ebeebdf; the current head only adds merges of origin/dev (the author's #7627 merge, then the maintainer's dev merge after #7627 landed), so this PR's diff is again src/codex/catalog/write-audit.ts, its test and structure/codex-home.md. Exact-head PR CI run 40150118941 and exact-head ci.yml lane=all run 40150182583 (maintainer mirror branch at this SHA) both succeeded.

Static merge simulation on current dev 5d995c9 (this Mac): dev=5d995c956dd970be3cb9fc1cad9cb78300060ba0 base=c2bcb529fa5762833aa32ef8ebd76c5b41b733f3 pr_files= 3 overlap=[]; merge clean; install exit=0; gui install exit=0; tsc exit=0; tsc doctor exit=0; structure:check exit=0; privacy:scan exit=0; layout+ratchet guards exit=0 54 pass 0 fail ;

Merged by the triage manager as maintainer integration (MAINTAINERS.md, dev only) with green exact-head PR CI, an independent review PASS, and the owner's approval of this bundle.

@lidge-jun
lidge-jun merged commit 5b71daf into lidge-jun:dev Oct 25, 2026
63 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lidge-jun lidge-jun Awaiting requested review from lidge-jun lidge-jun is a code owner
@Ingwannu Ingwannu Awaiting requested review from Ingwannu Ingwannu is a code owner

Assignees

No one assigned

Labels

enhancement New feature or request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@luvs01 @lidge-jun