fix(#12): correct elevate command in service login prompt

[fullsend-ai-coder]

The service login prompt printed an incorrect elevate command with two issues: it used 'ocm backplane' instead of the shipped binary name 'ocm-backplane', and it was missing the -n flag (--no-reason) that prompts the user for an elevation reason.

Fix the format string to use the correct binary name and include the -n flag. Add a test that captures stdout during service login and asserts the correct command format is printed.

---

Closes #12

Post-script verification

• Branch is not main/master (agent/12-fix-elevate-command)
• Secret scan passed (gitleaks — ae64d7cf510a866657b8dcb2881b8c7dc3fe2a16..HEAD)
• Pre-commit hooks passed (authoritative run on runner)
• Tests ran inside sandbox

[samanthajayasinghe]

/fs-review

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 2:34 AM UTC · Completed 2:40 AM UTC
Commit: ae64d7c · View workflow run →

[fullsend-ai-review]

Review — PR #13: fix(#12): correct elevate command in service login prompt

Verdict: request-changes

Summary

This PR fixes the elevate command printed during service login (--service flag) by changing the binary name from ocm backplane to ocm-backplane and adding the -n flag for the no-reason prompt. While the intent is correct and matches issue #12, the fix introduces a new correctness bug: the oc token after -- is no longer consumed as the reason argument (because -n now prepends an empty string as the reason), causing the resulting command to execute oc oc get manifestworks... which will fail.

Findings

1. 🔴 HIGH — Double oc in the elevate command causes execution failure

File: cmd/ocm-backplane/login/login.go, line 291

The elevate command's RunElevate() function in pkg/elevate/elevate.go uses exec.Command("oc", argv[1:]...) to execute the elevated command. This means oc is always prepended internally — the arguments after -- should be the oc subcommand (e.g., get, patch), not oc get.

Old command (before this PR):

ocm backplane elevate -- oc get manifestworks -n <ns> -l ...

This accidentally worked because oc was consumed as argv[0] (the reason), and get manifestworks -n <ns> ... became argv[1:], producing the correct exec.Command("oc", "get", "manifestworks", "-n", "<ns>", ...).

New command (this PR):

ocm-backplane elevate -n -- oc get manifestworks -n <ns> -l ...

With -n, an empty string is prepended as argv[0] (the reason). Now oc is argv[1], producing exec.Command("oc", "oc", "get", "manifestworks", ...) → oc oc get manifestworks..., which fails with `"unknown command 'oc'".

This is confirmed by all documented examples in the repo:

• CONTRIBUTING.md:140: ocm backplane elevate -n -- get po -A (no oc after --)
• README.md:359: ocm-backplane elevate -n -- get secret xxx (no oc after --)

Remediation: Remove oc from the command string after --:

listManifestWork := fmt.Sprintf("ocm-backplane elevate -n -- get manifestworks -n %s -l api.openshift.com/id=%s", managingClusterName, targetClusterID)

2. 🟡 LOW — Test assertions don't catch the double-oc bug

File: cmd/ocm-backplane/login/login_test.go, line 427–428

The test checks:

Expect(output).To(ContainSubstring("ocm-backplane elevate -n --"))
Expect(output).NotTo(ContainSubstring("ocm backplane elevate --"))

These substring checks verify the binary name and flag presence but don't detect the oc duplication. A more thorough assertion would verify the full command pattern:

Expect(output).To(ContainSubstring("ocm-backplane elevate -n -- get manifestworks"))
Expect(output).NotTo(ContainSubstring("-- oc "))

3. 🟡 LOW — Test doesn't guard os.Stdout restoration on failure

File: cmd/ocm-backplane/login/login_test.go, lines 413–422

The test captures stdout by replacing os.Stdout with a pipe, but restoration (os.Stdout = old) is not deferred. If runLogin or io.ReadAll panics, os.Stdout remains pointed at the closed pipe, breaking all subsequent test output. Using defer for restoration is safer:

old := os.Stdout
defer func() { os.Stdout = old }()

Additionally, the error from os.Pipe() is silently discarded.

---

Labels: PR fixes a bug in the service login command output

[fullsend-ai-review]

See the review comment for full details.

[fullsend-ai-review]

[high] correctness

The printed elevate command includes 'oc' after '--', but with the '-n' flag, 'oc' is no longer consumed as the reason argument. The elevate command internally prepends 'oc' via exec.Command("oc", argv[1:]...), so the resulting execution would be 'oc oc get manifestworks...' which fails. All documented examples (CONTRIBUTING.md, README.md) confirm that 'oc' should NOT appear after '--'.

Suggested fix: Remove 'oc' from the format string: fmt.Sprintf("ocm-backplane elevate -n -- get manifestworks -n %s -l api.openshift.com/id=%s", managingClusterName, targetClusterID)

[fullsend-ai-review]

[low] test-quality

The test assertions only check for substring presence of 'ocm-backplane elevate -n --' and absence of 'ocm backplane elevate --', but don't detect the double-'oc' bug. The assertions should verify the full command pattern including what follows '--'.

Suggested fix: Add assertion: Expect(output).To(ContainSubstring("-- get manifestworks")) and Expect(output).NotTo(ContainSubstring("-- oc "))

[fullsend-ai-review]

[low] test-quality

os.Stdout is reassigned without a deferred restore. If runLogin or io.ReadAll panics, os.Stdout remains pointed at a closed pipe, corrupting subsequent test output. The os.Pipe() error is also silently discarded.

Suggested fix: Use 'defer func() { os.Stdout = old }()' immediately after saving the old value, and check the error from os.Pipe().