[v0.1/IPC Review] GridDims の三重検証と ResourceReport Optional 集計の語彙を 1.0 で凍結 #146

Closed
opened 2026-09-05 11:57:59 +09:00 by natsukium · 3 comments
Owner

背景

GridDimsResourceReport は proto/IPC の core 型だが、1.0 で凍結すると破壊的変更が不可能になる箇所に「二重検証」と「語彙の紛らわしさ」が残っている。

1. GridDimsadmit / clamp / admit_announced 三重検証

crates/felis-protocol/src/messages.rs: RequestedDimsadmit (create: 0→daemon default, 範囲外は Err GeometryRejection), clamp (resize: 範囲外は clamp), admit_announced (Size/attach 報告: 0 は sentinel ではなく error) の 3 パスを持つ。

  • admitadmit_announced の違いは rows/cols0 を sentinel と見るか error と見るかの 1 bit のみ。
  • clamp は resize 専用だが、InputResizeSessionCreate の両方が GridDims を運ぶため、decode 後に「どちらの検証を呼ぶべきか」は family ではなく verb で決まる。
  • proto/felis.proto コメントは「SpawnArgs.dimsInputResize.dims は receiver が bounds を apply」と書くが、proto 上は同じ GridDims 型で区別がない。

結果、将来「0 を valid geometry にする」変更は 3 関数すべてに影響し、片方だけ直すと他で 0 sentinel が valid にすり替わる。

2. ResourceReport の Optional 集計フィールド

message ResourceReport {
  ResourceKind resource = 1;
  ResourceUnit unit = 2;
  SubjectScope scope = 5;
  uint64 total_used = 6;
  optional uint64 max_subject_used = 7; // DAEMON scope では unset
  optional uint64 per_subject_limit = 8; // DAEMON scope では unset
  optional uint64 global_limit = 9; // scope により unset
}

max_subject_used / per_subject_limit / global_limit は scope により optional で、reference/cli.md Daemon status 表は scope ごとの denominator を「total_used against global_limit, max_subject_used against per_subject_limit」と説明する。

問題: optional の absent と 0 の区別が docs に埋もれており、json 出力で absent は key 自体が消えるため、consumer が 0 (値) と null (absent) を区別しないと「limit 0」 と「limit なし」を誤読する。DAEMON scope で max_subject_used が absent になることは reference/ipc.md「Ops (kind=5)」節に散在するが、proto 上は optional の理由がコメントだけで型に反映されない。

3. SpawnArgsEnvBase の二重 env 語彙

SpawnArgsenv: repeated EnvPair (string key/value)env_base: optional EnvBase (bytes key/value) の二層を持つ。前者は CLI --env KEY=VAL の user-typed UTF-8、後者は relay が捕捉した host-native bytes(Unix raw bytes / Windows UTF-16 LE)。

docs は REQ-912 inherit-then-sanitize と FRLY carrier block の第一候補を説明するが、proto 上で stringbytes の使い分け理由が EnvPair / EnvBytesPair のコメントにしかない。1.0 で語彙を凍結するなら「どちらが UTF-8 保証でどちらが platform-native か」を reference/ipc.md の Session 節の表で凍結すべき。

問い

1.0 で以下を決める:

  1. RequestedDims の 3 関数を admit_create / admit_resize / admit_announced に rename し、proto コメントに「同じ GridDims 型でも検証は verb で決まる」と frozen として明記するか。あるいは SpawnArgs.dimsInputResize.dims を別 message 型 (CreateDims / ResizeDims) に分け、proto 上で sentinel 意味を分離するか。
  2. ResourceReport の optional 3 field を non-optional + 0 = no limit の sentinel に正規化するか、現行の optional のまま reference/ipc.mdreference/cli.md に「absent の意味」表を凍結するか。
  3. SpawnArgs.env / env_basestring vs bytes 使い分けを reference/ipc.md「Session (kind=4)」節の表に凍結として刻むか。

提案

  • 最小 breaking (推奨): RequestedDims の 3 関数を rename せず、コメントと reference/ipc.md「Session (kind=4)」節に「GridDims は wire 上同一だが SpawnArgs.dimsadmit (0 sentinel + refuse), InputResize.dimsclamp (0→min), GridSizeadmit_announced (0 error) の三者で検証が異なる」と frozen 表を追加。ResourceReportoptional のまま残し、reference/cli.md Daemon status 表に「max_subject_usedSUBJECT_SCOPE_DAEMON で absent」行を frozen として追加。SpawnArgs.env / env_base は proto コメントのまま frozen とし、reference/ipc.md に「env は UTF-8 の user-typed, env_base は platform-native bytes」行を追加。
  • 将来の major で GridDims を分離するなら explanation/architecture/ipc.md に Revisit trigger を残す:「0 sentinel を廃止し optional GridDims にするなら major bump」。

どちらも 1.0 で docs に刻むことが目的で、wire 型自体は変えない案。ResourceReport を non-optional に変えるなら BREAKING.mdbase: <sha> が必要になるため、今回は docs 凍結に留める。

判定基準

  • crates/felis-protocol/src/messages.rsRequestedDims 3 関数のコメントと docs/reference/ipc.md「Session (kind=4)」節の表が 1:1 で対応し、新 contributor が「どの検証を呼ぶか」を再発明しないこと。
  • ResourceReport の 3 optional field が reference/ipc.md「Ops (kind=5)」と reference/cli.md Daemon status で同じ「absent の意味」表で説明されていること。
  • SpawnArgs.env / env_basestring vs bytes 区別が reference/ipc.md の Session 節で凍結として読めること。

対象ファイル

  • crates/felis-protocol/src/messages.rs (RequestedDims, GridDims, SpawnArgs)
  • crates/felis-protocol/proto/felis.proto (GridDims, ResourceReport, SpawnArgs, EnvBase)
  • docs/reference/ipc.md「Session (kind=4)」「Ops (kind=5)」「Semantic limits」
  • docs/reference/cli.md「Daemon status」
  • docs/explanation/architecture/ipc.md「Session lifecycle」「Carrier choices」

Parent: #12 および #52 / #16 / #26

## 背景 `GridDims` と `ResourceReport` は proto/IPC の core 型だが、1.0 で凍結すると破壊的変更が不可能になる箇所に「二重検証」と「語彙の紛らわしさ」が残っている。 ### 1. `GridDims` の `admit` / `clamp` / `admit_announced` 三重検証 `crates/felis-protocol/src/messages.rs: RequestedDims` は `admit` (create: `0`→daemon default, 範囲外は `Err GeometryRejection`), `clamp` (resize: 範囲外は clamp), `admit_announced` (Size/attach 報告: `0` は sentinel ではなく error) の 3 パスを持つ。 - `admit` と `admit_announced` の違いは `rows/cols` の `0` を sentinel と見るか error と見るかの 1 bit のみ。 - `clamp` は resize 専用だが、`InputResize` と `SessionCreate` の両方が `GridDims` を運ぶため、decode 後に「どちらの検証を呼ぶべきか」は family ではなく verb で決まる。 - `proto/felis.proto` コメントは「`SpawnArgs.dims` と `InputResize.dims` は receiver が bounds を apply」と書くが、proto 上は同じ `GridDims` 型で区別がない。 結果、将来「`0` を valid geometry にする」変更は 3 関数すべてに影響し、片方だけ直すと他で 0 sentinel が valid にすり替わる。 ### 2. `ResourceReport` の Optional 集計フィールド ```proto message ResourceReport { ResourceKind resource = 1; ResourceUnit unit = 2; SubjectScope scope = 5; uint64 total_used = 6; optional uint64 max_subject_used = 7; // DAEMON scope では unset optional uint64 per_subject_limit = 8; // DAEMON scope では unset optional uint64 global_limit = 9; // scope により unset } ``` `max_subject_used` / `per_subject_limit` / `global_limit` は scope により optional で、`reference/cli.md` Daemon status 表は scope ごとの denominator を「`total_used` against `global_limit`, `max_subject_used` against `per_subject_limit`」と説明する。 問題: `optional` の absent と `0` の区別が docs に埋もれており、`json` 出力で absent は key 自体が消えるため、consumer が `0` (値) と `null` (absent) を区別しないと「limit 0」 と「limit なし」を誤読する。`DAEMON` scope で `max_subject_used` が absent になることは `reference/ipc.md`「Ops (kind=5)」節に散在するが、proto 上は `optional` の理由がコメントだけで型に反映されない。 ### 3. `SpawnArgs` と `EnvBase` の二重 env 語彙 `SpawnArgs` は `env: repeated EnvPair (string key/value)` と `env_base: optional EnvBase (bytes key/value)` の二層を持つ。前者は CLI `--env KEY=VAL` の user-typed UTF-8、後者は relay が捕捉した host-native bytes(Unix raw bytes / Windows UTF-16 LE)。 docs は `REQ-912` inherit-then-sanitize と `FRLY` carrier block の第一候補を説明するが、proto 上で `string` と `bytes` の使い分け理由が `EnvPair` / `EnvBytesPair` のコメントにしかない。1.0 で語彙を凍結するなら「どちらが UTF-8 保証でどちらが platform-native か」を `reference/ipc.md` の Session 節の表で凍結すべき。 ## 問い 1.0 で以下を決める: 1. `RequestedDims` の 3 関数を `admit_create` / `admit_resize` / `admit_announced` に rename し、proto コメントに「同じ `GridDims` 型でも検証は verb で決まる」と frozen として明記するか。あるいは `SpawnArgs.dims` と `InputResize.dims` を別 message 型 (`CreateDims` / `ResizeDims`) に分け、proto 上で sentinel 意味を分離するか。 2. `ResourceReport` の optional 3 field を non-optional + `0 = no limit` の sentinel に正規化するか、現行の `optional` のまま `reference/ipc.md` と `reference/cli.md` に「absent の意味」表を凍結するか。 3. `SpawnArgs.env` / `env_base` の `string` vs `bytes` 使い分けを `reference/ipc.md`「Session (kind=4)」節の表に凍結として刻むか。 ## 提案 - **最小 breaking (推奨)**: `RequestedDims` の 3 関数を rename せず、コメントと `reference/ipc.md`「Session (kind=4)」節に「`GridDims` は wire 上同一だが `SpawnArgs.dims` は `admit` (0 sentinel + refuse), `InputResize.dims` は `clamp` (0→min), `GridSize` は `admit_announced` (0 error) の三者で検証が異なる」と frozen 表を追加。`ResourceReport` は `optional` のまま残し、`reference/cli.md` Daemon status 表に「`max_subject_used` は `SUBJECT_SCOPE_DAEMON` で absent」行を frozen として追加。`SpawnArgs.env` / `env_base` は proto コメントのまま frozen とし、`reference/ipc.md` に「`env` は UTF-8 の user-typed, `env_base` は platform-native bytes」行を追加。 - 将来の major で `GridDims` を分離するなら `explanation/architecture/ipc.md` に Revisit trigger を残す:「`0` sentinel を廃止し `optional GridDims` にするなら major bump」。 どちらも 1.0 で docs に刻むことが目的で、wire 型自体は変えない案。`ResourceReport` を non-optional に変えるなら `BREAKING.md` に `base: <sha>` が必要になるため、今回は docs 凍結に留める。 ## 判定基準 - `crates/felis-protocol/src/messages.rs` の `RequestedDims` 3 関数のコメントと `docs/reference/ipc.md`「Session (kind=4)」節の表が 1:1 で対応し、新 contributor が「どの検証を呼ぶか」を再発明しないこと。 - `ResourceReport` の 3 optional field が `reference/ipc.md`「Ops (kind=5)」と `reference/cli.md` Daemon status で同じ「absent の意味」表で説明されていること。 - `SpawnArgs.env` / `env_base` の `string` vs `bytes` 区別が `reference/ipc.md` の Session 節で凍結として読めること。 ## 対象ファイル - `crates/felis-protocol/src/messages.rs` (`RequestedDims`, `GridDims`, `SpawnArgs`) - `crates/felis-protocol/proto/felis.proto` (`GridDims`, `ResourceReport`, `SpawnArgs`, `EnvBase`) - `docs/reference/ipc.md`「Session (kind=4)」「Ops (kind=5)」「Semantic limits」 - `docs/reference/cli.md`「Daemon status」 - `docs/explanation/architecture/ipc.md`「Session lifecycle」「Carrier choices」 Parent: #12 および #52 / #16 / #26
Author
Owner

Triage (2026-09-05)

Verdict: accept as proposed (docs-only freeze), P1. No wire change: the three RequestedDims paths, the optional semantics of ResourceReport, and the env (UTF-8, user-typed) vs env_base (platform-native bytes) split get frozen tables in reference/ipc.md and reference/cli.md, with a Revisit trigger in explanation/architecture/ipc.md for the 0-sentinel major bump. The JSON absent-key vs 0 point for ResourceReport also belongs in the daemon status section of reference/cli.md, since machine consumers read it there. Parent: #52.

## Triage (2026-09-05) **Verdict: accept as proposed (docs-only freeze), P1.** No wire change: the three `RequestedDims` paths, the `optional` semantics of `ResourceReport`, and the `env` (UTF-8, user-typed) vs `env_base` (platform-native bytes) split get frozen tables in `reference/ipc.md` and `reference/cli.md`, with a Revisit trigger in `explanation/architecture/ipc.md` for the `0`-sentinel major bump. The JSON absent-key vs `0` point for `ResourceReport` also belongs in the `daemon status` section of `reference/cli.md`, since machine consumers read it there. Parent: #52.
Author
Owner

リリース前なので最小breakingにこだわる必要はない

リリース前なので最小breakingにこだわる必要はない
Author
Owner

Triage plan (2026-09-05)

Source-grounded triage against main at 07a77ae4, reviewed through six rounds of an independent reviewer (pi luna then sol); every finding was verified against the source and folded in, and the design decisions below were settled with the maintainer on 2026-09-05. The order that supersedes the tracker's is posted on #12: this issue lands before #29 (schemas, goldens, and reference tables freeze there) and, because it breaks the wire, before #50.

GridDims sentinels, ResourceReport optionals, env/env_base

Claim check

  • messages.rs:231-275 RequestedDims::{admit, admit_announced, clamp}:
    the only difference between admit and admit_announced is whether
    rows/cols 0 means "daemon default" (create) or is an error
    (announced). Pixel 0 = unknown in all three.
  • ResourceReport (felis.proto:1619-1640): the three optionals carry
    presence deliberately and the comments say when each is unset;
    ipc.md:1164-1176 repeats it; cli.md:183-220 does not say that an
    absent field is an omitted JSON key.
  • EnvPair (string) vs EnvBytesPair/EnvBase (bytes):
    felis.proto:453-481, comments only.

Verdict

accept, with one wire change per the maintainer's note.

**Decision (dims) — settled 2026-09-05: make SpawnArgs.dims an
optional GridDims and drop the rows/cols 0 sentinel from creates.
Absent = the daemon default; present = every axis must be in band
(pixel 0 = unknown stays, because a headless create genuinely has no
pixel size). admit and admit_announced then collapse into one
admit (create and announced), leaving two paths: admit (reject) and
clamp (live resize). Wire cost: a message field already has presence in
proto3, so optional on it does not change encoding; the semantic
break (a dims { rows: 0 } create is now refused instead of defaulted)
gets the base: line. Alternative: keep the sentinel and rename the
functions (admit_create/admit_resize/admit_announced), docs-only.
Rejected because one type with three checks is exactly the trap the
issue names, and the fix is a presence bit the wire already has.

ResourceReport: keep optional. A 0 = no limit sentinel would
reintroduce the ambiguity the issue objects to elsewhere; presence is the
right encoding. Docs state it. env/env_base: docs only.

Approach

  • felis.proto: optional GridDims dims = 5 in SpawnArgs, comment
    rewritten; GridDims comment states zero validity per carrying
    field
    (create: absent = default, present rows/cols must be in band;
    InputResize: any value, clamped; announced: rows/cols 0 refused;
    pixel 0 = unknown everywhere), not a global rule; just proto.
    BREAKING.md base: line whose prose names the semantic break buf
    cannot see: an old client's rows: 0 create is now refused, and a new
    client's absent dims makes an old daemon's converter return
    MissingField, which ends the connection rather than refusing the
    create — the same rebuild-together posture as #144.
  • messages.rs: SpawnArgs.dims: Option<RequestedDims>; delete
    admit_announced, admit takes the announced rule (the Kani harness
    messages.rs:627-651 is re-pointed at admit, same property); the
    daemon's one create path (serve.rs:922 create_session, shared by
    SessionMsg::Create and OpsMsg::Spawn) maps None → default dims; CLI
    sessions spawn --rows/--cols (verify the flag names) sends None when
    neither is given, refuses a partial pair locally (usage, exit 2); the
    bridge builds SpawnArgs in two places (cli_bridge.rs:1168-1183,
    :1616-1629) with rows/cols independently optional — both send
    None when neither is given and answer a partial pair with a local
    invalid_request, tested in the bridge goldens.
  • docs/reference/ipc.md "Session (kind = 4)": a table for GridDims
    per carrying field (SpawnArgs.dims: absent = default, present =
    admitted; InputResize.dims: clamped; GridSize/SessionInfo.dims:
    admitted); "Ops (kind = 5)": absent-vs-0 table for the three
    ResourceReport optionals; env (UTF-8, user-typed, applied last) vs
    env_base (platform-native bytes, replaces the base) rows.
  • docs/reference/cli.md "Daemon status": "an unset field is an omitted
    key in --format json; 0 is a measured zero"; sessions spawn
    geometry flags: partial pair is usage.
  • docs/explanation/architecture/ipc.md: why presence, not sentinels,
    for both dims and resource ceilings; Revisit trigger for pixel 0.
  • docs/reference/spec.md REQ-605a (the create sentinel is part of its
    text) and its owner, docs/explanation/architecture/session-lifecycle.md
    "Geometry bounds"; skills/felis/SKILL.md (sessions spawn / bridge
    sessions.spawn geometry params); CHANGELOG.md.
  • Tests: proto round-trip with absent dims; daemon create with
    None/partial/in-band/out-of-band; CLI partial-pair refusal; the Kani
    harness (messages.rs:627-651) re-pointed at admit. The existing
    geometry tests encode the old sentinel and must change with it:
    messages.rs:1030 ("sentinels survive", an all-zero create admitted)
    becomes a pixel-only sentinel case, and
    crates/felis-protocol/tests/proptest_geometry.rs:59-133in_bounds
    draws zero rows/cols today and the offender classification treats
    every zero axis as valid — draws rows/cols only in MIN..=MAX, keeps
    zero for pixel axes, and classifies offenders per axis.

Dependencies / risk / labels

Before #50 and #29; can share a PR with #144 only if the maintainer wants
one base: line, otherwise separate. M. Keep labels. Parent #52.

## Triage plan (2026-09-05) Source-grounded triage against `main` at `07a77ae4`, reviewed through six rounds of an independent reviewer (`pi` luna then sol); every finding was verified against the source and folded in, and the design decisions below were settled with the maintainer on 2026-09-05. The order that supersedes the tracker's is posted on #12: this issue lands before #29 (schemas, goldens, and reference tables freeze there) and, because it breaks the wire, before #50. **`GridDims` sentinels, `ResourceReport` optionals, `env`/`env_base`** ### Claim check - `messages.rs:231-275` `RequestedDims::{admit, admit_announced, clamp}`: the only difference between `admit` and `admit_announced` is whether rows/cols `0` means "daemon default" (create) or is an error (announced). Pixel `0` = unknown in all three. - `ResourceReport` (`felis.proto:1619-1640`): the three optionals carry presence deliberately and the comments say when each is unset; `ipc.md:1164-1176` repeats it; `cli.md:183-220` does not say that an absent field is an omitted JSON key. - `EnvPair` (string) vs `EnvBytesPair`/`EnvBase` (bytes): `felis.proto:453-481`, comments only. ### Verdict **accept, with one wire change per the maintainer's note.** **Decision (dims) — settled 2026-09-05: make `SpawnArgs.dims` an `optional GridDims` and drop the rows/cols `0` sentinel from creates. Absent = the daemon default; present = every axis must be in band (pixel `0` = unknown stays, because a headless create genuinely has no pixel size). `admit` and `admit_announced` then collapse into one `admit` (create and announced), leaving two paths: `admit` (reject) and `clamp` (live resize). Wire cost: a message field already has presence in proto3, so `optional` on it does not change encoding; the *semantic* break (a `dims { rows: 0 }` create is now refused instead of defaulted) gets the `base:` line. Alternative: keep the sentinel and rename the functions (`admit_create`/`admit_resize`/`admit_announced`), docs-only. Rejected because one type with three checks is exactly the trap the issue names, and the fix is a presence bit the wire already has. `ResourceReport`: keep `optional`. A `0 = no limit` sentinel would reintroduce the ambiguity the issue objects to elsewhere; presence is the right encoding. Docs state it. `env`/`env_base`: docs only. ### Approach - `felis.proto`: `optional GridDims dims = 5` in `SpawnArgs`, comment rewritten; `GridDims` comment states zero validity *per carrying field* (create: absent = default, present rows/cols must be in band; `InputResize`: any value, clamped; announced: rows/cols `0` refused; pixel `0` = unknown everywhere), not a global rule; `just proto`. `BREAKING.md` `base:` line whose prose names the semantic break `buf` cannot see: an old client's `rows: 0` create is now refused, and a new client's absent `dims` makes an old daemon's converter return `MissingField`, which ends the connection rather than refusing the create — the same rebuild-together posture as #144. - `messages.rs`: `SpawnArgs.dims: Option<RequestedDims>`; delete `admit_announced`, `admit` takes the announced rule (the Kani harness `messages.rs:627-651` is re-pointed at `admit`, same property); the daemon's one create path (`serve.rs:922` `create_session`, shared by `SessionMsg::Create` and `OpsMsg::Spawn`) maps `None` → default dims; CLI `sessions spawn --rows/--cols` (verify the flag names) sends `None` when neither is given, refuses a partial pair locally (usage, exit 2); the bridge builds `SpawnArgs` in two places (`cli_bridge.rs:1168-1183`, `:1616-1629`) with `rows`/`cols` independently optional — both send `None` when neither is given and answer a partial pair with a local `invalid_request`, tested in the bridge goldens. - `docs/reference/ipc.md` "Session (kind = 4)": a table for `GridDims` per carrying field (`SpawnArgs.dims`: absent = default, present = admitted; `InputResize.dims`: clamped; `GridSize`/`SessionInfo.dims`: admitted); "Ops (kind = 5)": absent-vs-`0` table for the three `ResourceReport` optionals; `env` (UTF-8, user-typed, applied last) vs `env_base` (platform-native bytes, replaces the base) rows. - `docs/reference/cli.md` "Daemon status": "an unset field is an omitted key in `--format json`; `0` is a measured zero"; `sessions spawn` geometry flags: partial pair is `usage`. - `docs/explanation/architecture/ipc.md`: why presence, not sentinels, for both dims and resource ceilings; Revisit trigger for pixel `0`. - `docs/reference/spec.md` REQ-605a (the create sentinel is part of its text) and its owner, `docs/explanation/architecture/session-lifecycle.md` "Geometry bounds"; `skills/felis/SKILL.md` (`sessions spawn` / bridge `sessions.spawn` geometry params); `CHANGELOG.md`. - Tests: proto round-trip with absent dims; daemon create with `None`/partial/in-band/out-of-band; CLI partial-pair refusal; the Kani harness (`messages.rs:627-651`) re-pointed at `admit`. The existing geometry tests encode the old sentinel and must change with it: `messages.rs:1030` ("sentinels survive", an all-zero create admitted) becomes a pixel-only sentinel case, and `crates/felis-protocol/tests/proptest_geometry.rs:59-133` — `in_bounds` draws zero rows/cols today and the offender classification treats every zero axis as valid — draws rows/cols only in `MIN..=MAX`, keeps zero for pixel axes, and classifies offenders per axis. ### Dependencies / risk / labels Before #50 and #29; can share a PR with #144 only if the maintainer wants one `base:` line, otherwise separate. **M.** Keep labels. Parent #52.
Sign in to join this conversation.
No description provided.