automation: scaffold issue #30 orchestration state machine + run ledger plan - #54
Conversation
|
Readiness driver check: all required CI/CodeRabbit checks are green, but this PR currently has 0 changed files vs main and cannot satisfy issue #30 implementation scope yet. Exact blocker: no code/docs diff to review + required approving review gate remains. |
|
Added first-pass issue #30 scaffold in commit 58bce31:\n- Introduced SDLC state enum + transition validation ()\n- Added durable run-ledger record model under \n- Added load/save/transition helpers and state-module tests (round-trip + invalid-transition guard)\n\nValidation: running 0 tests test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 1 filtered out; finished in 0.00s passed locally.\n\nThis turns PR #54 into a concrete implementation branch; next increment is wiring these transitions into orchestration execution and resume/recovery flows. |
|
Correction (prior comment had shell-escaped formatting issues): Added first-pass issue #30 scaffold in commit 58bce31:
Validation: cargo test -q state:: passed locally. This turns PR #54 into a concrete implementation branch; next increment is wiring these transitions into orchestration execution and resume/recovery flows. |
|
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:
📝 WalkthroughWalkthroughAdds a persisted SDLC run state machine, filesystem-backed run ledger with serialized transitions via file locks, run/step ID validation to prevent path traversal, public ledger save/load/transition APIs, and tests; ledger JSON stored under Changes
Sequence DiagramsequenceDiagram
participant Caller
participant LedgerOps as Ledger Ops
participant StateMachine as State Machine
participant FS as File System
Caller->>LedgerOps: transition_sdlc_run_ledger(run_id, next, actor, reason)
LedgerOps->>FS: with_run_ledger_lock(run_id) -> load_sdlc_run_ledger(run_id)
FS-->>LedgerOps: ledger (optional)
LedgerOps->>StateMachine: current_state.can_transition_to(next)
StateMachine-->>LedgerOps: allowed / denied
alt allowed
LedgerOps->>LedgerOps: create SdlcTransitionRecord (timestamp, actor, reason)
LedgerOps->>LedgerOps: append transition, update state, metadata
LedgerOps->>FS: save_sdlc_run_ledger(updated_ledger)
FS-->>LedgerOps: path
LedgerOps-->>Caller: updated SdlcRunLedgerRecord
else denied
LedgerOps-->>Caller: Error (invalid transition)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/state/mod.rs (1)
946-1007: Tests are a solid start; add one retry/idempotency case.Please add a test that replays the same transition (or same target state) to lock in deterministic resume behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/state/mod.rs` around lines 946 - 1007, Add a new unit test that verifies idempotency by creating and saving an SdlcRunLedgerRecord (use save_sdlc_run_ledger and SdlcRunState::Selected), then call transition_sdlc_run_ledger to move it to SdlcRunState::Branched and assert state and transitions length, then call transition_sdlc_run_ledger again with the same target state/actor/message and assert the call succeeds but does not append a duplicate transition (state remains Branched and transitions.len() remains unchanged, e.g. 1); ensure you mirror existing tests' temp dir setup/teardown and use the same identifiers (run_id like "run-ledger-idempotent") so the test is deterministic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/state/mod.rs`:
- Around line 411-412: The code constructs filesystem paths using ledger.run_id
(seen where path = dir.join(format!("{}.json", ledger.run_id)) and the similar
usage later) which permits path traversal; validate and sanitize ledger.run_id
before using it in path construction by rejecting or normalizing any value
containing path separators, backslashes, NUL, or path-segment tokens like "..",
and restrict it to a safe whitelist (e.g., alphanumerics, hyphen, underscore) or
map it to a generated safe filename (hash/UUID) if it doesn't match; update both
occurrences that join ledger.run_id to use the sanitized/validated value and
return an error when validation fails.
- Around line 443-466: The load->mutate->save sequence in update transitions is
racy; wrap the change in a concurrency-safe update by either (A) implementing an
optimistic concurrency check/retry: read with load_sdlc_run_ledger, record a
stable marker (e.g., ledger.updated_at or add a numeric ledger.version), perform
mutation (push SdlcTransitionRecord, set ledger.state/updated_at/actor,
increment version) then before calling save_sdlc_run_ledger verify the marker is
unchanged and retry or return a conflict if it changed, or (B) use a
process-level/file-level lock around load_sdlc_run_ledger..save_sdlc_run_ledger
to serialize transitions; update save_sdlc_run_ledger and the ledger struct (add
version if using optimistic locking) and make update logic in the function that
performs the push/assigns state (the block that constructs SdlcTransitionRecord)
retry or fail on concurrent modification.
- Line 90: The method can_transition_to currently declared as pub fn
can_transition_to(&self, next: &SdlcRunState) -> bool should be made non-public
to comply with the rule that public functions return Result; change its
signature to fn can_transition_to(&self, next: &SdlcRunState) -> bool (remove
pub) and update any call sites within the crate to use the private method (or,
if it must remain public, change the return type to Result<bool, TuttiError> and
propagate/construct an appropriate TuttiError); locate the function by the name
can_transition_to and the SdlcRunState type in src/state/mod.rs when making the
change.
- Around line 447-452: The transition check currently rejects retries by
returning Err(TuttiError::State(...)) when previous.can_transition_to(&next) is
false; add an idempotent shortcut before that check: if previous == next {
return Ok(()); } so that retrying the same state is treated as a no-op success;
update the function that calls previous.can_transition_to(&next) to perform this
equality check and return Ok instead of an error.
---
Nitpick comments:
In `@src/state/mod.rs`:
- Around line 946-1007: Add a new unit test that verifies idempotency by
creating and saving an SdlcRunLedgerRecord (use save_sdlc_run_ledger and
SdlcRunState::Selected), then call transition_sdlc_run_ledger to move it to
SdlcRunState::Branched and assert state and transitions length, then call
transition_sdlc_run_ledger again with the same target state/actor/message and
assert the call succeeds but does not append a duplicate transition (state
remains Branched and transitions.len() remains unchanged, e.g. 1); ensure you
mirror existing tests' temp dir setup/teardown and use the same identifiers
(run_id like "run-ledger-idempotent") so the test is deterministic.
|
Addressed the actionable CodeRabbit blockers in commit :\n\n- tightened SDLC transition API surface by making \ internal-only\n- added run_id validation across workflow checkpoint/output/intent + run-ledger paths (blocks traversal segments like )\n- added a run-ledger transition lock to avoid concurrent lost updates\n- made transition retries idempotent when next state equals current state\n- added regression tests for idempotent retry + run_id validation\n\nValidation: \ and running 1 test |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/state/mod.rs (1)
399-416:⚠️ Potential issue | 🔴 CriticalValidate
step_idbefore using it in file paths (path traversal risk).
step_idis still interpolated into"{step_id}.json"without validation in both save/load intent/output flows. A craftedstep_idcontaining separators or..can escape the intended run directory.Proposed fix
+fn validate_step_id(step_id: &str) -> Result<()> { + if step_id.is_empty() + || step_id.contains('/') + || step_id.contains('\\') + || step_id.contains("..") + { + return Err(TuttiError::State(format!( + "invalid step_id '{step_id}': must not contain path separators or traversal segments" + ))); + } + Ok(()) +} pub fn save_workflow_output( project_root: &Path, run_id: &str, step_id: &str, json: &serde_json::Value, ) -> Result<PathBuf> { validate_run_id(run_id)?; + validate_step_id(step_id)?; @@ pub fn save_workflow_intent( project_root: &Path, run_id: &str, step_id: &str, record: &WorkflowStepIntentRecord, ) -> Result<PathBuf> { validate_run_id(run_id)?; + validate_step_id(step_id)?; @@ pub fn load_workflow_intent( project_root: &Path, run_id: &str, step_id: &str, ) -> Result<Option<WorkflowStepIntentRecord>> { validate_run_id(run_id)?; + validate_step_id(step_id)?;Also applies to: 528-565
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/state/mod.rs` around lines 399 - 416, Validate and reject or sanitize the step_id before interpolating it into filenames in save_workflow_output (and the corresponding load/save intent/output functions referenced in the comment) to prevent path traversal: ensure step_id is non-empty, not absolute, does not contain path separators or components like ".." (e.g. verify Path::new(step_id).file_name() yields the same string and contains no "/" or "\" and is valid UTF-8), and return an error if it fails validation; only then construct the path with format!("{step_id}.json") and continue with create_dir_all/write/read operations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/state/mod.rs`:
- Around line 498-503: The code currently mutates ledger metadata when previous
== next; change the early-return path in the function that handles the SDLC run
ledger so it does NOT modify ledger.updated_at or ledger.actor or call
save_sdlc_run_ledger in that case. Instead, detect the no-op transition
(previous == next) and return Ok(ledger) immediately without persisting any
changes, keeping the existing ledger record immutable for idempotent retries;
references: the previous, next, ledger variables and the save_sdlc_run_ledger
call.
- Around line 114-145: with_run_ledger_lock currently creates a
".transition.lock" file but doesn't recover stale locks or guarantee removal on
panic; change it to create/write a lock file containing the current PID and
timestamp, attempt to acquire the lock using create_new as now, and when
encountering AlreadyExists inspect the lock (metadata or contents) to decide
eviction: if the file's mtime is older than a configurable threshold (e.g.,
several seconds) or the PID in the file is no longer a live process, remove the
stale lock and retry; also introduce a RAII guard type (e.g.,
RunLedgerLockGuard) that holds lock_path and removes the file in Drop to ensure
unlock on normal exit or panic, and update with_run_ledger_lock to return/hold
that guard while running op so the lock is always cleaned up and eviction errors
are handled and propagated.
---
Outside diff comments:
In `@src/state/mod.rs`:
- Around line 399-416: Validate and reject or sanitize the step_id before
interpolating it into filenames in save_workflow_output (and the corresponding
load/save intent/output functions referenced in the comment) to prevent path
traversal: ensure step_id is non-empty, not absolute, does not contain path
separators or components like ".." (e.g. verify Path::new(step_id).file_name()
yields the same string and contains no "/" or "\" and is valid UTF-8), and
return an error if it fails validation; only then construct the path with
format!("{step_id}.json") and continue with create_dir_all/write/read
operations.
|
Addressed remaining actionable CodeRabbit blockers in commit
Validation (local): |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/state/mod.rs (1)
421-421:⚠️ Potential issue | 🔴 CriticalSanitize
step_idbefore path joins to close traversal gaps.
run_idis validated now, butstep_idis still interpolated into filenames directly in both workflow outputs and intents. A craftedstep_id(e.g., containing separators) can escape the intended run directory.🔧 Proposed fix
+fn validate_path_component(kind: &str, value: &str) -> Result<()> { + if value.is_empty() + || value.contains('/') + || value.contains('\\') + || value.contains("..") + { + return Err(TuttiError::State(format!( + "invalid {kind} '{value}': must not contain path separators or traversal segments" + ))); + } + Ok(()) +} + pub fn save_workflow_output( project_root: &Path, run_id: &str, step_id: &str, json: &serde_json::Value, ) -> Result<PathBuf> { validate_run_id(run_id)?; + validate_path_component("step_id", step_id)?; @@ pub fn save_workflow_intent( project_root: &Path, run_id: &str, step_id: &str, record: &WorkflowStepIntentRecord, ) -> Result<PathBuf> { validate_run_id(run_id)?; + validate_path_component("step_id", step_id)?; @@ pub fn load_workflow_intent( project_root: &Path, run_id: &str, step_id: &str, ) -> Result<Option<WorkflowStepIntentRecord>> { validate_run_id(run_id)?; + validate_path_component("step_id", step_id)?;Also applies to: 547-547, 564-564
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/state/mod.rs` at line 421, The filename interpolation uses untrusted step_id in dir.join(format!("{step_id}.json")), which allows path traversal; sanitize and validate step_id before joining by rejecting or normalizing any path separators or traversal sequences (e.g., '/', '\', "..") and restricting to an allowed character set (or replacing unsafe characters with a safe encoding), ensure it is non-empty, then use the sanitized value in dir.join; apply the same fix for every occurrence where step_id is used to build a path (the expressions using dir.join(format!("{step_id}.json")) and similar uses at the other locations), and add unit tests to assert that inputs with separators are rejected or sanitized.
♻️ Duplicate comments (1)
src/state/mod.rs (1)
152-154:⚠️ Potential issue | 🟠 MajorMake lock release panic-safe with a Drop guard.
Lock removal is manual after
op(). Ifop()unwinds, the lock file is left behind until stale-lock eviction, which can cause avoidable transition failures.🔒 Proposed fix
+struct RunLedgerLockGuard { + path: PathBuf, +} + +impl Drop for RunLedgerLockGuard { + fn drop(&mut self) { + let _ = std::fs::remove_file(&self.path); + } +} + fn with_run_ledger_lock<T>(project_root: &Path, op: impl FnOnce() -> Result<T>) -> Result<T> { @@ - let result = op(); - let _ = std::fs::remove_file(lock_path); - result + let _guard = RunLedgerLockGuard { + path: lock_path.clone(), + }; + op() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/state/mod.rs` around lines 152 - 154, The lock file is removed manually after calling op(), so a panic during op() will leave the lock file behind; introduce an RAII guard (e.g., a small struct LockFileGuard that owns lock_path and implements Drop) and create it before calling op() so its Drop removes the lock via std::fs::remove_file (ignore/remove errors as before); then call op() normally and let the guard automatically clean up on both normal return and unwinding. Ensure the guard is created in the same scope where op() is invoked and that no extra moves prevent Drop from running.
🧹 Nitpick comments (1)
src/state/mod.rs (1)
1112-1120: Broaden traversal regression tests to all touched persistence APIs.The current test only asserts one
run_identry point. Since validation was wired into multiple save/load functions, add a small table-driven test over all of them to prevent partial regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/state/mod.rs` around lines 1112 - 1120, Extend the existing run_id_with_path_segments_is_rejected test into a table-driven test that iterates over every persistence API that validates run_id (e.g., load_workflow_checkpoint, save_workflow_checkpoint, load_runner_state, save_runner_state, and any load/save task/checkpoint functions) instead of asserting only load_workflow_checkpoint; keep the same setup using ensure_tutti_dir and the temp dir, call each API variant with the path segment "../escape", and assert each returned Err contains "invalid run_id" so all save/load entry points are covered and prevent partial regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/state/mod.rs`:
- Line 421: The filename interpolation uses untrusted step_id in
dir.join(format!("{step_id}.json")), which allows path traversal; sanitize and
validate step_id before joining by rejecting or normalizing any path separators
or traversal sequences (e.g., '/', '\', "..") and restricting to an allowed
character set (or replacing unsafe characters with a safe encoding), ensure it
is non-empty, then use the sanitized value in dir.join; apply the same fix for
every occurrence where step_id is used to build a path (the expressions using
dir.join(format!("{step_id}.json")) and similar uses at the other locations),
and add unit tests to assert that inputs with separators are rejected or
sanitized.
---
Duplicate comments:
In `@src/state/mod.rs`:
- Around line 152-154: The lock file is removed manually after calling op(), so
a panic during op() will leave the lock file behind; introduce an RAII guard
(e.g., a small struct LockFileGuard that owns lock_path and implements Drop) and
create it before calling op() so its Drop removes the lock via
std::fs::remove_file (ignore/remove errors as before); then call op() normally
and let the guard automatically clean up on both normal return and unwinding.
Ensure the guard is created in the same scope where op() is invoked and that no
extra moves prevent Drop from running.
---
Nitpick comments:
In `@src/state/mod.rs`:
- Around line 1112-1120: Extend the existing
run_id_with_path_segments_is_rejected test into a table-driven test that
iterates over every persistence API that validates run_id (e.g.,
load_workflow_checkpoint, save_workflow_checkpoint, load_runner_state,
save_runner_state, and any load/save task/checkpoint functions) instead of
asserting only load_workflow_checkpoint; keep the same setup using
ensure_tutti_dir and the temp dir, call each API variant with the path segment
"../escape", and assert each returned Err contains "invalid run_id" so all
save/load entry points are covered and prevent partial regressions.
|
Addressed the remaining actionable CodeRabbit blocker in commit :\n- Added and enforced it in , , and to block path traversal via step IDs.\n- Added regression test .\n\nValidation: and running 0 tests test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 1 filtered out; finished in 0.00s passed locally.\n\n@coderabbitai review |
|
Correction (previous comment had shell-escaped formatting issues): Addressed the remaining actionable CodeRabbit blocker in commit 37c98c1.
Validation: cargo fmt and cargo test -q state:: passed locally. @coderabbitai review |
|
✅ Actions performedReview triggered.
|
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/state/mod.rs (1)
127-168:⚠️ Potential issue | 🟠 MajorUse a Drop guard so lock cleanup is panic-safe.
with_run_ledger_lockcurrently relies on explicit cleanup afterop(). Ifop()unwinds,.transition.lockmay remain until stale-lock eviction kicks in, causing avoidable transition failures.🔧 Proposed fix
+struct RunLedgerLockGuard { + path: PathBuf, +} + +impl Drop for RunLedgerLockGuard { + fn drop(&mut self) { + let _ = std::fs::remove_file(&self.path); + } +} + fn with_run_ledger_lock<T>(project_root: &Path, op: impl FnOnce() -> Result<T>) -> Result<T> { @@ - let result = op(); - let _ = std::fs::remove_file(lock_path); - result + let _guard = RunLedgerLockGuard { path: lock_path }; + op() }#!/bin/bash # Verify whether lock cleanup is guarded by Drop (panic-safe) or manual only. rg -n -C3 'with_run_ledger_lock|transition\.lock|impl Drop|remove_file\(lock_path\)' src/state/mod.rsExpected result: if cleanup is only
remove_file(lock_path)afterop()and no guard tied to lock lifetime, this concern is confirmed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/state/mod.rs` around lines 127 - 168, Replace the manual post-op remove_file with a panic-safe Drop guard: after successfully creating the lock file in with_run_ledger_lock (the OpenOptions create_new branch), construct a LockGuard (e.g., struct LockGuard { path: PathBuf }) that owns lock_path and impl Drop for LockGuard to remove_file(&self.path) ignoring errors; keep the guard alive for the duration of op() so the file is removed even if op() panics/unwinds, and remove the explicit std::fs::remove_file(lock_path) call at the end of with_run_ledger_lock.
🧹 Nitpick comments (1)
src/state/mod.rs (1)
269-269: Add an assertion forstate/run-ledgerin the directory-structure test.Line 269 introduces a new required subdirectory, but
ensure_tutti_dir_creates_structuredoes not assert it exists. Add coverage to prevent regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/state/mod.rs` at line 269, The directory-structure test lacks an assertion for the newly required subdirectory "state/run-ledger"; update the test function ensure_tutti_dir_creates_structure to assert that the path "state/run-ledger" (or the corresponding PathBuf returned/created by the helper) exists after directory creation, using the same pattern as the other assertions (e.g., fs::metadata or Path::exists) so regressions are caught.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/state/mod.rs`:
- Around line 476-484: save_sdlc_run_ledger currently writes the ledger file
directly which can corrupt state on partial writes; modify save_sdlc_run_ledger
to write the JSON to a temporary file in the same directory (e.g.,
path.with_extension("json.tmp") or similar), fsync the temp file and parent dir
as appropriate, then atomically rename the temp file to the final path, and
propagate errors as before; ensure validate_run_id and
transition_sdlc_run_ledger usage remains unchanged and remove or correct the
#[allow(dead_code)] attribute since the function is used.
---
Duplicate comments:
In `@src/state/mod.rs`:
- Around line 127-168: Replace the manual post-op remove_file with a panic-safe
Drop guard: after successfully creating the lock file in with_run_ledger_lock
(the OpenOptions create_new branch), construct a LockGuard (e.g., struct
LockGuard { path: PathBuf }) that owns lock_path and impl Drop for LockGuard to
remove_file(&self.path) ignoring errors; keep the guard alive for the duration
of op() so the file is removed even if op() panics/unwinds, and remove the
explicit std::fs::remove_file(lock_path) call at the end of
with_run_ledger_lock.
---
Nitpick comments:
In `@src/state/mod.rs`:
- Line 269: The directory-structure test lacks an assertion for the newly
required subdirectory "state/run-ledger"; update the test function
ensure_tutti_dir_creates_structure to assert that the path "state/run-ledger"
(or the corresponding PathBuf returned/created by the helper) exists after
directory creation, using the same pattern as the other assertions (e.g.,
fs::metadata or Path::exists) so regressions are caught.
Dismissing stale CodeRabbit CHANGES_REQUESTED; all cited blockers were addressed in follow-up commits and checks are green.
|
Addressed remaining CodeRabbit nitpick in commit :\n- switched run-ledger atomic save temp path to a unique filename per write attempt () to avoid tmp-file collisions under concurrent saves.\n\nValidation: and running 1 test |
|
Correction (previous comment had shell-escaped formatting issues): Addressed remaining CodeRabbit nitpick in commit 35d9242:
Validation: cargo fmt --all and cargo test -q (all passing). @coderabbitai review |
|
✅ Actions performedReview triggered.
|
|
✅ Actions performedReview triggered.
|
|
@coderabbitai review please |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/state/mod.rs (1)
105-125: Consider using an allowlist instead of denylist for stronger validation.The current validation blocks path separators and
.., which prevents the most dangerous path traversal attacks. However, it still permits characters that could cause issues on some filesystems (e.g.,?,*,:,<,>on Windows, or control characters).An allowlist approach (e.g.,
[A-Za-z0-9_-]) would be more defensive.♻️ Proposed refactor
fn validate_run_id(run_id: &str) -> Result<()> { - if run_id.is_empty() || run_id.contains('/') || run_id.contains('\\') || run_id.contains("..") { + let valid = !run_id.is_empty() + && run_id + .chars() + .all(|c| c.is_ascii_alphanumeric() || c == '-' || c == '_'); + if !valid { return Err(TuttiError::State(format!( - "invalid run_id '{run_id}': must not contain path separators or traversal segments" + "invalid run_id '{run_id}': only [A-Za-z0-9_-] allowed" ))); } Ok(()) } fn validate_step_id(step_id: &str) -> Result<()> { - if step_id.is_empty() - || step_id.contains('/') - || step_id.contains('\\') - || step_id.contains("..") - { + let valid = !step_id.is_empty() + && step_id + .chars() + .all(|c| c.is_ascii_alphanumeric() || c == '-' || c == '_'); + if !valid { return Err(TuttiError::State(format!( - "invalid step_id '{step_id}': must not contain path separators or traversal segments" + "invalid step_id '{step_id}': only [A-Za-z0-9_-] allowed" ))); } Ok(()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/state/mod.rs` around lines 105 - 125, Replace the current denylist checks in validate_run_id and validate_step_id with an allowlist-based validation: require the id to match a strict regex (e.g. ^[A-Za-z0-9_-]+$) and return the TuttiError::State if it does not match or is empty; update the error text to reflect the allowed character set and add/adjust tests that exercise invalid characters like ? * : < > and control chars as well as valid examples to ensure the new regex is enforced.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/state/mod.rs`:
- Around line 105-125: Replace the current denylist checks in validate_run_id
and validate_step_id with an allowlist-based validation: require the id to match
a strict regex (e.g. ^[A-Za-z0-9_-]+$) and return the TuttiError::State if it
does not match or is empty; update the error text to reflect the allowed
character set and add/adjust tests that exercise invalid characters like ? * : <
> and control chars as well as valid examples to ensure the new regex is
enforced.
|
Addressed latest actionable CodeRabbit nitpick in commit c76b548.
Validation:
@coderabbitai review |
|
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Readiness driver fix: merged latest main into this branch (commit 6858731) to clear the BEHIND gate and refresh required checks. Current remaining blocker after checks is required approving review from a writer other than the PR author. |
…choir runs (#71) * feat(health): add startup grace window to wait_for_agent_idle (#67) Prevent fresh prompt steps from falsely completing before the agent has consumed the prompt. The startup grace period (default 30s) gates completion detection until real working activity is observed. Key changes: - wait_for_agent_idle accepts a startup_grace Duration parameter - AgentStatus::Working counts as activity even without pane hash change, requiring 2+ consecutive polls to avoid flicker false positives - First pane capture no longer counts as a hash "change" - Completion signals before any activity are held until grace expires - "Unravelling" added to claude-code working patterns - startup_grace_secs field threaded through config and automation Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * docs: add 0.3.0 changelog entry for issue #67 and prior unreleased changes Cover startup grace window (#67), persistent memory (#62/#63), merge gate enforcement (#59), permissions suggest (#53), orchestration state machine (#54/#55), and all fixes shipped since 0.2.0. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: reduce startup grace to 10s and validate wait settings - Reduce DEFAULT_STARTUP_GRACE_SECS from 30 to 10 so the completion-before-activity path fires before typical wait timeouts - Validate that wait_timeout_secs/startup_grace_secs are only set when wait_for_idle is true, failing fast with actionable guidance Addresses CodeRabbit feedback on PR #71. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2 participants
Summary
Creates a focused implementation kickoff for #30 with explicit state-machine contract and recovery invariants.
Included
Why now
This unblocks implementation by locking down the contract before wiring persistence + CLI recovery surfaces.
Refs #30
Summary by CodeRabbit
New Features
Chores
Tests