mirror of
https://github.com/coder/registry.git
synced 2026-06-02 20:48:14 +00:00
fix(coder-labs/modules/codex): address deep-review findings
Script fixes: - Rename write_minimal_default_config to build_minimal_default_config (no longer writes to disk, emits JSON to stdout). - Guard corrupted existing config: if dasel cannot parse the existing TOML, error out and exit instead of silently proceeding. - Atomic config write: write to a temp file and mv, preventing data loss if the process is interrupted mid-write. - Add jq availability check before populate_config_toml, consistent with how other registry modules handle hard dependencies. - Normalize blank lines between function definitions. Test fixes: - idempotent-mcp-deep-merge: use sed address range to only replace the github server command, assert filesystem command is still npx. - workdir-trusted-project: tighten regex to require bracket syntax instead of matching any line containing the path. - Rename idempotent-run-twice-no-change to idempotent-stable-after-roundtrip (test runs 3 times, not 2). - Remove unnecessary regex escaping of forward slashes. - Strengthen combination test assertions to check values, not just key presence.
This commit is contained in:
@@ -346,9 +346,7 @@ describe("codex", async () => {
|
||||
id,
|
||||
"/home/coder/.codex/config.toml",
|
||||
);
|
||||
expect(configToml).toMatch(
|
||||
new RegExp(`projects.*${workdir.replace(/\//g, "\\/")}.*`),
|
||||
);
|
||||
expect(configToml).toMatch(new RegExp(`\\[projects\\..*${workdir}.*\\]`));
|
||||
expect(configToml).toMatch(/trust_level\s*=\s*['"]trusted['"]/);
|
||||
});
|
||||
|
||||
@@ -473,14 +471,14 @@ EOF`,
|
||||
});
|
||||
await runScripts(id, scripts);
|
||||
|
||||
// User customizes the github MCP server between restarts
|
||||
// User customizes ONLY the github MCP server between restarts
|
||||
await execContainer(id, [
|
||||
"bash",
|
||||
"-c",
|
||||
[
|
||||
"CONFIG=/home/coder/.codex/config.toml",
|
||||
// Replace the github command the user has customized
|
||||
"sed -i \"s/command = .npx./command = 'my-custom-npx'/\" $CONFIG",
|
||||
// Use sed address range to only replace command under github section
|
||||
"sed -i '/github/,/^$/{ s/command = .*/command = '\"'\"'my-custom-npx'\"'\"'/; }' $CONFIG",
|
||||
].join(" && "),
|
||||
]);
|
||||
|
||||
@@ -492,9 +490,9 @@ EOF`,
|
||||
);
|
||||
// User's customized github command preserved
|
||||
expect(config).toMatch(/command\s*=\s*['"]my-custom-npx['"]/);
|
||||
// filesystem server still present (not lost by shallow merge)
|
||||
expect(config).toContain("mcp_servers");
|
||||
// filesystem server still present with original command
|
||||
expect(config).toContain("filesystem");
|
||||
expect(config).toMatch(/command\s*=\s*['"]npx['"]/);
|
||||
});
|
||||
|
||||
test("idempotent-base-config-preserves-user-edits", async () => {
|
||||
@@ -526,7 +524,7 @@ EOF`,
|
||||
expect(config).toContain("preferred_auth_method");
|
||||
});
|
||||
|
||||
test("idempotent-run-twice-no-change", async () => {
|
||||
test("idempotent-stable-after-roundtrip", async () => {
|
||||
const { id, scripts } = await setup();
|
||||
|
||||
// First run
|
||||
@@ -637,11 +635,11 @@ EOF`,
|
||||
"/home/coder/.codex/config.toml",
|
||||
);
|
||||
// Base config keys present
|
||||
expect(config).toContain("sandbox_mode");
|
||||
expect(config).toContain("preferred_auth_method");
|
||||
expect(config).toMatch(/sandbox_mode\s*=\s*['"]danger-full-access['"]/);
|
||||
expect(config).toMatch(/preferred_auth_method\s*=\s*['"]apikey['"]/);
|
||||
// MCP server present
|
||||
expect(config).toContain("mcp_servers");
|
||||
expect(config).toContain("github");
|
||||
expect(config).toMatch(/command\s*=\s*['"]npx['"]/);
|
||||
});
|
||||
|
||||
test("all-config-sources-combined", async () => {
|
||||
@@ -668,12 +666,12 @@ EOF`,
|
||||
"/home/coder/.codex/config.toml",
|
||||
);
|
||||
// Base config
|
||||
expect(config).toContain("sandbox_mode");
|
||||
expect(config).toContain("preferred_auth_method");
|
||||
expect(config).toMatch(/sandbox_mode\s*=\s*['"]danger-full-access['"]/);
|
||||
expect(config).toMatch(/preferred_auth_method\s*=\s*['"]apikey['"]/);
|
||||
// MCP
|
||||
expect(config).toContain("github");
|
||||
expect(config).toMatch(/command\s*=\s*['"]npx['"]/);
|
||||
// AI gateway
|
||||
expect(config).toContain("model_providers");
|
||||
expect(config).toContain("[model_providers.aigateway]");
|
||||
});
|
||||
|
||||
test("idempotent-all-sources-user-edits-survive", async () => {
|
||||
@@ -717,11 +715,11 @@ EOF`,
|
||||
);
|
||||
// User edits survived
|
||||
expect(config).toMatch(/preferred_auth_method\s*=\s*['"]oauth['"]/);
|
||||
expect(config).toContain("user_note");
|
||||
expect(config).toMatch(/user_note\s*=\s*['"]do not touch['"]/);
|
||||
// Module config still present
|
||||
expect(config).toContain("sandbox_mode");
|
||||
expect(config).toMatch(/sandbox_mode\s*=\s*['"]danger-full-access['"]/);
|
||||
expect(config).toContain("github");
|
||||
expect(config).toContain("model_providers");
|
||||
expect(config).toContain("[model_providers.aigateway]");
|
||||
});
|
||||
|
||||
test("custom-config-drops-reasoning-effort", async () => {
|
||||
|
||||
@@ -26,7 +26,6 @@ printf "install_codex: %s\n" "$${ARG_INSTALL}"
|
||||
printf "model_reasoning_effort: %s\n" "$${ARG_MODEL_REASONING_EFFORT}"
|
||||
echo "--------------------------------"
|
||||
|
||||
|
||||
function install_dasel() {
|
||||
if command_exists dasel; then
|
||||
return 0
|
||||
@@ -59,7 +58,6 @@ function install_dasel() {
|
||||
fi
|
||||
}
|
||||
|
||||
|
||||
function add_path_to_shell_profiles() {
|
||||
local path_dir="$1"
|
||||
|
||||
@@ -146,7 +144,7 @@ function install_codex() {
|
||||
}
|
||||
|
||||
# Builds the minimal default config as a JSON string on stdout.
|
||||
function write_minimal_default_config() {
|
||||
function build_minimal_default_config() {
|
||||
local json
|
||||
json=$(jq -n '{preferred_auth_method: "apikey"}')
|
||||
|
||||
@@ -177,7 +175,7 @@ function populate_config_toml() {
|
||||
new_json=$(echo "$${ARG_BASE_CONFIG_TOML}" | dasel -i toml -o json)
|
||||
else
|
||||
printf "Using minimal default configuration\n"
|
||||
new_json=$(write_minimal_default_config)
|
||||
new_json=$(build_minimal_default_config)
|
||||
fi
|
||||
|
||||
if [ -n "$${ARG_MCP}" ]; then
|
||||
@@ -197,13 +195,19 @@ function populate_config_toml() {
|
||||
# Deep-merge with existing config (existing user values win).
|
||||
if [ -s "$${config_path}" ]; then
|
||||
local existing_json
|
||||
existing_json=$(dasel -i toml -o json < "$${config_path}")
|
||||
if ! existing_json=$(dasel -i toml -o json < "$${config_path}" 2>/dev/null); then
|
||||
printf "Error: existing %s contains invalid TOML, cannot merge\n" "$${config_path}" >&2
|
||||
exit 1
|
||||
fi
|
||||
new_json=$(echo "$${new_json}" "$${existing_json}" | jq -s '.[0] * .[1]')
|
||||
fi
|
||||
|
||||
# Single conversion: JSON to TOML.
|
||||
echo "$${new_json}" | dasel -i json -o toml > "$${config_path}"
|
||||
# Single conversion: JSON to TOML (atomic via temp file).
|
||||
local tmp
|
||||
tmp=$(mktemp "$${config_path}.XXXXXX")
|
||||
echo "$${new_json}" | dasel -i json -o toml > "$${tmp}" && mv "$${tmp}" "$${config_path}"
|
||||
}
|
||||
|
||||
function setup_workdir() {
|
||||
if [ -n "$${ARG_WORKDIR}" ] && [ ! -d "$${ARG_WORKDIR}" ]; then
|
||||
echo "Creating workdir: $${ARG_WORKDIR}"
|
||||
@@ -230,6 +234,10 @@ EOF
|
||||
|
||||
install_codex
|
||||
install_dasel
|
||||
if ! command_exists jq; then
|
||||
printf "Error: jq is required but not installed.\n" >&2
|
||||
exit 1
|
||||
fi
|
||||
populate_config_toml
|
||||
setup_workdir
|
||||
add_auth_json
|
||||
|
||||
Reference in New Issue
Block a user