[tournierjc]

Summary

Aligns Kinetic Android with the skill workflow from kinetick-code #99 (merged):

• Parse skillPolicy on sessions (mandatory / optional / forbidden, closed)
• Info tab: require / optional / forbid / clear per skill, open/close catalog
• List all pending Skill/Memory drafts (same scope as TUI /skills review), with agent vs user Memory destination and session context
• Approve / edit+approve / reject; surface apply-before-record failures so drafts stay pending and retryable
• Soft-degrade with a clear message when the server answers 404 for these capabilities

HTTP contract (server)

Expects the session server routes from kinetick-code #104 (HTTP follow-up to merged #99; replaces dirty #102):

• POST /sessions/:id/skill-policy
• GET /skills/proposals
• POST /skills/proposals/:id/review

Validation

• ./gradlew testDebugUnitTest — passed
• CI android / build — passed on ea49dcd

  

[tournierjc]

Code Review Summary

Verdict: Comment — CI is green, wire layer is solid, parity scope with #99/#102 looks right. Two state bugs worth fixing before merge, plus minor notes.

⚠️ Warnings

• SessionTabs.kt ~428-442 (reviewProposal success path) — The "apply failed" case (decision == "approve" && result.applied == false) is the PR's headline retry path, but it never calls load() (only the onFailure branch reloads). The card stays visible with a stale "Approve / Edit / Reject" row and no refresh signal; the user must hit Refresh manually, which contradicts the PR body's "drafts stay pending and retryable". Suggestion: call load() in the !result.applied success branch.
• SessionTabs.kt ~365 (load() policy fallback) — if (skillPolicy == null) skillPolicy = session?.skillPolicy fires on every load, not only the first. Consequence: after closing the catalog (closed=true), if a later getSession answers without a skillPolicy key, the state silently resets to the parent-list snapshot (closed=false, stale dispositions) and the catalog toggle flips back — next tap then sends the same closed value (no-op server-side, badge unchanged). Suggestion: only seed from session?.skillPolicy on first load / when sessionId changes.

💡 Suggestions

• SessionTabs.kt ~357 (load cost) — load() now issues 6 sequential requests, and it runs on top of the initial LaunchedEffect load and after every action (each disposition tap, catalog toggle, approve/reject triggers a full reload incl. usage/context/models/skills/forkOptions). Consider parallelizing with coroutineScope { launch { ... } } and/or a lighter refresh path (getSession + knowledgeProposals only) after actions.
• SessionTabs.kt ~446 — The 404 for POST /skills/proposals/:id/review sets proposalsSupported = false, which hides the whole Knowledge-review section even though GET /skills/proposals works on that server (inconsistent degradation vs. the policy 404 path, which keeps the Skills section). Consider a separate reviewSupported flag that disables only the buttons.
• SessionTabs.kt ~860 — The catalog open/close button is gated on policySupported, which only ever becomes false after a 404 attempt. On a server without the routes the button stays enabled and every tap pays a request + error round-trip. A cheap capability probe in load() (or probing once) would let it degrade up-front, like the proposals block does.
• SessionTabs.kt ~783 (edit box) — OutlinedTextField inside the verticalScroll Column: keyboard occlusion of the draft field is a known Compose pitfall here; the column lacks imePadding(). Worth a quick manual check.
• Wire / tests — Wire.knowledgeProposals / knowledgeReview / updateSkillPolicy payloads are covered by WireTest ✅, but the KnowledgeReviewResult.error-style failure shape (apply failure returned as 200 with applied:false + message, if the server does that) has no test. Minor.

✅ Looks Good

• Wire decoders are null-safe with sane defaults (strList, b, object-or-array tolerance in knowledgeProposals) and mirror the existing style; new WireTest coverage matches the PR body's claims.
• dispositionFor() handles trim/lowercase and the closed → "hidden" case; hidden skills correctly don't get a badge.
• Proposals are listed globally (not session-filtered), matching TUI /skills review, with this-session-first sorting.
• 404 handling degrades with clear user-facing messages (README updated to document the server prerequisites).

---

Reviewed by Hermes Agent

[tournierjc]

Thanks — addressed in 2234389.

Warnings

• Apply-failed refresh — onSuccess already reloaded after applied:false; kept that path and switched post-action reloads to a lighter refreshSkillState() (live session + skills + proposals) so the pending card stays current without a full Info tab reload.
• skillPolicy seed — now remember(sessionId) { session?.skillPolicy }. Later getSession only overwrites when the live projection includes skillPolicy; a missing key no longer resurrects the parent-list snapshot after close/disposition updates.

Suggestions taken

• Parallelized initial load() with async/coroutineScope.
• Split reviewSupported from proposalsSupported — list 404 still hides the section; review 404 keeps the list and disables Approve/Edit/Reject.
• One-shot policy capability probe via updateSkillPolicy(dispositions = emptyMap()) (no-op patch) so catalog/disposition controls degrade before the first failed tap.
• Added imePadding() on the Info tab scroll column for the draft editor.

Left as-is

• WireTest already covers applied:false / status:pending on knowledgeReview; no separate error-message field on the wire today.

./gradlew testDebugUnitTest green on this tip.

[tournierjc]

Final Review — after the state-bug fixes (2234389)

Verdict: Approve. All findings from the previous review are addressed on the new head (CI build pass); verified by reading git diff ea49dcd..HEAD, no merge blockers left.

✅ Verified fixed

• Retry reload — reviewProposal success path now always calls refreshSkillState(), so the pending-retry card is current after applied:false (SessionTabs.kt:522).
• Policy reset bug — skillPolicy seeds once via remember(sessionId) and load/refreshSkillState only overwrite when the live getSession projection actually carries skillPolicy; a missing key no longer resurrects the parent-list snapshot (SessionTabs.kt:342, 381-388).
• Parallel load — the 6 requests run as async jobs in a coroutineScope; post-action refreshes use the lighter refreshSkillState() (session + skills + proposals) instead of a full reload.
• reviewSupported split — a 404 on POST /review now disables only the action buttons and shows "Listing works, but approve/reject is not supported", keeping the list visible (SessionTabs.kt:346, 526, 808, 823).
• Capability probe — one-shot probe per session at end of load() degrades the catalog/disposition controls before the first failed tap.
• IME — imePadding() added to the scrollable column.

💡 Non-blocking notes on the new commit

• Probe is a real POST (updateSkillPolicy(sessionId, dispositions = emptyMap())), so on an unsupported server it logs one 500 unexpected skill-policy response per session open — the client throws 500 on an empty body before inspecting JSON, so the 404 never reaches the caller and the probe fails closed. Consequence: the one-shot probe works (button disables), the message stays quiet (probe failure sets no message), and the "Skill policy updates are not supported" hint just doesn't show — acceptable, but a GET-based capability check would be cleaner. Also probe failures keep policySupported=true for non-404 errors, so the first real tap pays a round-trip on a 5xx-broken server.
• refreshSkillState() calls refreshProposals(), which resets proposalsSupported = true on success — fine given the split above (a review 404 no longer hides the section), just noting the flag is intentionally re-armed per load.
• refreshSkillState() reads session?.workspaceDir captured at composition — pre-existing behavior, unchanged.

Ship it: squash-merge per convention (--squash --delete-branch) once kinetick-code#104 side is ready, since the Android feature needs those server routes to be more than soft-degraded.

---

Reviewed by Hermes Agent