feat(risk): make the risk taxonomy typed and fail closed on a bad one - #2213
Open
sang-neo03 wants to merge 2 commits into
Open
feat(risk): make the risk taxonomy typed and fail closed on a bad one#2213sang-neo03 wants to merge 2 commits into
sang-neo03 wants to merge 2 commits into
Conversation
|
Important Review skippedToo many files! This PR contains 423 files, which is 123 over the limit of 300. To get a review, reduce the PR to 300 files or fewer by splitting it into smaller PRs or changing its base branch. Usage-priced reviews support at most 300 files. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (423)
You can disable this status message by setting the 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
domain/base
sang-neo03
force-pushed
the
feat/typed-risk-level
branch
from
August 6, 2026 08:52
32d3481 to
528b201
Compare
Misspelling a shortcut's risk level used to be silent and destructive: `Risk: "high-risk-wrtie"` compiled, passed CI, and at runtime the confirmation gate compared against the literal "high-risk-write", missed, and ran the command. --yes was never even registered, so the user could not have confirmed if they had wanted to. 64 high-risk write commands sit behind that comparison. The taxonomy was declared four times (internal/core, internal/cmdutil, errs, extension/platform) and only the plugin SDK's copy was typed. Three layers now, because no single one is enough: - Type. internal/core.Risk is a defined type and the single definition; cmdutil and shortcuts/common re-export it, extension/platform keeps its own SDK-facing type but derives its constants from core and converts via Core/FromCore, and errs keeps the wire strings (it cannot import core without a cycle). A consistency test pins all three together. Shortcut.Risk, SetRisk/GetRisk, cmdmeta.Meta.Risk and the service command spec are typed; the 548 declarations under shortcuts/ and the SetRisk calls under cmd/ use the constants. Values crossing a string boundary — the generated service catalog, cobra annotations — go through core.ParseRisk instead of a bare conversion. Note what the type does not do: an untyped literal still converts, so `Risk: "high-risk-wrtie"` compiles. That is why the other two layers exist rather than being belt-and-braces. - CI. The quality gate rejects a manifest risk outside the closed enum — the manifest is exported from the live tree, so this sees every mounted command's real annotation — and a new risk-literal rule rejects a hand-written level in cmd/ and shortcuts/, at a zero baseline. The sheets flag-defs generator validates its JSON input the same way. - Runtime. cmdutil.EnforceRiskDeclaration refuses to run a command whose declared level is not in the taxonomy, and RequiresConfirmation treats an unrecognised level as the highest tier rather than as read. LARKSUITE_CLI_ALLOW_INVALID_RISK downgrades the refusal to "confirm first"; it cannot downgrade it to "run". --yes is registered for an invalid declaration too, so the downgrade path is usable. Tests cover the reproduction (a misspelled level must not execute, with or without --yes), the downgrade switch, the unchanged high-risk contract, the lower tiers staying ungated, ParseRisk's absent/valid/invalid split, and the cross-package consistency of the three declarations.
sang-neo03
force-pushed
the
feat/typed-risk-level
branch
from
August 6, 2026 08:54
528b201 to
83c052a
Compare
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@67ed241f5a45030282bc7471f07a3ee70d11c340🧩 Skill updatenpx skills add larksuite/cli#feat/typed-risk-level -y -g |
riskLiteralsInFile carried the whole ast.Inspect switch inline: two node shapes, each with its own guard chain, at a cyclomatic complexity of 12. Each shape is now its own function returning a riskLiteralUse, so the walker reads as "did this node use a literal, and where", and no function in the file is above 5. invalidRiskDeclaration has no caller outside cmdutil, so it is no longer part of the package's exported surface.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2213 +/- ##
==========================================
- Coverage 76.08% 76.08% -0.01%
==========================================
Files 983 986 +3
Lines 103429 103556 +127
==========================================
+ Hits 78692 78786 +94
- Misses 18752 18781 +29
- Partials 5985 5989 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A misspelled risk level silently disarmed the confirmation gate. Writing
Risk: "high-risk-wrtie"on any high-risk write shortcut compiled, passed CI, and at runtime the gate compared the declaration against the literal"high-risk-write", missed, and executed the command — with--yesnever registered, so the user could not have confirmed even if they had wanted to. 64 high-risk write commands sit behind that comparison, and the taxonomy was declared four times across the tree with only the plugin SDK's copy typed.Scope is the risk-level path only. Config persistence and API-response decoding are untouched.
Changes
Type — one definition, typed at the seams
internal/core.Riskbecomes a defined type withParseRisk/IsValid/Rank, and is the single definition.internal/cmdutilandshortcuts/commonre-export it;extension/platformkeeps its own SDK-facing type (public signature unchanged) but derives its constants from core and converts withCore()/FromCore();errskeeps the wire strings because it cannot import core without an import cycle. A consistency test pins the three value sets together.Shortcut.Risk,SetRisk/GetRisk,cmdmeta.Meta.Riskand the service command spec are typed. The 548Risk:declarations undershortcuts/and theSetRiskcalls undercmd/now use the constants.core.ParseRiskrather than a bare conversion. An out-of-taxonomy catalog value is kept as-is instead of being normalised to a default, so the runtime gate can refuse it.CI — the layer that actually catches a typo
riskoutside the closed enum. The manifest is exported from the live command tree, so this sees every mounted command's real annotation, including commands whose risk arrived as a string.risk-literalrule rejects a hand-written level incmd/andshortcuts/, scanned in full at a zero baseline.Runtime — fail closed
cmdutil.EnforceRiskDeclarationrefuses to run a command whose declared level is not in the taxonomy;RequiresConfirmationtreats an unrecognised level as the highest tier rather than asread.LARKSUITE_CLI_ALLOW_INVALID_RISKdowngrades the refusal to "confirm first". It cannot downgrade it to "run".--yesis registered for an invalid declaration too, so the downgrade path is usable.internal/invalid_risk_declaration.Test Plan
go build ./...,go vet ./...,gofmtcleango test ./shortcuts/... -count=1passesgo test ./internal/... ./errs/... ./extension/... ./cmd/... -count=1— 15 failures, all pre-existing: the same set fails on a cleanorigin/main(local registry is not generated, e.g.service "im" not found in registry). Verified by stashing this branch and re-running the identical command.s.Risk == "high-risk-write"gate makesTestRiskGateRefusesMisspelledDeclarationfail with the fixture command executing.--yes; the downgrade switch still requires confirmation and then runs; the unchanged high-risk contract; lower tiers stay ungated;ParseRisk's absent/valid/invalid split; cross-package taxonomy consistency; manifest enum validation; the risk-literal rule and the tree's zero baseline.Notes for review
Two choices worth a second opinion:
LARKSUITE_CLI_ALLOW_INVALID_RISKis my pick for the downgrade switch; happy to rename.invalid_risk_declarationunderinternal. Adding a subtype is additive to the error contract, but say the word if you would rather reuse an existing one.Worth knowing for anyone reviewing the type change: a defined string type does not make a misspelled literal a compile error — Go converts untyped string constants implicitly, so
Risk: "high-risk-wrtie"still builds. The type stops astringvariable from flowing in and gives editors the candidate list; the actual typo defence is the CI rules plus the runtime gate. Making it a build error would requireRiskto become a struct with an unexported field, which changes JSON/YAML handling and the plugin SDK signature.Related Issues