LCORE-1589: Human-in-the-Loop Spike#1459
LCORE-1589: Human-in-the-Loop Spike#1459jrobertboos wants to merge 2 commits intolightspeed-core:mainfrom
Conversation
WalkthroughAdds two Human-in-the-Loop (HIL) design documents defining an asynchronous approval workflow for MCP tool calling: a spike exploring design decisions and a full specification covering control flow, API endpoints, data models, configuration, error handling, and implementation tasks. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant LCS as LCS (server)
participant Llama as Llama Stack (agent)
participant Store as Approval Storage (cache/DB)
participant MCP as MCP Tool/Server
Client->>LCS: query/stream request (may invoke MCP)
LCS->>Llama: forward request
Llama->>LCS: emits mcp_approval_request (requires_action)
LCS->>Store: persist approval request (TTL)
LCS-->>Client: HTTP 200 / stream event with status:"requires_action" + required_action
Client->>LCS: POST /approvals/{id} (approve/deny)
LCS->>Store: validate & fetch approval request
LCS->>Llama: send mcp_approval_response (approved/denied)
alt approved
Llama->>MCP: resume/execute MCP call
MCP->>Llama: tool response
Llama->>LCS: final output / continue stream
LCS-->>Client: return/stream result
else denied
Llama->>LCS: handle denial (terminate or continue)
LCS-->>Client: return error/status update
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/design/human-in-the-loop/human-in-the-loop-spike.md`:
- Around line 137-139: Replace all placeholder ticket keys "LCORE-????" in this
document (notably in the story header "Add `require_approval` configuration for
MCP servers" and the other repeated headings/keys) with real issue keys or
explicitly mark them as TODO outside merge scope; update every instance so the
design links to a traceable ticket and regenerate any related anchors/README
references if used to ensure cross-references remain valid.
- Line 72: Fix the markdown fence lint violations by adding explicit language
identifiers to all fenced code blocks and ensuring required blank lines before
and after each fence; e.g., replace bare/unnamed fences near the table row
containing | B. "always" | Secure by default, opt-out for trusted tools | and
the suggested fences like ```text and ```sql with properly spaced fences (blank
line above and below) and add a language tag (e.g., ```text, ```sql) so every
fenced block in human-in-the-loop-spike.md (also at the other listed
occurrences) follows markdownlint MD040/MD031 rules.
In `@docs/design/human-in-the-loop/human-in-the-loop.md`:
- Around line 337-355: The docs introduce a new "requires_action" response shape
for /query and /streaming_query (payload keys like conversation_id,
status="requires_action", required_action.type="mcp_approval", approvals[] with
id/server_label/tool_name/arguments/expires_at) but the doc still claims "No API
changes"; update the API contract by explicitly modeling the response as a
union/oneOf (e.g., QueryResponse = oneOf{QueryResult, RequiresActionResult}) or
call out version-gating; clearly list both shapes and add OpenAPI/typed-client
guidance so consumers of query and streaming_query know to handle both variants
(or upgrade the endpoint version).
- Around line 307-309: Choose one authorization error semantics (either "403
Forbidden for unauthorized/ownership failures" or "404 Not Found for not owned
by user") and apply it consistently: update the lines containing "Response
(404): Approval not found or not owned by user" and any other endpoint
descriptions that mention ownership/authorization to use the chosen status, and
update the error matrix entry that currently lists 403 so it matches. Search for
the exact phrase "Approval not found or not owned by user" and the error-matrix
rows that mention 403/404 (sections referenced around the existing text) and
change the status codes and any explanatory text so all endpoint sections and
the matrix use the same policy.
- Around line 392-393: Update the phrasing in the "5. **Rate limiting**" bullet
so the sentence reads "Approval endpoints should be rate-limited to prevent
abuse (handled by existing rate limiting infrastructure)." — find the bullet
titled "Rate limiting" and replace the unhyphenated "should be rate limited" (or
similar) with the hyphenated "should be rate-limited" to ensure grammatical
consistency.
- Around line 93-103: The approval continuation flow is ambiguous: clarify
whether POST /approvals/{id} resumes the agent and returns the assistant's final
output or if the client must call /query again. Update human-in-the-loop.md (and
the related diagram lines ~310-329) to explicitly state the contract for POST
/approvals/{id}: either (A) POST /approvals/{id} returns 200 with approval
metadata plus resumed agent output in a named field (e.g., mcp_approval_response
includes "resumed_output" or "final_response" and any tool result), or (B) POST
/approvals/{id} returns only approval status metadata and the client is required
to re-issue a GET/POST to /query to retrieve the agent’s continued output. Pick
one behavior, update the sequence diagram labels (mcp_approval_response, tool
result + response) and document exact JSON shape and status codes for the chosen
contract so clients know how to continue.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9d842f74-dc75-4099-be6a-11e98dcf5265
📒 Files selected for processing (2)
docs/design/human-in-the-loop/human-in-the-loop-spike.mddocs/design/human-in-the-loop/human-in-the-loop.md
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
- GitHub Check: E2E: server mode / ci
🧰 Additional context used
🪛 LanguageTool
docs/design/human-in-the-loop/human-in-the-loop.md
[style] ~61-~61: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...re interaction - U3: As a user, I want to see what a tool will do before it runs,...
(REP_WANT_TO_VB)
[style] ~63-~63: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...unwanted changes - U4: As a user, I want to deny a tool invocation, so that I can s...
(REP_WANT_TO_VB)
[grammar] ~393-~393: Use a hyphen to join words.
Context: ...event abuse (handled by existing rate limiting infrastructure). ### Migration...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.22.0)
docs/design/human-in-the-loop/human-in-the-loop-spike.md
[warning] 72-72: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 128-128: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 150-150: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 272-272: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 294-294: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 315-315: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 323-323: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 338-338: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 358-358: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
| | Option | Description | | ||
| |--------|-------------| | ||
| | A. `"never"` (current behavior) | Backwards compatible, opt-in HIL | | ||
| | B. `"always"` | Secure by default, opt-out for trusted tools | |
There was a problem hiding this comment.
Fix markdown fence lint violations (MD040/MD031).
There is at least one fenced block without a language (Line 72) and multiple fences missing required blank lines around them. This will keep docs lint noisy/failing where markdownlint is enforced.
Suggested doc cleanup
-```
+```text
...
-```
+```
+
```sql
...</details>
Also applies to: 128-128, 150-150, 272-272, 294-294, 315-315, 323-323, 338-338, 358-358
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.22.0)</summary>
[warning] 72-72: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against the current code and only fix it if needed.
In @docs/design/human-in-the-loop/human-in-the-loop-spike.md at line 72, Fix the
markdown fence lint violations by adding explicit language identifiers to all
fenced code blocks and ensuring required blank lines before and after each
fence; e.g., replace bare/unnamed fences near the table row containing | B.
"always" | Secure by default, opt-out for trusted tools | and the suggested
fences like text and sql with properly spaced fences (blank line above and
below) and add a language tag (e.g., text, sql) so every fenced block in
human-in-the-loop-spike.md (also at the other listed occurrences) follows
markdownlint MD040/MD031 rules.
</details>
<!-- fingerprinting:phantom:triton:hawk:9b03e7ec-f5cf-4fea-8293-25b4eb3bd5f8 -->
<!-- This is an auto-generated comment by CodeRabbit -->
| <!-- type: Story --> | ||
| <!-- key: LCORE-???? --> | ||
| ### LCORE-???? Add `require_approval` configuration for MCP servers |
There was a problem hiding this comment.
Replace placeholder ticket keys before merge.
LCORE-???? placeholders make the execution plan non-actionable and break traceability from design to delivery. Please resolve these to real issue keys (or explicitly mark as TODO outside merge scope).
Also applies to: 167-169, 194-196, 222-224, 250-253, 275-277, 301-304
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/design/human-in-the-loop/human-in-the-loop-spike.md` around lines 137 -
139, Replace all placeholder ticket keys "LCORE-????" in this document (notably
in the story header "Add `require_approval` configuration for MCP servers" and
the other repeated headings/keys) with real issue keys or explicitly mark them
as TODO outside merge scope; update every instance so the design links to a
traceable ticket and regenerate any related anchors/README references if used to
ensure cross-references remain valid.
| │ │ POST /approvals/{id} │ │ | ||
| │ │ ────────────────────────────► │ | ||
| │ │ {approve: true} │ │ | ||
| │ │ │ │ | ||
| │ │ │ ───── mcp_approval_response ──────────►│ | ||
| │ │ │ │ | ||
| │ │ │ ◄────── tool result + response ────────┤ | ||
| │ │ │ │ | ||
| │ │ ◄─── 200 OK ────────────────┤ │ | ||
| │ │ (final response) │ │ | ||
| │ │ |
There was a problem hiding this comment.
Approval continuation flow is ambiguous after POST /approvals/{id}.
The diagram implies a final assistant response is returned after approval, but the endpoint example returns only approval status metadata. Please define the exact continuation contract (e.g., whether client must call /query again, or approval endpoint can return resumed agent output).
Also applies to: 310-329
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/design/human-in-the-loop/human-in-the-loop.md` around lines 93 - 103,
The approval continuation flow is ambiguous: clarify whether POST
/approvals/{id} resumes the agent and returns the assistant's final output or if
the client must call /query again. Update human-in-the-loop.md (and the related
diagram lines ~310-329) to explicitly state the contract for POST
/approvals/{id}: either (A) POST /approvals/{id} returns 200 with approval
metadata plus resumed agent output in a named field (e.g., mcp_approval_response
includes "resumed_output" or "final_response" and any tool result), or (B) POST
/approvals/{id} returns only approval status metadata and the client is required
to re-issue a GET/POST to /query to retrieve the agent’s continued output. Pick
one behavior, update the sequence diagram labels (mcp_approval_response, tool
result + response) and document exact JSON shape and status codes for the chosen
contract so clients know how to continue.
| Response (404): Approval not found or not owned by user | ||
| Response (410): Approval expired | ||
|
|
There was a problem hiding this comment.
Resolve conflicting authorization error semantics (404 vs 403).
Line 307/331 state “not found or not owned by user” as 404, but the error matrix includes 403 forbidden for unauthorized access. Pick one policy and apply it consistently across endpoint sections and the matrix.
Also applies to: 331-334, 369-375
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/design/human-in-the-loop/human-in-the-loop.md` around lines 307 - 309,
Choose one authorization error semantics (either "403 Forbidden for
unauthorized/ownership failures" or "404 Not Found for not owned by user") and
apply it consistently: update the lines containing "Response (404): Approval not
found or not owned by user" and any other endpoint descriptions that mention
ownership/authorization to use the chosen status, and update the error matrix
entry that currently lists 403 so it matches. Search for the exact phrase
"Approval not found or not owned by user" and the error-matrix rows that mention
403/404 (sections referenced around the existing text) and change the status
codes and any explanatory text so all endpoint sections and the matrix use the
same policy.
| Query and streaming_query endpoints may return: | ||
| ```json | ||
| { | ||
| "conversation_id": "conv_xyz", | ||
| "status": "requires_action", | ||
| "required_action": { | ||
| "type": "mcp_approval", | ||
| "approvals": [ | ||
| { | ||
| "id": "apr_abc123", | ||
| "server_label": "github-mcp", | ||
| "tool_name": "create_issue", | ||
| "arguments": {"title": "Bug report", "body": "..."}, | ||
| "expires_at": "2026-04-01T10:05:00Z" | ||
| } | ||
| ] | ||
| } | ||
| } | ||
| ``` |
There was a problem hiding this comment.
Clarify response contract and versioning for requires_action.
This introduces a second response shape for /query/streaming while Line 401 says “No API changes to existing endpoints.” That is effectively an API contract change for typed clients and OpenAPI consumers. Please explicitly define this as a union/oneOf response model (or version-gate it) to avoid runtime/client breakage.
Also applies to: 397-402
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/design/human-in-the-loop/human-in-the-loop.md` around lines 337 - 355,
The docs introduce a new "requires_action" response shape for /query and
/streaming_query (payload keys like conversation_id, status="requires_action",
required_action.type="mcp_approval", approvals[] with
id/server_label/tool_name/arguments/expires_at) but the doc still claims "No API
changes"; update the API contract by explicitly modeling the response as a
union/oneOf (e.g., QueryResponse = oneOf{QueryResult, RequiresActionResult}) or
call out version-gating; clearly list both shapes and add OpenAPI/typed-client
guidance so consumers of query and streaming_query know to handle both variants
(or upgrade the endpoint version).
| 5. **Rate limiting**: Approval endpoints should be rate-limited to prevent | ||
| abuse (handled by existing rate limiting infrastructure). |
There was a problem hiding this comment.
Use hyphenated “rate-limited” phrasing.
Line 393 should read “should be rate-limited” for grammar consistency.
🧰 Tools
🪛 LanguageTool
[grammar] ~393-~393: Use a hyphen to join words.
Context: ...event abuse (handled by existing rate limiting infrastructure). ### Migration...
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/design/human-in-the-loop/human-in-the-loop.md` around lines 392 - 393,
Update the phrasing in the "5. **Rate limiting**" bullet so the sentence reads
"Approval endpoints should be rate-limited to prevent abuse (handled by existing
rate limiting infrastructure)." — find the bullet titled "Rate limiting" and
replace the unhyphenated "should be rate limited" (or similar) with the
hyphenated "should be rate-limited" to ensure grammatical consistency.
| for trusted tools. | ||
|
|
||
| **PoC validation**: Not yet built. The core mechanism (Llama Stack approval | ||
| types) is already implemented and tested in the codebase. |
There was a problem hiding this comment.
which code base exactly? llama-stack or LCS?
There was a problem hiding this comment.
in this case llama stack but this part is not needed
| ### Decision 3: Permanent allow/deny list storage | ||
|
|
||
| Where should permanent tool allow/deny lists be configured? | ||
|
|
||
| | Option | Description | | ||
| |--------|-------------| | ||
| | A. Database only | Runtime configuration, managed via API | | ||
| | B. YAML config only | Deployment-time configuration | | ||
| | C. Both | YAML baseline with database overrides | | ||
|
|
||
| **Recommendation**: Option B (YAML config only). Keeps security policy | ||
| declarative and auditable. Runtime overrides can be added in a future iteration | ||
| if customer demand exists. |
There was a problem hiding this comment.
Do we need to have permanent allow/deny list of tools/actions for an MCP server in the initial implementation of the /approval API?
Especially if Decision 2 is "C. Per-request | Each tool invocation requires separate approval", sounds like we can keep things simple for the first implementation, and come back and implement features like "allow/deny lists" at a later point?
There was a problem hiding this comment.
Well we will definitely need an allow/deny list of some sort in order for our users to configure the HITL settings for each mcp server. Because for example a log reading tool might not need HITL approval while a tool that is potentially destructive must have some sort of HITL.
| **Recommendation**: Option A (`"never"`). Maintains backwards compatibility. | ||
| Operators explicitly enable HIL for servers with write operations. |
There was a problem hiding this comment.
Are we concerned with backward compatibility yet? If we're still in the prototype landscape with our clients, is it better to switch this to "always" so that this change is more immediately surfaced to our clients?
| ├──────────────────────────────────────────────────────────────────────────────┤ | ||
| │ │ | ||
| │ ┌────────┐ POST /query ┌─────────────────┐ responses.create ┌───────────┐ | ||
| │ │ Client │ ───────────────► │ LCS Query │ ───────────────────► │ Llama │ |
There was a problem hiding this comment.
Is the Client here an MCP server?
There was a problem hiding this comment.
Client is LS App team.
| CREATE TABLE IF NOT EXISTS approval_requests ( | ||
| id TEXT PRIMARY KEY, | ||
| conversation_id TEXT NOT NULL, | ||
| user_id TEXT NOT NULL, | ||
| server_label TEXT NOT NULL, | ||
| tool_name TEXT NOT NULL, | ||
| arguments TEXT NOT NULL, -- JSON | ||
| status TEXT NOT NULL DEFAULT 'pending', -- pending, approved, denied, expired | ||
| created_at TEXT NOT NULL, | ||
| expires_at TEXT NOT NULL, | ||
| decision_reason TEXT, | ||
| decided_at TEXT | ||
| ); |
There was a problem hiding this comment.
What'll be the policy for clearing up this table? I understand that when an approval is required, a new row will be created to keep track of this particular request along with all other pending requests. Should we also explicitly state once a response is received from the client (approval/denial), we will delete the row to ensure database doesn't get bloated with outdated data?
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
docs/design/human-in-the-loop/human-in-the-loop.md (3)
93-103:⚠️ Potential issue | 🟠 MajorClarify the continuation contract after
POST /approvals/{id}.The doc still implies two behaviors (approval endpoint returns final response vs client must re-query). Define one explicit contract and keep examples/diagram consistent.
Also applies to: 322-329, 556-558
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/design/human-in-the-loop/human-in-the-loop.md` around lines 93 - 103, The documentation is ambiguous about whether POST /approvals/{id} returns the final response or requires the client to re-query; pick one behavior and update the contract and diagrams to match: if you choose synchronous final response, state that POST /approvals/{id} returns 200 with the final tool result (mcp_approval_response) and remove any arrows/notes implying re-query; if you choose asynchronous, state that POST /approvals/{id} returns an acknowledgment and clients must GET /approvals/{id} (or another polling/webhook) to receive the final tool result, and update the diagram and all examples (references to POST /approvals/{id}, mcp_approval_response, and "tool result + response") to consistently reflect that single chosen flow.
337-355:⚠️ Potential issue | 🟠 MajorDocument
/query/streaming_queryas a response union instead of “no API changes.”Adding
requires_actionintroduces a new response variant; this is an API contract change for typed consumers and should be modeled explicitly.Also applies to: 397-402
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/design/human-in-the-loop/human-in-the-loop.md` around lines 337 - 355, The docs currently state “no API changes” but the addition of the requires_action response is a new response variant for the /query and /streaming_query endpoints; update the documentation in human-in-the-loop.md to document the endpoints’ response as a union type that includes the new requires_action shape (with conversation_id, status, and required_action.mcp_approval structure including approvals items and expires_at) instead of claiming no API changes, and add the same union-style documentation for the other occurrence referenced (around lines ~397-402) so typed consumers can see the new contract.
307-308:⚠️ Potential issue | 🟠 MajorUnify authorization semantics (404 vs 403) across endpoint docs and error matrix.
The endpoint sections and error table still conflict on ownership/authorization failures. Pick one policy and apply it consistently.
Also applies to: 331-332, 369-375
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/design/human-in-the-loop/human-in-the-loop.md` around lines 307 - 308, The docs currently mix 404 and 403 for authorization/ownership failures (e.g., the lines "Response (404): Approval not found or not owned by user" and "Response (410): Approval expired"); pick one canonical policy (recommend using 403 for "not owned/forbidden" and 404 only for truly non-existent resources) and apply it consistently across the endpoint descriptions and the error matrix including the other affected sections (around the blocks noted at 331-332 and 369-375); update the response codes and any corresponding error text so all references to ownership/authorization failures use the chosen status code and adjust examples/messages to match.docs/design/human-in-the-loop/human-in-the-loop-spike.md (1)
138-139:⚠️ Potential issue | 🟠 MajorReplace unresolved
LCORE-????placeholders with real ticket IDs before merge.These placeholders still make the implementation plan non-actionable and untraceable to delivery work items.
Also applies to: 167-168, 194-195, 222-223, 251-252, 276-277, 302-303
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/design/human-in-the-loop/human-in-the-loop-spike.md` around lines 138 - 139, Replace every unresolved placeholder "LCORE-????" in this document (e.g., the heading "### LCORE-???? Add `require_approval` configuration for MCP servers" and the other repeated occurrences) with the actual ticket/issue IDs that track the work; update all instances so the headings and inline keys reference real, authoritative ticket numbers before merging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/design/human-in-the-loop/human-in-the-loop.md`:
- Around line 72-105: The ASCII diagram code fence in the human-in-the-loop doc
is unlabeled and triggers markdown lint; update the fenced block around the
Request Flow diagram in human-in-the-loop.md by adding a language identifier
(e.g., ```text) to the opening fence so the diagram is treated as plain text and
linting passes, leaving the content unchanged.
---
Duplicate comments:
In `@docs/design/human-in-the-loop/human-in-the-loop-spike.md`:
- Around line 138-139: Replace every unresolved placeholder "LCORE-????" in this
document (e.g., the heading "### LCORE-???? Add `require_approval` configuration
for MCP servers" and the other repeated occurrences) with the actual
ticket/issue IDs that track the work; update all instances so the headings and
inline keys reference real, authoritative ticket numbers before merging.
In `@docs/design/human-in-the-loop/human-in-the-loop.md`:
- Around line 93-103: The documentation is ambiguous about whether POST
/approvals/{id} returns the final response or requires the client to re-query;
pick one behavior and update the contract and diagrams to match: if you choose
synchronous final response, state that POST /approvals/{id} returns 200 with the
final tool result (mcp_approval_response) and remove any arrows/notes implying
re-query; if you choose asynchronous, state that POST /approvals/{id} returns an
acknowledgment and clients must GET /approvals/{id} (or another polling/webhook)
to receive the final tool result, and update the diagram and all examples
(references to POST /approvals/{id}, mcp_approval_response, and "tool result +
response") to consistently reflect that single chosen flow.
- Around line 337-355: The docs currently state “no API changes” but the
addition of the requires_action response is a new response variant for the
/query and /streaming_query endpoints; update the documentation in
human-in-the-loop.md to document the endpoints’ response as a union type that
includes the new requires_action shape (with conversation_id, status, and
required_action.mcp_approval structure including approvals items and expires_at)
instead of claiming no API changes, and add the same union-style documentation
for the other occurrence referenced (around lines ~397-402) so typed consumers
can see the new contract.
- Around line 307-308: The docs currently mix 404 and 403 for
authorization/ownership failures (e.g., the lines "Response (404): Approval not
found or not owned by user" and "Response (410): Approval expired"); pick one
canonical policy (recommend using 403 for "not owned/forbidden" and 404 only for
truly non-existent resources) and apply it consistently across the endpoint
descriptions and the error matrix including the other affected sections (around
the blocks noted at 331-332 and 369-375); update the response codes and any
corresponding error text so all references to ownership/authorization failures
use the chosen status code and adjust examples/messages to match.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 22330491-d6b6-4565-b487-db4717d039c7
📒 Files selected for processing (2)
docs/design/human-in-the-loop/human-in-the-loop-spike.mddocs/design/human-in-the-loop/human-in-the-loop.md
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-04-05T12:19:36.009Z
Learnt from: CR
Repo: lightspeed-core/lightspeed-stack PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-05T12:19:36.009Z
Learning: Applies to src/**/*.py : Use `from llama_stack_client import AsyncLlamaStackClient` for Llama Stack imports
Applied to files:
docs/design/human-in-the-loop/human-in-the-loop-spike.md
🪛 LanguageTool
docs/design/human-in-the-loop/human-in-the-loop.md
[style] ~61-~61: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...re interaction - U3: As a user, I want to see what a tool will do before it runs,...
(REP_WANT_TO_VB)
[style] ~63-~63: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...unwanted changes - U4: As a user, I want to deny a tool invocation, so that I can s...
(REP_WANT_TO_VB)
[grammar] ~393-~393: Use a hyphen to join words.
Context: ...event abuse (handled by existing rate limiting infrastructure). ### Migration...
(QB_NEW_EN_HYPHEN)
| ``` | ||
| ┌──────────────────────────────────────────────────────────────────────────────┐ | ||
| │ Request Flow │ | ||
| ├──────────────────────────────────────────────────────────────────────────────┤ | ||
| │ │ | ||
| │ ┌────────┐ POST /query ┌─────────────────┐ responses.create ┌───────────┐ | ||
| │ │ Client │ ───────────────► │ LCS Query │ ───────────────────► │ Llama │ | ||
| │ │ │ │ Endpoint │ │ Stack │ | ||
| │ └────────┘ └─────────────────┘ └───────────┘ | ||
| │ │ │ │ | ||
| │ │ │ ◄─────── mcp_approval_request ─────────┤ | ||
| │ │ │ │ | ||
| │ │ ┌──────▼──────┐ │ | ||
| │ │ │ Store │ │ | ||
| │ │ │ Approval │ │ | ||
| │ │ │ Request │ │ | ||
| │ │ └──────┬──────┘ │ | ||
| │ │ │ │ | ||
| │ │ ◄─── 200 OK ────────────────┤ │ | ||
| │ │ status: requires_action│ │ | ||
| │ │ │ │ | ||
| │ │ POST /approvals/{id} │ │ | ||
| │ │ ────────────────────────────► │ | ||
| │ │ {approve: true} │ │ | ||
| │ │ │ │ | ||
| │ │ │ ───── mcp_approval_response ──────────►│ | ||
| │ │ │ │ | ||
| │ │ │ ◄────── tool result + response ────────┤ | ||
| │ │ │ │ | ||
| │ │ ◄─── 200 OK ────────────────┤ │ | ||
| │ │ (final response) │ │ | ||
| │ │ | ||
| └──────────────────────────────────────────────────────────────────────────────┘ | ||
| ``` |
There was a problem hiding this comment.
Add a language identifier to the ASCII diagram code fence.
This fenced block is unlabeled and will trigger markdown lint in stricter setups.
Suggested fix
-```
+```text
┌──────────────────────────────────────────────────────────────────────────────┐
│ Request Flow │
...
└──────────────────────────────────────────────────────────────────────────────┘</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **IMPORTANT**
> Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
```suggestion
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/design/human-in-the-loop/human-in-the-loop.md` around lines 72 - 105,
The ASCII diagram code fence in the human-in-the-loop doc is unlabeled and
triggers markdown lint; update the fenced block around the Request Flow diagram
in human-in-the-loop.md by adding a language identifier (e.g., ```text) to the
opening fence so the diagram is treated as plain text and linting passes,
leaving the content unchanged.
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
3 participants
Description
Type of change
Tools used to create PR
Identify any AI code assistants used in this PR (for transparency and review context)
Related Tickets & Documents
Checklist before requesting a review
Testing
Summary by CodeRabbit