feat: add configurable content-safety scanning#606
Merged
MaxHuang22 merged 15 commits intolarksuite:mainfrom Apr 23, 2026
Merged
feat: add configurable content-safety scanning#606MaxHuang22 merged 15 commits intolarksuite:mainfrom
MaxHuang22 merged 15 commits intolarksuite:mainfrom
Conversation
📝 WalkthroughWalkthroughAdds a content-safety subsystem: provider API and registry, regex-based provider with config/normalization/scanner, integration into response/output flows (scan→warn/block), env-driven mode (off/warn/block), and tests. Response handlers now pass command path into scanning. Changes
Sequence DiagramsequenceDiagram
participant Client as API/Service
participant Resp as Response Handler
participant Output as Output Layer
participant Scanner as Scan Orchestrator
participant Provider as Content Safety Provider
participant Config as Config Loader
Client->>Resp: HandleResponse(resp, opts{CommandPath})
Resp->>Resp: parse result & check business error
Resp->>Scanner: ScanForSafety(CommandPath, result, errOut)
alt Provider registered
Scanner->>Provider: Scan(ctx, ScanRequest{Path, Data})
Provider->>Config: LoadConfig()/EnsureDefaultConfig()
Config-->>Provider: Config (allowlist + rules)
Provider->>Provider: normalize data & run scanner.walk()
Provider-->>Scanner: return Alert or nil
else No provider
Scanner-->>Resp: return empty ScanResult
end
alt Mode == block and blocked
Scanner-->>Resp: ScanResult{Blocked:true, BlockErr}
Resp-->>Client: return BlockErr (halt)
else Mode == warn and alert
Scanner-->>Resp: ScanResult{Alert:alert}
Resp->>Output: inject "_content_safety_alert" into Envelope and/or write warning to stderr
Resp-->>Client: output with alert/warning
else Mode == off or no alert
Scanner-->>Resp: ScanResult{nil}
Resp-->>Client: output unchanged
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
github-actions
Bot
added
the
size/L
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@5307f7158f98e3ff0522fed896e42d3f6befdba9🧩 Skill updatenpx skills add MaxHuang22/cli#feat/safety-prompt-injection -y -g |
![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
🧹 Nitpick comments (5)
internal/security/contentsafety/normalize_test.go (1)
62-68: Minor: assertion could be tighter.Comparing
any-wrapped channels with!=works because channels are comparable, but asserting via a type-asserted identity check (e.g.,gotCh, ok := got.(chan int); !ok || gotCh != ch) expresses intent more clearly and avoids relying on interface-value equality for future non-comparable types. Optional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/security/contentsafety/normalize_test.go` around lines 62 - 68, Update the test TestNormalize_UnmarshalableValue to assert the returned value's concrete type and identity instead of relying on interface equality: after calling normalize(ch) assign with a type assertion (gotCh, ok := got.(chan int)) and fail the test if !ok or gotCh != ch. This clarifies intent and avoids depending on interface-value comparability for the normalize function's result.internal/output/envelope.go (1)
8-14: Consider typingContentSafetyAlertas*extcs.Alert.Using
interface{}withomitemptycorrectly omits a nil interface value, but if a caller ever assigns a typed-nil pointer (e.g.,var a *extcs.Alert; env.ContentSafetyAlert = a),omitemptywill not drop it and the envelope would emit"_content_safety_alert": null. The current caller inshortcuts/common/runner.goguards withif scanResult.Alert != nil, so this is fine today, but a concrete pointer type would make the contract self-enforcing. Optional; only worth doing if importingextension/contentsafetyfrom theoutputpackage is acceptable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/output/envelope.go` around lines 8 - 14, Change the ContentSafetyAlert field from interface{} to a concrete pointer type to make the contract self-enforcing: replace ContentSafetyAlert interface{} with ContentSafetyAlert *extcs.Alert and keep the json tag `"_content_safety_alert,omitempty"`; import the extension/contentsafety package (alias extcs) in the file and update any code referencing Envelope.ContentSafetyAlert (e.g., assignments in shortcuts/common/runner.go) to use the *extcs.Alert type so nil pointer values are omitted correctly by omitempty and the code compiles.internal/output/emit_test.go (1)
42-44: Restore the previous provider instead of forcingnil.These tests mutate a package-global registry. Registering
nilin cleanup, or not restoring afterTestScanForSafety_NoProvider, can leak state across tests and remove any provider that was already registered.Proposed test helper
+func registerContentSafetyProvider(t *testing.T, p extcs.Provider) { + t.Helper() + prev := extcs.GetProvider() + extcs.Register(p) + t.Cleanup(func() { + extcs.Register(prev) + }) +} + func TestScanForSafety_ModeWarn_WithAlert(t *testing.T) { t.Setenv("LARKSUITE_CLI_CONTENT_SAFETY_MODE", "warn") alert := &extcs.Alert{Provider: "mock", MatchedRules: []string{"r1"}} mp := &mockProvider{name: "mock", alert: alert} - // Register mock provider (save and restore) - extcs.Register(mp) - defer extcs.Register(nil) + registerContentSafetyProvider(t, mp)func TestScanForSafety_NoProvider(t *testing.T) { t.Setenv("LARKSUITE_CLI_CONTENT_SAFETY_MODE", "warn") - extcs.Register(nil) + registerContentSafetyProvider(t, nil)mp := &mockProvider{name: "mock", err: errors.New("scan broke")} - extcs.Register(mp) - defer extcs.Register(nil) + registerContentSafetyProvider(t, mp)Also applies to: 83-98
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/output/emit_test.go` around lines 42 - 44, The tests currently call extcs.Register(mp) then defer extcs.Register(nil), which overwrites any existing global provider and can leak state across tests; change to capture the previous provider before registering (e.g., prev := extcs.Register(...) or call extcs.GetProvider()/prev := extcs.Register? depending on API) and in the defer restore that saved provider with extcs.Register(prev) so the original provider is preserved; apply the same pattern to the other test block around lines 83-98 and ensure TestScanForSafety_NoProvider also restores the prior provider after it runs.shortcuts/common/runner_contentsafety_test.go (2)
36-43: Use the standard test factory helper.This manually constructs
cmdutil.Factory; prefercmdutil.TestFactory(t, config)so shortcut tests use the repo’s standard in-memory config wiring.Proposed adjustment
+ config := &core.CliConfig{Brand: core.BrandFeishu} + factory := cmdutil.TestFactory(t, config) + factory.IOStreams = &cmdutil.IOStreams{Out: stdout, ErrOut: stderr} rctx := &RuntimeContext{ ctx: context.Background(), - Config: &core.CliConfig{Brand: core.BrandFeishu}, + Config: config, Cmd: cmd, resolvedAs: core.AsBot, - Factory: &cmdutil.Factory{ - IOStreams: &cmdutil.IOStreams{Out: stdout, ErrOut: stderr}, - }, + Factory: factory, }As per coding guidelines,
**/*_test.go: Usecmdutil.TestFactory(t, config)for test factories.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/common/runner_contentsafety_test.go` around lines 36 - 43, Replace the manually constructed cmdutil.Factory in the test’s RuntimeContext with the standard test helper: call cmdutil.TestFactory(t, cfg) (where cfg is the &core.CliConfig{Brand: core.BrandFeishu} used now) and assign its result to RuntimeContext.Factory; ensure the test passes the testing.T instance (t) into TestFactory and use the returned Factory instead of creating &cmdutil.Factory{IOStreams: ...} so the test uses the repo’s in-memory config wiring.
51-53: Restore the previous provider in cleanup.These tests replace a package-global provider. Cleanup should restore the provider that was present before the test instead of always clearing it.
Proposed helper
+func registerCSTestProvider(t *testing.T, p extcs.Provider) { + t.Helper() + prev := extcs.GetProvider() + extcs.Register(p) + t.Cleanup(func() { + extcs.Register(prev) + }) +} + func TestOut_ContentSafetyWarn(t *testing.T) { t.Setenv("LARKSUITE_CLI_CONTENT_SAFETY_MODE", "warn") alert := &extcs.Alert{Provider: "test", MatchedRules: []string{"r1"}} - extcs.Register(&csTestProvider{alert: alert}) - defer extcs.Register(nil) + registerCSTestProvider(t, &csTestProvider{alert: alert})alert := &extcs.Alert{Provider: "test", MatchedRules: []string{"r1"}} - extcs.Register(&csTestProvider{alert: alert}) - defer extcs.Register(nil) + registerCSTestProvider(t, &csTestProvider{alert: alert})Also applies to: 70-72
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/common/runner_contentsafety_test.go` around lines 51 - 53, Save the existing package-global provider before calling extcs.Register and restore that saved value in the deferred cleanup instead of always registering nil; e.g. capture old := extcs.Register(&csTestProvider{alert: alert}) (or call a getter if Register doesn't return the old provider), then use defer extcs.Register(old) to restore the previous provider; update both places that register csTestProvider (the block that creates alert := &extcs.Alert{...} and the similar block at lines 70-72) to follow this pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/client/response.go`:
- Around line 64-75: The warn-mode content-safety alert is being dropped when
opts.OutputPath != "" because the code returns via saveAndPrint before embedding
or emitting the alert, and it is also dropped for non-object JSON roots when
casting result to map[string]interface{} fails; update the logic in the response
handling (around scanResult, saveAndPrint, scanResult.Alert and
_content_safety_alert) to always surface warn-mode alerts to stderr: if
scanResult.Alert != nil, first emit a-warning message (to opts.ErrOut / stderr)
describing the alert, then attempt to embed it into the stdout JSON by setting
m["_content_safety_alert"] when result is a map[string]interface{} (but do not
fail if the cast fails), and only after emitting the stderr warning call
saveAndPrint when opts.OutputPath != ""; ensure progress/warnings go to stderr
and JSON envelope to stdout per guidelines.
In `@internal/cmdutil/factory_default.go`:
- Around line 25-26: Import block in internal/cmdutil/factory_default.go has
incorrect ordering and comment spacing; move the blank imports (e.g., the
side-effect imports "github.com/larksuite/cli/internal/vfs/localfileio" and
"github.com/larksuite/cli/internal/security/contentsafety") so they are
alphabetically sorted and placed before the regular imports (like the util
import), adjust trailing comments to have a single space after code for proper
alignment, and then run gofmt -w internal/cmdutil/factory_default.go to apply
and normalize the formatting.
In `@internal/security/contentsafety/config.go`:
- Around line 33-69: Replace direct os.* filesystem calls in LoadConfig and
EnsureDefaultConfig with the project's vfs helpers so tests can mock disk I/O:
use vfs.ReadFile instead of os.ReadFile when reading the config in LoadConfig,
use vfs.Stat instead of os.Stat to check existence, vfs.MkdirAll instead of
os.MkdirAll to create the configDir, and vfs.WriteFile instead of os.WriteFile
when writing the default config in EnsureDefaultConfig; keep existing behavior
(filepath.Join, json.Unmarshal, json.MarshalIndent, returning wrapped errors)
and the same symbols (LoadConfig, EnsureDefaultConfig, configFileName,
defaultRawConfig, rawConfig, rule, Config) so only the underlying filesystem
calls change.
In `@internal/security/contentsafety/provider_test.go`:
- Around line 16-20: The helper writeTestConfig does not check the error
returned by os.WriteFile which can hide failures; update writeTestConfig to
capture the error from os.WriteFile (called inside writeTestConfig) and call
t.Fatalf or t.Helper plus t.Fatalf/t.Fatalf-like failure (or use
require.NoError) when the write fails so the test fails immediately with the
write error instead of later confusing scan/read errors.
In `@internal/security/contentsafety/provider.go`:
- Around line 46-50: The matched rule IDs are collected from the map hits into
matched which yields nondeterministic order; after building matched (in the
function returning &extcs.Alert{Provider: p.Name(), MatchedRules: matched}),
sort matched (e.g., using sort.Strings) before constructing the extcs.Alert so
_content_safety_alert.matched_rules is deterministic; ensure you import the sort
package if not present and keep the rest of the return logic (Provider:
p.Name(), MatchedRules: matched) unchanged.
- Around line 53-61: The loadOrCreate path in regexProvider (method
loadOrCreate) is not concurrency-safe: concurrent Scan calls can race calling
EnsureDefaultConfig and writing to errOut; wrap the load/create sequence with a
mutex or singleflight so only one goroutine runs EnsureDefaultConfig while
others wait and then retry LoadConfig (do not cache successful config in the
provider), e.g., protect the sequence LoadConfig -> if error then
EnsureDefaultConfig -> LoadConfig with a sync.Mutex or sync/singleflight keyed
on p.configDir so callers of loadOrCreate always reload after the create and
only one creation occurs.
---
Nitpick comments:
In `@internal/output/emit_test.go`:
- Around line 42-44: The tests currently call extcs.Register(mp) then defer
extcs.Register(nil), which overwrites any existing global provider and can leak
state across tests; change to capture the previous provider before registering
(e.g., prev := extcs.Register(...) or call extcs.GetProvider()/prev :=
extcs.Register? depending on API) and in the defer restore that saved provider
with extcs.Register(prev) so the original provider is preserved; apply the same
pattern to the other test block around lines 83-98 and ensure
TestScanForSafety_NoProvider also restores the prior provider after it runs.
In `@internal/output/envelope.go`:
- Around line 8-14: Change the ContentSafetyAlert field from interface{} to a
concrete pointer type to make the contract self-enforcing: replace
ContentSafetyAlert interface{} with ContentSafetyAlert *extcs.Alert and keep the
json tag `"_content_safety_alert,omitempty"`; import the extension/contentsafety
package (alias extcs) in the file and update any code referencing
Envelope.ContentSafetyAlert (e.g., assignments in shortcuts/common/runner.go) to
use the *extcs.Alert type so nil pointer values are omitted correctly by
omitempty and the code compiles.
In `@internal/security/contentsafety/normalize_test.go`:
- Around line 62-68: Update the test TestNormalize_UnmarshalableValue to assert
the returned value's concrete type and identity instead of relying on interface
equality: after calling normalize(ch) assign with a type assertion (gotCh, ok :=
got.(chan int)) and fail the test if !ok or gotCh != ch. This clarifies intent
and avoids depending on interface-value comparability for the normalize
function's result.
In `@shortcuts/common/runner_contentsafety_test.go`:
- Around line 36-43: Replace the manually constructed cmdutil.Factory in the
test’s RuntimeContext with the standard test helper: call cmdutil.TestFactory(t,
cfg) (where cfg is the &core.CliConfig{Brand: core.BrandFeishu} used now) and
assign its result to RuntimeContext.Factory; ensure the test passes the
testing.T instance (t) into TestFactory and use the returned Factory instead of
creating &cmdutil.Factory{IOStreams: ...} so the test uses the repo’s in-memory
config wiring.
- Around line 51-53: Save the existing package-global provider before calling
extcs.Register and restore that saved value in the deferred cleanup instead of
always registering nil; e.g. capture old :=
extcs.Register(&csTestProvider{alert: alert}) (or call a getter if Register
doesn't return the old provider), then use defer extcs.Register(old) to restore
the previous provider; update both places that register csTestProvider (the
block that creates alert := &extcs.Alert{...} and the similar block at lines
70-72) to follow this pattern.
🪄 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: edb6d3b8-16e7-46cc-b5cd-e2a90ad012f0
📒 Files selected for processing (24)
cmd/api/api.gocmd/service/service.goextension/contentsafety/registry.goextension/contentsafety/types.goextension/contentsafety/types_test.gointernal/client/response.gointernal/cmdutil/factory_default.gointernal/envvars/envvars.gointernal/output/emit.gointernal/output/emit_core.gointernal/output/emit_core_test.gointernal/output/emit_test.gointernal/output/envelope.gointernal/output/exitcode.gointernal/security/contentsafety/config.gointernal/security/contentsafety/config_test.gointernal/security/contentsafety/normalize.gointernal/security/contentsafety/normalize_test.gointernal/security/contentsafety/provider.gointernal/security/contentsafety/provider_test.gointernal/security/contentsafety/scanner.gointernal/security/contentsafety/scanner_test.goshortcuts/common/runner.goshortcuts/common/runner_contentsafety_test.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #606 +/- ##
==========================================
+ Coverage 60.20% 60.35% +0.15%
==========================================
Files 407 414 +7
Lines 43340 43646 +306
==========================================
+ Hits 26091 26342 +251
- Misses 15221 15263 +42
- Partials 2028 2041 +13 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/output/emit_core.go`:
- Around line 89-107: runContentSafety spawns a scan goroutine that may outlive
the 100ms timeout and race on the shared errOut writer; fix by giving the
goroutine its own private buffered io.Writer and ensuring the parent discards or
flushes it on timeout, or by waiting briefly for the goroutine to finish after
canceling the context. Concretely, in runContentSafety replace passing the
shared errOut into extcs.ScanRequest with a bytes.Buffer or io.Pipe-backed
buffered writer unique to that goroutine (capture it in the goroutine and
copy/flush/ignore its contents after the select), still call cancel() on
timeout, and only return after either receiving the scan result from ch or after
copying/discarding the buffered output so the provider cannot write into the
original errOut after the function returns; ensure you keep the existing panic
recovery in the goroutine and preserve sending result{a,e} to ch.
🪄 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: 4b47e0d5-90d3-4bbf-b6e3-1fb69d1500eb
📒 Files selected for processing (7)
extension/contentsafety/types.goextension/contentsafety/types_test.gointernal/client/response.gointernal/output/emit_core.gointernal/security/contentsafety/config.gointernal/security/contentsafety/provider.gointernal/security/contentsafety/provider_test.go
✅ Files skipped from review due to trivial changes (1)
- extension/contentsafety/types.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/client/response.go
- internal/security/contentsafety/config.go
- internal/security/contentsafety/provider.go
- internal/security/contentsafety/provider_test.go
…rt, and registry Change-Id: Ibeac6366c7201293057bc3b063f75ac34565bcd5
Change-Id: I7d4729a5ddcab2553abc110f8f6ecc88435ae921
Change-Id: I215dad7cf3072711d05e45f7d384162e1f8752d4
…ules, and allowlist matching Change-Id: I75e10df28f1f8d4f433cb2b469a0ff317af3bf70
…nd allowlist Change-Id: I658889b3647cbbbde6881e0c5f7c13887a1eb1d4
…ation, and scan orchestration Change-Id: I1cb9df75f1a4d176d660e2e7a9561314c3787191
… field Change-Id: I5fdb311e1c8d983a35a58667970b9fd3ac729a5c
…rmat() Change-Id: I33eef1dba14c8a9bd1998857311bdd611f33b916
… and register provider Change-Id: Ic3981db6c546a19eadea095d82175f92f4783bec
Change-Id: Ia2491f7a17caceea3125ff9fb58d750dc196d7e7
Change-Id: I86c5afdfbbdb68d8137f0ca09ef3b5a1139f4b4e
…atched rules, emit warn on --output path Change-Id: Ib4982cd54e1bfe0580a0eb03368e6ca818304e1b
…timeout Change-Id: Ia5a770d7387ba6d3b7fa318fc5f1384214ea10b7
…hortcut data Change-Id: I641e89113d1a2f2285ac6109bd3d7264f5845ea7
… test, scanTimeout comment Change-Id: Ie45a2e365ee7098e214e94f8871026cc12029d83
MaxHuang22
force-pushed
the
feat/safety-prompt-injection
branch
from
40d54b5 to
5307f71
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
internal/security/contentsafety/config_test.go (1)
38-66: Check os.WriteFile errors in negative-path setups.Lines 40, 49, 58, 92 ignore the write error. If the temp-dir write ever fails,
LoadConfigmay then fail for a different reason (e.g. "file not found") and produce misleading test output. Same pattern aswriteTestConfigwhich was already hardened.🧪 Proposed fix
- os.WriteFile(filepath.Join(dir, "content-safety.json"), []byte(`{bad`), 0644) + if err := os.WriteFile(filepath.Join(dir, "content-safety.json"), []byte(`{bad`), 0644); err != nil { + t.Fatal(err) + }(apply similarly to lines 49, 58, 92)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/security/contentsafety/config_test.go` around lines 38 - 66, Several tests (TestLoadConfig_InvalidJSON, TestLoadConfig_InvalidRegex, TestLoadConfig_EmptyRules) call os.WriteFile and ignore its error; update each test to capture the return value from os.WriteFile and fail the test immediately on error (e.g., if err := os.WriteFile(...); err != nil { t.Fatalf("os.WriteFile failed: %v", err) }) before calling LoadConfig so failures to create the temporary config file don’t produce misleading LoadConfig errors.internal/output/emit_test.go (1)
95-126: LGTM — fail-open semantics for scan errors and timeout are well-covered.The string check
strings.Contains(buf.String(), "scan error")tightly couples the test to the stderr wording inemit.go; consider asserting on a more stable substring (e.g. the provider name) if the warning message evolves. Optional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/output/emit_test.go` around lines 95 - 126, The test TestScanForSafety_ScanError_FailOpen is brittle because it asserts the exact stderr wording ("scan error"); update the assertion to check for a stable substring such as the provider name instead—use the mock provider's name (mp.name or the literal "mock") when checking buf.String() so the test only verifies that the provider emitted a warning rather than coupling to the full message text emitted by ScanForSafety/emit.go; replace the strings.Contains(buf.String(), "scan error") check accordingly and keep the rest of the test unchanged.internal/security/contentsafety/config.go (1)
56-73: Minor:vfs.Staterror is treated as "missing" regardless of cause.Any non-nil
Staterror (permission denied, I/O error, etc.) falls through toMkdirAll/WriteFile. In practice the subsequent write will surface the real error, so behavior is acceptable, but gating onerrors.Is(err, fs.ErrNotExist)would make intent clearer and avoid redundant write attempts on transient Stat failures.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/security/contentsafety/config.go` around lines 56 - 73, In EnsureDefaultConfig, don’t treat any non-nil vfs.Stat error as "missing" — after calling vfs.Stat(path) keep the existing early return when err==nil, but if err!=nil check errors.Is(err, fs.ErrNotExist) and only proceed to create the dir/file in that case; for any other Stat error return a wrapped error (e.g., fmt.Errorf("stat config: %w", err)). This change uses errors.Is and references EnsureDefaultConfig and vfs.Stat so transient permission/I/O errors are surfaced immediately instead of attempting MkdirAll/WriteFile.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/security/contentsafety/config.go`:
- Around line 99-108: In IsAllowlisted, empty strings in allowlist (entry) can
incorrectly match cmdPath (especially via HasPrefix with "."), so update the
loop over allowlist to skip blank entries (e.g., if entry == "" or trimmed entry
length == 0 then continue) before testing for "all", exact match or prefix;
reference the IsAllowlisted function and variables cmdPath, allowlist and entry
when applying this guard so empty/whitespace entries do not cause false
positives.
In `@internal/security/contentsafety/provider.go`:
- Around line 71-74: In loadOrCreate, the EnsureDefaultConfig failure is being
masked by returning the stale err from LoadConfig; change the error handling so
that when EnsureDefaultConfig returns errC you return errC (the creation error)
rather than err, i.e., check the result of EnsureDefaultConfig and propagate
errC directly so callers see the actual MkdirAll/WriteFile permission or
creation error instead of the prior read error.
In `@shortcuts/common/runner.go`:
- Around line 520-533: The "pretty" branch double-runs safety scanning and
double-reports alerts; change the flow so output.ScanForSafety is executed only
once and its result is honored by the downstream output path when prettyFn is
nil: call output.ScanForSafety(...) once into scanResult, call
output.WriteAlertWarning(...) only if you intend a stderr warning, then when
prettyFn == nil attach scanResult.Alert (or a marker like
meta["_content_safety_alert_checked"]=true plus the alert payload) to the meta
passed to ctx.Out(data, meta) so ctx.Out/ctx.Out's internals can detect the
checked flag and skip a second ScanForSafety. Update handling around
scanResult.Blocked, WriteAlertWarning, prettyFn, ctx.Out and the meta envelope
to ensure a single scan and single, consistent alert reporting.
---
Nitpick comments:
In `@internal/output/emit_test.go`:
- Around line 95-126: The test TestScanForSafety_ScanError_FailOpen is brittle
because it asserts the exact stderr wording ("scan error"); update the assertion
to check for a stable substring such as the provider name instead—use the mock
provider's name (mp.name or the literal "mock") when checking buf.String() so
the test only verifies that the provider emitted a warning rather than coupling
to the full message text emitted by ScanForSafety/emit.go; replace the
strings.Contains(buf.String(), "scan error") check accordingly and keep the rest
of the test unchanged.
In `@internal/security/contentsafety/config_test.go`:
- Around line 38-66: Several tests (TestLoadConfig_InvalidJSON,
TestLoadConfig_InvalidRegex, TestLoadConfig_EmptyRules) call os.WriteFile and
ignore its error; update each test to capture the return value from os.WriteFile
and fail the test immediately on error (e.g., if err := os.WriteFile(...); err
!= nil { t.Fatalf("os.WriteFile failed: %v", err) }) before calling LoadConfig
so failures to create the temporary config file don’t produce misleading
LoadConfig errors.
In `@internal/security/contentsafety/config.go`:
- Around line 56-73: In EnsureDefaultConfig, don’t treat any non-nil vfs.Stat
error as "missing" — after calling vfs.Stat(path) keep the existing early return
when err==nil, but if err!=nil check errors.Is(err, fs.ErrNotExist) and only
proceed to create the dir/file in that case; for any other Stat error return a
wrapped error (e.g., fmt.Errorf("stat config: %w", err)). This change uses
errors.Is and references EnsureDefaultConfig and vfs.Stat so transient
permission/I/O errors are surfaced immediately instead of attempting
MkdirAll/WriteFile.
🪄 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: 4063ecc6-edff-4061-8abd-f81f701e9bf2
📒 Files selected for processing (24)
cmd/api/api.gocmd/service/service.goextension/contentsafety/registry.goextension/contentsafety/types.goextension/contentsafety/types_test.gointernal/client/response.gointernal/cmdutil/factory_default.gointernal/envvars/envvars.gointernal/output/emit.gointernal/output/emit_core.gointernal/output/emit_core_test.gointernal/output/emit_test.gointernal/output/envelope.gointernal/output/exitcode.gointernal/security/contentsafety/config.gointernal/security/contentsafety/config_test.gointernal/security/contentsafety/normalize.gointernal/security/contentsafety/normalize_test.gointernal/security/contentsafety/provider.gointernal/security/contentsafety/provider_test.gointernal/security/contentsafety/scanner.gointernal/security/contentsafety/scanner_test.goshortcuts/common/runner.goshortcuts/common/runner_contentsafety_test.go
✅ Files skipped from review due to trivial changes (7)
- internal/cmdutil/factory_default.go
- internal/envvars/envvars.go
- internal/security/contentsafety/normalize_test.go
- internal/security/contentsafety/scanner_test.go
- extension/contentsafety/registry.go
- internal/security/contentsafety/scanner.go
- cmd/service/service.go
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/output/exitcode.go
- internal/output/envelope.go
- cmd/api/api.go
- extension/contentsafety/types_test.go
- extension/contentsafety/types.go
liangshuo-1
approved these changes
Apr 23, 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 a configurable, regex-based content-safety scanning layer at the CLI output boundary to detect prompt-injection patterns in API responses. Activated via
LARKSUITE_CLI_CONTENT_SAFETY_MODE(off/warn/block); rules are driven by~/.lark-cli/content-safety.jsonwith 4 default patterns (instruction override, role injection, system-prompt leak, delimiter smuggling). Fail-open by design: scan errors, timeouts (100 ms), and panics are suppressed and the response passes through unchanged.Changes
extension/contentsafety/: newProviderinterface,Alertstruct, andRegister/GetProviderregistryinternal/security/contentsafety/: regex provider, config loader with lazy-create (emits stderr notice on first use), recursive tree-walker scanner, normalizerinternal/output/emit.go+emit_core.go:ScanForSafety()entry point, mode parsing, command-path normalizationinternal/output/envelope.go: added_content_safety_alertfield for warn-mode alert embeddinginternal/output/exitcode.go: addedExitContentSafety = 6internal/envvars/envvars.go: addedCliContentSafetyModeconstantshortcuts/common/runner.go: integrated scanning inOut()andOutFormat()(shortcut output path)internal/client/response.go: integrated scanning inHandleResponse()(api/service output path)cmd/api/api.go,cmd/service/service.go: passCommandPathtoResponseOptionsinternal/cmdutil/factory_default.go: blank-import to register the regex provider viainit()Test Plan
go build ./...passedgo vet ./...passedinternal/security/contentsafety,internal/output,shortcuts/common(warn/block/off modes)_content_safety_alertin envelope, data suppression in block modeLARKSUITE_CLI_CONTENT_SAFETY_MODE=block lark-cli api GET /open-apis/bot/v3/infowith matching rule → exit 6, stderr structured error, stdout emptyRelated Issues
N/A
Summary by CodeRabbit
New Features
Exit Codes