feat(cfl): migrate config diagnostics to presenters for #271 by rianjs · Pull Request #443 · open-cli-collective/atlassian-cli

rianjs · 2026-06-24T21:43:27Z

Summary

Move config test diagnostics into ConfigPresenter output models.
Move config clear preview, warning, cancellation, no-op, success, and env-note output into presenter-owned models while preserving direct prompt text as the documented exception.
Add exact presenter/command tests and a redacted live proof note for feat(cfl): migrate diagnostics and advisory output to presenters for #271 #436.

Closes #436

Verification

rtk go test ./tools/cfl/internal/cmd/configcmd ./tools/cfl/internal/present
rtk go test ./tools/cfl/internal/present ./tools/cfl/internal/cmd/configcmd ./tools/cfl/internal/cmd/page ./tools/cfl/internal/cmd/root ./shared/present
rtk golangci-lint run ./tools/cfl/internal/present ./tools/cfl/internal/cmd/configcmd ./tools/cfl/internal/cmd/page ./tools/cfl/internal/cmd/root
rtk go test ./tools/cfl/... ./shared/...
rtk proxy git diff --check
grep gates documented in docs/proofs/271-436-cfl-diagnostics-advisories.md
live bin/cfl --no-color config test and bin/cfl --no-color --non-interactive config clear transcript documented with identity fields redacted

Closes #436

rianjs · 2026-06-24T21:52:48Z

No functional blockers in the refactor itself, but I see a few coverage gaps that leave room for regressions:

config clear env-note on successful deletion is untested. The new joinWithEnvNote path is only exercised for the no-op branch, so a regression that drops the environment override note after a successful clear or clear --all would still pass the suite. See tools/cfl/internal/present/config.go:108, tools/cfl/internal/present/config.go:115, tools/cfl/internal/cmd/configcmd/clear_test.go:54, and tools/cfl/internal/cmd/configcmd/clear_test.go:138.
The --all keyring-open failure advisory is presenter-only tested. PresentClearAllPlan has special behavior when PlanClear returns an error, but there is no command-level test that exercises runClear(..., all=true) with that error path. A wiring bug in runClear could therefore stay green. See tools/cfl/internal/present/config.go:77 and tools/cfl/internal/present/config_test.go:69.
The grep gates are documented, not enforced. The proof note shows the rg checks, but because they live only in docs/proofs/271-436-cfl-diagnostics-advisories.md, a future reintroduction of command-local diagnostic strings could still pass the test suite as long as output stays the same. If the migration contract matters, this should be backed by an executable check. See docs/proofs/271-436-cfl-diagnostics-advisories.md:87.

monit-reviewer

Automated PR Review

Reviewed commit: 9463246

Approved with 5 non-blocking suggestions below. Address at your discretion.

Summary

Reviewer	Findings
documentation:docs-reviewer	2
harness-engineering:harness-enforcement-reviewer	1
harness-engineering:harness-knowledge-reviewer	1
harness-engineering:harness-self-documenting-code-reviewer	1

documentation:docs-reviewer (2 findings)

💡 Suggestion - docs/proofs/271-436-cfl-diagnostics-advisories.md:160

The live proof references a temporary directory '/tmp/cfl-436-proof.u3byyu' that does not persist. While ephemeral directories are expected for live transcripts, a brief note clarifying that the path is intentionally ephemeral would help future readers understand why the artifact is unverifiable after the fact.

💡 Suggestion - docs/proofs/271-436-cfl-diagnostics-advisories.md:27

Ticket #435 is referenced ('contains the exact command and live proof for the legacy-editor advisory') but no link is provided. Readers cannot navigate to the associated proof without knowing where to find it.

harness-engineering:harness-enforcement-reviewer (1 findings)

💡 Suggestion - tools/cfl/internal/cmd/configcmd/clear_test.go:185

TestConfigDiagnosticsPresenterBoundaryGrepGate hardcodes specific filenames ('test.go', 'clear.go') rather than walking all non-test Go files in the configcmd package. A new command file added to configcmd that directly emits diagnostic strings would not be covered by this gate. Consider using filepath.Glob("*.go") filtered to exclude test files so the boundary is enforced across the whole package automatically.

harness-engineering:harness-knowledge-reviewer (1 findings)

💡 Suggestion - docs/proofs/271-436-cfl-diagnostics-advisories.md:15

The rule that all diagnostic/status output must be presenter-owned — with one-shot confirmation prompt text as the sole direct-stderr exception — is a project-wide architectural convention, but it is captured only in this ticket-tied proof file. If no durable architecture doc covers the presenter pattern (e.g., a note in the shared presenter package or a dedicated architecture doc), this convention risks being invisible to future contributors writing new commands.

harness-engineering:harness-self-documenting-code-reviewer (1 findings)

💡 Suggestion - tools/cfl/internal/present/config.go:42

PresentTestConnectionSuccess returns stderrOnly("success!\n") with an embedded newline whose blank-line intent (separator before the auth-details block) is implicit and requires tracing the rendering contract to understand. Other methods in the same file use explicit empty strings in a joined slice for internal blank lines (e.g., PresentTestFailure). Using strings.Join([]string{"success!", ""}, "\n") would make the blank-line intent self-evident and consistent with the pattern used elsewhere.

1 PR discussion thread considered.

Completed in 3m 45s | $2.18 | sonnet | daemon 0.2.132 | Glorfindel

Field	Value
Model	sonnet
Reviewers	hybrid-synthesis, documentation:docs-reviewer, harness-engineering:harness-architecture-reviewer, harness-engineering:harness-enforcement-reviewer, harness-engineering:harness-knowledge-reviewer, harness-engineering:harness-self-documenting-code-reviewer, security:security-code-auditor
Engine	claude · sonnet
Reviewed by	pr-review-daemon · monit-pr-reviewer
Duration	3m 45s wall · 3m 42s compute (Reviewers: 2m 50s · Synthesis: 52s)
Cost	$2.18 (estimated)
Tokens	389.4k in / 24.3k out
Turns	14

Per-workstream usage

Workstream	Model	In	Out	Cache read	Cache create	Cost
hybrid-synthesis	sonnet	38.0k	2.6k	13.1k	24.9k (1h)	$0.19
documentation:docs-reviewer	sonnet	36.2k	1.6k	13.1k	23.1k (1h)	$0.17
harness-engineering:harness-architecture-reviewer	sonnet	61.2k	1.0k	13.1k	48.0k (1h)	$0.31
harness-engineering:harness-enforcement-reviewer	sonnet	63.8k	3.3k	13.1k	50.7k (1h)	$0.36
harness-engineering:harness-knowledge-reviewer	sonnet	64.4k	4.0k	13.1k	51.3k (1h)	$0.37
harness-engineering:harness-self-documenting-code-reviewer	sonnet	67.7k	10.3k	13.1k	54.5k (1h)	$0.49
security:security-code-auditor	sonnet	58.0k	1.5k	13.1k	44.8k (1h)	$0.30

_{Re-reviews only run when @monit-reviewer is re-requested as a reviewer — push as many commits as you need, then re-request when ready. PRs targeting branches other than main, master are skipped, even when @monit-reviewer is re-requested.}

monit-reviewer · 2026-06-24T21:59:50Z

+
+## Live CLI Transcript
+
+The binary was rebuilt:


🔵 Low (documentation:docs-reviewer): The live proof references a temporary directory '/tmp/cfl-436-proof.u3byyu' that does not persist. While ephemeral directories are expected for live transcripts, a brief note clarifying that the path is intentionally ephemeral would help future readers understand why the artifact is unverifiable after the fact.

Reply to this thread when addressed.

monit-reviewer · 2026-06-24T21:59:50Z

+
+## Verification Commands
+
+Executed:


🔵 Low (documentation:docs-reviewer): Ticket #435 is referenced ('contains the exact command and live proof for the legacy-editor advisory') but no link is provided. Readers cannot navigate to the associated proof without knowing where to find it.

Reply to this thread when addressed.

monit-reviewer · 2026-06-24T21:59:50Z

+	testutil.Contains(t, err.Error(), "plaintext artifacts were cleaned")
+	testutil.Contains(t, err.Error(), "keyring bundle")
+	_, statErr := os.Stat(sharedPath)
+	testutil.True(t, os.IsNotExist(statErr))


🔵 Low (harness-engineering:harness-enforcement-reviewer): TestConfigDiagnosticsPresenterBoundaryGrepGate hardcodes specific filenames ('test.go', 'clear.go') rather than walking all non-test Go files in the configcmd package. A new command file added to configcmd that directly emits diagnostic strings would not be covered by this gate. Consider using filepath.Glob("*.go") filtered to exclude test files so the boundary is enforced across the whole package automatically.

Reply to this thread when addressed.

monit-reviewer · 2026-06-24T21:59:50Z

+- orchestrate config, API, keyring, and prompt control flow only
+- emit presenter-owned diagnostic/status models through
+  `tools/cfl/internal/present/config.go`
+- preserve `config test` progress timing through an explicit no-newline


🔵 Low (harness-engineering:harness-knowledge-reviewer): The rule that all diagnostic/status output must be presenter-owned — with one-shot confirmation prompt text as the sole direct-stderr exception — is a project-wide architectural convention, but it is captured only in this ticket-tied proof file. If no durable architecture doc covers the presenter pattern (e.g., a note in the shared presenter package or a dedicated architecture doc), this convention risks being invisible to future contributors writing new commands.

Reply to this thread when addressed.

monit-reviewer · 2026-06-24T21:59:50Z

+}
+
+func (ConfigPresenter) PresentTestConnectionSuccess() *sharedpresent.OutputModel {
+	return stderrOnly("success!\n")


🔵 Low (harness-engineering:harness-self-documenting-code-reviewer): PresentTestConnectionSuccess returns stderrOnly("success!\n") with an embedded newline whose blank-line intent (separator before the auth-details block) is implicit and requires tracing the rendering contract to understand. Other methods in the same file use explicit empty strings in a joined slice for internal blank lines (e.g., PresentTestFailure). Using strings.Join([]string{"success!", ""}, "\n") would make the blank-line intent self-evident and consistent with the pattern used elsewhere.

Reply to this thread when addressed.

rianjs added 3 commits June 24, 2026 17:43

Migrate config diagnostics to presenters

8a98d0c

Closes #436

Preserve config test progress timing

1261a3a

Split config test connection success output

6cc4838

Cover config clear advisory edge cases

9463246

monit-reviewer approved these changes Jun 24, 2026

View reviewed changes

rianjs marked this pull request as ready for review June 24, 2026 22:01

rianjs merged commit 6a70902 into main Jun 24, 2026
10 checks passed

rianjs deleted the feat/436-cfl-diagnostic-advisory-presenters branch June 24, 2026 22:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat(cfl): migrate config diagnostics to presenters for #271#443

feat(cfl): migrate config diagnostics to presenters for #271#443
rianjs merged 4 commits into
mainfrom
feat/436-cfl-diagnostic-advisory-presenters

rianjs commented Jun 24, 2026

Uh oh!

rianjs commented Jun 24, 2026

Uh oh!

monit-reviewer left a comment

Uh oh!

monit-reviewer Jun 24, 2026

Uh oh!

monit-reviewer Jun 24, 2026

Uh oh!

monit-reviewer Jun 24, 2026

Uh oh!

monit-reviewer Jun 24, 2026

Uh oh!

monit-reviewer Jun 24, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

rianjs commented Jun 24, 2026

Summary

Verification

Uh oh!

rianjs commented Jun 24, 2026

Uh oh!

monit-reviewer left a comment

Choose a reason for hiding this comment

Automated PR Review

Summary

Uh oh!

monit-reviewer Jun 24, 2026

Choose a reason for hiding this comment

Uh oh!

monit-reviewer Jun 24, 2026

Choose a reason for hiding this comment

Uh oh!

monit-reviewer Jun 24, 2026

Choose a reason for hiding this comment

Uh oh!

monit-reviewer Jun 24, 2026

Choose a reason for hiding this comment

Uh oh!

monit-reviewer Jun 24, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants