Skip to content

chore(store): cover NormalizeBotSetupCodeDefaults validation gates#104

Merged
steipete merged 1 commit into
openclaw:mainfrom
KrasimirKralev:test/setup-code-defaults-coverage
Jul 21, 2026
Merged

chore(store): cover NormalizeBotSetupCodeDefaults validation gates#104
steipete merged 1 commit into
openclaw:mainfrom
KrasimirKralev:test/setup-code-defaults-coverage

Conversation

@KrasimirKralev

Copy link
Copy Markdown
Contributor

What Problem This Solves

NormalizeBotSetupCodeDefaults (apps/api/internal/store/setup_code_defaults.go) validates the defaults payload carried by every bot setup-code grant. It runs on the mint path of both store backends (internal/store/sqlite/setup_codes.go:85 and internal/store/postgres/setup_codes.go:85), so it sits on the public API boundary. It enforces several non-obvious gates:

  • defaultTo is trimmed, then bounded for UTF-8 validity, NUL bytes, and a rune-count length cap (not byte length);
  • allowFrom has a hard entry-count cap, per-entry trim + non-empty check + the same UTF-8/NUL/length validation, and first-seen de-duplication;
  • agentActivity: true requires the dedicated agent_activity:write scope.

The function shipped with no direct test coverage. A regression in any of these gates — dropping the dedup, measuring the length in bytes instead of runes, or removing the scope requirement — would not be caught before merge.

Why This Change Was Made

Coverage-only. This adds one new file, apps/api/internal/store/setup_code_defaults_test.go (+162, test-only), pinning the validator's current main behavior. The tests drive the real exported NormalizeBotSetupCodeDefaults directly (no stubs), including the boundary cases (256-rune multibyte defaultTo, exactly-at-cap allowFrom) and the pointer cases (agentActivity nil / true / false).

User Impact

No user-visible or runtime change — no production code is touched. For maintainers, the setup-code defaults validation gates now regress loudly instead of silently: a future edit that breaks the rune-length measure, the de-duplication, the NUL/UTF-8 checks, or the agent_activity:write gate will fail this suite.

Evidence

Linux, Go toolchain from go.mod (1.26.x), branched off current main (c9cf5ba).

12/12 pass on clean main:

$ go test ./internal/store/ -run TestNormalizeBotSetupCodeDefaults -v
--- PASS: TestNormalizeBotSetupCodeDefaults_TrimsAndDedupes (0.00s)
--- PASS: TestNormalizeBotSetupCodeDefaults_NilAllowFromStaysNil (0.00s)
--- PASS: TestNormalizeBotSetupCodeDefaults_DefaultToLengthIsRuneCount (0.00s)
--- PASS: TestNormalizeBotSetupCodeDefaults_DefaultToRejectsNUL (0.00s)
--- PASS: TestNormalizeBotSetupCodeDefaults_DefaultToRejectsInvalidUTF8 (0.00s)
--- PASS: TestNormalizeBotSetupCodeDefaults_AllowFromTooManyEntries (0.00s)
--- PASS: TestNormalizeBotSetupCodeDefaults_AllowFromAtCapPasses (0.00s)
--- PASS: TestNormalizeBotSetupCodeDefaults_AllowFromRejectsEmptyAfterTrim (0.00s)
--- PASS: TestNormalizeBotSetupCodeDefaults_AllowFromEntryValidated (0.00s)
--- PASS: TestNormalizeBotSetupCodeDefaults_AgentActivityRequiresScope (0.00s)
--- PASS: TestNormalizeBotSetupCodeDefaults_AgentActivityWithScopePasses (0.00s)
--- PASS: TestNormalizeBotSetupCodeDefaults_AgentActivityFalseNeedsNoScope (0.00s)
ok      github.com/openclaw/clickclack/apps/api/internal/store

Non-vacuous — each case bites when the target is mutated (mutation applied to setup_code_defaults.go, suite re-run, then reverted):

Mutation to setup_code_defaults.go Result
utf8.RuneCountInString(value)len(value) (byte length) DefaultToLengthIsRuneCount fails
allowFrom count cap >>= AllowFromAtCapPasses fails
de-duplication disabled TrimsAndDedupes fails
agentActivity scope gate disabled AgentActivityRequiresScope fails
NUL check inverted (>= 0< 0) 6 tests fail
(reverted — control) 12/12 pass

gofmt -l reports the file clean; go vet ./internal/store/ exits 0; the full go test ./internal/store/ package is green with the file added.

Scope boundary: no production code changed, no new config/defaults, no dependency change — a single new _test.go under apps/api/internal/store. Maintainer edits to the branch are welcome.

Related: none — coverage mined from the module itself; no linked issue.

AI-assisted contribution.


Generated by Claude Code

@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. labels Jul 20, 2026
@clawsweeper

clawsweeper Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed July 20, 2026, 9:55 AM ET / 13:55 UTC.

Summary
The PR adds direct Go tests for NormalizeBotSetupCodeDefaults, covering normalization, boundary validation, de-duplication, and the agent_activity:write scope requirement.

Reproducibility: not applicable. This PR adds characterization coverage rather than fixing a separately reported user-visible failure. The supplied body includes direct test output, but it could not be independently run or inspected here.

Review metrics: 2 noteworthy metrics.

  • Patch surface: 1 file added, 162 lines added, 0 removed. The supplied metadata indicates a narrowly scoped test-only change.
  • Direct test cases: 12 reported validator cases. The PR body reports coverage of the main normalization and authorization gates.

Merge readiness
Overall: 🦐 gold shrimp
Proof: 🐚 platinum hermit
Patch quality: 🦐 gold shrimp
Result: needs maintainer review before merge.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • [P2] Make the PR ready for review after the full checkout/diff comparison and required checks complete.

Risk before merge

  • [P1] The test assertions may encode incidental behavior or omit existing test-helper conventions; this cannot be checked without reading the full diff and current store test suite.
  • [P1] The PR is a draft and several non-Go checks were still in progress in the supplied context, so merge readiness has not yet been established.

Maintainer options:

  1. Decide the mitigation before merge
    After the checkout is inspectable, compare the full test file with the current validator and nearby store tests, then land the focused regression suite if every assertion matches the existing public validation contract.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

  • [P2] No repair lane is appropriate: the supplied patch is already a focused implementation, but the mandatory independent source review could not complete in this environment.

Security
Cleared: Based on the supplied metadata, the patch adds only Go tests and no dependency, workflow, permissions, secret, package-resolution, or runtime execution changes.

Review details

Best possible solution:

After the checkout is inspectable, compare the full test file with the current validator and nearby store tests, then land the focused regression suite if every assertion matches the existing public validation contract.

Do we have a high-confidence way to reproduce the issue?

Not applicable: this PR adds characterization coverage rather than fixing a separately reported user-visible failure. The supplied body includes direct test output, but it could not be independently run or inspected here.

Is this the best way to solve the issue?

Unclear: direct tests of the exported validator are plausibly the narrowest solution, but the full test file and current implementation could not be compared because read-only checkout commands were unavailable.

AGENTS.md: unclear because the file could not be read completely.

Codex review notes: model internal, reasoning high; reviewed against c9cf5ba3691a.

Label changes

Label justifications:

  • P3: This is a low-risk test-coverage improvement with no reported production behavior change.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🐚 platinum hermit and patch quality is 🦐 gold shrimp.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The PR body supplies after-change terminal output for the targeted test suite and package checks; for this internal test-only change, that is directly relevant behavior proof.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body supplies after-change terminal output for the targeted test suite and package checks; for this internal test-only change, that is directly relevant behavior proof.
Evidence reviewed

What I checked:

  • PR scope from supplied GitHub context: The branch adds one test file with 162 lines and no production-code changes; the supplied PR body reports 12 direct validator cases and package validation output. (apps/api/internal/store/setup_code_defaults_test.go:1, 85018f9f9cb3)
  • Read-only checkout inspection unavailable: The required read-only command failed before producing repository content because the sandbox rejected loopback address setup (bwrap: loopback: Failed RTM_NEWADDR: Operation not permitted), so current-main source, full AGENTS.md, and git history could not be independently verified.

Likely related people:

  • unknown: The supplied context identifies the validator and its store paths, but checkout history inspection was unavailable, so no evidence-backed individual can be named. (role: likely current area owner; confidence: low; files: apps/api/internal/store/setup_code_defaults.go, apps/api/internal/store/sqlite/setup_codes.go, apps/api/internal/store/postgres/setup_codes.go)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@steipete
steipete marked this pull request as ready for review July 21, 2026 01:57
@steipete
steipete merged commit 82a4140 into openclaw:main Jul 21, 2026
9 checks passed
@steipete

Copy link
Copy Markdown
Contributor

Landed via rebase as 82a41403c931804101a3a968bb2dc32f823e5d02. Thanks, @KrasimirKralev.

Maintainer verification:

  • Confirmed the patch is test-only and still matches current main: one added _test.go file, no production, dependency, generated, workflow, docs, or release changes.
  • Compared coverage with and without the patch. Existing integration coverage reached 81.8% for NormalizeBotSetupCodeDefaults and 57.1% for validateBotSetupDefault; this suite raises both to 100%, covering previously unpinned UTF-8, NUL, length, entry-count, and pointer/scope branches.
  • Focused proof: go test ./internal/store -run '^TestNormalizeBotSetupCodeDefaults' -count=1 -v (12/12 passed) and go vet ./internal/store.
  • Full local proof on the candidate rebased onto current main: pnpm check passed, including production web/SDK builds, all Go packages, FakeCo policy tests, desktop contract tests, typechecks, lint, and formatting.
  • Canonical Codex AutoReview: clean, no accepted/actionable findings; patch assessed correct at 0.96 confidence.
  • Exact PR head 85018f9f9cb34b14790cd004ace5d6dc7aada3ac had all nine hosted checks green, including both Socket security checks.
  • Post-merge proof on the landed SHA is green: CI run 29794545603 (Go, TypeScript, Docker Image, Playwright E2E) and desktop run 29794545642 (macOS, Windows, Linux).

The landed tree exactly matches the reviewed rebased candidate. This is coverage-only, so no changelog entry was added.

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

Labels

P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. proof: sufficient Contributor real behavior proof is sufficient. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants