Skip to content

feat(sessions): consume flattened gateway classification - #1069

Open
RomneyDa wants to merge 3 commits into
mainfrom
feat/flatten-session-classification
Open

feat(sessions): consume flattened gateway classification#1069
RomneyDa wants to merge 3 commits into
mainfrom
feat/flatten-session-classification

Conversation

@RomneyDa

@RomneyDa RomneyDa commented Jul 30, 2026

Copy link
Copy Markdown
Member

What Problem This Solves\n\nWindows sessions cannot use the new Gateway-derived facts without decoding an unshipped nested session object. That object duplicates existing title fields and makes the app display policy depend on Gateway-owned strings.\n\n## Why This Change Was Made\n\nThis removes the nested session DTO and consumes the Gateway flattened classification, agentId, accountId, peerKind, isMain, and isBackground fields directly. Windows retains its local, localized title resolver and suppresses transport-derived direct-peer display names before rendering them.\n\n## User Impact\n\nWhen connected to the updated Gateway, the Windows app categorizes and filters sessions from canonical derived facts while keeping its own display and localization policy. Older Gateways continue through the existing bounded key fallback.\n\n## Evidence\n\n- Companion to https://github.com/openclaw/openclaw/pull/106832.\n- Fresh Parallels Windows app-snapshot validation passed on final head 9e2b337.\n- The parser and display resolver cover flat session facts, sparse updates, direct-peer privacy, title formatting, tray filtering, and chat projection.\n- Rubber-duck / independent autoreview: clean; the final fallback correction preserved older-Gateway titles when classification is absent.\n\n## Change Type\n\n- [ ] Bug fix\n- [x] Feature\n- [x] Refactor\n- [ ] Docs or instructions\n- [x] Tests or validation\n- [x] Security hardening\n- [ ] Chore or infrastructure\n\n## Scope\n\n- [x] Tray or WinUI UX\n- [ ] Windows node capability\n- [ ] Local MCP or winnode\n- [x] Gateway, connection, or pairing\n- [ ] Setup or onboarding\n- [x] Permissions, privacy, or security\n- [x] Tests, CI, or docs\n\n## Validation\n\nWindows 11 ARM64 Parallels app snapshot pre-openclaw-windows-app-e2e-2026-07-15:\n\n- ./build.ps1: passed, 0 warnings, 0 errors.\n- dotnet test ./tests/OpenClaw.Shared.Tests/OpenClaw.Shared.Tests.csproj --no-restore: passed; 3,381 passed, 32 skipped, 0 failed.\n- dotnet test ./tests/OpenClaw.Tray.Tests/OpenClaw.Tray.Tests.csproj --no-restore: passed; 2,022 passed, 0 skipped, 0 failed.\n\n## Real Behavior Proof\n\n- Environment tested: Windows 11 ARM64 Parallels app snapshot.\n- PR head or commit tested: 9e2b337.\n- Exact command: OPENCLAW_REPO=/Users/dallin/dev/worktrees/openclaw/session-ux ./scripts/parallels-windows-vm.sh run-tests --ref feat/flatten-session-classification.\n- Evidence after fix: the app parser reads only the six flattened fields; direct-peer display names are never selected as a title when peerKind is direct; absent flat fields retain the bounded legacy-key fallback.\n- Observed result: complete build and both required test projects passed.\n- Screenshot or artifact links verified? N/A — no visual layout change.\n- Not verified or blocked: no live Gateway is configured in the restored app snapshot; protocol-to-Windows behavior is proven through the final parser/display and native suites.\n\n## Security Impact\n\n- New permissions or capabilities? No\n- Secrets or tokens handling changed? No\n- New or changed network calls? No\n- Command or tool execution surface changed? No\n- Data access scope changed? No\n\n## Compatibility and Migration\n\n- Backward compatible? Yes\n- Config or environment changes? No\n- Migration needed? No\n\n## Review Conversations\n\n- [x] I replied to or resolved every bot review conversation addressed by this PR.\n- [x] I left unresolved only conversations that still need maintainer judgment.

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. labels Jul 30, 2026
@clawsweeper

clawsweeper Bot commented Jul 30, 2026

Copy link
Copy Markdown

Codex review: needs changes before merge. Reviewed August 1, 2026, 3:18 PM ET / 19:18 UTC.

ClawSweeper review

What this changes

This PR migrates Windows session parsing and tray/chat display logic from a nested Gateway presentation object to flattened session classification fields and a local display resolver.

Merge readiness

Blocked until stronger real behavior proof is added - 10 items remain

This PR is still necessary but needs changes before merge. It removes a currently supported nested Gateway session payload without retaining that payload as a fallback, and its direct-session privacy guard still accepts DerivedTitle, which can expose a peer-derived identifier. Likely related people: RomneyDa (high confidence) and Karen (medium confidence).

Priority: P1
Reviewed head: 9e2b337f4ce75a13164aca66cedad8333d6d5cd3

Review scores

Measure Result What it means
Overall readiness 🦪 silver shellfish (2/6) The PR has substantive focused implementation and reported test runs, but two P1 correctness boundaries and missing end-to-end Gateway evidence keep it below merge readiness.
Proof confidence 🦪 silver shellfish (2/6) Needs stronger real behavior proof before merge: The reported Windows build and native test suites are useful supplemental evidence, but the PR body explicitly lacks a live Gateway and supplies no artifact showing a flattened session payload becoming a safe Windows tray/chat title at the current head. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Patch quality 🦪 silver shellfish (2/6) Security review found an item that needs attention.

Verification

Check Result Evidence
Real behavior Needs proof Needs stronger real behavior proof before merge: The reported Windows build and native test suites are useful supplemental evidence, but the PR body explicitly lacks a live Gateway and supplies no artifact showing a flattened session payload becoming a safe Windows tray/chat title at the current head. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed 6 items Current main still consumes the legacy payload: Current main parses presentation, stores it on the session, and falls back to its channel when the top-level field is absent. The PR removes that model and parsing path, so a server that sends only the existing nested shape loses its structured title and classification facts rather than receiving equivalent fallback behavior.
Direct title fallback remains unsafe: At the PR head, direct sessions skip DisplayName but still unconditionally accept DerivedTitle before generating a safe classification title. The direct-session regression test covers only DisplayName, not a populated peer-derived DerivedTitle.
Protocol snapshot duplicates isMain: The updated sessions.list response-field snapshot lists isMain once at the beginning and again among the newly flattened fields, leaving the protocol contract declaration internally inconsistent.
Findings 3 actionable findings [P1] Retain a bounded nested-presentation fallback
[P1] Suppress derived titles for direct sessions
[P2] Declare isMain once in the protocol snapshot
Security Needs attention Block peer-derived direct-session titles: DerivedTitle is accepted after the direct-session DisplayName guard, so a Gateway-supplied direct-peer identifier can be shown in tray or chat titles.

How this fits together

Gateway session payloads are parsed into shared Windows session models, then tray and chat surfaces derive titles, filtering, background state, and agent metadata. This PR changes the compatibility boundary between Gateway-owned session facts and Windows-local display/privacy policy.

flowchart LR
  Gateway[Gateway session payload] --> Parser[Windows session parser]
  Parser --> Facts[Session facts and fallbacks]
  Facts --> Resolver[Local display resolver]
  Resolver --> Privacy[Direct-peer privacy decision]
  Privacy --> Tray[Tray session lists]
  Privacy --> Chat[Chat thread projection]
Loading

Before merge

  • Retain a bounded nested-presentation fallback (P1) - Current main still parses the nested presentation object, but this path reads only flattened fields. A Gateway that has not shipped the companion change loses the explicit title, classification, peer kind, agent/account identity, background state, and possibly main-session state despite the PR's backward-compatibility claim. Preserve the legacy shape only when the flattened fields are absent, with regression coverage for both payload versions.
  • Suppress derived titles for direct sessions (P1) - The direct-session branch skips DisplayName, but the next fallback always accepts DerivedTitle. Because this value is Gateway-derived, a direct peer name or phone-like identifier can still reach tray and chat titles. The existing direct-session test does not populate DerivedTitle; block or sanitize it and add that regression case.
  • Declare isMain once in the protocol snapshot (P2) - The updated sessions.list response field list contains isMain twice, once in the original fields and once among the flattened additions. Remove the duplicate so the snapshot remains an unambiguous contract for the parser and protocol guard.
  • Resolve security concern: Block peer-derived direct-session titles - DerivedTitle is accepted after the direct-session DisplayName guard, so a Gateway-supplied direct-peer identifier can be shown in tray or chat titles.
  • Resolve merge risk (P2) - Older Gateways that send only the currently supported nested presentation object will silently lose structured title, classification, agent/account, peer-kind, background, and nested main-session facts if this lands without a bounded legacy parse fallback.
  • Resolve merge risk (P1) - A direct session with a nonempty peer-derived DerivedTitle can render that value in tray and chat because only DisplayName is suppressed.
  • Resolve merge risk (P1) - The protocol snapshot's duplicate isMain entry can leave contract checks and future maintenance unclear even if runtime parsing accepts the field.
  • Improve patch quality - Preserve and test the legacy nested-presentation fallback for an older Gateway payload.
  • Improve patch quality - Add a direct-session regression with a peer-derived DerivedTitle and confirm it never renders.
  • Improve patch quality - Attach redacted current-head Gateway-to-Windows proof showing flattened facts, safe title output, and legacy fallback behavior.

Findings

  • [P1] Retain a bounded nested-presentation fallback — src/OpenClaw.Shared/OpenClawGatewayClient.cs:3839-3845
  • [P1] Suppress derived titles for direct sessions — src/OpenClaw.Shared/Sessions/SessionDisplayResolver.cs:26-31
  • [P2] Declare isMain once in the protocol snapshot — tests/OpenClaw.Shared.Tests/Protocol/gateway-protocol-snapshot.json:72
  • [medium] Block peer-derived direct-session titles — src/OpenClaw.Shared/Sessions/SessionDisplayResolver.cs:28
Agent review details

Security

Needs attention: The patch does not add an execution or dependency surface, but its direct-session display fallback can disclose peer-derived data in user-visible Windows UI.

Review metrics

Metric Value Why it matters
Session contract migration 17 files affected, 378 added, 761 removed The change crosses the shared protocol parser, local display policy, tray, chat, and their tests, so compatibility proof must cover the complete path.
Prior blockers 2 prior findings remain The direct-title privacy path and duplicate protocol field were already reported and are unchanged at the current head.

Merge-risk options

Maintainer options:

  1. Preserve legacy compatibility and direct-peer privacy (recommended)
    Keep the existing nested presentation shape as a fallback only when flattened facts are absent, suppress DerivedTitle for direct sessions, and add regression coverage before merge.
  2. Pause until the Gateway contract is available
    Hold this PR until the companion Gateway protocol change is merged and a maintainer confirms that dropping the legacy nested compatibility path is acceptable.
Copy recommended automerge instruction
@clawsweeper automerge

Special instructions:
Preserve nested presentation parsing only as a lower-priority fallback, prevent DerivedTitle from rendering for direct sessions, remove the duplicate isMain snapshot entry, add regression tests, and provide current-head Gateway-to-Windows proof.

Technical review

Best possible solution:

Prefer flattened fields when present, preserve the existing nested presentation parser as a strictly lower-priority compatibility fallback until the Gateway contract change is shipped, and make all direct-session peer-derived title candidates resolve to the local safe generated title.

Do we have a high-confidence way to reproduce the issue?

Yes, by source: a session with Classification or PeerKind set to direct and a peer-derived DerivedTitle takes the resolver's direct title fallback at the PR head. Current main also demonstrates the legacy nested-presentation path that the PR removes.

Is this the best way to solve the issue?

No. The flattened contract is a reasonable primary path, but deleting the existing nested path contradicts the stated older-Gateway compatibility claim, and the direct-session title policy must cover DerivedTitle as well as DisplayName.

Full review comments:

  • [P1] Retain a bounded nested-presentation fallback — src/OpenClaw.Shared/OpenClawGatewayClient.cs:3839-3845
    Current main still parses the nested presentation object, but this path reads only flattened fields. A Gateway that has not shipped the companion change loses the explicit title, classification, peer kind, agent/account identity, background state, and possibly main-session state despite the PR's backward-compatibility claim. Preserve the legacy shape only when the flattened fields are absent, with regression coverage for both payload versions.
    Confidence: 0.95
    Late finding: first raised on code an earlier review cycle already covered.
  • [P1] Suppress derived titles for direct sessions — src/OpenClaw.Shared/Sessions/SessionDisplayResolver.cs:26-31
    The direct-session branch skips DisplayName, but the next fallback always accepts DerivedTitle. Because this value is Gateway-derived, a direct peer name or phone-like identifier can still reach tray and chat titles. The existing direct-session test does not populate DerivedTitle; block or sanitize it and add that regression case.
    Confidence: 0.99
  • [P2] Declare isMain once in the protocol snapshot — tests/OpenClaw.Shared.Tests/Protocol/gateway-protocol-snapshot.json:72
    The updated sessions.list response field list contains isMain twice, once in the original fields and once among the flattened additions. Remove the duplicate so the snapshot remains an unambiguous contract for the parser and protocol guard.
    Confidence: 0.99

Overall correctness: patch is incorrect
Overall confidence: 0.97

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 8ac00c5f4977.

Labels

Label changes:

  • add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The reported Windows build and native test suites are useful supplemental evidence, but the PR body explicitly lacks a live Gateway and supplies no artifact showing a flattened session payload becoming a safe Windows tray/chat title at the current head. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • remove status: ⏳ waiting on author: Current PR status label is status: 📣 needs proof.

Label justifications:

  • P1: The PR can expose direct-peer identifiers and regress existing Gateway session display behavior for active users.
  • merge-risk: 🚨 compatibility: It replaces a currently parsed session payload shape while claiming older-Gateway fallback behavior.
  • merge-risk: 🚨 security-boundary: Gateway-derived direct-session fields cross into user-visible tray and chat title rendering.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦪 silver shellfish.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The reported Windows build and native test suites are useful supplemental evidence, but the PR body explicitly lacks a live Gateway and supplies no artifact showing a flattened session payload becoming a safe Windows tray/chat title at the current head. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence

Security concerns:

  • [medium] Block peer-derived direct-session titles — src/OpenClaw.Shared/Sessions/SessionDisplayResolver.cs:28
    DerivedTitle is accepted after the direct-session DisplayName guard, so a Gateway-supplied direct-peer identifier can be shown in tray or chat titles.
    Confidence: 0.99

Acceptance criteria:

  • [P1] ./build.ps1.
  • [P1] dotnet test ./tests/OpenClaw.Shared.Tests/OpenClaw.Shared.Tests.csproj --no-restore.
  • [P1] dotnet test ./tests/OpenClaw.Tray.Tests/OpenClaw.Tray.Tests.csproj --no-restore.
  • [P1] Focused regression coverage for flat direct sessions, nested-only legacy sessions, and flattened-field precedence.

What I checked:

  • Current main still consumes the legacy payload: Current main parses presentation, stores it on the session, and falls back to its channel when the top-level field is absent. The PR removes that model and parsing path, so a server that sends only the existing nested shape loses its structured title and classification facts rather than receiving equivalent fallback behavior. (src/OpenClaw.Shared/OpenClawGatewayClient.cs:3862, 8ac00c5f4977)
  • Direct title fallback remains unsafe: At the PR head, direct sessions skip DisplayName but still unconditionally accept DerivedTitle before generating a safe classification title. The direct-session regression test covers only DisplayName, not a populated peer-derived DerivedTitle. (src/OpenClaw.Shared/Sessions/SessionDisplayResolver.cs:28, 9e2b337f4ce7)
  • Protocol snapshot duplicates isMain: The updated sessions.list response-field snapshot lists isMain once at the beginning and again among the newly flattened fields, leaving the protocol contract declaration internally inconsistent. (tests/OpenClaw.Shared.Tests/Protocol/gateway-protocol-snapshot.json:72, 9e2b337f4ce7)
  • Feature provenance: git blame attributes the current main session presentation resolver to the readable-session-name work, establishing that the removed compatibility behavior is an intentional current-main feature rather than dead code. (src/OpenClaw.Shared/Sessions/SessionPresentationResolver.cs:16, 959b3527ac7b)
  • Not implemented on main or released: Current main is 8ac00c5f; it does not contain the PR-head commit, and no release tag contains that commit. The latest supplied release is v0.6.12, so this is not an implemented-on-main cleanup candidate. (9e2b337f4ce7)
  • Current-head proof is incomplete for the changed boundary: The PR body reports Windows build and unit-suite runs but explicitly says no live Gateway was configured. That does not directly prove a flattened Gateway payload reaches the Windows parser and produces the intended safe title behavior. (9e2b337f4ce7)

Likely related people:

  • RomneyDa: The commit signed by Dallin Romney that introduced the current SessionPresentationResolver and readable-session-title behavior is the principal provenance for the code this PR replaces; the same person authored this PR's current head. (role: introduced current session-display behavior and recent area contributor; confidence: high; commits: 959b3527ac7b, 9e2b337f4ce7; files: src/OpenClaw.Shared/Sessions/SessionPresentationResolver.cs, src/OpenClaw.Shared/Sessions/SessionDisplayResolver.cs, src/OpenClaw.Tray.WinUI/Services/SessionTitleFormatter.cs)
  • Karen: Current main history shows Karen's chat-surface migration immediately before this review, and this PR changes the session-to-chat projection used by that surface. (role: recent adjacent chat contributor; confidence: medium; commits: 9acafa606a4d; files: src/OpenClaw.Tray.WinUI/Chat/OpenClawChatDataProvider.cs)

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (12 earlier review cycles; latest 8 shown)
  • reviewed 2026-07-30T17:19:08.429Z sha 9e2b337 :: needs changes before merge. :: [P1] Block derived titles for direct sessions | [P2] Remove the duplicate isMain protocol field
  • reviewed 2026-07-31T22:39:29.929Z sha 9e2b337 :: needs changes before merge. :: [P1] Block derived titles for direct sessions | [P2] Remove the duplicate isMain snapshot field
  • reviewed 2026-08-01T01:55:13.913Z sha 9e2b337 :: found issues before merge. :: [P1] Block derived titles for direct sessions | [P2] Remove the duplicate isMain snapshot field
  • reviewed 2026-08-01T10:20:21.675Z sha 9e2b337 :: found issues before merge. :: [P1] Suppress derived titles for direct sessions | [P2] Remove the duplicate isMain snapshot entry
  • reviewed 2026-08-01T12:21:32.163Z sha 9e2b337 :: needs changes before merge. :: [P1] Suppress derived titles for direct sessions | [P2] Remove the duplicate isMain snapshot entry
  • reviewed 2026-08-01T13:53:25.201Z sha 9e2b337 :: needs changes before merge. :: [P1] Suppress derived titles for direct sessions | [P2] Remove the duplicate isMain snapshot field
  • reviewed 2026-08-01T16:40:28.011Z sha 9e2b337 :: needs changes before merge. :: [P1] Suppress derived titles for direct sessions | [P2] Remove the duplicate isMain snapshot field
  • reviewed 2026-08-01T17:57:52.696Z sha 9e2b337 :: found issues before merge. :: [P1] Suppress derived titles for direct sessions | [P2] Remove the duplicate isMain snapshot field

@clawsweeper clawsweeper Bot added P1 Urgent regression or broken agent/channel workflow affecting real users now. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. and removed P2 Normal priority bug or improvement with limited blast radius. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. labels Jul 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. P1 Urgent regression or broken agent/channel workflow affecting real users now. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant