feat(server): make the REST API usable by external work-queue dispatchers - #3187
Conversation
|
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:
WalkthroughThe session API now supports scoped listing, readiness waiting, callback delivery, and persistent idempotent creation. Session metadata persists on ChangesSession API lifecycle
WebSocket compression test
Estimated code review effort: 5 (Critical) | ~90 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
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: 2
🧹 Nitpick comments (2)
src/server/callback.rs (1)
320-355: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the generation bump so the test drives real code.
debounce_drops_stale_generation_after_flickerre-implements the bump fromhandle_status_changerather than calling it. The test would still pass if the production bump changed, so it verifies the pattern rather than the implementation.A small named helper used by both would close that gap and shorten the test:
♻️ Proposed refactor
/// Claim the next debounce generation for `session_id`. The caller's timer /// fires only if this value is still current when the window elapses. fn claim_generation(session_id: &str) -> u64 { let mut guard = debounce_state().lock().unwrap(); let entry = guard .entry(session_id.to_string()) .or_insert(DebounceEntry { generation: 0 }); entry.generation = entry.generation.wrapping_add(1); entry.generation }
handle_status_changethen callsclaim_generation(&session_id), and the test calls it twice instead of duplicating the body.Optional, and the existing test does document the intent well either way.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/callback.rs` around lines 320 - 355, Extract the generation-increment logic from handle_status_change into a named claim_generation helper that updates debounce_state and returns the new generation. Replace the production inline logic and both manual setup blocks in debounce_drops_stale_generation_after_flicker with calls to this helper, preserving the stale-generation assertions.src/server/api/sessions.rs (1)
5124-5132: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy liftConsider rejecting key reuse with a different request body.
The lookup matches on the key alone. If a caller reuses a key with a different
path,tool, ortitle, the handler returns the earlier, unrelated session with 200 and never creates the requested one.The plugin path already solved this:
find_idempotent_matchinsrc/server/session_service.rscompares apayload_hashand reportsConflicton mismatch. Reusing that shape here (a 409 on mismatch) would keep the two surfaces consistent and make a client-side key collision loud instead of silent.Not a blocker for this PR, since the documented contract is "a retry returns the existing session", and a well-behaved dispatcher does not reuse keys across different requests.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/api/sessions.rs` around lines 5124 - 5132, The idempotency lookup in the session handler currently matches only the key and silently returns an unrelated session when the request body differs. Update the flow around find_by_idempotency_key and SessionResponse::from_instance to compute and compare a payload_hash for fields such as path, tool, and title, returning HTTP 409 on mismatch while preserving the existing 200 response for identical retries; follow the find_idempotent_match behavior in session_service.rs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/server/callback.rs`:
- Around line 81-98: Update is_forbidden_target so IPv4-mapped IPv6 addresses
are canonicalized and classified through the existing IPv4 rules before applying
the native IPv6 checks. Ensure mapped loopback, private, link-local,
unspecified, and multicast targets are rejected consistently across callback
validation and resolve_is_safe checks.
In `@src/server/mod.rs`:
- Around line 567-581: Update AppState::idempotency_lock in
src/server/mod.rs:567-581 to remove the map entry under the write lock when the
returned Arc’s strong count reaches 1 after the holder releases it, following
the existing changed_files_cached pruning pattern; also correct the
idempotency_locks field documentation to avoid claiming an instance_locks-style
bound. In src/server/api/sessions.rs:5121-5136, make no call-site change; update
the nearby comment to state that IDEMPOTENCY_KEY_MAX_LEN limits individual key
length, not map size.
---
Nitpick comments:
In `@src/server/api/sessions.rs`:
- Around line 5124-5132: The idempotency lookup in the session handler currently
matches only the key and silently returns an unrelated session when the request
body differs. Update the flow around find_by_idempotency_key and
SessionResponse::from_instance to compute and compare a payload_hash for fields
such as path, tool, and title, returning HTTP 409 on mismatch while preserving
the existing 200 response for identical retries; follow the
find_idempotent_match behavior in session_service.rs.
In `@src/server/callback.rs`:
- Around line 320-355: Extract the generation-increment logic from
handle_status_change into a named claim_generation helper that updates
debounce_state and returns the new generation. Replace the production inline
logic and both manual setup blocks in
debounce_drops_stale_generation_after_flicker with calls to this helper,
preserving the stale-generation assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 04f5666a-c9ea-48c2-a810-295420896c4f
📒 Files selected for processing (8)
docs/api.mdsrc/plugin/session_api.rssrc/server/api/sessions.rssrc/server/callback.rssrc/server/mod.rssrc/server/session_service.rssrc/server/session_spawn.rssrc/session/instance.rs
23ac363 to
3ed0711
Compare
Seluj78
changed the title
Bundle ReportBundle size has no change ✅ |
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #3187 +/- ##
=======================================
Coverage 85.76% 85.77%
=======================================
Files 290 290
Lines 17976 17976
Branches 5404 5404
=======================================
+ Hits 15418 15419 +1
Misses 2140 2140
+ Partials 418 417 -1 see 1 file with indirect coverage changes
Continue to review full report in Codecov by Harness.
|
`Ipv6Addr::is_loopback()` matches only `::1`, so `is_forbidden_target` let every IPv4-mapped form through: `::ffff:127.0.0.1`, `::ffff:169.254.169.254` (cloud metadata), and mapped RFC1918 addresses all cleared create-time validation and the pre-dispatch re-resolve check while the OS still dialed the v4 target, defeating the callback_url SSRF guard entirely. Route the address through `to_canonical()` before classifying so mapped forms are judged by the existing IPv4 rules; a genuine v6 address is unaffected, and a mapped *public* address stays allowed. Also move the SSRF doc block back onto `is_forbidden_target`; it had drifted above `strip_ipv6_brackets`, so rustdoc attached it to the wrong function. Addresses review comment from @coderabbitai on #3187.
`idempotency_locks` only ever inserted, so a long-lived daemon serving a work queue accumulated one String plus one Arc<Mutex<()>> per distinct key for its whole lifetime. Unlike `instance_locks`, entry count is not bounded by the session count: keys are caller-supplied, one per request. Prune entries whose strong count is 1 on the miss path, under the write lock, mirroring `changed_files_cached`. A strong count of 1 means the map holds the only reference, so nothing is mid-flight on that key and the created session's persisted `idempotency_key` is already the durable dedup record. A waiter can only clone the Arc while holding that same write lock, so pruning cannot race one away. Also correct two docs that claimed a bound the code did not provide: the field doc implied `instance_locks`' session-count bound, and the `IDEMPOTENCY_KEY_MAX_LEN` comment claimed the length cap prevented unbounded map growth when it only caps a single key's size. Addresses review comment from @coderabbitai on #3187.
jerome-benoit
left a comment
jerome-benoit
left a comment
There was a problem hiding this comment.
Review — external dispatcher API (#3187)
Verdict: REQUEST-CHANGES. One blocking wire-contract defect; the SSRF guard is reasonable but has a self-disclosed reachable residual.
BLOCKING — status wire format is self-contradictory and mis-documented
- Normal responses serialize
format!("{:?}", inst.status)= PascalCase (sessions.rs:347), pinned by existing tests (:7897"Running",:8036) and depended on by the bundled web UI (Dashboard.tsx:45-46,s.status === "Waiting"). - The new
?wait=readypath overwrites it withfresh.status.as_str()= lowercase (sessions.rs:5492;instance.rs:58-64). So the same POST endpoint returns"Idle"normally and"idle"with?wait=ready; the callback body is lowercase too. docs/api.md:45-47(added here) asserts "lowercase on the wire … never in the JSON API" — the opposite of what the API emits. A dispatcher coded to these docs never matches aGET /api/sessionspoll: exactly the automation this PR targets.
Ask: pick one casing across SessionResponse / ?wait=ready / callback, fix docs/api.md, and flag the wire change as breaking per AGENTS.md.
MEDIUM — SSRF DNS-rebinding TOCTOU (self-disclosed, reachable)
resolve_is_safe calls lookup_host, then reqwest re-resolves on connect (callback.rs). Attacker DNS returns a public IP for the check, private/metadata for the connect. Bounded (blind, POST-only, response discarded, redirects Policy::none(), auth-gated) so not critical, but the "re-checked before every dispatch" claim is defeatable. Ask: pin the vetted IP via a custom reqwest resolver, or downgrade the "SSRF-safe" wording to "SSRF-guarded, DNS-rebinding residual."
LOW
- Denylist gaps pass both gates: NAT64
64:ff9b::/96, IPv4-compatible::a.b.c.d, CGNAT100.64.0.0/10(proven with std-onlyto_canonical). Low reachability; prefer anis_globalallowlist posture or add these ranges. debounce_stateglobal map is inserted (callback.rs:207) but never pruned in production (.removeonly under#[cfg(test)]) → unbounded growth over daemon lifetime; ironic vs theidempotency_locksprune this PR added.callback_url(embeds bearer tokens) persisted plaintext insessions.json; correctly kept out ofSessionResponse, but a new secret-at-rest surface.?state=<invalid>now returns 400 (was silently ignored) — minor behavior change.
Verified & rejected (not issues)
Idempotency lock leak (prune is correct: retain(strong_count>1) under write lock, read/write exclusive); ?wait=ready DoS (bounded 10s, post-spawn); auth bypass (behind auth_middleware); redirect SSRF (Policy::none()); IPv4-mapped ::ffff: (unwrapped, tested); retro-compat (optional fields, response shape unchanged).
`SessionResponse.status` is built with `format!("{:?}")`, so the HTTP
API's wire form has always been PascalCase (`Running`); existing tests
pin it and the web dashboard compares against it, and `from_api_str`
accepts only that spelling. But the new `?wait=ready` path and the
callback payload both used `Status::as_str()`, the lowercase CLI/hook
vocabulary. The same POST endpoint therefore answered `"Idle"` without
the query param and `"idle"` with it, emitting a value AoE's own parser
rejects, and `docs/api.md` documented the lowercase form as the only one
the JSON API ever returns, which is the reverse of the truth.
Add `Status::wire_str()` as the single explicit source for the API
spelling and use it in all three places. Spelled out rather than left as
`format!("{:?}")` so renaming a variant cannot silently change the public
API; the existing round-trip test now pins `wire_str` against `Debug` and
through `from_api_str`. Docs corrected to PascalCase, with a note that the
CLI/`[status_hooks]` env form is deliberately lowercase.
No released behavior changes: `SessionResponse` emits the same bytes as
before, so only this PR's own unshipped surfaces move.
Addresses review comment from @jerome-benoit on #3187.
The guard resolved the callback host, checked the addresses, then handed the URL to a shared client that resolved the name a second time on connect. That is a TOCTOU: hostile DNS can answer with a public address for the check and a loopback or metadata address for the connect, so the approved target and the reached target differ. The old comment conceded this residual instead of closing it. `resolve_is_safe` becomes `resolve_vetted_addrs`, returning the approved addresses, and the dispatch builds a per-request client with `resolve_to_addrs` pinning exactly those. The connect no longer performs its own lookup, so a changed DNS answer cannot redirect it. `resolve_to_addrs` is builder-level, so the shared client is gone and each dispatch builds its own. Callbacks are debounced and fire per status transition, so losing connection reuse costs far less than leaving the window open. Docs and the module comment now describe the guarantee that actually holds. Addresses review comment from @jerome-benoit on #3187.
Three ranges cleared both callback gates while still reaching internal targets, confirmed with a std-only probe: 64:ff9b::169.254.169.254 (NAT64, RFC 6052) -> not blocked ::169.254.169.254 (IPv4-compatible) -> not blocked 100.64.0.1 (CGNAT, RFC 6598) -> not blocked `to_canonical()` only unwraps `::ffff:` mapped addresses, so the other two v6 forms carrying an embedded IPv4 stayed on the v6 path where `is_loopback()` means only `::1`. Extract the embedded address from all three forms and judge it by the IPv4 rules, and treat 100.64.0.0/10 as private (`Ipv4Addr::is_shared` would cover it but is nightly-only). Tests gain a row per form, plus 100.63/100.128 and a global v6 address on the accept side so the new masks cannot over-block their neighbours. Addresses review comment from @jerome-benoit on #3187.
`debounce_state` was inserted on every fire-worthy transition and removed only under `#[cfg(test)]`, so production kept one entry per session id that ever fired, for the daemon's lifetime. Same leak this PR already fixed for `idempotency_locks`. Extract the two halves into `bump_debounce` / `claim_debounce` and have the winning task drop its entry, leaving the map holding only sessions with a debounce window in flight. Check and removal share one lock acquisition: releasing between them would let a transition arriving in the gap insert a fresh entry that the call then deletes, stranding that newer task. A superseded task removes nothing, since the newer generation owns the entry. Extracting the helpers also makes the invariant testable; the test now covers supersede, fire-once, and the no-leftover-entry property, and fails if the removal is dropped. Addresses review comment from @jerome-benoit on #3187.
`callback_url` is persisted in the session store, so a URL carrying a bearer token becomes a secret at rest even though the field is never echoed back in a response. Say so, and point callers at authenticating deliveries without embedding credentials in the URL. Also spell out that an unrecognized `?state=` value is a 400 rather than being ignored, which is the safer contract for an automation client but worth stating. Addresses review comments from @jerome-benoit on #3187.
Thanks, this was a genuinely useful review. All six items addressed; every finding held up under checking, including both doc-accuracy ones. BLOCKING — status casing: fixed, and you were right about the directionYou're correct, and I had this backwards in my own head and in the docs. Standardized on PascalCase, since that is the shipped contract your citations pin. Added On flagging it breaking: going PascalCase means MEDIUM — DNS rebinding: closed rather than rewordedTook the resolver option. LOW — denylist gaps: addedReproduced all three with a std-only probe before changing anything: LOW — debounce_state leak: fixedFair hit, and the irony is deserved. Extracted LOW — callback_url at rest, and strict
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/session/instance.rs (2)
5620-5637: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert the literal API values in this contract test.
The test derives the expected wire value from
format!("{status:?}"). If a future change updates theDebugspelling andwire_str()together, the test can pass while the public API changes. Add expected literals such as(Status::Running, "Running"), then retain theDebugcomparison as a separate drift check if needed.As per coding guidelines, keep these status permutations in one table-driven test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/session/instance.rs` around lines 5620 - 5637, Update the table-driven status contract test around wire_str and from_api_str to store explicit literal API values, such as (Status::Running, "Running"), instead of deriving wire from Debug formatting. Assert each literal against wire_str and retain the Debug comparison separately as a drift check, while keeping all status permutations in the same test.Source: Coding guidelines
867-879: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd serde coverage for the persisted metadata.
callback_urlandidempotency_keychange theInstancestorage schema. Add one in-module test that verifies populated values round-trip, legacy JSON without these fields deserializes toNone, andNonevalues remain omitted.As per coding guidelines, use an in-module unit test for this pure serialization contract.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/session/instance.rs` around lines 867 - 879, Add an in-module unit test for the Instance serde contract covering populated callback_url and idempotency_key round-tripping, legacy JSON that omits both fields deserializing them as None, and None values being omitted during serialization. Keep the test focused on these fields and use the existing Instance serialization/deserialization helpers and test conventions.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/server/callback.rs`:
- Around line 221-230: Bound the DNS lookup in resolve_vetted_addrs with
tokio::time::timeout using a new RESOLVE_TIMEOUT constant declared beside
CONNECT_TIMEOUT; preserve the existing fail-closed behavior by returning None
when the lookup times out, errors, or yields forbidden/empty addresses.
In `@src/session/instance.rs`:
- Around line 70-77: Update the documentation comment for the status wire-form
implementation to reference the existing test `status_api_wire_form_round_trips`
instead of `wire_str_matches_debug`, without changing the documented behavior or
implementation.
---
Nitpick comments:
In `@src/session/instance.rs`:
- Around line 5620-5637: Update the table-driven status contract test around
wire_str and from_api_str to store explicit literal API values, such as
(Status::Running, "Running"), instead of deriving wire from Debug formatting.
Assert each literal against wire_str and retain the Debug comparison separately
as a drift check, while keeping all status permutations in the same test.
- Around line 867-879: Add an in-module unit test for the Instance serde
contract covering populated callback_url and idempotency_key round-tripping,
legacy JSON that omits both fields deserializing them as None, and None values
being omitted during serialization. Keep the test focused on these fields and
use the existing Instance serialization/deserialization helpers and test
conventions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: def389de-4b45-4a00-abfd-dc04258ebec0
📒 Files selected for processing (4)
docs/api.mdsrc/server/api/sessions.rssrc/server/callback.rssrc/session/instance.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/api.md
- src/server/api/sessions.rs
External dispatchers polling the session list have no way to exclude trashed/archived rows without knowing to filter trashed_at/archived_at client-side, so a large trash bucket reads as live. Add ?state=live (exclude trashed+archived) and ?state=trashed; no param keeps the existing unfiltered behavior since the web dashboard's Trash view depends on fetching everything and filtering client-side today. Part of #3156.
External dispatchers need a completion signal without polling, and a way to retry a timed-out create without double-spawning a worktree + agent. Add two per-session create-time fields: - callback_url: an HTTP POST fires here on Idle/Waiting/Error transitions, dispatched via a new consumer subscribed to the same status_tx broadcast the web-push consumer uses. SSRF-guarded (rejects loopback/private/link-local targets at create time and re-validates the resolved address before every dispatch) and debounced (mirrors status_hooks.rs's generation-counter pattern) so sub-second tmux-scrape flicker doesn't fire duplicate callbacks. - idempotency_key: persisted on the created Instance (survives a daemon restart, unlike an in-memory-only map) and guarded by a per-key mutex so two concurrent requests sharing a new key can't both scan-miss and both create a session. A retry with a known key returns the existing session as 200 instead of creating a duplicate. Design informed by a /debate consult: initial draft used a bare in-memory idempotency map with no locking and no dwell on the callback, which a security/correctness review flagged as an SSRF vector, a flicker-driven duplicate-dispatch bug, and a restart-unsafe idempotency guarantee. Part of #3156.
POST /api/sessions returns 201 while the new session's status is still Starting, a real race for a dispatcher that sends a message immediately after create. Add ?wait=ready: subscribes to status_tx before checking current state (so a transition landing between subscribe and check isn't missed), then races the broadcast against a 10s bound, falling back to re-reading live state on a lagged receiver. Always returns the freshest observed status rather than the stale pre-wait snapshot, per the /debate review that caught the original draft silently returning 201 with a lied-about status. Part of #3156.
External dispatchers reverse-engineered field names and status casing from the binary because docs/api.md only covered send/output. Document GET /api/sessions and its state= filter, the actual lowercase status values (the issue reporter's PascalCase claim came from Rust Debug output, not the wire format), and the new callback_url/idempotency_key/ wait=ready fields from #3156.
`Ipv6Addr::is_loopback()` matches only `::1`, so `is_forbidden_target` let every IPv4-mapped form through: `::ffff:127.0.0.1`, `::ffff:169.254.169.254` (cloud metadata), and mapped RFC1918 addresses all cleared create-time validation and the pre-dispatch re-resolve check while the OS still dialed the v4 target, defeating the callback_url SSRF guard entirely. Route the address through `to_canonical()` before classifying so mapped forms are judged by the existing IPv4 rules; a genuine v6 address is unaffected, and a mapped *public* address stays allowed. Also move the SSRF doc block back onto `is_forbidden_target`; it had drifted above `strip_ipv6_brackets`, so rustdoc attached it to the wrong function. Addresses review comment from @coderabbitai on #3187.
`idempotency_locks` only ever inserted, so a long-lived daemon serving a work queue accumulated one String plus one Arc<Mutex<()>> per distinct key for its whole lifetime. Unlike `instance_locks`, entry count is not bounded by the session count: keys are caller-supplied, one per request. Prune entries whose strong count is 1 on the miss path, under the write lock, mirroring `changed_files_cached`. A strong count of 1 means the map holds the only reference, so nothing is mid-flight on that key and the created session's persisted `idempotency_key` is already the durable dedup record. A waiter can only clone the Arc while holding that same write lock, so pruning cannot race one away. Also correct two docs that claimed a bound the code did not provide: the field doc implied `instance_locks`' session-count bound, and the `IDEMPOTENCY_KEY_MAX_LEN` comment claimed the length cap prevented unbounded map growth when it only caps a single key's size. Addresses review comment from @coderabbitai on #3187.
The spec waited for streamed lines to render, then sampled `counts.binary` exactly once and required it to be non-zero. But the server starts every live-ws connection in text mode: `deflate` is initialized false and only flips once the client's `caps` message is processed, with the deflater built on the first frame after that (`live_ws.rs`). The lines that satisfy the render wait can therefore be pre-switch JSON text frames, so a slow CI worker legitimately observes `binary == 0` and the test fails despite correct behavior. Poll for the binary count instead. The shim prints every 0.2s, so once the switch lands binary frames must follow. The second assertion stays a plain compare: the counter increments inside the wrapped `onmessage` before the app paints, so a new line in the DOM implies its frame was already counted. Not caused by this PR (the diff is Rust-only and does not touch the WS relay); the flake was latent and surfaced on this run.
`SessionResponse.status` is built with `format!("{:?}")`, so the HTTP
API's wire form has always been PascalCase (`Running`); existing tests
pin it and the web dashboard compares against it, and `from_api_str`
accepts only that spelling. But the new `?wait=ready` path and the
callback payload both used `Status::as_str()`, the lowercase CLI/hook
vocabulary. The same POST endpoint therefore answered `"Idle"` without
the query param and `"idle"` with it, emitting a value AoE's own parser
rejects, and `docs/api.md` documented the lowercase form as the only one
the JSON API ever returns, which is the reverse of the truth.
Add `Status::wire_str()` as the single explicit source for the API
spelling and use it in all three places. Spelled out rather than left as
`format!("{:?}")` so renaming a variant cannot silently change the public
API; the existing round-trip test now pins `wire_str` against `Debug` and
through `from_api_str`. Docs corrected to PascalCase, with a note that the
CLI/`[status_hooks]` env form is deliberately lowercase.
No released behavior changes: `SessionResponse` emits the same bytes as
before, so only this PR's own unshipped surfaces move.
Addresses review comment from @jerome-benoit on #3187.
The guard resolved the callback host, checked the addresses, then handed the URL to a shared client that resolved the name a second time on connect. That is a TOCTOU: hostile DNS can answer with a public address for the check and a loopback or metadata address for the connect, so the approved target and the reached target differ. The old comment conceded this residual instead of closing it. `resolve_is_safe` becomes `resolve_vetted_addrs`, returning the approved addresses, and the dispatch builds a per-request client with `resolve_to_addrs` pinning exactly those. The connect no longer performs its own lookup, so a changed DNS answer cannot redirect it. `resolve_to_addrs` is builder-level, so the shared client is gone and each dispatch builds its own. Callbacks are debounced and fire per status transition, so losing connection reuse costs far less than leaving the window open. Docs and the module comment now describe the guarantee that actually holds. Addresses review comment from @jerome-benoit on #3187.
Three ranges cleared both callback gates while still reaching internal targets, confirmed with a std-only probe: 64:ff9b::169.254.169.254 (NAT64, RFC 6052) -> not blocked ::169.254.169.254 (IPv4-compatible) -> not blocked 100.64.0.1 (CGNAT, RFC 6598) -> not blocked `to_canonical()` only unwraps `::ffff:` mapped addresses, so the other two v6 forms carrying an embedded IPv4 stayed on the v6 path where `is_loopback()` means only `::1`. Extract the embedded address from all three forms and judge it by the IPv4 rules, and treat 100.64.0.0/10 as private (`Ipv4Addr::is_shared` would cover it but is nightly-only). Tests gain a row per form, plus 100.63/100.128 and a global v6 address on the accept side so the new masks cannot over-block their neighbours. Addresses review comment from @jerome-benoit on #3187.
`debounce_state` was inserted on every fire-worthy transition and removed only under `#[cfg(test)]`, so production kept one entry per session id that ever fired, for the daemon's lifetime. Same leak this PR already fixed for `idempotency_locks`. Extract the two halves into `bump_debounce` / `claim_debounce` and have the winning task drop its entry, leaving the map holding only sessions with a debounce window in flight. Check and removal share one lock acquisition: releasing between them would let a transition arriving in the gap insert a fresh entry that the call then deletes, stranding that newer task. A superseded task removes nothing, since the newer generation owns the entry. Extracting the helpers also makes the invariant testable; the test now covers supersede, fire-once, and the no-leftover-entry property, and fails if the removal is dropped. Addresses review comment from @jerome-benoit on #3187.
`callback_url` is persisted in the session store, so a URL carrying a bearer token becomes a secret at rest even though the field is never echoed back in a response. Say so, and point callers at authenticating deliveries without embedding credentials in the URL. Also spell out that an unrecognized `?state=` value is a 400 rather than being ignored, which is the safer contract for an automation client but worth stating. Addresses review comments from @jerome-benoit on #3187.
The dispatch task acquires its DISPATCH_CONCURRENCY permit before calling resolve_vetted_addrs, and the reqwest connect/total timeouts only start once build_pinned_client runs, so the pre-dispatch lookup_host was unbounded while holding a permit. Callback hosts pointing at unresponsive resolvers could therefore pin all eight permits and starve callbacks for every other session. Wrap the lookup in RESOLVE_TIMEOUT (5s). Both the timeout and a lookup error fail closed to None, preserving the existing behavior. Addresses review comment from @coderabbitai on #3187.
The doc pointed at `wire_str_matches_debug`, which does not exist: the assertion was folded into the existing `status_api_wire_form_round_trips` rather than added as its own test fn. Point readers at the real name. Addresses review comment from @coderabbitai on #3187.
1868e89 to
83eec5a
Compare
|
Post-merge follow-up: I re-verified this against Confirmed fixed (file:line):
Two non-blocking test-coverage gaps for a future follow-up (code is correct; only the regression guard is missing):
No functional regressions found. Nice work. |
`aoe list --json` did not carry a state field or the `trashed_at`/ `archived_at` timestamps, so a scripted consumer that provisions sessions could not distinguish a trashed session from a genuinely failed one — the title stays in the listing, `aoe add` refuses a duplicate title+path, and attaching would resume something the user threw away. The REST API already solved this in agent-of-empires#3187 with `GET /api/sessions?state=live|trashed|all`; the CLI just never got the same treatment (agent-of-empires#3350). Add `aoe list --state <live|trashed|all>` mirroring the API vocabulary, defaulting to `all` so today's unfiltered behavior is preserved. Include a `state` string plus `trashed_at` and `archived_at` (`skip_if_none`) in every `--json` record so a consumer keying on state can distinguish the three cases from a single call. The human-readable table gains a `STATE` column only under `--state=all` where rows are mixed; under `--state=live` or `trashed` every row has the same state and the column carries no information. The API's `SessionScope` enum moved to a new bare-core `src/session/scope.rs` so the CLI (bare-core) and the serve daemon share one source of truth and cannot drift. `SessionScope::matches` carries the filter predicate, replacing the ad hoc `instance_matches_scope` helper the API had inline. Explicitly left for follow-up (per the issue body's own list): archive/ snooze/favorite flags on `aoe list`, and a matching `--state` implementation for `aoe session list-trash` / `aoe session show`.
2 participants
Note: Claude handled the implementation and prose via back-and-forth. Idea, decisions, and everything else are mine. — @Seluj78
Description
An external work-queue dispatcher (a daemon that polls a ticket queue and drives
aoe servesessions programmatically) hit several rough edges in the REST API. This PR addresses the five that were scoped for a single PR (the reporter explicitly offered to split each item out); a sixth item raised in a comment (the TUI not noticing sessions created externally) looks stale against the current code and is left for a follow-up once reproduced, see the issue comment.GET /api/sessionsfilter. The endpoint returned every session, including trashed and archived ones, with no way to filter server-side; a dispatcher had to know to filtertrashed_at/archived_atclient-side or it would treat a trashed session as live. Added?state=live|trashed|all; no param keeps the existing unfiltered behavior (the web dashboard's Trash view still depends on fetching everything).Docs.
docs/api.mdcoveredsend/outputbut notGET /api/sessions, its new filter, or the actual status value casing (lowercase on the wire; the issue reporter's PascalCase claim came from RustDebugoutput they found in the binary, not the JSON API). Documented all of the above plus the three new create-time fields below.callback_url. A session created with this set receives an HTTP POST when it transitions towaiting,idle, orerror, dispatched via a new consumer on the samestatus_txbroadcast the web-push feature already uses. SSRF-guarded (rejects loopback/private/link-local targets at create time and re-validates the resolved address before every dispatch) and debounced (reusingstatus_hooks.rs's existing generation-counter pattern) so a session flickering between states doesn't fire duplicate callbacks.?wait=readyon create.POST /api/sessionsused to return201immediately while the session was stillstarting, a real race for a dispatcher that sends a message right after create.?wait=readyblocks (bounded to 10s) until the status leavesstarting, and always returns the freshest observed status rather than a stale snapshot.idempotency_keyon create. A dispatcher retrying a timed-out create could previously double-spawn a worktree and agent. The key is persisted on the created session (survives a daemon restart) and guarded by a per-key mutex, so a retry with the same key returns the existing session instead of creating a duplicate.Items 3-5 went through a
/debatedesign review before implementation; the first draft had a bare in-memory idempotency map with no locking (races, doesn't survive restart) and a callback consumer with no debounce (duplicate dispatch on flicker) and no SSRF validation. All three were reworked based on that review.Part of #3156. Item 6 from that issue (TUI file-watch gap) is not addressed here; see this comment asking for a repro, since the current
DiskWatchStatecode already appears to watchsessions.json/groups.json.PR Type
Checklist
Test Coverage Analysis
Web dashboard / structured view (Playwright user story)
TUI / CLI (e2e test)
list_sessions_state_filter,wait_until_left_starting_*(4 tests covering immediate-return, broadcast-resolve, timeout, and vanished-instance paths),find_by_idempotency_key_matches_trashed_but_not_missing, and 8 tests in the newcallback.rsmodule (SSRF rejection for loopback/private/link-local targets, debounce generation logic, fire-worthy status matching).AI Usage
AI Model/Tool used: Claude (via Claude Code, agent-of-empires
/aoe-implementskill), with a/debatedesign consult (Gemini 3.1 Pro + GPT-5.6) before implementing items 3-5.Any Additional AI Details you'd like to share:
Investigation, plan,
/debateconsult, and implementation were all drafted by Claude under my direction. I reviewed the scope decisions (which of the issue's 6 items to bundle into one PR, the docs-only vs. full-OpenAPI call, the callback semantics, the idempotency and readiness definitions) and the/debatesynthesis before any code was written.Summary
state=live|trashed|allfilters toGET /api/sessions.wait=readyfor session creation.Benefits
External work-queue dispatchers can filter sessions, receive safe status updates, retry requests without creating duplicates, and wait for session readiness. Existing unfiltered session listing behavior remains unchanged.
Further enhancement
The TUI file-watch issue remains outside this change and should be addressed separately.