natsukium/felis
Watch
1
Code Issues 27 Pull requests 1 Projects Releases Packages Wiki Activity Actions

[v0.1/P1] Track retarget completion with operation IDs #24

New issue
Closed
opened 2026-09-03 16:18:05 +09:00 by natsukium · 2 comments
natsukium commented 2026-09-03 16:18:05 +09:00
Owner
Copy link

Parent: #12 (P1.5). Supersedes the completion question in #6.

Why

Switch and retarget commands report accepted when a push is only queued. Dial, authentication, attach, and supersession happen later, so the field promises a stronger completion boundary than exists.

Scope

  • Rename immediate accepted results to queued.
  • Return an operation id for each retarget operation.
  • Add optional --wait reporting landed, failed, or superseded per target attachment.
  • Bound wait time explicitly.
  • Keep timed-out operations queryable by id so timeout does not erase the eventual verdict.

Acceptance criteria

  • Non-waiting success means only successful queue admission and says queued.
  • Waiting returns a terminal verdict for every selected target or a queryable timeout.
  • Superseded operations cannot later be reported as landed.
  • Operation retention and query limits are bounded.
  • IPC, CLI machine output, lifecycle docs, and golden tests agree.
Parent: #12 (P1.5). Supersedes the completion question in #6. ## Why Switch and retarget commands report `accepted` when a push is only queued. Dial, authentication, attach, and supersession happen later, so the field promises a stronger completion boundary than exists. ## Scope - Rename immediate `accepted` results to `queued`. - Return an operation id for each retarget operation. - Add optional `--wait` reporting `landed`, `failed`, or `superseded` per target attachment. - Bound wait time explicitly. - Keep timed-out operations queryable by id so timeout does not erase the eventual verdict. ## Acceptance criteria - [ ] Non-waiting success means only successful queue admission and says `queued`. - [ ] Waiting returns a terminal verdict for every selected target or a queryable timeout. - [ ] Superseded operations cannot later be reported as landed. - [ ] Operation retention and query limits are bounded. - [ ] IPC, CLI machine output, lifecycle docs, and golden tests agree.
natsukium commented 2026-09-03 20:35:14 +09:00
Author
Owner
Copy link

Triage plan (2026-09-03)

Source-grounded triage against main at 69076d42, reviewed through seven rounds of an independent reviewer (pi sol/luna) until it passed with no findings. The dependency order that supersedes the tracker's is posted on #12. Where a "Review amendments" section below conflicts with an earlier section, the amendment is the decision.

Claim check

Accurate, and the current state is a recorded decision, not an oversight.

  • crates/felis-protocol/proto/felis.proto:1310-1329 OpsSwitched.accepted: "Acceptance, not landing ... Operation ids that collect landed/failed/superseded are deliberately deferred; the envelope leaves room for them as an additive minor."
  • crates/felis-cli/src/cli_sessions.rs:1659-1666 and :1722-1727 ("Accepted, not landed: each window now runs its own attach"); the not_accepted error kind at crates/felis-cli/src/cli_output.rs:143, :186, listed in the closed set at docs/reference/cli.md:109.
  • docs/reference/ipc.md:834-838 and docs/explanation/architecture/ipc.md "Acceptance, not landing": deferral with a Revisit if: "a script that retargets a window and then acts on the window's new home".
  • The revisit trigger has partial evidence: skills/felis/SKILL.md:343-354 tells an agent not to read accepted as "the move was accepted" and to "confirm with sessions info", i.e. the polling workaround for exactly this gap, written for the product's own agent consumer.
  • What the daemon can and cannot observe is fixed by the wire: a same-daemon switch lands as a new connection sending Session::Attach { id } (crates/felis-client-core/src/connector.rs AttachSessionReq), which the daemon could correlate; a carrier retarget (PushRetargetHost, felis.proto:1615) lands on another daemon, which the origin daemon can never observe (docs/reference/ipc.md:816-820: "this daemon cannot resolve another daemon's roster").

Verdict

accept-with-changes, split into a v0.1 part and a post-v0.1 part.

The recorded reason for deferring ("needs the daemon to model the client's post-push lifecycle") is still true for failed: a window that fails to land has no connection to the origin daemon left to report on, so failed is only ever inferable by timeout. landed and superseded, by contrast, are observable inside the session actor with one field on the wire. And --wait for carrier targets is unobservable by construction and should be refused, not emulated.

v0.1 (freeze-relevant, small):

  1. Rename accepted → queued in OpsSwitched, the CLI/bridge objects, and not_accepted → not_queued (coordinated under #23, which owns the error.kind set).
  2. Mint an operation_id per push in the session actor and carry it on OpsSwitched, on PushReattach / PushRetargetHost, and as an optional field on SessionAttach, so a landing can be attributed later. This is the part that is expensive after the tag (a missing echo field means the verdict can never be attributed) and cheap before it.
  3. Record landed for same-daemon targets in the session actor (attach carrying an operation_id marks it), superseded when a later push targets the same attachment before it landed. Bounded ring (e.g. 256 operations daemon-wide), oldest evicted.

post-v0.1 (or in v0.1 if the above lands early): --wait <timeout> and Ops::OperationStatus { id } query with landed | superseded | pending; failed is reported only as "the attachment's superseded connection closed and nothing landed within the wait", and documented as inferred.

Principle check: this grows the IPC vocabulary with typed data (principle 1 explicitly allows it) and adds no heuristic. The "Revisit if" trigger is at least half met by the agent skill's own polling advice.

Approach

Protocol (felis.proto, messages.rs, convert.rs)

  • OpsSwitched: accepted → queued; add uint64 operation_id. PushReattach / PushRetargetHost: add operation_id. SessionAttach: add optional uint64 operation_id (echo; must not be a Correlation, which #48 makes an exclusive request/stream identity). Later: OpsOperationStatus { id } / OpsOperationReport { oneof verdict { Pending; Landed { attachment }; Superseded { by }; } }.

Daemon (crates/felis-daemon/src/serve/session_task.rs)

  • Push enqueue (:1170-1200, the accepted += 1 loop): mint operation_id from a daemon-wide AtomicU64, stamp it on the pushed frame, and record {op, attachment_id, state: Pending} in a VecDeque on the pool (bounded). Any newer push to the same attachment_id sets the older op Superseded.
  • SubscribeReq (serve.rs:571-580) carries operation_id; on a successful subscribe the actor marks it Landed if it is still Pending.
  • route_ops: OperationStatus lookup (post-v0.1).

Client core / CLI / bridge

  • connector.rs attach(id, intent) gains operation_id: Option<u64>; the GUI's reattach push handler (event_handler.rs RetargetRequested/reattach path) threads the id from the push into the landing.
  • cli_sessions.rs:1600-1730: output objects gain operation_id; --wait (post-v0.1) polls OperationStatus with a bounded deadline and prints per-attachment verdicts.

Docs cascade

  • docs/reference/ipc.md:801-840 (Switch/Switched, the accepted paragraph), Push section, minor ledger or 2.0 base; docs/explanation/architecture/ipc.md "Acceptance, not landing" (rewrite: what became observable, why failed stays inferred, why carrier targets never get a verdict); docs/reference/cli.md:175-177, :476-483, :535 (accepted → queued, operation_id), :109 error kinds; docs/reference/control-surfaces.md verb table; skills/felis/SKILL.md:327-354, :397-399; CHANGELOG.md.

Tests

  • session_task.rs tests beside :3640: push then attach-with-id → Landed; two pushes to one attachment → first Superseded, second Pending; a landed op can never later flip to superseded (state machine is monotone: Pending → Landed | Superseded).
  • Ring bound: N+1 ops evict the oldest and a query for it is no_match.
  • CLI golden: switch --format json object shape with queued and operation_id; not_queued error.

Dependencies

  • #48 first (decide that operation_id is a plain field, not correlation) and #47 (arm placement of OperationStatus).
  • #23 (rename lands with the contract simplification; one rename of the error set, not two).
  • #22 (a window that closes on exit produces no landing; document that as "the attachment detached" rather than failed).
  • #30 after.
  • Tracker step 6 holds for the v0.1 part.

Risk/effort

M for the v0.1 part (one field on three messages plus a bounded table in the actor); L with --wait/query. Main risk: promising failed and then reporting it on a timeout; the docs must call it inferred, or the contract over-promises exactly as accepted was accused of doing.

Labels

Keep priority/P1, release/v0.1.0 for the rename + operation_id + landed/superseded recording. Move --wait and OperationStatus to a post-v0.1 follow-up issue unless #24 lands before #30 with room to spare; they are additive once the id is on the wire.

Review amendments (round 2)

  • One operation id per OpsSwitch, not per push. SwitchScope::All fans out to several attachments (session_task.rs:1175-1200 loops over targets), so a scalar operation_id on OpsSwitched cannot name per-push ids. The daemon mints exactly one operation_id per OpsSwitch request, stamps the same id on every resulting PushReattach / PushRetargetHost, and records one entry {operation_id, targets: [{attachment_id, state}]} with one verdict per targeted attachment (Pending | Landed | Superseded { by }). OpsSwitched returns {queued, operation_id}; the later OpsOperationReport returns the per-attachment vector.
  • Incarnation-safe identity. The id is {daemon_incarnation: u64 (random, minted at daemon start and reported in Welcome/daemon status), sequence: u64} encoded as one OperationId message (two fixed64 fields), so an id from a previous daemon can never resolve to a later operation; a query carrying a foreign incarnation gets Unknown { reason: OtherIncarnation }. This is decided now, even though the query arm lands post-v0.1.0, because the field shape freezes at the tag.
  • Eviction and post-restart answers are normative. The record ring is bounded (MAX_OPERATION_RECORDS = 256, oldest evicted); a query for an evicted id returns Unknown { reason: Evicted }; after restart every earlier id returns Unknown { reason: OtherIncarnation }. Both are documented in ipc.md and tested.

Review amendments (round 3)

  • Per-target landing attribution. A reattach mints a new attachment id (session_task.rs:944), so an operation id alone cannot say which of several targets landed. Each PushReattach / PushRetargetHost carries {operation_id, target_attachment_id} (the attachment the push was delivered to), and SessionAttach echoes both. The daemon validates the pair against the pending record (operation_id known, target_attachment_id among its targets and still Pending), marks exactly that target Landed { new_attachment_id }, and ignores an unknown or already-settled pair with a debug log. Test: an all-mirror switch with three windows whose landings arrive in reverse order produces three Landed entries each naming the right original attachment.

Review amendments (round 4)

  • Attribution is same-daemon only. For PushRetargetHost the SessionAttach lands on the target daemon, which holds no record; the origin daemon can only ever report Pending (or Superseded) for a carrier target. Scope the {operation_id, target_attachment_id} validation and the reordered-landing test to PushReattach; document carrier targets as unobservable by design (matching the "carrier targets never get a verdict" statement above). A client-to-origin completion report is rejected for v0.1.0 (it would make the client a participant in daemon bookkeeping across hosts); "Revisit if" a cross-host verdict is needed.
## Triage plan (2026-09-03) Source-grounded triage against `main` at `69076d42`, reviewed through seven rounds of an independent reviewer (`pi` sol/luna) until it passed with no findings. The dependency order that supersedes the tracker's is posted on #12. Where a "Review amendments" section below conflicts with an earlier section, the amendment is the decision. ## Claim check Accurate, and the current state is a *recorded* decision, not an oversight. - `crates/felis-protocol/proto/felis.proto:1310-1329` `OpsSwitched.accepted`: "Acceptance, not landing ... Operation ids that collect landed/failed/superseded are deliberately deferred; the envelope leaves room for them as an additive minor." - `crates/felis-cli/src/cli_sessions.rs:1659-1666` and `:1722-1727` ("Accepted, not landed: each window now runs its own attach"); the `not_accepted` error kind at `crates/felis-cli/src/cli_output.rs:143`, `:186`, listed in the closed set at `docs/reference/cli.md:109`. - `docs/reference/ipc.md:834-838` and `docs/explanation/architecture/ipc.md` "Acceptance, not landing": deferral with a *Revisit if*: "a script that retargets a window and then acts on the window's new home". - The revisit trigger has partial evidence: `skills/felis/SKILL.md:343-354` tells an agent not to read `accepted` as "the move was accepted" and to "confirm with `sessions info`", i.e. the polling workaround for exactly this gap, written for the product's own agent consumer. - What the daemon can and cannot observe is fixed by the wire: a same-daemon switch lands as a new connection sending `Session::Attach { id }` (`crates/felis-client-core/src/connector.rs` `AttachSessionReq`), which the daemon *could* correlate; a carrier retarget (`PushRetargetHost`, `felis.proto:1615`) lands on **another daemon**, which the origin daemon can never observe (`docs/reference/ipc.md:816-820`: "this daemon cannot resolve another daemon's roster"). ## Verdict **accept-with-changes**, split into a v0.1 part and a post-v0.1 part. The recorded reason for deferring ("needs the daemon to model the client's post-push lifecycle") is still true for `failed`: a window that fails to land has no connection to the origin daemon left to report on, so `failed` is only ever inferable by timeout. `landed` and `superseded`, by contrast, are observable inside the session actor with one field on the wire. And `--wait` for carrier targets is unobservable by construction and should be refused, not emulated. v0.1 (freeze-relevant, small): 1. Rename `accepted` → `queued` in `OpsSwitched`, the CLI/bridge objects, and `not_accepted` → `not_queued` (coordinated under #23, which owns the `error.kind` set). 2. Mint an `operation_id` per push in the session actor and carry it on `OpsSwitched`, on `PushReattach` / `PushRetargetHost`, and as an optional field on `SessionAttach`, so a landing can be attributed later. This is the part that is expensive after the tag (a missing echo field means the verdict can never be attributed) and cheap before it. 3. Record `landed` for same-daemon targets in the session actor (attach carrying an `operation_id` marks it), `superseded` when a later push targets the same attachment before it landed. Bounded ring (e.g. 256 operations daemon-wide), oldest evicted. post-v0.1 (or in v0.1 if the above lands early): `--wait <timeout>` and `Ops::OperationStatus { id }` query with `landed | superseded | pending`; `failed` is reported only as "the attachment's superseded connection closed and nothing landed within the wait", and documented as inferred. Principle check: this grows the IPC vocabulary with typed data (principle 1 explicitly allows it) and adds no heuristic. The "Revisit if" trigger is at least half met by the agent skill's own polling advice. ## Approach **Protocol (`felis.proto`, `messages.rs`, `convert.rs`)** - `OpsSwitched`: `accepted` → `queued`; add `uint64 operation_id`. `PushReattach` / `PushRetargetHost`: add `operation_id`. `SessionAttach`: add `optional uint64 operation_id` (echo; must *not* be a `Correlation`, which #48 makes an exclusive request/stream identity). Later: `OpsOperationStatus { id }` / `OpsOperationReport { oneof verdict { Pending; Landed { attachment }; Superseded { by }; } }`. **Daemon (`crates/felis-daemon/src/serve/session_task.rs`)** - Push enqueue (`:1170-1200`, the `accepted += 1` loop): mint `operation_id` from a daemon-wide `AtomicU64`, stamp it on the pushed frame, and record `{op, attachment_id, state: Pending}` in a `VecDeque` on the pool (bounded). Any newer push to the same `attachment_id` sets the older op `Superseded`. - `SubscribeReq` (`serve.rs:571-580`) carries `operation_id`; on a successful subscribe the actor marks it `Landed` if it is still `Pending`. - `route_ops`: `OperationStatus` lookup (post-v0.1). **Client core / CLI / bridge** - `connector.rs` `attach(id, intent)` gains `operation_id: Option<u64>`; the GUI's reattach push handler (`event_handler.rs` `RetargetRequested`/reattach path) threads the id from the push into the landing. - `cli_sessions.rs:1600-1730`: output objects gain `operation_id`; `--wait` (post-v0.1) polls `OperationStatus` with a bounded deadline and prints per-attachment verdicts. **Docs cascade** - `docs/reference/ipc.md:801-840` (Switch/Switched, the `accepted` paragraph), Push section, minor ledger or 2.0 base; `docs/explanation/architecture/ipc.md` "Acceptance, not landing" (rewrite: what became observable, why `failed` stays inferred, why carrier targets never get a verdict); `docs/reference/cli.md:175-177`, `:476-483`, `:535` (`accepted` → `queued`, `operation_id`), `:109` error kinds; `docs/reference/control-surfaces.md` verb table; `skills/felis/SKILL.md:327-354`, `:397-399`; `CHANGELOG.md`. **Tests** - `session_task.rs` tests beside `:3640`: push then attach-with-id → `Landed`; two pushes to one attachment → first `Superseded`, second `Pending`; a landed op can never later flip to superseded (state machine is monotone: `Pending → Landed | Superseded`). - Ring bound: N+1 ops evict the oldest and a query for it is `no_match`. - CLI golden: `switch --format json` object shape with `queued` and `operation_id`; `not_queued` error. ## Dependencies - **#48 first** (decide that `operation_id` is a plain field, not correlation) and **#47** (arm placement of `OperationStatus`). - **#23** (rename lands with the contract simplification; one rename of the error set, not two). - **#22** (a window that closes on exit produces no landing; document that as "the attachment detached" rather than `failed`). - **#30** after. - Tracker step 6 holds for the v0.1 part. ## Risk/effort **M** for the v0.1 part (one field on three messages plus a bounded table in the actor); **L** with `--wait`/query. Main risk: promising `failed` and then reporting it on a timeout; the docs must call it inferred, or the contract over-promises exactly as `accepted` was accused of doing. ## Labels Keep `priority/P1`, `release/v0.1.0` for the rename + `operation_id` + `landed/superseded` recording. Move `--wait` and `OperationStatus` to a `post-v0.1` follow-up issue unless #24 lands before #30 with room to spare; they are additive once the id is on the wire. ## Review amendments (round 2) - **One operation id per `OpsSwitch`, not per push.** `SwitchScope::All` fans out to several attachments (`session_task.rs:1175-1200` loops over `targets`), so a scalar `operation_id` on `OpsSwitched` cannot name per-push ids. The daemon mints exactly one `operation_id` per `OpsSwitch` request, stamps the *same* id on every resulting `PushReattach` / `PushRetargetHost`, and records one entry `{operation_id, targets: [{attachment_id, state}]}` with one verdict per targeted attachment (`Pending | Landed | Superseded { by }`). `OpsSwitched` returns `{queued, operation_id}`; the later `OpsOperationReport` returns the per-attachment vector. - **Incarnation-safe identity.** The id is `{daemon_incarnation: u64 (random, minted at daemon start and reported in `Welcome`/`daemon status`), sequence: u64}` encoded as one `OperationId` message (two fixed64 fields), so an id from a previous daemon can never resolve to a later operation; a query carrying a foreign incarnation gets `Unknown { reason: OtherIncarnation }`. This is decided now, even though the query arm lands post-v0.1.0, because the field shape freezes at the tag. - **Eviction and post-restart answers are normative.** The record ring is bounded (`MAX_OPERATION_RECORDS = 256`, oldest evicted); a query for an evicted id returns `Unknown { reason: Evicted }`; after restart every earlier id returns `Unknown { reason: OtherIncarnation }`. Both are documented in `ipc.md` and tested. ## Review amendments (round 3) - **Per-target landing attribution.** A reattach mints a new attachment id (`session_task.rs:944`), so an operation id alone cannot say which of several targets landed. Each `PushReattach` / `PushRetargetHost` carries `{operation_id, target_attachment_id}` (the attachment the push was delivered to), and `SessionAttach` echoes both. The daemon validates the pair against the pending record (`operation_id` known, `target_attachment_id` among its targets and still `Pending`), marks exactly that target `Landed { new_attachment_id }`, and ignores an unknown or already-settled pair with a `debug` log. Test: an all-mirror switch with three windows whose landings arrive in reverse order produces three `Landed` entries each naming the right original attachment. ## Review amendments (round 4) - **Attribution is same-daemon only.** For `PushRetargetHost` the `SessionAttach` lands on the *target* daemon, which holds no record; the origin daemon can only ever report `Pending` (or `Superseded`) for a carrier target. Scope the `{operation_id, target_attachment_id}` validation and the reordered-landing test to `PushReattach`; document carrier targets as unobservable by design (matching the "carrier targets never get a verdict" statement above). A client-to-origin completion report is rejected for v0.1.0 (it would make the client a participant in daemon bookkeeping across hosts); "Revisit if" a cross-host verdict is needed.
natsukium commented 2026-09-19 13:33:54 +09:00
Author
Owner
Copy link

The "Track retarget completion" scope here is resolved by the decision on #296: operation ids leave the v0.1 contract, and queued (queue admission) stays the only completion claim a switch makes. The reasoning is in #296's decision comment (#296 (comment)); the removal lands in #298. No follow-up issue is filed for completion tracking: it is deferred until a consumer that must act on the window's new home appears, which is the recorded revisit trigger in docs/explanation/architecture/ipc.md "What a switch reply can report".

The "Track retarget completion" scope here is resolved by the decision on #296: operation ids leave the v0.1 contract, and `queued` (queue admission) stays the only completion claim a switch makes. The reasoning is in #296's decision comment (https://git.natsukium.com/natsukium/felis/issues/296#issuecomment-2971); the removal lands in #298. No follow-up issue is filed for completion tracking: it is deferred until a consumer that must act on the window's new home appears, which is the recorded revisit trigger in `docs/explanation/architecture/ipc.md` "What a switch reply can report".
Sign in to join this conversation.
No Branch/Tag specified
Branches Tags
main
ci/darwin-smoke
No results found.
Labels
Clear labels   
priority/P0
Must be resolved before the v0.1.0 tag

  
priority/P1
Must be settled before the first public contract freezes

  
priority/P2
Explicitly deferred until after v0.1.0

  
release/v0.1.0
Work tracked for the first tagged release

  
status/blocked
Cannot start now: waits on an upstream release, another issue, or a recorded revisit trigger

  
status/planned
Triaged with a source-grounded plan and a passed pi review; ready to implement

  
type/bug
Shipped behavior that contradicts the docs or the code's own intent; needs a product code change

  
type/design
An open scope or trade-off question; needs a recorded decision in the owning explanation doc before code

  
type/test-gap
Documented or reviewed behavior that no test pins; usually split from a review finding, needs no product code change

  
type/tracker
Parent issue that tracks child work

No labels
priority/P0
priority/P1
priority/P2
release/v0.1.0
status/blocked
status/planned
type/bug
type/design
type/test-gap
type/tracker
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
natsukium/felis#24
No description provided.