Repository navigation
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
github-actions
Bot
changed the title
📝 WalkthroughWalkthroughSideband frame diagnostics now record metadata without payload excerpts. They track U+FFFD presence across text and binary payloads. Diagnostic files use owner-only permissions, with tests covering log content and non-Windows permissions. ChangesSideband diagnostic logging
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 2⚔️ Resolve merge conflicts 💡
🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/server/live.ts`:
- Line 116: Update logLiveSidebandFrame in src/server/live.ts to open the frame
log descriptor, enforce mode 0o600 with fchmodSync, append the record through
that descriptor, and close it reliably. In tests/server-live.test.ts:850, retain
the creation-path permission assertion and add a reuse-path case that
pre-creates frameLogPath with mode 0o644, invokes the logger, and verifies it
becomes 0o600.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 18ff4741-3c9f-4f44-bbf0-da4c85da76fc
📒 Files selected for processing (2)
src/server/live.tstests/server-live.test.ts
| fffd, | ||
| }; | ||
| appendFileSync(logPath, `${JSON.stringify(record)}\n`); | ||
| appendFileSync(logPath, `${JSON.stringify(record)}\n`, { mode: 0o600 }); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate files ---'
git ls-files 'src/server/live.ts' 'tests/server-live.test.ts' 'package.json' 'bun.lock' 'bun.lockb' 'tsconfig.json'
printf '%s\n' '--- source outline ---'
ast-grep outline src/server/live.ts || true
printf '%s\n' '--- source context ---'
sed -n '1,180p' src/server/live.ts
printf '%s\n' '--- test context ---'
sed -n '810,875p' tests/server-live.test.ts
printf '%s\n' '--- filesystem permission references ---'
rg -n -S 'appendFileSync|fchmodSync|chmodSync|frameLogPath|logPath|forensic|frame log' src tests package.json bun.lock bun.lockb 2>/dev/null || true
printf '%s\n' '--- runtime/config context ---'
for f in package.json bun.lock bun.lockb tsconfig.json; do
if [ -f "$f" ]; then
echo "### $f"
sed -n '1,180p' "$f"
fi
doneRepository: luvs01/opencodex
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- enclosing frame-log test ---'
rg -n -B 45 -A 12 'frameLogPath|OCX_LIVE_FRAME_LOG|frame-log' tests/server-live.test.ts
printf '%s\n' '--- package scripts and runtime declarations ---'
node - <<'JS'
const fs = require("fs");
const pkg = JSON.parse(fs.readFileSync("package.json", "utf8"));
console.log(JSON.stringify({
engines: pkg.engines,
type: pkg.type,
scripts: pkg.scripts,
dependencies: pkg.dependencies,
devDependencies: pkg.devDependencies,
}, null, 2));
JS
printf '%s\n' '--- focused permission patterns ---'
rg -n -S -C 3 'chmodSync\(path, 0o600\)|appendFileSync\(path|openSync\(.*0o600|fchmodSync' src/usage src/server tests/server-live.test.ts
printf '%s\n' '--- standalone existing-file mode probe ---'
node - <<'JS'
"use strict";
const {
appendFileSync,
chmodSync,
mkdtempSync,
readFileSync,
rmSync,
statSync,
} = require("node:fs");
const { join } = require("node:path");
const { tmpdir } = require("node:os");
const dir = mkdtempSync(join(tmpdir(), "ocx-mode-probe-"));
const path = join(dir, "frame.jsonl");
try {
appendFileSync(path, "seed\n", { mode: 0o644 });
chmodSync(path, 0o644);
const before = statSync(path).mode & 0o777;
appendFileSync(path, "record\n", { mode: 0o600 });
const after = statSync(path).mode & 0o777;
console.log(JSON.stringify({ before: before.toString(8), after: after.toString(8), content: readFileSync(path, "utf8") }));
} finally {
rmSync(dir, { recursive: true, force: true });
}
JSRepository: luvs01/opencodex
Length of output: 10915
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- Bun availability ---'
if command -v bun >/dev/null 2>&1; then
bun --version
printf '%s\n' '--- Bun node:fs descriptor probe ---'
bun - <<'JS'
import { chmodSync, closeSync, fchmodSync, mkdtempSync, openSync, readFileSync, rmSync, statSync, writeSync } from "node:fs";
import { join } from "node:path";
import { tmpdir } from "node:os";
const dir = mkdtempSync(join(tmpdir(), "ocx-bun-mode-probe-"));
const path = join(dir, "frame.jsonl");
try {
writeSync(openSync(path, "w", 0o644), "seed\n");
chmodSync(path, 0o644);
const before = statSync(path).mode & 0o777;
const fd = openSync(path, "a", 0o600);
fchmodSync(fd, 0o600);
writeSync(fd, "record\n");
closeSync(fd);
const after = statSync(path).mode & 0o777;
console.log(JSON.stringify({ before: before.toString(8), after: after.toString(8), content: readFileSync(path, "utf8") }));
} finally {
rmSync(dir, { recursive: true, force: true });
}
JS
else
echo 'bun unavailable'
fi
printf '%s\n' '--- test imports and TEST_DIR lifecycle ---'
sed -n '1,90p' tests/server-live.test.ts
rg -n -B 8 -A 12 'TEST_DIR|rmSync\(TEST_DIR|mkdirSync\(TEST_DIR|mkdtempSync' tests/server-live.test.tsRepository: luvs01/opencodex
Length of output: 6885
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- Bun availability ---'
if command -v bun >/dev/null 2>&1; then
bun --version
printf '%s\n' '--- Bun node:fs descriptor probe ---'
bun - <<'JS'
import { chmodSync, closeSync, fchmodSync, mkdtempSync, openSync, readFileSync, rmSync, statSync, writeSync } from "node:fs";
import { join } from "node:path";
import { tmpdir } from "node:os";
const dir = mkdtempSync(join(tmpdir(), "ocx-bun-mode-probe-"));
const path = join(dir, "frame.jsonl");
try {
const seedFd = openSync(path, "w", 0o644);
writeSync(seedFd, "seed\n");
closeSync(seedFd);
chmodSync(path, 0o644);
const before = statSync(path).mode & 0o777;
const fd = openSync(path, "a", 0o600);
fchmodSync(fd, 0o600);
writeSync(fd, "record\n");
closeSync(fd);
const after = statSync(path).mode & 0o777;
console.log(JSON.stringify({ before: before.toString(8), after: after.toString(8), content: readFileSync(path, "utf8") }));
} finally {
rmSync(dir, { recursive: true, force: true });
}
JS
else
echo 'bun unavailable'
fi
printf '%s\n' '--- test imports and TEST_DIR lifecycle ---'
sed -n '1,90p' tests/server-live.test.ts
rg -n -B 8 -A 12 'TEST_DIR|rmSync\(TEST_DIR|mkdirSync\(TEST_DIR|mkdtempSync' tests/server-live.test.tsRepository: luvs01/opencodex
Length of output: 6885
Harden reused frame log files before appending.
{ mode: 0o600 } affects only file creation. If OCX_LIVE_FRAME_LOG points to an existing 0o644 file, logLiveSidebandFrame keeps its permissive mode. On POSIX, open the file, call fchmodSync(fd, 0o600), append the record, and close the descriptor.
In tests/server-live.test.ts, retain the creation-path assertion and add a reuse-path case that pre-creates frameLogPath with 0o644, invokes the logger, and asserts 0o600.
📍 Affects 2 files
src/server/live.ts#L116-L116(this comment)tests/server-live.test.ts#L850-L850
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/server/live.ts` at line 116, Update logLiveSidebandFrame in
src/server/live.ts to open the frame log descriptor, enforce mode 0o600 with
fchmodSync, append the record through that descriptor, and close it reliably. In
tests/server-live.test.ts:850, retain the creation-path permission assertion and
add a reuse-path case that pre-creates frameLogPath with mode 0o644, invokes the
logger, and verifies it becomes 0o600.
|
✅ Deterministic PR hygiene checks passed. |
✅ READY
Hygiene✅ Deterministic PR hygiene checks passed. |
luvs01
pushed a commit
that referenced
this pull request
Aug 28, 2026
Two independent reviewers found the same defect: the fix was half-inert. apiError gained a status parameter and apiJson gained transportError, and no production caller passed or read either, so the 404 to 4 and 409 to 5 mapping never ran and the retained transport cause was never printed. The behavior existed only in this phase's own tests while the commit message claimed it shipped. Status is now threaded through all 19 apiError call sites and the cause through the 14 status === 0 guards; two new tests assert the call sites rather than the helpers, so it cannot go inert again. tray had the identical lidge-jun#2697 defect: windowsTrayCommand reports failure through process.exitCode and returns void, so ocx tray install printed an error and exited 0. The recurrence guard could not see it because the pattern was anchored on 'await handle\w+(', scoping it to a naming convention rather than to the defect class. Broadened to 'await [\w.]+(', which surfaced four more candidates; each was verified in its handler before being allowlisted with its own reason. Also corrects the login allowlist reason (it throws rather than exits 1), passes the env seam through to configuredAdminToken so doctor's check no longer reads real machine state for half its comparison, and extends assertNotAdminToken with the equality arm that catches an operator-set admin token without the ocx_admin_ prefix. The #180 matrix test asserting a 404 exits 1 now expects 4. That issue is a feature request, not an exit-code contract; the test described then-current behavior, and the change is the one 010.3 specified and the PR disclosed.
luvs01
changed the title
Owner
Author
|
Ported upstream; closing. |
devin-ai-integration Bot
added a commit
that referenced
this pull request
Sep 26, 2026
cli-account.test.ts sits exactly at its file-size ratchet cap (2313); the new test pushed it to 2336 and failed 'file-size ratchet: repository'. cli-account-alias-target.test.ts is the sibling kept out of the #180 matrix file for this reason, so the case moves there byte-for-byte semantics, ported to that file's harness. Co-Authored-By: Devin AI <devin-ai-integration[bot]@users.noreply.github.com>
github-actions
Bot
deleted the
codex/propose-fix-for-live-frame-logging-vulnerability
branch
October 4, 2026 12:47
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
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Description
contexttext from frames and record only metadata fields:ts,dir,kind,bytes, and a booleanfffdflag when U+FFFD is present in decoded text, implemented inlogLiveSidebandFrameinsrc/server/live.ts.String.prototype.includesover the decoder result and do not persist surrounding text.0o600) by passing{ mode: 0o600 }toappendFileSyncso new files are private on POSIX systems.tests/server-live.test.tsto assert that nocontextproperty or payload text is present in the log and to check POSIX file mode is0600when applicable.Testing
bun run typecheck, which succeeded (no type errors).bun run test -- tests/server-live.test.ts --test-name-pattern "sideband frame log records metadata without payload content", which passed and verifies metadata-only logging and file permissions.bun run privacy:scan, which passed.bun run testwas started under the pinned Bun runtime; it exercised many suites but an unrelated existing test (tests/server-auth.test.ts) failed (expected 401 vs received 403) during the full run and that unrelated failure was observed while the overall long suite was running.Codex Task
Summary by CodeRabbit