Refactor CLI tool with multi-package architecture#9
Conversation
This refactoring was done to support pluggable adapters in the future to add support for additional frameworks for AFM. 1. afm-core is a lightweight package containing only the CLI logic, AFM parser, and interface definitions. 2. afm-langchain is an implementation of the definitions in afm-core using LangChain. 3. afm-cli is a metapackage that contains no functional code. It has afm-core and afm-langchain as dependencies.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughConverts the monolithic langchain-interpreter into a uv workspace "python-interpreter" (packages: afm-core, afm-langchain, afm-cli); adds an AgentRunner protocol with discovery/loader; refactors the CLI to use pluggable runners and adds update-checking; reorganizes release workflows into language-specific pipelines; and introduces packaging, docs, tests, and workspace Docker changes. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as afm-cli
participant Core as afm-core
participant Loader as Runner Discovery
participant Backend as LangChainRunner
participant LLM as LLM Provider
User->>CLI: afm run agent.afm.md --framework langchain
CLI->>Core: parse_afm_file(agent.afm.md)
Core-->>CLI: AFMRecord
CLI->>Loader: load_runner("langchain")
Loader->>Loader: discover_runners()
Loader->>Backend: load entry point
Loader-->>CLI: LangChainRunner class
CLI->>Backend: instantiate LangChainRunner(afm_record)
CLI->>Backend: connect / arun(user_input)
Backend->>LLM: send inference request
LLM-->>Backend: return response
Backend-->>CLI: agent response
CLI->>User: display output
CLI->>Backend: disconnect()
sequenceDiagram
participant GitHub
participant Release as release-python.yml
participant Validate as Validate Job
participant Test as Test Job
participant Build as Build/Publish
participant Docker as release-docker.yml
participant Finalize as release-finalize.yml
GitHub->>Release: manual or workflow_run trigger
Release->>Validate: validate branch & tag uniqueness
Release->>Test: run tests
Test-->>Release: test results
Release->>Build: uv build & publish to PyPI
Build-->>Release: package published
Release->>Docker: call release-docker.yml (multi-arch build, Trivy scan)
Docker-->>Release: image pushed & SARIF uploaded
Release->>Finalize: call release-finalize.yml with tag/version
Finalize->>GitHub: create release, notes, and push branch/tag
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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. Comment |
This was done as the new implementation isn't langchain specific.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
python-interpreter/README.md (1)
60-75:⚠️ Potential issue | 🟡 MinorProject structure section in README is stale after the multi-package refactor.
The directory tree shown in lines 60–75 displays a monolithic layout (
src/afm/,agent.py,providers.py, etc.) that no longer matches the actual structure. The project now uses a workspace layout withpackages/afm-cli/,packages/afm-core/, andpackages/afm-langchain/, but the README still shows the old single-package structure. This will mislead contributors about how the project is organized.Update the tree to reflect the actual workspace structure.
🤖 Fix all issues with AI agents
In @.github/workflows/release-finalize.yml:
- Around line 80-88: The NOTES variable is being assigned a multi-line string
that preserves the YAML run: block indentation, causing leading spaces in
release_notes.md; change the assignment for NOTES (the branches that reference
COMMITS, IMPLEMENTATION, and RERELEASE_NOTE) to build the string using an
unindented heredoc or a method that strips indentation (e.g., assign via
NOTES=$(cat <<'EOF' ... EOF) or use printf to concatenate lines without the
run-block indentation) so the "## Changes in $IMPLEMENTATION" header, the
$COMMITS content, and $RERELEASE_NOTE are written with no extra leading spaces.
In @.github/workflows/release-python.yml:
- Around line 232-248: The multi-line if: block for the finalize job is being
treated as a non-empty string because it includes the ${ { } } delimiters inside
a YAML block; remove the ${ { and } } wrappers and leave the boolean expression
itself in the block (so the if: uses the implicit expression form), e.g. change
the finalize job's if: to the same multiline expression without ${ { } }, and
make the identical fix for the bump-version job so both jobs evaluate the
conditional properly instead of always being truthy.
In `@python-interpreter/packages/afm-cli/pyproject.toml`:
- Around line 13-16: The dependency list in pyproject.toml pins afm-core to
0.1.3 but leaves afm-langchain unpinned; update the dependencies array to pin
afm-langchain to a specific version or compatible range (e.g., match the
released afm-langchain package version) so it will not resolve to an arbitrary
PyPI release—modify the entry for "afm-langchain" in the dependencies block to
the chosen pinned version or range that is known compatible with afm-core.
In `@python-interpreter/packages/afm-core/tests/test_cli.py`:
- Around line 54-58: The test_version test calls meta_version("afm-cli") which
raises PackageNotFoundError when the afm-cli package isn't installed; update
test_version to handle that by trying to get meta_version("afm-cli") inside a
try/except (catching importlib.metadata.PackageNotFoundError) or by
conditionally asserting only if meta_version succeeds; reference the test
function test_version and the meta_version call and mirror the _version_callback
behavior so the test passes when afm-cli is absent.
In `@RELEASING.md`:
- Around line 124-129: Update the workflow filename references in RELEASING.md
so they match the actual workflow file name (release.yml): replace any
occurrences of `release-common.yml` with `release.yml` in the "Workflow Files"
table (the table with header "File | Purpose") and in the "Summary" table (the
block around lines 133-138) so both tables consistently reference `release.yml`
for the ballerina workflow; ensure no other mentions of `release-common.yml`
remain in the document.
- Line 12: The section heading "Langchain Interpreter (Python)" is misleading;
update the heading to "Python Interpreter" to match the renamed directory
`python-interpreter` and the described scope covering `afm-core` and
`afm-langchain`, and ensure any internal references in that section that mention
"Langchain Interpreter" are updated to "Python Interpreter" as well.
🧹 Nitpick comments (18)
.github/workflows/python-interpreter.yml (2)
37-41: Consider running format/lint checks before tests for faster feedback.Formatting and lint checks are typically faster than test suites. Moving them before
pytestwould give quicker CI feedback on style violations.Suggested reordering
- name: Install dependencies run: uv sync --dev + - name: Check formatting + run: uv run ruff format --check . + + - name: Lint with ruff + run: uv run ruff check . + - name: Run tests run: uv run pytest - - name: Check formatting - run: uv run ruff format --check . - - - name: Lint with ruff - run: uv run ruff check .
77-78:docker/build-push-actioncan be updated to v6.v6 is the current major version of this action.
Suggested update
- name: Build and push Docker image - uses: docker/build-push-action@v5 + uses: docker/build-push-action@v6python-interpreter/README.md (1)
1-3: Consider updating the subtitle to reflect the broader scope.Line 1 was renamed to "AFM Python Interpreter," but Line 3 still describes this as "A LangChain-based reference implementation." Since the refactor introduces pluggable backends (the whole point of the rename), the description could be updated to reflect that LangChain is now one of potentially many backends.
python-interpreter/packages/afm-langchain/src/afm_langchain/tools/mcp.py (1)
137-137: Unused field_toolsonMCPClient.
self._toolsis declared but never assigned or read withinMCPClient. The tool caching is handled at theMCPManagerlevel instead. Consider removing this dead field.♻️ Suggested cleanup
) -> None: """Initialize an MCP client.""" self.name = name self.url = url self.authentication = authentication self.tool_filter = tool_filter - self._tools: list[BaseTool] | None = Nonepython-interpreter/packages/afm-langchain/src/afm_langchain/providers.py (1)
116-149:_get_api_keyfallback logic is reasonable but theelseclause could benefit from a comment.When
auth_typeis"bearer"with a falsytokenor"api-key"with a falsyapi_key, theif/elifchain falls through to theelsedict-inspection block. This works but is subtle — the model validator onClientAuthenticationshould preventNonevalues, but empty strings could still slip through. A brief inline comment explaining the fallback intent would help future readers.python-interpreter/packages/afm-core/src/afm/__init__.py (1)
17-17: Hardcoded__version__is fine but risks drift withpyproject.toml.Consider using
importlib.metadata.version("afm-core")to derive the version from the installed package metadata (single source of truth), especially since the PR mentions automated version bumping in the release workflow.♻️ Alternative: derive version from package metadata
-__version__ = "0.1.3" +from importlib.metadata import version + +__version__ = version("afm-core")python-interpreter/packages/afm-core/src/afm/runner.py (1)
109-109: Prefer lazy%-style formatting for logger calls.Using f-strings in logging calls evaluates the string unconditionally, even when the log level is disabled. Use
logger.info("Loaded runner: %s (%s)", ep.name, ep.value)instead.Suggested fix
- logger.info(f"Loaded runner: {ep.name} ({ep.value})") + logger.info("Loaded runner: %s (%s)", ep.name, ep.value)python-interpreter/packages/afm-core/src/afm/update.py (2)
102-111: Executable-path heuristic for detecting install context is fragile.Substring checks like
"pipx" in executableor"uv" in executablecan produce false positives — e.g., a username containing "uv" (/home/uvalentine/...) or a path with "pipx" in a non-pipx context. Since this only affects the upgrade command suggestion in a non-critical notification, the impact is low, but you could tighten the match.Slightly more robust alternative
def _detect_upgrade_command() -> str: """Detect the installation context and return the appropriate upgrade command string.""" executable = sys.executable or "" + # Normalize to forward slashes for consistent matching + parts = executable.replace("\\", "/").split("/") - if "pipx" in executable: + if "pipx" in parts: return f"pipx upgrade {PYPI_PACKAGE}" - elif "uv" in executable: + elif "uv" in parts: return f"uv tool upgrade {PYPI_PACKAGE}" else: return f"pip install -U {PYPI_PACKAGE}"
155-199:get_update_notification()unconditionally setsnotified_versionwithout checking it first.Unlike
notify_if_update_available()(line 244),get_update_notification()doesn't check whethernotified_versionalready matcheslatestbefore returning the message and saving state. This means every call toget_update_notification()that finds an update will return the message and overwrite the state, regardless of prior notification.In the current flow this works because
get_update_notification()is called in the console chat path (which replaces the stderr notification), but if both functions are ever called for the same UI flow, the dedup guard innotify_if_update_available()will be pre-empted. Consider adding the samenotified_versioncheck for consistency.python-interpreter/packages/afm-core/pyproject.toml (1)
27-28: Duplicateafmscript entry point acrossafm-coreandafm-cli.Both
afm-core(line 28) andafm-cli(line 19 of itspyproject.toml) declareafm = "afm.cli:cli". While they resolve to the same target, having the same console script in two packages can cause pip/uv warnings and makes the ownership ambiguous. Consider keeping the entry point only inafm-cli(the user-facing metapackage) and removing it fromafm-core.python-interpreter/packages/afm-core/src/afm/cli.py (3)
285-294: Narrow the exception handler toPackageNotFoundError.
except Exceptionis overly broad. The only expected failure here isafm-clinot being installed.Proposed fix
+from importlib.metadata import PackageNotFoundError, version -from importlib.metadata import version ... def _version_callback(ctx: click.Context, _param: click.Parameter, value: bool) -> None: if not value or ctx.resilient_parsing: return core_version = version("afm-core") try: cli_version = version("afm-cli") click.echo(f"afm-cli {cli_version} (afm-core {core_version})") - except Exception: + except PackageNotFoundError: click.echo(f"afm-core {core_version}") ctx.exit()
315-331:"Loading:"is printed after parsing has already completed.In the
validatecommand,click.echo(f"Loading: {file}")at line 330 runs only afterparse_afm_filesucceeds. Consider moving it before thetryblock so it prints immediately, or renaming to something like"Validated:"to better reflect the state.Proposed fix (option A: move before parsing)
def validate(file: Path) -> None: + click.echo(f"Loading: {file}") try: afm = parse_afm_file(str(file), resolve_env=False) except AFMError as e: raise click.ClickException(f"Failed to parse AFM file: {e}") from e except Exception as e: raise click.ClickException(f"Unexpected error parsing AFM file: {e}") from e - click.echo(f"Loading: {file}") click.echo(format_validation_output(afm))
448-455: Runner constructor convention (runner_cls(afm)) is not enforced by theAgentRunnerprotocol.The protocol defines runtime methods and properties but not
__init__. If a third-party runner expects a different constructor signature, this will fail at runtime. Consider documenting this convention (e.g., in theAgentRunnerprotocol docstring or a factory method) so backend authors know they must accept a singleAFMRecordpositional argument.python-interpreter/packages/afm-langchain/src/afm_langchain/backend.py (2)
250-253:self.toolsproperty re-evaluated on every tool call iteration.Inside the agent loop,
self.tools(line 253) calls_get_all_tools()which concatenates two lists on each access. Consider caching the tool list before the loop (similar toall_toolson line 91) to avoid redundant list allocations per iteration.♻️ Suggested improvement
+ available_tools = self.tools # Main agent loop to handle tool calls while iterations < max_iterations: # ... for tool_call in response.tool_calls: tool_name = tool_call["name"] - tool = next((t for t in self.tools if t.name == tool_name), None) + tool = next((t for t in available_tools if t.name == tool_name), None)
297-304: Exception handling: consider a dedicatedexcept AgentErrorclause.The current pattern catches
AgentErrorinside the genericexcept Exceptionand re-raises it viaisinstancecheck. A separateexcept AgentError: raiseclause before the generic handler would be cleaner and more explicit.♻️ Suggested simplification
except InputValidationError: raise except OutputValidationError: raise + except AgentError: + raise except Exception as e: - if isinstance(e, AgentError): - raise raise AgentError(f"Agent execution failed: {e}") from e.github/workflows/release-finalize.yml (2)
63-70: Release notes for Python packages will include commits from all packages.The path filter
-- "$IMPLEMENTATION/"resolves topython-interpreter/for bothafm-coreandafm-langchainreleases. This means the release notes for anafm-corerelease will also include commits that only touchedafm-langchainfiles, and vice versa.Consider using a more specific path filter based on the
tagor a new input to narrow the commit scope per package.
40-51: No error handling if the branch or tag push fails.If
git push originon Line 50 or 51 fails (e.g., due to a race condition or permission issue), the workflow continues to the next step, potentially creating a GitHub Release pointing to a non-existent tag. Consider addingset -euo pipefailat the start of the script to fail fast on any command error.📝 Proposed fix
run: | + set -euo pipefail git config user.name "github-actions[bot]" git config user.email "github-actions[bot]@users.noreply.github.com".github/workflows/release.yml (1)
232-242: Missing fallback when no version file is found in bump-version.If neither
Ballerina.tomlnorpyproject.tomlexists (e.g., directory structure changes between validate and bump),VERSION_FILEwill be unset and thesedcommand on Line 239 will fail with an unclear error. Whilevalidatechecks this earlier, adding anelseclause here would make failures easier to diagnose.📝 Proposed fix
if [ -f "$IMPLEMENTATION/Ballerina.toml" ]; then VERSION_FILE="$IMPLEMENTATION/Ballerina.toml" elif [ -f "$IMPLEMENTATION/pyproject.toml" ]; then VERSION_FILE="$IMPLEMENTATION/pyproject.toml" + else + echo "::error::No version file found for $IMPLEMENTATION" + exit 1 fi
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
python-interpreter/README.md (1)
43-49: Consider updating Docker image name to reflect generic architecture.The Docker image is still named
afm-langchain-interpreter, which implies a LangChain-specific implementation. Since the project now supports pluggable backends, consider using a more generic name likeafm-python-interpreterorafm-interpreterto align with the new architecture.If the Docker image specifically bundles the LangChain backend by default, the current naming may be intentional and acceptable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python-interpreter/README.md` around lines 43 - 49, The README uses the Docker image name afm-langchain-interpreter which implies a LangChain-only build; update the docker tag and run references to a more generic name (e.g., afm-python-interpreter or afm-interpreter) by changing the build command (docker build -t afm-langchain-interpreter .) and the run command invocation (afm-langchain-interpreter afm /app/agent.afm.md) to the chosen generic name, and if the image truly bundles LangChain by default, keep the current name but add a brief note explaining that it includes the LangChain backend.python-interpreter/packages/afm-core/tests/test_cli.py (2)
237-240: Missingexit_codeassertion.
runner.invokeresult is not checked for success. If the CLI exits with an error after setting up uvicorn config but before completing, this test would still pass. Consider addingassert result.exit_code == 0to catch unexpected failures.Proposed fix
- runner.invoke(cli, ["run", str(sample_agent_path), "--port", "9000"]) + result = runner.invoke(cli, ["run", str(sample_agent_path), "--port", "9000"]) + assert result.exit_code == 0 # Should have called uvicorn.run assert mock_uvicorn.run.called or mock_uvicorn.Config.called🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python-interpreter/packages/afm-core/tests/test_cli.py` around lines 237 - 240, The test calls runner.invoke(cli, ["run", str(sample_agent_path), "--port", "9000"]) but doesn't check the invocation result; capture the return value (e.g., result = runner.invoke(...)) and assert result.exit_code == 0 to ensure the CLI completed successfully. Keep the existing uvicorn assertions (mock_uvicorn.run.called or mock_uvicorn.Config.called) but add the exit_code check immediately after the invoke to catch failures from cli/run before or after uvicorn setup.
165-169: Unusedsample_minimal_pathfixture parameter in three tests.
test_requires_at_least_one_interface,test_creates_app_with_webhook, andtest_creates_app_with_both_interfacesall declaresample_minimal_pathas a parameter but never reference it. This unnecessarily triggers fixture setup and is misleading to readers.Proposed fix (example for one method; apply similarly to the other two)
- def test_requires_at_least_one_interface(self, sample_minimal_path: Path): + def test_requires_at_least_one_interface(self): agent = _make_mock_agent() with pytest.raises(ValueError, match="At least one HTTP interface"): create_unified_app(agent)Also applies to: 189-203, 205-219
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@python-interpreter/packages/afm-core/tests/test_cli.py` around lines 165 - 169, The three test functions test_requires_at_least_one_interface, test_creates_app_with_webhook, and test_creates_app_with_both_interfaces declare the fixture parameter sample_minimal_path but never use it; remove the unused parameter from each test signature (i.e., delete ", sample_minimal_path: Path" or "sample_minimal_path" from the def lines) so the fixture is not unnecessarily invoked and the test intent is clearer; verify no other references to sample_minimal_path exist inside those functions and run tests to confirm no further changes are needed..github/workflows/release-ballerina.yml (1)
91-112:fetch-depth: 0is unnecessary for the test job.The test job only needs to build and run tests — it doesn't inspect git history. Using the default shallow clone (
fetch-depth: 1) would be faster.📝 Proposed fix
- name: Checkout repository uses: actions/checkout@v4 with: ref: ${{ inputs.branch }} - fetch-depth: 0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release-ballerina.yml around lines 91 - 112, The workflow checkout step in the "test" job sets fetch-depth: 0 which forces a full clone; remove the fetch-depth key (or set it to 1) in the "Checkout repository" step that uses actions/checkout@v4 so the test job uses the default shallow clone and runs faster; update the checkout step under the test job accordingly (refer to the "test" job and the "Checkout repository" step that uses actions/checkout@v4 and the fetch-depth field)..github/workflows/release-docker.yml (1)
84-98: Trivy scan runs post-push — vulnerable images are briefly available on GHCR.The vulnerability scan (line 84) executes after the image is pushed (line 67). If the scan fails, the image has already been published. This is a common pattern and may be acceptable, but if the goal is to gate publication on scan results, consider building without pushing first, scanning the local image, then pushing only on success.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release-docker.yml around lines 84 - 98, The Trivy scan ("Scan Docker image for vulnerabilities") runs after the image push step, so a failed scan still leaves a published image; change the workflow to build the image without pushing (use the existing build step or docker/build-push-action with push: false and tag matching steps.docker-tags.outputs.FULL_IMAGE), run the Trivy scan against that local tag (image-ref pointing to the local image), and only run the push step (the current "Push Docker image" step) conditionally on the scan succeeding (use job/step outputs or job status to gate the push); keep the "Upload Trivy scan results" step but run it regardless for visibility.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/release-ballerina.yml:
- Around line 158-162: The current sed command replaces every line matching
^version = "..." in VERSION_FILE, corrupting dependency versions; change the
replacement to only affect the first match by using the sed range for the first
occurrence, e.g. replace the sed invocation in the block that touches
VERSION_FILE with: sed -i '0,/^version = ".*"/s//version = "'"$NEXT_VERSION"'"'
"$VERSION_FILE" (i.e. update the sed command that currently reads sed -i
"s/^version = \".*\"/version = \"$NEXT_VERSION\"/" to use the 0,/pattern/ range
so only the first ^version = ... line is updated).
In @.github/workflows/release-docker.yml:
- Around line 49-65: The workflow currently sets UPDATE_LATEST when
inputs.branch == 'main' || inputs.branch == 'dev', causing dev builds to be
tagged as :latest; change the condition to only set UPDATE_LATEST for main
(remove the dev branch) so the run block that appends ,$FULL_IMAGE:latest only
executes for main; specifically update the env key UPDATE_LATEST (or the branch
check used in the run block) so it uses inputs.branch == 'main' and leave the
TAGS/FULL_IMAGE/TAGS append logic unchanged.
- Around line 84-98: Update the GitHub Actions step versions to pinned, current
releases: replace the aquasecurity action reference
aquasecurity/trivy-action@0.33.1 with aquasecurity/trivy-action@0.34.0, and
replace the upload action github/codeql-action/upload-sarif@v4 with a fixed
release tag such as github/codeql-action/upload-sarif@v4.31.10 (or another
chosen exact v4.x.y), ensuring the steps that use image-ref, format, output and
sarif_file remain unchanged.
---
Duplicate comments:
In @.github/workflows/release-finalize.yml:
- Around line 72-105: The heredoc blocks using "cat <<-EOF > release_notes.md"
are indented with spaces so the dash-only variant only strips tabs and leaves
leading spaces in the generated release_notes.md; update each "cat <<-EOF >
release_notes.md" block so the heredoc content is dedented to column 0 (move the
lines starting with "## Changes in $IMPLEMENTATION", "$COMMITS", and the "No
changes..." text to flush-left) or alternatively replace the indented "cat
<<-EOF" usage with an unindented "cat <<'EOF' > release_notes.md" / dedented
heredoc closing token so the output has no extra leading spaces; ensure you
update all occurrences that reference $IMPLEMENTATION and $COMMITS accordingly.
In `@python-interpreter/packages/afm-core/tests/test_cli.py`:
- Around line 54-61: Test already updated to handle PackageNotFoundError for
afm-cli in test_version: keep the try/except around the assert
meta_version("afm-cli") in result.output and ensure the fallback assertion
asserting f"afm-core {meta_version('afm-core')}" remains in the except block;
verify the test function name test_version and calls to runner.invoke(cli,
["--version"]) and meta_version are unchanged so the behavior and fallback are
preserved.
---
Nitpick comments:
In @.github/workflows/release-ballerina.yml:
- Around line 91-112: The workflow checkout step in the "test" job sets
fetch-depth: 0 which forces a full clone; remove the fetch-depth key (or set it
to 1) in the "Checkout repository" step that uses actions/checkout@v4 so the
test job uses the default shallow clone and runs faster; update the checkout
step under the test job accordingly (refer to the "test" job and the "Checkout
repository" step that uses actions/checkout@v4 and the fetch-depth field).
In @.github/workflows/release-docker.yml:
- Around line 84-98: The Trivy scan ("Scan Docker image for vulnerabilities")
runs after the image push step, so a failed scan still leaves a published image;
change the workflow to build the image without pushing (use the existing build
step or docker/build-push-action with push: false and tag matching
steps.docker-tags.outputs.FULL_IMAGE), run the Trivy scan against that local tag
(image-ref pointing to the local image), and only run the push step (the current
"Push Docker image" step) conditionally on the scan succeeding (use job/step
outputs or job status to gate the push); keep the "Upload Trivy scan results"
step but run it regardless for visibility.
In `@python-interpreter/packages/afm-core/tests/test_cli.py`:
- Around line 237-240: The test calls runner.invoke(cli, ["run",
str(sample_agent_path), "--port", "9000"]) but doesn't check the invocation
result; capture the return value (e.g., result = runner.invoke(...)) and assert
result.exit_code == 0 to ensure the CLI completed successfully. Keep the
existing uvicorn assertions (mock_uvicorn.run.called or
mock_uvicorn.Config.called) but add the exit_code check immediately after the
invoke to catch failures from cli/run before or after uvicorn setup.
- Around line 165-169: The three test functions
test_requires_at_least_one_interface, test_creates_app_with_webhook, and
test_creates_app_with_both_interfaces declare the fixture parameter
sample_minimal_path but never use it; remove the unused parameter from each test
signature (i.e., delete ", sample_minimal_path: Path" or "sample_minimal_path"
from the def lines) so the fixture is not unnecessarily invoked and the test
intent is clearer; verify no other references to sample_minimal_path exist
inside those functions and run tests to confirm no further changes are needed.
In `@python-interpreter/README.md`:
- Around line 43-49: The README uses the Docker image name
afm-langchain-interpreter which implies a LangChain-only build; update the
docker tag and run references to a more generic name (e.g.,
afm-python-interpreter or afm-interpreter) by changing the build command (docker
build -t afm-langchain-interpreter .) and the run command invocation
(afm-langchain-interpreter afm /app/agent.afm.md) to the chosen generic name,
and if the image truly bundles LangChain by default, keep the current name but
add a brief note explaining that it includes the LangChain backend.
|
|
||
| VERSION_FILE="ballerina-interpreter/Ballerina.toml" | ||
| sed -i "s/^version = \".*\"/version = \"$NEXT_VERSION\"/" "$VERSION_FILE" | ||
| git add "$VERSION_FILE" | ||
| git commit -m "Bump $IMPLEMENTATION version to $NEXT_VERSION" | ||
| git commit -m "Bump ballerina-interpreter version to $NEXT_VERSION" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find Ballerina.toml files in the repository
fd "Ballerina.toml" --type fRepository: wso2/reference-implementations-afm
Length of output: 114
🏁 Script executed:
# Read the Ballerina.toml file and check for lines matching ^version =
cat ballerina-interpreter/Ballerina.tomlRepository: wso2/reference-implementations-afm
Length of output: 1777
sed replaces all version entries in Ballerina.toml, corrupting dependency versions.
Line 160's sed -i "s/^version = \".*\"/version = \"$NEXT_VERSION\"/" replaces every line matching ^version = "...". The Ballerina.toml file has 10 such lines: the package version (line 3) plus 9 dependency versions (lines 10, 15, 20, 25, 30, 35, 40, 45, 50). This would set all dependency versions to $NEXT_VERSION, breaking the build. The read step (line 53) correctly uses head -1 to grab only the first match, but the write step lacks this constraint.
📝 Proposed fix — limit replacement to the first match
- sed -i "s/^version = \".*\"/version = \"$NEXT_VERSION\"/" "$VERSION_FILE"
+ sed -i '0,/^version = ".*"/{s/^version = ".*"/version = "'"$NEXT_VERSION"'"/}' "$VERSION_FILE"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/release-ballerina.yml around lines 158 - 162, The current
sed command replaces every line matching ^version = "..." in VERSION_FILE,
corrupting dependency versions; change the replacement to only affect the first
match by using the sed range for the first occurrence, e.g. replace the sed
invocation in the block that touches VERSION_FILE with: sed -i '0,/^version =
".*"/s//version = "'"$NEXT_VERSION"'"' "$VERSION_FILE" (i.e. update the sed
command that currently reads sed -i "s/^version = \".*\"/version =
\"$NEXT_VERSION\"/" to use the 0,/pattern/ range so only the first ^version =
... line is updated).
- Handle scenarios wehre only afm-core is installed - Notify every time instead of one time as other applications do
Previously no matter the installation method used, the helper text would suggest uv add afm-langchain. With this change similar to update notifications we will see a dynamic msg.
There was a problem hiding this comment.
Pull request overview
This PR implements a comprehensive architectural refactor of the Python AFM interpreter, transitioning from a monolithic structure to a modular multi-package workspace with pluggable execution backends.
Changes:
- Restructures code into 3 packages:
afm-core(parser, CLI, protocols),afm-langchain(LangChain backend), andafm-cli(metapackage) - Introduces AgentRunner protocol for pluggable backends with entry-point-based discovery
- Adds comprehensive update notification system with package manager detection
- Refactors CI/CD to support independent package releases with dedicated workflows for Python and Ballerina
Reviewed changes
Copilot reviewed 61 out of 72 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
python-interpreter/pyproject.toml |
Workspace configuration with uv and pytest settings |
python-interpreter/packages/afm-core/pyproject.toml |
Core package configuration (v0.1.7) with CLI and interface dependencies |
python-interpreter/packages/afm-langchain/pyproject.toml |
LangChain backend (v0.1.4) with runner entry point registration |
python-interpreter/packages/afm-cli/pyproject.toml |
Metapackage (v0.2.10) that bundles core and langchain |
python-interpreter/packages/afm-core/src/afm/runner.py |
AgentRunner protocol and pluggable backend discovery system |
python-interpreter/packages/afm-core/src/afm/update.py |
Update checker with PyPI integration and install command detection |
python-interpreter/packages/afm-core/src/afm/cli.py |
Refactored CLI with validate, run, and framework commands |
python-interpreter/packages/afm-core/src/afm/parser.py |
Added resolve_env parameter for validation without env vars |
python-interpreter/packages/afm-langchain/src/afm_langchain/backend.py |
Renamed Agent class to LangChainRunner |
.github/workflows/release-python.yml |
New Python release workflow supporting per-package releases |
.github/workflows/release-docker.yml |
Reusable Docker build/push/scan workflow |
.github/workflows/release-finalize.yml |
Reusable finalization workflow for tags and releases |
python-interpreter/Dockerfile |
Updated multi-stage build for workspace packages |
| Multiple test files | Updated imports, mocks, and fixtures for new architecture |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 61 out of 72 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Will add a separate workflow for this later
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 61 out of 72 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
python-interpreter/packages/afm-core/src/afm/update.py:1
- The magic number
0x00000008lacks clarity. Consider defining a named constant likeDETACHED_PROCESS = 0x00000008to improve code readability and maintainability.
python-interpreter/packages/afm-core/tests/test_update.py:1 - In test file line references, this appears to test the actual implementation. The test for Windows-specific
creationflagsshould verify the flag is set correctly. Consider adding an explicit test case that verifies the DETACHED_PROCESS flag (0x00000008) is used on Windows platforms.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
README.md
Outdated
| Contributions are welcome! When adding a new implementation: | ||
|
|
||
| 1. Create a new directory: `{language/framework}-{type}/` (e.g., `langchain-interpreter/`) | ||
| 1. Create a new directory: `{language/framework}-{type}/` (e.g., `python-interpreter/`) |
There was a problem hiding this comment.
This and the rest still says python?
There was a problem hiding this comment.
Following our offline discussion, refactored the README files to reduce confusion b68cbb3
RadCod3
force-pushed
the
feat/afm-cli
branch
from
21f805c to
b68cbb3
Compare
README.md
Outdated
| ├── ballerina-interpreter/ # Ballerina-based AFM interpreter | ||
| ├── langchain-interpreter/ # LangChain-based AFM interpreter | ||
| └── .github/workflows/ # CI/CD (path-filtered per implementation) | ||
| ├── python-interpreter/ # Python-based AFM interpreters (plugin-based) |
There was a problem hiding this comment.
Can we have langchain-interpreter also here (one level down)?
README.md
Outdated
| > [!NOTE] | ||
| > Python-based implementations share a common runtime and CLI ([`afm-core`](./python-interpreter/packages/afm-core/)) that make it easy to switch between backends. LangChain is currently the supported backend; support for additional frameworks is planned. |
There was a problem hiding this comment.
Would redo this to match the user's PoV as discussed offline.
> [!NOTE]
> Python-based implementations are made available via a single package/CLI tool ... facilitating easily switching between backends. LangChain is currently the supported backend; support for additional frameworks is planned.
Co-authored-by: Maryam Ziyad <maryamziyadm@gmail.com>
Purpose
This PR implements a unified CLI tool for AFM based on a modular, multi-package architecture.
Resolves #8
High-Level Architecture
The Python implementation has been restructured from a monolithic script into a uv workspace that decouples the core AFM engine from specific execution frameworks. This design allows for a lightweight core and pluggable execution backends.
Modular Packages:
afm-core: A lightweight package containing the CLI logic, AFM parser, and Pydantic models. It defines the formalAgentRunnerprotocol and handles interface implementations (Console, Web, Webhook).afm-langchain: A concrete implementation of theAgentRunnerprotocol using LangChain. It handles LLM orchestration and prompt generation.afm-cli(The Metapackage): A "one-click" convenience package that installs both the core engine and the LangChain adapter, providing an immediate working environment for users.Pluggable Discovery & Lazy Loading:
importlib.metadatato scan for installed packages that register under theafm.runnerentry point. This allows users to add new frameworks (e.g.,afm-llamaindex) simply by installing them.CI/CD & Release Workflows:
To support this multi-package structure, the release process has been completely modularized.
release-python.yml: A dedicated workspace-aware workflow for Python.afm-coreandafm-langchain.uv.release-docker.ymlfor container image management.release-ballerina.yml: A dedicated workflow for the Ballerina implementation.release-docker.ymlfor container image management.release-docker.yml(New Reusable): Standardizes the Docker build-push-scan pipeline.release-finalize.yml(Reusable): Standardizes the finalization of releases (Git tags, release branches, and GitHub Releases) across all implementations.<package-or-implementation>-v<version>(e.g.,afm-core-v0.1.0orballerina-interpreter-v0.1.0).release-<package-or-implementation>-<version>(e.g.,release-afm-core-0.1.0).:v<version>.:latesttag is only updated during releases frommainordevbranches to ensure stable production tags are maintained.afm-langchain-interpreter,afm-ballerina-interpreter) to avoid collisions and provide clarity in the registry.Summary by CodeRabbit
New Features
Documentation
Tests
Chores