Skip to content

test(privatefile, workspaceidentity): 补齐凭据原子写与路径排除边界的单元测试 - #517

Open
Qiyuanqiii wants to merge 2 commits into
mainfrom
Qiyuanqiii/test-privatefile-workspaceidentity-coverage
Open

Qiyuanqiii wants to merge 2 commits into
mainfrom
Qiyuanqiii/test-privatefile-workspaceidentity-coverage

Conversation

@Qiyuanqiii

@Qiyuanqiii Qiyuanqiii commented Oct 29, 2026 •

Copy link
Copy Markdown
Member

Description

internal/privatefile and internal/workspaceidentity are two packages without a single test file, and both sit on a security boundary:

  • internal/workspaceidentity provides PathPolicy, which decides which paths an Agent may not read or write, and Fingerprint, which binds a workspace root to a platform file identity (dev/ino, or volume serial number plus file index).
  • internal/privatefile performs the atomic write and the cross-process lock behind the OAuth credential files, and is imported directly by internal/chatgptauth and internal/mcpoauth.

internal/privatefile/file.go already reserves an injection point for tests:

// rename may be nil to use os.Rename; callers may inject it to test an interrupted commit.

No test in the repository used it, so the interrupted-commit failure path was never verified.

This PR adds tests only. No production code changes, no runtime behavior changes.

Test design

Follows the existing repository conventions. Internal test packages (package privatefile / package workspaceidentity), stable error-code assertions (apperror.CodeOf(err) != apperror.CodeInvalidArgument, matching internal/application/agent_code_project_config_test.go), no new dependencies, no new test framework, and no contact with internal/llm or any API contract.

Covers the refusal paths, not the happy path. The value of a security boundary lives in its rejection branches. PathPolicy covers absolute paths, .. escapes, unnormalized paths, control characters, invalid UTF-8, malformed globs, and an invalid policy itself; privatefile covers an injected interrupted commit (the original file content stays unchanged and the temporary file is cleaned up), a missing parent directory, an already-cancelled context, a context that times out while waiting for the lock, and mutual exclusion between concurrent holders.

Locks the current contract instead of making a semantic decision for maintainers. MatchPath("a/**", "a") currently returns true, that is, ** also matches zero path segments. The existing test (agent_code_project_config_test.go uses ExcludePaths: ["secrets/**"]) passes under both semantics, so the intent cannot be inferred. This PR asserts the current implementation behavior and states the reason in a code comment: ** covering zero segments keeps the decision for Check("a") consistent with Check("a/b/c"), avoiding a split where the directory is visible but its contents are hidden. If maintainers think at least one segment should be required, that is a separate behavior change and belongs in its own issue, not in a test PR.

Platform portability. The 0600 permission assertion is skipped on Windows, where Chmod only affects the read-only bit. The symlink case skips itself with t.Skipf where symlinks cannot be created, so it never passes silently. The process-lock case relies on mutual exclusion between different handles, which Linux flock and Windows LockFileEx both provide.

21 test functions were added, including three table-driven groups that cover 37 further cases.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Refactoring (no functional changes)
  • Documentation update
  • CI / Build / Tooling

This is a test-only addition: no production file changes, and the classified check is backend only.

Surface governance

  • No Surface is added, promoted, downgraded, deprecated, or removed.

N/A — no Surface change. No registry entry, generated inventory, or documentation contract is touched, because the change only adds test files and alters no public contract.

Validation

Current revision: b0a3eef5, synchronized with main through b58f8981.

Check Result Revision
gofmt -l internal/privatefile internal/workspaceidentity No output b0a3eef5
go vet ./internal/privatefile/... ./internal/workspaceidentity/... Passed b0a3eef5
go test -count=1 ./internal/privatefile/... ./internal/workspaceidentity/... Passed: privatefile 0.623s, workspaceidentity 0.450s b0a3eef5
go test -race -count=1 ./internal/privatefile/... ./internal/workspaceidentity/... Passed: privatefile 1.525s, workspaceidentity 1.357s b0a3eef5
Downstream regression, ./internal/codeintel/... ./internal/credential/... Passed: codeintel 7.616s, credential 0.234s b0a3eef5
go build ./... Passed b0a3eef5
go test -count=1 ./... (full repository) The run was stopped after internal/app, because the heavy internal/application and internal/store packages exceed the local session budget on this Windows host. The only failure observed, internal/app TestRunCommandExecuteIsConfirmedAuditedAndExactlyOnce (controlled command execution platform is unavailable: approved executable is unavailable), reproduces identically on the untouched base b58f8981 in a separate worktree, so it is a limitation of this host and not a regression from this PR. b0a3eef5
Coverage of the two changed packages privatefile 83.3%, workspaceidentity 91.7%; both packages had no test file at all before, so they carried no coverage b0a3eef5
scripts/ci/classify_changes.py, base b58f8981 Selected checks: backend; full, store, web, journeys, lsp, rust, native, ui, race, audit, and packaging are all false b0a3eef5
Symlink subtest isolation TestFingerprintRefusesFilesSymlinksAndMissingRoots now runs regular file, missing root, and symlinked root as separate subtests, so a host without symlink support skips only the symlink case b0a3eef5
CI Select affected checks and Go checks Passed on Linux, run 40882124390: go vet and go test for both changed packages, plus internal/releasegate and internal/producte2e; every other gate was skipped by classification b0a3eef5

Local validation used Windows amd64 with Go 1.26.10, the version pinned by go.mod and selected automatically through GOTOOLCHAIN=auto.

Limitations

  • The suite was executed on Windows amd64 only. The macOS lock_unix.go and identity_unix.go paths were not exercised on a real macOS machine.
  • TestFingerprintRefusesFilesSymlinksAndMissingRoots skipped locally because symlink creation is not available for this account on Windows; the Linux CI run exercised it instead.
  • No end-to-end check against real credential files or a real OAuth flow. This PR only covers pure local unit behavior.
  • internal/llm, the OpenAPI contract, and every frontend file are untouched, so npm run check:api and the frontend suites were not run.

Checklist

  • My code follows the project's coding style (go fmt, go vet)
  • I have performed a self-review of my code
  • I have added tests that prove my fix is effective or my feature works
  • New and existing unit tests pass locally with my changes — the two changed packages, their downstream consumers, and the CI Go checks gate pass; the single internal/app failure seen during the full local run also fails on the untouched base
  • I have updated the documentation accordingly (if applicable) — not applicable, this is a test-only change
  • I have signed the CLA — to be confirmed by the contributor before merge
  • I did not use AI/LLM to create this PR, or I disclosed the tool/model below and reviewed its output; I did not attribute commits to AI and will answer maintainer questions and review comments myself without AI/LLM.
    • The commit in this PR is authored by the MiniMax-AI-Dev account, so the "no AI attribution in commits" clause does not hold here. The tool and model are disclosed below, the contributor has reviewed the generated tests, and the contributor will answer maintainer questions and review comments.

AI Assistance

The tests, the validation runs, and this description were prepared with MiniMax Code using the MiniMax M3.1 Flash model. The single commit is authored by the MiniMax-AI-Dev GitHub account. The contributor has reviewed the generated tests and remains responsible for answering maintainer questions and review comments.

Related Issues

None.

internal/privatefile 与 internal/workspaceidentity 此前没有任何测试文件,而两者都处在安全边界上:前者负责 OAuth 凭据的原子写与跨进程锁,后者决定 Agent 不允许读写哪些路径。

internal/privatefile/file.go 已预留可注入的 rename 钩子用于测试提交被中断,但仓库中从未有测试使用它。

本 PR 不改动任何生产代码,仅新增测试:
- internal/privatefile        0% -> 83.3%
- internal/workspaceidentity 0% -> 95.4%

新增 44 个用例覆盖中断提交后原文件不变且临时文件已清理、路径逃逸与绝对路径拒绝、非法 glob 与控制字符、并发持锁互斥等拒绝路径。
Qiyuanqiii pushed a commit that referenced this pull request Oct 30, 2026
internal/approval 是审批账本的核心安全原语,但 bounded_grant.go 与
SessionGrant/CreateGrantRequest/RevokeGrantRequest 的校验矩阵此前没有
单元测试(包覆盖率 24.9%)。本 PR 仅新增测试,无生产代码改动:

- boundedGrantPermission 权限白名单(command_runtime/command_process
  × ask/auto/full,及 workspace_access 历史路径)
- MatchesBoundedGrantScope 全字段绑定校验(逐字段漂移拒绝)
- CheckBoundedConsumption 消费核验:作用域漂移、显式撤销否决、过期、
  ordinal 超用、以及"额度耗尽型撤销"的合法终局用例
- SessionGrant.Validate 有界/遗留(legacy)一致性、TTL≤15min、
  MaxUses≤8、撤销元数据一致性等边界
- GrantConsumption/Record/DecisionRequest 校验与指纹辅助函数绑定

包覆盖率 24.9% → 87.6%。与 #517(privatefile/workspaceidentity)互补,
不触碰其文件。
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants