GLM-review.md

ralph-rs Code Review: Bugs & Semantic Abnormalities

Generated by deep audit across all source files — April 2026

---

Critical / High Severity

H1. session_id parsed from harness output but never persisted

• Files: executor.rs:60,104-107,373, storage.rs:477
• ParsedHarnessOutput extracts session_id from harness JSON output. The DB schema has a session_id column. create_execution_log accepts session_id, but update_execution_log has no session_id parameter. On line 373 of executor.rs, create_execution_log is called with session_id: None. Result: session IDs are always lost — confirmed data loss.

H2. resume_plan does NOT reset Aborted steps

• File: runner.rs:617-618
• find_resume_point correctly returns steps with StepStatus::Aborted, but the reset condition only covers Failed and InProgress. An Aborted step at the resume point will NOT be reset to Pending. When execute_step encounters it, it will either bail (because attempts >= max_attempts) or proceed with stale state.

H3. Path traversal vulnerability in agent name / hook name handling

• Files: commands/agents.rs:74,93,145, hook_library.rs:115-117,344-392
• agents_dir().join(format!("{name}.md")) and hooks_dir()?.join(format!("{name}.md")) accept unvalidated user input. A name like ../../etc/shadow would resolve outside the target directory, enabling arbitrary file read, write, or delete.

H4. status_indicator() returns identical placeholder for all statuses

• File: tui/app.rs:186-195
• Every StepStatus variant returns " " (two spaces). The corresponding status_icon() in output.rs:126-141 correctly returns distinct Unicode icons (○, ▶, ✔, ✘, ⊘). The TUI step list shows no status iconography — every step looks identical before the number/title.

H5. Ctrl+C not handled in TUI AddStep input mode

• File: tui/input.rs:107
• KeyCode::Char('c') in handle_add_mode() unconditionally pushes 'c' to the input buffer. The Ctrl+C handler exists only in handle_normal_mode(). Users must press Esc, then q/Ctrl+C to quit from input mode.

H6. No transaction wrapping for import — partial state on failure

• File: import.rs:90-157
• import_plan_from_data() creates a plan, updates status, creates steps in a loop, and attaches dependencies — all as separate operations. If any step creation or dependency attachment fails partway, the database is left with a partial plan in Ready status with incomplete steps. No rollback mechanism.

H7. remove_agent_file_args can corrupt command line if prompt contains {agent_file}

• File: harness.rs:153-172
• {prompt} replacement happens before remove_agent_file_args. If the prompt text contains the literal string {agent_file}, the function will find it and incorrectly remove the preceding argument (e.g., the -p flag), silently corrupting the harness command line.

H8. Hook library comment stripping breaks values containing #

• File: hook_library.rs:163-166
• The frontmatter parser strips everything from the first # to end of line. Values like description: "Fix issue #123" become description: "Fix issue . Since # is extremely common in issue references and URLs, this is a significant parsing bug.

---

Medium Severity

M1. Timeout path doesn't reap child process — potential zombie

• File: executor.rs:681
• After child.kill(), the code reads stdout/stderr but never calls child.wait(). On Unix, the killed process may not be reaped, potentially leaving a zombie. The graceful_shutdown path correctly calls child.wait(), but the timeout path does not.

M2. run_tests() is synchronous, blocking the tokio runtime

• File: test_runner.rs:47
• run_tests() uses std::process::Command::output() which blocks the current thread. During test execution, the tokio runtime cannot process other async tasks (including abort_rx signal handling), so Ctrl+C during test execution won't be detected until after tests finish.

M3. Synchronous checkout_existing_branch in async context

• File: runner.rs:756-769
• Uses std::process::Command (synchronous) while called from the async run_plan call chain. This blocks the tokio runtime thread for the duration of the git checkout. All other git operations in git.rs are also synchronous but called from non-async contexts.

M4. Abort before execution log creation leaves inconsistent state

• File: executor.rs:327-337
• When abort is detected at this point, attempt has been incremented but set_step_attempts has NOT been called. The StepResult reports attempts_used = step.attempts + 1, but the database still shows step.attempts. No execution log entry is created for this aborted attempt.

M5. resume and skip commands lack run lock

• Files: main.rs:490-512,515-524
• ralph resume and ralph skip modify step status without acquiring a run lock. Concurrent invocations of ralph run and ralph skip could race on step state.

M6. Plan::from_row column index mapping is inverted vs. natural table order

• File: plan.rs:154-189
• from_row expects plan_harness at position 9 and created_at at position 10, but ALTER TABLE ADD COLUMN appends plan_harness at position 11 (after updated_at). This works today only because every SQL query explicitly reorders columns. A future SELECT * would silently corrupt data.

M7. Runtime panic in suffix_between for edge-case keys

• File: frac_index.rs:120-123
• key_between("0", "00") panics because the suffix recursion produces an empty b_suffix. Input keys like this could arise from import/export. Should return an error instead of panicking.

M8. No UNIQUE constraint on (plan_id, step_id, lifecycle, hook_name) in step_hooks

• File: db.rs:192-206
• Allows attaching the same hook to the same step/lifecycle multiple times, causing duplicate hook execution.

M9. No validation that default_harness exists in harnesses map

• File: config.rs:72-81
• A config with default_harness: "foobar" that doesn't exist in the harnesses map loads successfully but fails at runtime with "Unknown harness" for every operation.

M10. std::process::exit(130) on second Ctrl+C skips all destructors

• File: signal.rs:97
• The RunLock::drop never runs, leaving a stale row in run_locks. Users must use --force or manually clean up the DB row on next run.

M11. SQL injection in user-facing error message

• File: run_lock.rs:121
• The project path is interpolated directly into a suggested DELETE FROM run_locks WHERE project = '{project}' SQL command. If the path contains a single quote (e.g., /tmp/o'brien/project), the suggested command is broken. More concerning, blind copy-paste could be destructive.

M12. Race condition in run lock acquisition

• File: run_lock.rs:113-133
• The acquire sequence (query PID → check liveness → delete → insert) is non-atomic. Two processes could both pass the PID check and insert their own rows concurrently. Should use an explicit SQLite transaction.

M13. yaml_escape doesn't escape newlines in double-quoted strings

• File: hook_library.rs:294-302
• Literal newlines inside descriptions will break the YAML frontmatter when round-tripped through serialize_hook → parse_hook. YAML requires \n escape sequences inside double-quoted strings.

M14. Frontmatter delimiter collision with body content

• File: hook_library.rs:148-150
• rest.find("\n---") matches \n--- anywhere, including in diff output (--- a/old_file). The closing frontmatter delimiter should require --- at column 0 followed by end-of-line.

M15. Hardcoded 'pending' string in reset_step SQL

• File: storage.rs:400
• Every other status update uses status.as_str(). If the StepStatus::Pending string representation ever changes, this query will insert an unparseable value, silently corrupting step state.

M16. update_step_fields doesn't check affected == 0 for missing step

• File: storage.rs:306-325
• All comparable update functions bail with "not found" when affected == 0, but this one silently succeeds on a nonexistent step ID.

M17. update_step_fields_ext issues up to 7 separate non-atomic SQL UPDATEs

• File: storage.rs:340-386
• Each UPDATE sets its own updated_at = strftime(...), leading to non-atomic partial updates and inconsistent timestamps if any statement fails mid-way.

M18. get_all_changed_files mangles renamed/copied file entries

• File: git.rs:83-94
• git status --porcelain outputs renames as XY old_path -> new_path. The code strips the first 3 characters with l.get(3..), returning old_path -> new_path as a single string — an invalid file path.

M19. get_diff silently swallows git errors, returning empty diff

• File: git.rs:97-114
• Both git diff and git diff --cached use .unwrap_or_default(). A corrupt repository or lock contention produces an empty diff, masking real problems — the executor would think there are no changes.

M20. Hook commands run via sh -c with env vars containing shell metacharacters

• File: hooks.rs:73-74
• Environment variables like RALPH_PROJECT_DIR are set via cmd.env() (safe), but if a hook command references them via $RALPH_PROJECT_DIR, shell expansion processes them. A project path like /tmp/$(malicious)/proj would be expanded.

M21. No timeout on hook execution

• File: hooks.rs:80-83
• cmd.output() blocks indefinitely. A hanging hook blocks the entire ralph run with no timeout mechanism.

M22. print_report() always uses ANSI colors, ignores --no-color

• File: preflight.rs:61-70
• Hardcoded ANSI escape codes (\x1b[32m) are always emitted, bypassing the OutputContext.color system. With --no-color or NO_COLOR, the preflight report still contains escape codes.

M23. Cannot specify plan slug with --import-json on step add

• File: cli.rs:398-444
• The title positional arg conflicts with --import-json, so ralph step add --import-json file.json my-plan fails because "my-plan" is parsed as title. There's no way to target a non-active plan with bulk import.

M24. plan harness generate discards plan parameter

• File: main.rs:170-171
• The plan field is explicitly ignored (let _ = plan;). Users targeting a specific plan will see it silently ignored.

M25. build_plan_harness_args panics on unknown harness via index access

• File: plan_harness.rs:161,218
• &config.harnesses[harness_name] will panic if the key doesn't exist. Should use get() with explicit error handling.

---

Low Severity

L1. find_active_plan loads all plans then filters in Rust

• File: storage.rs:76-88
• Should use a SQL WHERE status IN (...) query instead of fetching all plans and filtering in memory.

L2. Silent truncation to 100 rows in list_execution_logs_for_plan

• File: storage.rs:551
• When limit is None, the function silently caps results at 100 with no way for the caller to know results were truncated.

L3. FailureReason::NoChanges is dead code

• File: executor.rs:124-126
• Marked #[allow(dead_code)]. When no changes are detected, the code uses FailureReason::TestFailed instead.

L4. extract_changed_files_from_diff misses deleted and renamed files

• File: executor.rs:862-873
• Only extracts +++ b/ lines. Deleted files and old names in renames are lost, potentially missing relevant context for the AI agent.

L5. build_prior_step_summaries excludes Failed/Aborted steps

• File: executor.rs:835-836
• Steps that failed (and were rolled back) could provide useful "here's what didn't work" context, but they're excluded.

L6. skip_step _reason parameter is unused

• File: runner.rs:646
• Accepted but never stored or displayed — dead code suggesting an incomplete feature.

L7. run_plan double-counts already-completed steps in steps_succeeded

• File: runner.rs:154-156
• Steps that were complete before this run started are counted as "succeeded" in the result, inflating the metric.

L8. dry_run_report returns current plan status as final_status

• File: runner.rs:898
• For a Ready plan, final_status will be Ready, which is semantically misleading for a "run result."

L9. run_all_plans stops on InProgress status

• File: runner.rs:573-593
• Independent plans that don't depend on an in-progress plan won't get a chance to run.

L10. setup_branch auto-commits ALL dirty state including untracked files

• File: runner.rs:730-737
• git add -A && git commit stages and commits everything. No preview or opt-out for secrets, build artifacts, etc.

L11. Misleading --plan warning text

• File: main.rs:367-368
• Warning says --plan is ignored when --all is set, but there is no --plan flag — it's a positional argument.

L12. _config variable named with underscore prefix but widely used

• File: main.rs:64
• Violates Rust naming convention — suggests unused but is referenced 9+ times.

L13. Init CLI accepts --slug and --branch flags that are silently discarded

• File: cli.rs:44-49, main.rs:80-93
• Users can pass these flags but they do nothing.

L14. --one and --all not mutually exclusive at the CLI level

• File: cli.rs:86-87
• No conflicts_with or group constraint. The code gives --all priority, but only warns about --plan.

L15. Inconsistency: PlanDependencyCommand::Remove missing required = true

• File: cli.rs:366-369
• Add has required = true on depends_on, Remove does not. The runtime error message is identical, but the timing of detection differs.

L16. Inconsistent "no plan" JSON output across commands

• Files: commands/run.rs:148-155, commands/run.rs:29-38
• cmd_status (JSON) prints "null". cmd_log (JSON) prints nothing. Should be consistent.

L17. tail_lines trailing newline inconsistency

• File: test_runner.rs:119-126
• When text fits within the limit, the original (possibly with trailing newline) is preserved. When truncated, join("\n") produces no trailing newline.

L18. char_index panics on invalid characters

• File: frac_index.rs:10-17
• Invalid characters in sort keys cause a runtime panic. Should return a Result.

L19. Missing Archived variant from PlanStatus roundtrip test

• File: plan.rs:357-370
• The Archived variant's serialization roundtrip is never tested.

L20. No mutual exclusivity check between rolled_back and committed

• File: plan.rs:315-316
• Both could be true simultaneously (violating business logic) with no assertion.

L21. CURRENT_VERSION could drift from MIGRATIONS.len()

• File: db.rs:12-19
• Uses const rather than computing from MIGRATIONS.len(). If a migration is added without updating the constant, tests fail but production behavior is subtle.

L22. TOCTOU race in load_or_create_config

• File: config.rs:356-375
• Between path.exists() and fs::write(), another process could create the file. Benign in practice since both write the same default config.

L23. timeout_secs: 0 meaning "no timeout" is semantically ambiguous

• File: config.rs:311
• Zero seconds could reasonably be read as "immediate timeout." Should use Option<u64> instead.

L24. No restrictive file permissions on database file

• File: db.rs:28-35
• The database is created with default permissions, potentially readable by other users on shared systems.

L25. rollback_except TOCTOU race between listing and deleting untracked files

• File: git.rs:164-176
• Between get_untracked_files() and deletion, the filesystem can change.

L26. stage_except silently ignores git reset HEAD errors

• File: git.rs:145
• let _ = git(...) discards all errors including permission denied and lock contention.

L27. git checkout -- . is a deprecated pattern

• File: git.rs:121
• Modern git (2.23+) recommends git restore . instead.

L28. TUI ListState recreated every frame

• File: tui/ui.rs:75-77
• Causes viewport "jump" on each render. Should be stored in App and reused.

L29. Hardcoded default max_retries of 3 in TUI detail panel

• File: tui/ui.rs:136
• Doesn't respect user's Config.max_retries_per_step setting.

L30. confirm() docstring overstates accepted inputs

• File: output.rs:255-256
• Claims "and similar" but only accepts y, Y, yes, Yes, YES — not mixed case like yEs.

L31. emit_ndjson() silently swallows serialization errors

• File: output.rs:45-51
• For a machine-readable stream, a serialization error should be fatal to avoid corrupt output.

L32. LogEntrySummary From<&ExecutionLog> always omits stdout/stderr

• File: output.rs:423-443
• If any code path uses .into(), log output will silently be lost.

L33. Release closure reopens database instead of reusing connection

• File: run_lock.rs:73-84
• Opens a brand-new Connection for lock release, which could fail if the DB is temporarily unavailable.

L34. PID reuse can produce false "lock held" conditions

• File: run_lock.rs:114
• If a crashed ralph process's PID is reused by an unrelated process, pid_is_alive returns true and incorrectly refuses the lock.

L35. Global FIRST_SIGNAL AtomicBool shared across concurrent tests

• File: signal.rs:38,47
• One test's new() reset could race with another test's swap under --threads > 1.

L36. spawn_signal_listener consumes self, preventing programmatic abort

• File: signal.rs:69
• After spawning, there's no way to trigger graceful shutdown from application code.

L37. Hook import partial success with error exit code

• File: commands/hooks.rs:178-208
• When some hooks collide, successfully imported hooks are on disk but the command returns a non-zero exit code.

L38. --archived silently ignored when --status is also set

• File: commands/plan.rs:210-219
• The status filter takes precedence entirely, making --archived a no-op in combination with --status.

L39. truncate_text keeps the tail rather than the head

• File: prompt.rs:228-237
• For diffs and test output, the most informative context is typically at the top. Callers may lose critical header information.

L40. format_prior_steps uses local slice numbering instead of plan step numbers

• File: prompt.rs:153-183
• Steps 3, 7, and 10 would be labeled "Step 1", "Step 2", "Step 3" instead of their actual numbers.

L41. Agent file scaffold has incorrect whitespace

• File: commands/agents.rs:113-141
• format! macro with \ line continuations includes 9 spaces of source-code indentation in each output line.

L42. empty_string slug silently treated as None

• File: main.rs:48
• slug.filter(|s| !s.is_empty()) means ralph status "" silently falls back to active plan lookup instead of producing an error.

L43. --lines applies per-stream, not total

• File: commands/run.rs:253-284
• --lines 50 could produce up to 100 lines (50 stdout + 50 stderr), which may surprise users expecting a total cap.

L44. Hook commands output only to stderr

• Files: commands/plan.rs:452-487, commands/step.rs:501-557
• ralph plan hooks my-plan | grep step produces no output because all text goes to stderr.

L45. check_harness_auth() ignores harness config, hardcoded to copilot

• File: preflight.rs:164
• Custom harness auth requirements are never detected.

L46. Unknown harness name silently skips auth check in preflight

• File: preflight.rs:98-101
• If the harness name doesn't exist in config.harnesses, the auth check is silently skipped with no warning.

L47. No validation (or version check) on import

• File: import.rs:90-157
• ralph_rs_version is included in exports but never checked on import. Future schema changes could silently drop fields.

L48. --harness global flag overrides more-specific per-command/per-step flags

• Files: main.rs:348, cli.rs:80
• ralph --harness A run --harness B uses harness A, not B. Counterintuitive — more-specific flags should override global ones.

L49. hook_library::load_all errors silently swallowed

• File: hooks.rs:41
• unwrap_or_default() converts permission errors, I/O failures, and corrupt files into an empty vector, indistinguishable from "no hooks configured."

L50. Post-lifecycle hooks silently swallow all errors

• File: hooks.rs:161-183,207-232
• Database errors, permission errors, and all other failures produce the same "Warning" output. Critical infrastructure problems go unnoticed.

L51. update_step_status doc comment claims it bumps attempts, but it doesn't

• File: storage.rs:248
• Misleading comment — the attempt bumping is done separately by set_step_attempts.

L52. Missing index verification for V2/V3 indexes in test

• File: db.rs:301-323
• Only V1 indexes are verified. V2/V3 index regressions would not be caught.

---

Summary Statistics

Severity	Count
High	8
Medium	25
Low	52
Total	85

Top Priority Fixes

1. H2 — Add StepStatus::Aborted to the reset condition in resume_plan
2. H1 — Add session_id parameter to update_execution_log or remove the dead column
3. H3 — Validate agent/hook names reject path separators (/, \, ..)
4. H4 — Implement proper status indicators in TUI matching output.rs icons
5. H5 — Handle Ctrl+C in handle_add_mode by checking KeyModifiers::CONTROL
6. H6 — Wrap import operations in a SQLite transaction
7. H7 — Escape or check for {agent_file} in prompt text before arg processing
8. H8 — Fix frontmatter comment parser to only strip # outside quoted values