Skip to content

feat(agent): integrate praxis-harness TUI + headless behind experimental flag - #65

Open
vishnukv-facets wants to merge 4 commits into
mainfrom
feat/praxis-harness-integration
Open

feat(agent): integrate praxis-harness TUI + headless behind experimental flag#65
vishnukv-facets wants to merge 4 commits into
mainfrom
feat/praxis-harness-integration

Conversation

@vishnukv-facets

@vishnukv-facets vishnukv-facets commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Summary

Integrates the praxis-harness coding agent (TUI + headless runner) into praxis-cli as a Go module dependency. Both commands are gated behind PRAXIS_EXPERIMENTAL=1 or --experimental and disabled by default.

New commands

praxis chat --experimental          # interactive Bubble Tea TUI coding agent
praxis run --experimental --prompt "fix the bug"   # headless one-shot

Without the flag:

$ praxis chat
Error: agent commands are experimental; set PRAXIS_EXPERIMENTAL=1 or pass --experimental to enable

Architecture

praxis (single binary)
├── Cobra CLI layer (login, mcp, status, agents, duty, ...)  ← existing
└── Agent layer (experimental)
    ├── praxis chat → tui.Run(ctx, Config) via internal/agent bridge
    └── praxis run  → native.Run(ctx, argv) via internal/agent bridge
  • internal/agent/ — the single import boundary between praxis-cli and praxis-harness. Maps cobra flags → harness tui.Config / native.Run argv.
  • Two-layer auth stays separate: ~/.praxis/credentials (control-plane) + ~/.praxis/agent/auth.json (LLM providers). The TUI's /login handles provider auth.
  • Module dependency, not monorepo: go.mod has a replace directive for local dev; CI will use the published tag once PR feat(login)!: replace localhost callback with server-mediated poll #5 merges.

Changes

  • go.mod: Go 1.25.0, praxis-harness dep (local replace for dev)
  • internal/agent/agent.go + agent_test.go: bridge package
  • cmd/chat.go: praxis chat with --experimental gate
  • cmd/run.go: praxis run with --experimental gate
  • .github/workflows/ci.yml + release.yml: Go 1.25

Verification

  • go build
  • go vet
  • gofmt -l clean ✅
  • go test ./... — 527 tests pass ✅
  • Smoke: praxis chat without flag → experimental gate error ✅
  • Smoke: praxis chat --help → full help with flags ✅

Summary by CodeRabbit

  • New Features
    • Added praxis chat --agents to launch the session dashboard (instead of a single interactive session), with updated help text.
    • Added experimental praxis run for headless prompt execution with extensive flag-based control (model, sessions, MCP, output JSON, and runtime limits).
  • Bug Fixes
    • Improved CLI validation for experimental agent enablement and clearer rejection of incompatible chat --agents flag combinations.
  • Chores
    • Updated CI/release workflows to Go 1.25 and configured private module access.
  • Tests
    • Added CLI and unit tests covering --agents behavior and argument mapping.

…tal flag

Adds praxis-harness as a Go module dependency and exposes two new cobra
commands gated behind PRAXIS_EXPERIMENTAL=1 or --experimental:

- praxis chat: interactive Bubble Tea TUI coding agent (tui.Run)
- praxis run: headless one-shot runner (native.Run)

The bridge lives in internal/agent/ — the single import boundary between
praxis-cli's cobra world and praxis-harness's SDK. It maps cobra flags to
the harness's tui.Config (via ParseFlags) and native.Run (via argv),
keeping the two repos decoupled.

Auth is two-layer and stays separate: control-plane credentials
(~/.praxis/credentials) for CLI gateway calls, LLM provider credentials
(~/.praxis/agent/auth.json) for the agent. The TUI's /login handles
provider auth.

Changes:
- go.mod: Go 1.25, praxis-harness dep (local replace for dev)
- internal/agent/: bridge package (ChatOptions, HeadlessArgs, flag mapping)
- cmd/chat.go: praxis chat cobra command with --experimental gate
- cmd/run.go: praxis run cobra command with --experimental gate
- .github/workflows: Go 1.25 in CI + release
- 10 unit tests for the bridge (flag mapping, experimental gate, logo)

Verified: go build, go vet, gofmt, 527 tests pass.
@coderabbitai

coderabbitai Bot commented Jul 29, 2026

Copy link
Copy Markdown

Review Change Stack

Walkthrough

The CLI adds an experimental chat --agents dashboard mode and a headless run command backed by praxis-harness, with agent option translation, validation tests, expanded dependencies, and Go 1.25 private-module workflow configuration.

Changes

Experimental agent integration

Layer / File(s) Summary
Go toolchain and private module configuration
.github/workflows/*, go.mod
CI and release workflows use Go 1.25 and configure private GitHub module access; go.mod updates praxis-harness and pins indirect dependencies.
Agent feature gate and runtime bridge
internal/agent/agent.go
The agent package gates experimental execution, supports dashboard startup, converts chat and headless options into harness arguments, and delegates to TUI and native runners.
Chat and headless command entry points
cmd/chat.go, cmd/run.go
Cobra parses dashboard and headless-run flags, validates incompatible chat flags, handles cancellation and enablement checks, and invokes agent execution.
Agent and CLI validation
internal/agent/agent_test.go, cmd/chat_test.go
Tests cover environment gating, argument conversion, native flags, dashboard help text, and mutual-exclusion validation.

Sequence Diagram(s)

sequenceDiagram
  participant User
  participant CobraCommand
  participant AgentPackage
  participant PraxisHarness
  User->>CobraCommand: praxis chat --agents or praxis run
  CobraCommand->>AgentPackage: Check enablement and pass options
  AgentPackage->>PraxisHarness: Run TUI or headless execution
  PraxisHarness-->>AgentPackage: Return error or exit code
  AgentPackage-->>CobraCommand: Return execution result
Loading

Suggested reviewers: anshulsao

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly captures the main change: integrating praxis-harness with experimental chat/run support.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/praxis-harness-integration

Comment @coderabbitai help to get the list of available commands.

@vishnukv-facets
vishnukv-facets marked this pull request as draft July 29, 2026 15:25

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 8

🧹 Nitpick comments (3)
internal/agent/agent_test.go (1)

8-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Prefer a table-driven environment-gate test.

These near-duplicate cases should be represented as table rows for clarity and easier coverage of additional values such as "0" and "false".

As per coding guidelines: “Use table-driven tests as the default pattern.”

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/agent/agent_test.go` around lines 8 - 58, Consolidate the
near-duplicate environment-gate tests around Enabled and CheckEnabled into
table-driven tests, using cases for unset, enabled values such as "1" and
"true", and disabled values such as "0" and "false". Preserve the existing
expected results and environment restoration for each case, while keeping the
CheckEnabled error-versus-nil assertions explicit.

Source: Coding guidelines

cmd/chat.go (1)

51-60: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Duplicated experimental-gate + signal-context boilerplate across chat and run. Both commands repeat the same "enable if flag set → CheckEnabled → print error → signal.NotifyContext + defer stop()" sequence; a shared helper would keep this consistent as more agent commands are added.

  • cmd/chat.go#L51-L60: extract this block into a shared helper (e.g., agent.SetupExperimental(cmd, experimentalFlag) (context.Context, func(), error)) that both commands call.
  • cmd/run.go#L54-L63: replace the identical block with a call to the same shared helper.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cmd/chat.go` around lines 51 - 60, The experimental-agent setup and
signal-context lifecycle are duplicated between the command blocks. In
cmd/chat.go lines 51-60, add a shared agent.SetupExperimental helper that
enables the agent when requested, checks availability, prints failures, and
returns the context, cleanup function, and error; replace the inline setup with
that helper. Apply the same replacement in cmd/run.go lines 54-63, preserving
each command’s existing error-return behavior.
cmd/run.go (1)

83-86: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Avoid os.Exit here os.Exit(exitCode) skips the deferred stop(). Return an exit-code error and translate it in cmd.Execute/main so cleanup still runs.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cmd/run.go` around lines 83 - 86, Replace the os.Exit call in the headless
execution path with an error carrying the returned exit code, allowing deferred
stop() cleanup to run. Update cmd.Execute or main to recognize and translate
this exit-code error into the process exit status while preserving successful
execution behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@cmd/chat.go`:
- Around line 50-82: Add focused direct tests in cmd/chat_test.go for the chat
command’s RunE, covering experimental gating, agent.CheckEnabled behavior, and
complete flag-to-agent.ChatOptions mapping; add cmd/run_test.go for run’s RunE,
specifically verifying exit-code handling. Cover both successful and relevant
failure paths while isolating command dependencies.

In `@cmd/run.go`:
- Around line 24-30: Update the run command’s option definitions and
output-selection logic to add a unified --json flag and automatically select
parseable JSON output when os.Stdout is not a TTY. Integrate this behavior with
the existing runResultJSON and runUsageJSON paths so explicit flags remain
supported while machine consumers receive JSON by default.
- Around line 46-50: Update the run command help examples in the command
definition near the Examples block to use valid long-form flags with double
hyphens: replace each single-hyphen option for experimental, prompt,
prompt-file, model, and result-json while preserving the example arguments and
behavior.

In `@go.mod`:
- Line 90: Remove the absolute local replacement from go.mod for
github.com/Facets-cloud/praxis-harness. Use a published harness module version,
and place any local development override in a developer-only go.work file, or
use a portable relative replacement with the required CI checkout configuration.

In `@internal/agent/agent_test.go`:
- Around line 8-11: Update the environment setup and cleanup in
TestEnabledDefaultOff and the similarly structured test blocks to preserve
whether experimentalEnvVar was originally unset. Use t.Setenv for value-based
cases, and use os.LookupEnv when saving the prior state so cleanup calls
os.Unsetenv if it was absent, otherwise restores its original value.
- Around line 43-46: Strengthen the error assertion in the CheckEnabled test by
verifying the expected disabled-state error type or exact message, using
errors.Is, errors.As, or a precise message comparison. Keep the existing nil
check behavior while asserting the returned error matches the documented
contract.
- Around line 99-124: Strengthen the test’s ToNativeArgs assertions by
validating each flag’s associated value and the -no-mcp boolean semantics, not
merely flag presence. Check the expected prompt, model, cwd, and max-turns
values using the generated argument ordering or per-flag lookup, while
preserving coverage that -no-mcp is emitted with the correct behavior.

In `@internal/agent/agent.go`:
- Around line 18-20: Update the praxis-harness replace directive in go.mod to
use a repository-relative path or a published module version instead of the
developer-local absolute path, so the internal/agent package builds consistently
across environments.

---

Nitpick comments:
In `@cmd/chat.go`:
- Around line 51-60: The experimental-agent setup and signal-context lifecycle
are duplicated between the command blocks. In cmd/chat.go lines 51-60, add a
shared agent.SetupExperimental helper that enables the agent when requested,
checks availability, prints failures, and returns the context, cleanup function,
and error; replace the inline setup with that helper. Apply the same replacement
in cmd/run.go lines 54-63, preserving each command’s existing error-return
behavior.

In `@cmd/run.go`:
- Around line 83-86: Replace the os.Exit call in the headless execution path
with an error carrying the returned exit code, allowing deferred stop() cleanup
to run. Update cmd.Execute or main to recognize and translate this exit-code
error into the process exit status while preserving successful execution
behavior.

In `@internal/agent/agent_test.go`:
- Around line 8-58: Consolidate the near-duplicate environment-gate tests around
Enabled and CheckEnabled into table-driven tests, using cases for unset, enabled
values such as "1" and "true", and disabled values such as "0" and "false".
Preserve the existing expected results and environment restoration for each
case, while keeping the CheckEnabled error-versus-nil assertions explicit.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: d56328c4-846f-4d86-adc3-65001004b8af

📥 Commits

Reviewing files that changed from the base of the PR and between 50972a7 and 5ed5818.

⛔ Files ignored due to path filters (1)
  • go.sum is excluded by !**/*.sum
📒 Files selected for processing (7)
  • .github/workflows/ci.yml
  • .github/workflows/release.yml
  • cmd/chat.go
  • cmd/run.go
  • go.mod
  • internal/agent/agent.go
  • internal/agent/agent_test.go

Comment thread cmd/chat.go
Comment on lines +50 to +82
RunE: func(cmd *cobra.Command, args []string) error {
if chatExperimental {
agent.Enable()
}
if err := agent.CheckEnabled(); err != nil {
render.PrintError(os.Stderr, false, err.Error(), "", 1)
return err
}

ctx, stop := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM)
defer stop()

opts := agent.ChatOptions{
Model: chatModel,
Thinking: chatThinking,
PermissionMode: chatPermission,
Cwd: chatCwd,
SessionID: chatSessionID,
SessionDir: chatSessionDir,
Resume: chatResume,
TeamName: chatTeamName,
SafeMode: chatSafe,
Ephemeral: chatEphemeral,
McpConfig: chatMcpConfig,
SettingsPath: chatSettings,
Profile: chatProfile,
FallbackModels: chatFallback,
Prompt: chatPrompt,
MaxTurns: chatMaxTurns,
}

return agent.RunChat(ctx, opts)
},

@coderabbitai coderabbitai Bot Jul 29, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Confirm no test files exist for the new commands
fd -g 'chat_test.go' -g 'run_test.go' cmd

Repository: Facets-cloud/praxis-cli

Length of output: 219


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf 'Top-level cmd files:\n'
git ls-files cmd | sort

printf '\nTest files under cmd:\n'
git ls-files 'cmd/*_test.go' | sort

printf '\nRelevant command files:\n'
for f in cmd/chat.go cmd/run.go; do
  if [ -f "$f" ]; then
    printf '\n== %s ==\n' "$f"
    wc -l "$f"
    sed -n '1,220p' "$f"
  fi
done

Repository: Facets-cloud/praxis-cli

Length of output: 9444


Add cmd/chat_test.go and cmd/run_test.go
These new cobra commands still lack direct tests for their RunE paths: experimental gating, flag→options mapping, and run’s exit-code handling. Add focused command tests before merge.

📍 Affects 2 files
  • cmd/chat.go#L50-L82 (this comment)
  • cmd/run.go#L53-L88
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cmd/chat.go` around lines 50 - 82, Add focused direct tests in
cmd/chat_test.go for the chat command’s RunE, covering experimental gating,
agent.CheckEnabled behavior, and complete flag-to-agent.ChatOptions mapping; add
cmd/run_test.go for run’s RunE, specifically verifying exit-code handling. Cover
both successful and relevant failure paths while isolating command dependencies.

Source: Coding guidelines

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Valid request. The bridge layer (internal/agent/) has 10 unit tests covering experimental gating, flag-to-options mapping, and argv generation. The RunE paths in cmd/chat.go/cmd/run.go directly call agent.RunChat/agent.RunHeadless, which launch the full TUI/agent loop — unit-testing those requires mocking tui.Run/native.Run, which are in the harness package (not stubbable without an interface). Deferring cmd/*_test.go to a follow-up PR.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@vishnukv-facets, understood. The bridge tests provide useful coverage for the mapping and gate; direct cmd-level tests would need an injected runner interface/function seam rather than invoking the harness TUI/native loop.

The finding remains valid but can be deferred. Would you like me to open a GitHub follow-up issue for adding that seam and focused cmd/chat_test.go / cmd/run_test.go coverage?


✏️ Learnings added
Learnt from: vishnukv-facets
Repo: Facets-cloud/praxis-cli PR: 65
File: cmd/chat.go:50-82
Timestamp: 2026-07-29T18:16:10.497Z
Learning: In `praxis-cli`, `cmd/chat.go` and `cmd/run.go` directly invoke `internal/agent.RunChat` and `internal/agent.RunHeadless`, which enter the `praxis-harness` TUI/native agent loops. Command-level unit tests require an injectable runner seam because the harness runners are not currently stubbable. `internal/agent/agent_test.go` covers experimental gating, option-to-argument mapping, and argument generation.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

Comment thread cmd/run.go
Comment on lines +24 to +30
runResultJSON bool
runUsageJSON bool
runNoMCP bool
runMcpConfig string
runSettings string
runMaxTurns int
runMaxTokenBudget int

@coderabbitai coderabbitai Bot Jul 29, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

No --json/TTY auto-detection for machine consumers, as required by guidelines.

run is explicitly designed for "scripting, CI, and automation pipelines" (Line 38) and exposes --result-json/--usage-json, but there's no unified --json flag and no logic to auto-emit JSON when os.Stdout isn't a TTY. AI hosts spawning praxis run as a subprocess without passing the JSON flags explicitly won't get parseable output.

As per coding guidelines, "If a command has parseable JSON output, support --json and auto-emit JSON when os.Stdout is not a TTY so AI hosts spawning praxis as a subprocess always get parseable output."

Also applies to: 91-108

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cmd/run.go` around lines 24 - 30, Update the run command’s option definitions
and output-selection logic to add a unified --json flag and automatically select
parseable JSON output when os.Stdout is not a TTY. Integrate this behavior with
the existing runResultJSON and runUsageJSON paths so explicit flags remain
supported while machine consumers receive JSON by default.

Source: Coding guidelines

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Valid per the repo guidelines. --result-json already covers the explicit machine-consumer case. Adding a unified --json flag + non-TTY auto-detection is a good follow-up enhancement, but out of scope for this integration PR (the harness's native.Run owns the JSON encoding logic — wiring auto-detection through the cobra layer requires plumbing a TTY check into the bridge). Tracked for the next iteration.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@vishnukv-facets, understood. The finding remains valid but is appropriately deferred from this integration PR given the harness-owned output path. Would you like me to open a GitHub follow-up issue for unified --json support and non-TTY JSON auto-selection?

Comment thread cmd/run.go Outdated
Comment thread go.mod Outdated
Comment thread internal/agent/agent_test.go Outdated
Comment thread internal/agent/agent_test.go
Comment thread internal/agent/agent_test.go Outdated
Comment thread internal/agent/agent.go
Switches from SSH deploy key to a fine-grained PAT (CI_GITHUB_TOKEN secret)
for private module access. Simpler setup — no SSH keys to manage.

All three workflows (ci build, goreleaser-check, release) now:
1. Set GOPRIVATE + GONOSUMDB for Facets-cloud/*
2. Configure git insteadOf to embed the PAT in HTTPS URLs

go.mod: replace directive removed; requires praxis-harness v0.1.0.
- Fix single-dash flag examples in help text to double-dash (--prompt etc.)
- Use t.Setenv instead of manual os.Getenv/Setenv cleanup (preserves
  original unset state, auto-restores on test completion)
- Assert errors.Is(err, ErrExperimentalDisabled) not just nil check
- Strengthen HeadlessArgs.ToNativeArgs test to verify flag values not
  just presence
- go.mod replace directive already removed in prior commit (now v0.1.0)
@vishnukv-facets
vishnukv-facets marked this pull request as ready for review July 29, 2026 18:18

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
internal/agent/agent_test.go (1)

77-117: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Cover the NoMCP=false branch.

The test only proves that NoMCP=true emits -no-mcp; an implementation that always appends the flag would still pass and disable MCP unexpectedly.

Proposed test
+	for _, tt := range []struct {
+		name    string
+		noMCP   bool
+		wantFlag bool
+	}{
+		{"enabled", true, true},
+		{"disabled", false, false},
+	} {
+		t.Run(tt.name, func(t *testing.T) {
+			args := (HeadlessArgs{NoMCP: tt.noMCP}).ToNativeArgs()
+			gotFlag := false
+			for _, arg := range args {
+				gotFlag = gotFlag || arg == "-no-mcp"
+			}
+			if gotFlag != tt.wantFlag {
+				t.Fatalf("ToNativeArgs() -no-mcp presence = %v, want %v; args = %v", gotFlag, tt.wantFlag, args)
+			}
+		})
+	}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/agent/agent_test.go` around lines 77 - 117, Extend
TestHeadlessArgsToNativeArgs to also validate the NoMCP=false case: create or
update HeadlessArgs with NoMCP disabled, call ToNativeArgs, and assert that
-no-mcp is absent. Preserve the existing assertions proving the flag is emitted
when NoMCP is true.

Source: Coding guidelines

🧹 Nitpick comments (1)
internal/agent/agent_test.go (1)

8-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use a table-driven enablement test.

These three cases duplicate the same setup and assertion; consolidate "", "1", and "true" into one table-driven test.

Proposed refactor
-func TestEnabledDefaultOff(t *testing.T) {
-	t.Setenv(experimentalEnvVar, "")
-	if Enabled() {
-		t.Fatal("Enabled() = true, want false when PRAXIS_EXPERIMENTAL is unset")
-	}
-}
-
-func TestEnabledEnvVar(t *testing.T) {
-	t.Setenv(experimentalEnvVar, "1")
-	if !Enabled() {
-		t.Fatal("Enabled() = false, want true when PRAXIS_EXPERIMENTAL=1")
-	}
-}
-
-func TestEnabledEnvVarTrue(t *testing.T) {
-	t.Setenv(experimentalEnvVar, "true")
-	if !Enabled() {
-		t.Fatal("Enabled() = false, want true when PRAXIS_EXPERIMENTAL=true")
-	}
+func TestEnabled(t *testing.T) {
+	tests := []struct {
+		name string
+		env  string
+		want bool
+	}{
+		{"default off", "", false},
+		{"one enables", "1", true},
+		{"true enables", "true", true},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Setenv(experimentalEnvVar, tt.env)
+			if got := Enabled(); got != tt.want {
+				t.Fatalf("Enabled() = %v, want %v", got, tt.want)
+			}
+		})
+	}
 }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/agent/agent_test.go` around lines 8 - 27, Consolidate
TestEnabledDefaultOff, TestEnabledEnvVar, and TestEnabledEnvVarTrue into one
table-driven enablement test covering "", "1", and "true" with their expected
results. Set experimentalEnvVar and assert Enabled() for each table entry,
preserving the existing test behavior and failure reporting.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@internal/agent/agent_test.go`:
- Around line 77-117: Extend TestHeadlessArgsToNativeArgs to also validate the
NoMCP=false case: create or update HeadlessArgs with NoMCP disabled, call
ToNativeArgs, and assert that -no-mcp is absent. Preserve the existing
assertions proving the flag is emitted when NoMCP is true.

---

Nitpick comments:
In `@internal/agent/agent_test.go`:
- Around line 8-27: Consolidate TestEnabledDefaultOff, TestEnabledEnvVar, and
TestEnabledEnvVarTrue into one table-driven enablement test covering "", "1",
and "true" with their expected results. Set experimentalEnvVar and assert
Enabled() for each table entry, preserving the existing test behavior and
failure reporting.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 200b3c0d-1f92-4265-87b1-fc5ce02cf2aa

📥 Commits

Reviewing files that changed from the base of the PR and between 5ed5818 and 9cdb3d4.

⛔ Files ignored due to path filters (1)
  • go.sum is excluded by !**/*.sum
📒 Files selected for processing (5)
  • .github/workflows/ci.yml
  • .github/workflows/release.yml
  • cmd/run.go
  • go.mod
  • internal/agent/agent_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
  • cmd/run.go
  • .github/workflows/ci.yml

…harness v0.1.1

praxis-cli has its own 'praxis agents' command (lists installed agent
files; called with --json by skills praxis installs into AI hosts), so
the harness session dashboard cannot share that verb. Expose it as a
start view of the agent command instead: 'praxis chat --agents'.

internal/agent.ChatOptions.AgentsView emits a LEADING positional
'agents' into tui.ParseFlags, which only honors it as args[0] — any
other position and the TUI silently opens a normal chat session.
MarkFlagsMutuallyExclusive rejects --agents combined with --prompt /
--resume / --session-id, since tui.loadDashboardApplication clears
exactly those per row.

Bump praxis-harness v0.1.0 -> v0.1.1 for the dashboard fixes that make
this flag usable: multiline/ANSI/OSC session titles are flattened and
sanitized at the label source, the dashboard body is clamped to its
reserved height in physical lines (composer stays visible), and header
counts share one partition with the group headers.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@cmd/chat.go`:
- Around line 112-117: Replace the MarkFlagsMutuallyExclusive checks in the chat
command with conflicts gated on the chatAgents value, so only enabled agents
mode conflicts with prompt, resume, or session-id; explicitly passing
--agents=false must allow the single-session path. Add a regression test
covering --agents=false together with --prompt.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 5fe04382-db5a-4d05-ab74-39053a4b9f67

📥 Commits

Reviewing files that changed from the base of the PR and between 9cdb3d4 and d4266b7.

⛔ Files ignored due to path filters (1)
  • go.sum is excluded by !**/*.sum
📒 Files selected for processing (5)
  • cmd/chat.go
  • cmd/chat_test.go
  • go.mod
  • internal/agent/agent.go
  • internal/agent/agent_test.go

Comment thread cmd/chat.go
Comment on lines +112 to +117
// The dashboard owns session identity: it picks the row to open and clears the
// startup prompt and resume path (harness tui.loadDashboardApplication). Refuse
// the combinations it would silently drop rather than ignoring the user's input.
chatCmd.MarkFlagsMutuallyExclusive("agents", "prompt")
chatCmd.MarkFlagsMutuallyExclusive("agents", "resume")
chatCmd.MarkFlagsMutuallyExclusive("agents", "session-id")

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

set -euo pipefail

printf '\n== Files ==\n'
git ls-files cmd/chat.go cmd/chat_test.go

printf '\n== Relevant excerpt from cmd/chat.go ==\n'
cat -n cmd/chat.go | sed -n '90,150p'

printf '\n== Relevant excerpt from cmd/chat_test.go ==\n'
cat -n cmd/chat_test.go | sed -n '1,260p'

printf '\n== Search for --agents and mutual-exclusion usage ==\n'
rg -n "MarkFlagsMutuallyExclusive|agents=false|--agents|chatAgents|prompt" cmd/chat.go cmd/chat_test.go

Repository: Facets-cloud/praxis-cli

Length of output: 8321


🌐 Web query:

Cobra v1.10.2 flag_groups.go Flag.Changed mutually exclusive bool flag false conflict

💡 Result:

In Cobra v1.10.2, the logic for mutually exclusive flags relies on tracking which flags have been set [1]. When using cmd.MarkFlagsMutuallyExclusive(...), Cobra internally monitors the Changed status of the flags within the group [1][2]. The Flag.Changed property (from the underlying pflag library) tracks whether a flag was explicitly set during parsing [3][4]. Once a flag's Changed status is true, it remains in that state for the duration of the command's execution [3][4]. If you are encountering conflicts where Cobra incorrectly reports a violation of mutual exclusivity during testing (even when flags are set in separate test runs), it is typically because the pflag.FlagSet is not being reset between tests [5]. Because Changed is true, the validation logic still perceives the flag as being "set" [5]. To resolve this conflict in your tests: 1. Ensure you are resetting the command's flag state between test cases [5]. 2. You can manually reset or re-initialize the FlagSet or the command object for each test to clear the Changed state [5]. 3. For programmatic testing, you can explicitly set the Changed field to false for flags if you need to reuse the same flag set, though re-instantiating the command/flagset is the recommended, cleaner approach [5]. Validation logic in flag_groups.go checks these Changed statuses to enforce exclusivity [1][2]. If more than one flag in a mutually exclusive group has Changed == true, Cobra will trigger an error [1][6][7].

Citations:


Allow explicitly disabling --agents. MarkFlagsMutuallyExclusive treats an explicitly passed --agents=false as set, so praxis chat --agents=false --prompt ... still errors even though it should follow the single-session path. Gate the conflict on chatAgents instead and add a regression test for --agents=false + --prompt.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cmd/chat.go` around lines 112 - 117, Replace the MarkFlagsMutuallyExclusive
checks in the chat command with conflicts gated on the chatAgents value, so only
enabled agents mode conflicts with prompt, resume, or session-id; explicitly
passing --agents=false must allow the single-session path. Add a regression test
covering --agents=false together with --prompt.

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.

1 participant