Skip to content

fix(apps): forbid online env var delete; surface release note - #2844

Open
chengshihao-miaoda wants to merge 1 commit into
mainfrom
fix/apps-env-online-delete-and-release-note
Open

chengshihao-miaoda wants to merge 1 commit into
mainfrom
fix/apps-env-online-delete-and-release-note

Conversation

@chengshihao-miaoda

@chengshihao-miaoda chengshihao-miaoda commented Oct 22, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

  • CLI could delete online env vars directly, bypassing the release-based change control.
  • Both +env-set and +env-delete returned a 200 OK with no indication that the online runtime keeps serving the pre-release snapshot until +release-create runs — users mistook the immediate success for live effect.

Changes

  • shortcuts/apps/apps_env.go
    • Reject --environment online for +env-delete at Validate (covers both --yes execute and --dry-run preview); the error message points users at the dev + +release-create workflow or the developer console.
    • Add an envVarReleaseNote constant and include a note field in the success envelopes of +env-set and +env-delete, so JSON and pretty formats both carry the "release required to go live" reminder.
  • shortcuts/apps/apps_env_test.go
    • BuildsDeleteBodyWithKeys now uses --environment dev and asserts the release note.
    • New RejectsOnlineEnvironment covers both --yes and --dry-run rejection paths with typed *errs.ValidationError + SubtypeFailedPrecondition.
    • Removed OnlineDryRunDoesNotRequireYes (subsumed by RejectsOnlineEnvironment).
  • skills/lark-apps/references/lark-apps-env.md: documents data.note on success and the online-delete constraint; drops the stale --environment online delete example.

Test Plan

  • CI make test passes (local Go toolchain is 1.18/1.22; go.mod requires 1.23).
  • lark-cli apps +env-delete --environment online --key FOO --yes returns a validation error with dev + release-create guidance.
  • lark-cli apps +env-delete --environment online --key FOO --dry-run returns the same validation error (previously previewed the request).
  • lark-cli apps +env-set --environment online --key FOO --value <v> --yes success JSON contains data.note referencing +release-create.
  • lark-cli apps +env-delete --environment dev --key FOO --yes success JSON contains data.note.

Related Issues

None. Observed while cleaning up lark-apps skill guidance around env-var mutations.

Summary by CodeRabbit

  • Behavior Changes
    • Environment-variable updates now clarify that changes take effect online only after a release.
    • Deleting environment variables directly in online is no longer supported. Delete the key in dev and release, or remove it through the developer console.
  • Documentation
    • Updated environment-variable guidance to explain the release requirement and supported deletion options.

…d note

CLI could delete online env vars directly, bypassing the release-based
change control, and both set/delete success looked instantaneous even
though the online runtime keeps serving the pre-release snapshot.

- `+env-delete --environment online` is now rejected at Validate (fails
  for --yes execution and --dry-run alike); users must delete in dev
  then run `+release-create`, or remove from the developer console.
- `+env-set` and `+env-delete` success envelopes include a `note` field
  pointing at `+release-create`, so JSON and pretty consumers see the
  same reminder that online propagation requires a release.
- Skill reference doc updated to document both constraints.
@chengshihao-miaoda chengshihao-miaoda added the size/S Low-risk docs, CI, test, or chore only changes label Oct 22, 2026
@CLAassistant

CLAassistant commented Oct 22, 2026 •

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions Bot added size/M Single-domain feat or fix with limited business impact and removed size/S Low-risk docs, CI, test, or chore only changes labels Oct 22, 2026
@coderabbitai

coderabbitai Bot commented Oct 22, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: abd1d5c4-b342-40f2-ba0b-7ad67718e956
📥 Commits

Reviewing files that changed from the base of the PR and between 017b19a and 02ebcca.

📒 Files selected for processing: 3
  • shortcuts/apps/apps_env.go
  • shortcuts/apps/apps_env_test.go
  • skills/lark-apps/references/lark-apps-env.md

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

📝 Walkthrough

Set and delete environment-variable responses now state that online changes take effect after a release. Deleting variables from the online environment is rejected during validation, with alternative cleanup instructions.

Changes: Environment variable mutations

Layer / File(s) Summary
Mutation response release notes
shortcuts/apps/apps_env.go, shortcuts/apps/apps_env_test.go, skills/lark-apps/references/lark-apps-env.md
Set and delete responses include a note that online changes take effect after a release. Tests and reference documentation reflect the release note and use dev for deletion.
Reject online deletion
shortcuts/apps/apps_env.go, shortcuts/apps/apps_env_test.go, skills/lark-apps/references/lark-apps-env.md
Validation rejects deletion from online with a failed-precondition error. Tests cover normal execution and dry-run. Reference documentation describes the restriction and alternative cleanup steps.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: qingniaotonghua

Merge Risk: ⚪ Minimal · up to 02ebc

Online deletion is rejected before a request is made, and successful mutations provide release guidance. No actionable merge-blocking risk remains after normal checks.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 2 files. (1 skipped: 1 … Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Title check Passed The title clearly identifies the two main changes: blocking online environment-variable deletion and exposing the release requirement.
Description check Passed The description includes all required sections and clearly explains the motivation, implementation changes, tests, and related issues. The test plan remains unchecked because the required Go toolchain…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Description check

Explanation

The description includes all required sections and clearly explains the motivation, implementation changes, tests, and related issues. The test plan remains unchecked because the required Go toolchain is unavailable locally, but this does not make the description incomplete.

Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 2 files. (1 skipped: 1 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

✨ Finishing Touches: 💡 2
📝 Generate docstrings: 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
🛠️ Fix failing CI checks: 💡
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

Copy link
Copy Markdown

🚀 PR Preview Install Guide

🧰 CLI update

npm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@02ebcca5319802664c41c2e6b7b04cad64ac4460

🧩 Skill update

npx skills add larksuite/cli#fix/apps-env-online-delete-and-release-note -y -g

@codecov

codecov Bot commented Oct 22, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.39%. Comparing base (017b19a) to head (02ebcca).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2844      +/-   ##
==========================================
- Coverage   76.40%   76.39%   -0.01%     
==========================================
  Files        1151     1151              
  Lines      133041   133046       +5     
==========================================
- Hits       101646   101643       -3     
- Misses      23150    23158       +8     
  Partials     8245     8245              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

This branch has not been deployed

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

Labels

size/M Single-domain feat or fix with limited business impact

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants