ci: add L4 plugin-integration and sidecar-integration CI jobs#1840
Merged
Conversation
The two new L4 jobs (plugin-integration, sidecar-integration) still run on every PR and report status in the results summary, but their failure no longer blocks the merge gate during the initial soak. Once they prove stable, add them back to the results FAILED loop to make them required.
Merge the byte-identical assertInstallEnvelope into a single neutral assertReasonCodeEnvelope shared by both policy-denial and install-time reason_code tests. Reword the observer-panic-isolation comment to describe the baseline-relative assertion instead of a hardcoded exit 0, and the auditPlugin comment to say it is based on (not mirrors) the shipped example.
Extract the 230-line TestSidecarHMACRoundTrip into a short top-level flow (start stubs -> build+run fork -> assert) backed by focused helpers: the in-test sidecar handler splits into verifyProxyRequest (steps 0-4) and forwardWithInjectedToken; the two httptest stand-ins become mockUpstream / inTestSidecar types sharing a requestSink; build/run/assert move into buildAuthsidecarFork, runFork, and the two assert* helpers. Behavior unchanged.
|
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:
📝 WalkthroughWalkthroughAdds CI wiring for plugin and sidecar integration jobs, a shared plugin fork-testing harness with behavioral suites, and an authsidecar HMAC roundtrip test with a demo handler input update. ChangesPlugin and sidecar integration coverage
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
github-actions
Bot
added
the
size/M
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@064b3cb98880d2bb5f60b943185a2b201c9d3dbe🧩 Skill updatenpx skills add larksuite/cli#ci/plugin-integration -y -g |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
.github/workflows/ci.yml (1)
54-54: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueSet
persist-credentials: falseon the new checkouts.Neither new job pushes or needs the credential after checkout, so persisting the token in
.git/configis unnecessary attack surface. Static analysis (zizmorartipacked) flags both steps.🔒 Proposed hardening (apply to both jobs)
- uses: actions/checkout@93cb6efe18208431cddfb8368fd83d5badbf9bfd # v5 + with: + persist-credentials: falseAlso applies to: 67-67
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml at line 54, The checkout steps used by both CI jobs need to disable credential persistence. Update the actions/checkout usage in the workflow so the new checkouts set persist-credentials to false, since these jobs do not push or need the token after checkout. Apply the change to each affected checkout step in the workflow to remove the token from .git/config and match the hardening guidance.Source: Linters/SAST tools
tests/plugin_e2e/restrict_test.go (1)
49-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
assertReasonCodeEnvelopeinTestReadonlyDenialto avoid duplicated assertion logic.Lines 52-66 manually check the same envelope shape (exit 2, valid JSON,
error.type=="validation",error.subtype=="failed_precondition", hint containsreason_code <X>) thatassertReasonCodeEnvelope(lines 168-185) already encapsulates. If the envelope contract changes, both sites need updating.♻️ Proposed refactor
for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { res := run(t, bin, tc.args...) - if res.exit != 2 { - t.Fatalf("exit=%d stdout=%s stderr=%s", res.exit, res.stdout, res.stderr) - } - if !gjson.Valid(res.stderr) { - t.Fatalf("stderr not JSON: %s", res.stderr) - } - if got := gjson.Get(res.stderr, "error.type").String(); got != "validation" { - t.Errorf("error.type=%q want validation", got) - } - if got := gjson.Get(res.stderr, "error.subtype").String(); got != "failed_precondition" { - t.Errorf("error.subtype=%q want failed_precondition", got) - } - if hint := gjson.Get(res.stderr, "error.hint").String(); !strings.Contains(hint, "reason_code "+tc.reasonCode) { - t.Errorf("hint=%q want to contain reason_code %s", hint, tc.reasonCode) - } + assertReasonCodeEnvelope(t, res, tc.reasonCode) }) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/plugin_e2e/restrict_test.go` around lines 49 - 67, `TestReadonlyDenial` is duplicating the reason-code envelope checks that already exist in `assertReasonCodeEnvelope`; replace the inline assertions in the test loop with a call to that helper using the `run` result and the expected `reasonCode`. Keep the test focused on the readonly-denial cases and rely on `assertReasonCodeEnvelope` for verifying exit code, JSON shape, `error.type`, `error.subtype`, and the hint content.tests/plugin_e2e/harness.go (1)
54-58: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAvoid shell invocation in
gitArchiveto eliminate command-injection pattern.
dstis internally constructed (os.MkdirTemp+filepath.Join) andshellQuoteproperly escapes it, so this is not exploitable. However, thebash -c+ string-concatenation pattern is flagged by static analysis and sets a poor precedent. UseStdoutPipeto pipegit archivedirectly intotarwithout a shell:♻️ Proposed refactor
func gitArchive(root, dst string) error { - c := exec.Command("bash", "-c", "git archive HEAD | tar -x -C "+shellQuote(dst)) - c.Dir = root - return runCmd(c) + gitCmd := exec.Command("git", "archive", "HEAD") + gitCmd.Dir = root + stdout, err := gitCmd.StdoutPipe() + if err != nil { + return err + } + tarCmd := exec.Command("tar", "-x", "-C", dst) + tarCmd.Stdin = stdout + if err := gitCmd.Start(); err != nil { + return err + } + if err := tarCmd.Start(); err != nil { + gitCmd.Process.Kill() + return err + } + if err := tarCmd.Wait(); err != nil { + gitCmd.Process.Kill() + return err + } + return gitCmd.Wait() }Note:
shellQuote,runCmd, andcmdErrorbecome dead code in this file after the refactor — consider removing them or confirming they're used elsewhere.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/plugin_e2e/harness.go` around lines 54 - 58, Replace the shell-based implementation in gitArchive with a direct process pipeline so it no longer uses bash -c or string concatenation. Launch git archive with exec.Command, connect its StdoutPipe to a tar process that extracts into dst, and run both commands with proper error handling. After the refactor, check whether shellQuote, runCmd, and cmdError are still referenced in harness.go and remove them if they are now unused.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.github/workflows/ci.yml:
- Line 54: The checkout steps used by both CI jobs need to disable credential
persistence. Update the actions/checkout usage in the workflow so the new
checkouts set persist-credentials to false, since these jobs do not push or need
the token after checkout. Apply the change to each affected checkout step in the
workflow to remove the token from .git/config and match the hardening guidance.
In `@tests/plugin_e2e/harness.go`:
- Around line 54-58: Replace the shell-based implementation in gitArchive with a
direct process pipeline so it no longer uses bash -c or string concatenation.
Launch git archive with exec.Command, connect its StdoutPipe to a tar process
that extracts into dst, and run both commands with proper error handling. After
the refactor, check whether shellQuote, runCmd, and cmdError are still
referenced in harness.go and remove them if they are now unused.
In `@tests/plugin_e2e/restrict_test.go`:
- Around line 49-67: `TestReadonlyDenial` is duplicating the reason-code
envelope checks that already exist in `assertReasonCodeEnvelope`; replace the
inline assertions in the test loop with a call to that helper using the `run`
result and the expected `reasonCode`. Keep the test focused on the
readonly-denial cases and rely on `assertReasonCodeEnvelope` for verifying exit
code, JSON shape, `error.type`, `error.subtype`, and the hint content.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b4ac0c7c-c256-45a0-bff1-7003a8f8367d
📒 Files selected for processing (11)
.github/workflows/ci.ymlMakefilesidecar/server-demo/handler_test.gotests/plugin_e2e/degrade_subsystem_test.gotests/plugin_e2e/diagnostics_test.gotests/plugin_e2e/harness.gotests/plugin_e2e/install_test.gotests/plugin_e2e/observe_wrap_test.gotests/plugin_e2e/restrict_test.gotests/plugin_e2e/smoke_test.gotests/sidecar_e2e/roundtrip_test.go
…ma catalog Two plugin_e2e tests resolved differently on a clean CI runner than on a developer box because they used run(), which inherits the host's ~/.lark-cli metadata cache and network. On CI (cold cache, offline) they failed; locally they passed. - Replace TestSubsystemTransportAbort with TestRuntimeCatalogResolvesSchema. The transport round-trip it targeted is unreachable in a bare-module fork offline (credential resolution fails first); the transport path is covered end-to-end by the sidecar suite and the credential-block test. The new test seeds an obviously-synthetic service into a bare-module fork's on-disk cache and asserts `schema` resolves it from the runtime catalog -- the primary fix for #1764 (module builds now consult the runtime catalog instead of returning "Unknown service"). It complements the existing cold-cache degrade test, pinning both branches, and runs fully offline. - Drop the readonly parent-group sub-case: mixed_children_policy needs a metadata-enumerated child tree the bare-module fork does not have, so offline it collapses to domain_not_allowed; that reason_code is covered by cmdpolicy unit tests. - Fix errorlint in run() (errors.As), and stream `git archive` into `tar` through an explicit pipe instead of a shell in gitArchive. - Set persist-credentials: false on the two new job checkouts and reuse assertReasonCodeEnvelope in TestReadonlyDenial.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/plugin_e2e/harness.go`:
- Around line 67-69: The shared stderr buffer in the test harness causes a data
race because `archive.Stderr` and `extract.Stderr` both write to the same
`strings.Builder`. Update `harness.go` so `archive.Run()` and `extract.Run()`
each use their own stderr buffer or a concurrency-safe writer, and then combine
or inspect the results after both commands finish. Use the `archive`, `extract`,
and `errBuf` setup in the harness to locate and split the shared writer.
🪄 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: b748ce86-3e34-41e4-bf09-43b4e97df1a6
📒 Files selected for processing (4)
.github/workflows/ci.ymltests/plugin_e2e/degrade_subsystem_test.gotests/plugin_e2e/harness.gotests/plugin_e2e/restrict_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/ci.yml
- tests/plugin_e2e/restrict_test.go
The previous run() inherited the host environment, so a fork's startup metadata behavior depended on the developer machine's ~/.lark-cli cache and live network. Tests passed locally but flaked on CI (e.g. TestSubsystemCredentialBlock reaching a metadata fetch instead of the credential block). Move the isolated, offline environment (fresh empty LARKSUITE_CLI_CONFIG_DIR + LARKSUITE_CLI_REMOTE_META=off) into run() itself and fold the per-file runEnv/runIsolated helpers into the shared runWithEnv, so every test reproduces the bare-module customer state on any machine, including CI. Tests needing runtime metadata still seed it explicitly via runWithSeededCatalog.
archive.Stderr and extract.Stderr shared one strings.Builder; os/exec spawns a stderr-copy goroutine per command, so git archive and tar (which run concurrently) could write the builder simultaneously. strings.Builder is not concurrency-safe, so go test -race would flag it on any run where both processes emit stderr. Give each process its own buffer and report the relevant one on failure. Flagged by CodeRabbit on PR #1840.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1840 +/- ##
==========================================
+ Coverage 74.40% 74.66% +0.26%
==========================================
Files 860 878 +18
Lines 89215 91764 +2549
==========================================
+ Hits 66384 68520 +2136
- Misses 17701 17930 +229
- Partials 5130 5314 +184 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Review feedback on the sidecar round-trip test surfaced two gaps: 1. False-green window: docs +fetch --as user routes TWO requests through the proxy — the mandatory /open-apis/authen/v1/user_info identity probe (enrichUserInfo verifies the sentinel UAT) and then the docs call. The sinks kept only the last request, so assertions could pass against the auxiliary probe. Capture every request per stub, track the HMAC verify outcome per request, and scope all assertions to the docs request (POST /open-apis/docs_ai/v1/documents/<doc>/fetch), additionally checking method and X-Lark-Proxy-Target=open.feishu.cn. 2. Real network egress: runFork did not set LARKSUITE_CLI_REMOTE_META=off, so the fork's startup metadata refresh hit the real open.feishu.cn/api/tools/open/api_definition, contradicting the offline/secret-free contract. Set it; the run is now fully offline. runFork now also returns the fork's outcome and the test asserts exit 0 with an ok:true envelope, so the round trip must be a genuine end-to-end success rather than bytes that happened to flow. The mock upstream answers the identity probe with a parseable user_info body to keep the user resolution clean.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/sidecar_e2e/roundtrip_test.go (1)
597-617: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winAssert that proxy protocol headers never reach upstream.
The test validates token injection but would still pass if forwarding leaked HMAC signatures, timestamps, target, identity, or body-digest headers. Explicitly assert every
sidecar.HeaderProxy*field andHeaderBodySHA256is absent upstream.Proposed assertion
if gotAuth != wantAuth { t.Fatalf("mock upstream Authorization = %q, want %q", gotAuth, wantAuth) } + for _, h := range []string{ + sidecar.HeaderProxyVersion, + sidecar.HeaderProxyTarget, + sidecar.HeaderProxyIdentity, + sidecar.HeaderProxySignature, + sidecar.HeaderProxyTimestamp, + sidecar.HeaderProxyAuthHeader, + sidecar.HeaderBodySHA256, + } { + if value := got.headers.Get(h); value != "" { + t.Fatalf("mock upstream received proxy header %s: %q", h, value) + } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/sidecar_e2e/roundtrip_test.go` around lines 597 - 617, Extend assertInjectedTokenReachedUpstream to verify that every proxy protocol header represented by sidecar.HeaderProxy* and sidecar.HeaderBodySHA256 is absent from got.headers, failing with the header name and received value when any is present.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/sidecar_e2e/roundtrip_test.go`:
- Around line 82-90: Replace the strings.Contains-based request selectors
associated with docsReqMarker, including the additional selectors around the
referenced request-handling blocks, with exact path equality checks against
docsReqMarker. Ensure only the expected "/documents/<testDocToken>/fetch"
endpoint matches, rejecting incorrect prefixes, versions, document IDs, or
suffixes.
---
Outside diff comments:
In `@tests/sidecar_e2e/roundtrip_test.go`:
- Around line 597-617: Extend assertInjectedTokenReachedUpstream to verify that
every proxy protocol header represented by sidecar.HeaderProxy* and
sidecar.HeaderBodySHA256 is absent from got.headers, failing with the header
name and received value when any is present.
🪄 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: 5f6a0113-874f-442e-b2d3-d1d73140261f
📒 Files selected for processing (1)
tests/sidecar_e2e/roundtrip_test.go
…ff upstream Two CodeRabbit findings on the previous commit: 1. The docs-request selectors used strings.Contains, so a wrong API prefix or version (e.g. docs_ai/v2) could still match while the mock answers any path with a generic success. Select by the full exact path /open-apis/docs_ai/v1/documents/<doc>/fetch instead. 2. The upstream assertion proved token injection but would not notice the forward leaking sidecar wire-protocol headers. Assert every X-Lark-Proxy-* header and X-Lark-Body-Sha256 is absent from the forwarded docs request.
The coverage job's filter only excluded tests/cli_e2e, so the new tests/plugin_e2e (no build tag) survived go list ./... and ran a second time inside coverage — and since coverage sits in the results blocking loop, a plugin_e2e failure would have blocked merges, defeating the observe-only soft launch this PR establishes for plugin-integration. Exclude the whole tests/ subtree: everything under it is an L3/L4 suite with a dedicated job, the same rationale cli_e2e was excluded under. (sidecar_e2e is tag-gated and never entered go list; verified tests/plugin_e2e was the only survivor.)
- plugin_e2e/sidecar_e2e: strip the host's LARKSUITE_CLI_* namespace before composing fork environments. Appending overrides to a raw os.Environ() only isolated the variables we set; a developer machine exporting e.g. LARKSUITE_CLI_AUTH_PROXY leaked it into the fork, contradicting the determinism the harness comments claim. Verified by running both suites with hostile LARKSUITE_CLI_* exports. - plugin_e2e: guard builtForks with a mutex — the map was safe only under the unstated convention that no test uses t.Parallel(). - plugin_e2e: name the 60s timeout in run()'s failure message instead of surfacing a bare exit=-1. - sidecar_e2e: replace the overstated "mirrors steps 0-8 exactly" comment with a per-step coverage accounting (mirrored / replaced by assertion / not covered), renumber the in-test handler's step comments to match server-demo's, and mirror server-demo's step 1 timestamp presence check (server-demo enforces presence only; no freshness window exists there either). - Makefile: document that integration-test builds ~20 plugin_e2e forks (~1 min warm, GOPROXY downloads when cold).
The results-job comment promised to add plugin-integration and sidecar-integration back into the blocking loop "once they have proven stable" with nothing holding that promise. Point it at issue #1894, which carries the graduation criteria (4 consecutive weeks with zero false positives).
liangshuo-1
approved these changes
Jul 16, 2026
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
2 participants
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
Adds two offline, real-binary (L4) integration CI jobs that build a customer-style fork of the CLI and assert the observable command envelope:
extension/platformcontract and checks the denial / observe / wrap / install / degrade behavior a real binary produces. This path only exists in a built binary, so unit tests cannot cover it.authsidecar-tagged binary and proves the transport interceptor signs outbound requests and that the resolved token reaches the upstream, using an in-test sidecar and a mock upstream.The plugin job is motivated by #1764: a plugin integrated through the Go-module path was built without embedded API metadata and surfaced a confusing schema error. The new job exercises that exact real-binary path.
Changes
tests/plugin_e2e/— new fork-build harness (harness.go) plus tests: registration smoke,Restrictdenials (readonly / identity / denylist / multi-rule), install-time reason codes on broken forks, observe/wrap hooks, and offline degrade when a fork is built without embedded metadata.tests/sidecar_e2e/roundtrip_test.go—authsidecarbuild-tag HMAC round-trip test with an in-test sidecar and mock upstream.Makefile— add asidecar-testtarget that builds and runs theauthsidecar-tagged suite.sidecar/server-demo/handler_test.go— repair compilation of the demo server test..github/workflows/ci.yml— add theplugin-integrationandsidecar-integrationjobs (run onpull_request, offline, no secrets, read-onlyGITHUB_TOKEN) and wire them into the results gate as observe-only (non-blocking).Test Plan
go test ./tests/plugin_e2e/...— builds customer forks and asserts the denial / observe / wrap / install / degrade envelopes; verified green locally.make sidecar-test(equivalentlygo test -tags authsidecar ./tests/sidecar_e2e/...) — HMAC round-trip; verified green locally.TestDegradeStubMetadataSchemafail with the pre-fix error, confirming the job catches the regression.Related Issues
Summary by CodeRabbit
Tests
Chores
sidecar-testtarget for running sidecar-tagged checks.