Summary

Code review of the gateway resource management commands added in #213 surfaced several bugs, inconsistencies, and maintainability improvements. Grouped by priority below.

Bugs

1. --disabled flag always sends disabled=false to the API

Severity: High
Files: source_list.go:67-71, destination_list.go:67-71, source_count.go, destination_count.go

When --disabled is not passed, the code still sends disabled=false as a query parameter:

if sc.disabled {
    params["disabled"] = "true"
} else {
    params["disabled"] = "false"  // ← always sent, even when flag is omitted
}

The intent of omitting --disabled is "don't filter by disabled status", but this actively filters out disabled resources. The parameter should only be sent when the flag is explicitly set.

Fix: Use cmd.Flags().Changed("disabled") to detect whether the user explicitly passed the flag.

2. Fragile 404 detection via string matching in resolve*ID functions

Severity: High
Files: source_get.go:101-104, destination_get.go:107-108, transformation_get.go:99-100

errMsg := strings.ToLower(err.Error())
if !strings.Contains(errMsg, "404") && !strings.Contains(errMsg, "not found") {
    return "", err
}

If the API ever changes its error format (structured error, different wording), this silently breaks and falls through to the name-lookup path instead of returning the real error.

Fix: Add a structured error type in the client package (e.g. hookdeck.IsNotFoundError(err)) and use that instead of string matching.

3. Missing ValidateAPIKey in connection_enable.go (and likely connection_disable.go)

Severity: Medium
Files: connection_enable.go:30-31

Every other command calls Config.Profile.ValidateAPIKey() before making API calls. connectionEnableCmd skips this check entirely.

4. event list text output shows raw WebhookID field

Severity: Medium
Files: event_list.go:199

fmt.Printf("%s %s %s\n", color.Green(e.ID), e.Status, e.WebhookID)

User-facing terminology should be "Connection ID" not the raw struct field WebhookID. The output should label it accordingly (e.g. Connection: web_xxx).

Inconsistencies

5. Mixed checkmark characters across commands

Severity: Low

Create/update/upsert commands use ✔ (U+2714):

• source_create.go:122, destination_create.go:147, transformation_create.go:106, etc.

Enable/disable commands use ✓ (U+2713):

• source_enable.go:43, destination_enable.go:43, connection_enable.go:44, etc.

Fix: Pick one and define it as a constant in helptext.go.

6. Redundant nil check in resolveSourceID

Severity: Low
Files: source_get.go:112

if result.Models == nil || len(result.Models) == 0 {

len(nil) is 0 in Go, so the nil check is redundant. destination_get.go:118 correctly uses just len(result.Models) == 0.

7. Delete/enable/disable commands only accept IDs, not names

Severity: Low
Files: source_delete.go, source_enable.go, destination_delete.go, destination_enable.go, etc.

get commands support <id-or-name> via the resolve*ID functions, but delete, enable, and disable only accept bare IDs. This is a UX inconsistency — users can get my-source but must delete src_abc123.

Code Quality / Maintainability

8. Significant boilerplate across resource types

Severity: Medium

List commands (source_list.go, destination_list.go, event_list.go, etc.) follow an identical pattern: build params → call client → branch on --output json → format text. Enable/disable commands are nearly identical ~45-line files with only the resource type swapped.

A factory function like makeEnableCmd(resource, clientFn) would eliminate ~200 lines of duplication.

9. Hardcoded magic strings in source_common.go

Severity: Medium
Files: source_common.go:62-64,76,81

Default auth config values like "sha256", "hex", "x-webhook-signature", "x-api-key" are duplicated across buildSourceConfigFromIndividualFlags and normalizeSourceConfigAuth.

Fix: Extract to named constants.

10. event_list has 25+ filter flags mapped manually

Severity: Low
Files: event_list.go:100-172

~70 lines of if field != "" { params[key] = field }. A helper like addNonEmpty(params, "key", value) or struct tags would be cleaner and less error-prone.

11. --connection-id maps to API param webhook_id

Severity: Low
Files: event_list.go:104

params["webhook_id"] = ec.connectionID

The mismatch between the user-facing flag name and the API parameter name is confusing. At minimum, add a comment. Ideally define a constant APIParamConnectionID = "webhook_id".

Missing Tests

12. No tests for new resource commands

Severity: Medium

The PR added ~50 new command files but only 2 new test files. Destination, transformation, event, request, and attempt commands have no unit tests. Bugs #1 and #2 above are exactly the kind of issues tests would have caught.

Suggested Priority

1. Bug fix: taking cli_path from attempt request instead of destination #1 — disabled=false always sent (silently hides disabled resources)
2. Bug Feat/unauthenticated user #3 — Missing ValidateAPIKey in connection enable/disable
3. Bug fix: Fix to send data in string_data in case the content-type is text/plain or x-wwww-form-urlencoded #4 — WebhookID shown in event list text output
4. Bug feat: logout command & creating guest access when calling listen #2 — Structured 404 error checking
5. docker login #5 — Normalize checkmark character
6. #12 — Add test coverage

From code review of #213