fix(codex): validate a managed fragment standalone before collision filtering

review50-r2 held ebfe60d9 NOT-CLEAR. The parser-backed collision predicate asks
whether the user's document still parses with a managed table appended, and
treats failure as a collision. That failure has two causes — the user owns a
conflicting path, or the block is malformed on its own — and the predicate could
not tell them apart.

So an intrinsically invalid fragment was classified as colliding and DELETED.
The render guard then passed precisely because the bad input was gone, the user's
file was modified, and the receipt reported the resource projected:

    fragment:  [mcp_servers.exa]\nurl =
    result:    projected ["codex-default-config"], failed []
    written:   an empty managed block

An authoring failure was indistinguishable from a successful projection with no
managed content, which is the opposite of what a fail-before-write guard is for.

A managed fragment is now required to be a valid TOML document on its own,
checked before anything is dropped. An invalid one throws: zero projected, one
failed entry naming the TOML error with line and column, and the existing config
left byte-unchanged.

Correction to my previous handoff: I told the reviewer that two fragment blocks
colliding with EACH OTHER was a real gap in the new mechanism. That was wrong.
The render guard already caught it, verified by the new pin passing against the
held candidate as well as this one. It is kept as a guard test and named as such,
not as a repair.
This commit is contained in:
v-openrig-build committed 2026-09-01 09:10:05 +00:00
1 parent ebfe60d90c
commit a760ec2737
2 files changed
+63 -6

No files matched your search

@@ -607,6 +607,9 @@ export class CodexRuntimeAdapter implements RuntimeAdapter {
const existing = this.fs.exists(configPath) ? this.fs.readFile(configPath) : "";
const fragment = this.fs.readFile(entry.absolutePath);
// Before anything is dropped: a fragment that is invalid on its own must
// fail loudly, never be "resolved" by the collision filter deleting it.
assertFragmentParsesStandalone(fragment, entry.absolutePath, entry.effectiveId);
const rendered = upsertManagedCodexConfigFragment(existing, entry.effectiveId, fragment);
assertRendersAsLoadableToml(rendered, configPath, entry.effectiveId);
this.fs.writeFile(configPath, rendered);
@@ -1248,6 +1251,24 @@ function parsesAsToml(candidate: string): boolean {
try { parseToml(candidate); return true; } catch { return false; }
}
/**
* A managed fragment must be a valid TOML document on its own, checked BEFORE
* any collision filtering. Without this, an authoring error in the fragment is
* indistinguishable from a user collision and gets silently dropped — the
* write then succeeds precisely because the bad input was deleted, which is the
* opposite of what the render guard is for.
*/
function assertFragmentParsesStandalone(fragment: string, sourcePath: string, id: string): void {
try {
parseToml(fragment);
} catch (err) {
throw new Error(
`Codex config fragment '${id}' is not valid TOML on its own (${sourcePath}); ` +
`nothing was projected and the existing config was left unchanged. ${(err as Error).message}`,
);
}
}
/**
* Drop the fragment tables that would collide with the user's own.
*
@@ -1267,12 +1288,19 @@ function parsesAsToml(candidate: string): boolean {
* The user's values are never merged, rewritten or overwritten — a colliding
* managed table simply stands down.
*
* Two remaining limits, both erring toward a config that loads:
* - If the user's existing file does not parse at all, no collision can be
* discriminated, so nothing is dropped and the render check below refuses the
* write. We do not silently "fix" a file we cannot read.
* - Keys before the fragment's first header are not tables and pass through; a
* duplicate one is caught by the render check rather than guessing whose wins.
* CALLERS MUST VALIDATE THE FRAGMENT STANDALONE FIRST. "Appending this block
* makes the document unparseable" has two causes — the user owns a conflicting
* path, or the block is malformed on its own — and this predicate cannot tell
* them apart. Left unguarded it answered both with "collides" and DELETED an
* invalid authored fragment, turning a resource error into a clean-looking empty
* managed block while the receipt said projected (review50-r2, 2026-09-01,
* reproduced). `assertFragmentParsesStandalone` eliminates the second cause
* before we get here, so a failure that survives to this point is a real
* collision.
*
* One remaining limit: keys before the fragment's first header are not tables
* and pass through; a duplicate one is caught by the render check rather than
* guessing whose key wins.
*/
function dropCollidingFragmentTables(
fragment: string,
@@ -1395,6 +1395,35 @@ describe("Codex runtime adapter", () => {
expect(after.profiles.notes.text).toBe((parseToml(userConfig) as Record<string, any>).profiles.notes.text);
});
it("refuses an intrinsically invalid managed fragment instead of deleting it (r2 NOT-CLEAR, 09-01)", async () => {
// review50-r2 blocking finding on candidate ebfe60d9. "appending this block
// breaks the parse" has TWO causes — a user collision, or a malformed block.
// Conflating them let the collision filter DELETE an invalid authored
// fragment, after which the render guard passed precisely because the bad
// input was gone, and the receipt said projected.
const original = 'model = "gpt-5"\n';
const { project, read } = await projectFragment(original, "[mcp_servers.exa]\nurl =\n");
const result = await project();
expect(result.projected).toEqual([]); // zero projected
expect(result.failed).toHaveLength(1); // one failed projection
expect(result.failed[0]!.error).toMatch(/not valid TOML on its own/);
expect(read()).toBe(original); // user file byte-unchanged
});
it("refuses a fragment whose own tables collide with each other", async () => {
// Closed by the same standalone check: the per-block collision test compares
// each block against the USER only, so it could never have caught this.
const original = 'model = "gpt-5"\n';
const dup = "[mcp_servers.exa]\nurl = \"a\"\n\n[mcp_servers.exa]\nurl = \"b\"\n";
const { project, read } = await projectFragment(original, dup);
const result = await project();
expect(result.projected).toEqual([]);
expect(result.failed).toHaveLength(1);
expect(read()).toBe(original);
});
it("leaves the fragment intact when the user's config does not parse, and refuses the write", async () => {
// No collision can be discriminated against a file we cannot read, so
// nothing is dropped and the render guard refuses rather than "fixing" it.