docs: ADR-0003 extension architecture#110
Open
mrbrandao wants to merge 2 commits into
Open
Conversation
Add Architecture Decision Record for the extension system with 5 kinds (target, repo, runtime, source, scan), YAML manifests, and language-agnostic protocol. Design doc includes kind taxonomy diagram, discovery flow, manifest schema, and hello world examples. Co-Authored-By: Claude Sonnet <noreply@anthropic.com>
mrbrandao
added
documentation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds formal documentation and a developer guide for a Lola extension system: defines five extension kinds, an Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as Lola CLI
participant Manager as Extension Manager
participant Registry as Built-in Registry
participant FS as Extensions Directory
participant ExtProc as External Extension Process
CLI->>Manager: start / ext command
Manager->>Registry: load built-in factories
Manager->>FS: scan for `extension.yaml` and `lola-ext-*` binaries
FS-->>Manager: manifest files & binaries
Manager->>ExtProc: spawn external extension (stdin/stdout)
ExtProc-->>Manager: register via protocol (JSON messages)
CLI->>Manager: invoke extension action (JSON request)
Manager->>ExtProc: forward request
ExtProc-->>Manager: JSON response
Manager-->>CLI: return result
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.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
🧹 Nitpick comments (2)
docs/dev-guide/design/extension-architecture.md (2)
162-163: Add error handling for malformed JSON input.The script parses stdin without handling potential JSON decode errors. While this is a "hello world" example, adding basic error handling would make it more robust and instructive.
♻️ Suggested enhancement
-request = json.loads(sys.stdin.read()) -action = request["action"] +try: + request = json.loads(sys.stdin.read()) + action = request["action"] +except (json.JSONDecodeError, KeyError) as e: + print(json.dumps({"error": str(e)}), file=sys.stderr) + sys.exit(1)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/dev-guide/design/extension-architecture.md` around lines 162 - 163, The code reads stdin and calls json.loads(sys.stdin.read()) then accesses request["action"] without handling malformed input; update the block around json.loads and the access to request["action"] (the request variable and action lookup) to catch json.JSONDecodeError and KeyError (and optionally generic Exception), emit a clear error to stderr (or process logger) and exit with a non-zero status, and only proceed to use action when the JSON was parsed and the "action" key exists.
93-94: Consider noting the jq dependency.The script relies on
jqfor JSON parsing but doesn't document this dependency. For a "hello world" example meant to be runnable, noting this requirement would help users.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/dev-guide/design/extension-architecture.md` around lines 93 - 94, The example uses jq in the line action=$(echo "$input" | jq -r '.action') but the document doesn't mention jq as a prerequisite; update the text around the code snippet to state that jq is required (or modify the example to check for jq at runtime and print a friendly error if missing), and reference the two shell lines (input=$(cat) and action=$(echo "$input" | jq -r '.action')) so readers know exactly where the dependency is used.
🤖 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/dev-guide/design/extension-architecture.md`:
- Around line 149-180: Add a brief setup note to the extension-architecture docs
that instructs users to make the example script my-catalog.py executable and to
ensure it contains the proper shebang (#!/usr/bin/env python3); specifically,
mention running chmod +x on the script and verifying the shebang line so the
script can be run directly as an executable.
- Around line 134-138: The fenced code block showing the directory structure
(the block starting with "~/.lola/extensions/my-catalog/") lacks a language
identifier; add "text" after the opening triple backticks so it becomes ```text
to improve rendering/tooling (update the fenced block that contains
extension.yaml and my-catalog.py).
- Around line 193-195: The fenced code block containing the protocol diagram
(the line starting with "Core → [write request to stdin] → Extension process →
[read response from stdout] → Core") lacks a language specifier; update the
opening fence from ``` to ```text so the block is rendered consistently (i.e.,
change the code fence for that protocol diagram to use the `text` language
identifier).
- Around line 71-75: Update the fenced code block showing the directory tree in
extension-architecture.md to include a language specifier (e.g., change the
opening ``` to ```text) so the block is rendered and highlighted correctly;
locate the block containing "~/.lola/extensions/hello-target/" and modify its
fence to use the "text" language identifier.
- Around line 170-176: The error branch in the resolve action prints an error
JSON but still exits with code 0; update the resolve handling (the branch where
action == "resolve" that looks up name in CATALOG and prints the error JSON) to
call sys.exit(1) after printing the error so the process returns a non-zero exit
code on failure; ensure sys is imported at the top if not already (reference the
resolve branch and the CATALOG/name lookup to locate the change).
- Around line 88-121: Add a brief "Setup" note immediately after the
hello-target.sh example that tells users to make the script executable before
running it; explicitly state they should set executable permissions on the
installed extension script (e.g., use chmod +x on the hello-target.sh /
installed extension file) so they don't hit "permission denied" errors when
invoking the script.
---
Nitpick comments:
In `@docs/dev-guide/design/extension-architecture.md`:
- Around line 162-163: The code reads stdin and calls
json.loads(sys.stdin.read()) then accesses request["action"] without handling
malformed input; update the block around json.loads and the access to
request["action"] (the request variable and action lookup) to catch
json.JSONDecodeError and KeyError (and optionally generic Exception), emit a
clear error to stderr (or process logger) and exit with a non-zero status, and
only proceed to use action when the JSON was parsed and the "action" key exists.
- Around line 93-94: The example uses jq in the line action=$(echo "$input" | jq
-r '.action') but the document doesn't mention jq as a prerequisite; update the
text around the code snippet to state that jq is required (or modify the example
to check for jq at runtime and print a friendly error if missing), and reference
the two shell lines (input=$(cat) and action=$(echo "$input" | jq -r '.action'))
so readers know exactly where the dependency is used.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 610836da-0f5c-4351-b75c-168e26d99fa5
📒 Files selected for processing (2)
docs/adr/0003-extension-architecture.mddocs/dev-guide/design/extension-architecture.md
- Add `text` language specifier to three bare fenced code blocks (MD040) - Document `jq` prerequisite for bash target example - Add `chmod +x` setup notes for both hello world examples - Add try/except error handling for malformed JSON input in Python example - Exit with code 1 when resolve action finds no matching module Co-Authored-By: Claude <noreply@anthropic.com>
Collaborator
Author
|
@SecKatie @sergio-correia, this is also good to go, check out |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Related Issues
Part of the Lola Go migration and extension platform architecture initiative.
Test Plan
AI Disclosure
AI-assisted with Claude Code
Summary by CodeRabbit