Skip to content

feat: add kosli attest decision command (hidden/BETA)#912

Merged
jumboduck merged 5 commits into
mainfrom
feat/attest-decision
May 27, 2026
Merged

feat: add kosli attest decision command (hidden/BETA)#912
jumboduck merged 5 commits into
mainfrom
feat/attest-decision

Conversation

@jumboduck
Copy link
Copy Markdown
Contributor

Summary

  • Adds kosli attest decision command (hidden, BETA) per kosli-dev/server#5695
  • Records a compliance decision against a control on a Kosli trail, posting to POST /api/v2/attestations/{org}/{flow}/trail/{trail}/system
  • Payload: type_name=decision, control at top level, is_compliant nested under attestation_data
  • Adds CreateControl test helper (direct POST /api/v2/controls/{org}) for test setup

Test plan

  • Error cases: missing --name, --control, --compliant; invalid fingerprint; bad annotation key; invalid name dot-format; too many args
  • Happy path: compliant and non-compliant decisions against a trail
  • Happy path: decision linked to artifact by --fingerprint
  • Happy path: with --attachments, --description, --annotate, --user-data
  • kosli attest decision --help reflects all flags correctly
  • Command is hidden from kosli attest --help

🤖 Generated with Claude Code

jumboduck and others added 5 commits May 27, 2026 10:49
Implements `kosli attest decision` per kosli-dev/server#5695.
Records a compliance decision against a control on a trail, posting
to the /api/v2/attestations/{org}/{flow}/trail/{trail}/system endpoint
with type_name="decision". The command is hidden (BETA).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The /system endpoint requires control and is_compliant nested under
attestation_data rather than at the top-level payload, matching the
server's schema validation.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Move control/is_compliant back to top-level payload fields; keep
  attestation_data as a required-but-empty field (reserved for future
  structured inputs per server schema)
- Fix flag bindings to point to top-level payload fields
- Add CreateControl helper to testHelpers.go (POST /api/v2/controls/{org})
- Call CreateControl in SetupTest so RCTL-043 exists before attesting

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…evel

The server schema requires control as a top-level field and is_compliant
nested under attestation_data.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 27, 2026

Claude finished @jumboduck's task in 2m 47s —— View job


PR Review: kosli attest decision (hidden/BETA)

  • Phase 1: Static analysis (go build, go vet, go mod tidy)
  • Phase 2: Code review (read changed files, check patterns)
  • Phase 3: Post review findings

Static Analysis

All three checks pass cleanly: go build ./..., go vet ./..., and go mod tidy produce no errors or diff.

Summary

Clean, well-structured PR that follows the established attestation command pattern closely. The code is modeled closely on attestGeneric and attestOverride, which makes it easy to review. Test coverage is thorough — error cases, happy paths with various flag combinations, and edge cases like invalid annotation keys and dot-format names are all covered.

Findings

Three inline comments posted — all low severity:

  1. API endpoint suffix (attestDecision.go:153) — Every other attestation command uses the type name as the URL suffix (/generic, /custom, /override, etc.), but this one posts to /system. Flagging in case this is unintentional — if the server route is deliberately system, then no issue.

  2. --compliant default value (attestDecision.go:142) — The flag defaults to false while attest generic defaults to true. Since the flag is effectively required (checked via cmd.Flags().Changed), this doesn't affect runtime behavior, but the --help output could be misleading. Minor nit for a BETA command.

  3. CreateControl idempotency (testHelpers.go:521-540) — The new test helper does a raw POST, while other helpers use command run() methods that tend to be idempotent. If SetupTest() runs multiple times and the server rejects duplicate controls, tests could fail. Worth verifying.

What's done well

  • Follows the existing attestation command conventions precisely (struct layout, PreRunE validation order, run() method structure)
  • The DecisionAttestationData nested struct cleanly separates decision-specific fields under attestation_data in the JSON payload
  • Good use of Hidden: true for the BETA phase
  • Test suite covers a solid mix of error paths and happy paths including attachments, annotations, descriptions, user-data, and fingerprint binding
  • CreateControl helper is a useful addition to the test infrastructure

Comment thread cmd/kosli/attestDecision.go
Comment thread cmd/kosli/attestDecision.go
Comment thread cmd/kosli/testHelpers.go
Comment on lines 521 to 540
}
}

// CreateControl creates a control in the org via the API.
func CreateControl(org, identifier, name string, t *testing.T) {
t.Helper()
u, err := url.JoinPath(global.Host, "api/v2/controls", org)
require.NoError(t, err, "control URL should be constructed without error")

reqParams := &requests.RequestParams{
Method: http.MethodPost,
URL: u,
Payload: map[string]string{"identifier": identifier, "name": name},
Token: global.ApiToken,
}
_, err = kosliClient.Do(reqParams)
require.NoError(t, err, "control should be created without error")
}

// CreatePolicy creates a policy on the server
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: the other helpers like CreateFlow / BeginTrail use command run() methods, which are idempotent on the server side (PUT semantics or "create if not exists"). This helper does a raw POST /api/v2/controls/{org}, which may return an error if a control with this identifier already exists — e.g., if SetupTest() runs multiple times in the same test process.

If the server's control endpoint is idempotent on re-creation, this is fine. Otherwise, consider either checking for existence first, or ignoring "already exists" errors. Worth verifying against the server.

@jumboduck jumboduck merged commit 419e203 into main May 27, 2026
20 checks passed
@jumboduck jumboduck deleted the feat/attest-decision branch May 27, 2026 09:47
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