[tournierjc]

Change

#99 is merged. This PR adds the session-server HTTP surface so Android and other HTTP clients can drive that skill workflow:

• POST /sessions/:id/skill-policy — mandatory / optional / forbidden dispositions and closed catalog
• GET /skills/proposals — list Skill/Memory drafts awaiting review. Query: status, kind, sessionId, limit (default 50, clamped to the store's 200 cap)
• POST /skills/proposals/:id/review — approve / reject (optional editedDraft, reviewNote). Reviewing an unknown proposal answers 404 and an already-reviewed one 409, so a client can tell a lost retry from a broken server

Also wires those methods onto TuiServerRuntime, documents them in docs/harness-integration.md and docs/tui-capabilities.md, and extends the server-http unit tests.

Replaces dirty draft #102 (that branch still carried pre-merge #99 history). Pairs with kinetic-android #5.

Review follow-up on this PR: the transfer scaffolding is gone — the branch is a single commit and the diff against main is the five published files — and the three code/boundary findings are fixed with tests (route-shape pinning, limit clamp, 404/409 mapping).

Validation

• Checks run and results: pnpm verify at ec67eed — 15 gates passed (linux aarch64, Node 26.5.1, pnpm 9.12.0); skipped test:windows and test:sandbox as not applicable on this platform, and test:release-package as it needs an npm release archive. packages/tui/test/unit/server-http.test.ts (24 tests) is declared in test/vitest-suites.json under capability.
• Negative checks: the two new boundary assertions fail against the pre-fix http.ts (500 instead of 404/409; limit=100000 forwarded unclamped), so they pin the fix rather than the fixture.
• Performance: basic (HTTP route/capability wiring only; no new per-turn prompt assembly beyond feat: skill management with prompt lookup, idle drafts, and human review #99). No perf:full label — nothing here touches the paths the performance rules cover.
• NOT RUN, platform limitations and live-service boundaries: no live-provider or live-service acceptance, no interactive TUI path, and this box is the only platform the gates ran on. GitHub CI is the platform record.

Publication and contribution checks

• I have permission to contribute these changes under the existing licenses applicable to the changed files/packages; imported material and its provenance are identified and existing notices are preserved.
• No credentials, account data, real user content, internal source history or private review material is included.
• Added/removed source files were reviewed before regenerating release/public-source.json; new tests are declared in test/vitest-suites.json where applicable. (No source file added or removed: the change edits five existing paths, so the inventory is unchanged and check:source/check:tsconfig are unaffected.)
• Shared English/Chinese documentation and capability/verification records are updated where applicable: the HTTP routes and their response shapes are in docs/harness-integration.md, and docs/tui-capabilities.md now points its capability record at the server surface. No localized product string changed, so no copy.zh-Hans entry applies. Mock/offline results are not described as live-service acceptance.

Maintainer handoff

Publication scope or license changes (if any): none.

Shared-source port: not needed.

[tournierjc]

Code Review Summary

Verdict: Comment — the final tree is clean: routes, validation, 404 capability degradation, docs, and tests all match the #99 port shapes and the kinetic-android #5 client. Findings below are mostly about the branch hygiene and a few test/contract gaps, not the code itself.

🔴 Critical (before merge)

• Branch history is full of transfer scaffolding — main..HEAD is 20 commits: .cursor-transfer/http.part[0-3].b64, http.ts.gz.b64, .cursor-size-probe*.txt, a .github/workflows/assemble-http-restore.yml workflow, chore: 25k size probe, x, multiple duplicate "restore complete HTTP routes" commits. The files all net to zero vs main (verified: merge-base diff = exactly the 4 published files), and CI is green, so the content is fine — but per AGENTS.md ("apply reviewed files individually", no probe/temp artifacts in history) this must land as a squash with a clean message, and the force-push/rewrite noise should be collapsed. Do not merge as a 20-commit merge commit.

⚠️ Warnings

• http.ts:208 — /skills/proposals route shadows the flat /skills route with no boundary test. Any path under /skills/ other than proposals (e.g. GET /skills/pdf) used to fall through to the 404 tail of handleTuiServerRequest; it now enters handleSkillProposals and its status depends on which capabilities the runtime exposes (404 "knowledge proposals are not supported" on a runtime with listSkills but not listKnowledgeProposals, 405 on a POST, 404 "not found" on a minimal runtime). That's not a functional break today (nothing routes there), but it's a contract-fragile shadow. Suggestion: gate the dispatch with segments.length >= 2 && segments[1] === 'proposals' semantics (e.g. require segments.length === 2 || (segments.length === 4 && segments[3] === 'review')) and add a test for a stray /skills/<other> path on the writable runtime.
• KnowledgeProposalError leaks 500s at this boundary. proposal-store.ts throws KnowledgeProposalError('not-found' | 'not-pending', …) for review of a missing/already-reviewed proposal; the error carries code, not status, so toHttpStatus() (http.ts:1130) maps it to 500. Concretely: a double-approve — the exact retry scenario kinetic-android feat(providers): give DeepSeek its endpoint contract (thinking shape + effort levels) #5 builds its "stays pending, retry" UX around — answers 500 from the server. The new unit test fakes the runtime and never exercises this. Suggestion: add a not-found → 404, not-pending → 409 mapping (or status on the error) with one test, so clients can distinguish "gone" from "server broken".
• List filter contract is wider than the port. GET /skills/proposals accepts kind, sessionId, limit here, but TuiRuntimePort.listKnowledgeProposals (port.ts:324) and the adapter path declare them, while the fake in the new test only implements {status, sessionId}. Nothing asserts the adapter forwards kind/limit end-to-end. Suggestion: one test asserting the filter object passed to the runtime carries kind and limit.

💡 Suggestions

• test/unit/server-http.test.ts — the skill-policy fake's mandatory/optional/forbidden lists are computed from the request dispositions, so they assert echo, not merge semantics (null-clearing, accumulation across calls are untested). The 404-degradation test also only covers GET /skills/proposals + skill-policy; add the third new route (POST /skills/proposals/:id/review) to the minimal-runtime 404 test.
• http.ts:818 — limit is unbounded (existing parseLimitQuery convention: ≥1, no max). The draft store holds every past proposal, so ?limit=100000 serializes the full store over the LAN. Consider clamping (e.g. 200), matching what clients use (limit=30).
• http.ts review handler — body cast before validation. (await readJsonBody(request)) as {decision?: unknown} on a non-object body (e.g. [], "x") yields undefined primitives and 400s correctly, so no bug — just noting it relies on decision being strictly 'approve'|'reject'; fine as-is.
• docs/harness-integration.md — the new rows don't document the review response shape ({applied, title, status}) while sibling rows document bodies; kinetic-android feat(providers): give DeepSeek its endpoint contract (thinking shape + effort levels) #5 depends on applied:false semantics, worth one line.

✅ Looks Good

• Runtime gating is consistent with the existing style: capability check before body read → clean 404, matching the PR's "soft-degrade" story and the android client.
• Body/query validation is thorough and typed (dispositions enum + null, closed boolean, decision strict, editedDraft/reviewNote string checks, status/kind enum whitelists) with 400s via handleAction.
• Docs update in harness-integration.md correctly retires the old "GET /skills is read-only, no writes" sentence.
• Route shapes, response fields, and method handling exactly match what kinetic-android feat(providers): give DeepSeek its endpoint contract (thinking shape + effort levels) #5's KcodeClient sends/expects (incl. applied:false pending-retry).
• updateSessionSkillPolicy returns the full Session (with skillPolicy), which is what the Android policy badges consume.

---

Reviewed by Hermes Agent

[tournierjc]

Review follow-up — ec67eed

The branch is now a single commit on main (force-pushed with --force-with-lease). git diff --name-only origin/main HEAD is the five published files; the .cursor-transfer/* parts, the assemble-http-restore workflow and the size probes are gone. Since it is one commit, either your usual merge commit or a squash lands the same tree — the merge is yours.

Point by point:

🔴 Branch history (critical) — done: 28 commits collapsed into one with a message that describes the routes and the review fixes.

⚠️ /skills/proposals shadows the flat /skills route — the symptom did not reproduce, the tightness gap is fixed. On the old head the dispatch tested segments[1] === 'proposals', so GET /skills/pdf already fell through to the main 404 tail ({"error":"not found"}) — new test confirms that on the writable runtime. What was loose is the handler's own tail: any /skills/proposals/<x> length was admitted and answered by handleSkillProposals's fallback. http.ts:208 now requires segments.length === 2 || (segments.length === 4 && segments[3] === 'review'), and a test asserts GET /skills/pdf, GET /skills/proposals/kp_abc123 and GET /skills/proposals/a/b/c all answer the plain 404 {"error":"not found"}.

⚠️ KnowledgeProposalError leaked 500s — done. toHttpStatus now consults a knowledgeProposalHttpStatus() helper that reads the domain error's code (matched by name, because the class lives in local-runtime, which this package does not depend on): not-found → 404, not-pending → 409, anything else still fails closed to 500. Tests: /skills/proposals/kp_missing/review → 404, /skills/proposals/kp_reviewed/review → 409.

⚠️ List filter wider than the port — done. The fake records the filter it receives; the test asserts the runtime gets exactly {status:'pending', kind:'memory', sessionId:'session-1', limit:30}.

💡 Skill-policy fake echoed the request — done. The fake keeps state: patches merge (null clears, later calls accumulate) and the projection sorts, mirroring applySessionSkillPolicyPatch + listSkillsByDisposition. The test now makes two calls and asserts the merged policy, plus 400 when neither field is present. POST /skills/proposals/:id/review was added to the minimal-runtime 404 case.

💡 limit unbounded — done, clamped to 200 at the boundary (mirroring the store's own cap), with a test asserting ?limit=100000 reaches the runtime as 200.

💡 Review response shape undocumented — done: docs/harness-integration.md documents {applied, title, status}, the limit cap and the 404/409; docs/tui-capabilities.md now points its capability record at the HTTP surface.

💡 Body cast before validation — left as-is, per the review.

Negative checks: reverting http.ts to the previous revision and re-running the suite fails the two new boundary tests (expected 100000 to be 200, expected 500 to be 404) — they pin the fix, not the fixture.

Local gates: pnpm verify at ec67eed — 15 gates passed (linux aarch64, Node 26.5.1, pnpm 9.12.0); test:windows, test:sandbox and test:release-package skipped as not applicable here. server-http.test.ts is 24 tests in the capability suite.

Two notes for the record:

• apply-unavailable (a TUI host refusing a user-scoped Memory apply, local-runtime-v2/src/services.ts) still answers 500. Idle drafts never set targetRef: 'user', so it is not reachable from this surface today; I left it out of the mapping rather than invent a status for it.
• For kinetic-android #5: the new 409 lands in SessionTabs.kt's generic decision == "approve" failure branch ("Apply failed; draft stays pending — …"), which is readable but generic. Special-casing 409 there would say "already reviewed" instead.

[tournierjc]

Final Review — after the fixes (squashed head ec67eed)

Verdict: Approve — no merge blockers. The branch is now a single clean commit rebased on main (which carries the #105 upstream merge), the 20-commit .cursor-transfer scaffolding is gone, and every finding from the previous review is addressed and verified in the diff:

✅ Verified fixed

• Branch hygiene — one commit, feat(server): expose skill policy and knowledge review over HTTP, rebased on current main. Squash request satisfied via rebase; merge is clean.
• /skills shadowing — dispatch guard now pins the exact path shape (length===2 or length===4 && [3]==='review'), stray /skills/pdf, /skills/proposals/:id, /skills/proposals/a/b/c fall through to the plain 404 tail, with a test asserting identical wording on the writable runtime.
• Domain codes → 404/409 — knowledgeProposalHttpStatus maps KnowledgeProposalError not-found→404, not-pending→409, matched by name (no local-runtime dependency) with a clear comment; tests cover the double-approve 409 case.
• Filter forwarding — kind/limit forwarding asserted on the recorded filter object ({status, kind, sessionId, limit}), review forwarding recorded too.
• Limit clamp — clampKnowledgeProposalLimit caps at 200 (mirrors the draft-store cap), tested (?limit=100000 → forwarded limit 200).
• Test gaps — skill-policy fake now mirrors the product's merge semantics (null-clear + accumulation across calls, sorted buckets); empty body → 400; minimal-runtime 404 test covers all three new routes.
• Docs — review response shape {applied, title, status}, the 404/409 semantics, limit clamp/default, and proposal fields documented in harness-integration.md; cross-link added in tui-capabilities.md.

Notes (non-blocking, no changes requested)

• limit default is store-side, not route-side: docs say "default 50" — that's proposal-store.ts's default kicking in when the param is omitted. The route forwards nothing, so the client contract is "omit = store default". Accurate as written; just be aware the number lives in two places.
• 429/apply-unavailable domain codes fall through to 500 — correct: an apply failure must stay retryable and 500 keeps the client's "draft stays pending — retry" path intact.
• Android pairing: kinetic-android feat(providers): give DeepSeek its endpoint contract (thinking shape + effort levels) #5's probe POSTs {dispositions:{}} — {} is not "no body", so this route answers 400 ("dispositions or closed is required"), not 404. The probe's 404-only degradation only triggers on runtimes where updateSessionSkillPolicy is absent — which is exactly the soft-degrade case it targets (the shipped TUI runtime always has it), so the probe never fires on capable servers. Semantics are fine as-is; a {} → no-op 200 would be marginally friendlier if a future client probes on the full runtime.
• CI at this head: changes/verification pass, verify (ubuntu/macos) + performance + source-history-artifact pending on the rebase push — confirm green before merge.

Ready to land: gh pr merge 104 --squash --delete-branch (server first, then kinetic-android#5).

---

Reviewed by Hermes Agent

[tournierjc]

Closed because the head branch was renamed cursor/skill-http-after-99-9cd5 → feat/server-skill-review to match AGENTS.md. GitHub closes a pull request whose head branch is renamed (only base branches are repointed — docs), so this PR could not follow the rename; the commit, the review thread and the gate results are unchanged.

Continued in #106, from the renamed branch (feat/server-skill-review, same single commit ec67eed).

[tournierjc]

Final review: approved — all findings from the previous round verified fixed on squashed head ec67eed (see the detailed comment). Full CI suite green on this head (verify ubuntu+macos, changes, performance, source-history-artifact, verification). Clean to squash-merge.