feat: add diagnose-scope audit test for minimum-privilege scope analysis#370
feat: add diagnose-scope audit test for minimum-privilege scope analysis#370MaxHuang22 merged 1 commit intomainfrom
Conversation
github-actions
bot
added
the
size/S
📝 WalkthroughWalkthroughAdds a new test that builds a deterministic JSON snapshot of scope coverage across API command domains and shortcut services, merges identity types, annotates scopes with recommendation and priority metadata, and optionally writes the snapshot to disk when Changes
Sequence Diagram(s)sequenceDiagram
participant Test as TestScopeSnapshot
participant Registry as Registry
participant Shortcuts as Shortcuts
participant FileSys as FileSystem
Test->>Registry: ListFromMetaProjects()
Registry-->>Test: domains
Test->>Registry: CollectCommandScopes(domains)
Registry-->>Test: command->scopes
Test->>Shortcuts: AllShortcuts()
Shortcuts-->>Test: shortcuts list
Test->>Test: merge identities, normalize method names, annotate Recommend/InPriority, sort
alt SCOPE_SNAPSHOT_DIR set
Test->>FileSys: create dir (if needed)
Test->>FileSys: write snapshot.json
FileSys-->>Test: write result
else not set
Test-->>Test: skip writing
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR adds Confidence Score: 5/5Safe to merge — all changes are test-only and do not affect the binary or production code paths. The single changed file is gated behind env vars and compiles only into the test binary. Core logic (identity merging, sorting, registry self-init) is correct. Remaining findings are P2 style/description issues that don't block merge. cmd/diagnose_scope_test.go — PR description references TestScopeDiff, TestScopeTable, and TestDiagScope which were not committed.
|
| Filename | Overview |
|---|---|
| cmd/diagnose_scope_test.go | New test-only file that builds a deterministic scope snapshot for audit purposes; core logic is sound, but PR description describes TestScopeDiff, TestScopeTable, and TestDiagScope which are not present in the committed file. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[TestScopeSnapshot] -->|env SCOPE_SNAPSHOT_DIR set| B[registry.Init]
B --> C[diagAllKnownDomains]
C --> D[registry.ListFromMetaProjects]
C --> E[shortcuts.AllShortcuts]
D & E --> F[merged domain list]
F --> G[diagBuild domains]
G --> H{for each domain x identity}
H --> I[registry.CollectCommandScopes\napi methods]
H --> J[shortcuts.AllShortcuts\nfiltered by Service+identity]
I & J --> K[merge into map methodKey→entry\nappendUniq identities]
K --> L[sort methods slice]
L --> M[build scopes list\nLoadScopePriorities + LoadAutoApproveSet]
M --> N[diagOutput]
N --> O[json.MarshalIndent]
O --> P[os.WriteFile snapshot.json]
Reviews (3): Last reviewed commit: "feat: add scope snapshot test for minimu..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@a9a8dbedca7f9e8d0e79dfd945c249b8ec5cff1d🧩 Skill updatenpx skills add larksuite/cli#feat/diagnose-scope-test -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: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/diagnose_scope_test.go`:
- Around line 145-153: The comparator used in the sort.Slice call that sorts the
methods slice (comparing methods[i].Domain, .Type, .Method) is missing
tie-breakers and can produce nondeterministic ordering when a method has
multiple scopes; update the comparator inside the sort.Slice that sorts methods
to further compare methods[i].Scope and then methods[i].Name (or the equivalent
Name field) as final tie-breakers to ensure stable ordering, and apply the same
change to the other similar sort.Slice comparator mentioned (the second
occurrence around the other sort) so both sorts use Domain, Type, Method, Scope,
Name ordering.
- Around line 551-552: The table example comment for TestScopeTable is
misleading because the test is skipped unless SCOPE_TABLE is set; update the
comment lines (the example invoking TestScopeTable) to include SCOPE_TABLE=1 so
it reads like "SCOPE_TABLE=1 go test ./cmd/ -run TestScopeTable -v" (apply the
same change to the other occurrence at the nearby comment block covering lines
613-616) so the example matches the gate that checks the SCOPE_TABLE environment
variable.
- Around line 395-397: The tests (e.g., TestDiagAllKnownDomains) call
registry.Init() against the process-wide registry and can pick up local cache
state; update each test to call a shared helper (create or import
ensureFreshRegistry(t *testing.T) similar to internal/registry/registry_test.go)
before registry.Init() so LARKSUITE_CLI_CONFIG_DIR is set to a fresh temp dir
and LARKSUITE_CLI_REMOTE_META is disabled, then reinitialize the registry and
proceed to call diagAllKnownDomains (and other functions like
diagAllKnownDomains used in tests at the listed locations); ensure the helper
resets environment and calls registry.Init() so tests are isolated and
deterministic.
- Around line 554-565: The test reads SCOPE_SNAPSHOT_DIR and SCOPE_DIFF_BASE and
uses them directly for filesystem operations; update the test to validate both
environment path values with validate.SafeInputPath before any file I/O (before
calling os.MkdirAll, filepath.Join, ioutil.ReadFile/WriteFile) so the harness
honors the repo path-safety contract; locate the setup around
diagBuild(diagAllKnownDomains()) and adjust the code that reads
SCOPE_SNAPSHOT_DIR and later SCOPE_DIFF_BASE (and the blocks at lines noted: the
MkdirAll/Join/WriteFile and the later ReadFile/Join/WriteFile blocks) to call
validate.SafeInputPath on each env var and handle validation errors by t.Skip or
t.Fatalf as appropriate.
- Around line 602-609: The code currently ignores errors from json.MarshalIndent
and os.WriteFile and always logs success; update the block that writes the
snapshot (the branch using os.Getenv("SCOPE_SNAPSHOT_DIR"),
json.MarshalIndent(diff, ...), and os.WriteFile(path, ...)) to check and handle
both errors: if json.MarshalIndent returns an error, call t.Fatalf or t.Errorf
(prefer t.Fatalf in this test) with the marshal error and context; after
attempting os.WriteFile check its error and call t.Fatalf/t.Errorf with the
write error instead of discarding it, and only call t.Logf("Wrote diff to %s",
path) when both operations succeed. Ensure you reference the same variables
(diff, dir, path) so the changes are localized to that snippet.
🪄 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: 2e3af79c-f3c7-477d-8d88-2b981691662e
📒 Files selected for processing (1)
cmd/diagnose_scope_test.go
MaxHuang22
force-pushed
the
feat/diagnose-scope-test
branch
from
b0b6fba to
d537860
Compare
|
Tip: Greploop — Automatically fix all review issues by running Use the Greptile plugin for Claude Code to query reviews, search comments, and manage custom context directly from your terminal. |
There was a problem hiding this comment.
♻️ Duplicate comments (3)
cmd/diagnose_scope_test.go (3)
121-129:⚠️ Potential issue | 🟠 MajorComplete the sort key to keep snapshots deterministic.
At Line 121, the comparator stops at
Method; entries sharing the same method but different scopes can still reorder across runs (map iteration), causing noisy snapshot/diff churn.💡 Suggested fix
sort.Slice(methods, func(i, j int) bool { if methods[i].Domain != methods[j].Domain { return methods[i].Domain < methods[j].Domain } if methods[i].Type != methods[j].Type { return methods[i].Type < methods[j].Type } - return methods[i].Method < methods[j].Method + if methods[i].Method != methods[j].Method { + return methods[i].Method < methods[j].Method + } + return methods[i].Scope < methods[j].Scope })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/diagnose_scope_test.go` around lines 121 - 129, The sort comparator used in sort.Slice over the methods slice stops at Method, which allows entries with identical Domain, Type and Method but different scope to reorder; update the comparator in the sort.Slice call (the anonymous func) to include the scope field as a final tiebreaker (e.g., compare methods[i].Scope vs methods[j].Scope) so sorting is fully deterministic across runs.
182-183:⚠️ Potential issue | 🟠 MajorIsolate registry/config state before
registry.Init().At Line 182, this test initializes registry without setting an isolated config dir (and remote-meta off), so audit output can drift with machine-local cache/state. Set test env first (prefer a small helper reused by this file).
Based on learnings,
ensureFreshRegistry(t *testing.T)ininternal/registry/registry_test.gosetsLARKSUITE_CLI_CONFIG_DIR, turnsLARKSUITE_CLI_REMOTE_METAoff, then reinitializes the registry to avoid machine-local cache reads.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/diagnose_scope_test.go` around lines 182 - 183, The test calls registry.Init() directly which can read machine-local cache; before initializing the registry (before registry.Init() / before calling diagBuild(diagAllKnownDomains())), ensure the registry/config state is isolated by calling the existing helper ensureFreshRegistry(t) (or replicate its steps: set LARKSUITE_CLI_CONFIG_DIR to a temp dir, set LARKSUITE_CLI_REMOTE_META to "off", then reinitialize via registry.Init()); replace the direct registry.Init() call with a call to ensureFreshRegistry(t) so the test uses an isolated config and stable audit output.
177-195:⚠️ Potential issue | 🟠 MajorValidate
SCOPE_SNAPSHOT_DIRbefore using it for file I/O.At Line 177,
SCOPE_SNAPSHOT_DIRis consumed as a raw path and then used byMkdirAll,Join, andWriteFile. Please run it throughvalidate.SafeInputPathfirst and fail/skip when invalid.As per coding guidelines,
**/*.go: Validate paths usingvalidate.SafeInputPathbefore any file I/O operations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/diagnose_scope_test.go` around lines 177 - 195, The test reads SCOPE_SNAPSHOT_DIR and uses it directly for file I/O; run the environment value through validate.SafeInputPath and abort/skip on invalid input. Specifically, after retrieving dir := os.Getenv("SCOPE_SNAPSHOT_DIR"), call validated, err := validate.SafeInputPath(dir) (or use the library's returned value pattern), and if err != nil call t.Skipf or t.Fatalf as appropriate; then use the validated path variable for os.MkdirAll, filepath.Join (for "snapshot.json"), and os.WriteFile instead of the raw dir to ensure all uses in this test (MkdirAll, Join, WriteFile) operate on a validated path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@cmd/diagnose_scope_test.go`:
- Around line 121-129: The sort comparator used in sort.Slice over the methods
slice stops at Method, which allows entries with identical Domain, Type and
Method but different scope to reorder; update the comparator in the sort.Slice
call (the anonymous func) to include the scope field as a final tiebreaker
(e.g., compare methods[i].Scope vs methods[j].Scope) so sorting is fully
deterministic across runs.
- Around line 182-183: The test calls registry.Init() directly which can read
machine-local cache; before initializing the registry (before registry.Init() /
before calling diagBuild(diagAllKnownDomains())), ensure the registry/config
state is isolated by calling the existing helper ensureFreshRegistry(t) (or
replicate its steps: set LARKSUITE_CLI_CONFIG_DIR to a temp dir, set
LARKSUITE_CLI_REMOTE_META to "off", then reinitialize via registry.Init());
replace the direct registry.Init() call with a call to ensureFreshRegistry(t) so
the test uses an isolated config and stable audit output.
- Around line 177-195: The test reads SCOPE_SNAPSHOT_DIR and uses it directly
for file I/O; run the environment value through validate.SafeInputPath and
abort/skip on invalid input. Specifically, after retrieving dir :=
os.Getenv("SCOPE_SNAPSHOT_DIR"), call validated, err :=
validate.SafeInputPath(dir) (or use the library's returned value pattern), and
if err != nil call t.Skipf or t.Fatalf as appropriate; then use the validated
path variable for os.MkdirAll, filepath.Join (for "snapshot.json"), and
os.WriteFile instead of the raw dir to ensure all uses in this test (MkdirAll,
Join, WriteFile) operate on a validated path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3b64cc55-36f9-4a37-9def-5db000f93a01
📒 Files selected for processing (1)
cmd/diagnose_scope_test.go
Add cmd/diagnose_scope_test.go that exports a JSON snapshot of all API methods and shortcuts with their minimum-privilege scopes, identity support, auto-approve status, and scope_priorities coverage. Consumed by scripts/scope_audit.py for diff and reporting. Change-Id: I89b9ac03413e412447e26c5a21e06d18922709e3 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
MaxHuang22
force-pushed
the
feat/diagnose-scope-test
branch
from
d537860 to
a9a8dbe
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
cmd/diagnose_scope_test.go (2)
186-187:⚠️ Potential issue | 🟠 MajorIsolate registry state before calling
registry.Init().At Line 186, this test initializes global registry state without isolating config/cache inputs, so results can drift by machine or prior test process state.
Based on learnings,
ensureFreshRegistry(t *testing.T)ininternal/registry/registry_test.gosetsLARKSUITE_CLI_CONFIG_DIR, disables remote meta, then reinitializes to avoid machine-local cache reads.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/diagnose_scope_test.go` around lines 186 - 187, The test currently calls registry.Init() directly which uses global registry state; replace that with a fresh isolated setup by calling the helper ensureFreshRegistry(t *testing.T) (the one defined in internal/registry/registry_test.go) before invoking registry.Init() or instead of it, so that LARKSUITE_CLI_CONFIG_DIR is set, remote meta is disabled, and the registry is reinitialized; then run result := diagBuild(diagAllKnownDomains()) as before to ensure deterministic test outcomes.
181-193:⚠️ Potential issue | 🟠 MajorValidate
SCOPE_SNAPSHOT_DIRbefore filesystem I/O.At Lines 181-193, the env path is used directly in
MkdirAll,Join, andWriteFile. Validate it withvalidate.SafeInputPathfirst, then proceed with file operations.As per coding guidelines,
**/*.go: Validate paths usingvalidate.SafeInputPathbefore any file I/O operations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/diagnose_scope_test.go` around lines 181 - 193, The test currently uses the environment path variable dir directly for filesystem operations (os.MkdirAll, filepath.Join, and file write) — first call validate.SafeInputPath(dir) and handle returned error (t.Skip or t.Fatalf as appropriate) before any I/O; update the block around the SCOPE_SNAPSHOT_DIR usage so the validated path replaces dir for subsequent calls to os.MkdirAll, filepath.Join (for snapshot.json) and any WriteFile operations, referencing the existing dir variable and validate.SafeInputPath to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@cmd/diagnose_scope_test.go`:
- Around line 186-187: The test currently calls registry.Init() directly which
uses global registry state; replace that with a fresh isolated setup by calling
the helper ensureFreshRegistry(t *testing.T) (the one defined in
internal/registry/registry_test.go) before invoking registry.Init() or instead
of it, so that LARKSUITE_CLI_CONFIG_DIR is set, remote meta is disabled, and the
registry is reinitialized; then run result := diagBuild(diagAllKnownDomains())
as before to ensure deterministic test outcomes.
- Around line 181-193: The test currently uses the environment path variable dir
directly for filesystem operations (os.MkdirAll, filepath.Join, and file write)
— first call validate.SafeInputPath(dir) and handle returned error (t.Skip or
t.Fatalf as appropriate) before any I/O; update the block around the
SCOPE_SNAPSHOT_DIR usage so the validated path replaces dir for subsequent calls
to os.MkdirAll, filepath.Join (for snapshot.json) and any WriteFile operations,
referencing the existing dir variable and validate.SafeInputPath to locate the
change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bbe16f14-7701-4f30-9c09-34b2ba2eba78
📒 Files selected for processing (1)
cmd/diagnose_scope_test.go
2 participants
Summary
cmd/diagnose_scope_test.gothat exports a JSON snapshot of all API methods and shortcuts with their minimum-privilege scopesscripts/scope_audit.pyfor diff reporting and Lark webhook notificationsDetails
Snapshot JSON includes per-method:
domain,type(api/shortcut),method(e.g.calendar.calendars.search),scope,identity([user], [bot], or both)And per-scope:
recommend(auto-approve status),in_priority(whether scope exists inscope_priorities.json)Usage
SCOPE_SNAPSHOT_DIR=/tmp/scope-audit go test ./cmd/ -run TestScopeSnapshot -vTest plan
go test ./cmd/ -run TestScopeSnapshot -v— skips without env varSCOPE_SNAPSHOT_DIR=/tmp/scope-audit go test ./cmd/ -run TestScopeSnapshot -v— generatessnapshot.json🤖 Generated with Claude Code