fix(cli): reconcile normalized Codex hooks

[astandrik]

What does this PR do?

Fixes the Codex configuration corruption reported in #1432 when a managed lifecycle hook has been normalized into inline TOML without marker comments.

• Reconciles owned SessionStart and SubagentStart hooks structurally across inline assignments, dotted or quoted paths, and array-of-tables.
• Preserves foreign entries, including hooks with the same matcher, and rejects malformed or ambiguous ownership without modifying the file.
• Runs a read-only installer preflight and safely migrates owned TOML hooks when hooks.json is used.
• Strengthens managed array-table path collision guards, including the equivalent Kimi configuration path.
• Keeps ownership-safe skill, profile, and JSON-hook cleanup independent when ambiguous TOML blocks config mutation.

Why is this PR larger than usual?

The existing TOML layer has no dependency-backed parser, so safe reconciliation requires bounded structural parsing, ownership validation, representation-aware rewriting, and byte-identical fail-closed paths. The implementation and regression matrix are kept together as one atomic bug fix; most added lines are parser safety logic and tests, with no new dependency or public CLI/MCP API.

Regression evidence

• Fail before: scripts/test.sh --suites cli reported 257 passed and 1 failed. cli_codex_inline_hook_issue1432 showed install appending a conflicting [[hooks.SessionStart]] block to the inline fixture from the issue.
• Pass after: scripts/test.sh --suites config_toml_edit,cli reports 304 passed under ASan/UBSan after rebasing onto current origin/main; both the issue fixture and independent-uninstall-cleanup regressions pass.
• Full local run: 7397 passed, 3 failed, and 4 skipped. The isolated py_lsp_scale rerun passed. The remaining mcp and index_supervisor containment cases reproduced with worker processes stuck in macOS UE state; their implementation and test files are unchanged by this PR.

Verification

• scripts/test.sh --suites config_toml_edit,cli
• changed-file clang-format --dry-run --Werror
• changed-source -Wall -Wextra -Werror syntax check
• make -f Makefile.cbm lint-no-suppress
• git diff --check origin/main...HEAD
• scripts/check-dco.sh origin/main..HEAD

make -f Makefile.cbm lint-ci is locally blocked before source analysis because cppcheck is not installed in the environment.

Checklist

• Every commit is signed off (git commit -s) — required, CI rejects unsigned commits
• Tests pass locally (make -f Makefile.cbm test) — the focused suites pass; the full-run exceptions are documented above
• Lint passes (make -f Makefile.cbm lint-ci) — blocked locally by the missing cppcheck; GitHub CI remains the authoritative run
• New behavior is covered by a test (reproduce-first for bug fixes)

Rollback

Revert both PR commits, newest first:

git revert c09be6ad874e06771540ffe078118742c3e21679 7d10fcc0fcc026e8010c6cdd8e148689e3035dc4

Closes #1432

[Copilot]

🟡 Not ready to approve

There is a confirmed validation bug in the new TOML number parser (signed base integers) and an uninstall-path cleanup gap when hook preflight fails.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Pull request overview

This PR fixes Codex config.toml corruption (duplicate hooks.SessionStart definitions) by adding structural reconciliation of installer-owned SessionStart/SubagentStart hooks across TOML representations, and by introducing a read-only preflight path to fail closed before mutating Codex artifacts.

Changes:

• Add a new TOML reconciliation engine that can detect/repair owned Codex lifecycle hooks in both inline and array-of-tables forms while preserving foreign entries.
• Wire the CLI installer/uninstaller to run a Codex hook preflight and to migrate/clean up TOML-owned hooks when hooks.json is used.
• Add regression + matrix tests covering inline, mixed, malformed/ambiguous, and BOM/CRLF scenarios.

File summaries

File	Description
tests/test_config_toml_edit.c	Adds extensive reconciliation and fail-closed regression tests for Codex TOML hook editing.
tests/test_cli.c	Adds CLI-level regression tests for issue #1432, read-only preflight, and install/dry-run/idempotency lifecycle.
src/cli/config_toml_edit.h	Introduces the Codex hook reconciliation API and mode enum.
src/cli/config_toml_edit.c	Implements structural parsing + reconciliation for owned Codex hooks and strengthens collision guards.
src/cli/cli.c	Switches Codex hook install/removal to reconciliation API and adds installer/uninstaller preflight gating.

Review details

• Files reviewed: 5/5 changed files
• Comments generated: 2
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

src/cli/config_toml_edit.c:4278

• edit->replacement can be NULL when replacement_len == 0 (e.g., deletion edits pushed with NULL). This relies on toml_buffer_append() being safe with a NULL pointer and zero length; that’s not guaranteed unless toml_buffer_append() explicitly handles len == 0 without dereferencing the pointer. To make this robust, skip the append when replacement_len == 0, or ensure toml_buffer_append() defensively returns OK for len == 0 regardless of data pointer value.

        if (edit->start < cursor || edit->end > len ||
            toml_buffer_append(output, data + cursor, edit->start - cursor) != TOML_EDIT_OK ||
            toml_buffer_append(output, edit->replacement, edit->replacement_len) != TOML_EDIT_OK) {
            return TOML_EDIT_ERR;
        }

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (3)

src/cli/config_toml_edit.c:2919

• The base-prefixed integer branch only triggers when pos == 0U, so signed hex/octal/binary forms like -0x10 / +0o77 are treated as invalid. TOML allows an optional sign on integers (including base-prefixed), so this can cause hook reconciliation to fail-closed on valid user configs that include signed base-prefixed numbers inside inline hook objects. Consider accepting the base prefix at pos (after the optional sign) and adjusting the digit start/end accordingly; update the invalid-fixture tests that currently assume signed base-prefixed values are malformed.

static int toml_codex_number_is_valid(const char *data, size_t len) {
    size_t pos = 0U;
    if (pos < len && (data[pos] == '+' || data[pos] == '-')) {
        pos++;
    }
    if (pos == len) {
        return 0;
    }
    if ((len - pos == 3U && memcmp(data + pos, "inf", 3U) == 0) ||
        (len - pos == 3U && memcmp(data + pos, "nan", 3U) == 0)) {
        return 1;
    }
    if (pos == 0U && len >= 3U && data[0] == '0' &&
        (data[1] == 'x' || data[1] == 'o' || data[1] == 'b')) {
        int base = data[1] == 'x' ? 16 : (data[1] == 'o' ? 8 : 2);
        return toml_codex_digits_are_valid(data, 2U, len, base, NULL, NULL);
    }

src/cli/config_toml_edit.c:2720

• Duplicate-key detection is O(n) per insertion, which becomes O(n²) for large inline tables/objects. Since TOML_CODEX_MAX_ITEMS is 65536, a crafted inline hook value with many fields could cause very slow parsing even though you ultimately fail-closed. Consider tightening the maximum expected field count for these Codex-specific objects (e.g., a small fixed upper bound), or switching duplicate detection to something sub-quadratic (e.g., sort once then scan, or a small hash set keyed by the rendered key-path).

static int toml_codex_field_vector_push(toml_codex_field_vector_t *vector, toml_key_path_t *key,
                                        size_t value_start, size_t value_end) {
    if (!vector || !key || value_start > value_end || vector->count >= TOML_CODEX_MAX_ITEMS) {
        return TOML_EDIT_ERR;
    }
    for (size_t i = 0U; i < vector->count; ++i) {
        if (toml_key_path_equal(&vector->items[i].key, key)) {
            return TOML_EDIT_ERR;
        }
    }

tests/test_cli.c:7947

• These new CLI tests hardcode /tmp/... for temp directories. This is not portable on Windows (and can be problematic in sandboxed CI), even though other parts of the test suite appear to use helper tempdir utilities. Prefer using an existing cross-platform tempdir helper (or deriving the base temp directory from the environment) and then applying the XXXXXX template under that directory.

    char tmpdir[256];
    snprintf(tmpdir, sizeof(tmpdir), "/tmp/cli-codex-inline-hook-XXXXXX");
    if (!cbm_mkdtemp(tmpdir))
        FAIL("cbm_mkdtemp failed");

[github-actions]

Thanks for opening this — it has been seen, and it is queued.

This note is automated, but it is not a brush-off: it exists so you know where your PR stands instead of having to guess from silence.

Current review status: working through a backlog. 0.9.1-rc.1 is out, so the release freeze that held reviews is over — but it left a large queue of open pull requests behind it, and we are reading through them oldest-first. The background is in discussion #1144.

What that means for this PR, concretely:

• It will not be closed for inactivity. No stale bot touches pull requests here.
• It may still sit a while before a human reads it. That is on us, not on you.
• Older PRs are read first, so a recent one is not being skipped — it is behind a queue.

Things that will genuinely speed it up whenever review does happen:

• Keep it rebased on main — the tree is moving quickly right now, and a conflicting branch cannot be reviewed as the diff you intended.
• Get CI green, or say which failures you believe are pre-existing.
• Keep the change to one claim. Bundled features and refactors get split before they get merged, which costs you a round trip.
• Every commit needs a sign-off (git commit -s) — CI enforces DCO.

If this fixes a bug, a reproduction we can run is worth more than a description of the symptom.

Thanks for contributing, and sorry in advance for the wait.