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

Commit 7f54390

Browse files
z23ccclaude
andcommitted
fix(fn-20): address 4 iter-2 adversarial review findings
1. [REQUIRED] TCP daemon port discovery (server.rs) — serve_tcp now writes the bound port to .flow/.state/flowctl.port on startup and removes it on shutdown. CLI approval clients can now find the TCP endpoint instead of silently falling back to direct DB writes. Uses listener.local_addr() so port=0 ephemeral binds also publish their real port. 2. [IMPORTANT] Browser approvals drop resolver (handlers/approvals.rs) — daemon approve/reject handlers now default resolver to "dashboard" when the client omits it, so every browser-driven resolution has a non-null audit trail entry. CLI callers passing --resolver still win. 3. [IMPORTANT] epic audit receipts bypass archive (epic.rs) — added review_belongs_to_epic() helper matching both the legacy "-{id}." infix pattern and the new "epic-audit-{id}-*" prefix. Both archive and clean paths now use it, so audit receipts are moved with the rest of the epic's review artifacts. 4. [NICE-TO-HAVE] orphan approvals (approvals.rs) — ApprovalStore::create() now validates the referenced task_id exists before inserting. Typos return ValidationError instead of creating ghost records that show up in the UI with no real work to reconcile to. Added create_rejects_nonexistent_task test + updated in-mem/server tests to seed epic+task rows. Tests: 285 passed, 0 failed. Clippy clean. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent d710120 commit 7f54390

4 files changed

Lines changed: 134 additions & 7 deletions

File tree

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

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -965,7 +965,7 @@ fn cmd_archive(id: &str, force: bool, json_mode: bool) {
965965
for entry in review_entries {
966966
let name = entry.file_name();
967967
let name_str = name.to_string_lossy();
968-
if name_str.contains(&format!("-{id}.")) {
968+
if review_belongs_to_epic(&name_str, id) {
969969
let dest = archive_dir.join(&*name);
970970
let _ = fs::rename(entry.path(), &dest);
971971
moved.push(format!("reviews/{name_str}"));
@@ -1081,7 +1081,8 @@ fn cmd_archive_silent(id: &str, flow_dir: &Path) {
10811081
if let Ok(entries) = fs::read_dir(&reviews_dir) {
10821082
for entry in entries.flatten() {
10831083
let name = entry.file_name();
1084-
if name.to_string_lossy().contains(&format!("-{id}.")) {
1084+
let name_str = name.to_string_lossy();
1085+
if review_belongs_to_epic(&name_str, id) {
10851086
let _ = fs::rename(entry.path(), archive_dir.join(&name));
10861087
}
10871088
}
@@ -1288,6 +1289,19 @@ fn cmd_set_auto_execute(id: &str, pending: bool, done: bool, json_mode: bool) {
12881289
}
12891290
}
12901291

1292+
// ── Shared helpers ─────────────────────────────────────────────────
1293+
1294+
/// Returns true if a review filename belongs to the given epic.
1295+
///
1296+
/// Matches both naming schemes used in `.flow/reviews/`:
1297+
/// - Task-suffixed reviews (plan/impl/cross-model): `*-{epic_id}.<task-num>-*.json`
1298+
/// matched via the `-{id}.` infix
1299+
/// - Epic-level audit receipts: `epic-audit-{id}-<timestamp>.json`
1300+
/// matched via the `-{id}-` prefix
1301+
fn review_belongs_to_epic(name: &str, id: &str) -> bool {
1302+
name.contains(&format!("-{id}.")) || name.starts_with(&format!("epic-audit-{id}-"))
1303+
}
1304+
12911305
// ── Audit command ───────────────────────────────────────────────────
12921306

12931307
/// Find the most recent `epic-audit-<id>-*.json` receipt in `.flow/reviews/`.

flowctl/crates/flowctl-daemon/src/handlers/approvals.rs

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -79,9 +79,13 @@ pub async fn approve_approval_handler(
7979
Json(body): Json<Option<ResolveRequest>>,
8080
) -> Result<Json<serde_json::Value>, AppError> {
8181
let body = body.unwrap_or_default();
82+
// Browser clients typically don't send a resolver; fall back to
83+
// "dashboard" so the audit trail still records *something* rather
84+
// than a null. CLI callers should pass --resolver explicitly.
85+
let resolver = body.resolver.or_else(|| Some("dashboard".to_string()));
8286
let store = LibSqlApprovalStore::new(state.db.clone());
8387
let resolved = store
84-
.approve(&id, body.resolver)
88+
.approve(&id, resolver)
8589
.await
8690
.map_err(service_error_to_app_error)?;
8791

@@ -102,9 +106,11 @@ pub async fn reject_approval_handler(
102106
Json(body): Json<Option<ResolveRequest>>,
103107
) -> Result<Json<serde_json::Value>, AppError> {
104108
let body = body.unwrap_or_default();
109+
// Browser fallback (see approve_approval_handler for rationale).
110+
let resolver = body.resolver.or_else(|| Some("dashboard".to_string()));
105111
let store = LibSqlApprovalStore::new(state.db.clone());
106112
let resolved = store
107-
.reject(&id, body.resolver, body.reason)
113+
.reject(&id, resolver, body.reason)
108114
.await
109115
.map_err(service_error_to_app_error)?;
110116

flowctl/crates/flowctl-daemon/src/server.rs

Lines changed: 46 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -157,20 +157,41 @@ pub async fn serve_tcp(
157157
.await
158158
.with_context(|| format!("failed to bind TCP: {addr}"))?;
159159

160-
info!("daemon API listening on http://{addr}");
160+
// Resolve the bound port (honors port=0 ephemeral binds) and persist it
161+
// to .flow/.state/flowctl.port so CLI clients can discover the TCP
162+
// transport. Without this, `flowctl approval` CLIs silently fall back
163+
// to direct DB writes and bypass the daemon event bus.
164+
let bound_port = listener
165+
.local_addr()
166+
.map(|a| a.port())
167+
.unwrap_or(port);
168+
let port_file = runtime.paths.state_dir.join("flowctl.port");
169+
if let Err(e) = std::fs::write(&port_file, bound_port.to_string()) {
170+
tracing::warn!(
171+
"failed to write port file {}: {} — CLI clients may bypass daemon",
172+
port_file.display(),
173+
e
174+
);
175+
}
176+
177+
info!("daemon API listening on http://127.0.0.1:{bound_port}");
161178

162179
let (state, cancel) = create_state(runtime, event_bus).await?;
163180

164181
let router = build_router(state);
165182

166-
axum::serve(listener, router)
183+
let serve_result = axum::serve(listener, router)
167184
.with_graceful_shutdown(async move {
168185
cancel.cancelled().await;
169186
info!("HTTP server shutting down");
170187
})
171188
.await
172-
.context("HTTP server error")?;
189+
.context("HTTP server error");
190+
191+
// Best-effort cleanup of the port file on shutdown.
192+
let _ = std::fs::remove_file(&port_file);
173193

194+
serve_result?;
174195
Ok(())
175196
}
176197

@@ -445,6 +466,17 @@ mod tests {
445466
let (_tmp, runtime, event_bus) = test_setup();
446467
let mut event_rx = event_bus.subscribe();
447468
let (state, _cancel) = create_state(runtime, event_bus).await.unwrap();
469+
470+
// Seed epic+task so approval create() existence check passes.
471+
state.db.execute(
472+
"INSERT INTO epics (id, title, status, file_path, created_at, updated_at) VALUES ('fn-1', 'Test', 'open', 'epics/fn-1.md', '2025-01-01T00:00:00Z', '2025-01-01T00:00:00Z')",
473+
(),
474+
).await.unwrap();
475+
state.db.execute(
476+
"INSERT INTO tasks (id, epic_id, title, status, file_path, created_at, updated_at) VALUES ('fn-1.1', 'fn-1', 'Test Task', 'todo', 'tasks/fn-1.1.md', '2025-01-01T00:00:00Z', '2025-01-01T00:00:00Z')",
477+
(),
478+
).await.unwrap();
479+
448480
let app = build_router(state);
449481

450482
// Create
@@ -536,6 +568,17 @@ mod tests {
536568
async fn approval_reject_records_reason() {
537569
let (_tmp, runtime, event_bus) = test_setup();
538570
let (state, _cancel) = create_state(runtime, event_bus).await.unwrap();
571+
572+
// Seed epic+task so approval create() existence check passes.
573+
state.db.execute(
574+
"INSERT INTO epics (id, title, status, file_path, created_at, updated_at) VALUES ('fn-2', 'Test', 'open', 'epics/fn-2.md', '2025-01-01T00:00:00Z', '2025-01-01T00:00:00Z')",
575+
(),
576+
).await.unwrap();
577+
state.db.execute(
578+
"INSERT INTO tasks (id, epic_id, title, status, file_path, created_at, updated_at) VALUES ('fn-2.1', 'fn-2', 'Test Task', 'todo', 'tasks/fn-2.1.md', '2025-01-01T00:00:00Z', '2025-01-01T00:00:00Z')",
579+
(),
580+
).await.unwrap();
581+
539582
let app = build_router(state);
540583

541584
let req = axum::http::Request::builder()

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

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -123,6 +123,35 @@ fn row_to_approval(row: libsql::Row) -> ServiceResult<Approval> {
123123
#[async_trait::async_trait]
124124
impl ApprovalStore for LibSqlApprovalStore {
125125
async fn create(&self, req: CreateApprovalRequest) -> ServiceResult<Approval> {
126+
// Reject orphan approvals: the referenced task must exist. Without
127+
// this check a typo creates a ghost pending record with no way to
128+
// reconcile it to real work.
129+
let exists: i64 = {
130+
let mut rows = self
131+
.conn
132+
.query(
133+
"SELECT COUNT(*) FROM tasks WHERE id = ?1",
134+
params![req.task_id.clone()],
135+
)
136+
.await
137+
.map_err(|e| ServiceError::ValidationError(format!("task lookup: {e}")))?;
138+
let row = rows
139+
.next()
140+
.await
141+
.map_err(|e| ServiceError::ValidationError(format!("task lookup row: {e}")))?
142+
.ok_or_else(|| {
143+
ServiceError::ValidationError("task lookup returned no rows".into())
144+
})?;
145+
row.get(0)
146+
.map_err(|e| ServiceError::ValidationError(format!("task lookup value: {e}")))?
147+
};
148+
if exists == 0 {
149+
return Err(ServiceError::ValidationError(format!(
150+
"task {} does not exist",
151+
req.task_id
152+
)));
153+
}
154+
126155
let id = Self::new_id();
127156
let now = Utc::now().timestamp();
128157
let payload_str = serde_json::to_string(&req.payload)
@@ -248,11 +277,46 @@ mod tests {
248277

249278
async fn in_mem_store() -> LibSqlApprovalStore {
250279
let (db, conn) = flowctl_db::open_memory_async().await.unwrap();
280+
// Seed tasks referenced by the tests so the existence check passes.
281+
let now = "2026-01-01T00:00:00Z";
282+
for tid in &["fn-1.1", "fn-1.2"] {
283+
let (epic_id, _num) = tid.split_once('.').unwrap();
284+
conn.execute(
285+
"INSERT OR IGNORE INTO epics
286+
(id, title, status, file_path, created_at, updated_at, body)
287+
VALUES (?1, ?1, 'open', ?1, ?2, ?2, '')",
288+
params![epic_id.to_string(), now.to_string()],
289+
)
290+
.await
291+
.expect("seed epic");
292+
conn.execute(
293+
"INSERT OR IGNORE INTO tasks
294+
(id, epic_id, title, status, file_path, created_at, updated_at, body)
295+
VALUES (?1, ?2, ?1, 'todo', ?1, ?3, ?3, '')",
296+
params![tid.to_string(), epic_id.to_string(), now.to_string()],
297+
)
298+
.await
299+
.expect("seed task");
300+
}
251301
// Leak the db so conn stays valid for the test lifetime.
252302
Box::leak(Box::new(db));
253303
LibSqlApprovalStore::new(conn)
254304
}
255305

306+
#[tokio::test]
307+
async fn create_rejects_nonexistent_task() {
308+
let store = in_mem_store().await;
309+
let err = store
310+
.create(CreateApprovalRequest {
311+
task_id: "fn-999.99".into(),
312+
kind: ApprovalKind::FileAccess,
313+
payload: serde_json::json!({}),
314+
})
315+
.await
316+
.expect_err("should reject nonexistent task");
317+
assert!(matches!(err, ServiceError::ValidationError(_)));
318+
}
319+
256320
#[tokio::test]
257321
async fn create_and_get() {
258322
let store = in_mem_store().await;

0 commit comments

Comments
 (0)