Skip to content

AGENTS.md#91

Open
stephen-derosa wants to merge 7 commits intolivekit:mainfrom
stephen-derosa:sderosa/clt-2485-AGENTS.MD
Open

AGENTS.md#91
stephen-derosa wants to merge 7 commits intolivekit:mainfrom
stephen-derosa:sderosa/clt-2485-AGENTS.MD

Conversation

@stephen-derosa
Copy link
Copy Markdown
Contributor

Overview

An AGENTS.MD file for guiding cursor on development inside the project

Key points

  • Architecture
  • Build preferences
  • Testing
  • Formatting
  • comments on readability vs performance

Any other points?

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds an AGENTS.md guidance document intended for development agents working in the LiveKit C++ Client SDK repo, summarizing architecture, build flow, coding conventions, dependencies, testing, and pitfalls.

Changes:

  • Introduces AGENTS.md with project architecture notes and key internal/public types.
  • Documents recommended build and packaging commands, plus dependency/tooling expectations.
  • Captures coding conventions (logging, public API boundaries, error handling) and testing guidance.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@stephen-derosa stephen-derosa force-pushed the sderosa/clt-2485-AGENTS.MD branch 2 times, most recently from bc80b84 to cf6bc19 Compare April 13, 2026 18:00
@stephen-derosa stephen-derosa force-pushed the sderosa/clt-2485-AGENTS.MD branch from cf6bc19 to 348ce39 Compare April 13, 2026 18:04
SDK features follow pattern:

1. Serialize a protobuf `FfiRequest` message.
2. Send it to Rust via `FfiClient::instance().sendRequest(req)`.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there are two types of ffi messages here, one is synchronous call that we send a ffi request to rust ffi layer, and get back the FfiResponse.

Another one is async calls:
c++ setup a async handler that listens to event with a |request_async_id|
Serialize a protobuf FfiRequest message with a |request_async_id|
Send it to Rust via FfiClient::instance().sendRequest(req).
Receive a synchronous FfiResponse and an asynchronous FfiEvent callback.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good call out -- updated!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated, lmk what you think

| `cmake/` | Build helpers (`protobuf.cmake`, `spdlog.cmake`, `LiveKitConfig.cmake.in`) |
| `docker/` | Dockerfile for CI and SDK distribution images |
| `.github/workflows/` | GitHub Actions CI workflows |

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m wondering if we should add a dedicated section to clarify the FFI design rules. For example, we should explicitly state that the main implementation logic lives in Rust, and that the C++ layer must remain a thin wrapper over the FFI.

It would also be helpful to include a section describing the threading model, covering questions like:

Is FfiClient thread-safe? (this should be clearly defined)
On which threads are RoomDelegate callbacks invoked?
Are they coming from FFI/Tokio threads?
In the FFI layer, are audio callbacks executed on a dedicated high-priority Tokio runtime?
What are the threading models for:
LocalAudioTrack / LocalVideoTrack
AudioStream / VideoStream
DataStream
What are the thread-safety guarantees of the public API?
(explicitly define whether it is thread-safe or not)

If applicable, clarifying these points would make the design much easier to understand and help avoid misuse.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have the core principals section:

### Core Principle
Rust owns as much of the business logic as possible. If a feature may be used by another SDK it should be implemented in Rust. Since this is an SDK, ensure backwards compatibility is maintained when possible.

I added "The C++ layer should be a thin wrapper around the Rust core. " after the first sentence.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WRT threading, added this in this commit. In general i was trying not to provide too much information to avoid confusion, but i think this is a good call out.

@xianshijing-lk
Copy link
Copy Markdown
Collaborator

xianshijing-lk commented Apr 13, 2026

Do you find it useful to have the agents.md ?

like, if claude / codex / cursor generate better results with this agents.md ? or it does not have much impact?

AGENTS.md Outdated

### Error Handling

- Throw exceptions for broken/unrecoverable states.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably a larger team discussion: I am fairly anti-exception for robotics/embedded applications. Some safety certification specifications explicitly disallow exceptions. This would apply more to AV/aerospace customers of LiveKit due to safety requirements.

It would be unfortunate to lose a customer opportunity due to C++ feature usage like this. Looking at the existing SDK code we may not be able to undo existing exceptions, but for new code I'd prefer not to use them when possible.

bool, std::optional, Expected<Type, Error> (in STL later), etc. (which I think we're already using in places)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Im curious to know more of the "Some safet certification specifications explicitily disallow exceptions". I could think of a car rolling on the street, exception, car doesnt stop. ha.

In general i agree with the sentiment of dont use exceptions if you dont have to, this line is definitely bad.

I could change this to "Do not throw exceptions which must be caught by the application. Instead, prefer bool, std::optional, or a custom implementation of Result<Type, Error>".

Curious on your thoughts as well @xianshijing-lk

Copy link
Copy Markdown
Contributor

@alan-george-lk alan-george-lk Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea is you avoid using them as much as possible in your own code (some third-party libraries like STL, ROS, etc. may throw), and handle errors using easy to follow control flow (overhead cost incurred by that, tradeoffs).

@stephen-derosa
Copy link
Copy Markdown
Contributor Author

Do you find it useful to have the agents.md ?

like, if claude / codex / cursor generate better results with this agents.md ? or it does not have much impact?

@xianshijing-lk yeah i have found this file to be useful. Its hard to quantify but i have noticed the rules taking place such as copyright and logging preferences.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants