config: delete AppMode pretenders and VerifierVerdictPolicy - #5844
Conversation
Audit slice: dead/pretender mode abstractions (item 1). AppMode::Auto had no user spelling: parse() maps "auto" to Agent and the variant was never in the Tab cycle (crates/config/src/app_mode.rs). Only tests constructed it directly. Delete the variant and drop the defensive Auto arms from every exhaustive match in codewhale-tui, plus the init comment that explained the ghost (crates/tui/src/tui/app/init.rs). command-contract's CommandMode::Auto existed only as the adapter mirror of the dead variant (crates/tui/src/commands/contract.rs); remove the mirror and its adapter arms. CommandMode has no consumers outside the tui adapter, so the boundary shape stays otherwise untouched. Tests updated (not weakened): the Auto assertions in app_mode helper and base_policy_for_mode tests, the cycle_mode_reverse Auto leg, the protocol_parity mode-label round trip, the command-contract boundary mapping test, and the engine mode-invariant matrix "auto-compat" case (behavior identical to the remaining "agent" case). Gates: cargo fmt --check; nextest codewhale-tui 11909 passed; codewhale-config 639 passed; codewhale-command-contract 29 passed; dead-code budget PASS 425; vocabulary gate exit 0.
Audit slice: dead/pretender mode abstractions (item 2).
Delete the AppMode::Yolo variant. parse() keeps accepting the legacy
spellings ("yolo" | "4" | "bypass" | "bypass-permissions" |
"bypasspermissions") and now returns AppMode::Agent directly: YOLO was
a permission change (Full Access + trust + shell), never a mode. The
posture split already lives at the edges and is untouched there:
settings load rewrites default_mode=yolo into Act + full-access
posture (crates/tui/src/settings.rs), the runtime wire reader re-derives
Bypass from the raw legacy spelling (crates/tui/src/runtime_policy.rs),
and --yolo keeps working through the launch flag plus the new
set_mode_yolo_compat elevation.
Mode-shaped entry points keep their behavior through the posture:
- set_mode loses the Yolo fold; the transient full-access elevation
moves to App::set_mode_yolo_compat / App::select_yolo_compat with the
same lock refusal, baseline capture, and once-per-install toast.
- Alt+Y routes through apply_yolo_compat_update; /mode yolo and /zidong
route through the compat path before parse folds the alias.
- authority/engine drop the mode==Yolo disjuncts; auto_approve and
ApprovalMode::Bypass carry the posture, so sandbox, shell, tool
approval, and narrowing behavior are unchanged.
CommandMode::Yolo dies with it (no external consumers of the
command-contract enum).
Tests updated (not weakened): yolo spellings now assert parse -> Agent;
set_mode_yolo_compat/select_yolo_compat cover the elevation, baseline
restore, locked-policy refusal, and startup-default persistence the
variant tests pinned; engine posture tests pass Agent + explicit
full-access flags instead of the retired Yolo mode.
Gates: cargo fmt --check; nextest codewhale-tui 11908 passed;
codewhale-config 639 passed; codewhale-command-contract 29 passed;
dead-code budget PASS 425; vocabulary gate exit 0.
Audit slice: dead/pretender mode abstractions (item 3).
VerifierVerdictPolicy was a single-value enum ("hunt" only) — one value
is not a setting. Delete the enum and the [verifier].verdict_policy
field. ConfigToml has no deny_unknown_fields, so an old config that
still carries the key keeps loading and the key is read-only dropped
(never re-serialized), matching the retired launch_screen pattern.
harness_profiles (ConfigToml field, resolve_harness_profile helper, and
the whole crates/config/src/harness.rs type family) was accepted and
serialized but had no runtime consumer — harness.rs said so itself
("wired later"). Per the no-framework-without-a-caller rule, remove the
writable schema entry and delete the dead types with it. Old configs
with [[harness_profiles]] tables keep parsing (unknown keys ignored).
The cli bundle exporter drops its harness routing (the key can no
longer occur); the portable bundle's profiles table remains for
importing older bundles.
Docs and config.example.toml drop the matching schema sections. Tests
updated (not weakened): the Hunt assertions become a legacy-config
loads-cleanly test; the twelve harness-only tests are removed with the
feature they encoded.
Gates: cargo fmt --check; nextest codewhale-config 626 passed;
codewhale-tui 11908 passed; codewhale-command-contract 29 passed;
codewhale-cli 332 passed; dead-code budget PASS 425; vocabulary gate
exit 0.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_36e55167-ff1d-4367-b4ec-d5bbae7a0461) |
|
Claude finished @Hmbown's task in 3m 9s —— View job Review: config: delete AppMode pretenders and VerifierVerdictPolicy
SummaryThis is a clean, well-executed deletion of the
Findings (doc drift, not code bugs)
Neither blocks merge; both are pre-existing-style doc lag that a maintainer may want swept up separately (possibly same PR, possibly a fast follow). |
There was a problem hiding this comment.
🟡 Changes recommended
The new set_mode_yolo_compat path is not idempotent, which can cause unnecessary mode-change hooks/engine syncs and makes PersistedSame outcomes effectively unreachable for repeated legacy YOLO selections.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Cleans up configuration and runtime posture by removing the deprecated “pretender” modes (Auto, Yolo) and the retired verifier/harness-profile config surfaces, while preserving backward compatibility by mapping legacy strings to Act (Agent) plus Full Access via approval posture (Bypass / auto_approve) instead of a separate mode.
Changes:
- Remove
AppMode::Auto/AppMode::Yolo(andCommandModemirrors), routing legacy “yolo/bypass/4” inputs through a dedicated Full Access compat path. - Remove
VerifierVerdictPolicy/[verifier].verdict_policyand delete the[[harness_profiles]]schema surface (code + docs + bundle export). - Update TUI/engine/tool-catalog/authority logic and tests to treat “Full Access” as an approval posture (Bypass) rather than a mode.
File summaries
| File | Description |
|---|---|
| docs/CONFIGURATION.md | Removes documentation for [[harness_profiles]] and [verifier].verdict_policy. |
| crates/tui/src/tui/widgets/mod.rs | Removes mode-color branching on deleted mode variants. |
| crates/tui/src/tui/underwater.rs | Removes UI ink/label branching on deleted mode variants. |
| crates/tui/src/tui/ui/tests.rs | Updates UI approval/mode tests to remove Yolo mode usage and assert Bypass posture instead. |
| crates/tui/src/tui/ui/event_loop.rs | Routes Alt+Y legacy shortcut through YOLO compat apply path. |
| crates/tui/src/tui/ui/approval_routing.rs | Removes AppMode::Yolo checks from UI-side auto-approve helper. |
| crates/tui/src/tui/ui/apply.rs | Adds apply_yolo_compat_update for legacy YOLO shortcut handling. |
| crates/tui/src/tui/startup_defaults.rs | Removes startup-default test tied to AppMode::Yolo. |
| crates/tui/src/tui/hotbar/actions.rs | Removes hotbar actions/descriptions for deleted mode variants. |
| crates/tui/src/tui/app/types.rs | Removes UI text mapping for deleted mode variants. |
| crates/tui/src/tui/app/tests.rs | Updates app-mode parsing/cycle tests and YOLO entry-point tests to use compat APIs. |
| crates/tui/src/tui/app/init.rs | Removes default-mode “yolo” inference; treats compat elevation as flag-only. |
| crates/tui/src/tui/app.rs | Introduces set_mode_yolo_compat / select_yolo_compat and removes AppMode::Yolo handling from set_mode. |
| crates/tui/src/runtime_threads/tests.rs | Updates CLI/runtime parsing tests to map legacy YOLO spellings to Agent. |
| crates/tui/src/runtime_policy.rs | Normalizes runtime request/persisted shapes: legacy YOLO strings imply Bypass posture, mode stays Agent. |
| crates/tui/src/prompts.rs | Renames/updates prompt contract test to treat Full Access as posture rather than mode. |
| crates/tui/src/lib.rs | Removes direct-workflow “Yolo mode” selection; expresses Full Access via approval posture. |
| crates/tui/src/exec_agent.rs | Removes “Yolo mode when auto_approve” behavior; keeps mode Agent and uses posture fields. |
| crates/tui/src/core/protocol_parity.rs | Removes lossless “auto/yolo” string round-trip; keeps only real modes. |
| crates/tui/src/core/engine/tool_catalog/tests.rs | Removes Yolo from tool-catalog test matrices. |
| crates/tui/src/core/engine/tests.rs | Updates engine tests to remove Yolo/Auto mode usage and assert Full Access via posture/auto_approve. |
| crates/tui/src/core/engine/preview.rs | Removes “mode==Yolo implies auto_approve” in prompt/session-facts building. |
| crates/tui/src/core/engine.rs | Removes catalog-mode remap to Yolo for bypass; relies on input policy mode + posture. |
| crates/tui/src/core/authority.rs | Deletes Yolo/Auto mode rows from policy tables; treats Bypass approval posture as the Full Access contract. |
| crates/tui/src/config/tests.rs | Updates verifier config parsing/merge tests after verdict-policy removal. |
| crates/tui/src/config_ui.rs | Removes handling of deleted AppMode variants from config UI mapping. |
| crates/tui/src/commands/groups/core/core.rs | Removes “Auto/Yolo mode tips” branching and tests. |
| crates/tui/src/commands/groups/config/status.rs | Updates status test that previously forced AppMode::Yolo. |
| crates/tui/src/commands/groups/config/config.rs | Routes `/mode yolo |
| crates/tui/src/commands/contract.rs | Removes CommandMode mappings for deleted variants and updates adapter tests. |
| crates/config/src/tests.rs | Replaces verifier verdict-policy default/rejection tests with legacy-acceptance coverage. |
| crates/config/src/lib.rs | Removes harness exports and VerifierVerdictPolicy; simplifies VerifierConfigToml. |
| crates/config/src/harness.rs | Deletes the harness profile/posture schema module. |
| crates/config/src/app_mode.rs | Removes Auto/Yolo variants; maps legacy YOLO spellings to Agent at parse time. |
| crates/command-contract/src/types.rs | Removes CommandMode variants for Auto and Yolo. |
| crates/cli/src/config_bundles.rs | Removes harness profile export bucket/sectioning from portable bundles. |
| config.example.toml | Removes commented examples for harness profiles and verdict_policy. |
Review details
- Files reviewed: 37/37 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| /// On-disk schema for `[verifier]`. | ||
| #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] | ||
| #[derive(Debug, Clone, Default, Serialize, Deserialize, PartialEq, Eq)] | ||
| pub struct VerifierConfigToml { | ||
| /// Enable automatic verifier preview when the runtime wires a | ||
| /// claim-of-done trigger. Manual `run_verifiers` remains available | ||
| /// regardless. | ||
| #[serde(default)] | ||
| pub enabled: bool, | ||
| /// How verifier verdicts map into the goal/hunt system. | ||
| #[serde(default)] | ||
| pub verdict_policy: VerifierVerdictPolicy, | ||
| } | ||
|
|
||
| impl Default for VerifierConfigToml { | ||
| fn default() -> Self { | ||
| Self { | ||
| enabled: false, | ||
| verdict_policy: VerifierVerdictPolicy::Hunt, | ||
| } | ||
| } | ||
| } |
| let previous_mode = self.mode; | ||
| // Same baseline-refresh rule as a mode hop: the elevation must not | ||
| // bleed into the restored Agent surface. | ||
| if previous_mode.uses_agent_baseline() && !self.yolo { | ||
| self.mode_prefs = ModeSessionPrefs { | ||
| agent_allow_shell: self.allow_shell, | ||
| agent_trust_mode: self.trust_mode, | ||
| agent_approval_mode: self.approval_mode, | ||
| }; | ||
| } |
| let config: ConfigToml = toml::from_str( | ||
| r#" | ||
| [verifier] | ||
| enabled = true | ||
| verdict_policy = "hunt" | ||
|
|
||
| [verifier.unknown_extra] | ||
| key = "value" | ||
| "#, |
![codewhale-agent[bot]](https://avatars.githubusercontent.com/u/101357273?s=60&v=4){kind=small}
There was a problem hiding this comment.
Codewhale review
PR removes AppMode::Auto/Yolo and VerifierVerdictPolicy, deletes the preview harness_profiles schema, and routes legacy YOLO spellings through Act + Bypass posture paths. The split between modes and approval posture is generally consistent across TUI/engine/authority code, but a few edge-case regressions and test blind spots remain.
Findings
- [WARNING] Legacy YOLO raw-string detection omits the
autoalias in RuntimePolicyProjection (crates/tui/src/runtime_policy.rs:105)
The newlegacy_yolo_aliashelper only recognizesyolo,4,bypass,bypass-permissions, andbypasspermissions. Howeverautois still parsed toAppMode::Agentelsewhere and historically represented an Auto/Yolo-looking posture that was collapsed byvisible_mode. If RuntimePolicyProjection receives a persisted or wire-sentmode = "auto", it will now follow theNone/Suggest branch unlessauto_approveis also true, potentially downgrading behavior compared with the previousparsed_mode == AppMode::Yolo || auto_approvelogic. This may be intentional, but it is an unmentioned compatibility change and lacks a test asserting theautoprojection path. - [WARNING] Verifier legacy unknown-field acceptance is not actually asserted for
verdict_policyspecifically (crates/config/src/tests.rs:42)
The new testretired_verifier_verdict_policy_is_accepted_and_dropped_on_loadexpects deserialization to succeed and only assertsverifier.enabled. It does not assert that the legacyverdict_policyvalue is dropped from a round-trip serialization. The test title claims the key is dropped, but the absence ofdeny_unknown_fieldsmeans serde silently ignores all unknown keys, so the test does not prove the intended persistence behavior. - [INFO]
switch_yolo_compat_with_statusreportsChangedeven when no live state changed (crates/tui/src/commands/groups/config/config.rs:2983)
set_mode_yolo_compatreturnstrueunconditionally after the locked-policy guard, so invoking/mode yolorepeatedly (or pressing Alt+Y repeatedly) will keep reporting 'Switched to Act mode.' and issuing mode-change hooks even when the app is already in the same elevated state. This matches some oldYolobehavior but is still noisy and may cause unnecessary ModeChange hooks and persistence work. - [INFO] Removed harness profile tests leave coverage gap if any code still constructs the deleted types (
crates/config/src/lib.rs:861)
The PR deletescrates/config/src/harness.rsand all direct harness profile unit tests. Dead-code CI passing suggests no references remain, but this is a large removal of schema/public API. If downstream crates or external consumers still try to deserialize[[harness_profiles]], they will now silently ignore that table becauseConfigTomlno longer has the field and nodeny_unknown_fieldsis used. That may be acceptable cleanup, but the silent downgrade from a previously validated preview schema to an ignored section is a user-facing config behavior change.
Suggestions
-
crates/config/src/tests.rs:42— Strengthen the legacy verifier test to prove serialization dropsverdict_policy, not just that parsing accepts unknown keys.let serialized = toml::to_string_pretty(&config).expect("serialize verifier config");
Assessment
The refactor is coherent and removes dead/compat enum branches while preserving the Bypass posture for full access. The main risks are silent compatibility changes for old auto/YOLO wire strings and an undertested legacy verifier serialization guarantee.
Advisory review by Codewhale (codewhale review --pr 5844 --post, head 1ce049e1f9508023a34348b858c145a3c798050f). Line-specific findings are also posted as inline review comments; mechanical fixes arrive as committable suggestions you can apply from the Files tab. CODEOWNERS approval still governs merge.
| /// Legacy mode spellings that carried the Full Access posture. `AppMode:: | ||
| /// parse` folds them to Agent; the posture is re-derived from the raw wire | ||
| /// value so old persisted shapes keep their permission meaning. | ||
| #[must_use] |
There was a problem hiding this comment.
[WARNING] Legacy YOLO raw-string detection omits the auto alias in RuntimePolicyProjection
The new legacy_yolo_alias helper only recognizes yolo, 4, bypass, bypass-permissions, and bypasspermissions. However auto is still parsed to AppMode::Agent elsewhere and historically represented an Auto/Yolo-looking posture that was collapsed by visible_mode. If RuntimePolicyProjection receives a persisted or wire-sent mode = "auto", it will now follow the None/Suggest branch unless auto_approve is also true, potentially downgrading behavior compared with the previous parsed_mode == AppMode::Yolo || auto_approve logic. This may be intentional, but it is an unmentioned compatibility change and lacks a test asserting the auto projection path.
| enabled = true | ||
| verdict_policy = "hunt" | ||
|
|
||
| [verifier.unknown_extra] |
There was a problem hiding this comment.
[WARNING] Verifier legacy unknown-field acceptance is not actually asserted for verdict_policy specifically
The new test retired_verifier_verdict_policy_is_accepted_and_dropped_on_load expects deserialization to succeed and only asserts verifier.enabled. It does not assert that the legacy verdict_policy value is dropped from a round-trip serialization. The test title claims the key is dropped, but the absence of deny_unknown_fields means serde silently ignores all unknown keys, so the test does not prove the intended persistence behavior.
| true, | ||
| ), | ||
| SettingSelection::PersistedSame => { | ||
| (app.mode_startup_default_receipt(AppMode::Agent), false) |
There was a problem hiding this comment.
[INFO] switch_yolo_compat_with_status reports Changed even when no live state changed
set_mode_yolo_compat returns true unconditionally after the locked-policy guard, so invoking /mode yolo repeatedly (or pressing Alt+Y repeatedly) will keep reporting 'Switched to Act mode.' and issuing mode-change hooks even when the app is already in the same elevated state. This matches some old Yolo behavior but is still noisy and may cause unnecessary ModeChange hooks and persistence work.
| @@ -866,10 +861,6 @@ pub struct ConfigToml { | |||
| /// applies the defaults documented in [`LspConfigToml`]. | |||
There was a problem hiding this comment.
[INFO] Removed harness profile tests leave coverage gap if any code still constructs the deleted types
The PR deletes crates/config/src/harness.rs and all direct harness profile unit tests. Dead-code CI passing suggests no references remain, but this is a large removal of schema/public API. If downstream crates or external consumers still try to deserialize [[harness_profiles]], they will now silently ignore that table because ConfigToml no longer has the field and no deny_unknown_fields is used. That may be acceptable cleanup, but the silent downgrade from a previously validated preview schema to an ignored section is a user-facing config behavior change.
| enabled = true | ||
| verdict_policy = "hunt" | ||
|
|
||
| [verifier.unknown_extra] |
There was a problem hiding this comment.
Strengthen the legacy verifier test to prove serialization drops verdict_policy, not just that parsing accepts unknown keys.
| [verifier.unknown_extra] | |
| let serialized = toml::to_string_pretty(&config).expect("serialize verifier config"); |
3 commits: delete AppMode::Auto + CommandMode mirror, finish YOLO as parse alias, delete VerifierVerdictPolicy + harness_profiles schema. Gates: fmt clean, dead-code PASS at 425. Full matrix via CI.
No-Issue: audit-leftover hygiene with no single tracking issue.