-
Notifications
You must be signed in to change notification settings - Fork 1.1k
refactor(codex): extract encoding, revision, paths, and TOML leaves from prompt-layers (split S10 L1/2) #3590
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
baef8af
f2c9b29
82e069c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,80 @@ | ||
| // --------------------------------------------------------------------------- | ||
| // Character policy — see the header. Defined over Unicode SCALAR VALUES, not | ||
| // UTF-16 code units, because a lone surrogate is not a scalar value and UTF-8 | ||
| // encoding would silently substitute U+FFFD. | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| export interface CharacterFinding { | ||
| /** code-point index, consistent across module, route and editor */ | ||
| position: number; | ||
| reason: "control" | "unpaired-surrogate"; | ||
| codePoint: number; | ||
| } | ||
|
|
||
| /** Tab to four spaces, CRLF and lone CR to LF. Applied BEFORE validation. */ | ||
| export function normalizeBody(body: string): string { | ||
| return body.replace(/\r\n?/g, "\n").replace(/\t/g, " "); | ||
| } | ||
|
|
||
| /** First offending scalar, or null. Run AFTER normalizeBody. */ | ||
| export function findInvalidCharacter(body: string): CharacterFinding | null { | ||
| let position = 0; | ||
| for (let i = 0; i < body.length; ) { | ||
| const code = body.codePointAt(i)!; | ||
| const unit = body.charCodeAt(i); | ||
| const isHighSurrogate = unit >= 0xd800 && unit <= 0xdbff; | ||
| const isLowSurrogate = unit >= 0xdc00 && unit <= 0xdfff; | ||
| // codePointAt only combines a well-formed pair, so a surviving surrogate | ||
| // code point here is unpaired by construction. | ||
| if ((isHighSurrogate || isLowSurrogate) && code === unit) { | ||
| return { position, reason: "unpaired-surrogate", codePoint: code }; | ||
| } | ||
| const isNewline = code === 0x0a; | ||
| const isC0 = code < 0x20 && !isNewline; | ||
| const isDel = code === 0x7f; | ||
| const isC1 = code >= 0x80 && code <= 0x9f; | ||
| if (isC0 || isDel || isC1) { | ||
| return { position, reason: "control", codePoint: code }; | ||
| } | ||
| i += code > 0xffff ? 2 : 1; | ||
| position += 1; | ||
| } | ||
| return null; | ||
| } | ||
|
|
||
| /** | ||
| * TOML basic-string encoding, total over the accepted set: three rules, none of | ||
| * them in the range where `Bun.TOML.parse` misbehaves. `\r` cannot appear | ||
| * because normalizeBody removed it; control characters cannot appear because | ||
| * findInvalidCharacter rejected them. | ||
| */ | ||
| export function encodeBasicString(body: string): string { | ||
| return `"${body.replace(/\\/g, "\\\\").replace(/"/g, '\\"').replace(/\n/g, "\\n")}"`; | ||
| } | ||
|
|
||
| /** | ||
| * Inverse of `encodeBasicString`, deliberately narrow: it accepts ONLY the three | ||
| * escapes we emit. `\t`, `\f`, `\b`, `\r` and `\uXXXX` are refused rather than | ||
| * guessed — decoding them correctly is exactly the ambiguity the restricted set | ||
| * exists to avoid. | ||
| */ | ||
| export function decodeBasicString(literal: string): string | null { | ||
| if (literal.length < 2 || !literal.startsWith('"') || !literal.endsWith('"')) return null; | ||
| const inner = literal.slice(1, -1); | ||
| let out = ""; | ||
| for (let i = 0; i < inner.length; i += 1) { | ||
| const ch = inner[i]!; | ||
| if (ch !== "\\") { | ||
| if (ch === '"') return null; // unescaped quote: not a single literal | ||
| out += ch; | ||
| continue; | ||
| } | ||
| const next = inner[i + 1]; | ||
| if (next === "\\") out += "\\"; | ||
| else if (next === '"') out += '"'; | ||
| else if (next === "n") out += "\n"; | ||
| else return null; // any other escape is outside what we will decode | ||
| i += 1; | ||
| } | ||
| return out; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,54 @@ | ||
| import { realpathSync } from "node:fs"; | ||
| import { join, resolve } from "node:path"; | ||
| import { expandUserPath } from "../../config"; | ||
| import { CODEX_CONFIG_PATH } from "../paths"; | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // Paths | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| export interface Paths { | ||
| configPath?: string; | ||
| storePath?: string; | ||
| baseVariantDir?: string; | ||
| } | ||
|
|
||
| function activeCodexHome(): string { | ||
| const raw = process.env.CODEX_HOME?.trim(); | ||
| if (!raw) return CODEX_CONFIG_PATH.slice(0, -"/config.toml".length); | ||
| const path = resolve(expandUserPath(raw)); | ||
| try { | ||
| return realpathSync.native(path); | ||
| } catch { | ||
| return path; | ||
| } | ||
| } | ||
|
|
||
| export function activeConfigPath(opts?: Paths): string { | ||
| return opts?.configPath ?? join(activeCodexHome(), "config.toml"); | ||
| } | ||
|
|
||
| export function activeStorePath(opts?: Paths): string { | ||
| return opts?.storePath ?? join(activeCodexHome(), "opencodex-prompt.json"); | ||
| } | ||
|
|
||
| /** | ||
| * Where authored base-prompt variants live, one markdown file per variant. | ||
| * | ||
| * A directory of real files rather than another JSON store, because | ||
| * `model_instructions_file` points Codex at a path it reads directly. Embedding the | ||
| * bodies in `opencodex-prompt.json` would mean materialising a temp file at selection | ||
| * time, which is a second write path for no gain. | ||
| */ | ||
| export function activeBaseVariantDir(opts?: Paths): string { | ||
| return opts?.baseVariantDir ?? join(activeCodexHome(), "opencodex-prompt-base"); | ||
| } | ||
|
|
||
|
|
||
| export function journalPathFor(storePath: string): string { | ||
| return `${storePath.replace(/\.json$/, "")}.journal`; | ||
| } | ||
|
|
||
| export function lockPathFor(storePath: string): string { | ||
| return `${storePath.replace(/\.json$/, "")}.lock`; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,55 @@ | ||
| import { existsSync, readFileSync } from "node:fs"; | ||
| import { createHash, type Hash } from "node:crypto"; | ||
|
|
||
| /** | ||
| * Feed one named field into a fingerprint, framed so that no two distinct states | ||
| * can produce the same digest. | ||
| * | ||
| * Framing is the whole point. Concatenating `name + ":" + contents` is ambiguous: | ||
| * an adversarial review of the first version of this function showed that | ||
| * `{override: "left", agents: "right\nAGENTS.md:tail"}` and | ||
| * `{override: "left\nAGENTS.md:right", agents: "tail"}` hashed identically, because | ||
| * a file's own bytes can imitate the separator that follows it. That is exactly a | ||
| * missed invalidation: the fingerprint is the probe's admission key, so two | ||
| * different prompt states sharing a digest means one caller is served the other's | ||
| * stale text. | ||
| * | ||
| * A byte length cannot be forged by content, so each field carries one. Absence is | ||
| * a length of -1 rather than a sentinel string, because a sentinel is just more | ||
| * content: the same review found that `null` collided with a file whose bytes were | ||
| * literally NUL + "absent". | ||
| */ | ||
| export function updateFingerprintField(hash: Hash, name: string, contents: string | null): void { | ||
| const bytes = contents === null ? -1 : Buffer.byteLength(contents, "utf8"); | ||
| hash.update(`\n${name}:${bytes}:`); | ||
| if (contents !== null) hash.update(contents); | ||
| } | ||
|
|
||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // Byte-level hashing. The revision covers COMPLETE file bytes plus existence, | ||
| // so removing the marker while leaving the value intact still changes it. | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| export function readFileOrNull(path: string): string | null { | ||
| try { | ||
| if (!existsSync(path)) return null; | ||
| return readFileSync(path, "utf8"); | ||
| } catch { | ||
| return null; | ||
| } | ||
| } | ||
|
|
||
| export function computeRevision(configBytes: string | null, storeBytes: string | null): string { | ||
| const hash = createHash("sha256"); | ||
| // Length-framed for the reason given on updateFingerprintField: with a bare | ||
| // separator, config bytes ending in "\nstore:" shift the boundary and two | ||
| // different pairs hash alike. That matters twice over — this value is both the | ||
| // probe's admission input and the optimistic-concurrency token compared in | ||
| // commit(), where a collision would let a write built on stale bytes through. | ||
| updateFingerprintField(hash, "cfg", configBytes); | ||
| updateFingerprintField(hash, "store", storeBytes); | ||
| return `sha256:${hash.digest("hex")}`; | ||
| } | ||
|
|
||
| export { readFileOrNull as readFileBytes }; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,163 @@ | ||
| import { OCX_SECTION_MARKER } from "../injected-marker"; | ||
| import { encodeBasicString } from "./encoding"; | ||
| import { TABLE_HEADER, ANY_DEV_INSTRUCTIONS, DEV_INSTRUCTIONS_KEY } from "./toml-read"; | ||
|
|
||
| /** Line editing, not re-serialization: the user's comments and layout survive. */ | ||
| function dominantEol(content: string): "\r\n" | "\n" { | ||
| const crlf = (content.match(/\r\n/g) ?? []).length; | ||
| if (crlf === 0) return "\n"; | ||
| const bareLf = (content.match(/\n/g) ?? []).length - crlf; | ||
| return crlf >= bareLf ? "\r\n" : "\n"; | ||
| } | ||
|
|
||
| function splitLines(content: string): string[] { | ||
| return content.replace(/\r\n/g, "\n").split("\n"); | ||
| } | ||
|
|
||
| /** | ||
| * A leading UTF-8 BOM, split off so line editing never steps over it. | ||
| * | ||
| * Codex reads config.toml with Rust `toml_edit`, which accepts a BOM at byte 0 and | ||
| * nowhere else. Inserting the generated block at line index 0 pushed the BOM down | ||
| * to byte 58, the write reported success because our own byte comparison matched | ||
| * what we intended to write, and the next parse failed with | ||
| * "Expected a key but found (0xEF)" — a config file the user could no longer load, | ||
| * produced by a write that told them it worked. | ||
| * | ||
| * Editors on Windows write this byte routinely, so the file is not exotic. | ||
| */ | ||
| function splitBom(content: string): { bom: string; body: string } { | ||
| return content.startsWith("\ufeff") | ||
| ? { bom: "\ufeff", body: content.slice(1) } | ||
| : { bom: "", body: content }; | ||
| } | ||
|
|
||
| function joinLines(lines: string[], eol: "\r\n" | "\n"): string { | ||
| const text = lines.join("\n"); | ||
| return eol === "\n" ? text : text.replace(/\n/g, "\r\n"); | ||
| } | ||
|
|
||
| function firstTableIndex(lines: string[]): number { | ||
| const idx = lines.findIndex(l => TABLE_HEADER.test(l)); | ||
| return idx === -1 ? lines.length : idx; | ||
| } | ||
|
|
||
| /** Set a root-scope boolean, inserting above the first table when absent. */ | ||
| export function setRootBool(content: string, key: string, value: boolean): string { | ||
| const eol = dominantEol(content); | ||
| const { bom, body } = splitBom(content); | ||
| const lines = splitLines(body); | ||
| const limit = firstTableIndex(lines); | ||
| const escaped = key.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); | ||
| const pattern = new RegExp(`^(\\s*${escaped}\\s*=\\s*)(?:true|false)(\\s*(?:#.*)?)$`); | ||
| for (let i = 0; i < limit; i += 1) { | ||
| const m = pattern.exec(lines[i]!); | ||
| if (m) { | ||
| lines[i] = `${m[1]}${value}${m[2]}`; | ||
| return bom + joinLines(lines, eol); | ||
| } | ||
| } | ||
| lines.splice(limit, 0, `${key} = ${value}`); | ||
| return bom + joinLines(lines, eol); | ||
| } | ||
|
|
||
| /** | ||
| * Set or REMOVE a root-scope basic string. `null` removes the key. | ||
| * | ||
| * Removal is what selecting the default variant does, and it has to be a real deletion | ||
| * rather than an empty string: `model_instructions_file = ""` is a path Codex would try | ||
| * to read, not an absent setting. | ||
| */ | ||
| export function setRootString(content: string, key: string, value: string | null): string { | ||
| const eol = dominantEol(content); | ||
| const { bom, body } = splitBom(content); | ||
| const lines = splitLines(body); | ||
| const limit = firstTableIndex(lines); | ||
| const escaped = key.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); | ||
| const pattern = new RegExp(`^\\s*${escaped}\\s*=\\s*"[^"]*"\\s*(?:#.*)?$`); | ||
| for (let i = 0; i < limit; i += 1) { | ||
| if (!pattern.test(lines[i]!)) continue; | ||
| if (value === null) lines.splice(i, 1); | ||
| else lines[i] = `${key} = ${encodeBasicString(value)}`; | ||
| return bom + joinLines(lines, eol); | ||
| } | ||
| if (value === null) return bom + joinLines(lines, eol); | ||
| lines.splice(limit, 0, `${key} = ${encodeBasicString(value)}`); | ||
| return bom + joinLines(lines, eol); | ||
| } | ||
|
|
||
| /** Set a boolean inside `[table]`, appending the table when absent. */ | ||
| export function setTableBool(content: string, table: string, key: string, value: boolean): string { | ||
| const eol = dominantEol(content); | ||
| const { bom, body } = splitBom(content); | ||
| const lines = splitLines(body); | ||
| const escaped = table.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); | ||
| const start = lines.findIndex(l => new RegExp(`^\\s*\\[${escaped}\\]\\s*(?:#.*)?$`).test(l)); | ||
| if (start === -1) { | ||
| const tail = lines.length > 0 && lines[lines.length - 1] === "" ? lines.length - 1 : lines.length; | ||
| lines.splice(tail, 0, `[${table}]`, `${key} = ${value}`); | ||
| return bom + joinLines(lines, eol); | ||
| } | ||
| const keyEscaped = key.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); | ||
| const pattern = new RegExp(`^(\\s*${keyEscaped}\\s*=\\s*)(?:true|false)(\\s*(?:#.*)?)$`); | ||
| let end = start + 1; | ||
| while (end < lines.length && !TABLE_HEADER.test(lines[end]!)) end += 1; | ||
| for (let i = start + 1; i < end; i += 1) { | ||
| const m = pattern.exec(lines[i]!); | ||
| if (m) { | ||
| lines[i] = `${m[1]}${value}${m[2]}`; | ||
| return bom + joinLines(lines, eol); | ||
| } | ||
| } | ||
| lines.splice(end, 0, `${key} = ${value}`); | ||
| return bom + joinLines(lines, eol); | ||
| } | ||
|
|
||
| /** | ||
| * Replace, insert, or remove the generated two-line block. Canonical form is | ||
| * marker + assignment at the top of the document; replacement is "find the | ||
| * marker, replace the next line" rather than a span search. | ||
| */ | ||
| export function setProjection(content: string | null, projection: string | null): string { | ||
| const base = content ?? ""; | ||
| const eol = dominantEol(base); | ||
| // The BOM is held aside for the whole edit. This is the function that produced | ||
| // the corruption: the insert below is at index 0, which put the marker line | ||
| // ahead of a byte that is only legal at byte 0. | ||
| const { bom, body } = splitBom(base); | ||
| const lines = splitLines(body); | ||
| const limit = firstTableIndex(lines); | ||
|
|
||
| let markerAt = -1; | ||
| for (let i = 0; i < limit; i += 1) { | ||
| if (i > 0 && lines[i - 1]!.includes(OCX_SECTION_MARKER) && ANY_DEV_INSTRUCTIONS.test(lines[i]!)) { | ||
| markerAt = i - 1; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| if (markerAt !== -1) { | ||
| if (projection === null) lines.splice(markerAt, 2); | ||
| else lines[markerAt + 1] = `${DEV_INSTRUCTIONS_KEY} = ${encodeBasicString(projection)}`; | ||
| return bom + joinLines(lines, eol); | ||
| } | ||
|
|
||
| if (projection === null) return bom + joinLines(lines, eol); | ||
| lines.splice(0, 0, OCX_SECTION_MARKER, `${DEV_INSTRUCTIONS_KEY} = ${encodeBasicString(projection)}`); | ||
| return bom + joinLines(lines, eol); | ||
| } | ||
|
|
||
|
|
||
| /** Remove an unowned or reshaped `developer_instructions` from the root scope. */ | ||
| export function removeUnownedProjection(content: string): string { | ||
| const eol = dominantEol(content); | ||
| const lines = splitLines(content); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win Preserve the BOM while removing a projection.
Split the BOM with 🤖 Prompt for AI Agents |
||
| const limit = firstTableIndex(lines); | ||
| for (let i = 0; i < limit; i += 1) { | ||
| if (!ANY_DEV_INSTRUCTIONS.test(lines[i]!)) continue; | ||
| const marked = i > 0 && lines[i - 1]!.includes(OCX_SECTION_MARKER); | ||
| lines.splice(marked ? i - 1 : i, marked ? 2 : 1); | ||
| return joinLines(lines, eol); | ||
| } | ||
| return joinLines(lines, eol); | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Match escaped TOML basic strings before inserting.
This pattern does not match the
\"sequence thatencodeBasicStringwrites at Lines 81 and 85. A secondsetRootStringcall for a value containing"appends a duplicate key. A later TOML parse then fails. A removal call also leaves the old key in place.Match escaped characters in the string body, for example with
"(?:[^"\\\\\\r\\n]|\\\\.)*", before replacing or deleting the line.🤖 Prompt for AI Agents