🧪 Add koncur-mcp action and image build for analyzer-mcp - #207
🧪 Add koncur-mcp action and image build for analyzer-mcp#207JonahSussman wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughAdds a callable GitHub Actions workflow to run MCP end-to-end tests, updates the nightly image build matrix to include analyzer-mcp, and introduces a composite action that loads test images into podman, configures providers, builds and runs Koncur MCP tests, and uploads artifacts. Changes
Sequence DiagramsequenceDiagram
participant GHA as "GitHub Actions"
participant ImageBuild as "Image Build\nWorkflow"
participant Artifacts as "Artifact Storage"
participant KoncurAction as "Koncur MCP\nComposite Action"
participant Podman as "Podman"
participant Koncur as "Koncur CLI"
GHA->>ImageBuild: call build-images (reusable workflow)
ImageBuild->>Artifacts: publish image artifacts
GHA->>KoncurAction: run-koncur-action (with image_pattern/ref)
KoncurAction->>Artifacts: download image artifacts
KoncurAction->>Podman: load images, create volume & pod
KoncurAction->>Podman: start Java + Go providers
KoncurAction->>KoncurAction: extract analyzer-mcp binary & rules
KoncurAction->>Koncur: checkout & build Koncur (Go 1.25)
KoncurAction->>Koncur: run "koncur run tests" for MCP
Koncur->>Artifacts: upload test outputs (on failure/always)
KoncurAction->>Podman: remove analyzer-mcp-pod
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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. 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: 4
🧹 Nitpick comments (4)
koncur-mcp/action.yml (4)
98-102: Fixedsleep 10is a race waiting to happen.The providers are gRPC services on ports 14651/14653. A hard 10s sleep will either waste CI time or, on a slow runner, let
analyzer-mcpstart before the providers accept connections. Prefer polling, e.g.:for port in 14651 14653; do for i in {1..30}; do (echo > /dev/tcp/127.0.0.1/$port) >/dev/null 2>&1 && break sleep 1 done doneAlso worth checking
podman logs java-provider/golang-provideron failure to aid debugging.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@koncur-mcp/action.yml` around lines 98 - 102, Replace the brittle "Wait for providers to be ready" step that just does sleep 10 with a polling loop that checks the gRPC ports 14651 and 14653 until they accept connections (with a reasonable retry count/timeout) so the "Wait for providers to be ready" step reliably blocks until services are up; update the action step (named "Wait for providers to be ready") to attempt TCP connects to 127.0.0.1:14651 and :14653 in a loop (break on success, fail after retries), and on failure emit podman logs for java-provider and golang-provider to aid debugging.
108-111: Leakmcp-extract/rules-extractcontainers onpodman cpfailure.The
&&betweenpodman cp ...andpodman rm ...means a failed copy leaves the stopped container behind. Since the pod cleanup at the end only removesanalyzer-mcp-pod, these orphans persist (admittedly not a big deal on ephemeral runners, but it does hide the original error). Atrapor unconditionalpodman rm -fwould be more robust:- podman cp $(podman create --name mcp-extract $MCP_IMG):/usr/local/bin/analyzer-mcp . && \ - podman rm mcp-extract + podman create --name mcp-extract "$MCP_IMG" >/dev/null + trap 'podman rm -f mcp-extract >/dev/null 2>&1 || true' EXIT + podman cp mcp-extract:/usr/local/bin/analyzer-mcp .Same pattern applies to
rules-extractat Lines 117-118.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@koncur-mcp/action.yml` around lines 108 - 111, The podman create + podman cp sequence can leave stopped containers (mcp-extract and rules-extract) if podman cp fails because the current script uses && to only rm on success; change to always remove those temporary containers on exit or after the cp: ensure the script registers a trap to run podman rm -f mcp-extract (and podman rm -f rules-extract) or unconditionally run podman rm -f right after the cp attempt so the temporary containers are cleaned up even on failure (apply the same fix for the analyzer-mcp extraction and the rules-extract extraction).
42-58: Image-name regex matching is fragile and order-dependent.A few concerns with the pattern matching:
podman loadoutput format isn't guaranteed to be a singleLoaded image: <name>; multi-tag tarballs or newer podman versions can emit multiple lines, andawk '{print $NF}'on a multi-line stdout will concatenate fields and mis-capture the reference..*kantra.*will also matchquay.io/konveyor/kantra-*or any future image whose name happens to contain "kantra". Similarly, if any image tag/registry path contains "provider" (e.g. a futureanalyzer-mcp-provider), it could be misclassified.- If neither
analyzer-mcp,java*-provider,generic*-provider, norkantramatches, the image is silently ignored — no warning is printed.Consider anchoring on the image short name only (e.g.
basenameof the repo path), and emitting a warning when a loaded image doesn't match any expected bucket. Also consider failing the step if any of the four required env vars ends up empty.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@koncur-mcp/action.yml` around lines 42 - 58, The image matching loop is fragile: replace the current naive parsing of podman load output and regexes (the loaded=$(podman load -i "${image}" | awk '{print $NF}') and the .*analyzer-mcp.*, .*java(-external)?-provider.*, .*generic(-external)?-provider.*, .*kantra.* checks) with robust extraction of each image reference emitted by podman load (iterate over each output line, parse the actual image reference), compute the image short name (basename of the repo path, e.g. strip registry and namespace) and then anchor matches against that short name (use exact or anchored regexes like ^analyzer-mcp(-.*)?$, ^java(-external)?-provider(-.*)?$, ^generic(-external)?-provider(-.*)?$, ^kantra(-.*)?$) to set MCP_IMG, JAVA_PROVIDER_IMG, GENERIC_PROVIDER_IMG, RUNNER_IMG; emit a warning when a loaded image does not match any bucket and, after the loop, fail the step if any of MCP_IMG, JAVA_PROVIDER_IMG, GENERIC_PROVIDER_IMG, or RUNNER_IMG is empty.
221-224: Cleanup doesn't remove thetest-datavolume or extraction containers.
podman pod rm -f analyzer-mcp-podhandles the pod, but thetest-datavolume from Line 65 and any leftovermcp-extract/rules-extractcontainers aren't cleaned up. On self-hosted runners this would accumulate; on ephemeral GitHub-hosted runners it's only a hygiene issue. Consider:- run: podman pod rm -f analyzer-mcp-pod || true + run: | + podman pod rm -f analyzer-mcp-pod || true + podman rm -f mcp-extract rules-extract 2>/dev/null || true + podman volume rm -f test-data || true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@koncur-mcp/action.yml` around lines 221 - 224, The "Cleanup pod" step only removes the pod (analyzer-mcp-pod) but leaves the test-data volume and extraction containers (mcp-extract, rules-extract) behind; update the Cleanup pod step to also remove those resources by explicitly stopping/removing the mcp-extract and rules-extract containers and deleting the test-data volume (use podman rm -f for the containers and podman volume rm -f test-data or equivalent safe removal commands), ensuring the step still uses if: always() so cleanup runs regardless of job outcome.
🤖 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/e2e-mcp-koncur.yaml:
- Around line 3-15: The workflow is missing inputs under the workflow_call
trigger so callers can't pass repo/ref; add an inputs block under workflow_call
mirroring workflow_dispatch.inputs (declare repo with type: string, required:
false and default: github.repository or empty, and ref with type: string,
required: false, default: main) or create a shared YAML anchor and reference it
for both workflow_call and workflow_dispatch; update the repo/ref input
declarations (workflow_dispatch.inputs.repo) to explicitly include type and
required fields so the values used later (inputs.repo / inputs.ref) are
populated when invoked via workflow_call.
In `@koncur-mcp/action.yml`:
- Around line 28-36: The download-artifacts step (id: download-artifacts)
currently uses continue-on-error: true which masks missing artifacts; remove
that flag (or set it false) so the workflow fails when no artifacts are found,
and/or after the "Load images into podman" loop add an explicit assertion that
MCP_IMG, JAVA_PROVIDER_IMG, GENERIC_PROVIDER_IMG, and RUNNER_IMG are non-empty
(fail the job with a clear error if any are unset) to surface the real cause
instead of allowing podman run to error later; reference the
inputs.image_pattern and the "Load images into podman" loop when implementing
the check.
- Around line 3-16: Remove the unused skip_maven input block (the entire
skip_maven entry) and strip all unsupported type: fields from the remaining
inputs in action.yml (remove the type: lines under image_pattern and ref);
because composite action inputs are always strings, do not rely on non-string
defaults and ensure any defaults left are literal strings (ref's default "main"
is fine) so the composite action only contains string-compatible input
declarations like image_pattern and ref without type: annotations.
- Around line 171-197: The checkout step (name: Checkout koncur, uses:
actions/checkout@v6) is wiping provider-config.json and rulesets created earlier
because it defaults to cleaning the workspace; update the workflow so those
files survive: either check the koncur repo out into a subdirectory (set a path
for actions/checkout and then build with go build in that directory, e.g.,
adjust the Build Koncur step to run the build from the koncur subdir) and keep
the Create MCP target config step referencing $(pwd)/provider-config.json and
$(pwd)/rulesets from the workspace root, or move provider-config.json and
rulesets into ${RUNNER_TEMP} before the checkout and update the Create MCP
target config step to use those absolute ${RUNNER_TEMP}/provider-config.json and
${RUNNER_TEMP}/rulesets paths (ensure the Create MCP target config step uses
those preserved locations when writing target-mcp.yaml).
---
Nitpick comments:
In `@koncur-mcp/action.yml`:
- Around line 98-102: Replace the brittle "Wait for providers to be ready" step
that just does sleep 10 with a polling loop that checks the gRPC ports 14651 and
14653 until they accept connections (with a reasonable retry count/timeout) so
the "Wait for providers to be ready" step reliably blocks until services are up;
update the action step (named "Wait for providers to be ready") to attempt TCP
connects to 127.0.0.1:14651 and :14653 in a loop (break on success, fail after
retries), and on failure emit podman logs for java-provider and golang-provider
to aid debugging.
- Around line 108-111: The podman create + podman cp sequence can leave stopped
containers (mcp-extract and rules-extract) if podman cp fails because the
current script uses && to only rm on success; change to always remove those
temporary containers on exit or after the cp: ensure the script registers a trap
to run podman rm -f mcp-extract (and podman rm -f rules-extract) or
unconditionally run podman rm -f right after the cp attempt so the temporary
containers are cleaned up even on failure (apply the same fix for the
analyzer-mcp extraction and the rules-extract extraction).
- Around line 42-58: The image matching loop is fragile: replace the current
naive parsing of podman load output and regexes (the loaded=$(podman load -i
"${image}" | awk '{print $NF}') and the .*analyzer-mcp.*,
.*java(-external)?-provider.*, .*generic(-external)?-provider.*, .*kantra.*
checks) with robust extraction of each image reference emitted by podman load
(iterate over each output line, parse the actual image reference), compute the
image short name (basename of the repo path, e.g. strip registry and namespace)
and then anchor matches against that short name (use exact or anchored regexes
like ^analyzer-mcp(-.*)?$, ^java(-external)?-provider(-.*)?$,
^generic(-external)?-provider(-.*)?$, ^kantra(-.*)?$) to set MCP_IMG,
JAVA_PROVIDER_IMG, GENERIC_PROVIDER_IMG, RUNNER_IMG; emit a warning when a
loaded image does not match any bucket and, after the loop, fail the step if any
of MCP_IMG, JAVA_PROVIDER_IMG, GENERIC_PROVIDER_IMG, or RUNNER_IMG is empty.
- Around line 221-224: The "Cleanup pod" step only removes the pod
(analyzer-mcp-pod) but leaves the test-data volume and extraction containers
(mcp-extract, rules-extract) behind; update the Cleanup pod step to also remove
those resources by explicitly stopping/removing the mcp-extract and
rules-extract containers and deleting the test-data volume (use podman rm -f for
the containers and podman volume rm -f test-data or equivalent safe removal
commands), ensuring the step still uses if: always() so cleanup runs regardless
of job outcome.
🪄 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: 0e68e549-d179-4e24-8a46-9d1bf47cc00b
📒 Files selected for processing (3)
.github/workflows/e2e-mcp-koncur.yaml.github/workflows/nightly-matrix-config.yamlkoncur-mcp/action.yml
| on: | ||
| workflow_call: | ||
| workflow_dispatch: | ||
| inputs: | ||
| repo: | ||
| description: | | ||
| This is the repo to simulate the testing from. | ||
| ref: | ||
| description: | | ||
| The ref that should be tested from. | ||
| required: false | ||
| type: string | ||
| default: main |
There was a problem hiding this comment.
workflow_call trigger is missing its own inputs block.
workflow_dispatch.inputs.repo / inputs.ref are declared, but workflow_call: on Line 4 is empty. When another workflow calls this one via workflow_call, inputs.repo and inputs.ref used below (Lines 22-23, 35) will always be empty strings, so the caller cannot parameterize repo/ref at all — only the dispatch path works.
Either duplicate the inputs under workflow_call, or define a shared anchor. Also, workflow_dispatch.inputs.repo is missing type: and required:, which defaults it to a required string with no default (click "Run workflow" with empty repo → empty string, then inputs.repo || github.repository falls back, so functionally OK, but worth declaring explicitly).
Proposed fix
on:
- workflow_call:
+ workflow_call:
+ inputs:
+ repo:
+ description: "This is the repo to simulate the testing from."
+ required: false
+ type: string
+ ref:
+ description: "The ref that should be tested from."
+ required: false
+ type: string
+ default: main
workflow_dispatch:
inputs:
repo:
description: |
This is the repo to simulate the testing from.
+ required: false
+ type: string
ref:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/e2e-mcp-koncur.yaml around lines 3 - 15, The workflow is
missing inputs under the workflow_call trigger so callers can't pass repo/ref;
add an inputs block under workflow_call mirroring workflow_dispatch.inputs
(declare repo with type: string, required: false and default: github.repository
or empty, and ref with type: string, required: false, default: main) or create a
shared YAML anchor and reference it for both workflow_call and
workflow_dispatch; update the repo/ref input declarations
(workflow_dispatch.inputs.repo) to explicitly include type and required fields
so the values used later (inputs.repo / inputs.ref) are populated when invoked
via workflow_call.
| inputs: | ||
| skip_maven: | ||
| description: "Whether to setup access to the testing maven packages" | ||
| default: true | ||
| type: boolean | ||
| image_pattern: | ||
| description: The pattern used to download the image's that have been built to test | ||
| required: false | ||
| type: string | ||
| ref: | ||
| description: The ref of koncur to use for testing | ||
| required: false | ||
| type: string | ||
| default: main |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "action.yml" -o -name "action.yaml" | head -20Repository: konveyor/ci
Length of output: 161
🏁 Script executed:
fd "action\.ya?ml" -t fRepository: konveyor/ci
Length of output: 153
🌐 Web query:
GitHub composite action inputs type field specification
💡 Result:
In GitHub composite actions, defined in action.yml with runs: using: "composite", the inputs section does not support a type field specification. Inputs are always treated as strings. Official documentation for metadata syntax lists supported fields for inputs.<input_id> as: description (required, string), required (optional, boolean), default (optional, string), and deprecationMessage (optional, string). No type field is mentioned for actions, including composite ones. Examples in composite action docs show inputs without type, using string defaults like 'World'. The type field (e.g., boolean, number, choice) is supported in workflow-level inputs like workflow_dispatch and workflow_call, but not in action metadata. Even if type: boolean is added to composite action inputs, it is ignored; values remain strings. This is confirmed by open GitHub issue #2238 in actions/runner (updated 2025), where boolean inputs behave as strings, requiring string comparisons like ${{ inputs.mybool == 'true' }}. No changelog or update indicates support as of 2026-04-16. Access inputs via ${{ inputs.myinput }} in steps (not $INPUT_MYINPUT env vars, which are unavailable in composites) or map to env:. Example action.yml: inputs: who-to-greet: description: 'Who to greet' required: true default: 'World' flag: description: 'A flag' required: false default: 'false' # type: boolean # Ignored/unsupported Usage treats flag as string.
Citations:
- 1: https://docs.github.com/actions/creating-actions/creating-a-composite-action
- 2: https://docs.github.com/en/actions/reference/workflows-and-actions/metadata-syntax
- 3: https://docs.github.com/en/actions/tutorials/create-actions/create-a-composite-action
- 4: https://docs.github.com/en/actions/creating-actions/metadata-syntax-for-github-actions
- 5: Boolean inputs are not actually booleans in composite actions actions/runner#2238
- 6: Boolean inputs are not actually booleans in composite actions actions/runner#2238
🏁 Script executed:
cat -n ./koncur-mcp/action.ymlRepository: konveyor/ci
Length of output: 8396
Remove unused input and unsupported type fields.
Composite action inputs don't support the type: field—it's silently ignored. All inputs are strings at runtime; skip_maven: default: true will be the string "true". Also, skip_maven is never referenced in any step and should be removed.
Suggested cleanup
inputs:
- skip_maven:
- description: "Whether to setup access to the testing maven packages"
- default: true
- type: boolean
image_pattern:
description: The pattern used to download the image's that have been built to test
required: false
- type: string
ref:
description: The ref of koncur to use for testing
required: false
- type: string
default: main🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@koncur-mcp/action.yml` around lines 3 - 16, Remove the unused skip_maven
input block (the entire skip_maven entry) and strip all unsupported type: fields
from the remaining inputs in action.yml (remove the type: lines under
image_pattern and ref); because composite action inputs are always strings, do
not rely on non-string defaults and ensure any defaults left are literal strings
(ref's default "main" is fine) so the composite action only contains
string-compatible input declarations like image_pattern and ref without type:
annotations.
| - name: Download Images from artifacts | ||
| id: download-artifacts | ||
| continue-on-error: true | ||
| if: ${{ inputs.image_pattern }} | ||
| uses: actions/download-artifact@v4 | ||
| with: | ||
| pattern: ${{ inputs.image_pattern }} | ||
| path: ${{ runner.temp }}/images | ||
| merge-multiple: true |
There was a problem hiding this comment.
continue-on-error: true on artifact download will mask a missing-artifact failure.
If the pattern matches zero artifacts (or the upstream build job didn't publish them with matching names), this step will swallow the error and the subsequent "Load images into podman" loop will run over an empty directory, leaving MCP_IMG, JAVA_PROVIDER_IMG, etc. unset. The later podman run $JAVA_PROVIDER_IMG ... will then fail with a confusing error instead of pointing at the real cause.
Consider removing continue-on-error (or at least asserting that MCP_IMG, JAVA_PROVIDER_IMG, GENERIC_PROVIDER_IMG, and RUNNER_IMG are all non-empty after the load step).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@koncur-mcp/action.yml` around lines 28 - 36, The download-artifacts step (id:
download-artifacts) currently uses continue-on-error: true which masks missing
artifacts; remove that flag (or set it false) so the workflow fails when no
artifacts are found, and/or after the "Load images into podman" loop add an
explicit assertion that MCP_IMG, JAVA_PROVIDER_IMG, GENERIC_PROVIDER_IMG, and
RUNNER_IMG are non-empty (fail the job with a clear error if any are unset) to
surface the real cause instead of allowing podman run to error later; reference
the inputs.image_pattern and the "Load images into podman" loop when
implementing the check.
| - name: Checkout koncur | ||
| uses: actions/checkout@v6 | ||
| with: | ||
| repository: konveyor/koncur | ||
| ref: ${{ inputs.ref }} | ||
|
|
||
| - name: Setup Go | ||
| uses: actions/setup-go@v6 | ||
| with: | ||
| go-version: '1.25' | ||
|
|
||
| - name: Build Koncur | ||
| shell: bash | ||
| run: | | ||
| go build -o $HOME/.local/bin/koncur ./cmd/koncur/main.go | ||
|
|
||
| - name: Create MCP target config | ||
| shell: bash | ||
| run: | | ||
| mkdir -p .koncur/config | ||
| cat > .koncur/config/target-mcp.yaml << EOF | ||
| type: mcp | ||
| mcp: | ||
| binaryPath: $HOME/.local/bin/analyzer-mcp | ||
| providerConfig: $(pwd)/provider-config.json | ||
| rules: $(pwd)/rulesets | ||
| EOF |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "action.yml" -type f | head -20Repository: konveyor/ci
Length of output: 161
🏁 Script executed:
cat -n ./koncur-mcp/action.ymlRepository: konveyor/ci
Length of output: 8396
🌐 Web query:
actions/checkout@v6 clean parameter default behavior GitHub Actions
💡 Result:
The default behavior of the clean parameter in actions/checkout@v6 is true. When clean: true (default), the action executes git clean -ffdx && git reset --hard HEAD before fetching/checkout to remove untracked files, directories, and reset changes in the target directory (default: $GITHUB_WORKSPACE). Set clean: false to skip this, useful when checking out multiple repos into subdirectories with the path parameter to avoid deleting files from prior checkouts.
Citations:
- 1: https://raw.githubusercontent.com/actions/checkout/v6/action.yml
- 2: https://github.com/actions/checkout/blob/main/action.yml
- 3: https://github.com/actions/checkout?tab=readme-ov-file
- 4: https://github.com/actions/checkout
- 5: https://www.spotcheckcc.com
Koncur checkout will wipe provider-config.json and rulesets/ from the workspace.
actions/checkout@v6 defaults to clean: true and checks out into $GITHUB_WORKSPACE (the workspace root) when no path: is specified. This executes git clean -ffdx && git reset --hard HEAD, removing untracked files. The previous steps write provider-config.json (line 124) and extract rulesets/ (line 117) into the workspace root, but the "Create MCP target config" step (lines 187–197) then references them via $(pwd)/provider-config.json and $(pwd)/rulesets — at that point $(pwd) is the koncur repo and those files are gone.
Fix either:
- Check koncur out into a subdirectory:
Suggested diff
- name: Checkout koncur
uses: actions/checkout@v6
with:
repository: konveyor/koncur
ref: ${{ inputs.ref }}
+ path: koncurThen build with go build -C koncur ... and keep $(pwd) pointing at the workspace root when writing target-mcp.yaml.
- Move
provider-config.jsonandrulesets/to${RUNNER_TEMP}before checkout and reference those absolute paths intarget-mcp.yaml.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@koncur-mcp/action.yml` around lines 171 - 197, The checkout step (name:
Checkout koncur, uses: actions/checkout@v6) is wiping provider-config.json and
rulesets created earlier because it defaults to cleaning the workspace; update
the workflow so those files survive: either check the koncur repo out into a
subdirectory (set a path for actions/checkout and then build with go build in
that directory, e.g., adjust the Build Koncur step to run the build from the
koncur subdir) and keep the Create MCP target config step referencing
$(pwd)/provider-config.json and $(pwd)/rulesets from the workspace root, or move
provider-config.json and rulesets into ${RUNNER_TEMP} before the checkout and
update the Create MCP target config step to use those absolute
${RUNNER_TEMP}/provider-config.json and ${RUNNER_TEMP}/rulesets paths (ensure
the Create MCP target config step uses those preserved locations when writing
target-mcp.yaml).
JonahSussman
changed the title
Signed-off-by: JonahSussman <sussmanjonah@gmail.com>
Signed-off-by: JonahSussman <sussmanjonah@gmail.com>
c67d98f to
e959e1d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
koncur-mcp/action.yml (1)
67-71: Pin theanalyzer-lspcheckout to a ref to avoid silent test drift.No
ref:is specified, so this resolves tokonveyor/analyzer-lsp's default branch at job time. Theexamples/tree is what feeds thetest-datavolume and is hardcoded intoprovider-config.jsonlocations (/analyzer-lsp/examples/java,.../golang,.../builtin). A rename/removal upstream will turn this action red for reasons unrelated to the PR under test. Consider pinning to a tag, commit SHA, or an input-controlled ref.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@koncur-mcp/action.yml` around lines 67 - 71, Pin the external checkout step named "Checkout analyzer-lsp for examples" that uses actions/checkout@v6 to a stable ref so the job doesn't silently drift; update that step to include a ref (tag, commit SHA, or an input-controlled ref) when checking out konveyor/analyzer-lsp so the examples used by provider-config.json paths (/analyzer-lsp/examples/java, /analyzer-lsp/examples/golang, /analyzer-lsp/examples/builtin) remain stable across runs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@koncur-mcp/action.yml`:
- Around line 98-102: The "Wait for providers to be ready" step currently uses a
blind sleep which is flaky; replace the fixed sleep with a poll that attempts to
connect to each provider port (or call each provider's health/readiness
endpoint) in a loop until success or a timeout, updating the bash `run:` block
in the step (named "Wait for providers to be ready") to repeatedly try TCP
connects (e.g., with netcat/nc, curl, or bash /dev/tcp) for the JDTLS/java
provider port(s) and other providers, sleep a short interval between attempts,
and fail the job if the timeout is reached so the workflow is deterministic.
Ensure the loop logs retries and the successful host:port so the step is
transparent.
---
Nitpick comments:
In `@koncur-mcp/action.yml`:
- Around line 67-71: Pin the external checkout step named "Checkout analyzer-lsp
for examples" that uses actions/checkout@v6 to a stable ref so the job doesn't
silently drift; update that step to include a ref (tag, commit SHA, or an
input-controlled ref) when checking out konveyor/analyzer-lsp so the examples
used by provider-config.json paths (/analyzer-lsp/examples/java,
/analyzer-lsp/examples/golang, /analyzer-lsp/examples/builtin) remain stable
across runs.
🪄 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: 23e8e09e-a278-4972-b747-088acece2463
📒 Files selected for processing (3)
.github/workflows/e2e-mcp-koncur.yaml.github/workflows/nightly-matrix-config.yamlkoncur-mcp/action.yml
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/nightly-matrix-config.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/e2e-mcp-koncur.yaml
| - name: Wait for providers to be ready | ||
| shell: bash | ||
| run: | | ||
| echo "Waiting for providers to start..." | ||
| sleep 10 |
There was a problem hiding this comment.
Flaky readiness: blind sleep 10 instead of a real health check.
A fixed 10-second sleep is unreliable on cold or busy runners — the JDTLS-based Java provider in particular can take much longer to warm up, and on a fast runner you are wasting seconds. Prefer polling the provider ports until they accept connections (or use a provider-native health/readiness check).
Suggested poll-based wait
- name: Wait for providers to be ready
shell: bash
run: |
- echo "Waiting for providers to start..."
- sleep 10
+ echo "Waiting for providers to start..."
+ for port in 14651 14653; do
+ for i in {1..60}; do
+ if (exec 3<>/dev/tcp/127.0.0.1/${port}) 2>/dev/null; then
+ exec 3<&- 3>&-
+ echo "provider on :${port} is up"
+ break
+ fi
+ sleep 1
+ if [[ $i -eq 60 ]]; then
+ echo "provider on :${port} did not become ready in time" >&2
+ podman logs java-provider || true
+ podman logs golang-provider || true
+ exit 1
+ fi
+ done
+ done🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@koncur-mcp/action.yml` around lines 98 - 102, The "Wait for providers to be
ready" step currently uses a blind sleep which is flaky; replace the fixed sleep
with a poll that attempts to connect to each provider port (or call each
provider's health/readiness endpoint) in a loop until success or a timeout,
updating the bash `run:` block in the step (named "Wait for providers to be
ready") to repeatedly try TCP connects (e.g., with netcat/nc, curl, or bash
/dev/tcp) for the JDTLS/java provider port(s) and other providers, sleep a short
interval between attempts, and fail the job if the timeout is reached so the
workflow is deterministic. Ensure the loop logs retries and the successful
host:port so the step is transparent.
1 participant
Summary
koncur-mcp/composite action for MCP e2e testing (starts providers, extracts analyzer-mcp binary, runs koncur)e2e-mcp-koncur.yamlreusable workflow (builds images then calls the action)konveyor-ecosystem/analyzer-mcpto nightly image build matrixTest plan
koncur-test.yamlworkflowSummary by CodeRabbit