Refactor subscription-manager usage

[chmeliik]

See individual commits for more details. These are the changes necessary so that subscription-manager can also be used in other subcommands, not just prefetch. Plus a minor improvement to prefetch behavior.

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (4) 📘 Rule violations (0) 📎 Requirement gaps (0)

1. Unneeded subman init fails 🐞 Bug ≡ Correctness

Description

PrefetchDependencies.New() initializes SubscriptionManagerCli whenever RHSMOrg/RHSMActivationKey are
set, which can fail early if subscription-manager is not installed even when the run won’t process
RPM input (or when input is empty and prefetch exits early). This is a behavioral regression that
can break pipelines that always mount RHSM secrets but sometimes run without RPMs.

Code

pkg/commands/prefetch_dependencies/main.go[R81-87]

+	if local_config.RHSMOrg != "" && local_config.RHSMActivationKey != "" {
+		smCli, err := cliwrappers.NewSubscriptionManagerCli(executor)
+		if err != nil {
+			return nil, fmt.Errorf("RHSM registration requested but cannot be done: %w", err)
+		}
+		pd.SubscriptionManagerCli = smCli
+	}

Evidence

New() constructs SubscriptionManagerCli purely based on RHSM parameters, before Run() determines
whether RPMs are present; NewSubscriptionManagerCli() returns an error if the binary is not
available; CheckCliToolAvailable() returns (false, nil) when the binary is missing, making this
failure deterministic on hosts without subscription-manager.

pkg/commands/prefetch_dependencies/main.go[55-90]
pkg/commands/prefetch_dependencies/main.go[92-127]
pkg/cliwrappers/subscription_manager.go[26-35]
pkg/cliwrappers/cli_executor.go[137-145]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`prefetch-dependencies` currently constructs `SubscriptionManagerCli` in `New()` whenever RHSM params are present. This makes the command fail on systems without `subscription-manager` even if the input has no RPMs (or input is empty), because `NewSubscriptionManagerCli()` hard-fails when the binary is missing.

### Issue Context
`Run()` only needs RHSM registration inside the `containsRPM(decodedJSONInput)` path. Therefore the availability check/initialization should be deferred until RPM input is actually detected.

### Fix Focus Areas
- pkg/commands/prefetch_dependencies/main.go[55-90]
- pkg/commands/prefetch_dependencies/main.go[112-127]

### Suggested fix
Move `NewSubscriptionManagerCli(executor)` into the RPM path (e.g., inside `registerRHSM()` or directly under `if containsRPM(...) && registerRHSM { ... }`). Ensure `pd.SubscriptionManagerCli` is non-nil before calling `Register/Unregister` and only perform tool availability checks when RPM registration is actually required.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. RHSM file-read cause lost 🐞 Bug ◔ Observability

Description

PrefetchDependencies.registerRHSM() drops the underlying os.ReadFile error and returns a new
string-only error, losing the root cause (e.g., permission denied vs missing file). This makes RHSM
registration failures significantly harder to debug.

Code

pkg/commands/prefetch_dependencies/main.go[R175-182]

+	org, err := os.ReadFile(pd.Config.RHSMOrg)
+	if err != nil {
+		return fmt.Errorf("failed to read %s file", pd.Config.RHSMOrg)
+	}
+	key, err := os.ReadFile(pd.Config.RHSMActivationKey)
+	if err != nil {
+		return fmt.Errorf("failed to read %s file", pd.Config.RHSMActivationKey)
+	}

Evidence

registerRHSM() returns fmt.Errorf messages that do not include the original err, so callers cannot
see the real failure reason and the error chain cannot be inspected/wrapped meaningfully.

pkg/commands/prefetch_dependencies/main.go[174-182]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`registerRHSM()` returns errors like `fmt.Errorf("failed to read %s file", path)` without including the underlying `os.ReadFile` error, which discards critical debugging context.

### Issue Context
This function is the only place RHSM org/key file content is read before calling subscription-manager.

### Fix Focus Areas
- pkg/commands/prefetch_dependencies/main.go[174-182]

### Suggested fix
Change both error returns to wrap the underlying error, e.g.:
- `return fmt.Errorf("failed to read %s file: %w", pd.Config.RHSMOrg, err)`
- `return fmt.Errorf("failed to read %s file: %w", pd.Config.RHSMActivationKey, err)`

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Nil Register params panic 🐞 Bug ☼ Reliability

Description

SubscriptionManagerCli.Register() dereferences params without a nil check, so a nil call will
panic at runtime. Since this method is exposed via an interface, it’s a sharp edge for future
callers and tests/mocks.

Code

pkg/cliwrappers/subscription_manager.go[R38-43]

+func (sm *SubscriptionManagerCli) Register(params *SubscriptionManagerRegisterParams) error {
+	args := []string{"register"}
+	if params.Force {
+		args = append(args, "--force")
+	}
+	args = append(args, "--org", params.Org, "--activationkey", params.ActivationKey)

Evidence

Register() reads params.Force, params.Org, and params.ActivationKey unconditionally; passing
nil will crash.

pkg/cliwrappers/subscription_manager.go[37-44]
pkg/cliwrappers/subscription_manager.go[11-20]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`SubscriptionManagerCli.Register(nil)` will panic because the implementation dereferences `params` without validation.

### Issue Context
`Register` is part of a public interface (`SubscriptionManagerCliInterface`), so defensive checks help prevent runtime crashes from accidental misuse.

### Fix Focus Areas
- pkg/cliwrappers/subscription_manager.go[37-44]

### Suggested fix
Add an early check:
```go
if params == nil {
   return errors.New("subscription-manager register params cannot be nil")
}
```
Optionally also validate required fields (Org/ActivationKey not empty) and return a clear error.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

4. Register error not wrapped 🐞 Bug ⚙ Maintainability

Description

On failure, SubscriptionManagerCli.Register() returns a fresh generic error instead of wrapping the
underlying failure from retryer.Run(), preventing programmatic inspection (errors.Is/As) and losing
error type/context in the returned value. Logging helps humans, but callers that only propagate the
error still lose the causal chain.

Code

pkg/cliwrappers/subscription_manager.go[R49-57]

+	retryer := NewRetryer(command).StopIfOutputContains("unauthorized")
+	_, stderr, _, err := retryer.Run()
+	if err != nil {
+		submanLog.Errorf("subscription-manager register failed: %s", err.Error())
+		if stderr != "" {
+			submanLog.Errorf("stderr:\n%s", stderr)
+		}
+		return errors.New("subscription-manager register command failed")
+	}

Evidence

The returned error is created via errors.New(...), so it cannot retain the underlying error from
retryer.Run() and cannot be unwrapped by callers.

pkg/cliwrappers/subscription_manager.go[49-57]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`Register()` discards the underlying error returned by the retryer and returns a new generic error. This prevents callers from inspecting/root-causing failures via wrapped errors.

### Issue Context
The code already logs stderr and the raw error; improving the returned error helps callers that need structured handling and improves higher-level error messages.

### Fix Focus Areas
- pkg/cliwrappers/subscription_manager.go[49-57]

### Suggested fix
Return a wrapped error:
```go
return fmt.Errorf("subscription-manager register command failed: %w", err)
```
If you want to avoid leaking sensitive details, keep stderr logging but still wrap `err` (which should not include the activation key, since it’s a CLI arg).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[chmeliik]

Manually tested the relevant scenarios with variations of:

sudo env PATH=/home/acmiel/RedHat/Hermeto/hermeto/venv/bin:$PATH \
  ./konflux-build-cli prefetch-dependencies --input rpm --rhsm-activation-key activationkey.txt --rhsm-org org.txt

Pre-registration and unregistration still work as expected, stderr logging also works as expected

ERRO[0004] subscription-manager register failed: exit status 8  logger=SubscriptionManagerCli
ERRO[0004] stderr:
Error: this command requires root access to execute  logger=SubscriptionManagerCli

[chmeliik]

Except for "Nil Register params panic", all of Qodo's concerns were valid (though two of them weren't problems introduced by these changes). Addressed the three relevant ones.

With regards to "Nil Register params panic", it is technically true but it's a pattern used by all the CLI wrappers.