Feat/Rename /goal → /hunt with HuntVerdict + trophy cards (#2092)#2306
Feat/Rename /goal → /hunt with HuntVerdict + trophy cards (#2092)#2306idling11 wants to merge 5 commits into
Conversation
![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 renames the /goal command to /hunt (and associated structures from GoalState to HuntState), introducing tracking for hunt verdicts (Hunting, Hunted, Wounded, Escaped) and writing trophy cards to disk upon completing, wounding, or escaping a hunt. The reviewer feedback highlights several important improvements: adding checks to ensure an active hunt exists before marking it as complete, wounded, or escaped; handling errors when writing trophy cards instead of silently ignoring them; renaming the goal.rs file and the goal field on the App struct to hunt for consistency; and changing write_trophy_card to take an immutable &App reference.
| Some("done") | Some("complete") | Some("hunted") => { | ||
| let prev = app.goal.verdict; | ||
| app.goal.verdict = HuntVerdict::Hunted; | ||
| let elapsed = app | ||
| .goal | ||
| .goal_started_at | ||
| .started_at | ||
| .map(|t| crate::tui::notifications::humanize_duration(t.elapsed())) | ||
| .unwrap_or_else(|| "unknown".to_string()); | ||
| CommandResult::message(format!("Goal marked complete! Elapsed: {elapsed}")) | ||
| if prev != HuntVerdict::Hunted { | ||
| if let Err(e) = write_trophy_card(app) { | ||
| return CommandResult::error(format!( | ||
| "Hunt complete but trophy write failed: {e}" | ||
| )); | ||
| } | ||
| } | ||
| CommandResult::message(format!("Hunt complete! Elapsed: {elapsed}")) | ||
| } |
There was a problem hiding this comment.
If there is no active hunt (i.e., app.goal.quarry is None), marking a hunt as complete will write a trophy card with the name "untitled" and print a success message. We should guard against this by returning an error if no active hunt exists.
Some("done") | Some("complete") | Some("hunted") => {
if app.goal.quarry.is_none() {
return CommandResult::error("No active hunt to mark complete.");
}
let prev = app.goal.verdict;
app.goal.verdict = HuntVerdict::Hunted;
let elapsed = app
.goal
.started_at
.map(|t| crate::tui::notifications::humanize_duration(t.elapsed()))
.unwrap_or_else(|| "unknown".to_string());
if prev != HuntVerdict::Hunted {
if let Err(e) = write_trophy_card(app) {
return CommandResult::error(format!(
"Hunt complete but trophy write failed: {e}"
));
}
}
CommandResult::message(format!("Hunt complete! Elapsed: {elapsed}"))
}| Some("wound") | Some("wounded") => { | ||
| app.goal.verdict = HuntVerdict::Wounded; | ||
| let _ = write_trophy_card(app); | ||
| CommandResult::message("Hunt wounded — progress saved, can be resumed.") | ||
| } |
There was a problem hiding this comment.
Ignoring the error from write_trophy_card silently using let _ = ... can lead to a poor user experience if the trophy file fails to write (e.g., due to permissions or disk space issues). Additionally, we should ensure there is an active hunt before marking it as wounded.
Some("wound") | Some("wounded") => {
if app.goal.quarry.is_none() {
return CommandResult::error("No active hunt to mark wounded.");
}
app.goal.verdict = HuntVerdict::Wounded;
if let Err(e) = write_trophy_card(app) {
return CommandResult::error(format!(
"Hunt wounded but trophy write failed: {e}"
));
}
CommandResult::message("Hunt wounded — progress saved, can be resumed.")
}| Some("escape") | Some("escaped") => { | ||
| app.goal.verdict = HuntVerdict::Escaped; | ||
| let _ = write_trophy_card(app); | ||
| CommandResult::message("Hunt escaped — quarry abandoned.") | ||
| } |
There was a problem hiding this comment.
Ignoring the error from write_trophy_card silently using let _ = ... can lead to a poor user experience if the trophy file fails to write. Additionally, we should ensure there is an active hunt before marking it as escaped.
Some("escape") | Some("escaped") => {
if app.goal.quarry.is_none() {
return CommandResult::error("No active hunt to mark escaped.");
}
app.goal.verdict = HuntVerdict::Escaped;
if let Err(e) = write_trophy_card(app) {
return CommandResult::error(format!(
"Hunt escaped but trophy write failed: {e}"
));
}
CommandResult::message("Hunt escaped — quarry abandoned.")
}| @@ -1,61 +1,75 @@ | |||
| //! /goal command — set a session objective with token budget and progress tracking. | |||
| //! /hunt command — declare a quarry with token budget and verdict tracking (#2092). | |||
There was a problem hiding this comment.
Since the /goal command has been renamed to /hunt, and the associated data structures have been renamed from GoalState to HuntState, the file goal.rs should be renamed to hunt.rs to maintain consistency and improve project maintainability.
|
|
||
| /// Write a trophy card to `~/.codewhale/trophies/<date>-<slug>.md` for the | ||
| /// current hunt verdict (#2092). Returns the path written on success. | ||
| fn write_trophy_card(app: &mut App) -> Result<std::path::PathBuf, std::io::Error> { |
There was a problem hiding this comment.
The write_trophy_card function only reads from app and does not perform any mutations. It should take an immutable reference app: &App instead of a mutable one app: &mut App to follow Rust best practices and clarify the function's intent.
| fn write_trophy_card(app: &mut App) -> Result<std::path::PathBuf, std::io::Error> { | |
| fn write_trophy_card(app: &App) -> Result<std::path::PathBuf, std::io::Error> { |
| pub viewport: ViewportState, | ||
| /// Goal sub-state. | ||
| pub goal: GoalState, | ||
| pub goal: HuntState, |
There was a problem hiding this comment.
To align with the renaming of GoalState to HuntState and /goal to /hunt, the goal field on the App struct should be renamed to hunt (i.e., pub hunt: HuntState). This will prevent confusion and improve code readability.
| pub goal: HuntState, | |
| pub hunt: HuntState, |
| }, | ||
| viewport: ViewportState::default(), | ||
| goal: GoalState::default(), | ||
| goal: HuntState::default(), |
There was a problem hiding this comment.
Rename the goal field initialization to hunt to match the struct field rename.
| goal: HuntState::default(), | |
| hunt: HuntState::default(), |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| impl Default for HuntVerdict { | ||
| fn default() -> Self { | ||
| Self::Hunting | ||
| } |
There was a problem hiding this comment.
HuntVerdict::default() is Hunting even when no hunt is active
After /hunt clear the code resets verdict to HuntVerdict::default() which is Hunting. This means app.goal.verdict == HuntVerdict::Hunting cannot distinguish "actively hunting" from "no hunt set" — both states look identical. If any future code branches on the verdict without also checking app.goal.quarry.is_some(), it will misread a cleared state as an active one.
| Some("wound") | Some("wounded") => { | ||
| app.hunt.verdict = HuntVerdict::Wounded; | ||
| write_trophy_card(app); | ||
| CommandResult::message("Hunt wounded — progress saved, can be resumed.") | ||
| } | ||
| Some("escape") | Some("escaped") => { | ||
| app.hunt.verdict = HuntVerdict::Escaped; | ||
| write_trophy_card(app); | ||
| CommandResult::message("Hunt escaped — quarry abandoned.") | ||
| } |
There was a problem hiding this comment.
The wounded, escaped, and hunted arms update app.hunt.verdict before calling write_trophy_card, but neither arm checks that app.hunt.quarry is Some first. When called with no active hunt, write_trophy_card returns None immediately (nothing is written), yet the state is left with verdict = Wounded (or Escaped/Hunted) and quarry = None. A subsequent /hunt query then hits the else branch and prints "No hunt set." — contradicting the verdict that was just silently applied. The wounded arm's confirmation message "progress saved, can be resumed." is especially misleading because the write was a no-op and no progress exists to resume.
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
1 participant
Summary
Rename
/goalto/hunt, introduce four verdict states, and generatetrophy cards on completion so interrupted sessions can be recovered.
Closes: #2092
Changes
Core data model
GoalState→HuntState(fields:quarry,token_budget,started_at,verdict)HuntVerdictenum:Hunting,Hunted,Wounded,EscapedCommand
/goal→/hunt(old aliasesgoal/mubiaopreserved)/hunt hunted— mark complete, write trophy card/hunt wounded— mark interrupted (resumable), write trophy card/hunt escaped— mark abandoned, write trophy card/hunt clear— remove current hunt/hunt <quarry> [budget: N]— declare a new huntTrophy cards
~/.codewhale/trophies/<date>-<slug>.mdUpdated files
crates/tui/src/tui/app.rscrates/tui/src/commands/goal.rscrates/tui/src/commands/mod.rscrates/tui/src/tui/ui.rscrates/tui/src/tui/sidebar.rscrates/tui/src/prompts.rsTests
Greptile Summary
This PR renames
/goal→/hunt, introduces theHuntVerdictenum (Hunting,Hunted,Wounded,Escaped), replacesGoalStatewithHuntState, and adds a trophy-card writer that persists hunt outcomes to~/.codewhale/trophies/<date>-<time>-<slug>.md. Old aliases (goal,mubiao) are preserved and a new CJK alias (狩猎) is added.app.rs):GoalState→HuntStatewithquarry,token_budget,started_at,verdict;HuntVerdictgainsserdederives for machine-readable trophy cards.goal.rs): four new verdict arms (hunted/wounded/escaped/clear) plus the trophy-card writer with time-based filenames and dash-collapsing slug logic.ui.rs,sidebar.rs,prompts.rs,mod.rs): field accesses and system-prompt heading updated throughout; relay instruction labels inmod.rsstill use the old "Goal:" / "Goal token budget:" strings.Confidence Score: 3/5
The rename is mechanically sound, but several issues flagged in prior review rounds remain unaddressed in goal.rs.
The trophy-card writer silently swallows write errors on the wounded and escaped paths while still telling the user progress saved, can be resumed — a direct contradiction. The verdict mutation arms also do not guard against an absent quarry, leaving the app in an inconsistent state. These functional gaps in the new code paths warrant a second look before merging.
crates/tui/src/commands/goal.rs — the wounded/escaped verdict arms and write_trophy_card error handling need attention before this is production-ready.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A["/hunt <quarry>"] -->|sets quarry + verdict=Hunting| B[HuntState: Hunting] B -->|/hunt hunted| C[HuntState: Hunted] B -->|/hunt wounded| D[HuntState: Wounded] B -->|/hunt escaped| E[HuntState: Escaped] C -->|write_trophy_card| F[trophy .md file] D -->|write_trophy_card| F E -->|write_trophy_card| F C -->|/hunt clear| G[HuntState: reset] D -->|/hunt clear| G E -->|/hunt clear| G G -->|verdict = Hunting default| B B -->|/hunt <quarry> again| BReviews (3): Last reviewed commit: "fix: remove unused HuntVerdict import in..." | Re-trigger Greptile