diff --git a/cmd/entire/cli/agent/codex/trust.go b/cmd/entire/cli/agent/codex/trust.go index b8dde10964..2147fa9a7a 100644 --- a/cmd/entire/cli/agent/codex/trust.go +++ b/cmd/entire/cli/agent/codex/trust.go @@ -4,14 +4,16 @@ import ( "encoding/json" "os" "path/filepath" - "regexp" "strings" + + "github.com/pelletier/go-toml/v2" ) // HookTrustGaps returns the snake_case event labels declared in // /.codex/hooks.json that don't have a matching -// `[hooks.state."::0:0"]` entry in the user's Codex -// config.toml — i.e. events the local user hasn't approved yet. +// `[hooks.state.<"hooks.json:event:0:0">]` entry (any TOML key quoting) +// in the user's Codex config.toml — i.e. events the local user hasn't +// approved yet. // // This is the structural form of the trust check: we don't recompute // Codex's hook hash, we only look at key presence. That misses the @@ -20,24 +22,29 @@ import ( // to surface fresh additions like "you trusted three hooks last month // but a new PostToolUse arrived" inside our SessionStart welcome. // -// Returns nil when: -// - .codex/hooks.json doesn't exist (entire isn't installed in this repo) -// - The user's config.toml can't be read -// - Every declared event already has a state entry -func HookTrustGaps(repoRoot string) []string { +// The bool reports whether the check was actually performed. It is +// false — with nil gaps — when hooks.json is missing/malformed or the +// user's config.toml can't be read or parsed as TOML: "can't tell" +// cases where the caller must not claim the hooks were verified. +// Callers that only warn on gaps (the SessionStart banner) can ignore +// it; doctor uses it to say "not verified" instead of "OK". +func HookTrustGaps(repoRoot string) ([]string, bool) { hooksJSONPath := filepath.Join(repoRoot, ".codex", "hooks.json") declared, ok := declaredCodexEvents(hooksJSONPath) - if !ok || len(declared) == 0 { - return nil + if !ok { + return nil, false + } + if len(declared) == 0 { + return nil, true } configPath := codexConfigPath() if configPath == "" { - return nil + return nil, false } trusted, ok := readCodexTrustedKeys(configPath) if !ok { - return nil + return nil, false } var gaps []string @@ -49,7 +56,7 @@ func HookTrustGaps(repoRoot string) []string { gaps = append(gaps, ev) } } - return gaps + return gaps, true } func codexConfigPath() string { @@ -124,20 +131,40 @@ func MissingEntireHooks(repoRoot string) []string { return missing } -// codexTrustStateHeaderRegex matches `[hooks.state.""]` headers in -// the user's Codex config.toml. Quote-only — Codex's own writer emits -// quoted keys (codex-rs/tui/src/app/background_requests.rs:874), and -// looser parsing would invite false matches in user-edited configs. -var codexTrustStateHeaderRegex = regexp.MustCompile(`(?m)^\[hooks\.state\."([^"]+)"\]`) - +// readCodexTrustedKeys parses the user's Codex config.toml and returns +// the set of keys under the `hooks.state` table. The parse is +// structural (not a header regex): Codex's writer has emitted both +// basic (double-quoted) and literal (single-quoted) TOML keys — on +// Windows the backslashes in the hooks.json path make literal quoting +// the natural serialization — and a real TOML parse handles every +// quoting form, unescaping basic strings so keys compare cleanly +// against filepath.Join output (issue #1761). +// +// Returns ok=false when the file is missing or not valid TOML — "can't +// tell" cases where callers must stay silent rather than flag every +// hook as untrusted. A parseable config without a hooks.state table +// returns an empty set with ok=true: that's a real "nothing approved +// yet" state and the gap warning should fire. func readCodexTrustedKeys(configPath string) (map[string]struct{}, bool) { data, err := os.ReadFile(configPath) //nolint:gosec // path resolved from CODEX_HOME or HOME if err != nil { return nil, false } + var config map[string]any + if err := toml.Unmarshal(data, &config); err != nil { + return nil, false + } keys := make(map[string]struct{}) - for _, m := range codexTrustStateHeaderRegex.FindAllStringSubmatch(string(data), -1) { - keys[m[1]] = struct{}{} + hooks, ok := config["hooks"].(map[string]any) + if !ok { + return keys, true + } + state, ok := hooks["state"].(map[string]any) + if !ok { + return keys, true + } + for k := range state { + keys[k] = struct{}{} } return keys, true } diff --git a/cmd/entire/cli/agent/codex/trust_test.go b/cmd/entire/cli/agent/codex/trust_test.go index b7e24fdf35..1578ac971f 100644 --- a/cmd/entire/cli/agent/codex/trust_test.go +++ b/cmd/entire/cli/agent/codex/trust_test.go @@ -8,6 +8,19 @@ import ( "github.com/stretchr/testify/require" ) +// Shared hooks.json fixtures. The two-event form exercises the +// trusted/untrusted split; the one-event form is for tests where only +// config.toml parsing behavior matters. +const ( + hooksJSONSessionStartAndPostToolUse = `{ + "hooks": { + "SessionStart": [{"matcher": null, "hooks": [{"type":"command","command":"x","timeout":30}]}], + "PostToolUse": [{"matcher": null, "hooks": [{"type":"command","command":"x","timeout":30}]}] + } +}` + hooksJSONSessionStartOnly = `{"hooks":{"SessionStart":[{"matcher":null,"hooks":[{"type":"command","command":"x","timeout":30}]}]}}` +) + // writeTrustFixture sets up the .codex/hooks.json fixture and points // CODEX_HOME at an isolated temp directory so HookTrustGaps resolves // the user config without touching ~/.codex on the dev machine. Tests @@ -55,19 +68,15 @@ trusted_hash = "sha256:ccc" ` require.NoError(t, os.WriteFile(filepath.Join(os.Getenv("CODEX_HOME"), "config.toml"), []byte(configTOML), 0o600)) - gaps := HookTrustGaps(repoRoot) + gaps, ok := HookTrustGaps(repoRoot) + require.True(t, ok) require.Equal(t, []string{"post_tool_use"}, gaps) } // TestHookTrustGaps_NoGapsWhenAllTrusted returns nil when every declared // event has a state entry, even if extra entries exist for other paths. func TestHookTrustGaps_NoGapsWhenAllTrusted(t *testing.T) { - hooksJSON := `{ - "hooks": { - "SessionStart": [{"matcher": null, "hooks": [{"type":"command","command":"x","timeout":30}]}], - "PostToolUse": [{"matcher": null, "hooks": [{"type":"command","command":"x","timeout":30}]}] - } -}` + hooksJSON := hooksJSONSessionStartAndPostToolUse repoRoot, hooksPath := writeTrustFixture(t, hooksJSON) // Trust both, plus an unrelated entry from another repo to make sure @@ -83,7 +92,8 @@ trusted_hash = "sha256:ccc" ` require.NoError(t, os.WriteFile(filepath.Join(os.Getenv("CODEX_HOME"), "config.toml"), []byte(configTOML), 0o600)) - gaps := HookTrustGaps(repoRoot) + gaps, ok := HookTrustGaps(repoRoot) + require.True(t, ok) require.Empty(t, gaps) } @@ -92,7 +102,9 @@ trusted_hash = "sha256:ccc" func TestHookTrustGaps_NilWhenHooksJSONMissing(t *testing.T) { tmp := t.TempDir() t.Setenv("CODEX_HOME", tmp) - require.Nil(t, HookTrustGaps(tmp)) + gaps, ok := HookTrustGaps(tmp) + require.Nil(t, gaps) + require.False(t, ok) } // TestHookTrustGaps_NilWhenConfigUnreadable — first-run users have no @@ -100,7 +112,7 @@ func TestHookTrustGaps_NilWhenHooksJSONMissing(t *testing.T) { // our partial detection staying quiet is the right behavior; we'd // otherwise duplicate the warning. func TestHookTrustGaps_NilWhenConfigUnreadable(t *testing.T) { - hooksJSON := `{"hooks":{"SessionStart":[{"matcher":null,"hooks":[{"type":"command","command":"x","timeout":30}]}]}}` + hooksJSON := hooksJSONSessionStartOnly tmp := t.TempDir() codexHome := filepath.Join(tmp, "codex-home") require.NoError(t, os.MkdirAll(filepath.Join(tmp, "repo", ".codex"), 0o750)) @@ -108,7 +120,9 @@ func TestHookTrustGaps_NilWhenConfigUnreadable(t *testing.T) { require.NoError(t, os.WriteFile(filepath.Join(tmp, "repo", ".codex", "hooks.json"), []byte(hooksJSON), 0o600)) t.Setenv("CODEX_HOME", codexHome) - require.Nil(t, HookTrustGaps(filepath.Join(tmp, "repo"))) + gaps, ok := HookTrustGaps(filepath.Join(tmp, "repo")) + require.Nil(t, gaps) + require.False(t, ok) } // TestMissingEntireHooks_FlagsStaleFile — user enabled Codex on an @@ -173,5 +187,151 @@ func TestHookTrustGaps_HandlesNonzeroHandlerIndex(t *testing.T) { trusted_hash = "sha256:aaa" ` require.NoError(t, os.WriteFile(filepath.Join(os.Getenv("CODEX_HOME"), "config.toml"), []byte(configTOML), 0o600)) - require.Empty(t, HookTrustGaps(repoRoot)) + gaps, ok := HookTrustGaps(repoRoot) + require.True(t, ok) + require.Empty(t, gaps) +} + +// TestHookTrustGaps_SingleQuotedStateKeys — current Codex serializes +// state keys as TOML literal (single-quoted) strings, the natural form +// on Windows where backslashes need no escaping. The trust check must +// recognize them the same as double-quoted keys (issue #1761). +func TestHookTrustGaps_SingleQuotedStateKeys(t *testing.T) { + hooksJSON := hooksJSONSessionStartAndPostToolUse + repoRoot, hooksPath := writeTrustFixture(t, hooksJSON) + + configTOML := `[hooks.state.'` + hooksPath + `:session_start:0:0'] +trusted_hash = "sha256:aaa" + +[hooks.state.'` + hooksPath + `:post_tool_use:0:0'] +trusted_hash = "sha256:bbb" +` + require.NoError(t, os.WriteFile(filepath.Join(os.Getenv("CODEX_HOME"), "config.toml"), []byte(configTOML), 0o600)) + + gaps, ok := HookTrustGaps(repoRoot) + require.True(t, ok) + require.Empty(t, gaps) +} + +// TestHookTrustGaps_SingleQuotedFlagsMissingEvent — literal-quoted keys +// must not only count as trust; a genuinely missing event must still be +// flagged when the other entries are single-quoted. +func TestHookTrustGaps_SingleQuotedFlagsMissingEvent(t *testing.T) { + hooksJSON := hooksJSONSessionStartAndPostToolUse + repoRoot, hooksPath := writeTrustFixture(t, hooksJSON) + + configTOML := `[hooks.state.'` + hooksPath + `:session_start:0:0'] +trusted_hash = "sha256:aaa" +` + require.NoError(t, os.WriteFile(filepath.Join(os.Getenv("CODEX_HOME"), "config.toml"), []byte(configTOML), 0o600)) + + gaps, ok := HookTrustGaps(repoRoot) + require.True(t, ok) + require.Equal(t, []string{"post_tool_use"}, gaps) +} + +// TestHookTrustGaps_MalformedConfigStaysSilent — a config.toml that +// isn't valid TOML is a "can't tell" case, not a "nothing trusted" +// case. The old regex scraper degraded it to zero trusted keys and +// flagged every hook; structural parsing must report ok=false instead +// (same contract as an unreadable file) so doctor can say "not +// verified" rather than claiming OK. +func TestHookTrustGaps_MalformedConfigStaysSilent(t *testing.T) { + hooksJSON := hooksJSONSessionStartOnly + repoRoot, _ := writeTrustFixture(t, hooksJSON) + require.NoError(t, os.WriteFile(filepath.Join(os.Getenv("CODEX_HOME"), "config.toml"), []byte("[hooks.state.\"unterminated\n"), 0o600)) + gaps, ok := HookTrustGaps(repoRoot) + require.Nil(t, gaps) + require.False(t, ok) +} + +// TestHookTrustGaps_ConfigWithoutStateSection — a parseable config that +// simply has no hooks.state table is the genuine "fresh clone, nothing +// approved yet" state: the gap warning must fire. +func TestHookTrustGaps_ConfigWithoutStateSection(t *testing.T) { + hooksJSON := hooksJSONSessionStartOnly + repoRoot, _ := writeTrustFixture(t, hooksJSON) + require.NoError(t, os.WriteFile(filepath.Join(os.Getenv("CODEX_HOME"), "config.toml"), []byte("model = \"gpt-5\"\n"), 0o600)) + gaps, ok := HookTrustGaps(repoRoot) + require.True(t, ok) + require.Equal(t, []string{"session_start"}, gaps) +} + +// The readCodexTrustedKeys tests below hardcode Windows-shaped keys. +// Full-flow HookTrustGaps tests can't exercise backslash paths on a +// POSIX dev machine (filepath.Join emits "/"), but key parsing is +// OS-independent — this is exactly the layer issue #1761 lives in. + +// TestReadCodexTrustedKeys_WindowsLiteralKeys — the reported bug: a +// literal (single-quoted) key holding a Windows path must come back +// verbatim, backslashes intact. +func TestReadCodexTrustedKeys_WindowsLiteralKeys(t *testing.T) { + t.Parallel() + configPath := filepath.Join(t.TempDir(), "config.toml") + configTOML := `[hooks.state.'C:\repo\.codex\hooks.json:session_start:0:0'] +trusted_hash = "sha256:aaa" +` + require.NoError(t, os.WriteFile(configPath, []byte(configTOML), 0o600)) + + keys, ok := readCodexTrustedKeys(configPath) + require.True(t, ok) + require.Contains(t, keys, `C:\repo\.codex\hooks.json:session_start:0:0`) +} + +// TestReadCodexTrustedKeys_WindowsBasicEscapedKeys — the latent second +// bug from issue #1761: a basic (double-quoted) key holding a Windows +// path carries escaped backslashes in the raw file. The parser must +// unescape them so the key compares equal to filepath.Join output. The +// old regex captured the raw `C:\\repo\\...` text and could never +// match. +func TestReadCodexTrustedKeys_WindowsBasicEscapedKeys(t *testing.T) { + t.Parallel() + configPath := filepath.Join(t.TempDir(), "config.toml") + configTOML := `[hooks.state."C:\\repo\\.codex\\hooks.json:stop:0:0"] +trusted_hash = "sha256:bbb" +` + require.NoError(t, os.WriteFile(configPath, []byte(configTOML), 0o600)) + + keys, ok := readCodexTrustedKeys(configPath) + require.True(t, ok) + require.Contains(t, keys, `C:\repo\.codex\hooks.json:stop:0:0`) +} + +// TestReadCodexTrustedKeys_DottedAssignmentForm — trust entries written +// as dotted-key assignments under a [hooks.state] table instead of one +// header per entry are the same data in valid TOML; structural parsing +// handles the shape the old header regex could never see. +func TestReadCodexTrustedKeys_DottedAssignmentForm(t *testing.T) { + t.Parallel() + configPath := filepath.Join(t.TempDir(), "config.toml") + configTOML := `[hooks.state] +'C:\repo\.codex\hooks.json:post_tool_use:0:0'.trusted_hash = "sha256:ccc" +` + require.NoError(t, os.WriteFile(configPath, []byte(configTOML), 0o600)) + + keys, ok := readCodexTrustedKeys(configPath) + require.True(t, ok) + require.Contains(t, keys, `C:\repo\.codex\hooks.json:post_tool_use:0:0`) +} + +// TestReadCodexTrustedKeys_UnrelatedSectionsIgnored — keys elsewhere in +// the config (top-level settings, other tables) must not leak into the +// trusted set. +func TestReadCodexTrustedKeys_UnrelatedSectionsIgnored(t *testing.T) { + t.Parallel() + configPath := filepath.Join(t.TempDir(), "config.toml") + configTOML := `model = "gpt-5" + +[projects."/some/repo"] +trust_level = "trusted" + +[hooks.state."/repo/.codex/hooks.json:stop:0:0"] +trusted_hash = "sha256:ddd" +` + require.NoError(t, os.WriteFile(configPath, []byte(configTOML), 0o600)) + + keys, ok := readCodexTrustedKeys(configPath) + require.True(t, ok) + require.Len(t, keys, 1) + require.Contains(t, keys, "/repo/.codex/hooks.json:stop:0:0") } diff --git a/cmd/entire/cli/doctor.go b/cmd/entire/cli/doctor.go index 94af749a0d..da54a74f87 100644 --- a/cmd/entire/cli/doctor.go +++ b/cmd/entire/cli/doctor.go @@ -448,10 +448,17 @@ func checkCodexHookTrust(cmd *cobra.Command) { w := cmd.OutOrStdout() missing := codex.MissingEntireHooks(repoRoot) - gaps := codex.HookTrustGaps(repoRoot) + gaps, checked := codex.HookTrustGaps(repoRoot) if len(missing) == 0 && len(gaps) == 0 { - fmt.Fprintln(w, "✓ Codex hook trust: OK") + // checked=false is "can't tell" (unreadable/unparseable hooks.json + // or config.toml), which must not render as a verified OK. + if checked { + fmt.Fprintln(w, "✓ Codex hook trust: OK") + } else { + fmt.Fprintln(w, "Codex hook trust: NOT VERIFIED") + fmt.Fprintln(w, " Could not read hook approvals from Codex's config.toml.") + } return } diff --git a/cmd/entire/cli/doctor_test.go b/cmd/entire/cli/doctor_test.go index c2872699aa..976d3ed137 100644 --- a/cmd/entire/cli/doctor_test.go +++ b/cmd/entire/cli/doctor_test.go @@ -550,6 +550,34 @@ trusted_hash = "sha256:ccc" require.NotContains(t, out, "Codex hook trust: REVIEW NEEDED") } +// TestCheckCodexHookTrust_NotVerifiedWhenConfigUnparseable — a +// config.toml that fails TOML parsing (e.g. a duplicate key from a bad +// manual edit) is a "can't tell" case. Doctor must not render it as a +// verified "✓ OK", and must not flag the hooks as untrusted either — +// Codex's own startup warning covers real trust problems. +func TestCheckCodexHookTrust_NotVerifiedWhenConfigUnparseable(t *testing.T) { + dir := setupGitRepoForPhaseTest(t) + t.Chdir(dir) + + codexDir := filepath.Join(dir, ".codex") + require.NoError(t, os.MkdirAll(codexDir, 0o750)) + require.NoError(t, os.WriteFile(filepath.Join(codexDir, "hooks.json"), []byte(canonicalCodexHooksJSON()), 0o600)) + + codexHome := filepath.Join(t.TempDir(), "codex-home") + require.NoError(t, os.MkdirAll(codexHome, 0o750)) + malformedTOML := "model = \"gpt-5\"\nmodel = \"duplicate key fails the parse\"\n" + require.NoError(t, os.WriteFile(filepath.Join(codexHome, "config.toml"), []byte(malformedTOML), 0o600)) + t.Setenv("CODEX_HOME", codexHome) + + cmd, stdout := newTestCmd(t) + checkCodexHookTrust(cmd) + + out := stdout.String() + require.Contains(t, out, "Codex hook trust: NOT VERIFIED") + require.NotContains(t, out, "✓ Codex hook trust: OK") + require.NotContains(t, out, "REVIEW NEEDED") +} + // TestConfirmDoctorFix_CancelledContext verifies that a cancelled command // context makes the confirm prompt return (false, nil) rather than surfacing a // wrapped error — doctor fixes are skipped cleanly on interrupt. diff --git a/cmd/entire/cli/lifecycle.go b/cmd/entire/cli/lifecycle.go index a1c2ddab82..af05358054 100644 --- a/cmd/entire/cli/lifecycle.go +++ b/cmd/entire/cli/lifecycle.go @@ -193,7 +193,7 @@ func handleLifecycleSessionStart(ctx context.Context, ag agent.Agent, event *age // path, so missing entries here flag exactly that case. if ag.Name() == agent.AgentNameCodex { if root, err := paths.WorktreeRoot(ctx); err == nil { - if gaps := codex.HookTrustGaps(root); len(gaps) > 0 { + if gaps, _ := codex.HookTrustGaps(root); len(gaps) > 0 { message += fmt.Sprintf(" %d new hook(s) await approval (%s). Open /hooks to trust them.", len(gaps), strings.Join(gaps, ", ")) } } diff --git a/go.mod b/go.mod index 66bfc9af7e..5338439414 100644 --- a/go.mod +++ b/go.mod @@ -26,6 +26,7 @@ require ( github.com/muesli/termenv v0.16.0 github.com/ogen-go/ogen v1.23.0 github.com/oklog/ulid/v2 v2.1.1 + github.com/pelletier/go-toml/v2 v2.3.1 github.com/posthog/posthog-go v1.19.0 github.com/sergi/go-diff v1.4.0 github.com/spf13/cobra v1.10.2 @@ -111,7 +112,6 @@ require ( github.com/mitchellh/reflectwalk v1.0.2 // indirect github.com/muesli/cancelreader v0.2.2 // indirect github.com/nwaples/rardecode/v2 v2.2.2 // indirect - github.com/pelletier/go-toml/v2 v2.3.1 // indirect github.com/pierrec/lz4/v4 v4.1.26 // indirect github.com/pjbgf/sha1cd v0.6.0 // indirect github.com/pkoukk/tiktoken-go v0.1.8 // indirect