Skip to content
This repository was archived by the owner on Apr 11, 2026. It is now read-only.

Commit d710120

Browse files
z23ccclaude
andcommitted
fix(fn-20): address 3 adversarial review findings
1. Approval race condition (HIGH) — approve/reject now checks affected-row count from the conditional UPDATE. Concurrent resolutions that lose the race return InvalidTransition instead of reloading the row and falsely emitting a second ApprovalResolved event. (flowctl-service/src/approvals.rs) 2. CLI daemon discovery (MEDIUM) — flow_state_dir now walks up from cwd to find the nearest .flow/ directory instead of rooting at env::current_dir(). Running flowctl approval from a subdirectory no longer silently bypasses the daemon. (flowctl-cli/src/commands/approval.rs) 3. Outputs handoff ordering (MEDIUM) — Phase 5c (Outputs Dump) is now reordered BEFORE Phase 5 (Complete) in the canonical phase sequence, so the narrative handoff artifact exists before flowctl done unblocks dependents. (flowctl-cli/src/commands/workflow/phase.rs, agents/worker.md) All flowctl-service approval tests pass (5/5); full suite 284+ tests pass; clippy clean. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent e29f590 commit d710120

4 files changed

Lines changed: 49 additions & 9 deletions

File tree

agents/worker.md

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -435,6 +435,8 @@ Continue until SHIP verdict. Save final `REVIEW_ITERATIONS` count for Phase 5 ev
435435
<!-- section:core -->
436436
## Phase 5: Complete
437437
438+
**Prerequisite:** Phase 5c (Outputs Dump) must have run if `outputs.enabled=true`. The phase registry orders 5c before 5 so the narrative handoff file exists before dependents unblock.
439+
438440
**Verify before completing:**
439441
```bash
440442
<FLOWCTL> guard
@@ -519,9 +521,11 @@ Status MUST be `done`. If not:
519521
<!-- section:outputs -->
520522
## Phase 5c: Outputs Dump (if outputs.enabled)
521523
524+
**Runs BEFORE Phase 5 completion.** Phase 5c must produce the handoff artifact before `flowctl done` fires, otherwise a dependent task can start re-anchoring and race past the missing file. The phase registry in `flowctl-cli/src/commands/workflow/phase.rs` enforces this ordering (5c before 5).
525+
522526
**Skip if `outputs.enabled` is false.** This is gated on its own config key — independent from `memory.enabled`. Outputs are a lightweight narrative handoff layer (plain markdown, no verification), separate from the verified memory system.
523527
524-
After completing the task, write a ≤200-word narrative dump to `.flow/outputs/<TASK_ID>.md` for the next worker in this epic:
528+
Write a ≤200-word narrative dump to `.flow/outputs/<TASK_ID>.md` for the next worker in this epic:
525529
526530
```bash
527531
# Check if outputs is enabled (default: true)

flowctl/crates/flowctl-cli/src/commands/approval.rs

Lines changed: 22 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -95,10 +95,28 @@ pub fn dispatch(cmd: &ApprovalCmd, json: bool) {
9595
// ── Transport resolution ────────────────────────────────────────────
9696

9797
fn flow_state_dir() -> PathBuf {
98-
env::current_dir()
99-
.unwrap_or_else(|_| PathBuf::from("."))
100-
.join(FLOW_DIR)
101-
.join(STATE_DIR)
98+
find_flow_dir().join(STATE_DIR)
99+
}
100+
101+
/// Walk up from cwd to locate the nearest `.flow/` directory. Falls back to
102+
/// `./.flow` if none found (fresh repo). This matches how other tools resolve
103+
/// project roots (walk up until `.git`) and avoids the subdirectory pitfall
104+
/// where `flowctl approval ...` was bypassing the daemon when run from a
105+
/// crate folder.
106+
fn find_flow_dir() -> PathBuf {
107+
let cwd = env::current_dir().unwrap_or_else(|_| PathBuf::from("."));
108+
let mut cursor: &Path = &cwd;
109+
loop {
110+
let candidate = cursor.join(FLOW_DIR);
111+
if candidate.is_dir() {
112+
return candidate;
113+
}
114+
match cursor.parent() {
115+
Some(parent) => cursor = parent,
116+
None => break,
117+
}
118+
}
119+
cwd.join(FLOW_DIR)
102120
}
103121

104122
/// Where the daemon writes its PID + socket.

flowctl/crates/flowctl-cli/src/commands/workflow/phase.rs

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -67,10 +67,12 @@ const PHASE_DEFS: &[PhaseDef] = &[
6767
];
6868

6969
/// Canonical ordering of all phases — used to merge sequences.
70-
const CANONICAL_ORDER: &[&str] = &["0", "1", "2a", "2", "2.5", "3", "4", "5", "5c", "5b", "6"];
70+
/// Phase 5c (outputs dump) runs BEFORE 5 (completion) so the narrative
71+
/// handoff artifact exists before dependents unblock and begin re-anchor.
72+
const CANONICAL_ORDER: &[&str] = &["0", "1", "2a", "2", "2.5", "3", "4", "5c", "5", "5b", "6"];
7173

7274
/// Default phase sequence (Worktree + Teams, always includes Phase 0).
73-
/// Phase 5c is appended when `outputs.enabled` is true (default).
75+
/// Phase 5c is inserted before 5 when `outputs.enabled` is true (default).
7476
const PHASE_SEQ_DEFAULT: &[&str] = &["0", "1", "2", "2.5", "3", "5", "5b", "6"];
7577
const PHASE_SEQ_TDD: &[&str] = &["0", "1", "2a", "2", "2.5", "3", "5", "5b", "6"];
7678
const PHASE_SEQ_REVIEW: &[&str] = &["0", "1", "2", "2.5", "3", "4", "5", "5b", "6"];

flowctl/crates/flowctl-service/src/approvals.rs

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -181,6 +181,7 @@ impl ApprovalStore for LibSqlApprovalStore {
181181
}
182182

183183
async fn approve(&self, id: &str, resolver: Option<String>) -> ServiceResult<Approval> {
184+
// Pre-check for better error messages; authoritative guard is the UPDATE below.
184185
let existing = self.load_row(id).await?;
185186
if existing.status != ApprovalStatus::Pending {
186187
return Err(ServiceError::InvalidTransition(format!(
@@ -189,14 +190,21 @@ impl ApprovalStore for LibSqlApprovalStore {
189190
)));
190191
}
191192
let now = Utc::now().timestamp();
192-
self.conn
193+
let affected = self
194+
.conn
193195
.execute(
194196
"UPDATE approvals SET status = 'approved', resolved_at = ?1, resolver = ?2
195197
WHERE id = ?3 AND status = 'pending'",
196198
params![now, resolver.clone(), id.to_string()],
197199
)
198200
.await
199201
.map_err(|e| ServiceError::ValidationError(format!("update failed: {e}")))?;
202+
if affected == 0 {
203+
// Lost a race with another resolver — the row is no longer pending.
204+
return Err(ServiceError::InvalidTransition(format!(
205+
"approval {id} was resolved concurrently"
206+
)));
207+
}
200208
self.load_row(id).await
201209
}
202210

@@ -206,6 +214,7 @@ impl ApprovalStore for LibSqlApprovalStore {
206214
resolver: Option<String>,
207215
reason: Option<String>,
208216
) -> ServiceResult<Approval> {
217+
// Pre-check for better error messages; authoritative guard is the UPDATE below.
209218
let existing = self.load_row(id).await?;
210219
if existing.status != ApprovalStatus::Pending {
211220
return Err(ServiceError::InvalidTransition(format!(
@@ -214,14 +223,21 @@ impl ApprovalStore for LibSqlApprovalStore {
214223
)));
215224
}
216225
let now = Utc::now().timestamp();
217-
self.conn
226+
let affected = self
227+
.conn
218228
.execute(
219229
"UPDATE approvals SET status = 'rejected', resolved_at = ?1, resolver = ?2, reason = ?3
220230
WHERE id = ?4 AND status = 'pending'",
221231
params![now, resolver.clone(), reason.clone(), id.to_string()],
222232
)
223233
.await
224234
.map_err(|e| ServiceError::ValidationError(format!("update failed: {e}")))?;
235+
if affected == 0 {
236+
// Lost a race with another resolver — the row is no longer pending.
237+
return Err(ServiceError::InvalidTransition(format!(
238+
"approval {id} was resolved concurrently"
239+
)));
240+
}
225241
self.load_row(id).await
226242
}
227243
}

0 commit comments

Comments
 (0)